[PATCH 0/6] Gitweb caching changes v2

[PATCH 0/6] Gitweb caching changes v2

[John 'Warthog9' Hawley]

[-- Attachment #1: Type: text/plain, Size: 44 bytes --]

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 1428 bytes --]


Evening everyone,

This is the latest incarnation of gitweb w/ caching.  This is finally at the point where it should probably start either being considered for inclusion or mainline, or I need to accept that this will never get in and more perminantely fork (as is the case with Fedora where this is going in as gitweb-caching as a parrallel rpm package).

That said this brings the base up to mainline (again), it updates a number of elements in the caching engine, and this is a much cleaner break-out of the tree vs. what I am currently developing against.

New things known to work:
    - Better breakout
    - You can actually disable the cache now

- John 'Warthog9' Hawley 

John 'Warthog9' Hawley (6):
  GITWEB - Load Checking
  GITWEB - Missmatching git w/ gitweb
  GITWEB - Add git:// link to summary pages
  GITWEB - Makefile changes
  GITWEB - File based caching layer
  GITWEB - Separate defaults from main file

 .gitignore                  |    1 +
 Makefile                    |   15 +-
 gitweb/Makefile             |   14 +
 gitweb/cache.pm             |  293 +++++++
 gitweb/gitweb.css           |    6 +
 gitweb/gitweb.perl          | 1821 ++++++++++++++++++++-----------------------
 gitweb/gitweb_defaults.perl |  468 +++++++++++
 7 files changed, 1651 insertions(+), 967 deletions(-)
 create mode 100644 gitweb/Makefile
 create mode 100644 gitweb/cache.pm
 create mode 100644 gitweb/gitweb_defaults.perl

[PATCH 1/6] GITWEB - Load Checking

[John 'Warthog9' Hawley]

[-- Attachment #1: Type: text/plain, Size: 44 bytes --]

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 922 bytes --]


This changes the behavior, slightly, of gitweb so that it verifies
that the box isn't inundated with before attempting to serve gitweb.
If the box is overloaded, it basically returns a 503 server unavailable
until the load falls below the defined threshold.  This helps dramatically
if you have a box that's I/O bound, reaches a certain load and you
don't want gitweb, the I/O hog that it is, increasing the pain the
server is already undergoing.

adds $maxload configuration variable.  Default is a load of 300,
which for most cases should never be hit.

Please note this makes the assumption that /proc/loadavg exists
as there is no good way to read load averages on a great number of
platforms [READ: Windows], or that it's reasonably accurate.

Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
 gitweb/gitweb.perl |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-GITWEB-Load-Checking.patch --]
[-- Type: text/x-patch; name="0001-GITWEB-Load-Checking.patch", Size: 1240 bytes --]

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7e477af..813e48f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -221,6 +221,11 @@ our %avatar_size = (
 	'double'  => 32
 );
 
+# Used to set the maximum load that we will still respond to gitweb queries.
+# if we exceed this than we do the processing to figure out if there's a mirror
+# and redirect to it, or to just return 503 server busy
+our $maxload = 300;
+
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (
@@ -551,6 +556,25 @@ if (-e $GITWEB_CONFIG) {
 	do $GITWEB_CONFIG_SYSTEM if -e $GITWEB_CONFIG_SYSTEM;
 }
 
+# loadavg throttle
+sub get_loadavg() {
+    my $load;
+    my @loads;
+
+    open($load, '<', '/proc/loadavg') or return 0;
+    @loads = split(/\s+/, scalar <$load>);
+    close($load);
+    return $loads[0];
+}
+
+if (get_loadavg() > $maxload) {
+    print "Content-Type: text/plain\n";
+    print "Status: 503 Excessive load on server\n";
+    print "\n";
+    print "The load average on the server is too high\n";
+    exit 0;
+}
+
 # version of the core git binary
 our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
 $number_of_git_cmds++;

[PATCH 2/6] GITWEB - Missmatching git w/ gitweb

[John 'Warthog9' Hawley]

[-- Attachment #1: Type: text/plain, Size: 44 bytes --]

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 651 bytes --]


This adds $missmatch_git so that gitweb can run with a miss-matched
git install.  Gitweb, generally, runs fine on a very broad range of
git versions, but it's not always practicle or useful to upgrade it
every time you upgrade git.

This allows the administrator to realize they are miss-matched, and
should they be so inclined, disable the check entirely and run in
a miss-matched fasion.

This is more here to give an obvious warning as to whats going on
vs. silently failing.

Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
 gitweb/gitweb.perl |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-GITWEB-Missmatching-git-w-gitweb.patch --]
[-- Type: text/x-patch; name="0002-GITWEB-Missmatching-git-w-gitweb.patch", Size: 1613 bytes --]

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 813e48f..d84f4c0 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -221,6 +221,9 @@ our %avatar_size = (
 	'double'  => 32
 );
 
+# This is here to allow for missmatch git & gitweb versions
+our $missmatch_git = '';
+
 # Used to set the maximum load that we will still respond to gitweb queries.
 # if we exceed this than we do the processing to figure out if there's a mirror
 # and redirect to it, or to just return 503 server busy
@@ -579,6 +582,25 @@ if (get_loadavg() > $maxload) {
 our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
 $number_of_git_cmds++;
 
+# There's a pretty serious flaw that we silently fail if git doesn't find something it needs
+# a quick and simple check is to have gitweb do a simple check - are we running on the same
+# version of git that we shipped with - if not, throw up an error so that people doing
+# first installs don't have to debug perl to figure out whats going on
+if (
+	$git_version ne $version
+	&&
+	$missmatch_git eq ''
+){
+	git_header_html();
+	print "<p><b>*** Warning ***</b></p>\n";
+	print "<p>\n";
+	print "This version of gitweb was compiled for <b>$version</b> however git version <b>$git_version</b> was found<br/>\n";
+	print "If you are sure this version of git works with this version of gitweb - please define <b>\$missmatch_git</b> to a non empty string in your git config file.\n";
+	print "</p>\n";
+	git_footer_html();
+	exit;
+}
+
 $projects_list ||= $projectroot;
 
 # ======================================================================

[PATCH 3/6] GITWEB - Add git:// link to summary pages

[John 'Warthog9' Hawley]

[-- Attachment #1: Type: text/plain, Size: 44 bytes --]

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 423 bytes --]


This adds a git:// link to the summary pages should a common
$gitlinkurl be defined (default is nothing defined, thus nothing
shown)

This does make the assumption that the git trees share a common
path, and nothing to date is known to actually make use of the link

Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
 gitweb/gitweb.perl |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0003-GITWEB-Add-git-link-to-summary-pages.patch --]
[-- Type: text/x-patch; name="0003-GITWEB-Add-git-link-to-summary-pages.patch", Size: 1208 bytes --]

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index d84f4c0..7ad096c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -224,6 +224,10 @@ our %avatar_size = (
 # This is here to allow for missmatch git & gitweb versions
 our $missmatch_git = '';
 
+#This is here to deal with an extra link on the summary pages - if it's left blank
+# this link will not be shwon.  If it's set, this will be prepended to the repo and used
+our $gitlinkurl = '';
+
 # Used to set the maximum load that we will still respond to gitweb queries.
 # if we exceed this than we do the processing to figure out if there's a mirror
 # and redirect to it, or to just return 503 server busy
@@ -4454,6 +4458,10 @@ sub git_project_list_body {
 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"log")}, "log") . " | " .
 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"tree")}, "tree") .
 		      ($pr->{'forks'} ? " | " . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"forks")}, "forks") : '') .
+     		if( $gitlinkurl ne '' ){
+			print " | ". $cgi->a({-href => "git://$gitlinkurl/".esc_html($pr->{'path'})}, "git");
+		}
+		print "".
 		      "</td>\n" .
 		      "</tr>\n";
 	}

[PATCH 4/6] GITWEB - Makefile changes

[John 'Warthog9' Hawley]

[-- Attachment #1: Type: text/plain, Size: 44 bytes --]

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 444 bytes --]


This adjust the makefiles so that you can do such things as

	make gitweb

from the top level make tree, or if your in the gitweb directory
itself typing

	make

will call back up to the main Makefile and build gitweb

Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
 Makefile        |    4 +++-
 gitweb/Makefile |   14 ++++++++++++++
 2 files changed, 17 insertions(+), 1 deletions(-)
 create mode 100644 gitweb/Makefile


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0004-GITWEB-Makefile-changes.patch --]
[-- Type: text/x-patch; name="0004-GITWEB-Makefile-changes.patch", Size: 980 bytes --]

diff --git a/Makefile b/Makefile
index 4a1e5bc..8db9d01 100644
--- a/Makefile
+++ b/Makefile
@@ -1509,6 +1509,8 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 	chmod +x $@+ && \
 	mv $@+ $@
 
+.PHONY: gitweb
+gitweb: gitweb/gitweb.cgi
 ifdef JSMIN
 OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js
 gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js
@@ -1537,7 +1539,7 @@ endif
 	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
 	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
 	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
-	    $< >$@+ && \
+	    $(patsubst %.cgi,%.perl,$@) >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
 
diff --git a/gitweb/Makefile b/gitweb/Makefile
new file mode 100644
index 0000000..8d318b3
--- /dev/null
+++ b/gitweb/Makefile
@@ -0,0 +1,14 @@
+SHELL = /bin/bash
+
+FILES = gitweb.cgi
+
+.PHONY: $(FILES)
+
+all: $(FILES)
+
+$(FILES):
+	$(MAKE) $(MFLAGS) -C ../ -f Makefile gitweb/$@
+
+clean:
+	rm -rf $(FILES)
+

[PATCH 6/6] GITWEB - Separate defaults from main file

[John 'Warthog9' Hawley]

[-- Attachment #1: Type: text/plain, Size: 44 bytes --]

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 1090 bytes --]


This is an attempt to break out the default values & associated
documentation from the main gitweb file so that it's easier to
browse / read and understand without the associated code involved.

This helps by making defaults self contained with their documentation
making it easier for someone to read through things and find what
they want

This is also a not-so-subtle start of trying to break up gitweb into
separate files for easier maintainability, having everything in a
single file is just a mess and makes the whole thing more complicated
than it needs to be.  This is a bit of a baby step towards breaking it
up for easier maintenance.

Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
 .gitignore                  |    1 +
 Makefile                    |   15 +-
 gitweb/Makefile             |    2 +-
 gitweb/gitweb.perl          |  515 +++++--------------------------------------
 gitweb/gitweb_defaults.perl |  468 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 537 insertions(+), 464 deletions(-)
 create mode 100644 gitweb/gitweb_defaults.perl


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0006-GITWEB-Separate-defaults-from-main-file.patch --]
[-- Type: text/x-patch; name="0006-GITWEB-Separate-defaults-from-main-file.patch", Size: 40510 bytes --]

diff --git a/.gitignore b/.gitignore
index ac02a58..5e48102 100644
--- a/.gitignore
+++ b/.gitignore
@@ -151,6 +151,7 @@
 /git-core-*/?*
 /gitk-git/gitk-wish
 /gitweb/gitweb.cgi
+/gitweb/gitweb_defaults.pl
 /test-chmtime
 /test-ctype
 /test-date
diff --git a/Makefile b/Makefile
index 8db9d01..2c5f139 100644
--- a/Makefile
+++ b/Makefile
@@ -1510,14 +1510,16 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 	mv $@+ $@
 
 .PHONY: gitweb
-gitweb: gitweb/gitweb.cgi
+gitweb: gitweb/gitweb.cgi gitweb/gitweb_defaults.pl
 ifdef JSMIN
-OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js
-gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js
+OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js gitweb/gitweb_defaults.pl
+gitweb/gitweb.cgi gitweb/gitweb_defaults.pl: gitweb/gitweb.perl gitweb/gitweb.min.js gitweb/gitweb_defaults.perl
 else
-OTHER_PROGRAMS += gitweb/gitweb.cgi
-gitweb/gitweb.cgi: gitweb/gitweb.perl
+OTHER_PROGRAMS += gitweb/gitweb.cgi gitweb/gitweb_defaults.pl
+gitweb/gitweb.cgi: gitweb/gitweb_defaults.pl
+gitweb/gitweb.cgi gitweb/gitweb_defaults.pl: gitweb/gitweb.perl gitweb/gitweb_defaults.perl
 endif
+	#$(QUIET_GEN)$(RM) $@ $@+ &&
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
 	    -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \
@@ -1539,7 +1541,7 @@ endif
 	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
 	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
 	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
-	    $(patsubst %.cgi,%.perl,$@) >$@+ && \
+	    $(patsubst %.cgi,%.perl,$(patsubst %.pl, %.perl, $@)) >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
 
@@ -1913,6 +1915,7 @@ clean:
 	$(MAKE) -C Documentation/ clean
 ifndef NO_PERL
 	$(RM) gitweb/gitweb.cgi
+	$(RM) gitweb/gitweb_defaults.pl
 	$(MAKE) -C perl clean
 endif
 	$(MAKE) -C templates/ clean
diff --git a/gitweb/Makefile b/gitweb/Makefile
index 8d318b3..2bd421a 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -1,6 +1,6 @@
 SHELL = /bin/bash
 
-FILES = gitweb.cgi
+FILES = gitweb.cgi gitweb_defaults.pl
 
 .PHONY: $(FILES)
 
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3b44371..fd41539 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -36,466 +36,67 @@ our $version = "++GIT_VERSION++";
 our $my_url = $cgi->url();
 our $my_uri = $cgi->url(-absolute => 1);
 
-# Base URL for relative URLs in gitweb ($logo, $favicon, ...),
-# needed and used only for URLs with nonempty PATH_INFO
-our $base_url = $my_url;
-
-# When the script is used as DirectoryIndex, the URL does not contain the name
-# of the script file itself, and $cgi->url() fails to strip PATH_INFO, so we
-# have to do it ourselves. We make $path_info global because it's also used
-# later on.
 #
-# Another issue with the script being the DirectoryIndex is that the resulting
-# $my_url data is not the full script URL: this is good, because we want
-# generated links to keep implying the script name if it wasn't explicitly
-# indicated in the URL we're handling, but it means that $my_url cannot be used
-# as base URL.
-# Therefore, if we needed to strip PATH_INFO, then we know that we have
-# to build the base URL ourselves:
-our $path_info = $ENV{"PATH_INFO"};
-if ($path_info) {
-	if ($my_url =~ s,\Q$path_info\E$,, &&
-	    $my_uri =~ s,\Q$path_info\E$,, &&
-	    defined $ENV{'SCRIPT_NAME'}) {
-		$base_url = $cgi->url(-base => 1) . $ENV{'SCRIPT_NAME'};
-	}
-}
-
-# core git executable to use
-# this can just be "git" if your webserver has a sensible PATH
-our $GIT = "++GIT_BINDIR++/git";
-
-# absolute fs-path which will be prepended to the project path
-#our $projectroot = "/pub/scm";
-our $projectroot = "++GITWEB_PROJECTROOT++";
-
-# fs traversing limit for getting project list
-# the number is relative to the projectroot
-our $project_maxdepth = "++GITWEB_PROJECT_MAXDEPTH++";
-
-# target of the home link on top of all pages
-our $home_link = $my_uri || "/";
-
-# string of the home link on top of all pages
-our $home_link_str = "++GITWEB_HOME_LINK_STR++";
-
-# name of your site or organization to appear in page titles
-# replace this with something more descriptive for clearer bookmarks
-our $site_name = "++GITWEB_SITENAME++"
-                 || ($ENV{'SERVER_NAME'} || "Untitled") . " Git";
-
-# filename of html text to include at top of each page
-our $site_header = "++GITWEB_SITE_HEADER++";
-# html text to include at home page
-our $home_text = "++GITWEB_HOMETEXT++";
-# filename of html text to include at bottom of each page
-our $site_footer = "++GITWEB_SITE_FOOTER++";
-
-# URI of stylesheets
-our @stylesheets = ("++GITWEB_CSS++");
-# URI of a single stylesheet, which can be overridden in GITWEB_CONFIG.
-our $stylesheet = undef;
-# URI of GIT logo (72x27 size)
-our $logo = "++GITWEB_LOGO++";
-# URI of GIT favicon, assumed to be image/png type
-our $favicon = "++GITWEB_FAVICON++";
-# URI of gitweb.js (JavaScript code for gitweb)
-our $javascript = "++GITWEB_JS++";
-
-# URI and label (title) of GIT logo link
-#our $logo_url = "http://www.kernel.org/pub/software/scm/git/docs/";
-#our $logo_label = "git documentation";
-our $logo_url = "http://git-scm.com/";
-our $logo_label = "git homepage";
-
-# source of projects list
-our $projects_list = "++GITWEB_LIST++";
-
-# the width (in characters) of the projects list "Description" column
-our $projects_list_description_width = 25;
-
-# default order of projects list
-# valid values are none, project, descr, owner, and age
-our $default_projects_order = "project";
-
-# show repository only if this file exists
-# (only effective if this variable evaluates to true)
-our $export_ok = "++GITWEB_EXPORT_OK++";
-
-# show repository only if this subroutine returns true
-# when given the path to the project, for example:
-#    sub { return -e "$_[0]/git-daemon-export-ok"; }
-our $export_auth_hook = undef;
-
-# only allow viewing of repositories also shown on the overview page
-our $strict_export = "++GITWEB_STRICT_EXPORT++";
-
-# list of git base URLs used for URL to where fetch project from,
-# i.e. full URL is "$git_base_url/$project"
-our @git_base_url_list = grep { $_ ne '' } ("++GITWEB_BASE_URL++");
-
-# default blob_plain mimetype and default charset for text/plain blob
-our $default_blob_plain_mimetype = 'text/plain';
-our $default_text_plain_charset  = undef;
-
-# file to use for guessing MIME types before trying /etc/mime.types
-# (relative to the current git repository)
-our $mimetypes_file = undef;
-
-# assume this charset if line contains non-UTF-8 characters;
-# it should be valid encoding (see Encoding::Supported(3pm) for list),
-# for which encoding all byte sequences are valid, for example
-# 'iso-8859-1' aka 'latin1' (it is decoded without checking, so it
-# could be even 'utf-8' for the old behavior)
-our $fallback_encoding = 'latin1';
-
-# rename detection options for git-diff and git-diff-tree
-# - default is '-M', with the cost proportional to
-#   (number of removed files) * (number of new files).
-# - more costly is '-C' (which implies '-M'), with the cost proportional to
-#   (number of changed files + number of removed files) * (number of new files)
-# - even more costly is '-C', '--find-copies-harder' with cost
-#   (number of files in the original tree) * (number of new files)
-# - one might want to include '-B' option, e.g. '-B', '-M'
-our @diff_opts = ('-M'); # taken from git_commit
-
-# Disables features that would allow repository owners to inject script into
-# the gitweb domain.
-our $prevent_xss = 0;
-
-# information about snapshot formats that gitweb is capable of serving
-our %known_snapshot_formats = (
-	# name => {
-	# 	'display' => display name,
-	# 	'type' => mime type,
-	# 	'suffix' => filename suffix,
-	# 	'format' => --format for git-archive,
-	# 	'compressor' => [compressor command and arguments]
-	# 	                (array reference, optional)
-	# 	'disabled' => boolean (optional)}
-	#
-	'tgz' => {
-		'display' => 'tar.gz',
-		'type' => 'application/x-gzip',
-		'suffix' => '.tar.gz',
-		'format' => 'tar',
-		'compressor' => ['gzip']},
-
-	'tbz2' => {
-		'display' => 'tar.bz2',
-		'type' => 'application/x-bzip2',
-		'suffix' => '.tar.bz2',
-		'format' => 'tar',
-		'compressor' => ['bzip2']},
-
-	'txz' => {
-		'display' => 'tar.xz',
-		'type' => 'application/x-xz',
-		'suffix' => '.tar.xz',
-		'format' => 'tar',
-		'compressor' => ['xz'],
-		'disabled' => 1},
-
-	'zip' => {
-		'display' => 'zip',
-		'type' => 'application/x-zip',
-		'suffix' => '.zip',
-		'format' => 'zip'},
-);
-
-# Aliases so we understand old gitweb.snapshot values in repository
-# configuration.
-our %known_snapshot_format_aliases = (
-	'gzip'  => 'tgz',
-	'bzip2' => 'tbz2',
-	'xz'    => 'txz',
-
-	# backward compatibility: legacy gitweb config support
-	'x-gzip' => undef, 'gz' => undef,
-	'x-bzip2' => undef, 'bz2' => undef,
-	'x-zip' => undef, '' => undef,
-);
-
-# Pixel sizes for icons and avatars. If the default font sizes or lineheights
-# are changed, it may be appropriate to change these values too via
-# $GITWEB_CONFIG.
-our %avatar_size = (
-	'default' => 16,
-	'double'  => 32
+# Define and than setup our configuration 
+#
+our(
+	$VERSION,
+	$path_info,
+	$GIT,
+	$projectroot,
+	$project_maxdepth,
+	$home_link,
+	$home_link_str,
+	$site_name,
+	$site_header,
+	$home_text,
+	$site_footer,
+	@stylesheets,
+	$stylesheet,
+	$logo,
+	$favicon,
+	$javascript,
+	$logo_url,
+	$logo_label,
+	$projects_list,
+	$projects_list_description_width,
+	$default_projects_order,
+	$export_ok,
+	$export_auth_hook,
+	$strict_export,
+	@git_base_url_list,
+	$default_blob_plain_mimetype,
+	$default_text_plain_charset,
+	$mimetypes_file,
+	$missmatch_git,
+	$gitlinkurl,
+	$maxload,
+	$cache_enable,
+	$minCacheTime,
+	$maxCacheTime,
+	$cachedir,
+	$backgroundCache,
+	$nocachedata,
+	$nocachedatabin,
+	$fullhashpath,
+	$fullhashbinpath,
+	$export_auth_hook,
+	%known_snapshot_format_aliases,
+	%known_snapshot_formats,
+	$path_info,
+	$fallback_encoding,
+	%avatar_size,
+	$project_maxdepth,
+	$headerRefresh,
+	$base_url,
+	$projects_list_description_width,
+	$default_projects_order,
+	$prevent_xss,
+	@diff_opts,
+	%feature
 );
 
-# This is here to allow for missmatch git & gitweb versions
-our $missmatch_git = '';
-
-#This is here to deal with an extra link on the summary pages - if it's left blank
-# this link will not be shwon.  If it's set, this will be prepended to the repo and used
-our $gitlinkurl = '';
-
-# Used to set the maximum load that we will still respond to gitweb queries.
-# if we exceed this than we do the processing to figure out if there's a mirror
-# and redirect to it, or to just return 503 server busy
-our $maxload = 300;
-
-# This enables/disables the caching layer in gitweb.  This currently only supports the
-# 'dumb' file based caching layer, primarily used on git.kernel.org.  this is reasonably
-# effective but it has the downside of requiring a huge amount of disk space if there
-# are a number of repositories involved.  It is not uncommon for git.kernel.org to have
-# on the order of 80G - 120G accumulate over the course of a few months.  It is recommended
-# that the cache directory be periodically completely deleted, and this is safe to perform.
-# Suggested mechanism
-# mv $cacheidr $cachedir.flush;mkdir $cachedir;rm -rf $cachedir.flush
-# Value is binary. 0 = disabled (default), 1 = enabled.
-our $cache_enable = 0;
-
-# Used to set the minimum cache timeout for the dynamic caching algorithm.  Basically
-# if we calculate the cache to be under this number of seconds we set the cache timeout 
-# to this minimum.
-# Value is in seconds.  1 = 1 seconds, 60 = 1 minute, 600 = 10 minutes, 3600 = 1 hour
-our $minCacheTime = 20;
-
-# Used to set the maximum cache timeout for the dynamic caching algorithm.  Basically
-# if we calculate the cache to exceed this number of seconds we set the cache timeout 
-# to this maximum.
-# Value is in seconds.  1 = 1 seconds, 60 = 1 minute, 600 = 10 minutes, 3600 = 1 hour
-our $maxCacheTime = 1200;
-
-# If you need to change the location of the caching directory, override this
-# otherwise this will probably do fine for you
-our $cachedir = 'cache';
-
-# If this is set (to 1) cache will do it's best to always display something instead
-# of making someone wait for the cache to update.  This will launch the cacheUpdate
-# into the background and it will lock a <file>.bg file and will only lock the 
-# actual cache file when it needs to write into it.  In theory this will make 
-# gitweb seem more responsive at the price of possibly stale data.
-our $backgroundCache = 1;
-
-# Used to set the maximum cache file life.  If a cache files last modify time exceeds
-# this value, it will assume that the data is just too old, and HAS to be regenerated
-# instead of trying to display the existing cache data.
-# Value is in seconds.  1 = 1 seconds, 60 = 1 minute, 600 = 10 minutes, 3600 = 1 hour
-# 18000 = 5 hours
-our $maxCacheLife = 18000;
-
-# You define site-wide feature defaults here; override them with
-# $GITWEB_CONFIG as necessary.
-our %feature = (
-	# feature => {
-	# 	'sub' => feature-sub (subroutine),
-	# 	'override' => allow-override (boolean),
-	# 	'default' => [ default options...] (array reference)}
-	#
-	# if feature is overridable (it means that allow-override has true value),
-	# then feature-sub will be called with default options as parameters;
-	# return value of feature-sub indicates if to enable specified feature
-	#
-	# if there is no 'sub' key (no feature-sub), then feature cannot be
-	# overriden
-	#
-	# use gitweb_get_feature(<feature>) to retrieve the <feature> value
-	# (an array) or gitweb_check_feature(<feature>) to check if <feature>
-	# is enabled
-
-	# Enable the 'blame' blob view, showing the last commit that modified
-	# each line in the file. This can be very CPU-intensive.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'blame'}{'default'} = [1];
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'blame'}{'override'} = 1;
-	# and in project config gitweb.blame = 0|1;
-	'blame' => {
-		'sub' => sub { feature_bool('blame', @_) },
-		'override' => 0,
-		'default' => [0]},
-
-	# Enable the 'snapshot' link, providing a compressed archive of any
-	# tree. This can potentially generate high traffic if you have large
-	# project.
-
-	# Value is a list of formats defined in %known_snapshot_formats that
-	# you wish to offer.
-	# To disable system wide have in $GITWEB_CONFIG
-	# $feature{'snapshot'}{'default'} = [];
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'snapshot'}{'override'} = 1;
-	# and in project config, a comma-separated list of formats or "none"
-	# to disable.  Example: gitweb.snapshot = tbz2,zip;
-	'snapshot' => {
-		'sub' => \&feature_snapshot,
-		'override' => 0,
-		'default' => ['tgz']},
-
-	# Enable text search, which will list the commits which match author,
-	# committer or commit text to a given string.  Enabled by default.
-	# Project specific override is not supported.
-	'search' => {
-		'override' => 0,
-		'default' => [1]},
-
-	# Enable grep search, which will list the files in currently selected
-	# tree containing the given string. Enabled by default. This can be
-	# potentially CPU-intensive, of course.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'grep'}{'default'} = [1];
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'grep'}{'override'} = 1;
-	# and in project config gitweb.grep = 0|1;
-	'grep' => {
-		'sub' => sub { feature_bool('grep', @_) },
-		'override' => 0,
-		'default' => [1]},
-
-	# Enable the pickaxe search, which will list the commits that modified
-	# a given string in a file. This can be practical and quite faster
-	# alternative to 'blame', but still potentially CPU-intensive.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'pickaxe'}{'default'} = [1];
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'pickaxe'}{'override'} = 1;
-	# and in project config gitweb.pickaxe = 0|1;
-	'pickaxe' => {
-		'sub' => sub { feature_bool('pickaxe', @_) },
-		'override' => 0,
-		'default' => [1]},
-
-	# Enable showing size of blobs in a 'tree' view, in a separate
-	# column, similar to what 'ls -l' does.  This cost a bit of IO.
-
-	# To disable system wide have in $GITWEB_CONFIG
-	# $feature{'show-sizes'}{'default'} = [0];
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'show-sizes'}{'override'} = 1;
-	# and in project config gitweb.showsizes = 0|1;
-	'show-sizes' => {
-		'sub' => sub { feature_bool('showsizes', @_) },
-		'override' => 0,
-		'default' => [1]},
-
-	# Make gitweb use an alternative format of the URLs which can be
-	# more readable and natural-looking: project name is embedded
-	# directly in the path and the query string contains other
-	# auxiliary information. All gitweb installations recognize
-	# URL in either format; this configures in which formats gitweb
-	# generates links.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'pathinfo'}{'default'} = [1];
-	# Project specific override is not supported.
-
-	# Note that you will need to change the default location of CSS,
-	# favicon, logo and possibly other files to an absolute URL. Also,
-	# if gitweb.cgi serves as your indexfile, you will need to force
-	# $my_uri to contain the script name in your $GITWEB_CONFIG.
-	'pathinfo' => {
-		'override' => 0,
-		'default' => [0]},
-
-	# Make gitweb consider projects in project root subdirectories
-	# to be forks of existing projects. Given project $projname.git,
-	# projects matching $projname/*.git will not be shown in the main
-	# projects list, instead a '+' mark will be added to $projname
-	# there and a 'forks' view will be enabled for the project, listing
-	# all the forks. If project list is taken from a file, forks have
-	# to be listed after the main project.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'forks'}{'default'} = [1];
-	# Project specific override is not supported.
-	'forks' => {
-		'override' => 0,
-		'default' => [0]},
-
-	# Insert custom links to the action bar of all project pages.
-	# This enables you mainly to link to third-party scripts integrating
-	# into gitweb; e.g. git-browser for graphical history representation
-	# or custom web-based repository administration interface.
-
-	# The 'default' value consists of a list of triplets in the form
-	# (label, link, position) where position is the label after which
-	# to insert the link and link is a format string where %n expands
-	# to the project name, %f to the project path within the filesystem,
-	# %h to the current hash (h gitweb parameter) and %b to the current
-	# hash base (hb gitweb parameter); %% expands to %.
-
-	# To enable system wide have in $GITWEB_CONFIG e.g.
-	# $feature{'actions'}{'default'} = [('graphiclog',
-	# 	'/git-browser/by-commit.html?r=%n', 'summary')];
-	# Project specific override is not supported.
-	'actions' => {
-		'override' => 0,
-		'default' => []},
-
-	# Allow gitweb scan project content tags described in ctags/
-	# of project repository, and display the popular Web 2.0-ish
-	# "tag cloud" near the project list. Note that this is something
-	# COMPLETELY different from the normal Git tags.
-
-	# gitweb by itself can show existing tags, but it does not handle
-	# tagging itself; you need an external application for that.
-	# For an example script, check Girocco's cgi/tagproj.cgi.
-	# You may want to install the HTML::TagCloud Perl module to get
-	# a pretty tag cloud instead of just a list of tags.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'ctags'}{'default'} = ['path_to_tag_script'];
-	# Project specific override is not supported.
-	'ctags' => {
-		'override' => 0,
-		'default' => [0]},
-
-	# The maximum number of patches in a patchset generated in patch
-	# view. Set this to 0 or undef to disable patch view, or to a
-	# negative number to remove any limit.
-
-	# To disable system wide have in $GITWEB_CONFIG
-	# $feature{'patches'}{'default'} = [0];
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'patches'}{'override'} = 1;
-	# and in project config gitweb.patches = 0|n;
-	# where n is the maximum number of patches allowed in a patchset.
-	'patches' => {
-		'sub' => \&feature_patches,
-		'override' => 0,
-		'default' => [16]},
-
-	# Avatar support. When this feature is enabled, views such as
-	# shortlog or commit will display an avatar associated with
-	# the email of the committer(s) and/or author(s).
-
-	# Currently available providers are gravatar and picon.
-	# If an unknown provider is specified, the feature is disabled.
-
-	# Gravatar depends on Digest::MD5.
-	# Picon currently relies on the indiana.edu database.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'avatar'}{'default'} = ['<provider>'];
-	# where <provider> is either gravatar or picon.
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'avatar'}{'override'} = 1;
-	# and in project config gitweb.avatar = <provider>;
-	'avatar' => {
-		'sub' => \&feature_avatar,
-		'override' => 0,
-		'default' => ['']},
-
-	# Enable displaying how much time and how many git commands
-	# it took to generate and display page.  Disabled by default.
-	# Project specific override is not supported.
-	'timed' => {
-		'override' => 0,
-		'default' => [0]},
-
-	# Enable turning some links into links to actions which require
-	# JavaScript to run (like 'blame_incremental').  Not enabled by
-	# default.  Project specific override is currently not supported.
-	'javascript-actions' => {
-		'override' => 0,
-		'default' => [0]},
-);
+do 'gitweb_defaults.pl';
 
 sub gitweb_get_feature {
 	my ($name) = @_;
diff --git a/gitweb/gitweb_defaults.perl b/gitweb/gitweb_defaults.perl
new file mode 100644
index 0000000..ede0daf
--- /dev/null
+++ b/gitweb/gitweb_defaults.perl
@@ -0,0 +1,468 @@
+# gitweb - simple web interface to track changes in git repositories
+#
+# (C) 2005-2006, Kay Sievers <kay.sievers@vrfy.org>
+# (C) 2005, Christian Gierke
+#
+# This program is licensed under the GPLv2
+
+# Base URL for relative URLs in gitweb ($logo, $favicon, ...),
+# needed and used only for URLs with nonempty PATH_INFO
+$base_url = $my_url;
+
+# When the script is used as DirectoryIndex, the URL does not contain the name
+# of the script file itself, and $cgi->url() fails to strip PATH_INFO, so we
+# have to do it ourselves. We make $path_info global because it's also used
+# later on.
+#
+# Another issue with the script being the DirectoryIndex is that the resulting
+# $my_url data is not the full script URL: this is good, because we want
+# generated links to keep implying the script name if it wasn't explicitly
+# indicated in the URL we're handling, but it means that $my_url cannot be used
+# as base URL.
+# Therefore, if we needed to strip PATH_INFO, then we know that we have
+# to build the base URL ourselves:
+$path_info = $ENV{"PATH_INFO"};
+if ($path_info) {
+	if ($my_url =~ s,\Q$path_info\E$,, &&
+	    $my_uri =~ s,\Q$path_info\E$,, &&
+	    defined $ENV{'SCRIPT_NAME'}) {
+		$base_url = $cgi->url(-base => 1) . $ENV{'SCRIPT_NAME'};
+	}
+}
+
+# core git executable to use
+# this can just be "git" if your webserver has a sensible PATH
+$GIT = "++GIT_BINDIR++/git";
+
+# absolute fs-path which will be prepended to the project path
+#our $projectroot = "/pub/scm";
+$projectroot = "++GITWEB_PROJECTROOT++";
+
+# fs traversing limit for getting project list
+# the number is relative to the projectroot
+$project_maxdepth = "++GITWEB_PROJECT_MAXDEPTH++";
+
+# target of the home link on top of all pages
+$home_link = $my_uri || "/";
+
+# string of the home link on top of all pages
+$home_link_str = "++GITWEB_HOME_LINK_STR++";
+
+# name of your site or organization to appear in page titles
+# replace this with something more descriptive for clearer bookmarks
+$site_name = "++GITWEB_SITENAME++"
+                 || ($ENV{'SERVER_NAME'} || "Untitled") . " Git";
+
+# filename of html text to include at top of each page
+$site_header = "++GITWEB_SITE_HEADER++";
+# html text to include at home page
+$home_text = "++GITWEB_HOMETEXT++";
+# filename of html text to include at bottom of each page
+$site_footer = "++GITWEB_SITE_FOOTER++";
+
+# URI of stylesheets
+@stylesheets = ("++GITWEB_CSS++");
+# URI of a single stylesheet, which can be overridden in GITWEB_CONFIG.
+$stylesheet = undef;
+# URI of GIT logo (72x27 size)
+$logo = "++GITWEB_LOGO++";
+# URI of GIT favicon, assumed to be image/png type
+$favicon = "++GITWEB_FAVICON++";
+# URI of gitweb.js (JavaScript code for gitweb)
+$javascript = "++GITWEB_JS++";
+
+# URI and label (title) of GIT logo link
+#our $logo_url = "http://www.kernel.org/pub/software/scm/git/docs/";
+#our $logo_label = "git documentation";
+$logo_url = "http://git-scm.com/";
+$logo_label = "git homepage";
+
+# source of projects list
+$projects_list = "++GITWEB_LIST++";
+
+# the width (in characters) of the projects list "Description" column
+$projects_list_description_width = 25;
+
+# default order of projects list
+# valid values are none, project, descr, owner, and age
+$default_projects_order = "project";
+
+# show repository only if this file exists
+# (only effective if this variable evaluates to true)
+$export_ok = "++GITWEB_EXPORT_OK++";
+
+# show repository only if this subroutine returns true
+# when given the path to the project, for example:
+#    sub { return -e "$_[0]/git-daemon-export-ok"; }
+$export_auth_hook = undef;
+
+# only allow viewing of repositories also shown on the overview page
+$strict_export = "++GITWEB_STRICT_EXPORT++";
+
+# list of git base URLs used for URL to where fetch project from,
+# i.e. full URL is "$git_base_url/$project"
+@git_base_url_list = grep { $_ ne '' } ("++GITWEB_BASE_URL++");
+
+# default blob_plain mimetype and default charset for text/plain blob
+$default_blob_plain_mimetype = 'text/plain';
+$default_text_plain_charset  = undef;
+
+# file to use for guessing MIME types before trying /etc/mime.types
+# (relative to the current git repository)
+$mimetypes_file = undef;
+
+# assume this charset if line contains non-UTF-8 characters;
+# it should be valid encoding (see Encoding::Supported(3pm) for list),
+# for which encoding all byte sequences are valid, for example
+# 'iso-8859-1' aka 'latin1' (it is decoded without checking, so it
+# could be even 'utf-8' for the old behavior)
+$fallback_encoding = 'latin1';
+
+# rename detection options for git-diff and git-diff-tree
+# - default is '-M', with the cost proportional to
+#   (number of removed files) * (number of new files).
+# - more costly is '-C' (which implies '-M'), with the cost proportional to
+#   (number of changed files + number of removed files) * (number of new files)
+# - even more costly is '-C', '--find-copies-harder' with cost
+#   (number of files in the original tree) * (number of new files)
+# - one might want to include '-B' option, e.g. '-B', '-M'
+@diff_opts = ('-M'); # taken from git_commit
+
+# Disables features that would allow repository owners to inject script into
+# the gitweb domain.
+$prevent_xss = 0;
+
+# information about snapshot formats that gitweb is capable of serving
+%known_snapshot_formats = (
+	# name => {
+	# 	'display' => display name,
+	# 	'type' => mime type,
+	# 	'suffix' => filename suffix,
+	# 	'format' => --format for git-archive,
+	# 	'compressor' => [compressor command and arguments]
+	# 	                (array reference, optional)
+	# 	'disabled' => boolean (optional)}
+	#
+	'tgz' => {
+		'display' => 'tar.gz',
+		'type' => 'application/x-gzip',
+		'suffix' => '.tar.gz',
+		'format' => 'tar',
+		'compressor' => ['gzip']},
+
+	'tbz2' => {
+		'display' => 'tar.bz2',
+		'type' => 'application/x-bzip2',
+		'suffix' => '.tar.bz2',
+		'format' => 'tar',
+		'compressor' => ['bzip2']},
+
+	'txz' => {
+		'display' => 'tar.xz',
+		'type' => 'application/x-xz',
+		'suffix' => '.tar.xz',
+		'format' => 'tar',
+		'compressor' => ['xz'],
+		'disabled' => 1},
+
+	'zip' => {
+		'display' => 'zip',
+		'type' => 'application/x-zip',
+		'suffix' => '.zip',
+		'format' => 'zip'},
+);
+
+# Aliases so we understand old gitweb.snapshot values in repository
+# configuration.
+%known_snapshot_format_aliases = (
+	'gzip'  => 'tgz',
+	'bzip2' => 'tbz2',
+	'xz'    => 'txz',
+
+	# backward compatibility: legacy gitweb config support
+	'x-gzip' => undef, 'gz' => undef,
+	'x-bzip2' => undef, 'bz2' => undef,
+	'x-zip' => undef, '' => undef,
+);
+
+# Pixel sizes for icons and avatars. If the default font sizes or lineheights
+# are changed, it may be appropriate to change these values too via
+# $GITWEB_CONFIG.
+%avatar_size = (
+	'default' => 16,
+	'double'  => 32
+);
+
+# This is here to allow for missmatch git & gitweb versions
+$missmatch_git = '';
+
+#This is here to deal with an extra link on the summary pages - if it's left blank
+# this link will not be shwon.  If it's set, this will be prepended to the repo and used
+$gitlinkurl = '';
+
+# Used to set the maximum load that we will still respond to gitweb queries.
+# if we exceed this than we do the processing to figure out if there's a mirror
+# and redirect to it, or to just return 503 server busy
+$maxload = 300;
+
+# This enables/disables the caching layer in gitweb.  This currently only supports the
+# 'dumb' file based caching layer, primarily used on git.kernel.org.  this is reasonably
+# effective but it has the downside of requiring a huge amount of disk space if there
+# are a number of repositories involved.  It is not uncommon for git.kernel.org to have
+# on the order of 80G - 120G accumulate over the course of a few months.  It is recommended
+# that the cache directory be periodically completely deleted, and this is safe to perform.
+# Suggested mechanism
+# mv $cacheidr $cachedir.flush;mkdir $cachedir;rm -rf $cachedir.flush
+# Value is binary. 0 = disabled (default), 1 = enabled.
+$cache_enable = 0;
+
+# Used to set the minimum cache timeout for the dynamic caching algorithm.  Basically
+# if we calculate the cache to be under this number of seconds we set the cache timeout 
+# to this minimum.
+# Value is in seconds.  1 = 1 seconds, 60 = 1 minute, 600 = 10 minutes, 3600 = 1 hour
+$minCacheTime = 20;
+
+# Used to set the maximum cache timeout for the dynamic caching algorithm.  Basically
+# if we calculate the cache to exceed this number of seconds we set the cache timeout 
+# to this maximum.
+# Value is in seconds.  1 = 1 seconds, 60 = 1 minute, 600 = 10 minutes, 3600 = 1 hour
+$maxCacheTime = 1200;
+
+# If you need to change the location of the caching directory, override this
+# otherwise this will probably do fine for you
+$cachedir = 'cache';
+
+# If this is set (to 1) cache will do it's best to always display something instead
+# of making someone wait for the cache to update.  This will launch the cacheUpdate
+# into the background and it will lock a <file>.bg file and will only lock the 
+# actual cache file when it needs to write into it.  In theory this will make 
+# gitweb seem more responsive at the price of possibly stale data.
+$backgroundCache = 1;
+
+# Used to set the maximum cache file life.  If a cache files last modify time exceeds
+# this value, it will assume that the data is just too old, and HAS to be regenerated
+# instead of trying to display the existing cache data.
+# Value is in seconds.  1 = 1 seconds, 60 = 1 minute, 600 = 10 minutes, 3600 = 1 hour
+# 18000 = 5 hours
+$maxCacheLife = 18000;
+
+# You define site-wide feature defaults here; override them with
+# $GITWEB_CONFIG as necessary.
+%feature = (
+	# feature => {
+	# 	'sub' => feature-sub (subroutine),
+	# 	'override' => allow-override (boolean),
+	# 	'default' => [ default options...] (array reference)}
+	#
+	# if feature is overridable (it means that allow-override has true value),
+	# then feature-sub will be called with default options as parameters;
+	# return value of feature-sub indicates if to enable specified feature
+	#
+	# if there is no 'sub' key (no feature-sub), then feature cannot be
+	# overriden
+	#
+	# use gitweb_get_feature(<feature>) to retrieve the <feature> value
+	# (an array) or gitweb_check_feature(<feature>) to check if <feature>
+	# is enabled
+
+	# Enable the 'blame' blob view, showing the last commit that modified
+	# each line in the file. This can be very CPU-intensive.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'blame'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'blame'}{'override'} = 1;
+	# and in project config gitweb.blame = 0|1;
+	'blame' => {
+		'sub' => sub { feature_bool('blame', @_) },
+		'override' => 0,
+		'default' => [0]},
+
+	# Enable the 'snapshot' link, providing a compressed archive of any
+	# tree. This can potentially generate high traffic if you have large
+	# project.
+
+	# Value is a list of formats defined in %known_snapshot_formats that
+	# you wish to offer.
+	# To disable system wide have in $GITWEB_CONFIG
+	# $feature{'snapshot'}{'default'} = [];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'snapshot'}{'override'} = 1;
+	# and in project config, a comma-separated list of formats or "none"
+	# to disable.  Example: gitweb.snapshot = tbz2,zip;
+	'snapshot' => {
+		'sub' => \&feature_snapshot,
+		'override' => 0,
+		'default' => ['tgz']},
+
+	# Enable text search, which will list the commits which match author,
+	# committer or commit text to a given string.  Enabled by default.
+	# Project specific override is not supported.
+	'search' => {
+		'override' => 0,
+		'default' => [1]},
+
+	# Enable grep search, which will list the files in currently selected
+	# tree containing the given string. Enabled by default. This can be
+	# potentially CPU-intensive, of course.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'grep'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'grep'}{'override'} = 1;
+	# and in project config gitweb.grep = 0|1;
+	'grep' => {
+		'sub' => sub { feature_bool('grep', @_) },
+		'override' => 0,
+		'default' => [1]},
+
+	# Enable the pickaxe search, which will list the commits that modified
+	# a given string in a file. This can be practical and quite faster
+	# alternative to 'blame', but still potentially CPU-intensive.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'pickaxe'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'pickaxe'}{'override'} = 1;
+	# and in project config gitweb.pickaxe = 0|1;
+	'pickaxe' => {
+		'sub' => sub { feature_bool('pickaxe', @_) },
+		'override' => 0,
+		'default' => [1]},
+
+	# Enable showing size of blobs in a 'tree' view, in a separate
+	# column, similar to what 'ls -l' does.  This cost a bit of IO.
+
+	# To disable system wide have in $GITWEB_CONFIG
+	# $feature{'show-sizes'}{'default'} = [0];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'show-sizes'}{'override'} = 1;
+	# and in project config gitweb.showsizes = 0|1;
+	'show-sizes' => {
+		'sub' => sub { feature_bool('showsizes', @_) },
+		'override' => 0,
+		'default' => [1]},
+
+	# Make gitweb use an alternative format of the URLs which can be
+	# more readable and natural-looking: project name is embedded
+	# directly in the path and the query string contains other
+	# auxiliary information. All gitweb installations recognize
+	# URL in either format; this configures in which formats gitweb
+	# generates links.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'pathinfo'}{'default'} = [1];
+	# Project specific override is not supported.
+
+	# Note that you will need to change the default location of CSS,
+	# favicon, logo and possibly other files to an absolute URL. Also,
+	# if gitweb.cgi serves as your indexfile, you will need to force
+	# $my_uri to contain the script name in your $GITWEB_CONFIG.
+	'pathinfo' => {
+		'override' => 0,
+		'default' => [0]},
+
+	# Make gitweb consider projects in project root subdirectories
+	# to be forks of existing projects. Given project $projname.git,
+	# projects matching $projname/*.git will not be shown in the main
+	# projects list, instead a '+' mark will be added to $projname
+	# there and a 'forks' view will be enabled for the project, listing
+	# all the forks. If project list is taken from a file, forks have
+	# to be listed after the main project.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'forks'}{'default'} = [1];
+	# Project specific override is not supported.
+	'forks' => {
+		'override' => 0,
+		'default' => [0]},
+
+	# Insert custom links to the action bar of all project pages.
+	# This enables you mainly to link to third-party scripts integrating
+	# into gitweb; e.g. git-browser for graphical history representation
+	# or custom web-based repository administration interface.
+
+	# The 'default' value consists of a list of triplets in the form
+	# (label, link, position) where position is the label after which
+	# to insert the link and link is a format string where %n expands
+	# to the project name, %f to the project path within the filesystem,
+	# %h to the current hash (h gitweb parameter) and %b to the current
+	# hash base (hb gitweb parameter); %% expands to %.
+
+	# To enable system wide have in $GITWEB_CONFIG e.g.
+	# $feature{'actions'}{'default'} = [('graphiclog',
+	# 	'/git-browser/by-commit.html?r=%n', 'summary')];
+	# Project specific override is not supported.
+	'actions' => {
+		'override' => 0,
+		'default' => []},
+
+	# Allow gitweb scan project content tags described in ctags/
+	# of project repository, and display the popular Web 2.0-ish
+	# "tag cloud" near the project list. Note that this is something
+	# COMPLETELY different from the normal Git tags.
+
+	# gitweb by itself can show existing tags, but it does not handle
+	# tagging itself; you need an external application for that.
+	# For an example script, check Girocco's cgi/tagproj.cgi.
+	# You may want to install the HTML::TagCloud Perl module to get
+	# a pretty tag cloud instead of just a list of tags.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'ctags'}{'default'} = ['path_to_tag_script'];
+	# Project specific override is not supported.
+	'ctags' => {
+		'override' => 0,
+		'default' => [0]},
+
+	# The maximum number of patches in a patchset generated in patch
+	# view. Set this to 0 or undef to disable patch view, or to a
+	# negative number to remove any limit.
+
+	# To disable system wide have in $GITWEB_CONFIG
+	# $feature{'patches'}{'default'} = [0];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'patches'}{'override'} = 1;
+	# and in project config gitweb.patches = 0|n;
+	# where n is the maximum number of patches allowed in a patchset.
+	'patches' => {
+		'sub' => \&feature_patches,
+		'override' => 0,
+		'default' => [16]},
+
+	# Avatar support. When this feature is enabled, views such as
+	# shortlog or commit will display an avatar associated with
+	# the email of the committer(s) and/or author(s).
+
+	# Currently available providers are gravatar and picon.
+	# If an unknown provider is specified, the feature is disabled.
+
+	# Gravatar depends on Digest::MD5.
+	# Picon currently relies on the indiana.edu database.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'avatar'}{'default'} = ['<provider>'];
+	# where <provider> is either gravatar or picon.
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'avatar'}{'override'} = 1;
+	# and in project config gitweb.avatar = <provider>;
+	'avatar' => {
+		'sub' => \&feature_avatar,
+		'override' => 0,
+		'default' => ['']},
+
+	# Enable displaying how much time and how many git commands
+	# it took to generate and display page.  Disabled by default.
+	# Project specific override is not supported.
+	'timed' => {
+		'override' => 0,
+		'default' => [0]},
+
+	# Enable turning some links into links to actions which require
+	# JavaScript to run (like 'blame_incremental').  Not enabled by
+	# default.  Project specific override is currently not supported.
+	'javascript-actions' => {
+		'override' => 0,
+		'default' => [0]},
+);
+1;

Re: [PATCH 0/6] Gitweb caching changes v2

[Sverre Rabbelier]

Heya,

On Fri, Dec 11, 2009 at 00:45, John 'Warthog9' Hawley
<warthog9@kernel.org> wrote:
> John 'Warthog9' Hawley (6):
>  GITWEB - Load Checking
>  GITWEB - Missmatching git w/ gitweb
>  GITWEB - Add git:// link to summary pages
>  GITWEB - Makefile changes
>  GITWEB - File based caching layer
>  GITWEB - Separate defaults from main file

I'd prefer not to be shouted at, how about s/GITWEB/gitweb: /g ? :)

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 1/6] GITWEB - Load Checking

[Sverre Rabbelier]

Heya,

On Fri, Dec 11, 2009 at 00:45, John 'Warthog9' Hawley
<warthog9@kernel.org> wrote:
>  gitweb/gitweb.perl |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
>
>

Also, what happened to including patches inline for ease of review?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 1/6] GITWEB - Load Checking

[Jakub Narebski]

"John 'Warthog9' Hawley" <warthog9@kernel.org> writes:

> This changes the behavior, slightly, of gitweb so that it verifies
> that the box isn't inundated with before attempting to serve gitweb.
> If the box is overloaded, it basically returns a 503 server unavailable
> until the load falls below the defined threshold.  This helps dramatically
> if you have a box that's I/O bound, reaches a certain load and you
> don't want gitweb, the I/O hog that it is, increasing the pain the
> server is already undergoing.
> 
> adds $maxload configuration variable.  Default is a load of 300,
> which for most cases should never be hit.

Your patch doesn't allow for *turning off* this feature.  Reasonable
solution would be to use 'undef' or negative number to turn off this
check (this feature).

> 
> Please note this makes the assumption that /proc/loadavg exists
> as there is no good way to read load averages on a great number of
> platforms [READ: Windows], or that it's reasonably accurate.

What about MacOS X, or FreeBSD, or OpenSolaris?

You should mention that it is intended that if gitweb cannot read load
average (for example /proc/loadavg does not exist), then the feature
is turned off, i.e. the check always succeeds.  Which is reasonable.

> 
> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>

Why signoff is different from author (warthog9@kernel.org)?  Why this
email for signoff?  Just curious...

> ---
>  gitweb/gitweb.perl |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-) 

Please post patches inline, not as attachement.

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7e477af..813e48f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -221,6 +221,11 @@ our %avatar_size = (
>  	'double'  => 32
>  );
>  
> +# Used to set the maximum load that we will still respond to gitweb queries.
> +# if we exceed this than we do the processing to figure out if there's a mirror
> +# and redirect to it, or to just return 503 server busy

I'd probably say:

+# Used to set the maximum load that we will still respond to gitweb queries.
+# If server load exceed this value then return "503 server busy" error,
+# (it is also possible to redirect to mirror, if it exists, instead).

> +our $maxload = 300;
> +
>  # You define site-wide feature defaults here; override them with
>  # $GITWEB_CONFIG as necessary.
>  our %feature = (
> @@ -551,6 +556,25 @@ if (-e $GITWEB_CONFIG) {
>  	do $GITWEB_CONFIG_SYSTEM if -e $GITWEB_CONFIG_SYSTEM;
>  }
>  
> +# loadavg throttle
> +sub get_loadavg() {
> +    my $load;
> +    my @loads;
> +
> +    open($load, '<', '/proc/loadavg') or return 0;

Why not use one of existing CPAN modules: Sys::Info::Device::CPU,
BSD::getloadavg, Sys::CpuLoad?

Style:

+    open (my $load, '<', '/proc/loadavg') or return 0;

and of course no "my $load" at beginning.  Also perhaps $fh, or
$loadfh instead of $load?  But this is a minor nit.

> +    @loads = split(/\s+/, scalar <$load>);
> +    close($load);
> +    return $loads[0];
> +}
> +
> +if (get_loadavg() > $maxload) {
> +    print "Content-Type: text/plain\n";
> +    print "Status: 503 Excessive load on server\n";
> +    print "\n";
> +    print "The load average on the server is too high\n";
> +    exit 0;

Why not use die_error subroutine?  Is it to have generate absolutely
minimal load, and that is why you do not use die_error(), or even
$cgi->header()?

Wouldn't a better solution be to use here-doc syntax?

+    print <<'EOF';
+Content-Type: text/plain; charset=utf-8
+Status: 503 Excessive load on server
+
+The load average on the server is too high
+EOF
+    exit 0;


> +}
> +
>  # version of the core git binary
>  our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
>  $number_of_git_cmds++;

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 1/6] GITWEB - Load Checking

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

>> +# loadavg throttle
>> +sub get_loadavg() {
>> +    my $load;
>> +    my @loads;
>> +
>> +    open($load, '<', '/proc/loadavg') or return 0;
>
> Why not use one of existing CPAN modules: Sys::Info::Device::CPU,
> BSD::getloadavg, Sys::CpuLoad?

I would prefer to hear something along the lines of...

    I like this.  Here is a follow-up patch you can squash in to
    support other platforms.

I gave the patches a cursory look (I somehow didn't see 5/6, though) and
they all looked decently done, except that some of the lines were
excessively long.

Re: [PATCH 1/6] GITWEB - Load Checking

[J.H.]

<snip>
>> adds $maxload configuration variable.  Default is a load of 300,
>> which for most cases should never be hit.
> 
> Your patch doesn't allow for *turning off* this feature.  Reasonable
> solution would be to use 'undef' or negative number to turn off this
> check (this feature).

Well there's the opposite argument that setting the number arbitrarily 
high, 4096 for instance would also in essence negate this (though I'll 
admit I've reached and exceeded those numbers before)

That said I agree, being able to turn this off needs to be added and 
will be shortly.

>> Please note this makes the assumption that /proc/loadavg exists
>> as there is no good way to read load averages on a great number of
>> platforms [READ: Windows], or that it's reasonably accurate.
> 
> What about MacOS X, or FreeBSD, or OpenSolaris?

Will comment on this further down

> You should mention that it is intended that if gitweb cannot read load
> average (for example /proc/loadavg does not exist), then the feature
> is turned off, i.e. the check always succeeds.  Which is reasonable.

That's fine.

> 
>> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> 
> Why signoff is different from author (warthog9@kernel.org)?  Why this
> email for signoff?  Just curious...

My bad, did the patches up on my laptop but had to send them out from 
kernel.org, thus the miss-match: I.E. user error.

<snip>

>> +# loadavg throttle
>> +sub get_loadavg() {
>> +    my $load;
>> +    my @loads;
>> +
>> +    open($load, '<', '/proc/loadavg') or return 0;
> 
> Why not use one of existing CPAN modules: Sys::Info::Device::CPU,
> BSD::getloadavg, Sys::CpuLoad?

Here's the fundamental problem:

Sys:Info:Device:CPU
	Windows:
		Using this method under Windows is not recommended
		since, the WMI interface will possibly take at least 2
		seconds to complete the request.

BSD::getloadavg
	While this more or less supports anything with a libc getloadavg
	(and thus might be the best one I've seen, I'll admit I didn't
	notice this one when I looked years ago) getting it to work on
	windows looks, exciting.

Sys::CpuLoad:
	http://cpansearch.perl.org/src/CLINTDW/Sys-CpuLoad-0.03/README
	Specifically:
		- Currently FreeBSD and OpenBSD are supported.
		- Wanted: HPUX 11.11 ...
		- Todo: Win32 support

	So this doesn't really buy me anything but, maybe, BSD support.
	
So at the end of the day, none of those really gets me a "useful" cross 
platform load checker (though like I said BSD::getloadavg looks to be 
the best of the ones you mentioned) and more or less Windows is going to 
lose this as a usable feature no matter what.

I think I'd almost rather set this up so that if it can't get something 
useful (I.E. /proc/loadavg is missing) it just skips past it as if the 
load was 0.

I might try out the BSD::getloadavg but I want to take a look and see if 
that's easily installed or not, if it's not it might be difficult to 
justify that as a dependency.

<snip>

>> +if (get_loadavg() > $maxload) {
>> +    print "Content-Type: text/plain\n";
>> +    print "Status: 503 Excessive load on server\n";
>> +    print "\n";
>> +    print "The load average on the server is too high\n";
>> +    exit 0;
> 
> Why not use die_error subroutine?  Is it to have generate absolutely
> minimal load, and that is why you do not use die_error(), or even
> $cgi->header()?
> 
> Wouldn't a better solution be to use here-doc syntax?
> 
> +    print <<'EOF';
> +Content-Type: text/plain; charset=utf-8
> +Status: 503 Excessive load on server
> +
> +The load average on the server is too high
> +EOF
> +    exit 0;

It was intended to be the most minimal possible, mainly get in, get out. 
  Also not sure the die_error existed in gitweb when this was originally 
written.  Probably worth switching to it now since it's there either 
way, and I don't think using it would add enough overhead to matter.

- John 'Warthog9' Hawley

Re: [PATCH 1/6] GITWEB - Load Checking

[Junio C Hamano]

"J.H." <warthog9@kernel.org> writes:

> It was intended to be the most minimal possible, mainly get in, get
> out. Also not sure the die_error existed in gitweb when this was
> originally written.  Probably worth switching to it now since it's
> there either way, and I don't think using it would add enough overhead
> to matter.

Thanks; all sounded a reasonable response to the review.  Are you
re-rolling the series anytime soon (I am asking because then I'd rather
not to queue this round especially because I didn't see 5/6).

Re: [PATCH 1/6] GITWEB - Load Checking

[J.H.]

Junio C Hamano wrote:
> "J.H." <warthog9@kernel.org> writes:
> 
>> It was intended to be the most minimal possible, mainly get in, get
>> out. Also not sure the die_error existed in gitweb when this was
>> originally written.  Probably worth switching to it now since it's
>> there either way, and I don't think using it would add enough overhead
>> to matter.
> 
> Thanks; all sounded a reasonable response to the review.  Are you
> re-rolling the series anytime soon (I am asking because then I'd rather
> not to queue this round especially because I didn't see 5/6).

I'll probably have some changes up and about tomorrow, and it's a little 
troubling that 5/6 didn't come through for you

6 at least made it to marc.info: 
http://marc.info/?l=git&m=126048884825985&w=2

and 5 seems to have been eaten by a grue somewhere.  It was a *big* 
patch mainly because all the caching flips over in a single go.  If you 
want I can privately bounce 5 & 6 to you so you have a complete tree 
right now?

- John 'Warthog9' Hawley

Re: [PATCH 1/6] GITWEB - Load Checking

[J.H.]

J.H. wrote:
> Junio C Hamano wrote:
>> "J.H." <warthog9@kernel.org> writes:
>>
>>> It was intended to be the most minimal possible, mainly get in, get
>>> out. Also not sure the die_error existed in gitweb when this was
>>> originally written.  Probably worth switching to it now since it's
>>> there either way, and I don't think using it would add enough overhead
>>> to matter.
>>
>> Thanks; all sounded a reasonable response to the review.  Are you
>> re-rolling the series anytime soon (I am asking because then I'd rather
>> not to queue this round especially because I didn't see 5/6).
> 
> I'll probably have some changes up and about tomorrow, and it's a little 
> troubling that 5/6 didn't come through for you
> 
> 6 at least made it to marc.info: 
> http://marc.info/?l=git&m=126048884825985&w=2
> 
> and 5 seems to have been eaten by a grue somewhere.  It was a *big* 
> patch mainly because all the caching flips over in a single go.  If you 
> want I can privately bounce 5 & 6 to you so you have a complete tree 
> right now?

Not to reply to myself but this might also be helpful:

http://git.kernel.org/?p=git/warthog9/gitweb.git;a=shortlog;h=refs/heads/gitweb-ml-v2

- John 'Warthog9' Hawley

Re: [PATCH 1/6] GITWEB - Load Checking

[Junio C Hamano]

"J.H." <warthog9@kernel.org> writes:

>> Thanks; all sounded a reasonable response to the review.  Are you
>> re-rolling the series anytime soon (I am asking because then I'd rather
>> not to queue this round especially because I didn't see 5/6).
>
> I'll probably have some changes up and about tomorrow, and it's a
> little troubling that 5/6 didn't come through for you
>
> 6 at least made it to marc.info:
> http://marc.info/?l=git&m=126048884825985&w=2

Sorry; I meant to say "[PATCH 5/6]", not "5 and 6 didn't come".

> and 5 seems to have been eaten by a grue somewhere.  It was a *big*
> patch mainly because all the caching flips over in a single go.  If
> you want I can privately bounce 5 & 6 to you so you have a complete
> tree right now?

Thanks, but not interested, in the sense that it wouldn't make much sense
to me to have a version tonight that is known to go stale within a few
days.  I only pick up and queue patches to 'pu' to save me from later
trouble of finding them from the mailing list backlog, and not to actively
review and engage in the discussion to polish them right now.  We are
already deep in pre-release freeze and my attention is not currently on
anything that won't go in to the upcoming release.

I want to have a solid 1.6.6 before the holidays as a present to all ;-).

Re: [PATCH 1/6] GITWEB - Load Checking

[Jakub Narebski]

On Fri, 11 Dec 2009, J.H. wrote:
> <snip>
>>> adds $maxload configuration variable.  Default is a load of 300,
>>> which for most cases should never be hit.
>> 
>> Your patch doesn't allow for *turning off* this feature.  Reasonable
>> solution would be to use 'undef' or negative number to turn off this
>> check (this feature).
> 
> Well there's the opposite argument that setting the number arbitrarily 
> high, 4096 for instance would also in essence negate this (though I'll 
> admit I've reached and exceeded those numbers before)
> 
> That said I agree, being able to turn this off needs to be added and 
> will be shortly.

Simplest solution would be to used 'undef' (undefined value) for 
"turned off", i.e.:

  if (defined $maxload && get_loadavg() > $maxload) {

>>> Please note this makes the assumption that /proc/loadavg exists
>>> as there is no good way to read load averages on a great number of
>>> platforms [READ: Windows], or that it's reasonably accurate.
>> 
>> What about MacOS X, or FreeBSD, or OpenSolaris?
> 
> Will comment on this further down

I think it would be better to write in commit message that because finding
load average is OS dependent, there is provided (sample) solution which
uses /proc/loadavg and works (at least) on Linux.  And that for platforms
which do not have /proc/loadavg the feature is simply turned off (by the
way of using load=0 if load cannot be determined).
 
>> You should mention that it is intended that if gitweb cannot read load
>> average (for example /proc/loadavg does not exist), then the feature
>> is turned off, i.e. the check always succeeds.  Which is reasonable.
> 
> That's fine.

See above proposal.  This information should be present in commit message,
and perhaps maybe even as one-line comment above opening /proc/loadavg.

>>> +# loadavg throttle
>>> +sub get_loadavg() {
>>> +    my $load;
>>> +    my @loads;
>>> +
>>> +    open($load, '<', '/proc/loadavg') or return 0;
>> 
>> Why not use one of existing CPAN modules: Sys::Info::Device::CPU,
>> BSD::getloadavg, Sys::CpuLoad?
> 
> Here's the fundamental problem:
> 
> Sys:Info:Device:CPU
> 	Windows:
> 		Using this method under Windows is not recommended
> 		since, the WMI interface will possibly take at least 2
> 		seconds to complete the request.
> 
> BSD::getloadavg
> 	While this more or less supports anything with a libc getloadavg
> 	(and thus might be the best one I've seen, I'll admit I didn't
> 	notice this one when I looked years ago) getting it to work on
> 	windows looks, exciting.
> 
> Sys::CpuLoad:
> 	http://cpansearch.perl.org/src/CLINTDW/Sys-CpuLoad-0.03/README
> 	Specifically:
> 		- Currently FreeBSD and OpenBSD are supported.
> 		- Wanted: HPUX 11.11 ...
> 		- Todo: Win32 support
> 
> 	So this doesn't really buy me anything but, maybe, BSD support.
> 	
> So at the end of the day, none of those really gets me a "useful" cross 
> platform load checker (though like I said BSD::getloadavg looks to be 
> the best of the ones you mentioned) and more or less Windows is going to 
> lose this as a usable feature no matter what.
> 
> I think I'd almost rather set this up so that if it can't get something 
> useful (I.E. /proc/loadavg is missing) it just skips past it as if the 
> load was 0.
> 
> I might try out the BSD::getloadavg but I want to take a look and see if 
> that's easily installed or not, if it's not it might be difficult to 
> justify that as a dependency.

After thinking about this a bit, now I don't think that it is terribly
important.  You *might* describe alternate approaches (roads not taken)
in commit message, but requiring /proc/loadavg for the feature to work
is fine for first patch (it makes patch simpler).

>>> +if (get_loadavg()> $maxload) {
>>> +    print "Content-Type: text/plain\n";
>>> +    print "Status: 503 Excessive load on server\n";
>>> +    print "\n";
>>> +    print "The load average on the server is too high\n";
>>> +    exit 0;
>> 
>> Why not use die_error subroutine?  Is it to have generate absolutely
>> minimal load, and that is why you do not use die_error(), or even
>> $cgi->header()?
>> 
>> Wouldn't a better solution be to use here-doc syntax?
>> 
>> +    print <<'EOF';
>> +Content-Type: text/plain; charset=utf-8
>> +Status: 503 Excessive load on server
>> +
>> +The load average on the server is too high
>> +EOF
>> +    exit 0;
> 
> It was intended to be the most minimal possible, mainly get in, get out. 
>
>   Also not sure the die_error existed in gitweb when this was originally 
> written.  Probably worth switching to it now since it's there either 
> way, and I don't think using it would add enough overhead to matter.

Well, if you are not worring excessively about overhead, then I think
using die_error would be the best solution, as it would preserve look
of gitweb.  It would require extending die_error by 503 response, or
rather %http_responses hash and comment above die_error.

Also I think that Status: should be before Content-Type: header (but
probably it is not required by the standard).

-- 
Jakub Narebski
Poland

Re: [PATCH 2/6] GITWEB - Missmatching git w/ gitweb

[Jakub Narebski]

"John 'Warthog9' Hawley" <warthog9@kernel.org> writes:

> This adds $missmatch_git so that gitweb can run with a miss-matched
> git install.  Gitweb, generally, runs fine on a very broad range of
> git versions, but it's not always practicle or useful to upgrade it
> every time you upgrade git.
> 
> This allows the administrator to realize they are miss-matched, and
> should they be so inclined, disable the check entirely and run in
> a miss-matched fasion.
> 
> This is more here to give an obvious warning as to whats going on
> vs. silently failing.

First, why one would want to require that gitweb version (version at
the time of build) and runtime git version (version of git used to run
commands) match?

Second, it is mismatch, not missmatch (one 's', not double 's').

Third, in my opinion it would be better to name variable in question
e.g. $versions_must_match and also flip its meaning (true means check
that versions match, and show an error otherwise).

>
> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>

signoff mismatch

> ---
>  gitweb/gitweb.perl |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 813e48f..d84f4c0 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -221,6 +221,9 @@ our %avatar_size = (
>  	'double'  => 32
>  );
>  
> +# This is here to allow for missmatch git & gitweb versions
> +our $missmatch_git = '';
> +

First, 'undef' is false, so it could have been written as

+our $missmatch_git;

Or if you prefer explicit false-ish value as default, 0 would be I
think better than empty string '':

+our $missmatch_git = 0;


Second, there is question whether default should be to allow
mismatched versions (current behaviour, more lenient...) or deny (or
warn about) mismatched version, i.e. should it be $versions_must_match
false by default, or $allow_versions_mismatch false by default.

>  # Used to set the maximum load that we will still respond to gitweb queries.
>  # if we exceed this than we do the processing to figure out if there's a mirror
>  # and redirect to it, or to just return 503 server busy
> @@ -579,6 +582,25 @@ if (get_loadavg() > $maxload) {
>  our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
>  $number_of_git_cmds++;
>  
> +# There's a pretty serious flaw that we silently fail if git doesn't find something it needs
> +# a quick and simple check is to have gitweb do a simple check - are we running on the same
> +# version of git that we shipped with - if not, throw up an error so that people doing
> +# first installs don't have to debug perl to figure out whats going on

Could you please clean up language in above comment?  It is very
convoluted.  Please also limit line width of above comment to 76 / 80
columns.

> +if (
> +	$git_version ne $version
> +	&&
> +	$missmatch_git eq ''
> +){

Style

+if (!$allow_versions_mismatch &&
+    $git_version ne $version) {

Do not compare $missmatch_git / $allow_versions_mismatch against '':
it is a boolean value!

> +	git_header_html();

Shouldn't this be "500 Internal Server Error" or something (using the
optional parameter to git_header_html())?

> +	print "<p><b>*** Warning ***</b></p>\n";
> +	print "<p>\n";
> +	print "This version of gitweb was compiled for <b>$version</b> however git version <b>$git_version</b> was found<br/>\n";
> +	print "If you are sure this version of git works with this version of gitweb - please define <b>\$missmatch_git</b> to a non empty string in your git config file.\n";

Too long lines.  Here-doc could be better here.

> +	print "</p>\n";
> +	git_footer_html();
> +	exit;
> +}
> +
>  $projects_list ||= $projectroot;
>  
>  # ======================================================================

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 2/6] GITWEB - Missmatching git w/ gitweb

[Johannes Schindelin]

Hi,

On Thu, 10 Dec 2009, John 'Warthog9' Hawley wrote:

> This adds $missmatch_git so that gitweb can run with a miss-matched
> git install.

I'm not a native English speaker and all, but I thought it was spelt 
'mismatch', i.e. with only one 's'.  Maybe even name it 
'allow_different_git_version' or 'no_strict_git_version'.

A few comments on the patch: the style of the if() statement disagrees 
with the other ones; please use the same style.

Also, as with 1/6, turning off the feature might be better done by setting 
it to undef.

Finally, would it not be nicer if the warning really was only a warning, 
i.e. that the script would try to continue after giving the users a pretty 
warning header?

Ciao,
Dscho

Re: [PATCH 3/6] GITWEB - Add git:// link to summary pages

[Johannes Schindelin]

Hi,

On Thu, 10 Dec 2009, John 'Warthog9' Hawley wrote:

> This adds a git:// link to the summary pages should a common $gitlinkurl 
> be defined (default is nothing defined, thus nothing shown)

Nice.

I forgot to mention in my comments to 2/6 that you seem to wrap after more 
than 80 characters.  However, I have no idea what the suggested line width 
is for gitweb.

Again, this could be done by having the variable defined as undef.

Maybe it would be even nicer if the administrator could specify the 
protocol, e.g. when they do not want/cannot allow git:// but only http:// 
access to the repositories?

Ciao,
Dscho

Re: [PATCH 3/6] GITWEB - Add git:// link to summary pages

[Jakub Narebski]

"John 'Warthog9' Hawley" <warthog9@kernel.org> writes:

> This adds a git:// link to the summary pages should a common
> $gitlinkurl be defined (default is nothing defined, thus nothing
> shown)
> 
> This does make the assumption that the git trees share a common
> path, and nothing to date is known to actually make use of the link

The problem I had and have with this patch is the duplication of data:
$gitlinkurl contains subset of information in @git_base_url_list,
which in turn is filled from GITWEB_BASE_URL build config variable.

I can understand that for performance reason you don't want to check
$projectroot/$project/cloneurl nor gitweb.url config variable for
each and every displayed project; if the link to repository (for git)
cannot be derived from project path (repository path), then simply
do not dosplay it.

> 
> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> ---
>  gitweb/gitweb.perl |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index d84f4c0..7ad096c 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -224,6 +224,10 @@ our %avatar_size = (
>  # This is here to allow for missmatch git & gitweb versions
>  our $missmatch_git = '';
>  
> +#This is here to deal with an extra link on the summary pages - if it's left blank
> +# this link will not be shwon.  If it's set, this will be prepended to the repo and used

s/shwon/shown/

I'd say that 'Full URL is "$gitlinkurl/$project"' instead of last
sentence in above comment.

Please watch for excessive line lengths.

> +our $gitlinkurl = '';

Why not

our $gitlinkurl_base = "++GITWEB_BASE_URL++";

of course changing the name everywhere.

> +
>  # Used to set the maximum load that we will still respond to gitweb queries.
>  # if we exceed this than we do the processing to figure out if there's a mirror
>  # and redirect to it, or to just return 503 server busy
> @@ -4454,6 +4458,10 @@ sub git_project_list_body {
>  		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"log")}, "log") . " | " .
>  		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"tree")}, "tree") .
>  		      ($pr->{'forks'} ? " | " . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"forks")}, "forks") : '') .
> +     		if( $gitlinkurl ne '' ){
> +			print " | ". $cgi->a({-href => "git://$gitlinkurl/".esc_html($pr->{'path'})}, "git");
> +		}
> +		print "".

Does it even pass tests?

 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"log")}, "log") . " | " .
 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"tree")}, "tree") .
 		      ($pr->{'forks'} ? " | " . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"forks")}, "forks") : '') .
+     		      ($gitlinkurl_base ? 
+		       " | " . $cgi->a({-href=>"$gitlinkurl_base/$pr->{'path'}", "git") : '') .
 		      "</td>\n" .
 		      "</tr>\n";
 	}

Changes made: 
* Instead of using separate if conditional statement and print
  statement (note that you forgot to change '.' to ';' to end
  statement) use ternary conditional operator "?:"
* Make $gitlinkurl_base include "git://" protocol specifier
* Do not create "git" link if $gitlinkurl_base is false, which means
  undef, empty string '' and 0 (but 0 is not very likely to be base
  for "git" link).
* Do not use esc_html on fragment of URL.  The CGI.pm should escape
  attributes itself.  If it was HTTP link, one should perhaps esc_url
  on whole link, but esc_html is for escaping HTML.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 1/6] GITWEB - Load Checking

[Mihamina Rakotomandimby]

> "John 'Warthog9' Hawley" <warthog9@kernel.org> :
> +    open($load, '<', '/proc/loadavg') or return 0;

What about systems not having /proc/loadavg

-- 
       Architecte Informatique chez Blueline/Gulfsat:
    Administration Systeme, Recherche & Developpement
                +261 34 29 155 34 / +261 33 11 207 36

Re: [PATCH 4/6] GITWEB - Makefile changes

[Jakub Narebski]

"John 'Warthog9' Hawley" <warthog9@kernel.org> writes:

Below are _proposed_ changes to make commit message easier to read, in
my opinion.  But they are not _necessary_ changes.

> This adjust the makefiles so that you can do such things as

Add "gitweb" target to main Makefile so you would be able to simply
use

> 
> 	make gitweb
> 
> from the top level make tree,

instead of requiring to spell it in full

	make gitweb/gitweb.cgi

>                               or if your in the gitweb directory
> itself typing

Add Makefile in gitweb subdirectory so one can simply run
> 
> 	make

when in gitweb subdirectory,

> 
> will call back up to the main Makefile and build gitweb
> 
> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>

Signoff mismatch.

> ---
>  Makefile        |    4 +++-
>  gitweb/Makefile |   14 ++++++++++++++
>  2 files changed, 17 insertions(+), 1 deletions(-)
>  create mode 100644 gitweb/Makefile

IMPORTANT!

A note about this change: I think it would be better to move creating
gitweb.cgi (and optionally gitweb.min.js) to gitweb/Makefile, and make
main Makefile call gitweb/Makefile, and not vice versa like in your
solution.

If it is possible.
 
> diff --git a/Makefile b/Makefile
> index 4a1e5bc..8db9d01 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1509,6 +1509,8 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
>  	chmod +x $@+ && \
>  	mv $@+ $@
>  
> +.PHONY: gitweb

Why it is here, and not with the .PHONY block at line 1924 of
Makefile?  It would be nice to have comment supporting this choice in
email with this patch (or in commit message).

> +gitweb: gitweb/gitweb.cgi
>  ifdef JSMIN
>  OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js
>  gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js
> @@ -1537,7 +1539,7 @@ endif
>  	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
>  	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
>  	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
> -	    $< >$@+ && \
> +	    $(patsubst %.cgi,%.perl,$@) >$@+ && \

Why this change?

>  	chmod +x $@+ && \
>  	mv $@+ $@
>  
> diff --git a/gitweb/Makefile b/gitweb/Makefile
> new file mode 100644
> index 0000000..8d318b3
> --- /dev/null
> +++ b/gitweb/Makefile
> @@ -0,0 +1,14 @@
> +SHELL = /bin/bash

Why is this needed?

> +
> +FILES = gitweb.cgi
> +
> +.PHONY: $(FILES)

Why .PHONY?  $(FILES) are created.

> +
> +all: $(FILES)
> +
> +$(FILES):
> +	$(MAKE) $(MFLAGS) -C ../ -f Makefile gitweb/$@
> +
> +clean:
> +	rm -rf $(FILES)
> +

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 6/6] GITWEB - Separate defaults from main file

[Jakub Narebski]

"John 'Warthog9' Hawley" <warthog9@kernel.org> writes:

> This is an attempt to break out the default values & associated
> documentation from the main gitweb file so that it's easier to
> browse / read and understand without the associated code involved.
> 
> This helps by making defaults self contained with their documentation
> making it easier for someone to read through things and find what
> they want
> 
> This is also a not-so-subtle start of trying to break up gitweb into
> separate files for easier maintainability, having everything in a
> single file is just a mess and makes the whole thing more complicated
> than it needs to be.  This is a bit of a baby step towards breaking it
> up for easier maintenance.

The question is if easier maintenance and development by spliting
gitweb for developers offsets ease of install for users.

> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>

Signoff mismatch.

> ---
>  .gitignore                  |    1 +
>  Makefile                    |   15 +-
>  gitweb/Makefile             |    2 +-
>  gitweb/gitweb.perl          |  515 +++++--------------------------------------
>  gitweb/gitweb_defaults.perl |  468 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 537 insertions(+), 464 deletions(-)
>  create mode 100644 gitweb/gitweb_defaults.perl
> 
> 
> diff --git a/.gitignore b/.gitignore
> index ac02a58..5e48102 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -151,6 +151,7 @@
>  /git-core-*/?*
>  /gitk-git/gitk-wish
>  /gitweb/gitweb.cgi
> +/gitweb/gitweb_defaults.pl

Hmmm... gitweb/gitweb_defaults.perl as source file, and
gitweb/gitweb_defaults.pl as generated file?  Wouldn't it be better to
go with the convention used elsewhere in gitweb and use
gitweb/gitweb_defaults.perl.in or gitweb/gitweb_defaults.pl.in as
source file?

>  /test-chmtime
>  /test-ctype
>  /test-date
> diff --git a/Makefile b/Makefile
> index 8db9d01..2c5f139 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1510,14 +1510,16 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
>  	mv $@+ $@
>  
>  .PHONY: gitweb
> -gitweb: gitweb/gitweb.cgi
> +gitweb: gitweb/gitweb.cgi gitweb/gitweb_defaults.pl
>  ifdef JSMIN
> -OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js
> -gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js
> +OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js gitweb/gitweb_defaults.pl
> +gitweb/gitweb.cgi gitweb/gitweb_defaults.pl: gitweb/gitweb.perl gitweb/gitweb.min.js gitweb/gitweb_defaults.perl
>  else
> -OTHER_PROGRAMS += gitweb/gitweb.cgi
> -gitweb/gitweb.cgi: gitweb/gitweb.perl
> +OTHER_PROGRAMS += gitweb/gitweb.cgi gitweb/gitweb_defaults.pl
> +gitweb/gitweb.cgi: gitweb/gitweb_defaults.pl
> +gitweb/gitweb.cgi gitweb/gitweb_defaults.pl: gitweb/gitweb.perl gitweb/gitweb_defaults.perl
>  endif

> +	#$(QUIET_GEN)$(RM) $@ $@+ &&

What this line is about?

>  	$(QUIET_GEN)$(RM) $@ $@+ && \
>  	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
>  	    -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \
> @@ -1539,7 +1541,7 @@ endif
>  	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
>  	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
>  	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
> -	    $(patsubst %.cgi,%.perl,$@) >$@+ && \
> +	    $(patsubst %.cgi,%.perl,$(patsubst %.pl, %.perl, $@)) >$@+ && \

Why the slightly inconsistent style ("%.cgi,%perl" vs "%.pl, %perl")?


Also wouldn't all replacements be in the new gitweb_defaults file, so
there would be no need then to do replacements for gitweb.cgi?

Oh, I see there is at least one that stayed in gitweb.perl: $version

>  	chmod +x $@+ && \
>  	mv $@+ $@
>  
> @@ -1913,6 +1915,7 @@ clean:
>  	$(MAKE) -C Documentation/ clean
>  ifndef NO_PERL
>  	$(RM) gitweb/gitweb.cgi
> +	$(RM) gitweb/gitweb_defaults.pl
>  	$(MAKE) -C perl clean
>  endif
>  	$(MAKE) -C templates/ clean
> diff --git a/gitweb/Makefile b/gitweb/Makefile
> index 8d318b3..2bd421a 100644
> --- a/gitweb/Makefile
> +++ b/gitweb/Makefile
> @@ -1,6 +1,6 @@
>  SHELL = /bin/bash
>  
> -FILES = gitweb.cgi
> +FILES = gitweb.cgi gitweb_defaults.pl
>  
>  .PHONY: $(FILES)
>  
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 3b44371..fd41539 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -36,466 +36,67 @@ our $version = "++GIT_VERSION++";
>  our $my_url = $cgi->url();
>  our $my_uri = $cgi->url(-absolute => 1);
>  

[cut deletion]

> +# Define and than setup our configuration 
> +#
> +our(
> +	$VERSION,
> +	$path_info,
> +	$GIT,
> +	$projectroot,
> +	$project_maxdepth,
> +	$home_link,
> +	$home_link_str,
> +	$site_name,
> +	$site_header,
> +	$home_text,
> +	$site_footer,
> +	@stylesheets,
> +	$stylesheet,
> +	$logo,
> +	$favicon,
> +	$javascript,
> +	$logo_url,
> +	$logo_label,
> +	$projects_list,
> +	$projects_list_description_width,
> +	$default_projects_order,
> +	$export_ok,
> +	$export_auth_hook,
> +	$strict_export,
> +	@git_base_url_list,
> +	$default_blob_plain_mimetype,
> +	$default_text_plain_charset,
> +	$mimetypes_file,
> +	$missmatch_git,
> +	$gitlinkurl,
> +	$maxload,
> +	$cache_enable,
> +	$minCacheTime,
> +	$maxCacheTime,
> +	$cachedir,
> +	$backgroundCache,
> +	$nocachedata,
> +	$nocachedatabin,
> +	$fullhashpath,
> +	$fullhashbinpath,
> +	$export_auth_hook,
> +	%known_snapshot_format_aliases,
> +	%known_snapshot_formats,
> +	$path_info,
> +	$fallback_encoding,
> +	%avatar_size,
> +	$project_maxdepth,
> +	$headerRefresh,
> +	$base_url,
> +	$projects_list_description_width,
> +	$default_projects_order,
> +	$prevent_xss,
> +	@diff_opts,
> +	%feature
>  );

Why this block is required?  Why not have variables defined (using
"our") in gitweb_defaults file?

[cut deletion]  

> +do 'gitweb_defaults.pl';
>  
>  sub gitweb_get_feature {
>  	my ($name) = @_;
> diff --git a/gitweb/gitweb_defaults.perl b/gitweb/gitweb_defaults.perl
> new file mode 100644
> index 0000000..ede0daf
> --- /dev/null
> +++ b/gitweb/gitweb_defaults.perl
> @@ -0,0 +1,468 @@
> +# gitweb - simple web interface to track changes in git repositories
> +#
> +# (C) 2005-2006, Kay Sievers <kay.sievers@vrfy.org>
> +# (C) 2005, Christian Gierke
> +#
> +# This program is licensed under the GPLv2
> +
> +# Base URL for relative URLs in gitweb ($logo, $favicon, ...),
> +# needed and used only for URLs with nonempty PATH_INFO
> +$base_url = $my_url;

Why not "our $base_url = $my_url;"?

[cut]

> +1;

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 0/6] Gitweb caching changes v2

[Jakub Narebski]

"John 'Warthog9' Hawley" <warthog9@kernel.org> writes:

> Evening everyone,
> 
> This is the latest incarnation of gitweb w/ caching.  This is
> finally at the point where it should probably start either being
> considered for inclusion or mainline, or I need to accept that this
> will never get in and more perminantely fork (as is the case with
> Fedora where this is going in as gitweb-caching as a parrallel rpm
> package).
> 
> That said this brings the base up to mainline (again), it updates a
> number of elements in the caching engine, and this is a much cleaner
> break-out of the tree vs. what I am currently developing against.
> 
> New things known to work:
>     - Better breakout
>     - You can actually disable the cache now
> 
> - John 'Warthog9' Hawley 
> 
> John 'Warthog9' Hawley (6):
>   GITWEB - Load Checking
>   GITWEB - Missmatching git w/ gitweb
>   GITWEB - Add git:// link to summary pages
>   GITWEB - Makefile changes
>   GITWEB - File based caching layer

This patch didn't made it to git mailing list.  I suspect that you ran
afoul vger anti-SPAM filter.

Does this "File based caching layer" have anything common with GSoC
2008 project, available at git://repo.or.cz/git/gitweb-caching.git ?

>   GITWEB - Separate defaults from main file

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 6/6] GITWEB - Separate defaults from main file

[J.H.]

>> This is also a not-so-subtle start of trying to break up gitweb into
>> separate files for easier maintainability, having everything in a
>> single file is just a mess and makes the whole thing more complicated
>> than it needs to be.  This is a bit of a baby step towards breaking it
>> up for easier maintenance.
> 
> The question is if easier maintenance and development by spliting
> gitweb for developers offsets ease of install for users.

This would just get dropped into the same location that gitweb.cgi 
exists in, there is no real difference in installation, and thus I can't 
see this as an issue for users.

> 
>> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> 
> Signoff mismatch.
> 
>> ---
>>  .gitignore                  |    1 +
>>  Makefile                    |   15 +-
>>  gitweb/Makefile             |    2 +-
>>  gitweb/gitweb.perl          |  515 +++++--------------------------------------
>>  gitweb/gitweb_defaults.perl |  468 +++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 537 insertions(+), 464 deletions(-)
>>  create mode 100644 gitweb/gitweb_defaults.perl
>>
>>
>> diff --git a/.gitignore b/.gitignore
>> index ac02a58..5e48102 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -151,6 +151,7 @@
>>  /git-core-*/?*
>>  /gitk-git/gitk-wish
>>  /gitweb/gitweb.cgi
>> +/gitweb/gitweb_defaults.pl
> 
> Hmmm... gitweb/gitweb_defaults.perl as source file, and
> gitweb/gitweb_defaults.pl as generated file?  Wouldn't it be better to
> go with the convention used elsewhere in gitweb and use
> gitweb/gitweb_defaults.perl.in or gitweb/gitweb_defaults.pl.in as
> source file?

I think you got confused, the committed file is .perl the generated file 
is .pl.

>> +	#$(QUIET_GEN)$(RM) $@ $@+ &&
> 
> What this line is about?

Cruft, thought I had deleted and excluded it, won't be there in next 
version.

> 
>>  	$(QUIET_GEN)$(RM) $@ $@+ && \
>>  	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
>>  	    -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \
>> @@ -1539,7 +1541,7 @@ endif
>>  	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
>>  	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
>>  	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
>> -	    $(patsubst %.cgi,%.perl,$@) >$@+ && \
>> +	    $(patsubst %.cgi,%.perl,$(patsubst %.pl, %.perl, $@)) >$@+ && \
> 
> Why the slightly inconsistent style ("%.cgi,%perl" vs "%.pl, %perl")?

Considering that the defaults is more of an include vs. a cgi it 
probably shouldn't share the standard expected executable suffix, thus I 
used .pl.  Could just as easily change it to .pm, or something else but 
I think it would make the most sense to leave things we are expecting 
the webserver to directly execute as .cgi, and includes as a different 
suffix.

> Also wouldn't all replacements be in the new gitweb_defaults file, so
> there would be no need then to do replacements for gitweb.cgi?

Not all replacements are done in one or the other, and since it's 
basically a NOP to perform the full set of replacements on both files 
that seemed the easiest way to ensure they were done in both places.

> Oh, I see there is at least one that stayed in gitweb.perl: $version
> 

<snip>

>> +# Define and than setup our configuration 
>> +#
>> +our(
>> +	$VERSION,
>> +	$path_info,
>> +	$GIT,
>> +	$projectroot,
>> +	$project_maxdepth,
>> +	$home_link,
>> +	$home_link_str,
>> +	$site_name,
>> +	$site_header,
>> +	$home_text,
>> +	$site_footer,
>> +	@stylesheets,
>> +	$stylesheet,
>> +	$logo,
>> +	$favicon,
>> +	$javascript,
>> +	$logo_url,
>> +	$logo_label,
>> +	$projects_list,
>> +	$projects_list_description_width,
>> +	$default_projects_order,
>> +	$export_ok,
>> +	$export_auth_hook,
>> +	$strict_export,
>> +	@git_base_url_list,
>> +	$default_blob_plain_mimetype,
>> +	$default_text_plain_charset,
>> +	$mimetypes_file,
>> +	$missmatch_git,
>> +	$gitlinkurl,
>> +	$maxload,
>> +	$cache_enable,
>> +	$minCacheTime,
>> +	$maxCacheTime,
>> +	$cachedir,
>> +	$backgroundCache,
>> +	$nocachedata,
>> +	$nocachedatabin,
>> +	$fullhashpath,
>> +	$fullhashbinpath,
>> +	$export_auth_hook,
>> +	%known_snapshot_format_aliases,
>> +	%known_snapshot_formats,
>> +	$path_info,
>> +	$fallback_encoding,
>> +	%avatar_size,
>> +	$project_maxdepth,
>> +	$headerRefresh,
>> +	$base_url,
>> +	$projects_list_description_width,
>> +	$default_projects_order,
>> +	$prevent_xss,
>> +	@diff_opts,
>> +	%feature
>>  );
> 
> Why this block is required?  Why not have variables defined (using
> "our") in gitweb_defaults file?

Wanted to make sure things were properly defined, if in an unexpected 
state, should a user have gitweb.cgi in place but not the defaults.

> 
> [cut deletion]  
> 
>> +do 'gitweb_defaults.pl';
>>  
>>  sub gitweb_get_feature {
>>  	my ($name) = @_;
>> diff --git a/gitweb/gitweb_defaults.perl b/gitweb/gitweb_defaults.perl
>> new file mode 100644
>> index 0000000..ede0daf
>> --- /dev/null
>> +++ b/gitweb/gitweb_defaults.perl
>> @@ -0,0 +1,468 @@
>> +# gitweb - simple web interface to track changes in git repositories
>> +#
>> +# (C) 2005-2006, Kay Sievers <kay.sievers@vrfy.org>
>> +# (C) 2005, Christian Gierke
>> +#
>> +# This program is licensed under the GPLv2
>> +
>> +# Base URL for relative URLs in gitweb ($logo, $favicon, ...),
>> +# needed and used only for URLs with nonempty PATH_INFO
>> +$base_url = $my_url;
> 
> Why not "our $base_url = $my_url;"?

same reason as the other 'our' includes above, though why this ended up 
as a separate patch vs. the rest of the file I don't know.

- John 'Warthog9' Hawley

Re: [PATCH 4/6] GITWEB - Makefile changes

[J.H.]

<snip>

> IMPORTANT!
> 
> A note about this change: I think it would be better to move creating
> gitweb.cgi (and optionally gitweb.min.js) to gitweb/Makefile, and make
> main Makefile call gitweb/Makefile, and not vice versa like in your
> solution.
> 
> If it is possible.

It's quite possible, and I'm fine with doing that.  If no one has any 
objections I can re-work those with the understanding that the build 
process for gitweb shift to the gitweb/ directory instead of the main 
Makefile.

>  
>> diff --git a/Makefile b/Makefile
>> index 4a1e5bc..8db9d01 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1509,6 +1509,8 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
>>  	chmod +x $@+ && \
>>  	mv $@+ $@
>>  
>> +.PHONY: gitweb
> 
> Why it is here, and not with the .PHONY block at line 1924 of
> Makefile?  It would be nice to have comment supporting this choice in
> email with this patch (or in commit message).

There are 6 other instances of .PHONY in the makefile, having the .PHONY 
localized seemed to make it the most obvious since it was right next to 
the actual target.

> 
>> +gitweb: gitweb/gitweb.cgi
>>  ifdef JSMIN
>>  OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js
>>  gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js
>> @@ -1537,7 +1539,7 @@ endif
>>  	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
>>  	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
>>  	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
>> -	    $< >$@+ && \
>> +	    $(patsubst %.cgi,%.perl,$@) >$@+ && \
> 
> Why this change?

Preparation for a later change.  The change could happen all at the same 
time if it makes more logical sense.

> 
>>  	chmod +x $@+ && \
>>  	mv $@+ $@
>>  
>> diff --git a/gitweb/Makefile b/gitweb/Makefile
>> new file mode 100644
>> index 0000000..8d318b3
>> --- /dev/null
>> +++ b/gitweb/Makefile
>> @@ -0,0 +1,14 @@
>> +SHELL = /bin/bash
> 
> Why is this needed?
> 
>> +
>> +FILES = gitweb.cgi
>> +
>> +.PHONY: $(FILES)
> 
> Why .PHONY?  $(FILES) are created.

 From this makefile I wanted to explicitly call up to the main makefile 
no matter what, the main makefile doesn't consider the targets .PHONY 
and it has all the dependencies that it would expect.

>> +
>> +all: $(FILES)
>> +
>> +$(FILES):
>> +	$(MAKE) $(MFLAGS) -C ../ -f Makefile gitweb/$@
>> +
>> +clean:
>> +	rm -rf $(FILES)
>> +

Re: [PATCH 4/6] GITWEB - Makefile changes

[Jakub Narebski]

On Fri, 11 Dec 2009, J.H. wrote:

>> IMPORTANT!
>> 
>> A note about this change: I think it would be better to move creating
>> gitweb.cgi (and optionally gitweb.min.js) to gitweb/Makefile, and make
>> main Makefile call gitweb/Makefile, and not vice versa like in your
>> solution.
>> 
>> If it is possible.
> 
> It's quite possible, and I'm fine with doing that.  If no one has any 
> objections I can re-work those with the understanding that the build 
> process for gitweb shift to the gitweb/ directory instead of the main 
> Makefile.

In my opinion it would be better solution because it would reduce size
of main (master) Makefile, and not be much larger than this solution.
 
git-gui/, Documentation/, perl/ all have their own makefiles, which do
the work, and are called from main (master) Makefile.

>>> diff --git a/Makefile b/Makefile
>>> index 4a1e5bc..8db9d01 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -1509,6 +1509,8 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
>>>  	chmod +x $@+ && \
>>>  	mv $@+ $@
>>>  
>>> +.PHONY: gitweb
>> 
>> Why it is here, and not with the .PHONY block at line 1924 of
>> Makefile?  It would be nice to have comment supporting this choice in
>> email with this patch (or in commit message).
> 
> There are 6 other instances of .PHONY in the makefile, having the .PHONY 
> localized seemed to make it the most obvious since it was right next to 
> the actual target.

I was thinking here about this large block of .PHONY declarations,
the one which is not inside conditional.
 
>>> +gitweb: gitweb/gitweb.cgi
>>>  ifdef JSMIN
>>>  OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js
>>>  gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js
>>> @@ -1537,7 +1539,7 @@ endif
>>>  	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
>>>  	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
>>>  	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
>>> -	    $< >$@+ && \
>>> +	    $(patsubst %.cgi,%.perl,$@) >$@+ && \
>> 
>> Why this change?
> 
> Preparation for a later change.  The change could happen all at the same 
> time if it makes more logical sense.

Please at least describe this change in commit message.

But I think it could be moved to other patch, or put in separate patch.
This change has nothing to do with easier gitweb generation.
 
>>>  	chmod +x $@+ && \
>>>  	mv $@+ $@
>>>  
>>> diff --git a/gitweb/Makefile b/gitweb/Makefile
>>> new file mode 100644
>>> index 0000000..8d318b3
>>> --- /dev/null
>>> +++ b/gitweb/Makefile
>>> @@ -0,0 +1,14 @@
>>> +SHELL = /bin/bash
>> 
>> Why is this needed?

Why do you need to define SHELL?
 
>>> +
>>> +FILES = gitweb.cgi
>>> +
>>> +.PHONY: $(FILES)
>> 
>> Why .PHONY?  $(FILES) are created.
> 
>  From this makefile I wanted to explicitly call up to the main makefile 
> no matter what, the main makefile doesn't consider the targets .PHONY 
> and it has all the dependencies that it would expect.

What is the reason of this phony .PHONY?  If gitweb.cgi is newer than
gitweb.perl (and other sources), then without .PHONY it wouldn't be 
regenerated.  With .PHONY it would call master Makefile... which would
notice that gitweb.cgi is newer than gitweb.perl and do not regenerate.
 
So what this .PHONY does is unnecessary call make on master Makefile...



But I guess this issue would be moot if it was the other way around,
i.e. master Makefile calling gitweb/Makefile.

-- 
Jakub Narebski
Poland

Re: [PATCH 0/6] Gitweb caching changes v2

[Jakub Narebski]

On Fri, 11 Dec 2009, J.H. (John 'Warthog9' Hawley) wrote:
> Jakub Narebski wrote:
>> "John 'Warthog9' Hawley" <warthog9@kernel.org> writes:

>>> John 'Warthog9' Hawley (6):
>>>   GITWEB - Load Checking
>>>   GITWEB - Missmatching git w/ gitweb
>>>   GITWEB - Add git:// link to summary pages
>>>   GITWEB - Makefile changes
>>>   GITWEB - File based caching layer
>> 
>> This patch didn't made it to git mailing list.  I suspect that you ran
>> afoul vger anti-SPAM filter.
>> 
>> Does this "File based caching layer" have anything common with GSoC
>> 2008 project, available at git://repo.or.cz/git/gitweb-caching.git ?
> 
> Yeah, it does seem that way (like I said eaten by a grue), it 
> *currently* has nothing to do with Lea's GSoC code but it is still my 
> intention, long term, to integrate the two.
> 
> The patch, in all it's glory can be viewed at: 
> http://git.kernel.org/?p=git/warthog9/gitweb.git;a=commitdiff;h=42641b1e3bfae14d5cc2e0150355e89cb87951db
> 
> It is anything but a small patch to gitweb, the patch is 117K and 
> comprises 3539 lines (including git header commit information).  There's 
> not any real good way to break it up as it's a bit of an all or nothing 
> patch.

First, why do you reinvent the wheel instead of using one of existing
caching interfaces like CHI or Cache::Cache (perhaps creating a custom
backend or middle layer which incorporates required features, like being
load-aware)?  This way changing from file-based cache to e.g. mmap based
one or to memcached would be very simple.  And you would avoid pitfals
in doing your own cache management.  perl-Cache-Cache should be available
package in extras repositories.

If module is no available this would simply mean no caching, like in many
(or not so many) other cases with optional features in gitweb.


Second, if you can't use CGI::Cache directly, you can always steal the
idea from it, then the change to gitweb itself would be minimal:

  "Internally, the CGI::Cache module ties the output file descriptor
  (usually STDOUT) to an internal variable to which all output is saved."


P.S. I'll postpone critique of the patch itself for now.  The above issues
are much more important.

-- 
Jakub Narebski
Poland

Re: [PATCH 0/6] Gitweb caching changes v2

[J.H.]

Jakub Narebski wrote:
> On Fri, 11 Dec 2009, J.H. (John 'Warthog9' Hawley) wrote:
>> Jakub Narebski wrote:
>>> "John 'Warthog9' Hawley" <warthog9@kernel.org> writes:
> 
>>>> John 'Warthog9' Hawley (6):
>>>>   GITWEB - Load Checking
>>>>   GITWEB - Missmatching git w/ gitweb
>>>>   GITWEB - Add git:// link to summary pages
>>>>   GITWEB - Makefile changes
>>>>   GITWEB - File based caching layer
>>> This patch didn't made it to git mailing list.  I suspect that you ran
>>> afoul vger anti-SPAM filter.
>>>
>>> Does this "File based caching layer" have anything common with GSoC
>>> 2008 project, available at git://repo.or.cz/git/gitweb-caching.git ?
>> Yeah, it does seem that way (like I said eaten by a grue), it 
>> *currently* has nothing to do with Lea's GSoC code but it is still my 
>> intention, long term, to integrate the two.
>>
>> The patch, in all it's glory can be viewed at: 
>> http://git.kernel.org/?p=git/warthog9/gitweb.git;a=commitdiff;h=42641b1e3bfae14d5cc2e0150355e89cb87951db
>>
>> It is anything but a small patch to gitweb, the patch is 117K and 
>> comprises 3539 lines (including git header commit information).  There's 
>> not any real good way to break it up as it's a bit of an all or nothing 
>> patch.
> 
> First, why do you reinvent the wheel instead of using one of existing
> caching interfaces like CHI or Cache::Cache (perhaps creating a custom
> backend or middle layer which incorporates required features, like being
> load-aware)?

Well for starters this isn't exactly a reinvention of the wheel, and 
this isn't something "new" per-se.  This code has been actively running 
on git.kernel.org for something like 3 - 4 years so there's something to 
be said for the devil we know and understand.  As well using the other 
caching strategies involves adding dramatically more complex 
interactions with caching layer.  The caching layer is actually quite 
specific to how git + gitweb works and solves more than just "caching" 
on the surface.  Specifically it solves the stampeding herd problem 
which would have to be solved either way even if I didn't implement my 
own caching, and since I had to do that caching was barely a step beyond 
that to implement.

>  This way changing from file-based cache to e.g. mmap based
> one or to memcached would be very simple.

True but these are *VERY* different caching strategies than the one I've 
got here, yes it's using files as a backend but it's doing so with 
specific goals in mind.  As I've said I plan to integrate Lea's 
memcached based caching into this in the future and that has different 
advantages and disadvantages.

At the end of the day the "normal" caching engines aren't as efficient 
as mine and there is the case the very high performance sites are going 
to have to investigate a number of different solutions to see what works 
best for them.  Mine is also *dramatically* simpler to setup as well, 
turn it on, point it at a directory and your done.

>  And you would avoid pitfals
> in doing your own cache management.  perl-Cache-Cache should be available
> package in extras repositories.

There's pitfalls if I do it myself, or I use one of the other "common" 
perl modules.  I did it this way years ago, I've maintained it and it 
works pretty well.  I won't admit that it's the smartest caching engine 
on the planet, far from it, but it has evolved specifically for gitweb 
and that itself saves me a lot of pitfalls from cache engine + gitweb 
integration.

> If module is no available this would simply mean no caching, like in many
> (or not so many) other cases with optional features in gitweb.

Yes, but as can be seen from how you enable various other caching 
engines the setup of those is non-trivial, this is and either way 
caching *HAS* to be explicitly turned on by the admin/user since they 
are going to have to do *some* configuration, or at least be aware that 
their webapp is going to chew up some sort of resource.

> Second, if you can't use CGI::Cache directly, you can always steal the
> idea from it, then the change to gitweb itself would be minimal:
> 
>   "Internally, the CGI::Cache module ties the output file descriptor
>   (usually STDOUT) to an internal variable to which all output is saved."

I thought about that 3 years ago, and decided it wasn't a good option 
for gitweb.  Why?  There's too many assumptions throughout the code that 
when you do a print it will go immediately out.  Things like error 
messages and such.  Breaking out the prints into prints (which will do 
what is expected) and passing around the output in the $output variables 
makes it a lot simpler easier to differentiate about how / what your 
looking at and a *LOT* easier to debug.

> P.S. I'll postpone critique of the patch itself for now.  The above issues
> are much more important.

That's fine.  The issues your raising aren't new though, and stem back 
to before I created gitweb-caching, got rehashed with Lea's patches and 
not surprisingly are back on the table now.  Like I said above, there is 
no one caching strategy that's perfect in all cases here and that's 
again why I eventually plan to merge Lea's changes (which uses 
memcached) in as well, I'm just trying to get code that I'm getting 
considerable demand for, that's proven, upstream.

- John 'Warthog9' Hawley

Re: [PATCH 6/6] GITWEB - Separate defaults from main file

[Jakub Narebski]

On Fri, 11 Dec 2009, J.H. wrote:

>>> This is also a not-so-subtle start of trying to break up gitweb into
>>> separate files for easier maintainability, having everything in a
>>> single file is just a mess and makes the whole thing more complicated
>>> than it needs to be.  This is a bit of a baby step towards breaking it
>>> up for easier maintenance.
>> 
>> The question is if easier maintenance and development by spliting
>> gitweb for developers offsets ease of install for users.
> 
> This would just get dropped into the same location that gitweb.cgi 
> exists in, there is no real difference in installation, and thus I can't 
> see this as an issue for users.

To be more exact you have to know that you have to drop _generated files_,
which means (for this version of patch) gitweb.cgi and gitweb_defaults.pl
(or whatever the generated file with config variables would be named).


ATTENTION!

Your changes would make stop working all gitweb tests.  Currently they
do some magic with generated gitweb config file "$(pwd)/gitweb_config.perl"
set via GITWEB_CONFIG configuration variable to be able to run
_unprocessed_ gitweb/gitweb.perl (without any substitutions).  This
allow to run tests on "live" version of gitweb.

After your changes it would be no longer possible, at least not if we
want to be sure that we test the same version of gitweb as gitweb_defaults.

It would probably mean that we need to move to testing built version,
i.e. gitweb.cgi, not gitweb.perl

>>> ---
>>>  .gitignore                  |    1 +
>>>  Makefile                    |   15 +-
>>>  gitweb/Makefile             |    2 +-
>>>  gitweb/gitweb.perl          |  515 +++++--------------------------------------
>>>  gitweb/gitweb_defaults.perl |  468 +++++++++++++++++++++++++++++++++++++++
>>>  5 files changed, 537 insertions(+), 464 deletions(-)
>>>  create mode 100644 gitweb/gitweb_defaults.perl
>>>
>>>
>>> diff --git a/.gitignore b/.gitignore
>>> index ac02a58..5e48102 100644
>>> --- a/.gitignore
>>> +++ b/.gitignore
>>> @@ -151,6 +151,7 @@
>>>  /git-core-*/?*
>>>  /gitk-git/gitk-wish
>>>  /gitweb/gitweb.cgi
>>> +/gitweb/gitweb_defaults.pl
>> 
>> Hmmm... gitweb/gitweb_defaults.perl as source file, and
>> gitweb/gitweb_defaults.pl as generated file?  Wouldn't it be better to
>> go with the convention used elsewhere in gitweb and use
>> gitweb/gitweb_defaults.perl.in or gitweb/gitweb_defaults.pl.in as
>> source file?
> 
> I think you got confused, the committed file is .perl the generated file 
> is .pl.

Maybe I wasn't entirely clean.  I meant that the committed source file
should perhaps have *.in extension to denote that it is to be processed
via variable substitution, so it would be

  committed file: gitweb/gitweb_defaults.pl.in
  generated file: gitweb/gitweb_defaults.pl
 
or whatever name (*.pm?) we agree on.

>>>  	$(QUIET_GEN)$(RM) $@ $@+ && \
>>>  	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
>>>  	    -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \
>>> @@ -1539,7 +1541,7 @@ endif
>>>  	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
>>>  	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
>>>  	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
>>> -	    $(patsubst %.cgi,%.perl,$@) >$@+ && \
>>> +	    $(patsubst %.cgi,%.perl,$(patsubst %.pl, %.perl, $@)) >$@+ && \
>> 
>> Why the slightly inconsistent style ("%.cgi,%perl" vs "%.pl, %perl")?
> 
> Considering that the defaults is more of an include vs. a cgi it 
> probably shouldn't share the standard expected executable suffix, thus I 
> used .pl.  Could just as easily change it to .pm, or something else but 
> I think it would make the most sense to leave things we are expecting 
> the webserver to directly execute as .cgi, and includes as a different 
> suffix.

I was not asking about that, but about

+	    $(patsubst %.cgi,%.perl,$(patsubst %.pl, %.perl, $@)) >$@+ && \

vs

+	    $(patsubst %.cgi,%.perl,$(patsubst %.pl,%.perl,$@)) >$@+ && \

But after thinking about it a bit, and consulting make documentation
(in particular definition of $@ variable) this rule shouldn't work at all.

`$@'
     The file name of the target of the rule.  If the target is an
     archive member, then `$@' is the name of the archive file.  In a
     pattern rule that has multiple targets (*note Introduction to
     Pattern Rules: Pattern Intro.), `$@' is the name of whichever
     target caused the rule's commands to be run.
 
What we need is to run pattern substitution for _two_ files, perhaps
using the for loop.

Also I think the order of substitutions would be better to be reversed:

    $(patsubst %.pl,%.perl,$(patsubst %.cgi,%.perl,$FILE)) >$FILE+

This way the gitweb_defaults file can even have *.perl extension

>> Also wouldn't all replacements be in the new gitweb_defaults file, so
>> there would be no need then to do replacements for gitweb.cgi?
> 
> Not all replacements are done in one or the other, and since it's 
> basically a NOP to perform the full set of replacements on both files 
> that seemed the easiest way to ensure they were done in both places.
> 
>> Oh, I see there is at least one that stayed in gitweb.perl: $version
>> 
> 
> <snip>

O.K.

But Makefile would be (slightly) simpler if replacements were needed only
for single file of two.
 
>>> +# Define and than setup our configuration 
>>> +#
>>> +our(
>>> +	$VERSION,
>>> +	$path_info,
>>> +	$GIT,
[...]
>>> +	$prevent_xss,
>>> +	@diff_opts,
>>> +	%feature
>>>  );
>> 
>> Why this block is required?  Why not have variables defined (using
>> "our") in gitweb_defaults file?
> 
> Wanted to make sure things were properly defined, if in an unexpected 
> state, should a user have gitweb.cgi in place but not the defaults.

In my opinion it actually *makes worse*.  I am not sure if gitweb would
work if the variables are undefined, and you would lose 'undeclared 
variable' warning.
 
>>> diff --git a/gitweb/gitweb_defaults.perl b/gitweb/gitweb_defaults.perl
>>> new file mode 100644
>>> index 0000000..ede0daf
>>> --- /dev/null
>>> +++ b/gitweb/gitweb_defaults.perl
>>> @@ -0,0 +1,468 @@
>>> +# gitweb - simple web interface to track changes in git repositories
>>> +#
>>> +# (C) 2005-2006, Kay Sievers <kay.sievers@vrfy.org>
>>> +# (C) 2005, Christian Gierke
>>> +#
>>> +# This program is licensed under the GPLv2

This header should probably be modified, at least stating what the file
is for.

>>> +
>>> +# Base URL for relative URLs in gitweb ($logo, $favicon, ...),
>>> +# needed and used only for URLs with nonempty PATH_INFO
>>> +$base_url = $my_url;
>> 
>> Why not "our $base_url = $my_url;"?
> 
> same reason as the other 'our' includes above,

See comment above about pre-declaring variables actually making it
worse wrt checking.

> though why this ended up  
> as a separate patch vs. the rest of the file I don't know.

Errr... what you are talking about here?

-- 
Jakub Narebski
Poland

Re: [PATCH 0/6] Gitweb caching changes v2

[Jakub Narebski]

On Fri, 11 Dec 2009, J.H. wrote:
> Jakub Narebski wrote:
>> On Fri, 11 Dec 2009, J.H. (John 'Warthog9' Hawley) wrote:
>>> Jakub Narebski wrote:
>>>> "John 'Warthog9' Hawley" <warthog9@kernel.org> writes:
>>>>>
>>>>>   GITWEB - File based caching layer
>>>>>
>>>> This patch didn't made it to git mailing list.  I suspect that you ran
>>>> afoul vger anti-SPAM filter.
>>>
>>> Yeah, it does seem that way (like I said eaten by a grue),

It _might_ be caused by the fact that you used attachement.  But it might
not; you can always use vger-taboo.perl script to check.

>>>> Does this "File based caching layer" have anything common with GSoC
>>>> 2008 project, available at git://repo.or.cz/git/gitweb-caching.git ?
>>>
>>> It *currently* has nothing to do with Lea's GSoC code but it is still my 
>>> intention, in long term, to integrate the two.

The question would be then whether it makes sense to have two caches at
different levels in the stack (see also discussion below about Lea 
approach).

>>> The patch, in all it's glory can be viewed at: 
>>> http://git.kernel.org/?p=git/warthog9/gitweb.git;a=commitdiff;h=42641b1e3bfae14d5cc2e0150355e89cb87951db
>>>
>>> It is anything but a small patch to gitweb, the patch is 117K and 
>>> comprises 3539 lines (including git header commit information).  There's 
>>> not any real good way to break it up as it's a bit of an all or nothing 
>>> patch.
>> 
>> First, why do you reinvent the wheel instead of using one of existing
>> caching interfaces like CHI or Cache::Cache (perhaps creating a custom
>> backend or middle layer which incorporates required features, like being
>> load-aware)?
> 
> Well for starters this isn't exactly a reinvention of the wheel, and 
> this isn't something "new" per-se.  This code has been actively running 
> on git.kernel.org for something like 3 - 4 years so there's something to 
> be said for the devil we know and understand.

Well, if it is not reinventing the wheel, then it is yak shaving (yet
another ...) ;-)

The fact that the code was used and tested at one single installation
doesn't mean that it doesn't have warts that could be avoided by using
ready solution (at least for parts of it).

> As well using the other  
> caching strategies involves adding dramatically more complex 
> interactions with caching layer.

I am hoping that if it was done right, by using CHI or Cache::Cache
caching interface, then choosing alternate caching engine, or adding
extra level of cache would be simple and decoupled from issues specific
to web app or gitweb in particular.

> The caching layer is actually quite  
> specific to how git + gitweb works and solves more than just "caching" 
> on the surface.  

The idea is for gitweb/cache.pm (or gitweb/Gitweb-Cache.pm, or 
gitweb/Gitweb/Cache.pm) is to encapsulate issues specific to gitweb,
like generating cache key, or printing "Generating...", etc.

Perhaps also the idea of filling cache in background (but see discussion
below about capturing STDOUT) could be put there.

> Specifically it solves the stampeding herd problem  
> which would have to be solved either way even if I didn't implement my 
> own caching, and since I had to do that caching was barely a step beyond 
> that to implement.

CHI tries to solve cache miss stampedes via expires_variance mechanism.
There is Cache::Adaptive (and its subclass Cache::Adaptive::ByLoad)
which does adaptive lifetime control (it accepts any Cache::Cache 
compatible cache, so I think it also accepts CHI compatible cache).
Those problems _were_ solved.

>>  This way changing from file-based cache to e.g. mmap based
>> one or to memcached would be very simple.
> 
> True but these are *VERY* different caching strategies than the one I've 
> got here, yes it's using files as a backend but it's doing so with 
> specific goals in mind.  As I've said I plan to integrate Lea's 
> memcached based caching into this in the future and that has different 
> advantages and disadvantages.

Errr... besides using Cache::Cache compatible cache (see!!!), for example
Cache::Memcached, Lea Wiemann's gitweb caching did caching at entirely
different level than original kernel.org's gitweb.

The stack for gitweb looks somewhat like this:

  git commands output       open my $fd, '-|, git_cmd(), ...
          |
          v
  parsed output data        parse_ls_tree_line, parse_commit, ...
          |
          v
  generated HTML etc.       print ...
          :
          V
       caching              optional
    reverse proxy           Varnish, Squid

If I understand correctly Lea Wiemann code cache git command output.
The fork of gitweb used at repo.or.cz does caching of parsed data at
least for most intensive projects list page.  This patch was about caching
generated output.  HTTP caching requires that gitweb can respond to
If-Modified-Since (and generate Last-Modified) and If-None-Match (and
generate ETag) in a time that is much faster than generating full response.

There are advantages and disadvantages for each method of caching; also
the balance might depend on the view used.  For example 'snapshot' view
is best cached via output caching with file-based cache, while for 
'blame_incremental' view straight caching of output doesn't make much
sense while caching command output should give good behaviour.

> At the end of the day the "normal" caching engines aren't as efficient 
> as mine and there is the case the very high performance sites are going 
> to have to investigate a number of different solutions to see what works 
> best for them.  Mine is also *dramatically* simpler to setup as well, 
> turn it on, point it at a directory and you're done.

Do you have any benchmarks?

>>  And you would avoid pitfals
>> in doing your own cache management.  perl-Cache-Cache should be available
>> package in extras repositories.
> 
> There's pitfalls if I do it myself, or I use one of the other "common" 
> perl modules.  I did it this way years ago, I've maintained it and it 
> works pretty well.  I won't admit that it's the smartest caching engine 
> on the planet, far from it, but it has evolved specifically for gitweb 
> and that itself saves me a lot of pitfalls from cache engine + gitweb 
> integration.

If I remember correctly the solution presented here has serious 
disadvantage of not having any cache expire policy, and not being 
size-aware.

>> If module is no available this would simply mean no caching, like in many
>> (or not so many) other cases with optional features in gitweb.
> 
> Yes, but as can be seen from how you enable various other caching 
> engines the setup of those is non-trivial, this is and either way 
> caching *HAS* to be explicitly turned on by the admin/user since they 
> are going to have to do *some* configuration, or at least be aware that 
> their webapp is going to chew up some sort of resource.

I wonder if there is any data that describes when one should enable 
caching, and when one can do without it, e.g. depending on the number
and total size of repositories presented via gitweb.

IMHO cache storage is orthogonal to expire policy, which in turn is
orthogonal on cache use in gitweb.  And those parts should be kept separate
(and tested independently), even if we decide on homegrown caching
solution.

>> Second, if you can't use CGI::Cache directly, you can always steal the
>> idea from it, then the change to gitweb itself would be minimal:
>> 
>>   "Internally, the CGI::Cache module ties the output file descriptor
>>   (usually STDOUT) to an internal variable to which all output is saved."
> 
> I thought about that 3 years ago, and decided it wasn't a good option 
> for gitweb.  Why?  There's too many assumptions throughout the code that 
> when you do a print it will go immediately out.  Things like error 
> messages and such.  Breaking out the prints into prints (which will do 
> what is expected) and passing around the output in the $output variables 
> makes it a lot simpler easier to differentiate about how / what your 
> looking at and a *LOT* easier to debug.

Note that in quite a few places we print directly to output, streaming
the response, for performance (to reduce latency).  If all data must be
first gathered in $output variable (increasing memory pressure in the
case of large files for 'blob_plain', large snapshots, large patches in
'patch' and 'patches' views) then we must wait for it to finish, and not
get data as soon as it is available.

Besides instead of just capturing STDOUT in tied variable (STDERR goes
to web server log courtesy of CGI.pm) we can tee it, i.e. capture it
to $output variable as it is streamed to web browser.  See Capture::Tiny
(although I am not sure how it would interact with CGI.pm logging) and
e.g. PerlIO::tee mechanism from PerlIO::Util.

Going the route of CGI::Cache would mean minimal changes to gitweb...
and no diference in performance if caching is turned off (see streaming).
 
>> P.S. I'll postpone critique of the patch itself for now.  The above issues
>> are much more important.
> 
> That's fine.  The issues your raising aren't new though, and stem back 
> to before I created gitweb-caching, got rehashed with Lea's patches and 
> not surprisingly are back on the table now.  Like I said above, there is 
> no one caching strategy that's perfect in all cases here and that's 
> again why I eventually plan to merge Lea's changes (which uses 
> memcached) in as well, I'm just trying to get code that I'm getting 
> considerable demand for, that's proven, upstream.

Well, there are two solutions.  One is first to decide on proper solution
for gitweb caching.  Another is to have _some_ caching and then improve it.


So below there are a few initial comments about gitweb/cache.pm code:

* gitweb/cache.pm should be, I think, a proper module (require'd or use'd)

* you do not follow coding style used elsewhere in gitweb, e.g. spaces
  around {} and (), for example it is

    }elsif( $cache_enable == 1 ){

  and should be

    } elsif ($cache_enable == 1) {

* flags that are boolean are compared to 0 and 1

* cache key should be generated from both PATH_INFO and QUERY_STRING
  in generic case (unless you turn off $path_info as default, and turn off
  support for path_info URLs); see %input_params hash or href(-replay=>1)

* gitweb till now does not include any variable data in error info

* duplicated code (e.g. fork / cacheUpdate + cacheDisplay / cacheUpdate...)

* inconsistent naming style: cache_fetch but cacheDisplay.

* old style open using globs instead of local filehandles:

    open(cacheFile, '<', "$fullhashpath");

  and should be

    open(my $cache_fh, '<', $fullhashpath);

* busy wait 'do { sleep 2; open ... } while (...)' instead of non-blocking
  wait like select / IO::Select.

That's all from skimming gitweb-ml-v2:gitweb/cache.pm
-- 
Jakub Narebski
Poland

Re: [PATCH 6/6] GITWEB - Separate defaults from main file

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> On Fri, 11 Dec 2009, J.H. wrote:
>
>>>> This is also a not-so-subtle start of trying to break up gitweb into
>>>> separate files for easier maintainability, having everything in a
>>>> single file is just a mess and makes the whole thing more complicated
>>>> than it needs to be.  This is a bit of a baby step towards breaking it
>>>> up for easier maintenance.
>>> 
>>> The question is if easier maintenance and development by spliting
>>> gitweb for developers offsets ease of install for users.
>> 
>> This would just get dropped into the same location that gitweb.cgi 
>> exists in, there is no real difference in installation, and thus I can't 
>> see this as an issue for users.
>
> To be more exact you have to know that you have to drop _generated files_,
> which means (for this version of patch) gitweb.cgi and gitweb_defaults.pl
> (or whatever the generated file with config variables would be named).
>
>
> ATTENTION!

You didn't have to shout.

Any progress on this front?

Not that I am anxious to queue new topics to 'next' right now (we are
frozen for 1.6.6), but I think having what is proven to work well at a
real site like k.org is much better than waiting for an unproven
reimplementation using somebody else's framework only for your theoretical
cleanliness.  John has better things to do than doing such a rewrite
himself, and even if you helped the process by producing a competing
caching scheme based on existing web caching engines, the aggregated
result (not just the web caching engine you base your work on) needs to
get a similar field exposure to prove itself that it can scale to the load
k.org sees, which would be quite a lot of work, no?

Re: [PATCH 6/6] GITWEB - Separate defaults from main file

[J.H.]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> On Fri, 11 Dec 2009, J.H. wrote:
>>
>>>>> This is also a not-so-subtle start of trying to break up gitweb into
>>>>> separate files for easier maintainability, having everything in a
>>>>> single file is just a mess and makes the whole thing more complicated
>>>>> than it needs to be.  This is a bit of a baby step towards breaking it
>>>>> up for easier maintenance.
>>>> The question is if easier maintenance and development by spliting
>>>> gitweb for developers offsets ease of install for users.
>>> This would just get dropped into the same location that gitweb.cgi 
>>> exists in, there is no real difference in installation, and thus I can't 
>>> see this as an issue for users.
>> To be more exact you have to know that you have to drop _generated files_,
>> which means (for this version of patch) gitweb.cgi and gitweb_defaults.pl
>> (or whatever the generated file with config variables would be named).
>>
>>
>> ATTENTION!
> 
> You didn't have to shout.
> 
> Any progress on this front?

Sadly, no.  Busy weekend and a need to get some of the kernel.org 
servers upgraded has taken some precedence.  I should be circling back 
around on this tomorrow I think.

> Not that I am anxious to queue new topics to 'next' right now (we are
> frozen for 1.6.6), but I think having what is proven to work well at a
> real site like k.org is much better than waiting for an unproven
> reimplementation using somebody else's framework only for your theoretical
> cleanliness.  John has better things to do than doing such a rewrite
> himself, and even if you helped the process by producing a competing
> caching scheme based on existing web caching engines, the aggregated
> result (not just the web caching engine you base your work on) needs to
> get a similar field exposure to prove itself that it can scale to the load
> k.org sees, which would be quite a lot of work, no?

Re: [PATCH 6/6] GITWEB - Separate defaults from main file

[Jakub Narebski]

On Wed, 16 Dec 2009, Junio C Hamano wrote:

> Not that I am anxious to queue new topics to 'next' right now (we are
> frozen for 1.6.6), but I think having what is proven to work well at a
> real site like k.org is much better than waiting for an unproven
> reimplementation using somebody else's framework only for your theoretical
> cleanliness.  John has better things to do than doing such a rewrite
> himself, and even if you helped the process by producing a competing
> caching scheme based on existing web caching engines, the aggregated
> result (not just the web caching engine you base your work on) needs to
> get a similar field exposure to prove itself that it can scale to the load
> k.org sees, which would be quite a lot of work, no?

I'm not against (well, not much against) custom caching that kernel.org
uses, but I am against large change to gitweb code currently accompanying
caching, namely gather then output solution, which would negatively 
affect performance when caching is turned off.

Also I'd like to have caching code (the one that didn't made it to git
mailing list for some reason, probably vger anti-SPAM filter) cleaned up
for submission: remove commented-out code, reduce code duplication, 
separate dealing with orthogonal issues (cache itself, adaptivity of cache,
background generation and 'in progress' info, generating key for cache
(and improve key generation to include path_info / use %input_params)),
follow the same style that gitweb itself uses.

As for the "[PATCH 6/6] GITWEB - Separate defaults from main file" patch,
it would require modifying gitweb tests to use generated gitweb/gitweb.cgi
rather than source gitweb/gitweb.perl.


As for having caching code tested by git.kernel.org: IIRC there was issue
with it not having cache expiration thus gathering GB of cached data.
-- 
Jakub Narebski
Poland

Re: [PATCH 6/6] GITWEB - Separate defaults from main file

[Jakub Narebski]

On Tue, 15 Dec 2009, 18:00 -0800, J.H. wrote:
> Junio C Hamano wrote:

> > Any progress on this front?
> 
> Sadly, no.  Busy weekend and a need to get some of the kernel.org 
> servers upgraded has taken some precedence.  I should be circling back 
> around on this tomorrow I think.

So should I wait for reroll with proposals for improvements (modified 
patches)?

-- 
Jakub Narebski
Poland

Re: [PATCH 6/6] GITWEB - Separate defaults from main file

[J.H.]

Jakub Narebski wrote:
> On Tue, 15 Dec 2009, 18:00 -0800, J.H. wrote:
>> Junio C Hamano wrote:
> 
>>> Any progress on this front?
>> Sadly, no.  Busy weekend and a need to get some of the kernel.org 
>> servers upgraded has taken some precedence.  I should be circling back 
>> around on this tomorrow I think.
> 
> So should I wait for reroll with proposals for improvements (modified 
> patches)?

I'd probably wait, though it's starting to look like if I get to gitweb 
today it will be this evening as I ventured off into getting the last 6 
of the kernel.org servers upgraded.  Either way I will have a new patch 
series and some changes in my own git tree shortly.

- John 'Warthog9' Hawley

[PATCHv2 1/6] gitweb: Load checking

[Jakub Narebski]

From: John 'Warthog9' Hawley <warthog9@kernel.org>

This changes slightly the behavior of gitweb, so that it verifies
that the box isn't inundated with before attempting to serve gitweb.
If the box is overloaded, it basically returns a 503 Server Unavailable
until the load falls below the defined threshold.  This helps dramatically
if you have a box that's I/O bound, reaches a certain load and you
don't want gitweb, the I/O hog that it is, increasing the pain the
server is already undergoing.

This behavior is controlled by $maxload configuration variable.
Default is a load of 300, which for most cases should never be hit.
Unset it (set it to undefined value, i.e. undef) to turn off checking.

Currently it requires that '/proc/loadavg' file exists, otherwise the
load check is bypassed (load is taken to be 0).  So platforms that do
not implement '/proc/loadavg' currently cannot use this feature.

Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is my take on this patch, with all my concerns taken into 
consideration... well, all except describing alterante approaches
to straight using /proc/loadavg.

Differences to original version by John 'Warthog9' Hawley (J.H.):
* Slightly improved wording in commit message and in comments
* $maxload described in gitweb/README, in "Gitweb config file variables"
  section
* You can use '$maxload = undef;' to turn off load checking
* Error page for too high load is generated using die_error, which had
  to be extended to handle 503 Service Unavailable HTTP error code

 gitweb/README      |    7 ++++++-
 gitweb/gitweb.perl |   39 +++++++++++++++++++++++++++++++++++----
 2 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/gitweb/README b/gitweb/README
index e34ee79..6c2c8e1 100644
--- a/gitweb/README
+++ b/gitweb/README
@@ -174,7 +174,7 @@ not include variables usually directly set during build):
    Base URL for relative URLs in pages generated by gitweb,
    (e.g. $logo, $favicon, @stylesheets if they are relative URLs),
    needed and used only for URLs with nonempty PATH_INFO via
-   <base href="$base_url>.  Usually gitweb sets its value correctly,
+   <base href="$base_url">.  Usually gitweb sets its value correctly,
    and there is no need to set this variable, e.g. to $my_uri or "/".
  * $home_link
    Target of the home link on top of all pages (the first part of view
@@ -228,6 +228,11 @@ not include variables usually directly set during build):
    repositories from launching cross-site scripting (XSS) attacks.  Set this
    to true if you don't trust the content of your repositories. The default
    is false.
+ * $maxload
+   Used to set the maximum load that we will still respond to gitweb queries.
+   If server load exceed this value then return "503 Service Unavaliable" error.
+   Server load is taken to be 0 if gitweb cannot determine its value.  Set it to
+   undefined value to turn it off.  The default is 300.
 
 
 Projects list file format
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7e477af..a0f0444 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -221,6 +221,12 @@ our %avatar_size = (
 	'double'  => 32
 );
 
+# Used to set the maximum load that we will still respond to gitweb queries.
+# If server load exceed this value then return "503 server busy" error.
+# If gitweb cannot determined server load, it is taken to be 0.
+# Leave it undefined (or set to 'undef') to turn off load checking.
+our $maxload = 300;
+
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (
@@ -551,6 +557,26 @@ if (-e $GITWEB_CONFIG) {
 	do $GITWEB_CONFIG_SYSTEM if -e $GITWEB_CONFIG_SYSTEM;
 }
 
+# Get loadavg of system, to compare against $maxload.
+# Currently it requires '/proc/loadavg' present to get loadavg;
+# if it is not present it returns 0, which means no load checking.
+sub get_loadavg {
+	open my $fd, '<', '/proc/loadavg'
+		or return 0;
+	my @load = split(/\s+/, scalar <$fd>);
+	close $fd;
+
+	# The first three columns measure CPU and IO utilization of the last one,
+	# five, and 10 minute periods.  The fourth column shows the number of
+	# currently running processes and the total number of processes in the m/n
+	# format.  The last column displays the last process ID used.
+	return $load[0] || 0;
+}
+
+if (defined $maxload && get_loadavg() > $maxload) {
+	die_error(503, "The load average on the server is too high");
+}
+
 # version of the core git binary
 our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
 $number_of_git_cmds++;
@@ -3354,14 +3380,19 @@ sub git_footer_html {
 # 500: The server isn't configured properly, or
 #      an internal error occurred (e.g. failed assertions caused by bugs), or
 #      an unknown error occurred (e.g. the git binary died unexpectedly).
+# 503: The server is currently unavailable (because it is overloaded,
+#      or down for maintenance).  Generally, this is a temporary state.
 sub die_error {
 	my $status = shift || 500;
 	my $error = shift || "Internal server error";
 
-	my %http_responses = (400 => '400 Bad Request',
-			      403 => '403 Forbidden',
-			      404 => '404 Not Found',
-			      500 => '500 Internal Server Error');
+	my %http_responses = (
+		400 => '400 Bad Request',
+		403 => '403 Forbidden',
+		404 => '404 Not Found',
+		500 => '500 Internal Server Error',
+		503 => '503 Service Unavailable',
+	);
 	git_header_html($http_responses{$status});
 	print <<EOF;
 <div class="page_body">
-- 
1.6.5.3

[RFC/PATCHv2 2/6] gitweb: Add option to force version match

[Jakub Narebski]

From: John 'Warthog9' Hawley <warthog9@kernel.org>

This adds $git_versions_must_match variable, which is set to true
value checks that we are running on the same version of git that we
shipped with, and if not throw '500 Internal Server Error' error.
What is checked is the version of gitweb (embedded in building
gitweb.cgi), against version of runtime git binary used.

Gitweb can usually run with a mismatched git install.  This is more
here to give an obvious warning as to whats going on vs. silently
failing.

By default this feature is turned off.

Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I don't quite see the reason behind such option, and I think that
error (instead of for example warning) on version mismatch is too much.

This is an RFC because formatting of error page is a bit rough, and
(ab)uses exist CSS classes instead of creating new classnames for
semantic markup.

Differences from original version, by J.H.:
* Changed name and flipped meaning of config variable, from
  $missmatch_git to $git_versions_must_match
* $git_versions_must_match is boolean flag - do not compare with an
  empty string.
* Changed error message a bit, fixed style, added entry in README

 gitweb/README      |    3 +++
 gitweb/gitweb.perl |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/gitweb/README b/gitweb/README
index 6c2c8e1..608b0f8 100644
--- a/gitweb/README
+++ b/gitweb/README
@@ -233,6 +233,9 @@ not include variables usually directly set during build):
    If server load exceed this value then return "503 Service Unavaliable" error.
    Server load is taken to be 0 if gitweb cannot determine its value.  Set it to
    undefined value to turn it off.  The default is 300.
+ * $git_versions_must_match
+   If set, gitweb fails with 500 Internal Server Error if the version of gitweb
+   doesn't match version of git binary.  The default is false.
 
 
 Projects list file format
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3222131..b9bd865 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -221,6 +221,9 @@ our %avatar_size = (
 	'double'  => 32
 );
 
+# If it is true, exit if gitweb version and git binary version don't match
+our $git_versions_must_match = 0;
+
 # Used to set the maximum load that we will still respond to gitweb queries.
 # If server load exceed this value then return "503 server busy" error.
 # If gitweb cannot determined server load, it is taken to be 0.
@@ -581,6 +584,36 @@ if (defined $maxload && get_loadavg() > $maxload) {
 our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
 $number_of_git_cmds++;
 
+# Throw an error if git versions does not match, if $git_versions_must_match is true.
+if ($git_versions_must_match &&
+    $git_version ne $version) {
+	git_header_html('500 - Internal Server Error');
+	my $admin_contact =
+		defined $ENV{'SERVER_ADMIN'} ? ", $ENV{'SERVER_ADMIN'}," : '';
+	print <<"EOT";
+<div class="page_body">
+<br /><br />
+500 - Internal Server Error
+<br />
+</div>
+<hr />
+<div class="readme">
+<h1 align="center">*** Warning ***</h1>
+<p>
+This version of gitweb was compiled for <b>@{[esc_html($version)]}</b>,
+however git version <b>@{[esc_html($git_version)]}</b> was found on server,
+and administrator requested strict version checking.
+</p>
+<p>
+Please contact the server administrator${admin_contact} to either configure
+gitweb to allow mismatched versions, or update git or gitweb installation.
+</p>
+</div>
+EOT
+	git_footer_html();
+	exit;
+}
+
 $projects_list ||= $projectroot;
 
 # ======================================================================
-- 
1.6.5.3

[PATCHv2 3/6] gitweb: Optionally add "git" links in project list page

[Jakub Narebski]

From: John 'Warthog9' Hawley <warthog9@kernel.org>

This adds a "git" link for each project in the project list page,
should a common $gitlinkurl_base be defined and not empty.  The full
URL of each link is composed of $gitlinkurl_base and project name.
It is intended for git:// links, and in fact GITWEB_BASE_URL build
variable is used as its default value only if it starts with git://

This does make the assumption that the git repositories share a common
path.  Nothing to date is known to actually make use of introduced
link.

Created "git" link follows rel=vcs-* microformat specification:
  http://kitenet.net/~joey/rfc/rel-vcs/

Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I think it might be good idea... but for the fact "Nothing to date is
known to actually make use of introduced link".  What's its intended
use?

Differences to original version by John 'Warthog9' Hawley (J.H.):
* It doesn't cause syntax error ;-)
* Escaping of attribute value is left to CGI.pm
* $gitlinkurl got renamed to $gitlinkurl_base, now includes git://
  prefix, and defaults to GITWEB_BASE_URL if it begins with git://
* Added description to gitweb/README
* Uses rel=vcs-* microformat by Joey Hess

I assume that nobody sane would define $gitlinkurl_base to "0"...

 gitweb/README      |    4 ++++
 gitweb/gitweb.perl |    8 ++++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/gitweb/README b/gitweb/README
index 608b0f8..36fb059 100644
--- a/gitweb/README
+++ b/gitweb/README
@@ -71,6 +71,7 @@ You can specify the following configuration variables when building GIT:
  * GITWEB_BASE_URL
    Git base URLs used for URL to where fetch project from, i.e. full
    URL is "$git_base_url/$project".  Shown on projects summary page.
+   If it begins with "git://" it is also used for $gitlinkurl_base, see below.
    Repository URL for project can be also configured per repository; this
    takes precedence over URLs composed from base URL and a project name.
    Note that you can setup multiple base URLs (for example one for
@@ -204,6 +205,9 @@ not include variables usually directly set during build):
    access, and one for http:// "dumb" protocol access).  Note that per
    repository configuration in 'cloneurl' file, or as values of gitweb.url
    project config.
+ * $gitlinkurl_base
+   Git base URL used (if it is defined and not empty) for "git" link in
+   projects list, for each project.  Full URL is "$gitlinkurl_base/$project".
  * $default_blob_plain_mimetype
    Default mimetype for blob_plain (raw) view, if mimetype checking
    doesn't result in some other type; by default 'text/plain'.
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b9bd865..efb6471 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -224,6 +224,10 @@ our %avatar_size = (
 # If it is true, exit if gitweb version and git binary version don't match
 our $git_versions_must_match = 0;
 
+# If this variable is set and not empty, add an extra link called "git"
+# for each project in project list.  Full URL is "$gitlinkurl_base/$project".
+our $gitlinkurl_base = ("++GITWEB_BASE_URL++" =~ m!^(git://.*)$!) ? $1 : '';
+
 # Used to set the maximum load that we will still respond to gitweb queries.
 # If server load exceed this value then return "503 server busy" error.
 # If gitweb cannot determined server load, it is taken to be 0.
@@ -4472,6 +4476,10 @@ sub git_project_list_body {
 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"log")}, "log") . " | " .
 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"tree")}, "tree") .
 		      ($pr->{'forks'} ? " | " . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"forks")}, "forks") : '') .
+		      ($gitlinkurl_base ?
+		       " | " . $cgi->a({-href=>"$gitlinkurl_base/$pr->{'path'}",
+		                        -rel=>"vcs-git"}, "git")
+		      : '') .
 		      "</td>\n" .
 		      "</tr>\n";
 	}
-- 
1.6.5.3

[PATCH/RFCv2 4/6] gitweb: Makefile improvements

[Jakub Narebski]

From: John 'Warthog9' Hawley <warthog9@kernel.org>

This commit adjust the main Makefile so you can simply run

     make gitweb

which in turn calls gitweb/Makefile.  This means that in order to
generate gitweb, you can simply run 'make' from gitweb subdirectory:

     cd gitweb
     make

Targets gitweb/gitweb.cgi and (dependent on JSMIN being defined)
gitweb/gitweb.min.js in main Makefile are preserved for backward
compatibility.

Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This implements separate Makefile for gitweb, with main Makefile calling
it, like for gitk-git/, git-gui/, Documentation/, t/, and templates/
directories.

It is marked as RFC because I don't feel that my make-fu is strong enough
to be sure that there are no errors / mistakes.  Very slightly tested.

 Makefile        |   64 +++++----------------------
 gitweb/Makefile |  129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 141 insertions(+), 52 deletions(-)
 create mode 100644 gitweb/Makefile

diff --git a/Makefile b/Makefile
index 4a1e5bc..50d815e 100644
--- a/Makefile
+++ b/Makefile
@@ -280,29 +280,6 @@ pathsep = :
 # JavaScript minifier invocation that can function as filter
 JSMIN =
 
-# default configuration for gitweb
-GITWEB_CONFIG = gitweb_config.perl
-GITWEB_CONFIG_SYSTEM = /etc/gitweb.conf
-GITWEB_HOME_LINK_STR = projects
-GITWEB_SITENAME =
-GITWEB_PROJECTROOT = /pub/git
-GITWEB_PROJECT_MAXDEPTH = 2007
-GITWEB_EXPORT_OK =
-GITWEB_STRICT_EXPORT =
-GITWEB_BASE_URL =
-GITWEB_LIST =
-GITWEB_HOMETEXT = indextext.html
-GITWEB_CSS = gitweb.css
-GITWEB_LOGO = git-logo.png
-GITWEB_FAVICON = git-favicon.png
-ifdef JSMIN
-GITWEB_JS = gitweb.min.js
-else
-GITWEB_JS = gitweb.js
-endif
-GITWEB_SITE_HEADER =
-GITWEB_SITE_FOOTER =
-
 export prefix bindir sharedir sysconfdir
 
 CC = gcc
@@ -1509,6 +1486,11 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 	chmod +x $@+ && \
 	mv $@+ $@
 
+
+.PHONY: gitweb
+gitweb:
+	$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all
+
 ifdef JSMIN
 OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js
 gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js
@@ -1516,30 +1498,13 @@ else
 OTHER_PROGRAMS += gitweb/gitweb.cgi
 gitweb/gitweb.cgi: gitweb/gitweb.perl
 endif
-	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
-	    -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \
-	    -e 's|++GIT_BINDIR++|$(bindir)|g' \
-	    -e 's|++GITWEB_CONFIG++|$(GITWEB_CONFIG)|g' \
-	    -e 's|++GITWEB_CONFIG_SYSTEM++|$(GITWEB_CONFIG_SYSTEM)|g' \
-	    -e 's|++GITWEB_HOME_LINK_STR++|$(GITWEB_HOME_LINK_STR)|g' \
-	    -e 's|++GITWEB_SITENAME++|$(GITWEB_SITENAME)|g' \
-	    -e 's|++GITWEB_PROJECTROOT++|$(GITWEB_PROJECTROOT)|g' \
-	    -e 's|"++GITWEB_PROJECT_MAXDEPTH++"|$(GITWEB_PROJECT_MAXDEPTH)|g' \
-	    -e 's|++GITWEB_EXPORT_OK++|$(GITWEB_EXPORT_OK)|g' \
-	    -e 's|++GITWEB_STRICT_EXPORT++|$(GITWEB_STRICT_EXPORT)|g' \
-	    -e 's|++GITWEB_BASE_URL++|$(GITWEB_BASE_URL)|g' \
-	    -e 's|++GITWEB_LIST++|$(GITWEB_LIST)|g' \
-	    -e 's|++GITWEB_HOMETEXT++|$(GITWEB_HOMETEXT)|g' \
-	    -e 's|++GITWEB_CSS++|$(GITWEB_CSS)|g' \
-	    -e 's|++GITWEB_LOGO++|$(GITWEB_LOGO)|g' \
-	    -e 's|++GITWEB_FAVICON++|$(GITWEB_FAVICON)|g' \
-	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
-	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
-	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
-	    $< >$@+ && \
-	chmod +x $@+ && \
-	mv $@+ $@
+	$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) $(patsubst gitweb/%,%,$@)
+
+ifdef JSMIN
+gitweb/gitweb.min.js: gitweb/gitweb.js
+	$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) $(patsubst gitweb/%,%,$@)
+endif # JSMIN
+
 
 git-instaweb: git-instaweb.sh gitweb/gitweb.cgi gitweb/gitweb.css gitweb/gitweb.js
 	$(QUIET_GEN)$(RM) $@ $@+ && \
@@ -1566,11 +1531,6 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: % : unimplemented.sh
 	mv $@+ $@
 endif # NO_PERL
 
-ifdef JSMIN
-gitweb/gitweb.min.js: gitweb/gitweb.js
-	$(QUIET_GEN)$(JSMIN) <$< >$@
-endif # JSMIN
-
 configure: configure.ac
 	$(QUIET_GEN)$(RM) $@ $<+ && \
 	sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
diff --git a/gitweb/Makefile b/gitweb/Makefile
new file mode 100644
index 0000000..c9eb1ee
--- /dev/null
+++ b/gitweb/Makefile
@@ -0,0 +1,129 @@
+# The default target of this Makefile is...
+all::
+
+# Define V=1 to have a more verbose compile.
+#
+# Define JSMIN to point to JavaScript minifier that functions as
+# a filter to have gitweb.js minified.
+#
+
+prefix ?= $(HOME)
+bindir ?= $(prefix)/bin
+RM ?= rm -f
+
+# JavaScript minifier invocation that can function as filter
+JSMIN ?=
+
+# default configuration for gitweb
+GITWEB_CONFIG = gitweb_config.perl
+GITWEB_CONFIG_SYSTEM = /etc/gitweb.conf
+GITWEB_HOME_LINK_STR = projects
+GITWEB_SITENAME =
+GITWEB_PROJECTROOT = /pub/git
+GITWEB_PROJECT_MAXDEPTH = 2007
+GITWEB_EXPORT_OK =
+GITWEB_STRICT_EXPORT =
+GITWEB_BASE_URL =
+GITWEB_LIST =
+GITWEB_HOMETEXT = indextext.html
+GITWEB_CSS = gitweb.css
+GITWEB_LOGO = git-logo.png
+GITWEB_FAVICON = git-favicon.png
+ifdef JSMIN
+GITWEB_JS = gitweb.min.js
+else
+GITWEB_JS = gitweb.js
+endif
+GITWEB_SITE_HEADER =
+GITWEB_SITE_FOOTER =
+
+# include user config
+-include ../config.mak.autogen
+-include ../config.mak
+
+# determine version
+../GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
+	$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE
+
+-include ../GIT-VERSION-FILE
+
+### Build rules
+
+SHELL_PATH ?= $(SHELL)
+PERL_PATH  ?= /usr/bin/perl
+
+# Shell quote;
+bindir_SQ = $(subst ','\'',$(bindir))         #'
+SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH)) #'
+PERL_PATH_SQ  = $(subst ','\'',$(PERL_PATH))  #'
+
+# Quiet generation (unless V=1)
+QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
+QUIET_SUBDIR1  =
+
+ifneq ($(findstring $(MAKEFLAGS),w),w)
+PRINT_DIR = --no-print-directory
+else # "make -w"
+NO_SUBDIR = :
+endif
+
+ifneq ($(findstring $(MAKEFLAGS),s),s)
+ifndef V
+	QUIET          = @
+	QUIET_GEN      = $(QUIET)echo '   ' GEN $@;
+	QUIET_SUBDIR0  = +@subdir=
+	QUIET_SUBDIR1  = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
+	                 $(MAKE) $(PRINT_DIR) -C $$subdir
+	export V
+	export QUIET
+	export QUIET_GEN
+	export QUIET_SUBDIR0
+	export QUIET_SUBDIR1
+endif
+endif
+
+all:: gitweb.cgi
+
+ifdef JSMIN
+FILES=gitweb.cgi gitweb.min.js
+gitweb.cgi: gitweb.perl gitweb.min.js
+else # !JSMIN
+FILES=gitweb.cgi
+gitweb.cgi: gitweb.perl
+endif # JSMIN
+
+gitweb.cgi:
+	$(QUIET_GEN)$(RM) $@ $@+ && \
+	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
+	    -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \
+	    -e 's|++GIT_BINDIR++|$(bindir)|g' \
+	    -e 's|++GITWEB_CONFIG++|$(GITWEB_CONFIG)|g' \
+	    -e 's|++GITWEB_CONFIG_SYSTEM++|$(GITWEB_CONFIG_SYSTEM)|g' \
+	    -e 's|++GITWEB_HOME_LINK_STR++|$(GITWEB_HOME_LINK_STR)|g' \
+	    -e 's|++GITWEB_SITENAME++|$(GITWEB_SITENAME)|g' \
+	    -e 's|++GITWEB_PROJECTROOT++|$(GITWEB_PROJECTROOT)|g' \
+	    -e 's|"++GITWEB_PROJECT_MAXDEPTH++"|$(GITWEB_PROJECT_MAXDEPTH)|g' \
+	    -e 's|++GITWEB_EXPORT_OK++|$(GITWEB_EXPORT_OK)|g' \
+	    -e 's|++GITWEB_STRICT_EXPORT++|$(GITWEB_STRICT_EXPORT)|g' \
+	    -e 's|++GITWEB_BASE_URL++|$(GITWEB_BASE_URL)|g' \
+	    -e 's|++GITWEB_LIST++|$(GITWEB_LIST)|g' \
+	    -e 's|++GITWEB_HOMETEXT++|$(GITWEB_HOMETEXT)|g' \
+	    -e 's|++GITWEB_CSS++|$(GITWEB_CSS)|g' \
+	    -e 's|++GITWEB_LOGO++|$(GITWEB_LOGO)|g' \
+	    -e 's|++GITWEB_FAVICON++|$(GITWEB_FAVICON)|g' \
+	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
+	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
+	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
+	    $< >$@+ && \
+	chmod +x $@+ && \
+	mv $@+ $@
+
+ifdef JSMIN
+gitweb.min.js: gitweb.js
+	$(QUIET_GEN)$(JSMIN) <$< >$@
+endif # JSMIN
+
+clean:
+	$(RM) $(FILES)
+
+.PHONY: all clean .FORCE-GIT-VERSION-FILE
-- 
1.6.5.3