Re: [PATCH 8/8 v6] gitweb: Add an option to force version match

[Jakub Narebski <jnareb@gmail.com>]

On Mon, 1 Feb 2010, at 17:01:37 -0800, J.H. wrote:

> Starting to pop off the stack, and this came up first.  A quick reading
> of this, I'd sign-off and agree to patches 1-7 completely.
> 
> I'm still going to take issue that this being off by default is the
> wrong behavior and leaving this off by default more or less means that
> it will never get run and it becomes useless code.  If this isn't on by
> default, it shouldn't be committed, as I can't think of a legitimate use
> case where an admin is going to turn this on.

Well, I don't think that mismatched git and gitweb version should be
serious problem in practice, unless they are seriously out of sync.  
And in such situation (where either git is stale and gitweb updated,
or git updated and gitweb kept stale e.g. because it is heavily 
customized with not ported changes) gitweb admin should turn this
feature on.

> 
> I'd still argue this needs to be on by default to at least give admins
> the explicit warning that if they want to deviate they are taking their
> own risks, and that gitweb might not run as expected.  Once the warning
> is disabled in a configuration file it's not like it's going to be
> re-enabled.  People loading gitweb from their distro's package
> management will likely be in sync properly and will never see this.
> Those who are installing gitweb independent of their distro's package
> management will at least be warned of the risk, at least until better
> error reporting is done and in gitweb.

If you want to have it turned on by default (which is _incompatible_
change, and which was not announced enough, I think; it might mean
that gitweb can stop working after git or gitweb update), beside
changing commit message and gitweb/README (and of course definition of
$git_versions_must_match variable), you would have also update 
explanation in die_error for version mismatch.

Current version, as requested by Petr 'Pasky' Baudis, explains how
to turn feature off if it was enabled.  For this it needs to check
which config file is present, but we know at least that some config
file had to be used (beside possibility of hand-editing gitweb.cgi).
This is not the case if this feature is turned on by default: there
is possible that there is no gitweb config file, and all configuration
was done at build time.  How to explain then how to turn this feature
off?  What happens if both $GIWEB_CONFIG and $GITWEB_CONFIG_SYSTEM
are empty because of misconfiguration during build ($GITWEB_CONFIG
is set by default to gitweb_config.perl, and $GITWEB_CONFIG_SYSTEM
is set by default to /etc/gitweb.conf)?  Which one to recommend if both
variables are set, but neither file exists?

The difficulty of explanation how to turn this feature off if it is
on by default was one of main reasons to not have it turned on by
default.


Side-note: Perhaps error is too strong a measure, and it would be better
to just issue warning somewhere instead?

> 
> I've got a slightly modified version of this that re-enables it by
> default, it passes t9501 for me.

As it should, because gitweb-lib.sh was modified to explicitly turn off
this feature (see below), to allow testing gitweb without need to 
recompile whole git, and to allow testing _not installed_ (source)
version of gitweb.

> > diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh
> > index 5a734b1..66a3e2d 100755
> > --- a/t/gitweb-lib.sh
> > +++ b/t/gitweb-lib.sh
> > @@ -26,6 +26,7 @@ our \$projects_list = '';
> >  our \$export_ok = '';
> >  our \$strict_export = '';
> >  our \$maxload = undef;
> > +our \$git_versions_must_match = 0;
> >  
> >  EOF
> >  

-- 
Jakub Narebski
Poland