@matheusbsbs / matheus-bagliano / commits / b2eec6663f

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>

Elijah Newren committed Apr 20, 2026 at 22:30 UTC b2eec6663f10a53dead5e7a4e5e9b91220bf9473
2 files changed +82 -26
merge-ort.c
+22 -26
@@ -2953,32 +2953,6 @@ static int process_renames(struct merge_options *opt,
2953 if (!oldinfo || oldinfo->merged.clean)
2954 continue;
2955
2956 - /*
2957 - * Rename caching from a previous commit might give us an
2958 - * irrelevant rename for the current commit.
2959 - *
2960 - * Imagine:
2961 - * foo/A -> bar/A
2962 - * was a cached rename for the upstream side from the
2963 - * previous commit (without the directories being renamed),
2964 - * but the next commit being replayed
2965 - * * does NOT add or delete files
2966 - * * does NOT have directory renames
2967 - * * does NOT modify any files under bar/
2968 - * * does NOT modify foo/A
2969 - * * DOES modify other files under foo/ (otherwise the
2970 - * !oldinfo check above would have already exited for
2971 - * us)
2972 - * In such a case, our trivial directory resolution will
2973 - * have already merged bar/, and our attempt to process
2974 - * the cached
2975 - * foo/A -> bar/A
2976 - * would be counterproductive, and lack the necessary
2977 - * information anyway. Skip such renames.
2978 - */
2979 - if (!newinfo)
2980 - continue;
2981 -
2956 /*
2957 * diff_filepairs have copies of pathnames, thus we have to
2958 * use standard 'strcmp()' (negated) instead of '=='.
@@ -3329,6 +3303,28 @@ static void use_cached_pairs(struct merge_options *opt,
3303 if (!new_name)
3304 new_name = old_name;
3305
3306 + /*
3307 + * If this is a rename and the target path is either
3308 + * absent from opt->priv->paths (because a parent
3309 + * directory was trivially resolved) or already cleanly
3310 + * resolved (e.g. all three sides agree on its content),
3311 + * the cached rename is irrelevant for this commit.
3312 + * Skip it here rather than in process_renames() to
3313 + * preserve VERIFY_CI(newinfo)'s ability to catch bugs
3314 + * for non-cached renames (see 979ee83e8a90 (merge-ort:
3315 + * fix corner case recursive submodule/directory conflict
3316 + * handling, 2025-12-29) for an example of a bug that
3317 + * assertion caught). The rename remains in cached_pairs
3318 + * for use in subsequent commits.
3319 + */
3320 + if (entry->value) {
3321 + struct merged_info *mi;
3322 +
3323 + mi = strmap_get(&opt->priv->paths, new_name);
3324 + if (!mi || mi->clean)
3325 + continue;
3326 + }
3327 +
3328 /*
3329 * cached_pairs has *copies* of old_name and new_name,
3330 * because it has to persist across merges. Since
t/t6429-merge-sequence-rename-caching.sh
+60
@@ -846,4 +846,64 @@ test_expect_success 'rename a file, use it on first pick, but irrelevant on seco
846 )
847 '
848
849 +#
850 +# In the following testcase:
851 +# Base: subdir/file_1
852 +# Upstream: file_1 (renamed from subdir/file)
853 +# Topic_1: subdir/file_2 (modified subdir/file)
854 +# Topic_2: subdir/file_2, file_2 (added another "file" with same contents)
855 +# Topic_3: file_2 (deleted subdir/file)
856 +#
857 +#
858 +# This testcase presents no problems for git traditionally, but the fact that
859 +# subdir/file -> file
860 +# gets cached after the first pick presents a problem for the third commit
861 +# to be replayed, because file has contents file_2 on all three sides and
862 +# is thus trivially resolved early. The point of renames is to allow us to
863 +# three-way merge contents across multiple filenames, but if the target is
864 +# already resolved, we risk throwing an assertion. Verify that the code
865 +# correctly drops the irrelevant rename in order to avoid hitting that
866 +# assertion.
867 +#
868 +test_expect_success 'cached rename does not assert on trivially clean target' '
869 + git init cached-rename-trivially-clean-target &&
870 + (
871 + cd cached-rename-trivially-clean-target &&
872 +
873 + mkdir subdir &&
874 + printf "%s\n" 1 2 3 >subdir/file &&
875 + git add subdir/file &&
876 + git commit -m orig &&
877 +
878 + git branch upstream &&
879 + git branch topic &&
880 +
881 + git switch upstream &&
882 + git mv subdir/file file &&
883 + git commit -m "rename subdir/file to file" &&
884 +
885 + git switch topic &&
886 +
887 + echo 4 >>subdir/file &&
888 + git add subdir/file &&
889 + git commit -m "modify subdir/file" &&
890 +
891 + cp subdir/file file &&
892 + git add file &&
893 + git commit -m "copy subdir/file to file" &&
894 +
895 + git rm subdir/file &&
896 + git commit -m "delete subdir/file" &&
897 +
898 + git switch upstream &&
899 + git replay --onto HEAD upstream..topic &&
900 + git checkout topic &&
901 +
902 + git ls-files >tracked-files &&
903 + test_line_count = 1 tracked-files &&
904 + printf "%s\n" 1 2 3 4 >expect &&
905 + test_cmp expect file
906 + )
907 +'
908 +
909 test_done