Re: [PATCH v2 1/8] fetch: split out tests for output format

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Apr 29, 2023, 17:34 UTC
Message-ID: <20230429173448.GD3271@szeder.dev>
In-Reply-To: <0d0d50d14c557f8313747e7d0e104c2c0819dab9.1682593865.git.ps@pks.im>

On Thu, Apr 27, 2023 at 01:13:08PM +0200, Patrick Steinhardt wrote:

Show 16 quoted lines

> We're about to introduce a new porcelain mode for the output of
> git-fetch(1). As part of that we'll be introducing a set of new tests
> that only relate to the output of this command.
> 
> Split out tests that exercise the output format of git-fetch(1) so that
> it becomes easier to verify this functionality as a standalone unit. As
> the tests assume that the default branch is called "main" we set up the
> corresponding GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME environment variable
> accordingly.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t5510-fetch.sh        | 53 ----------------------------------
>  t/t5574-fetch-output.sh | 63 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+), 53 deletions(-)
>  create mode 100755 t/t5574-fetch-output.sh

Show 21 quoted lines

> diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
> new file mode 100755
> index 0000000000..f91b654d38
> --- /dev/null
> +++ b/t/t5574-fetch-output.sh
> @@ -0,0 +1,63 @@
> +#!/bin/sh
> +
> +test_description='git fetch output format'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'fetch aligned output' '
> +	git clone . full-output &&
> +	test_commit looooooooooooong-tag &&
> +	(
> +		cd full-output &&
> +		git -c fetch.output=full fetch origin >actual 2>&1 &&

Why is that 2>&1 redirection used here? If the output the test case is interested in goes to the command's standard output, then it's unnecessary. However, if it goes to standard error, then why is standard output redirected as well?

I understand that this patch just moves existing test cases around as-is, so this is not something you introduced, but I point it out here, because later patches of this series add several new test cases following this anti-pattern.

Since this series creates a new test script, perhaps this might be the right time to clean this up.

Show 38 quoted lines

> +		grep -e "->" actual | cut -c 22- >../actual
> +	) &&
> +	cat >expect <<-\EOF &&
> +	main                 -> origin/main
> +	looooooooooooong-tag -> looooooooooooong-tag
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'fetch compact output' '
> +	git clone . compact &&
> +	test_commit extraaa &&
> +	(
> +		cd compact &&
> +		git -c fetch.output=compact fetch origin >actual 2>&1 &&
> +		grep -e "->" actual | cut -c 22- >../actual
> +	) &&
> +	cat >expect <<-\EOF &&
> +	main       -> origin/*
> +	extraaa    -> *
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '--no-show-forced-updates' '
> +	mkdir forced-updates &&
> +	(
> +		cd forced-updates &&
> +		git init &&
> +		test_commit 1 &&
> +		test_commit 2
> +	) &&
> +	git clone forced-updates forced-update-clone &&
> +	git clone forced-updates no-forced-update-clone &&
> +	git -C forced-updates reset --hard HEAD~1 &&
> +	(
> +		cd forced-update-clone &&
> +		git fetch --show-forced-updates origin 2>output &&

Oh, look, there are some good examples to follow here as well :)

Show 13 quoted lines

> +		test_i18ngrep "(forced update)" output
> +	) &&
> +	(
> +		cd no-forced-update-clone &&
> +		git fetch --no-show-forced-updates origin 2>output &&
> +		test_i18ngrep ! "(forced update)" output
> +	)
> +'
> +
> +test_done
> -- 
> 2.40.1
>