[RFC/PATCH 0/4] config include directives

This series provides a way for config files to include other config
files in two ways:

  1. From other files in the filesystem. This is implemented by patch 1
     below, and is hopefully straightforward and uncontroversial.  See
     that patch for more rationale.

  2. From blobs in the repo. This is implemented by patch 4, with
     patches 2 and 3 providing the necessary refactoring. This
     is one way of implementing the often asked-for "respect shared
     config inside the repo" feature, but attempts to mitigate some of
     the security concerns. The interface for using it safely is a bit
     raw, but I think it's a sane building block, and somebody could
     write a fancier shared-config updater on top of it if they wanted
     to.

  [1/4]: config: add include directive
  [2/4]: config: factor out config file stack management
  [3/4]: config: support parsing config data from buffers
  [4/4]: config: allow including config from repository blobs

-Peff

It can be useful to split your ~/.gitconfig across multiple
files. For example, you might have a "main" file which is
used on many machines, but a small set of per-machine
tweaks. Or you may want to make some of your config public
(e.g., clever aliases) while keeping other data back (e.g.,
your name or other identifying information). Or you may want
to include a number of config options in some subset of your
repos without copying and pasting (e.g., you want to
reference them from the .git/config of participating repos).

This patch introduces an include directive for config files.
It looks like:

  [include]
    path = /path/to/file

This is syntactically backwards-compatible with existing git
config parsers (i.e., they will see it as another config
entry and ignore it unless you are looking up include.path).

The implementation provides a "git_config_include" callback
which wraps regular config callbacks.  Callers can pass it
to git_config_from_file, and it will transparently follow
any include directives, passing all of the discovered
options to the real callback.

Include directives are turned on for regular git config
parsing (i.e., when you call git_config()), as well as for
lookups via the "git config" program. They are not turned on
in other cases, including:

  1. Parsing of other config-like files, like .gitmodules.
     There isn't a real need, and I'd rather be conservative
     and avoid unnecessary incompatibility or confusion.

  2. Writing files via "git config"; we want to treat
     include.* variables as literal items to be copied (or
     modified), and not expand them. So "git config
     --unset-all foo.bar" would operate _only_ on
     .git/config, not any of its included files (just as it
     also does not operate on ~/.gitconfig).

Signed-off-by: Jeff King <redacted>
---
 Documentation/config.txt     |   15 ++++++
 Documentation/git-config.txt |    5 ++
 builtin/config.c             |   29 +++++++++---
 cache.h                      |    6 +++
 config.c                     |   58 +++++++++++++++++++++++++
 t/t1305-config-include.sh    |   98 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 204 insertions(+), 7 deletions(-)
 create mode 100755 t/t1305-config-include.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index abeb82b..e55dae1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -84,6 +84,17 @@ customary UNIX fashion.
 
 Some variables may require a special value format.
 
+Includes
+~~~~~~~~
+
+You can include one config file from another by setting the special
+`include.path` variable to the name of the file to be included. The
+included file is expanded immediately, as if its contents had been
+found at the location of the include directive. If the value of the
+`include.path` variable is a relative path, the path is considered to be
+relative to the configuration file in which the include directive was
+found. See below for examples.
+
 Example
 ~~~~~~~
 

@@ -106,6 +117,10 @@ Example
 		gitProxy="ssh" for "kernel.org"
 		gitProxy=default-proxy ; for the rest
 
+	[include]
+		path = /path/to/foo.inc ; include by absolute path
+		path = foo ; expand "foo" relative to the current file
+
 Variables
 ~~~~~~~~~
 

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e7ecf5d..aa8303b 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt

@@ -178,6 +178,11 @@ See also <<FILES>>.
 	Opens an editor to modify the specified config file; either
 	'--system', '--global', or repository (default).
 
+--includes::
+--no-includes::
+	Respect `include.*` directives in config files when looking up
+	values. Defaults to on.
+
 [[FILES]]
 FILES
 -----

diff --git a/builtin/config.c b/builtin/config.c
index d35c06a..9105f87 100644
--- a/builtin/config.c
+++ b/builtin/config.c

@@ -25,6 +25,7 @@ static const char *given_config_file;
 static int actions, types;
 static const char *get_color_slot, *get_colorbool_slot;
 static int end_null;
+static int respect_includes = 1;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)

@@ -74,6 +75,8 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "path", &types, "value is a path (file or directory name)", TYPE_PATH),
 	OPT_GROUP("Other"),
 	OPT_BOOLEAN('z', "null", &end_null, "terminate values with NUL byte"),
+	OPT_BOOLEAN(0, "includes", &respect_includes,
+		    "respect include directives on lookup"),
 	OPT_END(),
 };
 

@@ -161,6 +164,9 @@ static int get_value(const char *key_, const char *regex_)
 	int ret = -1;
 	char *global = NULL, *repo_config = NULL;
 	const char *system_wide = NULL, *local;
+	struct git_config_include_data inc;
+	config_fn_t fn;
+	void *data;
 
 	local = config_exclusive_filename;
 	if (!local) {

@@ -213,19 +219,28 @@ static int get_value(const char *key_, const char *regex_)
 		}
 	}
 
+	fn = show_config;
+	data = NULL;
+	if (respect_includes) {
+		inc.fn = fn;
+		inc.data = data;
+		fn = git_config_include;
+		data = &inc;
+	}
+
 	if (do_all && system_wide)
-		git_config_from_file(show_config, system_wide, NULL);
+		git_config_from_file(fn, system_wide, data);
 	if (do_all && global)
-		git_config_from_file(show_config, global, NULL);
+		git_config_from_file(fn, global, data);
 	if (do_all)
-		git_config_from_file(show_config, local, NULL);
-	git_config_from_parameters(show_config, NULL);
+		git_config_from_file(fn, local, data);
+	git_config_from_parameters(fn, data);
 	if (!do_all && !seen)
-		git_config_from_file(show_config, local, NULL);
+		git_config_from_file(fn, local, data);
 	if (!do_all && !seen && global)
-		git_config_from_file(show_config, global, NULL);
+		git_config_from_file(fn, global, data);
 	if (!do_all && !seen && system_wide)
-		git_config_from_file(show_config, system_wide, NULL);
+		git_config_from_file(fn, system_wide, data);
 
 	free(key);
 	if (regexp) {

diff --git a/cache.h b/cache.h
index 10afd71..21bbb0a 100644
--- a/cache.h
+++ b/cache.h

@@ -1138,6 +1138,12 @@ extern const char *get_commit_output_encoding(void);
 
 extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
 
+struct git_config_include_data {
+	config_fn_t fn;
+	void *data;
+};
+int git_config_include(const char *name, const char *value, void *vdata);
+
 extern const char *config_exclusive_filename;
 
 #define MAX_GITNAME (1000)

diff --git a/config.c b/config.c
index 40f9c6d..a6966c1 100644
--- a/config.c
+++ b/config.c

@@ -874,10 +874,68 @@ int git_config_system(void)
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
 }
 
+static int handle_path_include(const char *path, void *data)
+{
+	int ret = 0;
+	struct strbuf buf = STRBUF_INIT;
+
+	/*
+	 * Use an absolute value as-is, but interpret relative paths
+	 * based on the including config file.
+	 */
+	if (!is_absolute_path(path)) {
+		char *slash;
+		if (!cf)
+			return error("relative config includes must come from files");
+		strbuf_addstr(&buf, absolute_path(cf->name));
+		slash = find_last_dir_sep(buf.buf);
+		if (!slash)
+			die("BUG: no directory separator in an absolute path?");
+		strbuf_setlen(&buf, slash - buf.buf + 1);
+		strbuf_addf(&buf, "%s", path);
+		path = buf.buf;
+	}
+
+	if (!access(path, R_OK))
+		ret = git_config_from_file(git_config_include, path, data);
+	strbuf_release(&buf);
+	return ret;
+}
+
+int git_config_include(const char *name, const char *value, void *vdata)
+{
+	const struct git_config_include_data *data = vdata;
+	const char *type;
+	int ret;
+
+	/*
+	 * Pass along all values, including "include" directives; this makes it
+	 * possible to query information on the includes themselves.
+	 */
+	ret = data->fn(name, value, data->data);
+	if (ret < 0)
+		return ret;
+
+	if (prefixcmp(name, "include."))
+		return ret;
+	type = strrchr(name, '.') + 1;
+
+	if (!strcmp(type, "path"))
+		ret = handle_path_include(value, vdata);
+
+	return ret;
+}
+
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
 	int ret = 0, found = 0;
 	const char *home = NULL;
+	struct git_config_include_data inc;
+
+	inc.fn = fn;
+	inc.data = data;
+	fn = git_config_include;
+	data = &inc;
 
 	/* Setting $GIT_CONFIG makes git read _only_ the given config file. */
 	if (config_exclusive_filename)

diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
new file mode 100755
index 0000000..4db3091
--- /dev/null
+++ b/t/t1305-config-include.sh

@@ -0,0 +1,98 @@
+#!/bin/sh
+
+test_description='test config file include directives'
+. ./test-lib.sh
+
+test_expect_success 'include file by absolute path' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = \"$PWD/one\"" >base &&
+	echo 1 >expect &&
+	git config -f base test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'include file by relative path' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >base &&
+	echo 1 >expect &&
+	git config -f base test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'recursive relative paths' '
+	mkdir subdir &&
+	echo "[test]three = 3" >subdir/three &&
+	echo "[include]path = three" >subdir/two &&
+	echo "[include]path = subdir/two" >base &&
+	echo 3 >expect &&
+	git config -f base test.three >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'include options can still be examined' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >base &&
+	echo one >expect &&
+	git config -f base include.path >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'listing includes option and expansion' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >base &&
+	cat >expect <<-\EOF &&
+	include.path=one
+	test.one=1
+	EOF
+	git config -f base --list >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'writing config file does not expand includes' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >base &&
+	git config -f base test.two 2 &&
+	echo 2 >expect &&
+	git config -f base --no-includes test.two >actual &&
+	test_cmp expect actual &&
+	test_must_fail git config -f base --no-includes test.one
+'
+
+test_expect_success 'config modification does not affect includes' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >base &&
+	git config -f base test.one 2 &&
+	echo 1 >expect &&
+	git config -f one test.one >actual &&
+	test_cmp expect actual &&
+	cat >expect <<-\EOF &&
+	1
+	2
+	EOF
+	git config -f base --get-all test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'missing include files are ignored' '
+	cat >base <<-\EOF &&
+	[include]path = foo
+	[test]value = yes
+	EOF
+	echo yes >expect &&
+	git config -f base test.value >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'absolute includes from command line work' '
+	echo "[test]one = 1" >one &&
+	echo 1 >expect &&
+	git -c include.path="$PWD/one" config test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'relative includes from command line fail' '
+	echo "[test]one = 1" >one &&
+	test_must_fail git -c include.path=one config test.one
+'
+
+test_done

-- 
1.7.9.rc2.293.gaae2

Because a config callback may start parsing a new file, the
global context regarding the current config file is stored
as a stack. Currently we only need to manage that stack from
git_config_from_file. Let's factor it out to allow new
sources of config data.

Signed-off-by: Jeff King <redacted>
---
 config.c |   30 +++++++++++++++++++-----------
 1 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index a6966c1..b82f749 100644
--- a/config.c
+++ b/config.c

@@ -826,6 +826,23 @@ int git_default_config(const char *var, const char *value, void *dummy)
 	return 0;
 }
 
+static void config_file_push(config_file *top, const char *name)
+{
+	top->prev = cf;
+	top->f = NULL;
+	top->name = name;
+	top->linenr = 1;
+	top->eof = 0;
+	strbuf_init(&top->value, 1024);
+	cf = top;
+}
+
+static void config_file_pop(config_file *top)
+{
+	strbuf_release(&top->value);
+	cf = top->prev;
+}
+
 int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 {
 	int ret;

@@ -835,21 +852,12 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 	if (f) {
 		config_file top;
 
-		/* push config-file parsing state stack */
-		top.prev = cf;
+		config_file_push(&top, filename);
 		top.f = f;
-		top.name = filename;
-		top.linenr = 1;
-		top.eof = 0;
-		strbuf_init(&top.value, 1024);
-		cf = ⊤
 
 		ret = git_parse_file(fn, data);
 
-		/* pop config-file parsing state stack */
-		strbuf_release(&top.value);
-		cf = top.prev;
-
+		config_file_pop(&top);
 		fclose(f);
 	}
 	return ret;

-- 
1.7.9.rc2.293.gaae2

The only two ways to parse config data are from a file or
from the command-line. Because the command-line format is
totally different from the file format, they don't share any
code. Therefore, to add new sources of file-like config data,
we have to refactor git_parse_file to handle reading from
something besides stdio.

To fix this, our config_file structure now holds either a
"FILE *" pointer or a memory buffer. We intercept calls to
fgetc and ungetc and either pass them along to stdio, or
fake them with our buffer. This leaves the main parsing code
intact and easy to read.

Signed-off-by: Jeff King <redacted>
---
 cache.h  |    1 +
 config.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index 21bbb0a..a298897 100644
--- a/cache.h
+++ b/cache.h

@@ -1110,6 +1110,7 @@ extern int update_server_info(int);
 typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int git_default_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
+extern int git_config_from_buffer(config_fn_t fn, void *, const char *, char *, unsigned long );
 extern void git_config_push_parameter(const char *text);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
 extern int git_config(config_fn_t fn, void *);

diff --git a/config.c b/config.c
index b82f749..49a3d1a 100644
--- a/config.c
+++ b/config.c

@@ -18,6 +18,9 @@ typedef struct config_file {
 	const char *name;
 	int linenr;
 	int eof;
+	char *buf;
+	unsigned long size;
+	unsigned long cur;
 	struct strbuf value;
 	char var[MAXNAME];
 } config_file;

@@ -101,19 +104,45 @@ int git_config_from_parameters(config_fn_t fn, void *data)
 	return nr > 0;
 }
 
+static int get_one_char(void)
+{
+	if (cf->f)
+		return fgetc(cf->f);
+	else if (cf->buf) {
+		if (cf->cur < cf->size)
+			return cf->buf[cf->cur++];
+		return EOF;
+	}
+
+	die("BUG: attempt to read from NULL config_file");
+}
+
+static int unget_one_char(int c)
+{
+	if (cf->f)
+		ungetc(c, cf->f);
+	else if (cf->buf) {
+		if (cf->cur == 0)
+			return EOF;
+		cf->buf[--cf->cur] = c;
+		return c;
+	}
+
+	die("BUG: attempt to ungetc NULL config_file");
+}
+
 static int get_next_char(void)
 {
 	int c;
-	FILE *f;
 
 	c = '\n';
-	if (cf && ((f = cf->f) != NULL)) {
-		c = fgetc(f);
+	if (cf && (cf->f || cf->buf)) {
+		c = get_one_char();
 		if (c == '\r') {
 			/* DOS like systems */
-			c = fgetc(f);
+			c = get_one_char();
 			if (c != '\n') {
-				ungetc(c, f);
+				unget_one_char(c);
 				c = '\r';
 			}
 		}

@@ -833,6 +862,9 @@ static void config_file_push(config_file *top, const char *name)
 	top->name = name;
 	top->linenr = 1;
 	top->eof = 0;
+	top->buf = NULL;
+	top->size = 0;
+	top->cur = 0;
 	strbuf_init(&top->value, 1024);
 	cf = top;
 }

@@ -863,6 +895,22 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 	return ret;
 }
 
+int git_config_from_buffer(config_fn_t fn, void *data, const char *name,
+			   char *buf, unsigned long size)
+{
+	int ret;
+	config_file top;
+
+	config_file_push(&top, name);
+	top.buf = buf;
+	top.size = size;
+
+	ret = git_parse_file(fn, data);
+
+	config_file_pop(&top);
+	return ret;
+}
+
 const char *git_etc_gitconfig(void)
 {
 	static const char *system_wide;

-- 
1.7.9.rc2.293.gaae2

One often-requested feature is to allow projects to ship
suggested config to people who clone. The most obvious way
of implementing this would be to respect .gitconfig files
within the working tree. However, this has two problems:

  1. Because git configuration can cause the execution of
     arbitrary code, that creates a potential security problem.
     While you may be comfortable running "make" on a newly
     cloned project, you at least have the opportunity to
     inspect the downloaded contents.  But by automatically
     respecting downloaded git configuration, you cannot
     even safely use git to inspect those contents!

  2. Configuration options tend not to be tied to a specific
     version of the project. So if you are using "git
     checkout" to sight-see to an older revision, you
     probably still want to be using the most recent version
     of the suggested config.

Instead, this patch lets you include configuration directly
from a blob in the repository (using the usual object name
lookup rules). This avoids (2) by pointing directly to a tag
or branch tip. It is still possible to be dangerous as in
(1) above, but the danger can be avoided by not pointing
directly into remote blobs (and the documentation warns of
this and gives a safe example).

Signed-off-by: Jeff King <redacted>
---
 Documentation/config.txt  |   41 ++++++++++++++++++++++++++++++++++++++++-
 config.c                  |   25 ++++++++++++++++++++++++-
 t/t1305-config-include.sh |   38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e55dae1..38e83df 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -93,7 +93,14 @@ included file is expanded immediately, as if its contents had been
 found at the location of the include directive. If the value of the
 `include.path` variable is a relative path, the path is considered to be
 relative to the configuration file in which the include directive was
-found. See below for examples.
+found.
+
+You can also include configuration from a blob stored in your repository
+by setting the special `include.ref` variable to the name of an object
+containing your configuration data (in the same format as a regular
+config file).
+
+See below for examples.
 
 Example
 ~~~~~~~

@@ -120,6 +127,38 @@ Example
 	[include]
 		path = /path/to/foo.inc ; include by absolute path
 		path = foo ; expand "foo" relative to the current file
+		ref = config:.gitconfig ; look on "config" branch
+		ref = origin/master:.gitconfig ; this is unsafe! see below
+
+
+Security Considerations
+~~~~~~~~~~~~~~~~~~~~~~~
+
+Because git configuration may cause git to execute arbitrary shell
+commands, it is important to verify any configuration you receive over
+the network. In particular, it is not a good idea to point `include.ref`
+directly at a remote tracking branch like `origin/master:shared-config`.
+After a fetch, you have no way of inspecting the shared-config you have
+just received without running git (and thus respecting the downloaded
+config). Instead, you can create a local tag representing the last
+verified version of the config, and only update the tag after inspecting
+any new content.
+
+For example:
+
+	# initially, look at their suggested config
+	git show origin/master:shared-config
+
+	# if it looks good to you, point a local ref at it
+	git tag config origin/master
+	git config include.ref config:shared-config
+
+	# much later, fetch any changes and examine them
+	git fetch origin
+	git diff config origin/master -- shared-config
+
+	# If the changes look OK, update your local version
+	git tag -f config origin/master
 
 Variables
 ~~~~~~~~~

diff --git a/config.c b/config.c
index 49a3d1a..c41fb3b 100644
--- a/config.c
+++ b/config.c

@@ -941,7 +941,7 @@ static int handle_path_include(const char *path, void *data)
 	 */
 	if (!is_absolute_path(path)) {
 		char *slash;
-		if (!cf)
+		if (!cf || !cf->f)
 			return error("relative config includes must come from files");
 		strbuf_addstr(&buf, absolute_path(cf->name));
 		slash = find_last_dir_sep(buf.buf);

@@ -958,6 +958,27 @@ static int handle_path_include(const char *path, void *data)
 	return ret;
 }
 
+static int handle_ref_include(const char *ref, void *data)
+{
+	unsigned char sha1[20];
+	char *buf;
+	unsigned long size;
+	enum object_type type;
+	int ret;
+
+	if (get_sha1(ref, sha1))
+		return 0;
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("unable to read include ref '%s'", ref);
+	if (type != OBJ_BLOB)
+		return error("include ref '%s' is not a blob", ref);
+
+	ret = git_config_from_buffer(git_config_include, data, ref, buf, size);
+	free(buf);
+	return ret;
+}
+
 int git_config_include(const char *name, const char *value, void *vdata)
 {
 	const struct git_config_include_data *data = vdata;

@@ -978,6 +999,8 @@ int git_config_include(const char *name, const char *value, void *vdata)
 
 	if (!strcmp(type, "path"))
 		ret = handle_path_include(value, vdata);
+	else if (!strcmp(type, "ref"))
+		ret = handle_ref_include(value, vdata);
 
 	return ret;
 }

diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 4db3091..31d3b9b 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh

@@ -95,4 +95,42 @@ test_expect_success 'relative includes from command line fail' '
 	test_must_fail git -c include.path=one config test.one
 '
 
+test_expect_success 'include from ref' '
+	echo "[test]one = 1" >one &&
+	git add one &&
+	git commit -m one &&
+	rm one &&
+	echo "[include]ref = HEAD:one" >base &&
+	echo 1 >expect &&
+	git config -f base test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'relative file include from ref fails' '
+	echo "[test]two = 2" >two &&
+	echo "[include]path = two" >one &&
+	git add one &&
+	git commit -m one &&
+	echo "[include]ref = HEAD:one" >base &&
+	test_must_fail git config -f base test.two
+'
+
+test_expect_success 'non-existent include refs are ignored' '
+	cat >base <<-\EOF &&
+	[include]ref = my-missing-config-branch:foo.cfg
+	[test]value = yes
+	EOF
+	echo yes >expect &&
+	git config -f base test.value >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'non-blob include refs fail' '
+	cat >base <<-\EOF &&
+	[include]ref = HEAD
+	[test]value = yes
+	EOF
+	test_must_fail git config -f base test.value
+'
+
 test_done

-- 
1.7.9.rc2.293.gaae2

Am 1/26/2012 8:37, schrieb Jeff King:

This patch introduces an include directive for config files.

Nice. I haven't had a need for it, yet, but the concept looks good.

+test_expect_success 'recursive relative paths' '
+	mkdir subdir &&
+	echo "[test]three = 3" >subdir/three &&
+	echo "[include]path = three" >subdir/two &&
+	echo "[include]path = subdir/two" >base &&
+	echo 3 >expect &&
+	git config -f base test.three >actual &&
+	test_cmp expect actual
+'

Isn't it rather "chained relative paths"? Recursive would be if I write

  [include]path = .gitconfig

in my ~/.gitconfig. What happens in this case?

-- Hannes

Am 1/26/2012 8:42, schrieb Jeff King:

+static int handle_ref_include(const char *ref, void *data)
+{
+	unsigned char sha1[20];
+	char *buf;
+	unsigned long size;
+	enum object_type type;
+	int ret;
+
+	if (get_sha1(ref, sha1))
+		return 0;
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("unable to read include ref '%s'", ref);
+	if (type != OBJ_BLOB)
+		return error("include ref '%s' is not a blob", ref);
+
+	ret = git_config_from_buffer(git_config_include, data, ref, buf, size);
+	free(buf);
+	return ret;
+}

What happens if a ref cannot be resolved, for example due to repository
corruption? Does git just emit an error and then carries on, or does it
always die? Can I run at least git-fsck in such a case?

-- Hannes

On Thu, Jan 26, 2012 at 10:16:22AM +0100, Johannes Sixt wrote:

quoted

+test_expect_success 'recursive relative paths' '
+	mkdir subdir &&
+	echo "[test]three = 3" >subdir/three &&
+	echo "[include]path = three" >subdir/two &&
+	echo "[include]path = subdir/two" >base &&
+	echo 3 >expect &&
+	git config -f base test.three >actual &&
+	test_cmp expect actual
+'

Isn't it rather "chained relative paths"? Recursive would be if I write

  [include]path = .gitconfig

in my ~/.gitconfig. What happens in this case?

Good point. I used "recursive" because it is recursing in the include
function within git, but obviously from the user's perspective, it is
not a recursion.

And no, I didn't do any cycle detection. We could either do:

  1. Record some canonical name for each source we look at (probably
     realpath() for files, and the sha1 for refs), and don't descend
     into already-seen sources.

  2. Simply provide a maximum depth, and don't include beyond it.

The latter is much simpler to implement, but I think the former is a
little nicer for the user.

-Peff

On Thu, Jan 26, 2012 at 10:25:32AM +0100, Johannes Sixt wrote:

Am 1/26/2012 8:42, schrieb Jeff King:

quoted

+static int handle_ref_include(const char *ref, void *data)
+{
+	unsigned char sha1[20];
+	char *buf;
+	unsigned long size;
+	enum object_type type;
+	int ret;
+
+	if (get_sha1(ref, sha1))
+		return 0;
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("unable to read include ref '%s'", ref);
+	if (type != OBJ_BLOB)
+		return error("include ref '%s' is not a blob", ref);
+
+	ret = git_config_from_buffer(git_config_include, data, ref, buf, size);
+	free(buf);
+	return ret;
+}

What happens if a ref cannot be resolved, for example due to repository
corruption? Does git just emit an error and then carries on, or does it
always die? Can I run at least git-fsck in such a case?

Names which do not resolve are explicitly ignored, because I wanted to
flexibility in specifying the includes. E.g., you might say put:

  [include]
          ref = refs/config

in your ~/.gitconfig, and then only use the feature in some of your
repositories (I'm not sure if that is a good idea yet in practice or
not. But as I said before, I think of this is a building block, and I'd
like people to experiment and see if it fills their needs).

Obviously the trade-off is that we would silently ignore a typo in the
object name.

However, I did explicitly return an error for a failure to find a sha1,
or a non-blob sha1, as those are more severe configuration errors (where
the former is basically repository corruption). We only return an error
here, but git_config will eventually die() because of it, noting the
file and line number where the include happened[1].

It's then up to you to fix the config file before you can continue using
git. You can do so by hand, but I think using "git config" to do so will
not work; even though it correctly does not expand includes while
writing, the git wrapper incidentally reads the config before actually
running the config command.

We already have similar problems where setting a bool option to a
non-bool value will cause many git commands git to die (e.g., try
setting "color.ui" to "foo"). But it's less often an issue, because
unless the config option you have messed up is very basic (like
core.bare), you can still run "git config".

So it's certainly recoverable if you are comfortable editing the config
file. But we could also make it a little friendlier by turning those
errors into warnings, at the minor cost of making errors less
noticeable.

-Peff

[1] The error reporting just shows the source of the deepest file,
    because git_parse_file will actually call die(). So if I have a
    .git/config that includes a ref that includes another ref that has
    an error, I see only:

      $ git config foo.value
      error: include ref 'HEAD:subdir' is not a blob
      fatal: bad config file line 5 in HEAD:config

    But solving the problem without git is hard. I know the problem is
    in the ref, but I can't edit the ref. The source of the include
    chain has to come from a file I can edit outside of git (since we
    always start from the files), but I'm not told which file included
    it.

    So it would be a little nicer to say something like:

      error: include ref 'HEAD:subdir' is not a blob (at HEAD:config, line 5)
      error: included file 'HEAD:config' had errors at .git/config, line 9
      fatal: unable to parse configuration

    which shows the complete trail, and you know to edit .git/config. In
    practice, I don't know if it is much of an issue. There are only 3
    places that git actually reads config from, and it is likely that
    you just edited one.

On Thu, Jan 26, 2012 at 08:37, Jeff King [off-list ref] wrote:

This patch introduces an include directive for config files.
It looks like:

 [include]
   path = /path/to/file

Very nice, I'd been meaning to resurrect my gitconfig.d series, and
this series implements a lot of the structural changes needed for that
sort of thing.

What do you think of an option (e.g. include.gitconfig_d = true) that
would cause git to look in:

    /etc/gitconfig.d/*
    ~/.gitconfig.d/*
    .git/config.d/*

As well as the usual:

    /etc/gitconfig
    ~/.gitconfig
    .git/config

It would make including third-party config easy since you could just
symlink it in, and it would follow the convention of a lot of other
programs that have a foo and a foo.d directory.

On Fri, Jan 27, 2012 at 01:02:52AM +0100, Ævar Arnfjörð Bjarmason wrote:

On Thu, Jan 26, 2012 at 08:37, Jeff King [off-list ref] wrote:

quoted

This patch introduces an include directive for config files.
It looks like:

 [include]
   path = /path/to/file

Very nice, I'd been meaning to resurrect my gitconfig.d series, and
this series implements a lot of the structural changes needed for that
sort of thing.

Yeah, that seems like a reasonable thing to do. It could make life
easier for package managers (I think the only reason it has not come up
much is that there simply isn't a lot of third-party git config).

What do you think of an option (e.g. include.gitconfig_d = true) that
would cause git to look in:

    /etc/gitconfig.d/*
    ~/.gitconfig.d/*
    .git/config.d/*

Hmm. Is that really worth having an option? I.e., why not just always
check those directories?

I could see having

  [include]
        dir = /path/to/gitconfig.d

for non-standard directories, though (or perhaps even simpler, the
"path" directive should auto-detect a file versus a directory. Similarly
the "ref" form could detect and expand a tree).

-Peff

On Thu, Jan 26, 2012 at 4:25 PM, Johannes Sixt [off-list ref] wrote:

Am 1/26/2012 8:42, schrieb Jeff King:

quoted

+static int handle_ref_include(const char *ref, void *data)
+{
+     unsigned char sha1[20];
+     char *buf;
+     unsigned long size;
+     enum object_type type;
+     int ret;
+
+     if (get_sha1(ref, sha1))
+             return 0;
+     buf = read_sha1_file(sha1, &type, &size);
+     if (!buf)
+             return error("unable to read include ref '%s'", ref);
+     if (type != OBJ_BLOB)
+             return error("include ref '%s' is not a blob", ref);
+
+     ret = git_config_from_buffer(git_config_include, data, ref, buf, size);
+     free(buf);
+     return ret;
+}

What happens if a ref cannot be resolved, for example due to repository
corruption? Does git just emit an error and then carries on, or does it
always die? Can I run at least git-fsck in such a case?

Moreover, if I specify sha-1 in the config (it's discouraged but not
forbidden from the code), can git-prune remove the blob?
-- 
Duy

On Thu, Jan 26, 2012 at 2:42 PM, Jeff King [off-list ref] wrote:

+Security Considerations
+~~~~~~~~~~~~~~~~~~~~~~~
+
+Because git configuration may cause git to execute arbitrary shell
+commands, it is important to verify any configuration you receive over
+the network. In particular, it is not a good idea to point `include.ref`
+directly at a remote tracking branch like `origin/master:shared-config`.
+After a fetch, you have no way of inspecting the shared-config you have
+just received without running git (and thus respecting the downloaded
+config). Instead, you can create a local tag representing the last
+verified version of the config, and only update the tag after inspecting
+any new content.

It may be a good idea to tell users the ref include.ref points to has
been updated at the end of git-fetch. Showing a diff is even better.
-- 
Duy

On 01/26/2012 08:37 AM, Jeff King wrote:

[...]
This patch introduces an include directive for config files.
It looks like:

  [include]
    path = /path/to/file

I like it.

How about a short comment or two?

Doesn't this code accept all keys of the form "include\.(.*\.)?path"
(e.g., "include.foo.path")?  If that is your intention, then the
documentation should be fixed.  If not, then a single strcmp(name,
"include.path") would seem sufficient.

 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
 	int ret = 0, found = 0;
 	const char *home = NULL;
+	struct git_config_include_data inc;
+
+	inc.fn = fn;
+	inc.data = data;
+	fn = git_config_include;
+	data = &inc;
 
 	/* Setting $GIT_CONFIG makes git read _only_ the given config file. */
 	if (config_exclusive_filename)

The comment just after your addition should be adjusted, since now "the
given config file and any files that it includes" are read.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

On Fri, Jan 27, 2012 at 06:07:33AM +0100, Michael Haggerty wrote:

quoted

+struct git_config_include_data {
+	config_fn_t fn;
+	void *data;
+};
+int git_config_include(const char *name, const char *value, void *vdata);
+
 extern const char *config_exclusive_filename;
 
 #define MAX_GITNAME (1000)

How about a short comment or two?

I had originally planned to document this somewhat non-intuitive
interface in the config API documentation. But then I noticed we didn't
have such a document, and promptly forgot about documenting.

I'd rather have an API document, but I admit that the thought of
describing the config interface frightens me. It has some nasty corners.
But maybe starting one with the non-scary bits would be better, and then
I could add this to it.

quoted

+int git_config_include(const char *name, const char *value, void *vdata)
+{
+	const struct git_config_include_data *data = vdata;
+	const char *type;
+	int ret;
+
+	/*
+	 * Pass along all values, including "include" directives; this makes it
+	 * possible to query information on the includes themselves.
+	 */
+	ret = data->fn(name, value, data->data);
+	if (ret < 0)
+		return ret;
+
+	if (prefixcmp(name, "include."))
+		return ret;
+	type = strrchr(name, '.') + 1;
+
+	if (!strcmp(type, "path"))
+		ret = handle_path_include(value, vdata);
+
+	return ret;
+}
+

Doesn't this code accept all keys of the form "include\.(.*\.)?path"
(e.g., "include.foo.path")?  If that is your intention, then the
documentation should be fixed.  If not, then a single strcmp(name,
"include.path") would seem sufficient.

It does. I was considering (but haven't yet written) a patch that would
allow for conditional inclusion, like:

  [include "foo"]
          path = /some/file

where "foo" would be the condition. Specifically, I wanted to enable
includes when certain features were available in the parsing version of
git. For example, the pager.* variables were originally bools, but later
learned to take arbitrary strings. So my config with arbitrary strings
works on modern git, but causes earlier versions of git to barf. I'd
like to be able to do something like:

  [include "per-command-pager-strings"]
          path = /path/to/my/pager.config

where "per-command-pager-strings" would be a flag known internally to
git versions that support that feature.

I didn't end up implementing it right away, because of course those same
early versions of git also don't know about "include" at all. So using
any include effectively works as a conditional for that particular
feature. But as new incompatible config semantics are added
post-include, they could take advantage of a similar scheme.

So I wanted to leave the code open to adding such a patch later, if and
when it becomes useful. That being said, the code above is wrong.
For my scheme to work, versions of git that handle includes but don't
have the conditional-include patch (if it ever comes) would want to
explicitly disallow includes with subsections.

I'll fix it in the re-roll.

quoted

+	struct git_config_include_data inc;
+
+	inc.fn = fn;
+	inc.data = data;
+	fn = git_config_include;
+	data = &inc;
 
 	/* Setting $GIT_CONFIG makes git read _only_ the given config file. */
 	if (config_exclusive_filename)

The comment just after your addition should be adjusted, since now "the
given config file and any files that it includes" are read.

Will do.

-Peff

On Fri, Jan 27, 2012 at 10:47:29AM +0700, Nguyen Thai Ngoc Duy wrote:

quoted

What happens if a ref cannot be resolved, for example due to repository
corruption? Does git just emit an error and then carries on, or does it
always die? Can I run at least git-fsck in such a case?

Moreover, if I specify sha-1 in the config (it's discouraged but not
forbidden from the code), can git-prune remove the blob?

Yes. I don't think we want to get into connectivity guarantees for
config (because they can be quite complex, and involve files totally
outside the repo). I think it's OK for the user to be responsible for
either using a ref, or making sure that a bare sha1 they point to is
reachable from a ref.

-Peff

On Fri, Jan 27, 2012 at 11:01:00AM +0700, Nguyen Thai Ngoc Duy wrote:

On Thu, Jan 26, 2012 at 2:42 PM, Jeff King [off-list ref] wrote:

quoted

+Security Considerations
+~~~~~~~~~~~~~~~~~~~~~~~
+
+Because git configuration may cause git to execute arbitrary shell
+commands, it is important to verify any configuration you receive over
+the network. In particular, it is not a good idea to point `include.ref`
+directly at a remote tracking branch like `origin/master:shared-config`.
+After a fetch, you have no way of inspecting the shared-config you have
+just received without running git (and thus respecting the downloaded
+config). Instead, you can create a local tag representing the last
+verified version of the config, and only update the tag after inspecting
+any new content.

It may be a good idea to tell users the ref include.ref points to has
been updated at the end of git-fetch. Showing a diff is even better.

I really didn't want to have to let other parts of git know or care
about this mechanism. At least not for now. In the long run, I have no
problem with some porcelain growing up around the feature to make it
simpler to use. But I'd really rather focus on the bare-bones
functionality for now, see how people use it, and then find ways to
address deficiencies in their workflows once we have data.

-Peff

On Fri, Jan 27, 2012 at 01:32, Jeff King [off-list ref] wrote:

On Fri, Jan 27, 2012 at 01:02:52AM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

On Thu, Jan 26, 2012 at 08:37, Jeff King [off-list ref] wrote:

quoted

This patch introduces an include directive for config files.
It looks like:

 [include]
   path = /path/to/file

Very nice, I'd been meaning to resurrect my gitconfig.d series, and
this series implements a lot of the structural changes needed for that
sort of thing.

Yeah, that seems like a reasonable thing to do. It could make life
easier for package managers (I think the only reason it has not come up
much is that there simply isn't a lot of third-party git config).

quoted

What do you think of an option (e.g. include.gitconfig_d = true) that
would cause git to look in:

    /etc/gitconfig.d/*
    ~/.gitconfig.d/*
    .git/config.d/*

Hmm. Is that really worth having an option? I.e., why not just always
check those directories?

You're right, always just including those directories is a much better
option, an extra stat() doesn't cost us much.

Thanks again for working on this.

On Thu, Jan 26, 2012 at 08:35, Jeff King [off-list ref] wrote:

This series provides a way for config files to include other config
files in two ways:

 1. From other files in the filesystem. This is implemented by patch 1
    below, and is hopefully straightforward and uncontroversial.  See
    that patch for more rationale.

 2. From blobs in the repo. This is implemented by patch 4, with
    patches 2 and 3 providing the necessary refactoring. This
    is one way of implementing the often asked-for "respect shared
    config inside the repo" feature, but attempts to mitigate some of
    the security concerns. The interface for using it safely is a bit
    raw, but I think it's a sane building block, and somebody could
    write a fancier shared-config updater on top of it if they wanted
    to.

 [1/4]: config: add include directive
 [2/4]: config: factor out config file stack management
 [3/4]: config: support parsing config data from buffers
 [4/4]: config: allow including config from repository blobs

I expect you've thought about this, but our current API is (from
add.c):

	git_config(add_config, NULL);

Followed by:

    static int add_config(const char *var, const char *value, void *cb)
    {
    	if (!strcmp(var, "add.ignoreerrors") ||
    	    !strcmp(var, "add.ignore-errors")) {
    		ignore_add_errors = git_config_bool(var, value);
    		return 0;
    	}
    	return git_default_config(var, value, cb);
    }

I.e. that function gets called with one key at a time, and stashes it
to a local value.

If you write the function like that it means your patch series just
works since values encountered later will override earlier ones, but
have you checked git's code to make sure we don't have anything like:

    static int ignore_add_errors_is_set = 0;
    static int add_config(const char *var, const char *value, void *cb)
    {
    	if (!ignore_add_errors_is_set &&
            (!strcmp(var, "add.ignoreerrors") ||
    	     !strcmp(var, "add.ignore-errors"))) {
    		ignore_add_errors = git_config_bool(var, value);
            ignore_add_errors_is_set = 1;
    		return 0;
    	}
    	return git_default_config(var, value, cb);
    }

Which would mean that the include config support would be silently
ignored.

On Fri, Jan 27, 2012 at 10:51:34AM +0100, Ævar Arnfjörð Bjarmason wrote:

If you write the function like that it means your patch series just
works since values encountered later will override earlier ones, but
have you checked git's code to make sure we don't have anything like:

    static int ignore_add_errors_is_set = 0;
    static int add_config(const char *var, const char *value, void *cb)
    {
    	if (!ignore_add_errors_is_set &&
            (!strcmp(var, "add.ignoreerrors") ||
    	     !strcmp(var, "add.ignore-errors"))) {
    		ignore_add_errors = git_config_bool(var, value);
            ignore_add_errors_is_set = 1;
    		return 0;
    	}
    	return git_default_config(var, value, cb);
    }

Which would mean that the include config support would be silently
ignored.

I'm not sure what the issue is. If you write code like this, it will
already ignore the second invocation when it is found later in the same
file, or when it is found in a later file (i.e., in both .git/config and
.gitconfig). So I don't think includes introduce a new problem with
respect to code like this (and no, I didn't check exhaustively, but I
don't recall seeing code like this in git).

A bigger potential problem is multi-key values that form lists. For
example, I cannot use a later "remote.foo.url" line to override an
earlier one; instead, it gets appended to the list of URLs for "foo".
In practice, it's not a problem because the list-like options don't tend
to be found in multiple places. And again, this is not a new problem of
includes, since we already handle multiple files.

Accidentally including the same file twice would cause duplicates for
multi-key values. But I'm going to take Junio's suggestion to avoid
including the same file twice (which also prevents infinite loops due to
cycles).

-Peff