Re: [PATCH v12 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.
>
> Signed-off-by: 徐沛文 (Aleen) <aleen42@vip.qq.com>
> ---
>  Documentation/git-am.txt |  6 +++++-
>  builtin/am.c             | 24 +++++++++++++++++++-----
>  t/t4150-am.sh            | 12 ++++++++++++
>  t/t7512-status-help.sh   |  1 +
>  wt-status.c              |  3 +++
>  5 files changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
> index ba17063f621..fe3af32f7f7 100644
> --- a/Documentation/git-am.txt
> +++ b/Documentation/git-am.txt
> @@ -18,7 +18,7 @@ SYNOPSIS
>  	 [--quoted-cr=<action>]
>  	 [--empty=(die|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
>  -----------
> @@ -199,6 +199,10 @@ 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::
> +	Keep recording the empty patch as an empty commit with
> +	the contents of the e-mail message as its log.
> +
>  DISCUSSION
>  ----------
>  
> diff --git a/builtin/am.c b/builtin/am.c
> index cc6512275aa..2ae6fabb28a 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 DIE_EMPTY_COMMIT:
> -				printf_ln(_("Patch is empty."));
> +				printf_ln(_("Patch is empty.\n"
> +							"If you want to keep recording it, run \"git am --allow-empty\"."));

An overly deep indentation.  Make "If" align with "Patch",
probably.  Also "to keep recording it" -> "to record it".

"If you want to record the e-mail text as the log message of an
empty commit" may be a bit too much, but just saying "record it" may
not be clear enough to explain what gets recorded how.

> @@ -1898,10 +1899,15 @@ 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.
>   */
> -static void am_resolve(struct am_state *state)
> +static void am_resolve(struct am_state *state, int allow_empty)
>  {
> +	if (allow_empty) {
> +		goto commit;
> +	}

Lose {} around a single statement block.

But more importantly, doesn't this break "git am" when the user
gives "--allow-empty" in cases where you are not expecting?

Like when the patch application (with or without -3) fails, either
during "git am -i" or "git am" without "-i"?

The parameter claims to be "allow empty", implying that the option
is about allowing to end up in an empty commit (and depending on the
input, the resulting commit may not be empty and that is OK).  But
what the code does is to _ignore_ the unmerged check, not recording
the manual resolution the user did for later reuse, and force a call
to do_commit() fail if the index is still unmerged.

>  	validate_resume_state(state);
>  
>  	say(state, stdout, _("Applying: %.*s"), linelen(state->msg), state->msg);

I suspect that the real code that wants to be changed is below this
line, where we say "if the index does not have changes (relative to
the HEAD), then complain and die".  Perhaps it should become
something along this line:

	if (!repo_index_has_changes(...)) {
		if (allow_empty) {
			printf_ln(_("No changes - recording an empty commit.");
		} else {
			printf_ln(_("No changes - did you forget ..."
				    "..."));
			die_user_resolve(state);
		}
	}

i.e. "when the index does not have changes, tell the user that we'd
create an empty commit, if allow-empty is in effect. otherwise,
complain and die".

I suspect that the worst failure mode may be "git am" (without -3)
first fails to apply a patch (hence the index is unchanged wrt
HEAD), then "git am" that is run with "--allow-empty" ignores the
failure and creates an empty commit.  Even with the "something along
this line" change above, such a failure mode still exists.  You
really need to see if we had an input without any patch and do the
skipping only when it is the case, I think.