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

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

[Jakub Narebski]

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

[PATCHv2 1/5] gitweb: Remove function prototypes

[Jakub Narebski]

Use of function prototypes is considered bad practice in Perl.  The
ones used here didn't accomplish anything anyhow, so they've been
removed.

>From perlsub(1):

  [...] the intent of this feature [prototypes] is primarily to let
  you define subroutines that work like built-in functions [...]
  you can generate new syntax with it [...]

We don't want to have subroutines behaving exactly like built-in
functions, we don't want to define new syntax / syntactic sugar, so
prototypes in gitweb are not needed... and they can have unintended
consequences.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Perl::Critic::Policy::Subroutines::ProhibitSubroutinePrototypes

  Don't write 'sub my_function (@@) {}'.

  Contrary to common belief, subroutine prototypes do not enable
  compile-time checks for proper arguments. Don't use them.

See also Damian Conway's book "Perl Best Practices",
chapter "9.10. Prototypes" (Don't use subroutine prototypes.)


This follows similar patch for git-send-email.perl by Bill Pemberton.
Also "sub S_ISGITLINK($) {" line caused `imenu` in my old cperl-mode
(4.23) in GNU Emacs 21.4.1 to fail, sometimes.

This patch was send with slightly different commit message as
standalone patch earlier. This is the replacement patch, which differs
only in the commit message.

 gitweb/gitweb.perl |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3f99361..06e9160 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -838,7 +838,7 @@ exit;
 ## ======================================================================
 ## action links
 
-sub href (%) {
+sub href {
 	my %params = @_;
 	# default is to use -absolute url() i.e. $my_uri
 	my $href = $params{-full} ? $my_url : $my_uri;
@@ -1036,7 +1036,7 @@ sub esc_url {
 }
 
 # replace invalid utf8 character with SUBSTITUTION sequence
-sub esc_html ($;%) {
+sub esc_html {
 	my $str = shift;
 	my %opts = @_;
 
@@ -1296,7 +1296,7 @@ use constant {
 };
 
 # submodule/subproject, a commit object reference
-sub S_ISGITLINK($) {
+sub S_ISGITLINK {
 	my $mode = shift;
 
 	return (($mode & S_IFMT) == S_IFGITLINK)
@@ -2615,7 +2615,7 @@ sub parsed_difftree_line {
 }
 
 # parse line of git-ls-tree output
-sub parse_ls_tree_line ($;%) {
+sub parse_ls_tree_line {
 	my $line = shift;
 	my %opts = @_;
 	my %res;
@@ -3213,7 +3213,6 @@ sub git_print_header_div {
 	      "\n</div>\n";
 }
 
-#sub git_print_authorship (\%) {
 sub git_print_authorship {
 	my $co = shift;
 
@@ -3269,8 +3268,7 @@ sub git_print_page_path {
 	print "<br/></div>\n";
 }
 
-# sub git_print_log (\@;%) {
-sub git_print_log ($;%) {
+sub git_print_log {
 	my $log = shift;
 	my %opts = @_;
 
-- 
1.6.3

[PATCH 2/5] gitweb: Do not use bareword filehandles

[Jakub Narebski]

The script was using bareword filehandles.  This is considered a bad
practice so they have been changed to indirect filehandles.
Changes touch git_get_project_ctags and mimetype_guess_file.

While at it rename local variable from $mime to $mimetype (in
mimetype_guess_file) to better reflect its value (its contents).

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Perl::Critic::Policy::InputOutput::ProhibitBarewordFileHandles

  Write open my $fh, q{<}, $filename; instead of open FH, q{<}, $filename;.

  Using bareword symbols to refer to file handles is particularly evil
  because they are global, and you have no idea if that symbol already
  points to some other file handle. You can mitigate some of that risk by
  'local'izing the symbol first, but that's pretty ugly. Since Perl 5.6, you
  can use an undefined scalar variable as a lexical reference to an
  anonymous filehandle.

See also Damian Conway's book "Perl Best Practices",
chapter "10.1. Filehandles" (Don't use bareword filehandles.)


This follows similar patch for git-send-email.perl by Bill Pemberton
http://permalink.gmane.org/gmane.comp.version-control.git/117886

CC-ed Pasky, who is responsible for code in both cases...

 gitweb/gitweb.perl |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 06e9160..a9daa1d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2065,18 +2065,18 @@ sub git_get_project_ctags {
 	my $ctags = {};
 
 	$git_dir = "$projectroot/$path";
-	unless (opendir D, "$git_dir/ctags") {
+	unless (opendir my $dh, "$git_dir/ctags") {
 		return $ctags;
 	}
-	foreach (grep { -f $_ } map { "$git_dir/ctags/$_" } readdir(D)) {
-		open CT, $_ or next;
-		my $val = <CT>;
+	foreach (grep { -f $_ } map { "$git_dir/ctags/$_" } readdir($dh)) {
+		open my $ct, $_ or next;
+		my $val = <$ct>;
 		chomp $val;
-		close CT;
+		close $ct;
 		my $ctag = $_; $ctag =~ s#.*/##;
 		$ctags->{$ctag} = $val;
 	}
-	closedir D;
+	closedir $dh;
 	$ctags;
 }
 
@@ -2804,18 +2804,18 @@ sub mimetype_guess_file {
 	-r $mimemap or return undef;
 
 	my %mimemap;
-	open(MIME, $mimemap) or return undef;
-	while (<MIME>) {
+	open(my $mh, $mimemap) or return undef;
+	while (<$mh>) {
 		next if m/^#/; # skip comments
-		my ($mime, $exts) = split(/\t+/);
+		my ($mimetype, $exts) = split(/\t+/);
 		if (defined $exts) {
 			my @exts = split(/\s+/, $exts);
 			foreach my $ext (@exts) {
-				$mimemap{$ext} = $mime;
+				$mimemap{$ext} = $mimetype;
 			}
 		}
 	}
-	close(MIME);
+	close($mh);
 
 	$filename =~ /\.([^.]*)$/;
 	return $mimemap{$1};
-- 
1.6.3

[PATCH 3/5] gitweb: Always use three argument form of open

[Jakub Narebski]

In most cases (except insert_file() subroutine) we used old two argument
form of 'open' to open files for reading.  This can cause subtle bugs when
$projectroot or $projects_list file starts with mode characters ('>', '<',
'+<', '|', etc.) or with leading whitespace; and also when $projects_list
file or $mimetypes_file or ctags files end with trailing whitespace or '|'.

Additionally it is also more clear to explicitly state that we open those
files for reading.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Perl::Critic::Policy::InputOutput::ProhibitTwoArgOpen

  Write open $fh, q{<}, $filename; instead of open $fh, "<$filename";.

  The three-argument form of open (introduced in Perl 5.6) prevents subtle
  bugs that occur when the filename starts with funny characters like
  '>' or '<'.  It's also more explicitly clear to define the input mode of
  the file, and not to e.g. use open( $fh, 'foo.txt' );

See also Damian Conway's book "Perl Best Practices",
chapter "10.4. Opening Cleanly" (Use either the 'IO::File' module
or the three-argument form of 'open'.)


This patch _textually_ depends on the previous patch (no bareword
filehandles), even if _conceptually_ they are quite independent.

 gitweb/gitweb.perl |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a9daa1d..852beb6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2050,7 +2050,7 @@ sub git_get_project_description {
 	my $path = shift;
 
 	$git_dir = "$projectroot/$path";
-	open my $fd, "$git_dir/description"
+	open my $fd, '<', "$git_dir/description"
 		or return git_get_project_config('description');
 	my $descr = <$fd>;
 	close $fd;
@@ -2069,7 +2069,7 @@ sub git_get_project_ctags {
 		return $ctags;
 	}
 	foreach (grep { -f $_ } map { "$git_dir/ctags/$_" } readdir($dh)) {
-		open my $ct, $_ or next;
+		open my $ct, '<', $_ or next;
 		my $val = <$ct>;
 		chomp $val;
 		close $ct;
@@ -2129,7 +2129,7 @@ sub git_get_project_url_list {
 	my $path = shift;
 
 	$git_dir = "$projectroot/$path";
-	open my $fd, "$git_dir/cloneurl"
+	open my $fd, '<', "$git_dir/cloneurl"
 		or return wantarray ?
 		@{ config_to_multi(git_get_project_config('url')) } :
 		   config_to_multi(git_get_project_config('url'));
@@ -2187,7 +2187,7 @@ sub git_get_projects_list {
 		# 'libs%2Fklibc%2Fklibc.git H.+Peter+Anvin'
 		# 'linux%2Fhotplug%2Fudev.git Greg+Kroah-Hartman'
 		my %paths;
-		open my ($fd), $projects_list or return;
+		open my $fd, '<', $projects_list or return;
 	PROJECT:
 		while (my $line = <$fd>) {
 			chomp $line;
@@ -2250,7 +2250,7 @@ sub git_get_project_list_from_file {
 	# 'libs%2Fklibc%2Fklibc.git H.+Peter+Anvin'
 	# 'linux%2Fhotplug%2Fudev.git Greg+Kroah-Hartman'
 	if (-f $projects_list) {
-		open (my $fd , $projects_list);
+		open(my $fd, '<', $projects_list);
 		while (my $line = <$fd>) {
 			chomp $line;
 			my ($pr, $ow) = split ' ', $line;
@@ -2804,7 +2804,7 @@ sub mimetype_guess_file {
 	-r $mimemap or return undef;
 
 	my %mimemap;
-	open(my $mh, $mimemap) or return undef;
+	open(my $mh, '<', $mimemap) or return undef;
 	while (<$mh>) {
 		next if m/^#/; # skip comments
 		my ($mimetype, $exts) = split(/\t+/);
-- 
1.6.3

[PATCH 4/5] gitweb: Localize magic variable $/

[Jakub Narebski]

Instead of undefining and then restoring magic variable $/ (input
record separator) for 'slurp mode', localize it.

While at it, state explicitly that "local $/;" makes it undefined, by
using explicit  "local $/ = undef;".

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Perl::Critic::Policy::Variables::RequireLocalizedPunctuationVars

  Magic variables should be assigned as "local".

  Punctuation variables (and their English.pm equivalents) are global
  variables. Messing with globals is dangerous in a complex program as it
  can lead to very subtle and hard to fix bugs. If you must change a magic
  variable in a non-trivial program, do it in a local scope.

  For example, to slurp a filehandle into a scalar, it's common to set the
  record separator to undef instead of a newline. If you choose to do this
  (instead of using File::Slurp!) then be sure to localize the global and
  change it for as short a time as possible.

See also Damian Conway's book "Perl Best Practices",
5.6. Localizing Punctuation Variables (If you're forced to modify
a punctuation variable, localize it.)


Perl::Critic::Policy::Variables::RequireInitializationForLocalVars

  Write  'local $foo = $bar;'  instead of just  'local $foo;'.

  Most people don't realize that a localized copy of a variable does
  not retain its original value. Unless you initialize the variable
  when you local-ize it, it defaults to undef. If you want the
  variable to retain its original value, just initialize it to
  itself. If you really do want the localized copy to be undef, then
  make it explicit.


 gitweb/gitweb.perl |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 852beb6..1cb3a4f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3326,7 +3326,7 @@ sub git_get_link_target {
 	open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
 		or return;
 	{
-		local $/;
+		local $/ = undef;
 		$link_target = <$fd>;
 	}
 	close $fd
@@ -4801,11 +4801,10 @@ sub git_blob_plain {
 		-content_disposition =>
 			($sandbox ? 'attachment' : 'inline')
 			. '; filename="' . $save_as . '"');
-	undef $/;
+	local $/ = undef;
 	binmode STDOUT, ':raw';
 	print <$fd>;
 	binmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi
-	$/ = "\n";
 	close $fd;
 }
 
@@ -4907,12 +4906,15 @@ sub git_tree {
 		}
 	}
 	die_error(404, "No such tree") unless defined($hash);
-	$/ = "\0";
-	open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
-		or die_error(500, "Open git-ls-tree failed");
-	my @entries = map { chomp; $_ } <$fd>;
-	close $fd or die_error(404, "Reading tree failed");
-	$/ = "\n";
+
+	{
+		local $/ = "\0";
+		open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
+			or die_error(500, "Open git-ls-tree failed");
+		my @entries = map { chomp; $_ } <$fd>;
+		close $fd
+			or die_error(404, "Reading tree failed");
+	}
 
 	my $refs = git_get_references();
 	my $ref = format_ref_marker($refs, $hash_base);
@@ -5807,7 +5809,7 @@ sub git_search {
 
 		print "<table class=\"pickaxe search\">\n";
 		my $alternate = 1;
-		$/ = "\n";
+		local $/ = "\n";
 		open my $fd, '-|', git_cmd(), '--no-pager', 'log', @diff_opts,
 			'--pretty=format:%H', '--no-abbrev', '--raw', "-S$searchtext",
 			($search_use_regexp ? '--pickaxe-regex' : ());
@@ -5877,7 +5879,7 @@ sub git_search {
 		print "<table class=\"grep_search\">\n";
 		my $alternate = 1;
 		my $matches = 0;
-		$/ = "\n";
+		local $/ = "\n";
 		open my $fd, "-|", git_cmd(), 'grep', '-n',
 			$search_use_regexp ? ('-E', '-i') : '-F',
 			$searchtext, $co{'tree'};
-- 
1.6.3

[PATCH 5/5] gitweb: Use block form of map/grep in a few cases more

[Jakub Narebski]

Use block form of 'grep' i.e. 'grep {BLOCK} LIST' rather than
'grep(EXPR, LIST)' in filter_snapshot_fmts subroutine.  This makes
code more readable, as expression is rather long, and statement above
there is 'map' with very similar expression also in the block form.

Remove unnecessary and misleading parentheses around block form 'map'
arguments in quote_command subroutine.

The inner "map" in format_snapshot_links was left alone, as it is not
clear whether adding parentheses or changing it into block form would
improve readibility and clarity of this code.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Perl::Critic::Policy::BuiltinFunctions::RequireBlockGrep

  Write grep { $_ =~ /$pattern/ } @list instead of grep /$pattern/, @list.

  The expression forms of grep and map are awkward and hard to read. Use the
  block forms instead.

See also Damian Conway's book "Perl Best Practices", section
8.13. Mapping and Grepping (Always use a block with a map and grep.)

NOTE: In my opinion the expression form when using function-like call to
"grep" or "map" e.g. grep(/$pattern/, @list) is readable enough.  In more
complicated cases (with more complicated expressions, especially with
explicit $_) it might be better to use block form instead, as stated above.

And what do *you* think?

 gitweb/gitweb.perl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1cb3a4f..f465666 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -458,8 +458,8 @@ sub filter_snapshot_fmts {
 	@fmts = map {
 		exists $known_snapshot_format_aliases{$_} ?
 		       $known_snapshot_format_aliases{$_} : $_} @fmts;
-	@fmts = grep(exists $known_snapshot_formats{$_}, @fmts);
-
+	@fmts = grep {
+		exists $known_snapshot_formats{$_} } @fmts;
 }
 
 our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
@@ -1838,7 +1838,7 @@ sub git_cmd {
 # Try to avoid using this function wherever possible.
 sub quote_command {
 	return join(' ',
-		    map( { my $a = $_; $a =~ s/(['!])/'\\$1'/g; "'$a'" } @_ ));
+		map { my $a = $_; $a =~ s/(['!])/'\\$1'/g; "'$a'" } @_ );
 }
 
 # get HEAD ref of given project as hash
-- 
1.6.3

Re: [PATCH 2/5] gitweb: Do not use bareword filehandles

[Petr Baudis]

On Sun, May 10, 2009 at 02:36:19AM +0200, Jakub Narebski wrote:
> The script was using bareword filehandles.  This is considered a bad
> practice so they have been changed to indirect filehandles.
> Changes touch git_get_project_ctags and mimetype_guess_file.
> 
> While at it rename local variable from $mime to $mimetype (in
> mimetype_guess_file) to better reflect its value (its contents).
> 
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> Perl::Critic::Policy::InputOutput::ProhibitBarewordFileHandles
> 
>   Write open my $fh, q{<}, $filename; instead of open FH, q{<}, $filename;.
> 
>   Using bareword symbols to refer to file handles is particularly evil
>   because they are global, and you have no idea if that symbol already
>   points to some other file handle. You can mitigate some of that risk by
>   'local'izing the symbol first, but that's pretty ugly. Since Perl 5.6, you
>   can use an undefined scalar variable as a lexical reference to an
>   anonymous filehandle.
> 
> See also Damian Conway's book "Perl Best Practices",
> chapter "10.1. Filehandles" (Don't use bareword filehandles.)
> 
> 
> This follows similar patch for git-send-email.perl by Bill Pemberton
> http://permalink.gmane.org/gmane.comp.version-control.git/117886
> 
> CC-ed Pasky, who is responsible for code in both cases...

Yeah, the book I learnt Perl from many years ago used bareword
filehandles (but it was an excellent textbook in most other aspects)
so this is a custom I have to work hard to evict. ;-)

Acked-by: Petr Baudis <pasky@suse.cz>

Re: [PATCHv2 1/5] gitweb: Remove function prototypes

[Jakub Narebski]

On Sun, 10 May 2009, Jakub Narebski wrote:

> This patch was send with slightly different commit message as
> standalone patch earlier. This is the replacement patch, which differs
> only in the commit message.

I see that v1 version of patch (send as standalone patch) was already 
accepted.  Just skip this patch then, as it differs only in that it has 
more wordy commit message...

-- 
Jakub Narebski
Poland

Re: [PATCH 2/5] gitweb: Do not use bareword filehandles

[Jakub Narebski]

On Sun, 10 May 2009, Petr Baudis wrote:
> On Sun, May 10, 2009 at 02:36:19AM +0200, Jakub Narebski wrote:

> > ---
> > Perl::Critic::Policy::InputOutput::ProhibitBarewordFileHandles
> > 
> >   Write open my $fh, q{<}, $filename; instead of open FH, q{<}, $filename;.
> > 
> >   Using bareword symbols to refer to file handles is particularly evil
> >   because they are global, and you have no idea if that symbol already
> >   points to some other file handle. You can mitigate some of that risk by
> >   'local'izing the symbol first, but that's pretty ugly. Since Perl 5.6, you
> >   can use an undefined scalar variable as a lexical reference to an
> >   anonymous filehandle.
> > 
> > See also Damian Conway's book "Perl Best Practices",
> > chapter "10.1. Filehandles" (Don't use bareword filehandles.)
> > 
> > 
> > This follows similar patch for git-send-email.perl by Bill Pemberton
> > http://permalink.gmane.org/gmane.comp.version-control.git/117886
> > 
> > CC-ed Pasky, who is responsible for code in both cases...
> 
> Yeah, the book I learnt Perl from many years ago used bareword
> filehandles (but it was an excellent textbook in most other aspects)
> so this is a custom I have to work hard to evict. ;-)
> 
> Acked-by: Petr Baudis <pasky@suse.cz>

Well, on one hand this is less of an issue for standalone script like
gitweb is than for library / module.  On the other hand we use indirect
filehandles everywhere else, so it is also matter of style consistency.

"Perl Best Practices" gives as an example of bad behavior which we can
get by using bareword [global] filehandles: 1) automatic closing of
other filehandle with the same name (e.g. not only you use FILE for
bareword filehandle); 2) open might fail if there exist subroutine /
constant with the same name as filehandle e.g. EXDEV (from POSIX module).

Note however that prior to Perl 5.6 it was not that easy to avoid
using bareword filehandles. Perhaps the book in question (or author of
the textbook you mention) started with earlier Perl...


P.S. Git.pm generates surprisingly small amount of perlcritic-isms,
most of whose are either matter of style, or are false positives.
-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> The following series consist of some code cleanups for gitweb.perl.
> They're based on suggestions by perlcritic (Perl::Critic).

Nice.

But this series, when queued to 'pu', seems to break t9500; I haven't
looked at the breakage myself yet.

Jakub, did you run this through the testsuite already (the problem could
well be on my end and that is why I am asking)?

[PATCH v2 2/5] gitweb: Do not use bareword filehandles

[Jakub Narebski]

gitweb: Do not use bareword filehandles

The script was using bareword filehandles.  This is considered a bad
practice so they have been changed to indirect filehandles.

Changes touch git_get_project_ctags and mimetype_guess_file;
while at it rename local variable from $mime to $mimetype (in
mimetype_guess_file) to better reflect its value (its contents).

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I'm extremely sorry for this stupid mistake. Below there is interdiff
(indented, as to not be mistaken for diff itself)

	diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
	index a9daa1d..584644c 100755
	--- a/gitweb/gitweb.perl
	+++ b/gitweb/gitweb.perl
	@@ -2065,9 +2065,8 @@ sub git_get_project_ctags {
	        my $ctags = {};
	 
	        $git_dir = "$projectroot/$path";
	-       unless (opendir my $dh, "$git_dir/ctags") {
	-               return $ctags;
	-       }
	+       opendir my $dh, "$git_dir/ctags"
	+               or return $ctags;
	        foreach (grep { -f $_ } map { "$git_dir/ctags/$_" } readdir($dh)) {
	                open my $ct, $_ or next;
	                my $val = <$ct>;

 gitweb/gitweb.perl |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 06e9160..584644c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2065,18 +2065,17 @@ sub git_get_project_ctags {
 	my $ctags = {};
 
 	$git_dir = "$projectroot/$path";
-	unless (opendir D, "$git_dir/ctags") {
-		return $ctags;
-	}
-	foreach (grep { -f $_ } map { "$git_dir/ctags/$_" } readdir(D)) {
-		open CT, $_ or next;
-		my $val = <CT>;
+	opendir my $dh, "$git_dir/ctags"
+		or return $ctags;
+	foreach (grep { -f $_ } map { "$git_dir/ctags/$_" } readdir($dh)) {
+		open my $ct, $_ or next;
+		my $val = <$ct>;
 		chomp $val;
-		close CT;
+		close $ct;
 		my $ctag = $_; $ctag =~ s#.*/##;
 		$ctags->{$ctag} = $val;
 	}
-	closedir D;
+	closedir $dh;
 	$ctags;
 }
 
@@ -2804,18 +2803,18 @@ sub mimetype_guess_file {
 	-r $mimemap or return undef;
 
 	my %mimemap;
-	open(MIME, $mimemap) or return undef;
-	while (<MIME>) {
+	open(my $mh, $mimemap) or return undef;
+	while (<$mh>) {
 		next if m/^#/; # skip comments
-		my ($mime, $exts) = split(/\t+/);
+		my ($mimetype, $exts) = split(/\t+/);
 		if (defined $exts) {
 			my @exts = split(/\s+/, $exts);
 			foreach my $ext (@exts) {
-				$mimemap{$ext} = $mime;
+				$mimemap{$ext} = $mimetype;
 			}
 		}
 	}
-	close(MIME);
+	close($mh);
 
 	$filename =~ /\.([^.]*)$/;
 	return $mimemap{$1};
-- 
1.6.3

[PATCH v2 3/5] gitweb: Always use three argument form of open

[Jakub Narebski]

>From 94638fb6edf3ea693228c680a6a30271ccd77522 Mon Sep 17 00:00:00 2001
From: Jakub Narebski <jnareb@gmail.com>
Date: Mon, 11 May 2009 03:25:55 +0200
Subject: [PATCH] gitweb: Localize magic variable $/

Instead of undefining and then restoring magic variable $/ (input
record separator) for 'slurp mode', localize it.

While at it, state explicitely that "local $/;" makes it undefined, by
using explicit  "local $/ = undef;".

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This also required correction. I am extremely sorry about that.
Below there is (whitespace mangled) interdiff to previous version

	diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
	index 76c0684..4efeeed 100755
	--- a/gitweb/gitweb.perl
	+++ b/gitweb/gitweb.perl
	@@ -4906,11 +4906,12 @@ sub git_tree {
	        }
	        die_error(404, "No such tree") unless defined($hash);
	 
	+       my @entries = ();
	        {
	                local $/ = "\0";
	                open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
	                        or die_error(500, "Open git-ls-tree failed");
	-               my @entries = map { chomp; $_ } <$fd>;
	+               @entries = map { chomp; $_ } <$fd>;
	                close $fd
	                        or die_error(404, "Reading tree failed");
	        }



 gitweb/gitweb.perl |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e7cab90..4efeeed 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3325,7 +3325,7 @@ sub git_get_link_target {
 	open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
 		or return;
 	{
-		local $/;
+		local $/ = undef;
 		$link_target = <$fd>;
 	}
 	close $fd
@@ -4800,11 +4800,10 @@ sub git_blob_plain {
 		-content_disposition =>
 			($sandbox ? 'attachment' : 'inline')
 			. '; filename="' . $save_as . '"');
-	undef $/;
+	local $/ = undef;
 	binmode STDOUT, ':raw';
 	print <$fd>;
 	binmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi
-	$/ = "\n";
 	close $fd;
 }
 
@@ -4906,12 +4905,16 @@ sub git_tree {
 		}
 	}
 	die_error(404, "No such tree") unless defined($hash);
-	$/ = "\0";
-	open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
-		or die_error(500, "Open git-ls-tree failed");
-	my @entries = map { chomp; $_ } <$fd>;
-	close $fd or die_error(404, "Reading tree failed");
-	$/ = "\n";
+
+	my @entries = ();
+	{
+		local $/ = "\0";
+		open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
+			or die_error(500, "Open git-ls-tree failed");
+		@entries = map { chomp; $_ } <$fd>;
+		close $fd
+			or die_error(404, "Reading tree failed");
+	}
 
 	my $refs = git_get_references();
 	my $ref = format_ref_marker($refs, $hash_base);
@@ -5806,7 +5809,7 @@ sub git_search {
 
 		print "<table class=\"pickaxe search\">\n";
 		my $alternate = 1;
-		$/ = "\n";
+		local $/ = "\n";
 		open my $fd, '-|', git_cmd(), '--no-pager', 'log', @diff_opts,
 			'--pretty=format:%H', '--no-abbrev', '--raw', "-S$searchtext",
 			($search_use_regexp ? '--pickaxe-regex' : ());
@@ -5876,7 +5879,7 @@ sub git_search {
 		print "<table class=\"grep_search\">\n";
 		my $alternate = 1;
 		my $matches = 0;
-		$/ = "\n";
+		local $/ = "\n";
 		open my $fd, "-|", git_cmd(), 'grep', '-n',
 			$search_use_regexp ? ('-E', '-i') : '-F',
 			$searchtext, $co{'tree'};
-- 
1.6.3

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

[Jakub Narebski]

On Mon, 11 May 2009, Junio C Hamano wrote:

> But this series, when queued to 'pu', seems to break t9500; I haven't
> looked at the breakage myself yet.

I'm sorry about that. My bad. The fix is in the email (unless you
prefer for me to just resend the series)...

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> On Mon, 11 May 2009, Junio C Hamano wrote:
>
>> But this series, when queued to 'pu', seems to break t9500; I haven't
>> looked at the breakage myself yet.
>
> I'm sorry about that. My bad. The fix is in the email (unless you
> prefer for me to just resend the series)...

That's Ok.  I had them near the tip of 'pu', and I can just replace them.

But this episode does not give much confidence in Perl::Critic does it?
The runtime "use strict" diagnosed undeclared globals in the cleaned up
code, but presumably the Critic did not complain anything about it, right?

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

[Daniel Pittman]

Junio C Hamano <gitster@pobox.com> writes:
> Jakub Narebski <jnareb@gmail.com> writes:
>> On Mon, 11 May 2009, Junio C Hamano wrote:
>>
>>> But this series, when queued to 'pu', seems to break t9500; I haven't
>>> looked at the breakage myself yet.
>>
>> I'm sorry about that. My bad. The fix is in the email (unless you
>> prefer for me to just resend the series)...
>
> That's Ok.  I had them near the tip of 'pu', and I can just replace them.
>
> But this episode does not give much confidence in Perl::Critic does it?
> The runtime "use strict" diagnosed undeclared globals in the cleaned up
> code, but presumably the Critic did not complain anything about it, right?

Perl::Critic is about coding style, not about tests like 'use strict'
that are detected by Perl already, for better or worse.

In other words: like checkpatch on the LKML, not like sparse. ;)

Regards,
        Daniel

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

[Jakub Narebski]

On Mon, 11 May 2009, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes: 
>> On Mon, 11 May 2009, Junio C Hamano wrote:
>>
>>> But this series, when queued to 'pu', seems to break t9500; I haven't
>>> looked at the breakage myself yet.
>>
>> I'm sorry about that. My bad. The fix is in the email (unless you
>> prefer for me to just resend the series)...
> 
> That's Ok.  I had them near the tip of 'pu', and I can just replace them.
> 
> But this episode does not give much confidence in Perl::Critic does it?
> The runtime "use strict" diagnosed undeclared globals in the cleaned up
> code, but presumably the Critic did not complain anything about it, right?

Well, IIRC it didn't complain because I didn't run Perl::Critic (or to
be more exact http://perlcritic.com) on cleaned up code... But I guess
that Perl::Critic might not try to catch them because 'use strict'
catches them.

P.S. Of course Perl::Critic is not perfect. Most funny quirk was it
complaining in --harsh (severity 3) mode 
  Main code has high complexity score (54) at line 1, column 1.
  Consider refactoring.  Severity: 3
about "#!/usr/bin/perl" line!

-- 
Jakub Narebski
Poland