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

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 9, 2020, 18:28 UTC
Message-ID: <20201009182833.GA437455@nand.local>
In-Reply-To: <20201009175506.GA957408@coredump.intra.peff.net>

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