merge-recursive: apply collision handling unification to recursive case

In the en/merge-path-collision topic (see commit ac193e0e0aa5, "Merge
branch 'en/merge-path-collision'", 2019-01-04), all the "file collision"
conflict types were modified for consistency.  In particular,
rename/add, rename/rename(2to1) and each rename/add piece of a
rename/rename(1to2)/add[/add] conflict were made to behave like add/add
conflicts have always been handled.

However, this consistency was not enforced when opt->priv->call_depth >
0 for rename/rename conflicts.  Update rename/rename(1to2) and
rename/rename(2to1) conflicts in the recursive case to also be
consistent.  As an added bonus, this simplifies the code considerably.

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 aee1769a7ac344c0d0fed681588efce4c09df564..3728b3f6598deb61e52b840845eca1872e697d12 100644 (file)
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1557,35 +1557,6 @@ static int handle_file_collision(struct merge_options *opt,
                                             b, a);
        }
 
-       /*
-        * In the recursive case, we just opt to undo renames
-        */
-       if (opt->priv->call_depth && (prev_path1 || prev_path2)) {
-               /* Put first file (a->oid, a->mode) in its original spot */
-               if (prev_path1) {
-                       if (update_file(opt, 1, a, prev_path1))
-                               return -1;
-               } else {
-                       if (update_file(opt, 1, a, collide_path))
-                               return -1;
-               }
-
-               /* Put second file (b->oid, b->mode) in its original spot */
-               if (prev_path2) {
-                       if (update_file(opt, 1, b, prev_path2))
-                               return -1;
-               } else {
-                       if (update_file(opt, 1, b, collide_path))
-                               return -1;
-               }
-
-               /* Don't leave something at collision path if unrenaming both */
-               if (prev_path1 && prev_path2)
-                       remove_file(opt, 1, collide_path, 0);
-
-               return 0;
-       }
-
        /* Remove rename sources if rename/add or rename/rename(2to1) */
        if (prev_path1)
                remove_file(opt, 1, prev_path1,
@@ -1746,85 +1717,56 @@ static int handle_rename_rename_1to2(struct merge_options *opt,
                return -1;
        free(path_desc);
 
-       if (opt->priv->call_depth) {
-               /*
-                * FIXME: For rename/add-source conflicts (if we could detect
-                * such), this is wrong.  We should instead find a unique
-                * pathname and then either rename the add-source file to that
-                * unique path, or use that unique path instead of src here.
-                */
-               if (update_file(opt, 0, &mfi.blob, o->path))
-                       return -1;
+       if (opt->priv->call_depth)
+               remove_file_from_index(opt->repo->index, o->path);
 
-               /*
-                * Above, we put the merged content at the merge-base's
-                * path.  Now we usually need to delete both a->path and
-                * b->path.  However, the rename on each side of the merge
-                * could also be involved in a rename/add conflict.  In
-                * such cases, we should keep the added file around,
-                * resolving the conflict at that path in its favor.
-                */
-               add = &ci->ren1->dst_entry->stages[flip_stage(2)];
-               if (is_valid(add)) {
-                       if (update_file(opt, 0, add, a->path))
-                               return -1;
-               }
-               else
-                       remove_file_from_index(opt->repo->index, a->path);
-               add = &ci->ren2->dst_entry->stages[flip_stage(3)];
-               if (is_valid(add)) {
-                       if (update_file(opt, 0, add, b->path))
-                               return -1;
-               }
-               else
-                       remove_file_from_index(opt->repo->index, b->path);
+       /*
+        * For each destination path, we need to see if there is a
+        * rename/add collision.  If not, we can write the file out
+        * to the specified location.
+        */
+       add = &ci->ren1->dst_entry->stages[flip_stage(2)];
+       if (is_valid(add)) {
+               add->path = mfi.blob.path = a->path;
+               if (handle_file_collision(opt, a->path,
+                                         NULL, NULL,
+                                         ci->ren1->branch,
+                                         ci->ren2->branch,
+                                         &mfi.blob, add) < 0)
+                       return -1;
        } else {
-               /*
-                * For each destination path, we need to see if there is a
-                * rename/add collision.  If not, we can write the file out
-                * to the specified location.
-                */
-               add = &ci->ren1->dst_entry->stages[flip_stage(2)];
-               if (is_valid(add)) {
-                       add->path = mfi.blob.path = a->path;
-                       if (handle_file_collision(opt, a->path,
-                                                 NULL, NULL,
-                                                 ci->ren1->branch,
-                                                 ci->ren2->branch,
-                                                 &mfi.blob, add) < 0)
-                               return -1;
-               } else {
-                       char *new_path = find_path_for_conflict(opt, a->path,
-                                                               ci->ren1->branch,
-                                                               ci->ren2->branch);
-                       if (update_file(opt, 0, &mfi.blob,
-                                       new_path ? new_path : a->path))
-                               return -1;
-                       free(new_path);
-                       if (update_stages(opt, a->path, NULL, a, NULL))
-                               return -1;
-               }
+               char *new_path = find_path_for_conflict(opt, a->path,
+                                                       ci->ren1->branch,
+                                                       ci->ren2->branch);
+               if (update_file(opt, 0, &mfi.blob,
+                               new_path ? new_path : a->path))
+                       return -1;
+               free(new_path);
+               if (!opt->priv->call_depth &&
+                   update_stages(opt, a->path, NULL, a, NULL))
+                       return -1;
+       }
 
-               add = &ci->ren2->dst_entry->stages[flip_stage(3)];
-               if (is_valid(add)) {
-                       add->path = mfi.blob.path = b->path;
-                       if (handle_file_collision(opt, b->path,
-                                                 NULL, NULL,
-                                                 ci->ren1->branch,
-                                                 ci->ren2->branch,
-                                                 add, &mfi.blob) < 0)
-                               return -1;
-               } else {
-                       char *new_path = find_path_for_conflict(opt, b->path,
-                                                               ci->ren2->branch,
-                                                               ci->ren1->branch);
-                       if (update_file(opt, 0, &mfi.blob,
-                                       new_path ? new_path : b->path))
-                               return -1;
-                       free(new_path);
-                       if (update_stages(opt, b->path, NULL, NULL, b))
-                               return -1;
-               }
+       add = &ci->ren2->dst_entry->stages[flip_stage(3)];
+       if (is_valid(add)) {
+               add->path = mfi.blob.path = b->path;
+               if (handle_file_collision(opt, b->path,
+                                         NULL, NULL,
+                                         ci->ren1->branch,
+                                         ci->ren2->branch,
+                                         add, &mfi.blob) < 0)
+                       return -1;
+       } else {
+               char *new_path = find_path_for_conflict(opt, b->path,
+                                                       ci->ren2->branch,
+                                                       ci->ren1->branch);
+               if (update_file(opt, 0, &mfi.blob,
+                               new_path ? new_path : b->path))
+                       return -1;
+               free(new_path);
+               if (!opt->priv->call_depth &&
+                   update_stages(opt, b->path, NULL, NULL, b))
+                       return -1;
        }
 
        return 0;

diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh
index 7d73afdcdaafea81ed09b163ea425a9c881a8a9f..b3bf4626178681cd1acb8dc0e7a70d20dcd95cfb 100755 (executable)
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -60,9 +60,9 @@ test_expect_success 'merge simple rename+criss-cross with no modifications' '
                test_must_fail git merge -s recursive R2^0 &&
 
                git ls-files -s >out &&
-               test_line_count = 2 out &&
+               test_line_count = 5 out &&
                git ls-files -u >out &&
-               test_line_count = 2 out &&
+               test_line_count = 3 out &&
                git ls-files -o >out &&
                test_line_count = 1 out &&
 
@@ -133,9 +133,9 @@ test_expect_success 'merge criss-cross + rename merges with basic modification'
                test_must_fail git merge -s recursive R2^0 &&
 
                git ls-files -s >out &&
-               test_line_count = 2 out &&
+               test_line_count = 5 out &&
                git ls-files -u >out &&
-               test_line_count = 2 out &&
+               test_line_count = 3 out &&
                git ls-files -o >out &&
                test_line_count = 1 out &&
 
@@ -218,8 +218,18 @@ test_expect_success 'git detects differently handled merges conflict' '
                git ls-files -o >out &&
                test_line_count = 1 out &&
 
-               git rev-parse >expect       \
-                       C:new_a  D:new_a  E:new_a &&
+               git cat-file -p C:new_a >ours &&
+               git cat-file -p C:a >theirs &&
+               >empty &&
+               test_must_fail git merge-file \
+                       -L "Temporary merge branch 1" \
+                       -L "" \
+                       -L "Temporary merge branch 2" \
+                       ours empty theirs &&
+               sed -e "s/^\([<=>]\)/\1\1\1/" ours >ours-tweaked &&
+               git hash-object ours-tweaked >expect &&
+               git rev-parse >>expect      \
+                                 D:new_a  E:new_a &&
                git rev-parse   >actual     \
                        :1:new_a :2:new_a :3:new_a &&
                test_cmp expect actual &&
@@ -257,7 +267,8 @@ test_expect_success 'git detects differently handled merges conflict, swapped' '
                ctime=$(git log --no-walk --date=raw --format=%cd C | awk "{print \$1}") &&
                newctime=$(($btime+1)) &&
                git fast-export --no-data --all | sed -e s/$ctime/$newctime/ | git fast-import --force --quiet &&
-               # End of differences; rest is copy-paste of last test
+               # End of most differences; rest is copy-paste of last test,
+               # other than swapping C:a and C:new_a due to order switch
 
                git checkout D^0 &&
                test_must_fail git merge -s recursive E^0 &&
@@ -269,8 +280,18 @@ test_expect_success 'git detects differently handled merges conflict, swapped' '
                git ls-files -o >out &&
                test_line_count = 1 out &&
 
-               git rev-parse >expect       \
-                       C:new_a  D:new_a  E:new_a &&
+               git cat-file -p C:a >ours &&
+               git cat-file -p C:new_a >theirs &&
+               >empty &&
+               test_must_fail git merge-file \
+                       -L "Temporary merge branch 1" \
+                       -L "" \
+                       -L "Temporary merge branch 2" \
+                       ours empty theirs &&
+               sed -e "s/^\([<=>]\)/\1\1\1/" ours >ours-tweaked &&
+               git hash-object ours-tweaked >expect &&
+               git rev-parse >>expect      \
+                                 D:new_a  E:new_a &&
                git rev-parse   >actual     \
                        :1:new_a :2:new_a :3:new_a &&
                test_cmp expect actual &&