{"thread":{"id":"63993","subject":"[PATCH 0/4] dangling symrefs and fetchRemoteHEAD=create","startedAt":"2025-08-19T19:20:06Z","lastAt":"2025-09-23T17:33:31Z","messageCount":22,"participants":["Jeff King","Eric Sunshine","Patrick Steinhardt","SZEDER Gábor","Junio C Hamano","Toon Claes"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"524467","messageId":"20250819192004.GA1058857@coredump.intra.peff.net","threadId":"63993","inReplyTo":null,"subject":"[PATCH 0/4] dangling symrefs and fetchRemoteHEAD=create","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-19T19:20:04Z","receivedAt":"2025-08-19T19:20:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This fixes a bug I found while investigating another semi-related bug\n(that has already been fixed by Patrick), mentioned in the \"PS\" here:\n\n https://lore.kernel.org/git/20250724104536.GA1316505@coredump.intra.peff.net/\n\nThe issue is that:\n\n git remote add -m does-not-exist origin \n git config remote.origin.followRemoteHEAD create\n git fetch\n\nwill overwrite the refs/remotes/origin/HEAD we created, even though we\nasked it to do so only on creation. The issue is actually in the refs\ncode, and how it perceives dangling symrefs with respect to creation\nevents. And so this actually affects \"update-ref\", as well.\n\nA fix is in the final patch, along with a detailed explanation. The\nearlier patches are just cleanup of the related test script before we\nadd our new test there.\n\n [1/4]: t5510: make confusing config cleanup more explicit\n [2/4]: t5510: stop changing top-level working directory\n [3/4]: t5510: prefer \"git -C\" to subshell for followRemoteHEAD tests\n [4/4]: refs: do not clobber dangling symrefs\n\n refs/files-backend.c | 34 ++-\n refs/reftable-backend.c | 30 ++-\n t/t1400-update-ref.sh | 21 ++\n t/t5510-fetch.sh | 543 ++++++++++++++++++----------------------\n 4 files changed, 319 insertions(+), 309 deletions(-)\n\n-Peff\n"},{"id":"524468","messageId":"20250819192356.GA1059166@coredump.intra.peff.net","threadId":"63993","inReplyTo":"20250819192004.GA1058857@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] dangling symrefs and fetchRemoteHEAD=create","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-19T19:23:56Z","receivedAt":"2025-08-19T19:23:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 19, 2025 at 03:20:04PM -0400, Jeff King wrote:\n\n> A fix is in the final patch, along with a detailed explanation. The\n> earlier patches are just cleanup of the related test script before we\n> add our new test there.\n> \n> [1/4]: t5510: make confusing config cleanup more explicit\n> [2/4]: t5510: stop changing top-level working directory\n> [3/4]: t5510: prefer \"git -C\" to subshell for followRemoteHEAD tests\n> [4/4]: refs: do not clobber dangling symrefs\n\nOh, one thing I forgot to mention: this must be applied on top of\nps/reflog-migrate-fixes. Specifically the changes to check_old_oid() in\n046c67325c (refs: stop unsetting REF_HAVE_OLD for log-only updates,\n2025-08-06). Otherwise this logic kicks in for split-HEAD updates, which\nmakes no sense (if we are creating \"refs/heads/foo\" and HEAD happens to\npoint to that branch, we split off a reflog update of HEAD, but we\nshould not enforce any old-oid rules since we are not writing HEAD at\nall).\n\n-Peff\n"},{"id":"524469","messageId":"20250819192455.GA1059295@coredump.intra.peff.net","threadId":"63993","inReplyTo":"20250819192004.GA1058857@coredump.intra.peff.net","subject":"[PATCH 1/4] t5510: make confusing config cleanup more explicit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-19T19:24:55Z","receivedAt":"2025-08-19T19:24:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Several tests set a config variable in a sub-repo we chdir into via a\nsubshell, like this:\n\n (\n\tcd \"$D\" &&\n\tcd two &&\n\tgit config foo.bar baz\n )\n\nBut they also clean up the variable with a when_finished directive\noutside of the subshell, like this:\n\n test_when_finished \"git config unset foo.bar\"\n\nAt first glance, this shouldn't work! The cleanup clause cannot be run\nfrom the subshell (since environment changes there are lost by the time\nthe test snippet finishes). But since the cleanup command runs outside\nthe subshell, our working directory will not have been switched into\n\"two\".\n\nBut it does work. Why?\n\nThe answer is that an earlier test does a \"cd two\" that moves the whole\ntest's working directory out of $TRASH_DIRECTORY and into \"two\". So the\nsubshell is a bit of a red herring; we are already in the right\ndirectory! That's why we need the \"cd $D\" at the top of the shell, to\nput us back to a known spot.\n\nLet's make this cleanup code more explicitly specify where we expect the\nconfig command to run. That makes the script more robust against running\na subset of the tests, and ultimately will make it easier to refactor\nthe script to avoid these top-level chdirs.\n\nSigned-off-by: Jeff King \n---\n t/t5510-fetch.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex ebc696546b..64fea9f4a5 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -119,7 +119,7 @@ test_expect_success \"fetch test remote HEAD change\" '\n \ttest \"z$head\" = \"z$branch\"'\n \n test_expect_success \"fetch test followRemoteHEAD never\" '\n-\ttest_when_finished \"git config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n \t(\n \t\tcd \"$D\" &&\n \t\tcd two &&\n@@ -134,7 +134,7 @@ test_expect_success \"fetch test followRemoteHEAD never\" '\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn no change\" '\n-\ttest_when_finished \"git config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n \t(\n \t\tcd \"$D\" &&\n \t\tcd two &&\n@@ -154,7 +154,7 @@ test_expect_success \"fetch test followRemoteHEAD warn no change\" '\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn create\" '\n-\ttest_when_finished \"git config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n \t(\n \t\tcd \"$D\" &&\n \t\tcd two &&\n@@ -170,7 +170,7 @@ test_expect_success \"fetch test followRemoteHEAD warn create\" '\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn detached\" '\n-\ttest_when_finished \"git config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n \t(\n \t\tcd \"$D\" &&\n \t\tcd two &&\n@@ -187,7 +187,7 @@ test_expect_success \"fetch test followRemoteHEAD warn detached\" '\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn quiet\" '\n-\ttest_when_finished \"git config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n \t(\n \t\tcd \"$D\" &&\n \t\tcd two &&\n@@ -205,7 +205,7 @@ test_expect_success \"fetch test followRemoteHEAD warn quiet\" '\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn-if-not-branch branch is same\" '\n-\ttest_when_finished \"git config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n \t(\n \t\tcd \"$D\" &&\n \t\tcd two &&\n@@ -223,7 +223,7 @@ test_expect_success \"fetch test followRemoteHEAD warn-if-not-branch branch is sa\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn-if-not-branch branch is different\" '\n-\ttest_when_finished \"git config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n \t(\n \t\tcd \"$D\" &&\n \t\tcd two &&\n@@ -243,7 +243,7 @@ test_expect_success \"fetch test followRemoteHEAD warn-if-not-branch branch is di\n '\n \n test_expect_success \"fetch test followRemoteHEAD always\" '\n-\ttest_when_finished \"git config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n \t(\n \t\tcd \"$D\" &&\n \t\tcd two &&\n@@ -260,7 +260,7 @@ test_expect_success \"fetch test followRemoteHEAD always\" '\n '\n \n test_expect_success 'followRemoteHEAD does not kick in with refspecs' '\n-\ttest_when_finished \"git config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n \t(\n \t\tcd \"$D\" &&\n \t\tcd two &&\n-- \n2.51.0.326.gecbb38d78e\n\n"},{"id":"524470","messageId":"20250819192606.GB1059295@coredump.intra.peff.net","threadId":"63993","inReplyTo":"20250819192004.GA1058857@coredump.intra.peff.net","subject":"[PATCH 2/4] t5510: stop changing top-level working directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-19T19:26:06Z","receivedAt":"2025-08-19T19:26:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Several tests in t5510 do a bare \"cd subrepo\", not in a subshell. This\nchanges the working directory for subsequent tests. As a result, almost\nevery test has to start with \"cd $D\" to go back to the top-level.\n\nOur usual style is to do per-test environment changes like this in a\nsubshell, so that tests can assume they are starting at the top-level\n$TRASH_DIRECTORY.\n\nLet's switch to that style, which lets us drop all of that extra\npath-handling.\n\nMost cases can switch to using a subshell, but in a few spots we can\nsimplify by doing \"git init foo && git -C foo ...\". We do have to make\nsure that we weren't intentionally touching the environment in any code\nwhich was moved into a subshell (e.g., with a test_when_finished), but\nthat isn't the case for any of these tests.\n\nAll of the references to the $D variable can go away, replaced generally\nwith $PWD or $TRASH_DIRECTORY (if we use it inside a chdir'd subshell).\nNote in one test, \"fetch --prune prints the remotes url\", we make sure\nto use $(pwd) to get the Windows-style path on that platform (for the\nother tests, the exact form doesn't matter).\n\nSigned-off-by: Jeff King \n---\n t/t5510-fetch.sh | 356 +++++++++++++++++++++--------------------------\n 1 file changed, 161 insertions(+), 195 deletions(-)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 64fea9f4a5..93e309e213 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -14,8 +14,6 @@ then\n \ttest_done\n fi\n \n-D=$(pwd)\n-\n test_expect_success setup '\n \techo >file original &&\n \tgit add file &&\n@@ -51,46 +49,50 @@ test_expect_success \"clone and setup child repos\" '\n '\n \n test_expect_success \"fetch test\" '\n-\tcd \"$D\" &&\n \techo >file updated by origin &&\n \tgit commit -a -m \"updated by origin\" &&\n-\tcd two &&\n-\tgit fetch &&\n-\tgit rev-parse --verify refs/heads/one &&\n-\tmine=$(git rev-parse refs/heads/one) &&\n-\this=$(cd ../one && git rev-parse refs/heads/main) &&\n-\ttest \"z$mine\" = \"z$his\"\n+\t(\n+\t\tcd two &&\n+\t\tgit fetch &&\n+\t\tgit rev-parse --verify refs/heads/one &&\n+\t\tmine=$(git rev-parse refs/heads/one) &&\n+\t\this=$(cd ../one && git rev-parse refs/heads/main) &&\n+\t\ttest \"z$mine\" = \"z$his\"\n+\t)\n '\n \n test_expect_success \"fetch test for-merge\" '\n-\tcd \"$D\" &&\n-\tcd three &&\n-\tgit fetch &&\n-\tgit rev-parse --verify refs/heads/two &&\n-\tgit rev-parse --verify refs/heads/one &&\n-\tmain_in_two=$(cd ../two && git rev-parse main) &&\n-\tone_in_two=$(cd ../two && git rev-parse one) &&\n-\t{\n-\t\techo \"$one_in_two\t\" &&\n-\t\techo \"$main_in_two\tnot-for-merge\"\n-\t} >expected &&\n-\tcut -f -2 .git/FETCH_HEAD >actual &&\n-\ttest_cmp expected actual'\n+\t(\n+\t\tcd three &&\n+\t\tgit fetch &&\n+\t\tgit rev-parse --verify refs/heads/two &&\n+\t\tgit rev-parse --verify refs/heads/one &&\n+\t\tmain_in_two=$(cd ../two && git rev-parse main) &&\n+\t\tone_in_two=$(cd ../two && git rev-parse one) &&\n+\t\t{\n+\t\t\techo \"$one_in_two\t\" &&\n+\t\t\techo \"$main_in_two\tnot-for-merge\"\n+\t\t} >expected &&\n+\t\tcut -f -2 .git/FETCH_HEAD >actual &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n \n test_expect_success \"fetch test remote HEAD\" '\n-\tcd \"$D\" &&\n-\tcd two &&\n-\tgit fetch &&\n-\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n-\tgit rev-parse --verify refs/remotes/origin/main &&\n-\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n-\tbranch=$(git rev-parse refs/remotes/origin/main) &&\n-\ttest \"z$head\" = \"z$branch\"'\n+\t(\n+\t\tcd two &&\n+\t\tgit fetch &&\n+\t\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n+\t\tgit rev-parse --verify refs/remotes/origin/main &&\n+\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n+\t\tbranch=$(git rev-parse refs/remotes/origin/main) &&\n+\t\ttest \"z$head\" = \"z$branch\"\n+\t)\n+'\n \n test_expect_success \"fetch test remote HEAD in bare repository\" '\n \ttest_when_finished rm -rf barerepo &&\n \t(\n-\t\tcd \"$D\" &&\n \t\tgit init --bare barerepo &&\n \t\tcd barerepo &&\n \t\tgit remote add upstream ../two &&\n@@ -105,23 +107,24 @@ test_expect_success \"fetch test remote HEAD in bare repository\" '\n \n \n test_expect_success \"fetch test remote HEAD change\" '\n-\tcd \"$D\" &&\n-\tcd two &&\n-\tgit switch -c other &&\n-\tgit push -u origin other &&\n-\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n-\tgit rev-parse --verify refs/remotes/origin/main &&\n-\tgit rev-parse --verify refs/remotes/origin/other &&\n-\tgit remote set-head origin other &&\n-\tgit fetch &&\n-\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n-\tbranch=$(git rev-parse refs/remotes/origin/other) &&\n-\ttest \"z$head\" = \"z$branch\"'\n+\t(\n+\t\tcd two &&\n+\t\tgit switch -c other &&\n+\t\tgit push -u origin other &&\n+\t\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n+\t\tgit rev-parse --verify refs/remotes/origin/main &&\n+\t\tgit rev-parse --verify refs/remotes/origin/other &&\n+\t\tgit remote set-head origin other &&\n+\t\tgit fetch &&\n+\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n+\t\tbranch=$(git rev-parse refs/remotes/origin/other) &&\n+\t\ttest \"z$head\" = \"z$branch\"\n+\t)\n+'\n \n test_expect_success \"fetch test followRemoteHEAD never\" '\n-\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n \t(\n-\t\tcd \"$D\" &&\n \t\tcd two &&\n \t\tgit update-ref --no-deref -d refs/remotes/origin/HEAD &&\n \t\tgit config set remote.origin.followRemoteHEAD \"never\" &&\n@@ -134,9 +137,8 @@ test_expect_success \"fetch test followRemoteHEAD never\" '\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn no change\" '\n-\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n \t(\n-\t\tcd \"$D\" &&\n \t\tcd two &&\n \t\tgit rev-parse --verify refs/remotes/origin/other &&\n \t\tgit remote set-head origin other &&\n@@ -154,9 +156,8 @@ test_expect_success \"fetch test followRemoteHEAD warn no change\" '\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn create\" '\n-\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n \t(\n-\t\tcd \"$D\" &&\n \t\tcd two &&\n \t\tgit update-ref --no-deref -d refs/remotes/origin/HEAD &&\n \t\tgit config set remote.origin.followRemoteHEAD \"warn\" &&\n@@ -170,9 +171,8 @@ test_expect_success \"fetch test followRemoteHEAD warn create\" '\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn detached\" '\n-\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n \t(\n-\t\tcd \"$D\" &&\n \t\tcd two &&\n \t\tgit update-ref --no-deref -d refs/remotes/origin/HEAD &&\n \t\tgit update-ref refs/remotes/origin/HEAD HEAD &&\n@@ -187,9 +187,8 @@ test_expect_success \"fetch test followRemoteHEAD warn detached\" '\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn quiet\" '\n-\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n \t(\n-\t\tcd \"$D\" &&\n \t\tcd two &&\n \t\tgit rev-parse --verify refs/remotes/origin/other &&\n \t\tgit remote set-head origin other &&\n@@ -205,9 +204,8 @@ test_expect_success \"fetch test followRemoteHEAD warn quiet\" '\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn-if-not-branch branch is same\" '\n-\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n \t(\n-\t\tcd \"$D\" &&\n \t\tcd two &&\n \t\tgit rev-parse --verify refs/remotes/origin/other &&\n \t\tgit remote set-head origin other &&\n@@ -223,9 +221,8 @@ test_expect_success \"fetch test followRemoteHEAD warn-if-not-branch branch is sa\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn-if-not-branch branch is different\" '\n-\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n \t(\n-\t\tcd \"$D\" &&\n \t\tcd two &&\n \t\tgit rev-parse --verify refs/remotes/origin/other &&\n \t\tgit remote set-head origin other &&\n@@ -243,9 +240,8 @@ test_expect_success \"fetch test followRemoteHEAD warn-if-not-branch branch is di\n '\n \n test_expect_success \"fetch test followRemoteHEAD always\" '\n-\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n \t(\n-\t\tcd \"$D\" &&\n \t\tcd two &&\n \t\tgit rev-parse --verify refs/remotes/origin/other &&\n \t\tgit remote set-head origin other &&\n@@ -260,9 +256,8 @@ test_expect_success \"fetch test followRemoteHEAD always\" '\n '\n \n test_expect_success 'followRemoteHEAD does not kick in with refspecs' '\n-\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n+\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n \t(\n-\t\tcd \"$D\" &&\n \t\tcd two &&\n \t\tgit remote set-head origin other &&\n \t\tgit config set remote.origin.followRemoteHEAD always &&\n@@ -274,93 +269,100 @@ test_expect_success 'followRemoteHEAD does not kick in with refspecs' '\n '\n \n test_expect_success 'fetch --prune on its own works as expected' '\n-\tcd \"$D\" &&\n \tgit clone . prune &&\n-\tcd prune &&\n-\tgit update-ref refs/remotes/origin/extrabranch main &&\n+\t(\n+\t\tcd prune &&\n+\t\tgit update-ref refs/remotes/origin/extrabranch main &&\n \n-\tgit fetch --prune origin &&\n-\ttest_must_fail git rev-parse origin/extrabranch\n+\t\tgit fetch --prune origin &&\n+\t\ttest_must_fail git rev-parse origin/extrabranch\n+\t)\n '\n \n test_expect_success 'fetch --prune with a branch name keeps branches' '\n-\tcd \"$D\" &&\n \tgit clone . prune-branch &&\n-\tcd prune-branch &&\n-\tgit update-ref refs/remotes/origin/extrabranch main &&\n+\t(\n+\t\tcd prune-branch &&\n+\t\tgit update-ref refs/remotes/origin/extrabranch main &&\n \n-\tgit fetch --prune origin main &&\n-\tgit rev-parse origin/extrabranch\n+\t\tgit fetch --prune origin main &&\n+\t\tgit rev-parse origin/extrabranch\n+\t)\n '\n \n test_expect_success 'fetch --prune with a namespace keeps other namespaces' '\n-\tcd \"$D\" &&\n \tgit clone . prune-namespace &&\n-\tcd prune-namespace &&\n+\t(\n+\t\tcd prune-namespace &&\n \n-\tgit fetch --prune origin refs/heads/a/*:refs/remotes/origin/a/* &&\n-\tgit rev-parse origin/main\n+\t\tgit fetch --prune origin refs/heads/a/*:refs/remotes/origin/a/* &&\n+\t\tgit rev-parse origin/main\n+\t)\n '\n \n test_expect_success 'fetch --prune handles overlapping refspecs' '\n-\tcd \"$D\" &&\n \tgit update-ref refs/pull/42/head main &&\n \tgit clone . prune-overlapping &&\n-\tcd prune-overlapping &&\n-\tgit config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&\n+\t(\n+\t\tcd prune-overlapping &&\n+\t\tgit config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&\n \n-\tgit fetch --prune origin &&\n-\tgit rev-parse origin/main &&\n-\tgit rev-parse origin/pr/42 &&\n+\t\tgit fetch --prune origin &&\n+\t\tgit rev-parse origin/main &&\n+\t\tgit rev-parse origin/pr/42 &&\n \n-\tgit config --unset-all remote.origin.fetch &&\n-\tgit config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&\n-\tgit config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* &&\n+\t\tgit config --unset-all remote.origin.fetch &&\n+\t\tgit config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&\n+\t\tgit config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* &&\n \n-\tgit fetch --prune origin &&\n-\tgit rev-parse origin/main &&\n-\tgit rev-parse origin/pr/42\n+\t\tgit fetch --prune origin &&\n+\t\tgit rev-parse origin/main &&\n+\t\tgit rev-parse origin/pr/42\n+\t)\n '\n \n test_expect_success 'fetch --prune --tags prunes branches but not tags' '\n-\tcd \"$D\" &&\n \tgit clone . prune-tags &&\n-\tcd prune-tags &&\n-\tgit tag sometag main &&\n-\t# Create what looks like a remote-tracking branch from an earlier\n-\t# fetch that has since been deleted from the remote:\n-\tgit update-ref refs/remotes/origin/fake-remote main &&\n-\n-\tgit fetch --prune --tags origin &&\n-\tgit rev-parse origin/main &&\n-\ttest_must_fail git rev-parse origin/fake-remote &&\n-\tgit rev-parse sometag\n+\t(\n+\t\tcd prune-tags &&\n+\t\tgit tag sometag main &&\n+\t\t# Create what looks like a remote-tracking branch from an earlier\n+\t\t# fetch that has since been deleted from the remote:\n+\t\tgit update-ref refs/remotes/origin/fake-remote main &&\n+\n+\t\tgit fetch --prune --tags origin &&\n+\t\tgit rev-parse origin/main &&\n+\t\ttest_must_fail git rev-parse origin/fake-remote &&\n+\t\tgit rev-parse sometag\n+\t)\n '\n \n test_expect_success 'fetch --prune --tags with branch does not prune other things' '\n-\tcd \"$D\" &&\n \tgit clone . prune-tags-branch &&\n-\tcd prune-tags-branch &&\n-\tgit tag sometag main &&\n-\tgit update-ref refs/remotes/origin/extrabranch main &&\n+\t(\n+\t\tcd prune-tags-branch &&\n+\t\tgit tag sometag main &&\n+\t\tgit update-ref refs/remotes/origin/extrabranch main &&\n \n-\tgit fetch --prune --tags origin main &&\n-\tgit rev-parse origin/extrabranch &&\n-\tgit rev-parse sometag\n+\t\tgit fetch --prune --tags origin main &&\n+\t\tgit rev-parse origin/extrabranch &&\n+\t\tgit rev-parse sometag\n+\t)\n '\n \n test_expect_success 'fetch --prune --tags with refspec prunes based on refspec' '\n-\tcd \"$D\" &&\n \tgit clone . prune-tags-refspec &&\n-\tcd prune-tags-refspec &&\n-\tgit tag sometag main &&\n-\tgit update-ref refs/remotes/origin/foo/otherbranch main &&\n-\tgit update-ref refs/remotes/origin/extrabranch main &&\n-\n-\tgit fetch --prune --tags origin refs/heads/foo/*:refs/remotes/origin/foo/* &&\n-\ttest_must_fail git rev-parse refs/remotes/origin/foo/otherbranch &&\n-\tgit rev-parse origin/extrabranch &&\n-\tgit rev-parse sometag\n+\t(\n+\t\tcd prune-tags-refspec &&\n+\t\tgit tag sometag main &&\n+\t\tgit update-ref refs/remotes/origin/foo/otherbranch main &&\n+\t\tgit update-ref refs/remotes/origin/extrabranch main &&\n+\n+\t\tgit fetch --prune --tags origin refs/heads/foo/*:refs/remotes/origin/foo/* &&\n+\t\ttest_must_fail git rev-parse refs/remotes/origin/foo/otherbranch &&\n+\t\tgit rev-parse origin/extrabranch &&\n+\t\tgit rev-parse sometag\n+\t)\n '\n \n test_expect_success 'fetch --tags gets tags even without a configured remote' '\n@@ -381,21 +383,21 @@ test_expect_success 'fetch --tags gets tags even without a configured remote' '\n '\n \n test_expect_success REFFILES 'fetch --prune fails to delete branches' '\n-\tcd \"$D\" &&\n \tgit clone . prune-fail &&\n-\tcd prune-fail &&\n-\tgit update-ref refs/remotes/origin/extrabranch main &&\n-\tgit pack-refs --all &&\n-\t: this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds &&\n-\t>.git/packed-refs.new &&\n+\t(\n+\t\tcd prune-fail &&\n+\t\tgit update-ref refs/remotes/origin/extrabranch main &&\n+\t\tgit pack-refs --all &&\n+\t\t: this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds &&\n+\t\t>.git/packed-refs.new &&\n \n-\ttest_must_fail git fetch --prune origin\n+\t\ttest_must_fail git fetch --prune origin\n+\t)\n '\n \n test_expect_success 'fetch --atomic works with a single branch' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n \n-\tcd \"$D\" &&\n \tgit clone . atomic &&\n \tgit branch atomic-branch &&\n \toid=$(git rev-parse atomic-branch) &&\n@@ -408,9 +410,8 @@ test_expect_success 'fetch --atomic works with a single branch' '\n '\n \n test_expect_success 'fetch --atomic works with multiple branches' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n \n-\tcd \"$D\" &&\n \tgit clone . atomic &&\n \tgit branch atomic-branch-1 &&\n \tgit branch atomic-branch-2 &&\n@@ -423,9 +424,8 @@ test_expect_success 'fetch --atomic works with multiple branches' '\n '\n \n test_expect_success 'fetch --atomic works with mixed branches and tags' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n \n-\tcd \"$D\" &&\n \tgit clone . atomic &&\n \tgit branch atomic-mixed-branch &&\n \tgit tag atomic-mixed-tag &&\n@@ -437,9 +437,8 @@ test_expect_success 'fetch --atomic works with mixed branches and tags' '\n '\n \n test_expect_success 'fetch --atomic prunes references' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n \n-\tcd \"$D\" &&\n \tgit branch atomic-prune-delete &&\n \tgit clone . atomic &&\n \tgit branch --delete atomic-prune-delete &&\n@@ -453,9 +452,8 @@ test_expect_success 'fetch --atomic prunes references' '\n '\n \n test_expect_success 'fetch --atomic aborts with non-fast-forward update' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n \n-\tcd \"$D\" &&\n \tgit branch atomic-non-ff &&\n \tgit clone . atomic &&\n \tgit rev-parse HEAD >actual &&\n@@ -472,9 +470,8 @@ test_expect_success 'fetch --atomic aborts with non-fast-forward update' '\n '\n \n test_expect_success 'fetch --atomic executes a single reference transaction only' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n \n-\tcd \"$D\" &&\n \tgit clone . atomic &&\n \tgit branch atomic-hooks-1 &&\n \tgit branch atomic-hooks-2 &&\n@@ -499,9 +496,8 @@ test_expect_success 'fetch --atomic executes a single reference transaction only\n '\n \n test_expect_success 'fetch --atomic aborts all reference updates if hook aborts' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n \n-\tcd \"$D\" &&\n \tgit clone . atomic &&\n \tgit branch atomic-hooks-abort-1 &&\n \tgit branch atomic-hooks-abort-2 &&\n@@ -536,9 +532,8 @@ test_expect_success 'fetch --atomic aborts all reference updates if hook aborts'\n '\n \n test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n \n-\tcd \"$D\" &&\n \tgit clone . atomic &&\n \toid=$(git rev-parse HEAD) &&\n \n@@ -574,8 +569,7 @@ test_expect_success REFFILES 'fetch --atomic fails transaction if reference lock\n '\n \n test_expect_success '--refmap=\"\" ignores configured refspec' '\n-\tcd \"$TRASH_DIRECTORY\" &&\n-\tgit clone \"$D\" remote-refs &&\n+\tgit clone . remote-refs &&\n \tgit -C remote-refs rev-parse remotes/origin/main >old &&\n \tgit -C remote-refs update-ref refs/remotes/origin/main main~1 &&\n \tgit -C remote-refs rev-parse remotes/origin/main >new &&\n@@ -599,34 +593,26 @@ test_expect_success '--refmap=\"\" and --prune' '\n \n test_expect_success 'fetch tags when there is no tags' '\n \n- cd \"$D\" &&\n-\n- mkdir notags &&\n- cd notags &&\n- git init &&\n-\n- git fetch -t ..\n+\tgit init notags &&\n+\tgit -C notags fetch -t ..\n \n '\n \n test_expect_success 'fetch following tags' '\n \n-\tcd \"$D\" &&\n \tgit tag -a -m \"annotated\" anno HEAD &&\n \tgit tag light HEAD &&\n \n-\tmkdir four &&\n-\tcd four &&\n-\tgit init &&\n-\n-\tgit fetch .. :track &&\n-\tgit show-ref --verify refs/tags/anno &&\n-\tgit show-ref --verify refs/tags/light\n-\n+\tgit init four &&\n+\t(\n+\t\tcd four &&\n+\t\tgit fetch .. :track &&\n+\t\tgit show-ref --verify refs/tags/anno &&\n+\t\tgit show-ref --verify refs/tags/light\n+\t)\n '\n \n test_expect_success 'fetch uses remote ref names to describe new refs' '\n-\tcd \"$D\" &&\n \tgit init descriptive &&\n \t(\n \t\tcd descriptive &&\n@@ -654,30 +640,20 @@ test_expect_success 'fetch uses remote ref names to describe new refs' '\n \n test_expect_success 'fetch must not resolve short tag name' '\n \n-\tcd \"$D\" &&\n-\n-\tmkdir five &&\n-\tcd five &&\n-\tgit init &&\n-\n-\ttest_must_fail git fetch .. anno:five\n+\tgit init five &&\n+\ttest_must_fail git -C five fetch .. anno:five\n \n '\n \n test_expect_success 'fetch can now resolve short remote name' '\n \n-\tcd \"$D\" &&\n \tgit update-ref refs/remotes/six/HEAD HEAD &&\n \n-\tmkdir six &&\n-\tcd six &&\n-\tgit init &&\n-\n-\tgit fetch .. six:six\n+\tgit init six &&\n+\tgit -C six fetch .. six:six\n '\n \n test_expect_success 'create bundle 1' '\n-\tcd \"$D\" &&\n \techo >file updated again by origin &&\n \tgit commit -a -m \"tip\" &&\n \tgit bundle create --version=3 bundle1 main^..main\n@@ -691,35 +667,36 @@ test_expect_success 'header of bundle looks right' '\n \tOID refs/heads/main\n \n \tEOF\n-\tsed -e \"s/$OID_REGEX/OID/g\" -e \"5q\" \"$D\"/bundle1 >actual &&\n+\tsed -e \"s/$OID_REGEX/OID/g\" -e \"5q\" bundle1 >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'create bundle 2' '\n-\tcd \"$D\" &&\n \tgit bundle create bundle2 main~2..main\n '\n \n test_expect_success 'unbundle 1' '\n-\tcd \"$D/bundle\" &&\n-\tgit checkout -b some-branch &&\n-\ttest_must_fail git fetch \"$D/bundle1\" main:main\n+\t(\n+\t\tcd bundle &&\n+\t\tgit checkout -b some-branch &&\n+\t\ttest_must_fail git fetch bundle1 main:main\n+\t)\n '\n \n \n test_expect_success 'bundle 1 has only 3 files ' '\n-\tcd \"$D\" &&\n \ttest_bundle_object_count bundle1 3\n '\n \n test_expect_success 'unbundle 2' '\n-\tcd \"$D/bundle\" &&\n-\tgit fetch ../bundle2 main:main &&\n-\ttest \"tip\" = \"$(git log -1 --pretty=oneline main | cut -d\" \" -f2)\"\n+\t(\n+\t\tcd bundle &&\n+\t\tgit fetch ../bundle2 main:main &&\n+\t\ttest \"tip\" = \"$(git log -1 --pretty=oneline main | cut -d\" \" -f2)\"\n+\t)\n '\n \n test_expect_success 'bundle does not prerequisite objects' '\n-\tcd \"$D\" &&\n \ttouch file2 &&\n \tgit add file2 &&\n \tgit commit -m add.file2 file2 &&\n@@ -729,7 +706,6 @@ test_expect_success 'bundle does not prerequisite objects' '\n \n test_expect_success 'bundle should be able to create a full history' '\n \n-\tcd \"$D\" &&\n \tgit tag -a -m \"1.0\" v1.0 main &&\n \tgit bundle create bundle4 v1.0\n \n@@ -783,7 +759,6 @@ test_expect_success 'quoting of a strangely named repo' '\n \n test_expect_success 'bundle should record HEAD correctly' '\n \n-\tcd \"$D\" &&\n \tgit bundle create bundle5 HEAD main &&\n \tgit bundle list-heads bundle5 >actual &&\n \tfor h in HEAD refs/heads/main\n@@ -803,7 +778,6 @@ test_expect_success 'mark initial state of origin/main' '\n \n test_expect_success 'explicit fetch should update tracking' '\n \n-\tcd \"$D\" &&\n \tgit branch -f side &&\n \t(\n \t\tcd three &&\n@@ -818,7 +792,6 @@ test_expect_success 'explicit fetch should update tracking' '\n \n test_expect_success 'explicit pull should update tracking' '\n \n-\tcd \"$D\" &&\n \tgit branch -f side &&\n \t(\n \t\tcd three &&\n@@ -832,15 +805,13 @@ test_expect_success 'explicit pull should update tracking' '\n '\n \n test_expect_success 'explicit --refmap is allowed only with command-line refspec' '\n-\tcd \"$D\" &&\n \t(\n \t\tcd three &&\n \t\ttest_must_fail git fetch --refmap=\"*:refs/remotes/none/*\"\n \t)\n '\n \n test_expect_success 'explicit --refmap option overrides remote.*.fetch' '\n-\tcd \"$D\" &&\n \tgit branch -f side &&\n \t(\n \t\tcd three &&\n@@ -855,7 +826,6 @@ test_expect_success 'explicit --refmap option overrides remote.*.fetch' '\n '\n \n test_expect_success 'explicitly empty --refmap option disables remote.*.fetch' '\n-\tcd \"$D\" &&\n \tgit branch -f side &&\n \t(\n \t\tcd three &&\n@@ -870,7 +840,6 @@ test_expect_success 'explicitly empty --refmap option disables remote.*.fetch' '\n \n test_expect_success 'configured fetch updates tracking' '\n \n-\tcd \"$D\" &&\n \tgit branch -f side &&\n \t(\n \t\tcd three &&\n@@ -884,7 +853,6 @@ test_expect_success 'configured fetch updates tracking' '\n '\n \n test_expect_success 'non-matching refspecs do not confuse tracking update' '\n-\tcd \"$D\" &&\n \tgit update-ref refs/odd/location HEAD &&\n \t(\n \t\tcd three &&\n@@ -901,14 +869,12 @@ test_expect_success 'non-matching refspecs do not confuse tracking update' '\n \n test_expect_success 'pushing nonexistent branch by mistake should not segv' '\n \n-\tcd \"$D\" &&\n \ttest_must_fail git push seven no:no\n \n '\n \n test_expect_success 'auto tag following fetches minimum' '\n \n-\tcd \"$D\" &&\n \tgit clone .git follow &&\n \tgit checkout HEAD^0 &&\n \t(\n@@ -1307,7 +1273,7 @@ test_expect_success 'fetch --prune prints the remotes url' '\n \t\tcd only-prunes &&\n \t\tgit fetch --prune origin 2>&1 | head -n1 >../actual\n \t) &&\n-\techo \"From ${D}/.\" >expect &&\n+\techo \"From $(pwd)/.\" >expect &&\n \ttest_cmp expect actual\n '\n \n@@ -1357,14 +1323,14 @@ test_expect_success 'fetching with auto-gc does not lock up' '\n \techo \"$*\" &&\n \tfalse\n \tEOF\n-\tgit clone \"file://$D\" auto-gc &&\n+\tgit clone \"file://$PWD\" auto-gc &&\n \ttest_commit test2 &&\n \t(\n \t\tcd auto-gc &&\n \t\tgit config fetch.unpackLimit 1 &&\n \t\tgit config gc.autoPackLimit 1 &&\n \t\tgit config gc.autoDetach false &&\n-\t\tGIT_ASK_YESNO=\"$D/askyesno\" git fetch --verbose >fetch.out 2>&1 &&\n+\t\tGIT_ASK_YESNO=\"$TRASH_DIRECTORY/askyesno\" git fetch --verbose >fetch.out 2>&1 &&\n \t\ttest_grep \"Auto packing the repository\" fetch.out &&\n \t\t! grep \"Should I try again\" fetch.out\n \t)\n-- \n2.51.0.326.gecbb38d78e\n\n"},{"id":"524471","messageId":"20250819192716.GC1059295@coredump.intra.peff.net","threadId":"63993","inReplyTo":"20250819192004.GA1058857@coredump.intra.peff.net","subject":"[PATCH 3/4] t5510: prefer \"git -C\" to subshell for followRemoteHEAD tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-19T19:27:16Z","receivedAt":"2025-08-19T19:27:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These tests set config within a sub-repo using (cd two && git config),\nand then a separate test_when_finished outside the subshell to clean it\nup. We can't use test_config to do this, because the cleanup command it\nregisters inside the subshell would be lost. Nor can we do it before\nentering the subshell, because the config has to be set after some other\ncommands are run.\n\nLet's switch these tests to use \"git -C\" for each command instead of a\nsubshell. That lets us use test_config (with -C also) at the appropriate\npart of the test. And we no longer need the manual cleanup command.\n\nSigned-off-by: Jeff King \n---\nIt is perhaps debatable whether this makes the result more readable.\nIt's fewer lines, but there is \"-C\" sprinkled everywhere. So if people\nfind this ugly we can drop it (and I'd rewrite patch 4 to use the\nsubshell form in its new test).\n\n t/t5510-fetch.sh | 202 +++++++++++++++++++----------------------------\n 1 file changed, 83 insertions(+), 119 deletions(-)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 93e309e213..24379ec7aa 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -123,149 +123,113 @@ test_expect_success \"fetch test remote HEAD change\" '\n '\n \n test_expect_success \"fetch test followRemoteHEAD never\" '\n-\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n-\t(\n-\t\tcd two &&\n-\t\tgit update-ref --no-deref -d refs/remotes/origin/HEAD &&\n-\t\tgit config set remote.origin.followRemoteHEAD \"never\" &&\n-\t\tGIT_TRACE_PACKET=$PWD/trace.out git fetch &&\n-\t\t# Confirm that we do not even ask for HEAD when we are\n-\t\t# not going to act on it.\n-\t\ttest_grep ! \"ref-prefix HEAD\" trace.out &&\n-\t\ttest_must_fail git rev-parse --verify refs/remotes/origin/HEAD\n-\t)\n+\tgit -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&\n+\ttest_config -C two remote.origin.followRemoteHEAD \"never\" &&\n+\tGIT_TRACE_PACKET=$PWD/trace.out git -C two fetch &&\n+\t# Confirm that we do not even ask for HEAD when we are\n+\t# not going to act on it.\n+\ttest_grep ! \"ref-prefix HEAD\" trace.out &&\n+\ttest_must_fail git -C two rev-parse --verify refs/remotes/origin/HEAD\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn no change\" '\n-\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n-\t(\n-\t\tcd two &&\n-\t\tgit rev-parse --verify refs/remotes/origin/other &&\n-\t\tgit remote set-head origin other &&\n-\t\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n-\t\tgit rev-parse --verify refs/remotes/origin/main &&\n-\t\tgit config set remote.origin.followRemoteHEAD \"warn\" &&\n-\t\tgit fetch >output &&\n-\t\techo \"${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ},\" \\\n-\t\t\t\"but we have ${SQ}other${SQ} locally.\" >expect &&\n-\t\ttest_cmp expect output &&\n-\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n-\t\tbranch=$(git rev-parse refs/remotes/origin/other) &&\n-\t\ttest \"z$head\" = \"z$branch\"\n-\t)\n+\tgit -C two rev-parse --verify refs/remotes/origin/other &&\n+\tgit -C two remote set-head origin other &&\n+\tgit -C two rev-parse --verify refs/remotes/origin/HEAD &&\n+\tgit -C two rev-parse --verify refs/remotes/origin/main &&\n+\ttest_config -C two remote.origin.followRemoteHEAD \"warn\" &&\n+\tgit -C two fetch >output &&\n+\techo \"${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ},\" \\\n+\t\t\"but we have ${SQ}other${SQ} locally.\" >expect &&\n+\ttest_cmp expect output &&\n+\thead=$(git -C two rev-parse refs/remotes/origin/HEAD) &&\n+\tbranch=$(git -C two rev-parse refs/remotes/origin/other) &&\n+\ttest \"z$head\" = \"z$branch\"\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn create\" '\n-\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n-\t(\n-\t\tcd two &&\n-\t\tgit update-ref --no-deref -d refs/remotes/origin/HEAD &&\n-\t\tgit config set remote.origin.followRemoteHEAD \"warn\" &&\n-\t\tgit rev-parse --verify refs/remotes/origin/main &&\n-\t\toutput=$(git fetch) &&\n-\t\ttest \"z\" = \"z$output\" &&\n-\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n-\t\tbranch=$(git rev-parse refs/remotes/origin/main) &&\n-\t\ttest \"z$head\" = \"z$branch\"\n-\t)\n+\tgit -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&\n+\ttest_config -C two remote.origin.followRemoteHEAD \"warn\" &&\n+\tgit -C two rev-parse --verify refs/remotes/origin/main &&\n+\toutput=$(git -C two fetch) &&\n+\ttest \"z\" = \"z$output\" &&\n+\thead=$(git -C two rev-parse refs/remotes/origin/HEAD) &&\n+\tbranch=$(git -C two rev-parse refs/remotes/origin/main) &&\n+\ttest \"z$head\" = \"z$branch\"\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn detached\" '\n-\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n-\t(\n-\t\tcd two &&\n-\t\tgit update-ref --no-deref -d refs/remotes/origin/HEAD &&\n-\t\tgit update-ref refs/remotes/origin/HEAD HEAD &&\n-\t\tHEAD=$(git log --pretty=\"%H\") &&\n-\t\tgit config set remote.origin.followRemoteHEAD \"warn\" &&\n-\t\tgit fetch >output &&\n-\t\techo \"${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ},\" \\\n-\t\t\t\"but we have a detached HEAD pointing to\" \\\n-\t\t\t\"${SQ}${HEAD}${SQ} locally.\" >expect &&\n-\t\ttest_cmp expect output\n-\t)\n+\tgit -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&\n+\tgit -C two update-ref refs/remotes/origin/HEAD HEAD &&\n+\tHEAD=$(git -C two log --pretty=\"%H\") &&\n+\ttest_config -C two remote.origin.followRemoteHEAD \"warn\" &&\n+\tgit -C two fetch >output &&\n+\techo \"${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ},\" \\\n+\t\t\"but we have a detached HEAD pointing to\" \\\n+\t\t\"${SQ}${HEAD}${SQ} locally.\" >expect &&\n+\ttest_cmp expect output\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn quiet\" '\n-\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n-\t(\n-\t\tcd two &&\n-\t\tgit rev-parse --verify refs/remotes/origin/other &&\n-\t\tgit remote set-head origin other &&\n-\t\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n-\t\tgit rev-parse --verify refs/remotes/origin/main &&\n-\t\tgit config set remote.origin.followRemoteHEAD \"warn\" &&\n-\t\toutput=$(git fetch --quiet) &&\n-\t\ttest \"z\" = \"z$output\" &&\n-\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n-\t\tbranch=$(git rev-parse refs/remotes/origin/other) &&\n-\t\ttest \"z$head\" = \"z$branch\"\n-\t)\n+\tgit -C two rev-parse --verify refs/remotes/origin/other &&\n+\tgit -C two remote set-head origin other &&\n+\tgit -C two rev-parse --verify refs/remotes/origin/HEAD &&\n+\tgit -C two rev-parse --verify refs/remotes/origin/main &&\n+\ttest_config -C two remote.origin.followRemoteHEAD \"warn\" &&\n+\toutput=$(git -C two fetch --quiet) &&\n+\ttest \"z\" = \"z$output\" &&\n+\thead=$(git -C two rev-parse refs/remotes/origin/HEAD) &&\n+\tbranch=$(git -C two rev-parse refs/remotes/origin/other) &&\n+\ttest \"z$head\" = \"z$branch\"\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn-if-not-branch branch is same\" '\n-\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n-\t(\n-\t\tcd two &&\n-\t\tgit rev-parse --verify refs/remotes/origin/other &&\n-\t\tgit remote set-head origin other &&\n-\t\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n-\t\tgit rev-parse --verify refs/remotes/origin/main &&\n-\t\tgit config set remote.origin.followRemoteHEAD \"warn-if-not-main\" &&\n-\t\tactual=$(git fetch) &&\n-\t\ttest \"z\" = \"z$actual\" &&\n-\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n-\t\tbranch=$(git rev-parse refs/remotes/origin/other) &&\n-\t\ttest \"z$head\" = \"z$branch\"\n-\t)\n+\tgit -C two rev-parse --verify refs/remotes/origin/other &&\n+\tgit -C two remote set-head origin other &&\n+\tgit -C two rev-parse --verify refs/remotes/origin/HEAD &&\n+\tgit -C two rev-parse --verify refs/remotes/origin/main &&\n+\ttest_config -C two remote.origin.followRemoteHEAD \"warn-if-not-main\" &&\n+\tactual=$(git -C two fetch) &&\n+\ttest \"z\" = \"z$actual\" &&\n+\thead=$(git -C two rev-parse refs/remotes/origin/HEAD) &&\n+\tbranch=$(git -C two rev-parse refs/remotes/origin/other) &&\n+\ttest \"z$head\" = \"z$branch\"\n '\n \n test_expect_success \"fetch test followRemoteHEAD warn-if-not-branch branch is different\" '\n-\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n-\t(\n-\t\tcd two &&\n-\t\tgit rev-parse --verify refs/remotes/origin/other &&\n-\t\tgit remote set-head origin other &&\n-\t\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n-\t\tgit rev-parse --verify refs/remotes/origin/main &&\n-\t\tgit config set remote.origin.followRemoteHEAD \"warn-if-not-some/different-branch\" &&\n-\t\tgit fetch >actual &&\n-\t\techo \"${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ},\" \\\n-\t\t\t\"but we have ${SQ}other${SQ} locally.\" >expect &&\n-\t\ttest_cmp expect actual &&\n-\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n-\t\tbranch=$(git rev-parse refs/remotes/origin/other) &&\n-\t\ttest \"z$head\" = \"z$branch\"\n-\t)\n+\tgit -C two rev-parse --verify refs/remotes/origin/other &&\n+\tgit -C two remote set-head origin other &&\n+\tgit -C two rev-parse --verify refs/remotes/origin/HEAD &&\n+\tgit -C two rev-parse --verify refs/remotes/origin/main &&\n+\ttest_config -C two remote.origin.followRemoteHEAD \"warn-if-not-some/different-branch\" &&\n+\tgit -C two fetch >actual &&\n+\techo \"${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ},\" \\\n+\t\t\"but we have ${SQ}other${SQ} locally.\" >expect &&\n+\ttest_cmp expect actual &&\n+\thead=$(git -C two rev-parse refs/remotes/origin/HEAD) &&\n+\tbranch=$(git -C two rev-parse refs/remotes/origin/other) &&\n+\ttest \"z$head\" = \"z$branch\"\n '\n \n test_expect_success \"fetch test followRemoteHEAD always\" '\n-\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n-\t(\n-\t\tcd two &&\n-\t\tgit rev-parse --verify refs/remotes/origin/other &&\n-\t\tgit remote set-head origin other &&\n-\t\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n-\t\tgit rev-parse --verify refs/remotes/origin/main &&\n-\t\tgit config set remote.origin.followRemoteHEAD \"always\" &&\n-\t\tgit fetch &&\n-\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n-\t\tbranch=$(git rev-parse refs/remotes/origin/main) &&\n-\t\ttest \"z$head\" = \"z$branch\"\n-\t)\n+\tgit -C two rev-parse --verify refs/remotes/origin/other &&\n+\tgit -C two remote set-head origin other &&\n+\tgit -C two rev-parse --verify refs/remotes/origin/HEAD &&\n+\tgit -C two rev-parse --verify refs/remotes/origin/main &&\n+\ttest_config -C two remote.origin.followRemoteHEAD \"always\" &&\n+\tgit -C two fetch &&\n+\thead=$(git -C two rev-parse refs/remotes/origin/HEAD) &&\n+\tbranch=$(git -C two rev-parse refs/remotes/origin/main) &&\n+\ttest \"z$head\" = \"z$branch\"\n '\n \n test_expect_success 'followRemoteHEAD does not kick in with refspecs' '\n-\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n-\t(\n-\t\tcd two &&\n-\t\tgit remote set-head origin other &&\n-\t\tgit config set remote.origin.followRemoteHEAD always &&\n-\t\tgit fetch origin refs/heads/main:refs/remotes/origin/main &&\n-\t\techo refs/remotes/origin/other >expect &&\n-\t\tgit symbolic-ref refs/remotes/origin/HEAD >actual &&\n-\t\ttest_cmp expect actual\n-\t)\n+\tgit -C two remote set-head origin other &&\n+\ttest_config -C two remote.origin.followRemoteHEAD always &&\n+\tgit -C two fetch origin refs/heads/main:refs/remotes/origin/main &&\n+\techo refs/remotes/origin/other >expect &&\n+\tgit -C two symbolic-ref refs/remotes/origin/HEAD >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'fetch --prune on its own works as expected' '\n-- \n2.51.0.326.gecbb38d78e\n\n"},{"id":"524472","messageId":"20250819192934.GD1059295@coredump.intra.peff.net","threadId":"63993","inReplyTo":"20250819192004.GA1058857@coredump.intra.peff.net","subject":"[PATCH 4/4] refs: do not clobber dangling symrefs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-19T19:29:34Z","receivedAt":"2025-08-19T19:29:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When given an expected \"before\" state, the ref-writing code will avoid\noverwriting any ref that does not match that expected state. We use the\nnull oid as a sentinel value for \"nothing should exist\", and likewise\nthat is the sentinel value we get when trying to read a ref that does\nnot exist.\n\nBut there's one corner case where this is ambiguous: dangling symrefs.\nTrying to read them will yield the null oid, but there is potentially\nsomething of value there: the dangling symref itself.\n\nFor a normal recursive write, this is OK. Imagine we have a symref\n\"FOO_HEAD\" that points to a ref \"refs/heads/bar\" that does not exist,\nand we try to write to it with a create operation like:\n\n oid=$(git rev-parse HEAD) ;# or whatever\n git symbolic-ref FOO_HEAD refs/heads/bar\n echo \"create FOO_HEAD $oid\" | git update-ref --stdin\n\nThe attempt to resolve FOO_HEAD will actually resolve \"bar\", yielding\nthe null oid. That matches our expectation, and the write proceeds. This\nis correct, because we are not writing FOO_HEAD at all, but writing its\ndestination \"bar\", which in fact does not exist.\n\nBut what if the operation asked not to dereference symrefs? Like this:\n\n echo \"create FOO_HEAD $oid\" | git update-ref --no-deref --stdin\n\nResolving FOO_HEAD would still result in a null oid, and the write will\nproceed. But it will overwrite FOO_HEAD itself, removing the fact that\nit ever pointed to \"bar\".\n\nThis case is a little esoteric; we are clobbering a symref with a\nno-deref write of a regular ref value. But the same problem occurs when\nwriting symrefs. For example:\n\n echo \"symref-create FOO_HEAD refs/heads/other\" |\n git update-ref --no-deref --stdin\n\nThe \"create\" operation asked us to create FOO_HEAD only if it did not\nexist. But we silently overwrite the existing value.\n\nYou can trigger this without using update-ref via the fetch\nfollowRemoteHEAD code. In \"create\" mode, it should not overwrite an\nexisting value. But if you manually create a symref pointing to a value\nthat does not yet exist (either via symbolic-ref or with \"remote add\n-m\"), create mode will happily overwrite it.\n\nInstead, we should detect this case and refuse to write. The correct\nspecification to overwrite FOO_HEAD in this case is to provide an\nexpected target ref value, like:\n\n echo \"symref-update FOO_HEAD refs/heads/other ref refs/heads/bar\" |\n git update-ref --no-deref --stdin\n\nNote that the non-symref \"update\" directive does not allow you to do\nthis (you can only specify an oid). This is a weakness in the update-ref\ninterface, and you'd have to overwrite unconditionally, like:\n\n echo \"update FOO_HEAD $oid\" | git update-ref --no-deref --stdin\n\nLikewise other symref operations like symref-delete do not accept the\n\"ref\" keyword. You should be able to do:\n\n echo \"symref-delete FOO_HEAD ref refs/heads/bar\"\n\nbut cannot (and can only delete unconditionally). This patch doesn't\naddress those gaps. We may want to do so in a future patch for\ncompleteness, but it's not clear if anybody actually wants to perform\nthose operations. The symref update case (specifically, via\nfollowRemoteHEAD) is what I ran into in the wild.\n\nThe code for the fix is relatively straight-forward given the discussion\nabove. But note that we have to implement it independently for the files\nand reftable backends. The \"old oid\" checks happen as part of the\nlocking process, which is implemented separately for each system. We may\nwant to factor this out somehow, but it's beyond the scope of this\npatch. (Another curiosity is that the messages in the reftable code are\nmarked for translation, but the ones in the files backend are not. I\nfollowed local convention in each case, but we may want to harmonize\nthis at some point).\n\nSigned-off-by: Jeff King \n---\n refs/files-backend.c | 34 ++++++++++++++++++++++++++++++----\n refs/reftable-backend.c | 30 +++++++++++++++++++++++++++---\n t/t1400-update-ref.sh | 21 +++++++++++++++++++++\n t/t5510-fetch.sh | 9 +++++++++\n 4 files changed, 87 insertions(+), 7 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 905555365b..a4419ef62d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2512,13 +2512,37 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,\n */\n static enum ref_transaction_error check_old_oid(struct ref_update *update,\n \t\t\t\t\t\tstruct object_id *oid,\n+\t\t\t\t\t\tstruct strbuf *referent,\n \t\t\t\t\t\tstruct strbuf *err)\n {\n \tif (update->flags & REF_LOG_ONLY ||\n-\t !(update->flags & REF_HAVE_OLD) ||\n-\t oideq(oid, &update->old_oid))\n+\t !(update->flags & REF_HAVE_OLD))\n \t\treturn 0;\n \n+\tif (oideq(oid, &update->old_oid)) {\n+\t\t/*\n+\t\t * Normally matching the expected old oid is enough. Either we\n+\t\t * found the ref at the expected state, or we are creating and\n+\t\t * expect the null oid (and likewise found nothing).\n+\t\t *\n+\t\t * But there is one exception for the null oid: if we found a\n+\t\t * symref pointing to nothing we'll also get the null oid. In\n+\t\t * regular recursive mode, that's good (we'll write to what the\n+\t\t * symref points to, which doesn't exist). But in no-deref\n+\t\t * mode, it means we'll clobber the symref, even though the\n+\t\t * caller asked for this to be a creation event. So flag\n+\t\t * that case to preserve the dangling symref.\n+\t\t */\n+\t\tif ((update->flags & REF_NO_DEREF) && referent->len &&\n+\t\t is_null_oid(oid)) {\n+\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n+\t\t\t\t \"dangling symref already exists\",\n+\t\t\t\t ref_update_original_update_refname(update));\n+\t\t\treturn REF_TRANSACTION_ERROR_CREATE_EXISTS;\n+\t\t}\n+\t\treturn 0;\n+\t}\n+\n \tif (is_null_oid(&update->old_oid)) {\n \t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n \t\t\t \"reference already exists\",\n@@ -2658,7 +2682,8 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re\n \t\t\tif (update->old_target)\n \t\t\t\tret = ref_update_check_old_target(referent.buf, update, err);\n \t\t\telse\n-\t\t\t\tret = check_old_oid(update, &lock->old_oid, err);\n+\t\t\t\tret = check_old_oid(update, &lock->old_oid,\n+\t\t\t\t\t\t &referent, err);\n \t\t\tif (ret)\n \t\t\t\tgoto out;\n \t\t} else {\n@@ -2690,7 +2715,8 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re\n \t\t\tret = REF_TRANSACTION_ERROR_EXPECTED_SYMREF;\n \t\t\tgoto out;\n \t\t} else {\n-\t\t\tret = check_old_oid(update, &lock->old_oid, err);\n+\t\t\tret = check_old_oid(update, &lock->old_oid,\n+\t\t\t\t\t &referent, err);\n \t\t\tif (ret) {\n \t\t\t\tgoto out;\n \t\t\t}\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 99fafd75eb..ef98584bf9 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1272,9 +1272,33 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor\n \t\tret = ref_update_check_old_target(referent->buf, u, err);\n \t\tif (ret)\n \t\t\treturn ret;\n-\t} else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD &&\n-\t\t !oideq(¤t_oid, &u->old_oid)) {\n-\t\tif (is_null_oid(&u->old_oid)) {\n+\t} else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD) {\n+\t\tif (oideq(¤t_oid, &u->old_oid)) {\n+\t\t\t/*\n+\t\t\t * Normally matching the expected old oid is enough. Either we\n+\t\t\t * found the ref at the expected state, or we are creating and\n+\t\t\t * expect the null oid (and likewise found nothing).\n+\t\t\t *\n+\t\t\t * But there is one exception for the null oid: if we found a\n+\t\t\t * symref pointing to nothing we'll also get the null oid. In\n+\t\t\t * regular recursive mode, that's good (we'll write to what the\n+\t\t\t * symref points to, which doesn't exist). But in no-deref\n+\t\t\t * mode, it means we'll clobber the symref, even though the\n+\t\t\t * caller asked for this to be a creation event. So flag\n+\t\t\t * that case to preserve the dangling symref.\n+\t\t\t *\n+\t\t\t * Everything else is OK and we can fall through to the\n+\t\t\t * end of the conditional chain.\n+\t\t\t */\n+\t\t\tif ((u->flags & REF_NO_DEREF) &&\n+\t\t\t referent->len &&\n+\t\t\t is_null_oid(&u->old_oid)) {\n+\t\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n+\t\t\t\t\t \"dangling symref already exists\"),\n+\t\t\t\t\t ref_update_original_update_refname(u));\n+\t\t\t\treturn REF_TRANSACTION_ERROR_CREATE_EXISTS;\n+\t\t\t}\n+\t\t} else if (is_null_oid(&u->old_oid)) {\n \t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n \t\t\t\t\t \"reference already exists\"),\n \t\t\t\t ref_update_original_update_refname(u));\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex d29d23cb89..29b31e3b9b 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -2310,4 +2310,25 @@ test_expect_success 'update-ref should also create reflog for HEAD' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'dangling symref not overwritten by creation' '\n+\ttest_when_finished \"git update-ref -d refs/heads/dangling\" &&\n+\tgit symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&\n+\ttest_must_fail git update-ref --no-deref --stdin 2>err <<-\\EOF &&\n+\tcreate refs/heads/dangling HEAD\n+\tEOF\n+\ttest_grep \"cannot lock.*dangling symref already exists\" err &&\n+\ttest_must_fail git rev-parse --verify refs/heads/dangling &&\n+\ttest_must_fail git rev-parse --verify refs/heads/does-not-exist\n+'\n+\n+test_expect_success 'dangling symref overwritten without old oid' '\n+\ttest_when_finished \"git update-ref -d refs/heads/dangling\" &&\n+\tgit symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&\n+\tgit update-ref --no-deref --stdin <<-\\EOF &&\n+\tupdate refs/heads/dangling HEAD\n+\tEOF\n+\tgit rev-parse --verify refs/heads/dangling &&\n+\ttest_must_fail git rev-parse --verify refs/heads/does-not-exist\n+'\n+\n test_done\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 24379ec7aa..83d1aadf9f 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -232,6 +232,15 @@ test_expect_success 'followRemoteHEAD does not kick in with refspecs' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'followRemoteHEAD create does not overwrite dangling symref' '\n+\tgit -C two remote add -m does-not-exist custom-head ../one &&\n+\ttest_config -C two remote.custom-head.followRemoteHEAD create &&\n+\tgit -C two fetch custom-head &&\n+\techo refs/remotes/custom-head/does-not-exist >expect &&\n+\tgit -C two symbolic-ref refs/remotes/custom-head/HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'fetch --prune on its own works as expected' '\n \tgit clone . prune &&\n \t(\n-- \n2.51.0.326.gecbb38d78e\n"},{"id":"524478","messageId":"8797c495-8277-4f65-845b-167542b82949@charter.net","threadId":"63993","inReplyTo":"20250819192455.GA1059295@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] t5510: make confusing config cleanup more explicit","fromName":"Eric Sunshine","fromEmail":"ericsunshine@charter.net","sentAt":"2025-08-19T20:03:23Z","receivedAt":"2025-08-19T20:05:00Z","isPatch":true,"sender":{"key":"ericsunshine@charter.net","avatar":null},"body":"On 8/19/25 3:24 PM, Jeff King wrote:\n> Several tests set a config variable in a sub-repo we chdir into via a\n> subshell, like this:\n> \n> (\n> \tcd \"$D\" &&\n> \tcd two &&\n> \tgit config foo.bar baz\n> )\n> \n> But they also clean up the variable with a when_finished directive\n> outside of the subshell, like this:\n> \n> test_when_finished \"git config unset foo.bar\"\n> \n> At first glance, this shouldn't work! The cleanup clause cannot be run\n> from the subshell (since environment changes there are lost by the time\n> the test snippet finishes). But since the cleanup command runs outside\n> the subshell, our working directory will not have been switched into\n> \"two\".\n> \n> But it does work. Why?\n> \n> The answer is that an earlier test does a \"cd two\" that moves the whole\n> test's working directory out of $TRASH_DIRECTORY and into \"two\". So the\n> subshell is a bit of a red herring; we are already in the right\n> directory! That's why we need the \"cd $D\" at the top of the shell, to\n> put us back to a known spot.\n> \n> Let's make this cleanup code more explicitly specify where we expect the\n> config command to run. That makes the script more robust against running\n> a subset of the tests, and ultimately will make it easier to refactor\n> the script to avoid these top-level chdirs.\n> \n> Signed-off-by: Jeff King \n> ---\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> @@ -119,7 +119,7 @@ test_expect_success \"fetch test remote HEAD change\" '\n> -\ttest_when_finished \"git config unset remote.origin.followRemoteHEAD\" &&\n> +\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n\nFor what it's worth, I have an unsent patch from a much larger unsent \nseries which cleans up the t5510 messiness differently. (The below patch \nis probably whitespace damaged by the MUA.)\n\n\nFrom: Eric Sunshine \nDate: Sun, 20 Nov 2022 00:48:00 -0500\nSubject: [PATCH 17/41] t5510: stop invoking `cd` outside of subshell\n\nSigned-off-by: Eric Sunshine \n---\n t/t5510-fetch.sh | 123 ++++++++++++++++++-----------------------------\n 1 file changed, 47 insertions(+), 76 deletions(-)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex c0b745e33b..051af8d3f7 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -8,8 +8,6 @@ test_description='Per branch config variables affects \n\"git fetch\".\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-bundle.sh\n\n-D=$(pwd)\n-\n test_expect_success setup '\n \techo >file original &&\n \tgit add file &&\n@@ -48,19 +46,20 @@ test_expect_success \"clone and setup child repos\" '\n '\n\n test_expect_success \"fetch test\" '\n-\tcd \"$D\" &&\n \techo >file updated by origin &&\n \tgit commit -a -m \"updated by origin\" &&\n+\t(\n \tcd two &&\n \tgit fetch &&\n \tgit rev-parse --verify refs/heads/one &&\n \tmine=$(git rev-parse refs/heads/one) &&\n \this=$(cd ../one && git rev-parse refs/heads/main) &&\n \ttest \"z$mine\" = \"z$his\"\n+\t)\n '\n\n test_expect_success \"fetch test for-merge\" '\n-\tcd \"$D\" &&\n+\t(\n \tcd three &&\n \tgit fetch &&\n \tgit rev-parse --verify refs/heads/two &&\n@@ -72,41 +71,46 @@ test_expect_success \"fetch test for-merge\" '\n \t\techo \"$main_in_two\tnot-for-merge\"\n \t} >expected &&\n \tcut -f -2 .git/FETCH_HEAD >actual &&\n-\ttest_cmp expected actual'\n+\ttest_cmp expected actual\n+\t)\n+'\n\n test_expect_success 'fetch --prune on its own works as expected' '\n-\tcd \"$D\" &&\n \tgit clone . prune &&\n+\t(\n \tcd prune &&\n \tgit update-ref refs/remotes/origin/extrabranch main &&\n\n \tgit fetch --prune origin &&\n \ttest_must_fail git rev-parse origin/extrabranch\n+\t)\n '\n\n test_expect_success 'fetch --prune with a branch name keeps branches' '\n-\tcd \"$D\" &&\n \tgit clone . prune-branch &&\n+\t(\n \tcd prune-branch &&\n \tgit update-ref refs/remotes/origin/extrabranch main &&\n\n \tgit fetch --prune origin main &&\n \tgit rev-parse origin/extrabranch\n+\t)\n '\n\n test_expect_success 'fetch --prune with a namespace keeps other \nnamespaces' '\n-\tcd \"$D\" &&\n \tgit clone . prune-namespace &&\n+\t(\n \tcd prune-namespace &&\n\n \tgit fetch --prune origin refs/heads/a/*:refs/remotes/origin/a/* &&\n \tgit rev-parse origin/main\n+\t)\n '\n\n test_expect_success 'fetch --prune handles overlapping refspecs' '\n-\tcd \"$D\" &&\n \tgit update-ref refs/pull/42/head main &&\n \tgit clone . prune-overlapping &&\n+\t(\n \tcd prune-overlapping &&\n \tgit config --add remote.origin.fetch \nrefs/pull/*/head:refs/remotes/origin/pr/* &&\n\n@@ -121,11 +125,12 @@ test_expect_success 'fetch --prune handles \noverlapping refspecs' '\n \tgit fetch --prune origin &&\n \tgit rev-parse origin/main &&\n \tgit rev-parse origin/pr/42\n+\t)\n '\n\n test_expect_success 'fetch --prune --tags prunes branches but not tags' '\n-\tcd \"$D\" &&\n \tgit clone . prune-tags &&\n+\t(\n \tcd prune-tags &&\n \tgit tag sometag main &&\n \t# Create what looks like a remote-tracking branch from an earlier\n@@ -136,11 +141,12 @@ test_expect_success 'fetch --prune --tags prunes \nbranches but not tags' '\n \tgit rev-parse origin/main &&\n \ttest_must_fail git rev-parse origin/fake-remote &&\n \tgit rev-parse sometag\n+\t)\n '\n\n test_expect_success 'fetch --prune --tags with branch does not prune \nother things' '\n-\tcd \"$D\" &&\n \tgit clone . prune-tags-branch &&\n+\t(\n \tcd prune-tags-branch &&\n \tgit tag sometag main &&\n \tgit update-ref refs/remotes/origin/extrabranch main &&\n@@ -148,11 +154,12 @@ test_expect_success 'fetch --prune --tags with \nbranch does not prune other thing\n \tgit fetch --prune --tags origin main &&\n \tgit rev-parse origin/extrabranch &&\n \tgit rev-parse sometag\n+\t)\n '\n\n test_expect_success 'fetch --prune --tags with refspec prunes based on \nrefspec' '\n-\tcd \"$D\" &&\n \tgit clone . prune-tags-refspec &&\n+\t(\n \tcd prune-tags-refspec &&\n \tgit tag sometag main &&\n \tgit update-ref refs/remotes/origin/foo/otherbranch main &&\n@@ -162,23 +169,24 @@ test_expect_success 'fetch --prune --tags with \nrefspec prunes based on refspec'\n \ttest_must_fail git rev-parse refs/remotes/origin/foo/otherbranch &&\n \tgit rev-parse origin/extrabranch &&\n \tgit rev-parse sometag\n+\t)\n '\n\n test_expect_success REFFILES 'fetch --prune fails to delete branches' '\n-\tcd \"$D\" &&\n \tgit clone . prune-fail &&\n+\t(\n \tcd prune-fail &&\n \tgit update-ref refs/remotes/origin/extrabranch main &&\n \t: this will prevent --prune from locking packed-refs for deleting \nrefs, but adding loose refs still succeeds &&\n \t>.git/packed-refs.new &&\n\n \ttest_must_fail git fetch --prune origin\n+\t)\n '\n\n test_expect_success 'fetch --atomic works with a single branch' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n\n-\tcd \"$D\" &&\n \tgit clone . atomic &&\n \tgit branch atomic-branch &&\n \toid=$(git rev-parse atomic-branch) &&\n@@ -191,9 +199,8 @@ test_expect_success 'fetch --atomic works with a \nsingle branch' '\n '\n\n test_expect_success 'fetch --atomic works with multiple branches' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n\n-\tcd \"$D\" &&\n \tgit clone . atomic &&\n \tgit branch atomic-branch-1 &&\n \tgit branch atomic-branch-2 &&\n@@ -206,9 +213,8 @@ test_expect_success 'fetch --atomic works with \nmultiple branches' '\n '\n\n test_expect_success 'fetch --atomic works with mixed branches and tags' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n\n-\tcd \"$D\" &&\n \tgit clone . atomic &&\n \tgit branch atomic-mixed-branch &&\n \tgit tag atomic-mixed-tag &&\n@@ -220,9 +226,8 @@ test_expect_success 'fetch --atomic works with mixed \nbranches and tags' '\n '\n\n test_expect_success 'fetch --atomic prunes references' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n\n-\tcd \"$D\" &&\n \tgit branch atomic-prune-delete &&\n \tgit clone . atomic &&\n \tgit branch --delete atomic-prune-delete &&\n@@ -236,9 +241,8 @@ test_expect_success 'fetch --atomic prunes references' '\n '\n\n test_expect_success 'fetch --atomic aborts with non-fast-forward update' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n\n-\tcd \"$D\" &&\n \tgit branch atomic-non-ff &&\n \tgit clone . atomic &&\n \tgit rev-parse HEAD >actual &&\n@@ -255,9 +259,8 @@ test_expect_success 'fetch --atomic aborts with \nnon-fast-forward update' '\n '\n\n test_expect_success 'fetch --atomic executes a single reference \ntransaction only' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n\n-\tcd \"$D\" &&\n \tgit clone . atomic &&\n \tgit branch atomic-hooks-1 &&\n \tgit branch atomic-hooks-2 &&\n@@ -282,9 +285,8 @@ test_expect_success 'fetch --atomic executes a \nsingle reference transaction only\n '\n\n test_expect_success 'fetch --atomic aborts all reference updates if \nhook aborts' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n\n-\tcd \"$D\" &&\n \tgit clone . atomic &&\n \tgit branch atomic-hooks-abort-1 &&\n \tgit branch atomic-hooks-abort-2 &&\n@@ -319,9 +321,8 @@ test_expect_success 'fetch --atomic aborts all \nreference updates if hook aborts'\n '\n\n test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '\n-\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\ttest_when_finished \"rm -rf atomic\" &&\n\n-\tcd \"$D\" &&\n \tgit clone . atomic &&\n \toid=$(git rev-parse HEAD) &&\n\n@@ -344,8 +345,7 @@ test_expect_success 'fetch --atomic --append appends \nto FETCH_HEAD' '\n '\n\n test_expect_success '--refmap=\"\" ignores configured refspec' '\n-\tcd \"$TRASH_DIRECTORY\" &&\n-\tgit clone \"$D\" remote-refs &&\n+\tgit clone . remote-refs &&\n \tgit -C remote-refs rev-parse remotes/origin/main >old &&\n \tgit -C remote-refs update-ref refs/remotes/origin/main main~1 &&\n \tgit -C remote-refs rev-parse remotes/origin/main >new &&\n@@ -368,35 +368,31 @@ test_expect_success '--refmap=\"\" and --prune' '\n '\n\n test_expect_success 'fetch tags when there is no tags' '\n-\n- cd \"$D\" &&\n-\n mkdir notags &&\n+ (\n cd notags &&\n git init &&\n\n git fetch -t ..\n-\n+ )\n '\n\n test_expect_success 'fetch following tags' '\n-\n-\tcd \"$D\" &&\n \tgit tag -a -m \"annotated\" anno HEAD &&\n \tgit tag light HEAD &&\n\n \tmkdir four &&\n+\t(\n \tcd four &&\n \tgit init &&\n\n \tgit fetch .. :track &&\n \tgit show-ref --verify refs/tags/anno &&\n \tgit show-ref --verify refs/tags/light\n-\n+\t)\n '\n\n test_expect_success 'fetch uses remote ref names to describe new refs' '\n-\tcd \"$D\" &&\n \tgit init descriptive &&\n \t(\n \t\tcd descriptive &&\n@@ -423,31 +419,28 @@ test_expect_success 'fetch uses remote ref names \nto describe new refs' '\n '\n\n test_expect_success 'fetch must not resolve short tag name' '\n-\n-\tcd \"$D\" &&\n-\n \tmkdir five &&\n+\t(\n \tcd five &&\n \tgit init &&\n\n \ttest_must_fail git fetch .. anno:five\n-\n+\t)\n '\n\n test_expect_success 'fetch can now resolve short remote name' '\n-\n-\tcd \"$D\" &&\n \tgit update-ref refs/remotes/six/HEAD HEAD &&\n\n \tmkdir six &&\n+\t(\n \tcd six &&\n \tgit init &&\n\n \tgit fetch .. six:six\n+\t)\n '\n\n test_expect_success 'create bundle 1' '\n-\tcd \"$D\" &&\n \techo >file updated again by origin &&\n \tgit commit -a -m \"tip\" &&\n \tgit bundle create --version=3 bundle1 main^..main\n@@ -461,35 +454,30 @@ test_expect_success 'header of bundle looks right' '\n \tOID refs/heads/main\n\n \tEOF\n-\tsed -e \"s/$OID_REGEX/OID/g\" -e \"5q\" \"$D\"/bundle1 >actual &&\n+\tsed -e \"s/$OID_REGEX/OID/g\" -e \"5q\" \"$(pwd)\"/bundle1 >actual &&\n \ttest_cmp expect actual\n '\n\n test_expect_success 'create bundle 2' '\n-\tcd \"$D\" &&\n \tgit bundle create bundle2 main~2..main\n '\n\n test_expect_success 'unbundle 1' '\n-\tcd \"$D/bundle\" &&\n-\tgit checkout -b some-branch &&\n-\ttest_must_fail git fetch \"$D/bundle1\" main:main\n+\tgit -C bundle checkout -b some-branch &&\n+\ttest_must_fail git -C bundle fetch \"../bundle1\" main:main\n '\n\n\n test_expect_success 'bundle 1 has only 3 files ' '\n-\tcd \"$D\" &&\n \ttest_bundle_object_count bundle1 3\n '\n\n test_expect_success 'unbundle 2' '\n-\tcd \"$D/bundle\" &&\n-\tgit fetch ../bundle2 main:main &&\n-\ttest \"tip\" = \"$(git log -1 --pretty=oneline main | cut -d\" \" -f2)\"\n+\tgit -C bundle fetch ../bundle2 main:main &&\n+\ttest \"tip\" = \"$(git -C bundle log -1 --pretty=oneline main | cut -d\" \" \n-f2)\"\n '\n\n test_expect_success 'bundle does not prerequisite objects' '\n-\tcd \"$D\" &&\n \ttouch file2 &&\n \tgit add file2 &&\n \tgit commit -m add.file2 file2 &&\n@@ -498,11 +486,8 @@ test_expect_success 'bundle does not prerequisite \nobjects' '\n '\n\n test_expect_success 'bundle should be able to create a full history' '\n-\n-\tcd \"$D\" &&\n \tgit tag -a -m \"1.0\" v1.0 main &&\n \tgit bundle create bundle4 v1.0\n-\n '\n\n test_expect_success 'fetch with a non-applying branch..merge' '\n@@ -553,7 +538,6 @@ test_expect_success 'quoting of a strangely named \nrepo' '\n\n test_expect_success 'bundle should record HEAD correctly' '\n\n-\tcd \"$D\" &&\n \tgit bundle create bundle5 HEAD main &&\n \tgit bundle list-heads bundle5 >actual &&\n \tfor h in HEAD refs/heads/main\n@@ -573,7 +557,6 @@ test_expect_success 'mark initial state of \norigin/main' '\n\n test_expect_success 'explicit fetch should update tracking' '\n\n-\tcd \"$D\" &&\n \tgit branch -f side &&\n \t(\n \t\tcd three &&\n@@ -588,7 +571,6 @@ test_expect_success 'explicit fetch should update \ntracking' '\n\n test_expect_success 'explicit pull should update tracking' '\n\n-\tcd \"$D\" &&\n \tgit branch -f side &&\n \t(\n \t\tcd three &&\n@@ -602,7 +584,6 @@ test_expect_success 'explicit pull should update \ntracking' '\n '\n\n test_expect_success 'explicit --refmap is allowed only with \ncommand-line refspec' '\n-\tcd \"$D\" &&\n \t(\n \t\tcd three &&\n \t\ttest_must_fail git fetch --refmap=\"*:refs/remotes/none/*\"\n@@ -610,7 +591,6 @@ test_expect_success 'explicit --refmap is allowed \nonly with command-line refspec\n '\n\n test_expect_success 'explicit --refmap option overrides remote.*.fetch' '\n-\tcd \"$D\" &&\n \tgit branch -f side &&\n \t(\n \t\tcd three &&\n@@ -625,7 +605,6 @@ test_expect_success 'explicit --refmap option \noverrides remote.*.fetch' '\n '\n\n test_expect_success 'explicitly empty --refmap option disables \nremote.*.fetch' '\n-\tcd \"$D\" &&\n \tgit branch -f side &&\n \t(\n \t\tcd three &&\n@@ -639,8 +618,6 @@ test_expect_success 'explicitly empty --refmap \noption disables remote.*.fetch' '\n '\n\n test_expect_success 'configured fetch updates tracking' '\n-\n-\tcd \"$D\" &&\n \tgit branch -f side &&\n \t(\n \t\tcd three &&\n@@ -654,7 +631,6 @@ test_expect_success 'configured fetch updates \ntracking' '\n '\n\n test_expect_success 'non-matching refspecs do not confuse tracking \nupdate' '\n-\tcd \"$D\" &&\n \tgit update-ref refs/odd/location HEAD &&\n \t(\n \t\tcd three &&\n@@ -670,15 +646,10 @@ test_expect_success 'non-matching refspecs do not \nconfuse tracking update' '\n '\n\n test_expect_success 'pushing nonexistent branch by mistake should not \nsegv' '\n-\n-\tcd \"$D\" &&\n \ttest_must_fail git push seven no:no\n-\n '\n\n test_expect_success 'auto tag following fetches minimum' '\n-\n-\tcd \"$D\" &&\n \tgit clone .git follow &&\n \tgit checkout HEAD^0 &&\n \t(\n@@ -1063,7 +1034,7 @@ test_expect_success 'fetch --prune prints the \nremotes url' '\n \t\tcd only-prunes &&\n \t\tgit fetch --prune origin 2>&1 | head -n1 >../actual\n \t) &&\n-\techo \"From ${D}/.\" >expect &&\n+\techo \"From $(pwd)/.\" >expect &&\n \ttest_cmp expect actual\n '\n\n@@ -1097,14 +1068,14 @@ test_expect_success 'fetching with auto-gc does \nnot lock up' '\n \techo \"$*\" &&\n \tfalse\n \tEOF\n-\tgit clone \"file://$D\" auto-gc &&\n+\tgit clone \"file://$(pwd)\" auto-gc &&\n \ttest_commit test2 &&\n \t(\n \t\tcd auto-gc &&\n \t\tgit config fetch.unpackLimit 1 &&\n \t\tgit config gc.autoPackLimit 1 &&\n \t\tgit config gc.autoDetach false &&\n-\t\tGIT_ASK_YESNO=\"$D/askyesno\" git fetch --verbose >fetch.out 2>&1 &&\n+\t\tGIT_ASK_YESNO=\"$(pwd)/askyesno\" git fetch --verbose >fetch.out 2>&1 &&\n \t\ttest_i18ngrep \"Auto packing the repository\" fetch.out &&\n \t\t! grep \"Should I try again\" fetch.out\n \t)\n-- \n2.39.0.152.ga5737674b6\n\n"},{"id":"524480","messageId":"CAPig+cS5CDA_yBkHa-nuR0jBg5yygwncKAs8yK_2civBMx1gVg@mail.gmail.com","threadId":"63993","inReplyTo":"8797c495-8277-4f65-845b-167542b82949@charter.net","subject":"Re: [PATCH 1/4] t5510: make confusing config cleanup more explicit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-08-19T20:16:38Z","receivedAt":"2025-08-19T20:16:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 19, 2025 at 4:05 PM Eric Sunshine wrote:\n> On 8/19/25 3:24 PM, Jeff King wrote:\n> > - test_when_finished \"git config unset remote.origin.followRemoteHEAD\" &&\n> > + test_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n>\n> For what it's worth, I have an unsent patch from a much larger unsent\n> series which cleans up the t5510 messiness differently. (The below patch\n> is probably whitespace damaged by the MUA.)\n\nFor the sake of completeness regarding the \"much larger unsent\nseries\": That series modifies `chainlint` to detect `cd` invocations\noutside of a subshell and fixes the very many instances it discovered.\nThe series ended up consisting of 41 patches.\n"},{"id":"524485","messageId":"20250819205338.GA1071667@coredump.intra.peff.net","threadId":"63993","inReplyTo":"8797c495-8277-4f65-845b-167542b82949@charter.net","subject":"Re: [PATCH 1/4] t5510: make confusing config cleanup more explicit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-19T20:53:38Z","receivedAt":"2025-08-19T20:53:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 19, 2025 at 04:03:23PM -0400, Eric Sunshine wrote:\n\n> > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> > @@ -119,7 +119,7 @@ test_expect_success \"fetch test remote HEAD change\" '\n> > -\ttest_when_finished \"git config unset remote.origin.followRemoteHEAD\" &&\n> > +\ttest_when_finished \"git -C \\\"$D/two\\\" config unset remote.origin.followRemoteHEAD\" &&\n> \n> For what it's worth, I have an unsent patch from a much larger unsent series\n> which cleans up the t5510 messiness differently. (The below patch is\n> probably whitespace damaged by the MUA.)\n\nSee my patch 2, which does this part. I split this out because it got\ncomplicated to explain why that patch wasn't breaking these lines. ;)\n\n-Peff\n"},{"id":"524512","messageId":"aKV44BDyIMyarinZ@pks.im","threadId":"63993","inReplyTo":"20250819192934.GD1059295@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] refs: do not clobber dangling symrefs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-20T07:27:28Z","receivedAt":"2025-08-20T07:27:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Aug 19, 2025 at 03:29:34PM -0400, Jeff King wrote:\n> The code for the fix is relatively straight-forward given the discussion\n> above. But note that we have to implement it independently for the files\n> and reftable backends. The \"old oid\" checks happen as part of the\n> locking process, which is implemented separately for each system. We may\n> want to factor this out somehow, but it's beyond the scope of this\n> patch.\n\nYeah, there's a bunch of duplication here in general. I originally\nwanted to refactor this at some point in time, but I never got around to\nit. Also because I kind of shied away from it: the logic to lock and\ncheck refs is quite intertwined with one another in both backends, so I\nwas afraid that this would ulmitately lead to splitting hairs.\n\n> (Another curiosity is that the messages in the reftable code are\n> marked for translation, but the ones in the files backend are not. I\n> followed local convention in each case, but we may want to harmonize\n> this at some point).\n\nOh, interesting. I guess translating these messages is the right thing\nto do, as the messages are user facing. But this definitely does not\nhave to be part of this patch series.\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 905555365b..a4419ef62d 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2512,13 +2512,37 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,\n> */\n> static enum ref_transaction_error check_old_oid(struct ref_update *update,\n> \t\t\t\t\t\tstruct object_id *oid,\n> +\t\t\t\t\t\tstruct strbuf *referent,\n> \t\t\t\t\t\tstruct strbuf *err)\n> {\n> \tif (update->flags & REF_LOG_ONLY ||\n> -\t !(update->flags & REF_HAVE_OLD) ||\n> -\t oideq(oid, &update->old_oid))\n> +\t !(update->flags & REF_HAVE_OLD))\n> \t\treturn 0;\n> \n> +\tif (oideq(oid, &update->old_oid)) {\n> +\t\t/*\n> +\t\t * Normally matching the expected old oid is enough. Either we\n> +\t\t * found the ref at the expected state, or we are creating and\n> +\t\t * expect the null oid (and likewise found nothing).\n> +\t\t *\n> +\t\t * But there is one exception for the null oid: if we found a\n> +\t\t * symref pointing to nothing we'll also get the null oid. In\n> +\t\t * regular recursive mode, that's good (we'll write to what the\n> +\t\t * symref points to, which doesn't exist). But in no-deref\n> +\t\t * mode, it means we'll clobber the symref, even though the\n> +\t\t * caller asked for this to be a creation event. So flag\n> +\t\t * that case to preserve the dangling symref.\n> +\t\t */\n> +\t\tif ((update->flags & REF_NO_DEREF) && referent->len &&\n> +\t\t is_null_oid(oid)) {\n> +\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n> +\t\t\t\t \"dangling symref already exists\",\n> +\t\t\t\t ref_update_original_update_refname(update));\n> +\t\t\treturn REF_TRANSACTION_ERROR_CREATE_EXISTS;\n> +\t\t}\n> +\t\treturn 0;\n> +\t}\n> +\n> \tif (is_null_oid(&update->old_oid)) {\n> \t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n> \t\t\t \"reference already exists\",\n\nMakes sense. If we've got an all-zero old object ID _but_ the locked\nreference points to a nonexistet ref we refuse the update.\n\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 99fafd75eb..ef98584bf9 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -1272,9 +1272,33 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor\n> \t\tret = ref_update_check_old_target(referent->buf, u, err);\n> \t\tif (ret)\n> \t\t\treturn ret;\n> -\t} else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD &&\n> -\t\t !oideq(¤t_oid, &u->old_oid)) {\n> -\t\tif (is_null_oid(&u->old_oid)) {\n> +\t} else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD) {\n> +\t\tif (oideq(¤t_oid, &u->old_oid)) {\n> +\t\t\t/*\n> +\t\t\t * Normally matching the expected old oid is enough. Either we\n> +\t\t\t * found the ref at the expected state, or we are creating and\n> +\t\t\t * expect the null oid (and likewise found nothing).\n> +\t\t\t *\n> +\t\t\t * But there is one exception for the null oid: if we found a\n> +\t\t\t * symref pointing to nothing we'll also get the null oid. In\n> +\t\t\t * regular recursive mode, that's good (we'll write to what the\n> +\t\t\t * symref points to, which doesn't exist). But in no-deref\n> +\t\t\t * mode, it means we'll clobber the symref, even though the\n> +\t\t\t * caller asked for this to be a creation event. So flag\n> +\t\t\t * that case to preserve the dangling symref.\n> +\t\t\t *\n> +\t\t\t * Everything else is OK and we can fall through to the\n> +\t\t\t * end of the conditional chain.\n> +\t\t\t */\n> +\t\t\tif ((u->flags & REF_NO_DEREF) &&\n> +\t\t\t referent->len &&\n> +\t\t\t is_null_oid(&u->old_oid)) {\n> +\t\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n> +\t\t\t\t\t \"dangling symref already exists\"),\n> +\t\t\t\t\t ref_update_original_update_refname(u));\n> +\t\t\t\treturn REF_TRANSACTION_ERROR_CREATE_EXISTS;\n> +\t\t\t}\n> +\t\t} else if (is_null_oid(&u->old_oid)) {\n\nWouldn't it be more natural to put the new check into this `if\n(is_null_oid(&u->old_oid))` branch? Makes it a bit more explicit that we\nreally only care about the case where we expect the ref to not exist.\n\nAh, no. I missed that you also change the original condition and move\nthe `oideq()` call into the whole thing. Makes sense.\n\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index d29d23cb89..29b31e3b9b 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -2310,4 +2310,25 @@ test_expect_success 'update-ref should also create reflog for HEAD' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'dangling symref not overwritten by creation' '\n> +\ttest_when_finished \"git update-ref -d refs/heads/dangling\" &&\n> +\tgit symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&\n> +\ttest_must_fail git update-ref --no-deref --stdin 2>err <<-\\EOF &&\n> +\tcreate refs/heads/dangling HEAD\n> +\tEOF\n> +\ttest_grep \"cannot lock.*dangling symref already exists\" err &&\n> +\ttest_must_fail git rev-parse --verify refs/heads/dangling &&\n> +\ttest_must_fail git rev-parse --verify refs/heads/does-not-exist\n> +'\n> +\n> +test_expect_success 'dangling symref overwritten without old oid' '\n> +\ttest_when_finished \"git update-ref -d refs/heads/dangling\" &&\n> +\tgit symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&\n> +\tgit update-ref --no-deref --stdin <<-\\EOF &&\n> +\tupdate refs/heads/dangling HEAD\n> +\tEOF\n> +\tgit rev-parse --verify refs/heads/dangling &&\n> +\ttest_must_fail git rev-parse --verify refs/heads/does-not-exist\n\nDo we also want to verify that the dangling symref got converted into a\nnormal ref? Or do we already have other tests that do so?\n\nPatrick\n"},{"id":"524548","messageId":"20250820191441.GA1661980@coredump.intra.peff.net","threadId":"63993","inReplyTo":"aKV44BDyIMyarinZ@pks.im","subject":"Re: [PATCH 4/4] refs: do not clobber dangling symrefs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-20T19:14:41Z","receivedAt":"2025-08-20T19:14:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 20, 2025 at 09:27:28AM +0200, Patrick Steinhardt wrote:\n\n> > (Another curiosity is that the messages in the reftable code are\n> > marked for translation, but the ones in the files backend are not. I\n> > followed local convention in each case, but we may want to harmonize\n> > this at some point).\n> \n> Oh, interesting. I guess translating these messages is the right thing\n> to do, as the messages are user facing. But this definitely does not\n> have to be part of this patch series.\n\nI know we left some unpack-trees messages untranslated because we\nthought users might depend on them (see unpack_plumbing_errors). I\nwondered if we might have done the same for the ref messages, but\nthere's certainly no infrastructure around it. So it may just have been\nthe case that nobody (yet) bothered to mark them.\n\n> > +\t} else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD) {\n> > +\t\tif (oideq(¤t_oid, &u->old_oid)) {\n> > +\t\t\t/*\n> > +\t\t\t * Normally matching the expected old oid is enough. Either we\n> > +\t\t\t * found the ref at the expected state, or we are creating and\n> > +\t\t\t * expect the null oid (and likewise found nothing).\n> > +\t\t\t *\n> > +\t\t\t * But there is one exception for the null oid: if we found a\n> > +\t\t\t * symref pointing to nothing we'll also get the null oid. In\n> > +\t\t\t * regular recursive mode, that's good (we'll write to what the\n> > +\t\t\t * symref points to, which doesn't exist). But in no-deref\n> > +\t\t\t * mode, it means we'll clobber the symref, even though the\n> > +\t\t\t * caller asked for this to be a creation event. So flag\n> > +\t\t\t * that case to preserve the dangling symref.\n> > +\t\t\t *\n> > +\t\t\t * Everything else is OK and we can fall through to the\n> > +\t\t\t * end of the conditional chain.\n> > +\t\t\t */\n> > +\t\t\tif ((u->flags & REF_NO_DEREF) &&\n> > +\t\t\t referent->len &&\n> > +\t\t\t is_null_oid(&u->old_oid)) {\n> > +\t\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n> > +\t\t\t\t\t \"dangling symref already exists\"),\n> > +\t\t\t\t\t ref_update_original_update_refname(u));\n> > +\t\t\t\treturn REF_TRANSACTION_ERROR_CREATE_EXISTS;\n> > +\t\t\t}\n> > +\t\t} else if (is_null_oid(&u->old_oid)) {\n> \n> Wouldn't it be more natural to put the new check into this `if\n> (is_null_oid(&u->old_oid))` branch? Makes it a bit more explicit that we\n> really only care about the case where we expect the ref to not exist.\n> \n> Ah, no. I missed that you also change the original condition and move\n> the `oideq()` call into the whole thing. Makes sense.\n\nYep, exactly. If we did the is_null_oid() check first then we'd have to\ncheck oideq() again inside that block, duplicating that logic. So there\nis no winning. :) I tried to keep the logic as close to the original as\npossible.\n\n> > +test_expect_success 'dangling symref overwritten without old oid' '\n> > +\ttest_when_finished \"git update-ref -d refs/heads/dangling\" &&\n> > +\tgit symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&\n> > +\tgit update-ref --no-deref --stdin <<-\\EOF &&\n> > +\tupdate refs/heads/dangling HEAD\n> > +\tEOF\n> > +\tgit rev-parse --verify refs/heads/dangling &&\n> > +\ttest_must_fail git rev-parse --verify refs/heads/does-not-exist\n> \n> Do we also want to verify that the dangling symref got converted into a\n> normal ref? Or do we already have other tests that do so?\n\nMy intent was that the above test does so: we know that \"dangling\" now\npoints to a valid oid and that \"does-not-exist\" was not written to.\nErgo, \"dangling\" is now a normal ref (the only other option is that it\nremained a symref and was pointed somewhere else entirely, but that\nseems like an unlikely bug to have).\n\n-Peff\n"},{"id":"524850","messageId":"aKtq47vmCrUZCUCF@szeder.dev","threadId":"63993","inReplyTo":"20250819192716.GC1059295@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] t5510: prefer \"git -C\" to subshell for followRemoteHEAD tests","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2025-08-24T19:41:23Z","receivedAt":"2025-08-24T19:41:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Aug 19, 2025 at 03:27:16PM -0400, Jeff King wrote:\n> These tests set config within a sub-repo using (cd two && git config),\n> and then a separate test_when_finished outside the subshell to clean it\n> up. We can't use test_config to do this, because the cleanup command it\n> registers inside the subshell would be lost. Nor can we do it before\n> entering the subshell, because the config has to be set after some other\n> commands are run.\n> \n> Let's switch these tests to use \"git -C\" for each command instead of a\n> subshell. That lets us use test_config (with -C also) at the appropriate\n> part of the test. And we no longer need the manual cleanup command.\n> \n> Signed-off-by: Jeff King \n> ---\n> It is perhaps debatable whether this makes the result more readable.\n> It's fewer lines, but there is \"-C\" sprinkled everywhere. So if people\n> find this ugly we can drop it (and I'd rewrite patch 4 to use the\n> subshell form in its new test).\n\nI for one think that the original is much more readable.\n\nWith the subshell it's quite clear, even at a cursory glance, which\ncommands are executed in a subdirectory, but when using '-C dir' all\nover we have to look closely. Furthermore, when there is a command\noutside of the subshell, we can be fairly sure that it's intentional,\nbut when a command without '-C dir' lurks among many others using '-C\ndir', then we can't be so sure, but have to investigate whether that\nwas intentional or oversight.\n\n> t/t5510-fetch.sh | 202 +++++++++++++++++++----------------------------\n> 1 file changed, 83 insertions(+), 119 deletions(-)\n> \n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 93e309e213..24379ec7aa 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -123,149 +123,113 @@ test_expect_success \"fetch test remote HEAD change\" '\n> '\n> \n> test_expect_success \"fetch test followRemoteHEAD never\" '\n> -\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n> -\t(\n> -\t\tcd two &&\n> -\t\tgit update-ref --no-deref -d refs/remotes/origin/HEAD &&\n> -\t\tgit config set remote.origin.followRemoteHEAD \"never\" &&\n> -\t\tGIT_TRACE_PACKET=$PWD/trace.out git fetch &&\n> -\t\t# Confirm that we do not even ask for HEAD when we are\n> -\t\t# not going to act on it.\n> -\t\ttest_grep ! \"ref-prefix HEAD\" trace.out &&\n> -\t\ttest_must_fail git rev-parse --verify refs/remotes/origin/HEAD\n> -\t)\n> +\tgit -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&\n> +\ttest_config -C two remote.origin.followRemoteHEAD \"never\" &&\n> +\tGIT_TRACE_PACKET=$PWD/trace.out git -C two fetch &&\n> +\t# Confirm that we do not even ask for HEAD when we are\n> +\t# not going to act on it.\n> +\ttest_grep ! \"ref-prefix HEAD\" trace.out &&\n> +\ttest_must_fail git -C two rev-parse --verify refs/remotes/origin/HEAD\n> '\n> \n> test_expect_success \"fetch test followRemoteHEAD warn no change\" '\n> -\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n> -\t(\n> -\t\tcd two &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/other &&\n> -\t\tgit remote set-head origin other &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/main &&\n> -\t\tgit config set remote.origin.followRemoteHEAD \"warn\" &&\n> -\t\tgit fetch >output &&\n> -\t\techo \"${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ},\" \\\n> -\t\t\t\"but we have ${SQ}other${SQ} locally.\" >expect &&\n> -\t\ttest_cmp expect output &&\n> -\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n> -\t\tbranch=$(git rev-parse refs/remotes/origin/other) &&\n> -\t\ttest \"z$head\" = \"z$branch\"\n> -\t)\n> +\tgit -C two rev-parse --verify refs/remotes/origin/other &&\n> +\tgit -C two remote set-head origin other &&\n> +\tgit -C two rev-parse --verify refs/remotes/origin/HEAD &&\n> +\tgit -C two rev-parse --verify refs/remotes/origin/main &&\n> +\ttest_config -C two remote.origin.followRemoteHEAD \"warn\" &&\n> +\tgit -C two fetch >output &&\n> +\techo \"${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ},\" \\\n> +\t\t\"but we have ${SQ}other${SQ} locally.\" >expect &&\n> +\ttest_cmp expect output &&\n> +\thead=$(git -C two rev-parse refs/remotes/origin/HEAD) &&\n> +\tbranch=$(git -C two rev-parse refs/remotes/origin/other) &&\n> +\ttest \"z$head\" = \"z$branch\"\n> '\n> \n> test_expect_success \"fetch test followRemoteHEAD warn create\" '\n> -\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n> -\t(\n> -\t\tcd two &&\n> -\t\tgit update-ref --no-deref -d refs/remotes/origin/HEAD &&\n> -\t\tgit config set remote.origin.followRemoteHEAD \"warn\" &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/main &&\n> -\t\toutput=$(git fetch) &&\n> -\t\ttest \"z\" = \"z$output\" &&\n> -\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n> -\t\tbranch=$(git rev-parse refs/remotes/origin/main) &&\n> -\t\ttest \"z$head\" = \"z$branch\"\n> -\t)\n> +\tgit -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&\n> +\ttest_config -C two remote.origin.followRemoteHEAD \"warn\" &&\n> +\tgit -C two rev-parse --verify refs/remotes/origin/main &&\n> +\toutput=$(git -C two fetch) &&\n> +\ttest \"z\" = \"z$output\" &&\n> +\thead=$(git -C two rev-parse refs/remotes/origin/HEAD) &&\n> +\tbranch=$(git -C two rev-parse refs/remotes/origin/main) &&\n> +\ttest \"z$head\" = \"z$branch\"\n> '\n> \n> test_expect_success \"fetch test followRemoteHEAD warn detached\" '\n> -\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n> -\t(\n> -\t\tcd two &&\n> -\t\tgit update-ref --no-deref -d refs/remotes/origin/HEAD &&\n> -\t\tgit update-ref refs/remotes/origin/HEAD HEAD &&\n> -\t\tHEAD=$(git log --pretty=\"%H\") &&\n> -\t\tgit config set remote.origin.followRemoteHEAD \"warn\" &&\n> -\t\tgit fetch >output &&\n> -\t\techo \"${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ},\" \\\n> -\t\t\t\"but we have a detached HEAD pointing to\" \\\n> -\t\t\t\"${SQ}${HEAD}${SQ} locally.\" >expect &&\n> -\t\ttest_cmp expect output\n> -\t)\n> +\tgit -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&\n> +\tgit -C two update-ref refs/remotes/origin/HEAD HEAD &&\n> +\tHEAD=$(git -C two log --pretty=\"%H\") &&\n> +\ttest_config -C two remote.origin.followRemoteHEAD \"warn\" &&\n> +\tgit -C two fetch >output &&\n> +\techo \"${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ},\" \\\n> +\t\t\"but we have a detached HEAD pointing to\" \\\n> +\t\t\"${SQ}${HEAD}${SQ} locally.\" >expect &&\n> +\ttest_cmp expect output\n> '\n> \n> test_expect_success \"fetch test followRemoteHEAD warn quiet\" '\n> -\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n> -\t(\n> -\t\tcd two &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/other &&\n> -\t\tgit remote set-head origin other &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/main &&\n> -\t\tgit config set remote.origin.followRemoteHEAD \"warn\" &&\n> -\t\toutput=$(git fetch --quiet) &&\n> -\t\ttest \"z\" = \"z$output\" &&\n> -\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n> -\t\tbranch=$(git rev-parse refs/remotes/origin/other) &&\n> -\t\ttest \"z$head\" = \"z$branch\"\n> -\t)\n> +\tgit -C two rev-parse --verify refs/remotes/origin/other &&\n> +\tgit -C two remote set-head origin other &&\n> +\tgit -C two rev-parse --verify refs/remotes/origin/HEAD &&\n> +\tgit -C two rev-parse --verify refs/remotes/origin/main &&\n> +\ttest_config -C two remote.origin.followRemoteHEAD \"warn\" &&\n> +\toutput=$(git -C two fetch --quiet) &&\n> +\ttest \"z\" = \"z$output\" &&\n> +\thead=$(git -C two rev-parse refs/remotes/origin/HEAD) &&\n> +\tbranch=$(git -C two rev-parse refs/remotes/origin/other) &&\n> +\ttest \"z$head\" = \"z$branch\"\n> '\n> \n> test_expect_success \"fetch test followRemoteHEAD warn-if-not-branch branch is same\" '\n> -\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n> -\t(\n> -\t\tcd two &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/other &&\n> -\t\tgit remote set-head origin other &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/main &&\n> -\t\tgit config set remote.origin.followRemoteHEAD \"warn-if-not-main\" &&\n> -\t\tactual=$(git fetch) &&\n> -\t\ttest \"z\" = \"z$actual\" &&\n> -\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n> -\t\tbranch=$(git rev-parse refs/remotes/origin/other) &&\n> -\t\ttest \"z$head\" = \"z$branch\"\n> -\t)\n> +\tgit -C two rev-parse --verify refs/remotes/origin/other &&\n> +\tgit -C two remote set-head origin other &&\n> +\tgit -C two rev-parse --verify refs/remotes/origin/HEAD &&\n> +\tgit -C two rev-parse --verify refs/remotes/origin/main &&\n> +\ttest_config -C two remote.origin.followRemoteHEAD \"warn-if-not-main\" &&\n> +\tactual=$(git -C two fetch) &&\n> +\ttest \"z\" = \"z$actual\" &&\n> +\thead=$(git -C two rev-parse refs/remotes/origin/HEAD) &&\n> +\tbranch=$(git -C two rev-parse refs/remotes/origin/other) &&\n> +\ttest \"z$head\" = \"z$branch\"\n> '\n> \n> test_expect_success \"fetch test followRemoteHEAD warn-if-not-branch branch is different\" '\n> -\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n> -\t(\n> -\t\tcd two &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/other &&\n> -\t\tgit remote set-head origin other &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/main &&\n> -\t\tgit config set remote.origin.followRemoteHEAD \"warn-if-not-some/different-branch\" &&\n> -\t\tgit fetch >actual &&\n> -\t\techo \"${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ},\" \\\n> -\t\t\t\"but we have ${SQ}other${SQ} locally.\" >expect &&\n> -\t\ttest_cmp expect actual &&\n> -\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n> -\t\tbranch=$(git rev-parse refs/remotes/origin/other) &&\n> -\t\ttest \"z$head\" = \"z$branch\"\n> -\t)\n> +\tgit -C two rev-parse --verify refs/remotes/origin/other &&\n> +\tgit -C two remote set-head origin other &&\n> +\tgit -C two rev-parse --verify refs/remotes/origin/HEAD &&\n> +\tgit -C two rev-parse --verify refs/remotes/origin/main &&\n> +\ttest_config -C two remote.origin.followRemoteHEAD \"warn-if-not-some/different-branch\" &&\n> +\tgit -C two fetch >actual &&\n> +\techo \"${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ},\" \\\n> +\t\t\"but we have ${SQ}other${SQ} locally.\" >expect &&\n> +\ttest_cmp expect actual &&\n> +\thead=$(git -C two rev-parse refs/remotes/origin/HEAD) &&\n> +\tbranch=$(git -C two rev-parse refs/remotes/origin/other) &&\n> +\ttest \"z$head\" = \"z$branch\"\n> '\n> \n> test_expect_success \"fetch test followRemoteHEAD always\" '\n> -\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n> -\t(\n> -\t\tcd two &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/other &&\n> -\t\tgit remote set-head origin other &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/HEAD &&\n> -\t\tgit rev-parse --verify refs/remotes/origin/main &&\n> -\t\tgit config set remote.origin.followRemoteHEAD \"always\" &&\n> -\t\tgit fetch &&\n> -\t\thead=$(git rev-parse refs/remotes/origin/HEAD) &&\n> -\t\tbranch=$(git rev-parse refs/remotes/origin/main) &&\n> -\t\ttest \"z$head\" = \"z$branch\"\n> -\t)\n> +\tgit -C two rev-parse --verify refs/remotes/origin/other &&\n> +\tgit -C two remote set-head origin other &&\n> +\tgit -C two rev-parse --verify refs/remotes/origin/HEAD &&\n> +\tgit -C two rev-parse --verify refs/remotes/origin/main &&\n> +\ttest_config -C two remote.origin.followRemoteHEAD \"always\" &&\n> +\tgit -C two fetch &&\n> +\thead=$(git -C two rev-parse refs/remotes/origin/HEAD) &&\n> +\tbranch=$(git -C two rev-parse refs/remotes/origin/main) &&\n> +\ttest \"z$head\" = \"z$branch\"\n> '\n> \n> test_expect_success 'followRemoteHEAD does not kick in with refspecs' '\n> -\ttest_when_finished \"git -C two config unset remote.origin.followRemoteHEAD\" &&\n> -\t(\n> -\t\tcd two &&\n> -\t\tgit remote set-head origin other &&\n> -\t\tgit config set remote.origin.followRemoteHEAD always &&\n> -\t\tgit fetch origin refs/heads/main:refs/remotes/origin/main &&\n> -\t\techo refs/remotes/origin/other >expect &&\n> -\t\tgit symbolic-ref refs/remotes/origin/HEAD >actual &&\n> -\t\ttest_cmp expect actual\n> -\t)\n> +\tgit -C two remote set-head origin other &&\n> +\ttest_config -C two remote.origin.followRemoteHEAD always &&\n> +\tgit -C two fetch origin refs/heads/main:refs/remotes/origin/main &&\n> +\techo refs/remotes/origin/other >expect &&\n> +\tgit -C two symbolic-ref refs/remotes/origin/HEAD >actual &&\n> +\ttest_cmp expect actual\n> '\n> \n> test_expect_success 'fetch --prune on its own works as expected' '\n> -- \n> 2.51.0.326.gecbb38d78e\n> \n> \n"},{"id":"524867","messageId":"xmqqfrdftnet.fsf@gitster.g","threadId":"63993","inReplyTo":"aKtq47vmCrUZCUCF@szeder.dev","subject":"Re: [PATCH 3/4] t5510: prefer \"git -C\" to subshell for followRemoteHEAD tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-25T15:46:02Z","receivedAt":"2025-08-25T15:46:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> I for one think that the original is much more readable.\n>\n> With the subshell it's quite clear, even at a cursory glance, which\n> commands are executed in a subdirectory, but when using '-C dir' all\n> over we have to look closely. Furthermore, when there is a command\n> outside of the subshell, we can be fairly sure that it's intentional,\n> but when a command without '-C dir' lurks among many others using '-C\n> dir', then we can't be so sure, but have to investigate whether that\n> was intentional or oversight.\n\nUnfortunately I tend to agree. A few downsides I find a bit\nproblematic in the subshell solution are\n\n - The temporary files subshell creates sometimes are harder to follow\n\n \t( cd there && git foo >../actual && ... ) &&\n\ttest_cmp expect actual\n\n than they need to be. With \"git -C there\", obviously paths used\n when they get created and used match:\n\n\tgit -C there >actual &&\n\ttest_cmp expect actual\n\n - Test framework helpers like test_when_finished and test_commit\n that rely on the global shell variables to keep track of the\n states do not work well inside subshells.\n\n - Some platforms have expensive forks.\n\nbut in a context that these are not huge problems, I tend to prefer\nthe \"cd in a subshell\" pattern over\n\n>> +\tgit -C two update-ref --no-deref -d refs/remotes/origin/HEAD &&\n>> +\ttest_config -C two remote.origin.followRemoteHEAD \"never\" &&\n>> +\tGIT_TRACE_PACKET=$PWD/trace.out git -C two fetch &&\n\nespecially where \"-C there\" is harder to spot. If the above were\n\n\tgit -C two do this &&\n\tgit -C two do that >actual &&\n\tgit -C two do something else &&\n\ni.e., with aligned \"-C two\" to make it obvious that these are doing\ntheir thing in the same other place, the tradeoff might have been\ndifferent, though.\n"},{"id":"524916","messageId":"20250826034434.GB388997@coredump.intra.peff.net","threadId":"63993","inReplyTo":"xmqqfrdftnet.fsf@gitster.g","subject":"Re: [PATCH 3/4] t5510: prefer \"git -C\" to subshell for followRemoteHEAD tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-26T03:44:34Z","receivedAt":"2025-08-26T03:44:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 25, 2025 at 08:46:02AM -0700, Junio C Hamano wrote:\n\n> > I for one think that the original is much more readable.\n> >\n> > With the subshell it's quite clear, even at a cursory glance, which\n> > commands are executed in a subdirectory, but when using '-C dir' all\n> > over we have to look closely. Furthermore, when there is a command\n> > outside of the subshell, we can be fairly sure that it's intentional,\n> > but when a command without '-C dir' lurks among many others using '-C\n> > dir', then we can't be so sure, but have to investigate whether that\n> > was intentional or oversight.\n> \n> Unfortunately I tend to agree. A few downsides I find a bit\n> problematic in the subshell solution are\n> [...]\n\nOK, I am happy to drop that patch (3/4). The resulting change to the\nfinal patch to match style would be:\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 6e8b741491..bac464a9ec 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -269,12 +269,16 @@ test_expect_success 'followRemoteHEAD does not kick in with refspecs' '\n '\n \n test_expect_success 'followRemoteHEAD create does not overwrite dangling symref' '\n-\tgit -C two remote add -m does-not-exist custom-head ../one &&\n-\ttest_config -C two remote.custom-head.followRemoteHEAD create &&\n-\tgit -C two fetch custom-head &&\n-\techo refs/remotes/custom-head/does-not-exist >expect &&\n-\tgit -C two symbolic-ref refs/remotes/custom-head/HEAD >actual &&\n-\ttest_cmp expect actual\n+\ttest_when_finished \"git -C two config unset remote.custom-head.followRemoteHEAD\" &&\n+\t(\n+\t\tcd two &&\n+\t\tgit remote add -m does-not-exist custom-head ../one &&\n+\t\tgit config remote.custom-head.followRemoteHEAD create &&\n+\t\tgit fetch custom-head &&\n+\t\techo refs/remotes/custom-head/does-not-exist >expect &&\n+\t\tgit symbolic-ref refs/remotes/custom-head/HEAD >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n '\n \n test_expect_success 'fetch --prune on its own works as expected' '\n\n\nBut both patches are already in 'next'. How do you want to proceed? I\ncan prepare a patch on top converting back to sub-shells. Or if we are\ngoing to do the post-release rewind of next, that is an opportunity to\nfix things cleanly. Or we could leave it as-is if it is not worth the\nbother at this point.\n\n-Peff\n"},{"id":"524959","messageId":"xmqq7byqm8o4.fsf@gitster.g","threadId":"63993","inReplyTo":"20250826034434.GB388997@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] t5510: prefer \"git -C\" to subshell for followRemoteHEAD tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-26T14:58:35Z","receivedAt":"2025-08-26T14:58:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Unfortunately I tend to agree. A few downsides I find a bit\n>> problematic in the subshell solution are\n>> [...]\n>\n> OK, I am happy to drop that patch (3/4). The resulting change to the\n> final patch to match style would be:\n> ...\n> But both patches are already in 'next'. How do you want to proceed? I\n> can prepare a patch on top converting back to sub-shells. Or if we are\n> going to do the post-release rewind of next, that is an opportunity to\n> fix things cleanly. Or we could leave it as-is if it is not worth the\n> bother at this point.\n\nThe last one ;-). This is the kind of preference that falls into\n\"once the code is written in one way, it is not worth the patch\nnoise to rewrite it in the other way\" category.\n"},{"id":"526923","messageId":"20250922122332.584428-1-toon@iotcl.com","threadId":"63993","inReplyTo":"20250819192934.GD1059295@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] refs: do not clobber dangling symrefs","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-09-22T12:23:32Z","receivedAt":"2025-09-22T12:24:07Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Hi Peff,\n\nAt $DAYJOB we hit into an edge-case where this patch breaks our expectancies.\n\nWe use `update FOO_HEAD 000...000 000..000` to delete a symref, if that symref\nis dangling (otherwise the old oid would have resolved to something). I've\nattached a patch that would allow this (on top of your patches). Do you think it\nmakes sense to allow this scenario?\n\n--\nCheers,\nToon\n\n\n--- >8 ---\nSubject: [PATCH] refs: allow deleting dangling symrefs by updating to zero oid\n\nIn 450fc2bace (refs: do not clobber dangling symrefs, 2025-08-19) we\nchanged how dangling symrefs are dealt with. This guards us from\ncreating a symref, while it already exists.\n\nBut this breaks behavior when you want to delete such dangling symref.\nWhen you're aware your symref is dangling, you know the old oid resolves\nto the null oid, and thus you can pass that together with the null oid\nthe new oid to delete the symref. Thus when the new oid is the null oid,\ncontinue the ref update as before the change mentioned earlier.\n\nSigned-off-by: Toon Claes \n---\n refs/files-backend.c | 2 +-\n refs/reftable-backend.c | 2 +-\n t/t1400-update-ref.sh | 9 +++++++++\n 3 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 1b3bf26add..5e46d3a110 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2537,7 +2537,7 @@ static enum ref_transaction_error check_old_oid(struct ref_update *update,\n \t\t * that case to preserve the dangling symref.\n \t\t */\n \t\tif ((update->flags & REF_NO_DEREF) && referent->len &&\n-\t\t is_null_oid(oid)) {\n+\t\t is_null_oid(oid) && !is_null_oid(&update->new_oid)) {\n \t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n \t\t\t\t \"dangling symref already exists\",\n \t\t\t\t ref_update_original_update_refname(update));\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 9e889da2ff..ed505f6054 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1294,7 +1294,7 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor\n \t\t\t */\n \t\t\tif ((u->flags & REF_NO_DEREF) &&\n \t\t\t referent->len &&\n-\t\t\t is_null_oid(&u->old_oid)) {\n+\t\t\t is_null_oid(&u->old_oid) && !is_null_oid(&u->new_oid)) {\n \t\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n \t\t\t\t\t \"dangling symref already exists\"),\n \t\t\t\t\t ref_update_original_update_refname(u));\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex b7415ec9d5..85cd9da0af 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -2389,4 +2389,13 @@ test_expect_success 'dangling symref overwritten without old oid' '\n \ttest_must_fail git rev-parse --verify refs/heads/does-not-exist\n '\n\n+test_expect_success 'dangling symref delete with old oid zero' '\n+\ttest_when_finished \"git update-ref -d refs/heads/dangling\" &&\n+\tgit symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&\n+\techo \"update refs/heads/dangling $Z $Z\" >stdin &&\n+\tgit update-ref --no-deref --stdin writes:\n\n> We use `update FOO_HEAD 000...000 000..000` to delete a symref, if that symref\n> is dangling (otherwise the old oid would have resolved to something). I've\n> attached a patch that would allow this (on top of your patches). Do you think it\n> makes sense to allow this scenario?\n> ...\n> +\ttest_when_finished \"git update-ref -d refs/heads/dangling\" &&\n> +\tgit symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&\n> +\techo \"update refs/heads/dangling $Z $Z\" >stdin &&\n> +\tgit update-ref --no-deref --stdin +\ttest_must_fail git rev-parse --verify refs/heads/dangling &&\n> +\ttest_must_fail git rev-parse --verify refs/heads/does-not-exist\n> +'\n> +\n> test_done\n> --\n> 2.51.0\n"},{"id":"526953","messageId":"20250922171203.GA2202085@coredump.intra.peff.net","threadId":"63993","inReplyTo":"20250922122332.584428-1-toon@iotcl.com","subject":"Re: [PATCH 4/4] refs: do not clobber dangling symrefs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-22T17:12:03Z","receivedAt":"2025-09-22T17:12:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 22, 2025 at 02:23:32PM +0200, Toon Claes wrote:\n\n> At $DAYJOB we hit into an edge-case where this patch breaks our expectancies.\n> \n> We use `update FOO_HEAD 000...000 000..000` to delete a symref, if that symref\n> is dangling (otherwise the old oid would have resolved to something). I've\n> attached a patch that would allow this (on top of your patches). Do you think it\n> makes sense to allow this scenario?\n\nHmm. That's a funny command. You are providing _two_ null oids. The\nfirst one says \"this should be a deletion\" and the second one says \"the\nprevious state is that this should be deleted\". So it should always be a\nnoop, if we are checking both sides.\n\nI think the \"right\" way to say that is just:\n\n update FOO_HEAD 000...000\n\nwith no old-oid field at all. Or just:\n\n delete FOO_HEAD\n\nbut the two are internally the same thing.\n\nSo I think allowing this is working against what the patch is trying to\ndo, which is to consistently enforce the old-oid match that the user\nasked for. The only thing that makes it an oddball is that it is\ninherently a broken thing to ask for in the first place (at least under\nthe new, enforced regime). So we could perhaps allow it as a special\ncase for historical reasons without hurting anybody too badly.\n\nI'd prefer not to do that, just because the refs code is already\ncomplicated enough. But whether that's practical would depend on how\nwidespread this pattern is. Presumably it would not be that big a deal\nto fix what you're sending (and assuming this is Gitaly, I'd guess that\nit is bundled along with Git, so you are not that worried about people\nusing new Git with old Gitaly). But I'm not sure how we'd find out if\nother people are doing the same thing in the wild.\n\nSo I dunno. My inclination is to say that the double-null-oid invocation\nis weird and wrong, and callers should update if they need to. But I\ncould be convinced otherwise.\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 1b3bf26add..5e46d3a110 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2537,7 +2537,7 @@ static enum ref_transaction_error check_old_oid(struct ref_update *update,\n> \t\t * that case to preserve the dangling symref.\n> \t\t */\n> \t\tif ((update->flags & REF_NO_DEREF) && referent->len &&\n> -\t\t is_null_oid(oid)) {\n> +\t\t is_null_oid(oid) && !is_null_oid(&update->new_oid)) {\n> \t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n> \t\t\t\t \"dangling symref already exists\",\n> \t\t\t\t ref_update_original_update_refname(update));\n\nI think the implementation here (and the matching one in the reftable\ncode) is correct for what you want to do. We should probably note the\nspecial case in the comment above, too.\n\n-Peff\n"},{"id":"526954","messageId":"20250922172140.GB2202085@coredump.intra.peff.net","threadId":"63993","inReplyTo":"xmqqwm5qv5xh.fsf@gitster.g","subject":"Re: [PATCH 4/4] refs: do not clobber dangling symrefs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-22T17:21:40Z","receivedAt":"2025-09-22T17:21:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 22, 2025 at 08:54:34AM -0700, Junio C Hamano wrote:\n\n> Toon Claes writes:\n> \n> > We use `update FOO_HEAD 000...000 000..000` to delete a symref, if that symref\n> > is dangling (otherwise the old oid would have resolved to something). I've\n> > attached a patch that would allow this (on top of your patches). Do you think it\n> > makes sense to allow this scenario?\n> > ...\n> > +\ttest_when_finished \"git update-ref -d refs/heads/dangling\" &&\n> > +\tgit symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&\n> > +\techo \"update refs/heads/dangling $Z $Z\" >stdin &&\n> > +\tgit update-ref --no-deref --stdin \n> \"git update-ref --help\" seems to show that the \"--stdin\" mode has a\n> separate command that is designed for exactly the purpose of removing\n> a symbolic ref, though. If you are changing the semantics of \"update\"\n> to make it safer while dealing with a dangling symbolic ref, do you\n> also need to touch the code path that handles \"symref-delete\" command?\n\nI don't think so. Whatever we are trying to write (whether a regular\nref, a symref, or a deletion), the \"check the old value\" code path ends\nup in the same place.\n\nIMHO the directives for \"update-ref --stdin\" are a bit mis-designed.\nAll of update/delete/verify should accept either \"old-oid\" or\n\"old-target\" (you do not need it for create, which always implies an\nold-oid of all-zeroes).\n\nAnd then symref-* is used when you want the _new_ thing to be a symref.\nSo symref-delete is not needed at all. You just have symref-* directives\nfor create/update/verify. Which almost could be replaced by \"ref\n\", but IIRC there was some syntactic ambiguity (because we\nallow new-target to be a ref, so you'd have to pick some invalid name\nlike \":symref\").\n\nIt is probably too late now to switch from \"symref-update foo\" to\n\"update :ref foo\" (and again, I think that may have even been considered\nand rejected). But we could add support for \"ref \" to the\nnon-symref commands. That is not just a syntactic weakness, but\nsomething you literally _can't_ do now (convert a symref into a regular\nref atomically).\n\nAnyway, all very off-topic for Toon's issue, though. I think his patch\nas-is does the right thing for his case, if we want to loosen it for\nhistorical reasons (see my other response).\n\n-Peff\n"},{"id":"526957","messageId":"xmqqecryv192.fsf@gitster.g","threadId":"63993","inReplyTo":"20250922172140.GB2202085@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] refs: do not clobber dangling symrefs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-22T17:35:37Z","receivedAt":"2025-09-22T17:35:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... But we could add support for \"ref \" to the\n> non-symref commands. That is not just a syntactic weakness, but\n> something you literally _can't_ do now (convert a symref into a regular\n> ref atomically).\n\nOK. Your explanation makes sense.\n\n"},{"id":"527037","messageId":"87cy7hy0gc.fsf@iotcl.com","threadId":"63993","inReplyTo":"20250922171203.GA2202085@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] refs: do not clobber dangling symrefs","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-09-23T09:36:51Z","receivedAt":"2025-09-23T09:37:05Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Jeff King writes:\n\n> So I dunno. My inclination is to say that the double-null-oid invocation\n> is weird and wrong, and callers should update if they need to. But I\n> could be convinced otherwise.\n\nThanks for your feedback, and I have to agree. I'll get in touch with\nthe Gitaly team to see if we can rid of this odd invocation. For the\nrecord, this conversation has been happening here[1].\n\n[1]: https://gitlab.com/gitlab-org/gitaly/-/merge_requests/8161#note_2767808133\n\n\n-- \nCheers,\nToon\n"},{"id":"527109","messageId":"20250923173322.GA1136654@coredump.intra.peff.net","threadId":"63993","inReplyTo":"87cy7hy0gc.fsf@iotcl.com","subject":"Re: [PATCH 4/4] refs: do not clobber dangling symrefs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-23T17:33:22Z","receivedAt":"2025-09-23T17:33:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 23, 2025 at 11:36:51AM +0200, Toon Claes wrote:\n\n> Jeff King writes:\n> \n> > So I dunno. My inclination is to say that the double-null-oid invocation\n> > is weird and wrong, and callers should update if they need to. But I\n> > could be convinced otherwise.\n> \n> Thanks for your feedback, and I have to agree. I'll get in touch with\n> the Gitaly team to see if we can rid of this odd invocation. For the\n> record, this conversation has been happening here[1].\n> \n> [1]: https://gitlab.com/gitlab-org/gitaly/-/merge_requests/8161#note_2767808133\n\nLooking over that conversation, I do think you might consider using\nsymref-delete. As noted there, doing \"delete \" is going\nto delete unconditionally, whether it's a symref, a real ref, or nothing\nis there at all.\n\nIf you know it's a symref pointing to \"refs/heads/foo\", then the safest\nthing is:\n\n symref-delete FOO_HEAD refs/heads/foo\n\nwhich guarantees the operation is doing what you expected.\n\nThere's an open question there of: how do I know what it's pointing to?\nBut that's kind of the point of the \"old-target\" (and \"old-oid\")\noptions. They take information you discovered previously non-atomically\nand atomically perform the operation while checking (under lock) that\nthings haven't changed unexpectedly.\n\nSo from the test perspective, I think you just know what's in the test\nfixture. From the Gitaly API perspective, the caller should have some\nidea of what they're deleting (just like they should for a real ref).\n\n-Peff\n"}]}