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

Hi,

On Mon, May 21, 2018 at 11:10 AM, Derrick Stolee [off-list ref] 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.