{"thread":{"id":"31903","subject":"The config include mechanism doesn't allow for overwriting","startedAt":"2012-10-22T15:55:00Z","lastAt":"2012-11-23T10:37:51Z","messageCount":25,"participants":["Ævar Arnfjörð Bjarmason","Jeff King","John Szakmeister","Johannes Sixt","Junio C Hamano","Joachim Schmitz"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"201701","messageId":"CACBZZX4cu9XuS5AtduWrNeXNUeZ4rqDUzRdmyz2b3cXtmo1nqQ@mail.gmail.com","threadId":"31903","inReplyTo":null,"subject":"The config include mechanism doesn't allow for overwriting","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2012-10-22T15:55:00Z","receivedAt":"2012-10-22T15:55:00Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I was hoping to write something like this:\n\n [user]\n name = Luser\n email = some-default@example.com\n [include]\n path = ~/.gitconfig.d/user-email\n\nWhere that file would contain:\n\n [user]\n email = local-email@example.com\n\nBut when you do that git prints:\n\n $ git config --get user.email\n some-default@example.com\n error: More than one value for the key user.email: local-email@example.com\n\nI couldn't find information in either the commt that introduced the\nfeature or the documentation explaining whether this was the intent or\nnot.\n\nI think config inclusion is much less useful when you can't clobber\npreviously assigned values.\n"},{"id":"201710","messageId":"20121022211505.GA3301@sigill.intra.peff.net","threadId":"31903","inReplyTo":"CACBZZX4cu9XuS5AtduWrNeXNUeZ4rqDUzRdmyz2b3cXtmo1nqQ@mail.gmail.com","subject":"Re: The config include mechanism doesn't allow for overwriting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-22T21:15:06Z","receivedAt":"2012-10-22T21:15:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 22, 2012 at 05:55:00PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I was hoping to write something like this:\n> \n> [user]\n> name = Luser\n> email = some-default@example.com\n> [include]\n> path = ~/.gitconfig.d/user-email\n> \n> Where that file would contain:\n> \n> [user]\n> email = local-email@example.com\n\nThe intent is that it would work as you expect, and produce\nlocal-email@example.com.\n\n> But when you do that git prints:\n> \n> $ git config --get user.email\n> some-default@example.com\n> error: More than one value for the key user.email: local-email@example.com\n\nUgh. The config code just feeds all the values sequentially to the\ncallback. The normal callbacks within git will overwrite old values,\nwhether from earlier in the file, from a file with lower priority (e.g.,\n/etc/gitconfig versus ~/.gitconfig), or from an earlier included. Which\nyou can check with:\n\n $ git var GIT_AUTHOR_IDENT\n Luser 1350936694 -0400\n\nBut git-config takes it upon itself to detect duplicates in its\ncallback. Which is just silly, since it is not something that regular\ngit would do. git-config should behave as much like the internal git\nparser as possible.\n\n> I think config inclusion is much less useful when you can't clobber\n> previously assigned values.\n\nAgreed. But I think the bug is in git-config, not in the include\nmechanism. I think I'd like to do something like the patch below, which\njust reuses the regular config code for git-config, collects the values,\nand then reports them. It does mean we use a little more memory (for the\nsake of simplicity, we store values instead of streaming them out), but\nthe code is much shorter, less confusing, and automatically matches what\nregular git_config() does.\n\nIt fails a few tests in t1300, but it looks like those tests are testing\nfor the behavior we have identified as wrong, and should be fixed.\n\n---\n builtin/config.c | 111 ++++++++++++-----------------------\n 1 file changed, 38 insertions(+), 73 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex d6a066b..72cb0a8 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -16,7 +16,6 @@ static int do_not_match;\n static int use_key_regexp;\n static int do_all;\n static int do_not_match;\n-static int seen;\n static char delim = '=';\n static char key_delim = ' ';\n static char term = '\\n';\n@@ -110,12 +109,19 @@ static int show_config(const char *key_, const char *value_, void *cb)\n \treturn 0;\n }\n \n-static int show_config(const char *key_, const char *value_, void *cb)\n+struct strbuf_list {\n+\tstruct strbuf *items;\n+\tint nr;\n+\tint alloc;\n+};\n+\n+static int collect_config(const char *key_, const char *value_, void *cb)\n {\n+\tstruct strbuf_list *values = cb;\n+\tstruct strbuf *buf;\n \tchar value[256];\n \tconst char *vptr = value;\n \tint must_free_vptr = 0;\n-\tint dup_error = 0;\n \tint must_print_delim = 0;\n \n \tif (!use_key_regexp && strcmp(key_, key))\n@@ -126,12 +132,15 @@ static int show_config(const char *key_, const char *value_, void *cb)\n \t (do_not_match ^ !!regexec(regexp, (value_?value_:\"\"), 0, NULL, 0)))\n \t\treturn 0;\n \n+\tALLOC_GROW(values->items, values->nr + 1, values->alloc);\n+\tbuf = &values->items[values->nr++];\n+\tstrbuf_init(buf, 0);\n+\n \tif (show_keys) {\n-\t\tprintf(\"%s\", key_);\n+\t\tstrbuf_addstr(buf, key_);\n \t\tmust_print_delim = 1;\n \t}\n-\tif (seen && !do_all)\n-\t\tdup_error = 1;\n+\n \tif (types == TYPE_INT)\n \t\tsprintf(value, \"%d\", git_config_int(key_, value_?value_:\"\"));\n \telse if (types == TYPE_BOOL)\n@@ -153,16 +162,12 @@ static int show_config(const char *key_, const char *value_, void *cb)\n \t\tvptr = \"\";\n \t\tmust_print_delim = 0;\n \t}\n-\tseen++;\n-\tif (dup_error) {\n-\t\terror(\"More than one value for the key %s: %s\",\n-\t\t\t\tkey_, vptr);\n-\t}\n-\telse {\n-\t\tif (must_print_delim)\n-\t\t\tprintf(\"%c\", key_delim);\n-\t\tprintf(\"%s%c\", vptr, term);\n-\t}\n+\n+\tif (must_print_delim)\n+\t\tstrbuf_addch(buf, key_delim);\n+\tstrbuf_addstr(buf, vptr);\n+\tstrbuf_addch(buf, term);\n+\n \tif (must_free_vptr)\n \t\t/* If vptr must be freed, it's a pointer to a\n \t\t * dynamically allocated buffer, it's safe to cast to\n@@ -175,20 +180,8 @@ static int get_value(const char *key_, const char *regex_)\n \n static int get_value(const char *key_, const char *regex_)\n {\n-\tint ret = CONFIG_GENERIC_ERROR;\n-\tchar *global = NULL, *xdg = NULL, *repo_config = NULL;\n-\tconst char *system_wide = NULL, *local;\n-\tstruct config_include_data inc = CONFIG_INCLUDE_INIT;\n-\tconfig_fn_t fn;\n-\tvoid *data;\n-\n-\tlocal = given_config_file;\n-\tif (!local) {\n-\t\tlocal = repo_config = git_pathdup(\"config\");\n-\t\tif (git_config_system())\n-\t\t\tsystem_wide = git_etc_gitconfig();\n-\t\thome_config_paths(&global, &xdg, \"config\");\n-\t}\n+\tstruct strbuf_list values = {0};\n+\tint i;\n \n \tif (use_key_regexp) {\n \t\tchar *tl;\n@@ -211,14 +204,11 @@ static int get_value(const char *key_, const char *regex_)\n \t\tif (regcomp(key_regexp, key, REG_EXTENDED)) {\n \t\t\tfprintf(stderr, \"Invalid key pattern: %s\\n\", key_);\n \t\t\tfree(key);\n-\t\t\tret = CONFIG_INVALID_PATTERN;\n-\t\t\tgoto free_strings;\n+\t\t\treturn CONFIG_INVALID_PATTERN;\n \t\t}\n \t} else {\n-\t\tif (git_config_parse_key(key_, &key, NULL)) {\n-\t\t\tret = CONFIG_INVALID_KEY;\n-\t\t\tgoto free_strings;\n-\t\t}\n+\t\tif (git_config_parse_key(key_, &key, NULL))\n+\t\t\treturn CONFIG_INVALID_KEY;\n \t}\n \n \tif (regex_) {\n@@ -230,37 +220,12 @@ static int get_value(const char *key_, const char *regex_)\n \t\tregexp = (regex_t*)xmalloc(sizeof(regex_t));\n \t\tif (regcomp(regexp, regex_, REG_EXTENDED)) {\n \t\t\tfprintf(stderr, \"Invalid pattern: %s\\n\", regex_);\n-\t\t\tret = CONFIG_INVALID_PATTERN;\n-\t\t\tgoto free_strings;\n+\t\t\treturn CONFIG_INVALID_PATTERN;\n \t\t}\n \t}\n \n-\tfn = show_config;\n-\tdata = NULL;\n-\tif (respect_includes) {\n-\t\tinc.fn = fn;\n-\t\tinc.data = data;\n-\t\tfn = git_config_include;\n-\t\tdata = &inc;\n-\t}\n-\n-\tif (do_all && system_wide)\n-\t\tgit_config_from_file(fn, system_wide, data);\n-\tif (do_all && xdg)\n-\t\tgit_config_from_file(fn, xdg, data);\n-\tif (do_all && global)\n-\t\tgit_config_from_file(fn, global, data);\n-\tif (do_all)\n-\t\tgit_config_from_file(fn, local, data);\n-\tgit_config_from_parameters(fn, data);\n-\tif (!do_all && !seen)\n-\t\tgit_config_from_file(fn, local, data);\n-\tif (!do_all && !seen && global)\n-\t\tgit_config_from_file(fn, global, data);\n-\tif (!do_all && !seen && xdg)\n-\t\tgit_config_from_file(fn, xdg, data);\n-\tif (!do_all && !seen && system_wide)\n-\t\tgit_config_from_file(fn, system_wide, data);\n+\tgit_config_with_options(collect_config, &values,\n+\t\t\t\tgiven_config_file, respect_includes);\n \n \tfree(key);\n \tif (regexp) {\n@@ -268,16 +233,16 @@ static int get_value(const char *key_, const char *regex_)\n \t\tfree(regexp);\n \t}\n \n-\tif (do_all)\n-\t\tret = !seen;\n-\telse\n-\t\tret = (seen == 1) ? 0 : seen > 1 ? 2 : 1;\n+\tif (!values.nr)\n+\t\treturn 1;\n \n-free_strings:\n-\tfree(repo_config);\n-\tfree(global);\n-\tfree(xdg);\n-\treturn ret;\n+\tfor (i = 0; i < values.nr; i++) {\n+\t\tstruct strbuf *buf = values.items + i;\n+\t\tif (do_all || i == values.nr - 1)\n+\t\t\tfwrite(buf->buf, 1, buf->len, stdout);\n+\t\tstrbuf_release(buf);\n+\t}\n+\treturn 0;\n }\n \n static char *normalize_value(const char *key, const char *value)\n"},{"id":"201734","messageId":"CACBZZX5mOb7_i9r8AqNK5V3r-gVnzN+rkeY9xrhecGv1rS-anA@mail.gmail.com","threadId":"31903","inReplyTo":"20121022211505.GA3301@sigill.intra.peff.net","subject":"Re: The config include mechanism doesn't allow for overwriting","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2012-10-23T14:13:44Z","receivedAt":"2012-10-23T14:13:44Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Oct 22, 2012 at 11:15 PM, Jeff King wrote:\n> On Mon, Oct 22, 2012 at 05:55:00PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> I was hoping to write something like this:\n>>\n>> [user]\n>> name = Luser\n>> email = some-default@example.com\n>> [include]\n>> path = ~/.gitconfig.d/user-email\n>>\n>> Where that file would contain:\n>>\n>> [user]\n>> email = local-email@example.com\n>\n> The intent is that it would work as you expect, and produce\n> local-email@example.com.\n>\n>> But when you do that git prints:\n>>\n>> $ git config --get user.email\n>> some-default@example.com\n>> error: More than one value for the key user.email: local-email@example.com\n>\n> Ugh. The config code just feeds all the values sequentially to the\n> callback. The normal callbacks within git will overwrite old values,\n> whether from earlier in the file, from a file with lower priority (e.g.,\n> /etc/gitconfig versus ~/.gitconfig), or from an earlier included. Which\n> you can check with:\n>\n> $ git var GIT_AUTHOR_IDENT\n> Luser 1350936694 -0400\n>\n> But git-config takes it upon itself to detect duplicates in its\n> callback. Which is just silly, since it is not something that regular\n> git would do. git-config should behave as much like the internal git\n> parser as possible.\n>\n>> I think config inclusion is much less useful when you can't clobber\n>> previously assigned values.\n>\n> Agreed. But I think the bug is in git-config, not in the include\n> mechanism. I think I'd like to do something like the patch below, which\n> just reuses the regular config code for git-config, collects the values,\n> and then reports them. It does mean we use a little more memory (for the\n> sake of simplicity, we store values instead of streaming them out), but\n> the code is much shorter, less confusing, and automatically matches what\n> regular git_config() does.\n>\n> It fails a few tests in t1300, but it looks like those tests are testing\n> for the behavior we have identified as wrong, and should be fixed.\n\nI think this patch looks good.\n\nOne other thing I think is worth clarifying (and I think should be\nbroken) is if you write a configuration like:\n\n [foo]\n bar = one\n [foo]\n bar = two\n [foo]\n bar = three\n\n\"git-{config,var} -l\" will both give you:\n\n foo.bar=one\n foo.bar=two\n foo.bar=three\n\nAnd git config --get foo.bar will give you:\n\n $ git config -f /tmp/test --get foo.bar\n one\n error: More than one value for the key foo.bar: two\n error: More than one value for the key foo.bar: three\n\nI think that it would be better if the config mechanism just silently\noverwrote keys that clobbered earlier keys like your patch does.\n\nBut in addition can we simplify things for the consumers of\n\"git-{config,var} -l\" by only printing:\n\n foo.bar=three\n\nOr are there too many variables like \"include.path\" that can\nlegitimately appear more than once.\n"},{"id":"201766","messageId":"20121023223502.GA23194@sigill.intra.peff.net","threadId":"31903","inReplyTo":"CACBZZX5mOb7_i9r8AqNK5V3r-gVnzN+rkeY9xrhecGv1rS-anA@mail.gmail.com","subject":"[PATCH 0/8] fix git-config with duplicate variable entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-23T22:35:02Z","receivedAt":"2012-10-23T22:35:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 23, 2012 at 04:13:44PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > It fails a few tests in t1300, but it looks like those tests are testing\n> > for the behavior we have identified as wrong, and should be fixed.\n> \n> I think this patch looks good.\n\nThanks. It had a few minor flaws (like a memory leak). I fixed those,\nupdated the tests, and split it out into a few more readable commits. In\nthe process, I managed to uncover and fix a few other memory leaks in\nthe area. I think this version is much more readable, and writing the\nrationale for patch 7 convinced me that it's the right thing to do.\nAnother round of review would be appreciated.\n\n [1/8]: t1300: style updates\n [2/8]: t1300: remove redundant test\n [3/8]: t1300: test \"git config --get-all\" more thoroughly\n [4/8]: git-config: remove memory leak of key regexp\n [5/8]: git-config: fix regexp memory leaks on error conditions\n [6/8]: git-config: collect values instead of immediately printing\n [7/8]: git-config: do not complain about duplicate entries\n [8/8]: git-config: use git_config_with_options\n\nFor those just joining us, the interesting bit is patch 7, which fixes\nsome inconsistencies between the \"git-config\" tool and how the internal\nconfig callbacks work.\n\n> One other thing I think is worth clarifying (and I think should be\n> broken) is if you write a configuration like:\n> \n> [foo]\n> bar = one\n> [foo]\n> bar = two\n> [foo]\n> bar = three\n> \n> \"git-{config,var} -l\" will both give you:\n> \n> foo.bar=one\n> foo.bar=two\n> foo.bar=three\n\nYes, that looks right.\n\n> And git config --get foo.bar will give you:\n> \n> $ git config -f /tmp/test --get foo.bar\n> one\n> error: More than one value for the key foo.bar: two\n> error: More than one value for the key foo.bar: three\n> \n> I think that it would be better if the config mechanism just silently\n> overwrote keys that clobbered earlier keys like your patch does.\n\nRight.\n\n> But in addition can we simplify things for the consumers of\n> \"git-{config,var} -l\" by only printing:\n> \n> foo.bar=three\n> \n> Or are there too many variables like \"include.path\" that can\n> legitimately appear more than once.\n\nNo. Some variables can legitimately appear multiple times. E.g.,\nremote.*.fetch, remote.*.push, and remote.*.url. Probably more that I am\nforgetting. There are not many, but they do exist.\n\nIt's OK to tweak the regular \"get\" for them, since they are already\nbroken for that case[1]. You need to use \"--get-all\" if you expect the\nvariable to have multiple values. But when we are listing, we do not\nhave the hint as to what is expected, and we need to show all entries.\n\n-Peff\n\n[1] So the one useful thing that the current duplicate check is doing is\n flagging errors where you wanted to use --get-all, but forgot to.\n However, it's not really a sufficient safeguard anyway, since it\n would not catch cases where the list was split across multiple\n files (which does work with the internal callbacks that handle\n lists, since they never even see that multiple files are involved).\n It's much more important for git-config to be consistent with the\n internal parsing behavior.\n"},{"id":"201767","messageId":"20121023223554.GA17392@sigill.intra.peff.net","threadId":"31903","inReplyTo":"20121023223502.GA23194@sigill.intra.peff.net","subject":"[PATCH 1/8] t1300: style updates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-23T22:35:54Z","receivedAt":"2012-10-23T22:35:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The t1300 test script is quite old, and does not use our\nmodern techniques or styles. This patch updates it in the\nfollowing ways:\n\n 1. Use test_cmp instead of cmp (to make failures easier to\n debug).\n\n 2. Use test_cmp instead of 'test $(command) = expected'.\n This makes failures much easier to debug, and also\n makes sure that $(command) exits appropriately.\n\n 3. Write tests with the usual style of:\n\n test_expect_success 'test name' '\n test commands &&\n\t ...\n '\n\n rather than one-liners, or using backslash-continuation.\n This is purely a style fixup.\n\nThere are still a few command happening outside of\ntest_expect invocations, but they are all innoccuous system\ncommands like \"cat\" and \"cp\". In an ideal world, each test\nwould be self sufficient and all commands would happen\ninside test_expect, but it is not immediately obvious how\nthe grouping should work (some of the commands impact the\nsubsequent tests, and some of them are setting up and\nmodifying state that many tests depend on). This patch just\npicks the low-hanging style fruit, and we can do more fixes\non top later.\n\nSigned-off-by: Jeff King \n---\n t/t1300-repo-config.sh | 185 ++++++++++++++++++++++++++++++-------------------\n 1 file changed, 113 insertions(+), 72 deletions(-)\n\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex e127f35..e12dd4a 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -55,11 +55,13 @@ test_expect_success 'replace with non-match' \\\n \ttest_cmp expect .git/config\n '\n \n-test_expect_success 'replace with non-match' \\\n-\t'git config core.penguin kingpin !blue'\n+test_expect_success 'replace with non-match' '\n+\tgit config core.penguin kingpin !blue\n+'\n \n-test_expect_success 'replace with non-match (actually matching)' \\\n-\t'git config core.penguin \"very blue\" !kingpin'\n+test_expect_success 'replace with non-match (actually matching)' '\n+\tgit config core.penguin \"very blue\" !kingpin\n+'\n \n cat > expect << EOF\n [core]\n@@ -108,8 +110,9 @@ EOF\n lines\n EOF\n \n-test_expect_success 'unset with cont. lines' \\\n-\t'git config --unset beta.baz'\n+test_expect_success 'unset with cont. lines' '\n+\tgit config --unset beta.baz\n+'\n \n cat > expect <<\\EOF\n [alpha]\n@@ -133,8 +136,9 @@ cp .git/config .git/config2\n \n cp .git/config .git/config2\n \n-test_expect_success 'multiple unset' \\\n-\t'git config --unset-all beta.haha'\n+test_expect_success 'multiple unset' '\n+\tgit config --unset-all beta.haha\n+'\n \n cat > expect << EOF\n [beta] ; silly comment # another comment\n@@ -145,7 +149,9 @@ EOF\n [nextSection] noNewline = ouch\n EOF\n \n-test_expect_success 'multiple unset is correct' 'test_cmp expect .git/config'\n+test_expect_success 'multiple unset is correct' '\n+\ttest_cmp expect .git/config\n+'\n \n cp .git/config2 .git/config\n \n@@ -156,8 +162,9 @@ rm .git/config2\n \n rm .git/config2\n \n-test_expect_success '--replace-all' \\\n-\t'git config --replace-all beta.haha gamma'\n+test_expect_success '--replace-all' '\n+\tgit config --replace-all beta.haha gamma\n+'\n \n cat > expect << EOF\n [beta] ; silly comment # another comment\n@@ -169,7 +176,9 @@ EOF\n [nextSection] noNewline = ouch\n EOF\n \n-test_expect_success 'all replaced' 'test_cmp expect .git/config'\n+test_expect_success 'all replaced' '\n+\ttest_cmp expect .git/config\n+'\n \n cat > expect << EOF\n [beta] ; silly comment # another comment\n@@ -200,7 +209,11 @@ test_expect_success 'really really mean test' '\n \ttest_cmp expect .git/config\n '\n \n-test_expect_success 'get value' 'test alpha = $(git config beta.haha)'\n+test_expect_success 'get value' '\n+\techo alpha >expect &&\n+\tgit config beta.haha >actual &&\n+\ttest_cmp expect actual\n+'\n \n cat > expect << EOF\n [beta] ; silly comment # another comment\n@@ -231,18 +244,21 @@ test_expect_success 'ambiguous get' '\n \ttest_cmp expect .git/config\n '\n \n-test_expect_success 'non-match' \\\n-\t'git config --get nextsection.nonewline !for'\n+test_expect_success 'non-match' '\n+\tgit config --get nextsection.nonewline !for\n+'\n \n-test_expect_success 'non-match value' \\\n-\t'test wow = $(git config --get nextsection.nonewline !for)'\n+test_expect_success 'non-match value' '\n+\ttest wow = $(git config --get nextsection.nonewline !for)\n+'\n \n test_expect_success 'ambiguous get' '\n \ttest_must_fail git config --get nextsection.nonewline\n '\n \n-test_expect_success 'get multivar' \\\n-\t'git config --get-all nextsection.nonewline'\n+test_expect_success 'get multivar' '\n+\tgit config --get-all nextsection.nonewline\n+'\n \n cat > expect << EOF\n [beta] ; silly comment # another comment\n@@ -290,8 +306,9 @@ test_expect_success 'correct key' 'git config 123456.a123 987'\n \n test_expect_success 'correct key' 'git config 123456.a123 987'\n \n-test_expect_success 'hierarchical section' \\\n-\t'git config Version.1.2.3eX.Alpha beta'\n+test_expect_success 'hierarchical section' '\n+\tgit config Version.1.2.3eX.Alpha beta\n+'\n \n cat > expect << EOF\n [beta] ; silly comment # another comment\n@@ -307,7 +324,9 @@ EOF\n \tAlpha = beta\n EOF\n \n-test_expect_success 'hierarchical section value' 'test_cmp expect .git/config'\n+test_expect_success 'hierarchical section value' '\n+\ttest_cmp expect .git/config\n+'\n \n cat > expect << EOF\n beta.noindent=sillyValue\n@@ -316,9 +335,10 @@ EOF\n version.1.2.3eX.alpha=beta\n EOF\n \n-test_expect_success 'working --list' \\\n-\t'git config --list > output && cmp output expect'\n-\n+test_expect_success 'working --list' '\n+\tgit config --list > output &&\n+\ttest_cmp expect output\n+'\n cat > expect << EOF\n EOF\n \n@@ -332,8 +352,9 @@ EOF\n nextsection.nonewline wow2 for me\n EOF\n \n-test_expect_success '--get-regexp' \\\n-\t'git config --get-regexp in > output && cmp output expect'\n+test_expect_success '--get-regexp' '\n+\tgit config --get-regexp in > output && test_cmp expect output\n+'\n \n cat > expect << EOF\n wow2 for me\n@@ -353,41 +374,47 @@ echo false > expect\n \tvariable =\n EOF\n \n-test_expect_success 'get variable with no value' \\\n-\t'git config --get novalue.variable ^$'\n+test_expect_success 'get variable with no value' '\n+\tgit config --get novalue.variable ^$\n+'\n \n-test_expect_success 'get variable with empty value' \\\n-\t'git config --get emptyvalue.variable ^$'\n+test_expect_success 'get variable with empty value' '\n+\tgit config --get emptyvalue.variable ^$\n+'\n \n echo novalue.variable > expect\n \n-test_expect_success 'get-regexp variable with no value' \\\n-\t'git config --get-regexp novalue > output &&\n-\t cmp output expect'\n+test_expect_success 'get-regexp variable with no value' '\n+\tgit config --get-regexp novalue > output &&\n+\t test_cmp expect output'\n \n echo 'novalue.variable true' > expect\n \n-test_expect_success 'get-regexp --bool variable with no value' \\\n-\t'git config --bool --get-regexp novalue > output &&\n-\t cmp output expect'\n+test_expect_success 'get-regexp --bool variable with no value' '\n+\tgit config --bool --get-regexp novalue > output &&\n+\ttest_cmp expect output\n+'\n \n echo 'emptyvalue.variable ' > expect\n \n-test_expect_success 'get-regexp variable with empty value' \\\n-\t'git config --get-regexp emptyvalue > output &&\n-\t cmp output expect'\n+test_expect_success 'get-regexp variable with empty value' '\n+\tgit config --get-regexp emptyvalue > output &&\n+\ttest_cmp expect output\n+'\n \n echo true > expect\n \n-test_expect_success 'get bool variable with no value' \\\n-\t'git config --bool novalue.variable > output &&\n-\t cmp output expect'\n+test_expect_success 'get bool variable with no value' '\n+\tgit config --bool novalue.variable > output &&\n+\ttest_cmp expect output\n+'\n \n echo false > expect\n \n-test_expect_success 'get bool variable with empty value' \\\n-\t'git config --bool emptyvalue.variable > output &&\n-\t cmp output expect'\n+test_expect_success 'get bool variable with empty value' '\n+\tgit config --bool emptyvalue.variable > output &&\n+\ttest_cmp expect output\n+'\n \n test_expect_success 'no arguments, but no crash' '\n \ttest_must_fail git config >output 2>&1 &&\n@@ -427,8 +454,9 @@ test_expect_success 'new variable inserts into proper section' '\n \ttest_cmp expect .git/config\n '\n \n-test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' \\\n-\t'test_must_fail git config --file non-existing-config -l'\n+test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' '\n+\ttest_must_fail git config --file non-existing-config -l\n+'\n \n cat > other-config << EOF\n [ein]\n@@ -444,8 +472,10 @@ test_expect_success 'alternative GIT_CONFIG' '\n \ttest_cmp expect output\n '\n \n-test_expect_success 'alternative GIT_CONFIG (--file)' \\\n-\t'git config --file other-config -l > output && cmp output expect'\n+test_expect_success 'alternative GIT_CONFIG (--file)' '\n+\tgit config --file other-config -l > output &&\n+\ttest_cmp expect output\n+'\n \n test_expect_success 'refer config from subdirectory' '\n \tmkdir x &&\n@@ -489,8 +519,9 @@ EOF\n weird\n EOF\n \n-test_expect_success \"rename section\" \\\n-\t\"git config --rename-section branch.eins branch.zwei\"\n+test_expect_success 'rename section' '\n+\tgit config --rename-section branch.eins branch.zwei\n+'\n \n cat > expect << EOF\n # Hallo\n@@ -503,17 +534,22 @@ test_expect_success \"rename succeeded\" \"test_cmp expect .git/config\"\n weird\n EOF\n \n-test_expect_success \"rename succeeded\" \"test_cmp expect .git/config\"\n+test_expect_success 'rename succeeded' '\n+\ttest_cmp expect .git/config\n+'\n \n-test_expect_success \"rename non-existing section\" '\n+test_expect_success 'rename non-existing section' '\n \ttest_must_fail git config --rename-section \\\n \t\tbranch.\"world domination\" branch.drei\n '\n \n-test_expect_success \"rename succeeded\" \"test_cmp expect .git/config\"\n+test_expect_success 'rename succeeded' '\n+\ttest_cmp expect .git/config\n+'\n \n-test_expect_success \"rename another section\" \\\n-\t'git config --rename-section branch.\"1 234 blabl/a\" branch.drei'\n+test_expect_success 'rename another section' '\n+\tgit config --rename-section branch.\"1 234 blabl/a\" branch.drei\n+'\n \n cat > expect << EOF\n # Hallo\n@@ -526,14 +562,17 @@ EOF\n weird\n EOF\n \n-test_expect_success \"rename succeeded\" \"test_cmp expect .git/config\"\n+test_expect_success 'rename succeeded' '\n+\ttest_cmp expect .git/config\n+'\n \n cat >> .git/config << EOF\n [branch \"vier\"] z = 1\n EOF\n \n-test_expect_success \"rename a section with a var on the same line\" \\\n-\t'git config --rename-section branch.vier branch.zwei'\n+test_expect_success 'rename a section with a var on the same line' '\n+\tgit config --rename-section branch.vier branch.zwei\n+'\n \n cat > expect << EOF\n # Hallo\n@@ -548,7 +587,9 @@ EOF\n \tz = 1\n EOF\n \n-test_expect_success \"rename succeeded\" \"test_cmp expect .git/config\"\n+test_expect_success 'rename succeeded' '\n+\ttest_cmp expect .git/config\n+'\n \n test_expect_success 'renaming empty section name is rejected' '\n \ttest_must_fail git config --rename-section branch.zwei \"\"\n@@ -562,7 +603,9 @@ EOF\n [branch \"zwei\"] a = 1 [branch \"vier\"]\n EOF\n \n-test_expect_success \"remove section\" \"git config --remove-section branch.zwei\"\n+test_expect_success 'remove section' '\n+\tgit config --remove-section branch.zwei\n+'\n \n cat > expect << EOF\n # Hallo\n@@ -571,8 +614,9 @@ EOF\n weird\n EOF\n \n-test_expect_success \"section was removed properly\" \\\n-\t\"test_cmp expect .git/config\"\n+test_expect_success 'section was removed properly' '\n+\ttest_cmp expect .git/config\n+'\n \n cat > expect << EOF\n [gitcvs]\n@@ -583,7 +627,6 @@ test_expect_success 'section ending' '\n EOF\n \n test_expect_success 'section ending' '\n-\n \trm -f .git/config &&\n \tgit config gitcvs.enabled true &&\n \tgit config gitcvs.ext.dbname %Ggitcvs1.%a.%m.sqlite &&\n@@ -593,7 +636,6 @@ test_expect_success numbers '\n '\n \n test_expect_success numbers '\n-\n \tgit config kilo.gram 1k &&\n \tgit config mega.ton 1m &&\n \tk=$(git config --int --get kilo.gram) &&\n@@ -607,7 +649,6 @@ test_expect_success 'invalid unit' '\n EOF\n \n test_expect_success 'invalid unit' '\n-\n \tgit config aninvalid.unit \"1auto\" &&\n \ts=$(git config aninvalid.unit) &&\n \ttest \"z1auto\" = \"z$s\" &&\n@@ -616,7 +657,7 @@ test_expect_success 'invalid unit' '\n \t\techo config should have failed\n \t\tfalse\n \tfi &&\n-\tcmp actual expect\n+\ttest_cmp actual expect\n '\n \n cat > expect << EOF\n@@ -646,7 +687,7 @@ test_expect_success bool '\n \t git config --bool --get bool.true$i >>result\n \t git config --bool --get bool.false$i >>result\n done &&\n-\tcmp expect result'\n+\ttest_cmp expect result'\n \n test_expect_success 'invalid bool (--get)' '\n \n@@ -680,7 +721,7 @@ test_expect_success 'set --bool' '\n \tgit config --bool bool.false2 \"\" &&\n \tgit config --bool bool.false3 nO &&\n \tgit config --bool bool.false4 FALSE &&\n-\tcmp expect .git/config'\n+\ttest_cmp expect .git/config'\n \n cat > expect <<\\EOF\n [int]\n@@ -695,7 +736,7 @@ test_expect_success 'set --int' '\n \tgit config --int int.val1 01 &&\n \tgit config --int int.val2 -1 &&\n \tgit config --int int.val3 5m &&\n-\tcmp expect .git/config'\n+\ttest_cmp expect .git/config'\n \n cat >expect <<\\EOF\n [bool]\n@@ -844,7 +885,7 @@ test_expect_success 'value continued on next line' '\n \n test_expect_success 'value continued on next line' '\n \tgit config --list > result &&\n-\tcmp result expect\n+\ttest_cmp result expect\n '\n \n cat > .git/config <<\\EOF\n-- \n1.8.0.3.g3456896\n"},{"id":"201768","messageId":"20121023223622.GB17392@sigill.intra.peff.net","threadId":"31903","inReplyTo":"20121023223502.GA23194@sigill.intra.peff.net","subject":"[PATCH 2/8] t1300: remove redundant test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-23T22:36:22Z","receivedAt":"2012-10-23T22:36:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This test checks that git-config fails for an ambiguous\n\"get\", but we check the exact same thing 3 tests beforehand.\n\nSigned-off-by: Jeff King \n---\nI update the matching test later in the series, and I didn't want to\nhave to do it twice.\n\n t/t1300-repo-config.sh | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex e12dd4a..c6489dc 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -275,10 +275,6 @@ test_expect_success 'multivar replace' '\n \ttest_cmp expect .git/config\n '\n \n-test_expect_success 'ambiguous value' '\n-\ttest_must_fail git config nextsection.nonewline\n-'\n-\n test_expect_success 'ambiguous unset' '\n \ttest_must_fail git config --unset nextsection.nonewline\n '\n-- \n1.8.0.3.g3456896\n"},{"id":"201769","messageId":"20121023223638.GC17392@sigill.intra.peff.net","threadId":"31903","inReplyTo":"20121023223502.GA23194@sigill.intra.peff.net","subject":"[PATCH 3/8] t1300: test \"git config --get-all\" more thoroughly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-23T22:36:38Z","receivedAt":"2012-10-23T22:36:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We check that we can \"--get-all\" a multi-valued variable,\nbut we do not actually confirm that the output is sensible.\nDoing so reveals that it works fine, but this will help us\nensure we do not have regressions in the next few patches,\nwhich will touch this area.\n\nSigned-off-by: Jeff King \n---\n t/t1300-repo-config.sh | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex c6489dc..74a297e 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -256,8 +256,13 @@ test_expect_success 'ambiguous get' '\n \ttest_must_fail git config --get nextsection.nonewline\n '\n \n-test_expect_success 'get multivar' '\n-\tgit config --get-all nextsection.nonewline\n+test_expect_success 'multi-valued get-all returns all' '\n+\tcat >expect <<-\\EOF &&\n+\twow\n+\twow2 for me\n+\tEOF\n+\tgit config --get-all nextsection.nonewline >actual &&\n+\ttest_cmp expect actual\n '\n \n cat > expect << EOF\n-- \n1.8.0.3.g3456896\n"},{"id":"201770","messageId":"20121023223650.GD17392@sigill.intra.peff.net","threadId":"31903","inReplyTo":"20121023223502.GA23194@sigill.intra.peff.net","subject":"[PATCH 4/8] git-config: remove memory leak of key regexp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-23T22:36:50Z","receivedAt":"2012-10-23T22:36:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is only called once per invocation, so it's not a major\nleak, but it's easy to fix.\n\nSigned-off-by: Jeff King \n---\n builtin/config.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex e1c33e0..e660d48 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -248,6 +248,10 @@ static int get_value(const char *key_, const char *regex_)\n \t\tgit_config_from_file(fn, system_wide, data);\n \n \tfree(key);\n+\tif (key_regexp) {\n+\t\tregfree(key_regexp);\n+\t\tfree(key_regexp);\n+\t}\n \tif (regexp) {\n \t\tregfree(regexp);\n \t\tfree(regexp);\n-- \n1.8.0.3.g3456896\n"},{"id":"201771","messageId":"20121023223855.GE17392@sigill.intra.peff.net","threadId":"31903","inReplyTo":"20121023223502.GA23194@sigill.intra.peff.net","subject":"[PATCH 5/8] git-config: fix regexp memory leaks on error conditions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-23T22:38:55Z","receivedAt":"2012-10-23T22:38:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The get_value function has a goto label for cleaning up on\nerrors, but it only cleans up half of what the function\nmight allocate. Let's also clean up the key and regexp\nvariables there.\n\nNote that we need to take special care when compiling the\nregex fails to clean it up ourselves, since it is in a\nhalf-constructed state (we would want to free it, but not\nregfree it).\n\nSimilarly, we fix git_config_parse_key to return NULL when it\nfails, not a pointer to some already-freed memory.\n\nSigned-off-by: Jeff King \n---\nThe diff is annoying in an interesting way: what I actually did was move\nthe regex cleanup code down, but it shows it as moving the bottom bits\nup. I think it is just one of those ambiguous cases where either way is\nequally valid and minimal.\n\n builtin/config.c | 23 +++++++++++++----------\n config.c | 1 +\n 2 files changed, 14 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex e660d48..60d36e7 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -195,7 +195,8 @@ static int get_value(const char *key_, const char *regex_)\n \t\tkey_regexp = (regex_t*)xmalloc(sizeof(regex_t));\n \t\tif (regcomp(key_regexp, key, REG_EXTENDED)) {\n \t\t\tfprintf(stderr, \"Invalid key pattern: %s\\n\", key_);\n-\t\t\tfree(key);\n+\t\t\tfree(key_regexp);\n+\t\t\tkey_regexp = NULL;\n \t\t\tret = CONFIG_INVALID_PATTERN;\n \t\t\tgoto free_strings;\n \t\t}\n@@ -215,6 +216,8 @@ static int get_value(const char *key_, const char *regex_)\n \t\tregexp = (regex_t*)xmalloc(sizeof(regex_t));\n \t\tif (regcomp(regexp, regex_, REG_EXTENDED)) {\n \t\t\tfprintf(stderr, \"Invalid pattern: %s\\n\", regex_);\n+\t\t\tfree(regexp);\n+\t\t\tregexp = NULL;\n \t\t\tret = CONFIG_INVALID_PATTERN;\n \t\t\tgoto free_strings;\n \t\t}\n@@ -247,6 +250,15 @@ static int get_value(const char *key_, const char *regex_)\n \tif (!do_all && !seen && system_wide)\n \t\tgit_config_from_file(fn, system_wide, data);\n \n+\tif (do_all)\n+\t\tret = !seen;\n+\telse\n+\t\tret = (seen == 1) ? 0 : seen > 1 ? 2 : 1;\n+\n+free_strings:\n+\tfree(repo_config);\n+\tfree(global);\n+\tfree(xdg);\n \tfree(key);\n \tif (key_regexp) {\n \t\tregfree(key_regexp);\n@@ -257,15 +269,6 @@ static int get_value(const char *key_, const char *regex_)\n \t\tfree(regexp);\n \t}\n \n-\tif (do_all)\n-\t\tret = !seen;\n-\telse\n-\t\tret = (seen == 1) ? 0 : seen > 1 ? 2 : 1;\n-\n-free_strings:\n-\tfree(repo_config);\n-\tfree(global);\n-\tfree(xdg);\n \treturn ret;\n }\n \ndiff --git a/config.c b/config.c\nindex 08e47e2..2fbe634 100644\n--- a/config.c\n+++ b/config.c\n@@ -1280,6 +1280,7 @@ out_free_ret_1:\n \n out_free_ret_1:\n \tfree(*store_key);\n+\t*store_key = NULL;\n \treturn -CONFIG_INVALID_KEY;\n }\n \n-- \n1.8.0.3.g3456896\n"},{"id":"201772","messageId":"20121023224004.GF17392@sigill.intra.peff.net","threadId":"31903","inReplyTo":"20121023223502.GA23194@sigill.intra.peff.net","subject":"[PATCH 6/8] git-config: collect values instead of immediately printing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-23T22:40:04Z","receivedAt":"2012-10-23T22:40:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a refactor that will allow us to more easily tweak\nthe behavior for multi-valued variables, and it will\nultimately allow us to remove a lot git-config's custom code\nin favor of the regular git_config code.\n\nIt does mean we're no longer streaming, and we're storing\nmore in memory for the --get-all case, but in practice it is\na tiny amount of data, and the results are instantaneous.\n\nSigned-off-by: Jeff King \n---\nThe increase in line count is nicely offset by the next two patches.\n\n builtin/config.c | 50 +++++++++++++++++++++++++++++++++++---------------\n 1 file changed, 35 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 60d36e7..08e83fc 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -15,7 +15,6 @@ static int do_not_match;\n static int use_key_regexp;\n static int do_all;\n static int do_not_match;\n-static int seen;\n static char delim = '=';\n static char key_delim = ' ';\n static char term = '\\n';\n@@ -95,8 +94,16 @@ static int show_config(const char *key_, const char *value_, void *cb)\n \treturn 0;\n }\n \n-static int show_config(const char *key_, const char *value_, void *cb)\n+struct strbuf_list {\n+\tstruct strbuf *items;\n+\tint nr;\n+\tint alloc;\n+};\n+\n+static int collect_config(const char *key_, const char *value_, void *cb)\n {\n+\tstruct strbuf_list *values = cb;\n+\tstruct strbuf *buf;\n \tchar value[256];\n \tconst char *vptr = value;\n \tint must_free_vptr = 0;\n@@ -111,11 +118,15 @@ static int show_config(const char *key_, const char *value_, void *cb)\n \t (do_not_match ^ !!regexec(regexp, (value_?value_:\"\"), 0, NULL, 0)))\n \t\treturn 0;\n \n+\tALLOC_GROW(values->items, values->nr + 1, values->alloc);\n+\tbuf = &values->items[values->nr++];\n+\tstrbuf_init(buf, 0);\n+\n \tif (show_keys) {\n-\t\tprintf(\"%s\", key_);\n+\t\tstrbuf_addstr(buf, key_);\n \t\tmust_print_delim = 1;\n \t}\n-\tif (seen && !do_all)\n+\tif (values->nr > 1 && !do_all)\n \t\tdup_error = 1;\n \tif (types == TYPE_INT)\n \t\tsprintf(value, \"%d\", git_config_int(key_, value_?value_:\"\"));\n@@ -138,15 +149,15 @@ static int show_config(const char *key_, const char *value_, void *cb)\n \t\tvptr = \"\";\n \t\tmust_print_delim = 0;\n \t}\n-\tseen++;\n \tif (dup_error) {\n \t\terror(\"More than one value for the key %s: %s\",\n \t\t\t\tkey_, vptr);\n \t}\n \telse {\n \t\tif (must_print_delim)\n-\t\t\tprintf(\"%c\", key_delim);\n-\t\tprintf(\"%s%c\", vptr, term);\n+\t\t\tstrbuf_addch(buf, key_delim);\n+\t\tstrbuf_addstr(buf, vptr);\n+\t\tstrbuf_addch(buf, term);\n \t}\n \tif (must_free_vptr)\n \t\t/* If vptr must be freed, it's a pointer to a\n@@ -166,6 +177,8 @@ static int get_value(const char *key_, const char *regex_)\n \tstruct config_include_data inc = CONFIG_INCLUDE_INIT;\n \tconfig_fn_t fn;\n \tvoid *data;\n+\tstruct strbuf_list values = {0};\n+\tint i;\n \n \tlocal = given_config_file;\n \tif (!local) {\n@@ -223,8 +236,8 @@ static int get_value(const char *key_, const char *regex_)\n \t\t}\n \t}\n \n-\tfn = show_config;\n-\tdata = NULL;\n+\tfn = collect_config;\n+\tdata = &values;\n \tif (respect_includes) {\n \t\tinc.fn = fn;\n \t\tinc.data = data;\n@@ -241,19 +254,26 @@ static int get_value(const char *key_, const char *regex_)\n \tif (do_all)\n \t\tgit_config_from_file(fn, local, data);\n \tgit_config_from_parameters(fn, data);\n-\tif (!do_all && !seen)\n+\tif (!do_all && !values.nr)\n \t\tgit_config_from_file(fn, local, data);\n-\tif (!do_all && !seen && global)\n+\tif (!do_all && !values.nr && global)\n \t\tgit_config_from_file(fn, global, data);\n-\tif (!do_all && !seen && xdg)\n+\tif (!do_all && !values.nr && xdg)\n \t\tgit_config_from_file(fn, xdg, data);\n-\tif (!do_all && !seen && system_wide)\n+\tif (!do_all && !values.nr && system_wide)\n \t\tgit_config_from_file(fn, system_wide, data);\n \n \tif (do_all)\n-\t\tret = !seen;\n+\t\tret = !values.nr;\n \telse\n-\t\tret = (seen == 1) ? 0 : seen > 1 ? 2 : 1;\n+\t\tret = (values.nr == 1) ? 0 : values.nr > 1 ? 2 : 1;\n+\n+\tfor (i = 0; i < values.nr; i++) {\n+\t\tstruct strbuf *buf = values.items + i;\n+\t\tfwrite(buf->buf, 1, buf->len, stdout);\n+\t\tstrbuf_release(buf);\n+\t}\n+\tfree(values.items);\n \n free_strings:\n \tfree(repo_config);\n-- \n1.8.0.3.g3456896\n"},{"id":"201773","messageId":"20121023224100.GG17392@sigill.intra.peff.net","threadId":"31903","inReplyTo":"20121023223502.GA23194@sigill.intra.peff.net","subject":"[PATCH 7/8] git-config: do not complain about duplicate entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-23T22:41:00Z","receivedAt":"2012-10-23T22:41:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If git-config is asked for a single value, it will complain\nand exit with an error if it finds multiple instances of\nthat value. This is unlike the usual internal config\nparsing, however, which will generally overwrite previous\nvalues, leaving only the final one. For example:\n\n [set a multivar]\n $ git config user.email one@example.com\n $ git config --add user.email two@example.com\n\n [use the internal parser to fetch it]\n $ git var GIT_AUTHOR_IDENT\n Your Name ...\n\n [use git-config to fetch it]\n $ git config user.email\n one@example.com\n error: More than one value for the key user.email: two@example.com\n\nThis overwriting behavior is critical for the regular\nparser, which starts with the lowest-priority file (e.g.,\n/etc/gitconfig) and proceeds to the highest-priority file\n($GIT_DIR/config). Overwriting yields the highest priority\nvalue at the end.\n\nGit-config solves this problem by implementing its own\nparsing. It goes from highest to lowest priorty, but does\nnot proceed to the next file if it has seen a value.\n\nSo in practice, this distinction never mattered much,\nbecause it only triggered for values in the same file. And\nthere was not much point in doing that; the real value is in\noverwriting values from lower-priority files.\n\nHowever, this changed with the implementation of config\ninclude files. Now we might see an include overriding a\nvalue from the parent file, which is a sensible thing to do,\nbut git-config will flag as a duplication.\n\nThis patch drops the duplicate detection for git-config and\nswitches to a pure-overwrite model (for the single case;\n--get-all can still be used if callers want to do something\nmore fancy).\n\nAs is shown by the modifications to the test suite, this is\na user-visible change in behavior. An alternative would be\nto just change the include case, but this is much cleaner\nfor a few reasons:\n\n 1. If you change the include case, then to what? If you\n just stop parsing includes after getting a value, then\n you will get a _different_ answer than the regular\n config parser (you'll get the first value instead of\n the last value). So you'd want to implement overwrite\n semantics anyway.\n\n 2. Even though it is a change in behavior for git-config,\n it is bringing us in line with what the internal\n parsers already do.\n\n 3. The file-order reimplementation is the only thing\n keeping us from sharing more code with the internal\n config parser, which will help keep differences to a\n minimum.\n\nGoing under the assumption that the primary purpose of\ngit-config is to behave identically to how git's internal\nparsing works, this change can be seen as a bug-fix.\n\nSigned-off-by: Jeff King \n---\n builtin/config.c | 27 +++++++++------------------\n t/t1300-repo-config.sh | 6 ++++--\n t/t9700/test.pl | 3 +--\n 3 files changed, 14 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 08e83fc..77efa69 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -107,7 +107,6 @@ static int collect_config(const char *key_, const char *value_, void *cb)\n \tchar value[256];\n \tconst char *vptr = value;\n \tint must_free_vptr = 0;\n-\tint dup_error = 0;\n \tint must_print_delim = 0;\n \n \tif (!use_key_regexp && strcmp(key_, key))\n@@ -126,8 +125,6 @@ static int collect_config(const char *key_, const char *value_, void *cb)\n \t\tstrbuf_addstr(buf, key_);\n \t\tmust_print_delim = 1;\n \t}\n-\tif (values->nr > 1 && !do_all)\n-\t\tdup_error = 1;\n \tif (types == TYPE_INT)\n \t\tsprintf(value, \"%d\", git_config_int(key_, value_?value_:\"\"));\n \telse if (types == TYPE_BOOL)\n@@ -149,16 +146,12 @@ static int collect_config(const char *key_, const char *value_, void *cb)\n \t\tvptr = \"\";\n \t\tmust_print_delim = 0;\n \t}\n-\tif (dup_error) {\n-\t\terror(\"More than one value for the key %s: %s\",\n-\t\t\t\tkey_, vptr);\n-\t}\n-\telse {\n-\t\tif (must_print_delim)\n-\t\t\tstrbuf_addch(buf, key_delim);\n-\t\tstrbuf_addstr(buf, vptr);\n-\t\tstrbuf_addch(buf, term);\n-\t}\n+\n+\tif (must_print_delim)\n+\t\tstrbuf_addch(buf, key_delim);\n+\tstrbuf_addstr(buf, vptr);\n+\tstrbuf_addch(buf, term);\n+\n \tif (must_free_vptr)\n \t\t/* If vptr must be freed, it's a pointer to a\n \t\t * dynamically allocated buffer, it's safe to cast to\n@@ -263,14 +256,12 @@ static int get_value(const char *key_, const char *regex_)\n \tif (!do_all && !values.nr && system_wide)\n \t\tgit_config_from_file(fn, system_wide, data);\n \n-\tif (do_all)\n-\t\tret = !values.nr;\n-\telse\n-\t\tret = (values.nr == 1) ? 0 : values.nr > 1 ? 2 : 1;\n+\tret = !values.nr;\n \n \tfor (i = 0; i < values.nr; i++) {\n \t\tstruct strbuf *buf = values.items + i;\n-\t\tfwrite(buf->buf, 1, buf->len, stdout);\n+\t\tif (do_all || i == values.nr - 1)\n+\t\t\tfwrite(buf->buf, 1, buf->len, stdout);\n \t\tstrbuf_release(buf);\n \t}\n \tfree(values.items);\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 74a297e..8cb45f1 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -252,8 +252,10 @@ test_expect_success 'non-match value' '\n \ttest wow = $(git config --get nextsection.nonewline !for)\n '\n \n-test_expect_success 'ambiguous get' '\n-\ttest_must_fail git config --get nextsection.nonewline\n+test_expect_success 'multi-valued get returns final one' '\n+\techo \"wow2 for me\" >expect &&\n+\tgit config --get nextsection.nonewline >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'multi-valued get-all returns all' '\ndiff --git a/t/t9700/test.pl b/t/t9700/test.pl\nindex 3b9b484..0d4e366 100755\n--- a/t/t9700/test.pl\n+++ b/t/t9700/test.pl\n@@ -46,8 +46,7 @@ BEGIN\n # Save and restore STDERR; we will probably extract this into a\n # \"dies_ok\" method and possibly move the STDERR handling to Git.pm.\n open our $tmpstderr, \">&STDERR\" or die \"cannot save STDERR\"; close STDERR;\n-eval { $r->config(\"test.dupstring\") };\n-ok($@, \"config: duplicate entry in scalar context fails\");\n+is($r->config(\"test.dupstring\"), \"value2\", \"config: multivar\");\n eval { $r->config_bool(\"test.boolother\") };\n ok($@, \"config_bool: non-boolean values fail\");\n open STDERR, \">&\", $tmpstderr or die \"cannot restore STDERR\";\n-- \n1.8.0.3.g3456896\n"},{"id":"201774","messageId":"20121023224119.GH17392@sigill.intra.peff.net","threadId":"31903","inReplyTo":"20121023223502.GA23194@sigill.intra.peff.net","subject":"[PATCH 8/8] git-config: use git_config_with_options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-23T22:41:19Z","receivedAt":"2012-10-23T22:41:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The git-config command has always implemented its own file\nlookup and parsing order. This was necessary because its\nduplicate-entry handling did not match the way git's\ninternal callbacks worked. Now that this is no longer the\ncase, we are free to reuse the existing parsing code.\n\nThis saves us a few lines of code, but most importantly, it\nmeans that the logic for which files are examined is\ncontained only in one place and cannot diverge.\n\nSigned-off-by: Jeff King \n---\n builtin/config.c | 44 ++------------------------------------------\n 1 file changed, 2 insertions(+), 42 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 77efa69..f881053 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -165,22 +165,9 @@ static int get_value(const char *key_, const char *regex_)\n static int get_value(const char *key_, const char *regex_)\n {\n \tint ret = CONFIG_GENERIC_ERROR;\n-\tchar *global = NULL, *xdg = NULL, *repo_config = NULL;\n-\tconst char *system_wide = NULL, *local;\n-\tstruct config_include_data inc = CONFIG_INCLUDE_INIT;\n-\tconfig_fn_t fn;\n-\tvoid *data;\n \tstruct strbuf_list values = {0};\n \tint i;\n \n-\tlocal = given_config_file;\n-\tif (!local) {\n-\t\tlocal = repo_config = git_pathdup(\"config\");\n-\t\tif (git_config_system())\n-\t\t\tsystem_wide = git_etc_gitconfig();\n-\t\thome_config_paths(&global, &xdg, \"config\");\n-\t}\n-\n \tif (use_key_regexp) {\n \t\tchar *tl;\n \n@@ -229,32 +216,8 @@ static int get_value(const char *key_, const char *regex_)\n \t\t}\n \t}\n \n-\tfn = collect_config;\n-\tdata = &values;\n-\tif (respect_includes) {\n-\t\tinc.fn = fn;\n-\t\tinc.data = data;\n-\t\tfn = git_config_include;\n-\t\tdata = &inc;\n-\t}\n-\n-\tif (do_all && system_wide)\n-\t\tgit_config_from_file(fn, system_wide, data);\n-\tif (do_all && xdg)\n-\t\tgit_config_from_file(fn, xdg, data);\n-\tif (do_all && global)\n-\t\tgit_config_from_file(fn, global, data);\n-\tif (do_all)\n-\t\tgit_config_from_file(fn, local, data);\n-\tgit_config_from_parameters(fn, data);\n-\tif (!do_all && !values.nr)\n-\t\tgit_config_from_file(fn, local, data);\n-\tif (!do_all && !values.nr && global)\n-\t\tgit_config_from_file(fn, global, data);\n-\tif (!do_all && !values.nr && xdg)\n-\t\tgit_config_from_file(fn, xdg, data);\n-\tif (!do_all && !values.nr && system_wide)\n-\t\tgit_config_from_file(fn, system_wide, data);\n+\tgit_config_with_options(collect_config, &values,\n+\t\t\t\tgiven_config_file, respect_includes);\n \n \tret = !values.nr;\n \n@@ -267,9 +230,6 @@ free_strings:\n \tfree(values.items);\n \n free_strings:\n-\tfree(repo_config);\n-\tfree(global);\n-\tfree(xdg);\n \tfree(key);\n \tif (key_regexp) {\n \t\tregfree(key_regexp);\n-- \n1.8.0.3.g3456896\n"},{"id":"201781","messageId":"CAEBDL5V0Ffyp306rVo-USCBy_AXXMHXN1yrWmkF1BhzFaq60nA@mail.gmail.com","threadId":"31903","inReplyTo":"CACBZZX5mOb7_i9r8AqNK5V3r-gVnzN+rkeY9xrhecGv1rS-anA@mail.gmail.com","subject":"Re: The config include mechanism doesn't allow for overwriting","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2012-10-24T00:46:47Z","receivedAt":"2012-10-24T00:46:47Z","isPatch":false,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Tue, Oct 23, 2012 at 10:13 AM, Ævar Arnfjörð Bjarmason\n wrote:\n[snip]\n> And git config --get foo.bar will give you:\n>\n> $ git config -f /tmp/test --get foo.bar\n> one\n> error: More than one value for the key foo.bar: two\n> error: More than one value for the key foo.bar: three\n>\n> I think that it would be better if the config mechanism just silently\n> overwrote keys that clobbered earlier keys like your patch does.\n>\n> But in addition can we simplify things for the consumers of\n> \"git-{config,var} -l\" by only printing:\n>\n> foo.bar=three\n>\n> Or are there too many variables like \"include.path\" that can\n> legitimately appear more than once.\n\nI frequently use pushurl in my remotes to push my master branch both\nto the original repo and my forked version. I find it very helpful in\nmy workflow, and would hate to lose that. That said, I do like the\nidea of having a config file and the ability to override some of the\nvariables.\n\n-John\n"},{"id":"201782","messageId":"20121024005130.GA19638@sigill.intra.peff.net","threadId":"31903","inReplyTo":"CAEBDL5V0Ffyp306rVo-USCBy_AXXMHXN1yrWmkF1BhzFaq60nA@mail.gmail.com","subject":"Re: The config include mechanism doesn't allow for overwriting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-24T00:51:30Z","receivedAt":"2012-10-24T00:51:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 23, 2012 at 08:46:47PM -0400, John Szakmeister wrote:\n\n> On Tue, Oct 23, 2012 at 10:13 AM, Ævar Arnfjörð Bjarmason\n> wrote:\n> [snip]\n> > And git config --get foo.bar will give you:\n> >\n> > $ git config -f /tmp/test --get foo.bar\n> > one\n> > error: More than one value for the key foo.bar: two\n> > error: More than one value for the key foo.bar: three\n> >\n> > I think that it would be better if the config mechanism just silently\n> > overwrote keys that clobbered earlier keys like your patch does.\n> >\n> > But in addition can we simplify things for the consumers of\n> > \"git-{config,var} -l\" by only printing:\n> >\n> > foo.bar=three\n> >\n> > Or are there too many variables like \"include.path\" that can\n> > legitimately appear more than once.\n> \n> I frequently use pushurl in my remotes to push my master branch both\n> to the original repo and my forked version. I find it very helpful in\n> my workflow, and would hate to lose that. That said, I do like the\n> idea of having a config file and the ability to override some of the\n> variables.\n\nNo, that won't go anywhere. We really do have two classes of variables:\nthings that are expected to be single values, and things that are\nexpected to be lists.\n\nFrom the perspective of the config code, we don't know or care which is\nwhich, and just feed all entries sequentially to a C callback. In\npractice, the callbacks do one of two things:\n\n 1. Append the values into a list.\n\n 2. Overwrite, and end up with the final value seen.\n\nThe trouble is that git-config has to print the values in a reasonable\nway, so it asks the caller to give a hint about which it wants (--get\nversus --get-all). But in the single-value case did not behave like the\nC callbacks, which is what my series fixes.\n\nUsing \"git config -l\" is more like asking the config machinery to just\nfeed us everything, which is what the C callbacks see. Which is more\nflexible, but way less convenient for the caller. But it doesn't need to\nbe fixed, since the caller has all the information to implement whatever\nsemantics they like.\n\n-Peff\n"},{"id":"201791","messageId":"50878BAB.60809@viscovery.net","threadId":"31903","inReplyTo":"20121023223554.GA17392@sigill.intra.peff.net","subject":"Re: [PATCH 1/8] t1300: style updates","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-24T06:33:15Z","receivedAt":"2012-10-24T06:33:15Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/24/2012 0:35, schrieb Jeff King:\n> -test_expect_success 'non-match value' \\\n> -\t'test wow = $(git config --get nextsection.nonewline !for)'\n> +test_expect_success 'non-match value' '\n> +\ttest wow = $(git config --get nextsection.nonewline !for)\n> +'\n\nHere's a case you forgot to update to test_cmp.\n\n> +test_expect_success 'get-regexp variable with no value' '\n> +\tgit config --get-regexp novalue > output &&\n> +\t test_cmp expect output'\n\nAnd while you are here, you might want to remove this extra space. ;)\n\nOtherwise, looks fine.\n\n-- Hannes\n"},{"id":"201792","messageId":"50878BB1.8050902@viscovery.net","threadId":"31903","inReplyTo":"20121023224119.GH17392@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] git-config: use git_config_with_options","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-24T06:33:21Z","receivedAt":"2012-10-24T06:33:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"All looked sane. Thanks for a pleasant read!\n\n-- Hannes\n"},{"id":"201793","messageId":"20121024063712.GA17789@sigill.intra.peff.net","threadId":"31903","inReplyTo":"50878BAB.60809@viscovery.net","subject":"Re: [PATCH 1/8] t1300: style updates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-24T06:37:12Z","receivedAt":"2012-10-24T06:37:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 24, 2012 at 08:33:15AM +0200, Johannes Sixt wrote:\n\n> Am 10/24/2012 0:35, schrieb Jeff King:\n> > -test_expect_success 'non-match value' \\\n> > -\t'test wow = $(git config --get nextsection.nonewline !for)'\n> > +test_expect_success 'non-match value' '\n> > +\ttest wow = $(git config --get nextsection.nonewline !for)\n> > +'\n> \n> Here's a case you forgot to update to test_cmp.\n\nThanks. I noticed I left quite a few of those in (the other changes I\ndid mechanically, but I only fixed up the \"test\" ones in nearby spots).\n\n> > +test_expect_success 'get-regexp variable with no value' '\n> > +\tgit config --get-regexp novalue > output &&\n> > +\t test_cmp expect output'\n> \n> And while you are here, you might want to remove this extra space. ;)\n> \n> Otherwise, looks fine.\n\nThanks, I'll fix up both.\n\n-Peff\n"},{"id":"201795","messageId":"20121024070701.GA29403@sigill.intra.peff.net","threadId":"31903","inReplyTo":"20121024063712.GA17789@sigill.intra.peff.net","subject":"[PATCHv2 1/8] t1300: style updates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-24T07:07:01Z","receivedAt":"2012-10-24T07:07:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 24, 2012 at 02:37:12AM -0400, Jeff King wrote:\n\n> > Here's a case you forgot to update to test_cmp.\n> [...]\n> > And while you are here, you might want to remove this extra space. ;)\n> > \n> > Otherwise, looks fine.\n> \n> Thanks, I'll fix up both.\n\nHere's an updated version of patch 1 that I'm planning on queuing. It's\nrather tedious to read, but if anybody feels like giving it one more\nrun-through, let me know if you see any problems.\n\nI won't bother re-posting the other patches, as they are unchanged on\ntop.\n\n-- >8 --\nSubject: [PATCH] t1300: style updates\n\nThe t1300 test script is quite old, and does not use our\nmodern techniques or styles. This patch updates it in the\nfollowing ways:\n\n 1. Use test_cmp instead of cmp (to make failures easier to\n debug).\n\n 2. Use test_cmp instead of 'test $(command) = expected'.\n This makes failures much easier to debug, and also\n makes sure that $(command) exits appropriately.\n\n 3. Use test_must_fail (easier to read, and checks more\n rigorously for signal death).\n\n 4. Write tests with the usual style of:\n\n test_expect_success 'test name' '\n test commands &&\n\t ...\n '\n\n rather than one-liners, or using backslash-continuation.\n This is purely a style fixup.\n\nThere are still a few command happening outside of\ntest_expect invocations, but they are all innoccuous system\ncommands like \"cat\" and \"cp\". In an ideal world, each test\nwould be self sufficient and all commands would happen\ninside test_expect, but it is not immediately obvious how\nthe grouping should work (some of the commands impact the\nsubsequent tests, and some of them are setting up and\nmodifying state that many tests depend on). This patch just\npicks the low-hanging style fruit, and we can do more fixes\non top later.\n\nSigned-off-by: Jeff King \n---\n t/t1300-repo-config.sh | 301 +++++++++++++++++++++++++++++--------------------\n 1 file changed, 178 insertions(+), 123 deletions(-)\n\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex e127f35..feb7430 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -55,11 +55,13 @@ test_expect_success 'replace with non-match' \\\n \ttest_cmp expect .git/config\n '\n \n-test_expect_success 'replace with non-match' \\\n-\t'git config core.penguin kingpin !blue'\n+test_expect_success 'replace with non-match' '\n+\tgit config core.penguin kingpin !blue\n+'\n \n-test_expect_success 'replace with non-match (actually matching)' \\\n-\t'git config core.penguin \"very blue\" !kingpin'\n+test_expect_success 'replace with non-match (actually matching)' '\n+\tgit config core.penguin \"very blue\" !kingpin\n+'\n \n cat > expect << EOF\n [core]\n@@ -108,8 +110,9 @@ EOF\n lines\n EOF\n \n-test_expect_success 'unset with cont. lines' \\\n-\t'git config --unset beta.baz'\n+test_expect_success 'unset with cont. lines' '\n+\tgit config --unset beta.baz\n+'\n \n cat > expect <<\\EOF\n [alpha]\n@@ -133,8 +136,9 @@ cp .git/config .git/config2\n \n cp .git/config .git/config2\n \n-test_expect_success 'multiple unset' \\\n-\t'git config --unset-all beta.haha'\n+test_expect_success 'multiple unset' '\n+\tgit config --unset-all beta.haha\n+'\n \n cat > expect << EOF\n [beta] ; silly comment # another comment\n@@ -145,7 +149,9 @@ EOF\n [nextSection] noNewline = ouch\n EOF\n \n-test_expect_success 'multiple unset is correct' 'test_cmp expect .git/config'\n+test_expect_success 'multiple unset is correct' '\n+\ttest_cmp expect .git/config\n+'\n \n cp .git/config2 .git/config\n \n@@ -156,8 +162,9 @@ rm .git/config2\n \n rm .git/config2\n \n-test_expect_success '--replace-all' \\\n-\t'git config --replace-all beta.haha gamma'\n+test_expect_success '--replace-all' '\n+\tgit config --replace-all beta.haha gamma\n+'\n \n cat > expect << EOF\n [beta] ; silly comment # another comment\n@@ -169,7 +176,9 @@ EOF\n [nextSection] noNewline = ouch\n EOF\n \n-test_expect_success 'all replaced' 'test_cmp expect .git/config'\n+test_expect_success 'all replaced' '\n+\ttest_cmp expect .git/config\n+'\n \n cat > expect << EOF\n [beta] ; silly comment # another comment\n@@ -200,7 +209,11 @@ test_expect_success 'really really mean test' '\n \ttest_cmp expect .git/config\n '\n \n-test_expect_success 'get value' 'test alpha = $(git config beta.haha)'\n+test_expect_success 'get value' '\n+\techo alpha >expect &&\n+\tgit config beta.haha >actual &&\n+\ttest_cmp expect actual\n+'\n \n cat > expect << EOF\n [beta] ; silly comment # another comment\n@@ -231,18 +244,23 @@ test_expect_success 'ambiguous get' '\n \ttest_cmp expect .git/config\n '\n \n-test_expect_success 'non-match' \\\n-\t'git config --get nextsection.nonewline !for'\n+test_expect_success 'non-match' '\n+\tgit config --get nextsection.nonewline !for\n+'\n \n-test_expect_success 'non-match value' \\\n-\t'test wow = $(git config --get nextsection.nonewline !for)'\n+test_expect_success 'non-match value' '\n+\techo wow >expect &&\n+\tgit config --get nextsection.nonewline !for >actual &&\n+\ttest_cmp expect actual\n+'\n \n test_expect_success 'ambiguous get' '\n \ttest_must_fail git config --get nextsection.nonewline\n '\n \n-test_expect_success 'get multivar' \\\n-\t'git config --get-all nextsection.nonewline'\n+test_expect_success 'get multivar' '\n+\tgit config --get-all nextsection.nonewline\n+'\n \n cat > expect << EOF\n [beta] ; silly comment # another comment\n@@ -290,8 +308,9 @@ test_expect_success 'correct key' 'git config 123456.a123 987'\n \n test_expect_success 'correct key' 'git config 123456.a123 987'\n \n-test_expect_success 'hierarchical section' \\\n-\t'git config Version.1.2.3eX.Alpha beta'\n+test_expect_success 'hierarchical section' '\n+\tgit config Version.1.2.3eX.Alpha beta\n+'\n \n cat > expect << EOF\n [beta] ; silly comment # another comment\n@@ -307,7 +326,9 @@ EOF\n \tAlpha = beta\n EOF\n \n-test_expect_success 'hierarchical section value' 'test_cmp expect .git/config'\n+test_expect_success 'hierarchical section value' '\n+\ttest_cmp expect .git/config\n+'\n \n cat > expect << EOF\n beta.noindent=sillyValue\n@@ -316,9 +337,10 @@ EOF\n version.1.2.3eX.alpha=beta\n EOF\n \n-test_expect_success 'working --list' \\\n-\t'git config --list > output && cmp output expect'\n-\n+test_expect_success 'working --list' '\n+\tgit config --list > output &&\n+\ttest_cmp expect output\n+'\n cat > expect << EOF\n EOF\n \n@@ -332,8 +354,10 @@ EOF\n nextsection.nonewline wow2 for me\n EOF\n \n-test_expect_success '--get-regexp' \\\n-\t'git config --get-regexp in > output && cmp output expect'\n+test_expect_success '--get-regexp' '\n+\tgit config --get-regexp in >output &&\n+\ttest_cmp expect output\n+'\n \n cat > expect << EOF\n wow2 for me\n@@ -353,41 +377,48 @@ echo false > expect\n \tvariable =\n EOF\n \n-test_expect_success 'get variable with no value' \\\n-\t'git config --get novalue.variable ^$'\n+test_expect_success 'get variable with no value' '\n+\tgit config --get novalue.variable ^$\n+'\n \n-test_expect_success 'get variable with empty value' \\\n-\t'git config --get emptyvalue.variable ^$'\n+test_expect_success 'get variable with empty value' '\n+\tgit config --get emptyvalue.variable ^$\n+'\n \n echo novalue.variable > expect\n \n-test_expect_success 'get-regexp variable with no value' \\\n-\t'git config --get-regexp novalue > output &&\n-\t cmp output expect'\n+test_expect_success 'get-regexp variable with no value' '\n+\tgit config --get-regexp novalue > output &&\n+\ttest_cmp expect output\n+'\n \n echo 'novalue.variable true' > expect\n \n-test_expect_success 'get-regexp --bool variable with no value' \\\n-\t'git config --bool --get-regexp novalue > output &&\n-\t cmp output expect'\n+test_expect_success 'get-regexp --bool variable with no value' '\n+\tgit config --bool --get-regexp novalue > output &&\n+\ttest_cmp expect output\n+'\n \n echo 'emptyvalue.variable ' > expect\n \n-test_expect_success 'get-regexp variable with empty value' \\\n-\t'git config --get-regexp emptyvalue > output &&\n-\t cmp output expect'\n+test_expect_success 'get-regexp variable with empty value' '\n+\tgit config --get-regexp emptyvalue > output &&\n+\ttest_cmp expect output\n+'\n \n echo true > expect\n \n-test_expect_success 'get bool variable with no value' \\\n-\t'git config --bool novalue.variable > output &&\n-\t cmp output expect'\n+test_expect_success 'get bool variable with no value' '\n+\tgit config --bool novalue.variable > output &&\n+\ttest_cmp expect output\n+'\n \n echo false > expect\n \n-test_expect_success 'get bool variable with empty value' \\\n-\t'git config --bool emptyvalue.variable > output &&\n-\t cmp output expect'\n+test_expect_success 'get bool variable with empty value' '\n+\tgit config --bool emptyvalue.variable > output &&\n+\ttest_cmp expect output\n+'\n \n test_expect_success 'no arguments, but no crash' '\n \ttest_must_fail git config >output 2>&1 &&\n@@ -427,8 +458,9 @@ test_expect_success 'new variable inserts into proper section' '\n \ttest_cmp expect .git/config\n '\n \n-test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' \\\n-\t'test_must_fail git config --file non-existing-config -l'\n+test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' '\n+\ttest_must_fail git config --file non-existing-config -l\n+'\n \n cat > other-config << EOF\n [ein]\n@@ -444,8 +476,10 @@ test_expect_success 'alternative GIT_CONFIG' '\n \ttest_cmp expect output\n '\n \n-test_expect_success 'alternative GIT_CONFIG (--file)' \\\n-\t'git config --file other-config -l > output && cmp output expect'\n+test_expect_success 'alternative GIT_CONFIG (--file)' '\n+\tgit config --file other-config -l > output &&\n+\ttest_cmp expect output\n+'\n \n test_expect_success 'refer config from subdirectory' '\n \tmkdir x &&\n@@ -489,8 +523,9 @@ EOF\n weird\n EOF\n \n-test_expect_success \"rename section\" \\\n-\t\"git config --rename-section branch.eins branch.zwei\"\n+test_expect_success 'rename section' '\n+\tgit config --rename-section branch.eins branch.zwei\n+'\n \n cat > expect << EOF\n # Hallo\n@@ -503,17 +538,22 @@ test_expect_success \"rename succeeded\" \"test_cmp expect .git/config\"\n weird\n EOF\n \n-test_expect_success \"rename succeeded\" \"test_cmp expect .git/config\"\n+test_expect_success 'rename succeeded' '\n+\ttest_cmp expect .git/config\n+'\n \n-test_expect_success \"rename non-existing section\" '\n+test_expect_success 'rename non-existing section' '\n \ttest_must_fail git config --rename-section \\\n \t\tbranch.\"world domination\" branch.drei\n '\n \n-test_expect_success \"rename succeeded\" \"test_cmp expect .git/config\"\n+test_expect_success 'rename succeeded' '\n+\ttest_cmp expect .git/config\n+'\n \n-test_expect_success \"rename another section\" \\\n-\t'git config --rename-section branch.\"1 234 blabl/a\" branch.drei'\n+test_expect_success 'rename another section' '\n+\tgit config --rename-section branch.\"1 234 blabl/a\" branch.drei\n+'\n \n cat > expect << EOF\n # Hallo\n@@ -526,14 +566,17 @@ EOF\n weird\n EOF\n \n-test_expect_success \"rename succeeded\" \"test_cmp expect .git/config\"\n+test_expect_success 'rename succeeded' '\n+\ttest_cmp expect .git/config\n+'\n \n cat >> .git/config << EOF\n [branch \"vier\"] z = 1\n EOF\n \n-test_expect_success \"rename a section with a var on the same line\" \\\n-\t'git config --rename-section branch.vier branch.zwei'\n+test_expect_success 'rename a section with a var on the same line' '\n+\tgit config --rename-section branch.vier branch.zwei\n+'\n \n cat > expect << EOF\n # Hallo\n@@ -548,7 +591,9 @@ EOF\n \tz = 1\n EOF\n \n-test_expect_success \"rename succeeded\" \"test_cmp expect .git/config\"\n+test_expect_success 'rename succeeded' '\n+\ttest_cmp expect .git/config\n+'\n \n test_expect_success 'renaming empty section name is rejected' '\n \ttest_must_fail git config --rename-section branch.zwei \"\"\n@@ -562,7 +607,9 @@ EOF\n [branch \"zwei\"] a = 1 [branch \"vier\"]\n EOF\n \n-test_expect_success \"remove section\" \"git config --remove-section branch.zwei\"\n+test_expect_success 'remove section' '\n+\tgit config --remove-section branch.zwei\n+'\n \n cat > expect << EOF\n # Hallo\n@@ -571,8 +618,9 @@ EOF\n weird\n EOF\n \n-test_expect_success \"section was removed properly\" \\\n-\t\"test_cmp expect .git/config\"\n+test_expect_success 'section was removed properly' '\n+\ttest_cmp expect .git/config\n+'\n \n cat > expect << EOF\n [gitcvs]\n@@ -583,7 +631,6 @@ test_expect_success 'section ending' '\n EOF\n \n test_expect_success 'section ending' '\n-\n \trm -f .git/config &&\n \tgit config gitcvs.enabled true &&\n \tgit config gitcvs.ext.dbname %Ggitcvs1.%a.%m.sqlite &&\n@@ -593,30 +640,25 @@ test_expect_success 'invalid unit' '\n '\n \n test_expect_success numbers '\n-\n \tgit config kilo.gram 1k &&\n \tgit config mega.ton 1m &&\n-\tk=$(git config --int --get kilo.gram) &&\n-\ttest z1024 = \"z$k\" &&\n-\tm=$(git config --int --get mega.ton) &&\n-\ttest z1048576 = \"z$m\"\n+\techo 1024 >expect &&\n+\techo 1048576 >>expect &&\n+\tgit config --int --get kilo.gram >actual &&\n+\tgit config --int --get mega.ton >>actual &&\n+\ttest_cmp expect actual\n '\n \n-cat > expect <actual\n-\tthen\n-\t\techo config should have failed\n-\t\tfalse\n-\tfi &&\n-\tcmp actual expect\n+\techo 1auto >expect &&\n+\tgit config aninvalid.unit >actual &&\n+\ttest_cmp expect actual &&\n+\tcat > expect <<-\\EOF\n+\tfatal: bad config value for '\\''aninvalid.unit'\\'' in .git/config\n+\tEOF\n+\ttest_must_fail git config --int --get aninvalid.unit 2>actual &&\n+\ttest_cmp actual expect\n '\n \n cat > expect << EOF\n@@ -646,7 +688,7 @@ test_expect_success bool '\n \t git config --bool --get bool.true$i >>result\n \t git config --bool --get bool.false$i >>result\n done &&\n-\tcmp expect result'\n+\ttest_cmp expect result'\n \n test_expect_success 'invalid bool (--get)' '\n \n@@ -680,7 +722,7 @@ test_expect_success 'set --bool' '\n \tgit config --bool bool.false2 \"\" &&\n \tgit config --bool bool.false3 nO &&\n \tgit config --bool bool.false4 FALSE &&\n-\tcmp expect .git/config'\n+\ttest_cmp expect .git/config'\n \n cat > expect <<\\EOF\n [int]\n@@ -695,39 +737,37 @@ cat >expect <<\\EOF\n \tgit config --int int.val1 01 &&\n \tgit config --int int.val2 -1 &&\n \tgit config --int int.val3 5m &&\n-\tcmp expect .git/config'\n+\ttest_cmp expect .git/config\n+'\n \n-cat >expect <<\\EOF\n-[bool]\n-\ttrue1 = true\n+test_expect_success 'get --bool-or-int' '\n+\tcat >.git/config <<-\\EOF &&\n+\t[bool]\n+\ttrue1\n \ttrue2 = true\n-\tfalse1 = false\n-\tfalse2 = false\n-[int]\n+\tfalse = false\n+\t[int]\n \tint1 = 0\n \tint2 = 1\n \tint3 = -1\n-EOF\n-\n-test_expect_success 'get --bool-or-int' '\n-\trm -f .git/config &&\n-\t(\n-\t\techo \"[bool]\"\n-\t\techo true1\n-\t\techo true2 = true\n-\t\techo false = false\n-\t\techo \"[int]\"\n-\t\techo int1 = 0\n-\t\techo int2 = 1\n-\t\techo int3 = -1\n-\t) >>.git/config &&\n-\ttest $(git config --bool-or-int bool.true1) = true &&\n-\ttest $(git config --bool-or-int bool.true2) = true &&\n-\ttest $(git config --bool-or-int bool.false) = false &&\n-\ttest $(git config --bool-or-int int.int1) = 0 &&\n-\ttest $(git config --bool-or-int int.int2) = 1 &&\n-\ttest $(git config --bool-or-int int.int3) = -1\n-\n+\tEOF\n+\tcat >expect <<-\\EOF &&\n+\ttrue\n+\ttrue\n+\tfalse\n+\t0\n+\t1\n+\t-1\n+\tEOF\n+\t{\n+\t\tgit config --bool-or-int bool.true1 &&\n+\t\tgit config --bool-or-int bool.true2 &&\n+\t\tgit config --bool-or-int bool.false &&\n+\t\tgit config --bool-or-int int.int1 &&\n+\t\tgit config --bool-or-int int.int2 &&\n+\t\tgit config --bool-or-int int.int3\n+\t} >actual &&\n+\ttest_cmp expect actual\n '\n \n cat >expect <<\\EOF\n@@ -844,7 +884,7 @@ test_expect_success 'value continued on next line' '\n \n test_expect_success 'value continued on next line' '\n \tgit config --list > result &&\n-\tcmp result expect\n+\ttest_cmp result expect\n '\n \n cat > .git/config <<\\EOF\n@@ -880,11 +920,12 @@ test_expect_success SYMLINKS 'symlinked configuration' '\n \n test_expect_success 'inner whitespace kept verbatim' '\n \tgit config section.val \"foo \t bar\" &&\n-\ttest \"z$(git config section.val)\" = \"zfoo \t bar\"\n+\techo \"foo \t bar\" >expect &&\n+\tgit config section.val >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success SYMLINKS 'symlinked configuration' '\n-\n \tln -s notyet myconfig &&\n \tGIT_CONFIG=myconfig git config test.frotz nitfol &&\n \ttest -h myconfig &&\n@@ -893,9 +934,15 @@ test_expect_success SYMLINKS 'symlinked configuration' '\n \tGIT_CONFIG=myconfig git config test.xyzzy rezrov &&\n \ttest -h myconfig &&\n \ttest -f notyet &&\n-\ttest \"z$(GIT_CONFIG=notyet git config test.frotz)\" = znitfol &&\n-\ttest \"z$(GIT_CONFIG=notyet git config test.xyzzy)\" = zrezrov\n-\n+\tcat >expect <<-\\EOF &&\n+\tnitfol\n+\trezrov\n+\tEOF\n+\t{\n+\t\tGIT_CONFIG=notyet git config test.frotz &&\n+\t\tGIT_CONFIG=notyet git config test.xyzzy\n+\t} >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'nonexistent configuration' '\n@@ -927,12 +974,20 @@ test_expect_success 'git -c \"key=value\" support' '\n \tgit commit -m 'initial commit' &&\n \tgit config branch.master.mergeoptions 'echo \\\"' &&\n \ttest_must_fail git merge master\n-\t\"\n+\"\n \n test_expect_success 'git -c \"key=value\" support' '\n-\ttest \"z$(git -c core.name=value config core.name)\" = zvalue &&\n-\ttest \"z$(git -c foo.CamelCase=value config foo.camelcase)\" = zvalue &&\n-\ttest \"z$(git -c foo.flag config --bool foo.flag)\" = ztrue &&\n+\tcat >expect <<-\\EOF &&\n+\tvalue\n+\tvalue\n+\ttrue\n+\tEOF\n+\t{\n+\t\tgit -c core.name=value config core.name &&\n+\t\tgit -c foo.CamelCase=value config foo.camelcase &&\n+\t\tgit -c foo.flag config --bool foo.flag\n+\t} >actual &&\n+\ttest_cmp expect actual &&\n \ttest_must_fail git -c name=value config core.name\n '\n \n-- \n1.8.0.3.g3456896\n"},{"id":"201822","messageId":"CACBZZX7NtpzDJsRa7ie6GhWE3b0VUMvTgeNkuxhF3UuQJtTNKQ@mail.gmail.com","threadId":"31903","inReplyTo":"50878BB1.8050902@viscovery.net","subject":"Re: [PATCH 8/8] git-config: use git_config_with_options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2012-10-24T19:14:50Z","receivedAt":"2012-10-24T19:14:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Yeah same here. Thanks for tackling this bug. Looking forward to using\nthe include mechanism for overriding user.email in future versions.\n"},{"id":"203564","messageId":"7vsj84rt1g.fsf@alter.siamese.dyndns.org","threadId":"31903","inReplyTo":"20121023223502.GA23194@sigill.intra.peff.net","subject":"Re: [PATCH 0/8] fix git-config with duplicate variable entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-20T19:08:43Z","receivedAt":"2012-11-20T19:08:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Oct 23, 2012 at 04:13:44PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > It fails a few tests in t1300, but it looks like those tests are testing\n>> > for the behavior we have identified as wrong, and should be fixed.\n>> \n>> I think this patch looks good.\n>\n> Thanks. It had a few minor flaws (like a memory leak). I fixed those,\n> updated the tests, and split it out into a few more readable commits. In\n> the process, I managed to uncover and fix a few other memory leaks in\n> the area. I think this version is much more readable, and writing the\n> rationale for patch 7 convinced me that it's the right thing to do.\n> Another round of review would be appreciated.\n>\n> [1/8]: t1300: style updates\n> [2/8]: t1300: remove redundant test\n> [3/8]: t1300: test \"git config --get-all\" more thoroughly\n> [4/8]: git-config: remove memory leak of key regexp\n> [5/8]: git-config: fix regexp memory leaks on error conditions\n> [6/8]: git-config: collect values instead of immediately printing\n> [7/8]: git-config: do not complain about duplicate entries\n> [8/8]: git-config: use git_config_with_options\n>\n> For those just joining us, the interesting bit is patch 7, which fixes\n> some inconsistencies between the \"git-config\" tool and how the internal\n> config callbacks work.\n\n\nThe way for the Porcelain scripts to mimick the internal \"last one\nwins\" has been to grab values out of --get-all and do the \"last one\nwins\" themselves, and I agree that a mode that frees them from\nhaving to worry about it is a good *addition*. I would even say\nthat if we were designing \"git config\" plumbing without existing\nusers, it would be the only sensible behaviour for \"--get\".\n\nBut \"git config --get\" users have been promised to get errors on\nduplicate values so far, so I have to say this needs to come with a\nbig red sign about API breakage.\n\nI would feel safer to introduce --get-one or something now, and\nworry about migration as a separate step.\n"},{"id":"203617","messageId":"20121121192738.GA16280@sigill.intra.peff.net","threadId":"31903","inReplyTo":"7vsj84rt1g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/8] fix git-config with duplicate variable entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-21T19:27:39Z","receivedAt":"2012-11-21T19:27:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 20, 2012 at 11:08:43AM -0800, Junio C Hamano wrote:\n\n> > Thanks. It had a few minor flaws (like a memory leak). I fixed those,\n> > updated the tests, and split it out into a few more readable commits. In\n> > the process, I managed to uncover and fix a few other memory leaks in\n> > the area. I think this version is much more readable, and writing the\n> > rationale for patch 7 convinced me that it's the right thing to do.\n> > Another round of review would be appreciated.\n> >\n> > [1/8]: t1300: style updates\n> > [2/8]: t1300: remove redundant test\n> > [3/8]: t1300: test \"git config --get-all\" more thoroughly\n> > [4/8]: git-config: remove memory leak of key regexp\n> > [5/8]: git-config: fix regexp memory leaks on error conditions\n> > [6/8]: git-config: collect values instead of immediately printing\n> > [7/8]: git-config: do not complain about duplicate entries\n> > [8/8]: git-config: use git_config_with_options\n> >\n> > For those just joining us, the interesting bit is patch 7, which fixes\n> > some inconsistencies between the \"git-config\" tool and how the internal\n> > config callbacks work.\n> \n> The way for the Porcelain scripts to mimick the internal \"last one\n> wins\" has been to grab values out of --get-all and do the \"last one\n> wins\" themselves, and I agree that a mode that frees them from\n> having to worry about it is a good *addition*. I would even say\n> that if we were designing \"git config\" plumbing without existing\n> users, it would be the only sensible behaviour for \"--get\".\n> \n> But \"git config --get\" users have been promised to get errors on\n> duplicate values so far, so I have to say this needs to come with a\n> big red sign about API breakage.\n\nThe problem is that scripts currently using \"--get\" are broken by\nduplicate entries in include files, whereas internal git handles them\njust fine. Introducing a new switch means that everybody also has to\nstart using it.\n\nThat is not an excuse for breakage, of course, but only a motivation for\nconsidering it. And here is what I think mitigates that breakage:\n\n 1. If you are a script that cares about multiple values and choosing\n one (whether last-one-wins or otherwise), you are already using\n --get-all and are not affected.\n\n 2. If you are a script that only wants to mimic how get retrieves\n a single value, then this is a bug fix, as it brings \"--get\" in\n line with git's internal use.\n\n 3. If you are a script that only wants a single value, but cares about\n duplicates, you are already failing to notice them when the\n duplicates are cross-file. That is, we already do \"last one wins\"\n if an entry is found in ~/.gitconfig and .git/config.\n\nI would argue that anybody fetching standard git config variables (and\nnot using --list or --get-all) falls into case (2) and is being fixed,\nas they will not otherwise match the behavior of git itself.\n\nFor other variables that porcelains want to stuff inside the config\nfiles, I suppose they could fall into case (3). But I am not sure why\nthey would care about duplicates. They have asked git for a single\nvalue, and if they care more deeply about multiple values (but only\nwithin a single file!), what do they hope to accomplish by not calling\n--get-all? That is, what is the use case where this makes any sense?\n\nThe only case I can think of where the distinction even matters is:\n\n 1. Porcelain foo writes to the .gitfoo file via \"git config -f\n .gitfoo\".\n\n 2. It accidentally writes using \"--add\" instead of just setting the\n value.\n\n 3. It fetches via \"git config -f .gitfoo --get foo.var\". Before my\n patch, duplicate detection would notice the bug in (2) and barf.\n Now, it silently takes the most recently added value (which is\n probably what was meant anyway).\n\nIOW, I do not see a legitimate use case for this, but see it as an extra\ncheck on broken config. But it is catching an unlikely condition, and is\nan overly restrictive check, in that it is triggering on totally\nreasonable config. So we are not breaking a use case so much as a\nloosening a (in my opinion) useless check.\n\nBut maybe I am missing some more sane case.\n\n> I would feel safer to introduce --get-one or something now, and\n> worry about migration as a separate step.\n\nPart of my impetus for fixing --get is that it let us cleanup and\nharmonize the git_config() and git-config implementations. If we are not\ngoing to do that, adding an extra option is not that appealing, as we\nare stuck with the old duplicated code, anyway. As I mentioned above,\nthis only really affects include files (because from git-config's\nperspective, the entries it sees are all \"the same file\", as they come\nfrom the same call into git_config_from_file). If we are not going to\nfix --get for all callers, it would probably make more sense to just\nomit the duplicate suppression for entries across include-file\nboundaries (which are something that callers would have to opt into when\nusing a specific file, anyway). IOW, to treat it as a bug in the include\nsystem and fix it there.\n\n-Peff\n"},{"id":"203621","messageId":"7vehjm20yu.fsf@alter.siamese.dyndns.org","threadId":"31903","inReplyTo":"20121121192738.GA16280@sigill.intra.peff.net","subject":"Re: [PATCH 0/8] fix git-config with duplicate variable entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-21T19:46:33Z","receivedAt":"2012-11-21T19:46:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> The way for the Porcelain scripts to mimick the internal \"last one\n>> wins\" has been to grab values out of --get-all and do the \"last one\n>> wins\" themselves, and I agree that a mode that frees them from\n>> having to worry about it is a good *addition*. I would even say\n>> that if we were designing \"git config\" plumbing without existing\n>> users, it would be the only sensible behaviour for \"--get\".\n>> \n>> But \"git config --get\" users have been promised to get errors on\n>> duplicate values so far, so I have to say this needs to come with a\n>> big red sign about API breakage.\n>\n> The problem is that scripts currently using \"--get\" are broken by\n> duplicate entries in include files, whereas internal git handles them\n> just fine. Introducing a new switch means that everybody also has to\n> start using it.\n\nNot exactly. There are three classes of people:\n\n - wrote scripts using --get; found out that --get barfs if you feed\n two or more of the same, and have long learned to accept it as a\n limitation and adjusted their configuration files to avoid it.\n They have been doing just fine and wouldn't benefit from this\n series at all.\n\n - wrote scripts using --get, relying on it barfing if fed zero\n (i.e. missing) or two or more (i.e. ambiguous), perhaps a way to\n keep their configuration files (arguably unnecessarily) clean.\n They are directly harmed by this series.\n\n - wrote scripts using --get-all and did the last-one-wins\n themselves. They wouldn't benefit directly from this series,\n unless they are willing to spend a bit of time to remove their\n own last-one-wins logic and replace --get-all with --get (but the\n same effort is needed to migrate to --get-one).\n\n - wanted to write scripts using --get, but after finding out that\n it barfs if you feed two, gave up emulating the internal, without\n realizing that they could do so with --get-all. They can now\n write their scripts without using --get-all.\n\nThe only people who benefit are from the last class; it does not\nmatter if they have to write --get-one or --get.\n\n> That is not an excuse for breakage, of course, but only a motivation for\n> considering it. And here is what I think mitigates that breakage:\n>\n> 1. If you are a script that cares about multiple values and choosing\n> one (whether last-one-wins or otherwise), you are already using\n> --get-all and are not affected.\n\nCorrect.\n\n> 2. If you are a script that only wants to mimic how get retrieves\n> a single value, then this is a bug fix, as it brings \"--get\" in\n> line with git's internal use.\n\nBut by definition, no such script exists (if it used \"--get\", it\ndidn't mimic the internal in the first place).\n\n> 3. If you are a script that only wants a single value, but cares about\n> duplicates, you are already failing to notice them when the\n> duplicates are cross-file. That is, we already do \"last one wins\"\n> if an entry is found in ~/.gitconfig and .git/config.\n\nYeah, so the only ones that can be harmed is a config lint that uses\nthe --get option with --file to make sure variables they know must\nbe single value are indeed so, and they are not doing a thorough\njob, unless they are checking across files themselves, at which\npoint they are better off using --get-all piped to \"sort | uniq -c\"\nor something. So breakages do not matter much for correctly written\nscripts.\n\n> I would argue that anybody fetching standard git config variables (and\n> not using --list or --get-all) falls into case (2) and is being fixed,\n> as they will not otherwise match the behavior of git itself.\n\nAs people shove random stuff that git itself does not care about in\ntheir config files, they may not care, though.\n\n> IOW, I do not see a legitimate use case for this, but see it as an extra\n> check on broken config.\n\nYes, I agree with the latter half of that sentence.\n"},{"id":"203625","messageId":"20121121200624.GF16280@sigill.intra.peff.net","threadId":"31903","inReplyTo":"7vehjm20yu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/8] fix git-config with duplicate variable entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-21T20:06:24Z","receivedAt":"2012-11-21T20:06:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 21, 2012 at 11:46:33AM -0800, Junio C Hamano wrote:\n\n> > The problem is that scripts currently using \"--get\" are broken by\n> > duplicate entries in include files, whereas internal git handles them\n> > just fine. Introducing a new switch means that everybody also has to\n> > start using it.\n> \n> Not exactly. There are three classes of people:\n> \n> - wrote scripts using --get; found out that --get barfs if you feed\n> two or more of the same, and have long learned to accept it as a\n> limitation and adjusted their configuration files to avoid it.\n> They have been doing just fine and wouldn't benefit from this\n> series at all.\n> \n> - wrote scripts using --get, relying on it barfing if fed zero\n> (i.e. missing) or two or more (i.e. ambiguous), perhaps a way to\n> keep their configuration files (arguably unnecessarily) clean.\n> They are directly harmed by this series.\n> \n> - wrote scripts using --get-all and did the last-one-wins\n> themselves. They wouldn't benefit directly from this series,\n> unless they are willing to spend a bit of time to remove their\n> own last-one-wins logic and replace --get-all with --get (but the\n> same effort is needed to migrate to --get-one).\n> \n> - wanted to write scripts using --get, but after finding out that\n> it barfs if you feed two, gave up emulating the internal, without\n> realizing that they could do so with --get-all. They can now\n> write their scripts without using --get-all.\n\nI have a feeling that your cases 2 and 4 do not really exist. Because we\ndid \"last one wins\" in the case that it mattered (between different\nfiles), it was always \"good enough\" to just assume that using \"--get\"\nbehaved like git did internally, and nobody really noticed or cared that\nwe did duplicate detection at all. But that is just from my own\nperspective; it is not like I did a complete survey of git-config users.\n\nMore compelling to me is that the only ones negatively affected are your\ncase 2, and that is qualified by the \"arguably unnecessary\" you wrote.\n\nEveryone else is not negatively impacted, and can later move to using\n\"--get\" if they want to give up any home-grown --get-all code.\n\n> > 2. If you are a script that only wants to mimic how get retrieves\n> > a single value, then this is a bug fix, as it brings \"--get\" in\n> > line with git's internal use.\n> \n> But by definition, no such script exists (if it used \"--get\", it\n> didn't mimic the internal in the first place).\n\nThey do not exist if we assume that anyone using \"--get\" carefully\nthought about the distinction. But I have the impression that is not the\ncase, and that they _meant_ to behave just like git, and did not realize\nthey were not doing so. Even our own scripts are guilty of this.\n\n> Yeah, so the only ones that can be harmed is a config lint that uses\n> the --get option with --file to make sure variables they know must\n> be single value are indeed so, and they are not doing a thorough\n> job, unless they are checking across files themselves, at which\n> point they are better off using --get-all piped to \"sort | uniq -c\"\n> or something. So breakages do not matter much for correctly written\n> scripts.\n\nRight.\n\n> > IOW, I do not see a legitimate use case for this, but see it as an extra\n> > check on broken config.\n> \n> Yes, I agree with the latter half of that sentence.\n\nSo what do we want to do?\n\n-Peff\n"},{"id":"203628","messageId":"7v624y1ydr.fsf@alter.siamese.dyndns.org","threadId":"31903","inReplyTo":"20121121200624.GF16280@sigill.intra.peff.net","subject":"Re: [PATCH 0/8] fix git-config with duplicate variable entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-21T20:42:24Z","receivedAt":"2012-11-21T20:42:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So what do we want to do?\n\nNothing. We'd just let it graduate along with other topics, and\ndeal with a case where somebody screams, which is unlikely to happen\n;-).\n"},{"id":"203693","messageId":"k8njm8$b1l$1@ger.gmane.org","threadId":"31903","inReplyTo":"7vehjm20yu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/8] fix git-config with duplicate variable entries","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-11-23T10:37:51Z","receivedAt":"2012-11-23T10:37:51Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"Junio C Hamano wrote:\n> Jeff King writes:\n\n...\n\n> Not exactly. There are three classes of people:\n>\n> - wrote scripts using --get; found out that --get barfs if you feed\n> two or more of the same, and have long learned to accept it as a\n> limitation and adjusted their configuration files to avoid it.\n> They have been doing just fine and wouldn't benefit from this\n> series at all.\n>\n> - wrote scripts using --get, relying on it barfing if fed zero\n> (i.e. missing) or two or more (i.e. ambiguous), perhaps a way to\n> keep their configuration files (arguably unnecessarily) clean.\n> They are directly harmed by this series.\n>\n> - wrote scripts using --get-all and did the last-one-wins\n> themselves. They wouldn't benefit directly from this series,\n> unless they are willing to spend a bit of time to remove their\n> own last-one-wins logic and replace --get-all with --get (but the\n> same effort is needed to migrate to --get-one).\n>\n> - wanted to write scripts using --get, but after finding out that\n> it barfs if you feed two, gave up emulating the internal, without\n> realizing that they could do so with --get-all. They can now\n> write their scripts without using --get-all.\n\nThere are three classes ofpeople: those that can count and those that can't\n\nSorry could not resist ;-) \n"}]}