{"thread":{"id":"48965","subject":"[PATCH] config: fix case sensitive subsection names on writing","startedAt":"2018-07-27T20:51:23Z","lastAt":"2018-08-03T15:51:05Z","messageCount":33,"participants":["Stefan Beller","Brandon Williams","Junio C Hamano","Jeff King","Johannes Schindelin","Eric Sunshine","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"353752","messageId":"20180727205117.243770-1-sbeller@google.com","threadId":"48965","inReplyTo":null,"subject":"[PATCH] config: fix case sensitive subsection names on writing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-27T20:51:17Z","receivedAt":"2018-07-27T20:51:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"A use reported a submodule issue regarding strange case indentation\nissues, but it could be boiled down to the following test case:\n\n $ git init test && cd test\n $ git config foo.\"Bar\".key test\n $ git config foo.\"bar\".key test\n $ tail -n 3 .git/config\n [foo \"Bar\"]\n key = test\n key = test\n\nSub sections are case sensitive and we have a test for correctly reading\nthem. However we do not have a test for writing out config correctly with\ncase sensitive subsection names, which is why this went unnoticed in\n6ae996f2acf (git_config_set: make use of the config parser's event\nstream, 2018-04-09)\n\nMake the subsection case sensitive and provide a test for both reading\nand writing.\n\nReported-by: JP Sugarbroad \nSigned-off-by: Stefan Beller \n---\n config.c | 2 +-\n t/t1300-config.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 3aacddfec4b..3ded92b678b 100644\n--- a/config.c\n+++ b/config.c\n@@ -2374,7 +2374,7 @@ static int store_aux_event(enum config_event_t type,\n \t\tstore->is_keys_section =\n \t\t\tstore->parsed[store->parsed_nr].is_keys_section =\n \t\t\tcf->var.len - 1 == store->baselen &&\n-\t\t\t!strncasecmp(cf->var.buf, store->key, store->baselen);\n+\t\t\t!strncmp(cf->var.buf, store->key, store->baselen);\n \t\tif (store->is_keys_section) {\n \t\t\tstore->section_seen = 1;\n \t\t\tALLOC_GROW(store->seen, store->seen_nr + 1,\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 03c223708eb..8325d4495f4 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1218,6 +1218,24 @@ test_expect_success 'last one wins: three level vars' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setting different case subsections ' '\n+\ttest_when_finished \"rm -f caseSens caseSens_actual caseSens_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.\n+\tgit config -f caseSens v.\"A\".r VAL &&\n+\tgit config -f caseSens v.\"a\".r val &&\n+\n+\techo VAL >caseSens_expect &&\n+\tgit config -f caseSens v.\"A\".r >caseSens_actual &&\n+\ttest_cmp caseSens_expect caseSens_actual &&\n+\n+\techo val >caseSens_expect &&\n+\tgit config -f caseSens v.\"a\".r >caseSens_actual &&\n+\ttest_cmp caseSens_expect caseSens_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-- \n2.18.0.345.g5c9ce644c3-goog\n\n"},{"id":"353755","messageId":"20180727212140.GA54208@google.com","threadId":"48965","inReplyTo":"20180727205117.243770-1-sbeller@google.com","subject":"Re: [PATCH] config: fix case sensitive subsection names on writing","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-27T21:21:40Z","receivedAt":"2018-07-27T21:21:44Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/27, Stefan Beller wrote:\n> A use reported a submodule issue regarding strange case indentation\n> issues, but it could be boiled down to the following test case:\n> \n> $ git init test && cd test\n> $ git config foo.\"Bar\".key test\n> $ git config foo.\"bar\".key test\n> $ tail -n 3 .git/config\n> [foo \"Bar\"]\n> key = test\n> key = test\n> \n> Sub sections are case sensitive and we have a test for correctly reading\n> them. However we do not have a test for writing out config correctly with\n> case sensitive subsection names, which is why this went unnoticed in\n> 6ae996f2acf (git_config_set: make use of the config parser's event\n> stream, 2018-04-09)\n> \n> Make the subsection case sensitive and provide a test for both reading\n> and writing.\n> \n> Reported-by: JP Sugarbroad \n> Signed-off-by: Stefan Beller \n> ---\n> config.c | 2 +-\n> t/t1300-config.sh | 18 ++++++++++++++++++\n> 2 files changed, 19 insertions(+), 1 deletion(-)\n> \n> diff --git a/config.c b/config.c\n> index 3aacddfec4b..3ded92b678b 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2374,7 +2374,7 @@ static int store_aux_event(enum config_event_t type,\n> \t\tstore->is_keys_section =\n> \t\t\tstore->parsed[store->parsed_nr].is_keys_section =\n> \t\t\tcf->var.len - 1 == store->baselen &&\n> -\t\t\t!strncasecmp(cf->var.buf, store->key, store->baselen);\n> +\t\t\t!strncmp(cf->var.buf, store->key, store->baselen);\n\nI've done some work in the config part of our codebase but I don't\nreally know whats going on here due to two things: this is a callback\nfunction and it relies on global state...\n\nI can say though that we might want to be careful about completely\nconverting this to a case sensitive compare. Our config is a key\nvalue store with the following format: 'section.subsection.key'. IIRC\nboth section and key are always compared case insensitively. The\nsubsection can be case sensitive or insensitive based on how its\nstored in the config files itself:\n\n # Case insensitive\n [section.subsection]\n key = value\n\n # Case sensitive \n [section \"subsection\"]\n key = value\n\nBut I don't know how you distinguish between these cases when a config\nis specified on a single line (section.subsection.key). Do we always\nassume the sensitive version because the insensitive version is\ndocumented to be deprecated?\n\nEither way you're probably going to need to be careful about how you do\nstring comparison against the different parts.\n\n> \t\tif (store->is_keys_section) {\n> \t\t\tstore->section_seen = 1;\n> \t\t\tALLOC_GROW(store->seen, store->seen_nr + 1,\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index 03c223708eb..8325d4495f4 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -1218,6 +1218,24 @@ test_expect_success 'last one wins: three level vars' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'setting different case subsections ' '\n> +\ttest_when_finished \"rm -f caseSens caseSens_actual caseSens_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.\n> +\tgit config -f caseSens v.\"A\".r VAL &&\n> +\tgit config -f caseSens v.\"a\".r val &&\n> +\n> +\techo VAL >caseSens_expect &&\n> +\tgit config -f caseSens v.\"A\".r >caseSens_actual &&\n> +\ttest_cmp caseSens_expect caseSens_actual &&\n> +\n> +\techo val >caseSens_expect &&\n> +\tgit config -f caseSens v.\"a\".r >caseSens_actual &&\n> +\ttest_cmp caseSens_expect caseSens_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> -- \n> 2.18.0.345.g5c9ce644c3-goog\n> \n\n-- \nBrandon Williams\n"},{"id":"353757","messageId":"xmqqk1pgl5rf.fsf@gitster-ct.c.googlers.com","threadId":"48965","inReplyTo":"20180727205117.243770-1-sbeller@google.com","subject":"Re: [PATCH] config: fix case sensitive subsection names on writing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T21:37:24Z","receivedAt":"2018-07-27T21:37:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> A use reported a submodule issue regarding strange case indentation\n> issues, but it could be boiled down to the following test case:\n>\n> $ git init test && cd test\n> $ git config foo.\"Bar\".key test\n> $ git config foo.\"bar\".key test\n> $ tail -n 3 .git/config\n> [foo \"Bar\"]\n> key = test\n> key = test\n>\n> Sub sections are case sensitive and we have a test for correctly reading\n> them. However we do not have a test for writing out config correctly with\n> case sensitive subsection names, which is why this went unnoticed in\n> 6ae996f2acf (git_config_set: make use of the config parser's event\n> stream, 2018-04-09)\n\nThanks for finding this bug and fixing it; yes it would have been\neven nicer if we caught it before it hit 'master', but we can only\ndo what we can X-<.\n\n> diff --git a/config.c b/config.c\n> index 3aacddfec4b..3ded92b678b 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2374,7 +2374,7 @@ static int store_aux_event(enum config_event_t type,\n> \t\tstore->is_keys_section =\n> \t\t\tstore->parsed[store->parsed_nr].is_keys_section =\n> \t\t\tcf->var.len - 1 == store->baselen &&\n> -\t\t\t!strncasecmp(cf->var.buf, store->key, store->baselen);\n> +\t\t\t!strncmp(cf->var.buf, store->key, store->baselen);\n> \t\tif (store->is_keys_section) {\n> \t\t\tstore->section_seen = 1;\n> \t\t\tALLOC_GROW(store->seen, store->seen_nr + 1,\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index 03c223708eb..8325d4495f4 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -1218,6 +1218,24 @@ test_expect_success 'last one wins: three level vars' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'setting different case subsections ' '\n> +\ttest_when_finished \"rm -f caseSens caseSens_actual caseSens_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.\n> +\tgit config -f caseSens v.\"A\".r VAL &&\n> +\tgit config -f caseSens v.\"a\".r val &&\n\nIt probably is easier to read to write these as \"v.A.r\" and \"v.a.r\"\nrespectively.\n\n> +\n> +\techo VAL >caseSens_expect &&\n> +\tgit config -f caseSens v.\"A\".r >caseSens_actual &&\n> +\ttest_cmp caseSens_expect caseSens_actual &&\n> +\n> +\techo val >caseSens_expect &&\n> +\tgit config -f caseSens v.\"a\".r >caseSens_actual &&\n> +\ttest_cmp caseSens_expect caseSens_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"},{"id":"353758","messageId":"xmqqfu04l5ns.fsf@gitster-ct.c.googlers.com","threadId":"48965","inReplyTo":"20180727212140.GA54208@google.com","subject":"Re: [PATCH] config: fix case sensitive subsection names on writing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T21:39:35Z","receivedAt":"2018-07-27T21:39:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Either way you're probably going to need to be careful about how you do\n> string comparison against the different parts.\n\nGood suggestion.\n\n>> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n>> index 03c223708eb..8325d4495f4 100755\n>> --- a/t/t1300-config.sh\n>> +++ b/t/t1300-config.sh\n>> @@ -1218,6 +1218,24 @@ test_expect_success 'last one wins: three level vars' '\n>> \ttest_cmp expect actual\n>> '\n>> \n>> +test_expect_success 'setting different case subsections ' '\n>> +\ttest_when_finished \"rm -f caseSens caseSens_actual caseSens_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.\n\nIn other words, perhaps add\n\n\t# \"V.a.r\" and \"v.a.R\" are the same variable, though\n\nand corresponding test here?\n\n>> +\tgit config -f caseSens v.\"A\".r VAL &&\n>> +\tgit config -f caseSens v.\"a\".r val &&\n>> +\n>> +\techo VAL >caseSens_expect &&\n>> +\tgit config -f caseSens v.\"A\".r >caseSens_actual &&\n>> +\ttest_cmp caseSens_expect caseSens_actual &&\n>> +\n>> +\techo val >caseSens_expect &&\n>> +\tgit config -f caseSens v.\"a\".r >caseSens_actual &&\n>> +\ttest_cmp caseSens_expect caseSens_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>> -- \n>> 2.18.0.345.g5c9ce644c3-goog\n>> \n"},{"id":"353768","messageId":"CAGZ79kaVS96_K-G-_hEnRecBS843tjn7=Am0xZQjZABCdC7L0A@mail.gmail.com","threadId":"48965","inReplyTo":"xmqqfu04l5ns.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] config: fix case sensitive subsection names on writing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-27T23:35:10Z","receivedAt":"2018-07-27T23:35:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 27, 2018 at 2:39 PM Junio C Hamano wrote:\n>\n> Brandon Williams writes:\n>\n> > Either way you're probably going to need to be careful about how you do\n> > string comparison against the different parts.\n>\n> Good suggestion.\n\nThe suggestion is a rabit hole and was a waste of time.\n\nHowever I did some more manual testing and inspected the code\nwith trace_printf debugging, and it turns out the strings compared\nare brought into the correct form already.\n\n> >> + # v.a.r and v.A.r are not the same variable, as the middle\n> >> + # level of a three-level configuration variable name is\n> >> + # case sensitive.\n>\n> In other words, perhaps add\n>\n> # \"V.a.r\" and \"v.a.R\" are the same variable, though\n>\n> and corresponding test here?\n\nI removed that section and went for a shorter,\nmore concise expression.\n\npatch to follow.\n"},{"id":"353769","messageId":"20180727233606.179965-1-sbeller@google.com","threadId":"48965","inReplyTo":"CAGZ79kaVS96_K-G-_hEnRecBS843tjn7=Am0xZQjZABCdC7L0A@mail.gmail.com","subject":"[PATCH] config: fix case sensitive subsection names on writing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-27T23:36:06Z","receivedAt":"2018-07-27T23:36:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"A use reported a submodule issue regarding strange case indentation\nissues, but it could be boiled down to the following test case:\n\n $ git init test && cd test\n $ git config foo.\"Bar\".key test\n $ git config foo.\"bar\".key test\n $ tail -n 3 .git/config\n [foo \"Bar\"]\n key = test\n key = test\n\nSub sections are case sensitive and we have a test for correctly reading\nthem. However we do not have a test for writing out config correctly with\ncase sensitive subsection names, which is why this went unnoticed in\n6ae996f2acf (git_config_set: make use of the config parser's event\nstream, 2018-04-09)\n\nMake the subsection case sensitive and provide a test for writing.\nThe test for reading is just above this test.\n\nReported-by: JP Sugarbroad \nSigned-off-by: Stefan Beller \n---\n\nI really appreciate the work by DScho (and Peff as I recall him as an active\nreviewer there) on 4f4d0b42bae (Merge branch 'js/empty-config-section-fix',\n2018-05-08), as the corner cases are all correct, modulo the one line fix\nin this patch.\n\nThanks,\nStefan\n\n config.c | 2 +-\n t/t1300-config.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 58 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 7968ef7566a..de646d2c56f 100644\n--- a/config.c\n+++ b/config.c\n@@ -2362,7 +2362,7 @@ static int store_aux_event(enum config_event_t type,\n \t\tstore->is_keys_section =\n \t\t\tstore->parsed[store->parsed_nr].is_keys_section =\n \t\t\tcf->var.len - 1 == store->baselen &&\n-\t\t\t!strncasecmp(cf->var.buf, store->key, store->baselen);\n+\t\t\t!strncmp(cf->var.buf, store->key, store->baselen);\n \t\tif (store->is_keys_section) {\n \t\t\tstore->section_seen = 1;\n \t\t\tALLOC_GROW(store->seen, store->seen_nr + 1,\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 03c223708eb..710e2b04ad8 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1218,6 +1218,63 @@ test_expect_success 'last one wins: three level vars' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'old-fashioned settings are case insensitive' '\n+\ttest_when_finished \"rm -f testConfig testConfig_expect testConfig_actual\" &&\n+\n+\tcat >testConfig <<-EOF &&\n+\t\t# insensitive:\n+\t\t[V.A]\n+\t\tr = value1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t# insensitive:\n+\t\t[V.A]\n+\t\tQr = value2\n+\tEOF\n+\n+\tfor key in \"v.a.r\" \"V.A.r\" \"v.A.r\" \"V.a.r\"\n+\tdo\n+\t\tcp testConfig testConfig_actual &&\n+\t\tgit config -f testConfig_actual v.a.r value2 &&\n+\t\ttest_cmp testConfig_expect testConfig_actual\n+\tdone\n+'\n+\n+test_expect_success 'setting different case sensitive subsections ' '\n+\ttest_when_finished \"rm -f testConfig testConfig_expect testConfig_actual\" &&\n+\n+\tcat >testConfig <<-EOF &&\n+\t\t# V insensitive A sensitive:\n+\t\t[V \"A\"]\n+\t\tr = value1\n+\t\t# same as above:\n+\t\t[V \"a\"]\n+\t\tr = value2\n+\tEOF\n+\n+\tgit config -f testConfig v.a.r value3 &&\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t# V insensitive A sensitive:\n+\t\t[V \"A\"]\n+\t\tr = value1\n+\t\t# same as above:\n+\t\t[V \"a\"]\n+\t\tQr = value3\n+\tEOF\n+\ttest_cmp testConfig_expect testConfig &&\n+\n+\tgit config -f testConfig v.A.r value4 &&\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t# V insensitive A sensitive:\n+\t\t[V \"A\"]\n+\t\tQr = value4\n+\t\t# same as above:\n+\t\t[V \"a\"]\n+\t\tQr = value3\n+\tEOF\n+\ttest_cmp testConfig_expect testConfig\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-- \n2.18.0.345.g5c9ce644c3-goog\n\n"},{"id":"353770","messageId":"CAGZ79kawMESBLHRV6CN7QJVPgdxL_1-BJ3Wu+xAFdpJn5PbyRg@mail.gmail.com","threadId":"48965","inReplyTo":"20180727233606.179965-1-sbeller@google.com","subject":"Re: [PATCH] config: fix case sensitive subsection names on writing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-27T23:37:26Z","receivedAt":"2018-07-27T23:37:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I cc'd the wrong peff. Sorry about that.\n\nOn Fri, Jul 27, 2018 at 4:36 PM Stefan Beller wrote:\n....\n"},{"id":"353771","messageId":"xmqqlg9wjhq7.fsf@gitster-ct.c.googlers.com","threadId":"48965","inReplyTo":"20180727233606.179965-1-sbeller@google.com","subject":"Re: [PATCH] config: fix case sensitive subsection names on writing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-28T01:01:52Z","receivedAt":"2018-07-28T01:01:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> A use reported a submodule issue regarding strange case indentation\n\ns/use/&r/; Is this \"indentation\" issue?\n\n> issues, but it could be boiled down to the following test case:\n> ...\n\n> +test_expect_success 'old-fashioned settings are case insensitive' '\n> +\ttest_when_finished \"rm -f testConfig testConfig_expect testConfig_actual\" &&\n> +\n> +\tcat >testConfig <<-EOF &&\n> +\t\t# insensitive:\n> +\t\t[V.A]\n> +\t\tr = value1\n> +\tEOF\n> +\tq_to_tab >testConfig_expect <<-EOF &&\n> +\t\t# insensitive:\n> +\t\t[V.A]\n> +\t\tQr = value2\n> +\tEOF\n\nIt is unfortunate that we hardcode the exact indentation\nin the test to make it care. Perhaps a wrapper around test_cmp that\nis used locally in this file to first strip the leading HT from both\nsides of the comparison would make it more robust?\n\n> +\tfor key in \"v.a.r\" \"V.A.r\" \"v.A.r\" \"V.a.r\"\n> +\tdo\n> +\t\tcp testConfig testConfig_actual &&\n> +\t\tgit config -f testConfig_actual v.a.r value2 &&\n> +\t\ttest_cmp testConfig_expect testConfig_actual\n> +\tdone\n> +'\n\nI think you meant to use \"$key\" when setting the variable to value2.\n\nWhen the test_cmp fails with \"v.a.r\" but later succeeds and most\nimportantly succeeds with \"V.a.r\" (i.e. the last one), wouldn't the\nwhole thing suceed? I think the common trick people use is to end\nthe last one with \"|| return 1\" in a loop inside test_expect_success.\n\n"},{"id":"353772","messageId":"xmqqh8kkjg23.fsf@gitster-ct.c.googlers.com","threadId":"48965","inReplyTo":"20180727233606.179965-1-sbeller@google.com","subject":"Re: [PATCH] config: fix case sensitive subsection names on writing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-28T01:37:56Z","receivedAt":"2018-07-28T01:38:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I really appreciate the work by DScho (and Peff as I recall him as an active\n> reviewer there) on 4f4d0b42bae (Merge branch 'js/empty-config-section-fix',\n> 2018-05-08), as the corner cases are all correct, modulo the one line fix\n> in this patch.\n\nAmen to the early part of that ;--) Even though it was merely\ncosmetic, and did not affect correctness, that longstandng bug was\nvery annoying bug to a lot of people. I too am very happy to see it\ndisappear.\n\nI would still hold the judgment on \"all except only this one\"\nmyself. That's a bit too early in my mind.\n"},{"id":"353782","messageId":"CAGZ79ka2yRv5+EaBoHx_TWdM1Qgt9F7YZiW=X35MyOehfcdQBg@mail.gmail.com","threadId":"48965","inReplyTo":"xmqqlg9wjhq7.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] config: fix case sensitive subsection names on writing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-28T03:52:59Z","receivedAt":"2018-07-28T03:53:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 27, 2018 at 6:01 PM Junio C Hamano wrote:\n>\n> Stefan Beller writes:\n>\n> > A use reported a submodule issue regarding strange case indentation\n>\n> s/use/&r/; Is this \"indentation\" issue?\n\neh case sensitivity*\n\n> > + q_to_tab >testConfig_expect <<-EOF &&\n> > + # insensitive:\n> > + [V.A]\n> > + Qr = value2\n> > + EOF\n>\n> It is unfortunate that we hardcode the exact indentation\n> in the test to make it care. Perhaps a wrapper around test_cmp that\n> is used locally in this file to first strip the leading HT from both\n> sides of the comparison would make it more robust?\n\nI think this is valuable for the second test to see where it was replaced.\n\n>\n> > + for key in \"v.a.r\" \"V.A.r\" \"v.A.r\" \"V.a.r\"\n> > + do\n> > + cp testConfig testConfig_actual &&\n> > + git config -f testConfig_actual v.a.r value2 &&\n> > + test_cmp testConfig_expect testConfig_actual\n> > + done\n> > +'\n>\n> I think you meant to use \"$key\" when setting the variable to value2.\n>\n> When the test_cmp fails with \"v.a.r\" but later succeeds and most\n> importantly succeeds with \"V.a.r\" (i.e. the last one), wouldn't the\n> whole thing suceed? I think the common trick people use is to end\n> the last one with \"|| return 1\" in a loop inside test_expect_success.\n>\n\nHah, of course this patch is not as easy.\n\nThe problem is in git_parse_source (config.c, line 755) when we call\nget_base_var it normalizes both styles, (and lower cases the dot style).\n\nSo in case of dot style, the strncasecmp is correct, but for the new\nextended style we need to strncmp.\n\nSo this is correct for extended but not any more for the former dot style.\n\nI wonder if we want to either (a) add another CONFIG_EVENT_SECTION\nthat indicates the difference between the two, or have a flag in 'cf' that\ntells us what kind of section style we have, such that we can check\nappropriately for the case.\nOr we could encode it in the 'cf->var' value to distinguish the old\nand new style.\n\nThanks,\nStefan\n"},{"id":"353796","messageId":"20180728105351.GA13283@sigill.intra.peff.net","threadId":"48965","inReplyTo":"CAGZ79ka2yRv5+EaBoHx_TWdM1Qgt9F7YZiW=X35MyOehfcdQBg@mail.gmail.com","subject":"Re: [PATCH] config: fix case sensitive subsection names on writing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-28T10:53:51Z","receivedAt":"2018-07-28T10:53:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 27, 2018 at 08:52:59PM -0700, Stefan Beller wrote:\n\n> > > + for key in \"v.a.r\" \"V.A.r\" \"v.A.r\" \"V.a.r\"\n> > > + do\n> > > + cp testConfig testConfig_actual &&\n> > > + git config -f testConfig_actual v.a.r value2 &&\n> > > + test_cmp testConfig_expect testConfig_actual\n> > > + done\n> > > +'\n> >\n> > I think you meant to use \"$key\" when setting the variable to value2.\n> >\n> > When the test_cmp fails with \"v.a.r\" but later succeeds and most\n> > importantly succeeds with \"V.a.r\" (i.e. the last one), wouldn't the\n> > whole thing suceed? I think the common trick people use is to end\n> > the last one with \"|| return 1\" in a loop inside test_expect_success.\n> \n> Hah, of course this patch is not as easy.\n> \n> The problem is in git_parse_source (config.c, line 755) when we call\n> get_base_var it normalizes both styles, (and lower cases the dot style).\n> \n> So in case of dot style, the strncasecmp is correct, but for the new\n> extended style we need to strncmp.\n> \n> So this is correct for extended but not any more for the former dot style.\n\nHmm, it looks like this has always been broken. Before your patch (but\nafter v2.18.0), running \"git config v.A.r value2\" writes:\n\n [V.A]\n r = value1\n r = value2\n\nwhich is obviously broken (we decided it was the right section, but\ndidn't match the variables, leading to a weird duplicate). After your\npatch we write:\n\n [V.A]\n r = value1\n [v \"A\"]\n r = value2\n\nI.e., we do not consider them the same value at all. But that's actually\nwhat happened before v2.18, too. I think you could almost justify that\nbehavior as \"The section.subsection syntax is deprecated, so we match it\ncase-insensitively on reading but not on writing; since we never write\nsuch sections ourselves, you are on your own for modifying such entries\nif you choose to write them manually\".\n\nI dunno. Maybe that is too harsh. But this syntax has been deprecated\nfor 7 years, and nobody has noticed the bug until now (when _still_\nnobody wants to use it, but rather we're poking at it as \"gee, I wonder\nif this even works\").\n\n> I wonder if we want to either (a) add another CONFIG_EVENT_SECTION\n> that indicates the difference between the two, or have a flag in 'cf' that\n> tells us what kind of section style we have, such that we can check\n> appropriately for the case.\n\nI'd think that the parse_event_data could carry type-specific\ninformation. But...\n\n> Or we could encode it in the 'cf->var' value to distinguish the old\n> and new style.\n\nEven if we fix the section-matching here, I suspect that would just\ntrigger a separate bug later when we match the full key (so we might add\nto the correct section, but we would not correctly replace an existing\nkey). To fix that, the information does need to be carried for the whole\nlifetime of cf->var, not just during the header event.\n\n-Peff\n"},{"id":"353871","messageId":"nycvar.QRO.7.76.6.1807301438440.10478@tvgsbejvaqbjf.bet","threadId":"48965","inReplyTo":"xmqqh8kkjg23.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] config: fix case sensitive subsection names on writing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-30T12:49:56Z","receivedAt":"2018-07-30T12:50:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 27 Jul 2018, Junio C Hamano wrote:\n\n> Stefan Beller writes:\n>\n> [...]\n\nThanks for the patch!\n\nThe only thing that was not clear to me from the patch and from the commit\nmessage was: the first part *is* case insensitive, right? How does the\npatch take care of that? Is it relying on `git_config_parse_key()` to do\nthat? If so, I don't see it...\n\n> I would still hold the judgment on \"all except only this one\"\n> myself. That's a bit too early in my mind.\n\nAgreed. I seem to remember that I had a funny problem \"in the reverse\",\nwhere http..* is case-sensitive, but in an unexpected way: if the URL\ncontains upper-case characters, the part of the config key needs to\nbe downcased, otherwise the setting won't be picked up.\n\nCiao,\nDscho\n"},{"id":"353972","messageId":"20180730230443.74416-1-sbeller@google.com","threadId":"48965","inReplyTo":"nycvar.QRO.7.76.6.1807301438440.10478@tvgsbejvaqbjf.bet","subject":"[PATCH 0/3] config: fix case sensitive subsection names on writing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-30T23:04:40Z","receivedAt":"2018-07-30T23:04:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jul 30, 2018 at 5:50 AM Johannes Schindelin wrote:\n> Thanks for the patch!\n>\n> The only thing that was not clear to me from the patch and from the commit\n> message was: the first part *is* case insensitive, right?\n\nright.\n\n> How does the\n> patch take care of that? Is it relying on `git_config_parse_key()` to do\n> that? If so, I don't see it...\n\nIt turns out it doesn't quite do that;\nThe parsing code takes the old notation into account and translates any\n [V.A]\n r = ...\ninto a lower cased \"v.a.\" for ease of comparison. That happens in\nget_base_var, which would call further into get_extended_base_var\nif the new notation is used.\n\nThe code in store_aux_event however is written without the consideration\nof the old code and has no way of knowing the capitalization of the\nsection or subsection (which were forced to lowercase in the old\ndot notation). \n\nSo either we have to do some major surgery, or the old notation gets\nsome regression while fixing the new notation.\n\n> > I would still hold the judgment on \"all except only this one\"\n> > myself. That's a bit too early in my mind.\n>\n> Agreed. I seem to remember that I had a funny problem \"in the reverse\",\n> where http..* is case-sensitive, but in an unexpected way: if the URL\n> contains upper-case characters, the part of the config key needs to\n> be downcased, otherwise the setting won't be picked up.\n\nI wrote some patches that show more of what is happening.\n\nI suggest to drop the last patch and only take the first two,\nor if we decide we want to be fully correct, we'd want to discuss\nhow to make it happen. (where to store the information that we are\ndealing with an old notation)\n\nThanks,\nStefan\n\nStefan Beller (3):\n t1300: document current behavior of setting options\n config: fix case sensitive subsection names on writing\n config: treat section case insensitive in store_aux_event\n\n config.c | 16 ++++++++-\n t/t1300-config.sh | 89 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 104 insertions(+), 1 deletion(-)\n\n-- \n2.18.0.132.g195c49a2227\n\n"},{"id":"353973","messageId":"20180730230443.74416-2-sbeller@google.com","threadId":"48965","inReplyTo":"20180730230443.74416-1-sbeller@google.com","subject":"[PATCH 1/3] t1300: document current behavior of setting options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-30T23:04:41Z","receivedAt":"2018-07-30T23:04:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This documents current behavior of the config machinery, when changing\nthe value of some settings. This patch just serves to provide a baseline\nfor the follow up that will fix some issues with the current behavior.\n\nSigned-off-by: Stefan Beller \n---\n t/t1300-config.sh | 86 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 86 insertions(+)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 03c223708eb..ced13012409 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1218,6 +1218,92 @@ test_expect_success 'last one wins: three level vars' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'old-fashioned settings are case insensitive' '\n+\ttest_when_finished \"rm -f testConfig testConfig_expect testConfig_actual\" &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V.A]\n+\t\tQr = value2\n+\tEOF\n+\tgit config -f testConfig_actual \"v.a.r\" value2 &&\n+\ttest_cmp testConfig_expect testConfig_actual &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V.A]\n+\t\tQR = value2\n+\tEOF\n+\tgit config -f testConfig_actual \"V.a.R\" value2 &&\n+\ttest_cmp testConfig_expect testConfig_actual &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\t\tQr = value2\n+\tEOF\n+\tgit config -f testConfig_actual \"V.A.r\" value2 &&\n+\ttest_cmp testConfig_expect testConfig_actual &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\t\tQr = value2\n+\tEOF\n+\tgit config -f testConfig_actual \"v.A.r\" value2 &&\n+\ttest_cmp testConfig_expect testConfig_actual\n+'\n+\n+test_expect_success 'setting different case sensitive subsections ' '\n+\ttest_when_finished \"rm -f testConfig testConfig_expect testConfig_actual\" &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V \"A\"]\n+\t\tR = v1\n+\t\t[K \"E\"]\n+\t\tY = v1\n+\t\t[a \"b\"]\n+\t\tc = v1\n+\t\t[d \"e\"]\n+\t\tf = v1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V \"A\"]\n+\t\tQr = v2\n+\t\t[K \"E\"]\n+\t\tQy = v2\n+\t\t[a \"b\"]\n+\t\tQc = v2\n+\t\t[d \"e\"]\n+\t\tf = v1\n+\t\tQf = v2\n+\tEOF\n+\t# exact match\n+\tgit config -f testConfig_actual a.b.c v2 &&\n+\t# match section and subsection, key is cased differently.\n+\tgit config -f testConfig_actual K.E.y v2 &&\n+\t# section and key are matched case insensitive, but subsection needs\n+\t# to match; When writing out new values only the key is adjusted\n+\tgit config -f testConfig_actual v.A.r v2 &&\n+\t# subsection is not matched:\n+\tgit config -f testConfig_actual d.E.f v2 &&\n+\ttest_cmp testConfig_expect testConfig_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-- \n2.18.0.132.g195c49a2227\n\n"},{"id":"353974","messageId":"20180730230443.74416-3-sbeller@google.com","threadId":"48965","inReplyTo":"20180730230443.74416-1-sbeller@google.com","subject":"[PATCH 2/3] config: fix case sensitive subsection names on writing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-30T23:04:42Z","receivedAt":"2018-07-30T23:04:54Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"A use reported a submodule issue regarding strange case indentation\nissues, but it could be boiled down to the following test case:\n\n $ git init test && cd test\n $ git config foo.\"Bar\".key test\n $ git config foo.\"bar\".key test\n $ tail -n 3 .git/config\n [foo \"Bar\"]\n key = test\n key = test\n\nSub sections are case sensitive and we have a test for correctly reading\nthem. However we do not have a test for writing out config correctly with\ncase sensitive subsection names, which is why this went unnoticed in\n6ae996f2acf (git_config_set: make use of the config parser's event\nstream, 2018-04-09)\n\nMake the subsection case sensitive and provide a test for writing.\nThe test for reading is just above this test.\n\nReported-by: JP Sugarbroad \nSigned-off-by: Stefan Beller \n---\n config.c | 2 +-\n t/t1300-config.sh | 3 +++\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 7968ef7566a..de646d2c56f 100644\n--- a/config.c\n+++ b/config.c\n@@ -2362,7 +2362,7 @@ static int store_aux_event(enum config_event_t type,\n \t\tstore->is_keys_section =\n \t\t\tstore->parsed[store->parsed_nr].is_keys_section =\n \t\t\tcf->var.len - 1 == store->baselen &&\n-\t\t\t!strncasecmp(cf->var.buf, store->key, store->baselen);\n+\t\t\t!strncmp(cf->var.buf, store->key, store->baselen);\n \t\tif (store->is_keys_section) {\n \t\t\tstore->section_seen = 1;\n \t\t\tALLOC_GROW(store->seen, store->seen_nr + 1,\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex ced13012409..e5d16f53ee1 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1250,6 +1250,7 @@ test_expect_success 'old-fashioned settings are case insensitive' '\n \tq_to_tab >testConfig_expect <<-EOF &&\n \t\t[V.A]\n \t\tr = value1\n+\t\t[V \"A\"]\n \t\tQr = value2\n \tEOF\n \tgit config -f testConfig_actual \"V.A.r\" value2 &&\n@@ -1262,6 +1263,7 @@ test_expect_success 'old-fashioned settings are case insensitive' '\n \tq_to_tab >testConfig_expect <<-EOF &&\n \t\t[V.A]\n \t\tr = value1\n+\t\t[v \"A\"]\n \t\tQr = value2\n \tEOF\n \tgit config -f testConfig_actual \"v.A.r\" value2 &&\n@@ -1290,6 +1292,7 @@ test_expect_success 'setting different case sensitive subsections ' '\n \t\tQc = v2\n \t\t[d \"e\"]\n \t\tf = v1\n+\t\t[d \"E\"]\n \t\tQf = v2\n \tEOF\n \t# exact match\n-- \n2.18.0.132.g195c49a2227\n\n"},{"id":"353975","messageId":"20180730230443.74416-4-sbeller@google.com","threadId":"48965","inReplyTo":"20180730230443.74416-1-sbeller@google.com","subject":"[PATCH 3/3] config: treat section case insensitive in store_aux_event","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-30T23:04:43Z","receivedAt":"2018-07-30T23:04:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is a no-op because the section names are lower-cased already in\nget_base_var, this is purely for demonstration that we do not need to\ncare about case issues in this part of the code.\n\nSigned-off-by: Stefan Beller \n---\n config.c | 16 +++++++++++++++-\n 1 file changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex de646d2c56f..c247164ad17 100644\n--- a/config.c\n+++ b/config.c\n@@ -2355,14 +2355,28 @@ static int store_aux_event(enum config_event_t type,\n \tstore->parsed[store->parsed_nr].type = type;\n \n \tif (type == CONFIG_EVENT_SECTION) {\n+\t\tchar *p;\n+\t\tint slen; /* section length */\n \t\tif (cf->var.len < 2 || cf->var.buf[cf->var.len - 1] != '.')\n \t\t\treturn error(\"invalid section name '%s'\", cf->var.buf);\n \n+\t\tp = strchr(cf->var.buf, '.');\n+\t\tif (!p)\n+\t\t\t/* no subsection, so treat all as section: */\n+\t\t\tslen = store->baselen;\n+\t\telse\n+\t\t\tslen = p - cf->var.buf;\n+\n+\t\tif (slen > store->baselen)\n+\t\t\tslen = store->baselen;\n+\n \t\t/* Is this the section we were looking for? */\n \t\tstore->is_keys_section =\n \t\t\tstore->parsed[store->parsed_nr].is_keys_section =\n \t\t\tcf->var.len - 1 == store->baselen &&\n-\t\t\t!strncmp(cf->var.buf, store->key, store->baselen);\n+\t\t\t!strncasecmp(cf->var.buf, store->key, slen) &&\n+\t\t\t!strncmp(cf->var.buf + slen, store->key + slen,\n+\t\t\t\t store->baselen - slen);\n \t\tif (store->is_keys_section) {\n \t\t\tstore->section_seen = 1;\n \t\t\tALLOC_GROW(store->seen, store->seen_nr + 1,\n-- \n2.18.0.132.g195c49a2227\n\n"},{"id":"354042","messageId":"xmqq7elbe8po.fsf@gitster-ct.c.googlers.com","threadId":"48965","inReplyTo":"20180730230443.74416-1-sbeller@google.com","subject":"Re: [PATCH 0/3] config: fix case sensitive subsection names on writing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T15:16:51Z","receivedAt":"2018-07-31T15:16:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> It turns out it doesn't quite do that;\n> The parsing code takes the old notation into account and translates any\n> [V.A]\n> r = ...\n> into a lower cased \"v.a.\" for ease of comparison. That happens in\n> get_base_var, which would call further into get_extended_base_var\n> if the new notation is used.\n>\n> The code in store_aux_event however is written without the consideration\n> of the old code and has no way of knowing the capitalization of the\n> section or subsection (which were forced to lowercase in the old\n> dot notation). \n>\n> So either we have to do some major surgery, or the old notation gets\n> some regression while fixing the new notation.\n\nAs long as\n\n - the regression is documented clearly (i.e. what unexpected thing\n happens, just like you have a good description in the log message\n of PATCH 2/3 for \"[foo \"Bar\"] key = ...\"),\n\n - users are nudged to use the new style instead, and\n\n - writing with \"git config\" (or \"git init/clone\" for that matter)\n won't produce these old-style sections\n\nI think it is OK to punt. I would expect, as Git's userbase is a\nlot wider than 10 years ago, there is at least some people who did\nexploit the fact that \"[V.A] r = one\" and \"[v.a] r = two\" give a\nsingle \"v.a.r\" variable that is multi-valued, and it indeed would be\na regression to them, to which no good workaround exists. \n\nBut breaking '[V \"a\"] r = ...' is a more grave sin. Even though I\nhate to rob Peter to pay Paul (or vice versa), in this case it is\njustified to fix the more basic form sooner and wait for an actual\ncomplaint before fixing the new and deliberate regression introduced\nwhile fixing it to the old-style variable.\n\n\n"},{"id":"354118","messageId":"xmqqr2jj9n45.fsf@gitster-ct.c.googlers.com","threadId":"48965","inReplyTo":"20180730230443.74416-3-sbeller@google.com","subject":"Re: [PATCH 2/3] config: fix case sensitive subsection names on writing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T20:16:58Z","receivedAt":"2018-07-31T20:17:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> A use reported a submodule issue regarding strange case indentation\n> issues, but it could be boiled down to the following test case:\n>\n> $ git init test && cd test\n> $ git config foo.\"Bar\".key test\n> $ git config foo.\"bar\".key test\n> $ tail -n 3 .git/config\n> [foo \"Bar\"]\n> key = test\n> key = test\n>\n> Sub sections are case sensitive and we have a test for correctly reading\n> them. However we do not have a test for writing out config correctly with\n> case sensitive subsection names, which is why this went unnoticed in\n> 6ae996f2acf (git_config_set: make use of the config parser's event\n> stream, 2018-04-09)\n\nAm I correct to understand that this patch is a \"FIX\" for breakage\nintroduced by that commit? The phrasing is not helping me to pick\na good base to queue these patches on.\n\nThanks.\n"},{"id":"354181","messageId":"20180801193413.146994-1-sbeller@google.com","threadId":"48965","inReplyTo":"xmqq7elbe8po.fsf@gitster-ct.c.googlers.com","subject":"[PATCH 0/3] sb/config-write-fix done without robbing Peter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-01T19:34:10Z","receivedAt":"2018-08-01T19:34:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Am I correct to understand that this patch is a \"FIX\" for breakage\n> introduced by that commit? The phrasing is not helping me to pick\n> a good base to queue these patches on.\n\nPlease pick 4f4d0b42bae (Merge branch 'js/empty-config-section-fix', 2018-05-08)\nas the base of this new series (am needs -3 to apply), although I developed this\nseries on origin/master.\n\n> Even though I hate to rob Peter to pay Paul (or vice versa)\n\nYeah me, too. Here is a proper fix (i.e. only pay Paul, without the robbery),\nand a documentation of the second bug that we discovered.\n\nThe first patch stands as is unchanged, and the second and third patch\nare different enough that range-diff doesn't want to show a diff.\n\nThanks,\nStefan\n\nStefan Beller (3):\n t1300: document current behavior of setting options\n config: fix case sensitive subsection names on writing\n git-config: document accidental multi-line setting in deprecated\n syntax\n\n Documentation/git-config.txt | 21 +++++++++\n config.c | 12 ++++-\n t/t1300-config.sh | 87 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 119 insertions(+), 1 deletion(-)\n"},{"id":"354182","messageId":"20180801193413.146994-2-sbeller@google.com","threadId":"48965","inReplyTo":"20180801193413.146994-1-sbeller@google.com","subject":"[PATCH 1/3] t1300: document current behavior of setting options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-01T19:34:11Z","receivedAt":"2018-08-01T19:34:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This documents current behavior of the config machinery, when changing\nthe value of some settings. This patch just serves to provide a baseline\nfor the follow up that will fix some issues with the current behavior.\n\nSigned-off-by: Stefan Beller \n---\n t/t1300-config.sh | 86 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 86 insertions(+)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 03c223708eb..ced13012409 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1218,6 +1218,92 @@ test_expect_success 'last one wins: three level vars' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'old-fashioned settings are case insensitive' '\n+\ttest_when_finished \"rm -f testConfig testConfig_expect testConfig_actual\" &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V.A]\n+\t\tQr = value2\n+\tEOF\n+\tgit config -f testConfig_actual \"v.a.r\" value2 &&\n+\ttest_cmp testConfig_expect testConfig_actual &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V.A]\n+\t\tQR = value2\n+\tEOF\n+\tgit config -f testConfig_actual \"V.a.R\" value2 &&\n+\ttest_cmp testConfig_expect testConfig_actual &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\t\tQr = value2\n+\tEOF\n+\tgit config -f testConfig_actual \"V.A.r\" value2 &&\n+\ttest_cmp testConfig_expect testConfig_actual &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\t\tQr = value2\n+\tEOF\n+\tgit config -f testConfig_actual \"v.A.r\" value2 &&\n+\ttest_cmp testConfig_expect testConfig_actual\n+'\n+\n+test_expect_success 'setting different case sensitive subsections ' '\n+\ttest_when_finished \"rm -f testConfig testConfig_expect testConfig_actual\" &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V \"A\"]\n+\t\tR = v1\n+\t\t[K \"E\"]\n+\t\tY = v1\n+\t\t[a \"b\"]\n+\t\tc = v1\n+\t\t[d \"e\"]\n+\t\tf = v1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V \"A\"]\n+\t\tQr = v2\n+\t\t[K \"E\"]\n+\t\tQy = v2\n+\t\t[a \"b\"]\n+\t\tQc = v2\n+\t\t[d \"e\"]\n+\t\tf = v1\n+\t\tQf = v2\n+\tEOF\n+\t# exact match\n+\tgit config -f testConfig_actual a.b.c v2 &&\n+\t# match section and subsection, key is cased differently.\n+\tgit config -f testConfig_actual K.E.y v2 &&\n+\t# section and key are matched case insensitive, but subsection needs\n+\t# to match; When writing out new values only the key is adjusted\n+\tgit config -f testConfig_actual v.A.r v2 &&\n+\t# subsection is not matched:\n+\tgit config -f testConfig_actual d.E.f v2 &&\n+\ttest_cmp testConfig_expect testConfig_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-- \n2.18.0.132.g195c49a2227\n\n"},{"id":"354183","messageId":"20180801193413.146994-3-sbeller@google.com","threadId":"48965","inReplyTo":"20180801193413.146994-1-sbeller@google.com","subject":"[PATCH 2/3] config: fix case sensitive subsection names on writing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-01T19:34:12Z","receivedAt":"2018-08-01T19:34:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"A use reported a submodule issue regarding strange case indentation\nissues, but it could be boiled down to the following test case:\n\n $ git init test && cd test\n $ git config foo.\"Bar\".key test\n $ git config foo.\"bar\".key test\n $ tail -n 3 .git/config\n [foo \"Bar\"]\n key = test\n key = test\n\nSub sections are case sensitive and we have a test for correctly reading\nthem. However we do not have a test for writing out config correctly with\ncase sensitive subsection names, which is why this went unnoticed in\n6ae996f2acf (git_config_set: make use of the config parser's event\nstream, 2018-04-09)\n\nUnfortunately we have to make a distinction between old style configuration\nthat looks like\n\n [foo.Bar]\n key = test\n\nand the new quoted style as seen above. The old style is documented as\ncase-agnostic, hence we need to keep 'strncasecmp'; although the\nresulting setting for the old style config differs from the configuration.\nThat will be fixed in a follow up patch.\n\nReported-by: JP Sugarbroad \nSigned-off-by: Stefan Beller \n---\n config.c | 12 +++++++++++-\n t/t1300-config.sh | 1 +\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 7968ef7566a..1d3bf9b5fc0 100644\n--- a/config.c\n+++ b/config.c\n@@ -37,6 +37,7 @@ struct config_source {\n \tint eof;\n \tstruct strbuf value;\n \tstruct strbuf var;\n+\tunsigned section_name_old_dot_style : 1;\n \n \tint (*do_fgetc)(struct config_source *c);\n \tint (*do_ungetc)(int c, struct config_source *conf);\n@@ -605,6 +606,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n \n static int get_extended_base_var(struct strbuf *name, int c)\n {\n+\tcf->section_name_old_dot_style = 0;\n \tdo {\n \t\tif (c == '\\n')\n \t\t\tgoto error_incomplete_line;\n@@ -641,6 +643,7 @@ static int get_extended_base_var(struct strbuf *name, int c)\n \n static int get_base_var(struct strbuf *name)\n {\n+\tcf->section_name_old_dot_style = 1;\n \tfor (;;) {\n \t\tint c = get_next_char();\n \t\tif (cf->eof)\n@@ -2355,14 +2358,21 @@ static int store_aux_event(enum config_event_t type,\n \tstore->parsed[store->parsed_nr].type = type;\n \n \tif (type == CONFIG_EVENT_SECTION) {\n+\t\tint (*cmpfn)(const char *, const char *, size_t);\n+\n \t\tif (cf->var.len < 2 || cf->var.buf[cf->var.len - 1] != '.')\n \t\t\treturn error(\"invalid section name '%s'\", cf->var.buf);\n \n+\t\tif (cf->section_name_old_dot_style)\n+\t\t\tcmpfn = strncasecmp;\n+\t\telse\n+\t\t\tcmpfn = strncmp;\n+\n \t\t/* Is this the section we were looking for? */\n \t\tstore->is_keys_section =\n \t\t\tstore->parsed[store->parsed_nr].is_keys_section =\n \t\t\tcf->var.len - 1 == store->baselen &&\n-\t\t\t!strncasecmp(cf->var.buf, store->key, store->baselen);\n+\t\t\t!cmpfn(cf->var.buf, store->key, store->baselen);\n \t\tif (store->is_keys_section) {\n \t\t\tstore->section_seen = 1;\n \t\t\tALLOC_GROW(store->seen, store->seen_nr + 1,\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex ced13012409..a93f966f128 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1290,6 +1290,7 @@ test_expect_success 'setting different case sensitive subsections ' '\n \t\tQc = v2\n \t\t[d \"e\"]\n \t\tf = v1\n+\t\t[d \"E\"]\n \t\tQf = v2\n \tEOF\n \t# exact match\n-- \n2.18.0.132.g195c49a2227\n\n"},{"id":"354184","messageId":"20180801193413.146994-4-sbeller@google.com","threadId":"48965","inReplyTo":"20180801193413.146994-1-sbeller@google.com","subject":"[PATCH 3/3] git-config: document accidental multi-line setting in deprecated syntax","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-01T19:34:13Z","receivedAt":"2018-08-01T19:34:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The bug was noticed when writing the previous patch; a fix for this bug\nis not easy though: If we choose to ignore the case of the subsection\n(and revert most of the code of the previous patch, just keeping\ns/strncasecmp/strcmp/), then we'd introduce new sections using the\nnew syntax, such that\n\n --------\n [section.subsection]\n key = value1\n --------\n\n git config section.Subsection.key value2\n\nwould result in\n\n --------\n [section.subsection]\n key = value1\n [section.Subsection]\n key = value2\n --------\n\nwhich is even more confusing. A proper fix would replace the first\noccurrence of 'key'. As the syntax is deprecated, let's prefer to not\nspend time on fixing the behavior and just document it instead.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/git-config.txt | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 18ddc78f42d..8e240435bee 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -453,6 +453,27 @@ http.sslverify false\n \n include::config.txt[]\n \n+BUGS\n+----\n+When using the deprecated `[section.subsection]` syntax, changing a value\n+will result in adding a multi-line key instead of a change, if the subsection\n+is given with at least one uppercase character. For example when the config\n+looks like\n+\n+--------\n+ [section.subsection]\n+ key = value1\n+--------\n+\n+and running `git config section.Subsection.key value2` will result in\n+\n+--------\n+ [section.subsection]\n+ key = value1\n+ key = value2\n+--------\n+\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\n-- \n2.18.0.132.g195c49a2227\n\n"},{"id":"354186","messageId":"CAPig+cSrD4WthFSGH4YETyFtKwZve9qitz8Da-OVVOS4yJps_g@mail.gmail.com","threadId":"48965","inReplyTo":"20180801193413.146994-1-sbeller@google.com","subject":"Re: [PATCH 0/3] sb/config-write-fix done without robbing Peter","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-01T19:58:24Z","receivedAt":"2018-08-01T19:58:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 1, 2018 at 3:34 PM Stefan Beller wrote:\n> The first patch stands as is unchanged, and the second and third patch\n> are different enough that range-diff doesn't want to show a diff.\n\nFor future reference, range-diff's --creation-factor tweak may help\nhere. Depending upon just how heavily changed they are, you may be\nable to use it to coerce range-diff into doing what you want.\n\n(Also, interdiff may be a valid alternative.)\n"},{"id":"354194","messageId":"15bd43c8-361a-eb05-3712-e9fd02428b72@ramsayjones.plus.com","threadId":"48965","inReplyTo":"20180801193413.146994-3-sbeller@google.com","subject":"Re: [PATCH 2/3] config: fix case sensitive subsection names on writing","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-08-01T21:01:21Z","receivedAt":"2018-08-01T21:01:26Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 01/08/18 20:34, Stefan Beller wrote:\n> A use reported a submodule issue regarding strange case indentation\n\ns/use/user/ ?\n\nATB,\nRamsay Jones\n\n"},{"id":"354202","messageId":"xmqqftzx67vo.fsf@gitster-ct.c.googlers.com","threadId":"48965","inReplyTo":"15bd43c8-361a-eb05-3712-e9fd02428b72@ramsayjones.plus.com","subject":"Re: [PATCH 2/3] config: fix case sensitive subsection names on writing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-01T22:26:35Z","receivedAt":"2018-08-01T22:26:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> On 01/08/18 20:34, Stefan Beller wrote:\n>> A use reported a submodule issue regarding strange case indentation\n>\n> s/use/user/ ?\n\nTrue. Also s/indentation/something else/ ?\n\nInsufficient proofreading, perhaps?\n\n"},{"id":"354205","messageId":"xmqqva8t4s63.fsf@gitster-ct.c.googlers.com","threadId":"48965","inReplyTo":"20180801193413.146994-3-sbeller@google.com","subject":"Re: [PATCH 2/3] config: fix case sensitive subsection names on writing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-01T22:51:16Z","receivedAt":"2018-08-01T22:51:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> A use reported a submodule issue regarding strange case indentation\n> issues, but it could be boiled down to the following test case:\n\nPerhaps\n\ns/use/user/\ns/case indentation issues/section mix-up/\n\n> ... However we do not have a test for writing out config correctly with\n> case sensitive subsection names, which is why this went unnoticed in\n> 6ae996f2acf (git_config_set: make use of the config parser's event\n> stream, 2018-04-09)\n\ns/unnoticed in \\(.*04-09)\\)/unnoticed when \\1 broke it./\n\nThis is why I asked if the patch is a \"FIX\" for an issue introduced\nby the cited commit.\n\n> Unfortunately we have to make a distinction between old style configuration\n> that looks like\n>\n> [foo.Bar]\n> key = test\n>\n> and the new quoted style as seen above. The old style is documented as\n> case-agnostic, hence we need to keep 'strncasecmp'; although the\n> resulting setting for the old style config differs from the configuration.\n> That will be fixed in a follow up patch.\n>\n> Reported-by: JP Sugarbroad \n> Signed-off-by: Stefan Beller \n> ---\n> config.c | 12 +++++++++++-\n> t/t1300-config.sh | 1 +\n> 2 files changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/config.c b/config.c\n> index 7968ef7566a..1d3bf9b5fc0 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -37,6 +37,7 @@ struct config_source {\n> \tint eof;\n> \tstruct strbuf value;\n> \tstruct strbuf var;\n> +\tunsigned section_name_old_dot_style : 1;\n> \n> \tint (*do_fgetc)(struct config_source *c);\n> \tint (*do_ungetc)(int c, struct config_source *conf);\n> @@ -605,6 +606,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n> \n> static int get_extended_base_var(struct strbuf *name, int c)\n> {\n> +\tcf->section_name_old_dot_style = 0;\n> \tdo {\n> \t\tif (c == '\\n')\n> \t\t\tgoto error_incomplete_line;\n> @@ -641,6 +643,7 @@ static int get_extended_base_var(struct strbuf *name, int c)\n> \n> static int get_base_var(struct strbuf *name)\n> {\n> +\tcf->section_name_old_dot_style = 1;\n> \tfor (;;) {\n> \t\tint c = get_next_char();\n> \t\tif (cf->eof)\n\nOK, let me rephrase. The basic parse structure is that\n\n * upon seeing '[', we call get_base_var(), which stuffs the\n \"section\" (including subsection, if exists) in the strbuf.\n\n * get_base_var() upon seeing a space after \"[section \", calls\n get_extended_base_var(). This space can never exist in an\n old-style three-level names, where it is spelled as\n \"[section.subsection]\". This space cannot exist in two-level\n names, either. The closing ']' is eaten by this function before\n it returns.\n\n * get_extended_base_var() grabs the \"double quoted\" subsection name\n and eats the closing ']' before it returns.\n\nSo you set the new bit (section_name_old_dot_style) at the beginning\nof get_base_var(), i.e. declare that you assume we are reading old\nstyle, but upon entering get_extended_base_var(), unset it, because\nnow you know we are parsing a modern style three-level name(s).\n\nFeels quite sensible way to keep track of old/new styles.\n\nWhen parsing two-level names, old-style bit is set, which we may\nneed to be careful, thoguh.\n\n> @@ -2355,14 +2358,21 @@ static int store_aux_event(enum config_event_t type,\n> \tstore->parsed[store->parsed_nr].type = type;\n> \n> \tif (type == CONFIG_EVENT_SECTION) {\n> +\t\tint (*cmpfn)(const char *, const char *, size_t);\n> +\n> \t\tif (cf->var.len < 2 || cf->var.buf[cf->var.len - 1] != '.')\n> \t\t\treturn error(\"invalid section name '%s'\", cf->var.buf);\n> \n> +\t\tif (cf->section_name_old_dot_style)\n> +\t\t\tcmpfn = strncasecmp;\n> +\t\telse\n> +\t\t\tcmpfn = strncmp;\n> +\n> \t\t/* Is this the section we were looking for? */\n> \t\tstore->is_keys_section =\n> \t\t\tstore->parsed[store->parsed_nr].is_keys_section =\n> \t\t\tcf->var.len - 1 == store->baselen &&\n> -\t\t\t!strncasecmp(cf->var.buf, store->key, store->baselen);\n> +\t\t\t!cmpfn(cf->var.buf, store->key, store->baselen);\n\nOK. Section names should still be case insensitive (only the case\nsensitivity of subsection names is special), but presumably that's\nalready normalized by the caller so we do not have to worry when we\nuse strncmp()? Can we add a test to demonstrate that it works\ncorrectly?\n\nThanks.\n\n"},{"id":"354314","messageId":"xmqq4lgc1rbv.fsf@gitster-ct.c.googlers.com","threadId":"48965","inReplyTo":"20180801193413.146994-1-sbeller@google.com","subject":"Re: [PATCH 0/3] sb/config-write-fix done without robbing Peter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-02T19:49:56Z","receivedAt":"2018-08-02T19:50:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> Am I correct to understand that this patch is a \"FIX\" for breakage\n>> introduced by that commit? The phrasing is not helping me to pick\n>> a good base to queue these patches on.\n>\n> Please pick 4f4d0b42bae (Merge branch 'js/empty-config-section-fix', 2018-05-08)\n> as the base of this new series (am needs -3 to apply), although I developed this\n> series on origin/master.\n>\n>> Even though I hate to rob Peter to pay Paul (or vice versa)\n>\n> Yeah me, too. Here is a proper fix (i.e. only pay Paul, without the robbery),\n> and a documentation of the second bug that we discovered.\n\nThanks.\n\nI started with this in 'config'\n\n [V.A]\tr = one\n [v.a]\tr = two\n\nand did \"git config --replace-all v.a.r 1\", which resulted in\n\n [V.A]\n [v.a]\n\t\tr = 1\n\nwhich is \"correct\", but defeats the \"empty section is irritating\"\ntweak that introduced the bug these patches try to fix, which is\nunfortunate.\n\nBut then \"git config v.A.r 2\" would give us\n\n [V.A]\n [v.a]\n\t\tr = 1\n\t\tr = 2\n\nSo this seems still broken, somewhat.\n\n"},{"id":"354340","messageId":"CAGZ79kbeUHVQmfTFkoGEM617iVa_3mCsLjpYGoEebqxNKAM_xA@mail.gmail.com","threadId":"48965","inReplyTo":"xmqqva8t4s63.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] config: fix case sensitive subsection names on writing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-03T00:30:10Z","receivedAt":"2018-08-03T00:30:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 1, 2018 at 3:51 PM Junio C Hamano wrote:\n>\n> Stefan Beller writes:\n>\n> > A use reported a submodule issue regarding strange case indentation\n> > issues, but it could be boiled down to the following test case:\n>\n> Perhaps\n>\n> s/use/user/\n> s/case indentation issues/section mix-up/\n\nwill be fixed in a reroll\n\n>\n> > ... However we do not have a test for writing out config correctly with\n> > case sensitive subsection names, which is why this went unnoticed in\n> > 6ae996f2acf (git_config_set: make use of the config parser's event\n> > stream, 2018-04-09)\n>\n> s/unnoticed in \\(.*04-09)\\)/unnoticed when \\1 broke it./\n>\n> This is why I asked if the patch is a \"FIX\" for an issue introduced\n> by the cited commit.\n\nI did not check further down the history if it was a recent brakage.\n\n> > static int get_base_var(struct strbuf *name)\n> > {\n> > + cf->section_name_old_dot_style = 1;\n> > for (;;) {\n> > int c = get_next_char();\n> > if (cf->eof)\n>\n> OK, let me rephrase. The basic parse structure is that\n>\n> * upon seeing '[', we call get_base_var(), which stuffs the\n> \"section\" (including subsection, if exists) in the strbuf.\n>\n> * get_base_var() upon seeing a space after \"[section \", calls\n> get_extended_base_var(). This space can never exist in an\n> old-style three-level names, where it is spelled as\n> \"[section.subsection]\". This space cannot exist in two-level\n> names, either. The closing ']' is eaten by this function before\n> it returns.\n>\n> * get_extended_base_var() grabs the \"double quoted\" subsection name\n> and eats the closing ']' before it returns.\n>\n> So you set the new bit (section_name_old_dot_style) at the beginning\n> of get_base_var(), i.e. declare that you assume we are reading old\n> style, but upon entering get_extended_base_var(), unset it, because\n> now you know we are parsing a modern style three-level name(s).\n>\n> Feels quite sensible way to keep track of old/new styles.\n>\n> When parsing two-level names, old-style bit is set, which we may\n> need to be careful, thoguh.\n\nI considered setting it only when seeing the dot, but then we'd have\nto make sure it is properly initialized.\n\nAnd *technically* the two level is old style, so I figured it's ok.\n\n> > - !strncasecmp(cf->var.buf, store->key, store->baselen);\n> > + !cmpfn(cf->var.buf, store->key, store->baselen);\n>\n> OK. Section names should still be case insensitive (only the case\n> sensitivity of subsection names is special), but presumably that's\n> already normalized by the caller so we do not have to worry when we\n> use strncmp()? Can we add a test to demonstrate that it works\n> correctly?\n\nThat was already demonstrated (but not tested) in\nhttps://public-inbox.org/git/20180730230443.74416-4-sbeller@google.com/\n"},{"id":"354341","messageId":"20180803003417.76153-1-sbeller@google.com","threadId":"48965","inReplyTo":"20180801193413.146994-1-sbeller@google.com","subject":"[PATCH 0/3] Reroll of sb/config-write-fix","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-03T00:34:14Z","receivedAt":"2018-08-03T00:34:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Only fix was in the commit message of the second patch:\n\n2: c667e555066 ! 1749: 38e5f6f335d config: fix case sensitive subsection names on writing\n @@ -2,8 +2,8 @@\n \n config: fix case sensitive subsection names on writing\n \n - A use reported a submodule issue regarding strange case indentation\n - issues, but it could be boiled down to the following test case:\n + A user reported a submodule issue regarding a section mix-up,\n + but it could be boiled down to the following test case:\n\nprevious version at\nhttps://public-inbox.org/git/20180801193413.146994-1-sbeller@google.com/\n\nStefan Beller (3):\n t1300: document current behavior of setting options\n config: fix case sensitive subsection names on writing\n git-config: document accidental multi-line setting in deprecated\n syntax\n\n Documentation/git-config.txt | 21 +++++++++\n config.c | 12 ++++-\n t/t1300-config.sh | 87 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 119 insertions(+), 1 deletion(-)\n\n-- \n2.18.0.132.g195c49a2227\n\n"},{"id":"354342","messageId":"20180803003417.76153-2-sbeller@google.com","threadId":"48965","inReplyTo":"20180803003417.76153-1-sbeller@google.com","subject":"[PATCH 1/3] t1300: document current behavior of setting options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-03T00:34:15Z","receivedAt":"2018-08-03T00:34:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This documents current behavior of the config machinery, when changing\nthe value of some settings. This patch just serves to provide a baseline\nfor the follow up that will fix some issues with the current behavior.\n\nSigned-off-by: Stefan Beller \n---\n t/t1300-config.sh | 86 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 86 insertions(+)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 24706ba4125..e87cfc1804f 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1218,6 +1218,92 @@ test_expect_success 'last one wins: three level vars' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'old-fashioned settings are case insensitive' '\n+\ttest_when_finished \"rm -f testConfig testConfig_expect testConfig_actual\" &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V.A]\n+\t\tQr = value2\n+\tEOF\n+\tgit config -f testConfig_actual \"v.a.r\" value2 &&\n+\ttest_cmp testConfig_expect testConfig_actual &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V.A]\n+\t\tQR = value2\n+\tEOF\n+\tgit config -f testConfig_actual \"V.a.R\" value2 &&\n+\ttest_cmp testConfig_expect testConfig_actual &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\t\tQr = value2\n+\tEOF\n+\tgit config -f testConfig_actual \"V.A.r\" value2 &&\n+\ttest_cmp testConfig_expect testConfig_actual &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V.A]\n+\t\tr = value1\n+\t\tQr = value2\n+\tEOF\n+\tgit config -f testConfig_actual \"v.A.r\" value2 &&\n+\ttest_cmp testConfig_expect testConfig_actual\n+'\n+\n+test_expect_success 'setting different case sensitive subsections ' '\n+\ttest_when_finished \"rm -f testConfig testConfig_expect testConfig_actual\" &&\n+\n+\tcat >testConfig_actual <<-EOF &&\n+\t\t[V \"A\"]\n+\t\tR = v1\n+\t\t[K \"E\"]\n+\t\tY = v1\n+\t\t[a \"b\"]\n+\t\tc = v1\n+\t\t[d \"e\"]\n+\t\tf = v1\n+\tEOF\n+\tq_to_tab >testConfig_expect <<-EOF &&\n+\t\t[V \"A\"]\n+\t\tQr = v2\n+\t\t[K \"E\"]\n+\t\tQy = v2\n+\t\t[a \"b\"]\n+\t\tQc = v2\n+\t\t[d \"e\"]\n+\t\tf = v1\n+\t\tQf = v2\n+\tEOF\n+\t# exact match\n+\tgit config -f testConfig_actual a.b.c v2 &&\n+\t# match section and subsection, key is cased differently.\n+\tgit config -f testConfig_actual K.E.y v2 &&\n+\t# section and key are matched case insensitive, but subsection needs\n+\t# to match; When writing out new values only the key is adjusted\n+\tgit config -f testConfig_actual v.A.r v2 &&\n+\t# subsection is not matched:\n+\tgit config -f testConfig_actual d.E.f v2 &&\n+\ttest_cmp testConfig_expect testConfig_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-- \n2.18.0.132.g195c49a2227\n\n"},{"id":"354343","messageId":"20180803003417.76153-3-sbeller@google.com","threadId":"48965","inReplyTo":"20180803003417.76153-1-sbeller@google.com","subject":"[PATCH 2/3] config: fix case sensitive subsection names on writing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-03T00:34:16Z","receivedAt":"2018-08-03T00:34:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"A user reported a submodule issue regarding a section mix-up,\nbut it could be boiled down to the following test case:\n\n $ git init test && cd test\n $ git config foo.\"Bar\".key test\n $ git config foo.\"bar\".key test\n $ tail -n 3 .git/config\n [foo \"Bar\"]\n key = test\n key = test\n\nSub sections are case sensitive and we have a test for correctly reading\nthem. However we do not have a test for writing out config correctly with\ncase sensitive subsection names, which is why this went unnoticed in\n6ae996f2acf (git_config_set: make use of the config parser's event\nstream, 2018-04-09)\n\nUnfortunately we have to make a distinction between old style configuration\nthat looks like\n\n [foo.Bar]\n key = test\n\nand the new quoted style as seen above. The old style is documented as\ncase-agnostic, hence we need to keep 'strncasecmp'; although the\nresulting setting for the old style config differs from the configuration.\nThat will be fixed in a follow up patch.\n\nReported-by: JP Sugarbroad \nSigned-off-by: Stefan Beller \n---\n config.c | 12 +++++++++++-\n t/t1300-config.sh | 1 +\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 66645047eb3..ffc2ffeafeb 100644\n--- a/config.c\n+++ b/config.c\n@@ -37,6 +37,7 @@ struct config_source {\n \tint eof;\n \tstruct strbuf value;\n \tstruct strbuf var;\n+\tunsigned section_name_old_dot_style : 1;\n \n \tint (*do_fgetc)(struct config_source *c);\n \tint (*do_ungetc)(int c, struct config_source *conf);\n@@ -605,6 +606,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n \n static int get_extended_base_var(struct strbuf *name, int c)\n {\n+\tcf->section_name_old_dot_style = 0;\n \tdo {\n \t\tif (c == '\\n')\n \t\t\tgoto error_incomplete_line;\n@@ -641,6 +643,7 @@ static int get_extended_base_var(struct strbuf *name, int c)\n \n static int get_base_var(struct strbuf *name)\n {\n+\tcf->section_name_old_dot_style = 1;\n \tfor (;;) {\n \t\tint c = get_next_char();\n \t\tif (cf->eof)\n@@ -2364,14 +2367,21 @@ static int store_aux_event(enum config_event_t type,\n \tstore->parsed[store->parsed_nr].type = type;\n \n \tif (type == CONFIG_EVENT_SECTION) {\n+\t\tint (*cmpfn)(const char *, const char *, size_t);\n+\n \t\tif (cf->var.len < 2 || cf->var.buf[cf->var.len - 1] != '.')\n \t\t\treturn error(\"invalid section name '%s'\", cf->var.buf);\n \n+\t\tif (cf->section_name_old_dot_style)\n+\t\t\tcmpfn = strncasecmp;\n+\t\telse\n+\t\t\tcmpfn = strncmp;\n+\n \t\t/* Is this the section we were looking for? */\n \t\tstore->is_keys_section =\n \t\t\tstore->parsed[store->parsed_nr].is_keys_section =\n \t\t\tcf->var.len - 1 == store->baselen &&\n-\t\t\t!strncasecmp(cf->var.buf, store->key, store->baselen);\n+\t\t\t!cmpfn(cf->var.buf, store->key, store->baselen);\n \t\tif (store->is_keys_section) {\n \t\t\tstore->section_seen = 1;\n \t\t\tALLOC_GROW(store->seen, store->seen_nr + 1,\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex e87cfc1804f..4976e2fcd3f 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1290,6 +1290,7 @@ test_expect_success 'setting different case sensitive subsections ' '\n \t\tQc = v2\n \t\t[d \"e\"]\n \t\tf = v1\n+\t\t[d \"E\"]\n \t\tQf = v2\n \tEOF\n \t# exact match\n-- \n2.18.0.132.g195c49a2227\n\n"},{"id":"354344","messageId":"20180803003417.76153-4-sbeller@google.com","threadId":"48965","inReplyTo":"20180803003417.76153-1-sbeller@google.com","subject":"[PATCH 3/3] git-config: document accidental multi-line setting in deprecated syntax","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-03T00:34:17Z","receivedAt":"2018-08-03T00:34:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The bug was noticed when writing the previous patch; a fix for this bug\nis not easy though: If we choose to ignore the case of the subsection\n(and revert most of the code of the previous patch, just keeping\ns/strncasecmp/strcmp/), then we'd introduce new sections using the\nnew syntax, such that\n\n --------\n [section.subsection]\n key = value1\n --------\n\n git config section.Subsection.key value2\n\nwould result in\n\n --------\n [section.subsection]\n key = value1\n [section.Subsection]\n key = value2\n --------\n\nwhich is even more confusing. A proper fix would replace the first\noccurrence of 'key'. As the syntax is deprecated, let's prefer to not\nspend time on fixing the behavior and just document it instead.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/git-config.txt | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 18ddc78f42d..8e240435bee 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -453,6 +453,27 @@ http.sslverify false\n \n include::config.txt[]\n \n+BUGS\n+----\n+When using the deprecated `[section.subsection]` syntax, changing a value\n+will result in adding a multi-line key instead of a change, if the subsection\n+is given with at least one uppercase character. For example when the config\n+looks like\n+\n+--------\n+ [section.subsection]\n+ key = value1\n+--------\n+\n+and running `git config section.Subsection.key value2` will result in\n+\n+--------\n+ [section.subsection]\n+ key = value1\n+ key = value2\n+--------\n+\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\n-- \n2.18.0.132.g195c49a2227\n\n"},{"id":"354396","messageId":"xmqqy3dnzbx7.fsf@gitster-ct.c.googlers.com","threadId":"48965","inReplyTo":"CAGZ79kbeUHVQmfTFkoGEM617iVa_3mCsLjpYGoEebqxNKAM_xA@mail.gmail.com","subject":"Re: [PATCH 2/3] config: fix case sensitive subsection names on writing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-03T15:51:00Z","receivedAt":"2018-08-03T15:51:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> And *technically* the two level is old style, so I figured it's ok.\n\nIf we call the bit not after the recentness of the style but after\nwhat it is about, e.g. \"is the section name as a whole (including\nits possible subsection part) case insensitive?\", then yes, a\ntwo-level name can be treated exactly the same way as the old style\nnames with a subsection. Perhaps we should do that rename to save\nus from future confusion.\n\n>> > - !strncasecmp(cf->var.buf, store->key, store->baselen);\n>> > + !cmpfn(cf->var.buf, store->key, store->baselen);\n>>\n>> OK. Section names should still be case insensitive (only the case\n>> sensitivity of subsection names is special), but presumably that's\n>> already normalized by the caller so we do not have to worry when we\n>> use strncmp()? Can we add a test to demonstrate that it works\n>> correctly?\n>\n> That was already demonstrated (but not tested) in\n> https://public-inbox.org/git/20180730230443.74416-4-sbeller@google.com/\n\nYeah, I also manually tried when I was playing with the old-style\nnames to see how well it works. We would want to make sure this\nwon't get broken in the future, still.\n\nThanks.\n"}]}