Hi Elijah,
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.
Ciao,
Dscho
