Re: [PATCH] t7406: Make a test_must_fail call fail for the right reason

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 3, 2018, 23:41 UTC
Message-ID: <xmqqd0uzuifd.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180803231407.10662-1-newren@gmail.com>

Elijah Newren <newren@gmail.com> writes:

Show 19 quoted lines

> A test making use of test_must_fail was failing like this:
>   fatal: ambiguous argument '|': unknown revision or path not in the working tree.
> when the intent was to verify that a specific string was not found
> in the output of the git diff command, i.e. that grep returned
> non-zero.  Fix the test to do that.
> ---
>  t/t7406-submodule-update.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index f604ef7a72..7be8b59569 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -599,7 +599,7 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
>  	 ) &&
>  	 git diff --raw | grep "	submodule" &&
>  	 git submodule update --checkout &&
> -	 test_must_fail git diff --raw \| grep "	submodule" &&
> +	 git diff --raw | test_must_fail grep "	submodule" &&

Good spotting, but a few comments.

 * I've seen "do not have 'git' upstream on a pipee (it would hide
   the exit status from an unexpected failure of 'git') recently.
   We probably want to do the same.

 * We do not use test_must_fail for non-git things, as we are not in
   the business of protecting us from unexpected segfault of system
   binaries like grep.

So an immediate improvement for this line would be

	! git diff --raw | grep " submodule" &&

and longer-term clean-up would aim for

	git diff --raw >differs &&
	! grep " submodule" &&

or something like that. I suspect that --raw may want to be updated to --name-only or something, as I do not see the tests using the object names hence no strong need for using --raw format.