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

[PATCHv2 38/56] merge-recursive: Fix modify/delete resolution in the recursive case

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

When o->call_depth>0 and we have conflicts, we try to find "middle ground" when creating the virtual merge base. In the case of content conflicts, this can be done by doing a three-way content merge and using the result. In all parts where the three-way content merge is clean, it is the correct middle ground, and in parts where it conflicts there is no middle ground but the conflict markers provide a good compromise since they are unlikely to accidentally match any further changes.

In the case of a modify/delete conflict, we cannot do the same thing. Accepting either endpoint as the resolution for the virtual merge base runs the risk that when handling the non-recursive case we will silently accept one person's resolution over another without flagging a conflict. In this case, the closest "middle ground" we have is actually the merge base of the candidate merge bases. (We could alternatively attempt a three way content merge using an empty file in place of the deleted file, but that seems to be more work than necessary.)

Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
No changes since v1.
 merge-recursive.c                 |   38 +++++++++++++++++++++++-------------
 t/t6036-recursive-corner-cases.sh |    4 +-
 2 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index ccf71d3..a0dc0bf 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1324,27 +1324,42 @@ error_return:
 
 static void handle_delete_modify(struct merge_options *o,
 				 const char *path,
-				 const char *new_path,
+				 unsigned char *o_sha, int o_mode,
 				 unsigned char *a_sha, int a_mode,
 				 unsigned char *b_sha, int b_mode)
 {
-	if (!a_sha) {
+	char *renamed = NULL;
+	if (dir_in_way(path, !o->call_depth)) {
+		renamed = unique_path(o, path, a_sha ? o->branch1 : o->branch2);
+	}
+
+	if (o->call_depth) {
+		/*
+		 * We cannot arbitrarily accept either a_sha or b_sha as
+		 * correct; since there is no true "middle point" between
+		 * them, simply reuse the base version for virtual merge base.
+		 */
+		remove_file_from_cache(path);
+		update_file(o, 0, o_sha, o_mode, renamed ? renamed : path);
+	} else if (!a_sha) {
 		output(o, 1, "CONFLICT (delete/modify): %s deleted in %s "
 		       "and modified in %s. Version %s of %s left in tree%s%s.",
 		       path, o->branch1,
 		       o->branch2, o->branch2, path,
-		       NULL == new_path ? "" : " at ",
-		       NULL == new_path ? "" : new_path);
-		update_file(o, 0, b_sha, b_mode, new_path ? new_path : path);
+		       NULL == renamed ? "" : " at ",
+		       NULL == renamed ? "" : renamed);
+		update_file(o, 0, b_sha, b_mode, renamed ? renamed : path);
 	} else {
 		output(o, 1, "CONFLICT (delete/modify): %s deleted in %s "
 		       "and modified in %s. Version %s of %s left in tree%s%s.",
 		       path, o->branch2,
 		       o->branch1, o->branch1, path,
-		       NULL == new_path ? "" : " at ",
-		       NULL == new_path ? "" : new_path);
-		update_file(o, 0, a_sha, a_mode, new_path ? new_path : path);
+		       NULL == renamed ? "" : " at ",
+		       NULL == renamed ? "" : renamed);
+		update_file(o, 0, a_sha, a_mode, renamed ? renamed : path);
 	}
+	free(renamed);
+
 }
 
 static int merge_content(struct merge_options *o,
@@ -1514,14 +1529,9 @@ static int process_entry(struct merge_options *o,
 			remove_file(o, 1, path, !a_sha);
 		} else {
 			/* Modify/delete; deleted side may have put a directory in the way */
-			char *renamed = NULL;
 			clean_merge = 0;
-			if (dir_in_way(path, !o->call_depth)) {
-				renamed = unique_path(o, path, a_sha ? o->branch1 : o->branch2);
-			}
-			handle_delete_modify(o, path, renamed,
+			handle_delete_modify(o, path, o_sha, o_mode,
 					     a_sha, a_mode, b_sha, b_mode);
-			free(renamed);
 		}
 	} else if ((!o_sha && a_sha && !b_sha) ||
 		   (!o_sha && !a_sha && b_sha)) {
diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh
index b046e1b..314fdae 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -296,7 +296,7 @@ test_expect_success 'setup criss-cross + modify/delete resolved differently' '
 	git tag E
 '
 
-test_expect_failure 'git detects conflict merging criss-cross+modify/delete' '
+test_expect_success 'git detects conflict merging criss-cross+modify/delete' '
 	git checkout D^0 &&
 
 	test_must_fail git merge -s recursive E^0 &&
@@ -308,7 +308,7 @@ test_expect_failure 'git detects conflict merging criss-cross+modify/delete' '
 	test $(git rev-parse :2:file) = $(git rev-parse B:file)
 '
 
-test_expect_failure 'git detects conflict merging criss-cross+modify/delete, reverse direction' '
+test_expect_success 'git detects conflict merging criss-cross+modify/delete, reverse direction' '
 	git reset --hard &&
 	git checkout E^0 &&
 

-- 
1.7.6.100.gac5c1
Previous: Elijah NewrenNext: Elijah Newren
Message 43 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.