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

From: Denton Liu <[email-masked]>
Date: Wed, 22 Apr 2020 04:54:24 -0400

Hi Junio,

On Tue, Apr 21, 2020 at 01:44:08PM -0700, Junio C Hamano wrote:
> Denton Liu <[email-masked]> writes:
> 
> > On Mon, Apr 20, 2020 at 01:09:49PM -0700, Junio C Hamano wrote:
> >> Eric Sunshine <[email-masked]> writes:
> >> 
> >> > On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <[email-masked]> 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 <[email-masked]>
> >> >> ---
> >> >> 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.
> 
> Thanks.  
> 
> So the 4-patch test-must-fail-fix series is now complete?  Whee.

Hannes suggested that we should drop the tip commit of this series[1]
and I tend to agree with him. Aside from that, though, the series is
ready to go.

(I could improve 3/8 as suggested here[2] but I'll throw it in the next
series instead since I'm trying to get into the habit of not adding in
unrelated patches.)

[1]: https://lore.kernel.org/git/6c[email-masked]/
[2]: https://lore.kernel.org/git/[email-masked]/