{"thread":{"id":"56858","subject":"[PATCH v3 1/2] fetch: Protect branches checked out in all worktrees","startedAt":"2021-11-08T20:15:53Z","lastAt":"2021-11-09T05:48:23Z","messageCount":5,"participants":["Anders Kaseorg","Junio C Hamano","Bagas Sanjaya"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"440683","messageId":"alpine.DEB.2.21.999.2111081514460.100671@scrubbing-bubbles.mit.edu","threadId":"56858","inReplyTo":null,"subject":"[PATCH v3 1/2] fetch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-08T20:15:27Z","receivedAt":"2021-11-08T20:15:53Z","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 <andersk@mit.edu>\n---\n builtin/fetch.c       | 27 +++++++++++++--------------\n t/t5516-fetch-push.sh | 11 +++++++++++\n 2 files changed, 24 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f7abbc31ff..61c8fc9983 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,17 @@ 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    !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@@ -1387,16 +1389,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..4ef4ecbe71 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1771,4 +1771,15 @@ 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+\n test_done\n-- \n2.33.1\n\n"},{"id":"440696","messageId":"xmqqa6ie5k3u.fsf@gitster.g","threadId":"56858","inReplyTo":"alpine.DEB.2.21.999.2111081514460.100671@scrubbing-bubbles.mit.edu","subject":"Re: [PATCH v3 1/2] fetch: Protect branches checked out in all worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-08T23:00:05Z","receivedAt":"2021-11-08T23:00:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Kaseorg <andersk@mit.edu> writes:\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\nTrue.\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    !is_null_oid(&ref->old_oid)) {\n\nWe used to allow \"git fetch\" into a bare repository to update the\nbranch that happens to be pointed at by the HEAD symref.  The new\ncode still allow it, but the way it does so is subtle [*].\n\n    Side note: The new code only works because find-shared-symref\n    ignores a bare repository or a bare worktree.  I would not be\n    surprised if somebody starts arguing that the behaviour to\n    ignore bare worktrees is a bug in that function and may accept a\n    patch to correct it, and when I do so, I may not remember that\n    this new code depends on that \"bug\".\n\nI would sleep better if we were one bit more careful, perhaps like\nso:\n\n+\tif (!update_head_ok &&\n+\t    (wt = find_shared_symref(...)) &&\n+\t    !wt->is_bare &&\n\t    !is_null_oid(...)) {\n\nto make sure we do not rely on that particular aspect of how\nfind_shared_symref() works.  The function asks \"please find a\nworktree, if any, whose HEAD points at this ref\", and it feels\nunnatural for the answer to the question is affected by the\nbare-ness of the worktree.\n\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\nOK, the former is about this worktree, and the latter is about\nworktree somewhere else.  It may clarify if we phrased the latter a\nbit differently, e.g. \"checked out in another worktree\".  Once we\nsay \"check(ed) out\", we know we are talking about a branch, and\nformat_display() would be showing the name of the branch on the same\nline anyway, so we could save the 6 letter spaces and tell the user\nthat it is not happening here, but some other place.\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\nThis is because at this point in the test sequence, new-wt is the\ncurrent branch for the worktree we added in the test immediately\nbefore this one.  And we refuse unless update-head-ok is given.  OK.\n\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\nWhat is being tested here?  We created a bare clone bare.git and\nadded a worktree bare-wt to it.  And we try to fetch into that bare\nrepository, which would allow overwriting the branch pointed at by\nHEAD (which is new-wt) or any branch if there weren't a worktree\nthat has a working tree.  But because it has a working tree attached\nto it, namely, bare-wt, overwriting the current branch for that\nworktree is prevented.  Good.\n\n> +\tgit fetch -u cloned HEAD:new-wt &&\n> +\tgit -C bare.git fetch -u ../cloned HEAD:bare-wt\n\nThese are to ensure that overriding the safety still works fine.\nGood.\n\nI cannot shake the feeling that this single test step is testing way\ntoo many things and burden future developers who break one of the\nsteps to understand which step was broken, but these three are good\nthings to test.\n\nOverall, looks quite good.\n\nThanks.\n\n\n> +'\n> +\n>  test_done\n"},{"id":"440705","messageId":"8709db5c-8ac0-b327-09ba-087e392c297b@mit.edu","threadId":"56858","inReplyTo":"xmqqa6ie5k3u.fsf@gitster.g","subject":"Re: [PATCH v3 1/2] fetch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-08T23:31:35Z","receivedAt":"2021-11-08T23:31:51Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On 11/8/21 15:00, Junio C Hamano wrote:\n> I would sleep better if we were one bit more careful, perhaps like\n> so:\n> \n> +\tif (!update_head_ok &&\n> +\t    (wt = find_shared_symref(...)) &&\n> +\t    !wt->is_bare &&\n> \t    !is_null_oid(...)) {\n> \n> to make sure we do not rely on that particular aspect of how\n> find_shared_symref() works.  The function asks \"please find a\n> worktree, if any, whose HEAD points at this ref\", and it feels\n> unnatural for the answer to the question is affected by the\n> bare-ness of the worktree.\n\nNot unreasonable to be explicit at the call site.  Will update.\n\nI note that branch.c already relies on this aspect of find_shared_symref().\n\nAnd, now that I’m reading branch.c again, I see that it also has a \nworktree-awareness bug in its safety check, deserving of a third patch \nin this series.\n\n$ git init\n$ git commit --allow-empty -am foo\n$ git worktree add wt\n$ git branch -M wt main\n   # correctly disallowed\nfatal: Cannot force update the current branch.\n\n$ git branch -M wt  # incorrectly allowed\n$ git worktree list\n\n/tmp/test     6de7a5d [wt]\n\n/tmp/test/wt  6de7a5d [wt]\n\n\n> OK, the former is about this worktree, and the latter is about\n> worktree somewhere else.  It may clarify if we phrased the latter a\n> bit differently, e.g. \"checked out in another worktree\".\n\nAlright.  (Of course, as noted in previous discussion, this error path \nis currently reachable.)\n\n> I cannot shake the feeling that this single test step is testing way\n> too many things and burden future developers who break one of the\n> steps to understand which step was broken, but these three are good\n> things to test.\n\nI can split the bare repository part into a separate test_expect_success \nstep?  This duplicates some commands but maybe helps readability overall.\n\nAnders\n"},{"id":"440730","messageId":"a1acda2b-7cd0-0c1a-8c60-681d79418872@gmail.com","threadId":"56858","inReplyTo":"alpine.DEB.2.21.999.2111081514460.100671@scrubbing-bubbles.mit.edu","subject":"Re: [PATCH v3 1/2] fetch: Protect branches checked out in all worktrees","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-11-09T05:19:36Z","receivedAt":"2021-11-09T05:20:05Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 09/11/21 03.15, Anders Kaseorg wrote:\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\nWe can say \"we'll also refuse fetching when we're on a detached HEAD (for \nexample if we're rebasing on the branch in question or bisecting).\"\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"440732","messageId":"503ee252-c733-ce8e-14ed-d4d1909682b8@mit.edu","threadId":"56858","inReplyTo":"a1acda2b-7cd0-0c1a-8c60-681d79418872@gmail.com","subject":"Re: [PATCH v3 1/2] fetch: Protect branches checked out in all worktrees","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2021-11-09T05:48:05Z","receivedAt":"2021-11-09T05:48:23Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On 11/8/21 21:19, Bagas Sanjaya wrote:\n> On 09/11/21 03.15, Anders Kaseorg wrote:\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> We can say \"we'll also refuse fetching when we're on a detached HEAD \n> (for example if we're rebasing on the branch in question or bisecting).\"\n\nNo, I meant what I wrote.  We don’t refuse the fetch if we’re on a \ndetached HEAD for some other reason.  That would be quite unhelpful.\n\nAnders\n"}]}