git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/3] Fix dual rename into each other plus conflicting adds
@ 2022-06-22  4:20 Elijah Newren via GitGitGadget
  2022-06-22  4:20 ` [PATCH 1/3] t6423: add tests of dual directory rename plus add/add conflict Elijah Newren via GitGitGadget
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-06-22  4:20 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren

This series adds some testcases based on the tensorflow repository issue
reported by Glen Choo at [1], demonstrating bugs in both the ort and
recursive strategies. It also provides a fix for the ort strategy.

[1]
https://lore.kernel.org/git/kl6lee006mle.fsf@chooglen-macbookpro.roam.corp.google.com/

Elijah Newren (3):
  t6423: add tests of dual directory rename plus add/add conflict
  merge-ort: shuffle the computation and cleanup of potential collisions
  merge-ort: fix issue with dual rename and add/add conflict

 merge-ort.c                         |  60 ++++++++++------
 t/t6423-merge-rename-directories.sh | 102 ++++++++++++++++++++++++++++
 2 files changed, 142 insertions(+), 20 deletions(-)


base-commit: e54793a95afeea1e10de1e5ad7eab914e7416250
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1268%2Fnewren%2Ffix-dual-rename-into-each-other-plus-conflicting-adds-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1268/newren/fix-dual-rename-into-each-other-plus-conflicting-adds-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1268
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/3] t6423: add tests of dual directory rename plus add/add conflict
  2022-06-22  4:20 [PATCH 0/3] Fix dual rename into each other plus conflicting adds Elijah Newren via GitGitGadget
@ 2022-06-22  4:20 ` Elijah Newren via GitGitGadget
  2022-06-27 18:20   ` Jonathan Tan
  2022-06-27 22:30   ` Calvin Wan
  2022-06-22  4:20 ` [PATCH 2/3] merge-ort: shuffle the computation and cleanup of potential collisions Elijah Newren via GitGitGadget
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 10+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-06-22  4:20 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

This is an attempt at minimalizing a testcase reported by Glen Choo
with tensorflow where merge-ort would report an assertion failure:

    Assertion failed: (ci->filemask == 2 || ci->filemask == 4), function apply_directory_rename_modifications, file merge-ort.c, line 2410

reversing the direction of the merge provides a different error:

    error: cache entry has null sha1: ...
    fatal: unable to write .git/index

so we add testcases for both.  With these new testcases, the
recursive strategy differs in that it returns the latter error for
both merge directions.

These testcases are somehow a little different than Glen's original
tensorflow testcase in that these ones trigger a bug with the recursive
algorithm whereas his testcase didn't.  I figure that means these
testcases somehow manage to be more comprehensive.

Reported-by: Glen Choo <chooglen@google.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6423-merge-rename-directories.sh | 102 ++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index 479db32cd62..296c04f8046 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -5199,6 +5199,108 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '
 	)
 '
 
+# Testcase 12l, Both sides rename a directory into the other side, both add
+#   a file with after directory renames are the same filename
+#   Commit O: sub1/file,                 sub2/other
+#   Commit A: sub3/file,                 sub2/{other, new_add_add_file_1}
+#   Commit B: sub1/{file, newfile}, sub1/sub2/{other, new_add_add_file_2}
+#
+#   In words:
+#     A: sub1/ -> sub3/, add sub2/new_add_add_file_1
+#     B: sub2/ -> sub1/sub2, add sub1/newfile, add sub1/sub2/new_add_add_file_2
+#
+#   Expected: sub3/{file, newfile, sub2/other}
+#             CONFLICT (add/add): sub3/sub2/new_add_add_file
+
+test_setup_12l () {
+	test_create_repo 12l_$1 &&
+	(
+		cd 12l_$1 &&
+
+		mkdir -p sub1 sub2
+		echo file >sub1/file &&
+		echo other >sub2/other &&
+		git add sub1 sub2 &&
+		git commit -m "O" &&
+
+		git branch O &&
+		git branch A &&
+		git branch B &&
+
+		git checkout A &&
+		git mv sub1 sub3 &&
+		echo conflicting >sub2/new_add_add_file &&
+		git add sub2 &&
+		test_tick &&
+		git add -u &&
+		git commit -m "A" &&
+
+		git checkout B &&
+		echo dissimilar >sub2/new_add_add_file &&
+		echo brand >sub1/newfile &&
+		git add sub1 sub2 &&
+		git mv sub2 sub1 &&
+		test_tick &&
+		git commit -m "B"
+	)
+}
+
+test_expect_merge_algorithm failure failure '12l (B into A): Rename into each other + add/add conflict' '
+	test_setup_12l BintoA &&
+	(
+		cd 12l_BintoA &&
+
+		git checkout -q A^0 &&
+
+		test_must_fail git -c merge.directoryRenames=true merge -s recursive B^0 &&
+
+		git ls-files -s >out &&
+		test_line_count = 5 out &&
+
+		git rev-parse >actual \
+			:0:sub3/file :0:sub3/newfile :0:sub3/sub2/other \
+			:2:sub1/sub2/new_add_add_file \
+			:3:sub1/sub2/new_add_add_file &&
+		git rev-parse >expect \
+			O:sub1/file  B:sub1/newfile O:sub2/other \
+			A:sub2/new_add_add_file \
+			B:sub1/sub2/new_add_add_file &&
+		test_cmp expect actual &&
+
+		git ls-files -o >actual &&
+		test_write_lines actual expect out >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_merge_algorithm failure failure '12l (A into B): Rename into each other + add/add conflict' '
+	test_setup_12l AintoB &&
+	(
+		cd 12l_AintoB &&
+
+		git checkout -q B^0 &&
+
+		test_must_fail git -c merge.directoryRenames=true merge -s recursive A^0 &&
+
+		git ls-files -s >out &&
+		test_line_count = 5 out &&
+
+		git rev-parse >actual \
+			:0:sub3/file :0:sub3/newfile :0:sub3/sub2/other \
+			:2:sub1/sub2/new_add_add_file \
+			:3:sub1/sub2/new_add_add_file &&
+		git rev-parse >expect \
+			O:sub1/file  B:sub1/newfile O:sub2/other \
+			B:sub1/sub2/new_add_add_file \
+			A:sub2/new_add_add_file &&
+		test_cmp expect actual &&
+
+		git ls-files -o >actual &&
+		test_write_lines actual expect out >expect &&
+		test_cmp expect actual
+	)
+'
+
 ###########################################################################
 # SECTION 13: Checking informational and conflict messages
 #
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/3] merge-ort: shuffle the computation and cleanup of potential collisions
  2022-06-22  4:20 [PATCH 0/3] Fix dual rename into each other plus conflicting adds Elijah Newren via GitGitGadget
  2022-06-22  4:20 ` [PATCH 1/3] t6423: add tests of dual directory rename plus add/add conflict Elijah Newren via GitGitGadget
@ 2022-06-22  4:20 ` Elijah Newren via GitGitGadget
  2022-06-27 18:48   ` Jonathan Tan
  2022-06-22  4:20 ` [PATCH 3/3] merge-ort: fix issue with dual rename and add/add conflict Elijah Newren via GitGitGadget
  2022-06-22  4:36 ` [PATCH 0/3] Fix dual rename into each other plus conflicting adds Elijah Newren
  3 siblings, 1 reply; 10+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-06-22  4:20 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

Run compute_collisions() for renames on both sides of history before
any calls to collect_renames(), and do not free the computed collisions
until after both calls to collect_renames().  This is just a code
reorganization at this point that doesn't make sense on its own, but
will permit us to use the computed collision info from both sides
within each call to collect_renames() in a subsequent commit.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 56 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 8545354dafd..fa6667de18c 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2259,6 +2259,27 @@ static void compute_collisions(struct strmap *collisions,
 	}
 }
 
+static void free_collisions(struct strmap *collisions)
+{
+	struct hashmap_iter iter;
+	struct strmap_entry *entry;
+
+	/* Free each value in the collisions map */
+	strmap_for_each_entry(collisions, &iter, entry) {
+		struct collision_info *info = entry->value;
+		string_list_clear(&info->source_files, 0);
+	}
+	/*
+	 * In compute_collisions(), we set collisions.strdup_strings to 0
+	 * so that we wouldn't have to make another copy of the new_path
+	 * allocated by apply_dir_rename().  But now that we've used them
+	 * and have no other references to these strings, it is time to
+	 * deallocate them.
+	 */
+	free_strmap_strings(collisions);
+	strmap_clear(collisions, 1);
+}
+
 static char *check_for_directory_rename(struct merge_options *opt,
 					const char *path,
 					unsigned side_index,
@@ -2314,7 +2335,8 @@ static char *check_for_directory_rename(struct merge_options *opt,
 	}
 
 	new_path = handle_path_level_conflicts(opt, path, side_index,
-					       rename_info, collisions);
+					       rename_info,
+					       &collisions[side_index]);
 	*clean_merge &= (new_path != NULL);
 
 	return new_path;
@@ -3023,18 +3045,15 @@ static int detect_regular_renames(struct merge_options *opt,
 static int collect_renames(struct merge_options *opt,
 			   struct diff_queue_struct *result,
 			   unsigned side_index,
+			   struct strmap *collisions,
 			   struct strmap *dir_renames_for_side,
 			   struct strmap *rename_exclusions)
 {
 	int i, clean = 1;
-	struct strmap collisions;
 	struct diff_queue_struct *side_pairs;
-	struct hashmap_iter iter;
-	struct strmap_entry *entry;
 	struct rename_info *renames = &opt->priv->renames;
 
 	side_pairs = &renames->pairs[side_index];
-	compute_collisions(&collisions, dir_renames_for_side, side_pairs);
 
 	for (i = 0; i < side_pairs->nr; ++i) {
 		struct diff_filepair *p = side_pairs->queue[i];
@@ -3050,7 +3069,7 @@ static int collect_renames(struct merge_options *opt,
 						      side_index,
 						      dir_renames_for_side,
 						      rename_exclusions,
-						      &collisions,
+						      collisions,
 						      &clean);
 
 		possibly_cache_new_pair(renames, p, side_index, new_path);
@@ -3076,20 +3095,6 @@ static int collect_renames(struct merge_options *opt,
 		result->queue[result->nr++] = p;
 	}
 
-	/* Free each value in the collisions map */
-	strmap_for_each_entry(&collisions, &iter, entry) {
-		struct collision_info *info = entry->value;
-		string_list_clear(&info->source_files, 0);
-	}
-	/*
-	 * In compute_collisions(), we set collisions.strdup_strings to 0
-	 * so that we wouldn't have to make another copy of the new_path
-	 * allocated by apply_dir_rename().  But now that we've used them
-	 * and have no other references to these strings, it is time to
-	 * deallocate them.
-	 */
-	free_strmap_strings(&collisions);
-	strmap_clear(&collisions, 1);
 	return clean;
 }
 
@@ -3100,6 +3105,7 @@ static int detect_and_process_renames(struct merge_options *opt,
 {
 	struct diff_queue_struct combined = { 0 };
 	struct rename_info *renames = &opt->priv->renames;
+	struct strmap collisions[3];
 	int need_dir_renames, s, i, clean = 1;
 	unsigned detection_run = 0;
 
@@ -3149,12 +3155,22 @@ static int detect_and_process_renames(struct merge_options *opt,
 	ALLOC_GROW(combined.queue,
 		   renames->pairs[1].nr + renames->pairs[2].nr,
 		   combined.alloc);
+	for (int i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {
+		int other_side = 3 - i;
+		compute_collisions(&collisions[i],
+				   &renames->dir_renames[other_side],
+				   &renames->pairs[i]);
+	}
 	clean &= collect_renames(opt, &combined, MERGE_SIDE1,
+				 collisions,
 				 &renames->dir_renames[2],
 				 &renames->dir_renames[1]);
 	clean &= collect_renames(opt, &combined, MERGE_SIDE2,
+				 collisions,
 				 &renames->dir_renames[1],
 				 &renames->dir_renames[2]);
+	for (int i = MERGE_SIDE1; i <= MERGE_SIDE2; i++)
+		free_collisions(&collisions[i]);
 	STABLE_QSORT(combined.queue, combined.nr, compare_pairs);
 	trace2_region_leave("merge", "directory renames", opt->repo);
 
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 3/3] merge-ort: fix issue with dual rename and add/add conflict
  2022-06-22  4:20 [PATCH 0/3] Fix dual rename into each other plus conflicting adds Elijah Newren via GitGitGadget
  2022-06-22  4:20 ` [PATCH 1/3] t6423: add tests of dual directory rename plus add/add conflict Elijah Newren via GitGitGadget
  2022-06-22  4:20 ` [PATCH 2/3] merge-ort: shuffle the computation and cleanup of potential collisions Elijah Newren via GitGitGadget
@ 2022-06-22  4:20 ` Elijah Newren via GitGitGadget
  2022-06-27 18:47   ` Jonathan Tan
  2022-06-22  4:36 ` [PATCH 0/3] Fix dual rename into each other plus conflicting adds Elijah Newren
  3 siblings, 1 reply; 10+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-06-22  4:20 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

There is code in both merge-recursive and merge-ort for avoiding doubly
transitive renames (i.e. one side renames directory A/ -> B/, and the
other side renames directory B/ -> C/), because this combination would
otherwise make a mess for new files added to A/ on the first side and
wondering which directory they end up in -- especially if there were
even more renames such as the first side renaming C/ -> D/.  In such
cases, it just turns "off" directory rename detection for the higher
order transitive cases.

The testcases added in t6423 a couple commits ago are slightly different
but similar in principle.  They involve a similar case of paired
renaming but instead of A/ -> B/ and B/ -> C/, the second side renames
a leading directory of B/ to C/.  And both sides add a new file
somewhere under the directory that the other side will rename.  While
the new files added start within different directories and thus could
logically end up within different directories, it is weird for a file
on one side to end up where the other one started and not move along
with it.  So, let's just turn off directory rename detection in this
case as well.

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

diff --git a/merge-ort.c b/merge-ort.c
index fa6667de18c..5bcb9a4980b 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2292,9 +2292,13 @@ static char *check_for_directory_rename(struct merge_options *opt,
 	struct strmap_entry *rename_info;
 	struct strmap_entry *otherinfo = NULL;
 	const char *new_dir;
+	int other_side = 3 - side_index;
 
+	/* Cases where there is no new path, so we return NULL */
 	if (strmap_empty(dir_renames))
 		return new_path;
+	if (strmap_get(&collisions[other_side], path))
+		return new_path;
 	rename_info = check_dir_renamed(path, dir_renames);
 	if (!rename_info)
 		return new_path;
diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index 296c04f8046..4286ae987c4 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -5245,7 +5245,7 @@ test_setup_12l () {
 	)
 }
 
-test_expect_merge_algorithm failure failure '12l (B into A): Rename into each other + add/add conflict' '
+test_expect_merge_algorithm failure success '12l (B into A): Rename into each other + add/add conflict' '
 	test_setup_12l BintoA &&
 	(
 		cd 12l_BintoA &&
@@ -5273,7 +5273,7 @@ test_expect_merge_algorithm failure failure '12l (B into A): Rename into each ot
 	)
 '
 
-test_expect_merge_algorithm failure failure '12l (A into B): Rename into each other + add/add conflict' '
+test_expect_merge_algorithm failure success '12l (A into B): Rename into each other + add/add conflict' '
 	test_setup_12l AintoB &&
 	(
 		cd 12l_AintoB &&
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/3] Fix dual rename into each other plus conflicting adds
  2022-06-22  4:20 [PATCH 0/3] Fix dual rename into each other plus conflicting adds Elijah Newren via GitGitGadget
                   ` (2 preceding siblings ...)
  2022-06-22  4:20 ` [PATCH 3/3] merge-ort: fix issue with dual rename and add/add conflict Elijah Newren via GitGitGadget
@ 2022-06-22  4:36 ` Elijah Newren
  3 siblings, 0 replies; 10+ messages in thread
From: Elijah Newren @ 2022-06-22  4:36 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget; +Cc: Git Mailing List

On Tue, Jun 21, 2022 at 9:21 PM Elijah Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> This series adds some testcases based on the tensorflow repository issue
> reported by Glen Choo at [1], demonstrating bugs in both the ort and
> recursive strategies. It also provides a fix for the ort strategy.

Also, just to be clear, this is not a regression since 2.36.  It's
been with the ort strategy since it was introduced, and with the
recursive strategy for a number of years.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] t6423: add tests of dual directory rename plus add/add conflict
  2022-06-22  4:20 ` [PATCH 1/3] t6423: add tests of dual directory rename plus add/add conflict Elijah Newren via GitGitGadget
@ 2022-06-27 18:20   ` Jonathan Tan
  2022-06-27 22:30   ` Calvin Wan
  1 sibling, 0 replies; 10+ messages in thread
From: Jonathan Tan @ 2022-06-27 18:20 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget; +Cc: Jonathan Tan, git, Elijah Newren

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

> From: Elijah Newren <newren@gmail.com>
> 
> This is an attempt at minimalizing a testcase reported by Glen Choo
> with tensorflow where merge-ort would report an assertion failure:
> 
>     Assertion failed: (ci->filemask == 2 || ci->filemask == 4), function apply_directory_rename_modifications, file merge-ort.c, line 2410

First of all, thanks for this fix - I've verified with Glen Choo's test
cases and it does fix it.

> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
> index 479db32cd62..296c04f8046 100755
> --- a/t/t6423-merge-rename-directories.sh
> +++ b/t/t6423-merge-rename-directories.sh
> @@ -5199,6 +5199,108 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '
>  	)
>  '
>  
> +# Testcase 12l, Both sides rename a directory into the other side, both add
> +#   a file with after directory renames are the same filename
> +#   Commit O: sub1/file,                 sub2/other
> +#   Commit A: sub3/file,                 sub2/{other, new_add_add_file_1}
> +#   Commit B: sub1/{file, newfile}, sub1/sub2/{other, new_add_add_file_2}
> +#
> +#   In words:
> +#     A: sub1/ -> sub3/, add sub2/new_add_add_file_1
> +#     B: sub2/ -> sub1/sub2, add sub1/newfile, add sub1/sub2/new_add_add_file_2
> +#
> +#   Expected: sub3/{file, newfile, sub2/other}
> +#             CONFLICT (add/add): sub3/sub2/new_add_add_file

The "expected" might need to be updated. After all patches are applied,
this is the tree I get (note that it's "sub1/sub2/new_add_add_file, not
"sub3/sub2/new_add_add_file"):

  .
  |-- sub1
  |   `-- sub2
  |       `-- new_add_add_file
  `-- sub3
      |-- file
      |-- newfile
      `-- sub2
          `-- other

Also, at first glance, "newfile" shouldn't belong in a minimal
reproduction, but it somehow changes the output. When I apply this diff
(to the state after all patches are applied):

  diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
  index 4286ae987c..9472fb7233 100755
  --- a/t/t6423-merge-rename-directories.sh
  +++ b/t/t6423-merge-rename-directories.sh
  @@ -5237,7 +5237,6 @@ test_setup_12l () {
   
                  git checkout B &&
                  echo dissimilar >sub2/new_add_add_file &&
  -               echo brand >sub1/newfile &&
                  git add sub1 sub2 &&
                  git mv sub2 sub1 &&
                  test_tick &&
  @@ -5255,14 +5254,14 @@ test_expect_merge_algorithm failure success '12l (B into A): Rename into each ot
                  test_must_fail git -c merge.directoryRenames=true merge -s recursive B^0 &&
   
                  git ls-files -s >out &&
  -               test_line_count = 5 out &&
  +               test_line_count = 4 out &&
   
                  git rev-parse >actual \
  -                       :0:sub3/file :0:sub3/newfile :0:sub3/sub2/other \
  +                       :0:sub3/file :0:sub3/sub2/other \
                          :2:sub1/sub2/new_add_add_file \
                          :3:sub1/sub2/new_add_add_file &&
                  git rev-parse >expect \
  -                       O:sub1/file  B:sub1/newfile O:sub2/other \
  +                       O:sub1/file  O:sub2/other \
                          A:sub2/new_add_add_file \
                          B:sub1/sub2/new_add_add_file &&
                  test_cmp expect actual &&

I get:

  .
  |-- sub1
  |   `-- sub2
  |       |-- new_add_add_file
  |       `-- other
  `-- sub3
      `-- file

(Note the path to "other".) I haven't figured out what's going on,
though.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] merge-ort: fix issue with dual rename and add/add conflict
  2022-06-22  4:20 ` [PATCH 3/3] merge-ort: fix issue with dual rename and add/add conflict Elijah Newren via GitGitGadget
@ 2022-06-27 18:47   ` Jonathan Tan
  0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Tan @ 2022-06-27 18:47 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget; +Cc: Jonathan Tan, git, Elijah Newren

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> The testcases added in t6423 a couple commits ago are slightly different
> but similar in principle.  They involve a similar case of paired
> renaming but instead of A/ -> B/ and B/ -> C/, the second side renames
> a leading directory of B/ to C/.  

Hmm...one side moved sub1 -> sub3 and the other moved sub2 from the root
to under sub1, right? So it's more like A/ -> B/ and C/ -> A/C/.

> And both sides add a new file
> somewhere under the directory that the other side will rename.  

Rename or move, I think.

> While
> the new files added start within different directories and thus could
> logically end up within different directories, it is weird for a file
> on one side to end up where the other one started and not move along
> with it.  So, let's just turn off directory rename detection in this
> case as well.

Makes sense.

> diff --git a/merge-ort.c b/merge-ort.c
> index fa6667de18c..5bcb9a4980b 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -2292,9 +2292,13 @@ static char *check_for_directory_rename(struct merge_options *opt,
>  	struct strmap_entry *rename_info;
>  	struct strmap_entry *otherinfo = NULL;
>  	const char *new_dir;
> +	int other_side = 3 - side_index;
>  
> +	/* Cases where there is no new path, so we return NULL */

What do you mean by "no new path"?

>  	if (strmap_empty(dir_renames))
>  		return new_path;
> +	if (strmap_get(&collisions[other_side], path))
> +		return new_path;

So as far as I understand, collisions here, for a given side, is a map.
The map's keys are all the filenames of added and renamed files (I'm
assuming that's what 'A' and 'R' are) from that side after any directory
moves on the other side are applied. So, for example, if we add "foo/a"
on side A and rename "foo/" to "bar/" on side B, then side A's collision
map would have a key "bar/a". So I'm not sure if "collision" is the
right name here, but the existing code already uses it so I'll leave it
be.

It makes sense that this situation (of side A having "bar/a" because
side B renamed "foo/" to "bar/", and at the same time, side B adds its
own "bar/a") is weird, as stated in the commit message, so I don't mind
disabling checking for directory rename in this case. However, in
theory, I don't see how disabling this check would fix anything, since
the bug seems to be about two different files on different sides being
renamed to the same filename through some convoluted means. (Unless this
is the only convoluted means to do it, and disabling it means that the
bug wouldn't occur.)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] merge-ort: shuffle the computation and cleanup of potential collisions
  2022-06-22  4:20 ` [PATCH 2/3] merge-ort: shuffle the computation and cleanup of potential collisions Elijah Newren via GitGitGadget
@ 2022-06-27 18:48   ` Jonathan Tan
  2022-06-27 21:04     ` Calvin Wan
  0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Tan @ 2022-06-27 18:48 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget; +Cc: Jonathan Tan, git, Elijah Newren

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -2314,7 +2335,8 @@ static char *check_for_directory_rename(struct merge_options *opt,
>  	}
>  
>  	new_path = handle_path_level_conflicts(opt, path, side_index,
> -					       rename_info, collisions);
> +					       rename_info,
> +					       &collisions[side_index]);

Is this a fix of a latent bug? handle_path_level_conflicts() is not
changed in this patch.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] merge-ort: shuffle the computation and cleanup of potential collisions
  2022-06-27 18:48   ` Jonathan Tan
@ 2022-06-27 21:04     ` Calvin Wan
  0 siblings, 0 replies; 10+ messages in thread
From: Calvin Wan @ 2022-06-27 21:04 UTC (permalink / raw)
  To: Jonathan Tan
  Cc: Calvin Wan, Elijah Newren via GitGitGadget, git, Elijah Newren

Jonathan Tan <jonathantanmy@google.com> writes:
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > @@ -2314,7 +2335,8 @@ static char *check_for_directory_rename(struct merge_options *opt,
> >  	}
> >  
> >  	new_path = handle_path_level_conflicts(opt, path, side_index,
> > -					       rename_info, collisions);
> > +					       rename_info,
> > +					       &collisions[side_index]);
> 
> Is this a fix of a latent bug? handle_path_level_conflicts() is not
> changed in this patch.
> 

I don't think so. IIUC this is what's happening given the callstack:

detect_and_process_renames()
  - Now defines `struct strmap collisions[3];` and computes all
    three collisions here
  - Passes collisions into collect_renames()
collect_renames()
  - Originally defined as `struct strmap collisions;` and computed
    collisions in here
  - Now takes collisions as an argument
  - Passes collisions into check_for_directory_rename()
check_for_directory_rename()
  - Collisions isn't used in this function at all except to pass into
    handle_path_level_conflicts
handle_path_level_conflicts()
  - Expecting pointer to singular collisions, not an array so side_index
    is now required

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] t6423: add tests of dual directory rename plus add/add conflict
  2022-06-22  4:20 ` [PATCH 1/3] t6423: add tests of dual directory rename plus add/add conflict Elijah Newren via GitGitGadget
  2022-06-27 18:20   ` Jonathan Tan
@ 2022-06-27 22:30   ` Calvin Wan
  1 sibling, 0 replies; 10+ messages in thread
From: Calvin Wan @ 2022-06-27 22:30 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget; +Cc: Calvin Wan, git, Elijah Newren

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +# Testcase 12l, Both sides rename a directory into the other side, both add
> +#   a file with after directory renames are the same filename
> +#   Commit O: sub1/file,                 sub2/other
> +#   Commit A: sub3/file,                 sub2/{other, new_add_add_file_1}
> +#   Commit B: sub1/{file, newfile}, sub1/sub2/{other, new_add_add_file_2}
> +#
> +#   In words:
> +#     A: sub1/ -> sub3/, add sub2/new_add_add_file_1
> +#     B: sub2/ -> sub1/sub2, add sub1/newfile, add sub1/sub2/new_add_add_file_2
> +#
> +#   Expected: sub3/{file, newfile, sub2/other}
> +#             CONFLICT (add/add): sub3/sub2/new_add_add_file

Grammatically, I could not understand

"both add a file with after directory renames are the same filename"

I also found the same issue with `Expected` that Jonathan mentions. I ran the
command separately and got

CONFLICT (add/add): Merge conflict in sub1/sub2/new_add_add_file

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-06-27 22:32 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-22  4:20 [PATCH 0/3] Fix dual rename into each other plus conflicting adds Elijah Newren via GitGitGadget
2022-06-22  4:20 ` [PATCH 1/3] t6423: add tests of dual directory rename plus add/add conflict Elijah Newren via GitGitGadget
2022-06-27 18:20   ` Jonathan Tan
2022-06-27 22:30   ` Calvin Wan
2022-06-22  4:20 ` [PATCH 2/3] merge-ort: shuffle the computation and cleanup of potential collisions Elijah Newren via GitGitGadget
2022-06-27 18:48   ` Jonathan Tan
2022-06-27 21:04     ` Calvin Wan
2022-06-22  4:20 ` [PATCH 3/3] merge-ort: fix issue with dual rename and add/add conflict Elijah Newren via GitGitGadget
2022-06-27 18:47   ` Jonathan Tan
2022-06-22  4:36 ` [PATCH 0/3] Fix dual rename into each other plus conflicting adds Elijah Newren

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).