threads / patch / 56010

patch, 3 partst6423: test directory renames causing rename-to-self

Subject: [PATCH 1/3] t6423: test directory renames causing rename-to-self

## tl;dr

12 messages between Jun 26, 2021 and Jun 30, 2021. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Elijah Newren via GitGitGadget· Jun 26, 2021, 17:05 UTC · lore

[PATCH 0/3] Fix bugs from interesting renaming pairs: one side renames A/file -> B/file, the other B/ -> A/

Anders Kaseorg recently reported a few issues in an interesting rename case[1]. I was able to duplicate and find multiple bugs from it; two in merge-recursive, and one in merge-ort. This series has some fixes.

[1] https://lore.kernel.org/git/CABPp-BGDfucqae=HNES_QmmsjpDbdHrR6CG=H3gtiDygHzquVg@mail.gmail.com/

Elijah Newren (3):
  t6423: test directory renames causing rename-to-self
  merge-ort: ensure we consult df_conflict and path_conflicts
  merge-recursive: handle rename-to-self case

 merge-ort.c                         |   6 +-
 merge-recursive.c                   |  19 +++--
 t/t6423-merge-rename-directories.sh | 117 ++++++++++++++++++++++++++++
 3 files changed, 135 insertions(+), 7 deletions(-)

base-commit: 670b81a890388c60b7032a4f5b879f2ece8c4558
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1039%2Fnewren%2Frename-plus-dir-rename-cancel-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1039/newren/rename-plus-dir-rename-cancel-v1
Pull-Request: https://github.com/git/git/pull/1039

-- 
gitgitgadget

Elijah Newren via GitGitGadget· Jun 26, 2021, 17:05 UTC · re: Elijah Newren via GitGitGadget · lore

From: Elijah Newren <newren@gmail.com>

Directory rename detection can cause transitive renames, e.g. if the two
different sides of history each do one half of:
    A/file -> B/file
    B/     -> C/
then directory rename detection transitively renames to give us C/file.
Since the default for merge.directoryRenames is conflict, this results
in an error message saying it is unclear whether the file should be
placed at B/file or C/file.

What if C/ is A/, though? In such a case, the transitive rename would give us A/file, the original name we started with. Logically, having an error message with B/file vs. A/file should be fine, as should leaving the file where it started. But the logic in both merge-recursive and merge-ort did not handle a case of a filename being renamed to itself correctly; merge-recursive had two bugs, and merge-ort had one. Add some testcases covering such a scenario.

Based-on-testcase-by: Anders Kaseorg <andersk@mit.edu>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6423-merge-rename-directories.sh | 117 ++++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)

Show changes to t/t6423-merge-rename-directories.sh +117 −0

diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index be84d22419d..2a2ab907338 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -5024,6 +5024,123 @@ test_expect_failure '12h: renaming a file within a renamed directory' '
 	)
 '
 
+# Testcase 12i, Directory rename causes rename-to-self
+#   Commit O: source/{subdir/foo, bar, baz_1}
+#   Commit A: source/{foo, bar, baz_1}
+#   Commit B: source/{subdir/{foo, bar}, baz_2}
+#   Expected: source/{foo, bar, baz_2}, with conflicts on
+#                source/bar vs. source/subdir/bar
+
+test_setup_12i () {
+	test_create_repo 12i &&
+	(
+		cd 12i &&
+
+		mkdir -p source/subdir &&
+		echo foo >source/subdir/foo &&
+		echo bar >source/bar &&
+		echo baz >source/baz &&
+		git add source &&
+		git commit -m orig &&
+
+		git branch O &&
+		git branch A &&
+		git branch B &&
+
+		git switch A &&
+		git mv source/subdir/foo source/foo &&
+		git commit -m A &&
+
+		git switch B &&
+		git mv source/bar source/subdir/bar &&
+		echo more baz >>source/baz &&
+		git commit -m B
+	)
+}
+
+test_expect_merge_algorithm failure failure '12i: Directory rename causes rename-to-self' '
+	test_setup_12i &&
+	(
+		cd 12i &&
+
+		git checkout A^0 &&
+
+		test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&
+
+		test_path_is_missing source/subdir &&
+		test_path_is_file source/bar &&
+		test_path_is_file source/baz &&
+
+		git ls-files | uniq >tracked &&
+		test_line_count = 3 tracked &&
+
+		git status --porcelain -uno >actual &&
+		cat >expect <<-\EOF &&
+		UU source/bar
+		 M source/baz
+		EOF
+		test_cmp expect actual
+	)
+'
+
+# Testcase 12j, Directory rename to root causes rename-to-self
+#   Commit O: {subdir/foo, bar, baz_1}
+#   Commit A: {foo, bar, baz_1}
+#   Commit B: {subdir/{foo, bar}, baz_2}
+#   Expected: {foo, bar, baz_2}, with conflicts on bar vs. subdir/bar
+
+test_setup_12j () {
+	test_create_repo 12j &&
+	(
+		cd 12j &&
+
+		mkdir -p subdir &&
+		echo foo >subdir/foo &&
+		echo bar >bar &&
+		echo baz >baz &&
+		git add . &&
+		git commit -m orig &&
+
+		git branch O &&
+		git branch A &&
+		git branch B &&
+
+		git switch A &&
+		git mv subdir/foo foo &&
+		git commit -m A &&
+
+		git switch B &&
+		git mv bar subdir/bar &&
+		echo more baz >>baz &&
+		git commit -m B
+	)
+}
+
+test_expect_merge_algorithm failure failure '12j: Directory rename to root causes rename-to-self' '
+	test_setup_12j &&
+	(
+		cd 12j &&
+
+		git checkout A^0 &&
+
+		test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&
+
+		test_path_is_missing subdir &&
+		test_path_is_file bar &&
+		test_path_is_file baz &&
+
+		git ls-files | uniq >tracked &&
+		test_line_count = 3 tracked &&
+
+		git status --porcelain -uno >actual &&
+		cat >expect <<-\EOF &&
+		UU bar
+		 M baz
+		EOF
+		test_cmp expect actual
+	)
+'
+
 ###########################################################################
 # SECTION 13: Checking informational and conflict messages
 #

-- 
gitgitgadget

Derrick Stolee· Jun 29, 2021, 12:50 UTC · re: Elijah Newren via GitGitGadget · lore

Re: [PATCH 1/3] t6423: test directory renames causing rename-to-self

On 6/26/2021 1:05 PM, Elijah Newren via GitGitGadget wrote:

Show 12 quoted lines

> From: Elijah Newren <newren@gmail.com>
> 
> Directory rename detection can cause transitive renames, e.g. if the two
> different sides of history each do one half of:
>     A/file -> B/file
>     B/     -> C/
> then directory rename detection transitively renames to give us C/file.
> Since the default for merge.directoryRenames is conflict, this results
> in an error message saying it is unclear whether the file should be
> placed at B/file or C/file.
> 
> What if C/ is A/, though?

This case seems interesting, but somehow missing from the test cases below. Each of those cases include renaming up or down the directory hierarchy instead of doing a sideways rename.

Show 6 quoted lines

> +# Testcase 12i, Directory rename causes rename-to-self
> +#   Commit O: source/{subdir/foo, bar, baz_1}
> +#   Commit A: source/{foo, bar, baz_1}
> +#   Commit B: source/{subdir/{foo, bar}, baz_2}
> +#   Expected: source/{foo, bar, baz_2}, with conflicts on
> +#                source/bar vs. source/subdir/bar

This test goes deeper.

Show 5 quoted lines

> +# Testcase 12j, Directory rename to root causes rename-to-self
> +#   Commit O: {subdir/foo, bar, baz_1}
> +#   Commit A: {foo, bar, baz_1}
> +#   Commit B: {subdir/{foo, bar}, baz_2}
> +#   Expected: {foo, bar, baz_2}, with conflicts on bar vs. subdir/bar

This test goes higher.

Does the problematic case not hit when going out to the side, such as "with conflicts on subdir/bar vs otherdir/bar"?

If so, then _maybe_ the commit message could indicate this is an omission on purpose. If not, then maybe a third test should be added?

Thanks, -Stolee

Elijah Newren· Jun 30, 2021, 16:33 UTC · re: Derrick Stolee · lore

Re: [PATCH 1/3] t6423: test directory renames causing rename-to-self

On Tue, Jun 29, 2021 at 5:50 AM Derrick Stolee <stolee@gmail.com> wrote:

Show 42 quoted lines

>
> On 6/26/2021 1:05 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Directory rename detection can cause transitive renames, e.g. if the two
> > different sides of history each do one half of:
> >     A/file -> B/file
> >     B/     -> C/
> > then directory rename detection transitively renames to give us C/file.
> > Since the default for merge.directoryRenames is conflict, this results
> > in an error message saying it is unclear whether the file should be
> > placed at B/file or C/file.
> >
> > What if C/ is A/, though?
>
> This case seems interesting, but somehow missing from the test cases
> below. Each of those cases include renaming up or down the directory
> hierarchy instead of doing a sideways rename.
>
> > +# Testcase 12i, Directory rename causes rename-to-self
> > +#   Commit O: source/{subdir/foo, bar, baz_1}
> > +#   Commit A: source/{foo, bar, baz_1}
> > +#   Commit B: source/{subdir/{foo, bar}, baz_2}
> > +#   Expected: source/{foo, bar, baz_2}, with conflicts on
> > +#                source/bar vs. source/subdir/bar
>
> This test goes deeper.
>
> > +# Testcase 12j, Directory rename to root causes rename-to-self
> > +#   Commit O: {subdir/foo, bar, baz_1}
> > +#   Commit A: {foo, bar, baz_1}
> > +#   Commit B: {subdir/{foo, bar}, baz_2}
> > +#   Expected: {foo, bar, baz_2}, with conflicts on bar vs. subdir/bar
>
> This test goes higher.
>
> Does the problematic case not hit when going out to the side, such
> as "with conflicts on subdir/bar vs otherdir/bar"?
>
> If so, then _maybe_ the commit message could indicate this is an
> omission on purpose. If not, then maybe a third test should be
> added?

The only special case in the code is renaming to the root directory, so I didn't think of extending beyond two cases. However, while adding more testcases doesn't exercise the current incarnation of the code further, it's pretty easy to add another and it certainly doesn't hurt. I'll resubmit with a third testcase.

Elijah Newren via GitGitGadget· Jun 26, 2021, 17:05 UTC · re: Elijah Newren via GitGitGadget · lore

[PATCH 2/3] merge-ort: ensure we consult df_conflict and path_conflicts

From: Elijah Newren <newren@gmail.com>

Path conflicts (typically rename path conflicts, e.g. rename/rename(1to2) or rename/add/delete), and directory/file conflicts should obviously result in files not being marked as clean in the merge. We had a codepath where we missed consulting the path_conflict and df_conflict flags, based on match_mask. Granted, it requires an unusual setup to trigger this codepath (directory rename causing rename-to-self is the only case I can think of), but we still need to handle it. To make it clear that we have audited the other codepaths that do not explicitly mention these flags, add some assertions that the flags are not set.

Reported-by: Anders Kaseorg <andersk@mit.edu>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c                         | 6 +++++-
 t/t6423-merge-rename-directories.sh | 4 ++--
 2 files changed, 7 insertions(+), 3 deletions(-)

Show changes to 2 files +7 −3

merge-ort.c, t/t6423-merge-rename-directories.sh

diff --git a/merge-ort.c b/merge-ort.c
index b954f7184a5..373dbac5079 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -3237,7 +3237,7 @@ static void process_entry(struct merge_options *opt,
 	 *       above.
 	 */
 	if (ci->match_mask) {
-		ci->merged.clean = 1;
+		ci->merged.clean = !ci->df_conflict && !ci->path_conflict;
 		if (ci->match_mask == 6) {
 			/* stages[1] == stages[2] */
 			ci->merged.result.mode = ci->stages[1].mode;
@@ -3249,6 +3249,8 @@ static void process_entry(struct merge_options *opt,
 
 			ci->merged.result.mode = ci->stages[side].mode;
 			ci->merged.is_null = !ci->merged.result.mode;
+			if (ci->merged.is_null)
+				ci->merged.clean = 1;
 			oidcpy(&ci->merged.result.oid, &ci->stages[side].oid);
 
 			assert(othermask == 2 || othermask == 4);
@@ -3421,6 +3423,7 @@ static void process_entry(struct merge_options *opt,
 				   path)) {
 			ci->merged.is_null = 1;
 			ci->merged.clean = 1;
+			assert(!ci->df_conflict && !ci->path_conflict);
 		} else if (ci->path_conflict &&
 			   oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {
 			/*
@@ -3447,6 +3450,7 @@ static void process_entry(struct merge_options *opt,
 		ci->merged.is_null = 1;
 		ci->merged.result.mode = 0;
 		oidcpy(&ci->merged.result.oid, null_oid());
+		assert(!ci->df_conflict);
 		ci->merged.clean = !ci->path_conflict;
 	}
 
diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index 2a2ab907338..7480daab46a 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -5058,7 +5058,7 @@ test_setup_12i () {
 	)
 }
 
-test_expect_merge_algorithm failure failure '12i: Directory rename causes rename-to-self' '
+test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' '
 	test_setup_12i &&
 	(
 		cd 12i &&
@@ -5116,7 +5116,7 @@ test_setup_12j () {
 	)
 }
 
-test_expect_merge_algorithm failure failure '12j: Directory rename to root causes rename-to-self' '
+test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' '
 	test_setup_12j &&
 	(
 		cd 12j &&

-- 
gitgitgadget

Elijah Newren via GitGitGadget· Jun 26, 2021, 17:05 UTC · re: Elijah Newren via GitGitGadget · lore

[PATCH 3/3] merge-recursive: handle rename-to-self case

From: Elijah Newren <newren@gmail.com>

Directory rename detection can cause transitive renames, e.g. if the two
different sides of history each do one half of:
    A/file -> B/file
    B/     -> C/
then directory rename detection transitively renames to give us
    A/file -> C/file

However, when C/ == A/, note that this gives us
    A/file -> A/file.

merge-recursive assumed that any rename D -> E would have D != E. While that is almost always true, the above is a special case where it is not. So we cannot do things like delete the rename source, we cannot assume that a file existing at path E implies a rename/add conflict and we have to be careful about what stages end up in the output.

This change feels a bit hackish. It took me surprisingly many hours to find, and given merge-recursive's design causing it to attempt to enumerate all combinations of edge and corner cases with special code for each combination, I'm worried there are other similar fixes needed elsewhere if we can just come up with the right special testcase. Perhaps an audit would rule it out, but I have not the energy. merge-recursive deserves to die, and since it is on its way out anyway, fixing this particular bug narrowly will have to be good enough.

Reported-by: Anders Kaseorg <andersk@mit.edu>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c                   | 19 +++++++++++++------
 t/t6423-merge-rename-directories.sh |  4 ++--
 2 files changed, 15 insertions(+), 8 deletions(-)

Show changes to 2 files +15 −8

merge-recursive.c, t/t6423-merge-rename-directories.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index d146bb116f7..c895145a8f5 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2804,12 +2804,19 @@ static int process_renames(struct merge_options *opt,
 			int renamed_stage = a_renames == renames1 ? 2 : 3;
 			int other_stage =   a_renames == renames1 ? 3 : 2;
 
+			/*
+			 * Directory renames have a funny corner case...
+			 */
+			int renamed_to_self = !strcmp(ren1_src, ren1_dst);
+
 			/* BUG: We should only remove ren1_src in the base
 			 * stage and in other_stage (think of rename +
 			 * add-source case).
 			 */
-			remove_file(opt, 1, ren1_src,
-				    renamed_stage == 2 || !was_tracked(opt, ren1_src));
+			if (!renamed_to_self)
+				remove_file(opt, 1, ren1_src,
+					    renamed_stage == 2 ||
+					    !was_tracked(opt, ren1_src));
 
 			oidcpy(&src_other.oid,
 			       &ren1->src_entry->stages[other_stage].oid);
@@ -2823,6 +2830,9 @@ static int process_renames(struct merge_options *opt,
 			    ren1->dir_rename_original_type == 'A') {
 				setup_rename_conflict_info(RENAME_VIA_DIR,
 							   opt, ren1, NULL);
+			} else if (renamed_to_self) {
+				setup_rename_conflict_info(RENAME_NORMAL,
+							   opt, ren1, NULL);
 			} else if (oideq(&src_other.oid, null_oid())) {
 				setup_rename_conflict_info(RENAME_DELETE,
 							   opt, ren1, NULL);
@@ -3180,7 +3190,6 @@ static int handle_rename_normal(struct merge_options *opt,
 	struct rename *ren = ci->ren1;
 	struct merge_file_info mfi;
 	int clean;
-	int side = (ren->branch == opt->branch1 ? 2 : 3);
 
 	/* Merge the content and write it out */
 	clean = handle_content_merge(&mfi, opt, path, was_dirty(opt, path),
@@ -3190,9 +3199,7 @@ static int handle_rename_normal(struct merge_options *opt,
 	    opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT &&
 	    ren->dir_rename_original_dest) {
 		if (update_stages(opt, path,
-				  NULL,
-				  side == 2 ? &mfi.blob : NULL,
-				  side == 2 ? NULL : &mfi.blob))
+				  &mfi.blob, &mfi.blob, &mfi.blob))
 			return -1;
 		clean = 0; /* not clean, but conflicted */
 	}
diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index 7480daab46a..d67b5c28944 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -5058,7 +5058,7 @@ test_setup_12i () {
 	)
 }
 
-test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' '
+test_expect_success '12i: Directory rename causes rename-to-self' '
 	test_setup_12i &&
 	(
 		cd 12i &&
@@ -5116,7 +5116,7 @@ test_setup_12j () {
 	)
 }
 
-test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' '
+test_expect_success '12j: Directory rename to root causes rename-to-self' '
 	test_setup_12j &&
 	(
 		cd 12j &&

-- 
gitgitgadget

Junio C Hamano· Jun 29, 2021, 04:02 UTC · re: Elijah Newren via GitGitGadget · lore

Re: [PATCH 3/3] merge-recursive: handle rename-to-self case

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 11 quoted lines

> From: Elijah Newren <newren@gmail.com>
>
> Directory rename detection can cause transitive renames, e.g. if the two
> different sides of history each do one half of:
>     A/file -> B/file
>     B/     -> C/
> then directory rename detection transitively renames to give us
>     A/file -> C/file
>
> However, when C/ == A/, note that this gives us
>     A/file -> A/file.

Heh, of course this is recent because the "guess that directory B has been moved to C in its entirety" heuristic is quite new.

Nicely fixed and described.  Will queue.  Thanks.

Show 100 quoted lines

>
> merge-recursive assumed that any rename D -> E would have D != E.  While
> that is almost always true, the above is a special case where it is not.
> So we cannot do things like delete the rename source, we cannot assume
> that a file existing at path E implies a rename/add conflict and we have
> to be careful about what stages end up in the output.
>
> This change feels a bit hackish.  It took me surprisingly many hours to
> find, and given merge-recursive's design causing it to attempt to
> enumerate all combinations of edge and corner cases with special code
> for each combination, I'm worried there are other similar fixes needed
> elsewhere if we can just come up with the right special testcase.
> Perhaps an audit would rule it out, but I have not the energy.
> merge-recursive deserves to die, and since it is on its way out anyway,
> fixing this particular bug narrowly will have to be good enough.
>
> Reported-by: Anders Kaseorg <andersk@mit.edu>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-recursive.c                   | 19 +++++++++++++------
>  t/t6423-merge-rename-directories.sh |  4 ++--
>  2 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index d146bb116f7..c895145a8f5 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2804,12 +2804,19 @@ static int process_renames(struct merge_options *opt,
>  			int renamed_stage = a_renames == renames1 ? 2 : 3;
>  			int other_stage =   a_renames == renames1 ? 3 : 2;
>  
> +			/*
> +			 * Directory renames have a funny corner case...
> +			 */
> +			int renamed_to_self = !strcmp(ren1_src, ren1_dst);
> +
>  			/* BUG: We should only remove ren1_src in the base
>  			 * stage and in other_stage (think of rename +
>  			 * add-source case).
>  			 */
> -			remove_file(opt, 1, ren1_src,
> -				    renamed_stage == 2 || !was_tracked(opt, ren1_src));
> +			if (!renamed_to_self)
> +				remove_file(opt, 1, ren1_src,
> +					    renamed_stage == 2 ||
> +					    !was_tracked(opt, ren1_src));
>  
>  			oidcpy(&src_other.oid,
>  			       &ren1->src_entry->stages[other_stage].oid);
> @@ -2823,6 +2830,9 @@ static int process_renames(struct merge_options *opt,
>  			    ren1->dir_rename_original_type == 'A') {
>  				setup_rename_conflict_info(RENAME_VIA_DIR,
>  							   opt, ren1, NULL);
> +			} else if (renamed_to_self) {
> +				setup_rename_conflict_info(RENAME_NORMAL,
> +							   opt, ren1, NULL);
>  			} else if (oideq(&src_other.oid, null_oid())) {
>  				setup_rename_conflict_info(RENAME_DELETE,
>  							   opt, ren1, NULL);
> @@ -3180,7 +3190,6 @@ static int handle_rename_normal(struct merge_options *opt,
>  	struct rename *ren = ci->ren1;
>  	struct merge_file_info mfi;
>  	int clean;
> -	int side = (ren->branch == opt->branch1 ? 2 : 3);
>  
>  	/* Merge the content and write it out */
>  	clean = handle_content_merge(&mfi, opt, path, was_dirty(opt, path),
> @@ -3190,9 +3199,7 @@ static int handle_rename_normal(struct merge_options *opt,
>  	    opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT &&
>  	    ren->dir_rename_original_dest) {
>  		if (update_stages(opt, path,
> -				  NULL,
> -				  side == 2 ? &mfi.blob : NULL,
> -				  side == 2 ? NULL : &mfi.blob))
> +				  &mfi.blob, &mfi.blob, &mfi.blob))
>  			return -1;
>  		clean = 0; /* not clean, but conflicted */
>  	}
> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
> index 7480daab46a..d67b5c28944 100755
> --- a/t/t6423-merge-rename-directories.sh
> +++ b/t/t6423-merge-rename-directories.sh
> @@ -5058,7 +5058,7 @@ test_setup_12i () {
>  	)
>  }
>  
> -test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' '
> +test_expect_success '12i: Directory rename causes rename-to-self' '
>  	test_setup_12i &&
>  	(
>  		cd 12i &&
> @@ -5116,7 +5116,7 @@ test_setup_12j () {
>  	)
>  }
>  
> -test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' '
> +test_expect_success '12j: Directory rename to root causes rename-to-self' '
>  	test_setup_12j &&
>  	(
>  		cd 12j &&

Derrick Stolee· Jun 29, 2021, 12:55 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] merge-recursive: handle rename-to-self case

On 6/29/2021 12:02 AM, Junio C Hamano wrote:

Show 18 quoted lines

> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Elijah Newren <newren@gmail.com>
>>
>> Directory rename detection can cause transitive renames, e.g. if the two
>> different sides of history each do one half of:
>>     A/file -> B/file
>>     B/     -> C/
>> then directory rename detection transitively renames to give us
>>     A/file -> C/file
>>
>> However, when C/ == A/, note that this gives us
>>     A/file -> A/file.
> 
> Heh, of course this is recent because the "guess that directory B
> has been moved to C in its entirety" heuristic is quite new.
> 
> Nicely fixed and described.  Will queue.  Thanks.

I agree that the fixes in patches 2 and 3 are rather clean. The tests added in patch 1 provide significant confidence in the result, though I had a minor question about a possible third test case.

Seeing the solutions that make the two tests pass leaves me to consider that the third test case is likely unnecessary, because there is nothing special about how the directories are being renamed that must be handled by the fix.

Thanks, -Stolee

Elijah Newren via GitGitGadget· Jun 30, 2021, 17:29 UTC · re: Elijah Newren via GitGitGadget · lore

[PATCH v2 0/3] Fix bugs from interesting renaming pairs: one side renames A/file -> B/file, the other B/ -> A/

Changes since v1:

 * Added a third testcase

[1] https://lore.kernel.org/git/CABPp-BGDfucqae=HNES_QmmsjpDbdHrR6CG=H3gtiDygHzquVg@mail.gmail.com/

Elijah Newren (3):
  t6423: test directory renames causing rename-to-self
  merge-ort: ensure we consult df_conflict and path_conflicts
  merge-recursive: handle rename-to-self case

 merge-ort.c                         |   6 +-
 merge-recursive.c                   |  19 ++-
 t/t6423-merge-rename-directories.sh | 175 ++++++++++++++++++++++++++++
 3 files changed, 193 insertions(+), 7 deletions(-)

base-commit: 670b81a890388c60b7032a4f5b879f2ece8c4558
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1039%2Fnewren%2Frename-plus-dir-rename-cancel-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1039/newren/rename-plus-dir-rename-cancel-v2
Pull-Request: https://github.com/git/git/pull/1039

Range-diff vs v1:

 1:  d3572e8bc85 ! 1:  86b2eaea75e t6423: test directory renames causing rename-to-self
     @@ t/t6423-merge-rename-directories.sh: test_expect_failure '12h: renaming a file w
      +		test_cmp expect actual
      +	)
      +'
     ++
     ++# Testcase 12k, Directory rename with sibling causes rename-to-self
     ++#   Commit O: dirB/foo, dirA/{bar, baz_1}
     ++#   Commit A: dirA/{foo, bar, baz_1}
     ++#   Commit B: dirB/{foo, bar}, dirA/baz_2
     ++#   Expected: dirA/{foo, bar, baz_2}, with conflicts on dirA/bar vs. dirB/bar
     ++
     ++test_setup_12k () {
     ++	test_create_repo 12k &&
     ++	(
     ++		cd 12k &&
     ++
     ++		mkdir dirA dirB &&
     ++		echo foo >dirB/foo &&
     ++		echo bar >dirA/bar &&
     ++		echo baz >dirA/baz &&
     ++		git add . &&
     ++		git commit -m orig &&
     ++
     ++		git branch O &&
     ++		git branch A &&
     ++		git branch B &&
     ++
     ++		git switch A &&
     ++		git mv dirB/* dirA/ &&
     ++		git commit -m A &&
     ++
     ++		git switch B &&
     ++		git mv dirA/bar dirB/bar &&
     ++		echo more baz >>dirA/baz &&
     ++		git commit -m B
     ++	)
     ++}
     ++
     ++test_expect_merge_algorithm failure failure '12k: Directory rename with sibling causes rename-to-self' '
     ++	test_setup_12k &&
     ++	(
     ++		cd 12k &&
     ++
     ++		git checkout A^0 &&
     ++
     ++		test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&
     ++
     ++		test_path_is_missing dirB &&
     ++		test_path_is_file dirA/bar &&
     ++		test_path_is_file dirA/baz &&
     ++
     ++		git ls-files | uniq >tracked &&
     ++		test_line_count = 3 tracked &&
     ++
     ++		git status --porcelain -uno >actual &&
     ++		cat >expect <<-\EOF &&
     ++		UU dirA/bar
     ++		 M dirA/baz
     ++		EOF
     ++		test_cmp expect actual
     ++	)
     ++'
      +
       ###########################################################################
       # SECTION 13: Checking informational and conflict messages
 2:  052f40c3c1a ! 2:  6e7ef18bf53 merge-ort: ensure we consult df_conflict and path_conflicts
     @@ t/t6423-merge-rename-directories.sh: test_setup_12j () {
       	test_setup_12j &&
       	(
       		cd 12j &&
     +@@ t/t6423-merge-rename-directories.sh: test_setup_12k () {
     + 	)
     + }
     + 
     +-test_expect_merge_algorithm failure failure '12k: Directory rename with sibling causes rename-to-self' '
     ++test_expect_merge_algorithm failure success '12k: Directory rename with sibling causes rename-to-self' '
     + 	test_setup_12k &&
     + 	(
     + 		cd 12k &&
 3:  dea97c25e52 ! 3:  7c96d6c7a0a merge-recursive: handle rename-to-self case
     @@ t/t6423-merge-rename-directories.sh: test_setup_12j () {
       	test_setup_12j &&
       	(
       		cd 12j &&
     +@@ t/t6423-merge-rename-directories.sh: test_setup_12k () {
     + 	)
     + }
     + 
     +-test_expect_merge_algorithm failure success '12k: Directory rename with sibling causes rename-to-self' '
     ++test_expect_success '12k: Directory rename with sibling causes rename-to-self' '
     + 	test_setup_12k &&
     + 	(
     + 		cd 12k &&

-- 
gitgitgadget

Elijah Newren via GitGitGadget· Jun 30, 2021, 17:29 UTC · re: Elijah Newren via GitGitGadget · lore

[PATCH v2 1/3] t6423: test directory renames causing rename-to-self

From: Elijah Newren <newren@gmail.com>

Directory rename detection can cause transitive renames, e.g. if the two
different sides of history each do one half of:
    A/file -> B/file
    B/     -> C/
then directory rename detection transitively renames to give us C/file.
Since the default for merge.directoryRenames is conflict, this results
in an error message saying it is unclear whether the file should be
placed at B/file or C/file.

Based-on-testcase-by: Anders Kaseorg <andersk@mit.edu>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6423-merge-rename-directories.sh | 175 ++++++++++++++++++++++++++++
 1 file changed, 175 insertions(+)

Show changes to t/t6423-merge-rename-directories.sh +175 −0

diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index be84d22419d..89fe6778b94 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -5024,6 +5024,181 @@ test_expect_failure '12h: renaming a file within a renamed directory' '
 	)
 '
 
+# Testcase 12i, Directory rename causes rename-to-self
+#   Commit O: source/{subdir/foo, bar, baz_1}
+#   Commit A: source/{foo, bar, baz_1}
+#   Commit B: source/{subdir/{foo, bar}, baz_2}
+#   Expected: source/{foo, bar, baz_2}, with conflicts on
+#                source/bar vs. source/subdir/bar
+
+test_setup_12i () {
+	test_create_repo 12i &&
+	(
+		cd 12i &&
+
+		mkdir -p source/subdir &&
+		echo foo >source/subdir/foo &&
+		echo bar >source/bar &&
+		echo baz >source/baz &&
+		git add source &&
+		git commit -m orig &&
+
+		git branch O &&
+		git branch A &&
+		git branch B &&
+
+		git switch A &&
+		git mv source/subdir/foo source/foo &&
+		git commit -m A &&
+
+		git switch B &&
+		git mv source/bar source/subdir/bar &&
+		echo more baz >>source/baz &&
+		git commit -m B
+	)
+}
+
+test_expect_merge_algorithm failure failure '12i: Directory rename causes rename-to-self' '
+	test_setup_12i &&
+	(
+		cd 12i &&
+
+		git checkout A^0 &&
+
+		test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&
+
+		test_path_is_missing source/subdir &&
+		test_path_is_file source/bar &&
+		test_path_is_file source/baz &&
+
+		git ls-files | uniq >tracked &&
+		test_line_count = 3 tracked &&
+
+		git status --porcelain -uno >actual &&
+		cat >expect <<-\EOF &&
+		UU source/bar
+		 M source/baz
+		EOF
+		test_cmp expect actual
+	)
+'
+
+# Testcase 12j, Directory rename to root causes rename-to-self
+#   Commit O: {subdir/foo, bar, baz_1}
+#   Commit A: {foo, bar, baz_1}
+#   Commit B: {subdir/{foo, bar}, baz_2}
+#   Expected: {foo, bar, baz_2}, with conflicts on bar vs. subdir/bar
+
+test_setup_12j () {
+	test_create_repo 12j &&
+	(
+		cd 12j &&
+
+		mkdir -p subdir &&
+		echo foo >subdir/foo &&
+		echo bar >bar &&
+		echo baz >baz &&
+		git add . &&
+		git commit -m orig &&
+
+		git branch O &&
+		git branch A &&
+		git branch B &&
+
+		git switch A &&
+		git mv subdir/foo foo &&
+		git commit -m A &&
+
+		git switch B &&
+		git mv bar subdir/bar &&
+		echo more baz >>baz &&
+		git commit -m B
+	)
+}
+
+test_expect_merge_algorithm failure failure '12j: Directory rename to root causes rename-to-self' '
+	test_setup_12j &&
+	(
+		cd 12j &&
+
+		git checkout A^0 &&
+
+		test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&
+
+		test_path_is_missing subdir &&
+		test_path_is_file bar &&
+		test_path_is_file baz &&
+
+		git ls-files | uniq >tracked &&
+		test_line_count = 3 tracked &&
+
+		git status --porcelain -uno >actual &&
+		cat >expect <<-\EOF &&
+		UU bar
+		 M baz
+		EOF
+		test_cmp expect actual
+	)
+'
+
+# Testcase 12k, Directory rename with sibling causes rename-to-self
+#   Commit O: dirB/foo, dirA/{bar, baz_1}
+#   Commit A: dirA/{foo, bar, baz_1}
+#   Commit B: dirB/{foo, bar}, dirA/baz_2
+#   Expected: dirA/{foo, bar, baz_2}, with conflicts on dirA/bar vs. dirB/bar
+
+test_setup_12k () {
+	test_create_repo 12k &&
+	(
+		cd 12k &&
+
+		mkdir dirA dirB &&
+		echo foo >dirB/foo &&
+		echo bar >dirA/bar &&
+		echo baz >dirA/baz &&
+		git add . &&
+		git commit -m orig &&
+
+		git branch O &&
+		git branch A &&
+		git branch B &&
+
+		git switch A &&
+		git mv dirB/* dirA/ &&
+		git commit -m A &&
+
+		git switch B &&
+		git mv dirA/bar dirB/bar &&
+		echo more baz >>dirA/baz &&
+		git commit -m B
+	)
+}
+
+test_expect_merge_algorithm failure failure '12k: Directory rename with sibling causes rename-to-self' '
+	test_setup_12k &&
+	(
+		cd 12k &&
+
+		git checkout A^0 &&
+
+		test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&
+
+		test_path_is_missing dirB &&
+		test_path_is_file dirA/bar &&
+		test_path_is_file dirA/baz &&
+
+		git ls-files | uniq >tracked &&
+		test_line_count = 3 tracked &&
+
+		git status --porcelain -uno >actual &&
+		cat >expect <<-\EOF &&
+		UU dirA/bar
+		 M dirA/baz
+		EOF
+		test_cmp expect actual
+	)
+'
+
 ###########################################################################
 # SECTION 13: Checking informational and conflict messages
 #

-- 
gitgitgadget

Elijah Newren via GitGitGadget· Jun 30, 2021, 17:29 UTC · re: Elijah Newren via GitGitGadget · lore

[PATCH v2 2/3] merge-ort: ensure we consult df_conflict and path_conflicts

From: Elijah Newren <newren@gmail.com>

Reported-by: Anders Kaseorg <andersk@mit.edu>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c                         | 6 +++++-
 t/t6423-merge-rename-directories.sh | 6 +++---
 2 files changed, 8 insertions(+), 4 deletions(-)

Show changes to 2 files +8 −4

merge-ort.c, t/t6423-merge-rename-directories.sh

diff --git a/merge-ort.c b/merge-ort.c
index b954f7184a5..373dbac5079 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -3237,7 +3237,7 @@ static void process_entry(struct merge_options *opt,
 	 *       above.
 	 */
 	if (ci->match_mask) {
-		ci->merged.clean = 1;
+		ci->merged.clean = !ci->df_conflict && !ci->path_conflict;
 		if (ci->match_mask == 6) {
 			/* stages[1] == stages[2] */
 			ci->merged.result.mode = ci->stages[1].mode;
@@ -3249,6 +3249,8 @@ static void process_entry(struct merge_options *opt,
 
 			ci->merged.result.mode = ci->stages[side].mode;
 			ci->merged.is_null = !ci->merged.result.mode;
+			if (ci->merged.is_null)
+				ci->merged.clean = 1;
 			oidcpy(&ci->merged.result.oid, &ci->stages[side].oid);
 
 			assert(othermask == 2 || othermask == 4);
@@ -3421,6 +3423,7 @@ static void process_entry(struct merge_options *opt,
 				   path)) {
 			ci->merged.is_null = 1;
 			ci->merged.clean = 1;
+			assert(!ci->df_conflict && !ci->path_conflict);
 		} else if (ci->path_conflict &&
 			   oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {
 			/*
@@ -3447,6 +3450,7 @@ static void process_entry(struct merge_options *opt,
 		ci->merged.is_null = 1;
 		ci->merged.result.mode = 0;
 		oidcpy(&ci->merged.result.oid, null_oid());
+		assert(!ci->df_conflict);
 		ci->merged.clean = !ci->path_conflict;
 	}
 
diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index 89fe6778b94..c3968dc3642 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -5058,7 +5058,7 @@ test_setup_12i () {
 	)
 }
 
-test_expect_merge_algorithm failure failure '12i: Directory rename causes rename-to-self' '
+test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' '
 	test_setup_12i &&
 	(
 		cd 12i &&
@@ -5116,7 +5116,7 @@ test_setup_12j () {
 	)
 }
 
-test_expect_merge_algorithm failure failure '12j: Directory rename to root causes rename-to-self' '
+test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' '
 	test_setup_12j &&
 	(
 		cd 12j &&
@@ -5174,7 +5174,7 @@ test_setup_12k () {
 	)
 }
 
-test_expect_merge_algorithm failure failure '12k: Directory rename with sibling causes rename-to-self' '
+test_expect_merge_algorithm failure success '12k: Directory rename with sibling causes rename-to-self' '
 	test_setup_12k &&
 	(
 		cd 12k &&

-- 
gitgitgadget

Elijah Newren via GitGitGadget· Jun 30, 2021, 17:30 UTC · re: Elijah Newren via GitGitGadget · lore

[PATCH v2 3/3] merge-recursive: handle rename-to-self case

From: Elijah Newren <newren@gmail.com>

Directory rename detection can cause transitive renames, e.g. if the two
different sides of history each do one half of:
    A/file -> B/file
    B/     -> C/
then directory rename detection transitively renames to give us
    A/file -> C/file

However, when C/ == A/, note that this gives us
    A/file -> A/file.

Reported-by: Anders Kaseorg <andersk@mit.edu>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c                   | 19 +++++++++++++------
 t/t6423-merge-rename-directories.sh |  6 +++---
 2 files changed, 16 insertions(+), 9 deletions(-)

Show changes to 2 files +16 −9

merge-recursive.c, t/t6423-merge-rename-directories.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index d146bb116f7..c895145a8f5 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2804,12 +2804,19 @@ static int process_renames(struct merge_options *opt,
 			int renamed_stage = a_renames == renames1 ? 2 : 3;
 			int other_stage =   a_renames == renames1 ? 3 : 2;
 
+			/*
+			 * Directory renames have a funny corner case...
+			 */
+			int renamed_to_self = !strcmp(ren1_src, ren1_dst);
+
 			/* BUG: We should only remove ren1_src in the base
 			 * stage and in other_stage (think of rename +
 			 * add-source case).
 			 */
-			remove_file(opt, 1, ren1_src,
-				    renamed_stage == 2 || !was_tracked(opt, ren1_src));
+			if (!renamed_to_self)
+				remove_file(opt, 1, ren1_src,
+					    renamed_stage == 2 ||
+					    !was_tracked(opt, ren1_src));
 
 			oidcpy(&src_other.oid,
 			       &ren1->src_entry->stages[other_stage].oid);
@@ -2823,6 +2830,9 @@ static int process_renames(struct merge_options *opt,
 			    ren1->dir_rename_original_type == 'A') {
 				setup_rename_conflict_info(RENAME_VIA_DIR,
 							   opt, ren1, NULL);
+			} else if (renamed_to_self) {
+				setup_rename_conflict_info(RENAME_NORMAL,
+							   opt, ren1, NULL);
 			} else if (oideq(&src_other.oid, null_oid())) {
 				setup_rename_conflict_info(RENAME_DELETE,
 							   opt, ren1, NULL);
@@ -3180,7 +3190,6 @@ static int handle_rename_normal(struct merge_options *opt,
 	struct rename *ren = ci->ren1;
 	struct merge_file_info mfi;
 	int clean;
-	int side = (ren->branch == opt->branch1 ? 2 : 3);
 
 	/* Merge the content and write it out */
 	clean = handle_content_merge(&mfi, opt, path, was_dirty(opt, path),
@@ -3190,9 +3199,7 @@ static int handle_rename_normal(struct merge_options *opt,
 	    opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT &&
 	    ren->dir_rename_original_dest) {
 		if (update_stages(opt, path,
-				  NULL,
-				  side == 2 ? &mfi.blob : NULL,
-				  side == 2 ? NULL : &mfi.blob))
+				  &mfi.blob, &mfi.blob, &mfi.blob))
 			return -1;
 		clean = 0; /* not clean, but conflicted */
 	}
diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index c3968dc3642..01d27b66399 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -5058,7 +5058,7 @@ test_setup_12i () {
 	)
 }
 
-test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' '
+test_expect_success '12i: Directory rename causes rename-to-self' '
 	test_setup_12i &&
 	(
 		cd 12i &&
@@ -5116,7 +5116,7 @@ test_setup_12j () {
 	)
 }
 
-test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' '
+test_expect_success '12j: Directory rename to root causes rename-to-self' '
 	test_setup_12j &&
 	(
 		cd 12j &&
@@ -5174,7 +5174,7 @@ test_setup_12k () {
 	)
 }
 
-test_expect_merge_algorithm failure success '12k: Directory rename with sibling causes rename-to-self' '
+test_expect_success '12k: Directory rename with sibling causes rename-to-self' '
 	test_setup_12k &&
 	(
 		cd 12k &&

-- 
gitgitgadget