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

From: Brandon Williams <bmwill@google.com>
Date: Jul 27, 2018, 21:21 UTC
Message-ID: <20180727212140.GA54208@google.com>
In-Reply-To: <20180727205117.243770-1-sbeller@google.com>

On 07/27, Stefan Beller wrote:

Show 37 quoted lines

> A use reported a submodule issue regarding strange case indentation
> issues, but it could be boiled down to the following test case:
> 
>   $ git init test  && cd test
>   $ git config foo."Bar".key test
>   $ git config foo."bar".key test
>   $ tail -n 3 .git/config
>   [foo "Bar"]
>         key = test
>         key = test
> 
> Sub sections are case sensitive and we have a test for correctly reading
> them. However we do not have a test for writing out config correctly with
> case sensitive subsection names, which is why this went unnoticed in
> 6ae996f2acf (git_config_set: make use of the config parser's event
> stream, 2018-04-09)
> 
> Make the subsection case sensitive and provide a test for both reading
> and writing.
> 
> Reported-by: JP Sugarbroad <jpsugar@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  config.c          |  2 +-
>  t/t1300-config.sh | 18 ++++++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/config.c b/config.c
> index 3aacddfec4b..3ded92b678b 100644
> --- a/config.c
> +++ b/config.c
> @@ -2374,7 +2374,7 @@ static int store_aux_event(enum config_event_t type,
>  		store->is_keys_section =
>  			store->parsed[store->parsed_nr].is_keys_section =
>  			cf->var.len - 1 == store->baselen &&
> -			!strncasecmp(cf->var.buf, store->key, store->baselen);
> +			!strncmp(cf->var.buf, store->key, store->baselen);

I've done some work in the config part of our codebase but I don't really know whats going on here due to two things: this is a callback function and it relies on global state...

I can say though that we might want to be careful about completely converting this to a case sensitive compare. Our config is a key value store with the following format: 'section.subsection.key'. IIRC both section and key are always compared case insensitively. The subsection can be case sensitive or insensitive based on how its stored in the config files itself:

  # Case insensitive
  [section.subsection]
      key = value

  # Case sensitive 
  [section "subsection"]
      key = value

But I don't know how you distinguish between these cases when a config is specified on a single line (section.subsection.key). Do we always assume the sensitive version because the insensitive version is documented to be deprecated?

Either way you're probably going to need to be careful about how you do string comparison against the different parts.

Show 35 quoted lines

>  		if (store->is_keys_section) {
>  			store->section_seen = 1;
>  			ALLOC_GROW(store->seen, store->seen_nr + 1,
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 03c223708eb..8325d4495f4 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1218,6 +1218,24 @@ test_expect_success 'last one wins: three level vars' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'setting different case subsections ' '
> +	test_when_finished "rm -f caseSens caseSens_actual caseSens_expect" &&
> +
> +	# v.a.r and v.A.r are not the same variable, as the middle
> +	# level of a three-level configuration variable name is
> +	# case sensitive.
> +	git config -f caseSens v."A".r VAL &&
> +	git config -f caseSens v."a".r val &&
> +
> +	echo VAL >caseSens_expect &&
> +	git config -f caseSens v."A".r >caseSens_actual &&
> +	test_cmp caseSens_expect caseSens_actual &&
> +
> +	echo val >caseSens_expect &&
> +	git config -f caseSens v."a".r >caseSens_actual &&
> +	test_cmp caseSens_expect caseSens_actual
> +'
> +
>  for VAR in a .a a. a.0b a."b c". a."b c".0d
>  do
>  	test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" '
> -- 
> 2.18.0.345.g5c9ce644c3-goog
>

-- 
Brandon Williams