Thread (97 messages) 97 messages, 11 authors, 2021-12-11

Re: [PATCH v17 2/3] am: support --empty=<option> to handle empty patches

From: Junio C Hamano <hidden>
Date: 2021-12-07 18:12:44

""徐沛文 (Aleen)" via GitGitGadget"  [off-list ref]
writes:


This step look mostly good and well done, except for just a few
things that remain.
quoted hunk ↗ jump to hunk
+enum empty_action {
+	STOP_ON_EMPTY_COMMIT = 0,  /* output errors and stop in the middle of an am session */
+	DROP_EMPTY_COMMIT,         /* skip with a notice message, unless "--quiet" has been passed */
+	KEEP_EMPTY_COMMIT          /* keep recording as empty commits */
+};
It is friendly to future developers to end the last item in enum
with a comma, unless the current last item MUST stay to be the last
one even when they add new ones.  I.e.

	KEEP_EMPTY_COMMIT,          /* keep recording as empty commits */
quoted hunk ↗ jump to hunk
 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 */
Mental note.  After this series graduates to 'master', at some point
in the future, we should clean these members up to be of their
respective enum types, not "int".
quoted hunk ↗ jump to hunk
@@ -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;
This causes the code that produces the "Applying" message jumped
over, so the user will not see anything done for this step.  

I think we want to mimic the above case arm and do something like

				say(state, stdout, _("Creating an empty commit: %.*s"),
					linelen(state->msg), state->msg);

to avoid being mum about what was done in this step.
quoted hunk ↗ jump to hunk
+				break;
+			case STOP_ON_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:
quoted hunk ↗ jump to hunk
+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 &&
For the test data prepared by the earlier part of this patch, this
does not make a difference, but by using %B instead of %s, I think
you can catch a future bug that only keeps the subject intact while
munging the body of the message.

Other than these, looking quite good.

Thanks.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help