[BUG] submodule config does not apply to upper case submodules?

[BUG] submodule config does not apply to upper case submodules?

[Lars Schneider]

It looks like as if submodule configs ("submodule.*") for submodules
with upper case names are ignored. The test cases shows that skipping
a submodule during a recursive clone seems not to work.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---

I observed the bug on Windows, macOS, and Linux and at least on
v2.11.0 and v2.11.1:
https://travis-ci.org/larsxschneider/git/builds/201828672

Right now I have no time to fix it but I might be able to look into it
next week (if no one else tackles it before that).

Cheers,
Lars


Notes:
    Base Commit: 3b9e3c2ced (v2.11.1)
    Diff on Web: https://github.com/larsxschneider/git/commit/a122feaf9f
    Checkout:    git fetch https://github.com/larsxschneider/git submodule/uppercase-bug-v1 && git checkout a122feaf9f

 t/t7400-submodule-basic.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b77cce8e40..83b5c0d1e0 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1116,5 +1116,39 @@ test_expect_success 'submodule helper list is not confused by common prefixes' '
 	test_cmp expect actual
 '

+test_expect_success 'submodule config does not apply to upper case submodules' '
+	test_when_finished "rm -rf super lowersub clone-success clone-failure" &&
+	mkdir lowersub &&
+	(
+		cd lowersub &&
+		git init &&
+		>t &&
+		git add t &&
+		git commit -m "initial commit lowersub"
+	) &&
+	mkdir UPPERSUB &&
+	(
+		cd UPPERSUB &&
+		git init &&
+		>t &&
+		git add t &&
+		git commit -m "initial commit UPPERSUB"
+	) &&
+	mkdir super &&
+	(
+		cd super &&
+		git init &&
+		>t &&
+		git add t &&
+		git commit -m "initial commit super" &&
+		git submodule add ../lowersub &&
+		git submodule add ../UPPERSUB &&
+		git commit -m "add submodules"
+	) &&
+	git -c submodule.lowersub.update=none clone --recursive super clone-success 2>&1 |
+		grep "Skipping submodule" &&
+	git -c submodule.UPPERSUB.update=none clone --recursive super clone-failure 2>&1 |
+		grep "Skipping submodule"
+'

 test_done

base-commit: 3b9e3c2cede15057af3ff8076c45ad5f33829436
--
2.11.0

[PATCH v1] t7400: cleanup "submodule add clone shallow submodule" test

[Lars Schneider]

The test creates a "super" directory that is not removed after the
test finished. This directory is not used in any subsequent tests and
should therefore be removed.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---

I just noticed that my bug report test does not run properly without this
patch: http://public-inbox.org/git/20170215111704.78320-1-larsxschneider@gmail.com/

@Junio: I think this patch should be applied regardless of the bug.

Thank,
Lars

Notes:
    Base Commit: 3b9e3c2ced (v2.11.1)
    Diff on Web: https://github.com/larsxschneider/git/commit/1699a6894e
    Checkout:    git fetch https://github.com/larsxschneider/git submodule/cleanup-test-v1 && git checkout 1699a6894e

 t/t7400-submodule-basic.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b77cce8e40..08df483280 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1078,6 +1078,7 @@ test_expect_success 'submodule with UTF-8 name' '
 '

 test_expect_success 'submodule add clone shallow submodule' '
+	test_when_finished "rm -rf super" &&
 	mkdir super &&
 	pwd=$(pwd) &&
 	(

base-commit: 3b9e3c2cede15057af3ff8076c45ad5f33829436
--
2.11.0

Re: [BUG] submodule config does not apply to upper case submodules?

[Stefan Beller]

On Wed, Feb 15, 2017 at 3:17 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
> It looks like as if submodule configs ("submodule.*") for submodules
> with upper case names are ignored. The test cases shows that skipping
> a submodule during a recursive clone seems not to work.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>
> I observed the bug on Windows, macOS, and Linux and at least on
> v2.11.0 and v2.11.1:
> https://travis-ci.org/larsxschneider/git/builds/201828672

Thanks for the bug report.

>
> Right now I have no time to fix it but I might be able to look into it
> next week (if no one else tackles it before that).

I might look into it before next week.

> Notes:
>     Base Commit: 3b9e3c2ced (v2.11.1)
>     Diff on Web: https://github.com/larsxschneider/git/commit/a122feaf9f
>     Checkout:    git fetch https://github.com/larsxschneider/git submodule/uppercase-bug-v1 && git checkout a122feaf9f

I like these notes, though base commit is duplicate with below.


> +test_expect_success 'submodule config does not apply to upper case submodules' '
...
> +               git submodule add ../UPPERSUB &&
> +               git commit -m "add submodules"
> +       ) &&

up to here we only do "setup"-sy stuff.
("setup being a trigger word that you cannot omit
the test for subsequent tests to work)
So maybe have
    test_expect_success 'setup submodule with lower and uppercase' '
    ...
    '
    test_expect_success 'just the clone' '
    ...
    '
    test_expect_success ' check for lower case'
        grep -e "Skipping submodule *lowersub*" err
    '
    test_expect_failure ' check for upper case'
        grep ...
    '
> +       git -c submodule.lowersub.update=none clone --recursive super clone-success 2>&1 |
> +               grep "Skipping submodule" &&
> +       git -c submodule.UPPERSUB.update=none clone --recursive super clone-failure 2>&1 |
> +               grep "Skipping submodule"

I'd rather give both options in one invocation and then grep from a file, e.g.

    git -c submodule.lowersub.update=none -c submodule.UPPERSUB.update=none \
        clone --recursive super super_clone 2>err 1>out &&
    grep -e "Skipping submodule *lowersub*" err

> +'
>
>  test_done
>

> base-commit: 3b9e3c2cede15057af3ff8076c45ad5f33829436

Heh, I see what you did here. :)

> --
> 2.11.0
>

Re: [PATCH v1] t7400: cleanup "submodule add clone shallow submodule" test

[Stefan Beller]

On Wed, Feb 15, 2017 at 3:33 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
> @Junio: I think this patch should be applied regardless of the bug.

Sounds good; thanks!
Stefan

Re: [PATCH v1] t7400: cleanup "submodule add clone shallow submodule" test

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

> The test creates a "super" directory that is not removed after the
> test finished. This directory is not used in any subsequent tests and
> should therefore be removed.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>
> I just noticed that my bug report test does not run properly without this
> patch: http://public-inbox.org/git/20170215111704.78320-1-larsxschneider@gmail.com/
>
> @Junio: I think this patch should be applied regardless of the bug.

Without the other one, this is not strictly needed, but I agree that
it is a good code hygiene to make sure each test cleans up after
itself.

Is this the only one that needs change in the script from that
"hygiene" point of view, or are there others?  An alternative that
is also acceptable is to squash this one into the other patch.

>  t/t7400-submodule-basic.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index b77cce8e40..08df483280 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -1078,6 +1078,7 @@ test_expect_success 'submodule with UTF-8 name' '
>  '
>
>  test_expect_success 'submodule add clone shallow submodule' '
> +	test_when_finished "rm -rf super" &&
>  	mkdir super &&
>  	pwd=$(pwd) &&
>  	(
>
> base-commit: 3b9e3c2cede15057af3ff8076c45ad5f33829436
> --
> 2.11.0

Re: [BUG] submodule config does not apply to upper case submodules?

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

> It looks like as if submodule configs ("submodule.*") for submodules
> with upper case names are ignored.

This observation is surprising, as the second level in three-level
names like "<section>.<name>.<variable>" is designed to be case
sensitive.  A code that uses the config API needs to do extra things
to cause the behaviour you showed, i.e. to get submodule.U.update
ignored while submodule.l.update to be honoured.  Perhaps somebody
downcases things too aggressively before comparing?

This is worth making it work as expected, needless to say ;-)

Re: [BUG] submodule config does not apply to upper case submodules?

[Jonathan Tan]

On 02/15/2017 10:53 AM, Junio C Hamano wrote:
> Lars Schneider <larsxschneider@gmail.com> writes:
>
>> It looks like as if submodule configs ("submodule.*") for submodules
>> with upper case names are ignored.
>
> This observation is surprising, as the second level in three-level
> names like "<section>.<name>.<variable>" is designed to be case
> sensitive.  A code that uses the config API needs to do extra things
> to cause the behaviour you showed, i.e. to get submodule.U.update
> ignored while submodule.l.update to be honoured.  Perhaps somebody
> downcases things too aggressively before comparing?
>
> This is worth making it work as expected, needless to say ;-)

I had some time to look into this, and yes, command-line parameters are 
too aggressively downcased ("git_config_parse_parameter" calls 
"strbuf_tolower" on the entire key part in config.c). Updating the 
original patch to use "test_global_config" makes the test pass, and 
commenting out the "strbuf_tolower" line in config.c also makes the test 
pass.

I'll see if I can fix this.

Re: [BUG] submodule config does not apply to upper case submodules?

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> I had some time to look into this, and yes, command-line parameters
> are too aggressively downcased ("git_config_parse_parameter" calls
> "strbuf_tolower" on the entire key part in config.c).

Ahh, thanks.  So this is not about submodules at all; it is -c var=VAL
where var is downcased too aggressively.

Re: [BUG] submodule config does not apply to upper case submodules?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> I had some time to look into this, and yes, command-line parameters
>> are too aggressively downcased ("git_config_parse_parameter" calls
>> "strbuf_tolower" on the entire key part in config.c).
>
> Ahh, thanks.  So this is not about submodules at all; it is -c var=VAL
> where var is downcased too aggressively.

Perhaps something like this?

 config.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index c6b874a7bf..98bf8fee32 100644
--- a/config.c
+++ b/config.c
@@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)
 	strbuf_release(&env);
 }
 
+static void canonicalize_config_variable_name(struct strbuf *var)
+{
+	char *first_dot = strchr(var->buf, '.');
+	char *last_dot = strrchr(var->buf, '.');
+	char *cp;
+
+	if (first_dot)
+		for (cp = var->buf; *cp && cp < first_dot; cp++)
+			*cp = tolower(*cp);
+	if (last_dot)
+		for (cp = last_dot; *cp; cp++)
+			*cp = tolower(*cp);
+}
+
 int git_config_parse_parameter(const char *text,
 			       config_fn_t fn, void *data)
 {
@@ -223,7 +237,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]);
 	if (fn(pair[0]->buf, value, data) < 0) {
 		strbuf_list_free(pair);
 		return -1;

Re: [BUG] submodule config does not apply to upper case submodules?

[Stefan Beller]

On Wed, Feb 15, 2017 at 3:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jonathan Tan <jonathantanmy@google.com> writes:
>>
>>> I had some time to look into this, and yes, command-line parameters
>>> are too aggressively downcased ("git_config_parse_parameter" calls
>>> "strbuf_tolower" on the entire key part in config.c).
>>
>> Ahh, thanks.  So this is not about submodules at all; it is -c var=VAL
>> where var is downcased too aggressively.
>
> Perhaps something like this?

Yes; though I'd place it in strbuf.{c,h} as it is operating
on the internals of the strbuf. (Do we make any promises outside of
strbuf about the internals? I mean we use .buf all the time, so maybe
I am overly cautious here)

>
>  config.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/config.c b/config.c
> index c6b874a7bf..98bf8fee32 100644
> --- a/config.c
> +++ b/config.c
> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)
>         strbuf_release(&env);
>  }
>
> +static void canonicalize_config_variable_name(struct strbuf *var)
> +{
> +       char *first_dot = strchr(var->buf, '.');
> +       char *last_dot = strrchr(var->buf, '.');

If first_dot != NULL, then last_dot !+ NULL as well.
(either both are NULL or none of them),
so we can loose one condition below.

> +       char *cp;
> +
> +       if (first_dot)
> +               for (cp = var->buf; *cp && cp < first_dot; cp++)
> +                       *cp = tolower(*cp);
> +       if (last_dot)
> +               for (cp = last_dot; *cp; cp++)
> +                       *cp = tolower(*cp);
> +}
> +
>  int git_config_parse_parameter(const char *text,
>                                config_fn_t fn, void *data)

Re: [BUG] submodule config does not apply to upper case submodules?

[Jonathan Tan]

On 02/15/2017 03:11 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> Perhaps something like this?

This looks good. I was hoping to unify the processing logic between this 
CLI parsing and the usual stream parsing, but this approach is probably 
simpler.

>  config.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/config.c b/config.c
> index c6b874a7bf..98bf8fee32 100644
> --- a/config.c
> +++ b/config.c
> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)
>  	strbuf_release(&env);
>  }
>
> +static void canonicalize_config_variable_name(struct strbuf *var)
> +{
> +	char *first_dot = strchr(var->buf, '.');
> +	char *last_dot = strrchr(var->buf, '.');
> +	char *cp;
> +
> +	if (first_dot)
> +		for (cp = var->buf; *cp && cp < first_dot; cp++)

"*cp &&" is unnecessary, as far as I can tell.

> +			*cp = tolower(*cp);
> +	if (last_dot)
> +		for (cp = last_dot; *cp; cp++)
> +			*cp = tolower(*cp);
> +}
> +
>  int git_config_parse_parameter(const char *text,
>  			       config_fn_t fn, void *data)
>  {
> @@ -223,7 +237,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]);
>  	if (fn(pair[0]->buf, value, data) < 0) {
>  		strbuf_list_free(pair);
>  		return -1;

Re: [BUG] submodule config does not apply to upper case submodules?

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Yes; though I'd place it in strbuf.{c,h} as it is operating
> on the internals of the strbuf. (Do we make any promises outside of
> strbuf about the internals? I mean we use .buf all the time, so maybe
> I am overly cautious here)

I'd rather have it not use struct strbuf as an interface.  It only
needs to pass "char *" and its promise that it touches the string
in-place without changing the length need to be documented as a
comment before the function.

>>  config.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/config.c b/config.c
>> index c6b874a7bf..98bf8fee32 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)
>>         strbuf_release(&env);
>>  }
>>
>> +static void canonicalize_config_variable_name(struct strbuf *var)
>> +{
>> +       char *first_dot = strchr(var->buf, '.');
>> +       char *last_dot = strrchr(var->buf, '.');
>
> If first_dot != NULL, then last_dot !+ NULL as well.
> (either both are NULL or none of them),
> so we can loose one condition below.

I do not think it is worth it, though.

>> +       char *cp;
>> +
>> +       if (first_dot)
>> +               for (cp = var->buf; *cp && cp < first_dot; cp++)
>> +                       *cp = tolower(*cp);
>> +       if (last_dot)
>> +               for (cp = last_dot; *cp; cp++)
>> +                       *cp = tolower(*cp);

	if (first_dot) {
		scan up to first dot
		if (last_dot)
			scan from last dot to the end
	}

would be uglier.

Re: [BUG] submodule config does not apply to upper case submodules?

[Stefan Beller]

On Wed, Feb 15, 2017 at 3:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Yes; though I'd place it in strbuf.{c,h} as it is operating
>> on the internals of the strbuf. (Do we make any promises outside of
>> strbuf about the internals? I mean we use .buf all the time, so maybe
>> I am overly cautious here)
>
> I'd rather have it not use struct strbuf as an interface.  It only
> needs to pass "char *" and its promise that it touches the string
> in-place without changing the length need to be documented as a
> comment before the function.
>
>>>  config.c | 16 +++++++++++++++-
>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/config.c b/config.c
>>> index c6b874a7bf..98bf8fee32 100644
>>> --- a/config.c
>>> +++ b/config.c
>>> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)
>>>         strbuf_release(&env);
>>>  }
>>>
>>> +static void canonicalize_config_variable_name(struct strbuf *var)
>>> +{
>>> +       char *first_dot = strchr(var->buf, '.');
>>> +       char *last_dot = strrchr(var->buf, '.');
>>
>> If first_dot != NULL, then last_dot !+ NULL as well.
>> (either both are NULL or none of them),
>> so we can loose one condition below.
>
> I do not think it is worth it, though.
>
>>> +       char *cp;
>>> +
>>> +       if (first_dot)
>>> +               for (cp = var->buf; *cp && cp < first_dot; cp++)
>>> +                       *cp = tolower(*cp);
>>> +       if (last_dot)
>>> +               for (cp = last_dot; *cp; cp++)
>>> +                       *cp = tolower(*cp);
>
>         if (first_dot) {
>                 scan up to first dot
>                 if (last_dot)

just leave out the 'if (last_dot)' ?

    if (first_dot)  {
        /* also implies last_dot */
        do 0 -> first
        do last -> end
    }

[PATCH] config: preserve <subsection> case for one-shot config on the command line

[Junio C Hamano]

The "git -c <var>=<val> cmd" mechanism is to pretend that a
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, '.');
+	if (dot)
+		for (cp = varname; cp < dot; cp++)
+			*cp = tolower(*cp);
+	dot = strrchr(varname, '.');
+	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

Re: [BUG] submodule config does not apply to upper case submodules?

[Junio C Hamano]

Ahh, that would work, too.

On Wed, Feb 15, 2017 at 3:43 PM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Feb 15, 2017 at 3:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> Yes; though I'd place it in strbuf.{c,h} as it is operating
>>> on the internals of the strbuf. (Do we make any promises outside of
>>> strbuf about the internals? I mean we use .buf all the time, so maybe
>>> I am overly cautious here)
>>
>> I'd rather have it not use struct strbuf as an interface.  It only
>> needs to pass "char *" and its promise that it touches the string
>> in-place without changing the length need to be documented as a
>> comment before the function.
>>
>>>>  config.c | 16 +++++++++++++++-
>>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/config.c b/config.c
>>>> index c6b874a7bf..98bf8fee32 100644
>>>> --- a/config.c
>>>> +++ b/config.c
>>>> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)
>>>>         strbuf_release(&env);
>>>>  }
>>>>
>>>> +static void canonicalize_config_variable_name(struct strbuf *var)
>>>> +{
>>>> +       char *first_dot = strchr(var->buf, '.');
>>>> +       char *last_dot = strrchr(var->buf, '.');
>>>
>>> If first_dot != NULL, then last_dot !+ NULL as well.
>>> (either both are NULL or none of them),
>>> so we can loose one condition below.
>>
>> I do not think it is worth it, though.
>>
>>>> +       char *cp;
>>>> +
>>>> +       if (first_dot)
>>>> +               for (cp = var->buf; *cp && cp < first_dot; cp++)
>>>> +                       *cp = tolower(*cp);
>>>> +       if (last_dot)
>>>> +               for (cp = last_dot; *cp; cp++)
>>>> +                       *cp = tolower(*cp);
>>
>>         if (first_dot) {
>>                 scan up to first dot
>>                 if (last_dot)
>
> just leave out the 'if (last_dot)' ?
>
>     if (first_dot)  {
>         /* also implies last_dot */
>         do 0 -> first
>         do last -> end
>     }

Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line

[Lars Schneider]

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

Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

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

Yuck.

> But your patch below fixes that, too!

Heh.

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

I think a new test for submodule (although it already exists thanks
t you) is  a bit too roundabout way to demonstrate the breakage and
protect the fix.

We'd perhaps want some tests for "git config", probably.

In a repository with /etc/gitconfig and $HOME/ that are tightly
controlled (I think our test framework already do so), running these
would demonstrate both breakages, I would think, but I am sure
people can come up with other forms that are a lot easier to read..

    $ git -c v.A.r=val -c v.a.r=ue config --get-all v.a.r
    $ git -c v.A.r=val -c v.a.r=ue config --get-all v.A.r
    $ git -c v.a.r=val -c v.A.r=ue config --get-all v.a.r
    $ git -c v.a.r=val -c v.A.r=ue config --get-all v.A.r

Thanks.

Re: [BUG] submodule config does not apply to upper case submodules?

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> On 02/15/2017 03:11 PM, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> Perhaps something like this?
>
> This looks good. I was hoping to unify the processing logic between
> this CLI parsing and the usual stream parsing, but this approach is
> probably simpler.

I looked at the stream parsing before I wrote the patch for the same
reason, and I agree that the stream parsing of course does not get a
single variable name at one place [*1*], so there is nothing that
can be shared.

[Footnote]

*1* Instead "[<section> "<subsection>"] <var> = <val>" is split and
    each piece is assembled into section.subsection.var with its own
    downcasing.

Re: [BUG] submodule config does not apply to upper case submodules?

[Jeff King]

On Wed, Feb 15, 2017 at 03:37:37PM -0800, Junio C Hamano wrote:

> Stefan Beller <sbeller@google.com> writes:
> 
> > Yes; though I'd place it in strbuf.{c,h} as it is operating
> > on the internals of the strbuf. (Do we make any promises outside of
> > strbuf about the internals? I mean we use .buf all the time, so maybe
> > I am overly cautious here)
> 
> I'd rather have it not use struct strbuf as an interface.  It only
> needs to pass "char *" and its promise that it touches the string
> in-place without changing the length need to be documented as a
> comment before the function.

This code also uses the hacky strbuf_split() interface. It would be nice
to one day move off of it (the only other strbuf-specific function used
there is strbuf_trim).

One _could_ actually parse the whole thing left-to-right (soaking up
whitespace and doing the canonicalizing) instead of dealing with a split
function at all. But the canonicalize bit you added here would not be
reusable then. And it's probably not worth holding up the bugfix here.

> >> +static void canonicalize_config_variable_name(struct strbuf *var)
> >> +{
> >> +       char *first_dot = strchr(var->buf, '.');
> >> +       char *last_dot = strrchr(var->buf, '.');
> >
> > If first_dot != NULL, then last_dot !+ NULL as well.
> > (either both are NULL or none of them),
> > so we can loose one condition below.
> 
> I do not think it is worth it, though.

If you really want to be picky, you do not need to find the first dot
at all. You can downcase everything until you see a dot, and then
find the last dot (if any) from there.

I don't think it matters much in practice.

-Peff

Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line

[Jeff King]

On Thu, Feb 16, 2017 at 11:30:28AM +0100, Lars Schneider wrote:

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

Hrm, is it?

  $ git config --file foo submodule.UPPERSUB.update none
  $ cat foo
  [submodule "UPPERSUB"]
	update = none

I could believe that some of the submodule code may try to pass it
through "-c", though, so certain config ends up being missed.

AFAICT, though, the writing code takes what you gave it verbatim. The
reader is responsible for downcasing everything but the subsection
before it hands it to a callback. Commands calling git-config for lookup
should generally ask for the canonical downcased name. There is some
code to downcase, but IIRC there are corner cases around some of the
regex lookup functions.

-Peff

Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Feb 16, 2017 at 11:30:28AM +0100, Lars Schneider wrote:
>
>> 
>> > 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
>
> Hrm, is it?
>
>   $ git config --file foo submodule.UPPERSUB.update none
>   $ cat foo
>   [submodule "UPPERSUB"]
> 	update = none
>
> I could believe that some of the submodule code may try to pass it
> through "-c", though, so certain config ends up being missed.

You are right.  

The builtin/config.c::get_value() codepath, when it is not using the
hacky regexp interface, uses config.c::git_config_parse_key(), and
it does the right thing.  git_config_parse_parameter(), which is the
broken one we found, is not used.

When I did the patch in response to Jonathan's observation, I did
briefly wonder if it should be using git_config_parse_key() instead
of doing its own thing, but I didn't follow it up fully before
deciding that it is the quickest to replace the tolower thing.  If I
at least tried to see if it is feasible, I would have noticed that
the query from the command line wouldn't share the same problem as
Lars reported.

I still haven't queued any of the variants I posted (and I do not
think other people sent their own versions, either).  I need to pick
one and queue, with a test or two.  Perhaps after -rc2.  

Others are welcome to work on it while I cut -rc2 tomorrow, so that
by the time I see their patch all that is left for me to do is to
apply it ;-)

Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I still haven't queued any of the variants I posted (and I do not
> think other people sent their own versions, either).  I need to pick
> one and queue, with a test or two.  Perhaps after -rc2.  
>
> Others are welcome to work on it while I cut -rc2 tomorrow, so that
> by the time I see their patch all that is left for me to do is to
> apply it ;-)

Since nothing seems to have happened in the meantime, here is what
I'll queue so that we won't forget for now.  Lars's tests based on
how the scripted "git submodule" uses "git config" may still be
valid, but it is somewhat a roundabout way to demonstrate the
breakage and not very effective way to protect the fix, so I added a
new test that directly tests "git -c <var>=<val> <command>".

I am not sure if this updated one is worth doing, or the previous
"strchr and strrchr" I originally wrote was easier to understand.

One thing I noticed is that "git config --get X" will correctly
diagnose that a dot-less X is not a valid variable name, but we do
not seem to diagnose "git -c X=V <cmd>" as invalid.

Perhaps we should, but it is not the focus of this topic.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 15 Feb 2017 15:48:44 -0800
Subject: [PATCH] config: preserve <subsection> case for one-shot config on the
 command line

The "git -c <var>=<val> cmd" mechanism is to pretend that a
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                  | 30 ++++++++++++++++++++++++++++-
 t/t1351-config-cmdline.sh | 48 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100755 t/t1351-config-cmdline.sh

diff --git a/config.c b/config.c
index 0dfed682b8..ba9a5911b0 100644
--- a/config.c
+++ b/config.c
@@ -199,6 +199,34 @@ 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 *cp, *last_dot;
+
+	/* downcase the first segment */
+	for (cp = varname; *cp; cp++) {
+		if (*cp == '.')
+			break;
+		*cp = tolower(*cp);
+	}
+	if (!*cp)
+		return;
+
+	/* scan for the last dot */
+	for (last_dot = cp; *cp; cp++)
+		if (*cp == '.')
+			last_dot = cp;
+
+	/* downcase the last segment */
+	for (cp = last_dot; *cp; cp++)
+		*cp = tolower(*cp);
+}
+
 int git_config_parse_parameter(const char *text,
 			       config_fn_t fn, void *data)
 {
@@ -221,7 +249,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;
diff --git a/t/t1351-config-cmdline.sh b/t/t1351-config-cmdline.sh
new file mode 100755
index 0000000000..acb8dc3b15
--- /dev/null
+++ b/t/t1351-config-cmdline.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+test_description='git -c var=val'
+
+. ./test-lib.sh
+
+test_expect_success 'last one wins: two level vars' '
+	echo VAL >expect &&
+
+	# sec.var and sec.VAR are the same variable, as the first
+	# and the last level of a configuration variable name is
+	# case insensitive.  Test both setting and querying.
+
+	git -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&
+	test_cmp expect actual &&
+	git -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&
+	test_cmp expect actual &&
+
+	git -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&
+	test_cmp expect actual &&
+	git -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'last one wins: three level vars' '
+	echo val >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.  Test both setting and querying.
+
+	git -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&
+	test_cmp expect actual &&
+	git -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&
+	test_cmp expect actual &&
+
+	echo VAL >expect &&
+	git -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&
+	test_cmp expect actual &&
+	git -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&
+	test_cmp expect actual &&
+	git -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&
+	test_cmp expect actual &&
+	git -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.12.0-rc2-221-g8fa194a99f

Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line

[Lars Schneider]

> On 20 Feb 2017, at 10:58, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> I still haven't queued any of the variants I posted (and I do not
>> think other people sent their own versions, either).  I need to pick
>> one and queue, with a test or two.  Perhaps after -rc2.  
>> 
>> Others are welcome to work on it while I cut -rc2 tomorrow, so that
>> by the time I see their patch all that is left for me to do is to
>> apply it ;-)
> 
> Since nothing seems to have happened in the meantime, here is what
> I'll queue so that we won't forget for now.  Lars's tests based on
> how the scripted "git submodule" uses "git config" may still be
> valid, but it is somewhat a roundabout way to demonstrate the
> breakage and not very effective way to protect the fix, so I added a
> new test that directly tests "git -c <var>=<val> <command>".

Agreed. Please ignore my tests.
If you want to you could queue this tiny cleanup, though:
http://public-inbox.org/git/20170215113325.14393-1-larsxschneider@gmail.com/

> ...
> 
> +/*
> + * 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 *cp, *last_dot;

What does cp stand for? "char pointer"?

> +
> +	/* downcase the first segment */
> +	for (cp = varname; *cp; cp++) {
> +		if (*cp == '.')
> +			break;
> +		*cp = tolower(*cp);
> +	}
> +	if (!*cp)
> +		return;
> +
> +	/* scan for the last dot */
> +	for (last_dot = cp; *cp; cp++)
> +		if (*cp == '.')
> +			last_dot = cp;
> +
> +	/* downcase the last segment */
> +	for (cp = last_dot; *cp; cp++)
> +		*cp = tolower(*cp);
> +}
> +
> int git_config_parse_parameter(const char *text,
> 			       config_fn_t fn, void *data)
> {
> @@ -221,7 +249,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;
> diff --git a/t/t1351-config-cmdline.sh b/t/t1351-config-cmdline.sh
> new file mode 100755
> index 0000000000..acb8dc3b15
> --- /dev/null
> +++ b/t/t1351-config-cmdline.sh
> @@ -0,0 +1,48 @@
> +#!/bin/sh
> +
> +test_description='git -c var=val'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'last one wins: two level vars' '
> +	echo VAL >expect &&
> +
> +	# sec.var and sec.VAR are the same variable, as the first
> +	# and the last level of a configuration variable name is
> +	# case insensitive.  Test both setting and querying.
> +
> +	git -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&
> +	test_cmp expect actual &&
> +	git -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&
> +	test_cmp expect actual &&
> +
> +	git -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&
> +	test_cmp expect actual &&
> +	git -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'last one wins: three level vars' '
> +	echo val >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.  Test both setting and querying.
> +
> +	git -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&
> +	test_cmp expect actual &&
> +	git -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&
> +	test_cmp expect actual &&
> +
> +	echo VAL >expect &&
> +	git -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&
> +	test_cmp expect actual &&
> +	git -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&
> +	test_cmp expect actual &&
> +	git -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&
> +	test_cmp expect actual &&
> +	git -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_done
> -- 
> 2.12.0-rc2-221-g8fa194a99f
> 

Looks good to me!

Thank you,
Lars

Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line

[Jeff King]

On Mon, Feb 20, 2017 at 01:58:07AM -0800, Junio C Hamano wrote:

> Since nothing seems to have happened in the meantime, here is what
> I'll queue so that we won't forget for now.  Lars's tests based on
> how the scripted "git submodule" uses "git config" may still be
> valid, but it is somewhat a roundabout way to demonstrate the
> breakage and not very effective way to protect the fix, so I added a
> new test that directly tests "git -c <var>=<val> <command>".

Yeah, I agree that is the best way to cover this code.

> I am not sure if this updated one is worth doing, or the previous
> "strchr and strrchr" I originally wrote was easier to understand.

I think either is OK. I would actually have written it halfway in
between, like:

  static void canonicalize_config_variable_name(char *varname)
  {
          const char *p;

          /* downcase the first segment */
          for (p = varname; *p; p++) {
	          if (*p == '.')
		          break;
                  *p = tolower(*p);
	  }

          /* locate end of middle segment, if there is one */
          p = strrchr(p, '.');
          if (!p)
                  return; /* invalid single-level var, but let it pass */
          for (; *p; p++)
                  *p = tolower(*p);
  }

You can toss that on the bikeshed heap. :)

The other change from yours is that it flips the order of the strrchr
and return. Yours is more efficient in the sense that we know there is
no dot, so we do not have to keep searching at all (though of course
this case is invalid and we would not expect to see it in practice).

But it did take me a minute in yours to figure out why last_dot was
always set to a dot (the answer is that it starts at "cp", which must
itself be a dot, because we would already have returned otherwise).

> One thing I noticed is that "git config --get X" will correctly
> diagnose that a dot-less X is not a valid variable name, but we do
> not seem to diagnose "git -c X=V <cmd>" as invalid.

I don't think that was intentional. We just never caught the case. It
might be reasonable to do so (and it's easy to catch here while
canonicalizing). I think there are probably some other cases we _could_
catch, too (e.g., invalid characters for keynames). But I'm not excited
about duplicating the logic from the file-parser.

> Perhaps we should, but it is not the focus of this topic.

Definitely.

> diff --git a/t/t1351-config-cmdline.sh b/t/t1351-config-cmdline.sh
> new file mode 100755
> index 0000000000..acb8dc3b15
> --- /dev/null
> +++ b/t/t1351-config-cmdline.sh
> @@ -0,0 +1,48 @@
> +#!/bin/sh
> +
> +test_description='git -c var=val'
> +
> +. ./test-lib.sh

There are a bunch of other "git -c" tests inside t1300. I don't know if
it would be better to put them all together.

Arguably, those other ones should get pulled out here to the new script.
More scripts means that the tests have fewer hidden dependencies, and we
can parallelize the run more. I admit I've shied away from new scripts
in the past because the number allocation is such a pain.

> +test_expect_success 'last one wins: two level vars' '
> +	echo VAL >expect &&
> +
> +	# sec.var and sec.VAR are the same variable, as the first
> +	# and the last level of a configuration variable name is
> +	# case insensitive.  Test both setting and querying.

I assume by "setting and querying" here you mean "setting via -c,
querying via git-config".

> +	git -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&
> +	test_cmp expect actual &&
> +	git -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&
> +	test_cmp expect actual &&
> +
> +	git -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&
> +	test_cmp expect actual &&
> +	git -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&
> +	test_cmp expect actual

Looks good.

> +test_expect_success 'last one wins: three level vars' '
> +	echo val >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.  Test both setting and querying.
> +
> +	git -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&
> +	test_cmp expect actual &&
> +	git -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&
> +	test_cmp expect actual &&
> +
> +	echo VAL >expect &&
> +	git -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&
> +	test_cmp expect actual &&
> +	git -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&
> +	test_cmp expect actual &&
> +	git -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&
> +	test_cmp expect actual &&
> +	git -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&
> +	test_cmp expect actual
> +'

There are two blocks of tests, each with their own "expect" value.
Should the top "expect" here be moved down with its block to make that
more clear?

Other than that nit, the tests look good to me.

-Peff

Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +. ./test-lib.sh
>
> There are a bunch of other "git -c" tests inside t1300. I don't know if
> it would be better to put them all together.

I considered it, but it is already very long and I suspect it would
be better in the longer term to split the command-line one out into
a separate script, for the reasons you state.

I can move these at the end of 1300 in the meantime, and leave the
split for somebody else to be done later.

> Arguably, those other ones should get pulled out here to the new script.
> More scripts means that the tests have fewer hidden dependencies, and we
> can parallelize the run more. I admit I've shied away from new scripts
> in the past because the number allocation is such a pain.
>
>> +test_expect_success 'last one wins: two level vars' '
>> +	echo VAL >expect &&
>> +
>> +	# sec.var and sec.VAR are the same variable, as the first
>> +	# and the last level of a configuration variable name is
>> +	# case insensitive.  Test both setting and querying.
>
> I assume by "setting and querying" here you mean "setting via -c,
> querying via git-config".

Yes.  Should I update it to be more explicit?

> There are two blocks of tests, each with their own "expect" value.
> Should the top "expect" here be moved down with its block to make that
> more clear?

Perhaps.  Let me try that one.

Thanks.

[PATCH v2] config: preserve <subsection> case for one-shot config on the command line

[Junio C Hamano]

The "git -c <var>=<val> cmd" mechanism is to pretend that a
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>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Changes from v1:

   - update the comment before the second loop to find the last
     dot.

   - move two new tests into existing t1300 (at least for now).

   - make it clear that the second of the new tests check two
     aspects of "three level vars" by setting up the expectation for
     the first half near the actual tests and adding comments on
     what it tests before the second half.

 config.c               | 30 +++++++++++++++++++++++++++++-
 t/t1300-repo-config.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 0dfed682b8..4128debc71 100644
--- a/config.c
+++ b/config.c
@@ -199,6 +199,34 @@ 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 *cp, *last_dot;
+
+	/* downcase the first segment */
+	for (cp = varname; *cp; cp++) {
+		if (*cp == '.')
+			break;
+		*cp = tolower(*cp);
+	}
+	if (!*cp)
+		return;
+
+	/* find the last dot (we start from the first dot we just found) */
+	for (last_dot = cp; *cp; cp++)
+		if (*cp == '.')
+			last_dot = cp;
+
+	/* downcase the last segment */
+	for (cp = last_dot; *cp; cp++)
+		*cp = tolower(*cp);
+}
+
 int git_config_parse_parameter(const char *text,
 			       config_fn_t fn, void *data)
 {
@@ -221,7 +249,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;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 923bfc5a26..7a16f66a9d 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1097,6 +1097,52 @@ test_expect_success 'multiple git -c appends config' '
 	test_cmp expect actual
 '
 
+test_expect_success 'last one wins: two level vars' '
+
+	# sec.var and sec.VAR are the same variable, as the first
+	# and the last level of a configuration variable name is
+	# case insensitive.
+
+	echo VAL >expect &&
+
+	git -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&
+	test_cmp expect actual &&
+	git -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&
+	test_cmp expect actual &&
+
+	git -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&
+	test_cmp expect actual &&
+	git -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'last one wins: three level vars' '
+
+	# 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.
+
+	echo val >expect &&
+	git -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&
+	test_cmp expect actual &&
+	git -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&
+	test_cmp expect actual &&
+
+	# v.a.r and V.a.R are the same variable, as the first
+	# and the last level of a configuration variable name is
+	# case insensitive.
+
+	echo VAL >expect &&
+	git -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&
+	test_cmp expect actual &&
+	git -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&
+	test_cmp expect actual &&
+	git -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&
+	test_cmp expect actual &&
+	git -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git -c is not confused by empty environment' '
 	GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
 '
-- 
2.12.0-rc2-221-ga92f6f5816

Re: [PATCH v2] config: preserve <subsection> case for one-shot config on the command line

[Jeff King]

On Tue, Feb 21, 2017 at 09:17:07AM -0800, Junio C Hamano wrote:

>  * Changes from v1:
> 
>    - update the comment before the second loop to find the last
>      dot.
> 
>    - move two new tests into existing t1300 (at least for now).
> 
>    - make it clear that the second of the new tests check two
>      aspects of "three level vars" by setting up the expectation for
>      the first half near the actual tests and adding comments on
>      what it tests before the second half.

Thanks, this addresses all of my (admittedly minor) concerns.

-Peff

Re: [PATCH v2] config: preserve <subsection> case for one-shot config on the command line

[Stefan Beller]

On Tue, Feb 21, 2017 at 9:50 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 21, 2017 at 09:17:07AM -0800, Junio C Hamano wrote:
>
>>  * Changes from v1:
>>
>>    - update the comment before the second loop to find the last
>>      dot.
>>
>>    - move two new tests into existing t1300 (at least for now).
>>
>>    - make it clear that the second of the new tests check two
>>      aspects of "three level vars" by setting up the expectation for
>>      the first half near the actual tests and adding comments on
>>      what it tests before the second half.
>
> Thanks, this addresses all of my (admittedly minor) concerns.
>
> -Peff

This patch looks different than what I last looked at. I like it.
Though the manual search of the last dot instead of using
strrchr seems to be over-optimizing to me.
Anyway it is still very understandable.

Thanks,
Stefan

[PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'

[Junio C Hamano]

The parsing of one-shot assignments of configuration variables that
come from the command line historically was quite loose and allowed
anything to pass.

The configuration variable names that come from files are validated
in git_config_parse_source(), which uses get_base_var() that grabs
the <section> (and subsection) while making sure that <section>
consists of iskeychar() letters, the function itself that makes sure
that the first letter in <variable> is isalpha(), and get_value()
that grabs the remainder of the <variable> name while making sure
that it consists of iskeychar() letters.

Perform an equivalent check in canonicalize_config_variable_name()
to catch invalid configuration variable names that come from the
command line.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

    Junio C Hamano <gitster@pobox.com> writes:

    > One thing I noticed is that "git config --get X" will correctly
    > diagnose that a dot-less X is not a valid variable name, but we do
    > not seem to diagnose "git -c X=V <cmd>" as invalid.
    >
    > Perhaps we should, but it is not the focus of this topic.

    And here is a follow-up on that tangent.

 config.c               | 48 +++++++++++++++++++++++++++++++++++++-----------
 t/t1300-repo-config.sh |  7 +++++++
 2 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 4128debc71..e7f7ff1938 100644
--- a/config.c
+++ b/config.c
@@ -199,32 +199,62 @@ void git_config_push_parameter(const char *text)
 	strbuf_release(&env);
 }
 
+static inline int iskeychar(int c)
+{
+	return isalnum(c) || c == '-';
+}
+
 /*
  * downcase the <section> and <variable> in <section>.<variable> or
  * <section>.<subsection>.<variable> and do so in place.  <subsection>
  * is left intact.
+ *
+ * The configuration variable names that come from files are validated
+ * in git_config_parse_source(), which uses get_base_var() that grabs
+ * the <section> (and subsection) while making sure that <section>
+ * consists of iskeychar() letters, the function itself that makes
+ * sure that the first letter in <variable> is isalpha(), and
+ * get_value() that grabs the remainder of the <variable> name while
+ * making sure that it consists of iskeychar() letters.  Perform a
+ * matching validation for configuration variables that come from
+ * the command line.
  */
-static void canonicalize_config_variable_name(char *varname)
+static int canonicalize_config_variable_name(char *varname)
 {
-	char *cp, *last_dot;
+	char *cp, *first_dot, *last_dot;
 
 	/* downcase the first segment */
 	for (cp = varname; *cp; cp++) {
 		if (*cp == '.')
 			break;
+		if (!iskeychar(*cp))
+			return -1;
 		*cp = tolower(*cp);
 	}
 	if (!*cp)
-		return;
+		return -1; /* no dot anywhere? */
+
+	first_dot = cp;
+	if (first_dot == varname)
+		return -1; /* no section? */
 
 	/* find the last dot (we start from the first dot we just found) */
-	for (last_dot = cp; *cp; cp++)
+	for (; *cp; cp++)
 		if (*cp == '.')
 			last_dot = cp;
 
+	if (!last_dot[1])
+		return -1; /* no variable? */
+
 	/* downcase the last segment */
-	for (cp = last_dot; *cp; cp++)
+	for (cp = last_dot + 1; *cp; cp++) {
+		if (cp == last_dot + 1 && !isalpha(*cp))
+			return -1;
+		else if (!iskeychar(*cp))
+			return -1;
 		*cp = tolower(*cp);
+	}
+	return 0;
 }
 
 int git_config_parse_parameter(const char *text,
@@ -249,7 +279,8 @@ int git_config_parse_parameter(const char *text,
 		strbuf_list_free(pair);
 		return error("bogus config parameter: %s", text);
 	}
-	canonicalize_config_variable_name(pair[0]->buf);
+	if (canonicalize_config_variable_name(pair[0]->buf))
+		return error("bogus config parameter: %s", text);
 	if (fn(pair[0]->buf, value, data) < 0) {
 		strbuf_list_free(pair);
 		return -1;
@@ -382,11 +413,6 @@ static char *parse_value(void)
 	}
 }
 
-static inline int iskeychar(int c)
-{
-	return isalnum(c) || c == '-';
-}
-
 static int get_value(config_fn_t fn, void *data, struct strbuf *name)
 {
 	int c;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 7a16f66a9d..92a5d0dfb1 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1143,6 +1143,13 @@ test_expect_success 'last one wins: three level vars' '
 	test_cmp expect 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'" '
+		test_must_fail git -c $VAR=VAL config -l
+	'
+done
+
 test_expect_success 'git -c is not confused by empty environment' '
 	GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
 '
-- 
2.12.0-rc2-221-ga92f6f5816

Re: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'

[Stefan Beller]

On Tue, Feb 21, 2017 at 10:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> The parsing of one-shot assignments of configuration variables that
> come from the command line historically was quite loose and allowed
> anything to pass.
>
> The configuration variable names that come from files are validated
> in git_config_parse_source(), which uses get_base_var() that grabs
> the <section> (and subsection) while making sure that <section>
> consists of iskeychar() letters, the function itself that makes sure
> that the first letter in <variable> is isalpha(), and get_value()
> that grabs the remainder of the <variable> name while making sure
> that it consists of iskeychar() letters.
>
> Perform an equivalent check in canonicalize_config_variable_name()
> to catch invalid configuration variable names that come from the
> command line.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>     Junio C Hamano <gitster@pobox.com> writes:
>
>     > One thing I noticed is that "git config --get X" will correctly
>     > diagnose that a dot-less X is not a valid variable name, but we do
>     > not seem to diagnose "git -c X=V <cmd>" as invalid.
>     >
>     > Perhaps we should, but it is not the focus of this topic.
>
>     And here is a follow-up on that tangent.

Combining this thought with another email[1] that flew by,
do we also need to have this treatment for "git clone -c"
[1] http://public-inbox.org/git/EC270E42-9431-446C-96F9-E1A0C3E45333@gmail.com/

>
> +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'" '
> +               test_must_fail git -c $VAR=VAL config -l
> +       '
> +done
> +
>  test_expect_success 'git -c is not confused by empty environment' '
>         GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list

Do we also want to test obscure cases of expected success?
e.g. I suspect we never use a."b c".d in the test suite elsewhere but it
would be a valid key to be handed to git?

Re: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Combining this thought with another email[1] that flew by,
> do we also need to have this treatment for "git clone -c"

You tell me ;-) 

Do we share the same parser?  If not, should we make them share the
same code?

>> +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'" '
>> +               test_must_fail git -c $VAR=VAL config -l
>> +       '
>> +done
>> +
>>  test_expect_success 'git -c is not confused by empty environment' '
>>         GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
>
> Do we also want to test obscure cases of expected success?
> e.g. I suspect we never use a."b c".d in the test suite elsewhere but it
> would be a valid key to be handed to git?

I wasn't aiming for anything obscure (and a."b c".d is not at all
obscure); as the new tests like "git -c V.a.R config --get V.A.R"
added in the previous step makes sure that the second level is not
molested and passed as is, so it is less urgent to see what can and
cannot come at the second level.

I didn't check if the existing coverage was sufficient, but we
certainly should test that three-level names with non-alpha and
non-keychar letters in the second are allowed in the overall "git
config" test, not limited to the case where the configuration comes
on a one-shot command line but from files.  I tend to think that is
a separate issue, though.

[PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Junio C Hamano]

The parsing of one-shot assignments of configuration variables that
come from the command line historically was quite loose and allowed
anything to pass.

The configuration variable names that come from files are validated
in git_config_parse_source(), which uses get_base_var() that grabs
the <section> (and subsection) while making sure that <section>
consists of iskeychar() letters, the function itself that makes sure
that the first letter in <variable> is isalpha(), and get_value()
that grabs the remainder of the <variable> name while making sure
that it consists of iskeychar() letters.

Perform an equivalent check in canonicalize_config_variable_name()
to catch invalid configuration variable names that come from the
command line.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Now with an updated test; while writing it it uncovered a bug in
   the original test that expected to fail---they failed alright but
   sometimes failed for a wrong reason.

 config.c               | 48 +++++++++++++++++++++++++++++++++++++-----------
 t/t1300-repo-config.sh | 16 ++++++++++++++++
 2 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 4128debc71..e7f7ff1938 100644
--- a/config.c
+++ b/config.c
@@ -199,32 +199,62 @@ void git_config_push_parameter(const char *text)
 	strbuf_release(&env);
 }
 
+static inline int iskeychar(int c)
+{
+	return isalnum(c) || c == '-';
+}
+
 /*
  * downcase the <section> and <variable> in <section>.<variable> or
  * <section>.<subsection>.<variable> and do so in place.  <subsection>
  * is left intact.
+ *
+ * The configuration variable names that come from files are validated
+ * in git_config_parse_source(), which uses get_base_var() that grabs
+ * the <section> (and subsection) while making sure that <section>
+ * consists of iskeychar() letters, the function itself that makes
+ * sure that the first letter in <variable> is isalpha(), and
+ * get_value() that grabs the remainder of the <variable> name while
+ * making sure that it consists of iskeychar() letters.  Perform a
+ * matching validation for configuration variables that come from
+ * the command line.
  */
-static void canonicalize_config_variable_name(char *varname)
+static int canonicalize_config_variable_name(char *varname)
 {
-	char *cp, *last_dot;
+	char *cp, *first_dot, *last_dot;
 
 	/* downcase the first segment */
 	for (cp = varname; *cp; cp++) {
 		if (*cp == '.')
 			break;
+		if (!iskeychar(*cp))
+			return -1;
 		*cp = tolower(*cp);
 	}
 	if (!*cp)
-		return;
+		return -1; /* no dot anywhere? */
+
+	first_dot = cp;
+	if (first_dot == varname)
+		return -1; /* no section? */
 
 	/* find the last dot (we start from the first dot we just found) */
-	for (last_dot = cp; *cp; cp++)
+	for (; *cp; cp++)
 		if (*cp == '.')
 			last_dot = cp;
 
+	if (!last_dot[1])
+		return -1; /* no variable? */
+
 	/* downcase the last segment */
-	for (cp = last_dot; *cp; cp++)
+	for (cp = last_dot + 1; *cp; cp++) {
+		if (cp == last_dot + 1 && !isalpha(*cp))
+			return -1;
+		else if (!iskeychar(*cp))
+			return -1;
 		*cp = tolower(*cp);
+	}
+	return 0;
 }
 
 int git_config_parse_parameter(const char *text,
@@ -249,7 +279,8 @@ int git_config_parse_parameter(const char *text,
 		strbuf_list_free(pair);
 		return error("bogus config parameter: %s", text);
 	}
-	canonicalize_config_variable_name(pair[0]->buf);
+	if (canonicalize_config_variable_name(pair[0]->buf))
+		return error("bogus config parameter: %s", text);
 	if (fn(pair[0]->buf, value, data) < 0) {
 		strbuf_list_free(pair);
 		return -1;
@@ -382,11 +413,6 @@ static char *parse_value(void)
 	}
 }
 
-static inline int iskeychar(int c)
-{
-	return isalnum(c) || c == '-';
-}
-
 static int get_value(config_fn_t fn, void *data, struct strbuf *name)
 {
 	int c;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 7a16f66a9d..ea371020fa 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1143,6 +1143,22 @@ test_expect_success 'last one wins: three level vars' '
 	test_cmp expect 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'" '
+		test_must_fail git -c "$VAR=VAL" config -l
+	'
+done
+
+for VAR in a.b a."b c".d
+do
+	test_expect_success "git -c $VAR=VAL works with valid '$VAR'" '
+		echo VAL >expect &&
+		git -c "$VAR=VAL" config --get "$VAR" >actual &&
+		test_cmp expect actual
+	'
+done
+
 test_expect_success 'git -c is not confused by empty environment' '
 	GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
 '
-- 
2.12.0-rc2-222-gff02733afe

Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>  	/* find the last dot (we start from the first dot we just found) */
> -	for (last_dot = cp; *cp; cp++)
> +	for (; *cp; cp++)
>  		if (*cp == '.')
>  			last_dot = cp;

This line probably needs this fix-up on top.

-- >8 --
Subject: [PATCH] config: squelch stupid compiler warnings

Some compilers do not realize that *cp is always '.' when the loop
to find the last dot begins, and instead gives a useless warning
that says last_dot may be uninitialized.

Squelch it by being a bit more explicit if stupid.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.c b/config.c
index e7f7ff1938..90de27853f 100644
--- a/config.c
+++ b/config.c
@@ -239,7 +239,7 @@ static int canonicalize_config_variable_name(char *varname)
 		return -1; /* no section? */
 
 	/* find the last dot (we start from the first dot we just found) */
-	for (; *cp; cp++)
+	for (last_dot = cp; *cp; cp++)
 		if (*cp == '.')
 			last_dot = cp;
 
-- 
2.12.0-rc2-231-g83a1c8597c

Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Jeff King]

On Tue, Feb 21, 2017 at 01:24:38PM -0800, Junio C Hamano wrote:

> The parsing of one-shot assignments of configuration variables that
> come from the command line historically was quite loose and allowed
> anything to pass.
> 
> The configuration variable names that come from files are validated
> in git_config_parse_source(), which uses get_base_var() that grabs
> the <section> (and subsection) while making sure that <section>
> consists of iskeychar() letters, the function itself that makes sure
> that the first letter in <variable> is isalpha(), and get_value()
> that grabs the remainder of the <variable> name while making sure
> that it consists of iskeychar() letters.
> 
> Perform an equivalent check in canonicalize_config_variable_name()
> to catch invalid configuration variable names that come from the
> command line.

FWIW, the code looks OK here. It is a shame to duplicate the policy
found in git_config_parse_key(), though.

I wonder if we could make a master version of that which canonicalizes
in-place, and then just wrap it for the git_config_parse_key()
interface. Actually, I guess the function you just wrote _is_ that inner
function, as long as it learned about the "quiet" flag.

-Peff

Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> FWIW, the code looks OK here. It is a shame to duplicate the policy
> found in git_config_parse_key(), though.
>
> I wonder if we could make a master version of that which canonicalizes
> in-place, and then just wrap it for the git_config_parse_key()
> interface. Actually, I guess the function you just wrote _is_ that inner
> function, as long as it learned about the "quiet" flag.

Hmm, I obviously missed an opportunity.  I thought about doing a
similar thing with the policy in parse-source, but that side didn't
seem worth doing, as the config-parse-source callgraph is quite a
mess (as it has to parse the .ini like format with line breaks and
comments, not the simple "<string>[.<string>].<string>" thing, it
cannot quite avoid it), and it needs to take advantage of _some_
policy to parse the pieces.

We could loosen the policy implemented by config-parse-key interface
(e.g. change config-parse-source to let anything that begins with a
non-whitespace continue to be processed with get_value(), instead of
only allowing string that begins with isalpha(); similarly loosen
get_value() to allow any non-dot non-space string, not just
iskeychar() bytes) and first turn what is read into the simple
"<string>[.<string>].<string>" format, and then reuse the new
"master" logic to validate.  That would allow us to update the
"master" logic to make it tighter or looser to some degree, but the
source parser still needs to hardcode _some_ policy (e.g. the first
level and the last level names begin with a non-space) that allows
it to guess what "<string>" pieces the contents being parsed from
the .ini looking format meant to express.

But you are right.  config-parse-key does have the simpler string
that can just be given to the canonicalize thing and we should be
able to reuse it.

Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> FWIW, the code looks OK here. It is a shame to duplicate the policy
>> found in git_config_parse_key(), though.
>>
>> I wonder if we could make a master version of that which canonicalizes
>> in-place, and then just wrap it for the git_config_parse_key()
>> interface. Actually, I guess the function you just wrote _is_ that inner
>> function, as long as it learned about the "quiet" flag.
>
> Hmm, I obviously missed an opportunity....
> ...
> But you are right.  config-parse-key does have the simpler string
> that can just be given to the canonicalize thing and we should be
> able to reuse it.

Actually, I think we can just use the existing config_parse_key()
instead of adding the new function.  It adds one allocation and
deallocation, but it's not like this is a performance-critical
codepath that we absolutely avoid extra allocations.  After all, we
are still using the strbuf-split thing :-/.

The attached patch shows the updated fix.  It needs a preparatory
code move (not shown here) to make git_config_parse_key() available
to git_config_parse_parameter(), though.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 23 Feb 2017 15:04:40 -0800
Subject: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command' and
 keep subsection intact

The parsing of one-shot assignments of configuration variables that
come from the command line historically was quite loose and allowed
anything to pass.  It also downcased everything in the variable name,
even a three-level <section>.<subsection>.<variable> name in which
the <subsection> part must be treated in a case sensible manner.

Existing git_config_parse_key() helper is used to parse the variable
name that comes from the command line, i.e. "git config VAR VAL",
and handles these details correctly.  Replace the strbuf_tolower()
call in git-config_parse_parameter() with a call to it to correct
both issues.  git_config_parse_key() does a bit more things that are
not necessary for the purpose of this codepath (e.g. it allocates a
separate buffer to return the canonicalized variable name because it
takes a "const char *" input), but we are not in a performance-critical
codepath here.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.c               | 14 ++++++++----
 t/t1300-repo-config.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/config.c b/config.c
index b8cce1dffa..39f20dcd2c 100644
--- a/config.c
+++ b/config.c
@@ -295,7 +295,9 @@ int git_config_parse_parameter(const char *text,
 			       config_fn_t fn, void *data)
 {
 	const char *value;
+	char *canonical_name;
 	struct strbuf **pair;
+	int ret = 0;
 
 	pair = strbuf_split_str(text, '=', 2);
 	if (!pair[0])
@@ -313,13 +315,15 @@ int git_config_parse_parameter(const char *text,
 		strbuf_list_free(pair);
 		return error("bogus config parameter: %s", text);
 	}
-	strbuf_tolower(pair[0]);
-	if (fn(pair[0]->buf, value, data) < 0) {
-		strbuf_list_free(pair);
+
+	if (git_config_parse_key(pair[0]->buf, &canonical_name, NULL))
 		return -1;
-	}
+
+	ret = (fn(canonical_name, value, data) < 0) ? -1 : 0;
+
+	free(canonical_name);
 	strbuf_list_free(pair);
-	return 0;
+	return ret;
 }
 
 int git_config_from_parameters(config_fn_t fn, void *data)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 923bfc5a26..ea371020fa 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1097,6 +1097,68 @@ test_expect_success 'multiple git -c appends config' '
 	test_cmp expect actual
 '
 
+test_expect_success 'last one wins: two level vars' '
+
+	# sec.var and sec.VAR are the same variable, as the first
+	# and the last level of a configuration variable name is
+	# case insensitive.
+
+	echo VAL >expect &&
+
+	git -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&
+	test_cmp expect actual &&
+	git -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&
+	test_cmp expect actual &&
+
+	git -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&
+	test_cmp expect actual &&
+	git -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'last one wins: three level vars' '
+
+	# 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.
+
+	echo val >expect &&
+	git -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&
+	test_cmp expect actual &&
+	git -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&
+	test_cmp expect actual &&
+
+	# v.a.r and V.a.R are the same variable, as the first
+	# and the last level of a configuration variable name is
+	# case insensitive.
+
+	echo VAL >expect &&
+	git -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&
+	test_cmp expect actual &&
+	git -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&
+	test_cmp expect actual &&
+	git -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&
+	test_cmp expect actual &&
+	git -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&
+	test_cmp expect 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'" '
+		test_must_fail git -c "$VAR=VAL" config -l
+	'
+done
+
+for VAR in a.b a."b c".d
+do
+	test_expect_success "git -c $VAR=VAL works with valid '$VAR'" '
+		echo VAL >expect &&
+		git -c "$VAR=VAL" config --get "$VAR" >actual &&
+		test_cmp expect actual
+	'
+done
+
 test_expect_success 'git -c is not confused by empty environment' '
 	GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
 '
-- 
2.12.0-rc2-289-g9f26f1516a

Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Jeff King]

On Thu, Feb 23, 2017 at 03:19:58PM -0800, Junio C Hamano wrote:

> > But you are right.  config-parse-key does have the simpler string
> > that can just be given to the canonicalize thing and we should be
> > able to reuse it.
> 
> Actually, I think we can just use the existing config_parse_key()
> instead of adding the new function.  It adds one allocation and
> deallocation, but it's not like this is a performance-critical
> codepath that we absolutely avoid extra allocations.  After all, we
> are still using the strbuf-split thing :-/.

Yeah, you're right. This is much nicer, and everything else was
premature optimization.

> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Thu, 23 Feb 2017 15:04:40 -0800
> Subject: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command' and
>  keep subsection intact

Long subject. :)

I'd have just said:

  config: pass variables through git_config_parse_parameter()

That is "what", but the "why" can come in the next paragraph.

> The parsing of one-shot assignments of configuration variables that
> come from the command line historically was quite loose and allowed
> anything to pass.  It also downcased everything in the variable name,
> even a three-level <section>.<subsection>.<variable> name in which
> the <subsection> part must be treated in a case sensible manner.
> 
> Existing git_config_parse_key() helper is used to parse the variable
> name that comes from the command line, i.e. "git config VAR VAL",
> and handles these details correctly.  Replace the strbuf_tolower()
> call in git-config_parse_parameter() with a call to it to correct
> both issues.  git_config_parse_key() does a bit more things that are
> not necessary for the purpose of this codepath (e.g. it allocates a
> separate buffer to return the canonicalized variable name because it
> takes a "const char *" input), but we are not in a performance-critical
> codepath here.

Nicely explained.

> diff --git a/config.c b/config.c
> index b8cce1dffa..39f20dcd2c 100644
> --- a/config.c
> +++ b/config.c
> @@ -295,7 +295,9 @@ int git_config_parse_parameter(const char *text,
>  			       config_fn_t fn, void *data)
>  {
>  	const char *value;
> +	char *canonical_name;
>  	struct strbuf **pair;
> +	int ret = 0;
>  
>  	pair = strbuf_split_str(text, '=', 2);
>  	if (!pair[0])

Hmm. I suspect one cannot do:

  git -c 'section.subsection with an = in it.key=foo' ...

Definitely not a new problem, nor something that should block your
patch. But if we want to fix it, I suspect the problem will ultimately
involve parsing left-to-right to get the key first, then confirming it
has an =, and then the value.

> @@ -313,13 +315,15 @@ int git_config_parse_parameter(const char *text,
>  		strbuf_list_free(pair);
>  		return error("bogus config parameter: %s", text);
>  	}
> -	strbuf_tolower(pair[0]);
> -	if (fn(pair[0]->buf, value, data) < 0) {
> -		strbuf_list_free(pair);
> +
> +	if (git_config_parse_key(pair[0]->buf, &canonical_name, NULL))
>  		return -1;
> -	}

I think git_config_parse_key() will free canonical_name itself if it
returns failure. But do you need to strbuf_list_free(pair) here?

Or alternatively:

  int ret = -1;
  if (!parse(...))
          ret = fn(...);

or use a "got out". Whatever. You don't need me to teach you about error
exits. :)

> +	ret = (fn(canonical_name, value, data) < 0) ? -1 : 0;
> +
> +	free(canonical_name);
>  	strbuf_list_free(pair);
> -	return 0;
> +	return ret;

Looks good.

> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 923bfc5a26..ea371020fa 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh

I just skimmed these, as they look like the previous ones.

So overall I like it, modulo the minor error-leak.

-Peff

Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>  	pair = strbuf_split_str(text, '=', 2);
>>  	if (!pair[0])
>
> Hmm. I suspect one cannot do:
>
>   git -c 'section.subsection with an = in it.key=foo' ...
>
> Definitely not a new problem, nor something that should block your
> patch. But if we want to fix it, I suspect the problem will ultimately
> involve parsing left-to-right to get the key first, then confirming it
> has an =, and then the value.

Backtracking will not fundamentally "fix" parsing of

	a.b=c=.d

between twhse two

	[a "b="] c = ".d"
	[a]      b = "c=.d"

unfortunately, I think.  I do not think it is worth doing the "best
effort" with erroring out when ambiguous, because there is no way
for the end user to disambiguate, unless we introduce a different
syntax, at which point we cannot use config_parse_key() anymore.

>> +	if (git_config_parse_key(pair[0]->buf, &canonical_name, NULL))
>>  		return -1;
>> -	}
>
> I think git_config_parse_key() will free canonical_name itself if it
> returns failure. But do you need to strbuf_list_free(pair) here?

Yeah, I missed that one.  Thanks.

Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Jeff King]

On Thu, Feb 23, 2017 at 08:17:44PM -0800, Junio C Hamano wrote:

> > Hmm. I suspect one cannot do:
> >
> >   git -c 'section.subsection with an = in it.key=foo' ...
> >
> > Definitely not a new problem, nor something that should block your
> > patch. But if we want to fix it, I suspect the problem will ultimately
> > involve parsing left-to-right to get the key first, then confirming it
> > has an =, and then the value.
> 
> Backtracking will not fundamentally "fix" parsing of
> 
> 	a.b=c=.d
> 
> between twhse two
> 
> 	[a "b="] c = ".d"
> 	[a]      b = "c=.d"
> 
> unfortunately, I think.  I do not think it is worth doing the "best
> effort" with erroring out when ambiguous, because there is no way
> for the end user to disambiguate, unless we introduce a different
> syntax, at which point we cannot use config_parse_key() anymore.

Ah, yeah, you're right. I thought the problem was just that the "split"
was too naive, but it really is that the whole syntax is badly
specified.

I guess "git config --list" suffers from the same problem. You can get
around it there with "-z", but that probably would not be very pleasant
here. :)

Probably not worth worrying too much about if nobody is complaining.

-Peff

Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Backtracking will not fundamentally "fix" parsing of
>> 
>> 	a.b=c=.d
>> 
>> between twhse two
>> 
>> 	[a "b="] c = ".d"
>> 	[a]      b = "c=.d"
>> 
>> unfortunately, I think.  I do not think it is worth doing the "best
>> effort" with erroring out when ambiguous, because there is no way
>> for the end user to disambiguate, unless we introduce a different
>> syntax, at which point we cannot use config_parse_key() anymore.
>
> Ah, yeah, you're right. I thought the problem was just that the "split"
> was too naive, but it really is that the whole syntax is badly
> specified.
>
> I guess "git config --list" suffers from the same problem. You can get
> around it there with "-z", but that probably would not be very pleasant
> here. :)
>
> Probably not worth worrying too much about if nobody is complaining.

Yup.

Anyway, here is an updated one (the part of the patch to t/ is not
shown as it is unchanged).

-- >8 --
Subject: [PATCH] config: use git_config_parse_key() in git_config_parse_parameter()

The parsing of one-shot assignments of configuration variables that
come from the command line historically was quite loose and allowed
anything to pass.  It also downcased everything in the variable name,
even a three-level <section>.<subsection>.<variable> name in which
the <subsection> part must be treated in a case sensitive manner.

Existing git_config_parse_key() helper is used to parse the variable
name that comes from the command line, i.e. "git config VAR VAL",
and handles these details correctly.  Replace the strbuf_tolower()
call in git_config_parse_parameter() with a call to it to correct
both issues.  git_config_parse_key() does a bit more things that are
not necessary for the purpose of this codepath (e.g. it allocates a
separate buffer to return the canonicalized variable name because it
takes a "const char *" input), but we are not in a performance-critical
codepath here.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/config.c b/config.c
index b8cce1dffa..1c1a1520ff 100644
--- a/config.c
+++ b/config.c
@@ -295,7 +295,9 @@ int git_config_parse_parameter(const char *text,
 			       config_fn_t fn, void *data)
 {
 	const char *value;
+	char *canonical_name;
 	struct strbuf **pair;
+	int ret;
 
 	pair = strbuf_split_str(text, '=', 2);
 	if (!pair[0])
@@ -313,13 +315,15 @@ int git_config_parse_parameter(const char *text,
 		strbuf_list_free(pair);
 		return error("bogus config parameter: %s", text);
 	}
-	strbuf_tolower(pair[0]);
-	if (fn(pair[0]->buf, value, data) < 0) {
-		strbuf_list_free(pair);
-		return -1;
+
+	if (git_config_parse_key(pair[0]->buf, &canonical_name, NULL)) {
+		ret = -1;
+	} else {
+		ret = (fn(canonical_name, value, data) < 0) ? -1 : 0;
+		free(canonical_name);
 	}
 	strbuf_list_free(pair);
-	return 0;
+	return ret;
 }
 
 int git_config_from_parameters(config_fn_t fn, void *data)
-- 
2.12.0-rc2-308-gbf7e63c428

Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Jeff King]

On Thu, Feb 23, 2017 at 10:08:57PM -0800, Junio C Hamano wrote:

> Anyway, here is an updated one (the part of the patch to t/ is not
> shown as it is unchanged).
> 
> -- >8 --
> Subject: [PATCH] config: use git_config_parse_key() in git_config_parse_parameter()

Looks good. Nice and simple.

-Peff