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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mar 7, 2023, 16:23 UTC
Message-ID: <5551d67b-3021-8cfc-53b5-318f223ded6d@dunelm.org.uk>
In-Reply-To: <CAMMLpeRfD+81fsEtvKFvVepPpwYm0_-AD=QHMwhoc+LtiXpavw@mail.gmail.com>

Hi Alex

On 04/03/2023 23:24, Alex Henrie wrote:

Show 22 quoted lines

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

Show 6 quoted lines

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

[1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.1801292251240.35@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz/

> I will rephrase the documentation in v6 to make it more clear that the
> absence of a specific value on the command line does not clobber a
> specific value set in the configuration, as Glen suggested.

The documentation in v6 is certainly quite clear on this point.

Show 23 quoted lines

>>> +test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.merges=rebase-cousins' '
>>> +     test_config rebase.merges rebase-cousins &&
>>> +     git checkout -b override-config-rebase-cousins main &&
>>> +     git rebase --rebase-merges=no-rebase-cousins HEAD^ &&
>>> +     test_cmp_graph HEAD^.. <<-\EOF
>>> +     *   Merge the topic branch '\''onebranch'\''
>>> +     |\
>>> +     | * D
>>> +     | * G
>>> +     o | H
>>> +     |/
>>> +     o A
>>> +     EOF
>>> +'
>>
>> I'm not sure this test adds much value, it is hard to see what kind of
>> regression would allow the others to pass but not this one.
> 
> I was worried that I or someone else would forget to explicitly set
> rebase_cousins to 0 when no-rebase-cousins is given on the command
> line, assuming that it is already 0 because that is the default. The
> test makes me feel better, but I am happy to remove it if you still
> think it's overkill.

Given we're using the same code to parse the command line argument and the config setting and we have a test for rebase.rebaseMerges=no-rebase-cousins I think we could drop it.

Show 12 quoted lines

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

Thanks for working on this

Phillip

> Thanks for the feedback,
> 
> -Alex