Re: commit-graph: change in "best" merge-base when ambiguous

From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	Jeff King <peff@peff.net>, Junio C Hamano <gitster@pobox.com>,
	Jakub Narebski <jnareb@gmail.com>,
	Jeff Hostetler <git@jeffhostetler.com>
Subject: Re: commit-graph: change in "best" merge-base when ambiguous
Date: Mon, 21 May 2018 11:33:11 -0700	[thread overview]
Message-ID: <CABPp-BFEd+fK_i3qoYWudYS5mhWE1jsXR_xcSCZoJ=4Vd61LAQ@mail.gmail.com> (raw)
In-Reply-To: <e78a115a-a5ea-3c0a-5437-51ba0bcc56e1@gmail.com>

Hi,

On Mon, May 21, 2018 at 11:10 AM, Derrick Stolee <stolee@gmail.com> wrote:
> Hello all,
>
> While working on the commit-graph feature, I made a test commit that sets
> core.commitGraph and gc.commitGraph to true by default AND runs 'git
> commit-graph write --reachable' after each 'git commit' command. This helped
> me find instances in the test suite where the commit-graph feature changes
> existing functionality. Most of these were in regards to grafts,
> replace-objects, and shallow-clones (as expected) or when trying to find a
> corrupt or hidden commit (the commit-graph hides this corrupt/missing data).
> However, there was one interesting case that I'd like to mention on-list.
>
> In t6024-recursive-merge.sh, we have the following commit structure:
>
>     # 1 - A - D - F
>     #   \   X   /
>     #     B   X
>     #       X   \
>     # 2 - C - E - G
>
> When merging F to G, there are two "best" merge-bases, A and C. With
> core.commitGraph=false, 'git merge-base F G' returns A, while it returns C
> when core.commitGraph=true. This is due to the new walk order when using
> generation numbers, although I have not dug deep into the code to point out
> exactly where the choice between A and C is made. Likely it's just whatever
> order they are inserted into a list.

Ooh, interesting.

Just a guess, but could it be related to relative ordering of
committer timestamps?  Ordering of committer timestamps apparently
affects order of merge-bases returned to merge-recursive, and although
that shouldn't have mattered, a few bugs meant that it did and the
order ended up determining what contents a successful merge would
have.  See this recent post:

https://public-inbox.org/git/CABPp-BFc1OLYKzS5rauOehvEugPc0oGMJp-NMEAmVMW7QR=4Eg@mail.gmail.com/

The fact that the merge was successful for both orderings of merge
bases was the real bug, though; it should have detected and reported a
conflict both ways.

I'm not sure where else we have an accidental and incorrect dependence
on merge-base tie-breaker or ordering logic, but if it's like this
one, changing the tie-breaker should be okay.