Re: [PATCH 2/7] gitweb: Add option to force version match
From: Jakub Narebski <hidden>
Date: 2016-06-15 22:48:00
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 <redacted> ---
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(-)
quoted hunk
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
quoted hunk
@@ -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