From: Eric Sunshine <ericsunshine@charter.net>
Date: Tue, 19 Aug 2025 20:03:23 GMT
Subject: Re: [PATCH 1/4] t5510: make confusing config cleanup more explicit
Message-ID: <8797c495-8277-4f65-845b-167542b82949@charter.net>
In-Reply-To: <20250819192455.GA1059295@coredump.intra.peff.net>

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