{"thread":{"id":"58675","subject":"[PATCH] merge-ort: fix bug with dir rename vs change dir to symlink","startedAt":"2022-10-22T19:04:18Z","lastAt":"2022-10-22T19:04:18Z","messageCount":1,"participants":["Elijah Newren via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"465572","messageId":"pull.1391.git.1666465450590.gitgitgadget@gmail.com","threadId":"58675","inReplyTo":null,"subject":"[PATCH] merge-ort: fix bug with dir rename vs change dir to symlink","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-22T19:04:10Z","receivedAt":"2022-10-22T19:04:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nWhen changing a directory to a symlink on one side of history, and\nrenaming the parent of that directory to a different directory name\non the other side, e.g. with this kind of setup:\n\n    Base commit: Has a file named dir/subdir/file\n    Side1:       Rename dir/ -> renamed-dir/\n    Side2:       delete dir/subdir/file, add dir/subdir as symlink\n\nThen merge-ort was running into an assertion failure:\n\n    git: merge-ort.c:2622: apply_directory_rename_modifications: Assertion `ci->dirmask == 0' failed\n\nmerge-recursive did not have as obvious an issue handling this case,\nlikely because we never fixed it to handle the case from commit\n902c521a35 (\"t6423: more involved directory rename test\", 2020-10-15)\nwhere we need to be careful about nested renames when a directory rename\noccurs (dir/ -> renamed-dir/ implies dir/subdir/ ->\nrenamed-dir/subdir/).  However, merge-recursive does have multiple\nproblems with this testcase:\n\n  * Incorrect stages for the file: merge-recursive omits the stage in\n    the index corresponding to the base stage, making `git status`\n    report \"added by us\" for renamed-dir/subdir/file instead of the\n    expected \"deleted by them\".\n\n  * Poor directory/file conflict handling: For the renamed-dir/subdir\n    symlink, instead of reporting a file/directory conflict as\n    expected, it reports \"Error: Refusing to lose untracked file at\n    renamed-dir/subdir\".  This is a lie because there is no untracked\n    file at that location.  It then does the normal suboptimal\n    merge-recursive thing of having the symlink be tracked in the index\n    at a location where it can't be written due to D/F conflicts\n    (namely, renamed-dir/subdir), but writes it to the working tree at\n    a different location as a new untracked file (namely,\n    renamed-dir/subdir~B^0)\n\nTechnically, these problems don't prevent the user from resolving the\nmerge if they can figure out to ignore the confusion, but because both\npieces of output are quite confusing I don't want to modify the test\nto claim the recursive also passes it even if it doesn't have the bug\nthat ort did.\n\nSo, fix the bug in ort by splitting the conflict_info for \"dir/subdir\"\ninto two, one for the directory part, one for the file (i.e. symlink)\npart, since the symlink is being renamed by directory rename detection.\nThe directory part is needed for proper nesting, since there are still\nconflict_info fields for files underneath it (though those are marked\nas is_null, they are still present until the entries are processed,\nand the entry processing wants every non-toplevel entry to have a\nparent directory).\n\nReported-by: Stefano Rivera <stefano@rivera.za.net>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    merge-ort: fix bug with dir rename vs change dir to symlink\n    \n    This fixes the bug reported over here:\n    https://lore.kernel.org/git/20220916124234.h57p67mwywzok23s@haydn.kardiogramm.net/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1391%2Fnewren%2Fdir-rename-vs-symlink-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1391/newren/dir-rename-vs-symlink-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1391\n\n merge-ort.c                         | 36 +++++++++++++++++--\n t/t6423-merge-rename-directories.sh | 56 +++++++++++++++++++++++++++++\n 2 files changed, 90 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 99dcee2db8a..d846960e130 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2619,8 +2619,40 @@ static void apply_directory_rename_modifications(struct merge_options *opt,\n \t}\n \n \tassert(ci->filemask == 2 || ci->filemask == 4);\n-\tassert(ci->dirmask == 0);\n-\tstrmap_remove(&opt->priv->paths, old_path, 0);\n+\tassert(ci->dirmask == 0 || ci->dirmask == 1);\n+\tif (ci->dirmask == 0)\n+\t\tstrmap_remove(&opt->priv->paths, old_path, 0);\n+\telse {\n+\t\t/*\n+\t\t * This file exists on one side, but we still had a directory\n+\t\t * at the old location that we can't remove until after\n+\t\t * processing all paths below it.  So, make a copy of ci in\n+\t\t * new_ci and only put the file information into it.\n+\t\t */\n+\t\tnew_ci = mem_pool_calloc(&opt->priv->pool, 1, sizeof(*new_ci));\n+\t\tmemcpy(new_ci, ci, sizeof(*ci));\n+\t\tassert(!new_ci->match_mask);\n+\t\tnew_ci->dirmask = 0;\n+\t\tnew_ci->stages[1].mode = 0;\n+\t\toidcpy(&new_ci->stages[1].oid, null_oid());\n+\n+\t\t/*\n+\t\t * Now that we have the file information in new_ci, make sure\n+\t\t * ci only has the directory information.\n+\t\t */\n+\t\tci->filemask = 0;\n+\t\tci->merged.clean = 1;\n+\t\tfor (i = MERGE_BASE; i <= MERGE_SIDE2; i++) {\n+\t\t\tif (ci->dirmask & (1 << i))\n+\t\t\t\tcontinue;\n+\t\t\t/* zero out any entries related to files */\n+\t\t\tci->stages[i].mode = 0;\n+\t\t\toidcpy(&ci->stages[i].oid, null_oid());\n+\t\t}\n+\n+\t\t// Now we want to focus on new_ci, so reassign ci to it\n+\t\tci = new_ci;\n+\t}\n \n \tbranch_with_new_path   = (ci->filemask == 2) ? opt->branch1 : opt->branch2;\n \tbranch_with_dir_rename = (ci->filemask == 2) ? opt->branch2 : opt->branch1;\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex a4941878fe2..944de75b805 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5304,6 +5304,62 @@ test_expect_merge_algorithm failure success '12l (A into B): Rename into each ot\n \t)\n '\n \n+# Testcase 12m, Directory rename, plus change of parent dir to symlink\n+#   Commit O:  dir/subdir/file\n+#   Commit A:  renamed-dir/subdir/file\n+#   Commit B:  dir/subdir\n+#   In words:\n+#     A: dir/subdir/ -> renamed-dir/subdir\n+#     B: delete dir/subdir/file, add dir/subdir as symlink\n+#\n+#   Expected: CONFLICT (rename/delete): renamed-dir/subdir/file,\n+#             CONFLICT (file location): renamed-dir/subdir vs. dir/subdir\n+#             CONFLICT (directory/file): renamed-dir/subdir symlink has\n+#                                        renamed-dir/subdir in the way\n+\n+test_setup_12m () {\n+\tgit init 12m &&\n+\t(\n+\t\tcd 12m &&\n+\n+\t\tmkdir -p dir/subdir &&\n+\t\techo 1 >dir/subdir/file &&\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 dir/ renamed-dir/ &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit switch B &&\n+\t\tgit rm dir/subdir/file &&\n+\t\tmkdir dir &&\n+\t\tln -s /dev/null dir/subdir &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"B\"\n+\t)\n+}\n+\n+test_expect_merge_algorithm failure success '12m: Change parent of renamed-dir to symlink on other side' '\n+\ttest_setup_12m &&\n+\t(\n+\t\tcd 12m &&\n+\n+\t\tgit checkout -q A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&\n+\n+\t\ttest_stdout_line_count = 3 git ls-files -s &&\n+\t\ttest_stdout_line_count = 2 ls -1 renamed-dir &&\n+\t\ttest_path_is_missing dir\n+\t)\n+'\n+\n ###########################################################################\n # SECTION 13: Checking informational and conflict messages\n #\n\nbase-commit: 79f2338b3746d23454308648b2491e5beba4beff\n-- \ngitgitgadget\n"}]}