t6036: add lots of detail for directory/file conflicts in recursive case
There was a discussion of problematic directory/file conflicts with virtual merge bases on the mailing list years ago at https://public-inbox.org/git/AANLkTimwUQafGDrjxWrfU9uY1uKoFLJhxYs=vssOPqdf@mail.gmail.com/ Part of these corresponding tests made it into this testsuite. However, the more problematic one didn't. And there are others that showcase the problems even more. Add a very lengthy explanation, some of it from that email, describing the tradeoffs in picking a recursive merge-base when you're dealing with an add/add directory/file conflict. The solution picked years ago is relatively good, but there is the potential to do even better, assuming we're willing to pay a certain performance cost. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren committed
Jul 4, 2018 at 15:13 UTC
327ac9cb9dda3ef8be5cc66a01daa32c6e8c8639
1 file changed
+201
-55
t/t6036-recursive-corner-cases.sh
+201
-55
@@ -345,40 +345,97 @@ test_expect_success 'git detects conflict merging criss-cross+modify/delete, rev
345
)
346
'
347
348
+# SORRY FOR THE SUPER LONG DESCRIPTION, BUT THIS NEXT ONE IS HAIRY
349
#
350
# criss-cross + d/f conflict via add/add:
351
# Commit A: Neither file 'a' nor directory 'a/' exists.
352
# Commit B: Introduce 'a'
353
# Commit C: Introduce 'a/file'
353
-# Commit D: Merge B & C, keeping 'a' and deleting 'a/'
354
-#
355
-# Two different later cases:
354
+# Commit D1: Merge B & C, keeping 'a' and deleting 'a/'
355
# Commit E1: Merge B & C, deleting 'a' but keeping 'a/file'
357
-# Commit E2: Merge B & C, deleting 'a' but keeping a slightly modified 'a/file'
356
#
359
-# B D
357
+# B D1 or D2
358
# o---o
359
# / \ / \
360
# A o X ? F
361
# \ / \ /
362
# o---o
365
-# C E1 or E2
366
-#
367
-# Merging D & E1 requires we first create a virtual merge base X from
368
-# merging A & B in memory. Now, if X could keep both 'a' and 'a/file' in
369
-# the index, then the merge of D & E1 could be resolved cleanly with both
370
-# 'a' and 'a/file' removed. Since git does not currently allow creating
371
-# such a tree, the best we can do is have X contain both 'a~<unique>' and
372
-# 'a/file' resulting in the merge of D and E1 having a rename/delete
373
-# conflict for 'a'. (Although this merge appears to be unsolvable with git
374
-# currently, git could do a lot better than it currently does with these
375
-# d/f conflicts, which is the purpose of this test.)
376
-#
377
-# Merge of D & E2 has similar issues for path 'a', but should always result
378
-# in a modify/delete conflict for path 'a/file'.
379
-#
380
-# We run each merge in both directions, to check for directional issues
381
-# with D/F conflict handling.
363
+# C E1 or E2 or E3
364
+#
365
+# I'll describe D2, E2, & E3 (which are alternatives for D1 & E1) more below...
366
+#
367
+# Merging D1 & E1 requires we first create a virtual merge base X from
368
+# merging A & B in memory. There are several possibilities for the merge-base:
369
+# 1: Keep both 'a' and 'a/file' (assuming crazy filesystem allowing a tree
370
+# with a directory and file at same path): results in merge of D1 & E1
371
+# being clean with both files deleted. Bad (no conflict detected).
372
+# 2: Keep 'a' but not 'a/file': Merging D1 & E1 is clean and matches E1. Bad.
373
+# 3: Keep 'a/file' but not 'a': Merging D1 & E1 is clean and matches D1. Bad.
374
+# 4: Keep neither file: Merging D1 & E1 reports the D/F add/add conflict.
375
+#
376
+# So 4 sounds good for this case, but if we were to merge D1 & E3, where E3
377
+# is defined as:
378
+# Commit E3: Merge B & C, keeping modified a, and deleting a/
379
+# then we'd get an add/add conflict for 'a', which seems suboptimal. A little
380
+# creativity leads us to an alternate choice:
381
+# 5: Keep 'a' as 'a~$UNIQUE' and a/file; results:
382
+# Merge D1 & E1: rename/delete conflict for 'a'; a/file silently deleted
383
+# Merge D1 & E3 is clean, as expected.
384
+#
385
+# So choice 5 at least provides some kind of conflict for the original case,
386
+# and can merge cleanly as expected with D1 and E3. It also made things just
387
+# slightly funny for merging D1 and e$, where E4 is defined as:
388
+# Commit E4: Merge B & C, modifying 'a' and renaming to 'a2', and deleting 'a/'
389
+# in this case, we'll get a rename/rename(1to2) conflict because a~$UNIQUE
390
+# gets renamed to 'a' in D1 and to 'a2' in E4. But that's better than having
391
+# two files (both 'a' and 'a2') sitting around without the user being notified
392
+# that we could detect they were related and need to be merged. Also, choice
393
+# 5 makes the handling of 'a/file' seem suboptimal. What if we were to merge
394
+# D2 and E4, where D2 is:
395
+# Commit D2: Merge B & C, renaming 'a'->'a2', keeping 'a/file'
396
+# This would result in a clean merge with 'a2' having three-way merged
397
+# contents (good), and deleting 'a/' (bad) -- it doesn't detect the
398
+# conflict in how the different sides treated a/file differently.
399
+# Continuing down the creative route:
400
+# 6: Keep 'a' as 'a~$UNIQUE1' and keep 'a/' as 'a~$UNIQUE2/'; results:
401
+# Merge D1 & E1: rename/delete conflict for 'a' and each path under 'a/'.
402
+# Merge D1 & E3: clean, as expected.
403
+# Merge D1 & E4: rename/rename(1to2) conflict on 'a' vs 'a2'.
404
+# Merge D2 & E4: clean for 'a2', rename/delete for a/file
405
+#
406
+# Choice 6 could cause rename detection to take longer (providing more targets
407
+# that need to be searched). Also, the conflict message for each path under
408
+# 'a/' might be annoying unless we can detect it at the directory level, print
409
+# it once, and then suppress it for individual filepaths underneath.
410
+#
411
+#
412
+# As of time of writing, git uses choice 5. Directory rename detection and
413
+# rename detection performance improvements might make choice 6 a desirable
414
+# improvement. But we can at least document where we fall short for now...
415
+#
416
+#
417
+# Historically, this testcase also used:
418
+# Commit E2: Merge B & C, deleting 'a' but keeping slightly modified 'a/file'
419
+# The merge of D1 & E2 is very similar to D1 & E1 -- it has similar issues for
420
+# path 'a', but should always result in a modify/delete conflict for path
421
+# 'a/file'. These tests ran the two merges
422
+# D1 & E1
423
+# D1 & E2
424
+# in both directions, to check for directional issues with D/F conflict
425
+# handling. Later we added
426
+# D1 & E3
427
+# D1 & E4
428
+# D2 & E4
429
+# for good measure, though we only ran those one way because we had pretty
430
+# good confidence in merge-recursive's directional handling of D/F issues.
431
+#
432
+# Just to summarize all the intermediate merge commits:
433
+# Commit D1: Merge B & C, keeping a and deleting a/
434
+# Commit D2: Merge B & C, renaming a->a2, keeping a/file
435
+# Commit E1: Merge B & C, deleting a but keeping a/file
436
+# Commit E2: Merge B & C, deleting a but keeping slightly modified a/file
437
+# Commit E3: Merge B & C, keeping modified a, and deleting a/
438
+# Commit E4: Merge B & C, modifying 'a' and renaming to 'a2', and deleting 'a/'
439
#
440
441
test_expect_success 'setup differently handled merges of directory/file conflict' '
@@ -395,56 +452,70 @@ test_expect_success 'setup differently handled merges of directory/file conflict
452
git branch B &&
453
git checkout -b C &&
454
mkdir a &&
398
- echo 10 >a/file &&
455
+ test_write_lines a b c d e f g >a/file &&
456
git add a/file &&
457
test_tick &&
458
git commit -m C &&
459
460
git checkout B &&
404
- echo 5 >a &&
461
+ test_write_lines 1 2 3 4 5 6 7 >a &&
462
git add a &&
463
test_tick &&
464
git commit -m B &&
465
466
git checkout B^0 &&
410
- test_must_fail git merge C &&
411
- git clean -f &&
412
- rm -rf a/ &&
413
- echo 5 >a &&
414
- git add a &&
415
- test_tick &&
416
- git commit -m D &&
417
- git tag D &&
467
+ git merge -s ours -m D1 C^0 &&
468
+ git tag D1 &&
469
+
470
+ git checkout B^0 &&
471
+ test_must_fail git merge C^0 &&
472
+ git clean -fd &&
473
+ git rm -rf a/ &&
474
+ git rm a &&
475
+ git cat-file -p B:a >a2 &&
476
+ git add a2 &&
477
+ git commit -m D2 &&
478
+ git tag D2 &&
479
480
git checkout C^0 &&
420
- test_must_fail git merge B &&
421
- git clean -f &&
422
- git rm --cached a &&
423
- echo 10 >a/file &&
424
- git add a/file &&
425
- test_tick &&
426
- git commit -m E1 &&
481
+ git merge -s ours -m E1 B^0 &&
482
git tag E1 &&
483
484
git checkout C^0 &&
430
- test_must_fail git merge B &&
431
- git clean -f &&
432
- git rm --cached a &&
433
- printf "10\n11\n" >a/file &&
485
+ git merge -s ours -m E2 B^0 &&
486
+ test_write_lines a b c d e f g h >a/file &&
487
git add a/file &&
435
- test_tick &&
436
- git commit -m E2 &&
437
- git tag E2
488
+ git commit --amend -C HEAD &&
489
+ git tag E2 &&
490
+
491
+ git checkout C^0 &&
492
+ test_must_fail git merge B^0 &&
493
+ git clean -fd &&
494
+ git rm -rf a/ &&
495
+ test_write_lines 1 2 3 4 5 6 7 8 >a &&
496
+ git add a &&
497
+ git commit -m E3 &&
498
+ git tag E3
499
+
500
+ git checkout C^0 &&
501
+ test_must_fail git merge B^0 &&
502
+ git clean -fd &&
503
+ git rm -rf a/ &&
504
+ git rm a &&
505
+ test_write_lines 1 2 3 4 5 6 7 8 >a2 &&
506
+ git add a2 &&
507
+ git commit -m E4 &&
508
+ git tag E4
509
)
510
'
511
441
-test_expect_success 'merge of D & E1 fails but has appropriate contents' '
512
+test_expect_success 'merge of D1 & E1 fails but has appropriate contents' '
513
test_when_finished "git -C directory-file reset --hard" &&
514
test_when_finished "git -C directory-file clean -fdqx" &&
515
(
516
cd directory-file &&
517
447
- git checkout D^0 &&
518
+ git checkout D1^0 &&
519
520
test_must_fail git merge -s recursive E1^0 &&
521
@@ -463,7 +534,7 @@ test_expect_success 'merge of D & E1 fails but has appropriate contents' '
534
)
535
'
536
466
-test_expect_success 'merge of E1 & D fails but has appropriate contents' '
537
+test_expect_success 'merge of E1 & D1 fails but has appropriate contents' '
538
test_when_finished "git -C directory-file reset --hard" &&
539
test_when_finished "git -C directory-file clean -fdqx" &&
540
(
@@ -471,7 +542,7 @@ test_expect_success 'merge of E1 & D fails but has appropriate contents' '
542
543
git checkout E1^0 &&
544
474
- test_must_fail git merge -s recursive D^0 &&
545
+ test_must_fail git merge -s recursive D1^0 &&
546
547
git ls-files -s >out &&
548
test_line_count = 2 out &&
@@ -488,13 +559,13 @@ test_expect_success 'merge of E1 & D fails but has appropriate contents' '
559
)
560
'
561
491
-test_expect_success 'merge of D & E2 fails but has appropriate contents' '
562
+test_expect_success 'merge of D1 & E2 fails but has appropriate contents' '
563
test_when_finished "git -C directory-file reset --hard" &&
564
test_when_finished "git -C directory-file clean -fdqx" &&
565
(
566
cd directory-file &&
567
497
- git checkout D^0 &&
568
+ git checkout D1^0 &&
569
570
test_must_fail git merge -s recursive E2^0 &&
571
@@ -515,7 +586,7 @@ test_expect_success 'merge of D & E2 fails but has appropriate contents' '
586
)
587
'
588
518
-test_expect_success 'merge of E2 & D fails but has appropriate contents' '
589
+test_expect_success 'merge of E2 & D1 fails but has appropriate contents' '
590
test_when_finished "git -C directory-file reset --hard" &&
591
test_when_finished "git -C directory-file clean -fdqx" &&
592
(
@@ -523,7 +594,7 @@ test_expect_success 'merge of E2 & D fails but has appropriate contents' '
594
595
git checkout E2^0 &&
596
526
- test_must_fail git merge -s recursive D^0 &&
597
+ test_must_fail git merge -s recursive D1^0 &&
598
599
git ls-files -s >out &&
600
test_line_count = 4 out &&
@@ -538,7 +609,82 @@ test_expect_success 'merge of E2 & D fails but has appropriate contents' '
609
:3:a :2:a/file :1:a/file :0:ignore-me &&
610
test_cmp expect actual
611
541
- test_path_is_file a~D^0
612
+ test_path_is_file a~D1^0
613
+ )
614
+'
615
+
616
+test_expect_success 'merge of D1 & E3 succeeds' '
617
+ test_when_finished "git -C directory-file reset --hard" &&
618
+ test_when_finished "git -C directory-file clean -fdqx" &&
619
+ (
620
+ cd directory-file &&
621
+
622
+ git checkout D1^0 &&
623
+
624
+ git merge -s recursive E3^0 &&
625
+
626
+ git ls-files -s >out &&
627
+ test_line_count = 2 out &&
628
+ git ls-files -u >out &&
629
+ test_line_count = 0 out &&
630
+ git ls-files -o >out &&
631
+ test_line_count = 1 out &&
632
+
633
+ git rev-parse >expect \
634
+ A:ignore-me E3:a &&
635
+ git rev-parse >actual \
636
+ :0:ignore-me :0:a &&
637
+ test_cmp expect actual
638
+ )
639
+'
640
+
641
+test_expect_success 'merge of D1 & E4 notifies user a and a2 are related' '
642
+ test_when_finished "git -C directory-file reset --hard" &&
643
+ test_when_finished "git -C directory-file clean -fdqx" &&
644
+ (
645
+ cd directory-file &&
646
+
647
+ git checkout D1^0 &&
648
+
649
+ test_must_fail git merge -s recursive E4^0 &&
650
+
651
+ git ls-files -s >out &&
652
+ test_line_count = 4 out &&
653
+ git ls-files -u >out &&
654
+ test_line_count = 3 out &&
655
+ git ls-files -o >out &&
656
+ test_line_count = 1 out &&
657
+
658
+ git rev-parse >expect \
659
+ A:ignore-me B:a D1:a E4:a2 &&
660
+ git rev-parse >actual \
661
+ :0:ignore-me :1:a~Temporary\ merge\ branch\ 2 :2:a :3:a2 &&
662
+ test_cmp expect actual
663
+ )
664
+'
665
+
666
+test_expect_failure 'merge of D2 & E4 merges a2s & reports conflict for a/file' '
667
+ test_when_finished "git -C directory-file reset --hard" &&
668
+ test_when_finished "git -C directory-file clean -fdqx" &&
669
+ (
670
+ cd directory-file &&
671
+
672
+ git checkout D2^0 &&
673
+
674
+ test_must_fail git merge -s recursive E4^0 &&
675
+
676
+ git ls-files -s >out &&
677
+ test_line_count = 3 out &&
678
+ git ls-files -u >out &&
679
+ test_line_count = 1 out &&
680
+ git ls-files -o >out &&
681
+ test_line_count = 1 out &&
682
+
683
+ git rev-parse >expect \
684
+ A:ignore-me E4:a2 D2:a/file &&
685
+ git rev-parse >actual \
686
+ :0:ignore-me :0:a2 :2:a/file &&
687
+ test_cmp expect actual
688
)
689
'
690