Re: [PATCH] t5510-fetch: upgrade to a more modern style

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 1, 2022, 20:11 UTC
Message-ID: <xmqqo81kva1j.fsf@gitster.g>
In-Reply-To: <20220331175412.305968-1-gitter.spiros@gmail.com>

Elia Pinto <gitter.spiros@gmail.com> writes:

Show 41 quoted lines

> Clean up the code style so all the tests, and not just a few,
> that chdir around isolate themselves in a subshell.
>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> this patch was inspired by a Junio #leftoverbit
> https://lore.kernel.org/git/xmqqmtjh0x5f.fsf@gitster.g/
>  t/t5510-fetch.sh | 927 ++++++++++++++++++++++++-----------------------
>  1 file changed, 477 insertions(+), 450 deletions(-)
>
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 6f38a69fbb..d0b249d276 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -48,342 +48,349 @@ test_expect_success "clone and setup child repos" '
>  '
>  
>  test_expect_success "fetch test" '
> -	cd "$D" &&
> -	echo >file updated by origin &&
> -	git commit -a -m "updated by origin" &&
> -	cd two &&
> -	git fetch &&
> -	git rev-parse --verify refs/heads/one &&
> -	mine=$(git rev-parse refs/heads/one) &&
> -	his=$(cd ../one && git rev-parse refs/heads/main) &&
> -	test "z$mine" = "z$his"
> +	(
> +		cd "$D" &&
> +		echo >file updated by origin &&
> +		git commit -a -m "updated by origin" &&
> +		(
> +			cd two &&
> +			git fetch &&
> +			git rev-parse --verify refs/heads/one &&
> +			mine=$(git rev-parse refs/heads/one) &&
> +			his=$(cd ../one && git rev-parse refs/heads/main) &&
> +			test "z$mine" = "z$his"
> +		)
> +	)
>  '

I think the idea of the "first unconditionally go to $D and then do these things" pattern was that these tests anticipate that the step before them will leave the process in an unexpected directory when they begin. If the original version of this test fails when we created the first commit "updated by origin", the next test piece will start in "$D" directory, and if we successfully run it to the end, the next test piece will start in "$D/ two".

Now, the point of this patch is to make sure each test piece will not chdir around by isolating the parts that run in different directories inside subshells. The purpose of doing so is? It is to relieve later tests from having to worry about "going back to the known starting place".

So, it is dubious that we want the subshell around the whole thing, whose first command is to go to "$D", after we apply this patch. Removal of that part was the primary reason why we are writing this patch.

So I'd expect that the above test piece would become more like in "git diff -w" output.

It is important to notice that the reason why we had 'cd "$D"' in this test is *not* because the previous test has chdir'ed around, but to look similar to later tests in the series.

Thanks.

diff --git c/t/t5510-fetch.sh w/t/t5510-fetch.sh
index 6f38a69fbb..1ed27607e2 100755
--- c/t/t5510-fetch.sh
+++ w/t/t5510-fetch.sh
@@ -48,15 +48,16 @@ test_expect_success "clone and setup child repos" '
 '
 
 test_expect_success "fetch test" '
-	cd "$D" &&
 	echo >file updated by origin &&
 	git commit -a -m "updated by origin" &&
+	(
 		cd two &&
 		git fetch &&
 		git rev-parse --verify refs/heads/one &&
 		mine=$(git rev-parse refs/heads/one) &&
 		his=$(cd ../one && git rev-parse refs/heads/main) &&
 		test "z$mine" = "z$his"
+	)
 '
 
 test_expect_success "fetch test for-merge" '