Re: [PATCH 09/30] directory rename detection: testcases checking which side did the rename

[Elijah Newren <newren@gmail.com>]

On Mon, Nov 13, 2017 at 4:25 PM, Stefan Beller <sbeller@google.com> wrote:
> On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren <newren@gmail.com> wrote:
>> +# Testcase 6b, Same rename done on both sides
>> +#   (Related to testcases 6c and 8e)
>> +#   Commit A: z/{b,c}
>> +#   Commit B: y/{b,c}
>> +#   Commit C: y/{b,c}, z/d
>
> Missing expected state

Oops!

>> +#   Note: If we did directory rename detection here, we'd move z/d into y/,
>> +#         but C did that rename and still decided to put the file into z/,
>> +#         so we probably shouldn't apply directory rename detection for it.
>
> correct. Also we don't want to see a rename/rename conflict (obviously).

Interestingly, (and this is unrelated to directory rename detection)
merge-recursive.c has a RENAME_ONE_FILE_TO_ONE value exactly for the
case where one file was renamed on both sides of history, but was
renamed to the exact same thing on both sides.  And in those cases, it
turns what would otherwise be a rename/rename(1to2) conflict into
essentially a RENAME_NORMAL case.  So, we wouldn't have to worry about
a rename/rename conflict anyway.

>> +# Testcase 6e, Add/add from one-side
>> +#   Commit A: z/{b,c}
>> +#   Commit B: z/{b,c} (no change)
>> +#   Commit C: y/{b,c,d_1}, z/d_2
>> +#   Expected: y/{b,c,d_1}, z/d_2
>> +#   NOTE: Again, this seems obvious but just checking that the implementation
>> +#         doesn't "accidentally detect a rename" and give us y/{b,c} +
>> +#         add/add conflict on y/d_1 vs y/d_2.
>
> What is less obvious in all these cases is the "(no change)" part to me.
> I would think that at least *something* changes in B in all cases above, maybe
> add file u/r (un-related) to have the tree ids changed?
> ("Less obvious" as in: we don't rely on the "no changes" part to make
> the decision,
> which sounds tempting so far)

Yes, I could have introduced unrelated changes into the test, and my
assumption is that the real world testcase would have such, it's just
that in paring down to a minimal testcase I end up with a "no change"
commit.

>>  test_done
>
> No conclusion box here, so my (misguided) suggestion:

Yeah, the conclusion was actually in the summary.  I could potentially
restate it here:

"Only apply implicit directory renames to directories if the _other_
side of history is the one doing the renaming"

I can add that.