git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 3/4] t5510: prefer "git -C" to subshell for followRemoteHEAD tests

From
SZEDER Gábor <szeder.dev@gmail.com>
Date
Aug 24, 2025, 19:41 UTC
Message-ID
<aKtq47vmCrUZCUCF@szeder.dev>
In-Reply-To
<20250819192716.GC1059295@coredump.intra.peff.net>
On Tue, Aug 19, 2025 at 03:27:16PM -0400, Jeff King wrote:
Show 17 quoted lines
> These tests set config within a sub-repo using (cd two && git config),
> and then a separate test_when_finished outside the subshell to clean it
> up. We can't use test_config to do this, because the cleanup command it
> registers inside the subshell would be lost. Nor can we do it before
> entering the subshell, because the config has to be set after some other
> commands are run.
> 
> Let's switch these tests to use "git -C" for each command instead of a
> subshell. That lets us use test_config (with -C also) at the appropriate
> part of the test. And we no longer need the manual cleanup command.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> It is perhaps debatable whether this makes the result more readable.
> It's fewer lines, but there is "-C" sprinkled everywhere. So if people
> find this ugly we can drop it (and I'd rewrite patch 4 to use the
> subshell form in its new test).
I for one think that the original is much more readable.

With the subshell it's quite clear, even at a cursory glance, which commands are executed in a subdirectory, but when using '-C dir' all over we have to look closely. Furthermore, when there is a command outside of the subshell, we can be fairly sure that it's intentional, but when a command without '-C dir' lurks among many others using '-C dir', then we can't be so sure, but have to investigate whether that was intentional or oversight.

Show 244 quoted lines
>  t/t5510-fetch.sh | 202 +++++++++++++++++++----------------------------
>  1 file changed, 83 insertions(+), 119 deletions(-)
> 
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 93e309e213..24379ec7aa 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -123,149 +123,113 @@ test_expect_success "fetch test remote HEAD change" '
>  '
>  
>  test_expect_success "fetch test followRemoteHEAD never" '
> -	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
> -	(
> -		cd two &&
> -		git update-ref --no-deref -d refs/remotes/origin/HEAD &&
> -		git config set remote.origin.followRemoteHEAD "never" &&
> -		GIT_TRACE_PACKET=$PWD/trace.out git fetch &&
> -		# Confirm that we do not even ask for HEAD when we are
> -		# not going to act on it.
> -		test_grep ! "ref-prefix HEAD" trace.out &&
> -		test_must_fail git rev-parse --verify refs/remotes/origin/HEAD
> -	)
> +	git -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&
> +	test_config -C two remote.origin.followRemoteHEAD "never" &&
> +	GIT_TRACE_PACKET=$PWD/trace.out git -C two fetch &&
> +	# Confirm that we do not even ask for HEAD when we are
> +	# not going to act on it.
> +	test_grep ! "ref-prefix HEAD" trace.out &&
> +	test_must_fail git -C two rev-parse --verify refs/remotes/origin/HEAD
>  '
>  
>  test_expect_success "fetch test followRemoteHEAD warn no change" '
> -	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
> -	(
> -		cd two &&
> -		git rev-parse --verify refs/remotes/origin/other &&
> -		git remote set-head origin other &&
> -		git rev-parse --verify refs/remotes/origin/HEAD &&
> -		git rev-parse --verify refs/remotes/origin/main &&
> -		git config set remote.origin.followRemoteHEAD "warn" &&
> -		git fetch >output &&
> -		echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \
> -			"but we have ${SQ}other${SQ} locally." >expect &&
> -		test_cmp expect output &&
> -		head=$(git rev-parse refs/remotes/origin/HEAD) &&
> -		branch=$(git rev-parse refs/remotes/origin/other) &&
> -		test "z$head" = "z$branch"
> -	)
> +	git -C two rev-parse --verify refs/remotes/origin/other &&
> +	git -C two remote set-head origin other &&
> +	git -C two rev-parse --verify refs/remotes/origin/HEAD &&
> +	git -C two rev-parse --verify refs/remotes/origin/main &&
> +	test_config -C two remote.origin.followRemoteHEAD "warn" &&
> +	git -C two fetch >output &&
> +	echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \
> +		"but we have ${SQ}other${SQ} locally." >expect &&
> +	test_cmp expect output &&
> +	head=$(git -C two rev-parse refs/remotes/origin/HEAD) &&
> +	branch=$(git -C two rev-parse refs/remotes/origin/other) &&
> +	test "z$head" = "z$branch"
>  '
>  
>  test_expect_success "fetch test followRemoteHEAD warn create" '
> -	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
> -	(
> -		cd two &&
> -		git update-ref --no-deref -d refs/remotes/origin/HEAD &&
> -		git config set remote.origin.followRemoteHEAD "warn" &&
> -		git rev-parse --verify refs/remotes/origin/main &&
> -		output=$(git fetch) &&
> -		test "z" = "z$output" &&
> -		head=$(git rev-parse refs/remotes/origin/HEAD) &&
> -		branch=$(git rev-parse refs/remotes/origin/main) &&
> -		test "z$head" = "z$branch"
> -	)
> +	git -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&
> +	test_config -C two remote.origin.followRemoteHEAD "warn" &&
> +	git -C two rev-parse --verify refs/remotes/origin/main &&
> +	output=$(git -C two fetch) &&
> +	test "z" = "z$output" &&
> +	head=$(git -C two rev-parse refs/remotes/origin/HEAD) &&
> +	branch=$(git -C two rev-parse refs/remotes/origin/main) &&
> +	test "z$head" = "z$branch"
>  '
>  
>  test_expect_success "fetch test followRemoteHEAD warn detached" '
> -	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
> -	(
> -		cd two &&
> -		git update-ref --no-deref -d refs/remotes/origin/HEAD &&
> -		git update-ref refs/remotes/origin/HEAD HEAD &&
> -		HEAD=$(git log --pretty="%H") &&
> -		git config set remote.origin.followRemoteHEAD "warn" &&
> -		git fetch >output &&
> -		echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \
> -			"but we have a detached HEAD pointing to" \
> -			"${SQ}${HEAD}${SQ} locally." >expect &&
> -		test_cmp expect output
> -	)
> +	git -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&
> +	git -C two update-ref refs/remotes/origin/HEAD HEAD &&
> +	HEAD=$(git -C two log --pretty="%H") &&
> +	test_config -C two remote.origin.followRemoteHEAD "warn" &&
> +	git -C two fetch >output &&
> +	echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \
> +		"but we have a detached HEAD pointing to" \
> +		"${SQ}${HEAD}${SQ} locally." >expect &&
> +	test_cmp expect output
>  '
>  
>  test_expect_success "fetch test followRemoteHEAD warn quiet" '
> -	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
> -	(
> -		cd two &&
> -		git rev-parse --verify refs/remotes/origin/other &&
> -		git remote set-head origin other &&
> -		git rev-parse --verify refs/remotes/origin/HEAD &&
> -		git rev-parse --verify refs/remotes/origin/main &&
> -		git config set remote.origin.followRemoteHEAD "warn" &&
> -		output=$(git fetch --quiet) &&
> -		test "z" = "z$output" &&
> -		head=$(git rev-parse refs/remotes/origin/HEAD) &&
> -		branch=$(git rev-parse refs/remotes/origin/other) &&
> -		test "z$head" = "z$branch"
> -	)
> +	git -C two rev-parse --verify refs/remotes/origin/other &&
> +	git -C two remote set-head origin other &&
> +	git -C two rev-parse --verify refs/remotes/origin/HEAD &&
> +	git -C two rev-parse --verify refs/remotes/origin/main &&
> +	test_config -C two remote.origin.followRemoteHEAD "warn" &&
> +	output=$(git -C two fetch --quiet) &&
> +	test "z" = "z$output" &&
> +	head=$(git -C two rev-parse refs/remotes/origin/HEAD) &&
> +	branch=$(git -C two rev-parse refs/remotes/origin/other) &&
> +	test "z$head" = "z$branch"
>  '
>  
>  test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is same" '
> -	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
> -	(
> -		cd two &&
> -		git rev-parse --verify refs/remotes/origin/other &&
> -		git remote set-head origin other &&
> -		git rev-parse --verify refs/remotes/origin/HEAD &&
> -		git rev-parse --verify refs/remotes/origin/main &&
> -		git config set remote.origin.followRemoteHEAD "warn-if-not-main" &&
> -		actual=$(git fetch) &&
> -		test "z" = "z$actual" &&
> -		head=$(git rev-parse refs/remotes/origin/HEAD) &&
> -		branch=$(git rev-parse refs/remotes/origin/other) &&
> -		test "z$head" = "z$branch"
> -	)
> +	git -C two rev-parse --verify refs/remotes/origin/other &&
> +	git -C two remote set-head origin other &&
> +	git -C two rev-parse --verify refs/remotes/origin/HEAD &&
> +	git -C two rev-parse --verify refs/remotes/origin/main &&
> +	test_config -C two remote.origin.followRemoteHEAD "warn-if-not-main" &&
> +	actual=$(git -C two fetch) &&
> +	test "z" = "z$actual" &&
> +	head=$(git -C two rev-parse refs/remotes/origin/HEAD) &&
> +	branch=$(git -C two rev-parse refs/remotes/origin/other) &&
> +	test "z$head" = "z$branch"
>  '
>  
>  test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is different" '
> -	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
> -	(
> -		cd two &&
> -		git rev-parse --verify refs/remotes/origin/other &&
> -		git remote set-head origin other &&
> -		git rev-parse --verify refs/remotes/origin/HEAD &&
> -		git rev-parse --verify refs/remotes/origin/main &&
> -		git config set remote.origin.followRemoteHEAD "warn-if-not-some/different-branch" &&
> -		git fetch >actual &&
> -		echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \
> -			"but we have ${SQ}other${SQ} locally." >expect &&
> -		test_cmp expect actual &&
> -		head=$(git rev-parse refs/remotes/origin/HEAD) &&
> -		branch=$(git rev-parse refs/remotes/origin/other) &&
> -		test "z$head" = "z$branch"
> -	)
> +	git -C two rev-parse --verify refs/remotes/origin/other &&
> +	git -C two remote set-head origin other &&
> +	git -C two rev-parse --verify refs/remotes/origin/HEAD &&
> +	git -C two rev-parse --verify refs/remotes/origin/main &&
> +	test_config -C two remote.origin.followRemoteHEAD "warn-if-not-some/different-branch" &&
> +	git -C two fetch >actual &&
> +	echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \
> +		"but we have ${SQ}other${SQ} locally." >expect &&
> +	test_cmp expect actual &&
> +	head=$(git -C two rev-parse refs/remotes/origin/HEAD) &&
> +	branch=$(git -C two rev-parse refs/remotes/origin/other) &&
> +	test "z$head" = "z$branch"
>  '
>  
>  test_expect_success "fetch test followRemoteHEAD always" '
> -	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
> -	(
> -		cd two &&
> -		git rev-parse --verify refs/remotes/origin/other &&
> -		git remote set-head origin other &&
> -		git rev-parse --verify refs/remotes/origin/HEAD &&
> -		git rev-parse --verify refs/remotes/origin/main &&
> -		git config set remote.origin.followRemoteHEAD "always" &&
> -		git fetch &&
> -		head=$(git rev-parse refs/remotes/origin/HEAD) &&
> -		branch=$(git rev-parse refs/remotes/origin/main) &&
> -		test "z$head" = "z$branch"
> -	)
> +	git -C two rev-parse --verify refs/remotes/origin/other &&
> +	git -C two remote set-head origin other &&
> +	git -C two rev-parse --verify refs/remotes/origin/HEAD &&
> +	git -C two rev-parse --verify refs/remotes/origin/main &&
> +	test_config -C two remote.origin.followRemoteHEAD "always" &&
> +	git -C two fetch &&
> +	head=$(git -C two rev-parse refs/remotes/origin/HEAD) &&
> +	branch=$(git -C two rev-parse refs/remotes/origin/main) &&
> +	test "z$head" = "z$branch"
>  '
>  
>  test_expect_success 'followRemoteHEAD does not kick in with refspecs' '
> -	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
> -	(
> -		cd two &&
> -		git remote set-head origin other &&
> -		git config set remote.origin.followRemoteHEAD always &&
> -		git fetch origin refs/heads/main:refs/remotes/origin/main &&
> -		echo refs/remotes/origin/other >expect &&
> -		git symbolic-ref refs/remotes/origin/HEAD >actual &&
> -		test_cmp expect actual
> -	)
> +	git -C two remote set-head origin other &&
> +	test_config -C two remote.origin.followRemoteHEAD always &&
> +	git -C two fetch origin refs/heads/main:refs/remotes/origin/main &&
> +	echo refs/remotes/origin/other >expect &&
> +	git -C two symbolic-ref refs/remotes/origin/HEAD >actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success 'fetch --prune on its own works as expected' '
> -- 
> 2.51.0.326.gecbb38d78e
> 
>
Previous: Jeff KingNext: Junio C Hamano
Message 9 of 22 in “dangling symrefs and fetchRemoteHEAD=create”
  1. 0/4 dangling symrefs and fetchRemoteHEAD=createJeff King, Aug 19, 2025
  2. Jeff KingAug 19, 2025
  3. 1/4 t5510: make confusing config cleanup more explicitJeff King, Aug 19, 2025
  4. Eric SunshineAug 19, 2025
  5. Eric SunshineAug 19, 2025
  6. Jeff KingAug 19, 2025
  7. 2/4 t5510: stop changing top-level working directoryJeff King, Aug 19, 2025
  8. 3/4 t5510: prefer "git -C" to subshell for followRemoteHEAD testsJeff King, Aug 19, 2025
  9. SZEDER GáborAug 24, 2025
  10. Junio C HamanoAug 25, 2025
  11. Jeff KingAug 26, 2025
  12. Junio C HamanoAug 26, 2025
  13. 4/4 refs: do not clobber dangling symrefsJeff King, Aug 19, 2025
  14. Patrick SteinhardtAug 20, 2025
  15. Jeff KingAug 20, 2025
  16. Toon ClaesSep 22, 2025
  17. Junio C HamanoSep 22, 2025
  18. Jeff KingSep 22, 2025
  19. Junio C HamanoSep 22, 2025
  20. Jeff KingSep 22, 2025
  21. Toon ClaesSep 23, 2025
  22. Jeff KingSep 23, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.