[PATCH 2/7] gitweb: Add option to force version match

[PATCH 2/7] gitweb: Add option to force version match

[John 'Warthog9' Hawley]

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.

v3 - warthog9: adjust to use die_error instead of recreating it
v2 - Jakub: Changes to make non-default, and change naming
v1 - warthog9: Initial

Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/README      |    3 +++
 gitweb/gitweb.perl |   28 ++++++++++++++++++++++++++++
 2 files changed, 31 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 0a07d3a..f17593f 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.
@@ -587,6 +590,31 @@ 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) {
+	my $admin_contact =
+		defined $ENV{'SERVER_ADMIN'} ? ", $ENV{'SERVER_ADMIN'}," : '';
+	my $err_msg = <<EOT;
+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>
+EOT
+	die_error(500, $err_msg);
+}
+
 $projects_list ||= $projectroot;
 
 # ======================================================================
-- 
1.6.5.2

Re: [PATCH 1/7] gitweb: Load checking

[Jakub Narebski]

On Wed, 13 Jan 2010, John 'Warthog9' Hawley wrote:
> 
> v3 - warthog9: small additional adjustment to indicate where new load
>      checking should be added for future improvements
> v2 - Jakub: switching to using

"switching to using" what?

> v1 - warthog9: Initial creation
> 
> Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---

Also, such comments should be put in the comment section, here i.e. between
"---\n" separator and the diffstat, and not in the commit message.

>  gitweb/README      |    7 ++++++-
>  gitweb/gitweb.perl |   45 +++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 47 insertions(+), 5 deletions(-)

Other that this minor nick (which I guess could be fixed by Junio when
applying), this one looks good.

-- 
Jakub Narebski
Poland

Re: [PATCH 2/7] gitweb: Add option to force version match

[Jakub Narebski]

On Wed, 13 Jan 2010, John 'Warthog9' Hawley wrote:
> From: John 'Warthog9' Hawley <warthog9@kernel.org>
> 
> This adds $git_versions_must_match variable, which is set to true

s/is set to true value/if 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.

[moved version info below, after "---" separator]

> Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---

Moved to the correct place: information about patch versioning should
be put in comment section[1], not in commit message.
 
[1] Which means between "---" separator and diffstat.

> v3 - warthog9: adjust to use die_error instead of recreating it
> v2 - Jakub: Changes to make non-default, and change naming
> v1 - warthog9: Initial

>  gitweb/README      |    3 +++
>  gitweb/gitweb.perl |   28 ++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+), 0 deletions(-)

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl

> @@ -587,6 +590,31 @@ 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) {
> +	my $admin_contact =
> +		defined $ENV{'SERVER_ADMIN'} ? ", $ENV{'SERVER_ADMIN'}," : '';
> +	my $err_msg = <<EOT;
> +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>
> +EOT
> +	die_error(500, $err_msg);
> +}

I'm sorry to be this picky, but don't you think that caller should not need
to know "intimate" details on how die_error works.  In particular please
notice that you must have known that die_error wraps its output in <div>...
which is not even mentioned in comments.

The second argument of die_error() subroutine is meant to be alternative
description of error condition: short and one line.  For situations such
like this one, where we want to describe error in more details, it would
be better, I think, to change signature of die_error() to take (optionally)
three arguments, and use 'die_error(500, undef, $err_msg);', with $err_msg
starting from '<h1>...</h1>'.

@@ -3460,6 +3460,7 @@
 sub die_error {
 	my $status = shift || 500;
 	my $error = shift || "Internal server error";
+	my $extra = shift;
 
 	my %http_responses = (
 		400 => '400 Bad Request',
@@ -3475,8 +3476,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;
 }


Other that this minor issue it looks good.

-- 
Jakub Narebski
Poland