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

[Phillip Wood <phillip.wood123@gmail.com>]

On 01/07/2025 16:54, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>>    * 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.
> 
> I understand that principe.  What I was wondering was if there are
> enough places to use these particular ones to make it worthwhile to
> enlarge the set of OPT_* macros.

There are six users of each of these macros so I think it is worthwhile. 
That's two more users than there are for OPT_RERERE_AUTOUPDATE() and 
twice as many users as OPT_CONTAINS().

>>>    * 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.
> 
> Yeah, I was wondering about that approach, too.  Another benefit
> with the "validate just after we parse the value before we assign
> the result to a variable or a struct member" approach is that we can
> also complain about -1 that is given from the command line (which
> the current code ignores, if I am not mistaken, because it needs to
> be silent if that -1 is there merely because it is the "not set yet"
> sentinel value).
> 
> Or perhaps the valid value range Réne has been workingon canbe used
> here?

That would be nice

Thanks

Phillip