From ff6d54771af2c92867f773a264200f22b4cb2ab9 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 5 Aug 2019 15:33:50 -0700 Subject: [PATCH] merge-recursive: avoid directory rename detection in recursive case Ever since commit 8c8e5bd6eb33 ("merge-recursive: switch directory rename detection default", 2019-04-05), the default handling with directory rename detection was to report a conflict and leave unstaged entries in the index. However, when creating a virtual merge base in the recursive case, we absolutely need a tree, and the only way a tree can be written is if we have no unstaged entries -- otherwise we hit a BUG(). There are a few fixes possible here which at least fix the BUG(), but none of them seem optimal for other reasons; see the comments with the new testcase 13e in t6043 for details (which testcase triggered a BUG() prior to this patch). As such, just opt for a very conservative and simple choice that is still relatively reasonable: have the recursive case treat 'conflict' as 'false' for opt->detect_directory_renames. Reported-by: Emily Shaffer Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 3 +- t/t6043-merge-rename-directories.sh | 111 ++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/merge-recursive.c b/merge-recursive.c index d2e380b7ed..c7691d9b54 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2856,7 +2856,8 @@ static int detect_and_process_renames(struct merge_options *opt, head_pairs = get_diffpairs(opt, common, head); merge_pairs = get_diffpairs(opt, common, merge); - if (opt->detect_directory_renames) { + if ((opt->detect_directory_renames == 2) || + (opt->detect_directory_renames == 1 && !opt->call_depth)) { dir_re_head = get_directory_renames(head_pairs); dir_re_merge = get_directory_renames(merge_pairs); diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index 50b7543483..c966147d5d 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -4403,4 +4403,115 @@ test_expect_success '13d-check(info): messages for rename/rename(1to1) via dual ) ' +# Testcase 13e, directory rename in virtual merge base +# +# This testcase has a slightly different setup than all the above cases, in +# order to include a recursive case: +# +# A C +# o - o +# / \ / \ +# O o X ? +# \ / \ / +# o o +# B D +# +# Commit O: a/{z,y} +# Commit A: b/{z,y} +# Commit B: a/{z,y,x} +# Commit C: b/{z,y,x} +# Commit D: b/{z,y}, a/x +# Expected: b/{z,y,x} (sort of; see below for why this might not be expected) +# +# NOTES: 'X' represents a virtual merge base. With the default of +# directory rename detection yielding conflicts, merging A and B +# results in a conflict complaining about whether 'x' should be +# under 'a/' or 'b/'. However, when creating the virtual merge +# base 'X', since virtual merge bases need to be written out as a +# tree, we cannot have a conflict, so some resolution has to be +# picked. +# +# In choosing the right resolution, it's worth noting here that +# commits C & D are merges of A & B that choose different +# locations for 'x' (i.e. they resolve the conflict differently), +# and so it would be nice when merging C & D if git could detect +# this difference of opinion and report a conflict. But the only +# way to do so that I can think of would be to have the virtual +# merge base place 'x' in some directory other than either 'a/' or +# 'b/', which seems a little weird -- especially since it'd result +# in a rename/rename(1to2) conflict with a source path that never +# existed in any version. +# +# So, for now, when directory rename detection is set to +# 'conflict' just avoid doing directory rename detection at all in +# the recursive case. This will not allow us to detect a conflict +# in the outer merge for this special kind of setup, but it at +# least avoids hitting a BUG(). +# +test_expect_success '13e-setup: directory rename detection in recursive case' ' + test_create_repo 13e && + ( + cd 13e && + + mkdir a && + echo z >a/z && + echo y >a/y && + git add a && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + git mv a/ b/ && + test_tick && + git commit -m "A" && + + git checkout B && + echo x >a/x && + git add a && + test_tick && + git commit -m "B" && + + git branch C A && + git branch D B && + + git checkout C && + test_must_fail git -c merge.directoryRenames=conflict merge B && + git add b/x && + test_tick && + git commit -m "C" && + + + git checkout D && + test_must_fail git -c merge.directoryRenames=conflict merge A && + git add b/x && + mkdir a && + git mv b/x a/x && + test_tick && + git commit -m "D" + ) +' + +test_expect_success '13e-check: directory rename detection in recursive case' ' + ( + cd 13e && + + git checkout --quiet D^0 && + + git -c merge.directoryRenames=conflict merge -s recursive C^0 >out 2>err && + + test_i18ngrep ! CONFLICT out && + test_i18ngrep ! BUG: err && + test_i18ngrep ! core.dumped err && + test_must_be_empty err && + + git ls-files >paths && + ! grep a/x paths && + grep b/x paths + ) +' + test_done