Re: [PATCH 1/3] gitweb: Move check-ref-format code into separate function

From: Jakub Narębski <jnareb@gmail.com>
Date: Dec 3, 2013, 19:38 UTC
Message-ID: <CANQwDwd0_bgKfjsRFjohmzBAnN7vDm-pYYsfe0Q71Za2K7Hw4w@mail.gmail.com>
In-Reply-To: <xmqqd2ldoj0s.fsf@gitster.dls.corp.google.com>

On Tue, Dec 3, 2013 at 8:02 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Krzesimir Nowak <krzesimir@endocode.com> writes:

Show 9 quoted lines

>> +sub check_ref_format {
>> +     my $input = shift || return undef;
>> +
>> +     # restrictions on ref name according to git-check-ref-format
>> +     if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
>> +             return undef;
>> +     }
>> +     return $input;
>> +}

[...]

Show 21 quoted lines

>> @@ -1462,10 +1472,9 @@ sub validate_refname {
>>       # it must be correct pathname
>>       $input = validate_pathname($input)
>>               or return undef;
>> -     # restrictions on ref name according to git-check-ref-format
>> -     if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
>> -             return undef;
>> -     }
>
> So far, so good.
>
>> +     # check git-check-ref-format restrictions
>> +     $input = check_ref_format($input)
>> +             or return undef;
>>       return $input;
>
> Hmmm.  Why do you need "<LF><INDENT>or return under" here?  It would
> not hurt too much per-se (strictly speaking, if the $input were a
> string "0", this will return undef instead of "0", which should be
> an OK name as far as the regexp is concerned), but it seems to be
> making the logic unnecessarily complex for no real gain.

I think this simply follows "$input = validate_sth($input) or return undef;" pattern used in this area of gitweb code (perhaps mis-used).

Stricly speaking pure refactoring (no functional change, e.g. no assign to $input) would be "check_ref_format($input) or return undef;", or even "return check_ref_format($input);" if we keep check_ref_format() passthru on valid refname.

-- 
Jakub Narebski