From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 07 Dec 2021 18:12:37 GMT
Subject: Re: [PATCH v17 2/3] am: support --empty=<option> to handle empty patches
Message-ID: <xmqq4k7k9td6.fsf@gitster.g>
In-Reply-To: <b9e03f2342b3fc52515f063f28b34ad9e1dc338a.1638865913.git.gitgitgadget@gmail.com>

""徐沛文 (Aleen)" via GitGitGadget"  <gitgitgadget@gmail.com>
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.