{"thread":{"id":"57973","subject":"[PATCH 0/4] Create branch_checked_out() helper","startedAt":"2022-06-08T20:09:13Z","lastAt":"2022-06-19T13:58:50Z","messageCount":28,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee","Ævar Arnfjörð Bjarmason","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"456887","messageId":"pull.1254.git.1654718942.gitgitgadget@gmail.com","threadId":"57973","inReplyTo":null,"subject":"[PATCH 0/4] Create branch_checked_out() helper","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-08T20:08:58Z","receivedAt":"2022-06-08T20:09:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This is a replacement for some patches from v2 of my 'git rebase\n--update-refs' topic [1]. After some feedback from Philip, I've decided to\npull that topic while I rework how I track the refs to rewrite [2]. This\nseries moves forward with the branch_checked_out() helper that was a bit\nmore complicated than expected at first glance. This series is a culmination\nof the discussion started by Junio at [3].\n\n[1]\nhttps://lore.kernel.org/git/pull.1247.v2.git.1654634569.gitgitgadget@gmail.com\n[2]\nhttps://lore.kernel.org/git/34264915-8a37-62db-f954-0b5297639b34@github.com/\n[3] https://lore.kernel.org/git/xmqqr140t9am.fsf@gitster.g/\n\nHere is the patch outline:\n\n 1. Add a basic form of branch_checked_out() that only looks at the HEAD of\n each worktree. Since this doesn't look at BISECT_HEAD or REBASE_HEAD,\n the helper isn't linked to any consumer in this patch. A test script is\n added that verifies the current behavior checks that are implemented,\n even if they are not connected yet.\n 2. Make branch_checked_out() actually look at BISECT_HEAD and REBASE_HEAD.\n Add tests for those cases, which was previously untested in the Git test\n suite. Use branch_checked_out() in 'git branch -f' to demonstrate this\n helper works as expected.\n 3. Use branch_checked_out() in 'git fetch' when checking refs that would be\n updated by the refspec. Add tests for that scenario, too.\n 4. Use branch_checked_out() in 'git branch -D' to prevent deleting refs\n from under an existing checkout. The commit message here describes some\n barriers to removing other uses of find_shared_symref() that could be\n good investigations for later.\n\nThanks, -Stolee\n\nDerrick Stolee (4):\n branch: add branch_checked_out() helper\n branch: check for bisects and rebases\n fetch: use new branch_checked_out() and add tests\n branch: use branch_checked_out() when deleting refs\n\n branch.c | 77 +++++++++++++++++++++++++++++++++----\n branch.h | 8 ++++\n builtin/branch.c | 8 ++--\n builtin/fetch.c | 25 ++++++------\n t/t2407-worktree-heads.sh | 81 +++++++++++++++++++++++++++++++++++++++\n 5 files changed, 173 insertions(+), 26 deletions(-)\n create mode 100755 t/t2407-worktree-heads.sh\n\n\nbase-commit: 2668e3608e47494f2f10ef2b6e69f08a84816bcb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1254%2Fderrickstolee%2Fbranch-checked-out-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1254/derrickstolee/branch-checked-out-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1254\n-- \ngitgitgadget\n"},{"id":"456888","messageId":"dbb7eae390c90d4b710f48d8875bd7db0409aea3.1654718942.git.gitgitgadget@gmail.com","threadId":"57973","inReplyTo":"pull.1254.git.1654718942.gitgitgadget@gmail.com","subject":"[PATCH 1/4] branch: add branch_checked_out() helper","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-08T20:08:59Z","receivedAt":"2022-06-08T20:09:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe validate_new_branchname() method contains a check to see if a branch\nis checked out in any non-bare worktree. This is intended to prevent a\nforce push that will mess up an existing checkout. This helper is not\nsuitable to performing just that check, because the method will die()\nwhen the branch is checked out instead of returning an error code.\n\nCreate a new branch_checked_out() helper that performs the most basic\nform of this check. To ensure we can call branch_checked_out() in a loop\nwith good performance, do a single preparation step that iterates over\nall worktrees and stores their current HEAD branches in a strmap. The\nbranch_checked_out() helper can then discover these branches using a\nhash lookup.\n\nThis helper is currently missing some key functionality. Namely: it\ndoesn't look for active rebases or bisects which mean that the branch is\n\"checked out\" even though HEAD doesn't point to that ref. This\nfunctionality will be added in a coming change.\n\nWe could use branch_checked_out() in validate_new_branchname(), but this\nmissing functionality would be a regression. However, we have no tests\nthat cover this case!\n\nAdd a new test script that will be expanded with these cross-worktree\nref updates. The current tests would still pass if we refactored\nvalidate_new_branchname() to use this version of branch_checked_out().\nThe next change will fix that functionality and add the proper test\ncoverage.\n\nSigned-off-by: Derrick Stolee \n---\n branch.c | 42 +++++++++++++++++++++++++++++++++++++++\n branch.h | 8 ++++++++\n t/t2407-worktree-heads.sh | 24 ++++++++++++++++++++++\n 3 files changed, 74 insertions(+)\n create mode 100755 t/t2407-worktree-heads.sh\n\ndiff --git a/branch.c b/branch.c\nindex 2d6569b0c62..061a11f3415 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -10,6 +10,7 @@\n #include \"worktree.h\"\n #include \"submodule-config.h\"\n #include \"run-command.h\"\n+#include \"strmap.h\"\n \n struct tracking {\n \tstruct refspec_item spec;\n@@ -369,6 +370,47 @@ int validate_branchname(const char *name, struct strbuf *ref)\n \treturn ref_exists(ref->buf);\n }\n \n+static int initialized_checked_out_branches;\n+static struct strmap current_checked_out_branches = STRMAP_INIT;\n+\n+static void prepare_checked_out_branches(void)\n+{\n+\tint i = 0;\n+\tstruct worktree **worktrees;\n+\n+\tif (initialized_checked_out_branches)\n+\t\treturn;\n+\tinitialized_checked_out_branches = 1;\n+\n+\tworktrees = get_worktrees();\n+\n+\twhile (worktrees[i]) {\n+\t\tstruct worktree *wt = worktrees[i++];\n+\n+\t\tif (wt->is_bare)\n+\t\t\tcontinue;\n+\n+\t\tif (wt->head_ref)\n+\t\t\tstrmap_put(¤t_checked_out_branches,\n+\t\t\t\t wt->head_ref,\n+\t\t\t\t xstrdup(wt->path));\n+\t}\n+\n+\tfree_worktrees(worktrees);\n+}\n+\n+int branch_checked_out(const char *refname, char **path)\n+{\n+\tconst char *path_in_set;\n+\tprepare_checked_out_branches();\n+\n+\tpath_in_set = strmap_get(¤t_checked_out_branches, refname);\n+\tif (path_in_set && path)\n+\t\t*path = xstrdup(path_in_set);\n+\n+\treturn !!path_in_set;\n+}\n+\n /*\n * Check if a branch 'name' can be created as a new branch; die otherwise.\n * 'force' can be used when it is OK for the named branch already exists.\ndiff --git a/branch.h b/branch.h\nindex 560b6b96a8f..5ea93d217b1 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -101,6 +101,14 @@ void create_branches_recursively(struct repository *r, const char *name,\n \t\t\t\t const char *tracking_name, int force,\n \t\t\t\t int reflog, int quiet, enum branch_track track,\n \t\t\t\t int dry_run);\n+\n+/*\n+ * Returns true if the branch at 'refname' is checked out at any\n+ * non-bare worktree. The path of the worktree is stored in the\n+ * given 'path', if provided.\n+ */\n+int branch_checked_out(const char *refname, char **path);\n+\n /*\n * Check if 'name' can be a valid name for a branch; die otherwise.\n * Return 1 if the named branch already exists; return 0 otherwise.\ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nnew file mode 100755\nindex 00000000000..dd905dc1a5c\n--- /dev/null\n+++ b/t/t2407-worktree-heads.sh\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+\n+test_description='test operations trying to overwrite refs at worktree HEAD'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tfor i in 1 2 3 4\n+\tdo\n+\t\ttest_commit $i &&\n+\t\tgit branch wt-$i &&\n+\t\tgit worktree add wt-$i wt-$i || return 1\n+\tdone\n+'\n+\n+test_expect_success 'refuse to overwrite: checked out in worktree' '\n+\tfor i in 1 2 3 4\n+\tdo\n+\t\ttest_must_fail git branch -f wt-$i HEAD 2>err\n+\t\tgrep \"cannot force update the branch\" err || return 1\n+\tdone\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"456889","messageId":"18bad9b0c496fc0ceab1e567aee83f2160ae5d75.1654718942.git.gitgitgadget@gmail.com","threadId":"57973","inReplyTo":"pull.1254.git.1654718942.gitgitgadget@gmail.com","subject":"[PATCH 2/4] branch: check for bisects and rebases","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-08T20:09:00Z","receivedAt":"2022-06-08T20:09:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe branch_checked_out() helper was added by the previous change, but it\nused an over-simplified view to check if a branch is checked out. It\nonly focused on the HEAD symref, but ignored whether a bisect or rebase\nwas happening.\n\nTeach branch_checked_out() to check for these things, and also add tests\nto ensure that we do not lose this functionality in the future.\n\nNow that this test coverage exists, we can safely refactor\nvalidate_new_branchname() to use branch_checked_out().\n\nNote that we need to prepend \"refs/heads/\" to the 'state.branch' after\ncalling wt_status_check_*(). We also need to duplicate wt->path so the\nvalue is not freed at the end of the call.\n\nSigned-off-by: Derrick Stolee \n---\n branch.c | 35 +++++++++++++++++++++++++++--------\n t/t2407-worktree-heads.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 56 insertions(+), 8 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 061a11f3415..c0fe6ea0b65 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -385,6 +385,7 @@ static void prepare_checked_out_branches(void)\n \tworktrees = get_worktrees();\n \n \twhile (worktrees[i]) {\n+\t\tstruct wt_status_state state = { 0 };\n \t\tstruct worktree *wt = worktrees[i++];\n \n \t\tif (wt->is_bare)\n@@ -394,6 +395,29 @@ static void prepare_checked_out_branches(void)\n \t\t\tstrmap_put(¤t_checked_out_branches,\n \t\t\t\t wt->head_ref,\n \t\t\t\t xstrdup(wt->path));\n+\n+\t\tif (wt_status_check_rebase(wt, &state) &&\n+\t\t (state.rebase_in_progress || state.rebase_interactive_in_progress) &&\n+\t\t state.branch) {\n+\t\t\tstruct strbuf ref = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&ref, \"refs/heads/%s\", state.branch);\n+\t\t\tstrmap_put(¤t_checked_out_branches,\n+\t\t\t\t ref.buf,\n+\t\t\t\t xstrdup(wt->path));\n+\t\t\tstrbuf_release(&ref);\n+\t\t}\n+\t\twt_status_state_free_buffers(&state);\n+\n+\t\tif (wt_status_check_bisect(wt, &state) &&\n+\t\t state.branch) {\n+\t\t\tstruct strbuf ref = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&ref, \"refs/heads/%s\", state.branch);\n+\t\t\tstrmap_put(¤t_checked_out_branches,\n+\t\t\t\t ref.buf,\n+\t\t\t\t xstrdup(wt->path));\n+\t\t\tstrbuf_release(&ref);\n+\t\t}\n+\t\twt_status_state_free_buffers(&state);\n \t}\n \n \tfree_worktrees(worktrees);\n@@ -419,9 +443,7 @@ int branch_checked_out(const char *refname, char **path)\n */\n int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n {\n-\tstruct worktree **worktrees;\n-\tconst struct worktree *wt;\n-\n+\tchar *path;\n \tif (!validate_branchname(name, ref))\n \t\treturn 0;\n \n@@ -429,13 +451,10 @@ 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-\tworktrees = get_worktrees();\n-\twt = find_shared_symref(worktrees, \"HEAD\", ref->buf);\n-\tif (wt && !wt->is_bare)\n+\tif (branch_checked_out(ref->buf, &path))\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-\tfree_worktrees(worktrees);\n+\t\t ref->buf + strlen(\"refs/heads/\"), path);\n \n \treturn 1;\n }\ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nindex dd905dc1a5c..12faca7f655 100755\n--- a/t/t2407-worktree-heads.sh\n+++ b/t/t2407-worktree-heads.sh\n@@ -21,4 +21,33 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '\n \tdone\n '\n \n+test_expect_success 'refuse to overwrite: worktree in bisect' '\n+\ttest_when_finished test_might_fail git -C wt-4 bisect reset &&\n+\n+\t(\n+\t\tgit -C wt-4 bisect start &&\n+\t\tgit -C wt-4 bisect bad HEAD &&\n+\t\tgit -C wt-4 bisect good HEAD~3\n+\t) &&\n+\n+\ttest_must_fail git branch -f wt-4 HEAD 2>err &&\n+\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err\n+'\n+\n+. \"$TEST_DIRECTORY\"/lib-rebase.sh\n+\n+test_expect_success 'refuse to overwrite: worktree in rebase' '\n+\ttest_when_finished test_might_fail git -C wt-4 rebase --abort &&\n+\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_LINES=\"edit 1 2 3\" \\\n+\t\t\tgit -C wt-4 rebase -i HEAD~3 >rebase &&\n+\t\tgit -C wt-4 status\n+\t) &&\n+\n+\ttest_must_fail git branch -f wt-4 HEAD 2>err &&\n+\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"456890","messageId":"4540dbeed385341f8c5b45134e1a65dc48c75b0c.1654718942.git.gitgitgadget@gmail.com","threadId":"57973","inReplyTo":"pull.1254.git.1654718942.gitgitgadget@gmail.com","subject":"[PATCH 3/4] fetch: use new branch_checked_out() and add tests","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-08T20:09:01Z","receivedAt":"2022-06-08T20:09:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen fetching refs from a remote, it is possible that the refspec will\ncause use to overwrite a ref that is checked out in a worktree. The\nexisting logic in builtin/fetch.c uses a possibly-slow mechanism. Update\nthose sections to use the new, more efficient branch_checked_out()\nhelper.\n\nThese uses were not previously tested, so add a test case that can be\nused for these kinds of collisions. There is only one test now, but more\ntests will be added as other consumers of branch_checked_out() are\nadded.\n\nNote that there are two uses in builtin/fetch.c, but only one of the\nmessages is tested. This is because the tested check is run before\ncompleting the fetch, and the untested check is not reachable without\nconcurrent updates to the filesystem. Thus, it is beneficial to keep\nthat extra check for the sake of defense-in-depth. However, we should\nnot attempt to test the check, as the effort required is too\ncomplicated to be worth the effort.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/fetch.c | 25 +++++++++++--------------\n t/t2407-worktree-heads.sh | 29 +++++++++++++++++++++++++++--\n 2 files changed, 38 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ac29c2b1ae3..1ba56240312 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -885,7 +885,7 @@ static int update_local_ref(struct ref *ref,\n \t\t\t struct worktree **worktrees)\n {\n \tstruct commit *current = NULL, *updated;\n-\tconst struct worktree *wt;\n+\tchar *path = NULL;\n \tconst char *pretty_ref = prettify_refname(ref->name);\n \tint fast_forward = 0;\n \n@@ -900,17 +900,17 @@ static int update_local_ref(struct ref *ref,\n \t}\n \n \tif (!update_head_ok &&\n-\t (wt = find_shared_symref(worktrees, \"HEAD\", ref->name)) &&\n-\t !wt->is_bare && !is_null_oid(&ref->old_oid)) {\n+\t !is_null_oid(&ref->old_oid) &&\n+\t branch_checked_out(ref->name, &path)) {\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 wt->is_current ?\n-\t\t\t\t _(\"can't fetch in current branch\") :\n-\t\t\t\t _(\"checked out in another worktree\"),\n+\t\t\t path ? _(\"can't fetch in current branch\") :\n+\t\t\t\t _(\"checked out in another worktree\"),\n \t\t\t remote, pretty_ref, summary_width);\n+\t\tfree(path);\n \t\treturn 1;\n \t}\n \n@@ -1434,19 +1434,16 @@ cleanup:\n \treturn result;\n }\n \n-static void check_not_current_branch(struct ref *ref_map,\n-\t\t\t\t struct worktree **worktrees)\n+static void check_not_current_branch(struct ref *ref_map)\n {\n-\tconst struct worktree *wt;\n+\tchar *path;\n \tfor (; ref_map; ref_map = ref_map->next)\n \t\tif (ref_map->peer_ref &&\n \t\t starts_with(ref_map->peer_ref->name, \"refs/heads/\") &&\n-\t\t (wt = find_shared_symref(worktrees, \"HEAD\",\n-\t\t\t\t\t ref_map->peer_ref->name)) &&\n-\t\t !wt->is_bare)\n+\t\t branch_checked_out(ref_map->peer_ref->name, &path))\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+\t\t\t ref_map->peer_ref->name, path);\n }\n \n static int truncate_fetch_head(void)\n@@ -1650,7 +1647,7 @@ static int do_fetch(struct transport *transport,\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, worktrees);\n+\t\tcheck_not_current_branch(ref_map);\n \n \tretcode = open_fetch_head(&fetch_head);\n \tif (retcode)\ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nindex 12faca7f655..f3f8b0b2b79 100755\n--- a/t/t2407-worktree-heads.sh\n+++ b/t/t2407-worktree-heads.sh\n@@ -10,6 +10,15 @@ test_expect_success 'setup' '\n \t\ttest_commit $i &&\n \t\tgit branch wt-$i &&\n \t\tgit worktree add wt-$i wt-$i || return 1\n+\tdone &&\n+\n+\t# Create a server that updates each branch by one commit\n+\tgit clone . server &&\n+\tgit remote add server ./server &&\n+\tfor i in 1 2 3 4\n+\tdo\n+\t\tgit -C server checkout wt-$i &&\n+\t\ttest_commit -C server A-$i || return 1\n \tdone\n '\n \n@@ -21,6 +30,16 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '\n \tdone\n '\n \n+test_expect_success 'refuse to overwrite during fetch' '\n+\ttest_must_fail git fetch server +refs/heads/wt-3:refs/heads/wt-3 2>err &&\n+\tgrep \"refusing to fetch into branch '\\''refs/heads/wt-3'\\''\" err &&\n+\n+\t# General fetch into refs/heads/ will fail on first ref,\n+\t# so use a generic error message check.\n+\ttest_must_fail git fetch server +refs/heads/*:refs/heads/* 2>err &&\n+\tgrep \"refusing to fetch into branch\" err\n+'\n+\n test_expect_success 'refuse to overwrite: worktree in bisect' '\n \ttest_when_finished test_might_fail git -C wt-4 bisect reset &&\n \n@@ -31,7 +50,10 @@ test_expect_success 'refuse to overwrite: worktree in bisect' '\n \t) &&\n \n \ttest_must_fail git branch -f wt-4 HEAD 2>err &&\n-\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err\n+\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err &&\n+\n+\ttest_must_fail git fetch server +refs/heads/wt-4:refs/heads/wt-4 2>err &&\n+\tgrep \"refusing to fetch into branch '\\''refs/heads/wt-4'\\''\" err\n '\n \n . \"$TEST_DIRECTORY\"/lib-rebase.sh\n@@ -47,7 +69,10 @@ test_expect_success 'refuse to overwrite: worktree in rebase' '\n \t) &&\n \n \ttest_must_fail git branch -f wt-4 HEAD 2>err &&\n-\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err\n+\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err &&\n+\n+\ttest_must_fail git fetch server +refs/heads/wt-4:refs/heads/wt-4 2>err &&\n+\tgrep \"refusing to fetch into branch '\\''refs/heads/wt-4'\\''\" err\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"456891","messageId":"af645b43032e8b138da3aae1732047208b3b2890.1654718942.git.gitgitgadget@gmail.com","threadId":"57973","inReplyTo":"pull.1254.git.1654718942.gitgitgadget@gmail.com","subject":"[PATCH 4/4] branch: use branch_checked_out() when deleting refs","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-08T20:09:02Z","receivedAt":"2022-06-08T20:09:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThis is the last current use of find_shared_symref() that can easily be\nreplaced by branch_checked_out(). The benefit of this switch is that the\ncode is a bit simpler, but also it is faster on repeated calls.\n\nThe remaining uses of find_shared_symref() are non-trivial to remove, so\nwe probably should not continue in that direction:\n\n* builtin/notes.c uses find_shared_symref() with \"NOTES_MERGE_REF\"\n instead of \"HEAD\", so it doesn't have an immediate analogue with\n branch_checked_out(). Perhaps we should consider extending it to\n include that symref in addition to HEAD, BISECT_HEAD, and\n REBASE_HEAD.\n\n* receive-pack.c checks to see if a worktree has a checkout for the ref\n that is being updated. The tricky part is that it can actually decide\n to update the worktree directly instead of just skipping the update.\n This all depends on the receive.denyCurrentBranch config option. The\n implementation currenty cares about receiving the worktree in the\n result, so the current branch_checked_out() prototype is insufficient\n currently. This is something to investigate later, though, since a\n large number of refs could be updated at the same time and using the\n strmap implementation of branch_checked_out() could be beneficial.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/branch.c | 8 ++++----\n t/t2407-worktree-heads.sh | 5 ++++-\n 2 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 5d00d0b8d32..8e11e433840 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -253,12 +253,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tname = mkpathdup(fmt, bname.buf);\n \n \t\tif (kinds == FILTER_REFS_BRANCHES) {\n-\t\t\tconst struct worktree *wt =\n-\t\t\t\tfind_shared_symref(worktrees, \"HEAD\", name);\n-\t\t\tif (wt) {\n+\t\t\tchar *path;\n+\t\t\tif (branch_checked_out(name, &path)) {\n \t\t\t\terror(_(\"Cannot delete branch '%s' \"\n \t\t\t\t\t\"checked out at '%s'\"),\n-\t\t\t\t bname.buf, wt->path);\n+\t\t\t\t bname.buf, path);\n+\t\t\t\tfree(path);\n \t\t\t\tret = 1;\n \t\t\t\tcontinue;\n \t\t\t}\ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nindex f3f8b0b2b79..6dcc0d39a2d 100755\n--- a/t/t2407-worktree-heads.sh\n+++ b/t/t2407-worktree-heads.sh\n@@ -26,7 +26,10 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '\n \tfor i in 1 2 3 4\n \tdo\n \t\ttest_must_fail git branch -f wt-$i HEAD 2>err\n-\t\tgrep \"cannot force update the branch\" err || return 1\n+\t\tgrep \"cannot force update the branch\" err &&\n+\n+\t\ttest_must_fail git branch -D wt-$i 2>err\n+\t\tgrep \"Cannot delete branch\" err || return 1\n \tdone\n '\n \n-- \ngitgitgadget\n"},{"id":"456901","messageId":"xmqqsfoeq0co.fsf@gitster.g","threadId":"57973","inReplyTo":"18bad9b0c496fc0ceab1e567aee83f2160ae5d75.1654718942.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] branch: check for bisects and rebases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-08T22:03:35Z","receivedAt":"2022-06-08T22:03:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\n> index dd905dc1a5c..12faca7f655 100755\n> --- a/t/t2407-worktree-heads.sh\n> +++ b/t/t2407-worktree-heads.sh\n> @@ -21,4 +21,33 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '\n> \tdone\n> '\n> \n> +test_expect_success 'refuse to overwrite: worktree in bisect' '\n> +\ttest_when_finished test_might_fail git -C wt-4 bisect reset &&\n> +\n> +\t(\n> +\t\tgit -C wt-4 bisect start &&\n> +\t\tgit -C wt-4 bisect bad HEAD &&\n> +\t\tgit -C wt-4 bisect good HEAD~3\n> +\t) &&\n> +\n> +\ttest_must_fail git branch -f wt-4 HEAD 2>err &&\n> +\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err\n> +'\n> +\n> +. \"$TEST_DIRECTORY\"/lib-rebase.sh\n> +\n> +test_expect_success 'refuse to overwrite: worktree in rebase' '\n> +\ttest_when_finished test_might_fail git -C wt-4 rebase --abort &&\n> +\n> +\t(\n> +\t\tset_fake_editor &&\n> +\t\tFAKE_LINES=\"edit 1 2 3\" \\\n> +\t\t\tgit -C wt-4 rebase -i HEAD~3 >rebase &&\n> +\t\tgit -C wt-4 status\n> +\t) &&\n> +\n> +\ttest_must_fail git branch -f wt-4 HEAD 2>err &&\n> +\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err\n> +'\n> +\n> test_done\n\nNice to see that each additional feature corresponds to each\nadditional test.\n\nThanks.\n"},{"id":"456968","messageId":"xmqq8rq5iel7.fsf@gitster.g","threadId":"57973","inReplyTo":"dbb7eae390c90d4b710f48d8875bd7db0409aea3.1654718942.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] branch: add branch_checked_out() helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-09T23:47:48Z","receivedAt":"2022-06-09T23:47:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> +static void prepare_checked_out_branches(void)\n> +{\n> +\tint i = 0;\n> +\tstruct worktree **worktrees;\n> +\n> +\tif (initialized_checked_out_branches)\n> +\t\treturn;\n> +\tinitialized_checked_out_branches = 1;\n> +\n> +\tworktrees = get_worktrees();\n> +\n> +\twhile (worktrees[i]) {\n> +\t\tstruct worktree *wt = worktrees[i++];\n> +\n> +\t\tif (wt->is_bare)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (wt->head_ref)\n> +\t\t\tstrmap_put(¤t_checked_out_branches,\n> +\t\t\t\t wt->head_ref,\n> +\t\t\t\t xstrdup(wt->path));\n> +\t}\n\nIf two worktrees have by accident or by bug checked out the same\nbranch, this strmap_put() will leak, as it overwrites the path to\nthe worktree we found earlier with the branch checked out with the\npath to the worktree we just discovered with the same branch checked\nout. We could easily work it around by immediately freeing what\ncomes back from strmap_put(), presumably. The resulting code\n\n\t\tif (wt->head_ref)\n\t\t\tfree(strmap_put(...));\n\nmay look strange, though, especially because we will see more\ninstances of strmap_put() in this loop in later steps.\n"},{"id":"457104","messageId":"6cd7db33-6ab5-9843-4483-4cce9835b177@github.com","threadId":"57973","inReplyTo":"pull.1254.git.1654718942.gitgitgadget@gmail.com","subject":"[PATCH 5/5] branch: fix branch_checked_out() leaks","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-13T14:59:58Z","receivedAt":"2022-06-13T18:40:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/8/2022 4:08 PM, Derrick Stolee via GitGitGadget wrote:\n> This is a replacement for some patches from v2 of my 'git rebase\n> --update-refs' topic [1]. After some feedback from Philip, I've decided to\n> pull that topic while I rework how I track the refs to rewrite [2]. This\n> series moves forward with the branch_checked_out() helper that was a bit\n> more complicated than expected at first glance. This series is a culmination\n> of the discussion started by Junio at [3].\n> \n\nJunio pointed out that patch 1 introduced a memory leak when a ref\nis checked out in multiple places. Here is a patch to fix that\nscenario. It applies cleanly on top of patch 4, so I include it as\na new \"patch 5\". I will include it in any v2 of the full series, if\nneeded.\n\nThanks,\n-Stolee\n\n---- >8 ----\n\nFrom c3842b36ebb4053ac49b0306154b840431f9bf6f Mon Sep 17 00:00:00 2001\nFrom: Derrick Stolee \nDate: Mon, 13 Jun 2022 10:33:20 -0400\nSubject: [PATCH 5/5] branch: fix branch_checked_out() leaks\n\nThe branch_checked_out() method populates a strmap linking a refname to\na worktree that has that branch checked out. While unlikely, it is\npossible that a bug or filesystem manipulation could create a scenario\nwhere the same ref is checked out in multiple places. Further, there are\nsome states in an interactive rebase where HEAD and REBASE_HEAD point to\nthe same ref, leading to multiple insertions into the strmap. In either\ncase, the strmap_put() method returns the old value which is leaked.\n\nUpdate branch_checked_out() to consume that pointer and free it.\n\nAdd a test in t2407 that checks this erroneous case. The test \"checks\nitself\" by first confirming that the filesystem manipulations it makes\ntrigger the branch_checked_out() logic, and then sets up similar\nmanipulations to make it look like there are multiple worktrees pointing\nto the same ref.\n\nWhile TEST_PASSES_SANITIZE_LEAK would be helpful to demonstrate the\nleakage and prevent it in the future, t2407 uses helpers such as 'git\nclone' that cause the test to fail under that mode.\n\nSigned-off-by: Derrick Stolee \n---\n branch.c | 25 +++++++++++++++----------\n t/t2407-worktree-heads.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 54 insertions(+), 10 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex c0fe6ea0b65..390c092a18f 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -385,25 +385,29 @@ static void prepare_checked_out_branches(void)\n \tworktrees = get_worktrees();\n \n \twhile (worktrees[i]) {\n+\t\tchar *old;\n \t\tstruct wt_status_state state = { 0 };\n \t\tstruct worktree *wt = worktrees[i++];\n \n \t\tif (wt->is_bare)\n \t\t\tcontinue;\n \n-\t\tif (wt->head_ref)\n-\t\t\tstrmap_put(¤t_checked_out_branches,\n-\t\t\t\t wt->head_ref,\n-\t\t\t\t xstrdup(wt->path));\n+\t\tif (wt->head_ref) {\n+\t\t\told = strmap_put(¤t_checked_out_branches,\n+\t\t\t\t\t wt->head_ref,\n+\t\t\t\t\t xstrdup(wt->path));\n+\t\t\tfree(old);\n+\t\t}\n \n \t\tif (wt_status_check_rebase(wt, &state) &&\n \t\t (state.rebase_in_progress || state.rebase_interactive_in_progress) &&\n \t\t state.branch) {\n \t\t\tstruct strbuf ref = STRBUF_INIT;\n \t\t\tstrbuf_addf(&ref, \"refs/heads/%s\", state.branch);\n-\t\t\tstrmap_put(¤t_checked_out_branches,\n-\t\t\t\t ref.buf,\n-\t\t\t\t xstrdup(wt->path));\n+\t\t\told = strmap_put(¤t_checked_out_branches,\n+\t\t\t\t\t ref.buf,\n+\t\t\t\t\t xstrdup(wt->path));\n+\t\t\tfree(old);\n \t\t\tstrbuf_release(&ref);\n \t\t}\n \t\twt_status_state_free_buffers(&state);\n@@ -412,9 +416,10 @@ static void prepare_checked_out_branches(void)\n \t\t state.branch) {\n \t\t\tstruct strbuf ref = STRBUF_INIT;\n \t\t\tstrbuf_addf(&ref, \"refs/heads/%s\", state.branch);\n-\t\t\tstrmap_put(¤t_checked_out_branches,\n-\t\t\t\t ref.buf,\n-\t\t\t\t xstrdup(wt->path));\n+\t\t\told = strmap_put(¤t_checked_out_branches,\n+\t\t\t\t\t ref.buf,\n+\t\t\t\t\t xstrdup(wt->path));\n+\t\t\tfree(old);\n \t\t\tstrbuf_release(&ref);\n \t\t}\n \t\twt_status_state_free_buffers(&state);\ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nindex 6dcc0d39a2d..0760595337b 100755\n--- a/t/t2407-worktree-heads.sh\n+++ b/t/t2407-worktree-heads.sh\n@@ -78,4 +78,43 @@ test_expect_success 'refuse to overwrite: worktree in rebase' '\n \tgrep \"refusing to fetch into branch '\\''refs/heads/wt-4'\\''\" err\n '\n \n+test_expect_success 'refuse to overwrite when in error states' '\n+\ttest_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&\n+\ttest_when_finished rm -rf .git/worktrees/wt-*/BISECT_* &&\n+\n+\tgit branch -f fake1 &&\n+\tmkdir -p .git/worktrees/wt-3/rebase-merge &&\n+\ttouch .git/worktrees/wt-3/rebase-merge/interactive &&\n+\techo refs/heads/fake1 >.git/worktrees/wt-3/rebase-merge/head-name &&\n+\techo refs/heads/fake2 >.git/worktrees/wt-3/rebase-merge/onto &&\n+\n+\tgit branch -f fake2 &&\n+\ttouch .git/worktrees/wt-4/BISECT_LOG &&\n+\techo refs/heads/fake2 >.git/worktrees/wt-4/BISECT_START &&\n+\n+\t# First, ensure we prevent writing when only one reason to fail.\n+\tfor i in 1 2\n+\tdo\n+\t\ttest_must_fail git branch -f fake$i HEAD 2>err &&\n+\t\tgrep \"cannot force update the branch '\\''fake$i'\\'' checked out at\" err ||\n+\t\t\treturn 1\n+\tdone &&\n+\n+\t# Second, set up duplicate values.\n+\tmkdir -p .git/worktrees/wt-4/rebase-merge &&\n+\ttouch .git/worktrees/wt-4/rebase-merge/interactive &&\n+\techo refs/heads/fake2 >.git/worktrees/wt-4/rebase-merge/head-name &&\n+\techo refs/heads/fake1 >.git/worktrees/wt-4/rebase-merge/onto &&\n+\n+\ttouch .git/worktrees/wt-1/BISECT_LOG &&\n+\techo refs/heads/fake1 >.git/worktrees/wt-1/BISECT_START &&\n+\n+\tfor i in 1 2\n+\tdo\n+\t\ttest_must_fail git branch -f fake$i HEAD 2>err &&\n+\t\tgrep \"cannot force update the branch '\\''fake$i'\\'' checked out at\" err ||\n+\t\t\treturn 1\n+\tdone\n+'\n+\n test_done\n-- \n2.36.1.220.g1fae7daf425\n\n\n"},{"id":"457150","messageId":"xmqq35g8415f.fsf@gitster.g","threadId":"57973","inReplyTo":"6cd7db33-6ab5-9843-4483-4cce9835b177@github.com","subject":"Re: [PATCH 5/5] branch: fix branch_checked_out() leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-13T23:03:08Z","receivedAt":"2022-06-13T23:03:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> \twhile (worktrees[i]) {\n> +\t\tchar *old;\n> \t\tstruct wt_status_state state = { 0 };\n> \t\tstruct worktree *wt = worktrees[i++];\n> \n> \t\tif (wt->is_bare)\n> \t\t\tcontinue;\n> \n> -\t\tif (wt->head_ref)\n> -\t\t\tstrmap_put(¤t_checked_out_branches,\n> -\t\t\t\t wt->head_ref,\n> -\t\t\t\t xstrdup(wt->path));\n> +\t\tif (wt->head_ref) {\n> +\t\t\told = strmap_put(¤t_checked_out_branches,\n> +\t\t\t\t\t wt->head_ref,\n> +\t\t\t\t\t xstrdup(wt->path));\n> +\t\t\tfree(old);\n> +\t\t}\n\nWhile it is equivalent to\n\n\t\t\tfree(strmap_put(¤t_checked_out_branches,\n \t\twt->head_ref,\n\t\t\t\t\txstrdup(wt->path)));\n\nwriting the \"put the new one\" and \"discard the old one (if exists)\"\nas separate steps like the above does make it easier to follow, at\nleast to me.\n\nThanks. Will queue.\n"},{"id":"457154","messageId":"220614.86leu0yur8.gmgdl@evledraar.gmail.com","threadId":"57973","inReplyTo":"dbb7eae390c90d4b710f48d8875bd7db0409aea3.1654718942.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] branch: add branch_checked_out() helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-13T23:59:32Z","receivedAt":"2022-06-14T00:05:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 08 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n> [...]\n> +\tpath_in_set = strmap_get(¤t_checked_out_branches, refname);\n> +\tif (path_in_set && path)\n> +\t\t*path = xstrdup(path_in_set);\n\nLooking at the end state of this series there is nothing that passes a\nnull \"path\" to this function, i.e. it's all &some_variable.\n\nSo just dropping that && seems better from a code understanding &\nanalysis perspective.\n\nMore generally, can't we just expose an API that gives us the strmap\nitself, so that callers don't need to keep any special-behavior in mind?\nI.e. just \"make me a strmap of checked out branches\".\n\nThen you can just use strmap_get(), or xstrdup(strmap_get()), and if the\npattern of \"get me the item but duped\" is found elsewhere maybe we\nshould eventually have a strmap_get_dup() or whatever.\n\nI.e. just making it: xstrdup(strmap_get(checked_out_branches_strmap(), refname)).\n"},{"id":"457155","messageId":"220614.86h74oyuc8.gmgdl@evledraar.gmail.com","threadId":"57973","inReplyTo":"4540dbeed385341f8c5b45134e1a65dc48c75b0c.1654718942.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] fetch: use new branch_checked_out() and add tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-14T00:05:51Z","receivedAt":"2022-06-14T00:14:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 08 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n\nre a comment on 1/4...\n\n> \tstruct commit *current = NULL, *updated;\n> -\tconst struct worktree *wt;\n> +\tchar *path = NULL;\n\nInit'd to NULL here...\n\n> \tconst char *pretty_ref = prettify_refname(ref->name);\n> \tint fast_forward = 0;\n> \n> @@ -900,17 +900,17 @@ static int update_local_ref(struct ref *ref,\n> \t}\n> \n> \tif (!update_head_ok &&\n> -\t (wt = find_shared_symref(worktrees, \"HEAD\", ref->name)) &&\n> -\t !wt->is_bare && !is_null_oid(&ref->old_oid)) {\n> +\t !is_null_oid(&ref->old_oid) &&\n> +\t branch_checked_out(ref->name, &path)) {\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 wt->is_current ?\n> -\t\t\t\t _(\"can't fetch in current branch\") :\n> -\t\t\t\t _(\"checked out in another worktree\"),\n> +\t\t\t path ? _(\"can't fetch in current branch\") :\n> +\t\t\t\t _(\"checked out in another worktree\"),\n> \t\t\t remote, pretty_ref, summary_width);\n> +\t\tfree(path);\n> \t\treturn 1;\n\n..but only used if branch_checkoed_out() returns non-zero, but (and I've\nonly eyeballed this, not run it) isn't it impossible for that function\nnot to return successfully and have \"path\" be non-NULL?\n\nThis seems to be a defense against the underlying strmap_get() starting\nto return nonsensical results, but if we instead just trust it...\n\nAlso re the \"can't we just expose the strmap?\" comment on 1/4, here we\nstrmap_get() it, and xstrdup() and free() it, but we never needed our\nown copy, or even cared about the string at all, we're just checking\n\"was it in the set?\".\n\nWhich is what strmap_contains() would give us, if we just exposed it at\nthat level. So:\n\n\tstruct strmap *map = make_my_branchs_map_thing();\n\n [...]\n if ([...] && strmap_contains(map, ref->name)) {\n\t ...\n\nSeems more straightforward, no?\n\nLooking at the other callers it seems none of them need the xstrdup() at\nall, if looking at the end state. This one is a strmp_contains(), then\nan error() which could use strmap_get() without the xstrdup(), as we\nformat it immediately.\n\nThe remaining two are calling die() right afterwards, which ditto can\nuse a plain strmap_get() without xstrdup().\n\nIt's not that I care about the extra xstrdup(), which is obviously\ntrivial, it's just harder to read & reason about APIs which e.g. \"&&\npath\", and strdup(), only to find that apparently no user in-tree cared\nabout those...\n\nMaybe they'll be needed, but let's just add them then, and in the\nmeantime aim for simpler? :)\n"},{"id":"457159","messageId":"220614.868rq0ytaa.gmgdl@evledraar.gmail.com","threadId":"57973","inReplyTo":"6cd7db33-6ab5-9843-4483-4cce9835b177@github.com","subject":"Re: [PATCH 5/5] branch: fix branch_checked_out() leaks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-14T00:33:59Z","receivedAt":"2022-06-14T00:37:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 13 2022, Derrick Stolee wrote:\n\n> On 6/8/2022 4:08 PM, Derrick Stolee via GitGitGadget wrote:\n>> This is a replacement for some patches from v2 of my 'git rebase\n>> --update-refs' topic [1]. After some feedback from Philip, I've decided to\n>> pull that topic while I rework how I track the refs to rewrite [2]. This\n>> series moves forward with the branch_checked_out() helper that was a bit\n>> more complicated than expected at first glance. This series is a culmination\n>> of the discussion started by Junio at [3].\n>> \n>\n> Junio pointed out that patch 1 introduced a memory leak when a ref\n> is checked out in multiple places. Here is a patch to fix that\n> scenario. It applies cleanly on top of patch 4, so I include it as\n> a new \"patch 5\". I will include it in any v2 of the full series, if\n> needed.\n>\n> Thanks,\n> -Stolee\n>\n> ---- >8 ----\n>\n> From c3842b36ebb4053ac49b0306154b840431f9bf6f Mon Sep 17 00:00:00 2001\n> From: Derrick Stolee \n> Date: Mon, 13 Jun 2022 10:33:20 -0400\n> Subject: [PATCH 5/5] branch: fix branch_checked_out() leaks\n>\n> The branch_checked_out() method populates a strmap linking a refname to\n> a worktree that has that branch checked out. While unlikely, it is\n> possible that a bug or filesystem manipulation could create a scenario\n> where the same ref is checked out in multiple places. Further, there are\n> some states in an interactive rebase where HEAD and REBASE_HEAD point to\n> the same ref, leading to multiple insertions into the strmap. In either\n> case, the strmap_put() method returns the old value which is leaked.\n>\n> Update branch_checked_out() to consume that pointer and free it.\n>\n> Add a test in t2407 that checks this erroneous case. The test \"checks\n> itself\" by first confirming that the filesystem manipulations it makes\n> trigger the branch_checked_out() logic, and then sets up similar\n> manipulations to make it look like there are multiple worktrees pointing\n> to the same ref.\n>\n> While TEST_PASSES_SANITIZE_LEAK would be helpful to demonstrate the\n> leakage and prevent it in the future, t2407 uses helpers such as 'git\n> clone' that cause the test to fail under that mode.\n\nIf you apply this:\n\t\n\tdiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\n\tindex 0760595337b..d41171acb83 100755\n\t--- a/t/t2407-worktree-heads.sh\n\t+++ b/t/t2407-worktree-heads.sh\n\t@@ -10,16 +10,8 @@ test_expect_success 'setup' '\n\t \t\ttest_commit $i &&\n\t \t\tgit branch wt-$i &&\n\t \t\tgit worktree add wt-$i wt-$i || return 1\n\t-\tdone &&\n\t-\n\t-\t# Create a server that updates each branch by one commit\n\t-\tgit clone . server &&\n\t-\tgit remote add server ./server &&\n\t-\tfor i in 1 2 3 4\n\t-\tdo\n\t-\t\tgit -C server checkout wt-$i &&\n\t-\t\ttest_commit -C server A-$i || return 1\n\t \tdone\n\t+\n\t '\n\t \n\t test_expect_success 'refuse to overwrite: checked out in worktree' '\n\nAnd compile with SANITIZE=leak then this will pass as:\n\n\t./t2407-worktree-heads.sh --run=1,6\n\nI.e. you only needed the earlier part of the setup, and not \"clone.\n\nGiven that I think it makes sense to just create a\nt2408-worktree-heads-leak.sh or something for this new test, then you\ncan use TEST_PASSES_SANITIZE_LEAK.\n\nNormally I'd just say \"let's leave it for later\", but in this case the\nentire point of the commit and the relatively lengthy test is to deal\nwith a memory leak, so just copy/pasting the few lines of setup you\nactually need to a new test & testing with SANITIZE=leak seems worth the\neffort in this case.\n"},{"id":"457177","messageId":"42ada1a3-adc2-2c43-9aa1-7854858865ae@gmail.com","threadId":"57973","inReplyTo":"dbb7eae390c90d4b710f48d8875bd7db0409aea3.1654718942.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] branch: add branch_checked_out() helper","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-06-14T10:09:15Z","receivedAt":"2022-06-14T10:10:07Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stolee\n\nOn 08/06/2022 21:08, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The validate_new_branchname() method contains a check to see if a branch\n> is checked out in any non-bare worktree. This is intended to prevent a\n> force push that will mess up an existing checkout. This helper is not\n> suitable to performing just that check, because the method will die()\n> when the branch is checked out instead of returning an error code.\n> \n> Create a new branch_checked_out() helper that performs the most basic\n> form of this check. To ensure we can call branch_checked_out() in a loop\n> with good performance, do a single preparation step that iterates over\n> all worktrees and stores their current HEAD branches in a strmap. The\n> branch_checked_out() helper can then discover these branches using a\n> hash lookup.\n> \n> This helper is currently missing some key functionality. Namely: it\n> doesn't look for active rebases or bisects which mean that the branch is\n> \"checked out\" even though HEAD doesn't point to that ref. This\n> functionality will be added in a coming change.\n> \n> We could use branch_checked_out() in validate_new_branchname(), but this\n> missing functionality would be a regression. However, we have no tests\n> that cover this case!\n> \n> Add a new test script that will be expanded with these cross-worktree\n> ref updates. The current tests would still pass if we refactored\n> validate_new_branchname() to use this version of branch_checked_out().\n> The next change will fix that functionality and add the proper test\n> coverage.\n> \n> Signed-off-by: Derrick Stolee \n> ---\n> branch.c | 42 +++++++++++++++++++++++++++++++++++++++\n> branch.h | 8 ++++++++\n> t/t2407-worktree-heads.sh | 24 ++++++++++++++++++++++\n> 3 files changed, 74 insertions(+)\n> create mode 100755 t/t2407-worktree-heads.sh\n> \n> diff --git a/branch.c b/branch.c\n> index 2d6569b0c62..061a11f3415 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -10,6 +10,7 @@\n> #include \"worktree.h\"\n> #include \"submodule-config.h\"\n> #include \"run-command.h\"\n> +#include \"strmap.h\"\n> \n> struct tracking {\n> \tstruct refspec_item spec;\n> @@ -369,6 +370,47 @@ int validate_branchname(const char *name, struct strbuf *ref)\n> \treturn ref_exists(ref->buf);\n> }\n> \n> +static int initialized_checked_out_branches;\n> +static struct strmap current_checked_out_branches = STRMAP_INIT;\n\nI looks like this map is never freed which I think makes sense but makes \nme wonder about the relevance of patch 5. I think it would probably be \nworth marking the map with UNLEAK() in prepare_checked_out_branches().\n\n> +static void prepare_checked_out_branches(void)\n> +{\n> +\tint i = 0;\n> +\tstruct worktree **worktrees;\n> +\n> +\tif (initialized_checked_out_branches)\n> +\t\treturn;\n> +\tinitialized_checked_out_branches = 1;\n> +\n> +\tworktrees = get_worktrees();\n> +\n> +\twhile (worktrees[i]) {\n> +\t\tstruct worktree *wt = worktrees[i++];\n> +\n> +\t\tif (wt->is_bare)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (wt->head_ref)\n> +\t\t\tstrmap_put(¤t_checked_out_branches,\n> +\t\t\t\t wt->head_ref,\n> +\t\t\t\t xstrdup(wt->path));\n\nSTRMAP_INIT sets .strdup_strings = 1, so the xstrdup() is unnecessary.\n\n> +\t}\n> +\n> +\tfree_worktrees(worktrees);\n> +}\n> +\n> +int branch_checked_out(const char *refname, char **path)\n> +{\n> +\tconst char *path_in_set;\n> +\tprepare_checked_out_branches();\n> +\n> +\tpath_in_set = strmap_get(¤t_checked_out_branches, refname);\n> +\tif (path_in_set && path)\n> +\t\t*path = xstrdup(path_in_set);\n> +\n> +\treturn !!path_in_set;\n> +}\n\nI like the idea of having a specific function to see if a branch is \ncheckout out rather than Ævar's suggestion of forcing all callers to do\n strmap_get(get_worktree_refs_strmap(), ref)\nwhich will quickly get tiresome. I do wonder though if we'd be better \noff with a thin wrapper around strmap_get() such as\n\nconst char* branch_checked_out(const char *refname)\n{\n\tprepare_checked_out_branches();\n\treturn strmap_get(¤t_checked_out_branches, refname);\n}\n\nso that the user can choose whether to copy the path or not.\n\nBest Wishes\n\nPhillip\n\n> /*\n> * Check if a branch 'name' can be created as a new branch; die otherwise.\n> * 'force' can be used when it is OK for the named branch already exists.\n> diff --git a/branch.h b/branch.h\n> index 560b6b96a8f..5ea93d217b1 100644\n> --- a/branch.h\n> +++ b/branch.h\n> @@ -101,6 +101,14 @@ void create_branches_recursively(struct repository *r, const char *name,\n> \t\t\t\t const char *tracking_name, int force,\n> \t\t\t\t int reflog, int quiet, enum branch_track track,\n> \t\t\t\t int dry_run);\n> +\n> +/*\n> + * Returns true if the branch at 'refname' is checked out at any\n> + * non-bare worktree. The path of the worktree is stored in the\n> + * given 'path', if provided.\n> + */\n> +int branch_checked_out(const char *refname, char **path);\n> +\n> /*\n> * Check if 'name' can be a valid name for a branch; die otherwise.\n> * Return 1 if the named branch already exists; return 0 otherwise.\n> diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\n> new file mode 100755\n> index 00000000000..dd905dc1a5c\n> --- /dev/null\n> +++ b/t/t2407-worktree-heads.sh\n> @@ -0,0 +1,24 @@\n> +#!/bin/sh\n> +\n> +test_description='test operations trying to overwrite refs at worktree HEAD'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\tfor i in 1 2 3 4\n> +\tdo\n> +\t\ttest_commit $i &&\n> +\t\tgit branch wt-$i &&\n> +\t\tgit worktree add wt-$i wt-$i || return 1\n> +\tdone\n> +'\n> +\n> +test_expect_success 'refuse to overwrite: checked out in worktree' '\n> +\tfor i in 1 2 3 4\n> +\tdo\n> +\t\ttest_must_fail git branch -f wt-$i HEAD 2>err\n> +\t\tgrep \"cannot force update the branch\" err || return 1\n> +\tdone\n> +'\n> +\n> +test_done\n\n"},{"id":"457178","messageId":"5137243d-ff20-e184-493b-29af64c408d9@gmail.com","threadId":"57973","inReplyTo":"4540dbeed385341f8c5b45134e1a65dc48c75b0c.1654718942.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] fetch: use new branch_checked_out() and add tests","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-06-14T10:10:59Z","receivedAt":"2022-06-14T10:11:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stolee\n\nOn 08/06/2022 21:09, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> [...]\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index ac29c2b1ae3..1ba56240312 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -885,7 +885,7 @@ static int update_local_ref(struct ref *ref,\n> \t\t\t struct worktree **worktrees)\n> {\n> \tstruct commit *current = NULL, *updated;\n> -\tconst struct worktree *wt;\n> +\tchar *path = NULL;\n> \tconst char *pretty_ref = prettify_refname(ref->name);\n> \tint fast_forward = 0;\n> \n> @@ -900,17 +900,17 @@ static int update_local_ref(struct ref *ref,\n> \t}\n> \n> \tif (!update_head_ok &&\n> -\t (wt = find_shared_symref(worktrees, \"HEAD\", ref->name)) &&\n> -\t !wt->is_bare && !is_null_oid(&ref->old_oid)) {\n> +\t !is_null_oid(&ref->old_oid) &&\n> +\t branch_checked_out(ref->name, &path)) {\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 wt->is_current ?\n> -\t\t\t\t _(\"can't fetch in current branch\") :\n> -\t\t\t\t _(\"checked out in another worktree\"),\n> +\t\t\t path ? _(\"can't fetch in current branch\") :\n> +\t\t\t\t _(\"checked out in another worktree\"),\n\nI'm confused by this, isn't path always non-null?\n\nBest Wishes\n\nPhillip\n\n> \t\t\t remote, pretty_ref, summary_width);\n> +\t\tfree(path);\n> \t\treturn 1;\n> \t}\n> \n> @@ -1434,19 +1434,16 @@ cleanup:\n> \treturn result;\n> }\n> \n> -static void check_not_current_branch(struct ref *ref_map,\n> -\t\t\t\t struct worktree **worktrees)\n> +static void check_not_current_branch(struct ref *ref_map)\n> {\n> -\tconst struct worktree *wt;\n> +\tchar *path;\n> \tfor (; ref_map; ref_map = ref_map->next)\n> \t\tif (ref_map->peer_ref &&\n> \t\t starts_with(ref_map->peer_ref->name, \"refs/heads/\") &&\n> -\t\t (wt = find_shared_symref(worktrees, \"HEAD\",\n> -\t\t\t\t\t ref_map->peer_ref->name)) &&\n> -\t\t !wt->is_bare)\n> +\t\t branch_checked_out(ref_map->peer_ref->name, &path))\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> +\t\t\t ref_map->peer_ref->name, path);\n> }\n> \n> static int truncate_fetch_head(void)\n> @@ -1650,7 +1647,7 @@ static int do_fetch(struct transport *transport,\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, worktrees);\n> +\t\tcheck_not_current_branch(ref_map);\n> \n> \tretcode = open_fetch_head(&fetch_head);\n> \tif (retcode)\n> diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\n> index 12faca7f655..f3f8b0b2b79 100755\n> --- a/t/t2407-worktree-heads.sh\n> +++ b/t/t2407-worktree-heads.sh\n> @@ -10,6 +10,15 @@ test_expect_success 'setup' '\n> \t\ttest_commit $i &&\n> \t\tgit branch wt-$i &&\n> \t\tgit worktree add wt-$i wt-$i || return 1\n> +\tdone &&\n> +\n> +\t# Create a server that updates each branch by one commit\n> +\tgit clone . server &&\n> +\tgit remote add server ./server &&\n> +\tfor i in 1 2 3 4\n> +\tdo\n> +\t\tgit -C server checkout wt-$i &&\n> +\t\ttest_commit -C server A-$i || return 1\n> \tdone\n> '\n> \n> @@ -21,6 +30,16 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '\n> \tdone\n> '\n> \n> +test_expect_success 'refuse to overwrite during fetch' '\n> +\ttest_must_fail git fetch server +refs/heads/wt-3:refs/heads/wt-3 2>err &&\n> +\tgrep \"refusing to fetch into branch '\\''refs/heads/wt-3'\\''\" err &&\n> +\n> +\t# General fetch into refs/heads/ will fail on first ref,\n> +\t# so use a generic error message check.\n> +\ttest_must_fail git fetch server +refs/heads/*:refs/heads/* 2>err &&\n> +\tgrep \"refusing to fetch into branch\" err\n> +'\n> +\n> test_expect_success 'refuse to overwrite: worktree in bisect' '\n> \ttest_when_finished test_might_fail git -C wt-4 bisect reset &&\n> \n> @@ -31,7 +50,10 @@ test_expect_success 'refuse to overwrite: worktree in bisect' '\n> \t) &&\n> \n> \ttest_must_fail git branch -f wt-4 HEAD 2>err &&\n> -\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err\n> +\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err &&\n> +\n> +\ttest_must_fail git fetch server +refs/heads/wt-4:refs/heads/wt-4 2>err &&\n> +\tgrep \"refusing to fetch into branch '\\''refs/heads/wt-4'\\''\" err\n> '\n> \n> . \"$TEST_DIRECTORY\"/lib-rebase.sh\n> @@ -47,7 +69,10 @@ test_expect_success 'refuse to overwrite: worktree in rebase' '\n> \t) &&\n> \n> \ttest_must_fail git branch -f wt-4 HEAD 2>err &&\n> -\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err\n> +\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err &&\n> +\n> +\ttest_must_fail git fetch server +refs/heads/wt-4:refs/heads/wt-4 2>err &&\n> +\tgrep \"refusing to fetch into branch '\\''refs/heads/wt-4'\\''\" err\n> '\n> \n> test_done\n\n"},{"id":"457180","messageId":"cdf0012a-f104-87e2-104f-db1c8080cf0a@gmail.com","threadId":"57973","inReplyTo":"42ada1a3-adc2-2c43-9aa1-7854858865ae@gmail.com","subject":"Re: [PATCH 1/4] branch: add branch_checked_out() helper","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-06-14T11:22:16Z","receivedAt":"2022-06-14T11:22:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 14/06/2022 11:09, Phillip Wood wrote:\n> [..]\n>> +static int initialized_checked_out_branches;\n>> +static struct strmap current_checked_out_branches = STRMAP_INIT;\n> \n> I looks like this map is never freed which I think makes sense but makes \n> me wonder about the relevance of patch 5. I think it would probably be \n> worth marking the map with UNLEAK() in prepare_checked_out_branches().\n\nActually I think UNLEAK() is for marking stack variables, not globals so \nignore that. I think patch 5 makes sense in that in keeps the leak bounded.\n\n>> +        if (wt->head_ref)\n>> +            strmap_put(¤t_checked_out_branches,\n>> +                   wt->head_ref,\n>> +                   xstrdup(wt->path));\n> \n> STRMAP_INIT sets .strdup_strings = 1, so the xstrdup() is unnecessary.\n\nSorry that's nonsense - it is wt->head_ref that is copied by \nstrmap_put(), not wt->path, so the xstrdup() call is correct\n\nThanks for working on this and sorry for the confusion\n\nPhillip\n"},{"id":"457186","messageId":"4fc234ab-de06-9e96-daf6-7b3496132541@github.com","threadId":"57973","inReplyTo":"220614.86leu0yur8.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/4] branch: add branch_checked_out() helper","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-14T13:32:24Z","receivedAt":"2022-06-14T13:32:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/13/2022 7:59 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Jun 08 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee \n>> [...]\n>> +\tpath_in_set = strmap_get(¤t_checked_out_branches, refname);\n>> +\tif (path_in_set && path)\n>> +\t\t*path = xstrdup(path_in_set);\n> \n> Looking at the end state of this series there is nothing that passes a\n> null \"path\" to this function, i.e. it's all &some_variable.\n> \n> So just dropping that && seems better from a code understanding &\n> analysis perspective.\n\nI don't see the value in making this method more prone to error in\nthe future, or less flexible to a possible caller that doesn't want\nto give a 'path'.\n \n> More generally, can't we just expose an API that gives us the strmap\n> itself, so that callers don't need to keep any special-behavior in mind?\n> I.e. just \"make me a strmap of checked out branches\".\n> \n> Then you can just use strmap_get(), or xstrdup(strmap_get()), and if the\n> pattern of \"get me the item but duped\" is found elsewhere maybe we\n> should eventually have a strmap_get_dup() or whatever.\n> \n> I.e. just making it: xstrdup(strmap_get(checked_out_branches_strmap(), refname)).\n\nThis seems unnecessarily complicated. It also risks someone inserting\nkey-value pairs in an improper place instead of being contained to\nprepare_checked_out_branches().\n\nIf your concern is about creating the static current_checked_out_branches,\nkeep in mind that the callers that call branch_checked_out() in a loop\nwould need to keep track of that strmap across several other methods.\nThat additional complexity is much worse than asking for a simple answer\nthrough the black box of branch_checked_out().\n\nThanks,\n-Stolee\n"},{"id":"457188","messageId":"526fd647-4d8c-7773-06e3-011db173fe28@github.com","threadId":"57973","inReplyTo":"42ada1a3-adc2-2c43-9aa1-7854858865ae@gmail.com","subject":"Re: [PATCH 1/4] branch: add branch_checked_out() helper","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-14T13:48:06Z","receivedAt":"2022-06-14T13:48:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/14/2022 6:09 AM, Phillip Wood wrote:\n> Hi Stolee\n> \n> On 08/06/2022 21:08, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n\n>> +        if (wt->head_ref)\n>> +            strmap_put(¤t_checked_out_branches,\n>> +                   wt->head_ref,\n>> +                   xstrdup(wt->path));\n> \n> STRMAP_INIT sets .strdup_strings = 1, so the xstrdup() is unnecessary.\n\nThat .strdup_strings is only for the key, not the value (since the\nvalue could be anything, not necessarily a string).\n\n>> +    }\n>> +\n>> +    free_worktrees(worktrees);\n>> +}\n>> +\n>> +int branch_checked_out(const char *refname, char **path)\n>> +{\n>> +    const char *path_in_set;\n>> +    prepare_checked_out_branches();\n>> +\n>> +    path_in_set = strmap_get(¤t_checked_out_branches, refname);\n>> +    if (path_in_set && path)\n>> +        *path = xstrdup(path_in_set);\n>> +\n>> +    return !!path_in_set;\n>> +}\n> \n> I like the idea of having a specific function to see if a branch is\n> checkout out rather than Ævar's suggestion of forcing all callers to do\n>     strmap_get(get_worktree_refs_strmap(), ref)\n> which will quickly get tiresome. I do wonder though if we'd be better\n> off with a thin wrapper around strmap_get() such as\n> \n> const char* branch_checked_out(const char *refname)\n> {\n>     prepare_checked_out_branches();\n>     return strmap_get(¤t_checked_out_branches, refname);\n> }\n> \n> so that the user can choose whether to copy the path or not.\n\nThis is an interesting suggestion. It changes callers a bit, but not\ntoo much. The pattern currently is:\n\n\tif (branch_checked_out(name, &path)) {\n\t\t/* do something */\n\t\tfree(path);\n\t}\n\nbut would become\n\n\tif ((path = branch_checked_out(name))) {\n\t\t/* do something */\n\t}\n\nSounds good to me.\n\nThanks,\n-Stolee\n"},{"id":"457189","messageId":"fa330b8c-e495-1ab8-2085-d1c2d7e06ff6@github.com","threadId":"57973","inReplyTo":"5137243d-ff20-e184-493b-29af64c408d9@gmail.com","subject":"Re: [PATCH 3/4] fetch: use new branch_checked_out() and add tests","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-14T14:06:33Z","receivedAt":"2022-06-14T14:06:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/14/2022 6:10 AM, Phillip Wood wrote:\n> Hi Stolee\n> \n> On 08/06/2022 21:09, Derrick Stolee via GitGitGadget wrote:\n>>           format_display(display, '!', _(\"[rejected]\"),\n>> -                   wt->is_current ?\n>> -                       _(\"can't fetch in current branch\") :\n>> -                       _(\"checked out in another worktree\"),\n>> +                   path ? _(\"can't fetch in current branch\") :\n>> +                      _(\"checked out in another worktree\"),\n> \n> I'm confused by this, isn't path always non-null?\n\nYes, at this point it is. This message needs to change since we\nno longer have access to the worktree. As noted in the commit\nmessage, this case requires concurrent changes to the filesystem\nto activate this logic, so having a simpler \"branch is checked\nout _somewhere_\" message should work here.\n\nThanks,\n-Stolee\n"},{"id":"457195","messageId":"220614.86zgifxnql.gmgdl@evledraar.gmail.com","threadId":"57973","inReplyTo":"4fc234ab-de06-9e96-daf6-7b3496132541@github.com","subject":"Re: [PATCH 1/4] branch: add branch_checked_out() helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-14T15:24:50Z","receivedAt":"2022-06-14T15:34:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 14 2022, Derrick Stolee wrote:\n\n> On 6/13/2022 7:59 PM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Wed, Jun 08 2022, Derrick Stolee via GitGitGadget wrote:\n>> \n>>> From: Derrick Stolee \n>>> [...]\n>>> +\tpath_in_set = strmap_get(¤t_checked_out_branches, refname);\n>>> +\tif (path_in_set && path)\n>>> +\t\t*path = xstrdup(path_in_set);\n>> \n>> Looking at the end state of this series there is nothing that passes a\n>> null \"path\" to this function, i.e. it's all &some_variable.\n>> \n>> So just dropping that && seems better from a code understanding &\n>> analysis perspective.\n>\n> I don't see the value in making this method more prone to error in\n> the future, or less flexible to a possible caller that doesn't want\n> to give a 'path'.\n\nI think creating wrappers for exsiting APIs when they're not really\nneeded is what's more error prone. I.e. you need to learn to use the new\nthing, whereas if it returns an existing type like \"struct strmap\",\n\"struct strbuf\" etc. there's no learning curve.\n\nIt doesn't matter *that much* in this case, but I think it really adds\nup.\n\n>> More generally, can't we just expose an API that gives us the strmap\n>> itself, so that callers don't need to keep any special-behavior in mind?\n>> I.e. just \"make me a strmap of checked out branches\".\n>> \n>> Then you can just use strmap_get(), or xstrdup(strmap_get()), and if the\n>> pattern of \"get me the item but duped\" is found elsewhere maybe we\n>> should eventually have a strmap_get_dup() or whatever.\n>> \n>> I.e. just making it: xstrdup(strmap_get(checked_out_branches_strmap(), refname)).\n>\n> This seems unnecessarily complicated. It also risks someone inserting\n> key-value pairs in an improper place instead of being contained to\n> prepare_checked_out_branches().\n\nI really don't think the internal APIs in git need to be this paranoid,\njust document it as \"don't touch this\".\n\nI think in practice we have (and can expect) pretty mmuch zero bugs due\nto misuse of APIs that are clearly meant to be global singletons,\nwhereas as this series & the fix-up shows it's more likely that we'll\nneed to deal with subtle memory leaks etc. due to over-wrapping in new\nAPIS :)\n\nOr, if it does need paranoia simply structure the API to have each\ncaller get its own copy, here all the callers are one-shot built-ins, so\nisn't this a premature optimization?\n\nAnd speaking of risking things, probably overly fragile as an underlying\n\"general\" API, if that's what you're aiming for.\n\nI.e. you're making the hard assumption that the set of checked out\nbranches aren't changing during the lifetime of the process. So\ne.g. builtin/checkout.c couldn't call this at the end of its run if\nsomething in it had needed to get the map earlier (before we switched\nbranches)...\n\n> If your concern is about creating the static current_checked_out_branches,\n> keep in mind that the callers that call branch_checked_out() in a loop\n> would need to keep track of that strmap across several other methods.\n> That additional complexity is much worse than asking for a simple answer\n> through the black box of branch_checked_out().\n\n...that being said I was not suggesting to do away with the static\npattern, but to keep it, and to just return the map.\n\nAs far as hypothetical hot loops are concerned that would be marginally\ncheaper, as you could move the \"get map\" outside a loop in such a\ncaller.\n\nI tried this fix-up on top of \"seen\", which passes your test, and as\nnoted allocates less memory:\n\ndiff --git a/branch.c b/branch.c\nindex c7896f69a7c..efe1f6ce865 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -373,14 +373,13 @@ int validate_branchname(const char *name, struct strbuf *ref)\n static int initialized_checked_out_branches;\n static struct strmap current_checked_out_branches = STRMAP_INIT;\n \n-static void prepare_checked_out_branches(void)\n+struct strmap *checked_out_branches_map(void)\n {\n \tint i = 0;\n \tstruct worktree **worktrees;\n \n \tif (initialized_checked_out_branches)\n-\t\treturn;\n-\tinitialized_checked_out_branches = 1;\n+\t\tgoto done;\n \n \tworktrees = get_worktrees();\n \n@@ -426,18 +425,10 @@ static void prepare_checked_out_branches(void)\n \t}\n \n \tfree_worktrees(worktrees);\n-}\n \n-int branch_checked_out(const char *refname, char **path)\n-{\n-\tconst char *path_in_set;\n-\tprepare_checked_out_branches();\n-\n-\tpath_in_set = strmap_get(¤t_checked_out_branches, refname);\n-\tif (path_in_set && path)\n-\t\t*path = xstrdup(path_in_set);\n-\n-\treturn !!path_in_set;\n+done:\n+\tinitialized_checked_out_branches = 1;\n+\treturn ¤t_checked_out_branches;\n }\n \n /*\n@@ -456,7 +447,8 @@ 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-\tif (branch_checked_out(ref->buf, &path))\n+\tpath = strmap_get(checked_out_branches_map() , ref->buf);\n+\tif (path)\n \t\tdie(_(\"cannot force update the branch '%s' \"\n \t\t \"checked out at '%s'\"),\n \t\t ref->buf + strlen(\"refs/heads/\"), path);\ndiff --git a/branch.h b/branch.h\nindex 5ea93d217b1..63cb0f7adc4 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -103,11 +103,12 @@ void create_branches_recursively(struct repository *r, const char *name,\n \t\t\t\t int dry_run);\n \n /*\n- * Returns true if the branch at 'refname' is checked out at any\n- * non-bare worktree. The path of the worktree is stored in the\n- * given 'path', if provided.\n+ * Lazily returns a \"struct strmap\" of checked out branches. The\n+ * returned value is a global that'll be populated on the first\n+ * call. You should only call read-only strmap API functions on this,\n+ * such as strmap_get(), strmap_contains() etc.\n */\n-int branch_checked_out(const char *refname, char **path);\n+struct strmap *checked_out_branches_map(void);\n \n /*\n * Check if 'name' can be a valid name for a branch; die otherwise.\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 8e11e433840..2913b2e75e4 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -24,6 +24,7 @@\n #include \"worktree.h\"\n #include \"help.h\"\n #include \"commit-reach.h\"\n+#include \"strmap.h\"\n \n static const char * const builtin_branch_usage[] = {\n \tN_(\"git branch [] [-r | -a] [--merged] [--no-merged]\"),\n@@ -253,12 +254,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tname = mkpathdup(fmt, bname.buf);\n \n \t\tif (kinds == FILTER_REFS_BRANCHES) {\n-\t\t\tchar *path;\n-\t\t\tif (branch_checked_out(name, &path)) {\n+\t\t\tconst char *path = strmap_get(checked_out_branches_map(),\n+\t\t\t\t\t\t name);\n+\t\t\tif (path) {\n \t\t\t\terror(_(\"Cannot delete branch '%s' \"\n \t\t\t\t\t\"checked out at '%s'\"),\n \t\t\t\t bname.buf, path);\n-\t\t\t\tfree(path);\n \t\t\t\tret = 1;\n \t\t\t\tcontinue;\n \t\t\t}\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 9ee7f2241ad..a57506a4003 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -30,6 +30,7 @@\n #include \"shallow.h\"\n #include \"worktree.h\"\n #include \"bundle-uri.h\"\n+#include \"strmap.h\"\n \n #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n \n@@ -890,7 +891,6 @@ static int update_local_ref(struct ref *ref,\n \t\t\t struct worktree **worktrees)\n {\n \tstruct commit *current = NULL, *updated;\n-\tchar *path = NULL;\n \tconst char *pretty_ref = prettify_refname(ref->name);\n \tint fast_forward = 0;\n \n@@ -905,17 +905,17 @@ static int update_local_ref(struct ref *ref,\n \t}\n \n \tif (!update_head_ok &&\n-\t !is_null_oid(&ref->old_oid) &&\n-\t branch_checked_out(ref->name, &path)) {\n+\t !is_null_oid(&ref->old_oid)) {\n+\t\tint in_set = strmap_contains(checked_out_branches_map(),\n+\t\t\t\t\t ref->name);\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 path ? _(\"can't fetch in current branch\") :\n+\t\t\t in_set ? _(\"can't fetch in current branch\") :\n \t\t\t\t _(\"checked out in another worktree\"),\n \t\t\t remote, pretty_ref, summary_width);\n-\t\tfree(path);\n \t\treturn 1;\n \t}\n \n@@ -1445,7 +1445,8 @@ static void check_not_current_branch(struct ref *ref_map)\n \tfor (; ref_map; ref_map = ref_map->next)\n \t\tif (ref_map->peer_ref &&\n \t\t starts_with(ref_map->peer_ref->name, \"refs/heads/\") &&\n-\t\t branch_checked_out(ref_map->peer_ref->name, &path))\n+\t\t (path = strmap_get(checked_out_branches_map(),\n+\t\t\t\t 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, path);\n"},{"id":"457196","messageId":"0d00d910-a7aa-dfd8-a24c-51968800c2a8@github.com","threadId":"57973","inReplyTo":"220614.868rq0ytaa.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 5/5] branch: fix branch_checked_out() leaks","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-14T15:37:27Z","receivedAt":"2022-06-14T15:37:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/13/2022 8:33 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Jun 13 2022, Derrick Stolee wrote:\n> \n>> On 6/8/2022 4:08 PM, Derrick Stolee via GitGitGadget wrote:\n\n>> While TEST_PASSES_SANITIZE_LEAK would be helpful to demonstrate the\n>> leakage and prevent it in the future, t2407 uses helpers such as 'git\n>> clone' that cause the test to fail under that mode.\n> \n> If you apply this:\n> \t\n> \tdiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\n> \tindex 0760595337b..d41171acb83 100755\n> \t--- a/t/t2407-worktree-heads.sh\n> \t+++ b/t/t2407-worktree-heads.sh\n> \t@@ -10,16 +10,8 @@ test_expect_success 'setup' '\n> \t \t\ttest_commit $i &&\n> \t \t\tgit branch wt-$i &&\n> \t \t\tgit worktree add wt-$i wt-$i || return 1\n> \t-\tdone &&\n> \t-\n> \t-\t# Create a server that updates each branch by one commit\n> \t-\tgit clone . server &&\n> \t-\tgit remote add server ./server &&\n> \t-\tfor i in 1 2 3 4\n> \t-\tdo\n> \t-\t\tgit -C server checkout wt-$i &&\n> \t-\t\ttest_commit -C server A-$i || return 1\n> \t \tdone\n> \t+\n> \t '\n> \t \n> \t test_expect_success 'refuse to overwrite: checked out in worktree' '\n> \n> And compile with SANITIZE=leak then this will pass as:\n> \n> \t./t2407-worktree-heads.sh --run=1,6\n\nOf course this works for the tests that don't need the 'server' repo,\nbut it fails in the tests that _do_ need it.\n\nI'm able to make this work by creating the 'server' with init and\ncreating the wt-$i branches from scratch (they don't need to be\nfast-forward updates).\n\nThe linux-leaks tests still fail due to 'git fetch' and 'git bisect'\ncalls, but these can be avoided by carefully splitting the tests and\nusing the !SANITIZE_LEAK prereq.\n\n> Normally I'd just say \"let's leave it for later\", but in this case the\n> entire point of the commit and the relatively lengthy test is to deal\n> with a memory leak, so just copy/pasting the few lines of setup you\n> actually need to a new test & testing with SANITIZE=leak seems worth the\n> effort in this case.\n\nWell, the point isn't to use automation to check for leaks, but instead\nto fix leaks and add tests for the case where we previously had leaks.\nThe tests demonstrate that we aren't accidentally introducing a use-after-\nfree or double-free.\n\nThanks,\n-Stolee\n"},{"id":"457204","messageId":"220614.86v8t3xkfi.gmgdl@evledraar.gmail.com","threadId":"57973","inReplyTo":"0d00d910-a7aa-dfd8-a24c-51968800c2a8@github.com","subject":"Re: [PATCH 5/5] branch: fix branch_checked_out() leaks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-14T16:43:57Z","receivedAt":"2022-06-14T16:46:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 14 2022, Derrick Stolee wrote:\n\n> On 6/13/2022 8:33 PM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Mon, Jun 13 2022, Derrick Stolee wrote:\n>> \n>>> On 6/8/2022 4:08 PM, Derrick Stolee via GitGitGadget wrote:\n>\n>>> While TEST_PASSES_SANITIZE_LEAK would be helpful to demonstrate the\n>>> leakage and prevent it in the future, t2407 uses helpers such as 'git\n>>> clone' that cause the test to fail under that mode.\n>> \n>> If you apply this:\n>> \t\n>> \tdiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\n>> \tindex 0760595337b..d41171acb83 100755\n>> \t--- a/t/t2407-worktree-heads.sh\n>> \t+++ b/t/t2407-worktree-heads.sh\n>> \t@@ -10,16 +10,8 @@ test_expect_success 'setup' '\n>> \t \t\ttest_commit $i &&\n>> \t \t\tgit branch wt-$i &&\n>> \t \t\tgit worktree add wt-$i wt-$i || return 1\n>> \t-\tdone &&\n>> \t-\n>> \t-\t# Create a server that updates each branch by one commit\n>> \t-\tgit clone . server &&\n>> \t-\tgit remote add server ./server &&\n>> \t-\tfor i in 1 2 3 4\n>> \t-\tdo\n>> \t-\t\tgit -C server checkout wt-$i &&\n>> \t-\t\ttest_commit -C server A-$i || return 1\n>> \t \tdone\n>> \t+\n>> \t '\n>> \t \n>> \t test_expect_success 'refuse to overwrite: checked out in worktree' '\n>> \n>> And compile with SANITIZE=leak then this will pass as:\n>> \n>> \t./t2407-worktree-heads.sh --run=1,6\n>\n> Of course this works for the tests that don't need the 'server' repo,\n> but it fails in the tests that _do_ need it.\n>\n> I'm able to make this work by creating the 'server' with init and\n> creating the wt-$i branches from scratch (they don't need to be\n> fast-forward updates).\n>\n> The linux-leaks tests still fail due to 'git fetch' and 'git bisect'\n> calls, but these can be avoided by carefully splitting the tests and\n> using the !SANITIZE_LEAK prereq.\n\nI mean copy/paste the existing file to a new one with this on top, then\nremove all tests that aren't your new one. That worked for me, doesn't\nit work for you?\n\n>> Normally I'd just say \"let's leave it for later\", but in this case the\n>> entire point of the commit and the relatively lengthy test is to deal\n>> with a memory leak, so just copy/pasting the few lines of setup you\n>> actually need to a new test & testing with SANITIZE=leak seems worth the\n>> effort in this case.\n>\n> Well, the point isn't to use automation to check for leaks, but instead\n> to fix leaks and add tests for the case where we previously had leaks.\n> The tests demonstrate that we aren't accidentally introducing a use-after-\n> free or double-free.\n\nI mean that the patch subject is \"fix[...]leaks\", and this gives it a\nCI'd regression test, which seems worth it.\n\nOf course it's also nice to check for use-after-fre etc., but we have\nother coverage for that. E.g. glibc in CI spots double-free issues with\nhow we set it up, and we tend to segfault soon enough (or someone runs\nSANITIZE=address) on use-after-free.\n"},{"id":"457209","messageId":"pull.1254.v2.git.1655234853.gitgitgadget@gmail.com","threadId":"57973","inReplyTo":"pull.1254.git.1654718942.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] Create branch_checked_out() helper","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-14T19:27:28Z","receivedAt":"2022-06-14T19:27:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This is a replacement for some patches from v2 of my 'git rebase\n--update-refs' topic [1]. After some feedback from Philip, I've decided to\npull that topic while I rework how I track the refs to rewrite [2]. This\nseries moves forward with the branch_checked_out() helper that was a bit\nmore complicated than expected at first glance. This series is a culmination\nof the discussion started by Junio at [3].\n\n[1]\nhttps://lore.kernel.org/git/pull.1247.v2.git.1654634569.gitgitgadget@gmail.com\n[2]\nhttps://lore.kernel.org/git/34264915-8a37-62db-f954-0b5297639b34@github.com/\n[3] https://lore.kernel.org/git/xmqqr140t9am.fsf@gitster.g/\n\nHere is the patch outline:\n\n 1. Add a basic form of branch_checked_out() that only looks at the HEAD of\n each worktree. Since this doesn't look at BISECT_HEAD or REBASE_HEAD,\n the helper isn't linked to any consumer in this patch. A test script is\n added that verifies the current behavior checks that are implemented,\n even if they are not connected yet.\n 2. Make branch_checked_out() actually look at BISECT_HEAD and REBASE_HEAD.\n Add tests for those cases, which was previously untested in the Git test\n suite. Use branch_checked_out() in 'git branch -f' to demonstrate this\n helper works as expected.\n 3. Use branch_checked_out() in 'git fetch' when checking refs that would be\n updated by the refspec. Add tests for that scenario, too.\n 4. Use branch_checked_out() in 'git branch -D' to prevent deleting refs\n from under an existing checkout. The commit message here describes some\n barriers to removing other uses of find_shared_symref() that could be\n good investigations for later.\n 5. Be careful when constructing the strmap to free old values, even though\n this should only happen in error scenarios. Add tests that verify this\n behavior.\n\n\nUpdates in v2\n=============\n\n * branch_checked_out() has a new prototype where it returns a 'const char\n *' instead of copying the path.\n * The test script is marked with TEST_PASSES_SANITIZE_LEAK and test are\n careful to avoid using 'git rebase' or 'git bisect' when possible.\n * Tests that cannot avoid memory-loss from 'git fetch' are marked with the\n \"!SANITIZE_LEAK\" prereq.\n * A previous replacement of 'wt->current' with 'path' is removed. This\n changes an error message, but it is a very rare scenario where this error\n would be output.\n\nThanks, -Stolee\n\nDerrick Stolee (5):\n branch: add branch_checked_out() helper\n branch: check for bisects and rebases\n fetch: use new branch_checked_out() and add tests\n branch: use branch_checked_out() when deleting refs\n branch: fix branch_checked_out() leaks\n\n branch.c | 76 +++++++++++++++++++---\n branch.h | 7 +++\n builtin/branch.c | 7 +--\n builtin/fetch.c | 22 +++----\n t/t2407-worktree-heads.sh | 129 ++++++++++++++++++++++++++++++++++++++\n 5 files changed, 215 insertions(+), 26 deletions(-)\n create mode 100755 t/t2407-worktree-heads.sh\n\n\nbase-commit: 2668e3608e47494f2f10ef2b6e69f08a84816bcb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1254%2Fderrickstolee%2Fbranch-checked-out-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1254/derrickstolee/branch-checked-out-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1254\n\nRange-diff vs v1:\n\n 1: dbb7eae390c ! 1: bf701119edc branch: add branch_checked_out() helper\n @@ branch.c: int validate_branchname(const char *name, struct strbuf *ref)\n +\tfree_worktrees(worktrees);\n +}\n +\n -+int branch_checked_out(const char *refname, char **path)\n ++const char *branch_checked_out(const char *refname)\n +{\n -+\tconst char *path_in_set;\n +\tprepare_checked_out_branches();\n -+\n -+\tpath_in_set = strmap_get(¤t_checked_out_branches, refname);\n -+\tif (path_in_set && path)\n -+\t\t*path = xstrdup(path_in_set);\n -+\n -+\treturn !!path_in_set;\n ++\treturn strmap_get(¤t_checked_out_branches, refname);\n +}\n +\n /*\n @@ branch.h: void create_branches_recursively(struct repository *r, const char *nam\n \t\t\t\t int dry_run);\n +\n +/*\n -+ * Returns true if the branch at 'refname' is checked out at any\n -+ * non-bare worktree. The path of the worktree is stored in the\n -+ * given 'path', if provided.\n ++ * If the branch at 'refname' is currently checked out in a worktree,\n ++ * then return the path to that worktree.\n + */\n -+int branch_checked_out(const char *refname, char **path);\n ++const char *branch_checked_out(const char *refname);\n +\n /*\n * Check if 'name' can be a valid name for a branch; die otherwise.\n @@ t/t2407-worktree-heads.sh (new)\n +\n +test_description='test operations trying to overwrite refs at worktree HEAD'\n +\n ++TEST_PASSES_SANITIZE_LEAK=true\n +. ./test-lib.sh\n +\n +test_expect_success 'setup' '\n ++\ttest_commit init &&\n ++\tgit branch -f fake-1 &&\n ++\tgit branch -f fake-2 &&\n ++\n +\tfor i in 1 2 3 4\n +\tdo\n +\t\ttest_commit $i &&\n 2: 18bad9b0c49 ! 2: 9347303db89 branch: check for bisects and rebases\n @@ branch.c: static void prepare_checked_out_branches(void)\n \t}\n \n \tfree_worktrees(worktrees);\n -@@ branch.c: int branch_checked_out(const char *refname, char **path)\n +@@ branch.c: const char *branch_checked_out(const char *refname)\n */\n int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n {\n -\tstruct worktree **worktrees;\n -\tconst struct worktree *wt;\n -\n -+\tchar *path;\n ++\tconst char *path;\n \tif (!validate_branchname(name, ref))\n \t\treturn 0;\n \n @@ branch.c: int validate_new_branchname(const char *name, struct strbuf *ref, int\n -\tworktrees = get_worktrees();\n -\twt = find_shared_symref(worktrees, \"HEAD\", ref->buf);\n -\tif (wt && !wt->is_bare)\n -+\tif (branch_checked_out(ref->buf, &path))\n ++\tif ((path = branch_checked_out(ref->buf)))\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 @@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: checked out\n '\n \n +test_expect_success 'refuse to overwrite: worktree in bisect' '\n -+\ttest_when_finished test_might_fail git -C wt-4 bisect reset &&\n ++\ttest_when_finished rm -rf .git/worktrees/wt-*/BISECT_* &&\n +\n -+\t(\n -+\t\tgit -C wt-4 bisect start &&\n -+\t\tgit -C wt-4 bisect bad HEAD &&\n -+\t\tgit -C wt-4 bisect good HEAD~3\n -+\t) &&\n ++\ttouch .git/worktrees/wt-4/BISECT_LOG &&\n ++\techo refs/heads/fake-2 >.git/worktrees/wt-4/BISECT_START &&\n +\n -+\ttest_must_fail git branch -f wt-4 HEAD 2>err &&\n -+\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err\n ++\ttest_must_fail git branch -f fake-2 HEAD 2>err &&\n ++\tgrep \"cannot force update the branch '\\''fake-2'\\'' checked out at.*wt-4\" err\n +'\n +\n -+. \"$TEST_DIRECTORY\"/lib-rebase.sh\n -+\n +test_expect_success 'refuse to overwrite: worktree in rebase' '\n -+\ttest_when_finished test_might_fail git -C wt-4 rebase --abort &&\n ++\ttest_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&\n +\n -+\t(\n -+\t\tset_fake_editor &&\n -+\t\tFAKE_LINES=\"edit 1 2 3\" \\\n -+\t\t\tgit -C wt-4 rebase -i HEAD~3 >rebase &&\n -+\t\tgit -C wt-4 status\n -+\t) &&\n ++\tmkdir -p .git/worktrees/wt-3/rebase-merge &&\n ++\ttouch .git/worktrees/wt-3/rebase-merge/interactive &&\n ++\techo refs/heads/fake-1 >.git/worktrees/wt-3/rebase-merge/head-name &&\n ++\techo refs/heads/fake-2 >.git/worktrees/wt-3/rebase-merge/onto &&\n +\n -+\ttest_must_fail git branch -f wt-4 HEAD 2>err &&\n -+\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err\n ++\ttest_must_fail git branch -f fake-1 HEAD 2>err &&\n ++\tgrep \"cannot force update the branch '\\''fake-1'\\'' checked out at.*wt-3\" err\n +'\n +\n test_done\n 3: 4540dbeed38 ! 3: 1c764bfcfe4 fetch: use new branch_checked_out() and add tests\n @@ Commit message\n concurrent updates to the filesystem. Thus, it is beneficial to keep\n that extra check for the sake of defense-in-depth. However, we should\n not attempt to test the check, as the effort required is too\n - complicated to be worth the effort.\n + complicated to be worth the effort. This use in update_local_ref()\n + also requires a change in the error message because we no longer have\n + access to the worktree struct, only the path of the worktree. This error\n + is so rare that making a distinction between the two is not critical.\n \n Signed-off-by: Derrick Stolee \n \n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n {\n \tstruct commit *current = NULL, *updated;\n -\tconst struct worktree *wt;\n -+\tchar *path = NULL;\n \tconst char *pretty_ref = prettify_refname(ref->name);\n \tint fast_forward = 0;\n \n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n -\t (wt = find_shared_symref(worktrees, \"HEAD\", ref->name)) &&\n -\t !wt->is_bare && !is_null_oid(&ref->old_oid)) {\n +\t !is_null_oid(&ref->old_oid) &&\n -+\t branch_checked_out(ref->name, &path)) {\n ++\t branch_checked_out(ref->name)) {\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 @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n -\t\t\t wt->is_current ?\n -\t\t\t\t _(\"can't fetch in current branch\") :\n -\t\t\t\t _(\"checked out in another worktree\"),\n -+\t\t\t path ? _(\"can't fetch in current branch\") :\n -+\t\t\t\t _(\"checked out in another worktree\"),\n ++\t\t\t _(\"can't fetch into checked-out branch\"),\n \t\t\t remote, pretty_ref, summary_width);\n -+\t\tfree(path);\n \t\treturn 1;\n \t}\n - \n @@ builtin/fetch.c: cleanup:\n \treturn result;\n }\n @@ builtin/fetch.c: cleanup:\n +static void check_not_current_branch(struct ref *ref_map)\n {\n -\tconst struct worktree *wt;\n -+\tchar *path;\n ++\tconst char *path;\n \tfor (; ref_map; ref_map = ref_map->next)\n \t\tif (ref_map->peer_ref &&\n \t\t starts_with(ref_map->peer_ref->name, \"refs/heads/\") &&\n -\t\t (wt = find_shared_symref(worktrees, \"HEAD\",\n -\t\t\t\t\t ref_map->peer_ref->name)) &&\n -\t\t !wt->is_bare)\n -+\t\t branch_checked_out(ref_map->peer_ref->name, &path))\n ++\t\t (path = branch_checked_out(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 @@ t/t2407-worktree-heads.sh: test_expect_success 'setup' '\n +\tdone &&\n +\n +\t# Create a server that updates each branch by one commit\n -+\tgit clone . server &&\n ++\tgit init server &&\n ++\ttest_commit -C server initial &&\n +\tgit remote add server ./server &&\n +\tfor i in 1 2 3 4\n +\tdo\n -+\t\tgit -C server checkout wt-$i &&\n ++\t\tgit -C server checkout -b wt-$i &&\n +\t\ttest_commit -C server A-$i || return 1\n ++\tdone &&\n ++\tfor i in 1 2\n ++\tdo\n ++\t\tgit -C server checkout -b fake-$i &&\n ++\t\ttest_commit -C server f-$i || return 1\n \tdone\n '\n \n -@@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: checked out in worktree' '\n - \tdone\n +@@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: worktree in rebase' '\n + \tgrep \"cannot force update the branch '\\''fake-1'\\'' checked out at.*wt-3\" err\n '\n \n -+test_expect_success 'refuse to overwrite during fetch' '\n ++test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: checked out' '\n +\ttest_must_fail git fetch server +refs/heads/wt-3:refs/heads/wt-3 2>err &&\n +\tgrep \"refusing to fetch into branch '\\''refs/heads/wt-3'\\''\" err &&\n +\n @@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: checked out\n +\tgrep \"refusing to fetch into branch\" err\n +'\n +\n - test_expect_success 'refuse to overwrite: worktree in bisect' '\n - \ttest_when_finished test_might_fail git -C wt-4 bisect reset &&\n - \n -@@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: worktree in bisect' '\n - \t) &&\n - \n - \ttest_must_fail git branch -f wt-4 HEAD 2>err &&\n --\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err\n -+\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err &&\n ++test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: worktree in bisect' '\n ++\ttest_when_finished rm -rf .git/worktrees/wt-*/BISECT_* &&\n +\n -+\ttest_must_fail git fetch server +refs/heads/wt-4:refs/heads/wt-4 2>err &&\n -+\tgrep \"refusing to fetch into branch '\\''refs/heads/wt-4'\\''\" err\n - '\n - \n - . \"$TEST_DIRECTORY\"/lib-rebase.sh\n -@@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: worktree in rebase' '\n - \t) &&\n - \n - \ttest_must_fail git branch -f wt-4 HEAD 2>err &&\n --\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err\n -+\tgrep \"cannot force update the branch '\\''wt-4'\\'' checked out at\" err &&\n ++\ttouch .git/worktrees/wt-4/BISECT_LOG &&\n ++\techo refs/heads/fake-2 >.git/worktrees/wt-4/BISECT_START &&\n ++\n ++\ttest_must_fail git fetch server +refs/heads/fake-2:refs/heads/fake-2 2>err &&\n ++\tgrep \"refusing to fetch into branch\" err\n ++'\n ++\n ++test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: worktree in rebase' '\n ++\ttest_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&\n ++\n ++\tmkdir -p .git/worktrees/wt-4/rebase-merge &&\n ++\ttouch .git/worktrees/wt-4/rebase-merge/interactive &&\n ++\techo refs/heads/fake-1 >.git/worktrees/wt-4/rebase-merge/head-name &&\n ++\techo refs/heads/fake-2 >.git/worktrees/wt-4/rebase-merge/onto &&\n ++\n ++\ttest_must_fail git fetch server +refs/heads/fake-1:refs/heads/fake-1 2>err &&\n ++\tgrep \"refusing to fetch into branch\" err\n ++'\n +\n -+\ttest_must_fail git fetch server +refs/heads/wt-4:refs/heads/wt-4 2>err &&\n -+\tgrep \"refusing to fetch into branch '\\''refs/heads/wt-4'\\''\" err\n - '\n - \n test_done\n 4: af645b43032 ! 4: d7e63f9dfd6 branch: use branch_checked_out() when deleting refs\n @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int fo\n -\t\t\tconst struct worktree *wt =\n -\t\t\t\tfind_shared_symref(worktrees, \"HEAD\", name);\n -\t\t\tif (wt) {\n -+\t\t\tchar *path;\n -+\t\t\tif (branch_checked_out(name, &path)) {\n ++\t\t\tconst char *path;\n ++\t\t\tif ((path = branch_checked_out(name))) {\n \t\t\t\terror(_(\"Cannot delete branch '%s' \"\n \t\t\t\t\t\"checked out at '%s'\"),\n -\t\t\t\t bname.buf, wt->path);\n +\t\t\t\t bname.buf, path);\n -+\t\t\t\tfree(path);\n \t\t\t\tret = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n -: ----------- > 5: 0aa9478bc38 branch: fix branch_checked_out() leaks\n\n-- \ngitgitgadget\n"},{"id":"457210","messageId":"bf701119edcebd6801dafc1ffa3856ea2fde584a.1655234853.git.gitgitgadget@gmail.com","threadId":"57973","inReplyTo":"pull.1254.v2.git.1655234853.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] branch: add branch_checked_out() helper","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-14T19:27:29Z","receivedAt":"2022-06-14T19:27:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe validate_new_branchname() method contains a check to see if a branch\nis checked out in any non-bare worktree. This is intended to prevent a\nforce push that will mess up an existing checkout. This helper is not\nsuitable to performing just that check, because the method will die()\nwhen the branch is checked out instead of returning an error code.\n\nCreate a new branch_checked_out() helper that performs the most basic\nform of this check. To ensure we can call branch_checked_out() in a loop\nwith good performance, do a single preparation step that iterates over\nall worktrees and stores their current HEAD branches in a strmap. The\nbranch_checked_out() helper can then discover these branches using a\nhash lookup.\n\nThis helper is currently missing some key functionality. Namely: it\ndoesn't look for active rebases or bisects which mean that the branch is\n\"checked out\" even though HEAD doesn't point to that ref. This\nfunctionality will be added in a coming change.\n\nWe could use branch_checked_out() in validate_new_branchname(), but this\nmissing functionality would be a regression. However, we have no tests\nthat cover this case!\n\nAdd a new test script that will be expanded with these cross-worktree\nref updates. The current tests would still pass if we refactored\nvalidate_new_branchname() to use this version of branch_checked_out().\nThe next change will fix that functionality and add the proper test\ncoverage.\n\nSigned-off-by: Derrick Stolee \n---\n branch.c | 36 ++++++++++++++++++++++++++++++++++++\n branch.h | 7 +++++++\n t/t2407-worktree-heads.sh | 29 +++++++++++++++++++++++++++++\n 3 files changed, 72 insertions(+)\n create mode 100755 t/t2407-worktree-heads.sh\n\ndiff --git a/branch.c b/branch.c\nindex 2d6569b0c62..42ba60b7cb5 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -10,6 +10,7 @@\n #include \"worktree.h\"\n #include \"submodule-config.h\"\n #include \"run-command.h\"\n+#include \"strmap.h\"\n \n struct tracking {\n \tstruct refspec_item spec;\n@@ -369,6 +370,41 @@ int validate_branchname(const char *name, struct strbuf *ref)\n \treturn ref_exists(ref->buf);\n }\n \n+static int initialized_checked_out_branches;\n+static struct strmap current_checked_out_branches = STRMAP_INIT;\n+\n+static void prepare_checked_out_branches(void)\n+{\n+\tint i = 0;\n+\tstruct worktree **worktrees;\n+\n+\tif (initialized_checked_out_branches)\n+\t\treturn;\n+\tinitialized_checked_out_branches = 1;\n+\n+\tworktrees = get_worktrees();\n+\n+\twhile (worktrees[i]) {\n+\t\tstruct worktree *wt = worktrees[i++];\n+\n+\t\tif (wt->is_bare)\n+\t\t\tcontinue;\n+\n+\t\tif (wt->head_ref)\n+\t\t\tstrmap_put(¤t_checked_out_branches,\n+\t\t\t\t wt->head_ref,\n+\t\t\t\t xstrdup(wt->path));\n+\t}\n+\n+\tfree_worktrees(worktrees);\n+}\n+\n+const char *branch_checked_out(const char *refname)\n+{\n+\tprepare_checked_out_branches();\n+\treturn strmap_get(¤t_checked_out_branches, refname);\n+}\n+\n /*\n * Check if a branch 'name' can be created as a new branch; die otherwise.\n * 'force' can be used when it is OK for the named branch already exists.\ndiff --git a/branch.h b/branch.h\nindex 560b6b96a8f..ef56103c050 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -101,6 +101,13 @@ void create_branches_recursively(struct repository *r, const char *name,\n \t\t\t\t const char *tracking_name, int force,\n \t\t\t\t int reflog, int quiet, enum branch_track track,\n \t\t\t\t int dry_run);\n+\n+/*\n+ * If the branch at 'refname' is currently checked out in a worktree,\n+ * then return the path to that worktree.\n+ */\n+const char *branch_checked_out(const char *refname);\n+\n /*\n * Check if 'name' can be a valid name for a branch; die otherwise.\n * Return 1 if the named branch already exists; return 0 otherwise.\ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nnew file mode 100755\nindex 00000000000..305ab46e38e\n--- /dev/null\n+++ b/t/t2407-worktree-heads.sh\n@@ -0,0 +1,29 @@\n+#!/bin/sh\n+\n+test_description='test operations trying to overwrite refs at worktree HEAD'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit init &&\n+\tgit branch -f fake-1 &&\n+\tgit branch -f fake-2 &&\n+\n+\tfor i in 1 2 3 4\n+\tdo\n+\t\ttest_commit $i &&\n+\t\tgit branch wt-$i &&\n+\t\tgit worktree add wt-$i wt-$i || return 1\n+\tdone\n+'\n+\n+test_expect_success 'refuse to overwrite: checked out in worktree' '\n+\tfor i in 1 2 3 4\n+\tdo\n+\t\ttest_must_fail git branch -f wt-$i HEAD 2>err\n+\t\tgrep \"cannot force update the branch\" err || return 1\n+\tdone\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"457211","messageId":"9347303db8911187f1d257e0336dc9ca523d713c.1655234853.git.gitgitgadget@gmail.com","threadId":"57973","inReplyTo":"pull.1254.v2.git.1655234853.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] branch: check for bisects and rebases","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-14T19:27:30Z","receivedAt":"2022-06-14T19:27:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe branch_checked_out() helper was added by the previous change, but it\nused an over-simplified view to check if a branch is checked out. It\nonly focused on the HEAD symref, but ignored whether a bisect or rebase\nwas happening.\n\nTeach branch_checked_out() to check for these things, and also add tests\nto ensure that we do not lose this functionality in the future.\n\nNow that this test coverage exists, we can safely refactor\nvalidate_new_branchname() to use branch_checked_out().\n\nNote that we need to prepend \"refs/heads/\" to the 'state.branch' after\ncalling wt_status_check_*(). We also need to duplicate wt->path so the\nvalue is not freed at the end of the call.\n\nSigned-off-by: Derrick Stolee \n---\n branch.c | 35 +++++++++++++++++++++++++++--------\n t/t2407-worktree-heads.sh | 22 ++++++++++++++++++++++\n 2 files changed, 49 insertions(+), 8 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 42ba60b7cb5..d4ddec6f4e5 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -385,6 +385,7 @@ static void prepare_checked_out_branches(void)\n \tworktrees = get_worktrees();\n \n \twhile (worktrees[i]) {\n+\t\tstruct wt_status_state state = { 0 };\n \t\tstruct worktree *wt = worktrees[i++];\n \n \t\tif (wt->is_bare)\n@@ -394,6 +395,29 @@ static void prepare_checked_out_branches(void)\n \t\t\tstrmap_put(¤t_checked_out_branches,\n \t\t\t\t wt->head_ref,\n \t\t\t\t xstrdup(wt->path));\n+\n+\t\tif (wt_status_check_rebase(wt, &state) &&\n+\t\t (state.rebase_in_progress || state.rebase_interactive_in_progress) &&\n+\t\t state.branch) {\n+\t\t\tstruct strbuf ref = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&ref, \"refs/heads/%s\", state.branch);\n+\t\t\tstrmap_put(¤t_checked_out_branches,\n+\t\t\t\t ref.buf,\n+\t\t\t\t xstrdup(wt->path));\n+\t\t\tstrbuf_release(&ref);\n+\t\t}\n+\t\twt_status_state_free_buffers(&state);\n+\n+\t\tif (wt_status_check_bisect(wt, &state) &&\n+\t\t state.branch) {\n+\t\t\tstruct strbuf ref = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&ref, \"refs/heads/%s\", state.branch);\n+\t\t\tstrmap_put(¤t_checked_out_branches,\n+\t\t\t\t ref.buf,\n+\t\t\t\t xstrdup(wt->path));\n+\t\t\tstrbuf_release(&ref);\n+\t\t}\n+\t\twt_status_state_free_buffers(&state);\n \t}\n \n \tfree_worktrees(worktrees);\n@@ -413,9 +437,7 @@ const char *branch_checked_out(const char *refname)\n */\n int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n {\n-\tstruct worktree **worktrees;\n-\tconst struct worktree *wt;\n-\n+\tconst char *path;\n \tif (!validate_branchname(name, ref))\n \t\treturn 0;\n \n@@ -423,13 +445,10 @@ 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-\tworktrees = get_worktrees();\n-\twt = find_shared_symref(worktrees, \"HEAD\", ref->buf);\n-\tif (wt && !wt->is_bare)\n+\tif ((path = branch_checked_out(ref->buf)))\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-\tfree_worktrees(worktrees);\n+\t\t ref->buf + strlen(\"refs/heads/\"), path);\n \n \treturn 1;\n }\ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nindex 305ab46e38e..a838f2be474 100755\n--- a/t/t2407-worktree-heads.sh\n+++ b/t/t2407-worktree-heads.sh\n@@ -26,4 +26,26 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '\n \tdone\n '\n \n+test_expect_success 'refuse to overwrite: worktree in bisect' '\n+\ttest_when_finished rm -rf .git/worktrees/wt-*/BISECT_* &&\n+\n+\ttouch .git/worktrees/wt-4/BISECT_LOG &&\n+\techo refs/heads/fake-2 >.git/worktrees/wt-4/BISECT_START &&\n+\n+\ttest_must_fail git branch -f fake-2 HEAD 2>err &&\n+\tgrep \"cannot force update the branch '\\''fake-2'\\'' checked out at.*wt-4\" err\n+'\n+\n+test_expect_success 'refuse to overwrite: worktree in rebase' '\n+\ttest_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&\n+\n+\tmkdir -p .git/worktrees/wt-3/rebase-merge &&\n+\ttouch .git/worktrees/wt-3/rebase-merge/interactive &&\n+\techo refs/heads/fake-1 >.git/worktrees/wt-3/rebase-merge/head-name &&\n+\techo refs/heads/fake-2 >.git/worktrees/wt-3/rebase-merge/onto &&\n+\n+\ttest_must_fail git branch -f fake-1 HEAD 2>err &&\n+\tgrep \"cannot force update the branch '\\''fake-1'\\'' checked out at.*wt-3\" err\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"457212","messageId":"1c764bfcfe4d2a3233055c8f5175ebbf0076c6dd.1655234853.git.gitgitgadget@gmail.com","threadId":"57973","inReplyTo":"pull.1254.v2.git.1655234853.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] fetch: use new branch_checked_out() and add tests","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-14T19:27:31Z","receivedAt":"2022-06-14T19:27:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen fetching refs from a remote, it is possible that the refspec will\ncause use to overwrite a ref that is checked out in a worktree. The\nexisting logic in builtin/fetch.c uses a possibly-slow mechanism. Update\nthose sections to use the new, more efficient branch_checked_out()\nhelper.\n\nThese uses were not previously tested, so add a test case that can be\nused for these kinds of collisions. There is only one test now, but more\ntests will be added as other consumers of branch_checked_out() are\nadded.\n\nNote that there are two uses in builtin/fetch.c, but only one of the\nmessages is tested. This is because the tested check is run before\ncompleting the fetch, and the untested check is not reachable without\nconcurrent updates to the filesystem. Thus, it is beneficial to keep\nthat extra check for the sake of defense-in-depth. However, we should\nnot attempt to test the check, as the effort required is too\ncomplicated to be worth the effort. This use in update_local_ref()\nalso requires a change in the error message because we no longer have\naccess to the worktree struct, only the path of the worktree. This error\nis so rare that making a distinction between the two is not critical.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/fetch.c | 22 +++++++-----------\n t/t2407-worktree-heads.sh | 47 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ac29c2b1ae3..7fdbfee5c93 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -885,7 +885,6 @@ static int update_local_ref(struct ref *ref,\n \t\t\t struct worktree **worktrees)\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@@ -900,16 +899,14 @@ static int update_local_ref(struct ref *ref,\n \t}\n \n \tif (!update_head_ok &&\n-\t (wt = find_shared_symref(worktrees, \"HEAD\", ref->name)) &&\n-\t !wt->is_bare && !is_null_oid(&ref->old_oid)) {\n+\t !is_null_oid(&ref->old_oid) &&\n+\t branch_checked_out(ref->name)) {\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 wt->is_current ?\n-\t\t\t\t _(\"can't fetch in current branch\") :\n-\t\t\t\t _(\"checked out in another worktree\"),\n+\t\t\t _(\"can't fetch into checked-out branch\"),\n \t\t\t remote, pretty_ref, summary_width);\n \t\treturn 1;\n \t}\n@@ -1434,19 +1431,16 @@ cleanup:\n \treturn result;\n }\n \n-static void check_not_current_branch(struct ref *ref_map,\n-\t\t\t\t struct worktree **worktrees)\n+static void check_not_current_branch(struct ref *ref_map)\n {\n-\tconst struct worktree *wt;\n+\tconst char *path;\n \tfor (; ref_map; ref_map = ref_map->next)\n \t\tif (ref_map->peer_ref &&\n \t\t starts_with(ref_map->peer_ref->name, \"refs/heads/\") &&\n-\t\t (wt = find_shared_symref(worktrees, \"HEAD\",\n-\t\t\t\t\t ref_map->peer_ref->name)) &&\n-\t\t !wt->is_bare)\n+\t\t (path = branch_checked_out(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+\t\t\t ref_map->peer_ref->name, path);\n }\n \n static int truncate_fetch_head(void)\n@@ -1650,7 +1644,7 @@ static int do_fetch(struct transport *transport,\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, worktrees);\n+\t\tcheck_not_current_branch(ref_map);\n \n \tretcode = open_fetch_head(&fetch_head);\n \tif (retcode)\ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nindex a838f2be474..1fbde05fe2b 100755\n--- a/t/t2407-worktree-heads.sh\n+++ b/t/t2407-worktree-heads.sh\n@@ -15,6 +15,21 @@ test_expect_success 'setup' '\n \t\ttest_commit $i &&\n \t\tgit branch wt-$i &&\n \t\tgit worktree add wt-$i wt-$i || return 1\n+\tdone &&\n+\n+\t# Create a server that updates each branch by one commit\n+\tgit init server &&\n+\ttest_commit -C server initial &&\n+\tgit remote add server ./server &&\n+\tfor i in 1 2 3 4\n+\tdo\n+\t\tgit -C server checkout -b wt-$i &&\n+\t\ttest_commit -C server A-$i || return 1\n+\tdone &&\n+\tfor i in 1 2\n+\tdo\n+\t\tgit -C server checkout -b fake-$i &&\n+\t\ttest_commit -C server f-$i || return 1\n \tdone\n '\n \n@@ -48,4 +63,36 @@ test_expect_success 'refuse to overwrite: worktree in rebase' '\n \tgrep \"cannot force update the branch '\\''fake-1'\\'' checked out at.*wt-3\" err\n '\n \n+test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: checked out' '\n+\ttest_must_fail git fetch server +refs/heads/wt-3:refs/heads/wt-3 2>err &&\n+\tgrep \"refusing to fetch into branch '\\''refs/heads/wt-3'\\''\" err &&\n+\n+\t# General fetch into refs/heads/ will fail on first ref,\n+\t# so use a generic error message check.\n+\ttest_must_fail git fetch server +refs/heads/*:refs/heads/* 2>err &&\n+\tgrep \"refusing to fetch into branch\" err\n+'\n+\n+test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: worktree in bisect' '\n+\ttest_when_finished rm -rf .git/worktrees/wt-*/BISECT_* &&\n+\n+\ttouch .git/worktrees/wt-4/BISECT_LOG &&\n+\techo refs/heads/fake-2 >.git/worktrees/wt-4/BISECT_START &&\n+\n+\ttest_must_fail git fetch server +refs/heads/fake-2:refs/heads/fake-2 2>err &&\n+\tgrep \"refusing to fetch into branch\" err\n+'\n+\n+test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: worktree in rebase' '\n+\ttest_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&\n+\n+\tmkdir -p .git/worktrees/wt-4/rebase-merge &&\n+\ttouch .git/worktrees/wt-4/rebase-merge/interactive &&\n+\techo refs/heads/fake-1 >.git/worktrees/wt-4/rebase-merge/head-name &&\n+\techo refs/heads/fake-2 >.git/worktrees/wt-4/rebase-merge/onto &&\n+\n+\ttest_must_fail git fetch server +refs/heads/fake-1:refs/heads/fake-1 2>err &&\n+\tgrep \"refusing to fetch into branch\" err\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"457213","messageId":"d7e63f9dfd608d7d00c08bb57b03310a884b2e8e.1655234853.git.gitgitgadget@gmail.com","threadId":"57973","inReplyTo":"pull.1254.v2.git.1655234853.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] branch: use branch_checked_out() when deleting refs","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-14T19:27:32Z","receivedAt":"2022-06-14T19:27:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThis is the last current use of find_shared_symref() that can easily be\nreplaced by branch_checked_out(). The benefit of this switch is that the\ncode is a bit simpler, but also it is faster on repeated calls.\n\nThe remaining uses of find_shared_symref() are non-trivial to remove, so\nwe probably should not continue in that direction:\n\n* builtin/notes.c uses find_shared_symref() with \"NOTES_MERGE_REF\"\n instead of \"HEAD\", so it doesn't have an immediate analogue with\n branch_checked_out(). Perhaps we should consider extending it to\n include that symref in addition to HEAD, BISECT_HEAD, and\n REBASE_HEAD.\n\n* receive-pack.c checks to see if a worktree has a checkout for the ref\n that is being updated. The tricky part is that it can actually decide\n to update the worktree directly instead of just skipping the update.\n This all depends on the receive.denyCurrentBranch config option. The\n implementation currenty cares about receiving the worktree in the\n result, so the current branch_checked_out() prototype is insufficient\n currently. This is something to investigate later, though, since a\n large number of refs could be updated at the same time and using the\n strmap implementation of branch_checked_out() could be beneficial.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/branch.c | 7 +++----\n t/t2407-worktree-heads.sh | 5 ++++-\n 2 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 5d00d0b8d32..f875952e7b5 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -253,12 +253,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tname = mkpathdup(fmt, bname.buf);\n \n \t\tif (kinds == FILTER_REFS_BRANCHES) {\n-\t\t\tconst struct worktree *wt =\n-\t\t\t\tfind_shared_symref(worktrees, \"HEAD\", name);\n-\t\t\tif (wt) {\n+\t\t\tconst char *path;\n+\t\t\tif ((path = branch_checked_out(name))) {\n \t\t\t\terror(_(\"Cannot delete branch '%s' \"\n \t\t\t\t\t\"checked out at '%s'\"),\n-\t\t\t\t bname.buf, wt->path);\n+\t\t\t\t bname.buf, path);\n \t\t\t\tret = 1;\n \t\t\t\tcontinue;\n \t\t\t}\ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nindex 1fbde05fe2b..a5aec1486c5 100755\n--- a/t/t2407-worktree-heads.sh\n+++ b/t/t2407-worktree-heads.sh\n@@ -37,7 +37,10 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '\n \tfor i in 1 2 3 4\n \tdo\n \t\ttest_must_fail git branch -f wt-$i HEAD 2>err\n-\t\tgrep \"cannot force update the branch\" err || return 1\n+\t\tgrep \"cannot force update the branch\" err &&\n+\n+\t\ttest_must_fail git branch -D wt-$i 2>err\n+\t\tgrep \"Cannot delete branch\" err || return 1\n \tdone\n '\n \n-- \ngitgitgadget\n\n"},{"id":"457214","messageId":"0aa9478bc3815726a47ea99cac0fd333e95c1584.1655234853.git.gitgitgadget@gmail.com","threadId":"57973","inReplyTo":"pull.1254.v2.git.1655234853.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] branch: fix branch_checked_out() leaks","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-14T19:27:33Z","receivedAt":"2022-06-14T19:28:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe branch_checked_out() method populates a strmap linking a refname to\na worktree that has that branch checked out. While unlikely, it is\npossible that a bug or filesystem manipulation could create a scenario\nwhere the same ref is checked out in multiple places. Further, there are\nsome states in an interactive rebase where HEAD and REBASE_HEAD point to\nthe same ref, leading to multiple insertions into the strmap. In either\ncase, the strmap_put() method returns the old value which is leaked.\n\nUpdate branch_checked_out() to consume that pointer and free it.\n\nAdd a test in t2407 that checks this erroneous case. The test \"checks\nitself\" by first confirming that the filesystem manipulations it makes\ntrigger the branch_checked_out() logic, and then sets up similar\nmanipulations to make it look like there are multiple worktrees pointing\nto the same ref.\n\nWhile TEST_PASSES_SANITIZE_LEAK would be helpful to demonstrate the\nleakage and prevent it in the future, t2407 uses helpers such as 'git\nclone' that cause the test to fail under that mode.\n\nSigned-off-by: Derrick Stolee \n---\n branch.c | 25 +++++++++++++++----------\n t/t2407-worktree-heads.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 10 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex d4ddec6f4e5..6007ee7baeb 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -385,25 +385,29 @@ static void prepare_checked_out_branches(void)\n \tworktrees = get_worktrees();\n \n \twhile (worktrees[i]) {\n+\t\tchar *old;\n \t\tstruct wt_status_state state = { 0 };\n \t\tstruct worktree *wt = worktrees[i++];\n \n \t\tif (wt->is_bare)\n \t\t\tcontinue;\n \n-\t\tif (wt->head_ref)\n-\t\t\tstrmap_put(¤t_checked_out_branches,\n-\t\t\t\t wt->head_ref,\n-\t\t\t\t xstrdup(wt->path));\n+\t\tif (wt->head_ref) {\n+\t\t\told = strmap_put(¤t_checked_out_branches,\n+\t\t\t\t\t wt->head_ref,\n+\t\t\t\t\t xstrdup(wt->path));\n+\t\t\tfree(old);\n+\t\t}\n \n \t\tif (wt_status_check_rebase(wt, &state) &&\n \t\t (state.rebase_in_progress || state.rebase_interactive_in_progress) &&\n \t\t state.branch) {\n \t\t\tstruct strbuf ref = STRBUF_INIT;\n \t\t\tstrbuf_addf(&ref, \"refs/heads/%s\", state.branch);\n-\t\t\tstrmap_put(¤t_checked_out_branches,\n-\t\t\t\t ref.buf,\n-\t\t\t\t xstrdup(wt->path));\n+\t\t\told = strmap_put(¤t_checked_out_branches,\n+\t\t\t\t\t ref.buf,\n+\t\t\t\t\t xstrdup(wt->path));\n+\t\t\tfree(old);\n \t\t\tstrbuf_release(&ref);\n \t\t}\n \t\twt_status_state_free_buffers(&state);\n@@ -412,9 +416,10 @@ static void prepare_checked_out_branches(void)\n \t\t state.branch) {\n \t\t\tstruct strbuf ref = STRBUF_INIT;\n \t\t\tstrbuf_addf(&ref, \"refs/heads/%s\", state.branch);\n-\t\t\tstrmap_put(¤t_checked_out_branches,\n-\t\t\t\t ref.buf,\n-\t\t\t\t xstrdup(wt->path));\n+\t\t\told = strmap_put(¤t_checked_out_branches,\n+\t\t\t\t\t ref.buf,\n+\t\t\t\t\t xstrdup(wt->path));\n+\t\t\tfree(old);\n \t\t\tstrbuf_release(&ref);\n \t\t}\n \t\twt_status_state_free_buffers(&state);\ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nindex a5aec1486c5..b6be42f74a2 100755\n--- a/t/t2407-worktree-heads.sh\n+++ b/t/t2407-worktree-heads.sh\n@@ -98,4 +98,32 @@ test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: worktree in rebase\n \tgrep \"refusing to fetch into branch\" err\n '\n \n+test_expect_success 'refuse to overwrite when in error states' '\n+\ttest_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&\n+\ttest_when_finished rm -rf .git/worktrees/wt-*/BISECT_* &&\n+\n+\t# Both branches are currently under rebase.\n+\tmkdir -p .git/worktrees/wt-3/rebase-merge &&\n+\ttouch .git/worktrees/wt-3/rebase-merge/interactive &&\n+\techo refs/heads/fake-1 >.git/worktrees/wt-3/rebase-merge/head-name &&\n+\techo refs/heads/fake-2 >.git/worktrees/wt-3/rebase-merge/onto &&\n+\tmkdir -p .git/worktrees/wt-4/rebase-merge &&\n+\ttouch .git/worktrees/wt-4/rebase-merge/interactive &&\n+\techo refs/heads/fake-2 >.git/worktrees/wt-4/rebase-merge/head-name &&\n+\techo refs/heads/fake-1 >.git/worktrees/wt-4/rebase-merge/onto &&\n+\n+\t# Both branches are currently under bisect.\n+\ttouch .git/worktrees/wt-4/BISECT_LOG &&\n+\techo refs/heads/fake-2 >.git/worktrees/wt-4/BISECT_START &&\n+\ttouch .git/worktrees/wt-1/BISECT_LOG &&\n+\techo refs/heads/fake-1 >.git/worktrees/wt-1/BISECT_START &&\n+\n+\tfor i in 1 2\n+\tdo\n+\t\ttest_must_fail git branch -f fake-$i HEAD 2>err &&\n+\t\tgrep \"cannot force update the branch '\\''fake-$i'\\'' checked out at\" err ||\n+\t\t\treturn 1\n+\tdone\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"457533","messageId":"e562bd1b-8be6-db70-923a-cfa27c3edb36@gmail.com","threadId":"57973","inReplyTo":"pull.1254.v2.git.1655234853.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/5] Create branch_checked_out() helper","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-06-19T13:58:28Z","receivedAt":"2022-06-19T13:58:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stolee\n\nOn 14/06/2022 20:27, Derrick Stolee via GitGitGadget wrote:\n> This is a replacement for some patches from v2 of my 'git rebase\n> --update-refs' topic [1]. After some feedback from Philip, I've decided to\n> pull that topic while I rework how I track the refs to rewrite [2]. This\n> series moves forward with the branch_checked_out() helper that was a bit\n> more complicated than expected at first glance. This series is a culmination\n> of the discussion started by Junio at [3].\n> \n> [1]\n> https://lore.kernel.org/git/pull.1247.v2.git.1654634569.gitgitgadget@gmail.com\n> [2]\n> https://lore.kernel.org/git/34264915-8a37-62db-f954-0b5297639b34@github.com/\n> [3] https://lore.kernel.org/git/xmqqr140t9am.fsf@gitster.g/\n> \n> Here is the patch outline:\n> \n> 1. Add a basic form of branch_checked_out() that only looks at the HEAD of\n> each worktree. Since this doesn't look at BISECT_HEAD or REBASE_HEAD,\n> the helper isn't linked to any consumer in this patch. A test script is\n> added that verifies the current behavior checks that are implemented,\n> even if they are not connected yet.\n> 2. Make branch_checked_out() actually look at BISECT_HEAD and REBASE_HEAD.\n> Add tests for those cases, which was previously untested in the Git test\n> suite. Use branch_checked_out() in 'git branch -f' to demonstrate this\n> helper works as expected.\n> 3. Use branch_checked_out() in 'git fetch' when checking refs that would be\n> updated by the refspec. Add tests for that scenario, too.\n> 4. Use branch_checked_out() in 'git branch -D' to prevent deleting refs\n> from under an existing checkout. The commit message here describes some\n> barriers to removing other uses of find_shared_symref() that could be\n> good investigations for later.\n> 5. Be careful when constructing the strmap to free old values, even though\n> this should only happen in error scenarios. Add tests that verify this\n> behavior.\n> \n> \n> Updates in v2\n> =============\n> \n> * branch_checked_out() has a new prototype where it returns a 'const char\n> *' instead of copying the path.\n> * The test script is marked with TEST_PASSES_SANITIZE_LEAK and test are\n> careful to avoid using 'git rebase' or 'git bisect' when possible.\n> * Tests that cannot avoid memory-loss from 'git fetch' are marked with the\n> \"!SANITIZE_LEAK\" prereq.\n> * A previous replacement of 'wt->current' with 'path' is removed. This\n> changes an error message, but it is a very rare scenario where this error\n> would be output.\n\nThe changes look good to me (modulo Peff's fixup), thanks for working on \nthis\n\nBest Wishes\n\nPhillip\n"}]}