[PATCH] merge-ort: handle cached rename & trivial resolution interaction better

[PATCH] merge-ort: handle cached rename & trivial resolution interaction better

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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 <newren@gmail.com>
---
    merge-ort: handle cached rename & trivial resolution interaction better
    
    Longstanding bug discovered recently.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2095%2Fnewren%2Fbetter-cache-rename-trivial-resolution-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2095/newren/better-cache-rename-trivial-resolution-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2095

 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 00923ce3cd..544be9e466 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 15dd2d94b7..56ee968989 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

base-commit: e8955061076952cc5eab0300424fc48b601fe12d
-- 
gitgitgadget