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

> On Tue, Dec 3, 2013 at 8:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Krzesimir Nowak <krzesimir@endocode.com> writes:
> 
> >> +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;
> >> +}
> [...]
> >> @@ -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.
>