Re: [PATCH 10/30] directory rename detection: more involved edge/corner testcases

From: Elijah Newren <newren@gmail.com>
Date: Nov 14, 2017, 21:11 UTC
Message-ID: <CABPp-BE7qKiYBNxMquXRv+Gr=sA3jGs8JyKuqNSzAwKbOWOQ=Q@mail.gmail.com>
In-Reply-To: <CAGZ79kaDWxx2g2OMY-VKJZD0-0t7V33644aqPLHq5Hppo_Be+w@mail.gmail.com>

On Mon, Nov 13, 2017 at 4:42 PM, Stefan Beller <sbeller@google.com> wrote:

> On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren <newren@gmail.com> wrote:

> "In my opinion" ... sounds like commit message?

Sure, I can move it there.

Show 10 quoted lines

>> +# Testcase 7a, rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file
>> +#   Commit A: z/{b,c}
>> +#   Commit B: y/{b,c}
>> +#   Commit C: w/b, x/c, z/d
>> +#   Expected: y/d, CONFLICT(rename/rename for both z/b and z/c)
>> +#   NOTE: There's a rename of z/ here, y/ has more renames, so z/d -> y/d.
>
> But the creator of C intended to have z/d, not {w,x}/d, and as {w,x} == y,
> I am not sure I like this result. (I have no concrete counter example, just
> messy logic)

I'm open to alternative interpretations here. The biggest issue for me -- going back our discussion at the end of https://public-inbox.org/git/CABPp-BFKiam6AK-Gg_RzaLuLur-jz0kvv3TqsHNHg5+HTv_uzA@mail.gmail.com/ -- is "simple, predictable rule", which is consistent with the other rules and limits the number of nasty corner cases as much as possible. Perhaps you think this is one of those nasty corner cases, and that's fair, but I think it'd be hard to do much better.

After spending quite a while trying to think of any other alternative rules or ways of looking at this, I could only come up with two points:

  1) One could view this as a case where commit C didn't in fact do
any directory rename -- note that directory z/ still exists in that
commit.  Thus, only B did a rename, it renamed z/ -> y/, thus C's z/d
should be moved to y/d.  So, this choice is consistent with the other
rules we've got.

  2) An alternate (or maybe additional?) rule: We could decide that if
a source path is renamed on both sides of history, then we'll just
ignore both renames for consideration of directory rename detection.

The new rule idea would "fix" this testcase to your liking, although now we'd be somewhat inconsistent with the "directory still exists implies no directory rename occurred rule". But what other weirdness could entail? Here's a few I've thought of:

Commit O: z/{b,c,d} Commit A: y/{b,c} Commit B: z/{newb, newc, e}

Here, A renamed z/ -> y/. Except B renamed z/b and z/c differently, so all paths used to detect the z/ -> y/ rename are ignored, so there isn't a rename after all. I'm not so sure I like that decision. Let's keep looking though, and change it up a bit more:

Commit O: z/{b,c,d} Commit A: y/{b,c}, x/d Commit B: z/{newb, newc, d, e}

Here, A has a split rename. Since B renames z/b and z/c differently, we have to ignore the z/ -> y/ rename, and thus the only rename left implies z/ -> x/. Thus we'd end up with z/e getting moved into x/e. Seems weird to me, and less likely that a user would understand this rule than the "majority wins" one.

Show 8 quoted lines

>> +# Testcase 7c, rename/rename(1to...2or3); transitive rename may add complexity
>> +#   (Related to testcases 3b and 5c)
>> +#   Commit A: z/{b,c}, x/d
>> +#   Commit B: y/{b,c}, w/d
>> +#   Commit C: z/{b,c,d}
>> +#   Expected: y/{b,c}, CONFLICT(x/d -> w/d vs. y/d)
>
> CONFLICT(x/d -> y/d vs w/d) ?

I'm afraid I'm not following the question.

Show 6 quoted lines

>
>> +#   NOTE: z/ was renamed to y/ so we do not want to report
>> +#         either CONFLICT(x/d -> w/d vs. z/d)
>> +#         or CONFLiCT x/d -> w/d vs. y/d vs. z/d)
>
> "neither ... nor" instead of "not either or"?

Yes, thanks.

Show 18 quoted lines

>> +# Testcase 7e, transitive rename in rename/delete AND dirs in the way
>> +#   (Very similar to 'both rename source and destination involved in D/F conflict' from t6022-merge-rename.sh)
>> +#   (Also related to testcases 9c and 9d)
>> +#   Commit A: z/{b,c},     x/d_1
>> +#   Commit B: y/{b,c,d/g}, x/d/f
>> +#   Commit C: z/{b,c,d_1}
>> +#   Expected: rename/delete(x/d_1->y/d_1 vs. None) + D/F conflict on y/d
>> +#             y/{b,c,d/g}, y/d_1~C^0, x/d/f
>> +#   NOTE: x/d/f may be slightly confusing here.  x/d_1 -> z/d_1 implies
>> +#         there is a directory rename from x/ -> z/, performed by commit C.
>> +#         However, on the side of commit B, it renamed z/ -> y/, thus
>> +#         making a rename from x/ -> z/ when it was getting rid of z/ seems
>> +#         non-sensical.  Further, putting x/d/f into y/d/f also doesn't
>> +#         make a lot of sense because commit B did the renaming of z to y
>> +#         and it created x/d/f, and it clearly made these things separate,
>> +#         so it doesn't make much sense to push these together.
>
> This is confusing.

Indeed it is. When I first wrote this testcase, I didn't realize that I actually had two potentially directory renames involved and a doubly-transitive rename from it, on top of the D/F conflict. I can see two ways to resolve this.

1) Leave the testcase alone, just try to make the NOTE more clear:

NOTE: The main path of interest here is d_1 and where it ends up, but
this is actually a case that has two potential directory renames
involved and D/F conflict(s), so it makes sense to walk through each
step.  Commit B renames z/ -> y/.  Thus everything that C adds to z/
should be instead moved to y/.  This gives us the D/F conflict on y/d
because x/d_1 -> z/d_1 -> y/d_1 conflicts with y/d/g.  Further, commit
C renames x/ -> z/, thus everything B adds to x/ should instead be
moved to z/...BUT we removed z/ and renamed it to y/, so maybe
everything should move not from x/ to z/, but from x/ to z/ to y/.
Doing so might make sense from the logic so far, but note that commit
B had both an x/ and a y/; it did the renaming of z/ to y/ and created
x/d/f and it clearly made these things separate, so it doesn't make
much sense to push these together.  Doing so is what I'd call a doubly
transitive rename; see testcases 9c and 9d for further discussion of
this issue and how it's resolved.

2) Modify the testcase so it doesn't have two potential directory
renames involved.  Just add another unrelated file under x/ that
doesn't change on either side, thus removing the x/ -> z/ rename from
the mix.  That wouldn't actually change the expected result (other
than the new file should remain around), but it would change the
reasoning and simplify it:

NOTE: Commit B renames z/ -> y/.  Thus everything that C adds to z/
should be instead moved to y/.  This gives us the D/F conflict on y/d
because x/d_1 -> z/d_1 -> y/d_1 conflicts with y/d/g.  As a side note,
one could imagine an alternative implementation trying to resolve D/F
conflicts caused by renames by just undoing the rename, but in this
case that would end up with us needing to write an x/d_1, which would
still be a D/F conflict with x/d/f.