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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 7, 2021, 18:12 UTC
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.

Show 5 quoted lines

> +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 */

Show 8 quoted lines

>  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".

Show 21 quoted lines

> @@ -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.

Show 23 quoted lines

> +				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:

Show 5 quoted lines

> +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.