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

[PATCHv6 30/31] merge-recursive: avoid spurious rename/rename conflict from dir renames

From
Elijah Newren <newren@gmail.com>
Date
Jan 5, 2018, 20:27 UTC
Message-ID
<20180105202711.24311-31-newren@gmail.com>
In-Reply-To
<20180105202711.24311-1-newren@gmail.com>

If a file on one side of history was renamed, and merely modified on the other side, then applying a directory rename to the modified side gives us a rename/rename(1to2) conflict. We should only apply directory renames to pairs representing either adds or renames.

Making this change means that a directory rename testcase that was previously reported as a rename/delete conflict will now be reported as a modify/delete conflict.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c                   |  4 +--
 t/t6043-merge-rename-directories.sh | 55 +++++++++++++++++--------------------
 2 files changed, 27 insertions(+), 32 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index fe42cabad..d00786f71 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1951,7 +1951,7 @@ static void compute_collisions(struct hashmap *collisions,
 		char *new_path;
 		struct diff_filepair *pair = pairs->queue[i];
 
-		if (pair->status == 'D')
+		if (pair->status != 'A' && pair->status != 'R')
 			continue;
 		dir_rename_ent = check_dir_renamed(pair->two->path,
 						   dir_renames);
@@ -2178,7 +2178,7 @@ static struct string_list *get_renames(struct merge_options *o,
 		struct diff_filepair *pair = pairs->queue[i];
 		char *new_path; /* non-NULL only with directory renames */
 
-		if (pair->status == 'D') {
+		if (pair->status != 'A' && pair->status != 'R') {
 			diff_free_filepair(pair);
 			continue;
 		}
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
index c684f34b6..99dca4ff0 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -2070,18 +2070,23 @@ test_expect_success '8b-check: Dual-directory rename, one into the others way, w
 	)
 '
 
-# Testcase 8c, rename+modify/delete
-#   (Related to testcases 5b and 8d)
+# Testcase 8c, modify/delete or rename+modify/delete?
+#   (Related to testcases 5b, 8d, and 9h)
 #   Commit O: z/{b,c,d}
 #   Commit A: y/{b,c}
 #   Commit B: z/{b,c,d_modified,e}
-#   Expected: y/{b,c,e}, CONFLICT(rename+modify/delete: x/d -> y/d or deleted)
+#   Expected: y/{b,c,e}, CONFLICT(modify/delete: on z/d)
 #
-#   Note: This testcase doesn't present any concerns for me...until you
-#         compare it with testcases 5b and 8d.  See notes in 8d for more
-#         details.
-
-test_expect_success '8c-setup: rename+modify/delete' '
+#   Note: It could easily be argued that the correct resolution here is
+#         y/{b,c,e}, CONFLICT(rename/delete: z/d -> y/d vs deleted)
+#         and that the modifed version of d should be present in y/ after
+#         the merge, just marked as conflicted.  Indeed, I previously did
+#         argue that.  But applying directory renames to the side of
+#         history where a file is merely modified results in spurious
+#         rename/rename(1to2) conflicts -- see testcase 9h.  See also
+#         notes in 8d.
+
+test_expect_success '8c-setup: modify/delete or rename+modify/delete?' '
 	test_create_repo 8c &&
 	(
 		cd 8c &&
@@ -2114,32 +2119,32 @@ test_expect_success '8c-setup: rename+modify/delete' '
 	)
 '
 
-test_expect_success '8c-check: rename+modify/delete' '
+test_expect_success '8c-check: modify/delete or rename+modify/delete' '
 	(
 		cd 8c &&
 
 		git checkout A^0 &&
 
 		test_must_fail git merge -s recursive B^0 >out &&
-		test_i18ngrep "CONFLICT (rename/delete).* z/d.*y/d" out &&
+		test_i18ngrep "CONFLICT (modify/delete).* z/d" out &&
 
 		git ls-files -s >out &&
-		test_line_count = 4 out &&
+		test_line_count = 5 out &&
 		git ls-files -u >out &&
-		test_line_count = 1 out &&
+		test_line_count = 2 out &&
 		git ls-files -o >out &&
 		test_line_count = 1 out &&
 
 		git rev-parse >actual \
-			:0:y/b :0:y/c :0:y/e :3:y/d &&
+			:0:y/b :0:y/c :0:y/e :1:z/d :3:z/d &&
 		git rev-parse >expect \
-			O:z/b O:z/c B:z/e B:z/d &&
+			O:z/b O:z/c B:z/e O:z/d B:z/d &&
 		test_cmp expect actual &&
 
-		test_must_fail git rev-parse :1:y/d &&
-		test_must_fail git rev-parse :2:y/d &&
-		git ls-files -s y/d | grep ^100755 &&
-		test_path_is_file y/d
+		test_must_fail git rev-parse :2:z/d &&
+		git ls-files -s z/d | grep ^100755 &&
+		test_path_is_file z/d &&
+		test_path_is_missing y/d
 	)
 '
 
@@ -2153,16 +2158,6 @@ test_expect_success '8c-check: rename+modify/delete' '
 #
 #   Note: It would also be somewhat reasonable to resolve this as
 #             y/{b,c,e}, CONFLICT(rename/delete: x/d -> y/d or deleted)
-#   The logic being that the only difference between this testcase and 8c
-#   is that there is no modification to d.  That suggests that instead of a
-#   rename/modify vs. delete conflict, we should just have a rename/delete
-#   conflict, otherwise we are being inconsistent.
-#
-#   However...as far as consistency goes, we didn't report a conflict for
-#   path d_1 in testcase 5b due to a different file being in the way.  So,
-#   we seem to be forced to have cases where users can change things
-#   slightly and get what they may perceive as inconsistent results.  It
-#   would be nice to avoid that, but I'm not sure I see how.
 #
 #   In this case, I'm leaning towards: commit A was the one that deleted z/d
 #   and it did the rename of z to y, so the two "conflicts" (rename vs.
@@ -2891,7 +2886,7 @@ test_expect_success '9h-setup: Avoid dir rename on merely modified path' '
 	)
 '
 
-test_expect_failure '9h-check: Avoid dir rename on merely modified path' '
+test_expect_success '9h-check: Avoid dir rename on merely modified path' '
 	(
 		cd 9h &&
 
@@ -3927,7 +3922,7 @@ test_expect_success '12c-setup: Moving one directory hierarchy into another w/ c
 	)
 '
 
-test_expect_failure '12c-check: Moving one directory hierarchy into another w/ content merge' '
+test_expect_success '12c-check: Moving one directory hierarchy into another w/ content merge' '
 	(
 		cd 12c &&
 

-- 
2.14.2
Previous: Elijah NewrenNext: Elijah Newren
Message 13 of 34 in “[PATCHv6 00/31] Add directory rename detection to git”
  1. Elijah NewrenJan 5, 2018
  2. 01/31 directory rename detection: basic testcasesElijah Newren, Jan 5, 2018
  3. 07/31 directory rename detection: more involved edge/corner testcasesElijah Newren, Jan 5, 2018
  4. 08/31 directory rename detection: testcases exploring possibly suboptimal mergesElijah Newren, Jan 5, 2018
  5. 05/31 directory rename detection: files/directories in the way of some renamesElijah Newren, Jan 5, 2018
  6. 15/31 merge-recursive: make !o->detect_rename codepath more obviousElijah Newren, Jan 5, 2018
  7. 17/31 merge-recursive: add a new hashmap for storing directory renamesElijah Newren, Jan 5, 2018
  8. 19/31 merge-recursive: add get_directory_renames()Elijah Newren, Jan 5, 2018
  9. 18/31 merge-recursive: make a helper function for cleanup for handle_renamesElijah Newren, Jan 5, 2018
  10. 23/31 merge-recursive: check for file level conflicts then get new nameElijah Newren, Jan 5, 2018
  11. 25/31 merge-recursive: apply necessary modifications for directory renamesElijah Newren, Jan 5, 2018
  12. 28/31 merge-recursive: fix remaining directory rename + dirty overwrite casesElijah Newren, Jan 5, 2018
  13. 30/31 merge-recursive: avoid spurious rename/rename conflict from dir renamesElijah Newren, Jan 5, 2018
  14. 29/31 directory rename detection: new testcases showcasing a pair of bugsElijah Newren, Jan 5, 2018
  15. 31/31 merge-recursive: ensure we write updates for directory-renamed fileElijah Newren, Jan 5, 2018
  16. 26/31 merge-recursive: avoid clobbering untracked files with directory renamesElijah Newren, Jan 5, 2018
  17. 27/31 merge-recursive: fix overwriting dirty files involved in renamesElijah Newren, Jan 5, 2018
  18. 24/31 merge-recursive: when comparing files, don't include treesElijah Newren, Jan 5, 2018
  19. 22/31 merge-recursive: add computation of collisions due to dir rename & mergingElijah Newren, Jan 5, 2018
  20. 20/31 merge-recursive: check for directory level conflictsElijah Newren, Jan 5, 2018
  21. 21/31 merge-recursive: add a new hashmap for storing file collisionsElijah Newren, Jan 5, 2018
  22. 10/31 directory rename detection: tests for handling overwriting untracked filesElijah Newren, Jan 5, 2018
  23. 16/31 merge-recursive: split out code for determining diff_filepairsElijah Newren, Jan 5, 2018
  24. 06/31 directory rename detection: testcases checking which side did the renameElijah Newren, Jan 5, 2018
  25. 14/31 merge-recursive: fix leaks of allocated renames and diff_filepairsElijah Newren, Jan 5, 2018
  26. 09/31 directory rename detection: miscellaneous testcases to complete coverageElijah Newren, Jan 5, 2018
  27. 13/31 merge-recursive: introduce new functions to handle rename logicElijah Newren, Jan 5, 2018
  28. 02/31 directory rename detection: directory splitting testcasesElijah Newren, Jan 5, 2018
  29. 11/31 directory rename detection: tests for handling overwriting dirty filesElijah Newren, Jan 5, 2018
  30. 12/31 merge-recursive: move the get_renames() functionElijah Newren, Jan 5, 2018
  31. 04/31 directory rename detection: partially renamed directory testcase/discussionElijah Newren, Jan 5, 2018
  32. 03/31 directory rename detection: testcases to avoid taking detection too farElijah Newren, Jan 5, 2018
  33. SZEDER GáborJan 26, 2018
  34. Elijah NewrenJan 26, 2018

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

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