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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 21, 2017, 20:33 UTC
Message-ID: <xmqqlgszffm0.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kbR2QQyYO1dnQ0jW3-ztKEFj1MtJfDqEc0xoftMFeN=WA@mail.gmail.com>

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?

Show 13 quoted lines

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