{"thread":{"id":"48830","subject":"[PATCH] t6036: add lots of detail for directory/file conflicts in recursive case","startedAt":"2018-07-04T22:13:22Z","lastAt":"2018-07-04T22:13:22Z","messageCount":1,"participants":["Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"351702","messageId":"20180704221311.11004-1-newren@gmail.com","threadId":"48830","inReplyTo":null,"subject":"[PATCH] t6036: add lots of detail for directory/file conflicts in recursive case","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-04T22:13:11Z","receivedAt":"2018-07-04T22:13:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"There was a discussion of problematic directory/file conflicts with\nvirtual merge bases on the mailing list years ago at\n  https://public-inbox.org/git/AANLkTimwUQafGDrjxWrfU9uY1uKoFLJhxYs=vssOPqdf@mail.gmail.com/\nPart of these corresponding tests made it into this testsuite.  However,\nthe more problematic one didn't.  And there are others that showcase the\nproblems even more.  Add a very lengthy explanation, some of it from that\nemail, describing the tradeoffs in picking a recursive merge-base when\nyou're dealing with an add/add directory/file conflict.\n\nThe solution picked years ago is relatively good, but there is the\npotential to do even better, assuming we're willing to pay a certain\nperformance cost.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t6036-recursive-corner-cases.sh | 256 +++++++++++++++++++++++-------\n 1 file changed, 201 insertions(+), 55 deletions(-)\n\ndiff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh\nindex b5621303d..74e5920bc 100755\n--- a/t/t6036-recursive-corner-cases.sh\n+++ b/t/t6036-recursive-corner-cases.sh\n@@ -345,40 +345,97 @@ test_expect_success 'git detects conflict merging criss-cross+modify/delete, rev\n \t)\n '\n \n+#      SORRY FOR THE SUPER LONG DESCRIPTION, BUT THIS NEXT ONE IS HAIRY\n #\n # criss-cross + d/f conflict via add/add:\n #   Commit A: Neither file 'a' nor directory 'a/' exists.\n #   Commit B: Introduce 'a'\n #   Commit C: Introduce 'a/file'\n-#   Commit D: Merge B & C, keeping 'a' and deleting 'a/'\n-#\n-# Two different later cases:\n+#   Commit D1: Merge B & C, keeping 'a'    and deleting 'a/'\n #   Commit E1: Merge B & C, deleting 'a' but keeping 'a/file'\n-#   Commit E2: Merge B & C, deleting 'a' but keeping a slightly modified 'a/file'\n #\n-#      B   D\n+#      B   D1 or D2\n #      o---o\n #     / \\ / \\\n #  A o   X   ? F\n #     \\ / \\ /\n #      o---o\n-#      C   E1 or E2\n-#\n-# Merging D & E1 requires we first create a virtual merge base X from\n-# merging A & B in memory.  Now, if X could keep both 'a' and 'a/file' in\n-# the index, then the merge of D & E1 could be resolved cleanly with both\n-# 'a' and 'a/file' removed.  Since git does not currently allow creating\n-# such a tree, the best we can do is have X contain both 'a~<unique>' and\n-# 'a/file' resulting in the merge of D and E1 having a rename/delete\n-# conflict for 'a'.  (Although this merge appears to be unsolvable with git\n-# currently, git could do a lot better than it currently does with these\n-# d/f conflicts, which is the purpose of this test.)\n-#\n-# Merge of D & E2 has similar issues for path 'a', but should always result\n-# in a modify/delete conflict for path 'a/file'.\n-#\n-# We run each merge in both directions, to check for directional issues\n-# with D/F conflict handling.\n+#      C   E1 or E2 or E3\n+#\n+# I'll describe D2, E2, & E3 (which are alternatives for D1 & E1) more below...\n+#\n+# Merging D1 & E1 requires we first create a virtual merge base X from\n+# merging A & B in memory.  There are several possibilities for the merge-base:\n+#   1: Keep both 'a' and 'a/file' (assuming crazy filesystem allowing a tree\n+#      with a directory and file at same path): results in merge of D1 & E1\n+#      being clean with both files deleted.  Bad (no conflict detected).\n+#   2: Keep 'a' but not 'a/file': Merging D1 & E1 is clean and matches E1.  Bad.\n+#   3: Keep 'a/file' but not 'a': Merging D1 & E1 is clean and matches D1.  Bad.\n+#   4: Keep neither file: Merging D1 & E1 reports the D/F add/add conflict.\n+#\n+# So 4 sounds good for this case, but if we were to merge D1 & E3, where E3\n+# is defined as:\n+#   Commit E3: Merge B & C, keeping modified a, and deleting a/\n+# then we'd get an add/add conflict for 'a', which seems suboptimal.  A little\n+# creativity leads us to an alternate choice:\n+#   5: Keep 'a' as 'a~$UNIQUE' and a/file; results:\n+#        Merge D1 & E1: rename/delete conflict for 'a'; a/file silently deleted\n+#        Merge D1 & E3 is clean, as expected.\n+#\n+# So choice 5 at least provides some kind of conflict for the original case,\n+# and can merge cleanly as expected with D1 and E3.  It also made things just\n+# slightly funny for merging D1 and e$, where E4 is defined as:\n+#   Commit E4: Merge B & C, modifying 'a' and renaming to 'a2', and deleting 'a/'\n+# in this case, we'll get a rename/rename(1to2) conflict because a~$UNIQUE\n+# gets renamed to 'a' in D1 and to 'a2' in E4.  But that's better than having\n+# two files (both 'a' and 'a2') sitting around without the user being notified\n+# that we could detect they were related and need to be merged.  Also, choice\n+# 5 makes the handling of 'a/file' seem suboptimal.  What if we were to merge\n+# D2 and E4, where D2 is:\n+#   Commit D2: Merge B & C, renaming 'a'->'a2', keeping 'a/file'\n+# This would result in a clean merge with 'a2' having three-way merged\n+# contents (good), and deleting 'a/' (bad) -- it doesn't detect the\n+# conflict in how the different sides treated a/file differently.\n+# Continuing down the creative route:\n+#   6: Keep 'a' as 'a~$UNIQUE1' and keep 'a/' as 'a~$UNIQUE2/'; results:\n+#        Merge D1 & E1: rename/delete conflict for 'a' and each path under 'a/'.\n+#        Merge D1 & E3: clean, as expected.\n+#        Merge D1 & E4: rename/rename(1to2) conflict on 'a' vs 'a2'.\n+#        Merge D2 & E4: clean for 'a2', rename/delete for a/file\n+#\n+# Choice 6 could cause rename detection to take longer (providing more targets\n+# that need to be searched).  Also, the conflict message for each path under\n+# 'a/' might be annoying unless we can detect it at the directory level, print\n+# it once, and then suppress it for individual filepaths underneath.\n+#\n+#\n+# As of time of writing, git uses choice 5.  Directory rename detection and\n+# rename detection performance improvements might make choice 6 a desirable\n+# improvement.  But we can at least document where we fall short for now...\n+#\n+#\n+# Historically, this testcase also used:\n+#   Commit E2: Merge B & C, deleting 'a' but keeping slightly modified 'a/file'\n+# The merge of D1 & E2 is very similar to D1 & E1 -- it has similar issues for\n+# path 'a', but should always result in a modify/delete conflict for path\n+# 'a/file'.  These tests ran the two merges\n+#   D1 & E1\n+#   D1 & E2\n+# in both directions, to check for directional issues with D/F conflict\n+# handling. Later we added\n+#   D1 & E3\n+#   D1 & E4\n+#   D2 & E4\n+# for good measure, though we only ran those one way because we had pretty\n+# good confidence in merge-recursive's directional handling of D/F issues.\n+#\n+# Just to summarize all the intermediate merge commits:\n+#   Commit D1: Merge B & C, keeping a    and deleting a/\n+#   Commit D2: Merge B & C, renaming a->a2, keeping a/file\n+#   Commit E1: Merge B & C, deleting a but keeping a/file\n+#   Commit E2: Merge B & C, deleting a but keeping slightly modified a/file\n+#   Commit E3: Merge B & C, keeping modified a, and deleting a/\n+#   Commit E4: Merge B & C, modifying 'a' and renaming to 'a2', and deleting 'a/'\n #\n \n test_expect_success 'setup differently handled merges of directory/file conflict' '\n@@ -395,56 +452,70 @@ test_expect_success 'setup differently handled merges of directory/file conflict\n \t\tgit branch B &&\n \t\tgit checkout -b C &&\n \t\tmkdir a &&\n-\t\techo 10 >a/file &&\n+\t\ttest_write_lines a b c d e f g >a/file &&\n \t\tgit add a/file &&\n \t\ttest_tick &&\n \t\tgit commit -m C &&\n \n \t\tgit checkout B &&\n-\t\techo 5 >a &&\n+\t\ttest_write_lines 1 2 3 4 5 6 7 >a &&\n \t\tgit add a &&\n \t\ttest_tick &&\n \t\tgit commit -m B &&\n \n \t\tgit checkout B^0 &&\n-\t\ttest_must_fail git merge C &&\n-\t\tgit clean -f &&\n-\t\trm -rf a/ &&\n-\t\techo 5 >a &&\n-\t\tgit add a &&\n-\t\ttest_tick &&\n-\t\tgit commit -m D &&\n-\t\tgit tag D &&\n+\t\tgit merge -s ours -m D1 C^0 &&\n+\t\tgit tag D1 &&\n+\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git merge C^0 &&\n+\t\tgit clean -fd &&\n+\t\tgit rm -rf a/ &&\n+\t\tgit rm a &&\n+\t\tgit cat-file -p B:a >a2 &&\n+\t\tgit add a2 &&\n+\t\tgit commit -m D2 &&\n+\t\tgit tag D2 &&\n \n \t\tgit checkout C^0 &&\n-\t\ttest_must_fail git merge B &&\n-\t\tgit clean -f &&\n-\t\tgit rm --cached a &&\n-\t\techo 10 >a/file &&\n-\t\tgit add a/file &&\n-\t\ttest_tick &&\n-\t\tgit commit -m E1 &&\n+\t\tgit merge -s ours -m E1 B^0 &&\n \t\tgit tag E1 &&\n \n \t\tgit checkout C^0 &&\n-\t\ttest_must_fail git merge B &&\n-\t\tgit clean -f &&\n-\t\tgit rm --cached a &&\n-\t\tprintf \"10\\n11\\n\" >a/file &&\n+\t\tgit merge -s ours -m E2 B^0 &&\n+\t\ttest_write_lines a b c d e f g h >a/file &&\n \t\tgit add a/file &&\n-\t\ttest_tick &&\n-\t\tgit commit -m E2 &&\n-\t\tgit tag E2\n+\t\tgit commit --amend -C HEAD &&\n+\t\tgit tag E2 &&\n+\n+\t\tgit checkout C^0 &&\n+\t\ttest_must_fail git merge B^0 &&\n+\t\tgit clean -fd &&\n+\t\tgit rm -rf a/ &&\n+\t\ttest_write_lines 1 2 3 4 5 6 7 8 >a &&\n+\t\tgit add a &&\n+\t\tgit commit -m E3 &&\n+\t\tgit tag E3\n+\n+\t\tgit checkout C^0 &&\n+\t\ttest_must_fail git merge B^0 &&\n+\t\tgit clean -fd &&\n+\t\tgit rm -rf a/ &&\n+\t\tgit rm a &&\n+\t\ttest_write_lines 1 2 3 4 5 6 7 8 >a2 &&\n+\t\tgit add a2 &&\n+\t\tgit commit -m E4 &&\n+\t\tgit tag E4\n \t)\n '\n \n-test_expect_success 'merge of D & E1 fails but has appropriate contents' '\n+test_expect_success 'merge of D1 & E1 fails but has appropriate contents' '\n \ttest_when_finished \"git -C directory-file reset --hard\" &&\n \ttest_when_finished \"git -C directory-file clean -fdqx\" &&\n \t(\n \t\tcd directory-file &&\n \n-\t\tgit checkout D^0 &&\n+\t\tgit checkout D1^0 &&\n \n \t\ttest_must_fail git merge -s recursive E1^0 &&\n \n@@ -463,7 +534,7 @@ test_expect_success 'merge of D & E1 fails but has appropriate contents' '\n \t)\n '\n \n-test_expect_success 'merge of E1 & D fails but has appropriate contents' '\n+test_expect_success 'merge of E1 & D1 fails but has appropriate contents' '\n \ttest_when_finished \"git -C directory-file reset --hard\" &&\n \ttest_when_finished \"git -C directory-file clean -fdqx\" &&\n \t(\n@@ -471,7 +542,7 @@ test_expect_success 'merge of E1 & D fails but has appropriate contents' '\n \n \t\tgit checkout E1^0 &&\n \n-\t\ttest_must_fail git merge -s recursive D^0 &&\n+\t\ttest_must_fail git merge -s recursive D1^0 &&\n \n \t\tgit ls-files -s >out &&\n \t\ttest_line_count = 2 out &&\n@@ -488,13 +559,13 @@ test_expect_success 'merge of E1 & D fails but has appropriate contents' '\n \t)\n '\n \n-test_expect_success 'merge of D & E2 fails but has appropriate contents' '\n+test_expect_success 'merge of D1 & E2 fails but has appropriate contents' '\n \ttest_when_finished \"git -C directory-file reset --hard\" &&\n \ttest_when_finished \"git -C directory-file clean -fdqx\" &&\n \t(\n \t\tcd directory-file &&\n \n-\t\tgit checkout D^0 &&\n+\t\tgit checkout D1^0 &&\n \n \t\ttest_must_fail git merge -s recursive E2^0 &&\n \n@@ -515,7 +586,7 @@ test_expect_success 'merge of D & E2 fails but has appropriate contents' '\n \t)\n '\n \n-test_expect_success 'merge of E2 & D fails but has appropriate contents' '\n+test_expect_success 'merge of E2 & D1 fails but has appropriate contents' '\n \ttest_when_finished \"git -C directory-file reset --hard\" &&\n \ttest_when_finished \"git -C directory-file clean -fdqx\" &&\n \t(\n@@ -523,7 +594,7 @@ test_expect_success 'merge of E2 & D fails but has appropriate contents' '\n \n \t\tgit checkout E2^0 &&\n \n-\t\ttest_must_fail git merge -s recursive D^0 &&\n+\t\ttest_must_fail git merge -s recursive D1^0 &&\n \n \t\tgit ls-files -s >out &&\n \t\ttest_line_count = 4 out &&\n@@ -538,7 +609,82 @@ test_expect_success 'merge of E2 & D fails but has appropriate contents' '\n \t\t\t:3:a  :2:a/file  :1:a/file  :0:ignore-me &&\n \t\ttest_cmp expect actual\n \n-\t\ttest_path_is_file a~D^0\n+\t\ttest_path_is_file a~D1^0\n+\t)\n+'\n+\n+test_expect_success 'merge of D1 & E3 succeeds' '\n+\ttest_when_finished \"git -C directory-file reset --hard\" &&\n+\ttest_when_finished \"git -C directory-file clean -fdqx\" &&\n+\t(\n+\t\tcd directory-file &&\n+\n+\t\tgit checkout D1^0 &&\n+\n+\t\tgit merge -s recursive E3^0 &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 2 out &&\n+\t\tgit ls-files -u >out &&\n+\t\ttest_line_count = 0 out &&\n+\t\tgit ls-files -o >out &&\n+\t\ttest_line_count = 1 out &&\n+\n+\t\tgit rev-parse >expect    \\\n+\t\t\tA:ignore-me  E3:a &&\n+\t\tgit rev-parse   >actual   \\\n+\t\t\t:0:ignore-me :0:a &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'merge of D1 & E4 notifies user a and a2 are related' '\n+\ttest_when_finished \"git -C directory-file reset --hard\" &&\n+\ttest_when_finished \"git -C directory-file clean -fdqx\" &&\n+\t(\n+\t\tcd directory-file &&\n+\n+\t\tgit checkout D1^0 &&\n+\n+\t\ttest_must_fail git merge -s recursive E4^0 &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 4 out &&\n+\t\tgit ls-files -u >out &&\n+\t\ttest_line_count = 3 out &&\n+\t\tgit ls-files -o >out &&\n+\t\ttest_line_count = 1 out &&\n+\n+\t\tgit rev-parse >expect                  \\\n+\t\t\tA:ignore-me  B:a   D1:a  E4:a2 &&\n+\t\tgit rev-parse   >actual                \\\n+\t\t\t:0:ignore-me :1:a~Temporary\\ merge\\ branch\\ 2  :2:a  :3:a2 &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_failure 'merge of D2 & E4 merges a2s & reports conflict for a/file' '\n+\ttest_when_finished \"git -C directory-file reset --hard\" &&\n+\ttest_when_finished \"git -C directory-file clean -fdqx\" &&\n+\t(\n+\t\tcd directory-file &&\n+\n+\t\tgit checkout D2^0 &&\n+\n+\t\ttest_must_fail git merge -s recursive E4^0 &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 3 out &&\n+\t\tgit ls-files -u >out &&\n+\t\ttest_line_count = 1 out &&\n+\t\tgit ls-files -o >out &&\n+\t\ttest_line_count = 1 out &&\n+\n+\t\tgit rev-parse >expect                 \\\n+\t\t\tA:ignore-me  E4:a2  D2:a/file &&\n+\t\tgit rev-parse   >actual               \\\n+\t\t\t:0:ignore-me :0:a2  :2:a/file &&\n+\t\ttest_cmp expect actual\n \t)\n '\n \n-- \n2.18.0.131.g7487c9a82.dirty\n\n"}]}