* [PATCH] config: fix case sensitive subsection names on writing @ 2018-07-27 20:51 Stefan Beller 2018-07-27 21:21 ` Brandon Williams 2018-07-27 21:37 ` [PATCH] config: fix case sensitive subsection names on writing Junio C Hamano 0 siblings, 2 replies; 33+ messages in thread From: Stefan Beller @ 2018-07-27 20:51 UTC (permalink / raw) To: git; +Cc: Stefan Beller A use reported a submodule issue regarding strange case indentation issues, but it could be boiled down to the following test case: $ git init test && cd test $ git config foo."Bar".key test $ git config foo."bar".key test $ tail -n 3 .git/config [foo "Bar"] key = test key = test Sub sections are case sensitive and we have a test for correctly reading them. However we do not have a test for writing out config correctly with case sensitive subsection names, which is why this went unnoticed in 6ae996f2acf (git_config_set: make use of the config parser's event stream, 2018-04-09) Make the subsection case sensitive and provide a test for both reading and writing. Reported-by: JP Sugarbroad <jpsugar@google.com> Signed-off-by: Stefan Beller <sbeller@google.com> --- config.c | 2 +- t/t1300-config.sh | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index 3aacddfec4b..3ded92b678b 100644 --- a/config.c +++ b/config.c @@ -2374,7 +2374,7 @@ static int store_aux_event(enum config_event_t type, store->is_keys_section = store->parsed[store->parsed_nr].is_keys_section = cf->var.len - 1 == store->baselen && - !strncasecmp(cf->var.buf, store->key, store->baselen); + !strncmp(cf->var.buf, store->key, store->baselen); if (store->is_keys_section) { store->section_seen = 1; ALLOC_GROW(store->seen, store->seen_nr + 1, diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 03c223708eb..8325d4495f4 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1218,6 +1218,24 @@ test_expect_success 'last one wins: three level vars' ' test_cmp expect actual ' +test_expect_success 'setting different case subsections ' ' + test_when_finished "rm -f caseSens caseSens_actual caseSens_expect" && + + # v.a.r and v.A.r are not the same variable, as the middle + # level of a three-level configuration variable name is + # case sensitive. + git config -f caseSens v."A".r VAL && + git config -f caseSens v."a".r val && + + echo VAL >caseSens_expect && + git config -f caseSens v."A".r >caseSens_actual && + test_cmp caseSens_expect caseSens_actual && + + echo val >caseSens_expect && + git config -f caseSens v."a".r >caseSens_actual && + test_cmp caseSens_expect caseSens_actual +' + for VAR in a .a a. a.0b a."b c". a."b c".0d do test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" ' -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] config: fix case sensitive subsection names on writing 2018-07-27 20:51 [PATCH] config: fix case sensitive subsection names on writing Stefan Beller @ 2018-07-27 21:21 ` Brandon Williams 2018-07-27 21:39 ` Junio C Hamano 2018-07-27 21:37 ` [PATCH] config: fix case sensitive subsection names on writing Junio C Hamano 1 sibling, 1 reply; 33+ messages in thread From: Brandon Williams @ 2018-07-27 21:21 UTC (permalink / raw) To: Stefan Beller; +Cc: git On 07/27, Stefan Beller wrote: > A use reported a submodule issue regarding strange case indentation > issues, but it could be boiled down to the following test case: > > $ git init test && cd test > $ git config foo."Bar".key test > $ git config foo."bar".key test > $ tail -n 3 .git/config > [foo "Bar"] > key = test > key = test > > Sub sections are case sensitive and we have a test for correctly reading > them. However we do not have a test for writing out config correctly with > case sensitive subsection names, which is why this went unnoticed in > 6ae996f2acf (git_config_set: make use of the config parser's event > stream, 2018-04-09) > > Make the subsection case sensitive and provide a test for both reading > and writing. > > Reported-by: JP Sugarbroad <jpsugar@google.com> > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > config.c | 2 +- > t/t1300-config.sh | 18 ++++++++++++++++++ > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/config.c b/config.c > index 3aacddfec4b..3ded92b678b 100644 > --- a/config.c > +++ b/config.c > @@ -2374,7 +2374,7 @@ static int store_aux_event(enum config_event_t type, > store->is_keys_section = > store->parsed[store->parsed_nr].is_keys_section = > cf->var.len - 1 == store->baselen && > - !strncasecmp(cf->var.buf, store->key, store->baselen); > + !strncmp(cf->var.buf, store->key, store->baselen); I've done some work in the config part of our codebase but I don't really know whats going on here due to two things: this is a callback function and it relies on global state... I can say though that we might want to be careful about completely converting this to a case sensitive compare. Our config is a key value store with the following format: 'section.subsection.key'. IIRC both section and key are always compared case insensitively. The subsection can be case sensitive or insensitive based on how its stored in the config files itself: # Case insensitive [section.subsection] key = value # Case sensitive [section "subsection"] key = value But I don't know how you distinguish between these cases when a config is specified on a single line (section.subsection.key). Do we always assume the sensitive version because the insensitive version is documented to be deprecated? Either way you're probably going to need to be careful about how you do string comparison against the different parts. > if (store->is_keys_section) { > store->section_seen = 1; > ALLOC_GROW(store->seen, store->seen_nr + 1, > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > index 03c223708eb..8325d4495f4 100755 > --- a/t/t1300-config.sh > +++ b/t/t1300-config.sh > @@ -1218,6 +1218,24 @@ test_expect_success 'last one wins: three level vars' ' > test_cmp expect actual > ' > > +test_expect_success 'setting different case subsections ' ' > + test_when_finished "rm -f caseSens caseSens_actual caseSens_expect" && > + > + # v.a.r and v.A.r are not the same variable, as the middle > + # level of a three-level configuration variable name is > + # case sensitive. > + git config -f caseSens v."A".r VAL && > + git config -f caseSens v."a".r val && > + > + echo VAL >caseSens_expect && > + git config -f caseSens v."A".r >caseSens_actual && > + test_cmp caseSens_expect caseSens_actual && > + > + echo val >caseSens_expect && > + git config -f caseSens v."a".r >caseSens_actual && > + test_cmp caseSens_expect caseSens_actual > +' > + > for VAR in a .a a. a.0b a."b c". a."b c".0d > do > test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" ' > -- > 2.18.0.345.g5c9ce644c3-goog > -- Brandon Williams ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] config: fix case sensitive subsection names on writing 2018-07-27 21:21 ` Brandon Williams @ 2018-07-27 21:39 ` Junio C Hamano 2018-07-27 23:35 ` Stefan Beller 0 siblings, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2018-07-27 21:39 UTC (permalink / raw) To: Brandon Williams; +Cc: Stefan Beller, git Brandon Williams <bmwill@google.com> writes: > Either way you're probably going to need to be careful about how you do > string comparison against the different parts. Good suggestion. >> diff --git a/t/t1300-config.sh b/t/t1300-config.sh >> index 03c223708eb..8325d4495f4 100755 >> --- a/t/t1300-config.sh >> +++ b/t/t1300-config.sh >> @@ -1218,6 +1218,24 @@ test_expect_success 'last one wins: three level vars' ' >> test_cmp expect actual >> ' >> >> +test_expect_success 'setting different case subsections ' ' >> + test_when_finished "rm -f caseSens caseSens_actual caseSens_expect" && >> + >> + # v.a.r and v.A.r are not the same variable, as the middle >> + # level of a three-level configuration variable name is >> + # case sensitive. In other words, perhaps add # "V.a.r" and "v.a.R" are the same variable, though and corresponding test here? >> + git config -f caseSens v."A".r VAL && >> + git config -f caseSens v."a".r val && >> + >> + echo VAL >caseSens_expect && >> + git config -f caseSens v."A".r >caseSens_actual && >> + test_cmp caseSens_expect caseSens_actual && >> + >> + echo val >caseSens_expect && >> + git config -f caseSens v."a".r >caseSens_actual && >> + test_cmp caseSens_expect caseSens_actual >> +' >> + >> for VAR in a .a a. a.0b a."b c". a."b c".0d >> do >> test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" ' >> -- >> 2.18.0.345.g5c9ce644c3-goog >> ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] config: fix case sensitive subsection names on writing 2018-07-27 21:39 ` Junio C Hamano @ 2018-07-27 23:35 ` Stefan Beller 2018-07-27 23:36 ` Stefan Beller 0 siblings, 1 reply; 33+ messages in thread From: Stefan Beller @ 2018-07-27 23:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: Brandon Williams, git On Fri, Jul 27, 2018 at 2:39 PM Junio C Hamano <gitster@pobox.com> wrote: > > Brandon Williams <bmwill@google.com> writes: > > > Either way you're probably going to need to be careful about how you do > > string comparison against the different parts. > > Good suggestion. The suggestion is a rabit hole and was a waste of time. However I did some more manual testing and inspected the code with trace_printf debugging, and it turns out the strings compared are brought into the correct form already. > >> + # v.a.r and v.A.r are not the same variable, as the middle > >> + # level of a three-level configuration variable name is > >> + # case sensitive. > > In other words, perhaps add > > # "V.a.r" and "v.a.R" are the same variable, though > > and corresponding test here? I removed that section and went for a shorter, more concise expression. patch to follow. ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] config: fix case sensitive subsection names on writing 2018-07-27 23:35 ` Stefan Beller @ 2018-07-27 23:36 ` Stefan Beller 2018-07-27 23:37 ` Stefan Beller ` (2 more replies) 0 siblings, 3 replies; 33+ messages in thread From: Stefan Beller @ 2018-07-27 23:36 UTC (permalink / raw) To: git; +Cc: bmwill, peff, Johannes.Schindelin, gitster, Stefan Beller A use reported a submodule issue regarding strange case indentation issues, but it could be boiled down to the following test case: $ git init test && cd test $ git config foo."Bar".key test $ git config foo."bar".key test $ tail -n 3 .git/config [foo "Bar"] key = test key = test Sub sections are case sensitive and we have a test for correctly reading them. However we do not have a test for writing out config correctly with case sensitive subsection names, which is why this went unnoticed in 6ae996f2acf (git_config_set: make use of the config parser's event stream, 2018-04-09) Make the subsection case sensitive and provide a test for writing. The test for reading is just above this test. Reported-by: JP Sugarbroad <jpsugar@google.com> Signed-off-by: Stefan Beller <sbeller@google.com> --- I really appreciate the work by DScho (and Peff as I recall him as an active reviewer there) on 4f4d0b42bae (Merge branch 'js/empty-config-section-fix', 2018-05-08), as the corner cases are all correct, modulo the one line fix in this patch. Thanks, Stefan config.c | 2 +- t/t1300-config.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index 7968ef7566a..de646d2c56f 100644 --- a/config.c +++ b/config.c @@ -2362,7 +2362,7 @@ static int store_aux_event(enum config_event_t type, store->is_keys_section = store->parsed[store->parsed_nr].is_keys_section = cf->var.len - 1 == store->baselen && - !strncasecmp(cf->var.buf, store->key, store->baselen); + !strncmp(cf->var.buf, store->key, store->baselen); if (store->is_keys_section) { store->section_seen = 1; ALLOC_GROW(store->seen, store->seen_nr + 1, diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 03c223708eb..710e2b04ad8 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1218,6 +1218,63 @@ test_expect_success 'last one wins: three level vars' ' test_cmp expect actual ' +test_expect_success 'old-fashioned settings are case insensitive' ' + test_when_finished "rm -f testConfig testConfig_expect testConfig_actual" && + + cat >testConfig <<-EOF && + # insensitive: + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + # insensitive: + [V.A] + Qr = value2 + EOF + + for key in "v.a.r" "V.A.r" "v.A.r" "V.a.r" + do + cp testConfig testConfig_actual && + git config -f testConfig_actual v.a.r value2 && + test_cmp testConfig_expect testConfig_actual + done +' + +test_expect_success 'setting different case sensitive subsections ' ' + test_when_finished "rm -f testConfig testConfig_expect testConfig_actual" && + + cat >testConfig <<-EOF && + # V insensitive A sensitive: + [V "A"] + r = value1 + # same as above: + [V "a"] + r = value2 + EOF + + git config -f testConfig v.a.r value3 && + q_to_tab >testConfig_expect <<-EOF && + # V insensitive A sensitive: + [V "A"] + r = value1 + # same as above: + [V "a"] + Qr = value3 + EOF + test_cmp testConfig_expect testConfig && + + git config -f testConfig v.A.r value4 && + q_to_tab >testConfig_expect <<-EOF && + # V insensitive A sensitive: + [V "A"] + Qr = value4 + # same as above: + [V "a"] + Qr = value3 + EOF + test_cmp testConfig_expect testConfig +' + for VAR in a .a a. a.0b a."b c". a."b c".0d do test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" ' -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] config: fix case sensitive subsection names on writing 2018-07-27 23:36 ` Stefan Beller @ 2018-07-27 23:37 ` Stefan Beller 2018-07-28 1:01 ` Junio C Hamano 2018-07-28 1:37 ` Junio C Hamano 2 siblings, 0 replies; 33+ messages in thread From: Stefan Beller @ 2018-07-27 23:37 UTC (permalink / raw) To: git, Jeff King; +Cc: Brandon Williams, Johannes Schindelin, Junio C Hamano I cc'd the wrong peff. Sorry about that. On Fri, Jul 27, 2018 at 4:36 PM Stefan Beller <sbeller@google.com> wrote: .... ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] config: fix case sensitive subsection names on writing 2018-07-27 23:36 ` Stefan Beller 2018-07-27 23:37 ` Stefan Beller @ 2018-07-28 1:01 ` Junio C Hamano 2018-07-28 3:52 ` Stefan Beller 2018-07-28 1:37 ` Junio C Hamano 2 siblings, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2018-07-28 1:01 UTC (permalink / raw) To: Stefan Beller; +Cc: git, bmwill, peff, Johannes.Schindelin Stefan Beller <sbeller@google.com> writes: > A use reported a submodule issue regarding strange case indentation s/use/&r/; Is this "indentation" issue? > issues, but it could be boiled down to the following test case: > ... > +test_expect_success 'old-fashioned settings are case insensitive' ' > + test_when_finished "rm -f testConfig testConfig_expect testConfig_actual" && > + > + cat >testConfig <<-EOF && > + # insensitive: > + [V.A] > + r = value1 > + EOF > + q_to_tab >testConfig_expect <<-EOF && > + # insensitive: > + [V.A] > + Qr = value2 > + EOF It is unfortunate that we hardcode the exact indentation in the test to make it care. Perhaps a wrapper around test_cmp that is used locally in this file to first strip the leading HT from both sides of the comparison would make it more robust? > + for key in "v.a.r" "V.A.r" "v.A.r" "V.a.r" > + do > + cp testConfig testConfig_actual && > + git config -f testConfig_actual v.a.r value2 && > + test_cmp testConfig_expect testConfig_actual > + done > +' I think you meant to use "$key" when setting the variable to value2. When the test_cmp fails with "v.a.r" but later succeeds and most importantly succeeds with "V.a.r" (i.e. the last one), wouldn't the whole thing suceed? I think the common trick people use is to end the last one with "|| return 1" in a loop inside test_expect_success. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] config: fix case sensitive subsection names on writing 2018-07-28 1:01 ` Junio C Hamano @ 2018-07-28 3:52 ` Stefan Beller 2018-07-28 10:53 ` Jeff King 0 siblings, 1 reply; 33+ messages in thread From: Stefan Beller @ 2018-07-28 3:52 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Brandon Williams, peff, Johannes Schindelin On Fri, Jul 27, 2018 at 6:01 PM Junio C Hamano <gitster@pobox.com> wrote: > > Stefan Beller <sbeller@google.com> writes: > > > A use reported a submodule issue regarding strange case indentation > > s/use/&r/; Is this "indentation" issue? eh case sensitivity* > > + q_to_tab >testConfig_expect <<-EOF && > > + # insensitive: > > + [V.A] > > + Qr = value2 > > + EOF > > It is unfortunate that we hardcode the exact indentation > in the test to make it care. Perhaps a wrapper around test_cmp that > is used locally in this file to first strip the leading HT from both > sides of the comparison would make it more robust? I think this is valuable for the second test to see where it was replaced. > > > + for key in "v.a.r" "V.A.r" "v.A.r" "V.a.r" > > + do > > + cp testConfig testConfig_actual && > > + git config -f testConfig_actual v.a.r value2 && > > + test_cmp testConfig_expect testConfig_actual > > + done > > +' > > I think you meant to use "$key" when setting the variable to value2. > > When the test_cmp fails with "v.a.r" but later succeeds and most > importantly succeeds with "V.a.r" (i.e. the last one), wouldn't the > whole thing suceed? I think the common trick people use is to end > the last one with "|| return 1" in a loop inside test_expect_success. > Hah, of course this patch is not as easy. The problem is in git_parse_source (config.c, line 755) when we call get_base_var it normalizes both styles, (and lower cases the dot style). So in case of dot style, the strncasecmp is correct, but for the new extended style we need to strncmp. So this is correct for extended but not any more for the former dot style. I wonder if we want to either (a) add another CONFIG_EVENT_SECTION that indicates the difference between the two, or have a flag in 'cf' that tells us what kind of section style we have, such that we can check appropriately for the case. Or we could encode it in the 'cf->var' value to distinguish the old and new style. Thanks, Stefan ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] config: fix case sensitive subsection names on writing 2018-07-28 3:52 ` Stefan Beller @ 2018-07-28 10:53 ` Jeff King 0 siblings, 0 replies; 33+ messages in thread From: Jeff King @ 2018-07-28 10:53 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, git, Brandon Williams, peff, Johannes Schindelin On Fri, Jul 27, 2018 at 08:52:59PM -0700, Stefan Beller wrote: > > > + for key in "v.a.r" "V.A.r" "v.A.r" "V.a.r" > > > + do > > > + cp testConfig testConfig_actual && > > > + git config -f testConfig_actual v.a.r value2 && > > > + test_cmp testConfig_expect testConfig_actual > > > + done > > > +' > > > > I think you meant to use "$key" when setting the variable to value2. > > > > When the test_cmp fails with "v.a.r" but later succeeds and most > > importantly succeeds with "V.a.r" (i.e. the last one), wouldn't the > > whole thing suceed? I think the common trick people use is to end > > the last one with "|| return 1" in a loop inside test_expect_success. > > Hah, of course this patch is not as easy. > > The problem is in git_parse_source (config.c, line 755) when we call > get_base_var it normalizes both styles, (and lower cases the dot style). > > So in case of dot style, the strncasecmp is correct, but for the new > extended style we need to strncmp. > > So this is correct for extended but not any more for the former dot style. Hmm, it looks like this has always been broken. Before your patch (but after v2.18.0), running "git config v.A.r value2" writes: [V.A] r = value1 r = value2 which is obviously broken (we decided it was the right section, but didn't match the variables, leading to a weird duplicate). After your patch we write: [V.A] r = value1 [v "A"] r = value2 I.e., we do not consider them the same value at all. But that's actually what happened before v2.18, too. I think you could almost justify that behavior as "The section.subsection syntax is deprecated, so we match it case-insensitively on reading but not on writing; since we never write such sections ourselves, you are on your own for modifying such entries if you choose to write them manually". I dunno. Maybe that is too harsh. But this syntax has been deprecated for 7 years, and nobody has noticed the bug until now (when _still_ nobody wants to use it, but rather we're poking at it as "gee, I wonder if this even works"). > I wonder if we want to either (a) add another CONFIG_EVENT_SECTION > that indicates the difference between the two, or have a flag in 'cf' that > tells us what kind of section style we have, such that we can check > appropriately for the case. I'd think that the parse_event_data could carry type-specific information. But... > Or we could encode it in the 'cf->var' value to distinguish the old > and new style. Even if we fix the section-matching here, I suspect that would just trigger a separate bug later when we match the full key (so we might add to the correct section, but we would not correctly replace an existing key). To fix that, the information does need to be carried for the whole lifetime of cf->var, not just during the header event. -Peff ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] config: fix case sensitive subsection names on writing 2018-07-27 23:36 ` Stefan Beller 2018-07-27 23:37 ` Stefan Beller 2018-07-28 1:01 ` Junio C Hamano @ 2018-07-28 1:37 ` Junio C Hamano 2018-07-30 12:49 ` Johannes Schindelin 2 siblings, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2018-07-28 1:37 UTC (permalink / raw) To: Stefan Beller; +Cc: git, bmwill, peff, Johannes.Schindelin Stefan Beller <sbeller@google.com> writes: > I really appreciate the work by DScho (and Peff as I recall him as an active > reviewer there) on 4f4d0b42bae (Merge branch 'js/empty-config-section-fix', > 2018-05-08), as the corner cases are all correct, modulo the one line fix > in this patch. Amen to the early part of that ;--) Even though it was merely cosmetic, and did not affect correctness, that longstandng bug was very annoying bug to a lot of people. I too am very happy to see it disappear. I would still hold the judgment on "all except only this one" myself. That's a bit too early in my mind. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] config: fix case sensitive subsection names on writing 2018-07-28 1:37 ` Junio C Hamano @ 2018-07-30 12:49 ` Johannes Schindelin 2018-07-30 23:04 ` [PATCH 0/3] " Stefan Beller 0 siblings, 1 reply; 33+ messages in thread From: Johannes Schindelin @ 2018-07-30 12:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Stefan Beller, git, bmwill, peff Hi, On Fri, 27 Jul 2018, Junio C Hamano wrote: > Stefan Beller <sbeller@google.com> writes: > > [...] Thanks for the patch! The only thing that was not clear to me from the patch and from the commit message was: the first part *is* case insensitive, right? How does the patch take care of that? Is it relying on `git_config_parse_key()` to do that? If so, I don't see it... > I would still hold the judgment on "all except only this one" > myself. That's a bit too early in my mind. Agreed. I seem to remember that I had a funny problem "in the reverse", where http.<url>.* is case-sensitive, but in an unexpected way: if the URL contains upper-case characters, the <url> part of the config key needs to be downcased, otherwise the setting won't be picked up. Ciao, Dscho ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 0/3] config: fix case sensitive subsection names on writing 2018-07-30 12:49 ` Johannes Schindelin @ 2018-07-30 23:04 ` Stefan Beller 2018-07-30 23:04 ` [PATCH 1/3] t1300: document current behavior of setting options Stefan Beller ` (3 more replies) 0 siblings, 4 replies; 33+ messages in thread From: Stefan Beller @ 2018-07-30 23:04 UTC (permalink / raw) To: johannes.schindelin; +Cc: bmwill, git, gitster, peff, sbeller On Mon, Jul 30, 2018 at 5:50 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > Thanks for the patch! > > The only thing that was not clear to me from the patch and from the commit > message was: the first part *is* case insensitive, right? right. > How does the > patch take care of that? Is it relying on `git_config_parse_key()` to do > that? If so, I don't see it... It turns out it doesn't quite do that; The parsing code takes the old notation into account and translates any [V.A] r = ... into a lower cased "v.a." for ease of comparison. That happens in get_base_var, which would call further into get_extended_base_var if the new notation is used. The code in store_aux_event however is written without the consideration of the old code and has no way of knowing the capitalization of the section or subsection (which were forced to lowercase in the old dot notation). So either we have to do some major surgery, or the old notation gets some regression while fixing the new notation. > > I would still hold the judgment on "all except only this one" > > myself. That's a bit too early in my mind. > > Agreed. I seem to remember that I had a funny problem "in the reverse", > where http.<url>.* is case-sensitive, but in an unexpected way: if the URL > contains upper-case characters, the <url> part of the config key needs to > be downcased, otherwise the setting won't be picked up. I wrote some patches that show more of what is happening. I suggest to drop the last patch and only take the first two, or if we decide we want to be fully correct, we'd want to discuss how to make it happen. (where to store the information that we are dealing with an old notation) Thanks, Stefan Stefan Beller (3): t1300: document current behavior of setting options config: fix case sensitive subsection names on writing config: treat section case insensitive in store_aux_event config.c | 16 ++++++++- t/t1300-config.sh | 89 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-) -- 2.18.0.132.g195c49a2227 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/3] t1300: document current behavior of setting options 2018-07-30 23:04 ` [PATCH 0/3] " Stefan Beller @ 2018-07-30 23:04 ` Stefan Beller 2018-07-30 23:04 ` [PATCH 2/3] config: fix case sensitive subsection names on writing Stefan Beller ` (2 subsequent siblings) 3 siblings, 0 replies; 33+ messages in thread From: Stefan Beller @ 2018-07-30 23:04 UTC (permalink / raw) To: johannes.schindelin; +Cc: bmwill, git, gitster, peff, sbeller This documents current behavior of the config machinery, when changing the value of some settings. This patch just serves to provide a baseline for the follow up that will fix some issues with the current behavior. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/t1300-config.sh | 86 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 03c223708eb..ced13012409 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1218,6 +1218,92 @@ test_expect_success 'last one wins: three level vars' ' test_cmp expect actual ' +test_expect_success 'old-fashioned settings are case insensitive' ' + test_when_finished "rm -f testConfig testConfig_expect testConfig_actual" && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + Qr = value2 + EOF + git config -f testConfig_actual "v.a.r" value2 && + test_cmp testConfig_expect testConfig_actual && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + QR = value2 + EOF + git config -f testConfig_actual "V.a.R" value2 && + test_cmp testConfig_expect testConfig_actual && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + r = value1 + Qr = value2 + EOF + git config -f testConfig_actual "V.A.r" value2 && + test_cmp testConfig_expect testConfig_actual && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + r = value1 + Qr = value2 + EOF + git config -f testConfig_actual "v.A.r" value2 && + test_cmp testConfig_expect testConfig_actual +' + +test_expect_success 'setting different case sensitive subsections ' ' + test_when_finished "rm -f testConfig testConfig_expect testConfig_actual" && + + cat >testConfig_actual <<-EOF && + [V "A"] + R = v1 + [K "E"] + Y = v1 + [a "b"] + c = v1 + [d "e"] + f = v1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V "A"] + Qr = v2 + [K "E"] + Qy = v2 + [a "b"] + Qc = v2 + [d "e"] + f = v1 + Qf = v2 + EOF + # exact match + git config -f testConfig_actual a.b.c v2 && + # match section and subsection, key is cased differently. + git config -f testConfig_actual K.E.y v2 && + # section and key are matched case insensitive, but subsection needs + # to match; When writing out new values only the key is adjusted + git config -f testConfig_actual v.A.r v2 && + # subsection is not matched: + git config -f testConfig_actual d.E.f v2 && + test_cmp testConfig_expect testConfig_actual +' + for VAR in a .a a. a.0b a."b c". a."b c".0d do test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" ' -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 2/3] config: fix case sensitive subsection names on writing 2018-07-30 23:04 ` [PATCH 0/3] " Stefan Beller 2018-07-30 23:04 ` [PATCH 1/3] t1300: document current behavior of setting options Stefan Beller @ 2018-07-30 23:04 ` Stefan Beller 2018-07-31 20:16 ` Junio C Hamano 2018-07-30 23:04 ` [PATCH 3/3] config: treat section case insensitive in store_aux_event Stefan Beller 2018-07-31 15:16 ` [PATCH 0/3] config: fix case sensitive subsection names on writing Junio C Hamano 3 siblings, 1 reply; 33+ messages in thread From: Stefan Beller @ 2018-07-30 23:04 UTC (permalink / raw) To: johannes.schindelin; +Cc: bmwill, git, gitster, peff, sbeller A use reported a submodule issue regarding strange case indentation issues, but it could be boiled down to the following test case: $ git init test && cd test $ git config foo."Bar".key test $ git config foo."bar".key test $ tail -n 3 .git/config [foo "Bar"] key = test key = test Sub sections are case sensitive and we have a test for correctly reading them. However we do not have a test for writing out config correctly with case sensitive subsection names, which is why this went unnoticed in 6ae996f2acf (git_config_set: make use of the config parser's event stream, 2018-04-09) Make the subsection case sensitive and provide a test for writing. The test for reading is just above this test. Reported-by: JP Sugarbroad <jpsugar@google.com> Signed-off-by: Stefan Beller <sbeller@google.com> --- config.c | 2 +- t/t1300-config.sh | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index 7968ef7566a..de646d2c56f 100644 --- a/config.c +++ b/config.c @@ -2362,7 +2362,7 @@ static int store_aux_event(enum config_event_t type, store->is_keys_section = store->parsed[store->parsed_nr].is_keys_section = cf->var.len - 1 == store->baselen && - !strncasecmp(cf->var.buf, store->key, store->baselen); + !strncmp(cf->var.buf, store->key, store->baselen); if (store->is_keys_section) { store->section_seen = 1; ALLOC_GROW(store->seen, store->seen_nr + 1, diff --git a/t/t1300-config.sh b/t/t1300-config.sh index ced13012409..e5d16f53ee1 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1250,6 +1250,7 @@ test_expect_success 'old-fashioned settings are case insensitive' ' q_to_tab >testConfig_expect <<-EOF && [V.A] r = value1 + [V "A"] Qr = value2 EOF git config -f testConfig_actual "V.A.r" value2 && @@ -1262,6 +1263,7 @@ test_expect_success 'old-fashioned settings are case insensitive' ' q_to_tab >testConfig_expect <<-EOF && [V.A] r = value1 + [v "A"] Qr = value2 EOF git config -f testConfig_actual "v.A.r" value2 && @@ -1290,6 +1292,7 @@ test_expect_success 'setting different case sensitive subsections ' ' Qc = v2 [d "e"] f = v1 + [d "E"] Qf = v2 EOF # exact match -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 2/3] config: fix case sensitive subsection names on writing 2018-07-30 23:04 ` [PATCH 2/3] config: fix case sensitive subsection names on writing Stefan Beller @ 2018-07-31 20:16 ` Junio C Hamano 0 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2018-07-31 20:16 UTC (permalink / raw) To: Stefan Beller; +Cc: johannes.schindelin, bmwill, git, peff Stefan Beller <sbeller@google.com> writes: > A use reported a submodule issue regarding strange case indentation > issues, but it could be boiled down to the following test case: > > $ git init test && cd test > $ git config foo."Bar".key test > $ git config foo."bar".key test > $ tail -n 3 .git/config > [foo "Bar"] > key = test > key = test > > Sub sections are case sensitive and we have a test for correctly reading > them. However we do not have a test for writing out config correctly with > case sensitive subsection names, which is why this went unnoticed in > 6ae996f2acf (git_config_set: make use of the config parser's event > stream, 2018-04-09) Am I correct to understand that this patch is a "FIX" for breakage introduced by that commit? The phrasing is not helping me to pick a good base to queue these patches on. Thanks. ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 3/3] config: treat section case insensitive in store_aux_event 2018-07-30 23:04 ` [PATCH 0/3] " Stefan Beller 2018-07-30 23:04 ` [PATCH 1/3] t1300: document current behavior of setting options Stefan Beller 2018-07-30 23:04 ` [PATCH 2/3] config: fix case sensitive subsection names on writing Stefan Beller @ 2018-07-30 23:04 ` Stefan Beller 2018-07-31 15:16 ` [PATCH 0/3] config: fix case sensitive subsection names on writing Junio C Hamano 3 siblings, 0 replies; 33+ messages in thread From: Stefan Beller @ 2018-07-30 23:04 UTC (permalink / raw) To: johannes.schindelin; +Cc: bmwill, git, gitster, peff, sbeller This is a no-op because the section names are lower-cased already in get_base_var, this is purely for demonstration that we do not need to care about case issues in this part of the code. Signed-off-by: Stefan Beller <sbeller@google.com> --- config.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index de646d2c56f..c247164ad17 100644 --- a/config.c +++ b/config.c @@ -2355,14 +2355,28 @@ static int store_aux_event(enum config_event_t type, store->parsed[store->parsed_nr].type = type; if (type == CONFIG_EVENT_SECTION) { + char *p; + int slen; /* section length */ if (cf->var.len < 2 || cf->var.buf[cf->var.len - 1] != '.') return error("invalid section name '%s'", cf->var.buf); + p = strchr(cf->var.buf, '.'); + if (!p) + /* no subsection, so treat all as section: */ + slen = store->baselen; + else + slen = p - cf->var.buf; + + if (slen > store->baselen) + slen = store->baselen; + /* Is this the section we were looking for? */ store->is_keys_section = store->parsed[store->parsed_nr].is_keys_section = cf->var.len - 1 == store->baselen && - !strncmp(cf->var.buf, store->key, store->baselen); + !strncasecmp(cf->var.buf, store->key, slen) && + !strncmp(cf->var.buf + slen, store->key + slen, + store->baselen - slen); if (store->is_keys_section) { store->section_seen = 1; ALLOC_GROW(store->seen, store->seen_nr + 1, -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 0/3] config: fix case sensitive subsection names on writing 2018-07-30 23:04 ` [PATCH 0/3] " Stefan Beller ` (2 preceding siblings ...) 2018-07-30 23:04 ` [PATCH 3/3] config: treat section case insensitive in store_aux_event Stefan Beller @ 2018-07-31 15:16 ` Junio C Hamano 2018-08-01 19:34 ` [PATCH 0/3] sb/config-write-fix done without robbing Peter Stefan Beller 3 siblings, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2018-07-31 15:16 UTC (permalink / raw) To: Stefan Beller; +Cc: johannes.schindelin, bmwill, git, peff Stefan Beller <sbeller@google.com> writes: > It turns out it doesn't quite do that; > The parsing code takes the old notation into account and translates any > [V.A] > r = ... > into a lower cased "v.a." for ease of comparison. That happens in > get_base_var, which would call further into get_extended_base_var > if the new notation is used. > > The code in store_aux_event however is written without the consideration > of the old code and has no way of knowing the capitalization of the > section or subsection (which were forced to lowercase in the old > dot notation). > > So either we have to do some major surgery, or the old notation gets > some regression while fixing the new notation. As long as - the regression is documented clearly (i.e. what unexpected thing happens, just like you have a good description in the log message of PATCH 2/3 for "[foo "Bar"] key = ..."), - users are nudged to use the new style instead, and - writing with "git config" (or "git init/clone" for that matter) won't produce these old-style sections I think it is OK to punt. I would expect, as Git's userbase is a lot wider than 10 years ago, there is at least some people who did exploit the fact that "[V.A] r = one" and "[v.a] r = two" give a single "v.a.r" variable that is multi-valued, and it indeed would be a regression to them, to which no good workaround exists. But breaking '[V "a"] r = ...' is a more grave sin. Even though I hate to rob Peter to pay Paul (or vice versa), in this case it is justified to fix the more basic form sooner and wait for an actual complaint before fixing the new and deliberate regression introduced while fixing it to the old-style variable. ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 0/3] sb/config-write-fix done without robbing Peter 2018-07-31 15:16 ` [PATCH 0/3] config: fix case sensitive subsection names on writing Junio C Hamano @ 2018-08-01 19:34 ` Stefan Beller 2018-08-01 19:34 ` [PATCH 1/3] t1300: document current behavior of setting options Stefan Beller ` (5 more replies) 0 siblings, 6 replies; 33+ messages in thread From: Stefan Beller @ 2018-08-01 19:34 UTC (permalink / raw) To: gitster; +Cc: bmwill, git, johannes.schindelin, peff, sbeller > Am I correct to understand that this patch is a "FIX" for breakage > introduced by that commit? The phrasing is not helping me to pick > a good base to queue these patches on. Please pick 4f4d0b42bae (Merge branch 'js/empty-config-section-fix', 2018-05-08) as the base of this new series (am needs -3 to apply), although I developed this series on origin/master. > Even though I hate to rob Peter to pay Paul (or vice versa) Yeah me, too. Here is a proper fix (i.e. only pay Paul, without the robbery), and a documentation of the second bug that we discovered. The first patch stands as is unchanged, and the second and third patch are different enough that range-diff doesn't want to show a diff. Thanks, Stefan Stefan Beller (3): t1300: document current behavior of setting options config: fix case sensitive subsection names on writing git-config: document accidental multi-line setting in deprecated syntax Documentation/git-config.txt | 21 +++++++++ config.c | 12 ++++- t/t1300-config.sh | 87 ++++++++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 1 deletion(-) ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/3] t1300: document current behavior of setting options 2018-08-01 19:34 ` [PATCH 0/3] sb/config-write-fix done without robbing Peter Stefan Beller @ 2018-08-01 19:34 ` Stefan Beller 2018-08-01 19:34 ` [PATCH 2/3] config: fix case sensitive subsection names on writing Stefan Beller ` (4 subsequent siblings) 5 siblings, 0 replies; 33+ messages in thread From: Stefan Beller @ 2018-08-01 19:34 UTC (permalink / raw) To: gitster; +Cc: bmwill, git, johannes.schindelin, peff, sbeller This documents current behavior of the config machinery, when changing the value of some settings. This patch just serves to provide a baseline for the follow up that will fix some issues with the current behavior. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/t1300-config.sh | 86 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 03c223708eb..ced13012409 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1218,6 +1218,92 @@ test_expect_success 'last one wins: three level vars' ' test_cmp expect actual ' +test_expect_success 'old-fashioned settings are case insensitive' ' + test_when_finished "rm -f testConfig testConfig_expect testConfig_actual" && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + Qr = value2 + EOF + git config -f testConfig_actual "v.a.r" value2 && + test_cmp testConfig_expect testConfig_actual && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + QR = value2 + EOF + git config -f testConfig_actual "V.a.R" value2 && + test_cmp testConfig_expect testConfig_actual && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + r = value1 + Qr = value2 + EOF + git config -f testConfig_actual "V.A.r" value2 && + test_cmp testConfig_expect testConfig_actual && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + r = value1 + Qr = value2 + EOF + git config -f testConfig_actual "v.A.r" value2 && + test_cmp testConfig_expect testConfig_actual +' + +test_expect_success 'setting different case sensitive subsections ' ' + test_when_finished "rm -f testConfig testConfig_expect testConfig_actual" && + + cat >testConfig_actual <<-EOF && + [V "A"] + R = v1 + [K "E"] + Y = v1 + [a "b"] + c = v1 + [d "e"] + f = v1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V "A"] + Qr = v2 + [K "E"] + Qy = v2 + [a "b"] + Qc = v2 + [d "e"] + f = v1 + Qf = v2 + EOF + # exact match + git config -f testConfig_actual a.b.c v2 && + # match section and subsection, key is cased differently. + git config -f testConfig_actual K.E.y v2 && + # section and key are matched case insensitive, but subsection needs + # to match; When writing out new values only the key is adjusted + git config -f testConfig_actual v.A.r v2 && + # subsection is not matched: + git config -f testConfig_actual d.E.f v2 && + test_cmp testConfig_expect testConfig_actual +' + for VAR in a .a a. a.0b a."b c". a."b c".0d do test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" ' -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 2/3] config: fix case sensitive subsection names on writing 2018-08-01 19:34 ` [PATCH 0/3] sb/config-write-fix done without robbing Peter Stefan Beller 2018-08-01 19:34 ` [PATCH 1/3] t1300: document current behavior of setting options Stefan Beller @ 2018-08-01 19:34 ` Stefan Beller 2018-08-01 21:01 ` Ramsay Jones 2018-08-01 22:51 ` Junio C Hamano 2018-08-01 19:34 ` [PATCH 3/3] git-config: document accidental multi-line setting in deprecated syntax Stefan Beller ` (3 subsequent siblings) 5 siblings, 2 replies; 33+ messages in thread From: Stefan Beller @ 2018-08-01 19:34 UTC (permalink / raw) To: gitster; +Cc: bmwill, git, johannes.schindelin, peff, sbeller A use reported a submodule issue regarding strange case indentation issues, but it could be boiled down to the following test case: $ git init test && cd test $ git config foo."Bar".key test $ git config foo."bar".key test $ tail -n 3 .git/config [foo "Bar"] key = test key = test Sub sections are case sensitive and we have a test for correctly reading them. However we do not have a test for writing out config correctly with case sensitive subsection names, which is why this went unnoticed in 6ae996f2acf (git_config_set: make use of the config parser's event stream, 2018-04-09) Unfortunately we have to make a distinction between old style configuration that looks like [foo.Bar] key = test and the new quoted style as seen above. The old style is documented as case-agnostic, hence we need to keep 'strncasecmp'; although the resulting setting for the old style config differs from the configuration. That will be fixed in a follow up patch. Reported-by: JP Sugarbroad <jpsugar@google.com> Signed-off-by: Stefan Beller <sbeller@google.com> --- config.c | 12 +++++++++++- t/t1300-config.sh | 1 + 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index 7968ef7566a..1d3bf9b5fc0 100644 --- a/config.c +++ b/config.c @@ -37,6 +37,7 @@ struct config_source { int eof; struct strbuf value; struct strbuf var; + unsigned section_name_old_dot_style : 1; int (*do_fgetc)(struct config_source *c); int (*do_ungetc)(int c, struct config_source *conf); @@ -605,6 +606,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name) static int get_extended_base_var(struct strbuf *name, int c) { + cf->section_name_old_dot_style = 0; do { if (c == '\n') goto error_incomplete_line; @@ -641,6 +643,7 @@ static int get_extended_base_var(struct strbuf *name, int c) static int get_base_var(struct strbuf *name) { + cf->section_name_old_dot_style = 1; for (;;) { int c = get_next_char(); if (cf->eof) @@ -2355,14 +2358,21 @@ static int store_aux_event(enum config_event_t type, store->parsed[store->parsed_nr].type = type; if (type == CONFIG_EVENT_SECTION) { + int (*cmpfn)(const char *, const char *, size_t); + if (cf->var.len < 2 || cf->var.buf[cf->var.len - 1] != '.') return error("invalid section name '%s'", cf->var.buf); + if (cf->section_name_old_dot_style) + cmpfn = strncasecmp; + else + cmpfn = strncmp; + /* Is this the section we were looking for? */ store->is_keys_section = store->parsed[store->parsed_nr].is_keys_section = cf->var.len - 1 == store->baselen && - !strncasecmp(cf->var.buf, store->key, store->baselen); + !cmpfn(cf->var.buf, store->key, store->baselen); if (store->is_keys_section) { store->section_seen = 1; ALLOC_GROW(store->seen, store->seen_nr + 1, diff --git a/t/t1300-config.sh b/t/t1300-config.sh index ced13012409..a93f966f128 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1290,6 +1290,7 @@ test_expect_success 'setting different case sensitive subsections ' ' Qc = v2 [d "e"] f = v1 + [d "E"] Qf = v2 EOF # exact match -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 2/3] config: fix case sensitive subsection names on writing 2018-08-01 19:34 ` [PATCH 2/3] config: fix case sensitive subsection names on writing Stefan Beller @ 2018-08-01 21:01 ` Ramsay Jones 2018-08-01 22:26 ` Junio C Hamano 2018-08-01 22:51 ` Junio C Hamano 1 sibling, 1 reply; 33+ messages in thread From: Ramsay Jones @ 2018-08-01 21:01 UTC (permalink / raw) To: Stefan Beller, gitster; +Cc: bmwill, git, johannes.schindelin, peff On 01/08/18 20:34, Stefan Beller wrote: > A use reported a submodule issue regarding strange case indentation s/use/user/ ? ATB, Ramsay Jones ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/3] config: fix case sensitive subsection names on writing 2018-08-01 21:01 ` Ramsay Jones @ 2018-08-01 22:26 ` Junio C Hamano 0 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2018-08-01 22:26 UTC (permalink / raw) To: Ramsay Jones; +Cc: Stefan Beller, bmwill, git, johannes.schindelin, peff Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > On 01/08/18 20:34, Stefan Beller wrote: >> A use reported a submodule issue regarding strange case indentation > > s/use/user/ ? True. Also s/indentation/something else/ ? Insufficient proofreading, perhaps? ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/3] config: fix case sensitive subsection names on writing 2018-08-01 19:34 ` [PATCH 2/3] config: fix case sensitive subsection names on writing Stefan Beller 2018-08-01 21:01 ` Ramsay Jones @ 2018-08-01 22:51 ` Junio C Hamano 2018-08-03 0:30 ` Stefan Beller 1 sibling, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2018-08-01 22:51 UTC (permalink / raw) To: Stefan Beller; +Cc: bmwill, git, johannes.schindelin, peff Stefan Beller <sbeller@google.com> writes: > A use reported a submodule issue regarding strange case indentation > issues, but it could be boiled down to the following test case: Perhaps s/use/user/ s/case indentation issues/section mix-up/ > ... However we do not have a test for writing out config correctly with > case sensitive subsection names, which is why this went unnoticed in > 6ae996f2acf (git_config_set: make use of the config parser's event > stream, 2018-04-09) s/unnoticed in \(.*04-09)\)/unnoticed when \1 broke it./ This is why I asked if the patch is a "FIX" for an issue introduced by the cited commit. > Unfortunately we have to make a distinction between old style configuration > that looks like > > [foo.Bar] > key = test > > and the new quoted style as seen above. The old style is documented as > case-agnostic, hence we need to keep 'strncasecmp'; although the > resulting setting for the old style config differs from the configuration. > That will be fixed in a follow up patch. > > Reported-by: JP Sugarbroad <jpsugar@google.com> > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > config.c | 12 +++++++++++- > t/t1300-config.sh | 1 + > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/config.c b/config.c > index 7968ef7566a..1d3bf9b5fc0 100644 > --- a/config.c > +++ b/config.c > @@ -37,6 +37,7 @@ struct config_source { > int eof; > struct strbuf value; > struct strbuf var; > + unsigned section_name_old_dot_style : 1; > > int (*do_fgetc)(struct config_source *c); > int (*do_ungetc)(int c, struct config_source *conf); > @@ -605,6 +606,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name) > > static int get_extended_base_var(struct strbuf *name, int c) > { > + cf->section_name_old_dot_style = 0; > do { > if (c == '\n') > goto error_incomplete_line; > @@ -641,6 +643,7 @@ static int get_extended_base_var(struct strbuf *name, int c) > > static int get_base_var(struct strbuf *name) > { > + cf->section_name_old_dot_style = 1; > for (;;) { > int c = get_next_char(); > if (cf->eof) OK, let me rephrase. The basic parse structure is that * upon seeing '[', we call get_base_var(), which stuffs the "section" (including subsection, if exists) in the strbuf. * get_base_var() upon seeing a space after "[section ", calls get_extended_base_var(). This space can never exist in an old-style three-level names, where it is spelled as "[section.subsection]". This space cannot exist in two-level names, either. The closing ']' is eaten by this function before it returns. * get_extended_base_var() grabs the "double quoted" subsection name and eats the closing ']' before it returns. So you set the new bit (section_name_old_dot_style) at the beginning of get_base_var(), i.e. declare that you assume we are reading old style, but upon entering get_extended_base_var(), unset it, because now you know we are parsing a modern style three-level name(s). Feels quite sensible way to keep track of old/new styles. When parsing two-level names, old-style bit is set, which we may need to be careful, thoguh. > @@ -2355,14 +2358,21 @@ static int store_aux_event(enum config_event_t type, > store->parsed[store->parsed_nr].type = type; > > if (type == CONFIG_EVENT_SECTION) { > + int (*cmpfn)(const char *, const char *, size_t); > + > if (cf->var.len < 2 || cf->var.buf[cf->var.len - 1] != '.') > return error("invalid section name '%s'", cf->var.buf); > > + if (cf->section_name_old_dot_style) > + cmpfn = strncasecmp; > + else > + cmpfn = strncmp; > + > /* Is this the section we were looking for? */ > store->is_keys_section = > store->parsed[store->parsed_nr].is_keys_section = > cf->var.len - 1 == store->baselen && > - !strncasecmp(cf->var.buf, store->key, store->baselen); > + !cmpfn(cf->var.buf, store->key, store->baselen); OK. Section names should still be case insensitive (only the case sensitivity of subsection names is special), but presumably that's already normalized by the caller so we do not have to worry when we use strncmp()? Can we add a test to demonstrate that it works correctly? Thanks. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/3] config: fix case sensitive subsection names on writing 2018-08-01 22:51 ` Junio C Hamano @ 2018-08-03 0:30 ` Stefan Beller 2018-08-03 15:51 ` Junio C Hamano 0 siblings, 1 reply; 33+ messages in thread From: Stefan Beller @ 2018-08-03 0:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: Brandon Williams, git, Johannes Schindelin, peff On Wed, Aug 1, 2018 at 3:51 PM Junio C Hamano <gitster@pobox.com> wrote: > > Stefan Beller <sbeller@google.com> writes: > > > A use reported a submodule issue regarding strange case indentation > > issues, but it could be boiled down to the following test case: > > Perhaps > > s/use/user/ > s/case indentation issues/section mix-up/ will be fixed in a reroll > > > ... However we do not have a test for writing out config correctly with > > case sensitive subsection names, which is why this went unnoticed in > > 6ae996f2acf (git_config_set: make use of the config parser's event > > stream, 2018-04-09) > > s/unnoticed in \(.*04-09)\)/unnoticed when \1 broke it./ > > This is why I asked if the patch is a "FIX" for an issue introduced > by the cited commit. I did not check further down the history if it was a recent brakage. > > static int get_base_var(struct strbuf *name) > > { > > + cf->section_name_old_dot_style = 1; > > for (;;) { > > int c = get_next_char(); > > if (cf->eof) > > OK, let me rephrase. The basic parse structure is that > > * upon seeing '[', we call get_base_var(), which stuffs the > "section" (including subsection, if exists) in the strbuf. > > * get_base_var() upon seeing a space after "[section ", calls > get_extended_base_var(). This space can never exist in an > old-style three-level names, where it is spelled as > "[section.subsection]". This space cannot exist in two-level > names, either. The closing ']' is eaten by this function before > it returns. > > * get_extended_base_var() grabs the "double quoted" subsection name > and eats the closing ']' before it returns. > > So you set the new bit (section_name_old_dot_style) at the beginning > of get_base_var(), i.e. declare that you assume we are reading old > style, but upon entering get_extended_base_var(), unset it, because > now you know we are parsing a modern style three-level name(s). > > Feels quite sensible way to keep track of old/new styles. > > When parsing two-level names, old-style bit is set, which we may > need to be careful, thoguh. I considered setting it only when seeing the dot, but then we'd have to make sure it is properly initialized. And *technically* the two level is old style, so I figured it's ok. > > - !strncasecmp(cf->var.buf, store->key, store->baselen); > > + !cmpfn(cf->var.buf, store->key, store->baselen); > > OK. Section names should still be case insensitive (only the case > sensitivity of subsection names is special), but presumably that's > already normalized by the caller so we do not have to worry when we > use strncmp()? Can we add a test to demonstrate that it works > correctly? That was already demonstrated (but not tested) in https://public-inbox.org/git/20180730230443.74416-4-sbeller@google.com/ ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/3] config: fix case sensitive subsection names on writing 2018-08-03 0:30 ` Stefan Beller @ 2018-08-03 15:51 ` Junio C Hamano 0 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2018-08-03 15:51 UTC (permalink / raw) To: Stefan Beller; +Cc: Brandon Williams, git, Johannes Schindelin, peff Stefan Beller <sbeller@google.com> writes: > And *technically* the two level is old style, so I figured it's ok. If we call the bit not after the recentness of the style but after what it is about, e.g. "is the section name as a whole (including its possible subsection part) case insensitive?", then yes, a two-level name can be treated exactly the same way as the old style names with a subsection. Perhaps we should do that rename to save us from future confusion. >> > - !strncasecmp(cf->var.buf, store->key, store->baselen); >> > + !cmpfn(cf->var.buf, store->key, store->baselen); >> >> OK. Section names should still be case insensitive (only the case >> sensitivity of subsection names is special), but presumably that's >> already normalized by the caller so we do not have to worry when we >> use strncmp()? Can we add a test to demonstrate that it works >> correctly? > > That was already demonstrated (but not tested) in > https://public-inbox.org/git/20180730230443.74416-4-sbeller@google.com/ Yeah, I also manually tried when I was playing with the old-style names to see how well it works. We would want to make sure this won't get broken in the future, still. Thanks. ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 3/3] git-config: document accidental multi-line setting in deprecated syntax 2018-08-01 19:34 ` [PATCH 0/3] sb/config-write-fix done without robbing Peter Stefan Beller 2018-08-01 19:34 ` [PATCH 1/3] t1300: document current behavior of setting options Stefan Beller 2018-08-01 19:34 ` [PATCH 2/3] config: fix case sensitive subsection names on writing Stefan Beller @ 2018-08-01 19:34 ` Stefan Beller 2018-08-01 19:58 ` [PATCH 0/3] sb/config-write-fix done without robbing Peter Eric Sunshine ` (2 subsequent siblings) 5 siblings, 0 replies; 33+ messages in thread From: Stefan Beller @ 2018-08-01 19:34 UTC (permalink / raw) To: gitster; +Cc: bmwill, git, johannes.schindelin, peff, sbeller The bug was noticed when writing the previous patch; a fix for this bug is not easy though: If we choose to ignore the case of the subsection (and revert most of the code of the previous patch, just keeping s/strncasecmp/strcmp/), then we'd introduce new sections using the new syntax, such that -------- [section.subsection] key = value1 -------- git config section.Subsection.key value2 would result in -------- [section.subsection] key = value1 [section.Subsection] key = value2 -------- which is even more confusing. A proper fix would replace the first occurrence of 'key'. As the syntax is deprecated, let's prefer to not spend time on fixing the behavior and just document it instead. Signed-off-by: Stefan Beller <sbeller@google.com> --- Documentation/git-config.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 18ddc78f42d..8e240435bee 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -453,6 +453,27 @@ http.sslverify false include::config.txt[] +BUGS +---- +When using the deprecated `[section.subsection]` syntax, changing a value +will result in adding a multi-line key instead of a change, if the subsection +is given with at least one uppercase character. For example when the config +looks like + +-------- + [section.subsection] + key = value1 +-------- + +and running `git config section.Subsection.key value2` will result in + +-------- + [section.subsection] + key = value1 + key = value2 +-------- + + GIT --- Part of the linkgit:git[1] suite -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 0/3] sb/config-write-fix done without robbing Peter 2018-08-01 19:34 ` [PATCH 0/3] sb/config-write-fix done without robbing Peter Stefan Beller ` (2 preceding siblings ...) 2018-08-01 19:34 ` [PATCH 3/3] git-config: document accidental multi-line setting in deprecated syntax Stefan Beller @ 2018-08-01 19:58 ` Eric Sunshine 2018-08-02 19:49 ` Junio C Hamano 2018-08-03 0:34 ` [PATCH 0/3] Reroll of sb/config-write-fix Stefan Beller 5 siblings, 0 replies; 33+ messages in thread From: Eric Sunshine @ 2018-08-01 19:58 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, Brandon Williams, Git List, Johannes Schindelin, peff On Wed, Aug 1, 2018 at 3:34 PM Stefan Beller <sbeller@google.com> wrote: > The first patch stands as is unchanged, and the second and third patch > are different enough that range-diff doesn't want to show a diff. For future reference, range-diff's --creation-factor tweak may help here. Depending upon just how heavily changed they are, you may be able to use it to coerce range-diff into doing what you want. (Also, interdiff may be a valid alternative.) ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 0/3] sb/config-write-fix done without robbing Peter 2018-08-01 19:34 ` [PATCH 0/3] sb/config-write-fix done without robbing Peter Stefan Beller ` (3 preceding siblings ...) 2018-08-01 19:58 ` [PATCH 0/3] sb/config-write-fix done without robbing Peter Eric Sunshine @ 2018-08-02 19:49 ` Junio C Hamano 2018-08-03 0:34 ` [PATCH 0/3] Reroll of sb/config-write-fix Stefan Beller 5 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2018-08-02 19:49 UTC (permalink / raw) To: Stefan Beller; +Cc: bmwill, git, johannes.schindelin, peff Stefan Beller <sbeller@google.com> writes: >> Am I correct to understand that this patch is a "FIX" for breakage >> introduced by that commit? The phrasing is not helping me to pick >> a good base to queue these patches on. > > Please pick 4f4d0b42bae (Merge branch 'js/empty-config-section-fix', 2018-05-08) > as the base of this new series (am needs -3 to apply), although I developed this > series on origin/master. > >> Even though I hate to rob Peter to pay Paul (or vice versa) > > Yeah me, too. Here is a proper fix (i.e. only pay Paul, without the robbery), > and a documentation of the second bug that we discovered. Thanks. I started with this in 'config' [V.A] r = one [v.a] r = two and did "git config --replace-all v.a.r 1", which resulted in [V.A] [v.a] r = 1 which is "correct", but defeats the "empty section is irritating" tweak that introduced the bug these patches try to fix, which is unfortunate. But then "git config v.A.r 2" would give us [V.A] [v.a] r = 1 r = 2 So this seems still broken, somewhat. ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 0/3] Reroll of sb/config-write-fix 2018-08-01 19:34 ` [PATCH 0/3] sb/config-write-fix done without robbing Peter Stefan Beller ` (4 preceding siblings ...) 2018-08-02 19:49 ` Junio C Hamano @ 2018-08-03 0:34 ` Stefan Beller 2018-08-03 0:34 ` [PATCH 1/3] t1300: document current behavior of setting options Stefan Beller ` (2 more replies) 5 siblings, 3 replies; 33+ messages in thread From: Stefan Beller @ 2018-08-03 0:34 UTC (permalink / raw) To: sbeller; +Cc: bmwill, git, gitster, johannes.schindelin, peff Only fix was in the commit message of the second patch: 2: c667e555066 ! 1749: 38e5f6f335d config: fix case sensitive subsection names on writing @@ -2,8 +2,8 @@ config: fix case sensitive subsection names on writing - A use reported a submodule issue regarding strange case indentation - issues, but it could be boiled down to the following test case: + A user reported a submodule issue regarding a section mix-up, + but it could be boiled down to the following test case: previous version at https://public-inbox.org/git/20180801193413.146994-1-sbeller@google.com/ Stefan Beller (3): t1300: document current behavior of setting options config: fix case sensitive subsection names on writing git-config: document accidental multi-line setting in deprecated syntax Documentation/git-config.txt | 21 +++++++++ config.c | 12 ++++- t/t1300-config.sh | 87 ++++++++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 1 deletion(-) -- 2.18.0.132.g195c49a2227 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/3] t1300: document current behavior of setting options 2018-08-03 0:34 ` [PATCH 0/3] Reroll of sb/config-write-fix Stefan Beller @ 2018-08-03 0:34 ` Stefan Beller 2018-08-03 0:34 ` [PATCH 2/3] config: fix case sensitive subsection names on writing Stefan Beller 2018-08-03 0:34 ` [PATCH 3/3] git-config: document accidental multi-line setting in deprecated syntax Stefan Beller 2 siblings, 0 replies; 33+ messages in thread From: Stefan Beller @ 2018-08-03 0:34 UTC (permalink / raw) To: sbeller; +Cc: bmwill, git, gitster, johannes.schindelin, peff This documents current behavior of the config machinery, when changing the value of some settings. This patch just serves to provide a baseline for the follow up that will fix some issues with the current behavior. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/t1300-config.sh | 86 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 24706ba4125..e87cfc1804f 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1218,6 +1218,92 @@ test_expect_success 'last one wins: three level vars' ' test_cmp expect actual ' +test_expect_success 'old-fashioned settings are case insensitive' ' + test_when_finished "rm -f testConfig testConfig_expect testConfig_actual" && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + Qr = value2 + EOF + git config -f testConfig_actual "v.a.r" value2 && + test_cmp testConfig_expect testConfig_actual && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + QR = value2 + EOF + git config -f testConfig_actual "V.a.R" value2 && + test_cmp testConfig_expect testConfig_actual && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + r = value1 + Qr = value2 + EOF + git config -f testConfig_actual "V.A.r" value2 && + test_cmp testConfig_expect testConfig_actual && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + r = value1 + Qr = value2 + EOF + git config -f testConfig_actual "v.A.r" value2 && + test_cmp testConfig_expect testConfig_actual +' + +test_expect_success 'setting different case sensitive subsections ' ' + test_when_finished "rm -f testConfig testConfig_expect testConfig_actual" && + + cat >testConfig_actual <<-EOF && + [V "A"] + R = v1 + [K "E"] + Y = v1 + [a "b"] + c = v1 + [d "e"] + f = v1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V "A"] + Qr = v2 + [K "E"] + Qy = v2 + [a "b"] + Qc = v2 + [d "e"] + f = v1 + Qf = v2 + EOF + # exact match + git config -f testConfig_actual a.b.c v2 && + # match section and subsection, key is cased differently. + git config -f testConfig_actual K.E.y v2 && + # section and key are matched case insensitive, but subsection needs + # to match; When writing out new values only the key is adjusted + git config -f testConfig_actual v.A.r v2 && + # subsection is not matched: + git config -f testConfig_actual d.E.f v2 && + test_cmp testConfig_expect testConfig_actual +' + for VAR in a .a a. a.0b a."b c". a."b c".0d do test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" ' -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 2/3] config: fix case sensitive subsection names on writing 2018-08-03 0:34 ` [PATCH 0/3] Reroll of sb/config-write-fix Stefan Beller 2018-08-03 0:34 ` [PATCH 1/3] t1300: document current behavior of setting options Stefan Beller @ 2018-08-03 0:34 ` Stefan Beller 2018-08-03 0:34 ` [PATCH 3/3] git-config: document accidental multi-line setting in deprecated syntax Stefan Beller 2 siblings, 0 replies; 33+ messages in thread From: Stefan Beller @ 2018-08-03 0:34 UTC (permalink / raw) To: sbeller; +Cc: bmwill, git, gitster, johannes.schindelin, peff A user reported a submodule issue regarding a section mix-up, but it could be boiled down to the following test case: $ git init test && cd test $ git config foo."Bar".key test $ git config foo."bar".key test $ tail -n 3 .git/config [foo "Bar"] key = test key = test Sub sections are case sensitive and we have a test for correctly reading them. However we do not have a test for writing out config correctly with case sensitive subsection names, which is why this went unnoticed in 6ae996f2acf (git_config_set: make use of the config parser's event stream, 2018-04-09) Unfortunately we have to make a distinction between old style configuration that looks like [foo.Bar] key = test and the new quoted style as seen above. The old style is documented as case-agnostic, hence we need to keep 'strncasecmp'; although the resulting setting for the old style config differs from the configuration. That will be fixed in a follow up patch. Reported-by: JP Sugarbroad <jpsugar@google.com> Signed-off-by: Stefan Beller <sbeller@google.com> --- config.c | 12 +++++++++++- t/t1300-config.sh | 1 + 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index 66645047eb3..ffc2ffeafeb 100644 --- a/config.c +++ b/config.c @@ -37,6 +37,7 @@ struct config_source { int eof; struct strbuf value; struct strbuf var; + unsigned section_name_old_dot_style : 1; int (*do_fgetc)(struct config_source *c); int (*do_ungetc)(int c, struct config_source *conf); @@ -605,6 +606,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name) static int get_extended_base_var(struct strbuf *name, int c) { + cf->section_name_old_dot_style = 0; do { if (c == '\n') goto error_incomplete_line; @@ -641,6 +643,7 @@ static int get_extended_base_var(struct strbuf *name, int c) static int get_base_var(struct strbuf *name) { + cf->section_name_old_dot_style = 1; for (;;) { int c = get_next_char(); if (cf->eof) @@ -2364,14 +2367,21 @@ static int store_aux_event(enum config_event_t type, store->parsed[store->parsed_nr].type = type; if (type == CONFIG_EVENT_SECTION) { + int (*cmpfn)(const char *, const char *, size_t); + if (cf->var.len < 2 || cf->var.buf[cf->var.len - 1] != '.') return error("invalid section name '%s'", cf->var.buf); + if (cf->section_name_old_dot_style) + cmpfn = strncasecmp; + else + cmpfn = strncmp; + /* Is this the section we were looking for? */ store->is_keys_section = store->parsed[store->parsed_nr].is_keys_section = cf->var.len - 1 == store->baselen && - !strncasecmp(cf->var.buf, store->key, store->baselen); + !cmpfn(cf->var.buf, store->key, store->baselen); if (store->is_keys_section) { store->section_seen = 1; ALLOC_GROW(store->seen, store->seen_nr + 1, diff --git a/t/t1300-config.sh b/t/t1300-config.sh index e87cfc1804f..4976e2fcd3f 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1290,6 +1290,7 @@ test_expect_success 'setting different case sensitive subsections ' ' Qc = v2 [d "e"] f = v1 + [d "E"] Qf = v2 EOF # exact match -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 3/3] git-config: document accidental multi-line setting in deprecated syntax 2018-08-03 0:34 ` [PATCH 0/3] Reroll of sb/config-write-fix Stefan Beller 2018-08-03 0:34 ` [PATCH 1/3] t1300: document current behavior of setting options Stefan Beller 2018-08-03 0:34 ` [PATCH 2/3] config: fix case sensitive subsection names on writing Stefan Beller @ 2018-08-03 0:34 ` Stefan Beller 2 siblings, 0 replies; 33+ messages in thread From: Stefan Beller @ 2018-08-03 0:34 UTC (permalink / raw) To: sbeller; +Cc: bmwill, git, gitster, johannes.schindelin, peff The bug was noticed when writing the previous patch; a fix for this bug is not easy though: If we choose to ignore the case of the subsection (and revert most of the code of the previous patch, just keeping s/strncasecmp/strcmp/), then we'd introduce new sections using the new syntax, such that -------- [section.subsection] key = value1 -------- git config section.Subsection.key value2 would result in -------- [section.subsection] key = value1 [section.Subsection] key = value2 -------- which is even more confusing. A proper fix would replace the first occurrence of 'key'. As the syntax is deprecated, let's prefer to not spend time on fixing the behavior and just document it instead. Signed-off-by: Stefan Beller <sbeller@google.com> --- Documentation/git-config.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 18ddc78f42d..8e240435bee 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -453,6 +453,27 @@ http.sslverify false include::config.txt[] +BUGS +---- +When using the deprecated `[section.subsection]` syntax, changing a value +will result in adding a multi-line key instead of a change, if the subsection +is given with at least one uppercase character. For example when the config +looks like + +-------- + [section.subsection] + key = value1 +-------- + +and running `git config section.Subsection.key value2` will result in + +-------- + [section.subsection] + key = value1 + key = value2 +-------- + + GIT --- Part of the linkgit:git[1] suite -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] config: fix case sensitive subsection names on writing 2018-07-27 20:51 [PATCH] config: fix case sensitive subsection names on writing Stefan Beller 2018-07-27 21:21 ` Brandon Williams @ 2018-07-27 21:37 ` Junio C Hamano 1 sibling, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2018-07-27 21:37 UTC (permalink / raw) To: Stefan Beller; +Cc: git Stefan Beller <sbeller@google.com> writes: > A use reported a submodule issue regarding strange case indentation > issues, but it could be boiled down to the following test case: > > $ git init test && cd test > $ git config foo."Bar".key test > $ git config foo."bar".key test > $ tail -n 3 .git/config > [foo "Bar"] > key = test > key = test > > Sub sections are case sensitive and we have a test for correctly reading > them. However we do not have a test for writing out config correctly with > case sensitive subsection names, which is why this went unnoticed in > 6ae996f2acf (git_config_set: make use of the config parser's event > stream, 2018-04-09) Thanks for finding this bug and fixing it; yes it would have been even nicer if we caught it before it hit 'master', but we can only do what we can X-<. > diff --git a/config.c b/config.c > index 3aacddfec4b..3ded92b678b 100644 > --- a/config.c > +++ b/config.c > @@ -2374,7 +2374,7 @@ static int store_aux_event(enum config_event_t type, > store->is_keys_section = > store->parsed[store->parsed_nr].is_keys_section = > cf->var.len - 1 == store->baselen && > - !strncasecmp(cf->var.buf, store->key, store->baselen); > + !strncmp(cf->var.buf, store->key, store->baselen); > if (store->is_keys_section) { > store->section_seen = 1; > ALLOC_GROW(store->seen, store->seen_nr + 1, > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > index 03c223708eb..8325d4495f4 100755 > --- a/t/t1300-config.sh > +++ b/t/t1300-config.sh > @@ -1218,6 +1218,24 @@ test_expect_success 'last one wins: three level vars' ' > test_cmp expect actual > ' > > +test_expect_success 'setting different case subsections ' ' > + test_when_finished "rm -f caseSens caseSens_actual caseSens_expect" && > + > + # v.a.r and v.A.r are not the same variable, as the middle > + # level of a three-level configuration variable name is > + # case sensitive. > + git config -f caseSens v."A".r VAL && > + git config -f caseSens v."a".r val && It probably is easier to read to write these as "v.A.r" and "v.a.r" respectively. > + > + echo VAL >caseSens_expect && > + git config -f caseSens v."A".r >caseSens_actual && > + test_cmp caseSens_expect caseSens_actual && > + > + echo val >caseSens_expect && > + git config -f caseSens v."a".r >caseSens_actual && > + test_cmp caseSens_expect caseSens_actual > +' > + > for VAR in a .a a. a.0b a."b c". a."b c".0d > do > test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" ' ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2018-08-03 15:51 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-27 20:51 [PATCH] config: fix case sensitive subsection names on writing Stefan Beller 2018-07-27 21:21 ` Brandon Williams 2018-07-27 21:39 ` Junio C Hamano 2018-07-27 23:35 ` Stefan Beller 2018-07-27 23:36 ` Stefan Beller 2018-07-27 23:37 ` Stefan Beller 2018-07-28 1:01 ` Junio C Hamano 2018-07-28 3:52 ` Stefan Beller 2018-07-28 10:53 ` Jeff King 2018-07-28 1:37 ` Junio C Hamano 2018-07-30 12:49 ` Johannes Schindelin 2018-07-30 23:04 ` [PATCH 0/3] " Stefan Beller 2018-07-30 23:04 ` [PATCH 1/3] t1300: document current behavior of setting options Stefan Beller 2018-07-30 23:04 ` [PATCH 2/3] config: fix case sensitive subsection names on writing Stefan Beller 2018-07-31 20:16 ` Junio C Hamano 2018-07-30 23:04 ` [PATCH 3/3] config: treat section case insensitive in store_aux_event Stefan Beller 2018-07-31 15:16 ` [PATCH 0/3] config: fix case sensitive subsection names on writing Junio C Hamano 2018-08-01 19:34 ` [PATCH 0/3] sb/config-write-fix done without robbing Peter Stefan Beller 2018-08-01 19:34 ` [PATCH 1/3] t1300: document current behavior of setting options Stefan Beller 2018-08-01 19:34 ` [PATCH 2/3] config: fix case sensitive subsection names on writing Stefan Beller 2018-08-01 21:01 ` Ramsay Jones 2018-08-01 22:26 ` Junio C Hamano 2018-08-01 22:51 ` Junio C Hamano 2018-08-03 0:30 ` Stefan Beller 2018-08-03 15:51 ` Junio C Hamano 2018-08-01 19:34 ` [PATCH 3/3] git-config: document accidental multi-line setting in deprecated syntax Stefan Beller 2018-08-01 19:58 ` [PATCH 0/3] sb/config-write-fix done without robbing Peter Eric Sunshine 2018-08-02 19:49 ` Junio C Hamano 2018-08-03 0:34 ` [PATCH 0/3] Reroll of sb/config-write-fix Stefan Beller 2018-08-03 0:34 ` [PATCH 1/3] t1300: document current behavior of setting options Stefan Beller 2018-08-03 0:34 ` [PATCH 2/3] config: fix case sensitive subsection names on writing Stefan Beller 2018-08-03 0:34 ` [PATCH 3/3] git-config: document accidental multi-line setting in deprecated syntax Stefan Beller 2018-07-27 21:37 ` [PATCH] config: fix case sensitive subsection names on writing Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).