From: Jakub Narebski <jnareb@gmail.com>
Date: Wed, 13 Jan 2010 17:33:55 GMT
Subject: Re: [PATCH 2/7] gitweb: Add option to force version match
Message-ID: <201001131833.55603.jnareb@gmail.com>
In-Reply-To: <1263374828-15342-3-git-send-email-warthog9@eaglescrag.net>

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