Re: The config include mechanism doesn't allow for overwriting

[Jeff King <peff@peff.net>]

On Mon, Oct 22, 2012 at 05:55:00PM +0200, Ævar Arnfjörð Bjarmason wrote:

> I was hoping to write something like this:
> 
>     [user]
>         name = Luser
>         email = some-default@example.com
>     [include]
>         path = ~/.gitconfig.d/user-email
> 
> Where that file would contain:
> 
>     [user]
>         email = local-email@example.com

The intent is that it would work as you expect, and produce
local-email@example.com.

> But when you do that git prints:
> 
>     $ git config --get user.email
>      some-default@example.com
>      error: More than one value for the key user.email: local-email@example.com

Ugh. The config code just feeds all the values sequentially to the
callback. The normal callbacks within git will overwrite old values,
whether from earlier in the file, from a file with lower priority (e.g.,
/etc/gitconfig versus ~/.gitconfig), or from an earlier included. Which
you can check with:

  $ git var GIT_AUTHOR_IDENT
  Luser <local-email@example.com> 1350936694 -0400

But git-config takes it upon itself to detect duplicates in its
callback. Which is just silly, since it is not something that regular
git would do. git-config should behave as much like the internal git
parser as possible.

> I think config inclusion is much less useful when you can't clobber
> previously assigned values.

Agreed. But I think the bug is in git-config, not in the include
mechanism. I think I'd like to do something like the patch below, which
just reuses the regular config code for git-config, collects the values,
and then reports them. It does mean we use a little more memory (for the
sake of simplicity, we store values instead of streaming them out), but
the code is much shorter, less confusing, and automatically matches what
regular git_config() does.

It fails a few tests in t1300, but it looks like those tests are testing
for the behavior we have identified as wrong, and should be fixed.

---
 builtin/config.c | 111 ++++++++++++-----------------------
 1 file changed, 38 insertions(+), 73 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index d6a066b..72cb0a8 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -16,7 +16,6 @@ static int do_not_match;
 static int use_key_regexp;
 static int do_all;
 static int do_not_match;
-static int seen;
 static char delim = '=';
 static char key_delim = ' ';
 static char term = '\n';
@@ -110,12 +109,19 @@ static int show_config(const char *key_, const char *value_, void *cb)
 	return 0;
 }
 
-static int show_config(const char *key_, const char *value_, void *cb)
+struct strbuf_list {
+	struct strbuf *items;
+	int nr;
+	int alloc;
+};
+
+static int collect_config(const char *key_, const char *value_, void *cb)
 {
+	struct strbuf_list *values = cb;
+	struct strbuf *buf;
 	char value[256];
 	const char *vptr = value;
 	int must_free_vptr = 0;
-	int dup_error = 0;
 	int must_print_delim = 0;
 
 	if (!use_key_regexp && strcmp(key_, key))
@@ -126,12 +132,15 @@ static int show_config(const char *key_, const char *value_, void *cb)
 	    (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0)))
 		return 0;
 
+	ALLOC_GROW(values->items, values->nr + 1, values->alloc);
+	buf = &values->items[values->nr++];
+	strbuf_init(buf, 0);
+
 	if (show_keys) {
-		printf("%s", key_);
+		strbuf_addstr(buf, key_);
 		must_print_delim = 1;
 	}
-	if (seen && !do_all)
-		dup_error = 1;
+
 	if (types == TYPE_INT)
 		sprintf(value, "%d", git_config_int(key_, value_?value_:""));
 	else if (types == TYPE_BOOL)
@@ -153,16 +162,12 @@ static int show_config(const char *key_, const char *value_, void *cb)
 		vptr = "";
 		must_print_delim = 0;
 	}
-	seen++;
-	if (dup_error) {
-		error("More than one value for the key %s: %s",
-				key_, vptr);
-	}
-	else {
-		if (must_print_delim)
-			printf("%c", key_delim);
-		printf("%s%c", vptr, term);
-	}
+
+	if (must_print_delim)
+		strbuf_addch(buf, key_delim);
+	strbuf_addstr(buf, vptr);
+	strbuf_addch(buf, term);
+
 	if (must_free_vptr)
 		/* If vptr must be freed, it's a pointer to a
 		 * dynamically allocated buffer, it's safe to cast to
@@ -175,20 +180,8 @@ static int get_value(const char *key_, const char *regex_)
 
 static int get_value(const char *key_, const char *regex_)
 {
-	int ret = CONFIG_GENERIC_ERROR;
-	char *global = NULL, *xdg = NULL, *repo_config = NULL;
-	const char *system_wide = NULL, *local;
-	struct config_include_data inc = CONFIG_INCLUDE_INIT;
-	config_fn_t fn;
-	void *data;
-
-	local = given_config_file;
-	if (!local) {
-		local = repo_config = git_pathdup("config");
-		if (git_config_system())
-			system_wide = git_etc_gitconfig();
-		home_config_paths(&global, &xdg, "config");
-	}
+	struct strbuf_list values = {0};
+	int i;
 
 	if (use_key_regexp) {
 		char *tl;
@@ -211,14 +204,11 @@ static int get_value(const char *key_, const char *regex_)
 		if (regcomp(key_regexp, key, REG_EXTENDED)) {
 			fprintf(stderr, "Invalid key pattern: %s\n", key_);
 			free(key);
-			ret = CONFIG_INVALID_PATTERN;
-			goto free_strings;
+			return CONFIG_INVALID_PATTERN;
 		}
 	} else {
-		if (git_config_parse_key(key_, &key, NULL)) {
-			ret = CONFIG_INVALID_KEY;
-			goto free_strings;
-		}
+		if (git_config_parse_key(key_, &key, NULL))
+			return CONFIG_INVALID_KEY;
 	}
 
 	if (regex_) {
@@ -230,37 +220,12 @@ static int get_value(const char *key_, const char *regex_)
 		regexp = (regex_t*)xmalloc(sizeof(regex_t));
 		if (regcomp(regexp, regex_, REG_EXTENDED)) {
 			fprintf(stderr, "Invalid pattern: %s\n", regex_);
-			ret = CONFIG_INVALID_PATTERN;
-			goto free_strings;
+			return CONFIG_INVALID_PATTERN;
 		}
 	}
 
-	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(fn, system_wide, data);
-	if (do_all && xdg)
-		git_config_from_file(fn, xdg, data);
-	if (do_all && global)
-		git_config_from_file(fn, global, data);
-	if (do_all)
-		git_config_from_file(fn, local, data);
-	git_config_from_parameters(fn, data);
-	if (!do_all && !seen)
-		git_config_from_file(fn, local, data);
-	if (!do_all && !seen && global)
-		git_config_from_file(fn, global, data);
-	if (!do_all && !seen && xdg)
-		git_config_from_file(fn, xdg, data);
-	if (!do_all && !seen && system_wide)
-		git_config_from_file(fn, system_wide, data);
+	git_config_with_options(collect_config, &values,
+				given_config_file, respect_includes);
 
 	free(key);
 	if (regexp) {
@@ -268,16 +233,16 @@ static int get_value(const char *key_, const char *regex_)
 		free(regexp);
 	}
 
-	if (do_all)
-		ret = !seen;
-	else
-		ret = (seen == 1) ? 0 : seen > 1 ? 2 : 1;
+	if (!values.nr)
+		return 1;
 
-free_strings:
-	free(repo_config);
-	free(global);
-	free(xdg);
-	return ret;
+	for (i = 0; i < values.nr; i++) {
+		struct strbuf *buf = values.items + i;
+		if (do_all || i == values.nr - 1)
+			fwrite(buf->buf, 1, buf->len, stdout);
+		strbuf_release(buf);
+	}
+	return 0;
 }
 
 static char *normalize_value(const char *key, const char *value)