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