Re: [PATCH v4 3/6] tests: don't lose exit status with "(cd ...; test <op> $(git ...))"

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 20, 2022, 00:20 UTC
Message-ID: <xmqqsfhb0vum.fsf@gitster.g>
In-Reply-To: <patch-v4-3.6-f5b2489609c-20221219T101240Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> -	test "a" = "$(cd repo && test-tool path-utils prefix_path prefix "$(pwd)/../repolink/a")"
> +	echo "a" >expect &&
> +	test-tool -C repo path-utils prefix_path prefix "$(cd repo && pwd)/../repolink/a" >actual &&

If we fail to cd to 'repo', "$(cd repo && pwd)/../repolink/a" would silently expand to nonsense, but presumably "test-tool -C repo" would fail loudly in such a case, so we should be OK here?

Show 12 quoted lines

> @@ -28,9 +32,10 @@ test_expect_success 'preparing origin repository' '
>  
>  test_expect_success 'local clone without .git suffix' '
>  	git clone -l -s a b &&
> -	(cd b &&
> -	test "$(git config --bool core.bare)" = false &&
> -	git fetch)
> +	echo false >expect &&
> +	git -C b config --bool core.bare >actual &&
> +	test_cmp expect actual &&
> +	git -C b fetch
>  '

I am not sure if the above with full of "git -C" is strictly an improvement over

	(
		cd b &&
		echo false >expect &&
		git config --bool core.bare >actual &&
		test_cmp expect actual &&
		git fetch
	)

and even if it were, the reason why it is better would be vastly different from the reason why it is better that we no longer do "test $(cmd) = false". I very much hate the pattern described on the commit title of this step (which by definition this patch fixes many instances of). I.e.

	(cd ...; test <op> $(git ...))

might be something you find that needs improvements, but "don't lose exit status" ONLY applies to the "test <op> $(git ...)" part, and the other half, i.e. (cd ...), has nothing to do with "don't lose exit status" badness.