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

[PATCHv2 31/56] merge-recursive: Make dead code for rename/rename(2to1) conflicts undead

From
Elijah Newren <newren@gmail.com>
Date
Aug 12, 2011, 05:20 UTC
Message-ID
<1313126429-17368-32-git-send-email-newren@gmail.com>
In-Reply-To
<1313126429-17368-1-git-send-email-newren@gmail.com>

The code for rename_rename_2to1 conflicts (two files both being renamed to the same filename) was dead since the rename/add path was always being independently triggered for each of the renames instead. Further, reviving the dead code showed that it was inherently buggy and would always segfault -- among a few other bugs.

Move the else-if branch for the rename/rename block before the rename/add block to make sure it is checked first, and fix up the rename/rename(2to1) code segments to make it handle most cases. Work is still needed to handle higher dimensional corner cases such as rename/rename/modify/modify issues.

Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
No changes since v1.
 merge-recursive.c                 |   70 +++++++++++++++++++++++++-----------
 t/t6036-recursive-corner-cases.sh |   17 +++++----
 2 files changed, 57 insertions(+), 30 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index f395d20..f74feb5 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -998,17 +998,36 @@ static void conflict_rename_rename_2to1(struct merge_options *o,
 					struct rename *ren2,
 					const char *branch2)
 {
+	char *path = ren1->pair->two->path; /* same as ren2->pair->two->path */
 	/* Two files were renamed to the same thing. */
-	char *new_path1 = unique_path(o, ren1->pair->two->path, branch1);
-	char *new_path2 = unique_path(o, ren2->pair->two->path, branch2);
-	output(o, 1, "Renaming %s to %s and %s to %s instead",
-	       ren1->pair->one->path, new_path1,
-	       ren2->pair->one->path, new_path2);
-	remove_file(o, 0, ren1->pair->two->path, 0);
-	update_file(o, 0, ren1->pair->two->sha1, ren1->pair->two->mode, new_path1);
-	update_file(o, 0, ren2->pair->two->sha1, ren2->pair->two->mode, new_path2);
-	free(new_path2);
-	free(new_path1);
+	if (o->call_depth) {
+		struct merge_file_info mfi;
+		struct diff_filespec one, a, b;
+
+		one.path = a.path = b.path = path;
+		hashcpy(one.sha1, null_sha1);
+		one.mode = 0;
+		hashcpy(a.sha1, ren1->pair->two->sha1);
+		a.mode = ren1->pair->two->mode;
+		hashcpy(b.sha1, ren2->pair->two->sha1);
+		b.mode = ren2->pair->two->mode;
+		mfi = merge_file(o, &one, &a, &b, branch1, branch2);
+		output(o, 1, "Adding merged %s", path);
+		update_file(o, 0, mfi.sha, mfi.mode, path);
+	} else {
+		char *new_path1 = unique_path(o, path, branch1);
+		char *new_path2 = unique_path(o, path, branch2);
+		output(o, 1, "Renaming %s to %s and %s to %s instead",
+		       ren1->pair->one->path, new_path1,
+		       ren2->pair->one->path, new_path2);
+		remove_file(o, 0, path, 0);
+		update_file(o, 0, ren1->pair->two->sha1, ren1->pair->two->mode,
+			    new_path1);
+		update_file(o, 0, ren2->pair->two->sha1, ren2->pair->two->mode,
+			    new_path2);
+		free(new_path2);
+		free(new_path1);
+	}
 }
 
 static int process_renames(struct merge_options *o,
@@ -1023,12 +1042,12 @@ static int process_renames(struct merge_options *o,
 	for (i = 0; i < a_renames->nr; i++) {
 		sre = a_renames->items[i].util;
 		string_list_insert(&a_by_dst, sre->pair->two->path)->util
-			= sre->dst_entry;
+			= (void *)sre;
 	}
 	for (i = 0; i < b_renames->nr; i++) {
 		sre = b_renames->items[i].util;
 		string_list_insert(&b_by_dst, sre->pair->two->path)->util
-			= sre->dst_entry;
+			= (void *)sre;
 	}
 
 	for (i = 0, j = 0; i < a_renames->nr || j < b_renames->nr;) {
@@ -1140,6 +1159,23 @@ static int process_renames(struct merge_options *o,
 					clean_merge = 0;
 					conflict_rename_delete(o, ren1->pair, branch1, branch2);
 				}
+			} else if ((item = string_list_lookup(renames2Dst, ren1_dst))) {
+				char *ren2_src, *ren2_dst;
+				ren2 = item->util;
+				ren2_src = ren2->pair->one->path;
+				ren2_dst = ren2->pair->two->path;
+
+				clean_merge = 0;
+				ren2->processed = 1;
+				remove_file(o, 1, ren2_src,
+					    renamed_stage == 3 || would_lose_untracked(ren1_src));
+
+				output(o, 1, "CONFLICT (rename/rename): "
+				       "Rename %s->%s in %s. "
+				       "Rename %s->%s in %s",
+				       ren1_src, ren1_dst, branch1,
+				       ren2_src, ren2_dst, branch2);
+				conflict_rename_rename_2to1(o, ren1, branch1, ren2, branch2);
 			} else if ((dst_other.mode == ren1->pair->two->mode) &&
 				   sha_eq(dst_other.sha1, ren1->pair->two->sha1)) {
 				/* Added file on the other side
@@ -1180,16 +1216,6 @@ static int process_renames(struct merge_options *o,
 					update_file(o, 0, dst_other.sha1, dst_other.mode, new_path);
 					free(new_path);
 				}
-			} else if ((item = string_list_lookup(renames2Dst, ren1_dst))) {
-				ren2 = item->util;
-				clean_merge = 0;
-				ren2->processed = 1;
-				output(o, 1, "CONFLICT (rename/rename): "
-				       "Rename %s->%s in %s. "
-				       "Rename %s->%s in %s",
-				       ren1_src, ren1_dst, branch1,
-				       ren2->pair->one->path, ren2->pair->two->path, branch2);
-				conflict_rename_rename_2to1(o, ren1, branch1, ren2, branch2);
 			} else
 				try_merge = 1;
 
diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh
index 279f33c..b046e1b 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -66,13 +66,13 @@ test_expect_success 'merge simple rename+criss-cross with no modifications' '
 	test $(git rev-parse :2:three) = $(git rev-parse L2:three) &&
 	test $(git rev-parse :3:three) = $(git rev-parse R2:three) &&
 
-	cp two merged &&
+	cp one merged &&
 	>empty &&
 	test_must_fail git merge-file \
-		-L "Temporary merge branch 2" \
-		-L "" \
 		-L "Temporary merge branch 1" \
-		merged empty one &&
+		-L "" \
+		-L "Temporary merge branch 2" \
+		merged empty two &&
 	test $(git rev-parse :1:three) = $(git hash-object merged)
 '
 
@@ -145,11 +145,12 @@ test_expect_success 'merge criss-cross + rename merges with basic modification'
 	cp one merge-me &&
 	>empty &&
 	test_must_fail git merge-file \
-		-L "Temporary merge branch 2" \
-		-L "" \
 		-L "Temporary merge branch 1" \
-		merged empty merge-me &&
-	test $(git rev-parse :1:three) = $(git hash-object merged)
+		-L "" \
+		-L "Temporary merge branch 2" \
+		merge-me empty merged &&
+
+	test $(git rev-parse :1:three) = $(git hash-object merge-me)
 '
 
 #

-- 
1.7.6.100.gac5c1
Previous: Elijah NewrenNext: Elijah Newren
Message 36 of 65 in “[PATCHv2 00/57] Re-roll of en/merge-recursive from pu”
  1. Elijah NewrenAug 12, 2011
  2. 01/56 t6042: Add a testcase where git deletes an untracked fileElijah Newren, Aug 12, 2011
  3. 02/56 t6042: Add failing testcase for rename/modify/add-source conflictElijah Newren, Aug 12, 2011
  4. 03/56 t6042: Add a pair of cases where undetected renames cause issuesElijah Newren, Aug 12, 2011
  5. 04/56 t6042: Add a testcase where undetected rename causes silent file deletionElijah Newren, Aug 12, 2011
  6. 05/56 t6042: Add tests for content issues with modify/rename/directory conflictsElijah Newren, Aug 12, 2011
  7. 06/56 t6042: Ensure rename/rename conflicts leave index and workdir in sane stateElijah Newren, Aug 12, 2011
  8. 07/56 t6042: Add failing testcases for rename/rename/add-{source,dest} conflictsElijah Newren, Aug 12, 2011
  9. 08/56 t6036: Add differently resolved modify/delete conflict in criss-cross testElijah Newren, Aug 12, 2011
  10. 09/56 t6036: criss-cross with weird content can fool git into clean mergeElijah Newren, Aug 12, 2011
  11. 10/56 t6036: tests for criss-cross merges with various directory/file conflictsElijah Newren, Aug 12, 2011
  12. 11/56 t6036: criss-cross w/ rename/rename(1to2)/modify+rename/rename(2to1)/modifyElijah Newren, Aug 12, 2011
  13. 12/56 t6036: criss-cross + rename/rename(1to2)/add-source + modify/modifyElijah Newren, Aug 12, 2011
  14. 13/56 t6022: Remove unnecessary untracked files to make test cleanerElijah Newren, Aug 12, 2011
  15. 14/56 t6022: New tests checking for unnecessary updates of filesElijah Newren, Aug 12, 2011
  16. 15/56 t6022: Add testcase for merging a renamed file with a simple changeElijah Newren, Aug 12, 2011
  17. 16/56 merge-recursive: Make BUG message more legible by adding a newlineElijah Newren, Aug 12, 2011
  18. 17/56 merge-recursive: Correct a commentElijah Newren, Aug 12, 2011
  19. 18/56 merge-recursive: Mark some diff_filespec struct arguments constElijah Newren, Aug 12, 2011
  20. 19/56 merge-recursive: Consolidate different update_stages functionsElijah Newren, Aug 12, 2011
  21. 20/56 merge-recursive: Remember to free generated unique path namesElijah Newren, Aug 12, 2011
  22. 21/56 merge-recursive: Avoid working directory changes during recursive caseElijah Newren, Aug 12, 2011
  23. 22/56 merge-recursive: Fix recursive case with D/F conflict via add/add conflictElijah Newren, Aug 12, 2011
  24. 23/56 merge-recursive: Fix sorting order and directory change assumptionsElijah Newren, Aug 12, 2011
  25. 24/56 merge-recursive: Fix code checking for D/F conflicts still being presentElijah Newren, Aug 12, 2011
  26. 25/56 merge-recursive: Save D/F conflict filenames instead of unlinking themElijah Newren, Aug 12, 2011
  27. 26/56 merge-recursive: Split was_tracked() out of would_lose_untracked()Elijah Newren, Aug 12, 2011
  28. 27/56 string-list: Add API to remove an item from an unsorted listElijah Newren, Aug 12, 2011
  29. Johannes SixtAug 12, 2011
  30. Alex RiesenAug 12, 2011
  31. Elijah NewrenAug 12, 2011
  32. Johannes SixtAug 13, 2011
  33. 28/56 merge-recursive: Allow make_room_for_path() to remove D/F entriesElijah Newren, Aug 12, 2011
  34. 29/56 merge-recursive: Split update_stages_and_entry; only update stages at endElijah Newren, Aug 12, 2011
  35. 30/56 merge-recursive: Fix deletion of untracked file in rename/delete conflictsElijah Newren, Aug 12, 2011
  36. 31/56 merge-recursive: Make dead code for rename/rename(2to1) conflicts undeadElijah Newren, Aug 12, 2011
  37. 32/56 merge-recursive: Add comments about handling rename/add-source casesElijah Newren, Aug 12, 2011
  38. 33/56 merge-recursive: Improve handling of rename target vs. directory additionElijah Newren, Aug 12, 2011
  39. 34/56 merge-recursive: Consolidate process_entry() and process_df_entry()Elijah Newren, Aug 12, 2011
  40. 35/56 merge-recursive: Cleanup and consolidation of rename_conflict_infoElijah Newren, Aug 12, 2011
  41. 36/56 merge-recursive: Provide more info in conflict markers with file renamesElijah Newren, Aug 12, 2011
  42. 37/56 merge-recursive: When we detect we can skip an update, actually skip itElijah Newren, Aug 12, 2011
  43. 38/56 merge-recursive: Fix modify/delete resolution in the recursive caseElijah Newren, Aug 12, 2011
  44. 39/56 merge-recursive: Introduce a merge_file convenience functionElijah Newren, Aug 12, 2011
  45. 40/56 merge-recursive: Fix rename/rename(1to2) resolution for virtual merge baseElijah Newren, Aug 12, 2011
  46. 41/56 merge-recursive: Small cleanups for conflict_rename_rename_1to2Elijah Newren, Aug 12, 2011
  47. 42/56 merge-recursive: Defer rename/rename(2to1) handling until process_entryElijah Newren, Aug 12, 2011
  48. 43/56 merge-recursive: Record more data needed for merging with dual renamesElijah Newren, Aug 12, 2011
  49. 44/56 merge-recursive: Create function for merging with branchname:file markersElijah Newren, Aug 12, 2011
  50. 45/56 merge-recursive: Consider modifications in rename/rename(2to1) conflictsElijah Newren, Aug 12, 2011
  51. 46/56 merge-recursive: Make modify/delete handling code reusableElijah Newren, Aug 12, 2011
  52. 47/56 merge-recursive: Have conflict_rename_delete reuse modify/delete codeElijah Newren, Aug 12, 2011
  53. 48/56 merge-recursive: add handling for rename/rename/add-dest/add-destElijah Newren, Aug 12, 2011
  54. 49/56 merge-recursive: Fix working copy handling for rename/rename/add/addElijah Newren, Aug 12, 2011
  55. 50/56 t3030: fix accidental success in symlink renameElijah Newren, Aug 12, 2011
  56. 51/56 t6022: Add testcase for spurious "refusing to lose untracked" messagesElijah Newren, Aug 12, 2011
  57. 52/56 merge-recursive: Fix spurious 'refusing to lose untracked file...' messagesElijah Newren, Aug 12, 2011
  58. 53/56 t6022: Additional tests checking for unnecessary updates of filesElijah Newren, Aug 12, 2011
  59. 54/56 merge-recursive: Avoid unnecessary file rewritesElijah Newren, Aug 12, 2011
  60. 55/56 t6036: criss-cross + rename/rename(1to2)/add-dest + simple modifyElijah Newren, Aug 12, 2011
  61. 56/56 merge-recursive: Fix virtual merge base for rename/rename(1to2)/add-destElijah Newren, Aug 12, 2011
  62. Junio C HamanoAug 12, 2011
  63. Elijah NewrenAug 12, 2011
  64. Elijah NewrenAug 13, 2011
  65. 57/57 merge-recursive: Don't re-sort a list whose order we depend uponElijah Newren, Aug 13, 2011

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.