From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Patrick Steinhardt <ps@pks.im>
Subject: Re: [PATCH 3/4] t5510: prefer "git -C" to subshell for followRemoteHEAD tests
Date: Sun, 24 Aug 2025 21:41:23 +0200 [thread overview]
Message-ID: <aKtq47vmCrUZCUCF@szeder.dev> (raw)
In-Reply-To: <20250819192716.GC1059295@coredump.intra.peff.net>
On Tue, Aug 19, 2025 at 03:27:16PM -0400, Jeff King wrote:
> 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.
> 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
>
>
next prev parent reply other threads:[~2025-08-24 19:41 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-19 19:20 [PATCH 0/4] dangling symrefs and fetchRemoteHEAD=create Jeff King
2025-08-19 19:23 ` Jeff King
2025-08-19 19:24 ` [PATCH 1/4] t5510: make confusing config cleanup more explicit Jeff King
2025-08-19 20:03 ` Eric Sunshine
2025-08-19 20:16 ` Eric Sunshine
2025-08-19 20:53 ` Jeff King
2025-08-19 19:26 ` [PATCH 2/4] t5510: stop changing top-level working directory Jeff King
2025-08-19 19:27 ` [PATCH 3/4] t5510: prefer "git -C" to subshell for followRemoteHEAD tests Jeff King
2025-08-24 19:41 ` SZEDER Gábor [this message]
2025-08-25 15:46 ` Junio C Hamano
2025-08-26 3:44 ` Jeff King
2025-08-26 14:58 ` Junio C Hamano
2025-08-19 19:29 ` [PATCH 4/4] refs: do not clobber dangling symrefs Jeff King
2025-08-20 7:27 ` Patrick Steinhardt
2025-08-20 19:14 ` Jeff King
2025-09-22 12:23 ` Toon Claes
2025-09-22 15:54 ` Junio C Hamano
2025-09-22 17:21 ` Jeff King
2025-09-22 17:35 ` Junio C Hamano
2025-09-22 17:12 ` Jeff King
2025-09-23 9:36 ` Toon Claes
2025-09-23 17:33 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aKtq47vmCrUZCUCF@szeder.dev \
--to=szeder.dev@gmail.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
--cc=ps@pks.im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).