{"thread":{"id":"23589","subject":"[PATCH 0/5] gitweb: Miscellaneous improvements, in preparation for caching support","startedAt":"2010-04-24T13:46:25Z","lastAt":"2010-05-04T11:39:23Z","messageCount":10,"participants":["Jakub Narebski","Petr Baudis"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"140273","messageId":"20100424132255.30511.98829.stgit@localhost.localdomain","threadId":"23589","inReplyTo":null,"subject":"[PATCH 0/5] gitweb: Miscellaneous improvements, in preparation for caching support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-24T13:46:25Z","receivedAt":"2010-04-24T13:46:25Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Those are miscellaneous improvements to gitweb, that were part of\nseries that adds output caching support to gitweb.  Nevertheless they\nmake sense even outside gitweb caching.\n\n>From those \"gitweb: Use nonlocal jump instead of 'exit' in die_error\"\nwould be useful to add FastCGI (CGI::Fast or FCGI) support to gitweb.\n\n\"gitweb: Move generating page title to separate subroutine\" is pure\nrefactoring currently, but it improves code quality by reducing indent\nlevel (nesting) in git_header_html.\n\nThe patches are independent, beside \"gitweb: Add custom error handler\nusing die_error\" depending on changes introduced in \"gitweb: Use\nnonlocal jump instead of 'exit' in die_error\".\n\n---\nJakub Narebski (5):\n      gitweb: Move generating page title to separate subroutine\n      gitweb: Add custom error handler using die_error\n      gitweb: Use nonlocal jump instead of 'exit' in die_error\n      gitweb: href(..., -path_info => 0|1)\n      Export more test-related variables when running external tests\n\n gitweb/gitweb.perl |   69 ++++++++++++++++++++++++++++++++++++++--------------\n t/test-lib.sh      |    3 ++\n 2 files changed, 53 insertions(+), 19 deletions(-)\n mode change 100644 => 100755 t/test-lib.sh\n\n-- \nJakub Narebski\nShadeHawk on #git\nPoland\n"},{"id":"140274","messageId":"20100424134640.30511.37449.stgit@localhost.localdomain","threadId":"23589","inReplyTo":"20100424132255.30511.98829.stgit@localhost.localdomain","subject":"[PATCH 1/5] Export more test-related variables when running external tests","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-24T13:50:09Z","receivedAt":"2010-04-24T13:50:09Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Add exporting TEST_DIRECTORY and TRASH_DIRECTORY to test_external, for\nexternal tests to be able to find test script (and git sources), and\nto find trash directory (usually with test repository in it).\n\nAdd also exporting GIT_TEST_LONG, so that external test can skip\ntime-intensive tests unless test is invoked with `--long' option.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nCurrently there is no test that requires those variables; git test\nsuite uses test_external* only in t9700-perl-git.sh.  It would be\nneeded for tests of caching for gitweb.\n\nNevertheless I think it is a good change to have...\n\n t/test-lib.sh |    3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n mode change 100644 => 100755 t/test-lib.sh\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nold mode 100644\nnew mode 100755\nindex c582964..6187328\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -459,6 +459,9 @@ test_external () {\n \t\t# Announce the script to reduce confusion about the\n \t\t# test output that follows.\n \t\tsay_color \"\" \" run $test_count: $descr ($*)\"\n+\t\t# Export TEST_DIRECTORY, TRASH_DIRECTORY and GIT_TEST_LONG\n+\t\t# to be able to use them in script\n+\t\texport TEST_DIRECTORY TRASH_DIRECTORY GIT_TEST_LONG\n \t\t# Run command; redirect its stderr to &4 as in\n \t\t# test_run_, but keep its stdout on our stdout even in\n \t\t# non-verbose mode.\n"},{"id":"140275","messageId":"20100424135024.30511.78790.stgit@localhost.localdomain","threadId":"23589","inReplyTo":"20100424132255.30511.98829.stgit@localhost.localdomain","subject":"[PATCH 2/5] gitweb: href(..., -path_info => 0|1)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-24T13:53:19Z","receivedAt":"2010-04-24T13:53:19Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"If named boolean option -path_info is passed to href() subroutine, it\nwould use its value to decide whether to generate path_info URL form.\nIf this option is not passed, href() queries 'pathinfo' feature to\ncheck whether to generate path_info URL (if generating path_info link\nis possible at all).\n\nhref(-replay=>1, -path_info=>0) is meant to be used to generate a key\nfor caching gitweb output; alternate solution would be to use freeze()\nfrom Storable (core module) on %input_params hash (or its reference),\ne.g.:\n  $key = freeze \\%input_params;\nor other serialization of %input_params.\n\nWhile at it document extra options/flags to href().\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThe commit message explicitly mentions caching support, but in fast\nthis feature is just about having a way to request *canonical* URL\n(canonical link).\n\n gitweb/gitweb.perl |    7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex c356e95..6cefb09 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -977,6 +977,10 @@ exit;\n ## ======================================================================\n ## action links\n \n+# possible values of extra options\n+# -full => 0|1      - use absolute/full URL ($my_uri/$my_url as base)\n+# -replay => 1      - start from a current view (replay with modifications)\n+# -path_info => 0|1 - don't use/use path_info URL (if possible)\n sub href {\n \tmy %params = @_;\n \t# default is to use -absolute url() i.e. $my_uri\n@@ -993,7 +997,8 @@ sub href {\n \t}\n \n \tmy $use_pathinfo = gitweb_check_feature('pathinfo');\n-\tif ($use_pathinfo and defined $params{'project'}) {\n+\tif (defined $params{'project'} &&\n+\t    (exists $params{-path_info} ? $params{-path_info} : $use_pathinfo)) {\n \t\t# try to put as many parameters as possible in PATH_INFO:\n \t\t#   - project name\n \t\t#   - action\n"},{"id":"140276","messageId":"20100424135334.30511.26522.stgit@localhost.localdomain","threadId":"23589","inReplyTo":"20100424132255.30511.98829.stgit@localhost.localdomain","subject":"[PATCH 3/5] gitweb: Use nonlocal jump instead of 'exit' in die_error","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-24T13:56:13Z","receivedAt":"2010-04-24T13:56:13Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Use 'goto DONE' in place of 'exit' to end request processing in\ndie_error() subroutine.  While at it, do not end gitweb with 'exit'.\n\nThis would make it easier in the future to add support or improve\nsupport for persistent environments such as FastCGI and mod_perl.\nIt would also make it easier to make use of die_error() as an error\nhandler (for fatalsToBrowser).\n\nPerl 5 allows non-local jumps; the restriction is that you cannot jump\ninto a scope.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis patch is most independent of introducing caching support to\ngitweb, and I think it would be good to have nevertheless.\n\n gitweb/gitweb.perl |    5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 6cefb09..ed92dca 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -972,7 +972,8 @@ if ($action !~ m/^(?:opml|project_list|project_index)$/ &&\n \tdie_error(400, \"Project needed\");\n }\n $actions{$action}->();\n-exit;\n+DONE_GITWEB:\n+1;\n \n ## ======================================================================\n ## action links\n@@ -3432,7 +3433,7 @@ EOF\n \tprint \"</div>\\n\";\n \n \tgit_footer_html();\n-\texit;\n+\tgoto DONE_GITWEB;\n }\n \n ## ----------------------------------------------------------------------\n"},{"id":"140277","messageId":"20100424135627.30511.70847.stgit@localhost.localdomain","threadId":"23589","inReplyTo":"20100424132255.30511.98829.stgit@localhost.localdomain","subject":"[PATCH 4/5] gitweb: Add custom error handler using die_error","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-24T14:00:04Z","receivedAt":"2010-04-24T14:00:04Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Change the default message for errors (for fatalsToBrowser) to use\ndie_error() subroutine.  This way errors (and explicitely calling 'die\nMESSAGE') would generate 'Internal Server Error' error message.\n\nNote that call to set_message is intentionally not put in BEGIN block;\nwe set error handler to use die_error() only after we are sure that we\ncan use it, after all needed variables are set.\n\nDue to the fact that error handler set via set_message() subroutine\nfrom CGI::Carp (in the fatalsToBrowser case) is called after HTTP\nheaders were already printed (with exception of MOD_PERL), gitweb\ncannot return 'Status: 500 Internal Server Error'.\n\nThanks to the fact that die_error() no longer uses 'exit', errors\nwould be logged by CGI::Carp, independent on whether default error\nhandler is used, or handle_errors_html which uses die_error is used.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nBecause of the fact that with CGI::Carp::set_message and die_error\nyou cannot return proper HTTP status code, I have decided that\ncaching engine needs support for custom error handler anyway, a la\n'on_get_error' and 'on_set_error' in CHI.\n\nWith this patch most error messages, from fatal errors in gitweb or\nPerl, should get look of the rest of gitweb.  This is good idea even\nwithout caching support.\n\n gitweb/gitweb.perl |   27 +++++++++++++++++++++++----\n 1 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex ed92dca..e579c14 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -11,7 +11,7 @@ use strict;\n use warnings;\n use CGI qw(:standard :escapeHTML -nosticky);\n use CGI::Util qw(unescape);\n-use CGI::Carp qw(fatalsToBrowser);\n+use CGI::Carp qw(fatalsToBrowser set_message);\n use Encode;\n use Fcntl ':mode';\n use File::Find qw();\n@@ -952,6 +952,21 @@ if ($git_avatar eq 'gravatar') {\n \t$git_avatar = '';\n }\n \n+# custom error handler: 'die <message>' is Internal Server Error\n+sub handle_errors_html {\n+\tmy $msg = shift; # it is already HTML escaped\n+\n+\t# to avoid infinite loop where error occurs in die_error,\n+\t# change handler to default handler, disabling handle_errors_html\n+\tset_message(\"Error occured when inside die_error:\\n$msg\");\n+\n+\t# you cannot jump out of die_error when called as error handler;\n+\t# the subroutine set via CGI::Carp::set_message is called _after_\n+\t# HTTP headers are already written, so it cannot write them itself\n+\tdie_error(undef, undef, $msg, -error_handler => 1, -no_http_header => 1);\n+}\n+set_message(\\&handle_errors_html);\n+\n # dispatch\n if (!defined $action) {\n \tif (defined $hash) {\n@@ -3167,6 +3182,7 @@ sub blob_contenttype {\n sub git_header_html {\n \tmy $status = shift || \"200 OK\";\n \tmy $expires = shift;\n+\tmy %opts = @_;\n \n \tmy $title = \"$site_name\";\n \tif (defined $project) {\n@@ -3194,7 +3210,8 @@ sub git_header_html {\n \t\t$content_type = 'text/html';\n \t}\n \tprint $cgi->header(-type=>$content_type, -charset => 'utf-8',\n-\t                   -status=> $status, -expires => $expires);\n+\t                   -status=> $status, -expires => $expires)\n+\t\tunless ($opts{'-no_http_headers'});\n \tmy $mod_perl_version = $ENV{'MOD_PERL'} ? \" $ENV{'MOD_PERL'}\" : '';\n \tprint <<EOF;\n <?xml version=\"1.0\" encoding=\"utf-8\"?>\n@@ -3411,6 +3428,7 @@ sub die_error {\n \tmy $status = shift || 500;\n \tmy $error = esc_html(shift) || \"Internal Server Error\";\n \tmy $extra = shift;\n+\tmy %opts = @_;\n \n \tmy %http_responses = (\n \t\t400 => '400 Bad Request',\n@@ -3419,7 +3437,7 @@ sub die_error {\n \t\t500 => '500 Internal Server Error',\n \t\t503 => '503 Service Unavailable',\n \t);\n-\tgit_header_html($http_responses{$status});\n+\tgit_header_html($http_responses{$status}, undef, %opts);\n \tprint <<EOF;\n <div class=\"page_body\">\n <br /><br />\n@@ -3433,7 +3451,8 @@ EOF\n \tprint \"</div>\\n\";\n \n \tgit_footer_html();\n-\tgoto DONE_GITWEB;\n+\tgoto DONE_GITWEB\n+\t\tunless ($opts{'-error_handler'});\n }\n \n ## ----------------------------------------------------------------------\n"},{"id":"140278","messageId":"20100424140019.30511.55366.stgit@localhost.localdomain","threadId":"23589","inReplyTo":"20100424132255.30511.98829.stgit@localhost.localdomain","subject":"[PATCH 5/5] gitweb: Move generating page title to separate subroutine","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-24T14:01:10Z","receivedAt":"2010-04-24T14:01:10Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"get_page_title subroutine is currently used only in git_header_html.\nNevertheless refactoring title generation allowed to reduce indent\nlevel.\n\n[It would be used in more than one callsite in the patch adding\ncaching activity indicator to gitweb.]\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nRefactoring.\n\n gitweb/gitweb.perl |   32 +++++++++++++++++++-------------\n 1 files changed, 19 insertions(+), 13 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e579c14..7d75dc4 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3179,24 +3179,30 @@ sub blob_contenttype {\n ## ======================================================================\n ## functions printing HTML: header, footer, error page\n \n+sub get_page_title {\n+\tmy $title = to_utf8($site_name);\n+\n+\treturn $title unless (defined $project);\n+\t$title .= \" - \" . to_utf8($project);\n+\n+\treturn $title unless (defined $action);\n+\t$title .= \"/$action\"; # $action is US-ASCII (7bit ASCII)\n+\n+\treturn $title unless (defined $file_name);\n+\t$title .= \" - \" . esc_path($file_name);\n+\tif ($action eq \"tree\" && $file_name !~ m|/$|) {\n+\t\t$title .= \"/\";\n+\t}\n+\n+\treturn $title;\n+}\n+\n sub git_header_html {\n \tmy $status = shift || \"200 OK\";\n \tmy $expires = shift;\n \tmy %opts = @_;\n \n-\tmy $title = \"$site_name\";\n-\tif (defined $project) {\n-\t\t$title .= \" - \" . to_utf8($project);\n-\t\tif (defined $action) {\n-\t\t\t$title .= \"/$action\";\n-\t\t\tif (defined $file_name) {\n-\t\t\t\t$title .= \" - \" . esc_path($file_name);\n-\t\t\t\tif ($action eq \"tree\" && $file_name !~ m|/$|) {\n-\t\t\t\t\t$title .= \"/\";\n-\t\t\t\t}\n-\t\t\t}\n-\t\t}\n-\t}\n+\tmy $title = get_page_title();\n \tmy $content_type;\n \t# require explicit support from the UA if we are to send the page as\n \t# 'application/xhtml+xml', otherwise send it as plain old 'text/html'.\n"},{"id":"140331","messageId":"20100425092035.GO3563@machine.or.cz","threadId":"23589","inReplyTo":"20100424132255.30511.98829.stgit@localhost.localdomain","subject":"Re: [PATCH 0/5] gitweb: Miscellaneous improvements, in preparation for caching support","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2010-04-25T09:20:35Z","receivedAt":"2010-04-25T09:20:35Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Sat, Apr 24, 2010 at 03:46:25PM +0200, Jakub Narebski wrote:\n> Jakub Narebski (5):\n>       gitweb: Move generating page title to separate subroutine\n>       gitweb: Add custom error handler using die_error\n>       gitweb: Use nonlocal jump instead of 'exit' in die_error\n>       gitweb: href(..., -path_info => 0|1)\n>       Export more test-related variables when running external tests\n> \n>  gitweb/gitweb.perl |   69 ++++++++++++++++++++++++++++++++++++++--------------\n>  t/test-lib.sh      |    3 ++\n>  2 files changed, 53 insertions(+), 19 deletions(-)\n>  mode change 100644 => 100755 t/test-lib.sh\n\nExcept the last one, I don't think the patches bring something useful\nin on their own so ordinarily they would probably have to be a part of\nsome series that makes use of them, but there's no harm done either.\n\nAcked-by: Petr Baudis <pasky@suse.cz>\n"},{"id":"140335","messageId":"201004251346.01183.jnareb@gmail.com","threadId":"23589","inReplyTo":"20100425092035.GO3563@machine.or.cz","subject":"Re: [PATCH 0/5] gitweb: Miscellaneous improvements, in preparation for caching support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-25T11:45:57Z","receivedAt":"2010-04-25T11:45:57Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Petr Baudis wrote:\n> On Sat, Apr 24, 2010 at 03:46:25PM +0200, Jakub Narebski wrote:\n> > Jakub Narebski (5):\n> >       gitweb: Move generating page title to separate subroutine\n> >       gitweb: Add custom error handler using die_error\n> >       gitweb: Use nonlocal jump instead of 'exit' in die_error\n> >       gitweb: href(..., -path_info => 0|1)\n> >       Export more test-related variables when running external tests\n> > \n> >  gitweb/gitweb.perl |   69 ++++++++++++++++++++++++++++++++++++++--------------\n> >  t/test-lib.sh      |    3 ++\n> >  2 files changed, 53 insertions(+), 19 deletions(-)\n\n> >  mode change 100644 => 100755 t/test-lib.sh\n\nOoops, the mode change was accidental.\n\n> \n> Except the last one, I don't think the patches bring something useful\n> in on their own so ordinarily they would probably have to be a part of\n> some series that makes use of them, but there's no harm done either.\n\nActually \"gitweb: Use nonlocal jump instead of 'exit' in die_error\"\ncould in theory improve performance of gitweb on mod_perl, but I have\nnot benchmarked it...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"140334","messageId":"201004251347.11102.jnareb@gmail.com","threadId":"23589","inReplyTo":"20100424134640.30511.37449.stgit@localhost.localdomain","subject":"Re: [PATCH 1/5] Export more test-related variables when running external tests","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-25T11:47:10Z","receivedAt":"2010-04-25T11:47:10Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\n>  t/test-lib.sh |    3 +++\n>  1 files changed, 3 insertions(+), 0 deletions(-)\n>  mode change 100644 => 100755 t/test-lib.sh\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> old mode 100644\n> new mode 100755\n\nOoops, I'm sorry, the mode change is accidental.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"140902","messageId":"201005041339.26192.jnareb@gmail.com","threadId":"23589","inReplyTo":"20100424135334.30511.26522.stgit@localhost.localdomain","subject":"Re: [PATCH 3/5] gitweb: Use nonlocal jump instead of 'exit' in die_error","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-05-04T11:39:23Z","receivedAt":"2010-05-04T11:39:23Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 24 Apr 2010, Jakub Narebski wrote:\n\n> Use 'goto DONE' in place of 'exit' to end request processing in\n> die_error() subroutine.  While at it, do not end gitweb with 'exit'.\n> \n> This would make it easier in the future to add support or improve\n> support for persistent environments such as FastCGI and mod_perl.\n> It would also make it easier to make use of die_error() as an error\n> handler (for fatalsToBrowser).\n> \n> Perl 5 allows non-local jumps; the restriction is that you cannot jump\n> into a scope.\n> \n> Signed-off-by: Jakub Narebski <jnareb@gmail.com>\n> ---\n> This patch is most independent of introducing caching support to\n> gitweb, and I think it would be good to have nevertheless.\n\n>From *very preliminary* benchmarks it looks like this change improves \ngitweb performance for multiple connections when served from mod_perl.\n\nThe results of \"ab -n 10 -c <X> <URL>\", where X = 1, 2, 5, degrades\nless with the number of concurrent requests when this patch is applied \nthan without this patch.\n\n-- \nJakub Narebski\nPoland\n"}]}