{"thread":{"id":"65676","subject":"[PATCH] receive-pack: fix updateInstead with core.worktree","startedAt":"2026-05-22T15:45:12Z","lastAt":"2026-05-25T22:54:14Z","messageCount":5,"participants":["Alyssa Ross","Kristoffer Haugsbakk","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"543923","messageId":"20260522154418.5883-1-hi@alyssa.is","threadId":"65676","inReplyTo":null,"subject":"[PATCH] receive-pack: fix updateInstead with core.worktree","fromName":"Alyssa Ross","fromEmail":"hi@alyssa.is","sentAt":"2026-05-22T15:44:18Z","receivedAt":"2026-05-22T15:45:12Z","isPatch":true,"body":"This used to work, but when push_to_checkout() started being called\nbefore push_to_deploy(), push_to_checkout()'s side effect of adding\nGIT_WORK_TREE to the same environment that would be used by\npush_to_deploy() wasn't taken into account.  Fix by only mutating the\nenvironment for push_to_commit(), rather than the shared environment.\n\nFixes: a8cc594333 (\"hooks: fix an obscure TOCTOU \"did we just run a hook?\" race\")\nSigned-off-by: Alyssa Ross <hi@alyssa.is>\n---\n builtin/receive-pack.c |  2 +-\n t/t5516-fetch-push.sh  | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c7b2818f20..7ee157532d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1460,8 +1460,8 @@ static const char *push_to_checkout(unsigned char *hash,\n \n \topt.invoked_hook = invoked_hook;\n \n-\tstrvec_pushf(env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n \tstrvec_pushv(&opt.env, env->v);\n+\tstrvec_pushf(&opt.env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n \tstrvec_push(&opt.args, hash_to_hex(hash));\n \tif (run_hooks_opt(the_repository, push_to_checkout_hook, &opt))\n \t\treturn \"push-to-checkout hook declined\";\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 117cfa051f..f51fb11a6d 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1791,6 +1791,17 @@ test_expect_success 'updateInstead with push-to-checkout hook' '\n \t)\n '\n \n+test_expect_success 'denyCurrentBranch and core.worktree' '\n+\ttest_when_finished \"rm -fr cloned cloned.git\" &&\n+\tgit clone --separate-git-dir cloned.git . cloned &&\n+\tgit --git-dir cloned.git config receive.denyCurrentBranch updateInstead &&\n+\tgit --git-dir cloned.git config core.worktree \"$PWD/cloned\" &&\n+        test_commit raspberry &&\n+\tgit push cloned.git HEAD:main &&\n+\ttest_path_exists cloned/raspberry.t &&\n+\ttest_must_fail git push --delete cloned.git main\n+'\n+\n test_expect_success 'denyCurrentBranch and worktrees' '\n \ttest_when_finished \"rm -fr cloned && git worktree remove --force new-wt\" &&\n \tgit worktree add new-wt &&\n\nbase-commit: aec3f587505a472db67e9462d0702e7d463a449d\n-- \n2.53.0\n\n"},{"id":"543929","messageId":"60a0ec9b-0263-42f5-83b1-c55275c7772a@app.fastmail.com","threadId":"65676","inReplyTo":"20260522154418.5883-1-hi@alyssa.is","subject":"Re: [PATCH] receive-pack: fix updateInstead with core.worktree","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2026-05-22T16:21:07Z","receivedAt":"2026-05-22T16:21:38Z","isPatch":true,"body":"On Fri, May 22, 2026, at 17:44, Alyssa Ross wrote:\n> This used to work, but when push_to_checkout() started being called\n> before push_to_deploy(), push_to_checkout()'s side effect of adding\n> GIT_WORK_TREE to the same environment that would be used by\n> push_to_deploy() wasn't taken into account.  Fix by only mutating the\n> environment for push_to_commit(), rather than the shared environment.\n>\n> Fixes: a8cc594333 (\"hooks: fix an obscure TOCTOU \"did we just run a hook?\" race\")\n\nThis project doesn’t use `Fixes` trailers.[1] Mentions of commits go in\nthe commit message body (outside the trailers) using `git log -1\n--format-reference <cmt>`.\n\nThe Linux project has uses for this structured information since there\nis a lot of backporting of bugfixes. But I haven’t heard of a need for\nthat in this project.\n\n🔗 1: https://lore.kernel.org/git/72839071-153f-4306-a705-3be0dc203109@app.fastmail.com/\n\n> Signed-off-by: Alyssa Ross <hi@alyssa.is>\n> ---\n>[snip]\n"},{"id":"544018","messageId":"xmqqfr3ggx1h.fsf@gitster.g","threadId":"65676","inReplyTo":"20260522154418.5883-1-hi@alyssa.is","subject":"Re: [PATCH] receive-pack: fix updateInstead with core.worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-25T00:20:10Z","receivedAt":"2026-05-25T00:20:13Z","isPatch":true,"body":"Alyssa Ross <hi@alyssa.is> writes:\n\n> This used to work, but when push_to_checkout() started being called\n> before push_to_deploy(), ...\n\nWe tend to try describing where things started breaking a bit more\nprecisely.  The above seems to say that you know that in the past\npush_to__checkout() was not called before push_to_deploy(), and it\nno longer is the case these days?  Can you spell out in what commit\nthat change happened (refer to the commit using the \"git show -s\n--pretty=reference\" format)?  I.e.\n\n\t... but when X started doing Y at a8cc5943 (hooks: fix an\n\tobscure TOCTOU \"did we just run a hook?\" race, 2022-03-07),\n\t<<this bad thing>> started to happen.\n\nIt isn't really we are exercising \"checkout\" and \"deploy\" both at\nthe same time, but an old commit started to always call _checkout\nonly to see if that actually invokes the hook, and if it didn't,\nthen call _deploy.  The intent still is to use either one of these,\nbut as you exactly identified what is wrong in the current code, the\ncall to _checkout that is only done to probe if it is used at all\nstarted to contaminate the environment with that commit.\n\nSo this change ...\n\n> -\tstrvec_pushf(env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n>  \tstrvec_pushv(&opt.env, env->v);\n> +\tstrvec_pushf(&opt.env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n>  \tstrvec_push(&opt.args, hash_to_hex(hash));\n\n... looks like absolutely the right thing to do.  And ...\n\n>  \tif (run_hooks_opt(the_repository, push_to_checkout_hook, &opt))\n>  \t\treturn \"push-to-checkout hook declined\";\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 117cfa051f..f51fb11a6d 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1791,6 +1791,17 @@ test_expect_success 'updateInstead with push-to-checkout hook' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'denyCurrentBranch and core.worktree' '\n> +\ttest_when_finished \"rm -fr cloned cloned.git\" &&\n> +\tgit clone --separate-git-dir cloned.git . cloned &&\n> +\tgit --git-dir cloned.git config receive.denyCurrentBranch updateInstead &&\n> +\tgit --git-dir cloned.git config core.worktree \"$PWD/cloned\" &&\n> +        test_commit raspberry &&\n> +\tgit push cloned.git HEAD:main &&\n> +\ttest_path_exists cloned/raspberry.t &&\n> +\ttest_must_fail git push --delete cloned.git main\n> +'\n\n... a test that protects similar breakage in the future is also\nexcellent.\n\n>  test_expect_success 'denyCurrentBranch and worktrees' '\n>  \ttest_when_finished \"rm -fr cloned && git worktree remove --force new-wt\" &&\n>  \tgit worktree add new-wt &&\n>\n> base-commit: aec3f587505a472db67e9462d0702e7d463a449d\n"},{"id":"544068","messageId":"20260525162311.66240-2-hi@alyssa.is","threadId":"65676","inReplyTo":"20260522154418.5883-1-hi@alyssa.is","subject":"[PATCH v2] receive-pack: fix updateInstead with core.worktree","fromName":"Alyssa Ross","fromEmail":"hi@alyssa.is","sentAt":"2026-05-25T16:23:12Z","receivedAt":"2026-05-25T16:23:29Z","isPatch":true,"body":"Previously, only one of push_to_checkout() or push_to_deploy() was\ncalled.  In a8cc594333 (hooks: fix an obscure TOCTOU \"did we just run a\nhook?\" race, 2022-03-07), this was changed to always call\npush_to_checkout(), and then to call push_to_deploy() if\npush_to_checkout() didn't run anything.  This change didn't take into\naccount that push_to_checkout() had a side effect of modifying env, and\nthat modified env broke updating the worktree in push_to_deploy() if\ncore.worktree was configured.  To fix this, only mutate the environment\nused inside push_to_commit(), rather than the environment that might\nlater be passed to push_to_deploy().\n\nSigned-off-by: Alyssa Ross <hi@alyssa.is>\n---\nv2: reword commit message in response to feedback\n\n builtin/receive-pack.c |  2 +-\n t/t5516-fetch-push.sh  | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c7b2818f20..7ee157532d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1460,8 +1460,8 @@ static const char *push_to_checkout(unsigned char *hash,\n \n \topt.invoked_hook = invoked_hook;\n \n-\tstrvec_pushf(env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n \tstrvec_pushv(&opt.env, env->v);\n+\tstrvec_pushf(&opt.env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n \tstrvec_push(&opt.args, hash_to_hex(hash));\n \tif (run_hooks_opt(the_repository, push_to_checkout_hook, &opt))\n \t\treturn \"push-to-checkout hook declined\";\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 117cfa051f..db6cc18673 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1791,6 +1791,17 @@ test_expect_success 'updateInstead with push-to-checkout hook' '\n \t)\n '\n \n+test_expect_success 'denyCurrentBranch and core.worktree' '\n+\ttest_when_finished \"rm -fr cloned cloned.git\" &&\n+\tgit clone --separate-git-dir cloned.git . cloned &&\n+\tgit --git-dir cloned.git config receive.denyCurrentBranch updateInstead &&\n+\tgit --git-dir cloned.git config core.worktree \"$PWD/cloned\" &&\n+\ttest_commit raspberry &&\n+\tgit push cloned.git HEAD:main &&\n+\ttest_path_exists cloned/raspberry.t &&\n+\ttest_must_fail git push --delete cloned.git main\n+'\n+\n test_expect_success 'denyCurrentBranch and worktrees' '\n \ttest_when_finished \"rm -fr cloned && git worktree remove --force new-wt\" &&\n \tgit worktree add new-wt &&\n\nbase-commit: aec3f587505a472db67e9462d0702e7d463a449d\n-- \n2.53.0\n\n"},{"id":"544083","messageId":"xmqqv7cbcd7w.fsf@gitster.g","threadId":"65676","inReplyTo":"20260525162311.66240-2-hi@alyssa.is","subject":"Re: [PATCH v2] receive-pack: fix updateInstead with core.worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-25T22:54:11Z","receivedAt":"2026-05-25T22:54:14Z","isPatch":true,"body":"Alyssa Ross <hi@alyssa.is> writes:\n\n> Previously, only one of push_to_checkout() or push_to_deploy() was\n> called.  In a8cc594333 (hooks: fix an obscure TOCTOU \"did we just run a\n> hook?\" race, 2022-03-07), this was changed to always call\n> push_to_checkout(), and then to call push_to_deploy() if\n> push_to_checkout() didn't run anything.  This change didn't take into\n> account that push_to_checkout() had a side effect of modifying env, and\n> that modified env broke updating the worktree in push_to_deploy() if\n> core.worktree was configured.  To fix this, only mutate the environment\n> used inside push_to_commit(), rather than the environment that might\n> later be passed to push_to_deploy().\n>\n> Signed-off-by: Alyssa Ross <hi@alyssa.is>\n> ---\n> v2: reword commit message in response to feedback\n\nYou also fixed an incorrectly indentated line in the new test, which\nis very much appreciated.\n\nWill queue.  Thanks.\n\n>\n>  builtin/receive-pack.c |  2 +-\n>  t/t5516-fetch-push.sh  | 11 +++++++++++\n>  2 files changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index c7b2818f20..7ee157532d 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1460,8 +1460,8 @@ static const char *push_to_checkout(unsigned char *hash,\n>  \n>  \topt.invoked_hook = invoked_hook;\n>  \n> -\tstrvec_pushf(env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n>  \tstrvec_pushv(&opt.env, env->v);\n> +\tstrvec_pushf(&opt.env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n>  \tstrvec_push(&opt.args, hash_to_hex(hash));\n>  \tif (run_hooks_opt(the_repository, push_to_checkout_hook, &opt))\n>  \t\treturn \"push-to-checkout hook declined\";\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 117cfa051f..db6cc18673 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1791,6 +1791,17 @@ test_expect_success 'updateInstead with push-to-checkout hook' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'denyCurrentBranch and core.worktree' '\n> +\ttest_when_finished \"rm -fr cloned cloned.git\" &&\n> +\tgit clone --separate-git-dir cloned.git . cloned &&\n> +\tgit --git-dir cloned.git config receive.denyCurrentBranch updateInstead &&\n> +\tgit --git-dir cloned.git config core.worktree \"$PWD/cloned\" &&\n> +\ttest_commit raspberry &&\n> +\tgit push cloned.git HEAD:main &&\n> +\ttest_path_exists cloned/raspberry.t &&\n> +\ttest_must_fail git push --delete cloned.git main\n> +'\n> +\n>  test_expect_success 'denyCurrentBranch and worktrees' '\n>  \ttest_when_finished \"rm -fr cloned && git worktree remove --force new-wt\" &&\n>  \tgit worktree add new-wt &&\n>\n> base-commit: aec3f587505a472db67e9462d0702e7d463a449d\n"}]}