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

[Junio C Hamano <gitster@pobox.com>]

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

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

> 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".

> @@ -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?

> +		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);
> +		}
> +	}

> @@ -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.

>  };

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

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