[v14,3/3] am: support --allow-empty to record specific empty patches

Commit Message

徐沛文 (Aleen) Dec. 1, 2021, 3:37 a.m. UTC

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

This option helps to record specific empty patches in the middle
of an am session.

Signed-off-by: 徐沛文 (Aleen) <[e-mail]>
---
 Documentation/git-am.txt |  7 ++++++-
 builtin/am.c             | 29 ++++++++++++++++++++---------
 t/t4150-am.sh            | 24 ++++++++++++++++++++++++
 t/t7512-status-help.sh   |  1 +
 wt-status.c              |  3 +++
 5 files changed, 54 insertions(+), 10 deletions(-)

Comments

Junio C Hamano Dec. 2, 2021, 12:58 a.m. UTC | #1

""徐沛文 (Aleen)" via GitGitGadget"  <[e-mail]>
writes:

> +test_expect_success 'skip an empty patch in the middle of an am session' '
> +	git checkout empty-commit^ &&
> +	test_must_fail git am empty-commit.patch >err &&
> +	grep "Patch is empty." err &&
> +	grep "If you want to record it as an empty commit, run \"git am --allow-empty\"." err &&
> +	git am --skip &&
> +	test_path_is_missing .git/rebase-apply &&
> +	git rev-parse empty-commit^ >expected &&
> +	git rev-parse HEAD >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'record an empty patch as an empty commit in the middle of an am session' '
> +	git checkout empty-commit^ &&
> +	test_must_fail git am empty-commit.patch >err &&
> +	grep "Patch is empty." err &&
> +	grep "If you want to record it as an empty commit, run \"git am --allow-empty\"." err &&
> +	git am --allow-empty &&
> +	test_path_is_missing .git/rebase-apply &&
> +	git show empty-commit --format="%s" >expected &&
> +	git show HEAD --format="%s" >actual &&
> +	test_cmp actual expected
> +'

These two are "positive" tests, i.e. the feature does the right
thing when used in the expected situation.

I am worried about the cases where "--allow-empty" is used and
creates a commit that is empty, when it is reasonably expected to
either create a non-empty commit or still fail the same way to give
the user another chance to try.

For example, when "git am -3" fails on such an email message (there
are two failure modes: (a) one results in an unmerged index due to
merge conflicts, (b) the other results in a clean index due to not
finding the blob object recorded as a preimage), what happens when
the user runs "git am --allow-empty", after:

 (1) doing nothing since the failed "git am -3" application,
 (2) resolving the conflicts in working tree, or
 (3) resolving the conflicts in working tree and recording the
     resolution in the index.

There are 6 cases in the above matrix, and "git am --allow-empty"
should not create an empty commit in any one of them.  In 1a and 1b,
"git am --allow-empty" should continue to fail ("git am --skip"
would be a way to ignore and proceed).  3a and 3b should record a
non-empty commit as the result.

Similarly, when an email message with a patch is given to "git am"
and the application fails, and the user runs "git am --allow-empty"
without doing anything in the working tree, what happens?  It should
not happily create an empty commit with only the log message,
without the change, but continue to fail.  If the user resolves and
registers the resolution to the index before "git am --allow-empty",
then the command should create a non-empty commit.

Having positive tests are of course important to avoid regression in
the future, but negative tests, the new and shiny feature does not
misbehave when it shouldn't even kick in, is even more important.

Thanks.

Aleen 徐沛文 Dec. 6, 2021, 1:35 a.m. UTC | #2

Dears Hamano,

    For those 6 cases, it seems `git-am` should only work when the am session
    was stopped in the middle according to an empty patch, but not when the index
    has not changed. Does that mean we should record a new state for the am state?
    If that's right, I will try to patch it via version 15, and try to add additional
    test cases for these 6 situations. I was a little busy those days and sorry for
    the late answer.

Aleen

> These two are "positive" tests, i.e. the feature does the right
> thing when used in the expected situation.
> 
> I am worried about the cases where "--allow-empty" is used and
> creates a commit that is empty, when it is reasonably expected to
> either create a non-empty commit or still fail the same way to give
> the user another chance to try.
> 
> For example, when "git am -3" fails on such an email message (there
> are two failure modes: (a) one results in an unmerged index due to
> merge conflicts, (b) the other results in a clean index due to not
> finding the blob object recorded as a preimage), what happens when
> the user runs "git am --allow-empty", after:
> 
>  (1) doing nothing since the failed "git am -3" application,
>  (2) resolving the conflicts in working tree, or
>  (3) resolving the conflicts in working tree and recording the
>      resolution in the index.
> 
> There are 6 cases in the above matrix, and "git am --allow-empty"
> should not create an empty commit in any one of them.  In 1a and 1b,
> "git am --allow-empty" should continue to fail ("git am --skip"
> would be a way to ignore and proceed).  3a and 3b should record a
> non-empty commit as the result.
> 
> Similarly, when an email message with a patch is given to "git am"
> and the application fails, and the user runs "git am --allow-empty"
> without doing anything in the working tree, what happens?  It should
> not happily create an empty commit with only the log message,
> without the change, but continue to fail.  If the user resolves and
> registers the resolution to the index before "git am --allow-empty",
> then the command should create a non-empty commit.
> 
> Having positive tests are of course important to avoid regression in
> the future, but negative tests, the new and shiny feature does not
> misbehave when it shouldn't even kick in, is even more important.
> 
> Thanks.

Patch

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index cf9cace9678..b6c065b3f7a 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
 -----------
@@ -200,6 +200,11 @@  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::
+	After a patch failure on an input e-mail message lacking a patch,
+	the user can still record 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..58c7638b9bc 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 record it as an empty commit, run \"git am --allow-empty\"."));
 				die_user_resolve(state);
 				break;
 			}
@@ -1898,19 +1899,24 @@  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 when there is no changes.
  */
-static void am_resolve(struct am_state *state)
+static void am_resolve(struct am_state *state, int allow_empty)
 {
 	validate_resume_state(state);
 
 	say(state, stdout, _("Applying: %.*s"), linelen(state->msg), state->msg);
 
 	if (!repo_index_has_changes(the_repository, NULL, NULL)) {
-		printf_ln(_("No changes - did you forget to use 'git add'?\n"
-			"If there is nothing left to stage, chances are that something else\n"
-			"already introduced the same changes; you might want to skip this patch."));
-		die_user_resolve(state);
+		if (allow_empty)
+			printf_ln(_("No changes - record it as an empty commit."));
+		else {
+			printf_ln(_("No changes - did you forget to use 'git add'?\n"
+				    "If there is nothing left to stage, chances are that something else\n"
+				    "already introduced the same changes; you might want to skip this patch."));
+			die_user_resolve(state);
+		}
 	}
 
 	if (unmerged_cache()) {
@@ -2237,7 +2243,8 @@  enum resume_type {
 	RESUME_SKIP,
 	RESUME_ABORT,
 	RESUME_QUIT,
-	RESUME_SHOW_PATCH
+	RESUME_SHOW_PATCH,
+	RESUME_ALLOW_EMPTY
 };
 
 struct resume_mode {
@@ -2390,6 +2397,9 @@  int cmd_am(int argc, const char **argv, const char *prefix)
 		  N_("show the patch being applied"),
 		  PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
 		  parse_opt_show_current_patch, RESUME_SHOW_PATCH },
+		OPT_CMDMODE(0, "allow-empty", &resume.mode,
+			N_("record the empty patch as an empty commit"),
+			RESUME_ALLOW_EMPTY),
 		OPT_BOOL(0, "committer-date-is-author-date",
 			&state.committer_date_is_author_date,
 			N_("lie about committer date")),
@@ -2498,7 +2508,8 @@  int cmd_am(int argc, const char **argv, const char *prefix)
 		am_run(&state, 1);
 		break;
 	case RESUME_RESOLVED:
-		am_resolve(&state);
+	case RESUME_ALLOW_EMPTY:
+		am_resolve(&state, resume.mode == RESUME_ALLOW_EMPTY ? 1 : 0);
 		break;
 	case RESUME_SKIP:
 		am_skip(&state);
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 8c8bd4db220..3e60460022f 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -1201,4 +1201,28 @@  test_expect_success 'record as an empty commit when meeting e-mail message that
 	test_cmp actual expected
 '
 
+test_expect_success 'skip an empty patch in the middle of an am session' '
+	git checkout empty-commit^ &&
+	test_must_fail git am empty-commit.patch >err &&
+	grep "Patch is empty." err &&
+	grep "If you want to record it as an empty commit, run \"git am --allow-empty\"." err &&
+	git am --skip &&
+	test_path_is_missing .git/rebase-apply &&
+	git rev-parse empty-commit^ >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'record an empty patch as an empty commit in the middle of an am session' '
+	git checkout empty-commit^ &&
+	test_must_fail git am empty-commit.patch >err &&
+	grep "Patch is empty." err &&
+	grep "If you want to record it as an empty commit, run \"git am --allow-empty\"." err &&
+	git am --allow-empty &&
+	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
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 7f2956d77ad..9309becfe03 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -658,6 +658,7 @@  test_expect_success 'status in an am session: empty patch' '
 On branch am_empty
 You are in the middle of an am session.
 The current patch is empty.
+  (use "git am --allow-empty" to record this patch as an empty commit)
   (use "git am --skip" to skip this patch)
   (use "git am --abort" to restore the original branch)
 
diff --git a/wt-status.c b/wt-status.c
index 5d215f4e4f1..d578a0e9192 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1227,6 +1227,9 @@  static void show_am_in_progress(struct wt_status *s,
 		if (!s->state.am_empty_patch)
 			status_printf_ln(s, color,
 				_("  (fix conflicts and then run \"git am --continue\")"));
+		else
+			status_printf_ln(s, color,
+				_("  (use \"git am --allow-empty\" to record this patch as an empty commit)"));
 		status_printf_ln(s, color,
 			_("  (use \"git am --skip\" to skip this patch)"));
 		status_printf_ln(s, color,