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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.