[PATCH 0/4] dangling symrefs and fetchRemoteHEAD=create

[PATCH 0/4] dangling symrefs and fetchRemoteHEAD=create

[Jeff King]

This fixes a bug I found while investigating another semi-related bug
(that has already been fixed by Patrick), mentioned in the "PS" here:

  https://lore.kernel.org/git/20250724104536.GA1316505@coredump.intra.peff.net/

The issue is that:

  git remote add -m does-not-exist origin <url>
  git config remote.origin.followRemoteHEAD create
  git fetch

will overwrite the refs/remotes/origin/HEAD we created, even though we
asked it to do so only on creation. The issue is actually in the refs
code, and how it perceives dangling symrefs with respect to creation
events. And so this actually affects "update-ref", as well.

A fix is in the final patch, along with a detailed explanation. The
earlier patches are just cleanup of the related test script before we
add our new test there.

  [1/4]: t5510: make confusing config cleanup more explicit
  [2/4]: t5510: stop changing top-level working directory
  [3/4]: t5510: prefer "git -C" to subshell for followRemoteHEAD tests
  [4/4]: refs: do not clobber dangling symrefs

 refs/files-backend.c    |  34 ++-
 refs/reftable-backend.c |  30 ++-
 t/t1400-update-ref.sh   |  21 ++
 t/t5510-fetch.sh        | 543 ++++++++++++++++++----------------------
 4 files changed, 319 insertions(+), 309 deletions(-)

-Peff

Re: [PATCH 0/4] dangling symrefs and fetchRemoteHEAD=create

[Jeff King]

On Tue, Aug 19, 2025 at 03:20:04PM -0400, Jeff King wrote:

> A fix is in the final patch, along with a detailed explanation. The
> earlier patches are just cleanup of the related test script before we
> add our new test there.
> 
>   [1/4]: t5510: make confusing config cleanup more explicit
>   [2/4]: t5510: stop changing top-level working directory
>   [3/4]: t5510: prefer "git -C" to subshell for followRemoteHEAD tests
>   [4/4]: refs: do not clobber dangling symrefs

Oh, one thing I forgot to mention: this must be applied on top of
ps/reflog-migrate-fixes. Specifically the changes to check_old_oid() in
046c67325c (refs: stop unsetting REF_HAVE_OLD for log-only updates,
2025-08-06). Otherwise this logic kicks in for split-HEAD updates, which
makes no sense (if we are creating "refs/heads/foo" and HEAD happens to
point to that branch, we split off a reflog update of HEAD, but we
should not enforce any old-oid rules since we are not writing HEAD at
all).

-Peff

[PATCH 1/4] t5510: make confusing config cleanup more explicit

[Jeff King]

Several tests set a config variable in a sub-repo we chdir into via a
subshell, like this:

  (
	cd "$D" &&
	cd two &&
	git config foo.bar baz
  )

But they also clean up the variable with a when_finished directive
outside of the subshell, like this:

  test_when_finished "git config unset foo.bar"

At first glance, this shouldn't work! The cleanup clause cannot be run
from the subshell (since environment changes there are lost by the time
the test snippet finishes). But since the cleanup command runs outside
the subshell, our working directory will not have been switched into
"two".

But it does work. Why?

The answer is that an earlier test does a "cd two" that moves the whole
test's working directory out of $TRASH_DIRECTORY and into "two". So the
subshell is a bit of a red herring; we are already in the right
directory! That's why we need the "cd $D" at the top of the shell, to
put us back to a known spot.

Let's make this cleanup code more explicitly specify where we expect the
config command to run. That makes the script more robust against running
a subset of the tests, and ultimately will make it easier to refactor
the script to avoid these top-level chdirs.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5510-fetch.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ebc696546b..64fea9f4a5 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -119,7 +119,7 @@ test_expect_success "fetch test remote HEAD change" '
 	test "z$head" = "z$branch"'
 
 test_expect_success "fetch test followRemoteHEAD never" '
-	test_when_finished "git config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
 	(
 		cd "$D" &&
 		cd two &&
@@ -134,7 +134,7 @@ test_expect_success "fetch test followRemoteHEAD never" '
 '
 
 test_expect_success "fetch test followRemoteHEAD warn no change" '
-	test_when_finished "git config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
 	(
 		cd "$D" &&
 		cd two &&
@@ -154,7 +154,7 @@ test_expect_success "fetch test followRemoteHEAD warn no change" '
 '
 
 test_expect_success "fetch test followRemoteHEAD warn create" '
-	test_when_finished "git config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
 	(
 		cd "$D" &&
 		cd two &&
@@ -170,7 +170,7 @@ test_expect_success "fetch test followRemoteHEAD warn create" '
 '
 
 test_expect_success "fetch test followRemoteHEAD warn detached" '
-	test_when_finished "git config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
 	(
 		cd "$D" &&
 		cd two &&
@@ -187,7 +187,7 @@ test_expect_success "fetch test followRemoteHEAD warn detached" '
 '
 
 test_expect_success "fetch test followRemoteHEAD warn quiet" '
-	test_when_finished "git config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
 	(
 		cd "$D" &&
 		cd two &&
@@ -205,7 +205,7 @@ test_expect_success "fetch test followRemoteHEAD warn quiet" '
 '
 
 test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is same" '
-	test_when_finished "git config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
 	(
 		cd "$D" &&
 		cd two &&
@@ -223,7 +223,7 @@ test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is sa
 '
 
 test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is different" '
-	test_when_finished "git config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
 	(
 		cd "$D" &&
 		cd two &&
@@ -243,7 +243,7 @@ test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is di
 '
 
 test_expect_success "fetch test followRemoteHEAD always" '
-	test_when_finished "git config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
 	(
 		cd "$D" &&
 		cd two &&
@@ -260,7 +260,7 @@ test_expect_success "fetch test followRemoteHEAD always" '
 '
 
 test_expect_success 'followRemoteHEAD does not kick in with refspecs' '
-	test_when_finished "git config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
 	(
 		cd "$D" &&
 		cd two &&
-- 
2.51.0.326.gecbb38d78e

Re: [PATCH 1/4] t5510: make confusing config cleanup more explicit

[Eric Sunshine]

On 8/19/25 3:24 PM, Jeff King wrote:
> Several tests set a config variable in a sub-repo we chdir into via a
> subshell, like this:
> 
>    (
> 	cd "$D" &&
> 	cd two &&
> 	git config foo.bar baz
>    )
> 
> But they also clean up the variable with a when_finished directive
> outside of the subshell, like this:
> 
>    test_when_finished "git config unset foo.bar"
> 
> At first glance, this shouldn't work! The cleanup clause cannot be run
> from the subshell (since environment changes there are lost by the time
> the test snippet finishes). But since the cleanup command runs outside
> the subshell, our working directory will not have been switched into
> "two".
> 
> But it does work. Why?
> 
> The answer is that an earlier test does a "cd two" that moves the whole
> test's working directory out of $TRASH_DIRECTORY and into "two". So the
> subshell is a bit of a red herring; we are already in the right
> directory! That's why we need the "cd $D" at the top of the shell, to
> put us back to a known spot.
> 
> Let's make this cleanup code more explicitly specify where we expect the
> config command to run. That makes the script more robust against running
> a subset of the tests, and ultimately will make it easier to refactor
> the script to avoid these top-level chdirs.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> @@ -119,7 +119,7 @@ test_expect_success "fetch test remote HEAD change" '
> -	test_when_finished "git config unset remote.origin.followRemoteHEAD" &&
> +	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&

For what it's worth, I have an unsent patch from a much larger unsent 
series which cleans up the t5510 messiness differently. (The below patch 
is probably whitespace damaged by the MUA.)


From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sun, 20 Nov 2022 00:48:00 -0500
Subject: [PATCH 17/41] t5510: stop invoking `cd` outside of subshell

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
  t/t5510-fetch.sh | 123 ++++++++++++++++++-----------------------------
  1 file changed, 47 insertions(+), 76 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index c0b745e33b..051af8d3f7 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -8,8 +8,6 @@ test_description='Per branch config variables affects 
"git fetch".
  . ./test-lib.sh
  . "$TEST_DIRECTORY"/lib-bundle.sh

-D=$(pwd)
-
  test_expect_success setup '
  	echo >file original &&
  	git add file &&
@@ -48,19 +46,20 @@ 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" '
-	cd "$D" &&
+	(
  	cd three &&
  	git fetch &&
  	git rev-parse --verify refs/heads/two &&
@@ -72,41 +71,46 @@ test_expect_success "fetch test for-merge" '
  		echo "$main_in_two	not-for-merge"
  	} >expected &&
  	cut -f -2 .git/FETCH_HEAD >actual &&
-	test_cmp expected actual'
+	test_cmp expected actual
+	)
+'

  test_expect_success 'fetch --prune on its own works as expected' '
-	cd "$D" &&
  	git clone . prune &&
+	(
  	cd prune &&
  	git update-ref refs/remotes/origin/extrabranch main &&

  	git fetch --prune origin &&
  	test_must_fail git rev-parse origin/extrabranch
+	)
  '

  test_expect_success 'fetch --prune with a branch name keeps branches' '
-	cd "$D" &&
  	git clone . prune-branch &&
+	(
  	cd prune-branch &&
  	git update-ref refs/remotes/origin/extrabranch main &&

  	git fetch --prune origin main &&
  	git rev-parse origin/extrabranch
+	)
  '

  test_expect_success 'fetch --prune with a namespace keeps other 
namespaces' '
-	cd "$D" &&
  	git clone . prune-namespace &&
+	(
  	cd prune-namespace &&

  	git fetch --prune origin refs/heads/a/*:refs/remotes/origin/a/* &&
  	git rev-parse origin/main
+	)
  '

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

@@ -121,11 +125,12 @@ test_expect_success 'fetch --prune handles 
overlapping refspecs' '
  	git fetch --prune origin &&
  	git rev-parse origin/main &&
  	git rev-parse origin/pr/42
+	)
  '

  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
@@ -136,11 +141,12 @@ test_expect_success 'fetch --prune --tags prunes 
branches but not tags' '
  	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 &&
@@ -148,11 +154,12 @@ test_expect_success 'fetch --prune --tags with 
branch does not prune other thing
  	git fetch --prune --tags origin main &&
  	git rev-parse origin/extrabranch &&
  	git rev-parse sometag
+	)
  '

  test_expect_success 'fetch --prune --tags with refspec prunes based on 
refspec' '
-	cd "$D" &&
  	git clone . prune-tags-refspec &&
+	(
  	cd prune-tags-refspec &&
  	git tag sometag main &&
  	git update-ref refs/remotes/origin/foo/otherbranch main &&
@@ -162,23 +169,24 @@ test_expect_success 'fetch --prune --tags with 
refspec prunes based on refspec'
  	test_must_fail git rev-parse refs/remotes/origin/foo/otherbranch &&
  	git rev-parse origin/extrabranch &&
  	git rev-parse sometag
+	)
  '

  test_expect_success REFFILES 'fetch --prune fails to delete branches' '
-	cd "$D" &&
  	git clone . prune-fail &&
+	(
  	cd prune-fail &&
  	git update-ref refs/remotes/origin/extrabranch main &&
  	: this will prevent --prune from locking packed-refs for deleting 
refs, but adding loose refs still succeeds  &&
  	>.git/packed-refs.new &&

  	test_must_fail git fetch --prune origin
+	)
  '

  test_expect_success 'fetch --atomic works with a single branch' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&

-	cd "$D" &&
  	git clone . atomic &&
  	git branch atomic-branch &&
  	oid=$(git rev-parse atomic-branch) &&
@@ -191,9 +199,8 @@ test_expect_success 'fetch --atomic works with a 
single branch' '
  '

  test_expect_success 'fetch --atomic works with multiple branches' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&

-	cd "$D" &&
  	git clone . atomic &&
  	git branch atomic-branch-1 &&
  	git branch atomic-branch-2 &&
@@ -206,9 +213,8 @@ test_expect_success 'fetch --atomic works with 
multiple branches' '
  '

  test_expect_success 'fetch --atomic works with mixed branches and tags' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&

-	cd "$D" &&
  	git clone . atomic &&
  	git branch atomic-mixed-branch &&
  	git tag atomic-mixed-tag &&
@@ -220,9 +226,8 @@ test_expect_success 'fetch --atomic works with mixed 
branches and tags' '
  '

  test_expect_success 'fetch --atomic prunes references' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&

-	cd "$D" &&
  	git branch atomic-prune-delete &&
  	git clone . atomic &&
  	git branch --delete atomic-prune-delete &&
@@ -236,9 +241,8 @@ test_expect_success 'fetch --atomic prunes references' '
  '

  test_expect_success 'fetch --atomic aborts with non-fast-forward update' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&

-	cd "$D" &&
  	git branch atomic-non-ff &&
  	git clone . atomic &&
  	git rev-parse HEAD >actual &&
@@ -255,9 +259,8 @@ test_expect_success 'fetch --atomic aborts with 
non-fast-forward update' '
  '

  test_expect_success 'fetch --atomic executes a single reference 
transaction only' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&

-	cd "$D" &&
  	git clone . atomic &&
  	git branch atomic-hooks-1 &&
  	git branch atomic-hooks-2 &&
@@ -282,9 +285,8 @@ test_expect_success 'fetch --atomic executes a 
single reference transaction only
  '

  test_expect_success 'fetch --atomic aborts all reference updates if 
hook aborts' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&

-	cd "$D" &&
  	git clone . atomic &&
  	git branch atomic-hooks-abort-1 &&
  	git branch atomic-hooks-abort-2 &&
@@ -319,9 +321,8 @@ test_expect_success 'fetch --atomic aborts all 
reference updates if hook aborts'
  '

  test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&

-	cd "$D" &&
  	git clone . atomic &&
  	oid=$(git rev-parse HEAD) &&

@@ -344,8 +345,7 @@ test_expect_success 'fetch --atomic --append appends 
to FETCH_HEAD' '
  '

  test_expect_success '--refmap="" ignores configured refspec' '
-	cd "$TRASH_DIRECTORY" &&
-	git clone "$D" remote-refs &&
+	git clone . remote-refs &&
  	git -C remote-refs rev-parse remotes/origin/main >old &&
  	git -C remote-refs update-ref refs/remotes/origin/main main~1 &&
  	git -C remote-refs rev-parse remotes/origin/main >new &&
@@ -368,35 +368,31 @@ test_expect_success '--refmap="" and --prune' '
  '

  test_expect_success 'fetch tags when there is no tags' '
-
-    cd "$D" &&
-
      mkdir notags &&
+    (
      cd notags &&
      git init &&

      git fetch -t ..
-
+    )
  '

  test_expect_success 'fetch following tags' '
-
-	cd "$D" &&
  	git tag -a -m "annotated" anno HEAD &&
  	git tag light HEAD &&

  	mkdir four &&
+	(
  	cd four &&
  	git init &&

  	git fetch .. :track &&
  	git show-ref --verify refs/tags/anno &&
  	git show-ref --verify refs/tags/light
-
+	)
  '

  test_expect_success 'fetch uses remote ref names to describe new refs' '
-	cd "$D" &&
  	git init descriptive &&
  	(
  		cd descriptive &&
@@ -423,31 +419,28 @@ test_expect_success 'fetch uses remote ref names 
to describe new refs' '
  '

  test_expect_success 'fetch must not resolve short tag name' '
-
-	cd "$D" &&
-
  	mkdir five &&
+	(
  	cd five &&
  	git init &&

  	test_must_fail git fetch .. anno:five
-
+	)
  '

  test_expect_success 'fetch can now resolve short remote name' '
-
-	cd "$D" &&
  	git update-ref refs/remotes/six/HEAD HEAD &&

  	mkdir six &&
+	(
  	cd six &&
  	git init &&

  	git fetch .. six:six
+	)
  '

  test_expect_success 'create bundle 1' '
-	cd "$D" &&
  	echo >file updated again by origin &&
  	git commit -a -m "tip" &&
  	git bundle create --version=3 bundle1 main^..main
@@ -461,35 +454,30 @@ test_expect_success 'header of bundle looks right' '
  	OID refs/heads/main

  	EOF
-	sed -e "s/$OID_REGEX/OID/g" -e "5q" "$D"/bundle1 >actual &&
+	sed -e "s/$OID_REGEX/OID/g" -e "5q" "$(pwd)"/bundle1 >actual &&
  	test_cmp expect actual
  '

  test_expect_success 'create bundle 2' '
-	cd "$D" &&
  	git bundle create bundle2 main~2..main
  '

  test_expect_success 'unbundle 1' '
-	cd "$D/bundle" &&
-	git checkout -b some-branch &&
-	test_must_fail git fetch "$D/bundle1" main:main
+	git -C bundle checkout -b some-branch &&
+	test_must_fail git -C bundle fetch "../bundle1" main:main
  '


  test_expect_success 'bundle 1 has only 3 files ' '
-	cd "$D" &&
  	test_bundle_object_count bundle1 3
  '

  test_expect_success 'unbundle 2' '
-	cd "$D/bundle" &&
-	git fetch ../bundle2 main:main &&
-	test "tip" = "$(git log -1 --pretty=oneline main | cut -d" " -f2)"
+	git -C bundle fetch ../bundle2 main:main &&
+	test "tip" = "$(git -C bundle log -1 --pretty=oneline main | cut -d" " 
-f2)"
  '

  test_expect_success 'bundle does not prerequisite objects' '
-	cd "$D" &&
  	touch file2 &&
  	git add file2 &&
  	git commit -m add.file2 file2 &&
@@ -498,11 +486,8 @@ test_expect_success 'bundle does not prerequisite 
objects' '
  '

  test_expect_success 'bundle should be able to create a full history' '
-
-	cd "$D" &&
  	git tag -a -m "1.0" v1.0 main &&
  	git bundle create bundle4 v1.0
-
  '

  test_expect_success 'fetch with a non-applying branch.<name>.merge' '
@@ -553,7 +538,6 @@ test_expect_success 'quoting of a strangely named 
repo' '

  test_expect_success 'bundle should record HEAD correctly' '

-	cd "$D" &&
  	git bundle create bundle5 HEAD main &&
  	git bundle list-heads bundle5 >actual &&
  	for h in HEAD refs/heads/main
@@ -573,7 +557,6 @@ test_expect_success 'mark initial state of 
origin/main' '

  test_expect_success 'explicit fetch should update tracking' '

-	cd "$D" &&
  	git branch -f side &&
  	(
  		cd three &&
@@ -588,7 +571,6 @@ test_expect_success 'explicit fetch should update 
tracking' '

  test_expect_success 'explicit pull should update tracking' '

-	cd "$D" &&
  	git branch -f side &&
  	(
  		cd three &&
@@ -602,7 +584,6 @@ test_expect_success 'explicit pull should update 
tracking' '
  '

  test_expect_success 'explicit --refmap is allowed only with 
command-line refspec' '
-	cd "$D" &&
  	(
  		cd three &&
  		test_must_fail git fetch --refmap="*:refs/remotes/none/*"
@@ -610,7 +591,6 @@ test_expect_success 'explicit --refmap is allowed 
only with command-line refspec
  '

  test_expect_success 'explicit --refmap option overrides remote.*.fetch' '
-	cd "$D" &&
  	git branch -f side &&
  	(
  		cd three &&
@@ -625,7 +605,6 @@ test_expect_success 'explicit --refmap option 
overrides remote.*.fetch' '
  '

  test_expect_success 'explicitly empty --refmap option disables 
remote.*.fetch' '
-	cd "$D" &&
  	git branch -f side &&
  	(
  		cd three &&
@@ -639,8 +618,6 @@ test_expect_success 'explicitly empty --refmap 
option disables remote.*.fetch' '
  '

  test_expect_success 'configured fetch updates tracking' '
-
-	cd "$D" &&
  	git branch -f side &&
  	(
  		cd three &&
@@ -654,7 +631,6 @@ test_expect_success 'configured fetch updates 
tracking' '
  '

  test_expect_success 'non-matching refspecs do not confuse tracking 
update' '
-	cd "$D" &&
  	git update-ref refs/odd/location HEAD &&
  	(
  		cd three &&
@@ -670,15 +646,10 @@ test_expect_success 'non-matching refspecs do not 
confuse tracking update' '
  '

  test_expect_success 'pushing nonexistent branch by mistake should not 
segv' '
-
-	cd "$D" &&
  	test_must_fail git push seven no:no
-
  '

  test_expect_success 'auto tag following fetches minimum' '
-
-	cd "$D" &&
  	git clone .git follow &&
  	git checkout HEAD^0 &&
  	(
@@ -1063,7 +1034,7 @@ test_expect_success 'fetch --prune prints the 
remotes url' '
  		cd only-prunes &&
  		git fetch --prune origin 2>&1 | head -n1 >../actual
  	) &&
-	echo "From ${D}/." >expect &&
+	echo "From $(pwd)/." >expect &&
  	test_cmp expect actual
  '

@@ -1097,14 +1068,14 @@ test_expect_success 'fetching with auto-gc does 
not lock up' '
  	echo "$*" &&
  	false
  	EOF
-	git clone "file://$D" auto-gc &&
+	git clone "file://$(pwd)" auto-gc &&
  	test_commit test2 &&
  	(
  		cd auto-gc &&
  		git config fetch.unpackLimit 1 &&
  		git config gc.autoPackLimit 1 &&
  		git config gc.autoDetach false &&
-		GIT_ASK_YESNO="$D/askyesno" git fetch --verbose >fetch.out 2>&1 &&
+		GIT_ASK_YESNO="$(pwd)/askyesno" git fetch --verbose >fetch.out 2>&1 &&
  		test_i18ngrep "Auto packing the repository" fetch.out &&
  		! grep "Should I try again" fetch.out
  	)
-- 
2.39.0.152.ga5737674b6

Re: [PATCH 1/4] t5510: make confusing config cleanup more explicit

[Eric Sunshine]

On Tue, Aug 19, 2025 at 4:05 PM Eric Sunshine <ericsunshine@charter.net> wrote:
> On 8/19/25 3:24 PM, Jeff King wrote:
> > -     test_when_finished "git config unset remote.origin.followRemoteHEAD" &&
> > +     test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
>
> For what it's worth, I have an unsent patch from a much larger unsent
> series which cleans up the t5510 messiness differently. (The below patch
> is probably whitespace damaged by the MUA.)

For the sake of completeness regarding the "much larger unsent
series": That series modifies `chainlint` to detect `cd` invocations
outside of a subshell and fixes the very many instances it discovered.
The series ended up consisting of 41 patches.

Re: [PATCH 1/4] t5510: make confusing config cleanup more explicit

[Jeff King]

On Tue, Aug 19, 2025 at 04:03:23PM -0400, Eric Sunshine wrote:

> > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> > @@ -119,7 +119,7 @@ test_expect_success "fetch test remote HEAD change" '
> > -	test_when_finished "git config unset remote.origin.followRemoteHEAD" &&
> > +	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
> 
> For what it's worth, I have an unsent patch from a much larger unsent series
> which cleans up the t5510 messiness differently. (The below patch is
> probably whitespace damaged by the MUA.)

See my patch 2, which does this part. I split this out because it got
complicated to explain why that patch wasn't breaking these lines. ;)

-Peff

[PATCH 2/4] t5510: stop changing top-level working directory

[Jeff King]

Several tests in t5510 do a bare "cd subrepo", not in a subshell. This
changes the working directory for subsequent tests. As a result, almost
every test has to start with "cd $D" to go back to the top-level.

Our usual style is to do per-test environment changes like this in a
subshell, so that tests can assume they are starting at the top-level
$TRASH_DIRECTORY.

Let's switch to that style, which lets us drop all of that extra
path-handling.

Most cases can switch to using a subshell, but in a few spots we can
simplify by doing "git init foo && git -C foo ...". We do have to make
sure that we weren't intentionally touching the environment in any code
which was moved into a subshell (e.g., with a test_when_finished), but
that isn't the case for any of these tests.

All of the references to the $D variable can go away, replaced generally
with $PWD or $TRASH_DIRECTORY (if we use it inside a chdir'd subshell).
Note in one test, "fetch --prune prints the remotes url", we make sure
to use $(pwd) to get the Windows-style path on that platform (for the
other tests, the exact form doesn't matter).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5510-fetch.sh | 356 +++++++++++++++++++++--------------------------
 1 file changed, 161 insertions(+), 195 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 64fea9f4a5..93e309e213 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -14,8 +14,6 @@ then
 	test_done
 fi
 
-D=$(pwd)
-
 test_expect_success setup '
 	echo >file original &&
 	git add file &&
@@ -51,46 +49,50 @@ 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 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" '
-	cd "$D" &&
-	cd three &&
-	git fetch &&
-	git rev-parse --verify refs/heads/two &&
-	git rev-parse --verify refs/heads/one &&
-	main_in_two=$(cd ../two && git rev-parse main) &&
-	one_in_two=$(cd ../two && git rev-parse one) &&
-	{
-		echo "$one_in_two	" &&
-		echo "$main_in_two	not-for-merge"
-	} >expected &&
-	cut -f -2 .git/FETCH_HEAD >actual &&
-	test_cmp expected actual'
+	(
+		cd three &&
+		git fetch &&
+		git rev-parse --verify refs/heads/two &&
+		git rev-parse --verify refs/heads/one &&
+		main_in_two=$(cd ../two && git rev-parse main) &&
+		one_in_two=$(cd ../two && git rev-parse one) &&
+		{
+			echo "$one_in_two	" &&
+			echo "$main_in_two	not-for-merge"
+		} >expected &&
+		cut -f -2 .git/FETCH_HEAD >actual &&
+		test_cmp expected actual
+	)
+'
 
 test_expect_success "fetch test remote HEAD" '
-	cd "$D" &&
-	cd two &&
-	git fetch &&
-	git rev-parse --verify refs/remotes/origin/HEAD &&
-	git rev-parse --verify refs/remotes/origin/main &&
-	head=$(git rev-parse refs/remotes/origin/HEAD) &&
-	branch=$(git rev-parse refs/remotes/origin/main) &&
-	test "z$head" = "z$branch"'
+	(
+		cd two &&
+		git fetch &&
+		git rev-parse --verify refs/remotes/origin/HEAD &&
+		git rev-parse --verify refs/remotes/origin/main &&
+		head=$(git rev-parse refs/remotes/origin/HEAD) &&
+		branch=$(git rev-parse refs/remotes/origin/main) &&
+		test "z$head" = "z$branch"
+	)
+'
 
 test_expect_success "fetch test remote HEAD in bare repository" '
 	test_when_finished rm -rf barerepo &&
 	(
-		cd "$D" &&
 		git init --bare barerepo &&
 		cd barerepo &&
 		git remote add upstream ../two &&
@@ -105,23 +107,24 @@ test_expect_success "fetch test remote HEAD in bare repository" '
 
 
 test_expect_success "fetch test remote HEAD change" '
-	cd "$D" &&
-	cd two &&
-	git switch -c other &&
-	git push -u origin other &&
-	git rev-parse --verify refs/remotes/origin/HEAD &&
-	git rev-parse --verify refs/remotes/origin/main &&
-	git rev-parse --verify refs/remotes/origin/other &&
-	git remote set-head origin other &&
-	git fetch &&
-	head=$(git rev-parse refs/remotes/origin/HEAD) &&
-	branch=$(git rev-parse refs/remotes/origin/other) &&
-	test "z$head" = "z$branch"'
+	(
+		cd two &&
+		git switch -c other &&
+		git push -u origin other &&
+		git rev-parse --verify refs/remotes/origin/HEAD &&
+		git rev-parse --verify refs/remotes/origin/main &&
+		git rev-parse --verify refs/remotes/origin/other &&
+		git remote set-head origin other &&
+		git fetch &&
+		head=$(git rev-parse refs/remotes/origin/HEAD) &&
+		branch=$(git rev-parse refs/remotes/origin/other) &&
+		test "z$head" = "z$branch"
+	)
+'
 
 test_expect_success "fetch test followRemoteHEAD never" '
-	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
 	(
-		cd "$D" &&
 		cd two &&
 		git update-ref --no-deref -d refs/remotes/origin/HEAD &&
 		git config set remote.origin.followRemoteHEAD "never" &&
@@ -134,9 +137,8 @@ test_expect_success "fetch test followRemoteHEAD never" '
 '
 
 test_expect_success "fetch test followRemoteHEAD warn no change" '
-	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
 	(
-		cd "$D" &&
 		cd two &&
 		git rev-parse --verify refs/remotes/origin/other &&
 		git remote set-head origin other &&
@@ -154,9 +156,8 @@ test_expect_success "fetch test followRemoteHEAD warn no change" '
 '
 
 test_expect_success "fetch test followRemoteHEAD warn create" '
-	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
 	(
-		cd "$D" &&
 		cd two &&
 		git update-ref --no-deref -d refs/remotes/origin/HEAD &&
 		git config set remote.origin.followRemoteHEAD "warn" &&
@@ -170,9 +171,8 @@ test_expect_success "fetch test followRemoteHEAD warn create" '
 '
 
 test_expect_success "fetch test followRemoteHEAD warn detached" '
-	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
 	(
-		cd "$D" &&
 		cd two &&
 		git update-ref --no-deref -d refs/remotes/origin/HEAD &&
 		git update-ref refs/remotes/origin/HEAD HEAD &&
@@ -187,9 +187,8 @@ test_expect_success "fetch test followRemoteHEAD warn detached" '
 '
 
 test_expect_success "fetch test followRemoteHEAD warn quiet" '
-	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
 	(
-		cd "$D" &&
 		cd two &&
 		git rev-parse --verify refs/remotes/origin/other &&
 		git remote set-head origin other &&
@@ -205,9 +204,8 @@ test_expect_success "fetch test followRemoteHEAD warn quiet" '
 '
 
 test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is same" '
-	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
 	(
-		cd "$D" &&
 		cd two &&
 		git rev-parse --verify refs/remotes/origin/other &&
 		git remote set-head origin other &&
@@ -223,9 +221,8 @@ test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is sa
 '
 
 test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is different" '
-	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
 	(
-		cd "$D" &&
 		cd two &&
 		git rev-parse --verify refs/remotes/origin/other &&
 		git remote set-head origin other &&
@@ -243,9 +240,8 @@ test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is di
 '
 
 test_expect_success "fetch test followRemoteHEAD always" '
-	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
 	(
-		cd "$D" &&
 		cd two &&
 		git rev-parse --verify refs/remotes/origin/other &&
 		git remote set-head origin other &&
@@ -260,9 +256,8 @@ test_expect_success "fetch test followRemoteHEAD always" '
 '
 
 test_expect_success 'followRemoteHEAD does not kick in with refspecs' '
-	test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" &&
+	test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" &&
 	(
-		cd "$D" &&
 		cd two &&
 		git remote set-head origin other &&
 		git config set remote.origin.followRemoteHEAD always &&
@@ -274,93 +269,100 @@ test_expect_success 'followRemoteHEAD does not kick in with refspecs' '
 '
 
 test_expect_success 'fetch --prune on its own works as expected' '
-	cd "$D" &&
 	git clone . prune &&
-	cd prune &&
-	git update-ref refs/remotes/origin/extrabranch main &&
+	(
+		cd prune &&
+		git update-ref refs/remotes/origin/extrabranch main &&
 
-	git fetch --prune origin &&
-	test_must_fail git rev-parse origin/extrabranch
+		git fetch --prune origin &&
+		test_must_fail git rev-parse origin/extrabranch
+	)
 '
 
 test_expect_success 'fetch --prune with a branch name keeps branches' '
-	cd "$D" &&
 	git clone . prune-branch &&
-	cd prune-branch &&
-	git update-ref refs/remotes/origin/extrabranch main &&
+	(
+		cd prune-branch &&
+		git update-ref refs/remotes/origin/extrabranch main &&
 
-	git fetch --prune origin main &&
-	git rev-parse origin/extrabranch
+		git fetch --prune origin main &&
+		git rev-parse origin/extrabranch
+	)
 '
 
 test_expect_success 'fetch --prune with a namespace keeps other namespaces' '
-	cd "$D" &&
 	git clone . prune-namespace &&
-	cd prune-namespace &&
+	(
+		cd prune-namespace &&
 
-	git fetch --prune origin refs/heads/a/*:refs/remotes/origin/a/* &&
-	git rev-parse origin/main
+		git fetch --prune origin refs/heads/a/*:refs/remotes/origin/a/* &&
+		git rev-parse origin/main
+	)
 '
 
 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/* &&
+	(
+		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 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 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
+		git fetch --prune origin &&
+		git rev-parse origin/main &&
+		git rev-parse origin/pr/42
+	)
 '
 
 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 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 &&
+	(
+		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
+		git fetch --prune --tags origin main &&
+		git rev-parse origin/extrabranch &&
+		git rev-parse sometag
+	)
 '
 
 test_expect_success 'fetch --prune --tags with refspec prunes based on refspec' '
-	cd "$D" &&
 	git clone . prune-tags-refspec &&
-	cd prune-tags-refspec &&
-	git tag sometag main &&
-	git update-ref refs/remotes/origin/foo/otherbranch main &&
-	git update-ref refs/remotes/origin/extrabranch main &&
-
-	git fetch --prune --tags origin refs/heads/foo/*:refs/remotes/origin/foo/* &&
-	test_must_fail git rev-parse refs/remotes/origin/foo/otherbranch &&
-	git rev-parse origin/extrabranch &&
-	git rev-parse sometag
+	(
+		cd prune-tags-refspec &&
+		git tag sometag main &&
+		git update-ref refs/remotes/origin/foo/otherbranch main &&
+		git update-ref refs/remotes/origin/extrabranch main &&
+
+		git fetch --prune --tags origin refs/heads/foo/*:refs/remotes/origin/foo/* &&
+		test_must_fail git rev-parse refs/remotes/origin/foo/otherbranch &&
+		git rev-parse origin/extrabranch &&
+		git rev-parse sometag
+	)
 '
 
 test_expect_success 'fetch --tags gets tags even without a configured remote' '
@@ -381,21 +383,21 @@ test_expect_success 'fetch --tags gets tags even without a configured remote' '
 '
 
 test_expect_success REFFILES 'fetch --prune fails to delete branches' '
-	cd "$D" &&
 	git clone . prune-fail &&
-	cd prune-fail &&
-	git update-ref refs/remotes/origin/extrabranch main &&
-	git pack-refs --all &&
-	: this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds  &&
-	>.git/packed-refs.new &&
+	(
+		cd prune-fail &&
+		git update-ref refs/remotes/origin/extrabranch main &&
+		git pack-refs --all &&
+		: this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds  &&
+		>.git/packed-refs.new &&
 
-	test_must_fail git fetch --prune origin
+		test_must_fail git fetch --prune origin
+	)
 '
 
 test_expect_success 'fetch --atomic works with a single branch' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&
 
-	cd "$D" &&
 	git clone . atomic &&
 	git branch atomic-branch &&
 	oid=$(git rev-parse atomic-branch) &&
@@ -408,9 +410,8 @@ test_expect_success 'fetch --atomic works with a single branch' '
 '
 
 test_expect_success 'fetch --atomic works with multiple branches' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&
 
-	cd "$D" &&
 	git clone . atomic &&
 	git branch atomic-branch-1 &&
 	git branch atomic-branch-2 &&
@@ -423,9 +424,8 @@ test_expect_success 'fetch --atomic works with multiple branches' '
 '
 
 test_expect_success 'fetch --atomic works with mixed branches and tags' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&
 
-	cd "$D" &&
 	git clone . atomic &&
 	git branch atomic-mixed-branch &&
 	git tag atomic-mixed-tag &&
@@ -437,9 +437,8 @@ test_expect_success 'fetch --atomic works with mixed branches and tags' '
 '
 
 test_expect_success 'fetch --atomic prunes references' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&
 
-	cd "$D" &&
 	git branch atomic-prune-delete &&
 	git clone . atomic &&
 	git branch --delete atomic-prune-delete &&
@@ -453,9 +452,8 @@ test_expect_success 'fetch --atomic prunes references' '
 '
 
 test_expect_success 'fetch --atomic aborts with non-fast-forward update' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&
 
-	cd "$D" &&
 	git branch atomic-non-ff &&
 	git clone . atomic &&
 	git rev-parse HEAD >actual &&
@@ -472,9 +470,8 @@ test_expect_success 'fetch --atomic aborts with non-fast-forward update' '
 '
 
 test_expect_success 'fetch --atomic executes a single reference transaction only' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&
 
-	cd "$D" &&
 	git clone . atomic &&
 	git branch atomic-hooks-1 &&
 	git branch atomic-hooks-2 &&
@@ -499,9 +496,8 @@ test_expect_success 'fetch --atomic executes a single reference transaction only
 '
 
 test_expect_success 'fetch --atomic aborts all reference updates if hook aborts' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&
 
-	cd "$D" &&
 	git clone . atomic &&
 	git branch atomic-hooks-abort-1 &&
 	git branch atomic-hooks-abort-2 &&
@@ -536,9 +532,8 @@ test_expect_success 'fetch --atomic aborts all reference updates if hook aborts'
 '
 
 test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '
-	test_when_finished "rm -rf \"$D\"/atomic" &&
+	test_when_finished "rm -rf atomic" &&
 
-	cd "$D" &&
 	git clone . atomic &&
 	oid=$(git rev-parse HEAD) &&
 
@@ -574,8 +569,7 @@ test_expect_success REFFILES 'fetch --atomic fails transaction if reference lock
 '
 
 test_expect_success '--refmap="" ignores configured refspec' '
-	cd "$TRASH_DIRECTORY" &&
-	git clone "$D" remote-refs &&
+	git clone . remote-refs &&
 	git -C remote-refs rev-parse remotes/origin/main >old &&
 	git -C remote-refs update-ref refs/remotes/origin/main main~1 &&
 	git -C remote-refs rev-parse remotes/origin/main >new &&
@@ -599,34 +593,26 @@ test_expect_success '--refmap="" and --prune' '
 
 test_expect_success 'fetch tags when there is no tags' '
 
-    cd "$D" &&
-
-    mkdir notags &&
-    cd notags &&
-    git init &&
-
-    git fetch -t ..
+	git init notags &&
+	git -C notags fetch -t ..
 
 '
 
 test_expect_success 'fetch following tags' '
 
-	cd "$D" &&
 	git tag -a -m "annotated" anno HEAD &&
 	git tag light HEAD &&
 
-	mkdir four &&
-	cd four &&
-	git init &&
-
-	git fetch .. :track &&
-	git show-ref --verify refs/tags/anno &&
-	git show-ref --verify refs/tags/light
-
+	git init four &&
+	(
+		cd four &&
+		git fetch .. :track &&
+		git show-ref --verify refs/tags/anno &&
+		git show-ref --verify refs/tags/light
+	)
 '
 
 test_expect_success 'fetch uses remote ref names to describe new refs' '
-	cd "$D" &&
 	git init descriptive &&
 	(
 		cd descriptive &&
@@ -654,30 +640,20 @@ test_expect_success 'fetch uses remote ref names to describe new refs' '
 
 test_expect_success 'fetch must not resolve short tag name' '
 
-	cd "$D" &&
-
-	mkdir five &&
-	cd five &&
-	git init &&
-
-	test_must_fail git fetch .. anno:five
+	git init five &&
+	test_must_fail git -C five fetch .. anno:five
 
 '
 
 test_expect_success 'fetch can now resolve short remote name' '
 
-	cd "$D" &&
 	git update-ref refs/remotes/six/HEAD HEAD &&
 
-	mkdir six &&
-	cd six &&
-	git init &&
-
-	git fetch .. six:six
+	git init six &&
+	git -C six fetch .. six:six
 '
 
 test_expect_success 'create bundle 1' '
-	cd "$D" &&
 	echo >file updated again by origin &&
 	git commit -a -m "tip" &&
 	git bundle create --version=3 bundle1 main^..main
@@ -691,35 +667,36 @@ test_expect_success 'header of bundle looks right' '
 	OID refs/heads/main
 
 	EOF
-	sed -e "s/$OID_REGEX/OID/g" -e "5q" "$D"/bundle1 >actual &&
+	sed -e "s/$OID_REGEX/OID/g" -e "5q" bundle1 >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'create bundle 2' '
-	cd "$D" &&
 	git bundle create bundle2 main~2..main
 '
 
 test_expect_success 'unbundle 1' '
-	cd "$D/bundle" &&
-	git checkout -b some-branch &&
-	test_must_fail git fetch "$D/bundle1" main:main
+	(
+		cd bundle &&
+		git checkout -b some-branch &&
+		test_must_fail git fetch bundle1 main:main
+	)
 '
 
 
 test_expect_success 'bundle 1 has only 3 files ' '
-	cd "$D" &&
 	test_bundle_object_count bundle1 3
 '
 
 test_expect_success 'unbundle 2' '
-	cd "$D/bundle" &&
-	git fetch ../bundle2 main:main &&
-	test "tip" = "$(git log -1 --pretty=oneline main | cut -d" " -f2)"
+	(
+		cd bundle &&
+		git fetch ../bundle2 main:main &&
+		test "tip" = "$(git log -1 --pretty=oneline main | cut -d" " -f2)"
+	)
 '
 
 test_expect_success 'bundle does not prerequisite objects' '
-	cd "$D" &&
 	touch file2 &&
 	git add file2 &&
 	git commit -m add.file2 file2 &&
@@ -729,7 +706,6 @@ test_expect_success 'bundle does not prerequisite objects' '
 
 test_expect_success 'bundle should be able to create a full history' '
 
-	cd "$D" &&
 	git tag -a -m "1.0" v1.0 main &&
 	git bundle create bundle4 v1.0
 
@@ -783,7 +759,6 @@ test_expect_success 'quoting of a strangely named repo' '
 
 test_expect_success 'bundle should record HEAD correctly' '
 
-	cd "$D" &&
 	git bundle create bundle5 HEAD main &&
 	git bundle list-heads bundle5 >actual &&
 	for h in HEAD refs/heads/main
@@ -803,7 +778,6 @@ test_expect_success 'mark initial state of origin/main' '
 
 test_expect_success 'explicit fetch should update tracking' '
 
-	cd "$D" &&
 	git branch -f side &&
 	(
 		cd three &&
@@ -818,7 +792,6 @@ test_expect_success 'explicit fetch should update tracking' '
 
 test_expect_success 'explicit pull should update tracking' '
 
-	cd "$D" &&
 	git branch -f side &&
 	(
 		cd three &&
@@ -832,15 +805,13 @@ test_expect_success 'explicit pull should update tracking' '
 '
 
 test_expect_success 'explicit --refmap is allowed only with command-line refspec' '
-	cd "$D" &&
 	(
 		cd three &&
 		test_must_fail git fetch --refmap="*:refs/remotes/none/*"
 	)
 '
 
 test_expect_success 'explicit --refmap option overrides remote.*.fetch' '
-	cd "$D" &&
 	git branch -f side &&
 	(
 		cd three &&
@@ -855,7 +826,6 @@ test_expect_success 'explicit --refmap option overrides remote.*.fetch' '
 '
 
 test_expect_success 'explicitly empty --refmap option disables remote.*.fetch' '
-	cd "$D" &&
 	git branch -f side &&
 	(
 		cd three &&
@@ -870,7 +840,6 @@ test_expect_success 'explicitly empty --refmap option disables remote.*.fetch' '
 
 test_expect_success 'configured fetch updates tracking' '
 
-	cd "$D" &&
 	git branch -f side &&
 	(
 		cd three &&
@@ -884,7 +853,6 @@ test_expect_success 'configured fetch updates tracking' '
 '
 
 test_expect_success 'non-matching refspecs do not confuse tracking update' '
-	cd "$D" &&
 	git update-ref refs/odd/location HEAD &&
 	(
 		cd three &&
@@ -901,14 +869,12 @@ test_expect_success 'non-matching refspecs do not confuse tracking update' '
 
 test_expect_success 'pushing nonexistent branch by mistake should not segv' '
 
-	cd "$D" &&
 	test_must_fail git push seven no:no
 
 '
 
 test_expect_success 'auto tag following fetches minimum' '
 
-	cd "$D" &&
 	git clone .git follow &&
 	git checkout HEAD^0 &&
 	(
@@ -1307,7 +1273,7 @@ test_expect_success 'fetch --prune prints the remotes url' '
 		cd only-prunes &&
 		git fetch --prune origin 2>&1 | head -n1 >../actual
 	) &&
-	echo "From ${D}/." >expect &&
+	echo "From $(pwd)/." >expect &&
 	test_cmp expect actual
 '
 
@@ -1357,14 +1323,14 @@ test_expect_success 'fetching with auto-gc does not lock up' '
 	echo "$*" &&
 	false
 	EOF
-	git clone "file://$D" auto-gc &&
+	git clone "file://$PWD" auto-gc &&
 	test_commit test2 &&
 	(
 		cd auto-gc &&
 		git config fetch.unpackLimit 1 &&
 		git config gc.autoPackLimit 1 &&
 		git config gc.autoDetach false &&
-		GIT_ASK_YESNO="$D/askyesno" git fetch --verbose >fetch.out 2>&1 &&
+		GIT_ASK_YESNO="$TRASH_DIRECTORY/askyesno" git fetch --verbose >fetch.out 2>&1 &&
 		test_grep "Auto packing the repository" fetch.out &&
 		! grep "Should I try again" fetch.out
 	)
-- 
2.51.0.326.gecbb38d78e

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

[Jeff King]

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).

 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

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

[SZEDER Gábor]

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

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

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

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

Unfortunately I tend to agree.  A few downsides I find a bit
problematic in the subshell solution are

 - The temporary files subshell creates sometimes are harder to follow

 	( cd there && git foo >../actual && ... ) &&
	test_cmp expect actual

   than they need to be.  With "git -C there", obviously paths used
   when they get created and used match:

	git -C there >actual &&
	test_cmp expect actual

 - Test framework helpers like test_when_finished and test_commit
   that rely on the global shell variables to keep track of the
   states do not work well inside subshells.

 - Some platforms have expensive forks.

but in a context that these are not huge problems, I tend to prefer
the "cd in a subshell" pattern over

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

especially where "-C there" is harder to spot.  If the above were

	git -C two do this &&
	git -C two do that >actual &&
	git -C two do something else &&

i.e., with aligned "-C two" to make it obvious that these are doing
their thing in the same other place, the tradeoff might have been
different, though.

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

[Jeff King]

On Mon, Aug 25, 2025 at 08:46:02AM -0700, Junio C Hamano wrote:

> > 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.
> 
> Unfortunately I tend to agree.  A few downsides I find a bit
> problematic in the subshell solution are
> [...]

OK, I am happy to drop that patch (3/4). The resulting change to the
final patch to match style would be:

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 6e8b741491..bac464a9ec 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -269,12 +269,16 @@ test_expect_success 'followRemoteHEAD does not kick in with refspecs' '
 '
 
 test_expect_success 'followRemoteHEAD create does not overwrite dangling symref' '
-	git -C two remote add -m does-not-exist custom-head ../one &&
-	test_config -C two remote.custom-head.followRemoteHEAD create &&
-	git -C two fetch custom-head &&
-	echo refs/remotes/custom-head/does-not-exist >expect &&
-	git -C two symbolic-ref refs/remotes/custom-head/HEAD >actual &&
-	test_cmp expect actual
+	test_when_finished "git -C two config unset remote.custom-head.followRemoteHEAD" &&
+	(
+		cd two &&
+		git remote add -m does-not-exist custom-head ../one &&
+		git config remote.custom-head.followRemoteHEAD create &&
+		git fetch custom-head &&
+		echo refs/remotes/custom-head/does-not-exist >expect &&
+		git symbolic-ref refs/remotes/custom-head/HEAD >actual &&
+		test_cmp expect actual
+	)
 '
 
 test_expect_success 'fetch --prune on its own works as expected' '


But both patches are already in 'next'. How do you want to proceed? I
can prepare a patch on top converting back to sub-shells. Or if we are
going to do the post-release rewind of next, that is an opportunity to
fix things cleanly. Or we could leave it as-is if it is not worth the
bother at this point.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Unfortunately I tend to agree.  A few downsides I find a bit
>> problematic in the subshell solution are
>> [...]
>
> OK, I am happy to drop that patch (3/4). The resulting change to the
> final patch to match style would be:
> ...
> But both patches are already in 'next'. How do you want to proceed? I
> can prepare a patch on top converting back to sub-shells. Or if we are
> going to do the post-release rewind of next, that is an opportunity to
> fix things cleanly. Or we could leave it as-is if it is not worth the
> bother at this point.

The last one ;-).  This is the kind of preference that falls into
"once the code is written in one way, it is not worth the patch
noise to rewrite it in the other way" category.

[PATCH 4/4] refs: do not clobber dangling symrefs

[Jeff King]

When given an expected "before" state, the ref-writing code will avoid
overwriting any ref that does not match that expected state. We use the
null oid as a sentinel value for "nothing should exist", and likewise
that is the sentinel value we get when trying to read a ref that does
not exist.

But there's one corner case where this is ambiguous: dangling symrefs.
Trying to read them will yield the null oid, but there is potentially
something of value there: the dangling symref itself.

For a normal recursive write, this is OK. Imagine we have a symref
"FOO_HEAD" that points to a ref "refs/heads/bar" that does not exist,
and we try to write to it with a create operation like:

  oid=$(git rev-parse HEAD) ;# or whatever
  git symbolic-ref FOO_HEAD refs/heads/bar
  echo "create FOO_HEAD $oid" | git update-ref --stdin

The attempt to resolve FOO_HEAD will actually resolve "bar", yielding
the null oid. That matches our expectation, and the write proceeds. This
is correct, because we are not writing FOO_HEAD at all, but writing its
destination "bar", which in fact does not exist.

But what if the operation asked not to dereference symrefs? Like this:

  echo "create FOO_HEAD $oid" | git update-ref --no-deref --stdin

Resolving FOO_HEAD would still result in a null oid, and the write will
proceed. But it will overwrite FOO_HEAD itself, removing the fact that
it ever pointed to "bar".

This case is a little esoteric; we are clobbering a symref with a
no-deref write of a regular ref value. But the same problem occurs when
writing symrefs. For example:

  echo "symref-create FOO_HEAD refs/heads/other" |
  git update-ref --no-deref --stdin

The "create" operation asked us to create FOO_HEAD only if it did not
exist. But we silently overwrite the existing value.

You can trigger this without using update-ref via the fetch
followRemoteHEAD code. In "create" mode, it should not overwrite an
existing value. But if you manually create a symref pointing to a value
that does not yet exist (either via symbolic-ref or with "remote add
-m"), create mode will happily overwrite it.

Instead, we should detect this case and refuse to write. The correct
specification to overwrite FOO_HEAD in this case is to provide an
expected target ref value, like:

  echo "symref-update FOO_HEAD refs/heads/other ref refs/heads/bar" |
  git update-ref --no-deref --stdin

Note that the non-symref "update" directive does not allow you to do
this (you can only specify an oid). This is a weakness in the update-ref
interface, and you'd have to overwrite unconditionally, like:

  echo "update FOO_HEAD $oid" | git update-ref --no-deref --stdin

Likewise other symref operations like symref-delete do not accept the
"ref" keyword. You should be able to do:

  echo "symref-delete FOO_HEAD ref refs/heads/bar"

but cannot (and can only delete unconditionally). This patch doesn't
address those gaps. We may want to do so in a future patch for
completeness, but it's not clear if anybody actually wants to perform
those operations. The symref update case (specifically, via
followRemoteHEAD) is what I ran into in the wild.

The code for the fix is relatively straight-forward given the discussion
above. But note that we have to implement it independently for the files
and reftable backends. The "old oid" checks happen as part of the
locking process, which is implemented separately for each system. We may
want to factor this out somehow, but it's beyond the scope of this
patch. (Another curiosity is that the messages in the reftable code are
marked for translation, but the ones in the files backend are not. I
followed local convention in each case, but we may want to harmonize
this at some point).

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c    | 34 ++++++++++++++++++++++++++++++----
 refs/reftable-backend.c | 30 +++++++++++++++++++++++++++---
 t/t1400-update-ref.sh   | 21 +++++++++++++++++++++
 t/t5510-fetch.sh        |  9 +++++++++
 4 files changed, 87 insertions(+), 7 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 905555365b..a4419ef62d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2512,13 +2512,37 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,
  */
 static enum ref_transaction_error check_old_oid(struct ref_update *update,
 						struct object_id *oid,
+						struct strbuf *referent,
 						struct strbuf *err)
 {
 	if (update->flags & REF_LOG_ONLY ||
-	    !(update->flags & REF_HAVE_OLD) ||
-	    oideq(oid, &update->old_oid))
+	    !(update->flags & REF_HAVE_OLD))
 		return 0;
 
+	if (oideq(oid, &update->old_oid)) {
+		/*
+		 * Normally matching the expected old oid is enough. Either we
+		 * found the ref at the expected state, or we are creating and
+		 * expect the null oid (and likewise found nothing).
+		 *
+		 * But there is one exception for the null oid: if we found a
+		 * symref pointing to nothing we'll also get the null oid. In
+		 * regular recursive mode, that's good (we'll write to what the
+		 * symref points to, which doesn't exist). But in no-deref
+		 * mode, it means we'll clobber the symref, even though the
+		 * caller asked for this to be a creation event. So flag
+		 * that case to preserve the dangling symref.
+		 */
+		if ((update->flags & REF_NO_DEREF) && referent->len &&
+		    is_null_oid(oid)) {
+			strbuf_addf(err, "cannot lock ref '%s': "
+				    "dangling symref already exists",
+				    ref_update_original_update_refname(update));
+			return REF_TRANSACTION_ERROR_CREATE_EXISTS;
+		}
+		return 0;
+	}
+
 	if (is_null_oid(&update->old_oid)) {
 		strbuf_addf(err, "cannot lock ref '%s': "
 			    "reference already exists",
@@ -2658,7 +2682,8 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re
 			if (update->old_target)
 				ret = ref_update_check_old_target(referent.buf, update, err);
 			else
-				ret = check_old_oid(update, &lock->old_oid, err);
+				ret = check_old_oid(update, &lock->old_oid,
+						    &referent, err);
 			if (ret)
 				goto out;
 		} else {
@@ -2690,7 +2715,8 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re
 			ret = REF_TRANSACTION_ERROR_EXPECTED_SYMREF;
 			goto out;
 		} else {
-			ret = check_old_oid(update, &lock->old_oid, err);
+			ret = check_old_oid(update, &lock->old_oid,
+					    &referent, err);
 			if  (ret) {
 				goto out;
 			}
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 99fafd75eb..ef98584bf9 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1272,9 +1272,33 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
 		ret = ref_update_check_old_target(referent->buf, u, err);
 		if (ret)
 			return ret;
-	} else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD &&
-		   !oideq(&current_oid, &u->old_oid)) {
-		if (is_null_oid(&u->old_oid)) {
+	} else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD) {
+		if (oideq(&current_oid, &u->old_oid)) {
+			/*
+			 * Normally matching the expected old oid is enough. Either we
+			 * found the ref at the expected state, or we are creating and
+			 * expect the null oid (and likewise found nothing).
+			 *
+			 * But there is one exception for the null oid: if we found a
+			 * symref pointing to nothing we'll also get the null oid. In
+			 * regular recursive mode, that's good (we'll write to what the
+			 * symref points to, which doesn't exist). But in no-deref
+			 * mode, it means we'll clobber the symref, even though the
+			 * caller asked for this to be a creation event. So flag
+			 * that case to preserve the dangling symref.
+			 *
+			 * Everything else is OK and we can fall through to the
+			 * end of the conditional chain.
+			 */
+			if ((u->flags & REF_NO_DEREF) &&
+			    referent->len &&
+			    is_null_oid(&u->old_oid)) {
+				strbuf_addf(err, _("cannot lock ref '%s': "
+					    "dangling symref already exists"),
+					    ref_update_original_update_refname(u));
+				return REF_TRANSACTION_ERROR_CREATE_EXISTS;
+			}
+		} else if (is_null_oid(&u->old_oid)) {
 			strbuf_addf(err, _("cannot lock ref '%s': "
 					   "reference already exists"),
 				    ref_update_original_update_refname(u));
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index d29d23cb89..29b31e3b9b 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -2310,4 +2310,25 @@ test_expect_success 'update-ref should also create reflog for HEAD' '
 	test_cmp expect actual
 '
 
+test_expect_success 'dangling symref not overwritten by creation' '
+	test_when_finished "git update-ref -d refs/heads/dangling" &&
+	git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
+	test_must_fail git update-ref --no-deref --stdin 2>err <<-\EOF &&
+	create refs/heads/dangling HEAD
+	EOF
+	test_grep "cannot lock.*dangling symref already exists" err &&
+	test_must_fail git rev-parse --verify refs/heads/dangling &&
+	test_must_fail git rev-parse --verify refs/heads/does-not-exist
+'
+
+test_expect_success 'dangling symref overwritten without old oid' '
+	test_when_finished "git update-ref -d refs/heads/dangling" &&
+	git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
+	git update-ref --no-deref --stdin <<-\EOF &&
+	update refs/heads/dangling HEAD
+	EOF
+	git rev-parse --verify refs/heads/dangling &&
+	test_must_fail git rev-parse --verify refs/heads/does-not-exist
+'
+
 test_done
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 24379ec7aa..83d1aadf9f 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -232,6 +232,15 @@ test_expect_success 'followRemoteHEAD does not kick in with refspecs' '
 	test_cmp expect actual
 '
 
+test_expect_success 'followRemoteHEAD create does not overwrite dangling symref' '
+	git -C two remote add -m does-not-exist custom-head ../one &&
+	test_config -C two remote.custom-head.followRemoteHEAD create &&
+	git -C two fetch custom-head &&
+	echo refs/remotes/custom-head/does-not-exist >expect &&
+	git -C two symbolic-ref refs/remotes/custom-head/HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'fetch --prune on its own works as expected' '
 	git clone . prune &&
 	(
-- 
2.51.0.326.gecbb38d78e

Re: [PATCH 4/4] refs: do not clobber dangling symrefs

[Patrick Steinhardt]

On Tue, Aug 19, 2025 at 03:29:34PM -0400, Jeff King wrote:
> The code for the fix is relatively straight-forward given the discussion
> above. But note that we have to implement it independently for the files
> and reftable backends. The "old oid" checks happen as part of the
> locking process, which is implemented separately for each system. We may
> want to factor this out somehow, but it's beyond the scope of this
> patch.

Yeah, there's a bunch of duplication here in general. I originally
wanted to refactor this at some point in time, but I never got around to
it. Also because I kind of shied away from it: the logic to lock and
check refs is quite intertwined with one another in both backends, so I
was afraid that this would ulmitately lead to splitting hairs.

> (Another curiosity is that the messages in the reftable code are
> marked for translation, but the ones in the files backend are not. I
> followed local convention in each case, but we may want to harmonize
> this at some point).

Oh, interesting. I guess translating these messages is the right thing
to do, as the messages are user facing. But this definitely does not
have to be part of this patch series.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 905555365b..a4419ef62d 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2512,13 +2512,37 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,
>   */
>  static enum ref_transaction_error check_old_oid(struct ref_update *update,
>  						struct object_id *oid,
> +						struct strbuf *referent,
>  						struct strbuf *err)
>  {
>  	if (update->flags & REF_LOG_ONLY ||
> -	    !(update->flags & REF_HAVE_OLD) ||
> -	    oideq(oid, &update->old_oid))
> +	    !(update->flags & REF_HAVE_OLD))
>  		return 0;
>  
> +	if (oideq(oid, &update->old_oid)) {
> +		/*
> +		 * Normally matching the expected old oid is enough. Either we
> +		 * found the ref at the expected state, or we are creating and
> +		 * expect the null oid (and likewise found nothing).
> +		 *
> +		 * But there is one exception for the null oid: if we found a
> +		 * symref pointing to nothing we'll also get the null oid. In
> +		 * regular recursive mode, that's good (we'll write to what the
> +		 * symref points to, which doesn't exist). But in no-deref
> +		 * mode, it means we'll clobber the symref, even though the
> +		 * caller asked for this to be a creation event. So flag
> +		 * that case to preserve the dangling symref.
> +		 */
> +		if ((update->flags & REF_NO_DEREF) && referent->len &&
> +		    is_null_oid(oid)) {
> +			strbuf_addf(err, "cannot lock ref '%s': "
> +				    "dangling symref already exists",
> +				    ref_update_original_update_refname(update));
> +			return REF_TRANSACTION_ERROR_CREATE_EXISTS;
> +		}
> +		return 0;
> +	}
> +
>  	if (is_null_oid(&update->old_oid)) {
>  		strbuf_addf(err, "cannot lock ref '%s': "
>  			    "reference already exists",

Makes sense. If we've got an all-zero old object ID _but_ the locked
reference points to a nonexistet ref we refuse the update.

> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 99fafd75eb..ef98584bf9 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1272,9 +1272,33 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
>  		ret = ref_update_check_old_target(referent->buf, u, err);
>  		if (ret)
>  			return ret;
> -	} else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD &&
> -		   !oideq(&current_oid, &u->old_oid)) {
> -		if (is_null_oid(&u->old_oid)) {
> +	} else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD) {
> +		if (oideq(&current_oid, &u->old_oid)) {
> +			/*
> +			 * Normally matching the expected old oid is enough. Either we
> +			 * found the ref at the expected state, or we are creating and
> +			 * expect the null oid (and likewise found nothing).
> +			 *
> +			 * But there is one exception for the null oid: if we found a
> +			 * symref pointing to nothing we'll also get the null oid. In
> +			 * regular recursive mode, that's good (we'll write to what the
> +			 * symref points to, which doesn't exist). But in no-deref
> +			 * mode, it means we'll clobber the symref, even though the
> +			 * caller asked for this to be a creation event. So flag
> +			 * that case to preserve the dangling symref.
> +			 *
> +			 * Everything else is OK and we can fall through to the
> +			 * end of the conditional chain.
> +			 */
> +			if ((u->flags & REF_NO_DEREF) &&
> +			    referent->len &&
> +			    is_null_oid(&u->old_oid)) {
> +				strbuf_addf(err, _("cannot lock ref '%s': "
> +					    "dangling symref already exists"),
> +					    ref_update_original_update_refname(u));
> +				return REF_TRANSACTION_ERROR_CREATE_EXISTS;
> +			}
> +		} else if (is_null_oid(&u->old_oid)) {

Wouldn't it be more natural to put the new check into this `if
(is_null_oid(&u->old_oid))` branch? Makes it a bit more explicit that we
really only care about the case where we expect the ref to not exist.

Ah, no. I missed that you also change the original condition and move
the `oideq()` call into the whole thing. Makes sense.

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index d29d23cb89..29b31e3b9b 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -2310,4 +2310,25 @@ test_expect_success 'update-ref should also create reflog for HEAD' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'dangling symref not overwritten by creation' '
> +	test_when_finished "git update-ref -d refs/heads/dangling" &&
> +	git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
> +	test_must_fail git update-ref --no-deref --stdin 2>err <<-\EOF &&
> +	create refs/heads/dangling HEAD
> +	EOF
> +	test_grep "cannot lock.*dangling symref already exists" err &&
> +	test_must_fail git rev-parse --verify refs/heads/dangling &&
> +	test_must_fail git rev-parse --verify refs/heads/does-not-exist
> +'
> +
> +test_expect_success 'dangling symref overwritten without old oid' '
> +	test_when_finished "git update-ref -d refs/heads/dangling" &&
> +	git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
> +	git update-ref --no-deref --stdin <<-\EOF &&
> +	update refs/heads/dangling HEAD
> +	EOF
> +	git rev-parse --verify refs/heads/dangling &&
> +	test_must_fail git rev-parse --verify refs/heads/does-not-exist

Do we also want to verify that the dangling symref got converted into a
normal ref? Or do we already have other tests that do so?

Patrick

Re: [PATCH 4/4] refs: do not clobber dangling symrefs

[Jeff King]

On Wed, Aug 20, 2025 at 09:27:28AM +0200, Patrick Steinhardt wrote:

> > (Another curiosity is that the messages in the reftable code are
> > marked for translation, but the ones in the files backend are not. I
> > followed local convention in each case, but we may want to harmonize
> > this at some point).
> 
> Oh, interesting. I guess translating these messages is the right thing
> to do, as the messages are user facing. But this definitely does not
> have to be part of this patch series.

I know we left some unpack-trees messages untranslated because we
thought users might depend on them (see unpack_plumbing_errors). I
wondered if we might have done the same for the ref messages, but
there's certainly no infrastructure around it. So it may just have been
the case that nobody (yet) bothered to mark them.

> > +	} else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD) {
> > +		if (oideq(&current_oid, &u->old_oid)) {
> > +			/*
> > +			 * Normally matching the expected old oid is enough. Either we
> > +			 * found the ref at the expected state, or we are creating and
> > +			 * expect the null oid (and likewise found nothing).
> > +			 *
> > +			 * But there is one exception for the null oid: if we found a
> > +			 * symref pointing to nothing we'll also get the null oid. In
> > +			 * regular recursive mode, that's good (we'll write to what the
> > +			 * symref points to, which doesn't exist). But in no-deref
> > +			 * mode, it means we'll clobber the symref, even though the
> > +			 * caller asked for this to be a creation event. So flag
> > +			 * that case to preserve the dangling symref.
> > +			 *
> > +			 * Everything else is OK and we can fall through to the
> > +			 * end of the conditional chain.
> > +			 */
> > +			if ((u->flags & REF_NO_DEREF) &&
> > +			    referent->len &&
> > +			    is_null_oid(&u->old_oid)) {
> > +				strbuf_addf(err, _("cannot lock ref '%s': "
> > +					    "dangling symref already exists"),
> > +					    ref_update_original_update_refname(u));
> > +				return REF_TRANSACTION_ERROR_CREATE_EXISTS;
> > +			}
> > +		} else if (is_null_oid(&u->old_oid)) {
> 
> Wouldn't it be more natural to put the new check into this `if
> (is_null_oid(&u->old_oid))` branch? Makes it a bit more explicit that we
> really only care about the case where we expect the ref to not exist.
> 
> Ah, no. I missed that you also change the original condition and move
> the `oideq()` call into the whole thing. Makes sense.

Yep, exactly. If we did the is_null_oid() check first then we'd have to
check oideq() again inside that block, duplicating that logic. So there
is no winning. :) I tried to keep the logic as close to the original as
possible.

> > +test_expect_success 'dangling symref overwritten without old oid' '
> > +	test_when_finished "git update-ref -d refs/heads/dangling" &&
> > +	git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
> > +	git update-ref --no-deref --stdin <<-\EOF &&
> > +	update refs/heads/dangling HEAD
> > +	EOF
> > +	git rev-parse --verify refs/heads/dangling &&
> > +	test_must_fail git rev-parse --verify refs/heads/does-not-exist
> 
> Do we also want to verify that the dangling symref got converted into a
> normal ref? Or do we already have other tests that do so?

My intent was that the above test does so: we know that "dangling" now
points to a valid oid and that "does-not-exist" was not written to.
Ergo, "dangling" is now a normal ref (the only other option is that it
remained a symref and was pointed somewhere else entirely, but that
seems like an unlikely bug to have).

-Peff

Re: [PATCH 4/4] refs: do not clobber dangling symrefs

[Toon Claes]

Hi Peff,

At $DAYJOB we hit into an edge-case where this patch breaks our expectancies.

We use `update FOO_HEAD 000...000 000..000` to delete a symref, if that symref
is dangling (otherwise the old oid would have resolved to something). I've
attached a patch that would allow this (on top of your patches). Do you think it
makes sense to allow this scenario?

--
Cheers,
Toon


--- >8 ---
Subject: [PATCH] refs: allow deleting dangling symrefs by updating to zero oid

In 450fc2bace (refs: do not clobber dangling symrefs, 2025-08-19) we
changed how dangling symrefs are dealt with. This guards us from
creating a symref, while it already exists.

But this breaks behavior when you want to delete such dangling symref.
When you're aware your symref is dangling, you know the old oid resolves
to the null oid, and thus you can pass that together with the null oid
the new oid to delete the symref. Thus when the new oid is the null oid,
continue the ref update as before the change mentioned earlier.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 refs/files-backend.c    | 2 +-
 refs/reftable-backend.c | 2 +-
 t/t1400-update-ref.sh   | 9 +++++++++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1b3bf26add..5e46d3a110 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2537,7 +2537,7 @@ static enum ref_transaction_error check_old_oid(struct ref_update *update,
 		 * that case to preserve the dangling symref.
 		 */
 		if ((update->flags & REF_NO_DEREF) && referent->len &&
-		    is_null_oid(oid)) {
+		    is_null_oid(oid) && !is_null_oid(&update->new_oid)) {
 			strbuf_addf(err, "cannot lock ref '%s': "
 				    "dangling symref already exists",
 				    ref_update_original_update_refname(update));
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 9e889da2ff..ed505f6054 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1294,7 +1294,7 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
 			 */
 			if ((u->flags & REF_NO_DEREF) &&
 			    referent->len &&
-			    is_null_oid(&u->old_oid)) {
+			    is_null_oid(&u->old_oid) && !is_null_oid(&u->new_oid)) {
 				strbuf_addf(err, _("cannot lock ref '%s': "
 					    "dangling symref already exists"),
 					    ref_update_original_update_refname(u));
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index b7415ec9d5..85cd9da0af 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -2389,4 +2389,13 @@ test_expect_success 'dangling symref overwritten without old oid' '
 	test_must_fail git rev-parse --verify refs/heads/does-not-exist
 '

+test_expect_success 'dangling symref delete with old oid zero' '
+	test_when_finished "git update-ref -d refs/heads/dangling" &&
+	git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
+	echo "update refs/heads/dangling $Z $Z" >stdin &&
+	git update-ref --no-deref --stdin <stdin &&
+	test_must_fail git rev-parse --verify refs/heads/dangling &&
+	test_must_fail git rev-parse --verify refs/heads/does-not-exist
+'
+
 test_done
--
2.51.0

Re: [PATCH 4/4] refs: do not clobber dangling symrefs

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> We use `update FOO_HEAD 000...000 000..000` to delete a symref, if that symref
> is dangling (otherwise the old oid would have resolved to something). I've
> attached a patch that would allow this (on top of your patches). Do you think it
> makes sense to allow this scenario?
> ...
> +	test_when_finished "git update-ref -d refs/heads/dangling" &&
> +	git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
> +	echo "update refs/heads/dangling $Z $Z" >stdin &&
> +	git update-ref --no-deref --stdin <stdin &&

"git update-ref --help" seems to show that the "--stdin" mode has a
separate command that is designed for exactly the purpose of removing
a symbolic ref, though.  If you are changing the semantics of "update"
to make it safer while dealing with a dangling symbolic ref, do you
also need to touch the code path that handles "symref-delete" command?

> +	test_must_fail git rev-parse --verify refs/heads/dangling &&
> +	test_must_fail git rev-parse --verify refs/heads/does-not-exist
> +'
> +
>  test_done
> --
> 2.51.0

Re: [PATCH 4/4] refs: do not clobber dangling symrefs

[Jeff King]

On Mon, Sep 22, 2025 at 08:54:34AM -0700, Junio C Hamano wrote:

> Toon Claes <toon@iotcl.com> writes:
> 
> > We use `update FOO_HEAD 000...000 000..000` to delete a symref, if that symref
> > is dangling (otherwise the old oid would have resolved to something). I've
> > attached a patch that would allow this (on top of your patches). Do you think it
> > makes sense to allow this scenario?
> > ...
> > +	test_when_finished "git update-ref -d refs/heads/dangling" &&
> > +	git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
> > +	echo "update refs/heads/dangling $Z $Z" >stdin &&
> > +	git update-ref --no-deref --stdin <stdin &&
> 
> "git update-ref --help" seems to show that the "--stdin" mode has a
> separate command that is designed for exactly the purpose of removing
> a symbolic ref, though.  If you are changing the semantics of "update"
> to make it safer while dealing with a dangling symbolic ref, do you
> also need to touch the code path that handles "symref-delete" command?

I don't think so. Whatever we are trying to write (whether a regular
ref, a symref, or a deletion), the "check the old value" code path ends
up in the same place.

IMHO the directives for "update-ref --stdin" are a bit mis-designed.
All of update/delete/verify should accept either "old-oid" or
"old-target" (you do not need it for create, which always implies an
old-oid of all-zeroes).

And then symref-* is used when you want the _new_ thing to be a symref.
So symref-delete is not needed at all. You just have symref-* directives
for create/update/verify. Which almost could be replaced by "ref
<new-target>", but IIRC there was some syntactic ambiguity (because we
allow new-target to be a ref, so you'd have to pick some invalid name
like ":symref").

It is probably too late now to switch from "symref-update foo" to
"update :ref foo" (and again, I think that may have even been considered
and rejected). But we could add support for "ref <old-target>" to the
non-symref commands. That is not just a syntactic weakness, but
something you literally _can't_ do now (convert a symref into a regular
ref atomically).

Anyway, all very off-topic for Toon's issue, though. I think his patch
as-is does the right thing for his case, if we want to loosen it for
historical reasons (see my other response).

-Peff

Re: [PATCH 4/4] refs: do not clobber dangling symrefs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... But we could add support for "ref <old-target>" to the
> non-symref commands. That is not just a syntactic weakness, but
> something you literally _can't_ do now (convert a symref into a regular
> ref atomically).

OK.  Your explanation makes sense.

Re: [PATCH 4/4] refs: do not clobber dangling symrefs

[Jeff King]

On Mon, Sep 22, 2025 at 02:23:32PM +0200, Toon Claes wrote:

> At $DAYJOB we hit into an edge-case where this patch breaks our expectancies.
> 
> We use `update FOO_HEAD 000...000 000..000` to delete a symref, if that symref
> is dangling (otherwise the old oid would have resolved to something). I've
> attached a patch that would allow this (on top of your patches). Do you think it
> makes sense to allow this scenario?

Hmm. That's a funny command. You are providing _two_ null oids. The
first one says "this should be a deletion" and the second one says "the
previous state is that this should be deleted". So it should always be a
noop, if we are checking both sides.

I think the "right" way to say that is just:

  update FOO_HEAD 000...000

with no old-oid field at all. Or just:

  delete FOO_HEAD

but the two are internally the same thing.

So I think allowing this is working against what the patch is trying to
do, which is to consistently enforce the old-oid match that the user
asked for. The only thing that makes it an oddball is that it is
inherently a broken thing to ask for in the first place (at least under
the new, enforced regime). So we could perhaps allow it as a special
case for historical reasons without hurting anybody too badly.

I'd prefer not to do that, just because the refs code is already
complicated enough. But whether that's practical would depend on how
widespread this pattern is. Presumably it would not be that big a deal
to fix what you're sending (and assuming this is Gitaly, I'd guess that
it is bundled along with Git, so you are not that worried about people
using new Git with old Gitaly). But I'm not sure how we'd find out if
other people are doing the same thing in the wild.

So I dunno. My inclination is to say that the double-null-oid invocation
is weird and wrong, and callers should update if they need to. But I
could be convinced otherwise.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 1b3bf26add..5e46d3a110 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2537,7 +2537,7 @@ static enum ref_transaction_error check_old_oid(struct ref_update *update,
>  		 * that case to preserve the dangling symref.
>  		 */
>  		if ((update->flags & REF_NO_DEREF) && referent->len &&
> -		    is_null_oid(oid)) {
> +		    is_null_oid(oid) && !is_null_oid(&update->new_oid)) {
>  			strbuf_addf(err, "cannot lock ref '%s': "
>  				    "dangling symref already exists",
>  				    ref_update_original_update_refname(update));

I think the implementation here (and the matching one in the reftable
code) is correct for what you want to do. We should probably note the
special case in the comment above, too.

-Peff

Re: [PATCH 4/4] refs: do not clobber dangling symrefs

[Toon Claes]

Jeff King <peff@peff.net> writes:

> So I dunno. My inclination is to say that the double-null-oid invocation
> is weird and wrong, and callers should update if they need to. But I
> could be convinced otherwise.

Thanks for your feedback, and I have to agree. I'll get in touch with
the Gitaly team to see if we can rid of this odd invocation. For the
record, this conversation has been happening here[1].

[1]: https://gitlab.com/gitlab-org/gitaly/-/merge_requests/8161#note_2767808133


-- 
Cheers,
Toon

Re: [PATCH 4/4] refs: do not clobber dangling symrefs

[Jeff King]

On Tue, Sep 23, 2025 at 11:36:51AM +0200, Toon Claes wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So I dunno. My inclination is to say that the double-null-oid invocation
> > is weird and wrong, and callers should update if they need to. But I
> > could be convinced otherwise.
> 
> Thanks for your feedback, and I have to agree. I'll get in touch with
> the Gitaly team to see if we can rid of this odd invocation. For the
> record, this conversation has been happening here[1].
> 
> [1]: https://gitlab.com/gitlab-org/gitaly/-/merge_requests/8161#note_2767808133

Looking over that conversation, I do think you might consider using
symref-delete. As noted there, doing "delete <dangling-symref>" is going
to delete unconditionally, whether it's a symref, a real ref, or nothing
is there at all.

If you know it's a symref pointing to "refs/heads/foo", then the safest
thing is:

  symref-delete FOO_HEAD refs/heads/foo

which guarantees the operation is doing what you expected.

There's an open question there of: how do I know what it's pointing to?
But that's kind of the point of the "old-target" (and "old-oid")
options. They take information you discovered previously non-atomically
and atomically perform the operation while checking (under lock) that
things haven't changed unexpectedly.

So from the test perspective, I think you just know what's in the test
fixture. From the Gitaly API perspective, the caller should have some
idea of what they're deleting (just like they should for a real ref).

-Peff