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

From: Alex Henrie <alexhenrie24@gmail.com>
Date: Mar 4, 2023, 23:24 UTC
Message-ID: <CAMMLpeRfD+81fsEtvKFvVepPpwYm0_-AD=QHMwhoc+LtiXpavw@mail.gmail.com>
In-Reply-To: <1021e6d0-0cd0-d92c-4cb3-45dbf2f6626e@dunelm.org.uk>

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:

Show 14 quoted lines

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

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.

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

Show 10 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. 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?

Thanks for the feedback,

-Alex