{"thread":{"id":"56847","subject":"[PATCH] fetch: Protect branches checked out in all worktrees","startedAt":"2021-11-04T21:06:07Z","lastAt":"2021-11-08T20:12:33Z","messageCount":7,"participants":["Anders Kaseorg","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"440483","messageId":"9dfca5ec-4426-d3d4-988e-b81ebb087584@mit.edu","threadId":"56847","inReplyTo":null,"subject":"[PATCH] fetch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-04T21:05:54Z","receivedAt":"2021-11-04T21:06:07Z","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\n\ntree, not just the current one.\n\n\n\nFixes this previously reported bug:\n\n\n\nhttps://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de\n\n\n\nSigned-off-by: Anders Kaseorg <andersk@mit.edu>\n\n---\n\n builtin/fetch.c       | 22 +++++++++-------------\n\n t/t5516-fetch-push.sh | 10 ++++++++++\n\n 2 files changed, 19 insertions(+), 13 deletions(-)\n\n\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\n\nindex f7abbc31ff..a6549c2ab6 100644\n\n--- a/builtin/fetch.c\n\n+++ b/builtin/fetch.c\n\n@@ -28,6 +28,7 @@\n\n #include \"promisor-remote.h\"\n\n #include \"commit-graph.h\"\n\n #include \"shallow.h\"\n\n+#include \"worktree.h\"\n\n \n\n #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n\n \n\n@@ -854,7 +855,6 @@ static int update_local_ref(struct ref *ref,\n\n \t\t\t    int summary_width)\n\n {\n\n \tstruct commit *current = NULL, *updated;\n\n-\tstruct branch *current_branch = branch_get(NULL);\n\n \tconst char *pretty_ref = prettify_refname(ref->name);\n\n \tint fast_forward = 0;\n\n \n\n@@ -868,9 +868,8 @@ static int update_local_ref(struct ref *ref,\n\n \t\treturn 0;\n\n \t}\n\n \n\n-\tif (current_branch &&\n\n-\t    !strcmp(ref->name, current_branch->name) &&\n\n-\t    !(update_head_ok || is_bare_repository()) &&\n\n+\tif (!update_head_ok &&\n\n+\t    find_shared_symref(\"HEAD\", ref->name) &&\n\n \t    !is_null_oid(&ref->old_oid)) {\n\n \t\t/*\n\n \t\t * If this is the head, and it's not okay to update\n\n@@ -1387,16 +1386,13 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,\n\n \n\n static void check_not_current_branch(struct ref *ref_map)\n\n {\n\n-\tstruct branch *current_branch = branch_get(NULL);\n\n-\n\n-\tif (is_bare_repository() || !current_branch)\n\n-\t\treturn;\n\n-\n\n+\tconst struct worktree *wt;\n\n \tfor (; ref_map; ref_map = ref_map->next)\n\n-\t\tif (ref_map->peer_ref && !strcmp(current_branch->refname,\n\n-\t\t\t\t\tref_map->peer_ref->name))\n\n-\t\t\tdie(_(\"Refusing to fetch into current branch %s \"\n\n-\t\t\t    \"of non-bare repository\"), current_branch->refname);\n\n+\t\tif (ref_map->peer_ref &&\n\n+\t\t    (wt = find_shared_symref(\"HEAD\", ref_map->peer_ref->name)))\n\n+\t\t\tdie(_(\"Refusing to fetch into branch '%s' \"\n\n+\t\t\t      \"checked out at '%s'\"),\n\n+\t\t\t    ref_map->peer_ref->name, wt->path);\n\n }\n\n \n\n static int truncate_fetch_head(void)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n\nindex 8212ca56dc..c8cb62c05d 100755\n\n--- a/t/t5516-fetch-push.sh\n\n+++ b/t/t5516-fetch-push.sh\n\n@@ -1771,4 +1771,14 @@ test_expect_success 'denyCurrentBranch and worktrees' '\n\n \tgit -C cloned push origin HEAD:new-wt &&\n\n \ttest_must_fail git -C cloned push --delete origin new-wt\n\n '\n\n+\n\n+test_expect_success 'refuse fetch to current branch of worktree' '\n\n+\ttest_commit -C cloned second &&\n\n+\ttest_must_fail git fetch cloned HEAD:new-wt &&\n\n+\tgit clone --bare . bare.git &&\n\n+\tgit -C bare.git worktree add bare-wt &&\n\n+\ttest_must_fail git -C bare.git fetch ../cloned HEAD:bare-wt &&\n\n+\tgit fetch -u cloned HEAD:new-wt &&\n\n+\tgit -C bare.git fetch -u ../cloned HEAD:bare-wt\n\n+'\n\n test_done\n\n-- \n\n2.33.0\n\n"},{"id":"440484","messageId":"alpine.DEB.2.21.999.2111041709080.94135@tardis-on-the-dome.mit.edu","threadId":"56847","inReplyTo":"9dfca5ec-4426-d3d4-988e-b81ebb087584@mit.edu","subject":"[PATCH v2] fetch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-04T21:10:52Z","receivedAt":"2021-11-04T21:10:58Z","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\nSigned-off-by: Anders Kaseorg <andersk@mit.edu>\n---\n\n(Eep! Sorry for the newline corruption in the previous message. \nThunderbird has forsaken me.)\n\n builtin/fetch.c       | 22 +++++++++-------------\n t/t5516-fetch-push.sh | 10 ++++++++++\n 2 files changed, 19 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f7abbc31ff..a6549c2ab6 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,6 @@ 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 char *pretty_ref = prettify_refname(ref->name);\n \tint fast_forward = 0;\n \n@@ -868,9 +868,8 @@ 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    find_shared_symref(\"HEAD\", ref->name) &&\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@@ -1387,16 +1386,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..c8cb62c05d 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1771,4 +1771,14 @@ 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_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+'\n test_done\n-- \n2.33.0\n\n"},{"id":"440511","messageId":"YYTtdZblnHYgDgBq@coredump.intra.peff.net","threadId":"56847","inReplyTo":"alpine.DEB.2.21.999.2111041709080.94135@tardis-on-the-dome.mit.edu","subject":"Re: [PATCH v2] fetch: Protect branches checked out in all worktrees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-05T08:38:13Z","receivedAt":"2021-11-05T08:38:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 04, 2021 at 05:10:52PM -0400, 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\nI think that makes sense as a goal.\n\n>  #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n>  \n> @@ -854,7 +855,6 @@ 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 char *pretty_ref = prettify_refname(ref->name);\n>  \tint fast_forward = 0;\n>  \n> @@ -868,9 +868,8 @@ 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    find_shared_symref(\"HEAD\", ref->name) &&\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\nOK, so this is where we'd say \"no, I won't update the HEAD\". That makes\nsense, and the find_shared_symref() helper from worktree.h makes it\neasy.\n\nIf we get the non-default worktree here, it might be nice to tell the\nuser which worktree has it checked out. Otherwise it may lead to\nhead-scratching as they peek at their current HEAD.\n\nI also notice that find_shared_symref() will catch cases where we're on\na detached HEAD, but it's because we're rebasing or bisecting on a given\nbranch. Arguably that's a sensible thing to do for this check, but it is\na change from the current behavior (even if you are not using worktrees\nat all).\n\n> @@ -1387,16 +1386,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> [...]\n\nSo if the earlier hunk covered this case, then what is this hunk for? :)\n\nIt looks like we call it from do_fetch() when update_head_ok isn't set.\nI'm not clear why we have two distinct code paths that check\nupdate_head_ok. It seems like this one is the one that _actually_ kicks\nin, because it's only later that we call update_local_ref(), several\nlayers down. If I put a BUG() in that one, it is never reached in the\ntest suite. Hmm.\n\nIt looks like this situation is quite old, and certainly it's nothing\nnew in your patch. So let's file it as a curiosity for now, and we can\ndeal with it separately (if at all).\n\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\nThis one does update the message, which is nice (and from my analysis\nabove, the message from the other hunk doesn't matter at all!). It's a\nbit more verbose if you're not using worktrees at all, though. I used to get:\n\n  $ git init\n  $ git commit --allow-empty -qm foo\n  $ git fetch . main:main\n  fatal: Refusing to fetch into current branch refs/heads/main of non-bare repository\n\nbut now get:\n\n  $ git fetch . main:main\n  fatal: Refusing to fetch into branch 'refs/heads/main' checked out at '/home/peff/tmp'\n\nIt's technically correct, but perhaps it would be nicer to keep the old\nmessage if the worktree we found is the current one.\n\n-Peff\n"},{"id":"440520","messageId":"991240f7-2a3e-8ae7-ae25-a1d9d96d55d4@mit.edu","threadId":"56847","inReplyTo":"YYTtdZblnHYgDgBq@coredump.intra.peff.net","subject":"Re: [PATCH v2] fetch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-05T10:01:39Z","receivedAt":"2021-11-05T10:01:56Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On 11/5/21 01:38, Jeff King wrote:\n> If we get the non-default worktree here, it might be nice to tell the\n> user which worktree has it checked out. Otherwise it may lead to\n> head-scratching as they peek at their current HEAD.\n\nI’d agree if this case were reachable, but as per below, it seems not to \nbe. So I didn’t bother figuring out how to shoehorn such a message into \nthe arguments expected by format_display().\n\n> I also notice that find_shared_symref() will catch cases where we're on\n> a detached HEAD, but it's because we're rebasing or bisecting on a given\n> branch. Arguably that's a sensible thing to do for this check, but it is\n> a change from the current behavior (even if you are not using worktrees\n> at all).\n\nThat’s a good observation. I agree it seems sensible, but should be \nnoted in the commit message.\n\n> So if the earlier hunk covered this case, then what is this hunk for? :)\n\nYeah, that duplication confused me too. The relevant call paths seem to be:\n\n1. do_fetch calls check_not_current_branch (later hunk);\n2. do_fetch calls fetch_and_consume_refs → store_updated_refs → \nupdate_local_ref (earlier hunk);\n3. do_fetch calls backfill_tags → fetch_and_consume_refs → \nstore_updated_refs → update_local_ref (earlier hunk);\n\nin that order. That suggests there should be no way to trip the earlier \nhunk’s check without first tripping the latter hunk’s check.\n\n>    $ git fetch . main:main\n>    fatal: Refusing to fetch into branch 'refs/heads/main' checked out at '/home/peff/tmp'\n> \n> It's technically correct, but perhaps it would be nicer to keep the old\n> message if the worktree we found is the current one.\n\nI note that this new message is more consistent with\n\n$ git branch -d main\nerror: Cannot delete branch 'main' checked out at '/home/anders/tmp'\n\n\nWhile looking for other related messages, I noticed another bug: \nreceive-pack doesn’t stop you from updating or deleting a branch checked \nout in a worktree for a bare repository.\n\n$ git init\n$ git commit --allow-empty -qm foo\n$ git clone --bare . bare.git\n$ git -C bare.git worktree add wt\n$ git -C bare.git/wt push .. :wt\n\nTo ..\n\n  - [deleted]         wt\n\n\nThis is_bare_repository() check in update() in builtin/receive-pack.c \nseems wrong:\n\nconst struct worktree *worktree = is_bare_repository() ? NULL : \nfind_shared_symref(\"HEAD\", name);\n\n\nAnders\n"},{"id":"440523","messageId":"YYUbQqyYQDD5QEAz@coredump.intra.peff.net","threadId":"56847","inReplyTo":"991240f7-2a3e-8ae7-ae25-a1d9d96d55d4@mit.edu","subject":"Re: [PATCH v2] fetch: Protect branches checked out in all worktrees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-05T11:53:38Z","receivedAt":"2021-11-05T11:53:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 05, 2021 at 03:01:39AM -0700, Anders Kaseorg wrote:\n\n> On 11/5/21 01:38, Jeff King wrote:\n> > If we get the non-default worktree here, it might be nice to tell the\n> > user which worktree has it checked out. Otherwise it may lead to\n> > head-scratching as they peek at their current HEAD.\n> \n> I’d agree if this case were reachable, but as per below, it seems not to be.\n> So I didn’t bother figuring out how to shoehorn such a message into the\n> arguments expected by format_display().\n\nI was thinking of just:\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a6549c2ab6..61c8fc9983 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -855,6 +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+\tconst struct worktree *wt;\n \tconst char *pretty_ref = prettify_refname(ref->name);\n \tint fast_forward = 0;\n \n@@ -869,14 +870,16 @@ static int update_local_ref(struct ref *ref,\n \t}\n \n \tif (!update_head_ok &&\n-\t    find_shared_symref(\"HEAD\", ref->name) &&\n+\t    (wt = find_shared_symref(\"HEAD\", ref->name)) &&\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       _(\"branch checked out in worktree\"),\n \t\t\t       remote, pretty_ref, summary_width);\n \t\treturn 1;\n \t}\n\nthough of course that does not mention the working tree we found. We'd\nprobably have to format the string into an extra strbuf for that. But I\nagree, that if we don't think this is reachable, it may not be worth\nworrying about. But see below, where I get off on a bit of tangent.\n\n> > So if the earlier hunk covered this case, then what is this hunk for? :)\n> \n> Yeah, that duplication confused me too. The relevant call paths seem to be:\n> \n> 1. do_fetch calls check_not_current_branch (later hunk);\n> 2. do_fetch calls fetch_and_consume_refs → store_updated_refs →\n> update_local_ref (earlier hunk);\n> 3. do_fetch calls backfill_tags → fetch_and_consume_refs →\n> store_updated_refs → update_local_ref (earlier hunk);\n> \n> in that order. That suggests there should be no way to trip the earlier\n> hunk’s check without first tripping the latter hunk’s check.\n\nThat matches what I found. I think that the check in update_local_ref()\nbecame obsolete when we added check_not_current_branch() in 8ee5d73137\n(Fix fetch/pull when run without --update-head-ok, 2008-10-13). That\ncommit confused me at first, because there's no mention of the existing\nmechanism at all in the commit message nor the mailing list thread.\n\nBut it was indeed broken back then, and I think is even broken now! The\nproblem is that it was comparing against current_branch->name, which is\nnot the full refname, and thus would never match. The check added in\n8ee5d73137 gets this right.\n\nYour patch also ends up fixing this as a side effect, because it\nswitches to using find_shared_symref(), which eliminates the buggy code.\n\nBut all of that means we could actually drop check_not_current_branch()\nin favor of the update_local_ref() check. Doing this:\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f7abbc31ff..c52c44684a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -869,7 +869,7 @@ static int update_local_ref(struct ref *ref,\n \t}\n \n \tif (current_branch &&\n-\t    !strcmp(ref->name, current_branch->name) &&\n+\t    !strcmp(ref->name, current_branch->refname) &&\n \t    !(update_head_ok || is_bare_repository()) &&\n \t    !is_null_oid(&ref->old_oid)) {\n \t\t/*\n@@ -1385,20 +1385,6 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,\n \treturn result;\n }\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-\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-}\n-\n static int truncate_fetch_head(void)\n {\n \tconst char *filename = git_path_fetch_head(the_repository);\n@@ -1587,8 +1573,6 @@ static int do_fetch(struct transport *transport,\n \n \tref_map = get_ref_map(transport->remote, remote_refs, rs,\n \t\t\t      tags, &autotags);\n-\tif (!update_head_ok)\n-\t\tcheck_not_current_branch(ref_map);\n \n \tif (tags == TAGS_DEFAULT && autotags)\n \t\ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, \"1\");\n\npasses the entire test suite except for one test which expects:\n\n  git fetch . side:main\n\nto fail. But that is only because \"side\" and \"main\" point to the same\ncommit, and thus the fetch is a noop. The code in update_local_ref()\ncovers that case before checking the HEAD case (which I would argue is\na completely reasonable outcome).\n\nThe reason I bring this up is that I think doing the check in\nupdate_local_ref() makes much more sense. We don't abort the whole\nfetch, but just treat it as a normal per-ref failure. That gives us the\nusual status-table output (I thought it might also avoid wasting some\nwork of actually fetching objects, but I think the current check kicks\nin before we actually fetch anything).\n\nOn the other hand, this is a rare-ish error, and nobody has cared much\nabout the distinction in the last 13 years. So it probably doesn't\nreally matter.\n\nNow getting back to your patch and the message produced in the\nupdate_local_ref() check. I am content to say: this code path is not\nreally reachable, but cleaning it up is out of scope, so let's leave it\nfor another day. But if we do eventually want to keep just the check in\nupdate_local_ref(), then we might want to improve the message now while\nwe're thinking about it.\n\n> > It's technically correct, but perhaps it would be nicer to keep the old\n> > message if the worktree we found is the current one.\n> \n> I note that this new message is more consistent with\n> \n> $ git branch -d main\n> error: Cannot delete branch 'main' checked out at '/home/anders/tmp'\n\nYeah, that may be a reason to keep things consistent. My main worry is\njust that somebody who is not using worktrees but has a long repo\npathname may find the message a bit more overwhelming. But if we want to\ndeal with that, we should probably do so everywhere.\n\n> While looking for other related messages, I noticed another bug:\n> receive-pack doesn’t stop you from updating or deleting a branch checked out\n> in a worktree for a bare repository.\n> \n> $ git init\n> $ git commit --allow-empty -qm foo\n> $ git clone --bare . bare.git\n> $ git -C bare.git worktree add wt\n> $ git -C bare.git/wt push .. :wt\n> \n> To ..\n> \n>  - [deleted]         wt\n> \n> \n> This is_bare_repository() check in update() in builtin/receive-pack.c seems\n> wrong:\n> \n> const struct worktree *worktree = is_bare_repository() ? NULL :\n> find_shared_symref(\"HEAD\", name);\n\nHmm. That is the same check that is on the fetch side, isn't it? In a\nbare repository, we will not do any of these current-branch checks. What\nis weird in your example is that you are adding a worktree to a bare\nrepository. Is that a supported workflow? Should that make it non-bare?\n\nI notice that there is a worktree->is_bare flag, and that\nfind_shared_symref() would not report such a bare entry to us. But I'm\nreally unclear how any of that is supposed to work (how do you have a\nbare worktree, and what does it mean?).\n\nI think that's all orthogonal to the main purpose of your patch here.\nYou may want to post about it separately with a different subject to get\nthe attention of folks who work on worktrees.\n\n-Peff\n"},{"id":"440540","messageId":"xmqqbl2yxxa0.fsf@gitster.g","threadId":"56847","inReplyTo":"YYUbQqyYQDD5QEAz@coredump.intra.peff.net","subject":"Re: [PATCH v2] fetch: Protect branches checked out in all worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-05T18:39:03Z","receivedAt":"2021-11-05T18:39:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But all of that means we could actually drop check_not_current_branch()\n> in favor of the update_local_ref() check. Doing this:\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index f7abbc31ff..c52c44684a 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -869,7 +869,7 @@ static int update_local_ref(struct ref *ref,\n>  \t}\n>  \n>  \tif (current_branch &&\n> -\t    !strcmp(ref->name, current_branch->name) &&\n> +\t    !strcmp(ref->name, current_branch->refname) &&\n>  \t    !(update_head_ok || is_bare_repository()) &&\n>  \t    !is_null_oid(&ref->old_oid)) {\n>  \t\t/*\n> @@ -1385,20 +1385,6 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,\n>  \treturn result;\n>  }\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> -\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> -}\n> -\n>  static int truncate_fetch_head(void)\n>  {\n>  \tconst char *filename = git_path_fetch_head(the_repository);\n> @@ -1587,8 +1573,6 @@ static int do_fetch(struct transport *transport,\n>  \n>  \tref_map = get_ref_map(transport->remote, remote_refs, rs,\n>  \t\t\t      tags, &autotags);\n> -\tif (!update_head_ok)\n> -\t\tcheck_not_current_branch(ref_map);\n>  \n>  \tif (tags == TAGS_DEFAULT && autotags)\n>  \t\ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, \"1\");\n>\n> passes the entire test suite except for one test which expects:\n>\n>   git fetch . side:main\n>\n> to fail. But that is only because \"side\" and \"main\" point to the same\n> commit, and thus the fetch is a noop. The code in update_local_ref()\n> covers that case before checking the HEAD case (which I would argue is\n> a completely reasonable outcome).\n\nSo we can lose redundant code that was added there only because the\noriginal safety was broken by actually fixing the original safety?\n\nThat is very nice.\n\n> The reason I bring this up is that I think doing the check in\n> update_local_ref() makes much more sense. We don't abort the whole\n> fetch, but just treat it as a normal per-ref failure. That gives us the\n> usual status-table output (I thought it might also avoid wasting some\n> work of actually fetching objects, but I think the current check kicks\n> in before we actually fetch anything).\n\nYes I agree with that reasoning.  \n\nThanks for digging this to the bottom, both of you.\n"},{"id":"440681","messageId":"2e925989-d435-0355-d26d-e79a0a99929f@mit.edu","threadId":"56847","inReplyTo":"YYUbQqyYQDD5QEAz@coredump.intra.peff.net","subject":"Re: [PATCH v2] fetch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-08T20:12:17Z","receivedAt":"2021-11-08T20:12:33Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On 11/5/21 04:53, Jeff King wrote:\n> +\t\t\t       wt->is_current ?\n> +\t\t\t       _(\"can't fetch in current branch\") :\n> +\t\t\t       _(\"branch checked out in worktree\"),\n\nSure, I’ll go ahead and add this.  We can make further improvements here \nif we proceed with removing the other error path, which seems to have \nenough complications that we should consider it after merging this bug fix.\n\n> Hmm. That is the same check that is on the fetch side, isn't it? In a\n> bare repository, we will not do any of these current-branch checks. What\n> is weird in your example is that you are adding a worktree to a bare\n> repository. Is that a supported workflow? Should that make it non-bare?\n> \n> I notice that there is a worktree->is_bare flag, and that\n> find_shared_symref() would not report such a bare entry to us. But I'm\n> really unclear how any of that is supposed to work (how do you have a\n> bare worktree, and what does it mean?).\n\nworktree->is_bare is only ever potentially set for the main working tree \n(.git/..).  Although a bare repository doesn’t have a main working tree, \nit can still have other working trees created with git worktree.  Making \na temporary working tree can be useful to quickly inspect or change a \nbare repository.  It’s also useful for sharing refs, remotes, and object \nstorage between several working trees where any of them might be created \nor deleted, and there isn’t one that should be considered primary.\n\nThe Zulip installer works this way to save some disk space and \nre-cloning time while allowing for simple rollbacks to a previous \ndeployment directory (https://github.com/zulip/zulip/pull/18487), which \nis how my team stumbled on the issue.\n\n> I think that's all orthogonal to the main purpose of your patch here.\n> You may want to post about it separately with a different subject to get\n> the attention of folks who work on worktrees.\n\nI expect this subject line will have already caught the attention of \nthose folks, but I’ll add a patch 2/2 with a more specific subject line \nfor this issue.\n\nAnders\n"}]}