[RFC/PATCH 0/4] config include directives

[RFC/PATCH 0/4] config include directives

[Jeff King]

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

[PATCH 1/4] config: add include directive

[Jeff King]

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 <peff@peff.net>
---
 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

Re: [PATCH 1/4] config: add include directive

[Johannes Sixt]

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

Re: [PATCH 1/4] config: add include directive

[Jeff King]

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

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

Re: [PATCH 1/4] config: add include directive

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Another thing I wondered after reading this patch was that it will be a
rather common "mistake" to include the same file twice, one in ~/.gitconfig
and then another in project specific .git/config, or more likely, people
start using useful ones in ~their/.gitconfig, and then the site administrator
by popular demand adding the same include in /etc/gitconfig to retroactively
cause the same file included twice for them.

Your first alternative solution should solve this case nicely as well, I
would think.

Re: [PATCH 1/4] config: add include directive

[Jeff King]

On Thu, Jan 26, 2012 at 12:42:28PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Another thing I wondered after reading this patch was that it will be a
> rather common "mistake" to include the same file twice, one in ~/.gitconfig
> and then another in project specific .git/config, or more likely, people
> start using useful ones in ~their/.gitconfig, and then the site administrator
> by popular demand adding the same include in /etc/gitconfig to retroactively
> cause the same file included twice for them.
> 
> Your first alternative solution should solve this case nicely as well, I
> would think.

Agreed. I'll implement that, then.

There's a slight complication with when to clear the "I have seen this"
mark for each source. If you are interested only in breaking cycles,
then obviously you can just clear the marks as you pop the stack. But if
you want to stop repeated inclusion down different branches of the
include tree, you need to keep the marks until you're done (e.g., the
same file referenced by .git/config and ~/.gitconfig). But you do still
need to clear them, because we repeatedly call git_config with different
callback functions, and we need to re-parse each time.

I think it should be sufficient to make the marks live through
git_config(), and get cleared after that (so all of .git/config,
~/.gitconfig, and /etc/gitconfig will only load a given include once,
but another call to git_config will load it again).

-Peff

Re: [PATCH 1/4] config: add include directive

[Jeff King]

On Thu, Jan 26, 2012 at 05:25:32PM -0500, Jeff King wrote:

> There's a slight complication with when to clear the "I have seen this"
> mark for each source. If you are interested only in breaking cycles,
> then obviously you can just clear the marks as you pop the stack. But if
> you want to stop repeated inclusion down different branches of the
> include tree, you need to keep the marks until you're done (e.g., the
> same file referenced by .git/config and ~/.gitconfig). But you do still
> need to clear them, because we repeatedly call git_config with different
> callback functions, and we need to re-parse each time.
> 
> I think it should be sufficient to make the marks live through
> git_config(), and get cleared after that (so all of .git/config,
> ~/.gitconfig, and /etc/gitconfig will only load a given include once,
> but another call to git_config will load it again).

Ugh, actually it's a little trickier. Because a git_config callback can
call git_config again, we want one set of marks per git_config
invocation, but we need to handle two simultaneous invocations. But I
think it's OK. We can stuff the data into the "git_config_include_data",
which has the proper scope.

-Peff

Re: [PATCH 1/4] config: add include directive

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

This is a good design decision, I think, but I am not sure where this "we
do not let gitmodules to honor inclusion" is implemented in this patch. I
would have expected a patch to "git-submodule.sh" that adds --no-includes
where it invokes "git config"?

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

I cannot offer better wording, but the first reaction I had to this was
"Eh, if I include a file A that includes another file B, to which config
file the '[include] path = B' directive found in file A relative to?"

Logically it should be relative to A, and I think your implementation
makes it so, but the above "as if its contents had been found at..."
sounds as if it should be the same as writing '[include] path = B' in the
original config file that included A.

By the way, I was a bit surprised that you did not have to add the source
stacking to git_config_from_file().

It was added in 924aaf3 (config.c: Make git_config() work correctly when
called recursively, 2011-06-16) to address the situation where
git_config() ends up calling git_config() recursively. My gut feeling is
that logically that issue shouldn't be limited to configuration that is
coming from file, which in turn makes me suspect that the source stacking
may be better in one level higher than git_config_from_file() so that the
ones coming from parameters could also be treated as a different kind of
source to read configuration from.

But in practice, including from the command line (i.e. -c include.path=...)
is somewhat crazy and we probably would not want to support it, so...

Re: [PATCH 1/4] config: add include directive

[Jeff King]

On Thu, Jan 26, 2012 at 12:58:25PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> This is a good design decision, I think, but I am not sure where this "we
> do not let gitmodules to honor inclusion" is implemented in this patch. I
> would have expected a patch to "git-submodule.sh" that adds --no-includes
> where it invokes "git config"?

My goal was to leave git_config_from_file untouched, so that the code in
submodule.c:gitmodules_config would work without modification. And that
works, obviously.

However, I didn't think about the fact that git-submodule.sh would be
calling git-config separately, and that is accidentally changed by my
patch.  Even if we changed git-submodule to use "git config
--no-includes" that would break any third-party scripts that use "git
config" to read git-like files.

But it would be nice for callers doing "git config foo.bar" to get the
includes by default. So maybe the right rule is:

  1. In C:
     a. git_config() respects includes automatically.
     b. other callers do not do so automatically (e.g., gitmodules via
        submodule.c).

    (i.e., what is implemented by this patch)

  2. Callers of git-config:
     a. respect includes for lookup that checks all of the "normal"
        config spots in sequence: .git/config, ~/.gitconfig, and
        /etc/gitconfig. These are the shell equivalent of calling
        git_config().

     b. when we are looking in a specific file (via GIT_CONFIG or "git
        config -f"), do not respect includes (but allow --includes if
        the caller chooses). This specific file may be something like
        .gitmodules. Or perhaps somebody is saying "no, I really just
        want to know what is in _this_ file, not what the config
        ecosystem tells me in general".

And then because of 1a and 2a, most programs should Just Work without
any changes, but because of 1b and 2b, any special uses will have to
decide manually whether they would want to allow includes.

Does that make sense?

> > +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.
> 
> I cannot offer better wording, but the first reaction I had to this was
> "Eh, if I include a file A that includes another file B, to which config
> file the '[include] path = B' directive found in file A relative to?"
> 
> Logically it should be relative to A, and I think your implementation
> makes it so, but the above "as if its contents had been found at..."
> sounds as if it should be the same as writing '[include] path = B' in the
> original config file that included A.

Yes, it is relative to A. Anything else would be insane (because it
would mean "A" has to care about who is including it).

I had a similar thought while writing it, but hoped the sentence after
(that you snipped) would make it clear. But I see the "as if..." can be
ambiguous (because it's only mostly "as if"). I started trying to write
something like "with the exception of further includes, which..." but I
think that is just getting more confusing.

How about:

  The included file is processed immediately, before any other
  directives from the surrounding file.

What I wanted to make clear there is the ordering, which sometimes
matters.

> By the way, I was a bit surprised that you did not have to add the source
> stacking to git_config_from_file().
> 
> It was added in 924aaf3 (config.c: Make git_config() work correctly when
> called recursively, 2011-06-16) to address the situation where
> git_config() ends up calling git_config() recursively.

Yeah. I remembered discussing that patch with Ramsay a few months ago,
and I was pleased that it just worked in this case.

> My gut feeling is that logically that issue shouldn't be limited to
> configuration that is coming from file, which in turn makes me suspect
> that the source stacking may be better in one level higher than
> git_config_from_file() so that the ones coming from parameters could
> also be treated as a different kind of source to read configuration
> from.

I do factor it out in the second patch. I also considered how
git_config_from_parameters doesn't share the same source marking. But it
follows a completely different code-path, as its syntax is totally
different. So there's really not much point in adding to the config_file
stack (if you had another source that could be included from a file, you
would want to mention it in the stack. But the command-line parameters
must always be the "root" of the stack, so it's sufficient to simply
have a NULL stack to mark that).

> But in practice, including from the command line (i.e. -c include.path=...)
> is somewhat crazy and we probably would not want to support it, so...

Actually, I made sure it works, and it's in the tests. It even complains
if you use a relative path (I thought about using $PWD, but that is too
insane, as git changes the working directory behind the scenes).

The one use I think is to bundle a bunch of related config options, and
then turn them on selectively. So you _could_ do:

  cat >>~/.bashrc <<-\EOF
  FOO_OPTIONS="-c foo.one=1 -c foo.two=2"
  EOF
  # without FOO
  git blah
  # with FOO
  git $FOO_OPTIONS blah

but with this patch, you can do:

  cat >~/.gitconfig.foo <<-\EOF
  [foo]
  one = 1
  two = 2
  EOF
  git -c include.path=$HOME/.gitconfig.foo blah

which to my mind is a little more natural. I don't personally plan on
using it, but it was easy to implement (in fact, I'd have to write to
disallow it, since the include magic is in the callback handler).

-Peff

Re: [PATCH 1/4] config: add include directive

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> And then because of 1a and 2a, most programs should Just Work without
> any changes, but because of 1b and 2b, any special uses will have to
> decide manually whether they would want to allow includes.
>
> Does that make sense?

In short, the "git config" interface defaults to "--no-includes" when
reading from an explicit file with "-f" and "--includes" otherwise, which
sounds like a 100% sensible default to me.

> I had a similar thought while writing it, but hoped the sentence after
> (that you snipped) would make it clear.

I think the whole paragraph makes it reasonably clear; I was merely being
pedantic to see if you or others can come up with a clear and simple way
to rephrase it that can also satisfy such pedantry.

> How about:
>
>   The included file is processed immediately, before any other
>   directives from the surrounding file.
>
> What I wanted to make clear there is the ordering, which sometimes
> matters.

Hmm, I think the original is probably easier to read.

> The one use I think is to bundle a bunch of related config options, and
> then turn them on selectively.
> ...
> but with this patch, you can do:
>
>   cat >~/.gitconfig.foo <<-\EOF
>   [foo]
>   one = 1
>   two = 2
>   EOF
>   git -c include.path=$HOME/.gitconfig.foo blah

That is quite a sensible use case actually.

Thanks.

Re: [PATCH 1/4] config: add include directive

[Jeff King]

On Thu, Jan 26, 2012 at 09:23:59PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > And then because of 1a and 2a, most programs should Just Work without
> > any changes, but because of 1b and 2b, any special uses will have to
> > decide manually whether they would want to allow includes.
> >
> > Does that make sense?
> 
> In short, the "git config" interface defaults to "--no-includes" when
> reading from an explicit file with "-f" and "--includes" otherwise, which
> sounds like a 100% sensible default to me.

Yes, exactly. Thank you for explaining it in about 1/10 the words I
needed to use. :)

I'll put that behavior in the re-roll.

> > How about:
> >
> >   The included file is processed immediately, before any other
> >   directives from the surrounding file.
> >
> > What I wanted to make clear there is the ordering, which sometimes
> > matters.
> 
> Hmm, I think the original is probably easier to read.

OK, then I'll leave it unless somebody else wants to come up with
something brilliant.

-Peff

Re: [PATCH 1/4] config: add include directive

[Jens Lehmann]

Am 26.01.2012 23:51, schrieb Jeff King:
> However, I didn't think about the fact that git-submodule.sh would be
> calling git-config separately, and that is accidentally changed by my
> patch.  Even if we changed git-submodule to use "git config
> --no-includes" that would break any third-party scripts that use "git
> config" to read git-like files.
> 
> But it would be nice for callers doing "git config foo.bar" to get the
> includes by default. So maybe the right rule is:
> 
>   1. In C:
>      a. git_config() respects includes automatically.
>      b. other callers do not do so automatically (e.g., gitmodules via
>         submodule.c).
> 
>     (i.e., what is implemented by this patch)
> 
>   2. Callers of git-config:
>      a. respect includes for lookup that checks all of the "normal"
>         config spots in sequence: .git/config, ~/.gitconfig, and
>         /etc/gitconfig. These are the shell equivalent of calling
>         git_config().
> 
>      b. when we are looking in a specific file (via GIT_CONFIG or "git
>         config -f"), do not respect includes (but allow --includes if
>         the caller chooses). This specific file may be something like
>         .gitmodules. Or perhaps somebody is saying "no, I really just
>         want to know what is in _this_ file, not what the config
>         ecosystem tells me in general".
> 
> And then because of 1a and 2a, most programs should Just Work without
> any changes, but because of 1b and 2b, any special uses will have to
> decide manually whether they would want to allow includes.
> 
> Does that make sense?

To me it really does. It lets submodule.c:gitmodules_config and
"git config -f .gitmodules" behave in the same way, which is very
important for consistent behavior between the submodule script and
the submodule functionality that is already handled in c. And I don't
know of a use case for includes in .gitmodules (the main reason for
adding includes seems to be to enable users to have configuration
stored in the repo, which the .gitmodules file already is. And if it
is about having out of repo configuration blended in, .gitmodules
settings are always overridden by those in .git/config, and you can
use includes there).

The only thing I'm not so sure about is the GIT_CONFIG case. I don't
know if using this is rather a "I moved my config there, but please
respect includes there too" or a "I want git config to look at a
completely different file" use case. Probably both. But also the
question of where to look for relative paths seems not so easy to
answer for the GIT_CONFIG case, so it might be best to just disable
includes there too.

Re: [PATCH 1/4] config: add include directive

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 26, 2012 at 08:37, Jeff King <peff@peff.net> 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.

Re: [PATCH 1/4] config: add include directive

[Jeff King]

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 <peff@peff.net> 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.

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

Re: [PATCH 1/4] config: add include directive

[Ævar Arnfjörð Bjarmason]

On Fri, Jan 27, 2012 at 01:32, Jeff King <peff@peff.net> wrote:
> 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 <peff@peff.net> 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.
>
> 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?

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.

Re: [PATCH 1/4] config: add include directive

[Michael Haggerty]

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.

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

How about a short comment or two?

> diff --git a/config.c b/config.c
> index 40f9c6d..a6966c1 100644
> --- a/config.c
> +++ b/config.c
> [...]
> +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.

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

Re: [PATCH 1/4] config: add include directive

[Jeff King]

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

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

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

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

[PATCH 2/4] config: factor out config file stack management

[Jeff King]

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 <peff@peff.net>
---
 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 = &top;
 
 		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

[PATCH 3/4] config: support parsing config data from buffers

[Jeff King]

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 <peff@peff.net>
---
 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

[PATCH 4/4] config: allow including config from repository blobs

[Jeff King]

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 <peff@peff.net>
---
 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

Re: [PATCH 4/4] config: allow including config from repository blobs

[Johannes Sixt]

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

Re: [PATCH 4/4] config: allow including config from repository blobs

[Jeff King]

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

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

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.

Re: [PATCH 4/4] config: allow including config from repository blobs

[Nguyen Thai Ngoc Duy]

On Thu, Jan 26, 2012 at 4:25 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> 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?

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

Re: [PATCH 4/4] config: allow including config from repository blobs

[Jeff King]

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

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

Re: [PATCH 4/4] config: allow including config from repository blobs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Hmm, the concept is surely *fun*, but is this really worth it?

With this, projects could include README that says something like this:

	When working with our project, we would suggest using some tweaks
	to the configuration file. For your convenience, a copy of it
	already exists in the clone of your repository, and all you have
	to do is to run this in your repository:

	$ git config include.ref 4774acaf6657efed

	We may update the set of recommended tweaks from time to time, so
        watch this README file for such updates, and re-run the above command
	with the updated blob object name as needed.

	Note: if you are paranoid and suspect that the project might
	give you trojan horse, you could inspect the recommended
	tweaks first before including them, like this:

	$ git cat-file -p 4774acaf6657efed

The blob will be included in the repository and the most natural way for
such a project to arrange things is to keep it together with the source
tree, perhaps in a separate hierarchy, say "dev_tools/std_gitconfig" or
something.  Without the update in patches 3 and 4, the project should be
able to update the above for its participants with minimum fuss, e.g.

diff --git a/README b/README
index af31555..203d255 100644
--- a/README
+++ b/README
@@ -3,14 +3,15 @@
 	already exists in the clone of your repository, and all you have
 	to do is to run this in your repository:
 
-	$ git config include.ref 4774acaf6657efed
+	$ cp dev_tools/std_gitconfig .git/std_gitconfig
+	$ git config include.path std_gitconfig
 
 	We may update the set of recommended tweaks from time to time, so
-        watch this README file for such updates, and re-run the above command
-	with the updated blob object name as needed.
+        watch "git log -p dev_tools/std_gitconfig" for such updates and
+	update your .git/std_gitconfig as needed.
 
 	Note: if you are paranoid and suspect that the project might
 	give you trojan horse, you could inspect the recommended
 	tweaks first before including them, like this:
 
-	$ git cat-file -p 4774acaf6657efed
+	$ less dev_tools/std_gitconfig

Re: [PATCH 4/4] config: allow including config from repository blobs

[Jeff King]

On Thu, Jan 26, 2012 at 01:14:46PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +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).
> 
> Hmm, the concept is surely *fun*, but is this really worth it?
> [you could just tell people to copy the file into .git]

Yes, that does work. I liked the idea of putting it in the repo, though,
because it means you can actually _track_ the contents, including any
local changes you make.

So yeah, if you are just going to copy it once, or even periodically, it
is not that big an advantage. And the example I gave using "git tag" did
just that. But I also wanted to allow more complex things, like this:

  # clone and inspect
  git clone git://example.com/project.git
  cd project
  git show origin:devtools/std_gitconfig

  # well, that looks pretty good. But I'd like to tweak something.
  git checkout -b config origin
  $EDITOR devtools/std_gitconfig
  git commit -a -m "drop the foo option, which I hate"

  # OK, let's use it now.
  git config include.ref config:devtools/std_gitconfig

  # Weeks pass. Somebody else updates the std_gitconfig.
  git fetch
  # let's inspect the changes
  git checkout config
  git diff @{u} -- devtools/std_gitconfig
  # looks good, let's merge (not copy!) them in
  git merge @{u}

This is obviously an advanced thing to be doing. But it's cool to me,
because even if you aren't working on a shared project, it is a means of
versioning your config.

-Peff

Re: [PATCH 4/4] config: allow including config from repository blobs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So yeah, if you are just going to copy it once, or even periodically, it
> is not that big an advantage. And the example I gave using "git tag" did
> just that. But I also wanted to allow more complex things, like this:
>
>   # clone and inspect
>   git clone git://example.com/project.git
>   cd project
>   git show origin:devtools/std_gitconfig
>
>   # well, that looks pretty good. But I'd like to tweak something.
>   git checkout -b config origin
>   $EDITOR devtools/std_gitconfig
>   git commit -a -m "drop the foo option, which I hate"
>
>   # OK, let's use it now.
>   git config include.ref config:devtools/std_gitconfig
>
>   # Weeks pass. Somebody else updates the std_gitconfig.
>   git fetch
>   # let's inspect the changes
>   git checkout config
>   git diff @{u} -- devtools/std_gitconfig
>   # looks good, let's merge (not copy!) them in
>   git merge @{u}
>
> This is obviously an advanced thing to be doing.

The "which *I* hate" in the log message makes it sound as if it is a
personal preference, but in fact this is more about maintaining the
recommended configuration among participants, no?  And if you have the
source of the configuration on a branch so that people can work on it
among themselves, then "config.path = ../devtools/std_gitconfig" should be
sufficient, no?

The pros-and-cons between the volume of the change to read include from
blobs and the benefit illustrated in the use case did not look too good to
me, at least from the messages in this thread so far.

Re: [PATCH 4/4] config: allow including config from repository blobs

[Jeff King]

On Thu, Jan 26, 2012 at 04:35:59PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So yeah, if you are just going to copy it once, or even periodically, it
> > is not that big an advantage. And the example I gave using "git tag" did
> > just that. But I also wanted to allow more complex things, like this:
> >
> >   # clone and inspect
> >   git clone git://example.com/project.git
> >   cd project
> >   git show origin:devtools/std_gitconfig
> >
> >   # well, that looks pretty good. But I'd like to tweak something.
> >   git checkout -b config origin
> >   $EDITOR devtools/std_gitconfig
> >   git commit -a -m "drop the foo option, which I hate"
> >
> >   # OK, let's use it now.
> >   git config include.ref config:devtools/std_gitconfig
> >
> >   # Weeks pass. Somebody else updates the std_gitconfig.
> >   git fetch
> >   # let's inspect the changes
> >   git checkout config
> >   git diff @{u} -- devtools/std_gitconfig
> >   # looks good, let's merge (not copy!) them in
> >   git merge @{u}
> >
> > This is obviously an advanced thing to be doing.
> 
> The "which *I* hate" in the log message makes it sound as if it is a
> personal preference, but in fact this is more about maintaining the
> recommended configuration among participants, no?

No, I meant it explicitly to be about this single user hating it. Note
how the resulting commits are never pushed. It is purely a local
override, but with the added bonus that history is tracked so you can
merge in further changes from upstream.

Of course, you could also share it with others, or do whatever. Once
it's tracked by git, you can be as flexible as you like.

> And if you have the source of the configuration on a branch so that
> people can work on it among themselves, then "config.path =
> ../devtools/std_gitconfig" should be sufficient, no?

Yes, you _could_ just keep it in a branch, merge upstream's changes into
the branch, and then periodically copy it out to your .git directory.
But this removes that final step.

It also does allow "[include]ref = origin/meta:gitconfig" if you want to
live dangerously. I consider that a feature, because it lets the user
make the security tradeoff they deem appropriate. Yes, I want to have
git be secure by default, and yes I want to encourage awareness of the
issues in the documentation for the feature. But I suspect in practice
that many people fetch changes and run "make" without looking at them,
which is basically the exact same hole. If a user has already accepted
that risk, why deny them the convenience of accepting it somewhere else?

> The pros-and-cons between the volume of the change to read include from
> blobs and the benefit illustrated in the use case did not look too good to
> me, at least from the messages in this thread so far.

I didn't think the read-from-blob code was very big or complex (most of
the refactoring was to support parsing an arbitrary buffer, but I think
that may be a good thing to have in the long run, anyway).

But I guess that is all a matter of opinion.

-Peff

Re: [PATCH 4/4] config: allow including config from repository blobs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> No, I meant it explicitly to be about this single user hating it. Note
> how the resulting commits are never pushed. It is purely a local
> override, but with the added bonus that history is tracked so you can
> merge in further changes from upstream.
> ...
> It also does allow "[include]ref = origin/meta:gitconfig" if you want to
> live dangerously. I consider that a feature, because it lets the user
> make the security tradeoff they deem appropriate.

While I do not think origin/meta:config is a sensible default, I actually
do think that:

	[include]
        	ref = meta:gitconfig
        [branch "meta"]
		remote = origin
        	merge = refs/heads/meta

makes some sense. The earlier example with the in-tree dev_tools/config in
the same line of history as the usual source material to keep track of
private changes ("this single user hating it") was not realistic as it
forbids the user from sharing the rest of the source once she decides to
fork the config preference.

Re: [PATCH 4/4] config: allow including config from repository blobs

[Jeff King]

On Thu, Jan 26, 2012 at 09:30:56PM -0800, Junio C Hamano wrote:

> While I do not think origin/meta:config is a sensible default, I actually
> do think that:
> 
> 	[include]
>         	ref = meta:gitconfig
>         [branch "meta"]
> 		remote = origin
>         	merge = refs/heads/meta
> 
> makes some sense. The earlier example with the in-tree dev_tools/config in
> the same line of history as the usual source material to keep track of
> private changes ("this single user hating it") was not realistic as it
> forbids the user from sharing the rest of the source once she decides to
> fork the config preference.

I don't think having it in-tree makes a difference. I can fork the
regular tree into my config branch, and it contains only my config
changes. If I want to share config changes with people, then I do so by
sharing that branch. But it need not have any impact on the "real"
branch I create from the regular tree. The fact that the rest of the
source files are in the config branch are irrelevant.

That being said, I think it would be nicer for projects to carry meta
information like this out-of-tree in a special ref. It's just simpler to
work with, and it means the project's source isn't polluted with extra
junk.

-Peff

Re: [PATCH 4/4] config: allow including config from repository blobs

[Johannes Sixt]

Am 1/27/2012 6:42, schrieb Jeff King:
> That being said, I think it would be nicer for projects to carry meta
> information like this out-of-tree in a special ref. It's just simpler to
> work with, and it means the project's source isn't polluted with extra
> junk.

Really? I doubt that carrying configuration in a special ref outside the
normal contents will have any practical relevance:

To manage such a config file would mean to switch to a branch with
entirely different contents. But before you can test the new configuration
in action, you have to commit, switch branches, which exchanges the
worktree completely; and if the config change didn't work out, repeat the
process (and if we are talking about source code repository, this usally
includes a complete rebuild). Sure, you could keep the config branch in a
separate repository, but, again, how do you test an updated configuration?
It is not funny, and nobody will go this route.

Which raises doubts about the usefulness of the include.ref feature.

-- Hannes

Re: [PATCH 4/4] config: allow including config from repository blobs

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 1/27/2012 6:42, schrieb Jeff King:
>> That being said, I think it would be nicer for projects to carry meta
>> information like this out-of-tree in a special ref. It's just simpler to
>> work with, and it means the project's source isn't polluted with extra
>> junk.
>
> Really? I doubt that carrying configuration in a special ref outside the
> normal contents will have any practical relevance:
>
> To manage such a config file would mean to switch to a branch with
> entirely different contents. But before you can test the new configuration
> in action, you have to commit, switch branches, which exchanges the
> worktree completely; and if the config change didn't work out, repeat the
> process (and if we are talking about source code repository, this usally
> includes a complete rebuild). Sure, you could keep the config branch in a
> separate repository, but, again, how do you test an updated configuration?
> It is not funny, and nobody will go this route.
>
> Which raises doubts about the usefulness of the include.ref feature.

Hmm, good point.

What I envisioned, when I said "meta:gitconfig" might make sense, was to
do something like:

 * have a separate worktree via git-new-worktree in meta/, so that you
   do not have to switch away from the "source" branch and trash the
   working tree for it; and

 * update meta/gitconfig, perhaps make commit there, and possibly push
   it back for public consuption.

In other words, I think "you *could* keep the config branch in a separate
repository" is more like "you would most likely want to have a separate
checkout of the config branch for this 'meta' branch to be useful".

And at that point, as you said, setting include.path = meta/gitconfig
(with possibly adding meta/ in .git/info/exclude) would be far more
pleasant, because you would have a chance to experiment your changes to
the file before committing it.

So having include.ref, while it is a fun thought experiment, would not
help very much in the real life.

Re: [PATCH 4/4] config: allow including config from repository blobs

[Nguyen Thai Ngoc Duy]

On Thu, Jan 26, 2012 at 2:42 PM, Jeff King <peff@peff.net> 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

Re: [PATCH 4/4] config: allow including config from repository blobs

[Jeff King]

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 <peff@peff.net> 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.

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

Re: [RFC/PATCH 0/4] config include directives

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 26, 2012 at 08:35, Jeff King <peff@peff.net> 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.

Re: [RFC/PATCH 0/4] config include directives

[Jeff King]

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