The config include mechanism doesn't allow for overwriting

The config include mechanism doesn't allow for overwriting

[Ævar Arnfjörð Bjarmason]

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

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

I couldn't find information in either the commt that introduced the
feature or the documentation explaining whether this was the intent or
not.

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

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

[Jeff King]

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)

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

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 22, 2012 at 11:15 PM, Jeff King <peff@peff.net> wrote:
> 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.

I think this patch looks good.

One other thing I think is worth clarifying (and I think should be
broken) is if you write a configuration like:

    [foo]
        bar = one
    [foo]
        bar = two
    [foo]
        bar = three

"git-{config,var} -l" will both give you:

    foo.bar=one
    foo.bar=two
    foo.bar=three

And git config --get foo.bar will give you:

    $ git config -f /tmp/test --get foo.bar
    one
    error: More than one value for the key foo.bar: two
    error: More than one value for the key foo.bar: three

I think that it would be better if the config mechanism just silently
overwrote keys that clobbered earlier keys like your patch does.

But in addition can we simplify things for the consumers of
"git-{config,var} -l" by only printing:

    foo.bar=three

Or are there too many variables like "include.path" that can
legitimately appear more than once.

[PATCH 0/8] fix git-config with duplicate variable entries

[Jeff King]

On Tue, Oct 23, 2012 at 04:13:44PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > 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.
> 
> I think this patch looks good.

Thanks. It had a few minor flaws (like a memory leak). I fixed those,
updated the tests, and split it out into a few more readable commits. In
the process, I managed to uncover and fix a few other memory leaks in
the area. I think this version is much more readable, and writing the
rationale for patch 7 convinced me that it's the right thing to do.
Another round of review would be appreciated.

  [1/8]: t1300: style updates
  [2/8]: t1300: remove redundant test
  [3/8]: t1300: test "git config --get-all" more thoroughly
  [4/8]: git-config: remove memory leak of key regexp
  [5/8]: git-config: fix regexp memory leaks on error conditions
  [6/8]: git-config: collect values instead of immediately printing
  [7/8]: git-config: do not complain about duplicate entries
  [8/8]: git-config: use git_config_with_options

For those just joining us, the interesting bit is patch 7, which fixes
some inconsistencies between the "git-config" tool and how the internal
config callbacks work.

> One other thing I think is worth clarifying (and I think should be
> broken) is if you write a configuration like:
> 
>     [foo]
>         bar = one
>     [foo]
>         bar = two
>     [foo]
>         bar = three
> 
> "git-{config,var} -l" will both give you:
> 
>     foo.bar=one
>     foo.bar=two
>     foo.bar=three

Yes, that looks right.

> And git config --get foo.bar will give you:
> 
>     $ git config -f /tmp/test --get foo.bar
>     one
>     error: More than one value for the key foo.bar: two
>     error: More than one value for the key foo.bar: three
> 
> I think that it would be better if the config mechanism just silently
> overwrote keys that clobbered earlier keys like your patch does.

Right.

> But in addition can we simplify things for the consumers of
> "git-{config,var} -l" by only printing:
> 
>     foo.bar=three
> 
> Or are there too many variables like "include.path" that can
> legitimately appear more than once.

No. Some variables can legitimately appear multiple times. E.g.,
remote.*.fetch, remote.*.push, and remote.*.url. Probably more that I am
forgetting. There are not many, but they do exist.

It's OK to tweak the regular "get" for them, since they are already
broken for that case[1]. You need to use "--get-all" if you expect the
variable to have multiple values.  But when we are listing, we do not
have the hint as to what is expected, and we need to show all entries.

-Peff

[1] So the one useful thing that the current duplicate check is doing is
    flagging errors where you wanted to use --get-all, but forgot to.
    However, it's not really a sufficient safeguard anyway, since it
    would not catch cases where the list was split across multiple
    files (which does work with the internal callbacks that handle
    lists, since they never even see that multiple files are involved).
    It's much more important for git-config to be consistent with the
    internal parsing behavior.

[PATCH 1/8] t1300: style updates

[Jeff King]

The t1300 test script is quite old, and does not use our
modern techniques or styles. This patch updates it in the
following ways:

  1. Use test_cmp instead of cmp (to make failures easier to
     debug).

  2. Use test_cmp instead of 'test $(command) = expected'.
     This makes failures much easier to debug, and also
     makes sure that $(command) exits appropriately.

  3. Write tests with the usual style of:

       test_expect_success 'test name' '
               test commands &&
	       ...
       '

     rather than one-liners, or using backslash-continuation.
     This is purely a style fixup.

There are still a few command happening outside of
test_expect invocations, but they are all innoccuous system
commands like "cat" and "cp". In an ideal world, each test
would be self sufficient and all commands would happen
inside test_expect, but it is not immediately obvious how
the grouping should work (some of the commands impact the
subsequent tests, and some of them are setting up and
modifying state that many tests depend on). This patch just
picks the low-hanging style fruit, and we can do more fixes
on top later.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t1300-repo-config.sh | 185 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 113 insertions(+), 72 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index e127f35..e12dd4a 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -55,11 +55,13 @@ test_expect_success 'replace with non-match' \
 	test_cmp expect .git/config
 '
 
-test_expect_success 'replace with non-match' \
-	'git config core.penguin kingpin !blue'
+test_expect_success 'replace with non-match' '
+	git config core.penguin kingpin !blue
+'
 
-test_expect_success 'replace with non-match (actually matching)' \
-	'git config core.penguin "very blue" !kingpin'
+test_expect_success 'replace with non-match (actually matching)' '
+	git config core.penguin "very blue" !kingpin
+'
 
 cat > expect << EOF
 [core]
@@ -108,8 +110,9 @@ EOF
 lines
 EOF
 
-test_expect_success 'unset with cont. lines' \
-	'git config --unset beta.baz'
+test_expect_success 'unset with cont. lines' '
+	git config --unset beta.baz
+'
 
 cat > expect <<\EOF
 [alpha]
@@ -133,8 +136,9 @@ cp .git/config .git/config2
 
 cp .git/config .git/config2
 
-test_expect_success 'multiple unset' \
-	'git config --unset-all beta.haha'
+test_expect_success 'multiple unset' '
+	git config --unset-all beta.haha
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -145,7 +149,9 @@ EOF
 [nextSection] noNewline = ouch
 EOF
 
-test_expect_success 'multiple unset is correct' 'test_cmp expect .git/config'
+test_expect_success 'multiple unset is correct' '
+	test_cmp expect .git/config
+'
 
 cp .git/config2 .git/config
 
@@ -156,8 +162,9 @@ rm .git/config2
 
 rm .git/config2
 
-test_expect_success '--replace-all' \
-	'git config --replace-all beta.haha gamma'
+test_expect_success '--replace-all' '
+	git config --replace-all beta.haha gamma
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -169,7 +176,9 @@ EOF
 [nextSection] noNewline = ouch
 EOF
 
-test_expect_success 'all replaced' 'test_cmp expect .git/config'
+test_expect_success 'all replaced' '
+	test_cmp expect .git/config
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -200,7 +209,11 @@ test_expect_success 'really really mean test' '
 	test_cmp expect .git/config
 '
 
-test_expect_success 'get value' 'test alpha = $(git config beta.haha)'
+test_expect_success 'get value' '
+	echo alpha >expect &&
+	git config beta.haha >actual &&
+	test_cmp expect actual
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -231,18 +244,21 @@ test_expect_success 'ambiguous get' '
 	test_cmp expect .git/config
 '
 
-test_expect_success 'non-match' \
-	'git config --get nextsection.nonewline !for'
+test_expect_success 'non-match' '
+	git config --get nextsection.nonewline !for
+'
 
-test_expect_success 'non-match value' \
-	'test wow = $(git config --get nextsection.nonewline !for)'
+test_expect_success 'non-match value' '
+	test wow = $(git config --get nextsection.nonewline !for)
+'
 
 test_expect_success 'ambiguous get' '
 	test_must_fail git config --get nextsection.nonewline
 '
 
-test_expect_success 'get multivar' \
-	'git config --get-all nextsection.nonewline'
+test_expect_success 'get multivar' '
+	git config --get-all nextsection.nonewline
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -290,8 +306,9 @@ test_expect_success 'correct key' 'git config 123456.a123 987'
 
 test_expect_success 'correct key' 'git config 123456.a123 987'
 
-test_expect_success 'hierarchical section' \
-	'git config Version.1.2.3eX.Alpha beta'
+test_expect_success 'hierarchical section' '
+	git config Version.1.2.3eX.Alpha beta
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -307,7 +324,9 @@ EOF
 	Alpha = beta
 EOF
 
-test_expect_success 'hierarchical section value' 'test_cmp expect .git/config'
+test_expect_success 'hierarchical section value' '
+	test_cmp expect .git/config
+'
 
 cat > expect << EOF
 beta.noindent=sillyValue
@@ -316,9 +335,10 @@ EOF
 version.1.2.3eX.alpha=beta
 EOF
 
-test_expect_success 'working --list' \
-	'git config --list > output && cmp output expect'
-
+test_expect_success 'working --list' '
+	git config --list > output &&
+	test_cmp expect output
+'
 cat > expect << EOF
 EOF
 
@@ -332,8 +352,9 @@ EOF
 nextsection.nonewline wow2 for me
 EOF
 
-test_expect_success '--get-regexp' \
-	'git config --get-regexp in > output && cmp output expect'
+test_expect_success '--get-regexp' '
+	git config --get-regexp in > output && test_cmp expect output
+'
 
 cat > expect << EOF
 wow2 for me
@@ -353,41 +374,47 @@ echo false > expect
 	variable =
 EOF
 
-test_expect_success 'get variable with no value' \
-	'git config --get novalue.variable ^$'
+test_expect_success 'get variable with no value' '
+	git config --get novalue.variable ^$
+'
 
-test_expect_success 'get variable with empty value' \
-	'git config --get emptyvalue.variable ^$'
+test_expect_success 'get variable with empty value' '
+	git config --get emptyvalue.variable ^$
+'
 
 echo novalue.variable > expect
 
-test_expect_success 'get-regexp variable with no value' \
-	'git config --get-regexp novalue > output &&
-	 cmp output expect'
+test_expect_success 'get-regexp variable with no value' '
+	git config --get-regexp novalue > output &&
+	 test_cmp expect output'
 
 echo 'novalue.variable true' > expect
 
-test_expect_success 'get-regexp --bool variable with no value' \
-	'git config --bool --get-regexp novalue > output &&
-	 cmp output expect'
+test_expect_success 'get-regexp --bool variable with no value' '
+	git config --bool --get-regexp novalue > output &&
+	test_cmp expect output
+'
 
 echo 'emptyvalue.variable ' > expect
 
-test_expect_success 'get-regexp variable with empty value' \
-	'git config --get-regexp emptyvalue > output &&
-	 cmp output expect'
+test_expect_success 'get-regexp variable with empty value' '
+	git config --get-regexp emptyvalue > output &&
+	test_cmp expect output
+'
 
 echo true > expect
 
-test_expect_success 'get bool variable with no value' \
-	'git config --bool novalue.variable > output &&
-	 cmp output expect'
+test_expect_success 'get bool variable with no value' '
+	git config --bool novalue.variable > output &&
+	test_cmp expect output
+'
 
 echo false > expect
 
-test_expect_success 'get bool variable with empty value' \
-	'git config --bool emptyvalue.variable > output &&
-	 cmp output expect'
+test_expect_success 'get bool variable with empty value' '
+	git config --bool emptyvalue.variable > output &&
+	test_cmp expect output
+'
 
 test_expect_success 'no arguments, but no crash' '
 	test_must_fail git config >output 2>&1 &&
@@ -427,8 +454,9 @@ test_expect_success 'new variable inserts into proper section' '
 	test_cmp expect .git/config
 '
 
-test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' \
-	'test_must_fail git config --file non-existing-config -l'
+test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' '
+	test_must_fail git config --file non-existing-config -l
+'
 
 cat > other-config << EOF
 [ein]
@@ -444,8 +472,10 @@ test_expect_success 'alternative GIT_CONFIG' '
 	test_cmp expect output
 '
 
-test_expect_success 'alternative GIT_CONFIG (--file)' \
-	'git config --file other-config -l > output && cmp output expect'
+test_expect_success 'alternative GIT_CONFIG (--file)' '
+	git config --file other-config -l > output &&
+	test_cmp expect output
+'
 
 test_expect_success 'refer config from subdirectory' '
 	mkdir x &&
@@ -489,8 +519,9 @@ EOF
 weird
 EOF
 
-test_expect_success "rename section" \
-	"git config --rename-section branch.eins branch.zwei"
+test_expect_success 'rename section' '
+	git config --rename-section branch.eins branch.zwei
+'
 
 cat > expect << EOF
 # Hallo
@@ -503,17 +534,22 @@ test_expect_success "rename succeeded" "test_cmp expect .git/config"
 weird
 EOF
 
-test_expect_success "rename succeeded" "test_cmp expect .git/config"
+test_expect_success 'rename succeeded' '
+	test_cmp expect .git/config
+'
 
-test_expect_success "rename non-existing section" '
+test_expect_success 'rename non-existing section' '
 	test_must_fail git config --rename-section \
 		branch."world domination" branch.drei
 '
 
-test_expect_success "rename succeeded" "test_cmp expect .git/config"
+test_expect_success 'rename succeeded' '
+	test_cmp expect .git/config
+'
 
-test_expect_success "rename another section" \
-	'git config --rename-section branch."1 234 blabl/a" branch.drei'
+test_expect_success 'rename another section' '
+	git config --rename-section branch."1 234 blabl/a" branch.drei
+'
 
 cat > expect << EOF
 # Hallo
@@ -526,14 +562,17 @@ EOF
 weird
 EOF
 
-test_expect_success "rename succeeded" "test_cmp expect .git/config"
+test_expect_success 'rename succeeded' '
+	test_cmp expect .git/config
+'
 
 cat >> .git/config << EOF
 [branch "vier"] z = 1
 EOF
 
-test_expect_success "rename a section with a var on the same line" \
-	'git config --rename-section branch.vier branch.zwei'
+test_expect_success 'rename a section with a var on the same line' '
+	git config --rename-section branch.vier branch.zwei
+'
 
 cat > expect << EOF
 # Hallo
@@ -548,7 +587,9 @@ EOF
 	z = 1
 EOF
 
-test_expect_success "rename succeeded" "test_cmp expect .git/config"
+test_expect_success 'rename succeeded' '
+	test_cmp expect .git/config
+'
 
 test_expect_success 'renaming empty section name is rejected' '
 	test_must_fail git config --rename-section branch.zwei ""
@@ -562,7 +603,9 @@ EOF
   [branch "zwei"] a = 1 [branch "vier"]
 EOF
 
-test_expect_success "remove section" "git config --remove-section branch.zwei"
+test_expect_success 'remove section' '
+	git config --remove-section branch.zwei
+'
 
 cat > expect << EOF
 # Hallo
@@ -571,8 +614,9 @@ EOF
 weird
 EOF
 
-test_expect_success "section was removed properly" \
-	"test_cmp expect .git/config"
+test_expect_success 'section was removed properly' '
+	test_cmp expect .git/config
+'
 
 cat > expect << EOF
 [gitcvs]
@@ -583,7 +627,6 @@ test_expect_success 'section ending' '
 EOF
 
 test_expect_success 'section ending' '
-
 	rm -f .git/config &&
 	git config gitcvs.enabled true &&
 	git config gitcvs.ext.dbname %Ggitcvs1.%a.%m.sqlite &&
@@ -593,7 +636,6 @@ test_expect_success numbers '
 '
 
 test_expect_success numbers '
-
 	git config kilo.gram 1k &&
 	git config mega.ton 1m &&
 	k=$(git config --int --get kilo.gram) &&
@@ -607,7 +649,6 @@ test_expect_success 'invalid unit' '
 EOF
 
 test_expect_success 'invalid unit' '
-
 	git config aninvalid.unit "1auto" &&
 	s=$(git config aninvalid.unit) &&
 	test "z1auto" = "z$s" &&
@@ -616,7 +657,7 @@ test_expect_success 'invalid unit' '
 		echo config should have failed
 		false
 	fi &&
-	cmp actual expect
+	test_cmp actual expect
 '
 
 cat > expect << EOF
@@ -646,7 +687,7 @@ test_expect_success bool '
 	    git config --bool --get bool.true$i >>result
 	    git config --bool --get bool.false$i >>result
         done &&
-	cmp expect result'
+	test_cmp expect result'
 
 test_expect_success 'invalid bool (--get)' '
 
@@ -680,7 +721,7 @@ test_expect_success 'set --bool' '
 	git config --bool bool.false2 "" &&
 	git config --bool bool.false3 nO &&
 	git config --bool bool.false4 FALSE &&
-	cmp expect .git/config'
+	test_cmp expect .git/config'
 
 cat > expect <<\EOF
 [int]
@@ -695,7 +736,7 @@ test_expect_success 'set --int' '
 	git config --int int.val1 01 &&
 	git config --int int.val2 -1 &&
 	git config --int int.val3 5m &&
-	cmp expect .git/config'
+	test_cmp expect .git/config'
 
 cat >expect <<\EOF
 [bool]
@@ -844,7 +885,7 @@ test_expect_success 'value continued on next line' '
 
 test_expect_success 'value continued on next line' '
 	git config --list > result &&
-	cmp result expect
+	test_cmp result expect
 '
 
 cat > .git/config <<\EOF
-- 
1.8.0.3.g3456896

Re: [PATCH 1/8] t1300: style updates

[Johannes Sixt]

Am 10/24/2012 0:35, schrieb Jeff King:
> -test_expect_success 'non-match value' \
> -	'test wow = $(git config --get nextsection.nonewline !for)'
> +test_expect_success 'non-match value' '
> +	test wow = $(git config --get nextsection.nonewline !for)
> +'

Here's a case you forgot to update to test_cmp.

> +test_expect_success 'get-regexp variable with no value' '
> +	git config --get-regexp novalue > output &&
> +	 test_cmp expect output'

And while you are here, you might want to remove this extra space. ;)

Otherwise, looks fine.

-- Hannes

Re: [PATCH 1/8] t1300: style updates

[Jeff King]

On Wed, Oct 24, 2012 at 08:33:15AM +0200, Johannes Sixt wrote:

> Am 10/24/2012 0:35, schrieb Jeff King:
> > -test_expect_success 'non-match value' \
> > -	'test wow = $(git config --get nextsection.nonewline !for)'
> > +test_expect_success 'non-match value' '
> > +	test wow = $(git config --get nextsection.nonewline !for)
> > +'
> 
> Here's a case you forgot to update to test_cmp.

Thanks. I noticed I left quite a few of those in (the other changes I
did mechanically, but I only fixed up the "test" ones in nearby spots).

> > +test_expect_success 'get-regexp variable with no value' '
> > +	git config --get-regexp novalue > output &&
> > +	 test_cmp expect output'
> 
> And while you are here, you might want to remove this extra space. ;)
> 
> Otherwise, looks fine.

Thanks, I'll fix up both.

-Peff

[PATCHv2 1/8] t1300: style updates

[Jeff King]

On Wed, Oct 24, 2012 at 02:37:12AM -0400, Jeff King wrote:

> > Here's a case you forgot to update to test_cmp.
> [...]
> > And while you are here, you might want to remove this extra space. ;)
> > 
> > Otherwise, looks fine.
> 
> Thanks, I'll fix up both.

Here's an updated version of patch 1 that I'm planning on queuing. It's
rather tedious to read, but if anybody feels like giving it one more
run-through, let me know if you see any problems.

I won't bother re-posting the other patches, as they are unchanged on
top.

-- >8 --
Subject: [PATCH] t1300: style updates

The t1300 test script is quite old, and does not use our
modern techniques or styles. This patch updates it in the
following ways:

  1. Use test_cmp instead of cmp (to make failures easier to
     debug).

  2. Use test_cmp instead of 'test $(command) = expected'.
     This makes failures much easier to debug, and also
     makes sure that $(command) exits appropriately.

  3. Use test_must_fail (easier to read, and checks more
     rigorously for signal death).

  4. Write tests with the usual style of:

       test_expect_success 'test name' '
               test commands &&
	       ...
       '

     rather than one-liners, or using backslash-continuation.
     This is purely a style fixup.

There are still a few command happening outside of
test_expect invocations, but they are all innoccuous system
commands like "cat" and "cp". In an ideal world, each test
would be self sufficient and all commands would happen
inside test_expect, but it is not immediately obvious how
the grouping should work (some of the commands impact the
subsequent tests, and some of them are setting up and
modifying state that many tests depend on). This patch just
picks the low-hanging style fruit, and we can do more fixes
on top later.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t1300-repo-config.sh | 301 +++++++++++++++++++++++++++++--------------------
 1 file changed, 178 insertions(+), 123 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index e127f35..feb7430 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -55,11 +55,13 @@ test_expect_success 'replace with non-match' \
 	test_cmp expect .git/config
 '
 
-test_expect_success 'replace with non-match' \
-	'git config core.penguin kingpin !blue'
+test_expect_success 'replace with non-match' '
+	git config core.penguin kingpin !blue
+'
 
-test_expect_success 'replace with non-match (actually matching)' \
-	'git config core.penguin "very blue" !kingpin'
+test_expect_success 'replace with non-match (actually matching)' '
+	git config core.penguin "very blue" !kingpin
+'
 
 cat > expect << EOF
 [core]
@@ -108,8 +110,9 @@ EOF
 lines
 EOF
 
-test_expect_success 'unset with cont. lines' \
-	'git config --unset beta.baz'
+test_expect_success 'unset with cont. lines' '
+	git config --unset beta.baz
+'
 
 cat > expect <<\EOF
 [alpha]
@@ -133,8 +136,9 @@ cp .git/config .git/config2
 
 cp .git/config .git/config2
 
-test_expect_success 'multiple unset' \
-	'git config --unset-all beta.haha'
+test_expect_success 'multiple unset' '
+	git config --unset-all beta.haha
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -145,7 +149,9 @@ EOF
 [nextSection] noNewline = ouch
 EOF
 
-test_expect_success 'multiple unset is correct' 'test_cmp expect .git/config'
+test_expect_success 'multiple unset is correct' '
+	test_cmp expect .git/config
+'
 
 cp .git/config2 .git/config
 
@@ -156,8 +162,9 @@ rm .git/config2
 
 rm .git/config2
 
-test_expect_success '--replace-all' \
-	'git config --replace-all beta.haha gamma'
+test_expect_success '--replace-all' '
+	git config --replace-all beta.haha gamma
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -169,7 +176,9 @@ EOF
 [nextSection] noNewline = ouch
 EOF
 
-test_expect_success 'all replaced' 'test_cmp expect .git/config'
+test_expect_success 'all replaced' '
+	test_cmp expect .git/config
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -200,7 +209,11 @@ test_expect_success 'really really mean test' '
 	test_cmp expect .git/config
 '
 
-test_expect_success 'get value' 'test alpha = $(git config beta.haha)'
+test_expect_success 'get value' '
+	echo alpha >expect &&
+	git config beta.haha >actual &&
+	test_cmp expect actual
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -231,18 +244,23 @@ test_expect_success 'ambiguous get' '
 	test_cmp expect .git/config
 '
 
-test_expect_success 'non-match' \
-	'git config --get nextsection.nonewline !for'
+test_expect_success 'non-match' '
+	git config --get nextsection.nonewline !for
+'
 
-test_expect_success 'non-match value' \
-	'test wow = $(git config --get nextsection.nonewline !for)'
+test_expect_success 'non-match value' '
+	echo wow >expect &&
+	git config --get nextsection.nonewline !for >actual &&
+	test_cmp expect actual
+'
 
 test_expect_success 'ambiguous get' '
 	test_must_fail git config --get nextsection.nonewline
 '
 
-test_expect_success 'get multivar' \
-	'git config --get-all nextsection.nonewline'
+test_expect_success 'get multivar' '
+	git config --get-all nextsection.nonewline
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -290,8 +308,9 @@ test_expect_success 'correct key' 'git config 123456.a123 987'
 
 test_expect_success 'correct key' 'git config 123456.a123 987'
 
-test_expect_success 'hierarchical section' \
-	'git config Version.1.2.3eX.Alpha beta'
+test_expect_success 'hierarchical section' '
+	git config Version.1.2.3eX.Alpha beta
+'
 
 cat > expect << EOF
 [beta] ; silly comment # another comment
@@ -307,7 +326,9 @@ EOF
 	Alpha = beta
 EOF
 
-test_expect_success 'hierarchical section value' 'test_cmp expect .git/config'
+test_expect_success 'hierarchical section value' '
+	test_cmp expect .git/config
+'
 
 cat > expect << EOF
 beta.noindent=sillyValue
@@ -316,9 +337,10 @@ EOF
 version.1.2.3eX.alpha=beta
 EOF
 
-test_expect_success 'working --list' \
-	'git config --list > output && cmp output expect'
-
+test_expect_success 'working --list' '
+	git config --list > output &&
+	test_cmp expect output
+'
 cat > expect << EOF
 EOF
 
@@ -332,8 +354,10 @@ EOF
 nextsection.nonewline wow2 for me
 EOF
 
-test_expect_success '--get-regexp' \
-	'git config --get-regexp in > output && cmp output expect'
+test_expect_success '--get-regexp' '
+	git config --get-regexp in >output &&
+	test_cmp expect output
+'
 
 cat > expect << EOF
 wow2 for me
@@ -353,41 +377,48 @@ echo false > expect
 	variable =
 EOF
 
-test_expect_success 'get variable with no value' \
-	'git config --get novalue.variable ^$'
+test_expect_success 'get variable with no value' '
+	git config --get novalue.variable ^$
+'
 
-test_expect_success 'get variable with empty value' \
-	'git config --get emptyvalue.variable ^$'
+test_expect_success 'get variable with empty value' '
+	git config --get emptyvalue.variable ^$
+'
 
 echo novalue.variable > expect
 
-test_expect_success 'get-regexp variable with no value' \
-	'git config --get-regexp novalue > output &&
-	 cmp output expect'
+test_expect_success 'get-regexp variable with no value' '
+	git config --get-regexp novalue > output &&
+	test_cmp expect output
+'
 
 echo 'novalue.variable true' > expect
 
-test_expect_success 'get-regexp --bool variable with no value' \
-	'git config --bool --get-regexp novalue > output &&
-	 cmp output expect'
+test_expect_success 'get-regexp --bool variable with no value' '
+	git config --bool --get-regexp novalue > output &&
+	test_cmp expect output
+'
 
 echo 'emptyvalue.variable ' > expect
 
-test_expect_success 'get-regexp variable with empty value' \
-	'git config --get-regexp emptyvalue > output &&
-	 cmp output expect'
+test_expect_success 'get-regexp variable with empty value' '
+	git config --get-regexp emptyvalue > output &&
+	test_cmp expect output
+'
 
 echo true > expect
 
-test_expect_success 'get bool variable with no value' \
-	'git config --bool novalue.variable > output &&
-	 cmp output expect'
+test_expect_success 'get bool variable with no value' '
+	git config --bool novalue.variable > output &&
+	test_cmp expect output
+'
 
 echo false > expect
 
-test_expect_success 'get bool variable with empty value' \
-	'git config --bool emptyvalue.variable > output &&
-	 cmp output expect'
+test_expect_success 'get bool variable with empty value' '
+	git config --bool emptyvalue.variable > output &&
+	test_cmp expect output
+'
 
 test_expect_success 'no arguments, but no crash' '
 	test_must_fail git config >output 2>&1 &&
@@ -427,8 +458,9 @@ test_expect_success 'new variable inserts into proper section' '
 	test_cmp expect .git/config
 '
 
-test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' \
-	'test_must_fail git config --file non-existing-config -l'
+test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' '
+	test_must_fail git config --file non-existing-config -l
+'
 
 cat > other-config << EOF
 [ein]
@@ -444,8 +476,10 @@ test_expect_success 'alternative GIT_CONFIG' '
 	test_cmp expect output
 '
 
-test_expect_success 'alternative GIT_CONFIG (--file)' \
-	'git config --file other-config -l > output && cmp output expect'
+test_expect_success 'alternative GIT_CONFIG (--file)' '
+	git config --file other-config -l > output &&
+	test_cmp expect output
+'
 
 test_expect_success 'refer config from subdirectory' '
 	mkdir x &&
@@ -489,8 +523,9 @@ EOF
 weird
 EOF
 
-test_expect_success "rename section" \
-	"git config --rename-section branch.eins branch.zwei"
+test_expect_success 'rename section' '
+	git config --rename-section branch.eins branch.zwei
+'
 
 cat > expect << EOF
 # Hallo
@@ -503,17 +538,22 @@ test_expect_success "rename succeeded" "test_cmp expect .git/config"
 weird
 EOF
 
-test_expect_success "rename succeeded" "test_cmp expect .git/config"
+test_expect_success 'rename succeeded' '
+	test_cmp expect .git/config
+'
 
-test_expect_success "rename non-existing section" '
+test_expect_success 'rename non-existing section' '
 	test_must_fail git config --rename-section \
 		branch."world domination" branch.drei
 '
 
-test_expect_success "rename succeeded" "test_cmp expect .git/config"
+test_expect_success 'rename succeeded' '
+	test_cmp expect .git/config
+'
 
-test_expect_success "rename another section" \
-	'git config --rename-section branch."1 234 blabl/a" branch.drei'
+test_expect_success 'rename another section' '
+	git config --rename-section branch."1 234 blabl/a" branch.drei
+'
 
 cat > expect << EOF
 # Hallo
@@ -526,14 +566,17 @@ EOF
 weird
 EOF
 
-test_expect_success "rename succeeded" "test_cmp expect .git/config"
+test_expect_success 'rename succeeded' '
+	test_cmp expect .git/config
+'
 
 cat >> .git/config << EOF
 [branch "vier"] z = 1
 EOF
 
-test_expect_success "rename a section with a var on the same line" \
-	'git config --rename-section branch.vier branch.zwei'
+test_expect_success 'rename a section with a var on the same line' '
+	git config --rename-section branch.vier branch.zwei
+'
 
 cat > expect << EOF
 # Hallo
@@ -548,7 +591,9 @@ EOF
 	z = 1
 EOF
 
-test_expect_success "rename succeeded" "test_cmp expect .git/config"
+test_expect_success 'rename succeeded' '
+	test_cmp expect .git/config
+'
 
 test_expect_success 'renaming empty section name is rejected' '
 	test_must_fail git config --rename-section branch.zwei ""
@@ -562,7 +607,9 @@ EOF
   [branch "zwei"] a = 1 [branch "vier"]
 EOF
 
-test_expect_success "remove section" "git config --remove-section branch.zwei"
+test_expect_success 'remove section' '
+	git config --remove-section branch.zwei
+'
 
 cat > expect << EOF
 # Hallo
@@ -571,8 +618,9 @@ EOF
 weird
 EOF
 
-test_expect_success "section was removed properly" \
-	"test_cmp expect .git/config"
+test_expect_success 'section was removed properly' '
+	test_cmp expect .git/config
+'
 
 cat > expect << EOF
 [gitcvs]
@@ -583,7 +631,6 @@ test_expect_success 'section ending' '
 EOF
 
 test_expect_success 'section ending' '
-
 	rm -f .git/config &&
 	git config gitcvs.enabled true &&
 	git config gitcvs.ext.dbname %Ggitcvs1.%a.%m.sqlite &&
@@ -593,30 +640,25 @@ test_expect_success 'invalid unit' '
 '
 
 test_expect_success numbers '
-
 	git config kilo.gram 1k &&
 	git config mega.ton 1m &&
-	k=$(git config --int --get kilo.gram) &&
-	test z1024 = "z$k" &&
-	m=$(git config --int --get mega.ton) &&
-	test z1048576 = "z$m"
+	echo 1024 >expect &&
+	echo 1048576 >>expect &&
+	git config --int --get kilo.gram >actual &&
+	git config --int --get mega.ton >>actual &&
+	test_cmp expect actual
 '
 
-cat > expect <<EOF
-fatal: bad config value for 'aninvalid.unit' in .git/config
-EOF
-
 test_expect_success 'invalid unit' '
-
 	git config aninvalid.unit "1auto" &&
-	s=$(git config aninvalid.unit) &&
-	test "z1auto" = "z$s" &&
-	if git config --int --get aninvalid.unit 2>actual
-	then
-		echo config should have failed
-		false
-	fi &&
-	cmp actual expect
+	echo 1auto >expect &&
+	git config aninvalid.unit >actual &&
+	test_cmp expect actual &&
+	cat > expect <<-\EOF
+	fatal: bad config value for '\''aninvalid.unit'\'' in .git/config
+	EOF
+	test_must_fail git config --int --get aninvalid.unit 2>actual &&
+	test_cmp actual expect
 '
 
 cat > expect << EOF
@@ -646,7 +688,7 @@ test_expect_success bool '
 	    git config --bool --get bool.true$i >>result
 	    git config --bool --get bool.false$i >>result
         done &&
-	cmp expect result'
+	test_cmp expect result'
 
 test_expect_success 'invalid bool (--get)' '
 
@@ -680,7 +722,7 @@ test_expect_success 'set --bool' '
 	git config --bool bool.false2 "" &&
 	git config --bool bool.false3 nO &&
 	git config --bool bool.false4 FALSE &&
-	cmp expect .git/config'
+	test_cmp expect .git/config'
 
 cat > expect <<\EOF
 [int]
@@ -695,39 +737,37 @@ cat >expect <<\EOF
 	git config --int int.val1 01 &&
 	git config --int int.val2 -1 &&
 	git config --int int.val3 5m &&
-	cmp expect .git/config'
+	test_cmp expect .git/config
+'
 
-cat >expect <<\EOF
-[bool]
-	true1 = true
+test_expect_success 'get --bool-or-int' '
+	cat >.git/config <<-\EOF &&
+	[bool]
+	true1
 	true2 = true
-	false1 = false
-	false2 = false
-[int]
+	false = false
+	[int]
 	int1 = 0
 	int2 = 1
 	int3 = -1
-EOF
-
-test_expect_success 'get --bool-or-int' '
-	rm -f .git/config &&
-	(
-		echo "[bool]"
-		echo true1
-		echo true2 = true
-		echo false = false
-		echo "[int]"
-		echo int1 = 0
-		echo int2 = 1
-		echo int3 = -1
-	) >>.git/config &&
-	test $(git config --bool-or-int bool.true1) = true &&
-	test $(git config --bool-or-int bool.true2) = true &&
-	test $(git config --bool-or-int bool.false) = false &&
-	test $(git config --bool-or-int int.int1) = 0 &&
-	test $(git config --bool-or-int int.int2) = 1 &&
-	test $(git config --bool-or-int int.int3) = -1
-
+	EOF
+	cat >expect <<-\EOF &&
+	true
+	true
+	false
+	0
+	1
+	-1
+	EOF
+	{
+		git config --bool-or-int bool.true1 &&
+		git config --bool-or-int bool.true2 &&
+		git config --bool-or-int bool.false &&
+		git config --bool-or-int int.int1 &&
+		git config --bool-or-int int.int2 &&
+		git config --bool-or-int int.int3
+	} >actual &&
+	test_cmp expect actual
 '
 
 cat >expect <<\EOF
@@ -844,7 +884,7 @@ test_expect_success 'value continued on next line' '
 
 test_expect_success 'value continued on next line' '
 	git config --list > result &&
-	cmp result expect
+	test_cmp result expect
 '
 
 cat > .git/config <<\EOF
@@ -880,11 +920,12 @@ test_expect_success SYMLINKS 'symlinked configuration' '
 
 test_expect_success 'inner whitespace kept verbatim' '
 	git config section.val "foo 	  bar" &&
-	test "z$(git config section.val)" = "zfoo 	  bar"
+	echo "foo 	  bar" >expect &&
+	git config section.val >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success SYMLINKS 'symlinked configuration' '
-
 	ln -s notyet myconfig &&
 	GIT_CONFIG=myconfig git config test.frotz nitfol &&
 	test -h myconfig &&
@@ -893,9 +934,15 @@ test_expect_success SYMLINKS 'symlinked configuration' '
 	GIT_CONFIG=myconfig git config test.xyzzy rezrov &&
 	test -h myconfig &&
 	test -f notyet &&
-	test "z$(GIT_CONFIG=notyet git config test.frotz)" = znitfol &&
-	test "z$(GIT_CONFIG=notyet git config test.xyzzy)" = zrezrov
-
+	cat >expect <<-\EOF &&
+	nitfol
+	rezrov
+	EOF
+	{
+		GIT_CONFIG=notyet git config test.frotz &&
+		GIT_CONFIG=notyet git config test.xyzzy
+	} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'nonexistent configuration' '
@@ -927,12 +974,20 @@ test_expect_success 'git -c "key=value" support' '
 	git commit -m 'initial commit' &&
 	git config branch.master.mergeoptions 'echo \"' &&
 	test_must_fail git merge master
-	"
+"
 
 test_expect_success 'git -c "key=value" support' '
-	test "z$(git -c core.name=value config core.name)" = zvalue &&
-	test "z$(git -c foo.CamelCase=value config foo.camelcase)" = zvalue &&
-	test "z$(git -c foo.flag config --bool foo.flag)" = ztrue &&
+	cat >expect <<-\EOF &&
+	value
+	value
+	true
+	EOF
+	{
+		git -c core.name=value config core.name &&
+		git -c foo.CamelCase=value config foo.camelcase &&
+		git -c foo.flag config --bool foo.flag
+	} >actual &&
+	test_cmp expect actual &&
 	test_must_fail git -c name=value config core.name
 '
 
-- 
1.8.0.3.g3456896

[PATCH 2/8] t1300: remove redundant test

[Jeff King]

This test checks that git-config fails for an ambiguous
"get", but we check the exact same thing 3 tests beforehand.

Signed-off-by: Jeff King <peff@peff.net>
---
I update the matching test later in the series, and I didn't want to
have to do it twice.

 t/t1300-repo-config.sh | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index e12dd4a..c6489dc 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -275,10 +275,6 @@ test_expect_success 'multivar replace' '
 	test_cmp expect .git/config
 '
 
-test_expect_success 'ambiguous value' '
-	test_must_fail git config nextsection.nonewline
-'
-
 test_expect_success 'ambiguous unset' '
 	test_must_fail git config --unset nextsection.nonewline
 '
-- 
1.8.0.3.g3456896

[PATCH 3/8] t1300: test "git config --get-all" more thoroughly

[Jeff King]

We check that we can "--get-all" a multi-valued variable,
but we do not actually confirm that the output is sensible.
Doing so reveals that it works fine, but this will help us
ensure we do not have regressions in the next few patches,
which will touch this area.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t1300-repo-config.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index c6489dc..74a297e 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -256,8 +256,13 @@ test_expect_success 'ambiguous get' '
 	test_must_fail git config --get nextsection.nonewline
 '
 
-test_expect_success 'get multivar' '
-	git config --get-all nextsection.nonewline
+test_expect_success 'multi-valued get-all returns all' '
+	cat >expect <<-\EOF &&
+	wow
+	wow2 for me
+	EOF
+	git config --get-all nextsection.nonewline >actual &&
+	test_cmp expect actual
 '
 
 cat > expect << EOF
-- 
1.8.0.3.g3456896

[PATCH 4/8] git-config: remove memory leak of key regexp

[Jeff King]

This is only called once per invocation, so it's not a major
leak, but it's easy to fix.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/config.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/config.c b/builtin/config.c
index e1c33e0..e660d48 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -248,6 +248,10 @@ static int get_value(const char *key_, const char *regex_)
 		git_config_from_file(fn, system_wide, data);
 
 	free(key);
+	if (key_regexp) {
+		regfree(key_regexp);
+		free(key_regexp);
+	}
 	if (regexp) {
 		regfree(regexp);
 		free(regexp);
-- 
1.8.0.3.g3456896

[PATCH 5/8] git-config: fix regexp memory leaks on error conditions

[Jeff King]

The get_value function has a goto label for cleaning up on
errors, but it only cleans up half of what the function
might allocate. Let's also clean up the key and regexp
variables there.

Note that we need to take special care when compiling the
regex fails to clean it up ourselves, since it is in a
half-constructed state (we would want to free it, but not
regfree it).

Similarly, we fix git_config_parse_key to return NULL when it
fails, not a pointer to some already-freed memory.

Signed-off-by: Jeff King <peff@peff.net>
---
The diff is annoying in an interesting way: what I actually did was move
the regex cleanup code down, but it shows it as moving the bottom bits
up. I think it is just one of those ambiguous cases where either way is
equally valid and minimal.

 builtin/config.c | 23 +++++++++++++----------
 config.c         |  1 +
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index e660d48..60d36e7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -195,7 +195,8 @@ static int get_value(const char *key_, const char *regex_)
 		key_regexp = (regex_t*)xmalloc(sizeof(regex_t));
 		if (regcomp(key_regexp, key, REG_EXTENDED)) {
 			fprintf(stderr, "Invalid key pattern: %s\n", key_);
-			free(key);
+			free(key_regexp);
+			key_regexp = NULL;
 			ret = CONFIG_INVALID_PATTERN;
 			goto free_strings;
 		}
@@ -215,6 +216,8 @@ 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_);
+			free(regexp);
+			regexp = NULL;
 			ret = CONFIG_INVALID_PATTERN;
 			goto free_strings;
 		}
@@ -247,6 +250,15 @@ static int get_value(const char *key_, const char *regex_)
 	if (!do_all && !seen && system_wide)
 		git_config_from_file(fn, system_wide, data);
 
+	if (do_all)
+		ret = !seen;
+	else
+		ret = (seen == 1) ? 0 : seen > 1 ? 2 : 1;
+
+free_strings:
+	free(repo_config);
+	free(global);
+	free(xdg);
 	free(key);
 	if (key_regexp) {
 		regfree(key_regexp);
@@ -257,15 +269,6 @@ 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;
-
-free_strings:
-	free(repo_config);
-	free(global);
-	free(xdg);
 	return ret;
 }
 
diff --git a/config.c b/config.c
index 08e47e2..2fbe634 100644
--- a/config.c
+++ b/config.c
@@ -1280,6 +1280,7 @@ out_free_ret_1:
 
 out_free_ret_1:
 	free(*store_key);
+	*store_key = NULL;
 	return -CONFIG_INVALID_KEY;
 }
 
-- 
1.8.0.3.g3456896

[PATCH 6/8] git-config: collect values instead of immediately printing

[Jeff King]

This is a refactor that will allow us to more easily tweak
the behavior for multi-valued variables, and it will
ultimately allow us to remove a lot git-config's custom code
in favor of the regular git_config code.

It does mean we're no longer streaming, and we're storing
more in memory for the --get-all case, but in practice it is
a tiny amount of data, and the results are instantaneous.

Signed-off-by: Jeff King <peff@peff.net>
---
The increase in line count is nicely offset by the next two patches.

 builtin/config.c | 50 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 60d36e7..08e83fc 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -15,7 +15,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';
@@ -95,8 +94,16 @@ 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;
@@ -111,11 +118,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)
+	if (values->nr > 1 && !do_all)
 		dup_error = 1;
 	if (types == TYPE_INT)
 		sprintf(value, "%d", git_config_int(key_, value_?value_:""));
@@ -138,15 +149,15 @@ 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);
+			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
@@ -166,6 +177,8 @@ static int get_value(const char *key_, const char *regex_)
 	struct config_include_data inc = CONFIG_INCLUDE_INIT;
 	config_fn_t fn;
 	void *data;
+	struct strbuf_list values = {0};
+	int i;
 
 	local = given_config_file;
 	if (!local) {
@@ -223,8 +236,8 @@ static int get_value(const char *key_, const char *regex_)
 		}
 	}
 
-	fn = show_config;
-	data = NULL;
+	fn = collect_config;
+	data = &values;
 	if (respect_includes) {
 		inc.fn = fn;
 		inc.data = data;
@@ -241,19 +254,26 @@ static int get_value(const char *key_, const char *regex_)
 	if (do_all)
 		git_config_from_file(fn, local, data);
 	git_config_from_parameters(fn, data);
-	if (!do_all && !seen)
+	if (!do_all && !values.nr)
 		git_config_from_file(fn, local, data);
-	if (!do_all && !seen && global)
+	if (!do_all && !values.nr && global)
 		git_config_from_file(fn, global, data);
-	if (!do_all && !seen && xdg)
+	if (!do_all && !values.nr && xdg)
 		git_config_from_file(fn, xdg, data);
-	if (!do_all && !seen && system_wide)
+	if (!do_all && !values.nr && system_wide)
 		git_config_from_file(fn, system_wide, data);
 
 	if (do_all)
-		ret = !seen;
+		ret = !values.nr;
 	else
-		ret = (seen == 1) ? 0 : seen > 1 ? 2 : 1;
+		ret = (values.nr == 1) ? 0 : values.nr > 1 ? 2 : 1;
+
+	for (i = 0; i < values.nr; i++) {
+		struct strbuf *buf = values.items + i;
+		fwrite(buf->buf, 1, buf->len, stdout);
+		strbuf_release(buf);
+	}
+	free(values.items);
 
 free_strings:
 	free(repo_config);
-- 
1.8.0.3.g3456896

[PATCH 7/8] git-config: do not complain about duplicate entries

[Jeff King]

If git-config is asked for a single value, it will complain
and exit with an error if it finds multiple instances of
that value. This is unlike the usual internal config
parsing, however, which will generally overwrite previous
values, leaving only the final one. For example:

  [set a multivar]
  $ git config user.email one@example.com
  $ git config --add user.email two@example.com

  [use the internal parser to fetch it]
  $ git var GIT_AUTHOR_IDENT
  Your Name <two@example.com> ...

  [use git-config to fetch it]
  $ git config user.email
  one@example.com
  error: More than one value for the key user.email: two@example.com

This overwriting behavior is critical for the regular
parser, which starts with the lowest-priority file (e.g.,
/etc/gitconfig) and proceeds to the highest-priority file
($GIT_DIR/config). Overwriting yields the highest priority
value at the end.

Git-config solves this problem by implementing its own
parsing. It goes from highest to lowest priorty, but does
not proceed to the next file if it has seen a value.

So in practice, this distinction never mattered much,
because it only triggered for values in the same file. And
there was not much point in doing that; the real value is in
overwriting values from lower-priority files.

However, this changed with the implementation of config
include files. Now we might see an include overriding a
value from the parent file, which is a sensible thing to do,
but git-config will flag as a duplication.

This patch drops the duplicate detection for git-config and
switches to a pure-overwrite model (for the single case;
--get-all can still be used if callers want to do something
more fancy).

As is shown by the modifications to the test suite, this is
a user-visible change in behavior. An alternative would be
to just change the include case, but this is much cleaner
for a few reasons:

  1. If you change the include case, then to what? If you
     just stop parsing includes after getting a value, then
     you will get a _different_ answer than the regular
     config parser (you'll get the first value instead of
     the last value). So you'd want to implement overwrite
     semantics anyway.

  2. Even though it is a change in behavior for git-config,
     it is bringing us in line with what the internal
     parsers already do.

  3. The file-order reimplementation is the only thing
     keeping us from sharing more code with the internal
     config parser, which will help keep differences to a
     minimum.

Going under the assumption that the primary purpose of
git-config is to behave identically to how git's internal
parsing works, this change can be seen as a bug-fix.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/config.c       | 27 +++++++++------------------
 t/t1300-repo-config.sh |  6 ++++--
 t/t9700/test.pl        |  3 +--
 3 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 08e83fc..77efa69 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -107,7 +107,6 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 	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,8 +125,6 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 		strbuf_addstr(buf, key_);
 		must_print_delim = 1;
 	}
-	if (values->nr > 1 && !do_all)
-		dup_error = 1;
 	if (types == TYPE_INT)
 		sprintf(value, "%d", git_config_int(key_, value_?value_:""));
 	else if (types == TYPE_BOOL)
@@ -149,16 +146,12 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 		vptr = "";
 		must_print_delim = 0;
 	}
-	if (dup_error) {
-		error("More than one value for the key %s: %s",
-				key_, vptr);
-	}
-	else {
-		if (must_print_delim)
-			strbuf_addch(buf, key_delim);
-		strbuf_addstr(buf, vptr);
-		strbuf_addch(buf, 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
@@ -263,14 +256,12 @@ static int get_value(const char *key_, const char *regex_)
 	if (!do_all && !values.nr && system_wide)
 		git_config_from_file(fn, system_wide, data);
 
-	if (do_all)
-		ret = !values.nr;
-	else
-		ret = (values.nr == 1) ? 0 : values.nr > 1 ? 2 : 1;
+	ret = !values.nr;
 
 	for (i = 0; i < values.nr; i++) {
 		struct strbuf *buf = values.items + i;
-		fwrite(buf->buf, 1, buf->len, stdout);
+		if (do_all || i == values.nr - 1)
+			fwrite(buf->buf, 1, buf->len, stdout);
 		strbuf_release(buf);
 	}
 	free(values.items);
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 74a297e..8cb45f1 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -252,8 +252,10 @@ test_expect_success 'non-match value' '
 	test wow = $(git config --get nextsection.nonewline !for)
 '
 
-test_expect_success 'ambiguous get' '
-	test_must_fail git config --get nextsection.nonewline
+test_expect_success 'multi-valued get returns final one' '
+	echo "wow2 for me" >expect &&
+	git config --get nextsection.nonewline >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'multi-valued get-all returns all' '
diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 3b9b484..0d4e366 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -46,8 +46,7 @@ BEGIN
 # Save and restore STDERR; we will probably extract this into a
 # "dies_ok" method and possibly move the STDERR handling to Git.pm.
 open our $tmpstderr, ">&STDERR" or die "cannot save STDERR"; close STDERR;
-eval { $r->config("test.dupstring") };
-ok($@, "config: duplicate entry in scalar context fails");
+is($r->config("test.dupstring"), "value2", "config: multivar");
 eval { $r->config_bool("test.boolother") };
 ok($@, "config_bool: non-boolean values fail");
 open STDERR, ">&", $tmpstderr or die "cannot restore STDERR";
-- 
1.8.0.3.g3456896

[PATCH 8/8] git-config: use git_config_with_options

[Jeff King]

The git-config command has always implemented its own file
lookup and parsing order. This was necessary because its
duplicate-entry handling did not match the way git's
internal callbacks worked. Now that this is no longer the
case, we are free to reuse the existing parsing code.

This saves us a few lines of code, but most importantly, it
means that the logic for which files are examined is
contained only in one place and cannot diverge.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/config.c | 44 ++------------------------------------------
 1 file changed, 2 insertions(+), 42 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 77efa69..f881053 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -165,22 +165,9 @@ 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;
 	struct strbuf_list values = {0};
 	int i;
 
-	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");
-	}
-
 	if (use_key_regexp) {
 		char *tl;
 
@@ -229,32 +216,8 @@ static int get_value(const char *key_, const char *regex_)
 		}
 	}
 
-	fn = collect_config;
-	data = &values;
-	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 && !values.nr)
-		git_config_from_file(fn, local, data);
-	if (!do_all && !values.nr && global)
-		git_config_from_file(fn, global, data);
-	if (!do_all && !values.nr && xdg)
-		git_config_from_file(fn, xdg, data);
-	if (!do_all && !values.nr && system_wide)
-		git_config_from_file(fn, system_wide, data);
+	git_config_with_options(collect_config, &values,
+				given_config_file, respect_includes);
 
 	ret = !values.nr;
 
@@ -267,9 +230,6 @@ free_strings:
 	free(values.items);
 
 free_strings:
-	free(repo_config);
-	free(global);
-	free(xdg);
 	free(key);
 	if (key_regexp) {
 		regfree(key_regexp);
-- 
1.8.0.3.g3456896

Re: [PATCH 8/8] git-config: use git_config_with_options

[Johannes Sixt]

All looked sane. Thanks for a pleasant read!

-- Hannes

Re: [PATCH 8/8] git-config: use git_config_with_options

[Ævar Arnfjörð Bjarmason]

Yeah same here. Thanks for tackling this bug. Looking forward to using
the include mechanism for overriding user.email in future versions.

Re: [PATCH 0/8] fix git-config with duplicate variable entries

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Oct 23, 2012 at 04:13:44PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > 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.
>> 
>> I think this patch looks good.
>
> Thanks. It had a few minor flaws (like a memory leak). I fixed those,
> updated the tests, and split it out into a few more readable commits. In
> the process, I managed to uncover and fix a few other memory leaks in
> the area. I think this version is much more readable, and writing the
> rationale for patch 7 convinced me that it's the right thing to do.
> Another round of review would be appreciated.
>
>   [1/8]: t1300: style updates
>   [2/8]: t1300: remove redundant test
>   [3/8]: t1300: test "git config --get-all" more thoroughly
>   [4/8]: git-config: remove memory leak of key regexp
>   [5/8]: git-config: fix regexp memory leaks on error conditions
>   [6/8]: git-config: collect values instead of immediately printing
>   [7/8]: git-config: do not complain about duplicate entries
>   [8/8]: git-config: use git_config_with_options
>
> For those just joining us, the interesting bit is patch 7, which fixes
> some inconsistencies between the "git-config" tool and how the internal
> config callbacks work.


The way for the Porcelain scripts to mimick the internal "last one
wins" has been to grab values out of --get-all and do the "last one
wins" themselves, and I agree that a mode that frees them from
having to worry about it is a good *addition*.  I would even say
that if we were designing "git config" plumbing without existing
users, it would be the only sensible behaviour for "--get".

But "git config --get" users have been promised to get errors on
duplicate values so far, so I have to say this needs to come with a
big red sign about API breakage.

I would feel safer to introduce --get-one or something now, and
worry about migration as a separate step.

Re: [PATCH 0/8] fix git-config with duplicate variable entries

[Jeff King]

On Tue, Nov 20, 2012 at 11:08:43AM -0800, Junio C Hamano wrote:

> > Thanks. It had a few minor flaws (like a memory leak). I fixed those,
> > updated the tests, and split it out into a few more readable commits. In
> > the process, I managed to uncover and fix a few other memory leaks in
> > the area. I think this version is much more readable, and writing the
> > rationale for patch 7 convinced me that it's the right thing to do.
> > Another round of review would be appreciated.
> >
> >   [1/8]: t1300: style updates
> >   [2/8]: t1300: remove redundant test
> >   [3/8]: t1300: test "git config --get-all" more thoroughly
> >   [4/8]: git-config: remove memory leak of key regexp
> >   [5/8]: git-config: fix regexp memory leaks on error conditions
> >   [6/8]: git-config: collect values instead of immediately printing
> >   [7/8]: git-config: do not complain about duplicate entries
> >   [8/8]: git-config: use git_config_with_options
> >
> > For those just joining us, the interesting bit is patch 7, which fixes
> > some inconsistencies between the "git-config" tool and how the internal
> > config callbacks work.
> 
> The way for the Porcelain scripts to mimick the internal "last one
> wins" has been to grab values out of --get-all and do the "last one
> wins" themselves, and I agree that a mode that frees them from
> having to worry about it is a good *addition*.  I would even say
> that if we were designing "git config" plumbing without existing
> users, it would be the only sensible behaviour for "--get".
> 
> But "git config --get" users have been promised to get errors on
> duplicate values so far, so I have to say this needs to come with a
> big red sign about API breakage.

The problem is that scripts currently using "--get" are broken by
duplicate entries in include files, whereas internal git handles them
just fine.  Introducing a new switch means that everybody also has to
start using it.

That is not an excuse for breakage, of course, but only a motivation for
considering it. And here is what I think mitigates that breakage:

  1. If you are a script that cares about multiple values and choosing
     one (whether last-one-wins or otherwise), you are already using
     --get-all and are not affected.

  2. If you are a script that only wants to mimic how get retrieves
     a single value, then this is a bug fix, as it brings "--get" in
     line with git's internal use.

  3. If you are a script that only wants a single value, but cares about
     duplicates, you are already failing to notice them when the
     duplicates are cross-file. That is, we already do "last one wins"
     if an entry is found in ~/.gitconfig and .git/config.

I would argue that anybody fetching standard git config variables (and
not using --list or --get-all) falls into case (2) and is being fixed,
as they will not otherwise match the behavior of git itself.

For other variables that porcelains want to stuff inside the config
files, I suppose they could fall into case (3). But I am not sure why
they would care about duplicates. They have asked git for a single
value, and if they care more deeply about multiple values (but only
within a single file!), what do they hope to accomplish by not calling
--get-all? That is, what is the use case where this makes any sense?

The only case I can think of where the distinction even matters is:

  1. Porcelain foo writes to the .gitfoo file via "git config -f
     .gitfoo".

  2. It accidentally writes using "--add" instead of just setting the
     value.

  3. It fetches via "git config -f .gitfoo --get foo.var". Before my
     patch, duplicate detection would notice the bug in (2) and barf.
     Now, it silently takes the most recently added value (which is
     probably what was meant anyway).

IOW, I do not see a legitimate use case for this, but see it as an extra
check on broken config. But it is catching an unlikely condition, and is
an overly restrictive check, in that it is triggering on totally
reasonable config. So we are not breaking a use case so much as a
loosening a (in my opinion) useless check.

But maybe I am missing some more sane case.

> I would feel safer to introduce --get-one or something now, and
> worry about migration as a separate step.

Part of my impetus for fixing --get is that it let us cleanup and
harmonize the git_config() and git-config implementations. If we are not
going to do that, adding an extra option is not that appealing, as we
are stuck with the old duplicated code, anyway. As I mentioned above,
this only really affects include files (because from git-config's
perspective, the entries it sees are all "the same file", as they come
from the same call into git_config_from_file). If we are not going to
fix --get for all callers, it would probably make more sense to just
omit the duplicate suppression for entries across include-file
boundaries (which are something that callers would have to opt into when
using a specific file, anyway). IOW, to treat it as a bug in the include
system and fix it there.

-Peff

Re: [PATCH 0/8] fix git-config with duplicate variable entries

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> The way for the Porcelain scripts to mimick the internal "last one
>> wins" has been to grab values out of --get-all and do the "last one
>> wins" themselves, and I agree that a mode that frees them from
>> having to worry about it is a good *addition*.  I would even say
>> that if we were designing "git config" plumbing without existing
>> users, it would be the only sensible behaviour for "--get".
>> 
>> But "git config --get" users have been promised to get errors on
>> duplicate values so far, so I have to say this needs to come with a
>> big red sign about API breakage.
>
> The problem is that scripts currently using "--get" are broken by
> duplicate entries in include files, whereas internal git handles them
> just fine.  Introducing a new switch means that everybody also has to
> start using it.

Not exactly.  There are three classes of people:

 - wrote scripts using --get; found out that --get barfs if you feed
   two or more of the same, and have long learned to accept it as a
   limitation and adjusted their configuration files to avoid it.
   They have been doing just fine and wouldn't benefit from this
   series at all.

 - wrote scripts using --get, relying on it barfing if fed zero
   (i.e. missing) or two or more (i.e. ambiguous), perhaps a way to
   keep their configuration files (arguably unnecessarily) clean.
   They are directly harmed by this series.

 - wrote scripts using --get-all and did the last-one-wins
   themselves.  They wouldn't benefit directly from this series,
   unless they are willing to spend a bit of time to remove their
   own last-one-wins logic and replace --get-all with --get (but the
   same effort is needed to migrate to --get-one).

 - wanted to write scripts using --get, but after finding out that
   it barfs if you feed two, gave up emulating the internal, without
   realizing that they could do so with --get-all.  They can now
   write their scripts without using --get-all.

The only people who benefit are from the last class; it does not
matter if they have to write --get-one or --get.

> That is not an excuse for breakage, of course, but only a motivation for
> considering it. And here is what I think mitigates that breakage:
>
>   1. If you are a script that cares about multiple values and choosing
>      one (whether last-one-wins or otherwise), you are already using
>      --get-all and are not affected.

Correct.

>   2. If you are a script that only wants to mimic how get retrieves
>      a single value, then this is a bug fix, as it brings "--get" in
>      line with git's internal use.

But by definition, no such script exists (if it used "--get", it
didn't mimic the internal in the first place).

>   3. If you are a script that only wants a single value, but cares about
>      duplicates, you are already failing to notice them when the
>      duplicates are cross-file. That is, we already do "last one wins"
>      if an entry is found in ~/.gitconfig and .git/config.

Yeah, so the only ones that can be harmed is a config lint that uses
the --get option with --file to make sure variables they know must
be single value are indeed so, and they are not doing a thorough
job, unless they are checking across files themselves, at which
point they are better off using --get-all piped to "sort | uniq -c"
or something.  So breakages do not matter much for correctly written
scripts.

> I would argue that anybody fetching standard git config variables (and
> not using --list or --get-all) falls into case (2) and is being fixed,
> as they will not otherwise match the behavior of git itself.

As people shove random stuff that git itself does not care about in
their config files, they may not care, though.

> IOW, I do not see a legitimate use case for this, but see it as an extra
> check on broken config.

Yes, I agree with the latter half of that sentence.

Re: [PATCH 0/8] fix git-config with duplicate variable entries

[Jeff King]

On Wed, Nov 21, 2012 at 11:46:33AM -0800, Junio C Hamano wrote:

> > The problem is that scripts currently using "--get" are broken by
> > duplicate entries in include files, whereas internal git handles them
> > just fine.  Introducing a new switch means that everybody also has to
> > start using it.
> 
> Not exactly.  There are three classes of people:
> 
>  - wrote scripts using --get; found out that --get barfs if you feed
>    two or more of the same, and have long learned to accept it as a
>    limitation and adjusted their configuration files to avoid it.
>    They have been doing just fine and wouldn't benefit from this
>    series at all.
> 
>  - wrote scripts using --get, relying on it barfing if fed zero
>    (i.e. missing) or two or more (i.e. ambiguous), perhaps a way to
>    keep their configuration files (arguably unnecessarily) clean.
>    They are directly harmed by this series.
> 
>  - wrote scripts using --get-all and did the last-one-wins
>    themselves.  They wouldn't benefit directly from this series,
>    unless they are willing to spend a bit of time to remove their
>    own last-one-wins logic and replace --get-all with --get (but the
>    same effort is needed to migrate to --get-one).
> 
>  - wanted to write scripts using --get, but after finding out that
>    it barfs if you feed two, gave up emulating the internal, without
>    realizing that they could do so with --get-all.  They can now
>    write their scripts without using --get-all.

I have a feeling that your cases 2 and 4 do not really exist. Because we
did "last one wins" in the case that it mattered (between different
files), it was always "good enough" to just assume that using "--get"
behaved like git did internally, and nobody really noticed or cared that
we did duplicate detection at all. But that is just from my own
perspective; it is not like I did a complete survey of git-config users.

More compelling to me is that the only ones negatively affected are your
case 2, and that is qualified by the "arguably unnecessary" you wrote.

Everyone else is not negatively impacted, and can later move to using
"--get" if they want to give up any home-grown --get-all code.

> >   2. If you are a script that only wants to mimic how get retrieves
> >      a single value, then this is a bug fix, as it brings "--get" in
> >      line with git's internal use.
> 
> But by definition, no such script exists (if it used "--get", it
> didn't mimic the internal in the first place).

They do not exist if we assume that anyone using "--get" carefully
thought about the distinction. But I have the impression that is not the
case, and that they _meant_ to behave just like git, and did not realize
they were not doing so. Even our own scripts are guilty of this.

> Yeah, so the only ones that can be harmed is a config lint that uses
> the --get option with --file to make sure variables they know must
> be single value are indeed so, and they are not doing a thorough
> job, unless they are checking across files themselves, at which
> point they are better off using --get-all piped to "sort | uniq -c"
> or something.  So breakages do not matter much for correctly written
> scripts.

Right.

> > IOW, I do not see a legitimate use case for this, but see it as an extra
> > check on broken config.
> 
> Yes, I agree with the latter half of that sentence.

So what do we want to do?

-Peff

Re: [PATCH 0/8] fix git-config with duplicate variable entries

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So what do we want to do?

Nothing.  We'd just let it graduate along with other topics, and
deal with a case where somebody screams, which is unlikely to happen
;-).

Re: [PATCH 0/8] fix git-config with duplicate variable entries

[Joachim Schmitz]

Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:

...

> Not exactly.  There are three classes of people:
>
> - wrote scripts using --get; found out that --get barfs if you feed
>   two or more of the same, and have long learned to accept it as a
>   limitation and adjusted their configuration files to avoid it.
>   They have been doing just fine and wouldn't benefit from this
>   series at all.
>
> - wrote scripts using --get, relying on it barfing if fed zero
>   (i.e. missing) or two or more (i.e. ambiguous), perhaps a way to
>   keep their configuration files (arguably unnecessarily) clean.
>   They are directly harmed by this series.
>
> - wrote scripts using --get-all and did the last-one-wins
>   themselves.  They wouldn't benefit directly from this series,
>   unless they are willing to spend a bit of time to remove their
>   own last-one-wins logic and replace --get-all with --get (but the
>   same effort is needed to migrate to --get-one).
>
> - wanted to write scripts using --get, but after finding out that
>   it barfs if you feed two, gave up emulating the internal, without
>   realizing that they could do so with --get-all.  They can now
>   write their scripts without using --get-all.

There are three classes ofpeople: those that can count and those that can't

Sorry could not resist ;-) 

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

[John Szakmeister]

On Tue, Oct 23, 2012 at 10:13 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
[snip]
> And git config --get foo.bar will give you:
>
>     $ git config -f /tmp/test --get foo.bar
>     one
>     error: More than one value for the key foo.bar: two
>     error: More than one value for the key foo.bar: three
>
> I think that it would be better if the config mechanism just silently
> overwrote keys that clobbered earlier keys like your patch does.
>
> But in addition can we simplify things for the consumers of
> "git-{config,var} -l" by only printing:
>
>     foo.bar=three
>
> Or are there too many variables like "include.path" that can
> legitimately appear more than once.

I frequently use pushurl in my remotes to push my master branch both
to the original repo and my forked version.  I find it very helpful in
my workflow, and would hate to lose that.  That said, I do like the
idea of having a config file and the ability to override some of the
variables.

-John

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

[Jeff King]

On Tue, Oct 23, 2012 at 08:46:47PM -0400, John Szakmeister wrote:

> On Tue, Oct 23, 2012 at 10:13 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> [snip]
> > And git config --get foo.bar will give you:
> >
> >     $ git config -f /tmp/test --get foo.bar
> >     one
> >     error: More than one value for the key foo.bar: two
> >     error: More than one value for the key foo.bar: three
> >
> > I think that it would be better if the config mechanism just silently
> > overwrote keys that clobbered earlier keys like your patch does.
> >
> > But in addition can we simplify things for the consumers of
> > "git-{config,var} -l" by only printing:
> >
> >     foo.bar=three
> >
> > Or are there too many variables like "include.path" that can
> > legitimately appear more than once.
> 
> I frequently use pushurl in my remotes to push my master branch both
> to the original repo and my forked version.  I find it very helpful in
> my workflow, and would hate to lose that.  That said, I do like the
> idea of having a config file and the ability to override some of the
> variables.

No, that won't go anywhere. We really do have two classes of variables:
things that are expected to be single values, and things that are
expected to be lists.

From the perspective of the config code, we don't know or care which is
which, and just feed all entries sequentially to a C callback. In
practice, the callbacks do one of two things:

  1. Append the values into a list.

  2. Overwrite, and end up with the final value seen.

The trouble is that git-config has to print the values in a reasonable
way, so it asks the caller to give a hint about which it wants (--get
versus --get-all). But in the single-value case did not behave like the
C callbacks, which is what my series fixes.

Using "git config -l" is more like asking the config machinery to just
feed us everything, which is what the C callbacks see. Which is more
flexible, but way less convenient for the caller. But it doesn't need to
be fixed, since the caller has all the information to implement whatever
semantics they like.

-Peff