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

[Junio C Hamano]

Jeff King  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.

Re: [PATCH] config: preserve 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 = ".

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 " 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 00..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 

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

[Lars Schneider]

> On 20 Feb 2017, at 10:58, Junio C Hamano  wrote:
> 
> Junio C Hamano  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 = ".

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

> ...
> 
> +/*
> + * downcase the  and  in . or
> + * .. and do so in place.  
> + * 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 00..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 case for one-shot config on the command line

[Junio C Hamano]

Junio C Hamano <gits...@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 = ".

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 " as invalid.

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

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

The "git -c = cmd" mechanism is to pretend that a
configuration variable  is set to  while the cmd is
running.  The code to do so however downcased  in its entirety,
which is wrong for a three-level ...

The  part needs to stay as-is.

Reported-by: Lars Schneider <larsxschnei...@gmail.com>
Diagnosed-by: Jonathan Tan <jonathanta...@google.com>
Signed-off-by: Junio C Hamano <gits...@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();
 }
 
+/*
+ * downcase the  and  in . or
+ * .. and do so in place.  
+ * 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 00..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 -

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

[Junio C Hamano]

Jeff King  writes:

> On Thu, Feb 16, 2017 at 11:30:28AM +0100, Lars Schneider wrote:
>
>> 
>> > On 16 Feb 2017, at 00:48, Junio C Hamano  wrote:
>> > 
>> > The "git -c = 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 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  wrote:
> > 
> > The "git -c = 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 case for one-shot config on the command line

[Junio C Hamano]

Lars Schneider  writes:

>> On 16 Feb 2017, at 00:48, Junio C Hamano  wrote:
>> 
>> The "git -c = 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: [PATCH] config: preserve case for one-shot config on the command line

[Lars Schneider]

> On 16 Feb 2017, at 00:48, Junio C Hamano  wrote:
> 
> The "git -c = 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  is set to  while the cmd is
> running.  The code to do so however downcased  in its entirety,
> which is wrong for a three-level ...
> 
> The  part needs to stay as-is.
> 
> Reported-by: Lars Schneider 
> Diagnosed-by: Jonathan Tan 
> Signed-off-by: Junio C Hamano 
> ---
> 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();
> }
> 
> +/*
> + * downcase the  and  in . or
> + * .. and do so in place.  
> + * 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-larsxschnei...@gmail.com/

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

[Junio C Hamano]

The "git -c = cmd" mechanism is to pretend that a
configuration variable  is set to  while the cmd is
running.  The code to do so however downcased  in its entirety,
which is wrong for a three-level ...

The  part needs to stay as-is.

Reported-by: Lars Schneider 
Diagnosed-by: Jonathan Tan 
Signed-off-by: Junio C Hamano 
---
 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();
 }
 
+/*
+ * downcase the  and  in . or
+ * .. and do so in place.  
+ * 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