Re: [PATCH v4 3/3] rebase: add a config option for --rebase-merges
From: Phillip Wood <hidden>
Date: 2023-02-24 14:55:41
Hi Alex On 23/02/2023 05:34, Alex Henrie wrote:
quoted hunk ↗ jump to hunk
The purpose of the new option is to accommodate users who would like --rebase-merges to be on by default and to facilitate possibly turning on --rebase-merges by default without configuration in a future version of Git. Signed-off-by: Alex Henrie <redacted> --- Documentation/config/rebase.txt | 10 ++++ Documentation/git-rebase.txt | 3 +- builtin/rebase.c | 47 ++++++++++++---- t/t3430-rebase-merges.sh | 96 +++++++++++++++++++++++++++++++++ 4 files changed, 144 insertions(+), 12 deletions(-)diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt index f19bd0e040..308baa9dbb 100644 --- a/Documentation/config/rebase.txt +++ b/Documentation/config/rebase.txt@@ -67,3 +67,13 @@ rebase.rescheduleFailedExec:: rebase.forkPoint:: If set to false set `--no-fork-point` option by default. + +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`.
Thanks for updating this, it is much clearer what the different settings mean now. I not sure if having the config setting override the default when the user passes --rebase-merges without an argument is a good idea.
[...]
+static void parse_merges_value(struct rebase_options *options, const char *value)
+{
+ if (value) {
+ if (!strcmp("no-rebase-cousins", value))
+ options->rebase_cousins = 0;
+ else if (!strcmp("rebase-cousins", value))
+ options->rebase_cousins = 1;
+ else
+ die(_("Unknown mode: %s"), value);
+ }
+
+ options->rebase_merges = 1;
+}It's a shame we seem to have grown yet another callback since v2, I'm not sure we should need to add quite so much code just to support a new config option > [...]
quoted hunk ↗ jump to hunk
@@ -1436,14 +1467,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (options.exec.nr) imply_merge(&options, "--exec"); - if (rebase_merges) { - if (!strcmp("rebase-cousins", rebase_merges)) - options.rebase_cousins = 1; - else if (strcmp("no-rebase-cousins", rebase_merges)) - die(_("Unknown mode: %s"), rebase_merges); - options.rebase_merges = 1; + if (options.rebase_merges) imply_merge(&options, "--rebase-merges"); - }
As I have said before I really think this patch needs to follow the lead of eddfcd8ece (rebase: provide better error message for apply options vs. merge config, 2023-01-25) and provide an error message if --rebase-merges is enabled by rebase.merges and the user provides a command line option that requires the apply backend. So git -c rebase.merges=true rebase --whitespace=fix would result in error: apply options are incompatible with rebase.merges. Consider adding --no-rebase-merges
[...] +test_expect_success '--rebase-merges does not override rebase.merges=rebase-cousins' ' + test_config rebase.merges rebase-cousins && + git checkout -b no-override-config-rebase-cousins main && + git rebase --rebase-merges HEAD^ &&
I think this behavior is confusing for users and will break scripts that quite reasonably assume --rebase-merges is equivalent to --rebase-merges=no-rebase-cousins
[...] +test_expect_success 'local rebase.merges=false overrides global rebase.merges=true' ' + test_config_global rebase.merges true && + test_config rebase.merges false && + git checkout -b override-global-config E && + git rebase C && + test_cmp_graph C.. <<-\EOF + * B + * D + o C + EOF +' + +test_expect_success 'local rebase.merges="" does not override global rebase.merges=true' ' + test_config_global rebase.merges no-rebase-cousins && + test_config rebase.merges "" && + git checkout -b no-override-global-config E && + before="$(git rev-parse --verify HEAD)" && + test_tick && + git rebase C && + test_cmp_rev HEAD $before +'
These two tests seem to be testing the config subsystem rather than this patch. As Dscho has pointed out it is important to get a balance between test coverage and test run time. I think these two tests can definitely be dropped. Best Wishes Phillip