Re: [PATCH v12 3/3] am: support --allow-empty to record specific empty patches
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 30, 2021, 07:21 UTC
- Message-ID
- <xmqq8rx62jn7.fsf@gitster.g>
- In-Reply-To
- <d44dac09c872257306d9718564820435e8a07ca2.1638250645.git.gitgitgadget@gmail.com>
""徐沛文 (Aleen)" via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 50 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.
>
> 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.
Show 14 quoted lines
> @@ -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.