[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 0/3] config: fix case sensitive subsection names on writing

[Junio C Hamano]

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.

[PATCH 0/3] sb/config-write-fix done without robbing Peter

[Stefan Beller]

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

[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

[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

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

[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

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

> 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 0/3] Reroll of sb/config-write-fix

[Stefan Beller]

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

[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] config: fix case sensitive subsection names on writing

[Johannes Schindelin]

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

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

[Stefan Beller]

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

[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

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.