Re: [PATCH v4 3/3] rebase: add a config option for --rebase-merges
From: Johannes Schindelin <hidden>
Date: 2023-02-24 13:53:51
Hi Alex, On Wed, 22 Feb 2023, Alex Henrie wrote:
quoted hunk ↗ jump to hunk
diff --git a/builtin/rebase.c b/builtin/rebase.c index b68fc2fbb7..45cf445d42 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c@@ -771,6 +771,20 @@ static int run_specific_rebase(struct rebase_options *opts) return status ? -1 : 0; } +static void parse_merges_value(struct rebase_options *options, const char *value) +{ + if (value) { + if (!strcmp("no-rebase-cousins", value))
If you want to support `rebase.merges=` to imply `no-rebase-cousins`, this
would be the correct place to do it:
if (!*value || !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;
I would expect `options->rebase_merges = 0` if `value == NULL`. IOW I
would have expected `parse_merges_value()` to start with:
if (!value) {
options->rebase_merges = 0;
return;
}
This assumes, of course, the parse_options semantics, where a `--no-*` option
passes `NULL` as argument to the callback.
However, this is _not_ the parse_options callback, and if the (optional)
argument was not specified, we do end up with a `NULL` here in spite of
wanting to enable the rebase-merges mode.
However, a primary reason why you introduce the function is to support
config value parsing. And in config value parsing, a "maybe-bool" with a
NULL value is considered to be equivalent to `true`! (See
`git_parse_maybe_bool_text()` or
https://git-scm.com/docs/git-config#Documentation/git-config.txt-true for
details.). For example,
[http]
sslVerify
is equivalent to
[http]
sslVerify = true
But since `git_parse_maybe_bool()` already takes care of handling that
case (in which case we do not even want to call `git_parse_maybe_bool()`),
you can limit that function to handling the command-line semantics.
So with those confusingly disagreeing semantics, I see not only myself,
but other readers doing very, very well, indeed, with a code comment that
explains under what circumstances we expect this callback to be called
with `value == NULL`.
quoted hunk ↗ jump to hunk
+} + static int rebase_config(const char *var, const char *value, void *data) { struct rebase_options *opts = data;@@ -815,6 +829,13 @@ static int rebase_config(const char *var, const char *value, void *data) return 0; } + if (!strcmp(var, "rebase.merges") && value && *value) {
Why do we require a non-empty `value` here? [rebase] merges should be equivalent to `true`, [rebase] merges = should probably be equivalent to `false`, and both are handled correctly by `git_parse_maybe_bool()`.
quoted hunk ↗ jump to hunk
+ opts->rebase_merges = git_parse_maybe_bool(value); + if (opts->rebase_merges < 0) + parse_merges_value(opts, value); + return 0; + } + if (!strcmp(var, "rebase.backend")) { return git_config_string(&opts->default_backend, var, value); }@@ -980,6 +1001,18 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset) return 0; } +static int parse_opt_merges(const struct option *opt, const char *arg, int unset) +{ + struct rebase_options *options = opt->value; + + if (unset) + options->rebase_merges = 0; + else + parse_merges_value(options, arg); + + return 0; +} +
It is kind of inelegant to require a _second_ callback for the command-line parsing, but I guess if we want a `--no-rebase-merges` option to override a config setting, we cannot help it.
quoted hunk ↗ jump to hunk
static void NORETURN error_on_missing_default_upstream(void) { struct branch *current_branch = branch_get(NULL);@@ -1035,7 +1068,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) struct object_id branch_base; int ignore_whitespace = 0; const char *gpg_sign = NULL; - const char *rebase_merges = NULL; struct string_list strategy_options = STRING_LIST_INIT_NODUP; struct object_id squash_onto; char *squash_onto_name = NULL;@@ -1137,10 +1169,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) &options.allow_empty_message, N_("allow rebasing commits with empty messages"), PARSE_OPT_HIDDEN), - {OPTION_STRING, 'r', "rebase-merges", &rebase_merges, - N_("mode"), + OPT_CALLBACK_F('r', "rebase-merges", &options, N_("mode"), N_("try to rebase merges instead of skipping them"), - PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"}, + PARSE_OPT_OPTARG, parse_opt_merges), OPT_BOOL(0, "fork-point", &options.fork_point, N_("use 'merge-base --fork-point' to refine upstream")), OPT_STRING('s', "strategy", &options.strategy,@@ -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"); - } if (options.type == REBASE_APPLY) { if (ignore_whitespace)diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index c73949df18..d4b0e8fd49 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh@@ -284,6 +284,102 @@ test_expect_success '--rebase-merges="" is invalid syntax' ' test_cmp expect actual ' +test_expect_success 'rebase.merges="" is equivalent to not passing --rebase-merges' ' + test_config rebase.merges "" && + git checkout -b config-merges-blank E && + git rebase C && + test_cmp_graph C.. <<-\EOF + * B + * D + o C + EOF +' + +test_expect_success 'rebase.merges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' ' + test_config rebase.merges rebase-cousins && + git checkout -b config-rebase-cousins main && + git rebase HEAD^ && + test_cmp_graph HEAD^.. <<-\EOF + * Merge the topic branch '\''onebranch'\'' + |\ + | * D + | * G + |/ + o H + EOF +' + +test_expect_success '--no-rebase-merges overrides rebase.merges=no-rebase-cousins' ' + test_config rebase.merges no-rebase-cousins && + git checkout -b override-config-no-rebase-cousins E && + git rebase --no-rebase-merges C && + test_cmp_graph C.. <<-\EOF + * B + * D + o C + EOF +' + +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 +' + +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 +' + +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^ && + test_cmp_graph HEAD^.. <<-\EOF + * Merge the topic branch '\''onebranch'\'' + |\ + | * D + | * G + |/ + o H + EOF +' + +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 +' +
I understand the temptation to introduce exhaustive matrices that test all the different settings in all the different ways they can be specified. However, I would much prefer to keep the tests succinct, not the least to avoid the every-increasing runtime of Git's CI. It's already taking about an order of magnitude or two too long to be reasonable. So I'd suggest reducing the tests to a single one instead of eight: verify that `rebase.merges=no-rebase-cousins` is heeded, and that `--no-rebase-cousins` overrides that. That should be plenty sufficient to prevent regressions. Ciao, Johannes
test_expect_success 'refs/rewritten/* is worktree-local' ' git worktree add wt && cat >wt/script-from-scratch <<-\EOF && -- 2.39.2