From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 08 Jun 2020 14:02:48 GMT
Subject: Re: [PATCH v2] grep: add configuration variables for --heading
Message-ID: <1c61583e-5af4-2c11-0147-213b1c484fbd@gmail.com>
In-Reply-To: <2lg0C_0k4SnWqhB0oW3wGE6eixtc_ZT5GgZJFzH8XC6g5dJXmSGGnrV9uZVT4QIOG9Sjc7yuLOR_v5kwGIV1hnMSojXinNG16G3tOOMg-Fc=@emersion.fr>

Hi Simon

On 08/06/2020 11:22, Simon Ser wrote:
> On Wednesday, May 27, 2020 6:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
>> Naturally it follows that a command-line option
>>
>> $ git config grep.heading yes
>> $ git grep --no-heading -e pattern
>>
>> is a way to countermand the configured default per invocation
>> basis.
> 
> Is see where you're getting at, but this is missing for a handful of
> options, like grep.lineNumber and grep.column. I'd rather not create an
> inconsistency here.

Those options do exist - try
   git -c grep.column=true grep --no-column ...
and you will see that the column number is not printed. This is because 
OPT_BOOL() without the PARSE_OPT_NONEG flag creates as --no- option.

If these existing options are not documented or tested perhaps you would 
be able to add a second patch to fix that.

>> When writing new tests, we often get too excited and stop at showing
>> off how well the shiny new feature works, but we should make sure
>> that we test the "negative" case, too, i.e. that the "feature" can
>> be disabled when the user does not want to trigger it, and that the
>> "feature" notices incorrect invocations and fails appropriately.
>>
>> E.g.
>>
>> git -c grep.heading=yes grep --no-heading ...
>>
>> should not leave the opt->heading true, and
>>
>> git -c grep.heading=nonsense grep ...
>>
>> should fail, saying "grep.heading must be a bool" (or something
>> along that line).
> 
> Note, these new tests are only required if --no-heading is added to the
> patch.

That is not correct in the case of `git grep -c grep.heading=nonsense 
grep ...`. Anyway --no-heading does exist.

Best Wishes

Phillip