[PATCH 0/3] Resending sb/config-write-fix

[PATCH 0/3] Resending sb/config-write-fix

[Stefan Beller]

This is a resend of sb/config-write-fix, with a slightly
better commit message and a renamed variable.

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(-)

./git-range-diff origin/sb/config-write-fix...HEAD >>0000-cover-letter.patch 
2.18.0.597.ga71716f1ad-goog

1:  999d9026272 ! 1:  e40f57f3da1 t1300: document current behavior of setting options
    @@ -7,7 +7,6 @@
         for the follow up that will fix some issues with the current behavior.
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/t/t1300-config.sh b/t/t1300-config.sh
      --- a/t/t1300-config.sh
2:  c667e555066 ! 2:  f01cb1d9dae 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:
     
           $ git init test  && cd test
           $ git config foo."Bar".key test
    @@ -32,7 +32,6 @@
     
         Reported-by: JP Sugarbroad <jpsugar@google.com>
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/config.c b/config.c
      --- a/config.c
    @@ -41,7 +40,7 @@
      	int eof;
      	struct strbuf value;
      	struct strbuf var;
    -+	unsigned section_name_old_dot_style : 1;
    ++	unsigned subsection_case_sensitive : 1;
      
      	int (*do_fgetc)(struct config_source *c);
      	int (*do_ungetc)(int c, struct config_source *conf);
    @@ -49,7 +48,7 @@
      
      static int get_extended_base_var(struct strbuf *name, int c)
      {
    -+	cf->section_name_old_dot_style = 0;
    ++	cf->subsection_case_sensitive = 0;
      	do {
      		if (c == '\n')
      			goto error_incomplete_line;
    @@ -57,7 +56,7 @@
      
      static int get_base_var(struct strbuf *name)
      {
    -+	cf->section_name_old_dot_style = 1;
    ++	cf->subsection_case_sensitive = 1;
      	for (;;) {
      		int c = get_next_char();
      		if (cf->eof)
    @@ -70,7 +69,7 @@
      		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)
    ++		if (cf->subsection_case_sensitive)
     +			cmpfn = strncasecmp;
     +		else
     +			cmpfn = strncmp;
3:  6749bb283a8 ! 3:  6b5ad773490 git-config: document accidental multi-line setting in deprecated syntax
    @@ -29,7 +29,6 @@
         spend time on fixing the behavior and just document it instead.
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
      --- a/Documentation/git-config.txt

[PATCH 1/3] t1300: document current behavior of setting options

[Stefan Beller]

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 e43982a9c1f..c15c19bf78d 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1188,6 +1188,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.597.ga71716f1ad-goog

[PATCH 2/3] config: fix case sensitive subsection names on writing

[Stefan Beller]

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 02050529788..27e800c7ce8 100644
--- a/config.c
+++ b/config.c
@@ -35,6 +35,7 @@ struct config_source {
 	int eof;
 	struct strbuf value;
 	struct strbuf var;
+	unsigned subsection_case_sensitive : 1;
 
 	int (*do_fgetc)(struct config_source *c);
 	int (*do_ungetc)(int c, struct config_source *conf);
@@ -603,6 +604,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->subsection_case_sensitive = 0;
 	do {
 		if (c == '\n')
 			goto error_incomplete_line;
@@ -639,6 +641,7 @@ static int get_extended_base_var(struct strbuf *name, int c)
 
 static int get_base_var(struct strbuf *name)
 {
+	cf->subsection_case_sensitive = 1;
 	for (;;) {
 		int c = get_next_char();
 		if (cf->eof)
@@ -2328,14 +2331,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->subsection_case_sensitive)
+			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 c15c19bf78d..77c5899d000 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1260,6 +1260,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.597.ga71716f1ad-goog

[PATCH 3/3] git-config: document accidental multi-line setting in deprecated syntax

[Stefan Beller]

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 14da5fc157e..1ac2eab9482 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -430,6 +430,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.597.ga71716f1ad-goog

Re: [PATCH 0/3] Resending sb/config-write-fix

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> This is a resend of sb/config-write-fix, with a slightly
> better commit message and a renamed variable.
>
> Thanks,
> Stefan

Thanks.  Let's declare victory and mark it to be merged to 'next'.

Re: [PATCH 2/3] config: fix case sensitive subsection names on writing

[Junio C Hamano]

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.

[PATCH 2/3] config: fix case sensitive subsection names on writing

[Stefan Beller]

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

Re: [PATCH 2/3] config: fix case sensitive subsection names on writing

[Stefan Beller]

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/

Re: [PATCH 2/3] config: fix case sensitive subsection names on writing

[Junio C Hamano]

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.

Re: [PATCH 2/3] config: fix case sensitive subsection names on writing

[Junio C Hamano]

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?

Re: [PATCH 2/3] config: fix case sensitive subsection names on writing

[Ramsay Jones]

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

[PATCH 2/3] config: fix case sensitive subsection names on writing

[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)

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

Re: [PATCH 2/3] config: fix case sensitive subsection names on writing

[Junio C Hamano]

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.

[PATCH 2/3] config: fix case sensitive subsection names on writing

[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>
---
 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