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

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

[Elijah Newren via 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

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

[Elijah Newren via GitGitGadget]

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(+)

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

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

[Elijah Newren via GitGitGadget]

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(-)

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

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

[Elijah Newren via GitGitGadget]

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(-)

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

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

[Junio C Hamano]

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


>
> 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 &&

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

[Derrick Stolee]

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?

Thanks,
-Stolee

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

[Derrick Stolee]

On 6/29/2021 12:02 AM, Junio C Hamano wrote:
> "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

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

[Elijah Newren]

On Tue, Jun 29, 2021 at 5:50 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> 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.

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

[Elijah Newren via 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.

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

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

[Elijah Newren via GitGitGadget]

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 | 175 ++++++++++++++++++++++++++++
 1 file changed, 175 insertions(+)

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

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

[Elijah Newren via GitGitGadget]

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 | 6 +++---
 2 files changed, 8 insertions(+), 4 deletions(-)

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

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

[Elijah Newren via GitGitGadget]

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 |  6 +++---
 2 files changed, 16 insertions(+), 9 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 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