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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Mar 31 2022, Elia Pinto wrote:

> Clean up the code style so all the tests, and not just a few,
> that chdir around isolate themselves in a subshell.

Sounds sensible.

>  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 &&

Why the two levels of subshelling though? We don't need a new one every
time we change directories, or do we?

The point is usually to avoid cd-ing in our main shell, not that each
level needs a new shell & indentation...

> -	test_cmp expected actual'
> +	(
> +		cd "$D" &&
> +		(
> +			cd three &&

ditto..

> +			git fetch &&
> +			git rev-parse --verify refs/heads/two &&
> +			git rev-parse --verify refs/heads/one &&

FWIW an alternative here is to use git -C "$D/three", but that may end
up being too verbose..

>  test_expect_success 'fetch --prune handles overlapping refspecs' '
> -	cd "$D" &&
> -	git update-ref refs/pull/42/head main &&
> -	git clone . prune-overlapping &&
> -	cd prune-overlapping &&
> -	git config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&
> -
> -	git fetch --prune origin &&
> -	git rev-parse origin/main &&
> -	git rev-parse origin/pr/42 &&
> -
> -	git config --unset-all remote.origin.fetch &&
> -	git config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&
> -	git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* &&
> -
> -	git fetch --prune origin &&
> -	git rev-parse origin/main &&
> -	git rev-parse origin/pr/42
> +	(
> +		cd "$D" &&
> +		git update-ref refs/pull/42/head main &&
> +		git clone . prune-overlapping &&
> +		cd prune-overlapping &&
> +		git config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&
> +		git fetch --prune origin &&
> +		git rev-parse origin/main &&
> +		git rev-parse origin/pr/42 &&
> +		git config --unset-all remote.origin.fetch &&
> +		git config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&
> +		git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* &&
> +		git fetch --prune origin &&
> +		git rev-parse origin/main &&
> +		git rev-parse origin/pr/42
> +	)
>  '

Please don't lose grouping whitespace while at it.  I.e. the pre-image
intentionally splits "steps" by \n\n.

>  
>  test_expect_success 'fetch --prune --tags prunes branches but not tags' '
> -	cd "$D" &&
> -	git clone . prune-tags &&
> -	cd prune-tags &&
> -	git tag sometag main &&
> -	# Create what looks like a remote-tracking branch from an earlier
> -	# fetch that has since been deleted from the remote:
> -	git update-ref refs/remotes/origin/fake-remote main &&
> -
> -	git fetch --prune --tags origin &&
> -	git rev-parse origin/main &&
> -	test_must_fail git rev-parse origin/fake-remote &&
> -	git rev-parse sometag
> +	(
> +		cd "$D" &&
> +		git clone . prune-tags &&
> +		cd prune-tags &&
> +		git tag sometag main &&
> +		# Create what looks like a remote-tracking branch from an earlier
> +		# fetch that has since been deleted from the remote:
> +		git update-ref refs/remotes/origin/fake-remote main &&
> +		git fetch --prune --tags origin &&
> +		git rev-parse origin/main &&
> +		test_must_fail git rev-parse origin/fake-remote &&
> +		git rev-parse sometag
> +	)
>  '
>  
>  test_expect_success 'fetch --prune --tags with branch does not prune other things' '
> -	cd "$D" &&
> -	git clone . prune-tags-branch &&
> -	cd prune-tags-branch &&
> -	git tag sometag main &&
> -	git update-ref refs/remotes/origin/extrabranch main &&
> -
> -	git fetch --prune --tags origin main &&
> -	git rev-parse origin/extrabranch &&
> -	git rev-parse sometag
> +	(
> +		cd "$D" &&
> +		git clone . prune-tags-branch &&
> +		cd prune-tags-branch &&
> +		git tag sometag main &&
> +		git update-ref refs/remotes/origin/extrabranch main &&
> +		git fetch --prune --tags origin main &&
> +		git rev-parse origin/extrabranch &&
> +		git rev-parse sometag
> +	)
>  '

Skimming these these seem like much of the same code over & over again
with tiny variations. Perhaps even better would be splitting much of
this into a helper function(s)?

> -	git -C atomic fetch --atomic origin &&
> -	git -C atomic rev-parse origin/atomic-branch >actual &&
> -	test_cmp expected actual &&
> -	test $oid = "$(git -C atomic rev-parse --verify FETCH_HEAD)"
> +	(
> +		cd "$D" &&
> +		git clone . atomic &&
> +		git branch atomic-branch &&
> +		oid=$(git rev-parse atomic-branch) &&
> +		echo "$oid" >expected &&
> +		git -C atomic fetch --atomic origin &&
> +		git -C atomic rev-parse origin/atomic-branch >actual &&
> +		test_cmp expected actual &&
> +		test $oid = "$(git -C atomic rev-parse --verify FETCH_HEAD)"

speaking of modern style, perhaps it's worth it to fix these exit code
hiding issues? I.e. use test_cmp, test_cmp_rev etc.

> +		head_oid=$(git rev-parse HEAD) &&
> +		cat >expected <<-EOF &&
> +			prepared
> +			$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1
> +			$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2
> +			committed
> +			$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1
> +			$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2
> +		EOF

There was a discussion on-list the other day about how this particular
here-doc style is the odd one out, and we'd prefer the content aligned
with the "cat".

So if we're re-indenting all of these that would be a nice change while
we're at it, particularly as it would make the diff smaller, they'd
already be at the "right" indent level.