Re: [PATCH v6 2/3] am: support --empty option to handle empty patches

"Aleen via GitGitGadget" <gitgitgadget@gmail.com> writes:

Here is a place to explain to readers what it exactly means to "support --empty option to handle empty patches". You know what you meant, I happen to know what you meant because I haven't forgotten yet, but we are writing for developers in the future who need to fix bugs in 6 months down the road to decide if this commit is involved in the bug in "git am" they are hunting.

"without outputting error" -> "instead".

The first sentence used "to error" as a verb to mean "stop the operation in the middle with a message that diagnoses what is wrong". Saying "outputting error" to use the same word as a noun is not a good idea, especially the use of verb "to output", as it makes the word "error" look as if it is only the "message" part, and it becomes unclear if the "stopping due to an error" is also skipped.

I actually think it makes sense to give a notice/info message when "drop" skips an empty patch, unless "--quiet" is given.

"is specified by default" sounds a bit strange. Perhaps lose the last sentence, and update the first line like so:

    By default, or when the option is set to 'die', the command
    errors out on an input e-mail message that lacks a patch.

I think the user deserves "info:" (unless "--quiet").

OK.

It probably is easier to read without "else", i.e.

			else if (state->empty_type == KEEP_EMPTY_COMMIT) {
				if (run_applypatch_msg_hook(state))
					exit(1);
				goto commit;

Running hook here and outside this if() block a bit ugly. Can't we instead set a flag variable or something and let the existing call to run_applypatch_msg_hook() used?

Also, if developers ever add another possible value for the
state->empty_type member (by the way, "empty" in its name is good,
but "type" is iffy), it would be a bug if they do not consider what
should happen inside this "if (is_empty_or_missing_file())" block,
no?  In such a case, instead of if/else if/... cascade, consider
using switch().  Taking them together.
 
	while (state->cur <= state->last) {
        	const char *mail = am_path(state, msgnum(state));
		int apply_status;
		int to_keep;
		...
		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 empty patch '%.*s",
					linelen(state->msg0, state->msg);
				goto next;
				break;
			case KEEP_EMPTY_COMMIT:
				to_keep = 1;
				break;
			case DIE_EMPTY_COMMIT:
				pritnf_ln(_("Patch is empty."));
				die_user_resolve(state);
				break;
			}
		}

		if (run_applypatch_msg_hook(state))
			exit(1);
		if (to_keep)
			goto commit; /* empty: just record a message */

or something like that.

Unless this is a test for "format-patch" command to make sure that the above command will not output a single byte, I would rather see the above line written like this:

	: >empty.patch &&

This only creates a cover letter without anything, which sort of looks like an empty commit.

And this gives another empty patch with only a message.

I would sort-of understand if there is

	git format-patch --stdout --always --cover-letter HEAD^ >both.patch

that has two e-mail messages in it, but what's the significance of testing both cover-letter.patch and empty-commit.patch?

The receiving end would not care what's in the actual e-mail message, as long as it lacks the patch part, no? Or am I missing some subtle reason why we want to test both?

Nobody will understand "still", unless they know it used to be unconditionally a failure to feed an e-mail message that lacks a patch.

test_expect_success 'An empty input file is error regardless of --empty option' '

or something like that?

If any command before "rm" fails, it won't be cleaned up.

    test_expect_success 'a message without a patch is an error (default)' '
	test_when_finished "git am --abort || :" &&
	git checkout --detach empty-commit^ &&
	test_must_fail git am empty-commit.patch >err &&
	grep "Patch is empty" err
    '

or something like that. The title says it tests the default case, so do not test --empty=die case in the same test. Use a separate test to do the same for the case where an explicit "--empty=die" is given.

As I said, I do not quite see the point of testing with both empty-commit.patch and cover-letter.patch (unless the latter has an empty commit and a cover letter, totally in two messages).

The output 'err' is used by nobody; do not redirect it and just leave the standard output alone.

Don't test this condition when testing success or failure from "am". That is too much implementation detail.

OK, we make sure that HEAD does not move.

Lose this one? As I said, I may be missing some subtle reason why both need to be tested---and if it is the case, test it in a separate test, not 2 of them in a single test like this.

Did this test pass? Without a trailing whitespace after double-dash, I do not think this shell snippet reproduces a patch e-mail correctly

In any case, don't do this. Imagine a future developer may want to improve the patch output by adding other things after the "-- " signature line, in addition to the "git version". Or perhaps the "git version" trailer gets removed by default for privacy concerns.

Should such changes be prevented from being made, only because this test expects exact output we have today? No.

Verify what we _care_, e.g. "does the title of the resulting commit reproduce the one-liner message given to '-m' when the empty commit was made?" is a sensible question to ask.

Don't do this. Imagine a future developer may want to improve the cover letter template. Should that change be prevented from being made, because this test expects exact output we have today? No.

Thanks.