[PATCH 0/2] gitweb: Improve handling of configuration files

[PATCH 0/2] gitweb: Improve handling of configuration files

[Jakub Narebski]

This two-in-one series is response to Junio's concerns about being
backwards-incompatibile and different ways of solving "system-wide
policy" problem.

Patches 2/2 version A and 2/2 version B are mutually exclusive.
Junio, you would have to choose which one to include.

Drew, is the solution proposed in version A (making it easy to include
system-wide config in per-instance config) acceptable solution?

Table of contents:
~~~~~~~~~~~~~~~~~~
 [PATCH 1/2] gitweb: Refactor reading and parsing config file into
 [PATCHv1 2/2 (version A)] gitweb: Mention read_config_file in gitweb/README
 [PATCHv3 2/2 (version B)] gitweb: Use /etc/gitweb.conf even if gitweb_conf.perl exist

Shortlog:
~~~~~~~~~
Jakub Narebski (2):
  gitweb: Refactor reading and parsing config file into read_config_file
  gitweb: Mention read_config_file in gitweb/README

or

Jakub Narebski (2):
  gitweb: Refactor reading and parsing config file into read_config_file
  gitweb: Use /etc/gitweb.conf even if gitweb_conf.perl exist


Diffstat:
~~~~~~~~~
 gitweb/README      |   10 ++++++++++
 gitweb/gitweb.perl |   28 ++++++++++++++++++++--------
 2 files changed, 30 insertions(+), 8 deletions(-)

or

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

-- 
1.7.5

[PATCH 1/2] gitweb: Refactor reading and parsing config file into read_config_file

[Jakub Narebski]

Beside being obvious reduction of duplicated code, this is enables us
to easily call site-wide config file in per-installation config file.

The actual update to documentation is left for next commit, because of
possible exclusive alternative (possible other next commit) of always
reading system-wide config file and relying on per-instalation config
file overriding system-wide defaults.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I think it makes code more clear, both version A xor B ;-)

 gitweb/gitweb.perl |   28 ++++++++++++++++++++--------
 1 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e4ede13..ce92d67 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -635,18 +635,30 @@ sub filter_snapshot_fmts {
 # if it is true then gitweb config would be run for each request.
 our $per_request_config = 1;
 
+# read and parse gitweb config file given by its parameter.
+# returns true on success, false on recoverable error, allowing
+# to chain this subroutine, using first file that exists.
+# dies on errors during parsing config file, as it is unrecoverable.
+sub read_config_file {
+	my $filename = shift;
+	return unless defined $filename;
+	# die if there are errors parsing config file
+	if (-e $filename) {
+		do $filename;
+		die $@ if $@;
+		return 1;
+	}
+	return;
+}
+
 our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM);
 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) {
-		do $GITWEB_CONFIG_SYSTEM;
-		die $@ if $@;
-	}
+
+	# use first config file that exists
+	read_config_file($GITWEB_CONFIG) or
+	read_config_file($GITWEB_CONFIG_SYSTEM);
 }
 
 # Get loadavg of system, to compare against $maxload.
-- 
1.7.5

Re: [PATCH 1/2] gitweb: Refactor reading and parsing config file into read_config_file

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Beside being obvious reduction of duplicated code, this is enables us
> to easily call site-wide config file in per-installation config file.
>
> The actual update to documentation is left for next commit, because of
> possible exclusive alternative (possible other next commit) of always
> reading system-wide config file and relying on per-instalation config
> file overriding system-wide defaults.
>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>

As John acked this one and it looks obviously the right thing to do to me,
I'll apply this one directly to "master".

Thanks.

[PATCHv1 2/2 (version A)] gitweb: Mention read_config_file in gitweb/README

[Jakub Narebski]

Using

  read_config_file($GITWEB_CONFIG_SYSTEM);

at the very beginning of per-instance GITWEB_CONFIG file is quite
similar to e.g. using

  if [ -f /etc/bashrc ]; then
    . /etc/bashrc
  fi

in ~/.bashrc to read system-wide defaults.

This solution is backwards compatibile, but it requires to explicitly
request system-wide defaults... which otherwise are _not used at all_
when per-instance config file exists.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is alternate solution to "system-wide policy" problem, the other
way to make things convenient.  It was (soft of) suggested by Junio
in

  Re: [PATCHv2] gitweb: Use GITWEB_CONFIG_SYSTEM even if GITWEB_CONFIG does exist
  http://thread.gmane.org/gmane.comp.version-control.git/173603/focus=173815

While usually user's (here per-instance) configuration file overrides
system-wide configuration file settings (or in other words first
obtained value from user's and system-wide configuration file, in that
order, is used), like e.g. the case for ssh_config,... it is not
universal, as described in commit message.


This commit should be thought as exclusive to

  [PATCH 2/2 (version B)] gitweb: Use /etc/gitweb.conf even if gitweb_conf.perl exist

We have to choose one or the other.  Note that this other version
includes also change to gitweb/INSTALL...

 gitweb/README |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/gitweb/README b/gitweb/README
index a92bde7..ea10c91 100644
--- a/gitweb/README
+++ b/gitweb/README
@@ -127,6 +127,16 @@ 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).
+You can read defaults in system-wide GITWEB_CONFIG_SYSTEM from GITWEB_CONFIG
+by adding
+
+  read_config_file($GITWEB_CONFIG_SYSTEM);
+
+at very beginning of per-instance GITWEB_CONFIG file.  In this case
+settings in said per-instance file will override settings from
+system-wide configuration file.  Note that read_config_file checks
+itself that the $GITWEB_CONFIG_SYSTEM file exists.
+
 The most notable thing that is not configurable at compile time are the
 optional features, stored in the '%features' variable.
 
-- 
1.7.5

[PATCHv3 2/2 (version B)] gitweb: Use /etc/gitweb.conf even if gitweb_conf.perl 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 incompatibile change.
Check your resulting gitweb configuration, please.

Suggested-by: Drew Northup <drew.northup@maine.edu>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This commit is _*functionally*_ unchanged from previous version

  [PATCHv2] gitweb: Use GITWEB_CONFIG_SYSTEM even if GITWEB_CONFIG does exist
  http://thread.gmane.org/gmane.comp.version-control.git/173603/focus=173720

but it uses read_config_file() subroutine introduced in first patch.
This IMVHO makes gitweb code more clear.


This commit should be thought as exclusive to

  [PATCH 2/2 (version A)] gitweb: Mention read_config_file in gitweb/README

We have to choose one or the other.


 gitweb/INSTALL     |    8 +++++---
 gitweb/README      |    2 +-
 gitweb/gitweb.perl |    5 +++--
 3 files changed, 9 insertions(+), 6 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 ce92d67..e4b0932 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -656,9 +656,10 @@ sub evaluate_gitweb_config {
 	our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
 	our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++";
 
-	# use first config file that exists
-	read_config_file($GITWEB_CONFIG) or
+	# let settings in second override ones in first
 	read_config_file($GITWEB_CONFIG_SYSTEM);
+	read_config_file($GITWEB_CONFIG)
+		if ($GITWEB_CONFIG ne $GITWEB_CONFIG_SYSTEM);
 }
 
 # Get loadavg of system, to compare against $maxload.
-- 
1.7.5

Re: [PATCHv3 2/2 (version B)] gitweb: Use /etc/gitweb.conf even if gitweb_conf.perl exist

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> This commit should be thought as exclusive to
>
>   [PATCH 2/2 (version A)] gitweb: Mention read_config_file in gitweb/README
>
> We have to choose one or the other.

Who says?  We do not have to.  Why not do this instead?

	our $GITWEB_CONFIG_COMMON = $ENV{'GITWEB_CONFIG_COMMON'} ||
		"/etc/gitweb-common.conf";

	# common system-wide settings for convenience
        read_config_file($GITWEB_COMMON);

        # as always, use the per-instance GITWEB_CONFIG if exists,
        # otherwise use GITWEB_SYSTEM_CONFIG
        read_config_file($GITWEB_CONFIG) ||
        	read_config_file($GITWEB_CONFIG_SYSTEM);

There is no risk of hurting any existing installations, and people who do
have things that needs to be shared do not have to go around and update
all the per-instance configuration files.

 diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index ce92d67..e4b0932 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -656,9 +656,10 @@ sub evaluate_gitweb_config {
>  	our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
>  	our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++";
>  
> -	# use first config file that exists
> -	read_config_file($GITWEB_CONFIG) or
> +	# let settings in second override ones in first
>  	read_config_file($GITWEB_CONFIG_SYSTEM);
> +	read_config_file($GITWEB_CONFIG)
> +		if ($GITWEB_CONFIG ne $GITWEB_CONFIG_SYSTEM);
>  }
>  
>  # Get loadavg of system, to compare against $maxload.

[PATCH/RFC 2/2 (version C)] gitweb: Introduce common system-wide settings for convenience

[Jakub Narebski]

Because of backward compatibility we cannot change gitweb to always
use /etc/gitweb.conf (i.e. even if gitweb_config.perl exists).  For
common system-wide settings we therefore need new configuration
file: /etc/gitweb-common.conf.


Long description:

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

Moreover it is different from way git itself treats /etc/git.conf.  It
reads in stuff from /etc/git.conf and then local repos can change or
override things as needed.  In fact this is quite beneficial, because
it gives site admins a simple and easy way to give an automatic hint
to a repo about things the admin would like.

On the other hand changing current behavior may lead to the situation,
where something in /etc/gitweb.conf may interfere with unintended
interaction in the local repository.  One solution would be to
_require_ to do explicit include; with read_config_file() it is now
easy, as described in gitweb/README (description introduced in this
commit).

But as J.H. noticed we cannot ask people to modify their per-instance
gitweb config file to include system-wide settings, nor we can require
them to do this.

Therefore, as proposed by Junio, for gitweb to have centralized config
elements while retaining backwards compatibility, introduce separate
common system-wide configuration file, by default /etc/gitweb-common.conf

Noticed-by: Drew Northup <drew.northup@maine.edu>
Helped-by: John 'Warthog9' Hawley <warthog9@kernel.org>
Inspired-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
On Wed, 25 May 2011, Junio C Hamano <gitster@pobox.com> wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > This commit should be thought as exclusive to
> >
> >   [PATCH 2/2 (version A)] gitweb: Mention read_config_file in gitweb/README
> >
> > We have to choose one or the other.
> 
> Who says?  We do not have to.  Why not do this instead?
> 
> 	our $GITWEB_CONFIG_COMMON = $ENV{'GITWEB_CONFIG_COMMON'} ||
> 		"/etc/gitweb-common.conf";
> 
> 	# common system-wide settings for convenience
>         read_config_file($GITWEB_COMMON);
> 
>         # as always, use the per-instance GITWEB_CONFIG if exists,
>         # otherwise use GITWEB_SYSTEM_CONFIG
>         read_config_file($GITWEB_CONFIG) ||
>         	read_config_file($GITWEB_CONFIG_SYSTEM);
> 
> There is no risk of hurting any existing installations, and people who do
> have things that needs to be shared do not have to go around and update
> all the per-instance configuration files.

Here it is, version C.

It is currently marked as RFC for the following (minor) reasons:

1. In gitweb.perl we now have

  +	# Protect against duplications of file names, to not read config twice.
  +	# Only one of $GITWEB_CONFIG and $GITWEB_CONFIG_SYSTEM is used, so
  +	# there possibility of duplication of filename doesn't matter.
  +	$GITWEB_CONFIG = ""        if ($GITWEB_CONFIG eq $GITWEB_CONFIG_COMMON);
  +	$GITWEB_CONFIG_SYSTEM = "" if ($GITWEB_CONFIG_SYSTEM eq $GITWEB_CONFIG_COMMON);

to protect against double inclusion of configuration files.  It is
necessary?  Should we protect against such user errors?  Note that in
most cases second include wouldn't matter anyway...


2. To use first of per-instance and system wide configuration files we
now have in gitweb.perl

  	read_config_file($GITWEB_CONFIG) and return;
   	read_config_file($GITWEB_CONFIG_SYSTEM);

Is it more readable than

  	read_config_file($GITWEB_CONFIG) or
   	read_config_file($GITWEB_CONFIG_SYSTEM);

or correctly indented

  	read_config_file($GITWEB_CONFIG) or
   		read_config_file($GITWEB_CONFIG_SYSTEM);

or even

  	read_config_file($GITWEB_CONFIG)
		or
   			read_config_file($GITWEB_CONFIG_SYSTEM);


3. In includes changes to documentation, but does not include new
tests.  Should it?

 gitweb/INSTALL     |   13 ++++++++++++-
 gitweb/Makefile    |    2 ++
 gitweb/README      |   27 ++++++++++++++++++++++++---
 gitweb/gitweb.perl |   18 +++++++++++++++---
 4 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/gitweb/INSTALL b/gitweb/INSTALL
index 4964a67..a29ad0c 100644
--- a/gitweb/INSTALL
+++ b/gitweb/INSTALL
@@ -98,7 +98,7 @@ 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
@@ -108,6 +108,17 @@ for gitweb (in gitweb/README).
   GITWEB_CONFIG_SYSTEM build configuration variable, and override it
   through the GITWEB_CONFIG_SYSTEM environment variable.
 
+  Note that if per-instance configuration file exists, then system-wide
+  configuration is _not used at all_.  This is quite untypical and suprising
+  behavior.  On the other hand changing current behavior would break backwards
+  compatibility and can lead to unexpected changes in gitweb behavior.
+  Therefore gitweb also looks for common system-wide configuration file,
+  normally /etc/gitweb-common.conf (set during build time using build time
+  configuration variable GITWEB_CONFIG_COMMON, set it at runtime using
+  environment variable with the same name).  Settings from per-instance or
+  system-wide configuration file override those from common 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
   of a line is ignored. See perlsyn(1) for details.
diff --git a/gitweb/Makefile b/gitweb/Makefile
index 5d20515..1c85b5f 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -20,6 +20,7 @@ INSTALL ?= install
 # default configuration for gitweb
 GITWEB_CONFIG = gitweb_config.perl
 GITWEB_CONFIG_SYSTEM = /etc/gitweb.conf
+GITWEB_CONFIG_COMMON = /etc/gitweb-common.conf
 GITWEB_HOME_LINK_STR = projects
 GITWEB_SITENAME =
 GITWEB_PROJECTROOT = /pub/git
@@ -129,6 +130,7 @@ GITWEB_REPLACE = \
 	-e 's|++GIT_BINDIR++|$(bindir)|g' \
 	-e 's|++GITWEB_CONFIG++|$(GITWEB_CONFIG)|g' \
 	-e 's|++GITWEB_CONFIG_SYSTEM++|$(GITWEB_CONFIG_SYSTEM)|g' \
+	-e 's|++GITWEB_CONFIG_COMMON++|$(GITWEB_CONFIG_COMMON)|g' \
 	-e 's|++GITWEB_HOME_LINK_STR++|$(GITWEB_HOME_LINK_STR)|g' \
 	-e 's|++GITWEB_SITENAME++|$(GITWEB_SITENAME)|g' \
 	-e 's|++GITWEB_PROJECTROOT++|$(GITWEB_PROJECTROOT)|g' \
diff --git a/gitweb/README b/gitweb/README
index a92bde7..8743471 100644
--- a/gitweb/README
+++ b/gitweb/README
@@ -124,9 +124,30 @@ You can specify the following configuration variables when building GIT:
 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 obtains configuration data from the following sources in the
+following order:
+
+1. built-in values (some set during build stage),
+2. common system-wide configuration file (`GITWEB_CONFIG_COMMON`,
+   defaults to '/etc/gitweb-common.conf'),
+3. either per-instance configuration file (`GITWEB_CONFIG`, defaults to
+   'gitweb_config.perl' in the same directory as the installed gitweb),
+   or if it does not exists then system-wide configuration file
+   (`GITWEB_CONFIG_SYSTEM`, defaults to '/etc/gitweb.conf').
+
+Values obtained in later configuration files override values obtained earlier
+in above sequence.
+
+You can read defaults in system-wide GITWEB_CONFIG_SYSTEM from GITWEB_CONFIG
+by adding
+
+  read_config_file($GITWEB_CONFIG_SYSTEM);
+
+at very beginning of per-instance GITWEB_CONFIG file.  In this case
+settings in said per-instance file will override settings from
+system-wide configuration file.  Note that read_config_file checks
+itself that the $GITWEB_CONFIG_SYSTEM file exists.
+
 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 ce92d67..25a2618 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -651,13 +651,25 @@ sub read_config_file {
 	return;
 }
 
-our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM);
+our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM, $GITWEB_CONFIG_COMMON);
 sub evaluate_gitweb_config {
 	our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
 	our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++";
+	our $GITWEB_CONFIG_COMMON = $ENV{'GITWEB_CONFIG_COMMON'} || "++GITWEB_CONFIG_COMMON++";
 
-	# use first config file that exists
-	read_config_file($GITWEB_CONFIG) or
+	# Protect against duplications of file names, to not read config twice.
+	# Only one of $GITWEB_CONFIG and $GITWEB_CONFIG_SYSTEM is used, so
+	# there possibility of duplication of filename doesn't matter.
+	$GITWEB_CONFIG = ""        if ($GITWEB_CONFIG eq $GITWEB_CONFIG_COMMON);
+	$GITWEB_CONFIG_SYSTEM = "" if ($GITWEB_CONFIG_SYSTEM eq $GITWEB_CONFIG_COMMON);
+
+	# Common system-wide settings for convenience.
+	# Those settings can be ovverriden by GITWEB_CONFIG or GITWEB_CONFIG_SYSTEM.
+	read_config_file($GITWEB_CONFIG_COMMON);
+
+	# Use first config file that exists.  This means use the per-instance
+	# GITWEB_CONFIG if exists, otherwise use GITWEB_SYSTEM_CONFIG.
+	read_config_file($GITWEB_CONFIG) and return;
 	read_config_file($GITWEB_CONFIG_SYSTEM);
 }
 
-- 
1.7.5

Re: [PATCH 0/2] gitweb: Improve handling of configuration files

[J.H.]

On 05/25/2011 09:35 AM, Jakub Narebski wrote:
> This two-in-one series is response to Junio's concerns about being
> backwards-incompatibile and different ways of solving "system-wide
> policy" problem.

I realize I'm jumping on this mid way through, but I did read back
through the history.

Start with Patch 1, it looks fine and I support the change.  Makes it a
lot easier to include config files.

> Patches 2/2 version A and 2/2 version B are mutually exclusive.
> Junio, you would have to choose which one to include.

To speak to Junio's concerns seem to boil down to a general concern that
something in /etc/gitweb.conf may interfere with unintended interaction
in the local repository, and has suggested that we do an explicit
include to solve the problem.

I would argue against this, for a couple of reasons:

1) Git itself treats /etc/git.conf in this exact manor, it reads in
stuff from /etc/git.conf and then local repos can change or override
things as needed.  In fact this is quite beneficial, because it gives
site admins a simple and easy way to give an automatic hint to a repo
about things the admin would like.

Case in point turning on things like updateserverinfo, or automatic packing.

The fact that an admin doesn't need to do anything for this change to be
effective in most places is actually the expected result of the
sysadmin.  The systems administrator is saying "hey we need to have this
on everywhere and instead of getting 100 people to all change their
configs, we make one change and the hint is automatically everywhere".

>From a kernel.org perspective, that's what I'd expect.  I'm not keen on
having to go and either add some include line to each config file for
gitweb config (which feels like a kludge) just to centralize config
elements.

2) Trying to get people to do includes in their config files, or to
completely ignore the system wide config isn't a sustainable practice.
It would be akin to asking that every repository on kernel.org add
something to their config, which each individual person has to go and
add on their own.

That's akin to asking 400 people to make a change to 900+ repositories,
and then to remember to make that change for every new repo that gets
created.

Now I don't have 900 instances of gitweb running, but I am up to at
least 3 independent instances, and I can think of a 4th that is in the
works.  Having a system-wide config that gives the basics is appealing
to me.

Now that I've said all that, here's what I propose.

We go back to the way Jakub proposed this originally, where it reads in
the system wide config, and then overwrites things from the local
config.  This really is more in line with what people, particularly
system wide sysadmins, will expect.

To alleviate the concerns of people who thing the systemwide may cause
problems I propose, instead of an explicit include, we do an explicit
exclude for the system wide config.  We have a new config directive that
basically says throw out the systemwide config, take me back to defaults
and let me override from there.  This means needing to either store a
copy of the defaults on start-up, or like with gitweb-caching the
defaults are kept in a separate file and can just be re-read, and it
means that the local config would need to be read twice.

This means that if the individual wants to ignore the hints the systems
administrator is giving in system wide config, then they are making an
explicit choice to do so, either because they know better or something
like git-instaweb where it generally doesn't trust it.  Either way, it's
the exceptions who are making the choice, not the general case.

I'll admit this might even convince me that a single configuration
structure for the entire script is better than individual variable names.

- John 'Warthog9' Hawley

Re: [PATCH 0/2] gitweb: Improve handling of configuration files

[Junio C Hamano]

"J.H." <warthog9@kernel.org> writes:

> I would argue against this, for a couple of reasons:
>
> 1) Git itself treats /etc/git.conf in this exact manor,...
> Case in point turning on things like updateserverinfo, or automatic packing.

If we read both from the beginning of time it would have been more
convenient, and I agree with that 100%. So this is not a valid reason to
"argue against this".

> 2) Trying to get people to do includes in their config files, or to
> completely ignore the system wide config isn't a sustainable practice.

I also agree with this.

In any case, I do not think we have to choose between Jakub's two patches.