# [PATCH 0/5] gitweb: Miscellaneous improvements, in preparation for caching support 10 messages from 2010-04-24 to 2010-05-04. Participants: Jakub Narebski, Petr Baudis. Thread: https://gitlist.dev/t/23589 ## Jakub Narebski, 2010-04-24 13:46 Subject: [PATCH 0/5] gitweb: Miscellaneous improvements, in preparation for caching support Message-ID: <20100424132255.30511.98829.stgit@localhost.localdomain> URL: https://gitlist.dev/e/20100424132255.30511.98829.stgit%40localhost.localdomain ``` Those are miscellaneous improvements to gitweb, that were part of series that adds output caching support to gitweb. Nevertheless they make sense even outside gitweb caching. >From those "gitweb: Use nonlocal jump instead of 'exit' in die_error" would be useful to add FastCGI (CGI::Fast or FCGI) support to gitweb. "gitweb: Move generating page title to separate subroutine" is pure refactoring currently, but it improves code quality by reducing indent level (nesting) in git_header_html. The patches are independent, beside "gitweb: Add custom error handler using die_error" depending on changes introduced in "gitweb: Use nonlocal jump instead of 'exit' in die_error". --- Jakub Narebski (5): gitweb: Move generating page title to separate subroutine gitweb: Add custom error handler using die_error gitweb: Use nonlocal jump instead of 'exit' in die_error gitweb: href(..., -path_info => 0|1) Export more test-related variables when running external tests gitweb/gitweb.perl | 69 ++++++++++++++++++++++++++++++++++++++-------------- t/test-lib.sh | 3 ++ 2 files changed, 53 insertions(+), 19 deletions(-) mode change 100644 => 100755 t/test-lib.sh -- Jakub Narebski ShadeHawk on #git Poland ``` ## Jakub Narebski, 2010-04-24 13:50 Subject: [PATCH 1/5] Export more test-related variables when running external tests Message-ID: <20100424134640.30511.37449.stgit@localhost.localdomain> URL: https://gitlist.dev/e/20100424134640.30511.37449.stgit%40localhost.localdomain In-Reply-To: <20100424132255.30511.98829.stgit@localhost.localdomain> ``` Add exporting TEST_DIRECTORY and TRASH_DIRECTORY to test_external, for external tests to be able to find test script (and git sources), and to find trash directory (usually with test repository in it). Add also exporting GIT_TEST_LONG, so that external test can skip time-intensive tests unless test is invoked with `--long' option. Signed-off-by: Jakub Narebski --- Currently there is no test that requires those variables; git test suite uses test_external* only in t9700-perl-git.sh. It would be needed for tests of caching for gitweb. Nevertheless I think it is a good change to have... t/test-lib.sh | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) mode change 100644 => 100755 t/test-lib.sh diff --git a/t/test-lib.sh b/t/test-lib.sh old mode 100644 new mode 100755 index c582964..6187328 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -459,6 +459,9 @@ test_external () { # Announce the script to reduce confusion about the # test output that follows. say_color "" " run $test_count: $descr ($*)" + # Export TEST_DIRECTORY, TRASH_DIRECTORY and GIT_TEST_LONG + # to be able to use them in script + export TEST_DIRECTORY TRASH_DIRECTORY GIT_TEST_LONG # Run command; redirect its stderr to &4 as in # test_run_, but keep its stdout on our stdout even in # non-verbose mode. ``` ## Jakub Narebski, 2010-04-24 13:53 Subject: [PATCH 2/5] gitweb: href(..., -path_info => 0|1) Message-ID: <20100424135024.30511.78790.stgit@localhost.localdomain> URL: https://gitlist.dev/e/20100424135024.30511.78790.stgit%40localhost.localdomain In-Reply-To: <20100424132255.30511.98829.stgit@localhost.localdomain> ``` If named boolean option -path_info is passed to href() subroutine, it would use its value to decide whether to generate path_info URL form. If this option is not passed, href() queries 'pathinfo' feature to check whether to generate path_info URL (if generating path_info link is possible at all). href(-replay=>1, -path_info=>0) is meant to be used to generate a key for caching gitweb output; alternate solution would be to use freeze() from Storable (core module) on %input_params hash (or its reference), e.g.: $key = freeze \%input_params; or other serialization of %input_params. While at it document extra options/flags to href(). Signed-off-by: Jakub Narebski --- The commit message explicitly mentions caching support, but in fast this feature is just about having a way to request *canonical* URL (canonical link). gitweb/gitweb.perl | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index c356e95..6cefb09 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -977,6 +977,10 @@ exit; ## ====================================================================== ## action links +# possible values of extra options +# -full => 0|1 - use absolute/full URL ($my_uri/$my_url as base) +# -replay => 1 - start from a current view (replay with modifications) +# -path_info => 0|1 - don't use/use path_info URL (if possible) sub href { my %params = @_; # default is to use -absolute url() i.e. $my_uri @@ -993,7 +997,8 @@ sub href { } my $use_pathinfo = gitweb_check_feature('pathinfo'); - if ($use_pathinfo and defined $params{'project'}) { + if (defined $params{'project'} && + (exists $params{-path_info} ? $params{-path_info} : $use_pathinfo)) { # try to put as many parameters as possible in PATH_INFO: # - project name # - action ``` ## Jakub Narebski, 2010-04-24 13:56 Subject: [PATCH 3/5] gitweb: Use nonlocal jump instead of 'exit' in die_error Message-ID: <20100424135334.30511.26522.stgit@localhost.localdomain> URL: https://gitlist.dev/e/20100424135334.30511.26522.stgit%40localhost.localdomain In-Reply-To: <20100424132255.30511.98829.stgit@localhost.localdomain> ``` Use 'goto DONE' in place of 'exit' to end request processing in die_error() subroutine. While at it, do not end gitweb with 'exit'. This would make it easier in the future to add support or improve support for persistent environments such as FastCGI and mod_perl. It would also make it easier to make use of die_error() as an error handler (for fatalsToBrowser). Perl 5 allows non-local jumps; the restriction is that you cannot jump into a scope. Signed-off-by: Jakub Narebski --- This patch is most independent of introducing caching support to gitweb, and I think it would be good to have nevertheless. gitweb/gitweb.perl | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 6cefb09..ed92dca 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -972,7 +972,8 @@ if ($action !~ m/^(?:opml|project_list|project_index)$/ && die_error(400, "Project needed"); } $actions{$action}->(); -exit; +DONE_GITWEB: +1; ## ====================================================================== ## action links @@ -3432,7 +3433,7 @@ EOF print "\n"; git_footer_html(); - exit; + goto DONE_GITWEB; } ## ---------------------------------------------------------------------- ``` ## Jakub Narebski, 2010-04-24 14:00 Subject: [PATCH 4/5] gitweb: Add custom error handler using die_error Message-ID: <20100424135627.30511.70847.stgit@localhost.localdomain> URL: https://gitlist.dev/e/20100424135627.30511.70847.stgit%40localhost.localdomain In-Reply-To: <20100424132255.30511.98829.stgit@localhost.localdomain> ``` Change the default message for errors (for fatalsToBrowser) to use die_error() subroutine. This way errors (and explicitely calling 'die MESSAGE') would generate 'Internal Server Error' error message. Note that call to set_message is intentionally not put in BEGIN block; we set error handler to use die_error() only after we are sure that we can use it, after all needed variables are set. Due to the fact that error handler set via set_message() subroutine from CGI::Carp (in the fatalsToBrowser case) is called after HTTP headers were already printed (with exception of MOD_PERL), gitweb cannot return 'Status: 500 Internal Server Error'. Thanks to the fact that die_error() no longer uses 'exit', errors would be logged by CGI::Carp, independent on whether default error handler is used, or handle_errors_html which uses die_error is used. Signed-off-by: Jakub Narebski --- Because of the fact that with CGI::Carp::set_message and die_error you cannot return proper HTTP status code, I have decided that caching engine needs support for custom error handler anyway, a la 'on_get_error' and 'on_set_error' in CHI. With this patch most error messages, from fatal errors in gitweb or Perl, should get look of the rest of gitweb. This is good idea even without caching support. gitweb/gitweb.perl | 27 +++++++++++++++++++++++---- 1 files changed, 23 insertions(+), 4 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index ed92dca..e579c14 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -11,7 +11,7 @@ use strict; use warnings; use CGI qw(:standard :escapeHTML -nosticky); use CGI::Util qw(unescape); -use CGI::Carp qw(fatalsToBrowser); +use CGI::Carp qw(fatalsToBrowser set_message); use Encode; use Fcntl ':mode'; use File::Find qw(); @@ -952,6 +952,21 @@ if ($git_avatar eq 'gravatar') { $git_avatar = ''; } +# custom error handler: 'die ' is Internal Server Error +sub handle_errors_html { + my $msg = shift; # it is already HTML escaped + + # to avoid infinite loop where error occurs in die_error, + # change handler to default handler, disabling handle_errors_html + set_message("Error occured when inside die_error:\n$msg"); + + # you cannot jump out of die_error when called as error handler; + # the subroutine set via CGI::Carp::set_message is called _after_ + # HTTP headers are already written, so it cannot write them itself + die_error(undef, undef, $msg, -error_handler => 1, -no_http_header => 1); +} +set_message(\&handle_errors_html); + # dispatch if (!defined $action) { if (defined $hash) { @@ -3167,6 +3182,7 @@ sub blob_contenttype { sub git_header_html { my $status = shift || "200 OK"; my $expires = shift; + my %opts = @_; my $title = "$site_name"; if (defined $project) { @@ -3194,7 +3210,8 @@ sub git_header_html { $content_type = 'text/html'; } print $cgi->header(-type=>$content_type, -charset => 'utf-8', - -status=> $status, -expires => $expires); + -status=> $status, -expires => $expires) + unless ($opts{'-no_http_headers'}); my $mod_perl_version = $ENV{'MOD_PERL'} ? " $ENV{'MOD_PERL'}" : ''; print < @@ -3411,6 +3428,7 @@ sub die_error { my $status = shift || 500; my $error = esc_html(shift) || "Internal Server Error"; my $extra = shift; + my %opts = @_; my %http_responses = ( 400 => '400 Bad Request', @@ -3419,7 +3437,7 @@ sub die_error { 500 => '500 Internal Server Error', 503 => '503 Service Unavailable', ); - git_header_html($http_responses{$status}); + git_header_html($http_responses{$status}, undef, %opts); print <

@@ -3433,7 +3451,8 @@ EOF print "\n"; git_footer_html(); - goto DONE_GITWEB; + goto DONE_GITWEB + unless ($opts{'-error_handler'}); } ## ---------------------------------------------------------------------- ``` ## Jakub Narebski, 2010-04-24 14:01 Subject: [PATCH 5/5] gitweb: Move generating page title to separate subroutine Message-ID: <20100424140019.30511.55366.stgit@localhost.localdomain> URL: https://gitlist.dev/e/20100424140019.30511.55366.stgit%40localhost.localdomain In-Reply-To: <20100424132255.30511.98829.stgit@localhost.localdomain> ``` get_page_title subroutine is currently used only in git_header_html. Nevertheless refactoring title generation allowed to reduce indent level. [It would be used in more than one callsite in the patch adding caching activity indicator to gitweb.] Signed-off-by: Jakub Narebski --- Refactoring. gitweb/gitweb.perl | 32 +++++++++++++++++++------------- 1 files changed, 19 insertions(+), 13 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index e579c14..7d75dc4 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -3179,24 +3179,30 @@ sub blob_contenttype { ## ====================================================================== ## functions printing HTML: header, footer, error page +sub get_page_title { + my $title = to_utf8($site_name); + + return $title unless (defined $project); + $title .= " - " . to_utf8($project); + + return $title unless (defined $action); + $title .= "/$action"; # $action is US-ASCII (7bit ASCII) + + return $title unless (defined $file_name); + $title .= " - " . esc_path($file_name); + if ($action eq "tree" && $file_name !~ m|/$|) { + $title .= "/"; + } + + return $title; +} + sub git_header_html { my $status = shift || "200 OK"; my $expires = shift; my %opts = @_; - my $title = "$site_name"; - if (defined $project) { - $title .= " - " . to_utf8($project); - if (defined $action) { - $title .= "/$action"; - if (defined $file_name) { - $title .= " - " . esc_path($file_name); - if ($action eq "tree" && $file_name !~ m|/$|) { - $title .= "/"; - } - } - } - } + my $title = get_page_title(); my $content_type; # require explicit support from the UA if we are to send the page as # 'application/xhtml+xml', otherwise send it as plain old 'text/html'. ``` ## Petr Baudis, 2010-04-25 09:20 Subject: Re: [PATCH 0/5] gitweb: Miscellaneous improvements, in preparation for caching support Message-ID: <20100425092035.GO3563@machine.or.cz> URL: https://gitlist.dev/e/20100425092035.GO3563%40machine.or.cz In-Reply-To: <20100424132255.30511.98829.stgit@localhost.localdomain> ``` On Sat, Apr 24, 2010 at 03:46:25PM +0200, Jakub Narebski wrote: > Jakub Narebski (5): > gitweb: Move generating page title to separate subroutine > gitweb: Add custom error handler using die_error > gitweb: Use nonlocal jump instead of 'exit' in die_error > gitweb: href(..., -path_info => 0|1) > Export more test-related variables when running external tests > > gitweb/gitweb.perl | 69 ++++++++++++++++++++++++++++++++++++++-------------- > t/test-lib.sh | 3 ++ > 2 files changed, 53 insertions(+), 19 deletions(-) > mode change 100644 => 100755 t/test-lib.sh Except the last one, I don't think the patches bring something useful in on their own so ordinarily they would probably have to be a part of some series that makes use of them, but there's no harm done either. Acked-by: Petr Baudis ``` ## Jakub Narebski, 2010-04-25 11:45 Subject: Re: [PATCH 0/5] gitweb: Miscellaneous improvements, in preparation for caching support Message-ID: <201004251346.01183.jnareb@gmail.com> URL: https://gitlist.dev/e/201004251346.01183.jnareb%40gmail.com In-Reply-To: <20100425092035.GO3563@machine.or.cz> ``` Petr Baudis wrote: > On Sat, Apr 24, 2010 at 03:46:25PM +0200, Jakub Narebski wrote: > > Jakub Narebski (5): > > gitweb: Move generating page title to separate subroutine > > gitweb: Add custom error handler using die_error > > gitweb: Use nonlocal jump instead of 'exit' in die_error > > gitweb: href(..., -path_info => 0|1) > > Export more test-related variables when running external tests > > > > gitweb/gitweb.perl | 69 ++++++++++++++++++++++++++++++++++++++-------------- > > t/test-lib.sh | 3 ++ > > 2 files changed, 53 insertions(+), 19 deletions(-) > > mode change 100644 => 100755 t/test-lib.sh Ooops, the mode change was accidental. > > Except the last one, I don't think the patches bring something useful > in on their own so ordinarily they would probably have to be a part of > some series that makes use of them, but there's no harm done either. Actually "gitweb: Use nonlocal jump instead of 'exit' in die_error" could in theory improve performance of gitweb on mod_perl, but I have not benchmarked it... -- Jakub Narebski Poland ``` ## Jakub Narebski, 2010-04-25 11:47 Subject: Re: [PATCH 1/5] Export more test-related variables when running external tests Message-ID: <201004251347.11102.jnareb@gmail.com> URL: https://gitlist.dev/e/201004251347.11102.jnareb%40gmail.com In-Reply-To: <20100424134640.30511.37449.stgit@localhost.localdomain> ``` > t/test-lib.sh | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > mode change 100644 => 100755 t/test-lib.sh > > diff --git a/t/test-lib.sh b/t/test-lib.sh > old mode 100644 > new mode 100755 Ooops, I'm sorry, the mode change is accidental. -- Jakub Narebski Poland ``` ## Jakub Narebski, 2010-05-04 11:39 Subject: Re: [PATCH 3/5] gitweb: Use nonlocal jump instead of 'exit' in die_error Message-ID: <201005041339.26192.jnareb@gmail.com> URL: https://gitlist.dev/e/201005041339.26192.jnareb%40gmail.com In-Reply-To: <20100424135334.30511.26522.stgit@localhost.localdomain> ``` On Sat, 24 Apr 2010, Jakub Narebski wrote: > Use 'goto DONE' in place of 'exit' to end request processing in > die_error() subroutine. While at it, do not end gitweb with 'exit'. > > This would make it easier in the future to add support or improve > support for persistent environments such as FastCGI and mod_perl. > It would also make it easier to make use of die_error() as an error > handler (for fatalsToBrowser). > > Perl 5 allows non-local jumps; the restriction is that you cannot jump > into a scope. > > Signed-off-by: Jakub Narebski > --- > This patch is most independent of introducing caching support to > gitweb, and I think it would be good to have nevertheless. >From *very preliminary* benchmarks it looks like this change improves gitweb performance for multiple connections when served from mod_perl. The results of "ab -n 10 -c ", where X = 1, 2, 5, degrades less with the number of concurrent requests when this patch is applied than without this patch. -- Jakub Narebski Poland ```