From: Lars Schneider <larsxschneider@gmail.com>
Date: Thu, 16 Feb 2017 10:30:28 GMT
Subject: Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
Message-ID: <D0CDD1AC-05CA-47F3-8CB5-61EA1C6515A8@gmail.com>
In-Reply-To: <xmqqa89n10df.fsf_-_@gitster.mtv.corp.google.com>


> On 16 Feb 2017, at 00:48, Junio C Hamano <gitster@pobox.com> wrote:
> 
> The "git -c <var>=<val> cmd" mechanism is to pretend that a

The problem is also present for gitconfig variables e.g.
git config --local submodule.UPPERSUB.update none

But your patch below fixes that, too!


> configuration variable <var> is set to <val> while the cmd is
> running.  The code to do so however downcased <var> in its entirety,
> which is wrong for a three-level <section>.<subsection>.<variable>.
> 
> The <subsection> part needs to stay as-is.
> 
> Reported-by: Lars Schneider <larsxschneider@gmail.com>
> Diagnosed-by: Jonathan Tan <jonathantanmy@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> config.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/config.c b/config.c
> index 0dfed682b8..e9b93b5304 100644
> --- a/config.c
> +++ b/config.c
> @@ -199,6 +199,26 @@ void git_config_push_parameter(const char *text)
> 	strbuf_release(&env);
> }
> 
> +/*
> + * downcase the <section> and <variable> in <section>.<variable> or
> + * <section>.<subsection>.<variable> and do so in place.  <subsection>
> + * is left intact.
> + */
> +static void canonicalize_config_variable_name(char *varname)
> +{
> +	char *dot, *cp;
> +
> +	dot = strchr(varname, '.');
minor nit:
I think it would improve readability if we call this "first_dot" ...


> +	if (dot)
> +		for (cp = varname; cp < dot; cp++)
> +			*cp = tolower(*cp);
> +	dot = strrchr(varname, '.');
... and this "last_dot"?


> +	if (dot)
> +		for (cp = dot + 1; *cp; cp++)
> +			*cp = tolower(*cp);
> +}
> +
> +
> int git_config_parse_parameter(const char *text,
> 			       config_fn_t fn, void *data)
> {
> @@ -221,7 +241,7 @@ int git_config_parse_parameter(const char *text,
> 		strbuf_list_free(pair);
> 		return error("bogus config parameter: %s", text);
> 	}
> -	strbuf_tolower(pair[0]);
> +	canonicalize_config_variable_name(pair[0]->buf);
> 	if (fn(pair[0]->buf, value, data) < 0) {
> 		strbuf_list_free(pair);
> 		return -1;
> -- 
> 2.12.0-rc1-258-g3d3d1e383b
> 


The patch looks good to me and fixes the problem!

Should we add a test case to this patch?
If not, do you want me to improve my test case patch [1] 
or do you want to ditch the test?

Thank you,
Lars


[1] http://public-inbox.org/git/20170215111704.78320-1-larsxschneider@gmail.com/