[PATCH 0/5] gitweb: Some code cleanups (up to perlcritic --stern)

[Jakub Narebski <jnareb@gmail.com>]

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@virginia.edu>
  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