Re: [PATCH v5 3/3] rebase: add a config option for --rebase-merges

> On 04/03/2023 23:24, Alex Henrie wrote:
> > On Thu, Mar 2, 2023 at 2:37 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >
> >> On 25/02/2023 18:03, Alex Henrie wrote:
> >
> >>> +rebase.merges::
> >>> +     Whether and how to set the `--rebase-merges` option by default. Can
> >>> +     be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> >>> +     true is equivalent to `--rebase-merges` without an argument, setting to
> >>> +     `rebase-cousins` or `no-rebase-cousins` is equivalent to
> >>> +     `--rebase-merges` with that value as its argument, and setting to false
> >>> +     is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
> >>> +     command line without an argument overrides a `rebase.merges=false`
> >>> +     configuration but does not override other values of `rebase.merge`.
> >>
> >> I'm still not clear why the commandline doesn't override the config in
> >> all cases as is our usual practice. After all if the user has set
> >> rebase.merges then they don't need to pass --rebase-merges unless they
> >> want to override the config.
> >
> > Given the current push to turn rebase-merges on by default, it seems
> > likely that rebase-cousins will also be turned on by default at some
> > point after that.
>
> It is good to try and future proof things but this seems rather
> hypothetical. I don't really see how the choice of whether
> --rebase-merges is turned on by default is related to the choice of
> whether or not to rebase cousins by default. It is worth noting that the
> default was changed to from rebase-cousins to no-rebase-cousins early in
> the development of --rebase-merges[1] as it was felt to be less surprising.

> > There will be a warning about the default changing,
> > and we'll want to let users suppress that warning by setting
> > rebase.rebaseMerges=rebase-cousins. It would then be very confusing if
> > a --rebase-merges from some old alias continued to mean
> > --rebase-merges=no-rebase-cousins when the user expects it to start
> > behaving as though the default has already changed.
>
> But aren't you breaking those aliases now when
> rebase.rebaseMerges=rebase-cousins? That's what I'm objecting to. It
> seems like we're breaking things now to avoid a hypothetical future
> change breaking them which does not seem like the right trade off to me.
>
> It also does not fit with the way other optional arguments interact with
> their associated config setting. For example "git branch/checkout/switch
> --track" and branch.autoSetupMerge. If the optional argument to --track
> is omitted it defaults to "direct" irrespective of the config.

> >>> +test_expect_success '--rebase-merges overrides rebase.merges=false' '
> >>> +     test_config rebase.merges false &&
> >>> +     git checkout -b override-config-merges-false E &&
> >>> +     before="$(git rev-parse --verify HEAD)" &&
> >>> +     test_tick &&
> >>> +     git rebase --rebase-merges C &&
> >>> +     test_cmp_rev HEAD $before
> >>
> >> This test passes if the rebase does nothing, maybe pass --force and
> >> check the graph?
> >
> > The rebase is supposed to do nothing here.
>
> It's not doing nothing though - it is rebasing the branch, it just
> happens that everything fast-forwards so HEAD ends up unchanged. My
> point is that this test should verify the branch has been rebased. Maybe
> you could check the reflog message for HEAD@{0} is
>
>         rebase (finish): returning to refs/heads/override-config-merges-false
>
> > Checking that the commit
> > hash is the same is just a quick way to check that the entire graph is
> > the same. What more would be checked by checking the graph instead of
> > the hash?
>
> By using --force and checking the graph you check that the rebase
> actually happened.