Re: [PATCH 2/7] gitweb: Add option to force version match
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>'.
Show changes to diff +7 −1
@@ -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
Jakub Narebski