| Message ID | 20230223053410.644503-3-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/3] rebase: add documentation and test for --no-rebase-merges | expand |
Hi Alex, On Wed, 22 Feb 2023, Alex Henrie wrote: > 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`. > +} > + > 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()`. > + 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. > 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 > >
Hi Alex On 23/02/2023 05:34, Alex Henrie wrote: > 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 <alexhenrie24@gmail.com> > --- > 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 > [...] > @@ -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
On Fri, Feb 24, 2023 at 6:53 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > 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.). > On Wed, 22 Feb 2023, Alex Henrie wrote: > > > + 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()`. I didn't know that there was already an established convention for what NULL and "" mean for boolean config values. I should have looked at the source code of git_parse_maybe_bool more carefully. That does change things because we're going to want to follow the established convention here. -Alex
On Fri, Feb 24, 2023 at 7:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > Thanks for updating this, it is much clearer what the different settings > mean now. Happy to help. Thanks for the feedback. > I not sure if having the config setting override the default > when the user passes --rebase-merges without an argument is a good idea. > 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 In that case, we're going to need two config options: A rebase.merges boolean for whether --rebase-merges should be on by default and a rebase.cousins boolean for whether rebase-cousins or no-rebase-cousins is the default. I will try to incorporate that change and the others that you and Johannes suggested in v5. -Alex
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`. diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index c98784a0d2..b02f9cbb8c 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -537,7 +537,8 @@ See also INCOMPATIBLE OPTIONS below. by recreating the merge commits. Any resolved merge conflicts or manual amendments in these merge commits will have to be resolved/re-applied manually. `--no-rebase-merges` can be used to - countermand a previous `--rebase-merges`. + countermand both the `rebase.merges` config option and a previous + `--rebase-merges`. + By default, or when `no-rebase-cousins` was specified, commits which do not have `<upstream>` as direct ancestor will keep their original branch point, 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)) + options->rebase_cousins = 0; + else if (!strcmp("rebase-cousins", value)) + options->rebase_cousins = 1; + else + die(_("Unknown mode: %s"), value); + } + + options->rebase_merges = 1; +} + 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) { + 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; +} + 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 +' + test_expect_success 'refs/rewritten/* is worktree-local' ' git worktree add wt && cat >wt/script-from-scratch <<-\EOF &&
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 <alexhenrie24@gmail.com> --- 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(-)