merge-recursive: Fix modify/delete resolution in the recursive case

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>

diff --git a/merge-recursive.c b/merge-recursive.c
index 8b88d6266c329e1a7af76a66f3e2d922d5dc5faf..897cada6cf3053149b8fda5dd2fc0f3a9a13b3c6 100644 (file)
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1322,27 +1322,42 @@ static int blob_unchanged(const unsigned char *o_sha,
 
 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,
@@ -1512,14 +1527,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 b046e1be7197b810925af5fbb43b2f999d10cec1..314fdaeb187a4db9cfa18e6019494d315794405a 100755 (executable)
--- 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 &&