From: Elijah Newren <newren@gmail.com>
Date: Fri, 26 Nov 2021 20:14:03 GMT
Subject: Re: [PATCH v11 2/2] am: support --empty=<option> to handle empty patches
Message-ID: <CABPp-BF-eYcJ7eS==VK3M2xOqf5z4KNK4dP6qd_UcfLO1NWBeg@mail.gmail.com>
In-Reply-To: <6051ad9440a966124e9147ec344ee6d87c46944a.1637681215.git.gitgitgadget@gmail.com>

On Tue, Nov 23, 2021 at 8:38 AM 徐沛文 (Aleen) via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: =?UTF-8?q?=E5=BE=90=E6=B2=9B=E6=96=87=20=28Aleen=29?=
>  <aleen42@vip.qq.com>
>
> Since that the command 'git-format-patch' can include patches of
> commits that emit no changes, the 'git-am' command should also
> support an option, named as '--empty', to specify how to handle
> those empty patches. In this commit, we have implemented three
> valid options ('die', 'drop' and 'keep').
>
> Signed-off-by: 徐沛文 (Aleen) <aleen42@vip.qq.com>
> ---
>  Documentation/git-am.txt |  8 ++++++
>  builtin/am.c             | 55 ++++++++++++++++++++++++++++++++++++----
>  t/t4150-am.sh            | 49 +++++++++++++++++++++++++++++++++++
>  3 files changed, 107 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
> index 0a4a984dfde..ba17063f621 100644
> --- a/Documentation/git-am.txt
> +++ b/Documentation/git-am.txt
> @@ -16,6 +16,7 @@ SYNOPSIS
>          [--exclude=<path>] [--include=<path>] [--reject] [-q | --quiet]
>          [--[no-]scissors] [-S[<keyid>]] [--patch-format=<format>]
>          [--quoted-cr=<action>]
> +        [--empty=(die|drop|keep)]
>          [(<mbox> | <Maildir>)...]
>  'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=(diff|raw)])
>
> @@ -63,6 +64,13 @@ OPTIONS
>  --quoted-cr=<action>::
>         This flag will be passed down to 'git mailinfo' (see linkgit:git-mailinfo[1]).
>
> +--empty=(die|drop|keep)::
> +       By default, or when the option is set to 'die', the command
> +       errors out on an input e-mail message that lacks a patch. When
> +       this option is set to 'drop', skip such an e-mail message instead.
> +       When this option is set to 'keep', create an empty commit,
> +       recording the contents of the e-mail message as its log.

What does 'errors out' mean?  Is the am operation aborted, and the
user return to the pre-am state?  Or is the am operation interrupted,
with the user being asked to choose whether to keep or drop the patch?
 Or something else (my first thought was "Are you going to leave the
index locked?")?  This description is not that clear.  To me, the
wording suggests aborted (or worse), but what you actually implemented
was an interrupt-and-ask.

Can I suggest using 'ask' instead of 'die'?  I think that will be
clearer, and it matches the term used by git rebase --empty.

Also, the only instructions given to the user when you interrupt
include how to skip the patch, but I don't see anything for how to
keep it.  The instructions are:
'''
Patch is empty.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
'''

I tried it manually, and it turns out "git am --continue" will just
spit out basically the same message again:
'''
Applying: empty commit
No changes - did you forget to use 'git add'?
If there is nothing left to stage, chances are that something else
already introduced the same changes; you might want to skip this patch.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
'''

And if I try to run `git commit --allow-empty` (which I happen to
remember is the command suggested by `git rebase --empty=ask` when it
stops), then I'm given an empty editor; it is not pre-populated with
the appropriate commit message.  Can the portion of the empty patch
corresponding to the commit message be added to .git/COMMIT_EDITMSG to
correct that?  Also, can some extra words be printed before
interrupting to explain what to do when you want to keep the empty
commit?  Something like:
"""
The current commit being applied is empty.  If you wish to commit it
anyway, use:
    git commit --allow-empty
"""

> +
>  -m::
>  --message-id::
>         Pass the `-m` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]),
> diff --git a/builtin/am.c b/builtin/am.c
> index 8677ea2348a..cc6512275aa 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -87,6 +87,12 @@ enum show_patch_type {
>         SHOW_PATCH_DIFF = 1,
>  };
>
> +enum empty_action {
> +       DIE_EMPTY_COMMIT = 0,  /* output errors */
> +       DROP_EMPTY_COMMIT,     /* skip with a notice message, unless "--quiet" has been passed */
> +       KEEP_EMPTY_COMMIT      /* keep recording as empty commits */
> +};
> +
>  struct am_state {
>         /* state directory path */
>         char *dir;
> @@ -118,6 +124,7 @@ struct am_state {
>         int message_id;
>         int scissors; /* enum scissors_type */
>         int quoted_cr; /* enum quoted_cr_action */
> +       int empty_type; /* enum empty_action */
>         struct strvec git_apply_opts;
>         const char *resolvemsg;
>         int committer_date_is_author_date;
> @@ -178,6 +185,25 @@ static int am_option_parse_quoted_cr(const struct option *opt,
>         return 0;
>  }
>
> +static int am_option_parse_empty(const struct option *opt,
> +                                    const char *arg, int unset)
> +{
> +       int *opt_value = opt->value;
> +
> +       BUG_ON_OPT_NEG(unset);
> +
> +       if (!strcmp(arg, "die"))
> +               *opt_value = DIE_EMPTY_COMMIT;
> +       else if (!strcmp(arg, "drop"))
> +               *opt_value = DROP_EMPTY_COMMIT;
> +       else if (!strcmp(arg, "keep"))
> +               *opt_value = KEEP_EMPTY_COMMIT;
> +       else
> +               return error(_("Invalid value for --empty: %s"), arg);
> +
> +       return 0;
> +}
> +
>  /**
>   * Returns path relative to the am_state directory.
>   */
> @@ -1248,11 +1274,6 @@ static int parse_mail(struct am_state *state, const char *mail)
>                 goto finish;
>         }
>
> -       if (is_empty_or_missing_file(am_path(state, "patch"))) {
> -               printf_ln(_("Patch is empty."));
> -               die_user_resolve(state);
> -       }
> -
>         strbuf_addstr(&msg, "\n\n");
>         strbuf_addbuf(&msg, &mi.log_message);
>         strbuf_stripspace(&msg, 0);
> @@ -1763,6 +1784,7 @@ static void am_run(struct am_state *state, int resume)
>         while (state->cur <= state->last) {
>                 const char *mail = am_path(state, msgnum(state));
>                 int apply_status;
> +               int to_keep;
>
>                 reset_ident_date();
>
> @@ -1792,8 +1814,27 @@ static void am_run(struct am_state *state, int resume)
>                 if (state->interactive && do_interactive(state))
>                         goto next;
>
> +               to_keep = 0;
> +               if (is_empty_or_missing_file(am_path(state, "patch"))) {
> +                       switch (state->empty_type) {
> +                       case DROP_EMPTY_COMMIT:
> +                               say(state, stdout, _("Skipping: %.*s"), linelen(state->msg), state->msg);
> +                               goto next;
> +                               break;
> +                       case KEEP_EMPTY_COMMIT:
> +                               to_keep = 1;
> +                               break;
> +                       case DIE_EMPTY_COMMIT:
> +                               printf_ln(_("Patch is empty."));
> +                               die_user_resolve(state);
> +                               break;
> +                       }
> +               }
> +
>                 if (run_applypatch_msg_hook(state))
>                         exit(1);
> +               if (to_keep)
> +                       goto commit;
>
>                 say(state, stdout, _("Applying: %.*s"), linelen(state->msg), state->msg);
>
> @@ -1827,6 +1868,7 @@ static void am_run(struct am_state *state, int resume)
>                         die_user_resolve(state);
>                 }
>
> +commit:
>                 do_commit(state);
>
>  next:
> @@ -2357,6 +2399,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
>                 { OPTION_STRING, 'S', "gpg-sign", &state.sign_commit, N_("key-id"),
>                   N_("GPG-sign commits"),
>                   PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
> +               OPT_CALLBACK_F(0, "empty", &state.empty_type, "{drop,keep,die}",
> +                 N_("how to handle empty patches"),
> +                 PARSE_OPT_NONEG, am_option_parse_empty),
>                 OPT_HIDDEN_BOOL(0, "rebasing", &state.rebasing,
>                         N_("(internal use for git-rebase)")),
>                 OPT_END()
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index 2aaaa0d7ded..8c8bd4db220 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -196,6 +196,12 @@ test_expect_success setup '
>
>         git format-patch -M --stdout lorem^ >rename-add.patch &&
>
> +       git checkout -b empty-commit &&
> +       git commit -m "empty commit" --allow-empty &&
> +
> +       : >empty.patch &&
> +       git format-patch --always --stdout empty-commit^ >empty-commit.patch &&
> +
>         # reset time
>         sane_unset test_tick &&
>         test_tick
> @@ -1152,4 +1158,47 @@ test_expect_success 'apply binary blob in partial clone' '
>         git -C client am ../patch
>  '
>
> +test_expect_success 'an empty input file is error regardless of --empty option' '
> +       test_when_finished "git am --abort || :" &&
> +       test_must_fail git am --empty=drop empty.patch 2>actual &&
> +       echo "Patch format detection failed." >expected &&
> +       test_cmp expected actual
> +'
> +
> +test_expect_success 'invalid when passing the --empty option alone' '
> +       test_when_finished "git am --abort || :" &&
> +       git checkout empty-commit^ &&
> +       test_must_fail git am --empty empty-commit.patch 2>err &&
> +       echo "error: Invalid value for --empty: empty-commit.patch" >expected &&
> +       test_cmp expected err
> +'
> +
> +test_expect_success 'a message without a patch is an error (default)' '
> +       test_when_finished "git am --abort || :" &&
> +       test_must_fail git am empty-commit.patch >err &&
> +       grep "Patch is empty" err
> +'
> +
> +test_expect_success 'a message without a patch is an error where an explicit "--empty=die" is given' '
> +       test_when_finished "git am --abort || :" &&
> +       test_must_fail git am --empty=die empty-commit.patch >err &&
> +       grep "Patch is empty." err
> +'
> +
> +test_expect_success 'a message without a patch will be skipped when "--empty=drop" is given' '
> +       git am --empty=drop empty-commit.patch >output &&
> +       git rev-parse empty-commit^ >expected &&
> +       git rev-parse HEAD >actual &&
> +       test_cmp expected actual &&
> +       grep "Skipping: empty commit" output
> +'
> +
> +test_expect_success 'record as an empty commit when meeting e-mail message that lacks a patch' '
> +       git am --empty=keep empty-commit.patch &&
> +       test_path_is_missing .git/rebase-apply &&
> +       git show empty-commit --format="%s" >expected &&
> +       git show HEAD --format="%s" >actual &&
> +       test_cmp actual expected
> +'
> +
>  test_done
> --
> gitgitgadget