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

[Jakub Narebski <jnareb@gmail.com>]

Junio C Hamano <gitster@pobox.com> writes:

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

> > @@ -583,6 +586,33 @@ sub get_loadavg {
> >  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;
> > +<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.
> > +Running an instance of gitweb that is not matched to the git binaries may
> > +result in unexpected behavior of gitweb, and loss of functionality or
> > +incorrect data on displayed pages.
> > +</p>
> > +<p>
> > +Please update the git or gitweb installation so that their versions match, or
> > +if you feel you are sure that you wish to proceed with running gitweb
> > +with unmatched versions please contact the server administrator${admin_contact}
> > +to configure gitweb to allow mismatched versions.  This can be done by
> > +setting \$git_versions_must_match to @{[esc_html($git_versions_must_match)]}

Errr... the above line does not makes sense, as $git_versions_must_match
is true (true value), and not false (false value). so you would get something
like:

  'setting $git_versions_must_match to 1 (false value) [...]'

> > +(false value) in gitweb configuration file,
> > +'@{[esc_path(-e $GITWEB_CONFIG ? $GITWEB_CONFIG : $GITWEB_CONFIG_SYSTEM)]}'.
> > +</p>
> > +EOT
> > +	die_error(500, 'Internal server error', $err_msg);

As I wrote in my earlier response, this is not enough.  If this error
message (description of this situation) is meant to describe how to turn
this feature off, it has to deal with situation where config file does
not exist.  With this feature off by default we knew that it had to be
changed in some gitweb config file, so one of $GITWEB_CONFIG and
$GITWEB_CONFIG_SYSTEM must exists.  This is not true if this feature is
turned on by default.

You need to describe how to deal with the following situations:

* Both $GITWEB_CONFIG and $GITWEB_CONFIG_SYSTEM are empty or undefined.
  You might skip this situation, as by default both are set by
  gitweb/Makefile, $GITWEB_CONFIG to gitweb_config.perl and
  $GITWEB_CONFIG_SYSTEM to /etc/gitweb.conf
 
* Both $GITWEB_CONFIG and $GITWEB_CONFIG_SYSTEM are set and non-empty,
  but neither file exists (this means that $projects_list is set during
  build stage).

Current code deals correctly only with situation where either
$GITWEB_CONFIG or $GITWEB_CONFIG_SYSTEM exists, and it further assumes
that neither is undefined (you would get perl errors in case if either
is undefined).

> Why, why, why?
> 
> This is not even a "*** Warning ***".  You are refusing to let them do
> anything useful until they either flip the bit off or reinstall git and/or
> gitweb.  It is a _fatal error_ message.

Ooops.

> 
> To whom are you giving this _warning_?  Please read the message yourself
> again.

The original message (from "Gitweb caching v2" thread) was ment purely
for server administrator.  Current version tries to address both
ordinary user (which has to contact gitweb administrator) and gitweb
administrator (who needs to know how to remote error condition, either
by bringing git and gitweb versions in sync, or by changing
configuration).  And does it badly...

[...]
> So I have to ask a basic question I asked (at least I tried to) last night
> again.  Whom are you trying to help?
[...]

-- 
Jakub Narebski
Poland
ShadeHawk on #git