Fix bugs from interesting renaming pairs: one side renames A/file -> B/file, the other B/ -> A/

From: Elijah Newren <[email protected]>

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.

---
 t/t6423-merge-rename-directories.sh | 117 ++++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)

index be84d22419d..2a2ab907338 100755
@@ -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

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

From: Elijah Newren <[email protected]>

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.

---
 merge-ort.c                         | 6 +++++-
 t/t6423-merge-rename-directories.sh | 4 ++--
 2 files changed, 7 insertions(+), 3 deletions(-)

index b954f7184a5..373dbac5079 100644
@@ -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;
 	}
 

index 2a2ab907338..7480daab46a 100755
@@ -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

From: Elijah Newren <[email protected]>

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.

---
 merge-recursive.c                   | 19 +++++++++++++------
 t/t6423-merge-rename-directories.sh |  4 ++--
 2 files changed, 15 insertions(+), 8 deletions(-)

index d146bb116f7..c895145a8f5 100644
@@ -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 */
 	}

index 7480daab46a..d67b5c28944 100755
@@ -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

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.

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.

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

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.

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.

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

From: Elijah Newren <[email protected]>

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.

---
 t/t6423-merge-rename-directories.sh | 175 ++++++++++++++++++++++++++++
 1 file changed, 175 insertions(+)

index be84d22419d..89fe6778b94 100755
@@ -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

From: Elijah Newren <[email protected]>

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.

---
 merge-ort.c                         | 6 +++++-
 t/t6423-merge-rename-directories.sh | 6 +++---
 2 files changed, 8 insertions(+), 4 deletions(-)

index b954f7184a5..373dbac5079 100644
@@ -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;
 	}
 

index 89fe6778b94..c3968dc3642 100755
@@ -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

From: Elijah Newren <[email protected]>

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.

---
 merge-recursive.c                   | 19 +++++++++++++------
 t/t6423-merge-rename-directories.sh |  6 +++---
 2 files changed, 16 insertions(+), 9 deletions(-)

index d146bb116f7..c895145a8f5 100644
@@ -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 */
 	}

index c3968dc3642..01d27b66399 100755
@@ -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