gitweb: Use GITWEB_CONFIG_SYSTEM even if GITWEB_CONFIG does exist

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.

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

index 4964a67..0584919 100644
@@ -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

index a92bde7..334f13e 100644
@@ -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.
 

index acdc5b8..9527cd2 100755
@@ -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

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.

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?

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

Right.

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

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

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.

---

This version addresses this issue.

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

index 4964a67..0584919 100644
@@ -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

index a92bde7..334f13e 100644
@@ -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.
 

index e4ede13..0a23cb6 100755
@@ -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

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

... either way.

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

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.

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

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