diff mbox series

[v6,2/3] am: support --empty option to handle empty patches

Message ID 96d8573dc808bc32990842675ca32a0d1e8a8cef.1637232636.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series am: support --empty=(die|drop|keep) option to handle empty patches | expand

Commit Message

徐沛文 (Aleen) Nov. 18, 2021, 10:50 a.m. UTC
From: Aleen <aleen42@vip.qq.com>

Signed-off-by: Aleen <aleen42@vip.qq.com>
---
 Documentation/git-am.txt |  9 +++++
 builtin/am.c             | 48 +++++++++++++++++++++++---
 t/t4150-am.sh            | 73 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 125 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Nov. 19, 2021, 12:56 a.m. UTC | #1
"Aleen via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Aleen <aleen42@vip.qq.com>

Here is a place to explain to readers what it exactly means to
"support --empty option to handle empty patches".  You know what you
meant, I happen to know what you meant because I haven't forgotten
yet, but we are writing for developers in the future who need to fix
bugs in 6 months down the road to decide if this commit is involved
in the bug in "git am" they are hunting.

> Signed-off-by: Aleen <aleen42@vip.qq.com>
> ---
>  Documentation/git-am.txt |  9 +++++
>  builtin/am.c             | 48 +++++++++++++++++++++++---
>  t/t4150-am.sh            | 73 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 125 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
> index 0a4a984dfde..665bc89ca9f 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,14 @@ OPTIONS
>  --quoted-cr=<action>::
>  	This flag will be passed down to 'git mailinfo' (see linkgit:git-mailinfo[1]).
>  
> +--empty-commit=(die|drop|keep)::
> +	The command usually errors out when seeing an input e-mail
> +	message that lacks a patch. When this option is set to
> +	'drop', skip such an e-mail message without outputting error.

"without outputting error" -> "instead".

The first sentence used "to error" as a verb to mean "stop the
operation in the middle with a message that diagnoses what is
wrong".  Saying "outputting error" to use the same word as a noun is
not a good idea, especially the use of verb "to output", as it makes
the word "error" look as if it is only the "message" part, and it
becomes unclear if the "stopping due to an error" is also skipped.

I actually think it makes sense to give a notice/info message when
"drop" skips an empty patch, unless "--quiet" is given.

> +	When this option is set to 'keep', create an empty commit,
> +	recording the contents of the e-mail message as its log.
> +	'die' is specified by default.

"is specified by default" sounds a bit strange.  Perhaps lose the
last sentence, and update the first line like so:

    By default, or when the option is set to 'die', the command
    errors out on an input e-mail message that lacks a patch.

> diff --git a/builtin/am.c b/builtin/am.c
> index 8677ea2348a..1a3ed87b445 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 without outputting errors */

I think the user deserves "info:" (unless "--quiet").

> +	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,23 @@ static int am_option_parse_quoted_cr(const struct option *opt,
>  	return 0;
>  }
>  
> +static int am_option_parse_empty_commit(const struct option *opt,
> +				     const char *arg, int unset)
> +{
> +	int *opt_value = opt->value;
> +
> +	if (unset || !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;
> +}

OK.

> @@ -1792,6 +1811,20 @@ static void am_run(struct am_state *state, int resume)
>  		if (state->interactive && do_interactive(state))
>  			goto next;
>  
> +		if (is_empty_or_missing_file(am_path(state, "patch"))) {
> +			if (state->empty_type == DROP_EMPTY_COMMIT)
> +				goto next;
> +			else if (state->empty_type == KEEP_EMPTY_COMMIT) {
> +				if (run_applypatch_msg_hook(state))
> +					exit(1);
> +				else
> +					goto commit;

It probably is easier to read without "else", i.e.

			else if (state->empty_type == KEEP_EMPTY_COMMIT) {
				if (run_applypatch_msg_hook(state))
					exit(1);
				goto commit;

Running hook here and outside this if() block a bit ugly.  Can't we
instead set a flag variable or something and let the existing call
to run_applypatch_msg_hook() used?

> +			} else if (state->empty_type == DIE_EMPTY_COMMIT) {
> +				printf_ln(_("Patch is empty."));
> +				die_user_resolve(state);
> +			}

Also, if developers ever add another possible value for the
state->empty_type member (by the way, "empty" in its name is good,
but "type" is iffy), it would be a bug if they do not consider what
should happen inside this "if (is_empty_or_missing_file())" block,
no?  In such a case, instead of if/else if/... cascade, consider
using switch().  Taking them together.
 
	while (state->cur <= state->last) {
        	const char *mail = am_path(state, msgnum(state));
		int apply_status;
		int to_keep;
		...
		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 empty patch '%.*s",
					linelen(state->msg0, state->msg);
				goto next;
				break;
			case KEEP_EMPTY_COMMIT:
				to_keep = 1;
				break;
			case DIE_EMPTY_COMMIT:
				pritnf_ln(_("Patch is empty."));
				die_user_resolve(state);
				break;
			}
		}

		if (run_applypatch_msg_hook(state))
			exit(1);
		if (to_keep)
			goto commit; /* empty: just record a message */

or something like that.

> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index 2aaaa0d7ded..3119556884d 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -196,6 +196,13 @@ 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 &&
> +
> +	git format-patch --stdout empty-commit^ >empty.patch &&

Unless this is a test for "format-patch" command to make sure that
the above command will not output a single byte, I would rather see
the above line written like this:

	: >empty.patch &&

> +	git format-patch --stdout --cover-letter empty-commit^ >cover-letter.patch &&

This only creates a cover letter without anything, which sort of
looks like an empty commit.

> +	git format-patch --always --stdout empty-commit^ >empty-commit.patch &&

And this gives another empty patch with only a message.

I would sort-of understand if there is

	git format-patch --stdout --always --cover-letter HEAD^ >both.patch

that has two e-mail messages in it, but what's the significance of
testing both cover-letter.patch and empty-commit.patch?

The receiving end would not care what's in the actual e-mail
message, as long as it lacks the patch part, no?  Or am I missing
some subtle reason why we want to test both?

> @@ -1152,4 +1159,70 @@ test_expect_success 'apply binary blob in partial clone' '
>  	git -C client am ../patch
>  '
>  
> +test_expect_success 'still output error with --empty when meeting empty files' '
> +	test_must_fail git am --empty=drop empty.patch 2>actual &&

Nobody will understand "still", unless they know it used to be
unconditionally a failure to feed an e-mail message that lacks a
patch.

test_expect_success 'An empty input file is error regardless of --empty option' '

or something like that?

> +	echo Patch format detection failed. >expected &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'error when meeting e-mail message that lacks a patch by default' '
> +	git checkout empty-commit^ &&
> +	test_must_fail git am empty-commit.patch >err &&
> +	test_path_is_dir .git/rebase-apply &&
> +	test_i18ngrep "Patch is empty." err &&
> +	rm -fr .git/rebase-apply &&

If any command before "rm" fails, it won't be cleaned up.

    test_expect_success 'a message without a patch is an error (default)' '
	test_when_finished "git am --abort || :" &&
	git checkout --detach empty-commit^ &&
	test_must_fail git am empty-commit.patch >err &&
	grep "Patch is empty" err
    '

or something like that.  The title says it tests the default case,
so do not test --empty=die case in the same test.  Use a separate
test to do the same for the case where an explicit "--empty=die"
is given.

As I said, I do not quite see the point of testing with both
empty-commit.patch and cover-letter.patch (unless the latter has an
empty commit and a cover letter, totally in two messages).

> +	test_must_fail git am --empty=die empty-commit.patch >err &&
> +	test_path_is_dir .git/rebase-apply &&
> +	test_i18ngrep "Patch is empty." err &&
> +	rm -fr .git/rebase-apply &&
> +
> +	test_must_fail git am --empty=die cover-letter.patch >err &&
> +	test_path_is_dir .git/rebase-apply &&
> +	test_i18ngrep "Patch is empty." err &&
> +	rm -fr .git/rebase-apply
> +'
> +
> +test_expect_success 'skip without error when meeting e-mail message that lacks a patch' '
> +	git am --empty=drop empty-commit.patch >err &&

The output 'err' is used by nobody; do not redirect it and just
leave the standard output alone.

> +	test_path_is_missing .git/rebase-apply &&

Don't test this condition when testing success or failure from
"am".  That is too much implementation detail.

> +	git rev-parse empty-commit^ >expected &&
> +	git rev-parse HEAD >actual &&
> +	test_cmp expected actual &&

OK, we make sure that HEAD does not move.

> +	git am --empty=drop cover-letter.patch >err &&
> +	test_path_is_missing .git/rebase-apply &&
> +	test_cmp_rev empty-commit^ HEAD

Lose this one?  As I said, I may be missing some subtle reason why
both need to be tested---and if it is the case, test it in a
separate test, not 2 of them in a single test like this.

> +'
> +
> +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="%B" &&
> +		echo "--" &&

Did this test pass?  Without a trailing whitespace after double-dash,
I do not think this shell snippet reproduces a patch e-mail correctly

In any case, don't do this.  Imagine a future developer may want to
improve the patch output by adding other things after the "-- "
signature line, in addition to the "git version".  Or perhaps the
"git version" trailer gets removed by default for privacy concerns.

Should such changes be prevented from being made, only because this
test expects exact output we have today?  No.

Verify what we _care_, e.g. "does the title of the resulting commit
reproduce the one-liner message given to '-m' when the empty commit
was made?" is a sensible question to ask.

> +		git version | sed -e "s/^git version //" &&
> +		echo

> +	} >expected &&
> +	git show HEAD --format="%B" >actual &&
> +	test_cmp actual expected &&
> +
> +	git am --empty=keep cover-letter.patch &&
> +	test_path_is_missing .git/rebase-apply &&
> +	{
> +		echo "*** SUBJECT HERE ***" &&
> +		echo &&
> +		echo "*** BLURB HERE ***" &&
> +		echo &&
> +		echo "A U Thor (1):" &&
> +		printf "  " &&
> +		git show empty-commit --format="%B" &&
> +		echo "--" &&
> +		git version | sed -e "s/^git version //" &&
> +		echo

Don't do this.  Imagine a future developer may want to improve the
cover letter template.  Should that change be prevented from being
made, because this test expects exact output we have today?  No.

> +	} >expected &&
> +	git show HEAD --format="%B" >actual &&
> +	test_cmp actual expected
> +'
> +
>  test_done

Thanks.
Bagas Sanjaya Nov. 19, 2021, 10:33 a.m. UTC | #2
On 18/11/21 17.50, Aleen via GitGitGadget wrote:
> +test_expect_success 'still output error with --empty when meeting empty files' '
> +	test_must_fail git am --empty=drop empty.patch 2>actual &&
> +	echo Patch format detection failed. >expected &&
> +	test_cmp expected actual
> +'

Why isn't the echo string quoted?
Ævar Arnfjörð Bjarmason Nov. 19, 2021, 12:10 p.m. UTC | #3
On Fri, Nov 19 2021, Bagas Sanjaya wrote:

> On 18/11/21 17.50, Aleen via GitGitGadget wrote:
>> +test_expect_success 'still output error with --empty when meeting empty files' '
>> +	test_must_fail git am --empty=drop empty.patch 2>actual &&
>> +	echo Patch format detection failed. >expected &&
>> +	test_cmp expected actual
>> +'
>
> Why isn't the echo string quoted?

There's no need to quote the arguments given to echo in cases like
these.

It's not the same, i.e. it'll get N arguments on argv and not on, but it
makes it easier to spot things that do need quoting, rather than
over-quoting everything.
Eric Sunshine Nov. 19, 2021, 12:20 p.m. UTC | #4
On Fri, Nov 19, 2021 at 7:12 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Fri, Nov 19 2021, Bagas Sanjaya wrote:
> > On 18/11/21 17.50, Aleen via GitGitGadget wrote:
> >> +test_expect_success 'still output error with --empty when meeting empty files' '
> >> +    test_must_fail git am --empty=drop empty.patch 2>actual &&
> >> +    echo Patch format detection failed. >expected &&
> >> +    test_cmp expected actual
> >> +'
> >
> > Why isn't the echo string quoted?
>
> There's no need to quote the arguments given to echo in cases like
> these.
>
> It's not the same, i.e. it'll get N arguments on argv and not on, but it
> makes it easier to spot things that do need quoting, rather than
> over-quoting everything.

I recently expressed an opposing opinion in [1], stating effectively
that omitting the quotes like this is "an accident waiting to happen":

    ... the lack of quotes ... in the `echo ... >expect` statement
    gives me a moment's pause since it relies upon the fact that
    `echo` will insert exactly one space between the ... arguments
    (which happens to match the single space in the [command's output]
    ). For clarity and that extra bit of robustness, I'd probably have
    used a single double-quoted string argument with `echo`.

But, it's a fairly minor objection.

[1]: https://lore.kernel.org/git/CAPig+cQVSUg1aqry_hMydJ=Uo=-VhOog6TUTpG=0on0LUcw8Dg@mail.gmail.com/
Junio C Hamano Nov. 19, 2021, 4:46 p.m. UTC | #5
Bagas Sanjaya <bagasdotme@gmail.com> writes:

> On 18/11/21 17.50, Aleen via GitGitGadget wrote:
>> +test_expect_success 'still output error with --empty when meeting empty files' '
>> +	test_must_fail git am --empty=drop empty.patch 2>actual &&
>> +	echo Patch format detection failed. >expected &&
>> +	test_cmp expected actual
>> +'
>
> Why isn't the echo string quoted?

You may doing so without realizing, but such a "why does the code
not do X" question, especially without stating why you think the
code should do X, gets irritating quickly.

    I think it is better to quote the arguments to echo to make it a
    single string, because ...

would have helped the author of the patch.

If this were one-off, I would have let it go, but I thought it may
be worth commenting, as you seem to often ask this kind of
"questions".
Junio C Hamano Nov. 19, 2021, 4:49 p.m. UTC | #6
Eric Sunshine <sunshine@sunshineco.com> writes:

> I recently expressed an opposing opinion in [1], stating effectively
> that omitting the quotes like this is "an accident waiting to happen":
>
>     ... the lack of quotes ... in the `echo ... >expect` statement
>     gives me a moment's pause since it relies upon the fact that
>     `echo` will insert exactly one space between the ... arguments
>     (which happens to match the single space in the [command's output]
>     ). For clarity and that extra bit of robustness, I'd probably have
>     used a single double-quoted string argument with `echo`.
>
> But, it's a fairly minor objection.

It indeed is minor enough that a patch to turn an existing

	echo A B C >expect &&
	test_cmp expect actual

into

	echo "A B C" >expect &&
	test_cmp expect actual

is not welcome.  But it still is worth pointing out and correcting
in a patch to add new code, I would think.  It all depends on what
we care about, and the use of test_cmp means we do care about exact
shape of the string, including the inter-word spacing.

Thanks.
diff mbox series

Patch

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 0a4a984dfde..665bc89ca9f 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,14 @@  OPTIONS
 --quoted-cr=<action>::
 	This flag will be passed down to 'git mailinfo' (see linkgit:git-mailinfo[1]).
 
+--empty-commit=(die|drop|keep)::
+	The command usually errors out when seeing an input e-mail
+	message that lacks a patch. When this option is set to
+	'drop', skip such an e-mail message without outputting error.
+	When this option is set to 'keep', create an empty commit,
+	recording the contents of the e-mail message as its log.
+	'die' is specified by default.
+
 -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..1a3ed87b445 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 without outputting errors */
+	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,23 @@  static int am_option_parse_quoted_cr(const struct option *opt,
 	return 0;
 }
 
+static int am_option_parse_empty_commit(const struct option *opt,
+				     const char *arg, int unset)
+{
+	int *opt_value = opt->value;
+
+	if (unset || !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 +1272,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);
@@ -1792,6 +1811,20 @@  static void am_run(struct am_state *state, int resume)
 		if (state->interactive && do_interactive(state))
 			goto next;
 
+		if (is_empty_or_missing_file(am_path(state, "patch"))) {
+			if (state->empty_type == DROP_EMPTY_COMMIT)
+				goto next;
+			else if (state->empty_type == KEEP_EMPTY_COMMIT) {
+				if (run_applypatch_msg_hook(state))
+					exit(1);
+				else
+					goto commit;
+			} else if (state->empty_type == DIE_EMPTY_COMMIT) {
+				printf_ln(_("Patch is empty."));
+				die_user_resolve(state);
+			}
+		}
+
 		if (run_applypatch_msg_hook(state))
 			exit(1);
 
@@ -1827,6 +1860,7 @@  static void am_run(struct am_state *state, int resume)
 			die_user_resolve(state);
 		}
 
+commit:
 		do_commit(state);
 
 next:
@@ -2357,6 +2391,10 @@  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) "" },
+		{ OPTION_CALLBACK, 0, "empty", &state.empty_type,
+		  "(die|drop|keep)",
+		  N_("specify how to handle empty patches"),
+		  PARSE_OPT_OPTARG, am_option_parse_empty_commit },
 		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..3119556884d 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -196,6 +196,13 @@  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 &&
+
+	git format-patch --stdout empty-commit^ >empty.patch &&
+	git format-patch --stdout --cover-letter empty-commit^ >cover-letter.patch &&
+	git format-patch --always --stdout empty-commit^ >empty-commit.patch &&
+
 	# reset time
 	sane_unset test_tick &&
 	test_tick
@@ -1152,4 +1159,70 @@  test_expect_success 'apply binary blob in partial clone' '
 	git -C client am ../patch
 '
 
+test_expect_success 'still output error with --empty when meeting empty files' '
+	test_must_fail git am --empty=drop empty.patch 2>actual &&
+	echo Patch format detection failed. >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'error when meeting e-mail message that lacks a patch by default' '
+	git checkout empty-commit^ &&
+	test_must_fail git am empty-commit.patch >err &&
+	test_path_is_dir .git/rebase-apply &&
+	test_i18ngrep "Patch is empty." err &&
+	rm -fr .git/rebase-apply &&
+
+	test_must_fail git am --empty=die empty-commit.patch >err &&
+	test_path_is_dir .git/rebase-apply &&
+	test_i18ngrep "Patch is empty." err &&
+	rm -fr .git/rebase-apply &&
+
+	test_must_fail git am --empty=die cover-letter.patch >err &&
+	test_path_is_dir .git/rebase-apply &&
+	test_i18ngrep "Patch is empty." err &&
+	rm -fr .git/rebase-apply
+'
+
+test_expect_success 'skip without error when meeting e-mail message that lacks a patch' '
+	git am --empty=drop empty-commit.patch >err &&
+	test_path_is_missing .git/rebase-apply &&
+	git rev-parse empty-commit^ >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual &&
+
+	git am --empty=drop cover-letter.patch >err &&
+	test_path_is_missing .git/rebase-apply &&
+	test_cmp_rev empty-commit^ HEAD
+'
+
+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="%B" &&
+		echo "--" &&
+		git version | sed -e "s/^git version //" &&
+		echo
+	} >expected &&
+	git show HEAD --format="%B" >actual &&
+	test_cmp actual expected &&
+
+	git am --empty=keep cover-letter.patch &&
+	test_path_is_missing .git/rebase-apply &&
+	{
+		echo "*** SUBJECT HERE ***" &&
+		echo &&
+		echo "*** BLURB HERE ***" &&
+		echo &&
+		echo "A U Thor (1):" &&
+		printf "  " &&
+		git show empty-commit --format="%B" &&
+		echo "--" &&
+		git version | sed -e "s/^git version //" &&
+		echo
+	} >expected &&
+	git show HEAD --format="%B" >actual &&
+	test_cmp actual expected
+'
+
 test_done