| Message ID | 20240210074859.552497-4-brianmlyles@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c282eba2d561b726e4a60c8837e4eaa2ac6537fa |
| Headers | show |
| Series | None | expand |
I just noticed that I incorrectly specified `--empty=drop` in the subject of this commit. It should read "rebase: update `--empty=ask` to `--empty=stop`". This will need to be corrected in a v3 reroll.
Hi Brian On 11/02/2024 04:54, Brian Lyles wrote: > I just noticed that I incorrectly specified `--empty=drop` in the > subject of this commit. It should read "rebase: update `--empty=ask` to > `--empty=stop`". This will need to be corrected in a v3 reroll. Thanks for flagging that, I'm afraid it will probably be next week before I take a proper look at these Best Wishes Phillip
Hi Brian On 10/02/2024 07:43, Brian Lyles wrote: > When git-am(1) got its own `--empty` option in 7c096b8d61 (am: support > --empty=<option> to handle empty patches, 2021-12-09), `stop` was used > instead of `ask`. `stop` is a more accurate term for describing what > really happens, and consistency is good. > > Update git-rebase(1) to also use `stop`, while keeping `ask` as a > deprecated synonym. Update the tests to primarily use `stop`, but also > ensure that `ask` is still allowed. > > In a future commit, we'll be adding a new `--empty` option for > git-cherry-pick(1) as well, making the consistency even more relevant. I'm sightly nervous of deprecating "ask" as the warnings have the potential to annoy users but it would be good to use consistent terminology so it may well be worth it. This patch the previous ones look good apart from one minor issue ... > Signed-off-by: Brian Lyles <brianmlyles@gmail.com> > Reported-by: Elijah Newren <newren@gmail.com> I think we normally put Reported-by: and Helped-by: etc above the patch authors Signed-off-by: trailer. Best Wishes Phillip > --- > Documentation/git-rebase.txt | 15 ++++++++------- > builtin/rebase.c | 16 ++++++++++------ > t/t3424-rebase-empty.sh | 21 ++++++++++++++++----- > 3 files changed, 34 insertions(+), 18 deletions(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 68cdebd2aa..6f64084a95 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -289,23 +289,24 @@ See also INCOMPATIBLE OPTIONS below. > + > See also INCOMPATIBLE OPTIONS below. > > ---empty=(ask|drop|keep):: > +--empty=(drop|keep|stop):: > How to handle commits that are not empty to start and are not > clean cherry-picks of any upstream commit, but which become > empty after rebasing (because they contain a subset of already > upstream changes): > + > -- > -`ask`;; > - The rebase will halt when the commit is applied, allowing you to > - choose whether to drop it, edit files more, or just commit the empty > - changes. This option is implied when `-i`/`--interactive` is > - specified. > `drop`;; > The commit will be dropped. This is the default behavior. > `keep`;; > The commit will be kept. This option is implied when `--exec` is > specified unless `-i`/`--interactive` is also specified. > +`stop`;; > +`ask`;; > + The rebase will halt when the commit is applied, allowing you to > + choose whether to drop it, edit files more, or just commit the empty > + changes. This option is implied when `-i`/`--interactive` is > + specified. `ask` is a deprecated synonym of `stop`. > -- > + > Note that commits which start empty are kept (unless `--no-keep-empty` > @@ -705,7 +706,7 @@ be dropped automatically with `--no-keep-empty`). > Similar to the apply backend, by default the merge backend drops > commits that become empty unless `-i`/`--interactive` is specified (in > which case it stops and asks the user what to do). The merge backend > -also has an `--empty=(ask|drop|keep)` option for changing the behavior > +also has an `--empty=(drop|keep|stop)` option for changing the behavior > of handling commits that become empty. > > Directory rename detection > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 4084a6abb8..3b9bb2fa06 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -58,7 +58,7 @@ enum empty_type { > EMPTY_UNSPECIFIED = -1, > EMPTY_DROP, > EMPTY_KEEP, > - EMPTY_ASK > + EMPTY_STOP > }; > > enum action { > @@ -954,10 +954,14 @@ static enum empty_type parse_empty_value(const char *value) > return EMPTY_DROP; > else if (!strcasecmp(value, "keep")) > return EMPTY_KEEP; > - else if (!strcasecmp(value, "ask")) > - return EMPTY_ASK; > + else if (!strcasecmp(value, "stop")) > + return EMPTY_STOP; > + else if (!strcasecmp(value, "ask")) { > + warning(_("--empty=ask is deprecated; use '--empty=stop' instead.")); > + return EMPTY_STOP; > + } > > - die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value); > + die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"stop\"."), value); > } > > static int parse_opt_keep_empty(const struct option *opt, const char *arg, > @@ -1136,7 +1140,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > "instead of ignoring them"), > 1, PARSE_OPT_HIDDEN), > OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate), > - OPT_CALLBACK_F(0, "empty", &options, "(drop|keep|ask)", > + OPT_CALLBACK_F(0, "empty", &options, "(drop|keep|stop)", > N_("how to handle commits that become empty"), > PARSE_OPT_NONEG, parse_opt_empty), > OPT_CALLBACK_F('k', "keep-empty", &options, NULL, > @@ -1553,7 +1557,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > > if (options.empty == EMPTY_UNSPECIFIED) { > if (options.flags & REBASE_INTERACTIVE_EXPLICIT) > - options.empty = EMPTY_ASK; > + options.empty = EMPTY_STOP; > else if (options.exec.nr > 0) > options.empty = EMPTY_KEEP; > else > diff --git a/t/t3424-rebase-empty.sh b/t/t3424-rebase-empty.sh > index 73ff35ced2..1ee6b00fd5 100755 > --- a/t/t3424-rebase-empty.sh > +++ b/t/t3424-rebase-empty.sh > @@ -72,6 +72,17 @@ test_expect_success 'rebase --merge --empty=keep' ' > test_cmp expect actual > ' > > +test_expect_success 'rebase --merge --empty=stop' ' > + git checkout -B testing localmods && > + test_must_fail git rebase --merge --empty=stop upstream && > + > + git rebase --skip && > + > + test_write_lines D C B A >expect && > + git log --format=%s >actual && > + test_cmp expect actual > +' > + > test_expect_success 'rebase --merge --empty=ask' ' > git checkout -B testing localmods && > test_must_fail git rebase --merge --empty=ask upstream && > @@ -101,9 +112,9 @@ test_expect_success 'rebase --interactive --empty=keep' ' > test_cmp expect actual > ' > > -test_expect_success 'rebase --interactive --empty=ask' ' > +test_expect_success 'rebase --interactive --empty=stop' ' > git checkout -B testing localmods && > - test_must_fail git rebase --interactive --empty=ask upstream && > + test_must_fail git rebase --interactive --empty=stop upstream && > > git rebase --skip && > > @@ -112,7 +123,7 @@ test_expect_success 'rebase --interactive --empty=ask' ' > test_cmp expect actual > ' > > -test_expect_success 'rebase --interactive uses default of --empty=ask' ' > +test_expect_success 'rebase --interactive uses default of --empty=stop' ' > git checkout -B testing localmods && > test_must_fail git rebase --interactive upstream && > > @@ -194,9 +205,9 @@ test_expect_success 'rebase --exec uses default of --empty=keep' ' > test_cmp expect actual > ' > > -test_expect_success 'rebase --exec --empty=ask' ' > +test_expect_success 'rebase --exec --empty=stop' ' > git checkout -B testing localmods && > - test_must_fail git rebase --exec "true" --empty=ask upstream && > + test_must_fail git rebase --exec "true" --empty=stop upstream && > > git rebase --skip && >
phillip.wood123@gmail.com writes: >> Signed-off-by: Brian Lyles <brianmlyles@gmail.com> >> Reported-by: Elijah Newren <newren@gmail.com> > > I think we normally put Reported-by: and Helped-by: etc above the > patch authors Signed-off-by: trailer. True. Teaching how to fish, the rule is to try emulating chronological order of events. Elijah reported and then the patch was written, and the "seal on the envelope" is what the author's sign-off attached at the end of the chain of events.
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 68cdebd2aa..6f64084a95 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -289,23 +289,24 @@ See also INCOMPATIBLE OPTIONS below. + See also INCOMPATIBLE OPTIONS below. ---empty=(ask|drop|keep):: +--empty=(drop|keep|stop):: How to handle commits that are not empty to start and are not clean cherry-picks of any upstream commit, but which become empty after rebasing (because they contain a subset of already upstream changes): + -- -`ask`;; - The rebase will halt when the commit is applied, allowing you to - choose whether to drop it, edit files more, or just commit the empty - changes. This option is implied when `-i`/`--interactive` is - specified. `drop`;; The commit will be dropped. This is the default behavior. `keep`;; The commit will be kept. This option is implied when `--exec` is specified unless `-i`/`--interactive` is also specified. +`stop`;; +`ask`;; + The rebase will halt when the commit is applied, allowing you to + choose whether to drop it, edit files more, or just commit the empty + changes. This option is implied when `-i`/`--interactive` is + specified. `ask` is a deprecated synonym of `stop`. -- + Note that commits which start empty are kept (unless `--no-keep-empty` @@ -705,7 +706,7 @@ be dropped automatically with `--no-keep-empty`). Similar to the apply backend, by default the merge backend drops commits that become empty unless `-i`/`--interactive` is specified (in which case it stops and asks the user what to do). The merge backend -also has an `--empty=(ask|drop|keep)` option for changing the behavior +also has an `--empty=(drop|keep|stop)` option for changing the behavior of handling commits that become empty. Directory rename detection diff --git a/builtin/rebase.c b/builtin/rebase.c index 4084a6abb8..3b9bb2fa06 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -58,7 +58,7 @@ enum empty_type { EMPTY_UNSPECIFIED = -1, EMPTY_DROP, EMPTY_KEEP, - EMPTY_ASK + EMPTY_STOP }; enum action { @@ -954,10 +954,14 @@ static enum empty_type parse_empty_value(const char *value) return EMPTY_DROP; else if (!strcasecmp(value, "keep")) return EMPTY_KEEP; - else if (!strcasecmp(value, "ask")) - return EMPTY_ASK; + else if (!strcasecmp(value, "stop")) + return EMPTY_STOP; + else if (!strcasecmp(value, "ask")) { + warning(_("--empty=ask is deprecated; use '--empty=stop' instead.")); + return EMPTY_STOP; + } - die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value); + die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"stop\"."), value); } static int parse_opt_keep_empty(const struct option *opt, const char *arg, @@ -1136,7 +1140,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) "instead of ignoring them"), 1, PARSE_OPT_HIDDEN), OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate), - OPT_CALLBACK_F(0, "empty", &options, "(drop|keep|ask)", + OPT_CALLBACK_F(0, "empty", &options, "(drop|keep|stop)", N_("how to handle commits that become empty"), PARSE_OPT_NONEG, parse_opt_empty), OPT_CALLBACK_F('k', "keep-empty", &options, NULL, @@ -1553,7 +1557,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (options.empty == EMPTY_UNSPECIFIED) { if (options.flags & REBASE_INTERACTIVE_EXPLICIT) - options.empty = EMPTY_ASK; + options.empty = EMPTY_STOP; else if (options.exec.nr > 0) options.empty = EMPTY_KEEP; else diff --git a/t/t3424-rebase-empty.sh b/t/t3424-rebase-empty.sh index 73ff35ced2..1ee6b00fd5 100755 --- a/t/t3424-rebase-empty.sh +++ b/t/t3424-rebase-empty.sh @@ -72,6 +72,17 @@ test_expect_success 'rebase --merge --empty=keep' ' test_cmp expect actual ' +test_expect_success 'rebase --merge --empty=stop' ' + git checkout -B testing localmods && + test_must_fail git rebase --merge --empty=stop upstream && + + git rebase --skip && + + test_write_lines D C B A >expect && + git log --format=%s >actual && + test_cmp expect actual +' + test_expect_success 'rebase --merge --empty=ask' ' git checkout -B testing localmods && test_must_fail git rebase --merge --empty=ask upstream && @@ -101,9 +112,9 @@ test_expect_success 'rebase --interactive --empty=keep' ' test_cmp expect actual ' -test_expect_success 'rebase --interactive --empty=ask' ' +test_expect_success 'rebase --interactive --empty=stop' ' git checkout -B testing localmods && - test_must_fail git rebase --interactive --empty=ask upstream && + test_must_fail git rebase --interactive --empty=stop upstream && git rebase --skip && @@ -112,7 +123,7 @@ test_expect_success 'rebase --interactive --empty=ask' ' test_cmp expect actual ' -test_expect_success 'rebase --interactive uses default of --empty=ask' ' +test_expect_success 'rebase --interactive uses default of --empty=stop' ' git checkout -B testing localmods && test_must_fail git rebase --interactive upstream && @@ -194,9 +205,9 @@ test_expect_success 'rebase --exec uses default of --empty=keep' ' test_cmp expect actual ' -test_expect_success 'rebase --exec --empty=ask' ' +test_expect_success 'rebase --exec --empty=stop' ' git checkout -B testing localmods && - test_must_fail git rebase --exec "true" --empty=ask upstream && + test_must_fail git rebase --exec "true" --empty=stop upstream && git rebase --skip &&
When git-am(1) got its own `--empty` option in 7c096b8d61 (am: support --empty=<option> to handle empty patches, 2021-12-09), `stop` was used instead of `ask`. `stop` is a more accurate term for describing what really happens, and consistency is good. Update git-rebase(1) to also use `stop`, while keeping `ask` as a deprecated synonym. Update the tests to primarily use `stop`, but also ensure that `ask` is still allowed. In a future commit, we'll be adding a new `--empty` option for git-cherry-pick(1) as well, making the consistency even more relevant. Signed-off-by: Brian Lyles <brianmlyles@gmail.com> Reported-by: Elijah Newren <newren@gmail.com> --- Documentation/git-rebase.txt | 15 ++++++++------- builtin/rebase.c | 16 ++++++++++------ t/t3424-rebase-empty.sh | 21 ++++++++++++++++----- 3 files changed, 34 insertions(+), 18 deletions(-)