From: Stefan Beller <sbeller@google.com>
Date: Tue, 21 Feb 2017 19:15:52 GMT
Subject: Re: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'
Message-ID: <CAGZ79kbR2QQyYO1dnQ0jW3-ztKEFj1MtJfDqEc0xoftMFeN=WA@mail.gmail.com>
In-Reply-To: <xmqq37f7gyuj.fsf_-_@gitster.mtv.corp.google.com>

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?