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

From: Derrick Stolee <stolee@gmail.com>
Date: Oct 9, 2020, 18:33 UTC
Message-ID: <4c138121-ef58-c870-60b2-8140e6e0cbee@gmail.com>
In-Reply-To: <20201009182833.GA437455@nand.local>

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