{"thread":{"id":"19292","subject":"[PATCH 0/4] gitweb: Some code cleanups, part 2 (low hanging fruit)","startedAt":"2009-05-11T17:36:26Z","lastAt":"2009-05-11T17:52:23Z","messageCount":6,"participants":["Jakub Narebski","Giuseppe Bilotta"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"113553","messageId":"20090511173025.15152.94215.stgit@localhost.localdomain","threadId":"19292","inReplyTo":null,"subject":"[PATCH 0/4] gitweb: Some code cleanups, part 2 (low hanging fruit)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-05-11T17:36:26Z","receivedAt":"2009-05-11T17:36:26Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"The following series was inspired by severity: 3 Perl::Critic\n(http://perlcritic.com) suggestions for gitweb; the patches listed\nbelow are simple and obvious fixes.  That is why there is no detailed\nexplanation of perlcritic policy for each patch.\n\nThis time I have checked that t9500 passes...\n---\nJakub Narebski (4):\n  gitweb: Remove unused $hash_base parameter from normalize_link_target\n  gitweb: Simplify snapshot format detection logic in evaluate_path_info\n  gitweb: Use capturing parentheses only when you intend to capture\n  gitweb: Replace wrongly added tabs with spaces\n\n gitweb/gitweb.perl |   20 +++++++++-----------\n 1 files changed, 9 insertions(+), 11 deletions(-)\n\n-- \nJakub Narebski\nShadeHawk on #git\nPoland\n"},{"id":"113554","messageId":"20090511173632.15152.80943.stgit@localhost.localdomain","threadId":"19292","inReplyTo":"20090511173025.15152.94215.stgit@localhost.localdomain","subject":"[PATCH 1/4] gitweb: Replace wrongly added tabs with spaces","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-05-11T17:37:28Z","receivedAt":"2009-05-11T17:37:28Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"In two places there was hard tab character instead of space.\nFix this.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nI have not searched who was responsible for that typo...\n\n gitweb/gitweb.perl |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 8c51f3e..beb79ee 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3990,7 +3990,7 @@ sub fill_project_list_info {\n \t\t\t    ($pname !~ /\\/$/) &&\n \t\t\t    (-d \"$projectroot/$pname\")) {\n \t\t\t\t$pr->{'forks'} = \"-d $projectroot/$pname\";\n-\t\t\t}\telse {\n+\t\t\t} else {\n \t\t\t\t$pr->{'forks'} = 0;\n \t\t\t}\n \t\t}\n@@ -6282,7 +6282,7 @@ XML\n \t# end of feed\n \tif ($format eq 'rss') {\n \t\tprint \"</channel>\\n</rss>\\n\";\n-\t}\telsif ($format eq 'atom') {\n+\t} elsif ($format eq 'atom') {\n \t\tprint \"</feed>\\n\";\n \t}\n }\n"},{"id":"113555","messageId":"20090511173733.15152.83661.stgit@localhost.localdomain","threadId":"19292","inReplyTo":"20090511173025.15152.94215.stgit@localhost.localdomain","subject":"[PATCH 2/4] gitweb: Use capturing parentheses only when you intend to capture","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-05-11T17:39:43Z","receivedAt":"2009-05-11T17:39:43Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Non-capturing groups are useful because they have better runtime\nperformance and do not copy strings to the magic global capture\nvariables.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\n\"Perl Best Practices\", section 12.14. Capturing Parentheses (Use\ncapturing parentheses only when you intend to capture.)\n\n gitweb/gitweb.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex beb79ee..097bd18 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -828,7 +828,7 @@ if (!defined $action) {\n if (!defined($actions{$action})) {\n \tdie_error(400, \"Unknown action\");\n }\n-if ($action !~ m/^(opml|project_list|project_index)$/ &&\n+if ($action !~ m/^(?:opml|project_list|project_index)$/ &&\n     !$project) {\n \tdie_error(400, \"Project needed\");\n }\n"},{"id":"113556","messageId":"20090511173950.15152.61267.stgit@localhost.localdomain","threadId":"19292","inReplyTo":"20090511173025.15152.94215.stgit@localhost.localdomain","subject":"[PATCH 3/4] gitweb: Simplify snapshot format detection logic in evaluate_path_info","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-05-11T17:42:47Z","receivedAt":"2009-05-11T17:42:47Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"This issue was caught by perlcritic in harsh severity level noticing\nthat catch variable was used outside conditional thanks to the\nPerl::Critic::Policy::RegularExpressions::ProhibitCaptureWithoutTest\npolicy.  See \"Perl Best Practices\", chapter 12. Regular Expressions,\nsection 12.15. Captured Values:\n\n   Pattern matches that fail never assign anything to $1, $2, etc.,\n   nor do they leave those variables undefined. After an unsuccessful\n   pattern match, the numeric capture variables remain exactly as they\n   were before the match was attempted.\n\nNew version is in my opinion much easier to understand; previous\nversion worked correctly due to the fact that we returned from loop\non first found match.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis is not the only place caught by this policy, but this is the one\nwhere fix was obviously needed to improve readibility of code.\n\nIn _most_ (but not all) of other places we assume that output we parse\nis in given format, and that regexp would always match...\n\n gitweb/gitweb.perl |    7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 097bd18..c72ae10 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -690,9 +690,10 @@ sub evaluate_path_info {\n \t\t# format key itself, with a prepended dot\n \t\twhile (my ($fmt, $opt) = each %known_snapshot_formats) {\n \t\t\tmy $hash = $refname;\n-\t\t\tmy $sfx;\n-\t\t\t$hash =~ s/(\\Q$opt->{'suffix'}\\E|\\Q.$fmt\\E)$//;\n-\t\t\tnext unless $sfx = $1;\n+\t\t\tunless ($hash =~ s/(\\Q$opt->{'suffix'}\\E|\\Q.$fmt\\E)$//) {\n+\t\t\t\tnext;\n+\t\t\t}\n+\t\t\tmy $sfx = $1;\n \t\t\t# a valid suffix was found, so set the snapshot format\n \t\t\t# and reset the hash parameter\n \t\t\t$input_params{'snapshot_format'} = $fmt;\n"},{"id":"113557","messageId":"20090511174253.15152.8987.stgit@localhost.localdomain","threadId":"19292","inReplyTo":"20090511173025.15152.94215.stgit@localhost.localdomain","subject":"[PATCH 4/4] gitweb: Remove unused $hash_base parameter from normalize_link_target","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-05-11T17:45:11Z","receivedAt":"2009-05-11T17:45:11Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"...since it was decided for normalize_link_target to only mangle\npathname, and do not try to check if target is present in $hash_base\ntree, for performance reasons.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis was caugth by Perl::Critic::Policy::Variables::ProhibitReusedNames \n - Do not reuse a variable name in a lexical scope.\n\n(That was probably me that forgot to remove parameter, but I didn't\nsearch for authorship of this fragment.)\n\n gitweb/gitweb.perl |    7 ++-----\n 1 files changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex c72ae10..05702e4 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3339,10 +3339,7 @@ sub git_get_link_target {\n # return target of link relative to top directory (top tree);\n # return undef if it is not possible (including absolute links).\n sub normalize_link_target {\n-\tmy ($link_target, $basedir, $hash_base) = @_;\n-\n-\t# we can normalize symlink target only if $hash_base is provided\n-\treturn unless $hash_base;\n+\tmy ($link_target, $basedir) = @_;\n \n \t# absolute symlinks (beginning with '/') cannot be normalized\n \treturn if (substr($link_target, 0, 1) eq '/');\n@@ -3398,7 +3395,7 @@ sub git_print_tree_entry {\n \t\tif (S_ISLNK(oct $t->{'mode'})) {\n \t\t\tmy $link_target = git_get_link_target($t->{'hash'});\n \t\t\tif ($link_target) {\n-\t\t\t\tmy $norm_target = normalize_link_target($link_target, $basedir, $hash_base);\n+\t\t\t\tmy $norm_target = normalize_link_target($link_target, $basedir);\n \t\t\t\tif (defined $norm_target) {\n \t\t\t\t\tprint \" -> \" .\n \t\t\t\t\t      $cgi->a({-href => href(action=>\"object\", hash_base=>$hash_base,\n"},{"id":"113558","messageId":"cb7bb73a0905111052g4f3f7e23yfbe54bf3cb3e2518@mail.gmail.com","threadId":"19292","inReplyTo":"20090511173950.15152.61267.stgit@localhost.localdomain","subject":"Re: [PATCH 3/4] gitweb: Simplify snapshot format detection logic in evaluate_path_info","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-05-11T17:52:23Z","receivedAt":"2009-05-11T17:52:23Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, May 11, 2009 at 7:42 PM, Jakub Narebski <jnareb@gmail.com> wrote:\n> This issue was caught by perlcritic in harsh severity level noticing\n> that catch variable was used outside conditional thanks to the\n> Perl::Critic::Policy::RegularExpressions::ProhibitCaptureWithoutTest\n> policy.  See \"Perl Best Practices\", chapter 12. Regular Expressions,\n> section 12.15. Captured Values:\n>\n>   Pattern matches that fail never assign anything to $1, $2, etc.,\n>   nor do they leave those variables undefined. After an unsuccessful\n>   pattern match, the numeric capture variables remain exactly as they\n>   were before the match was attempted.\n>\n> New version is in my opinion much easier to understand; previous\n> version worked correctly due to the fact that we returned from loop\n> on first found match.\n>\n> Signed-off-by: Jakub Narebski <jnareb@gmail.com>\n> ---\n> This is not the only place caught by this policy, but this is the one\n> where fix was obviously needed to improve readibility of code.\n>\n> In _most_ (but not all) of other places we assume that output we parse\n> is in given format, and that regexp would always match...\n>\n>  gitweb/gitweb.perl |    7 ++++---\n>  1 files changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 097bd18..c72ae10 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -690,9 +690,10 @@ sub evaluate_path_info {\n>                # format key itself, with a prepended dot\n>                while (my ($fmt, $opt) = each %known_snapshot_formats) {\n>                        my $hash = $refname;\n> -                       my $sfx;\n> -                       $hash =~ s/(\\Q$opt->{'suffix'}\\E|\\Q.$fmt\\E)$//;\n> -                       next unless $sfx = $1;\n> +                       unless ($hash =~ s/(\\Q$opt->{'suffix'}\\E|\\Q.$fmt\\E)$//) {\n> +                               next;\n> +                       }\n> +                       my $sfx = $1;\n>                        # a valid suffix was found, so set the snapshot format\n>                        # and reset the hash parameter\n>                        $input_params{'snapshot_format'} = $fmt;\n\nTotally mea culpa on this, I was optimizing it the wrong way.\n\nAcked-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"}]}