{"thread":{"id":"45146","subject":"[BUG] submodule config does not apply to upper case submodules?","startedAt":"2017-02-15T11:17:19Z","lastAt":"2017-02-24T06:11:31Z","messageCount":41,"participants":["Lars Schneider","Stefan Beller","Junio C Hamano","Jonathan Tan","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"311640","messageId":"20170215111704.78320-1-larsxschneider@gmail.com","threadId":"45146","inReplyTo":null,"subject":"[BUG] submodule config does not apply to upper case submodules?","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-02-15T11:17:04Z","receivedAt":"2017-02-15T11:17:19Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"It looks like as if submodule configs (\"submodule.*\") for submodules\nwith upper case names are ignored. The test cases shows that skipping\na submodule during a recursive clone seems not to work.\n\nSigned-off-by: Lars Schneider \n---\n\nI observed the bug on Windows, macOS, and Linux and at least on\nv2.11.0 and v2.11.1:\nhttps://travis-ci.org/larsxschneider/git/builds/201828672\n\nRight now I have no time to fix it but I might be able to look into it\nnext week (if no one else tackles it before that).\n\nCheers,\nLars\n\n\nNotes:\n Base Commit: 3b9e3c2ced (v2.11.1)\n Diff on Web: https://github.com/larsxschneider/git/commit/a122feaf9f\n Checkout: git fetch https://github.com/larsxschneider/git submodule/uppercase-bug-v1 && git checkout a122feaf9f\n\n t/t7400-submodule-basic.sh | 34 ++++++++++++++++++++++++++++++++++\n 1 file changed, 34 insertions(+)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex b77cce8e40..83b5c0d1e0 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1116,5 +1116,39 @@ test_expect_success 'submodule helper list is not confused by common prefixes' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success 'submodule config does not apply to upper case submodules' '\n+\ttest_when_finished \"rm -rf super lowersub clone-success clone-failure\" &&\n+\tmkdir lowersub &&\n+\t(\n+\t\tcd lowersub &&\n+\t\tgit init &&\n+\t\t>t &&\n+\t\tgit add t &&\n+\t\tgit commit -m \"initial commit lowersub\"\n+\t) &&\n+\tmkdir UPPERSUB &&\n+\t(\n+\t\tcd UPPERSUB &&\n+\t\tgit init &&\n+\t\t>t &&\n+\t\tgit add t &&\n+\t\tgit commit -m \"initial commit UPPERSUB\"\n+\t) &&\n+\tmkdir super &&\n+\t(\n+\t\tcd super &&\n+\t\tgit init &&\n+\t\t>t &&\n+\t\tgit add t &&\n+\t\tgit commit -m \"initial commit super\" &&\n+\t\tgit submodule add ../lowersub &&\n+\t\tgit submodule add ../UPPERSUB &&\n+\t\tgit commit -m \"add submodules\"\n+\t) &&\n+\tgit -c submodule.lowersub.update=none clone --recursive super clone-success 2>&1 |\n+\t\tgrep \"Skipping submodule\" &&\n+\tgit -c submodule.UPPERSUB.update=none clone --recursive super clone-failure 2>&1 |\n+\t\tgrep \"Skipping submodule\"\n+'\n\n test_done\n\nbase-commit: 3b9e3c2cede15057af3ff8076c45ad5f33829436\n--\n2.11.0\n\n"},{"id":"311641","messageId":"20170215113325.14393-1-larsxschneider@gmail.com","threadId":"45146","inReplyTo":"20170215111704.78320-1-larsxschneider@gmail.com","subject":"[PATCH v1] t7400: cleanup \"submodule add clone shallow submodule\" test","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-02-15T11:33:25Z","receivedAt":"2017-02-15T11:33:34Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"The test creates a \"super\" directory that is not removed after the\ntest finished. This directory is not used in any subsequent tests and\nshould therefore be removed.\n\nSigned-off-by: Lars Schneider \n---\n\nI just noticed that my bug report test does not run properly without this\npatch: http://public-inbox.org/git/20170215111704.78320-1-larsxschneider@gmail.com/\n\n@Junio: I think this patch should be applied regardless of the bug.\n\nThank,\nLars\n\nNotes:\n Base Commit: 3b9e3c2ced (v2.11.1)\n Diff on Web: https://github.com/larsxschneider/git/commit/1699a6894e\n Checkout: git fetch https://github.com/larsxschneider/git submodule/cleanup-test-v1 && git checkout 1699a6894e\n\n t/t7400-submodule-basic.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex b77cce8e40..08df483280 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1078,6 +1078,7 @@ test_expect_success 'submodule with UTF-8 name' '\n '\n\n test_expect_success 'submodule add clone shallow submodule' '\n+\ttest_when_finished \"rm -rf super\" &&\n \tmkdir super &&\n \tpwd=$(pwd) &&\n \t(\n\nbase-commit: 3b9e3c2cede15057af3ff8076c45ad5f33829436\n--\n2.11.0\n\n"},{"id":"311658","messageId":"CAGZ79kboUB=1NRDw_=yvUhm4US-TtfyNA5uu4XetkXAEfkBr+A@mail.gmail.com","threadId":"45146","inReplyTo":"20170215111704.78320-1-larsxschneider@gmail.com","subject":"Re: [BUG] submodule config does not apply to upper case submodules?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-15T18:14:54Z","receivedAt":"2017-02-15T18:15:02Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 15, 2017 at 3:17 AM, Lars Schneider\n wrote:\n> It looks like as if submodule configs (\"submodule.*\") for submodules\n> with upper case names are ignored. The test cases shows that skipping\n> a submodule during a recursive clone seems not to work.\n>\n> Signed-off-by: Lars Schneider \n> ---\n>\n> I observed the bug on Windows, macOS, and Linux and at least on\n> v2.11.0 and v2.11.1:\n> https://travis-ci.org/larsxschneider/git/builds/201828672\n\nThanks for the bug report.\n\n>\n> Right now I have no time to fix it but I might be able to look into it\n> next week (if no one else tackles it before that).\n\nI might look into it before next week.\n\n> Notes:\n> Base Commit: 3b9e3c2ced (v2.11.1)\n> Diff on Web: https://github.com/larsxschneider/git/commit/a122feaf9f\n> Checkout: git fetch https://github.com/larsxschneider/git submodule/uppercase-bug-v1 && git checkout a122feaf9f\n\nI like these notes, though base commit is duplicate with below.\n\n\n> +test_expect_success 'submodule config does not apply to upper case submodules' '\n...\n> + git submodule add ../UPPERSUB &&\n> + git commit -m \"add submodules\"\n> + ) &&\n\nup to here we only do \"setup\"-sy stuff.\n(\"setup being a trigger word that you cannot omit\nthe test for subsequent tests to work)\nSo maybe have\n test_expect_success 'setup submodule with lower and uppercase' '\n ...\n '\n test_expect_success 'just the clone' '\n ...\n '\n test_expect_success ' check for lower case'\n grep -e \"Skipping submodule *lowersub*\" err\n '\n test_expect_failure ' check for upper case'\n grep ...\n '\n> + git -c submodule.lowersub.update=none clone --recursive super clone-success 2>&1 |\n> + grep \"Skipping submodule\" &&\n> + git -c submodule.UPPERSUB.update=none clone --recursive super clone-failure 2>&1 |\n> + grep \"Skipping submodule\"\n\nI'd rather give both options in one invocation and then grep from a file, e.g.\n\n git -c submodule.lowersub.update=none -c submodule.UPPERSUB.update=none \\\n clone --recursive super super_clone 2>err 1>out &&\n grep -e \"Skipping submodule *lowersub*\" err\n\n> +'\n>\n> test_done\n>\n\n> base-commit: 3b9e3c2cede15057af3ff8076c45ad5f33829436\n\nHeh, I see what you did here. :)\n\n> --\n> 2.11.0\n>\n"},{"id":"311661","messageId":"CAGZ79kaP5+vXYv=0fV+-dvLy_i0oH_f-5dvSzrap3zjK3kDvnQ@mail.gmail.com","threadId":"45146","inReplyTo":"20170215113325.14393-1-larsxschneider@gmail.com","subject":"Re: [PATCH v1] t7400: cleanup \"submodule add clone shallow submodule\" test","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-15T18:29:36Z","receivedAt":"2017-02-15T18:29:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 15, 2017 at 3:33 AM, Lars Schneider\n wrote:\n> @Junio: I think this patch should be applied regardless of the bug.\n\nSounds good; thanks!\nStefan\n"},{"id":"311663","messageId":"xmqq4lzv47t7.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"20170215113325.14393-1-larsxschneider@gmail.com","subject":"Re: [PATCH v1] t7400: cleanup \"submodule add clone shallow submodule\" test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T18:39:48Z","receivedAt":"2017-02-15T18:39:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> The test creates a \"super\" directory that is not removed after the\n> test finished. This directory is not used in any subsequent tests and\n> should therefore be removed.\n>\n> Signed-off-by: Lars Schneider \n> ---\n>\n> I just noticed that my bug report test does not run properly without this\n> patch: http://public-inbox.org/git/20170215111704.78320-1-larsxschneider@gmail.com/\n>\n> @Junio: I think this patch should be applied regardless of the bug.\n\nWithout the other one, this is not strictly needed, but I agree that\nit is a good code hygiene to make sure each test cleans up after\nitself.\n\nIs this the only one that needs change in the script from that\n\"hygiene\" point of view, or are there others? An alternative that\nis also acceptable is to squash this one into the other patch.\n\n> t/t7400-submodule-basic.sh | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index b77cce8e40..08df483280 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -1078,6 +1078,7 @@ test_expect_success 'submodule with UTF-8 name' '\n> '\n>\n> test_expect_success 'submodule add clone shallow submodule' '\n> +\ttest_when_finished \"rm -rf super\" &&\n> \tmkdir super &&\n> \tpwd=$(pwd) &&\n> \t(\n>\n> base-commit: 3b9e3c2cede15057af3ff8076c45ad5f33829436\n> --\n> 2.11.0\n"},{"id":"311666","messageId":"xmqqzihn2smp.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"20170215111704.78320-1-larsxschneider@gmail.com","subject":"Re: [BUG] submodule config does not apply to upper case submodules?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T18:53:02Z","receivedAt":"2017-02-15T18:53:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> It looks like as if submodule configs (\"submodule.*\") for submodules\n> with upper case names are ignored.\n\nThis observation is surprising, as the second level in three-level\nnames like \"
..\" is designed to be case\nsensitive. A code that uses the config API needs to do extra things\nto cause the behaviour you showed, i.e. to get submodule.U.update\nignored while submodule.l.update to be honoured. Perhaps somebody\ndowncases things too aggressively before comparing?\n\nThis is worth making it work as expected, needless to say ;-)\n"},{"id":"311695","messageId":"f238248f-0df2-19a5-581d-95c8a61b4632@google.com","threadId":"45146","inReplyTo":"xmqqzihn2smp.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] submodule config does not apply to upper case submodules?","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-15T22:54:48Z","receivedAt":"2017-02-15T22:55:01Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 02/15/2017 10:53 AM, Junio C Hamano wrote:\n> Lars Schneider writes:\n>\n>> It looks like as if submodule configs (\"submodule.*\") for submodules\n>> with upper case names are ignored.\n>\n> This observation is surprising, as the second level in three-level\n> names like \"
..\" is designed to be case\n> sensitive. A code that uses the config API needs to do extra things\n> to cause the behaviour you showed, i.e. to get submodule.U.update\n> ignored while submodule.l.update to be honoured. Perhaps somebody\n> downcases things too aggressively before comparing?\n>\n> This is worth making it work as expected, needless to say ;-)\n\nI had some time to look into this, and yes, command-line parameters are \ntoo aggressively downcased (\"git_config_parse_parameter\" calls \n\"strbuf_tolower\" on the entire key part in config.c). Updating the \noriginal patch to use \"test_global_config\" makes the test pass, and \ncommenting out the \"strbuf_tolower\" line in config.c also makes the test \npass.\n\nI'll see if I can fix this.\n"},{"id":"311696","messageId":"xmqqy3x712if.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"f238248f-0df2-19a5-581d-95c8a61b4632@google.com","subject":"Re: [BUG] submodule config does not apply to upper case submodules?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T23:02:32Z","receivedAt":"2017-02-15T23:02:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> I had some time to look into this, and yes, command-line parameters\n> are too aggressively downcased (\"git_config_parse_parameter\" calls\n> \"strbuf_tolower\" on the entire key part in config.c).\n\nAhh, thanks. So this is not about submodules at all; it is -c var=VAL\nwhere var is downcased too aggressively.\n\n"},{"id":"311697","messageId":"xmqqtw7v123n.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"xmqqy3x712if.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] submodule config does not apply to upper case submodules?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T23:11:24Z","receivedAt":"2017-02-15T23:11:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Tan writes:\n>\n>> I had some time to look into this, and yes, command-line parameters\n>> are too aggressively downcased (\"git_config_parse_parameter\" calls\n>> \"strbuf_tolower\" on the entire key part in config.c).\n>\n> Ahh, thanks. So this is not about submodules at all; it is -c var=VAL\n> where var is downcased too aggressively.\n\nPerhaps something like this?\n\n config.c | 16 +++++++++++++++-\n 1 file changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex c6b874a7bf..98bf8fee32 100644\n--- a/config.c\n+++ b/config.c\n@@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)\n \tstrbuf_release(&env);\n }\n \n+static void canonicalize_config_variable_name(struct strbuf *var)\n+{\n+\tchar *first_dot = strchr(var->buf, '.');\n+\tchar *last_dot = strrchr(var->buf, '.');\n+\tchar *cp;\n+\n+\tif (first_dot)\n+\t\tfor (cp = var->buf; *cp && cp < first_dot; cp++)\n+\t\t\t*cp = tolower(*cp);\n+\tif (last_dot)\n+\t\tfor (cp = last_dot; *cp; cp++)\n+\t\t\t*cp = tolower(*cp);\n+}\n+\n int git_config_parse_parameter(const char *text,\n \t\t\t config_fn_t fn, void *data)\n {\n@@ -223,7 +237,7 @@ int git_config_parse_parameter(const char *text,\n \t\tstrbuf_list_free(pair);\n \t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n-\tstrbuf_tolower(pair[0]);\n+\tcanonicalize_config_variable_name(pair[0]);\n \tif (fn(pair[0]->buf, value, data) < 0) {\n \t\tstrbuf_list_free(pair);\n \t\treturn -1;\n"},{"id":"311699","messageId":"CAGZ79kaKhjNPGRVJ6H=CMKQ1RKXmVvSPOMo4c3haNeS60aWQXA@mail.gmail.com","threadId":"45146","inReplyTo":"xmqqtw7v123n.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] submodule config does not apply to upper case submodules?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-15T23:28:34Z","receivedAt":"2017-02-15T23:28:57Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 15, 2017 at 3:11 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Jonathan Tan writes:\n>>\n>>> I had some time to look into this, and yes, command-line parameters\n>>> are too aggressively downcased (\"git_config_parse_parameter\" calls\n>>> \"strbuf_tolower\" on the entire key part in config.c).\n>>\n>> Ahh, thanks. So this is not about submodules at all; it is -c var=VAL\n>> where var is downcased too aggressively.\n>\n> Perhaps something like this?\n\nYes; though I'd place it in strbuf.{c,h} as it is operating\non the internals of the strbuf. (Do we make any promises outside of\nstrbuf about the internals? I mean we use .buf all the time, so maybe\nI am overly cautious here)\n\n>\n> config.c | 16 +++++++++++++++-\n> 1 file changed, 15 insertions(+), 1 deletion(-)\n>\n> diff --git a/config.c b/config.c\n> index c6b874a7bf..98bf8fee32 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)\n> strbuf_release(&env);\n> }\n>\n> +static void canonicalize_config_variable_name(struct strbuf *var)\n> +{\n> + char *first_dot = strchr(var->buf, '.');\n> + char *last_dot = strrchr(var->buf, '.');\n\nIf first_dot != NULL, then last_dot !+ NULL as well.\n(either both are NULL or none of them),\nso we can loose one condition below.\n\n> + char *cp;\n> +\n> + if (first_dot)\n> + for (cp = var->buf; *cp && cp < first_dot; cp++)\n> + *cp = tolower(*cp);\n> + if (last_dot)\n> + for (cp = last_dot; *cp; cp++)\n> + *cp = tolower(*cp);\n> +}\n> +\n> int git_config_parse_parameter(const char *text,\n> config_fn_t fn, void *data)\n"},{"id":"311700","messageId":"c609866a-f1b9-6fe5-f97a-d2180c290983@google.com","threadId":"45146","inReplyTo":"xmqqtw7v123n.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] submodule config does not apply to upper case submodules?","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-15T23:33:05Z","receivedAt":"2017-02-15T23:33:28Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 02/15/2017 03:11 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n> Perhaps something like this?\n\nThis looks good. I was hoping to unify the processing logic between this \nCLI parsing and the usual stream parsing, but this approach is probably \nsimpler.\n\n> config.c | 16 +++++++++++++++-\n> 1 file changed, 15 insertions(+), 1 deletion(-)\n>\n> diff --git a/config.c b/config.c\n> index c6b874a7bf..98bf8fee32 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)\n> \tstrbuf_release(&env);\n> }\n>\n> +static void canonicalize_config_variable_name(struct strbuf *var)\n> +{\n> +\tchar *first_dot = strchr(var->buf, '.');\n> +\tchar *last_dot = strrchr(var->buf, '.');\n> +\tchar *cp;\n> +\n> +\tif (first_dot)\n> +\t\tfor (cp = var->buf; *cp && cp < first_dot; cp++)\n\n\"*cp &&\" is unnecessary, as far as I can tell.\n\n> +\t\t\t*cp = tolower(*cp);\n> +\tif (last_dot)\n> +\t\tfor (cp = last_dot; *cp; cp++)\n> +\t\t\t*cp = tolower(*cp);\n> +}\n> +\n> int git_config_parse_parameter(const char *text,\n> \t\t\t config_fn_t fn, void *data)\n> {\n> @@ -223,7 +237,7 @@ int git_config_parse_parameter(const char *text,\n> \t\tstrbuf_list_free(pair);\n> \t\treturn error(\"bogus config parameter: %s\", text);\n> \t}\n> -\tstrbuf_tolower(pair[0]);\n> +\tcanonicalize_config_variable_name(pair[0]);\n> \tif (fn(pair[0]->buf, value, data) < 0) {\n> \t\tstrbuf_list_free(pair);\n> \t\treturn -1;\n\n"},{"id":"311703","messageId":"xmqqh93v10vy.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"CAGZ79kaKhjNPGRVJ6H=CMKQ1RKXmVvSPOMo4c3haNeS60aWQXA@mail.gmail.com","subject":"Re: [BUG] submodule config does not apply to upper case submodules?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T23:37:37Z","receivedAt":"2017-02-15T23:38:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Yes; though I'd place it in strbuf.{c,h} as it is operating\n> on the internals of the strbuf. (Do we make any promises outside of\n> strbuf about the internals? I mean we use .buf all the time, so maybe\n> I am overly cautious here)\n\nI'd rather have it not use struct strbuf as an interface. It only\nneeds to pass \"char *\" and its promise that it touches the string\nin-place without changing the length need to be documented as a\ncomment before the function.\n\n>> config.c | 16 +++++++++++++++-\n>> 1 file changed, 15 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/config.c b/config.c\n>> index c6b874a7bf..98bf8fee32 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)\n>> strbuf_release(&env);\n>> }\n>>\n>> +static void canonicalize_config_variable_name(struct strbuf *var)\n>> +{\n>> + char *first_dot = strchr(var->buf, '.');\n>> + char *last_dot = strrchr(var->buf, '.');\n>\n> If first_dot != NULL, then last_dot !+ NULL as well.\n> (either both are NULL or none of them),\n> so we can loose one condition below.\n\nI do not think it is worth it, though.\n\n>> + char *cp;\n>> +\n>> + if (first_dot)\n>> + for (cp = var->buf; *cp && cp < first_dot; cp++)\n>> + *cp = tolower(*cp);\n>> + if (last_dot)\n>> + for (cp = last_dot; *cp; cp++)\n>> + *cp = tolower(*cp);\n\n\tif (first_dot) {\n\t\tscan up to first dot\n\t\tif (last_dot)\n\t\t\tscan from last dot to the end\n\t}\n\nwould be uglier.\n"},{"id":"311705","messageId":"CAGZ79kZgmtH1-1i5Fenq8kELbafBL1tCx66SGqGVBmjbpLoBGQ@mail.gmail.com","threadId":"45146","inReplyTo":"xmqqh93v10vy.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] submodule config does not apply to upper case submodules?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-15T23:43:16Z","receivedAt":"2017-02-15T23:43:24Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 15, 2017 at 3:37 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Yes; though I'd place it in strbuf.{c,h} as it is operating\n>> on the internals of the strbuf. (Do we make any promises outside of\n>> strbuf about the internals? I mean we use .buf all the time, so maybe\n>> I am overly cautious here)\n>\n> I'd rather have it not use struct strbuf as an interface. It only\n> needs to pass \"char *\" and its promise that it touches the string\n> in-place without changing the length need to be documented as a\n> comment before the function.\n>\n>>> config.c | 16 +++++++++++++++-\n>>> 1 file changed, 15 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/config.c b/config.c\n>>> index c6b874a7bf..98bf8fee32 100644\n>>> --- a/config.c\n>>> +++ b/config.c\n>>> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)\n>>> strbuf_release(&env);\n>>> }\n>>>\n>>> +static void canonicalize_config_variable_name(struct strbuf *var)\n>>> +{\n>>> + char *first_dot = strchr(var->buf, '.');\n>>> + char *last_dot = strrchr(var->buf, '.');\n>>\n>> If first_dot != NULL, then last_dot !+ NULL as well.\n>> (either both are NULL or none of them),\n>> so we can loose one condition below.\n>\n> I do not think it is worth it, though.\n>\n>>> + char *cp;\n>>> +\n>>> + if (first_dot)\n>>> + for (cp = var->buf; *cp && cp < first_dot; cp++)\n>>> + *cp = tolower(*cp);\n>>> + if (last_dot)\n>>> + for (cp = last_dot; *cp; cp++)\n>>> + *cp = tolower(*cp);\n>\n> if (first_dot) {\n> scan up to first dot\n> if (last_dot)\n\njust leave out the 'if (last_dot)' ?\n\n if (first_dot) {\n /* also implies last_dot */\n do 0 -> first\n do last -> end\n }\n"},{"id":"311706","messageId":"xmqqa89n10df.fsf_-_@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"xmqqtw7v123n.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] config: preserve case for one-shot config on the command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T23:48:44Z","receivedAt":"2017-02-15T23:49:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"git -c = cmd\" mechanism is to pretend that a\nconfiguration variable is set to while the cmd is\nrunning. The code to do so however downcased in its entirety,\nwhich is wrong for a three-level
...\n\nThe part needs to stay as-is.\n\nReported-by: Lars Schneider \nDiagnosed-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n config.c | 22 +++++++++++++++++++++-\n 1 file changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 0dfed682b8..e9b93b5304 100644\n--- a/config.c\n+++ b/config.c\n@@ -199,6 +199,26 @@ void git_config_push_parameter(const char *text)\n \tstrbuf_release(&env);\n }\n \n+/*\n+ * downcase the
and in
. or\n+ *
.. and do so in place. \n+ * is left intact.\n+ */\n+static void canonicalize_config_variable_name(char *varname)\n+{\n+\tchar *dot, *cp;\n+\n+\tdot = strchr(varname, '.');\n+\tif (dot)\n+\t\tfor (cp = varname; cp < dot; cp++)\n+\t\t\t*cp = tolower(*cp);\n+\tdot = strrchr(varname, '.');\n+\tif (dot)\n+\t\tfor (cp = dot + 1; *cp; cp++)\n+\t\t\t*cp = tolower(*cp);\n+}\n+\n+\n int git_config_parse_parameter(const char *text,\n \t\t\t config_fn_t fn, void *data)\n {\n@@ -221,7 +241,7 @@ int git_config_parse_parameter(const char *text,\n \t\tstrbuf_list_free(pair);\n \t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n-\tstrbuf_tolower(pair[0]);\n+\tcanonicalize_config_variable_name(pair[0]->buf);\n \tif (fn(pair[0]->buf, value, data) < 0) {\n \t\tstrbuf_list_free(pair);\n \t\treturn -1;\n-- \n2.12.0-rc1-258-g3d3d1e383b\n\n"},{"id":"311707","messageId":"CAPc5daVwYNvChw8_q7m=NS8aM5QKRMvUKN6r8PMycLZKy=PLKg@mail.gmail.com","threadId":"45146","inReplyTo":"CAGZ79kZgmtH1-1i5Fenq8kELbafBL1tCx66SGqGVBmjbpLoBGQ@mail.gmail.com","subject":"Re: [BUG] submodule config does not apply to upper case submodules?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T23:53:31Z","receivedAt":"2017-02-15T23:53:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ahh, that would work, too.\n\nOn Wed, Feb 15, 2017 at 3:43 PM, Stefan Beller wrote:\n> On Wed, Feb 15, 2017 at 3:37 PM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>> Yes; though I'd place it in strbuf.{c,h} as it is operating\n>>> on the internals of the strbuf. (Do we make any promises outside of\n>>> strbuf about the internals? I mean we use .buf all the time, so maybe\n>>> I am overly cautious here)\n>>\n>> I'd rather have it not use struct strbuf as an interface. It only\n>> needs to pass \"char *\" and its promise that it touches the string\n>> in-place without changing the length need to be documented as a\n>> comment before the function.\n>>\n>>>> config.c | 16 +++++++++++++++-\n>>>> 1 file changed, 15 insertions(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/config.c b/config.c\n>>>> index c6b874a7bf..98bf8fee32 100644\n>>>> --- a/config.c\n>>>> +++ b/config.c\n>>>> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)\n>>>> strbuf_release(&env);\n>>>> }\n>>>>\n>>>> +static void canonicalize_config_variable_name(struct strbuf *var)\n>>>> +{\n>>>> + char *first_dot = strchr(var->buf, '.');\n>>>> + char *last_dot = strrchr(var->buf, '.');\n>>>\n>>> If first_dot != NULL, then last_dot !+ NULL as well.\n>>> (either both are NULL or none of them),\n>>> so we can loose one condition below.\n>>\n>> I do not think it is worth it, though.\n>>\n>>>> + char *cp;\n>>>> +\n>>>> + if (first_dot)\n>>>> + for (cp = var->buf; *cp && cp < first_dot; cp++)\n>>>> + *cp = tolower(*cp);\n>>>> + if (last_dot)\n>>>> + for (cp = last_dot; *cp; cp++)\n>>>> + *cp = tolower(*cp);\n>>\n>> if (first_dot) {\n>> scan up to first dot\n>> if (last_dot)\n>\n> just leave out the 'if (last_dot)' ?\n>\n> if (first_dot) {\n> /* also implies last_dot */\n> do 0 -> first\n> do last -> end\n> }\n"},{"id":"311733","messageId":"D0CDD1AC-05CA-47F3-8CB5-61EA1C6515A8@gmail.com","threadId":"45146","inReplyTo":"xmqqa89n10df.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] config: preserve case for one-shot config on the command line","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-02-16T10:30:28Z","receivedAt":"2017-02-16T10:30:36Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 16 Feb 2017, at 00:48, Junio C Hamano wrote:\n> \n> The \"git -c = cmd\" mechanism is to pretend that a\n\nThe problem is also present for gitconfig variables e.g.\ngit config --local submodule.UPPERSUB.update none\n\nBut your patch below fixes that, too!\n\n\n> configuration variable is set to while the cmd is\n> running. The code to do so however downcased in its entirety,\n> which is wrong for a three-level
...\n> \n> The part needs to stay as-is.\n> \n> Reported-by: Lars Schneider \n> Diagnosed-by: Jonathan Tan \n> Signed-off-by: Junio C Hamano \n> ---\n> config.c | 22 +++++++++++++++++++++-\n> 1 file changed, 21 insertions(+), 1 deletion(-)\n> \n> diff --git a/config.c b/config.c\n> index 0dfed682b8..e9b93b5304 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -199,6 +199,26 @@ void git_config_push_parameter(const char *text)\n> \tstrbuf_release(&env);\n> }\n> \n> +/*\n> + * downcase the
and in
. or\n> + *
.. and do so in place. \n> + * is left intact.\n> + */\n> +static void canonicalize_config_variable_name(char *varname)\n> +{\n> +\tchar *dot, *cp;\n> +\n> +\tdot = strchr(varname, '.');\nminor nit:\nI think it would improve readability if we call this \"first_dot\" ...\n\n\n> +\tif (dot)\n> +\t\tfor (cp = varname; cp < dot; cp++)\n> +\t\t\t*cp = tolower(*cp);\n> +\tdot = strrchr(varname, '.');\n... and this \"last_dot\"?\n\n\n> +\tif (dot)\n> +\t\tfor (cp = dot + 1; *cp; cp++)\n> +\t\t\t*cp = tolower(*cp);\n> +}\n> +\n> +\n> int git_config_parse_parameter(const char *text,\n> \t\t\t config_fn_t fn, void *data)\n> {\n> @@ -221,7 +241,7 @@ int git_config_parse_parameter(const char *text,\n> \t\tstrbuf_list_free(pair);\n> \t\treturn error(\"bogus config parameter: %s\", text);\n> \t}\n> -\tstrbuf_tolower(pair[0]);\n> +\tcanonicalize_config_variable_name(pair[0]->buf);\n> \tif (fn(pair[0]->buf, value, data) < 0) {\n> \t\tstrbuf_list_free(pair);\n> \t\treturn -1;\n> -- \n> 2.12.0-rc1-258-g3d3d1e383b\n> \n\n\nThe patch looks good to me and fixes the problem!\n\nShould we add a test case to this patch?\nIf not, do you want me to improve my test case patch [1] \nor do you want to ditch the test?\n\nThank you,\nLars\n\n\n[1] http://public-inbox.org/git/20170215111704.78320-1-larsxschneider@gmail.com/"},{"id":"311771","messageId":"xmqqlgt6ysum.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"D0CDD1AC-05CA-47F3-8CB5-61EA1C6515A8@gmail.com","subject":"Re: [PATCH] config: preserve case for one-shot config on the command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-16T16:59:29Z","receivedAt":"2017-02-16T16:59:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> On 16 Feb 2017, at 00:48, Junio C Hamano wrote:\n>> \n>> The \"git -c = cmd\" mechanism is to pretend that a\n>\n> The problem is also present for gitconfig variables e.g.\n> git config --local submodule.UPPERSUB.update none\n\nYuck.\n\n> But your patch below fixes that, too!\n\nHeh.\n\n> Should we add a test case to this patch?\n> If not, do you want me to improve my test case patch [1] \n> or do you want to ditch the test?\n\nI think a new test for submodule (although it already exists thanks\nt you) is a bit too roundabout way to demonstrate the breakage and\nprotect the fix.\n\nWe'd perhaps want some tests for \"git config\", probably.\n\nIn a repository with /etc/gitconfig and $HOME/ that are tightly\ncontrolled (I think our test framework already do so), running these\nwould demonstrate both breakages, I would think, but I am sure\npeople can come up with other forms that are a lot easier to read..\n\n $ git -c v.A.r=val -c v.a.r=ue config --get-all v.a.r\n $ git -c v.A.r=val -c v.a.r=ue config --get-all v.A.r\n $ git -c v.a.r=val -c v.A.r=ue config --get-all v.a.r\n $ git -c v.a.r=val -c v.A.r=ue config --get-all v.A.r\n\nThanks.\n"},{"id":"311793","messageId":"xmqqpoiix97g.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"c609866a-f1b9-6fe5-f97a-d2180c290983@google.com","subject":"Re: [BUG] submodule config does not apply to upper case submodules?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-16T18:49:07Z","receivedAt":"2017-02-16T18:49:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> On 02/15/2017 03:11 PM, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>> Perhaps something like this?\n>\n> This looks good. I was hoping to unify the processing logic between\n> this CLI parsing and the usual stream parsing, but this approach is\n> probably simpler.\n\nI looked at the stream parsing before I wrote the patch for the same\nreason, and I agree that the stream parsing of course does not get a\nsingle variable name at one place [*1*], so there is nothing that\ncan be shared.\n\n[Footnote]\n\n*1* Instead \"[
\"\"] = \" is split and\n each piece is assembled into section.subsection.var with its own\n downcasing.\n\n"},{"id":"311824","messageId":"20170216232200.ysnho6rzpsn65y6v@sigill.intra.peff.net","threadId":"45146","inReplyTo":"xmqqh93v10vy.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] submodule config does not apply to upper case submodules?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-16T23:22:00Z","receivedAt":"2017-02-16T23:22:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 15, 2017 at 03:37:37PM -0800, Junio C Hamano wrote:\n\n> Stefan Beller writes:\n> \n> > Yes; though I'd place it in strbuf.{c,h} as it is operating\n> > on the internals of the strbuf. (Do we make any promises outside of\n> > strbuf about the internals? I mean we use .buf all the time, so maybe\n> > I am overly cautious here)\n> \n> I'd rather have it not use struct strbuf as an interface. It only\n> needs to pass \"char *\" and its promise that it touches the string\n> in-place without changing the length need to be documented as a\n> comment before the function.\n\nThis code also uses the hacky strbuf_split() interface. It would be nice\nto one day move off of it (the only other strbuf-specific function used\nthere is strbuf_trim).\n\nOne _could_ actually parse the whole thing left-to-right (soaking up\nwhitespace and doing the canonicalizing) instead of dealing with a split\nfunction at all. But the canonicalize bit you added here would not be\nreusable then. And it's probably not worth holding up the bugfix here.\n\n> >> +static void canonicalize_config_variable_name(struct strbuf *var)\n> >> +{\n> >> + char *first_dot = strchr(var->buf, '.');\n> >> + char *last_dot = strrchr(var->buf, '.');\n> >\n> > If first_dot != NULL, then last_dot !+ NULL as well.\n> > (either both are NULL or none of them),\n> > so we can loose one condition below.\n> \n> I do not think it is worth it, though.\n\nIf you really want to be picky, you do not need to find the first dot\nat all. You can downcase everything until you see a dot, and then\nfind the last dot (if any) from there.\n\nI don't think it matters much in practice.\n\n-Peff\n"},{"id":"311826","messageId":"20170216232730.xsx3xks5ppjws5rg@sigill.intra.peff.net","threadId":"45146","inReplyTo":"D0CDD1AC-05CA-47F3-8CB5-61EA1C6515A8@gmail.com","subject":"Re: [PATCH] config: preserve case for one-shot config on the command line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-16T23:27:30Z","receivedAt":"2017-02-16T23:27:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 16, 2017 at 11:30:28AM +0100, Lars Schneider wrote:\n\n> \n> > On 16 Feb 2017, at 00:48, Junio C Hamano wrote:\n> > \n> > The \"git -c = cmd\" mechanism is to pretend that a\n> \n> The problem is also present for gitconfig variables e.g.\n> git config --local submodule.UPPERSUB.update none\n\nHrm, is it?\n\n $ git config --file foo submodule.UPPERSUB.update none\n $ cat foo\n [submodule \"UPPERSUB\"]\n\tupdate = none\n\nI could believe that some of the submodule code may try to pass it\nthrough \"-c\", though, so certain config ends up being missed.\n\nAFAICT, though, the writing code takes what you gave it verbatim. The\nreader is responsible for downcasing everything but the subsection\nbefore it hands it to a callback. Commands calling git-config for lookup\nshould generally ask for the canonical downcased name. There is some\ncode to downcase, but IIRC there are corner cases around some of the\nregex lookup functions.\n\n-Peff\n"},{"id":"311833","messageId":"xmqqwpcptxps.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"20170216232730.xsx3xks5ppjws5rg@sigill.intra.peff.net","subject":"Re: [PATCH] config: preserve case for one-shot config on the command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-17T01:25:35Z","receivedAt":"2017-02-17T01:25:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Feb 16, 2017 at 11:30:28AM +0100, Lars Schneider wrote:\n>\n>> \n>> > On 16 Feb 2017, at 00:48, Junio C Hamano wrote:\n>> > \n>> > The \"git -c = cmd\" mechanism is to pretend that a\n>> \n>> The problem is also present for gitconfig variables e.g.\n>> git config --local submodule.UPPERSUB.update none\n>\n> Hrm, is it?\n>\n> $ git config --file foo submodule.UPPERSUB.update none\n> $ cat foo\n> [submodule \"UPPERSUB\"]\n> \tupdate = none\n>\n> I could believe that some of the submodule code may try to pass it\n> through \"-c\", though, so certain config ends up being missed.\n\nYou are right. \n\nThe builtin/config.c::get_value() codepath, when it is not using the\nhacky regexp interface, uses config.c::git_config_parse_key(), and\nit does the right thing. git_config_parse_parameter(), which is the\nbroken one we found, is not used.\n\nWhen I did the patch in response to Jonathan's observation, I did\nbriefly wonder if it should be using git_config_parse_key() instead\nof doing its own thing, but I didn't follow it up fully before\ndeciding that it is the quickest to replace the tolower thing. If I\nat least tried to see if it is feasible, I would have noticed that\nthe query from the command line wouldn't share the same problem as\nLars reported.\n\nI still haven't queued any of the variants I posted (and I do not\nthink other people sent their own versions, either). I need to pick\none and queue, with a test or two. Perhaps after -rc2. \n\nOthers are welcome to work on it while I cut -rc2 tomorrow, so that\nby the time I see their patch all that is left for me to do is to\napply it ;-)\n"},{"id":"312133","messageId":"xmqqino5jia8.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"xmqqwpcptxps.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] config: preserve case for one-shot config on the command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-20T09:58:07Z","receivedAt":"2017-02-20T09:58:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I still haven't queued any of the variants I posted (and I do not\n> think other people sent their own versions, either). I need to pick\n> one and queue, with a test or two. Perhaps after -rc2. \n>\n> Others are welcome to work on it while I cut -rc2 tomorrow, so that\n> by the time I see their patch all that is left for me to do is to\n> apply it ;-)\n\nSince nothing seems to have happened in the meantime, here is what\nI'll queue so that we won't forget for now. Lars's tests based on\nhow the scripted \"git submodule\" uses \"git config\" may still be\nvalid, but it is somewhat a roundabout way to demonstrate the\nbreakage and not very effective way to protect the fix, so I added a\nnew test that directly tests \"git -c = \".\n\nI am not sure if this updated one is worth doing, or the previous\n\"strchr and strrchr\" I originally wrote was easier to understand.\n\nOne thing I noticed is that \"git config --get X\" will correctly\ndiagnose that a dot-less X is not a valid variable name, but we do\nnot seem to diagnose \"git -c X=V \" as invalid.\n\nPerhaps we should, but it is not the focus of this topic.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Wed, 15 Feb 2017 15:48:44 -0800\nSubject: [PATCH] config: preserve case for one-shot config on the\n command line\n\nThe \"git -c = cmd\" mechanism is to pretend that a\nconfiguration variable is set to while the cmd is\nrunning. The code to do so however downcased in its entirety,\nwhich is wrong for a three-level
...\n\nThe part needs to stay as-is.\n\nReported-by: Lars Schneider \nDiagnosed-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n config.c | 30 ++++++++++++++++++++++++++++-\n t/t1351-config-cmdline.sh | 48 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 77 insertions(+), 1 deletion(-)\n create mode 100755 t/t1351-config-cmdline.sh\n\ndiff --git a/config.c b/config.c\nindex 0dfed682b8..ba9a5911b0 100644\n--- a/config.c\n+++ b/config.c\n@@ -199,6 +199,34 @@ void git_config_push_parameter(const char *text)\n \tstrbuf_release(&env);\n }\n \n+/*\n+ * downcase the
and in
. or\n+ *
.. and do so in place. \n+ * is left intact.\n+ */\n+static void canonicalize_config_variable_name(char *varname)\n+{\n+\tchar *cp, *last_dot;\n+\n+\t/* downcase the first segment */\n+\tfor (cp = varname; *cp; cp++) {\n+\t\tif (*cp == '.')\n+\t\t\tbreak;\n+\t\t*cp = tolower(*cp);\n+\t}\n+\tif (!*cp)\n+\t\treturn;\n+\n+\t/* scan for the last dot */\n+\tfor (last_dot = cp; *cp; cp++)\n+\t\tif (*cp == '.')\n+\t\t\tlast_dot = cp;\n+\n+\t/* downcase the last segment */\n+\tfor (cp = last_dot; *cp; cp++)\n+\t\t*cp = tolower(*cp);\n+}\n+\n int git_config_parse_parameter(const char *text,\n \t\t\t config_fn_t fn, void *data)\n {\n@@ -221,7 +249,7 @@ int git_config_parse_parameter(const char *text,\n \t\tstrbuf_list_free(pair);\n \t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n-\tstrbuf_tolower(pair[0]);\n+\tcanonicalize_config_variable_name(pair[0]->buf);\n \tif (fn(pair[0]->buf, value, data) < 0) {\n \t\tstrbuf_list_free(pair);\n \t\treturn -1;\ndiff --git a/t/t1351-config-cmdline.sh b/t/t1351-config-cmdline.sh\nnew file mode 100755\nindex 0000000000..acb8dc3b15\n--- /dev/null\n+++ b/t/t1351-config-cmdline.sh\n@@ -0,0 +1,48 @@\n+#!/bin/sh\n+\n+test_description='git -c var=val'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'last one wins: two level vars' '\n+\techo VAL >expect &&\n+\n+\t# sec.var and sec.VAR are the same variable, as the first\n+\t# and the last level of a configuration variable name is\n+\t# case insensitive. Test both setting and querying.\n+\n+\tgit -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'last one wins: three level vars' '\n+\techo val >expect &&\n+\n+\t# v.a.r and v.A.r are not the same variable, as the middle\n+\t# level of a three-level configuration variable name is\n+\t# case sensitive. Test both setting and querying.\n+\n+\tgit -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&\n+\ttest_cmp expect actual &&\n+\n+\techo VAL >expect &&\n+\tgit -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.12.0-rc2-221-g8fa194a99f\n\n"},{"id":"312155","messageId":"CC2769D6-2F21-433B-A972-ED795F31A868@gmail.com","threadId":"45146","inReplyTo":"xmqqino5jia8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] config: preserve case for one-shot config on the command line","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-02-20T17:17:46Z","receivedAt":"2017-02-20T17:18:14Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 20 Feb 2017, at 10:58, Junio C Hamano wrote:\n> \n> Junio C Hamano writes:\n> \n>> I still haven't queued any of the variants I posted (and I do not\n>> think other people sent their own versions, either). I need to pick\n>> one and queue, with a test or two. Perhaps after -rc2. \n>> \n>> Others are welcome to work on it while I cut -rc2 tomorrow, so that\n>> by the time I see their patch all that is left for me to do is to\n>> apply it ;-)\n> \n> Since nothing seems to have happened in the meantime, here is what\n> I'll queue so that we won't forget for now. Lars's tests based on\n> how the scripted \"git submodule\" uses \"git config\" may still be\n> valid, but it is somewhat a roundabout way to demonstrate the\n> breakage and not very effective way to protect the fix, so I added a\n> new test that directly tests \"git -c = \".\n\nAgreed. Please ignore my tests.\nIf you want to you could queue this tiny cleanup, though:\nhttp://public-inbox.org/git/20170215113325.14393-1-larsxschneider@gmail.com/\n\n> ...\n> \n> +/*\n> + * downcase the
and in
. or\n> + *
.. and do so in place. \n> + * is left intact.\n> + */\n> +static void canonicalize_config_variable_name(char *varname)\n> +{\n> +\tchar *cp, *last_dot;\n\nWhat does cp stand for? \"char pointer\"?\n\n> +\n> +\t/* downcase the first segment */\n> +\tfor (cp = varname; *cp; cp++) {\n> +\t\tif (*cp == '.')\n> +\t\t\tbreak;\n> +\t\t*cp = tolower(*cp);\n> +\t}\n> +\tif (!*cp)\n> +\t\treturn;\n> +\n> +\t/* scan for the last dot */\n> +\tfor (last_dot = cp; *cp; cp++)\n> +\t\tif (*cp == '.')\n> +\t\t\tlast_dot = cp;\n> +\n> +\t/* downcase the last segment */\n> +\tfor (cp = last_dot; *cp; cp++)\n> +\t\t*cp = tolower(*cp);\n> +}\n> +\n> int git_config_parse_parameter(const char *text,\n> \t\t\t config_fn_t fn, void *data)\n> {\n> @@ -221,7 +249,7 @@ int git_config_parse_parameter(const char *text,\n> \t\tstrbuf_list_free(pair);\n> \t\treturn error(\"bogus config parameter: %s\", text);\n> \t}\n> -\tstrbuf_tolower(pair[0]);\n> +\tcanonicalize_config_variable_name(pair[0]->buf);\n> \tif (fn(pair[0]->buf, value, data) < 0) {\n> \t\tstrbuf_list_free(pair);\n> \t\treturn -1;\n> diff --git a/t/t1351-config-cmdline.sh b/t/t1351-config-cmdline.sh\n> new file mode 100755\n> index 0000000000..acb8dc3b15\n> --- /dev/null\n> +++ b/t/t1351-config-cmdline.sh\n> @@ -0,0 +1,48 @@\n> +#!/bin/sh\n> +\n> +test_description='git -c var=val'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'last one wins: two level vars' '\n> +\techo VAL >expect &&\n> +\n> +\t# sec.var and sec.VAR are the same variable, as the first\n> +\t# and the last level of a configuration variable name is\n> +\t# case insensitive. Test both setting and querying.\n> +\n> +\tgit -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tgit -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'last one wins: three level vars' '\n> +\techo val >expect &&\n> +\n> +\t# v.a.r and v.A.r are not the same variable, as the middle\n> +\t# level of a three-level configuration variable name is\n> +\t# case sensitive. Test both setting and querying.\n> +\n> +\tgit -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\techo VAL >expect &&\n> +\tgit -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_done\n> -- \n> 2.12.0-rc2-221-g8fa194a99f\n> \n\nLooks good to me!\n\nThank you,\nLars\n\n"},{"id":"312194","messageId":"20170221073813.w4zrkky2d4drnwbs@sigill.intra.peff.net","threadId":"45146","inReplyTo":"xmqqino5jia8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] config: preserve case for one-shot config on the command line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-21T07:38:13Z","receivedAt":"2017-02-21T07:38:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 20, 2017 at 01:58:07AM -0800, Junio C Hamano wrote:\n\n> Since nothing seems to have happened in the meantime, here is what\n> I'll queue so that we won't forget for now. Lars's tests based on\n> how the scripted \"git submodule\" uses \"git config\" may still be\n> valid, but it is somewhat a roundabout way to demonstrate the\n> breakage and not very effective way to protect the fix, so I added a\n> new test that directly tests \"git -c = \".\n\nYeah, I agree that is the best way to cover this code.\n\n> I am not sure if this updated one is worth doing, or the previous\n> \"strchr and strrchr\" I originally wrote was easier to understand.\n\nI think either is OK. I would actually have written it halfway in\nbetween, like:\n\n static void canonicalize_config_variable_name(char *varname)\n {\n const char *p;\n\n /* downcase the first segment */\n for (p = varname; *p; p++) {\n\t if (*p == '.')\n\t\t break;\n *p = tolower(*p);\n\t }\n\n /* locate end of middle segment, if there is one */\n p = strrchr(p, '.');\n if (!p)\n return; /* invalid single-level var, but let it pass */\n for (; *p; p++)\n *p = tolower(*p);\n }\n\nYou can toss that on the bikeshed heap. :)\n\nThe other change from yours is that it flips the order of the strrchr\nand return. Yours is more efficient in the sense that we know there is\nno dot, so we do not have to keep searching at all (though of course\nthis case is invalid and we would not expect to see it in practice).\n\nBut it did take me a minute in yours to figure out why last_dot was\nalways set to a dot (the answer is that it starts at \"cp\", which must\nitself be a dot, because we would already have returned otherwise).\n\n> One thing I noticed is that \"git config --get X\" will correctly\n> diagnose that a dot-less X is not a valid variable name, but we do\n> not seem to diagnose \"git -c X=V \" as invalid.\n\nI don't think that was intentional. We just never caught the case. It\nmight be reasonable to do so (and it's easy to catch here while\ncanonicalizing). I think there are probably some other cases we _could_\ncatch, too (e.g., invalid characters for keynames). But I'm not excited\nabout duplicating the logic from the file-parser.\n\n> Perhaps we should, but it is not the focus of this topic.\n\nDefinitely.\n\n> diff --git a/t/t1351-config-cmdline.sh b/t/t1351-config-cmdline.sh\n> new file mode 100755\n> index 0000000000..acb8dc3b15\n> --- /dev/null\n> +++ b/t/t1351-config-cmdline.sh\n> @@ -0,0 +1,48 @@\n> +#!/bin/sh\n> +\n> +test_description='git -c var=val'\n> +\n> +. ./test-lib.sh\n\nThere are a bunch of other \"git -c\" tests inside t1300. I don't know if\nit would be better to put them all together.\n\nArguably, those other ones should get pulled out here to the new script.\nMore scripts means that the tests have fewer hidden dependencies, and we\ncan parallelize the run more. I admit I've shied away from new scripts\nin the past because the number allocation is such a pain.\n\n> +test_expect_success 'last one wins: two level vars' '\n> +\techo VAL >expect &&\n> +\n> +\t# sec.var and sec.VAR are the same variable, as the first\n> +\t# and the last level of a configuration variable name is\n> +\t# case insensitive. Test both setting and querying.\n\nI assume by \"setting and querying\" here you mean \"setting via -c,\nquerying via git-config\".\n\n> +\tgit -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tgit -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&\n> +\ttest_cmp expect actual\n\nLooks good.\n\n> +test_expect_success 'last one wins: three level vars' '\n> +\techo val >expect &&\n> +\n> +\t# v.a.r and v.A.r are not the same variable, as the middle\n> +\t# level of a three-level configuration variable name is\n> +\t# case sensitive. Test both setting and querying.\n> +\n> +\tgit -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\techo VAL >expect &&\n> +\tgit -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThere are two blocks of tests, each with their own \"expect\" value.\nShould the top \"expect\" here be moved down with its block to make that\nmore clear?\n\nOther than that nit, the tests look good to me.\n\n-Peff\n"},{"id":"312206","messageId":"xmqqino3h3zv.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"20170221073813.w4zrkky2d4drnwbs@sigill.intra.peff.net","subject":"Re: [PATCH] config: preserve case for one-shot config on the command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-21T17:01:56Z","receivedAt":"2017-02-21T17:02:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +. ./test-lib.sh\n>\n> There are a bunch of other \"git -c\" tests inside t1300. I don't know if\n> it would be better to put them all together.\n\nI considered it, but it is already very long and I suspect it would\nbe better in the longer term to split the command-line one out into\na separate script, for the reasons you state.\n\nI can move these at the end of 1300 in the meantime, and leave the\nsplit for somebody else to be done later.\n\n> Arguably, those other ones should get pulled out here to the new script.\n> More scripts means that the tests have fewer hidden dependencies, and we\n> can parallelize the run more. I admit I've shied away from new scripts\n> in the past because the number allocation is such a pain.\n>\n>> +test_expect_success 'last one wins: two level vars' '\n>> +\techo VAL >expect &&\n>> +\n>> +\t# sec.var and sec.VAR are the same variable, as the first\n>> +\t# and the last level of a configuration variable name is\n>> +\t# case insensitive. Test both setting and querying.\n>\n> I assume by \"setting and querying\" here you mean \"setting via -c,\n> querying via git-config\".\n\nYes. Should I update it to be more explicit?\n\n> There are two blocks of tests, each with their own \"expect\" value.\n> Should the top \"expect\" here be moved down with its block to make that\n> more clear?\n\nPerhaps. Let me try that one.\n\nThanks.\n\n"},{"id":"312207","messageId":"xmqqd1ebh3ak.fsf_-_@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"xmqqino3h3zv.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] config: preserve case for one-shot config on the command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-21T17:17:07Z","receivedAt":"2017-02-21T17:17:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"git -c = cmd\" mechanism is to pretend that a\nconfiguration variable is set to while the cmd is\nrunning. The code to do so however downcased in its entirety,\nwhich is wrong for a three-level
...\n\nThe part needs to stay as-is.\n\nReported-by: Lars Schneider \nDiagnosed-by: Jonathan Tan \nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n\n * Changes from v1:\n\n - update the comment before the second loop to find the last\n dot.\n\n - move two new tests into existing t1300 (at least for now).\n\n - make it clear that the second of the new tests check two\n aspects of \"three level vars\" by setting up the expectation for\n the first half near the actual tests and adding comments on\n what it tests before the second half.\n\n config.c | 30 +++++++++++++++++++++++++++++-\n t/t1300-repo-config.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 75 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 0dfed682b8..4128debc71 100644\n--- a/config.c\n+++ b/config.c\n@@ -199,6 +199,34 @@ void git_config_push_parameter(const char *text)\n \tstrbuf_release(&env);\n }\n \n+/*\n+ * downcase the
and in
. or\n+ *
.. and do so in place. \n+ * is left intact.\n+ */\n+static void canonicalize_config_variable_name(char *varname)\n+{\n+\tchar *cp, *last_dot;\n+\n+\t/* downcase the first segment */\n+\tfor (cp = varname; *cp; cp++) {\n+\t\tif (*cp == '.')\n+\t\t\tbreak;\n+\t\t*cp = tolower(*cp);\n+\t}\n+\tif (!*cp)\n+\t\treturn;\n+\n+\t/* find the last dot (we start from the first dot we just found) */\n+\tfor (last_dot = cp; *cp; cp++)\n+\t\tif (*cp == '.')\n+\t\t\tlast_dot = cp;\n+\n+\t/* downcase the last segment */\n+\tfor (cp = last_dot; *cp; cp++)\n+\t\t*cp = tolower(*cp);\n+}\n+\n int git_config_parse_parameter(const char *text,\n \t\t\t config_fn_t fn, void *data)\n {\n@@ -221,7 +249,7 @@ int git_config_parse_parameter(const char *text,\n \t\tstrbuf_list_free(pair);\n \t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n-\tstrbuf_tolower(pair[0]);\n+\tcanonicalize_config_variable_name(pair[0]->buf);\n \tif (fn(pair[0]->buf, value, data) < 0) {\n \t\tstrbuf_list_free(pair);\n \t\treturn -1;\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 923bfc5a26..7a16f66a9d 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1097,6 +1097,52 @@ test_expect_success 'multiple git -c appends config' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'last one wins: two level vars' '\n+\n+\t# sec.var and sec.VAR are the same variable, as the first\n+\t# and the last level of a configuration variable name is\n+\t# case insensitive.\n+\n+\techo VAL >expect &&\n+\n+\tgit -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'last one wins: three level vars' '\n+\n+\t# v.a.r and v.A.r are not the same variable, as the middle\n+\t# level of a three-level configuration variable name is\n+\t# case sensitive.\n+\n+\techo val >expect &&\n+\tgit -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# v.a.r and V.a.R are the same variable, as the first\n+\t# and the last level of a configuration variable name is\n+\t# case insensitive.\n+\n+\techo VAL >expect &&\n+\tgit -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git -c is not confused by empty environment' '\n \tGIT_CONFIG_PARAMETERS=\"\" git -c x.one=1 config --list\n '\n-- \n2.12.0-rc2-221-ga92f6f5816\n\n"},{"id":"312210","messageId":"20170221175021.znvpfvnlfjh4jsrf@sigill.intra.peff.net","threadId":"45146","inReplyTo":"xmqqd1ebh3ak.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] config: preserve case for one-shot config on the command line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-21T17:50:21Z","receivedAt":"2017-02-21T17:50:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 21, 2017 at 09:17:07AM -0800, Junio C Hamano wrote:\n\n> * Changes from v1:\n> \n> - update the comment before the second loop to find the last\n> dot.\n> \n> - move two new tests into existing t1300 (at least for now).\n> \n> - make it clear that the second of the new tests check two\n> aspects of \"three level vars\" by setting up the expectation for\n> the first half near the actual tests and adding comments on\n> what it tests before the second half.\n\nThanks, this addresses all of my (admittedly minor) concerns.\n\n-Peff\n"},{"id":"312211","messageId":"CAGZ79kb-fbm9N9ocVpW43nd_S9JMvcBiU8eiA7AoPVg6W1Sodw@mail.gmail.com","threadId":"45146","inReplyTo":"20170221175021.znvpfvnlfjh4jsrf@sigill.intra.peff.net","subject":"Re: [PATCH v2] config: preserve case for one-shot config on the command line","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-21T17:57:32Z","receivedAt":"2017-02-21T17:57:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Feb 21, 2017 at 9:50 AM, Jeff King wrote:\n> On Tue, Feb 21, 2017 at 09:17:07AM -0800, Junio C Hamano wrote:\n>\n>> * Changes from v1:\n>>\n>> - update the comment before the second loop to find the last\n>> dot.\n>>\n>> - move two new tests into existing t1300 (at least for now).\n>>\n>> - make it clear that the second of the new tests check two\n>> aspects of \"three level vars\" by setting up the expectation for\n>> the first half near the actual tests and adding comments on\n>> what it tests before the second half.\n>\n> Thanks, this addresses all of my (admittedly minor) concerns.\n>\n> -Peff\n\nThis patch looks different than what I last looked at. I like it.\nThough the manual search of the last dot instead of using\nstrrchr seems to be over-optimizing to me.\nAnyway it is still very understandable.\n\nThanks,\nStefan\n"},{"id":"312213","messageId":"xmqq37f7gyuj.fsf_-_@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"xmqqino5jia8.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-21T18:53:08Z","receivedAt":"2017-02-21T18:56:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The parsing of one-shot assignments of configuration variables that\ncome from the command line historically was quite loose and allowed\nanything to pass.\n\nThe configuration variable names that come from files are validated\nin git_config_parse_source(), which uses get_base_var() that grabs\nthe
(and subsection) while making sure that
\nconsists of iskeychar() letters, the function itself that makes sure\nthat the first letter in is isalpha(), and get_value()\nthat grabs the remainder of the name while making sure\nthat it consists of iskeychar() letters.\n\nPerform an equivalent check in canonicalize_config_variable_name()\nto catch invalid configuration variable names that come from the\ncommand line.\n\nSigned-off-by: Junio C Hamano \n---\n\n Junio C Hamano writes:\n\n > One thing I noticed is that \"git config --get X\" will correctly\n > diagnose that a dot-less X is not a valid variable name, but we do\n > not seem to diagnose \"git -c X=V \" as invalid.\n >\n > Perhaps we should, but it is not the focus of this topic.\n\n And here is a follow-up on that tangent.\n\n config.c | 48 +++++++++++++++++++++++++++++++++++++-----------\n t/t1300-repo-config.sh | 7 +++++++\n 2 files changed, 44 insertions(+), 11 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 4128debc71..e7f7ff1938 100644\n--- a/config.c\n+++ b/config.c\n@@ -199,32 +199,62 @@ void git_config_push_parameter(const char *text)\n \tstrbuf_release(&env);\n }\n \n+static inline int iskeychar(int c)\n+{\n+\treturn isalnum(c) || c == '-';\n+}\n+\n /*\n * downcase the
and in
. or\n *
.. and do so in place. \n * is left intact.\n+ *\n+ * The configuration variable names that come from files are validated\n+ * in git_config_parse_source(), which uses get_base_var() that grabs\n+ * the
(and subsection) while making sure that
\n+ * consists of iskeychar() letters, the function itself that makes\n+ * sure that the first letter in is isalpha(), and\n+ * get_value() that grabs the remainder of the name while\n+ * making sure that it consists of iskeychar() letters. Perform a\n+ * matching validation for configuration variables that come from\n+ * the command line.\n */\n-static void canonicalize_config_variable_name(char *varname)\n+static int canonicalize_config_variable_name(char *varname)\n {\n-\tchar *cp, *last_dot;\n+\tchar *cp, *first_dot, *last_dot;\n \n \t/* downcase the first segment */\n \tfor (cp = varname; *cp; cp++) {\n \t\tif (*cp == '.')\n \t\t\tbreak;\n+\t\tif (!iskeychar(*cp))\n+\t\t\treturn -1;\n \t\t*cp = tolower(*cp);\n \t}\n \tif (!*cp)\n-\t\treturn;\n+\t\treturn -1; /* no dot anywhere? */\n+\n+\tfirst_dot = cp;\n+\tif (first_dot == varname)\n+\t\treturn -1; /* no section? */\n \n \t/* find the last dot (we start from the first dot we just found) */\n-\tfor (last_dot = cp; *cp; cp++)\n+\tfor (; *cp; cp++)\n \t\tif (*cp == '.')\n \t\t\tlast_dot = cp;\n \n+\tif (!last_dot[1])\n+\t\treturn -1; /* no variable? */\n+\n \t/* downcase the last segment */\n-\tfor (cp = last_dot; *cp; cp++)\n+\tfor (cp = last_dot + 1; *cp; cp++) {\n+\t\tif (cp == last_dot + 1 && !isalpha(*cp))\n+\t\t\treturn -1;\n+\t\telse if (!iskeychar(*cp))\n+\t\t\treturn -1;\n \t\t*cp = tolower(*cp);\n+\t}\n+\treturn 0;\n }\n \n int git_config_parse_parameter(const char *text,\n@@ -249,7 +279,8 @@ int git_config_parse_parameter(const char *text,\n \t\tstrbuf_list_free(pair);\n \t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n-\tcanonicalize_config_variable_name(pair[0]->buf);\n+\tif (canonicalize_config_variable_name(pair[0]->buf))\n+\t\treturn error(\"bogus config parameter: %s\", text);\n \tif (fn(pair[0]->buf, value, data) < 0) {\n \t\tstrbuf_list_free(pair);\n \t\treturn -1;\n@@ -382,11 +413,6 @@ static char *parse_value(void)\n \t}\n }\n \n-static inline int iskeychar(int c)\n-{\n-\treturn isalnum(c) || c == '-';\n-}\n-\n static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n {\n \tint c;\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 7a16f66a9d..92a5d0dfb1 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1143,6 +1143,13 @@ test_expect_success 'last one wins: three level vars' '\n \ttest_cmp expect actual\n '\n \n+for VAR in a .a a. a.0b a.\"b c\". a.\"b c\".0d\n+do\n+\ttest_expect_success \"git -c $VAR=VAL rejects invalid '$VAR'\" '\n+\t\ttest_must_fail git -c $VAR=VAL config -l\n+\t'\n+done\n+\n test_expect_success 'git -c is not confused by empty environment' '\n \tGIT_CONFIG_PARAMETERS=\"\" git -c x.one=1 config --list\n '\n-- \n2.12.0-rc2-221-ga92f6f5816\n\n"},{"id":"312215","messageId":"CAGZ79kbR2QQyYO1dnQ0jW3-ztKEFj1MtJfDqEc0xoftMFeN=WA@mail.gmail.com","threadId":"45146","inReplyTo":"xmqq37f7gyuj.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-21T19:15:52Z","receivedAt":"2017-02-21T19:16:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Feb 21, 2017 at 10:53 AM, Junio C Hamano wrote:\n> The parsing of one-shot assignments of configuration variables that\n> come from the command line historically was quite loose and allowed\n> anything to pass.\n>\n> The configuration variable names that come from files are validated\n> in git_config_parse_source(), which uses get_base_var() that grabs\n> the
(and subsection) while making sure that
\n> consists of iskeychar() letters, the function itself that makes sure\n> that the first letter in is isalpha(), and get_value()\n> that grabs the remainder of the name while making sure\n> that it consists of iskeychar() letters.\n>\n> Perform an equivalent check in canonicalize_config_variable_name()\n> to catch invalid configuration variable names that come from the\n> command line.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> Junio C Hamano writes:\n>\n> > One thing I noticed is that \"git config --get X\" will correctly\n> > diagnose that a dot-less X is not a valid variable name, but we do\n> > not seem to diagnose \"git -c X=V \" as invalid.\n> >\n> > Perhaps we should, but it is not the focus of this topic.\n>\n> And here is a follow-up on that tangent.\n\nCombining this thought with another email[1] that flew by,\ndo we also need to have this treatment for \"git clone -c\"\n[1] http://public-inbox.org/git/EC270E42-9431-446C-96F9-E1A0C3E45333@gmail.com/\n\n>\n> +for VAR in a .a a. a.0b a.\"b c\". a.\"b c\".0d\n> +do\n> + test_expect_success \"git -c $VAR=VAL rejects invalid '$VAR'\" '\n> + test_must_fail git -c $VAR=VAL config -l\n> + '\n> +done\n> +\n> test_expect_success 'git -c is not confused by empty environment' '\n> GIT_CONFIG_PARAMETERS=\"\" git -c x.one=1 config --list\n\nDo we also want to test obscure cases of expected success?\ne.g. I suspect we never use a.\"b c\".d in the test suite elsewhere but it\nwould be a valid key to be handed to git?\n"},{"id":"312220","messageId":"xmqqlgszffm0.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"CAGZ79kbR2QQyYO1dnQ0jW3-ztKEFj1MtJfDqEc0xoftMFeN=WA@mail.gmail.com","subject":"Re: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-21T20:33:59Z","receivedAt":"2017-02-21T20:34:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Combining this thought with another email[1] that flew by,\n> do we also need to have this treatment for \"git clone -c\"\n\nYou tell me ;-) \n\nDo we share the same parser? If not, should we make them share the\nsame code?\n\n>> +for VAR in a .a a. a.0b a.\"b c\". a.\"b c\".0d\n>> +do\n>> + test_expect_success \"git -c $VAR=VAL rejects invalid '$VAR'\" '\n>> + test_must_fail git -c $VAR=VAL config -l\n>> + '\n>> +done\n>> +\n>> test_expect_success 'git -c is not confused by empty environment' '\n>> GIT_CONFIG_PARAMETERS=\"\" git -c x.one=1 config --list\n>\n> Do we also want to test obscure cases of expected success?\n> e.g. I suspect we never use a.\"b c\".d in the test suite elsewhere but it\n> would be a valid key to be handed to git?\n\nI wasn't aiming for anything obscure (and a.\"b c\".d is not at all\nobscure); as the new tests like \"git -c V.a.R config --get V.A.R\"\nadded in the previous step makes sure that the second level is not\nmolested and passed as is, so it is less urgent to see what can and\ncannot come at the second level.\n\nI didn't check if the existing coverage was sufficient, but we\ncertainly should test that three-level names with non-alpha and\nnon-keychar letters in the second are allowed in the overall \"git\nconfig\" test, not limited to the case where the configuration comes\non a one-shot command line but from files. I tend to think that is\na separate issue, though.\n"},{"id":"312227","messageId":"xmqqd1ebfd9l.fsf_-_@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"xmqqlgszffm0.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-21T21:24:38Z","receivedAt":"2017-02-21T21:25:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The parsing of one-shot assignments of configuration variables that\ncome from the command line historically was quite loose and allowed\nanything to pass.\n\nThe configuration variable names that come from files are validated\nin git_config_parse_source(), which uses get_base_var() that grabs\nthe
(and subsection) while making sure that
\nconsists of iskeychar() letters, the function itself that makes sure\nthat the first letter in is isalpha(), and get_value()\nthat grabs the remainder of the name while making sure\nthat it consists of iskeychar() letters.\n\nPerform an equivalent check in canonicalize_config_variable_name()\nto catch invalid configuration variable names that come from the\ncommand line.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Now with an updated test; while writing it it uncovered a bug in\n the original test that expected to fail---they failed alright but\n sometimes failed for a wrong reason.\n\n config.c | 48 +++++++++++++++++++++++++++++++++++++-----------\n t/t1300-repo-config.sh | 16 ++++++++++++++++\n 2 files changed, 53 insertions(+), 11 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 4128debc71..e7f7ff1938 100644\n--- a/config.c\n+++ b/config.c\n@@ -199,32 +199,62 @@ void git_config_push_parameter(const char *text)\n \tstrbuf_release(&env);\n }\n \n+static inline int iskeychar(int c)\n+{\n+\treturn isalnum(c) || c == '-';\n+}\n+\n /*\n * downcase the
and in
. or\n *
.. and do so in place. \n * is left intact.\n+ *\n+ * The configuration variable names that come from files are validated\n+ * in git_config_parse_source(), which uses get_base_var() that grabs\n+ * the
(and subsection) while making sure that
\n+ * consists of iskeychar() letters, the function itself that makes\n+ * sure that the first letter in is isalpha(), and\n+ * get_value() that grabs the remainder of the name while\n+ * making sure that it consists of iskeychar() letters. Perform a\n+ * matching validation for configuration variables that come from\n+ * the command line.\n */\n-static void canonicalize_config_variable_name(char *varname)\n+static int canonicalize_config_variable_name(char *varname)\n {\n-\tchar *cp, *last_dot;\n+\tchar *cp, *first_dot, *last_dot;\n \n \t/* downcase the first segment */\n \tfor (cp = varname; *cp; cp++) {\n \t\tif (*cp == '.')\n \t\t\tbreak;\n+\t\tif (!iskeychar(*cp))\n+\t\t\treturn -1;\n \t\t*cp = tolower(*cp);\n \t}\n \tif (!*cp)\n-\t\treturn;\n+\t\treturn -1; /* no dot anywhere? */\n+\n+\tfirst_dot = cp;\n+\tif (first_dot == varname)\n+\t\treturn -1; /* no section? */\n \n \t/* find the last dot (we start from the first dot we just found) */\n-\tfor (last_dot = cp; *cp; cp++)\n+\tfor (; *cp; cp++)\n \t\tif (*cp == '.')\n \t\t\tlast_dot = cp;\n \n+\tif (!last_dot[1])\n+\t\treturn -1; /* no variable? */\n+\n \t/* downcase the last segment */\n-\tfor (cp = last_dot; *cp; cp++)\n+\tfor (cp = last_dot + 1; *cp; cp++) {\n+\t\tif (cp == last_dot + 1 && !isalpha(*cp))\n+\t\t\treturn -1;\n+\t\telse if (!iskeychar(*cp))\n+\t\t\treturn -1;\n \t\t*cp = tolower(*cp);\n+\t}\n+\treturn 0;\n }\n \n int git_config_parse_parameter(const char *text,\n@@ -249,7 +279,8 @@ int git_config_parse_parameter(const char *text,\n \t\tstrbuf_list_free(pair);\n \t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n-\tcanonicalize_config_variable_name(pair[0]->buf);\n+\tif (canonicalize_config_variable_name(pair[0]->buf))\n+\t\treturn error(\"bogus config parameter: %s\", text);\n \tif (fn(pair[0]->buf, value, data) < 0) {\n \t\tstrbuf_list_free(pair);\n \t\treturn -1;\n@@ -382,11 +413,6 @@ static char *parse_value(void)\n \t}\n }\n \n-static inline int iskeychar(int c)\n-{\n-\treturn isalnum(c) || c == '-';\n-}\n-\n static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n {\n \tint c;\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 7a16f66a9d..ea371020fa 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1143,6 +1143,22 @@ test_expect_success 'last one wins: three level vars' '\n \ttest_cmp expect actual\n '\n \n+for VAR in a .a a. a.0b a.\"b c\". a.\"b c\".0d\n+do\n+\ttest_expect_success \"git -c $VAR=VAL rejects invalid '$VAR'\" '\n+\t\ttest_must_fail git -c \"$VAR=VAL\" config -l\n+\t'\n+done\n+\n+for VAR in a.b a.\"b c\".d\n+do\n+\ttest_expect_success \"git -c $VAR=VAL works with valid '$VAR'\" '\n+\t\techo VAL >expect &&\n+\t\tgit -c \"$VAR=VAL\" config --get \"$VAR\" >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n test_expect_success 'git -c is not confused by empty environment' '\n \tGIT_CONFIG_PARAMETERS=\"\" git -c x.one=1 config --list\n '\n-- \n2.12.0-rc2-222-gff02733afe\n\n"},{"id":"312268","messageId":"xmqqfuj7dofx.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"xmqqd1ebfd9l.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-22T01:06:10Z","receivedAt":"2017-02-22T01:06:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \t/* find the last dot (we start from the first dot we just found) */\n> -\tfor (last_dot = cp; *cp; cp++)\n> +\tfor (; *cp; cp++)\n> \t\tif (*cp == '.')\n> \t\t\tlast_dot = cp;\n\nThis line probably needs this fix-up on top.\n\n-- >8 --\nSubject: [PATCH] config: squelch stupid compiler warnings\n\nSome compilers do not realize that *cp is always '.' when the loop\nto find the last dot begins, and instead gives a useless warning\nthat says last_dot may be uninitialized.\n\nSquelch it by being a bit more explicit if stupid.\n\nSigned-off-by: Junio C Hamano \n---\n config.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex e7f7ff1938..90de27853f 100644\n--- a/config.c\n+++ b/config.c\n@@ -239,7 +239,7 @@ static int canonicalize_config_variable_name(char *varname)\n \t\treturn -1; /* no section? */\n \n \t/* find the last dot (we start from the first dot we just found) */\n-\tfor (; *cp; cp++)\n+\tfor (last_dot = cp; *cp; cp++)\n \t\tif (*cp == '.')\n \t\t\tlast_dot = cp;\n \n-- \n2.12.0-rc2-231-g83a1c8597c\n\n"},{"id":"312373","messageId":"20170223055831.u3yofkby3c56t7l4@sigill.intra.peff.net","threadId":"45146","inReplyTo":"xmqqd1ebfd9l.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-23T05:58:31Z","receivedAt":"2017-02-23T06:59:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 21, 2017 at 01:24:38PM -0800, Junio C Hamano wrote:\n\n> The parsing of one-shot assignments of configuration variables that\n> come from the command line historically was quite loose and allowed\n> anything to pass.\n> \n> The configuration variable names that come from files are validated\n> in git_config_parse_source(), which uses get_base_var() that grabs\n> the
(and subsection) while making sure that
\n> consists of iskeychar() letters, the function itself that makes sure\n> that the first letter in is isalpha(), and get_value()\n> that grabs the remainder of the name while making sure\n> that it consists of iskeychar() letters.\n> \n> Perform an equivalent check in canonicalize_config_variable_name()\n> to catch invalid configuration variable names that come from the\n> command line.\n\nFWIW, the code looks OK here. It is a shame to duplicate the policy\nfound in git_config_parse_key(), though.\n\nI wonder if we could make a master version of that which canonicalizes\nin-place, and then just wrap it for the git_config_parse_key()\ninterface. Actually, I guess the function you just wrote _is_ that inner\nfunction, as long as it learned about the \"quiet\" flag.\n\n-Peff\n"},{"id":"312375","messageId":"xmqqo9xt8jcf.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"20170223055831.u3yofkby3c56t7l4@sigill.intra.peff.net","subject":"Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-23T07:19:44Z","receivedAt":"2017-02-23T07:19:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> FWIW, the code looks OK here. It is a shame to duplicate the policy\n> found in git_config_parse_key(), though.\n>\n> I wonder if we could make a master version of that which canonicalizes\n> in-place, and then just wrap it for the git_config_parse_key()\n> interface. Actually, I guess the function you just wrote _is_ that inner\n> function, as long as it learned about the \"quiet\" flag.\n\nHmm, I obviously missed an opportunity. I thought about doing a\nsimilar thing with the policy in parse-source, but that side didn't\nseem worth doing, as the config-parse-source callgraph is quite a\nmess (as it has to parse the .ini like format with line breaks and\ncomments, not the simple \"[.].\" thing, it\ncannot quite avoid it), and it needs to take advantage of _some_\npolicy to parse the pieces.\n\nWe could loosen the policy implemented by config-parse-key interface\n(e.g. change config-parse-source to let anything that begins with a\nnon-whitespace continue to be processed with get_value(), instead of\nonly allowing string that begins with isalpha(); similarly loosen\nget_value() to allow any non-dot non-space string, not just\niskeychar() bytes) and first turn what is read into the simple\n\"[.].\" format, and then reuse the new\n\"master\" logic to validate. That would allow us to update the\n\"master\" logic to make it tighter or looser to some degree, but the\nsource parser still needs to hardcode _some_ policy (e.g. the first\nlevel and the last level names begin with a non-space) that allows\nit to guess what \"\" pieces the contents being parsed from\nthe .ini looking format meant to express.\n\nBut you are right. config-parse-key does have the simpler string\nthat can just be given to the canonicalize thing and we should be\nable to reuse it.\n\n\n\n"},{"id":"312470","messageId":"xmqqy3ww5wbl.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"xmqqo9xt8jcf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-23T23:19:58Z","receivedAt":"2017-02-23T23:20:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> FWIW, the code looks OK here. It is a shame to duplicate the policy\n>> found in git_config_parse_key(), though.\n>>\n>> I wonder if we could make a master version of that which canonicalizes\n>> in-place, and then just wrap it for the git_config_parse_key()\n>> interface. Actually, I guess the function you just wrote _is_ that inner\n>> function, as long as it learned about the \"quiet\" flag.\n>\n> Hmm, I obviously missed an opportunity....\n> ...\n> But you are right. config-parse-key does have the simpler string\n> that can just be given to the canonicalize thing and we should be\n> able to reuse it.\n\nActually, I think we can just use the existing config_parse_key()\ninstead of adding the new function. It adds one allocation and\ndeallocation, but it's not like this is a performance-critical\ncodepath that we absolutely avoid extra allocations. After all, we\nare still using the strbuf-split thing :-/.\n\nThe attached patch shows the updated fix. It needs a preparatory\ncode move (not shown here) to make git_config_parse_key() available\nto git_config_parse_parameter(), though.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Thu, 23 Feb 2017 15:04:40 -0800\nSubject: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command' and\n keep subsection intact\n\nThe parsing of one-shot assignments of configuration variables that\ncome from the command line historically was quite loose and allowed\nanything to pass. It also downcased everything in the variable name,\neven a three-level
.. name in which\nthe part must be treated in a case sensible manner.\n\nExisting git_config_parse_key() helper is used to parse the variable\nname that comes from the command line, i.e. \"git config VAR VAL\",\nand handles these details correctly. Replace the strbuf_tolower()\ncall in git-config_parse_parameter() with a call to it to correct\nboth issues. git_config_parse_key() does a bit more things that are\nnot necessary for the purpose of this codepath (e.g. it allocates a\nseparate buffer to return the canonicalized variable name because it\ntakes a \"const char *\" input), but we are not in a performance-critical\ncodepath here.\n\nSigned-off-by: Junio C Hamano \n---\n config.c | 14 ++++++++----\n t/t1300-repo-config.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 71 insertions(+), 5 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex b8cce1dffa..39f20dcd2c 100644\n--- a/config.c\n+++ b/config.c\n@@ -295,7 +295,9 @@ int git_config_parse_parameter(const char *text,\n \t\t\t config_fn_t fn, void *data)\n {\n \tconst char *value;\n+\tchar *canonical_name;\n \tstruct strbuf **pair;\n+\tint ret = 0;\n \n \tpair = strbuf_split_str(text, '=', 2);\n \tif (!pair[0])\n@@ -313,13 +315,15 @@ int git_config_parse_parameter(const char *text,\n \t\tstrbuf_list_free(pair);\n \t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n-\tstrbuf_tolower(pair[0]);\n-\tif (fn(pair[0]->buf, value, data) < 0) {\n-\t\tstrbuf_list_free(pair);\n+\n+\tif (git_config_parse_key(pair[0]->buf, &canonical_name, NULL))\n \t\treturn -1;\n-\t}\n+\n+\tret = (fn(canonical_name, value, data) < 0) ? -1 : 0;\n+\n+\tfree(canonical_name);\n \tstrbuf_list_free(pair);\n-\treturn 0;\n+\treturn ret;\n }\n \n int git_config_from_parameters(config_fn_t fn, void *data)\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 923bfc5a26..ea371020fa 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1097,6 +1097,68 @@ test_expect_success 'multiple git -c appends config' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'last one wins: two level vars' '\n+\n+\t# sec.var and sec.VAR are the same variable, as the first\n+\t# and the last level of a configuration variable name is\n+\t# case insensitive.\n+\n+\techo VAL >expect &&\n+\n+\tgit -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'last one wins: three level vars' '\n+\n+\t# v.a.r and v.A.r are not the same variable, as the middle\n+\t# level of a three-level configuration variable name is\n+\t# case sensitive.\n+\n+\techo val >expect &&\n+\tgit -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# v.a.r and V.a.R are the same variable, as the first\n+\t# and the last level of a configuration variable name is\n+\t# case insensitive.\n+\n+\techo VAL >expect &&\n+\tgit -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+for VAR in a .a a. a.0b a.\"b c\". a.\"b c\".0d\n+do\n+\ttest_expect_success \"git -c $VAR=VAL rejects invalid '$VAR'\" '\n+\t\ttest_must_fail git -c \"$VAR=VAL\" config -l\n+\t'\n+done\n+\n+for VAR in a.b a.\"b c\".d\n+do\n+\ttest_expect_success \"git -c $VAR=VAL works with valid '$VAR'\" '\n+\t\techo VAL >expect &&\n+\t\tgit -c \"$VAR=VAL\" config --get \"$VAR\" >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n test_expect_success 'git -c is not confused by empty environment' '\n \tGIT_CONFIG_PARAMETERS=\"\" git -c x.one=1 config --list\n '\n-- \n2.12.0-rc2-289-g9f26f1516a\n\n\n\n\n"},{"id":"312492","messageId":"20170224004105.ayddcwlnpmq7tifu@sigill.intra.peff.net","threadId":"45146","inReplyTo":"xmqqy3ww5wbl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T00:41:05Z","receivedAt":"2017-02-24T00:48:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2017 at 03:19:58PM -0800, Junio C Hamano wrote:\n\n> > But you are right. config-parse-key does have the simpler string\n> > that can just be given to the canonicalize thing and we should be\n> > able to reuse it.\n> \n> Actually, I think we can just use the existing config_parse_key()\n> instead of adding the new function. It adds one allocation and\n> deallocation, but it's not like this is a performance-critical\n> codepath that we absolutely avoid extra allocations. After all, we\n> are still using the strbuf-split thing :-/.\n\nYeah, you're right. This is much nicer, and everything else was\npremature optimization.\n\n> -- >8 --\n> From: Junio C Hamano \n> Date: Thu, 23 Feb 2017 15:04:40 -0800\n> Subject: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command' and\n> keep subsection intact\n\nLong subject. :)\n\nI'd have just said:\n\n config: pass variables through git_config_parse_parameter()\n\nThat is \"what\", but the \"why\" can come in the next paragraph.\n\n> The parsing of one-shot assignments of configuration variables that\n> come from the command line historically was quite loose and allowed\n> anything to pass. It also downcased everything in the variable name,\n> even a three-level
.. name in which\n> the part must be treated in a case sensible manner.\n> \n> Existing git_config_parse_key() helper is used to parse the variable\n> name that comes from the command line, i.e. \"git config VAR VAL\",\n> and handles these details correctly. Replace the strbuf_tolower()\n> call in git-config_parse_parameter() with a call to it to correct\n> both issues. git_config_parse_key() does a bit more things that are\n> not necessary for the purpose of this codepath (e.g. it allocates a\n> separate buffer to return the canonicalized variable name because it\n> takes a \"const char *\" input), but we are not in a performance-critical\n> codepath here.\n\nNicely explained.\n\n> diff --git a/config.c b/config.c\n> index b8cce1dffa..39f20dcd2c 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -295,7 +295,9 @@ int git_config_parse_parameter(const char *text,\n> \t\t\t config_fn_t fn, void *data)\n> {\n> \tconst char *value;\n> +\tchar *canonical_name;\n> \tstruct strbuf **pair;\n> +\tint ret = 0;\n> \n> \tpair = strbuf_split_str(text, '=', 2);\n> \tif (!pair[0])\n\nHmm. I suspect one cannot do:\n\n git -c 'section.subsection with an = in it.key=foo' ...\n\nDefinitely not a new problem, nor something that should block your\npatch. But if we want to fix it, I suspect the problem will ultimately\ninvolve parsing left-to-right to get the key first, then confirming it\nhas an =, and then the value.\n\n> @@ -313,13 +315,15 @@ int git_config_parse_parameter(const char *text,\n> \t\tstrbuf_list_free(pair);\n> \t\treturn error(\"bogus config parameter: %s\", text);\n> \t}\n> -\tstrbuf_tolower(pair[0]);\n> -\tif (fn(pair[0]->buf, value, data) < 0) {\n> -\t\tstrbuf_list_free(pair);\n> +\n> +\tif (git_config_parse_key(pair[0]->buf, &canonical_name, NULL))\n> \t\treturn -1;\n> -\t}\n\nI think git_config_parse_key() will free canonical_name itself if it\nreturns failure. But do you need to strbuf_list_free(pair) here?\n\nOr alternatively:\n\n int ret = -1;\n if (!parse(...))\n ret = fn(...);\n\nor use a \"got out\". Whatever. You don't need me to teach you about error\nexits. :)\n\n> +\tret = (fn(canonical_name, value, data) < 0) ? -1 : 0;\n> +\n> +\tfree(canonical_name);\n> \tstrbuf_list_free(pair);\n> -\treturn 0;\n> +\treturn ret;\n\nLooks good.\n\n> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n> index 923bfc5a26..ea371020fa 100755\n> --- a/t/t1300-repo-config.sh\n> +++ b/t/t1300-repo-config.sh\n\nI just skimmed these, as they look like the previous ones.\n\nSo overall I like it, modulo the minor error-leak.\n\n-Peff\n"},{"id":"312499","messageId":"xmqqh93k5ijb.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"20170224004105.ayddcwlnpmq7tifu@sigill.intra.peff.net","subject":"Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-24T04:17:44Z","receivedAt":"2017-02-24T04:18:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> \tpair = strbuf_split_str(text, '=', 2);\n>> \tif (!pair[0])\n>\n> Hmm. I suspect one cannot do:\n>\n> git -c 'section.subsection with an = in it.key=foo' ...\n>\n> Definitely not a new problem, nor something that should block your\n> patch. But if we want to fix it, I suspect the problem will ultimately\n> involve parsing left-to-right to get the key first, then confirming it\n> has an =, and then the value.\n\nBacktracking will not fundamentally \"fix\" parsing of\n\n\ta.b=c=.d\n\nbetween twhse two\n\n\t[a \"b=\"] c = \".d\"\n\t[a] b = \"c=.d\"\n\nunfortunately, I think. I do not think it is worth doing the \"best\neffort\" with erroring out when ambiguous, because there is no way\nfor the end user to disambiguate, unless we introduce a different\nsyntax, at which point we cannot use config_parse_key() anymore.\n\n>> +\tif (git_config_parse_key(pair[0]->buf, &canonical_name, NULL))\n>> \t\treturn -1;\n>> -\t}\n>\n> I think git_config_parse_key() will free canonical_name itself if it\n> returns failure. But do you need to strbuf_list_free(pair) here?\n\nYeah, I missed that one. Thanks.\n"},{"id":"312501","messageId":"20170224042227.2rjgf4zbiadxbrtz@sigill.intra.peff.net","threadId":"45146","inReplyTo":"xmqqh93k5ijb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T04:22:27Z","receivedAt":"2017-02-24T04:22:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2017 at 08:17:44PM -0800, Junio C Hamano wrote:\n\n> > Hmm. I suspect one cannot do:\n> >\n> > git -c 'section.subsection with an = in it.key=foo' ...\n> >\n> > Definitely not a new problem, nor something that should block your\n> > patch. But if we want to fix it, I suspect the problem will ultimately\n> > involve parsing left-to-right to get the key first, then confirming it\n> > has an =, and then the value.\n> \n> Backtracking will not fundamentally \"fix\" parsing of\n> \n> \ta.b=c=.d\n> \n> between twhse two\n> \n> \t[a \"b=\"] c = \".d\"\n> \t[a] b = \"c=.d\"\n> \n> unfortunately, I think. I do not think it is worth doing the \"best\n> effort\" with erroring out when ambiguous, because there is no way\n> for the end user to disambiguate, unless we introduce a different\n> syntax, at which point we cannot use config_parse_key() anymore.\n\nAh, yeah, you're right. I thought the problem was just that the \"split\"\nwas too naive, but it really is that the whole syntax is badly\nspecified.\n\nI guess \"git config --list\" suffers from the same problem. You can get\naround it there with \"-z\", but that probably would not be very pleasant\nhere. :)\n\nProbably not worth worrying too much about if nobody is complaining.\n\n-Peff\n"},{"id":"312502","messageId":"xmqqd1e85ddy.fsf@gitster.mtv.corp.google.com","threadId":"45146","inReplyTo":"20170224042227.2rjgf4zbiadxbrtz@sigill.intra.peff.net","subject":"Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-24T06:08:57Z","receivedAt":"2017-02-24T06:09:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Backtracking will not fundamentally \"fix\" parsing of\n>> \n>> \ta.b=c=.d\n>> \n>> between twhse two\n>> \n>> \t[a \"b=\"] c = \".d\"\n>> \t[a] b = \"c=.d\"\n>> \n>> unfortunately, I think. I do not think it is worth doing the \"best\n>> effort\" with erroring out when ambiguous, because there is no way\n>> for the end user to disambiguate, unless we introduce a different\n>> syntax, at which point we cannot use config_parse_key() anymore.\n>\n> Ah, yeah, you're right. I thought the problem was just that the \"split\"\n> was too naive, but it really is that the whole syntax is badly\n> specified.\n>\n> I guess \"git config --list\" suffers from the same problem. You can get\n> around it there with \"-z\", but that probably would not be very pleasant\n> here. :)\n>\n> Probably not worth worrying too much about if nobody is complaining.\n\nYup.\n\nAnyway, here is an updated one (the part of the patch to t/ is not\nshown as it is unchanged).\n\n-- >8 --\nSubject: [PATCH] config: use git_config_parse_key() in git_config_parse_parameter()\n\nThe parsing of one-shot assignments of configuration variables that\ncome from the command line historically was quite loose and allowed\nanything to pass. It also downcased everything in the variable name,\neven a three-level
.. name in which\nthe part must be treated in a case sensitive manner.\n\nExisting git_config_parse_key() helper is used to parse the variable\nname that comes from the command line, i.e. \"git config VAR VAL\",\nand handles these details correctly. Replace the strbuf_tolower()\ncall in git_config_parse_parameter() with a call to it to correct\nboth issues. git_config_parse_key() does a bit more things that are\nnot necessary for the purpose of this codepath (e.g. it allocates a\nseparate buffer to return the canonicalized variable name because it\ntakes a \"const char *\" input), but we are not in a performance-critical\ncodepath here.\n\nSigned-off-by: Junio C Hamano \n---\n config.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex b8cce1dffa..1c1a1520ff 100644\n--- a/config.c\n+++ b/config.c\n@@ -295,7 +295,9 @@ int git_config_parse_parameter(const char *text,\n \t\t\t config_fn_t fn, void *data)\n {\n \tconst char *value;\n+\tchar *canonical_name;\n \tstruct strbuf **pair;\n+\tint ret;\n \n \tpair = strbuf_split_str(text, '=', 2);\n \tif (!pair[0])\n@@ -313,13 +315,15 @@ int git_config_parse_parameter(const char *text,\n \t\tstrbuf_list_free(pair);\n \t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n-\tstrbuf_tolower(pair[0]);\n-\tif (fn(pair[0]->buf, value, data) < 0) {\n-\t\tstrbuf_list_free(pair);\n-\t\treturn -1;\n+\n+\tif (git_config_parse_key(pair[0]->buf, &canonical_name, NULL)) {\n+\t\tret = -1;\n+\t} else {\n+\t\tret = (fn(canonical_name, value, data) < 0) ? -1 : 0;\n+\t\tfree(canonical_name);\n \t}\n \tstrbuf_list_free(pair);\n-\treturn 0;\n+\treturn ret;\n }\n \n int git_config_from_parameters(config_fn_t fn, void *data)\n-- \n2.12.0-rc2-308-gbf7e63c428\n\n"},{"id":"312503","messageId":"20170224061045.bejaswcrc4youwsv@sigill.intra.peff.net","threadId":"45146","inReplyTo":"xmqqd1e85ddy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T06:10:46Z","receivedAt":"2017-02-24T06:11:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2017 at 10:08:57PM -0800, Junio C Hamano wrote:\n\n> Anyway, here is an updated one (the part of the patch to t/ is not\n> shown as it is unchanged).\n> \n> -- >8 --\n> Subject: [PATCH] config: use git_config_parse_key() in git_config_parse_parameter()\n\nLooks good. Nice and simple.\n\n-Peff\n"}]}