[PATCH 0/8] Miscelanous gitweb improvements from 'Gitweb caching v5' by J.H.

[PATCH 0/8] Miscelanous gitweb improvements from 'Gitweb caching v5' by J.H.

[Jakub Narebski]

This series of patches consist of improved and reordered, and rebased early
parts (1-7) of "[PATCH 0/9] Gitweb caching v5" thread by John 'Warthog9'
Hawley.  Those patches are also available from
  git://git.kernel.org/pub/scm/git/warthog9/gitweb.git gitweb-ml-v5

I have added signoff where it was missing, and changed all authorship
of patches by J.H. to use @kernel.org email address.

Pull request:
~~~~~~~~~~~~~
The following changes since commit 3a985c27fe8bb9ed2fa9580a27c4b4dd7c7192ea:
  Junio C Hamano (1):
        Update draft release notes to 1.7.0

are available in the git repository at:

  git://repo.or.cz/git/jnareb-git.git gitweb/gitweb-ml-v5

Table of contents:
~~~~~~~~~~~~~~~~~~
 [PATCH 1/8] gitweb: Make running t9501 test with '--debug' reliable and usable
 [PATCH 2/8] gitweb: Load checking
 [PATCH 3/8] gitweb: Makefile improvements
 [PATCH 4/8] gitweb: Check that $site_header etc. are defined before using them
 [PATCH 5/8] gitweb: add a get function to compliment print_local_time
 [PATCH 6/8] gitweb: add a get function to compliment print_sort_th
 [PATCH 7/8] gitweb: Add optional extra parameter to die_error, for extended explanation
 [PATCH 8/8] gitweb: Add an option to force version match

First patch in series is my addition, made to be able to run t9501 test in
'debug' mode without it screwing terminal due to dumping (binary!) snapshot
to it.  All patches in series passes t9500, t9501 and t9502; it allowed to
find bugs in their implementation.

I have reordered patches so those that are less controversial and more
ready to be applied come first, and those that are a bit controversial and
perhaps less ready later (and also later those patches that introduce new
feature without any user for it).

The patches 5 and 6 "add a get function" introduce new format_* functions
which do not have yet any user in gitweb.

Patch 7 "optional extra parameter to die_error" is used only in patch 8,
but it would be I think good feature to have anyway.

In patch 8 the $git_versions_must_match variable is again set to 0 (off) by
default; Pasky's complaint in
  Message-ID: <20100124215924.GA9553@machine.or.cz>
  http://permalink.gmane.org/gmane.comp.version-control.git/137922
made me notice (see my reply to it) that it could be very hard to explain
how to setup gitweb to turn this feature off, in some rare cases.  So it is
again off, and with more detailed explanation of an error.

Note that like 'Gitweb caching v5' thread this series is missing
  gitweb: Optionally add "git" links in project list page
that was present in 'Gitweb caching v2' and 'Miscelanous gitweb
improvements from J.H.' threads.

Diffstat:
~~~~~~~~~
 Makefile                                 |   65 +++------------
 gitweb/Makefile                          |  129 ++++++++++++++++++++++++++++++
 gitweb/README                            |   12 +++-
 gitweb/gitweb.perl                       |  121 ++++++++++++++++++++++++----
 t/gitweb-lib.sh                          |    2 +
 t/t9501-gitweb-standalone-http-status.sh |   57 ++++++++++++-
 6 files changed, 309 insertions(+), 77 deletions(-)
 create mode 100644 gitweb/Makefile

-- 
Jakub Narebski
Poland

[PATCH 1/8 v1] gitweb: Make running t9501 test with '--debug' reliable and usable

[Jakub Narebski]

Remove test_debug lines after 'snapshots: tgz only default format
enabled' and 'snapshots: all enabled in default, use default disabled
value' tests.  Those tests constitute of multiple gitweb_run
invocation, therefore outputting gitweb.output for the last gitweb_run
wouldn't help much in debugging test failure, and can only confuse.

For snapshot tests which check for "200 OK" status, change
  test_debug 'cat gitweb.output'
to
  test_debug 'cat gitweb.headers'
Otherwise when running this test with '--debug' option,
t/t9501-gitweb-standalone-http-status.sh would dump *binary data* (the
snapshot itself) to standard output, which can mess up state of terminal
due to term control characters which can be embedded in output.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch appears in this series for the first time.

I have decided against cleaning up t/t9501-gitweb-standalone-http-status.sh
by conforming to single style of quoting and whitespaces, and by putting 
initial test_commit inside test_expect_success setup.  This is the minimal
change.

 t/t9501-gitweb-standalone-http-status.sh |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index 0688a57..9e8bc01 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -33,7 +33,6 @@ test_expect_success \
     grep "403 - Snapshot format not allowed" gitweb.output &&
     gitweb_run "p=.git;a=snapshot;h=HEAD;sf=zip" &&
     grep "403 - Unsupported snapshot format" gitweb.output'
-test_debug 'cat gitweb.output'
 
 
 cat >>gitweb_config.perl <<\EOF
@@ -50,7 +49,6 @@ test_expect_success \
     grep "403 - Snapshot format not allowed" gitweb.output &&
     gitweb_run "p=.git;a=snapshot;h=HEAD;sf=zip" &&
     grep "Status: 200 OK" gitweb.output'
-test_debug 'cat gitweb.output'
 
 
 cat >>gitweb_config.perl <<\EOF
@@ -72,7 +70,7 @@ test_expect_success \
     'snapshots: tgz explicitly enabled' \
     'gitweb_run "p=.git;a=snapshot;h=HEAD;sf=tgz" &&
     grep "Status: 200 OK" gitweb.output'
-test_debug 'cat gitweb.output'
+test_debug 'cat gitweb.headers'
 
 
 # ----------------------------------------------------------------------
@@ -82,7 +80,7 @@ test_expect_success 'snapshots: good tree-ish id' '
 	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
 	grep "Status: 200 OK" gitweb.output
 '
-test_debug 'cat gitweb.output'
+test_debug 'cat gitweb.headers'
 
 test_expect_success 'snapshots: bad tree-ish id' '
 	gitweb_run "p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz" &&
@@ -105,7 +103,7 @@ test_expect_success 'snapshots: good object id' '
 	gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
 	grep "Status: 200 OK" gitweb.output
 '
-test_debug 'cat gitweb.output'
+test_debug 'cat gitweb.headers'
 
 test_expect_success 'snapshots: bad object id' '
 	gitweb_run "p=.git;a=snapshot;h=abcdef01234;sf=tgz" &&
-- 
1.6.6.1

[PATCH 2/8 v6] 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
(provisions are included for additional checks to be added by others).

There is simple test in t/t9501-gitweb-standalone-http-status.sh to
check that it correctly returns "503 Service Unavailable" if load is
too high, and also if there are any Perl warnings or errors.

Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Changes from version from 'Gitweb caching v5' and
  git://git.kernel.org/pub/scm/git/warthog9/gitweb.git gitweb-ml-v5

* Load checking was moved after setting $git_version, as this variable
  is used in git_header_html, which in turn is used in die_error.  
  Otherwise we would get "Use of unitialized value" warning.

* In git_footer_html we check if $action is defined before comparing
  it with 'blame_incremental'.  This is first time that git_footer_html
  can be called (via die_error) with $action undefined; earlier all calls
  to git_footer_html were after dispatch, which set $action to appropriate
  default value if it was not set (actionless gitweb URL, e.g. projects
  list or project summary).  Otherise we would get "Use of unitialized
  value" warning.

* t/gitweb-lib.sh turns off load checking, just in case.

* There was added test for this feature to t9501, for the simplest case
  of $maxload set to 0.


 gitweb/README                            |    7 ++++-
 gitweb/gitweb.perl                       |   48 ++++++++++++++++++++++++++---
 t/gitweb-lib.sh                          |    1 +
 t/t9501-gitweb-standalone-http-status.sh |   22 +++++++++++++
 4 files changed, 72 insertions(+), 6 deletions(-)
 mode change 100644 => 100755 t/gitweb-lib.sh

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..e2522cc 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,12 +557,38 @@ 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 {
+	if( -e '/proc/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;
+	}
+	# additional checks for load average should go here for things that don't export
+	# /proc/loadavg
+
+	return 0;
+}
+
 # version of the core git binary
 our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
 $number_of_git_cmds++;
 
 $projects_list ||= $projectroot;
 
+if (defined $maxload && get_loadavg() > $maxload) {
+	die_error(503, "The load average on the server is too high");
+}
+
 # ======================================================================
 # input validation and dispatch
 
@@ -3328,7 +3360,8 @@ sub git_footer_html {
 	}
 
 	print qq!<script type="text/javascript" src="$javascript"></script>\n!;
-	if ($action eq 'blame_incremental') {
+	if (defined $action &&
+	    $action eq 'blame_incremental') {
 		print qq!<script type="text/javascript">\n!.
 		      qq!startBlame("!. href(action=>"blame_data", -replay=>1) .qq!",\n!.
 		      qq!           "!. href() .qq!");\n!.
@@ -3354,14 +3387,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">
diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh
old mode 100644
new mode 100755
index 76d8b7b..5a734b1
--- a/t/gitweb-lib.sh
+++ b/t/gitweb-lib.sh
@@ -25,6 +25,7 @@ our \$favicon = 'file:///$TEST_DIRECTORY/../gitweb/git-favicon.png';
 our \$projects_list = '';
 our \$export_ok = '';
 our \$strict_export = '';
+our \$maxload = undef;
 
 EOF
 
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index 9e8bc01..7590f10 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -112,4 +112,26 @@ test_expect_success 'snapshots: bad object id' '
 test_debug 'cat gitweb.output'
 
 
+# ----------------------------------------------------------------------
+# load checking
+
+# always hit the load limit
+cat >>gitweb_config.perl <<\EOF
+our $maxload = 0;
+EOF
+
+test_expect_success 'load checking: load too high (default action)' '
+	gitweb_run "p=.git" &&
+	grep "Status: 503 Service Unavailable" gitweb.headers &&
+	grep "503 - The load average on the server is too high" gitweb.body
+'
+test_debug 'cat gitweb.log' # just in case
+test_debug 'cat gitweb.headers'
+
+# turn off load checking
+cat >>gitweb_config.perl <<\EOF
+our $maxload = undef;
+EOF
+
+
 test_done
-- 
1.6.6.1

[PATCH 3/8 v6] 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 patch is unchanged from the version from 'Gitweb caching v5':
  git://git.kernel.org/pub/scm/git/warthog9/gitweb.git gitweb-ml-v5

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

diff --git a/Makefile b/Makefile
index af08c8f..eb0fffb 100644
--- a/Makefile
+++ b/Makefile
@@ -278,29 +278,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
@@ -1538,6 +1515,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
@@ -1545,30 +1527,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) $@ $@+ && \
@@ -1595,12 +1560,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
-
 ifndef NO_PYTHON
 $(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS
 $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
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.6.1

[PATCH 4/8 v6] gitweb: Check that $site_header etc. are defined before using them

[Jakub Narebski]

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

If one of $site_header, $site_footer or $home_text is not defined you
get extraneous errors in the web logs, for example (line wrapped for
better readibility):

 [Wed Jan 13 16:55:42 2010] [error] [client ::1] [Wed Jan 13 16:55:42 2010]
 gitweb.cgi: Use of uninitialized value $site_header in -f at
 /var/www/gitweb/gitweb.cgi line 3287., referer: http://git/gitweb.cgi

This ensures that those variables are defined before trying to use it.

Note that such error can happen only because of an error in gitweb
config file; building gitweb.cgi can make mentioned variables holding
empty string (it is even the default), but they not undefined.

Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Changes from version from 'Gitweb caching v5' and
  git://git.kernel.org/pub/scm/git/warthog9/gitweb.git gitweb-ml-v5

* Check explicitly that $site_header is defined, and not only that it
  is false-ish
* Check also for $site_footer and $home_text being defined
* Slightly more detailed commit message


I have decided not to protect against undefined $projects_list, as
such check would have to be more complicated and quite different from
checks for $site_header, $site_footer and $home_text.

Note that it is purely defensive programming, as this should not
happen unless there are very strange errors in gitweb config file.

 gitweb/gitweb.perl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e2522cc..a4148d3 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3254,7 +3254,7 @@ EOF
 	print "</head>\n" .
 	      "<body>\n";
 
-	if (-f $site_header) {
+	if (defined $site_header && -f $site_header) {
 		insert_file($site_header);
 	}
 
@@ -3355,7 +3355,7 @@ sub git_footer_html {
 		print "</div>\n"; # class="page_footer"
 	}
 
-	if (-f $site_footer) {
+	if (defined $site_footer && -f $site_footer) {
 		insert_file($site_footer);
 	}
 
@@ -4781,7 +4781,7 @@ sub git_project_list {
 	}
 
 	git_header_html();
-	if (-f $home_text) {
+	if (defined $home_text && -f $home_text) {
 		print "<div class=\"index_include\">\n";
 		insert_file($home_text);
 		print "</div>\n";
-- 
1.6.6.1

[PATCH 5/8 v6] gitweb: add a get function to compliment print_local_time

[Jakub Narebski]

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

This adds a get function (named format_local_time) for print_local_time,
so that the basic function can be used outside of their straight printing
operation.

Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Changes from version from 'Gitweb caching v5' and
  git://git.kernel.org/pub/scm/git/warthog9/gitweb.git gitweb-ml-v5

* Change function name from get_* to format_*, following gitweb
  subroutine naming convention.
* Add final ';'

 gitweb/gitweb.perl |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a4148d3..debaf55 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3509,14 +3509,21 @@ sub git_print_header_div {
 }
 
 sub print_local_time {
+	print format_local_time(@_);
+}
+
+sub format_local_time {
+	my $localtime = '';
 	my %date = @_;
 	if ($date{'hour_local'} < 6) {
-		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
+		$localtime .= sprintf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
 			$date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
 	} else {
-		printf(" (%02d:%02d %s)",
+		$localtime .= sprintf(" (%02d:%02d %s)",
 			$date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
 	}
+
+	return $localtime;
 }
 
 # Outputs the author name and date in long form
-- 
1.6.6.1

[PATCH 6/8 v6] gitweb: add a get function to compliment print_sort_th

[Jakub Narebski]

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

This adds a get function (named format_sort_th) for print_sort_th,
so that the basic function can be used outside of their straight
printing operation.

Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Changes from version from 'Gitweb caching v5' and
  git://git.kernel.org/pub/scm/git/warthog9/gitweb.git gitweb-ml-v5

* Change function name from get_* to format_*, following gitweb
  subroutine naming convention.

 gitweb/gitweb.perl |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index debaf55..466fa8a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4347,17 +4347,24 @@ sub fill_project_list_info {
 # print 'sort by' <th> element, generating 'sort by $name' replay link
 # if that order is not selected
 sub print_sort_th {
+	print format_sort_th(@_);
+}
+
+sub format_sort_th {
 	my ($name, $order, $header) = @_;
+	my $sort_th = "";
 	$header ||= ucfirst($name);
 
 	if ($order eq $name) {
-		print "<th>$header</th>\n";
+		$sort_th .= "<th>$header</th>\n";
 	} else {
-		print "<th>" .
-		      $cgi->a({-href => href(-replay=>1, order=>$name),
-		               -class => "header"}, $header) .
-		      "</th>\n";
+		$sort_th .= "<th>" .
+		            $cgi->a({-href => href(-replay=>1, order=>$name),
+		                     -class => "header"}, $header) .
+		            "</th>\n";
 	}
+
+	return $sort_th;
 }
 
 sub git_project_list_body {
-- 
1.6.6.1

[PATCH 7/8 v6] gitweb: Add optional extra parameter to die_error, for extended explanation

[Jakub Narebski]

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

This is a small change that just adds a 3rd, optional, parameter to die_error
that allows for extended error information to be output along with what the
error was.

Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Changes from version from 'Gitweb caching v5' and
  git://git.kernel.org/pub/scm/git/warthog9/gitweb.git gitweb-ml-v5

* Slightly changed commit summary (commit description).
* Align with spaces, instead of indenting with tab

Those changes are purely cosmetic; the commit is practically unchanged
from the version by J.H.

 gitweb/gitweb.perl |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 466fa8a..d0c3ff2 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3392,6 +3392,7 @@ sub git_footer_html {
 sub die_error {
 	my $status = shift || 500;
 	my $error = shift || "Internal server error";
+	my $extra = shift;
 
 	my %http_responses = (
 		400 => '400 Bad Request',
@@ -3406,8 +3407,13 @@ sub die_error {
 <br /><br />
 $status - $error
 <br />
-</div>
 EOF
+	if (defined $extra) {
+		print "<hr />\n" .
+		      "$extra\n";
+	}
+	print "</div>\n";
+
 	git_footer_html();
 	exit;
 }
-- 
1.6.6.1

[PATCH 8/8 v6] gitweb: Add an option to force version match

[Jakub Narebski]

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

This adds $git_versions_must_match variable, which if set to true,
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 what's going on vs. silently
failing.

By default this feature is turned off.

Add tests to t9501-gitweb-standalone-http-status.sh that this feature
works correctly (as expected) if turned on, both in match and no match
case.

Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Changes from version from 'Gitweb caching v5' and
  git://git.kernel.org/pub/scm/git/warthog9/gitweb.git gitweb-ml-v5

* Again turned off by default
* More detailed error description, as requested by Petr 'Pasky' Baudis
  in http://permalink.gmane.org/gmane.comp.version-control.git/137922
* Which in turn required to set $GITWEB_CONFIG_SYSTEM to be able to
  use its value in error description; otherwise we would get 'Variable 
  "$GITWEB_CONFIG_SYSTEM" is not imported' error.
* Turn off forcing version matching in t/gitweb-lib.sh
* Add some tests for this feature in t9501-gitweb-standalone-http-status.sh

 gitweb/README                            |    5 +++++
 gitweb/gitweb.perl                       |   29 ++++++++++++++++++++++++++++-
 t/gitweb-lib.sh                          |    1 +
 t/t9501-gitweb-standalone-http-status.sh |   27 +++++++++++++++++++++++++++
 4 files changed, 61 insertions(+), 1 deletions(-)

diff --git a/gitweb/README b/gitweb/README
index 6c2c8e1..83a25a9 100644
--- a/gitweb/README
+++ b/gitweb/README
@@ -233,6 +233,11 @@ 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 to true value, gitweb fails with "500 Internal Server Error" error
+   if the version of the gitweb doesn't match version of the git binary.
+   Gitweb can usually run with a mismatched git install.   The default is 0
+   (false).
 
 
 Projects list file format
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index d0c3ff2..e69efeb 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.
@@ -550,10 +553,10 @@ sub filter_snapshot_fmts {
 }
 
 our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
+our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++";
 if (-e $GITWEB_CONFIG) {
 	do $GITWEB_CONFIG;
 } else {
-	our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++";
 	do $GITWEB_CONFIG_SYSTEM if -e $GITWEB_CONFIG_SYSTEM;
 }
 
@@ -583,6 +586,30 @@ sub get_loadavg {
 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) {
+	my $admin_contact =
+		defined $ENV{'SERVER_ADMIN'} ? ", $ENV{'SERVER_ADMIN'}," : '';
+	my $err_msg = <<EOT;
+<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 by setting
+\$git_versions_must_match to @{[esc_html($git_versions_must_match)]}
+(true value) in gitweb configuration file,
+'@{[esc_path(-e $GITWEB_CONFIG ? $GITWEB_CONFIG : $GITWEB_CONFIG_SYSTEM)]}'.
+</p>
+<p>
+Please contact the server administrator${admin_contact} to either configure
+gitweb to allow mismatched versions, or update git or gitweb installation
+so that their versions do match.
+</p>
+EOT
+	die_error(500, 'Internal server error', $err_msg);
+}
+
 $projects_list ||= $projectroot;
 
 if (defined $maxload && get_loadavg() > $maxload) {
diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh
index 5a734b1..66a3e2d 100755
--- a/t/gitweb-lib.sh
+++ b/t/gitweb-lib.sh
@@ -26,6 +26,7 @@ our \$projects_list = '';
 our \$export_ok = '';
 our \$strict_export = '';
 our \$maxload = undef;
+our \$git_versions_must_match = 0;
 
 EOF
 
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index 7590f10..e195f97 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -133,5 +133,32 @@ cat >>gitweb_config.perl <<\EOF
 our $maxload = undef;
 EOF
 
+# ======================================================================
+# check $git_versions_must_match feature
+# should be last section, just in case
+cp -f gitweb_config.perl gitweb_config.perl.bak
+echo 'our $git_versions_must_match = 1;' >>gitweb_config.perl
+
+cat <<\EOF >>gitweb_config.perl
+our $version = "current";
+EOF
+test_expect_success 'force version match: no match' '
+	gitweb_run "p=.git" &&
+	grep "Status: 500 Internal Server Error" gitweb.headers &&
+	grep "500 - Internal server error" gitweb.body
+'
+test_debug 'cat gitweb.headers'
+
+cat <<\EOF >>gitweb_config.perl
+# must be kept in sync with code in gitweb/gitweb.perl
+our $version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
+EOF
+test_expect_success 'force version match: match' '
+	gitweb_run "p=.git" &&
+	grep "Status: 200 OK" gitweb.headers
+'
+test_debug 'cat gitweb.headers'
+
+mv -f gitweb_config.perl.bak gitweb_config.perl
 
 test_done
-- 
1.6.6.1

Re: [PATCH 5/8 v6] gitweb: add a get function to compliment print_local_time

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> This adds a get function (named format_local_time) for print_local_time,
> so that the basic function can be used outside of their straight printing
> operation.

I didn't particularly feel like giving praise to print-local-time nor
print-sort-th, so I rewrote the log message for these two commits ;-).

    gitweb: add a "string" variant of print_local_time
    
    Add a function (named format_local_time) that returns the string that
    print_local_time would print.

Thanks.  Now I can drop GIT_SKIP_TESTS=t95?? while building 'pu'.  All
pushed out to the usual places.

Re: [PATCH 8/8 v6] gitweb: Add an option to force version match

[J.H.]

Starting to pop off the stack, and this came up first.  A quick reading
of this, I'd sign-off and agree to patches 1-7 completely.

I'm still going to take issue that this being off by default is the
wrong behavior and leaving this off by default more or less means that
it will never get run and it becomes useless code.  If this isn't on by
default, it shouldn't be committed, as I can't think of a legitimate use
case where an admin is going to turn this on.

I'd still argue this needs to be on by default to at least give admins
the explicit warning that if they want to deviate they are taking their
own risks, and that gitweb might not run as expected.  Once the warning
is disabled in a configuration file it's not like it's going to be
re-enabled.  People loading gitweb from their distro's package
management will likely be in sync properly and will never see this.
Those who are installing gitweb independent of their distro's package
management will at least be warned of the risk, at least until better
error reporting is done and in gitweb.

I've got a slightly modified version of this that re-enables it by
default, it passes t9501 for me.

- John 'Warthog9' Hawley

On 01/30/2010 02:30 PM, Jakub Narebski wrote:
> From: John 'Warthog9' Hawley <warthog9@kernel.org>
> 
> This adds $git_versions_must_match variable, which if set to true,
> 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 what's going on vs. silently
> failing.
> 
> By default this feature is turned off.
> 
> Add tests to t9501-gitweb-standalone-http-status.sh that this feature
> works correctly (as expected) if turned on, both in match and no match
> case.
> 
> Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> Changes from version from 'Gitweb caching v5' and
>   git://git.kernel.org/pub/scm/git/warthog9/gitweb.git gitweb-ml-v5
> 
> * Again turned off by default
> * More detailed error description, as requested by Petr 'Pasky' Baudis
>   in http://permalink.gmane.org/gmane.comp.version-control.git/137922
> * Which in turn required to set $GITWEB_CONFIG_SYSTEM to be able to
>   use its value in error description; otherwise we would get 'Variable 
>   "$GITWEB_CONFIG_SYSTEM" is not imported' error.
> * Turn off forcing version matching in t/gitweb-lib.sh
> * Add some tests for this feature in t9501-gitweb-standalone-http-status.sh
> 
>  gitweb/README                            |    5 +++++
>  gitweb/gitweb.perl                       |   29 ++++++++++++++++++++++++++++-
>  t/gitweb-lib.sh                          |    1 +
>  t/t9501-gitweb-standalone-http-status.sh |   27 +++++++++++++++++++++++++++
>  4 files changed, 61 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/README b/gitweb/README
> index 6c2c8e1..83a25a9 100644
> --- a/gitweb/README
> +++ b/gitweb/README
> @@ -233,6 +233,11 @@ 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 to true value, gitweb fails with "500 Internal Server Error" error
> +   if the version of the gitweb doesn't match version of the git binary.
> +   Gitweb can usually run with a mismatched git install.   The default is 0
> +   (false).
>  
>  
>  Projects list file format
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index d0c3ff2..e69efeb 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.
> @@ -550,10 +553,10 @@ sub filter_snapshot_fmts {
>  }
>  
>  our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
> +our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++";
>  if (-e $GITWEB_CONFIG) {
>  	do $GITWEB_CONFIG;
>  } else {
> -	our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++";
>  	do $GITWEB_CONFIG_SYSTEM if -e $GITWEB_CONFIG_SYSTEM;
>  }
>  
> @@ -583,6 +586,30 @@ sub get_loadavg {
>  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) {
> +	my $admin_contact =
> +		defined $ENV{'SERVER_ADMIN'} ? ", $ENV{'SERVER_ADMIN'}," : '';
> +	my $err_msg = <<EOT;
> +<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 by setting
> +\$git_versions_must_match to @{[esc_html($git_versions_must_match)]}
> +(true value) in gitweb configuration file,
> +'@{[esc_path(-e $GITWEB_CONFIG ? $GITWEB_CONFIG : $GITWEB_CONFIG_SYSTEM)]}'.
> +</p>
> +<p>
> +Please contact the server administrator${admin_contact} to either configure
> +gitweb to allow mismatched versions, or update git or gitweb installation
> +so that their versions do match.
> +</p>
> +EOT
> +	die_error(500, 'Internal server error', $err_msg);
> +}
> +
>  $projects_list ||= $projectroot;
>  
>  if (defined $maxload && get_loadavg() > $maxload) {
> diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh
> index 5a734b1..66a3e2d 100755
> --- a/t/gitweb-lib.sh
> +++ b/t/gitweb-lib.sh
> @@ -26,6 +26,7 @@ our \$projects_list = '';
>  our \$export_ok = '';
>  our \$strict_export = '';
>  our \$maxload = undef;
> +our \$git_versions_must_match = 0;
>  
>  EOF
>  
> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
> index 7590f10..e195f97 100755
> --- a/t/t9501-gitweb-standalone-http-status.sh
> +++ b/t/t9501-gitweb-standalone-http-status.sh
> @@ -133,5 +133,32 @@ cat >>gitweb_config.perl <<\EOF
>  our $maxload = undef;
>  EOF
>  
> +# ======================================================================
> +# check $git_versions_must_match feature
> +# should be last section, just in case
> +cp -f gitweb_config.perl gitweb_config.perl.bak
> +echo 'our $git_versions_must_match = 1;' >>gitweb_config.perl
> +
> +cat <<\EOF >>gitweb_config.perl
> +our $version = "current";
> +EOF
> +test_expect_success 'force version match: no match' '
> +	gitweb_run "p=.git" &&
> +	grep "Status: 500 Internal Server Error" gitweb.headers &&
> +	grep "500 - Internal server error" gitweb.body
> +'
> +test_debug 'cat gitweb.headers'
> +
> +cat <<\EOF >>gitweb_config.perl
> +# must be kept in sync with code in gitweb/gitweb.perl
> +our $version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
> +EOF
> +test_expect_success 'force version match: match' '
> +	gitweb_run "p=.git" &&
> +	grep "Status: 200 OK" gitweb.headers
> +'
> +test_debug 'cat gitweb.headers'
> +
> +mv -f gitweb_config.perl.bak gitweb_config.perl
>  
>  test_done

Re: [PATCH 8/8 v6] gitweb: Add an option to force version match

[Jakub Narebski]

On Mon, 1 Feb 2010, at 17:01:37 -0800, J.H. wrote:

> Starting to pop off the stack, and this came up first.  A quick reading
> of this, I'd sign-off and agree to patches 1-7 completely.
> 
> I'm still going to take issue that this being off by default is the
> wrong behavior and leaving this off by default more or less means that
> it will never get run and it becomes useless code.  If this isn't on by
> default, it shouldn't be committed, as I can't think of a legitimate use
> case where an admin is going to turn this on.

Well, I don't think that mismatched git and gitweb version should be
serious problem in practice, unless they are seriously out of sync.  
And in such situation (where either git is stale and gitweb updated,
or git updated and gitweb kept stale e.g. because it is heavily 
customized with not ported changes) gitweb admin should turn this
feature on.

> 
> I'd still argue this needs to be on by default to at least give admins
> the explicit warning that if they want to deviate they are taking their
> own risks, and that gitweb might not run as expected.  Once the warning
> is disabled in a configuration file it's not like it's going to be
> re-enabled.  People loading gitweb from their distro's package
> management will likely be in sync properly and will never see this.
> Those who are installing gitweb independent of their distro's package
> management will at least be warned of the risk, at least until better
> error reporting is done and in gitweb.

If you want to have it turned on by default (which is _incompatible_
change, and which was not announced enough, I think; it might mean
that gitweb can stop working after git or gitweb update), beside
changing commit message and gitweb/README (and of course definition of
$git_versions_must_match variable), you would have also update 
explanation in die_error for version mismatch.

Current version, as requested by Petr 'Pasky' Baudis, explains how
to turn feature off if it was enabled.  For this it needs to check
which config file is present, but we know at least that some config
file had to be used (beside possibility of hand-editing gitweb.cgi).
This is not the case if this feature is turned on by default: there
is possible that there is no gitweb config file, and all configuration
was done at build time.  How to explain then how to turn this feature
off?  What happens if both $GIWEB_CONFIG and $GITWEB_CONFIG_SYSTEM
are empty because of misconfiguration during build ($GITWEB_CONFIG
is set by default to gitweb_config.perl, and $GITWEB_CONFIG_SYSTEM
is set by default to /etc/gitweb.conf)?  Which one to recommend if both
variables are set, but neither file exists?

The difficulty of explanation how to turn this feature off if it is
on by default was one of main reasons to not have it turned on by
default.


Side-note: Perhaps error is too strong a measure, and it would be better
to just issue warning somewhere instead?

> 
> I've got a slightly modified version of this that re-enables it by
> default, it passes t9501 for me.

As it should, because gitweb-lib.sh was modified to explicitly turn off
this feature (see below), to allow testing gitweb without need to 
recompile whole git, and to allow testing _not installed_ (source)
version of gitweb.

> > diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh
> > index 5a734b1..66a3e2d 100755
> > --- a/t/gitweb-lib.sh
> > +++ b/t/gitweb-lib.sh
> > @@ -26,6 +26,7 @@ our \$projects_list = '';
> >  our \$export_ok = '';
> >  our \$strict_export = '';
> >  our \$maxload = undef;
> > +our \$git_versions_must_match = 0;
> >  
> >  EOF
> >  

-- 
Jakub Narebski
Poland

Re: [PATCH 8/8 v6] gitweb: Add an option to force version match

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> On Mon, 1 Feb 2010, at 17:01:37 -0800, J.H. wrote:
>
>> Starting to pop off the stack, and this came up first.  A quick reading
>> of this, I'd sign-off and agree to patches 1-7 completely.
>> 
>> I'm still going to take issue that this being off by default is the
>> wrong behavior and leaving this off by default more or less means that
>> it will never get run and it becomes useless code.  If this isn't on by
>> default, it shouldn't be committed, as I can't think of a legitimate use
>> case where an admin is going to turn this on.
>
> Well, I don't think that mismatched git and gitweb version should be
> serious problem in practice, unless they are seriously out of sync.  
> And in such situation (where either git is stale and gitweb updated,
> or git updated and gitweb kept stale e.g. because it is heavily 
> customized with not ported changes) gitweb admin should turn this
> feature on.

I am not quite sure why the "boolean" default matters that much to deserve
this much of bandwidth.

If it is assumed that most everybody uses matching git and gitweb given by
their distro, then the default does not matter to them.  So let's think
about others.

If it is by default on, then here are what the site administrators would
do:

 - Some may want to _always_ make sure they run matching versions and
   consider having different versions of git and gitweb as _a mistake_; .
   they leave the check on by default, and they don't have to do anything.

 - Some may _not_ even care.  They flip it off, and keep it that way.
   They do that _only_ once.

Between these two classes of people, if you make the default off, you are
making more careful ones pay a bit more "one time" price, while allowing
lazy ones potentially shoot themselves in the foot more easily.  As the
maintainer, I am sympathetic John's insistence of having this check on by
default, and would even suggest that the configuration variable has to be
set to the exact string "I accept the risk of running non-matching
versions of git and gitweb" to turn the warning off ;-), but either way,
it doesn't seem to make too much of a difference.

Admittedly, unlike John or Pasky, I do not run a public gitweb instanace
nor have to deal with error reports from the end users, and that might be
contributing to my bias.

But I think we need to try helping the third category of people:

 - Some may need to _keep_ unmatched versions of git and gitweb for the
   users on their box.  Perhaps interactive users need a certain version
   of git, but they want to show spiffier looking gitweb to the general
   public by installing newer one.  They check if the combination of the
   particular (older) git and (newer) gitweb work well together, and then
   they say "don't warn, I know it is Ok _now_".

Unless the configuration can express "this combination has been checked,
so do not warn, but do warn when any other untested combination is being
used", this class of people won't be helped by having a configuration.

They instead have to _remember_ that they need to flip the warning bit on
before updating their git and/or gitweb, only to get reminded that they
need to make sure the combination works well.

That's a funny chicken-and-egg problem, isn't it?

Re: [PATCH 8/8 v6] gitweb: Add an option to force version match

[Junio C Hamano]

"J.H." <warthog9@eaglescrag.net> writes:

> Starting to pop off the stack, and this came up first.  A quick reading
> of this, I'd sign-off and agree to patches 1-7 completely.

Then let me queue up to 7th to 'next', so I do not have to mentally keep
track of which ones are already judged to be good, and we can move forward
immediately after 1.7.0 ships, regardless of the discusson on the 8th one.