{"thread":{"id":"56859","subject":"[PATCH v3 2/2] receive-pack: Protect current branch for bare repository worktree","startedAt":"2021-11-08T20:16:20Z","lastAt":"2021-11-11T00:11:40Z","messageCount":34,"participants":["Anders Kaseorg","Junio C Hamano","Johannes Schindelin","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"440684","messageId":"alpine.DEB.2.21.999.2111081515380.100671@scrubbing-bubbles.mit.edu","threadId":"56859","inReplyTo":null,"subject":"[PATCH v3 2/2] receive-pack: Protect current branch for bare repository worktree","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-08T20:16:12Z","receivedAt":"2021-11-08T20:16:20Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"A bare repository won’t have a working tree at .., but it may still have\nseparate working trees created with git worktree. We should protect the\ncurrent branch of such working trees from being updated or deleted,\naccording to receive.denyCurrentBranch.\n\nSigned-off-by: Anders Kaseorg \n---\n builtin/receive-pack.c | 10 +++++-----\n t/t5516-fetch-push.sh | 11 ++++++++---\n 2 files changed, 13 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 49b846d960..5efc9bc9fa 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1456,11 +1456,11 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n \t\twork_tree = worktree->path;\n \telse if (git_work_tree_cfg)\n \t\twork_tree = git_work_tree_cfg;\n-\telse\n-\t\twork_tree = \"..\";\n-\n-\tif (is_bare_repository())\n+\telse if (is_bare_repository())\n \t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n+\telse\n+\t\twork_tree = \"..\";\n+\n \tif (worktree)\n \t\tgit_dir = get_worktree_git_dir(worktree);\n \tif (!git_dir)\n@@ -1486,7 +1486,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tstruct object_id *old_oid = &cmd->old_oid;\n \tstruct object_id *new_oid = &cmd->new_oid;\n \tint do_update_worktree = 0;\n-\tconst struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref(\"HEAD\", name);\n+\tconst struct worktree *worktree = find_shared_symref(\"HEAD\", name);\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 4ef4ecbe71..52a4686afe 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1763,20 +1763,25 @@ test_expect_success 'updateInstead with push-to-checkout hook' '\n \n test_expect_success 'denyCurrentBranch and worktrees' '\n \tgit worktree add new-wt &&\n+\tgit clone --bare . bare.git &&\n+\tgit -C bare.git worktree add bare-wt &&\n \tgit clone . cloned &&\n \ttest_commit -C cloned first &&\n \ttest_config receive.denyCurrentBranch refuse &&\n \ttest_must_fail git -C cloned push origin HEAD:new-wt &&\n+\ttest_config -C bare.git receive.denyCurrentBranch refuse &&\n+\ttest_must_fail git -C cloned push ../bare.git HEAD:bare-wt &&\n \ttest_config receive.denyCurrentBranch updateInstead &&\n \tgit -C cloned push origin HEAD:new-wt &&\n-\ttest_must_fail git -C cloned push --delete origin new-wt\n+\ttest_must_fail git -C cloned push --delete origin new-wt &&\n+\ttest_config -C bare.git receive.denyCurrentBranch updateInstead &&\n+\tgit -C cloned push ../bare.git HEAD:bare-wt &&\n+\ttest_must_fail git -C cloned push --delete ../bare.git bare-wt\n '\n \n test_expect_success 'refuse fetch to current branch of worktree' '\n \ttest_commit -C cloned second &&\n \ttest_must_fail git fetch cloned HEAD:new-wt &&\n-\tgit clone --bare . bare.git &&\n-\tgit -C bare.git worktree add bare-wt &&\n \ttest_must_fail git -C bare.git fetch ../cloned HEAD:bare-wt &&\n \tgit fetch -u cloned HEAD:new-wt &&\n \tgit -C bare.git fetch -u ../cloned HEAD:bare-wt\n-- \n2.33.1\n"},{"id":"440704","messageId":"xmqqzgqe448a.fsf@gitster.g","threadId":"56859","inReplyTo":"alpine.DEB.2.21.999.2111081515380.100671@scrubbing-bubbles.mit.edu","subject":"Re: [PATCH v3 2/2] receive-pack: Protect current branch for bare repository worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-08T23:28:21Z","receivedAt":"2021-11-08T23:28:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Kaseorg writes:\n\n> A bare repository won’t have a working tree at .., but it may still have\n\nMy reading hiccupped after \"at\"; perhaps enclose the double-dot\ninside a pair of double quotes would make it easier to follow.\n\n> separate working trees created with git worktree. We should protect the\n> current branch of such working trees from being updated or deleted,\n> according to receive.denyCurrentBranch.\n\nGood point. I was wondering about that exact thing while reading\nthe fetch side.\n\n> @@ -1456,11 +1456,11 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n> \t\twork_tree = worktree->path;\n> \telse if (git_work_tree_cfg)\n> \t\twork_tree = git_work_tree_cfg;\n\nNot a fault of this patch at all, but I am not sure if this existing\nbit of code is correct. Everything else in this function works by\nassuming that the worktree that comes from the caller was checked\nwith find_shared_symref(\"HEAD\", name) to ensure that, if not NULL,\nit has the branch checked out and updating to the new commit given\nas the other parameter makes sense.\n\nBut this \"fall back to configured worktree\" is taken when the gave\nus NULL worktree or worktree without the .path member (i.e. no\ncheckout), and it must have come from a NULL return from the call to\nfind_shared_symref(). IOW, the function said \"no worktree\nassociated with the repository checks out that branch being\nupdated.\" I doubt it is a bug to update the working tree of the\nrepository with the commit pushed to some branch that is *not* HEAD,\nonly because core.worktree was set to point at an explicit location.\n\n> -\telse\n> -\t\twork_tree = \"..\";\n> -\n> -\tif (is_bare_repository())\n> +\telse if (is_bare_repository())\n> \t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n> +\telse\n> +\t\twork_tree = \"..\";\n> +\n> \tif (worktree)\n> \t\tgit_dir = get_worktree_git_dir(worktree);\n> \tif (!git_dir)\n> @@ -1486,7 +1486,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n> \tstruct object_id *old_oid = &cmd->old_oid;\n> \tstruct object_id *new_oid = &cmd->new_oid;\n> \tint do_update_worktree = 0;\n> -\tconst struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref(\"HEAD\", name);\n> +\tconst struct worktree *worktree = find_shared_symref(\"HEAD\", name);\n\nOK. This change does make sense. The worktree we happen to be in\nmight be bare, but there can be other worktrees that have the branch\nin question checked out, so find_shared_symref() must be called\nregardless.\n\nThe callsite of the other function this patch modifies is in this\nupdate() function much later, and I think it should be updated to\nuse the variable \"worktree\" instead of calling find_shared_symref()\nagain with the same parameters.\n\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 4ef4ecbe71..52a4686afe 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1763,20 +1763,25 @@ test_expect_success 'updateInstead with push-to-checkout hook' '\n> \n> test_expect_success 'denyCurrentBranch and worktrees' '\n> \tgit worktree add new-wt &&\n> +\tgit clone --bare . bare.git &&\n> +\tgit -C bare.git worktree add bare-wt &&\n\nWe create a bare.git bare repository with a bare-wt worktree that\nhas a working tree. bare-wt branch must be protected now.\n\n> \tgit clone . cloned &&\n> \ttest_commit -C cloned first &&\n> \ttest_config receive.denyCurrentBranch refuse &&\n> \ttest_must_fail git -C cloned push origin HEAD:new-wt &&\n> +\ttest_config -C bare.git receive.denyCurrentBranch refuse &&\n> +\ttest_must_fail git -C cloned push ../bare.git HEAD:bare-wt &&\n\nAnd pushing to that branch is refused (which is the default without\nthe receive.denyCurrentBranch configuration, too). Good.\n\n> \ttest_config receive.denyCurrentBranch updateInstead &&\n> \tgit -C cloned push origin HEAD:new-wt &&\n> -\ttest_must_fail git -C cloned push --delete origin new-wt\n> +\ttest_must_fail git -C cloned push --delete origin new-wt &&\n\n> +\ttest_config -C bare.git receive.denyCurrentBranch updateInstead &&\n> +\tgit -C cloned push ../bare.git HEAD:bare-wt &&\n\nAnd when set to update, it would update the working tree as expected.\n\nWe are not checking if we correctly update the working tree; we are\nonly seeing \"git push\" succeeds. Which might want to be tightened\nup.\n\n> +\ttest_must_fail git -C cloned push --delete ../bare.git bare-wt\n\nAnd even with updateInstead, we do not let the branch to be deleted.\n\n> '\n> \n> test_expect_success 'refuse fetch to current branch of worktree' '\n> \ttest_commit -C cloned second &&\n> \ttest_must_fail git fetch cloned HEAD:new-wt &&\n> -\tgit clone --bare . bare.git &&\n> -\tgit -C bare.git worktree add bare-wt &&\n\nIt is a bit sad that these two tests are so inter-dependent.\nDepending on an earlier failure of other tests, this may fail in an\nunexpected way.\n\n> \ttest_must_fail git -C bare.git fetch ../cloned HEAD:bare-wt &&\n> \tgit fetch -u cloned HEAD:new-wt &&\n> \tgit -C bare.git fetch -u ../cloned HEAD:bare-wt\n\nI think the core of the patch looks well thought out. If the tests\nare cleaned up a bit more, it would be perfect.\n\nThanks.\n"},{"id":"440711","messageId":"xmqqpmra40p6.fsf@gitster.g","threadId":"56859","inReplyTo":"xmqqzgqe448a.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] receive-pack: Protect current branch for bare repository worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-09T00:44:37Z","receivedAt":"2021-11-09T00:44:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> @@ -1456,11 +1456,11 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n>> \t\twork_tree = worktree->path;\n>> \telse if (git_work_tree_cfg)\n>> \t\twork_tree = git_work_tree_cfg;\n>\n> Not a fault of this patch at all, but I am not sure if this existing\n> bit of code is correct. Everything else in this function works by\n> assuming that the worktree that comes from the caller was checked\n> with find_shared_symref(\"HEAD\", name) to ensure that, if not NULL,\n> it has the branch checked out and updating to the new commit given\n> as the other parameter makes sense.\n>\n> But this \"fall back to configured worktree\" is taken when the gave\n> us NULL worktree or worktree without the .path member (i.e. no\n> checkout), and it must have come from a NULL return from the call to\n> find_shared_symref(). IOW, the function said \"no worktree\n> associated with the repository checks out that branch being\n> updated.\" I doubt it is a bug to update the working tree of the\n> repository with the commit pushed to some branch that is *not* HEAD,\n> only because core.worktree was set to point at an explicit location.\n\nNot \"I doubt\", but I suspect it is a bug. Sorry.\n\nBut in practice, especially with the new code structure, we'd never\nflip do_update_worktree on unless find_shared_symref() says that the\nref we are updating in the function is what is checked out, which\nmeans worktree is always non-NULL when we call update_worktree().\n\nSo, unless there is some situation where worktree->path is NULL for\na worktree with a checkout, the \"else if\" above is a dead code, I\nthink.\n\nSimilarly, I suspect that is_bare_repository() call the patch moved\ninto the if/else if/ chain is even reachable with the updated\ncaller. find_shared_symref() is always called, and unless it gives\na non-NULL worktree, do_update_worktree never becomes true.\n\nAnyway, enough bug finding in the existing code. I think the\nupdate-instead was Dscho's invention and when the codepath was\nupdated to be worktree ready, Dscho helped Hariom to do so, so\nI'll CC Dscho to see if he has input.\n\nThanks.\n"},{"id":"440712","messageId":"a25d105a-875b-fa6a-771a-37936779f067@mit.edu","threadId":"56859","inReplyTo":"xmqqzgqe448a.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] receive-pack: Protect current branch for bare repository worktree","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T01:10:43Z","receivedAt":"2021-11-09T01:15:24Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On 11/8/21 15:28, Junio C Hamano wrote:\n> My reading hiccupped after \"at\"; perhaps enclose the double-dot\n> inside a pair of double quotes would make it easier to follow.\n\nWill update.\n\n>> @@ -1456,11 +1456,11 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n>> \t\twork_tree = worktree->path;\n>> \telse if (git_work_tree_cfg)\n>> \t\twork_tree = git_work_tree_cfg;\n> \n> Not a fault of this patch at all, but I am not sure if this existing\n> bit of code is correct.\nPerhaps this code is unreachable?\n\nThe worktree argument of update_worktree() should never be NULL, because \nwe’d only have set do_update_worktree = 1 if find_shared_symref() \nreturned non-NULL. And it looks to me like worktree->path should always \nbe initialized to non-NULL, in either get_main_worktree() or \nget_linked_worktree()? I haven’t read enough of the code to be totally \nconfident in this though.\n\n> The callsite of the other function this patch modifies is in this\n> update() function much later, and I think it should be updated to\n> use the variable \"worktree\" instead of calling find_shared_symref()\n> again with the same parameters.\n\nWill update.\n\n> We are not checking if we correctly update the working tree; we are\n> only seeing \"git push\" succeeds. Which might want to be tightened\n> up.\n\nReasonable. This is also the case with the existing test.\n\n> It is a bit sad that these two tests are so inter-dependent.\n> Depending on an earlier failure of other tests, this may fail in an\n> unexpected way.\n\nYeah, I guess I wasn’t sure how much interdependence was allowed or \nexpected. For example, the existing test already fails when run by \nitself (./t5516-fetch-push.sh --run=103) because the repository starts \nout empty. I’ll see what I can do, perhaps making use of the \ntest_when_finished helper.\n\nAnders\n"},{"id":"440724","messageId":"20211109030028.2196416-1-andersk@mit.edu","threadId":"56859","inReplyTo":"a25d105a-875b-fa6a-771a-37936779f067@mit.edu","subject":"[PATCH v4 1/4] fetch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T03:00:25Z","receivedAt":"2021-11-09T03:01:29Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"Refuse to fetch into the currently checked out branch of any working\ntree, not just the current one.\n\nFixes this previously reported bug:\n\nhttps://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de\n\nAs a side effect of using find_shared_symref, we’ll also refuse the\nfetch when we’re on a detached HEAD because we’re rebasing or bisecting\non the branch in question. This seems like a sensible change.\n\nSigned-off-by: Anders Kaseorg \n---\n builtin/fetch.c | 28 ++++++++++++++--------------\n t/t5516-fetch-push.sh | 18 ++++++++++++++++++\n 2 files changed, 32 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f7abbc31ff..0ef2170ef9 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -28,6 +28,7 @@\n #include \"promisor-remote.h\"\n #include \"commit-graph.h\"\n #include \"shallow.h\"\n+#include \"worktree.h\"\n \n #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n \n@@ -854,7 +855,7 @@ static int update_local_ref(struct ref *ref,\n \t\t\t int summary_width)\n {\n \tstruct commit *current = NULL, *updated;\n-\tstruct branch *current_branch = branch_get(NULL);\n+\tconst struct worktree *wt;\n \tconst char *pretty_ref = prettify_refname(ref->name);\n \tint fast_forward = 0;\n \n@@ -868,16 +869,18 @@ static int update_local_ref(struct ref *ref,\n \t\treturn 0;\n \t}\n \n-\tif (current_branch &&\n-\t !strcmp(ref->name, current_branch->name) &&\n-\t !(update_head_ok || is_bare_repository()) &&\n+\tif (!update_head_ok &&\n+\t (wt = find_shared_symref(\"HEAD\", ref->name)) &&\n+\t !wt->is_bare &&\n \t !is_null_oid(&ref->old_oid)) {\n \t\t/*\n \t\t * If this is the head, and it's not okay to update\n \t\t * the head, and the old value of the head isn't empty...\n \t\t */\n \t\tformat_display(display, '!', _(\"[rejected]\"),\n-\t\t\t _(\"can't fetch in current branch\"),\n+\t\t\t wt->is_current ?\n+\t\t\t _(\"can't fetch in current branch\") :\n+\t\t\t _(\"checked out in another worktree\"),\n \t\t\t remote, pretty_ref, summary_width);\n \t\treturn 1;\n \t}\n@@ -1387,16 +1390,13 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,\n \n static void check_not_current_branch(struct ref *ref_map)\n {\n-\tstruct branch *current_branch = branch_get(NULL);\n-\n-\tif (is_bare_repository() || !current_branch)\n-\t\treturn;\n-\n+\tconst struct worktree *wt;\n \tfor (; ref_map; ref_map = ref_map->next)\n-\t\tif (ref_map->peer_ref && !strcmp(current_branch->refname,\n-\t\t\t\t\tref_map->peer_ref->name))\n-\t\t\tdie(_(\"Refusing to fetch into current branch %s \"\n-\t\t\t \"of non-bare repository\"), current_branch->refname);\n+\t\tif (ref_map->peer_ref &&\n+\t\t (wt = find_shared_symref(\"HEAD\", ref_map->peer_ref->name)))\n+\t\t\tdie(_(\"Refusing to fetch into branch '%s' \"\n+\t\t\t \"checked out at '%s'\"),\n+\t\t\t ref_map->peer_ref->name, wt->path);\n }\n \n static int truncate_fetch_head(void)\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 8212ca56dc..2c2d6fa6e7 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1771,4 +1771,22 @@ test_expect_success 'denyCurrentBranch and worktrees' '\n \tgit -C cloned push origin HEAD:new-wt &&\n \ttest_must_fail git -C cloned push --delete origin new-wt\n '\n+\n+test_expect_success 'refuse fetch to current branch of worktree' '\n+\ttest_when_finished \"git worktree remove --force wt\" &&\n+\tgit worktree add wt &&\n+\ttest_commit apple &&\n+\ttest_must_fail git fetch . HEAD:wt &&\n+\tgit fetch -u . HEAD:wt\n+'\n+\n+test_expect_success 'refuse fetch to current branch of bare repository worktree' '\n+\ttest_when_finished \"rm -fr bare.git\" &&\n+\tgit clone --bare . bare.git &&\n+\tgit -C bare.git worktree add wt &&\n+\ttest_commit banana &&\n+\ttest_must_fail git -C bare.git fetch .. HEAD:wt &&\n+\tgit -C bare.git fetch -u .. HEAD:wt\n+'\n+\n test_done\n-- \n2.33.1\n\n"},{"id":"440725","messageId":"20211109030028.2196416-2-andersk@mit.edu","threadId":"56859","inReplyTo":"20211109030028.2196416-1-andersk@mit.edu","subject":"[PATCH v4 2/4] receive-pack: Clean dead code from update_worktree()","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T03:00:26Z","receivedAt":"2021-11-09T03:01:39Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"update_worktree() can only be called with a non-NULL worktree parameter,\nbecause that’s the only case where we set do_update_worktree = 1.\nworktree->path is always initialized to non-NULL.\n\nSigned-off-by: Anders Kaseorg \n---\n builtin/receive-pack.c | 20 +++++---------------\n 1 file changed, 5 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 49b846d960..cf575280fc 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1449,29 +1449,19 @@ static const char *push_to_checkout(unsigned char *hash,\n \n static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)\n {\n-\tconst char *retval, *work_tree, *git_dir = NULL;\n+\tconst char *retval, *git_dir;\n \tstruct strvec env = STRVEC_INIT;\n \n-\tif (worktree && worktree->path)\n-\t\twork_tree = worktree->path;\n-\telse if (git_work_tree_cfg)\n-\t\twork_tree = git_work_tree_cfg;\n-\telse\n-\t\twork_tree = \"..\";\n-\n \tif (is_bare_repository())\n \t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n-\tif (worktree)\n-\t\tgit_dir = get_worktree_git_dir(worktree);\n-\tif (!git_dir)\n-\t\tgit_dir = get_git_dir();\n+\tgit_dir = get_worktree_git_dir(worktree);\n \n \tstrvec_pushf(&env, \"GIT_DIR=%s\", absolute_path(git_dir));\n \n \tif (!hook_exists(push_to_checkout_hook))\n-\t\tretval = push_to_deploy(sha1, &env, work_tree);\n+\t\tretval = push_to_deploy(sha1, &env, worktree->path);\n \telse\n-\t\tretval = push_to_checkout(sha1, &env, work_tree);\n+\t\tretval = push_to_checkout(sha1, &env, worktree->path);\n \n \tstrvec_clear(&env);\n \treturn retval;\n@@ -1579,7 +1569,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t}\n \n \tif (do_update_worktree) {\n-\t\tret = update_worktree(new_oid->hash, find_shared_symref(\"HEAD\", name));\n+\t\tret = update_worktree(new_oid->hash, worktree);\n \t\tif (ret)\n \t\t\treturn ret;\n \t}\n-- \n2.33.1\n\n"},{"id":"440726","messageId":"20211109030028.2196416-3-andersk@mit.edu","threadId":"56859","inReplyTo":"20211109030028.2196416-1-andersk@mit.edu","subject":"[PATCH v4 3/4] receive-pack: Protect current branch for bare repository worktree","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T03:00:27Z","receivedAt":"2021-11-09T03:01:51Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"A bare repository won’t have a working tree at \"..\", but it may still\nhave separate working trees created with git worktree. We should protect\nthe current branch of such working trees from being updated or deleted,\naccording to receive.denyCurrentBranch.\n\nSigned-off-by: Anders Kaseorg \n---\n builtin/receive-pack.c | 4 +---\n t/t5516-fetch-push.sh | 12 ++++++++++++\n 2 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex cf575280fc..5a3c6d8423 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1452,8 +1452,6 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n \tconst char *retval, *git_dir;\n \tstruct strvec env = STRVEC_INIT;\n \n-\tif (is_bare_repository())\n-\t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n \tgit_dir = get_worktree_git_dir(worktree);\n \n \tstrvec_pushf(&env, \"GIT_DIR=%s\", absolute_path(git_dir));\n@@ -1476,7 +1474,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tstruct object_id *old_oid = &cmd->old_oid;\n \tstruct object_id *new_oid = &cmd->new_oid;\n \tint do_update_worktree = 0;\n-\tconst struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref(\"HEAD\", name);\n+\tconst struct worktree *worktree = find_shared_symref(\"HEAD\", name);\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 2c2d6fa6e7..06cd34b0db 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1772,6 +1772,18 @@ test_expect_success 'denyCurrentBranch and worktrees' '\n \ttest_must_fail git -C cloned push --delete origin new-wt\n '\n \n+test_expect_success 'denyCurrentBranch and bare repository worktrees' '\n+\ttest_when_finished \"rm -fr bare.git\" &&\n+\tgit clone --bare . bare.git &&\n+\tgit -C bare.git worktree add wt &&\n+\ttest_commit grape &&\n+\ttest_config -C bare.git receive.denyCurrentBranch refuse &&\n+\ttest_must_fail git push bare.git HEAD:wt &&\n+\ttest_config -C bare.git receive.denyCurrentBranch updateInstead &&\n+\tgit push bare.git HEAD:wt &&\n+\ttest_must_fail git push --delete bare.git wt\n+'\n+\n test_expect_success 'refuse fetch to current branch of worktree' '\n \ttest_when_finished \"git worktree remove --force wt\" &&\n \tgit worktree add wt &&\n-- \n2.33.1\n\n"},{"id":"440727","messageId":"20211109030028.2196416-4-andersk@mit.edu","threadId":"56859","inReplyTo":"20211109030028.2196416-1-andersk@mit.edu","subject":"[PATCH v4 4/4] branch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T03:00:28Z","receivedAt":"2021-11-09T03:01:52Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"Refuse to force-move a branch over the currently checked out branch of\nany working tree, not just the current one.\n\nSigned-off-by: Anders Kaseorg \n---\n branch.c | 10 ++++++----\n t/t3200-branch.sh | 7 +++++++\n 2 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 07a46430b3..581f0c02c2 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -199,7 +199,7 @@ int validate_branchname(const char *name, struct strbuf *ref)\n */\n int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n {\n-\tconst char *head;\n+\tconst struct worktree *wt;\n \n \tif (!validate_branchname(name, ref))\n \t\treturn 0;\n@@ -208,9 +208,11 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n \t\tdie(_(\"A branch named '%s' already exists.\"),\n \t\t ref->buf + strlen(\"refs/heads/\"));\n \n-\thead = resolve_ref_unsafe(\"HEAD\", 0, NULL, NULL);\n-\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n-\t\tdie(_(\"Cannot force update the current branch.\"));\n+\twt = find_shared_symref(\"HEAD\", ref->buf);\n+\tif (wt && !wt->is_bare)\n+\t\tdie(_(\"Cannot force update the branch '%s'\"\n+\t\t \"checked out at '%s'.\"),\n+\t\t ref->buf + strlen(\"refs/heads/\"), wt->path);\n \n \treturn 1;\n }\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex e575ffb4ff..4c868bf971 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -168,6 +168,13 @@ test_expect_success 'git branch -M foo bar should fail when bar is checked out'\n \ttest_must_fail git branch -M bar foo\n '\n \n+test_expect_success 'git branch -M foo bar should fail when bar is checked out in worktree' '\n+\tgit branch -f bar &&\n+\ttest_when_finished \"git worktree remove wt && git branch -D wt\" &&\n+\tgit worktree add wt &&\n+\ttest_must_fail git branch -M bar wt\n+'\n+\n test_expect_success 'git branch -M baz bam should succeed when baz is checked out' '\n \tgit checkout -b baz &&\n \tgit branch bam &&\n-- \n2.33.1\n\n"},{"id":"440741","messageId":"nycvar.QRO.7.76.6.2111091635531.54@tvgsbejvaqbjf.bet","threadId":"56859","inReplyTo":"xmqqzgqe448a.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] receive-pack: Protect current branch for bare repository worktree","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-09T15:37:08Z","receivedAt":"2021-11-09T15:37:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 8 Nov 2021, Junio C Hamano wrote:\n\n> Anders Kaseorg writes:\n>\n> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> > index 4ef4ecbe71..52a4686afe 100755\n> > --- a/t/t5516-fetch-push.sh\n> > +++ b/t/t5516-fetch-push.sh\n> > @@ -1763,20 +1763,25 @@ test_expect_success 'updateInstead with push-to-checkout hook' '\n> >\n> > test_expect_success 'denyCurrentBranch and worktrees' '\n> > \tgit worktree add new-wt &&\n> > +\tgit clone --bare . bare.git &&\n> > +\tgit -C bare.git worktree add bare-wt &&\n>\n> We create a bare.git bare repository with a bare-wt worktree that\n> has a working tree. bare-wt branch must be protected now.\n>\n> [...]\n>\n> > '\n> >\n> > test_expect_success 'refuse fetch to current branch of worktree' '\n> > \ttest_commit -C cloned second &&\n> > \ttest_must_fail git fetch cloned HEAD:new-wt &&\n> > -\tgit clone --bare . bare.git &&\n> > -\tgit -C bare.git worktree add bare-wt &&\n>\n> It is a bit sad that these two tests are so inter-dependent.\n> Depending on an earlier failure of other tests, this may fail in an\n> unexpected way.\n\nIndeed. Maybe we should keep the latter as-is, and add this to the former?\n\n\ttest_when_finished \"rm -rf bare.git bare-wt\" &&\n\nCiao,\nDscho\n"},{"id":"440743","messageId":"nycvar.QRO.7.76.6.2111091638110.54@tvgsbejvaqbjf.bet","threadId":"56859","inReplyTo":"xmqqpmra40p6.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] receive-pack: Protect current branch for bare repository worktree","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-09T16:04:58Z","receivedAt":"2021-11-09T16:05:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 8 Nov 2021, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n> >> @@ -1456,11 +1456,11 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n> >> \t\twork_tree = worktree->path;\n> >> \telse if (git_work_tree_cfg)\n> >> \t\twork_tree = git_work_tree_cfg;\n> >\n> > Not a fault of this patch at all, but I am not sure if this existing\n> > bit of code is correct. Everything else in this function works by\n> > assuming that the worktree that comes from the caller was checked\n> > with find_shared_symref(\"HEAD\", name) to ensure that, if not NULL,\n> > it has the branch checked out and updating to the new commit given\n> > as the other parameter makes sense.\n> >\n> > But this \"fall back to configured worktree\" is taken when the gave\n> > us NULL worktree or worktree without the .path member (i.e. no\n> > checkout), and it must have come from a NULL return from the call to\n> > find_shared_symref(). IOW, the function said \"no worktree\n> > associated with the repository checks out that branch being\n> > updated.\" I doubt it is a bug to update the working tree of the\n> > repository with the commit pushed to some branch that is *not* HEAD,\n> > only because core.worktree was set to point at an explicit location.\n>\n> Not \"I doubt\", but I suspect it is a bug. Sorry.\n>\n> But in practice, especially with the new code structure, we'd never\n> flip do_update_worktree on unless find_shared_symref() says that the\n> ref we are updating in the function is what is checked out, which\n> means worktree is always non-NULL when we call update_worktree().\n>\n> So, unless there is some situation where worktree->path is NULL for\n> a worktree with a checkout, the \"else if\" above is a dead code, I\n> think.\n>\n> Similarly, I suspect that is_bare_repository() call the patch moved\n> into the if/else if/ chain is even reachable with the updated\n> caller. find_shared_symref() is always called, and unless it gives\n> a non-NULL worktree, do_update_worktree never becomes true.\n>\n> Anyway, enough bug finding in the existing code. I think the\n> update-instead was Dscho's invention and when the codepath was\n> updated to be worktree ready, Dscho helped Hariom to do so, so\n> I'll CC Dscho to see if he has input.\n\nIt's such a blast from the past! I first worked on this in 1404bcbb6b3\n(receive-pack: add another option for receive.denyCurrentBranch,\n2014-11-26), and Hariom & I worked on this last year, before the pandemic\nhit over here (and therefore it feels like a decade ago).\n\nThe `worktree` variable was introduced in 4ef346482d6\n(receive.denyCurrentBranch: respect all worktrees, 2020-02-23), and since\nthe patch under discussion does away with the `is_bare_repository()` call,\nI think that we now can safely change these lines:\n\n if (do_update_worktree) {\n ret = update_worktree(new_oid->hash, find_shared_symref(\"HEAD\", name));\n if (ret)\n return ret;\n }\n\nto pass `worktree` directly to the `update_worktree()` function, rather\nthan calling `find_shared_symref()` again.\n\nAnd since that is the case, I think your analysis is correct, we always\ncall `update_worktree()` with a `worktree` parameter that is non-`NULL`.\n\nAs to the riddle about why we check `git_work_tree_cfg` at all? Back when\nI introduced support for `denyCurrentBranch = updateInstead`, there were\nno worktrees, the only way to give a bare repository a worktree was via\nthat config.\n\nAnd from how I read the code in `worktree`, both \"main\" and \"linked\"\nworktrees do have a `path` attribute that is non-`NULL`. We therefore\nreally have to look at the `is_bare` attribute to know whether\n`worktree->path` _actually_ refers to a worktree. But as you also pointed\nout, `find_shared_symref()` skips any worktree with non-zero `is_bare`.\n\nWe also can be pretty certain that only the `if (worktree &&\nworktree->path)` arm is hit, we should probably turn the code into:\n\n\tif (!worktree || (!worktree->path && !worktree->is_bare))\n BUG(\"update_worktree() called without a path\");\n\n\tif (worktree->is_bare)\n\t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n\n\twork_tree = worktree->path;\n\nCiao,\nDscho\n"},{"id":"440744","messageId":"nycvar.QRO.7.76.6.2111091706290.54@tvgsbejvaqbjf.bet","threadId":"56859","inReplyTo":"20211109030028.2196416-1-andersk@mit.edu","subject":"Re: [PATCH v4 1/4] fetch: Protect branches checked out in all worktrees","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-09T16:09:47Z","receivedAt":"2021-11-09T16:10:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Anders,\n\nOn Mon, 8 Nov 2021, Anders Kaseorg wrote:\n\n> Refuse to fetch into the currently checked out branch of any working\n> tree, not just the current one.\n>\n> Fixes this previously reported bug:\n>\n> https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de\n>\n> As a side effect of using find_shared_symref, we’ll also refuse the\n> fetch when we’re on a detached HEAD because we’re rebasing or bisecting\n> on the branch in question. This seems like a sensible change.\n>\n> Signed-off-by: Anders Kaseorg \n> ---\n> builtin/fetch.c | 28 ++++++++++++++--------------\n> t/t5516-fetch-push.sh | 18 ++++++++++++++++++\n> 2 files changed, 32 insertions(+), 14 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index f7abbc31ff..0ef2170ef9 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -28,6 +28,7 @@\n> #include \"promisor-remote.h\"\n> #include \"commit-graph.h\"\n> #include \"shallow.h\"\n> +#include \"worktree.h\"\n>\n> #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n>\n> @@ -854,7 +855,7 @@ static int update_local_ref(struct ref *ref,\n> \t\t\t int summary_width)\n> {\n> \tstruct commit *current = NULL, *updated;\n> -\tstruct branch *current_branch = branch_get(NULL);\n> +\tconst struct worktree *wt;\n> \tconst char *pretty_ref = prettify_refname(ref->name);\n> \tint fast_forward = 0;\n>\n> @@ -868,16 +869,18 @@ static int update_local_ref(struct ref *ref,\n> \t\treturn 0;\n> \t}\n>\n> -\tif (current_branch &&\n> -\t !strcmp(ref->name, current_branch->name) &&\n> -\t !(update_head_ok || is_bare_repository()) &&\n> +\tif (!update_head_ok &&\n> +\t (wt = find_shared_symref(\"HEAD\", ref->name)) &&\n> +\t !wt->is_bare &&\n> \t !is_null_oid(&ref->old_oid)) {\n> \t\t/*\n> \t\t * If this is the head, and it's not okay to update\n> \t\t * the head, and the old value of the head isn't empty...\n> \t\t */\n> \t\tformat_display(display, '!', _(\"[rejected]\"),\n> -\t\t\t _(\"can't fetch in current branch\"),\n> +\t\t\t wt->is_current ?\n> +\t\t\t _(\"can't fetch in current branch\") :\n> +\t\t\t _(\"checked out in another worktree\"),\n> \t\t\t remote, pretty_ref, summary_width);\n> \t\treturn 1;\n> \t}\n> @@ -1387,16 +1390,13 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,\n>\n> static void check_not_current_branch(struct ref *ref_map)\n> {\n> -\tstruct branch *current_branch = branch_get(NULL);\n> -\n> -\tif (is_bare_repository() || !current_branch)\n> -\t\treturn;\n> -\n> +\tconst struct worktree *wt;\n> \tfor (; ref_map; ref_map = ref_map->next)\n> -\t\tif (ref_map->peer_ref && !strcmp(current_branch->refname,\n> -\t\t\t\t\tref_map->peer_ref->name))\n> -\t\t\tdie(_(\"Refusing to fetch into current branch %s \"\n> -\t\t\t \"of non-bare repository\"), current_branch->refname);\n> +\t\tif (ref_map->peer_ref &&\n> +\t\t (wt = find_shared_symref(\"HEAD\", ref_map->peer_ref->name)))\n\nDo we need `&& !wt->is_bare` here, too?\n\n> +\t\t\tdie(_(\"Refusing to fetch into branch '%s' \"\n> +\t\t\t \"checked out at '%s'\"),\n> +\t\t\t ref_map->peer_ref->name, wt->path);\n> }\n>\n> static int truncate_fetch_head(void)\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 8212ca56dc..2c2d6fa6e7 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1771,4 +1771,22 @@ test_expect_success 'denyCurrentBranch and worktrees' '\n> \tgit -C cloned push origin HEAD:new-wt &&\n> \ttest_must_fail git -C cloned push --delete origin new-wt\n> '\n> +\n> +test_expect_success 'refuse fetch to current branch of worktree' '\n> +\ttest_when_finished \"git worktree remove --force wt\" &&\n\nDo we also need `&& git branch -D wt` here?\n\n> +\tgit worktree add wt &&\n> +\ttest_commit apple &&\n> +\ttest_must_fail git fetch . HEAD:wt &&\n> +\tgit fetch -u . HEAD:wt\n\nMaybe even `test_path_exists wt/apple.t`, to verify that the worktree has\nbeen updated?\n\nThese would also apply to the next test case.\n\n> +'\n> +\n> +test_expect_success 'refuse fetch to current branch of bare repository worktree' '\n> +\ttest_when_finished \"rm -fr bare.git\" &&\n> +\tgit clone --bare . bare.git &&\n> +\tgit -C bare.git worktree add wt &&\n> +\ttest_commit banana &&\n> +\ttest_must_fail git -C bare.git fetch .. HEAD:wt &&\n> +\tgit -C bare.git fetch -u .. HEAD:wt\n> +'\n> +\n> test_done\n\nThanks for working on this!\nDscho\n"},{"id":"440745","messageId":"nycvar.QRO.7.76.6.2111091711450.54@tvgsbejvaqbjf.bet","threadId":"56859","inReplyTo":"20211109030028.2196416-2-andersk@mit.edu","subject":"Re: [PATCH v4 2/4] receive-pack: Clean dead code from update_worktree()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-09T16:16:35Z","receivedAt":"2021-11-09T16:16:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Anders,\n\nlooks good, just one suggestion, see inlined.\n\nOn Mon, 8 Nov 2021, Anders Kaseorg wrote:\n\n> update_worktree() can only be called with a non-NULL worktree parameter,\n> because that’s the only case where we set do_update_worktree = 1.\n> worktree->path is always initialized to non-NULL.\n>\n> Signed-off-by: Anders Kaseorg \n> ---\n> builtin/receive-pack.c | 20 +++++---------------\n> 1 file changed, 5 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 49b846d960..cf575280fc 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1449,29 +1449,19 @@ static const char *push_to_checkout(unsigned char *hash,\n>\n> static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)\n> {\n> -\tconst char *retval, *work_tree, *git_dir = NULL;\n> +\tconst char *retval, *git_dir;\n> \tstruct strvec env = STRVEC_INIT;\n>\n> -\tif (worktree && worktree->path)\n> -\t\twork_tree = worktree->path;\n> -\telse if (git_work_tree_cfg)\n> -\t\twork_tree = git_work_tree_cfg;\n> -\telse\n> -\t\twork_tree = \"..\";\n\nWe might want to make sure that `worktree` and `worktree->path` are\nnon-`NULL`, and otherwise call a `BUG()`.\n\n> -\n> \tif (is_bare_repository())\n\nOkay, I lied, I have two suggestions. Shouldn't this be turned into\n`worktree->is_bare`?\n\nOf course, `find_shared_symref()` will currently not return any worktree\nwith non-zero `is_bare`...\n\n> \t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n> -\tif (worktree)\n> -\t\tgit_dir = get_worktree_git_dir(worktree);\n> -\tif (!git_dir)\n> -\t\tgit_dir = get_git_dir();\n> +\tgit_dir = get_worktree_git_dir(worktree);\n>\n> \tstrvec_pushf(&env, \"GIT_DIR=%s\", absolute_path(git_dir));\n>\n> \tif (!hook_exists(push_to_checkout_hook))\n> -\t\tretval = push_to_deploy(sha1, &env, work_tree);\n> +\t\tretval = push_to_deploy(sha1, &env, worktree->path);\n> \telse\n> -\t\tretval = push_to_checkout(sha1, &env, work_tree);\n> +\t\tretval = push_to_checkout(sha1, &env, worktree->path);\n>\n> \tstrvec_clear(&env);\n> \treturn retval;\n> @@ -1579,7 +1569,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n> \t}\n>\n> \tif (do_update_worktree) {\n> -\t\tret = update_worktree(new_oid->hash, find_shared_symref(\"HEAD\", name));\n> +\t\tret = update_worktree(new_oid->hash, worktree);\n\nMakes sense. If `worktree` is `NULL`, `do_update_worktree` won't ever be\nset, and if `worktree` is not `NULL`, even before we remove that\n`is_bare_repository()` ternary, it is set to `find_shared_symref(\"HEAD\",\nname)` (and `name` is not changed in the `update()` function).\n\nThanks,\nDscho\n\n> \t\tif (ret)\n> \t\t\treturn ret;\n> \t}\n> --\n> 2.33.1\n>\n>\n"},{"id":"440746","messageId":"nycvar.QRO.7.76.6.2111091717230.54@tvgsbejvaqbjf.bet","threadId":"56859","inReplyTo":"20211109030028.2196416-3-andersk@mit.edu","subject":"Re: [PATCH v4 3/4] receive-pack: Protect current branch for bare repository worktree","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-09T16:22:45Z","receivedAt":"2021-11-09T16:22:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Anders,\n\nOn Mon, 8 Nov 2021, Anders Kaseorg wrote:\n\n> A bare repository won’t have a working tree at \"..\", but it may still\n> have separate working trees created with git worktree. We should protect\n> the current branch of such working trees from being updated or deleted,\n> according to receive.denyCurrentBranch.\n>\n> Signed-off-by: Anders Kaseorg \n> ---\n> builtin/receive-pack.c | 4 +---\n> t/t5516-fetch-push.sh | 12 ++++++++++++\n> 2 files changed, 13 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index cf575280fc..5a3c6d8423 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1452,8 +1452,6 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n> \tconst char *retval, *git_dir;\n> \tstruct strvec env = STRVEC_INIT;\n>\n> -\tif (is_bare_repository())\n> -\t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n> \tgit_dir = get_worktree_git_dir(worktree);\n>\n> \tstrvec_pushf(&env, \"GIT_DIR=%s\", absolute_path(git_dir));\n> @@ -1476,7 +1474,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n> \tstruct object_id *old_oid = &cmd->old_oid;\n> \tstruct object_id *new_oid = &cmd->new_oid;\n> \tint do_update_worktree = 0;\n> -\tconst struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref(\"HEAD\", name);\n> +\tconst struct worktree *worktree = find_shared_symref(\"HEAD\", name);\n\nWhile `find_shared_symref()` currently won't return a `worktree` with a\nnon-zero `is_bare`, to future-proof the code we might want to turn the\n`if (worktree)` below (8 lines outside the current diff context) into `if\n(worktree && !worktree->is_bare)`.\n\n>\n> \t/* only refs/... are allowed */\n> \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 2c2d6fa6e7..06cd34b0db 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1772,6 +1772,18 @@ test_expect_success 'denyCurrentBranch and worktrees' '\n> \ttest_must_fail git -C cloned push --delete origin new-wt\n> '\n>\n> +test_expect_success 'denyCurrentBranch and bare repository worktrees' '\n> +\ttest_when_finished \"rm -fr bare.git\" &&\n\nWhile `wt/` will be created inside `bare.git` and therefore be removed,\nthe branch `wt` won't. Maybe add `&& git branch -D wt`?\n\n> +\tgit clone --bare . bare.git &&\n> +\tgit -C bare.git worktree add wt &&\n> +\ttest_commit grape &&\n\nI like fruit, too! Apple, banana, grape, yummy. I wonder what's next :-)\n\n> +\ttest_config -C bare.git receive.denyCurrentBranch refuse &&\n> +\ttest_must_fail git push bare.git HEAD:wt &&\n> +\ttest_config -C bare.git receive.denyCurrentBranch updateInstead &&\n> +\tgit push bare.git HEAD:wt &&\n\nMaybe make sure that `bare.git/wt/grape.t` exists? We do want the worktree\nto be updated, after all...\n\nThanks,\nDscho\n\n> +\ttest_must_fail git push --delete bare.git wt\n> +'\n> +\n> test_expect_success 'refuse fetch to current branch of worktree' '\n> \ttest_when_finished \"git worktree remove --force wt\" &&\n> \tgit worktree add wt &&\n> --\n> 2.33.1\n>\n>\n"},{"id":"440747","messageId":"nycvar.QRO.7.76.6.2111091724150.54@tvgsbejvaqbjf.bet","threadId":"56859","inReplyTo":"20211109030028.2196416-4-andersk@mit.edu","subject":"Re: [PATCH v4 4/4] branch: Protect branches checked out in all worktrees","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-09T16:24:47Z","receivedAt":"2021-11-09T16:25:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Anders,\n\nOn Mon, 8 Nov 2021, Anders Kaseorg wrote:\n\n> Refuse to force-move a branch over the currently checked out branch of\n> any working tree, not just the current one.\n\nGood catch!\n\nI read through the four patches, offered a couple of suggestions, but\nnothing major. Well done!\n\nThank you,\nDscho\n\n>\n> Signed-off-by: Anders Kaseorg \n> ---\n> branch.c | 10 ++++++----\n> t/t3200-branch.sh | 7 +++++++\n> 2 files changed, 13 insertions(+), 4 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 07a46430b3..581f0c02c2 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -199,7 +199,7 @@ int validate_branchname(const char *name, struct strbuf *ref)\n> */\n> int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n> {\n> -\tconst char *head;\n> +\tconst struct worktree *wt;\n>\n> \tif (!validate_branchname(name, ref))\n> \t\treturn 0;\n> @@ -208,9 +208,11 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n> \t\tdie(_(\"A branch named '%s' already exists.\"),\n> \t\t ref->buf + strlen(\"refs/heads/\"));\n>\n> -\thead = resolve_ref_unsafe(\"HEAD\", 0, NULL, NULL);\n> -\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n> -\t\tdie(_(\"Cannot force update the current branch.\"));\n> +\twt = find_shared_symref(\"HEAD\", ref->buf);\n> +\tif (wt && !wt->is_bare)\n> +\t\tdie(_(\"Cannot force update the branch '%s'\"\n> +\t\t \"checked out at '%s'.\"),\n> +\t\t ref->buf + strlen(\"refs/heads/\"), wt->path);\n>\n> \treturn 1;\n> }\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index e575ffb4ff..4c868bf971 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -168,6 +168,13 @@ test_expect_success 'git branch -M foo bar should fail when bar is checked out'\n> \ttest_must_fail git branch -M bar foo\n> '\n>\n> +test_expect_success 'git branch -M foo bar should fail when bar is checked out in worktree' '\n> +\tgit branch -f bar &&\n> +\ttest_when_finished \"git worktree remove wt && git branch -D wt\" &&\n> +\tgit worktree add wt &&\n> +\ttest_must_fail git branch -M bar wt\n> +'\n> +\n> test_expect_success 'git branch -M baz bam should succeed when baz is checked out' '\n> \tgit checkout -b baz &&\n> \tgit branch bam &&\n> --\n> 2.33.1\n>\n>\n"},{"id":"440771","messageId":"316e8579-d720-b40e-66fb-3280e8de1922@mit.edu","threadId":"56859","inReplyTo":"nycvar.QRO.7.76.6.2111091706290.54@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 1/4] fetch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T22:52:36Z","receivedAt":"2021-11-09T22:53:21Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On 11/9/21 08:09, Johannes Schindelin wrote:\n>> +\t\t (wt = find_shared_symref(\"HEAD\", ref_map->peer_ref->name)))\n> \n> Do we need `&& !wt->is_bare` here, too?\n\nSure (in the “future-proofing” sense), will add.\n\n>> +test_expect_success 'refuse fetch to current branch of worktree' '\n>> +\ttest_when_finished \"git worktree remove --force wt\" &&\n> \n> Do we also need `&& git branch -D wt` here?\n\nWill add.\n\n>> +\tgit fetch -u . HEAD:wt\n> \n> Maybe even `test_path_exists wt/apple.t`, to verify that the worktree has\n> been updated?\n\nNot here; git fetch -u never updates working trees, not even the main \nworking tree. Is that a bug? I don’t know, but if it is, it’s \ncertainly a separate one.\n\nAnders\n"},{"id":"440772","messageId":"12fc97fb-50dd-a441-5b8f-dd73e6d693b1@mit.edu","threadId":"56859","inReplyTo":"nycvar.QRO.7.76.6.2111091711450.54@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 2/4] receive-pack: Clean dead code from update_worktree()","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T22:58:10Z","receivedAt":"2021-11-09T22:58:28Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On 11/9/21 08:16, Johannes Schindelin wrote:\n> We might want to make sure that `worktree` and `worktree->path` are\n> non-`NULL`, and otherwise call a `BUG()`.\n\nOkay.\n\n> Okay, I lied, I have two suggestions. Shouldn't this be turned into\n> `worktree->is_bare`?\n\nSure (but in the next commit, since removing is_bare_repository() is a \nbug fix, not pure cleanup).\n\nAnders\n"},{"id":"440773","messageId":"xmqqa6id0w9y.fsf@gitster.g","threadId":"56859","inReplyTo":"316e8579-d720-b40e-66fb-3280e8de1922@mit.edu","subject":"Re: [PATCH v4 1/4] fetch: Protect branches checked out in all worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-09T23:00:41Z","receivedAt":"2021-11-09T23:00:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Kaseorg writes:\n\n>>> +\tgit fetch -u . HEAD:wt\n>> Maybe even `test_path_exists wt/apple.t`, to verify that the\n>> worktree has\n>> been updated?\n>\n> Not here; git fetch -u never updates working trees, not even the main\n> working tree.\n\nCorrect. The \"--update-head-ok\" option was invented to let the user\ntell Git this: I know updating the ref may make the relationship\nbetween HEAD, the index and the working tree inconsistent, and you\nwill normally prevent me from doing so to save me trouble. But in\nthis call, I will reconcile the inconsistencies myself, so do not\nworry about the issue and just update the ref.\n\nSo there is nothing to fix here. If the user wanted to update the\nworking tree, taking the material that was just fetched from the\nother side into account, \"git pull\" would have been used instead.\n\nThanks.\n"},{"id":"440776","messageId":"2f983e36-532f-ac87-9ade-fba4c6b9d276@mit.edu","threadId":"56859","inReplyTo":"nycvar.QRO.7.76.6.2111091717230.54@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 3/4] receive-pack: Protect current branch for bare repository worktree","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T23:03:49Z","receivedAt":"2021-11-09T23:04:03Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On 11/9/21 08:22, Johannes Schindelin wrote:\n> While `find_shared_symref()` currently won't return a `worktree` with a\n> non-zero `is_bare`, to future-proof the code we might want to turn the\n> `if (worktree)` below (8 lines outside the current diff context) into `if\n> (worktree && !worktree->is_bare)`.\n\nWill do.\n\n>> +test_expect_success 'denyCurrentBranch and bare repository worktrees' '\n>> +\ttest_when_finished \"rm -fr bare.git\" &&\n> \n> While `wt/` will be created inside `bare.git` and therefore be removed,\n> the branch `wt` won't. Maybe add `&& git branch -D wt`?\n\nThe branch ‘wt’ is inside bare.git.\n\n> I like fruit, too! Apple, banana, grape, yummy. I wonder what's next :-)\n\nYay!\n\n> Maybe make sure that `bare.git/wt/grape.t` exists? We do want the worktree\n> to be updated, after all...\n\nRight, I’d forgotten this one. Will do.\n\nThanks all for the helpful reviews!\n\nAnders\n"},{"id":"440777","messageId":"20211109230941.2518143-1-andersk@mit.edu","threadId":"56859","inReplyTo":"2f983e36-532f-ac87-9ade-fba4c6b9d276@mit.edu","subject":"[PATCH v5 1/4] fetch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T23:09:38Z","receivedAt":"2021-11-09T23:10:00Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"Refuse to fetch into the currently checked out branch of any working\ntree, not just the current one.\n\nFixes this previously reported bug:\n\nhttps://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de\n\nAs a side effect of using find_shared_symref, we’ll also refuse the\nfetch when we’re on a detached HEAD because we’re rebasing or bisecting\non the branch in question. This seems like a sensible change.\n\nSigned-off-by: Anders Kaseorg \n---\n builtin/fetch.c | 29 +++++++++++++++--------------\n t/t5516-fetch-push.sh | 18 ++++++++++++++++++\n 2 files changed, 33 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f7abbc31ff..ed8a906717 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -28,6 +28,7 @@\n #include \"promisor-remote.h\"\n #include \"commit-graph.h\"\n #include \"shallow.h\"\n+#include \"worktree.h\"\n \n #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n \n@@ -854,7 +855,7 @@ static int update_local_ref(struct ref *ref,\n \t\t\t int summary_width)\n {\n \tstruct commit *current = NULL, *updated;\n-\tstruct branch *current_branch = branch_get(NULL);\n+\tconst struct worktree *wt;\n \tconst char *pretty_ref = prettify_refname(ref->name);\n \tint fast_forward = 0;\n \n@@ -868,16 +869,18 @@ static int update_local_ref(struct ref *ref,\n \t\treturn 0;\n \t}\n \n-\tif (current_branch &&\n-\t !strcmp(ref->name, current_branch->name) &&\n-\t !(update_head_ok || is_bare_repository()) &&\n+\tif (!update_head_ok &&\n+\t (wt = find_shared_symref(\"HEAD\", ref->name)) &&\n+\t !wt->is_bare &&\n \t !is_null_oid(&ref->old_oid)) {\n \t\t/*\n \t\t * If this is the head, and it's not okay to update\n \t\t * the head, and the old value of the head isn't empty...\n \t\t */\n \t\tformat_display(display, '!', _(\"[rejected]\"),\n-\t\t\t _(\"can't fetch in current branch\"),\n+\t\t\t wt->is_current ?\n+\t\t\t _(\"can't fetch in current branch\") :\n+\t\t\t _(\"checked out in another worktree\"),\n \t\t\t remote, pretty_ref, summary_width);\n \t\treturn 1;\n \t}\n@@ -1387,16 +1390,14 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,\n \n static void check_not_current_branch(struct ref *ref_map)\n {\n-\tstruct branch *current_branch = branch_get(NULL);\n-\n-\tif (is_bare_repository() || !current_branch)\n-\t\treturn;\n-\n+\tconst struct worktree *wt;\n \tfor (; ref_map; ref_map = ref_map->next)\n-\t\tif (ref_map->peer_ref && !strcmp(current_branch->refname,\n-\t\t\t\t\tref_map->peer_ref->name))\n-\t\t\tdie(_(\"Refusing to fetch into current branch %s \"\n-\t\t\t \"of non-bare repository\"), current_branch->refname);\n+\t\tif (ref_map->peer_ref &&\n+\t\t (wt = find_shared_symref(\"HEAD\", ref_map->peer_ref->name)) &&\n+\t\t !wt->is_bare)\n+\t\t\tdie(_(\"Refusing to fetch into branch '%s' \"\n+\t\t\t \"checked out at '%s'\"),\n+\t\t\t ref_map->peer_ref->name, wt->path);\n }\n \n static int truncate_fetch_head(void)\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 8212ca56dc..f07e32126f 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1771,4 +1771,22 @@ test_expect_success 'denyCurrentBranch and worktrees' '\n \tgit -C cloned push origin HEAD:new-wt &&\n \ttest_must_fail git -C cloned push --delete origin new-wt\n '\n+\n+test_expect_success 'refuse fetch to current branch of worktree' '\n+\ttest_when_finished \"git worktree remove --force wt && git branch -D wt\" &&\n+\tgit worktree add wt &&\n+\ttest_commit apple &&\n+\ttest_must_fail git fetch . HEAD:wt &&\n+\tgit fetch -u . HEAD:wt\n+'\n+\n+test_expect_success 'refuse fetch to current branch of bare repository worktree' '\n+\ttest_when_finished \"rm -fr bare.git\" &&\n+\tgit clone --bare . bare.git &&\n+\tgit -C bare.git worktree add wt &&\n+\ttest_commit banana &&\n+\ttest_must_fail git -C bare.git fetch .. HEAD:wt &&\n+\tgit -C bare.git fetch -u .. HEAD:wt\n+'\n+\n test_done\n-- \n2.33.1\n\n"},{"id":"440779","messageId":"20211109230941.2518143-3-andersk@mit.edu","threadId":"56859","inReplyTo":"20211109230941.2518143-1-andersk@mit.edu","subject":"[PATCH v5 3/4] receive-pack: Protect current branch for bare repository worktree","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T23:09:40Z","receivedAt":"2021-11-09T23:10:29Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"A bare repository won’t have a working tree at \"..\", but it may still\nhave separate working trees created with git worktree. We should protect\nthe current branch of such working trees from being updated or deleted,\naccording to receive.denyCurrentBranch.\n\nSigned-off-by: Anders Kaseorg \n---\n builtin/receive-pack.c | 6 +++---\n t/t5516-fetch-push.sh | 14 ++++++++++++++\n 2 files changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 542431e692..04a2ec6abc 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1455,7 +1455,7 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n \tif (!worktree || !worktree->path)\n \t\tBUG(\"worktree->path must be non-NULL\");\n \n-\tif (is_bare_repository())\n+\tif (worktree->is_bare)\n \t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n \tgit_dir = get_worktree_git_dir(worktree);\n \n@@ -1479,7 +1479,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tstruct object_id *old_oid = &cmd->old_oid;\n \tstruct object_id *new_oid = &cmd->new_oid;\n \tint do_update_worktree = 0;\n-\tconst struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref(\"HEAD\", name);\n+\tconst struct worktree *worktree = find_shared_symref(\"HEAD\", name);\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n@@ -1491,7 +1491,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tfree(namespaced_name);\n \tnamespaced_name = strbuf_detach(&namespaced_name_buf, NULL);\n \n-\tif (worktree) {\n+\tif (worktree && !worktree->is_bare) {\n \t\tswitch (deny_current_branch) {\n \t\tcase DENY_IGNORE:\n \t\t\tbreak;\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f07e32126f..4847793a1c 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1769,9 +1769,23 @@ test_expect_success 'denyCurrentBranch and worktrees' '\n \ttest_must_fail git -C cloned push origin HEAD:new-wt &&\n \ttest_config receive.denyCurrentBranch updateInstead &&\n \tgit -C cloned push origin HEAD:new-wt &&\n+\ttest_path_exists new-wt/first.t &&\n \ttest_must_fail git -C cloned push --delete origin new-wt\n '\n \n+test_expect_success 'denyCurrentBranch and bare repository worktrees' '\n+\ttest_when_finished \"rm -fr bare.git\" &&\n+\tgit clone --bare . bare.git &&\n+\tgit -C bare.git worktree add wt &&\n+\ttest_commit grape &&\n+\ttest_config -C bare.git receive.denyCurrentBranch refuse &&\n+\ttest_must_fail git push bare.git HEAD:wt &&\n+\ttest_config -C bare.git receive.denyCurrentBranch updateInstead &&\n+\tgit push bare.git HEAD:wt &&\n+\ttest_path_exists bare.git/wt/grape.t &&\n+\ttest_must_fail git push --delete bare.git wt\n+'\n+\n test_expect_success 'refuse fetch to current branch of worktree' '\n \ttest_when_finished \"git worktree remove --force wt && git branch -D wt\" &&\n \tgit worktree add wt &&\n-- \n2.33.1\n\n"},{"id":"440780","messageId":"20211109230941.2518143-4-andersk@mit.edu","threadId":"56859","inReplyTo":"20211109230941.2518143-1-andersk@mit.edu","subject":"[PATCH v5 4/4] branch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T23:09:41Z","receivedAt":"2021-11-09T23:10:35Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"Refuse to force-move a branch over the currently checked out branch of\nany working tree, not just the current one.\n\nSigned-off-by: Anders Kaseorg \n---\n branch.c | 10 ++++++----\n t/t3200-branch.sh | 7 +++++++\n 2 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 07a46430b3..581f0c02c2 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -199,7 +199,7 @@ int validate_branchname(const char *name, struct strbuf *ref)\n */\n int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n {\n-\tconst char *head;\n+\tconst struct worktree *wt;\n \n \tif (!validate_branchname(name, ref))\n \t\treturn 0;\n@@ -208,9 +208,11 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n \t\tdie(_(\"A branch named '%s' already exists.\"),\n \t\t ref->buf + strlen(\"refs/heads/\"));\n \n-\thead = resolve_ref_unsafe(\"HEAD\", 0, NULL, NULL);\n-\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n-\t\tdie(_(\"Cannot force update the current branch.\"));\n+\twt = find_shared_symref(\"HEAD\", ref->buf);\n+\tif (wt && !wt->is_bare)\n+\t\tdie(_(\"Cannot force update the branch '%s'\"\n+\t\t \"checked out at '%s'.\"),\n+\t\t ref->buf + strlen(\"refs/heads/\"), wt->path);\n \n \treturn 1;\n }\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex e575ffb4ff..4c868bf971 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -168,6 +168,13 @@ test_expect_success 'git branch -M foo bar should fail when bar is checked out'\n \ttest_must_fail git branch -M bar foo\n '\n \n+test_expect_success 'git branch -M foo bar should fail when bar is checked out in worktree' '\n+\tgit branch -f bar &&\n+\ttest_when_finished \"git worktree remove wt && git branch -D wt\" &&\n+\tgit worktree add wt &&\n+\ttest_must_fail git branch -M bar wt\n+'\n+\n test_expect_success 'git branch -M baz bam should succeed when baz is checked out' '\n \tgit checkout -b baz &&\n \tgit branch bam &&\n-- \n2.33.1\n\n"},{"id":"440781","messageId":"20211109230941.2518143-2-andersk@mit.edu","threadId":"56859","inReplyTo":"20211109230941.2518143-1-andersk@mit.edu","subject":"[PATCH v5 2/4] receive-pack: Clean dead code from update_worktree()","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T23:09:39Z","receivedAt":"2021-11-09T23:10:37Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"update_worktree() can only be called with a non-NULL worktree parameter,\nbecause that’s the only case where we set do_update_worktree = 1.\nworktree->path is always initialized to non-NULL.\n\nSigned-off-by: Anders Kaseorg \n---\n builtin/receive-pack.c | 21 +++++++--------------\n 1 file changed, 7 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 49b846d960..542431e692 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1449,29 +1449,22 @@ static const char *push_to_checkout(unsigned char *hash,\n \n static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)\n {\n-\tconst char *retval, *work_tree, *git_dir = NULL;\n+\tconst char *retval, *git_dir;\n \tstruct strvec env = STRVEC_INIT;\n \n-\tif (worktree && worktree->path)\n-\t\twork_tree = worktree->path;\n-\telse if (git_work_tree_cfg)\n-\t\twork_tree = git_work_tree_cfg;\n-\telse\n-\t\twork_tree = \"..\";\n+\tif (!worktree || !worktree->path)\n+\t\tBUG(\"worktree->path must be non-NULL\");\n \n \tif (is_bare_repository())\n \t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n-\tif (worktree)\n-\t\tgit_dir = get_worktree_git_dir(worktree);\n-\tif (!git_dir)\n-\t\tgit_dir = get_git_dir();\n+\tgit_dir = get_worktree_git_dir(worktree);\n \n \tstrvec_pushf(&env, \"GIT_DIR=%s\", absolute_path(git_dir));\n \n \tif (!hook_exists(push_to_checkout_hook))\n-\t\tretval = push_to_deploy(sha1, &env, work_tree);\n+\t\tretval = push_to_deploy(sha1, &env, worktree->path);\n \telse\n-\t\tretval = push_to_checkout(sha1, &env, work_tree);\n+\t\tretval = push_to_checkout(sha1, &env, worktree->path);\n \n \tstrvec_clear(&env);\n \treturn retval;\n@@ -1579,7 +1572,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t}\n \n \tif (do_update_worktree) {\n-\t\tret = update_worktree(new_oid->hash, find_shared_symref(\"HEAD\", name));\n+\t\tret = update_worktree(new_oid->hash, worktree);\n \t\tif (ret)\n \t\t\treturn ret;\n \t}\n-- \n2.33.1\n\n"},{"id":"440784","messageId":"xmqqk0hgzz77.fsf@gitster.g","threadId":"56859","inReplyTo":"xmqqa6id0w9y.fsf@gitster.g","subject":"Re: [PATCH v4 1/4] fetch: Protect branches checked out in all worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-09T23:28:12Z","receivedAt":"2021-11-09T23:28:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Another minor thing I noticed and meant to say but kept forgetting.\nWe do not upcase the first word after the : in the commit\ntitle. E.g.\n\nSubject: Re: [PATCH v4 1/4] fetch: protect branches checked out in all worktrees\n\nnot \"fetch: Protect ...\".\n\nThanks.\n"},{"id":"440785","messageId":"99a95f0e-f90b-26fc-3a34-bd90d47de215@mit.edu","threadId":"56859","inReplyTo":"xmqqk0hgzz77.fsf@gitster.g","subject":"Re: [PATCH v4 1/4] fetch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T23:32:42Z","receivedAt":"2021-11-09T23:32:56Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On 11/9/21 15:28, Junio C Hamano wrote:\n> Another minor thing I noticed and meant to say but kept forgetting.\n> We do not upcase the first word after the : in the commit\n> title.\n\nAh okay. Will lowercase if a v6 becomes necessary, otherwise I assume \nyou’ll take care of it.\n\nAnders\n"},{"id":"440806","messageId":"211110.8635o4k6i7.gmgdl@evledraar.gmail.com","threadId":"56859","inReplyTo":"20211109230941.2518143-1-andersk@mit.edu","subject":"Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-10T03:56:03Z","receivedAt":"2021-11-10T03:57:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 09 2021, Anders Kaseorg wrote:\n\n> Refuse to fetch into the currently checked out branch of any working\n> tree, not just the current one.\n>\n> Fixes this previously reported bug:\n>\n> https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de\n>\n> As a side effect of using find_shared_symref, we’ll also refuse the\n> fetch when we’re on a detached HEAD because we’re rebasing or bisecting\n> on the branch in question. This seems like a sensible change.\n\nMissing tests though, would be nice to have a test that saw what\nhappened when the branch is in that \"git bisect start\" or rebasing\nstate.\n\nAlso what those commands to if the branch is updated, e.g. with\ngit-update-ref.\n"},{"id":"440807","messageId":"211110.86y25wirtj.gmgdl@evledraar.gmail.com","threadId":"56859","inReplyTo":"20211109230941.2518143-2-andersk@mit.edu","subject":"Re: [PATCH v5 2/4] receive-pack: Clean dead code from update_worktree()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-10T03:57:40Z","receivedAt":"2021-11-10T03:59:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 09 2021, Anders Kaseorg wrote:\n\n> +\tif (!worktree || !worktree->path)\n> +\t\tBUG(\"worktree->path must be non-NULL\");\n\nPerhaps a metter of taste, but I think BUG() should really be used for\nthings that need a custom message over and beyond what assert() gives\nus.\n\nIn this case using BUG() gives you a worse message, if you do:\n\n assert(worktree && worktree->path)\n\nYou'll get a sensible message from any modern compiler quotign the\nvariable etc, all of which says the same thing as that BUG() message,\njust with less verbosity.\n"},{"id":"440808","messageId":"211110.86tugkirnh.gmgdl@evledraar.gmail.com","threadId":"56859","inReplyTo":"20211109230941.2518143-3-andersk@mit.edu","subject":"Re: [PATCH v5 3/4] receive-pack: Protect current branch for bare repository worktree","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-10T04:00:20Z","receivedAt":"2021-11-10T04:03:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 09 2021, Anders Kaseorg wrote:\n\n> +test_expect_success 'denyCurrentBranch and bare repository worktrees' '\n> +\ttest_when_finished \"rm -fr bare.git\" &&\n> +\tgit clone --bare . bare.git &&\n> +\tgit -C bare.git worktree add wt &&\n> +\ttest_commit grape &&\n> +\ttest_config -C bare.git receive.denyCurrentBranch refuse &&\n> +\ttest_must_fail git push bare.git HEAD:wt &&\n> +\ttest_config -C bare.git receive.denyCurrentBranch updateInstead &&\n> +\tgit push bare.git HEAD:wt &&\n> +\ttest_path_exists bare.git/wt/grape.t &&\n> +\ttest_must_fail git push --delete bare.git wt\n> +'\n> +\n> test_expect_success 'refuse fetch to current branch of worktree' '\n> \ttest_when_finished \"git worktree remove --force wt && git branch -D wt\" &&\n> \tgit worktree add wt &&\n\nNit: Pick either a \"git init sub-repo\" or \"rm -rf when-done.git\" pattern\nas you're doing here, or test_config. It doesn't make sense to combine\nthe two.\n\nWe don't need to run around in test_when_finished and unset config for\nsomething we're about to \"rm -rf\" anyway.\n\nI think it's good practice to avoid test_config whenever possible,\ni.e. it's made redundant by using a sturdier test pattern of not\nneedlessly sharing state.\n\nBut when that's needed, i.e. you need one persistent repo you're\nmodifying, is when it should be used.\n"},{"id":"440809","messageId":"211110.86pmr8ira2.gmgdl@evledraar.gmail.com","threadId":"56859","inReplyTo":"20211109230941.2518143-4-andersk@mit.edu","subject":"Re: [PATCH v5 4/4] branch: Protect branches checked out in all worktrees","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-10T04:03:24Z","receivedAt":"2021-11-10T04:11:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 09 2021, Anders Kaseorg wrote:\n\n> [...]\n> \tif (!validate_branchname(name, ref))\n> \t\treturn 0;\n> @@ -208,9 +208,11 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n> \t\tdie(_(\"A branch named '%s' already exists.\"),\n> \t\t ref->buf + strlen(\"refs/heads/\"));\n> \n> -\thead = resolve_ref_unsafe(\"HEAD\", 0, NULL, NULL);\n> -\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n> -\t\tdie(_(\"Cannot force update the current branch.\"));\n> +\twt = find_shared_symref(\"HEAD\", ref->buf);\n> +\tif (wt && !wt->is_bare)\n> +\t\tdie(_(\"Cannot force update the branch '%s'\"\n\ndie() etc. messages should start with lower-case. See CodingGuidelines.\n\nHere you're changing an existing die() message, but since it's something\ntranslators will need to re-do let's fix it while we're at it.\n"},{"id":"440831","messageId":"nycvar.QRO.7.76.6.2111101303380.21127@tvgsbejvaqbjf.bet","threadId":"56859","inReplyTo":"211110.86y25wirtj.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v5 2/4] receive-pack: Clean dead code from update_worktree()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-10T12:11:11Z","receivedAt":"2021-11-10T12:11:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote:\n\n>\n> On Tue, Nov 09 2021, Anders Kaseorg wrote:\n>\n> > +\tif (!worktree || !worktree->path)\n> > +\t\tBUG(\"worktree->path must be non-NULL\");\n>\n> Perhaps a metter of taste, but I think BUG() should really be used for\n> things that need a custom message over and beyond what assert() gives\n> us.\n>\n> In this case using BUG() gives you a worse message, if you do:\n>\n> assert(worktree && worktree->path)\n>\n> You'll get a sensible message from any modern compiler quotign the\n> variable etc, all of which says the same thing as that BUG() message,\n> just with less verbosity.\n\nMaybe code reviews should stay away from contentious matters of taste.\n\nThis claim that `assert()` would somehow be preferable to `BUG()` is not\nbacked up by our very own coding guidelines. See for yourself:\nhttps://github.com/git/git/blob/v2.33.1/Documentation/CodingGuidelines\ndoes not mention it.\n\nThe question of `assert()` vs `BUG()` has been brought up on this mailing\nlist before, without a clear preference for `assert()`, in contrast to\nwhat the comment quoted above would want to make believe.\n\nAnd the fact that BUG() allows for a well-crafted message without having\nto rely on the compiler to guess as to what would make for a helpful\nmessage, that alone speaks volumes.\n\nCiao,\nJohannes\n"},{"id":"440832","messageId":"nycvar.QRO.7.76.6.2111101315330.21127@tvgsbejvaqbjf.bet","threadId":"56859","inReplyTo":"20211109230941.2518143-1-andersk@mit.edu","subject":"Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-10T12:18:55Z","receivedAt":"2021-11-10T12:19:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Anders,\n\nresponding here instead of to the cover letter (because there is none\n;-)): great work!\n\nOthers pointed out the really tiny nit that some phrases should start with\nlower-case, which would be nice to see addressed. I did not find any major\nissue anymore (apart from the slightly iffy assumption that `buf->ref`\nstarts with `refs/heads/` and therefore `buf->ref + strlen(\"refs/heads/\")`\nwould not overrun, but I _think_ the current code enforces that prefix\nsomewhere along the lines), so:\n\n\tAcked-by: Johannes Schindelin \n\nThank you for this contribution!\nDscho\n\nOn Tue, 9 Nov 2021, Anders Kaseorg wrote:\n\n> Refuse to fetch into the currently checked out branch of any working\n> tree, not just the current one.\n>\n> Fixes this previously reported bug:\n>\n> https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de\n>\n> As a side effect of using find_shared_symref, we’ll also refuse the\n> fetch when we’re on a detached HEAD because we’re rebasing or bisecting\n> on the branch in question. This seems like a sensible change.\n>\n> Signed-off-by: Anders Kaseorg \n> ---\n> builtin/fetch.c | 29 +++++++++++++++--------------\n> t/t5516-fetch-push.sh | 18 ++++++++++++++++++\n> 2 files changed, 33 insertions(+), 14 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index f7abbc31ff..ed8a906717 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -28,6 +28,7 @@\n> #include \"promisor-remote.h\"\n> #include \"commit-graph.h\"\n> #include \"shallow.h\"\n> +#include \"worktree.h\"\n>\n> #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n>\n> @@ -854,7 +855,7 @@ static int update_local_ref(struct ref *ref,\n> \t\t\t int summary_width)\n> {\n> \tstruct commit *current = NULL, *updated;\n> -\tstruct branch *current_branch = branch_get(NULL);\n> +\tconst struct worktree *wt;\n> \tconst char *pretty_ref = prettify_refname(ref->name);\n> \tint fast_forward = 0;\n>\n> @@ -868,16 +869,18 @@ static int update_local_ref(struct ref *ref,\n> \t\treturn 0;\n> \t}\n>\n> -\tif (current_branch &&\n> -\t !strcmp(ref->name, current_branch->name) &&\n> -\t !(update_head_ok || is_bare_repository()) &&\n> +\tif (!update_head_ok &&\n> +\t (wt = find_shared_symref(\"HEAD\", ref->name)) &&\n> +\t !wt->is_bare &&\n> \t !is_null_oid(&ref->old_oid)) {\n> \t\t/*\n> \t\t * If this is the head, and it's not okay to update\n> \t\t * the head, and the old value of the head isn't empty...\n> \t\t */\n> \t\tformat_display(display, '!', _(\"[rejected]\"),\n> -\t\t\t _(\"can't fetch in current branch\"),\n> +\t\t\t wt->is_current ?\n> +\t\t\t _(\"can't fetch in current branch\") :\n> +\t\t\t _(\"checked out in another worktree\"),\n> \t\t\t remote, pretty_ref, summary_width);\n> \t\treturn 1;\n> \t}\n> @@ -1387,16 +1390,14 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,\n>\n> static void check_not_current_branch(struct ref *ref_map)\n> {\n> -\tstruct branch *current_branch = branch_get(NULL);\n> -\n> -\tif (is_bare_repository() || !current_branch)\n> -\t\treturn;\n> -\n> +\tconst struct worktree *wt;\n> \tfor (; ref_map; ref_map = ref_map->next)\n> -\t\tif (ref_map->peer_ref && !strcmp(current_branch->refname,\n> -\t\t\t\t\tref_map->peer_ref->name))\n> -\t\t\tdie(_(\"Refusing to fetch into current branch %s \"\n> -\t\t\t \"of non-bare repository\"), current_branch->refname);\n> +\t\tif (ref_map->peer_ref &&\n> +\t\t (wt = find_shared_symref(\"HEAD\", ref_map->peer_ref->name)) &&\n> +\t\t !wt->is_bare)\n> +\t\t\tdie(_(\"Refusing to fetch into branch '%s' \"\n> +\t\t\t \"checked out at '%s'\"),\n> +\t\t\t ref_map->peer_ref->name, wt->path);\n> }\n>\n> static int truncate_fetch_head(void)\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 8212ca56dc..f07e32126f 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1771,4 +1771,22 @@ test_expect_success 'denyCurrentBranch and worktrees' '\n> \tgit -C cloned push origin HEAD:new-wt &&\n> \ttest_must_fail git -C cloned push --delete origin new-wt\n> '\n> +\n> +test_expect_success 'refuse fetch to current branch of worktree' '\n> +\ttest_when_finished \"git worktree remove --force wt && git branch -D wt\" &&\n> +\tgit worktree add wt &&\n> +\ttest_commit apple &&\n> +\ttest_must_fail git fetch . HEAD:wt &&\n> +\tgit fetch -u . HEAD:wt\n> +'\n> +\n> +test_expect_success 'refuse fetch to current branch of bare repository worktree' '\n> +\ttest_when_finished \"rm -fr bare.git\" &&\n> +\tgit clone --bare . bare.git &&\n> +\tgit -C bare.git worktree add wt &&\n> +\ttest_commit banana &&\n> +\ttest_must_fail git -C bare.git fetch .. HEAD:wt &&\n> +\tgit -C bare.git fetch -u .. HEAD:wt\n> +'\n> +\n> test_done\n> --\n> 2.33.1\n>\n>\n"},{"id":"440879","messageId":"xmqqr1bnwtln.fsf@gitster.g","threadId":"56859","inReplyTo":"20211109230941.2518143-1-andersk@mit.edu","subject":"Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-10T22:09:40Z","receivedAt":"2021-11-10T22:11:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Kaseorg writes:\n\n> +\tif (!update_head_ok &&\n> +\t (wt = find_shared_symref(\"HEAD\", ref->name)) &&\n> +\t !wt->is_bare &&\n> \t !is_null_oid(&ref->old_oid)) {\n> \t\t/*\n> \t\t * If this is the head, and it's not okay to update\n> \t\t * the head, and the old value of the head isn't empty...\n> \t\t */\n> \t\tformat_display(display, '!', _(\"[rejected]\"),\n> -\t\t\t _(\"can't fetch in current branch\"),\n> +\t\t\t wt->is_current ?\n> +\t\t\t _(\"can't fetch in current branch\") :\n> +\t\t\t _(\"checked out in another worktree\"),\n> \t\t\t remote, pretty_ref, summary_width);\n> \t\treturn 1;\n> \t}\n> @@ -1387,16 +1390,14 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,\n> \n> static void check_not_current_branch(struct ref *ref_map)\n> {\n> -\tstruct branch *current_branch = branch_get(NULL);\n> -\n> -\tif (is_bare_repository() || !current_branch)\n> -\t\treturn;\n> -\n> +\tconst struct worktree *wt;\n> \tfor (; ref_map; ref_map = ref_map->next)\n> -\t\tif (ref_map->peer_ref && !strcmp(current_branch->refname,\n> -\t\t\t\t\tref_map->peer_ref->name))\n> -\t\t\tdie(_(\"Refusing to fetch into current branch %s \"\n> -\t\t\t \"of non-bare repository\"), current_branch->refname);\n> +\t\tif (ref_map->peer_ref &&\n> +\t\t (wt = find_shared_symref(\"HEAD\", ref_map->peer_ref->name)) &&\n> +\t\t !wt->is_bare)\n> +\t\t\tdie(_(\"Refusing to fetch into branch '%s' \"\n> +\t\t\t \"checked out at '%s'\"),\n> +\t\t\t ref_map->peer_ref->name, wt->path);\n> }\n\nAnother thing for the next development cycle.\n\nThe find_shared_symref() function is handy (and more correct than\ndereferencing HEAD in the current worktree alone, of course), but\nits memory ownership model may need to be rethought.\n\nThe current semantics is a caller can call find_shared_symref() to\nreceive at most one worktree, and the caller can use it UNTIL\nanybody makes another call to find_shared_symref(), at which point,\nthe worktree instance becomes unusable and off limit. The caller\ncannot, and should not attempt to, free the worktree instance.\n\nEach time find_shared_symref() is called, we enumerate all the\nworktrees and store them in a list that is static to the function.\nThe returned worktree instance points into that list. It is not\ntechnically leaked because the static \"worktrees\" list in the\nfunction holds onto it, and each time the function is called, the\nold list of worktrees is discarded and rebuilt anew. What it means\nis that a code like the above, a loop in check_not_current_branch(),\nthat repeatedly calls find_shared_symref() is both inefficient\n(because it takes many \"snapshots\" of the worktrees attached to the\nrepository), and also risks an inconsistent view of the world\n(because it takes many \"snapshots\", and each iteration uses\ndifferent ones).\n\nI suspect that having a new API function that lets the above be\nrewritten along the lines of ...\n\n\tstruct worktree **all = get_worktrees();\n\tfor ( ; ref_map; ref_map = ref_map->next) {\n \tif (!ref_map->peer_ref)\n\t\t\tcontinue;\n wt = find_wt_with_HEAD(all, ref->map->peer_ref->name);\n\t\tif (!wt->is_bare)\n\t\t\tdie(_(\"...\"));\n\t}\n\tfree_worktrees(all);\n\n... would help.\n\nThanks.\n"},{"id":"440882","messageId":"alpine.DEB.2.21.999.2111101828580.104475@tardis-on-the-dome.mit.edu","threadId":"56859","inReplyTo":"xmqqr1bnwtln.fsf@gitster.g","subject":"Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-10T23:33:22Z","receivedAt":"2021-11-10T23:33:38Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On Wed, 10 Nov 2021, Junio C Hamano wrote:\n> The find_shared_symref() function is handy (and more correct than\n> dereferencing HEAD in the current worktree alone, of course), but\n> its memory ownership model may need to be rethought.\n\nI wasn’t sure if we wanted to expand the scope of this series, but I do \nagree. How about moving worktrees from the static variable to a parameter \nof find_shared_symref()? Would you like me to rebase the series onto this \npatch?\n\nAnders\n\n-- >8 --\nSubject: [PATCH] worktree: simplify find_shared_symref() memory ownership model\n\nStoring the worktrees list in a static variable meant that\nfind_shared_symref() had to rebuild the list on each call (which is\ninefficient when the call site is in a loop), and also that each call\ninvalidated the pointer returned by the previous call (which is\nconfusing).\n\nInstead, make it the caller’s responsibility to pass in the worktrees\nlist and manage its lifetime.\n\nSigned-off-by: Anders Kaseorg \n---\n branch.c | 14 ++++++----\n builtin/branch.c | 7 ++++-\n builtin/notes.c | 6 +++-\n builtin/receive-pack.c | 63 +++++++++++++++++++++++++++---------------\n worktree.c | 8 ++----\n worktree.h | 5 ++--\n 6 files changed, 65 insertions(+), 38 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 07a46430b3..302cc5a04d 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -357,14 +357,16 @@ void remove_branch_state(struct repository *r, int verbose)\n \n void die_if_checked_out(const char *branch, int ignore_current_worktree)\n {\n+\tstruct worktree **worktrees = get_worktrees();\n \tconst struct worktree *wt;\n \n-\twt = find_shared_symref(\"HEAD\", branch);\n-\tif (!wt || (ignore_current_worktree && wt->is_current))\n-\t\treturn;\n-\tskip_prefix(branch, \"refs/heads/\", &branch);\n-\tdie(_(\"'%s' is already checked out at '%s'\"),\n-\t branch, wt->path);\n+\twt = find_shared_symref(worktrees, \"HEAD\", branch);\n+\tif (wt && (!ignore_current_worktree || !wt->is_current)) {\n+\t\tskip_prefix(branch, \"refs/heads/\", &branch);\n+\t\tdie(_(\"'%s' is already checked out at '%s'\"), branch, wt->path);\n+\t}\n+\n+\tfree_worktrees(worktrees);\n }\n \n int replace_each_worktree_head_symref(const char *oldref, const char *newref,\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 7a1d1eeb07..d8f2164cd7 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -193,6 +193,7 @@ static void delete_branch_config(const char *branchname)\n static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t int quiet)\n {\n+\tstruct worktree **worktrees;\n \tstruct commit *head_rev = NULL;\n \tstruct object_id oid;\n \tchar *name = NULL;\n@@ -229,6 +230,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tif (!head_rev)\n \t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n \t}\n+\n+\tworktrees = get_worktrees();\n+\n \tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n \t\tchar *target = NULL;\n \t\tint flags = 0;\n@@ -239,7 +243,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \n \t\tif (kinds == FILTER_REFS_BRANCHES) {\n \t\t\tconst struct worktree *wt =\n-\t\t\t\tfind_shared_symref(\"HEAD\", name);\n+\t\t\t\tfind_shared_symref(worktrees, \"HEAD\", name);\n \t\t\tif (wt) {\n \t\t\t\terror(_(\"Cannot delete branch '%s' \"\n \t\t\t\t\t\"checked out at '%s'\"),\n@@ -300,6 +304,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \n \tfree(name);\n \tstrbuf_release(&bname);\n+\tfree_worktrees(worktrees);\n \n \treturn ret;\n }\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 71c59583a1..7f60408dbb 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -861,15 +861,19 @@ static int merge(int argc, const char **argv, const char *prefix)\n \t\tupdate_ref(msg.buf, default_notes_ref(), &result_oid, NULL, 0,\n \t\t\t UPDATE_REFS_DIE_ON_ERR);\n \telse { /* Merge has unresolved conflicts */\n+\t\tstruct worktree **worktrees;\n \t\tconst struct worktree *wt;\n \t\t/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */\n \t\tupdate_ref(msg.buf, \"NOTES_MERGE_PARTIAL\", &result_oid, NULL,\n \t\t\t 0, UPDATE_REFS_DIE_ON_ERR);\n \t\t/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */\n-\t\twt = find_shared_symref(\"NOTES_MERGE_REF\", default_notes_ref());\n+\t\tworktrees = get_worktrees();\n+\t\twt = find_shared_symref(worktrees, \"NOTES_MERGE_REF\",\n+\t\t\t\t\tdefault_notes_ref());\n \t\tif (wt)\n \t\t\tdie(_(\"a notes merge into %s is already in-progress at %s\"),\n \t\t\t default_notes_ref(), wt->path);\n+\t\tfree_worktrees(worktrees);\n \t\tif (create_symref(\"NOTES_MERGE_REF\", default_notes_ref(), NULL))\n \t\t\tdie(_(\"failed to store link to current notes ref (%s)\"),\n \t\t\t default_notes_ref());\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 49b846d960..017c365298 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1486,12 +1486,17 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tstruct object_id *old_oid = &cmd->old_oid;\n \tstruct object_id *new_oid = &cmd->new_oid;\n \tint do_update_worktree = 0;\n-\tconst struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref(\"HEAD\", name);\n+\tstruct worktree **worktrees = get_worktrees();\n+\tconst struct worktree *worktree =\n+\t\tis_bare_repository() ?\n+\t\t\tNULL :\n+\t\t\tfind_shared_symref(worktrees, \"HEAD\", name);\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n \t\trp_error(\"refusing to create funny ref '%s' remotely\", name);\n-\t\treturn \"funny refname\";\n+\t\tret = \"funny refname\";\n+\t\tgoto out;\n \t}\n \n \tstrbuf_addf(&namespaced_name_buf, \"%s%s\", get_git_namespace(), name);\n@@ -1510,7 +1515,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\trp_error(\"refusing to update checked out branch: %s\", name);\n \t\t\tif (deny_current_branch == DENY_UNCONFIGURED)\n \t\t\t\trefuse_unconfigured_deny();\n-\t\t\treturn \"branch is currently checked out\";\n+\t\t\tret = \"branch is currently checked out\";\n+\t\t\tgoto out;\n \t\tcase DENY_UPDATE_INSTEAD:\n \t\t\t/* pass -- let other checks intervene first */\n \t\t\tdo_update_worktree = 1;\n@@ -1521,13 +1527,15 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tif (!is_null_oid(new_oid) && !has_object_file(new_oid)) {\n \t\terror(\"unpack should have generated %s, \"\n \t\t \"but I can't find it!\", oid_to_hex(new_oid));\n-\t\treturn \"bad pack\";\n+\t\tret = \"bad pack\";\n+\t\tgoto out;\n \t}\n \n \tif (!is_null_oid(old_oid) && is_null_oid(new_oid)) {\n \t\tif (deny_deletes && starts_with(name, \"refs/heads/\")) {\n \t\t\trp_error(\"denying ref deletion for %s\", name);\n-\t\t\treturn \"deletion prohibited\";\n+\t\t\tret = \"deletion prohibited\";\n+\t\t\tgoto out;\n \t\t}\n \n \t\tif (worktree || (head_name && !strcmp(namespaced_name, head_name))) {\n@@ -1543,9 +1551,11 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\tif (deny_delete_current == DENY_UNCONFIGURED)\n \t\t\t\t\trefuse_unconfigured_deny_delete_current();\n \t\t\t\trp_error(\"refusing to delete the current branch: %s\", name);\n-\t\t\t\treturn \"deletion of the current branch prohibited\";\n+\t\t\t\tret = \"deletion of the current branch prohibited\";\n+\t\t\t\tgoto out;\n \t\t\tdefault:\n-\t\t\t\treturn \"Invalid denyDeleteCurrent setting\";\n+\t\t\t\tret = \"Invalid denyDeleteCurrent setting\";\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t}\n \t}\n@@ -1563,25 +1573,30 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t old_object->type != OBJ_COMMIT ||\n \t\t new_object->type != OBJ_COMMIT) {\n \t\t\terror(\"bad sha1 objects for %s\", name);\n-\t\t\treturn \"bad ref\";\n+\t\t\tret = \"bad ref\";\n+\t\t\tgoto out;\n \t\t}\n \t\told_commit = (struct commit *)old_object;\n \t\tnew_commit = (struct commit *)new_object;\n \t\tif (!in_merge_bases(old_commit, new_commit)) {\n \t\t\trp_error(\"denying non-fast-forward %s\"\n \t\t\t\t \" (you should pull first)\", name);\n-\t\t\treturn \"non-fast-forward\";\n+\t\t\tret = \"non-fast-forward\";\n+\t\t\tgoto out;\n \t\t}\n \t}\n \tif (run_update_hook(cmd)) {\n \t\trp_error(\"hook declined to update %s\", name);\n-\t\treturn \"hook declined\";\n+\t\tret = \"hook declined\";\n+\t\tgoto out;\n \t}\n \n \tif (do_update_worktree) {\n-\t\tret = update_worktree(new_oid->hash, find_shared_symref(\"HEAD\", name));\n+\t\tret = update_worktree(new_oid->hash,\n+\t\t\t\t find_shared_symref(worktrees, \"HEAD\",\n+\t\t\t\t\t\t\t name));\n \t\tif (ret)\n-\t\t\treturn ret;\n+\t\t\tgoto out;\n \t}\n \n \tif (is_null_oid(new_oid)) {\n@@ -1600,17 +1615,19 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\t\t old_oid,\n \t\t\t\t\t 0, \"push\", &err)) {\n \t\t\trp_error(\"%s\", err.buf);\n-\t\t\tstrbuf_release(&err);\n-\t\t\treturn \"failed to delete\";\n+\t\t\tret = \"failed to delete\";\n+\t\t} else {\n+\t\t\tret = NULL; /* good */\n \t\t}\n \t\tstrbuf_release(&err);\n-\t\treturn NULL; /* good */\n \t}\n \telse {\n \t\tstruct strbuf err = STRBUF_INIT;\n \t\tif (shallow_update && si->shallow_ref[cmd->index] &&\n-\t\t update_shallow_ref(cmd, si))\n-\t\t\treturn \"shallow error\";\n+\t\t update_shallow_ref(cmd, si)) {\n+\t\t\tret = \"shallow error\";\n+\t\t\tgoto out;\n+\t\t}\n \n \t\tif (ref_transaction_update(transaction,\n \t\t\t\t\t namespaced_name,\n@@ -1618,14 +1635,16 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\t\t 0, \"push\",\n \t\t\t\t\t &err)) {\n \t\t\trp_error(\"%s\", err.buf);\n-\t\t\tstrbuf_release(&err);\n-\n-\t\t\treturn \"failed to update ref\";\n+\t\t\tret = \"failed to update ref\";\n+\t\t} else {\n+\t\t\tret = NULL; /* good */\n \t\t}\n \t\tstrbuf_release(&err);\n-\n-\t\treturn NULL; /* good */\n \t}\n+\n+out:\n+\tfree_worktrees(worktrees);\n+\treturn ret;\n }\n \n static void run_update_post_hook(struct command *commands)\ndiff --git a/worktree.c b/worktree.c\nindex 092a4f92ad..cf13d63845 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -402,17 +402,13 @@ int is_worktree_being_bisected(const struct worktree *wt,\n * bisect). New commands that do similar things should update this\n * function as well.\n */\n-const struct worktree *find_shared_symref(const char *symref,\n+const struct worktree *find_shared_symref(struct worktree **worktrees,\n+\t\t\t\t\t const char *symref,\n \t\t\t\t\t const char *target)\n {\n \tconst struct worktree *existing = NULL;\n-\tstatic struct worktree **worktrees;\n \tint i = 0;\n \n-\tif (worktrees)\n-\t\tfree_worktrees(worktrees);\n-\tworktrees = get_worktrees();\n-\n \tfor (i = 0; worktrees[i]; i++) {\n \t\tstruct worktree *wt = worktrees[i];\n \t\tconst char *symref_target;\ndiff --git a/worktree.h b/worktree.h\nindex 8b7c408132..9e06fcbdf3 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -143,9 +143,10 @@ void free_worktrees(struct worktree **);\n /*\n * Check if a per-worktree symref points to a ref in the main worktree\n * or any linked worktree, and return the worktree that holds the ref,\n- * or NULL otherwise. The result may be destroyed by the next call.\n+ * or NULL otherwise.\n */\n-const struct worktree *find_shared_symref(const char *symref,\n+const struct worktree *find_shared_symref(struct worktree **worktrees,\n+\t\t\t\t\t const char *symref,\n \t\t\t\t\t const char *target);\n \n /*\n-- \n2.33.1\n"},{"id":"440885","messageId":"xmqq8rxvwp4b.fsf@gitster.g","threadId":"56859","inReplyTo":"nycvar.QRO.7.76.6.2111101315330.21127@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-10T23:46:28Z","receivedAt":"2021-11-10T23:46:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> ... (apart from the slightly iffy assumption that `buf->ref`\n> starts with `refs/heads/` and therefore `buf->ref + strlen(\"refs/heads/\")`\n> would not overrun, but I _think_ the current code enforces that prefix\n> somewhere along the lines)\n\nI think that is in 4/4, where the existing code does this:\n\n> diff --git a/branch.c b/branch.c\n> index 7a88a4861e..1aaf694b39 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -199,18 +199,20 @@ int validate_branchname(const char *name, struct strbuf *ref)\n> */\n> int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n> {\n> -\tconst char *head;\n> +\tconst struct worktree *wt;\n> \n> \tif (!validate_branchname(name, ref))\n> \t\treturn 0;\n\nThis takes a bare branch name in \"name\" (or a shorthand like @{-1}),\nexpand that into a full refname into \"ref\". Before passing the ref\ninto check_refname_format(), \"refs/heads/\" is unconditionally added\nat the beginning. So we know ref begins with \"refs/heads/\" after\nthis point.\n\n> \tif (!force)\n> \t\tdie(_(\"A branch named '%s' already exists.\"),\n> \t\t ref->buf + strlen(\"refs/heads/\"));\n\nAnd we already assume ref->buf has \"refs/heads/\" as its prefix. It\nmay be nice to use skip_prefix(), but it probably is not worth it.\n\n> +\twt = find_shared_symref(\"HEAD\", ref->buf);\n> +\tif (wt && !wt->is_bare)\n> +\t\tdie(_(\"Cannot force update the branch '%s'\"\n> +\t\t \"checked out at '%s'.\"),\n> +\t\t ref->buf + strlen(\"refs/heads/\"), wt->path);\n\nAnd this new use just reuses what we assume to be valid.\n\nSo, correctness-wise, I do not think there is much to tweak further\non top of this round. I've always queued this round more or less\nas-is.\n\nIn preparation for the next development cycle, however, it might\nmake sense to add a preparatory clean-up step to downcase the first\nword of \"die()\" messages in the files that are involved in this\nseries (not necessarily the ones that are touched by the patches,\nbut all of them) and then apply these four patches (with matching\nadjustments, like \"Cannot force update\" -> \"cannot force update\") on\ntop. In another review message, I also noticed some inefficient\ncode that is due to insufficient support from the worktree.c API,\nbut that is not about correctness and can be left out of the series\nto get these fixes early in the next cycle.\n\nThanks.\n\n\n"},{"id":"440890","messageId":"xmqq4k8jwnyg.fsf@gitster.g","threadId":"56859","inReplyTo":"xmqq8rxvwp4b.fsf@gitster.g","subject":"Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-11T00:11:35Z","receivedAt":"2021-11-11T00:11:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> So, correctness-wise, I do not think there is much to tweak further\n> on top of this round. I've always queued this round more or less\n> as-is.\n\nGahh. Sorry for the typo: \"I've already queued\", of course.\n\n"}]}