git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Emily Shaffer <emilyshaffer@google.com>
Date
Aug 6, 2019, 20:28 UTC
Message-ID
<20190806202829.GB26909@google.com>
In-Reply-To
<20190805223350.27504-1-newren@gmail.com>
On Mon, Aug 05, 2019 at 03:33:50PM -0700, Elijah Newren wrote:
Show 33 quoted lines
> 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. :)

Show 14 quoted lines
> 
>  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.

Show 74 quoted lines
>  
> 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...
Show 6 quoted lines
> +
> +		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.
Show 9 quoted lines
> +
> +		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?

Show 12 quoted lines
> +		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.
Show 5 quoted lines
> +		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.
Show 7 quoted lines
> +	)
> +'
> +
>  test_done
> -- 
> 2.22.0.246.g5ddf3d502a
>
Previous: Elijah NewrenNext: Elijah Newren
Message 8 of 11 in “BUG() during criss-cross merge with directory rename and deleted file”
  1. Emily ShafferJul 26, 2019
  2. 1/1 merge-recursive: avoid directory rename detection in recursive caseElijah Newren, Aug 5, 2019
  3. Junio C HamanoAug 6, 2019
  4. Elijah NewrenAug 6, 2019
  5. Junio C HamanoAug 6, 2019
  6. Junio C HamanoAug 6, 2019
  7. Elijah NewrenAug 6, 2019
  8. Emily ShafferAug 6, 2019
  9. Elijah NewrenAug 6, 2019
  10. Emily ShafferAug 6, 2019
  11. Jeff KingAug 8, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.