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

[Taylor Blau <me@ttaylorr.com>]

On Fri, Oct 09, 2020 at 02:33:02PM -0400, Derrick Stolee wrote:
> > 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'.

> >   - 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.

> > 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