threads / patch / 23589

patch, 5 partsgitweb: Miscellaneous improvements, in preparation for caching support

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

## tl;dr

10 messages between Apr 24, 2010 and May 4, 2010. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

Jakub Narebski· Apr 24, 2010, 13:46 UTC · lore

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· Apr 24, 2010, 13:50 UTC · re: Jakub Narebski · lore

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

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
Show changes to t/test-lib.sh +3 −0
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· Apr 25, 2010, 11:47 UTC · re: Jakub Narebski · lore

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

Show 7 quoted lines
>  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· Apr 24, 2010, 13:53 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +6 −1
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· Apr 24, 2010, 13:56 UTC · re: Jakub Narebski · lore

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

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

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

On Sat, 24 Apr 2010, Jakub Narebski wrote:
Show 15 quoted lines
> 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
Jakub Narebski· Apr 24, 2010, 14:00 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +23 −4
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'});
 }
 
 ## ----------------------------------------------------------------------
Jakub Narebski· Apr 24, 2010, 14:01 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +19 −13
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· Apr 25, 2010, 09:20 UTC · re: Jakub Narebski · lore

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

On Sat, Apr 24, 2010 at 03:46:25PM +0200, Jakub Narebski wrote:
Show 11 quoted lines
> 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>
Jakub Narebski· Apr 25, 2010, 11:45 UTC · re: Petr Baudis · lore

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

Petr Baudis wrote:
Show 11 quoted lines
> 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

← back to recent threads