| Message ID | 20230305050709.68736-4-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rebase: document, clean up, and introduce a config option for --rebase-merges | expand |
Hi Alex On 05/03/2023 05:07, 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 turning on > --rebase-merges by default without configuration in a future version of > Git. > > Name the new option rebase.rebaseMerges, even though it is a little > redundant, for consistency with the name of the command line option and > to be clear when scrolling through values in the [rebase] section of > .gitconfig. > > In the future, the default rebase-merges mode may change from > no-rebase-cousins to rebase-cousins. Support setting rebase.rebaseMerges > to the nonspecific value "true" for users who do not need or want to > care about the default changing in the future. Similarly, for users who > have --rebase-merges in an alias and want to get the future behavior > now, use the specific rebase-merges mode from the config if a specific > mode is not given on the command line. > > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > Documentation/config/rebase.txt | 11 ++++ > Documentation/git-rebase.txt | 17 +++--- > builtin/rebase.c | 79 ++++++++++++++++++-------- > t/t3422-rebase-incompatible-options.sh | 17 ++++++ > t/t3430-rebase-merges.sh | 68 ++++++++++++++++++++++ > 5 files changed, 161 insertions(+), 31 deletions(-) > > diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt > index f19bd0e040..f7d3218b1d 100644 > --- a/Documentation/config/rebase.txt > +++ b/Documentation/config/rebase.txt > @@ -67,3 +67,14 @@ rebase.rescheduleFailedExec:: > > rebase.forkPoint:: > If set to false set `--no-fork-point` option by default. > + > +rebase.rebaseMerges:: > + 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, This is a bit picky but how can rebase.rebaseMerges=true be equivalent to --rebase-merges without an argument when the behavior of --rebase-merges without an argument depends on the value of rebase.rebaseMerges? > 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.rebaseMerges=false` > + configuration, but the absence of a specific rebase-merges mode on the > + command line does not counteract a specific mode set in the configuration. I may not agree the with the design choice but the documentation here and below is very clear about the behavior of --rebase-merges without an argument which is good. > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 4e57a87624..6ec86c9c6e 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -537,16 +537,17 @@ 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.rebaseMerges` config option and a previous > + `--rebase-merges`. > + > When rebasing merges, there are two modes: `rebase-cousins` and > -`no-rebase-cousins`. If the mode is not specified, it defaults to > -`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have > -`<upstream>` as direct ancestor will keep their original branch point, i.e. > -commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path` > -option will keep their original ancestry by default. In `rebase-cousins` mode, > -such commits are instead rebased onto `<upstream>` (or `<onto>`, if > -specified). > +`no-rebase-cousins`. If the mode is not specified on the command line or in > +the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`. > +In `no-rebase-cousins` mode, commits which do not have `<upstream>` as direct > +ancestor will keep their original branch point, i.e. commits that would be > +excluded by linkgit:git-log[1]'s `--ancestry-path` option will keep their > +original ancestry by default. In `rebase-cousins` mode, such commits are > +instead rebased onto `<upstream>` (or `<onto>`, if specified). >[...] > 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.rebasemerges")) { > + 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); > + } I think we need } else { opts->rebase_cousins = 0; } here. Otherwise if rebase.rebaseMerges is set twice we wont follow the usual "last config wins" convention. For example [rebase] rebaseMerges=rebase-cousins rebaseMerges=true will result in us unexpectedly rebasing cousins The rest of the patch looks fine Best Wishes Phillip > + 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 without an argument " > + "instead, which does the same thing.")); > + return 0; > + } > + 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 without an argument " > - "instead, which does the same thing.")); > - 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.rebaseMerges. 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..2eba00bdf5 100755 > --- a/t/t3422-rebase-incompatible-options.sh > +++ b/t/t3422-rebase-incompatible-options.sh > @@ -85,6 +85,11 @@ test_rebase_am_only () { > test_must_fail git rebase $opt --reapply-cherry-picks A > " > > + test_expect_success "$opt incompatible with --rebase-merges" " > + git checkout B^0 && > + test_must_fail git rebase $opt --rebase-merges A > + " > + > test_expect_success "$opt incompatible with --update-refs" " > git checkout B^0 && > test_must_fail git rebase $opt --update-refs A > @@ -101,6 +106,12 @@ test_rebase_am_only () { > grep -e --no-autosquash err > " > > + test_expect_success "$opt incompatible with rebase.rebaseMerges" " > + git checkout B^0 && > + test_must_fail git -c rebase.rebaseMerges=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 +124,12 @@ test_rebase_am_only () { > git -c rebase.autosquash=true rebase --no-autosquash $opt A > " > > + test_expect_success "$opt okay with overridden rebase.rebaseMerges" " > + test_when_finished \"git reset --hard B^0\" && > + git checkout B^0 && > + git -c rebase.rebaseMerges=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 d46d9545f2..aa75e192d1 100755 > --- a/t/t3430-rebase-merges.sh > +++ b/t/t3430-rebase-merges.sh > @@ -278,6 +278,74 @@ test_expect_success 'do not rebase cousins unless asked for' ' > EOF > ' > > +test_expect_success '--rebase-merges="" is deprecated' ' > + git rebase --rebase-merges="" HEAD^ 2>actual && > + grep deprecated actual > +' > + > +test_expect_success 'rebase.rebaseMerges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' ' > + test_config rebase.rebaseMerges 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.rebaseMerges=no-rebase-cousins' ' > + test_config rebase.rebaseMerges 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.rebaseMerges=rebase-cousins' ' > + test_config rebase.rebaseMerges 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.rebaseMerges=false' ' > + test_config rebase.rebaseMerges 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.rebaseMerges=rebase-cousins' ' > + test_config rebase.rebaseMerges 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 &&
Phillip Wood <phillip.wood123@gmail.com> writes: >> +rebase.rebaseMerges:: >> + 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, > > This is a bit picky but how can rebase.rebaseMerges=true be equivalent > to --rebase-merges without an argument when the behavior of > --rebase-merges without an argument depends on the value of > rebase.rebaseMerges? True. I think the configuration is meant to give (when set to something other than Boolean) the default value to the option "--rebase-merges" that is given without value, so setting to false should be a no-op (a command line option would override it if given, and if there is no command line option, --rebase-merges is not used by default), setting it to a specific value between cousin choices would give --rebase-merges=<value> unless --no-rebase-merges is given, but setting it to true here makes the result undefined, unless the built-in default between cousin choices is described here. "Setting to true is equivalent to setting to no-rebase-cousins" and "Setting to false is a no-op but accepted only for completeness", perhaps? >> 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.rebaseMerges=false` >> + configuration, but the absence of a specific rebase-merges mode on the >> + command line does not counteract a specific mode set in the configuration. > > I may not agree the with the design choice but the documentation here > and below is very clear about the behavior of --rebase-merges without > an argument which is good. Is it? rebase.rebaseMerges=true does not give "a specific mode set in the configuration", so it still is unclear what --rebase-merges should do in that case. Unless what it means to set it to true is described, as you pointed out above, that is. >> +`no-rebase-cousins`. If the mode is not specified on the command line or in >> +the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`. This side could describe what setting it to "true" means, but it is a separate page so it would be more friendly to readers to cover it on both pages. > I think we need > > } else { > opts->rebase_cousins = 0; > } > > here. Otherwise if rebase.rebaseMerges is set twice we wont follow the > usual "last config wins" convention. For example > > [rebase] > rebaseMerges=rebase-cousins > rebaseMerges=true > > will result in us unexpectedly rebasing cousins Thanks for a careful review.
Alex Henrie <alexhenrie24@gmail.com> writes: > The purpose of the new option is to accommodate users who would like > --rebase-merges to be on by default and to facilitate turning on > --rebase-merges by default without configuration in a future version of > Git. > > Name the new option rebase.rebaseMerges, even though it is a little > redundant, for consistency with the name of the command line option and > to be clear when scrolling through values in the [rebase] section of > .gitconfig. This rationale makes sense to me. > In the future, the default rebase-merges mode may change from > no-rebase-cousins to rebase-cousins. I suspect a more likely future would be that the default is to rebase 'evil' merges instead of trying to recreate merge commits, but of course, the important thing is that we promote the default, not what the default will be... > Support setting rebase.rebaseMerges > to the nonspecific value "true" for users who do not need or want to > care about the default changing in the future. Similarly, for users who > have --rebase-merges in an alias and want to get the future behavior > now, use the specific rebase-merges mode from the config if a specific > mode is not given on the command line. so this rationale makes sense to me too :) > @@ -278,6 +278,74 @@ test_expect_success 'do not rebase cousins unless asked for' ' > EOF > ' > > +test_expect_success '--rebase-merges="" is deprecated' ' > + git rebase --rebase-merges="" HEAD^ 2>actual && > + grep deprecated actual > +' I believe this used to be on 2/3, i.e. https://lore.kernel.org/git/20230225180325.796624-3-alexhenrie24@gmail.com/ but your cover letter suggests that it was removed. Mechanical error? The rest of changes look good (besides what others have spotted).
Phillip Wood <phillip.wood123@gmail.com> writes: >> @@ -800,6 +813,15 @@ static int rebase_config(const char *var, const char *value, void *data) >> return 0; >> } >> >> + if (!strcmp(var, "rebase.rebasemerges")) { >> + 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); >> + } > > I think we need > > } else { > opts->rebase_cousins = 0; > } > > here. Otherwise if rebase.rebaseMerges is set twice we wont follow the > usual "last config wins" convention. For example > > [rebase] > rebaseMerges=rebase-cousins > rebaseMerges=true > > will result in us unexpectedly rebasing cousins Ah, good catch. An alternative might be to use the "lookup" functions (e.g. repo_config_get_maybe_bool()), which already implement "last one wins" semantics. It's a bigger change, but it may be easier for future readers to understand. I don't have a strong opinion on which is the 'better' approach.
On Tue, Mar 7, 2023 at 11:32 AM Junio C Hamano <gitster@pobox.com> wrote: > > Phillip Wood <phillip.wood123@gmail.com> writes: > > >> +rebase.rebaseMerges:: > >> + 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, > > > > This is a bit picky but how can rebase.rebaseMerges=true be equivalent > > to --rebase-merges without an argument when the behavior of > > --rebase-merges without an argument depends on the value of > > rebase.rebaseMerges? > > True. I think the configuration is meant to give (when set to > something other than Boolean) the default value to the option > "--rebase-merges" that is given without value, so setting to false > should be a no-op (a command line option would override it if given, > and if there is no command line option, --rebase-merges is not used > by default), setting it to a specific value between cousin choices > would give --rebase-merges=<value> unless --no-rebase-merges is > given, but setting it to true here makes the result undefined, > unless the built-in default between cousin choices is described > here. > > "Setting to true is equivalent to setting to no-rebase-cousins" and > "Setting to false is a no-op but accepted only for completeness", > perhaps? A false in the local configuration overrides a true in the global configuration, so I wouldn't call "false" a no-op. But it would be fine to say that "true" is equivalent to no-rebase-cousins (and then change the documentation for "true" if and when the default changes in the future). > >> +`no-rebase-cousins`. If the mode is not specified on the command line or in > >> +the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`. > > This side could describe what setting it to "true" means, but it is > a separate page so it would be more friendly to readers to cover it > on both pages. The longer the documentation is, the less likely people are to read any of it. I don't really want to repeat the detailed explanation that is in git-config.txt, but I see that other places in git-rebase.txt link to git-config.txt, and having a link seems like a good idea. Fair enough? -Alex
On Tue, Mar 7, 2023 at 5:02 PM Glen Choo <chooglen@google.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > +test_expect_success '--rebase-merges="" is deprecated' ' > > + git rebase --rebase-merges="" HEAD^ 2>actual && > > + grep deprecated actual > > +' > > I believe this used to be on 2/3, i.e. > > https://lore.kernel.org/git/20230225180325.796624-3-alexhenrie24@gmail.com/ > > but your cover letter suggests that it was removed. Mechanical error? I removed it from the second patch but accidentally added it back in the third patch. Thanks for catching that. -Alex
On Tue, Mar 7, 2023 at 5:02 PM Glen Choo <chooglen@google.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > In the future, the default rebase-merges mode may change from > > no-rebase-cousins to rebase-cousins. > > I suspect a more likely future would be that the default is to rebase > 'evil' merges instead of trying to recreate merge commits, but of > course, the important thing is that we promote the default, not what the > default will be... Glen, do you have any more thoughts? At this point, the only thing that's keeping me from implementing Phillip's request to make --rebase-merges without an argument clobber rebase.rebaseMerges is your suspicion that we might need to change the default rebase-merges mode in the future, and I assume that we would want to use the rebase.rebaseMerges config option to facilitate the transition. -Alex
Alex Henrie <alexhenrie24@gmail.com> writes: >> > In the future, the default rebase-merges mode may change from >> > no-rebase-cousins to rebase-cousins. >> >> I suspect a more likely future would be that the default is to rebase >> 'evil' merges instead of trying to recreate merge commits, but of >> course, the important thing is that we promote the default, not what the >> default will be... > > Glen, do you have any more thoughts? At this point, the only thing > that's keeping me from implementing Phillip's request to make > --rebase-merges without an argument clobber rebase.rebaseMerges is > your suspicion that we might need to change the default rebase-merges > mode in the future, and I assume that we would want to use the > rebase.rebaseMerges config option to facilitate the transition. (Sorry for the late reply) Ah, I don't really have more thoughts on the matter. I am fairly confident that we would _like_ to change the default to rebase 'evil' merges, but I don't know how likely that will be. Perhaps it would help to enumerate the rules to see if it is too confusing or not? The behaviors we can tweak are: - Whether to rebase merges or not (true, false, specified mode, or default) - What mode to use when rebasing merges (specified mode or default) And the sources are either CLI or config, with CLI always overriding config. Should we rebase a merge? - If neither CLI or config tells us whether or not to rebase a merge, default to "don't rebase merges". - If one of CLI or config tells us whether or not to rebase a merge, respect it. - If both CLI or config tell us whether or not to rebase a merge, respect CLI and ignore config. What mode should we use? - If neither CLI or config tells us what mode to use, default to "no-rebase-cousins" (or whatever default we decide). - If one of CLI or config tells us what mode to use, respect it. - If both CLI or config tell us what mode to use, respect CLI and ignore config. If users cleanly separate the two concepts, I think it is quite clear. (I'm not advocating for this approach, but) e.g. if we pretend that each behavior were configured separately, like: --[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)] I don't think there would be any confusion. (Having --rebase-merges-mode be a no-op without --rebase-merges is probably even more confusing to users, plus this would break backwards compatibility, so I don't think this is a good idea at all.) Your doc patch explains the rules pretty clearly, but perhaps it doesn't explain this mental model clearly enough, hence the confusion. If we don't find a good way to communicate this (I think it is clear, but other reviewers seem yet unconvinced), I wouldn't mind taking Phillip's suggestion to have "--rebase-merges" override "rebase.rebaseMerges='specific-mode'". > > -Alex
On Thu, Mar 16, 2023 at 11:57 AM Glen Choo <chooglen@google.com> wrote: > If users cleanly separate the two concepts, I think it is quite clear. > (I'm not advocating for this approach, but) e.g. if we pretend that each > behavior were configured separately, like: > > --[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)] > > I don't think there would be any confusion. Not being conversant with these options I agree the above isn't confusing. > (Having --rebase-merges-mode > be a no-op without --rebase-merges is probably even more confusing to > users, plus this would break backwards compatibility, so I don't think > this is a good idea at all.) I don't find it confusing. And how would it break backwards compatibility if --rebase-merges-mode doesn't exist now?
On Thu, Mar 16, 2023 at 11:32 AM Glen Choo <chooglen@google.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > >> > In the future, the default rebase-merges mode may change from > >> > no-rebase-cousins to rebase-cousins. > >> > >> I suspect a more likely future would be that the default is to rebase > >> 'evil' merges instead of trying to recreate merge commits, but of > >> course, the important thing is that we promote the default, not what the > >> default will be... > > > > Glen, do you have any more thoughts? At this point, the only thing > > that's keeping me from implementing Phillip's request to make > > --rebase-merges without an argument clobber rebase.rebaseMerges is > > your suspicion that we might need to change the default rebase-merges > > mode in the future, and I assume that we would want to use the > > rebase.rebaseMerges config option to facilitate the transition. > > (Sorry for the late reply) > > Ah, I don't really have more thoughts on the matter. I am fairly > confident that we would _like_ to change the default to rebase 'evil' > merges, but I don't know how likely that will be. > > Perhaps it would help to enumerate the rules to see if it is too > confusing or not? > > The behaviors we can tweak are: > > - Whether to rebase merges or not (true, false, specified mode, or > default) > - What mode to use when rebasing merges (specified mode or default) > > And the sources are either CLI or config, with CLI always overriding > config. > > Should we rebase a merge? > > - If neither CLI or config tells us whether or not to rebase a merge, > default to "don't rebase merges". > - If one of CLI or config tells us whether or not to rebase a merge, > respect it. > - If both CLI or config tell us whether or not to rebase a merge, > respect CLI and ignore config. > > What mode should we use? > > - If neither CLI or config tells us what mode to use, default to > "no-rebase-cousins" (or whatever default we decide). > - If one of CLI or config tells us what mode to use, respect it. > - If both CLI or config tell us what mode to use, respect CLI and ignore > config. > > If users cleanly separate the two concepts, I think it is quite clear. > (I'm not advocating for this approach, but) e.g. if we pretend that each > behavior were configured separately, like: > > --[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)] > > I don't think there would be any confusion. (Having --rebase-merges-mode > be a no-op without --rebase-merges is probably even more confusing to > users, plus this would break backwards compatibility, so I don't think > this is a good idea at all.) > > Your doc patch explains the rules pretty clearly, but perhaps it doesn't > explain this mental model clearly enough, hence the confusion. If we > don't find a good way to communicate this (I think it is clear, but > other reviewers seem yet unconvinced), I wouldn't mind taking Phillip's > suggestion to have "--rebase-merges" override > "rebase.rebaseMerges='specific-mode'". I got the impression that everyone, including Phillip,[1] already agrees that the proposed documentation is clear about the interaction between the config option and the command line option. However, it is a little weird when you consider that other flags with optional arguments, like `git branch --track`, unconditionally override their corresponding config options.[2] Let me ask a different but related question: If we add a rebase-evil-merges mode, do you think that would be orthogonal to the rebase-cousins mode? -Alex [1] https://lore.kernel.org/git/7cf19017-518b-245e-aea2-5dee55f88276@dunelm.org.uk/ [2] https://lore.kernel.org/git/5551d67b-3021-8cfc-53b5-318f223ded6d@dunelm.org.uk/
Alex Henrie <alexhenrie24@gmail.com> writes: >> Your doc patch explains the rules pretty clearly, but perhaps it doesn't >> explain this mental model clearly enough, hence the confusion. If we >> don't find a good way to communicate this (I think it is clear, but >> other reviewers seem yet unconvinced), I wouldn't mind taking Phillip's >> suggestion to have "--rebase-merges" override >> "rebase.rebaseMerges='specific-mode'". > > I got the impression that everyone, including Phillip,[1] already > agrees that the proposed documentation is clear about the interaction > between the config option and the command line option. However, it is > a little weird when you consider that other flags with optional > arguments, like `git branch --track`, unconditionally override their > corresponding config options.[2] Ah, I didn't consider other options like `git branch --track`. I haven't looked into what is the norm, but I think we should follow it (whatever it is). If other reviewers have a strong idea of what this norm is, I am happy to defer to them. If not, I can look into it given some time. > Let me ask a different but related question: If we add a > rebase-evil-merges mode, do you think that would be orthogonal to the > rebase-cousins mode? I am not an expert on this, so perhaps my opinion isn't that important ;) My understanding is that `[no-]rebase-cousins` affects which commits get rebased and onto where, whereas `rebase-evil-merges` would affect how the merge commits are generated (by rebasing the evil or by simply recreating the merges). From that perspective, it seems like yes, the two would be orthogonal. Hm. Maybe this means that we'd be introducing a new option, and that my hunch that we would change the default to `rebase-evil-merges` is more wrong than I expected. Though I guess this doesn't really affects what we do with the CLI options _now_, since the discussion is about what we do about defaults, and what the default is is quite immaterial. > > -Alex > > [1] https://lore.kernel.org/git/7cf19017-518b-245e-aea2-5dee55f88276@dunelm.org.uk/ > [2] https://lore.kernel.org/git/5551d67b-3021-8cfc-53b5-318f223ded6d@dunelm.org.uk/
Felipe Contreras <felipe.contreras@gmail.com> writes: >> --[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)] >> >> I don't think there would be any confusion. >> [...] >> (Having --rebase-merges-mode >> be a no-op without --rebase-merges is probably even more confusing to >> users, plus this would break backwards compatibility, so I don't think >> this is a good idea at all.) > > I don't find it confusing. And how would it break backwards > compatibility if --rebase-merges-mode doesn't exist now? I meant that for the above example to work, we would need to have `--[no-]rebase-merges` as a boolean that says whether we rebase merges at all, and `--rebase-merges-mode=[whatever]` would tell us what mode to use _if_ we were rebasing merges. This means that `--rebase-merges=not-a-boolean` would become invalid. We were in this position before, actually, with `git log -p -m`. The conclusion on a recent series [1] is that having such a no-op flag was probably a mistake (and unfortunately, one that is extremely hard to fix due to backwards compatibility requirements), so introducing more no-op flags is probably a bad idea :) [1] https://lore.kernel.org/git/871qn5pyez.fsf@osv.gnss.ru
On Thu, Mar 16, 2023 at 4:46 PM Glen Choo <chooglen@google.com> wrote: > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > >> --[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)] > >> > >> I don't think there would be any confusion. > >> [...] > >> (Having --rebase-merges-mode > >> be a no-op without --rebase-merges is probably even more confusing to > >> users, plus this would break backwards compatibility, so I don't think > >> this is a good idea at all.) > > > > I don't find it confusing. And how would it break backwards > > compatibility if --rebase-merges-mode doesn't exist now? > > I meant that for the above example to work, we would need to have > `--[no-]rebase-merges` as a boolean that says whether we rebase merges at > all, and `--rebase-merges-mode=[whatever]` would tell us what mode to > use _if_ we were rebasing merges. No, we don't need --no-rebase-merges for --rebase-merges-mode to work. It would be nice, but not necessary. > This means that > `--rebase-merges=not-a-boolean` would become invalid. No, it would not become invalid, that's yet another decision to make. `--rebase-merges=mode` could become synonymous with `--rebase-merges --rebase-merges-mode=mode`. A shortcut. Because it's redundant it could be deprecated in the future, but not necessarily invalid. > We were in this position before, actually, with `git log -p -m`. The > conclusion on a recent series [1] is that having such a no-op flag was > probably a mistake (and unfortunately, one that is extremely hard to fix > due to backwards compatibility requirements), No, `git log -m` was a mistake *only* if -p isn't turned on as well, which is an interface wart that could be fixed today. > so introducing more no-op flags is probably a bad idea :) Whether --rebase-merges-mode turns on --rebase-merges by default is a decision that could be made in the future. Just like `git log -m` turning on `-p` by default is a decision that could be made in the future (and probably should). --- Either way: introducing a new option cannot possibly break backwards compatibility. Cheers.
On Thu, Mar 16, 2023 at 4:39 PM Glen Choo <chooglen@google.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > >> Your doc patch explains the rules pretty clearly, but perhaps it doesn't > >> explain this mental model clearly enough, hence the confusion. If we > >> don't find a good way to communicate this (I think it is clear, but > >> other reviewers seem yet unconvinced), I wouldn't mind taking Phillip's > >> suggestion to have "--rebase-merges" override > >> "rebase.rebaseMerges='specific-mode'". > > > > I got the impression that everyone, including Phillip,[1] already > > agrees that the proposed documentation is clear about the interaction > > between the config option and the command line option. However, it is > > a little weird when you consider that other flags with optional > > arguments, like `git branch --track`, unconditionally override their > > corresponding config options.[2] > > Ah, I didn't consider other options like `git branch --track`. I haven't > looked into what is the norm, but I think we should follow it (whatever > it is). > > If other reviewers have a strong idea of what this norm is, I am happy > to defer to them. If not, I can look into it given some time. > > > Let me ask a different but related question: If we add a > > rebase-evil-merges mode, do you think that would be orthogonal to the > > rebase-cousins mode? > > I am not an expert on this, so perhaps my opinion isn't that important > ;) > > My understanding is that `[no-]rebase-cousins` affects which commits get > rebased and onto where, whereas `rebase-evil-merges` would affect how > the merge commits are generated (by rebasing the evil or by simply > recreating the merges). From that perspective, it seems like yes, the > two would be orthogonal. > > Hm. Maybe this means that we'd be introducing a new option, and that my > hunch that we would change the default to `rebase-evil-merges` is more > wrong than I expected. > > Though I guess this doesn't really affects what we do with the CLI > options _now_, since the discussion is about what we do about defaults, > and what the default is is quite immaterial. I looked through the code and documentation and found 12 good examples of command line flags with an optional argument that always override their corresponding config options whether or not the optional argument is given: git -c branch.autoSetupMerge=inherit branch --track mynewbranch git -c commit.gpgSign=mykey commit --gpg-sign git -c core.sharedRepository=0666 init --shared . git -c diff.ignoreSubmodules=untracked diff --ignore-submodules git -c diff.submodule=diff diff --submodule git -c format.attach=myboundary format-patch --attach HEAD^ git -c format.from='Dang Git <dang@example.org>' format-patch --from HEAD^ git -c format.thread=deep format-patch --thread HEAD~3 git -c gc.pruneExpire=1.week.ago gc --prune git -c log.decorate=full log --decorate git -c merge.log=1 merge --log mytopicbranch git -c pull.rebase=interactive pull --rebase I found 6 other similar examples where the config option is a yes/no/auto trilean, but I don't think those are good examples because it makes total sense for a command line flag without an argument to override a nonspecific "auto" value in the configuration: git -c color.diff=auto diff --color | less git -c color.grep=auto grep --color . | less git -c color.showbranch=auto show-branch --color master mytopicbranch | less git -c color.ui=auto log --color | less git -c fetch.recurseSubmodules=on-demand fetch --recurse-submodules git -c push.gpgSign=if-asked push --signed I found 1 good example where the config option does make a difference if the optional argument is omitted: git -c diff.colorMoved=plain diff --color-moved And I found 1 other similar example, but it's not good a example because the config option doesn't do anything unless the command line flag is specified: git -c diff.dirstat=lines diff --dirstat Given 12 good examples versus 1 good counterexample, I would say that the established convention is for the command line flag to always override the config option. Please let me know if there are other examples that I missed. It's worth noting that the documentation for `git format-patch --thread` explicitly says that format.thread takes precedence over --thread without an argument, but that is not correct: When I tested it, the command line argument always took precedence. Another problem is that the documentation for `git pack-objects` does not even mention that --unpack-unreachable has an optional argument, and it says that the argument to --cruft-expiration is required when it is not. I'm not going to fix all the documentation problems, at least not right now. However, in order to follow the established convention for flags with optional arguments, and because we probably aren't going to use rebase.rebaseMerges to facilitate a future change of defaults, I will redo the patch so that --rebase-merges without an argument takes precedence over rebase.rebaseMerges. Thanks for the feedback. I feel more confident about the way forward now. -Alex
Hi Alex, On Thu, 16 Mar 2023, Alex Henrie wrote: > If we add a rebase-evil-merges mode, do you think that would be > orthogonal to the rebase-cousins mode? Those two concepts are very much orthogonal. The rebase-evil-merges mode needs to change the way the `merge` command operates, from "forget everything but the commit message of the original merge commit" to "do try to figure out what amendments were made to the original merge and replay them". Whether cousins are rebased or not has everything to do with the `reset` command, though. Elsewhere in the thread, you mentioned a suspicion that `rebase-cousins` should become the default. However, that would be a radical shift from the spirit of the `git rebase [-r] <onto>` command: If `<onto>` is reachable from `HEAD`, the idea is that an unchanged rebase script will produce the identical commit topology. This is in line with a regular (non-`-r`) rebase as long as there are no merge commits between `<onto>` and `HEAD`. I am not sure that such a radical shift could ever be an easy sell. Ciao, Johannes
Alex Henrie <alexhenrie24@gmail.com> writes: [...] > Let me ask a different but related question: If we add a > rebase-evil-merges mode, do you think that would be orthogonal to the > rebase-cousins mode? This is the question that I considered as well. To me they look orthogonal, and I'm afraid we will have hard times extending --rebase-merges gracefully in a backward-compatible manner. The set: --rebase-merges=remerge --rebase-merges=rebase --rebase-merges=off looks natural to me, but "cousins" that affect what is to be rebased (as opposed to how, if at all), will apparently get in the way. Besides, I recently ran into similar problem with --diff-merges (when|how) dichotomy, and there was no elegant solution found, even though a working one has been implemented, and then there were some discussions around. Also, I'm not quite sure that these "cousins" options make no sense when we in fact flatten history (--no-rebase-merges case). To me it looks like there should have been separate --[no-]rebase-cousins option, provided we do need this choice in the first place, but I definitely might be wrong here as well. As a side note, it sounds both unfair and confusing to use "rebase-evil-merges" term to describe a mode that will actually rebase (all the) merges, as neither a merge needs to be 'evil' to be the object of rebase operation, nor dedicated mode is necessarily needed to rebase specifically 'evil' merges (whatever they actually are.) Thanks, -- Sergey Organov
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt index f19bd0e040..f7d3218b1d 100644 --- a/Documentation/config/rebase.txt +++ b/Documentation/config/rebase.txt @@ -67,3 +67,14 @@ rebase.rescheduleFailedExec:: rebase.forkPoint:: If set to false set `--no-fork-point` option by default. + +rebase.rebaseMerges:: + 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.rebaseMerges=false` + configuration, but the absence of a specific rebase-merges mode on the + command line does not counteract a specific mode set in the configuration. diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 4e57a87624..6ec86c9c6e 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -537,16 +537,17 @@ 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.rebaseMerges` config option and a previous + `--rebase-merges`. + When rebasing merges, there are two modes: `rebase-cousins` and -`no-rebase-cousins`. If the mode is not specified, it defaults to -`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have -`<upstream>` as direct ancestor will keep their original branch point, i.e. -commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path` -option will keep their original ancestry by default. In `rebase-cousins` mode, -such commits are instead rebased onto `<upstream>` (or `<onto>`, if -specified). +`no-rebase-cousins`. If the mode is not specified on the command line or in +the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`. +In `no-rebase-cousins` mode, commits which do not have `<upstream>` as direct +ancestor will keep their original branch point, i.e. commits that would be +excluded by linkgit:git-log[1]'s `--ancestry-path` option will keep their +original ancestry by default. In `rebase-cousins` mode, such commits are +instead rebased onto `<upstream>` (or `<onto>`, if specified). + It is currently only possible to recreate the merge commits using the `ort` merge strategy; different merge strategies can be used only via diff --git a/builtin/rebase.c b/builtin/rebase.c index c36ddc0050..04f815e3d0 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.rebasemerges")) { + 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 without an argument " + "instead, which does the same thing.")); + return 0; + } + 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 without an argument " - "instead, which does the same thing.")); - 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.rebaseMerges. 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..2eba00bdf5 100755 --- a/t/t3422-rebase-incompatible-options.sh +++ b/t/t3422-rebase-incompatible-options.sh @@ -85,6 +85,11 @@ test_rebase_am_only () { test_must_fail git rebase $opt --reapply-cherry-picks A " + test_expect_success "$opt incompatible with --rebase-merges" " + git checkout B^0 && + test_must_fail git rebase $opt --rebase-merges A + " + test_expect_success "$opt incompatible with --update-refs" " git checkout B^0 && test_must_fail git rebase $opt --update-refs A @@ -101,6 +106,12 @@ test_rebase_am_only () { grep -e --no-autosquash err " + test_expect_success "$opt incompatible with rebase.rebaseMerges" " + git checkout B^0 && + test_must_fail git -c rebase.rebaseMerges=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 +124,12 @@ test_rebase_am_only () { git -c rebase.autosquash=true rebase --no-autosquash $opt A " + test_expect_success "$opt okay with overridden rebase.rebaseMerges" " + test_when_finished \"git reset --hard B^0\" && + git checkout B^0 && + git -c rebase.rebaseMerges=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 d46d9545f2..aa75e192d1 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -278,6 +278,74 @@ test_expect_success 'do not rebase cousins unless asked for' ' EOF ' +test_expect_success '--rebase-merges="" is deprecated' ' + git rebase --rebase-merges="" HEAD^ 2>actual && + grep deprecated actual +' + +test_expect_success 'rebase.rebaseMerges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' ' + test_config rebase.rebaseMerges 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.rebaseMerges=no-rebase-cousins' ' + test_config rebase.rebaseMerges 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.rebaseMerges=rebase-cousins' ' + test_config rebase.rebaseMerges 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.rebaseMerges=false' ' + test_config rebase.rebaseMerges 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.rebaseMerges=rebase-cousins' ' + test_config rebase.rebaseMerges 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 turning on --rebase-merges by default without configuration in a future version of Git. Name the new option rebase.rebaseMerges, even though it is a little redundant, for consistency with the name of the command line option and to be clear when scrolling through values in the [rebase] section of .gitconfig. In the future, the default rebase-merges mode may change from no-rebase-cousins to rebase-cousins. Support setting rebase.rebaseMerges to the nonspecific value "true" for users who do not need or want to care about the default changing in the future. Similarly, for users who have --rebase-merges in an alias and want to get the future behavior now, use the specific rebase-merges mode from the config if a specific mode is not given on the command line. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- Documentation/config/rebase.txt | 11 ++++ Documentation/git-rebase.txt | 17 +++--- builtin/rebase.c | 79 ++++++++++++++++++-------- t/t3422-rebase-incompatible-options.sh | 17 ++++++ t/t3430-rebase-merges.sh | 68 ++++++++++++++++++++++ 5 files changed, 161 insertions(+), 31 deletions(-)