[v4,2/2] am: support --empty-commit option to handle empty patches

Commit Message

徐沛文 (Aleen) Nov. 16, 2021, 5:18 a.m. UTC

From: Aleen <[e-mail]>

Signed-off-by: Aleen <[e-mail]>
---
 Documentation/git-am.txt |  9 +++++
 builtin/am.c             | 48 +++++++++++++++++++++++---
 t/t4150-am.sh            | 73 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 125 insertions(+), 5 deletions(-)

Comments

Phillip Wood Nov. 16, 2021, 10:07 a.m. UTC | #1

Hi Aleen

Thanks for working on this

On 16/11/2021 05:18, Aleen via GitGitGadget wrote:
> From: Aleen <[e-mail]>
> 
> Signed-off-by: Aleen <[e-mail]>
> ---
>   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..d8d3bf202d7 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-commit=(die|skip|asis)]
>   	 [(<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|skip|asis)::
> +	The command usually errors out when seeing an input e-mail
> +	message that lacks a patch. When this option is set to
> +	'skip', skip such an e-mail message without outputting error.
> +	When this option is set to 'asis', create an empty commit,
> +	recording the contents of the e-mail message as its log.
> +	'die' is specified by default.

This feels sufficiently similar to the case of handling empty commits in 
'git rebase' that it is worth trying to have a similar user interface. 
Otherwise the two commands have two different option names doing more or 
less the same thing. 'git rebase' has --empty=[drop,keep,ask] where drop 
is the default. If am were to accept --empty=[drop,keep,die] it would 
offer a similar user experience.

Best Wishes

Phillip

>   -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..e7755c1377e 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -87,6 +87,12 @@ enum show_patch_type {
>   	SHOW_PATCH_DIFF = 1,
>   };
>   
> +enum empty_commit_action {
> +	DIE_EMPTY_COMMIT = 0,  /* output errors */
> +	SKIP_EMPTY_COMMIT,     /* skip without outputting errors */
> +	ASIS_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_commit; /* enum empty_commit_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, "skip"))
> +		*opt_value = SKIP_EMPTY_COMMIT;
> +	else if (!strcmp(arg, "asis"))
> +		*opt_value = ASIS_EMPTY_COMMIT;
> +	else
> +		return error(_("Invalid value for --empty-commit: %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_commit == SKIP_EMPTY_COMMIT)
> +				goto next;
> +			else if (state->empty_commit == ASIS_EMPTY_COMMIT) {
> +				if (run_applypatch_msg_hook(state))
> +					exit(1);
> +				else
> +					goto commit;
> +			} else if (state->empty_commit == 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-commit", &state.empty_commit,
> +		  "(die|skip|asis)",
> +		  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..e657180c201 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-commit when meeting empty files' '
> +	test_must_fail git am --empty-commit=skip 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-commit=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-commit=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-commit=skip 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-commit=skip 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-commit=asis 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-commit=asis 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
>

Aleen 徐沛文 Nov. 16, 2021, 10:31 a.m. UTC | #2

> > +--empty-commit=(die|skip|asis)::
> > +	The command usually errors out when seeing an input e-mail
> > +	message that lacks a patch. When this option is set to
> > +	'skip', skip such an e-mail message without outputting error.
> > +	When this option is set to 'asis', create an empty commit,
> > +	recording the contents of the e-mail message as its log.
> > +	'die' is specified by default.
> 
> This feels sufficiently similar to the case of handling empty commits in 
> 'git rebase' that it is worth trying to have a similar user interface. 
> Otherwise the two commands have two different option names doing more or 
> less the same thing. 'git rebase' has --empty=[drop,keep,ask] where drop 
> is the default. If am were to accept --empty=[drop,keep,die] it would 
> offer a similar user experience.
> 
> Best Wishes
> 
> Phillip

Dears Phillip,

It seems a good idea. Can Hamano make a decision?

Aleen

Junio C Hamano Nov. 17, 2021, 8:39 a.m. UTC | #3

Phillip Wood <[e-mail]> writes:

>>   +--empty-commit=(die|skip|asis)::
>> +	The command usually errors out when seeing an input e-mail
>> +	message that lacks a patch. When this option is set to
>> +	'skip', skip such an e-mail message without outputting error.
>> +	When this option is set to 'asis', create an empty commit,
>> +	recording the contents of the e-mail message as its log.
>> +	'die' is specified by default.
>
> This feels sufficiently similar to the case of handling empty commits
> in 'git rebase' that it is worth trying to have a similar user
> interface. Otherwise the two commands have two different option names
> doing more or less the same thing. 'git rebase' has
> --empty=[drop,keep,ask] where drop is the default. If am were to
>  accept --empty=[drop,keep,die] it would offer a similar user
> experience.

Ah, thanks for noticing.  I like the three words you suggest.  If we
already have a similar option, we definitely should follow suit, and
I think "--empty" would be a better fit than "--empty-commit" in the
context of talking about the _input_ to "am".

Patch

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 0a4a984dfde..d8d3bf202d7 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-commit=(die|skip|asis)]
 	 [(<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|skip|asis)::
+	The command usually errors out when seeing an input e-mail
+	message that lacks a patch. When this option is set to
+	'skip', skip such an e-mail message without outputting error.
+	When this option is set to 'asis', 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..e7755c1377e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -87,6 +87,12 @@  enum show_patch_type {
 	SHOW_PATCH_DIFF = 1,
 };
 
+enum empty_commit_action {
+	DIE_EMPTY_COMMIT = 0,  /* output errors */
+	SKIP_EMPTY_COMMIT,     /* skip without outputting errors */
+	ASIS_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_commit; /* enum empty_commit_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, "skip"))
+		*opt_value = SKIP_EMPTY_COMMIT;
+	else if (!strcmp(arg, "asis"))
+		*opt_value = ASIS_EMPTY_COMMIT;
+	else
+		return error(_("Invalid value for --empty-commit: %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_commit == SKIP_EMPTY_COMMIT)
+				goto next;
+			else if (state->empty_commit == ASIS_EMPTY_COMMIT) {
+				if (run_applypatch_msg_hook(state))
+					exit(1);
+				else
+					goto commit;
+			} else if (state->empty_commit == 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-commit", &state.empty_commit,
+		  "(die|skip|asis)",
+		  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..e657180c201 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-commit when meeting empty files' '
+	test_must_fail git am --empty-commit=skip 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-commit=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-commit=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-commit=skip 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-commit=skip 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-commit=asis 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-commit=asis 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