On Thu, Mar 2, 2023 at 2:37 AM Phillip Wood <phillip.wood123@xxxxxxxxx> 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. 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. > > +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. > > +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
