[v11,2/2] am: support --empty=<option> to handle empty patches

Commit Message

徐沛文 (Aleen) Nov. 23, 2021, 3:26 p.m. UTC

From: =?UTF-8?q?=E5=BE=90=E6=B2=9B=E6=96=87=20=28Aleen=29?=
 <[e-mail]>

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) <[e-mail]>
---
 Documentation/git-am.txt |  8 ++++++
 builtin/am.c             | 55 ++++++++++++++++++++++++++++++++++++----
 t/t4150-am.sh            | 49 +++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+), 5 deletions(-)

Comments

Elijah Newren Nov. 26, 2021, 8:14 p.m. UTC | #1

On Tue, Nov 23, 2021 at 8:38 AM 徐沛文 (Aleen) via GitGitGadget
<[e-mail]> wrote:
>
> From: =?UTF-8?q?=E5=BE=90=E6=B2=9B=E6=96=87=20=28Aleen=29?=
>  <[e-mail]>
>
> 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) <[e-mail]>
> ---
>  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

Aleen 徐沛文 Nov. 29, 2021, 9:19 a.m. UTC | #2

Dears Hamano,

   Elijah Newren has given two better suggestions:

       1. Use 'ask' rather than 'die'
       2. When erroring out 'Patch is empty', print out a tutorial information
          to help users using 'git commit --allow-empty' to keep recording as 
          an empty commit.

   Should we continue to implement these features in current PR?

Aleen

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

Aleen 徐沛文 Nov. 29, 2021, 10 a.m. UTC | #3

It is quite complicated to support 'git commit --allow-empty' extracting messages
from an empty patch, because 'git am' has to find somewhere to store the parsed state
for 'git commit' to read. How about directly supporting another interactive option
'--allow-empty' for 'git am' to keep recording?

> Dears Hamano,
> 
>    Elijah Newren has given two better suggestions:
> 
>        1. Use 'ask' rather than 'die'
>        2. When erroring out 'Patch is empty', print out a tutorial information
>           to help users using 'git commit --allow-empty' to keep recording as 
>           an empty commit.
> 
>    Should we continue to implement these features in current PR?
> 
> Aleen

Elijah Newren Nov. 29, 2021, 5:10 p.m. UTC | #4

On Mon, Nov 29, 2021 at 2:00 AM Aleen 徐沛文 <[e-mail]> wrote:
>
> It is quite complicated to support 'git commit --allow-empty' extracting messages
> from an empty patch, because 'git am' has to find somewhere to store the parsed state
> for 'git commit' to read.

.git/COMMIT_EDITMSG is such a place that already exists, assuming you
are just extracting a commit message.

> How about directly supporting another interactive option
> '--allow-empty' for 'git am' to keep recording?

--empty=keep was already part of your patch series; I don't see why
you'd need to add a synonym (--allow-empty) for it.

I think you were trying to ask if you could just leave out the
implementation of --empty=ask (or --empty=die) from your patches.
That's usually fine, but there is a small wrinkle here since it was
your chosen default.  You'll have to pick a different default, and
possibly alert users in the documentation that the default may change
in the future if/when the other option is implemented.

> > Dears Hamano,
> >
> >    Elijah Newren has given two better suggestions:
> >
> >        1. Use 'ask' rather than 'die'
> >        2. When erroring out 'Patch is empty', print out a tutorial information
> >           to help users using 'git commit --allow-empty' to keep recording as
> >           an empty commit.
> >
> >    Should we continue to implement these features in current PR?
> >
> > Aleen

Junio C Hamano Nov. 29, 2021, 6:17 p.m. UTC | #5

Elijah Newren <[e-mail]> writes:

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

I think it is the same as how "git am" without this sees a piece of
e-mail without any patch in it.  It exits with non-zero status, but
keeps the contents of .git/rebase-apply directory intact so that the
user can decide what the next action should be.  "the command stops
with non-zero status and gives control back to the user" might be a
better explanation.

As to the name of the option's value, I think 'die' is a much better
word than 'ask' for this behaviour.  It is not like we retain
control, give "do you want to do X or Y?"  prompt and do either X or
Y ourselves, which is what I expect out of 'ask'.  If we just exit
with non-zero value and give the control back to the user, then we
are not asking.

This is a tangent, but in the current Documentation/git-am.txt, the
phrasing for "--skip" and "--resolved" are a bit inconsistent.

 - The "--skip" operation calls the situation it is designed to handle
   "an aborted patch".

 - The "--resolved" operation calls it "a patch failure".

The "--abort" operation interestingly does not use any word to
explain the situation it is designed to be used in.  It just
assumes that the user knows "git am" session is somehow still in
effect.

Elijah Newren Nov. 29, 2021, 6:57 p.m. UTC | #6

On Mon, Nov 29, 2021 at 10:17 AM Junio C Hamano <[e-mail]> wrote:
>
> Elijah Newren <[e-mail]> writes:
>
> >> +--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?
>
> I think it is the same as how "git am" without this sees a piece of
> e-mail without any patch in it.  It exits with non-zero status, but
> keeps the contents of .git/rebase-apply directory intact so that the
> user can decide what the next action should be.  "the command stops
> with non-zero status and gives control back to the user" might be a
> better explanation.
>
> As to the name of the option's value, I think 'die' is a much better
> word than 'ask' for this behaviour.  It is not like we retain
> control, give "do you want to do X or Y?"  prompt and do either X or
> Y ourselves, which is what I expect out of 'ask'.  If we just exit
> with non-zero value and give the control back to the user, then we
> are not asking.

Well, I still think 'die' implies aborting the entire overall `am`
operation, and is thus a confusing term.  To me, 'stop' or 'interrupt'
would be better if you want a one-word term and don't like 'ask'.

If you don't like the term 'ask' here, perhaps we should also change
rebase?  rebase --empty has 'ask' as a choice for
stopping/interrupting the rebase operation and letting the user decide
how to handle it.  (There is a very subtly different context for
rebase's --empty=ask, namely in that it is only triggered for patches
that were not originally empty but become so do the the new base the
patches are being applied upon already having the changes from the
patch in question, but that doesn't seem like a big enough difference
to suggest it should have a different name.)


There's another bit to my query on the semantics, though.  The second
half of my critique was that the current form of the series does not
inform the user how to handle the situation when `git-am` stops.  It
only tells the user how to drop the patch.  (If there's only one
option, why doesn't git just take it automatically?)  I think it
should also tell the user how to keep the patch.  Now, if both options
are presented to the user when the operation stops, would that qualify
as the user having been 'asked' what to do?

Aleen 徐沛文 Nov. 30, 2021, 5:45 a.m. UTC | #7

I have introduced an interactive option `--allow-empty` in a new independent commit,
and it is not the synonym of the `--empty` option because it is only for the case in
a middle am session where users can resolve "died" problems by choosing whether to
record current empty patch. The `--empty` option acts like a strategy option to tell
`git-am` how to handle when meeting any empty patches.

> On Mon, Nov 29, 2021 at 2:00 AM Aleen 徐沛文 <[e-mail]> wrote:
> >
> > It is quite complicated to support 'git commit --allow-empty' extracting messages
> > from an empty patch, because 'git am' has to find somewhere to store the parsed state
> > for 'git commit' to read.
> 
> .git/COMMIT_EDITMSG is such a place that already exists, assuming you
> are just extracting a commit message.
> 
> > How about directly supporting another interactive option
> > '--allow-empty' for 'git am' to keep recording?
> 
> --empty=keep was already part of your patch series; I don't see why
> you'd need to add a synonym (--allow-empty) for it.
> 
> I think you were trying to ask if you could just leave out the
> implementation of --empty=ask (or --empty=die) from your patches.
> That's usually fine, but there is a small wrinkle here since it was
> your chosen default.  You'll have to pick a different default, and
> possibly alert users in the documentation that the default may change
> in the future if/when the other option is implemented.
> 
> > > Dears Hamano,
> > >
> > >    Elijah Newren has given two better suggestions:
> > >
> > >        1. Use 'ask' rather than 'die'
> > >        2. When erroring out 'Patch is empty', print out a tutorial information
> > >           to help users using 'git commit --allow-empty' to keep recording as
> > >           an empty commit.
> > >
> > >    Should we continue to implement these features in current PR?
> > >
> > > Aleen

Patch

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