{"thread":{"id":"63821","subject":"[PATCH 0/6] Fix various rename corner cases","startedAt":"2025-07-22T15:23:14Z","lastAt":"2025-08-06T23:15:32Z","messageCount":39,"participants":["Elijah Newren via GitGitGadget","Patrick Steinhardt","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"522449","messageId":"pull.1943.git.1753197791.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":null,"subject":"[PATCH 0/6] Fix various rename corner cases","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-22T15:23:05Z","receivedAt":"2025-07-22T15:23:14Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"At GitHub, we've got a real-world repository that has been triggering\nfailures of the form:\n\ngit: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.\n\n\nDigging in, this particular corner case requires multiple things to trigger:\n(1) a rename/delete of one file, and (2) a directory rename modifying an\nunrelated rename such that this unrelated rename's target becomes the source\nof the rename/delete from (1).\n\nUnfortunately, looking around, it's not the only bug in the area. Plus, some\nof our testcases in tangential situations were not checked closely enough or\nwere weird or buggy in various ways. Adding to the challenge was the fact\nthat the relevant renames optimization was sometimes triggering making\nrenames look like delete-and-add, and overlooking this meant I sometimes\nwasn't triggering what I thought I was triggering.\n\nThe combination of challenges sometimes made me think my fixes were breaking\nthings when sometimes I was just unaware of other bugs. I went in circles a\nfew times and took a rather non-linear path to finding and fixing these\nissues. While I think I've turned it into a nice linear progression of\npatches, I might be a bit too deep in the mud and it might not be as linear\nor clear as I think. Let me know and I'll try to clarify anything needed.\n\nElijah Newren (6):\n merge-ort: update comments to modern testfile location\n merge-ort: drop unnecessary temporary in check_for_directory_rename()\n t6423: document two bugs with rename-to-self testcases\n t6423: fix missed staging of file in testcases 12i,12j,12k\n merge-ort: fix incorrect file handling\n merge-ort: fix directory rename on top of source of other\n rename/delete\n\n merge-ort.c | 41 ++-\n t/t6423-merge-rename-directories.sh | 520 +++++++++++++++++++++++++++-\n 2 files changed, 534 insertions(+), 27 deletions(-)\n\n\nbase-commit: 16bd9f20a403117f2e0d9bcda6c6e621d3763e77\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1943%2Fnewren%2Ffix-rename-corner-cases-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1943/newren/fix-rename-corner-cases-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1943\n-- \ngitgitgadget\n"},{"id":"522450","messageId":"dccc204430535f8c9eb74d7861365dbb2c42bb02.1753197791.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.git.1753197791.gitgitgadget@gmail.com","subject":"[PATCH 1/6] merge-ort: update comments to modern testfile location","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-22T15:23:06Z","receivedAt":"2025-07-22T15:23:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn commit 919df3195553 (Collect merge-related tests to t64xx,\n2020-08-10), merge related tests were moved from t60xx to t64xx. Some\ncomments in merge-ort relating to some tricky code referenced specific\ntestcases within certain testfiles for additional information, but\nreferred to their historical testfile names; update the testfile names\nto mention their modern location.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 47b3d1730ece..d87ba6dd42bf 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2163,7 +2163,7 @@ static int handle_content_merge(struct merge_options *opt,\n \t\t/*\n \t\t * FIXME: If opt->priv->call_depth && !clean, then we really\n \t\t * should not make result->mode match either a->mode or\n-\t\t * b->mode; that causes t6036 \"check conflicting mode for\n+\t\t * b->mode; that causes t6416 \"check conflicting mode for\n \t\t * regular file\" to fail. It would be best to use some other\n \t\t * mode, but we'll confuse all kinds of stuff if we use one\n \t\t * where S_ISREG(result->mode) isn't true, and if we use\n@@ -2520,7 +2520,7 @@ static void compute_collisions(struct strmap *collisions,\n \t * happening, and fall back to no-directory-rename detection\n \t * behavior for those paths.\n \t *\n-\t * See testcases 9e and all of section 5 from t6043 for examples.\n+\t * See testcases 9e and all of section 5 from t6423 for examples.\n \t */\n \tfor (i = 0; i < pairs->nr; ++i) {\n \t\tstruct strmap_entry *rename_info;\n@@ -2618,7 +2618,7 @@ static char *check_for_directory_rename(struct merge_options *opt,\n \t * That's why otherinfo and dir_rename_exclusions is here.\n \t *\n \t * As it turns out, this also prevents N-way transient rename\n-\t * confusion; See testcases 9c and 9d of t6043.\n+\t * confusion; See testcases 9c and 9d of t6423.\n \t */\n \tnew_dir = rename_info->value; /* old_dir = rename_info->key; */\n \totherinfo = strmap_get_entry(dir_rename_exclusions, new_dir);\n-- \ngitgitgadget\n\n"},{"id":"522451","messageId":"58df0710efc042b014a0c8282ce1d7fa62fbb760.1753197791.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.git.1753197791.gitgitgadget@gmail.com","subject":"[PATCH 2/6] merge-ort: drop unnecessary temporary in check_for_directory_rename()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-22T15:23:07Z","receivedAt":"2025-07-22T15:23:16Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\ncheck_for_directory_rename() had a weirdly coded check for whether a\nstrmap contained a certain key. Replace the temporary variable and call\nto strmap_get_entry() with the more natural strmap_contains() call.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex d87ba6dd42bf..9b9d82ed10f7 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2580,7 +2580,6 @@ static char *check_for_directory_rename(struct merge_options *opt,\n {\n \tchar *new_path;\n \tstruct strmap_entry *rename_info;\n-\tstruct strmap_entry *otherinfo;\n \tconst char *new_dir;\n \tint other_side = 3 - side_index;\n \n@@ -2615,14 +2614,13 @@ static char *check_for_directory_rename(struct merge_options *opt,\n \t * to not let Side1 do the rename to dumbdir, since we know that is\n \t * the source of one of our directory renames.\n \t *\n-\t * That's why otherinfo and dir_rename_exclusions is here.\n+\t * That's why dir_rename_exclusions is here.\n \t *\n \t * As it turns out, this also prevents N-way transient rename\n \t * confusion; See testcases 9c and 9d of t6423.\n \t */\n \tnew_dir = rename_info->value; /* old_dir = rename_info->key; */\n-\totherinfo = strmap_get_entry(dir_rename_exclusions, new_dir);\n-\tif (otherinfo) {\n+\tif (strmap_contains(dir_rename_exclusions, new_dir)) {\n \t\tpath_msg(opt, INFO_DIR_RENAME_SKIPPED_DUE_TO_RERENAME, 1,\n \t\t\t rename_info->key, path, new_dir, NULL,\n \t\t\t _(\"WARNING: Avoiding applying %s -> %s rename \"\n-- \ngitgitgadget\n\n"},{"id":"522452","messageId":"bda42aa85cf4f332ef60aca1a8937ed4b868fa87.1753197791.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.git.1753197791.gitgitgadget@gmail.com","subject":"[PATCH 3/6] t6423: document two bugs with rename-to-self testcases","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-22T15:23:08Z","receivedAt":"2025-07-22T15:23:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen commit 98a1a00d5301 (t6423: add a testcase causing a failed\nassertion in process_renames, 2025-03-06) was added, I tweaked\nthe commit message, and moved the test into t6423. However, that\nstill left two other things missing that made this test unlike the\nothers in the same testfile:\n\n * It didn't have an English description of the test setup like\n all other tests in t6423\n\n * It didn't check that the right number of files were present at\n the end\n\nThe former issue is a minor detail that isn't that critical, but the\nlatter feels more important. If it had been done, I might have noticed\nanother bug. In particular, this testcase involves\n Side A: rename world -> tools/world\nand\n Side B: rename tools/ -> \n Side B: remove world\nThe tools/ -> rename turns the world -> tools/world rename\ninto world -> world, i.e. a rename-to-self case. But, it's a path\nconflict because merge.directoryRenames defaults to false. There's\nno content conflict because Side A didn't modify world, so we should\njust take the content of world from Side B -- i.e. delete it. So, we\nhave a conflict on the path, but not on its content. We could consider\nletting the content trump since it is unconflicted, but if we are going\nto leave a conflict, it should certainly represent that 'world' existed\nboth in the base version and on Side A. Currently it doesn't.\n\nAdd a description of this test, add some checking of the number of\nentries in the index at the end of the merge, and mark the test as\nexpecting to fail for now. A subsequent commit will fix this bug.\n\nWhile at it, I found another related bug from a nearly identical setup\nbut setting merge.directoryRenames=true. Copy testcase 12n into 12n2,\nchanging it to use merge instead of cherry-pick, and turn on directory\nrenames for this test. In this case, since there is no content conflict\nand no path conflict, it should be okay to delete the file.\nUnfortunately, the code resolves without conflict but silently leaves\nworld despite the fact it should be deleted. It might also be okay if\nthe code spuriously thought there was a modify/delete conflict here;\nthat would at least notify users to look closer and then when they\nnotice there was no change since the base version, they can easily\nresolve. A conflict notice is much better than silently providing the\nwrong resolution. Cover this with the 12n2 testcase, which for now is\nmarked as expecting to fail as well.\n\nSigned-off-by: Elijah Newren \n---\n t/t6423-merge-rename-directories.sh | 101 +++++++++++++++++++++++++++-\n 1 file changed, 99 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex f48ed6d03534..69de7a3b84af 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5056,6 +5056,25 @@ test_expect_success '12m: Change parent of renamed-dir to symlink on other side'\n \t)\n '\n \n+# Testcase 12n, Directory rename transitively makes rename back to self\n+#\n+# (Since this is a cherry-pick instead of merge, the labels are a bit weird.\n+# O, the original commit, is A~1 rather than what branch O points to.)\n+#\n+# Commit O: tools/hello\n+# world\n+# Commit A: tools/hello\n+# tools/world\n+# Commit B: hello\n+# In words:\n+# A: world -> tools/world\n+# B: tools/ -> /, i.e. rename all of tools to toplevel directory\n+# delete world\n+#\n+# Expected:\n+# CONFLICT (file location): tools/world vs. world\n+#\n+\n test_setup_12n () {\n \tgit init 12n &&\n \t(\n@@ -5084,7 +5103,7 @@ test_setup_12n () {\n \t)\n }\n \n-test_expect_success '12n: Directory rename transitively makes rename back to self' '\n+test_expect_failure '12n: Directory rename transitively makes rename back to self' '\n \ttest_setup_12n &&\n \t(\n \t\tcd 12n &&\n@@ -5092,7 +5111,85 @@ test_expect_success '12n: Directory rename transitively makes rename back to sel\n \t\tgit checkout -q B^0 &&\n \n \t\ttest_must_fail git cherry-pick A^0 >out &&\n-\t\tgrep \"CONFLICT (file location).*should perhaps be moved\" out\n+\t\tgrep \"CONFLICT (file location).*should perhaps be moved\" out &&\n+\n+\t\t# Should have 1 entry for hello, and 1 for world\n+\t\ttest_stdout_line_count = 2 git ls-files -s &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s hello &&\n+\t\ttest_stdout_line_count = 2 git ls-files -s world\n+\t)\n+'\n+\n+# Testcase 12n2, Directory rename transitively makes rename back to self\n+#\n+# Commit O: tools/hello\n+# world\n+# Commit A: tools/hello\n+# tools/world\n+# Commit B: hello\n+# In words:\n+# A: world -> tools/world\n+# B: tools/ -> /, i.e. rename all of tools to toplevel directory\n+# delete world\n+#\n+# Expected:\n+# CONFLICT (file location): tools/world vs. world\n+#\n+\n+test_setup_12n2 () {\n+\tgit init 12n2 &&\n+\t(\n+\t\tcd 12n2 &&\n+\n+\t\tmkdir tools &&\n+\t\techo hello >tools/hello &&\n+\t\tgit add tools/hello &&\n+\t\techo world >world &&\n+\t\tgit add world &&\n+\t\tgit commit -m \"O\" &&\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 world tools/world &&\n+\t\tgit commit -m \"Move world into tools/\" &&\n+\n+\t\tgit switch B &&\n+\t\tgit mv tools/hello hello &&\n+\t\tgit rm world &&\n+\t\tgit commit -m \"Move hello from tools/ to toplevel\"\n+\t)\n+}\n+\n+test_expect_failure '12n2: Directory rename transitively makes rename back to self' '\n+\ttest_setup_12n2 &&\n+\t(\n+\t\tcd 12n2 &&\n+\n+\t\tgit checkout -q B^0 &&\n+\n+\t\t# NOTE: Since merge.directoryRenames=true, there is no path\n+\t\t# conflict for world vs. tools/world; it should end up at\n+\t\t# world. The fact that world was unmodified on side A, means\n+\t\t# there was no content conflict; we should just take the\n+\t\t# content from side B -- i.e. delete the file. So merging\n+\t\t# could just delete world.\n+\t\t#\n+\t\t# However, rename-to-self-via-directory-rename is a bit more\n+\t\t# challenging. Relax this test to allow world to be treated\n+\t\t# as a modify/delete conflict as well.\n+\n+\t\ttest_might_fail git -c merge.directoryRenames=true merge A^0 >out &&\n+\n+\t\t# Should have 1 entry for hello, and either 0 or 2 for world\n+\t\ttest_stdout_line_count = 1 git ls-files -s hello &&\n+\t\ttest_stdout_line_count != 1 git ls-files -s world &&\n+\t\tif test_stdout_line_count != 0 git ls-files -s world\n+\t\tthen\n+\t\t\tgrep \"CONFLICT (modify/delete).*world deleted in HEAD\" out\n+\t\tfi\n \t)\n '\n \n-- \ngitgitgadget\n\n"},{"id":"522453","messageId":"3b3b258cec5f0080beb64501f7510f7acbc3a91b.1753197791.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.git.1753197791.gitgitgadget@gmail.com","subject":"[PATCH 4/6] t6423: fix missed staging of file in testcases 12i,12j,12k","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-22T15:23:09Z","receivedAt":"2025-07-22T15:23:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nCommit 806f83287f8d (t6423: test directory renames causing\nrename-to-self, 2021-06-30) introduced testcase 12i-12k but omitted\nstaging one of the files and copy-pasted that mistake to the other\ntests. This means the merge runs with an unstaged change, even though\nthat isn't related to what is being tested and makes the test look more\ncomplicated than it is.\n\nThe cover letter for the series associated with the above commit noted\nthat these testcases triggered two bugs in merge-recursive but only one\nin merge-ort; in merge-recursive these testcases also triggered a\nsilent deletion of the file in question when it shouldn't be deleted.\nWhat I didn't realize at the time was that the deletion bug in merge-ort\nwas merely being sidestepped by the \"relevant renames\" optimization but\ncan actually be triggered. A subsequent commit will deal with that\nadditional bug, but it was complicated by the mistaken forgotten\nstaging, so this commit first fixes that issue.\n\nSigned-off-by: Elijah Newren \n---\n t/t6423-merge-rename-directories.sh | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 69de7a3b84af..c2032eb6cfa1 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -4747,6 +4747,7 @@ test_setup_12i () {\n \t\tgit switch B &&\n \t\tgit mv source/bar source/subdir/bar &&\n \t\techo more baz >>source/baz &&\n+\t\tgit add source/baz &&\n \t\tgit commit -m B\n \t)\n }\n@@ -4771,7 +4772,7 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\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\tM source/baz\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -4806,6 +4807,7 @@ test_setup_12j () {\n \t\tgit switch B &&\n \t\tgit mv bar subdir/bar &&\n \t\techo more baz >>baz &&\n+\t\tgit add baz &&\n \t\tgit commit -m B\n \t)\n }\n@@ -4830,7 +4832,7 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '\n \t\tgit status --porcelain -uno >actual &&\n \t\tcat >expect <<-\\EOF &&\n \t\tUU bar\n-\t\t M baz\n+\t\tM baz\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -4865,6 +4867,7 @@ test_setup_12k () {\n \t\tgit switch B &&\n \t\tgit mv dirA/bar dirB/bar &&\n \t\techo more baz >>dirA/baz &&\n+\t\tgit add dirA/baz &&\n \t\tgit commit -m B\n \t)\n }\n@@ -4889,7 +4892,7 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\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\tM dirA/baz\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -5114,7 +5117,7 @@ test_expect_failure '12n: Directory rename transitively makes rename back to sel\n \t\tgrep \"CONFLICT (file location).*should perhaps be moved\" out &&\n \n \t\t# Should have 1 entry for hello, and 1 for world\n-\t\ttest_stdout_line_count = 2 git ls-files -s &&\n+\t\ttest_stdout_line_count = 3 git ls-files -s &&\n \t\ttest_stdout_line_count = 1 git ls-files -s hello &&\n \t\ttest_stdout_line_count = 2 git ls-files -s world\n \t)\n-- \ngitgitgadget\n\n"},{"id":"522454","messageId":"2c7d4e022c59609bf263a7045fceb1854441bb29.1753197791.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.git.1753197791.gitgitgadget@gmail.com","subject":"[PATCH 5/6] merge-ort: fix incorrect file handling","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-22T15:23:10Z","receivedAt":"2025-07-22T15:23:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWe have multiple bugs here -- accidental silent file deletion,\naccidental silent file retention for files that should be deleted,\nand incorrect number of entries left in the index.\n\nThe series merged at commit d3b88be1b450 (Merge branch\n'en/merge-dir-rename-corner-case-fix', 2021-07-16) introduced testcase\n12i-12k in t6423 which checked for rename-to-self cases, and fixed bugs\nthat merge-ort and merge-recursive had with these testcases. At the\ntime, I noted that merge-ort had one bug for these cases, while\nmerge-recursive had two. It turns out that merge-ort did in fact have\nanother bug, but the \"relevant renames\" optimizations were masking it.\nIf we modify testcase 12i from t6423 to modify the file in the commit\nthat renames it (but only modify it enough that it can still be detected\nas a rename), then we can trigger silent deletion of the file.\n\nTweak testcase 12i slightly to make the file in question have more than\none line in it, but which doesn't change how it operates. Make this\nchange to otherwise minimize the changes between this testcase and a new\none that we want to add. Then duplicate the testcase as 12i2, changing\nit so that it adds a single line to the file in question when it is\nrenamed, as a testcase for this bug.\n\nFurther, commit 98a1a00d5301 (t6423: add a testcase causing a failed\nassertion in process_renames, 2025-03-06), fixed an issue with\nrename-to-self but added a new testcase, 12n, that only checked for\nwhether the merge ran to completion. A few commits ago, we modified\nthis test to check for the number of entries in the index -- but noted\nthat the number was wrong. And we also noted a\nsilently-keep-instead-of-delete bug at the same time in the new testcase\n12n2.\n\nFix to merge-ort to prevent multiple bugs with rename-to-self cases:\n * silent deletion of file expected to be kept (t6423 testcase 12i2)\n * silent retention of file expected to be removed (t6423 testcase 12n2)\n * wrong number of extries left in the index (t6423 testcase 12n)\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 11 +++++\n t/t6423-merge-rename-directories.sh | 69 +++++++++++++++++++++++++++--\n 2 files changed, 77 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 9b9d82ed10f7..feb06720c7e1 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2873,6 +2873,17 @@ static int process_renames(struct merge_options *opt,\n \t\t\tnewinfo = new_ent->value;\n \t\t}\n \n+\t\t/*\n+\t\t * Directory renames can result in rename-to-self, which we\n+\t\t * want to skip so we don't mark oldpath for deletion.\n+\t\t *\n+\t\t * Note that we can avoid strcmp here because of prior\n+\t\t * diligence in apply_directory_rename_modifications() to\n+\t\t * ensure we reused existing paths from opt->priv->paths.\n+\t\t */\n+\t\tif (oldpath == newpath)\n+\t\t\tcontinue;\n+\n \t\t/*\n \t\t * If pair->one->path isn't in opt->priv->paths, that means\n \t\t * that either directory rename detection removed that\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex c2032eb6cfa1..6611331769e9 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -4731,7 +4731,7 @@ test_setup_12i () {\n \n \t\tmkdir -p source/subdir &&\n \t\techo foo >source/subdir/foo &&\n-\t\techo bar >source/bar &&\n+\t\tprintf \"%d\\n\" 1 2 3 4 5 6 7 >source/bar &&\n \t\techo baz >source/baz &&\n \t\tgit add source &&\n \t\tgit commit -m orig &&\n@@ -4778,6 +4778,69 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\n \t)\n '\n \n+# Testcase 12i2, Identical to 12i except that source/subdir/bar modified on unrenamed side\n+# Commit O: source/{subdir/foo, bar, baz_1}\n+# Commit A: source/{foo, bar_2, 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_12i2 () {\n+\tgit init 12i2 &&\n+\t(\n+\t\tcd 12i2 &&\n+\n+\t\tmkdir -p source/subdir &&\n+\t\techo foo >source/subdir/foo &&\n+\t\tprintf \"%d\\n\" 1 2 3 4 5 6 7 >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\techo 8 >> source/bar &&\n+\t\tgit add source/bar &&\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 add source/baz &&\n+\t\tgit commit -m B\n+\t)\n+}\n+\n+test_expect_success '12i2: Directory rename causes rename-to-self' '\n+\ttest_setup_12i2 &&\n+\t(\n+\t\tcd 12i2 &&\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 >actual &&\n+\t\tuniq 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\tM 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@@ -5106,7 +5169,7 @@ test_setup_12n () {\n \t)\n }\n \n-test_expect_failure '12n: Directory rename transitively makes rename back to self' '\n+test_expect_success '12n: Directory rename transitively makes rename back to self' '\n \ttest_setup_12n &&\n \t(\n \t\tcd 12n &&\n@@ -5166,7 +5229,7 @@ test_setup_12n2 () {\n \t)\n }\n \n-test_expect_failure '12n2: Directory rename transitively makes rename back to self' '\n+test_expect_success '12n2: Directory rename transitively makes rename back to self' '\n \ttest_setup_12n2 &&\n \t(\n \t\tcd 12n2 &&\n-- \ngitgitgadget\n\n"},{"id":"522455","messageId":"0813d42d91fe9e27f713de3c7d2377c28311ae57.1753197791.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.git.1753197791.gitgitgadget@gmail.com","subject":"[PATCH 6/6] merge-ort: fix directory rename on top of source of other rename/delete","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-22T15:23:11Z","receivedAt":"2025-07-22T15:23:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nAt GitHub, we've got a real-world repository that has been triggering\nfailures of the form:\n\n git: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.\n\nwhich comes from the line:\n\n VERIFY_CI(newinfo);\n\nUnfortunately, this one has been quite complex to unravel, and is a\nbit complex to explain. So, I'm going to carefully try to explain each\nrelevant piece needed to understand the fix, then carefully build up\nfrom a simple testcase to some of the relevant testcases.\n\n== New special case we need to consider ==\n\nRename pairs in the diffcore machinery connect the source path of a\nrename with the destination path of a rename. Since we have rename\npairs to consider on both sides of history since the merge base,\nmerging has to consider a few special cases of possible overlap:\n\n A) two rename pairs having the same target path\n B) two rename pairs having the same source path\n C) the source path of one rename pair being the target path of a\n different rename pair\n\nSome of these came up often enough that we gave them names:\n A) a rename/rename(2to1) conflict (looks similar to an add/add conflict)\n B) a rename/rename(1to2) conflict, which represents the same path being\n renamed differently on the two sides of history\n C) not yet named\n\nmerge-ort is well-prepared to handle cases (A) and (B), as was\nmerge-recursive (which was merge-ort's predecessor). Case (C) was\nbriefly considered during the years of merge-recursive maintenance,\nbut the full extent of support it got was a few FIXME/TODO comments\nlittered around the code highlighting some of the places that would\nprobably need to be fixed to support it. When I wrote merge-ort I\nignored case (C) entirely, since I believed that case (C) was only\npossible if we were to support break detection during merges. Not\nonly had break detection never been supported by any merge algorithm,\nI thought break detection wasn't worth the effort to support in a\nmerge algorithm. However, it turns out that case (C) can be triggered\nwithout break detection, if there's enough moving pieces.\n\nBefore I dive into how to trigger case (C) with directory renames plus\nother renames, it might be helpful to use a simpler example with break\ndetection first. And before we get to that it may help to explain\nsome more basics of handling renames in the merge algorithm. So, let\nme first backup and provide a quick refresher on on each of\n\n * handling renames\n * what break detection would mean, if supported in merging\n * handling directory renames\n\nFrom there, I'll build up from a basic directory rename detection case\nto one that triggers a failure currently.\n\n== Handling renames ==\n\nIn the merge machinery when we have a rename of a path A -> B,\nprocessing that rename needs to remove path A, and make sure that path B\nhas the relevant information. Note that if the content was also\nmodified on both sides, this may mean that we have 3 different stages\nthat need to be stored at path B instead of having some stored at path\nA.\n\nHaving all stages stored at path B makes it much easier for users to\ninvestigate and resolve the content conflict associated with a renamed\npath. For example:\n * \"git status\" doesn't have to figure out how to list paths A & B and\n attempt to connect them for users; it can just list path B.\n * Users can use \"git ls-files -u B\" (instead of trying to find the\n previous name of the file so they can list both, i.e. \"git ls-files\n -u A B\")\n * Users can resolve via \"git add B\" (without needing to \"git rm A\")\n\n== What break detection would mean ==\n\nIf break detection were supported, we might have cases where A -> B\n*and* C -> A, meaning that both rename pairs might believe they need to\nupdate A. In particular, the processing of A -> B would need to be\ncareful to not clear out all stages of A and mark it resolved, while\nboth renames would need to figure out which stages of A belong with A\nand which belong with B, so that both paths have the right stages\nassociated with them.\n\nmerge-ort (like merge-recursive before it) makes no attempt to handle\nbreak detection; it runs with break detection turned off. It would\nneed to be retrofitted to handle such cases.\n\n== Directory rename detection ==\n\nIf one side of history renames directory D/ -> E/, and the other side of\nhistory adds new files to E/, then directory rename detection notices\nand suggests moving those new files to E/. A similar thing is done for\npaths renamed into D/, causing them to be transitively renamed into E/.\n\nThe default in the merge machinery is to report a conflict whenever a\ndirectory rename might modify the location of a path, so that users can\ndecide whether they wanted the original path or the\ndirectory-rename-induced location. However, that means the default\ncodepath still runs through all the directory rename detection logic, it\njust supplements it with providing conflict notices when it is done.\n\n== Building up increasingly complex testcases ==\n\nI'll start with a really simple directory rename example, and then\nslowly add twists that explain new pieces until we get to the\nproblematic cases:\n\n=== Testcase 1 ===\n\nLet's start with a concrete example, where particular files/directories of\ninterest that exist or are changed on each side are called out:\n\n Original: \n our side: rename B/file -> C/file\n their side: rename C/ -> A/\n\nFor this case, we'd expect to see the original B/file appear not at\nC/file but at A/file.\n\n(We would also expect a conflict notice that the user will want to\nchoose between C/file and A/file, but I'm going to ignore conflict\nnotices from here on by assuming merge.directoryRenames is set to\n`true` rather than `conflict`; the only difference that assumption\nmakes is whether that makes the merge be considered to be conflicted\nand whether it prints a conflict notice; what is written to the index\nor working directory is unchanged.)\n\n=== Testcase 2 ===\n\nModify testcase 1 by having A/file exist from the start:\n\n Original: A/file exists\n our side: rename B/file -> C/file\n their side: rename C/ -> A/\n\nIn such a case, to avoid user confusion at what looks kind of like an\nadd/add conflict (even though the original path at A/file was not added\nby either side of the merge), we turn off directory rename detection for\nthis path and print a \"in the way\" warning to the user:\n CONFLICT (implicit dir rename): Existing file/dir ... in the way ...\nThe testcases in section 5 of t6423 explore these in more detail.\n\n=== Testcase 3 ===\n\nLet's modify testcase 1 in a slightly different way: have A/file be\nadded by their side rather than it already existing.\n\n Original: \n our side: rename B/file -> C/file\n their side: rename C/ -> A/\n add A/file\n\nIn this case, the directory rename detection basically transforms our\nside's original B/file -> C/file into a B/file -> A/file, and so we\nget a rename/add conflict, with one version of A/file coming from the\nrenamed file, and another coming from the new A/file, each stored as\nstages 2 and 3 in conflicts. This kind of add/add conflict is perhaps\nslightly more complex than a regular add/add conflict, but with the\nprinted messages it makes sense where it came from and we have\ndifferent stages of the file to work with to resolve the conflict.\n\n=== Testcase 4 ===\n\nLet's do something similar to testcase 3, but have the opposite side of\nhistory add A/file:\n\n Original: \n our side: rename B/file -> C/file\n add A/file\n their side: rename C/ -> A/\n\nNow if we allow directory rename detection to modify C/file to A/file,\nthen we also get a rename/add conflict, but in this case we'd need both\nhigher order stages being recorded on side 2, which makes no sense. The\nindex can't store multiple stage 2 entries, and even if we could, it\nwould probably be confusing for users to work with. So, similar to what\nwe do when there was an A/file in the original version, we simply turn\noff directory rename detection for cases like this and provide the \"in\nthe way\" CONFLICT notice to the user.\n\n=== Testcase 5 ===\n\nWe're slowly getting closer. Let's mix it up by having A/file exist at\nthe beginning but not exist on their side:\n\n original: A/file exists\n our side: rename B/file -> C/file\n their side: rename C/ -> A/\n rename A/file -> D/file\n\nFor this case, you could say that since A/file -> D/file, it's no longer\nin the way of C/file being moved by directory rename detection to\nA/file. But that would give us a case where A/file is both the source\nand the target of a rename, similar to break detection, which the code\nisn't currently equipped to handle.\n\nThis is not yet the case that causes current failures; to the current\ncode, this kind of looks like testcase 4 in that A/file is in the way\non our side (since A/file was in the original and was umodified by our\nside). So, it results in a \"in the way\" notification with directory\nrename detection being turned off for A/file so that B/file ends up at\nC/file.\n\nPerhaps the resolution could be improved in the future, but our \"in\nthe way\" checks prevented such problems by noticing that A/file exists\non our side and thus turns off directory rename detection from\naffecting C/file's location. So, while the merge result could be\nperhaps improved, the fact that this is currently handled by giving\nthe user an \"in the way\" message gives the user a chance to resolve\nand prevents the code from tripping itself up.\n\n=== Testcase 6 ===\n\nLet's modify testcase 5 a bit more, to also delete A/file on our side:\n\n original: A/file exists\n our side: rename B/file -> C/file\n delete A/file\n their side: rename C/ -> A/\n rename A/file -> D/file\n\nNow the \"in the way\" logic doesn't detect that there's an A/file in\nthe way (neither side has an A/file anymore), so it's fine to\ntransitively rename C/file further to A/file...except that we end up\nwith A/file being both the source of one rename, and the target of a\ndifferent rename. Each rename pair tries to handle the resolution of\nthe source and target paths of its own rename. But when we go to\nprocess the second rename pair in process_renames(), we do not expect\neither the source or the destination to be marked as handled already;\nso, when we hit the sanity checks that these are not handled:\n\n VERIFY_CI(oldinfo);\n VERIFY_CI(newinfo);\n\nthen one of these is going to throw an assertion failure since the\nprevious rename pair already marked both of its paths as handled.\nThis will give us an error of the form:\n\n git: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.\n\nThis is the failure we're currently triggering, and it fundamentally\ndepends on:\n * a path existing in the original\n * that original path being removed or renamed on *both* sides\n * some kind of directory rename moving some *other* path into that\n original path\n\nThis was added as testcase 12q in t6423.\n\n=== Testcase 7 ===\n\nBonus bug found while investigating!\n\nLet's go back to the comparison between testcases 2 & 3, and set up a\nfile present on their side that we need to consider:\n\n Original: A/file exists\n our side: rename B/file -> C/file\n rename A/file -> D/file\n their side: rename C/ -> A/\n\nHere, there is no A/file in the way on our side like testcase 4.\nThere is an A/file present on their side like testcase 3, which was an\nadd/add conflict, but that's associated with the file be renamed to\nD/file. So, that really shouldn't be an add/add conflict because we\ninstead want all modes of the original A/file to be transported to\nD/file.\n\nUnfortunately, the current code kind of treats it like an add/add\nconflict instead...but even worse. There is also a valid mode for\nA/file in the original, which normally goes to stage 1. However, an\nadd/add conflict should be represented in the index with no mode at\nstage 1 (for the original side), only modes at stages 2 and 3 (for our\nand their side), so for an add/add we'd expect that mode for A/file in\nthe original version to be cleared out (or be transported to D/file).\n\nUnfortunately, the code currently leaves not only the stage 3 entry\nfor A/file intact, it also leaves the stage 1 entry for A/file. This\nresults in `git ls-files -u A/file` output of the form:\n\n 100644 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 1\tA/file\n 100644 0cfbf08886fca9a91cb753ec8734c84fcbe52c9f 2\tA/file\n 100644 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 3\tA/file\n\nThis would likely cause users to believe this isn't an add/add\nconflict; rather, this would lead them to believe that A/file was only\nmodified on our side and that therefore it should not have been a\nconflict in the first place. And while resolving the conflict in\nfavor of our side is the correct resolution (because stages 1 and 3\nshould have been cleared out in the first place), this is certainly\nlikely to cause confusion for anyone attempting to investigate why\nthis path was marked as conflicted.\n\nThis was added as testcase 12p in t6423.\n\n== Attempted solutions that I discarded ==\n\n1) For each side of history, create a strset of the sources of each\n rename on the other side of history. Then when using directory\n renames to modify existing renames, verify that we aren't renaming\n to a source of another rename.\n\n Unfortunately, the \"relevant renames\" optimization in merge-ort\n means we often don't detect renames -- we just see a delete and an\n add -- which is easy to forget and makes debugging testcases harder,\n but it also turns out that this solution in insufficient to solve\n the related problems in the area (more on that below).\n\n2) Modify the code to be aware of the possibility of renaming to\n the source of another side's rename, and make all the conflict\n resolution logic for each case (including existing\n rename/rename(2to1) and rename/rename(1to2) cases) handle the\n additional complexity. It turns out there was much more code to\n audit than I wanted, for a really niche case. I didn't like how\n many changes were needed, and aborted.\n\n== Solution ==\n\nWe do not want the stages of unrelated files appearing at the same path\nin the index except when dealing with an add/add conflict. While we\npreviously handled this for stages 2 & 3, we also need to worry about\nstage 1. So check for a stage 1 index entry being in the way of a\ndirectory rename.\n\nHowever, if we can detect that the stage 1 index entry is actually from\na related file due to a directory-rename-causes-rename-to-self\nsituation, then we can allow the stage 1 entry to remain.\n\nFrom this wording, you may note that it's not just rename cases that\nare a problem; bugs could be triggered with directory renames vs simple\nadds. That leads us to...\n\n== Testcases 8+ ==\n\nAnother bonus bug, found via understanding our final solution (and the\nfailure of our first attempted solution)!\n\nLet's tweak testcase 7 a bit:\n\n Original: A/file exists\n our side: delete A/file\n add -> C/file\n their side: delete A/file\n rename C/ -> A/\n\nHere, there doesn't seem to be a big problem. Sure C/file gets modified\nvia the directory rename of C/ -> A/ so that it becomes A/file, but\nthere's no file in the way, right? Actually, here we have a problem\nthat the stage 1 entry of A/file would be combined with the stage 2\nentry of C/file, and make it look like a modify/delete conflict.\nPerhaps there is some extra checking that could be added to the code to\nmake it attempt to clear out the stage 1 entry of A/file, but the\nvarious rename-to-self-via-directory-rename testcases make that a bit\nmore difficult. For now, it's easier to just treat this as a\npath-in-the-way situation and not allow the directory rename to modify\nC/file.\n\nThat sounds all well and good, but it does have an interesting side\neffect. Due to the \"relevant renames\" optimizations in merge-ort (i.e.\nonly detect the renames you need), 100% renames whose files weren't\nmodified on the other side often go undetected. This means that if we\nmodify this testcase slightly to:\n\n Original: A/file exists\n our side: A/file -> C/file\n their side: rename C/ -> A/\n\nThen although this looks like where the directory rename just moves\nC/file back to A/file and there's no problem, we may not detect the\nA/file -> C/file rename. Instead it will look like a deletion of A/file\nand an addition of C/file. The directory rename then appears to be\nmoving C/file to A/file, which is on top of an \"unrelated\" file (or at\nleast a file it doesn't know is related). So, we will report\npath-in-the-way conflicts now in cases where we didn't before. That's\nbetter than silently and accidentally combining stages of unrelated\nfiles and making them look like a modify/delete; users can investigate\nthe reported conflict and simply resolve it.\n\nThis means we tweak the expected solution for testcases 12i, 12j, and\n12k. (Those three tests are basically the same test repeated three\ntimes, but I was worried when I added those that subtle differences in\nparent/child, sibling/sibling, and toplevel directories might mess up\nhow rename-to-self testcases actually get handled.)\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 18 +-\n t/t6423-merge-rename-directories.sh | 357 ++++++++++++++++++++++++++--\n 2 files changed, 355 insertions(+), 20 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex feb06720c7e1..f1ecccee940b 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2313,14 +2313,20 @@ static char *apply_dir_rename(struct strmap_entry *rename_info,\n \treturn strbuf_detach(&new_path, NULL);\n }\n \n-static int path_in_way(struct strmap *paths, const char *path, unsigned side_mask)\n+static int path_in_way(struct strmap *paths,\n+\t\t const char *path,\n+\t\t unsigned side_mask,\n+\t\t struct diff_filepair *p)\n {\n \tstruct merged_info *mi = strmap_get(paths, path);\n \tstruct conflict_info *ci;\n \tif (!mi)\n \t\treturn 0;\n \tINITIALIZE_CI(ci, mi);\n-\treturn mi->clean || (side_mask & (ci->filemask | ci->dirmask));\n+\treturn mi->clean || (side_mask & (ci->filemask | ci->dirmask))\n+\t // See testcases 12n, 12p, 12q for more details on this next condition\n+\t\t\t || ((ci->filemask & 0x01) &&\n+\t\t\t strcmp(p->one->path, path));\n }\n \n /*\n@@ -2332,6 +2338,7 @@ static int path_in_way(struct strmap *paths, const char *path, unsigned side_mas\n static char *handle_path_level_conflicts(struct merge_options *opt,\n \t\t\t\t\t const char *path,\n \t\t\t\t\t unsigned side_index,\n+\t\t\t\t\t struct diff_filepair *p,\n \t\t\t\t\t struct strmap_entry *rename_info,\n \t\t\t\t\t struct strmap *collisions)\n {\n@@ -2366,7 +2373,7 @@ static char *handle_path_level_conflicts(struct merge_options *opt,\n \t */\n \tif (c_info->reported_already) {\n \t\tclean = 0;\n-\t} else if (path_in_way(&opt->priv->paths, new_path, 1 << side_index)) {\n+\t} else if (path_in_way(&opt->priv->paths, new_path, 1 << side_index, p)) {\n \t\tc_info->reported_already = 1;\n \t\tstrbuf_add_separated_string_list(&collision_paths, \", \",\n \t\t\t\t\t\t &c_info->source_files);\n@@ -2573,6 +2580,7 @@ static void free_collisions(struct strmap *collisions)\n static char *check_for_directory_rename(struct merge_options *opt,\n \t\t\t\t\tconst char *path,\n \t\t\t\t\tunsigned side_index,\n+\t\t\t\t\tstruct diff_filepair *p,\n \t\t\t\t\tstruct strmap *dir_renames,\n \t\t\t\t\tstruct strmap *dir_rename_exclusions,\n \t\t\t\t\tstruct strmap *collisions,\n@@ -2629,7 +2637,7 @@ static char *check_for_directory_rename(struct merge_options *opt,\n \t\treturn NULL;\n \t}\n \n-\tnew_path = handle_path_level_conflicts(opt, path, side_index,\n+\tnew_path = handle_path_level_conflicts(opt, path, side_index, p,\n \t\t\t\t\t rename_info,\n \t\t\t\t\t &collisions[side_index]);\n \t*clean_merge &= (new_path != NULL);\n@@ -3428,7 +3436,7 @@ static int collect_renames(struct merge_options *opt,\n \t\t}\n \n \t\tnew_path = check_for_directory_rename(opt, p->two->path,\n-\t\t\t\t\t\t side_index,\n+\t\t\t\t\t\t side_index, p,\n \t\t\t\t\t\t dir_renames_for_side,\n \t\t\t\t\t\t rename_exclusions,\n \t\t\t\t\t\t collisions,\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 6611331769e9..f4d92b97ff80 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -4759,10 +4759,29 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\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\t# NOTE: A potentially better resolution would be for\n+\t\t# source/bar -> source/subdir/bar\n+\t\t# to use the directory rename to become\n+\t\t# source/bar -> source/bar\n+\t\t# (a rename to self), and thus we end up with bar with\n+\t\t# a path conflict (given merge.directoryRenames=conflict).\n+\t\t# However, since the relevant renames optimization\n+\t\t# prevents us from noticing\n+\t\t# source/bar -> source/subdir/bar\n+\t\t# as a rename and looking at it just as\n+\t\t# delete source/bar\n+\t\t# add source/subdir/bar\n+\t\t# the directory rename of source/subdir/bar -> source/bar does\n+\t\t# not look like a rename-to-self situation but a\n+\t\t# rename-on-top-of-other-file situation. We do not want\n+\t\t# stage 1 entries from an unrelated file, so we expect an\n+\t\t# error about there being a file in the way.\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\n+\t\tgrep \"CONFLICT (implicit dir rename).*source/bar in the way\" out &&\n+\t\ttest_path_is_missing source/bar &&\n+\t\ttest_path_is_file source/subdir/bar &&\n \t\ttest_path_is_file source/baz &&\n \n \t\tgit ls-files >actual &&\n@@ -4771,8 +4790,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\n \n \t\tgit status --porcelain -uno >actual &&\n \t\tcat >expect <<-\\EOF &&\n-\t\tUU source/bar\n \t\tM source/baz\n+\t\tR source/bar -> source/subdir/bar\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -4882,10 +4901,29 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '\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\t# NOTE: A potentially better resolution would be for\n+\t\t# bar -> subdir/bar\n+\t\t# to use the directory rename to become\n+\t\t# bar -> bar\n+\t\t# (a rename to self), and thus we end up with bar with\n+\t\t# a path conflict (given merge.directoryRenames=conflict).\n+\t\t# However, since the relevant renames optimization\n+\t\t# prevents us from noticing\n+\t\t# bar -> subdir/bar\n+\t\t# as a rename and looking at it just as\n+\t\t# delete bar\n+\t\t# add subdir/bar\n+\t\t# the directory rename of subdir/bar -> bar does not look\n+\t\t# like a rename-to-self situation but a\n+\t\t# rename-on-top-of-other-file situation. We do not want\n+\t\t# stage 1 entries from an unrelated file, so we expect an\n+\t\t# error about there being a file in the way.\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\t\tgrep \"CONFLICT (implicit dir rename).*bar in the way\" out &&\n+\n+\t\ttest_path_is_missing bar &&\n+\t\ttest_path_is_file subdir/bar &&\n \t\ttest_path_is_file baz &&\n \n \t\tgit ls-files >actual &&\n@@ -4894,8 +4932,8 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '\n \n \t\tgit status --porcelain -uno >actual &&\n \t\tcat >expect <<-\\EOF &&\n-\t\tUU bar\n \t\tM baz\n+\t\tR bar -> subdir/bar\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -4942,10 +4980,29 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\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\t# NOTE: A potentially better resolution would be for\n+\t\t# dirA/bar -> dirB/bar\n+\t\t# to use the directory rename (dirB/ -> dirA/) to become\n+\t\t# dirA/bar -> dirA/bar\n+\t\t# (a rename to self), and thus we end up with bar with\n+\t\t# a path conflict (given merge.directoryRenames=conflict).\n+\t\t# However, since the relevant renames optimization\n+\t\t# prevents us from noticing\n+\t\t# dirA/bar -> dirB/bar\n+\t\t# as a rename and looking at it just as\n+\t\t# delete dirA/bar\n+\t\t# add dirB/bar\n+\t\t# the directory rename of dirA/bar -> dirB/bar does\n+\t\t# not look like a rename-to-self situation but a\n+\t\t# rename-on-top-of-other-file situation. We do not want\n+\t\t# stage 1 entries from an unrelated file, so we expect an\n+\t\t# error about there being a file in the way.\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\t\tgrep \"CONFLICT (implicit dir rename).*dirA/bar in the way\" out &&\n+\n+\t\ttest_path_is_missing dirA/bar &&\n+\t\ttest_path_is_file dirB/bar &&\n \t\ttest_path_is_file dirA/baz &&\n \n \t\tgit ls-files >actual &&\n@@ -4954,8 +5011,8 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\n \n \t\tgit status --porcelain -uno >actual &&\n \t\tcat >expect <<-\\EOF &&\n-\t\tUU dirA/bar\n \t\tM dirA/baz\n+\t\tR dirA/bar -> dirB/bar\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -5259,6 +5316,276 @@ test_expect_success '12n2: Directory rename transitively makes rename back to se\n \t)\n '\n \n+# Testcase 12o, Directory rename hits other rename source; file still in way on same side\n+# Commit O: A/file1_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# C/other\n+# Commit A: A/file1_1\n+# A/stuff\n+# B/stuff\n+# C/file1_2\n+# C/other\n+# Commit B: D/file2_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# A/other\n+# In words:\n+# A: rename B/file1_2 -> C/file1_2\n+# B: rename C/ -> A/\n+# rename A/file1_1 -> D/file2_1\n+# Rationale:\n+# A/stuff is unmodified, it shows up in final output\n+# B/stuff is unmodified, it shows up in final output\n+# C/other touched on one side (rename to A), so A/other shows up in output\n+# A/file1 is renamed to D/file2\n+# B/file1 -> C/file1 and even though C/ -> A/, A/file1 is\n+# \"in the way\" so we don't do the directory rename\n+# Expected: A/stuff\n+# B/stuff\n+# A/other\n+# D/file2\n+# C/file1\n+# + CONFLICT (implicit dir rename): A/file1 in way of C/file1\n+#\n+\n+test_setup_12o () {\n+\tgit init 12o &&\n+\t(\n+\t\tcd 12o &&\n+\n+\t\tmkdir -p A B C &&\n+\t\techo 1 >A/file1 &&\n+\t\techo 2 >B/file1 &&\n+\t\techo other >C/other &&\n+\t\techo Astuff >A/stuff &&\n+\t\techo Bstuff >B/stuff &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"O\" &&\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 B/file1 C/ &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit switch B &&\n+\t\tmkdir -p D &&\n+\t\tgit mv A/file1 D/file2 &&\n+\t\tgit mv C/other A/other &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"B\"\n+\t)\n+}\n+\n+test_expect_success '12o: Directory rename hits other rename source; file still in way on same side' '\n+\ttest_setup_12o &&\n+\t(\n+\t\tcd 12o &&\n+\n+\t\tgit checkout -q A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\n+\t\ttest_stdout_line_count = 5 git ls-files -s &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/other &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s B/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s D/file2 &&\n+\n+\t\tgrep \"CONFLICT (implicit dir rename).*Existing file/dir at A/file1 in the way\" out &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s C/file1\n+\t)\n+'\n+\n+# Testcase 12p, Directory rename hits other rename source; file still in way on other side\n+# Commit O: A/file1_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# C/other\n+# Commit A: D/file2_1\n+# A/stuff\n+# B/stuff\n+# C/file1_2\n+# C/other\n+# Commit B: A/file1_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# A/other\n+# Short version:\n+# A: rename A/file1_1 -> D/file2_1\n+# rename B/file1_2 -> C/file1_2\n+# B: Rename C/ -> A/\n+# Rationale:\n+# A/stuff is unmodified, it shows up in final output\n+# B/stuff is unmodified, it shows up in final output\n+# C/other touched on one side (rename to A), so A/other shows up in output\n+# A/file1 is renamed to D/file2\n+# B/file1 -> C/file1 and even though C/ -> A/, A/file1 is\n+# \"in the way\" so we don't do the directory rename\n+# Expected: A/stuff\n+# B/stuff\n+# A/other\n+# D/file2\n+# C/file1\n+# + CONFLICT (implicit dir rename): A/file1 in way of C/file1\n+#\n+\n+test_setup_12p () {\n+\tgit init 12p &&\n+\t(\n+\t\tcd 12p &&\n+\n+\t\tmkdir -p A B C &&\n+\t\techo 1 >A/file1 &&\n+\t\techo 2 >B/file1 &&\n+\t\techo other >C/other &&\n+\t\techo Astuff >A/stuff &&\n+\t\techo Bstuff >B/stuff &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit switch A &&\n+\t\tmkdir -p D &&\n+\t\tgit mv A/file1 D/file2 &&\n+\t\tgit mv B/file1 C/ &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit switch B &&\n+\t\tgit mv C/other A/other &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"B\"\n+\t)\n+}\n+\n+test_expect_success '12p: Directory rename hits other rename source; file still in way on other side' '\n+\ttest_setup_12p &&\n+\t(\n+\t\tcd 12p &&\n+\n+\t\tgit checkout -q A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\n+\t\ttest_stdout_line_count = 5 git ls-files -s &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/other &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s B/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s D/file2 &&\n+\n+\t\tgrep \"CONFLICT (implicit dir rename).*Existing file/dir at A/file1 in the way\" out &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s C/file1\n+\t)\n+'\n+\n+# Testcase 12q, Directory rename hits other rename source; file removed though\n+# Commit O: A/file1_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# C/other\n+# Commit A: A/stuff\n+# B/stuff\n+# C/file1_2\n+# C/other\n+# Commit B: D/file2_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# A/other\n+# In words:\n+# A: delete A/file1_1, rename B/file1_2 -> C/file1_2\n+# B: Rename C/ -> A/, rename A/file1_1 -> D/file2_1\n+# Rationale:\n+# A/stuff is unmodified, it shows up in final output\n+# B/stuff is unmodified, it shows up in final output\n+# C/other touched on one side (rename to A), so A/other shows up in output\n+# A/file1 is rename/delete to D/file2, so two stages for D/file2\n+# B/file1 -> C/file1 and even though C/ -> A/, A/file1 as a source was\n+# \"in the way\" (ish) so we don't do the directory rename\n+# Expected: A/stuff\n+# B/stuff\n+# A/other\n+# D/file2 (two stages)\n+# C/file1\n+# + CONFLICT (implicit dir rename): A/file1 in way of C/file1\n+# + CONFLICT (rename/delete): D/file2\n+#\n+\n+test_setup_12q () {\n+\tgit init 12q &&\n+\t(\n+\t\tcd 12q &&\n+\n+\t\tmkdir -p A B C &&\n+\t\techo 1 >A/file1 &&\n+\t\techo 2 >B/file1 &&\n+\t\techo other >C/other &&\n+\t\techo Astuff >A/stuff &&\n+\t\techo Bstuff >B/stuff &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"O\" &&\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 rm A/file1 &&\n+\t\tgit mv B/file1 C/ &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit switch B &&\n+\t\tmkdir -p D &&\n+\t\tgit mv A/file1 D/file2 &&\n+\t\tgit mv C/other A/other &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"B\"\n+\t)\n+}\n+\n+test_expect_success '12q: Directory rename hits other rename source; file removed though' '\n+\ttest_setup_12q &&\n+\t(\n+\t\tcd 12q &&\n+\n+\t\tgit checkout -q A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\n+\t\tgrep \"CONFLICT (rename/delete).*A/file1.*D/file2\" out &&\n+\t\tgrep \"CONFLICT (implicit dir rename).*Existing file/dir at A/file1 in the way\" out &&\n+\n+\t\ttest_stdout_line_count = 6 git ls-files -s &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/other &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s B/stuff &&\n+\t\ttest_stdout_line_count = 2 git ls-files -s D/file2 &&\n+\n+\t\t# This is a slightly suboptimal resolution; allowing the\n+\t\t# rename of C/ -> A/ to affect C/file1 and further rename\n+\t\t# it to A/file1 would probably be preferable, but since\n+\t\t# A/file1 existed as the source of another rename, allowing\n+\t\t# the dir rename of C/file1 -> A/file1 would mean modifying\n+\t\t# the code so that renames do not adjust both their source\n+\t\t# and target paths in all cases.\n+\t\t! grep \"CONFLICT (file location)\" out &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s C/file1\n+\t)\n+'\n \n ###########################################################################\n # SECTION 13: Checking informational and conflict messages\n-- \ngitgitgadget\n"},{"id":"523173","messageId":"aIx7OEX6AEqNsIHb@pks.im","threadId":"63821","inReplyTo":"bda42aa85cf4f332ef60aca1a8937ed4b868fa87.1753197791.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/6] t6423: document two bugs with rename-to-self testcases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-01T08:30:48Z","receivedAt":"2025-08-01T08:31:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jul 22, 2025 at 03:23:08PM +0000, Elijah Newren via GitGitGadget wrote:\n> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> index f48ed6d03534..69de7a3b84af 100755\n> --- a/t/t6423-merge-rename-directories.sh\n> +++ b/t/t6423-merge-rename-directories.sh\n> @@ -5092,7 +5111,85 @@ test_expect_success '12n: Directory rename transitively makes rename back to sel\n> \t\tgit checkout -q B^0 &&\n> \n> \t\ttest_must_fail git cherry-pick A^0 >out &&\n> -\t\tgrep \"CONFLICT (file location).*should perhaps be moved\" out\n> +\t\tgrep \"CONFLICT (file location).*should perhaps be moved\" out &&\n\nLet's use `test_grep` while at it.\n\n[snip]\n> +test_expect_failure '12n2: Directory rename transitively makes rename back to self' '\n> +\ttest_setup_12n2 &&\n> +\t(\n> +\t\tcd 12n2 &&\n> +\n> +\t\tgit checkout -q B^0 &&\n> +\n> +\t\t# NOTE: Since merge.directoryRenames=true, there is no path\n> +\t\t# conflict for world vs. tools/world; it should end up at\n> +\t\t# world. The fact that world was unmodified on side A, means\n> +\t\t# there was no content conflict; we should just take the\n> +\t\t# content from side B -- i.e. delete the file. So merging\n> +\t\t# could just delete world.\n> +\t\t#\n> +\t\t# However, rename-to-self-via-directory-rename is a bit more\n> +\t\t# challenging. Relax this test to allow world to be treated\n> +\t\t# as a modify/delete conflict as well.\n> +\n> +\t\ttest_might_fail git -c merge.directoryRenames=true merge A^0 >out &&\n> +\n> +\t\t# Should have 1 entry for hello, and either 0 or 2 for world\n> +\t\ttest_stdout_line_count = 1 git ls-files -s hello &&\n> +\t\ttest_stdout_line_count != 1 git ls-files -s world &&\n> +\t\tif test_stdout_line_count != 0 git ls-files -s world\n> +\t\tthen\n> +\t\t\tgrep \"CONFLICT (modify/delete).*world deleted in HEAD\" out\n\nHere, as well.\n\n> +\t\tfi\n> \t)\n> '\n\nI found it to be a bit weird that we have this conditional here.\nShouldn't we expect one particular outcome? Even if multiple outcomes\nwould be techincally correct I think we should expect one particular\nresult, but we may add a comment to explain that different output would\nbe fine, too.\n\nPatrick\n"},{"id":"523174","messageId":"aIx7Qp_epPOpk8OF@pks.im","threadId":"63821","inReplyTo":"3b3b258cec5f0080beb64501f7510f7acbc3a91b.1753197791.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/6] t6423: fix missed staging of file in testcases 12i,12j,12k","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-01T08:30:58Z","receivedAt":"2025-08-01T08:31:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jul 22, 2025 at 03:23:09PM +0000, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> Commit 806f83287f8d (t6423: test directory renames causing\n> rename-to-self, 2021-06-30) introduced testcase 12i-12k but omitted\n> staging one of the files and copy-pasted that mistake to the other\n> tests. This means the merge runs with an unstaged change, even though\n> that isn't related to what is being tested and makes the test look more\n> complicated than it is.\n> \n> The cover letter for the series associated with the above commit noted\n\nIt might be a good idea to provide the message ID of that cover letter.\n\n> that these testcases triggered two bugs in merge-recursive but only one\n> in merge-ort; in merge-recursive these testcases also triggered a\n> silent deletion of the file in question when it shouldn't be deleted.\n> What I didn't realize at the time was that the deletion bug in merge-ort\n> was merely being sidestepped by the \"relevant renames\" optimization but\n> can actually be triggered. A subsequent commit will deal with that\n> additional bug, but it was complicated by the mistaken forgotten\n> staging, so this commit first fixes that issue.\n\nOkay.\n\n> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> index 69de7a3b84af..c2032eb6cfa1 100755\n> --- a/t/t6423-merge-rename-directories.sh\n> +++ b/t/t6423-merge-rename-directories.sh\n> @@ -5114,7 +5117,7 @@ test_expect_failure '12n: Directory rename transitively makes rename back to sel\n> \t\tgrep \"CONFLICT (file location).*should perhaps be moved\" out &&\n> \n> \t\t# Should have 1 entry for hello, and 1 for world\n> -\t\ttest_stdout_line_count = 2 git ls-files -s &&\n> +\t\ttest_stdout_line_count = 3 git ls-files -s &&\n> \t\ttest_stdout_line_count = 1 git ls-files -s hello &&\n> \t\ttest_stdout_line_count = 2 git ls-files -s world\n> \t)\n\nShould we also explicitly check `git ls-files -s baz`?\n\nPatrick\n"},{"id":"523175","messageId":"aIx7SXfRabJWpa0D@pks.im","threadId":"63821","inReplyTo":"2c7d4e022c59609bf263a7045fceb1854441bb29.1753197791.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/6] merge-ort: fix incorrect file handling","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-01T08:31:05Z","receivedAt":"2025-08-01T08:31:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jul 22, 2025 at 03:23:10PM +0000, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> We have multiple bugs here -- accidental silent file deletion,\n> accidental silent file retention for files that should be deleted,\n> and incorrect number of entries left in the index.\n> \n> The series merged at commit d3b88be1b450 (Merge branch\n> 'en/merge-dir-rename-corner-case-fix', 2021-07-16) introduced testcase\n> 12i-12k in t6423 which checked for rename-to-self cases, and fixed bugs\n> that merge-ort and merge-recursive had with these testcases. At the\n> time, I noted that merge-ort had one bug for these cases, while\n> merge-recursive had two. It turns out that merge-ort did in fact have\n> another bug, but the \"relevant renames\" optimizations were masking it.\n> If we modify testcase 12i from t6423 to modify the file in the commit\n> that renames it (but only modify it enough that it can still be detected\n> as a rename), then we can trigger silent deletion of the file.\n> \n> Tweak testcase 12i slightly to make the file in question have more than\n> one line in it, but which doesn't change how it operates.\n\nHm, the second part of this sentence doesn't quite parse for me. Do you\nmean to say that 12i is basically left intact except that you change the\ncontents of one line?\n\n> Make this\n> change to otherwise minimize the changes between this testcase and a new\n> one that we want to add. Then duplicate the testcase as 12i2, changing\n> it so that it adds a single line to the file in question when it is\n> renamed, as a testcase for this bug.\n\nOkay.\n\n> Further, commit 98a1a00d5301 (t6423: add a testcase causing a failed\n> assertion in process_renames, 2025-03-06), fixed an issue with\n> rename-to-self but added a new testcase, 12n, that only checked for\n> whether the merge ran to completion. A few commits ago, we modified\n> this test to check for the number of entries in the index -- but noted\n> that the number was wrong. And we also noted a\n> silently-keep-instead-of-delete bug at the same time in the new testcase\n> 12n2.\n> \n> Fix to merge-ort to prevent multiple bugs with rename-to-self cases:\n> * silent deletion of file expected to be kept (t6423 testcase 12i2)\n> * silent retention of file expected to be removed (t6423 testcase 12n2)\n> * wrong number of extries left in the index (t6423 testcase 12n)\n\nI think it would have been nice to also go a bit more in depth for what\nthe bug actually was and how it's fixed. You do add a comment, but that\nonly adds a single sentence of context.\n\n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 11 +++++\n> t/t6423-merge-rename-directories.sh | 69 +++++++++++++++++++++++++++--\n> 2 files changed, 77 insertions(+), 3 deletions(-)\n> \n> diff --git a/merge-ort.c b/merge-ort.c\n> index 9b9d82ed10f7..feb06720c7e1 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -2873,6 +2873,17 @@ static int process_renames(struct merge_options *opt,\n> \t\t\tnewinfo = new_ent->value;\n> \t\t}\n> \n> +\t\t/*\n> +\t\t * Directory renames can result in rename-to-self, which we\n> +\t\t * want to skip so we don't mark oldpath for deletion.\n> +\t\t *\n> +\t\t * Note that we can avoid strcmp here because of prior\n> +\t\t * diligence in apply_directory_rename_modifications() to\n> +\t\t * ensure we reused existing paths from opt->priv->paths.\n> +\t\t */\n> +\t\tif (oldpath == newpath)\n> +\t\t\tcontinue;\n\nMakes me wonder whether the additional brittleness is worth the saved\n`strcmp()` comparison. But on the other hand we do have tests now that\nwould break if the memory allocation patterns ever changed, so that's\nreassuring.\n\nPatrick\n"},{"id":"523176","messageId":"aIx7TxhXHhRI3vEt@pks.im","threadId":"63821","inReplyTo":"0813d42d91fe9e27f713de3c7d2377c28311ae57.1753197791.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/6] merge-ort: fix directory rename on top of source of other rename/delete","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-01T08:31:11Z","receivedAt":"2025-08-01T08:31:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jul 22, 2025 at 03:23:11PM +0000, Elijah Newren via GitGitGadget wrote:\n\nWhat a massive commit message. It almost felt like a blog post rather\nthan a commit message, but I certainly don't mind the additional\ncontext.\n\n> From: Elijah Newren \n> \n> At GitHub, we've got a real-world repository that has been triggering\n> failures of the form:\n> \n> git: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.\n> \n> which comes from the line:\n> \n> VERIFY_CI(newinfo);\n> \n> Unfortunately, this one has been quite complex to unravel, and is a\n> bit complex to explain. So, I'm going to carefully try to explain each\n> relevant piece needed to understand the fix, then carefully build up\n> from a simple testcase to some of the relevant testcases.\n> \n> == New special case we need to consider ==\n> \n> Rename pairs in the diffcore machinery connect the source path of a\n> rename with the destination path of a rename. Since we have rename\n> pairs to consider on both sides of history since the merge base,\n> merging has to consider a few special cases of possible overlap:\n> \n> A) two rename pairs having the same target path\n> B) two rename pairs having the same source path\n> C) the source path of one rename pair being the target path of a\n> different rename pair\n\nSo basically file A get's moved somewhere else and then replaced by a\ndifferent file B?\n\n> Some of these came up often enough that we gave them names:\n> A) a rename/rename(2to1) conflict (looks similar to an add/add conflict)\n> B) a rename/rename(1to2) conflict, which represents the same path being\n> renamed differently on the two sides of history\n> C) not yet named\n> \n> merge-ort is well-prepared to handle cases (A) and (B), as was\n> merge-recursive (which was merge-ort's predecessor). Case (C) was\n> briefly considered during the years of merge-recursive maintenance,\n> but the full extent of support it got was a few FIXME/TODO comments\n> littered around the code highlighting some of the places that would\n> probably need to be fixed to support it. When I wrote merge-ort I\n> ignored case (C) entirely, since I believed that case (C) was only\n> possible if we were to support break detection during merges. Not\n> only had break detection never been supported by any merge algorithm,\n> I thought break detection wasn't worth the effort to support in a\n> merge algorithm. However, it turns out that case (C) can be triggered\n> without break detection, if there's enough moving pieces.\n> \n> Before I dive into how to trigger case (C) with directory renames plus\n> other renames, it might be helpful to use a simpler example with break\n> detection first. And before we get to that it may help to explain\n> some more basics of handling renames in the merge algorithm. So, let\n> me first backup and provide a quick refresher on on each of\n\ns/on on/on/\n\n[snip]\n> == Directory rename detection ==\n> \n> If one side of history renames directory D/ -> E/, and the other side of\n> history adds new files to E/, then directory rename detection notices\n\nDid you mean to say \"D/\" here?\n\n[snip]\n> == Testcases 8+ ==\n> \n> Another bonus bug, found via understanding our final solution (and the\n> failure of our first attempted solution)!\n\ns/solution/solutions/ as there are multiple attempted solutions that\nwere discarded?\n\n> diff --git a/merge-ort.c b/merge-ort.c\n> index feb06720c7e1..f1ecccee940b 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -2313,14 +2313,20 @@ static char *apply_dir_rename(struct strmap_entry *rename_info,\n> \treturn strbuf_detach(&new_path, NULL);\n> }\n> \n> -static int path_in_way(struct strmap *paths, const char *path, unsigned side_mask)\n> +static int path_in_way(struct strmap *paths,\n> +\t\t const char *path,\n> +\t\t unsigned side_mask,\n> +\t\t struct diff_filepair *p)\n> {\n> \tstruct merged_info *mi = strmap_get(paths, path);\n> \tstruct conflict_info *ci;\n> \tif (!mi)\n> \t\treturn 0;\n> \tINITIALIZE_CI(ci, mi);\n> -\treturn mi->clean || (side_mask & (ci->filemask | ci->dirmask));\n> +\treturn mi->clean || (side_mask & (ci->filemask | ci->dirmask))\n> +\t // See testcases 12n, 12p, 12q for more details on this next condition\n\nThis should use `/* */`-style comments.\n\n> +\t\t\t || ((ci->filemask & 0x01) &&\n> +\t\t\t strcmp(p->one->path, path));\n\nSo if we have a stage 1 index entry and the path is the same due to a\ntransitive rename we can say that the path is not in the way?\n\nPatrick\n"},{"id":"523177","messageId":"aIx7VOgyFoPt8del@pks.im","threadId":"63821","inReplyTo":"pull.1943.git.1753197791.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/6] Fix various rename corner cases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-01T08:31:16Z","receivedAt":"2025-08-01T08:31:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jul 22, 2025 at 03:23:05PM +0000, Elijah Newren via GitGitGadget wrote:\n> At GitHub, we've got a real-world repository that has been triggering\n> failures of the form:\n> \n> git: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.\n> \n> \n> Digging in, this particular corner case requires multiple things to trigger:\n> (1) a rename/delete of one file, and (2) a directory rename modifying an\n> unrelated rename such that this unrelated rename's target becomes the source\n> of the rename/delete from (1).\n> \n> Unfortunately, looking around, it's not the only bug in the area. Plus, some\n> of our testcases in tangential situations were not checked closely enough or\n> were weird or buggy in various ways. Adding to the challenge was the fact\n> that the relevant renames optimization was sometimes triggering making\n> renames look like delete-and-add, and overlooking this meant I sometimes\n> wasn't triggering what I thought I was triggering.\n> \n> The combination of challenges sometimes made me think my fixes were breaking\n> things when sometimes I was just unaware of other bugs. I went in circles a\n> few times and took a rather non-linear path to finding and fixing these\n> issues. While I think I've turned it into a nice linear progression of\n> patches, I might be a bit too deep in the mud and it might not be as linear\n> or clear as I think. Let me know and I'll try to clarify anything needed.\n\nI had a read through this whole series and it looks plausible to me. I\nwon't claim to fully understand it though given that I don't have a lot\nof experience with the whole merge machinery.\n\nPatrick\n"},{"id":"523476","messageId":"CABPp-BEUFaePoJx-dn9hOE6r7mQV_W_6QF2K1sJJ2uXeL81rdg@mail.gmail.com","threadId":"63821","inReplyTo":"aIx7OEX6AEqNsIHb@pks.im","subject":"Re: [PATCH 3/6] t6423: document two bugs with rename-to-self testcases","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-08-04T19:15:15Z","receivedAt":"2025-08-04T19:15:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 1, 2025 at 1:31 AM Patrick Steinhardt wrote:\n>\n> On Tue, Jul 22, 2025 at 03:23:08PM +0000, Elijah Newren via GitGitGadget wrote:\n> > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> > index f48ed6d03534..69de7a3b84af 100755\n> > --- a/t/t6423-merge-rename-directories.sh\n> > +++ b/t/t6423-merge-rename-directories.sh\n> > @@ -5092,7 +5111,85 @@ test_expect_success '12n: Directory rename transitively makes rename back to sel\n> > git checkout -q B^0 &&\n> >\n> > test_must_fail git cherry-pick A^0 >out &&\n> > - grep \"CONFLICT (file location).*should perhaps be moved\" out\n> > + grep \"CONFLICT (file location).*should perhaps be moved\" out &&\n>\n> Let's use `test_grep` while at it.\n\nOh, right, it was test_i18ngrep that we weren't supposed to use.\nYeah, I can switch over to test_grep.\n\n> [snip]\n> > +test_expect_failure '12n2: Directory rename transitively makes rename back to self' '\n> > + test_setup_12n2 &&\n> > + (\n> > + cd 12n2 &&\n> > +\n> > + git checkout -q B^0 &&\n> > +\n> > + # NOTE: Since merge.directoryRenames=true, there is no path\n> > + # conflict for world vs. tools/world; it should end up at\n> > + # world. The fact that world was unmodified on side A, means\n> > + # there was no content conflict; we should just take the\n> > + # content from side B -- i.e. delete the file. So merging\n> > + # could just delete world.\n> > + #\n> > + # However, rename-to-self-via-directory-rename is a bit more\n> > + # challenging. Relax this test to allow world to be treated\n> > + # as a modify/delete conflict as well.\n> > +\n> > + test_might_fail git -c merge.directoryRenames=true merge A^0 >out &&\n> > +\n> > + # Should have 1 entry for hello, and either 0 or 2 for world\n> > + test_stdout_line_count = 1 git ls-files -s hello &&\n> > + test_stdout_line_count != 1 git ls-files -s world &&\n> > + if test_stdout_line_count != 0 git ls-files -s world\n> > + then\n> > + grep \"CONFLICT (modify/delete).*world deleted in HEAD\" out\n>\n> Here, as well.\n\nWill do.\n\n> > + fi\n> > )\n> > '\n>\n> I found it to be a bit weird that we have this conditional here.\n> Shouldn't we expect one particular outcome? Even if multiple outcomes\n> would be techincally correct I think we should expect one particular\n> result, but we may add a comment to explain that different output would\n> be fine, too.\n\nIsn't that exactly what I did, with the note I'll copy below?\n\n> > + # NOTE: Since merge.directoryRenames=true, there is no path\n> > + # conflict for world vs. tools/world; it should end up at\n> > + # world. The fact that world was unmodified on side A, means\n> > + # there was no content conflict; we should just take the\n> > + # content from side B -- i.e. delete the file. So merging\n> > + # could just delete world.\n> > + #\n> > + # However, rename-to-self-via-directory-rename is a bit more\n> > + # challenging. Relax this test to allow world to be treated\n> > + # as a modify/delete conflict as well.\n"},{"id":"523477","messageId":"CABPp-BF36TtAaqbV01m82Cj_7Mr23P1DEuSTR7oM9odLbD9q5g@mail.gmail.com","threadId":"63821","inReplyTo":"aIx7Qp_epPOpk8OF@pks.im","subject":"Re: [PATCH 4/6] t6423: fix missed staging of file in testcases 12i,12j,12k","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-08-04T19:23:49Z","receivedAt":"2025-08-04T19:24:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 1, 2025 at 1:31 AM Patrick Steinhardt wrote:\n>\n> On Tue, Jul 22, 2025 at 03:23:09PM +0000, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > Commit 806f83287f8d (t6423: test directory renames causing\n> > rename-to-self, 2021-06-30) introduced testcase 12i-12k but omitted\n> > staging one of the files and copy-pasted that mistake to the other\n> > tests. This means the merge runs with an unstaged change, even though\n> > that isn't related to what is being tested and makes the test look more\n> > complicated than it is.\n> >\n> > The cover letter for the series associated with the above commit noted\n>\n> It might be a good idea to provide the message ID of that cover letter.\n\nSounds like a good idea; will do.\n\n> > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> > index 69de7a3b84af..c2032eb6cfa1 100755\n> > --- a/t/t6423-merge-rename-directories.sh\n> > +++ b/t/t6423-merge-rename-directories.sh\n> > @@ -5114,7 +5117,7 @@ test_expect_failure '12n: Directory rename transitively makes rename back to sel\n> > grep \"CONFLICT (file location).*should perhaps be moved\" out &&\n> >\n> > # Should have 1 entry for hello, and 1 for world\n> > - test_stdout_line_count = 2 git ls-files -s &&\n> > + test_stdout_line_count = 3 git ls-files -s &&\n> > test_stdout_line_count = 1 git ls-files -s hello &&\n> > test_stdout_line_count = 2 git ls-files -s world\n> > )\n>\n> Should we also explicitly check `git ls-files -s baz`?\n\nWhy? There was no baz in this testcase -- not only did it not appear\nin the final commit, it didn't appear in either branch being merged\nnor anywhere in the entire history of the repository. Testcases\n12{i,j,k} all had such a file, but testcase 12n does not.\n"},{"id":"523489","messageId":"CABPp-BF_UGf2VXvkAj=nkEPwo=FVQTgVvrDFURkBnFv4Wkiknw@mail.gmail.com","threadId":"63821","inReplyTo":"aIx7SXfRabJWpa0D@pks.im","subject":"Re: [PATCH 5/6] merge-ort: fix incorrect file handling","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-08-04T22:08:50Z","receivedAt":"2025-08-04T22:09:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 1, 2025 at 1:31 AM Patrick Steinhardt wrote:\n>\n> On Tue, Jul 22, 2025 at 03:23:10PM +0000, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > We have multiple bugs here -- accidental silent file deletion,\n> > accidental silent file retention for files that should be deleted,\n> > and incorrect number of entries left in the index.\n> >\n> > The series merged at commit d3b88be1b450 (Merge branch\n> > 'en/merge-dir-rename-corner-case-fix', 2021-07-16) introduced testcase\n> > 12i-12k in t6423 which checked for rename-to-self cases, and fixed bugs\n> > that merge-ort and merge-recursive had with these testcases. At the\n> > time, I noted that merge-ort had one bug for these cases, while\n> > merge-recursive had two. It turns out that merge-ort did in fact have\n> > another bug, but the \"relevant renames\" optimizations were masking it.\n> > If we modify testcase 12i from t6423 to modify the file in the commit\n> > that renames it (but only modify it enough that it can still be detected\n> > as a rename), then we can trigger silent deletion of the file.\n> >\n> > Tweak testcase 12i slightly to make the file in question have more than\n> > one line in it, but which doesn't change how it operates.\n>\n> Hm, the second part of this sentence doesn't quite parse for me. Do you\n> mean to say that 12i is basically left intact except that you change the\n> contents of one line?\n\nYeah, sometimes my repeated editing of text leaves things not so\nclear. You are correct that I meant leaving the testcase intact other\nthan changing the initial contents of one file (though I changed the\ncontents so it had multiple lines, not just giving it a different\nsingle line). I'll reword it.\n\n> > Make this\n> > change to otherwise minimize the changes between this testcase and a new\n> > one that we want to add. Then duplicate the testcase as 12i2, changing\n> > it so that it adds a single line to the file in question when it is\n> > renamed, as a testcase for this bug.\n>\n> Okay.\n>\n> > Further, commit 98a1a00d5301 (t6423: add a testcase causing a failed\n> > assertion in process_renames, 2025-03-06), fixed an issue with\n> > rename-to-self but added a new testcase, 12n, that only checked for\n> > whether the merge ran to completion. A few commits ago, we modified\n> > this test to check for the number of entries in the index -- but noted\n> > that the number was wrong. And we also noted a\n> > silently-keep-instead-of-delete bug at the same time in the new testcase\n> > 12n2.\n> >\n> > Fix to merge-ort to prevent multiple bugs with rename-to-self cases:\n> > * silent deletion of file expected to be kept (t6423 testcase 12i2)\n> > * silent retention of file expected to be removed (t6423 testcase 12n2)\n> > * wrong number of extries left in the index (t6423 testcase 12n)\n>\n> I think it would have been nice to also go a bit more in depth for what\n> the bug actually was and how it's fixed. You do add a comment, but that\n> only adds a single sentence of context.\n\nWould something like this help:\n\n...all of these issues come up because in a rename-to-self case, when\nwe have a rename A->B, both A and B name the same file. The code in\nprocess_renames() assumes A & B are different, and tries to move the\nhigher order stages and file contents so that they are associated just\nwith the new path, but the assumptions of A & B being different can\ncause A to be deleted when it's not supposed to be or mark B as\nresolved and kept in place when it's supposed to be deleted. Since A\n& B are already the same path in the rename-to-self case, we can\nsimply skip the steps in process_renames() for such files.\n\n> > Signed-off-by: Elijah Newren \n> > ---\n> > merge-ort.c | 11 +++++\n> > t/t6423-merge-rename-directories.sh | 69 +++++++++++++++++++++++++++--\n> > 2 files changed, 77 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index 9b9d82ed10f7..feb06720c7e1 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -2873,6 +2873,17 @@ static int process_renames(struct merge_options *opt,\n> > newinfo = new_ent->value;\n> > }\n> >\n> > + /*\n> > + * Directory renames can result in rename-to-self, which we\n> > + * want to skip so we don't mark oldpath for deletion.\n> > + *\n> > + * Note that we can avoid strcmp here because of prior\n> > + * diligence in apply_directory_rename_modifications() to\n> > + * ensure we reused existing paths from opt->priv->paths.\n> > + */\n> > + if (oldpath == newpath)\n> > + continue;\n>\n> Makes me wonder whether the additional brittleness is worth the saved\n> `strcmp()` comparison. But on the other hand we do have tests now that\n> would break if the memory allocation patterns ever changed, so that's\n> reassuring.\n\nThere's no brittleness here; one of the many optimizations in\nmerge-ort.c was to intern *all* pathnames in struct\nmerge_options_internal's \"paths\" member; any code that needs to\ngenerate/compute a filename that may be part of the merge must check\nif that path already exists in opt->priv->paths, and if so use that\npointer instead so that all filename comparisons can be done with\ncheap pointer comparisons. See the big comment near the top of\nmerge_options_internal. Nearly all such\nstring-equality-via-pointer-equality checks were introduced about the\nsame time, and in other functions, which makes this one kind of an\noutlier. I figured whoever reviewed this patch or ran across this in\nthe code might get surprised by the pointer comparison, so I tried to\nadd a comment to address any questions. Looks like I wasn't thorough\nenough (and the first paragraph of the comment pre-dated my finding\nother bugs this patch fixed, which makes it slightly confusing), so\nI'll try to see if I can improve it for v2.\n"},{"id":"523492","messageId":"CABPp-BFUCPdxHi6XqNwGrUf7JBBJnd1SuPkHckg-8VyX=RGm+Q@mail.gmail.com","threadId":"63821","inReplyTo":"aIx7TxhXHhRI3vEt@pks.im","subject":"Re: [PATCH 6/6] merge-ort: fix directory rename on top of source of other rename/delete","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-08-04T22:33:14Z","receivedAt":"2025-08-04T22:33:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 1, 2025 at 1:31 AM Patrick Steinhardt wrote:\n>\n> On Tue, Jul 22, 2025 at 03:23:11PM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> What a massive commit message. It almost felt like a blog post rather\n> than a commit message, but I certainly don't mind the additional\n> context.\n\nYeah...it certainly is. I was spinning my wheels for a few weeks because of\n * the number of items needed to trigger the issue\n * the misleading/buggy testcases we had, now addressed earlier in this series\n * the number of other nearby bugs that also existed\n * the fact that \"relevant renames\" sometimes tricks you into\nthinking you're testing a rename testcase when you're not\nand when I started explaining it, I realized there was lots of assumed\nbackground needed to understand the bug that I'm not sure others on\nthe list would have.\n\nBesides, I was worried that _I_ would forget all these details in 6\nmonths, so I wanted it all spelled out.\n\nThanks for being understanding of the lengthy tome this commit message\nbecame. And for reading all of it!\n\n> > From: Elijah Newren \n> >\n> > At GitHub, we've got a real-world repository that has been triggering\n> > failures of the form:\n> >\n> > git: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.\n> >\n> > which comes from the line:\n> >\n> > VERIFY_CI(newinfo);\n> >\n> > Unfortunately, this one has been quite complex to unravel, and is a\n> > bit complex to explain. So, I'm going to carefully try to explain each\n> > relevant piece needed to understand the fix, then carefully build up\n> > from a simple testcase to some of the relevant testcases.\n> >\n> > == New special case we need to consider ==\n> >\n> > Rename pairs in the diffcore machinery connect the source path of a\n> > rename with the destination path of a rename. Since we have rename\n> > pairs to consider on both sides of history since the merge base,\n> > merging has to consider a few special cases of possible overlap:\n> >\n> > A) two rename pairs having the same target path\n> > B) two rename pairs having the same source path\n> > C) the source path of one rename pair being the target path of a\n> > different rename pair\n>\n> So basically file A get's moved somewhere else and then replaced by a\n> different file B?\n\nYup.\n\n>\n> > Some of these came up often enough that we gave them names:\n> > A) a rename/rename(2to1) conflict (looks similar to an add/add conflict)\n> > B) a rename/rename(1to2) conflict, which represents the same path being\n> > renamed differently on the two sides of history\n> > C) not yet named\n> >\n> > merge-ort is well-prepared to handle cases (A) and (B), as was\n> > merge-recursive (which was merge-ort's predecessor). Case (C) was\n> > briefly considered during the years of merge-recursive maintenance,\n> > but the full extent of support it got was a few FIXME/TODO comments\n> > littered around the code highlighting some of the places that would\n> > probably need to be fixed to support it. When I wrote merge-ort I\n> > ignored case (C) entirely, since I believed that case (C) was only\n> > possible if we were to support break detection during merges. Not\n> > only had break detection never been supported by any merge algorithm,\n> > I thought break detection wasn't worth the effort to support in a\n> > merge algorithm. However, it turns out that case (C) can be triggered\n> > without break detection, if there's enough moving pieces.\n> >\n> > Before I dive into how to trigger case (C) with directory renames plus\n> > other renames, it might be helpful to use a simpler example with break\n> > detection first. And before we get to that it may help to explain\n> > some more basics of handling renames in the merge algorithm. So, let\n> > me first backup and provide a quick refresher on on each of\n>\n> s/on on/on/\n\nThanks.\n\n> [snip]\n> > == Directory rename detection ==\n> >\n> > If one side of history renames directory D/ -> E/, and the other side of\n> > history adds new files to E/, then directory rename detection notices\n>\n> Did you mean to say \"D/\" here?\n\nYes, thanks.\n\n> [snip]\n> > == Testcases 8+ ==\n> >\n> > Another bonus bug, found via understanding our final solution (and the\n> > failure of our first attempted solution)!\n>\n> s/solution/solutions/ as there are multiple attempted solutions that\n> were discarded?\n\nYeah, I typed up this commit message and then found more issues, and\ninserted them earlier. I'll fix up the wording; thanks.\n\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index feb06720c7e1..f1ecccee940b 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -2313,14 +2313,20 @@ static char *apply_dir_rename(struct strmap_entry *rename_info,\n> > return strbuf_detach(&new_path, NULL);\n> > }\n> >\n> > -static int path_in_way(struct strmap *paths, const char *path, unsigned side_mask)\n> > +static int path_in_way(struct strmap *paths,\n> > + const char *path,\n> > + unsigned side_mask,\n> > + struct diff_filepair *p)\n> > {\n> > struct merged_info *mi = strmap_get(paths, path);\n> > struct conflict_info *ci;\n> > if (!mi)\n> > return 0;\n> > INITIALIZE_CI(ci, mi);\n> > - return mi->clean || (side_mask & (ci->filemask | ci->dirmask));\n> > + return mi->clean || (side_mask & (ci->filemask | ci->dirmask))\n> > + // See testcases 12n, 12p, 12q for more details on this next condition\n>\n> This should use `/* */`-style comments.\n\nYep, will fix.\n\n>\n> > + || ((ci->filemask & 0x01) &&\n> > + strcmp(p->one->path, path));\n>\n> So if we have a stage 1 index entry and the path is the same due to a\n> transitive rename we can say that the path is not in the way?\n\nRight if A -> A, then we know that the original A and the new A do in\nfact have related contents and thus the old A is not in the way of the\nnew A. I would have rather just checked for a stage 1 index entry and\nsaid that the presence of such a thing means there's a file in the way\n(that's what I originally did), but the rename-to-self case is special\nand is why the strcmp condition is there.\n"},{"id":"523502","messageId":"aJGK1afGLkNAQID6@pks.im","threadId":"63821","inReplyTo":"CABPp-BEUFaePoJx-dn9hOE6r7mQV_W_6QF2K1sJJ2uXeL81rdg@mail.gmail.com","subject":"Re: [PATCH 3/6] t6423: document two bugs with rename-to-self testcases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-05T04:38:45Z","receivedAt":"2025-08-05T04:38:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 04, 2025 at 12:15:15PM -0700, Elijah Newren wrote:\n> On Fri, Aug 1, 2025 at 1:31 AM Patrick Steinhardt wrote:\n> >\n> > On Tue, Jul 22, 2025 at 03:23:08PM +0000, Elijah Newren via GitGitGadget wrote:\n> > > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> > > index f48ed6d03534..69de7a3b84af 100755\n> > > --- a/t/t6423-merge-rename-directories.sh\n> > > +++ b/t/t6423-merge-rename-directories.sh\n> > I found it to be a bit weird that we have this conditional here.\n> > Shouldn't we expect one particular outcome? Even if multiple outcomes\n> > would be techincally correct I think we should expect one particular\n> > result, but we may add a comment to explain that different output would\n> > be fine, too.\n> \n> Isn't that exactly what I did, with the note I'll copy below?\n\nNot quite -- you do have a comment explaining why you relax the test.\nBut I think it would be preferable to _not_ relax the test but still\nhave a comment that says that the outcome isn't quite clear cut. This\nwould alert us if the outcome ever changed and thus make it way more of\na concious change if we had to adapt the test, but it would still leave\na future reader in the know that a changed test outcome might actually\nbe okay.\n\nPatrick\n"},{"id":"523503","messageId":"aJGK4tSCkWgBK32Q@pks.im","threadId":"63821","inReplyTo":"CABPp-BF36TtAaqbV01m82Cj_7Mr23P1DEuSTR7oM9odLbD9q5g@mail.gmail.com","subject":"Re: [PATCH 4/6] t6423: fix missed staging of file in testcases 12i,12j,12k","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-05T04:38:58Z","receivedAt":"2025-08-05T04:39:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 04, 2025 at 12:23:49PM -0700, Elijah Newren wrote:\n> On Fri, Aug 1, 2025 at 1:31 AM Patrick Steinhardt wrote:\n> > On Tue, Jul 22, 2025 at 03:23:09PM +0000, Elijah Newren via GitGitGadget wrote:\n> > > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> > > index 69de7a3b84af..c2032eb6cfa1 100755\n> > > --- a/t/t6423-merge-rename-directories.sh\n> > > +++ b/t/t6423-merge-rename-directories.sh\n> > > @@ -5114,7 +5117,7 @@ test_expect_failure '12n: Directory rename transitively makes rename back to sel\n> > > grep \"CONFLICT (file location).*should perhaps be moved\" out &&\n> > >\n> > > # Should have 1 entry for hello, and 1 for world\n> > > - test_stdout_line_count = 2 git ls-files -s &&\n> > > + test_stdout_line_count = 3 git ls-files -s &&\n> > > test_stdout_line_count = 1 git ls-files -s hello &&\n> > > test_stdout_line_count = 2 git ls-files -s world\n> > > )\n> >\n> > Should we also explicitly check `git ls-files -s baz`?\n> \n> Why? There was no baz in this testcase -- not only did it not appear\n> in the final commit, it didn't appear in either branch being merged\n> nor anywhere in the entire history of the repository. Testcases\n> 12{i,j,k} all had such a file, but testcase 12n does not.\n\nMostly because the line count was adjusted, so it seems clear to me that\n\"baz\" at least plays a role here. Otherwise there's a mismatch between\nthe number of lines we see and the state of files we verify.\n\nPatrick\n"},{"id":"523504","messageId":"aJGK6XFdmYj50I9J@pks.im","threadId":"63821","inReplyTo":"CABPp-BF_UGf2VXvkAj=nkEPwo=FVQTgVvrDFURkBnFv4Wkiknw@mail.gmail.com","subject":"Re: [PATCH 5/6] merge-ort: fix incorrect file handling","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-05T04:39:05Z","receivedAt":"2025-08-05T04:39:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 04, 2025 at 03:08:50PM -0700, Elijah Newren wrote:\n> On Fri, Aug 1, 2025 at 1:31 AM Patrick Steinhardt wrote:\n> > On Tue, Jul 22, 2025 at 03:23:10PM +0000, Elijah Newren via GitGitGadget wrote:\n> > > Further, commit 98a1a00d5301 (t6423: add a testcase causing a failed\n> > > assertion in process_renames, 2025-03-06), fixed an issue with\n> > > rename-to-self but added a new testcase, 12n, that only checked for\n> > > whether the merge ran to completion. A few commits ago, we modified\n> > > this test to check for the number of entries in the index -- but noted\n> > > that the number was wrong. And we also noted a\n> > > silently-keep-instead-of-delete bug at the same time in the new testcase\n> > > 12n2.\n> > >\n> > > Fix to merge-ort to prevent multiple bugs with rename-to-self cases:\n> > > * silent deletion of file expected to be kept (t6423 testcase 12i2)\n> > > * silent retention of file expected to be removed (t6423 testcase 12n2)\n> > > * wrong number of extries left in the index (t6423 testcase 12n)\n> >\n> > I think it would have been nice to also go a bit more in depth for what\n> > the bug actually was and how it's fixed. You do add a comment, but that\n> > only adds a single sentence of context.\n> \n> Would something like this help:\n> \n> ...all of these issues come up because in a rename-to-self case, when\n> we have a rename A->B, both A and B name the same file. The code in\n> process_renames() assumes A & B are different, and tries to move the\n> higher order stages and file contents so that they are associated just\n> with the new path, but the assumptions of A & B being different can\n> cause A to be deleted when it's not supposed to be or mark B as\n> resolved and kept in place when it's supposed to be deleted. Since A\n> & B are already the same path in the rename-to-self case, we can\n> simply skip the steps in process_renames() for such files.\n\nYes, it would!\n\n> > > Signed-off-by: Elijah Newren \n> > > ---\n> > > merge-ort.c | 11 +++++\n> > > t/t6423-merge-rename-directories.sh | 69 +++++++++++++++++++++++++++--\n> > > 2 files changed, 77 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/merge-ort.c b/merge-ort.c\n> > > index 9b9d82ed10f7..feb06720c7e1 100644\n> > > --- a/merge-ort.c\n> > > +++ b/merge-ort.c\n> > > @@ -2873,6 +2873,17 @@ static int process_renames(struct merge_options *opt,\n> > > newinfo = new_ent->value;\n> > > }\n> > >\n> > > + /*\n> > > + * Directory renames can result in rename-to-self, which we\n> > > + * want to skip so we don't mark oldpath for deletion.\n> > > + *\n> > > + * Note that we can avoid strcmp here because of prior\n> > > + * diligence in apply_directory_rename_modifications() to\n> > > + * ensure we reused existing paths from opt->priv->paths.\n> > > + */\n> > > + if (oldpath == newpath)\n> > > + continue;\n> >\n> > Makes me wonder whether the additional brittleness is worth the saved\n> > `strcmp()` comparison. But on the other hand we do have tests now that\n> > would break if the memory allocation patterns ever changed, so that's\n> > reassuring.\n> \n> There's no brittleness here; one of the many optimizations in\n> merge-ort.c was to intern *all* pathnames in struct\n> merge_options_internal's \"paths\" member; any code that needs to\n> generate/compute a filename that may be part of the merge must check\n> if that path already exists in opt->priv->paths, and if so use that\n> pointer instead so that all filename comparisons can be done with\n> cheap pointer comparisons. See the big comment near the top of\n> merge_options_internal. Nearly all such\n> string-equality-via-pointer-equality checks were introduced about the\n> same time, and in other functions, which makes this one kind of an\n> outlier. I figured whoever reviewed this patch or ran across this in\n> the code might get surprised by the pointer comparison, so I tried to\n> add a comment to address any questions. Looks like I wasn't thorough\n> enough (and the first paragraph of the comment pre-dated my finding\n> other bugs this patch fixed, which makes it slightly confusing), so\n> I'll try to see if I can improve it for v2.\n\nI think the current version is good enough -- it feels brittle to me,\nbut I don't see a strong reason to change it.\n\nPatrick\n"},{"id":"523580","messageId":"CABPp-BGmipbdicHeFfLQOq2EG5QUk5GxHxhDuwSKgM4x6YO34w@mail.gmail.com","threadId":"63821","inReplyTo":"aJGK1afGLkNAQID6@pks.im","subject":"Re: [PATCH 3/6] t6423: document two bugs with rename-to-self testcases","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-08-05T18:33:38Z","receivedAt":"2025-08-05T18:33:51Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Aug 4, 2025 at 9:38 PM Patrick Steinhardt wrote:\n>\n> On Mon, Aug 04, 2025 at 12:15:15PM -0700, Elijah Newren wrote:\n> > On Fri, Aug 1, 2025 at 1:31 AM Patrick Steinhardt wrote:\n> > >\n> > > On Tue, Jul 22, 2025 at 03:23:08PM +0000, Elijah Newren via GitGitGadget wrote:\n> > > > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> > > > index f48ed6d03534..69de7a3b84af 100755\n> > > > --- a/t/t6423-merge-rename-directories.sh\n> > > > +++ b/t/t6423-merge-rename-directories.sh\n> > > I found it to be a bit weird that we have this conditional here.\n> > > Shouldn't we expect one particular outcome? Even if multiple outcomes\n> > > would be techincally correct I think we should expect one particular\n> > > result, but we may add a comment to explain that different output would\n> > > be fine, too.\n> >\n> > Isn't that exactly what I did, with the note I'll copy below?\n>\n> Not quite -- you do have a comment explaining why you relax the test.\n> But I think it would be preferable to _not_ relax the test but still\n> have a comment that says that the outcome isn't quite clear cut. This\n> would alert us if the outcome ever changed and thus make it way more of\n> a concious change if we had to adapt the test, but it would still leave\n> a future reader in the know that a changed test outcome might actually\n> be okay.\n\nAh, gotcha.\n"},{"id":"523581","messageId":"CABPp-BECD8RwYx=ci1_kpu_vkctMZL-QqA+Da-sYfoCEhGKRQw@mail.gmail.com","threadId":"63821","inReplyTo":"aJGK4tSCkWgBK32Q@pks.im","subject":"Re: [PATCH 4/6] t6423: fix missed staging of file in testcases 12i,12j,12k","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-08-05T18:33:49Z","receivedAt":"2025-08-05T18:34:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Aug 4, 2025 at 9:39 PM Patrick Steinhardt wrote:\n>\n> On Mon, Aug 04, 2025 at 12:23:49PM -0700, Elijah Newren wrote:\n> > On Fri, Aug 1, 2025 at 1:31 AM Patrick Steinhardt wrote:\n> > > On Tue, Jul 22, 2025 at 03:23:09PM +0000, Elijah Newren via GitGitGadget wrote:\n> > > > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> > > > index 69de7a3b84af..c2032eb6cfa1 100755\n> > > > --- a/t/t6423-merge-rename-directories.sh\n> > > > +++ b/t/t6423-merge-rename-directories.sh\n> > > > @@ -5114,7 +5117,7 @@ test_expect_failure '12n: Directory rename transitively makes rename back to sel\n> > > > grep \"CONFLICT (file location).*should perhaps be moved\" out &&\n> > > >\n> > > > # Should have 1 entry for hello, and 1 for world\n> > > > - test_stdout_line_count = 2 git ls-files -s &&\n> > > > + test_stdout_line_count = 3 git ls-files -s &&\n> > > > test_stdout_line_count = 1 git ls-files -s hello &&\n> > > > test_stdout_line_count = 2 git ls-files -s world\n> > > > )\n> > >\n> > > Should we also explicitly check `git ls-files -s baz`?\n> >\n> > Why? There was no baz in this testcase -- not only did it not appear\n> > in the final commit, it didn't appear in either branch being merged\n> > nor anywhere in the entire history of the repository. Testcases\n> > 12{i,j,k} all had such a file, but testcase 12n does not.\n>\n> Mostly because the line count was adjusted, so it seems clear to me that\n> \"baz\" at least plays a role here. Otherwise there's a mismatch between\n> the number of lines we see and the state of files we verify.\n\nOh, oops, the _previous_ patch should have had the change from 2 to 3,\nwhen it also introduced the expectation for 1 copy of hello and 2\ncopies of world (and marked the test as expecting to fail). The\ncomment on the line above should also be fixed.\n\n(However, this still has nothing to do with \"baz\"; there's no such\nfile at the toplevel or under any subdirectory -- in any commit in the\nrepository this testcase is running on -- so it's clear that \"baz\"\ncannot play any kind of role here. But thanks for flagging this\nchange -- it definitely got squashed into the wrong patch.)\n"},{"id":"523582","messageId":"CABPp-BFnA0QV0BuJOLij80d2JXJt1h9vvNm+oFuwW2Gahdtc4Q@mail.gmail.com","threadId":"63821","inReplyTo":"aJGK6XFdmYj50I9J@pks.im","subject":"Re: [PATCH 5/6] merge-ort: fix incorrect file handling","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-08-05T18:34:11Z","receivedAt":"2025-08-05T18:34:23Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Aug 4, 2025 at 9:39 PM Patrick Steinhardt wrote:\n>\n> On Mon, Aug 04, 2025 at 03:08:50PM -0700, Elijah Newren wrote:\n> > On Fri, Aug 1, 2025 at 1:31 AM Patrick Steinhardt wrote:\n> > > On Tue, Jul 22, 2025 at 03:23:10PM +0000, Elijah Newren via GitGitGadget wrote:\n> > > > Further, commit 98a1a00d5301 (t6423: add a testcase causing a failed\n> > > > assertion in process_renames, 2025-03-06), fixed an issue with\n> > > > rename-to-self but added a new testcase, 12n, that only checked for\n> > > > whether the merge ran to completion. A few commits ago, we modified\n> > > > this test to check for the number of entries in the index -- but noted\n> > > > that the number was wrong. And we also noted a\n> > > > silently-keep-instead-of-delete bug at the same time in the new testcase\n> > > > 12n2.\n> > > >\n> > > > Fix to merge-ort to prevent multiple bugs with rename-to-self cases:\n> > > > * silent deletion of file expected to be kept (t6423 testcase 12i2)\n> > > > * silent retention of file expected to be removed (t6423 testcase 12n2)\n> > > > * wrong number of extries left in the index (t6423 testcase 12n)\n> > >\n> > > I think it would have been nice to also go a bit more in depth for what\n> > > the bug actually was and how it's fixed. You do add a comment, but that\n> > > only adds a single sentence of context.\n> >\n> > Would something like this help:\n> >\n> > ...all of these issues come up because in a rename-to-self case, when\n> > we have a rename A->B, both A and B name the same file. The code in\n> > process_renames() assumes A & B are different, and tries to move the\n> > higher order stages and file contents so that they are associated just\n> > with the new path, but the assumptions of A & B being different can\n> > cause A to be deleted when it's not supposed to be or mark B as\n> > resolved and kept in place when it's supposed to be deleted. Since A\n> > & B are already the same path in the rename-to-self case, we can\n> > simply skip the steps in process_renames() for such files.\n>\n> Yes, it would!\n\nGreat, I'll include it in the next round, which I'll send out soon.\n\n> > > > Signed-off-by: Elijah Newren \n> > > > ---\n> > > > merge-ort.c | 11 +++++\n> > > > t/t6423-merge-rename-directories.sh | 69 +++++++++++++++++++++++++++--\n> > > > 2 files changed, 77 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/merge-ort.c b/merge-ort.c\n> > > > index 9b9d82ed10f7..feb06720c7e1 100644\n> > > > --- a/merge-ort.c\n> > > > +++ b/merge-ort.c\n> > > > @@ -2873,6 +2873,17 @@ static int process_renames(struct merge_options *opt,\n> > > > newinfo = new_ent->value;\n> > > > }\n> > > >\n> > > > + /*\n> > > > + * Directory renames can result in rename-to-self, which we\n> > > > + * want to skip so we don't mark oldpath for deletion.\n> > > > + *\n> > > > + * Note that we can avoid strcmp here because of prior\n> > > > + * diligence in apply_directory_rename_modifications() to\n> > > > + * ensure we reused existing paths from opt->priv->paths.\n> > > > + */\n> > > > + if (oldpath == newpath)\n> > > > + continue;\n> > >\n> > > Makes me wonder whether the additional brittleness is worth the saved\n> > > `strcmp()` comparison. But on the other hand we do have tests now that\n> > > would break if the memory allocation patterns ever changed, so that's\n> > > reassuring.\n> >\n> > There's no brittleness here; one of the many optimizations in\n> > merge-ort.c was to intern *all* pathnames in struct\n> > merge_options_internal's \"paths\" member; any code that needs to\n> > generate/compute a filename that may be part of the merge must check\n> > if that path already exists in opt->priv->paths, and if so use that\n> > pointer instead so that all filename comparisons can be done with\n> > cheap pointer comparisons. See the big comment near the top of\n> > merge_options_internal. Nearly all such\n> > string-equality-via-pointer-equality checks were introduced about the\n> > same time, and in other functions, which makes this one kind of an\n> > outlier. I figured whoever reviewed this patch or ran across this in\n> > the code might get surprised by the pointer comparison, so I tried to\n> > add a comment to address any questions. Looks like I wasn't thorough\n> > enough (and the first paragraph of the comment pre-dated my finding\n> > other bugs this patch fixed, which makes it slightly confusing), so\n> > I'll try to see if I can improve it for v2.\n>\n> I think the current version is good enough -- it feels brittle to me,\n> but I don't see a strong reason to change it.\n\nOkay. Note that if there was a strong reason to change it, the logic\nto do so would mean that the pointer-comparison-instead-of-strcmp\noptimization used on pathnames in several other places of merge-ort\nwould also need to change, because they are all based on the same\ninterning-of-pathname-strings to allow that optimization to be safe.\nI did notice that the merge_options_internal comment on the paths\nmember could perhaps be improved slightly; I'll include that too.\n"},{"id":"523596","messageId":"pull.1943.v2.git.1754422546.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.git.1753197791.gitgitgadget@gmail.com","subject":"[PATCH v2 0/6] Fix various rename corner cases","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-05T19:35:40Z","receivedAt":"2025-08-05T19:35:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Changes since v1; many thanks to Patrick for reviewing v1 in detail:\n\n * Several commit message cleanups\n * Make the test in patch 3 have a single expectation while documenting that\n a reasonable alternative exists, instead of trying to document that both\n are valid and having the test accept both outcomes\n * Moved a hunk accidentally squashed into patch 4 back to patch 3, and\n adjusted a nearby incorrect comment related to the squashed change\n * Fix style issues (grep -> test_grep, // -> /* */)\n\nOriginal cover letter:\n\nAt GitHub, we've got a real-world repository that has been triggering\nfailures of the form:\n\ngit: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.\n\n\nDigging in, this particular corner case requires multiple things to trigger:\n(1) a rename/delete of one file, and (2) a directory rename modifying an\nunrelated rename such that this unrelated rename's target becomes the source\nof the rename/delete from (1).\n\nUnfortunately, looking around, it's not the only bug in the area. Plus, some\nof our testcases in tangential situations were not checked closely enough or\nwere weird or buggy in various ways. Adding to the challenge was the fact\nthat the relevant renames optimization was sometimes triggering making\nrenames look like delete-and-add, and overlooking this meant I sometimes\nwasn't triggering what I thought I was triggering.\n\nThe combination of challenges sometimes made me think my fixes were breaking\nthings when sometimes I was just unaware of other bugs. I went in circles a\nfew times and took a rather non-linear path to finding and fixing these\nissues. While I think I've turned it into a nice linear progression of\npatches, I might be a bit too deep in the mud and it might not be as linear\nor clear as I think. Let me know and I'll try to clarify anything needed.\n\nElijah Newren (6):\n merge-ort: update comments to modern testfile location\n merge-ort: drop unnecessary temporary in check_for_directory_rename()\n t6423: document two bugs with rename-to-self testcases\n t6423: fix missed staging of file in testcases 12i,12j,12k\n merge-ort: fix incorrect file handling\n merge-ort: fix directory rename on top of source of other\n rename/delete\n\n merge-ort.c | 41 ++-\n t/t6423-merge-rename-directories.sh | 519 +++++++++++++++++++++++++++-\n 2 files changed, 533 insertions(+), 27 deletions(-)\n\n\nbase-commit: 16bd9f20a403117f2e0d9bcda6c6e621d3763e77\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1943%2Fnewren%2Ffix-rename-corner-cases-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1943/newren/fix-rename-corner-cases-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1943\n\nRange-diff vs v1:\n\n 1: dccc2044305 = 1: dccc2044305 merge-ort: update comments to modern testfile location\n 2: 58df0710efc = 2: 58df0710efc merge-ort: drop unnecessary temporary in check_for_directory_rename()\n 3: bda42aa85cf ! 3: 1ea7bfd3bff t6423: document two bugs with rename-to-self testcases\n @@ t/t6423-merge-rename-directories.sh: test_expect_success '12n: Directory rename\n \n \t\ttest_must_fail git cherry-pick A^0 >out &&\n -\t\tgrep \"CONFLICT (file location).*should perhaps be moved\" out\n -+\t\tgrep \"CONFLICT (file location).*should perhaps be moved\" out &&\n ++\t\ttest_grep \"CONFLICT (file location).*should perhaps be moved\" out &&\n +\n -+\t\t# Should have 1 entry for hello, and 1 for world\n -+\t\ttest_stdout_line_count = 2 git ls-files -s &&\n ++\t\t# Should have 1 entry for hello, and 2 for world\n ++\t\ttest_stdout_line_count = 3 git ls-files -s &&\n +\t\ttest_stdout_line_count = 1 git ls-files -s hello &&\n +\t\ttest_stdout_line_count = 2 git ls-files -s world\n +\t)\n @@ t/t6423-merge-rename-directories.sh: test_expect_success '12n: Directory rename\n +\n +\t\tgit checkout -q B^0 &&\n +\n ++\t\ttest_might_fail git -c merge.directoryRenames=true merge A^0 >out &&\n ++\n ++\t\t# Should have 1 entry for hello, and either 0 or 2 for world\n ++\t\t#\n +\t\t# NOTE: Since merge.directoryRenames=true, there is no path\n +\t\t# conflict for world vs. tools/world; it should end up at\n +\t\t# world. The fact that world was unmodified on side A, means\n @@ t/t6423-merge-rename-directories.sh: test_expect_success '12n: Directory rename\n +\t\t#\n +\t\t# However, rename-to-self-via-directory-rename is a bit more\n +\t\t# challenging. Relax this test to allow world to be treated\n -+\t\t# as a modify/delete conflict as well.\n -+\n -+\t\ttest_might_fail git -c merge.directoryRenames=true merge A^0 >out &&\n -+\n -+\t\t# Should have 1 entry for hello, and either 0 or 2 for world\n ++\t\t# as a modify/delete conflict as well, meaning it will have\n ++\t\t# two higher order stages, that just so happen to match.\n ++\t\t#\n +\t\ttest_stdout_line_count = 1 git ls-files -s hello &&\n -+\t\ttest_stdout_line_count != 1 git ls-files -s world &&\n -+\t\tif test_stdout_line_count != 0 git ls-files -s world\n -+\t\tthen\n -+\t\t\tgrep \"CONFLICT (modify/delete).*world deleted in HEAD\" out\n -+\t\tfi\n ++\t\ttest_stdout_line_count = 2 git ls-files -s world &&\n ++\t\ttest_grep \"CONFLICT (modify/delete).*world deleted in HEAD\" out\n \t)\n '\n \n 4: 3b3b258cec5 ! 4: 29b5e00c556 t6423: fix missed staging of file in testcases 12i,12j,12k\n @@ Commit message\n that isn't related to what is being tested and makes the test look more\n complicated than it is.\n \n - The cover letter for the series associated with the above commit noted\n + The cover letter for the series associated with the above commit (see\n + Message-ID: pull.1039.git.git.1624727121.gitgitgadget@gmail.com) noted\n that these testcases triggered two bugs in merge-recursive but only one\n in merge-ort; in merge-recursive these testcases also triggered a\n silent deletion of the file in question when it shouldn't be deleted.\n @@ t/t6423-merge-rename-directories.sh: test_expect_success '12k: Directory rename\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n -@@ t/t6423-merge-rename-directories.sh: test_expect_failure '12n: Directory rename transitively makes rename back to sel\n - \t\tgrep \"CONFLICT (file location).*should perhaps be moved\" out &&\n - \n - \t\t# Should have 1 entry for hello, and 1 for world\n --\t\ttest_stdout_line_count = 2 git ls-files -s &&\n -+\t\ttest_stdout_line_count = 3 git ls-files -s &&\n - \t\ttest_stdout_line_count = 1 git ls-files -s hello &&\n - \t\ttest_stdout_line_count = 2 git ls-files -s world\n - \t)\n 5: 2c7d4e022c5 ! 5: a8a7535fa5e merge-ort: fix incorrect file handling\n @@ Commit message\n as a rename), then we can trigger silent deletion of the file.\n \n Tweak testcase 12i slightly to make the file in question have more than\n - one line in it, but which doesn't change how it operates. Make this\n - change to otherwise minimize the changes between this testcase and a new\n - one that we want to add. Then duplicate the testcase as 12i2, changing\n - it so that it adds a single line to the file in question when it is\n - renamed, as a testcase for this bug.\n + one line in it. This leaves the testcase intact other than changing the\n + initial contents of this one file. The purpose of this tweak is to\n + minimize the changes between this testcase and a new one that we want to\n + add. Then duplicate testcase 12i as 12i2, changing it so that it adds a\n + single line to the file in question when it is renamed; testcase 12i2\n + then serves as a testcase for this merge-ort bug that I previously\n + overlooked.\n \n Further, commit 98a1a00d5301 (t6423: add a testcase causing a failed\n assertion in process_renames, 2025-03-06), fixed an issue with\n @@ Commit message\n silently-keep-instead-of-delete bug at the same time in the new testcase\n 12n2.\n \n - Fix to merge-ort to prevent multiple bugs with rename-to-self cases:\n + In summary, we have the following bugs with rename-to-self cases:\n * silent deletion of file expected to be kept (t6423 testcase 12i2)\n * silent retention of file expected to be removed (t6423 testcase 12n2)\n * wrong number of extries left in the index (t6423 testcase 12n)\n \n + All of these bugs arise because in a rename-to-self case, when we have a\n + rename A->B, both A and B name the same file. The code in\n + process_renames() assumes A & B are different, and tries to move the\n + higher order stages and file contents so that they are associated just\n + with the new path, but the assumptions of A & B being different can\n + cause A to be deleted when it's not supposed to be or mark B as resolved\n + and kept in place when it's supposed to be deleted. Since A & B are\n + already the same path in the rename-to-self case, simply skip the steps\n + in process_renames() for such files to fix these bugs.\n +\n Signed-off-by: Elijah Newren \n \n ## merge-ort.c ##\n 6: 0813d42d91f ! 6: 7238c8caf2b merge-ort: fix directory rename on top of source of other rename/delete\n @@ Commit message\n other renames, it might be helpful to use a simpler example with break\n detection first. And before we get to that it may help to explain\n some more basics of handling renames in the merge algorithm. So, let\n - me first backup and provide a quick refresher on on each of\n + me first backup and provide a quick refresher on each of\n \n * handling renames\n * what break detection would mean, if supported in merging\n @@ Commit message\n == Directory rename detection ==\n \n If one side of history renames directory D/ -> E/, and the other side of\n - history adds new files to E/, then directory rename detection notices\n + history adds new files to D/, then directory rename detection notices\n and suggests moving those new files to E/. A similar thing is done for\n paths renamed into D/, causing them to be transitively renamed into E/.\n \n @@ Commit message\n \n == Testcases 8+ ==\n \n - Another bonus bug, found via understanding our final solution (and the\n + Another bonus bug, found via understanding our final solutions (and the\n failure of our first attempted solution)!\n \n Let's tweak testcase 7 a bit:\n @@ merge-ort.c: static char *apply_dir_rename(struct strmap_entry *rename_info,\n \tINITIALIZE_CI(ci, mi);\n -\treturn mi->clean || (side_mask & (ci->filemask | ci->dirmask));\n +\treturn mi->clean || (side_mask & (ci->filemask | ci->dirmask))\n -+\t // See testcases 12n, 12p, 12q for more details on this next condition\n ++\t /* See testcases 12{n,p,q} for more details on this next condition */\n +\t\t\t || ((ci->filemask & 0x01) &&\n +\t\t\t strcmp(p->one->path, path));\n }\n\n-- \ngitgitgadget\n"},{"id":"523597","messageId":"dccc204430535f8c9eb74d7861365dbb2c42bb02.1754422546.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v2.git.1754422546.gitgitgadget@gmail.com","subject":"[PATCH v2 1/6] merge-ort: update comments to modern testfile location","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-05T19:35:41Z","receivedAt":"2025-08-05T19:35:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn commit 919df3195553 (Collect merge-related tests to t64xx,\n2020-08-10), merge related tests were moved from t60xx to t64xx. Some\ncomments in merge-ort relating to some tricky code referenced specific\ntestcases within certain testfiles for additional information, but\nreferred to their historical testfile names; update the testfile names\nto mention their modern location.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 47b3d1730ece..d87ba6dd42bf 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2163,7 +2163,7 @@ static int handle_content_merge(struct merge_options *opt,\n \t\t/*\n \t\t * FIXME: If opt->priv->call_depth && !clean, then we really\n \t\t * should not make result->mode match either a->mode or\n-\t\t * b->mode; that causes t6036 \"check conflicting mode for\n+\t\t * b->mode; that causes t6416 \"check conflicting mode for\n \t\t * regular file\" to fail. It would be best to use some other\n \t\t * mode, but we'll confuse all kinds of stuff if we use one\n \t\t * where S_ISREG(result->mode) isn't true, and if we use\n@@ -2520,7 +2520,7 @@ static void compute_collisions(struct strmap *collisions,\n \t * happening, and fall back to no-directory-rename detection\n \t * behavior for those paths.\n \t *\n-\t * See testcases 9e and all of section 5 from t6043 for examples.\n+\t * See testcases 9e and all of section 5 from t6423 for examples.\n \t */\n \tfor (i = 0; i < pairs->nr; ++i) {\n \t\tstruct strmap_entry *rename_info;\n@@ -2618,7 +2618,7 @@ static char *check_for_directory_rename(struct merge_options *opt,\n \t * That's why otherinfo and dir_rename_exclusions is here.\n \t *\n \t * As it turns out, this also prevents N-way transient rename\n-\t * confusion; See testcases 9c and 9d of t6043.\n+\t * confusion; See testcases 9c and 9d of t6423.\n \t */\n \tnew_dir = rename_info->value; /* old_dir = rename_info->key; */\n \totherinfo = strmap_get_entry(dir_rename_exclusions, new_dir);\n-- \ngitgitgadget\n\n"},{"id":"523598","messageId":"58df0710efc042b014a0c8282ce1d7fa62fbb760.1754422546.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v2.git.1754422546.gitgitgadget@gmail.com","subject":"[PATCH v2 2/6] merge-ort: drop unnecessary temporary in check_for_directory_rename()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-05T19:35:42Z","receivedAt":"2025-08-05T19:35:51Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\ncheck_for_directory_rename() had a weirdly coded check for whether a\nstrmap contained a certain key. Replace the temporary variable and call\nto strmap_get_entry() with the more natural strmap_contains() call.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex d87ba6dd42bf..9b9d82ed10f7 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2580,7 +2580,6 @@ static char *check_for_directory_rename(struct merge_options *opt,\n {\n \tchar *new_path;\n \tstruct strmap_entry *rename_info;\n-\tstruct strmap_entry *otherinfo;\n \tconst char *new_dir;\n \tint other_side = 3 - side_index;\n \n@@ -2615,14 +2614,13 @@ static char *check_for_directory_rename(struct merge_options *opt,\n \t * to not let Side1 do the rename to dumbdir, since we know that is\n \t * the source of one of our directory renames.\n \t *\n-\t * That's why otherinfo and dir_rename_exclusions is here.\n+\t * That's why dir_rename_exclusions is here.\n \t *\n \t * As it turns out, this also prevents N-way transient rename\n \t * confusion; See testcases 9c and 9d of t6423.\n \t */\n \tnew_dir = rename_info->value; /* old_dir = rename_info->key; */\n-\totherinfo = strmap_get_entry(dir_rename_exclusions, new_dir);\n-\tif (otherinfo) {\n+\tif (strmap_contains(dir_rename_exclusions, new_dir)) {\n \t\tpath_msg(opt, INFO_DIR_RENAME_SKIPPED_DUE_TO_RERENAME, 1,\n \t\t\t rename_info->key, path, new_dir, NULL,\n \t\t\t _(\"WARNING: Avoiding applying %s -> %s rename \"\n-- \ngitgitgadget\n\n"},{"id":"523599","messageId":"1ea7bfd3bff81a648a5ed1d0112ec404ab64fc91.1754422546.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v2.git.1754422546.gitgitgadget@gmail.com","subject":"[PATCH v2 3/6] t6423: document two bugs with rename-to-self testcases","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-05T19:35:43Z","receivedAt":"2025-08-05T19:35:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen commit 98a1a00d5301 (t6423: add a testcase causing a failed\nassertion in process_renames, 2025-03-06) was added, I tweaked\nthe commit message, and moved the test into t6423. However, that\nstill left two other things missing that made this test unlike the\nothers in the same testfile:\n\n * It didn't have an English description of the test setup like\n all other tests in t6423\n\n * It didn't check that the right number of files were present at\n the end\n\nThe former issue is a minor detail that isn't that critical, but the\nlatter feels more important. If it had been done, I might have noticed\nanother bug. In particular, this testcase involves\n Side A: rename world -> tools/world\nand\n Side B: rename tools/ -> \n Side B: remove world\nThe tools/ -> rename turns the world -> tools/world rename\ninto world -> world, i.e. a rename-to-self case. But, it's a path\nconflict because merge.directoryRenames defaults to false. There's\nno content conflict because Side A didn't modify world, so we should\njust take the content of world from Side B -- i.e. delete it. So, we\nhave a conflict on the path, but not on its content. We could consider\nletting the content trump since it is unconflicted, but if we are going\nto leave a conflict, it should certainly represent that 'world' existed\nboth in the base version and on Side A. Currently it doesn't.\n\nAdd a description of this test, add some checking of the number of\nentries in the index at the end of the merge, and mark the test as\nexpecting to fail for now. A subsequent commit will fix this bug.\n\nWhile at it, I found another related bug from a nearly identical setup\nbut setting merge.directoryRenames=true. Copy testcase 12n into 12n2,\nchanging it to use merge instead of cherry-pick, and turn on directory\nrenames for this test. In this case, since there is no content conflict\nand no path conflict, it should be okay to delete the file.\nUnfortunately, the code resolves without conflict but silently leaves\nworld despite the fact it should be deleted. It might also be okay if\nthe code spuriously thought there was a modify/delete conflict here;\nthat would at least notify users to look closer and then when they\nnotice there was no change since the base version, they can easily\nresolve. A conflict notice is much better than silently providing the\nwrong resolution. Cover this with the 12n2 testcase, which for now is\nmarked as expecting to fail as well.\n\nSigned-off-by: Elijah Newren \n---\n t/t6423-merge-rename-directories.sh | 100 +++++++++++++++++++++++++++-\n 1 file changed, 98 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex f48ed6d03534..2def1522bd59 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5056,6 +5056,25 @@ test_expect_success '12m: Change parent of renamed-dir to symlink on other side'\n \t)\n '\n \n+# Testcase 12n, Directory rename transitively makes rename back to self\n+#\n+# (Since this is a cherry-pick instead of merge, the labels are a bit weird.\n+# O, the original commit, is A~1 rather than what branch O points to.)\n+#\n+# Commit O: tools/hello\n+# world\n+# Commit A: tools/hello\n+# tools/world\n+# Commit B: hello\n+# In words:\n+# A: world -> tools/world\n+# B: tools/ -> /, i.e. rename all of tools to toplevel directory\n+# delete world\n+#\n+# Expected:\n+# CONFLICT (file location): tools/world vs. world\n+#\n+\n test_setup_12n () {\n \tgit init 12n &&\n \t(\n@@ -5084,7 +5103,7 @@ test_setup_12n () {\n \t)\n }\n \n-test_expect_success '12n: Directory rename transitively makes rename back to self' '\n+test_expect_failure '12n: Directory rename transitively makes rename back to self' '\n \ttest_setup_12n &&\n \t(\n \t\tcd 12n &&\n@@ -5092,7 +5111,84 @@ test_expect_success '12n: Directory rename transitively makes rename back to sel\n \t\tgit checkout -q B^0 &&\n \n \t\ttest_must_fail git cherry-pick A^0 >out &&\n-\t\tgrep \"CONFLICT (file location).*should perhaps be moved\" out\n+\t\ttest_grep \"CONFLICT (file location).*should perhaps be moved\" out &&\n+\n+\t\t# Should have 1 entry for hello, and 2 for world\n+\t\ttest_stdout_line_count = 3 git ls-files -s &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s hello &&\n+\t\ttest_stdout_line_count = 2 git ls-files -s world\n+\t)\n+'\n+\n+# Testcase 12n2, Directory rename transitively makes rename back to self\n+#\n+# Commit O: tools/hello\n+# world\n+# Commit A: tools/hello\n+# tools/world\n+# Commit B: hello\n+# In words:\n+# A: world -> tools/world\n+# B: tools/ -> /, i.e. rename all of tools to toplevel directory\n+# delete world\n+#\n+# Expected:\n+# CONFLICT (file location): tools/world vs. world\n+#\n+\n+test_setup_12n2 () {\n+\tgit init 12n2 &&\n+\t(\n+\t\tcd 12n2 &&\n+\n+\t\tmkdir tools &&\n+\t\techo hello >tools/hello &&\n+\t\tgit add tools/hello &&\n+\t\techo world >world &&\n+\t\tgit add world &&\n+\t\tgit commit -m \"O\" &&\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 world tools/world &&\n+\t\tgit commit -m \"Move world into tools/\" &&\n+\n+\t\tgit switch B &&\n+\t\tgit mv tools/hello hello &&\n+\t\tgit rm world &&\n+\t\tgit commit -m \"Move hello from tools/ to toplevel\"\n+\t)\n+}\n+\n+test_expect_failure '12n2: Directory rename transitively makes rename back to self' '\n+\ttest_setup_12n2 &&\n+\t(\n+\t\tcd 12n2 &&\n+\n+\t\tgit checkout -q B^0 &&\n+\n+\t\ttest_might_fail git -c merge.directoryRenames=true merge A^0 >out &&\n+\n+\t\t# Should have 1 entry for hello, and either 0 or 2 for world\n+\t\t#\n+\t\t# NOTE: Since merge.directoryRenames=true, there is no path\n+\t\t# conflict for world vs. tools/world; it should end up at\n+\t\t# world. The fact that world was unmodified on side A, means\n+\t\t# there was no content conflict; we should just take the\n+\t\t# content from side B -- i.e. delete the file. So merging\n+\t\t# could just delete world.\n+\t\t#\n+\t\t# However, rename-to-self-via-directory-rename is a bit more\n+\t\t# challenging. Relax this test to allow world to be treated\n+\t\t# as a modify/delete conflict as well, meaning it will have\n+\t\t# two higher order stages, that just so happen to match.\n+\t\t#\n+\t\ttest_stdout_line_count = 1 git ls-files -s hello &&\n+\t\ttest_stdout_line_count = 2 git ls-files -s world &&\n+\t\ttest_grep \"CONFLICT (modify/delete).*world deleted in HEAD\" out\n \t)\n '\n \n-- \ngitgitgadget\n\n"},{"id":"523600","messageId":"29b5e00c556a3d39fb9ca57bf3903f43280def5d.1754422546.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v2.git.1754422546.gitgitgadget@gmail.com","subject":"[PATCH v2 4/6] t6423: fix missed staging of file in testcases 12i,12j,12k","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-05T19:35:44Z","receivedAt":"2025-08-05T19:35:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nCommit 806f83287f8d (t6423: test directory renames causing\nrename-to-self, 2021-06-30) introduced testcase 12i-12k but omitted\nstaging one of the files and copy-pasted that mistake to the other\ntests. This means the merge runs with an unstaged change, even though\nthat isn't related to what is being tested and makes the test look more\ncomplicated than it is.\n\nThe cover letter for the series associated with the above commit (see\nMessage-ID: pull.1039.git.git.1624727121.gitgitgadget@gmail.com) noted\nthat these testcases triggered two bugs in merge-recursive but only one\nin merge-ort; in merge-recursive these testcases also triggered a\nsilent deletion of the file in question when it shouldn't be deleted.\nWhat I didn't realize at the time was that the deletion bug in merge-ort\nwas merely being sidestepped by the \"relevant renames\" optimization but\ncan actually be triggered. A subsequent commit will deal with that\nadditional bug, but it was complicated by the mistaken forgotten\nstaging, so this commit first fixes that issue.\n\nSigned-off-by: Elijah Newren \n---\n t/t6423-merge-rename-directories.sh | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 2def1522bd59..e1251b4e12ce 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -4747,6 +4747,7 @@ test_setup_12i () {\n \t\tgit switch B &&\n \t\tgit mv source/bar source/subdir/bar &&\n \t\techo more baz >>source/baz &&\n+\t\tgit add source/baz &&\n \t\tgit commit -m B\n \t)\n }\n@@ -4771,7 +4772,7 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\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\tM source/baz\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -4806,6 +4807,7 @@ test_setup_12j () {\n \t\tgit switch B &&\n \t\tgit mv bar subdir/bar &&\n \t\techo more baz >>baz &&\n+\t\tgit add baz &&\n \t\tgit commit -m B\n \t)\n }\n@@ -4830,7 +4832,7 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '\n \t\tgit status --porcelain -uno >actual &&\n \t\tcat >expect <<-\\EOF &&\n \t\tUU bar\n-\t\t M baz\n+\t\tM baz\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -4865,6 +4867,7 @@ test_setup_12k () {\n \t\tgit switch B &&\n \t\tgit mv dirA/bar dirB/bar &&\n \t\techo more baz >>dirA/baz &&\n+\t\tgit add dirA/baz &&\n \t\tgit commit -m B\n \t)\n }\n@@ -4889,7 +4892,7 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\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\tM dirA/baz\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n-- \ngitgitgadget\n\n"},{"id":"523601","messageId":"a8a7535fa5ed9c609c5f0f6db9ea135e2fb02a62.1754422546.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v2.git.1754422546.gitgitgadget@gmail.com","subject":"[PATCH v2 5/6] merge-ort: fix incorrect file handling","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-05T19:35:45Z","receivedAt":"2025-08-05T19:35:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWe have multiple bugs here -- accidental silent file deletion,\naccidental silent file retention for files that should be deleted,\nand incorrect number of entries left in the index.\n\nThe series merged at commit d3b88be1b450 (Merge branch\n'en/merge-dir-rename-corner-case-fix', 2021-07-16) introduced testcase\n12i-12k in t6423 which checked for rename-to-self cases, and fixed bugs\nthat merge-ort and merge-recursive had with these testcases. At the\ntime, I noted that merge-ort had one bug for these cases, while\nmerge-recursive had two. It turns out that merge-ort did in fact have\nanother bug, but the \"relevant renames\" optimizations were masking it.\nIf we modify testcase 12i from t6423 to modify the file in the commit\nthat renames it (but only modify it enough that it can still be detected\nas a rename), then we can trigger silent deletion of the file.\n\nTweak testcase 12i slightly to make the file in question have more than\none line in it. This leaves the testcase intact other than changing the\ninitial contents of this one file. The purpose of this tweak is to\nminimize the changes between this testcase and a new one that we want to\nadd. Then duplicate testcase 12i as 12i2, changing it so that it adds a\nsingle line to the file in question when it is renamed; testcase 12i2\nthen serves as a testcase for this merge-ort bug that I previously\noverlooked.\n\nFurther, commit 98a1a00d5301 (t6423: add a testcase causing a failed\nassertion in process_renames, 2025-03-06), fixed an issue with\nrename-to-self but added a new testcase, 12n, that only checked for\nwhether the merge ran to completion. A few commits ago, we modified\nthis test to check for the number of entries in the index -- but noted\nthat the number was wrong. And we also noted a\nsilently-keep-instead-of-delete bug at the same time in the new testcase\n12n2.\n\nIn summary, we have the following bugs with rename-to-self cases:\n * silent deletion of file expected to be kept (t6423 testcase 12i2)\n * silent retention of file expected to be removed (t6423 testcase 12n2)\n * wrong number of extries left in the index (t6423 testcase 12n)\n\nAll of these bugs arise because in a rename-to-self case, when we have a\nrename A->B, both A and B name the same file. The code in\nprocess_renames() assumes A & B are different, and tries to move the\nhigher order stages and file contents so that they are associated just\nwith the new path, but the assumptions of A & B being different can\ncause A to be deleted when it's not supposed to be or mark B as resolved\nand kept in place when it's supposed to be deleted. Since A & B are\nalready the same path in the rename-to-self case, simply skip the steps\nin process_renames() for such files to fix these bugs.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 11 +++++\n t/t6423-merge-rename-directories.sh | 69 +++++++++++++++++++++++++++--\n 2 files changed, 77 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 9b9d82ed10f7..feb06720c7e1 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2873,6 +2873,17 @@ static int process_renames(struct merge_options *opt,\n \t\t\tnewinfo = new_ent->value;\n \t\t}\n \n+\t\t/*\n+\t\t * Directory renames can result in rename-to-self, which we\n+\t\t * want to skip so we don't mark oldpath for deletion.\n+\t\t *\n+\t\t * Note that we can avoid strcmp here because of prior\n+\t\t * diligence in apply_directory_rename_modifications() to\n+\t\t * ensure we reused existing paths from opt->priv->paths.\n+\t\t */\n+\t\tif (oldpath == newpath)\n+\t\t\tcontinue;\n+\n \t\t/*\n \t\t * If pair->one->path isn't in opt->priv->paths, that means\n \t\t * that either directory rename detection removed that\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex e1251b4e12ce..49eb10392bed 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -4731,7 +4731,7 @@ test_setup_12i () {\n \n \t\tmkdir -p source/subdir &&\n \t\techo foo >source/subdir/foo &&\n-\t\techo bar >source/bar &&\n+\t\tprintf \"%d\\n\" 1 2 3 4 5 6 7 >source/bar &&\n \t\techo baz >source/baz &&\n \t\tgit add source &&\n \t\tgit commit -m orig &&\n@@ -4778,6 +4778,69 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\n \t)\n '\n \n+# Testcase 12i2, Identical to 12i except that source/subdir/bar modified on unrenamed side\n+# Commit O: source/{subdir/foo, bar, baz_1}\n+# Commit A: source/{foo, bar_2, 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_12i2 () {\n+\tgit init 12i2 &&\n+\t(\n+\t\tcd 12i2 &&\n+\n+\t\tmkdir -p source/subdir &&\n+\t\techo foo >source/subdir/foo &&\n+\t\tprintf \"%d\\n\" 1 2 3 4 5 6 7 >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\techo 8 >> source/bar &&\n+\t\tgit add source/bar &&\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 add source/baz &&\n+\t\tgit commit -m B\n+\t)\n+}\n+\n+test_expect_success '12i2: Directory rename causes rename-to-self' '\n+\ttest_setup_12i2 &&\n+\t(\n+\t\tcd 12i2 &&\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 >actual &&\n+\t\tuniq 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\tM 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@@ -5106,7 +5169,7 @@ test_setup_12n () {\n \t)\n }\n \n-test_expect_failure '12n: Directory rename transitively makes rename back to self' '\n+test_expect_success '12n: Directory rename transitively makes rename back to self' '\n \ttest_setup_12n &&\n \t(\n \t\tcd 12n &&\n@@ -5166,7 +5229,7 @@ test_setup_12n2 () {\n \t)\n }\n \n-test_expect_failure '12n2: Directory rename transitively makes rename back to self' '\n+test_expect_success '12n2: Directory rename transitively makes rename back to self' '\n \ttest_setup_12n2 &&\n \t(\n \t\tcd 12n2 &&\n-- \ngitgitgadget\n\n"},{"id":"523602","messageId":"7238c8caf2b3c8c2516a8939c7590566cdaabd03.1754422546.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v2.git.1754422546.gitgitgadget@gmail.com","subject":"[PATCH v2 6/6] merge-ort: fix directory rename on top of source of other rename/delete","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-05T19:35:46Z","receivedAt":"2025-08-05T19:35:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nAt GitHub, we've got a real-world repository that has been triggering\nfailures of the form:\n\n git: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.\n\nwhich comes from the line:\n\n VERIFY_CI(newinfo);\n\nUnfortunately, this one has been quite complex to unravel, and is a\nbit complex to explain. So, I'm going to carefully try to explain each\nrelevant piece needed to understand the fix, then carefully build up\nfrom a simple testcase to some of the relevant testcases.\n\n== New special case we need to consider ==\n\nRename pairs in the diffcore machinery connect the source path of a\nrename with the destination path of a rename. Since we have rename\npairs to consider on both sides of history since the merge base,\nmerging has to consider a few special cases of possible overlap:\n\n A) two rename pairs having the same target path\n B) two rename pairs having the same source path\n C) the source path of one rename pair being the target path of a\n different rename pair\n\nSome of these came up often enough that we gave them names:\n A) a rename/rename(2to1) conflict (looks similar to an add/add conflict)\n B) a rename/rename(1to2) conflict, which represents the same path being\n renamed differently on the two sides of history\n C) not yet named\n\nmerge-ort is well-prepared to handle cases (A) and (B), as was\nmerge-recursive (which was merge-ort's predecessor). Case (C) was\nbriefly considered during the years of merge-recursive maintenance,\nbut the full extent of support it got was a few FIXME/TODO comments\nlittered around the code highlighting some of the places that would\nprobably need to be fixed to support it. When I wrote merge-ort I\nignored case (C) entirely, since I believed that case (C) was only\npossible if we were to support break detection during merges. Not\nonly had break detection never been supported by any merge algorithm,\nI thought break detection wasn't worth the effort to support in a\nmerge algorithm. However, it turns out that case (C) can be triggered\nwithout break detection, if there's enough moving pieces.\n\nBefore I dive into how to trigger case (C) with directory renames plus\nother renames, it might be helpful to use a simpler example with break\ndetection first. And before we get to that it may help to explain\nsome more basics of handling renames in the merge algorithm. So, let\nme first backup and provide a quick refresher on each of\n\n * handling renames\n * what break detection would mean, if supported in merging\n * handling directory renames\n\nFrom there, I'll build up from a basic directory rename detection case\nto one that triggers a failure currently.\n\n== Handling renames ==\n\nIn the merge machinery when we have a rename of a path A -> B,\nprocessing that rename needs to remove path A, and make sure that path B\nhas the relevant information. Note that if the content was also\nmodified on both sides, this may mean that we have 3 different stages\nthat need to be stored at path B instead of having some stored at path\nA.\n\nHaving all stages stored at path B makes it much easier for users to\ninvestigate and resolve the content conflict associated with a renamed\npath. For example:\n * \"git status\" doesn't have to figure out how to list paths A & B and\n attempt to connect them for users; it can just list path B.\n * Users can use \"git ls-files -u B\" (instead of trying to find the\n previous name of the file so they can list both, i.e. \"git ls-files\n -u A B\")\n * Users can resolve via \"git add B\" (without needing to \"git rm A\")\n\n== What break detection would mean ==\n\nIf break detection were supported, we might have cases where A -> B\n*and* C -> A, meaning that both rename pairs might believe they need to\nupdate A. In particular, the processing of A -> B would need to be\ncareful to not clear out all stages of A and mark it resolved, while\nboth renames would need to figure out which stages of A belong with A\nand which belong with B, so that both paths have the right stages\nassociated with them.\n\nmerge-ort (like merge-recursive before it) makes no attempt to handle\nbreak detection; it runs with break detection turned off. It would\nneed to be retrofitted to handle such cases.\n\n== Directory rename detection ==\n\nIf one side of history renames directory D/ -> E/, and the other side of\nhistory adds new files to D/, then directory rename detection notices\nand suggests moving those new files to E/. A similar thing is done for\npaths renamed into D/, causing them to be transitively renamed into E/.\n\nThe default in the merge machinery is to report a conflict whenever a\ndirectory rename might modify the location of a path, so that users can\ndecide whether they wanted the original path or the\ndirectory-rename-induced location. However, that means the default\ncodepath still runs through all the directory rename detection logic, it\njust supplements it with providing conflict notices when it is done.\n\n== Building up increasingly complex testcases ==\n\nI'll start with a really simple directory rename example, and then\nslowly add twists that explain new pieces until we get to the\nproblematic cases:\n\n=== Testcase 1 ===\n\nLet's start with a concrete example, where particular files/directories of\ninterest that exist or are changed on each side are called out:\n\n Original: \n our side: rename B/file -> C/file\n their side: rename C/ -> A/\n\nFor this case, we'd expect to see the original B/file appear not at\nC/file but at A/file.\n\n(We would also expect a conflict notice that the user will want to\nchoose between C/file and A/file, but I'm going to ignore conflict\nnotices from here on by assuming merge.directoryRenames is set to\n`true` rather than `conflict`; the only difference that assumption\nmakes is whether that makes the merge be considered to be conflicted\nand whether it prints a conflict notice; what is written to the index\nor working directory is unchanged.)\n\n=== Testcase 2 ===\n\nModify testcase 1 by having A/file exist from the start:\n\n Original: A/file exists\n our side: rename B/file -> C/file\n their side: rename C/ -> A/\n\nIn such a case, to avoid user confusion at what looks kind of like an\nadd/add conflict (even though the original path at A/file was not added\nby either side of the merge), we turn off directory rename detection for\nthis path and print a \"in the way\" warning to the user:\n CONFLICT (implicit dir rename): Existing file/dir ... in the way ...\nThe testcases in section 5 of t6423 explore these in more detail.\n\n=== Testcase 3 ===\n\nLet's modify testcase 1 in a slightly different way: have A/file be\nadded by their side rather than it already existing.\n\n Original: \n our side: rename B/file -> C/file\n their side: rename C/ -> A/\n add A/file\n\nIn this case, the directory rename detection basically transforms our\nside's original B/file -> C/file into a B/file -> A/file, and so we\nget a rename/add conflict, with one version of A/file coming from the\nrenamed file, and another coming from the new A/file, each stored as\nstages 2 and 3 in conflicts. This kind of add/add conflict is perhaps\nslightly more complex than a regular add/add conflict, but with the\nprinted messages it makes sense where it came from and we have\ndifferent stages of the file to work with to resolve the conflict.\n\n=== Testcase 4 ===\n\nLet's do something similar to testcase 3, but have the opposite side of\nhistory add A/file:\n\n Original: \n our side: rename B/file -> C/file\n add A/file\n their side: rename C/ -> A/\n\nNow if we allow directory rename detection to modify C/file to A/file,\nthen we also get a rename/add conflict, but in this case we'd need both\nhigher order stages being recorded on side 2, which makes no sense. The\nindex can't store multiple stage 2 entries, and even if we could, it\nwould probably be confusing for users to work with. So, similar to what\nwe do when there was an A/file in the original version, we simply turn\noff directory rename detection for cases like this and provide the \"in\nthe way\" CONFLICT notice to the user.\n\n=== Testcase 5 ===\n\nWe're slowly getting closer. Let's mix it up by having A/file exist at\nthe beginning but not exist on their side:\n\n original: A/file exists\n our side: rename B/file -> C/file\n their side: rename C/ -> A/\n rename A/file -> D/file\n\nFor this case, you could say that since A/file -> D/file, it's no longer\nin the way of C/file being moved by directory rename detection to\nA/file. But that would give us a case where A/file is both the source\nand the target of a rename, similar to break detection, which the code\nisn't currently equipped to handle.\n\nThis is not yet the case that causes current failures; to the current\ncode, this kind of looks like testcase 4 in that A/file is in the way\non our side (since A/file was in the original and was umodified by our\nside). So, it results in a \"in the way\" notification with directory\nrename detection being turned off for A/file so that B/file ends up at\nC/file.\n\nPerhaps the resolution could be improved in the future, but our \"in\nthe way\" checks prevented such problems by noticing that A/file exists\non our side and thus turns off directory rename detection from\naffecting C/file's location. So, while the merge result could be\nperhaps improved, the fact that this is currently handled by giving\nthe user an \"in the way\" message gives the user a chance to resolve\nand prevents the code from tripping itself up.\n\n=== Testcase 6 ===\n\nLet's modify testcase 5 a bit more, to also delete A/file on our side:\n\n original: A/file exists\n our side: rename B/file -> C/file\n delete A/file\n their side: rename C/ -> A/\n rename A/file -> D/file\n\nNow the \"in the way\" logic doesn't detect that there's an A/file in\nthe way (neither side has an A/file anymore), so it's fine to\ntransitively rename C/file further to A/file...except that we end up\nwith A/file being both the source of one rename, and the target of a\ndifferent rename. Each rename pair tries to handle the resolution of\nthe source and target paths of its own rename. But when we go to\nprocess the second rename pair in process_renames(), we do not expect\neither the source or the destination to be marked as handled already;\nso, when we hit the sanity checks that these are not handled:\n\n VERIFY_CI(oldinfo);\n VERIFY_CI(newinfo);\n\nthen one of these is going to throw an assertion failure since the\nprevious rename pair already marked both of its paths as handled.\nThis will give us an error of the form:\n\n git: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.\n\nThis is the failure we're currently triggering, and it fundamentally\ndepends on:\n * a path existing in the original\n * that original path being removed or renamed on *both* sides\n * some kind of directory rename moving some *other* path into that\n original path\n\nThis was added as testcase 12q in t6423.\n\n=== Testcase 7 ===\n\nBonus bug found while investigating!\n\nLet's go back to the comparison between testcases 2 & 3, and set up a\nfile present on their side that we need to consider:\n\n Original: A/file exists\n our side: rename B/file -> C/file\n rename A/file -> D/file\n their side: rename C/ -> A/\n\nHere, there is no A/file in the way on our side like testcase 4.\nThere is an A/file present on their side like testcase 3, which was an\nadd/add conflict, but that's associated with the file be renamed to\nD/file. So, that really shouldn't be an add/add conflict because we\ninstead want all modes of the original A/file to be transported to\nD/file.\n\nUnfortunately, the current code kind of treats it like an add/add\nconflict instead...but even worse. There is also a valid mode for\nA/file in the original, which normally goes to stage 1. However, an\nadd/add conflict should be represented in the index with no mode at\nstage 1 (for the original side), only modes at stages 2 and 3 (for our\nand their side), so for an add/add we'd expect that mode for A/file in\nthe original version to be cleared out (or be transported to D/file).\n\nUnfortunately, the code currently leaves not only the stage 3 entry\nfor A/file intact, it also leaves the stage 1 entry for A/file. This\nresults in `git ls-files -u A/file` output of the form:\n\n 100644 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 1\tA/file\n 100644 0cfbf08886fca9a91cb753ec8734c84fcbe52c9f 2\tA/file\n 100644 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 3\tA/file\n\nThis would likely cause users to believe this isn't an add/add\nconflict; rather, this would lead them to believe that A/file was only\nmodified on our side and that therefore it should not have been a\nconflict in the first place. And while resolving the conflict in\nfavor of our side is the correct resolution (because stages 1 and 3\nshould have been cleared out in the first place), this is certainly\nlikely to cause confusion for anyone attempting to investigate why\nthis path was marked as conflicted.\n\nThis was added as testcase 12p in t6423.\n\n== Attempted solutions that I discarded ==\n\n1) For each side of history, create a strset of the sources of each\n rename on the other side of history. Then when using directory\n renames to modify existing renames, verify that we aren't renaming\n to a source of another rename.\n\n Unfortunately, the \"relevant renames\" optimization in merge-ort\n means we often don't detect renames -- we just see a delete and an\n add -- which is easy to forget and makes debugging testcases harder,\n but it also turns out that this solution in insufficient to solve\n the related problems in the area (more on that below).\n\n2) Modify the code to be aware of the possibility of renaming to\n the source of another side's rename, and make all the conflict\n resolution logic for each case (including existing\n rename/rename(2to1) and rename/rename(1to2) cases) handle the\n additional complexity. It turns out there was much more code to\n audit than I wanted, for a really niche case. I didn't like how\n many changes were needed, and aborted.\n\n== Solution ==\n\nWe do not want the stages of unrelated files appearing at the same path\nin the index except when dealing with an add/add conflict. While we\npreviously handled this for stages 2 & 3, we also need to worry about\nstage 1. So check for a stage 1 index entry being in the way of a\ndirectory rename.\n\nHowever, if we can detect that the stage 1 index entry is actually from\na related file due to a directory-rename-causes-rename-to-self\nsituation, then we can allow the stage 1 entry to remain.\n\nFrom this wording, you may note that it's not just rename cases that\nare a problem; bugs could be triggered with directory renames vs simple\nadds. That leads us to...\n\n== Testcases 8+ ==\n\nAnother bonus bug, found via understanding our final solutions (and the\nfailure of our first attempted solution)!\n\nLet's tweak testcase 7 a bit:\n\n Original: A/file exists\n our side: delete A/file\n add -> C/file\n their side: delete A/file\n rename C/ -> A/\n\nHere, there doesn't seem to be a big problem. Sure C/file gets modified\nvia the directory rename of C/ -> A/ so that it becomes A/file, but\nthere's no file in the way, right? Actually, here we have a problem\nthat the stage 1 entry of A/file would be combined with the stage 2\nentry of C/file, and make it look like a modify/delete conflict.\nPerhaps there is some extra checking that could be added to the code to\nmake it attempt to clear out the stage 1 entry of A/file, but the\nvarious rename-to-self-via-directory-rename testcases make that a bit\nmore difficult. For now, it's easier to just treat this as a\npath-in-the-way situation and not allow the directory rename to modify\nC/file.\n\nThat sounds all well and good, but it does have an interesting side\neffect. Due to the \"relevant renames\" optimizations in merge-ort (i.e.\nonly detect the renames you need), 100% renames whose files weren't\nmodified on the other side often go undetected. This means that if we\nmodify this testcase slightly to:\n\n Original: A/file exists\n our side: A/file -> C/file\n their side: rename C/ -> A/\n\nThen although this looks like where the directory rename just moves\nC/file back to A/file and there's no problem, we may not detect the\nA/file -> C/file rename. Instead it will look like a deletion of A/file\nand an addition of C/file. The directory rename then appears to be\nmoving C/file to A/file, which is on top of an \"unrelated\" file (or at\nleast a file it doesn't know is related). So, we will report\npath-in-the-way conflicts now in cases where we didn't before. That's\nbetter than silently and accidentally combining stages of unrelated\nfiles and making them look like a modify/delete; users can investigate\nthe reported conflict and simply resolve it.\n\nThis means we tweak the expected solution for testcases 12i, 12j, and\n12k. (Those three tests are basically the same test repeated three\ntimes, but I was worried when I added those that subtle differences in\nparent/child, sibling/sibling, and toplevel directories might mess up\nhow rename-to-self testcases actually get handled.)\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 18 +-\n t/t6423-merge-rename-directories.sh | 357 ++++++++++++++++++++++++++--\n 2 files changed, 355 insertions(+), 20 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex feb06720c7e1..8f6118163849 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2313,14 +2313,20 @@ static char *apply_dir_rename(struct strmap_entry *rename_info,\n \treturn strbuf_detach(&new_path, NULL);\n }\n \n-static int path_in_way(struct strmap *paths, const char *path, unsigned side_mask)\n+static int path_in_way(struct strmap *paths,\n+\t\t const char *path,\n+\t\t unsigned side_mask,\n+\t\t struct diff_filepair *p)\n {\n \tstruct merged_info *mi = strmap_get(paths, path);\n \tstruct conflict_info *ci;\n \tif (!mi)\n \t\treturn 0;\n \tINITIALIZE_CI(ci, mi);\n-\treturn mi->clean || (side_mask & (ci->filemask | ci->dirmask));\n+\treturn mi->clean || (side_mask & (ci->filemask | ci->dirmask))\n+\t /* See testcases 12{n,p,q} for more details on this next condition */\n+\t\t\t || ((ci->filemask & 0x01) &&\n+\t\t\t strcmp(p->one->path, path));\n }\n \n /*\n@@ -2332,6 +2338,7 @@ static int path_in_way(struct strmap *paths, const char *path, unsigned side_mas\n static char *handle_path_level_conflicts(struct merge_options *opt,\n \t\t\t\t\t const char *path,\n \t\t\t\t\t unsigned side_index,\n+\t\t\t\t\t struct diff_filepair *p,\n \t\t\t\t\t struct strmap_entry *rename_info,\n \t\t\t\t\t struct strmap *collisions)\n {\n@@ -2366,7 +2373,7 @@ static char *handle_path_level_conflicts(struct merge_options *opt,\n \t */\n \tif (c_info->reported_already) {\n \t\tclean = 0;\n-\t} else if (path_in_way(&opt->priv->paths, new_path, 1 << side_index)) {\n+\t} else if (path_in_way(&opt->priv->paths, new_path, 1 << side_index, p)) {\n \t\tc_info->reported_already = 1;\n \t\tstrbuf_add_separated_string_list(&collision_paths, \", \",\n \t\t\t\t\t\t &c_info->source_files);\n@@ -2573,6 +2580,7 @@ static void free_collisions(struct strmap *collisions)\n static char *check_for_directory_rename(struct merge_options *opt,\n \t\t\t\t\tconst char *path,\n \t\t\t\t\tunsigned side_index,\n+\t\t\t\t\tstruct diff_filepair *p,\n \t\t\t\t\tstruct strmap *dir_renames,\n \t\t\t\t\tstruct strmap *dir_rename_exclusions,\n \t\t\t\t\tstruct strmap *collisions,\n@@ -2629,7 +2637,7 @@ static char *check_for_directory_rename(struct merge_options *opt,\n \t\treturn NULL;\n \t}\n \n-\tnew_path = handle_path_level_conflicts(opt, path, side_index,\n+\tnew_path = handle_path_level_conflicts(opt, path, side_index, p,\n \t\t\t\t\t rename_info,\n \t\t\t\t\t &collisions[side_index]);\n \t*clean_merge &= (new_path != NULL);\n@@ -3428,7 +3436,7 @@ static int collect_renames(struct merge_options *opt,\n \t\t}\n \n \t\tnew_path = check_for_directory_rename(opt, p->two->path,\n-\t\t\t\t\t\t side_index,\n+\t\t\t\t\t\t side_index, p,\n \t\t\t\t\t\t dir_renames_for_side,\n \t\t\t\t\t\t rename_exclusions,\n \t\t\t\t\t\t collisions,\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 49eb10392bed..533ac85dc834 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -4759,10 +4759,29 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\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\t# NOTE: A potentially better resolution would be for\n+\t\t# source/bar -> source/subdir/bar\n+\t\t# to use the directory rename to become\n+\t\t# source/bar -> source/bar\n+\t\t# (a rename to self), and thus we end up with bar with\n+\t\t# a path conflict (given merge.directoryRenames=conflict).\n+\t\t# However, since the relevant renames optimization\n+\t\t# prevents us from noticing\n+\t\t# source/bar -> source/subdir/bar\n+\t\t# as a rename and looking at it just as\n+\t\t# delete source/bar\n+\t\t# add source/subdir/bar\n+\t\t# the directory rename of source/subdir/bar -> source/bar does\n+\t\t# not look like a rename-to-self situation but a\n+\t\t# rename-on-top-of-other-file situation. We do not want\n+\t\t# stage 1 entries from an unrelated file, so we expect an\n+\t\t# error about there being a file in the way.\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\n+\t\tgrep \"CONFLICT (implicit dir rename).*source/bar in the way\" out &&\n+\t\ttest_path_is_missing source/bar &&\n+\t\ttest_path_is_file source/subdir/bar &&\n \t\ttest_path_is_file source/baz &&\n \n \t\tgit ls-files >actual &&\n@@ -4771,8 +4790,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\n \n \t\tgit status --porcelain -uno >actual &&\n \t\tcat >expect <<-\\EOF &&\n-\t\tUU source/bar\n \t\tM source/baz\n+\t\tR source/bar -> source/subdir/bar\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -4882,10 +4901,29 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '\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\t# NOTE: A potentially better resolution would be for\n+\t\t# bar -> subdir/bar\n+\t\t# to use the directory rename to become\n+\t\t# bar -> bar\n+\t\t# (a rename to self), and thus we end up with bar with\n+\t\t# a path conflict (given merge.directoryRenames=conflict).\n+\t\t# However, since the relevant renames optimization\n+\t\t# prevents us from noticing\n+\t\t# bar -> subdir/bar\n+\t\t# as a rename and looking at it just as\n+\t\t# delete bar\n+\t\t# add subdir/bar\n+\t\t# the directory rename of subdir/bar -> bar does not look\n+\t\t# like a rename-to-self situation but a\n+\t\t# rename-on-top-of-other-file situation. We do not want\n+\t\t# stage 1 entries from an unrelated file, so we expect an\n+\t\t# error about there being a file in the way.\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\t\tgrep \"CONFLICT (implicit dir rename).*bar in the way\" out &&\n+\n+\t\ttest_path_is_missing bar &&\n+\t\ttest_path_is_file subdir/bar &&\n \t\ttest_path_is_file baz &&\n \n \t\tgit ls-files >actual &&\n@@ -4894,8 +4932,8 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '\n \n \t\tgit status --porcelain -uno >actual &&\n \t\tcat >expect <<-\\EOF &&\n-\t\tUU bar\n \t\tM baz\n+\t\tR bar -> subdir/bar\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -4942,10 +4980,29 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\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\t# NOTE: A potentially better resolution would be for\n+\t\t# dirA/bar -> dirB/bar\n+\t\t# to use the directory rename (dirB/ -> dirA/) to become\n+\t\t# dirA/bar -> dirA/bar\n+\t\t# (a rename to self), and thus we end up with bar with\n+\t\t# a path conflict (given merge.directoryRenames=conflict).\n+\t\t# However, since the relevant renames optimization\n+\t\t# prevents us from noticing\n+\t\t# dirA/bar -> dirB/bar\n+\t\t# as a rename and looking at it just as\n+\t\t# delete dirA/bar\n+\t\t# add dirB/bar\n+\t\t# the directory rename of dirA/bar -> dirB/bar does\n+\t\t# not look like a rename-to-self situation but a\n+\t\t# rename-on-top-of-other-file situation. We do not want\n+\t\t# stage 1 entries from an unrelated file, so we expect an\n+\t\t# error about there being a file in the way.\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\t\tgrep \"CONFLICT (implicit dir rename).*dirA/bar in the way\" out &&\n+\n+\t\ttest_path_is_missing dirA/bar &&\n+\t\ttest_path_is_file dirB/bar &&\n \t\ttest_path_is_file dirA/baz &&\n \n \t\tgit ls-files >actual &&\n@@ -4954,8 +5011,8 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\n \n \t\tgit status --porcelain -uno >actual &&\n \t\tcat >expect <<-\\EOF &&\n-\t\tUU dirA/bar\n \t\tM dirA/baz\n+\t\tR dirA/bar -> dirB/bar\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -5258,6 +5315,276 @@ test_expect_success '12n2: Directory rename transitively makes rename back to se\n \t)\n '\n \n+# Testcase 12o, Directory rename hits other rename source; file still in way on same side\n+# Commit O: A/file1_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# C/other\n+# Commit A: A/file1_1\n+# A/stuff\n+# B/stuff\n+# C/file1_2\n+# C/other\n+# Commit B: D/file2_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# A/other\n+# In words:\n+# A: rename B/file1_2 -> C/file1_2\n+# B: rename C/ -> A/\n+# rename A/file1_1 -> D/file2_1\n+# Rationale:\n+# A/stuff is unmodified, it shows up in final output\n+# B/stuff is unmodified, it shows up in final output\n+# C/other touched on one side (rename to A), so A/other shows up in output\n+# A/file1 is renamed to D/file2\n+# B/file1 -> C/file1 and even though C/ -> A/, A/file1 is\n+# \"in the way\" so we don't do the directory rename\n+# Expected: A/stuff\n+# B/stuff\n+# A/other\n+# D/file2\n+# C/file1\n+# + CONFLICT (implicit dir rename): A/file1 in way of C/file1\n+#\n+\n+test_setup_12o () {\n+\tgit init 12o &&\n+\t(\n+\t\tcd 12o &&\n+\n+\t\tmkdir -p A B C &&\n+\t\techo 1 >A/file1 &&\n+\t\techo 2 >B/file1 &&\n+\t\techo other >C/other &&\n+\t\techo Astuff >A/stuff &&\n+\t\techo Bstuff >B/stuff &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"O\" &&\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 B/file1 C/ &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit switch B &&\n+\t\tmkdir -p D &&\n+\t\tgit mv A/file1 D/file2 &&\n+\t\tgit mv C/other A/other &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"B\"\n+\t)\n+}\n+\n+test_expect_success '12o: Directory rename hits other rename source; file still in way on same side' '\n+\ttest_setup_12o &&\n+\t(\n+\t\tcd 12o &&\n+\n+\t\tgit checkout -q A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\n+\t\ttest_stdout_line_count = 5 git ls-files -s &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/other &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s B/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s D/file2 &&\n+\n+\t\tgrep \"CONFLICT (implicit dir rename).*Existing file/dir at A/file1 in the way\" out &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s C/file1\n+\t)\n+'\n+\n+# Testcase 12p, Directory rename hits other rename source; file still in way on other side\n+# Commit O: A/file1_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# C/other\n+# Commit A: D/file2_1\n+# A/stuff\n+# B/stuff\n+# C/file1_2\n+# C/other\n+# Commit B: A/file1_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# A/other\n+# Short version:\n+# A: rename A/file1_1 -> D/file2_1\n+# rename B/file1_2 -> C/file1_2\n+# B: Rename C/ -> A/\n+# Rationale:\n+# A/stuff is unmodified, it shows up in final output\n+# B/stuff is unmodified, it shows up in final output\n+# C/other touched on one side (rename to A), so A/other shows up in output\n+# A/file1 is renamed to D/file2\n+# B/file1 -> C/file1 and even though C/ -> A/, A/file1 is\n+# \"in the way\" so we don't do the directory rename\n+# Expected: A/stuff\n+# B/stuff\n+# A/other\n+# D/file2\n+# C/file1\n+# + CONFLICT (implicit dir rename): A/file1 in way of C/file1\n+#\n+\n+test_setup_12p () {\n+\tgit init 12p &&\n+\t(\n+\t\tcd 12p &&\n+\n+\t\tmkdir -p A B C &&\n+\t\techo 1 >A/file1 &&\n+\t\techo 2 >B/file1 &&\n+\t\techo other >C/other &&\n+\t\techo Astuff >A/stuff &&\n+\t\techo Bstuff >B/stuff &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit switch A &&\n+\t\tmkdir -p D &&\n+\t\tgit mv A/file1 D/file2 &&\n+\t\tgit mv B/file1 C/ &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit switch B &&\n+\t\tgit mv C/other A/other &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"B\"\n+\t)\n+}\n+\n+test_expect_success '12p: Directory rename hits other rename source; file still in way on other side' '\n+\ttest_setup_12p &&\n+\t(\n+\t\tcd 12p &&\n+\n+\t\tgit checkout -q A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\n+\t\ttest_stdout_line_count = 5 git ls-files -s &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/other &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s B/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s D/file2 &&\n+\n+\t\tgrep \"CONFLICT (implicit dir rename).*Existing file/dir at A/file1 in the way\" out &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s C/file1\n+\t)\n+'\n+\n+# Testcase 12q, Directory rename hits other rename source; file removed though\n+# Commit O: A/file1_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# C/other\n+# Commit A: A/stuff\n+# B/stuff\n+# C/file1_2\n+# C/other\n+# Commit B: D/file2_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# A/other\n+# In words:\n+# A: delete A/file1_1, rename B/file1_2 -> C/file1_2\n+# B: Rename C/ -> A/, rename A/file1_1 -> D/file2_1\n+# Rationale:\n+# A/stuff is unmodified, it shows up in final output\n+# B/stuff is unmodified, it shows up in final output\n+# C/other touched on one side (rename to A), so A/other shows up in output\n+# A/file1 is rename/delete to D/file2, so two stages for D/file2\n+# B/file1 -> C/file1 and even though C/ -> A/, A/file1 as a source was\n+# \"in the way\" (ish) so we don't do the directory rename\n+# Expected: A/stuff\n+# B/stuff\n+# A/other\n+# D/file2 (two stages)\n+# C/file1\n+# + CONFLICT (implicit dir rename): A/file1 in way of C/file1\n+# + CONFLICT (rename/delete): D/file2\n+#\n+\n+test_setup_12q () {\n+\tgit init 12q &&\n+\t(\n+\t\tcd 12q &&\n+\n+\t\tmkdir -p A B C &&\n+\t\techo 1 >A/file1 &&\n+\t\techo 2 >B/file1 &&\n+\t\techo other >C/other &&\n+\t\techo Astuff >A/stuff &&\n+\t\techo Bstuff >B/stuff &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"O\" &&\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 rm A/file1 &&\n+\t\tgit mv B/file1 C/ &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit switch B &&\n+\t\tmkdir -p D &&\n+\t\tgit mv A/file1 D/file2 &&\n+\t\tgit mv C/other A/other &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"B\"\n+\t)\n+}\n+\n+test_expect_success '12q: Directory rename hits other rename source; file removed though' '\n+\ttest_setup_12q &&\n+\t(\n+\t\tcd 12q &&\n+\n+\t\tgit checkout -q A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\n+\t\tgrep \"CONFLICT (rename/delete).*A/file1.*D/file2\" out &&\n+\t\tgrep \"CONFLICT (implicit dir rename).*Existing file/dir at A/file1 in the way\" out &&\n+\n+\t\ttest_stdout_line_count = 6 git ls-files -s &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/other &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s B/stuff &&\n+\t\ttest_stdout_line_count = 2 git ls-files -s D/file2 &&\n+\n+\t\t# This is a slightly suboptimal resolution; allowing the\n+\t\t# rename of C/ -> A/ to affect C/file1 and further rename\n+\t\t# it to A/file1 would probably be preferable, but since\n+\t\t# A/file1 existed as the source of another rename, allowing\n+\t\t# the dir rename of C/file1 -> A/file1 would mean modifying\n+\t\t# the code so that renames do not adjust both their source\n+\t\t# and target paths in all cases.\n+\t\t! grep \"CONFLICT (file location)\" out &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s C/file1\n+\t)\n+'\n \n ###########################################################################\n # SECTION 13: Checking informational and conflict messages\n-- \ngitgitgadget\n"},{"id":"523604","messageId":"xmqq1pppzfnr.fsf@gitster.g","threadId":"63821","inReplyTo":"7238c8caf2b3c8c2516a8939c7590566cdaabd03.1754422546.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 6/6] merge-ort: fix directory rename on top of source of other rename/delete","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-05T20:18:00Z","receivedAt":"2025-08-05T20:18:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> +\treturn mi->clean || (side_mask & (ci->filemask | ci->dirmask))\n> +\t /* See testcases 12{n,p,q} for more details on this next condition */\n> +\t\t\t || ((ci->filemask & 0x01) &&\n> +\t\t\t strcmp(p->one->path, path));\n\nAll the other references to testcase in this file tell the readers\nwhich file to look at, but except for this one.\n\n\t/* See testcases 12[npq] of t6423 */\n\nor something, probably.\n\n"},{"id":"523606","messageId":"CABPp-BEp9dH3Kidv=cHtzHnBSXyx-Vd3bkHBSC=O3MxjAMMTeQ@mail.gmail.com","threadId":"63821","inReplyTo":"xmqq1pppzfnr.fsf@gitster.g","subject":"Re: [PATCH v2 6/6] merge-ort: fix directory rename on top of source of other rename/delete","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-08-05T20:47:15Z","receivedAt":"2025-08-05T20:47:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 5, 2025 at 1:18 PM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > + return mi->clean || (side_mask & (ci->filemask | ci->dirmask))\n> > + /* See testcases 12{n,p,q} for more details on this next condition */\n> > + || ((ci->filemask & 0x01) &&\n> > + strcmp(p->one->path, path));\n>\n> All the other references to testcase in this file tell the readers\n> which file to look at, but except for this one.\n>\n> /* See testcases 12[npq] of t6423 */\n>\n> or something, probably.\n\nGood catch; will fix.\n"},{"id":"523683","messageId":"pull.1943.v3.git.1754522122.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v2.git.1754422546.gitgitgadget@gmail.com","subject":"[PATCH v3 0/7] Fix various rename corner cases","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-06T23:15:15Z","receivedAt":"2025-08-06T23:15:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Changes since v2:\n\n * Modify comment to note testfile as well as testcase name; thanks to Junio\n for spotting\n * Clarify comment about skipping rename-to-self cases in process_renames()\n in the second-to-last patch; I meant to do this as part of v2 but forgot.\n * Add a new commit which clarifies the comment in merge_options_internal\n about the interning of strings in the \"path\" member.\n\nChanges since v1; many thanks to Patrick for reviewing v1 in detail:\n\n * Several commit message cleanups\n * Make the test in patch 3 have a single expectation while documenting that\n a reasonable alternative exists, instead of trying to document that both\n are valid and having the test accept both outcomes\n * Moved a hunk accidentally squashed into patch 4 back to patch 3, and\n adjusted a nearby incorrect comment related to the squashed change\n * Fix style issues (grep -> test_grep, // -> /* */)\n\nOriginal cover letter:\n\nAt GitHub, we've got a real-world repository that has been triggering\nfailures of the form:\n\ngit: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.\n\n\nDigging in, this particular corner case requires multiple things to trigger:\n(1) a rename/delete of one file, and (2) a directory rename modifying an\nunrelated rename such that this unrelated rename's target becomes the source\nof the rename/delete from (1).\n\nUnfortunately, looking around, it's not the only bug in the area. Plus, some\nof our testcases in tangential situations were not checked closely enough or\nwere weird or buggy in various ways. Adding to the challenge was the fact\nthat the relevant renames optimization was sometimes triggering making\nrenames look like delete-and-add, and overlooking this meant I sometimes\nwasn't triggering what I thought I was triggering.\n\nThe combination of challenges sometimes made me think my fixes were breaking\nthings when sometimes I was just unaware of other bugs. I went in circles a\nfew times and took a rather non-linear path to finding and fixing these\nissues. While I think I've turned it into a nice linear progression of\npatches, I might be a bit too deep in the mud and it might not be as linear\nor clear as I think. Let me know and I'll try to clarify anything needed.\n\nElijah Newren (7):\n merge-ort: update comments to modern testfile location\n merge-ort: drop unnecessary temporary in check_for_directory_rename()\n t6423: document two bugs with rename-to-self testcases\n t6423: fix missed staging of file in testcases 12i,12j,12k\n merge-ort: clarify the interning of strings in opt->priv->path\n merge-ort: fix incorrect file handling\n merge-ort: fix directory rename on top of source of other\n rename/delete\n\n merge-ort.c | 55 ++-\n t/t6423-merge-rename-directories.sh | 519 +++++++++++++++++++++++++++-\n 2 files changed, 544 insertions(+), 30 deletions(-)\n\n\nbase-commit: 16bd9f20a403117f2e0d9bcda6c6e621d3763e77\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1943%2Fnewren%2Ffix-rename-corner-cases-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1943/newren/fix-rename-corner-cases-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1943\n\nRange-diff vs v2:\n\n 1: dccc2044305 = 1: dccc2044305 merge-ort: update comments to modern testfile location\n 2: 58df0710efc = 2: 58df0710efc merge-ort: drop unnecessary temporary in check_for_directory_rename()\n 3: 1ea7bfd3bff = 3: 1ea7bfd3bff t6423: document two bugs with rename-to-self testcases\n 4: 29b5e00c556 = 4: 29b5e00c556 t6423: fix missed staging of file in testcases 12i,12j,12k\n -: ----------- > 5: c4caba16c72 merge-ort: clarify the interning of strings in opt->priv->path\n 5: a8a7535fa5e ! 6: c00b6821e76 merge-ort: fix incorrect file handling\n @@ merge-ort.c: static int process_renames(struct merge_options *opt,\n \t\t}\n \n +\t\t/*\n -+\t\t * Directory renames can result in rename-to-self, which we\n -+\t\t * want to skip so we don't mark oldpath for deletion.\n ++\t\t * Directory renames can result in rename-to-self; the code\n ++\t\t * below assumes we have A->B with different A & B, and tries\n ++\t\t * to move all entries to path B. If A & B are the same path,\n ++\t\t * the logic can get confused, so skip further processing when\n ++\t\t * A & B are already the same path.\n +\t\t *\n -+\t\t * Note that we can avoid strcmp here because of prior\n -+\t\t * diligence in apply_directory_rename_modifications() to\n -+\t\t * ensure we reused existing paths from opt->priv->paths.\n ++\t\t * As a reminder, we can avoid strcmp here because all paths\n ++\t\t * are interned in opt->priv->paths; see the comment above\n ++\t\t * \"paths\" in struct merge_options_internal.\n +\t\t */\n +\t\tif (oldpath == newpath)\n +\t\t\tcontinue;\n 6: 7238c8caf2b ! 7: c2eef5ef5dc merge-ort: fix directory rename on top of source of other rename/delete\n @@ merge-ort.c: static char *apply_dir_rename(struct strmap_entry *rename_info,\n \tINITIALIZE_CI(ci, mi);\n -\treturn mi->clean || (side_mask & (ci->filemask | ci->dirmask));\n +\treturn mi->clean || (side_mask & (ci->filemask | ci->dirmask))\n -+\t /* See testcases 12{n,p,q} for more details on this next condition */\n ++\t /* See testcases 12[npq] of t6423 for this next condition */\n +\t\t\t || ((ci->filemask & 0x01) &&\n +\t\t\t strcmp(p->one->path, path));\n }\n\n-- \ngitgitgadget\n"},{"id":"523684","messageId":"dccc204430535f8c9eb74d7861365dbb2c42bb02.1754522123.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v3.git.1754522122.gitgitgadget@gmail.com","subject":"[PATCH v3 1/7] merge-ort: update comments to modern testfile location","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-06T23:15:16Z","receivedAt":"2025-08-06T23:15:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn commit 919df3195553 (Collect merge-related tests to t64xx,\n2020-08-10), merge related tests were moved from t60xx to t64xx. Some\ncomments in merge-ort relating to some tricky code referenced specific\ntestcases within certain testfiles for additional information, but\nreferred to their historical testfile names; update the testfile names\nto mention their modern location.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 47b3d1730ece..d87ba6dd42bf 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2163,7 +2163,7 @@ static int handle_content_merge(struct merge_options *opt,\n \t\t/*\n \t\t * FIXME: If opt->priv->call_depth && !clean, then we really\n \t\t * should not make result->mode match either a->mode or\n-\t\t * b->mode; that causes t6036 \"check conflicting mode for\n+\t\t * b->mode; that causes t6416 \"check conflicting mode for\n \t\t * regular file\" to fail. It would be best to use some other\n \t\t * mode, but we'll confuse all kinds of stuff if we use one\n \t\t * where S_ISREG(result->mode) isn't true, and if we use\n@@ -2520,7 +2520,7 @@ static void compute_collisions(struct strmap *collisions,\n \t * happening, and fall back to no-directory-rename detection\n \t * behavior for those paths.\n \t *\n-\t * See testcases 9e and all of section 5 from t6043 for examples.\n+\t * See testcases 9e and all of section 5 from t6423 for examples.\n \t */\n \tfor (i = 0; i < pairs->nr; ++i) {\n \t\tstruct strmap_entry *rename_info;\n@@ -2618,7 +2618,7 @@ static char *check_for_directory_rename(struct merge_options *opt,\n \t * That's why otherinfo and dir_rename_exclusions is here.\n \t *\n \t * As it turns out, this also prevents N-way transient rename\n-\t * confusion; See testcases 9c and 9d of t6043.\n+\t * confusion; See testcases 9c and 9d of t6423.\n \t */\n \tnew_dir = rename_info->value; /* old_dir = rename_info->key; */\n \totherinfo = strmap_get_entry(dir_rename_exclusions, new_dir);\n-- \ngitgitgadget\n\n"},{"id":"523685","messageId":"58df0710efc042b014a0c8282ce1d7fa62fbb760.1754522123.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v3.git.1754522122.gitgitgadget@gmail.com","subject":"[PATCH v3 2/7] merge-ort: drop unnecessary temporary in check_for_directory_rename()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-06T23:15:17Z","receivedAt":"2025-08-06T23:15:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\ncheck_for_directory_rename() had a weirdly coded check for whether a\nstrmap contained a certain key. Replace the temporary variable and call\nto strmap_get_entry() with the more natural strmap_contains() call.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex d87ba6dd42bf..9b9d82ed10f7 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2580,7 +2580,6 @@ static char *check_for_directory_rename(struct merge_options *opt,\n {\n \tchar *new_path;\n \tstruct strmap_entry *rename_info;\n-\tstruct strmap_entry *otherinfo;\n \tconst char *new_dir;\n \tint other_side = 3 - side_index;\n \n@@ -2615,14 +2614,13 @@ static char *check_for_directory_rename(struct merge_options *opt,\n \t * to not let Side1 do the rename to dumbdir, since we know that is\n \t * the source of one of our directory renames.\n \t *\n-\t * That's why otherinfo and dir_rename_exclusions is here.\n+\t * That's why dir_rename_exclusions is here.\n \t *\n \t * As it turns out, this also prevents N-way transient rename\n \t * confusion; See testcases 9c and 9d of t6423.\n \t */\n \tnew_dir = rename_info->value; /* old_dir = rename_info->key; */\n-\totherinfo = strmap_get_entry(dir_rename_exclusions, new_dir);\n-\tif (otherinfo) {\n+\tif (strmap_contains(dir_rename_exclusions, new_dir)) {\n \t\tpath_msg(opt, INFO_DIR_RENAME_SKIPPED_DUE_TO_RERENAME, 1,\n \t\t\t rename_info->key, path, new_dir, NULL,\n \t\t\t _(\"WARNING: Avoiding applying %s -> %s rename \"\n-- \ngitgitgadget\n\n"},{"id":"523686","messageId":"1ea7bfd3bff81a648a5ed1d0112ec404ab64fc91.1754522123.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v3.git.1754522122.gitgitgadget@gmail.com","subject":"[PATCH v3 3/7] t6423: document two bugs with rename-to-self testcases","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-06T23:15:18Z","receivedAt":"2025-08-06T23:15:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen commit 98a1a00d5301 (t6423: add a testcase causing a failed\nassertion in process_renames, 2025-03-06) was added, I tweaked\nthe commit message, and moved the test into t6423. However, that\nstill left two other things missing that made this test unlike the\nothers in the same testfile:\n\n * It didn't have an English description of the test setup like\n all other tests in t6423\n\n * It didn't check that the right number of files were present at\n the end\n\nThe former issue is a minor detail that isn't that critical, but the\nlatter feels more important. If it had been done, I might have noticed\nanother bug. In particular, this testcase involves\n Side A: rename world -> tools/world\nand\n Side B: rename tools/ -> \n Side B: remove world\nThe tools/ -> rename turns the world -> tools/world rename\ninto world -> world, i.e. a rename-to-self case. But, it's a path\nconflict because merge.directoryRenames defaults to false. There's\nno content conflict because Side A didn't modify world, so we should\njust take the content of world from Side B -- i.e. delete it. So, we\nhave a conflict on the path, but not on its content. We could consider\nletting the content trump since it is unconflicted, but if we are going\nto leave a conflict, it should certainly represent that 'world' existed\nboth in the base version and on Side A. Currently it doesn't.\n\nAdd a description of this test, add some checking of the number of\nentries in the index at the end of the merge, and mark the test as\nexpecting to fail for now. A subsequent commit will fix this bug.\n\nWhile at it, I found another related bug from a nearly identical setup\nbut setting merge.directoryRenames=true. Copy testcase 12n into 12n2,\nchanging it to use merge instead of cherry-pick, and turn on directory\nrenames for this test. In this case, since there is no content conflict\nand no path conflict, it should be okay to delete the file.\nUnfortunately, the code resolves without conflict but silently leaves\nworld despite the fact it should be deleted. It might also be okay if\nthe code spuriously thought there was a modify/delete conflict here;\nthat would at least notify users to look closer and then when they\nnotice there was no change since the base version, they can easily\nresolve. A conflict notice is much better than silently providing the\nwrong resolution. Cover this with the 12n2 testcase, which for now is\nmarked as expecting to fail as well.\n\nSigned-off-by: Elijah Newren \n---\n t/t6423-merge-rename-directories.sh | 100 +++++++++++++++++++++++++++-\n 1 file changed, 98 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex f48ed6d03534..2def1522bd59 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5056,6 +5056,25 @@ test_expect_success '12m: Change parent of renamed-dir to symlink on other side'\n \t)\n '\n \n+# Testcase 12n, Directory rename transitively makes rename back to self\n+#\n+# (Since this is a cherry-pick instead of merge, the labels are a bit weird.\n+# O, the original commit, is A~1 rather than what branch O points to.)\n+#\n+# Commit O: tools/hello\n+# world\n+# Commit A: tools/hello\n+# tools/world\n+# Commit B: hello\n+# In words:\n+# A: world -> tools/world\n+# B: tools/ -> /, i.e. rename all of tools to toplevel directory\n+# delete world\n+#\n+# Expected:\n+# CONFLICT (file location): tools/world vs. world\n+#\n+\n test_setup_12n () {\n \tgit init 12n &&\n \t(\n@@ -5084,7 +5103,7 @@ test_setup_12n () {\n \t)\n }\n \n-test_expect_success '12n: Directory rename transitively makes rename back to self' '\n+test_expect_failure '12n: Directory rename transitively makes rename back to self' '\n \ttest_setup_12n &&\n \t(\n \t\tcd 12n &&\n@@ -5092,7 +5111,84 @@ test_expect_success '12n: Directory rename transitively makes rename back to sel\n \t\tgit checkout -q B^0 &&\n \n \t\ttest_must_fail git cherry-pick A^0 >out &&\n-\t\tgrep \"CONFLICT (file location).*should perhaps be moved\" out\n+\t\ttest_grep \"CONFLICT (file location).*should perhaps be moved\" out &&\n+\n+\t\t# Should have 1 entry for hello, and 2 for world\n+\t\ttest_stdout_line_count = 3 git ls-files -s &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s hello &&\n+\t\ttest_stdout_line_count = 2 git ls-files -s world\n+\t)\n+'\n+\n+# Testcase 12n2, Directory rename transitively makes rename back to self\n+#\n+# Commit O: tools/hello\n+# world\n+# Commit A: tools/hello\n+# tools/world\n+# Commit B: hello\n+# In words:\n+# A: world -> tools/world\n+# B: tools/ -> /, i.e. rename all of tools to toplevel directory\n+# delete world\n+#\n+# Expected:\n+# CONFLICT (file location): tools/world vs. world\n+#\n+\n+test_setup_12n2 () {\n+\tgit init 12n2 &&\n+\t(\n+\t\tcd 12n2 &&\n+\n+\t\tmkdir tools &&\n+\t\techo hello >tools/hello &&\n+\t\tgit add tools/hello &&\n+\t\techo world >world &&\n+\t\tgit add world &&\n+\t\tgit commit -m \"O\" &&\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 world tools/world &&\n+\t\tgit commit -m \"Move world into tools/\" &&\n+\n+\t\tgit switch B &&\n+\t\tgit mv tools/hello hello &&\n+\t\tgit rm world &&\n+\t\tgit commit -m \"Move hello from tools/ to toplevel\"\n+\t)\n+}\n+\n+test_expect_failure '12n2: Directory rename transitively makes rename back to self' '\n+\ttest_setup_12n2 &&\n+\t(\n+\t\tcd 12n2 &&\n+\n+\t\tgit checkout -q B^0 &&\n+\n+\t\ttest_might_fail git -c merge.directoryRenames=true merge A^0 >out &&\n+\n+\t\t# Should have 1 entry for hello, and either 0 or 2 for world\n+\t\t#\n+\t\t# NOTE: Since merge.directoryRenames=true, there is no path\n+\t\t# conflict for world vs. tools/world; it should end up at\n+\t\t# world. The fact that world was unmodified on side A, means\n+\t\t# there was no content conflict; we should just take the\n+\t\t# content from side B -- i.e. delete the file. So merging\n+\t\t# could just delete world.\n+\t\t#\n+\t\t# However, rename-to-self-via-directory-rename is a bit more\n+\t\t# challenging. Relax this test to allow world to be treated\n+\t\t# as a modify/delete conflict as well, meaning it will have\n+\t\t# two higher order stages, that just so happen to match.\n+\t\t#\n+\t\ttest_stdout_line_count = 1 git ls-files -s hello &&\n+\t\ttest_stdout_line_count = 2 git ls-files -s world &&\n+\t\ttest_grep \"CONFLICT (modify/delete).*world deleted in HEAD\" out\n \t)\n '\n \n-- \ngitgitgadget\n\n"},{"id":"523687","messageId":"29b5e00c556a3d39fb9ca57bf3903f43280def5d.1754522123.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v3.git.1754522122.gitgitgadget@gmail.com","subject":"[PATCH v3 4/7] t6423: fix missed staging of file in testcases 12i,12j,12k","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-06T23:15:19Z","receivedAt":"2025-08-06T23:15:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nCommit 806f83287f8d (t6423: test directory renames causing\nrename-to-self, 2021-06-30) introduced testcase 12i-12k but omitted\nstaging one of the files and copy-pasted that mistake to the other\ntests. This means the merge runs with an unstaged change, even though\nthat isn't related to what is being tested and makes the test look more\ncomplicated than it is.\n\nThe cover letter for the series associated with the above commit (see\nMessage-ID: pull.1039.git.git.1624727121.gitgitgadget@gmail.com) noted\nthat these testcases triggered two bugs in merge-recursive but only one\nin merge-ort; in merge-recursive these testcases also triggered a\nsilent deletion of the file in question when it shouldn't be deleted.\nWhat I didn't realize at the time was that the deletion bug in merge-ort\nwas merely being sidestepped by the \"relevant renames\" optimization but\ncan actually be triggered. A subsequent commit will deal with that\nadditional bug, but it was complicated by the mistaken forgotten\nstaging, so this commit first fixes that issue.\n\nSigned-off-by: Elijah Newren \n---\n t/t6423-merge-rename-directories.sh | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 2def1522bd59..e1251b4e12ce 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -4747,6 +4747,7 @@ test_setup_12i () {\n \t\tgit switch B &&\n \t\tgit mv source/bar source/subdir/bar &&\n \t\techo more baz >>source/baz &&\n+\t\tgit add source/baz &&\n \t\tgit commit -m B\n \t)\n }\n@@ -4771,7 +4772,7 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\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\tM source/baz\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -4806,6 +4807,7 @@ test_setup_12j () {\n \t\tgit switch B &&\n \t\tgit mv bar subdir/bar &&\n \t\techo more baz >>baz &&\n+\t\tgit add baz &&\n \t\tgit commit -m B\n \t)\n }\n@@ -4830,7 +4832,7 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '\n \t\tgit status --porcelain -uno >actual &&\n \t\tcat >expect <<-\\EOF &&\n \t\tUU bar\n-\t\t M baz\n+\t\tM baz\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -4865,6 +4867,7 @@ test_setup_12k () {\n \t\tgit switch B &&\n \t\tgit mv dirA/bar dirB/bar &&\n \t\techo more baz >>dirA/baz &&\n+\t\tgit add dirA/baz &&\n \t\tgit commit -m B\n \t)\n }\n@@ -4889,7 +4892,7 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\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\tM dirA/baz\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n-- \ngitgitgadget\n\n"},{"id":"523688","messageId":"c4caba16c72636c7b726c6753815f7c5f77ecd1d.1754522123.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v3.git.1754522122.gitgitgadget@gmail.com","subject":"[PATCH v3 5/7] merge-ort: clarify the interning of strings in opt->priv->path","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-06T23:15:20Z","receivedAt":"2025-08-06T23:15:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nBecause merge-ort is dealing with potentially all the pathnames in the\nrepository, it sometimes needs to do an awful lot of string comparisons.\nBecause of this, struct merge_options_internal's path member was\nenvisioned from the beginning to contain an interned value for every\npath in order to allow us to compare strings via pointer comparison\ninstead of using strcmp. See\n * 5b59c3db059d (merge-ort: setup basic internal data structures,\n 2020-12-13)\n * f591c4724615 (merge-ort: copy and adapt merge_3way() from\n merge-recursive.c, 2021-01-01)\nfor some of the early comments.\n\nHowever, the original comment was slightly misleading when it switched\nfrom mentioning paths to only mentioning directories. Fix that, and\nwhile at it also point to an example in the code which applies the extra\nneeded care to permit the pointer comparison optimization.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 9b9d82ed10f7..325b19b182ad 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -316,9 +316,14 @@ struct merge_options_internal {\n \t * (e.g. \"drivers/firmware/raspberrypi.c\").\n \t * * store all relevant paths in the repo, both directories and\n \t * files (e.g. drivers, drivers/firmware would also be included)\n-\t * * these keys serve to intern all the path strings, which allows\n-\t * us to do pointer comparison on directory names instead of\n-\t * strcmp; we just have to be careful to use the interned strings.\n+\t * * these keys serve to intern *all* path strings, which allows us\n+\t * to do pointer comparisons on file & directory names instead of\n+\t * using strcmp; however, for this pointer-comparison optimization\n+\t * to work, any code path that independently computes a path needs\n+\t * to check for it existing in this strmap, and if so, point to\n+\t * the path in this strmap instead of their computed copy. See\n+\t * the \"reuse known pointer\" comment in\n+\t * apply_directory_rename_modifications() for an example.\n \t *\n \t * The values of paths:\n \t * * either a pointer to a merged_info, or a conflict_info struct\n-- \ngitgitgadget\n\n"},{"id":"523689","messageId":"c00b6821e760ad79a1bf73f3997fc7ab7b6d494a.1754522123.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v3.git.1754522122.gitgitgadget@gmail.com","subject":"[PATCH v3 6/7] merge-ort: fix incorrect file handling","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-06T23:15:21Z","receivedAt":"2025-08-06T23:15:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWe have multiple bugs here -- accidental silent file deletion,\naccidental silent file retention for files that should be deleted,\nand incorrect number of entries left in the index.\n\nThe series merged at commit d3b88be1b450 (Merge branch\n'en/merge-dir-rename-corner-case-fix', 2021-07-16) introduced testcase\n12i-12k in t6423 which checked for rename-to-self cases, and fixed bugs\nthat merge-ort and merge-recursive had with these testcases. At the\ntime, I noted that merge-ort had one bug for these cases, while\nmerge-recursive had two. It turns out that merge-ort did in fact have\nanother bug, but the \"relevant renames\" optimizations were masking it.\nIf we modify testcase 12i from t6423 to modify the file in the commit\nthat renames it (but only modify it enough that it can still be detected\nas a rename), then we can trigger silent deletion of the file.\n\nTweak testcase 12i slightly to make the file in question have more than\none line in it. This leaves the testcase intact other than changing the\ninitial contents of this one file. The purpose of this tweak is to\nminimize the changes between this testcase and a new one that we want to\nadd. Then duplicate testcase 12i as 12i2, changing it so that it adds a\nsingle line to the file in question when it is renamed; testcase 12i2\nthen serves as a testcase for this merge-ort bug that I previously\noverlooked.\n\nFurther, commit 98a1a00d5301 (t6423: add a testcase causing a failed\nassertion in process_renames, 2025-03-06), fixed an issue with\nrename-to-self but added a new testcase, 12n, that only checked for\nwhether the merge ran to completion. A few commits ago, we modified\nthis test to check for the number of entries in the index -- but noted\nthat the number was wrong. And we also noted a\nsilently-keep-instead-of-delete bug at the same time in the new testcase\n12n2.\n\nIn summary, we have the following bugs with rename-to-self cases:\n * silent deletion of file expected to be kept (t6423 testcase 12i2)\n * silent retention of file expected to be removed (t6423 testcase 12n2)\n * wrong number of extries left in the index (t6423 testcase 12n)\n\nAll of these bugs arise because in a rename-to-self case, when we have a\nrename A->B, both A and B name the same file. The code in\nprocess_renames() assumes A & B are different, and tries to move the\nhigher order stages and file contents so that they are associated just\nwith the new path, but the assumptions of A & B being different can\ncause A to be deleted when it's not supposed to be or mark B as resolved\nand kept in place when it's supposed to be deleted. Since A & B are\nalready the same path in the rename-to-self case, simply skip the steps\nin process_renames() for such files to fix these bugs.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 14 ++++++\n t/t6423-merge-rename-directories.sh | 69 +++++++++++++++++++++++++++--\n 2 files changed, 80 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 325b19b182ad..e7bdcf7b967e 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2878,6 +2878,20 @@ static int process_renames(struct merge_options *opt,\n \t\t\tnewinfo = new_ent->value;\n \t\t}\n \n+\t\t/*\n+\t\t * Directory renames can result in rename-to-self; the code\n+\t\t * below assumes we have A->B with different A & B, and tries\n+\t\t * to move all entries to path B. If A & B are the same path,\n+\t\t * the logic can get confused, so skip further processing when\n+\t\t * A & B are already the same path.\n+\t\t *\n+\t\t * As a reminder, we can avoid strcmp here because all paths\n+\t\t * are interned in opt->priv->paths; see the comment above\n+\t\t * \"paths\" in struct merge_options_internal.\n+\t\t */\n+\t\tif (oldpath == newpath)\n+\t\t\tcontinue;\n+\n \t\t/*\n \t\t * If pair->one->path isn't in opt->priv->paths, that means\n \t\t * that either directory rename detection removed that\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex e1251b4e12ce..49eb10392bed 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -4731,7 +4731,7 @@ test_setup_12i () {\n \n \t\tmkdir -p source/subdir &&\n \t\techo foo >source/subdir/foo &&\n-\t\techo bar >source/bar &&\n+\t\tprintf \"%d\\n\" 1 2 3 4 5 6 7 >source/bar &&\n \t\techo baz >source/baz &&\n \t\tgit add source &&\n \t\tgit commit -m orig &&\n@@ -4778,6 +4778,69 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\n \t)\n '\n \n+# Testcase 12i2, Identical to 12i except that source/subdir/bar modified on unrenamed side\n+# Commit O: source/{subdir/foo, bar, baz_1}\n+# Commit A: source/{foo, bar_2, 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_12i2 () {\n+\tgit init 12i2 &&\n+\t(\n+\t\tcd 12i2 &&\n+\n+\t\tmkdir -p source/subdir &&\n+\t\techo foo >source/subdir/foo &&\n+\t\tprintf \"%d\\n\" 1 2 3 4 5 6 7 >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\techo 8 >> source/bar &&\n+\t\tgit add source/bar &&\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 add source/baz &&\n+\t\tgit commit -m B\n+\t)\n+}\n+\n+test_expect_success '12i2: Directory rename causes rename-to-self' '\n+\ttest_setup_12i2 &&\n+\t(\n+\t\tcd 12i2 &&\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 >actual &&\n+\t\tuniq 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\tM 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@@ -5106,7 +5169,7 @@ test_setup_12n () {\n \t)\n }\n \n-test_expect_failure '12n: Directory rename transitively makes rename back to self' '\n+test_expect_success '12n: Directory rename transitively makes rename back to self' '\n \ttest_setup_12n &&\n \t(\n \t\tcd 12n &&\n@@ -5166,7 +5229,7 @@ test_setup_12n2 () {\n \t)\n }\n \n-test_expect_failure '12n2: Directory rename transitively makes rename back to self' '\n+test_expect_success '12n2: Directory rename transitively makes rename back to self' '\n \ttest_setup_12n2 &&\n \t(\n \t\tcd 12n2 &&\n-- \ngitgitgadget\n\n"},{"id":"523690","messageId":"c2eef5ef5dc5d030859fd3269e2382429515a6b9.1754522123.git.gitgitgadget@gmail.com","threadId":"63821","inReplyTo":"pull.1943.v3.git.1754522122.gitgitgadget@gmail.com","subject":"[PATCH v3 7/7] merge-ort: fix directory rename on top of source of other rename/delete","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-06T23:15:22Z","receivedAt":"2025-08-06T23:15:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nAt GitHub, we've got a real-world repository that has been triggering\nfailures of the form:\n\n git: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.\n\nwhich comes from the line:\n\n VERIFY_CI(newinfo);\n\nUnfortunately, this one has been quite complex to unravel, and is a\nbit complex to explain. So, I'm going to carefully try to explain each\nrelevant piece needed to understand the fix, then carefully build up\nfrom a simple testcase to some of the relevant testcases.\n\n== New special case we need to consider ==\n\nRename pairs in the diffcore machinery connect the source path of a\nrename with the destination path of a rename. Since we have rename\npairs to consider on both sides of history since the merge base,\nmerging has to consider a few special cases of possible overlap:\n\n A) two rename pairs having the same target path\n B) two rename pairs having the same source path\n C) the source path of one rename pair being the target path of a\n different rename pair\n\nSome of these came up often enough that we gave them names:\n A) a rename/rename(2to1) conflict (looks similar to an add/add conflict)\n B) a rename/rename(1to2) conflict, which represents the same path being\n renamed differently on the two sides of history\n C) not yet named\n\nmerge-ort is well-prepared to handle cases (A) and (B), as was\nmerge-recursive (which was merge-ort's predecessor). Case (C) was\nbriefly considered during the years of merge-recursive maintenance,\nbut the full extent of support it got was a few FIXME/TODO comments\nlittered around the code highlighting some of the places that would\nprobably need to be fixed to support it. When I wrote merge-ort I\nignored case (C) entirely, since I believed that case (C) was only\npossible if we were to support break detection during merges. Not\nonly had break detection never been supported by any merge algorithm,\nI thought break detection wasn't worth the effort to support in a\nmerge algorithm. However, it turns out that case (C) can be triggered\nwithout break detection, if there's enough moving pieces.\n\nBefore I dive into how to trigger case (C) with directory renames plus\nother renames, it might be helpful to use a simpler example with break\ndetection first. And before we get to that it may help to explain\nsome more basics of handling renames in the merge algorithm. So, let\nme first backup and provide a quick refresher on each of\n\n * handling renames\n * what break detection would mean, if supported in merging\n * handling directory renames\n\nFrom there, I'll build up from a basic directory rename detection case\nto one that triggers a failure currently.\n\n== Handling renames ==\n\nIn the merge machinery when we have a rename of a path A -> B,\nprocessing that rename needs to remove path A, and make sure that path B\nhas the relevant information. Note that if the content was also\nmodified on both sides, this may mean that we have 3 different stages\nthat need to be stored at path B instead of having some stored at path\nA.\n\nHaving all stages stored at path B makes it much easier for users to\ninvestigate and resolve the content conflict associated with a renamed\npath. For example:\n * \"git status\" doesn't have to figure out how to list paths A & B and\n attempt to connect them for users; it can just list path B.\n * Users can use \"git ls-files -u B\" (instead of trying to find the\n previous name of the file so they can list both, i.e. \"git ls-files\n -u A B\")\n * Users can resolve via \"git add B\" (without needing to \"git rm A\")\n\n== What break detection would mean ==\n\nIf break detection were supported, we might have cases where A -> B\n*and* C -> A, meaning that both rename pairs might believe they need to\nupdate A. In particular, the processing of A -> B would need to be\ncareful to not clear out all stages of A and mark it resolved, while\nboth renames would need to figure out which stages of A belong with A\nand which belong with B, so that both paths have the right stages\nassociated with them.\n\nmerge-ort (like merge-recursive before it) makes no attempt to handle\nbreak detection; it runs with break detection turned off. It would\nneed to be retrofitted to handle such cases.\n\n== Directory rename detection ==\n\nIf one side of history renames directory D/ -> E/, and the other side of\nhistory adds new files to D/, then directory rename detection notices\nand suggests moving those new files to E/. A similar thing is done for\npaths renamed into D/, causing them to be transitively renamed into E/.\n\nThe default in the merge machinery is to report a conflict whenever a\ndirectory rename might modify the location of a path, so that users can\ndecide whether they wanted the original path or the\ndirectory-rename-induced location. However, that means the default\ncodepath still runs through all the directory rename detection logic, it\njust supplements it with providing conflict notices when it is done.\n\n== Building up increasingly complex testcases ==\n\nI'll start with a really simple directory rename example, and then\nslowly add twists that explain new pieces until we get to the\nproblematic cases:\n\n=== Testcase 1 ===\n\nLet's start with a concrete example, where particular files/directories of\ninterest that exist or are changed on each side are called out:\n\n Original: \n our side: rename B/file -> C/file\n their side: rename C/ -> A/\n\nFor this case, we'd expect to see the original B/file appear not at\nC/file but at A/file.\n\n(We would also expect a conflict notice that the user will want to\nchoose between C/file and A/file, but I'm going to ignore conflict\nnotices from here on by assuming merge.directoryRenames is set to\n`true` rather than `conflict`; the only difference that assumption\nmakes is whether that makes the merge be considered to be conflicted\nand whether it prints a conflict notice; what is written to the index\nor working directory is unchanged.)\n\n=== Testcase 2 ===\n\nModify testcase 1 by having A/file exist from the start:\n\n Original: A/file exists\n our side: rename B/file -> C/file\n their side: rename C/ -> A/\n\nIn such a case, to avoid user confusion at what looks kind of like an\nadd/add conflict (even though the original path at A/file was not added\nby either side of the merge), we turn off directory rename detection for\nthis path and print a \"in the way\" warning to the user:\n CONFLICT (implicit dir rename): Existing file/dir ... in the way ...\nThe testcases in section 5 of t6423 explore these in more detail.\n\n=== Testcase 3 ===\n\nLet's modify testcase 1 in a slightly different way: have A/file be\nadded by their side rather than it already existing.\n\n Original: \n our side: rename B/file -> C/file\n their side: rename C/ -> A/\n add A/file\n\nIn this case, the directory rename detection basically transforms our\nside's original B/file -> C/file into a B/file -> A/file, and so we\nget a rename/add conflict, with one version of A/file coming from the\nrenamed file, and another coming from the new A/file, each stored as\nstages 2 and 3 in conflicts. This kind of add/add conflict is perhaps\nslightly more complex than a regular add/add conflict, but with the\nprinted messages it makes sense where it came from and we have\ndifferent stages of the file to work with to resolve the conflict.\n\n=== Testcase 4 ===\n\nLet's do something similar to testcase 3, but have the opposite side of\nhistory add A/file:\n\n Original: \n our side: rename B/file -> C/file\n add A/file\n their side: rename C/ -> A/\n\nNow if we allow directory rename detection to modify C/file to A/file,\nthen we also get a rename/add conflict, but in this case we'd need both\nhigher order stages being recorded on side 2, which makes no sense. The\nindex can't store multiple stage 2 entries, and even if we could, it\nwould probably be confusing for users to work with. So, similar to what\nwe do when there was an A/file in the original version, we simply turn\noff directory rename detection for cases like this and provide the \"in\nthe way\" CONFLICT notice to the user.\n\n=== Testcase 5 ===\n\nWe're slowly getting closer. Let's mix it up by having A/file exist at\nthe beginning but not exist on their side:\n\n original: A/file exists\n our side: rename B/file -> C/file\n their side: rename C/ -> A/\n rename A/file -> D/file\n\nFor this case, you could say that since A/file -> D/file, it's no longer\nin the way of C/file being moved by directory rename detection to\nA/file. But that would give us a case where A/file is both the source\nand the target of a rename, similar to break detection, which the code\nisn't currently equipped to handle.\n\nThis is not yet the case that causes current failures; to the current\ncode, this kind of looks like testcase 4 in that A/file is in the way\non our side (since A/file was in the original and was umodified by our\nside). So, it results in a \"in the way\" notification with directory\nrename detection being turned off for A/file so that B/file ends up at\nC/file.\n\nPerhaps the resolution could be improved in the future, but our \"in\nthe way\" checks prevented such problems by noticing that A/file exists\non our side and thus turns off directory rename detection from\naffecting C/file's location. So, while the merge result could be\nperhaps improved, the fact that this is currently handled by giving\nthe user an \"in the way\" message gives the user a chance to resolve\nand prevents the code from tripping itself up.\n\n=== Testcase 6 ===\n\nLet's modify testcase 5 a bit more, to also delete A/file on our side:\n\n original: A/file exists\n our side: rename B/file -> C/file\n delete A/file\n their side: rename C/ -> A/\n rename A/file -> D/file\n\nNow the \"in the way\" logic doesn't detect that there's an A/file in\nthe way (neither side has an A/file anymore), so it's fine to\ntransitively rename C/file further to A/file...except that we end up\nwith A/file being both the source of one rename, and the target of a\ndifferent rename. Each rename pair tries to handle the resolution of\nthe source and target paths of its own rename. But when we go to\nprocess the second rename pair in process_renames(), we do not expect\neither the source or the destination to be marked as handled already;\nso, when we hit the sanity checks that these are not handled:\n\n VERIFY_CI(oldinfo);\n VERIFY_CI(newinfo);\n\nthen one of these is going to throw an assertion failure since the\nprevious rename pair already marked both of its paths as handled.\nThis will give us an error of the form:\n\n git: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.\n\nThis is the failure we're currently triggering, and it fundamentally\ndepends on:\n * a path existing in the original\n * that original path being removed or renamed on *both* sides\n * some kind of directory rename moving some *other* path into that\n original path\n\nThis was added as testcase 12q in t6423.\n\n=== Testcase 7 ===\n\nBonus bug found while investigating!\n\nLet's go back to the comparison between testcases 2 & 3, and set up a\nfile present on their side that we need to consider:\n\n Original: A/file exists\n our side: rename B/file -> C/file\n rename A/file -> D/file\n their side: rename C/ -> A/\n\nHere, there is no A/file in the way on our side like testcase 4.\nThere is an A/file present on their side like testcase 3, which was an\nadd/add conflict, but that's associated with the file be renamed to\nD/file. So, that really shouldn't be an add/add conflict because we\ninstead want all modes of the original A/file to be transported to\nD/file.\n\nUnfortunately, the current code kind of treats it like an add/add\nconflict instead...but even worse. There is also a valid mode for\nA/file in the original, which normally goes to stage 1. However, an\nadd/add conflict should be represented in the index with no mode at\nstage 1 (for the original side), only modes at stages 2 and 3 (for our\nand their side), so for an add/add we'd expect that mode for A/file in\nthe original version to be cleared out (or be transported to D/file).\n\nUnfortunately, the code currently leaves not only the stage 3 entry\nfor A/file intact, it also leaves the stage 1 entry for A/file. This\nresults in `git ls-files -u A/file` output of the form:\n\n 100644 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 1\tA/file\n 100644 0cfbf08886fca9a91cb753ec8734c84fcbe52c9f 2\tA/file\n 100644 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 3\tA/file\n\nThis would likely cause users to believe this isn't an add/add\nconflict; rather, this would lead them to believe that A/file was only\nmodified on our side and that therefore it should not have been a\nconflict in the first place. And while resolving the conflict in\nfavor of our side is the correct resolution (because stages 1 and 3\nshould have been cleared out in the first place), this is certainly\nlikely to cause confusion for anyone attempting to investigate why\nthis path was marked as conflicted.\n\nThis was added as testcase 12p in t6423.\n\n== Attempted solutions that I discarded ==\n\n1) For each side of history, create a strset of the sources of each\n rename on the other side of history. Then when using directory\n renames to modify existing renames, verify that we aren't renaming\n to a source of another rename.\n\n Unfortunately, the \"relevant renames\" optimization in merge-ort\n means we often don't detect renames -- we just see a delete and an\n add -- which is easy to forget and makes debugging testcases harder,\n but it also turns out that this solution in insufficient to solve\n the related problems in the area (more on that below).\n\n2) Modify the code to be aware of the possibility of renaming to\n the source of another side's rename, and make all the conflict\n resolution logic for each case (including existing\n rename/rename(2to1) and rename/rename(1to2) cases) handle the\n additional complexity. It turns out there was much more code to\n audit than I wanted, for a really niche case. I didn't like how\n many changes were needed, and aborted.\n\n== Solution ==\n\nWe do not want the stages of unrelated files appearing at the same path\nin the index except when dealing with an add/add conflict. While we\npreviously handled this for stages 2 & 3, we also need to worry about\nstage 1. So check for a stage 1 index entry being in the way of a\ndirectory rename.\n\nHowever, if we can detect that the stage 1 index entry is actually from\na related file due to a directory-rename-causes-rename-to-self\nsituation, then we can allow the stage 1 entry to remain.\n\nFrom this wording, you may note that it's not just rename cases that\nare a problem; bugs could be triggered with directory renames vs simple\nadds. That leads us to...\n\n== Testcases 8+ ==\n\nAnother bonus bug, found via understanding our final solutions (and the\nfailure of our first attempted solution)!\n\nLet's tweak testcase 7 a bit:\n\n Original: A/file exists\n our side: delete A/file\n add -> C/file\n their side: delete A/file\n rename C/ -> A/\n\nHere, there doesn't seem to be a big problem. Sure C/file gets modified\nvia the directory rename of C/ -> A/ so that it becomes A/file, but\nthere's no file in the way, right? Actually, here we have a problem\nthat the stage 1 entry of A/file would be combined with the stage 2\nentry of C/file, and make it look like a modify/delete conflict.\nPerhaps there is some extra checking that could be added to the code to\nmake it attempt to clear out the stage 1 entry of A/file, but the\nvarious rename-to-self-via-directory-rename testcases make that a bit\nmore difficult. For now, it's easier to just treat this as a\npath-in-the-way situation and not allow the directory rename to modify\nC/file.\n\nThat sounds all well and good, but it does have an interesting side\neffect. Due to the \"relevant renames\" optimizations in merge-ort (i.e.\nonly detect the renames you need), 100% renames whose files weren't\nmodified on the other side often go undetected. This means that if we\nmodify this testcase slightly to:\n\n Original: A/file exists\n our side: A/file -> C/file\n their side: rename C/ -> A/\n\nThen although this looks like where the directory rename just moves\nC/file back to A/file and there's no problem, we may not detect the\nA/file -> C/file rename. Instead it will look like a deletion of A/file\nand an addition of C/file. The directory rename then appears to be\nmoving C/file to A/file, which is on top of an \"unrelated\" file (or at\nleast a file it doesn't know is related). So, we will report\npath-in-the-way conflicts now in cases where we didn't before. That's\nbetter than silently and accidentally combining stages of unrelated\nfiles and making them look like a modify/delete; users can investigate\nthe reported conflict and simply resolve it.\n\nThis means we tweak the expected solution for testcases 12i, 12j, and\n12k. (Those three tests are basically the same test repeated three\ntimes, but I was worried when I added those that subtle differences in\nparent/child, sibling/sibling, and toplevel directories might mess up\nhow rename-to-self testcases actually get handled.)\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 18 +-\n t/t6423-merge-rename-directories.sh | 357 ++++++++++++++++++++++++++--\n 2 files changed, 355 insertions(+), 20 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex e7bdcf7b967e..a0e294171575 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2318,14 +2318,20 @@ static char *apply_dir_rename(struct strmap_entry *rename_info,\n \treturn strbuf_detach(&new_path, NULL);\n }\n \n-static int path_in_way(struct strmap *paths, const char *path, unsigned side_mask)\n+static int path_in_way(struct strmap *paths,\n+\t\t const char *path,\n+\t\t unsigned side_mask,\n+\t\t struct diff_filepair *p)\n {\n \tstruct merged_info *mi = strmap_get(paths, path);\n \tstruct conflict_info *ci;\n \tif (!mi)\n \t\treturn 0;\n \tINITIALIZE_CI(ci, mi);\n-\treturn mi->clean || (side_mask & (ci->filemask | ci->dirmask));\n+\treturn mi->clean || (side_mask & (ci->filemask | ci->dirmask))\n+\t /* See testcases 12[npq] of t6423 for this next condition */\n+\t\t\t || ((ci->filemask & 0x01) &&\n+\t\t\t strcmp(p->one->path, path));\n }\n \n /*\n@@ -2337,6 +2343,7 @@ static int path_in_way(struct strmap *paths, const char *path, unsigned side_mas\n static char *handle_path_level_conflicts(struct merge_options *opt,\n \t\t\t\t\t const char *path,\n \t\t\t\t\t unsigned side_index,\n+\t\t\t\t\t struct diff_filepair *p,\n \t\t\t\t\t struct strmap_entry *rename_info,\n \t\t\t\t\t struct strmap *collisions)\n {\n@@ -2371,7 +2378,7 @@ static char *handle_path_level_conflicts(struct merge_options *opt,\n \t */\n \tif (c_info->reported_already) {\n \t\tclean = 0;\n-\t} else if (path_in_way(&opt->priv->paths, new_path, 1 << side_index)) {\n+\t} else if (path_in_way(&opt->priv->paths, new_path, 1 << side_index, p)) {\n \t\tc_info->reported_already = 1;\n \t\tstrbuf_add_separated_string_list(&collision_paths, \", \",\n \t\t\t\t\t\t &c_info->source_files);\n@@ -2578,6 +2585,7 @@ static void free_collisions(struct strmap *collisions)\n static char *check_for_directory_rename(struct merge_options *opt,\n \t\t\t\t\tconst char *path,\n \t\t\t\t\tunsigned side_index,\n+\t\t\t\t\tstruct diff_filepair *p,\n \t\t\t\t\tstruct strmap *dir_renames,\n \t\t\t\t\tstruct strmap *dir_rename_exclusions,\n \t\t\t\t\tstruct strmap *collisions,\n@@ -2634,7 +2642,7 @@ static char *check_for_directory_rename(struct merge_options *opt,\n \t\treturn NULL;\n \t}\n \n-\tnew_path = handle_path_level_conflicts(opt, path, side_index,\n+\tnew_path = handle_path_level_conflicts(opt, path, side_index, p,\n \t\t\t\t\t rename_info,\n \t\t\t\t\t &collisions[side_index]);\n \t*clean_merge &= (new_path != NULL);\n@@ -3436,7 +3444,7 @@ static int collect_renames(struct merge_options *opt,\n \t\t}\n \n \t\tnew_path = check_for_directory_rename(opt, p->two->path,\n-\t\t\t\t\t\t side_index,\n+\t\t\t\t\t\t side_index, p,\n \t\t\t\t\t\t dir_renames_for_side,\n \t\t\t\t\t\t rename_exclusions,\n \t\t\t\t\t\t collisions,\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 49eb10392bed..533ac85dc834 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -4759,10 +4759,29 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\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\t# NOTE: A potentially better resolution would be for\n+\t\t# source/bar -> source/subdir/bar\n+\t\t# to use the directory rename to become\n+\t\t# source/bar -> source/bar\n+\t\t# (a rename to self), and thus we end up with bar with\n+\t\t# a path conflict (given merge.directoryRenames=conflict).\n+\t\t# However, since the relevant renames optimization\n+\t\t# prevents us from noticing\n+\t\t# source/bar -> source/subdir/bar\n+\t\t# as a rename and looking at it just as\n+\t\t# delete source/bar\n+\t\t# add source/subdir/bar\n+\t\t# the directory rename of source/subdir/bar -> source/bar does\n+\t\t# not look like a rename-to-self situation but a\n+\t\t# rename-on-top-of-other-file situation. We do not want\n+\t\t# stage 1 entries from an unrelated file, so we expect an\n+\t\t# error about there being a file in the way.\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\n+\t\tgrep \"CONFLICT (implicit dir rename).*source/bar in the way\" out &&\n+\t\ttest_path_is_missing source/bar &&\n+\t\ttest_path_is_file source/subdir/bar &&\n \t\ttest_path_is_file source/baz &&\n \n \t\tgit ls-files >actual &&\n@@ -4771,8 +4790,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\n \n \t\tgit status --porcelain -uno >actual &&\n \t\tcat >expect <<-\\EOF &&\n-\t\tUU source/bar\n \t\tM source/baz\n+\t\tR source/bar -> source/subdir/bar\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -4882,10 +4901,29 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '\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\t# NOTE: A potentially better resolution would be for\n+\t\t# bar -> subdir/bar\n+\t\t# to use the directory rename to become\n+\t\t# bar -> bar\n+\t\t# (a rename to self), and thus we end up with bar with\n+\t\t# a path conflict (given merge.directoryRenames=conflict).\n+\t\t# However, since the relevant renames optimization\n+\t\t# prevents us from noticing\n+\t\t# bar -> subdir/bar\n+\t\t# as a rename and looking at it just as\n+\t\t# delete bar\n+\t\t# add subdir/bar\n+\t\t# the directory rename of subdir/bar -> bar does not look\n+\t\t# like a rename-to-self situation but a\n+\t\t# rename-on-top-of-other-file situation. We do not want\n+\t\t# stage 1 entries from an unrelated file, so we expect an\n+\t\t# error about there being a file in the way.\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\t\tgrep \"CONFLICT (implicit dir rename).*bar in the way\" out &&\n+\n+\t\ttest_path_is_missing bar &&\n+\t\ttest_path_is_file subdir/bar &&\n \t\ttest_path_is_file baz &&\n \n \t\tgit ls-files >actual &&\n@@ -4894,8 +4932,8 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '\n \n \t\tgit status --porcelain -uno >actual &&\n \t\tcat >expect <<-\\EOF &&\n-\t\tUU bar\n \t\tM baz\n+\t\tR bar -> subdir/bar\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -4942,10 +4980,29 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\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\t# NOTE: A potentially better resolution would be for\n+\t\t# dirA/bar -> dirB/bar\n+\t\t# to use the directory rename (dirB/ -> dirA/) to become\n+\t\t# dirA/bar -> dirA/bar\n+\t\t# (a rename to self), and thus we end up with bar with\n+\t\t# a path conflict (given merge.directoryRenames=conflict).\n+\t\t# However, since the relevant renames optimization\n+\t\t# prevents us from noticing\n+\t\t# dirA/bar -> dirB/bar\n+\t\t# as a rename and looking at it just as\n+\t\t# delete dirA/bar\n+\t\t# add dirB/bar\n+\t\t# the directory rename of dirA/bar -> dirB/bar does\n+\t\t# not look like a rename-to-self situation but a\n+\t\t# rename-on-top-of-other-file situation. We do not want\n+\t\t# stage 1 entries from an unrelated file, so we expect an\n+\t\t# error about there being a file in the way.\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\t\tgrep \"CONFLICT (implicit dir rename).*dirA/bar in the way\" out &&\n+\n+\t\ttest_path_is_missing dirA/bar &&\n+\t\ttest_path_is_file dirB/bar &&\n \t\ttest_path_is_file dirA/baz &&\n \n \t\tgit ls-files >actual &&\n@@ -4954,8 +5011,8 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\n \n \t\tgit status --porcelain -uno >actual &&\n \t\tcat >expect <<-\\EOF &&\n-\t\tUU dirA/bar\n \t\tM dirA/baz\n+\t\tR dirA/bar -> dirB/bar\n \t\tEOF\n \t\ttest_cmp expect actual\n \t)\n@@ -5258,6 +5315,276 @@ test_expect_success '12n2: Directory rename transitively makes rename back to se\n \t)\n '\n \n+# Testcase 12o, Directory rename hits other rename source; file still in way on same side\n+# Commit O: A/file1_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# C/other\n+# Commit A: A/file1_1\n+# A/stuff\n+# B/stuff\n+# C/file1_2\n+# C/other\n+# Commit B: D/file2_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# A/other\n+# In words:\n+# A: rename B/file1_2 -> C/file1_2\n+# B: rename C/ -> A/\n+# rename A/file1_1 -> D/file2_1\n+# Rationale:\n+# A/stuff is unmodified, it shows up in final output\n+# B/stuff is unmodified, it shows up in final output\n+# C/other touched on one side (rename to A), so A/other shows up in output\n+# A/file1 is renamed to D/file2\n+# B/file1 -> C/file1 and even though C/ -> A/, A/file1 is\n+# \"in the way\" so we don't do the directory rename\n+# Expected: A/stuff\n+# B/stuff\n+# A/other\n+# D/file2\n+# C/file1\n+# + CONFLICT (implicit dir rename): A/file1 in way of C/file1\n+#\n+\n+test_setup_12o () {\n+\tgit init 12o &&\n+\t(\n+\t\tcd 12o &&\n+\n+\t\tmkdir -p A B C &&\n+\t\techo 1 >A/file1 &&\n+\t\techo 2 >B/file1 &&\n+\t\techo other >C/other &&\n+\t\techo Astuff >A/stuff &&\n+\t\techo Bstuff >B/stuff &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"O\" &&\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 B/file1 C/ &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit switch B &&\n+\t\tmkdir -p D &&\n+\t\tgit mv A/file1 D/file2 &&\n+\t\tgit mv C/other A/other &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"B\"\n+\t)\n+}\n+\n+test_expect_success '12o: Directory rename hits other rename source; file still in way on same side' '\n+\ttest_setup_12o &&\n+\t(\n+\t\tcd 12o &&\n+\n+\t\tgit checkout -q A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\n+\t\ttest_stdout_line_count = 5 git ls-files -s &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/other &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s B/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s D/file2 &&\n+\n+\t\tgrep \"CONFLICT (implicit dir rename).*Existing file/dir at A/file1 in the way\" out &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s C/file1\n+\t)\n+'\n+\n+# Testcase 12p, Directory rename hits other rename source; file still in way on other side\n+# Commit O: A/file1_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# C/other\n+# Commit A: D/file2_1\n+# A/stuff\n+# B/stuff\n+# C/file1_2\n+# C/other\n+# Commit B: A/file1_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# A/other\n+# Short version:\n+# A: rename A/file1_1 -> D/file2_1\n+# rename B/file1_2 -> C/file1_2\n+# B: Rename C/ -> A/\n+# Rationale:\n+# A/stuff is unmodified, it shows up in final output\n+# B/stuff is unmodified, it shows up in final output\n+# C/other touched on one side (rename to A), so A/other shows up in output\n+# A/file1 is renamed to D/file2\n+# B/file1 -> C/file1 and even though C/ -> A/, A/file1 is\n+# \"in the way\" so we don't do the directory rename\n+# Expected: A/stuff\n+# B/stuff\n+# A/other\n+# D/file2\n+# C/file1\n+# + CONFLICT (implicit dir rename): A/file1 in way of C/file1\n+#\n+\n+test_setup_12p () {\n+\tgit init 12p &&\n+\t(\n+\t\tcd 12p &&\n+\n+\t\tmkdir -p A B C &&\n+\t\techo 1 >A/file1 &&\n+\t\techo 2 >B/file1 &&\n+\t\techo other >C/other &&\n+\t\techo Astuff >A/stuff &&\n+\t\techo Bstuff >B/stuff &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit switch A &&\n+\t\tmkdir -p D &&\n+\t\tgit mv A/file1 D/file2 &&\n+\t\tgit mv B/file1 C/ &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit switch B &&\n+\t\tgit mv C/other A/other &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"B\"\n+\t)\n+}\n+\n+test_expect_success '12p: Directory rename hits other rename source; file still in way on other side' '\n+\ttest_setup_12p &&\n+\t(\n+\t\tcd 12p &&\n+\n+\t\tgit checkout -q A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\n+\t\ttest_stdout_line_count = 5 git ls-files -s &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/other &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s B/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s D/file2 &&\n+\n+\t\tgrep \"CONFLICT (implicit dir rename).*Existing file/dir at A/file1 in the way\" out &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s C/file1\n+\t)\n+'\n+\n+# Testcase 12q, Directory rename hits other rename source; file removed though\n+# Commit O: A/file1_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# C/other\n+# Commit A: A/stuff\n+# B/stuff\n+# C/file1_2\n+# C/other\n+# Commit B: D/file2_1\n+# A/stuff\n+# B/file1_2\n+# B/stuff\n+# A/other\n+# In words:\n+# A: delete A/file1_1, rename B/file1_2 -> C/file1_2\n+# B: Rename C/ -> A/, rename A/file1_1 -> D/file2_1\n+# Rationale:\n+# A/stuff is unmodified, it shows up in final output\n+# B/stuff is unmodified, it shows up in final output\n+# C/other touched on one side (rename to A), so A/other shows up in output\n+# A/file1 is rename/delete to D/file2, so two stages for D/file2\n+# B/file1 -> C/file1 and even though C/ -> A/, A/file1 as a source was\n+# \"in the way\" (ish) so we don't do the directory rename\n+# Expected: A/stuff\n+# B/stuff\n+# A/other\n+# D/file2 (two stages)\n+# C/file1\n+# + CONFLICT (implicit dir rename): A/file1 in way of C/file1\n+# + CONFLICT (rename/delete): D/file2\n+#\n+\n+test_setup_12q () {\n+\tgit init 12q &&\n+\t(\n+\t\tcd 12q &&\n+\n+\t\tmkdir -p A B C &&\n+\t\techo 1 >A/file1 &&\n+\t\techo 2 >B/file1 &&\n+\t\techo other >C/other &&\n+\t\techo Astuff >A/stuff &&\n+\t\techo Bstuff >B/stuff &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"O\" &&\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 rm A/file1 &&\n+\t\tgit mv B/file1 C/ &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit switch B &&\n+\t\tmkdir -p D &&\n+\t\tgit mv A/file1 D/file2 &&\n+\t\tgit mv C/other A/other &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"B\"\n+\t)\n+}\n+\n+test_expect_success '12q: Directory rename hits other rename source; file removed though' '\n+\ttest_setup_12q &&\n+\t(\n+\t\tcd 12q &&\n+\n+\t\tgit checkout -q A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&\n+\n+\t\tgrep \"CONFLICT (rename/delete).*A/file1.*D/file2\" out &&\n+\t\tgrep \"CONFLICT (implicit dir rename).*Existing file/dir at A/file1 in the way\" out &&\n+\n+\t\ttest_stdout_line_count = 6 git ls-files -s &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/other &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s A/stuff &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s B/stuff &&\n+\t\ttest_stdout_line_count = 2 git ls-files -s D/file2 &&\n+\n+\t\t# This is a slightly suboptimal resolution; allowing the\n+\t\t# rename of C/ -> A/ to affect C/file1 and further rename\n+\t\t# it to A/file1 would probably be preferable, but since\n+\t\t# A/file1 existed as the source of another rename, allowing\n+\t\t# the dir rename of C/file1 -> A/file1 would mean modifying\n+\t\t# the code so that renames do not adjust both their source\n+\t\t# and target paths in all cases.\n+\t\t! grep \"CONFLICT (file location)\" out &&\n+\t\ttest_stdout_line_count = 1 git ls-files -s C/file1\n+\t)\n+'\n \n ###########################################################################\n # SECTION 13: Checking informational and conflict messages\n-- \ngitgitgadget\n"}]}