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

[Denton Liu <liu.denton@gmail.com>]

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