Re: [PATCH v3 4/4] add-patch: add diff.context command line overrides

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 1, 2025, 09:59 UTC
Message-ID: <7686a5e0-23c3-4e3a-b5ea-3b8c80ba54fa@gmail.com>
In-Reply-To: <xmqqplel17s3.fsf@gitster.g>

On 30/06/2025 18:03, Junio C Hamano wrote:

Show 21 quoted lines

> "Leon Michalak via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Leon Michalak <leonmichalak6@gmail.com>
>>
>> This patch compliments the previous commit, where builtins that use
>> add-patch infrastructure now respect diff.context and
>> diff.interHunkContext file configurations.
>>
>> In particular, this patch helps users who don't want to set persistent
>> context configurations or just want a way to override them on a one-time
>> basis, by allowing the relevant builtins to accept corresponding command
>> line options that override the file configurations.
>>
>> This mimics commands such as diff and log, which allow for both context
>> file configuration and command line overrides.
> 
> I skimmed the patch briefly.  I am not sure if it is a good idea to
> 
>   * add OPT_DIFF_*() macros to parse-options API, as its utility is
>     very narrow, and forces those who are learning parse-options API
>     to learn one more thing.

It means that we have consistent help for all the commands with these options which I think is valuable. We have a number of other macros that define options that are shared between commands and I think that works quite well.

> 
>   * validation of the value range to be duplicated for each and every
>     users of the new OPT_DIFF_*() macros.

Yes the validation is awkward. If we changed the OPT_DIFF_* to use a callback that rejected negative values that would reduce the duplication.

> but other than that, looked reasonable to me.

I've left a couple of comments on the tests but the code changes look reasonable to me too

Thanks

Phillip