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

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

[Hardik Kumar]

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.

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.

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

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

[Junio C Hamano]

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

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

[Hardik Kumar]

On Wed Oct 28, 2026 at 9:13 AM IST, Junio C Hamano wrote:
> 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 ;-)

Yes of course what I was trying to say here was that based on the
Release Notes the use of `test_must_fail` must be limited to only Git
commands (expected to fail). So the patch is following that principle.

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

Thanks for explaining that! I will update the commit message in v2.

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