| Message ID | 20231104220330.14577-1-andy.koppe@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] rebase: support non-interactive autosquash | expand |
Andy Koppe <andy.koppe@gmail.com> writes: > 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. OK. You do not explicitly say "So far," by the way. Our log message convention is to first describe what happens in the system in the present tense to illustrate why it is suboptimal, to prepare readers' minds to anticipate the solution, which is described next. > 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. Nice. > 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. "i" and "no-i" are questionable (will talk about them later), but otherwise, nicely explained. > 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> > --- When asking reviews on a new iteration [PATCH v(N+1)], please summarize the differences relative to [PATCH vN]. For explaining such incremental changes for individual patches, here between the three-dash line and the diffstat is the place to do so. When you have a cover letter [PATCH 0/X], it can be done in that messaage. Either way is OK. Doing both is also helpful as long as the explanation done in two places do not contradict with each other. > Documentation/config/rebase.txt | 11 +++- > Documentation/git-rebase.txt | 2 +- > builtin/rebase.c | 63 ++++++++++++++----- > t/t3415-rebase-autosquash.sh | 83 +++++++++++++++++++++----- > t/t3422-rebase-incompatible-options.sh | 2 +- > 5 files changed, 129 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. If you trid to format the documentation before sending this patch, you'd have seen the second paragraph formatted as if it were a code snippet. Dedent the second paragraph (and later ones if you had more than one extra paragraphs), and turn the blank line between the paragraphs into a line with "+" (and nothing else) on it. See the description of `--autosquash` option in Documentation/git-rebase.txt for an example. > 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..0403c7415c 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,27 @@ 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); > + } > +} OK, by clearing opts->config_autosquash in this function, you keep the rebase.autosquash to be "the last one wins" as a whole. If a configuration file with lower precedence (e.g., /etc/gitconfig) says "[rebase] autosquash" to set it to "interactive,no-interactive", a separate setting in your ~/.gitconfig "[rebase] autosquash = false" would override both bits. A more involved design may let the users override these bits independently by allowing something like "!no-i" (take whatever the lower precedence configuration file says for the interactive case, but disable autosquash when running a non-interactive rebase) as the value, but I think the approach taken by this patch to allow replacing as a whole is OK. It is simpler to explain. Giving short-hands for often used command line options is one thing, but I do not think a short-hand is warranted here, especially when the other one needs to be a less-than-half legible "no-i" that does not allow "no-int" and friends, for configuration variable values. I'd strongly suggest dropping them. Thanks.
On 06/11/2023 00:50, Junio C Hamano wrote: > Andy Koppe <andy.koppe@gmail.com> writes: > Our log > message convention is to first describe what happens in the system > in the present tense to illustrate why it is suboptimal, to prepare > readers' minds to anticipate the solution, which is described next. > > When asking reviews on a new iteration [PATCH v(N+1)], please > summarize the differences relative to [PATCH vN]. For explaining > such incremental changes for individual patches, here between the > three-dash line and the diffstat is the place to do so. When you > have a cover letter [PATCH 0/X], it can be done in that messaage. > Either way is OK. Doing both is also helpful as long as the > explanation done in two places do not contradict with each other. > If you tried to format the documentation before sending this patch, > you'd have seen the second paragraph formatted as if it were a code > snippet. Dedent the second paragraph (and later ones if you had > more than one extra paragraphs), and turn the blank line between the > paragraphs into a line with "+" (and nothing else) on it. See the > description of `--autosquash` option in Documentation/git-rebase.txt > for an example. Sad thing is that I knew most of that from reading the contribution guidelines and previous experience, but obviously I don't always remember. So thanks for you patience in re-explaining that. > OK, by clearing opts->config_autosquash in this function, you keep > the rebase.autosquash to be "the last one wins" as a whole. If a > configuration file with lower precedence (e.g., /etc/gitconfig) says > "[rebase] autosquash" to set it to "interactive,no-interactive", a > separate setting in your ~/.gitconfig "[rebase] autosquash = false" > would override both bits. > > A more involved design may let the users override these bits > independently by allowing something like "!no-i" (take whatever the > lower precedence configuration file says for the interactive case, > but disable autosquash when running a non-interactive rebase) as the > value, but I think the approach taken by this patch to allow replacing > as a whole is OK. It is simpler to explain. > > Giving short-hands for often used command line options is one thing, > but I do not think a short-hand is warranted here, especially when > the other one needs to be a less-than-half legible "no-i" that does > not allow "no-int" and friends, for configuration variable values. > I'd strongly suggest dropping them. Dropped in v4, along with the attempt to expand rebase.autoSquash, following Phillip's 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..0403c7415c 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,27 @@ 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); + } +} + static void parse_rebase_merges_value(struct rebase_options *options, const char *value) { if (!strcmp("no-rebase-cousins", value)) @@ -772,8 +793,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 +1429,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 +1545,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 +1566,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 +1890,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 | 63 ++++++++++++++----- t/t3415-rebase-autosquash.sh | 83 +++++++++++++++++++++----- t/t3422-rebase-incompatible-options.sh | 2 +- 5 files changed, 129 insertions(+), 32 deletions(-)