The following series consist of some code cleanups for gitweb.perl. They're based on suggestions by perlcritic (Perl::Critic). Most policy modules are in turn based on Damian Conway's book "Perl Best Practices" (PBP). This series was inspired by similar series of patches for git-send-email by Bill Pemberton: Subject: [PATCH 0/6] cleanups for git-send-email Msg-Id: <1241010743-7020-1-git-send-email-wfp5p@xxxxxxxxxxxx> URL: http://thread.gmane.org/gmane.comp.version-control.git/117881 Not all suggestions by perlcritic were implemented (or, to be more exact, by its online version at http://perlcritic.com, which is running Perl-Critic version 1.096). Below there is list of perlcritic suggestions, sorted by severity (gentle, stern, harsh, cruel, brutal): first list of patches in series which applied perlcritic suggestions, then suggestions not applied, with short explanation why they were not used. This series deals with suggestion with severity level >= 4 (gentle and stern suggestions); perlcritic suggestions with severity level >= 3 (harsh) are in the works - but there are many more rules, and they are also more subjective. Shortlog (part 1/2): ================= Jakub Narebski (3): gitweb: Remove function prototypes gitweb: Do not use bareword filehandles gitweb: Always use three argument form of open All of those are for severity gentle (5). The following policies (suggestions) with severity 5 were not implemented: * Perl::Critic::Policy::ValuesAndExpressions::ProhibitLeadingZeros Write 'oct(755)' instead of '0755'. We know what we are doing here; besides above Perl::Critic policy has exceptions for chmod, mkdir, sysopen, umask, and we use octal numbers for filemode. * Perl::Critic::Policy::Subroutines::ProhibitExplicitReturnUndef Return failure with bare 'return' instead of 'return undef'. First, this is a matter of taste; second, this would require more detailed review. So it is left as it is... for now. * Perl::Critic::Policy::Subroutines::ProhibitNestedSubs Declaring a named sub inside another named sub does not prevent the inner sub from being global. This is more about possibility of mismatched expectations. Shortlog (part 2/2): ================= Jakub Narebski (1): gitweb: Localize magic variable $/ gitweb: Use block form of map/grep in a few cases more All of those are for severity stern (4). The following policies (suggestions) with severity 4 were not implemented: * Perl::Critic::Policy::Subroutines::RequireArgUnpacking Always unpack @_ first. This requires careful handling (wrapper functions are exception of this policy, but the tool does not detect it), and in most cases this is the matter of the following techique: my $a = shift; my ($b, $c) = @_; which could be improved. * Perl::Critic::Policy::Subroutines::RequireFinalReturn End every path through a subroutine with an explicit return statement. This is I think the matter of taste; note that this policy is not to be followed blindly, according to documentation. I think that all functions that do not return would be procedures (void as return type) in C. * Perl::Critic::Policy::ValuesAndExpressions::ProhibitConstantPragma Don't use 'constant FOO => 15', because it doesn't interpolate, but the Readonly module. The constants S_IFINVALID and S_IFGITLINK we declare follow naming of filemode constants S_IFMT and S_IXUSR from Fcntl module we use in our code. * Perl::Critic::Policy::InputOutput::RequireBriefOpen Close filehandles as soon as possible after opening them (because fFilehandles are a finite resource). In gitweb we use very small number of filehandles concurrently; usually only one filehandle is open. On the other hand for bigger output we try to stream it. * Perl::Critic::Policy::ValuesAndExpressions::ProhibitMixedBooleanOperators Don't mix high- and low-precedence booleans. The code in question is list form of magic "-|" open ... or die ..., where one of arguments uses '||' to set default value. This is intended, and not that cryptic. Diffstat: ========= gitweb/gitweb.perl | 72 ++++++++++++++++++++++++++-------------------------- 1 files changed, 36 insertions(+), 36 deletions(-) -- Jakub Narebski ShadeHawk on #git -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html