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@xxxxxxxxx> 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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html