Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'
- From
Jeff King <peff@peff.net>
- Date
- Feb 24, 2017, 00:41 UTC
- Message-ID
- <20170224004105.ayddcwlnpmq7tifu@sigill.intra.peff.net>
- In-Reply-To
- <xmqqy3ww5wbl.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 23, 2017 at 03:19:58PM -0800, Junio C Hamano wrote:
Show 9 quoted lines
> > 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.
Show 5 quoted lines
> -- >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.
Show 15 quoted lines
> 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.
Show 14 quoted lines
> 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.
Show 11 quoted lines
> @@ -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. :)
Show 6 quoted lines
> + 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