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

[Junio C Hamano <gitster@pobox.com>]

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

John, I'm sorry but I have to say this is somewhat incoherent.

> + * $git_versions_must_match
> +   If set to true value, gitweb fails with "500 Internal Server Error" error
> +   if the version of the gitweb doesn't match version of the git binary.
> +   Gitweb can usually run with a mismatched git install.   The default is 1
> +   (true).

I would understand that it if this were "Gitweb seldom runs correctly with
unmatched version of git, so this defaults to true".  If it can _usually_
run just fine, why should everybody need to flip this off?  This doesn't
make any sense to me.

> @@ -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)]}
> +(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);

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.

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

The message tells _you_ to consider using matching versions (as if _you_
have the choice and authority to do so), hints _you_ to decide if you are
Ok with running an unmatched combination (again, as if _you_ have the
authority to make that decision), and then instructs _you_ to contact the
server administrator (who presumably can flip the bit).

That doesn't make _any_ sense to me.  Hopefully anybody who installs or
upgrades gitweb/git will hit his gitweb installation at least once before
end users start hitting, so I would understand it if you wrote the above
message addressed to the server administrator.

If somebody updates his git without bothering to update gitweb, on the
other hand, the end user may see the message before the administrator
does.  If git and gitweb might be managed by different people at a
particular site (k.org?), I would understand that the administrator of the
gitweb side _might_ want to be told about it by the end user, and the
above might be an attempt to make that happen.

But even in that case, out of the three instructions, only the last one is
for the end user, and telling him to be certain the combinations do work
before bugging the gitweb administrator doesn't make much sense to me.

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

Even if it is to help a gitweb administrator who is not in charge of other
people in the administrator group who would install unmatching versions of
git without telling him, would this really be the best solution?  You'd be
the first to suffer from this when HPA or whoever installs a new version
of git at k.org.  There should be a better way to help communication
between the people in the administration group, without involving or
inconveniencing the end users like this patch seems to do.