threads / patch / 27353

patchgitweb: Use GITWEB_CONFIG_SYSTEM even if GITWEB_CONFIG does exist

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

## tl;dr

8 messages between May 14, 2011 and May 17, 2011. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Jakub Narebski· May 14, 2011, 19:37 UTC · lore
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(-)
Show changes to 3 files +10 −7

gitweb/INSTALL, gitweb/README, gitweb/gitweb.perl

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
Jonathan Nieder· May 14, 2011, 21:06 UTC · re: Jakub Narebski · lore

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

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.

Jakub Narebski· May 15, 2011, 09:53 UTC · re: Jonathan Nieder · lore

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

On Sat, 14 May 2011, Jonathan Nieder wrote:
Show 11 quoted lines
> 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 ;-)).

Show 7 quoted lines
> 
> 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.
Show 6 quoted lines
> 
> 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
Jakub Narebski· May 16, 2011, 09:53 UTC · re: Jakub Narebski · lore

[PATCHv2] 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 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(-)
Show changes to 3 files +12 −8

gitweb/INSTALL, gitweb/README, gitweb/gitweb.perl

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
Junio C Hamano· May 17, 2011, 05:32 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 5 quoted lines
> 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.
Drew Northup· May 17, 2011, 15:19 UTC · re: Junio C Hamano · lore

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

On Mon, 2011-05-16 at 22:32 -0700, Junio C Hamano wrote:
Show 12 quoted lines
> 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.

Show 6 quoted lines
> 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
Junio C Hamano· May 17, 2011, 17:28 UTC · re: Drew Northup · lore

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

Drew Northup <drew.northup@maine.edu> writes:
Show 16 quoted lines
> 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.

Show 11 quoted lines
>> 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.
Drew Northup· May 16, 2011, 12:10 UTC · re: Jakub Narebski · lore

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

On Sun, 2011-05-15 at 11:53 +0200, Jakub Narebski wrote:
Show 20 quoted lines
> 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

← back to recent threads