Skip to contentSkip to conversation

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

PATCH New Elijah Newren via GitGitGadget 1 participant 1 message Apr 20

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 merge-ort: handle cached rename & trivial resolution interaction better

From: Elijah Newren <[email protected]>

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.

---
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(-)
merge-ort.c +22 -26
index 00923ce3cd..544be9e466 100644
@@ -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
t/t6429-merge-sequence-rename-caching.sh +60 -0

64 lines not on this page · raw message

base-commit: e8955061076952cc5eab0300424fc48b601fe12d

Signature · 1 line
gitgitgadget