[PATCH 0/2] config includes, take 2

[PATCH 0/2] config includes, take 2

[Jeff King]

I decided to drop the "read config from a ref" bits of the series. There
was some debate about what the proper workflow should be around projects
sharing config. And since I am not part of a project that does so, I
can't contribute anything empirical to the discussion. So I'll let that
part live on in the list archive, and somebody whose project really
wants to experiment with it can feel free to do so.

That leaves the file-inclusion bits:

  [1/2]: imap-send: remove dead code
  [2/2]: config: add include directive

The first patch is new in this round, and is a necessary cleanup for the
second patch (though it might be worth applying regardless).

The second patch corresponds to patch 1/4 from the previous round. Among
the functional changes are:

  1. do not use includes for "git config" in single-file mode

  2. do not respect include.foo.path

  3. perform cycle and duplicate detection on included files

Doing (3) actually ended up changing the implementation a fair bit. The
original implementation worked purely as a drop-in callback wrapper. But
to suppress duplicates, you need to know not just which files you've
included, but which files you started from. So either
git_config_from_file has to learn about the include mechanism, or each
caller has to manually mark the file it is about to read.

I ended up just teaching the config machinery about the include
mechanism. It meant more changes (because we now pass an extra "include"
argument between the config functions) but I think turns out much more
readable.

That being said, I'm having doubts about the idea of duplicate
suppression at all. Suppose I have a setup like this:

  git config -f foo test.value foo
  git config test.value repo
  git config include.path $PWD/foo
  git config --global test.value global
  git config --global include.path $PWD/foo

That is, two config files, each of which sets test.value but also
includes "foo". One might assume from this that the output of "git
config test.value" would be "foo". But because of duplicate suppression,
it's not.

We first read the global config, which sets the value to "global", then
includes foo, which overwrites it to "foo". Then we read the repo
config, which sets the value to "repo", and then does _not_ actually
read foo. Because git config is read linearly and later values tend to
overwrite earlier ones, we would want to suppress the _first_ instance
of a file, not the second (or really, the final if it is included many
times). But that is impossible to do without generating a complete graph
of includes and then pruning the early ones.

Sure, this is a trivial toy example, and by looking at all of the
inputs, you can see what is happening and fix it. But the point of
duplicate suppression was that individual config files wouldn't have to
know or care what was being included elsewhere. A slightly more
real-world example is this:

  1. A config file "/etc/git-defaults-foo.cfg" sets a bunch of
     reasonable defaults for some group of developers.

  2. One of the developers really likes these foo defaults, but wants to
     override one particular option. So he does this:

       git config --global include.path /etc/git-defaults-foo.cfg
       git config --global some.option override

  3. On one particular repo, though, this dev wants the stock foo
     defaults. So he does:

       git config --global include.path /etc/git-defaults-foo.cfg

But we ignore the final include request; it's suppressed as a duplicate,
even though it would cancel his override.

So I'm actually thinking I should drop the duplicate suppression and
just do some sort of sanity check on include-depth to break cycles
(i.e., just die because it's a crazy thing to do, and we are really just
trying to tell the user their config is broken rather than go into an
infinite loop). As a bonus, it makes the code much simpler, too.

I'll post the patch with duplicate suppression for reference, but read
it with a grain of salt (or don't bother to read it at all if after
reading the above you think it should be thrown out).

-Peff

[PATCH 1/2] imap-send: remove dead code

[Jeff King]

The imap-send code was adapted from another project, and
still contains many unused bits of code. One of these bits
contains a type "struct string_list" which bears no
resemblence to the "struct string_list" we use elsewhere in
git. This causes the compiler to complain if git's
string_list ever becomes part of cache.h.

Let's just drop the dead code.

Signed-off-by: Jeff King <peff@peff.net>
---
This is necessary for patch 2, which does include string-list.h in
cache.h. If you read my cover letter, I think patch 2 might not be the
best approach. However, I think it might be worth applying this just as
a cleanup (e.g., even without the build problems, grepping for "struct
string_list" turns up this useless cruft).

 imap-send.c |   23 -----------------------
 1 files changed, 0 insertions(+), 23 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index e40125a..972ad62 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -42,28 +42,6 @@ struct store_conf {
 	unsigned trash_remote_new:1, trash_only_new:1;
 };
 
-struct string_list {
-	struct string_list *next;
-	char string[1];
-};
-
-struct channel_conf {
-	struct channel_conf *next;
-	char *name;
-	struct store_conf *master, *slave;
-	char *master_name, *slave_name;
-	char *sync_state;
-	struct string_list *patterns;
-	int mops, sops;
-	unsigned max_messages; /* for slave only */
-};
-
-struct group_conf {
-	struct group_conf *next;
-	char *name;
-	struct string_list *channels;
-};
-
 /* For message->status */
 #define M_RECENT       (1<<0) /* unsyncable flag; maildir_* depend on this being 1<<0 */
 #define M_DEAD         (1<<1) /* expunged */
@@ -71,7 +49,6 @@ struct group_conf {
 
 struct message {
 	struct message *next;
-	/* struct string_list *keywords; */
 	size_t size; /* zero implies "not fetched" */
 	int uid;
 	unsigned char flags, status;
-- 
1.7.9.rc1.29.g43677

[PATCH 2/2] config: add include directive

[Jeff King]

[If you haven't read my cover letter, do so now. The surprise ending is
 that I think this patch may not be the right approach, and I am posting
 it for reference. I don't want anybody to waste time reading and
 reviewing this without that context.]
-- >8 --

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 adds a new "include" parameter to
git_config_from_file to turn on includes.  Include
directives are turned on for "regular" git config parsing:
when you call git_config(), or when you make a call to the
"git config" program without using any single-file options.

Includes are off 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. Reading single files via "git config". This is for two
     reasons:

       a. backwards compatibility with scripts looking at
          config-like files.

       b. inspection of a specific file probably means you
	  care about just what's in that file, not a general
          lookup for "do we have this value anywhere at
	  all". If that is not the case, the caller can
	  always specify "--includes".

  3. 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/clone.c              |    2 +-
 builtin/config.c             |   19 ++++--
 builtin/init-db.c            |    2 +-
 cache.h                      |   19 ++++-
 config.c                     |  118 ++++++++++++++++++++++++++++------
 sequencer.c                  |    2 +-
 setup.c                      |    2 +-
 submodule.c                  |    2 +-
 t/t1305-config-include.sh    |  145 ++++++++++++++++++++++++++++++++++++++++++
 11 files changed, 295 insertions(+), 36 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/clone.c b/builtin/clone.c
index c62d4b5..32b7808 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -614,7 +614,7 @@ static void write_config(struct string_list *config)
 
 	for (i = 0; i < config->nr; i++) {
 		if (git_config_parse_parameter(config->items[i].string,
-					       write_one_config, NULL) < 0)
+					       write_one_config, NULL, NULL) < 0)
 			die("unable to write parameters to config file");
 	}
 }
diff --git a/builtin/config.c b/builtin/config.c
index d35c06a..be5d0fb 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 struct config_include_context include = CONFIG_INCLUDE_INIT(-1);
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -74,6 +75,7 @@ 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_BOOL(0, "includes", &include.enabled, "respect include directives on lookup"),
 	OPT_END(),
 };
 
@@ -214,18 +216,18 @@ static int get_value(const char *key_, const char *regex_)
 	}
 
 	if (do_all && system_wide)
-		git_config_from_file(show_config, system_wide, NULL);
+		git_config_from_file(show_config, system_wide, NULL, &include);
 	if (do_all && global)
-		git_config_from_file(show_config, global, NULL);
+		git_config_from_file(show_config, global, NULL, &include);
 	if (do_all)
-		git_config_from_file(show_config, local, NULL);
-	git_config_from_parameters(show_config, NULL);
+		git_config_from_file(show_config, local, NULL, &include);
+	git_config_from_parameters(show_config, NULL, &include);
 	if (!do_all && !seen)
-		git_config_from_file(show_config, local, NULL);
+		git_config_from_file(show_config, local, NULL, &include);
 	if (!do_all && !seen && global)
-		git_config_from_file(show_config, global, NULL);
+		git_config_from_file(show_config, global, NULL, &include);
 	if (!do_all && !seen && system_wide)
-		git_config_from_file(show_config, system_wide, NULL);
+		git_config_from_file(show_config, system_wide, NULL, &include);
 
 	free(key);
 	if (regexp) {
@@ -384,6 +386,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			config_exclusive_filename = given_config_file;
 	}
 
+	if (include.enabled == -1)
+		include.enabled = !config_exclusive_filename;
+
 	if (end_null) {
 		term = '\0';
 		delim = '\n';
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 0dacb8b..de6290f 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -146,7 +146,7 @@ static void copy_templates(const char *template_dir)
 	strcpy(template_path + template_len, "config");
 	repository_format_version = 0;
 	git_config_from_file(check_repository_format_version,
-			     template_path, NULL);
+			     template_path, NULL, NULL);
 	template_path[template_len] = 0;
 
 	if (repository_format_version &&
diff --git a/cache.h b/cache.h
index 9bd8c2d..f75923e 100644
--- a/cache.h
+++ b/cache.h
@@ -7,6 +7,7 @@
 #include "advice.h"
 #include "gettext.h"
 #include "convert.h"
+#include "string-list.h"
 
 #include SHA1_HEADER
 #ifndef git_SHA_CTX
@@ -1108,13 +1109,22 @@ extern int update_server_info(int);
 #define CONFIG_NOTHING_SET 5
 #define CONFIG_INVALID_PATTERN 6
 
+struct config_include_context {
+	struct string_list seen_paths;
+	int enabled;
+};
+#define CONFIG_INCLUDE_INIT(enable) { STRING_LIST_INIT_DUP, (enable) }
+
 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_file(config_fn_t fn, const char *, void *,
+				struct config_include_context *);
 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_from_parameters(config_fn_t fn, void *data,
+				      struct config_include_context *inc);
 extern int git_config(config_fn_t fn, void *);
-extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
+extern int git_config_early(config_fn_t fn, void *, const char *repo_config,
+			    struct config_include_context *inc);
 extern int git_parse_ulong(const char *, unsigned long *);
 extern int git_config_int(const char *, const char *);
 extern unsigned long git_config_ulong(const char *, const char *);
@@ -1137,7 +1147,8 @@ extern int config_error_nonbool(const char *);
 extern const char *get_log_output_encoding(void);
 extern const char *get_commit_output_encoding(void);
 
-extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
+extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data,
+				      struct config_include_context *inc);
 
 extern const char *config_exclusive_filename;
 
diff --git a/config.c b/config.c
index 40f9c6d..9b90751 100644
--- a/config.c
+++ b/config.c
@@ -28,6 +28,71 @@ static int zlib_compression_seen;
 
 const char *config_exclusive_filename = NULL;
 
+static int check_and_mark_include_path(struct config_include_context *inc,
+				       const char *path)
+{
+	const char *canonical;
+
+	if (!inc || !inc->enabled)
+		return 0;
+
+	/*
+	 * Using real_path would catch more duplicates, but we can't use it
+	 * here. It will die if some path components don't exist, and we have
+	 * no promise from the path in question actually exists.
+	 */
+	canonical = absolute_path(path);
+	if (unsorted_string_list_lookup(&inc->seen_paths, canonical))
+		return 1;
+
+	string_list_append(&inc->seen_paths, canonical);
+	return 0;
+}
+
+static int handle_path_include(const char *path, config_fn_t fn, void *data,
+			       struct config_include_context *inc)
+{
+	int ret = 0;
+	struct strbuf buf = STRBUF_INIT;
+
+	/*
+	 * Use an absolute path as-is, but interpret relative paths
+	 * based on the including config file.
+	 */
+	if (!is_absolute_path(path)) {
+		char *slash;
+
+		if (!cf || !cf->name)
+			return error("relative config includes must from from files");
+
+		slash = find_last_dir_sep(cf->name);
+		if (slash)
+			strbuf_add(&buf, cf->name, slash - cf->name + 1);
+		strbuf_addf(&buf, "%s", path);
+		path = buf.buf;
+	}
+
+	if (!access(path, R_OK))
+		ret = git_config_from_file(fn, path, data, inc);
+	strbuf_release(&buf);
+	return ret;
+}
+
+static int handle_config_variable(const char *var, const char *value,
+				  config_fn_t fn, void *data,
+				  struct config_include_context *inc)
+{
+	int r;
+
+	r = fn(var, value, data);
+	if (!r && inc && inc->enabled && value && !prefixcmp(var, "include.")) {
+		const char *type = var + 8;
+		if (!strcmp(type, "path"))
+			r = handle_path_include(value, fn, data, inc);
+	}
+	return r;
+}
+
 static void lowercase(char *p)
 {
 	for (; *p; p++)
@@ -47,8 +112,8 @@ void git_config_push_parameter(const char *text)
 	strbuf_release(&env);
 }
 
-int git_config_parse_parameter(const char *text,
-			       config_fn_t fn, void *data)
+int git_config_parse_parameter(const char *text, config_fn_t fn, void *data,
+			       struct config_include_context *inc)
 {
 	struct strbuf **pair;
 	pair = strbuf_split_str(text, '=', 2);
@@ -62,7 +127,10 @@ int git_config_parse_parameter(const char *text,
 		return error("bogus config parameter: %s", text);
 	}
 	lowercase(pair[0]->buf);
-	if (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) {
+
+	if (handle_config_variable(pair[0]->buf,
+				   pair[1] ? pair[1]->buf : NULL,
+				   fn, data, inc) < 0) {
 		strbuf_list_free(pair);
 		return -1;
 	}
@@ -70,7 +138,8 @@ int git_config_parse_parameter(const char *text,
 	return 0;
 }
 
-int git_config_from_parameters(config_fn_t fn, void *data)
+int git_config_from_parameters(config_fn_t fn, void *data,
+			       struct config_include_context *inc)
 {
 	const char *env = getenv(CONFIG_DATA_ENVIRONMENT);
 	char *envw;
@@ -89,7 +158,7 @@ int git_config_from_parameters(config_fn_t fn, void *data)
 	}
 
 	for (i = 0; i < nr; i++) {
-		if (git_config_parse_parameter(argv[i], fn, data) < 0) {
+		if (git_config_parse_parameter(argv[i], fn, data, inc) < 0) {
 			free(argv);
 			free(envw);
 			return -1;
@@ -191,7 +260,8 @@ static inline int iskeychar(int c)
 	return isalnum(c) || c == '-';
 }
 
-static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
+static int get_value(config_fn_t fn, void *data, char *name, unsigned int len,
+		     struct config_include_context *inc)
 {
 	int c;
 	char *value;
@@ -219,7 +289,8 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
 		if (!value)
 			return -1;
 	}
-	return fn(name, value, data);
+
+	return handle_config_variable(name, value, fn, data, inc);
 }
 
 static int get_extended_base_var(char *name, int baselen, int c)
@@ -277,7 +348,8 @@ static int get_base_var(char *name)
 	}
 }
 
-static int git_parse_file(config_fn_t fn, void *data)
+static int git_parse_file(config_fn_t fn, void *data,
+			  struct config_include_context *inc)
 {
 	int comment = 0;
 	int baselen = 0;
@@ -327,7 +399,7 @@ static int git_parse_file(config_fn_t fn, void *data)
 		if (!isalpha(c))
 			break;
 		var[baselen] = tolower(c);
-		if (get_value(fn, data, var, baselen+1) < 0)
+		if (get_value(fn, data, var, baselen+1, inc) < 0)
 			break;
 	}
 	die("bad config file line %d in %s", cf->linenr, cf->name);
@@ -826,11 +898,15 @@ int git_default_config(const char *var, const char *value, void *dummy)
 	return 0;
 }
 
-int git_config_from_file(config_fn_t fn, const char *filename, void *data)
+int git_config_from_file(config_fn_t fn, const char *filename, void *data,
+			 struct config_include_context *inc)
 {
 	int ret;
 	FILE *f = fopen(filename, "r");
 
+	if (check_and_mark_include_path(inc, filename))
+		return 0;
+
 	ret = -1;
 	if (f) {
 		config_file top;
@@ -844,7 +920,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 		strbuf_init(&top.value, 1024);
 		cf = &top;
 
-		ret = git_parse_file(fn, data);
+		ret = git_parse_file(fn, data, inc);
 
 		/* pop config-file parsing state stack */
 		strbuf_release(&top.value);
@@ -874,17 +950,17 @@ int git_config_system(void)
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
 }
 
-int git_config_early(config_fn_t fn, void *data, const char *repo_config)
+int git_config_early(config_fn_t fn, void *data, const char *repo_config,
+		     struct config_include_context *inc)
 {
 	int ret = 0, found = 0;
 	const char *home = NULL;
 
 	/* Setting $GIT_CONFIG makes git read _only_ the given config file. */
 	if (config_exclusive_filename)
-		return git_config_from_file(fn, config_exclusive_filename, data);
+		return git_config_from_file(fn, config_exclusive_filename, data, NULL);
 	if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
-		ret += git_config_from_file(fn, git_etc_gitconfig(),
-					    data);
+		ret += git_config_from_file(fn, git_etc_gitconfig(), data, inc);
 		found += 1;
 	}
 
@@ -893,17 +969,17 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		char buf[PATH_MAX];
 		char *user_config = mksnpath(buf, sizeof(buf), "%s/.gitconfig", home);
 		if (!access(user_config, R_OK)) {
-			ret += git_config_from_file(fn, user_config, data);
+			ret += git_config_from_file(fn, user_config, data, inc);
 			found += 1;
 		}
 	}
 
 	if (repo_config && !access(repo_config, R_OK)) {
-		ret += git_config_from_file(fn, repo_config, data);
+		ret += git_config_from_file(fn, repo_config, data, inc);
 		found += 1;
 	}
 
-	switch (git_config_from_parameters(fn, data)) {
+	switch (git_config_from_parameters(fn, data, inc)) {
 	case -1: /* error */
 		die("unable to parse command-line config");
 		break;
@@ -921,11 +997,13 @@ int git_config(config_fn_t fn, void *data)
 {
 	char *repo_config = NULL;
 	int ret;
+	struct config_include_context inc = CONFIG_INCLUDE_INIT(1);
 
 	repo_config = git_pathdup("config");
-	ret = git_config_early(fn, data, repo_config);
+	ret = git_config_early(fn, data, repo_config, &inc);
 	if (repo_config)
 		free(repo_config);
+	string_list_clear(&inc.seen_paths, 0);
 	return ret;
 }
 
@@ -1313,7 +1391,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 		 * As a side effect, we make sure to transform only a valid
 		 * existing config file.
 		 */
-		if (git_config_from_file(store_aux, config_filename, NULL)) {
+		if (git_config_from_file(store_aux, config_filename, NULL, NULL)) {
 			error("invalid config file %s", config_filename);
 			free(store.key);
 			if (store.value_regex != NULL) {
diff --git a/sequencer.c b/sequencer.c
index 5fcbcb8..50562b9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -636,7 +636,7 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 
 	if (!file_exists(opts_file))
 		return;
-	if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)
+	if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr, NULL) < 0)
 		die(_("Malformed options sheet: %s"), opts_file);
 }
 
diff --git a/setup.c b/setup.c
index 61c22e6..947fa51 100644
--- a/setup.c
+++ b/setup.c
@@ -329,7 +329,7 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
 	 * is a good one.
 	 */
 	snprintf(repo_config, PATH_MAX, "%s/config", gitdir);
-	git_config_early(check_repository_format_version, NULL, repo_config);
+	git_config_early(check_repository_format_version, NULL, repo_config, NULL);
 	if (GIT_REPO_VERSION < repository_format_version) {
 		if (!nongit_ok)
 			die ("Expected git repo version <= %d, found %d",
diff --git a/submodule.c b/submodule.c
index 9a28060..4c5b9be 100644
--- a/submodule.c
+++ b/submodule.c
@@ -116,7 +116,7 @@ void gitmodules_config(void)
 		}
 
 		if (!gitmodules_is_unmerged)
-			git_config_from_file(submodule_config, gitmodules_path.buf, NULL);
+			git_config_from_file(submodule_config, gitmodules_path.buf, NULL, NULL);
 		strbuf_release(&gitmodules_path);
 	}
 }
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
new file mode 100755
index 0000000..3d5a46d
--- /dev/null
+++ b/t/t1305-config-include.sh
@@ -0,0 +1,145 @@
+#!/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\"" >.gitconfig &&
+	echo 1 >expect &&
+	git config test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'include file by relative path' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >.gitconfig &&
+	echo 1 >expect &&
+	git config test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'chained relative paths' '
+	mkdir subdir &&
+	echo "[test]three = 3" >subdir/three &&
+	echo "[include]path = three" >subdir/two &&
+	echo "[include]path = subdir/two" >.gitconfig &&
+	echo 3 >expect &&
+	git config 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" >.gitconfig &&
+	echo one >expect &&
+	git config include.path >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'listing includes option and expansion' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >.gitconfig &&
+	cat >expect <<-\EOF &&
+	include.path=one
+	test.one=1
+	EOF
+	git config --list >actual.full &&
+	grep -v ^core actual.full >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'single file lookup does not expand includes by default' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >.gitconfig &&
+	test_must_fail git config -f .gitconfig test.one &&
+	test_must_fail git config --global test.one &&
+	echo 1 >expect &&
+	git config --includes -f .gitconfig test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'writing config file does not expand includes' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >.gitconfig &&
+	git config test.two 2 &&
+	echo 2 >expect &&
+	git config --no-includes test.two >actual &&
+	test_cmp expect actual &&
+	test_must_fail git config --no-includes test.one
+'
+
+test_expect_success 'config modification does not affect includes' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >.gitconfig &&
+	git config 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 --get-all test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'missing include files are ignored' '
+	cat >.gitconfig <<-\EOF &&
+	[include]path = foo
+	[test]value = yes
+	EOF
+	echo yes >expect &&
+	git config 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_expect_success 'include cycles are detected and broken' '
+	cat >.gitconfig <<-\EOF &&
+	[test]value = gitconfig
+	[include]path = cycle
+	EOF
+	cat >cycle <<-\EOF &&
+	[test]value = cycle
+	[include]path = .gitconfig
+	EOF
+	cat >expect <<-\EOF &&
+	gitconfig
+	cycle
+	EOF
+	git config --get-all test.value >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'multiple includes of the same file are suppressed' '
+	cat >.gitconfig <<-\EOF &&
+	[test]
+	value = base
+	[include]
+	path = A
+	path = B
+	EOF
+	echo "[include]path = C" >A &&
+	echo "[include]path = C" >B &&
+	echo "[test]value = C" >C &&
+	cat >expect <<-\EOF &&
+	base
+	C
+	EOF
+	git config --get-all test.value >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.7.9.rc1.29.g43677

Re: [PATCH 0/2] config includes, take 2

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... But the point of
> duplicate suppression was that individual config files wouldn't have to
> know or care what was being included elsewhere.

I think you wanted to say "the point of inclusion mechanism" is that
individual configuration files would not have to know, and I think I
agree.

> So I'm actually thinking I should drop the duplicate suppression and
> just do some sort of sanity check on include-depth to break cycles
> (i.e., just die because it's a crazy thing to do, and we are really just
> trying to tell the user their config is broken rather than go into an
> infinite loop). As a bonus, it makes the code much simpler, too.

Yeah, I stand corrected. It was a bad suggestion.

Thanks.

Re: [PATCH 0/2] config includes, take 2

[Michael Haggerty]

On 02/06/2012 07:27 AM, Jeff King wrote:
>   3. perform cycle and duplicate detection on included files
> [...]
> 
> We first read the global config, which sets the value to "global", then
> includes foo, which overwrites it to "foo". Then we read the repo
> config, which sets the value to "repo", and then does _not_ actually
> read foo. Because git config is read linearly and later values tend to
> overwrite earlier ones, we would want to suppress the _first_ instance
> of a file, not the second (or really, the final if it is included many
> times). But that is impossible to do without generating a complete graph
> of includes and then pruning the early ones.

ISTM that the main goal was to prevent infinite recursion, not avoid a
little bit of redundant reading.  If that is the goal, all you have to
record is the "include stack" context that caused the
currently-being-included file to be read and make sure that the
currently-being-included file didn't appear earlier on the stack.  The
fact that the same file was included earlier (but not as part of the
same include chain) needn't be considered an error.

> [...]
> So I'm actually thinking I should drop the duplicate suppression and
> just do some sort of sanity check on include-depth to break cycles
> (i.e., just die because it's a crazy thing to do, and we are really just
> trying to tell the user their config is broken rather than go into an
> infinite loop). As a bonus, it makes the code much simpler, too.

I agree that it is more sensible to error out than to ignore a recursive
include.

It might nevertheless be useful to have an "include call stack"
available for generating output for errors that occur when parsing
config files; something like:

Error when parsing /home/me/bogusconfigfile line 75
    which was included from /home/me/okconfigfile line 13
    which was included from /home/me/.gitconfig line 22

or

Error: /home/me/bogusconfigfile included recursively by
/home/me/bogusfile2 line 75
    which was included from /home/me/bogusconfigfile line 13
    which was included from /home/me/.gitconfig line 22

OTOH such error stack dumps could be generated when unwinding the C call
stack without the need for a separate "include call stack".

However, one could even imagine a command like

    $ git config --where-defined user.email
    user.email defined by /home/me/myfile2 line 75
        which was included from /home/me/myfile1 line 13
        which was included from /home/me/.gitconfig line 22
    user.email redefined by /home/me/project/.git/companyconfig line 8
        which was included from /home/me/project/.git/config line 20

This usage could not be implemented using the C stack, because the C
stack cannot be unwound multiple times.

But these are all just wild ideas.  I doubt that people's config files
will become so complicated that this much infrastructure is needed.

Michael

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

Re: [PATCH 0/2] config includes, take 2

[Jeff King]

On Mon, Feb 06, 2012 at 10:53:52AM +0100, Michael Haggerty wrote:

> ISTM that the main goal was to prevent infinite recursion, not avoid a
> little bit of redundant reading.

It was both, actually. There was a sense that we should not end up with
duplicate entries by reading the same file twice. However, entries which
actually create lists (and could therefore create duplicates) are by far
the minority compared to entries which overwrite. And it is the
overwrite-style entries which are harmed by suppressing duplicates.

> If that is the goal, all you have to record is the "include stack"
> context that caused the currently-being-included file to be read and
> make sure that the currently-being-included file didn't appear earlier
> on the stack.  The fact that the same file was included earlier (but
> not as part of the same include chain) needn't be considered an error.

I considered this, but decided the complexity wasn't worth it,
especially because getting it accurate means cooperation from
git_config_from_file, which otherwise doesn't know or care about this
mechanism. Instead I keep a simple depth counter and barf at a
reasonable maximum, printing the "from" and "to" files. Yes, it's not
nearly as elegant as keeping the whole stack, but I really don't expect
people to have complex super-deep includes, nor for it to be difficult
to hunt down the cause of a cycle.

Maybe that is short-sighted or lazy of me, but I'd just really be
surprised if people ever went more than 1 or 2 layers deep, or if they
actually ended up with a cycle at all.

There is a stack kept already by git_config_from_file, but it doesn't
respect the call-chain (so if I start a new git_config() call from
inside a callback, it is still part of the same stack).

We could possibly put a marker in the stack for that situation, and then
it would be usable for include cycle-detection.

> However, one could even imagine a command like
> 
>     $ git config --where-defined user.email
>     user.email defined by /home/me/myfile2 line 75
>         which was included from /home/me/myfile1 line 13
>         which was included from /home/me/.gitconfig line 22
>     user.email redefined by /home/me/project/.git/companyconfig line 8
>         which was included from /home/me/project/.git/config line 20

You can already implement this with the existing stack by providing a
suitable callback (since its your callback, you'd know that there was no
recursion of git_config, and therefore the stack refers only to a single
set of includes).

-Peff

Re: [PATCH 0/2] config includes, take 2

[Jeff King]

On Mon, Feb 06, 2012 at 05:06:22AM -0500, Jeff King wrote:

> > If that is the goal, all you have to record is the "include stack"
> > context that caused the currently-being-included file to be read and
> > make sure that the currently-being-included file didn't appear earlier
> > on the stack.  The fact that the same file was included earlier (but
> > not as part of the same include chain) needn't be considered an error.
> 
> I considered this, but decided the complexity wasn't worth it,
> especially because getting it accurate means cooperation from
> git_config_from_file, which otherwise doesn't know or care about this
> mechanism. Instead I keep a simple depth counter and barf at a
> reasonable maximum, printing the "from" and "to" files. Yes, it's not
> nearly as elegant as keeping the whole stack, but I really don't expect
> people to have complex super-deep includes, nor for it to be difficult
> to hunt down the cause of a cycle.

Oh, one other thing: to detect cycles, you have to use a canonical
version of the filename for comparisons. That makes the existing stack
that git_config_from_file keeps useless. Using real_path is hard,
because it will die() if some path components don't exist. Using
absolute_path is a reasonable compromise, but it can actually miss some
cycles if they involve symbolic links.

Not insurmountable if you can accept teaching git_config_from_file to
real_path() each file it reads. But again, it ends up getting complex
for IMHO not much gain. A stupid depth counter can't show you a stack
trace (and it may have false positives), but it's dirt simple and
probably good enough.

-Peff

Re: [PATCH 0/2] config includes, take 2

[David Aguilar]

On Sun, Feb 5, 2012 at 10:27 PM, Jeff King <peff@peff.net> wrote:
> That leaves the file-inclusion bits:
>
>  [1/2]: imap-send: remove dead code
>  [2/2]: config: add include directive
>
> The first patch is new in this round, and is a necessary cleanup for the
> second patch (though it might be worth applying regardless).
>
> The second patch corresponds to patch 1/4 from the previous round. Among
> the functional changes are:
>
>  1. do not use includes for "git config" in single-file mode

I have a questions about this.  Let's say I have ~/foo1.gitconfig:

[foo]
    bar = 1

...and ~/.gitconfig (I forgot the syntax):

[foo]
    bar = 0

#include "~/foo1.gitconfig"


Does that mean that:

    $ git config -f ~/.gitconfig foo.bar

...prints 0 and not 1?

I can see cases where this would be undesirable behavior.

For example, an application that uses "git config -z --list -f
~/.gitconfig" might expect that the result encompasses all of the
user-specific config bits.

Following this to its natural conclusion means "git config" might
learn some kind of --no-include flag for use with e.g. "git config
--no-include -f ~/.gitconfig".  That said, I don't know where I would
ever actually use such a flag.

I do know where I would use an `--include` flag (if following includes
were not the default behavior when using '-f'), though, and that's why
I'm asking.  The problem with not having it be the default means we
have to use a flag.  This makes it harder to support multiple versions
of git.

Maybe I'm mis-interpreting what you mean by, 'do not use includes for
"git config" in single-file mode', though.
--
David

Re: [PATCH 0/2] config includes, take 2

[Jeff King]

On Mon, Feb 06, 2012 at 09:01:21PM -0800, David Aguilar wrote:

> I have a questions about this.  Let's say I have ~/foo1.gitconfig:
> 
> [foo]
>     bar = 1
> 
> ...and ~/.gitconfig (I forgot the syntax):
> 
> [foo]
>     bar = 0
> 
> #include "~/foo1.gitconfig"
> 
> 
> Does that mean that:
> 
>     $ git config -f ~/.gitconfig foo.bar
> 
> ...prints 0 and not 1?

Yes, though the syntax is:

  [include]
    path = foo1.gitconfig

(it doesn't respect tilde-expansion, but it probably should). Note that
the syntax was specifically selected for backwards compatibility, and to
allow manipulation with existing tools.

> I can see cases where this would be undesirable behavior.
> 
> For example, an application that uses "git config -z --list -f
> ~/.gitconfig" might expect that the result encompasses all of the
> user-specific config bits.

The problem is that an application might also expect it _not_ to happen
(e.g., if the application is really interested in knowing what's in
~/.gitconfig). Because includes aren't respected now, the safe default
seems to be not to have includes (i.e., don't change behavior for this
case).

A bigger question for me is: if you are interested in getting an answer
from anywhere, why are you restricting with "-f"? IOW, is this a
real-world problem, or a hypothetical one? And if real-world, what is
the actual use case?

> Following this to its natural conclusion means "git config" might
> learn some kind of --no-include flag for use with e.g. "git config
> --no-include -f ~/.gitconfig".  That said, I don't know where I would
> ever actually use such a flag.

It already learned it as part of my series (and --includes, as well).

> I do know where I would use an `--include` flag (if following includes
> were not the default behavior when using '-f'), though, and that's why
> I'm asking.  The problem with not having it be the default means we
> have to use a flag.  This makes it harder to support multiple versions
> of git.

Yes, that's a general problem of adding new command-line options to turn
features off or on. We could add an environment variable to control this
feature. But I'd really like to hear the compelling use case first
(i.e., both why it cannot simply be spelled "git config -z --list", and
why not following includes is not OK).

> Maybe I'm mis-interpreting what you mean by, 'do not use includes for
> "git config" in single-file mode', though.

No, I think you understand the described behavior.

-Peff

Re: [PATCH 0/2] config includes, take 2

[David Aguilar]

On Mon, Feb 6, 2012 at 9:17 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Feb 06, 2012 at 09:01:21PM -0800, David Aguilar wrote:
>
>> I have a questions about this.  Let's say I have ~/foo1.gitconfig:
>>
>> [foo]
>>     bar = 1
>>
>> ...and ~/.gitconfig (I forgot the syntax):
>>
>> [foo]
>>     bar = 0
>>
>> #include "~/foo1.gitconfig"
>>
>>
>> Does that mean that:
>>
>>     $ git config -f ~/.gitconfig foo.bar
>>
>> ...prints 0 and not 1?
>
> Yes, though the syntax is:
>
>  [include]
>    path = foo1.gitconfig
>
> (it doesn't respect tilde-expansion, but it probably should). Note that
> the syntax was specifically selected for backwards compatibility, and to
> allow manipulation with existing tools.
>
>> I can see cases where this would be undesirable behavior.
>>
>> For example, an application that uses "git config -z --list -f
>> ~/.gitconfig" might expect that the result encompasses all of the
>> user-specific config bits.
>
> The problem is that an application might also expect it _not_ to happen
> (e.g., if the application is really interested in knowing what's in
> ~/.gitconfig). Because includes aren't respected now, the safe default
> seems to be not to have includes (i.e., don't change behavior for this
> case).
>
> A bigger question for me is: if you are interested in getting an answer
> from anywhere, why are you restricting with "-f"? IOW, is this a
> real-world problem, or a hypothetical one? And if real-world, what is
> the actual use case?

It's a real-world problem.  I haven't had to change the code in a
while which is why this thread caught my eye ;-)

I won't claim it's the best solution, but this has worked extremely
well in practice:

https://github.com/git-cola/git-cola/blob/master/cola/gitcfg.py

Here's the problem.  I need to know which config items are "user"
config (~/.gitconfig), which are "repo" config (.git/config), and
which are "system" config (/etc/gitconfig).

I also need to avoid calling "git config" too many times so I query
these files once and cache it all in memory.  We need only stat()
these files to know whether to redo the "git config" call.

Figuring out the repo config is tricky.  You can't just say "git
config --list" because that includes the user config stuff in
~/.gitconfig.

Figuring out the user config is easy because you can say "git config
--global --list".  This is inconsistent with the behavior for "git
config --list" because it does not include the --system config, which
one would expect given the overlay semantics.

Figuring out the system config can be done with --system.  That works fine.

The generic interface for getting a concise listing of values from
these sources is to use "git config -f" on ~/.gitconfig, .git/config,
and $(prefix)/etc/gitconfig.

git config --global and git config --system are both consistent in
that they return just the information relevant to them.  Is --global
just a shortcut for "-f ~/.gitconfig"?  If yes, then does that mean
"git config --global" will not honor includes?  If it is not a
shortcut, does that mean that "git config --global" and "git config -f
~/.gitconfig" are not really the same thing?  The documentation does
lead one to believe that they should be equivalent...

The takeaway is that querying these files provides a convenient way to
access the effective configuration values for the system, user, and
repo.


>> I do know where I would use an `--include` flag (if following includes
>> were not the default behavior when using '-f'), though, and that's why
>> I'm asking.  The problem with not having it be the default means we
>> have to use a flag.  This makes it harder to support multiple versions
>> of git.
>
> Yes, that's a general problem of adding new command-line options to turn
> features off or on. We could add an environment variable to control this
> feature. But I'd really like to hear the compelling use case first
> (i.e., both why it cannot simply be spelled "git config -z --list", and
> why not following includes is not OK).

Hopefully my explanation conveys why "git config -z --list" is insufficient.

Basically, I've been relying on the fact that querying each of these
files gives me exactly what users have effectively configured.
Pushing knowledge of includes onto the application is less fun.
Applications would either have to understand the [include] path = ...
construct and do something with it themselves or they'd have to check
the git version at runtime.

I would rather do without the runtime version check and have
everything "keep working".


So let's explore why I think it should follow includes.  Here are a
few reasons...

When the file format supports including other files then the most
logical thing for it to do is to follow those includes, no?  I know,
this is a tautology ;-).  I'll try again.

From a naive user's POV --
I am asking "git config" to evaluate this file.  I don't care what's
inside of it; it's a black box to me.  All I know is that I get config
values out the other side.  Sometimes git respects the included files.
Sometimes it does not (when using -f).
^^^^^^^^^
I think this subtle difference in behavior is best avoided.

Here's another.  The naive user will do: `git config --list -f file`.
They will then edit that file to add an include statement.  They will
then run `git config --list -f file` again and be confused as to why
their included configuration is not honored.

Right now these two are synonymous: "git config --system --list" and
"git config -f $(prefix)/etc/gitconfig --list".  Having one form honor
includes and not the other is inconsistent.

What if we frame this the other way around -- when would we not want
it to follow includes?  I can imagine someone writing a "git config"
editing program, but I think that use case is rare (and they haven't
spoken up, either ;-).

Well.. okay. My use case is rare too!  The dumb "cache the values
behind a stat() call" thing has worked really well in practice so I've
been happy with it so far.  It's dumb, and it won't notice when
included files change, but I really don't care because it solves the
99% case.

I'd be happy to rewrite it using another approach.  The
overlay-semantics inconsistency is what led me to using the generic
"-f" interface.  Performance issues led me to use a stat cache.  Using
each file's stat info as the cache key mapped nicely onto the generic
approach of using "-f".

Rewriting it (for me) would mean using --global and --system and
probably not caring so much about the distinction of repo vs. user
configuration (or rather, deal with the fact that `git config --list`
returns both the user config and the repo config). At that point I
might as well just write (reuse) a proper .gitconfig parser, but
what's the fun in that when "git config" is so nice and easy to parse?
;-)
-- 
David

Re: [PATCH 0/2] config includes, take 2

[Jeff King]

On Tue, Feb 07, 2012 at 02:05:34AM -0800, David Aguilar wrote:

> I won't claim it's the best solution, but this has worked extremely
> well in practice:
> 
> https://github.com/git-cola/git-cola/blob/master/cola/gitcfg.py
> 
> Here's the problem.  I need to know which config items are "user"
> config (~/.gitconfig), which are "repo" config (.git/config), and
> which are "system" config (/etc/gitconfig).

I'm not a git cola user. But from peeking at that code and a brief
"apt-get install git-cola && git cola", it looks like the point is to
show a dialog with per-repo and per-user settings, let the user tweak
them, and save them back to the appropriate spot.

But how does that interact with includes? Let's imagine I do this:

  git config --global include.path .gitidentity
  git config -f ~/.gitidentity user.name "Jeff King"

Without having git-cola expand includes, your call to "git config
--global --list" will not show the user.name variable, and it will
appear blank. Which is not great.

If git-cola does expand includes, then the name will appear. But what
will happen when I modify that field and try to save it? It will save it
using "git config --global", putting it into ~/.gitconfig. So now you'll
have two names in your config, one in .gitconfig and one in
.gitidentity.

And which one will take precedence?  That depends on what was in your
config file before. If there was a [user] section before the include
already in your .gitconfig, then the newly written value will go in that
section _before_ the include, and the include will still take
precedence. Otherwise, a new section will be written _after_ the
include, and that will take precedence.

So...yuck. It's kind of a complex and messy situation. But the important
thing is that when you are looking at a bidirectional editing situation
like this, blindly expanding includes is not going to solve the problem.
It's just going to paper over the complexity. In the long term,
something like git-cola that is trying to present and edit config to the
user would need to learn about includes, track the sources of each
variable, and then edit appropriately.

You can do that with my current patch by manually following include.path
directives. However, that's a pain.  Git-config could potentially help
with that (and even simplify the current code) by allowing something
like:

  $ git config --list-with-sources
  /home/peff/.gitconfig user.name=Jeff King
  /home/peff/.gitconfig user.email=peff@peff.net
  .git/config core.repositoryformatversion=0
  .git/config core.bare=false
  [etc]

(you would use the "-z" form, of course, and the filenames would be
NUL-separated, but I made up a human-readable output format above for
illustration purposes).

And then it would be natural to mention included files there, and you
could actually modify the file that contains the value in question. Of
course, you might not have _access_ to the file in question. You could
be pulling in options from a shared config file, and the right thing to
do is not to edit the shared file, but to override it. And that raises
the question of where, and how to tell git-config to make sure that the
option goes _after_ the include.

So fundamentally, includes make the idea of "overwrite this value" much
more complex. It's possible that git-config should learn all of this
complexity, and that git cola could rely on git-config being smart.  But
I was rather hoping that most of the automated tools could remain simple
and stupid, and that people who wanted to do complex things with
includes would be left to their own devices (i.e., using $EDITOR, or
smartly using "git config -f" on their individual files).

> Figuring out the repo config is tricky.  You can't just say "git
> config --list" because that includes the user config stuff in
> ~/.gitconfig.
> 
> Figuring out the user config is easy because you can say "git config
> --global --list".  This is inconsistent with the behavior for "git
> config --list" because it does not include the --system config, which
> one would expect given the overlay semantics.

I think you are mistaking a lack of source options for "look at the repo
config". But without providing a source, the request is "look everywhere
that git knows about". With "--global", it's about "look at this one
file" (just as "--system" is about "look at this other file"). So I
think what you are really lamenting is the lack of "git config --repo",
which would basically be:

  git config --list --file="$(git rev-parse --git-dir)/config"

That might be something we could improve in the documentation.

> The generic interface for getting a concise listing of values from
> these sources is to use "git config -f" on ~/.gitconfig, .git/config,
> and $(prefix)/etc/gitconfig.

Right. And --global and --system are basically just synonyms for the
prior two.

> git config --global and git config --system are both consistent in
> that they return just the information relevant to them.  Is --global
> just a shortcut for "-f ~/.gitconfig"?  If yes, then does that mean
> "git config --global" will not honor includes?  If it is not a
> shortcut, does that mean that "git config --global" and "git config -f
> ~/.gitconfig" are not really the same thing?  The documentation does
> lead one to believe that they should be equivalent...

Yes, it is a shortcut, and it follows the same code path (i.e., we
expand "--global" into "-f ~/.gitconfig" early on).

> > Yes, that's a general problem of adding new command-line options to turn
> > features off or on. We could add an environment variable to control this
> > feature. But I'd really like to hear the compelling use case first
> > (i.e., both why it cannot simply be spelled "git config -z --list", and
> > why not following includes is not OK).
> 
> Hopefully my explanation conveys why "git config -z --list" is insufficient.

Yes, I agree it's insufficient in the face of includes. Unfortunately, I
don't think simply expanding includes by default is sufficient, either.

Probably the simplest solution for this case would be something like:

  1. Always expand includes on listing.

  2. When editing, always override items in includes. So do _not_ reuse
     sections, but always put the values at the end of the file (or at
     least after any includes).

That's not as nice as doing it "right" (you'll end up with duplicated
values in included files and their parents, and sections may appear
multiple times). But it's simple and should have the desired semantics.

> From a naive user's POV --
> I am asking "git config" to evaluate this file.  I don't care what's
> inside of it; it's a black box to me.  All I know is that I get config
> values out the other side.  Sometimes git respects the included files.
> Sometimes it does not (when using -f).
> ^^^^^^^^^
> I think this subtle difference in behavior is best avoided.

It is not "sometimes when I ask git config to evaluate this file". It is
"git never expands includes when you ask it to evaluate a file".
Includes are expanded only when you ask git "search all files you know
about" (i.e., when you do not provide a file argument).

There is also a more complex issue with "-f", which is that git-config
is used not just to parse git configuration, but also to parse other
config-like files that should not respect includes (e.g.,
".gitmodules"). So to remain backwards-compatible, we need the default
for "-f" to not respect includes, and that is not really negotiable.

We could convert "--global" and "--system" to respect includes by
default (since we at least know these are actual git config files). But
that is introducing a subtle difference, which you complained about
above.

But if scripted uses like "git cola" are the ones that need it, I'd
rather provide a backwards-compatible way of tweaking the behavior (if a
command line option is not sufficient, then I'd rather provide an
environment variable).

> Here's another.  The naive user will do: `git config --list -f file`.
> They will then edit that file to add an include statement.  They will
> then run `git config --list -f file` again and be confused as to why
> their included configuration is not honored.

As I mentioned above, that one is not really negotiable. But it seems
like it is a mismatch of expectations to behavior, and that is something
we can at least address with documentation.

> Right now these two are synonymous: "git config --system --list" and
> "git config -f $(prefix)/etc/gitconfig --list".  Having one form honor
> includes and not the other is inconsistent.

With my patch, they are consistent, and neither should honor includes
(that being said, there is a bug in my patch which makes --list honor
includes in all cases; however, you are responding correctly to the
intended and advertised behavior of my patch, and that is what we should
be discussing).

> What if we frame this the other way around -- when would we not want
> it to follow includes?  I can imagine someone writing a "git config"
> editing program, but I think that use case is rare (and they haven't
> spoken up, either ;-).

But isn't "git cola" a git-config editing program?

> Well.. okay. My use case is rare too!  The dumb "cache the values
> behind a stat() call" thing has worked really well in practice so I've
> been happy with it so far.  It's dumb, and it won't notice when
> included files change, but I really don't care because it solves the
> 99% case.

I don't get the caching. How often would you need to call "git config"?
Isn't is only when the edit-config dialog appears? Is it really that
expensive to call when opening a user-editable dialog?

> Rewriting it (for me) would mean using --global and --system and
> probably not caring so much about the distinction of repo vs. user
> configuration (or rather, deal with the fact that `git config --list`
> returns both the user config and the repo config). At that point I
> might as well just write (reuse) a proper .gitconfig parser, but
> what's the fun in that when "git config" is so nice and easy to parse?

I definitely feel your pain. I don't want you to have to rewrite yet
another .gitconfig parser. Let's see if we can figure out reasonable
editing semantics that can go into git-config.

-Peff

Re: [PATCH 0/2] config includes, take 2

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So fundamentally, includes make the idea of "overwrite this value" much
> more complex.

I do not think it is anything new that include.path brings.  To give the
user a complete view of "what customization applies when working in this
repository?", and to allow the user to edit variables in the right file
among usual three places, the tool already needs to be aware of possible
places and give users a way to tell where the edit needs to go anyway.
include.path only adds one more thing for the tool to be aware of.

With your example, the editor can show

	git config -f .git/config --list

with "include.path=~/.gitident" listed as one of the key=value pairs
without showing key=value pairs included from that file.  Or it can show
user.name in effect is this value from .git/config, and optionally also
show that there are other definitions of user.name in ~/.gitconfig (which
we use as if we have "include.path=~/.gitconfig" at the top of .git/config
file) or ~/.gitident specified with include.path.

The tool needs to make it easy to jump to ~/.gitident; it needs to know
what include.path means.  The user can edit the value in ~/.gitident, or
after looking at ~/.gitident, choose to come back to .git/config and add
an overriding entry there.

> But isn't "git cola" a git-config editing program?

Yes, that is really the right "rhetorical" question to ask.  It needs to
know what it is editing, so it at least needs to be _aware_ of the
inclusion, where each item comes from, and how to edit contents of _one_
particular file.

And that issue is not something new that was introduced by include.path.

Re: [PATCH 0/2] config includes, take 2

[Jeff King]

On Tue, Feb 07, 2012 at 10:03:26AM -0800, Junio C Hamano wrote:

> > But isn't "git cola" a git-config editing program?
> 
> Yes, that is really the right "rhetorical" question to ask.  It needs to
> know what it is editing, so it at least needs to be _aware_ of the
> inclusion, where each item comes from, and how to edit contents of _one_
> particular file.
> 
> And that issue is not something new that was introduced by include.path.

True, but it was made more complex. David has already dealt with the 3
sources in his code. Adding includes expands this to N sources, and his
code needs to adapt. It would be nice if we could at least make the
adaptation backwards-compatible and as painless as possible.

But I just see the "right" answer in a complex case needing user input
that a simple call to "git config" can't provide. That is, pretending
that "git config --global" is still a single file for reading and
writing can get you something that _works_, but it will make a mess of
the user's config. The most elegant thing to me is to expand git-cola's
config editor into an N-pane editor instead of a 2-pane editor (not 3,
because it doesn't make sense to edit /etc/gitconfig with it, and nor
does the current version support it). People without includes wouldn't
notice the difference, and people with includes might appreciate the
power and flexibility.

-Peff

Re: [PATCH 0/2] config includes, take 2

[Jakub Narebski]

Jeff King <peff@peff.net> writes:

[...]
> Git-config could potentially help with that (and even simplify the
> current code) by allowing something like:
> 
>   $ git config --list-with-sources
>   /home/peff/.gitconfig user.name=Jeff King
>   /home/peff/.gitconfig user.email=peff@peff.net
>   .git/config core.repositoryformatversion=0
>   .git/config core.bare=false
>   [etc]
> 
> (you would use the "-z" form, of course, and the filenames would be
> NUL-separated, but I made up a human-readable output format above for
> illustration purposes).

That would be _very_ nice to have (even without includes support).

Filenames would be git-quoted like in ls-tree / diff-tree output without -z,
isn't it?  And is that TAB or SPC as a separator?
-- 
Jakub Narebski

Re: [PATCH 0/2] config includes, take 2

[Jeff King]

On Tue, Feb 07, 2012 at 11:16:47AM -0800, Jakub Narebski wrote:

> > Git-config could potentially help with that (and even simplify the
> > current code) by allowing something like:
> > 
> >   $ git config --list-with-sources
> >   /home/peff/.gitconfig user.name=Jeff King
> >   /home/peff/.gitconfig user.email=peff@peff.net
> >   .git/config core.repositoryformatversion=0
> >   .git/config core.bare=false
> >   [etc]
> > 
> > (you would use the "-z" form, of course, and the filenames would be
> > NUL-separated, but I made up a human-readable output format above for
> > illustration purposes).
> 
> That would be _very_ nice to have (even without includes support).
> 
> Filenames would be git-quoted like in ls-tree / diff-tree output without -z,
> isn't it?  And is that TAB or SPC as a separator?

Yeah, the output above is just illustrative. Without -z, I would do
quoted filename, TAB, and then $KEY=$VALUE (I didn't check whether we
quote an "=" in the subsection of a key, but we probably should).

-Peff

Re: [PATCH 0/2] config includes, take 2

[David Aguilar]

On Tue, Feb 7, 2012 at 11:16 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> Jeff King <peff@peff.net> writes:
>
> [...]
>> Git-config could potentially help with that (and even simplify the
>> current code) by allowing something like:
>>
>>   $ git config --list-with-sources
>>   /home/peff/.gitconfig user.name=Jeff King
>>   /home/peff/.gitconfig user.email=peff@peff.net
>>   .git/config core.repositoryformatversion=0
>>   .git/config core.bare=false
>>   [etc]
>>
>> (you would use the "-z" form, of course, and the filenames would be
>> NUL-separated, but I made up a human-readable output format above for
>> illustration purposes).
>
> That would be _very_ nice to have (even without includes support).

I like this as well.

Thanks for digging deep into this one, Jeff.  You've convinced me that
not following includes is the better default behavior.

You are correct in mentioning that what was really missing was
something akin to "git config --repo --list".  Since that command
would be a shortcut for "-f .git/config" then it would be consistent
in behavior.  The suggestion to have the app understand .[include]
path = and show separate panes for included files is an elegant
solution and definitely helpful for power users.  I do have to write
more code but that's fine since it enables new functionality.

Jeff, you mentioned possibly adding a "backwards-compatible way" of
accessing this stuff and hiding it behind an environment variable.  I
don't want to make us carry around backwards compat code paths just
for one particular use case so perhaps the best thing would be for me
to start preparing for this change now.  I already have various places
where functionality is guarded behind a git version check.  If what
we're talking about is git-cola adding "--include" when git >= 1.8(?)
then that works for me.

It should be noted that git-gui also uses `git config --global --list`
so I don't know if this has implications there.  E.g. maybe things
like user.name won't be overridden if done via an included config
there.

RE: the caching -- we call git config a few times in various places.
Getting the user.* fields.  Getting the diff.summary settings, etc.  I
started tweaking app startup for speed and noticed all the git-config
calls and was able to replace a handful of calls with a single call,
which was nice.  The difference is more pronounced on win32 (I am a
linux user but I do try to play nice with the msysgit folks).

Thanks Jeff,
-- 
David

Re: [PATCH 0/2] config includes, take 2

[Jeff King]

On Tue, Feb 07, 2012 at 11:16:47AM -0800, Jakub Narebski wrote:

> Jeff King <peff@peff.net> writes:
> 
> [...]
> > Git-config could potentially help with that (and even simplify the
> > current code) by allowing something like:
> > 
> >   $ git config --list-with-sources
> >   /home/peff/.gitconfig user.name=Jeff King
> >   /home/peff/.gitconfig user.email=peff@peff.net
> >   .git/config core.repositoryformatversion=0
> >   .git/config core.bare=false
> >   [etc]
> > 
> > (you would use the "-z" form, of course, and the filenames would be
> > NUL-separated, but I made up a human-readable output format above for
> > illustration purposes).
> 
> That would be _very_ nice to have (even without includes support).
> 
> Filenames would be git-quoted like in ls-tree / diff-tree output without -z,
> isn't it?  And is that TAB or SPC as a separator?

So the patch would look something like this. However, is the actual
filename really what callers want? It seems like in David's case, an
annotation of "repo", "global", or "system" (possibly in addition to the
filename) would be the most useful (because in the git-cola UI, it is
still nice to list things as "repo" or "global" instead of spewing the
whole filename at the user -- but you would still want the individual
filename for handling updates of includes).

---
 builtin/config.c |   20 ++++++++++++++++++--
 cache.h          |    1 +
 config.c         |    7 +++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index d35c06a..e0333f7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "color.h"
 #include "parse-options.h"
+#include "quote.h"
 
 static const char *const builtin_config_usage[] = {
 	"git config [options]",
@@ -41,6 +42,7 @@ static int end_null;
 #define ACTION_SET_ALL (1<<12)
 #define ACTION_GET_COLOR (1<<13)
 #define ACTION_GET_COLORBOOL (1<<14)
+#define ACTION_LIST_WITH_SOURCES (1<<15)
 
 #define TYPE_BOOL (1<<0)
 #define TYPE_INT (1<<1)
@@ -64,6 +66,7 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "rename-section", &actions, "rename section: old-name new-name", ACTION_RENAME_SECTION),
 	OPT_BIT(0, "remove-section", &actions, "remove a section: name", ACTION_REMOVE_SECTION),
 	OPT_BIT('l', "list", &actions, "list all", ACTION_LIST),
+	OPT_BIT(0, "list-with-sources", &actions, "list all variables with sources", ACTION_LIST_WITH_SOURCES),
 	OPT_BIT('e', "edit", &actions, "opens an editor", ACTION_EDIT),
 	OPT_STRING(0, "get-color", &get_color_slot, "slot", "find the color configured: [default]"),
 	OPT_STRING(0, "get-colorbool", &get_colorbool_slot, "slot", "find the color setting: [stdout-is-tty]"),
@@ -86,6 +89,18 @@ static void check_argc(int argc, int min, int max) {
 
 static int show_all_config(const char *key_, const char *value_, void *cb)
 {
+	int show_sources = *(int *)cb;
+
+	if (show_sources) {
+		const char *fn = current_config_filename();
+		if (!fn)
+			fn = "";
+		if (end_null)
+			printf("%s%c", fn, '\0');
+		else
+			write_name_quoted(fn, stdout, '\t');
+	}
+
 	if (value_)
 		printf("%s%c%s%c", key_, delim, value_, term);
 	else
@@ -418,9 +433,10 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			usage_with_options(builtin_config_usage, builtin_config_options);
 		}
 
-	if (actions == ACTION_LIST) {
+	if (actions == ACTION_LIST || actions == ACTION_LIST_WITH_SOURCES) {
+		int show_sources = actions & ACTION_LIST_WITH_SOURCES ? 1 : 0;
 		check_argc(argc, 0, 0);
-		if (git_config(show_all_config, NULL) < 0) {
+		if (git_config(show_all_config, &show_sources) < 0) {
 			if (config_exclusive_filename)
 				die_errno("unable to read config file '%s'",
 					  config_exclusive_filename);
diff --git a/cache.h b/cache.h
index 9bd8c2d..98b1d09 100644
--- a/cache.h
+++ b/cache.h
@@ -1138,6 +1138,7 @@ extern const char *get_log_output_encoding(void);
 extern const char *get_commit_output_encoding(void);
 
 extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
+extern const char *current_config_filename(void);
 
 extern const char *config_exclusive_filename;
 
diff --git a/config.c b/config.c
index 40f9c6d..7b4094b 100644
--- a/config.c
+++ b/config.c
@@ -1564,3 +1564,10 @@ int config_error_nonbool(const char *var)
 {
 	return error("Missing value for '%s'", var);
 }
+
+const char *current_config_filename(void)
+{
+	if (cf && cf->name)
+		return cf->name;
+	return NULL;
+}
-- 
1.7.9.rc2.14.g3da2b

Re: [PATCH 0/2] config includes, take 2

[Jakub Narebski]

On Thu, 9 Feb 2012, Jeff King wrote:
> On Tue, Feb 07, 2012 at 11:16:47AM -0800, Jakub Narebski wrote:
> > Jeff King <peff@peff.net> writes:
> > 
> > [...]
> > > Git-config could potentially help with that (and even simplify the
> > > current code) by allowing something like:
> > > 
> > >   $ git config --list-with-sources
> > >   /home/peff/.gitconfig user.name=Jeff King
> > >   /home/peff/.gitconfig user.email=peff@peff.net
> > >   .git/config core.repositoryformatversion=0
> > >   .git/config core.bare=false
> > >   [etc]
> > > 
> > > (you would use the "-z" form, of course, and the filenames would be
> > > NUL-separated, but I made up a human-readable output format above for
> > > illustration purposes).
> > 
> > That would be _very_ nice to have (even without includes support).
> > 
> > Filenames would be git-quoted like in ls-tree / diff-tree output without -z,
> > isn't it?  And is that TAB or SPC as a separator?
> 
> So the patch would look something like this. However, is the actual
> filename really what callers want? It seems like in David's case, an
> annotation of "repo", "global", or "system" (possibly in addition to the
> filename) would be the most useful (because in the git-cola UI, it is
> still nice to list things as "repo" or "global" instead of spewing the
> whole filename at the user -- but you would still want the individual
> filename for handling updates of includes).

I'm not sure if "system" / "global" / "local" or "repo" would be a good
idea.

First, in the case of includes you would have to provide pathnames of
included files.  This would introduce inconsistency.  Is "system"
the '/etc/gitconfig' file, or 'system' file in '.git' directory?

Second, people can have different build configuration, e.g. the prefix
might differ, so that "system" is not always '/etc/gitconfig'.  If you
want to edit config you would want to know which file to edit... and though
there is "git config --system --edit" it depends on having editor
configured correctly.


Just my two cents.
-- 
Jakub Narebski
Poland

Re: [PATCH 0/2] config includes, take 2

[Jeff King]

On Thu, Feb 09, 2012 at 08:24:42PM +0100, Jakub Narebski wrote:

> > So the patch would look something like this. However, is the actual
> > filename really what callers want? It seems like in David's case, an
> > annotation of "repo", "global", or "system" (possibly in addition to the
> > filename) would be the most useful (because in the git-cola UI, it is
> > still nice to list things as "repo" or "global" instead of spewing the
> > whole filename at the user -- but you would still want the individual
> > filename for handling updates of includes).
> 
> I'm not sure if "system" / "global" / "local" or "repo" would be a good
> idea.
> 
> First, in the case of includes you would have to provide pathnames of
> included files.  This would introduce inconsistency.  Is "system"
> the '/etc/gitconfig' file, or 'system' file in '.git' directory?

Yeah, it would have to be syntactically unambiguous with the filename.
I was thinking something of just including both, like this:

  global:/home/peff/.gitconfig<TAB>include.path=other-file
  global:/home/peff/other-file<TAB>some.key=value

That is, give a "context" (repo, global, system) to each lookup, and
then mention the individual file as well (either because it is the root
of that context, or because it was included). So a config editor could
present the context to the user as a purely decorative thing (i.e., tell
the user "these options affect all of your repos"), but use the filename
to actually update the values (i.e., "git config -f
/home/peff/other-file some.key newvalue").

> Second, people can have different build configuration, e.g. the prefix
> might differ, so that "system" is not always '/etc/gitconfig'.  If you
> want to edit config you would want to know which file to edit... and though
> there is "git config --system --edit" it depends on having editor
> configured correctly.

Without includes, something like git-cola could possibly get away with:

  git config --system some.key value

but that doesn't work for included files; for those, you'd want to have
the actual filename.

-Peff