* Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
2017-02-20 9:58 ` Junio C Hamano
@ 2017-02-20 17:17 ` Lars Schneider
2017-02-21 7:38 ` Jeff King
2017-02-21 18:53 ` [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command' Junio C Hamano
2 siblings, 0 replies; 41+ messages in thread
From: Lars Schneider @ 2017-02-20 17:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Jonathan Tan, git, sbeller
> 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
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
2017-02-20 9:58 ` Junio C Hamano
2017-02-20 17:17 ` Lars Schneider
@ 2017-02-21 7:38 ` Jeff King
2017-02-21 17:01 ` Junio C Hamano
2017-02-21 18:53 ` [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command' Junio C Hamano
2 siblings, 1 reply; 41+ messages in thread
From: Jeff King @ 2017-02-21 7:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, Jonathan Tan, git, sbeller
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
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
2017-02-21 7:38 ` Jeff King
@ 2017-02-21 17:01 ` Junio C Hamano
2017-02-21 17:17 ` [PATCH v2] " Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-02-21 17:01 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Jonathan Tan, git, sbeller
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.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2] config: preserve <subsection> case for one-shot config on the command line
2017-02-21 17:01 ` Junio C Hamano
@ 2017-02-21 17:17 ` Junio C Hamano
2017-02-21 17:50 ` Jeff King
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-02-21 17:17 UTC (permalink / raw)
To: git; +Cc: Jeff King, Lars Schneider, Jonathan Tan, sbeller
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
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v2] config: preserve <subsection> case for one-shot config on the command line
2017-02-21 17:17 ` [PATCH v2] " Junio C Hamano
@ 2017-02-21 17:50 ` Jeff King
2017-02-21 17:57 ` Stefan Beller
0 siblings, 1 reply; 41+ messages in thread
From: Jeff King @ 2017-02-21 17:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Lars Schneider, Jonathan Tan, sbeller
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] config: preserve <subsection> case for one-shot config on the command line
2017-02-21 17:50 ` Jeff King
@ 2017-02-21 17:57 ` Stefan Beller
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Beller @ 2017-02-21 17:57 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, git@vger.kernel.org, Lars Schneider, Jonathan Tan
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'
2017-02-20 9:58 ` Junio C Hamano
2017-02-20 17:17 ` Lars Schneider
2017-02-21 7:38 ` Jeff King
@ 2017-02-21 18:53 ` Junio C Hamano
2017-02-21 19:15 ` Stefan Beller
2 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-02-21 18:53 UTC (permalink / raw)
To: git
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
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'
2017-02-21 18:53 ` [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command' Junio C Hamano
@ 2017-02-21 19:15 ` Stefan Beller
2017-02-21 20:33 ` Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Stefan Beller @ 2017-02-21 19:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
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?
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'
2017-02-21 19:15 ` Stefan Beller
@ 2017-02-21 20:33 ` Junio C Hamano
2017-02-21 21:24 ` [PATCH v2] " Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-02-21 20:33 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'
2017-02-21 20:33 ` Junio C Hamano
@ 2017-02-21 21:24 ` Junio C Hamano
2017-02-22 1:06 ` Junio C Hamano
2017-02-23 5:58 ` Jeff King
0 siblings, 2 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-02-21 21:24 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Stefan Beller
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
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'
2017-02-21 21:24 ` [PATCH v2] " Junio C Hamano
@ 2017-02-22 1:06 ` Junio C Hamano
2017-02-23 5:58 ` Jeff King
1 sibling, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-02-22 1:06 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Stefan Beller
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
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'
2017-02-21 21:24 ` [PATCH v2] " Junio C Hamano
2017-02-22 1:06 ` Junio C Hamano
@ 2017-02-23 5:58 ` Jeff King
2017-02-23 7:19 ` Junio C Hamano
1 sibling, 1 reply; 41+ messages in thread
From: Jeff King @ 2017-02-23 5:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Stefan Beller
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'
2017-02-23 5:58 ` Jeff King
@ 2017-02-23 7:19 ` Junio C Hamano
2017-02-23 23:19 ` Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-02-23 7:19 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org, Stefan Beller
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.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'
2017-02-23 7:19 ` Junio C Hamano
@ 2017-02-23 23:19 ` Junio C Hamano
2017-02-24 0:41 ` Jeff King
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-02-23 23:19 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org, Stefan Beller
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
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'
2017-02-23 23:19 ` Junio C Hamano
@ 2017-02-24 0:41 ` Jeff King
2017-02-24 4:17 ` Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Jeff King @ 2017-02-24 0:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Stefan Beller
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
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'
2017-02-24 0:41 ` Jeff King
@ 2017-02-24 4:17 ` Junio C Hamano
2017-02-24 4:22 ` Jeff King
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-02-24 4:17 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org, Stefan Beller
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.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'
2017-02-24 4:17 ` Junio C Hamano
@ 2017-02-24 4:22 ` Jeff King
2017-02-24 6:08 ` Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Jeff King @ 2017-02-24 4:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Stefan Beller
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'
2017-02-24 4:22 ` Jeff King
@ 2017-02-24 6:08 ` Junio C Hamano
2017-02-24 6:10 ` Jeff King
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-02-24 6:08 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org, Stefan Beller
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
^ permalink raw reply related [flat|nested] 41+ messages in thread