[PATCH 0/8] t: replace incorrect test_must_fail usage (part 4)

[PATCH 0/8] t: replace incorrect test_must_fail usage (part 4)

[Denton Liu]

The overall scope of these patches is to replace inappropriate uses of
test_must_fail. IOW, we should only allow test_must_fail to run on `git`
and `test-tool`. Ultimately, we will conclude by making test_must_fail
error out on non-git commands. An advance view of the final series can
be found here[1].

This is the fourth part. It focuses on t[6-9]*.sh.

The first part can be found here[2]. The second part can be found
here[3]. The third part can be found here[4].

[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2
[2]: https://lore.kernel.org/git/cover.1576583819.git.liu.denton@gmail.com/
[3]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/
[4]: https://lore.kernel.org/git/cover.1585209554.git.liu.denton@gmail.com/

Denton Liu (8):
  t6030: use test_path_is_missing()
  t7408: replace incorrect uses of test_must_fail
  t7508: don't use `test_must_fail test_cmp`
  t9141: use test_path_is_missing()
  t9160: use test_path_is_missing()
  t9164: don't use `test_must_fail test_cmp`
  t9819: don't use test_must_fail with p4
  t9902: don't use `test_must_fail __git_*`

 t/t6030-bisect-porcelain.sh            |  8 ++++----
 t/t7408-submodule-reference.sh         |  8 ++++----
 t/t7508-status.sh                      |  2 +-
 t/t9141-git-svn-multiple-branches.sh   |  8 ++++----
 t/t9160-git-svn-preserve-empty-dirs.sh |  4 ++--
 t/t9164-git-svn-dcommit-concurrent.sh  |  4 ++--
 t/t9819-git-p4-case-folding.sh         |  2 +-
 t/t9902-completion.sh                  | 12 ++++++------
 8 files changed, 24 insertions(+), 24 deletions(-)

-- 
2.26.0.159.g23e2136ad0

[PATCH 1/8] t6030: use test_path_is_missing()

[Denton Liu]

The test_must_fail() function should only be used for git commands since
we should assume that external commands work sanely. Replace
`test_must_fail test -e` with `test_path_is_missing`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t6030-bisect-porcelain.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 821a0c88cf..1313142564 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -148,7 +148,7 @@ test_expect_success 'bisect start: no ".git/BISECT_START" created if junk rev' '
 	test_must_fail git bisect start $HASH4 foo -- &&
 	git branch > branch.output &&
 	grep "* other" branch.output > /dev/null &&
-	test_must_fail test -e .git/BISECT_START
+	test_path_is_missing .git/BISECT_START
 '
 
 test_expect_success 'bisect start: existing ".git/BISECT_START" not modified if junk rev' '
@@ -166,7 +166,7 @@ test_expect_success 'bisect start: no ".git/BISECT_START" if mistaken rev' '
 	test_must_fail git bisect start $HASH1 $HASH4 -- &&
 	git branch > branch.output &&
 	grep "* other" branch.output > /dev/null &&
-	test_must_fail test -e .git/BISECT_START
+	test_path_is_missing .git/BISECT_START
 '
 
 test_expect_success 'bisect start: no ".git/BISECT_START" if checkout error' '
@@ -175,7 +175,7 @@ test_expect_success 'bisect start: no ".git/BISECT_START" if checkout error' '
 	git branch &&
 	git branch > branch.output &&
 	grep "* other" branch.output > /dev/null &&
-	test_must_fail test -e .git/BISECT_START &&
+	test_path_is_missing .git/BISECT_START &&
 	test -z "$(git for-each-ref "refs/bisect/*")" &&
 	git checkout HEAD hello
 '
@@ -485,7 +485,7 @@ test_expect_success 'optimized merge base checks' '
 	git bisect bad &&
 	git bisect good "$A_HASH" > my_bisect_log4.txt &&
 	test_i18ngrep "merge base must be tested" my_bisect_log4.txt &&
-	test_must_fail test -f ".git/BISECT_ANCESTORS_OK"
+	test_path_is_missing ".git/BISECT_ANCESTORS_OK"
 '
 
 # This creates another side branch called "parallel" with some files
-- 
2.26.0.159.g23e2136ad0

[PATCH 2/8] t7408: replace incorrect uses of test_must_fail

[Denton Liu]

According to t/README, test_must_fail() should only be used to test for
failure in Git commands.

Replace the invocation of `test_must_fail test_path_is_file` with
`test_path_is_missing` since, in this test case, the path should not
exist at all.

In all the cases where `test_must_fail test_alternate_is_used` appears,
test_alternate_is_used() fails because test_line_count() cannot open the
non-existent $alternates_file. Replace
`test_must_fail test_alternate_is_used` with `test_path_is_missing` to
test for this directly.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t7408-submodule-reference.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 34ac28c056..a3892f494b 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -122,8 +122,8 @@ test_expect_success 'missing submodule alternate fails clone and submodule updat
 		# update of the submodule succeeds
 		test_must_fail git submodule update --init &&
 		# and we have no alternates:
-		test_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&
-		test_must_fail test_path_is_file sub/file1
+		test_path_is_missing .git/modules/sub/objects/info/alternates &&
+		test_path_is_missing sub/file1
 	)
 '
 
@@ -137,7 +137,7 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm
 		# update of the submodule succeeds
 		git submodule update --init &&
 		# and we have no alternates:
-		test_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&
+		test_path_is_missing .git/modules/sub/objects/info/alternates &&
 		test_path_is_file sub/file1
 	)
 '
@@ -182,7 +182,7 @@ check_that_two_of_three_alternates_are_used() {
 	# immediate submodule has alternate:
 	test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub &&
 	# but nested submodule has no alternate:
-	test_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
+	test_path_is_missing .git/modules/subwithsub/modules/sub/objects/info/alternates
 }
 
 
-- 
2.26.0.159.g23e2136ad0

[PATCH 3/8] t7508: don't use `test_must_fail test_cmp`

[Denton Liu]

The test_must_fail function should only be used for git commands since
we assume that external commands work sanely. Since test_cmp() just
wraps an external command, replace `test_must_fail test_cmp` with
`! test_cmp`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t7508-status.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 482ce3510e..8e969f3e36 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1471,7 +1471,7 @@ test_expect_success '"status.branch=true" same as "-b"' '
 test_expect_success '"status.branch=true" different from "--no-branch"' '
 	git status -s --no-branch  >expected_nobranch &&
 	git -c status.branch=true status -s >actual &&
-	test_must_fail test_cmp expected_nobranch actual
+	! test_cmp expected_nobranch actual
 '
 
 test_expect_success '"status.branch=true" weaker than "--no-branch"' '
-- 
2.26.0.159.g23e2136ad0

[PATCH 4/8] t9141: use test_path_is_missing()

[Denton Liu]

The test_must_fail() function should only be used for git commands since
we assume that external commands work sanely. Since, not only should
these directories not exist, but there shouldn't exist _any_ filesystem
entity in these paths, replace `test_must_fail test -d` with
`test_path_is_missing`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t9141-git-svn-multiple-branches.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t9141-git-svn-multiple-branches.sh b/t/t9141-git-svn-multiple-branches.sh
index 8e7f7d68b7..bf168a3645 100755
--- a/t/t9141-git-svn-multiple-branches.sh
+++ b/t/t9141-git-svn-multiple-branches.sh
@@ -90,10 +90,10 @@ test_expect_success 'Multiple branch or tag paths require -d' '
 	) &&
 	( cd svn_project &&
 		svn_cmd up &&
-		test_must_fail test -d b_one/Nope &&
-		test_must_fail test -d b_two/Nope &&
-		test_must_fail test -d tags_A/Tagless &&
-		test_must_fail test -d tags_B/Tagless
+		test_path_is_missing b_one/Nope &&
+		test_path_is_missing b_two/Nope &&
+		test_path_is_missing tags_A/Tagless &&
+		test_path_is_missing tags_B/Tagless
 	)
 '
 
-- 
2.26.0.159.g23e2136ad0

[PATCH 5/8] t9160: use test_path_is_missing()

[Denton Liu]

The test_must_fail() function should only be used for git commands since
we assume that external commands work sanely. Since, not only should
this file not exist, but there shouldn't exit _any_ filesystem entity in
these paths, replace `test_must_fail test -f` with
`test_path_is_missing`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t9160-git-svn-preserve-empty-dirs.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh
index 0ede3cfedb..36c6b1a12f 100755
--- a/t/t9160-git-svn-preserve-empty-dirs.sh
+++ b/t/t9160-git-svn-preserve-empty-dirs.sh
@@ -86,8 +86,8 @@ test_expect_success 'remove non-last entry from directory' '
 		cd "$GIT_REPO" &&
 		git checkout HEAD~2
 	) &&
-	test_must_fail test -f "$GIT_REPO"/2/.gitignore &&
-	test_must_fail test -f "$GIT_REPO"/3/.gitignore
+	test_path_is_missing "$GIT_REPO"/2/.gitignore &&
+	test_path_is_missing "$GIT_REPO"/3/.gitignore
 '
 
 # After re-cloning the repository with --placeholder-file specified, there
-- 
2.26.0.159.g23e2136ad0

[PATCH 6/8] t9164: don't use `test_must_fail test_cmp`

[Denton Liu]

The test_must_fail function should only be used for git commands since
we assume that external commands work sanely. Since test_cmp() just
wraps an external command, replace `test_must_fail test_cmp` with
`! test_cmp`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t9164-git-svn-dcommit-concurrent.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index 90346ff4e9..8466269bf5 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -92,7 +92,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '
 		echo 1 >> file &&
 		svn_cmd commit -m "changing file" &&
 		svn_cmd up &&
-		test_must_fail test_cmp auto_updated_file au_file_saved
+		! test_cmp auto_updated_file au_file_saved
 	)
 '
 
@@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' '
 		echo 2 >> file &&
 		svn_cmd commit -m "changing file once again" &&
 		echo 3 >> file &&
-		test_must_fail svn_cmd commit -m "this commit should fail" &&
+		! svn_cmd commit -m "this commit should fail" &&
 		svn_cmd revert file
 	)
 '
-- 
2.26.0.159.g23e2136ad0

[PATCH 7/8] t9819: don't use test_must_fail with p4

[Denton Liu]

We were using `test_must_fail p4` to test that the p4 command failed as
expected. However, test_must_fail() is used to ensure that commands fail
in an expected way, not due to something like a segv. Since we are not
in the business of verifying the sanity of the external world, replace
`test_must_fail p4` with `! p4` and assume that the `p4` command does
not die unexpectedly.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t9819-git-p4-case-folding.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh
index 600ce1e0b0..b4d93f0c17 100755
--- a/t/t9819-git-p4-case-folding.sh
+++ b/t/t9819-git-p4-case-folding.sh
@@ -30,7 +30,7 @@ test_expect_success 'Check p4 is in case-folding mode' '
 		cd "$cli" &&
 		>lc/FILE.TXT &&
 		p4 add lc/FILE.TXT &&
-		test_must_fail p4 submit -d "Cannot add file differing only in case" lc/FILE.TXT
+		! p4 submit -d "Cannot add file differing only in case" lc/FILE.TXT
 	)
 '
 
-- 
2.26.0.159.g23e2136ad0

[PATCH 8/8] t9902: don't use `test_must_fail __git_*`

[Denton Liu]

We should only use test_must_fail() to test git commands. Replace
`test_must_fail` with `!` so that we don't use test_must_fail() on the
completion functions.

This is done because test_must_fail() is used to except git exiting with
an expected error but it will still return an error if it detects
unexpected errors such as a segfault. In the case of these completion
functions, the return codes of the git commands aren't checked and, most
of the time, they will just explicitly return 1 or have an unrelated
command return 0. As a result, it doesn't really make sense to use
`test_must_fail` so use `!` instead.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t9902-completion.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 5505e5aa24..320c755971 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -294,7 +294,7 @@ test_expect_success '__git_find_repo_path - "git -C" while .git directory in par
 test_expect_success '__git_find_repo_path - non-existing path in "git -C"' '
 	(
 		__git_C_args=(-C non-existing) &&
-		test_must_fail __git_find_repo_path &&
+		! __git_find_repo_path &&
 		printf "$__git_repo_path" >"$actual"
 	) &&
 	test_must_be_empty "$actual"
@@ -303,7 +303,7 @@ test_expect_success '__git_find_repo_path - non-existing path in "git -C"' '
 test_expect_success '__git_find_repo_path - non-existing path in $__git_dir' '
 	(
 		__git_dir="non-existing" &&
-		test_must_fail __git_find_repo_path &&
+		! __git_find_repo_path &&
 		printf "$__git_repo_path" >"$actual"
 	) &&
 	test_must_be_empty "$actual"
@@ -313,7 +313,7 @@ test_expect_success '__git_find_repo_path - non-existing $GIT_DIR' '
 	(
 		GIT_DIR="$ROOT/non-existing" &&
 		export GIT_DIR &&
-		test_must_fail __git_find_repo_path &&
+		! __git_find_repo_path &&
 		printf "$__git_repo_path" >"$actual"
 	) &&
 	test_must_be_empty "$actual"
@@ -362,7 +362,7 @@ test_expect_success '__git_find_repo_path - not a git repository' '
 		cd non-repo &&
 		GIT_CEILING_DIRECTORIES="$ROOT" &&
 		export GIT_CEILING_DIRECTORIES &&
-		test_must_fail __git_find_repo_path &&
+		! __git_find_repo_path &&
 		printf "$__git_repo_path" >"$actual"
 	) &&
 	test_must_be_empty "$actual"
@@ -381,7 +381,7 @@ test_expect_success '__gitdir - finds repo' '
 test_expect_success '__gitdir - returns error when cannot find repo' '
 	(
 		__git_dir="non-existing" &&
-		test_must_fail __gitdir >"$actual"
+		! __gitdir >"$actual"
 	) &&
 	test_must_be_empty "$actual"
 '
@@ -608,7 +608,7 @@ test_expect_success '__git_is_configured_remote' '
 	git remote add remote_2 git://remote_2 &&
 	(
 		verbose __git_is_configured_remote remote_2 &&
-		test_must_fail __git_is_configured_remote non-existent
+		! __git_is_configured_remote non-existent
 	)
 '
 
-- 
2.26.0.159.g23e2136ad0

Re: [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4)

[Derrick Stolee]

On 4/20/2020 4:54 AM, Denton Liu wrote:
> The overall scope of these patches is to replace inappropriate uses of
> test_must_fail. IOW, we should only allow test_must_fail to run on `git`
> and `test-tool`. Ultimately, we will conclude by making test_must_fail
> error out on non-git commands. An advance view of the final series can
> be found here[1].

I appreciate your efforts here to use best-practices throughout the test
suite. Too often I look at a test script as an example on which to base
a new test, but then I just repeat a bad pattern.

This series looks good to me.

Thanks,
-Stolee

Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`

[Eric Sunshine]

On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote:
> The test_must_fail function should only be used for git commands since
> we assume that external commands work sanely. Since test_cmp() just
> wraps an external command, replace `test_must_fail test_cmp` with
> `! test_cmp`.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' '
> -               test_must_fail svn_cmd commit -m "this commit should fail" &&
> +               ! svn_cmd commit -m "this commit should fail" &&

Hmm, this doesn't look like 'test_cmp' mentioned in the commit message.

Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote:
>> The test_must_fail function should only be used for git commands since
>> we assume that external commands work sanely. Since test_cmp() just
>> wraps an external command, replace `test_must_fail test_cmp` with
>> `! test_cmp`.
>>
>> Signed-off-by: Denton Liu <liu.denton@gmail.com>
>> ---
>> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
>> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' '
>> -               test_must_fail svn_cmd commit -m "this commit should fail" &&
>> +               ! svn_cmd commit -m "this commit should fail" &&
>
> Hmm, this doesn't look like 'test_cmp' mentioned in the commit message.

Yeah, the other hunk is about test_cmp and this hunk is about
svn_cmd.  The stated rationale applies to both wrappers, I think.

    Subject: [PATCH 6/8] t9164: use test_must_fail only on git

    The `test_must_fail` function should only be used for git commands;
    we are not in the business of catching segmentation fault by external
    commands.  Shell helper functions test_cmp and svn_cmd used in this
    script are wrappers around external commands, so just use `! cmd`
    instead of `test_must_fail cmd`

perhaps, without any change to the code?

Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`

[Eric Sunshine]

On Mon, Apr 20, 2020 at 4:09 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > Hmm, this doesn't look like 'test_cmp' mentioned in the commit message.
>
> Yeah, the other hunk is about test_cmp and this hunk is about
> svn_cmd.  The stated rationale applies to both wrappers, I think.
>
>     Subject: [PATCH 6/8] t9164: use test_must_fail only on git
>
>     The `test_must_fail` function should only be used for git commands;
>     we are not in the business of catching segmentation fault by external
>     commands.  Shell helper functions test_cmp and svn_cmd used in this
>     script are wrappers around external commands, so just use `! cmd`
>     instead of `test_must_fail cmd`
>
> perhaps, without any change to the code?

That sounds fine.

Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`

[Denton Liu]

On Mon, Apr 20, 2020 at 01:09:49PM -0700, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote:
> >> The test_must_fail function should only be used for git commands since
> >> we assume that external commands work sanely. Since test_cmp() just
> >> wraps an external command, replace `test_must_fail test_cmp` with
> >> `! test_cmp`.
> >>
> >> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> >> ---
> >> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
> >> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' '
> >> -               test_must_fail svn_cmd commit -m "this commit should fail" &&
> >> +               ! svn_cmd commit -m "this commit should fail" &&
> >
> > Hmm, this doesn't look like 'test_cmp' mentioned in the commit message.
> 
> Yeah, the other hunk is about test_cmp and this hunk is about
> svn_cmd.  The stated rationale applies to both wrappers, I think.
> 
>     Subject: [PATCH 6/8] t9164: use test_must_fail only on git
> 
>     The `test_must_fail` function should only be used for git commands;
>     we are not in the business of catching segmentation fault by external
>     commands.  Shell helper functions test_cmp and svn_cmd used in this
>     script are wrappers around external commands, so just use `! cmd`
>     instead of `test_must_fail cmd`
> 
> perhaps, without any change to the code?

Thanks, this looks good to me too.

Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> On Mon, Apr 20, 2020 at 01:09:49PM -0700, Junio C Hamano wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>> 
>> > On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote:
>> >> The test_must_fail function should only be used for git commands since
>> >> we assume that external commands work sanely. Since test_cmp() just
>> >> wraps an external command, replace `test_must_fail test_cmp` with
>> >> `! test_cmp`.
>> >>
>> >> Signed-off-by: Denton Liu <liu.denton@gmail.com>
>> >> ---
>> >> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
>> >> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' '
>> >> -               test_must_fail svn_cmd commit -m "this commit should fail" &&
>> >> +               ! svn_cmd commit -m "this commit should fail" &&
>> >
>> > Hmm, this doesn't look like 'test_cmp' mentioned in the commit message.
>> 
>> Yeah, the other hunk is about test_cmp and this hunk is about
>> svn_cmd.  The stated rationale applies to both wrappers, I think.
>> 
>>     Subject: [PATCH 6/8] t9164: use test_must_fail only on git
>> 
>>     The `test_must_fail` function should only be used for git commands;
>>     we are not in the business of catching segmentation fault by external
>>     commands.  Shell helper functions test_cmp and svn_cmd used in this
>>     script are wrappers around external commands, so just use `! cmd`
>>     instead of `test_must_fail cmd`
>> 
>> perhaps, without any change to the code?
>
> Thanks, this looks good to me too.

Thanks.  

So the 4-patch test-must-fail-fix series is now complete?  Whee.

Re: [PATCH 3/8] t7508: don't use `test_must_fail test_cmp`

[Johannes Sixt]

Am 20.04.20 um 10:54 schrieb Denton Liu:
> --- a/t/t7508-status.sh
> +++ b/t/t7508-status.sh
> @@ -1471,7 +1471,7 @@ test_expect_success '"status.branch=true" same as "-b"' '
>  test_expect_success '"status.branch=true" different from "--no-branch"' '
>  	git status -s --no-branch  >expected_nobranch &&
>  	git -c status.branch=true status -s >actual &&
> -	test_must_fail test_cmp expected_nobranch actual
> +	! test_cmp expected_nobranch actual
>  '

Not your fault, but this is, of course, a very weak test case. Check
that some output that the program generates is _not_ equal to something
else? That condition should be very easy to satisfy.

-- Hannes

Re: [PATCH 8/8] t9902: don't use `test_must_fail __git_*`

[Johannes Sixt]

Am 20.04.20 um 10:54 schrieb Denton Liu:
> We should only use test_must_fail() to test git commands. Replace
> `test_must_fail` with `!` so that we don't use test_must_fail() on the
> completion functions.
> 
> This is done because test_must_fail() is used to except git exiting with
> an expected error but it will still return an error if it detects
> unexpected errors such as a segfault. In the case of these completion
> functions, the return codes of the git commands aren't checked and, most
> of the time, they will just explicitly return 1 or have an unrelated
> command return 0. As a result, it doesn't really make sense to use
> `test_must_fail` so use `!` instead.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t9902-completion.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 5505e5aa24..320c755971 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -294,7 +294,7 @@ test_expect_success '__git_find_repo_path - "git -C" while .git directory in par
>  test_expect_success '__git_find_repo_path - non-existing path in "git -C"' '
>  	(
>  		__git_C_args=(-C non-existing) &&
> -		test_must_fail __git_find_repo_path &&
> +		! __git_find_repo_path &&
>  		printf "$__git_repo_path" >"$actual"
>  	) &&
>  	test_must_be_empty "$actual"
> @@ -303,7 +303,7 @@ test_expect_success '__git_find_repo_path - non-existing path in "git -C"' '
>  test_expect_success '__git_find_repo_path - non-existing path in $__git_dir' '
>  	(
>  		__git_dir="non-existing" &&
> -		test_must_fail __git_find_repo_path &&
> +		! __git_find_repo_path &&
>  		printf "$__git_repo_path" >"$actual"
>  	) &&
>  	test_must_be_empty "$actual"
> @@ -313,7 +313,7 @@ test_expect_success '__git_find_repo_path - non-existing $GIT_DIR' '
>  	(
>  		GIT_DIR="$ROOT/non-existing" &&
>  		export GIT_DIR &&
> -		test_must_fail __git_find_repo_path &&
> +		! __git_find_repo_path &&
>  		printf "$__git_repo_path" >"$actual"
>  	) &&
>  	test_must_be_empty "$actual"
> @@ -362,7 +362,7 @@ test_expect_success '__git_find_repo_path - not a git repository' '
>  		cd non-repo &&
>  		GIT_CEILING_DIRECTORIES="$ROOT" &&
>  		export GIT_CEILING_DIRECTORIES &&
> -		test_must_fail __git_find_repo_path &&
> +		! __git_find_repo_path &&
>  		printf "$__git_repo_path" >"$actual"
>  	) &&
>  	test_must_be_empty "$actual"
> @@ -381,7 +381,7 @@ test_expect_success '__gitdir - finds repo' '
>  test_expect_success '__gitdir - returns error when cannot find repo' '
>  	(
>  		__git_dir="non-existing" &&
> -		test_must_fail __gitdir >"$actual"
> +		! __gitdir >"$actual"
>  	) &&
>  	test_must_be_empty "$actual"
>  '
> @@ -608,7 +608,7 @@ test_expect_success '__git_is_configured_remote' '
>  	git remote add remote_2 git://remote_2 &&
>  	(
>  		verbose __git_is_configured_remote remote_2 &&
> -		test_must_fail __git_is_configured_remote non-existent
> +		! __git_is_configured_remote non-existent
>  	)
>  '
>  
> 

I actually think that the use of test_must_fail has some merit in these
cases, because the shell function __git_find_repo_path runs `git
rev-parse` behind the scenes, and it runs it in such a way that
test_must_fail would do the right thing: the function just dispatches
into a handful of simple cases, each basically only with a variable
assignment, two of them in the form

	var=$(git rev-parse ...)

I would suggest to drop this patch.

-- Hannes

Re: [PATCH 8/8] t9902: don't use `test_must_fail __git_*`

[Denton Liu]

Hi Hannes,

On Tue, Apr 21, 2020 at 11:16:09PM +0200, Johannes Sixt wrote:
> Am 20.04.20 um 10:54 schrieb Denton Liu:
> > We should only use test_must_fail() to test git commands. Replace
> > `test_must_fail` with `!` so that we don't use test_must_fail() on the
> > completion functions.
> > 
> > This is done because test_must_fail() is used to except git exiting with
> > an expected error but it will still return an error if it detects
> > unexpected errors such as a segfault. In the case of these completion
> > functions, the return codes of the git commands aren't checked and, most
> > of the time, they will just explicitly return 1 or have an unrelated
> > command return 0. As a result, it doesn't really make sense to use
> > `test_must_fail` so use `!` instead.
> > 
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> >  t/t9902-completion.sh | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > index 5505e5aa24..320c755971 100755
> > --- a/t/t9902-completion.sh
> > +++ b/t/t9902-completion.sh
> > @@ -294,7 +294,7 @@ test_expect_success '__git_find_repo_path - "git -C" while .git directory in par
> >  test_expect_success '__git_find_repo_path - non-existing path in "git -C"' '
> >  	(
> >  		__git_C_args=(-C non-existing) &&
> > -		test_must_fail __git_find_repo_path &&
> > +		! __git_find_repo_path &&
> >  		printf "$__git_repo_path" >"$actual"
> >  	) &&
> >  	test_must_be_empty "$actual"
> > @@ -303,7 +303,7 @@ test_expect_success '__git_find_repo_path - non-existing path in "git -C"' '
> >  test_expect_success '__git_find_repo_path - non-existing path in $__git_dir' '
> >  	(
> >  		__git_dir="non-existing" &&
> > -		test_must_fail __git_find_repo_path &&
> > +		! __git_find_repo_path &&
> >  		printf "$__git_repo_path" >"$actual"
> >  	) &&
> >  	test_must_be_empty "$actual"
> > @@ -313,7 +313,7 @@ test_expect_success '__git_find_repo_path - non-existing $GIT_DIR' '
> >  	(
> >  		GIT_DIR="$ROOT/non-existing" &&
> >  		export GIT_DIR &&
> > -		test_must_fail __git_find_repo_path &&
> > +		! __git_find_repo_path &&
> >  		printf "$__git_repo_path" >"$actual"
> >  	) &&
> >  	test_must_be_empty "$actual"
> > @@ -362,7 +362,7 @@ test_expect_success '__git_find_repo_path - not a git repository' '
> >  		cd non-repo &&
> >  		GIT_CEILING_DIRECTORIES="$ROOT" &&
> >  		export GIT_CEILING_DIRECTORIES &&
> > -		test_must_fail __git_find_repo_path &&
> > +		! __git_find_repo_path &&
> >  		printf "$__git_repo_path" >"$actual"
> >  	) &&
> >  	test_must_be_empty "$actual"
> > @@ -381,7 +381,7 @@ test_expect_success '__gitdir - finds repo' '
> >  test_expect_success '__gitdir - returns error when cannot find repo' '
> >  	(
> >  		__git_dir="non-existing" &&
> > -		test_must_fail __gitdir >"$actual"
> > +		! __gitdir >"$actual"
> >  	) &&
> >  	test_must_be_empty "$actual"
> >  '
> > @@ -608,7 +608,7 @@ test_expect_success '__git_is_configured_remote' '
> >  	git remote add remote_2 git://remote_2 &&
> >  	(
> >  		verbose __git_is_configured_remote remote_2 &&
> > -		test_must_fail __git_is_configured_remote non-existent
> > +		! __git_is_configured_remote non-existent
> >  	)
> >  '
> >  
> > 
> 
> I actually think that the use of test_must_fail has some merit in these
> cases, because the shell function __git_find_repo_path runs `git
> rev-parse` behind the scenes, and it runs it in such a way that
> test_must_fail would do the right thing: the function just dispatches
> into a handful of simple cases, each basically only with a variable
> assignment, two of them in the form
> 
> 	var=$(git rev-parse ...)
> 
> I would suggest to drop this patch.

Thanks for the analysis. I agree with you. I think it's good to avoid
using test_must_fail unnecessarily but it wouldn't hurt to err on the
side of caution when we're potentially wrapping a git command (like in
this case).

In a future patch where I disable test_must_fail except for approved
commands, I'll add __git* commands to the whitelist.

Thanks,

Denton

> -- Hannes

Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`

[Denton Liu]

Hi Junio,

On Tue, Apr 21, 2020 at 01:44:08PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > On Mon, Apr 20, 2020 at 01:09:49PM -0700, Junio C Hamano wrote:
> >> Eric Sunshine <sunshine@sunshineco.com> writes:
> >> 
> >> > On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote:
> >> >> The test_must_fail function should only be used for git commands since
> >> >> we assume that external commands work sanely. Since test_cmp() just
> >> >> wraps an external command, replace `test_must_fail test_cmp` with
> >> >> `! test_cmp`.
> >> >>
> >> >> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> >> >> ---
> >> >> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
> >> >> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' '
> >> >> -               test_must_fail svn_cmd commit -m "this commit should fail" &&
> >> >> +               ! svn_cmd commit -m "this commit should fail" &&
> >> >
> >> > Hmm, this doesn't look like 'test_cmp' mentioned in the commit message.
> >> 
> >> Yeah, the other hunk is about test_cmp and this hunk is about
> >> svn_cmd.  The stated rationale applies to both wrappers, I think.
> >> 
> >>     Subject: [PATCH 6/8] t9164: use test_must_fail only on git
> >> 
> >>     The `test_must_fail` function should only be used for git commands;
> >>     we are not in the business of catching segmentation fault by external
> >>     commands.  Shell helper functions test_cmp and svn_cmd used in this
> >>     script are wrappers around external commands, so just use `! cmd`
> >>     instead of `test_must_fail cmd`
> >> 
> >> perhaps, without any change to the code?
> >
> > Thanks, this looks good to me too.
> 
> Thanks.  
> 
> So the 4-patch test-must-fail-fix series is now complete?  Whee.

Hannes suggested that we should drop the tip commit of this series[1]
and I tend to agree with him. Aside from that, though, the series is
ready to go.

(I could improve 3/8 as suggested here[2] but I'll throw it in the next
series instead since I'm trying to get into the habit of not adding in
unrelated patches.)

[1]: https://lore.kernel.org/git/6cfa2c447e1196d6c4325aff9fac52434d10fda8.1587372771.git.liu.denton@gmail.com/
[2]: https://lore.kernel.org/git/90faeb7a-1c6a-3fc6-6410-5e264c9340e8@kdbg.org/

Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> Hannes suggested that we should drop the tip commit of this series[1]
> and I tend to agree with him. Aside from that, though, the series is
> ready to go.
>
> (I could improve 3/8 as suggested here[2] but I'll throw it in the next
> series instead since I'm trying to get into the habit of not adding in
> unrelated patches.)
>
> [1]: https://lore.kernel.org/git/6cfa2c447e1196d6c4325aff9fac52434d10fda8.1587372771.git.liu.denton@gmail.com/
> [2]: https://lore.kernel.org/git/90faeb7a-1c6a-3fc6-6410-5e264c9340e8@kdbg.org/

I agree with you two that test_must_fail may make sense with these
__git_foo helpers used in the completion.  It is a bit of shame that
there is no opportunity to leave the reasoning behind the decision
for later developers, other than in the discussion thread.

I agree that 3/8 is good as-is in this series.

Thanks.