{"thread":{"id":"65525","subject":"[PATCH] merge-ort: handle cached rename & trivial resolution interaction better","startedAt":"2026-04-20T22:30:19Z","lastAt":"2026-04-20T22:30:19Z","messageCount":1,"participants":["Elijah Newren via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"541996","messageId":"pull.2095.git.1776724214171.gitgitgadget@gmail.com","threadId":"65525","inReplyTo":null,"subject":"[PATCH] merge-ort: handle cached rename & trivial resolution interaction better","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-20T22:30:14Z","receivedAt":"2026-04-20T22:30:19Z","isPatch":true,"body":"From: Elijah Newren <newren@gmail.com>\n\nBack in commit a562d90a350d (merge-ort: fix failing merges in special\ncorner case, 2025-11-03), we hit a rename assertion due to a trivial\ndirectory resolution affecting the parent of a cached rename.  Since\nthe path didn't need to be considered, we side-stepped it with\n\n   if (!newinfo)\n     continue;\n\nin process_renames().  We have since run into a case in production\nwhere a trivial resolution of a file affects the direct target of a\ncached rename rather than a parent directory of it.  Add a testcase\ndemonstrating this additional case.\n\nNow, if we were to follow the lead of commit a562d90a350d, we could\nresolve this alternate case with an extra condition on the above if:\n\n   if (!newinfo || newinfo->merged.clean)\n     continue;\n\nHowever, if we had done that earlier, we would have made 979ee83e8a90\n(merge-ort: fix corner case recursive submodule/directory conflict\nhandling, 2025-12-29) harder to find and fix, and this particular\nposition for this condition isn't actually at the root of the issue\nbut downstream from it.\n\nInstead, let's rip out this if-check from a562d90a350d and put in an\nalternative that more directly addresses trivially resolved paths that\nhappen to be cached renames or parent directories thereof, which is a\nbetter fix for the original testcase and which also solves the newly\nadded testcase as well.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    merge-ort: handle cached rename & trivial resolution interaction better\n    \n    Longstanding bug discovered recently.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2095%2Fnewren%2Fbetter-cache-rename-trivial-resolution-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2095/newren/better-cache-rename-trivial-resolution-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2095\n\n merge-ort.c                              | 48 +++++++++----------\n t/t6429-merge-sequence-rename-caching.sh | 60 ++++++++++++++++++++++++\n 2 files changed, 82 insertions(+), 26 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 00923ce3cd..544be9e466 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2953,32 +2953,6 @@ static int process_renames(struct merge_options *opt,\n \t\tif (!oldinfo || oldinfo->merged.clean)\n \t\t\tcontinue;\n \n-\t\t/*\n-\t\t * Rename caching from a previous commit might give us an\n-\t\t * irrelevant rename for the current commit.\n-\t\t *\n-\t\t * Imagine:\n-\t\t *     foo/A -> bar/A\n-\t\t * was a cached rename for the upstream side from the\n-\t\t * previous commit (without the directories being renamed),\n-\t\t * but the next commit being replayed\n-\t\t *     * does NOT add or delete files\n-\t\t *     * does NOT have directory renames\n-\t\t *     * does NOT modify any files under bar/\n-\t\t *     * does NOT modify foo/A\n-\t\t *     * DOES modify other files under foo/ (otherwise the\n-\t\t *       !oldinfo check above would have already exited for\n-\t\t *       us)\n-\t\t * In such a case, our trivial directory resolution will\n-\t\t * have already merged bar/, and our attempt to process\n-\t\t * the cached\n-\t\t *     foo/A -> bar/A\n-\t\t * would be counterproductive, and lack the necessary\n-\t\t * information anyway.  Skip such renames.\n-\t\t */\n-\t\tif (!newinfo)\n-\t\t\tcontinue;\n-\n \t\t/*\n \t\t * diff_filepairs have copies of pathnames, thus we have to\n \t\t * use standard 'strcmp()' (negated) instead of '=='.\n@@ -3329,6 +3303,28 @@ static void use_cached_pairs(struct merge_options *opt,\n \t\tif (!new_name)\n \t\t\tnew_name = old_name;\n \n+\t\t/*\n+\t\t * If this is a rename and the target path is either\n+\t\t * absent from opt->priv->paths (because a parent\n+\t\t * directory was trivially resolved) or already cleanly\n+\t\t * resolved (e.g. all three sides agree on its content),\n+\t\t * the cached rename is irrelevant for this commit.\n+\t\t * Skip it here rather than in process_renames() to\n+\t\t * preserve VERIFY_CI(newinfo)'s ability to catch bugs\n+\t\t * for non-cached renames (see 979ee83e8a90 (merge-ort:\n+\t\t * fix corner case recursive submodule/directory conflict\n+\t\t * handling, 2025-12-29) for an example of a bug that\n+\t\t * assertion caught).  The rename remains in cached_pairs\n+\t\t * for use in subsequent commits.\n+\t\t */\n+\t\tif (entry->value) {\n+\t\t\tstruct merged_info *mi;\n+\n+\t\t\tmi = strmap_get(&opt->priv->paths, new_name);\n+\t\t\tif (!mi || mi->clean)\n+\t\t\t\tcontinue;\n+\t\t}\n+\n \t\t/*\n \t\t * cached_pairs has *copies* of old_name and new_name,\n \t\t * because it has to persist across merges.  Since\ndiff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh\nindex 15dd2d94b7..56ee968989 100755\n--- a/t/t6429-merge-sequence-rename-caching.sh\n+++ b/t/t6429-merge-sequence-rename-caching.sh\n@@ -846,4 +846,64 @@ test_expect_success 'rename a file, use it on first pick, but irrelevant on seco\n \t)\n '\n \n+#\n+# In the following testcase:\n+#   Base:     subdir/file_1\n+#   Upstream: file_1         (renamed from subdir/file)\n+#   Topic_1:  subdir/file_2  (modified subdir/file)\n+#   Topic_2:  subdir/file_2, file_2  (added another \"file\" with same contents)\n+#   Topic_3:  file_2         (deleted subdir/file)\n+#\n+#\n+# This testcase presents no problems for git traditionally, but the fact that\n+#    subdir/file -> file\n+# gets cached after the first pick presents a problem for the third commit\n+# to be replayed, because file has contents file_2 on all three sides and\n+# is thus trivially resolved early.  The point of renames is to allow us to\n+# three-way merge contents across multiple filenames, but if the target is\n+# already resolved, we risk throwing an assertion.  Verify that the code\n+# correctly drops the irrelevant rename in order to avoid hitting that\n+# assertion.\n+#\n+test_expect_success 'cached rename does not assert on trivially clean target' '\n+\tgit init cached-rename-trivially-clean-target &&\n+\t(\n+\t\tcd cached-rename-trivially-clean-target &&\n+\n+\t\tmkdir subdir &&\n+\t\tprintf \"%s\\n\" 1 2 3 >subdir/file &&\n+\t\tgit add subdir/file &&\n+\t\tgit commit -m orig &&\n+\n+\t\tgit branch upstream &&\n+\t\tgit branch topic &&\n+\n+\t\tgit switch upstream &&\n+\t\tgit mv subdir/file file &&\n+\t\tgit commit -m \"rename subdir/file to file\" &&\n+\n+\t\tgit switch topic &&\n+\n+\t\techo 4 >>subdir/file &&\n+\t\tgit add subdir/file &&\n+\t\tgit commit -m \"modify subdir/file\" &&\n+\n+\t\tcp subdir/file file &&\n+\t\tgit add file &&\n+\t\tgit commit -m \"copy subdir/file to file\" &&\n+\n+\t\tgit rm subdir/file &&\n+\t\tgit commit -m \"delete subdir/file\" &&\n+\n+\t\tgit switch upstream &&\n+\t\tgit replay --onto HEAD upstream..topic &&\n+\t\tgit checkout topic &&\n+\n+\t\tgit ls-files >tracked-files &&\n+\t\ttest_line_count = 1 tracked-files &&\n+\t\tprintf \"%s\\n\" 1 2 3 4 >expect &&\n+\t\ttest_cmp expect file\n+\t)\n+'\n+\n test_done\n\nbase-commit: e8955061076952cc5eab0300424fc48b601fe12d\n-- \ngitgitgadget\n"}]}