From: Giuseppe Bilotta Date: Mon, 11 May 2009 17:52:23 GMT Subject: Re: [PATCH 3/4] gitweb: Simplify snapshot format detection logic in evaluate_path_info Message-ID: In-Reply-To: <20090511173950.15152.61267.stgit@localhost.localdomain> On Mon, May 11, 2009 at 7:42 PM, Jakub Narebski wrote: > This issue was caught by perlcritic in harsh severity level noticing > that catch variable was used outside conditional thanks to the > Perl::Critic::Policy::RegularExpressions::ProhibitCaptureWithoutTest > policy.  See "Perl Best Practices", chapter 12. Regular Expressions, > section 12.15. Captured Values: > >   Pattern matches that fail never assign anything to $1, $2, etc., >   nor do they leave those variables undefined. After an unsuccessful >   pattern match, the numeric capture variables remain exactly as they >   were before the match was attempted. > > New version is in my opinion much easier to understand; previous > version worked correctly due to the fact that we returned from loop > on first found match. > > Signed-off-by: Jakub Narebski > --- > This is not the only place caught by this policy, but this is the one > where fix was obviously needed to improve readibility of code. > > In _most_ (but not all) of other places we assume that output we parse > is in given format, and that regexp would always match... > >  gitweb/gitweb.perl |    7 ++++--- >  1 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index 097bd18..c72ae10 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -690,9 +690,10 @@ sub evaluate_path_info { >                # format key itself, with a prepended dot >                while (my ($fmt, $opt) = each %known_snapshot_formats) { >                        my $hash = $refname; > -                       my $sfx; > -                       $hash =~ s/(\Q$opt->{'suffix'}\E|\Q.$fmt\E)$//; > -                       next unless $sfx = $1; > +                       unless ($hash =~ s/(\Q$opt->{'suffix'}\E|\Q.$fmt\E)$//) { > +                               next; > +                       } > +                       my $sfx = $1; >                        # a valid suffix was found, so set the snapshot format >                        # and reset the hash parameter >                        $input_params{'snapshot_format'} = $fmt; Totally mea culpa on this, I was optimizing it the wrong way. Acked-by: Giuseppe Bilotta -- Giuseppe "Oblomov" Bilotta