Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=""

From: Alex Henrie <alexhenrie24@gmail.com>
Date: Feb 22, 2023, 23:33 UTC
Message-ID: <CAMMLpeRXYm-Qtr6556fZxmND+stku9cK+aBGr7h7ezw+Mrg=-g@mail.gmail.com>
In-Reply-To: <xmqq8rgpcn8l.fsf@gitster.g>

On Wed, Feb 22, 2023 at 3:18 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 16 quoted lines

>
> On 22/02/2023 01:38, Alex Henrie wrote:
> > On Tue, Feb 21, 2023 at 3:55 AM Phillip Wood <phillip.wood123@gmail.com> 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@pobox.com> wrote:

Show 8 quoted lines

>
> 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