Re: [PATCH v1] config: fix case-insensitive match for old-style [section.subsection]

[Junio C Hamano <gitster@pobox.com>]

RISHAV DEWAN <rishavdewan10@gmail.com> writes:

Thanks.

Although this was originally CC'ed to me, according to "git blame",
most of the code involved in the "bug" (i.e., matches() and its
caller) seems to be work by Dscho around 2018, so I'll redirect this
review in that direction ;-0.

> When writing to an old-style "[section.subsection]" header, the
> config file parser always folds the subsection to lower case
> (get_base_var() lowercases unconditionally), while the key given on
> the command line keeps whatever case the caller typed
> (do_parse_config_key() deliberately leaves the subsection segment
> untouched). matches() compared these two forms with a plain strcmp(),
> so a caller passing an upper-cased subsection (e.g. 'git config
> section.Subsection.key value2' against a file containing
> '[section.subsection]') never matched the existing key and a
> duplicate line was appended instead of replacing the value. This was
> a known, documented limitation (see the now-removed BUGS section of
> git-config.adoc).
>
> Track whether the currently active section was parsed case-
> insensitively (old-style) or case-sensitively (new-style, quoted) in
> struct config_store_data, and have matches() use that information to
> compare the subsection segment of the key accordingly, instead of an
> unconditional case-sensitive strcmp().
>
> Signed-off-by: RISHAV DEWAN <rishavdewan10@gmail.com>
> ---
>  Documentation/git-config.adoc | 21 ---------------------
>  config.c                      | 18 +++++++++++++++++-
>  t/t1300-config.sh             |  2 --
>  3 files changed, 17 insertions(+), 24 deletions(-)
>
> diff --git a/Documentation/git-config.adoc b/Documentation/git-config.adoc
> index 57af010ade..11dfd802b4 100644
> --- a/Documentation/git-config.adoc
> +++ b/Documentation/git-config.adoc
> @@ -634,27 +634,6 @@ http.sslverify false
>  
>  include::config.adoc[]
>  
> -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
> diff --git a/config.c b/config.c
> index 6a0de86e3a..7f086fcd75 100644
> --- a/config.c
> +++ b/config.c
> @@ -2594,6 +2594,7 @@ struct config_store_data {
>  	} *parsed;
>  	unsigned int parsed_nr, parsed_alloc, *seen, seen_nr, seen_alloc;
>  	unsigned int key_seen:1, section_seen:1, is_keys_section:1;
> +	unsigned int subsection_case_sensitive:1;
>  };
>  #define CONFIG_STORE_INIT { 0 }
>  
> @@ -2613,7 +2614,21 @@ static void config_store_data_clear(struct config_store_data *store)
>  static int matches(const char *key, const char *value,
>  		   const struct config_store_data *store)
>  {
> -	if (strcmp(key, store->key))
> +	/*
> +	 * The subsection part of "key" (key[0..store->baselen)) was parsed
> +	 * out of the config file using the case sensitivity of whichever
> +	 * section header it came from (see store_aux_event()): old-style
> +	 * "[section.subsection]" headers are folded to lower case while
> +	 * parsing, so they must be compared case-insensitively against
> +	 * store->key, which preserves whatever case the caller passed on
> +	 * the command line. New-style "[section "Subsection"]" headers keep
> +	 * their case, so they need an exact, case-sensitive comparison.
> +	 */
> +	int (*cmpfn)(const char *, const char *, size_t) =
> +		store->subsection_case_sensitive ? strncasecmp : strncmp;
> +
> +	if (cmpfn(key, store->key, store->baselen) ||
> +	    strcmp(key + store->baselen, store->key + store->baselen))
>  		return 0; /* not ours */
>  	if (store->fixed_value && value)
>  		return !strcmp(store->fixed_value, value);
> @@ -2654,6 +2669,7 @@ static int store_aux_event(enum config_event_t type, size_t begin, size_t end,
>  			!cmpfn(cs->var.buf, store->key, store->baselen);
>  		if (store->is_keys_section) {
>  			store->section_seen = 1;
> +			store->subsection_case_sensitive = cs->subsection_case_sensitive;
>  			ALLOC_GROW(store->seen, store->seen_nr + 1,
>  				   store->seen_alloc);
>  			store->seen[store->seen_nr] = store->parsed_nr;
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 87ca11a127..eaa3b83990 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1499,7 +1499,6 @@ test_expect_success 'old-fashioned settings are case insensitive' '
>  	EOF
>  	q_to_tab >testConfig_expect <<-EOF &&
>  	[V.A]
> -	r = value1
>  	Qr = value2
>  	EOF
>  	git config -f testConfig_actual "V.A.r" value2 &&
> @@ -1511,7 +1510,6 @@ test_expect_success 'old-fashioned settings are case insensitive' '
>  	EOF
>  	q_to_tab >testConfig_expect <<-EOF &&
>  	[V.A]
> -	r = value1
>  	Qr = value2
>  	EOF
>  	git config -f testConfig_actual "v.A.r" value2 &&