{"thread":{"id":"52894","subject":"[PATCH] merge-recursive: apply collision handling unification to recursive case","startedAt":"2020-02-27T00:05:11Z","lastAt":"2020-02-27T00:05:11Z","messageCount":1,"participants":["Elijah Newren via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"392588","messageId":"pull.715.git.git.1582761905294.gitgitgadget@gmail.com","threadId":"52894","inReplyTo":null,"subject":"[PATCH] merge-recursive: apply collision handling unification to recursive case","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-27T00:05:05Z","receivedAt":"2020-02-27T00:05:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nIn the en/merge-path-collision topic (see commit ac193e0e0aa5, \"Merge\nbranch 'en/merge-path-collision'\", 2019-01-04), all the \"file collision\"\nconflict types were modified for consistency.  In particular,\nrename/add, rename/rename(2to1) and each rename/add piece of a\nrename/rename(1to2)/add[/add] conflict were made to behave like add/add\nconflicts have always been handled.\n\nHowever, this consistency was not enforced when opt->priv->call_depth >\n0 for rename/rename conflicts.  Update rename/rename(1to2) and\nrename/rename(2to1) conflicts in the recursive case to also be\nconsistent.  As an added bonus, this simplifies the code considerably.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    merge-recursive: apply collision handling unification to recursive case\n    \n    This commit ties up a loose end that I was unaware of with the\n    en/merge-path-collision topic from very early 2019.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-715%2Fnewren%2Frecursive-collision-handling-redux-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-715/newren/recursive-collision-handling-redux-v1\nPull-Request: https://github.com/git/git/pull/715\n\n merge-recursive.c                 | 152 +++++++++---------------------\n t/t6036-recursive-corner-cases.sh |  39 ++++++--\n 2 files changed, 77 insertions(+), 114 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex aee1769a7ac..3728b3f6598 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1557,35 +1557,6 @@ static int handle_file_collision(struct merge_options *opt,\n \t\t\t\t\t     b, a);\n \t}\n \n-\t/*\n-\t * In the recursive case, we just opt to undo renames\n-\t */\n-\tif (opt->priv->call_depth && (prev_path1 || prev_path2)) {\n-\t\t/* Put first file (a->oid, a->mode) in its original spot */\n-\t\tif (prev_path1) {\n-\t\t\tif (update_file(opt, 1, a, prev_path1))\n-\t\t\t\treturn -1;\n-\t\t} else {\n-\t\t\tif (update_file(opt, 1, a, collide_path))\n-\t\t\t\treturn -1;\n-\t\t}\n-\n-\t\t/* Put second file (b->oid, b->mode) in its original spot */\n-\t\tif (prev_path2) {\n-\t\t\tif (update_file(opt, 1, b, prev_path2))\n-\t\t\t\treturn -1;\n-\t\t} else {\n-\t\t\tif (update_file(opt, 1, b, collide_path))\n-\t\t\t\treturn -1;\n-\t\t}\n-\n-\t\t/* Don't leave something at collision path if unrenaming both */\n-\t\tif (prev_path1 && prev_path2)\n-\t\t\tremove_file(opt, 1, collide_path, 0);\n-\n-\t\treturn 0;\n-\t}\n-\n \t/* Remove rename sources if rename/add or rename/rename(2to1) */\n \tif (prev_path1)\n \t\tremove_file(opt, 1, prev_path1,\n@@ -1746,85 +1717,56 @@ static int handle_rename_rename_1to2(struct merge_options *opt,\n \t\treturn -1;\n \tfree(path_desc);\n \n-\tif (opt->priv->call_depth) {\n-\t\t/*\n-\t\t * FIXME: For rename/add-source conflicts (if we could detect\n-\t\t * such), this is wrong.  We should instead find a unique\n-\t\t * pathname and then either rename the add-source file to that\n-\t\t * unique path, or use that unique path instead of src here.\n-\t\t */\n-\t\tif (update_file(opt, 0, &mfi.blob, o->path))\n-\t\t\treturn -1;\n+\tif (opt->priv->call_depth)\n+\t\tremove_file_from_index(opt->repo->index, o->path);\n \n-\t\t/*\n-\t\t * Above, we put the merged content at the merge-base's\n-\t\t * path.  Now we usually need to delete both a->path and\n-\t\t * b->path.  However, the rename on each side of the merge\n-\t\t * could also be involved in a rename/add conflict.  In\n-\t\t * such cases, we should keep the added file around,\n-\t\t * resolving the conflict at that path in its favor.\n-\t\t */\n-\t\tadd = &ci->ren1->dst_entry->stages[flip_stage(2)];\n-\t\tif (is_valid(add)) {\n-\t\t\tif (update_file(opt, 0, add, a->path))\n-\t\t\t\treturn -1;\n-\t\t}\n-\t\telse\n-\t\t\tremove_file_from_index(opt->repo->index, a->path);\n-\t\tadd = &ci->ren2->dst_entry->stages[flip_stage(3)];\n-\t\tif (is_valid(add)) {\n-\t\t\tif (update_file(opt, 0, add, b->path))\n-\t\t\t\treturn -1;\n-\t\t}\n-\t\telse\n-\t\t\tremove_file_from_index(opt->repo->index, b->path);\n+\t/*\n+\t * For each destination path, we need to see if there is a\n+\t * rename/add collision.  If not, we can write the file out\n+\t * to the specified location.\n+\t */\n+\tadd = &ci->ren1->dst_entry->stages[flip_stage(2)];\n+\tif (is_valid(add)) {\n+\t\tadd->path = mfi.blob.path = a->path;\n+\t\tif (handle_file_collision(opt, a->path,\n+\t\t\t\t\t  NULL, NULL,\n+\t\t\t\t\t  ci->ren1->branch,\n+\t\t\t\t\t  ci->ren2->branch,\n+\t\t\t\t\t  &mfi.blob, add) < 0)\n+\t\t\treturn -1;\n \t} else {\n-\t\t/*\n-\t\t * For each destination path, we need to see if there is a\n-\t\t * rename/add collision.  If not, we can write the file out\n-\t\t * to the specified location.\n-\t\t */\n-\t\tadd = &ci->ren1->dst_entry->stages[flip_stage(2)];\n-\t\tif (is_valid(add)) {\n-\t\t\tadd->path = mfi.blob.path = a->path;\n-\t\t\tif (handle_file_collision(opt, a->path,\n-\t\t\t\t\t\t  NULL, NULL,\n-\t\t\t\t\t\t  ci->ren1->branch,\n-\t\t\t\t\t\t  ci->ren2->branch,\n-\t\t\t\t\t\t  &mfi.blob, add) < 0)\n-\t\t\t\treturn -1;\n-\t\t} else {\n-\t\t\tchar *new_path = find_path_for_conflict(opt, a->path,\n-\t\t\t\t\t\t\t\tci->ren1->branch,\n-\t\t\t\t\t\t\t\tci->ren2->branch);\n-\t\t\tif (update_file(opt, 0, &mfi.blob,\n-\t\t\t\t\tnew_path ? new_path : a->path))\n-\t\t\t\treturn -1;\n-\t\t\tfree(new_path);\n-\t\t\tif (update_stages(opt, a->path, NULL, a, NULL))\n-\t\t\t\treturn -1;\n-\t\t}\n+\t\tchar *new_path = find_path_for_conflict(opt, a->path,\n+\t\t\t\t\t\t\tci->ren1->branch,\n+\t\t\t\t\t\t\tci->ren2->branch);\n+\t\tif (update_file(opt, 0, &mfi.blob,\n+\t\t\t\tnew_path ? new_path : a->path))\n+\t\t\treturn -1;\n+\t\tfree(new_path);\n+\t\tif (!opt->priv->call_depth &&\n+\t\t    update_stages(opt, a->path, NULL, a, NULL))\n+\t\t\treturn -1;\n+\t}\n \n-\t\tadd = &ci->ren2->dst_entry->stages[flip_stage(3)];\n-\t\tif (is_valid(add)) {\n-\t\t\tadd->path = mfi.blob.path = b->path;\n-\t\t\tif (handle_file_collision(opt, b->path,\n-\t\t\t\t\t\t  NULL, NULL,\n-\t\t\t\t\t\t  ci->ren1->branch,\n-\t\t\t\t\t\t  ci->ren2->branch,\n-\t\t\t\t\t\t  add, &mfi.blob) < 0)\n-\t\t\t\treturn -1;\n-\t\t} else {\n-\t\t\tchar *new_path = find_path_for_conflict(opt, b->path,\n-\t\t\t\t\t\t\t\tci->ren2->branch,\n-\t\t\t\t\t\t\t\tci->ren1->branch);\n-\t\t\tif (update_file(opt, 0, &mfi.blob,\n-\t\t\t\t\tnew_path ? new_path : b->path))\n-\t\t\t\treturn -1;\n-\t\t\tfree(new_path);\n-\t\t\tif (update_stages(opt, b->path, NULL, NULL, b))\n-\t\t\t\treturn -1;\n-\t\t}\n+\tadd = &ci->ren2->dst_entry->stages[flip_stage(3)];\n+\tif (is_valid(add)) {\n+\t\tadd->path = mfi.blob.path = b->path;\n+\t\tif (handle_file_collision(opt, b->path,\n+\t\t\t\t\t  NULL, NULL,\n+\t\t\t\t\t  ci->ren1->branch,\n+\t\t\t\t\t  ci->ren2->branch,\n+\t\t\t\t\t  add, &mfi.blob) < 0)\n+\t\t\treturn -1;\n+\t} else {\n+\t\tchar *new_path = find_path_for_conflict(opt, b->path,\n+\t\t\t\t\t\t\tci->ren2->branch,\n+\t\t\t\t\t\t\tci->ren1->branch);\n+\t\tif (update_file(opt, 0, &mfi.blob,\n+\t\t\t\tnew_path ? new_path : b->path))\n+\t\t\treturn -1;\n+\t\tfree(new_path);\n+\t\tif (!opt->priv->call_depth &&\n+\t\t    update_stages(opt, b->path, NULL, NULL, b))\n+\t\t\treturn -1;\n \t}\n \n \treturn 0;\ndiff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh\nindex 7d73afdcdaa..b3bf4626178 100755\n--- a/t/t6036-recursive-corner-cases.sh\n+++ b/t/t6036-recursive-corner-cases.sh\n@@ -60,9 +60,9 @@ test_expect_success 'merge simple rename+criss-cross with no modifications' '\n \t\ttest_must_fail git merge -s recursive R2^0 &&\n \n \t\tgit ls-files -s >out &&\n-\t\ttest_line_count = 2 out &&\n+\t\ttest_line_count = 5 out &&\n \t\tgit ls-files -u >out &&\n-\t\ttest_line_count = 2 out &&\n+\t\ttest_line_count = 3 out &&\n \t\tgit ls-files -o >out &&\n \t\ttest_line_count = 1 out &&\n \n@@ -133,9 +133,9 @@ test_expect_success 'merge criss-cross + rename merges with basic modification'\n \t\ttest_must_fail git merge -s recursive R2^0 &&\n \n \t\tgit ls-files -s >out &&\n-\t\ttest_line_count = 2 out &&\n+\t\ttest_line_count = 5 out &&\n \t\tgit ls-files -u >out &&\n-\t\ttest_line_count = 2 out &&\n+\t\ttest_line_count = 3 out &&\n \t\tgit ls-files -o >out &&\n \t\ttest_line_count = 1 out &&\n \n@@ -218,8 +218,18 @@ test_expect_success 'git detects differently handled merges conflict' '\n \t\tgit ls-files -o >out &&\n \t\ttest_line_count = 1 out &&\n \n-\t\tgit rev-parse >expect       \\\n-\t\t\tC:new_a  D:new_a  E:new_a &&\n+\t\tgit cat-file -p C:new_a >ours &&\n+\t\tgit cat-file -p C:a >theirs &&\n+\t\t>empty &&\n+\t\ttest_must_fail git merge-file \\\n+\t\t\t-L \"Temporary merge branch 1\" \\\n+\t\t\t-L \"\" \\\n+\t\t\t-L \"Temporary merge branch 2\" \\\n+\t\t\tours empty theirs &&\n+\t\tsed -e \"s/^\\([<=>]\\)/\\1\\1\\1/\" ours >ours-tweaked &&\n+\t\tgit hash-object ours-tweaked >expect &&\n+\t\tgit rev-parse >>expect      \\\n+\t\t\t\t  D:new_a  E:new_a &&\n \t\tgit rev-parse   >actual     \\\n \t\t\t:1:new_a :2:new_a :3:new_a &&\n \t\ttest_cmp expect actual &&\n@@ -257,7 +267,8 @@ test_expect_success 'git detects differently handled merges conflict, swapped' '\n \t\tctime=$(git log --no-walk --date=raw --format=%cd C | awk \"{print \\$1}\") &&\n \t\tnewctime=$(($btime+1)) &&\n \t\tgit fast-export --no-data --all | sed -e s/$ctime/$newctime/ | git fast-import --force --quiet &&\n-\t\t# End of differences; rest is copy-paste of last test\n+\t\t# End of most differences; rest is copy-paste of last test,\n+\t\t# other than swapping C:a and C:new_a due to order switch\n \n \t\tgit checkout D^0 &&\n \t\ttest_must_fail git merge -s recursive E^0 &&\n@@ -269,8 +280,18 @@ test_expect_success 'git detects differently handled merges conflict, swapped' '\n \t\tgit ls-files -o >out &&\n \t\ttest_line_count = 1 out &&\n \n-\t\tgit rev-parse >expect       \\\n-\t\t\tC:new_a  D:new_a  E:new_a &&\n+\t\tgit cat-file -p C:a >ours &&\n+\t\tgit cat-file -p C:new_a >theirs &&\n+\t\t>empty &&\n+\t\ttest_must_fail git merge-file \\\n+\t\t\t-L \"Temporary merge branch 1\" \\\n+\t\t\t-L \"\" \\\n+\t\t\t-L \"Temporary merge branch 2\" \\\n+\t\t\tours empty theirs &&\n+\t\tsed -e \"s/^\\([<=>]\\)/\\1\\1\\1/\" ours >ours-tweaked &&\n+\t\tgit hash-object ours-tweaked >expect &&\n+\t\tgit rev-parse >>expect      \\\n+\t\t\t\t  D:new_a  E:new_a &&\n \t\tgit rev-parse   >actual     \\\n \t\t\t:1:new_a :2:new_a :3:new_a &&\n \t\ttest_cmp expect actual &&\n\nbase-commit: 2d2118b814c11f509e1aa76cb07110f7231668dc\n-- \ngitgitgadget\n"}]}