{"thread":{"id":"54433","subject":"[PATCH 3/4] t6423: update directory rename detection tests with new rule","startedAt":"2020-10-15T20:46:39Z","lastAt":"2020-10-15T20:57:49Z","messageCount":6,"participants":["Elijah Newren via GitGitGadget","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"407678","messageId":"146d6f4735497f0228d22fc7aae2c419363e063a.1602794791.git.gitgitgadget@gmail.com","threadId":"54433","inReplyTo":"pull.879.git.git.1602794790.gitgitgadget@gmail.com","subject":"[PATCH 3/4] t6423: update directory rename detection tests with new rule","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-15T20:46:29Z","receivedAt":"2020-10-15T20:46:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nWhile investigating the issues highlighted by the testcase in the\nprevious patch, I also found a shortcoming in the directory rename\ndetection rules.  Split testcase 6b into two to explain this issue\nand update directory-rename-detection.txt to remove one of the previous\nrules that I know believe to be detrimental.  Also, update the wording\naround testcase 8e; while we are not modifying the results of that\ntestcase, we were previously unsure of the appropriate resolution of\nthat test and the new rule makes the previously chosen resolution for\nthat testcase a bit more solid.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n .../technical/directory-rename-detection.txt  |   5 +-\n t/t6423-merge-rename-directories.sh           | 144 +++++++++++++++---\n 2 files changed, 124 insertions(+), 25 deletions(-)\n\ndiff --git a/Documentation/technical/directory-rename-detection.txt b/Documentation/technical/directory-rename-detection.txt\nindex ce042cfcae..5d03539412 100644\n--- a/Documentation/technical/directory-rename-detection.txt\n+++ b/Documentation/technical/directory-rename-detection.txt\n@@ -70,10 +70,7 @@ additional rules:\n   a) If renames split a directory into two or more others, the directory\n      with the most renames, \"wins\".\n \n-  b) Avoid directory-rename-detection for a path, if that path is the\n-     source of a rename on either side of a merge.\n-\n-  c) Only apply implicit directory renames to directories if the other side\n+  b) Only apply implicit directory renames to directories if the other side\n      of history is the one doing the renaming.\n \n Limitations -- support in different commands\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 31aea47522..00eac6e9a2 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -1277,20 +1277,114 @@ test_expect_success '6a: Tricky rename/delete' '\n \t)\n '\n \n-# Testcase 6b, Same rename done on both sides\n+# Testcase 6b1, Same rename done on both sides\n+#   (Related to testcase 6b2 and 8e)\n+#   Commit O: z/{b,c,d,e}\n+#   Commit A: y/{b,c,d}, x/e\n+#   Commit B: y/{b,c,d}, z/{e,f}\n+#   Expected: y/{b,c,d,f}, x/e\n+#   Note: Directory rename detection says A renamed z/ -> y/ (3 paths renamed\n+#         to y/ and only 1 renamed to x/), therefore the new file 'z/f' in B\n+#         should be moved to 'y/f'.\n+#\n+#         This is a bit of an edge case where any behavior might surprise users,\n+#         whether that is treating A as renaming z/ -> y/, treating A as renaming\n+#         z/ -> x/, or treating A as not doing any directory rename.  However, I\n+#         think this answer is the least confusing and most consistent with the\n+#         rules elsewhere.\n+#\n+#         A note about z/ -> x/, since it may not be clear how that could come\n+#         about: If we were to ignore files renamed by both sides\n+#         (i.e. z/{b,c,d}), as directory rename detection did in git-2.18 thru\n+#         at least git-2.28, then we would note there are no renames from z/ to\n+#         y/ and one rename from z/ to x/ and thus come to the conclusion that\n+#         A renamed z/ -> x/.  This seems more confusing for end users than a\n+#         rename of z/ to y/, it makes directory rename detection behavior\n+#         harder for them to predict.  As such, we modified the rule, changed\n+#         the behavior on testcases 6b2 and 8e, and introduced this 6b1 testcase.\n+\n+test_setup_6b1 () {\n+\ttest_create_repo 6b1 &&\n+\t(\n+\t\tcd 6b1 &&\n+\n+\t\tmkdir z &&\n+\t\techo b >z/b &&\n+\t\techo c >z/c &&\n+\t\techo d >z/d &&\n+\t\techo e >z/e &&\n+\t\tgit add z &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout A &&\n+\t\tgit mv z y &&\n+\t\tmkdir x &&\n+\t\tgit mv y/e x/e &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit checkout B &&\n+\t\tgit mv z y &&\n+\t\tmkdir z &&\n+\t\tgit mv y/e z/e &&\n+\t\techo f >z/f &&\n+\t\tgit add z/f &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"B\"\n+\t)\n+}\n+\n+test_expect_failure '6b1: Same renames done on both sides, plus another rename' '\n+\ttest_setup_6b1 &&\n+\t(\n+\t\tcd 6b1 &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\tgit -c merge.directoryRenames=true merge -s recursive B^0 &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 5 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 >actual \\\n+\t\t\tHEAD:y/b HEAD:y/c HEAD:y/d HEAD:x/e HEAD:y/f &&\n+\t\tgit rev-parse >expect \\\n+\t\t\tO:z/b    O:z/c    O:z/d    O:z/e    B:z/f &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+# Testcase 6b2, Same rename done on both sides\n #   (Related to testcases 6c and 8e)\n #   Commit O: z/{b,c}\n #   Commit A: y/{b,c}\n #   Commit B: y/{b,c}, z/d\n-#   Expected: y/{b,c}, z/d\n-#   Note: If we did directory rename detection here, we'd move z/d into y/,\n-#         but B did that rename and still decided to put the file into z/,\n-#         so we probably shouldn't apply directory rename detection for it.\n-\n-test_setup_6b () {\n-\ttest_create_repo 6b &&\n+#   Expected: y/{b,c,d}\n+#   Alternate: y/{b,c}, z/d\n+#   Note: Directory rename detection says A renamed z/ -> y/, therefore the new\n+#         file 'z/d' in B should be moved to 'y/d'.\n+#\n+#         We could potentially ignore the renames of z/{b,c} on side A since\n+#         those were renamed on both sides.  However, it's a bit of a corner\n+#         case because what if there was also a z/e that side A moved to x/e\n+#         and side B left alone?  If we used the \"ignore renames done on both\n+#         sides\" logic, then we'd compute that A renamed z/ -> x/, and move\n+#         z/d to x/d.  That seems more surprising and uglier than allowing\n+#         the z/ -> y/ rename.\n+\n+test_setup_6b2 () {\n+\ttest_create_repo 6b2 &&\n \t(\n-\t\tcd 6b &&\n+\t\tcd 6b2 &&\n \n \t\tmkdir z &&\n \t\techo b >z/b &&\n@@ -1318,10 +1412,10 @@ test_setup_6b () {\n \t)\n }\n \n-test_expect_success '6b: Same rename done on both sides' '\n-\ttest_setup_6b &&\n+test_expect_failure '6b2: Same rename done on both sides' '\n+\ttest_setup_6b2 &&\n \t(\n-\t\tcd 6b &&\n+\t\tcd 6b2 &&\n \n \t\tgit checkout A^0 &&\n \n@@ -1335,7 +1429,7 @@ test_expect_success '6b: Same rename done on both sides' '\n \t\ttest_line_count = 1 out &&\n \n \t\tgit rev-parse >actual \\\n-\t\t\tHEAD:y/b HEAD:y/c HEAD:z/d &&\n+\t\t\tHEAD:y/b HEAD:y/c HEAD:y/d &&\n \t\tgit rev-parse >expect \\\n \t\t\tO:z/b    O:z/c    B:z/d &&\n \t\ttest_cmp expect actual\n@@ -1343,7 +1437,7 @@ test_expect_success '6b: Same rename done on both sides' '\n '\n \n # Testcase 6c, Rename only done on same side\n-#   (Related to testcases 6b and 8e)\n+#   (Related to testcases 6b1, 6b2, and 8e)\n #   Commit O: z/{b,c}\n #   Commit A: z/{b,c} (no change)\n #   Commit B: y/{b,c}, z/d\n@@ -2269,14 +2363,22 @@ test_expect_success '8d: rename/delete...or not?' '\n # Notes: In commit A, directory z got renamed to y.  In commit B, directory z\n #        did NOT get renamed; the directory is still present; instead it is\n #        considered to have just renamed a subset of paths in directory z\n-#        elsewhere.  However, this is much like testcase 6b (where commit B\n-#        moves all the original paths out of z/ but opted to keep d\n-#        within z/).  This makes it hard to judge where d should end up.\n+#        elsewhere.  This is much like testcase 6b2 (where commit B moves all\n+#        the original paths out of z/ but opted to keep d within z/).\n+#\n+#        It was not clear in the past what should be done with this testcase;\n+#        in fact, I noted that I \"just picked one\" previously.  However,\n+#        following the new logic for testcase 6b2, we should take the rename\n+#        and move z/d to y/d.\n #\n-#        It's possible that users would get confused about this, but what\n-#        should we do instead?  It's not at all clear to me whether z/d or\n-#        y/d or something else is a better resolution here, and other cases\n-#        start getting really tricky, so I just picked one.\n+#        6b1, 6b2, and this case are definitely somewhat fuzzy in terms of\n+#        whether they are optimal for end users, but (a) the default for\n+#        directory rename detection is to mark these all as conflicts\n+#        anyway, (b) it feels like this is less prone to higher order corner\n+#        case confusion, and (c) the current algorithm requires less global\n+#        knowledge (i.e. less coupling in the algorithm between renames done\n+#        on both sides) which thus means users are better able to predict\n+#        the behavior, and predict it without computing as many details.\n \n test_setup_8e () {\n \ttest_create_repo 8e &&\n-- \ngitgitgadget\n\n"},{"id":"407681","messageId":"531942d353758305e29879654b93f4ba3dcbcc63.1602794791.git.gitgitgadget@gmail.com","threadId":"54433","inReplyTo":"pull.879.git.git.1602794790.gitgitgadget@gmail.com","subject":"[PATCH 4/4] t6423: more involved rules for renaming directories into each other","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-15T20:46:30Z","receivedAt":"2020-10-15T20:46:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nTestcases 12b and 12c were both slightly weird; they were marked as\nhaving a weird resolution, but with the note that even straightforward\nsimple rules can give weird results when the input is bizarre.\n\nHowever, during optimization work for merge-ort, I discovered a\nsignificant speedup that is possible if we add one more fairly\nstraightforward rule: we don't bother doing directory rename detection\nif there are no new files added to the directory on the other side of\nthe history to be affected by the directory rename.  This seems like an\nobvious and straightforward rule, but there was one funny corner case\nwhere directory rename detection could affect only existing files: the\nfunny corner case where two directories are renamed into each other on\nopposite sides of history.  In other words, it only results in a\ndifferent output for testcases 12b and 12c.\n\nSince we already thought testcases 12b and 12c were weird anyway, and\nbecause the optimization often has a significant effect on common cases\n(but is entirely prevented if we can't change how 12b and 12c function),\nlet's add the additional rule and tweak how 12b and 12c work.  Split\nboth testcases into two (one where we add no new files, and one where\nthe side that doesn't rename a given directory will add files to it),\nand mark them with the new expectation.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n .../technical/directory-rename-detection.txt  |   3 +\n t/t6423-merge-rename-directories.sh           | 253 ++++++++++++++++--\n 2 files changed, 230 insertions(+), 26 deletions(-)\n\ndiff --git a/Documentation/technical/directory-rename-detection.txt b/Documentation/technical/directory-rename-detection.txt\nindex 5d03539412..49b83ef3cc 100644\n--- a/Documentation/technical/directory-rename-detection.txt\n+++ b/Documentation/technical/directory-rename-detection.txt\n@@ -73,6 +73,9 @@ additional rules:\n   b) Only apply implicit directory renames to directories if the other side\n      of history is the one doing the renaming.\n \n+  c) Do not perform directory rename detection for directories which had no\n+     new paths added to them.\n+\n Limitations -- support in different commands\n --------------------------------------------\n \ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 00eac6e9a2..06b46af765 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -4049,31 +4049,124 @@ test_expect_success '12a: Moving one directory hierarchy into another' '\n \t)\n '\n \n-# Testcase 12b, Moving two directory hierarchies into each other\n+# Testcase 12b1, Moving two directory hierarchies into each other\n #   (Related to testcases 1c and 12c)\n #   Commit O: node1/{leaf1, leaf2}, node2/{leaf3, leaf4}\n #   Commit A: node1/{leaf1, leaf2, node2/{leaf3, leaf4}}\n #   Commit B: node2/{leaf3, leaf4, node1/{leaf1, leaf2}}\n-#   Expected: node1/node2/node1/{leaf1, leaf2},\n+#   Expected: node1/node2/{leaf3, leaf4}\n+#             node2/node1/{leaf1, leaf2}\n+#   NOTE: If there were new files added to the old node1/ or node2/ directories,\n+#         then we would need to detect renames for those directories and would\n+#         find that:\n+#             commit A renames node2/ -> node1/node2/\n+#             commit B renames node1/ -> node2/node1/\n+#         Applying those directory renames to the initial result (making all\n+#         four paths experience a transitive renaming), yields\n+#             node1/node2/node1/{leaf1, leaf2}\n #             node2/node1/node2/{leaf3, leaf4}\n+#         as the result.  It may be really weird to have two directories\n+#         rename each other, but simple rules give weird results when given\n+#         weird inputs.  HOWEVER, the \"If\" at the beginning of those NOTE was\n+#         false; there were no new files added and thus there is no directory\n+#         rename detection to perform.  As such, we just have simple renames\n+#         and the expected answer is:\n+#             node1/node2/{leaf3, leaf4}\n+#             node2/node1/{leaf1, leaf2}\n+\n+test_setup_12b1 () {\n+\ttest_create_repo 12b1 &&\n+\t(\n+\t\tcd 12b1 &&\n+\n+\t\tmkdir -p node1 node2 &&\n+\t\techo leaf1 >node1/leaf1 &&\n+\t\techo leaf2 >node1/leaf2 &&\n+\t\techo leaf3 >node2/leaf3 &&\n+\t\techo leaf4 >node2/leaf4 &&\n+\t\tgit add node1 node2 &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout A &&\n+\t\tgit mv node2/ node1/ &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit checkout B &&\n+\t\tgit mv node1/ node2/ &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"B\"\n+\t)\n+}\n+\n+test_expect_failure '12b1: Moving two directory hierarchies into each other' '\n+\ttest_setup_12b1 &&\n+\t(\n+\t\tcd 12b1 &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\tgit -c merge.directoryRenames=true merge -s recursive B^0 &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 4 out &&\n+\n+\t\tgit rev-parse >actual \\\n+\t\t\tHEAD:node2/node1/leaf1 \\\n+\t\t\tHEAD:node2/node1/leaf2 \\\n+\t\t\tHEAD:node1/node2/leaf3 \\\n+\t\t\tHEAD:node1/node2/leaf4 &&\n+\t\tgit rev-parse >expect \\\n+\t\t\tO:node1/leaf1 \\\n+\t\t\tO:node1/leaf2 \\\n+\t\t\tO:node2/leaf3 \\\n+\t\t\tO:node2/leaf4 &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+# Testcase 12b2, Moving two directory hierarchies into each other\n+#   (Related to testcases 1c and 12c)\n+#   Commit O: node1/{leaf1, leaf2}, node2/{leaf3, leaf4}\n+#   Commit A: node1/{leaf1, leaf2, leaf5, node2/{leaf3, leaf4}}\n+#   Commit B: node2/{leaf3, leaf4, leaf6, node1/{leaf1, leaf2}}\n+#   Expected: node1/node2/{node1/{leaf1, leaf2}, leaf6}\n+#             node2/node1/{node2/{leaf3, leaf4}, leaf5}\n #   NOTE: Without directory renames, we would expect\n-#                   node2/node1/{leaf1, leaf2},\n-#                   node1/node2/{leaf3, leaf4}\n+#             A: node2/leaf3 -> node1/node2/leaf3\n+#             A: node2/leaf1 -> node1/node2/leaf4\n+#             A: Adds           node1/leaf5\n+#             B: node1/leaf1 -> node2/node1/leaf1\n+#             B: node1/leaf2 -> node2/node1/leaf2\n+#             B: Adds           node2/leaf6\n #         with directory rename detection, we note that\n #             commit A renames node2/ -> node1/node2/\n #             commit B renames node1/ -> node2/node1/\n-#         therefore, applying those directory renames to the initial result\n-#         (making all four paths experience a transitive renaming), yields\n-#         the expected result.\n+#         therefore, applying A's directory rename to the paths added in B gives:\n+#             B: node1/leaf1 -> node1/node2/node1/leaf1\n+#             B: node1/leaf2 -> node1/node2/node1/leaf2\n+#             B: Adds           node1/node2/leaf6\n+#         and applying B's directory rename to the paths added in A gives:\n+#             A: node2/leaf3 -> node2/node1/node2/leaf3\n+#             A: node2/leaf1 -> node2/node1/node2/leaf4\n+#             A: Adds           node2/node1/leaf5\n+#         resulting in the expected\n+#             node1/node2/{node1/{leaf1, leaf2}, leaf6}\n+#             node2/node1/{node2/{leaf3, leaf4}, leaf5}\n #\n #         You may ask, is it weird to have two directories rename each other?\n #         To which, I can do no more than shrug my shoulders and say that\n #         even simple rules give weird results when given weird inputs.\n \n-test_setup_12b () {\n-\ttest_create_repo 12b &&\n+test_setup_12b2 () {\n+\ttest_create_repo 12b2 &&\n \t(\n-\t\tcd 12b &&\n+\t\tcd 12b2 &&\n \n \t\tmkdir -p node1 node2 &&\n \t\techo leaf1 >node1/leaf1 &&\n@@ -4090,43 +4183,51 @@ test_setup_12b () {\n \n \t\tgit checkout A &&\n \t\tgit mv node2/ node1/ &&\n+\t\techo leaf5 >node1/leaf5 &&\n+\t\tgit add node1/leaf5 &&\n \t\ttest_tick &&\n \t\tgit commit -m \"A\" &&\n \n \t\tgit checkout B &&\n \t\tgit mv node1/ node2/ &&\n+\t\techo leaf6 >node2/leaf6 &&\n+\t\tgit add node2/leaf6 &&\n \t\ttest_tick &&\n \t\tgit commit -m \"B\"\n \t)\n }\n \n-test_expect_success '12b: Moving two directory hierarchies into each other' '\n-\ttest_setup_12b &&\n+test_expect_success '12b2: Moving two directory hierarchies into each other' '\n+\ttest_setup_12b2 &&\n \t(\n-\t\tcd 12b &&\n+\t\tcd 12b2 &&\n \n \t\tgit checkout A^0 &&\n \n \t\tgit -c merge.directoryRenames=true merge -s recursive B^0 &&\n \n \t\tgit ls-files -s >out &&\n-\t\ttest_line_count = 4 out &&\n+\t\ttest_line_count = 6 out &&\n \n \t\tgit rev-parse >actual \\\n \t\t\tHEAD:node1/node2/node1/leaf1 \\\n \t\t\tHEAD:node1/node2/node1/leaf2 \\\n \t\t\tHEAD:node2/node1/node2/leaf3 \\\n-\t\t\tHEAD:node2/node1/node2/leaf4 &&\n+\t\t\tHEAD:node2/node1/node2/leaf4 \\\n+\t\t\tHEAD:node2/node1/leaf5       \\\n+\t\t\tHEAD:node1/node2/leaf6       &&\n \t\tgit rev-parse >expect \\\n \t\t\tO:node1/leaf1 \\\n \t\t\tO:node1/leaf2 \\\n \t\t\tO:node2/leaf3 \\\n-\t\t\tO:node2/leaf4 &&\n+\t\t\tO:node2/leaf4 \\\n+\t\t\tA:node1/leaf5 \\\n+\t\t\tB:node2/leaf6 &&\n \t\ttest_cmp expect actual\n \t)\n '\n \n-# Testcase 12c, Moving two directory hierarchies into each other w/ content merge\n+# Testcase 12c1, Moving two directory hierarchies into each other w/ content merge\n #   (Related to testcase 12b)\n #   Commit O: node1/{       leaf1_1, leaf2_1}, node2/{leaf3_1, leaf4_1}\n #   Commit A: node1/{       leaf1_2, leaf2_2,  node2/{leaf3_2, leaf4_2}}\n@@ -4134,13 +4235,13 @@ test_expect_success '12b: Moving two directory hierarchies into each other' '\n #   Expected: Content merge conflicts for each of:\n #               node1/node2/node1/{leaf1, leaf2},\n #               node2/node1/node2/{leaf3, leaf4}\n-#   NOTE: This is *exactly* like 12c, except that every path is modified on\n+#   NOTE: This is *exactly* like 12b1, except that every path is modified on\n #         each side of the merge.\n \n-test_setup_12c () {\n-\ttest_create_repo 12c &&\n+test_setup_12c1 () {\n+\ttest_create_repo 12c1 &&\n \t(\n-\t\tcd 12c &&\n+\t\tcd 12c1 &&\n \n \t\tmkdir -p node1 node2 &&\n \t\tprintf \"1\\n2\\n3\\n4\\n5\\n6\\n7\\n8\\nleaf1\\n\" >node1/leaf1 &&\n@@ -4171,10 +4272,10 @@ test_setup_12c () {\n \t)\n }\n \n-test_expect_success '12c: Moving one directory hierarchy into another w/ content merge' '\n-\ttest_setup_12c &&\n+test_expect_failure '12c1: Moving one directory hierarchy into another w/ content merge' '\n+\ttest_setup_12c1 &&\n \t(\n-\t\tcd 12c &&\n+\t\tcd 12c1 &&\n \n \t\tgit checkout A^0 &&\n \n@@ -4183,6 +4284,102 @@ test_expect_success '12c: Moving one directory hierarchy into another w/ content\n \t\tgit ls-files -u >out &&\n \t\ttest_line_count = 12 out &&\n \n+\t\tgit rev-parse >actual \\\n+\t\t\t:1:node2/node1/leaf1 \\\n+\t\t\t:1:node2/node1/leaf2 \\\n+\t\t\t:1:node1/node2/leaf3 \\\n+\t\t\t:1:node1/node2/leaf4 \\\n+\t\t\t:2:node2/node1/leaf1 \\\n+\t\t\t:2:node2/node1/leaf2 \\\n+\t\t\t:2:node1/node2/leaf3 \\\n+\t\t\t:2:node1/node2/leaf4 \\\n+\t\t\t:3:node2/node1/leaf1 \\\n+\t\t\t:3:node2/node1/leaf2 \\\n+\t\t\t:3:node1/node2/leaf3 \\\n+\t\t\t:3:node1/node2/leaf4 &&\n+\t\tgit rev-parse >expect \\\n+\t\t\tO:node1/leaf1 \\\n+\t\t\tO:node1/leaf2 \\\n+\t\t\tO:node2/leaf3 \\\n+\t\t\tO:node2/leaf4 \\\n+\t\t\tA:node1/leaf1 \\\n+\t\t\tA:node1/leaf2 \\\n+\t\t\tA:node1/node2/leaf3 \\\n+\t\t\tA:node1/node2/leaf4 \\\n+\t\t\tB:node2/node1/leaf1 \\\n+\t\t\tB:node2/node1/leaf2 \\\n+\t\t\tB:node2/leaf3 \\\n+\t\t\tB:node2/leaf4 &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+# Testcase 12c2, Moving two directory hierarchies into each other w/ content merge\n+#   (Related to testcase 12b)\n+#   Commit O: node1/{       leaf1_1, leaf2_1}, node2/{leaf3_1, leaf4_1}\n+#   Commit A: node1/{       leaf1_2, leaf2_2,  node2/{leaf3_2, leaf4_2}, leaf5}\n+#   Commit B: node2/{node1/{leaf1_3, leaf2_3},        leaf3_3, leaf4_3,  leaf6}\n+#   Expected: Content merge conflicts for each of:\n+#               node1/node2/node1/{leaf1, leaf2}\n+#               node2/node1/node2/{leaf3, leaf4}\n+#             plus\n+#               node2/node1/leaf5\n+#               node1/node2/leaf6\n+#   NOTE: This is *exactly* like 12b2, except that every path from O is modified\n+#         on each side of the merge.\n+\n+test_setup_12c2 () {\n+\ttest_create_repo 12c2 &&\n+\t(\n+\t\tcd 12c2 &&\n+\n+\t\tmkdir -p node1 node2 &&\n+\t\tprintf \"1\\n2\\n3\\n4\\n5\\n6\\n7\\n8\\nleaf1\\n\" >node1/leaf1 &&\n+\t\tprintf \"1\\n2\\n3\\n4\\n5\\n6\\n7\\n8\\nleaf2\\n\" >node1/leaf2 &&\n+\t\tprintf \"1\\n2\\n3\\n4\\n5\\n6\\n7\\n8\\nleaf3\\n\" >node2/leaf3 &&\n+\t\tprintf \"1\\n2\\n3\\n4\\n5\\n6\\n7\\n8\\nleaf4\\n\" >node2/leaf4 &&\n+\t\tgit add node1 node2 &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout A &&\n+\t\tgit mv node2/ node1/ &&\n+\t\tfor i in `git ls-files`; do echo side A >>$i; done &&\n+\t\tgit add -u &&\n+\t\techo leaf5 >node1/leaf5 &&\n+\t\tgit add node1/leaf5 &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit checkout B &&\n+\t\tgit mv node1/ node2/ &&\n+\t\tfor i in `git ls-files`; do echo side B >>$i; done &&\n+\t\tgit add -u &&\n+\t\techo leaf6 >node2/leaf6 &&\n+\t\tgit add node2/leaf6 &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"B\"\n+\t)\n+}\n+\n+test_expect_success '12c2: Moving one directory hierarchy into another w/ content merge' '\n+\ttest_setup_12c2 &&\n+\t(\n+\t\tcd 12c2 &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\ttest_must_fail git -c merge.directoryRenames=true merge -s recursive B^0 &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 14 out &&\n+\t\tgit ls-files -u >out &&\n+\t\ttest_line_count = 12 out &&\n+\n \t\tgit rev-parse >actual \\\n \t\t\t:1:node1/node2/node1/leaf1 \\\n \t\t\t:1:node1/node2/node1/leaf2 \\\n@@ -4195,7 +4392,9 @@ test_expect_success '12c: Moving one directory hierarchy into another w/ content\n \t\t\t:3:node1/node2/node1/leaf1 \\\n \t\t\t:3:node1/node2/node1/leaf2 \\\n \t\t\t:3:node2/node1/node2/leaf3 \\\n-\t\t\t:3:node2/node1/node2/leaf4 &&\n+\t\t\t:3:node2/node1/node2/leaf4 \\\n+\t\t\t:0:node2/node1/leaf5       \\\n+\t\t\t:0:node1/node2/leaf6       &&\n \t\tgit rev-parse >expect \\\n \t\t\tO:node1/leaf1 \\\n \t\t\tO:node1/leaf2 \\\n@@ -4208,7 +4407,9 @@ test_expect_success '12c: Moving one directory hierarchy into another w/ content\n \t\t\tB:node2/node1/leaf1 \\\n \t\t\tB:node2/node1/leaf2 \\\n \t\t\tB:node2/leaf3 \\\n-\t\t\tB:node2/leaf4 &&\n+\t\t\tB:node2/leaf4 \\\n+\t\t\tA:node1/leaf5 \\\n+\t\t\tB:node2/leaf6 &&\n \t\ttest_cmp expect actual\n \t)\n '\n-- \ngitgitgadget\n"},{"id":"407679","messageId":"03af0de2b689f94a5b464ba1618857f55cb8b1bf.1602794791.git.gitgitgadget@gmail.com","threadId":"54433","inReplyTo":"pull.879.git.git.1602794790.gitgitgadget@gmail.com","subject":"[PATCH 1/4] directory-rename-detection.txt: update references to regression tests","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-15T20:46:27Z","receivedAt":"2020-10-15T20:46:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nThe regression tests for directory rename detection were renamed from\nt6043 to t6423 in commit 919df31955 (\"Collect merge-related tests to\nt64xx\", 2020-08-10); update this file to match.  Also, add a small\nclarification to nearby text while we're at it.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n Documentation/technical/directory-rename-detection.txt | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/directory-rename-detection.txt b/Documentation/technical/directory-rename-detection.txt\nindex 844629c8c4..ce042cfcae 100644\n--- a/Documentation/technical/directory-rename-detection.txt\n+++ b/Documentation/technical/directory-rename-detection.txt\n@@ -18,7 +18,8 @@ It is perhaps easiest to start with an example:\n More interesting possibilities exist, though, such as:\n \n   * one side of history renames x -> z, and the other renames some file to\n-    x/e, causing the need for the merge to do a transitive rename.\n+    x/e, causing the need for the merge to do a transitive rename so that\n+    the rename ends up at z/e.\n \n   * one side of history renames x -> z, but also renames all files within x.\n     For example, x/a -> z/alpha, x/b -> z/bravo, etc.\n@@ -35,7 +36,7 @@ More interesting possibilities exist, though, such as:\n     directory itself contained inner directories that were renamed to yet\n     other locations).\n \n-  * combinations of the above; see t/t6043-merge-rename-directories.sh for\n+  * combinations of the above; see t/t6423-merge-rename-directories.sh for\n     various interesting cases.\n \n Limitations -- applicability of directory renames\n@@ -62,7 +63,7 @@ directory rename detection applies:\n Limitations -- detailed rules and testcases\n -------------------------------------------\n \n-t/t6043-merge-rename-directories.sh contains extensive tests and commentary\n+t/t6423-merge-rename-directories.sh contains extensive tests and commentary\n which generate and explore the rules listed above.  It also lists a few\n additional rules:\n \n-- \ngitgitgadget\n\n"},{"id":"407680","messageId":"pull.879.git.git.1602794790.gitgitgadget@gmail.com","threadId":"54433","inReplyTo":null,"subject":"[PATCH 0/4] Directory rename detection testcases and rules","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-15T20:46:26Z","receivedAt":"2020-10-15T20:46:42Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"While testing merge-ort on some real world repositories a little while back,\nI found some issues with directory rename detection...some of which were\nissues in the current merge-recursive as well. Also, I found that there was\na nice optimization I could use if a new obvious-looking rule was added,\nthough it has one slight side effect to one corner case. Fixing the bugs and\nimplementing the new rules is a bit more involved, so for now this series\njust updates the rule descriptions and adds or modifies tests to document\nthe various cases.\n\nElijah Newren (4):\n  directory-rename-detection.txt: update references to regression tests\n  t6423: more involved directory rename test\n  t6423: update directory rename detection tests with new rule\n  t6423: more involved rules for renaming directories into each other\n\n .../technical/directory-rename-detection.txt  |  15 +-\n t/t6423-merge-rename-directories.sh           | 592 ++++++++++++++++--\n 2 files changed, 553 insertions(+), 54 deletions(-)\n\n\nbase-commit: d4a392452e292ff924e79ec8458611c0f679d6d4\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-879%2Fnewren%2Fdrd-testcases-and-rules-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-879/newren/drd-testcases-and-rules-v1\nPull-Request: https://github.com/git/git/pull/879\n-- \ngitgitgadget\n"},{"id":"407682","messageId":"c9637d70caed8a265096fa701833eca3ed52d0f3.1602794791.git.gitgitgadget@gmail.com","threadId":"54433","inReplyTo":"pull.879.git.git.1602794790.gitgitgadget@gmail.com","subject":"[PATCH 2/4] t6423: more involved directory rename test","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-15T20:46:28Z","receivedAt":"2020-10-15T20:46:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nAdd a new testcase modelled on a real world repository example that\nserved multiple purposes:\n  * it uncovered a bug in the current directory rename detection\n    implementation.\n  * it is a good test of needing to do directory rename detection for\n    a series of commits instead of just one (and uses rebase instead\n    of just merge like all the other tests in this testfile).\n  * it is an excellent stress test for some of the optimizations in\n    my new merge-ort engine\n\nI can expand on the final item later when I have submitted more of\nmerge-ort, but the bug is the main immediate concern.  It arises as\nfollows:\n\n  * dir/subdir/ has several files\n  * almost all files in dir/subdir/ are renamed to folder/subdir/\n  * one of the files in dir/subdir/ is renamed to folder/subdir/newsubdir/\n  * If the other side of history (that doesn't do the renames) adds a\n    new file to dir/subdir/, where should it be placed after the merge?\n\nThe most obvious two choices are: (1) leave the new file in dir/subdir/,\ndon't make it follow the rename, and (2) move the new file to\nfolder/subdir/, following the rename of most the files.  However,\nthere's a possible third choice here: (3) move the new file to\nfolder/subdir/newsubdir/.  The choice reinforce the fact that\nmerge.directoryRenames=conflict is a good default, but when the merge\nmachinery needs to stick it somewhere and notify the user of the\npossibility that they might want to place it elsewhere.  Surprisingly,\nthe current code would always choose (3), while the real world\nrepository was clearly expecting (2) -- move the file along with where\nthe herd of files was going, not with the special exception.\n\nThe problem here is that for the majority of the file renames,\n   dir/subdir/ -> folder/subdir/\nis actually represented as\n   dir/ -> folder/\nThis directory rename would have a big weight associated with it since\nmost the files followed that rename.  However, we always consult the\nmost immediate directory first, and there is only one rename rule for\nit:\n   dir/subdir/ -> folder/subdir/newsubdir/\nSince this rule is the only one for mapping from dir/subdir/, it\nautomatically wins and that directory rename was followed instead of the\ndesired dir/subdir/ -> folder/subdir/.\n\nUnfortunately, the fix is a bit involved so for now just add the\ntestcase documenting the issue.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t6423-merge-rename-directories.sh | 195 ++++++++++++++++++++++++++++\n 1 file changed, 195 insertions(+)\n\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex f7ecbb886d..31aea47522 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -4227,6 +4227,201 @@ test_expect_success '12e: Rename/merge subdir into the root, variant 2' '\n \t)\n '\n \n+# Testcase 12f, Rebase of patches with big directory rename\n+#   Commit O:\n+#              dir/subdir/{a,b,c,d,e_O,Makefile_TOP_O}\n+#              dir/subdir/tweaked/{f,g,h,Makefile_SUB_O}\n+#              dir/unchanged/<LOTS OF FILES>\n+#   Commit A:\n+#     (Remove f & g, move e into newsubdir, rename dir/->folder/, modify files)\n+#              folder/subdir/{a,b,c,d,Makefile_TOP_A}\n+#              folder/subdir/newsubdir/e_A\n+#              folder/subdir/tweaked/{h,Makefile_SUB_A}\n+#              folder/unchanged/<LOTS OF FILES>\n+#   Commit B1:\n+#     (add newfile.{c,py}, modify underscored files)\n+#              dir/{a,b,c,d,e_B1,Makefile_TOP_B1,newfile.c}\n+#              dir/tweaked/{f,g,h,Makefile_SUB_B1,newfile.py}\n+#              dir/unchanged/<LOTS OF FILES>\n+#   Commit B2:\n+#     (Modify e further, add newfile.rs)\n+#              dir/{a,b,c,d,e_B2,Makefile_TOP_B1,newfile.c,newfile.rs}\n+#              dir/tweaked/{f,g,h,Makefile_SUB_B1,newfile.py}\n+#              dir/unchanged/<LOTS OF FILES>\n+#   Expected:\n+#          B1-picked:\n+#              folder/subdir/{a,b,c,d,Makefile_TOP_Merge1,newfile.c}\n+#              folder/subdir/newsubdir/e_Merge1\n+#              folder/subdir/tweaked/{h,Makefile_SUB_Merge1,newfile.py}\n+#              folder/unchanged/<LOTS OF FILES>\n+#          B2-picked:\n+#              folder/subdir/{a,b,c,d,Makefile_TOP_Merge1,newfile.c,newfile.rs}\n+#              folder/subdir/newsubdir/e_Merge2\n+#              folder/subdir/tweaked/{h,Makefile_SUB_Merge1,newfile.py}\n+#              folder/unchanged/<LOTS OF FILES>\n+#\n+# Notes: This testcase happens to exercise lots of the\n+#        optimization-specific codepaths in merge-ort, and also\n+#        demonstrated a failing of the directory rename detection algorithm\n+#        in merge-recursive; newfile.c should not get pushed into\n+#        folder/subdir/newsubdir/, yet merge-recursive put it there because\n+#        the rename of dir/subdir/{a,b,c,d} -> folder/subdir/{a,b,c,d}\n+#        looks like\n+#            dir/ -> folder/,\n+#        whereas the rename of dir/subdir/e -> folder/subdir/newsubdir/e\n+#        looks like\n+#            dir/subdir/ -> folder/subdir/newsubdir/\n+#        and if we note that newfile.c is found in dir/subdir/, we might\n+#        overlook the dir/ -> folder/ rule that has more weight.\n+\n+test_setup_12f () {\n+\ttest_create_repo 12f &&\n+\t(\n+\t\tcd 12f &&\n+\n+\t\tmkdir -p dir/unchanged &&\n+\t\tmkdir -p dir/subdir/tweaked &&\n+\t\techo a >dir/subdir/a &&\n+\t\techo b >dir/subdir/b &&\n+\t\techo c >dir/subdir/c &&\n+\t\techo d >dir/subdir/d &&\n+\t\ttest_seq 1 10 >dir/subdir/e &&\n+\t\ttest_seq 10 20 >dir/subdir/Makefile &&\n+\t\techo f >dir/subdir/tweaked/f &&\n+\t\techo g >dir/subdir/tweaked/g &&\n+\t\techo h >dir/subdir/tweaked/h &&\n+\t\ttest_seq 20 30 >dir/subdir/tweaked/Makefile &&\n+\t\tfor i in `test_seq 1 88`; do\n+\t\t\techo content $i >dir/unchanged/file_$i\n+\t\tdone &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit switch A &&\n+\t\tgit rm dir/subdir/tweaked/f dir/subdir/tweaked/g &&\n+\t\ttest_seq 2 10 >dir/subdir/e &&\n+\t\ttest_seq 11 20 >dir/subdir/Makefile &&\n+\t\ttest_seq 21 30 >dir/subdir/tweaked/Makefile &&\n+\t\tmkdir dir/subdir/newsubdir &&\n+\t\tgit mv dir/subdir/e dir/subdir/newsubdir/ &&\n+\t\tgit mv dir folder &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit switch B &&\n+\t\tmkdir dir/subdir/newsubdir/ &&\n+\t\techo c code >dir/subdir/newfile.c &&\n+\t\techo python code >dir/subdir/newsubdir/newfile.py &&\n+\t\ttest_seq 1 11 >dir/subdir/e &&\n+\t\ttest_seq 10 21 >dir/subdir/Makefile &&\n+\t\ttest_seq 20 31 >dir/subdir/tweaked/Makefile &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"B1\" &&\n+\n+\t\techo rust code >dir/subdir/newfile.rs &&\n+\t\ttest_seq 1 12 >dir/subdir/e &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"B2\"\n+\t)\n+}\n+\n+test_expect_failure '12f: Trivial directory resolve, caching, all kinds of fun' '\n+\ttest_setup_12f &&\n+\t(\n+\t\tcd 12f &&\n+\n+\t\tgit checkout A^0 &&\n+\t\tgit branch Bmod B &&\n+\n+\t\tgit -c merge.directoryRenames=true rebase A Bmod &&\n+\n+\t\techo Checking the pick of B1... &&\n+\n+\t\ttest_must_fail git rev-parse Bmod~1:dir &&\n+\n+\t\tgit ls-tree -r Bmod~1 >out &&\n+\t\ttest_line_count = 98 out &&\n+\n+\t\tgit diff --name-status A Bmod~1 >actual &&\n+\t\tq_to_tab >expect <<-\\EOF &&\n+\t\tMQfolder/subdir/Makefile\n+\t\tAQfolder/subdir/newfile.c\n+\t\tMQfolder/subdir/newsubdir/e\n+\t\tAQfolder/subdir/newsubdir/newfile.py\n+\t\tMQfolder/subdir/tweaked/Makefile\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# Three-way merged files\n+\t\ttest_seq  2 11 >e_Merge1 &&\n+\t\ttest_seq 11 21 >Makefile_TOP &&\n+\t\ttest_seq 21 31 >Makefile_SUB &&\n+\t\tgit hash-object >expect      \\\n+\t\t\te_Merge1             \\\n+\t\t\tMakefile_TOP         \\\n+\t\t\tMakefile_SUB         &&\n+\t\tgit rev-parse >actual              \\\n+\t\t\tBmod~1:folder/subdir/newsubdir/e     \\\n+\t\t\tBmod~1:folder/subdir/Makefile        \\\n+\t\t\tBmod~1:folder/subdir/tweaked/Makefile &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# New files showed up at the right location with right contents\n+\t\tgit rev-parse >expect                \\\n+\t\t\tB~1:dir/subdir/newfile.c            \\\n+\t\t\tB~1:dir/subdir/newsubdir/newfile.py &&\n+\t\tgit rev-parse >actual                      \\\n+\t\t\tBmod~1:folder/subdir/newfile.c            \\\n+\t\t\tBmod~1:folder/subdir/newsubdir/newfile.py &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# Removed files\n+\t\ttest_path_is_missing folder/subdir/tweaked/f &&\n+\t\ttest_path_is_missing folder/subdir/tweaked/g &&\n+\n+\t\t# Unchanged files or directories\n+\t\tgit rev-parse >actual        \\\n+\t\t\tBmod~1:folder/subdir/a          \\\n+\t\t\tBmod~1:folder/subdir/b          \\\n+\t\t\tBmod~1:folder/subdir/c          \\\n+\t\t\tBmod~1:folder/subdir/d          \\\n+\t\t\tBmod~1:folder/unchanged         \\\n+\t\t\tBmod~1:folder/subdir/tweaked/h &&\n+\t\tgit rev-parse >expect          \\\n+\t\t\tO:dir/subdir/a         \\\n+\t\t\tO:dir/subdir/b         \\\n+\t\t\tO:dir/subdir/c         \\\n+\t\t\tO:dir/subdir/d         \\\n+\t\t\tO:dir/unchanged        \\\n+\t\t\tO:dir/subdir/tweaked/h &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\techo Checking the pick of B2... &&\n+\n+\t\ttest_must_fail git rev-parse Bmod:dir &&\n+\n+\t\tgit ls-tree -r Bmod >out &&\n+\t\ttest_line_count = 99 out &&\n+\n+\t\tgit diff --name-status Bmod~1 Bmod >actual &&\n+\t\tq_to_tab >expect <<-\\EOF &&\n+\t\tAQfolder/subdir/newfile.rs\n+\t\tMQfolder/subdir/newsubdir/e\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# Three-way merged file\n+\t\ttest_seq  2 12 >e_Merge2 &&\n+\t\tgit hash-object e_Merge2 >expect &&\n+\t\tgit rev-parse Bmod:folder/subdir/newsubdir/e >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n ###########################################################################\n # SECTION 13: Checking informational and conflict messages\n #\n-- \ngitgitgadget\n\n"},{"id":"407684","messageId":"CAPig+cQ=WjqfN68GcpaJBC4zjP_q2KnY9k15ErWkFM08TyCVjA@mail.gmail.com","threadId":"54433","inReplyTo":"c9637d70caed8a265096fa701833eca3ed52d0f3.1602794791.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] t6423: more involved directory rename test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-15T20:57:35Z","receivedAt":"2020-10-15T20:57:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Oct 15, 2020 at 4:46 PM Elijah Newren via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> Add a new testcase modelled on a real world repository example that\n> served multiple purposes:\n\nA couple minor style nits I noticed while quickly running my eye over\nthe patch...\n\n> +               for i in `test_seq 1 88`; do\n> +                       echo content $i >dir/unchanged/file_$i\n> +               done &&\n\n'do' on its own line, and prefer $(...) over `...`:\n\n    for i in $(test_seq 1 88)\n    do\n        echo content $i >dir/unchanged/file_$i\n    done &&\n\n(Not necessarily worth a re-roll, though.)\n"}]}