Re: [PATCH v4 2/3] rebase: stop accepting --rebase-merges=""

From: Alex Henrie <alexhenrie24@gmail.com>
Date: Feb 24, 2023, 19:56 UTC
Message-ID: <CAMMLpeQgY5Noyu4HMsi0LDOBktSURpZHybqwbFnqf5sGwmD=9g@mail.gmail.com>
In-Reply-To: <2b28155d-f82d-06b8-2df9-135608c6bf63@dunelm.org.uk>

On Fri, Feb 24, 2023 at 12:24 PM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 17 quoted lines

>
> On 24/02/2023 19:13, Junio C Hamano wrote:
> > Alex Henrie <alexhenrie24@gmail.com> writes:
> >
> >> Phillip is concerned about people and scripts assuming that
> >> --rebase-merges is equivalent to --rebase-merges=no-rebase-cousins,
> >> see [1].
> >
> > Isn't that already broken when you introduce rebase.merges
> > configuration?
>
> Scripts using --rebase-merges are not broken by the introduction of
> rebase.merges so long as we follow our usual convention of always
> allowing the commandline to override the config (i.e. --rebase-merges is
> always equivalent to --rebase-merges=no-rebase-cousins). I don't really
> understand why Alex is suggesting splitting the config into two based on
> my comments.

I was thinking that it would be less surprising to users if the option that broke the no-rebase-cousins assumption had "cousins" in its name. I should have stopped to think that that wouldn't really address your concern because regardless of what the option is named, it could still result in surprising behavior. I apologize for the unhelpful suggestion.

Show 6 quoted lines

> > People and scripts are already relying on the lack
> > of rebase-merges to flatten, and script writers will be surprised to
> > receive a "bug report" complaining that their script does not work
> > when the users set rebase.merges to anything but no.
>
> That is true.

In addition to specifying --no-rebase-merges rather than assuming it, shouldn't the "usual convention" for writing scripts also include being explicit about --rebase-merges=rebase-cousins or --rebase-merges=no-rebase-cousins? And if that is the case, is it really much of a loss to let rebase.merges=rebase-cousins override --rebase-merges without an argument?

-Alex