diff mbox series

[3/4] rebase: Update `--empty=ask` to `--empty=drop`

Message ID 20240119060721.3734775-4-brianmlyles@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/4] sequencer: Do not require `allow_empty` for redundant commit options | expand

Commit Message

Brian Lyles Jan. 19, 2024, 5:59 a.m. UTC 
From: Brian Lyles <brianmlyles@gmail.com>

When `git-am` 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. This commit updates
`git-rebase` to also use `stop`, while keeping `ask` as a deprecated
synonym.

In a future commit, we'll be adding a new `--empty` option for
`git-cherry-pick` as well, making the consistency even more relevant.

Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
---
 Documentation/git-rebase.txt |  8 +++++---
 builtin/rebase.c             | 12 ++++++------
 t/t3424-rebase-empty.sh      | 17 ++++++++++++++---
 3 files changed, 25 insertions(+), 12 deletions(-)

Comments

Phillip Wood Jan. 23, 2024, 2:24 p.m. UTC | #1 
Hi Brian

On 19/01/2024 05:59, brianmlyles@gmail.com wrote:
> From: Brian Lyles <brianmlyles@gmail.com>
> 
> When `git-am` 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,

I can see your reasoning but I think of stopping as git's way of asking 
what to do so I'm not sure if "stop" is better than "ask". I don't know 
how we ended up with two different terms - the prior art is "ask" so 
maybe we should change "am --empty" instead. Lets see what others think.

It would be helpful to mention the tests in the commit message - we end 
up with a mixture of "--empty=ask" and "--empty=stop" I assume that is 
by design

> and consistency is good. This commit updates
> `git-rebase` to also use `stop`, while keeping `ask` as a deprecated
> synonym.

If we're deprecating "ask" do we want to print a warning recommending 
"stop" instead?

Best Wishes

Phillip
Brian Lyles Jan. 27, 2024, 9:49 p.m. UTC | #2 
Hi Phillip

On Tue, Jan 23, 2024 at 8:24 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 19/01/2024 05:59, brianmlyles@gmail.com wrote:
> > From: Brian Lyles <brianmlyles@gmail.com>
> >
> > When `git-am` 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,
>
> I can see your reasoning but I think of stopping as git's way of asking
> what to do so I'm not sure if "stop" is better than "ask". I don't know
> how we ended up with two different terms - the prior art is "ask" so
> maybe we should change "am --empty" instead. Lets see what others think.

The suggestion to use 'stop' instead of 'ask' for rebase was initially
Elijah's[1], which I agreed with. I am certainly open to others'
opinions here though, and am content with whatever is decided. I am
mostly aiming for consistency between git-rebase(1), git-am(1), and
ultimately git-cherry-pick(1).

[1]: https://lore.kernel.org/git/CABPp-BGJfvBhO_zEX8nLoa8WNsjmwvtZ2qOjmYm9iPoZg4SwPw@mail.gmail.com/

> It would be helpful to mention the tests in the commit message - we end
> up with a mixture of "--empty=ask" and "--empty=stop" I assume that is
> by design

You are correct -- the intent being to ensure that `--ask` continues
working for as long as it is supported. I'll add this to the message in
v2.

> > and consistency is good. This commit updates
> > `git-rebase` to also use `stop`, while keeping `ask` as a deprecated
> > synonym.
>
> If we're deprecating "ask" do we want to print a warning recommending
> "stop" instead?

That makes sense -- I will include a warning for this in v2.

Thanks,
Brian Lyles
Phillip Wood Feb. 1, 2024, 2:02 p.m. UTC | #3 
Hi Brian

On 27/01/2024 21:49, Brian Lyles wrote:
> On Tue, Jan 23, 2024 at 8:24 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 19/01/2024 05:59, brianmlyles@gmail.com wrote:
>>> From: Brian Lyles <brianmlyles@gmail.com>
>>>
>>> When `git-am` 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,
>>
>> I can see your reasoning but I think of stopping as git's way of asking
>> what to do so I'm not sure if "stop" is better than "ask". I don't know
>> how we ended up with two different terms - the prior art is "ask" so
>> maybe we should change "am --empty" instead. Lets see what others think.
> 
> The suggestion to use 'stop' instead of 'ask' for rebase was initially
> Elijah's[1], which I agreed with. I am certainly open to others'
> opinions here though, and am content with whatever is decided. I am
> mostly aiming for consistency between git-rebase(1), git-am(1), and
> ultimately git-cherry-pick(1).
> 
> [1]: https://lore.kernel.org/git/CABPp-BGJfvBhO_zEX8nLoa8WNsjmwvtZ2qOjmYm9iPoZg4SwPw@mail.gmail.com/

Thanks for the link, that is useful context

>> It would be helpful to mention the tests in the commit message - we end
>> up with a mixture of "--empty=ask" and "--empty=stop" I assume that is
>> by design
> 
> You are correct -- the intent being to ensure that `--ask` continues
> working for as long as it is supported. I'll add this to the message in
> v2.

That makes sense,

Thanks

Phillip
diff mbox series

Patch

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3ee85f6d86..fe74d0c367 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -289,7 +289,7 @@  See also INCOMPATIBLE OPTIONS below.
 +
 See also INCOMPATIBLE OPTIONS below.
 
---empty=(drop|keep|ask)::
+--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
@@ -300,12 +300,14 @@  See also INCOMPATIBLE OPTIONS below.
 	The empty commit will be dropped. This is the default behavior.
 `keep`;;
 	The empty commit will be kept.
-`ask`;;
+`stop`;;
 	The rebase will halt when the empty 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 `--interactive` is specified.
 	Other options, like `--exec`, will use the default of drop unless
 	`-i`/`--interactive` is explicitly specified.
+`ask`;;
+	A deprecated synonym of `stop`.
 --
 +
 Note that commits which start empty are kept (unless `--no-keep-empty`
@@ -702,7 +704,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=(drop|keep|ask)` 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 043c65dccd..1fb9d8263d 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -62,7 +62,7 @@  enum empty_type {
 	EMPTY_UNSPECIFIED = -1,
 	EMPTY_DROP,
 	EMPTY_KEEP,
-	EMPTY_ASK
+	EMPTY_STOP
 };
 
 enum action {
@@ -963,10 +963,10 @@  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") || !strcasecmp(value, "ask"))
+		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,
@@ -1145,7 +1145,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,
@@ -1562,7 +1562,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 5e1045a0af..e3ddec88a2 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 &&