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.