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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 30/06/2025 18:03, Junio C Hamano wrote:
"Leon Michalak via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:

From: Leon Michalak <leonmichalak6@xxxxxxxxx>

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





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux