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

From: Johannes Sixt <j6t@kdbg.org>
To: Denton Liu <liu.denton@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 8/8] t9902: don't use `test_must_fail __git_*`
Date: Tue, 21 Apr 2020 23:16:09 +0200	[thread overview]
Message-ID: <60627dc6-fb9c-549f-d76d-0e110c5b0de2@kdbg.org> (raw)
In-Reply-To: <6cfa2c447e1196d6c4325aff9fac52434d10fda8.1587372771.git.liu.denton@gmail.com>

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