Re: [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case

[Elijah Newren <newren@gmail.com>]

On Tue, Aug 6, 2019 at 1:28 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> On Mon, Aug 05, 2019 at 03:33:50PM -0700, Elijah Newren wrote:
> > Ever since commit 8c8e5bd6eb33 ("merge-recursive: switch directory
> > rename detection default", 2019-04-05), the default handling with
> > directory rename detection was to report a conflict and leave unstaged
> > entries in the index.  However, when creating a virtual merge base in
> > the recursive case, we absolutely need a tree, and the only way a tree
> > can be written is if we have no unstaged entries -- otherwise we hit a
> > BUG().
...
> Thanks for picking this up and sorry I didn't end up sending anything -
> priority shifts on this end. :)

I totally understand.

...
> > +             git checkout C &&
> > +             test_must_fail git -c merge.directoryRenames=conflict merge B &&
> > +             git add b/x &&
> > +             test_tick &&
> > +             git commit -m "C" &&
>
> Then we merge C with B which places B as a mutual ancestor of D as well
> as C.
>
> > +
> > +
> > +             git checkout D &&
> > +             test_must_fail git -c merge.directoryRenames=conflict merge A &&
>
> Now we do the same thing merging A with D, which means that D has
> ancestors B from branching and A from merge, and C has ancestors A from
> branching and B from merge. So D and C have two closest ancestors
> (criss-cross merge).
>
> > +             git add b/x &&
> > +             mkdir a &&
> > +             git mv b/x a/x &&
>
> Now D adds contention over a/x and b/x (which were both mentioned in the
> ancestry too) to induce a conflict... or is this adding a resolution
> which can be decided on automatically? I guess later you are looking to
> make sure no CONFLICT still exists in the output, so you must be
> resolving the conflict here?

Yes, we are resolving the conflict for D by choosing to reject the
directory rename, placing 'x' in a/x instead of b/x.

When merge.directoryRenames=conflict, it'll make only b/x be present
in the working directory and mark it as conflicted (i.e. assume the
directory rename is probably the right resolution, but print a warning
and mark the index as needing to be updated to verify -- this allows
e.g. "git add -u" to do the "right thing").  For commit C, we just did
a "git add b/x" to accept the directory rename.  For commit D, we
wanted to say we didn't want the directory rename which you'd first
guess would be "git mv b/x a/x" BUT: (1) a/x has unstaged entries
which will cause git-mv to fail, and (2) directory a/ didn't exist --
both of these issues had to be corrected before running git-mv.

> > +             test_tick &&
> > +             git commit -m "D"
> > +     )
> > +'
> > +
> > +test_expect_success '13e-check: directory rename detection in recursive case' '
> > +     (
> > +             cd 13e &&
> > +
> > +             git checkout --quiet D^0 &&
> > +
> > +             git -c merge.directoryRenames=conflict merge -s recursive C^0 >out 2>err &&
>
> Now we finally do the recursive merge - C and D have equally likely
> ancestors A and B, and A and B have a rename conflict.
>
> > +
> > +             test_i18ngrep ! CONFLICT out &&
> > +             test_i18ngrep ! BUG: err &&
>
> The BUG is gone. But should it not use i18ngrep? BUG() isn't localized.

Technically, yes, you're right. However, this line's purpose isn't
correctness of the test but documentation for the person reading the
testcase about what it's real original purpose was; my real test was
the "test_must_be_empty err" check I have below it, but I added this
line just to document the intent better.  I kind of like the
"CONFLICT" and "BUG" lines looking similar just so the reader of the
testcase doesn't have to try to reason through why they are different,
although I guess it does present the problem that more careful readers
like yourself might do a double take.

If folks find it more readable to use regular grep instead of
test_i18ngrep, I can change this line as well as the "core dumped"
check immediately below over to regular grep.

> > +             test_i18ngrep ! core.dumped err &&
> > +             test_must_be_empty err &&
> > +
> > +             git ls-files >paths &&
> > +             ! grep a/x paths &&
> Finally, make sure that a/x has been truly disappeared...
>
> > +             grep b/x paths
> ...and b/x is the only x left standing.


Thanks for taking a look.  :-)