Re: [PATCH 1/3] gitweb: Add option to force version match
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Oct 28, 2010, 09:52 UTC
- Message-ID
- <m3fwvqir0o.fsf@localhost.localdomain>
- In-Reply-To
- <1288226574-19068-2-git-send-email-warthog9@eaglescrag.net>
"John 'Warthog9' Hawley" <warthog9@eaglescrag.net> writes:
Show 9 quoted lines
> This adds $git_versions_must_match variable, which is set to true, > 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.
Were you bitten by mismatch between git version and gitweb version? Just how serious is that (hypothetical?) situation?
Should it be warning, or erroring out?
> > By default this feature is turned on.
I think last time this patch stalled on discussion whether this feature should be turned on by default, where it can break some setups; or be turned off by default, where it is much less useful protecting against errors.
Show 7 quoted lines
> > Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org> > Signed-off-by: Jakub Narebski <jnareb@gmail.com> > --- > gitweb/README | 4 ++++ > gitweb/gitweb.perl | 27 +++++++++++++++++++++++++++ > 2 files changed, 31 insertions(+), 0 deletions(-)
For me, if $git_versions_must_match is to be on by default, I would prefer also update to t/gitweb-lib.sh, so hat I don't have to recompile git to test new version of gitweb...
Show 40 quoted lines
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 8d7e4c5..215a4e9 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -232,6 +232,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 = 1;
> +
> # 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.
> @@ -649,6 +652,29 @@ sub check_loadavg {
> }
> }
>
> +sub check_versionmatch {
> + # 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;
> +<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, 'Internal server error', $err_msg);
> + }
> +
> +}Nice.
Show 9 quoted lines
> +
> # ======================================================================
> # input validation and dispatch
>
> @@ -1075,6 +1101,7 @@ sub run_request {
> evaluate_uri();
> evaluate_gitweb_config();
> check_loadavg();
> + check_versionmatch();Shouldn't it be before check_loadavg()?
-- Jakub Narebski Poland ShadeHawk on #git