Re: [PATCH v4 08/24] Ensure index matches head before invoking merge machinery, round N

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 3145 bytes --]

Hi Elijah,

On Tue, 3 Sep 2019, Johannes Schindelin wrote:

> On Sat, 17 Aug 2019, Elijah Newren wrote:
>
> >   * t3030-merge-recursive.h: this test has always been broken in that it
> >     didn't make sure to make index match head before running.  But, it
> >     didn't care about the index or even the merge result, just the
> >     verbose output while running.  While commit eddd1a411d93
> >     ("merge-recursive: enforce rule that index matches head before
> >     merging", 2018-06-30) should have uncovered this broken test, it
> >     used a test_must_fail wrapper around the merge-recursive call
> >     because it was known that the merge resulted in a rename/rename
> >     conflict.  Thus, that fix only made this test fail for a different
> >     reason, and since the index == head check didn't happen until after
> >     coming all the way back out of the recursion, the testcase had
> >     enough information to pass the one check that it did perform.
>
> I fear that this test is still broken, it is a Schrödinger bug. Where
> `qsort()` is the cat, and the property "is it stable?" instead of death.
>
> In particular, at some stage in the recursive merge, a diff is generated
> with rename detection for the target file `a` that contains two lines `hello`
> but has two equally valid source files: `e` and `a~Temporary merge
> branch 2_0`, both containing just the line `hello`. And since their file
> contents are identical, the solution to the problem "which is the
> correct source file?" is ambiguous.
>
> If the `qsort()` in use is stable, the file `e` comes first, and wins.
> If the `qsort()` in use is not stable, all bets are off, and the file
> `a~Temporary merge branch 2_0` might be sorted first (which is the case,
> for example, when using the `qsort()` implementation of MS Visual C's
> runtime).
>
> Now, the _real_ problem is that t3030.35 expects the recursive merge to
> fail, which it does when `qsort()` is stable. However, when the order of
> `e` and `a~Temporary merge branch 2_0` is reversed, then that particular
> merge does _not_ result in a `rename/rename` conflict. And the exit code
> of the recursive merge is 0 for some reason!
>
> I don't quite understand why: clearly, there are conflicts (otherwise we
> would not have that funny suffix `~Temporary merge branch 2_0`.
>
> The real problem, though, is that even if it would fail, the outcome of
> that recursive merge is ambiguous, and that test case should not try to
> verify the precise order of the generated intermediate trees.

It might not be obvious from my mail, but it took me about 7 hours to
figure all of this out, hence I was a bit grumpy when I wrote that. My
apologies.

After having slept (and written a long review about the
`--update-branches` patch), it occurred to me that we might be better
off enforcing the use of `git_qsort()` in `diffcore-rename.c`, so that
we can at least guarantee stable rename detection in Git (which would
incidentally fix the test suite for the MSVC build that I maintain in
Git for Windows).

What do you think?

Ciao,
Dscho