Re: [PATCH 2/6] GITWEB - Missmatching git w/ gitweb

[Jakub Narebski <jnareb@gmail.com>]

"John 'Warthog9' Hawley" <warthog9@kernel.org> writes:

> This adds $missmatch_git so that gitweb can run with a miss-matched
> git install.  Gitweb, generally, runs fine on a very broad range of
> git versions, but it's not always practicle or useful to upgrade it
> every time you upgrade git.
> 
> This allows the administrator to realize they are miss-matched, and
> should they be so inclined, disable the check entirely and run in
> a miss-matched fasion.
> 
> This is more here to give an obvious warning as to whats going on
> vs. silently failing.

First, why one would want to require that gitweb version (version at
the time of build) and runtime git version (version of git used to run
commands) match?

Second, it is mismatch, not missmatch (one 's', not double 's').

Third, in my opinion it would be better to name variable in question
e.g. $versions_must_match and also flip its meaning (true means check
that versions match, and show an error otherwise).

>
> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>

signoff mismatch

> ---
>  gitweb/gitweb.perl |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 813e48f..d84f4c0 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -221,6 +221,9 @@ our %avatar_size = (
>  	'double'  => 32
>  );
>  
> +# This is here to allow for missmatch git & gitweb versions
> +our $missmatch_git = '';
> +

First, 'undef' is false, so it could have been written as

+our $missmatch_git;

Or if you prefer explicit false-ish value as default, 0 would be I
think better than empty string '':

+our $missmatch_git = 0;


Second, there is question whether default should be to allow
mismatched versions (current behaviour, more lenient...) or deny (or
warn about) mismatched version, i.e. should it be $versions_must_match
false by default, or $allow_versions_mismatch false by default.

>  # Used to set the maximum load that we will still respond to gitweb queries.
>  # if we exceed this than we do the processing to figure out if there's a mirror
>  # and redirect to it, or to just return 503 server busy
> @@ -579,6 +582,25 @@ if (get_loadavg() > $maxload) {
>  our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
>  $number_of_git_cmds++;
>  
> +# There's a pretty serious flaw that we silently fail if git doesn't find something it needs
> +# a quick and simple check is to have gitweb do a simple check - are we running on the same
> +# version of git that we shipped with - if not, throw up an error so that people doing
> +# first installs don't have to debug perl to figure out whats going on

Could you please clean up language in above comment?  It is very
convoluted.  Please also limit line width of above comment to 76 / 80
columns.

> +if (
> +	$git_version ne $version
> +	&&
> +	$missmatch_git eq ''
> +){

Style

+if (!$allow_versions_mismatch &&
+    $git_version ne $version) {

Do not compare $missmatch_git / $allow_versions_mismatch against '':
it is a boolean value!

> +	git_header_html();

Shouldn't this be "500 Internal Server Error" or something (using the
optional parameter to git_header_html())?

> +	print "<p><b>*** Warning ***</b></p>\n";
> +	print "<p>\n";
> +	print "This version of gitweb was compiled for <b>$version</b> however git version <b>$git_version</b> was found<br/>\n";
> +	print "If you are sure this version of git works with this version of gitweb - please define <b>\$missmatch_git</b> to a non empty string in your git config file.\n";

Too long lines.  Here-doc could be better here.

> +	print "</p>\n";
> +	git_footer_html();
> +	exit;
> +}
> +
>  $projects_list ||= $projectroot;
>  
>  # ======================================================================

-- 
Jakub Narebski
Poland
ShadeHawk on #git