Skip to content

Commit

Permalink
replay: side-step bug when directory renames are turned off
Browse files Browse the repository at this point in the history
There is a bug in the way renames are cached that rears its head when
directory renames are turned off.

This patch comes with a demonstration of this bug which will fail with
the following message unless the rename cache is explicitly reset in
`merge_check_renames_reusable()` when `merge.directoryRenames=false`:

	merge-ort.c:2909: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.
	Aborted

It is quite a curious bug: the same test case will succeed, without any
assertion, if instead run with `merge.directoryRenames=true`.

Further, the assertion does not manifest while replaying the first
commit, it manifests while replaying the _second_ commit of the commit
range. But it does _not_ manifest when the second commit is replayed
individually.

This would indicate that there is an incomplete rename cache left-over
from the first replayed commit which is being reused for the second
commit, and if directory rename detection is enabled, the missing paths
are somehow regenerated.

This was a fun, intense hunt.

The solution to the riddle is that indeed, there was a stale cached
rename information. The fix is easy: explicitly record the diff pair as
a potential rename.

Helped-by; Elijah Newren <newren@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
  • Loading branch information
dscho committed Jul 15, 2024
1 parent e928c11 commit f4c7994
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 0 deletions.
7 changes: 7 additions & 0 deletions merge-ort.c
Original file line number Diff line number Diff line change
Expand Up @@ -3292,6 +3292,13 @@ static int collect_renames(struct merge_options *opt,
continue;
}

if (opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_NONE &&
p->status == 'R') {
possibly_cache_new_pair(renames, p, side_index, NULL);
pool_diff_free_filepair(&opt->priv->pool, p);
continue;
}

new_path = check_for_directory_rename(opt, p->two->path,
side_index,
dir_renames_for_side,
Expand Down
22 changes: 22 additions & 0 deletions t/t3650-replay-basics.sh
Original file line number Diff line number Diff line change
Expand Up @@ -195,4 +195,26 @@ test_expect_success 'using replay on bare repo to rebase multiple divergent bran
done
'

test_expect_success 'merge.directoryRenames=false' '
# create a test case that stress-tests the rename caching
git switch -c rename-onto &&
mkdir -p to-rename &&
test_commit to-rename/move &&
mkdir -p renamed-directory &&
git mv to-rename/move* renamed-directory/ &&
test_tick &&
git commit -m renamed-directory &&
git switch -c rename-from HEAD^ &&
test_commit to-rename/add-a-file &&
echo modified >to-rename/add-a-file.t &&
test_tick &&
git commit -m modified to-rename/add-a-file.t &&
git -c merge.directoryRenames=false replay \
--onto rename-onto rename-onto..rename-from
'

test_done

0 comments on commit f4c7994

Please sign in to comment.