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:

quoted

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 <redacted>
---
 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