| Message ID | 20231105000808.10171-1-andy.koppe@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] rebase: support non-interactive autosquash | expand |
Hi Andy On 05/11/2023 00:08, Andy Koppe wrote: > So far, the rebase --autosquash option and rebase.autoSquash=true > config setting are quietly ignored when used without --interactive, Thanks for working on this, I agree that "--autosquash" being ignored without "--interactive" is something that we should address. I think there are several possible solutions 1 - make "--autosquash" imply "--interactive". This has the advantage that the user gets to check the commits are going to be reordered as they expect when they edit the todo list. It is hard to see how to accommodate the config setting though - I don't think we want "rebase.autosquash=true" to imply "--interactive". 2 - make "--autosquash" without "--interactive" an error. This would prevent the user being surprised that their commits are not squashed by a non-interactive rebase. Users who have set "rebase.autosquash=true" would have to pass "--no-autosquash" to perform any form of non-interactive rebase. This is similar to the current behavior where the user has to pass "--no-autosquash" if they want to use the apply backend with "rebase.autosquash=true". 3 - make "--autosquash" rearrange and squash commits without "--interactive". This is convenient but there is a risk in that the user does not get a chance to check the todo list before the commits are reordered and squashed. I think that risk is fairly small with an explicit "--autosquash" on the commandline. This is the approach taken by this patch. I do have some concerns about extending the config setting to non-interactive rebases though. If the user has commits that look like fixup! foo (HEAD) foo bar foo and runs "git -c rebase.autosquash=non-interactive rebase HEAD~2" then we'll silently squash the fixup into the wrong commit due to a prefix subject match. > except that they prevent fast-forward and that they trigger conflicts > with --apply and relatives, which is less than helpful particularly for > the config setting. The behavior to make the config setting incompatible with the apply backend was implemented to avoid users being surprised that their commits are not squashed by that backend even when they have set "rebase.autosquash=true"[1]. I think one could consider "--autosquash" being silently ignored without "--interactive" to be an oversight in 796abac7e1 (rebase: add coverage of other incompatible options, 2023-01-25) that introduced that change. [1] https://lore.kernel.org/git/pull.1466.v5.git.1674619434.gitgitgadget@gmail.com/ > Since the "merge" backend used for interactive rebase also is the > default for non-interactive rebase, there doesn't appear to be a > reason not to do --autosquash without --interactive, so support that. I think making "--autosquash" on the commandline work for non-interactive rebases is reasonable but I would be open to the argument that it would be better to make it an error and require "--interactive" to allow the user to check that the commits are going to be reordered as they expect. > Turn rebase.autoSquash into a comma-separated list of flags, with > "interactive" or "i" enabling auto-squashing with --interactive, and > "no-interactive" or "no-i" enabling it without. Make boolean true mean > "interactive" for backward compatibility. Please, please, please don't introduce abbreviated config settings, it just makes the interface more complicated. The user only has to set this once so I think the short names just add confusion. I also think "non-interactive" would be a better name for the config setting corresponding to non-interactive rebases. Does this mean the user can request that commits are only rearranged when the do not pass "--interactive"? As I said above I do have some concerns that the "rebase.autosquash=non-interactive" setting will catch people out. Having said that ignoring "rebase.autosquash=true" without "--interactive" as we do now is inconsistent with the behavior of "rebase.autosquash=true" with "--apply". One possibility would be to introduce "rebase.autosquash=interactive" which would not cause an error with "--apply" and always require an explicit "--autosquash" on the commandline to squash fixups without "--interactive" > Don't prevent fast-forwards or report conflicts with --apply options > when auto-squashing is not active. I think this change deserves to be in a separate commit (which probably means separating out the config changes into that commit) as it is not directly related to fixing "--autosquash" without "--interactive" on the commandline. It would revert 796abac7e1 (rebase: add coverage of other incompatible options, 2023-01-25) and part of eddfcd8ece (rebase: provide better error message for apply options vs. merge config, 2023-01-25) and so the commit message would need to explain why that is desirable (and why it is desirable to revert only the changes related to "rebase.autosquash" and not those related to "rebase.updateRefs") > Change the git-rebase and config/rebase documentation accordingly, and > extend t3415-rebase-autosquash.sh to test the new rebase.autosquash > values and combinations with and without --interactive. Thanks for adding tests for the new behavior. In summary I like "--autosquash" working without "--interactive" but I'm unsure about the config changes. Best Wishes Phillip > Signed-off-by: Andy Koppe <andy.koppe@gmail.com> > --- > Documentation/config/rebase.txt | 11 +++- > Documentation/git-rebase.txt | 2 +- > builtin/rebase.c | 66 +++++++++++++++----- > t/t3415-rebase-autosquash.sh | 83 +++++++++++++++++++++----- > t/t3422-rebase-incompatible-options.sh | 2 +- > 5 files changed, 132 insertions(+), 32 deletions(-) > > diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt > index 9c248accec..68191e5673 100644 > --- a/Documentation/config/rebase.txt > +++ b/Documentation/config/rebase.txt > @@ -9,7 +9,16 @@ rebase.stat:: > rebase. False by default. > > rebase.autoSquash:: > - If set to true enable `--autosquash` option by default. > + A comma-separated list of flags for when to enable auto-squashing. > + Specifying `interactive` or `i` enables auto-squashing for rebasing with > + `--interactive`, whereas `no-interactive` or `no-i` enables it for > + rebasing without that option. For example, setting this to `i,no-i` > + enables auto-squashing for both types. Setting it to true is equivalent > + to setting it to `interactive`. > + > + The `--autosquash` and `--no-autosquash` options of > + linkgit:git-rebase[1] override the setting here. > + Auto-squashing is disabled by default. > > rebase.autoStash:: > When set to true, automatically create a temporary stash entry > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index e7b39ad244..102ff91493 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -592,7 +592,7 @@ See also INCOMPATIBLE OPTIONS below. > When the commit log message begins with "squash! ..." or "fixup! ..." > or "amend! ...", and there is already a commit in the todo list that > matches the same `...`, automatically modify the todo list of > - `rebase -i`, so that the commit marked for squashing comes right after > + `rebase`, so that the commit marked for squashing comes right after > the commit to be modified, and change the action of the moved commit > from `pick` to `squash` or `fixup` or `fixup -C` respectively. A commit > matches the `...` if the commit subject matches, or if the `...` refers > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 261a9a61fc..26c3e5dcb4 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -131,7 +131,10 @@ struct rebase_options { > int reapply_cherry_picks; > int fork_point; > int update_refs; > - int config_autosquash; > + enum { > + AUTOSQUASH_INTERACTIVE = 1 << 0, > + AUTOSQUASH_NO_INTERACTIVE = 1 << 1, > + } config_autosquash; > int config_rebase_merges; > int config_update_refs; > }; > @@ -149,7 +152,6 @@ struct rebase_options { > .reapply_cherry_picks = -1, \ > .allow_empty_message = 1, \ > .autosquash = -1, \ > - .config_autosquash = -1, \ > .rebase_merges = -1, \ > .config_rebase_merges = -1, \ > .update_refs = -1, \ > @@ -711,10 +713,8 @@ static int run_specific_rebase(struct rebase_options *opts) > if (opts->type == REBASE_MERGE) { > /* Run sequencer-based rebase */ > setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1); > - if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) { > + if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) > setenv("GIT_SEQUENCE_EDITOR", ":", 1); > - opts->autosquash = 0; > - } > if (opts->gpg_sign_opt) { > /* remove the leading "-S" */ > char *tmp = xstrdup(opts->gpg_sign_opt + 2); > @@ -748,6 +748,30 @@ static int run_specific_rebase(struct rebase_options *opts) > return status ? -1 : 0; > } > > +static void parse_rebase_autosquash_value(struct rebase_options *opts, > + const char *var, const char *value) > +{ > + struct string_list tokens = STRING_LIST_INIT_NODUP; > + char *buf = xstrdup(value); > + > + opts->config_autosquash = 0; > + string_list_split_in_place(&tokens, buf, ",", -1); > + > + for (int i = 0; i < tokens.nr; i++) { > + const char *s = tokens.items[i].string; > + > + if (!strcmp(s, "i") || !strcmp(s, "interactive")) > + opts->config_autosquash |= AUTOSQUASH_INTERACTIVE; > + else if (!strcmp(s, "no-i") || !strcmp(s, "no-interactive")) > + opts->config_autosquash |= AUTOSQUASH_NO_INTERACTIVE; > + else > + die(_("invalid value for '%s': '%s'"), var, s); > + } > + > + string_list_clear(&tokens, 0); > + free(buf); > +} > + > static void parse_rebase_merges_value(struct rebase_options *options, const char *value) > { > if (!strcmp("no-rebase-cousins", value)) > @@ -772,8 +796,14 @@ static int rebase_config(const char *var, const char *value, > } > > if (!strcmp(var, "rebase.autosquash")) { > - opts->config_autosquash = git_config_bool(var, value); > - return 0; > + int b = git_parse_maybe_bool(value); > + > + if (b < 0) > + parse_rebase_autosquash_value(opts, var, value); > + else if (b) > + opts->config_autosquash = AUTOSQUASH_INTERACTIVE; > + else > + opts->config_autosquash = 0; > } > > if (!strcmp(var, "commit.gpgsign")) { > @@ -1402,13 +1432,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > state_dir_base, cmd_live_rebase, buf.buf); > } > > + if (options.autosquash == 1) { > + imply_merge(&options, "--autosquash"); > + } else if (options.autosquash == -1) { > + int conf = options.config_autosquash; > + options.autosquash = > + (options.flags & REBASE_INTERACTIVE_EXPLICIT) > + ? !!(conf & AUTOSQUASH_INTERACTIVE) > + : !!(conf & AUTOSQUASH_NO_INTERACTIVE); > + } > + > if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) || > (options.action != ACTION_NONE) || > (options.exec.nr > 0) || > - (options.autosquash == -1 && options.config_autosquash == 1) || > - options.autosquash == 1) { > + options.autosquash) { > allow_preemptive_ff = 0; > } > + > if (options.committer_date_is_author_date || options.ignore_date) > options.flags |= REBASE_FORCE; > > @@ -1508,7 +1548,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > if (is_merge(&options)) > die(_("apply options and merge options " > "cannot be used together")); > - else if (options.autosquash == -1 && options.config_autosquash == 1) > + else if (options.autosquash && options.config_autosquash) > 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")); > @@ -1529,11 +1569,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > 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 : > - ((options.config_autosquash >= 0) ? options.config_autosquash : 0); > - > if (options.type == REBASE_UNSPECIFIED) { > if (!strcmp(options.default_backend, "merge")) > options.type = REBASE_MERGE; > @@ -1858,3 +1893,4 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > free(keep_base_onto_name); > return !!ret; > } > + > diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh > index a364530d76..1386eb6394 100755 > --- a/t/t3415-rebase-autosquash.sh > +++ b/t/t3415-rebase-autosquash.sh > @@ -43,7 +43,7 @@ test_auto_fixup () { > > git tag $1 && > test_tick && > - git rebase $2 -i HEAD^^^ && > + git rebase $2 HEAD^^^ && > git log --oneline >actual && > if test -n "$no_squash" > then > @@ -61,15 +61,43 @@ test_auto_fixup () { > } > > test_expect_success 'auto fixup (option)' ' > - test_auto_fixup final-fixup-option --autosquash > + test_auto_fixup fixup-option --autosquash && > + test_auto_fixup fixup-option-i "-i --autosquash" > ' > > -test_expect_success 'auto fixup (config)' ' > - git config rebase.autosquash true && > - test_auto_fixup final-fixup-config-true && > - test_auto_fixup ! fixup-config-true-no --no-autosquash && > +test_expect_success 'auto fixup (config false)' ' > git config rebase.autosquash false && > - test_auto_fixup ! final-fixup-config-false > + test_auto_fixup ! fixup-config-false && > + test_auto_fixup ! fixup-config-false-i -i > +' > + > +test_expect_success 'auto fixup (config true)' ' > + git config rebase.autosquash true && > + test_auto_fixup ! fixup-config-true && > + test_auto_fixup fixup-config-true-i -i && > + test_auto_fixup ! fixup-config-true-i-no "-i --no-autosquash" > +' > + > +test_expect_success 'auto fixup (config interactive)' ' > + git config rebase.autosquash interactive && > + test_auto_fixup ! fixup-config-interactive && > + test_auto_fixup fixup-config-interactive-i -i && > + test_auto_fixup ! fixup-config-interactive-i-no "-i --no-autosquash" > +' > + > +test_expect_success 'auto fixup (config no-interactive)' ' > + git config rebase.autosquash no-interactive && > + test_auto_fixup fixup-config-no-interactive && > + test_auto_fixup ! fixup-config-no-interactive-i -i && > + test_auto_fixup ! fixup-config-no-interactive-no "--no-autosquash" > +' > + > +test_expect_success 'auto fixup (config always)' ' > + git config rebase.autosquash i,no-i && > + test_auto_fixup fixup-config-always && > + test_auto_fixup fixup-config-always-i -i && > + test_auto_fixup ! fixup-config-always-no --no-autosquash && > + test_auto_fixup ! fixup-config-always-i-no "-i --no-autosquash" > ' > > test_auto_squash () { > @@ -87,7 +115,7 @@ test_auto_squash () { > git commit -m "squash! first" -m "extra para for first" && > git tag $1 && > test_tick && > - git rebase $2 -i HEAD^^^ && > + git rebase $2 HEAD^^^ && > git log --oneline >actual && > if test -n "$no_squash" > then > @@ -105,15 +133,42 @@ test_auto_squash () { > } > > test_expect_success 'auto squash (option)' ' > - test_auto_squash final-squash --autosquash > + test_auto_squash squash-option --autosquash && > + test_auto_squash squash-option-i "-i --autosquash" > ' > > -test_expect_success 'auto squash (config)' ' > - git config rebase.autosquash true && > - test_auto_squash final-squash-config-true && > - test_auto_squash ! squash-config-true-no --no-autosquash && > +test_expect_success 'auto squash (config false)' ' > git config rebase.autosquash false && > - test_auto_squash ! final-squash-config-false > + test_auto_squash ! squash-config-false && > + test_auto_squash ! squash-config-false-i -i > +' > + > +test_expect_success 'auto squash (config true)' ' > + git config rebase.autosquash true && > + test_auto_squash ! squash-config-true && > + test_auto_squash squash-config-true-i -i && > + test_auto_squash ! squash-config-true-i-no "-i --no-autosquash" > +' > + > +test_expect_success 'auto squash (config interactive)' ' > + git config rebase.autosquash i && > + test_auto_squash ! squash-config-interactive && > + test_auto_squash squash-config-interactive-i -i && > + test_auto_squash ! squash-config-interactive-i-no "-i --no-autosquash" > +' > + > +test_expect_success 'auto squash (config no-interactive)' ' > + git config rebase.autosquash no-i && > + test_auto_squash squash-config-no-interactive && > + test_auto_squash ! squash-config-no-interactive-i -i && > + test_auto_squash ! squash-config-no-interactive-no "--no-autosquash" > +' > +test_expect_success 'auto squash (config always)' ' > + git config rebase.autosquash interactive,no-interactive && > + test_auto_squash squash-config-always && > + test_auto_squash squash-config-always-i -i && > + test_auto_squash ! squash-config-always-no --no-autosquash && > + test_auto_squash ! squash-config-always-i-no "-i --no-autosquash" > ' > > test_expect_success 'misspelled auto squash' ' > diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh > index 2eba00bdf5..e5119e7371 100755 > --- a/t/t3422-rebase-incompatible-options.sh > +++ b/t/t3422-rebase-incompatible-options.sh > @@ -102,7 +102,7 @@ test_rebase_am_only () { > > test_expect_success "$opt incompatible with rebase.autosquash" " > git checkout B^0 && > - test_must_fail git -c rebase.autosquash=true rebase $opt A 2>err && > + test_must_fail git -c rebase.autosquash=no-i rebase $opt A 2>err && > grep -e --no-autosquash err > " >
On 06/11/2023 11:06, Phillip Wood wrote: > On 05/11/2023 00:08, Andy Koppe wrote: >> So far, the rebase --autosquash option and rebase.autoSquash=true >> config setting are quietly ignored when used without --interactive, > > Thanks for working on this, I agree that "--autosquash" being ignored > without "--interactive" is something that we should address. I think > there are several possible solutions > > 1 - make "--autosquash" imply "--interactive". This has the advantage > that the user gets to check the commits are going to be reordered as > they expect when they edit the todo list. It is hard to see how to > accommodate the config setting though - I don't think we want > "rebase.autosquash=true" to imply "--interactive". > > 2 - make "--autosquash" without "--interactive" an error. This would > prevent the user being surprised that their commits are not squashed > by a non-interactive rebase. Users who have set > "rebase.autosquash=true" would have to pass "--no-autosquash" to > perform any form of non-interactive rebase. This is similar to the > current behavior where the user has to pass "--no-autosquash" if > they want to use the apply backend with "rebase.autosquash=true". > > 3 - make "--autosquash" rearrange and squash commits without > "--interactive". This is convenient but there is a risk in that the > user does not get a chance to check the todo list before the commits > are reordered and squashed. I think that risk is fairly small with > an explicit "--autosquash" on the commandline. This is the approach > taken by this patch. I do have some concerns about extending the > config setting to non-interactive rebases though. If the user has > commits that look like > > fixup! foo (HEAD) > foo bar > foo > > and runs "git -c rebase.autosquash=non-interactive rebase HEAD~2" > then we'll silently squash the fixup into the wrong commit due to a > prefix subject match. Good analysis. My order of preference is 3 (obviously), 1, 2. >> except that they prevent fast-forward and that they trigger conflicts >> with --apply and relatives, which is less than helpful particularly for >> the config setting. > > The behavior to make the config setting incompatible with the apply > backend was implemented to avoid users being surprised that their > commits are not squashed by that backend even when they have set > "rebase.autosquash=true"[1]. I think one could consider "--autosquash" > being silently ignored without "--interactive" to be an oversight in > 796abac7e1 (rebase: add coverage of other incompatible options, > 2023-01-25) that introduced that change. > > [1] > https://lore.kernel.org/git/pull.1466.v5.git.1674619434.gitgitgadget@gmail.com/ > >> Since the "merge" backend used for interactive rebase also is the >> default for non-interactive rebase, there doesn't appear to be a >> reason not to do --autosquash without --interactive, so support that. > > I think making "--autosquash" on the commandline work for > non-interactive rebases is reasonable but I would be open to the > argument that it would be better to make it an error and require > "--interactive" to allow the user to check that the commits are going to > be reordered as they expect. I found that once I got used to and started trusting the feature, particularly in connection with the corresponding git-commit support, I no longer felt the need to check the todo list as I'd inspect the log afterwards anyway. And of course there's always resetting to ORIG_HEAD when things do go wrong. So I think users should be trusted with this, especially as it's not a particularly dangerous feature, given it requires the squash markers to be present in the first place to do anything. >> Turn rebase.autoSquash into a comma-separated list of flags, with >> "interactive" or "i" enabling auto-squashing with --interactive, and >> "no-interactive" or "no-i" enabling it without. Make boolean true mean >> "interactive" for backward compatibility. > > Please, please, please don't introduce abbreviated config settings, it > just makes the interface more complicated. The user only has to set this > once so I think the short names just add confusion. Duly noted. > I also think > "non-interactive" would be a better name for the config setting > corresponding to non-interactive rebases. Does this mean the user can > request that commits are only rearranged when they do not pass > "--interactive"? Yes. That doesn't seem useful. > As I said above I do have some concerns that the > "rebase.autosquash=non-interactive" setting will catch people out. I think you're right, so I've gone back to interpreting it as a boolean, but officially make it affect interactive mode only. > Having said that ignoring "rebase.autosquash=true" without > "--interactive" as we do now is inconsistent with the behavior of > "rebase.autosquash=true" with "--apply". One possibility would be to > introduce "rebase.autosquash=interactive" which would not cause an error > with "--apply" and always require an explicit "--autosquash" on the > commandline to squash fixups without "--interactive" I don't think different error behaviour is worth a separate setting, as we can't change rebase.autosquash=true to do auto-squashing without --interactive without surprising people. >> Don't prevent fast-forwards or report conflicts with --apply options >> when auto-squashing is not active. > > I think this change deserves to be in a separate commit (which probably > means separating out the config changes into that commit) as it is not > directly related to fixing "--autosquash" without "--interactive" on the > commandline. Done in v4. > In summary I like "--autosquash" working without "--interactive" but I'm > unsure about the config changes. Thanks very much for the thoughtful review! Regards, Andy
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt index 9c248accec..68191e5673 100644 --- a/Documentation/config/rebase.txt +++ b/Documentation/config/rebase.txt @@ -9,7 +9,16 @@ rebase.stat:: rebase. False by default. rebase.autoSquash:: - If set to true enable `--autosquash` option by default. + A comma-separated list of flags for when to enable auto-squashing. + Specifying `interactive` or `i` enables auto-squashing for rebasing with + `--interactive`, whereas `no-interactive` or `no-i` enables it for + rebasing without that option. For example, setting this to `i,no-i` + enables auto-squashing for both types. Setting it to true is equivalent + to setting it to `interactive`. + + The `--autosquash` and `--no-autosquash` options of + linkgit:git-rebase[1] override the setting here. + Auto-squashing is disabled by default. rebase.autoStash:: When set to true, automatically create a temporary stash entry diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index e7b39ad244..102ff91493 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -592,7 +592,7 @@ See also INCOMPATIBLE OPTIONS below. When the commit log message begins with "squash! ..." or "fixup! ..." or "amend! ...", and there is already a commit in the todo list that matches the same `...`, automatically modify the todo list of - `rebase -i`, so that the commit marked for squashing comes right after + `rebase`, so that the commit marked for squashing comes right after the commit to be modified, and change the action of the moved commit from `pick` to `squash` or `fixup` or `fixup -C` respectively. A commit matches the `...` if the commit subject matches, or if the `...` refers diff --git a/builtin/rebase.c b/builtin/rebase.c index 261a9a61fc..26c3e5dcb4 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -131,7 +131,10 @@ struct rebase_options { int reapply_cherry_picks; int fork_point; int update_refs; - int config_autosquash; + enum { + AUTOSQUASH_INTERACTIVE = 1 << 0, + AUTOSQUASH_NO_INTERACTIVE = 1 << 1, + } config_autosquash; int config_rebase_merges; int config_update_refs; }; @@ -149,7 +152,6 @@ struct rebase_options { .reapply_cherry_picks = -1, \ .allow_empty_message = 1, \ .autosquash = -1, \ - .config_autosquash = -1, \ .rebase_merges = -1, \ .config_rebase_merges = -1, \ .update_refs = -1, \ @@ -711,10 +713,8 @@ static int run_specific_rebase(struct rebase_options *opts) if (opts->type == REBASE_MERGE) { /* Run sequencer-based rebase */ setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1); - if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) { + if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) setenv("GIT_SEQUENCE_EDITOR", ":", 1); - opts->autosquash = 0; - } if (opts->gpg_sign_opt) { /* remove the leading "-S" */ char *tmp = xstrdup(opts->gpg_sign_opt + 2); @@ -748,6 +748,30 @@ static int run_specific_rebase(struct rebase_options *opts) return status ? -1 : 0; } +static void parse_rebase_autosquash_value(struct rebase_options *opts, + const char *var, const char *value) +{ + struct string_list tokens = STRING_LIST_INIT_NODUP; + char *buf = xstrdup(value); + + opts->config_autosquash = 0; + string_list_split_in_place(&tokens, buf, ",", -1); + + for (int i = 0; i < tokens.nr; i++) { + const char *s = tokens.items[i].string; + + if (!strcmp(s, "i") || !strcmp(s, "interactive")) + opts->config_autosquash |= AUTOSQUASH_INTERACTIVE; + else if (!strcmp(s, "no-i") || !strcmp(s, "no-interactive")) + opts->config_autosquash |= AUTOSQUASH_NO_INTERACTIVE; + else + die(_("invalid value for '%s': '%s'"), var, s); + } + + string_list_clear(&tokens, 0); + free(buf); +} + static void parse_rebase_merges_value(struct rebase_options *options, const char *value) { if (!strcmp("no-rebase-cousins", value)) @@ -772,8 +796,14 @@ static int rebase_config(const char *var, const char *value, } if (!strcmp(var, "rebase.autosquash")) { - opts->config_autosquash = git_config_bool(var, value); - return 0; + int b = git_parse_maybe_bool(value); + + if (b < 0) + parse_rebase_autosquash_value(opts, var, value); + else if (b) + opts->config_autosquash = AUTOSQUASH_INTERACTIVE; + else + opts->config_autosquash = 0; } if (!strcmp(var, "commit.gpgsign")) { @@ -1402,13 +1432,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) state_dir_base, cmd_live_rebase, buf.buf); } + if (options.autosquash == 1) { + imply_merge(&options, "--autosquash"); + } else if (options.autosquash == -1) { + int conf = options.config_autosquash; + options.autosquash = + (options.flags & REBASE_INTERACTIVE_EXPLICIT) + ? !!(conf & AUTOSQUASH_INTERACTIVE) + : !!(conf & AUTOSQUASH_NO_INTERACTIVE); + } + if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) || (options.action != ACTION_NONE) || (options.exec.nr > 0) || - (options.autosquash == -1 && options.config_autosquash == 1) || - options.autosquash == 1) { + options.autosquash) { allow_preemptive_ff = 0; } + if (options.committer_date_is_author_date || options.ignore_date) options.flags |= REBASE_FORCE; @@ -1508,7 +1548,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (is_merge(&options)) die(_("apply options and merge options " "cannot be used together")); - else if (options.autosquash == -1 && options.config_autosquash == 1) + else if (options.autosquash && options.config_autosquash) 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")); @@ -1529,11 +1569,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) 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 : - ((options.config_autosquash >= 0) ? options.config_autosquash : 0); - if (options.type == REBASE_UNSPECIFIED) { if (!strcmp(options.default_backend, "merge")) options.type = REBASE_MERGE; @@ -1858,3 +1893,4 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) free(keep_base_onto_name); return !!ret; } + diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh index a364530d76..1386eb6394 100755 --- a/t/t3415-rebase-autosquash.sh +++ b/t/t3415-rebase-autosquash.sh @@ -43,7 +43,7 @@ test_auto_fixup () { git tag $1 && test_tick && - git rebase $2 -i HEAD^^^ && + git rebase $2 HEAD^^^ && git log --oneline >actual && if test -n "$no_squash" then @@ -61,15 +61,43 @@ test_auto_fixup () { } test_expect_success 'auto fixup (option)' ' - test_auto_fixup final-fixup-option --autosquash + test_auto_fixup fixup-option --autosquash && + test_auto_fixup fixup-option-i "-i --autosquash" ' -test_expect_success 'auto fixup (config)' ' - git config rebase.autosquash true && - test_auto_fixup final-fixup-config-true && - test_auto_fixup ! fixup-config-true-no --no-autosquash && +test_expect_success 'auto fixup (config false)' ' git config rebase.autosquash false && - test_auto_fixup ! final-fixup-config-false + test_auto_fixup ! fixup-config-false && + test_auto_fixup ! fixup-config-false-i -i +' + +test_expect_success 'auto fixup (config true)' ' + git config rebase.autosquash true && + test_auto_fixup ! fixup-config-true && + test_auto_fixup fixup-config-true-i -i && + test_auto_fixup ! fixup-config-true-i-no "-i --no-autosquash" +' + +test_expect_success 'auto fixup (config interactive)' ' + git config rebase.autosquash interactive && + test_auto_fixup ! fixup-config-interactive && + test_auto_fixup fixup-config-interactive-i -i && + test_auto_fixup ! fixup-config-interactive-i-no "-i --no-autosquash" +' + +test_expect_success 'auto fixup (config no-interactive)' ' + git config rebase.autosquash no-interactive && + test_auto_fixup fixup-config-no-interactive && + test_auto_fixup ! fixup-config-no-interactive-i -i && + test_auto_fixup ! fixup-config-no-interactive-no "--no-autosquash" +' + +test_expect_success 'auto fixup (config always)' ' + git config rebase.autosquash i,no-i && + test_auto_fixup fixup-config-always && + test_auto_fixup fixup-config-always-i -i && + test_auto_fixup ! fixup-config-always-no --no-autosquash && + test_auto_fixup ! fixup-config-always-i-no "-i --no-autosquash" ' test_auto_squash () { @@ -87,7 +115,7 @@ test_auto_squash () { git commit -m "squash! first" -m "extra para for first" && git tag $1 && test_tick && - git rebase $2 -i HEAD^^^ && + git rebase $2 HEAD^^^ && git log --oneline >actual && if test -n "$no_squash" then @@ -105,15 +133,42 @@ test_auto_squash () { } test_expect_success 'auto squash (option)' ' - test_auto_squash final-squash --autosquash + test_auto_squash squash-option --autosquash && + test_auto_squash squash-option-i "-i --autosquash" ' -test_expect_success 'auto squash (config)' ' - git config rebase.autosquash true && - test_auto_squash final-squash-config-true && - test_auto_squash ! squash-config-true-no --no-autosquash && +test_expect_success 'auto squash (config false)' ' git config rebase.autosquash false && - test_auto_squash ! final-squash-config-false + test_auto_squash ! squash-config-false && + test_auto_squash ! squash-config-false-i -i +' + +test_expect_success 'auto squash (config true)' ' + git config rebase.autosquash true && + test_auto_squash ! squash-config-true && + test_auto_squash squash-config-true-i -i && + test_auto_squash ! squash-config-true-i-no "-i --no-autosquash" +' + +test_expect_success 'auto squash (config interactive)' ' + git config rebase.autosquash i && + test_auto_squash ! squash-config-interactive && + test_auto_squash squash-config-interactive-i -i && + test_auto_squash ! squash-config-interactive-i-no "-i --no-autosquash" +' + +test_expect_success 'auto squash (config no-interactive)' ' + git config rebase.autosquash no-i && + test_auto_squash squash-config-no-interactive && + test_auto_squash ! squash-config-no-interactive-i -i && + test_auto_squash ! squash-config-no-interactive-no "--no-autosquash" +' +test_expect_success 'auto squash (config always)' ' + git config rebase.autosquash interactive,no-interactive && + test_auto_squash squash-config-always && + test_auto_squash squash-config-always-i -i && + test_auto_squash ! squash-config-always-no --no-autosquash && + test_auto_squash ! squash-config-always-i-no "-i --no-autosquash" ' test_expect_success 'misspelled auto squash' ' diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh index 2eba00bdf5..e5119e7371 100755 --- a/t/t3422-rebase-incompatible-options.sh +++ b/t/t3422-rebase-incompatible-options.sh @@ -102,7 +102,7 @@ test_rebase_am_only () { test_expect_success "$opt incompatible with rebase.autosquash" " git checkout B^0 && - test_must_fail git -c rebase.autosquash=true rebase $opt A 2>err && + test_must_fail git -c rebase.autosquash=no-i rebase $opt A 2>err && grep -e --no-autosquash err "
So far, the rebase --autosquash option and rebase.autoSquash=true config setting are quietly ignored when used without --interactive, except that they prevent fast-forward and that they trigger conflicts with --apply and relatives, which is less than helpful particularly for the config setting. Since the "merge" backend used for interactive rebase also is the default for non-interactive rebase, there doesn't appear to be a reason not to do --autosquash without --interactive, so support that. Turn rebase.autoSquash into a comma-separated list of flags, with "interactive" or "i" enabling auto-squashing with --interactive, and "no-interactive" or "no-i" enabling it without. Make boolean true mean "interactive" for backward compatibility. Don't prevent fast-forwards or report conflicts with --apply options when auto-squashing is not active. Change the git-rebase and config/rebase documentation accordingly, and extend t3415-rebase-autosquash.sh to test the new rebase.autosquash values and combinations with and without --interactive. Signed-off-by: Andy Koppe <andy.koppe@gmail.com> --- Documentation/config/rebase.txt | 11 +++- Documentation/git-rebase.txt | 2 +- builtin/rebase.c | 66 +++++++++++++++----- t/t3415-rebase-autosquash.sh | 83 +++++++++++++++++++++----- t/t3422-rebase-incompatible-options.sh | 2 +- 5 files changed, 132 insertions(+), 32 deletions(-)