[PATCH 0/4] gitweb: Some code cleanups, part 2 (low hanging fruit)

[PATCH 0/4] gitweb: Some code cleanups, part 2 (low hanging fruit)

[Jakub Narebski]

The following series was inspired by severity: 3 Perl::Critic
(http://perlcritic.com) suggestions for gitweb; the patches listed
below are simple and obvious fixes.  That is why there is no detailed
explanation of perlcritic policy for each patch.

This time I have checked that t9500 passes...
---
Jakub Narebski (4):
  gitweb: Remove unused $hash_base parameter from normalize_link_target
  gitweb: Simplify snapshot format detection logic in evaluate_path_info
  gitweb: Use capturing parentheses only when you intend to capture
  gitweb: Replace wrongly added tabs with spaces

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

-- 
Jakub Narebski
ShadeHawk on #git
Poland

[PATCH 1/4] gitweb: Replace wrongly added tabs with spaces

[Jakub Narebski]

In two places there was hard tab character instead of space.
Fix this.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I have not searched who was responsible for that typo...

 gitweb/gitweb.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8c51f3e..beb79ee 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3990,7 +3990,7 @@ sub fill_project_list_info {
 			    ($pname !~ /\/$/) &&
 			    (-d "$projectroot/$pname")) {
 				$pr->{'forks'} = "-d $projectroot/$pname";
-			}	else {
+			} else {
 				$pr->{'forks'} = 0;
 			}
 		}
@@ -6282,7 +6282,7 @@ XML
 	# end of feed
 	if ($format eq 'rss') {
 		print "</channel>\n</rss>\n";
-	}	elsif ($format eq 'atom') {
+	} elsif ($format eq 'atom') {
 		print "</feed>\n";
 	}
 }

[PATCH 2/4] gitweb: Use capturing parentheses only when you intend to capture

[Jakub Narebski]

Non-capturing groups are useful because they have better runtime
performance and do not copy strings to the magic global capture
variables.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
"Perl Best Practices", section 12.14. Capturing Parentheses (Use
capturing parentheses only when you intend to capture.)

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index beb79ee..097bd18 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -828,7 +828,7 @@ if (!defined $action) {
 if (!defined($actions{$action})) {
 	die_error(400, "Unknown action");
 }
-if ($action !~ m/^(opml|project_list|project_index)$/ &&
+if ($action !~ m/^(?:opml|project_list|project_index)$/ &&
     !$project) {
 	die_error(400, "Project needed");
 }

[PATCH 3/4] gitweb: Simplify snapshot format detection logic in evaluate_path_info

[Jakub Narebski]

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 <jnareb@gmail.com>
---
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;

[PATCH 4/4] gitweb: Remove unused $hash_base parameter from normalize_link_target

[Jakub Narebski]

...since it was decided for normalize_link_target to only mangle
pathname, and do not try to check if target is present in $hash_base
tree, for performance reasons.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This was caugth by Perl::Critic::Policy::Variables::ProhibitReusedNames 
 - Do not reuse a variable name in a lexical scope.

(That was probably me that forgot to remove parameter, but I didn't
search for authorship of this fragment.)

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c72ae10..05702e4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3339,10 +3339,7 @@ sub git_get_link_target {
 # return target of link relative to top directory (top tree);
 # return undef if it is not possible (including absolute links).
 sub normalize_link_target {
-	my ($link_target, $basedir, $hash_base) = @_;
-
-	# we can normalize symlink target only if $hash_base is provided
-	return unless $hash_base;
+	my ($link_target, $basedir) = @_;
 
 	# absolute symlinks (beginning with '/') cannot be normalized
 	return if (substr($link_target, 0, 1) eq '/');
@@ -3398,7 +3395,7 @@ sub git_print_tree_entry {
 		if (S_ISLNK(oct $t->{'mode'})) {
 			my $link_target = git_get_link_target($t->{'hash'});
 			if ($link_target) {
-				my $norm_target = normalize_link_target($link_target, $basedir, $hash_base);
+				my $norm_target = normalize_link_target($link_target, $basedir);
 				if (defined $norm_target) {
 					print " -> " .
 					      $cgi->a({-href => href(action=>"object", hash_base=>$hash_base,

Re: [PATCH 3/4] gitweb: Simplify snapshot format detection logic in evaluate_path_info

[Giuseppe Bilotta]

On Mon, May 11, 2009 at 7:42 PM, Jakub Narebski <jnareb@gmail.com> 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 <jnareb@gmail.com>
> ---
> 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.bilotta@gmail.com>


-- 
Giuseppe "Oblomov" Bilotta