merge-recursive: Consider modifications in rename/rename(2to1) conflicts

Our previous conflict resolution for renaming two different files to the
same name ignored the fact that each of those files may have modifications
from both sides of history to consider.  We need to do a three-way merge
for each of those files, and then handle the conflict of both sets of
merged contents trying to be recorded with the same name.

It is important to note that this changes our strategy in the recursive
case.  After doing a three-way content merge of each of the files
involved, we still are faced with the fact that we are trying to put both
of the results (including conflict markers) into the same path.  We could
do another two-way merge, but I think that becomes confusing.  Also,
taking a hint from the modify/delete and rename/delete cases we handled
earlier, a more useful "common ground" would be to keep the three-way
content merge but record it with the original filename.  The renames can
still be detected, we just allow it to be done in the o->call_depth=0
case.  This seems to result in simpler & easier to understand merge
conflicts as well, as evidenced by some of the changes needed in our
testsuite in t6036.  (However, it should be noted that this change will
cause problems those renames also occur along with a file being added
whose name matches the source of the rename.  Since git currently cannot
detect rename/add-source situations, though, this codepath is not
currently used for those cases anyway.

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 4ceb6aac8868aae5b2f432dae99307a784686717..3124144011f2c3b716e06d052a98d7f1c2eb8ca2 100644 (file)
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1081,6 +1081,8 @@ static void conflict_rename_rename_2to1(struct merge_options *o,
        struct diff_filespec *c1 = ci->pair1->two;
        struct diff_filespec *c2 = ci->pair2->two;
        char *path = c1->path; /* == c2->path */
+       struct merge_file_info mfi_c1;
+       struct merge_file_info mfi_c2;
 
        output(o, 1, "CONFLICT (rename/rename): "
               "Rename %s->%s in %s. "
@@ -1091,22 +1093,32 @@ static void conflict_rename_rename_2to1(struct merge_options *o,
        remove_file(o, 1, a->path, would_lose_untracked(a->path));
        remove_file(o, 1, b->path, would_lose_untracked(b->path));
 
+       mfi_c1 = merge_file_special_markers(o, a, c1, &ci->ren1_other,
+                                           o->branch1, c1->path,
+                                           o->branch2, ci->ren1_other.path);
+       mfi_c2 = merge_file_special_markers(o, b, &ci->ren2_other, c2,
+                                           o->branch1, ci->ren2_other.path,
+                                           o->branch2, c2->path);
+
        if (o->call_depth) {
-               struct merge_file_info mfi;
-               mfi = merge_file(o, path, null_sha1, 0,
-                                c1->sha1, c1->mode,
-                                c2->sha1, c2->mode,
-                                ci->branch1, ci->branch2);
-               output(o, 1, "Adding merged %s", path);
-               update_file(o, 0, mfi.sha, mfi.mode, path);
+               /*
+                * If mfi_c1.clean && mfi_c2.clean, then it might make
+                * sense to do a two-way merge of those results.  But, I
+                * think in all cases, it makes sense to have the virtual
+                * merge base just undo the renames; they can be detected
+                * again later for the non-recursive merge.
+                */
+               remove_file(o, 0, path, 0);
+               update_file(o, 0, mfi_c1.sha, mfi_c1.mode, a->path);
+               update_file(o, 0, mfi_c2.sha, mfi_c2.mode, b->path);
        } else {
                char *new_path1 = unique_path(o, path, ci->branch1);
                char *new_path2 = unique_path(o, path, ci->branch2);
                output(o, 1, "Renaming %s to %s and %s to %s instead",
                       a->path, new_path1, b->path, new_path2);
                remove_file(o, 0, path, 0);
-               update_file(o, 0, c1->sha1, c1->mode, new_path1);
-               update_file(o, 0, c2->sha1, c2->mode, new_path2);
+               update_file(o, 0, mfi_c1.sha, mfi_c1.mode, new_path1);
+               update_file(o, 0, mfi_c2.sha, mfi_c2.mode, new_path2);
                free(new_path2);
                free(new_path1);
        }

diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh
index 5a7af0ce919d39d550c0973fec376e6cd5cf2466..d8c6bdacc378b0647d23dc4e8dc5b7db137864e6 100755 (executable)
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -57,23 +57,15 @@ test_expect_success 'merge simple rename+criss-cross with no modifications' '
 
        test_must_fail git merge -s recursive R2^0 &&
 
-       test 5 = $(git ls-files -s | wc -l) &&
-       test 3 = $(git ls-files -u | wc -l) &&
-       test 0 = $(git ls-files -o | wc -l) &&
+       test 2 = $(git ls-files -s | wc -l) &&
+       test 2 = $(git ls-files -u | wc -l) &&
+       test 2 = $(git ls-files -o | wc -l) &&
 
-       test $(git rev-parse :0:one) = $(git rev-parse L2:one) &&
-       test $(git rev-parse :0:two) = $(git rev-parse R2:two) &&
        test $(git rev-parse :2:three) = $(git rev-parse L2:three) &&
        test $(git rev-parse :3:three) = $(git rev-parse R2:three) &&
 
-       cp one merged &&
-       >empty &&
-       test_must_fail git merge-file \
-               -L "Temporary merge branch 1" \
-               -L "" \
-               -L "Temporary merge branch 2" \
-               merged empty two &&
-       test $(git rev-parse :1:three) = $(git hash-object merged)
+       test $(git rev-parse L2:three) = $(git hash-object three~HEAD) &&
+       test $(git rev-parse R2:three) = $(git hash-object three~R2^0)
 '
 
 #
@@ -132,25 +124,15 @@ test_expect_success 'merge criss-cross + rename merges with basic modification'
 
        test_must_fail git merge -s recursive R2^0 &&
 
-       test 5 = $(git ls-files -s | wc -l) &&
-       test 3 = $(git ls-files -u | wc -l) &&
-       test 0 = $(git ls-files -o | wc -l) &&
+       test 2 = $(git ls-files -s | wc -l) &&
+       test 2 = $(git ls-files -u | wc -l) &&
+       test 2 = $(git ls-files -o | wc -l) &&
 
-       test $(git rev-parse :0:one) = $(git rev-parse L2:one) &&
-       test $(git rev-parse :0:two) = $(git rev-parse R2:two) &&
        test $(git rev-parse :2:three) = $(git rev-parse L2:three) &&
        test $(git rev-parse :3:three) = $(git rev-parse R2:three) &&
 
-       head -n 10 two >merged &&
-       cp one merge-me &&
-       >empty &&
-       test_must_fail git merge-file \
-               -L "Temporary merge branch 1" \
-               -L "" \
-               -L "Temporary merge branch 2" \
-               merge-me empty merged &&
-
-       test $(git rev-parse :1:three) = $(git hash-object merge-me)
+       test $(git rev-parse L2:three) = $(git hash-object three~HEAD) &&
+       test $(git rev-parse R2:three) = $(git hash-object three~R2^0)
 '
 
 #

diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
index bfc3179332f566e443a0acebb044cd61dd3d52d2..3be5059318cb96a3ef0fad7de03e273da79ba3e4 100755 (executable)
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -376,7 +376,7 @@ test_expect_success 'setup rename/rename (2to1) + modify/modify' '
        git commit -m C
 '
 
-test_expect_failure 'handle rename/rename (2to1) conflict correctly' '
+test_expect_success 'handle rename/rename (2to1) conflict correctly' '
        git checkout B^0 &&
 
        test_must_fail git merge -s recursive C^0 >out &&