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  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: [BUG] submodule config does not apply to upper case submodules?

[Junio C Hamano]

Jonathan Tan  writes:

> On 02/15/2017 03:11 PM, Junio C Hamano wrote:
>> Junio C Hamano  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 "[ ""]  = " 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?

[Junio C Hamano]

Ahh, that would work, too.

On Wed, Feb 15, 2017 at 3:43 PM, Stefan Beller  wrote:
> On Wed, Feb 15, 2017 at 3:37 PM, Junio C Hamano  wrote:
>> Stefan Beller  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();
  }

 +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: [BUG] submodule config does not apply to upper case submodules?

[Stefan Beller]

On Wed, Feb 15, 2017 at 3:37 PM, Junio C Hamano  wrote:
> Stefan Beller  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();
>>>  }
>>>
>>> +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: [BUG] submodule config does not apply to upper case submodules?

[Junio C Hamano]

Stefan Beller  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();
>>  }
>>
>> +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?

[Jonathan Tan]

On 02/15/2017 03:11 PM, Junio C Hamano wrote:

Junio C Hamano  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();
 }

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

[Stefan Beller]

On Wed, Feb 15, 2017 at 3:11 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Jonathan Tan  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();
>  }
>
> +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?

[Junio C Hamano]

Junio C Hamano  writes:

> Jonathan Tan  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();
 }
 
+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?

[Junio C Hamano]

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

[Jonathan Tan]

On 02/15/2017 10:53 AM, Junio C Hamano wrote:

Lars Schneider  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 ".." 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]

Lars Schneider  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 ".." 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?

[Stefan Beller]

On Wed, Feb 15, 2017 at 3:17 AM, Lars Schneider
 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 
> ---
>
> 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
>

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

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