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:37 UTC
Message-ID: <20201009183750.GA437683@nand.local>
In-Reply-To: <4c138121-ef58-c870-60b2-8140e6e0cbee@gmail.com>

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