From: Krzesimir Nowak <krzesimir@endocode.com>
Date: Wed, 04 Dec 2013 13:06:44 GMT
Subject: Re: [PATCH 1/3] gitweb: Move check-ref-format code into separate function
Message-ID: <1386162404.2173.15.camel@localhost.localdomain>
In-Reply-To: <CANQwDwd0_bgKfjsRFjohmzBAnN7vDm-pYYsfe0Q71Za2K7Hw4w@mail.gmail.com>

On Tue, 2013-12-03 at 20:38 +0100, Jakub Narębski wrote:
> 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).
> 

Reading the validate_* subs and check_ref_format sub makes me think that
they should just return either 0 when validation fails or 1 when it
succeeds.

If I add an extra branch ref named "0", then check_ref_format just
returns "0" as it matches the git-check-ref-format style regex. But "0"
is coerced to 0 and validate_and_filter_extra_branches will error out. I
actually reproduced that.

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

I fixed that too.

-- 
Krzesimir Nowak
Software Developer
Endocode AG

krzesimir@endocode.com

------
Endocode AG, Johannisstraße 20, 10117 Berlin
info@endocode.com | www.endocode.com

Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker
Aufsichtsratsvorsitzende: Jennifer Beecher

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B