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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 19, 2021, 23:07 UTC
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

Show 9 quoted lines

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

Show 6 quoted lines

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

Show 6 quoted lines

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