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

From: Jakub Narebski <jnareb@gmail.com>
Date: May 26, 2011, 14:58 UTC
Message-ID: <201105261658.20140.jnareb@gmail.com>
In-Reply-To: <7v8vtu4nmy.fsf@alter.siamese.dyndns.org>

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:

Show 24 quoted lines

> 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