Re: [PATCH 05/48] t6039: Add tests for content issues with modify/rename/directory conflicts

[Elijah Newren <newren@gmail.com>]

Hi,

Thanks for the many detailed reviews.  I've spent much of the weekend
(and before) trying to go through and clean things up, and am still
working on it.  There are some things to comment on outside of the
series re-roll I am putting together...

On Mon, Jul 18, 2011 at 5:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Could you add a description for this? Your description on 06/48 is
> beautifly done. Here is my attempt.
...
Done. :-)  It'll be in the reroll I submit.

> I think I said this in my earlier review, and this is not limited to 05/48
> but also applies to 03/48 and 04/48 as well, but there won't be perfect
> rename detection, and the rename detection logic could change (improve)
> over time. Ideally, I think the test should declare either case as a
> success: (1) detection succeeds and avoids unnecessary conflict, or (2)
> rename is missed and conflict is reported, but otherwise there is no data
> loss. If it expects only one but not the other, any time the rename logic
> is improved, the behaviour could change between (1) and (2) and it will
> cause a false positive "breakage" of these tests.

The same could be said for all the rename tests, but I'm a bit worried
about doing this, particularly for the set of tests I added.  I came
up with my testcases by carefully reviewing the code looking for ways
I could trigger problems (which were almost never covered by an
existing regression test), plus trying a few small modifications of
any tests I did come up with.  The tests I added that involve renames
really are specifically designed to test renaming situations.  If
rename detection logic is modified in a way that affects these tests,
and these tests aren't updated, then we are turning some useful unique
tests into mere duplication of simpler tests we have elsewhere.

If you are worried that these tests lack robustness in the face of
rename-detection changes, perhaps we should modify the tests to use
file contents that are more likely to continue being detected as
renames (e.g. longer lines or maybe more of them)?  Or, maybe add an
earlier check in the test to independently verify that the rename is
detected?