From: Elijah Newren <newren@xxxxxxxxx>
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@xxxxxxxxx>
---
merge-recursive: apply collision handling unification to recursive case
This commit ties up a loose end that I was unaware of with the
en/merge-path-collision topic from very early 2019.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-715%2Fnewren%2Frecursive-collision-handling-redux-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-715/newren/recursive-collision-handling-redux-v1
Pull-Request: https://github.com/git/git/pull/715
merge-recursive.c | 152 +++++++++---------------------
t/t6036-recursive-corner-cases.sh | 39 ++++++--
2 files changed, 77 insertions(+), 114 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index aee1769a7ac..3728b3f6598 100644
--- 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 7d73afdcdaa..b3bf4626178 100755
--- 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 &&
base-commit: 2d2118b814c11f509e1aa76cb07110f7231668dc
--
gitgitgadget
