Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 23, 2017, 07:19 UTC
Message-ID: <xmqqo9xt8jcf.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170223055831.u3yofkby3c56t7l4@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 7 quoted lines

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