Re: [PATCH 8/8] t9902: don't use `test_must_fail __git_*`
Hi Hannes,
On Tue, Apr 21, 2020 at 11:16:09PM +0200, Johannes Sixt wrote:
Show 88 quoted lines
> 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
Denton Liu
