On Wed, Feb 22, 2023 at 3:18 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > > On 22/02/2023 01:38, Alex Henrie wrote: > > On Tue, Feb 21, 2023 at 3:55 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > >> > >> Is there a particular reason you decided to redo the option parsing > >> rather than just calling parse_merges_value() from the existing "if > >> (rebase_merges)" block? I don't think it really matters, I'm just curious. > > > > Without a parse_opt_merges callback, how could we know whether the > > user passed --no-rebase-merges as opposed to passing nothing at all? > > const char *rebase_merges would be NULL in either case. It's an > > important distinction to make because --no-rebase-merges overrides > > rebase.merges but the absence of a command-line argument does not. > > The usual way we handle that is to set the value of rebase_merges from > the config before calling parse_options(). However your solution is fine. On Wed, Feb 22, 2023 at 4:08 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Is it? If it is not too much to ask, I'd prefer to have code that > does not surprise people, and "the usual way" you mentioned is what > readers around here expect to see. I didn't check and think about > the patch in quetion, and especially the existing code that the > patch needs to touch, too deeply, so if it is too convoluted already > that it would be a lot of work to make it work in "the usual way", > it may be OK, but otherwise, the solution may not be fine. There was a const char *rebase_merges in cmd_rebase and an int rebase_merges in struct rebase_options. I deleted const char *rebase_merges, leaving only int rebase_merges. int rebase_merges is set from rebase_config before it is set from parse_options. Do you want me to add back const char *rebase_merges? If so, where should it be declared so that it can be set from both rebase_config and parse_options? -Alex
