[PATCH 0/5] gitweb: Miscellaneous improvements, in preparation for caching support

[PATCH 0/5] gitweb: Miscellaneous improvements, in preparation for caching support

[Jakub Narebski]

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

[PATCH 1/5] Export more test-related variables when running external tests

[Jakub Narebski]

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

[PATCH 2/5] gitweb: href(..., -path_info => 0|1)

[Jakub Narebski]

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

[PATCH 3/5] gitweb: Use nonlocal jump instead of 'exit' in die_error

[Jakub Narebski]

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 <jnareb@gmail.com>
---
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 "</div>\n";
 
 	git_footer_html();
-	exit;
+	goto DONE_GITWEB;
 }
 
 ## ----------------------------------------------------------------------

[PATCH 4/5] gitweb: Add custom error handler using die_error

[Jakub Narebski]

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 <jnareb@gmail.com>
---
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 <message>' 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 <<EOF;
 <?xml version="1.0" encoding="utf-8"?>
@@ -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 <<EOF;
 <div class="page_body">
 <br /><br />
@@ -3433,7 +3451,8 @@ EOF
 	print "</div>\n";
 
 	git_footer_html();
-	goto DONE_GITWEB;
+	goto DONE_GITWEB
+		unless ($opts{'-error_handler'});
 }
 
 ## ----------------------------------------------------------------------

[PATCH 5/5] gitweb: Move generating page title to separate subroutine

[Jakub Narebski]

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

Re: [PATCH 0/5] gitweb: Miscellaneous improvements, in preparation for caching support

[Petr Baudis]

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 <pasky@suse.cz>

Re: [PATCH 0/5] gitweb: Miscellaneous improvements, in preparation for caching support

[Jakub Narebski]

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

Re: [PATCH 1/5] Export more test-related variables when running external tests

[Jakub Narebski]

>  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

Re: [PATCH 3/5] gitweb: Use nonlocal jump instead of 'exit' in die_error

[Jakub Narebski]

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 <jnareb@gmail.com>
> ---
> 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 <X> <URL>", 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