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

From: Jakub Narebski <jnareb@gmail.com>
Date: Feb 2, 2010, 01:35 UTC
Message-ID: <201002020235.19943.jnareb@gmail.com>
In-Reply-To: <4B677971.2080100@eaglescrag.net>

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

Show 8 quoted lines

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

Show 10 quoted lines

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

Show 12 quoted lines

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