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

> 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.