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

[Junio C Hamano <gitster@pobox.com>]

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

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