From: phillip.wood123@gmail.com
Date: Thu, 22 Feb 2024 16:34:22 GMT
Subject: Re: [PATCH v2 3/8] rebase: update `--empty=ask` to `--empty=drop`
Message-ID: <9f16544e-b6cc-414f-81e5-aac9e076f8df@gmail.com>
In-Reply-To: <20240210074859.552497-4-brianmlyles@gmail.com>

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 &&
>