Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
newren wants to merge 1 commit into
base: master
Choose a base branch
from
+82 −26
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 22 additions & 26 deletions merge-ort.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 '=='.
Expand Down Expand Up @@ -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
Expand Down
60 changes: 60 additions & 0 deletions t/t6429-merge-sequence-rename-caching.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading