From f4c7994efc5c07b459086be1206269177f1e1181 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 23 May 2023 14:35:41 +0200 Subject: [PATCH] replay: side-step bug when directory renames are turned off 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 Signed-off-by: Johannes Schindelin --- merge-ort.c | 7 +++++++ t/t3650-replay-basics.sh | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/merge-ort.c b/merge-ort.c index 6491070d965835..9230afd0cd39e9 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -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, diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh index 389670262e458e..58b37599357827 100755 --- a/t/t3650-replay-basics.sh +++ b/t/t3650-replay-basics.sh @@ -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