From ad87e4f6f19e78b3f2d7dde3d3ed403db4f79a03 Mon Sep 17 00:00:00 2001 From: Jakub Narebski Date: Mon, 11 May 2009 03:21:06 +0200 Subject: [PATCH 1/8] gitweb: Do not use bareword filehandles 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 Signed-off-by: Junio C Hamano --- gitweb/gitweb.perl | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 06e91608fa..584644cbee 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 = ; + 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 () { + 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}; From dff2b6d4842eef0a03a3c8b3761f72e2b55b609e Mon Sep 17 00:00:00 2001 From: Jakub Narebski Date: Sun, 10 May 2009 02:38:34 +0200 Subject: [PATCH 2/8] gitweb: Always use three argument form of open 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 Signed-off-by: Junio C Hamano --- gitweb/gitweb.perl | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 584644cbee..e7cab9020f 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; @@ -2068,7 +2068,7 @@ sub git_get_project_ctags { opendir my $dh, "$git_dir/ctags" or 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; @@ -2128,7 +2128,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')); @@ -2186,7 +2186,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; @@ -2249,7 +2249,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; @@ -2803,7 +2803,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+/); From 34122b57eca747022336f5a3dc1aa80377d1ce56 Mon Sep 17 00:00:00 2001 From: Jakub Narebski Date: Mon, 11 May 2009 03:29:40 +0200 Subject: [PATCH 3/8] gitweb: Always use three argument form of open From 94638fb6edf3ea693228c680a6a30271ccd77522 Mon Sep 17 00:00:00 2001 From: Jakub Narebski 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 Signed-off-by: Junio C Hamano --- gitweb/gitweb.perl | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index e7cab9020f..4efeeedccf 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 "\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 "
\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'}; From 68cedb1fea0bbcd5f7c32ce10e3c346bc6db38c5 Mon Sep 17 00:00:00 2001 From: Jakub Narebski Date: Sun, 10 May 2009 02:40:37 +0200 Subject: [PATCH 4/8] gitweb: Use block form of map/grep in a few cases more 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 Signed-off-by: Junio C Hamano --- gitweb/gitweb.perl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 4efeeedccf..8c51f3e79e 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 From 3278fbc5ce39e0f7bf095ce99912dccbc347b4d7 Mon Sep 17 00:00:00 2001 From: Jakub Narebski Date: Mon, 11 May 2009 19:37:28 +0200 Subject: [PATCH 5/8] 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 Signed-off-by: Junio C Hamano --- gitweb/gitweb.perl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 8c51f3e79e..beb79eebd5 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 "\n\n"; - } elsif ($format eq 'atom') { + } elsif ($format eq 'atom') { print "\n"; } } From e8bb4b38dfcbd5ff02ceb5e925d53c1460887df5 Mon Sep 17 00:00:00 2001 From: Jakub Narebski Date: Mon, 11 May 2009 19:39:43 +0200 Subject: [PATCH 6/8] 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 Signed-off-by: Junio C Hamano --- gitweb/gitweb.perl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index beb79eebd5..097bd18be5 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"); } From 095e914281395f6c0529ce39939d804eb2ccec02 Mon Sep 17 00:00:00 2001 From: Jakub Narebski Date: Mon, 11 May 2009 19:42:47 +0200 Subject: [PATCH 7/8] 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 Acked-by: Giuseppe Bilotta Signed-off-by: Junio C Hamano --- gitweb/gitweb.perl | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 097bd18be5..c72ae10ef1 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; From 15c54fe7aa9376de2e03045122723ebde09bfeeb Mon Sep 17 00:00:00 2001 From: Jakub Narebski Date: Mon, 11 May 2009 19:45:11 +0200 Subject: [PATCH 8/8] 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 Signed-off-by: Junio C Hamano --- gitweb/gitweb.perl | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index c72ae10ef1..05702e4070 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,