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 <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/merge-ort.c b/merge-ort.c
index 00923ce3cd749b4dfe379741117eac693c7d3bfc..544be9e466c9b55e12895e0da96dc535a3ac0947 100644 (file)
--- 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 15dd2d94b75f0af1742622a66660c5c2ca71830e..56ee96898982762ddf039886e8a3ffe027712fe2 100755 (executable)
--- 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