threads / discuss / 54368

2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

Subject: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

## tl;dr

14 messages between Oct 7, 2020 and Oct 9, 2020.

replies: 13people: 4as markdown or json

Thomas Braun· Oct 7, 2020, 20:28 UTC · lore

Hi,

I'm using

$ git --version git version 2.29.0.rc0.windows.1

Since I upgraded to that version (thanks to dscho for providing these so early) I'm seeing occasionally

$ git fetch origin +refs/head/abcd:refs/remotes/origin/abcd fatal: unexpected duplicate commit id 31a13139875bc5f49ddcbd42b4b4d3dc18c16576

The local branch abcd is at

$ git rev-parse HEAD b6ae1ee2adcb75aee09c4a8d72bfd66f32f6fae1

while its remote tracking branch is at

$ git rev-parse @{u} 31a13139875bc5f49ddcbd42b4b4d3dc18c16576

I have pull.rebase=true set but I think this does not matter for fetch. I do have protocol V2 enabled.

A full trace is

$ GIT_TRACE=1 git fetch origin +refs/heads/abcd:refs/remotes/origin/abcd 22:23:42.016859 exec-cmd.c:237 trace: resolved executable dir: C:/Program Files/Git/mingw64/bin 22:23:42.018858 git.c:444 trace: built-in: git fetch origin +refs/heads/abcd:refs/remotes/origin/abcd 22:23:42.020858 run-command.c:663 trace: run_command: unset GIT_PREFIX; GIT_PROTOCOL=version=2 ssh -o SendEnv=GIT_PROTOCOL git@github.com 'git-upload-pack '\''XXX/YYY.git'\''' 22:23:43.284643 run-command.c:663 trace: run_command: git rev-list --objects --stdin --not --all --quiet --alternate-refs 22:23:43.344644 run-command.c:663 trace: run_command: git rev-list --objects --stdin --not --all --quiet --alternate-refs 22:23:43.358613 exec-cmd.c:237 trace: resolved executable dir: C:/Program Files/Git/mingw64/libexec/git-core 22:23:43.360614 git.c:444 trace: built-in: git rev-list --objects --stdin --not --all --quiet --alternate-refs 22:23:43.492616 run-command.c:1617 run_processes_parallel: preparing to run up to 1 tasks 22:23:43.492616 run-command.c:1649 run_processes_parallel: done fatal: unexpected duplicate commit id 31a13139875bc5f49ddcbd42b4b4d3dc18c16576

The repo is public, so if it helps I can provide a link.

Anything I'm doing wrong here?

Thanks, Thomas

Jeff King· Oct 7, 2020, 21:06 UTC · re: Thomas Braun · lore

Re: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

On Wed, Oct 07, 2020 at 10:28:36PM +0200, Thomas Braun wrote:

Show 9 quoted lines

> $ git --version
> git version 2.29.0.rc0.windows.1
> 
> Since I upgraded to that version (thanks to dscho for providing these so
> early) I'm seeing occasionally
> 
> $ git fetch origin +refs/head/abcd:refs/remotes/origin/abcd
> fatal: unexpected duplicate commit id
> 31a13139875bc5f49ddcbd42b4b4d3dc18c16576

That message comes from the commit-graph code:

  $ git grep unexpected.duplicate.commit.id '*.c'
  commit-graph.c:                 die(_("unexpected duplicate commit id %s"),

So presumably it's related to the fetch.writeCommitGraph feature, though I thought it was not on by default (for a while it was tied to feature.experimental, but I think even that is not true in 2.29). Do you have that option set?

The message is in sort_and_scan_merged_commits(), which is trying to join multiple incremental commit-graph files together. Presumably you have two such files with the same commit appearing in both. I think we try to avoid that (by omitting commits from new incrementals that already appear in another one), but I wonder if there is a race or other condition that can cause it. In which case this code ought to be more lenient, and just quietly ignore the duplicate.

Is it possible to share the contents of your .git directory? If not, can you look in .git/objects/info/ and see if there are multiple commit-graph files (and if so, possibly share those; they don't contain any identifying info).

-Peff

Thomas Braun· Oct 8, 2020, 09:52 UTC · re: Jeff King · lore

Re: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

On 07.10.2020 23:06, Jeff King wrote:

Show 21 quoted lines

> On Wed, Oct 07, 2020 at 10:28:36PM +0200, Thomas Braun wrote:
> 
>> $ git --version
>> git version 2.29.0.rc0.windows.1
>>
>> Since I upgraded to that version (thanks to dscho for providing these so
>> early) I'm seeing occasionally
>>
>> $ git fetch origin +refs/head/abcd:refs/remotes/origin/abcd
>> fatal: unexpected duplicate commit id
>> 31a13139875bc5f49ddcbd42b4b4d3dc18c16576
> 
> That message comes from the commit-graph code:
> 
>   $ git grep unexpected.duplicate.commit.id '*.c'
>   commit-graph.c:                 die(_("unexpected duplicate commit id %s"),
> 
> So presumably it's related to the fetch.writeCommitGraph feature, though
> I thought it was not on by default (for a while it was tied to
> feature.experimental, but I think even that is not true in 2.29). Do you
> have that option set?

Yes I do have that set. The error also vanishes if I turn it off via git -c fetch.writeCommitGraph=false ...

Show 7 quoted lines

> The message is in sort_and_scan_merged_commits(), which is trying to
> join multiple incremental commit-graph files together. Presumably you
> have two such files with the same commit appearing in both. I think we
> try to avoid that (by omitting commits from new incrementals that
> already appear in another one), but I wonder if there is a race or other
> condition that can cause it. In which case this code ought to be more
> lenient, and just quietly ignore the duplicate.

Thanks for the explanation. Yes I do have multiple commit graph files.

$ ls -lh objects/info/commit-graphs total 445K -r--r--r-- 1 thomas 197121 123 Oct 8 11:29 commit-graph-chain -r--r--r-- 1 thomas 197121 3.2K Oct 8 11:29 graph-6444f51143e12b3f34c031e60a672d2b29d1c09e.graph -r--r--r-- 1 thomas 197121 434K Oct 8 11:29 graph-6d467862fac6cadecf90c11a054b8883234defec.graph -r--r--r-- 1 thomas 197121 1.9K Oct 8 11:29 graph-6ed4c0bfb0adcc15a7dc58159b3652a23d6d8c14.graph

> Is it possible to share the contents of your .git directory? If not, can
> you look in .git/objects/info/ and see if there are multiple
> commit-graph files (and if so, possibly share those; they don't contain
> any identifying info).

Yes sure, I can share that [1]. Thanks for looking into that.

[1]: http://byte-physics.de/Downloads/dotGitWriteCommitGraphDuplicatedCommitIssue.tar.gz

Jeff King· Oct 8, 2020, 12:06 UTC · re: Thomas Braun · lore

Re: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

On Thu, Oct 08, 2020 at 11:52:03AM +0200, Thomas Braun wrote:

Show 9 quoted lines

> > Is it possible to share the contents of your .git directory? If not, can
> > you look in .git/objects/info/ and see if there are multiple
> > commit-graph files (and if so, possibly share those; they don't contain
> > any identifying info).
> 
> Yes sure, I can share that [1]. Thanks for looking into that.
> 
> [1]:
> http://byte-physics.de/Downloads/dotGitWriteCommitGraphDuplicatedCommitIssue.tar.gz

Thanks, I was able to easily reproduce with:

  # make a cheap copy of the repo; if our test succeeds it modifies the
  # graph files, so just try it on a fresh copy each time
  rm -rf repo.git
  cp -al /path/to/extracted/tarball/.git repo.git
  git -C repo.git -c fetch.writecommitgraph fetch .

which yields:

  From .
   * branch                HEAD       -> FETCH_HEAD
  fatal: unexpected duplicate commit id 31a13139875bc5f49ddcbd42b4b4d3dc18c16576

The good news is that this isn't a regression in v2.29. The bad news is that it's been broken for many versions. :)

To solve your immediate problem, you can just remove the whole .git/objects/info/commit-graphs directory. It doesn't have any data that can't be regenerated from the actual objects.

The rest of this email is my look at what the actual bug is.

Bisecting finds a very curious culprit: 0bd52e27e3 (commit-graph.h: store an odb in 'struct write_commit_graph_context', 2020-02-03). That commit causes us to use a more consistent object directory name. As a result, this loop in split_graph_merge_strategy():

        while (g && (g->num_commits <= size_mult * num_commits ||
                    (max_commits && num_commits > max_commits))) {
                if (strcmp(g->obj_dir, ctx->odb->path))
                        break;

                num_commits += g->num_commits;
                g = g->base_graph;

                ctx->num_commit_graphs_after--;
        }

does not trigger the "break" on the strcmp, whereas before that commit it did. As a result, our "after" graph count is smaller (2 versus 4). And then later in merge_commit_graphs(), we know we're shrinking the number of graphs, so we add in the contents of those graph files.

So I _think_ everything being done by that patch is correct, and we didn't see the problem before simply because we were erroneously not rolling up the graph files. And once we do, we can see that indeed we have the same commit in two files. If I instrument the commit-graph code like this (I couldn't find a command to dump incremental graph file data; is there one?):

diff --git a/commit-graph.c b/commit-graph.c
index cbfeece112..4d22fa3b41 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1603,13 +1603,16 @@ static void merge_commit_graph(struct write_commit_graph_context *ctx,
 
 	ALLOC_GROW(ctx->commits.list, ctx->commits.nr + g->num_commits, ctx->commits.alloc);
 
+	warning("opening graph file %s", g->filename);
+
 	for (i = 0; i < g->num_commits; i++) {
 		struct object_id oid;
 		struct commit *result;
 
 		display_progress(ctx->progress, i + 1);
 
 		load_oid_from_graph(g, i + offset, &oid);
+		warning("has oid %s", oid_to_hex(&oid));
 
 		/* only add commits if they still exist in the repo */
 		result = lookup_commit_reference_gently(ctx->r, &oid, 1);

I see:

  warning: opening graph file objects/info/commit-graphs/graph-6ed4c0bfb0adcc15a7dc58159b3652a23d6d8c14.graph
  ...
  warning: has oid 31a13139875bc5f49ddcbd42b4b4d3dc18c16576
  ...
  warning: opening graph file objects/info/commit-graphs/graph-6444f51143e12b3f34c031e60a672d2b29d1c09e.graph
  ...
  warning: has oid 31a13139875bc5f49ddcbd42b4b4d3dc18c16576

I'm not sure how that happened, and whether it's a bug that we got into
this state at all. But regardless, it seems unfriendly that we can't
get out of it while merging the graphs. Doing this obviously makes the
problem go away:

diff --git a/commit-graph.c b/commit-graph.c
index cb042bdba8..ae1f94ccc4 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2023,8 +2023,11 @@ static void sort_and_scan_merged_commits(struct write_commit_graph_context *ctx)
 
 		if (i && oideq(&ctx->commits.list[i - 1]->object.oid,
 			  &ctx->commits.list[i]->object.oid)) {
-			die(_("unexpected duplicate commit id %s"),
-			    oid_to_hex(&ctx->commits.list[i]->object.oid));
+			/*
+			 * quietly ignore duplicates; these could come from
+			 * incremental graph files mentioning the same commit.
+			 */
+			continue;
 		} else {
 			unsigned int num_parents;
 

but it's not clear to me if that's papering over another bug, or
gracefully handling a situation that we ought to be.

-Peff

Derrick Stolee· Oct 8, 2020, 12:50 UTC · re: Jeff King · lore

Re: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

On 10/8/2020 8:06 AM, Jeff King wrote:

Show 8 quoted lines

> On Thu, Oct 08, 2020 at 11:52:03AM +0200, Thomas Braun wrote:
> 
>>> Is it possible to share the contents of your .git directory? If not, can
>>> you look in .git/objects/info/ and see if there are multiple
>>> commit-graph files (and if so, possibly share those; they don't contain
>>> any identifying info).
>>
>> Yes sure, I can share that [1]. Thanks for looking into that.

Thank you for the report! And thanks for sharing your data.

> To solve your immediate problem, you can just remove the whole
> .git/objects/info/commit-graphs directory. It doesn't have any data that
> can't be regenerated from the actual objects.

Yes, this is the best way forward for issues like this. Sorry
for the inconvenience!

> The rest of this email is my look at what the actual bug is.

...

> So I _think_ everything being done by that patch is correct, and we
> didn't see the problem before simply because we were erroneously not
> rolling up the graph files. And once we do, we can see that indeed we
> have the same commit in two files.

I think the die() that is reporting this answer is doing so because this state is one that _shouldn't_ happen. The intention is that there is always only one "graph position" for a commit object, and any case where we have multiple is an unknown and unsupported situation.

However, how wrong would that be (as long as the two "rows" have the same data)? It depends on how we find the commit:

1. If we are trying to look up a commit from a ref, then the binary
   search into the commit-graph(s) will find one of the rows first,
   then set "graph_pos" in the commit_graph_data_slab for that commit.

2. If we are looking up a commit via a graph position from a child
   commit in the commit-graph, then we will immediately navigate to
   that specific row to find the OID. If we previously parsed that
   commit, then we will not try to parse it a second time based on
   the populated "struct commit" we get from lookup_commit().
   Otherwise, we'll read the row to fill the parents and other data
   at that commit.

So, it seems that the existing commit-graph reading strategy can handle this "duplicate commit" case (at least, across layers).

 If I instrument the commit-graph code

> like this (I couldn't find a command to dump incremental graph file
> data; is there one?):

Not really. Could be useful for cases like this, though.

> I'm not sure how that happened, and whether it's a bug that we got into
> this state at all.

It is likely a bug that we got into this state. We should still be able to handle it gracefully.

Show 25 quoted lines

> But regardless, it seems unfriendly that we can't
> get out of it while merging the graphs. Doing this obviously makes the
> problem go away:
> 
> diff --git a/commit-graph.c b/commit-graph.c
> index cb042bdba8..ae1f94ccc4 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -2023,8 +2023,11 @@ static void sort_and_scan_merged_commits(struct write_commit_graph_context *ctx)
>  
>  		if (i && oideq(&ctx->commits.list[i - 1]->object.oid,
>  			  &ctx->commits.list[i]->object.oid)) {
> -			die(_("unexpected duplicate commit id %s"),
> -			    oid_to_hex(&ctx->commits.list[i]->object.oid));
> +			/*
> +			 * quietly ignore duplicates; these could come from
> +			 * incremental graph files mentioning the same commit.
> +			 */
> +			continue;
>  		} else {
>  			unsigned int num_parents;
>  
> 
> but it's not clear to me if that's papering over another bug, or
> gracefully handling a situation that we ought to be.

I think this is a good thing to do, at minimum. As I discussed above, the "input data" of the incremental commit-graph chain with duplicate commits across layers isn't actually _invalid_. It's unexpected based on what Git "should" be doing.

This kind of change is something we could possibly handle within the RC window, since it only unblocks people who are already in a bad state.

It would also be good to see if we can discover how this happened in the first place, but that might be a more lengthy investigation and patch.

Thanks, -Stolee

Derrick Stolee· Oct 8, 2020, 13:22 UTC · re: Derrick Stolee · lore

Re: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

On 10/8/2020 8:50 AM, Derrick Stolee wrote:

Show 31 quoted lines

> On 10/8/2020 8:06 AM, Jeff King wrote:
>> But regardless, it seems unfriendly that we can't
>> get out of it while merging the graphs. Doing this obviously makes the
>> problem go away:
>>
>> diff --git a/commit-graph.c b/commit-graph.c
>> index cb042bdba8..ae1f94ccc4 100644
>> --- a/commit-graph.c
>> +++ b/commit-graph.c
>> @@ -2023,8 +2023,11 @@ static void sort_and_scan_merged_commits(struct write_commit_graph_context *ctx)
>>  
>>  		if (i && oideq(&ctx->commits.list[i - 1]->object.oid,
>>  			  &ctx->commits.list[i]->object.oid)) {
>> -			die(_("unexpected duplicate commit id %s"),
>> -			    oid_to_hex(&ctx->commits.list[i]->object.oid));
>> +			/*
>> +			 * quietly ignore duplicates; these could come from
>> +			 * incremental graph files mentioning the same commit.
>> +			 */
>> +			continue;
>>  		} else {
>>  			unsigned int num_parents;
>>  
>>
>> but it's not clear to me if that's papering over another bug, or
>> gracefully handling a situation that we ought to be.
> 
> I think this is a good thing to do, at minimum. As I discussed above,
> the "input data" of the incremental commit-graph chain with duplicate
> commits across layers isn't actually _invalid_. It's unexpected based
> on what Git "should" be doing.

As I was working on my own version of this, I realized that just commenting here still creates duplicate commits in the new layer, which is even MORE unexpected. It could cause some confusion with the binary search, but likely that is still fine. The only "real" issue is that it is wasted data.

I'll send [1] to the list soon (after build & test validation), but it includes copying the pointers to a new "de-duplicated" list.

[1] https://github.com/gitgitgadget/git/pull/747

Thanks, -Stolee

Thomas Braun· Oct 9, 2020, 15:29 UTC · re: Derrick Stolee · lore

Re: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

On 08.10.2020 15:22, Derrick Stolee wrote:

Show 41 quoted lines

> On 10/8/2020 8:50 AM, Derrick Stolee wrote:
>> On 10/8/2020 8:06 AM, Jeff King wrote:
>>> But regardless, it seems unfriendly that we can't
>>> get out of it while merging the graphs. Doing this obviously makes the
>>> problem go away:
>>>
>>> diff --git a/commit-graph.c b/commit-graph.c
>>> index cb042bdba8..ae1f94ccc4 100644
>>> --- a/commit-graph.c
>>> +++ b/commit-graph.c
>>> @@ -2023,8 +2023,11 @@ static void sort_and_scan_merged_commits(struct write_commit_graph_context *ctx)
>>>  
>>>  		if (i && oideq(&ctx->commits.list[i - 1]->object.oid,
>>>  			  &ctx->commits.list[i]->object.oid)) {
>>> -			die(_("unexpected duplicate commit id %s"),
>>> -			    oid_to_hex(&ctx->commits.list[i]->object.oid));
>>> +			/*
>>> +			 * quietly ignore duplicates; these could come from
>>> +			 * incremental graph files mentioning the same commit.
>>> +			 */
>>> +			continue;
>>>  		} else {
>>>  			unsigned int num_parents;
>>>  
>>>
>>> but it's not clear to me if that's papering over another bug, or
>>> gracefully handling a situation that we ought to be.
>>
>> I think this is a good thing to do, at minimum. As I discussed above,
>> the "input data" of the incremental commit-graph chain with duplicate
>> commits across layers isn't actually _invalid_. It's unexpected based
>> on what Git "should" be doing.
> 
> As I was working on my own version of this, I realized that just
> commenting here still creates duplicate commits in the new layer,
> which is even MORE unexpected. It could cause some confusion with
> the binary search, but likely that is still fine. The only "real"
> issue is that it is wasted data.
> 
> I'll send [1] to the list soon (after build & test validation),
> but it includes copying the pointers to a new "de-duplicated" list.

Thanks both for digging into it.

I think I have a starting point for what goes wrong. I found a local repo with another broken commit graph. And after some fiddling the following script can reproduce it. I tried with git/git first but that seems not to trigger that.

# rm -rf dummy mkdir dummy cd dummy

git init

git remote add origin https://github.com/tango-controls/cppTango git remote add fork1 https://github.com/bourtemb/cppTango git remote add fork2 https://github.com/t-b/cppTango git fetch --all --jobs 12 git commit-graph verify rm -rf .git/objects/info/commit-graphs/ git commit-graph verify git fetch --jobs 12 git remote add fork3 git@github.com:t-b/cppTango.git git commit-graph verify git remote add fork4 git@github.com:t-b/cppTango.git git fetch --jobs 12 git commit-graph verify

The last verify outputs

commit-graph generation for commit 029341567c24582030592585b395f4438273263f is 1054 != 1 commit-graph generation for commit 1e8d10aec7ca6075f622c447d416071390698124 is 4294967295 != 1171 commit-graph generation for commit 296e93516189c0134843fd56ac4f10d36ccf284f is 1054 != 1 commit-graph generation for commit 4c0a7a3cd369d06b99d867be6b47a96c519efd7f is 1054 != 1 commit-graph has non-zero generation number for commit 4d39849950d3dc02b7426c780ac7991ec7221176, but zero elsewhere commit-graph has non-zero generation number for commit 4 [....]

Does that reproduce on your end as well?

Thomas

Derrick Stolee· Oct 9, 2020, 16:49 UTC · re: Thomas Braun · lore

Re: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

On 10/9/2020 11:29 AM, Thomas Braun wrote:

> I think I have a starting point for what goes wrong. I found a local
> repo with another broken commit graph. And after some fiddling the
> following script can reproduce it. I tried with git/git first but that
> seems not to trigger that.

I'm glad you're able to trigger bad commit-graph data somehow. Let's see what's going on:

Show 10 quoted lines

> # rm -rf dummy
> mkdir dummy
> cd dummy
> 
> git init
> 
> git remote add origin https://github.com/tango-controls/cppTango
> git remote add fork1 https://github.com/bourtemb/cppTango
> git remote add fork2 https://github.com/t-b/cppTango
> git fetch --all --jobs 12

My gut reaction is that this parallel fetching is causing an issue, but we will see. Do you get a repro if you drop the "--jobs 12"?

Show 24 quoted lines

> git commit-graph verify
> rm -rf .git/objects/info/commit-graphs/
> git commit-graph verify
> git fetch --jobs 12
> git remote add fork3 git@github.com:t-b/cppTango.git
> git commit-graph verify
> git remote add fork4 git@github.com:t-b/cppTango.git
> git fetch --jobs 12
> git commit-graph verify
> 
> The last verify outputs
> 
> commit-graph generation for commit
> 029341567c24582030592585b395f4438273263f is 1054 != 1
> commit-graph generation for commit
> 1e8d10aec7ca6075f622c447d416071390698124 is 4294967295 != 1171
> commit-graph generation for commit
> 296e93516189c0134843fd56ac4f10d36ccf284f is 1054 != 1
> commit-graph generation for commit
> 4c0a7a3cd369d06b99d867be6b47a96c519efd7f is 1054 != 1
> commit-graph has non-zero generation number for commit
> 4d39849950d3dc02b7426c780ac7991ec7221176, but zero elsewhere
> commit-graph has non-zero generation number for commit 4
> [....]

This looks more troubling than just duplicate rows, but perhaps those duplicate rows are causing sufficient confusion when reading the commit-graph during the 'verify' command?

I tried incorporating this into the Git test suite so I could test it on v2.29.0-rc0 and the current merge-check, but I'm failing to reproduce the failure with this script:

diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index c334ee9155..2b3f3db593 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -440,4 +440,26 @@ test_expect_success '--split=replace with partial Bloom data' '
 	verify_chain_files_exist $graphdir
 '
 
+test_expect_success 'test' '
+	git init dummy &&
+	(
+		cd dummy &&
+		export GIT_TRACE2_EVENT="$TRASH_DIRECTORY/../trace.txt" &&
+		git config fetch.writeCommitGraph true &&
+		git remote add origin https://github.com/tango-controls/cppTango &&
+		git remote add fork1 https://github.com/bourtemb/cppTango &&
+		git remote add fork2 https://github.com/t-b/cppTango &&
+		git fetch --all --jobs 12 &&
+		git commit-graph verify &&
+		rm -rf .git/objects/info/commit-graphs/ &&
+		git commit-graph verify &&
+		git fetch --jobs 12 &&
+		git remote add fork3 git@github.com:t-b/cppTango.git &&
+		git commit-graph verify &&
+		git remote add fork4 git@github.com:t-b/cppTango.git &&
+		git fetch --jobs 12 &&
+		git commit-graph verify
+	)
+'
+
 test_done

I tried this on Linux and Windows, and under "--stress" but never
saw a failure.

Thomas: some things that could possibly help is if you repro this
situation but also do something like

	export GIT_TRACE2_EVENT="$(pwd)/trace.txt"

so we can read the details of everything Git is tracing during
these parallel jobs. We might be able to stitch together a
sequence of events that lead to these failures.

Thanks,
Stolee

Thomas Braun· Oct 9, 2020, 17:12 UTC · re: Derrick Stolee · lore

Re: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

On 09.10.2020 18:49, Derrick Stolee wrote:

Show 101 quoted lines

> On 10/9/2020 11:29 AM, Thomas Braun wrote:
>> I think I have a starting point for what goes wrong. I found a local
>> repo with another broken commit graph. And after some fiddling the
>> following script can reproduce it. I tried with git/git first but that
>> seems not to trigger that.
> 
> I'm glad you're able to trigger bad commit-graph data somehow.
> Let's see what's going on:
> 
>> # rm -rf dummy
>> mkdir dummy
>> cd dummy
>>
>> git init
>>
>> git remote add origin https://github.com/tango-controls/cppTango
>> git remote add fork1 https://github.com/bourtemb/cppTango
>> git remote add fork2 https://github.com/t-b/cppTango
>> git fetch --all --jobs 12
> 
> My gut reaction is that this parallel fetching is causing
> an issue, but we will see. Do you get a repro if you drop
> the "--jobs 12"?
> 
>> git commit-graph verify
>> rm -rf .git/objects/info/commit-graphs/
>> git commit-graph verify
>> git fetch --jobs 12
>> git remote add fork3 git@github.com:t-b/cppTango.git
>> git commit-graph verify
>> git remote add fork4 git@github.com:t-b/cppTango.git
>> git fetch --jobs 12
>> git commit-graph verify
>>
>> The last verify outputs
>>
>> commit-graph generation for commit
>> 029341567c24582030592585b395f4438273263f is 1054 != 1
>> commit-graph generation for commit
>> 1e8d10aec7ca6075f622c447d416071390698124 is 4294967295 != 1171
>> commit-graph generation for commit
>> 296e93516189c0134843fd56ac4f10d36ccf284f is 1054 != 1
>> commit-graph generation for commit
>> 4c0a7a3cd369d06b99d867be6b47a96c519efd7f is 1054 != 1
>> commit-graph has non-zero generation number for commit
>> 4d39849950d3dc02b7426c780ac7991ec7221176, but zero elsewhere
>> commit-graph has non-zero generation number for commit 4
>> [....]
> 
> This looks more troubling than just duplicate rows, but
> perhaps those duplicate rows are causing sufficient
> confusion when reading the commit-graph during the
> 'verify' command?
> 
> I tried incorporating this into the Git test suite so I
> could test it on v2.29.0-rc0 and the current merge-check,
> but I'm failing to reproduce the failure with this script:
> 
> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> index c334ee9155..2b3f3db593 100755
> --- a/t/t5324-split-commit-graph.sh
> +++ b/t/t5324-split-commit-graph.sh
> @@ -440,4 +440,26 @@ test_expect_success '--split=replace with partial Bloom data' '
>  	verify_chain_files_exist $graphdir
>  '
>  
> +test_expect_success 'test' '
> +	git init dummy &&
> +	(
> +		cd dummy &&
> +		export GIT_TRACE2_EVENT="$TRASH_DIRECTORY/../trace.txt" &&
> +		git config fetch.writeCommitGraph true &&
> +		git remote add origin https://github.com/tango-controls/cppTango &&
> +		git remote add fork1 https://github.com/bourtemb/cppTango &&
> +		git remote add fork2 https://github.com/t-b/cppTango &&
> +		git fetch --all --jobs 12 &&
> +		git commit-graph verify &&
> +		rm -rf .git/objects/info/commit-graphs/ &&
> +		git commit-graph verify &&
> +		git fetch --jobs 12 &&
> +		git remote add fork3 git@github.com:t-b/cppTango.git &&
> +		git commit-graph verify &&
> +		git remote add fork4 git@github.com:t-b/cppTango.git &&
> +		git fetch --jobs 12 &&
> +		git commit-graph verify
> +	)
> +'
> +
>  test_done
> 
> I tried this on Linux and Windows, and under "--stress" but never
> saw a failure.
> 
> Thomas: some things that could possibly help is if you repro this
> situation but also do something like
> 
> 	export GIT_TRACE2_EVENT="$(pwd)/trace.txt"
> 
> so we can read the details of everything Git is tracing during
> these parallel jobs. We might be able to stitch together a
> sequence of events that lead to these failures.

Sure! Please find them attached. I retried with no jobs parameter as well, same issues.

I did some more bisecting of my git settings. And now it's getting embarrassing...

Can you reproduce it if you do

git config core.commitGraph false git config fetch.writeCommitGraph true ?

If I flip core.commitGraph to true I don't get an error anymore.

Derrick Stolee· Oct 9, 2020, 17:46 UTC · re: Thomas Braun · lore

Re: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

On 10/9/2020 1:12 PM, Thomas Braun wrote:

Show 11 quoted lines

> Sure! Please find them attached. I retried with no jobs parameter as
> well, same issues.
> 
> I did some more bisecting of my git settings. And now it's getting
> embarrassing...
> 
> Can you reproduce it if you do
> 
> git config core.commitGraph false
> git config fetch.writeCommitGraph true
> ?

I _can_ repro it in this case! I think there must be something very interesting going on where the commit-graph is parsed in _some_ places, but not in others. This is something that I can really start to dig into.

It's important to note that the final "git fetch" adds a layer to the commit-graph chain even though we shouldn't be reading from the commit-graph file! This is an unusual situation that is unexpected, but should be handled correctly.

My gut reaction is to end early in any commit-graph write method when core.commitGraph is false. But that only papers over the real issue here. I'll keep digging.

Thanks for the logs, they may come in handy! -Stolee

Jeff King· Oct 9, 2020, 17:55 UTC · re: Derrick Stolee · lore

Re: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

On Fri, Oct 09, 2020 at 01:46:07PM -0400, Derrick Stolee wrote:

Show 10 quoted lines

> > Can you reproduce it if you do
> > 
> > git config core.commitGraph false
> > git config fetch.writeCommitGraph true
> > ?
> 
> I _can_ repro it in this case! I think there must be something
> very interesting going on where the commit-graph is parsed in
> _some_ places, but not in others. This is something that I can
> really start to dig into.

Here's a much more minimal reproduction:

  git init repo
  cd repo

  git commit --allow-empty -m one
  git rev-parse HEAD |
  git -c core.commitGraph=false \
      commit-graph write --split=no-merge --stdin-commits
  git rev-parse HEAD |
  git -c core.commitGraph=false \
      commit-graph write --split=no-merge --stdin-commits

  git commit --allow-empty -m two
  git rev-parse HEAD |
  git commit-graph write --split --stdin-commits

The final write will die() with the "unexpected duplicate" message.

-Peff

Taylor Blau· Oct 9, 2020, 18:28 UTC · re: Jeff King · lore

Re: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

On Fri, Oct 09, 2020 at 01:55:06PM -0400, Jeff King wrote:

Show 31 quoted lines

> On Fri, Oct 09, 2020 at 01:46:07PM -0400, Derrick Stolee wrote:
>
> > > Can you reproduce it if you do
> > >
> > > git config core.commitGraph false
> > > git config fetch.writeCommitGraph true
> > > ?
> >
> > I _can_ repro it in this case! I think there must be something
> > very interesting going on where the commit-graph is parsed in
> > _some_ places, but not in others. This is something that I can
> > really start to dig into.
>
> Here's a much more minimal reproduction:
>
>   git init repo
>   cd repo
>
>   git commit --allow-empty -m one
>   git rev-parse HEAD |
>   git -c core.commitGraph=false \
>       commit-graph write --split=no-merge --stdin-commits
>   git rev-parse HEAD |
>   git -c core.commitGraph=false \
>       commit-graph write --split=no-merge --stdin-commits
>
>   git commit --allow-empty -m two
>   git rev-parse HEAD |
>   git commit-graph write --split --stdin-commits
>
> The final write will die() with the "unexpected duplicate" message.

Makes sense; the second commit-graph write won't know that 'one' is already in the graph because 'core.commitGraph' prevents 'prepare_commit_graph()' from actually loading the graph (actually loading the graph would be enough to stop the second write from occurring at all.)

I'm of two minds about what we could be doing here:

  - On the one hand we could be ignoring `core.commitGraph` setting
    during commit-graph writes and forcibly loading the commit-graph
    anyway.

  - But on the other hand, writing a commit graph with `core.commitGraph` set
    to false makes no sense. So, I'd almost rather have us die()
    immediately if core.commitGraph is set to false.

I think I'd advocate for the latter, along with Stolee's patch to not die in the case of duplicate commits in multiple layers of the graph.

> -Peff

Thanks, Taylor

Derrick Stolee· Oct 9, 2020, 18:33 UTC · re: Taylor Blau · lore

Re: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

On 10/9/2020 2:28 PM, Taylor Blau wrote:

Show 38 quoted lines

> On Fri, Oct 09, 2020 at 01:55:06PM -0400, Jeff King wrote:
>> On Fri, Oct 09, 2020 at 01:46:07PM -0400, Derrick Stolee wrote:
>>
>>>> Can you reproduce it if you do
>>>>
>>>> git config core.commitGraph false
>>>> git config fetch.writeCommitGraph true
>>>> ?
>>>
>>> I _can_ repro it in this case! I think there must be something
>>> very interesting going on where the commit-graph is parsed in
>>> _some_ places, but not in others. This is something that I can
>>> really start to dig into.
>>
>> Here's a much more minimal reproduction:
>>
>>   git init repo
>>   cd repo
>>
>>   git commit --allow-empty -m one
>>   git rev-parse HEAD |
>>   git -c core.commitGraph=false \
>>       commit-graph write --split=no-merge --stdin-commits
>>   git rev-parse HEAD |
>>   git -c core.commitGraph=false \
>>       commit-graph write --split=no-merge --stdin-commits
>>
>>   git commit --allow-empty -m two
>>   git rev-parse HEAD |
>>   git commit-graph write --split --stdin-commits
>>
>> The final write will die() with the "unexpected duplicate" message.
> 
> Makes sense; the second commit-graph write won't know that 'one' is
> already in the graph because 'core.commitGraph' prevents
> 'prepare_commit_graph()' from actually loading the graph (actually
> loading the graph would be enough to stop the second write from
> occurring at all.)

Right. We aren't parsing from the commit-graph, so we don't see that these commits are already in the file.

Show 9 quoted lines

> I'm of two minds about what we could be doing here:
> 
>   - On the one hand we could be ignoring `core.commitGraph` setting
>     during commit-graph writes and forcibly loading the commit-graph
>     anyway.
> 
>   - But on the other hand, writing a commit graph with `core.commitGraph` set
>     to false makes no sense. So, I'd almost rather have us die()
>     immediately if core.commitGraph is set to false.

I agree that we should just give up, but die() would not be correct. We should just "return 0", possibly with a warning.

> I think I'd advocate for the latter, along with Stolee's patch to not
> die in the case of duplicate commits in multiple layers of the graph.

If we agree that writing a commit-graph makes no sense if the feature is disabled, then I can include a patch that has a test similar to Peff's and that change.

Thanks, -Stolee

Taylor Blau· Oct 9, 2020, 18:37 UTC · re: Derrick Stolee · lore

Re: 2.29.0.rc0.windows.1: Duplicate commit id error message when fetching

On Fri, Oct 09, 2020 at 02:33:02PM -0400, Derrick Stolee wrote:

Show 8 quoted lines

> > Makes sense; the second commit-graph write won't know that 'one' is
> > already in the graph because 'core.commitGraph' prevents
> > 'prepare_commit_graph()' from actually loading the graph (actually
> > loading the graph would be enough to stop the second write from
> > occurring at all.)
>
> Right. We aren't parsing from the commit-graph, so we don't see
> that these commits are already in the file.

OK, I feel even better knowing that you and I both agree on the cause of this buglet ;-).

This also makes me think that this has probably existed since the beginning of commit-graphs, and that it only became easier to tickle in recent releases with things like '--split=no-merge'.

Show 6 quoted lines

> >   - But on the other hand, writing a commit graph with `core.commitGraph` set
> >     to false makes no sense. So, I'd almost rather have us die()
> >     immediately if core.commitGraph is set to false.
>
> I agree that we should just give up, but die() would not be correct.
> We should just "return 0", possibly with a warning.

Yeah; that sounds much better.

Show 6 quoted lines

> > I think I'd advocate for the latter, along with Stolee's patch to not
> > die in the case of duplicate commits in multiple layers of the graph.
>
> If we agree that writing a commit-graph makes no sense if the feature
> is disabled, then I can include a patch that has a test similar to
> Peff's and that change.

Sounds good. I'm certainly on board, but I want to hear what others think, too.

I thought that we had a configuration variable to control whether or not we write changed-path Bloom filters, so I was going to ask about what we should do if that was set to false, and the caller passed '--changed-paths'. But, I guess that my memory was wrong, since I couldn't find such a variable to begin with (we _do_ have 'commitGraph.readChangedPaths', but since that only controls reading no additional special care has to be taken).

Thanks for working on this.

> Thanks,
> -Stolee

Thanks, Taylor