Re: [PATCH] t1402: test forbidden characters in refnames

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

Patrick Steinhardt <ps@pks.im> writes:

> On Thu, Aug 13, 2026 at 08:43:56PM +0000, Nikolaus Schuetz via GitGitGadget wrote:
>> From: Nikolaus Schuetz <nikolauspschuetz@gmail.com>
>> 
>> git-check-ref-format(1) documents that a refname cannot contain a
>> space, tilde, caret, colon, question-mark, asterisk or open-bracket,
>> and that it cannot be the single character "@".  Of these, only "?"
>> was tested as a character embedded in an otherwise-valid refname;
>> "*" was checked only as a lone character or with --refspec-pattern.
>> 
>> Add the remaining forbidden characters in that embedded form, and
>> check that "@" alone is rejected even with --allow-onelevel -- where
>> "@" is otherwise a valid refname component, as "refs/@" confirms.
>
> Okay.
>
>> diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
>> index cabc516ae9..bc1e878a0f 100755
>> --- a/t/t1402-check-ref-format.sh
>> +++ b/t/t1402-check-ref-format.sh
>> @@ -51,12 +51,20 @@ invalid_ref '.refs/foo'
>>  invalid_ref 'refs/heads/foo.'
>>  invalid_ref 'heads/foo..bar'
>>  invalid_ref 'heads/foo?bar'
>> +invalid_ref 'heads/foo~bar'
>> +invalid_ref 'heads/foo^bar'
>> +invalid_ref 'heads/foo:bar'
>> +invalid_ref 'heads/foo*bar'
>> +invalid_ref 'heads/foo[bar'
>> +invalid_ref 'heads/foo bar'
>
> This feels a tiny bit excessive, but I guess it does not hurt to enforce
> this property, especially now that it's so easy to add new backends.

"Why would we even care to check these insane cases?" was my first
reaction, but I agree with you that these are to protect authors of
new backends from stupid mistakes.

> One thing I was briefly wondering is whether we could maybe have a
> simple loop here, as this feels quite repetitive. We could for example:
>
>     for c in '?' '~' '^' ':' '*' '[' ' '
>     do
>         invalid_ref "heads/foo${c}bar"
>     done

True.  And c does not have to be a single byte. ".." can also be
part of the repertoire.

> By the way, one weird bit: is it intentional that all of these really
> use "heads/something" instead of "refs/heads/something"? I guess it
> ultimately doesn't matter.
>
>>  valid_ref 'foo./bar'
>>  invalid_ref 'heads/foo.lock'
>>  invalid_ref 'heads///foo.lock'
>>  invalid_ref 'foo.lock/bar'
>>  invalid_ref 'foo.lock///bar'
>>  valid_ref 'heads/foo@bar'
>> +valid_ref 'refs/@'
>> +invalid_ref '@' --allow-onelevel
>
> This one certainly is a good addition, as these are quite a bit more
> subtle.
>
> Thanks!
>
> Patrick