config: fix case sensitive subsection names on writing
PATCH 1/1 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:
$ 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 = testSub 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.
--- config.c | 2 +- t/t1300-config.sh | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-)
config.c +1 -1
@@ -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,
t/t1300-config.sh +18 -0
@@ -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'" '
Signature · 1 line
2.18.0.345.g5c9ce644c3-goog
Quoted 32 lines · 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 = testSub 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 <[email protected]>Signed-off-by Stefan Beller <[email protected]>--- config.c | 2 +- t/t1300-config.sh | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-)config.c +0
-0
config.c static int store_aux_event(enum config_event_t type, · line 2377
Quoted 5 lines
config.c +1
-1@@ -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.
Quoted 35 lines
config.c +0
-0if (store->is_keys_section) { store->section_seen =1 ; ALLOC_GROW(store->seen, store->seen_nr + 1,t/t1300-config.sh +18
-0@@ -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'" 'Signature · 1 line
2.18.0.345.g5c9ce644c3-goog
Signature · 1 line
Brandon Williams
Quoted 2 lines · Brandon Williams
Either way you're probably going to need to be careful about how you do string comparison against the different parts.
Good suggestion.
Quoted 8 lines
Quoted 8 lines
t/t1300-config.sh +0
-0@@ -1218,6 +1218,24 @@ test_expect_success 'last one wins: three level vars' ' test_cmp expect actual '
Quoted 6 lines
t/t1300-config.sh test_expect_success 'last one wins: three level vars' ' · line 1226
Quoted 6 lines
t/t1300-config.sh +6
-0+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?
Quoted 18 lines
Quoted 18 lines
t/t1300-config.sh +12
-0+ 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'" 'Signature · 1 line
2.18.0.345.g5c9ce644c3-goog
Quoted 7 lines · Junio C Hamano
Quoted 2 lines · Brandon Williams
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.
Quoted 9 lines
Quoted 3 lines
t/t1300-config.sh test_expect_success 'last one wins: three level vars' ' · line 1226
Quoted 3 lines
t/t1300-config.sh +3
-0+ # 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.
PATCH 1/1 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:
$ 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 = testSub 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.
---
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(-)
config.c +1 -1
@@ -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,
t/t1300-config.sh +57 -0
63 lines not on this page · raw message
Signature · 1 line
2.18.0.345.g5c9ce644c3-goog
I cc'd the wrong peff. Sorry about that.
On Fri, Jul 27, 2018 at 4:36 PM Stefan Beller <[email protected]> wrote:
....
Quoted 1 line · Stefan Beller
A use reported a submodule issue regarding strange case indentation
s/use/&r/; Is this "indentation" issue?
Quoted 6 lines
issues, but it could be boiled down to the following test case:
...t/t1300-config.sh +3
-0+test_expect_success 'old-fashioned settings are case insensitive' ' + test_when_finished "rm -f testConfig testConfig_expect testConfig_actual" && +
t/t1300-config.sh test_expect_success 'last one wins: three level vars' ' · line 1233
Quoted 10 lines
t/t1300-config.sh +10
-0+ 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?
t/t1300-config.sh test_expect_success 'last one wins: three level vars' ' · line 1241
Quoted 7 lines
t/t1300-config.sh +7
-0+ 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.
Quoted 6 lines · Junio C Hamano
Quoted 1 line · Stefan Beller
A use reported a submodule issue regarding strange case indentation
s/use/&r/; Is this "indentation" issue?
eh case sensitivity*
Quoted 10 lines
t/t1300-config.sh test_expect_success 'last one wins: three level vars' ' · line 1233
Quoted 5 lines
t/t1300-config.sh +5
-0+ q_to_tab >testConfig_expect <<-EOF && + # insensitive: + [V.A] + Qr = value2 + EOFIt is unfortunate that we hardcode the exact indentation in the test to make it care. Perhaps a wrapper around
test_cmpthat 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.
Quoted 9 lines
t/t1300-config.sh test_expect_success 'last one wins: three level vars' ' · line 1241
Quoted 8 lines
t/t1300-config.sh +7
-0+ 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 +'
Quoted 7 lines
I think you meant to use "$key" when setting the variable to value2.
When the
test_cmpfails 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 insidetest_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
Quoted 17 lines · Stefan Beller
Quoted 8 lines
t/t1300-config.sh test_expect_success 'last one wins: three level vars' ' · line 1241
Quoted 7 lines
t/t1300-config.sh +7
-0+ 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 +'Quoted 6 lines
I think you meant to use "$key" when setting the variable to value2.
When the
test_cmpfails 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 insidetest_expect_success.Hah, of course this patch is not as easy.
Quoted 7 lines
The problem is in
git_parse_source(config.c, line 755) when we callget_base_varit 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").
Quoted 4 lines
I wonder if we want to either (a) add another
CONFIG_EVENT_SECTIONthat 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...
Quoted 2 lines
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
Quoted 4 lines · Stefan Beller
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.
Hi,
Quoted 3 lines · Junio C Hamano
Stefan Beller <[email protected]> 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...
Quoted 2 lines
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
cover config: fix case sensitive subsection names on writing
Quoted 4 lines · Johannes Schindelin
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.
Quoted 3 lines
How does the
patch take care of that? Is it relying ongit_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.
Quoted 7 lines
Quoted 2 lines
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(-)
Signature · 1 line
2.18.0.132.g195c49a2227
PATCH 1/3 t1300: document current behavior of setting options
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.
--- t/t1300-config.sh | 86 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+)
t/t1300-config.sh +86 -0
@@ -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'" '
Signature · 1 line
2.18.0.132.g195c49a2227
PATCH 2/3 config: fix case sensitive subsection names on writing
In reply to Stefan BellerA 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 = testSub 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.
--- config.c | 2 +- t/t1300-config.sh | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)
config.c +1 -1
@@ -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,
t/t1300-config.sh +3 -0
@@ -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
Signature · 1 line
2.18.0.132.g195c49a2227
Quoted 11 lines · 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
Quoted 5 lines
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.
PATCH 3/3 config: treat section case insensitive in store_aux_event
In reply to Stefan BellerThis 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.
--- config.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
config.c +15 -1
@@ -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,
Signature · 1 line
2.18.0.132.g195c49a2227
Quoted 8 lines · Stefan Beller
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 intoget_extended_base_varif the new notation is used.
Quoted 7 lines
The code in
store_aux_eventhowever 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.
cover sb/config-write-fix done without robbing Peter
Quoted 3 lines
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.
Quoted 1 line
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(-)
PATCH 1/3 t1300: document current behavior of setting options
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.
--- t/t1300-config.sh | 86 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+)
t/t1300-config.sh +86 -0
@@ -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'" '
Signature · 1 line
2.18.0.132.g195c49a2227
PATCH 2/3 config: fix case sensitive subsection names on writing
In reply to Stefan BellerA 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 = testSub 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.
--- config.c | 12 +++++++++++- t/t1300-config.sh | 1 + 2 files changed, 12 insertions(+), 1 deletion(-)
config.c +11 -1
@@ -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,
t/t1300-config.sh +1 -0
@@ -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
Signature · 1 line
2.18.0.132.g195c49a2227
Quoted 1 line · Stefan Beller
A use reported a submodule issue regarding strange case indentation
s/use/user/ ?
ATB,
Ramsay Jones
Quoted 4 lines · Ramsay Jones
Quoted 1 line · Stefan Beller
A use reported a submodule issue regarding strange case indentation
s/use/user/ ?
True. Also s/indentation/something else/ ?
Insufficient proofreading, perhaps?
Quoted 2 lines · Stefan Beller
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/
Quoted 4 lines
... 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.
Quoted 39 lines
Unfortunately we have to make a distinction between old style configuration that looks like
[foo.Bar]
key = testand 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 <[email protected]>Signed-off-by Stefan Beller <[email protected]>--- config.c | 12 +++++++++++- t/t1300-config.sh | 1 + 2 files changed, 12 insertions(+), 1 deletion(-)config.c +2
-0@@ -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;
config.c static int get_extended_base_var(struct strbuf *name, int c) · line 649
Quoted 7 lines
config.c +1
-0@@ -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.
Quoted 10 lines
config.c +3
-0@@ -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)
config.c static int store_aux_event(enum config_event_t type, · line 2375
Quoted 10 lines
config.c +5
-1+ 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.
Quoted 10 lines · Junio C Hamano
Quoted 2 lines · Stefan Beller
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
Quoted 10 lines
Quoted 5 lines
... 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.
Quoted 27 lines
Quoted 6 lines
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 ", callsget_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 ofget_base_var(), i.e. declare that you assume we are reading old style, but upon enteringget_extended_base_var(), unset it, because now you know we are parsing a modern style three-level name(s).
Quoted 4 lines
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.
Quoted 8 lines
config.c static int store_aux_event(enum config_event_t type, · line 2375
Quoted 2 lines
config.c +1
-1- !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/[email protected]/
Quoted 1 line · Stefan Beller
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.
Quoted 11 lines
Quoted 8 lines
config.c static int store_aux_event(enum config_event_t type, · line 2375
Quoted 2 lines
config.c +1
-1- !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/[email protected]/
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.
PATCH 3/3 git-config: document accidental multi-line setting in deprecated syntax
In reply to Stefan BellerThe 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.
--- Documentation/git-config.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
Documentation/git-config.txt +21 -0
@@ -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
Signature · 1 line
2.18.0.132.g195c49a2227
Re: [PATCH 0/3] sb/config-write-fix done without robbing Peter
In reply to Stefan BellerQuoted 2 lines · Stefan Beller
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.)
Re: [PATCH 0/3] sb/config-write-fix done without robbing Peter
In reply to Stefan BellerQuoted 4 lines · Stefan Beller
Quoted 3 lines
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.
Quoted 8 lines
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.Quoted 1 line
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 = twoand did "git config --replace-all v.a.r 1", which resulted in
[V.A]
[v.a]
r = 1which 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 = 2So this seems still broken, somewhat.
cover Reroll of sb/config-write-fix
In reply to Stefan BellerOnly 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/[email protected]/
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(-)
Signature · 1 line
2.18.0.132.g195c49a2227
PATCH 1/3 t1300: document current behavior of setting options
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.
--- t/t1300-config.sh | 86 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+)
t/t1300-config.sh +86 -0
@@ -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'" '
Signature · 1 line
2.18.0.132.g195c49a2227
PATCH 2/3 config: fix case sensitive subsection names on writing
In reply to Stefan BellerA 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 = testSub 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.
--- config.c | 12 +++++++++++- t/t1300-config.sh | 1 + 2 files changed, 12 insertions(+), 1 deletion(-)
config.c +11 -1
@@ -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,
t/t1300-config.sh +1 -0
@@ -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
Signature · 1 line
2.18.0.132.g195c49a2227
PATCH 3/3 git-config: document accidental multi-line setting in deprecated syntax
In reply to Stefan BellerThe 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.
--- Documentation/git-config.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
Documentation/git-config.txt +21 -0
@@ -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
Signature · 1 line
2.18.0.132.g195c49a2227
Quoted 11 lines · 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
Quoted 5 lines
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-<.
Quoted 19 lines
config.c +1
-1@@ -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,t/t1300-config.sh +0
-0@@ -1218,6 +1218,24 @@ test_expect_success 'last one wins: three level vars' ' test_cmp expect actual
t/t1300-config.sh test_expect_success 'last one wins: three level vars' ' · line 1228
Quoted 10 lines
t/t1300-config.sh +8
-0' +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.
Quoted 13 lines
t/t1300-config.sh +10
-0+ + 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'" '