Hi, On Mon, May 21, 2018 at 11:10 AM, Derrick Stolee <stolee@xxxxxxxxx> 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@xxxxxxxxxxxxxx/ 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.
