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

From: Denton Liu <liu.denton@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Eric Sunshine <sunshine@sunshineco.com>,
	Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`
Date: Tue, 21 Apr 2020 16:16:27 -0400	[thread overview]
Message-ID: <20200421201627.GA9357@generichostname> (raw)
In-Reply-To: <xmqqr1whrkaa.fsf@gitster.c.googlers.com>

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.