From bce385776fb1f7f368b41dcb12f9891b639ca3d3 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 25 Feb 2026 22:44:46 -0800 Subject: [PATCH] merge-ort: handle cached rename & trivial resolution interaction better Back in commit a562d90a350d (merge-ort: fix failing merges in special corner case, 2025-11-03), we hit a rename assertion due to a trivial directory resolution affecting the parent of a cached rename. Since the path didn't need to be considered, we side-stepped it with if (!newinfo) continue; in process_renames(). We have since run into a case in production where a trivial resolution of a file affects the direct target of a cached rename rather than a parent directory of it. Add a testcase demonstrating this additional case. Now, if we were to follow the lead of commit a562d90a350d, we could resolve this alternate case with an extra condition on the above if: if (!newinfo || newinfo->merged.clean) continue; However, if we had done that earlier, we would have made 979ee83e8a90 (merge-ort: fix corner case recursive submodule/directory conflict handling, 2025-12-29) harder to find and fix, and this particular position for this condition isn't actually at the root of the issue but downstream from it. Instead, let's rip out this if-check from a562d90a350d and put in an alternative that more directly addresses trivially resolved paths that happen to be cached renames or parent directories thereof, which is a better fix for the original testcase and which also solves the newly added testcase as well. Signed-off-by: Elijah Newren --- merge-ort.c | 48 +++++++++---------- t/t6429-merge-sequence-rename-caching.sh | 60 ++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 26 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index 00923ce3cd749b..544be9e466c9b5 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -2953,32 +2953,6 @@ static int process_renames(struct merge_options *opt, if (!oldinfo || oldinfo->merged.clean) continue; - /* - * Rename caching from a previous commit might give us an - * irrelevant rename for the current commit. - * - * Imagine: - * foo/A -> bar/A - * was a cached rename for the upstream side from the - * previous commit (without the directories being renamed), - * but the next commit being replayed - * * does NOT add or delete files - * * does NOT have directory renames - * * does NOT modify any files under bar/ - * * does NOT modify foo/A - * * DOES modify other files under foo/ (otherwise the - * !oldinfo check above would have already exited for - * us) - * In such a case, our trivial directory resolution will - * have already merged bar/, and our attempt to process - * the cached - * foo/A -> bar/A - * would be counterproductive, and lack the necessary - * information anyway. Skip such renames. - */ - if (!newinfo) - continue; - /* * diff_filepairs have copies of pathnames, thus we have to * use standard 'strcmp()' (negated) instead of '=='. @@ -3329,6 +3303,28 @@ static void use_cached_pairs(struct merge_options *opt, if (!new_name) new_name = old_name; + /* + * If this is a rename and the target path is either + * absent from opt->priv->paths (because a parent + * directory was trivially resolved) or already cleanly + * resolved (e.g. all three sides agree on its content), + * the cached rename is irrelevant for this commit. + * Skip it here rather than in process_renames() to + * preserve VERIFY_CI(newinfo)'s ability to catch bugs + * for non-cached renames (see 979ee83e8a90 (merge-ort: + * fix corner case recursive submodule/directory conflict + * handling, 2025-12-29) for an example of a bug that + * assertion caught). The rename remains in cached_pairs + * for use in subsequent commits. + */ + if (entry->value) { + struct merged_info *mi; + + mi = strmap_get(&opt->priv->paths, new_name); + if (!mi || mi->clean) + continue; + } + /* * cached_pairs has *copies* of old_name and new_name, * because it has to persist across merges. Since diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh index 15dd2d94b75f0a..56ee9689898276 100755 --- a/t/t6429-merge-sequence-rename-caching.sh +++ b/t/t6429-merge-sequence-rename-caching.sh @@ -846,4 +846,64 @@ test_expect_success 'rename a file, use it on first pick, but irrelevant on seco ) ' +# +# In the following testcase: +# Base: subdir/file_1 +# Upstream: file_1 (renamed from subdir/file) +# Topic_1: subdir/file_2 (modified subdir/file) +# Topic_2: subdir/file_2, file_2 (added another "file" with same contents) +# Topic_3: file_2 (deleted subdir/file) +# +# +# This testcase presents no problems for git traditionally, but the fact that +# subdir/file -> file +# gets cached after the first pick presents a problem for the third commit +# to be replayed, because file has contents file_2 on all three sides and +# is thus trivially resolved early. The point of renames is to allow us to +# three-way merge contents across multiple filenames, but if the target is +# already resolved, we risk throwing an assertion. Verify that the code +# correctly drops the irrelevant rename in order to avoid hitting that +# assertion. +# +test_expect_success 'cached rename does not assert on trivially clean target' ' + git init cached-rename-trivially-clean-target && + ( + cd cached-rename-trivially-clean-target && + + mkdir subdir && + printf "%s\n" 1 2 3 >subdir/file && + git add subdir/file && + git commit -m orig && + + git branch upstream && + git branch topic && + + git switch upstream && + git mv subdir/file file && + git commit -m "rename subdir/file to file" && + + git switch topic && + + echo 4 >>subdir/file && + git add subdir/file && + git commit -m "modify subdir/file" && + + cp subdir/file file && + git add file && + git commit -m "copy subdir/file to file" && + + git rm subdir/file && + git commit -m "delete subdir/file" && + + git switch upstream && + git replay --onto HEAD upstream..topic && + git checkout topic && + + git ls-files >tracked-files && + test_line_count = 1 tracked-files && + printf "%s\n" 1 2 3 4 >expect && + test_cmp expect file + ) +' + test_done