merge-ort: handle cached rename & trivial resolution interaction better
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
@@ -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