{"thread":{"id":"52529","subject":"[PATCH 00/16] t: replace incorrect test_must_fail usage (part 2)","startedAt":"2019-12-27T13:47:38Z","lastAt":"2020-01-26T20:23:25Z","messageCount":54,"participants":["Denton Liu","Johannes Sixt","Eric Sunshine","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":16},"messages":[{"id":"388970","messageId":"cover.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":null,"subject":"[PATCH 00/16] t: replace incorrect test_must_fail usage (part 2)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:09Z","receivedAt":"2019-12-27T13:47:38Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The overall scope of these patches is to replace inappropriate uses of\ntest_must_fail. IOW, we should only allow test_must_fail to run on `git`\nand `test-tool`. Ultimately, we will conclude by making test_must_fail\nerror out on non-git commands. An advance view of the final series can\nbe found here[1].\n\nThis is the second part. It focuses on t[234]*.sh. The first part can be\nfound here[2].\n\n[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2\n[2]: https://lore.kernel.org/git/cover.1576583819.git.liu.denton@gmail.com/\n\nDenton Liu (16):\n t2018: remove trailing space from test description\n t2018: add space between function name and ()\n t2018: use test_must_fail for failing git commands\n t2018: teach do_checkout() to accept `!` arg\n t2018: don't lose return code of git commands\n t2018: replace \"sha\" with \"oid\"\n t3030: use test_path_is_missing()\n t3310: extract common no_notes_merge_left()\n t3415: stop losing return codes of git commands\n t3415: increase granularity of test_auto_{fixup,squash}()\n t3419: stop losing return code of git command\n t3504: don't use `test_must_fail test_cmp`\n t3507: fix indentation\n t3507: use test_path_is_missing()\n t4124: only mark git command with test_must_fail\n t4124: let sed open its own files\n\n t/t2018-checkout-branch.sh | 71 ++++++++----\n t/t3030-merge-recursive.sh | 2 +-\n t/t3310-notes-merge-manual-resolve.sh | 21 ++--\n t/t3415-rebase-autosquash.sh | 153 +++++++++++++++++++-------\n t/t3419-rebase-patch-id.sh | 3 +-\n t/t3504-cherry-pick-rerere.sh | 2 +-\n t/t3507-cherry-pick-conflict.sh | 28 ++---\n t/t4124-apply-ws-rule.sh | 12 +-\n 8 files changed, 198 insertions(+), 94 deletions(-)\n\n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388971","messageId":"a0199f1e48b88a93ec181113752fab132b22fa91.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 01/16] t2018: remove trailing space from test description","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:10Z","receivedAt":"2019-12-27T13:47:39Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Signed-off-by: Denton Liu \n---\n t/t2018-checkout-branch.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 822381dd9d..e18abce3d2 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-test_description='checkout '\n+test_description='checkout'\n \n . ./test-lib.sh\n \n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388972","messageId":"9558d2491fa8dc68603426c1528154b4fe352650.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 02/16] t2018: add space between function name and ()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:11Z","receivedAt":"2019-12-27T13:47:39Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Signed-off-by: Denton Liu \n---\n t/t2018-checkout-branch.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex e18abce3d2..79b16e4677 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -12,7 +12,7 @@ test_description='checkout'\n # 2) HEAD is ; if is not specified, the old HEAD is used.\n #\n # If is not specified, \"git checkout\" is run with -b.\n-do_checkout() {\n+do_checkout () {\n \texp_branch=$1 &&\n \texp_ref=\"refs/heads/$exp_branch\" &&\n \n@@ -32,19 +32,19 @@ do_checkout() {\n \ttest $exp_sha = $(git rev-parse --verify HEAD)\n }\n \n-test_dirty_unmergeable() {\n+test_dirty_unmergeable () {\n \t! git diff --exit-code >/dev/null\n }\n \n-setup_dirty_unmergeable() {\n+setup_dirty_unmergeable () {\n \techo >>file1 change2\n }\n \n-test_dirty_mergeable() {\n+test_dirty_mergeable () {\n \t! git diff --cached --exit-code >/dev/null\n }\n \n-setup_dirty_mergeable() {\n+setup_dirty_mergeable () {\n \techo >file2 file2 &&\n \tgit add file2\n }\n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388973","messageId":"4eab751a3cf00bbffaf4b1084928dad264fa1572.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 04/16] t2018: teach do_checkout() to accept `!` arg","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:13Z","receivedAt":"2019-12-27T13:47:42Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, we were running `test_must_fail do_checkout`. However,\n`test_must_fail` should only be used on git commands. Teach\ndo_checkout() to accept `!` as a potential first argument which will\nprepend `test_must_fail` to the enclosed git command and skips the\nremainder of the function.\n\nThis increases the granularity of the test as, instead of blindly\nchecking that do_checkout() failed, we check that only the specific\nexpected invocation of git fails.\n\nSigned-off-by: Denton Liu \n---\n t/t2018-checkout-branch.sh | 21 +++++++++++++++------\n 1 file changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex e6b852939c..43551332ed 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -13,6 +13,12 @@ test_description='checkout'\n #\n # If is not specified, \"git checkout\" is run with -b.\n do_checkout () {\n+\tshould_fail= &&\n+\tif test \"x$1\" = \"x!\"\n+\tthen\n+\t\tshould_fail=test_must_fail &&\n+\t\tshift\n+\tfi &&\n \texp_branch=$1 &&\n \texp_ref=\"refs/heads/$exp_branch\" &&\n \n@@ -26,10 +32,13 @@ do_checkout () {\n \t\topts=\"$3\"\n \tfi\n \n-\tgit checkout $opts $exp_branch $exp_sha &&\n+\t$should_fail git checkout $opts $exp_branch $exp_sha &&\n \n-\ttest $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&\n-\ttest $exp_sha = $(git rev-parse --verify HEAD)\n+\tif test -z \"$should_fail\"\n+\tthen\n+\t\ttest $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&\n+\t\ttest $exp_sha = $(git rev-parse --verify HEAD)\n+\tfi\n }\n \n test_dirty_unmergeable () {\n@@ -92,7 +101,7 @@ test_expect_success 'checkout -b to a new branch, set to an explicit ref' '\n \n test_expect_success 'checkout -b to a new branch with unmergeable changes fails' '\n \tsetup_dirty_unmergeable &&\n-\ttest_must_fail do_checkout branch2 $HEAD1 &&\n+\tdo_checkout ! branch2 $HEAD1 &&\n \ttest_dirty_unmergeable\n '\n \n@@ -126,7 +135,7 @@ test_expect_success 'checkout -f -b to a new branch with mergeable changes disca\n \n test_expect_success 'checkout -b to an existing branch fails' '\n \ttest_when_finished git reset --hard HEAD &&\n-\ttest_must_fail do_checkout branch2 $HEAD2\n+\tdo_checkout ! branch2 $HEAD2\n '\n \n test_expect_success 'checkout -b to @{-1} fails with the right branch name' '\n@@ -165,7 +174,7 @@ test_expect_success 'checkout -B to an existing branch with unmergeable changes\n \tgit checkout branch1 &&\n \n \tsetup_dirty_unmergeable &&\n-\ttest_must_fail do_checkout branch2 $HEAD1 -B &&\n+\tdo_checkout ! branch2 $HEAD1 -B &&\n \ttest_dirty_unmergeable\n '\n \n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388974","messageId":"c584c9a52b492db2128846e86afb0aadddd6f2de.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 03/16] t2018: use test_must_fail for failing git commands","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:12Z","receivedAt":"2019-12-27T13:47:42Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, when we expected `git diff` to fail, we negated its status with\n`!`. However, if git ever exits unexpectedly, say due to a segfault, we\nwould not be able to tell the difference between that and a controlled\nfailure. Use `test_must_fail git diff` instead so that if an unepxected\nfailure occurs, we can catch it.\n\nWe had some instances of `test_must_fail test_dirty_{un,}mergable`.\nHowever, `test_must_fail` should only be used on git commands. Teach\ntest_dirty_{un,}mergable() to accept `!` as a potential first argument\nwhich will run the git command without test_must_fail(). This prevents\nthe double-negation where we were effectively running\n`test_must_fail test_must_fail git diff ...`.\n\nWhile we're at it, remove redirections to /dev/null since output is\nsilenced when running without `-v` and debugging output is useful with\n`-v`, remove redirections to /dev/null as it is not useful.\n\nSigned-off-by: Denton Liu \n---\n t/t2018-checkout-branch.sh | 22 ++++++++++++++++------\n 1 file changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 79b16e4677..e6b852939c 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -33,7 +33,12 @@ do_checkout () {\n }\n \n test_dirty_unmergeable () {\n-\t! git diff --exit-code >/dev/null\n+\tshould_fail=\"test_expect_code 1\" &&\n+\tif test \"x$1\" = \"x!\"\n+\tthen\n+\t\tshould_fail=\n+\tfi &&\n+\t$should_fail git diff --exit-code\n }\n \n setup_dirty_unmergeable () {\n@@ -41,7 +46,12 @@ setup_dirty_unmergeable () {\n }\n \n test_dirty_mergeable () {\n-\t! git diff --cached --exit-code >/dev/null\n+\tshould_fail=\"test_expect_code 1\" &&\n+\tif test \"x$1\" = \"x!\"\n+\tthen\n+\t\tshould_fail=\n+\tfi &&\n+\t$should_fail git diff --cached --exit-code\n }\n \n setup_dirty_mergeable () {\n@@ -93,7 +103,7 @@ test_expect_success 'checkout -f -b to a new branch with unmergeable changes dis\n \n \t# still dirty and on branch1\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n-\ttest_must_fail test_dirty_unmergeable\n+\ttest_dirty_unmergeable !\n '\n \n test_expect_success 'checkout -b to a new branch preserves mergeable changes' '\n@@ -111,7 +121,7 @@ test_expect_success 'checkout -f -b to a new branch with mergeable changes disca\n \ttest_when_finished git reset --hard HEAD &&\n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n-\ttest_must_fail test_dirty_mergeable\n+\ttest_dirty_mergeable !\n '\n \n test_expect_success 'checkout -b to an existing branch fails' '\n@@ -162,7 +172,7 @@ test_expect_success 'checkout -B to an existing branch with unmergeable changes\n test_expect_success 'checkout -f -B to an existing branch with unmergeable changes discards changes' '\n \t# still dirty and on branch1\n \tdo_checkout branch2 $HEAD1 \"-f -B\" &&\n-\ttest_must_fail test_dirty_unmergeable\n+\ttest_dirty_unmergeable !\n '\n \n test_expect_success 'checkout -B to an existing branch preserves mergeable changes' '\n@@ -179,7 +189,7 @@ test_expect_success 'checkout -f -B to an existing branch with mergeable changes\n \n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 \"-f -B\" &&\n-\ttest_must_fail test_dirty_mergeable\n+\ttest_dirty_mergeable !\n '\n \n test_expect_success 'checkout -b ' '\n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388975","messageId":"0e3072f03c3e34810acdb5f08c3fb5effc39d3a3.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 06/16] t2018: replace \"sha\" with \"oid\"","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:15Z","receivedAt":"2019-12-27T13:47:44Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"As part of the effort to become more hash-agnostic, replace all\ninstances of \"sha\" with \"oid\".\n\nSigned-off-by: Denton Liu \n---\n t/t2018-checkout-branch.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 69758041f4..6b36c7800a 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -4,12 +4,12 @@ test_description='checkout'\n \n . ./test-lib.sh\n \n-# Arguments: []\n+# Arguments: []\n #\n # Runs \"git checkout\" to switch to , testing that\n #\n # 1) we are on the specified branch, ;\n-# 2) HEAD is ; if is not specified, the old HEAD is used.\n+# 2) HEAD is ; if is not specified, the old HEAD is used.\n #\n # If is not specified, \"git checkout\" is run with -b.\n do_checkout () {\n@@ -22,9 +22,9 @@ do_checkout () {\n \texp_branch=$1 &&\n \texp_ref=\"refs/heads/$exp_branch\" &&\n \n-\t# if is not specified, use HEAD.\n+\t# if is not specified, use HEAD.\n \thead=$(git rev-parse --verify HEAD) &&\n-\texp_sha=${2:-$head} &&\n+\texp_oid=${2:-$head} &&\n \n \t# default options for git checkout: -b\n \tif [ -z \"$3\" ]; then\n@@ -33,16 +33,16 @@ do_checkout () {\n \t\topts=\"$3\"\n \tfi\n \n-\t$should_fail git checkout $opts $exp_branch $exp_sha &&\n+\t$should_fail git checkout $opts $exp_branch $exp_oid &&\n \n \tif test -z \"$should_fail\"\n \tthen\n \t\techo \"$exp_ref\" >ref.expect &&\n \t\tgit rev-parse --symbolic-full-name HEAD >ref.actual &&\n \t\ttest_cmp ref.expect ref.actual &&\n-\t\techo \"$exp_sha\" >sha.expect &&\n-\t\tgit rev-parse --verify HEAD >sha.actual &&\n-\t\ttest_cmp sha.expect sha.actual\n+\t\techo \"$exp_oid\" >oid.expect &&\n+\t\tgit rev-parse --verify HEAD >oid.actual &&\n+\t\ttest_cmp oid.expect oid.actual\n \tfi\n }\n \n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388976","messageId":"4fe247c09df89ebe908f366ee7c2a4ec1c209d86.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 05/16] t2018: don't lose return code of git commands","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:14Z","receivedAt":"2019-12-27T13:47:46Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We had some git commands wrapped in non-assignment command substitutions\nwhich would result in their return codes to be lost. Rewrite these\ninstances so that their return codes are now checked.\n\nSigned-off-by: Denton Liu \n---\n t/t2018-checkout-branch.sh | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 43551332ed..69758041f4 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -23,7 +23,8 @@ do_checkout () {\n \texp_ref=\"refs/heads/$exp_branch\" &&\n \n \t# if is not specified, use HEAD.\n-\texp_sha=${2:-$(git rev-parse --verify HEAD)} &&\n+\thead=$(git rev-parse --verify HEAD) &&\n+\texp_sha=${2:-$head} &&\n \n \t# default options for git checkout: -b\n \tif [ -z \"$3\" ]; then\n@@ -36,8 +37,12 @@ do_checkout () {\n \n \tif test -z \"$should_fail\"\n \tthen\n-\t\ttest $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&\n-\t\ttest $exp_sha = $(git rev-parse --verify HEAD)\n+\t\techo \"$exp_ref\" >ref.expect &&\n+\t\tgit rev-parse --symbolic-full-name HEAD >ref.actual &&\n+\t\ttest_cmp ref.expect ref.actual &&\n+\t\techo \"$exp_sha\" >sha.expect &&\n+\t\tgit rev-parse --verify HEAD >sha.actual &&\n+\t\ttest_cmp sha.expect sha.actual\n \tfi\n }\n \n@@ -159,7 +164,8 @@ test_expect_success 'checkout -B to a merge base' '\n '\n \n test_expect_success 'checkout -B to an existing branch from detached HEAD resets branch to HEAD' '\n-\tgit checkout $(git rev-parse --verify HEAD) &&\n+\thead=$(git rev-parse --verify HEAD) &&\n+\tgit checkout \"$head\" &&\n \n \tdo_checkout branch2 \"\" -B\n '\n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388977","messageId":"fd11241003c92f8709f32c66f3ca3b4e8ff8fee5.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 07/16] t3030: use test_path_is_missing()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:16Z","receivedAt":"2019-12-27T13:47:46Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Previously, we would use `test_must_fail test -d` to ensure that the\ndirectory is removed. However, test_must_fail() should only be used for\ngit commands. Use test_path_is_missing() instead to check that the\ndirectory has been removed.\n\nSigned-off-by: Denton Liu \n---\n t/t3030-merge-recursive.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex 2170758e38..d48d211a95 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -604,7 +604,7 @@ test_expect_success 'merge removes empty directories' '\n \tgit commit -mremoved-d/e &&\n \tgit checkout master &&\n \tgit merge -s recursive rm &&\n-\ttest_must_fail test -d d\n+\ttest_path_is_missing d\n '\n \n test_expect_success 'merge-recursive simple w/submodule' '\n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388978","messageId":"a991117b36e8a765e3323065e90fbfe2d63b710c.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 10/16] t3415: increase granularity of test_auto_{fixup,squash}()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:19Z","receivedAt":"2019-12-27T13:47:48Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We used to use `test_must_fail test_auto_{fixup,squash}` which would\nensure that the function failed. However, this is a little ham-fisted as\nthere is no way of ensuring that the expected part of the function\nactually fails.\n\nIncrease the granularity by accepting an optional `!` first argument\nwhich will check that the rebase does not squash in any commits by\nensuring that there are still 4 commits. If `!` is not provided, the old\nlogic is run.\n\nThis patch may be better reviewed with `--ignore-all-space`.\n\nSigned-off-by: Denton Liu \n---\n t/t3415-rebase-autosquash.sh | 64 +++++++++++++++++++++++++-----------\n 1 file changed, 44 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex b0add36f82..093de9005b 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -25,6 +25,13 @@ test_expect_success setup '\n '\n \n test_auto_fixup () {\n+\tno_squash= &&\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\tno_squash=true\n+\t\tshift\n+\tfi &&\n+\n \tgit reset --hard base &&\n \techo 1 >file1 &&\n \tgit add -u &&\n@@ -35,14 +42,19 @@ test_auto_fixup () {\n \ttest_tick &&\n \tgit rebase $2 -i HEAD^^^ &&\n \tgit log --oneline >actual &&\n-\ttest_line_count = 3 actual &&\n-\tgit diff --exit-code $1 &&\n-\techo 1 >expect &&\n-\tgit cat-file blob HEAD^:file1 >actual &&\n-\ttest_cmp expect actual &&\n-\tgit cat-file commit HEAD^ >commit &&\n-\tgrep first commit >actual &&\n-\ttest_line_count = 1 actual\n+\tif test -n \"$no_squash\"\n+\tthen\n+\t\ttest_line_count = 4 actual\n+\telse\n+\t\ttest_line_count = 3 actual &&\n+\t\tgit diff --exit-code $1 &&\n+\t\techo 1 >expect &&\n+\t\tgit cat-file blob HEAD^:file1 >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit cat-file commit HEAD^ >commit &&\n+\t\tgrep first commit >actual &&\n+\t\ttest_line_count = 1 actual\n+\tfi\n }\n \n test_expect_success 'auto fixup (option)' '\n@@ -52,12 +64,19 @@ test_expect_success 'auto fixup (option)' '\n test_expect_success 'auto fixup (config)' '\n \tgit config rebase.autosquash true &&\n \ttest_auto_fixup final-fixup-config-true &&\n-\ttest_must_fail test_auto_fixup fixup-config-true-no --no-autosquash &&\n+\ttest_auto_fixup ! fixup-config-true-no --no-autosquash &&\n \tgit config rebase.autosquash false &&\n-\ttest_must_fail test_auto_fixup final-fixup-config-false\n+\ttest_auto_fixup ! final-fixup-config-false\n '\n \n test_auto_squash () {\n+\tno_squash= &&\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\tno_squash=true\n+\t\tshift\n+\tfi &&\n+\n \tgit reset --hard base &&\n \techo 1 >file1 &&\n \tgit add -u &&\n@@ -68,14 +87,19 @@ test_auto_squash () {\n \ttest_tick &&\n \tgit rebase $2 -i HEAD^^^ &&\n \tgit log --oneline >actual &&\n-\ttest_line_count = 3 actual &&\n-\tgit diff --exit-code $1 &&\n-\techo 1 >expect &&\n-\tgit cat-file blob HEAD^:file1 >actual &&\n-\ttest_cmp expect actual &&\n-\tgit cat-file commit HEAD^ >commit &&\n-\tgrep first commit >actual &&\n-\ttest_line_count = 2 actual\n+\tif test -n \"$no_squash\"\n+\tthen\n+\t\ttest_line_count = 4 actual\n+\telse\n+\t\ttest_line_count = 3 actual &&\n+\t\tgit diff --exit-code $1 &&\n+\t\techo 1 >expect &&\n+\t\tgit cat-file blob HEAD^:file1 >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit cat-file commit HEAD^ >commit &&\n+\t\tgrep first commit >actual &&\n+\t\ttest_line_count = 2 actual\n+\tfi\n }\n \n test_expect_success 'auto squash (option)' '\n@@ -85,9 +109,9 @@ test_expect_success 'auto squash (option)' '\n test_expect_success 'auto squash (config)' '\n \tgit config rebase.autosquash true &&\n \ttest_auto_squash final-squash-config-true &&\n-\ttest_must_fail test_auto_squash squash-config-true-no --no-autosquash &&\n+\ttest_auto_squash ! squash-config-true-no --no-autosquash &&\n \tgit config rebase.autosquash false &&\n-\ttest_must_fail test_auto_squash final-squash-config-false\n+\ttest_auto_squash ! final-squash-config-false\n '\n \n test_expect_success 'misspelled auto squash' '\n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388979","messageId":"baf7ea00484eb7c324074e713178af2121fcd7f2.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 09/16] t3415: stop losing return codes of git commands","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:18Z","receivedAt":"2019-12-27T13:47:50Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When a command is in a non-assignment subshell, the return code will be\nlost in favour of the surrounding command's. Rewrite instances of this\nsuch that the return code of git commands is no longer lost.\n\nSigned-off-by: Denton Liu \n---\n t/t3415-rebase-autosquash.sh | 113 +++++++++++++++++++++++++----------\n 1 file changed, 82 insertions(+), 31 deletions(-)\n\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 22d218698e..b0add36f82 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -37,8 +37,12 @@ test_auto_fixup () {\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual &&\n \tgit diff --exit-code $1 &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest 1 = $(git cat-file commit HEAD^ | grep first | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep first commit >actual &&\n+\ttest_line_count = 1 actual\n }\n \n test_expect_success 'auto fixup (option)' '\n@@ -66,8 +70,12 @@ test_auto_squash () {\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual &&\n \tgit diff --exit-code $1 &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest 2 = $(git cat-file commit HEAD^ | grep first | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep first commit >actual &&\n+\ttest_line_count = 2 actual\n }\n \n test_expect_success 'auto squash (option)' '\n@@ -94,7 +102,8 @@ test_expect_success 'misspelled auto squash' '\n \tgit log --oneline >actual &&\n \ttest_line_count = 4 actual &&\n \tgit diff --exit-code final-missquash &&\n-\ttest 0 = $(git rev-list final-missquash...HEAD | wc -l)\n+\tgit rev-list final-missquash...HEAD >list &&\n+\ttest_must_be_empty list\n '\n \n test_expect_success 'auto squash that matches 2 commits' '\n@@ -113,9 +122,15 @@ test_expect_success 'auto squash that matches 2 commits' '\n \tgit log --oneline >actual &&\n \ttest_line_count = 4 actual &&\n \tgit diff --exit-code final-multisquash &&\n-\ttest 1 = \"$(git cat-file blob HEAD^^:file1)\" &&\n-\ttest 2 = $(git cat-file commit HEAD^^ | grep first | wc -l) &&\n-\ttest 1 = $(git cat-file commit HEAD | grep first | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^^ >commit &&\n+\tgrep first commit >actual &&\n+\ttest_line_count = 2 actual &&\n+\tgit cat-file commit HEAD >commit &&\n+\tgrep first commit >actual &&\n+\ttest_line_count = 1 actual\n '\n \n test_expect_success 'auto squash that matches a commit after the squash' '\n@@ -134,25 +149,38 @@ test_expect_success 'auto squash that matches a commit after the squash' '\n \tgit log --oneline >actual &&\n \ttest_line_count = 5 actual &&\n \tgit diff --exit-code final-presquash &&\n-\ttest 0 = \"$(git cat-file blob HEAD^^:file1)\" &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest 1 = $(git cat-file commit HEAD | grep third | wc -l) &&\n-\ttest 1 = $(git cat-file commit HEAD^ | grep third | wc -l)\n+\techo 0 >expect &&\n+\tgit cat-file blob HEAD^^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD >commit &&\n+\tgrep third commit >actual &&\n+\ttest_line_count = 1 actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep third commit >actual &&\n+\ttest_line_count = 1 actual\n '\n test_expect_success 'auto squash that matches a sha1' '\n \tgit reset --hard base &&\n \techo 1 >file1 &&\n \tgit add -u &&\n \ttest_tick &&\n-\tgit commit -m \"squash! $(git rev-parse --short HEAD^)\" &&\n+\toid=$(git rev-parse --short HEAD^) &&\n+\tgit commit -m \"squash! $oid\" &&\n \tgit tag final-shasquash &&\n \ttest_tick &&\n \tgit rebase --autosquash -i HEAD^^^ &&\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual &&\n \tgit diff --exit-code final-shasquash &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest 1 = $(git cat-file commit HEAD^ | grep squash | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep squash commit >actual &&\n+\ttest_line_count = 1 actual\n '\n \n test_expect_success 'auto squash that matches longer sha1' '\n@@ -160,15 +188,20 @@ test_expect_success 'auto squash that matches longer sha1' '\n \techo 1 >file1 &&\n \tgit add -u &&\n \ttest_tick &&\n-\tgit commit -m \"squash! $(git rev-parse --short=11 HEAD^)\" &&\n+\toid=$(git rev-parse --short=11 HEAD^) &&\n+\tgit commit -m \"squash! $oid\" &&\n \tgit tag final-longshasquash &&\n \ttest_tick &&\n \tgit rebase --autosquash -i HEAD^^^ &&\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual &&\n \tgit diff --exit-code final-longshasquash &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest 1 = $(git cat-file commit HEAD^ | grep squash | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep squash commit >actual &&\n+\ttest_line_count = 1 actual\n '\n \n test_auto_commit_flags () {\n@@ -183,8 +216,12 @@ test_auto_commit_flags () {\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual &&\n \tgit diff --exit-code final-commit-$1 &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest $2 = $(git cat-file commit HEAD^ | grep first | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep first commit >actual &&\n+\ttest_line_count = $2 actual\n }\n \n test_expect_success 'use commit --fixup' '\n@@ -210,11 +247,15 @@ test_auto_fixup_fixup () {\n \t(\n \t\tset_cat_todo_editor &&\n \t\ttest_must_fail git rebase --autosquash -i HEAD^^^^ >actual &&\n+\t\thead=$(git rev-parse --short HEAD) &&\n+\t\tparent1=$(git rev-parse --short HEAD^) &&\n+\t\tparent2=$(git rev-parse --short HEAD^^) &&\n+\t\tparent3=$(git rev-parse --short HEAD^^^) &&\n \t\tcat >expected <<-EOF &&\n-\t\tpick $(git rev-parse --short HEAD^^^) first commit\n-\t\t$1 $(git rev-parse --short HEAD^) $1! first\n-\t\t$1 $(git rev-parse --short HEAD) $1! $2! first\n-\t\tpick $(git rev-parse --short HEAD^^) second commit\n+\t\tpick $parent3 first commit\n+\t\t$1 $parent1 $1! first\n+\t\t$1 $head $1! $2! first\n+\t\tpick $parent2 second commit\n \t\tEOF\n \t\ttest_cmp expected actual\n \t) &&\n@@ -222,13 +263,17 @@ test_auto_fixup_fixup () {\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual\n \tgit diff --exit-code \"final-$1-$2\" &&\n-\ttest 2 = \"$(git cat-file blob HEAD^:file1)\" &&\n+\techo 2 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep first commit >actual &&\n \tif test \"$1\" = \"fixup\"\n \tthen\n-\t\ttest 1 = $(git cat-file commit HEAD^ | grep first | wc -l)\n+\t\ttest_line_count = 1 actual\n \telif test \"$1\" = \"squash\"\n \tthen\n-\t\ttest 3 = $(git cat-file commit HEAD^ | grep first | wc -l)\n+\t\ttest_line_count = 3 actual\n \telse\n \t\tfalse\n \tfi\n@@ -256,19 +301,25 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' '\n \techo 2 >file1 &&\n \tgit add -u &&\n \ttest_tick &&\n-\tgit commit -m \"squash! $(git rev-parse --short HEAD^)\" &&\n+\toid=$(git rev-parse --short HEAD^) &&\n+\tgit commit -m \"squash! $oid\" &&\n \techo 1 >file1 &&\n \tgit add -u &&\n \ttest_tick &&\n-\tgit commit -m \"squash! $(git log -n 1 --format=%s HEAD~2)\" &&\n+\tsubject=$(git log -n 1 --format=%s HEAD~2) &&\n+\tgit commit -m \"squash! $subject\" &&\n \tgit tag final-squash-instFmt &&\n \ttest_tick &&\n \tgit rebase --autosquash -i HEAD~4 &&\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual &&\n \tgit diff --exit-code final-squash-instFmt &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest 2 = $(git cat-file commit HEAD^ | grep squash | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep squash commit >actual &&\n+\ttest_line_count = 2 actual\n '\n \n test_expect_success 'autosquash with empty custom instructionFormat' '\n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388980","messageId":"b3e4f1698fd86f75f650c7928f2107ea0edd3600.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 12/16] t3504: don't use `test_must_fail test_cmp`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:21Z","receivedAt":"2019-12-27T13:47:53Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail function should only be used for git commands since\nwe should assume that external commands work sanely. Since test_cmp() just\nwraps an external command, replace `test_must_fail test_cmp` with\n`! test_cmp`.\n\nSigned-off-by: Denton Liu \n---\n t/t3504-cherry-pick-rerere.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh\nindex a267b2d144..c31141c471 100755\n--- a/t/t3504-cherry-pick-rerere.sh\n+++ b/t/t3504-cherry-pick-rerere.sh\n@@ -95,7 +95,7 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '\n test_expect_success 'cherry-pick conflict without rerere' '\n \ttest_config rerere.enabled false &&\n \ttest_must_fail git cherry-pick master &&\n-\ttest_must_fail test_cmp expect foo\n+\t! test_cmp expect foo\n '\n \n test_done\n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388981","messageId":"0cfc46c5fd0c2a391de9185fe215344720e4413e.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 13/16] t3507: fix indentation","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:22Z","receivedAt":"2019-12-27T13:47:54Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We had some test cases which were indented 7-spaces instead of a tab.\nFix this by reindenting with a tab instead.\n\nThis patch should appear empty with `--ignore-all-space`.\n\nSigned-off-by: Denton Liu \n---\n t/t3507-cherry-pick-conflict.sh | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 9b9b4ca8d4..2a0d119c8a 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -381,23 +381,23 @@ test_expect_success 'failed commit does not clear REVERT_HEAD' '\n '\n \n test_expect_success 'successful final commit clears revert state' '\n- pristine_detach picked-signed &&\n+\tpristine_detach picked-signed &&\n \n- test_must_fail git revert picked-signed base &&\n- echo resolved >foo &&\n- test_path_is_file .git/sequencer/todo &&\n- git commit -a &&\n- test_must_fail test_path_exists .git/sequencer\n+\ttest_must_fail git revert picked-signed base &&\n+\techo resolved >foo &&\n+\ttest_path_is_file .git/sequencer/todo &&\n+\tgit commit -a &&\n+\ttest_must_fail test_path_exists .git/sequencer\n '\n \n test_expect_success 'reset after final pick clears revert state' '\n- pristine_detach picked-signed &&\n+\tpristine_detach picked-signed &&\n \n- test_must_fail git revert picked-signed base &&\n- echo resolved >foo &&\n- test_path_is_file .git/sequencer/todo &&\n- git reset &&\n- test_must_fail test_path_exists .git/sequencer\n+\ttest_must_fail git revert picked-signed base &&\n+\techo resolved >foo &&\n+\ttest_path_is_file .git/sequencer/todo &&\n+\tgit reset &&\n+\ttest_must_fail test_path_exists .git/sequencer\n '\n \n test_expect_success 'revert conflict, diff3 -m style' '\n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388982","messageId":"e55a07b6141308062ee0670f5909ee2f78f931eb.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 15/16] t4124: only mark git command with test_must_fail","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:24Z","receivedAt":"2019-12-27T13:47:55Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail function should only be used for git commands since\nwe should assume that external commands work sanely. Since apply_patch\nwraps a sed and git invocation, rewrite it to accept an `!` argument\nwhich would cause only the git command to be prefixed with\n`test_must_fail`.\n\nSigned-off-by: Denton Liu \n---\n t/t4124-apply-ws-rule.sh | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\nindex ff51e9e789..21a4adc73a 100755\n--- a/t/t4124-apply-ws-rule.sh\n+++ b/t/t4124-apply-ws-rule.sh\n@@ -35,9 +35,15 @@ prepare_test_file () {\n }\n \n apply_patch () {\n+\tshould_fail= &&\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\tshould_fail=test_must_fail &&\n+\t\tshift\n+\tfi &&\n \t>target &&\n \tsed -e \"s|\\([ab]\\)/file|\\1/target|\" \n---\n t/t4124-apply-ws-rule.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\nindex 21a4adc73a..2b19ef9811 100755\n--- a/t/t4124-apply-ws-rule.sh\n+++ b/t/t4124-apply-ws-rule.sh\n@@ -42,7 +42,7 @@ apply_patch () {\n \t\tshift\n \tfi &&\n \t>target &&\n-\tsed -e \"s|\\([ab]\\)/file|\\1/target|\" \n---\n t/t3419-rebase-patch-id.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\nindex 49f548cdb9..94552669ae 100755\n--- a/t/t3419-rebase-patch-id.sh\n+++ b/t/t3419-rebase-patch-id.sh\n@@ -80,7 +80,8 @@ do_tests () {\n \t\tgit commit -q -m \"change big file again\" &&\n \t\tgit checkout -q other^{} &&\n \t\tgit rebase master &&\n-\t\ttest_must_fail test -n \"$(git rev-list master...HEAD~)\"\n+\t\tgit rev-list master...HEAD~ >revs &&\n+\t\ttest_must_be_empty revs\n \t'\n \n \ttest_expect_success $pr 'do not drop patch' '\n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388985","messageId":"e885dd9c39e71089e73615a6dc8a60e1439a0284.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 14/16] t3507: use test_path_is_missing()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:23Z","receivedAt":"2019-12-27T13:48:02Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() function should only be used for git commands since\nwe should assume that external commands work sanely. Replace\n`test_must_fail test_path_exists` with `test_path_is_missing` since we\nexpect these paths to not exist.\n\nSigned-off-by: Denton Liu \n---\n t/t3507-cherry-pick-conflict.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 2a0d119c8a..9bd482ce3b 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -168,7 +168,7 @@ test_expect_success 'successful final commit clears cherry-pick state' '\n \techo resolved >foo &&\n \ttest_path_is_file .git/sequencer/todo &&\n \tgit commit -a &&\n-\ttest_must_fail test_path_exists .git/sequencer\n+\ttest_path_is_missing .git/sequencer\n '\n \n test_expect_success 'reset after final pick clears cherry-pick state' '\n@@ -178,7 +178,7 @@ test_expect_success 'reset after final pick clears cherry-pick state' '\n \techo resolved >foo &&\n \ttest_path_is_file .git/sequencer/todo &&\n \tgit reset &&\n-\ttest_must_fail test_path_exists .git/sequencer\n+\ttest_path_is_missing .git/sequencer\n '\n \n test_expect_success 'failed cherry-pick produces dirty index' '\n@@ -387,7 +387,7 @@ test_expect_success 'successful final commit clears revert state' '\n \techo resolved >foo &&\n \ttest_path_is_file .git/sequencer/todo &&\n \tgit commit -a &&\n-\ttest_must_fail test_path_exists .git/sequencer\n+\ttest_path_is_missing .git/sequencer\n '\n \n test_expect_success 'reset after final pick clears revert state' '\n@@ -397,7 +397,7 @@ test_expect_success 'reset after final pick clears revert state' '\n \techo resolved >foo &&\n \ttest_path_is_file .git/sequencer/todo &&\n \tgit reset &&\n-\ttest_must_fail test_path_exists .git/sequencer\n+\ttest_path_is_missing .git/sequencer\n '\n \n test_expect_success 'revert conflict, diff3 -m style' '\n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"388986","messageId":"68eb05e10edf1f4b09bc4e02587ac5b15fd2c6e3.1577454401.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH 08/16] t3310: extract common no_notes_merge_left()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-27T13:47:17Z","receivedAt":"2019-12-27T13:48:05Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We had many statements which were duplicated. Extract and replace these\nduplicate statements with no_notes_merge_left().\n\nWhile we're at it, replace the test_might_fail(), which should only be\nused on git commands, with a compound command that always returns 0,\neven if the underlying ls fails.\n\nAlso, remove the redirection from stderr to /dev/null. This is because\nthe test scripts automatically suppress output already. Otherwise, if a\ndeveloper asks for verbose output via the `-v` flag, the stderr output\nmay be useful for debugging.\n\nSigned-off-by: Denton Liu \n---\n t/t3310-notes-merge-manual-resolve.sh | 21 +++++++++------------\n 1 file changed, 9 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\nindex 2dea846e25..1950764694 100755\n--- a/t/t3310-notes-merge-manual-resolve.sh\n+++ b/t/t3310-notes-merge-manual-resolve.sh\n@@ -32,6 +32,11 @@ verify_notes () {\n \ttest_cmp \"expect_log_$notes_ref\" \"output_log_$notes_ref\"\n }\n \n+no_notes_merge_left () {\n+\t{ ls .git/NOTES_MERGE_* >output || :; } &&\n+\ttest_must_be_empty output\n+}\n+\n cat <expect_notes_x\n 6e8e3febca3c2bb896704335cc4d0c34cb2f8715 $commit_sha4\n e5388c10860456ee60673025345fe2e153eb8cf8 $commit_sha3\n@@ -335,9 +340,7 @@ EOF\n y and z notes on 4th commit\n EOF\n \tgit notes merge --commit &&\n-\t# No .git/NOTES_MERGE_* files left\n-\ttest_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n-\ttest_must_be_empty output &&\n+\tno_notes_merge_left &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n \ttest \"$(git rev-parse refs/notes/m^1)\" = \"$(cat pre_merge_y)\" &&\n \ttest \"$(git rev-parse refs/notes/m^2)\" = \"$(cat pre_merge_z)\" &&\n@@ -397,9 +400,7 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \n test_expect_success 'abort notes merge' '\n \tgit notes merge --abort &&\n-\t# No .git/NOTES_MERGE_* files left\n-\ttest_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n-\ttest_must_be_empty output &&\n+\tno_notes_merge_left &&\n \t# m has not moved (still == y)\n \ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\" &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n@@ -464,9 +465,7 @@ EOF\n \techo \"new note on 5th commit\" > .git/NOTES_MERGE_WORKTREE/$commit_sha5 &&\n \t# Finalize merge\n \tgit notes merge --commit &&\n-\t# No .git/NOTES_MERGE_* files left\n-\ttest_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n-\ttest_must_be_empty output &&\n+\tno_notes_merge_left &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n \ttest \"$(git rev-parse refs/notes/m^1)\" = \"$(cat pre_merge_y)\" &&\n \ttest \"$(git rev-parse refs/notes/m^2)\" = \"$(cat pre_merge_z)\" &&\n@@ -553,9 +552,7 @@ EOF\n \n test_expect_success 'resolve situation by aborting the notes merge' '\n \tgit notes merge --abort &&\n-\t# No .git/NOTES_MERGE_* files left\n-\ttest_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n-\ttest_must_be_empty output &&\n+\tno_notes_merge_left &&\n \t# m has not moved (still == w)\n \ttest \"$(git rev-parse refs/notes/m)\" = \"$(git rev-parse refs/notes/w)\" &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n-- \n2.24.1.810.g65a2f617f4\n\n"},{"id":"389002","messageId":"41175b96-37d7-f550-b9c2-41d569145b91@kdbg.org","threadId":"52529","inReplyTo":"b3e4f1698fd86f75f650c7928f2107ea0edd3600.1577454401.git.liu.denton@gmail.com","subject":"Re: [PATCH 12/16] t3504: don't use `test_must_fail test_cmp`","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-12-27T20:39:24Z","receivedAt":"2019-12-27T20:40:01Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 27.12.19 um 14:47 schrieb Denton Liu:\n> The test_must_fail function should only be used for git commands since\n> we should assume that external commands work sanely. Since test_cmp() just\n> wraps an external command, replace `test_must_fail test_cmp` with\n> `! test_cmp`.\n> \n> Signed-off-by: Denton Liu \n> ---\n> t/t3504-cherry-pick-rerere.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh\n> index a267b2d144..c31141c471 100755\n> --- a/t/t3504-cherry-pick-rerere.sh\n> +++ b/t/t3504-cherry-pick-rerere.sh\n> @@ -95,7 +95,7 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '\n> test_expect_success 'cherry-pick conflict without rerere' '\n> \ttest_config rerere.enabled false &&\n> \ttest_must_fail git cherry-pick master &&\n> -\ttest_must_fail test_cmp expect foo\n> +\t! test_cmp expect foo\n\nThis is VERY suspicious. It is not reliable to check that something\nis not exactly equal to something else.\n\nFeel free to replace this patch by the following.\n\n----- 8< -----\nt3504: do check for conflict marker after failed cherry-pick\n\nThe test with disabled rerere should make sure that the cherry-picked\nresult does not have the conflict replaced with a recorded resolution.\n\nIt attempts to do so by ensuring that the file content is _not_ equal\nto some other file. That by itself is a very dubious check because just\nabout every random result of an incomplete cherry-pick would satisfy\nthe condition.\n\nIn this case, the intent was to check that the conflicting file does\n_not_ contain the resolved content. But the check actually uses the\nwrong reference file, but not the resolved content. Needless to say\nthat the non-equality is satisfied. And, on top of it, it uses a commit\nthat does not even touch the file that is checked.\n\nDo check for the expected result, which is content from both sides of\nthe merge and merge conflicts. (The latter we check for just the\nmiddle separator for brevity.)\n\nAs a side-effect, this also removes an incorrect use of test_must_fail.\n\nSigned-off-by: Johannes Sixt \n---\n t/t3504-cherry-pick-rerere.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh\nindex a267b2d144..80a0d08706 100755\n--- a/t/t3504-cherry-pick-rerere.sh\n+++ b/t/t3504-cherry-pick-rerere.sh\n@@ -94,8 +94,10 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '\n \n test_expect_success 'cherry-pick conflict without rerere' '\n \ttest_config rerere.enabled false &&\n-\ttest_must_fail git cherry-pick master &&\n-\ttest_must_fail test_cmp expect foo\n+\ttest_must_fail git cherry-pick foo-master &&\n+\tgrep ===== foo &&\n+\tgrep foo-dev foo &&\n+\tgrep foo-master foo\n '\n \n test_done\n-- \n2.24.1.524.gae569673ff\n"},{"id":"389003","messageId":"CAPig+cQRi34Avm7un=qvMJAZhiGa+Zy2s3NG1dcpZz=+rHG0LQ@mail.gmail.com","threadId":"52529","inReplyTo":"9558d2491fa8dc68603426c1528154b4fe352650.1577454401.git.liu.denton@gmail.com","subject":"Re: [PATCH 02/16] t2018: add space between function name and ()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-27T21:03:38Z","receivedAt":"2019-12-27T21:15:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 27, 2019 at 8:47 AM Denton Liu wrote:\n> t2018: add space between function name and ()\n\nPatch 1/16, which removed an entirely unnecessary space from the end\nof the description text, made sense without any additional\nexplanation. However, this patch's change could be puzzling to readers\nwho are not thoroughly familiar with the finer details of test coding\nguidelines. At a minimum, it would be nice to add a sentence to the\ncommit message stating that this change brings the style in line with\nthe coding guidelines.\n\n> Signed-off-by: Denton Liu \n"},{"id":"389007","messageId":"CAPig+cQmO2W7kcqWZwrSsC7-vkk_UVcC5NktY+0dddcyaegr6Q@mail.gmail.com","threadId":"52529","inReplyTo":"4fe247c09df89ebe908f366ee7c2a4ec1c209d86.1577454401.git.liu.denton@gmail.com","subject":"Re: [PATCH 05/16] t2018: don't lose return code of git commands","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-27T21:42:53Z","receivedAt":"2019-12-27T21:43:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 27, 2019 at 8:48 AM Denton Liu wrote:\n> We had some git commands wrapped in non-assignment command substitutions\n> which would result in their return codes to be lost. Rewrite these\n> instances so that their return codes are now checked.\n\nTry writing your commit messages in imperative mood:\n\n Fix invocations of Git commands so their exit codes are not lost\n within command substitutions...\n\nor something. Same comment about several other commit messages in this series.\n\nMore below...\n\n> Signed-off-by: Denton Liu \n> ---\n> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\n> @@ -23,7 +23,8 @@ do_checkout () {\n> # if is not specified, use HEAD.\n> - exp_sha=${2:-$(git rev-parse --verify HEAD)} &&\n> + head=$(git rev-parse --verify HEAD) &&\n> + exp_sha=${2:-$head} &&\n\nAre you sure this transformation is needed? In my tests, the exit code\nof the Git command is not lost.\n"},{"id":"389015","messageId":"xmqqpng9idut.fsf@gitster-ct.c.googlers.com","threadId":"52529","inReplyTo":"41175b96-37d7-f550-b9c2-41d569145b91@kdbg.org","subject":"Re: [PATCH 12/16] t3504: don't use `test_must_fail test_cmp`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-27T22:48:58Z","receivedAt":"2019-12-27T22:49:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> This is VERY suspicious. It is not reliable to check that something\n> is not exactly equal to something else.\n>\n> Feel free to replace this patch by the following.\n>\n> ----- 8< -----\n> t3504: do check for conflict marker after failed cherry-pick\n>\n> The test with disabled rerere should make sure that the cherry-picked\n> result does not have the conflict replaced with a recorded resolution.\n>\n> It attempts to do so by ensuring that the file content is _not_ equal\n> to some other file. That by itself is a very dubious check because just\n> about every random result of an incomplete cherry-pick would satisfy\n> the condition.\n\nGood thinking. Thanks for catching the bogosity of the original\ntest, which dates back to more than 10 years ago X-<.\n\n> In this case, the intent was to check that the conflicting file does\n> _not_ contain the resolved content. But the check actually uses the\n> wrong reference file, but not the resolved content. Needless to say\n> that the non-equality is satisfied. And, on top of it, it uses a commit\n> that does not even touch the file that is checked.\n>\n> Do check for the expected result, which is content from both sides of\n> the merge and merge conflicts. (The latter we check for just the\n> middle separator for brevity.)\n>\n> As a side-effect, this also removes an incorrect use of test_must_fail.\n>\n> Signed-off-by: Johannes Sixt \n> ---\n> t/t3504-cherry-pick-rerere.sh | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh\n> index a267b2d144..80a0d08706 100755\n> --- a/t/t3504-cherry-pick-rerere.sh\n> +++ b/t/t3504-cherry-pick-rerere.sh\n> @@ -94,8 +94,10 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '\n> \n> test_expect_success 'cherry-pick conflict without rerere' '\n> \ttest_config rerere.enabled false &&\n> -\ttest_must_fail git cherry-pick master &&\n> -\ttest_must_fail test_cmp expect foo\n> +\ttest_must_fail git cherry-pick foo-master &&\n> +\tgrep ===== foo &&\n> +\tgrep foo-dev foo &&\n> +\tgrep foo-master foo\n> '\n> \n> test_done\n"},{"id":"389018","messageId":"CAPig+cTaph9Mc1cdL6eNPnTF006YCZ14oLX+xKN9VTVYs7X2_A@mail.gmail.com","threadId":"52529","inReplyTo":"68eb05e10edf1f4b09bc4e02587ac5b15fd2c6e3.1577454401.git.liu.denton@gmail.com","subject":"Re: [PATCH 08/16] t3310: extract common no_notes_merge_left()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-28T07:20:11Z","receivedAt":"2019-12-28T07:20:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 27, 2019 at 8:48 AM Denton Liu wrote:\n> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\n> @@ -32,6 +32,11 @@ verify_notes () {\n> +no_notes_merge_left () {\n> + { ls .git/NOTES_MERGE_* >output || :; } &&\n> + test_must_be_empty output\n> +}\n\nThis function name leaves me thinking that it's talking about\ndirectionality (left vs. right) and gives insufficient clue that it's\ntalking about a .git/NOTES_MERGE_* file. A name such as\nassert_no_notes_merge_files() or notes_merge_files_gone() would make\nthe intention more obvious.\n\n> - # No .git/NOTES_MERGE_* files left\n> - test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n> - test_must_be_empty output &&\n\nOn the other hand, the original in-code comment was not confusing,\nprobably because it was obvious it was talking about an actual file,\ndue to spelling out .git/NOTES_MERGE_* explicitly and due to actually\nusing the literal word \"file\", plus the code following the comment\nmade it very obvious what was happening.\n\nThese observations may not be actionable since someone actually\nworking on this script will know that it's dealing with\n.git/NOTES_MERGES_*, but as a reviewer not familiar with this\nparticular script, reading the patch from top to bottom, I found the\nfunction name confusing.\n"},{"id":"389019","messageId":"CAPig+cQo1nbRo=n8-XOtycGijj3k1y_Zozu7VW-WTSBB9LncqQ@mail.gmail.com","threadId":"52529","inReplyTo":"c584c9a52b492db2128846e86afb0aadddd6f2de.1577454401.git.liu.denton@gmail.com","subject":"Re: [PATCH 03/16] t2018: use test_must_fail for failing git commands","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-28T07:55:20Z","receivedAt":"2019-12-28T07:55:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 27, 2019 at 8:47 AM Denton Liu wrote:\n> We had some instances of `test_must_fail test_dirty_{un,}mergable`.\n> However, `test_must_fail` should only be used on git commands. Teach\n> test_dirty_{un,}mergable() to accept `!` as a potential first argument\n\ns/mergable/mergeable/ twice above.\n\n> which will run the git command without test_must_fail(). This prevents\n> the double-negation where we were effectively running\n> `test_must_fail test_must_fail git diff ...`.\n\nThis change makes the situation even more confusing than it already\nis. For instance, you can now say:\n\n test_dirty_unmergable !\n\nwhich effectively says \"not unmergeable\", which is the same as \"not\nnot mergeable\". Does that mean it is mergeable? Does that mean it\nshould be calling test_dirty_mergeable()? Same situation arises with:\n\n test_dirty_mergeable !\n\nIt seems like there are four distinct cases that are being tested\nhere. If that's so, then rather than changing these functions to\naccept \"!\" as an argument, perhaps there should be four distinct\none-liner functions for testing those cases?\n"},{"id":"389020","messageId":"CAPig+cT242CjL9S0OcS1Pbsgf-u1N=SQzNfLdao03LU2OGEciQ@mail.gmail.com","threadId":"52529","inReplyTo":"4eab751a3cf00bbffaf4b1084928dad264fa1572.1577454401.git.liu.denton@gmail.com","subject":"Re: [PATCH 04/16] t2018: teach do_checkout() to accept `!` arg","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-28T08:34:03Z","receivedAt":"2019-12-28T08:34:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 27, 2019 at 8:47 AM Denton Liu wrote:\n> Before, we were running `test_must_fail do_checkout`. However,\n> `test_must_fail` should only be used on git commands. Teach\n> do_checkout() to accept `!` as a potential first argument which will\n> prepend `test_must_fail` to the enclosed git command and skips the\n> remainder of the function.\n\nThere's a grammatical problem here. s/skips/skip/ is one way to fix it.\n\nUse imperative mood when writing commit messages. Drop words such as\n\"before\" and \"were\". For instance:\n\n Stop using test_must_fail() with non-Git commands because...\n\n(Same comment applies to pretty much all commit messages in this series.)\n\n> This increases the granularity of the test as, instead of blindly\n> checking that do_checkout() failed, we check that only the specific\n> expected invocation of git fails.\n\nThis may be a case of trying to describe in prose too much of what is\nbetter described by the code itself. As a reviewer, I spent more time\ntrying to figure out what this was saying that I did merely looking at\nthe code and comprehending why the two checks following the\ngit-checkout invocation should be skipped. Consequently, I lean toward\ndropping \"...and skips the remainder...\" through the end of the commit\nmessage.\n\nMore below...\n\n> Signed-off-by: Denton Liu \n> ---\n> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\n> @@ -13,6 +13,12 @@ test_description='checkout'\n> #\n> # If is not specified, \"git checkout\" is run with -b.\n> do_checkout () {\n> + should_fail= &&\n> + if test \"x$1\" = \"x!\"\n> + then\n> + should_fail=test_must_fail &&\n> + shift\n> + fi &&\n\nYou forgot to update the function comment to talk about the new\noptional \"!\" argument.\n\n> @@ -26,10 +32,13 @@ do_checkout () {\n> - git checkout $opts $exp_branch $exp_sha &&\n> + $should_fail git checkout $opts $exp_branch $exp_sha &&\n\nIf I read this literally, it says that the git checkout should always\nfail. A more wisely chosen variable name would help to alleviate this\nproblem.\n\nWhen you start parameterizing the actual invocation of a command like\nthis (I'm not talking about the command arguments which are also\nparameterized), the abstraction level and cognitive load increase...\n\n> - test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&\n> - test $exp_sha = $(git rev-parse --verify HEAD)\n> + if test -z \"$should_fail\"\n> + then\n> + test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&\n> + test $exp_sha = $(git rev-parse --verify HEAD)\n> + fi\n> }\n\nYou could reduce the cognitive load by making the code easier to\nunderstand at-a-glance (though at the cost of a minor bit of\nduplication) by structuring it instead like this:\n\n if test -n \"$should_fail\"\n then\n test_must_fail git checkout $opts $exp_branch $exp_sha\n else\n git checkout $opts $exp_branch $exp_sha &&\n test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&\n test $exp_sha = $(git rev-parse --verify HEAD)\n fi\n\nwhere 'should_fail' is either empty or non-empty depending upon\nwhether \"!\" was supplied as an argument. (And, when coded this way,\n\"should_fail\" is a reasonable variable name.)\n"},{"id":"389099","messageId":"86lfqt36ah.fsf@gmail.com","threadId":"52529","inReplyTo":"c584c9a52b492db2128846e86afb0aadddd6f2de.1577454401.git.liu.denton@gmail.com","subject":"Re: [PATCH 03/16] t2018: use test_must_fail for failing git commands","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2019-12-30T20:30:30Z","receivedAt":"2019-12-30T20:30:36Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Denton Liu writes:\n\n> While we're at it, remove redirections to /dev/null since output is\n> silenced when running without `-v` and debugging output is useful with\n> `-v`, remove redirections to /dev/null as it is not useful.\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\nVery minor nit: The underlined part (after last comma) duplicates what\nalready has been said.\n\n-- \nJakub Narębski\n"},{"id":"389102","messageId":"86eewl35xv.fsf@gmail.com","threadId":"52529","inReplyTo":"CAPig+cTaph9Mc1cdL6eNPnTF006YCZ14oLX+xKN9VTVYs7X2_A@mail.gmail.com","subject":"Re: [PATCH 08/16] t3310: extract common no_notes_merge_left()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2019-12-30T20:38:04Z","receivedAt":"2019-12-30T20:38:09Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Eric Sunshine writes:\n> On Fri, Dec 27, 2019 at 8:48 AM Denton Liu wrote:\n>> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\n>> @@ -32,6 +32,11 @@ verify_notes () {\n>> +no_notes_merge_left () {\n>> + { ls .git/NOTES_MERGE_* >output || :; } &&\n>> + test_must_be_empty output\n>> +}\n>\n> This function name leaves me thinking that it's talking about\n> directionality (left vs. right) and gives insufficient clue that it's\n> talking about a .git/NOTES_MERGE_* file. A name such as\n> assert_no_notes_merge_files() or notes_merge_files_gone() would make\n> the intention more obvious.\n>\n>> - # No .git/NOTES_MERGE_* files left\n>> - test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n>> - test_must_be_empty output &&\n>\n> On the other hand, the original in-code comment was not confusing,\n> probably because it was obvious it was talking about an actual file,\n> due to spelling out .git/NOTES_MERGE_* explicitly and due to actually\n> using the literal word \"file\", plus the code following the comment\n> made it very obvious what was happening.\n>\n> These observations may not be actionable since someone actually\n> working on this script will know that it's dealing with\n> .git/NOTES_MERGES_*, but as a reviewer not familiar with this\n> particular script, reading the patch from top to bottom, I found the\n> function name confusing.\n\nThe problem with clarity of meaning is enhanced by the fact that during\nrefactoring the \"# No .git/NOTES_MERGE_* files left\" comment got lost.\nIt could have been added in the new function, as first line; or\nrephrased as this new function description.\n\nBest,\n-- \nJakub Narębski\n"},{"id":"389111","messageId":"868smt2zqh.fsf@gmail.com","threadId":"52529","inReplyTo":"54315fecfe373d8020f2172b9b43e02c0dae137d.1577454401.git.liu.denton@gmail.com","subject":"Re: [PATCH 16/16] t4124: let sed open its own files","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2019-12-30T22:52:06Z","receivedAt":"2019-12-30T22:52:14Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Denton Liu writes:\n\n> In one case, we were using a redirection operator to feed input into\n> sed. However, since sed is capable of opening its own files, make sed\n> open its own files instead of redirecting input into it.\n\nCould you please write in the commit message what advantages does this\nchange bring?\n\n> Signed-off-by: Denton Liu \n> ---\n> t/t4124-apply-ws-rule.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\n> index 21a4adc73a..2b19ef9811 100755\n> --- a/t/t4124-apply-ws-rule.sh\n> +++ b/t/t4124-apply-ws-rule.sh\n> @@ -42,7 +42,7 @@ apply_patch () {\n> \t\tshift\n> \tfi &&\n> \t>target &&\n> -\tsed -e \"s|\\([ab]\\)/file|\\1/target|\" +\tsed -e \"s|\\([ab]\\)/file|\\1/target|\" patch |\n> \t$should_fail git apply \"$@\"\n> }\n\n-- \nJakub Narębski\n"},{"id":"389116","messageId":"xmqqfth1fl72.fsf@gitster-ct.c.googlers.com","threadId":"52529","inReplyTo":"868smt2zqh.fsf@gmail.com","subject":"Re: [PATCH 16/16] t4124: let sed open its own files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-30T23:27:45Z","receivedAt":"2019-12-30T23:27:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Denton Liu writes:\n>\n>> In one case, we were using a redirection operator to feed input into\n>> sed. However, since sed is capable of opening its own files, make sed\n>> open its own files instead of redirecting input into it.\n>\n> Could you please write in the commit message what advantages does this\n> change bring?\n\nA fair question.\n\nMy version of short answer is \"nothing---it is not wrong to write it\neither way, and it is not worth the patch churn to rewrite it from\none form to the other, once the script is written\".\n\nIf we were to extend these tests in such a way that the command\nneeds to read from more than one input file, though, dropping the\nredirection like the patch does is a good first step toward that,\ni.e. extending\n\n\tsed -e \"expression\" patch\n\nto\n\n\tsed -e \"expression\" patch-1 patch-2 ...\n\nwould look more natural than starting from\n\n\tsed -e \"expression\" Jakub Narebski writes:\n> \n> > Denton Liu writes:\n> >\n> >> In one case, we were using a redirection operator to feed input into\n> >> sed. However, since sed is capable of opening its own files, make sed\n> >> open its own files instead of redirecting input into it.\n> >\n> > Could you please write in the commit message what advantages does this\n> > change bring?\n> \n> A fair question.\n> \n> My version of short answer is \"nothing---it is not wrong to write it\n> either way, and it is not worth the patch churn to rewrite it from\n> one form to the other, once the script is written\".\n\nIn one of my earliest contributions to Git, Gábor suggested that since\nsince 'head' and 'sed' can open its own input file, there's no need to\nredirect its standard input[1].\n\nI assumed that letting these programs open their own input was one of\nthose minor \"style cleanup\" things that was done such as removing\nspaces after redirection operators, indenting compound command blocks,\netc. Whenever I've been cleaning tests up, if I noticed a `sed <` in the\narea, I'd apply the transformation as a style cleanup.\n\nI guess the only tangible benefit I can think of is that programs which\nhave a file open can optimise on the fact that they can perform random\naccess on the file whereas with stdin, only linear access is allowed. I\ndon't know enough about the internals of various seds to be able to\ncomment on whether any of them actually take advantage of this, however.\n\nAnyway, if no one else feels strongly about this, I can drop this patch\nand I'll stop doing this cleanup in the future.\n\nThanks,\n\nDenton\n\n[1]: https://lore.kernel.org/git/20190317130539.GA23160@szeder.dev/\n"},{"id":"389146","messageId":"CAPig+cQWgH2MPZWZm1iL20at7BWN7-0p2=ogf8pHjGKvHDLL6g@mail.gmail.com","threadId":"52529","inReplyTo":"20200101082443.GB5152@generichostname","subject":"Re: [PATCH 16/16] t4124: let sed open its own files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-01T08:33:59Z","receivedAt":"2020-01-01T08:45:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 1, 2020 at 3:25 AM Denton Liu wrote:\n> I assumed that letting these programs open their own input was one of\n> those minor \"style cleanup\" things that was done such as removing\n> spaces after redirection operators, indenting compound command blocks,\n> etc. Whenever I've been cleaning tests up, if I noticed a `sed <` in the\n> area, I'd apply the transformation as a style cleanup.\n>\n> Anyway, if no one else feels strongly about this, I can drop this patch\n> and I'll stop doing this cleanup in the future.\n\nAs a reviewer, I'd just as soon not see that particular change which\nincreases reviewer burden yet provides no practical benefit. This is\nespecially true for these lengthy patch series which can easily lead\nto reviewer fatigue. (In fact, after reviewing this series -- and its\npredecessors -- I was strongly considering asking you to limit these\ncleanup series to 10 or fewer patches rather than 16, precisely due to\nfeeling reviewer fatigue.)\n"},{"id":"389147","messageId":"20200101084840.GC5152@generichostname","threadId":"52529","inReplyTo":"CAPig+cQmO2W7kcqWZwrSsC7-vkk_UVcC5NktY+0dddcyaegr6Q@mail.gmail.com","subject":"Re: [PATCH 05/16] t2018: don't lose return code of git commands","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-01T08:48:40Z","receivedAt":"2020-01-01T08:48:46Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Eric,\n\nOn Fri, Dec 27, 2019 at 04:42:53PM -0500, Eric Sunshine wrote:\n> On Fri, Dec 27, 2019 at 8:48 AM Denton Liu wrote:\n> > We had some git commands wrapped in non-assignment command substitutions\n> > which would result in their return codes to be lost. Rewrite these\n> > instances so that their return codes are now checked.\n> \n> Try writing your commit messages in imperative mood:\n> \n> Fix invocations of Git commands so their exit codes are not lost\n> within command substitutions...\n> \n> or something. Same comment about several other commit messages in this series.\n\nI thought that the preferred form of commit messages is to introduce the\nproblem that I'm trying to solve first (\"We had some git commands losing\nreturn codes\") then, after that, describe the changes I made in\nimperative mood (\"Rewrite these instances...\"). From what I can tell,\nall of my commit messages conform to this template.\n\nI'd prefer to keep the \"problem statement\" introduction in my commit\nmessages as it primes readers so they know \"why\" before \"what\" but I\ncan't think of any way to phrase the \"problem statement\" part in a way\nthat sounds good without resorting to past tense. Any suggestions?\n\n> \n> More below...\n> \n> > Signed-off-by: Denton Liu \n> > ---\n> > diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\n> > @@ -23,7 +23,8 @@ do_checkout () {\n> > # if is not specified, use HEAD.\n> > - exp_sha=${2:-$(git rev-parse --verify HEAD)} &&\n> > + head=$(git rev-parse --verify HEAD) &&\n> > + exp_sha=${2:-$head} &&\n> \n> Are you sure this transformation is needed? In my tests, the exit code\n> of the Git command is not lost.\n\nThanks for double checking, it's not needed. I'll drop this in my next\nreroll.\n"},{"id":"389148","messageId":"20200101085345.GD5152@generichostname","threadId":"52529","inReplyTo":"CAPig+cQWgH2MPZWZm1iL20at7BWN7-0p2=ogf8pHjGKvHDLL6g@mail.gmail.com","subject":"Re: [PATCH 16/16] t4124: let sed open its own files","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-01T08:53:45Z","receivedAt":"2020-01-01T08:53:50Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Eric,\n\nOn Wed, Jan 01, 2020 at 03:33:59AM -0500, Eric Sunshine wrote:\n> (In fact, after reviewing this series -- and its\n> predecessors -- I was strongly considering asking you to limit these\n> cleanup series to 10 or fewer patches rather than 16, precisely due to\n> feeling reviewer fatigue.)\n\nWill do. You've been unbelievably helpful as the primary reviewer for\nthese cleanup series and I'd like to make sure that I don't cause you to\nburn out.\n\nThanks for your patience,\n\nDenton\n"},{"id":"389149","messageId":"CAPig+cSeuZRySuaOAvNTiy1eyH0AQ4is-g8LLA7-wC+CztuH0A@mail.gmail.com","threadId":"52529","inReplyTo":"20200101084840.GC5152@generichostname","subject":"Re: [PATCH 05/16] t2018: don't lose return code of git commands","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-01T09:21:31Z","receivedAt":"2020-01-01T09:21:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 1, 2020 at 3:48 AM Denton Liu wrote:\n> On Fri, Dec 27, 2019 at 04:42:53PM -0500, Eric Sunshine wrote:\n> > On Fri, Dec 27, 2019 at 8:48 AM Denton Liu wrote:\n> > > We had some git commands wrapped in non-assignment command substitutions\n> > > which would result in their return codes to be lost. Rewrite these\n> > > instances so that their return codes are now checked.\n> >\n> > Try writing your commit messages in imperative mood:\n> >\n> > Fix invocations of Git commands so their exit codes are not lost\n> > within command substitutions...\n>\n> I thought that the preferred form of commit messages is to introduce the\n> problem that I'm trying to solve first (\"We had some git commands losing\n> return codes\") then, after that, describe the changes I made in\n> imperative mood (\"Rewrite these instances...\").\n\nMy suggested rewrite includes both the problem (\"exit codes ... lost\nwithin command substitutions\") and the change in one sentence. The\nshorter you can make the commit message without omitting important\ninformation and without being outright cryptic, the better. Let the\npatch itself do the talking as much as possible.\n\nHere's an example, from another patch in this series, which provides\ntoo much information:\n\n While we're at it, replace the test_might_fail(), which should\n only be used on git commands, with a compound command that always\n returns 0, even if the underlying ls fails.\n\nThe bit about \"...with a compound command...\" is unnecessary and\nwastes reviewer time and is far more difficult to understand than\nsimply looking at the code in the patch itself. This would have been\nsufficient:\n\n While here, stop using test_might_fail() with 'ls' since it should\n only be used with git commands.\n\n> From what I can tell,\n> all of my commit messages conform to this template.\n> I'd prefer to keep the \"problem statement\" introduction in my commit\n> messages as it primes readers so they know \"why\" before \"what\" but I\n> can't think of any way to phrase the \"problem statement\" part in a way\n> that sounds good without resorting to past tense. Any suggestions?\n\nUsing past tense makes it ambiguous to reviewers whether you're\ntalking about the state of the code which existed some time back or if\nyou're talking about the state of the code immediately before this\npatch is applied. Try rewriting your commit messages without words\nlike \"previously\", \"before\", \"had\", \"now\", etc. For instance, the\ncommit message for this patch could be rewritten (keeping your wording\nas much as possible) like this:\n\n t2018: don't lose return code of git commands\n\n The exit code of git commands wrapped in non-assignment command\n substitutions is lost. Rewrite these invocations so the exit codes\n are checked.\n"},{"id":"389168","messageId":"xmqqy2upemuv.fsf@gitster-ct.c.googlers.com","threadId":"52529","inReplyTo":"20200101084840.GC5152@generichostname","subject":"Re: [PATCH 05/16] t2018: don't lose return code of git commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-02T18:26:16Z","receivedAt":"2020-01-02T18:26:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n>> Try writing your commit messages in imperative mood:\n>> \n>> Fix invocations of Git commands so their exit codes are not lost\n>> within command substitutions...\n>> \n>> or something. Same comment about several other commit messages in this series.\n>\n> I thought that the preferred form of commit messages is to introduce the\n> problem that I'm trying to solve first (\"We had some git commands losing\n> return codes\") then, after that, describe the changes I made in\n> imperative mood (\"Rewrite these instances...\"). From what I can tell,\n> all of my commit messages conform to this template.\n>\n> I'd prefer to keep the \"problem statement\" introduction in my commit\n> messages as it primes readers so they know \"why\" before \"what\" but I\n> can't think of any way to phrase the \"problem statement\" part in a way\n> that sounds good without resorting to past tense. Any suggestions?\n\nI find Eric's slightly easier to follow in this particular case,\nprimarily because the problem being solved is so obvious (i.e. \"so\nthat their exit codes are not lost\" is sufficient to convey that the\ncurrent code lose exit codes and that it is bad to lose exit codes).\n\nWhen the problem is deeper or needs longer backstory to understand\nwhere it came from, I do prefer a separate description of the\ncurrent status and reason why the current status is undesirable\n(both in the present tense), followed by commands to the code to\nbecome better.\n\nThanks.\n\n\n\n>> \n>> More below...\n>> \n>> > Signed-off-by: Denton Liu \n>> > ---\n>> > diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\n>> > @@ -23,7 +23,8 @@ do_checkout () {\n>> > # if is not specified, use HEAD.\n>> > - exp_sha=${2:-$(git rev-parse --verify HEAD)} &&\n>> > + head=$(git rev-parse --verify HEAD) &&\n>> > + exp_sha=${2:-$head} &&\n>> \n>> Are you sure this transformation is needed? In my tests, the exit code\n>> of the Git command is not lost.\n>\n> Thanks for double checking, it's not needed. I'll drop this in my next\n> reroll.\n"},{"id":"389322","messageId":"cover.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1577454401.git.liu.denton@gmail.com","subject":"[PATCH v2 00/16] t: replace incorrect test_must_fail usage (part 2)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:52:57Z","receivedAt":"2020-01-07T04:53:30Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The overall scope of these patches is to replace inappropriate uses of\ntest_must_fail. IOW, we should only allow test_must_fail to run on `git`\nand `test-tool`. Ultimately, we will conclude by making test_must_fail\nerror out on non-git commands. An advance view of the final series can\nbe found here[1].\n\nThis is the second part. It focuses on t[234]*.sh. The first part can be\nfound here[2].\n\nChanges since v1:\n\n* Rewrite commit messages to be shorter and use the present tense when\n referring to the present state\n\n* Clean up as suggested by Eric\n\n* Replace 12/16 with J6t's patch\n\n* Drop \"let sed open its own files\"\n\n[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2\n[2]: https://lore.kernel.org/git/cover.1576583819.git.liu.denton@gmail.com/\n\nDenton Liu (15):\n t2018: remove trailing space from test description\n t2018: add space between function name and ()\n t2018: improve style of if-statement\n t2018: use test_expect_code for failing git commands\n t2018: teach do_checkout() to accept `!` arg\n t2018: don't lose return code of git commands\n t2018: replace \"sha\" with \"oid\"\n t3030: use test_path_is_missing()\n t3310: extract common notes_merge_files_gone()\n t3415: stop losing return codes of git commands\n t3415: increase granularity of test_auto_{fixup,squash}()\n t3419: stop losing return code of git command\n t3507: fix indentation\n t3507: use test_path_is_missing()\n t4124: only mark git command with test_must_fail\n\nJohannes Sixt (1):\n t3504: do check for conflict marker after failed cherry-pick\n\n t/t2018-checkout-branch.sh | 77 ++++++++-----\n t/t3030-merge-recursive.sh | 2 +-\n t/t3310-notes-merge-manual-resolve.sh | 22 ++--\n t/t3415-rebase-autosquash.sh | 153 +++++++++++++++++++-------\n t/t3419-rebase-patch-id.sh | 3 +-\n t/t3504-cherry-pick-rerere.sh | 6 +-\n t/t3507-cherry-pick-conflict.sh | 28 ++---\n t/t4124-apply-ws-rule.sh | 10 +-\n 8 files changed, 205 insertions(+), 96 deletions(-)\n\nRange-diff against v1:\n 1: 7517159cc1 = 1: a0199f1e48 t2018: remove trailing space from test description\n 2: 0674eee69e ! 2: f0f541b520 t2018: add space between function name and ()\n @@ Metadata\n ## Commit message ##\n t2018: add space between function name and ()\n \n + Add a space between the function name and () which brings the style in\n + line with Git's coding guidelines.\n +\n ## t/t2018-checkout-branch.sh ##\n @@ t/t2018-checkout-branch.sh: test_description='checkout'\n # 2) HEAD is ; if is not specified, the old HEAD is used.\n -: ---------- > 3: 501eb147c3 t2018: improve style of if-statement\n 3: 2451bff3af ! 4: 587e53053f t2018: use test_must_fail for failing git commands\n @@ Metadata\n Author: Denton Liu \n \n ## Commit message ##\n - t2018: use test_must_fail for failing git commands\n + t2018: use test_expect_code for failing git commands\n \n - Before, when we expected `git diff` to fail, we negated its status with\n + When we are expecting `git diff` to fail, we negate its status with\n `!`. However, if git ever exits unexpectedly, say due to a segfault, we\n would not be able to tell the difference between that and a controlled\n - failure. Use `test_must_fail git diff` instead so that if an unepxected\n - failure occurs, we can catch it.\n + failure. Use `test_expect_code 1 git diff` instead so that if an\n + unexpected failure occurs, we can catch it.\n \n - We had some instances of `test_must_fail test_dirty_{un,}mergable`.\n - However, `test_must_fail` should only be used on git commands. Teach\n - test_dirty_{un,}mergable() to accept `!` as a potential first argument\n - which will run the git command without test_must_fail(). This prevents\n - the double-negation where we were effectively running\n - `test_must_fail test_must_fail git diff ...`.\n + We have some instances of `test_must_fail test_dirty_{un,}mergable`.\n + However, `test_must_fail` should only be used on git commands. Convert\n + these instances to use the `test_dirty_{un,}mergeable_discards_changes`\n + helper functions so that we remove the double-negation.\n \n While we're at it, remove redirections to /dev/null since output is\n - silenced when running without `-v` and debugging output is useful with\n - `-v`, remove redirections to /dev/null as it is not useful.\n + silenced when running without `-v` anyway.\n \n ## t/t2018-checkout-branch.sh ##\n @@ t/t2018-checkout-branch.sh: do_checkout () {\n @@ t/t2018-checkout-branch.sh: do_checkout () {\n \n test_dirty_unmergeable () {\n -\t! git diff --exit-code >/dev/null\n -+\tshould_fail=\"test_expect_code 1\" &&\n -+\tif test \"x$1\" = \"x!\"\n -+\tthen\n -+\t\tshould_fail=\n -+\tfi &&\n -+\t$should_fail git diff --exit-code\n ++\ttest_expect_code 1 git diff --exit-code\n ++}\n ++\n ++test_dirty_unmergeable_discards_changes () {\n ++\tgit diff --exit-code\n }\n \n setup_dirty_unmergeable () {\n @@ t/t2018-checkout-branch.sh: setup_dirty_unmergeable () {\n \n test_dirty_mergeable () {\n -\t! git diff --cached --exit-code >/dev/null\n -+\tshould_fail=\"test_expect_code 1\" &&\n -+\tif test \"x$1\" = \"x!\"\n -+\tthen\n -+\t\tshould_fail=\n -+\tfi &&\n -+\t$should_fail git diff --cached --exit-code\n ++\ttest_expect_code 1 git diff --cached --exit-code\n ++}\n ++\n ++test_dirty_mergeable_discards_changes () {\n ++\tgit diff --cached --exit-code\n }\n \n setup_dirty_mergeable () {\n @@ t/t2018-checkout-branch.sh: test_expect_success 'checkout -f -b to a new branch\n \t# still dirty and on branch1\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n -\ttest_must_fail test_dirty_unmergeable\n -+\ttest_dirty_unmergeable !\n ++\ttest_dirty_unmergeable_discards_changes\n '\n \n test_expect_success 'checkout -b to a new branch preserves mergeable changes' '\n @@ t/t2018-checkout-branch.sh: test_expect_success 'checkout -f -b to a new branch\n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n -\ttest_must_fail test_dirty_mergeable\n -+\ttest_dirty_mergeable !\n ++\ttest_dirty_mergeable_discards_changes\n '\n \n test_expect_success 'checkout -b to an existing branch fails' '\n @@ t/t2018-checkout-branch.sh: test_expect_success 'checkout -B to an existing bran\n \t# still dirty and on branch1\n \tdo_checkout branch2 $HEAD1 \"-f -B\" &&\n -\ttest_must_fail test_dirty_unmergeable\n -+\ttest_dirty_unmergeable !\n ++\ttest_dirty_unmergeable_discards_changes\n '\n \n test_expect_success 'checkout -B to an existing branch preserves mergeable changes' '\n @@ t/t2018-checkout-branch.sh: test_expect_success 'checkout -f -B to an existing b\n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 \"-f -B\" &&\n -\ttest_must_fail test_dirty_mergeable\n -+\ttest_dirty_mergeable !\n ++\ttest_dirty_mergeable_discards_changes\n '\n \n test_expect_success 'checkout -b ' '\n 4: bc6330557e ! 5: c43c11b912 t2018: teach do_checkout() to accept `!` arg\n @@ Metadata\n ## Commit message ##\n t2018: teach do_checkout() to accept `!` arg\n \n - Before, we were running `test_must_fail do_checkout`. However,\n + We are running `test_must_fail do_checkout`. However,\n `test_must_fail` should only be used on git commands. Teach\n do_checkout() to accept `!` as a potential first argument which will\n - prepend `test_must_fail` to the enclosed git command and skips the\n - remainder of the function.\n + cause the function to expect the \"git checkout\" to fail.\n \n This increases the granularity of the test as, instead of blindly\n checking that do_checkout() failed, we check that only the specific\n @@ Commit message\n \n ## t/t2018-checkout-branch.sh ##\n @@ t/t2018-checkout-branch.sh: test_description='checkout'\n + \n + . ./test-lib.sh\n + \n +-# Arguments: []\n ++# Arguments: [!] []\n + #\n + # Runs \"git checkout\" to switch to , testing that\n + #\n +@@ t/t2018-checkout-branch.sh: test_description='checkout'\n + # 2) HEAD is ; if is not specified, the old HEAD is used.\n #\n # If is not specified, \"git checkout\" is run with -b.\n ++#\n ++# If the first argument is `!`, \"git checkout\" is expected to fail when\n ++# it is run.\n do_checkout () {\n +\tshould_fail= &&\n +\tif test \"x$1\" = \"x!\"\n +\tthen\n -+\t\tshould_fail=test_must_fail &&\n ++\t\tshould_fail=yes &&\n +\t\tshift\n +\tfi &&\n \texp_branch=$1 &&\n @@ t/t2018-checkout-branch.sh: do_checkout () {\n \tfi\n \n -\tgit checkout $opts $exp_branch $exp_sha &&\n -+\t$should_fail git checkout $opts $exp_branch $exp_sha &&\n - \n +-\n -\ttest $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&\n -\ttest $exp_sha = $(git rev-parse --verify HEAD)\n -+\tif test -z \"$should_fail\"\n ++\tif test -n \"$should_fail\"\n +\tthen\n ++\t\ttest_must_fail git checkout $opts $exp_branch $exp_sha\n ++\telse\n ++\t\tgit checkout $opts $exp_branch $exp_sha &&\n +\t\ttest $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&\n +\t\ttest $exp_sha = $(git rev-parse --verify HEAD)\n +\tfi\n 5: fb2b865e6a ! 6: e09a70f6ca t2018: don't lose return code of git commands\n @@ Metadata\n ## Commit message ##\n t2018: don't lose return code of git commands\n \n - We had some git commands wrapped in non-assignment command substitutions\n - which would result in their return codes to be lost. Rewrite these\n - instances so that their return codes are now checked.\n + Fix invocations of git commands so their exit codes are not lost\n + within non-assignment command substitutions.\n \n ## t/t2018-checkout-branch.sh ##\n @@ t/t2018-checkout-branch.sh: do_checkout () {\n - \texp_ref=\"refs/heads/$exp_branch\" &&\n - \n - \t# if is not specified, use HEAD.\n --\texp_sha=${2:-$(git rev-parse --verify HEAD)} &&\n -+\thead=$(git rev-parse --verify HEAD) &&\n -+\texp_sha=${2:-$head} &&\n - \n - \t# default options for git checkout: -b\n - \tif [ -z \"$3\" ]; then\n -@@ t/t2018-checkout-branch.sh: do_checkout () {\n - \n - \tif test -z \"$should_fail\"\n - \tthen\n + \t\ttest_must_fail git checkout $opts $exp_branch $exp_sha\n + \telse\n + \t\tgit checkout $opts $exp_branch $exp_sha &&\n -\t\ttest $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&\n -\t\ttest $exp_sha = $(git rev-parse --verify HEAD)\n +\t\techo \"$exp_ref\" >ref.expect &&\n 6: 7c26b921c3 ! 7: 71f84811c7 t2018: replace \"sha\" with \"oid\"\n @@ t/t2018-checkout-branch.sh: test_description='checkout'\n \n . ./test-lib.sh\n \n --# Arguments: []\n -+# Arguments: []\n +-# Arguments: [!] []\n ++# Arguments: [!] []\n #\n # Runs \"git checkout\" to switch to , testing that\n #\n @@ t/t2018-checkout-branch.sh: test_description='checkout'\n +# 2) HEAD is ; if is not specified, the old HEAD is used.\n #\n # If is not specified, \"git checkout\" is run with -b.\n - do_checkout () {\n + #\n @@ t/t2018-checkout-branch.sh: do_checkout () {\n \texp_branch=$1 &&\n \texp_ref=\"refs/heads/$exp_branch\" &&\n \n -\t# if is not specified, use HEAD.\n +-\texp_sha=${2:-$(git rev-parse --verify HEAD)} &&\n +\t# if is not specified, use HEAD.\n - \thead=$(git rev-parse --verify HEAD) &&\n --\texp_sha=${2:-$head} &&\n -+\texp_oid=${2:-$head} &&\n ++\texp_oid=${2:-$(git rev-parse --verify HEAD)} &&\n \n \t# default options for git checkout: -b\n - \tif [ -z \"$3\" ]; then\n + \tif test -z \"$3\"\n @@ t/t2018-checkout-branch.sh: do_checkout () {\n - \t\topts=\"$3\"\n - \tfi\n \n --\t$should_fail git checkout $opts $exp_branch $exp_sha &&\n -+\t$should_fail git checkout $opts $exp_branch $exp_oid &&\n - \n - \tif test -z \"$should_fail\"\n + \tif test -n \"$should_fail\"\n \tthen\n +-\t\ttest_must_fail git checkout $opts $exp_branch $exp_sha\n ++\t\ttest_must_fail git checkout $opts $exp_branch $exp_oid\n + \telse\n +-\t\tgit checkout $opts $exp_branch $exp_sha &&\n ++\t\tgit checkout $opts $exp_branch $exp_oid &&\n \t\techo \"$exp_ref\" >ref.expect &&\n \t\tgit rev-parse --symbolic-full-name HEAD >ref.actual &&\n \t\ttest_cmp ref.expect ref.actual &&\n 7: 9e37358f38 ! 8: f0da1d6350 t3030: use test_path_is_missing()\n @@ Metadata\n ## Commit message ##\n t3030: use test_path_is_missing()\n \n - Previously, we would use `test_must_fail test -d` to ensure that the\n - directory is removed. However, test_must_fail() should only be used for\n - git commands. Use test_path_is_missing() instead to check that the\n - directory has been removed.\n + We use `test_must_fail test -d` to ensure that the directory is removed.\n + However, test_must_fail() should only be used for git commands. Use\n + test_path_is_missing() instead to check that the directory has been\n + removed.\n \n ## t/t3030-merge-recursive.sh ##\n @@ t/t3030-merge-recursive.sh: test_expect_success 'merge removes empty directories' '\n 8: 9705769841 ! 9: 46fe82b856 t3310: extract common no_notes_merge_left()\n @@ Metadata\n Author: Denton Liu \n \n ## Commit message ##\n - t3310: extract common no_notes_merge_left()\n + t3310: extract common notes_merge_files_gone()\n \n - We had many statements which were duplicated. Extract and replace these\n - duplicate statements with no_notes_merge_left().\n + We have many statements which are duplicated. Extract and replace these\n + duplicate statements with notes_merge_files_gone().\n \n While we're at it, replace the test_might_fail(), which should only be\n - used on git commands, with a compound command that always returns 0,\n - even if the underlying ls fails.\n + used on git commands.\n \n Also, remove the redirection from stderr to /dev/null. This is because\n the test scripts automatically suppress output already. Otherwise, if a\n @@ t/t3310-notes-merge-manual-resolve.sh: verify_notes () {\n \ttest_cmp \"expect_log_$notes_ref\" \"output_log_$notes_ref\"\n }\n \n -+no_notes_merge_left () {\n ++notes_merge_files_gone () {\n ++\t# No .git/NOTES_MERGE_* files left\n +\t{ ls .git/NOTES_MERGE_* >output || :; } &&\n +\ttest_must_be_empty output\n +}\n @@ t/t3310-notes-merge-manual-resolve.sh: EOF\n -\t# No .git/NOTES_MERGE_* files left\n -\ttest_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n -\ttest_must_be_empty output &&\n -+\tno_notes_merge_left &&\n ++\tnotes_merge_files_gone &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n \ttest \"$(git rev-parse refs/notes/m^1)\" = \"$(cat pre_merge_y)\" &&\n \ttest \"$(git rev-parse refs/notes/m^2)\" = \"$(cat pre_merge_z)\" &&\n @@ t/t3310-notes-merge-manual-resolve.sh: test_expect_success 'redo merge of z into\n -\t# No .git/NOTES_MERGE_* files left\n -\ttest_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n -\ttest_must_be_empty output &&\n -+\tno_notes_merge_left &&\n ++\tnotes_merge_files_gone &&\n \t# m has not moved (still == y)\n \ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\" &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n @@ t/t3310-notes-merge-manual-resolve.sh: EOF\n -\t# No .git/NOTES_MERGE_* files left\n -\ttest_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n -\ttest_must_be_empty output &&\n -+\tno_notes_merge_left &&\n ++\tnotes_merge_files_gone &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n \ttest \"$(git rev-parse refs/notes/m^1)\" = \"$(cat pre_merge_y)\" &&\n \ttest \"$(git rev-parse refs/notes/m^2)\" = \"$(cat pre_merge_z)\" &&\n @@ t/t3310-notes-merge-manual-resolve.sh: EOF\n -\t# No .git/NOTES_MERGE_* files left\n -\ttest_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n -\ttest_must_be_empty output &&\n -+\tno_notes_merge_left &&\n ++\tnotes_merge_files_gone &&\n \t# m has not moved (still == w)\n \ttest \"$(git rev-parse refs/notes/m)\" = \"$(git rev-parse refs/notes/w)\" &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n 9: b6d8f6a6e1 ! 10: 32d2051b31 t3415: stop losing return codes of git commands\n @@ Metadata\n ## Commit message ##\n t3415: stop losing return codes of git commands\n \n - When a command is in a non-assignment subshell, the return code will be\n - lost in favour of the surrounding command's. Rewrite instances of this\n - such that the return code of git commands is no longer lost.\n + Fix invocations of git commands so their exit codes are not lost\n + within non-assignment command substitutions.\n \n ## t/t3415-rebase-autosquash.sh ##\n @@ t/t3415-rebase-autosquash.sh: test_auto_fixup () {\n10: e2818d1761 ! 11: 8b716c6a81 t3415: increase granularity of test_auto_{fixup,squash}()\n @@ Metadata\n ## Commit message ##\n t3415: increase granularity of test_auto_{fixup,squash}()\n \n - We used to use `test_must_fail test_auto_{fixup,squash}` which would\n + We are using `test_must_fail test_auto_{fixup,squash}` which would\n ensure that the function failed. However, this is a little ham-fisted as\n there is no way of ensuring that the expected part of the function\n actually fails.\n11: 0357bb8533 ! 12: ea36028d53 t3419: stop losing return code of git command\n @@ Metadata\n ## Commit message ##\n t3419: stop losing return code of git command\n \n - We had an instance of a git command in a non-assignment command\n - substitution. Its return code was lost so we would not be able to detect\n - if the command failed for some reason. Since we were testing to make\n - sure the output of the command was empty, rewrite it in a more canonical\n - way.\n + Fix invocation of git command so its exit codes is not lost within\n + a non-assignment command substitution.\n \n ## t/t3419-rebase-patch-id.sh ##\n @@ t/t3419-rebase-patch-id.sh: do_tests () {\n12: 35e32f21e1 < -: ---------- t3504: don't use `test_must_fail test_cmp`\n -: ---------- > 13: 88134bb6d1 t3504: do check for conflict marker after failed cherry-pick\n13: 1c38a5b3f7 ! 14: 96310b7d28 t3507: fix indentation\n @@ Metadata\n ## Commit message ##\n t3507: fix indentation\n \n - We had some test cases which were indented 7-spaces instead of a tab.\n - Fix this by reindenting with a tab instead.\n + We have some test cases which are indented 7-spaces instead of a tab.\n + Reindent with a tab instead.\n \n This patch should appear empty with `--ignore-all-space`.\n \n14: 7fa886809e = 15: 69125b8e2f t3507: use test_path_is_missing()\n15: e214e1a667 ! 16: b93ebc0e42 t4124: only mark git command with test_must_fail\n @@ t/t4124-apply-ws-rule.sh: prepare_test_file () {\n }\n \n apply_patch () {\n -+\tshould_fail= &&\n ++\tcmd_prefix= &&\n +\tif test \"x$1\" = 'x!'\n +\tthen\n -+\t\tshould_fail=test_must_fail &&\n ++\t\tcmd_prefix=test_must_fail &&\n +\t\tshift\n +\tfi &&\n \t>target &&\n \tsed -e \"s|\\([ab]\\)/file|\\1/target|\" \n---\n t/t2018-checkout-branch.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 822381dd9d..e18abce3d2 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-test_description='checkout '\n+test_description='checkout'\n \n . ./test-lib.sh\n \n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389324","messageId":"f0f541b520daaf910986a38130c21279afb396ed.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 02/16] t2018: add space between function name and ()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:52:59Z","receivedAt":"2020-01-07T04:53:38Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Add a space between the function name and () which brings the style in\nline with Git's coding guidelines.\n\nSigned-off-by: Denton Liu \n---\n t/t2018-checkout-branch.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex e18abce3d2..79b16e4677 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -12,7 +12,7 @@ test_description='checkout'\n # 2) HEAD is ; if is not specified, the old HEAD is used.\n #\n # If is not specified, \"git checkout\" is run with -b.\n-do_checkout() {\n+do_checkout () {\n \texp_branch=$1 &&\n \texp_ref=\"refs/heads/$exp_branch\" &&\n \n@@ -32,19 +32,19 @@ do_checkout() {\n \ttest $exp_sha = $(git rev-parse --verify HEAD)\n }\n \n-test_dirty_unmergeable() {\n+test_dirty_unmergeable () {\n \t! git diff --exit-code >/dev/null\n }\n \n-setup_dirty_unmergeable() {\n+setup_dirty_unmergeable () {\n \techo >>file1 change2\n }\n \n-test_dirty_mergeable() {\n+test_dirty_mergeable () {\n \t! git diff --cached --exit-code >/dev/null\n }\n \n-setup_dirty_mergeable() {\n+setup_dirty_mergeable () {\n \techo >file2 file2 &&\n \tgit add file2\n }\n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389325","messageId":"501eb147c3442b781b6b50be0691f326dc82f91a.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 03/16] t2018: improve style of if-statement","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:53:00Z","receivedAt":"2020-01-07T04:53:38Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Convert `[]` to `test` and break if-then into separate lines, both of\nwhich bring the style in line with Git's coding guidelines.\n\nSigned-off-by: Denton Liu \n---\n t/t2018-checkout-branch.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 79b16e4677..61206a90fb 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -20,7 +20,8 @@ do_checkout () {\n \texp_sha=${2:-$(git rev-parse --verify HEAD)} &&\n \n \t# default options for git checkout: -b\n-\tif [ -z \"$3\" ]; then\n+\tif test -z \"$3\"\n+\tthen\n \t\topts=\"-b\"\n \telse\n \t\topts=\"$3\"\n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389326","messageId":"587e53053f02ad0867dc903688c8887602950bd3.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 04/16] t2018: use test_expect_code for failing git commands","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:53:01Z","receivedAt":"2020-01-07T04:53:38Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When we are expecting `git diff` to fail, we negate its status with\n`!`. However, if git ever exits unexpectedly, say due to a segfault, we\nwould not be able to tell the difference between that and a controlled\nfailure. Use `test_expect_code 1 git diff` instead so that if an\nunexpected failure occurs, we can catch it.\n\nWe have some instances of `test_must_fail test_dirty_{un,}mergable`.\nHowever, `test_must_fail` should only be used on git commands. Convert\nthese instances to use the `test_dirty_{un,}mergeable_discards_changes`\nhelper functions so that we remove the double-negation.\n\nWhile we're at it, remove redirections to /dev/null since output is\nsilenced when running without `-v` anyway.\n\nSigned-off-by: Denton Liu \n---\n t/t2018-checkout-branch.sh | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 61206a90fb..7ca55efc6b 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -34,7 +34,11 @@ do_checkout () {\n }\n \n test_dirty_unmergeable () {\n-\t! git diff --exit-code >/dev/null\n+\ttest_expect_code 1 git diff --exit-code\n+}\n+\n+test_dirty_unmergeable_discards_changes () {\n+\tgit diff --exit-code\n }\n \n setup_dirty_unmergeable () {\n@@ -42,7 +46,11 @@ setup_dirty_unmergeable () {\n }\n \n test_dirty_mergeable () {\n-\t! git diff --cached --exit-code >/dev/null\n+\ttest_expect_code 1 git diff --cached --exit-code\n+}\n+\n+test_dirty_mergeable_discards_changes () {\n+\tgit diff --cached --exit-code\n }\n \n setup_dirty_mergeable () {\n@@ -94,7 +102,7 @@ test_expect_success 'checkout -f -b to a new branch with unmergeable changes dis\n \n \t# still dirty and on branch1\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n-\ttest_must_fail test_dirty_unmergeable\n+\ttest_dirty_unmergeable_discards_changes\n '\n \n test_expect_success 'checkout -b to a new branch preserves mergeable changes' '\n@@ -112,7 +120,7 @@ test_expect_success 'checkout -f -b to a new branch with mergeable changes disca\n \ttest_when_finished git reset --hard HEAD &&\n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n-\ttest_must_fail test_dirty_mergeable\n+\ttest_dirty_mergeable_discards_changes\n '\n \n test_expect_success 'checkout -b to an existing branch fails' '\n@@ -163,7 +171,7 @@ test_expect_success 'checkout -B to an existing branch with unmergeable changes\n test_expect_success 'checkout -f -B to an existing branch with unmergeable changes discards changes' '\n \t# still dirty and on branch1\n \tdo_checkout branch2 $HEAD1 \"-f -B\" &&\n-\ttest_must_fail test_dirty_unmergeable\n+\ttest_dirty_unmergeable_discards_changes\n '\n \n test_expect_success 'checkout -B to an existing branch preserves mergeable changes' '\n@@ -180,7 +188,7 @@ test_expect_success 'checkout -f -B to an existing branch with mergeable changes\n \n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 \"-f -B\" &&\n-\ttest_must_fail test_dirty_mergeable\n+\ttest_dirty_mergeable_discards_changes\n '\n \n test_expect_success 'checkout -b ' '\n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389336","messageId":"c43c11b91243b253702204241feb88adb3936ffe.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 05/16] t2018: teach do_checkout() to accept `!` arg","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:53:02Z","receivedAt":"2020-01-07T04:53:38Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We are running `test_must_fail do_checkout`. However,\n`test_must_fail` should only be used on git commands. Teach\ndo_checkout() to accept `!` as a potential first argument which will\ncause the function to expect the \"git checkout\" to fail.\n\nThis increases the granularity of the test as, instead of blindly\nchecking that do_checkout() failed, we check that only the specific\nexpected invocation of git fails.\n\nSigned-off-by: Denton Liu \n---\n t/t2018-checkout-branch.sh | 29 +++++++++++++++++++++--------\n 1 file changed, 21 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 7ca55efc6b..687ab6713c 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -4,7 +4,7 @@ test_description='checkout'\n \n . ./test-lib.sh\n \n-# Arguments: []\n+# Arguments: [!] []\n #\n # Runs \"git checkout\" to switch to , testing that\n #\n@@ -12,7 +12,16 @@ test_description='checkout'\n # 2) HEAD is ; if is not specified, the old HEAD is used.\n #\n # If is not specified, \"git checkout\" is run with -b.\n+#\n+# If the first argument is `!`, \"git checkout\" is expected to fail when\n+# it is run.\n do_checkout () {\n+\tshould_fail= &&\n+\tif test \"x$1\" = \"x!\"\n+\tthen\n+\t\tshould_fail=yes &&\n+\t\tshift\n+\tfi &&\n \texp_branch=$1 &&\n \texp_ref=\"refs/heads/$exp_branch\" &&\n \n@@ -27,10 +36,14 @@ do_checkout () {\n \t\topts=\"$3\"\n \tfi\n \n-\tgit checkout $opts $exp_branch $exp_sha &&\n-\n-\ttest $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&\n-\ttest $exp_sha = $(git rev-parse --verify HEAD)\n+\tif test -n \"$should_fail\"\n+\tthen\n+\t\ttest_must_fail git checkout $opts $exp_branch $exp_sha\n+\telse\n+\t\tgit checkout $opts $exp_branch $exp_sha &&\n+\t\ttest $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&\n+\t\ttest $exp_sha = $(git rev-parse --verify HEAD)\n+\tfi\n }\n \n test_dirty_unmergeable () {\n@@ -91,7 +104,7 @@ test_expect_success 'checkout -b to a new branch, set to an explicit ref' '\n \n test_expect_success 'checkout -b to a new branch with unmergeable changes fails' '\n \tsetup_dirty_unmergeable &&\n-\ttest_must_fail do_checkout branch2 $HEAD1 &&\n+\tdo_checkout ! branch2 $HEAD1 &&\n \ttest_dirty_unmergeable\n '\n \n@@ -125,7 +138,7 @@ test_expect_success 'checkout -f -b to a new branch with mergeable changes disca\n \n test_expect_success 'checkout -b to an existing branch fails' '\n \ttest_when_finished git reset --hard HEAD &&\n-\ttest_must_fail do_checkout branch2 $HEAD2\n+\tdo_checkout ! branch2 $HEAD2\n '\n \n test_expect_success 'checkout -b to @{-1} fails with the right branch name' '\n@@ -164,7 +177,7 @@ test_expect_success 'checkout -B to an existing branch with unmergeable changes\n \tgit checkout branch1 &&\n \n \tsetup_dirty_unmergeable &&\n-\ttest_must_fail do_checkout branch2 $HEAD1 -B &&\n+\tdo_checkout ! branch2 $HEAD1 -B &&\n \ttest_dirty_unmergeable\n '\n \n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389327","messageId":"e09a70f6ca9ec922f6da47f1e978de622f81698e.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 06/16] t2018: don't lose return code of git commands","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:53:03Z","receivedAt":"2020-01-07T04:53:39Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Fix invocations of git commands so their exit codes are not lost\nwithin non-assignment command substitutions.\n\nSigned-off-by: Denton Liu \n---\n t/t2018-checkout-branch.sh | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 687ab6713c..caf43571b1 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -41,8 +41,12 @@ do_checkout () {\n \t\ttest_must_fail git checkout $opts $exp_branch $exp_sha\n \telse\n \t\tgit checkout $opts $exp_branch $exp_sha &&\n-\t\ttest $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&\n-\t\ttest $exp_sha = $(git rev-parse --verify HEAD)\n+\t\techo \"$exp_ref\" >ref.expect &&\n+\t\tgit rev-parse --symbolic-full-name HEAD >ref.actual &&\n+\t\ttest_cmp ref.expect ref.actual &&\n+\t\techo \"$exp_sha\" >sha.expect &&\n+\t\tgit rev-parse --verify HEAD >sha.actual &&\n+\t\ttest_cmp sha.expect sha.actual\n \tfi\n }\n \n@@ -162,7 +166,8 @@ test_expect_success 'checkout -B to a merge base' '\n '\n \n test_expect_success 'checkout -B to an existing branch from detached HEAD resets branch to HEAD' '\n-\tgit checkout $(git rev-parse --verify HEAD) &&\n+\thead=$(git rev-parse --verify HEAD) &&\n+\tgit checkout \"$head\" &&\n \n \tdo_checkout branch2 \"\" -B\n '\n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389333","messageId":"71f84811c73174f3e8f85ca173dc65b37b698ad2.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 07/16] t2018: replace \"sha\" with \"oid\"","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:53:04Z","receivedAt":"2020-01-07T04:53:39Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"As part of the effort to become more hash-agnostic, replace all\ninstances of \"sha\" with \"oid\".\n\nSigned-off-by: Denton Liu \n---\n t/t2018-checkout-branch.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex caf43571b1..bbca7ef8da 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -4,12 +4,12 @@ test_description='checkout'\n \n . ./test-lib.sh\n \n-# Arguments: [!] []\n+# Arguments: [!] []\n #\n # Runs \"git checkout\" to switch to , testing that\n #\n # 1) we are on the specified branch, ;\n-# 2) HEAD is ; if is not specified, the old HEAD is used.\n+# 2) HEAD is ; if is not specified, the old HEAD is used.\n #\n # If is not specified, \"git checkout\" is run with -b.\n #\n@@ -25,8 +25,8 @@ do_checkout () {\n \texp_branch=$1 &&\n \texp_ref=\"refs/heads/$exp_branch\" &&\n \n-\t# if is not specified, use HEAD.\n-\texp_sha=${2:-$(git rev-parse --verify HEAD)} &&\n+\t# if is not specified, use HEAD.\n+\texp_oid=${2:-$(git rev-parse --verify HEAD)} &&\n \n \t# default options for git checkout: -b\n \tif test -z \"$3\"\n@@ -38,15 +38,15 @@ do_checkout () {\n \n \tif test -n \"$should_fail\"\n \tthen\n-\t\ttest_must_fail git checkout $opts $exp_branch $exp_sha\n+\t\ttest_must_fail git checkout $opts $exp_branch $exp_oid\n \telse\n-\t\tgit checkout $opts $exp_branch $exp_sha &&\n+\t\tgit checkout $opts $exp_branch $exp_oid &&\n \t\techo \"$exp_ref\" >ref.expect &&\n \t\tgit rev-parse --symbolic-full-name HEAD >ref.actual &&\n \t\ttest_cmp ref.expect ref.actual &&\n-\t\techo \"$exp_sha\" >sha.expect &&\n-\t\tgit rev-parse --verify HEAD >sha.actual &&\n-\t\ttest_cmp sha.expect sha.actual\n+\t\techo \"$exp_oid\" >oid.expect &&\n+\t\tgit rev-parse --verify HEAD >oid.actual &&\n+\t\ttest_cmp oid.expect oid.actual\n \tfi\n }\n \n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389328","messageId":"46fe82b856d217265b01bf2bcf1ff78fa9474fc3.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 09/16] t3310: extract common notes_merge_files_gone()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:53:06Z","receivedAt":"2020-01-07T04:53:41Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We have many statements which are duplicated. Extract and replace these\nduplicate statements with notes_merge_files_gone().\n\nWhile we're at it, replace the test_might_fail(), which should only be\nused on git commands.\n\nAlso, remove the redirection from stderr to /dev/null. This is because\nthe test scripts automatically suppress output already. Otherwise, if a\ndeveloper asks for verbose output via the `-v` flag, the stderr output\nmay be useful for debugging.\n\nSigned-off-by: Denton Liu \n---\n t/t3310-notes-merge-manual-resolve.sh | 22 ++++++++++------------\n 1 file changed, 10 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\nindex 2dea846e25..d746f4ba55 100755\n--- a/t/t3310-notes-merge-manual-resolve.sh\n+++ b/t/t3310-notes-merge-manual-resolve.sh\n@@ -32,6 +32,12 @@ verify_notes () {\n \ttest_cmp \"expect_log_$notes_ref\" \"output_log_$notes_ref\"\n }\n \n+notes_merge_files_gone () {\n+\t# No .git/NOTES_MERGE_* files left\n+\t{ ls .git/NOTES_MERGE_* >output || :; } &&\n+\ttest_must_be_empty output\n+}\n+\n cat <expect_notes_x\n 6e8e3febca3c2bb896704335cc4d0c34cb2f8715 $commit_sha4\n e5388c10860456ee60673025345fe2e153eb8cf8 $commit_sha3\n@@ -335,9 +341,7 @@ EOF\n y and z notes on 4th commit\n EOF\n \tgit notes merge --commit &&\n-\t# No .git/NOTES_MERGE_* files left\n-\ttest_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n-\ttest_must_be_empty output &&\n+\tnotes_merge_files_gone &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n \ttest \"$(git rev-parse refs/notes/m^1)\" = \"$(cat pre_merge_y)\" &&\n \ttest \"$(git rev-parse refs/notes/m^2)\" = \"$(cat pre_merge_z)\" &&\n@@ -397,9 +401,7 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \n test_expect_success 'abort notes merge' '\n \tgit notes merge --abort &&\n-\t# No .git/NOTES_MERGE_* files left\n-\ttest_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n-\ttest_must_be_empty output &&\n+\tnotes_merge_files_gone &&\n \t# m has not moved (still == y)\n \ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\" &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n@@ -464,9 +466,7 @@ EOF\n \techo \"new note on 5th commit\" > .git/NOTES_MERGE_WORKTREE/$commit_sha5 &&\n \t# Finalize merge\n \tgit notes merge --commit &&\n-\t# No .git/NOTES_MERGE_* files left\n-\ttest_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n-\ttest_must_be_empty output &&\n+\tnotes_merge_files_gone &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n \ttest \"$(git rev-parse refs/notes/m^1)\" = \"$(cat pre_merge_y)\" &&\n \ttest \"$(git rev-parse refs/notes/m^2)\" = \"$(cat pre_merge_z)\" &&\n@@ -553,9 +553,7 @@ EOF\n \n test_expect_success 'resolve situation by aborting the notes merge' '\n \tgit notes merge --abort &&\n-\t# No .git/NOTES_MERGE_* files left\n-\ttest_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&\n-\ttest_must_be_empty output &&\n+\tnotes_merge_files_gone &&\n \t# m has not moved (still == w)\n \ttest \"$(git rev-parse refs/notes/m)\" = \"$(git rev-parse refs/notes/w)\" &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389329","messageId":"32d2051b319449205b18048c800b906b675460f4.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 10/16] t3415: stop losing return codes of git commands","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:53:07Z","receivedAt":"2020-01-07T04:53:41Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Fix invocations of git commands so their exit codes are not lost\nwithin non-assignment command substitutions.\n\nSigned-off-by: Denton Liu \n---\n t/t3415-rebase-autosquash.sh | 113 +++++++++++++++++++++++++----------\n 1 file changed, 82 insertions(+), 31 deletions(-)\n\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 22d218698e..b0add36f82 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -37,8 +37,12 @@ test_auto_fixup () {\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual &&\n \tgit diff --exit-code $1 &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest 1 = $(git cat-file commit HEAD^ | grep first | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep first commit >actual &&\n+\ttest_line_count = 1 actual\n }\n \n test_expect_success 'auto fixup (option)' '\n@@ -66,8 +70,12 @@ test_auto_squash () {\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual &&\n \tgit diff --exit-code $1 &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest 2 = $(git cat-file commit HEAD^ | grep first | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep first commit >actual &&\n+\ttest_line_count = 2 actual\n }\n \n test_expect_success 'auto squash (option)' '\n@@ -94,7 +102,8 @@ test_expect_success 'misspelled auto squash' '\n \tgit log --oneline >actual &&\n \ttest_line_count = 4 actual &&\n \tgit diff --exit-code final-missquash &&\n-\ttest 0 = $(git rev-list final-missquash...HEAD | wc -l)\n+\tgit rev-list final-missquash...HEAD >list &&\n+\ttest_must_be_empty list\n '\n \n test_expect_success 'auto squash that matches 2 commits' '\n@@ -113,9 +122,15 @@ test_expect_success 'auto squash that matches 2 commits' '\n \tgit log --oneline >actual &&\n \ttest_line_count = 4 actual &&\n \tgit diff --exit-code final-multisquash &&\n-\ttest 1 = \"$(git cat-file blob HEAD^^:file1)\" &&\n-\ttest 2 = $(git cat-file commit HEAD^^ | grep first | wc -l) &&\n-\ttest 1 = $(git cat-file commit HEAD | grep first | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^^ >commit &&\n+\tgrep first commit >actual &&\n+\ttest_line_count = 2 actual &&\n+\tgit cat-file commit HEAD >commit &&\n+\tgrep first commit >actual &&\n+\ttest_line_count = 1 actual\n '\n \n test_expect_success 'auto squash that matches a commit after the squash' '\n@@ -134,25 +149,38 @@ test_expect_success 'auto squash that matches a commit after the squash' '\n \tgit log --oneline >actual &&\n \ttest_line_count = 5 actual &&\n \tgit diff --exit-code final-presquash &&\n-\ttest 0 = \"$(git cat-file blob HEAD^^:file1)\" &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest 1 = $(git cat-file commit HEAD | grep third | wc -l) &&\n-\ttest 1 = $(git cat-file commit HEAD^ | grep third | wc -l)\n+\techo 0 >expect &&\n+\tgit cat-file blob HEAD^^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD >commit &&\n+\tgrep third commit >actual &&\n+\ttest_line_count = 1 actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep third commit >actual &&\n+\ttest_line_count = 1 actual\n '\n test_expect_success 'auto squash that matches a sha1' '\n \tgit reset --hard base &&\n \techo 1 >file1 &&\n \tgit add -u &&\n \ttest_tick &&\n-\tgit commit -m \"squash! $(git rev-parse --short HEAD^)\" &&\n+\toid=$(git rev-parse --short HEAD^) &&\n+\tgit commit -m \"squash! $oid\" &&\n \tgit tag final-shasquash &&\n \ttest_tick &&\n \tgit rebase --autosquash -i HEAD^^^ &&\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual &&\n \tgit diff --exit-code final-shasquash &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest 1 = $(git cat-file commit HEAD^ | grep squash | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep squash commit >actual &&\n+\ttest_line_count = 1 actual\n '\n \n test_expect_success 'auto squash that matches longer sha1' '\n@@ -160,15 +188,20 @@ test_expect_success 'auto squash that matches longer sha1' '\n \techo 1 >file1 &&\n \tgit add -u &&\n \ttest_tick &&\n-\tgit commit -m \"squash! $(git rev-parse --short=11 HEAD^)\" &&\n+\toid=$(git rev-parse --short=11 HEAD^) &&\n+\tgit commit -m \"squash! $oid\" &&\n \tgit tag final-longshasquash &&\n \ttest_tick &&\n \tgit rebase --autosquash -i HEAD^^^ &&\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual &&\n \tgit diff --exit-code final-longshasquash &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest 1 = $(git cat-file commit HEAD^ | grep squash | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep squash commit >actual &&\n+\ttest_line_count = 1 actual\n '\n \n test_auto_commit_flags () {\n@@ -183,8 +216,12 @@ test_auto_commit_flags () {\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual &&\n \tgit diff --exit-code final-commit-$1 &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest $2 = $(git cat-file commit HEAD^ | grep first | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep first commit >actual &&\n+\ttest_line_count = $2 actual\n }\n \n test_expect_success 'use commit --fixup' '\n@@ -210,11 +247,15 @@ test_auto_fixup_fixup () {\n \t(\n \t\tset_cat_todo_editor &&\n \t\ttest_must_fail git rebase --autosquash -i HEAD^^^^ >actual &&\n+\t\thead=$(git rev-parse --short HEAD) &&\n+\t\tparent1=$(git rev-parse --short HEAD^) &&\n+\t\tparent2=$(git rev-parse --short HEAD^^) &&\n+\t\tparent3=$(git rev-parse --short HEAD^^^) &&\n \t\tcat >expected <<-EOF &&\n-\t\tpick $(git rev-parse --short HEAD^^^) first commit\n-\t\t$1 $(git rev-parse --short HEAD^) $1! first\n-\t\t$1 $(git rev-parse --short HEAD) $1! $2! first\n-\t\tpick $(git rev-parse --short HEAD^^) second commit\n+\t\tpick $parent3 first commit\n+\t\t$1 $parent1 $1! first\n+\t\t$1 $head $1! $2! first\n+\t\tpick $parent2 second commit\n \t\tEOF\n \t\ttest_cmp expected actual\n \t) &&\n@@ -222,13 +263,17 @@ test_auto_fixup_fixup () {\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual\n \tgit diff --exit-code \"final-$1-$2\" &&\n-\ttest 2 = \"$(git cat-file blob HEAD^:file1)\" &&\n+\techo 2 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep first commit >actual &&\n \tif test \"$1\" = \"fixup\"\n \tthen\n-\t\ttest 1 = $(git cat-file commit HEAD^ | grep first | wc -l)\n+\t\ttest_line_count = 1 actual\n \telif test \"$1\" = \"squash\"\n \tthen\n-\t\ttest 3 = $(git cat-file commit HEAD^ | grep first | wc -l)\n+\t\ttest_line_count = 3 actual\n \telse\n \t\tfalse\n \tfi\n@@ -256,19 +301,25 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' '\n \techo 2 >file1 &&\n \tgit add -u &&\n \ttest_tick &&\n-\tgit commit -m \"squash! $(git rev-parse --short HEAD^)\" &&\n+\toid=$(git rev-parse --short HEAD^) &&\n+\tgit commit -m \"squash! $oid\" &&\n \techo 1 >file1 &&\n \tgit add -u &&\n \ttest_tick &&\n-\tgit commit -m \"squash! $(git log -n 1 --format=%s HEAD~2)\" &&\n+\tsubject=$(git log -n 1 --format=%s HEAD~2) &&\n+\tgit commit -m \"squash! $subject\" &&\n \tgit tag final-squash-instFmt &&\n \ttest_tick &&\n \tgit rebase --autosquash -i HEAD~4 &&\n \tgit log --oneline >actual &&\n \ttest_line_count = 3 actual &&\n \tgit diff --exit-code final-squash-instFmt &&\n-\ttest 1 = \"$(git cat-file blob HEAD^:file1)\" &&\n-\ttest 2 = $(git cat-file commit HEAD^ | grep squash | wc -l)\n+\techo 1 >expect &&\n+\tgit cat-file blob HEAD^:file1 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit cat-file commit HEAD^ >commit &&\n+\tgrep squash commit >actual &&\n+\ttest_line_count = 2 actual\n '\n \n test_expect_success 'autosquash with empty custom instructionFormat' '\n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389330","messageId":"96310b7d289a912fa173dd15d55f3326005402cc.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 14/16] t3507: fix indentation","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:53:11Z","receivedAt":"2020-01-07T04:53:44Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We have some test cases which are indented 7-spaces instead of a tab.\nReindent with a tab instead.\n\nThis patch should appear empty with `--ignore-all-space`.\n\nSigned-off-by: Denton Liu \n---\n t/t3507-cherry-pick-conflict.sh | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 9b9b4ca8d4..2a0d119c8a 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -381,23 +381,23 @@ test_expect_success 'failed commit does not clear REVERT_HEAD' '\n '\n \n test_expect_success 'successful final commit clears revert state' '\n- pristine_detach picked-signed &&\n+\tpristine_detach picked-signed &&\n \n- test_must_fail git revert picked-signed base &&\n- echo resolved >foo &&\n- test_path_is_file .git/sequencer/todo &&\n- git commit -a &&\n- test_must_fail test_path_exists .git/sequencer\n+\ttest_must_fail git revert picked-signed base &&\n+\techo resolved >foo &&\n+\ttest_path_is_file .git/sequencer/todo &&\n+\tgit commit -a &&\n+\ttest_must_fail test_path_exists .git/sequencer\n '\n \n test_expect_success 'reset after final pick clears revert state' '\n- pristine_detach picked-signed &&\n+\tpristine_detach picked-signed &&\n \n- test_must_fail git revert picked-signed base &&\n- echo resolved >foo &&\n- test_path_is_file .git/sequencer/todo &&\n- git reset &&\n- test_must_fail test_path_exists .git/sequencer\n+\ttest_must_fail git revert picked-signed base &&\n+\techo resolved >foo &&\n+\ttest_path_is_file .git/sequencer/todo &&\n+\tgit reset &&\n+\ttest_must_fail test_path_exists .git/sequencer\n '\n \n test_expect_success 'revert conflict, diff3 -m style' '\n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389331","messageId":"b93ebc0e42dd145699c68587f4954775efbfa86d.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 16/16] t4124: only mark git command with test_must_fail","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:53:13Z","receivedAt":"2020-01-07T04:53:45Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail function should only be used for git commands since\nwe should assume that external commands work sanely. Since apply_patch\nwraps a sed and git invocation, rewrite it to accept an `!` argument\nwhich would cause only the git command to be prefixed with\n`test_must_fail`.\n\nSigned-off-by: Denton Liu \n---\n t/t4124-apply-ws-rule.sh | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\nindex ff51e9e789..971a5a7512 100755\n--- a/t/t4124-apply-ws-rule.sh\n+++ b/t/t4124-apply-ws-rule.sh\n@@ -35,9 +35,15 @@ prepare_test_file () {\n }\n \n apply_patch () {\n+\tcmd_prefix= &&\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\tcmd_prefix=test_must_fail &&\n+\t\tshift\n+\tfi &&\n \t>target &&\n \tsed -e \"s|\\([ab]\\)/file|\\1/target|\" \n\nThe test with disabled rerere should make sure that the cherry-picked\nresult does not have the conflict replaced with a recorded resolution.\n\nIt attempts to do so by ensuring that the file content is _not_ equal\nto some other file. That by itself is a very dubious check because just\nabout every random result of an incomplete cherry-pick would satisfy\nthe condition.\n\nIn this case, the intent was to check that the conflicting file does\n_not_ contain the resolved content. But the check actually uses the\nwrong reference file, but not the resolved content. Needless to say\nthat the non-equality is satisfied. And, on top of it, it uses a commit\nthat does not even touch the file that is checked.\n\nDo check for the expected result, which is content from both sides of\nthe merge and merge conflicts. (The latter we check for just the\nmiddle separator for brevity.)\n\nAs a side-effect, this also removes an incorrect use of test_must_fail.\n\nSigned-off-by: Johannes Sixt \nSigned-off-by: Denton Liu \n---\n t/t3504-cherry-pick-rerere.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh\nindex a267b2d144..80a0d08706 100755\n--- a/t/t3504-cherry-pick-rerere.sh\n+++ b/t/t3504-cherry-pick-rerere.sh\n@@ -94,8 +94,10 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '\n \n test_expect_success 'cherry-pick conflict without rerere' '\n \ttest_config rerere.enabled false &&\n-\ttest_must_fail git cherry-pick master &&\n-\ttest_must_fail test_cmp expect foo\n+\ttest_must_fail git cherry-pick foo-master &&\n+\tgrep ===== foo &&\n+\tgrep foo-dev foo &&\n+\tgrep foo-master foo\n '\n \n test_done\n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389338","messageId":"ea36028d53224d364ce95afe8b523e3400a7becd.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 12/16] t3419: stop losing return code of git command","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:53:09Z","receivedAt":"2020-01-07T04:53:47Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Fix invocation of git command so its exit codes is not lost within\na non-assignment command substitution.\n\nSigned-off-by: Denton Liu \n---\n t/t3419-rebase-patch-id.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\nindex 49f548cdb9..94552669ae 100755\n--- a/t/t3419-rebase-patch-id.sh\n+++ b/t/t3419-rebase-patch-id.sh\n@@ -80,7 +80,8 @@ do_tests () {\n \t\tgit commit -q -m \"change big file again\" &&\n \t\tgit checkout -q other^{} &&\n \t\tgit rebase master &&\n-\t\ttest_must_fail test -n \"$(git rev-list master...HEAD~)\"\n+\t\tgit rev-list master...HEAD~ >revs &&\n+\t\ttest_must_be_empty revs\n \t'\n \n \ttest_expect_success $pr 'do not drop patch' '\n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389332","messageId":"69125b8e2f33e9e538263829b0bcfc08c4812684.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 15/16] t3507: use test_path_is_missing()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:53:12Z","receivedAt":"2020-01-07T04:53:48Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() function should only be used for git commands since\nwe should assume that external commands work sanely. Replace\n`test_must_fail test_path_exists` with `test_path_is_missing` since we\nexpect these paths to not exist.\n\nSigned-off-by: Denton Liu \n---\n t/t3507-cherry-pick-conflict.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 2a0d119c8a..9bd482ce3b 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -168,7 +168,7 @@ test_expect_success 'successful final commit clears cherry-pick state' '\n \techo resolved >foo &&\n \ttest_path_is_file .git/sequencer/todo &&\n \tgit commit -a &&\n-\ttest_must_fail test_path_exists .git/sequencer\n+\ttest_path_is_missing .git/sequencer\n '\n \n test_expect_success 'reset after final pick clears cherry-pick state' '\n@@ -178,7 +178,7 @@ test_expect_success 'reset after final pick clears cherry-pick state' '\n \techo resolved >foo &&\n \ttest_path_is_file .git/sequencer/todo &&\n \tgit reset &&\n-\ttest_must_fail test_path_exists .git/sequencer\n+\ttest_path_is_missing .git/sequencer\n '\n \n test_expect_success 'failed cherry-pick produces dirty index' '\n@@ -387,7 +387,7 @@ test_expect_success 'successful final commit clears revert state' '\n \techo resolved >foo &&\n \ttest_path_is_file .git/sequencer/todo &&\n \tgit commit -a &&\n-\ttest_must_fail test_path_exists .git/sequencer\n+\ttest_path_is_missing .git/sequencer\n '\n \n test_expect_success 'reset after final pick clears revert state' '\n@@ -397,7 +397,7 @@ test_expect_success 'reset after final pick clears revert state' '\n \techo resolved >foo &&\n \ttest_path_is_file .git/sequencer/todo &&\n \tgit reset &&\n-\ttest_must_fail test_path_exists .git/sequencer\n+\ttest_path_is_missing .git/sequencer\n '\n \n test_expect_success 'revert conflict, diff3 -m style' '\n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389335","messageId":"f0da1d63500ad8b3e3ee4c1ccb3bde2342a9ec42.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 08/16] t3030: use test_path_is_missing()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:53:05Z","receivedAt":"2020-01-07T04:53:49Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We use `test_must_fail test -d` to ensure that the directory is removed.\nHowever, test_must_fail() should only be used for git commands. Use\ntest_path_is_missing() instead to check that the directory has been\nremoved.\n\nSigned-off-by: Denton Liu \n---\n t/t3030-merge-recursive.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex 2170758e38..d48d211a95 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -604,7 +604,7 @@ test_expect_success 'merge removes empty directories' '\n \tgit commit -mremoved-d/e &&\n \tgit checkout master &&\n \tgit merge -s recursive rm &&\n-\ttest_must_fail test -d d\n+\ttest_path_is_missing d\n '\n \n test_expect_success 'merge-recursive simple w/submodule' '\n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389334","messageId":"8b716c6a8198edb6a7f060cff797cb1085fdceab.1578372682.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v2 11/16] t3415: increase granularity of test_auto_{fixup,squash}()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-07T04:53:08Z","receivedAt":"2020-01-07T04:53:50Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We are using `test_must_fail test_auto_{fixup,squash}` which would\nensure that the function failed. However, this is a little ham-fisted as\nthere is no way of ensuring that the expected part of the function\nactually fails.\n\nIncrease the granularity by accepting an optional `!` first argument\nwhich will check that the rebase does not squash in any commits by\nensuring that there are still 4 commits. If `!` is not provided, the old\nlogic is run.\n\nThis patch may be better reviewed with `--ignore-all-space`.\n\nSigned-off-by: Denton Liu \n---\n t/t3415-rebase-autosquash.sh | 64 +++++++++++++++++++++++++-----------\n 1 file changed, 44 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex b0add36f82..093de9005b 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -25,6 +25,13 @@ test_expect_success setup '\n '\n \n test_auto_fixup () {\n+\tno_squash= &&\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\tno_squash=true\n+\t\tshift\n+\tfi &&\n+\n \tgit reset --hard base &&\n \techo 1 >file1 &&\n \tgit add -u &&\n@@ -35,14 +42,19 @@ test_auto_fixup () {\n \ttest_tick &&\n \tgit rebase $2 -i HEAD^^^ &&\n \tgit log --oneline >actual &&\n-\ttest_line_count = 3 actual &&\n-\tgit diff --exit-code $1 &&\n-\techo 1 >expect &&\n-\tgit cat-file blob HEAD^:file1 >actual &&\n-\ttest_cmp expect actual &&\n-\tgit cat-file commit HEAD^ >commit &&\n-\tgrep first commit >actual &&\n-\ttest_line_count = 1 actual\n+\tif test -n \"$no_squash\"\n+\tthen\n+\t\ttest_line_count = 4 actual\n+\telse\n+\t\ttest_line_count = 3 actual &&\n+\t\tgit diff --exit-code $1 &&\n+\t\techo 1 >expect &&\n+\t\tgit cat-file blob HEAD^:file1 >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit cat-file commit HEAD^ >commit &&\n+\t\tgrep first commit >actual &&\n+\t\ttest_line_count = 1 actual\n+\tfi\n }\n \n test_expect_success 'auto fixup (option)' '\n@@ -52,12 +64,19 @@ test_expect_success 'auto fixup (option)' '\n test_expect_success 'auto fixup (config)' '\n \tgit config rebase.autosquash true &&\n \ttest_auto_fixup final-fixup-config-true &&\n-\ttest_must_fail test_auto_fixup fixup-config-true-no --no-autosquash &&\n+\ttest_auto_fixup ! fixup-config-true-no --no-autosquash &&\n \tgit config rebase.autosquash false &&\n-\ttest_must_fail test_auto_fixup final-fixup-config-false\n+\ttest_auto_fixup ! final-fixup-config-false\n '\n \n test_auto_squash () {\n+\tno_squash= &&\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\tno_squash=true\n+\t\tshift\n+\tfi &&\n+\n \tgit reset --hard base &&\n \techo 1 >file1 &&\n \tgit add -u &&\n@@ -68,14 +87,19 @@ test_auto_squash () {\n \ttest_tick &&\n \tgit rebase $2 -i HEAD^^^ &&\n \tgit log --oneline >actual &&\n-\ttest_line_count = 3 actual &&\n-\tgit diff --exit-code $1 &&\n-\techo 1 >expect &&\n-\tgit cat-file blob HEAD^:file1 >actual &&\n-\ttest_cmp expect actual &&\n-\tgit cat-file commit HEAD^ >commit &&\n-\tgrep first commit >actual &&\n-\ttest_line_count = 2 actual\n+\tif test -n \"$no_squash\"\n+\tthen\n+\t\ttest_line_count = 4 actual\n+\telse\n+\t\ttest_line_count = 3 actual &&\n+\t\tgit diff --exit-code $1 &&\n+\t\techo 1 >expect &&\n+\t\tgit cat-file blob HEAD^:file1 >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit cat-file commit HEAD^ >commit &&\n+\t\tgrep first commit >actual &&\n+\t\ttest_line_count = 2 actual\n+\tfi\n }\n \n test_expect_success 'auto squash (option)' '\n@@ -85,9 +109,9 @@ test_expect_success 'auto squash (option)' '\n test_expect_success 'auto squash (config)' '\n \tgit config rebase.autosquash true &&\n \ttest_auto_squash final-squash-config-true &&\n-\ttest_must_fail test_auto_squash squash-config-true-no --no-autosquash &&\n+\ttest_auto_squash ! squash-config-true-no --no-autosquash &&\n \tgit config rebase.autosquash false &&\n-\ttest_must_fail test_auto_squash final-squash-config-false\n+\ttest_auto_squash ! final-squash-config-false\n '\n \n test_expect_success 'misspelled auto squash' '\n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"},{"id":"389598","messageId":"CAPig+cT_riAaTqvdCbFim0BvCaz1aUoHiS9u6dUsDWe46ZVKAw@mail.gmail.com","threadId":"52529","inReplyTo":"cover.1578372682.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 00/16] t: replace incorrect test_must_fail usage (part 2)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-10T21:45:33Z","receivedAt":"2020-01-10T21:45:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jan 6, 2020 at 11:53 PM Denton Liu wrote:\n> Changes since v1:\n> * Clean up as suggested by Eric\n> * Replace 12/16 with J6t's patch\n\nIn the future, please also provide a link in the mailing list archive\nto the previous round (v1 [1], in this case) to help reviewers\nunderstand what these bullet points mean since the points themselves\nare quite lacking in detail. Pointing people at the previous round\nhelps not only those who reviewed that round -- who may have already\nforgotten the details of their own and others' review comments -- but\nwill also help onboard people who start reviewing at this round.\n\n> Range-diff against v1:\n> -: ---------- > 3: 501eb147c3 t2018: improve style of if-statement\n> 16: 31aa0c7d15 < -: ---------- t4124: let sed open its own files\n\nDropping patches when re-rolling is fine, but please do not sneak in\nnew patches unrelated to any of the original changes in this series.\nNot only is it likely that such a patch will get overlooked by\nreviewers, but even when it is noticed, reviewers have to spend extra\ntime and effort trying to understand why the patch was added in the\nfirst place (especially since there is not even a mention of it in the\ncover letter). After digging into it, I can see that this newly-added\npatch (3/16) is an additional sensible style cleanup to the same test\nscript that many of the other patches are touching, so in some sense\none could argue that it's very vaguely related to v1, but please keep\nin mind the potentially negative effect it can have on reviewers[2]\nwhen new changes are added to a series, causing the series to diverge\nrather than converge.\n\n[1]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/\n[2]: https://lore.kernel.org/git/CAPig+cQqK-HiDjmnBFo-qeE6cZ73EveWg6Ygb-4BX3X_iPSJZA@mail.gmail.com/\n"},{"id":"389628","messageId":"CAPig+cSoDFVonhn=xoWasHcN_hJHVoTiMT7=aU9ujiW93qk8kg@mail.gmail.com","threadId":"52529","inReplyTo":"587e53053f02ad0867dc903688c8887602950bd3.1578372682.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 04/16] t2018: use test_expect_code for failing git commands","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-12T10:50:57Z","receivedAt":"2020-01-12T10:55:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jan 6, 2020 at 11:53 PM Denton Liu wrote:\n> t2018: use test_expect_code for failing git commands\n>\n> When we are expecting `git diff` to fail, we negate its status with\n> `!`. However, if git ever exits unexpectedly, say due to a segfault, we\n> would not be able to tell the difference between that and a controlled\n> failure. Use `test_expect_code 1 git diff` instead so that if an\n> unexpected failure occurs, we can catch it.\n>\n> We have some instances of `test_must_fail test_dirty_{un,}mergable`.\n> However, `test_must_fail` should only be used on git commands. Convert\n> these instances to use the `test_dirty_{un,}mergeable_discards_changes`\n> helper functions so that we remove the double-negation.\n\nI had to read all of this several times to understand what it was\ntrying to say. I think what made it difficult was a combination of\ntalking about using test_expect_code() for \"failing git commands\",\ncoupled with the use \"we\" in \"When we are ... we negate...\" which (to\nme) sounds as if you are describing the _desired_ way of coding this,\nnot the incorrect way. A possible rewrite:\n\n t2018: be more discerning when checking for expected exit codes\n\n Functions test_dirty_unmergeable() and test_dirty_mergeable()\n expect git-diff to exit with the specific code 1. However, rather\n than checking for that value explicitly, they instead negate the\n exit code. Unfortunately, this negation makes it impossible to\n distinguish the expected code from some other unexpected non-zero\n code, for instance, from a segmentation fault. Therefore, be more\n discerning by checking the exit code explicitly using\n test_expect_code().\n\n Furthermore, some callers of those functions want to negate the\n result again, and do so with test_must_fail(). However,\n test_must_fail() should only be used with git commands. Address\n this by introducing a couple new tiny helper functions which test\n the exact condition expected (without the unnecessarily confusing\n double-negation).\n\n> While we're at it, remove redirections to /dev/null since output is\n> silenced when running without `-v` anyway.\n\n> Signed-off-by: Denton Liu \n"},{"id":"390519","messageId":"4af0c20e2981ed7e361083a71cb25b9f4a55fa4f.1580069736.git.liu.denton@gmail.com","threadId":"52529","inReplyTo":"587e53053f02ad0867dc903688c8887602950bd3.1578372682.git.liu.denton@gmail.com","subject":"[PATCH v3] t2018: be more discerning when checking for expected exit codes","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-01-26T20:23:05Z","receivedAt":"2020-01-26T20:23:25Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Functions test_dirty_unmergeable() and test_dirty_mergeable()\nexpect git-diff to exit with the specific code 1. However, rather\nthan checking for that value explicitly, they instead negate the\nexit code. Unfortunately, this negation makes it impossible to\ndistinguish the expected code from some other unexpected non-zero\ncode, for instance, from a segmentation fault. Therefore, be more\ndiscerning by checking the exit code explicitly using\ntest_expect_code().\n\nFurthermore, some callers of those functions want to negate the\nresult again, and do so with test_must_fail(). However,\ntest_must_fail() should only be used with git commands. Address\nthis by introducing a couple new tiny helper functions which test\nthe exact condition expected (without the unnecessarily confusing\ndouble-negation).\n\nHelped-by: Eric Sunshine \nSigned-off-by: Denton Liu \n---\nThis patch is a complete replacement for 4/16: \"t2018: use\ntest_expect_code for failing git commands\"[1] in\n\"dl/test-must-fail-fixes-2\". It replaces the commit message with a\nmuch-improved version from Eric. Other than that, no code changes are\nmade.\n\n[1]: https://lore.kernel.org/git/587e53053f02ad0867dc903688c8887602950bd3.1578372682.git.liu.denton@gmail.com/\n\n t/t2018-checkout-branch.sh | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 61206a90fb..7ca55efc6b 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -34,7 +34,11 @@ do_checkout () {\n }\n \n test_dirty_unmergeable () {\n-\t! git diff --exit-code >/dev/null\n+\ttest_expect_code 1 git diff --exit-code\n+}\n+\n+test_dirty_unmergeable_discards_changes () {\n+\tgit diff --exit-code\n }\n \n setup_dirty_unmergeable () {\n@@ -42,7 +46,11 @@ setup_dirty_unmergeable () {\n }\n \n test_dirty_mergeable () {\n-\t! git diff --cached --exit-code >/dev/null\n+\ttest_expect_code 1 git diff --cached --exit-code\n+}\n+\n+test_dirty_mergeable_discards_changes () {\n+\tgit diff --cached --exit-code\n }\n \n setup_dirty_mergeable () {\n@@ -94,7 +102,7 @@ test_expect_success 'checkout -f -b to a new branch with unmergeable changes dis\n \n \t# still dirty and on branch1\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n-\ttest_must_fail test_dirty_unmergeable\n+\ttest_dirty_unmergeable_discards_changes\n '\n \n test_expect_success 'checkout -b to a new branch preserves mergeable changes' '\n@@ -112,7 +120,7 @@ test_expect_success 'checkout -f -b to a new branch with mergeable changes disca\n \ttest_when_finished git reset --hard HEAD &&\n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n-\ttest_must_fail test_dirty_mergeable\n+\ttest_dirty_mergeable_discards_changes\n '\n \n test_expect_success 'checkout -b to an existing branch fails' '\n@@ -163,7 +171,7 @@ test_expect_success 'checkout -B to an existing branch with unmergeable changes\n test_expect_success 'checkout -f -B to an existing branch with unmergeable changes discards changes' '\n \t# still dirty and on branch1\n \tdo_checkout branch2 $HEAD1 \"-f -B\" &&\n-\ttest_must_fail test_dirty_unmergeable\n+\ttest_dirty_unmergeable_discards_changes\n '\n \n test_expect_success 'checkout -B to an existing branch preserves mergeable changes' '\n@@ -180,7 +188,7 @@ test_expect_success 'checkout -f -B to an existing branch with mergeable changes\n \n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 \"-f -B\" &&\n-\ttest_must_fail test_dirty_mergeable\n+\ttest_dirty_mergeable_discards_changes\n '\n \n test_expect_success 'checkout -b ' '\n-- \n2.25.0.rc1.180.g49a268d3eb\n\n"}]}