threads / patch / 19292

patch, 4 partsgitweb: Some code cleanups, part 2 (low hanging fruit)

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

## tl;dr

6 messages between May 11, 2009 and May 11, 2009. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Jakub Narebski· May 11, 2009, 17:36 UTC · lore

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
Jakub Narebski· May 11, 2009, 17:37 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +2 −2
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";
 	}
 }
Jakub Narebski· May 11, 2009, 17:39 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +1 −1
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");
 }
Jakub Narebski· May 11, 2009, 17:42 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +4 −3
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;
Giuseppe Bilotta· May 11, 2009, 17:52 UTC · re: Jakub Narebski · lore

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

On Mon, May 11, 2009 at 7:42 PM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 44 quoted lines
> 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
Jakub Narebski· May 11, 2009, 17:45 UTC · re: Jakub Narebski · lore

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

...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(-)
Show changes to gitweb/gitweb.perl +2 −5
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,

← back to recent threads