{"thread":{"id":"53259","subject":"[PATCH 0/8] t: replace incorrect test_must_fail usage (part 4)","startedAt":"2020-04-20T08:54:56Z","lastAt":"2020-04-22T15:50:17Z","messageCount":20,"participants":["Denton Liu","Derrick Stolee","Eric Sunshine","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"395733","messageId":"cover.1587372771.git.liu.denton@gmail.com","threadId":"53259","inReplyTo":null,"subject":"[PATCH 0/8] t: replace incorrect test_must_fail usage (part 4)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-20T08:54:38Z","receivedAt":"2020-04-20T08:54:56Z","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 fourth part. It focuses on t[6-9]*.sh.\n\nThe first part can be found here[2]. The second part can be found\nhere[3]. The third part can be found here[4].\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[3]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/\n[4]: https://lore.kernel.org/git/cover.1585209554.git.liu.denton@gmail.com/\n\nDenton Liu (8):\n  t6030: use test_path_is_missing()\n  t7408: replace incorrect uses of test_must_fail\n  t7508: don't use `test_must_fail test_cmp`\n  t9141: use test_path_is_missing()\n  t9160: use test_path_is_missing()\n  t9164: don't use `test_must_fail test_cmp`\n  t9819: don't use test_must_fail with p4\n  t9902: don't use `test_must_fail __git_*`\n\n t/t6030-bisect-porcelain.sh            |  8 ++++----\n t/t7408-submodule-reference.sh         |  8 ++++----\n t/t7508-status.sh                      |  2 +-\n t/t9141-git-svn-multiple-branches.sh   |  8 ++++----\n t/t9160-git-svn-preserve-empty-dirs.sh |  4 ++--\n t/t9164-git-svn-dcommit-concurrent.sh  |  4 ++--\n t/t9819-git-p4-case-folding.sh         |  2 +-\n t/t9902-completion.sh                  | 12 ++++++------\n 8 files changed, 24 insertions(+), 24 deletions(-)\n\n-- \n2.26.0.159.g23e2136ad0\n\n"},{"id":"395734","messageId":"17db727121925cf0774e95a80ea13bbea8264931.1587372771.git.liu.denton@gmail.com","threadId":"53259","inReplyTo":"cover.1587372771.git.liu.denton@gmail.com","subject":"[PATCH 2/8] t7408: replace incorrect uses of test_must_fail","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-20T08:54:40Z","receivedAt":"2020-04-20T08:54:58Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"According to t/README, test_must_fail() should only be used to test for\nfailure in Git commands.\n\nReplace the invocation of `test_must_fail test_path_is_file` with\n`test_path_is_missing` since, in this test case, the path should not\nexist at all.\n\nIn all the cases where `test_must_fail test_alternate_is_used` appears,\ntest_alternate_is_used() fails because test_line_count() cannot open the\nnon-existent $alternates_file. Replace\n`test_must_fail test_alternate_is_used` with `test_path_is_missing` to\ntest for this directly.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n t/t7408-submodule-reference.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\nindex 34ac28c056..a3892f494b 100755\n--- a/t/t7408-submodule-reference.sh\n+++ b/t/t7408-submodule-reference.sh\n@@ -122,8 +122,8 @@ test_expect_success 'missing submodule alternate fails clone and submodule updat\n \t\t# update of the submodule succeeds\n \t\ttest_must_fail git submodule update --init &&\n \t\t# and we have no alternates:\n-\t\ttest_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&\n-\t\ttest_must_fail test_path_is_file sub/file1\n+\t\ttest_path_is_missing .git/modules/sub/objects/info/alternates &&\n+\t\ttest_path_is_missing sub/file1\n \t)\n '\n \n@@ -137,7 +137,7 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm\n \t\t# update of the submodule succeeds\n \t\tgit submodule update --init &&\n \t\t# and we have no alternates:\n-\t\ttest_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&\n+\t\ttest_path_is_missing .git/modules/sub/objects/info/alternates &&\n \t\ttest_path_is_file sub/file1\n \t)\n '\n@@ -182,7 +182,7 @@ check_that_two_of_three_alternates_are_used() {\n \t# immediate submodule has alternate:\n \ttest_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub &&\n \t# but nested submodule has no alternate:\n-\ttest_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub\n+\ttest_path_is_missing .git/modules/subwithsub/modules/sub/objects/info/alternates\n }\n \n \n-- \n2.26.0.159.g23e2136ad0\n\n"},{"id":"395735","messageId":"fd79ade3527e6be6b97f07d798a0ea38507f24c0.1587372771.git.liu.denton@gmail.com","threadId":"53259","inReplyTo":"cover.1587372771.git.liu.denton@gmail.com","subject":"[PATCH 3/8] t7508: don't use `test_must_fail test_cmp`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-20T08:54:41Z","receivedAt":"2020-04-20T08:54:59Z","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 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 <liu.denton@gmail.com>\n---\n t/t7508-status.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 482ce3510e..8e969f3e36 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1471,7 +1471,7 @@ test_expect_success '\"status.branch=true\" same as \"-b\"' '\n test_expect_success '\"status.branch=true\" different from \"--no-branch\"' '\n \tgit status -s --no-branch  >expected_nobranch &&\n \tgit -c status.branch=true status -s >actual &&\n-\ttest_must_fail test_cmp expected_nobranch actual\n+\t! test_cmp expected_nobranch actual\n '\n \n test_expect_success '\"status.branch=true\" weaker than \"--no-branch\"' '\n-- \n2.26.0.159.g23e2136ad0\n\n"},{"id":"395736","messageId":"f0b6b8916a2eac7aafdd9afe5d59306a7720b217.1587372771.git.liu.denton@gmail.com","threadId":"53259","inReplyTo":"cover.1587372771.git.liu.denton@gmail.com","subject":"[PATCH 1/8] t6030: use test_path_is_missing()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-20T08:54:39Z","receivedAt":"2020-04-20T08:55:00Z","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 -e` with `test_path_is_missing`.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n t/t6030-bisect-porcelain.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 821a0c88cf..1313142564 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -148,7 +148,7 @@ test_expect_success 'bisect start: no \".git/BISECT_START\" created if junk rev' '\n \ttest_must_fail git bisect start $HASH4 foo -- &&\n \tgit branch > branch.output &&\n \tgrep \"* other\" branch.output > /dev/null &&\n-\ttest_must_fail test -e .git/BISECT_START\n+\ttest_path_is_missing .git/BISECT_START\n '\n \n test_expect_success 'bisect start: existing \".git/BISECT_START\" not modified if junk rev' '\n@@ -166,7 +166,7 @@ test_expect_success 'bisect start: no \".git/BISECT_START\" if mistaken rev' '\n \ttest_must_fail git bisect start $HASH1 $HASH4 -- &&\n \tgit branch > branch.output &&\n \tgrep \"* other\" branch.output > /dev/null &&\n-\ttest_must_fail test -e .git/BISECT_START\n+\ttest_path_is_missing .git/BISECT_START\n '\n \n test_expect_success 'bisect start: no \".git/BISECT_START\" if checkout error' '\n@@ -175,7 +175,7 @@ test_expect_success 'bisect start: no \".git/BISECT_START\" if checkout error' '\n \tgit branch &&\n \tgit branch > branch.output &&\n \tgrep \"* other\" branch.output > /dev/null &&\n-\ttest_must_fail test -e .git/BISECT_START &&\n+\ttest_path_is_missing .git/BISECT_START &&\n \ttest -z \"$(git for-each-ref \"refs/bisect/*\")\" &&\n \tgit checkout HEAD hello\n '\n@@ -485,7 +485,7 @@ test_expect_success 'optimized merge base checks' '\n \tgit bisect bad &&\n \tgit bisect good \"$A_HASH\" > my_bisect_log4.txt &&\n \ttest_i18ngrep \"merge base must be tested\" my_bisect_log4.txt &&\n-\ttest_must_fail test -f \".git/BISECT_ANCESTORS_OK\"\n+\ttest_path_is_missing \".git/BISECT_ANCESTORS_OK\"\n '\n \n # This creates another side branch called \"parallel\" with some files\n-- \n2.26.0.159.g23e2136ad0\n\n"},{"id":"395737","messageId":"1eb50e1be2f548c65f5491a1554fc7d69fd261d4.1587372771.git.liu.denton@gmail.com","threadId":"53259","inReplyTo":"cover.1587372771.git.liu.denton@gmail.com","subject":"[PATCH 4/8] t9141: use test_path_is_missing()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-20T08:54:42Z","receivedAt":"2020-04-20T08:55: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 assume that external commands work sanely. Since, not only should\nthese directories not exist, but there shouldn't exist _any_ filesystem\nentity in these paths, replace `test_must_fail test -d` with\n`test_path_is_missing`.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n t/t9141-git-svn-multiple-branches.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9141-git-svn-multiple-branches.sh b/t/t9141-git-svn-multiple-branches.sh\nindex 8e7f7d68b7..bf168a3645 100755\n--- a/t/t9141-git-svn-multiple-branches.sh\n+++ b/t/t9141-git-svn-multiple-branches.sh\n@@ -90,10 +90,10 @@ test_expect_success 'Multiple branch or tag paths require -d' '\n \t) &&\n \t( cd svn_project &&\n \t\tsvn_cmd up &&\n-\t\ttest_must_fail test -d b_one/Nope &&\n-\t\ttest_must_fail test -d b_two/Nope &&\n-\t\ttest_must_fail test -d tags_A/Tagless &&\n-\t\ttest_must_fail test -d tags_B/Tagless\n+\t\ttest_path_is_missing b_one/Nope &&\n+\t\ttest_path_is_missing b_two/Nope &&\n+\t\ttest_path_is_missing tags_A/Tagless &&\n+\t\ttest_path_is_missing tags_B/Tagless\n \t)\n '\n \n-- \n2.26.0.159.g23e2136ad0\n\n"},{"id":"395738","messageId":"572d12a5fb4777b10417bb4fc7e38954796a8404.1587372771.git.liu.denton@gmail.com","threadId":"53259","inReplyTo":"cover.1587372771.git.liu.denton@gmail.com","subject":"[PATCH 5/8] t9160: use test_path_is_missing()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-20T08:54:43Z","receivedAt":"2020-04-20T08:55:03Z","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 assume that external commands work sanely. Since, not only should\nthis file not exist, but there shouldn't exit _any_ filesystem entity in\nthese paths, replace `test_must_fail test -f` with\n`test_path_is_missing`.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n t/t9160-git-svn-preserve-empty-dirs.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh\nindex 0ede3cfedb..36c6b1a12f 100755\n--- a/t/t9160-git-svn-preserve-empty-dirs.sh\n+++ b/t/t9160-git-svn-preserve-empty-dirs.sh\n@@ -86,8 +86,8 @@ test_expect_success 'remove non-last entry from directory' '\n \t\tcd \"$GIT_REPO\" &&\n \t\tgit checkout HEAD~2\n \t) &&\n-\ttest_must_fail test -f \"$GIT_REPO\"/2/.gitignore &&\n-\ttest_must_fail test -f \"$GIT_REPO\"/3/.gitignore\n+\ttest_path_is_missing \"$GIT_REPO\"/2/.gitignore &&\n+\ttest_path_is_missing \"$GIT_REPO\"/3/.gitignore\n '\n \n # After re-cloning the repository with --placeholder-file specified, there\n-- \n2.26.0.159.g23e2136ad0\n\n"},{"id":"395739","messageId":"dfccb04e2d03656e18286bcca2c558e19d748ffd.1587372771.git.liu.denton@gmail.com","threadId":"53259","inReplyTo":"cover.1587372771.git.liu.denton@gmail.com","subject":"[PATCH 6/8] t9164: don't use `test_must_fail test_cmp`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-20T08:54:44Z","receivedAt":"2020-04-20T08:55:05Z","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 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 <liu.denton@gmail.com>\n---\n t/t9164-git-svn-dcommit-concurrent.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh\nindex 90346ff4e9..8466269bf5 100755\n--- a/t/t9164-git-svn-dcommit-concurrent.sh\n+++ b/t/t9164-git-svn-dcommit-concurrent.sh\n@@ -92,7 +92,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '\n \t\techo 1 >> file &&\n \t\tsvn_cmd commit -m \"changing file\" &&\n \t\tsvn_cmd up &&\n-\t\ttest_must_fail test_cmp auto_updated_file au_file_saved\n+\t\t! test_cmp auto_updated_file au_file_saved\n \t)\n '\n \n@@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' '\n \t\techo 2 >> file &&\n \t\tsvn_cmd commit -m \"changing file once again\" &&\n \t\techo 3 >> file &&\n-\t\ttest_must_fail svn_cmd commit -m \"this commit should fail\" &&\n+\t\t! svn_cmd commit -m \"this commit should fail\" &&\n \t\tsvn_cmd revert file\n \t)\n '\n-- \n2.26.0.159.g23e2136ad0\n\n"},{"id":"395740","messageId":"6cfa2c447e1196d6c4325aff9fac52434d10fda8.1587372771.git.liu.denton@gmail.com","threadId":"53259","inReplyTo":"cover.1587372771.git.liu.denton@gmail.com","subject":"[PATCH 8/8] t9902: don't use `test_must_fail __git_*`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-20T08:54:46Z","receivedAt":"2020-04-20T08:55:11Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We should only use test_must_fail() to test git commands. Replace\n`test_must_fail` with `!` so that we don't use test_must_fail() on the\ncompletion functions.\n\nThis is done because test_must_fail() is used to except git exiting with\nan expected error but it will still return an error if it detects\nunexpected errors such as a segfault. In the case of these completion\nfunctions, the return codes of the git commands aren't checked and, most\nof the time, they will just explicitly return 1 or have an unrelated\ncommand return 0. As a result, it doesn't really make sense to use\n`test_must_fail` so use `!` instead.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n t/t9902-completion.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 5505e5aa24..320c755971 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -294,7 +294,7 @@ test_expect_success '__git_find_repo_path - \"git -C\" while .git directory in par\n test_expect_success '__git_find_repo_path - non-existing path in \"git -C\"' '\n \t(\n \t\t__git_C_args=(-C non-existing) &&\n-\t\ttest_must_fail __git_find_repo_path &&\n+\t\t! __git_find_repo_path &&\n \t\tprintf \"$__git_repo_path\" >\"$actual\"\n \t) &&\n \ttest_must_be_empty \"$actual\"\n@@ -303,7 +303,7 @@ test_expect_success '__git_find_repo_path - non-existing path in \"git -C\"' '\n test_expect_success '__git_find_repo_path - non-existing path in $__git_dir' '\n \t(\n \t\t__git_dir=\"non-existing\" &&\n-\t\ttest_must_fail __git_find_repo_path &&\n+\t\t! __git_find_repo_path &&\n \t\tprintf \"$__git_repo_path\" >\"$actual\"\n \t) &&\n \ttest_must_be_empty \"$actual\"\n@@ -313,7 +313,7 @@ test_expect_success '__git_find_repo_path - non-existing $GIT_DIR' '\n \t(\n \t\tGIT_DIR=\"$ROOT/non-existing\" &&\n \t\texport GIT_DIR &&\n-\t\ttest_must_fail __git_find_repo_path &&\n+\t\t! __git_find_repo_path &&\n \t\tprintf \"$__git_repo_path\" >\"$actual\"\n \t) &&\n \ttest_must_be_empty \"$actual\"\n@@ -362,7 +362,7 @@ test_expect_success '__git_find_repo_path - not a git repository' '\n \t\tcd non-repo &&\n \t\tGIT_CEILING_DIRECTORIES=\"$ROOT\" &&\n \t\texport GIT_CEILING_DIRECTORIES &&\n-\t\ttest_must_fail __git_find_repo_path &&\n+\t\t! __git_find_repo_path &&\n \t\tprintf \"$__git_repo_path\" >\"$actual\"\n \t) &&\n \ttest_must_be_empty \"$actual\"\n@@ -381,7 +381,7 @@ test_expect_success '__gitdir - finds repo' '\n test_expect_success '__gitdir - returns error when cannot find repo' '\n \t(\n \t\t__git_dir=\"non-existing\" &&\n-\t\ttest_must_fail __gitdir >\"$actual\"\n+\t\t! __gitdir >\"$actual\"\n \t) &&\n \ttest_must_be_empty \"$actual\"\n '\n@@ -608,7 +608,7 @@ test_expect_success '__git_is_configured_remote' '\n \tgit remote add remote_2 git://remote_2 &&\n \t(\n \t\tverbose __git_is_configured_remote remote_2 &&\n-\t\ttest_must_fail __git_is_configured_remote non-existent\n+\t\t! __git_is_configured_remote non-existent\n \t)\n '\n \n-- \n2.26.0.159.g23e2136ad0\n\n"},{"id":"395741","messageId":"e0d2bf4a204d56a629a417aa798e9efc208f4b08.1587372771.git.liu.denton@gmail.com","threadId":"53259","inReplyTo":"cover.1587372771.git.liu.denton@gmail.com","subject":"[PATCH 7/8] t9819: don't use test_must_fail with p4","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-20T08:54:45Z","receivedAt":"2020-04-20T08:55:17Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We were using `test_must_fail p4` to test that the p4 command failed as\nexpected. However, test_must_fail() is used to ensure that commands fail\nin an expected way, not due to something like a segv. Since we are not\nin the business of verifying the sanity of the external world, replace\n`test_must_fail p4` with `! p4` and assume that the `p4` command does\nnot die unexpectedly.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n t/t9819-git-p4-case-folding.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh\nindex 600ce1e0b0..b4d93f0c17 100755\n--- a/t/t9819-git-p4-case-folding.sh\n+++ b/t/t9819-git-p4-case-folding.sh\n@@ -30,7 +30,7 @@ test_expect_success 'Check p4 is in case-folding mode' '\n \t\tcd \"$cli\" &&\n \t\t>lc/FILE.TXT &&\n \t\tp4 add lc/FILE.TXT &&\n-\t\ttest_must_fail p4 submit -d \"Cannot add file differing only in case\" lc/FILE.TXT\n+\t\t! p4 submit -d \"Cannot add file differing only in case\" lc/FILE.TXT\n \t)\n '\n \n-- \n2.26.0.159.g23e2136ad0\n\n"},{"id":"395742","messageId":"e231e2db-ecbd-5b74-f563-551d03f568fb@gmail.com","threadId":"53259","inReplyTo":"cover.1587372771.git.liu.denton@gmail.com","subject":"Re: [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-04-20T11:49:49Z","receivedAt":"2020-04-20T11:49:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/20/2020 4:54 AM, Denton Liu wrote:\n> The overall scope of these patches is to replace inappropriate uses of\n> test_must_fail. IOW, we should only allow test_must_fail to run on `git`\n> and `test-tool`. Ultimately, we will conclude by making test_must_fail\n> error out on non-git commands. An advance view of the final series can\n> be found here[1].\n\nI appreciate your efforts here to use best-practices throughout the test\nsuite. Too often I look at a test script as an example on which to base\na new test, but then I just repeat a bad pattern.\n\nThis series looks good to me.\n\nThanks,\n-Stolee\n"},{"id":"395746","messageId":"CAPig+cQ6XS=ZDhAKGuDiGM4zcoxUhnghMY250qYLjuT8YZaoMg@mail.gmail.com","threadId":"53259","inReplyTo":"dfccb04e2d03656e18286bcca2c558e19d748ffd.1587372771.git.liu.denton@gmail.com","subject":"Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-04-20T16:21:37Z","receivedAt":"2020-04-20T16:21:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote:\n> The test_must_fail function should only be used for git commands since\n> we 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 <liu.denton@gmail.com>\n> ---\n> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh\n> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' '\n> -               test_must_fail svn_cmd commit -m \"this commit should fail\" &&\n> +               ! svn_cmd commit -m \"this commit should fail\" &&\n\nHmm, this doesn't look like 'test_cmp' mentioned in the commit message.\n"},{"id":"395759","messageId":"xmqqr1whrkaa.fsf@gitster.c.googlers.com","threadId":"53259","inReplyTo":"CAPig+cQ6XS=ZDhAKGuDiGM4zcoxUhnghMY250qYLjuT8YZaoMg@mail.gmail.com","subject":"Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-20T20:09:49Z","receivedAt":"2020-04-20T20:09:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote:\n>> The test_must_fail function should only be used for git commands since\n>> we 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 <liu.denton@gmail.com>\n>> ---\n>> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh\n>> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' '\n>> -               test_must_fail svn_cmd commit -m \"this commit should fail\" &&\n>> +               ! svn_cmd commit -m \"this commit should fail\" &&\n>\n> Hmm, this doesn't look like 'test_cmp' mentioned in the commit message.\n\nYeah, the other hunk is about test_cmp and this hunk is about\nsvn_cmd.  The stated rationale applies to both wrappers, I think.\n\n    Subject: [PATCH 6/8] t9164: use test_must_fail only on git\n\n    The `test_must_fail` function should only be used for git commands;\n    we are not in the business of catching segmentation fault by external\n    commands.  Shell helper functions test_cmp and svn_cmd used in this\n    script are wrappers around external commands, so just use `! cmd`\n    instead of `test_must_fail cmd`\n\nperhaps, without any change to the code?\n\n"},{"id":"395760","messageId":"CAPig+cTdwZjSVePSPTj4GxJm9CvGXppRNxjvsCTdU8TA1NRDkg@mail.gmail.com","threadId":"53259","inReplyTo":"xmqqr1whrkaa.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-04-20T20:13:57Z","receivedAt":"2020-04-20T20:14:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 20, 2020 at 4:09 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > Hmm, this doesn't look like 'test_cmp' mentioned in the commit message.\n>\n> Yeah, the other hunk is about test_cmp and this hunk is about\n> svn_cmd.  The stated rationale applies to both wrappers, I think.\n>\n>     Subject: [PATCH 6/8] t9164: use test_must_fail only on git\n>\n>     The `test_must_fail` function should only be used for git commands;\n>     we are not in the business of catching segmentation fault by external\n>     commands.  Shell helper functions test_cmp and svn_cmd used in this\n>     script are wrappers around external commands, so just use `! cmd`\n>     instead of `test_must_fail cmd`\n>\n> perhaps, without any change to the code?\n\nThat sounds fine.\n"},{"id":"395854","messageId":"20200421201627.GA9357@generichostname","threadId":"53259","inReplyTo":"xmqqr1whrkaa.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-21T20:16:27Z","receivedAt":"2020-04-21T20:16:33Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Mon, Apr 20, 2020 at 01:09:49PM -0700, Junio C Hamano wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> \n> > On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote:\n> >> The test_must_fail function should only be used for git commands since\n> >> we 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 <liu.denton@gmail.com>\n> >> ---\n> >> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh\n> >> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' '\n> >> -               test_must_fail svn_cmd commit -m \"this commit should fail\" &&\n> >> +               ! svn_cmd commit -m \"this commit should fail\" &&\n> >\n> > Hmm, this doesn't look like 'test_cmp' mentioned in the commit message.\n> \n> Yeah, the other hunk is about test_cmp and this hunk is about\n> svn_cmd.  The stated rationale applies to both wrappers, I think.\n> \n>     Subject: [PATCH 6/8] t9164: use test_must_fail only on git\n> \n>     The `test_must_fail` function should only be used for git commands;\n>     we are not in the business of catching segmentation fault by external\n>     commands.  Shell helper functions test_cmp and svn_cmd used in this\n>     script are wrappers around external commands, so just use `! cmd`\n>     instead of `test_must_fail cmd`\n> \n> perhaps, without any change to the code?\n\nThanks, this looks good to me too.\n"},{"id":"395857","messageId":"xmqqa734muw7.fsf@gitster.c.googlers.com","threadId":"53259","inReplyTo":"20200421201627.GA9357@generichostname","subject":"Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-21T20:44:08Z","receivedAt":"2020-04-21T20:44:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> On Mon, Apr 20, 2020 at 01:09:49PM -0700, Junio C Hamano wrote:\n>> Eric Sunshine <sunshine@sunshineco.com> writes:\n>> \n>> > On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote:\n>> >> The test_must_fail function should only be used for git commands since\n>> >> we 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 <liu.denton@gmail.com>\n>> >> ---\n>> >> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh\n>> >> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' '\n>> >> -               test_must_fail svn_cmd commit -m \"this commit should fail\" &&\n>> >> +               ! svn_cmd commit -m \"this commit should fail\" &&\n>> >\n>> > Hmm, this doesn't look like 'test_cmp' mentioned in the commit message.\n>> \n>> Yeah, the other hunk is about test_cmp and this hunk is about\n>> svn_cmd.  The stated rationale applies to both wrappers, I think.\n>> \n>>     Subject: [PATCH 6/8] t9164: use test_must_fail only on git\n>> \n>>     The `test_must_fail` function should only be used for git commands;\n>>     we are not in the business of catching segmentation fault by external\n>>     commands.  Shell helper functions test_cmp and svn_cmd used in this\n>>     script are wrappers around external commands, so just use `! cmd`\n>>     instead of `test_must_fail cmd`\n>> \n>> perhaps, without any change to the code?\n>\n> Thanks, this looks good to me too.\n\nThanks.  \n\nSo the 4-patch test-must-fail-fix series is now complete?  Whee.\n\n\n"},{"id":"395862","messageId":"90faeb7a-1c6a-3fc6-6410-5e264c9340e8@kdbg.org","threadId":"53259","inReplyTo":"fd79ade3527e6be6b97f07d798a0ea38507f24c0.1587372771.git.liu.denton@gmail.com","subject":"Re: [PATCH 3/8] t7508: don't use `test_must_fail test_cmp`","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2020-04-21T20:59:50Z","receivedAt":"2020-04-21T20:59:54Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.04.20 um 10:54 schrieb Denton Liu:\n> --- a/t/t7508-status.sh\n> +++ b/t/t7508-status.sh\n> @@ -1471,7 +1471,7 @@ test_expect_success '\"status.branch=true\" same as \"-b\"' '\n>  test_expect_success '\"status.branch=true\" different from \"--no-branch\"' '\n>  \tgit status -s --no-branch  >expected_nobranch &&\n>  \tgit -c status.branch=true status -s >actual &&\n> -\ttest_must_fail test_cmp expected_nobranch actual\n> +\t! test_cmp expected_nobranch actual\n>  '\n\nNot your fault, but this is, of course, a very weak test case. Check\nthat some output that the program generates is _not_ equal to something\nelse? That condition should be very easy to satisfy.\n\n-- Hannes\n"},{"id":"395865","messageId":"60627dc6-fb9c-549f-d76d-0e110c5b0de2@kdbg.org","threadId":"53259","inReplyTo":"6cfa2c447e1196d6c4325aff9fac52434d10fda8.1587372771.git.liu.denton@gmail.com","subject":"Re: [PATCH 8/8] t9902: don't use `test_must_fail __git_*`","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2020-04-21T21:16:09Z","receivedAt":"2020-04-21T21:16:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.04.20 um 10:54 schrieb Denton Liu:\n> We should only use test_must_fail() to test git commands. Replace\n> `test_must_fail` with `!` so that we don't use test_must_fail() on the\n> completion functions.\n> \n> This is done because test_must_fail() is used to except git exiting with\n> an expected error but it will still return an error if it detects\n> unexpected errors such as a segfault. In the case of these completion\n> functions, the return codes of the git commands aren't checked and, most\n> of the time, they will just explicitly return 1 or have an unrelated\n> command return 0. As a result, it doesn't really make sense to use\n> `test_must_fail` so use `!` instead.\n> \n> Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> ---\n>  t/t9902-completion.sh | 12 ++++++------\n>  1 file changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index 5505e5aa24..320c755971 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -294,7 +294,7 @@ test_expect_success '__git_find_repo_path - \"git -C\" while .git directory in par\n>  test_expect_success '__git_find_repo_path - non-existing path in \"git -C\"' '\n>  \t(\n>  \t\t__git_C_args=(-C non-existing) &&\n> -\t\ttest_must_fail __git_find_repo_path &&\n> +\t\t! __git_find_repo_path &&\n>  \t\tprintf \"$__git_repo_path\" >\"$actual\"\n>  \t) &&\n>  \ttest_must_be_empty \"$actual\"\n> @@ -303,7 +303,7 @@ test_expect_success '__git_find_repo_path - non-existing path in \"git -C\"' '\n>  test_expect_success '__git_find_repo_path - non-existing path in $__git_dir' '\n>  \t(\n>  \t\t__git_dir=\"non-existing\" &&\n> -\t\ttest_must_fail __git_find_repo_path &&\n> +\t\t! __git_find_repo_path &&\n>  \t\tprintf \"$__git_repo_path\" >\"$actual\"\n>  \t) &&\n>  \ttest_must_be_empty \"$actual\"\n> @@ -313,7 +313,7 @@ test_expect_success '__git_find_repo_path - non-existing $GIT_DIR' '\n>  \t(\n>  \t\tGIT_DIR=\"$ROOT/non-existing\" &&\n>  \t\texport GIT_DIR &&\n> -\t\ttest_must_fail __git_find_repo_path &&\n> +\t\t! __git_find_repo_path &&\n>  \t\tprintf \"$__git_repo_path\" >\"$actual\"\n>  \t) &&\n>  \ttest_must_be_empty \"$actual\"\n> @@ -362,7 +362,7 @@ test_expect_success '__git_find_repo_path - not a git repository' '\n>  \t\tcd non-repo &&\n>  \t\tGIT_CEILING_DIRECTORIES=\"$ROOT\" &&\n>  \t\texport GIT_CEILING_DIRECTORIES &&\n> -\t\ttest_must_fail __git_find_repo_path &&\n> +\t\t! __git_find_repo_path &&\n>  \t\tprintf \"$__git_repo_path\" >\"$actual\"\n>  \t) &&\n>  \ttest_must_be_empty \"$actual\"\n> @@ -381,7 +381,7 @@ test_expect_success '__gitdir - finds repo' '\n>  test_expect_success '__gitdir - returns error when cannot find repo' '\n>  \t(\n>  \t\t__git_dir=\"non-existing\" &&\n> -\t\ttest_must_fail __gitdir >\"$actual\"\n> +\t\t! __gitdir >\"$actual\"\n>  \t) &&\n>  \ttest_must_be_empty \"$actual\"\n>  '\n> @@ -608,7 +608,7 @@ test_expect_success '__git_is_configured_remote' '\n>  \tgit remote add remote_2 git://remote_2 &&\n>  \t(\n>  \t\tverbose __git_is_configured_remote remote_2 &&\n> -\t\ttest_must_fail __git_is_configured_remote non-existent\n> +\t\t! __git_is_configured_remote non-existent\n>  \t)\n>  '\n>  \n> \n\nI actually think that the use of test_must_fail has some merit in these\ncases, because the shell function __git_find_repo_path runs `git\nrev-parse` behind the scenes, and it runs it in such a way that\ntest_must_fail would do the right thing: the function just dispatches\ninto a handful of simple cases, each basically only with a variable\nassignment, two of them in the form\n\n\tvar=$(git rev-parse ...)\n\nI would suggest to drop this patch.\n\n-- Hannes\n"},{"id":"395887","messageId":"20200422083550.GA12348@generichostname","threadId":"53259","inReplyTo":"60627dc6-fb9c-549f-d76d-0e110c5b0de2@kdbg.org","subject":"Re: [PATCH 8/8] t9902: don't use `test_must_fail __git_*`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-22T08:35:50Z","receivedAt":"2020-04-22T08:35:56Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Hannes,\n\nOn Tue, Apr 21, 2020 at 11:16:09PM +0200, Johannes Sixt wrote:\n> Am 20.04.20 um 10:54 schrieb Denton Liu:\n> > We should only use test_must_fail() to test git commands. Replace\n> > `test_must_fail` with `!` so that we don't use test_must_fail() on the\n> > completion functions.\n> > \n> > This is done because test_must_fail() is used to except git exiting with\n> > an expected error but it will still return an error if it detects\n> > unexpected errors such as a segfault. In the case of these completion\n> > functions, the return codes of the git commands aren't checked and, most\n> > of the time, they will just explicitly return 1 or have an unrelated\n> > command return 0. As a result, it doesn't really make sense to use\n> > `test_must_fail` so use `!` instead.\n> > \n> > Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> > ---\n> >  t/t9902-completion.sh | 12 ++++++------\n> >  1 file changed, 6 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> > index 5505e5aa24..320c755971 100755\n> > --- a/t/t9902-completion.sh\n> > +++ b/t/t9902-completion.sh\n> > @@ -294,7 +294,7 @@ test_expect_success '__git_find_repo_path - \"git -C\" while .git directory in par\n> >  test_expect_success '__git_find_repo_path - non-existing path in \"git -C\"' '\n> >  \t(\n> >  \t\t__git_C_args=(-C non-existing) &&\n> > -\t\ttest_must_fail __git_find_repo_path &&\n> > +\t\t! __git_find_repo_path &&\n> >  \t\tprintf \"$__git_repo_path\" >\"$actual\"\n> >  \t) &&\n> >  \ttest_must_be_empty \"$actual\"\n> > @@ -303,7 +303,7 @@ test_expect_success '__git_find_repo_path - non-existing path in \"git -C\"' '\n> >  test_expect_success '__git_find_repo_path - non-existing path in $__git_dir' '\n> >  \t(\n> >  \t\t__git_dir=\"non-existing\" &&\n> > -\t\ttest_must_fail __git_find_repo_path &&\n> > +\t\t! __git_find_repo_path &&\n> >  \t\tprintf \"$__git_repo_path\" >\"$actual\"\n> >  \t) &&\n> >  \ttest_must_be_empty \"$actual\"\n> > @@ -313,7 +313,7 @@ test_expect_success '__git_find_repo_path - non-existing $GIT_DIR' '\n> >  \t(\n> >  \t\tGIT_DIR=\"$ROOT/non-existing\" &&\n> >  \t\texport GIT_DIR &&\n> > -\t\ttest_must_fail __git_find_repo_path &&\n> > +\t\t! __git_find_repo_path &&\n> >  \t\tprintf \"$__git_repo_path\" >\"$actual\"\n> >  \t) &&\n> >  \ttest_must_be_empty \"$actual\"\n> > @@ -362,7 +362,7 @@ test_expect_success '__git_find_repo_path - not a git repository' '\n> >  \t\tcd non-repo &&\n> >  \t\tGIT_CEILING_DIRECTORIES=\"$ROOT\" &&\n> >  \t\texport GIT_CEILING_DIRECTORIES &&\n> > -\t\ttest_must_fail __git_find_repo_path &&\n> > +\t\t! __git_find_repo_path &&\n> >  \t\tprintf \"$__git_repo_path\" >\"$actual\"\n> >  \t) &&\n> >  \ttest_must_be_empty \"$actual\"\n> > @@ -381,7 +381,7 @@ test_expect_success '__gitdir - finds repo' '\n> >  test_expect_success '__gitdir - returns error when cannot find repo' '\n> >  \t(\n> >  \t\t__git_dir=\"non-existing\" &&\n> > -\t\ttest_must_fail __gitdir >\"$actual\"\n> > +\t\t! __gitdir >\"$actual\"\n> >  \t) &&\n> >  \ttest_must_be_empty \"$actual\"\n> >  '\n> > @@ -608,7 +608,7 @@ test_expect_success '__git_is_configured_remote' '\n> >  \tgit remote add remote_2 git://remote_2 &&\n> >  \t(\n> >  \t\tverbose __git_is_configured_remote remote_2 &&\n> > -\t\ttest_must_fail __git_is_configured_remote non-existent\n> > +\t\t! __git_is_configured_remote non-existent\n> >  \t)\n> >  '\n> >  \n> > \n> \n> I actually think that the use of test_must_fail has some merit in these\n> cases, because the shell function __git_find_repo_path runs `git\n> rev-parse` behind the scenes, and it runs it in such a way that\n> test_must_fail would do the right thing: the function just dispatches\n> into a handful of simple cases, each basically only with a variable\n> assignment, two of them in the form\n> \n> \tvar=$(git rev-parse ...)\n> \n> I would suggest to drop this patch.\n\nThanks for the analysis. I agree with you. I think it's good to avoid\nusing test_must_fail unnecessarily but it wouldn't hurt to err on the\nside of caution when we're potentially wrapping a git command (like in\nthis case).\n\nIn a future patch where I disable test_must_fail except for approved\ncommands, I'll add __git* commands to the whitelist.\n\nThanks,\n\nDenton\n\n> -- Hannes\n"},{"id":"395889","messageId":"20200422085424.GA25366@generichostname","threadId":"53259","inReplyTo":"xmqqa734muw7.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-22T08:54:24Z","receivedAt":"2020-04-22T08:54:31Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Tue, Apr 21, 2020 at 01:44:08PM -0700, Junio C Hamano wrote:\n> Denton Liu <liu.denton@gmail.com> writes:\n> \n> > On Mon, Apr 20, 2020 at 01:09:49PM -0700, Junio C Hamano wrote:\n> >> Eric Sunshine <sunshine@sunshineco.com> writes:\n> >> \n> >> > On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote:\n> >> >> The test_must_fail function should only be used for git commands since\n> >> >> we 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 <liu.denton@gmail.com>\n> >> >> ---\n> >> >> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh\n> >> >> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' '\n> >> >> -               test_must_fail svn_cmd commit -m \"this commit should fail\" &&\n> >> >> +               ! svn_cmd commit -m \"this commit should fail\" &&\n> >> >\n> >> > Hmm, this doesn't look like 'test_cmp' mentioned in the commit message.\n> >> \n> >> Yeah, the other hunk is about test_cmp and this hunk is about\n> >> svn_cmd.  The stated rationale applies to both wrappers, I think.\n> >> \n> >>     Subject: [PATCH 6/8] t9164: use test_must_fail only on git\n> >> \n> >>     The `test_must_fail` function should only be used for git commands;\n> >>     we are not in the business of catching segmentation fault by external\n> >>     commands.  Shell helper functions test_cmp and svn_cmd used in this\n> >>     script are wrappers around external commands, so just use `! cmd`\n> >>     instead of `test_must_fail cmd`\n> >> \n> >> perhaps, without any change to the code?\n> >\n> > Thanks, this looks good to me too.\n> \n> Thanks.  \n> \n> So the 4-patch test-must-fail-fix series is now complete?  Whee.\n\nHannes suggested that we should drop the tip commit of this series[1]\nand I tend to agree with him. Aside from that, though, the series is\nready to go.\n\n(I could improve 3/8 as suggested here[2] but I'll throw it in the next\nseries instead since I'm trying to get into the habit of not adding in\nunrelated patches.)\n\n[1]: https://lore.kernel.org/git/6cfa2c447e1196d6c4325aff9fac52434d10fda8.1587372771.git.liu.denton@gmail.com/\n[2]: https://lore.kernel.org/git/90faeb7a-1c6a-3fc6-6410-5e264c9340e8@kdbg.org/\n"},{"id":"395907","messageId":"xmqqsggvjz9v.fsf@gitster.c.googlers.com","threadId":"53259","inReplyTo":"20200422085424.GA25366@generichostname","subject":"Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-22T15:50:04Z","receivedAt":"2020-04-22T15:50:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> Hannes suggested that we should drop the tip commit of this series[1]\n> and I tend to agree with him. Aside from that, though, the series is\n> ready to go.\n>\n> (I could improve 3/8 as suggested here[2] but I'll throw it in the next\n> series instead since I'm trying to get into the habit of not adding in\n> unrelated patches.)\n>\n> [1]: https://lore.kernel.org/git/6cfa2c447e1196d6c4325aff9fac52434d10fda8.1587372771.git.liu.denton@gmail.com/\n> [2]: https://lore.kernel.org/git/90faeb7a-1c6a-3fc6-6410-5e264c9340e8@kdbg.org/\n\nI agree with you two that test_must_fail may make sense with these\n__git_foo helpers used in the completion.  It is a bit of shame that\nthere is no opportunity to leave the reasoning behind the decision\nfor later developers, other than in the discussion thread.\n\nI agree that 3/8 is good as-is in this series.\n\nThanks.\n"}]}