Skip to content

Commit ff6d547

Browse files
newrengitster
newren
authored and
gitster
committed
merge-recursive: avoid directory rename detection in recursive case
Ever since commit 8c8e5bd ("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 <[email protected]> Signed-off-by: Elijah Newren <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 4d8ec15 commit ff6d547

File tree

2 files changed

+113
-1
lines changed

2 files changed

+113
-1
lines changed

‎merge-recursive.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2856,7 +2856,8 @@ static int detect_and_process_renames(struct merge_options *opt,
28562856
head_pairs = get_diffpairs(opt, common, head);
28572857
merge_pairs = get_diffpairs(opt, common, merge);
28582858

2859-
if (opt->detect_directory_renames) {
2859+
if ((opt->detect_directory_renames == 2) ||
2860+
(opt->detect_directory_renames == 1 && !opt->call_depth)) {
28602861
dir_re_head = get_directory_renames(head_pairs);
28612862
dir_re_merge = get_directory_renames(merge_pairs);
28622863

‎t/t6043-merge-rename-directories.sh

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4403,4 +4403,115 @@ test_expect_success '13d-check(info): messages for rename/rename(1to1) via dual
44034403
)
44044404
'
44054405

4406+
# Testcase 13e, directory rename in virtual merge base
4407+
#
4408+
# This testcase has a slightly different setup than all the above cases, in
4409+
# order to include a recursive case:
4410+
#
4411+
# A C
4412+
# o - o
4413+
# / \ / \
4414+
# O o X ?
4415+
# \ / \ /
4416+
# o o
4417+
# B D
4418+
#
4419+
# Commit O: a/{z,y}
4420+
# Commit A: b/{z,y}
4421+
# Commit B: a/{z,y,x}
4422+
# Commit C: b/{z,y,x}
4423+
# Commit D: b/{z,y}, a/x
4424+
# Expected: b/{z,y,x} (sort of; see below for why this might not be expected)
4425+
#
4426+
# NOTES: 'X' represents a virtual merge base. With the default of
4427+
# directory rename detection yielding conflicts, merging A and B
4428+
# results in a conflict complaining about whether 'x' should be
4429+
# under 'a/' or 'b/'. However, when creating the virtual merge
4430+
# base 'X', since virtual merge bases need to be written out as a
4431+
# tree, we cannot have a conflict, so some resolution has to be
4432+
# picked.
4433+
#
4434+
# In choosing the right resolution, it's worth noting here that
4435+
# commits C & D are merges of A & B that choose different
4436+
# locations for 'x' (i.e. they resolve the conflict differently),
4437+
# and so it would be nice when merging C & D if git could detect
4438+
# this difference of opinion and report a conflict. But the only
4439+
# way to do so that I can think of would be to have the virtual
4440+
# merge base place 'x' in some directory other than either 'a/' or
4441+
# 'b/', which seems a little weird -- especially since it'd result
4442+
# in a rename/rename(1to2) conflict with a source path that never
4443+
# existed in any version.
4444+
#
4445+
# So, for now, when directory rename detection is set to
4446+
# 'conflict' just avoid doing directory rename detection at all in
4447+
# the recursive case. This will not allow us to detect a conflict
4448+
# in the outer merge for this special kind of setup, but it at
4449+
# least avoids hitting a BUG().
4450+
#
4451+
test_expect_success '13e-setup: directory rename detection in recursive case' '
4452+
test_create_repo 13e &&
4453+
(
4454+
cd 13e &&
4455+
4456+
mkdir a &&
4457+
echo z >a/z &&
4458+
echo y >a/y &&
4459+
git add a &&
4460+
test_tick &&
4461+
git commit -m "O" &&
4462+
4463+
git branch O &&
4464+
git branch A &&
4465+
git branch B &&
4466+
4467+
git checkout A &&
4468+
git mv a/ b/ &&
4469+
test_tick &&
4470+
git commit -m "A" &&
4471+
4472+
git checkout B &&
4473+
echo x >a/x &&
4474+
git add a &&
4475+
test_tick &&
4476+
git commit -m "B" &&
4477+
4478+
git branch C A &&
4479+
git branch D B &&
4480+
4481+
git checkout C &&
4482+
test_must_fail git -c merge.directoryRenames=conflict merge B &&
4483+
git add b/x &&
4484+
test_tick &&
4485+
git commit -m "C" &&
4486+
4487+
4488+
git checkout D &&
4489+
test_must_fail git -c merge.directoryRenames=conflict merge A &&
4490+
git add b/x &&
4491+
mkdir a &&
4492+
git mv b/x a/x &&
4493+
test_tick &&
4494+
git commit -m "D"
4495+
)
4496+
'
4497+
4498+
test_expect_success '13e-check: directory rename detection in recursive case' '
4499+
(
4500+
cd 13e &&
4501+
4502+
git checkout --quiet D^0 &&
4503+
4504+
git -c merge.directoryRenames=conflict merge -s recursive C^0 >out 2>err &&
4505+
4506+
test_i18ngrep ! CONFLICT out &&
4507+
test_i18ngrep ! BUG: err &&
4508+
test_i18ngrep ! core.dumped err &&
4509+
test_must_be_empty err &&
4510+
4511+
git ls-files >paths &&
4512+
! grep a/x paths &&
4513+
grep b/x paths
4514+
)
4515+
'
4516+
44064517
test_done

0 commit comments

Comments
 (0)