""徐沛文 (Aleen)" via GitGitGadget" <gitgitgadget@xxxxxxxxx>
writes:
This step look mostly good and well done, except for just a few
things that remain.
> +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 */
> 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".
> @@ -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.
> + 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:
> +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.
