Re: [PATCH v17 3/3] am: support --allow-empty to record specific empty patches

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 7, 2021, 18:23 UTC
Message-ID: <xmqqpmq88eai.fsf@gitster.g>
In-Reply-To: <ea2dc088b37670cd7969a57777798b8f5bdd94e4.1638865913.git.gitgitgadget@gmail.com>

""徐沛文 (Aleen)" via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 39 quoted lines

> From: =?UTF-8?q?=E5=BE=90=E6=B2=9B=E6=96=87=20=28Aleen=29?=
>  <aleen42@vip.qq.com>
>
> This option helps to record specific empty patches in the middle
> of an am session. However, it is a valid resume value only when:
>
>     1. index has not changed
>     2. lacking a branch
>
> Signed-off-by: 徐沛文 (Aleen) <aleen42@vip.qq.com>
> ---
>  Documentation/git-am.txt |  7 +++++-
>  builtin/am.c             | 37 +++++++++++++++++++++-------
>  t/t4150-am.sh            | 53 ++++++++++++++++++++++++++++++++++++++++
>  t/t7512-status-help.sh   |  1 +
>  wt-status.c              |  3 +++
>  5 files changed, 91 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
> index 7676bd58ae7..8d3aa552a20 100644
> --- a/Documentation/git-am.txt
> +++ b/Documentation/git-am.txt
> @@ -18,7 +18,7 @@ SYNOPSIS
>  	 [--quoted-cr=<action>]
>  	 [--empty=(stop|drop|keep)]
>  	 [(<mbox> | <Maildir>)...]
> -'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=(diff|raw)])
> +'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=(diff|raw)] | --allow-empty)
>  
>  DESCRIPTION
>  -----------
> @@ -200,6 +200,11 @@ default.   You can use `--no-utf8` to override this.
>  	the e-mail message; if `diff`, show the diff portion only.
>  	Defaults to `raw`.
>  
> +--allow-empty::
> +	After a patch failure on an input e-mail message lacking a patch,
> +	the user can still record the empty patch as an empty commit with
> +	the contents of the e-mail message as its log.

"The user can still do X" is not technically incorrect, but this is not merely a possiblity but does actively does X, so

	After a patch failure on an input e-mail message lacking a patch,
	create an empty commit with the contents of the e-mail
	message as its log message.

or something.

Show 11 quoted lines

> diff --git a/builtin/am.c b/builtin/am.c
> index f45aa33366f..89a29ccac89 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1825,7 +1825,8 @@ static void am_run(struct am_state *state, int resume)
>  				to_keep = 1;
>  				break;
>  			case STOP_ON_EMPTY_COMMIT:
> -				printf_ln(_("Patch is empty."));
> +				printf_ln(_("Patch is empty.\n"
> +					    "If you want to record it as an empty commit, run \"git am --allow-empty\"."));

It would be better to hide the new part of the message behind advice_if_enabled(), and advise "--skip" first before giving "--allow-empty" as another possibility. It is rather a common mistake to grab patches with the cover letter and see "am" die on the cover letter.

If we were to add an advice message there, that is. I am not sure if it is necessary, especially since we give them in "git status".

Show 23 quoted lines

> @@ -1898,21 +1899,34 @@ next:
>  /**
>   * Resume the current am session after patch application failure. The user did
>   * all the hard work, and we do not have to do any patch application. Just
> - * trust and commit what the user has in the index and working tree.
> + * trust and commit what the user has in the index and working tree. If `allow_empty`
> + * is true, commit as an empty commit when index has not changed and lacking a patch.
>   */
> -static void am_resolve(struct am_state *state)
> +static void am_resolve(struct am_state *state, int allow_empty)
>  {
> +	int index_changed;
> +
>  	validate_resume_state(state);
>  
>  	say(state, stdout, _("Applying: %.*s"), linelen(state->msg), state->msg);
>  
> -	if (!repo_index_has_changes(the_repository, NULL, NULL)) {
> -		printf_ln(_("No changes - did you forget to use 'git add'?\n"
> -			"If there is nothing left to stage, chances are that something else\n"
> -			"already introduced the same changes; you might want to skip this patch."));
> +	index_changed = repo_index_has_changes(the_repository, NULL, NULL);
> +	if (allow_empty && (index_changed || !is_empty_or_missing_file(am_path(state, "patch")))) {

Overlong line.

> +		printf_ln(_("Invalid resume value."));

It is unclear to the user what "resume value" means. I am guessing that this is saying that the user gave us the "--allow-empty" option, but it is an inappropriate option for the situation. So

	"--allow-empty is not appropriate in this situation"

would be a slight improvement, but it is not clear what makes it inappropriate, iow, what the situation is, to the users.

Stepping back a bit, if we disect that overlong condition:

	if (allow_empty &&
	    (index_changed || !is_empty_or_missing_file(am_path(state, "patch")))) {

it seems to be computing something not quite right. What we want with "allow" empty is (1) if we have recorded changes to the index, that is perfectly fine. We'll create a non-empty commit, and (2) if we do not have any change in the index because the input did not have a patch, it is OK to create an empty commit out of the current index. But the condition is there only to complain about "--allow-empty" given when either (1) we have changes, or (2) when the input had a patch. These are conditions that we can internally ignore allow_empty settings and let the existing code handle the message as before this patch.

>  		die_user_resolve(state);

And I do not see how "when you have resolved with this problem, do one of these things" message would help in this situation. Perhaps we can get rid of the above block. We still need to compute index_changed, and make "no changes--we will die" conditional.

> +	if (!index_changed) {
> +		if (allow_empty)
> +			printf_ln(_("No changes - record it as an empty commit."));

A command to the user to "record it as an empty commit" is not what we want to give. Perhaps "recorded it as ..." would work better as a report of what we did for the user.

Also style:

		if (allow_empty) {
			printf_ln(...);
		} else {
                	... original multi-statement block come here ...
		}

That is, when one arm of if/else if/.../else cascade needs {}, put {} to all of them.

I think there is a logic error here once we remove the way too aggressive "you may not say --allow-empty when there is a patch" check we saw earlier.

When we see no changes added to the index and "--allow-empty" given, we shouldn't be blindly creating an empty commit without checking if we didn't have any patch. If the last failure that gave the control back from "am" to the user was a patch that did not apply, we do not want to create an empty commit, do we? So something like

	if (!repo_index_has_changes(the_repository, NULL, NULL)) {
		if (allow_empty &&
		    is_empty_or_missing_file(am_path(state, "patch"))) {
			printf_ln(_("No changes - recorded an empty commit."));
		} else {
			... original block ...

perhaps?

Show 7 quoted lines

> +		else {
> +			printf_ln(_("No changes - did you forget to use 'git add'?\n"
> +				    "If there is nothing left to stage, chances are that something else\n"
> +				    "already introduced the same changes; you might want to skip this patch."));
> +			die_user_resolve(state);
> +		}
> +	}

Show 7 quoted lines

> @@ -2237,7 +2251,8 @@ enum resume_type {
>  	RESUME_SKIP,
>  	RESUME_ABORT,
>  	RESUME_QUIT,
> -	RESUME_SHOW_PATCH
> +	RESUME_SHOW_PATCH,
> +	RESUME_ALLOW_EMPTY

Advice on the trailing comma applies here equally.

>  };

Show 11 quoted lines

> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
> index 7f2956d77ad..9309becfe03 100755
> --- a/t/t7512-status-help.sh
> +++ b/t/t7512-status-help.sh
> @@ -658,6 +658,7 @@ test_expect_success 'status in an am session: empty patch' '
>  On branch am_empty
>  You are in the middle of an am session.
>  The current patch is empty.
> +  (use "git am --allow-empty" to record this patch as an empty commit)
>    (use "git am --skip" to skip this patch)
>    (use "git am --abort" to restore the original branch)

You do not want to add the new one the first. "--skip" should come first, and perhaps this new one, and "--abort" at the end.

The general rule of thumb is to give common ones early before the less common ones and ones, and give ones with more severe consequence the last.

Show 14 quoted lines

> diff --git a/wt-status.c b/wt-status.c
> index 5d215f4e4f1..d578a0e9192 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1227,6 +1227,9 @@ static void show_am_in_progress(struct wt_status *s,
>  		if (!s->state.am_empty_patch)
>  			status_printf_ln(s, color,
>  				_("  (fix conflicts and then run \"git am --continue\")"));
> +		else
> +			status_printf_ln(s, color,
> +				_("  (use \"git am --allow-empty\" to record this patch as an empty commit)"));
>  		status_printf_ln(s, color,
>  			_("  (use \"git am --skip\" to skip this patch)"));
>  		status_printf_ln(s, color,