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

From: Alex Henrie <alexhenrie24@gmail.com>
Date: Feb 22, 2023, 01:38 UTC
Message-ID: <CAMMLpeQ9gEKNX5VtGCgLL_Qzk59ZYji57u-SPy-XeSRPuF2NwA@mail.gmail.com>
In-Reply-To: <852c6efd-49a7-f6f0-dd6a-b28cb0909784@dunelm.org.uk>

On Tue, Feb 21, 2023 at 3:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 11 quoted lines

>
> On 21/02/2023 05:58, Alex Henrie wrote:
> > The unusual syntax --rebase-merges="" (that is, --rebase-merges with an
> > empty string argument) has been an undocumented synonym of
> > --rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid
> > confusion when a rebase.merges config option is introduced, where
> > rebase.merges="" will be equivalent to not passing --rebase-merges.
>
> I think this is sensible in the context of adding a config option. It is
> a backwards incompatible change though, lets hope no one was relying on
> it.

Since the syntax is bizarre and undocumented, I doubt anyone is relying on it for anything serious, if anyone uses it at all.

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

All the same, your comment made me realize that it would probably make more sense to simply change the default value of --rebase-cousins from "" to "no-rebase-cousins" in this patch and then add the parse_opt_merges callback in the next patch when it is actually needed.

-Alex