* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Elijah Newren @ 2023-11-14 2:50 UTC (permalink / raw)
To: Taylor Blau
Cc: Jeff King, git, Eric W. Biederman, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <ZVKkgpiFaOwwDcdw@nand.local>
On Mon, Nov 13, 2023 at 2:34 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Mon, Nov 13, 2023 at 05:02:54PM -0500, Jeff King wrote:
> > On Fri, Nov 10, 2023 at 03:51:18PM -0800, Elijah Newren wrote:
> >
> > > This is unsafe; the object may need to be read later within the same
> > > merge. [...]
> > >
> > > I think (untested) that you could fix this by creating two packs
> > > instead of just one. In particular, add a call to
> > > flush_odb_transcation() after the "redo_after_renames" block in
> > > merge_ort_nonrecursive_internal().
> >
> > It might not be too hard to just let in-process callers access the
> > objects we've written. A quick and dirty patch is below, which works
> > with the test you suggested (the test still fails because it finds a
> > conflict, but it gets past the "woah, I can't find that sha1" part).
>
> That's a very slick idea, and I think that this series has some legs to
> stand on now as a result.
>
> There are a few of interesting conclusions we can draw here:
>
> 1. This solves the recursive merge problem that Elijah mentioned
> earlier where we could generate up to 2^N packs (where N is the
> maximum depth of the recursive merge).
>
> 2. This also solves the case where the merge-ort code needs to read an
> object that it wrote earlier on in the same process without having
> to flush out intermediate packs. So in the best case we need only a
> single pack (instead of two).
>
> 3. This also solves the 'replay' issue, *and* allows us to avoid the
> tmp-objdir thing there entirely, since we can simulate object reads
> in the bulk-checkin pack.
>
> So I think that this is a direction worth pursuing.
Agreed; this looks great. It's basically bringing fast-import-like
functionality -- writing objects to a single new packfile while making
previous objects accessible to subsequent ones -- to additional
callers.
> In terms of making those lookups faster, I think that what you'd want is
> an oidmap. The overhead is slightly unfortunate, but I think that any
> other solution which requires keeping the written array in sorted order
> would in practice be more expensive as you have to constantly reallocate
> and copy portions of the array around to maintain its invariant.
When comparing the overhead of an oidmap to a bunch of inodes,
however, it seems relatively cheap. :-)
> > I don't know if that is sufficient, though. Would other spawned
> > processes (hooks, external merge drivers, and so on) need to be able to
> > see these objects, too?
>
> Interesting point. In theory those processes could ask about newly
> created objects, and if they were spawned before the bulk-checkin pack
> was flushed, those lookups would fail.
One of the big design differences that I was pushing really hard with
git-replay was performance and things that came with it -- no
worktree, no per-commit hooks (which are nearly ruled out by no
worktree, but it's still worth calling out separately), etc. A
post-operation hook could be fine, but would also not get to assume a
worktree.
merge-tree is the same as far as hooks; I'd rather just not have them,
but if we did, they'd be a post-operation hook.
In both cases, that makes hooks not much of a sticking point.
External merge drivers, however...
> But that requires that, e.g. for hooks, that we know a-priori the object
> ID of some newly-written objects. If you wanted to make those lookups
> succeed, I think there are a couple of options:
>
> - teach child-processes about the bulk-checkin pack, and let them
> perform the same fake lookup
>
> - flush (but do not close) the bulk-checkin transaction
>
> In any event, I think that this is a sufficiently rare and niche case
> that we'd be OK to declare that you should not expect the above
> scenarios to work when using `--write-pack`. If someone does ask for
> that feature in the future, we could implement it relatively painlessly
> using one of the above options.
Seems reasonable to me.
^ permalink raw reply
* Re: [PATCH 1/1] attr: add native file mode values support
From: Junio C Hamano @ 2023-11-14 2:52 UTC (permalink / raw)
To: Joanna Wang; +Cc: git, tboegi
In-Reply-To: <CAMmZTi-7=L6XOB9-MwZCpD5QaziTi0x6J7m-UTtwQ0S00wnVgQ@mail.gmail.com>
Joanna Wang <jojwang@google.com> writes:
>>> Even if we assume that this code is currently meant to work only
>>> with GIT_ATTR_CHECKIN, I do not think this is what you want. When
>>> asked to perform "git add . ':(exclude,mode=160000)'", you not only
>>> want to exclude the submodules that are already known to this
>>> superproject, but also a repository that _can_ become a submodule of
>>> this superproject when added, no?
> Sorry, I was totally ignorant of ...
Nothing to be sorry about, and if I sounded like I was upset or
something, that wasn't my intention. Reviewers and more experienced
developers read posted patches to give pieces of advice exactly
because no single human is perfect and knows everything. We cover
holes in each others' knowledge and attention.
>>> On the other hand, the GIT_ATTR_CHECKOUT direction is hopefully much
>>> simpler. You'd see what the path in the index is, among a gitlink,
>>> a regular non-executable file, an executable file, or a symlink.
> I noticed for both the GIT_ATTR_CHECKOUT and GIT_ATTR_CHECKIN directions,
> in read_attr(), the indexed .gitattributes file is checked with the
> actual file as fallback or vice versa.
> I would think that we'd only want to use attributes from one state
> (e.g. what's actually in the file)
> or the other (e.g. what's indexed).
> So I guess I'm still not sure what the "direction" concept is.
> For GIT_ATTR_CHECKOUT, would we want to fallback to lstat?
When checking things out of the index, the index should be the
source of the truth. If something is not in the index, is there a
point in falling back to the workint tree state to decide if the
thing should be checked out of the index?
>>> But stepping back a bit,
>>> such an application is likely marking selected few paths with the
>>> attribute, and paths for which "mode" was "unset" are now given
>>> these natural "mode"; it is inevitable to crash with such uses.
> I'm confused. This does not match what I think is the current behavior
> of my patch.
> If "mode" was unset or removed for a path (meaning '<path> !mode' was
> added to .gitattributes),
> the code in my patch would respect that and not return the native mode.
> It would return 'unspecified' or 'unset'.
But the usual practice is *not* to cover all paths with explicit
attribute definition, isn't it? If somebody used the "foo"
attribute in their project to decide certain paths are worth giving
special treatments, there are paths with that attribute, perhaps
(totally made up example):
*.c foo=yes
Now, if you add a new "built-in" attribute next to 'mode' that
assigns "foo" in attr.c:git_check_attr() the same way to those paths
whose value is still ATTR__UNKNOWN after collect_some_attrs returns,
wouldn't a "hello.c" file get attribute 'foo' with value 'yes',
while a "hello.h" file (not mentioned by .gitattributes) will get
whatever value the built-in logic computed for it? If that existing
project were using "mode" (instead of "foo"), then doesn't this patch
change the behaviour for them?
>>> Again, this has one hole, I think. Paths that are not mentioned
>>> (not even with "!mode") would come to the function as ATTR__UNKNOWN
>>> and trigger the fallback behaviour, even when other paths are given
>>> end-user specified "mode" attribute values.
> What you are describing sounds correct/what I intended.
> So are you saying that the expected behavior is actually:
> If the user sets 'mode' for 1+ paths in the repo, then the native mode
> fallback should
> NOT be used for all paths in the repo?
That might be workable, but it breaks a well working system suddenly
when somebody uses "mode" in their .gitattibutes and we do not even
warn about the breakage, which is not ideal.
I think, when we see such an attribute is defined while reading from
.gitattributes and friends, we would probably want to warn and
ignore their definition altogether and use *only* the computed
value. This cannot be done in a backward compatible way, so it
would be better to carve out a namespace (and avoid overly generic
word like "mode") for ourselves to define built-in attributes that
do not overlap with end-user defined attribute names.
Perhaps call this 'builtin-object-mode' or something and document
that any attribute with a name that begins with 'builtin-' will
always get a computed value (possibly "unset"), it is an error
to define such an attribute in .gitattributes system, or something?
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Elijah Newren @ 2023-11-14 2:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Taylor Blau, git, Eric W. Biederman,
Patrick Steinhardt
In-Reply-To: <xmqqpm0d1591.fsf@gitster.g>
On Mon, Nov 13, 2023 at 5:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> > I posted an alternative in response to Elijah; the general idea being to
> > allow the usual object-lookup code to access the in-progress pack. That
> > would keep us limited to a single pack.
>
> If such a mechanism is done in a generic way, would we be able to
> simplify fast-import a lot, I wonder? IIRC, it had quite a lot of
> code to remember what it has written to its output to work around
> the exact issue your alternative tries to solve. In fact, maybe we
> could make fast-import a thin wrapper around the bulk checkin
> infrastructure?
fast-import also attempts to delta objects against previous ones as it
writes them to the pack. That's one thing still lacking from this
solution, but aside from that, it also sounds to me like the bulk
checkin infrastructure is getting closer to becoming a replacement for
much of the fast-import code.
^ permalink raw reply
* Re: [PATCH v3 2/3] update-index: add --show-index-version
From: Junio C Hamano @ 2023-11-14 2:55 UTC (permalink / raw)
To: Teng Long; +Cc: git
In-Reply-To: <20231114021839.8275-1-tenglong.tl@alibaba-inc.com>
Teng Long <dyroneteng@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> @@ -1181,15 +1183,20 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>>
>> getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;
>> if (preferred_index_format) {
>> - if (preferred_index_format < INDEX_FORMAT_LB ||
>> - INDEX_FORMAT_UB < preferred_index_format)
>> + if (preferred_index_format < 0) {
>> + printf(_("%d\n"), the_index.version);
>
> Maybe the "%d\n" shouldn't be translated? :)
Excellent.
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Elijah Newren @ 2023-11-14 3:08 UTC (permalink / raw)
To: Jeff King
Cc: Taylor Blau, git, Eric W. Biederman, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <20231113220546.GB2065691@coredump.intra.peff.net>
On Mon, Nov 13, 2023 at 2:05 PM Jeff King <peff@peff.net> wrote:
>
> On Fri, Nov 10, 2023 at 08:24:44PM -0500, Taylor Blau wrote:
>
> > > This does mean that for a recursive merge, that you'll get up to 2*N
> > > packfiles, where N is the depth of the recursive merge.
> >
> > We definitely want to avoid that ;-). I think there are a couple of
> > potential directions forward here, but the most promising one I think is
> > due to Johannes who suggests that we write loose objects into a
> > temporary directory with a replace_tmp_objdir() call, and then repack
> > that side directory before migrating a single pack back into the main
> > object store.
>
> I posted an alternative in response to Elijah; the general idea being to
> allow the usual object-lookup code to access the in-progress pack. That
> would keep us limited to a single pack.
>
> It _might_ be a terrible idea. E.g., if you write a non-bulk object that
> references a bulk one, then that non-bulk one is broken from the
> perspective of other processes (until the bulk checkin is flushed). But
> I think we'd always be writing to one or the other here, never
> interleaving?
I think it sounds like a really cool idea, personally. I also don't
see why any current uses would result in interleaving.
fast-import's created pack has the same kind of restriction...and has
even grown some extensions to work around the need for early access.
In particular, it has a `checkpoint` feature for flushing the current
pack and starting a new one, and also gained the `cat-blob` command
for when folks really wanted to access an object without writing out
the whole packfile. So, _if_ the need for early access arises, we
have some prior art to look to for ways to provide it.
^ permalink raw reply
* Re: [PATCH v2] glossary: add definitions for dereference & peel
From: Junio C Hamano @ 2023-11-14 4:49 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, ps, Kristoffer Haugsbakk, Victoria Dye
In-Reply-To: <pull.1610.v2.git.1699917471769.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> * Removed references to "peeling" a commit; the updated definition
> discusses "peeling" only in the context of tags.
> * Added a cross-link from "dereference" to "peel" (one already existed
> for "peel" to "dereference").
> ...
> +[[def_peel]]peel::
> + The action of recursively <<def_dereference,dereferencing>> a
> + <<def_tag_object,tag object>>.
> +
This was a bit surprising to me as I thought we would say "peel the
tag once" vs "peel the tag repeatedly", but upon inspecting the
existing code, documentation, and messages, we seem to mean by "to
peel" to dereference a tag repeatedly until it no longer is a tag,
which the new entry above exactly is (although "until the non-tag
object is revealed" is missing).
Thanks. Will queue.
^ permalink raw reply
* Re: [PATCH v2] credential/wincred: store oauth_refresh_token
From: Junio C Hamano @ 2023-11-14 7:40 UTC (permalink / raw)
To: M Hickford via GitGitGadget
Cc: git, Jeff King, Taylor Blau, Matthew John Cheetham, M Hickford
In-Reply-To: <pull.1534.v2.git.1699251395093.gitgitgadget@gmail.com>
"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: M Hickford <mirth.hickford@gmail.com>
>
> a5c7656 (credential: new attribute oauth_refresh_token) introduced
> a new confidential credential attribute for helpers to store.
>
> We encode the new attribute in the CredentialBlob, separated by
> newline:
>
> hunter2
> oauth_refresh_token=xyzzy
>
> This is extensible and backwards compatible. The credential protocol
> already assumes that attribute values do not contain newlines.
>
> This fixes test "helper (wincred) gets oauth_refresh_token" when
> t0303-credential-external.sh is run with
> GIT_TEST_CREDENTIAL_HELPER=wincred. This test was added in a5c76569e7
> (credential: new attribute oauth_refresh_token, 2023-04-21).
>
> Alternatives considered: store oauth_refresh_token in a wincred
> attribute. This would be insecure because wincred assumes attribute
> values to be non-confidential.
Earlier, a5c76569 (credential: new attribute oauth_refresh_token,
2023-04-21) built the "git" side support for the token, and taught
credential-cache to store the necessary information. Then 0ce02e2f
(credential/libsecret: store new attributes, 2023-06-16) was written
for libsecret to support the same interface.
And this one adds corresponding support for wincred. Do I
understand what is going on around this patch correctly?
I do not do Windows, but some people on this list certainly do and
would be capable of giving the patch a necessary nudge ;-)
Thanks.
> .../wincred/git-credential-wincred.c | 46 ++++++++++++++++---
> 1 file changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
> index 4cd56c42e24..5c6a7d65d4a 100644
> --- a/contrib/credential/wincred/git-credential-wincred.c
> +++ b/contrib/credential/wincred/git-credential-wincred.c
> @@ -35,7 +35,7 @@ static void *xmalloc(size_t size)
> }
>
> static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
> - *password_expiry_utc;
> + *password_expiry_utc, *oauth_refresh_token;
>
> static void write_item(const char *what, LPCWSTR wbuf, int wlen)
> {
> @@ -140,6 +140,11 @@ static void get_credential(void)
> DWORD num_creds;
> int i;
> CREDENTIAL_ATTRIBUTEW *attr;
> + WCHAR *secret;
> + WCHAR *line;
> + WCHAR *remaining_lines;
> + WCHAR *part;
> + WCHAR *remaining_parts;
>
> if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
> return;
> @@ -149,9 +154,23 @@ static void get_credential(void)
> if (match_cred(creds[i], 0)) {
> write_item("username", creds[i]->UserName,
> creds[i]->UserName ? wcslen(creds[i]->UserName) : 0);
> - write_item("password",
> - (LPCWSTR)creds[i]->CredentialBlob,
> - creds[i]->CredentialBlobSize / sizeof(WCHAR));
> + if (creds[i]->CredentialBlobSize > 0) {
> + secret = xmalloc(creds[i]->CredentialBlobSize);
> + wcsncpy_s(secret, creds[i]->CredentialBlobSize, (LPCWSTR)creds[i]->CredentialBlob, creds[i]->CredentialBlobSize / sizeof(WCHAR));
> + line = wcstok_s(secret, L"\r\n", &remaining_lines);
> + write_item("password", line, line ? wcslen(line) : 0);
> + while(line != NULL) {
> + part = wcstok_s(line, L"=", &remaining_parts);
> + if (!wcscmp(part, L"oauth_refresh_token")) {
> + write_item("oauth_refresh_token", remaining_parts, remaining_parts ? wcslen(remaining_parts) : 0);
> + }
> + line = wcstok_s(NULL, L"\r\n", &remaining_lines);
> + }
> + } else {
> + write_item("password",
> + (LPCWSTR)creds[i]->CredentialBlob,
> + creds[i]->CredentialBlobSize / sizeof(WCHAR));
> + }
> for (int j = 0; j < creds[i]->AttributeCount; j++) {
> attr = creds[i]->Attributes + j;
> if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
> @@ -160,6 +179,7 @@ static void get_credential(void)
> break;
> }
> }
> + free(secret);
> break;
> }
>
> @@ -170,16 +190,26 @@ static void store_credential(void)
> {
> CREDENTIALW cred;
> CREDENTIAL_ATTRIBUTEW expiry_attr;
> + WCHAR *secret;
> + int wlen;
>
> if (!wusername || !password)
> return;
>
> + if (oauth_refresh_token) {
> + wlen = _scwprintf(L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> + secret = xmalloc(sizeof(WCHAR) * wlen);
> + _snwprintf_s(secret, sizeof(WCHAR) * wlen, wlen, L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> + } else {
> + secret = _wcsdup(password);
> + }
> +
> cred.Flags = 0;
> cred.Type = CRED_TYPE_GENERIC;
> cred.TargetName = target;
> cred.Comment = L"saved by git-credential-wincred";
> - cred.CredentialBlobSize = (wcslen(password)) * sizeof(WCHAR);
> - cred.CredentialBlob = (LPVOID)password;
> + cred.CredentialBlobSize = wcslen(secret) * sizeof(WCHAR);
> + cred.CredentialBlob = (LPVOID)_wcsdup(secret);
> cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
> cred.AttributeCount = 0;
> cred.Attributes = NULL;
> @@ -194,6 +224,8 @@ static void store_credential(void)
> cred.TargetAlias = NULL;
> cred.UserName = wusername;
>
> + free(secret);
> +
> if (!CredWriteW(&cred, 0))
> die("CredWrite failed");
> }
> @@ -265,6 +297,8 @@ static void read_credential(void)
> password = utf8_to_utf16_dup(v);
> else if (!strcmp(buf, "password_expiry_utc"))
> password_expiry_utc = utf8_to_utf16_dup(v);
> + else if (!strcmp(buf, "oauth_refresh_token"))
> + oauth_refresh_token = utf8_to_utf16_dup(v);
> /*
> * Ignore other lines; we don't know what they mean, but
> * this future-proofs us when later versions of git do
>
> base-commit: bc5204569f7db44d22477485afd52ea410d83743
^ permalink raw reply
* Re: commit-graph paranoia performance, was Re: [ANNOUNCE] Git v2.43.0-rc1
From: Patrick Steinhardt @ 2023-11-14 8:46 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20231113205538.GA2028092@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2769 bytes --]
On Mon, Nov 13, 2023 at 03:55:38PM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 02:33:54AM +0900, Junio C Hamano wrote:
>
> > * The codepath to traverse the commit-graph learned to notice that a
> > commit is missing (e.g., corrupt repository lost an object), even
> > though it knows something about the commit (like its parents) from
> > what is in commit-graph.
> > (merge 7a5d604443 ps/do-not-trust-commit-graph-blindly-for-existence later to maint).
>
> I happened to be timing "rev-list" for an unrelated topic today, and I
> noticed that this change had a rather large effect. The commit message
> for 7a5d604443 claims a 30% performance regression. But that's when
> using "--topo-order", and actually writing out the result.
>
> Running "rev-list --count" on a copy of linux.git with a fully-built
> commit-graph shows that the run-time doubles:
>
> Benchmark 1: git.v2.42.1 rev-list --count HEAD
> Time (mean ± σ): 658.0 ms ± 5.2 ms [User: 613.5 ms, System: 44.4 ms]
> Range (min … max): 650.2 ms … 666.0 ms 10 runs
>
> Benchmark 2: git.v2.43.0-rc1 rev-list --count HEAD
> Time (mean ± σ): 1.333 s ± 0.019 s [User: 1.263 s, System: 0.069 s]
> Range (min … max): 1.302 s … 1.361 s 10 runs
>
> Summary
> git.v2.42.1 rev-list --count HEAD ran
> 2.03 ± 0.03 times faster than git.v2.43.0-rc1 rev-list --count HEAD
Ah, indeed. I thought I already benchmarked the worst-case behaviour by
simply doing a full graph walk, but of course the performance hit is
even worse when not outputting the commits at all but only counting
them.
> Now in defense of that patch, this particular command is going to be one
> of the most sensitive in terms of percent change, simply because it
> isn't doing much besides walking the commits. And 650ms isn't _that_ big
> in an absolute sense. But it also doesn't quite feel like nothing, even
> tacked onto a command that might otherwise take 1000ms to run.
>
> Should we default GIT_COMMIT_GRAPH_PARANOIA to "0"? Yes, some operations
> might miss a breakage, but that is true of so much of Git. For day to
> day commands we generally assume that the repository is not corrupted,
> and avoid looking at any data we can. Other commands (like "commit-graph
> verify", but maybe others) would probably want to be more careful
> (either by checking this case explicitly, or by enabling the paranoia
> flag themselves).
>
> -Peff
I'd be fine with that as a follow-up change, yes. I agree that in
general we shouldn't see this kind of corruption, and it's good that the
behaviour can be toggled so easily now.
I'm happy to write that patch if you don't plan to.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 0/4] refs: remove virtual `delete_refs()` function
From: Patrick Steinhardt @ 2023-11-14 8:58 UTC (permalink / raw)
To: git; +Cc: David Turner, Han-Wen Nienhuys
[-- Attachment #1: Type: text/plain, Size: 1612 bytes --]
Hi,
this patch series refactors the virtual `delete_refs()` function to
instead be implemented generically via a single reference transaction.
The main intent of this patch series is to reduce complexity that we
have in the reference backends so that it becomes easier to implement
new backends that have the same semantics as others. But at the same
time, benchmarks show that the new generic code is even faster than the
old backend-specific code. This is mostly because we avoid the overhead
of per-reference transactions when deleting many references, but also
because the transactional code in the files backend knows to avoid
rewriting the packed-refs file in case it contains none of the refs that
are to be deleted.
So in the end we have less duplicate code in the files backend, make it
easier to implement new backends, and have faster deletion of many refs
in the files backend.
Patrick
Patrick Steinhardt (4):
t5510: ensure that the packed-refs file needs locking
refs/files: use transactions to delete references
refs: deduplicate code to delete references
refs: remove `delete_refs` callback from backends
refs.c | 48 ++++++++++++++++++++++++++++++++++++++---
refs/debug.c | 15 -------------
refs/files-backend.c | 49 ------------------------------------------
refs/packed-backend.c | 50 -------------------------------------------
refs/refs-internal.h | 3 ---
t/t5510-fetch.sh | 1 +
6 files changed, 46 insertions(+), 120 deletions(-)
base-commit: e0939bec273052b1a8d69db4a3f7c87aaf83e220
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 1/4] t5510: ensure that the packed-refs file needs locking
From: Patrick Steinhardt @ 2023-11-14 8:58 UTC (permalink / raw)
To: git; +Cc: David Turner, Han-Wen Nienhuys
In-Reply-To: <cover.1699951815.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]
One of the tests in t5510 asserts that a `git fetch --prune` detects
failures to prune branches. This is done by locking the packed-refs
file, which would then later lead to a locking issue when Git tries to
rewrite the file to prune the branches from it.
Interestingly though, we do not pack the about-to-be-pruned branch into
the packed-refs file, so it never even contained that branch in the
first place. While this is good enough right now because the pruning
will always lock the file regardless of whether it contains the branch
or not, this is a mere implementation detail. In fact, we're about to
rewrite branch deletions to make use of the ref transaction interface,
which knows to skip rewrites of the packed-refs file in the case where
it does not contain the branches in the first place, and this will break
the test.
Prepare the test for that change by packing the refs before trying to
prune them.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t5510-fetch.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index dcadd56d3a..79592a3b0a 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -169,6 +169,7 @@ test_expect_success REFFILES 'fetch --prune fails to delete branches' '
git clone . prune-fail &&
cd prune-fail &&
git update-ref refs/remotes/origin/extrabranch main &&
+ git pack-refs --all &&
: this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds &&
>.git/packed-refs.new &&
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/4] refs/files: use transactions to delete references
From: Patrick Steinhardt @ 2023-11-14 8:58 UTC (permalink / raw)
To: git; +Cc: David Turner, Han-Wen Nienhuys
In-Reply-To: <cover.1699951815.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7477 bytes --]
In the `files_delete_refs()` callback function of the files backend we
implement deletion of references. This is done in two steps:
1. We lock the packed-refs file and delete all references from it in
a single transaction.
2. We delete all loose references via separate calls to
`refs_delete_ref()`.
These steps essentially duplicate the logic around locking and deletion
order that we already have in the transactional interfaces, where we do
know to lock and evict references from the packed-refs file. Despite the
fact that we duplicate the logic, it's also less efficient than if we
used a single generic transaction:
- The transactional interface knows to skip locking of the packed
refs in case they don't contain any of the refs which are about to
be deleted.
- We end up creating N+1 separate reference transactions, one for
the packed-refs file and N for the individual loose references.
Refactor the code to instead delete references via a single transaction.
As we don't assert the expected old object ID this is equivalent to the
previous behaviour, and we already do the same in the packed-refs
backend.
Despite the fact that the result is simpler to reason about, this change
also results in improved performance. The following benchmarks have been
executed in linux.git:
```
$ hyperfine -n '{rev}, packed={packed} refcount={refcount}' \
-L packed true,false -L refcount 1,1000 -L rev master,pks-ref-store-generic-delete-refs \
--setup 'git -C /home/pks/Development/git switch --detach {rev} && make -C /home/pks/Development/git -j17' \
--prepare 'printf "create refs/heads/new-branch-%d HEAD\n" $(seq {refcount}) | git -C /home/pks/Reproduction/linux.git update-ref --stdin && if test {packed} = true; then git pack-refs --all; fi' \
--warmup=10 \
'/home/pks/Development/git/bin-wrappers/git -C /home/pks/Reproduction/linux.git branch -d new-branch-{1..{refcount}}'
Benchmark 1: master packed=true refcount=1
Time (mean ± σ): 7.8 ms ± 1.6 ms [User: 3.4 ms, System: 4.4 ms]
Range (min … max): 5.5 ms … 11.0 ms 120 runs
Benchmark 2: master packed=false refcount=1
Time (mean ± σ): 7.0 ms ± 1.1 ms [User: 3.2 ms, System: 3.8 ms]
Range (min … max): 5.7 ms … 9.8 ms 180 runs
Benchmark 3: master packed=true refcount=1000
Time (mean ± σ): 283.8 ms ± 5.2 ms [User: 45.7 ms, System: 231.5 ms]
Range (min … max): 276.7 ms … 291.6 ms 10 runs
Benchmark 4: master packed=false refcount=1000
Time (mean ± σ): 284.4 ms ± 5.3 ms [User: 44.2 ms, System: 233.6 ms]
Range (min … max): 277.1 ms … 293.3 ms 10 runs
Benchmark 5: generic-delete-refs packed=true refcount=1
Time (mean ± σ): 6.2 ms ± 1.8 ms [User: 2.3 ms, System: 3.9 ms]
Range (min … max): 4.1 ms … 12.2 ms 142 runs
Benchmark 6: generic-delete-refs packed=false refcount=1
Time (mean ± σ): 7.1 ms ± 1.4 ms [User: 2.8 ms, System: 4.3 ms]
Range (min … max): 4.2 ms … 10.8 ms 157 runs
Benchmark 7: generic-delete-refs packed=true refcount=1000
Time (mean ± σ): 198.9 ms ± 1.7 ms [User: 29.5 ms, System: 165.7 ms]
Range (min … max): 196.1 ms … 201.4 ms 10 runs
Benchmark 8: generic-delete-refs packed=false refcount=1000
Time (mean ± σ): 199.7 ms ± 7.8 ms [User: 32.2 ms, System: 163.2 ms]
Range (min … max): 193.8 ms … 220.7 ms 10 runs
Summary
generic-delete-refs packed=true refcount=1 ran
1.14 ± 0.37 times faster than master packed=false refcount=1
1.15 ± 0.40 times faster than generic-delete-refs packed=false refcount=1
1.26 ± 0.44 times faster than master packed=true refcount=1
32.24 ± 9.17 times faster than generic-delete-refs packed=true refcount=1000
32.36 ± 9.29 times faster than generic-delete-refs packed=false refcount=1000
46.00 ± 13.10 times faster than master packed=true refcount=1000
46.10 ± 13.13 times faster than master packed=false refcount=1000
```
Especially in the case where we have many references we can see a clear
performance speedup of nearly 30%.
This is in contrast to the stated objecive in a27dcf89b68 (refs: make
delete_refs() virtual, 2016-09-04), where the virtual `delete_refs()`
function was introduced with the intent to speed things up rather than
making things slower. So it seems like we have outlived the need for a
virtual function.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs/files-backend.c | 66 +++++++++++++++++++++++---------------------
1 file changed, 34 insertions(+), 32 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index db5c0c7a72..778d3a96ba 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1268,49 +1268,51 @@ static int files_pack_refs(struct ref_store *ref_store,
static int files_delete_refs(struct ref_store *ref_store, const char *msg,
struct string_list *refnames, unsigned int flags)
{
- struct files_ref_store *refs =
- files_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
+ struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
- int i, result = 0;
+ struct string_list_item *item;
+ int ret = 0, failures = 0;
if (!refnames->nr)
return 0;
- if (packed_refs_lock(refs->packed_ref_store, 0, &err))
- goto error;
-
- if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {
- packed_refs_unlock(refs->packed_ref_store);
- goto error;
+ /*
+ * Since we don't check the references' old_oids, the
+ * individual updates can't fail, so we can pack all of the
+ * updates into a single transaction.
+ */
+ transaction = ref_store_transaction_begin(ref_store, &err);
+ if (!transaction) {
+ ret = error("%s", err.buf);
+ goto out;
}
- packed_refs_unlock(refs->packed_ref_store);
-
- for (i = 0; i < refnames->nr; i++) {
- const char *refname = refnames->items[i].string;
-
- if (refs_delete_ref(&refs->base, msg, refname, NULL, flags))
- result |= error(_("could not remove reference %s"), refname);
+ for_each_string_list_item(item, refnames) {
+ ret = ref_transaction_delete(transaction, item->string,
+ NULL, flags, msg, &err);
+ if (ret) {
+ warning(_("could not delete reference %s: %s"),
+ item->string, err.buf);
+ strbuf_reset(&err);
+ failures = 1;
+ }
}
- strbuf_release(&err);
- return result;
-
-error:
- /*
- * If we failed to rewrite the packed-refs file, then it is
- * unsafe to try to remove loose refs, because doing so might
- * expose an obsolete packed value for a reference that might
- * even point at an object that has been garbage collected.
- */
- if (refnames->nr == 1)
- error(_("could not delete reference %s: %s"),
- refnames->items[0].string, err.buf);
- else
- error(_("could not delete references: %s"), err.buf);
+ ret = ref_transaction_commit(transaction, &err);
+ if (ret) {
+ if (refnames->nr == 1)
+ error(_("could not delete reference %s: %s"),
+ refnames->items[0].string, err.buf);
+ else
+ error(_("could not delete references: %s"), err.buf);
+ }
+out:
+ if (!ret && failures)
+ ret = -1;
+ ref_transaction_free(transaction);
strbuf_release(&err);
- return -1;
+ return ret;
}
/*
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/4] refs: deduplicate code to delete references
From: Patrick Steinhardt @ 2023-11-14 8:58 UTC (permalink / raw)
To: git; +Cc: David Turner, Han-Wen Nienhuys
In-Reply-To: <cover.1699951815.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5709 bytes --]
Both the files and the packed-refs reference backends now use the same
generic transactions-based code to delete references. Let's pull these
implementations up into `refs_delete_refs()` to deduplicate the code.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 48 ++++++++++++++++++++++++++++++++++++++++---
refs/files-backend.c | 46 +----------------------------------------
refs/packed-backend.c | 45 +---------------------------------------
3 files changed, 47 insertions(+), 92 deletions(-)
diff --git a/refs.c b/refs.c
index fcae5dddc6..16bfa21df7 100644
--- a/refs.c
+++ b/refs.c
@@ -2599,13 +2599,55 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,
int refs_delete_refs(struct ref_store *refs, const char *logmsg,
struct string_list *refnames, unsigned int flags)
{
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
+ struct string_list_item *item;
+ int ret = 0, failures = 0;
char *msg;
- int retval;
+
+ if (!refnames->nr)
+ return 0;
msg = normalize_reflog_message(logmsg);
- retval = refs->be->delete_refs(refs, msg, refnames, flags);
+
+ /*
+ * Since we don't check the references' old_oids, the
+ * individual updates can't fail, so we can pack all of the
+ * updates into a single transaction.
+ */
+ transaction = ref_store_transaction_begin(refs, &err);
+ if (!transaction) {
+ ret = error("%s", err.buf);
+ goto out;
+ }
+
+ for_each_string_list_item(item, refnames) {
+ ret = ref_transaction_delete(transaction, item->string,
+ NULL, flags, msg, &err);
+ if (ret) {
+ warning(_("could not delete reference %s: %s"),
+ item->string, err.buf);
+ strbuf_reset(&err);
+ failures = 1;
+ }
+ }
+
+ ret = ref_transaction_commit(transaction, &err);
+ if (ret) {
+ if (refnames->nr == 1)
+ error(_("could not delete reference %s: %s"),
+ refnames->items[0].string, err.buf);
+ else
+ error(_("could not delete references: %s"), err.buf);
+ }
+
+out:
+ if (!ret && failures)
+ ret = -1;
+ ref_transaction_free(transaction);
+ strbuf_release(&err);
free(msg);
- return retval;
+ return ret;
}
int delete_refs(const char *msg, struct string_list *refnames,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 778d3a96ba..8d28810e67 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1268,51 +1268,7 @@ static int files_pack_refs(struct ref_store *ref_store,
static int files_delete_refs(struct ref_store *ref_store, const char *msg,
struct string_list *refnames, unsigned int flags)
{
- struct ref_transaction *transaction;
- struct strbuf err = STRBUF_INIT;
- struct string_list_item *item;
- int ret = 0, failures = 0;
-
- if (!refnames->nr)
- return 0;
-
- /*
- * Since we don't check the references' old_oids, the
- * individual updates can't fail, so we can pack all of the
- * updates into a single transaction.
- */
- transaction = ref_store_transaction_begin(ref_store, &err);
- if (!transaction) {
- ret = error("%s", err.buf);
- goto out;
- }
-
- for_each_string_list_item(item, refnames) {
- ret = ref_transaction_delete(transaction, item->string,
- NULL, flags, msg, &err);
- if (ret) {
- warning(_("could not delete reference %s: %s"),
- item->string, err.buf);
- strbuf_reset(&err);
- failures = 1;
- }
- }
-
- ret = ref_transaction_commit(transaction, &err);
- if (ret) {
- if (refnames->nr == 1)
- error(_("could not delete reference %s: %s"),
- refnames->items[0].string, err.buf);
- else
- error(_("could not delete references: %s"), err.buf);
- }
-
-out:
- if (!ret && failures)
- ret = -1;
- ref_transaction_free(transaction);
- strbuf_release(&err);
- return ret;
+ return refs_delete_refs(ref_store, msg, refnames, flags);
}
/*
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 59c78d7941..1589577005 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1691,50 +1691,7 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store UNUSED,
static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
struct string_list *refnames, unsigned int flags)
{
- struct packed_ref_store *refs =
- packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
- struct strbuf err = STRBUF_INIT;
- struct ref_transaction *transaction;
- struct string_list_item *item;
- int ret;
-
- (void)refs; /* We need the check above, but don't use the variable */
-
- if (!refnames->nr)
- return 0;
-
- /*
- * Since we don't check the references' old_oids, the
- * individual updates can't fail, so we can pack all of the
- * updates into a single transaction.
- */
-
- transaction = ref_store_transaction_begin(ref_store, &err);
- if (!transaction)
- return -1;
-
- for_each_string_list_item(item, refnames) {
- if (ref_transaction_delete(transaction, item->string, NULL,
- flags, msg, &err)) {
- warning(_("could not delete reference %s: %s"),
- item->string, err.buf);
- strbuf_reset(&err);
- }
- }
-
- ret = ref_transaction_commit(transaction, &err);
-
- if (ret) {
- if (refnames->nr == 1)
- error(_("could not delete reference %s: %s"),
- refnames->items[0].string, err.buf);
- else
- error(_("could not delete references: %s"), err.buf);
- }
-
- ref_transaction_free(transaction);
- strbuf_release(&err);
- return ret;
+ return refs_delete_refs(ref_store, msg, refnames, flags);
}
static int packed_pack_refs(struct ref_store *ref_store UNUSED,
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 4/4] refs: remove `delete_refs` callback from backends
From: Patrick Steinhardt @ 2023-11-14 8:58 UTC (permalink / raw)
To: git; +Cc: David Turner, Han-Wen Nienhuys
In-Reply-To: <cover.1699951815.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4122 bytes --]
Now that `refs_delete_refs` is implemented in a generic way via the ref
transaction interfaces there are no callers left that invoke the
`delete_refs` callback anymore. Remove it from all of our backends.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs/debug.c | 15 ---------------
refs/files-backend.c | 7 -------
refs/packed-backend.c | 7 -------
refs/refs-internal.h | 3 ---
4 files changed, 32 deletions(-)
diff --git a/refs/debug.c b/refs/debug.c
index b7ffc4ce67..83b7a0ba65 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -143,20 +143,6 @@ static int debug_create_symref(struct ref_store *ref_store,
return res;
}
-static int debug_delete_refs(struct ref_store *ref_store, const char *msg,
- struct string_list *refnames, unsigned int flags)
-{
- struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
- int res =
- drefs->refs->be->delete_refs(drefs->refs, msg, refnames, flags);
- int i;
- trace_printf_key(&trace_refs, "delete_refs {\n");
- for (i = 0; i < refnames->nr; i++)
- trace_printf_key(&trace_refs, "%s\n", refnames->items[i].string);
- trace_printf_key(&trace_refs, "}: %d\n", res);
- return res;
-}
-
static int debug_rename_ref(struct ref_store *ref_store, const char *oldref,
const char *newref, const char *logmsg)
{
@@ -458,7 +444,6 @@ struct ref_storage_be refs_be_debug = {
.pack_refs = debug_pack_refs,
.create_symref = debug_create_symref,
- .delete_refs = debug_delete_refs,
.rename_ref = debug_rename_ref,
.copy_ref = debug_copy_ref,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8d28810e67..ad8b1d143f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1265,12 +1265,6 @@ static int files_pack_refs(struct ref_store *ref_store,
return 0;
}
-static int files_delete_refs(struct ref_store *ref_store, const char *msg,
- struct string_list *refnames, unsigned int flags)
-{
- return refs_delete_refs(ref_store, msg, refnames, flags);
-}
-
/*
* People using contrib's git-new-workdir have .git/logs/refs ->
* /some/other/path/.git/logs/refs, and that may live on another device.
@@ -3258,7 +3252,6 @@ struct ref_storage_be refs_be_files = {
.pack_refs = files_pack_refs,
.create_symref = files_create_symref,
- .delete_refs = files_delete_refs,
.rename_ref = files_rename_ref,
.copy_ref = files_copy_ref,
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 1589577005..b9fa097a29 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1688,12 +1688,6 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store UNUSED,
return ref_transaction_commit(transaction, err);
}
-static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
- struct string_list *refnames, unsigned int flags)
-{
- return refs_delete_refs(ref_store, msg, refnames, flags);
-}
-
static int packed_pack_refs(struct ref_store *ref_store UNUSED,
struct pack_refs_opts *pack_opts UNUSED)
{
@@ -1722,7 +1716,6 @@ struct ref_storage_be refs_be_packed = {
.pack_refs = packed_pack_refs,
.create_symref = NULL,
- .delete_refs = packed_delete_refs,
.rename_ref = NULL,
.copy_ref = NULL,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 9db8aec4da..4af83bf9a5 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -553,8 +553,6 @@ typedef int create_symref_fn(struct ref_store *ref_store,
const char *ref_target,
const char *refs_heads_master,
const char *logmsg);
-typedef int delete_refs_fn(struct ref_store *ref_store, const char *msg,
- struct string_list *refnames, unsigned int flags);
typedef int rename_ref_fn(struct ref_store *ref_store,
const char *oldref, const char *newref,
const char *logmsg);
@@ -677,7 +675,6 @@ struct ref_storage_be {
pack_refs_fn *pack_refs;
create_symref_fn *create_symref;
- delete_refs_fn *delete_refs;
rename_ref_fn *rename_ref;
copy_ref_fn *copy_ref;
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: commit-graph paranoia performance, was Re: [ANNOUNCE] Git v2.43.0-rc1
From: Jeff King @ 2023-11-14 9:05 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Junio C Hamano, git
In-Reply-To: <ZVMz4iDWfC__H8Jp@tanuki>
On Tue, Nov 14, 2023 at 09:46:26AM +0100, Patrick Steinhardt wrote:
> > Should we default GIT_COMMIT_GRAPH_PARANOIA to "0"? Yes, some operations
> > might miss a breakage, but that is true of so much of Git. For day to
> > day commands we generally assume that the repository is not corrupted,
> > and avoid looking at any data we can. Other commands (like "commit-graph
> > verify", but maybe others) would probably want to be more careful
> > (either by checking this case explicitly, or by enabling the paranoia
> > flag themselves).
>
> I'd be fine with that as a follow-up change, yes. I agree that in
> general we shouldn't see this kind of corruption, and it's good that the
> behaviour can be toggled so easily now.
>
> I'm happy to write that patch if you don't plan to.
I hadn't started on it, so please feel free to go ahead.
-Peff
^ permalink raw reply
* Document git status -b --porcelain
From: Ondra Medek @ 2023-11-14 9:14 UTC (permalink / raw)
To: git
Hi,
I am using "git status -b --porcelain" and parsing output in a code.
However, the output is not documented for an empty repository or a
missing remote branch. (And maybe a detached head, too).
Doc for Porcelain Format Version 1
https://git-scm.com/docs/git-status#_porcelain_format_version_1
references doc for Short Format
https://git-scm.com/docs/git-status#_short_format
and it says:
> If -b is used the short-format status is preceded by a line
> ## branchname tracking info
However, when I have a fresh clone or an empty repository, "git status
-b --porcelain" results in:
## No commits yet on master...origin/master [gone]
So, it seems to me the format is something like "# [info1
]branchname...tracking[ info2]" and it should be documented what infos
may appear and when. Otherwise it's hard to write robust parsing of
such output.
Thanks
Ondra Medek
^ permalink raw reply
* Feature request: git status --branch-only
From: Ondra Medek @ 2023-11-14 10:16 UTC (permalink / raw)
To: git
Hello,
I am working on a tol which should fetch changes from a remote
repository on a user click. I want to limit fetch on the current
remote tracking branch (something like "origin/master"), but
surprisingly, it's hard to get it for all corner cases like a fresh
clone of an empty repository or detached head, etc. E.g see this SO
thread https://stackoverflow.com/questions/171550/find-out-which-remote-branch-a-local-branch-is-tracking/52896538
The most reliable way for me is to call
git status -b --no-ahead-behind --porcelain=v2
and parse the "# branch.upstream" line output. However, it is a bit
slow on large repos and yields unused output for me. So, I propose a
new switch "git status --branch-only" which would output branch status
only.
Note: workaround is to specify some non-existing directory, like
git status -b --no-ahead-behind --porcelain=v2 .this-dir-does-not-exists
Thanks
Ondra Medek
^ permalink raw reply
* [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default
From: Patrick Steinhardt @ 2023-11-14 10:23 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Karthik Nayak
[-- Attachment #1: Type: text/plain, Size: 6604 bytes --]
In 7a5d604443 (commit: detect commits that exist in commit-graph but not
in the ODB, 2023-10-31), we have introduced a new object existence check
into `repo_parse_commit_internal()` so that we do not parse commits via
the commit-graph that don't have a corresponding object in the object
database. This new check of course comes with a performance penalty,
which the commit put at around 30% for `git rev-list --topo-order`. But
there are in fact scenarios where the performance regression is even
higher. The following benchmark against linux.git with a fully-build
commit-graph:
Benchmark 1: git.v2.42.1 rev-list --count HEAD
Time (mean ± σ): 658.0 ms ± 5.2 ms [User: 613.5 ms, System: 44.4 ms]
Range (min … max): 650.2 ms … 666.0 ms 10 runs
Benchmark 2: git.v2.43.0-rc1 rev-list --count HEAD
Time (mean ± σ): 1.333 s ± 0.019 s [User: 1.263 s, System: 0.069 s]
Range (min … max): 1.302 s … 1.361 s 10 runs
Summary
git.v2.42.1 rev-list --count HEAD ran
2.03 ± 0.03 times faster than git.v2.43.0-rc1 rev-list --count HEAD
While it's a noble goal to ensure that results are the same regardless
of whether or not we have a potentially stale commit-graph, taking twice
as much time is a tough sell. Furthermore, we can generally assume that
the commit-graph will be updated by git-gc(1) or git-maintenance(1) as
required so that the case where the commit-graph is stale should not at
all be common.
With that in mind, default-disable GIT_COMMIT_GRAPH_PARANOIA and restore
the behaviour and thus performance previous to the mentioned commit. In
order to not be inconsistent, also disable this behaviour by default in
`lookup_commit_in_graph()`, where the object existence check has been
introduced right at its inception via f559d6d45e (revision: avoid
hitting packfiles when commits are in commit-graph, 2021-08-09).
This results in another speedup in commands that end up calling this
function, even though it's less pronounced compared to the above
benchmark. The following has been executed in linux.git with ~1.2
million references:
Benchmark 1: GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted
Time (mean ± σ): 2.947 s ± 0.003 s [User: 2.412 s, System: 0.534 s]
Range (min … max): 2.943 s … 2.949 s 3 runs
Benchmark 2: GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted
Time (mean ± σ): 2.724 s ± 0.030 s [User: 2.207 s, System: 0.514 s]
Range (min … max): 2.704 s … 2.759 s 3 runs
Summary
GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted ran
1.08 ± 0.01 times faster than GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted
So whereas 7a5d604443 initially introduced the logic to start doing an
object existence check in `repo_parse_commit_internal()` by default, the
updated logic will now instead cause `lookup_commit_in_graph()` to stop
doing the check by default. This behaviour continues to be tweakable by
the user via the GIT_COMMIT_GRAPH_PARANOIA environment variable.
Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git.txt | 6 +++---
commit-graph.c | 2 +-
commit.c | 2 +-
t/t5318-commit-graph.sh | 8 ++++----
4 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2535a30194..6c19fd1d76 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -917,9 +917,9 @@ for full details.
avoid issues with stale commit-graphs that contain references to
already-deleted commits, but comes with a performance penalty.
+
-The default is "true", which enables the aforementioned behavior.
-Setting this to "false" disables the existence check. This can lead to
-a performance improvement at the cost of consistency.
+The default is "false", which disables the aforementioned behavior.
+Setting this to "true" enables the existence check so that stale commits
+will never be returned from the commit-graph at the cost of performance.
`GIT_ALLOW_PROTOCOL`::
If set to a colon-separated list of protocols, behave as if
diff --git a/commit-graph.c b/commit-graph.c
index ee66098e07..a712917356 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1029,7 +1029,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje
uint32_t pos;
if (commit_graph_paranoia == -1)
- commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+ commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);
if (!prepare_commit_graph(repo))
return NULL;
diff --git a/commit.c b/commit.c
index 8405d7c3fc..37956b836c 100644
--- a/commit.c
+++ b/commit.c
@@ -577,7 +577,7 @@ int repo_parse_commit_internal(struct repository *r,
static int commit_graph_paranoia = -1;
if (commit_graph_paranoia == -1)
- commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+ commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);
if (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {
unparse_commit(r, &item->object.oid);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index d4fc65e078..4c751a6871 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -909,10 +909,10 @@ test_expect_success 'stale commit cannot be parsed when given directly' '
# Verify that it is possible to read the commit from the
# commit graph when not being paranoid, ...
- GIT_COMMIT_GRAPH_PARANOIA=false git rev-list B &&
+ git rev-list B &&
# ... but parsing the commit when double checking that
# it actually exists in the object database should fail.
- test_must_fail git rev-list -1 B
+ test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-list -1 B
)
'
@@ -936,9 +936,9 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '
# Again, we should be able to parse the commit when not
# being paranoid about commit graph staleness...
- GIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 &&
+ git rev-parse HEAD~2 &&
# ... but fail when we are paranoid.
- test_must_fail git rev-parse HEAD~2 2>error &&
+ test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-parse HEAD~2 2>error &&
grep "error: commit $oid exists in commit-graph but not in the object database" error
)
'
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default
From: Patrick Steinhardt @ 2023-11-14 10:35 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Karthik Nayak
In-Reply-To: <7e2d300c4af9a7853201121d66f982afa421bbba.1699957350.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 8759 bytes --]
On Tue, Nov 14, 2023 at 11:23:05AM +0100, Patrick Steinhardt wrote:
> In 7a5d604443 (commit: detect commits that exist in commit-graph but not
> in the ODB, 2023-10-31), we have introduced a new object existence check
> into `repo_parse_commit_internal()` so that we do not parse commits via
> the commit-graph that don't have a corresponding object in the object
> database. This new check of course comes with a performance penalty,
> which the commit put at around 30% for `git rev-list --topo-order`. But
> there are in fact scenarios where the performance regression is even
> higher. The following benchmark against linux.git with a fully-build
> commit-graph:
>
> Benchmark 1: git.v2.42.1 rev-list --count HEAD
> Time (mean ± σ): 658.0 ms ± 5.2 ms [User: 613.5 ms, System: 44.4 ms]
> Range (min … max): 650.2 ms … 666.0 ms 10 runs
>
> Benchmark 2: git.v2.43.0-rc1 rev-list --count HEAD
> Time (mean ± σ): 1.333 s ± 0.019 s [User: 1.263 s, System: 0.069 s]
> Range (min … max): 1.302 s … 1.361 s 10 runs
>
> Summary
> git.v2.42.1 rev-list --count HEAD ran
> 2.03 ± 0.03 times faster than git.v2.43.0-rc1 rev-list --count HEAD
>
> While it's a noble goal to ensure that results are the same regardless
> of whether or not we have a potentially stale commit-graph, taking twice
> as much time is a tough sell. Furthermore, we can generally assume that
> the commit-graph will be updated by git-gc(1) or git-maintenance(1) as
> required so that the case where the commit-graph is stale should not at
> all be common.
>
> With that in mind, default-disable GIT_COMMIT_GRAPH_PARANOIA and restore
> the behaviour and thus performance previous to the mentioned commit. In
> order to not be inconsistent, also disable this behaviour by default in
> `lookup_commit_in_graph()`, where the object existence check has been
> introduced right at its inception via f559d6d45e (revision: avoid
> hitting packfiles when commits are in commit-graph, 2021-08-09).
>
> This results in another speedup in commands that end up calling this
> function, even though it's less pronounced compared to the above
> benchmark. The following has been executed in linux.git with ~1.2
> million references:
>
> Benchmark 1: GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted
> Time (mean ± σ): 2.947 s ± 0.003 s [User: 2.412 s, System: 0.534 s]
> Range (min … max): 2.943 s … 2.949 s 3 runs
>
> Benchmark 2: GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted
> Time (mean ± σ): 2.724 s ± 0.030 s [User: 2.207 s, System: 0.514 s]
> Range (min … max): 2.704 s … 2.759 s 3 runs
>
> Summary
> GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted ran
> 1.08 ± 0.01 times faster than GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted
>
> So whereas 7a5d604443 initially introduced the logic to start doing an
> object existence check in `repo_parse_commit_internal()` by default, the
> updated logic will now instead cause `lookup_commit_in_graph()` to stop
> doing the check by default. This behaviour continues to be tweakable by
> the user via the GIT_COMMIT_GRAPH_PARANOIA environment variable.
>
> Reported-by: Jeff King <peff@peff.net>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
Gah, I forgot to run this with GIT_TEST_COMMIT_GRAPH=1 before sending
this patch. There are two test failures that this change introduces:
- t6022-rev-list-missing.sh, where we test for the `--missing=` option
of git-rev-list(1).
- t7700-repack.sh, where we also manually delete objects.
Both of these are expected failures: we knowingly corrupt the repository
and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are
updated. If we stick with the new stance that repository corruption
should not require us to pessimize the common case, then we'd have to
squash in something like the below.
Patrick
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
index 40265a4f66..9e3f063d08 100755
--- a/t/t6022-rev-list-missing.sh
+++ b/t/t6022-rev-list-missing.sh
@@ -27,6 +27,12 @@ do
'
done
+# When running with GIT_TEST_COMMIT_GRAPH=true we write a commit-graph, but
+# don't update it before forcefully deleting the commit object. We thus enable
+# GIT_COMMIT_GRAPH_PARANOIA so that this case is detected with such a stale
+# commit-graph.
+export GIT_COMMIT_GRAPH_PARANOIA=true
+
for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
do
for action in "allow-any" "print"
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index d2975e6c93..ca8ad9c420 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -271,7 +271,7 @@ test_expect_success 'repacking fails when missing .pack actually means missing o
ls .git/objects/pack/*.pack >before-pack-dir &&
test_must_fail git fsck &&
- test_must_fail git repack --cruft -d 2>err &&
+ test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=1 git repack --cruft -d 2>err &&
grep "bad object" err &&
# Before failing, the repack did not modify the
> ---
> Documentation/git.txt | 6 +++---
> commit-graph.c | 2 +-
> commit.c | 2 +-
> t/t5318-commit-graph.sh | 8 ++++----
> 4 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 2535a30194..6c19fd1d76 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -917,9 +917,9 @@ for full details.
> avoid issues with stale commit-graphs that contain references to
> already-deleted commits, but comes with a performance penalty.
> +
> -The default is "true", which enables the aforementioned behavior.
> -Setting this to "false" disables the existence check. This can lead to
> -a performance improvement at the cost of consistency.
> +The default is "false", which disables the aforementioned behavior.
> +Setting this to "true" enables the existence check so that stale commits
> +will never be returned from the commit-graph at the cost of performance.
>
> `GIT_ALLOW_PROTOCOL`::
> If set to a colon-separated list of protocols, behave as if
> diff --git a/commit-graph.c b/commit-graph.c
> index ee66098e07..a712917356 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1029,7 +1029,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje
> uint32_t pos;
>
> if (commit_graph_paranoia == -1)
> - commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
> + commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);
>
> if (!prepare_commit_graph(repo))
> return NULL;
> diff --git a/commit.c b/commit.c
> index 8405d7c3fc..37956b836c 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -577,7 +577,7 @@ int repo_parse_commit_internal(struct repository *r,
> static int commit_graph_paranoia = -1;
>
> if (commit_graph_paranoia == -1)
> - commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
> + commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);
>
> if (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {
> unparse_commit(r, &item->object.oid);
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index d4fc65e078..4c751a6871 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -909,10 +909,10 @@ test_expect_success 'stale commit cannot be parsed when given directly' '
>
> # Verify that it is possible to read the commit from the
> # commit graph when not being paranoid, ...
> - GIT_COMMIT_GRAPH_PARANOIA=false git rev-list B &&
> + git rev-list B &&
> # ... but parsing the commit when double checking that
> # it actually exists in the object database should fail.
> - test_must_fail git rev-list -1 B
> + test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-list -1 B
> )
> '
>
> @@ -936,9 +936,9 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '
>
> # Again, we should be able to parse the commit when not
> # being paranoid about commit graph staleness...
> - GIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 &&
> + git rev-parse HEAD~2 &&
> # ... but fail when we are paranoid.
> - test_must_fail git rev-parse HEAD~2 2>error &&
> + test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-parse HEAD~2 2>error &&
> grep "error: commit $oid exists in commit-graph but not in the object database" error
> )
> '
> --
> 2.42.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3] fuzz: add new oss-fuzz fuzzer for date.c / date.h
From: Arthur Chan via GitGitGadget @ 2023-11-14 10:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Arthur Chan, Arthur Chan
In-Reply-To: <pull.1612.v2.git.1699892568344.gitgitgadget@gmail.com>
From: Arthur Chan <arthur.chan@adalogics.com>
Signed-off-by: Arthur Chan <arthur.chan@adalogics.com>
---
fuzz: add new oss-fuzz fuzzer for date.c / date.h
This patch is aimed to add a new oss-fuzz fuzzer to the oss-fuzz
directory for fuzzing date.c / date.h in the base directory.
The .gitignore of the oss-fuzz directory and the Makefile have been
modified to accommodate the new fuzzer fuzz-date.c.
Fixed the objects order in .gitignore and Makefiles and fixed some of
the logic and formatting for the fuzz-date.c fuzzer in v2.
Fixed the creation and memory allocation of the fuzzing str in v3. Also
fixed the tz type and sign-extended the data before passing to the tz
variable.
Comment: Yes, indeed. It is quite annoying to have that twice. Yes, the
tz should be considered as attacker controllable and thus negative
values should be considered. But it is tricky to fuzz it because the
date.c::gm_time_t() will call die() if the value is invalid and that
exit the fuzzer directly. OSS-Fuzz may consider it as an issue (or bug)
because the fuzzer exit "unexpectedly". I agree that if we consider the
tz as "attacker controllable, we should include negative values, but
since it will cause the fuzzer exit, I am not sure if it is the right
approach from the fuzzing perspective. Also, it is something that date.c
already take care of with the conditional checking, thus it may also be
worth to do some checking and exclude some invalid values before calling
date.c::show_date() but this may result in copying some conditional
checking code from date.c.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1612%2Farthurscchan%2Fnew-fuzzer-date-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1612/arthurscchan/new-fuzzer-date-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1612
Range-diff vs v2:
1: 2928e2b858d ! 1: 046bca32889 fuzz: add new oss-fuzz fuzzer for date.c / date.h
@@ oss-fuzz/fuzz-date.c (new)
+{
+ int local;
+ int num;
-+ uint16_t tz;
++ int tz;
+ char *str;
++ int8_t *tmp_data;
+ timestamp_t ts;
+ enum date_mode_type dmtype;
+ struct date_mode *dm;
@@ oss-fuzz/fuzz-date.c (new)
+ return 0;
+
+ local = !!(*data & 0x10);
-+ dmtype = (enum date_mode_type)(*data % DATE_UNIX);
-+ if (dmtype == DATE_STRFTIME)
-+ /*
-+ * Currently DATE_STRFTIME is not supported.
-+ */
-+ return 0;
++ num = *data % DATE_UNIX;
++ if (num >= DATE_STRFTIME)
++ num++;
++ dmtype = (enum date_mode_type)num;
+ data++;
+ size--;
+
-+ tz = *data++;
-+ tz = (tz << 8) | *data++;
-+ tz = (tz << 8) | *data++;
++ tmp_data = (int8_t*)data;
++ tz = *tmp_data++;
++ tz = (tz << 8) | *tmp_data++;
++ tz = (tz << 8) | *tmp_data++;
++ data += 3;
+ size -= 3;
+
-+ str = (char *)malloc(size + 1);
-+ if (!str)
-+ return 0;
-+ memcpy(str, data, size);
-+ str[size] = '\0';
++ str = xmemdupz(data, size);
+
+ ts = approxidate_careful(str, &num);
+ free(str);
+
+ dm = date_mode_from_type(dmtype);
+ dm->local = local;
-+ show_date(ts, (int16_t)tz, dm);
++ show_date(ts, tz, dm);
+
+ date_mode_release(dm);
+
Makefile | 1 +
oss-fuzz/.gitignore | 1 +
oss-fuzz/fuzz-date.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 55 insertions(+)
create mode 100644 oss-fuzz/fuzz-date.c
diff --git a/Makefile b/Makefile
index 03adcb5a480..4b875ef6ce1 100644
--- a/Makefile
+++ b/Makefile
@@ -750,6 +750,7 @@ SCRIPTS = $(SCRIPT_SH_GEN) \
ETAGS_TARGET = TAGS
FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
+FUZZ_OBJS += oss-fuzz/fuzz-date.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
.PHONY: fuzz-objs
diff --git a/oss-fuzz/.gitignore b/oss-fuzz/.gitignore
index 9acb74412ef..5b954088254 100644
--- a/oss-fuzz/.gitignore
+++ b/oss-fuzz/.gitignore
@@ -1,3 +1,4 @@
fuzz-commit-graph
+fuzz-date
fuzz-pack-headers
fuzz-pack-idx
diff --git a/oss-fuzz/fuzz-date.c b/oss-fuzz/fuzz-date.c
new file mode 100644
index 00000000000..52bea5553a1
--- /dev/null
+++ b/oss-fuzz/fuzz-date.c
@@ -0,0 +1,53 @@
+#include "git-compat-util.h"
+#include "date.h"
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
+{
+ int local;
+ int num;
+ int tz;
+ char *str;
+ int8_t *tmp_data;
+ timestamp_t ts;
+ enum date_mode_type dmtype;
+ struct date_mode *dm;
+
+ if (size <= 4)
+ /*
+ * we use the first byte to fuzz dmtype and local,
+ * then the next three bytes to fuzz tz offset,
+ * and the remainder (at least one byte) is fed
+ * as end-user input to approxidate_careful().
+ */
+ return 0;
+
+ local = !!(*data & 0x10);
+ num = *data % DATE_UNIX;
+ if (num >= DATE_STRFTIME)
+ num++;
+ dmtype = (enum date_mode_type)num;
+ data++;
+ size--;
+
+ tmp_data = (int8_t*)data;
+ tz = *tmp_data++;
+ tz = (tz << 8) | *tmp_data++;
+ tz = (tz << 8) | *tmp_data++;
+ data += 3;
+ size -= 3;
+
+ str = xmemdupz(data, size);
+
+ ts = approxidate_careful(str, &num);
+ free(str);
+
+ dm = date_mode_from_type(dmtype);
+ dm->local = local;
+ show_date(ts, tz, dm);
+
+ date_mode_release(dm);
+
+ return 0;
+}
base-commit: dadef801b365989099a9929e995589e455c51fed
--
gitgitgadget
^ permalink raw reply related
* Re: Feature request: git status --branch-only
From: Phillip Wood @ 2023-11-14 12:28 UTC (permalink / raw)
To: Ondra Medek, git
In-Reply-To: <CAJsoDaFX7YdncsTy7UsjxaM1GCKs36-H5RhJ6kzgBUFBJyoGZQ@mail.gmail.com>
Hi Ondra
On 14/11/2023 10:16, Ondra Medek wrote:
> Hello,
> I am working on a tol which should fetch changes from a remote
> repository on a user click. I want to limit fetch on the current
> remote tracking branch (something like "origin/master"), but
> surprisingly, it's hard to get it for all corner cases like a fresh
> clone of an empty repository or detached head, etc. E.g see this SO
> thread https://stackoverflow.com/questions/171550/find-out-which-remote-branch-a-local-branch-is-tracking/52896538
I think you can do this by calling
git symbolic-ref --quiet HEAD
to get the full refname of the current branch. If HEAD is detached it
will print nothing and exit with exit code 1. Then you can call
git for-each-ref --format="%(upstream:short)" $refname
to get the upstream branch
Best Wishes
Phillip
^ permalink raw reply
* Re: Feature request: git status --branch-only
From: Ondra Medek @ 2023-11-14 12:40 UTC (permalink / raw)
To: phillip.wood; +Cc: git
In-Reply-To: <47fa8400-1e5f-437f-84b8-50bb09580325@gmail.com>
Hi Phillip,
it does not work for a fresh clone of an empty repository
git for-each-ref --format="%(upstream:short)" refs/heads/master
outputs nothing, while
git status -b --no-ahead-behind --porcelain=v2
outputs
# branch.oid (initial)
# branch.head master
# branch.upstream origin/master
I.e. it outputs a proper upstream branch.
Best regards
Ondra
Ondra Medek
On Tue, 14 Nov 2023 at 13:28, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Ondra
>
> On 14/11/2023 10:16, Ondra Medek wrote:
> > Hello,
> > I am working on a tol which should fetch changes from a remote
> > repository on a user click. I want to limit fetch on the current
> > remote tracking branch (something like "origin/master"), but
> > surprisingly, it's hard to get it for all corner cases like a fresh
> > clone of an empty repository or detached head, etc. E.g see this SO
> > thread https://stackoverflow.com/questions/171550/find-out-which-remote-branch-a-local-branch-is-tracking/52896538
>
> I think you can do this by calling
>
> git symbolic-ref --quiet HEAD
>
> to get the full refname of the current branch. If HEAD is detached it
> will print nothing and exit with exit code 1. Then you can call
>
> git for-each-ref --format="%(upstream:short)" $refname
>
> to get the upstream branch
>
> Best Wishes
>
> Phillip
^ permalink raw reply
* Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default
From: Taylor Blau @ 2023-11-14 14:42 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King, Junio C Hamano, Karthik Nayak
In-Reply-To: <ZVNNXNRfrwc_0Sj3@tanuki>
On Tue, Nov 14, 2023 at 11:35:08AM +0100, Patrick Steinhardt wrote:
> Gah, I forgot to run this with GIT_TEST_COMMIT_GRAPH=1 before sending
> this patch. There are two test failures that this change introduces:
>
> - t6022-rev-list-missing.sh, where we test for the `--missing=` option
> of git-rev-list(1).
>
> - t7700-repack.sh, where we also manually delete objects.
>
> Both of these are expected failures: we knowingly corrupt the repository
> and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are
> updated. If we stick with the new stance that repository corruption
> should not require us to pessimize the common case, then we'd have to
> squash in something like the below.
Good catch. Thanks for investigating and providing a thoughtful analysis
of why we should turn off GIT_COMMIT_GRAPH_PARANOIA by default. I agree
with your conclusion (with this follow-up patch squashed in, of course).
My hunch is that it would be easier for the maintainer to have a single
patch to queue instead of squashing this in themself. You may want to
send a "v2" to that effect.
Thanks,
Taylor
^ permalink raw reply
* Re: Feature request: git status --branch-only
From: Phillip Wood @ 2023-11-14 15:02 UTC (permalink / raw)
To: Ondra Medek, phillip.wood; +Cc: git
In-Reply-To: <CAJsoDaHX3t9bViq0F7gmJPD+PoE-ZqmJS5h=u-W900x9KEMmYA@mail.gmail.com>
Hi Ondra
On 14/11/2023 12:40, Ondra Medek wrote:
> Hi Phillip,
>
> it does not work for a fresh clone of an empty repository
>
> git for-each-ref --format="%(upstream:short)" refs/heads/master
>
> outputs nothing, while
Oh dear, that's a shame. I wonder if it is a bug because the
documentation says that
--format="%(upstream:track)"
should print "[gone]" whenever an unknown upstream ref is encountered
but trying that on a clone of an empty repository gives no output.
Best Wishes
Phillip
> git status -b --no-ahead-behind --porcelain=v2
>
> outputs
>
> # branch.oid (initial)
> # branch.head master
> # branch.upstream origin/master
>
> I.e. it outputs a proper upstream branch.
>
> Best regards
> Ondra
>
> Ondra Medek
>
>
> On Tue, 14 Nov 2023 at 13:28, Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Ondra
>>
>> On 14/11/2023 10:16, Ondra Medek wrote:
>>> Hello,
>>> I am working on a tol which should fetch changes from a remote
>>> repository on a user click. I want to limit fetch on the current
>>> remote tracking branch (something like "origin/master"), but
>>> surprisingly, it's hard to get it for all corner cases like a fresh
>>> clone of an empty repository or detached head, etc. E.g see this SO
>>> thread https://stackoverflow.com/questions/171550/find-out-which-remote-branch-a-local-branch-is-tracking/52896538
>>
>> I think you can do this by calling
>>
>> git symbolic-ref --quiet HEAD
>>
>> to get the full refname of the current branch. If HEAD is detached it
>> will print nothing and exit with exit code 1. Then you can call
>>
>> git for-each-ref --format="%(upstream:short)" $refname
>>
>> to get the upstream branch
>>
>> Best Wishes
>>
>> Phillip
>
^ permalink raw reply
* [PATCH] send-email: avoid duplicate specification warnings
From: Todd Zullinger @ 2023-11-14 16:38 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Jeff King,
Ondřej Pohořelský
With perl-Getopt-Long >= 2.55, a warning is issued for options which are
specified more than once. In addition to causing users to see warnings,
this results in test failures which compare the output. An example,
from t9001-send-email.37:
| +++ diff -u expect actual
| --- expect 2023-11-14 10:38:23.854346488 +0000
| +++ actual 2023-11-14 10:38:23.848346466 +0000
| @@ -1,2 +1,7 @@
| +Duplicate specification "no-chain-reply-to" for option "no-chain-reply-to"
| +Duplicate specification "to-cover|to-cover!" for option "to-cover"
| +Duplicate specification "cc-cover|cc-cover!" for option "cc-cover"
| +Duplicate specification "no-thread" for option "no-thread"
| +Duplicate specification "no-to-cover" for option "no-to-cover"
| fatal: longline.patch:35 is longer than 998 characters
| warning: no patches were sent
| error: last command exited with $?=1
| not ok 37 - reject long lines
Remove the duplicate option specs.
Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
I've run this through the full test suite. I also compared the output of
--help to ensure it only differs in the removal of the "Duplicate
specification" warnings. I _think_ that's a good sign that no other changes
will result. But I would be grateful to anyone who can confirm or reject that
theory.
git-send-email.perl | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index cacdbd6bb2..13d9c47fe5 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -491,7 +491,6 @@ sub config_regexp {
"bcc=s" => \@getopt_bcc,
"no-bcc" => \$no_bcc,
"chain-reply-to!" => \$chain_reply_to,
- "no-chain-reply-to" => sub {$chain_reply_to = 0},
"sendmail-cmd=s" => \$sendmail_cmd,
"smtp-server=s" => \$smtp_server,
"smtp-server-option=s" => \@smtp_server_options,
@@ -506,36 +505,27 @@ sub config_regexp {
"smtp-auth=s" => \$smtp_auth,
"no-smtp-auth" => sub {$smtp_auth = 'none'},
"annotate!" => \$annotate,
- "no-annotate" => sub {$annotate = 0},
"compose" => \$compose,
"quiet" => \$quiet,
"cc-cmd=s" => \$cc_cmd,
"header-cmd=s" => \$header_cmd,
"no-header-cmd" => \$no_header_cmd,
"suppress-from!" => \$suppress_from,
- "no-suppress-from" => sub {$suppress_from = 0},
"suppress-cc=s" => \@suppress_cc,
- "signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
- "no-signed-off-cc|no-signed-off-by-cc" => sub {$signed_off_by_cc = 0},
- "cc-cover|cc-cover!" => \$cover_cc,
- "no-cc-cover" => sub {$cover_cc = 0},
- "to-cover|to-cover!" => \$cover_to,
- "no-to-cover" => sub {$cover_to = 0},
+ "signed-off-by-cc!" => \$signed_off_by_cc,
+ "cc-cover!" => \$cover_cc,
+ "to-cover!" => \$cover_to,
"confirm=s" => \$confirm,
"dry-run" => \$dry_run,
"envelope-sender=s" => \$envelope_sender,
"thread!" => \$thread,
- "no-thread" => sub {$thread = 0},
"validate!" => \$validate,
- "no-validate" => sub {$validate = 0},
"transfer-encoding=s" => \$target_xfer_encoding,
"format-patch!" => \$format_patch,
- "no-format-patch" => sub {$format_patch = 0},
"8bit-encoding=s" => \$auto_8bit_encoding,
"compose-encoding=s" => \$compose_encoding,
"force" => \$force,
"xmailer!" => \$use_xmailer,
- "no-xmailer" => sub {$use_xmailer = 0},
"batch-size=i" => \$batch_size,
"relogin-delay=i" => \$relogin_delay,
"git-completion-helper" => \$git_completion_helper,
^ permalink raw reply related
* Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default
From: Junio C Hamano @ 2023-11-14 16:48 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King, Karthik Nayak
In-Reply-To: <ZVNNXNRfrwc_0Sj3@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> Gah, I forgot to run this with GIT_TEST_COMMIT_GRAPH=1 before sending
> this patch. There are two test failures that this change introduces:
>
> - t6022-rev-list-missing.sh, where we test for the `--missing=` option
> of git-rev-list(1).
I would have expected you to enable the paranoia mode automatically
when this option is in effect.
> Both of these are expected failures: we knowingly corrupt the repository
> and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are
> updated. If we stick with the new stance that repository corruption
> should not require us to pessimize the common case,...
Yeah, just like we try to be extra careful while running fsck,
because "--missing" is about finding these "corrupt" cases,
triggering the paranoia mode upon seeing the option would make
sense, no? It would fix the failure in 6022, right?
Thanks for working on this.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox