{"thread":{"id":"56010","subject":"[PATCH 1/3] t6423: test directory renames causing rename-to-self","startedAt":"2021-06-26T17:05:26Z","lastAt":"2021-06-30T17:30:11Z","messageCount":12,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Derrick Stolee","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"428524","messageId":"d3572e8bc85e4c7a33094f5da71957c2e59fd7f4.1624727121.git.gitgitgadget@gmail.com","threadId":"56010","inReplyTo":"pull.1039.git.git.1624727121.gitgitgadget@gmail.com","subject":"[PATCH 1/3] t6423: test directory renames causing rename-to-self","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-26T17:05:19Z","receivedAt":"2021-06-26T17:05:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nDirectory rename detection can cause transitive renames, e.g. if the two\ndifferent sides of history each do one half of:\n A/file -> B/file\n B/ -> C/\nthen directory rename detection transitively renames to give us C/file.\nSince the default for merge.directoryRenames is conflict, this results\nin an error message saying it is unclear whether the file should be\nplaced at B/file or C/file.\n\nWhat if C/ is A/, though? In such a case, the transitive rename would\ngive us A/file, the original name we started with. Logically, having\nan error message with B/file vs. A/file should be fine, as should\nleaving the file where it started. But the logic in both\nmerge-recursive and merge-ort did not handle a case of a filename being\nrenamed to itself correctly; merge-recursive had two bugs, and merge-ort\nhad one. Add some testcases covering such a scenario.\n\nBased-on-testcase-by: Anders Kaseorg \nSigned-off-by: Elijah Newren \n---\n t/t6423-merge-rename-directories.sh | 117 ++++++++++++++++++++++++++++\n 1 file changed, 117 insertions(+)\n\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex be84d22419d..2a2ab907338 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5024,6 +5024,123 @@ test_expect_failure '12h: renaming a file within a renamed directory' '\n \t)\n '\n \n+# Testcase 12i, Directory rename causes rename-to-self\n+# Commit O: source/{subdir/foo, bar, baz_1}\n+# Commit A: source/{foo, bar, baz_1}\n+# Commit B: source/{subdir/{foo, bar}, baz_2}\n+# Expected: source/{foo, bar, baz_2}, with conflicts on\n+# source/bar vs. source/subdir/bar\n+\n+test_setup_12i () {\n+\ttest_create_repo 12i &&\n+\t(\n+\t\tcd 12i &&\n+\n+\t\tmkdir -p source/subdir &&\n+\t\techo foo >source/subdir/foo &&\n+\t\techo bar >source/bar &&\n+\t\techo baz >source/baz &&\n+\t\tgit add source &&\n+\t\tgit commit -m orig &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit switch A &&\n+\t\tgit mv source/subdir/foo source/foo &&\n+\t\tgit commit -m A &&\n+\n+\t\tgit switch B &&\n+\t\tgit mv source/bar source/subdir/bar &&\n+\t\techo more baz >>source/baz &&\n+\t\tgit commit -m B\n+\t)\n+}\n+\n+test_expect_merge_algorithm failure failure '12i: Directory rename causes rename-to-self' '\n+\ttest_setup_12i &&\n+\t(\n+\t\tcd 12i &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&\n+\n+\t\ttest_path_is_missing source/subdir &&\n+\t\ttest_path_is_file source/bar &&\n+\t\ttest_path_is_file source/baz &&\n+\n+\t\tgit ls-files | uniq >tracked &&\n+\t\ttest_line_count = 3 tracked &&\n+\n+\t\tgit status --porcelain -uno >actual &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\tUU source/bar\n+\t\t M source/baz\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+# Testcase 12j, Directory rename to root causes rename-to-self\n+# Commit O: {subdir/foo, bar, baz_1}\n+# Commit A: {foo, bar, baz_1}\n+# Commit B: {subdir/{foo, bar}, baz_2}\n+# Expected: {foo, bar, baz_2}, with conflicts on bar vs. subdir/bar\n+\n+test_setup_12j () {\n+\ttest_create_repo 12j &&\n+\t(\n+\t\tcd 12j &&\n+\n+\t\tmkdir -p subdir &&\n+\t\techo foo >subdir/foo &&\n+\t\techo bar >bar &&\n+\t\techo baz >baz &&\n+\t\tgit add . &&\n+\t\tgit commit -m orig &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit switch A &&\n+\t\tgit mv subdir/foo foo &&\n+\t\tgit commit -m A &&\n+\n+\t\tgit switch B &&\n+\t\tgit mv bar subdir/bar &&\n+\t\techo more baz >>baz &&\n+\t\tgit commit -m B\n+\t)\n+}\n+\n+test_expect_merge_algorithm failure failure '12j: Directory rename to root causes rename-to-self' '\n+\ttest_setup_12j &&\n+\t(\n+\t\tcd 12j &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&\n+\n+\t\ttest_path_is_missing subdir &&\n+\t\ttest_path_is_file bar &&\n+\t\ttest_path_is_file baz &&\n+\n+\t\tgit ls-files | uniq >tracked &&\n+\t\ttest_line_count = 3 tracked &&\n+\n+\t\tgit status --porcelain -uno >actual &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\tUU bar\n+\t\t M baz\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n ###########################################################################\n # SECTION 13: Checking informational and conflict messages\n #\n-- \ngitgitgadget\n\n"},{"id":"428525","messageId":"pull.1039.git.git.1624727121.gitgitgadget@gmail.com","threadId":"56010","inReplyTo":null,"subject":"[PATCH 0/3] Fix bugs from interesting renaming pairs: one side renames A/file -> B/file, the other B/ -> A/","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-26T17:05:18Z","receivedAt":"2021-06-26T17:05:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Anders Kaseorg recently reported a few issues in an interesting rename\ncase[1]. I was able to duplicate and find multiple bugs from it; two in\nmerge-recursive, and one in merge-ort. This series has some fixes.\n\n[1]\nhttps://lore.kernel.org/git/CABPp-BGDfucqae=HNES_QmmsjpDbdHrR6CG=H3gtiDygHzquVg@mail.gmail.com/\n\nElijah Newren (3):\n t6423: test directory renames causing rename-to-self\n merge-ort: ensure we consult df_conflict and path_conflicts\n merge-recursive: handle rename-to-self case\n\n merge-ort.c | 6 +-\n merge-recursive.c | 19 +++--\n t/t6423-merge-rename-directories.sh | 117 ++++++++++++++++++++++++++++\n 3 files changed, 135 insertions(+), 7 deletions(-)\n\n\nbase-commit: 670b81a890388c60b7032a4f5b879f2ece8c4558\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1039%2Fnewren%2Frename-plus-dir-rename-cancel-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1039/newren/rename-plus-dir-rename-cancel-v1\nPull-Request: https://github.com/git/git/pull/1039\n-- \ngitgitgadget\n"},{"id":"428526","messageId":"052f40c3c1a6b35f9253faf698f8cbc87f81675e.1624727121.git.gitgitgadget@gmail.com","threadId":"56010","inReplyTo":"pull.1039.git.git.1624727121.gitgitgadget@gmail.com","subject":"[PATCH 2/3] merge-ort: ensure we consult df_conflict and path_conflicts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-26T17:05:20Z","receivedAt":"2021-06-26T17:05:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nPath conflicts (typically rename path conflicts, e.g.\nrename/rename(1to2) or rename/add/delete), and directory/file conflicts\nshould obviously result in files not being marked as clean in the merge.\nWe had a codepath where we missed consulting the path_conflict and\ndf_conflict flags, based on match_mask. Granted, it requires an unusual\nsetup to trigger this codepath (directory rename causing rename-to-self\nis the only case I can think of), but we still need to handle it. To\nmake it clear that we have audited the other codepaths that do not\nexplicitly mention these flags, add some assertions that the flags are\nnot set.\n\nReported-by: Anders Kaseorg \nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 6 +++++-\n t/t6423-merge-rename-directories.sh | 4 ++--\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex b954f7184a5..373dbac5079 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3237,7 +3237,7 @@ static void process_entry(struct merge_options *opt,\n \t * above.\n \t */\n \tif (ci->match_mask) {\n-\t\tci->merged.clean = 1;\n+\t\tci->merged.clean = !ci->df_conflict && !ci->path_conflict;\n \t\tif (ci->match_mask == 6) {\n \t\t\t/* stages[1] == stages[2] */\n \t\t\tci->merged.result.mode = ci->stages[1].mode;\n@@ -3249,6 +3249,8 @@ static void process_entry(struct merge_options *opt,\n \n \t\t\tci->merged.result.mode = ci->stages[side].mode;\n \t\t\tci->merged.is_null = !ci->merged.result.mode;\n+\t\t\tif (ci->merged.is_null)\n+\t\t\t\tci->merged.clean = 1;\n \t\t\toidcpy(&ci->merged.result.oid, &ci->stages[side].oid);\n \n \t\t\tassert(othermask == 2 || othermask == 4);\n@@ -3421,6 +3423,7 @@ static void process_entry(struct merge_options *opt,\n \t\t\t\t path)) {\n \t\t\tci->merged.is_null = 1;\n \t\t\tci->merged.clean = 1;\n+\t\t\tassert(!ci->df_conflict && !ci->path_conflict);\n \t\t} else if (ci->path_conflict &&\n \t\t\t oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {\n \t\t\t/*\n@@ -3447,6 +3450,7 @@ static void process_entry(struct merge_options *opt,\n \t\tci->merged.is_null = 1;\n \t\tci->merged.result.mode = 0;\n \t\toidcpy(&ci->merged.result.oid, null_oid());\n+\t\tassert(!ci->df_conflict);\n \t\tci->merged.clean = !ci->path_conflict;\n \t}\n \ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 2a2ab907338..7480daab46a 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5058,7 +5058,7 @@ test_setup_12i () {\n \t)\n }\n \n-test_expect_merge_algorithm failure failure '12i: Directory rename causes rename-to-self' '\n+test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' '\n \ttest_setup_12i &&\n \t(\n \t\tcd 12i &&\n@@ -5116,7 +5116,7 @@ test_setup_12j () {\n \t)\n }\n \n-test_expect_merge_algorithm failure failure '12j: Directory rename to root causes rename-to-self' '\n+test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' '\n \ttest_setup_12j &&\n \t(\n \t\tcd 12j &&\n-- \ngitgitgadget\n\n"},{"id":"428527","messageId":"dea97c25e52c0c5dcba7698cdc2af742344fe36d.1624727122.git.gitgitgadget@gmail.com","threadId":"56010","inReplyTo":"pull.1039.git.git.1624727121.gitgitgadget@gmail.com","subject":"[PATCH 3/3] merge-recursive: handle rename-to-self case","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-26T17:05:21Z","receivedAt":"2021-06-26T17:05:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nDirectory rename detection can cause transitive renames, e.g. if the two\ndifferent sides of history each do one half of:\n A/file -> B/file\n B/ -> C/\nthen directory rename detection transitively renames to give us\n A/file -> C/file\n\nHowever, when C/ == A/, note that this gives us\n A/file -> A/file.\n\nmerge-recursive assumed that any rename D -> E would have D != E. While\nthat is almost always true, the above is a special case where it is not.\nSo we cannot do things like delete the rename source, we cannot assume\nthat a file existing at path E implies a rename/add conflict and we have\nto be careful about what stages end up in the output.\n\nThis change feels a bit hackish. It took me surprisingly many hours to\nfind, and given merge-recursive's design causing it to attempt to\nenumerate all combinations of edge and corner cases with special code\nfor each combination, I'm worried there are other similar fixes needed\nelsewhere if we can just come up with the right special testcase.\nPerhaps an audit would rule it out, but I have not the energy.\nmerge-recursive deserves to die, and since it is on its way out anyway,\nfixing this particular bug narrowly will have to be good enough.\n\nReported-by: Anders Kaseorg \nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 19 +++++++++++++------\n t/t6423-merge-rename-directories.sh | 4 ++--\n 2 files changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d146bb116f7..c895145a8f5 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2804,12 +2804,19 @@ static int process_renames(struct merge_options *opt,\n \t\t\tint renamed_stage = a_renames == renames1 ? 2 : 3;\n \t\t\tint other_stage = a_renames == renames1 ? 3 : 2;\n \n+\t\t\t/*\n+\t\t\t * Directory renames have a funny corner case...\n+\t\t\t */\n+\t\t\tint renamed_to_self = !strcmp(ren1_src, ren1_dst);\n+\n \t\t\t/* BUG: We should only remove ren1_src in the base\n \t\t\t * stage and in other_stage (think of rename +\n \t\t\t * add-source case).\n \t\t\t */\n-\t\t\tremove_file(opt, 1, ren1_src,\n-\t\t\t\t renamed_stage == 2 || !was_tracked(opt, ren1_src));\n+\t\t\tif (!renamed_to_self)\n+\t\t\t\tremove_file(opt, 1, ren1_src,\n+\t\t\t\t\t renamed_stage == 2 ||\n+\t\t\t\t\t !was_tracked(opt, ren1_src));\n \n \t\t\toidcpy(&src_other.oid,\n \t\t\t &ren1->src_entry->stages[other_stage].oid);\n@@ -2823,6 +2830,9 @@ static int process_renames(struct merge_options *opt,\n \t\t\t ren1->dir_rename_original_type == 'A') {\n \t\t\t\tsetup_rename_conflict_info(RENAME_VIA_DIR,\n \t\t\t\t\t\t\t opt, ren1, NULL);\n+\t\t\t} else if (renamed_to_self) {\n+\t\t\t\tsetup_rename_conflict_info(RENAME_NORMAL,\n+\t\t\t\t\t\t\t opt, ren1, NULL);\n \t\t\t} else if (oideq(&src_other.oid, null_oid())) {\n \t\t\t\tsetup_rename_conflict_info(RENAME_DELETE,\n \t\t\t\t\t\t\t opt, ren1, NULL);\n@@ -3180,7 +3190,6 @@ static int handle_rename_normal(struct merge_options *opt,\n \tstruct rename *ren = ci->ren1;\n \tstruct merge_file_info mfi;\n \tint clean;\n-\tint side = (ren->branch == opt->branch1 ? 2 : 3);\n \n \t/* Merge the content and write it out */\n \tclean = handle_content_merge(&mfi, opt, path, was_dirty(opt, path),\n@@ -3190,9 +3199,7 @@ static int handle_rename_normal(struct merge_options *opt,\n \t opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT &&\n \t ren->dir_rename_original_dest) {\n \t\tif (update_stages(opt, path,\n-\t\t\t\t NULL,\n-\t\t\t\t side == 2 ? &mfi.blob : NULL,\n-\t\t\t\t side == 2 ? NULL : &mfi.blob))\n+\t\t\t\t &mfi.blob, &mfi.blob, &mfi.blob))\n \t\t\treturn -1;\n \t\tclean = 0; /* not clean, but conflicted */\n \t}\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 7480daab46a..d67b5c28944 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5058,7 +5058,7 @@ test_setup_12i () {\n \t)\n }\n \n-test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' '\n+test_expect_success '12i: Directory rename causes rename-to-self' '\n \ttest_setup_12i &&\n \t(\n \t\tcd 12i &&\n@@ -5116,7 +5116,7 @@ test_setup_12j () {\n \t)\n }\n \n-test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' '\n+test_expect_success '12j: Directory rename to root causes rename-to-self' '\n \ttest_setup_12j &&\n \t(\n \t\tcd 12j &&\n-- \ngitgitgadget\n"},{"id":"428710","messageId":"xmqqeecl1foi.fsf@gitster.g","threadId":"56010","inReplyTo":"dea97c25e52c0c5dcba7698cdc2af742344fe36d.1624727122.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] merge-recursive: handle rename-to-self case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-29T04:02:37Z","receivedAt":"2021-06-29T04:02:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> Directory rename detection can cause transitive renames, e.g. if the two\n> different sides of history each do one half of:\n> A/file -> B/file\n> B/ -> C/\n> then directory rename detection transitively renames to give us\n> A/file -> C/file\n>\n> However, when C/ == A/, note that this gives us\n> A/file -> A/file.\n\nHeh, of course this is recent because the \"guess that directory B\nhas been moved to C in its entirety\" heuristic is quite new.\n\nNicely fixed and described. Will queue. Thanks.\n\n\n>\n> merge-recursive assumed that any rename D -> E would have D != E. While\n> that is almost always true, the above is a special case where it is not.\n> So we cannot do things like delete the rename source, we cannot assume\n> that a file existing at path E implies a rename/add conflict and we have\n> to be careful about what stages end up in the output.\n>\n> This change feels a bit hackish. It took me surprisingly many hours to\n> find, and given merge-recursive's design causing it to attempt to\n> enumerate all combinations of edge and corner cases with special code\n> for each combination, I'm worried there are other similar fixes needed\n> elsewhere if we can just come up with the right special testcase.\n> Perhaps an audit would rule it out, but I have not the energy.\n> merge-recursive deserves to die, and since it is on its way out anyway,\n> fixing this particular bug narrowly will have to be good enough.\n>\n> Reported-by: Anders Kaseorg \n> Signed-off-by: Elijah Newren \n> ---\n> merge-recursive.c | 19 +++++++++++++------\n> t/t6423-merge-rename-directories.sh | 4 ++--\n> 2 files changed, 15 insertions(+), 8 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index d146bb116f7..c895145a8f5 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -2804,12 +2804,19 @@ static int process_renames(struct merge_options *opt,\n> \t\t\tint renamed_stage = a_renames == renames1 ? 2 : 3;\n> \t\t\tint other_stage = a_renames == renames1 ? 3 : 2;\n> \n> +\t\t\t/*\n> +\t\t\t * Directory renames have a funny corner case...\n> +\t\t\t */\n> +\t\t\tint renamed_to_self = !strcmp(ren1_src, ren1_dst);\n> +\n> \t\t\t/* BUG: We should only remove ren1_src in the base\n> \t\t\t * stage and in other_stage (think of rename +\n> \t\t\t * add-source case).\n> \t\t\t */\n> -\t\t\tremove_file(opt, 1, ren1_src,\n> -\t\t\t\t renamed_stage == 2 || !was_tracked(opt, ren1_src));\n> +\t\t\tif (!renamed_to_self)\n> +\t\t\t\tremove_file(opt, 1, ren1_src,\n> +\t\t\t\t\t renamed_stage == 2 ||\n> +\t\t\t\t\t !was_tracked(opt, ren1_src));\n> \n> \t\t\toidcpy(&src_other.oid,\n> \t\t\t &ren1->src_entry->stages[other_stage].oid);\n> @@ -2823,6 +2830,9 @@ static int process_renames(struct merge_options *opt,\n> \t\t\t ren1->dir_rename_original_type == 'A') {\n> \t\t\t\tsetup_rename_conflict_info(RENAME_VIA_DIR,\n> \t\t\t\t\t\t\t opt, ren1, NULL);\n> +\t\t\t} else if (renamed_to_self) {\n> +\t\t\t\tsetup_rename_conflict_info(RENAME_NORMAL,\n> +\t\t\t\t\t\t\t opt, ren1, NULL);\n> \t\t\t} else if (oideq(&src_other.oid, null_oid())) {\n> \t\t\t\tsetup_rename_conflict_info(RENAME_DELETE,\n> \t\t\t\t\t\t\t opt, ren1, NULL);\n> @@ -3180,7 +3190,6 @@ static int handle_rename_normal(struct merge_options *opt,\n> \tstruct rename *ren = ci->ren1;\n> \tstruct merge_file_info mfi;\n> \tint clean;\n> -\tint side = (ren->branch == opt->branch1 ? 2 : 3);\n> \n> \t/* Merge the content and write it out */\n> \tclean = handle_content_merge(&mfi, opt, path, was_dirty(opt, path),\n> @@ -3190,9 +3199,7 @@ static int handle_rename_normal(struct merge_options *opt,\n> \t opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT &&\n> \t ren->dir_rename_original_dest) {\n> \t\tif (update_stages(opt, path,\n> -\t\t\t\t NULL,\n> -\t\t\t\t side == 2 ? &mfi.blob : NULL,\n> -\t\t\t\t side == 2 ? NULL : &mfi.blob))\n> +\t\t\t\t &mfi.blob, &mfi.blob, &mfi.blob))\n> \t\t\treturn -1;\n> \t\tclean = 0; /* not clean, but conflicted */\n> \t}\n> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> index 7480daab46a..d67b5c28944 100755\n> --- a/t/t6423-merge-rename-directories.sh\n> +++ b/t/t6423-merge-rename-directories.sh\n> @@ -5058,7 +5058,7 @@ test_setup_12i () {\n> \t)\n> }\n> \n> -test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' '\n> +test_expect_success '12i: Directory rename causes rename-to-self' '\n> \ttest_setup_12i &&\n> \t(\n> \t\tcd 12i &&\n> @@ -5116,7 +5116,7 @@ test_setup_12j () {\n> \t)\n> }\n> \n> -test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' '\n> +test_expect_success '12j: Directory rename to root causes rename-to-self' '\n> \ttest_setup_12j &&\n> \t(\n> \t\tcd 12j &&\n"},{"id":"428744","messageId":"c088028d-3045-70ce-19e9-f3ffc9c57c98@gmail.com","threadId":"56010","inReplyTo":"d3572e8bc85e4c7a33094f5da71957c2e59fd7f4.1624727121.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] t6423: test directory renames causing rename-to-self","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-06-29T12:50:33Z","receivedAt":"2021-06-29T12:50:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/26/2021 1:05 PM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> Directory rename detection can cause transitive renames, e.g. if the two\n> different sides of history each do one half of:\n> A/file -> B/file\n> B/ -> C/\n> then directory rename detection transitively renames to give us C/file.\n> Since the default for merge.directoryRenames is conflict, this results\n> in an error message saying it is unclear whether the file should be\n> placed at B/file or C/file.\n> \n> What if C/ is A/, though?\n\nThis case seems interesting, but somehow missing from the test cases\nbelow. Each of those cases include renaming up or down the directory\nhierarchy instead of doing a sideways rename.\n\n> +# Testcase 12i, Directory rename causes rename-to-self\n> +# Commit O: source/{subdir/foo, bar, baz_1}\n> +# Commit A: source/{foo, bar, baz_1}\n> +# Commit B: source/{subdir/{foo, bar}, baz_2}\n> +# Expected: source/{foo, bar, baz_2}, with conflicts on\n> +# source/bar vs. source/subdir/bar\n\nThis test goes deeper.\n\n> +# Testcase 12j, Directory rename to root causes rename-to-self\n> +# Commit O: {subdir/foo, bar, baz_1}\n> +# Commit A: {foo, bar, baz_1}\n> +# Commit B: {subdir/{foo, bar}, baz_2}\n> +# Expected: {foo, bar, baz_2}, with conflicts on bar vs. subdir/bar\n\nThis test goes higher.\n\nDoes the problematic case not hit when going out to the side, such\nas \"with conflicts on subdir/bar vs otherdir/bar\"?\n\nIf so, then _maybe_ the commit message could indicate this is an\nomission on purpose. If not, then maybe a third test should be\nadded?\n\nThanks,\n-Stolee\n"},{"id":"428745","messageId":"7854e7b7-f128-e389-cc06-8b599c7b3e32@gmail.com","threadId":"56010","inReplyTo":"xmqqeecl1foi.fsf@gitster.g","subject":"Re: [PATCH 3/3] merge-recursive: handle rename-to-self case","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-06-29T12:55:29Z","receivedAt":"2021-06-29T12:55:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/29/2021 12:02 AM, Junio C Hamano wrote:\n> \"Elijah Newren via GitGitGadget\" writes:\n> \n>> From: Elijah Newren \n>>\n>> Directory rename detection can cause transitive renames, e.g. if the two\n>> different sides of history each do one half of:\n>> A/file -> B/file\n>> B/ -> C/\n>> then directory rename detection transitively renames to give us\n>> A/file -> C/file\n>>\n>> However, when C/ == A/, note that this gives us\n>> A/file -> A/file.\n> \n> Heh, of course this is recent because the \"guess that directory B\n> has been moved to C in its entirety\" heuristic is quite new.\n> \n> Nicely fixed and described. Will queue. Thanks.\n\nI agree that the fixes in patches 2 and 3 are rather clean. The\ntests added in patch 1 provide significant confidence in the\nresult, though I had a minor question about a possible third\ntest case.\n\nSeeing the solutions that make the two tests pass leaves me to\nconsider that the third test case is likely unnecessary, because\nthere is nothing special about how the directories are being\nrenamed that must be handled by the fix.\n\nThanks,\n-Stolee\n"},{"id":"428827","messageId":"CABPp-BFsXWNzS-6JSM7rYN0LLtTmT3UsgG7aTUmpSq6K2jHyFg@mail.gmail.com","threadId":"56010","inReplyTo":"c088028d-3045-70ce-19e9-f3ffc9c57c98@gmail.com","subject":"Re: [PATCH 1/3] t6423: test directory renames causing rename-to-self","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-30T16:33:13Z","receivedAt":"2021-06-30T16:33:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jun 29, 2021 at 5:50 AM Derrick Stolee wrote:\n>\n> On 6/26/2021 1:05 PM, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > Directory rename detection can cause transitive renames, e.g. if the two\n> > different sides of history each do one half of:\n> > A/file -> B/file\n> > B/ -> C/\n> > then directory rename detection transitively renames to give us C/file.\n> > Since the default for merge.directoryRenames is conflict, this results\n> > in an error message saying it is unclear whether the file should be\n> > placed at B/file or C/file.\n> >\n> > What if C/ is A/, though?\n>\n> This case seems interesting, but somehow missing from the test cases\n> below. Each of those cases include renaming up or down the directory\n> hierarchy instead of doing a sideways rename.\n>\n> > +# Testcase 12i, Directory rename causes rename-to-self\n> > +# Commit O: source/{subdir/foo, bar, baz_1}\n> > +# Commit A: source/{foo, bar, baz_1}\n> > +# Commit B: source/{subdir/{foo, bar}, baz_2}\n> > +# Expected: source/{foo, bar, baz_2}, with conflicts on\n> > +# source/bar vs. source/subdir/bar\n>\n> This test goes deeper.\n>\n> > +# Testcase 12j, Directory rename to root causes rename-to-self\n> > +# Commit O: {subdir/foo, bar, baz_1}\n> > +# Commit A: {foo, bar, baz_1}\n> > +# Commit B: {subdir/{foo, bar}, baz_2}\n> > +# Expected: {foo, bar, baz_2}, with conflicts on bar vs. subdir/bar\n>\n> This test goes higher.\n>\n> Does the problematic case not hit when going out to the side, such\n> as \"with conflicts on subdir/bar vs otherdir/bar\"?\n>\n> If so, then _maybe_ the commit message could indicate this is an\n> omission on purpose. If not, then maybe a third test should be\n> added?\n\nThe only special case in the code is renaming to the root directory,\nso I didn't think of extending beyond two cases. However, while\nadding more testcases doesn't exercise the current incarnation of the\ncode further, it's pretty easy to add another and it certainly doesn't\nhurt. I'll resubmit with a third testcase.\n"},{"id":"428837","messageId":"pull.1039.v2.git.git.1625074200.gitgitgadget@gmail.com","threadId":"56010","inReplyTo":"pull.1039.git.git.1624727121.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Fix bugs from interesting renaming pairs: one side renames A/file -> B/file, the other B/ -> A/","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-30T17:29:57Z","receivedAt":"2021-06-30T17:30:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Anders Kaseorg recently reported a few issues in an interesting rename\ncase[1]. I was able to duplicate and find multiple bugs from it; two in\nmerge-recursive, and one in merge-ort. This series has some fixes.\n\nChanges since v1:\n\n * Added a third testcase\n\n[1]\nhttps://lore.kernel.org/git/CABPp-BGDfucqae=HNES_QmmsjpDbdHrR6CG=H3gtiDygHzquVg@mail.gmail.com/\n\nElijah Newren (3):\n t6423: test directory renames causing rename-to-self\n merge-ort: ensure we consult df_conflict and path_conflicts\n merge-recursive: handle rename-to-self case\n\n merge-ort.c | 6 +-\n merge-recursive.c | 19 ++-\n t/t6423-merge-rename-directories.sh | 175 ++++++++++++++++++++++++++++\n 3 files changed, 193 insertions(+), 7 deletions(-)\n\n\nbase-commit: 670b81a890388c60b7032a4f5b879f2ece8c4558\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1039%2Fnewren%2Frename-plus-dir-rename-cancel-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1039/newren/rename-plus-dir-rename-cancel-v2\nPull-Request: https://github.com/git/git/pull/1039\n\nRange-diff vs v1:\n\n 1: d3572e8bc85 ! 1: 86b2eaea75e t6423: test directory renames causing rename-to-self\n @@ t/t6423-merge-rename-directories.sh: test_expect_failure '12h: renaming a file w\n +\t\ttest_cmp expect actual\n +\t)\n +'\n ++\n ++# Testcase 12k, Directory rename with sibling causes rename-to-self\n ++# Commit O: dirB/foo, dirA/{bar, baz_1}\n ++# Commit A: dirA/{foo, bar, baz_1}\n ++# Commit B: dirB/{foo, bar}, dirA/baz_2\n ++# Expected: dirA/{foo, bar, baz_2}, with conflicts on dirA/bar vs. dirB/bar\n ++\n ++test_setup_12k () {\n ++\ttest_create_repo 12k &&\n ++\t(\n ++\t\tcd 12k &&\n ++\n ++\t\tmkdir dirA dirB &&\n ++\t\techo foo >dirB/foo &&\n ++\t\techo bar >dirA/bar &&\n ++\t\techo baz >dirA/baz &&\n ++\t\tgit add . &&\n ++\t\tgit commit -m orig &&\n ++\n ++\t\tgit branch O &&\n ++\t\tgit branch A &&\n ++\t\tgit branch B &&\n ++\n ++\t\tgit switch A &&\n ++\t\tgit mv dirB/* dirA/ &&\n ++\t\tgit commit -m A &&\n ++\n ++\t\tgit switch B &&\n ++\t\tgit mv dirA/bar dirB/bar &&\n ++\t\techo more baz >>dirA/baz &&\n ++\t\tgit commit -m B\n ++\t)\n ++}\n ++\n ++test_expect_merge_algorithm failure failure '12k: Directory rename with sibling causes rename-to-self' '\n ++\ttest_setup_12k &&\n ++\t(\n ++\t\tcd 12k &&\n ++\n ++\t\tgit checkout A^0 &&\n ++\n ++\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&\n ++\n ++\t\ttest_path_is_missing dirB &&\n ++\t\ttest_path_is_file dirA/bar &&\n ++\t\ttest_path_is_file dirA/baz &&\n ++\n ++\t\tgit ls-files | uniq >tracked &&\n ++\t\ttest_line_count = 3 tracked &&\n ++\n ++\t\tgit status --porcelain -uno >actual &&\n ++\t\tcat >expect <<-\\EOF &&\n ++\t\tUU dirA/bar\n ++\t\t M dirA/baz\n ++\t\tEOF\n ++\t\ttest_cmp expect actual\n ++\t)\n ++'\n +\n ###########################################################################\n # SECTION 13: Checking informational and conflict messages\n 2: 052f40c3c1a ! 2: 6e7ef18bf53 merge-ort: ensure we consult df_conflict and path_conflicts\n @@ t/t6423-merge-rename-directories.sh: test_setup_12j () {\n \ttest_setup_12j &&\n \t(\n \t\tcd 12j &&\n +@@ t/t6423-merge-rename-directories.sh: test_setup_12k () {\n + \t)\n + }\n + \n +-test_expect_merge_algorithm failure failure '12k: Directory rename with sibling causes rename-to-self' '\n ++test_expect_merge_algorithm failure success '12k: Directory rename with sibling causes rename-to-self' '\n + \ttest_setup_12k &&\n + \t(\n + \t\tcd 12k &&\n 3: dea97c25e52 ! 3: 7c96d6c7a0a merge-recursive: handle rename-to-self case\n @@ t/t6423-merge-rename-directories.sh: test_setup_12j () {\n \ttest_setup_12j &&\n \t(\n \t\tcd 12j &&\n +@@ t/t6423-merge-rename-directories.sh: test_setup_12k () {\n + \t)\n + }\n + \n +-test_expect_merge_algorithm failure success '12k: Directory rename with sibling causes rename-to-self' '\n ++test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\n + \ttest_setup_12k &&\n + \t(\n + \t\tcd 12k &&\n\n-- \ngitgitgadget\n"},{"id":"428838","messageId":"86b2eaea75e5e23626c41bf61bf882591331bbef.1625074200.git.gitgitgadget@gmail.com","threadId":"56010","inReplyTo":"pull.1039.v2.git.git.1625074200.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] t6423: test directory renames causing rename-to-self","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-30T17:29:58Z","receivedAt":"2021-06-30T17:30:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nDirectory rename detection can cause transitive renames, e.g. if the two\ndifferent sides of history each do one half of:\n A/file -> B/file\n B/ -> C/\nthen directory rename detection transitively renames to give us C/file.\nSince the default for merge.directoryRenames is conflict, this results\nin an error message saying it is unclear whether the file should be\nplaced at B/file or C/file.\n\nWhat if C/ is A/, though? In such a case, the transitive rename would\ngive us A/file, the original name we started with. Logically, having\nan error message with B/file vs. A/file should be fine, as should\nleaving the file where it started. But the logic in both\nmerge-recursive and merge-ort did not handle a case of a filename being\nrenamed to itself correctly; merge-recursive had two bugs, and merge-ort\nhad one. Add some testcases covering such a scenario.\n\nBased-on-testcase-by: Anders Kaseorg \nSigned-off-by: Elijah Newren \n---\n t/t6423-merge-rename-directories.sh | 175 ++++++++++++++++++++++++++++\n 1 file changed, 175 insertions(+)\n\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex be84d22419d..89fe6778b94 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5024,6 +5024,181 @@ test_expect_failure '12h: renaming a file within a renamed directory' '\n \t)\n '\n \n+# Testcase 12i, Directory rename causes rename-to-self\n+# Commit O: source/{subdir/foo, bar, baz_1}\n+# Commit A: source/{foo, bar, baz_1}\n+# Commit B: source/{subdir/{foo, bar}, baz_2}\n+# Expected: source/{foo, bar, baz_2}, with conflicts on\n+# source/bar vs. source/subdir/bar\n+\n+test_setup_12i () {\n+\ttest_create_repo 12i &&\n+\t(\n+\t\tcd 12i &&\n+\n+\t\tmkdir -p source/subdir &&\n+\t\techo foo >source/subdir/foo &&\n+\t\techo bar >source/bar &&\n+\t\techo baz >source/baz &&\n+\t\tgit add source &&\n+\t\tgit commit -m orig &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit switch A &&\n+\t\tgit mv source/subdir/foo source/foo &&\n+\t\tgit commit -m A &&\n+\n+\t\tgit switch B &&\n+\t\tgit mv source/bar source/subdir/bar &&\n+\t\techo more baz >>source/baz &&\n+\t\tgit commit -m B\n+\t)\n+}\n+\n+test_expect_merge_algorithm failure failure '12i: Directory rename causes rename-to-self' '\n+\ttest_setup_12i &&\n+\t(\n+\t\tcd 12i &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&\n+\n+\t\ttest_path_is_missing source/subdir &&\n+\t\ttest_path_is_file source/bar &&\n+\t\ttest_path_is_file source/baz &&\n+\n+\t\tgit ls-files | uniq >tracked &&\n+\t\ttest_line_count = 3 tracked &&\n+\n+\t\tgit status --porcelain -uno >actual &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\tUU source/bar\n+\t\t M source/baz\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+# Testcase 12j, Directory rename to root causes rename-to-self\n+# Commit O: {subdir/foo, bar, baz_1}\n+# Commit A: {foo, bar, baz_1}\n+# Commit B: {subdir/{foo, bar}, baz_2}\n+# Expected: {foo, bar, baz_2}, with conflicts on bar vs. subdir/bar\n+\n+test_setup_12j () {\n+\ttest_create_repo 12j &&\n+\t(\n+\t\tcd 12j &&\n+\n+\t\tmkdir -p subdir &&\n+\t\techo foo >subdir/foo &&\n+\t\techo bar >bar &&\n+\t\techo baz >baz &&\n+\t\tgit add . &&\n+\t\tgit commit -m orig &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit switch A &&\n+\t\tgit mv subdir/foo foo &&\n+\t\tgit commit -m A &&\n+\n+\t\tgit switch B &&\n+\t\tgit mv bar subdir/bar &&\n+\t\techo more baz >>baz &&\n+\t\tgit commit -m B\n+\t)\n+}\n+\n+test_expect_merge_algorithm failure failure '12j: Directory rename to root causes rename-to-self' '\n+\ttest_setup_12j &&\n+\t(\n+\t\tcd 12j &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&\n+\n+\t\ttest_path_is_missing subdir &&\n+\t\ttest_path_is_file bar &&\n+\t\ttest_path_is_file baz &&\n+\n+\t\tgit ls-files | uniq >tracked &&\n+\t\ttest_line_count = 3 tracked &&\n+\n+\t\tgit status --porcelain -uno >actual &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\tUU bar\n+\t\t M baz\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+# Testcase 12k, Directory rename with sibling causes rename-to-self\n+# Commit O: dirB/foo, dirA/{bar, baz_1}\n+# Commit A: dirA/{foo, bar, baz_1}\n+# Commit B: dirB/{foo, bar}, dirA/baz_2\n+# Expected: dirA/{foo, bar, baz_2}, with conflicts on dirA/bar vs. dirB/bar\n+\n+test_setup_12k () {\n+\ttest_create_repo 12k &&\n+\t(\n+\t\tcd 12k &&\n+\n+\t\tmkdir dirA dirB &&\n+\t\techo foo >dirB/foo &&\n+\t\techo bar >dirA/bar &&\n+\t\techo baz >dirA/baz &&\n+\t\tgit add . &&\n+\t\tgit commit -m orig &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit switch A &&\n+\t\tgit mv dirB/* dirA/ &&\n+\t\tgit commit -m A &&\n+\n+\t\tgit switch B &&\n+\t\tgit mv dirA/bar dirB/bar &&\n+\t\techo more baz >>dirA/baz &&\n+\t\tgit commit -m B\n+\t)\n+}\n+\n+test_expect_merge_algorithm failure failure '12k: Directory rename with sibling causes rename-to-self' '\n+\ttest_setup_12k &&\n+\t(\n+\t\tcd 12k &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&\n+\n+\t\ttest_path_is_missing dirB &&\n+\t\ttest_path_is_file dirA/bar &&\n+\t\ttest_path_is_file dirA/baz &&\n+\n+\t\tgit ls-files | uniq >tracked &&\n+\t\ttest_line_count = 3 tracked &&\n+\n+\t\tgit status --porcelain -uno >actual &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\tUU dirA/bar\n+\t\t M dirA/baz\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n ###########################################################################\n # SECTION 13: Checking informational and conflict messages\n #\n-- \ngitgitgadget\n\n"},{"id":"428839","messageId":"6e7ef18bf53a579e360e4970a6fbcc0a68a821f6.1625074200.git.gitgitgadget@gmail.com","threadId":"56010","inReplyTo":"pull.1039.v2.git.git.1625074200.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] merge-ort: ensure we consult df_conflict and path_conflicts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-30T17:29:59Z","receivedAt":"2021-06-30T17:30:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nPath conflicts (typically rename path conflicts, e.g.\nrename/rename(1to2) or rename/add/delete), and directory/file conflicts\nshould obviously result in files not being marked as clean in the merge.\nWe had a codepath where we missed consulting the path_conflict and\ndf_conflict flags, based on match_mask. Granted, it requires an unusual\nsetup to trigger this codepath (directory rename causing rename-to-self\nis the only case I can think of), but we still need to handle it. To\nmake it clear that we have audited the other codepaths that do not\nexplicitly mention these flags, add some assertions that the flags are\nnot set.\n\nReported-by: Anders Kaseorg \nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 6 +++++-\n t/t6423-merge-rename-directories.sh | 6 +++---\n 2 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex b954f7184a5..373dbac5079 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3237,7 +3237,7 @@ static void process_entry(struct merge_options *opt,\n \t * above.\n \t */\n \tif (ci->match_mask) {\n-\t\tci->merged.clean = 1;\n+\t\tci->merged.clean = !ci->df_conflict && !ci->path_conflict;\n \t\tif (ci->match_mask == 6) {\n \t\t\t/* stages[1] == stages[2] */\n \t\t\tci->merged.result.mode = ci->stages[1].mode;\n@@ -3249,6 +3249,8 @@ static void process_entry(struct merge_options *opt,\n \n \t\t\tci->merged.result.mode = ci->stages[side].mode;\n \t\t\tci->merged.is_null = !ci->merged.result.mode;\n+\t\t\tif (ci->merged.is_null)\n+\t\t\t\tci->merged.clean = 1;\n \t\t\toidcpy(&ci->merged.result.oid, &ci->stages[side].oid);\n \n \t\t\tassert(othermask == 2 || othermask == 4);\n@@ -3421,6 +3423,7 @@ static void process_entry(struct merge_options *opt,\n \t\t\t\t path)) {\n \t\t\tci->merged.is_null = 1;\n \t\t\tci->merged.clean = 1;\n+\t\t\tassert(!ci->df_conflict && !ci->path_conflict);\n \t\t} else if (ci->path_conflict &&\n \t\t\t oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {\n \t\t\t/*\n@@ -3447,6 +3450,7 @@ static void process_entry(struct merge_options *opt,\n \t\tci->merged.is_null = 1;\n \t\tci->merged.result.mode = 0;\n \t\toidcpy(&ci->merged.result.oid, null_oid());\n+\t\tassert(!ci->df_conflict);\n \t\tci->merged.clean = !ci->path_conflict;\n \t}\n \ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 89fe6778b94..c3968dc3642 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5058,7 +5058,7 @@ test_setup_12i () {\n \t)\n }\n \n-test_expect_merge_algorithm failure failure '12i: Directory rename causes rename-to-self' '\n+test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' '\n \ttest_setup_12i &&\n \t(\n \t\tcd 12i &&\n@@ -5116,7 +5116,7 @@ test_setup_12j () {\n \t)\n }\n \n-test_expect_merge_algorithm failure failure '12j: Directory rename to root causes rename-to-self' '\n+test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' '\n \ttest_setup_12j &&\n \t(\n \t\tcd 12j &&\n@@ -5174,7 +5174,7 @@ test_setup_12k () {\n \t)\n }\n \n-test_expect_merge_algorithm failure failure '12k: Directory rename with sibling causes rename-to-self' '\n+test_expect_merge_algorithm failure success '12k: Directory rename with sibling causes rename-to-self' '\n \ttest_setup_12k &&\n \t(\n \t\tcd 12k &&\n-- \ngitgitgadget\n\n"},{"id":"428840","messageId":"7c96d6c7a0a4fd25794856e556e071f048771fd1.1625074200.git.gitgitgadget@gmail.com","threadId":"56010","inReplyTo":"pull.1039.v2.git.git.1625074200.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] merge-recursive: handle rename-to-self case","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-30T17:30:00Z","receivedAt":"2021-06-30T17:30:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nDirectory rename detection can cause transitive renames, e.g. if the two\ndifferent sides of history each do one half of:\n A/file -> B/file\n B/ -> C/\nthen directory rename detection transitively renames to give us\n A/file -> C/file\n\nHowever, when C/ == A/, note that this gives us\n A/file -> A/file.\n\nmerge-recursive assumed that any rename D -> E would have D != E. While\nthat is almost always true, the above is a special case where it is not.\nSo we cannot do things like delete the rename source, we cannot assume\nthat a file existing at path E implies a rename/add conflict and we have\nto be careful about what stages end up in the output.\n\nThis change feels a bit hackish. It took me surprisingly many hours to\nfind, and given merge-recursive's design causing it to attempt to\nenumerate all combinations of edge and corner cases with special code\nfor each combination, I'm worried there are other similar fixes needed\nelsewhere if we can just come up with the right special testcase.\nPerhaps an audit would rule it out, but I have not the energy.\nmerge-recursive deserves to die, and since it is on its way out anyway,\nfixing this particular bug narrowly will have to be good enough.\n\nReported-by: Anders Kaseorg \nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 19 +++++++++++++------\n t/t6423-merge-rename-directories.sh | 6 +++---\n 2 files changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d146bb116f7..c895145a8f5 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2804,12 +2804,19 @@ static int process_renames(struct merge_options *opt,\n \t\t\tint renamed_stage = a_renames == renames1 ? 2 : 3;\n \t\t\tint other_stage = a_renames == renames1 ? 3 : 2;\n \n+\t\t\t/*\n+\t\t\t * Directory renames have a funny corner case...\n+\t\t\t */\n+\t\t\tint renamed_to_self = !strcmp(ren1_src, ren1_dst);\n+\n \t\t\t/* BUG: We should only remove ren1_src in the base\n \t\t\t * stage and in other_stage (think of rename +\n \t\t\t * add-source case).\n \t\t\t */\n-\t\t\tremove_file(opt, 1, ren1_src,\n-\t\t\t\t renamed_stage == 2 || !was_tracked(opt, ren1_src));\n+\t\t\tif (!renamed_to_self)\n+\t\t\t\tremove_file(opt, 1, ren1_src,\n+\t\t\t\t\t renamed_stage == 2 ||\n+\t\t\t\t\t !was_tracked(opt, ren1_src));\n \n \t\t\toidcpy(&src_other.oid,\n \t\t\t &ren1->src_entry->stages[other_stage].oid);\n@@ -2823,6 +2830,9 @@ static int process_renames(struct merge_options *opt,\n \t\t\t ren1->dir_rename_original_type == 'A') {\n \t\t\t\tsetup_rename_conflict_info(RENAME_VIA_DIR,\n \t\t\t\t\t\t\t opt, ren1, NULL);\n+\t\t\t} else if (renamed_to_self) {\n+\t\t\t\tsetup_rename_conflict_info(RENAME_NORMAL,\n+\t\t\t\t\t\t\t opt, ren1, NULL);\n \t\t\t} else if (oideq(&src_other.oid, null_oid())) {\n \t\t\t\tsetup_rename_conflict_info(RENAME_DELETE,\n \t\t\t\t\t\t\t opt, ren1, NULL);\n@@ -3180,7 +3190,6 @@ static int handle_rename_normal(struct merge_options *opt,\n \tstruct rename *ren = ci->ren1;\n \tstruct merge_file_info mfi;\n \tint clean;\n-\tint side = (ren->branch == opt->branch1 ? 2 : 3);\n \n \t/* Merge the content and write it out */\n \tclean = handle_content_merge(&mfi, opt, path, was_dirty(opt, path),\n@@ -3190,9 +3199,7 @@ static int handle_rename_normal(struct merge_options *opt,\n \t opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT &&\n \t ren->dir_rename_original_dest) {\n \t\tif (update_stages(opt, path,\n-\t\t\t\t NULL,\n-\t\t\t\t side == 2 ? &mfi.blob : NULL,\n-\t\t\t\t side == 2 ? NULL : &mfi.blob))\n+\t\t\t\t &mfi.blob, &mfi.blob, &mfi.blob))\n \t\t\treturn -1;\n \t\tclean = 0; /* not clean, but conflicted */\n \t}\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex c3968dc3642..01d27b66399 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5058,7 +5058,7 @@ test_setup_12i () {\n \t)\n }\n \n-test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' '\n+test_expect_success '12i: Directory rename causes rename-to-self' '\n \ttest_setup_12i &&\n \t(\n \t\tcd 12i &&\n@@ -5116,7 +5116,7 @@ test_setup_12j () {\n \t)\n }\n \n-test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' '\n+test_expect_success '12j: Directory rename to root causes rename-to-self' '\n \ttest_setup_12j &&\n \t(\n \t\tcd 12j &&\n@@ -5174,7 +5174,7 @@ test_setup_12k () {\n \t)\n }\n \n-test_expect_merge_algorithm failure success '12k: Directory rename with sibling causes rename-to-self' '\n+test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\n \ttest_setup_12k &&\n \t(\n \t\tcd 12k &&\n-- \ngitgitgadget\n"}]}