| Message ID | 20230225180325.796624-4-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: add a config option for --rebase-merges | expand |
Alex Henrie <alexhenrie24@gmail.com> writes: > +rebase.merges:: As far as I can tell, when a config option is meant to be a stand in for a CLI option, we have typically named it <subcommand>.<camelcase option>, e.g. grep.fullName, log.diffMerges, push.followTags. This probably matters more if we aren't specifying the subcommand that this flag is more (like in this patch). By this convention, the config option would be rebase.rebaseMerges, which _does_ feel redundant and it's unlikely for "git rebase" to learn a "--merges" flag, so I don't have a strong opinion either way. Though as you mentioned in review club, this consistency might make it easier for users to read custom configs that use the "rebase." section. > + 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`. With the explanation here, I think having "--rebase-merges" _not_ override "rebase.merge=rebase-cousins" is quite confusing. I think there are two possibilities forward: - Be very consistent that "--rebase-merges" is just a synonym of "--rebase-merges=no-rebase-cousins". Then, have any value of "--rebase-merges" override any config value. I think this the easiest UX to understand, but I don't think we should be commiting to a default, especially since we may add a more sensible default (like rebasing 'evil merges') in the future. - Keep the existing behavior, but reword the docs to indicate that the "no value" form means "I want the default but don't care what it is", and an explicit value means "I want to use a particular mode and the rules of config vs CLI specificity should apply". Perhaps something like: When rebasing merges, there are two modes: `no-rebase-cousins` and `rebase-cousins`. If no mode is specified in either the config or CLI, it defaults to `no-rebase-cousins`. which I think is clearer that "rebase.merges=some-val" will win out over "--rebase-merges". What's nice is that we don't commit to a default value, so we can change it without having to update much of the wording here. The code looks good, no real comments there.
Hi Alex On 25/02/2023 18:03, 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 | 79 ++++++++++++++++++-------- > t/t3422-rebase-incompatible-options.sh | 12 ++++ > t/t3430-rebase-merges.sh | 63 ++++++++++++++++++++ > 5 files changed, 143 insertions(+), 24 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`. 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. > [...] > @@ -1513,13 +1539,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > break; > > if (i >= 0 || options.type == REBASE_APPLY) { > - if (is_merge(&options)) > - die(_("apply options and merge options " > - "cannot be used together")); > - else if (options.autosquash == -1 && options.config_autosquash == 1) > + if (options.autosquash == -1 && options.config_autosquash == 1) > die(_("apply options are incompatible with rebase.autosquash. Consider adding --no-autosquash")); > + else if (options.rebase_merges == -1 && options.config_rebase_merges == 1) > + die(_("apply options are incompatible with rebase.merges. Consider adding --no-rebase-merges")); This is good, thanks for adding it > [...] > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > index f50fbf1390..a825a77fb4 100755 > --- a/t/t3430-rebase-merges.sh > +++ b/t/t3430-rebase-merges.sh > @@ -283,6 +283,69 @@ test_expect_success '--rebase-merges="" is deprecated' ' > grep deprecated actual > ' > > +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 > +' 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. > +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? Best Wishes Phillip > +' > + > +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 'refs/rewritten/* is worktree-local' ' > git worktree add wt && > cat >wt/script-from-scratch <<-\EOF &&
> + > +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 originally believed that having the boolean values as an option here created unnecessary complexity but I was convinced during Review Club that the tradeoff for a user who just wants to turn this on is fine. I'm just wondering whether this option will possibly have additional values in the future. If so, what would a user expect from setting this config to true? Small typo at the end s/rebase.merge/rebase.merges > 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 ccc13200b5..ac096f27e1 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -123,6 +123,7 @@ struct rebase_options { > int fork_point; > int update_refs; > int config_autosquash; > + int config_rebase_merges; > int config_update_refs; > }; > > @@ -140,6 +141,8 @@ struct rebase_options { > .allow_empty_message = 1, \ > .autosquash = -1, \ > .config_autosquash = -1, \ > + .rebase_merges = -1, \ > + .config_rebase_merges = -1, \ > .update_refs = -1, \ > .config_update_refs = -1, \ > } > @@ -771,6 +774,16 @@ static int run_specific_rebase(struct rebase_options *opts) > return status ? -1 : 0; > } > > +static void parse_rebase_merges_value(struct rebase_options *options, const char *value) > +{ > + if (!strcmp("no-rebase-cousins", value)) > + options->rebase_cousins = 0; > + else if (!strcmp("rebase-cousins", value)) > + options->rebase_cousins = 1; > + else > + die(_("Unknown rebase-merges mode: %s"), value); > +} > + > static int rebase_config(const char *var, const char *value, void *data) > { > struct rebase_options *opts = data; > @@ -800,6 +813,15 @@ static int rebase_config(const char *var, const char *value, void *data) > return 0; > } > > + if (!strcmp(var, "rebase.merges")) { > + opts->config_rebase_merges = git_parse_maybe_bool(value); > + if (opts->config_rebase_merges < 0) { > + opts->config_rebase_merges = 1; > + parse_rebase_merges_value(opts, value); > + } > + return 0; > + } > + > if (!strcmp(var, "rebase.updaterefs")) { > opts->config_update_refs = git_config_bool(var, value); > return 0; > @@ -980,6 +1002,27 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset) > return 0; > } > > +static int parse_opt_rebase_merges(const struct option *opt, const char *arg, int unset) > +{ > + struct rebase_options *options = opt->value; > + > + options->rebase_merges = !unset; > + > + if (arg) { > + if (!*arg) { > + warning(_("--rebase-merges with an empty string " > + "argument is deprecated and will stop " > + "working in a future version of Git. Use " > + "--rebase-merges=no-rebase-cousins " > + "instead.")); > + arg = "no-rebase-cousins"; > + } > + parse_rebase_merges_value(options, arg); > + } > + > + return 0; > +} > + > static void NORETURN error_on_missing_default_upstream(void) > { > struct branch *current_branch = branch_get(NULL); > @@ -1035,7 +1078,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 +1179,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_rebase_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,21 +1477,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > if (options.exec.nr) > imply_merge(&options, "--exec"); > > - if (rebase_merges) { > - if (!*rebase_merges) > - warning(_("--rebase-merges with an empty string " > - "argument is deprecated and will stop " > - "working in a future version of Git. Use " > - "--rebase-merges=no-rebase-cousins " > - "instead.")); > - else 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; > - imply_merge(&options, "--rebase-merges"); > - } > - > if (options.type == REBASE_APPLY) { > if (ignore_whitespace) > strvec_push(&options.git_am_opts, > @@ -1513,13 +1539,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > break; > > if (i >= 0 || options.type == REBASE_APPLY) { > - if (is_merge(&options)) > - die(_("apply options and merge options " > - "cannot be used together")); > - else if (options.autosquash == -1 && options.config_autosquash == 1) > + if (options.autosquash == -1 && options.config_autosquash == 1) > die(_("apply options are incompatible with rebase.autosquash. Consider adding --no-autosquash")); > + else if (options.rebase_merges == -1 && options.config_rebase_merges == 1) > + die(_("apply options are incompatible with rebase.merges. Consider adding --no-rebase-merges")); > else if (options.update_refs == -1 && options.config_update_refs == 1) > die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs")); > + else if (is_merge(&options)) > + die(_("apply options and merge options " > + "cannot be used together")); > else > options.type = REBASE_APPLY; > } > @@ -1530,6 +1558,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > options.update_refs = (options.update_refs >= 0) ? options.update_refs : > ((options.config_update_refs >= 0) ? options.config_update_refs : 0); > > + if (options.rebase_merges == 1) > + imply_merge(&options, "--rebase-merges"); > + options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges : > + ((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0); > + > if (options.autosquash == 1) > imply_merge(&options, "--autosquash"); > options.autosquash = (options.autosquash >= 0) ? options.autosquash : > diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh > index 4711b37a28..af93f13849 100755 > --- a/t/t3422-rebase-incompatible-options.sh > +++ b/t/t3422-rebase-incompatible-options.sh > @@ -101,6 +101,12 @@ test_rebase_am_only () { > grep -e --no-autosquash err > " > > + test_expect_success "$opt incompatible with rebase.merges" " > + git checkout B^0 && > + test_must_fail git -c rebase.merges=true rebase $opt A 2>err && > + grep -e --no-rebase-merges err > + " > + Missing an additional test here "$opt incompatible with --rebase-merges" to match the test patterns from the other option/config combos. > test_expect_success "$opt incompatible with rebase.updateRefs" " > git checkout B^0 && > test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err && > @@ -113,6 +119,12 @@ test_rebase_am_only () { > git -c rebase.autosquash=true rebase --no-autosquash $opt A > " > > + test_expect_success "$opt okay with overridden rebase.merges" " > + test_when_finished \"git reset --hard B^0\" && > + git checkout B^0 && > + git -c rebase.merges=true rebase --no-rebase-merges $opt A > + " > + > test_expect_success "$opt okay with overridden rebase.updateRefs" " > test_when_finished \"git reset --hard B^0\" && > git checkout B^0 && > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > index f50fbf1390..a825a77fb4 100755 > --- a/t/t3430-rebase-merges.sh > +++ b/t/t3430-rebase-merges.sh > @@ -283,6 +283,69 @@ test_expect_success '--rebase-merges="" is deprecated' ' > grep deprecated actual > ' > > +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 'refs/rewritten/* is worktree-local' ' > git worktree add wt && > cat >wt/script-from-scratch <<-\EOF && > -- > 2.39.2 > > >
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: > > +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
On Thu, Mar 2, 2023 at 11:02 AM Calvin Wan <calvinwan@google.com> 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 originally believed that having the boolean values as an option here > created unnecessary complexity but I was convinced during Review Club > that the tradeoff for a user who just wants to turn this on is fine. I'm > just wondering whether this option will possibly have additional values > in the future. If so, what would a user expect from setting this config > to true? "true" will always mean "rebase merges in the default way, I don't care what it is". During a time period when the default is transitioning, "true" means that the user will get a warning, but there's nothing wrong with continuing to use "true" if they really don't care. -Alex
Hi Alex On 04/03/2023 23:24, Alex Henrie wrote: > 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: > >>> +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. It is good to try and future proof things but this seems rather hypothetical. I don't really see how the choice of whether --rebase-merges is turned on by default is related to the choice of whether or not to rebase cousins by default. It is worth noting that the default was changed to from rebase-cousins to no-rebase-cousins early in the development of --rebase-merges[1] as it was felt to be less surprising. > 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. But aren't you breaking those aliases now when rebase.rebaseMerges=rebase-cousins? That's what I'm objecting to. It seems like we're breaking things now to avoid a hypothetical future change breaking them which does not seem like the right trade off to me. It also does not fit with the way other optional arguments interact with their associated config setting. For example "git branch/checkout/switch --track" and branch.autoSetupMerge. If the optional argument to --track is omitted it defaults to "direct" irrespective of the config. [1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.1801292251240.35@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz/ > 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. The documentation in v6 is certainly quite clear on this point. >>> +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. Given we're using the same code to parse the command line argument and the config setting and we have a test for rebase.rebaseMerges=no-rebase-cousins I think we could drop it. >>> +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. It's not doing nothing though - it is rebasing the branch, it just happens that everything fast-forwards so HEAD ends up unchanged. My point is that this test should verify the branch has been rebased. Maybe you could check the reflog message for HEAD@{0} is rebase (finish): returning to refs/heads/override-config-merges-false > 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? By using --force and checking the graph you check that the rebase actually happened. Thanks for working on this Phillip > Thanks for the feedback, > > -Alex
On Tue, Mar 7, 2023 at 9:23 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > On 04/03/2023 23:24, Alex Henrie wrote: > > 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: > > > >>> +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. > > It is good to try and future proof things but this seems rather > hypothetical. I don't really see how the choice of whether > --rebase-merges is turned on by default is related to the choice of > whether or not to rebase cousins by default. It is worth noting that the > default was changed to from rebase-cousins to no-rebase-cousins early in > the development of --rebase-merges[1] as it was felt to be less surprising. > [1] > https://lore.kernel.org/git/nycvar.QRO.7.76.6.1801292251240.35@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz/ Thank you for sharing that link. Even though I got the tests right, I got confused and started thinking that rebase-cousins was a more thorough version of rebase-merges. In fact, they do opposite things: rebase-merges tries to preserve the graph and rebase-cousins intentionally restructures the graph. In my opinion using the word "rebase" in the names of both options was another unfortunate UI decision, but I understand the difference now. > > 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. > > But aren't you breaking those aliases now when > rebase.rebaseMerges=rebase-cousins? That's what I'm objecting to. It > seems like we're breaking things now to avoid a hypothetical future > change breaking them which does not seem like the right trade off to me. > > It also does not fit with the way other optional arguments interact with > their associated config setting. For example "git branch/checkout/switch > --track" and branch.autoSetupMerge. If the optional argument to --track > is omitted it defaults to "direct" irrespective of the config. What I really don't want is to paint ourselves into a corner. You're right that it's unlikely that the default will ever change from no-rebase-cousins to rebase-cousins; I was mistaken. However, Glen thinks that in the future we might have some kind of rebase-evil-merges mode as well, and that that might become the default. If we don't let the rebase.rebaseMerges config value control the default behavior of --rebase-merges without an argument on the command line, we would have to introduce a separate config option for the transition, which would be ugly. More voices would be helpful here. Does anyone else have an opinion on how likely it is that the default rebase-merges mode will change in the future? Or on whether rebase.rebaseMerges should be allowed to affect --rebase-merges in order to facilitate such a change? > >>> +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. > > It's not doing nothing though - it is rebasing the branch, it just > happens that everything fast-forwards so HEAD ends up unchanged. My > point is that this test should verify the branch has been rebased. Maybe > you could check the reflog message for HEAD@{0} is > > rebase (finish): returning to refs/heads/override-config-merges-false > > > 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? > > By using --force and checking the graph you check that the rebase > actually happened. I got the impression that people like that not checking the graph itself (or the reflog) makes the tests more concise, but I don't care much either way. For what it's worth, the way I did it matches the existing tests in the file. If you can find at least one other person who thinks that it ought to change for this patch series to be accepted, and no one else objects, I'll change it. > Thanks for working on this You're welcome; hopefully we can get the remaining details ironed out quickly. -Alex
Hi Alex On 12/03/2023 20:57, Alex Henrie wrote: > On Tue, Mar 7, 2023 at 9:23 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > >> On 04/03/2023 23:24, Alex Henrie wrote: >>> 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: >>> >>>>> +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. >> >> It is good to try and future proof things but this seems rather >> hypothetical. I don't really see how the choice of whether >> --rebase-merges is turned on by default is related to the choice of >> whether or not to rebase cousins by default. It is worth noting that the >> default was changed to from rebase-cousins to no-rebase-cousins early in >> the development of --rebase-merges[1] as it was felt to be less surprising. > >> [1] >> https://lore.kernel.org/git/nycvar.QRO.7.76.6.1801292251240.35@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz/ > > Thank you for sharing that link. Even though I got the tests right, I > got confused and started thinking that rebase-cousins was a more > thorough version of rebase-merges. In fact, they do opposite things: > rebase-merges tries to preserve the graph and rebase-cousins > intentionally restructures the graph. In my opinion using the word > "rebase" in the names of both options was another unfortunate UI > decision, but I understand the difference now. > >>> 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. >> >> But aren't you breaking those aliases now when >> rebase.rebaseMerges=rebase-cousins? That's what I'm objecting to. It >> seems like we're breaking things now to avoid a hypothetical future >> change breaking them which does not seem like the right trade off to me. >> >> It also does not fit with the way other optional arguments interact with >> their associated config setting. For example "git branch/checkout/switch >> --track" and branch.autoSetupMerge. If the optional argument to --track >> is omitted it defaults to "direct" irrespective of the config. > > What I really don't want is to paint ourselves into a corner. Nor do I but we already seem to be in some kind of corner as what you're proposing breaks the status quo and the existing convention that command line options override config settings. > You're > right that it's unlikely that the default will ever change from > no-rebase-cousins to rebase-cousins; I was mistaken. However, Glen > thinks that in the future we might have some kind of > rebase-evil-merges mode as well, and that that might become the > default. If we don't let the rebase.rebaseMerges config value control > the default behavior of --rebase-merges without an argument on the > command line, we would have to introduce a separate config option for > the transition, which would be ugly. I'm optimistic that Elijah's work on rebasing evil merges will allow us to improve --rebase-merges. I expect that we'll enable it by default once it is merged. I do not think that we should add another argument for --rebase-merges to disable it though as that would be orthogonal to "rebase-cousins" and "no-rebase-cousins" which control what commits get rebased not how merges are rebased. If users want to disable the better rebasing of merges we'll need to add a --remerge option or something like that. > More voices would be helpful here. Does anyone else have an opinion on > how likely it is that the default rebase-merges mode will change in > the future? Or on whether rebase.rebaseMerges should be allowed to > affect --rebase-merges in order to facilitate such a change? Getting other's views would indeed be helpful Best Wishes Phillip >>>>> +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. >> >> It's not doing nothing though - it is rebasing the branch, it just >> happens that everything fast-forwards so HEAD ends up unchanged. My >> point is that this test should verify the branch has been rebased. Maybe >> you could check the reflog message for HEAD@{0} is >> >> rebase (finish): returning to refs/heads/override-config-merges-false >> >>> 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? >> >> By using --force and checking the graph you check that the rebase >> actually happened. > > I got the impression that people like that not checking the graph > itself (or the reflog) makes the tests more concise, but I don't care > much either way. For what it's worth, the way I did it matches the > existing tests in the file. If you can find at least one other person > who thinks that it ought to change for this patch series to be > accepted, and no one else objects, I'll change it. > >> Thanks for working on this > > You're welcome; hopefully we can get the remaining details ironed out quickly. > > -Alex
On Mon, Mar 13, 2023 at 9:38 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > On 12/03/2023 20:57, Alex Henrie wrote: > > More voices would be helpful here. Does anyone else have an opinion on > > how likely it is that the default rebase-merges mode will change in > > the future? Or on whether rebase.rebaseMerges should be allowed to > > affect --rebase-merges in order to facilitate such a change? > > Getting other's views would indeed be helpful My view is that we shouldn't make assumptions about what is going to happen in the future. Most of the UI design warts of Git come precisely from the fact that somebody in the past did not think something in the future would happen, and when it does the excuse to not do what's sensible in the UI is "it's too late now". I would vote for whatever is more flexible to whatever happens in the future, not something that seems to make sense now, but adds yet more inertia and later on becomes yet another wart in the list of "oh, if only we had considered that back in 2023". Cheers.
Alex Henrie <alexhenrie24@gmail.com> writes: > What I really don't want is to paint ourselves into a corner. You're > right that it's unlikely that the default will ever change from > no-rebase-cousins to rebase-cousins; I was mistaken. However, Glen > thinks that in the future we might have some kind of > rebase-evil-merges mode as well, and that that might become the > default. If we don't let the rebase.rebaseMerges config value control > the default behavior of --rebase-merges without an argument on the > command line, we would have to introduce a separate config option for > the transition, which would be ugly. > > More voices would be helpful here. Does anyone else have an opinion on > how likely it is that the default rebase-merges mode will change in > the future? Or on whether rebase.rebaseMerges should be allowed to > affect --rebase-merges in order to facilitate such a change? Any such "opinion" about the future, or the belief that we can somehow predict one, would be wrong anyway. So I am not sure soliciting more voices to look into crystal balls would help all that much. Having said that... The default rebase-merges behaviour may be improved, but changing it in a totally backward incompatible way _without_ a carefully prepared transition strategy will be very unlikely, simply because existing end users would not allow us to. I do not offhand know if the configuration variable(s) you propose would serve as a good mechanism to help such transition. Thanks.
Hi Alex, Heads up: This mail is totally a tangent and is _not_ intended to distract from the patch series. On Sun, 12 Mar 2023, Alex Henrie wrote: > [...] Glen thinks that in the future we might have some kind of > rebase-evil-merges mode as well, and that that might become the default. While it probably won't be the default, like, ever, I can well imagine that there would be a good opportunity to add a config setting to make it the default _for that user_. About implementing that mode... I recently picked up a repair item from my ever-growing TODO list to brush up that "replay merge conflict resolutions and amendments" idea of Phillip's that I had implemented already four years ago (only to find out that it unfortunately caused a lot of nested merge conflicts when using it on Git for Windows' branch thicket; I tried my best to add a test case demonstrating the problematic behavior). While I had to stop working on it again, I at least left a rebased version at https://github.com/gitgitgadget/git/pull/1491 (in case anybody is interested where I wanted to take this PR, I left comments describing the changes I had planned on looking into). I mention this PR here in this thread not so much to derail the review, or even to distract from this patch series (I really detest when that happens in reviews of my own patches, so please accept my apology in advance should that happen here, it is _really_ not my intention), but as a point in favor of Glen's suspicion that we might get such a mode in the future to replay evil merges or merges with resolved conflicts. Ciao, Johannes
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 ccc13200b5..ac096f27e1 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -123,6 +123,7 @@ struct rebase_options { int fork_point; int update_refs; int config_autosquash; + int config_rebase_merges; int config_update_refs; }; @@ -140,6 +141,8 @@ struct rebase_options { .allow_empty_message = 1, \ .autosquash = -1, \ .config_autosquash = -1, \ + .rebase_merges = -1, \ + .config_rebase_merges = -1, \ .update_refs = -1, \ .config_update_refs = -1, \ } @@ -771,6 +774,16 @@ static int run_specific_rebase(struct rebase_options *opts) return status ? -1 : 0; } +static void parse_rebase_merges_value(struct rebase_options *options, const char *value) +{ + if (!strcmp("no-rebase-cousins", value)) + options->rebase_cousins = 0; + else if (!strcmp("rebase-cousins", value)) + options->rebase_cousins = 1; + else + die(_("Unknown rebase-merges mode: %s"), value); +} + static int rebase_config(const char *var, const char *value, void *data) { struct rebase_options *opts = data; @@ -800,6 +813,15 @@ static int rebase_config(const char *var, const char *value, void *data) return 0; } + if (!strcmp(var, "rebase.merges")) { + opts->config_rebase_merges = git_parse_maybe_bool(value); + if (opts->config_rebase_merges < 0) { + opts->config_rebase_merges = 1; + parse_rebase_merges_value(opts, value); + } + return 0; + } + if (!strcmp(var, "rebase.updaterefs")) { opts->config_update_refs = git_config_bool(var, value); return 0; @@ -980,6 +1002,27 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset) return 0; } +static int parse_opt_rebase_merges(const struct option *opt, const char *arg, int unset) +{ + struct rebase_options *options = opt->value; + + options->rebase_merges = !unset; + + if (arg) { + if (!*arg) { + warning(_("--rebase-merges with an empty string " + "argument is deprecated and will stop " + "working in a future version of Git. Use " + "--rebase-merges=no-rebase-cousins " + "instead.")); + arg = "no-rebase-cousins"; + } + parse_rebase_merges_value(options, arg); + } + + return 0; +} + static void NORETURN error_on_missing_default_upstream(void) { struct branch *current_branch = branch_get(NULL); @@ -1035,7 +1078,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 +1179,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_rebase_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,21 +1477,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (options.exec.nr) imply_merge(&options, "--exec"); - if (rebase_merges) { - if (!*rebase_merges) - warning(_("--rebase-merges with an empty string " - "argument is deprecated and will stop " - "working in a future version of Git. Use " - "--rebase-merges=no-rebase-cousins " - "instead.")); - else 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; - imply_merge(&options, "--rebase-merges"); - } - if (options.type == REBASE_APPLY) { if (ignore_whitespace) strvec_push(&options.git_am_opts, @@ -1513,13 +1539,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) break; if (i >= 0 || options.type == REBASE_APPLY) { - if (is_merge(&options)) - die(_("apply options and merge options " - "cannot be used together")); - else if (options.autosquash == -1 && options.config_autosquash == 1) + if (options.autosquash == -1 && options.config_autosquash == 1) die(_("apply options are incompatible with rebase.autosquash. Consider adding --no-autosquash")); + else if (options.rebase_merges == -1 && options.config_rebase_merges == 1) + die(_("apply options are incompatible with rebase.merges. Consider adding --no-rebase-merges")); else if (options.update_refs == -1 && options.config_update_refs == 1) die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs")); + else if (is_merge(&options)) + die(_("apply options and merge options " + "cannot be used together")); else options.type = REBASE_APPLY; } @@ -1530,6 +1558,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.update_refs = (options.update_refs >= 0) ? options.update_refs : ((options.config_update_refs >= 0) ? options.config_update_refs : 0); + if (options.rebase_merges == 1) + imply_merge(&options, "--rebase-merges"); + options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges : + ((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0); + if (options.autosquash == 1) imply_merge(&options, "--autosquash"); options.autosquash = (options.autosquash >= 0) ? options.autosquash : diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh index 4711b37a28..af93f13849 100755 --- a/t/t3422-rebase-incompatible-options.sh +++ b/t/t3422-rebase-incompatible-options.sh @@ -101,6 +101,12 @@ test_rebase_am_only () { grep -e --no-autosquash err " + test_expect_success "$opt incompatible with rebase.merges" " + git checkout B^0 && + test_must_fail git -c rebase.merges=true rebase $opt A 2>err && + grep -e --no-rebase-merges err + " + test_expect_success "$opt incompatible with rebase.updateRefs" " git checkout B^0 && test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err && @@ -113,6 +119,12 @@ test_rebase_am_only () { git -c rebase.autosquash=true rebase --no-autosquash $opt A " + test_expect_success "$opt okay with overridden rebase.merges" " + test_when_finished \"git reset --hard B^0\" && + git checkout B^0 && + git -c rebase.merges=true rebase --no-rebase-merges $opt A + " + test_expect_success "$opt okay with overridden rebase.updateRefs" " test_when_finished \"git reset --hard B^0\" && git checkout B^0 && diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index f50fbf1390..a825a77fb4 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -283,6 +283,69 @@ test_expect_success '--rebase-merges="" is deprecated' ' grep deprecated actual ' +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 '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 | 79 ++++++++++++++++++-------- t/t3422-rebase-incompatible-options.sh | 12 ++++ t/t3430-rebase-merges.sh | 63 ++++++++++++++++++++ 5 files changed, 143 insertions(+), 24 deletions(-)