Re: [PATCH 2/7] gitweb: Add option to force version match
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Jan 13, 2010, 17:33 UTC
- 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,/
Show 10 quoted lines
> 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
Show 28 quoted lines
> @@ -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