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

["Hardik Kumar" <hardikxk@gmail.com>]

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