From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 19 Nov 2021 23:07:03 GMT
Subject: Re: [PATCH v7 2/2] am: support --empty=<option> to handle empty patches
Message-ID: <xmqqpmqvoico.fsf@gitster.g>
In-Reply-To: <9e60e77c041bca28a50d706c865eb776e5fe2ec2.1637298299.git.gitgitgadget@gmail.com>

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

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

Titles of all the other tests seem to begin with lowercase, so "An
empty" -> "an empty", probably.

> +	test_must_fail git am --empty=drop empty.patch 2>actual &&
> +	echo Patch format detection failed. >expected &&

Quote for exactness, like the next test does.

    echo "Patch format detection failed." >expected

> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'invalid when passing the --empty option alone' '
> +	git checkout empty-commit^ &&
> +	test_must_fail git am --empty empty-commit.patch 2>err &&
> +	echo "error: Invalid value for --empty: empty-commit.patch" >expected &&
> +	test_cmp expected err
> +'

This mode of failure of "am" may not leave ".git/rebase-apply"
behind right now, or it may leave one in the future.  We do not want
to worry about left-over cruft to interfere with the next test, so
cleaning after ourselves with test_when_finished would be a good
idea, i.e.

    test_expect_success 'invalid when passing the --empty option alone' '
            test_when_finished "git am --abort || :" &&
            git checkout empty-commit^ &&
            test_must_fail git am --empty empty-commit.patch 2>err &&
            echo "error: Invalid value for --empty: empty-commit.patch" >expected &&
            test_cmp expected err
    '

> +test_expect_success 'a message without a patch is an error (default)' '
> +	test_when_finished "git am --abort || :" &&
> +	test_must_fail git am empty-commit.patch >err &&
> +	grep "Patch is empty" err &&
> +	rm -fr .git/rebase-apply
> +'

And the point of test_when_finished is to run the clean-up even when
other steps in the test fails.  For example, "test_must_fail git am"
may fail to fail for any reason.  Or "grep" after it may fail.
Because the pieces in a single test is strung together with &&, any
such failure means the control would NOT reach "rm -fr".  

Since we have test_when_finished that cleans up after ourselves even
in such a case, the last "rm -fr" step is unnecessary.

> +test_expect_success 'a message without a patch is an error where an explicit "--empty=die" is given' '
> +	test_when_finished "git am --abort || :" &&
> +	test_must_fail git am --empty=die empty-commit.patch >err &&
> +	grep "Patch is empty." err &&
> +	rm -fr .git/rebase-apply
> +'

Likewise.

Other than that (and po/ that should not be part of this patch),
things are looking good.

Thanks.