Re: [PATCH] t/t1467-refs-rename: Use test_must_fail in place of NOT operator

[Junio C Hamano <gitster@pobox.com>]

Hardik Kumar <hardikxk@gmail.com> writes:

> All tests in `t/t1467-refs-rename.sh` use test_must_fail for verifying
> when a test or command is expected to fail. `test_must_fail` is used for
> all Git commands.

"all Git commands" that are expected to fail, right?

For two "git" commands that appear immediately before the line you
touched in the patch, test_must_fail is not used ;-)

> The use of the NOT operator is replaced with `test_must_fail` to ensure
> consistency in the unit tests without any functional changes to the
> tests themselves.

A more common way for our proposed commit log message phrase the
above is

	Replace the "not" (!) operator with `test_must_fail` to ...

More importantly, the use of test_must_fail is *NOT* about
consistency.  The most important reason why it is the better way to
write than "! git refs rename ..." is that test_must_fail notices
and fails loudly if the command line getting tested does not fail in
a graceful way.  If somebody tries to update the "git refs" command,
introduces a bug and makes it segfault when 

	git refs rename refs/heads/symref refs/heads/renamed

is run, the command line prefixed with '!' will happily say "ah, the
command crashed and did not succeed, and I am happy", but that is
not the kind of failing that we want to be happy with.  If you
prefix the command line with test_must_fail, however, test_must_fail
would still fail when the "git refs rename" died with segfault or
otherwise killed by a signal.

Thanks.


> Signed-off-by: Hardik Kumar <hardikxk@gmail.com>
> ---
>  t/t1467-refs-rename.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t1467-refs-rename.sh b/t/t1467-refs-rename.sh
> index 2b28be75c8..b7d290d05b 100755
> --- a/t/t1467-refs-rename.sh
> +++ b/t/t1467-refs-rename.sh
> @@ -88,7 +88,7 @@ test_expect_success 'rename with symbolic ref fails' '
>  		A=$(git rev-parse A) &&
>  		git refs create refs/heads/target $A &&
>  		git symbolic-ref refs/heads/symref refs/heads/target &&
> -		! git refs rename refs/heads/symref refs/heads/renamed 2>err &&
> +		test_must_fail git refs rename refs/heads/symref refs/heads/renamed 2>err &&
>  		test_grep "is a symbolic ref, .* not supported" err
>  	)
>  '
>
> ---
> base-commit: 37b3475fb530c14109211faca33716a173db61ed
> change-id: 20261028-refs-rename-test-fail-ec7a8fa74e54