Subject: Re: [PATCH v4 3/8] fetch: add a test to exercise invalid output formats

From: Junio C Hamano <[email-masked]>
Date: Tue, 09 May 2023 10:58:33 -0700

Patrick Steinhardt <[email-masked]> writes:

> Add a testcase that exercises the logic when an invalid output format is
> passed via the `fetch.output` configuration.
>
> Signed-off-by: Patrick Steinhardt <[email-masked]>
> ---
>  t/t5574-fetch-output.sh | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

It makes perfect sense to make sure that invalid input gets rejected
and the command exits with non-zero status, and it is probably a
good thing that the end-user sees a message that explains why the
particular input is rejected (even though it adds one more thing
that needs to be updated when the message gets reworded).

But do we need to insist on no output to the standard output stream
when the command errors out?

Other than that, looking good.

THanks.

> diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
> index f91b654d38..a09750d225 100755
> --- a/t/t5574-fetch-output.sh
> +++ b/t/t5574-fetch-output.sh
> @@ -7,6 +7,25 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>  
>  . ./test-lib.sh
>  
> +test_expect_success 'fetch with invalid output format configuration' '
> +	test_when_finished "rm -rf clone" &&
> +	git clone . clone &&
> +
> +	test_must_fail git -C clone -c fetch.output= fetch origin >actual.out 2>actual.err &&
> +	cat >expect <<-EOF &&
> +	fatal: invalid value for ${SQ}fetch.output${SQ}: ${SQ}${SQ}
> +	EOF
> +	test_must_be_empty actual.out &&
> +	test_cmp expect actual.err &&
> +
> +	test_must_fail git -C clone -c fetch.output=garbage fetch origin >actual.out 2>actual.err &&
> +	cat >expect <<-EOF &&
> +	fatal: invalid value for ${SQ}fetch.output${SQ}: ${SQ}garbage${SQ}
> +	EOF
> +	test_must_be_empty actual.out &&
> +	test_cmp expect actual.err
> +'
> +
>  test_expect_success 'fetch aligned output' '
>  	git clone . full-output &&
>  	test_commit looooooooooooong-tag &&