From: Phillip Wood Date: Tue, 01 Jul 2025 09:59:55 GMT Subject: Re: [PATCH v3 4/4] add-patch: add diff.context command line overrides Message-ID: <7686a5e0-23c3-4e3a-b5ea-3b8c80ba54fa@gmail.com> In-Reply-To: On 30/06/2025 18:03, Junio C Hamano wrote: > "Leon Michalak via GitGitGadget" writes: > >> From: Leon Michalak >> >> 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