[PATCH] gitweb: Use GITWEB_CONFIG_SYSTEM even if GITWEB_CONFIG does exist

[PATCH] gitweb: Use GITWEB_CONFIG_SYSTEM even if GITWEB_CONFIG does exist

[Jakub Narebski]

gitweb obtains configuration from the following sources:

  1. per-instance configuration file (default: gitweb_conf.perl)
  2. system-wide configuration file (default: /etc/gitweb.conf)

If per-instance configuration file exists, then system-wide
configuration was _not used at all_.  This is quite untypical and
suprising behavior.

This commit changes gitweb behavior so that configuration in
per-instance configuration file can _override_ settings from
system-wide configuration file.

Suggested-by: Drew Northup <drew.northup@maine.edu>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is the response to discussion in the

  [PATCH/WIP] Starting work on a man page for /etc/gitweb.conf
  http://thread.gmane.org/gmane.comp.version-control.git/173422

The patch to gitweb.perl itself (without the commit message) was send
as part of 

  http://thread.gmane.org/gmane.comp.version-control.git/173422/focus=173489

(embedded in body of email).

Note that changes to gitweb/README and gitweb/INSTALL were
minimalized, so the final result might not be the best... but I think
it is good enough.

 gitweb/INSTALL     |    8 +++++---
 gitweb/README      |    2 +-
 gitweb/gitweb.perl |    7 ++++---
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/gitweb/INSTALL b/gitweb/INSTALL
index 4964a67..0584919 100644
--- a/gitweb/INSTALL
+++ b/gitweb/INSTALL
@@ -98,15 +98,17 @@ Gitweb config file
 See also "Runtime gitweb configuration" section in README file
 for gitweb (in gitweb/README).
 
-- You can configure gitweb further using the gitweb configuration file;
+- You can configure gitweb further using the per-instance gitweb configuration file;
   by default this is a file named gitweb_config.perl in the same place as
   gitweb.cgi script. You can control the default place for the config file
   using the GITWEB_CONFIG build configuration variable, and you can set it
-  using the GITWEB_CONFIG environment variable. If this file does not
-  exist, gitweb looks for a system-wide configuration file, normally
+  using the GITWEB_CONFIG environment variable.
+  gitweb also looks for a system-wide configuration file, normally
   /etc/gitweb.conf. You can change the default using the
   GITWEB_CONFIG_SYSTEM build configuration variable, and override it
   through the GITWEB_CONFIG_SYSTEM environment variable.
+  Settings from per-instance configuration file override those from
+  system-wide configuration file.
 
 - The gitweb config file is a fragment of perl code. You can set variables
   using "our $variable = value"; text from "#" character until the end
diff --git a/gitweb/README b/gitweb/README
index a92bde7..334f13e 100644
--- a/gitweb/README
+++ b/gitweb/README
@@ -126,7 +126,7 @@ Runtime gitweb configuration
 
 You can adjust gitweb behaviour using the file specified in `GITWEB_CONFIG`
 (defaults to 'gitweb_config.perl' in the same directory as the CGI), and
-as a fallback `GITWEB_CONFIG_SYSTEM` (defaults to /etc/gitweb.conf).
+`GITWEB_CONFIG_SYSTEM` (defaults to /etc/gitweb.conf), in that order.
 The most notable thing that is not configurable at compile time are the
 optional features, stored in the '%features' variable.
 
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index acdc5b8..9527cd2 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -637,12 +637,13 @@ sub evaluate_gitweb_config {
 	our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
 	our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++";
 	# die if there are errors parsing config file
+	if (-e $GITWEB_CONFIG_SYSTEM) {
+		do $GITWEB_CONFIG_SYSTEM;
+		die $@ if $@;
+	}
 	if (-e $GITWEB_CONFIG) {
 		do $GITWEB_CONFIG;
 		die $@ if $@;
-	} elsif (-e $GITWEB_CONFIG_SYSTEM) {
-		do $GITWEB_CONFIG_SYSTEM;
-		die $@ if $@;
 	}
 }
 
-- 
1.7.5

Re: [PATCH] gitweb: Use GITWEB_CONFIG_SYSTEM even if GITWEB_CONFIG does exist

[Jonathan Nieder]

Jakub Narebski wrote:

> If per-instance configuration file exists, then system-wide
> configuration was _not used at all_.  This is quite untypical and
> suprising behavior.

I agree.  How to avoid breaking existing installations, though?  (I'm
especially worried because distro packages tend to ship their own
/etc/gitweb.conf, so the admin might not even know about what's
there.)  For example, depending on the content of /etc/gitweb.conf,
this has the potential to break "git instaweb".

It could be simpler to document that users should put

	do $GITWEB_CONFIG_SYSTEM;

at the start of gitweb_config.conf to reuse options from the system
configuration file and override them.  But that's not very satisfying,
since I don't see a nice way to move to a better behavior after that
without breaking some existing installations.  (It would be possible
to check for a new ./gitweb-config-in-addition-to-what-was-in-etc.conf
file but that doesn't seem so nice.)

If this were a command-line tool, I would be happy as long as there is
some way to prevent reading /etc/gitweb.conf through the environment.
For a webapp I don't know how easy it is to set environment variables
typically.  So thoughts from people in that corner of the world would
be interesting.

Re: [PATCH] gitweb: Use GITWEB_CONFIG_SYSTEM even if GITWEB_CONFIG does exist

[Jakub Narebski]

On Sat, 14 May 2011, Jonathan Nieder wrote:
> Jakub Narebski wrote:
> 
> > If per-instance configuration file exists, then system-wide
> > configuration was _not used at all_.  This is quite untypical and
> > suprising behavior.
> 
> I agree.  How to avoid breaking existing installations, though?  (I'm
> especially worried because distro packages tend to ship their own
> /etc/gitweb.conf, so the admin might not even know about what's
> there.)  For example, depending on the content of /etc/gitweb.conf,
> this has the potential to break "git instaweb".

I don't think that this change has potential to break "git instaweb",
because "git instaweb" creates its own gitweb_conf.perl - settings
there would override distro's /etc/gitweb.conf.  But I have not checked
if it doesn't rely on some values being default; it shouldn't though.

It is a PITA to have to retain backward compatibility with our bugs
and mistakes.  Perhaps this change is for 1.8.0 version boundary, then?

> 
> It could be simpler to document that users should put
> 
> 	do $GITWEB_CONFIG_SYSTEM;

Well, you need to check if file exists, and die if there were any bugs
parsing this file (otherwise you can get strange errors which are really
hard to debug, to notice that they stem from broken configuration file;
I am speaking here from my bitter experience ;-)).

> 
> at the start of gitweb_config.conf to reuse options from the system
> configuration file and override them.  But that's not very satisfying,
> since I don't see a nice way to move to a better behavior after that
> without breaking some existing installations.  (It would be possible
> to check for a new ./gitweb-config-in-addition-to-what-was-in-etc.conf
> file but that doesn't seem so nice.)

Right.

> 
> If this were a command-line tool, I would be happy as long as there is
> some way to prevent reading /etc/gitweb.conf through the environment.
> For a webapp I don't know how easy it is to set environment variables
> typically.  So thoughts from people in that corner of the world would
> be interesting.

For Apache it is as simple as using

  SetEnv	GITWEB_CONFIG_SYSTEM	/dev/null

(Sidenote: I just noticed that if $GITWEB_CONFIG_SYSTEM eq $GITWEB_CONFIG,
then we don't need and should not re-read config file).

For Lighttpd it would be

  setenv.add-environment = ( "GITWEB_CONFIG_SYSTEM" => "/dev/null" )

For Mongoose it would be

  cgi_env	GITWEB_CONFIG_SYSTEM=/dev/null

For nginx it would be

  env		GITWEB_CONFIG_SYSTEM=/dev/null;

For IIS... is anyone running gitweb under IIS?


All examples taken from either gitweb/README, gitweb/INSTALL, or
git-instaweb.sh.  Instead of /dev/null it can be anything that
does not exist, but is not a false value (so "" won't work).

Not tested!
-- 
Jakub Narebski
Poland

[PATCHv2] gitweb: Use GITWEB_CONFIG_SYSTEM even if GITWEB_CONFIG does exist

[Jakub Narebski]

gitweb obtains configuration from the following sources:

  1. per-instance configuration file (default: gitweb_conf.perl)
  2. system-wide configuration file (default: /etc/gitweb.conf)

If per-instance configuration file exists, then system-wide
configuration is _not used at all_.  This is quite untypical and
suprising behavior.

This commit changes gitweb behavior so that configuration in
per-instance configuration file can _override_ settings from
system-wide configuration file.

Note that strictly speaking it is backwards incompatible change.
Check your resulting gitweb configuration, please.

Suggested-by: Drew Northup <drew.northup@maine.edu>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Jakub Narebski wrote:

> (Sidenote: I just noticed that if $GITWEB_CONFIG_SYSTEM eq $GITWEB_CONFIG,
> then we don't need and should not re-read config file).

This version addresses this issue.

 gitweb/INSTALL     |    8 +++++---
 gitweb/README      |    2 +-
 gitweb/gitweb.perl |   10 ++++++----
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/gitweb/INSTALL b/gitweb/INSTALL
index 4964a67..0584919 100644
--- a/gitweb/INSTALL
+++ b/gitweb/INSTALL
@@ -98,15 +98,17 @@ Gitweb config file
 See also "Runtime gitweb configuration" section in README file
 for gitweb (in gitweb/README).
 
-- You can configure gitweb further using the gitweb configuration file;
+- You can configure gitweb further using the per-instance gitweb configuration file;
   by default this is a file named gitweb_config.perl in the same place as
   gitweb.cgi script. You can control the default place for the config file
   using the GITWEB_CONFIG build configuration variable, and you can set it
-  using the GITWEB_CONFIG environment variable. If this file does not
-  exist, gitweb looks for a system-wide configuration file, normally
+  using the GITWEB_CONFIG environment variable.
+  gitweb also looks for a system-wide configuration file, normally
   /etc/gitweb.conf. You can change the default using the
   GITWEB_CONFIG_SYSTEM build configuration variable, and override it
   through the GITWEB_CONFIG_SYSTEM environment variable.
+  Settings from per-instance configuration file override those from
+  system-wide configuration file.
 
 - The gitweb config file is a fragment of perl code. You can set variables
   using "our $variable = value"; text from "#" character until the end
diff --git a/gitweb/README b/gitweb/README
index a92bde7..334f13e 100644
--- a/gitweb/README
+++ b/gitweb/README
@@ -126,7 +126,7 @@ Runtime gitweb configuration
 
 You can adjust gitweb behaviour using the file specified in `GITWEB_CONFIG`
 (defaults to 'gitweb_config.perl' in the same directory as the CGI), and
-as a fallback `GITWEB_CONFIG_SYSTEM` (defaults to /etc/gitweb.conf).
+`GITWEB_CONFIG_SYSTEM` (defaults to /etc/gitweb.conf), in that order.
 The most notable thing that is not configurable at compile time are the
 optional features, stored in the '%features' variable.
 
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e4ede13..0a23cb6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -640,13 +640,15 @@ sub evaluate_gitweb_config {
 	our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
 	our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++";
 	# die if there are errors parsing config file
-	if (-e $GITWEB_CONFIG) {
-		do $GITWEB_CONFIG;
-		die $@ if $@;
-	} elsif (-e $GITWEB_CONFIG_SYSTEM) {
+	if (-e $GITWEB_CONFIG_SYSTEM) {
 		do $GITWEB_CONFIG_SYSTEM;
 		die $@ if $@;
 	}
+	if ($GITWEB_CONFIG ne $GITWEB_CONFIG_SYSTEM &&
+	    -e $GITWEB_CONFIG) {
+		do $GITWEB_CONFIG;
+		die $@ if $@;
+	}
 }
 
 # Get loadavg of system, to compare against $maxload.
-- 
1.7.5

Re: [PATCHv2] gitweb: Use GITWEB_CONFIG_SYSTEM even if GITWEB_CONFIG does exist

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> This commit changes gitweb behavior so that configuration in
> per-instance configuration file can _override_ settings from
> system-wide configuration file.
>
> Note that strictly speaking it is backwards incompatible change.

I am not sure if we gain enough from this change.  As the system-wide one
can be arbitrarily overriden by per-instance one, the goal of this change
cannot be to make sure that the system administrator can enforce the
system wide policy over all the gitweb instances.

I think the goal is to let per-instance configuration have an easy way to
inherit from a common sane default, but if that is the case, wouldn't it
be a lot safer and more backward compatible way to just instruct people to
include that common default configuration at the beginning of per-instance
configuration file instead?  After all, you would need to give some advice
like this ...

> Check your resulting gitweb configuration, please.

... either way.

Re: [PATCHv2] gitweb: Use GITWEB_CONFIG_SYSTEM even if GITWEB_CONFIG does exist

[Drew Northup]

On Mon, 2011-05-16 at 22:32 -0700, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > This commit changes gitweb behavior so that configuration in
> > per-instance configuration file can _override_ settings from
> > system-wide configuration file.
> >
> > Note that strictly speaking it is backwards incompatible change.
> 
> I am not sure if we gain enough from this change.  As the system-wide one
> can be arbitrarily overriden by per-instance one, the goal of this change
> cannot be to make sure that the system administrator can enforce the
> system wide policy over all the gitweb instances.

Currently if there is a local configuration file
_there_is_no_system-wide_policy_. That's the problem.

> I think the goal is to let per-instance configuration have an easy way to
> inherit from a common sane default, but if that is the case, wouldn't it
> be a lot safer and more backward compatible way to just instruct people to
> include that common default configuration at the beginning of per-instance
> configuration file instead?  After all, you would need to give some advice
> like this ...

If that's currently being done anywhere this change will not cause any
pain. If the default /etc/gitweb.conf contains a set of commented-out
sample entries, as it should in the case of something like this, then
there will be no problems. As the local configurations will override the
system configuration items one by one, instead of wholesale and
outright, those items most likely to be a problem will be set correctly
in my estimation.

>From my day job I happen to know that asking the user to please respect
the existing policy by including it in their local configuration policy
doesn't work out very well. What usually happens is the user removes
whatever is there and replaces it with whatever the tutorial they are
currently looking at has in it. I highly doubt we'll change all of the
tutorials out on the Internet, or all of the mere mortals using them. We
can change Gitweb.

The difference between the current situation and the resultant one can
be summed up thusly:

Currently
/etc/gitweb.conf only: System policy
gitweb_config.perl only: Local policy
both: Local policy ONLY

New
/etc/gitweb.conf only: System policy
gitweb_config.perl only: Local policy
both: Local policy overrides System policy on a per-configurable item
basis

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [PATCHv2] gitweb: Use GITWEB_CONFIG_SYSTEM even if GITWEB_CONFIG does exist

[Junio C Hamano]

Drew Northup <drew.northup@maine.edu> writes:

> On Mon, 2011-05-16 at 22:32 -0700, Junio C Hamano wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>> 
>> > This commit changes gitweb behavior so that configuration in
>> > per-instance configuration file can _override_ settings from
>> > system-wide configuration file.
>> >
>> > Note that strictly speaking it is backwards incompatible change.
>> 
>> I am not sure if we gain enough from this change.  As the system-wide one
>> can be arbitrarily overriden by per-instance one, the goal of this change
>> cannot be to make sure that the system administrator can enforce the
>> system wide policy over all the gitweb instances.
>
> Currently if there is a local configuration file
> _there_is_no_system-wide_policy_. That's the problem.

As I said, the patch does not solve "system-wide policy" problem at all,
as the per-instance one can override anything. You _could_ sell this as a
convenience feature to me, and I already agreed it would be one possible
way to make things convienent.

>> I think the goal is to let per-instance configuration have an easy way to
>> inherit from a common sane default, but if that is the case, wouldn't it
>> be a lot safer and more backward compatible way to just instruct people to
>> include that common default configuration at the beginning of per-instance
>> configuration file instead?  After all, you would need to give some advice
>> like this ...
>
> If that's currently being done anywhere this change will not cause any
> pain. If the default /etc/gitweb.conf contains a set of commented-out
> sample entries, as it should in the case of something like this, then
> there will be no problems.

I could easily imagine a site whose primary objective of it is to support
a specific project. It would be natural for the sysadm group to decide to
use /etc/gitweb.conf to customize its primary gitweb instance to suite the
the need of serving that project.

The sysadm group may allow its users who have their own side projects run
their own instances using per-instance configuration. These users would
have been happily using their gitweb instances, with the knowledge that
anything specific to the primary project of the site in /etc/gitweb.conf
would not affect their instances.

These are broken with this change, no? The users of instaweb is also
affected the same way.

Although I am sympathetic to the wish to have a common configuration that
is shared by all instances by default (or even make it mandatory), I am
just pointing out that there is a downside if you use /etc/gitweb.conf as
the common configuration that is read by all instances.

Would it be a workable solution to make any instance read a new file, say,
/etc/gitweb-shared.conf, if it exists at the beginning, and to leave
/etc/gitweb.conf as the fallback default that is only used when
per-instance configuration does not exist?

If somebody can come up with a mechanism to mark configuration variables
that are set in the common configuration file as "not overridable", a
better name of the common file would be gitweb-policy.conf, as it does
more than "shared" whose purpose is to give common sane default.

I can go either way.

Re: [PATCH] gitweb: Use GITWEB_CONFIG_SYSTEM even if GITWEB_CONFIG does exist

[Drew Northup]

On Sun, 2011-05-15 at 11:53 +0200, Jakub Narebski wrote:
> On Sat, 14 May 2011, Jonathan Nieder wrote:
> > Jakub Narebski wrote:
> > 
> > > If per-instance configuration file exists, then system-wide
> > > configuration was _not used at all_.  This is quite untypical and
> > > suprising behavior.
> > 
> > I agree.  How to avoid breaking existing installations, though?  (I'm
> > especially worried because distro packages tend to ship their own
> > /etc/gitweb.conf, so the admin might not even know about what's
> > there.)  For example, depending on the content of /etc/gitweb.conf,
> > this has the potential to break "git instaweb".
> 
> I don't think that this change has potential to break "git instaweb",
> because "git instaweb" creates its own gitweb_conf.perl - settings
> there would override distro's /etc/gitweb.conf.  But I have not checked
> if it doesn't rely on some values being default; it shouldn't though.
> 
> It is a PITA to have to retain backward compatibility with our bugs
> and mistakes.  Perhaps this change is for 1.8.0 version boundary, then?

I'm comfortable putting this at the 1.8.0 boundary. Frankly, I think
that the default /etc/gitweb.conf should be a file full of commented-out
examples anyway. I do believe that at least one distro is doing that
already. Doing so decreases the potential pain for those still expecting
the old behavior.

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59