Re: [PATCH 1/3] gitweb: Add option to force version match

[Jakub Narebski <jnareb@gmail.com>]

"John 'Warthog9' Hawley" <warthog9@eaglescrag.net> writes:

> 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.

> 
> 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...  

> 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.

> +
>  # ======================================================================
>  # 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