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

[Emily Shaffer <emilyshaffer@google.com>]

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().
> 
> There are a few fixes possible here which at least fix the BUG(), but
> none of them seem optimal for other reasons; see the comments with the
> new testcase 13e in t6043 for details (which testcase triggered a BUG()
> prior to this patch).  As such, just opt for a very conservative and
> simple choice that is still relatively reasonable: have the recursive
> case treat 'conflict' as 'false' for opt->detect_directory_renames.
> 
> Reported-by: Emily Shaffer <emilyshaffer@google.com>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> 
> I really should introduce constants like
>   DETECT_DIRECTORY_RENAMES_NEVER    = 0
>   DETECT_DIRECTORY_RENAMES_CONFLICT = 1
>   DETECT_DIRECTORY_RENAMES_YES      = 2
> and then use them in the code to make it clearer, but I wanted to make
> the code change as simple and contained as possible given that this is
> built on maint, fixes a BUG() and we're already in -rc1.
> 
> I know this bug doesn't satisfy the normal criteria for making it into
> 2.23 (it's a bug that was present in 2.22 rather than a regression in
> 2.23), but given that it's a BUG() condition, I was hoping it is
> important and safe enough to include anyway.
> 
> (This fix does merge down cleanly to master, next, and pu.)

Thanks for picking this up and sorry I didn't end up sending anything -
priority shifts on this end. :)

> 
>  merge-recursive.c                   |   3 +-
>  t/t6043-merge-rename-directories.sh | 111 ++++++++++++++++++++++++++++
>  2 files changed, 113 insertions(+), 1 deletion(-)
> 
> diff --git a/merge-recursive.c b/merge-recursive.c
> index d2e380b7ed..c7691d9b54 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2856,7 +2856,8 @@ static int detect_and_process_renames(struct merge_options *opt,
>  	head_pairs = get_diffpairs(opt, common, head);
>  	merge_pairs = get_diffpairs(opt, common, merge);
>  
> -	if (opt->detect_directory_renames) {
So we used to say, "If you want to apply directory renames, or mark
directory renames as merge conflicts, we will go look for renames no
matter what."

> +	if ((opt->detect_directory_renames == 2) ||
> +	    (opt->detect_directory_renames == 1 && !opt->call_depth)) {
But now we say, "If you want to apply directory renames always, or if
you want to mark them as conflicts AND you aren't below the first layer
of a recursive merge, then we will go look for renames."

>  		dir_re_head = get_directory_renames(head_pairs);
>  		dir_re_merge = get_directory_renames(merge_pairs);
That means that when we usually prefer to mark directory renames as
conflicts and we are putting together a virtual ancestor, we
don't try to detect renames at all, which I imagine leaves it to the top
level merge to mark as conflicts for the user to resolve much later.
>  
> diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
> index 50b7543483..c966147d5d 100755
> --- a/t/t6043-merge-rename-directories.sh
> +++ b/t/t6043-merge-rename-directories.sh
> @@ -4403,4 +4403,115 @@ test_expect_success '13d-check(info): messages for rename/rename(1to1) via dual
>  	)
>  '
>  
> +# Testcase 13e, directory rename in virtual merge base
> +#
> +# This testcase has a slightly different setup than all the above cases, in
> +# order to include a recursive case:
> +#
> +#      A   C
> +#      o - o
> +#     / \ / \
> +#  O o   X   ?
> +#     \ / \ /
> +#      o   o
> +#      B   D
> +#
> +#   Commit O: a/{z,y}
> +#   Commit A: b/{z,y}
> +#   Commit B: a/{z,y,x}
> +#   Commit C: b/{z,y,x}
> +#   Commit D: b/{z,y}, a/x
> +#   Expected: b/{z,y,x}  (sort of; see below for why this might not be expected)
> +#
> +#   NOTES: 'X' represents a virtual merge base.  With the default of
> +#          directory rename detection yielding conflicts, merging A and B
> +#          results in a conflict complaining about whether 'x' should be
> +#          under 'a/' or 'b/'.  However, when creating the virtual merge
> +#          base 'X', since virtual merge bases need to be written out as a
> +#          tree, we cannot have a conflict, so some resolution has to be
> +#          picked.
> +#
> +#          In choosing the right resolution, it's worth noting here that
> +#          commits C & D are merges of A & B that choose different
> +#          locations for 'x' (i.e. they resolve the conflict differently),
> +#          and so it would be nice when merging C & D if git could detect
> +#          this difference of opinion and report a conflict.  But the only
> +#          way to do so that I can think of would be to have the virtual
> +#          merge base place 'x' in some directory other than either 'a/' or
> +#          'b/', which seems a little weird -- especially since it'd result
> +#          in a rename/rename(1to2) conflict with a source path that never
> +#          existed in any version.
> +#
> +#          So, for now, when directory rename detection is set to
> +#          'conflict' just avoid doing directory rename detection at all in
> +#          the recursive case.  This will not allow us to detect a conflict
> +#          in the outer merge for this special kind of setup, but it at
> +#          least avoids hitting a BUG().
> +#
> +test_expect_success '13e-setup: directory rename detection in recursive case' '
> +	test_create_repo 13e &&
> +	(
> +		cd 13e &&
> +
> +		mkdir a &&
> +		echo z >a/z &&
> +		echo y >a/y &&
> +		git add a &&
> +		test_tick &&
> +		git commit -m "O" &&
> +
> +		git branch O &&
> +		git branch A &&
> +		git branch B &&
> +
> +		git checkout A &&
> +		git mv a/ b/ &&
> +		test_tick &&
> +		git commit -m "A" &&
So we do a directory rename in branch A...

> +
> +		git checkout B &&
> +		echo x >a/x &&
> +		git add a &&
> +		test_tick &&
> +		git commit -m "B" &&
... And we add a new file to the directory in question in branch B...

So A and B will be the ones combined to make a virtual ancestor.
> +
> +		git branch C A &&
> +		git branch D B &&
> +
> +		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?

> +		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.

> +		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.
> +	)
> +'
> +
>  test_done
> -- 
> 2.22.0.246.g5ddf3d502a
>