{"thread":{"id":"26012","subject":"[PATCH 01/18] gitweb: Prepare for splitting gitweb","startedAt":"2010-12-09T21:57:06Z","lastAt":"2010-12-12T15:17:04Z","messageCount":60,"participants":["John 'Warthog9' Hawley","Jakub Narebski","Junio C Hamano","J.H."],"isPatch":true,"patchVersion":1,"patchTotal":18},"messages":[{"id":"157754","messageId":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":null,"subject":"[PATCH 00/18] Gitweb caching v8","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:06Z","receivedAt":"2010-12-09T21:57:06Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"Afternoon everyone,\n\n(Afternoon is like morning, right?)\n \nThis is the latest incarnation of gitweb w/ caching. Per the general\nconsensus and requests from the recent Gittogether I'm re-submitting \nmy patches.\n\nBunch of re-works in the code, and several requested features. Sadly the\npatch series has balloned as I've been adding things. It was 3-4 patches,\nit's now 18. This is based on top of Jakub's v7.2 patch series, but\nit should be more or less clean now.\n\nAs such there was a bunch of changes that I needed to do to Jakub's tree\nwhich are indicated in the series. Why did I do them up as separate things?\nMainly there's a bunch of history that's getting lost right now between\ngoing back and forth, and I wanted to have clear patches to discuss\nshould further discussion be needed.\n\nThis still differs, by two patches, from whats in production on kernel.org.\nIt's missing the index page git:// link, and kernel.org and kernel.org also\nhas the forced version matching. As a note I'll probably let this stew\nanother day or so on kernel.org and then I'll push it into the Fedora update\nstream, as there's a couple of things in this patch series that would be \ngood for them to have.\n\nThere is one additional script I've written that the Fedora folks are using,\nand that might be useful to include, which is an 'offline' cache file generator.\nIt basically wraps gitweb.cgi and at the end moves the cache file into the right\nplace. The Fedora folks were finding it took hours to generate their front\npage, and that doing a background generation almost never completed (due to \nprocess death). This was a simple way to handle that. If people would like\nI can add it in as an additional patch.\n\nv8:\n\t- Reverting several changes from Jakub's change set that make no sense\n - is_cacheable changed to always return true - nothing special about\n blame or blame_incremental as far as the caching engine is concerned\n - Reverted config file change \"caching_enabled\" back to \"cache_enable\" as this\n config file option is already in the wild in production code, as are all\n current gitweb-caching configuration variables.\n - Reverted change to reset_output as\n open STDOUT, \">&\", \\*STDOUT_REAL;\n causes assertion failures:\n Assertion !((((s->var)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVLV)) failed: file \"scalar.xs\", line 49 at gitweb.cgi line 1221.\n if we encounter an error *BEFORE* we've ever changed the output.\n - Cleanups there were indirectly mentioned by Jakub\n - Elimination of anything even remotely looking like duplicate code\n - Creation of isBinaryAction() and isFeedAction()\n - Adding in blacklist of \"dumb\" clients for purposes of downloading content\n - Added more explicit disablement of \"Generating...\" page\n - Added better error handling\n - Creation of .err file in the cache directory\n - Trap STDERR output into $output_err as this was spewing data prior to any header information being sent\n - Added hidden field in footer for url & hash of url, which is extremely useful for debugging\n\nv7:\n\t- Rework output system, now central STDOUT redirect\n\t- Various fixes to caching brought in from existing\n\t running system\n\nv6:\n\t- Never saw the light of day\n\t- Various testing, and reworks.\n\nv5:\n\t- Missed a couple of things that were in my local tree, and\n\t added them back in.\n\t- Split up the die_error and the version matching patch\n\t- Set version matching to be on by default - otherwise this\n\t really is code that will never get checked, or at best\n\t enabled by default by distributions\n\t- Added a minor code cleanup with respect to $site_header\n\t that was already in my tree\n\t- Applied against a more recent git tree vs. 1.6.6-rc2\n\t- Removed breakout patch for now (did that in v4 actually)\n\t and will deal with that separately \n\n\thttp://git.kernel.org/?p=git/warthog9/gitweb.git;a=shortlog;h=refs/heads/gitweb-ml-v5\n\nv4:\n\t- major re-working of the caching layer to use file handle\n\t redirection instead of buffering output\n\t- other minor improvements\n\n\thttp://git.kernel.org/?p=git/warthog9/gitweb.git;a=shortlog;h=refs/heads/gitweb-ml-v4\nv3:\n\t- various minor re-works based on mailing list feedback,\n\t this series was not sent to the mailing list.\nv2:\n\t- Better breakout\n\t- You can actually disable the cache now\n\n- John 'Warthog9' Hawley \n\n\nJakub Narebski (2):\n gitweb: Prepare for splitting gitweb\n gitweb: Minimal testing of gitweb caching\n\nJohn 'Warthog9' Hawley (16):\n gitweb: add output buffering and associated functions\n gitweb: File based caching layer (from git.kernel.org)\n gitweb: Regression fix concerning binary output of files\n gitweb: Add more explicit means of disabling 'Generating...' page\n gitweb: Revert back to $cache_enable vs. $caching_enabled\n gitweb: Change is_cacheable() to return true always\n gitweb: Revert reset_output() back to original code\n gitweb: Adding isBinaryAction() and isFeedAction() to determine the\n action type\n gitweb: add isDumbClient() check\n gitweb: Change file handles (in caching) to lexical variables as\n opposed to globs\n gitweb: Add commented url & url hash to page footer\n gitweb: add print_transient_header() function for central header\n printing\n gitweb: Add show_warning() to display an immediate warning, with\n refresh\n gitweb: When changing output (STDOUT) change STDERR as well\n gitweb: Prepare for cached error pages & better error page handling\n gitweb: Add better error handling for gitweb caching\n\n gitweb/Makefile | 20 +-\n gitweb/gitweb.perl | 176 ++++++++++-\n gitweb/lib/cache.pl | 488 +++++++++++++++++++++++++++++\n gitweb/static/gitweb.css | 6 +\n t/gitweb-lib.sh | 16 +\n t/t9500-gitweb-standalone-no-errors.sh | 20 ++\n t/t9501-gitweb-standalone-http-status.sh | 13 +\n t/t9502-gitweb-standalone-parse-output.sh | 33 ++\n 8 files changed, 762 insertions(+), 10 deletions(-)\n create mode 100644 gitweb/lib/cache.pl\n mode change 100644 => 100755 t/gitweb-lib.sh\n\n-- \n1.7.2.3\n"},{"id":"157739","messageId":"1291931844-28454-2-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 01/18] gitweb: Prepare for splitting gitweb","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:07Z","receivedAt":"2010-12-09T21:57:07Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"From: Jakub Narebski \n\nPrepare gitweb for having been split into modules that are to be\ninstalled alongside gitweb in 'lib/' subdirectory, by adding\n\n use lib __DIR__.'/lib';\n\nto gitweb.perl (to main gitweb script), and preparing for putting\nmodules (relative path) in $(GITWEB_MODULES) in gitweb/Makefile.\n\nThis preparatory work allows to add new module to gitweb by simply\nadding\n\n GITWEB_MODULES += \n\nto gitweb/Makefile (assuming that the module is in 'gitweb/lib/'\ndirectory).\n\nWhile at it pass GITWEBLIBDIR in addition to GITWEB_TEST_INSTALLED\nto test instaleed version of gitweb and installed version of modules\n(for tests which check individual (sub)modules).\n\n\nUsing __DIR__ from Dir::Self module (not in core, that's why currently\ngitweb includes excerpt of code from Dir::Self defining __DIR__) was\nchosen over using FindBin-based solution (in core since perl 5.00307,\nwhile gitweb itself requires at least perl 5.8.0) because FindBin uses\nBEGIN block, which is a problem under mod_perl and other persistent\nPerl environments (thought there are workarounds).\n\nAt Pavan Kumar Sankara suggestion gitweb/Makefile uses\n\n install [OPTION]... SOURCE... DIRECTORY\n\nformat (2nd format) with single SOURCE rather than\n\n install [OPTION]... SOURCE DEST\n\nformat (1st format) because of security reasons (race conditions).\nModern GNU install has `-T' / `--no-target-directory' option, but we\ncannot rely that the $(INSTALL) we are using supports this option.\n\nThe install-modules target in gitweb/Makefile uses shell 'for' loop,\ninstead of make's $(foreach) function, to avoid possible problem with\ngenerating a command line that exceeded the maximum argument list\nlength.\n\nSigned-off-by: Jakub Narebski \n---\n gitweb/Makefile | 17 +++++++++++++++--\n gitweb/gitweb.perl | 8 ++++++++\n 2 files changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/Makefile b/gitweb/Makefile\nindex 0a6ac00..f9e32eb 100644\n--- a/gitweb/Makefile\n+++ b/gitweb/Makefile\n@@ -57,6 +57,7 @@ PERL_PATH ?= /usr/bin/perl\n bindir_SQ = $(subst ','\\'',$(bindir))#'\n gitwebdir_SQ = $(subst ','\\'',$(gitwebdir))#'\n gitwebstaticdir_SQ = $(subst ','\\'',$(gitwebdir)/static)#'\n+gitweblibdir_SQ = $(subst ','\\'',$(gitwebdir)/lib)#'\n SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))#'\n PERL_PATH_SQ = $(subst ','\\'',$(PERL_PATH))#'\n DESTDIR_SQ = $(subst ','\\'',$(DESTDIR))#'\n@@ -153,20 +154,32 @@ test:\n \n test-installed:\n \tGITWEB_TEST_INSTALLED='$(DESTDIR_SQ)$(gitwebdir_SQ)' \\\n+\tGITWEBLIBDIR='$(DESTDIR_SQ)$(gitweblibdir_SQ)' \\\n \t\t$(MAKE) -C ../t gitweb-test\n \n ### Installation rules\n \n-install: all\n+install: all install-modules\n \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebdir_SQ)'\n \t$(INSTALL) -m 755 $(GITWEB_PROGRAMS) '$(DESTDIR_SQ)$(gitwebdir_SQ)'\n \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'\n \t$(INSTALL) -m 644 $(GITWEB_FILES) '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'\n \n+install-modules:\n+\tinstall_dirs=\"$(sort $(dir $(GITWEB_MODULES)))\" && \\\n+\tfor dir in $$install_dirs; do \\\n+\t\ttest -d '$(DESTDIR_SQ)$(gitweblibdir_SQ)/$$dir' || \\\n+\t\t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitweblibdir_SQ)/$$dir'; \\\n+\tdone\n+\tgitweb_modules=\"$(GITWEB_MODULES)\" && \\\n+\tfor mod in $$gitweb_modules; do \\\n+\t\t$(INSTALL) -m 644 lib/$$mod '$(DESTDIR_SQ)$(gitweblibdir_SQ)/$$(dirname $$mod)'; \\\n+\tdone\n+\n ### Cleaning rules\n \n clean:\n \t$(RM) gitweb.cgi static/gitweb.min.js static/gitweb.min.css GITWEB-BUILD-OPTIONS\n \n-.PHONY: all clean install test test-installed .FORCE-GIT-VERSION-FILE FORCE\n+.PHONY: all clean install install-modules test test-installed .FORCE-GIT-VERSION-FILE FORCE\n \ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 679f2da..cfa511c 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -10,6 +10,14 @@\n use 5.008;\n use strict;\n use warnings;\n+\n+use File::Spec;\n+# __DIR__ is taken from Dir::Self __DIR__ fragment\n+sub __DIR__ () {\n+\tFile::Spec->rel2abs(join '', (File::Spec->splitpath(__FILE__))[0, 1]);\n+}\n+use lib __DIR__ . '/lib';\n+\n use CGI qw(:standard :escapeHTML -nosticky);\n use CGI::Util qw(unescape);\n use CGI::Carp qw(fatalsToBrowser set_message);\n-- \n1.7.2.3\n"},{"id":"157755","messageId":"1291931844-28454-3-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 02/18] gitweb: add output buffering and associated functions","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:08Z","receivedAt":"2010-12-09T21:57:08Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"This adds output buffering for gitweb, mainly in preparation for\ncaching support. This is a dramatic change to how caching was being\ndone, mainly in passing around the variable manually and such.\n\nThis centrally flips the entire STDOUT to a variable, which after the\ncompletion of the run, flips it back and does a print on the resulting\ndata.\n\nThis should save on the previous 10K line patch (or so) that adds more\nexplicit output passing.\n\n[jn: modified reset_output to silence 'gitweb.perl: Name \"main::STDOUT_REAL\"\n used only once: possible typo at ../gitweb/gitweb.perl line 1130.' warning]\n\nSigned-off-by: Junio C Hamano \n---\n gitweb/gitweb.perl | 29 +++++++++++++++++++++++++++++\n 1 files changed, 29 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex cfa511c..cae0e34 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -39,6 +39,9 @@ BEGIN {\n \n our $version = \"++GIT_VERSION++\";\n \n+# Output buffer variable\n+our $output = \"\";\n+\n our ($my_url, $my_uri, $base_url, $path_info, $home_link);\n sub evaluate_uri {\n \tour $cgi;\n@@ -1134,6 +1137,25 @@ sub evaluate_argv {\n \t);\n }\n \n+sub change_output {\n+\tour $output;\n+\n+\t# Trap the 'proper' STDOUT to STDOUT_REAL for things like error messages and such\n+\topen(STDOUT_REAL,\">&STDOUT\") or die \"Unable to capture STDOUT $!\\n\";\n+\n+\t# Close STDOUT, so that it isn't being used anymore.\n+\tclose STDOUT;\n+\n+\t# Trap STDOUT to the $output variable, which is what I was using in the original\n+\t# patch anyway.\n+\topen(STDOUT,\">\", \\$output) || die \"Unable to open STDOUT: $!\"; #open STDOUT handle to use $var\n+}\n+\n+sub reset_output {\n+\t# This basically takes STDOUT_REAL and puts it back as STDOUT\n+\topen STDOUT, \">&\", \\*STDOUT_REAL;\n+}\n+\n sub run {\n \tevaluate_argv();\n \n@@ -1145,7 +1167,10 @@ sub run {\n \t\t$pre_dispatch_hook->()\n \t\t\tif $pre_dispatch_hook;\n \n+\t\tchange_output();\n \t\trun_request();\n+\t\treset_output();\n+\t\tprint $output;\n \n \t\t$post_dispatch_hook->()\n \t\t\tif $post_dispatch_hook;\n@@ -3655,6 +3680,10 @@ sub die_error {\n \t\t500 => '500 Internal Server Error',\n \t\t503 => '503 Service Unavailable',\n \t);\n+\t# Reset the output so that we are actually going to STDOUT as opposed\n+\t# to buffering the output.\n+\treset_output();\n+\n \tgit_header_html($http_responses{$status}, undef, %opts);\n \tprint <\n-- \n1.7.2.3\n"},{"id":"157757","messageId":"1291931844-28454-4-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 03/18] gitweb: File based caching layer (from git.kernel.org)","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:09Z","receivedAt":"2010-12-09T21:57:09Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"This is a relatively large patch that implements the file based\ncaching layer that is quite similar to the one used on such large\nsites as kernel.org and soon git.fedoraproject.org. This provides\na simple, and straight forward caching mechanism that scales\ndramatically better than Gitweb by itself.\n\nThe caching layer basically buffers the output that Gitweb would\nnormally return, and saves that output to a cache file on the local\ndisk. When the file is requested it attempts to gain a shared lock\non the cache file and cat it out to the client. Should an exclusive\nlock be on a file (it's being updated) the code has a choice to either\nupdate in the background and go ahead and show the stale page while\nupdate is being performed, or stall the client(s) until the page\nis generated.\n\nThere are two forms of stalling involved here, background building\nand non-background building, both of which are discussed in the\nconfiguration page.\n\nThere are still a few known \"issues\" with respect to this:\n- Code needs to be added to be \"browser\" aware so\n that clients like wget that are trying to get a\n binary blob don't obtain a \"Generating...\" page\n\nCaching is disabled by default. You can turn it on by setting\n$caching_enabled variable to true to enable file based caching.\n\n[jn: added error checking to loading 'cache.pl'; moved check\n for $caching_enabled outside out of cache_fetch, which required\n update to die_error()]\n\nSigned-off-by: Junio C Hamano \n---\n gitweb/Makefile | 3 +\n gitweb/gitweb.perl | 105 ++++++++++++--\n gitweb/lib/cache.pl | 348 ++++++++++++++++++++++++++++++++++++++++++++++\n gitweb/static/gitweb.css | 6 +\n 4 files changed, 450 insertions(+), 12 deletions(-)\n create mode 100644 gitweb/lib/cache.pl\n\ndiff --git a/gitweb/Makefile b/gitweb/Makefile\nindex f9e32eb..6ddd4f1 100644\n--- a/gitweb/Makefile\n+++ b/gitweb/Makefile\n@@ -113,6 +113,9 @@ endif\n \n GITWEB_FILES += static/git-logo.png static/git-favicon.png\n \n+# Gitweb caching\n+GITWEB_MODULES += cache.pl\n+\n GITWEB_REPLACE = \\\n \t-e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \\\n \t-e 's|++GIT_BINDIR++|$(bindir)|g' \\\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex cae0e34..3c3ff08 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -250,6 +250,53 @@ our %avatar_size = (\n # Leave it undefined (or set to 'undef') to turn off load checking.\n our $maxload = 300;\n \n+# This enables/disables the caching layer in gitweb. This currently only supports the\n+# 'dumb' file based caching layer, primarily used on git.kernel.org. this is reasonably\n+# effective but it has the downside of requiring a huge amount of disk space if there\n+# are a number of repositories involved. It is not uncommon for git.kernel.org to have\n+# on the order of 80G - 120G accumulate over the course of a few months. It is recommended\n+# that the cache directory be periodically completely deleted, and this is safe to perform.\n+# Suggested mechanism\n+# mv $cacheidr $cachedir.flush;mkdir $cachedir;rm -rf $cachedir.flush\n+our $caching_enabled = 0;\n+\n+# Used to set the minimum cache timeout for the dynamic caching algorithm. Basically\n+# if we calculate the cache to be under this number of seconds we set the cache timeout\n+# to this minimum.\n+# Value is in seconds. 1 = 1 seconds, 60 = 1 minute, 600 = 10 minutes, 3600 = 1 hour\n+our $minCacheTime = 20;\n+\n+# Used to set the maximum cache timeout for the dynamic caching algorithm. Basically\n+# if we calculate the cache to exceed this number of seconds we set the cache timeout\n+# to this maximum.\n+# Value is in seconds. 1 = 1 seconds, 60 = 1 minute, 600 = 10 minutes, 3600 = 1 hour\n+our $maxCacheTime = 1200;\n+\n+# If you need to change the location of the caching directory, override this\n+# otherwise this will probably do fine for you\n+our $cachedir = 'cache';\n+\n+# If this is set (to 1) cache will do it's best to always display something instead\n+# of making someone wait for the cache to update. This will launch the cacheUpdate\n+# into the background and it will lock a .bg file and will only lock the\n+# actual cache file when it needs to write into it. In theory this will make\n+# gitweb seem more responsive at the price of possibly stale data.\n+our $backgroundCache = 1;\n+\n+# Used to set the maximum cache file life. If a cache files last modify time exceeds\n+# this value, it will assume that the data is just too old, and HAS to be regenerated\n+# instead of trying to display the existing cache data.\n+# Value is in seconds. 1 = 1 seconds, 60 = 1 minute, 600 = 10 minutes, 3600 = 1 hour\n+# 18000 = 5 hours\n+our $maxCacheLife = 18000;\n+\n+# Used to enable or disable background forking of the gitweb caching. Mainly here for debugging purposes\n+our $cacheDoFork = 1;\n+\n+our $fullhashpath = *STDOUT;\n+our $fullhashbinpath = *STDOUT;\n+our $fullhashbinpathfinal = *STDOUT;\n+\n # configuration for 'highlight' (http://www.andre-simon.de/)\n # match by basename\n our %highlight_basename = (\n@@ -506,6 +553,15 @@ our %feature = (\n \t\t'default' => [0]},\n );\n \n+#\n+# Includes\n+#\n+if (!exists $INC{'cache.pl'}) {\n+\tmy $return = do 'cache.pl';\n+\tdie $@ if $@;\n+\tdie \"Couldn't read 'cache.pl': $!\" if (!defined $return);\n+}\n+\n sub gitweb_get_feature {\n \tmy ($name) = @_;\n \treturn unless exists $feature{$name};\n@@ -734,6 +790,10 @@ our %actions = (\n \t\"project_list\" => \\&git_project_list,\n \t\"project_index\" => \\&git_project_index,\n );\n+sub is_cacheable {\n+\tmy $action = shift;\n+\treturn !($action eq 'blame_data' || $action eq 'blame_incremental');\n+}\n \n # finally, we have the hash of allowed extra_options for the commands that\n # allow them\n@@ -1072,7 +1132,11 @@ sub dispatch {\n \t !$project) {\n \t\tdie_error(400, \"Project needed\");\n \t}\n-\t$actions{$action}->();\n+\tif ($caching_enabled && is_cacheable($action)) {\n+\t\tcache_fetch($action);\n+\t} else {\n+\t\t$actions{$action}->();\n+\t}\n }\n \n sub reset_timer {\n@@ -1142,6 +1206,7 @@ sub change_output {\n \n \t# Trap the 'proper' STDOUT to STDOUT_REAL for things like error messages and such\n \topen(STDOUT_REAL,\">&STDOUT\") or die \"Unable to capture STDOUT $!\\n\";\n+\tprint STDOUT_REAL \"\";\n \n \t# Close STDOUT, so that it isn't being used anymore.\n \tclose STDOUT;\n@@ -1167,10 +1232,7 @@ sub run {\n \t\t$pre_dispatch_hook->()\n \t\t\tif $pre_dispatch_hook;\n \n-\t\tchange_output();\n \t\trun_request();\n-\t\treset_output();\n-\t\tprint $output;\n \n \t\t$post_dispatch_hook->()\n \t\t\tif $post_dispatch_hook;\n@@ -3447,7 +3509,8 @@ sub git_header_html {\n \t# support xhtml+xml but choking when it gets what it asked for.\n \tif (defined $cgi->http('HTTP_ACCEPT') &&\n \t $cgi->http('HTTP_ACCEPT') =~ m/(,|;|\\s|^)application\\/xhtml\\+xml(,|;|\\s|$)/ &&\n-\t $cgi->Accept('application/xhtml+xml') != 0) {\n+\t $cgi->Accept('application/xhtml+xml') != 0 &&\n+\t !$caching_enabled) {\n \t\t$content_type = 'application/xhtml+xml';\n \t} else {\n \t\t$content_type = 'text/html';\n@@ -3592,6 +3655,7 @@ sub git_footer_html {\n \tmy $feed_class = 'rss_logo';\n \n \tprint \"
\\n\";\n+\tprint \"
Cache Last Updated: \". gmtime( time ) .\" GMT
\\n\";\n \tif (defined $project) {\n \t\tmy $descr = git_get_project_description($project);\n \t\tif (defined $descr) {\n@@ -3680,9 +3744,14 @@ sub die_error {\n \t\t500 => '500 Internal Server Error',\n \t\t503 => '503 Service Unavailable',\n \t);\n+\t# The output handlers for die_error need to be reset to STDOUT\n+\t# so that half the message isn't being output to random and\n+\t# half to STDOUT as expected. This is mainly for the benefit\n+\t# of using git_header_html() and git_footer_html() since\n+\t#\n \t# Reset the output so that we are actually going to STDOUT as opposed\n \t# to buffering the output.\n-\treset_output();\n+\treset_output() if ($caching_enabled);\n \n \tgit_header_html($http_responses{$status}, undef, %opts);\n \tprint <;\n-\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n+\tif ($caching_enabled) {\n+\t\topen BINOUT, '>', $fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n+\t}else{\n+\t\topen BINOUT, '>', \\$fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n+\t}\n+\tbinmode BINOUT, ':raw';\n+\tprint BINOUT <$fd>;\n+\tbinmode BINOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n+\tclose BINOUT;\n \tclose $fd;\n }\n \n@@ -5879,9 +5954,15 @@ sub git_snapshot {\n \n \topen my $fd, \"-|\", $cmd\n \t\tor die_error(500, \"Execute git-archive failed\");\n-\tbinmode STDOUT, ':raw';\n-\tprint <$fd>;\n-\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n+\tif ($caching_enabled) {\n+\t\topen BINOUT, '>', $fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n+\t}else{\n+\t\topen BINOUT, '>', \\$fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n+\t}\n+\tbinmode BINOUT, ':raw';\n+\tprint BINOUT <$fd>;\n+\tbinmode BINOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n+\tclose BINOUT;\n \tclose $fd;\n }\n \ndiff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\nnew file mode 100644\nindex 0000000..dd14bfb\n--- /dev/null\n+++ b/gitweb/lib/cache.pl\n@@ -0,0 +1,348 @@\n+# gitweb - simple web interface to track changes in git repositories\n+#\n+# (C) 2006, John 'Warthog9' Hawley \n+#\n+# This program is licensed under the GPLv2\n+\n+#\n+# Gitweb caching engine\n+#\n+\n+#use File::Path qw(make_path remove_tree);\n+use File::Path qw(mkpath rmtree); # Used for compatability reasons\n+use Digest::MD5 qw(md5 md5_hex md5_base64);\n+use Fcntl ':flock';\n+use File::Copy;\n+\n+sub cache_fetch {\n+\tmy ($action) = @_;\n+\tmy $cacheTime = 0;\n+\n+\tif(! -d $cachedir){\n+\t\tprint \"*** Warning ***: Caching enabled but cache directory does not exsist. ($cachedir)\\n\";\n+\t\tmkdir (\"cache\", 0755) || die \"Cannot create cache dir - you will need to manually create\";\n+\t\tprint \"Cache directory created successfully\\n\";\n+\t}\n+\n+\tour $full_url = \"$my_url?\". $ENV{'QUERY_STRING'};\n+\tour $urlhash = md5_hex($full_url);\n+\tour $fullhashdir = \"$cachedir/\". substr( $urlhash, 0, 2) .\"/\";\n+\n+\teval { mkpath( $fullhashdir, 0, 0777 ) };\n+\tif ($@) {\n+\t\tdie_error(500, \"Internal Server Error\", \"Could not create cache directory: $@\");\n+\t}\n+\t$fullhashpath = \"$fullhashdir/\". substr( $urlhash, 2 );\n+\t$fullhashbinpath = \"$fullhashpath.bin.wt\";\n+\t$fullhashbinpathfinal = \"$fullhashpath.bin\";\n+\n+\tif(! -e \"$fullhashpath\" ){\n+\t\tif(! $cacheDoFork || ! defined(my $childPid = fork()) ){\n+\t\t\tcacheUpdate($action,0);\n+\t\t\tcacheDisplay($action);\n+\t\t} elsif ( $childPid == 0 ){\n+\t\t\t#run the updater\n+\t\t\tcacheUpdate($action,1);\n+\t\t}else{\n+\t\t\tcacheWaitForUpdate($action);\n+\t\t}\n+\t}else{\n+\t\t#if cache is out dated, update\n+\t\t#else displayCache();\n+\t\topen(cacheFile, '<', \"$fullhashpath\");\n+\t\tstat(cacheFile);\n+\t\tclose(cacheFile);\n+\t\tmy $stat_time = (stat(_))[9];\n+\t\tmy $stat_size = (stat(_))[7];\n+\n+\t\t$cacheTime = get_loadavg() * 60;\n+\t\tif( $cacheTime > $maxCacheTime ){\n+\t\t\t$cacheTime = $maxCacheTime;\n+\t\t}\n+\t\tif( $cacheTime < $minCacheTime ){\n+\t\t\t$cacheTime = $minCacheTime;\n+\t\t}\n+\t\tif( $stat_time < (time - $cacheTime) || $stat_size == 0 ){\n+\t\t\tif( ! $cacheDoFork || ! defined(my $childPid = fork()) ){\n+\t\t\t\tcacheUpdate($action,0);\n+\t\t\t\tcacheDisplay($action);\n+\t\t\t} elsif ( $childPid == 0 ){\n+\t\t\t\t#run the updater\n+\t\t\t\t#print \"Running updater\\n\";\n+\t\t\t\tcacheUpdate($action,1);\n+\t\t\t}else{\n+\t\t\t\t#print \"Waiting for update\\n\";\n+\t\t\t\tcacheWaitForUpdate($action);\n+\t\t\t}\n+\t\t} else {\n+\t\t\tcacheDisplay($action);\n+\t\t}\n+\n+\n+\t}\n+\n+\t#\n+\t# If all of the caching failes - lets go ahead and press on without it and fall back to 'default'\n+\t# non-caching behavior. This is the softest of the failure conditions.\n+\t#\n+\t#$actions{$action}->();\n+}\n+\n+sub cacheUpdate {\n+\tmy ($action,$areForked) = @_;\n+\tmy $lockingStatus;\n+\tmy $fileData = \"\";\n+\n+\tif($backgroundCache){\n+\t\topen(cacheFileBG, '>:utf8', \"$fullhashpath.bg\");\n+\t\tmy $lockStatBG = flock(cacheFileBG,LOCK_EX|LOCK_NB);\n+\n+\t\t$lockStatus = $lockStatBG;\n+\t}else{\n+\t\topen(cacheFile, '>:utf8', \\$fullhashpath);\n+\t\tmy $lockStat = flock(cacheFile,LOCK_EX|LOCK_NB);\n+\n+\t\t$lockStatus = $lockStat;\n+\t}\n+\t#print \"lock status: $lockStat\\n\";\n+\n+\n+\tif (! $lockStatus ){\n+\t\tif ( $areForked ){\n+\t\t\texit(0);\n+\t\t}else{\n+\t\t\treturn;\n+\t\t}\n+\t}\n+\n+\tif(\n+\t\t$action eq \"snapshot\"\n+\t\t||\n+\t\t$action eq \"blob_plain\"\n+\t){\n+\t\tmy $openstat = open(cacheFileBinWT, '>>:utf8', \"$fullhashbinpath\");\n+\t\tmy $lockStatBin = flock(cacheFileBinWT,LOCK_EX|LOCK_NB);\n+\t}\n+\n+\t# Trap all output from the action\n+\tchange_output();\n+\n+\t$actions{$action}->();\n+\n+\t# Reset the outputs as we should be fine now\n+\treset_output();\n+\n+\n+\tif($backgroundCache){\n+\t\topen(cacheFile, '>:utf8', \"$fullhashpath\");\n+\t\t$lockStat = flock(cacheFile,LOCK_EX);\n+\n+\t\tif (! $lockStat ){\n+\t\t\tif ( $areForked ){\n+\t\t\t\texit(0);\n+\t\t\t}else{\n+\t\t\t\treturn;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\tif(\n+\t\t$action eq \"snapshot\"\n+\t\t||\n+\t\t$action eq \"blob_plain\"\n+\t){\n+\t\tmy $openstat = open(cacheFileBinFINAL, '>:utf8', \"$fullhashbinpathfinal\");\n+\t\t$lockStatBIN = flock(cacheFileBinFINAL,LOCK_EX);\n+\n+\t\tif (! $lockStatBIN ){\n+\t\t\tif ( $areForked ){\n+\t\t\t\texit(0);\n+\t\t\t}else{\n+\t\t\t\treturn;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\t# Actually dump the output to the proper file handler\n+\tlocal $/ = undef;\n+\t$|++;\n+\tprint cacheFile \"$output\";\n+\t$|--;\n+\tif(\n+\t\t$action eq \"snapshot\"\n+\t\t||\n+\t\t$action eq \"blob_plain\"\n+\t){\n+\t\tmove(\"$fullhashbinpath\", \"$fullhashbinpathfinal\") or die \"Binary Cache file could not be updated: $!\";\n+\n+\t\tflock(cacheFileBinFINAL,LOCK_UN);\n+\t\tclose(cacheFileBinFINAL);\n+\n+\t\tflock(cacheFileBinWT,LOCK_UN);\n+\t\tclose(cacheFileBinWT);\n+\t}\n+\n+\tflock(cacheFile,LOCK_UN);\n+\tclose(cacheFile);\n+\n+\tif($backgroundCache){\n+\t\tflock(cacheFileBG,LOCK_UN);\n+\t\tclose(cacheFileBG);\n+\t}\n+\n+\tif ( $areForked ){\n+\t\texit(0);\n+\t} else {\n+\t\treturn;\n+\t}\n+}\n+\n+\n+sub cacheWaitForUpdate {\n+\tmy ($action) = @_;\n+\tmy $x = 0;\n+\tmy $max = 10;\n+\tmy $lockStat = 0;\n+\n+\tif( $backgroundCache ){\n+\t\tif( -e \"$fullhashpath\" ){\n+\t\t\topen(cacheFile, '<:utf8', \"$fullhashpath\");\n+\t\t\t$lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);\n+\t\t\tstat(cacheFile);\n+\t\t\tclose(cacheFile);\n+\n+\t\t\tif( $lockStat && ( (stat(_))[9] > (time - $maxCacheLife) ) ){\n+\t\t\t\tcacheDisplay($action);\n+\t\t\t\treturn;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\tif(\n+\t\t$action eq \"atom\"\n+\t\t||\n+\t\t$action eq \"rss\"\n+\t\t||\n+\t\t$action eq \"opml\"\n+\t){\n+\t\tdo {\n+\t\t\tsleep 2 if $x > 0;\n+\t\t\topen(cacheFile, '<:utf8', \"$fullhashpath\");\n+\t\t\t$lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);\n+\t\t\tclose(cacheFile);\n+\t\t\t$x++;\n+\t\t\t$combinedLockStat = $lockStat;\n+\t\t} while ((! $combinedLockStat) && ($x < $max));\n+\n+\t\tif( $x != $max ){\n+\t\t\tcacheDisplay($action);\n+\t\t}\n+\t\treturn;\n+\t}\n+\n+\t$| = 1;\n+\n+\tprint $::cgi->header(\n+\t\t\t\t-type=>'text/html',\n+\t\t\t\t-charset => 'utf-8',\n+\t\t\t\t-status=> 200,\n+\t\t\t\t-expires => 'now',\n+\t\t\t\t# HTTP/1.0\n+\t\t\t\t-Pragma => 'no-cache',\n+\t\t\t\t# HTTP/1.1\n+\t\t\t\t-Cache_Control => join(\n+\t\t\t\t\t\t\t', ',\n+\t\t\t\t\t\t\tqw(\n+\t\t\t\t\t\t\t\tprivate\n+\t\t\t\t\t\t\t\tno-cache\n+\t\t\t\t\t\t\t\tno-store\n+\t\t\t\t\t\t\t\tmust-revalidate\n+\t\t\t\t\t\t\t\tmax-age=0\n+\t\t\t\t\t\t\t\tpre-check=0\n+\t\t\t\t\t\t\t\tpost-check=0\n+\t\t\t\t\t\t\t)\n+\t\t\t\t\t\t)\n+\t\t\t\t);\n+\n+\tprint <\n+\n+\n+\n+\n+\n+\n+\n+$title\n+\n+\n+EOF\n+\n+\tprint \"Generating..\";\n+\tdo {\n+\t\tprint \".\";\n+\t\tsleep 2 if $x > 0;\n+\t\topen(cacheFile, '<:utf8', \"$fullhashpath\");\n+\t\t$lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);\n+\t\tclose(cacheFile);\n+\t\t$x++;\n+\t\t$combinedLockStat = $lockStat;\n+\t} while ((! $combinedLockStat) && ($x < $max));\n+\tprint <\n+\n+EOF\n+\treturn;\n+}\n+\n+sub cacheDisplay {\n+\tlocal $/ = undef;\n+\t$|++;\n+\n+\tmy ($action) = @_;\n+\topen(cacheFile, '<:utf8', \"$fullhashpath\");\n+\t$lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);\n+\n+\tif (! $lockStat ){\n+\t\tclose(cacheFile);\n+\t\tcacheWaitForUpdate($action);\n+\t}\n+\n+\tif(\n+\t\t(\n+\t\t\t$action eq \"snapshot\"\n+\t\t\t||\n+\t\t\t$action eq \"blob_plain\"\n+\t\t)\n+\t){\n+\t\tmy $openstat = open(cacheFileBin, '<', \"$fullhashbinpathfinal\");\n+\t\t$lockStatBIN = flock(cacheFileBin,LOCK_SH|LOCK_NB);\n+\t\tif (! $lockStatBIN ){\n+\t\t\tsystem (\"echo 'cacheDisplay - bailing due to binary lock failure' >> /tmp/gitweb.log\");\n+\t\t\tclose(cacheFile);\n+\t\t\tclose(cacheFileBin);\n+\t\t\tcacheWaitForUpdate($action);\n+\t\t}\n+\n+\t\tmy $binfilesize = -s \"$fullhashbinpathfinal\";\n+\t\tprint \"Content-Length: $binfilesize\";\n+\t}\n+\twhile( ){\n+\t\tprint $_;\n+\t}\n+\tif(\n+\t\t$action eq \"snapshot\"\n+\t\t||\n+\t\t$action eq \"blob_plain\"\n+\t){\n+\t\tbinmode STDOUT, ':raw';\n+\t\tprint ;\n+\t\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n+\t\tclose(cacheFileBin);\n+\t}\n+\tclose(cacheFile);\n+\t$|--;\n+}\n+\n+1;\n+__END__\ndiff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css\nindex 4132aab..972d32e 100644\n--- a/gitweb/static/gitweb.css\n+++ b/gitweb/static/gitweb.css\n@@ -67,6 +67,12 @@ div.page_path {\n \tborder-width: 0px 0px 1px;\n }\n \n+div.cachetime {\n+\tfloat: left;\n+\tmargin-right: 10px;\n+\tcolor: #555555;\n+}\n+\n div.page_footer {\n \theight: 17px;\n \tpadding: 4px 8px;\n-- \n1.7.2.3\n"},{"id":"157740","messageId":"1291931844-28454-5-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 04/18] gitweb: Minimal testing of gitweb caching","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:10Z","receivedAt":"2010-12-09T21:57:10Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"From: Jakub Narebski \n\nAdd basic tests of caching support to t9500-gitweb-standalone-no-errors\ntest: set $caching_enabled to true and check for errors for first time\nrun (generating cache) and second time run (retrieving from cache) for a\nsingle view - summary view for a project. Check also that request for\nnon-existent object (which results in die_error() codepath to be called)\ndoesn't produce errors.\n\nCheck in t9501-gitweb-standalone-http-status that request for\nnon-existent object produces correct output (HTTP headers and HTML\noutput) also when caching is enabled.\n\nCheck in the t9502-gitweb-standalone-parse-output test that gitweb\nproduces the same output with and without caching, for first and\nsecond run, with binary (raw) or plain text (utf8) output.\n\nThe common routine that enables cache, gitweb_enable_caching, is\ndefined in t/gitweb-lib.sh\n\nSigned-off-by: Jakub Narebski \n---\n t/gitweb-lib.sh | 15 +++++++++++++\n t/t9500-gitweb-standalone-no-errors.sh | 20 +++++++++++++++++\n t/t9501-gitweb-standalone-http-status.sh | 13 +++++++++++\n t/t9502-gitweb-standalone-parse-output.sh | 33 +++++++++++++++++++++++++++++\n 4 files changed, 81 insertions(+), 0 deletions(-)\n mode change 100644 => 100755 t/gitweb-lib.sh\n\ndiff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh\nold mode 100644\nnew mode 100755\nindex b9bb95f..16ce811\n--- a/t/gitweb-lib.sh\n+++ b/t/gitweb-lib.sh\n@@ -52,6 +52,21 @@ EOF\n \texport SCRIPT_NAME\n }\n \n+gitweb_enable_caching () {\n+\ttest_expect_success 'enable caching' '\n+\t\tcat >>gitweb_config.perl <<-\\EOF &&\n+\t\tour $caching_enabled = 1;\n+\t\tour $minCacheTime = 60*60*24*7*30; # very long expiration time for tests (a month)\n+\t\tour $maxCacheTime = 60*60*24*7*30*365; # upper bound for dynamic (adaptive) caching\n+\t\tour $cachedir = \"cache\"; # for testsuite to clear the right thing\n+\t\t# required, because otherwise some tests might intermittently not pass\n+\t\tour $backgroundCache = 0; # should turn off cacheWaitForUpdate() / \"Generating...\"\n+\t\t#our $cacheDoFork = 0;\n+\t\tEOF\n+\t\trm -rf cache/\n+\t'\n+}\n+\n gitweb_run () {\n \tGATEWAY_INTERFACE='CGI/1.1'\n \tHTTP_ACCEPT='*/*'\ndiff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh\nindex 21cd286..86c1b50 100755\n--- a/t/t9500-gitweb-standalone-no-errors.sh\n+++ b/t/t9500-gitweb-standalone-no-errors.sh\n@@ -677,4 +677,24 @@ test_expect_success HIGHLIGHT \\\n \t gitweb_run \"p=.git;a=blob;f=test.sh\"'\n test_debug 'cat gitweb.log'\n \n+# ----------------------------------------------------------------------\n+# caching\n+\n+gitweb_enable_caching\n+\n+test_expect_success \\\n+\t'caching enabled (project summary, first run, generating cache)' \\\n+\t'gitweb_run \"p=.git;a=summary\"'\n+test_debug 'cat gitweb.log'\n+\n+test_expect_success \\\n+\t'caching enabled (project summary, second run, cached version)' \\\n+\t'gitweb_run \"p=.git;a=summary\"'\n+test_debug 'cat gitweb.log'\n+\n+test_expect_success \\\n+\t'caching enabled (non-existent commit, non-cache error page)' \\\n+\t'gitweb_run \"p=.git;a=commit;h=non-existent\"'\n+test_debug 'cat gitweb.log'\n+\n test_done\ndiff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\nindex 2487da1..5b1df3f 100755\n--- a/t/t9501-gitweb-standalone-http-status.sh\n+++ b/t/t9501-gitweb-standalone-http-status.sh\n@@ -134,5 +134,18 @@ cat >>gitweb_config.perl <<\\EOF\n our $maxload = undef;\n EOF\n \n+# ----------------------------------------------------------------------\n+# output caching\n+\n+gitweb_enable_caching\n+\n+test_expect_success 'caching enabled (non-existent commit, 404 error)' '\n+\tgitweb_run \"p=.git;a=commit;h=non-existent\" &&\n+\tgrep \"Status: 404 Not Found\" gitweb.headers &&\n+\tgrep \"404 - Unknown commit object\" gitweb.body\n+'\n+test_debug 'echo \"log\" && cat gitweb.log'\n+test_debug 'echo \"headers\" && cat gitweb.headers'\n+test_debug 'echo \"body\" && cat gitweb.body'\n \n test_done\ndiff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh\nindex dd83890..bc8eb01 100755\n--- a/t/t9502-gitweb-standalone-parse-output.sh\n+++ b/t/t9502-gitweb-standalone-parse-output.sh\n@@ -112,4 +112,37 @@ test_expect_success 'snapshot: hierarchical branch name (xx/test)' '\n '\n test_debug 'cat gitweb.headers'\n \n+\n+# ----------------------------------------------------------------------\n+# whether gitweb with caching enabled produces the same output\n+\n+test_expect_success 'setup for caching tests (utf8 patch, binary file)' '\n+\t. \"$TEST_DIRECTORY\"/t3901-utf8.txt &&\n+\tcp \"$TEST_DIRECTORY\"/test9200a.png image.png &&\n+\tgit add image.png &&\n+\tgit commit -F \"$TEST_DIRECTORY\"/t3900/1-UTF-8.txt &&\n+\tgitweb_run \"p=.git;a=patch\" &&\n+\tmv gitweb.body no_cache.txt &&\n+\tgitweb_run \"p=.git;a=blob_plain;f=image.png\" &&\n+\tmv gitweb.body no_cache.png\n+'\n+\n+gitweb_enable_caching\n+\n+for desc in 'generating cache' 'cached version'; do\n+\ttest_expect_success \"caching enabled, plain text (utf8) output, $desc\" '\n+\t\tgitweb_run \"p=.git;a=patch\" &&\n+\t\tmv gitweb.body cache.txt &&\n+\t\ttest_cmp no_cache.txt cache.txt\n+\t'\n+done\n+\n+for desc in 'generating cache' 'cached version'; do\n+\ttest_expect_success \"caching enabled, binary output (raw), $desc\" '\n+\t\tgitweb_run \"p=.git;a=blob_plain;f=image.png\" &&\n+\t\tmv gitweb.body cache.png &&\n+\t\tcmp no_cache.png cache.png\n+\t'\n+done\n+\n test_done\n-- \n1.7.2.3\n"},{"id":"157744","messageId":"1291931844-28454-6-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 05/18] gitweb: Regression fix concerning binary output of files","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:11Z","receivedAt":"2010-12-09T21:57:11Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"This solves the regression introduced with v7.2 of the gitweb-caching code,\nfix proposed by Jakub in his e-mail.\n\nSigned-off-by: John 'Warthog9' Hawley \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 3c3ff08..f2ef3da 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5664,7 +5664,7 @@ sub git_blob_plain {\n \tif ($caching_enabled) {\n \t\topen BINOUT, '>', $fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n \t}else{\n-\t\topen BINOUT, '>', \\$fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n+\t\topen BINOUT, '>&', \\$fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n \t}\n \tbinmode BINOUT, ':raw';\n \tprint BINOUT <$fd>;\n@@ -5957,7 +5957,7 @@ sub git_snapshot {\n \tif ($caching_enabled) {\n \t\topen BINOUT, '>', $fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n \t}else{\n-\t\topen BINOUT, '>', \\$fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n+\t\topen BINOUT, '>&', \\$fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n \t}\n \tbinmode BINOUT, ':raw';\n \tprint BINOUT <$fd>;\n-- \n1.7.2.3\n"},{"id":"157756","messageId":"1291931844-28454-7-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 06/18] gitweb: Add more explicit means of disabling 'Generating...' page","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:12Z","receivedAt":"2010-12-09T21:57:12Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"As requested this adds $cacheGenStatus variable, default 1 (on).\nIf caching is enabled it will explicitly disble the display of the\n'Generating...' page and just force the user to stall indefinately.\n\nAlso adding it to gitweb's test code as I'm sure the 'Generating...'\npage isn't that useful there.\n\nSigned-off-by: John 'Warthog9' Hawley \n---\n gitweb/gitweb.perl | 6 ++++++\n gitweb/lib/cache.pl | 2 ++\n t/gitweb-lib.sh | 1 +\n 3 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex f2ef3da..05e7ba6 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -293,6 +293,12 @@ our $maxCacheLife = 18000;\n # Used to enable or disable background forking of the gitweb caching. Mainly here for debugging purposes\n our $cacheDoFork = 1;\n \n+# Used to enable or disable the foreground \"Generating...\" page. This is here to be more explicit should\n+# people want to disable it.\n+# Default: 1 (True - Enabled)\n+# To disable set to 0\n+our $cacheGenStatus = 1;\n+\n our $fullhashpath = *STDOUT;\n our $fullhashbinpath = *STDOUT;\n our $fullhashbinpathfinal = *STDOUT;\ndiff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\nindex dd14bfb..a8ee99e 100644\n--- a/gitweb/lib/cache.pl\n+++ b/gitweb/lib/cache.pl\n@@ -224,6 +224,8 @@ sub cacheWaitForUpdate {\n \t\t$action eq \"rss\"\n \t\t||\n \t\t$action eq \"opml\"\n+\t\t||\n+\t\t! $cacheGenStatus\n \t){\n \t\tdo {\n \t\t\tsleep 2 if $x > 0;\ndiff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh\nindex 16ce811..10c4a3d 100755\n--- a/t/gitweb-lib.sh\n+++ b/t/gitweb-lib.sh\n@@ -26,6 +26,7 @@ our \\$projects_list = '';\n our \\$export_ok = '';\n our \\$strict_export = '';\n our \\$maxload = undef;\n+our \\$cacheGenStatus = 0;\n \n EOF\n \n-- \n1.7.2.3\n"},{"id":"157742","messageId":"1291931844-28454-8-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 07/18] gitweb: Revert back to $cache_enable vs. $caching_enabled","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:13Z","receivedAt":"2010-12-09T21:57:13Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"Simple enough, $cache_enable (along with all caching variables) are\nalready in production in multiple places and doing a small semantic\nchange without backwards compatibility is pointless breakage.\n\nThis reverts back to the previous variable to enable / disable caching\n\nSigned-off-by: John 'Warthog9' Hawley \n---\n gitweb/gitweb.perl | 12 ++++++------\n t/gitweb-lib.sh | 2 +-\n 2 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 05e7ba6..5eb0309 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -258,7 +258,7 @@ our $maxload = 300;\n # that the cache directory be periodically completely deleted, and this is safe to perform.\n # Suggested mechanism\n # mv $cacheidr $cachedir.flush;mkdir $cachedir;rm -rf $cachedir.flush\n-our $caching_enabled = 0;\n+our $cache_enable = 0;\n \n # Used to set the minimum cache timeout for the dynamic caching algorithm. Basically\n # if we calculate the cache to be under this number of seconds we set the cache timeout\n@@ -1138,7 +1138,7 @@ sub dispatch {\n \t !$project) {\n \t\tdie_error(400, \"Project needed\");\n \t}\n-\tif ($caching_enabled && is_cacheable($action)) {\n+\tif ($cache_enable && is_cacheable($action)) {\n \t\tcache_fetch($action);\n \t} else {\n \t\t$actions{$action}->();\n@@ -3516,7 +3516,7 @@ sub git_header_html {\n \tif (defined $cgi->http('HTTP_ACCEPT') &&\n \t $cgi->http('HTTP_ACCEPT') =~ m/(,|;|\\s|^)application\\/xhtml\\+xml(,|;|\\s|$)/ &&\n \t $cgi->Accept('application/xhtml+xml') != 0 &&\n-\t !$caching_enabled) {\n+\t !$cache_enable) {\n \t\t$content_type = 'application/xhtml+xml';\n \t} else {\n \t\t$content_type = 'text/html';\n@@ -3757,7 +3757,7 @@ sub die_error {\n \t#\n \t# Reset the output so that we are actually going to STDOUT as opposed\n \t# to buffering the output.\n-\treset_output() if ($caching_enabled);\n+\treset_output() if ($cache_enable && ! $cacheErrorCache);\n \n \tgit_header_html($http_responses{$status}, undef, %opts);\n \tprint <', $fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n \t}else{\n \t\topen BINOUT, '>&', \\$fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n@@ -5960,7 +5960,7 @@ sub git_snapshot {\n \n \topen my $fd, \"-|\", $cmd\n \t\tor die_error(500, \"Execute git-archive failed\");\n-\tif ($caching_enabled) {\n+\tif ($cache_enable) {\n \t\topen BINOUT, '>', $fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n \t}else{\n \t\topen BINOUT, '>&', \\$fullhashbinpath or die_error(500, \"Could not open bin dump file\");\ndiff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh\nindex 10c4a3d..a0f7696 100755\n--- a/t/gitweb-lib.sh\n+++ b/t/gitweb-lib.sh\n@@ -56,7 +56,7 @@ EOF\n gitweb_enable_caching () {\n \ttest_expect_success 'enable caching' '\n \t\tcat >>gitweb_config.perl <<-\\EOF &&\n-\t\tour $caching_enabled = 1;\n+\t\tour $cache_enable = 1;\n \t\tour $minCacheTime = 60*60*24*7*30; # very long expiration time for tests (a month)\n \t\tour $maxCacheTime = 60*60*24*7*30*365; # upper bound for dynamic (adaptive) caching\n \t\tour $cachedir = \"cache\"; # for testsuite to clear the right thing\n-- \n1.7.2.3\n"},{"id":"157743","messageId":"1291931844-28454-9-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 08/18] gitweb: Change is_cacheable() to return true always","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:14Z","receivedAt":"2010-12-09T21:57:14Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"is_cacheable() was set to return false for blame or blame_incremental\nwhich both use unique urls so there's no reason this shouldn't pass\nthrough the caching engine.\n\nLeaving the function in place for now should something actually arrise\nthat we can't use caching for (think ajaxy kinda things likely).\n\nSigned-off-by: John 'Warthog9' Hawley \n---\n gitweb/gitweb.perl | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 5eb0309..1d8bc74 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -798,7 +798,8 @@ our %actions = (\n );\n sub is_cacheable {\n \tmy $action = shift;\n-\treturn !($action eq 'blame_data' || $action eq 'blame_incremental');\n+\t# There are no known actions that do no involve a unique URL that shouldn't be cached.\n+\treturn 1;\n }\n \n # finally, we have the hash of allowed extra_options for the commands that\n-- \n1.7.2.3\n"},{"id":"157741","messageId":"1291931844-28454-10-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 09/18] gitweb: Revert reset_output() back to original code","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:15Z","receivedAt":"2010-12-09T21:57:15Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"Reverted change to reset_output as\n\n\topen STDOUT, \">&\", \\*STDOUT_REAL;\n\ncauses assertion failures:\n\n\tAssertion !((((s->var)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVLV)) failed: file \"scalar.xs\", line 49 at gitweb.cgi line 1221.\n\nif we encounter an error *BEFORE* we've ever changed the output.\n\nSigned-off-by: John 'Warthog9' Hawley \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 1d8bc74..e8c028b 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1225,7 +1225,7 @@ sub change_output {\n \n sub reset_output {\n \t# This basically takes STDOUT_REAL and puts it back as STDOUT\n-\topen STDOUT, \">&\", \\*STDOUT_REAL;\n+\topen(STDOUT,\">&STDOUT_REAL\");\n }\n \n sub run {\n-- \n1.7.2.3\n"},{"id":"157749","messageId":"1291931844-28454-11-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 10/18] gitweb: Adding isBinaryAction() and isFeedAction() to determine the action type","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:16Z","receivedAt":"2010-12-09T21:57:16Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"This is fairly self explanitory, these are here just to centralize the checking\nfor these types of actions, as special things need to be done with regards to\nthem inside the caching engine.\n\nisBinaryAction() returns true if the action deals with creating binary files\n(this needing :raw output)\n\nisFeedAction() returns true if the action deals with a news feed of some sort,\nbasically used to bypass the 'Generating...' message should it be a news reader\nas those will explode badly on that page.\n\nSigned-off-by: John 'Warthog9' Hawley \n---\n gitweb/lib/cache.pl | 69 ++++++++++++++++++++++++++-------------------------\n 1 files changed, 35 insertions(+), 34 deletions(-)\n\ndiff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\nindex a8ee99e..d55b572 100644\n--- a/gitweb/lib/cache.pl\n+++ b/gitweb/lib/cache.pl\n@@ -88,6 +88,34 @@ sub cache_fetch {\n \t#$actions{$action}->();\n }\n \n+sub isBinaryAction {\n+\tmy ($action) = @_;\n+\n+\tif(\n+\t\t$action eq \"snapshot\"\n+\t\t||\n+\t\t$action eq \"blob_plain\"\n+\t){\n+\t\treturn 1;\t# True\n+\t}\n+\n+\treturn 0;\t\t# False\n+}\n+\n+sub isFeedAction {\n+\tif(\n+\t\t$action eq \"atom\"\n+\t\t||\n+\t\t$action eq \"rss\"\n+\t\t||\n+\t\t$action eq \"opml\"\n+\t){\n+\t\treturn 1;\t# True\n+\t}\n+\n+\treturn 0;\t\t# False\n+}\n+\n sub cacheUpdate {\n \tmy ($action,$areForked) = @_;\n \tmy $lockingStatus;\n@@ -115,11 +143,7 @@ sub cacheUpdate {\n \t\t}\n \t}\n \n-\tif(\n-\t\t$action eq \"snapshot\"\n-\t\t||\n-\t\t$action eq \"blob_plain\"\n-\t){\n+\tif( isBinaryAction($action) ){\n \t\tmy $openstat = open(cacheFileBinWT, '>>:utf8', \"$fullhashbinpath\");\n \t\tmy $lockStatBin = flock(cacheFileBinWT,LOCK_EX|LOCK_NB);\n \t}\n@@ -146,11 +170,7 @@ sub cacheUpdate {\n \t\t}\n \t}\n \n-\tif(\n-\t\t$action eq \"snapshot\"\n-\t\t||\n-\t\t$action eq \"blob_plain\"\n-\t){\n+\tif( isBinaryAction($action) ){\n \t\tmy $openstat = open(cacheFileBinFINAL, '>:utf8', \"$fullhashbinpathfinal\");\n \t\t$lockStatBIN = flock(cacheFileBinFINAL,LOCK_EX);\n \n@@ -168,11 +188,7 @@ sub cacheUpdate {\n \t$|++;\n \tprint cacheFile \"$output\";\n \t$|--;\n-\tif(\n-\t\t$action eq \"snapshot\"\n-\t\t||\n-\t\t$action eq \"blob_plain\"\n-\t){\n+\tif( isBinaryAction($action) ){\n \t\tmove(\"$fullhashbinpath\", \"$fullhashbinpathfinal\") or die \"Binary Cache file could not be updated: $!\";\n \n \t\tflock(cacheFileBinFINAL,LOCK_UN);\n@@ -219,14 +235,10 @@ sub cacheWaitForUpdate {\n \t}\n \n \tif(\n-\t\t$action eq \"atom\"\n-\t\t||\n-\t\t$action eq \"rss\"\n-\t\t||\n-\t\t$action eq \"opml\"\n+\t\tisFeedAction($action)\n \t\t||\n \t\t! $cacheGenStatus\n-\t){\n+\t ){\n \t\tdo {\n \t\t\tsleep 2 if $x > 0;\n \t\t\topen(cacheFile, '<:utf8', \"$fullhashpath\");\n@@ -310,17 +322,10 @@ sub cacheDisplay {\n \t\tcacheWaitForUpdate($action);\n \t}\n \n-\tif(\n-\t\t(\n-\t\t\t$action eq \"snapshot\"\n-\t\t\t||\n-\t\t\t$action eq \"blob_plain\"\n-\t\t)\n-\t){\n+\tif( isBinaryAction($action) ){\n \t\tmy $openstat = open(cacheFileBin, '<', \"$fullhashbinpathfinal\");\n \t\t$lockStatBIN = flock(cacheFileBin,LOCK_SH|LOCK_NB);\n \t\tif (! $lockStatBIN ){\n-\t\t\tsystem (\"echo 'cacheDisplay - bailing due to binary lock failure' >> /tmp/gitweb.log\");\n \t\t\tclose(cacheFile);\n \t\t\tclose(cacheFileBin);\n \t\t\tcacheWaitForUpdate($action);\n@@ -332,11 +337,7 @@ sub cacheDisplay {\n \twhile( ){\n \t\tprint $_;\n \t}\n-\tif(\n-\t\t$action eq \"snapshot\"\n-\t\t||\n-\t\t$action eq \"blob_plain\"\n-\t){\n+\tif( isBinaryAction($action) ){\n \t\tbinmode STDOUT, ':raw';\n \t\tprint ;\n \t\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n-- \n1.7.2.3\n"},{"id":"157750","messageId":"1291931844-28454-12-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 11/18] gitweb: add isDumbClient() check","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:17Z","receivedAt":"2010-12-09T21:57:17Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"Basic check for the claimed Agent string, if it matches a known\nblacklist (wget and curl currently) don't display the 'Generating...'\npage.\n\nJakub has mentioned a couple of other possible ways to handle\nthis, so if a better way comes along this should be used as a\nwrapper to any better way we can find to deal with this.\n\nSigned-off-by: John 'Warthog9' Hawley \n---\n gitweb/lib/cache.pl | 30 ++++++++++++++++++++++++++++++\n 1 files changed, 30 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\nindex d55b572..5182a94 100644\n--- a/gitweb/lib/cache.pl\n+++ b/gitweb/lib/cache.pl\n@@ -116,6 +116,34 @@ sub isFeedAction {\n \treturn 0;\t\t# False\n }\n \n+# There have been a number of requests that things like \"dumb\" clients, I.E. wget\n+# lynx, links, etc (things that just download, but don't parse the html) actually\n+# work without getting the wonkiness that is the \"Generating...\" page.\n+#\n+# There's only one good way to deal with this, and that's to read the browser User\n+# Agent string and do matching based on that. This has a whole slew of error cases\n+# and mess, but there's no other way to determine if the \"Generating...\" page\n+# will break things.\n+#\n+# This assumes the client is not dumb, thus the default behavior is to return\n+# \"false\" (0) (and eventually the \"Generating...\" page). If it is a dumb client\n+# return \"true\" (1)\n+sub isDumbClient {\n+\tmy($user_agent) = $ENV{'HTTP_USER_AGENT'};\n+\t\n+\tif(\n+\t\t# wget case\n+\t\t$user_agent =~ /^Wget/i\n+\t\t||\n+\t\t# curl should be excluded I think, probably better safe than sorry\n+\t\t$user_agent =~ /^curl/i\n+\t ){\n+\t\treturn 1;\t# True\n+\t}\n+\n+\treturn 0;\n+}\n+\n sub cacheUpdate {\n \tmy ($action,$areForked) = @_;\n \tmy $lockingStatus;\n@@ -237,6 +265,8 @@ sub cacheWaitForUpdate {\n \tif(\n \t\tisFeedAction($action)\n \t\t||\n+\t\tisDumbClient()\n+\t\t||\n \t\t! $cacheGenStatus\n \t ){\n \t\tdo {\n-- \n1.7.2.3\n"},{"id":"157747","messageId":"1291931844-28454-13-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 12/18] gitweb: Change file handles (in caching) to lexical variables as opposed to globs","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:18Z","receivedAt":"2010-12-09T21:57:18Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"This isn't a huge change, it just adds global variables for the file handles,\nan additional cleanup to localize the variable a bit more which should alleviate\nthe issues that Jakub had with my original approach.\n\nSigned-off-by: John 'Warthog9' Hawley \n---\n gitweb/lib/cache.pl | 114 +++++++++++++++++++++++++++++++-------------------\n 1 files changed, 71 insertions(+), 43 deletions(-)\n\ndiff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\nindex 5182a94..fafc028 100644\n--- a/gitweb/lib/cache.pl\n+++ b/gitweb/lib/cache.pl\n@@ -14,6 +14,12 @@ use Digest::MD5 qw(md5 md5_hex md5_base64);\n use Fcntl ':flock';\n use File::Copy;\n \n+# Global declarations\n+our $cacheFile;\n+our $cacheFileBG;\n+our $cacheFileBinWT;\n+our $cacheFileBin;\n+\n sub cache_fetch {\n \tmy ($action) = @_;\n \tmy $cacheTime = 0;\n@@ -49,9 +55,9 @@ sub cache_fetch {\n \t}else{\n \t\t#if cache is out dated, update\n \t\t#else displayCache();\n-\t\topen(cacheFile, '<', \"$fullhashpath\");\n-\t\tstat(cacheFile);\n-\t\tclose(cacheFile);\n+\t\topen($cacheFile, '<', \"$fullhashpath\");\n+\t\tstat($cacheFile);\n+\t\tclose($cacheFile);\n \t\tmy $stat_time = (stat(_))[9];\n \t\tmy $stat_size = (stat(_))[7];\n \n@@ -150,13 +156,13 @@ sub cacheUpdate {\n \tmy $fileData = \"\";\n \n \tif($backgroundCache){\n-\t\topen(cacheFileBG, '>:utf8', \"$fullhashpath.bg\");\n-\t\tmy $lockStatBG = flock(cacheFileBG,LOCK_EX|LOCK_NB);\n+\t\topen($cacheFileBG, '>:utf8', \"$fullhashpath.bg\");\n+\t\tmy $lockStatBG = flock($cacheFileBG,LOCK_EX|LOCK_NB);\n \n \t\t$lockStatus = $lockStatBG;\n \t}else{\n-\t\topen(cacheFile, '>:utf8', \\$fullhashpath);\n-\t\tmy $lockStat = flock(cacheFile,LOCK_EX|LOCK_NB);\n+\t\topen($cacheFile, '>:utf8', \\$fullhashpath);\n+\t\tmy $lockStat = flock($cacheFile,LOCK_EX|LOCK_NB);\n \n \t\t$lockStatus = $lockStat;\n \t}\n@@ -172,8 +178,8 @@ sub cacheUpdate {\n \t}\n \n \tif( isBinaryAction($action) ){\n-\t\tmy $openstat = open(cacheFileBinWT, '>>:utf8', \"$fullhashbinpath\");\n-\t\tmy $lockStatBin = flock(cacheFileBinWT,LOCK_EX|LOCK_NB);\n+\t\tmy $openstat = open($cacheFileBinWT, '>>:utf8', \"$fullhashbinpath\");\n+\t\tmy $lockStatBin = flock($cacheFileBinWT,LOCK_EX|LOCK_NB);\n \t}\n \n \t# Trap all output from the action\n@@ -186,8 +192,8 @@ sub cacheUpdate {\n \n \n \tif($backgroundCache){\n-\t\topen(cacheFile, '>:utf8', \"$fullhashpath\");\n-\t\t$lockStat = flock(cacheFile,LOCK_EX);\n+\t\topen($cacheFile, '>:utf8', \"$fullhashpath\");\n+\t\t$lockStat = flock($cacheFile,LOCK_EX);\n \n \t\tif (! $lockStat ){\n \t\t\tif ( $areForked ){\n@@ -199,8 +205,8 @@ sub cacheUpdate {\n \t}\n \n \tif( isBinaryAction($action) ){\n-\t\tmy $openstat = open(cacheFileBinFINAL, '>:utf8', \"$fullhashbinpathfinal\");\n-\t\t$lockStatBIN = flock(cacheFileBinFINAL,LOCK_EX);\n+\t\tmy $openstat = open($cacheFileBinFINAL, '>:utf8', \"$fullhashbinpathfinal\");\n+\t\t$lockStatBIN = flock($cacheFileBinFINAL,LOCK_EX);\n \n \t\tif (! $lockStatBIN ){\n \t\t\tif ( $areForked ){\n@@ -214,24 +220,24 @@ sub cacheUpdate {\n \t# Actually dump the output to the proper file handler\n \tlocal $/ = undef;\n \t$|++;\n-\tprint cacheFile \"$output\";\n+\tprint $cacheFile \"$output\";\n \t$|--;\n \tif( isBinaryAction($action) ){\n \t\tmove(\"$fullhashbinpath\", \"$fullhashbinpathfinal\") or die \"Binary Cache file could not be updated: $!\";\n \n-\t\tflock(cacheFileBinFINAL,LOCK_UN);\n-\t\tclose(cacheFileBinFINAL);\n+\t\tflock($cacheFileBinFINAL,LOCK_UN);\n+\t\tclose($cacheFileBinFINAL);\n \n-\t\tflock(cacheFileBinWT,LOCK_UN);\n-\t\tclose(cacheFileBinWT);\n+\t\tflock($cacheFileBinWT,LOCK_UN);\n+\t\tclose($cacheFileBinWT);\n \t}\n \n-\tflock(cacheFile,LOCK_UN);\n-\tclose(cacheFile);\n+\tflock($cacheFile,LOCK_UN);\n+\tclose($cacheFile);\n \n \tif($backgroundCache){\n-\t\tflock(cacheFileBG,LOCK_UN);\n-\t\tclose(cacheFileBG);\n+\t\tflock($cacheFileBG,LOCK_UN);\n+\t\tclose($cacheFileBG);\n \t}\n \n \tif ( $areForked ){\n@@ -250,10 +256,10 @@ sub cacheWaitForUpdate {\n \n \tif( $backgroundCache ){\n \t\tif( -e \"$fullhashpath\" ){\n-\t\t\topen(cacheFile, '<:utf8', \"$fullhashpath\");\n-\t\t\t$lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);\n-\t\t\tstat(cacheFile);\n-\t\t\tclose(cacheFile);\n+\t\t\topen($cacheFile, '<:utf8', \"$fullhashpath\");\n+\t\t\t$lockStat = flock($cacheFile,LOCK_SH|LOCK_NB);\n+\t\t\tstat($cacheFile);\n+\t\t\tclose($cacheFile);\n \n \t\t\tif( $lockStat && ( (stat(_))[9] > (time - $maxCacheLife) ) ){\n \t\t\t\tcacheDisplay($action);\n@@ -271,9 +277,9 @@ sub cacheWaitForUpdate {\n \t ){\n \t\tdo {\n \t\t\tsleep 2 if $x > 0;\n-\t\t\topen(cacheFile, '<:utf8', \"$fullhashpath\");\n-\t\t\t$lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);\n-\t\t\tclose(cacheFile);\n+\t\t\topen($cacheFile, '<:utf8', \"$fullhashpath\");\n+\t\t\t$lockStat = flock($cacheFile,LOCK_SH|LOCK_NB);\n+\t\t\tclose($cacheFile);\n \t\t\t$x++;\n \t\t\t$combinedLockStat = $lockStat;\n \t\t} while ((! $combinedLockStat) && ($x < $max));\n@@ -326,9 +332,9 @@ EOF\n \tdo {\n \t\tprint \".\";\n \t\tsleep 2 if $x > 0;\n-\t\topen(cacheFile, '<:utf8', \"$fullhashpath\");\n-\t\t$lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);\n-\t\tclose(cacheFile);\n+\t\topen($cacheFile, '<:utf8', \"$fullhashpath\");\n+\t\t$lockStat = flock($cacheFile,LOCK_SH|LOCK_NB);\n+\t\tclose($cacheFile);\n \t\t$x++;\n \t\t$combinedLockStat = $lockStat;\n \t} while ((! $combinedLockStat) && ($x < $max));\n@@ -339,41 +345,63 @@ EOF\n \treturn;\n }\n \n+sub cacheDisplayErr {\n+\n+\treturn if ( ! -e \"$fullhashpath.err\" );\n+\n+\topen($cacheFileErr, '<:utf8', \"$fullhashpath.err\");\n+\t$lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);\n+\n+\tif (! $lockStatus ){\n+\t\tshow_warning(\n+\t\t\t\t\"

\".\n+\t\t\t\t\"*** Warning ***: Locking error when trying to lock error cache page, file $fullhashpath.err
/\\n\".\n+\t\t\t\t\"This is about as screwed up as it gets folks - see your systems administrator for more help with this.\".\n+\t\t\t\t\"

\"\n+\t\t\t\t);\n+\t}\n+\n+\twhile( <$cacheFileErr> ){\n+\t\tprint $_;\n+\t}\n+\texit(0);\n+}\n+\n sub cacheDisplay {\n \tlocal $/ = undef;\n \t$|++;\n \n \tmy ($action) = @_;\n-\topen(cacheFile, '<:utf8', \"$fullhashpath\");\n-\t$lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);\n+\topen($cacheFile, '<:utf8', \"$fullhashpath\");\n+\t$lockStat = flock($cacheFile,LOCK_SH|LOCK_NB);\n \n \tif (! $lockStat ){\n-\t\tclose(cacheFile);\n+\t\tclose($cacheFile);\n \t\tcacheWaitForUpdate($action);\n \t}\n \n \tif( isBinaryAction($action) ){\n-\t\tmy $openstat = open(cacheFileBin, '<', \"$fullhashbinpathfinal\");\n-\t\t$lockStatBIN = flock(cacheFileBin,LOCK_SH|LOCK_NB);\n+\t\tmy $openstat = open($cacheFileBin, '<', \"$fullhashbinpathfinal\");\n+\t\t$lockStatBIN = flock($cacheFileBin,LOCK_SH|LOCK_NB);\n \t\tif (! $lockStatBIN ){\n-\t\t\tclose(cacheFile);\n-\t\t\tclose(cacheFileBin);\n+\t\t\tclose($cacheFile);\n+\t\t\tclose($cacheFileBin);\n \t\t\tcacheWaitForUpdate($action);\n \t\t}\n \n \t\tmy $binfilesize = -s \"$fullhashbinpathfinal\";\n \t\tprint \"Content-Length: $binfilesize\";\n \t}\n-\twhile( ){\n+\twhile( <$cacheFile> ){\n \t\tprint $_;\n \t}\n \tif( isBinaryAction($action) ){\n \t\tbinmode STDOUT, ':raw';\n-\t\tprint ;\n+\t\tprint <$cacheFileBin>;\n \t\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n-\t\tclose(cacheFileBin);\n+\t\tclose($cacheFileBin);\n \t}\n-\tclose(cacheFile);\n+\tclose($cacheFile);\n \t$|--;\n }\n \n-- \n1.7.2.3\n"},{"id":"157748","messageId":"1291931844-28454-14-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 13/18] gitweb: Add commented url & url hash to page footer","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:19Z","receivedAt":"2010-12-09T21:57:19Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"This is mostly a debugging tool, but it adds a small bit of information\nto the footer:\n\n\n\nThe first bit tells you what the url that generated the page actually was, the second is\nthe hash used to store the file with the first two characters being used as the directory:\n\n/7a/31cfb8a43f5643679eec88aa9d7981\n\nAlso useful for greping through the existing cache and finding files with unique paths that\nyou may want to explicitly flush.\n\nSigned-off-by: John 'Warthog9' Hawley \n---\n gitweb/gitweb.perl | 7 +++++++\n gitweb/lib/cache.pl | 4 ++--\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e8c028b..7f8292e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -303,6 +303,9 @@ our $fullhashpath = *STDOUT;\n our $fullhashbinpath = *STDOUT;\n our $fullhashbinpathfinal = *STDOUT;\n \n+our $full_url;\n+our $urlhash;\n+\n # configuration for 'highlight' (http://www.andre-simon.de/)\n # match by basename\n our %highlight_basename = (\n@@ -3663,6 +3666,10 @@ sub git_footer_html {\n \n \tprint \"

\\n\";\n \tprint \"
Cache Last Updated: \". gmtime( time ) .\" GMT
\\n\";\n+\tprint\t\"\\n\" if ($cache_enable);\n \tif (defined $project) {\n \t\tmy $descr = git_get_project_description($project);\n \t\tif (defined $descr) {\ndiff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\nindex fafc028..63dbe9e 100644\n--- a/gitweb/lib/cache.pl\n+++ b/gitweb/lib/cache.pl\n@@ -30,8 +30,8 @@ sub cache_fetch {\n \t\tprint \"Cache directory created successfully\\n\";\n \t}\n \n-\tour $full_url = \"$my_url?\". $ENV{'QUERY_STRING'};\n-\tour $urlhash = md5_hex($full_url);\n+\t$full_url = \"$my_url?\". $ENV{'QUERY_STRING'};\n+\t$urlhash = md5_hex($full_url);\n \tour $fullhashdir = \"$cachedir/\". substr( $urlhash, 0, 2) .\"/\";\n \n \teval { mkpath( $fullhashdir, 0, 0777 ) };\n-- \n1.7.2.3\n"},{"id":"157745","messageId":"1291931844-28454-15-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 14/18] gitweb: add print_transient_header() function for central header printing","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:20Z","receivedAt":"2010-12-09T21:57:20Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"There are a few things I would like to reuse the transient header\ninformation I'm using, currently this is only the 'Generating...'\npage, but there is at least one additional warning page I would\nlike to use this on.\n\nSigned-off-by: John 'Warthog9' Hawley \n---\n gitweb/lib/cache.pl | 47 ++++++++++++++++++++++++++---------------------\n 1 files changed, 26 insertions(+), 21 deletions(-)\n\ndiff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\nindex 63dbe9e..723ae9b 100644\n--- a/gitweb/lib/cache.pl\n+++ b/gitweb/lib/cache.pl\n@@ -94,6 +94,31 @@ sub cache_fetch {\n \t#$actions{$action}->();\n }\n \n+sub print_transient_header {\n+\tprint $::cgi->header(\n+\t\t\t\t-type=>'text/html',\n+\t\t\t\t-charset => 'utf-8',\n+\t\t\t\t-status=> 200,\n+\t\t\t\t-expires => 'now',\n+\t\t\t\t# HTTP/1.0\n+\t\t\t\t-Pragma => 'no-cache',\n+\t\t\t\t# HTTP/1.1\n+\t\t\t\t-Cache_Control => join(\n+\t\t\t\t\t\t\t', ',\n+\t\t\t\t\t\t\tqw(\n+\t\t\t\t\t\t\t\tprivate\n+\t\t\t\t\t\t\t\tno-cache\n+\t\t\t\t\t\t\t\tno-store\n+\t\t\t\t\t\t\t\tmust-revalidate\n+\t\t\t\t\t\t\t\tmax-age=0\n+\t\t\t\t\t\t\t\tpre-check=0\n+\t\t\t\t\t\t\t\tpost-check=0\n+\t\t\t\t\t\t\t)\n+\t\t\t\t\t\t)\n+\t\t\t\t);\n+\treturn;\n+}\n+\n sub isBinaryAction {\n \tmy ($action) = @_;\n \n@@ -292,27 +317,7 @@ sub cacheWaitForUpdate {\n \n \t$| = 1;\n \n-\tprint $::cgi->header(\n-\t\t\t\t-type=>'text/html',\n-\t\t\t\t-charset => 'utf-8',\n-\t\t\t\t-status=> 200,\n-\t\t\t\t-expires => 'now',\n-\t\t\t\t# HTTP/1.0\n-\t\t\t\t-Pragma => 'no-cache',\n-\t\t\t\t# HTTP/1.1\n-\t\t\t\t-Cache_Control => join(\n-\t\t\t\t\t\t\t', ',\n-\t\t\t\t\t\t\tqw(\n-\t\t\t\t\t\t\t\tprivate\n-\t\t\t\t\t\t\t\tno-cache\n-\t\t\t\t\t\t\t\tno-store\n-\t\t\t\t\t\t\t\tmust-revalidate\n-\t\t\t\t\t\t\t\tmax-age=0\n-\t\t\t\t\t\t\t\tpre-check=0\n-\t\t\t\t\t\t\t\tpost-check=0\n-\t\t\t\t\t\t\t)\n-\t\t\t\t\t\t)\n-\t\t\t\t);\n+\tprint_transient_header();\n \n \tprint <\n-- \n1.7.2.3\n"},{"id":"157753","messageId":"1291931844-28454-16-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 15/18] gitweb: Add show_warning() to display an immediate warning, with refresh","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:21Z","receivedAt":"2010-12-09T21:57:21Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"die_error() is an immediate and abrupt action. show_warning() more or less\nfunctions identically, except that the page generated doesn't use the\ngitweb header or footer (in case they are broken) and has an auto-refresh\n(10 seconds) built into it.\n\nThis makes use of print_transient_header() which is also used in the\n'Generating...' page. Currently the only warning it throws is about\nthe cache needing to be created. If that fails it's a fatal error\nand we call die_error()\n\nSigned-off-by: John 'Warthog9' Hawley \n---\n gitweb/lib/cache.pl | 36 +++++++++++++++++++++++++++++++++---\n 1 files changed, 33 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\nindex 723ae9b..28e4240 100644\n--- a/gitweb/lib/cache.pl\n+++ b/gitweb/lib/cache.pl\n@@ -25,9 +25,13 @@ sub cache_fetch {\n \tmy $cacheTime = 0;\n \n \tif(! -d $cachedir){\n-\t\tprint \"*** Warning ***: Caching enabled but cache directory does not exsist. ($cachedir)\\n\";\n-\t\tmkdir (\"cache\", 0755) || die \"Cannot create cache dir - you will need to manually create\";\n-\t\tprint \"Cache directory created successfully\\n\";\n+\t\tmkdir (\"cache\", 0755) || die_error(500, \"Internal Server Error\", \"Cannot create cache dir () - you will need to manually create\");\n+\t\tshow_warning(\n+\t\t\t\t\"

\".\n+\t\t\t\t\"*** Warning ***: Caching enabled but cache directory did not exsist. ($cachedir)
/\\n\".\n+\t\t\t\t\"Cache directory created successfully\\n\".\n+\t\t\t\t\"

\"\n+\t\t\t\t);\n \t}\n \n \t$full_url = \"$my_url?\". $ENV{'QUERY_STRING'};\n@@ -119,6 +123,32 @@ sub print_transient_header {\n \treturn;\n }\n \n+sub show_warning {\n+\t$| = 1;\n+\n+\tmy $warning = esc_html(shift) || \"Unknown Warning\";\n+\n+\tprint_transient_header();\n+\n+\tprint <\n+\n+\n+\n+\n+\n+\n+\n+$title\n+\n+\n+$warning\n+\n+\n+EOF\n+\texit(0);\n+}\n+\n sub isBinaryAction {\n \tmy ($action) = @_;\n \n-- \n1.7.2.3\n"},{"id":"157746","messageId":"1291931844-28454-17-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 16/18] gitweb: When changing output (STDOUT) change STDERR as well","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:22Z","receivedAt":"2010-12-09T21:57:22Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"This sets up a trap for STDERR as well as STDOUT. This should\nprevent any transient error messages from git itself percolating\nup to gitweb and outputting errant information before the HTTP\nheader has been sent.\n\nSigned-off-by: John 'Warthog9' Hawley \n---\n gitweb/gitweb.perl | 22 +++++++++++++++++++++-\n gitweb/lib/cache.pl | 22 ----------------------\n 2 files changed, 21 insertions(+), 23 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7f8292e..d39982a 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1214,6 +1214,10 @@ sub evaluate_argv {\n sub change_output {\n \tour $output;\n \n+\t#\n+\t# STDOUT\n+\t#\n+\n \t# Trap the 'proper' STDOUT to STDOUT_REAL for things like error messages and such\n \topen(STDOUT_REAL,\">&STDOUT\") or die \"Unable to capture STDOUT $!\\n\";\n \tprint STDOUT_REAL \"\";\n@@ -1223,12 +1227,28 @@ sub change_output {\n \n \t# Trap STDOUT to the $output variable, which is what I was using in the original\n \t# patch anyway.\n-\topen(STDOUT,\">\", \\$output) || die \"Unable to open STDOUT: $!\"; #open STDOUT handle to use $var\n+\topen(STDOUT,\">\", \\$output) || die \"Unable to open STDOUT: $!\"; #open STDOUT handle to use $output\n+\n+\t#\n+\t# STDERR\n+\t#\n+\n+\t# Trap the 'proper' STDOUT to STDOUT_REAL for things like error messages and such\n+\topen(STDERR_REAL,\">&STDERR\") or die \"Unable to capture STDERR $!\\n\";\n+\tprint STDERR_REAL \"\";\n+\n+\t# Close STDOUT, so that it isn't being used anymore.\n+\tclose STDERR;\n+\n+\t# Trap STDOUT to the $output variable, which is what I was using in the original\n+\t# patch anyway.\n+\topen(STDERR,\">\", \\$output_err) || die \"Unable to open STDERR: $!\"; #open STDERR handle to use $output_err\n }\n \n sub reset_output {\n \t# This basically takes STDOUT_REAL and puts it back as STDOUT\n \topen(STDOUT,\">&STDOUT_REAL\");\n+\topen(STDERR,\">&STDERR_REAL\");\n }\n \n sub run {\ndiff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\nindex 28e4240..a8c902d 100644\n--- a/gitweb/lib/cache.pl\n+++ b/gitweb/lib/cache.pl\n@@ -380,28 +380,6 @@ EOF\n \treturn;\n }\n \n-sub cacheDisplayErr {\n-\n-\treturn if ( ! -e \"$fullhashpath.err\" );\n-\n-\topen($cacheFileErr, '<:utf8', \"$fullhashpath.err\");\n-\t$lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);\n-\n-\tif (! $lockStatus ){\n-\t\tshow_warning(\n-\t\t\t\t\"

\".\n-\t\t\t\t\"*** Warning ***: Locking error when trying to lock error cache page, file $fullhashpath.err
/\\n\".\n-\t\t\t\t\"This is about as screwed up as it gets folks - see your systems administrator for more help with this.\".\n-\t\t\t\t\"

\"\n-\t\t\t\t);\n-\t}\n-\n-\twhile( <$cacheFileErr> ){\n-\t\tprint $_;\n-\t}\n-\texit(0);\n-}\n-\n sub cacheDisplay {\n \tlocal $/ = undef;\n \t$|++;\n-- \n1.7.2.3\n"},{"id":"157752","messageId":"1291931844-28454-18-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 17/18] gitweb: Prepare for cached error pages & better error page handling","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:23Z","receivedAt":"2010-12-09T21:57:23Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"To quote myself from an e-mail of mine:\n\n\tI've got a hammer, it clearly solves all problems!\n\nThis is the prepatory work to set up a mechanism inside the\ncaching engine to cache the error pages instead of throwing\nthem straight out to the client.\n\nThis adds two functions:\n\ndie_error_cache() - this gets back called from die_error() so\nthat the error message generated can be cached.\n\ncacheDisplayErr() - this is a simplified version of cacheDisplay()\nthat does an initial check, if the error page exists - display it\nand exit. If not, return.\n\nSigned-off-by: John 'Warthog9' Hawley \n---\n gitweb/lib/cache.pl | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 52 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\nindex a8c902d..6cb82c8 100644\n--- a/gitweb/lib/cache.pl\n+++ b/gitweb/lib/cache.pl\n@@ -302,6 +302,36 @@ sub cacheUpdate {\n \t}\n }\n \n+sub die_error_cache {\n+\tmy ($output) = @_;\n+\n+\topen(my $cacheFileErr, '>:utf8', \"$fullhashpath.err\");\n+\tmy $lockStatus = flock($cacheFileErr,LOCK_EX|LOCK_NB);\n+\n+\tif (! $lockStatus ){\n+\t\tif ( $areForked ){\n+\t\t\texit(0);\n+\t\t}else{\n+\t\t\treturn;\n+\t\t}\n+\t}\n+\n+\t# Actually dump the output to the proper file handler\n+\tlocal $/ = undef;\n+\t$|++;\n+\tprint $cacheFileErr \"$output\";\n+\t$|--;\n+\n+\tflock($cacheFileErr,LOCK_UN);\n+\tclose($cacheFileErr);\n+\n+\tif ( $areForked ){\n+\t\texit(0);\n+\t}else{\n+\t\treturn;\n+\t}\n+}\n+\n \n sub cacheWaitForUpdate {\n \tmy ($action) = @_;\n@@ -380,6 +410,28 @@ EOF\n \treturn;\n }\n \n+sub cacheDisplayErr {\n+\n+\treturn if ( ! -e \"$fullhashpath.err\" );\n+\n+\topen($cacheFileErr, '<:utf8', \"$fullhashpath.err\");\n+\t$lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);\n+\n+\tif (! $lockStatus ){\n+\t\tshow_warning(\n+\t\t\t\t\"

\".\n+\t\t\t\t\"*** Warning ***: Locking error when trying to lock error cache page, file $fullhashpath.err
/\\n\".\n+\t\t\t\t\"This is about as screwed up as it gets folks - see your systems administrator for more help with this.\".\n+\t\t\t\t\"

\"\n+\t\t\t\t);\n+\t}\n+\n+\twhile( <$cacheFileErr> ){\n+\t\tprint $_;\n+\t}\n+\texit(0);\n+}\n+\n sub cacheDisplay {\n \tlocal $/ = undef;\n \t$|++;\n-- \n1.7.2.3\n"},{"id":"157751","messageId":"1291931844-28454-19-git-send-email-warthog9@eaglescrag.net","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"[PATCH 18/18] gitweb: Add better error handling for gitweb caching","fromName":"John 'Warthog9' Hawley","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-09T21:57:24Z","receivedAt":"2010-12-09T21:57:24Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"This basically finishes the plumbing for caching the error pages\nas the are generated.\n\nIf an error is hit, create a .err file with the error. This\nwill interrupt all currently waiting processes and they will display\nthe error, without any additional refreshing.\n\nOn a new request a generation will be attempted, should it succed the\n file is removed (if it exists).\n\nSigned-off-by: John 'Warthog9' Hawley \n---\n gitweb/gitweb.perl | 8 ++++++++\n gitweb/lib/cache.pl | 14 ++++++++++++++\n 2 files changed, 22 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex d39982a..5a9660a 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -41,6 +41,7 @@ our $version = \"++GIT_VERSION++\";\n \n # Output buffer variable\n our $output = \"\";\n+our $output_err = \"\";\n \n our ($my_url, $my_uri, $base_url, $path_info, $home_link);\n sub evaluate_uri {\n@@ -303,6 +304,9 @@ our $fullhashpath = *STDOUT;\n our $fullhashbinpath = *STDOUT;\n our $fullhashbinpathfinal = *STDOUT;\n \n+our $cacheErrorCache = 0; # false\n+our $cacheErrorCount = 0;\n+\n our $full_url;\n our $urlhash;\n \n@@ -3786,6 +3790,7 @@ sub die_error {\n \t# Reset the output so that we are actually going to STDOUT as opposed\n \t# to buffering the output.\n \treset_output() if ($cache_enable && ! $cacheErrorCache);\n+\t$cacheErrorCount++ if( $cacheErrorCache );\n \n \tgit_header_html($http_responses{$status}, undef, %opts);\n \tprint <\\n\";\n \n \tgit_footer_html();\n+\n+\tdie_error_cache($output) if ( $cacheErrorCache );\n+\n \tgoto DONE_GITWEB\n \t\tunless ($opts{'-error_handler'});\n }\ndiff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\nindex 6cb82c8..2e7ca69 100644\n--- a/gitweb/lib/cache.pl\n+++ b/gitweb/lib/cache.pl\n@@ -240,8 +240,14 @@ sub cacheUpdate {\n \t# Trap all output from the action\n \tchange_output();\n \n+\t# Set the error handler so we cache\n+\t$cacheErrorCache = 1; # true\n+\n \t$actions{$action}->();\n \n+\t# Reset Error Handler to not cache\n+\t$cacheErrorCache = 0; # false\n+\n \t# Reset the outputs as we should be fine now\n \treset_output();\n \n@@ -295,6 +301,8 @@ sub cacheUpdate {\n \t\tclose($cacheFileBG);\n \t}\n \n+\tunlink(\"$fullhashpath.err\") if (-e \"$fullhashpath.err\");\n+\n \tif ( $areForked ){\n \t\texit(0);\n \t} else {\n@@ -339,6 +347,9 @@ sub cacheWaitForUpdate {\n \tmy $max = 10;\n \tmy $lockStat = 0;\n \n+\t# Call cacheDisplayErr - if an error exists it will display and die. If not it will just return\n+\tcacheDisplayErr($action);\n+\n \tif( $backgroundCache ){\n \t\tif( -e \"$fullhashpath\" ){\n \t\t\topen($cacheFile, '<:utf8', \"$fullhashpath\");\n@@ -402,6 +413,9 @@ EOF\n \t\tclose($cacheFile);\n \t\t$x++;\n \t\t$combinedLockStat = $lockStat;\n+\n+\t\t# Call cacheDisplayErr - if an error exists it will display and die. If not it will just return\n+\t\tcacheDisplayErr($action);\n \t} while ((! $combinedLockStat) && ($x < $max));\n \tprint <\n-- \n1.7.2.3\n"},{"id":"157762","messageId":"m3bp4u34vj.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 00/18] Gitweb caching v8","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-09T23:26:59Z","receivedAt":"2010-12-09T23:26:59Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"John, could you please in the future Cc me? I am interested in gitweb\noutput caching development. Thanks in advance.\n\n\"John 'Warthog9' Hawley\" writes:\n\n> Afternoon everyone,\n> \n> (Afternoon is like morning, right?)\n> \n> This is the latest incarnation of gitweb w/ caching. Per the general\n> consensus and requests from the recent GitTogether I'm re-submitting \n> my patches.\n> \n> Bunch of re-works in the code, and several requested features. Sadly the\n> patch series has balloned as I've been adding things. It was 3-4 patches,\n> it's now 18. This is based on top of Jakub's v7.2 patch series, but\n> it should be more or less clean now.\n\nCould you please rebase it on top of v7.2 version? The v7.2 patch\nseries contained a few bugs that needs to be corrected.\n\n> \n> As such there was a bunch of changes that I needed to do to Jakub's tree\n> which are indicated in the series. Why did I do them up as separate things?\n> Mainly there's a bunch of history that's getting lost right now between\n> going back and forth, and I wanted to have clear patches to discuss\n> should further discussion be needed.\n\nI guess that in the final submission (i.e. the one that is to be\nmerged in into git.git repository) those changes would be squashed in,\nisn't it?\n\n> \n> This still differs, by two patches, from whats in production on kernel.org.\n> It's missing the index page git:// link, and kernel.org and kernel.org also\n> has the forced version matching. As a note I'll probably let this stew\n> another day or so on kernel.org and then I'll push it into the Fedora update\n> stream, as there's a couple of things in this patch series that would be \n> good for them to have.\n\nThere was some discussion about git:// link in the past; nevertheless\nthis issue is independent on gitweb caching and can (and should) be\nsent as a aeparate patch.\n\nIIRC we agreed that because of backward compatibility forced versions\nmatch is quite useless (in general)...\n\n> \n> There is one additional script I've written that the Fedora folks are using,\n> and that might be useful to include, which is an 'offline' cache file generator.\n> It basically wraps gitweb.cgi and at the end moves the cache file into the right\n> place. The Fedora folks were finding it took hours to generate their front\n> page, and that doing a background generation almost never completed (due to \n> process death). This was a simple way to handle that. If people would like\n> I can add it in as an additional patch.\n\nAre you detaching the background process?\n\nIt would be nice to have it as separate patch.\n\n> \n> v8:\n> \t- Reverting several changes from Jakub's change set that make no sense\n> - is_cacheable changed to always return true - nothing special about\n> blame or blame_incremental as far as the caching engine is concerned\n\n'blame_incremental' is just another version of 'blame' view. I have\ndisabled it when caching is enabled in my rewrite (you instead disabled\ncaching for 'blame_incremental' in your v7 and mine v7.x) because I\ncouldn't get it to work together with caching. Did you check that it\nworks?\n\nBesides, withou \"tee\"-ing, i.e. printing output as it is captured,\ncached 'blame_data' means that 'blame_incremental' is not incremental,\nand therefore it vanishes its advantage over 'blame'.\n\nIn the case data is in cache, then 'blame_inremental' doesn't have\nadvantage over 'blame' either.\n\n> - Reverted config file change \"caching_enabled\" back to \"cache_enable\" as this\n> config file option is already in the wild in production code, as are all\n> current gitweb-caching configuration variables.\n\n[Explitive deleted.] I dislike strongly this $cache_enable. I think it\nwould be better for backward compatibility (should we keep backward\ncompatibility with out-of-tree patches?) to use the same mechanism as\nprovided in \n\n [PATCHv6/RFC 22/24] gitweb: Support legacy options used by kernel.org caching engine\n http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163058\n http://repo.or.cz/w/git/jnareb-git.git/commitdiff/27ec67ad90ecd56ac3d05f6a9ea49b6faabf7d0a\n\nin my rewrite. Just set $caching_enabled to true if $cache_enable is\ndefined and true.\n\n> - Reverted change to reset_output as\n> open STDOUT, \">&\", \\*STDOUT_REAL;\n> causes assertion failures:\n> Assertion !((((s->var)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVLV)) failed: file \"scalar.xs\", line 49 at gitweb.cgi line 1221.\n> if we encounter an error *BEFORE* we've ever changed the output.\n\nWhich Perl version are you using? Because I think you found error in Perl.\nWell, at least I have not happen on this bug.\n\nI have nothing againts using\n\n open STDOUT, \">&STDOUT_REAL\";\n\nthough I really prefer that you used lexical filehandles, instead of\n\"globs\" which are global variables.\n\nThe following works:\n\n open STDOUT, '>&', fileno($fh);\n\nNote that fileno(Symbol::qualify_to_ref($fh)) might be needed...\n\n> - Cleanups there were indirectly mentioned by Jakub\n> - Elimination of anything even remotely looking like duplicate code\n> - Creation of isBinaryAction() and isFeedAction()\n\nCould you please do not use mixedCase names?\n\n\nFirst, that is what %actions_info from\n\n [PATCH 16/24] gitweb: Introduce %actions_info, gathering information about actions\n http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163038\n http://repo.or.cz/w/git/jnareb-git.git/commitdiff/305a10339b33d56b4a50708d71e8f42453c8cb1f\n\nI have invented for.\n\nSecond, why 'isBinaryAction()'? there isn't something inherently\ndifferent between binary (':raw') and text (':utf8') output, as I have\nrepeatedly said before. See my rewrite: there is no special case for\nbinary output (or perhaps binary output as in the case of 'blob_plain'\naction).\n\n> - Adding in blacklist of \"dumb\" clients for purposes of downloading content\n> - Added more explicit disablement of \"Generating...\" page\n\nGood, I'll check this.\n\n> - Added better error handling\n> - Creation of .err file in the cache directory\n> - Trap STDERR output into $output_err as this was spewing data prior\n> to any header information being sent\n\nWhy it is needed? We capture output of \"die\" via CGI::Util::set_message,\nand \"warn\" output is captured to web server logs... unless you explicitely\nuse \"print STDERR \" -- don't do that instead.\n\n> - Added hidden field in footer for url & hash of url, which is extremely useful\n> for debugging\n\nNice idea, I'll see it. Can it be disabled (information leakage)?\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157763","messageId":"m37hfi34q2.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-2-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 01/18] gitweb: Prepare for splitting gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-09T23:30:10Z","receivedAt":"2010-12-09T23:30:10Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> From: Jakub Narebski \n> +install-modules:\n> +\tinstall_dirs=\"$(sort $(dir $(GITWEB_MODULES)))\" && \\\n> +\tfor dir in $$install_dirs; do \\\n> +\t\ttest -d '$(DESTDIR_SQ)$(gitweblibdir_SQ)/$$dir' || \\\n> +\t\t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitweblibdir_SQ)/$$dir'; \\\n\nThis should be\n\n +\t\t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitweblibdir_SQ)'/$$dir; \\\n\nor even\n\n +\t\t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitweblibdir_SQ)'/\"$$dir\"; \\\n\nShell variables should be not inside single quotes (as oposed to make\nvariables, where it does not matter).\n\nPlease rebase on top of v7.4, where it was fixed.\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157764","messageId":"m339q634jx.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-6-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 05/18] gitweb: Regression fix concerning binary output of files","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-09T23:33:55Z","receivedAt":"2010-12-09T23:33:55Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> This solves the regression introduced with v7.2 of the gitweb-caching code,\n> fix proposed by Jakub in his e-mail.\n> \n> Signed-off-by: John 'Warthog9' Hawley \n> ---\n> gitweb/gitweb.perl | 4 ++--\n> 1 files changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 3c3ff08..f2ef3da 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5664,7 +5664,7 @@ sub git_blob_plain {\n> \tif ($caching_enabled) {\n> \t\topen BINOUT, '>', $fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n> \t}else{\n> -\t\topen BINOUT, '>', \\$fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n> +\t\topen BINOUT, '>&', \\$fullhashbinpath or die_error(500, \"Could not open bin dump file\");\n> \t}\n> \tbinmode BINOUT, ':raw';\n> \tprint BINOUT <$fd>;\n\nI'd rather you rebase on top of v7.4, where this issue was fixed in\ndifferent way... well, at least in easier to undertstand way (in the\nsolution used above one must know that if caching is disabled,\n$fullhashbinpath is *STDOUT - and has nothing to do with any _path_).\n\nThis probably should be squashed, if using v7.4 is not chosen.\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157765","messageId":"m3y67y1psd.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-8-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 07/18] gitweb: Revert back to $cache_enable vs. $caching_enabled","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-09T23:38:01Z","receivedAt":"2010-12-09T23:38:01Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> Simple enough, $cache_enable (along with all caching variables) are\n> already in production in multiple places and doing a small semantic\n> change without backwards compatibility is pointless breakage.\n\nFormally, there is no backward compatibility with any released code.\nUsing out-of-tree patches is on one's own risk.\n\nBut even discarding that, I'd rather use the same solution as in\n\n [PATCHv6/RFC 22/24] gitweb: Support legacy options used by kernel.org caching engine\n http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163058\n http://repo.or.cz/w/git/jnareb-git.git/commitdiff/27ec67ad90ecd56ac3d05f6a9ea49b6faabf7d0a\n\ni.e.\n\n our $cache_enable;\n\n [...]\n\n # somewhere just before call to cache_fetch()\n $caching_enabled = !!$cache_enable if defined $cache_enable;\n\n> \n> This reverts back to the previous variable to enable / disable caching\n\n[...]\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -258,7 +258,7 @@ our $maxload = 300;\n> # that the cache directory be periodically completely deleted, and this is safe to perform.\n> # Suggested mechanism\n> # mv $cacheidr $cachedir.flush;mkdir $cachedir;rm -rf $cachedir.flush\n> -our $caching_enabled = 0;\n> +our $cache_enable = 0;\n> \n> # Used to set the minimum cache timeout for the dynamic caching algorithm. Basically\n> # if we calculate the cache to be under this number of seconds we set the cache timeout\n> @@ -1138,7 +1138,7 @@ sub dispatch {\n> \t !$project) {\n> \t\tdie_error(400, \"Project needed\");\n> \t}\n> -\tif ($caching_enabled && is_cacheable($action)) {\n> +\tif ($cache_enable && is_cacheable($action)) {\n> \t\tcache_fetch($action);\n> \t} else {\n> \t\t$actions{$action}->();\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157766","messageId":"m3tyim1pey.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-9-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 08/18] gitweb: Change is_cacheable() to return true always","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-09T23:46:06Z","receivedAt":"2010-12-09T23:46:06Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> is_cacheable() was set to return false for blame or blame_incremental\n> which both use unique urls so there's no reason this shouldn't pass\n> through the caching engine.\n\nI have disabled caching 'blame_incremental' (and its workhorse\n'blame_data'), in slightly different way (by disabling these views\nrather than making them un-cacheable), because last time when I was\nchaing this it simply didn't work with caching. Did you check that it\nworks?\n\nBesides with caching (without \"tee\"-ing captre) 'blame_incremental'\nview doesn't offer any advantage over 'blame' view, so it should be\nIMHO disabled.\n \n> Leaving the function in place for now should something actually arrise\n> that we can't use caching for (think ajaxy kinda things likely).\n\nSidenote: I use it for 'cache' and for 'cache_clear' action in\n\n \"[RFC PATCHv6 24/24] gitweb: Add beginnings of cache administration page (proof of concept)\"\n http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163051\n http://repo.or.cz/w/git/jnareb-git.git/commitdiff/aa9fd77ff206eae8838fdde626d2afea563f9f75\n\n> \n> Signed-off-by: John 'Warthog9' Hawley \n> ---\n> gitweb/gitweb.perl | 3 ++-\n> 1 files changed, 2 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 5eb0309..1d8bc74 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -798,7 +798,8 @@ our %actions = (\n> );\n> sub is_cacheable {\n> \tmy $action = shift;\n> -\treturn !($action eq 'blame_data' || $action eq 'blame_incremental');\n> +\t# There are no known actions that do no involve a unique URL that shouldn't be cached.\n> +\treturn 1;\n> }\n> \n> # finally, we have the hash of allowed extra_options for the commands that\n> -- \n> 1.7.2.3\n> \n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157767","messageId":"m3pqta1ou3.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-10-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 09/18] gitweb: Revert reset_output() back to original code","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-09T23:58:41Z","receivedAt":"2010-12-09T23:58:41Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> Reverted change to reset_output as\n> \n> \topen STDOUT, \">&\", \\*STDOUT_REAL;\n\nFor somebody not following our discussion the above would be very,\nvery cryptic... though I suppose this would be squashed in final\n(ready to be merged in) version of the code.\n \n> causes assertion failures:\n> \n> \tAssertion !((((s->var)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVLV)) failed: file \"scalar.xs\", line 49 at gitweb.cgi line 1221.\n\nIt looks like bug in Perl, because it should give some kind of Perl\nerror, not failed assertion from within guts of Perl C code.\n\nWhich Perl version are you using?\n\n> if we encounter an error *BEFORE* we've ever changed the output.\n\nAnd how to reproduce this error (i.e. how did you found it)?\n \n> Signed-off-by: John 'Warthog9' Hawley \n> ---\n> gitweb/gitweb.perl | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 1d8bc74..e8c028b 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1225,7 +1225,7 @@ sub change_output {\n> \n> sub reset_output {\n> \t# This basically takes STDOUT_REAL and puts it back as STDOUT\n> -\topen STDOUT, \">&\", \\*STDOUT_REAL;\n> +\topen(STDOUT,\">&STDOUT_REAL\");\n\nHmmm... how to silence spurious warning then:\n\n gitweb.perl: Name \"main::STDOUT_REAL\" used only once: possible typo\n at ../gitweb/gitweb.perl line 1130.\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157768","messageId":"m3lj3y1ogb.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-11-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 10/18] gitweb: Adding isBinaryAction() and isFeedAction() to determine the action type","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T00:06:53Z","receivedAt":"2010-12-10T00:06:53Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> This is fairly self explanitory, these are here just to centralize the checking\n> for these types of actions, as special things need to be done with regards to\n> them inside the caching engine.\n> \n> isBinaryAction() returns true if the action deals with creating binary files\n> (this needing :raw output)\n\nWhy do you need special case binary / :raw output? It is not really\nnecessary if it is done in right way, as shown in my rewrite.\n\n> isFeedAction() returns true if the action deals with a news feed of some sort,\n> basically used to bypass the 'Generating...' message should it be a news reader\n> as those will explode badly on that page.\n\nWhy blacklisting 'feed', instead of whitelisting HTML-output?\n\n\nBTW., please don't use mixedCase names, but underline_separated.\n\n> \n> Signed-off-by: John 'Warthog9' Hawley \n> ---\n> gitweb/lib/cache.pl | 69 ++++++++++++++++++++++++++-------------------------\n> 1 files changed, 35 insertions(+), 34 deletions(-)\n> \n> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\n> index a8ee99e..d55b572 100644\n> --- a/gitweb/lib/cache.pl\n> +++ b/gitweb/lib/cache.pl\n> @@ -88,6 +88,34 @@ sub cache_fetch {\n> \t#$actions{$action}->();\n> }\n> \n> +sub isBinaryAction {\n> +\tmy ($action) = @_;\n> +\n> +\tif(\n> +\t\t$action eq \"snapshot\"\n> +\t\t||\n> +\t\t$action eq \"blob_plain\"\n> +\t){\n> +\t\treturn 1;\t# True\n> +\t}\n> +\n> +\treturn 0;\t\t# False\n> +}\n> +\n> +sub isFeedAction {\n> +\tif(\n> +\t\t$action eq \"atom\"\n> +\t\t||\n> +\t\t$action eq \"rss\"\n> +\t\t||\n> +\t\t$action eq \"opml\"\n> +\t){\n> +\t\treturn 1;\t# True\n> +\t}\n> +\n> +\treturn 0;\t\t# False\n> +}\n\nCompare to:\n\n+our %actions_info = ();\n+sub evaluate_actions_info {\n+ our %actions_info;\n+ our (%actions);\n+\n+ # unless explicitely stated otherwise, default output format is html\n+ foreach my $action (keys %actions) {\n+ $actions_info{$action}{'output_format'} = 'html';\n+ }\n+ # list all exceptions; undef means variable (no definite format)\n+ map { $actions_info{$_}{'output_format'} = 'text' }\n+ qw(commitdiff_plain patch patches project_index blame_data);\n+ map { $actions_info{$_}{'output_format'} = 'xml' }\n+ qw(rss atom opml); # there are different types (document formats) of XML\n+ map { $actions_info{$_}{'output_format'} = undef }\n+ qw(blob_plain object);\n+ $actions_info{'snapshot'}{'output_format'} = 'binary';\n+}\n\nInstead of 'xml' you can use 'feed'.\n\nThen e.g.:\n\n+sub action_outputs_html {\n+ my $action = shift;\n+ return $actions_info{$action}{'output_format'} eq 'html';\n+}\n\n\nSee \n \"gitweb: Introduce %actions_info, gathering information about actions\"\n \"gitweb: Show appropriate \"Generating...\" page when regenerating cache\"\n http://repo.or.cz/w/git/jnareb-git.git/shortlog/refs/heads/origin..refs/heads/gitweb/cache-kernel-v6\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157770","messageId":"m3hbem1o7a.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-12-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 11/18] gitweb: add isDumbClient() check","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T00:12:33Z","receivedAt":"2010-12-10T00:12:33Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> Basic check for the claimed Agent string, if it matches a known\n> blacklist (wget and curl currently) don't display the 'Generating...'\n> page.\n> \n> Jakub has mentioned a couple of other possible ways to handle\n> this, so if a better way comes along this should be used as a\n> wrapper to any better way we can find to deal with this.\n> \n> Signed-off-by: John 'Warthog9' Hawley \n> ---\n> gitweb/lib/cache.pl | 30 ++++++++++++++++++++++++++++++\n> 1 files changed, 30 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\n> index d55b572..5182a94 100644\n> --- a/gitweb/lib/cache.pl\n> +++ b/gitweb/lib/cache.pl\n> @@ -116,6 +116,34 @@ sub isFeedAction {\n> \treturn 0;\t\t# False\n> }\n> \n> +# There have been a number of requests that things like \"dumb\" clients, I.E. wget\n> +# lynx, links, etc (things that just download, but don't parse the html) actually\n> +# work without getting the wonkiness that is the \"Generating...\" page.\n> +#\n> +# There's only one good way to deal with this, and that's to read the browser User\n> +# Agent string and do matching based on that. This has a whole slew of error cases\n> +# and mess, but there's no other way to determine if the \"Generating...\" page\n> +# will break things.\n> +#\n> +# This assumes the client is not dumb, thus the default behavior is to return\n> +# \"false\" (0) (and eventually the \"Generating...\" page). If it is a dumb client\n> +# return \"true\" (1)\n> +sub isDumbClient {\n\nPlease don't use mixedCase, but underline_separated words,\ne.g. browser_is_robot(), or client_is_dumb().\n\n> +\tmy($user_agent) = $ENV{'HTTP_USER_AGENT'};\n\nWhat if $ENV{'HTTP_USER_AGENT'} is unset / undef, e.g. because we are\nruning gitweb as a script... which includes running gitweb tests?\n\n> +\t\n> +\tif(\n> +\t\t# wget case\n> +\t\t$user_agent =~ /^Wget/i\n> +\t\t||\n> +\t\t# curl should be excluded I think, probably better safe than sorry\n> +\t\t$user_agent =~ /^curl/i\n> +\t ){\n> +\t\treturn 1;\t# True\n> +\t}\n> +\n> +\treturn 0;\n> +}\n\nCompare (note: handcrafted solution is to whitelist, not blacklist):\n\n+sub browser_is_robot {\n+ return 1 if !exists $ENV{'HTTP_USER_AGENT'}; # gitweb run as script\n+ if (eval { require HTTP::BrowserDetect; }) {\n+ my $browser = HTTP::BrowserDetect->new();\n+ return $browser->robot();\n+ }\n+ # fallback on detecting known web browsers\n+ return 0 if ($ENV{'HTTP_USER_AGENT'} =~ /\\b(?:Mozilla|Opera|Safari|IE)\\b/);\n+ # be conservative; if not sure, assume non-interactive\n+ return 1;\n+}\n\nfrom\n\n \"[PATCHv6 17/24] gitweb: Show appropriate \"Generating...\" page when regenerating cache\"\n http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163040\n http://repo.or.cz/w/git/jnareb-git.git/commitdiff/48679f7985ccda16dc54fda97790841bab4a0ba2\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157771","messageId":"m3d3pa1o0j.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-13-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 12/18] gitweb: Change file handles (in caching) to lexical variables as opposed to globs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T00:16:20Z","receivedAt":"2010-12-10T00:16:20Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> This isn't a huge change, it just adds global variables for the file handles,\n> an additional cleanup to localize the variable a bit more which should alleviate\n> the issues that Jakub had with my original approach.\n> \n> Signed-off-by: John 'Warthog9' Hawley \n> ---\n> gitweb/lib/cache.pl | 114 +++++++++++++++++++++++++++++++-------------------\n> 1 files changed, 71 insertions(+), 43 deletions(-)\n> \n> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\n> index 5182a94..fafc028 100644\n> --- a/gitweb/lib/cache.pl\n> +++ b/gitweb/lib/cache.pl\n> @@ -14,6 +14,12 @@ use Digest::MD5 qw(md5 md5_hex md5_base64);\n> use Fcntl ':flock';\n> use File::Copy;\n> \n> +# Global declarations\n> +our $cacheFile;\n> +our $cacheFileBG;\n> +our $cacheFileBinWT;\n> +our $cacheFileBin;\n\nYou are trading globs for global (well, package) variables. They are\nnot lexical filehandles... though I'm not sure if it would be possible\nwithout restructuring code; note that if variable holding filehandle\nfalls out of scope, then file would be automatically closed.\n\nBTW. Do you really need all those types/variables?\n\n> +\n> sub cache_fetch {\n> \tmy ($action) = @_;\n> \tmy $cacheTime = 0;\n> @@ -49,9 +55,9 @@ sub cache_fetch {\n> \t}else{\n> \t\t#if cache is out dated, update\n> \t\t#else displayCache();\n> -\t\topen(cacheFile, '<', \"$fullhashpath\");\n> -\t\tstat(cacheFile);\n> -\t\tclose(cacheFile);\n> +\t\topen($cacheFile, '<', \"$fullhashpath\");\n> +\t\tstat($cacheFile);\n> +\t\tclose($cacheFile);\n[...]\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157772","messageId":"m38vzy1nkl.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-14-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 13/18] gitweb: Add commented url & url hash to page footer","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T00:26:00Z","receivedAt":"2010-12-10T00:26:00Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> This is mostly a debugging tool, but it adds a small bit of information\n> to the footer:\n> \n> \n\nNice idea. It helps with debugging and doesn't introduce information\nleakage.\n\nNote that in my rewrite there would be *three* pieces of information,\nnot two. Namely:\n\n Full URL: |http://localhost/gitweb-caching/gitweb.cgi/project.git|\n Key: |http://localhost/gitweb-caching/gitweb.cgi?p=/project.git;a=summary|\n Key hash: |7a31cfb8a43f5643679eec88aa9d7981|\n\n> \n> The first bit tells you what the url that generated the page actually was, the second is\n> the hash used to store the file with the first two characters being used as the directory:\n> \n> /7a/31cfb8a43f5643679eec88aa9d7981\n\nIsn't it\n\n /7a/7a31cfb8a43f5643679eec88aa9d7981\n\nin your series?\n\n> \n> Also useful for greping through the existing cache and finding files with unique paths that\n> you may want to explicitly flush.\n\nThough probably better 'cache_admin' page would be ultimately best\nsolution, see proof of concept in\n\n [RFC PATCHv6 24/24] gitweb: Add beginnings of cache administration page (proof of concept)\n http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163051\n http://repo.or.cz/w/git/jnareb-git.git/commitdiff/aa9fd77ff206eae8838fdde626d2afea563f9f75\n\n> \n> Signed-off-by: John 'Warthog9' Hawley \n> ---\n> gitweb/gitweb.perl | 7 +++++++\n> gitweb/lib/cache.pl | 4 ++--\n> 2 files changed, 9 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index e8c028b..7f8292e 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -303,6 +303,9 @@ our $fullhashpath = *STDOUT;\n> our $fullhashbinpath = *STDOUT;\n> our $fullhashbinpathfinal = *STDOUT;\n> \n> +our $full_url;\n> +our $urlhash;\n> +\n> # configuration for 'highlight' (http://www.andre-simon.de/)\n> # match by basename\n> our %highlight_basename = (\n> @@ -3663,6 +3666,10 @@ sub git_footer_html {\n> \n> \tprint \"

\\n\";\n> \tprint \"
Cache Last Updated: \". gmtime( time ) .\" GMT
\\n\";\n> +\tprint\t\"\\n\" if ($cache_enable);\n\nDon't you need to esc_html on it? $full_url can contain ' -->', and\nwhat you would do then?\n\n> \tif (defined $project) {\n> \t\tmy $descr = git_get_project_description($project);\n> \t\tif (defined $descr) {\n> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\n> index fafc028..63dbe9e 100644\n> --- a/gitweb/lib/cache.pl\n> +++ b/gitweb/lib/cache.pl\n> @@ -30,8 +30,8 @@ sub cache_fetch {\n> \t\tprint \"Cache directory created successfully\\n\";\n> \t}\n> \n> -\tour $full_url = \"$my_url?\". $ENV{'QUERY_STRING'};\n\nNote that $my_url is $cgi->url(), which does not include path_info.\n\n> -\tour $urlhash = md5_hex($full_url);\n> +\t$full_url = \"$my_url?\". $ENV{'QUERY_STRING'};\n> +\t$urlhash = md5_hex($full_url);\n> \tour $fullhashdir = \"$cachedir/\". substr( $urlhash, 0, 2) .\"/\";\n> \n> \teval { mkpath( $fullhashdir, 0, 0777 ) };\n> -- \n> 1.7.2.3\n\nLooks quite nice. \n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157773","messageId":"7vwrnieac8.fsf@alter.siamese.dyndns.org","threadId":"26012","inReplyTo":"m3d3pa1o0j.fsf@localhost.localdomain","subject":"Re: [PATCH 12/18] gitweb: Change file handles (in caching) to lexical variables as opposed to globs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-10T00:32:39Z","receivedAt":"2010-12-10T00:32:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n>> +# Global declarations\n>> +our $cacheFile;\n>> +our $cacheFileBG;\n>> +our $cacheFileBinWT;\n>> +our $cacheFileBin;\n>\n> You are trading globs for global (well, package) variables. They are\n> not lexical filehandles... though I'm not sure if it would be possible\n> without restructuring code; note that if variable holding filehandle\n> falls out of scope, then file would be automatically closed.\n\nHmm. why is it a bad idea, when you need to access these from practically\neverywhere, to use global variables to begin with? To a certain degree,\nit sounds like an unnecessary burden without much gain to me.\n"},{"id":"157774","messageId":"m34oam1n3t.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-15-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 14/18] gitweb: add print_transient_header() function for central header printing","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T00:36:09Z","receivedAt":"2010-12-10T00:36:09Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> There are a few things I would like to reuse the transient header\n> information I'm using, currently this is only the 'Generating...'\n> page, but there is at least one additional warning page I would\n> like to use this on.\n> \n> Signed-off-by: John 'Warthog9' Hawley \n> ---\n> gitweb/lib/cache.pl | 47 ++++++++++++++++++++++++++---------------------\n> 1 files changed, 26 insertions(+), 21 deletions(-)\n> \n> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\n> index 63dbe9e..723ae9b 100644\n> --- a/gitweb/lib/cache.pl\n> +++ b/gitweb/lib/cache.pl\n> @@ -94,6 +94,31 @@ sub cache_fetch {\n> \t#$actions{$action}->();\n> }\n> \n> +sub print_transient_header {\n> +\tprint $::cgi->header(\n\nWhy you use $::cgi->header() instead of equivalent $cgi->header()?\nNote that $::cgi->header() is $main::cgi->header(), and is not\nCGI::header().\n\n> +\t\t\t\t-type=>'text/html',\n> +\t\t\t\t-charset => 'utf-8',\n> +\t\t\t\t-status=> 200,\n> +\t\t\t\t-expires => 'now',\n> +\t\t\t\t# HTTP/1.0\n> +\t\t\t\t-Pragma => 'no-cache',\n> +\t\t\t\t# HTTP/1.1\n> +\t\t\t\t-Cache_Control => join(\n> +\t\t\t\t\t\t\t', ',\n> +\t\t\t\t\t\t\tqw(\n> +\t\t\t\t\t\t\t\tprivate\n> +\t\t\t\t\t\t\t\tno-cache\n> +\t\t\t\t\t\t\t\tno-store\n> +\t\t\t\t\t\t\t\tmust-revalidate\n> +\t\t\t\t\t\t\t\tmax-age=0\n> +\t\t\t\t\t\t\t\tpre-check=0\n> +\t\t\t\t\t\t\t\tpost-check=0\n> +\t\t\t\t\t\t\t)\n> +\t\t\t\t\t\t)\n> +\t\t\t\t);\n> +\treturn;\n> +}\n\nWhy not use\n\n\tour %no_cache = (\n\t\t# HTTP/1.0\n\t\t-Pragma => 'no-cache',\n\t\t# HTTP/1.1\n\t\t-Cache_Control => join(', ', qw(private no-cache no-store must-revalidate\n\t\t max-age=0 pre-check=0 post-check=0)),\n\t);\n\n(or something like that). This way you can reuse it even if content\ntype is different (e.g. 'text/plain').\n\nBut that is just a proposal.\n\n> +\n> sub isBinaryAction {\n> \tmy ($action) = @_;\n> \n> @@ -292,27 +317,7 @@ sub cacheWaitForUpdate {\n> \n> \t$| = 1;\n> \n> -\tprint $::cgi->header(\n> -\t\t\t\t-type=>'text/html',\n> -\t\t\t\t-charset => 'utf-8',\n> -\t\t\t\t-status=> 200,\n> -\t\t\t\t-expires => 'now',\n> -\t\t\t\t# HTTP/1.0\n> -\t\t\t\t-Pragma => 'no-cache',\n> -\t\t\t\t# HTTP/1.1\n> -\t\t\t\t-Cache_Control => join(\n> -\t\t\t\t\t\t\t', ',\n> -\t\t\t\t\t\t\tqw(\n> -\t\t\t\t\t\t\t\tprivate\n> -\t\t\t\t\t\t\t\tno-cache\n> -\t\t\t\t\t\t\t\tno-store\n> -\t\t\t\t\t\t\t\tmust-revalidate\n> -\t\t\t\t\t\t\t\tmax-age=0\n> -\t\t\t\t\t\t\t\tpre-check=0\n> -\t\t\t\t\t\t\t\tpost-check=0\n> -\t\t\t\t\t\t\t)\n> -\t\t\t\t\t\t)\n> -\t\t\t\t);\n> +\tprint_transient_header();\n> \n> \tprint < \n> -- \n> 1.7.2.3\n> \n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157775","messageId":"7vsjy6ea0k.fsf@alter.siamese.dyndns.org","threadId":"26012","inReplyTo":"1291931844-28454-1-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 00/18] Gitweb caching v8","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-10T00:39:39Z","receivedAt":"2010-12-10T00:39:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"It seems that t950X tests do not like me. I am getting these in gitweb.body:\n\n \n \n \n \n \n \n \n \n \n \n \n <p><strong>*** Warning ***:</strong> Caching enabled but cache directory did not exsist. (cache)<br/>/\\nCache directory created successfully\\n<p>\n \n \n"},{"id":"157776","messageId":"4D017796.4030506@eaglescrag.net","threadId":"26012","inReplyTo":"m3bp4u34vj.fsf@localhost.localdomain","subject":"Re: [PATCH 00/18] Gitweb caching v8","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-10T00:43:02Z","receivedAt":"2010-12-10T00:43:02Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"On 12/09/2010 03:26 PM, Jakub Narebski wrote:\n> John, could you please in the future Cc me? I am interested in gitweb\n> output caching development. Thanks in advance.\n\nApologies, apparently screwed up on my git send-email line. I'll get\nthat right one of these eons.\n\n> Could you please rebase it on top of v7.2 version? The v7.2 patch\n> series contained a few bugs that needs to be corrected.\n\nI assume you mean 7.4, as opposed to 7.2... otherwise already done!\n\n> I guess that in the final submission (i.e. the one that is to be\n> merged in into git.git repository) those changes would be squashed in,\n> isn't it?\n\nI have no objections to squashing the reversions into a single patch,\njust figured it was easier to break them out for the time being.\n\n>> This still differs, by two patches, from whats in production on kernel.org.\n>> It's missing the index page git:// link, and kernel.org and kernel.org also\n> \n>>\n>> has the forced version matching. As a note I'll probably let this stew\n>> another day or so on kernel.org and then I'll push it into the Fedora update\n>> stream, as there's a couple of things in this patch series that would be \n>> good for them to have.\n> \n> There was some discussion about git:// link in the past; nevertheless\n> this issue is independent on gitweb caching and can (and should) be\n> sent as a aeparate patch.\n> \n> IIRC we agreed that because of backward compatibility forced versions\n> match is quite useless (in general)...\n\nThe former wasn't submitted as that is a separate issue, the later was\nnot agreed on really but mostly me retracting the patches as they\nweren't making any headway.\n\nI mention the patches at all as clarification of what's actually running\non kernel.org, and eventually what will be in the gitweb-caching\npackages that are part of Fedora and EPEL.\n\n>> There is one additional script I've written that the Fedora folks are using,\n>> and that might be useful to include, which is an 'offline' cache file generator.\n>> It basically wraps gitweb.cgi and at the end moves the cache file into the right\n>> place. The Fedora folks were finding it took hours to generate their front\n>> page, and that doing a background generation almost never completed (due to \n>> process death). This was a simple way to handle that. If people would like\n>> I can add it in as an additional patch.\n> \n> Are you detaching the background process?\n\nNo, in fact I completely turn off forking (using the $cacheDoFork variable.)\n\n> It would be nice to have it as separate patch.\n\nI can add it easily enough.\n\n>> v8:\n>> \t- Reverting several changes from Jakub's change set that make no sense\n>> - is_cacheable changed to always return true - nothing special about\n>> blame or blame_incremental as far as the caching engine is concerned\n> \n> 'blame_incremental' is just another version of 'blame' view. I have\n> disabled it when caching is enabled in my rewrite (you instead disabled\n> caching for 'blame_incremental' in your v7 and mine v7.x) because I\n> couldn't get it to work together with caching. Did you check that it\n> works?\n\nblame works fine, blame_incremental generates but doesn't..... ohhhh\nsomeone added ajaxy kinda stuff and doesn't mention it anywhere.\n\nExciting.\n\nblame_data needs to not get a 'generating...' page in all likelihood,\ngenerating a blame_incremental page, letting it load and then refreshing\nthe whole thing gets me what I'm expecting.\n\nIs enough to mask.\n\nGuess I'm looking at a v9 now.\n\n> Besides, withou \"tee\"-ing, i.e. printing output as it is captured,\n> cached 'blame_data' means that 'blame_incremental' is not incremental,\n> and therefore it vanishes its advantage over 'blame'.\n\nThere are only 2 ways to get to a blame_incremental page\n\n1) By going to a blame page and clicking on the incremental link in the nav\n\n2) By enabling it by default so when you click 'blame' it goes to\nincremental first.\n\n> In the case data is in cache, then 'blame_inremental' doesn't have\n> advantage over 'blame' either.\n\nAgreed, though it's easy enough to support in the caching engine,\nbasically don't return 'Generating...' and wait for that data to cache.\n Not really an advantage except that your not waiting for the whole\ngeneration to get a page back at all.\n\n>> - Reverted change to reset_output as\n>> open STDOUT, \">&\", \\*STDOUT_REAL;\n>> causes assertion failures:\n>> Assertion !((((s->var)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVLV)) failed: file \"scalar.xs\", line 49 at gitweb.cgi line 1221.\n>> if we encounter an error *BEFORE* we've ever changed the output.\n> \n> Which Perl version are you using? Because I think you found error in Perl.\n> Well, at least I have not happen on this bug.\n\nThis is perl, v5.10.0 built for x86_64-linux-thread-multi\n\n> I have nothing againts using\n> \n> open STDOUT, \">&STDOUT_REAL\";\n> \n> though I really prefer that you used lexical filehandles, instead of\n> \"globs\" which are global variables.\n> \n> The following works:\n> \n> open STDOUT, '>&', fileno($fh);\n> \n> Note that fileno(Symbol::qualify_to_ref($fh)) might be needed...\n\nI see 0 advantage to shifting around STDOUT and STDERR to a lexical\nfilehandle vs. a glob in this case. STDOUT_REAL retains all the\nproperties of STDOUT should it be needed elsewhere, including what it\nwas going and what it was doing.\n\nI have no objection to shifting the file handles I'm using to lexical\nvariables, if nothing else the argument about them closing when falling\nout of scope is worth it, but for STDOUT, STDERR, etc I don't think\nswitching to lexicals makes a lot of sense\n\n>> - Cleanups there were indirectly mentioned by Jakub\n>> - Elimination of anything even remotely looking like duplicate code\n>> - Creation of isBinaryAction() and isFeedAction()\n> \n> Could you please do not use mixedCase names?\n\nI'm fine with renaming those if you wish.\n\n> First, that is what %actions_info from\n> \n> [PATCH 16/24] gitweb: Introduce %actions_info, gathering information about actions\n> http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163038\n> http://repo.or.cz/w/git/jnareb-git.git/commitdiff/305a10339b33d56b4a50708d71e8f42453c8cb1f\n> \n> I have invented for.\n\nI have not based any of my caching engine, right now, on anything you've\ndone for your rewrite.\n\n> Second, why 'isBinaryAction()'? there isn't something inherently\n> different between binary (':raw') and text (':utf8') output, as I have\n> repeatedly said before.\n\nIt's a binary action in that you are shoving something down the pipe\nwith the intention of sending the bits completely raw. You read the\ndata raw, and write the data raw. There is no interpretation of the\ndata as being anything but straight raw.\n\nRight now, in gitweb already, there are two places that treat output\ncompletely differently:\n\n\t- snapshot\n\t- blob_plain\n\nThe only reason isBinaryAction() (or any other function name or process\nyou want to grant it) exists is so that I can figure out if it's one of\nthose actions so I can deal with the cache and output handling\ndifferently for each.\n\nYes, I could flip the entire caching engine over to following the same\nmantra for everything and thus there is no need to care, but gitweb\nitself isn't really setup to handle that separation cleanly right now,\nand I'm trying to make as few bigger changes right now as is.\n\n\n>> - Added better error handling\n>> - Creation of .err file in the cache directory\n>> - Trap STDERR output into $output_err as this was spewing data prior\n>> to any header information being sent\n> \n> Why it is needed? We capture output of \"die\" via CGI::Util::set_message,\n> and \"warn\" output is captured to web server logs... unless you explicitely\n> use \"print STDERR \" -- don't do that instead.\n\nI have seen, in several instances, a case where git itself will generate\nan error, it shoves it to STDERR which makes it to the client before\nanything else, thus causing 500 level errors.\n\nAdded this so that STDERR got trapped and those messages didn't make it out.\n\n>> - Added hidden field in footer for url & hash of url, which is extremely useful\n>> for debugging\n> \n> Nice idea, I'll see it. Can it be disabled (information leakage)?\n\nThere's not really any information leakage per-se, unless you call\nmd5suming the url information leakage.\n\n- John 'Warthog9' Hawley\n"},{"id":"157777","messageId":"4D01782A.5060702@eaglescrag.net","threadId":"26012","inReplyTo":"7vsjy6ea0k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 00/18] Gitweb caching v8","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-10T00:45:30Z","receivedAt":"2010-12-10T00:45:30Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"On 12/09/2010 04:39 PM, Junio C Hamano wrote:\n> It seems that t950X tests do not like me. I am getting these in gitweb.body:\n> \n> \n> \n> \n> \n> \n> \n> \n> \n> \n> \n> \n> <p><strong>*** Warning ***:</strong> Caching enabled but cache directory did not exsist. (cache)<br/>/\\nCache directory created successfully\\n<p>\n> \n> \n\ncaching directory didn't exist prior to running the test, and so it\nthrows the warning that it needed to create it. The cache directory\nshould likely be created before caching tests.\n\n(Sorry, didn't catch that one as I've already got my caching directory\ncreated, and it doesn't really get deleted)\n\n- John 'Warthog9' Hawley\n"},{"id":"157778","messageId":"201012100147.20747.jnareb@gmail.com","threadId":"26012","inReplyTo":"7vwrnieac8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 12/18] gitweb: Change file handles (in caching) to lexical variables as opposed to globs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T00:47:19Z","receivedAt":"2010-12-10T00:47:19Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 10 Dec 2010, Junio C Hamano wrote:\n> Jakub Narebski writes:\n>> \"John 'Warthog9' Hawley\" writes:\n>>> \n>>> +# Global declarations\n>>> +our $cacheFile;\n>>> +our $cacheFileBG;\n>>> +our $cacheFileBinWT;\n>>> +our $cacheFileBin;\n>>\n>> You are trading globs for global (well, package) variables. They are\n>> not lexical filehandles... though I'm not sure if it would be possible\n>> without restructuring code; note that if variable holding filehandle\n>> falls out of scope, then file would be automatically closed.\n> \n> Hmm. why is it a bad idea, when you need to access these from practically\n> everywhere, to use global variables to begin with? To a certain degree,\n> it sounds like an unnecessary burden without much gain to me.\n\nIf you check my rewrite of gitweb output caching:\n\n \"[PATCHv6/RFC 00/24] gitweb: Simple file based output caching\"\n \n\nhttp://repo.or.cz/w/git/jnareb-git.git/shortlog/refs/heads/origin..refs/heads/gitweb/cache-kernel-v6\nhttps://github.com/jnareb/git/compare/origin...gitweb/cache-kernel-v6\n\nyou would see that I always use lexical filehandles, and I never need\nto use global variables / glob filehandles.\n\nhttp://en.wikipedia.org/wiki/Global_variables\n-- \nJakub Narebski\nPoland\n"},{"id":"157779","messageId":"m3zksezbkm.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-16-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 15/18] gitweb: Add show_warning() to display an immediate warning, with refresh","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T01:01:05Z","receivedAt":"2010-12-10T01:01:05Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> die_error() is an immediate and abrupt action. show_warning() more or less\n> functions identically, except that the page generated doesn't use the\n> gitweb header or footer (in case they are broken) and has an auto-refresh\n> (10 seconds) built into it.\n\nWhy not use gitweb header/footer? If they are broken, it should be\ncaught in git development. If we don't se them, the show_warning()\noutput would look out of place.\n\n> \n> This makes use of print_transient_header() which is also used in the\n> 'Generating...' page. Currently the only warning it throws is about\n> the cache needing to be created. If that fails it's a fatal error\n> and we call die_error()\n\nWhy do you feel the need to single out this case giving it warning,\nand single out this warning by showing warning page?\n\nNevertheless show_warning() _might_ be a good idea.\n\n> \n> Signed-off-by: John 'Warthog9' Hawley \n> ---\n> gitweb/lib/cache.pl | 36 +++++++++++++++++++++++++++++++++---\n> 1 files changed, 33 insertions(+), 3 deletions(-)\n> \n> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\n> index 723ae9b..28e4240 100644\n> --- a/gitweb/lib/cache.pl\n> +++ b/gitweb/lib/cache.pl\n> @@ -25,9 +25,13 @@ sub cache_fetch {\n> \tmy $cacheTime = 0;\n> \n> \tif(! -d $cachedir){\n> -\t\tprint \"*** Warning ***: Caching enabled but cache directory does not exsist. ($cachedir)\\n\";\n> -\t\tmkdir (\"cache\", 0755) || die \"Cannot create cache dir - you will need to manually create\";\n> -\t\tprint \"Cache directory created successfully\\n\";\n> +\t\tmkdir (\"cache\", 0755) || die_error(500, \"Internal Server Error\", \"Cannot create cache dir () - you will need to manually create\");\n> +\t\tshow_warning(\n> +\t\t\t\t\"

\".\n> +\t\t\t\t\"*** Warning ***: Caching enabled but cache directory did not exsist. ($cachedir)
/\\n\".\n\nMinor nit: s/exsist/exist/\n\nDon't you need to use esc_path() on $cachedir, \nusing either\n\n ...did not exist. (\".esc_path($cachedir).\")
\\n\";\n\nor using this trick\n\n ...did not exist. (@{[esc_path($cachedir)]})
\\n\";\n\n> +\t\t\t\t\"Cache directory created successfully\\n\".\n> +\t\t\t\t\"

\"\n> +\t\t\t\t);\n> \t}\n> \n> \t$full_url = \"$my_url?\". $ENV{'QUERY_STRING'};\n> @@ -119,6 +123,32 @@ sub print_transient_header {\n> \treturn;\n> }\n> \n> +sub show_warning {\n> +\t$| = 1;\n\n +\tlocal $| = 1;\n\n$| is global variable, and otherwise you would turn autoflush for all\ncode, which would matter e.g. for FastCGI.\n\n> +\n> +\tmy $warning = esc_html(shift) || \"Unknown Warning\";\n> +\n> +\tprint_transient_header();\n> +\n> +\tprint < +\n> +\n> +\n> +\n> +\n\n$content_type is not defined here.\n\n> +\n> +\n\nIt is \"noindex, nofollow\", isn't it?\n\n> +\n\nWhy 10 seconds?\n\n> +$title\n\n$title is not defined here.\n\n> +\n> +\n> +$warning\n> +\n> +\n> +EOF\n> +\texit(0);\n\n\"exit(0)\" and not \"goto DONE_GITWEB\", or \"goto DONE_REQUEST\"?\n\n> +}\n> +\n> sub isBinaryAction {\n> \tmy ($action) = @_;\n\nDidn't you ran gitweb tests?\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157780","messageId":"201012100227.27903.jnareb@gmail.com","threadId":"26012","inReplyTo":"4D017796.4030506@eaglescrag.net","subject":"Re: [PATCH 00/18] Gitweb caching v8","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T01:27:27Z","receivedAt":"2010-12-10T01:27:27Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 10 Dec 2010, J.H. wrote:\n> On 12/09/2010 03:26 PM, Jakub Narebski wrote:\n\n>> John, could you please in the future Cc me? I am interested in gitweb\n>> output caching development. Thanks in advance.\n> \n> Apologies, apparently screwed up on my git send-email line. I'll get\n> that right one of these eons.\n\nAh, I can understand this.\n\n>> I guess that in the final submission (i.e. the one that is to be\n>> merged in into git.git repository) those changes would be squashed in,\n>> isn't it?\n> \n> I have no objections to squashing the reversions into a single patch,\n> just figured it was easier to break them out for the time being.\n\nI guess that interdiff in comments would work as well, or almost as well...\n \n>>> There is one additional script I've written that the Fedora folks are using,\n>>> and that might be useful to include, which is an 'offline' cache file generator.\n>>> It basically wraps gitweb.cgi and at the end moves the cache file into the right\n>>> place. The Fedora folks were finding it took hours to generate their front\n>>> page, and that doing a background generation almost never completed (due to \n>>> process death). This was a simple way to handle that. If people would like\n>>> I can add it in as an additional patch.\n>> \n>> Are you detaching the background process?\n\nErrr... what I meant here is that perhaps detaching background process\nwould make it not die, but I am guessing here.\n \n> No, in fact I completely turn off forking (using the $cacheDoFork variable.)\n\nBTW. what I don't like is your code forking indiscriminately even if it\nis not needed (e.g. background cache generation is turned off).\n\n> \n>> It would be nice to have it as separate patch.\n> \n> I can add it easily enough.\n\nIt is only about caching most IO intensive page, i.e. projects_list page,\nisn't it? Why doesn't _it_ die, like background process?\n\n> \n>>> v8:\n>>> \t- Reverting several changes from Jakub's change set that make no sense\n>>> - is_cacheable changed to always return true - nothing special about\n>>> blame or blame_incremental as far as the caching engine is concerned\n>> \n>> 'blame_incremental' is just another version of 'blame' view. I have\n>> disabled it when caching is enabled in my rewrite (you instead disabled\n>> caching for 'blame_incremental' in your v7 and mine v7.x) because I\n>> couldn't get it to work together with caching. Did you check that it\n>> works?\n> \n> blame works fine, blame_incremental generates but doesn't..... ohhhh\n> someone added ajaxy kinda stuff and doesn't mention it anywhere.\n\nErrr... I thought that the 'incremental' part is self-explaining that\nit is Ajax-y stuff. Well, while commit is 4af819d (gitweb: Incremental\nblame (using JavaScript), 2009-09-01), perhaps I should have added some\ncomment in the code.\n\n> \n> Exciting.\n> \n> blame_data needs to not get a 'generating...' page in all likelihood,\n> generating a blame_incremental page, letting it load and then refreshing\n> the whole thing gets me what I'm expecting.\n\nHmmm... I wonder why it didn't work for me at that time...\n\n> \n> Is enough to mask.\n> \n> Guess I'm looking at a v9 now.\n> \n>> Besides, withou \"tee\"-ing, i.e. printing output as it is captured,\n>> cached 'blame_data' means that 'blame_incremental' is not incremental,\n>> and therefore it vanishes its advantage over 'blame'.\n\nI mean here that with current state of caching 'blame_incremental' stops\nto be incremental...\n \n> There are only 2 ways to get to a blame_incremental page\n> \n> 1) By going to a blame page and clicking on the incremental link in the nav\n> \n> 2) By enabling it by default so when you click 'blame' it goes to\n> incremental first.\n\n 3) By having JavaScript add ';js=1' to all links, so clicking on\n 'blame' link (with action set to 'blame') would result in \n 'blame_incremental' view.\n\n> \n>> In the case data is in cache, then 'blame_inremental' doesn't have\n>> advantage over 'blame' either.\n> \n> Agreed, though it's easy enough to support in the caching engine,\n> basically don't return 'Generating...' and wait for that data to cache.\n> Not really an advantage except that your not waiting for the whole\n> generation to get a page back at all.\n> \n>>> - Reverted change to reset_output as\n>>> open STDOUT, \">&\", \\*STDOUT_REAL;\n>>> causes assertion failures:\n>>> Assertion !((((s->var)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVLV)) failed: file \"scalar.xs\", line 49 at gitweb.cgi line 1221.\n>>> if we encounter an error *BEFORE* we've ever changed the output.\n>> \n>> Which Perl version are you using? Because I think you found error in Perl.\n>> Well, at least I have not happen on this bug.\n> \n> This is perl, v5.10.0 built for x86_64-linux-thread-multi\n\nCould you check with newer perl? I don't get this error.\n\n>> I have nothing againts using\n>> \n>> open STDOUT, \">&STDOUT_REAL\";\n>> \n>> though I really prefer that you used lexical filehandles, instead of\n>> \"globs\" which are global variables.\n\nAnd using 'print STDOUT_REAL \"\";' protects against spurious warning\n(the warning is really wrong in this case).\n \n>> The following works:\n>> \n>> open STDOUT, '>&', fileno($fh);\n>> \n>> Note that fileno(Symbol::qualify_to_ref($fh)) might be needed...\n> \n> I see 0 advantage to shifting around STDOUT and STDERR to a lexical\n> filehandle vs. a glob in this case. STDOUT_REAL retains all the\n> properties of STDOUT should it be needed elsewhere, including what it\n> was going and what it was doing.\n> \n> I have no objection to shifting the file handles I'm using to lexical\n> variables, if nothing else the argument about them closing when falling\n> out of scope is worth it, but for STDOUT, STDERR, etc I don't think\n> switching to lexicals makes a lot of sense\n\nWell... I'd have to agree that in current case (capturing engine embedded\nin gitweb, and gitweb-specific; no need for recursive capture) it would\nbe enough to use such globs.\n\n> \n>>> - Cleanups there were indirectly mentioned by Jakub\n>>> - Elimination of anything even remotely looking like duplicate code\n>>> - Creation of isBinaryAction() and isFeedAction()\n>> \n>> Could you please do not use mixedCase names?\n> \n> I'm fine with renaming those if you wish.\n> \n>> First, that is what %actions_info from\n>> \n>> [PATCH 16/24] gitweb: Introduce %actions_info, gathering information about actions\n>> http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163038\n>> http://repo.or.cz/w/git/jnareb-git.git/commitdiff/305a10339b33d56b4a50708d71e8f42453c8cb1f\n>> \n>> I have invented for.\n> \n> I have not based any of my caching engine, right now, on anything you've\n> done for your rewrite.\n\nWhat I meant here that if you will be doing yet another version, you\ncan take a look at it as a way to avoiding not very clear and nice\nlong alternatives in condition, or in regexp matched.\n\n> \n>> Second, why 'isBinaryAction()'? there isn't something inherently\n>> different between binary (':raw') and text (':utf8') output, as I have\n>> repeatedly said before.\n> \n> It's a binary action in that you are shoving something down the pipe\n> with the intention of sending the bits completely raw. You read the\n> data raw, and write the data raw. There is no interpretation of the\n> data as being anything but straight raw.\n> \n> Right now, in gitweb already, there are two places that treat output\n> completely differently:\n> \n> \t- snapshot\n> \t- blob_plain\n> \n> The only reason isBinaryAction() (or any other function name or process\n> you want to grant it) exists is so that I can figure out if it's one of\n> those actions so I can deal with the cache and output handling\n> differently for each.\n> \n> Yes, I could flip the entire caching engine over to following the same\n> mantra for everything and thus there is no need to care, but gitweb\n> itself isn't really setup to handle that separation cleanly right now,\n> and I'm trying to make as few bigger changes right now as is.\n\nAlways reading from cache in ':raw' mode and always printing from cache\nin ':raw' mode (i.e. setting STDOUT to ':raw' before printing / copying\ncache entry) would be in gitweb case enough to not special-case binary\nfiles.\n\nIn gitweb you always do \"binmode STDOUT, ':raw';\" _after_ starting capture,\nwhich means that it gets applied to cache file; and gitweb always do\n\"binmode STDOUT, ':utf8';\" before stopping capture.\n\nIf you print text data to file using ':utf8' layer (applied at beginning\nto cache file) it is in this file as correct sequence of bytes. Therefore\nyou can dump said cache file to STDOUT in ':raw' mode (or in ':utf8' mode)\n- both STDOUT and read cache file has to have the same mode.\n\n>>> - Added better error handling\n>>> - Creation of .err file in the cache directory\n>>> - Trap STDERR output into $output_err as this was spewing data prior\n>>> to any header information being sent\n>> \n>> Why it is needed? We capture output of \"die\" via CGI::Util::set_message,\n>> and \"warn\" output is captured to web server logs... unless you explicitely\n>> use \"print STDERR \" -- don't do that instead.\n> \n> I have seen, in several instances, a case where git itself will generate\n> an error, it shoves it to STDERR which makes it to the client before\n> anything else, thus causing 500 level errors.\n> \n> Added this so that STDERR got trapped and those messages didn't make it out.\n\nCould you give examples when it happens? Anything that happens after\n\"use CGI::Carp\" is parsed should have STDERR redirected to web server\nerrors log.\n\nI'll read the actual patch and comment on it.\n\n> \n>>> - Added hidden field in footer for url & hash of url, which is extremely useful\n>>> for debugging\n>> \n>> Nice idea, I'll see it. Can it be disabled (information leakage)?\n> \n> There's not really any information leakage per-se, unless you call\n> md5suming the url information leakage.\n\nAh, sorry, I send this comment before actually reading patch in question.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"157783","messageId":"m3vd32z9yk.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-17-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 16/18] gitweb: When changing output (STDOUT) change STDERR as well","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T01:36:02Z","receivedAt":"2010-12-10T01:36:02Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> This sets up a trap for STDERR as well as STDOUT. This should\n> prevent any transient error messages from git itself percolating\n> up to gitweb and outputting errant information before the HTTP\n> header has been sent.\n\nHmm... anuthing that happens after 'use CGI::Carp;' is parsed should\nhave STDERR redirected to web server logs, see CGI::Carp manpage\n\n [...]\n \n use CGI::Carp\n\n And the standard warn(), die (), croak(), confess() and carp() calls will\n automagically be replaced with functions that write out nicely time-stamped\n messages to the HTTP server error log.\n\n [...]\n\n REDIRECTING ERROR MESSAGES\n\n By default, error messages are sent to STDERR. Most HTTPD servers direct\n STDERR to the server's error log.\n\n [...]\n\nEspecially the second part.\n\n\nCould you give us example which causes described misbehaviour?\n\nI have nothing against this patch: if you have to have it, then you\nhave to have it. I oly try to understand what might be core cause\nbehind the issue that this patch is to solve...\n\n> Signed-off-by: John 'Warthog9' Hawley \n> ---\n> gitweb/gitweb.perl | 22 +++++++++++++++++++++-\n> gitweb/lib/cache.pl | 22 ----------------------\n> 2 files changed, 21 insertions(+), 23 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 7f8292e..d39982a 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1214,6 +1214,10 @@ sub evaluate_argv {\n> sub change_output {\n> \tour $output;\n> \n> +\t#\n> +\t# STDOUT\n> +\t#\n> +\n> \t# Trap the 'proper' STDOUT to STDOUT_REAL for things like error messages and such\n> \topen(STDOUT_REAL,\">&STDOUT\") or die \"Unable to capture STDOUT $!\\n\";\n> \tprint STDOUT_REAL \"\";\n> @@ -1223,12 +1227,28 @@ sub change_output {\n> \n> \t# Trap STDOUT to the $output variable, which is what I was using in the original\n> \t# patch anyway.\n> -\topen(STDOUT,\">\", \\$output) || die \"Unable to open STDOUT: $!\"; #open STDOUT handle to use $var\n> +\topen(STDOUT,\">\", \\$output) || die \"Unable to open STDOUT: $!\"; #open STDOUT handle to use $output\n> +\n> +\t#\n> +\t# STDERR\n> +\t#\n> +\n> +\t# Trap the 'proper' STDOUT to STDOUT_REAL for things like error messages and such\n> +\topen(STDERR_REAL,\">&STDERR\") or die \"Unable to capture STDERR $!\\n\";\n> +\tprint STDERR_REAL \"\";\n\n'print STDERR_REAL \"\";' nicely solves the spurious warning problem.\nNice.\n\n> +\n> +\t# Close STDOUT, so that it isn't being used anymore.\n> +\tclose STDERR;\n> +\n> +\t# Trap STDOUT to the $output variable, which is what I was using in the original\n> +\t# patch anyway.\n> +\topen(STDERR,\">\", \\$output_err) || die \"Unable to open STDERR: $!\"; #open STDERR handle to use $output_err\n\nErr... where $output_err is defined?\n\n> }\n> \n> sub reset_output {\n> \t# This basically takes STDOUT_REAL and puts it back as STDOUT\n> \topen(STDOUT,\">&STDOUT_REAL\");\n> +\topen(STDERR,\">&STDERR_REAL\");\n> }\n> \n> sub run {\n> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\n> index 28e4240..a8c902d 100644\n> --- a/gitweb/lib/cache.pl\n> +++ b/gitweb/lib/cache.pl\n> @@ -380,28 +380,6 @@ EOF\n> \treturn;\n> }\n> \n> -sub cacheDisplayErr {\n> -\n> -\treturn if ( ! -e \"$fullhashpath.err\" );\n> -\n> -\topen($cacheFileErr, '<:utf8', \"$fullhashpath.err\");\n> -\t$lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);\n> -\n> -\tif (! $lockStatus ){\n> -\t\tshow_warning(\n> -\t\t\t\t\"

\".\n> -\t\t\t\t\"*** Warning ***: Locking error when trying to lock error cache page, file $fullhashpath.err
/\\n\".\n> -\t\t\t\t\"This is about as screwed up as it gets folks - see your systems administrator for more help with this.\".\n> -\t\t\t\t\"

\"\n> -\t\t\t\t);\n> -\t}\n> -\n> -\twhile( <$cacheFileErr> ){\n> -\t\tprint $_;\n> -\t}\n> -\texit(0);\n> -}\n\nErrr... in which patch it was added?\n\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157785","messageId":"m3r5dqz9c5.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-18-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 17/18] gitweb: Prepare for cached error pages & better error page handling","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T01:49:28Z","receivedAt":"2010-12-10T01:49:28Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> To quote myself from an e-mail of mine:\n> \n> \tI've got a hammer, it clearly solves all problems!\n> \n> This is the prepatory work to set up a mechanism inside the\n> caching engine to cache the error pages instead of throwing\n> them straight out to the client.\n\nThere is no problem with capturing output of die_error, nor there is a\nproblem with caching error pages (perhaps transiently in memory).\n\nThe problem is that subroutines calling die_error assum that it would\nexit ending subroutine that is responsible for generating current\naction; see \"goto DONE_GITWEB\" which should be \"goto DONE_REQUEST\",\nand which was \"exit 0\" some time ago at the end of die_error().\n\nWith caching error pages you want die_error to exit $actions{$action}->(),\nbut not exit cache_fetch(). How do you intend to do it?\n\n> \n> This adds two functions:\n> \n> die_error_cache() - this gets back called from die_error() so\n> that the error message generated can be cached.\n\n*How* die_error_cache() gets called back from die_error()? I don't\nsee any changes to die_error(), or actually any calling sites for\ndie_error_cache() in the patch below.\n \n> cacheDisplayErr() - this is a simplified version of cacheDisplay()\n> that does an initial check, if the error page exists - display it\n> and exit. If not, return.\n\nErrr... isn't it removed in _preceding_ patch? WTF???\n\n> \n> Signed-off-by: John 'Warthog9' Hawley \n> ---\n> gitweb/lib/cache.pl | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 files changed, 52 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\n> index a8c902d..6cb82c8 100644\n> --- a/gitweb/lib/cache.pl\n> +++ b/gitweb/lib/cache.pl\n> @@ -302,6 +302,36 @@ sub cacheUpdate {\n> \t}\n> }\n> \n> +sub die_error_cache {\n> +\tmy ($output) = @_;\n> +\n> +\topen(my $cacheFileErr, '>:utf8', \"$fullhashpath.err\");\n> +\tmy $lockStatus = flock($cacheFileErr,LOCK_EX|LOCK_NB);\n\nWhy do you need to lock here? A comment would be nice.\n\n> +\n> +\tif (! $lockStatus ){\n> +\t\tif ( $areForked ){\n\nGrrrr...\n\nBut if it is here to stay, a comment if you please.\n\n> +\t\t\texit(0);\n> +\t\t}else{\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> +\n> +\t# Actually dump the output to the proper file handler\n> +\tlocal $/ = undef;\n> +\t$|++;\n\nWhy not\n\n +\tlocal $| = 1;\n\n\n> +\tprint $cacheFileErr \"$output\";\n> +\t$|--;\n> +\n> +\tflock($cacheFileErr,LOCK_UN);\n> +\tclose($cacheFileErr);\n\nClosing file will unlock it.\n\n> +\n> +\tif ( $areForked ){\n> +\t\texit(0);\n> +\t}else{\n> +\t\treturn;\n\nSo die_error_cache would not actually work like \"die\" here and like\ndie_error(), isn't it?\n\n> +\t}\n> +}\n> +\n> \n> sub cacheWaitForUpdate {\n> \tmy ($action) = @_;\n> @@ -380,6 +410,28 @@ EOF\n> \treturn;\n> }\n> \n> +sub cacheDisplayErr {\n> +\n> +\treturn if ( ! -e \"$fullhashpath.err\" );\n> +\n> +\topen($cacheFileErr, '<:utf8', \"$fullhashpath.err\");\n> +\t$lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);\n> +\n> +\tif (! $lockStatus ){\n> +\t\tshow_warning(\n> +\t\t\t\t\"

\".\n> +\t\t\t\t\"*** Warning ***: Locking error when trying to lock error cache page, file $fullhashpath.err
/\\n\".\n\nesc_path\n\n> +\t\t\t\t\"This is about as screwed up as it gets folks - see your systems administrator for more help with this.\".\n> +\t\t\t\t\"

\"\n> +\t\t\t\t);\n> +\t}\n> +\n> +\twhile( <$cacheFileErr> ){\n> +\t\tprint $_;\n> +\t}\n\nWhy not 'print <$cacheFileErr>' (list context), like in insert_file()\nsubroutine?\n\n> +\texit(0);\n> +}\n\nCallsites?\n\nNote: I have't read next commit yet.\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157787","messageId":"m3mxoez90s.fsf@localhost.localdomain","threadId":"26012","inReplyTo":"1291931844-28454-19-git-send-email-warthog9@eaglescrag.net","subject":"Re: [PATCH 18/18] gitweb: Add better error handling for gitweb caching","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T01:56:16Z","receivedAt":"2010-12-10T01:56:16Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"John 'Warthog9' Hawley\" writes:\n\n> This basically finishes the plumbing for caching the error pages\n> as the are generated.\n> \n> If an error is hit, create a .err file with the error. This\n> will interrupt all currently waiting processes and they will display\n> the error, without any additional refreshing.\n> \n> On a new request a generation will be attempted, should it succed the\n> file is removed (if it exists).\n\nCould you split 17 and 18 patches slightly differently, at least not\nusing variables which were not declared first?\n \n> Signed-off-by: John 'Warthog9' Hawley \n\nHmmm... I certainly hope that this complication is not really needed.\nI have trouble following code flow (no comments), so I'd try to do\nfresh review again tomorrow.\n\n> ---\n> gitweb/gitweb.perl | 8 ++++++++\n> gitweb/lib/cache.pl | 14 ++++++++++++++\n> 2 files changed, 22 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index d39982a..5a9660a 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -41,6 +41,7 @@ our $version = \"++GIT_VERSION++\";\n> \n> # Output buffer variable\n> our $output = \"\";\n> +our $output_err = \"\";\n\nIt is used by earlier patches, and declared only there.\n\n> \n> our ($my_url, $my_uri, $base_url, $path_info, $home_link);\n> sub evaluate_uri {\n> @@ -303,6 +304,9 @@ our $fullhashpath = *STDOUT;\n> our $fullhashbinpath = *STDOUT;\n> our $fullhashbinpathfinal = *STDOUT;\n> \n> +our $cacheErrorCache = 0; # false\n\n$capture_error_output, isn't it?\n\n> +our $cacheErrorCount = 0;\n\n$cached_error_count, or something like that, isn't it?\n\n> +\n> our $full_url;\n> our $urlhash;\n> \n> @@ -3786,6 +3790,7 @@ sub die_error {\n> \t# Reset the output so that we are actually going to STDOUT as opposed\n> \t# to buffering the output.\n> \treset_output() if ($cache_enable && ! $cacheErrorCache);\n> +\t$cacheErrorCount++ if( $cacheErrorCache );\n\nWhere it is decremented? A comment, if you please.\n\n> \n> \tgit_header_html($http_responses{$status}, undef, %opts);\n> \tprint < @@ -3801,6 +3806,9 @@ EOF\n> \tprint \"

\\n\";\n> \n> \tgit_footer_html();\n> +\n> +\tdie_error_cache($output) if ( $cacheErrorCache );\n> +\n\nThat's cache_die_error_output, or something like that, isn't it?\n\nIt's hard to review this patch when die_error_cache is defined in\nseparate (previous) patch.\n\n> \tgoto DONE_GITWEB\n> \t\tunless ($opts{'-error_handler'});\n> }\n> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl\n> index 6cb82c8..2e7ca69 100644\n> --- a/gitweb/lib/cache.pl\n> +++ b/gitweb/lib/cache.pl\n> @@ -240,8 +240,14 @@ sub cacheUpdate {\n> \t# Trap all output from the action\n> \tchange_output();\n> \n> +\t# Set the error handler so we cache\n> +\t$cacheErrorCache = 1; # true\n> +\n> \t$actions{$action}->();\n> \n> +\t# Reset Error Handler to not cache\n> +\t$cacheErrorCache = 0; # false\n> +\n> \t# Reset the outputs as we should be fine now\n> \treset_output();\n> \n> @@ -295,6 +301,8 @@ sub cacheUpdate {\n> \t\tclose($cacheFileBG);\n> \t}\n> \n> +\tunlink(\"$fullhashpath.err\") if (-e \"$fullhashpath.err\");\n> +\n> \tif ( $areForked ){\n> \t\texit(0);\n> \t} else {\n> @@ -339,6 +347,9 @@ sub cacheWaitForUpdate {\n> \tmy $max = 10;\n> \tmy $lockStat = 0;\n> \n> +\t# Call cacheDisplayErr - if an error exists it will display and die. If not it will just return\n> +\tcacheDisplayErr($action);\n> +\n> \tif( $backgroundCache ){\n> \t\tif( -e \"$fullhashpath\" ){\n> \t\t\topen($cacheFile, '<:utf8', \"$fullhashpath\");\n> @@ -402,6 +413,9 @@ EOF\n> \t\tclose($cacheFile);\n> \t\t$x++;\n> \t\t$combinedLockStat = $lockStat;\n> +\n> +\t\t# Call cacheDisplayErr - if an error exists it will display and die. If not it will just return\n> +\t\tcacheDisplayErr($action);\n> \t} while ((! $combinedLockStat) && ($x < $max));\n> \tprint < \n> -- \n> 1.7.2.3\n> \n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"157788","messageId":"4D019288.9060503@eaglescrag.net","threadId":"26012","inReplyTo":"m3y67y1psd.fsf@localhost.localdomain","subject":"Re: [PATCH 07/18] gitweb: Revert back to $cache_enable vs. $caching_enabled","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-10T02:38:00Z","receivedAt":"2010-12-10T02:38:00Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"> Formally, there is no backward compatibility with any released code.\n> Using out-of-tree patches is on one's own risk.\n\nI will have to beg to differ with you on this, the entirety of the\nexisting caching engine has been released code for a number of years,\nthere are rpm packages available for it, at the very least, in Fedora\nand in EPEL.\n\nThe caching engine *IS* released code, and this patchset is as much a\nnew feature as an attempt to merge a fork. Kernel.org isn't the only\none running this code, and that has been the case for several years now\nalready.\n\nClaiming that this isn't released code is doing me a disservice to me,\nand those who have submitted patches to it independent of git and the\nmainline gitweb.\n\nThinking about the patch series outside of that context will lead to me\nputting my foot down and arguing on those other users behalf. I'm not\nkeen on breaking them for no good reason, and I'm not seeing your change\nhere as one that's particularly worthwhile, while causing external\nbreakage for no reason.\n\n> But even discarding that, I'd rather use the same solution as in\n> \n> [PATCHv6/RFC 22/24] gitweb: Support legacy options used by kernel.org caching engine\n> http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163058\n> http://repo.or.cz/w/git/jnareb-git.git/commitdiff/27ec67ad90ecd56ac3d05f6a9ea49b6faabf7d0a\n> \n> i.e.\n> \n> our $cache_enable;\n> \n> [...]\n> \n> # somewhere just before call to cache_fetch()\n> $caching_enabled = !!$cache_enable if defined $cache_enable;\n> \n>>\n>> This reverts back to the previous variable to enable / disable caching\n\nIs there really any point in changing the name at all? The intention of\ncache_enable, at one point, was to allow for other caching engines and\nwhile there aren't any other caching engines that use it, it's already\ntreated identically to cache_enable.\n\nIf it really adds enough to the readability to the code, then I'm fine\nwith adding:\n\n\t$caching_enabled = $cache_enable if defined $cache_enable;\n\nBut now you are setting up two variables that control the same thing,\nadding the possibility for conflicts and confusion to end users.\n\nI just want that stated.\n\nAlso, why the double negative in your original snippet - that doesn't\nentirely make sense....\n\n | cache_enable | caching_enabled\n----------+-------------------+---------------------\nenabled: | 1 | 1\ndisabled: | 0 | 0\n\ndoing a double negative like that doesn't really buy you much except\nturning 0 into NULL or '' which is equivalent to 0...\n\n- John 'Warthog9' Hawley\n"},{"id":"157789","messageId":"4D0193BC.7010203@eaglescrag.net","threadId":"26012","inReplyTo":"m3pqta1ou3.fsf@localhost.localdomain","subject":"Re: [PATCH 09/18] gitweb: Revert reset_output() back to original code","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-10T02:43:08Z","receivedAt":"2010-12-10T02:43:08Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":">> Reverted change to reset_output as\n>>\n>> \topen STDOUT, \">&\", \\*STDOUT_REAL;\n> \n> For somebody not following our discussion the above would be very,\n> very cryptic... though I suppose this would be squashed in final\n> (ready to be merged in) version of the code.\n> \n>> causes assertion failures:\n>>\n>> \tAssertion !((((s->var)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVLV)) failed: file \"scalar.xs\", line 49 at gitweb.cgi line 1221.\n> \n> It looks like bug in Perl, because it should give some kind of Perl\n> error, not failed assertion from within guts of Perl C code.\n> \n> Which Perl version are you using?\n\nThis is perl, v5.10.0 built for x86_64-linux-thread-multi\n\n>> if we encounter an error *BEFORE* we've ever changed the output.\n> \n> And how to reproduce this error (i.e. how did you found it)?\n\nCause an error to occur before the caching engine switches output, for\ninstance fail on creating the cache dir, or disable caching all together\nand generate an error. I think the former is where I noticed it, it was\nconsistent though.\n\n- John 'Warthog9' Hawley\n"},{"id":"157791","messageId":"4D01A103.3090900@eaglescrag.net","threadId":"26012","inReplyTo":"m3lj3y1ogb.fsf@localhost.localdomain","subject":"Re: [PATCH 10/18] gitweb: Adding isBinaryAction() and isFeedAction() to determine the action type","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-10T03:39:47Z","receivedAt":"2010-12-10T03:39:47Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":">> This is fairly self explanitory, these are here just to centralize the checking\n>> for these types of actions, as special things need to be done with regards to\n>> them inside the caching engine.\n>>\n>> isBinaryAction() returns true if the action deals with creating binary files\n>> (this needing :raw output)\n> \n> Why do you need special case binary / :raw output? It is not really\n> necessary if it is done in right way, as shown in my rewrite.\n\nBecause that's not how my caching engine does it, and the reason for\nthat is I am mimicking how the rest of gitweb does it.\n\nI attempted at one point to do as you were suggesting, and it became too\ncumbersome. I eventually broke out the 'binary' packages into a special\ncase (thus mimicking how gitweb is already doing things), which also\ngives me the advantage of being able to checksum the resulting binary\nout of band, as well as being able to more trivially calculate the file\nsize being sent.\n\n>> isFeedAction() returns true if the action deals with a news feed of some sort,\n>> basically used to bypass the 'Generating...' message should it be a news reader\n>> as those will explode badly on that page.\n> \n> Why blacklisting 'feed', instead of whitelisting HTML-output?\n\nThere are a limited number of feed types and their ilk (standard xml\nformatted feed and atom), there are lots of html-output like things.\nEasier to default and have things work, generally, than to have things\nnot work the way you would expect.\n\n> BTW., please don't use mixedCase names, but underline_separated.\n\nfixed in v9\n\n- John 'Warthog9' Hawley\n"},{"id":"157792","messageId":"4D01A5F3.8030108@eaglescrag.net","threadId":"26012","inReplyTo":"m3hbem1o7a.fsf@localhost.localdomain","subject":"Re: [PATCH 11/18] gitweb: add isDumbClient() check","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-10T04:00:51Z","receivedAt":"2010-12-10T04:00:51Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":">> +\tmy($user_agent) = $ENV{'HTTP_USER_AGENT'};\n> \n> What if $ENV{'HTTP_USER_AGENT'} is unset / undef, e.g. because we are\n> runing gitweb as a script... which includes running gitweb tests?\n\nIt can be disabled for the running of tests, but the default is to show\n'Generating...' vs. not. I'd rather assume there's an intelligent\nclient on the other end and give users a reason why they aren't staring\nat their initial content immediately (and thus thinking something is\nbroken).\n\n>> +\t\n>> +\tif(\n>> +\t\t# wget case\n>> +\t\t$user_agent =~ /^Wget/i\n>> +\t\t||\n>> +\t\t# curl should be excluded I think, probably better safe than sorry\n>> +\t\t$user_agent =~ /^curl/i\n>> +\t ){\n>> +\t\treturn 1;\t# True\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n> \n> Compare (note: handcrafted solution is to whitelist, not blacklist):\n> \n> +sub browser_is_robot {\n> + return 1 if !exists $ENV{'HTTP_USER_AGENT'}; # gitweb run as script\n> + if (eval { require HTTP::BrowserDetect; }) {\n> + my $browser = HTTP::BrowserDetect->new();\n> + return $browser->robot();\n> + }\n> + # fallback on detecting known web browsers\n> + return 0 if ($ENV{'HTTP_USER_AGENT'} =~ /\\b(?:Mozilla|Opera|Safari|IE)\\b/);\n> + # be conservative; if not sure, assume non-interactive\n> + return 1;\n> +}\n\nMy initial look indicated that perl-http-browserdetect wasn't available\nfor RHEL / CentOS 5 - it is however available in EPEL.\n\nHowever there are a couple of things to note about User Agents at all:\n\t- They lie... a lot\n\t- Robots lie even more\n\nBlacklisting is still the better option, by a lot. I'll re-work this\nsome in v9, as I'm fine with the added dependency.\n\n- John 'Warthog9' Hawley\n"},{"id":"157795","messageId":"4D01C123.9050806@eaglescrag.net","threadId":"26012","inReplyTo":"7vwrnieac8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 12/18] gitweb: Change file handles (in caching) to lexical variables as opposed to globs","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-10T05:56:51Z","receivedAt":"2010-12-10T05:56:51Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"On 12/09/2010 04:32 PM, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n>>> +# Global declarations\n>>> +our $cacheFile;\n>>> +our $cacheFileBG;\n>>> +our $cacheFileBinWT;\n>>> +our $cacheFileBin;\n>>\n>> You are trading globs for global (well, package) variables. They are\n>> not lexical filehandles... though I'm not sure if it would be possible\n>> without restructuring code; note that if variable holding filehandle\n>> falls out of scope, then file would be automatically closed.\n> \n> Hmm. why is it a bad idea, when you need to access these from practically\n> everywhere, to use global variables to begin with? To a certain degree,\n> it sounds like an unnecessary burden without much gain to me.\n\nThis was why I used globs in the first place.\n\nTo answer Jakub's question first: yes, some of those are holding locks\nopen while things are happening. Since there's locks open, through a\nlot of functions, it's\n\nGenerally speaking I'm pretty good about opening and closing the files\nwhen it's needed. I *THINK* they can be made local variables, as I\ndon't think anything is kept open (within the caching engine) across\nfunctions.\n\nI've made a couple of changes, I'm going to have to test them, this\nmight be fixed in v9.\n\n- John 'Warthog9' Hawley\n"},{"id":"157796","messageId":"4D01C43D.6070803@eaglescrag.net","threadId":"26012","inReplyTo":"m38vzy1nkl.fsf@localhost.localdomain","subject":"Re: [PATCH 13/18] gitweb: Add commented url & url hash to page footer","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-10T06:10:05Z","receivedAt":"2010-12-10T06:10:05Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":">> \n> \n> Nice idea. It helps with debugging and doesn't introduce information\n> leakage.\n\nThat was the plan, and I've already got a scenario where it would be useful.\n\n>> The first bit tells you what the url that generated the page actually was, the second is\n>> the hash used to store the file with the first two characters being used as the directory:\n>>\n>> /7a/31cfb8a43f5643679eec88aa9d7981\n> \n> Isn't it\n> \n> /7a/7a31cfb8a43f5643679eec88aa9d7981\n> \n> in your series?\n\nNope\n\n\tour $fullhashdir = \"$cachedir/\". substr( $urlhash, 0, 2) .\"/\";\n\nand then a couple of lines later:\n\n\t$fullhashpath = \"$fullhashdir/\". substr( $urlhash, 2 );\n\nright at the top of cache_fetch()\n\n>> Also useful for greping through the existing cache and finding files with unique paths that\n>> you may want to explicitly flush.\n> \n> Though probably better 'cache_admin' page would be ultimately best\n> solution, see proof of concept in\n\nThe biggest problem with the cache admin page you've got there, is that\ngitweb itself doesn't have a framework for user administration,\nprivileges, etc. Limiting it to the local machine is also useless,\nthere are very few people who are going to have access, from 127.0.0.1\nto their web server, and this also breaks anything even remotely\nresembling virtual hosts.\n\nThe fact that it's unusable from virtual hosts makes this pretty much DOA.\n\nLike I've said in the past, we need to at least look at web frameworks\nfor gitweb, and if we want to provide things like the admin page than we\nneed to consider that we are going to need user management. That, in\nparticular, starts drifting towards needing a database to store things\nin and I for one am *NOT* in favor of that.\n\nI like the idea of a framework helping deal with things like page\nlayout, separating data access from content, etc. I do not like the\nidea of gitweb having a full blown setup with a database and all behind it.\n\n- John 'Warthog9' Hawley\n"},{"id":"157797","messageId":"4D01C630.30400@eaglescrag.net","threadId":"26012","inReplyTo":"m34oam1n3t.fsf@localhost.localdomain","subject":"Re: [PATCH 14/18] gitweb: add print_transient_header() function for central header printing","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-10T06:18:24Z","receivedAt":"2010-12-10T06:18:24Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":">> +sub print_transient_header {\n>> +\tprint $::cgi->header(\n> \n> Why you use $::cgi->header() instead of equivalent $cgi->header()?\n> Note that $::cgi->header() is $main::cgi->header(), and is not\n> CGI::header().\n\nBecause $main::cgi already was setup. Since I'm not redefining $cgi\nanywhere they evaluate to the same thing since cgi is already a global\nvariable coming from gitweb itself.\n\nThe way I have it now is it's more explicit to being the parent (main).\n It doesn't really matter either way, but I can change it if you like.\n\n>> +\t\t\t\t-type=>'text/html',\n>> +\t\t\t\t-charset => 'utf-8',\n>> +\t\t\t\t-status=> 200,\n>> +\t\t\t\t-expires => 'now',\n>> +\t\t\t\t# HTTP/1.0\n>> +\t\t\t\t-Pragma => 'no-cache',\n>> +\t\t\t\t# HTTP/1.1\n>> +\t\t\t\t-Cache_Control => join(\n>> +\t\t\t\t\t\t\t', ',\n>> +\t\t\t\t\t\t\tqw(\n>> +\t\t\t\t\t\t\t\tprivate\n>> +\t\t\t\t\t\t\t\tno-cache\n>> +\t\t\t\t\t\t\t\tno-store\n>> +\t\t\t\t\t\t\t\tmust-revalidate\n>> +\t\t\t\t\t\t\t\tmax-age=0\n>> +\t\t\t\t\t\t\t\tpre-check=0\n>> +\t\t\t\t\t\t\t\tpost-check=0\n>> +\t\t\t\t\t\t\t)\n>> +\t\t\t\t\t\t)\n>> +\t\t\t\t);\n>> +\treturn;\n>> +}\n> \n> Why not use\n> \n> \tour %no_cache = (\n> \t\t# HTTP/1.0\n> \t\t-Pragma => 'no-cache',\n> \t\t# HTTP/1.1\n> \t\t-Cache_Control => join(', ', qw(private no-cache no-store must-revalidate\n> \t\t max-age=0 pre-check=0 post-check=0)),\n> \t);\n> \n> (or something like that). This way you can reuse it even if content\n> type is different (e.g. 'text/plain').\n> \n> But that is just a proposal.\n\nFiner grained control, though they have the same basic setup. Probably\nwill add that, though it's not that big of a deal.\n\n- John 'Warthog9' Hawley\n"},{"id":"157798","messageId":"4D01D902.1030102@eaglescrag.net","threadId":"26012","inReplyTo":"m3zksezbkm.fsf@localhost.localdomain","subject":"Re: [PATCH 15/18] gitweb: Add show_warning() to display an immediate warning, with refresh","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-10T07:38:42Z","receivedAt":"2010-12-10T07:38:42Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"On 12/09/2010 05:01 PM, Jakub Narebski wrote:\n> \"John 'Warthog9' Hawley\" writes:\n> \n>> die_error() is an immediate and abrupt action. show_warning() more or less\n>> functions identically, except that the page generated doesn't use the\n>> gitweb header or footer (in case they are broken) and has an auto-refresh\n>> (10 seconds) built into it.\n> \n> Why not use gitweb header/footer? If they are broken, it should be\n> caught in git development. If we don't se them, the show_warning()\n> output would look out of place.\n\nThe only other 'transient' style page, the 'Generating...' page doesn't\nuse it, and I felt that since this was also transient, and only (likely)\nto be seen once it wasn't worth the header & footer.\n\nThat said I've added it back in, in v9.\n\n>> +sub show_warning {\n>> +\t$| = 1;\n> \n> +\tlocal $| = 1;\n> \n> $| is global variable, and otherwise you would turn autoflush for all\n> code, which would matter e.g. for FastCGI.\n\nSince the execution exits immediately after, wouldn't FastCGI reset at\nthat point, since execution of that thread has stopped? Or does FastCGI\nretain everything as is across subsequent executions of a process?\n\n>> +\n> \n> Why 10 seconds?\n\nLong enough to see the error, but not too long to be a nuisance. Mainly\njust there to warn the admin that it did something automatic they may\nnot have been expecting.\n\n>> +\n>> +\n>> +$warning\n>> +\n>> +\n>> +EOF\n>> +\texit(0);\n> \n> \"exit(0)\" and not \"goto DONE_GITWEB\", or \"goto DONE_REQUEST\"?\n\nDONE_REQUEST doesn't actually exist as a label, the exit was used\npartially for my lack of love for goto's, but mostly out of not\nrealizing what that was calling back to (mainly for the excitement of\nthings like PSGI and their ilk)\n\nI will change that that, but considering there are other locations where\nI do explicit exit's and those are actually inherent to the way the\ncaching engine currently works, I might need to go take a look at what's\ngoing on with respect to multi-threaded items inside of PSGI and their\nlike. It's possible the caching engine doesn't actually work on those...\n\n>> +}\n>> +\n>> sub isBinaryAction {\n>> \tmy ($action) = @_;\n> \n> Didn't you ran gitweb tests?\n\nI did, they passed for me - for whatever reason my cache dir wasn't\ncleaned up, and stayed resident once it was created.\n\n- John 'Warthog9' Hawley\n"},{"id":"157799","messageId":"4D01E5CC.6010301@eaglescrag.net","threadId":"26012","inReplyTo":"m3r5dqz9c5.fsf@localhost.localdomain","subject":"Re: [PATCH 17/18] gitweb: Prepare for cached error pages & better error page handling","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-10T08:33:16Z","receivedAt":"2010-12-10T08:33:16Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"> There is no problem with capturing output of die_error, nor there is a\n> problem with caching error pages (perhaps transiently in memory).\n> \n> The problem is that subroutines calling die_error assum that it would\n> exit ending subroutine that is responsible for generating current\n> action; see \"goto DONE_GITWEB\" which should be \"goto DONE_REQUEST\",\n> and which was \"exit 0\" some time ago at the end of die_error().\n> \n> With caching error pages you want die_error to exit $actions{$action}->(),\n> but not exit cache_fetch(). How do you intend to do it?\n\nWell there's one bug in how that function ends in looking at it again,\nbasically the return case shouldn't happen, and that function should\nend, like your suggesting in the first part of your question (with\nrespect to DONE_GITWEB)\n\nIn the second part, your not thinking with the fork() going (though in\nthinking sans the fork this might not work right).\n\nIt's the background process that will call die_error in such a way that\ndie_error_cache will get invoked. die_error_cache will write the .err\nfile out, and the whole thing should just exit.\n\nThough now that I say that there's an obvious bug in the case where\nforking didn't work at all, in that case you would get a blank page as\nthe connection would just be closed. If you refreshed (say hitting F5)\nyou'd get the error at that point.\n\nNeed to fix that non-forked problem though.\n\n>> This adds two functions:\n>>\n>> die_error_cache() - this gets back called from die_error() so\n>> that the error message generated can be cached.\n> \n> *How* die_error_cache() gets called back from die_error()? I don't\n> see any changes to die_error(), or actually any calling sites for\n> die_error_cache() in the patch below.\n> \n>> cacheDisplayErr() - this is a simplified version of cacheDisplay()\n>> that does an initial check, if the error page exists - display it\n>> and exit. If not, return.\n> \n> Errr... isn't it removed in _preceding_ patch? WTF???\n\nin breaking up the series it got included in the wrong spot, and\napparently removed and re-added correctly, should be fixed in v9\n\n>> +sub die_error_cache {\n>> +\tmy ($output) = @_;\n>> +\n>> +\topen(my $cacheFileErr, '>:utf8', \"$fullhashpath.err\");\n>> +\tmy $lockStatus = flock($cacheFileErr,LOCK_EX|LOCK_NB);\n> \n> Why do you need to lock here? A comment would be nice.\n\nAt any point when a write happens there's the potential for multiple\nsimultaneous writes. Locking becomes obvious, when your trying to\nprevent multiple processes from writing to the same thing at the same\ntime...\n\n>> +\n>> +\tif (! $lockStatus ){\n>> +\t\tif ( $areForked ){\n> \n> Grrrr...\n> \n> But if it is here to stay, a comment if you please.\n> \n>> +\t\t\texit(0);\n>> +\t\t}else{\n>> +\t\t\treturn;\n>> +\t\t}\n>> +\t}\n\nThe exit(0) or return have been removed in favor of DONE_GITWEB, as\nwe've already errored if we are broken here we should just die.\n\n>> +\n>> +\t# Actually dump the output to the proper file handler\n>> +\tlocal $/ = undef;\n>> +\t$|++;\n> \n> Why not\n> \n> +\tlocal $| = 1;\n> \n\nDone.\n\n> \n>> +\tprint $cacheFileErr \"$output\";\n>> +\t$|--;\n>> +\n>> +\tflock($cacheFileErr,LOCK_UN);\n>> +\tclose($cacheFileErr);\n> \n> Closing file will unlock it.\n\nDoesn't really hurt to be explicit though.\n\n>> +\n>> +\tif ( $areForked ){\n>> +\t\texit(0);\n>> +\t}else{\n>> +\t\treturn;\n> \n> So die_error_cache would not actually work like \"die\" here and like\n> die_error(), isn't it?\n\nthat was ejected, it was a bug. DONE_GITWEB is more correct, though I\nmight need to add a hook to display the error message in the case that\nthe process didn't fork.\n\n>> +\t}\n>> +}\n>> +\n>> \n>> sub cacheWaitForUpdate {\n>> \tmy ($action) = @_;\n>> @@ -380,6 +410,28 @@ EOF\n>> \treturn;\n>> }\n>> \n>> +sub cacheDisplayErr {\n>> +\n>> +\treturn if ( ! -e \"$fullhashpath.err\" );\n>> +\n>> +\topen($cacheFileErr, '<:utf8', \"$fullhashpath.err\");\n>> +\t$lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);\n>> +\n>> +\tif (! $lockStatus ){\n>> +\t\tshow_warning(\n>> +\t\t\t\t\"

\".\n>> +\t\t\t\t\"*** Warning ***: Locking error when trying to lock error cache page, file $fullhashpath.err
/\\n\".\n> \n> esc_path\n> \n>> +\t\t\t\t\"This is about as screwed up as it gets folks - see your systems administrator for more help with this.\".\n>> +\t\t\t\t\"

\"\n>> +\t\t\t\t);\n>> +\t}\n>> +\n>> +\twhile( <$cacheFileErr> ){\n>> +\t\tprint $_;\n>> +\t}\n> \n> Why not 'print <$cacheFileErr>' (list context), like in insert_file()\n> subroutine?\n\nI've had buffer problems with 'print <$cacheFileErr>' in some cases.\nThis is small enough it shouldn't happen, but I've gotten into the habit\nof doing it this way. I can change it if you like.\n\n> \n>> +\texit(0);\n>> +}\n> \n> Callsites?\n> \n> Note: I have't read next commit yet.\n\nNext patch.\n\nIf you'd rather I can squash 17 & 18 into a single commit.\n\n- John 'Warthog9' Hawley\n"},{"id":"157813","messageId":"201012101310.55094.jnareb@gmail.com","threadId":"26012","inReplyTo":"4D01A103.3090900@eaglescrag.net","subject":"Re: [PATCH 10/18] gitweb: Adding isBinaryAction() and isFeedAction() to determine the action type","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T12:10:53Z","receivedAt":"2010-12-10T12:10:53Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 10 Dec 2010, J.H. wrote:\n\n>>> This is fairly self explanatory, these are here just to centralize the checking\n>>> for these types of actions, as special things need to be done with regards to\n>>> them inside the caching engine.\n>>>\n>>> isBinaryAction() returns true if the action deals with creating binary files\n>>> (this needing :raw output)\n>> \n>> Why do you need special case binary / :raw output? It is not really\n>> necessary if it is done in right way, as shown in my rewrite.\n> \n> Because that's not how my caching engine does it, and the reason for\n> that is I am mimicking how the rest of gitweb does it.\n\nTo shorten the explanation why treating binary (needing :raw) output in\na special way is not necessary: with the way gitweb code is structured\n(with \"binmode STDOUT, ':raw'\" inside action subroutine), with the way\ncapturing output is done (by redirecting STDOUT), and even with the way\nkernel.org caching code is structured the only thing that needs to be\ndone to support both text (:utf8, as set at beginning of gitweb) and\nbinary (:raw) output is to *dump cache to STDOUT in binary mode*:\n\n\tbinmode $cache_fh, ':raw';\n\tbinmode STDOUT, ':raw';\n\tFile::Copy::copy($fh, \\*STDOUT);\n\nNothing more.\n\nJust dump cache file to STDOUT in binary mode.\n \n> I attempted at one point to do as you were suggesting, and it became too\n> cumbersome. I eventually broke out the 'binary' packages into a special\n> case (thus mimicking how gitweb is already doing things), which also\n> gives me the advantage of being able to checksum the resulting binary\n> out of band, as well as being able to more trivially calculate the file\n> size being sent.\n\nI don't see how it needs to be special-cased: the ordinary output would\nalso take advantage of this. Note that plain 'blob' action can also\nbe quite large.\n\nIf there is to be done smarter, i.e. HTTP-aware, parsing and dumping of\ncache entry file, e.g. by reading the HTTP header part to memory and\nfiddling with HTTP headers (e.g. adding Content-Length header), it can be\ndone in a contents-agnostic way.\n\nNote that with the way I do it in my rewrite, namely saving cached output\nto temporary file to rename it to final destination later (atomic update),\nwe can do mungling of HTTP headers before/during this final copying to\nfinal file, e.g. calculating Content-Length and perhaps Content-MD5 \nheaders.\n\n> \n>>> isFeedAction() returns true if the action deals with a news feed of some sort,\n>>> basically used to bypass the 'Generating...' message should it be a news reader\n>>> as those will explode badly on that page.\n>> \n>> Why blacklisting 'feed', instead of whitelisting HTML-output?\n> \n> There are a limited number of feed types and their ilk (standard xml\n> formatted feed and atom), there are lots of html-output like things.\n> Easier to default and have things work, generally, than to have things\n> not work the way you would expect.\n\nAh, I see from what you written in other subthreads of this thread that\nyou prefer to have \"Generating...\" page where it is not wanted that not\nhave it where it could be useful (i.e. blacklist approach), while I took\nthe opposite side (i.e. whitelist approach).\n\n-- \nJakub Narebski\nPoland\n"},{"id":"157814","messageId":"201012101325.16629.jnareb@gmail.com","threadId":"26012","inReplyTo":"201012101310.55094.jnareb@gmail.com","subject":"Re: [PATCH 10/18] gitweb: Adding isBinaryAction() and isFeedAction() to determine the action type","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T12:25:15Z","receivedAt":"2010-12-10T12:25:15Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 10 Dec 2010, Jakub Narebski wrote:\n> On Fri, 10 Dec 2010, J.H. wrote:\n> \n>>>> This is fairly self explanatory, these are here just to centralize the checking\n>>>> for these types of actions, as special things need to be done with regards to\n>>>> them inside the caching engine.\n>>>>\n>>>> isBinaryAction() returns true if the action deals with creating binary files\n>>>> (this needing :raw output)\n>>> \n>>> Why do you need special case binary / :raw output? It is not really\n>>> necessary if it is done in right way, as shown in my rewrite.\n>> \n>> Because that's not how my caching engine does it, and the reason for\n>> that is I am mimicking how the rest of gitweb does it.\n> \n> To shorten the explanation why treating binary (needing :raw) output in\n> a special way is not necessary: with the way gitweb code is structured\n> (with \"binmode STDOUT, ':raw'\" inside action subroutine), with the way\n> capturing output is done (by redirecting STDOUT), and even with the way\n> kernel.org caching code is structured the only thing that needs to be\n> done to support both text (:utf8, as set at beginning of gitweb) and\n> binary (:raw) output is to *dump cache to STDOUT in binary mode*:\n> \n> \tbinmode $cache_fh, ':raw';\n> \tbinmode STDOUT, ':raw';\n> \tFile::Copy::copy($fh, \\*STDOUT);\n> \n> Nothing more.\n> \n> Just dump cache file to STDOUT in binary mode.\n\nNote that special-casing binary output means that you would never be able\nto replace custom caching engine with e.g. CHI with Memcached backend,\nbecause that treating some actions in a special way interleaves gitweb\ncode with guts of caching code.\n\nAnd memcached might be a way that kernel.org would have to go...\n-- \nJakub Narebski\nPoland\n"},{"id":"157819","messageId":"201012101448.52936.jnareb@gmail.com","threadId":"26012","inReplyTo":"4D019288.9060503@eaglescrag.net","subject":"Re: [PATCH 07/18] gitweb: Revert back to $cache_enable vs. $caching_enabled","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T13:48:50Z","receivedAt":"2010-12-10T13:48:50Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 10 Dec 2010, J.H. wrote:\n\n> > Formally, there is no backward compatibility with any released code.\n> > Using out-of-tree patches is on one's own risk.\n> \n> I will have to beg to differ with you on this, the entirety of the\n> existing caching engine has been released code for a number of years,\n> there are rpm packages available for it, at the very least, in Fedora\n> and in EPEL.\n> \n> The caching engine *IS* released code, and this patchset is as much a\n> new feature as an attempt to merge a fork. Kernel.org isn't the only\n> one running this code, and that has been the case for several years now\n> already.\n> \n> Claiming that this isn't released code is doing me a disservice to me,\n> and those who have submitted patches to it independent of git and the\n> mainline gitweb.\n> \n> Thinking about the patch series outside of that context will lead to me\n> putting my foot down and arguing on those other users behalf. I'm not\n> keen on breaking them for no good reason, and I'm not seeing your change\n> here as one that's particularly worthwhile, while causing external\n> breakage for no reason.\n\nI am so very sorry. Please excuse me. I didn't intent this to be arguing\nagainst backwards compatibility with what amounts to gitweb fork, but rather\ngrumbling about maintaining our mistakes due to backwards compatibility\nrequirement. I see now that it reads as arguing for breaking backwards\ncompatibility: the \"Formally\" qualifier is too weak.\n\nThat said I would rather there was no need for forking, or at least for\nthe caching patches to be peer-reviewed on git mailing list, even if they\nwouldn't be accepted / merged in, or merged in soon enough to avoid need\nfor fork.\n\n> \n> > But even discarding that, I'd rather use the same solution as in\n> > \n> > [PATCHv6/RFC 22/24] gitweb: Support legacy options used by kernel.org caching engine\n> > http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163058\n> > http://repo.or.cz/w/git/jnareb-git.git/commitdiff/27ec67ad90ecd56ac3d05f6a9ea49b6faabf7d0a\n> > \n> > i.e.\n> > \n> > our $cache_enable;\n> > \n> > [...]\n> > \n> > # somewhere just before call to cache_fetch()\n> > $caching_enabled = !!$cache_enable if defined $cache_enable;\n> > \n> >>\n> >> This reverts back to the previous variable to enable / disable caching\n> \n> Is there really any point in changing the name at all? The intention of\n> cache_enable, at one point, was to allow for other caching engines and\n> while there aren't any other caching engines that use it, it's already\n> treated identically to cache_enable.\n> \n> If it really adds enough to the readability to the code, then I'm fine\n> with adding:\n> \n> \t$caching_enabled = $cache_enable if defined $cache_enable;\n> \n> But now you are setting up two variables that control the same thing,\n> adding the possibility for conflicts and confusion to end users.\n> \n> I just want that stated.\n\nI guess I can live (I'd have to live) with $cache_enable instead of\n$caching_enabled as name of *boolean* variable controlling whether\ncaching is turned on or off. Though I'd argue that $caching_enabled\nis better name:\n\n if ($caching_enabled) {\n\nreads naturally as \"if caching [is] enabled\"; not so with $cache_enable.\n$cache_enable as enum is just a bad, bad idea, as is conflating enabling\ncaching with selecting caching engine (c.f. http://lwn.net/Articles/412131/\nthough only very peripherally - it is about other \"conflated designs\").\n\n\nBTW. when leaving $cache_enable from \"[PATCHv6/RFC 22/24] gitweb: Support\nlegacy options used by kernel.org caching engine\" I forgot that it is\nactually $cache_enable (which is 0 by default) that needs to be set up\nif one wants caching. All the rest of cache config variables can be left\nat their default values... though, J.H., are they?\n\n> \n> Also, why the double negative in your original snippet - that doesn't\n> entirely make sense....\n\nI don't know why I felt that I needed to convert it to bool...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"157821","messageId":"201012101510.16504.jnareb@gmail.com","threadId":"26012","inReplyTo":"4D01D902.1030102@eaglescrag.net","subject":"Re: [PATCH 15/18] gitweb: Add show_warning() to display an immediate warning, with refresh","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T14:10:15Z","receivedAt":"2010-12-10T14:10:15Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 10 Dec 2010, J.H. wrote:\n> On 12/09/2010 05:01 PM, Jakub Narebski wrote:\n>> \"John 'Warthog9' Hawley\" writes:\n>> \n>>> die_error() is an immediate and abrupt action. show_warning() more or less\n>>> functions identically, except that the page generated doesn't use the\n>>> gitweb header or footer (in case they are broken) and has an auto-refresh\n>>> (10 seconds) built into it.\n>> \n>> Why not use gitweb header/footer? If they are broken, it should be\n>> caught in git development. If we don't se them, the show_warning()\n>> output would look out of place.\n> \n> The only other 'transient' style page, the 'Generating...' page doesn't\n> use it, and I felt that since this was also transient, and only (likely)\n> to be seen once it wasn't worth the header & footer.\n> \n> That said I've added it back in, in v9.\n\nWell, the contents and feel of show_warning() is more like die_error()\nrather than \"Generating...\" page, so I feel that if die_error() conforms\nto style of rest of gitweb pages, then show_warning() should too.\n \n>>> +sub show_warning {\n>>> +\t$| = 1;\n>> \n>> +\tlocal $| = 1;\n>> \n>> $| is global variable, and otherwise you would turn autoflush for all\n>> code, which would matter e.g. for FastCGI.\n> \n> Since the execution exits immediately after, wouldn't FastCGI reset at\n> that point, since execution of that thread has stopped? Or does FastCGI\n> retain everything as is across subsequent executions of a process?\n\nWell, with exit(0) it is a moot point... but it is good habit to localize\npunctation variables ($|, $/,)\n \n>>> +\n>> \n>> Why 10 seconds?\n> \n> Long enough to see the error, but not too long to be a nuisance. Mainly\n> just there to warn the admin that it did something automatic they may\n> not have been expecting.\n\nA comment if you please, then?\n \nHmmm... I guess there is no ned to make it configurable.\n\n>>> +\n>>> +\n>>> +$warning\n>>> +\n>>> +\n>>> +EOF\n>>> +\texit(0);\n>> \n>> \"exit(0)\" and not \"goto DONE_GITWEB\", or \"goto DONE_REQUEST\"?\n> \n> DONE_REQUEST doesn't actually exist as a label,\n\nErrr... DONE_REQUEST was introduced in\n\n [PATCH/RFC] gitweb: Go to DONE_REQUEST rather than DONE_GITWEB in die_error\n Message-ID: <1290723308-21685-1-git-send-email-jnareb@gmail.com>\n http://permalink.gmane.org/gmane.comp.version-control.git/162156\n\n\n> the exit was used \n> partially for my lack of love for goto's, but mostly out of not\n> realizing what that was calling back to (mainly for the excitement of\n> things like PSGI and their ilk)\n\n\nYou would have to do more than that. ModPerl::Registry that is used\nfor mod_perl support (which as deployment is I guess more widespread\nthan PSGI via wrapper using Plack::App::WrapCGI, or FastCGI deployment)\nredefines 'exit' so that CGI scripts that use 'exit' to end request\nkeep working without need to restart worker at each request; for real\nexit, for example from background process, you need to use CORE::exit.\nSee e.g. http://repo.or.cz/w/git/jnareb-git.git/commitdiff/8bd99a6d37cc\nthe ->_set_maybe_background() method.\n\n> \n> I will change that that, but considering there are other locations where\n> I do explicit exit's and those are actually inherent to the way the\n> caching engine currently works, I might need to go take a look at what's\n> going on with respect to multi-threaded items inside of PSGI and their\n> like. It's possible the caching engine doesn't actually work on those...\n\nThat would be a pity. In my rewrite I tried to take into acount both\nnon-persistent (plain CGI, running as script) and persistent (mod_perl,\nFastCGI, PSGI) web environments.\n\n>>> +}\n>>> +\n>>> sub isBinaryAction {\n>>> \tmy ($action) = @_;\n>> \n>> Didn't you ran gitweb tests?\n> \n> I did, they passed for me - for whatever reason my cache dir wasn't\n> cleaned up, and stayed resident once it was created.\n\nHmmm... I wonder why new tests in t9502 and t9503 didn't pass for me...\n\n\nP.S. I'll write separate email about problems with die_error, die-ing\nand output caching.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"157845","messageId":"201012102133.31816.jnareb@gmail.com","threadId":"26012","inReplyTo":"4D01E5CC.6010301@eaglescrag.net","subject":"Re: [PATCH 17/18] gitweb: Prepare for cached error pages & better error page handling","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-10T20:33:31Z","receivedAt":"2010-12-10T20:33:31Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 10 Dec 2010, J.H. wrote:\n\n> > There is no problem with capturing output of die_error, nor there is a\n> > problem with caching error pages (perhaps transiently in memory).\n> > \n> > The problem is that subroutines calling die_error assum that it would\n> > exit ending subroutine that is responsible for generating current\n> > action; see \"goto DONE_GITWEB\" which should be \"goto DONE_REQUEST\",\n> > and which was \"exit 0\" some time ago at the end of die_error().\n> > \n> > With caching error pages you want die_error to exit $actions{$action}->(),\n> > but not exit cache_fetch(). How do you intend to do it?\n> \n> Well there's one bug in how that function ends in looking at it again,\n> basically the return case shouldn't happen, and that function should\n> end, like your suggesting in the first part of your question (with\n> respect to DONE_GITWEB)\n> \n> In the second part, your not thinking with the fork() going (though in\n> thinking sans the fork this might not work right).\n> \n> It's the background process that will call die_error in such a way that\n> die_error_cache will get invoked. die_error_cache will write the .err\n> file out, and the whole thing should just exit.\n> \n> Though now that I say that there's an obvious bug in the case where\n> forking didn't work at all, in that case you would get a blank page as\n> the connection would just be closed. If you refreshed (say hitting F5)\n> you'd get the error at that point.\n> \n> Need to fix that non-forked problem though.\n\nWell, if you, the author, cannot follow code flow of your own code, what\ndoes it matter for being sure that this code is bug free? What does this\nmatter for maintability of this code?\n \n\nThat rant aside, error / exception handling in gitweb is currently not\nfitting well with output caching, at least the locking one.\n\ndie_error() functions as a kind of exception handling; we rely that on\nthe fact that calling die_error() would end request, independent on how\ndeep in the stack we are. Originally die_error() ended with 'exit',\nwhich ModPerl::Registry redefined for it to end request and not exit\nworker. Then 'exit' was replaced by 'goto DONE_GITWEB' to jump out of\nseveral levels of calls; I didn't know then about ModPerl::Registry\nredefining 'exit'... and actually it should be 'goto DONE_REQUEST', like\nin \"[PATCH/RFC] gitweb: Go to DONE_REQUEST rather than DONE_GITWEB in\ndie_error\"\n\n http://permalink.gmane.org/gmane.comp.version-control.git/162156\n\nIt is because die_error is exception mechanism, and in current incarnation\nalways ends request, that is why error pages (generated by die_error) were\nnot cached: we jump out of capturing and out of caching. The additional\nreasoning is that we don't want to bloat cache with error pages, which \nIMHO usually gets requested only once (assumption: different clients makes\ndifferent errors).\n\nNow in most cases the approach taken to modify die_error for caching only\nby adding explicit turning off capturing at the beginning of die_error is\nenough.\n\n1. Single client, no generating in background (note that if given URL is\n never cached, we would not invoke background generation to refresh shown\n stale data - there wouldn't be stale data).\n\n In this case if there is an expected error, die_error() gets explicitely\n invoked, turns off capturing, prints error page to client, and ends\n request.\n\n In the case of uncaught \"die\", it would be caught by CGI::Carp error\n handler, and passed to handle_errors_html() subroutine (thanks to gitweb\n using set_message(\\&handle_errors_html)), which runs die_error() with\n options making it not print HTTP header (which was already printed by\n CGI::Carp error handler), and not exit - the CGI::Carp error handler\n would end request instead. die_error() turns of capturing, prints\n error page, and CGI::Carp error handler ends request.\n\n2. Two clients arriving at exactly the same error (same link), at the\n same time. This is quite unlikely.\n\n In my rewrite there is loop in ->compute method in rewritten caching\n engine, which reads:\n\n do {\n ...\n } until ( || );\n\n This means that one client acquires writers lock, die_error prints error\n page and exists, second client notices that it didn't get anything but\n didn't try it itself yet, and dies itself on die_error()\n\n Dealing with \"die\"-ing works the same as in the case described above,\n so there is no problem from this area neither.\n\n Alternate solution would be to treat it as the case described below.\n\n3. Gitweb runs generating cache entry in background. Note that if error\n pages are not cached, there would be no stale pages to serve while \n regenerating data in background - so entering background process can\n be done only thanks to \"Generating...\" page.\n \n We can try _prevent this from happening_, as I did in my rewrite by\n introducing initial/startup delay in \"Generating...\" (which has also\n other reasons to use), or via 'generating_info_is_safe' protection.\n\n Otherwise we need to pass error page from background process to\n foreground proces that is showing \"Generating...\" page; well, to be\n more exact, with current workings of \"Generating...\" it would be its\n successor (next request, after reload / refresh).\n\n Note: the fact that it is *next request* that needs an error page\n (otherwise we would show \"Generating...\" page yet again).\n\n So what die_error needs to do if it finds itself in the background\n process (perhaps explicit $background boolean variable, perhaps\n comparing $$ with $my_pid, perhaps checking if STDOUT is closed)\n it needs to somehow write cache entry, perhaps in a special way\n marking it as error page. The problem is to do it in generic way,\n that would not make it impossible to use other caching engine, or\n other capturing engine, in the future.\n\n Note also that at the end of background process (perhaps at the\n end of die_error) we need to exit process, and not just end request,\n so we should use 'CORE::exit(0);'.\n\nThe problem with 3rd case makes me think that it is high time that\ndie_error use Perl 5 exception throwing and handling mechanism, namely\n\"die\" (for throwing errors, to be used in die_error), and \"eval BLOCK\"\n(to catch errors).\n\nAs proposed on #perl channel when asking about this situation, die_error\nwould use 'die \\$DIE_ERROR' to throw reference, or throw an object, to\neasy distinguish between handled error from die_error, and unhandled\nerror from Perl (where we assume that all errors are strings).\n\nrun_request() or run() would then use 'eval { ... }', which has the\nadditional advantage that we can get rid of CGI::Carp::set_message,\nwhich doesn't allow to use custom HTTP status, and supposedly doesn't\nwork with mod_perl 2.0. Instead of adding capture_stop() to die_error(),\nthe capture mechanism should use 'eval { ... }', and just print response\nif there was exception (like Capture::Tiny does)... or return captured\nerror page to be cached in the case of being in background process.\n\n\nWell, any way we choose to handle it, the code should be very clear,\nand handle all cases (other caching engines, perhaps also other capture\nengines, non-persistent and persistent environments, redefined 'exit'\nlike in ModPerl::Registry case, not redefined 'exit' like I think in\nFastCGI case, etc., etc.).\n\n>>> This adds two functions:\n>>>\n>>> die_error_cache() - this gets back called from die_error() so\n>>> that the error message generated can be cached.\n>> \n>> *How* die_error_cache() gets called back from die_error()? I don't\n>> see any changes to die_error(), or actually any calling sites for\n>> die_error_cache() in the patch below.\n>> \n>>> cacheDisplayErr() - this is a simplified version of cacheDisplay()\n>>> that does an initial check, if the error page exists - display it\n>>> and exit. If not, return.\n>> \n>> Errr... isn't it removed in _preceding_ patch? WTF???\n> \n> in breaking up the series it got included in the wrong spot, and\n> apparently removed and re-added correctly, should be fixed in v9\n\n[...]\n> \n> If you'd rather I can squash 17 & 18 into a single commit.\n\nYes, please. Splitting those changes into 17 & 18 didn't make it more\nclear (usually smaller commit == easier to review), but rather less\ntransparent.\n \n>>> +sub die_error_cache {\n>>> +\tmy ($output) = @_;\n>>> +\n>>> +\topen(my $cacheFileErr, '>:utf8', \"$fullhashpath.err\");\n>>> +\tmy $lockStatus = flock($cacheFileErr,LOCK_EX|LOCK_NB);\n>> \n>> Why do you need to lock here? A comment would be nice.\n> \n> At any point when a write happens there's the potential for multiple\n> simultaneous writes. Locking becomes obvious, when your trying to\n> prevent multiple processes from writing to the same thing at the same\n> time...\n\nOr you can use 'write to File::Temp::tempfile, rename file' trick for\natomic update to file. I use it in early commits in my rewrite of gitweb\ncaching, see e.g.:\n\n http://repo.or.cz/w/git/jnareb-git.git/blob/refs/heads/gitweb/cache-kernel-v6:/gitweb/lib/GitwebCache/SimpleFileCache.pm#l362\n \n>>> +\n>>> +\tif (! $lockStatus ){\n>>> +\t\tif ( $areForked ){\n>> \n>> Grrrr...\n\nGlobal variables, action at considerable distance.\n \n>> But if it is here to stay, a comment if you please.\n>> \n>>> +\t\t\texit(0);\n>>> +\t\t}else{\n>>> +\t\t\treturn;\n>>> +\t\t}\n>>> +\t}\n> \n> The exit(0) or return have been removed in favor of DONE_GITWEB, as\n> we've already errored if we are broken here we should just die.\n\nNote the difference between exit and Core::exit, when running gitweb\nfrom mod_perl using ModPerl::Registry handler.\n \n\n>>> +\n>>> +\tflock($cacheFileErr,LOCK_UN);\n>>> +\tclose($cacheFileErr);\n>> \n>> Closing file will unlock it.\n> \n> Doesn't really hurt to be explicit though.\n\nO.K., but please note that I have found LOCK_UN to be unreliable.\n \n>>> +\n>>> +\tif ( $areForked ){\n>>> +\t\texit(0);\n>>> +\t}else{\n>>> +\t\treturn;\n>> \n>> So die_error_cache would not actually work like \"die\" here and like\n>> die_error(), isn't it?\n> \n> that was ejected, it was a bug. DONE_GITWEB is more correct, though I\n> might need to add a hook to display the error message in the case that\n> the process didn't fork.\n\nBy the way, why do you fork indiscriminately (remember that forking\nis not without performance cost), even when background generation is\nturned off, or you don't need background generation?\n \nWouldn't fallback on non-background generation if fork() fails, as in\nmy rewrite of gitweb caching series be a better solution?\n\n>>> +\twhile( <$cacheFileErr> ){\n>>> +\t\tprint $_;\n>>> +\t}\n>> \n>> Why not 'print <$cacheFileErr>' (list context), like in insert_file()\n>> subroutine?\n> \n> I've had buffer problems with 'print <$cacheFileErr>' in some cases.\n> This is small enough it shouldn't happen, but I've gotten into the habit\n> of doing it this way. I can change it if you like.\n\nPerhaps\n\n print while <$cacheFileErr>;\n\n(we use it in already in \"print while <$fd>;\" in git_blame_common())?\n\nOr, if we use File::Copy, perhaps File::Copy::copy($cacheFileErr, \\*STDOUT);\nor something like that.\n \n-- \nJakub Narebski\nPoland\n"},{"id":"157857","messageId":"7vzksd9nq2.fsf@alter.siamese.dyndns.org","threadId":"26012","inReplyTo":"4D01A5F3.8030108@eaglescrag.net","subject":"Re: [PATCH 11/18] gitweb: add isDumbClient() check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-11T00:07:01Z","receivedAt":"2010-12-11T00:07:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"J.H.\" writes:\n\n> My initial look indicated that perl-http-browserdetect wasn't available\n> for RHEL / CentOS 5 - it is however available in EPEL.\n>\n> However there are a couple of things to note about User Agents at all:\n> \t- They lie... a lot\n> \t- Robots lie even more\n>\n> Blacklisting is still the better option, by a lot. I'll re-work this\n> some in v9, as I'm fine with the added dependency.\n\nThanks, both. I sense that we finally are going to get a single version\nof gitweb that can be used at larger sites ;-)\n"},{"id":"157858","messageId":"201012110115.16225.jnareb@gmail.com","threadId":"26012","inReplyTo":"7vzksd9nq2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 11/18] gitweb: add isDumbClient() check","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-11T00:15:15Z","receivedAt":"2010-12-11T00:15:15Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> \"J.H.\" writes:\n> \n> > My initial look indicated that perl-http-browserdetect wasn't available\n> > for RHEL / CentOS 5 - it is however available in EPEL.\n> >\n> > However there are a couple of things to note about User Agents at all:\n> > \t- They lie... a lot\n> > \t- Robots lie even more\n> >\n> > Blacklisting is still the better option, by a lot. I'll re-work this\n> > some in v9, as I'm fine with the added dependency.\n> \n> Thanks, both. I sense that we finally are going to get a single version\n> of gitweb that can be used at larger sites ;-)\n\nI wouldn't be so optimistic. While we borrow features and ideas from\neach other, the difference still remains that J.H. patches are bit hacky\nbut are tested, while my rewrite is IMHO cleaner but untested (well, \nuntested on real life load).\n\nAnyway the main issue that was discovered by PATCHv6 by me, and v8 by J.H.\nis that die_error sucks... well, at least if background caching is enabled.\n\nAnyway, J.H. plans v9, I plan shortened rewrite.\n-- \nJakub Narebski\nPoland\n"},{"id":"157860","messageId":"4D02D0C4.2020207@eaglescrag.net","threadId":"26012","inReplyTo":"201012110115.16225.jnareb@gmail.com","subject":"Re: [PATCH 11/18] gitweb: add isDumbClient() check","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-11T01:15:48Z","receivedAt":"2010-12-11T01:15:48Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"On 12/10/2010 04:15 PM, Jakub Narebski wrote:\n> Junio C Hamano wrote:\n>> \"J.H.\" writes:\n>>\n>>> My initial look indicated that perl-http-browserdetect wasn't available\n>>> for RHEL / CentOS 5 - it is however available in EPEL.\n>>>\n>>> However there are a couple of things to note about User Agents at all:\n>>> \t- They lie... a lot\n>>> \t- Robots lie even more\n>>>\n>>> Blacklisting is still the better option, by a lot. I'll re-work this\n>>> some in v9, as I'm fine with the added dependency.\n>>\n>> Thanks, both. I sense that we finally are going to get a single version\n>> of gitweb that can be used at larger sites ;-)\n> \n> I wouldn't be so optimistic. While we borrow features and ideas from\n> each other, the difference still remains that J.H. patches are bit hacky\n> but are tested, while my rewrite is IMHO cleaner but untested (well, \n> untested on real life load).\n\nAt this point I'm not sure there is a way to rectify the two patch\nseries, and while we may borrow ideas from each other it's becoming\nclear that we are both, generally speaking, heading in different\ndirections for what we want and need out of gitweb. Jakub's patches for\nthe admin page are indicative of that.\n\n> Anyway the main issue that was discovered by PATCHv6 by me, and v8 by J.H.\n> is that die_error sucks... well, at least if background caching is enabled.\n\nI'd agree with that, and as such I'm working on a complete re-work of\nerror handling in gitweb for v9. Things are looking pretty good so far,\nbut to claim that it's a non-invasive patch would be akin to selling\nsomeone the Brooklyn bridge.\n\nThat said, the way Gitweb handles it's errors and things like exit are\nappalling and this has been something that's needed doing for a while\nanyway. Guess now's the time to do it. Might be a few days for me to\nget far enough for any of it to be worthwhile sharing, late next week\nmaybe. That said I hit vacation starting on the 20th so it might be\nnext year before that is finalized.\n\n- John 'Warthog9' Hawley\n"},{"id":"157861","messageId":"201012110240.32854.jnareb@gmail.com","threadId":"26012","inReplyTo":"4D02D0C4.2020207@eaglescrag.net","subject":"Re: [PATCH 11/18] gitweb: add isDumbClient() check","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-11T01:40:31Z","receivedAt":"2010-12-11T01:40:31Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 11 Dec 2010, J.H. wrote:\n> On 12/10/2010 04:15 PM, Jakub Narebski wrote:\n>> Junio C Hamano wrote:\n>>> \"J.H.\" writes:\n>>>\n>>>> My initial look indicated that perl-http-browserdetect wasn't available\n>>>> for RHEL / CentOS 5 - it is however available in EPEL.\n>>>>\n>>>> However there are a couple of things to note about User Agents at all:\n>>>> \t- They lie... a lot\n>>>> \t- Robots lie even more\n>>>>\n>>>> Blacklisting is still the better option, by a lot. I'll re-work this\n>>>> some in v9, as I'm fine with the added dependency.\n>>>\n>>> Thanks, both. I sense that we finally are going to get a single version\n>>> of gitweb that can be used at larger sites ;-)\n>> \n>> I wouldn't be so optimistic. While we borrow features and ideas from\n>> each other, the difference still remains that J.H. patches are bit hacky\n>> but are tested, while my rewrite is IMHO cleaner but untested (well, \n>> untested on real life load).\n> \n> At this point I'm not sure there is a way to rectify the two patch\n> series, and while we may borrow ideas from each other it's becoming\n> clear that we are both, generally speaking, heading in different\n> directions for what we want and need out of gitweb. Jakub's patches for\n> the admin page are indicative of that.\n\nActually the cache administration page was just proof of concept. Perhaps\na better solution would be to provide script that can be run to safely\nclean cache (or just heavily outdated entries).\n\n\nWhat I want from caching series is a clean separation between capturing\n(so it can be replaced in the future e.g. by Capture::Tiny, or capturing\nto mmapped fragment for Cache::FastMmap-like cache, or simple capturing\nto memory for Memcached), caching engine (so it can be replaced by some\ngood and tested caching engine, like Cache::Cache, Cache, Cache::Memcached,\nCache::FastMmap, CHI and its drivers and options like cache levels), and\ncaching output module. Modular build makes it easier to catch errors\nand allows for unit testing each component separately. And you can simply\nuse 'require ' instead of doing manual error handling and \nprotecting against redefine errors / multiple include via 'do '.\n\nWhat I don't like is caching engine guts strewn all over the gitweb.\nI'd rather capturing engine was not tied too tightly with gitweb. The\nleast controversial is \"output caching\" part...\n\nAnyway I'd try to keep my rewrite feature-compatibile with J.H. series,\nincluding (from v7) also backward compatibility with cache config option\nnames, including $cache_enable. (Grrr... API/ABI backwards compatibility).\n\n> \n>> Anyway the main issue that was discovered by PATCHv6 by me, and v8 by J.H.\n>> is that die_error sucks... well, at least if background caching is enabled.\n> \n> I'd agree with that, and as such I'm working on a complete re-work of\n> error handling in gitweb for v9. Things are looking pretty good so far,\n> but to claim that it's a non-invasive patch would be akin to selling\n> someone the Brooklyn bridge.\n\nHmmm... I am also thinking about changing the way error handling is done\nin gitweb, but I don't think it would be very invasive: for a non-cached\ncase it would be simply one \"eval\" in run_request() or run(), and \"die\"\ninstead of \"goto DONE_XXX\" in die_error().\n \nNow if only there were HTTP::Server::Simple::FCGI so I would be able to\ntest fastCGI support without need to install mod_fcgi / mod_fastcgi for\nApache... (local::lib and cpanm for the win!).\n\n> That said, the way Gitweb handles it's errors and things like exit are\n> appalling and this has been something that's needed doing for a while\n> anyway. Guess now's the time to do it. Might be a few days for me to\n> get far enough for any of it to be worthwhile sharing, late next week\n> maybe. That said I hit vacation starting on the 20th so it might be\n> next year before that is finalized.\n\nI also don't think that output caching can be done before end of this year,\nsorry.\n\nHmmm... I guess that in shortened minimal version of my rewrite of output\ncaching for gitweb (without zero-size check, adaptive cache lifetime, \nperhaps even without support for alternate caching engines) I should also\ninclude minimal improvement to die_error-handling. Just like there is\n\"gitweb: Prepare for splitting gitweb\" there.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"157877","messageId":"4D045CD6.9060806@eaglescrag.net","threadId":"26012","inReplyTo":"m3vd32z9yk.fsf@localhost.localdomain","subject":"Re: [PATCH 16/18] gitweb: When changing output (STDOUT) change STDERR as well","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-12T05:25:42Z","receivedAt":"2010-12-12T05:25:42Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"> Hmm... anuthing that happens after 'use CGI::Carp;' is parsed should\n> have STDERR redirected to web server logs, see CGI::Carp manpage\n> \n> [...]\n> \n> use CGI::Carp\n> \n> And the standard warn(), die (), croak(), confess() and carp() calls will\n> automagically be replaced with functions that write out nicely time-stamped\n> messages to the HTTP server error log.\n> \n> [...]\n> \n> REDIRECTING ERROR MESSAGES\n> \n> By default, error messages are sent to STDERR. Most HTTPD servers direct\n> STDERR to the server's error log.\n> \n> [...]\n> \n> Especially the second part.\n\nThat was not what I was seeing, so either something I was doing was\nhorking how CGI::Carp works, or their claim that \"most HTTPD server\ndirect STDERR to the server's error log\" is false.\n\n> Could you give us example which causes described misbehaviour?\n\nWhile I was working on the trapping of the error pages I started getting\n500 errors when going to a non-existent sha1. Running the command from\nthe cli revealed that a message from a git command was making it out to\nthe console. Redirecting STDERR masked the error from git, and stopped\npremature data being sent out before the headers were sent.\n\n> I have nothing against this patch: if you have to have it, then you\n> have to have it. I oly try to understand what might be core cause\n> behind the issue that this patch is to solve...\n\nI've re-tried this, if you remove this patch and attempt to visit a\nnon-exist sha1, *boom*\n\nI can only speculate that CGI::Carp only redirects the output inside of\nperl, and does not handle the case when called programs (like git) write\nmore directly to STDERR.\n\n- John 'Warthog9' Hawley\n"},{"id":"157889","messageId":"201012121617.04997.jnareb@gmail.com","threadId":"26012","inReplyTo":"4D045CD6.9060806@eaglescrag.net","subject":"Re: [PATCH 16/18] gitweb: When changing output (STDOUT) change STDERR as well","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-12T15:17:04Z","receivedAt":"2010-12-12T15:17:04Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 12 Dec 2010, J.H. wrote:\n\n> > Hmm... anuthing that happens after 'use CGI::Carp;' is parsed should\n> > have STDERR redirected to web server logs, see CGI::Carp manpage\n> > \n> > [...]\n> > \n> > use CGI::Carp\n> > \n> > And the standard warn(), die (), croak(), confess() and carp() calls will\n> > automagically be replaced with functions that write out nicely time-stamped\n> > messages to the HTTP server error log.\n> > \n> > [...]\n> > \n> > REDIRECTING ERROR MESSAGES\n> > \n> > By default, error messages are sent to STDERR. Most HTTPD servers direct\n> > STDERR to the server's error log.\n> > \n> > [...]\n> > \n> > Especially the second part.\n> \n> That was not what I was seeing, so either something I was doing was\n> horking how CGI::Carp works, or their claim that \"most HTTPD server\n> direct STDERR to the server's error log\" is false.\n> \n> > Could you give us example which causes described misbehaviour?\n> \n> While I was working on the trapping of the error pages I started getting\n> 500 errors when going to a non-existent sha1. Running the command from\n> the cli revealed that a message from a git command was making it out to\n> the console. Redirecting STDERR masked the error from git, and stopped\n> premature data being sent out before the headers were sent.\n\nGenerally if something worked, and stopped working, don't you think\nthat you should concentrate on fixing your code, and not papering\nover the issue?\n\n\nThe fact that \"Running the command from the cli revealed that a message\nfrom a git command was making it out to the console.\" doesn't mean\nanything, because when running gitweb from commandline both stdout\nand stderr are redirected to terminal, by default. So you should\nworry only if there is premature data being sent to standard output,\nwith standard error redirected to /dev/null (2>/dev/null).\n\nWhat CGI::Carp does is (re)define 'die' and 'warn' to support\nfatalsToBrowser and warningsToBrowser, and to add timestamp and other\nauxiliary information: in the end 'die' calls 'CORE::die', and 'warn'\ncalls 'CORE::warn' - both of which write to STDERR. This means that\nwarnings from git commands sent to standard error do not get timestamp\nappended. Note that standard output from git commands run by gitweb\nis always captured.\n \n> > I have nothing against this patch: if you have to have it, then you\n> > have to have it. I oly try to understand what might be core cause\n> > behind the issue that this patch is to solve...\n> \n> I've re-tried this, if you remove this patch and attempt to visit a\n> non-exist sha1, *boom*\n> \n> I can only speculate that CGI::Carp only redirects the output inside of\n> perl, and does not handle the case when called programs (like git) write\n> more directly to STDERR.\n\nCGI::Carp doesn't redirect output: it adds timestamp and prints it to\nSTDERR (unless one use 'carpout') to the result of 'die' and 'warn' calls.\n\n*Without your series* when I visit non-existing sha1, or non-existing\nfile I get correctly 404 error from gitweb. So you have borked something.\n\nThe CGI standard (http://tools.ietf.org/html/rfc3875) doesn't talk about\n'standard error' stream at all; on the other hand it talks only about\n'standard input' and 'standard output'. I have checked with simple CGI\nscript in Perl, that neither using die or warn (both before any HTTP \nheaders are send), neither with plain CGI or with mod_perl \n(ModPerl::Registry), with CGI::Carp I never get the error you see.\nWithout CGI::Carp I get '500 Internal Server Error' instead of nicer\none formatted by CGI::Carp, but I don't get it even without CGI::Carp\nwith 'warn' and printing to STDERR directly.\n\nThe standard error stream either gets discarded (mod_cgid), or is\nwritten to /var/log/httpd/error_log (mod_perl).\n\n-- \nJakub Narebski\nPoland\n"}]}