Re: [PATCH v6 4/8] worktree: simplify find_shared_symref() memory ownership model
flat view
From: Johannes Schindelin <hidden>
Date: 2021-11-22 12:45:44
Hi Anders, tl;dr this looks great! On Fri, 12 Nov 2021, Anders Kaseorg wrote:
Storing the worktrees list in a static variable meant that find_shared_symref() had to rebuild the list on each call (which is inefficient when the call site is in a loop), and also that each call invalidated the pointer returned by the previous call (which is confusing). Instead, make it the caller’s responsibility to pass in the worktrees list and manage its lifetime.
Thank you for cleaning this up!
quoted hunk ↗ jump to hunk
Signed-off-by: Anders Kaseorg <redacted> --- branch.c | 14 ++++++---- builtin/branch.c | 7 ++++- builtin/notes.c | 6 +++- builtin/receive-pack.c | 63 +++++++++++++++++++++++++++--------------- worktree.c | 8 ++---- worktree.h | 5 ++-- 6 files changed, 65 insertions(+), 38 deletions(-)diff --git a/branch.c b/branch.c index 147827cf46..c7b9ba0e10 100644 --- a/branch.c +++ b/branch.c@@ -357,14 +357,16 @@ void remove_branch_state(struct repository *r, int verbose) void die_if_checked_out(const char *branch, int ignore_current_worktree) { + struct worktree **worktrees = get_worktrees(); const struct worktree *wt; - wt = find_shared_symref("HEAD", branch); - if (!wt || (ignore_current_worktree && wt->is_current)) - return; - skip_prefix(branch, "refs/heads/", &branch); - die(_("'%s' is already checked out at '%s'"), - branch, wt->path); + wt = find_shared_symref(worktrees, "HEAD", branch); + if (wt && (!ignore_current_worktree || !wt->is_current)) { + skip_prefix(branch, "refs/heads/", &branch); + die(_("'%s' is already checked out at '%s'"), branch, wt->path); + } + + free_worktrees(worktrees);
This is the only caller that is not in `builtin/`, i.e. it is not at once clear how many times we would potentially re-generate the list. I had a quick look: $ git grep die_if_checked_out branch.c:void die_if_checked_out(const char *branch, int ignore_current_worktree) branch.h:void die_if_checked_out(const char *branch, int ignore_current_worktree); builtin/checkout.c: die_if_checked_out(new_branch_info->path, 1); builtin/rebase.c: die_if_checked_out(buf.buf, 1); builtin/worktree.c: die_if_checked_out(symref.buf, 0); builtin/worktree.c: die_if_checked_out(symref.buf, 0); This suggests that all the callers of the `die_if_checked_out()` are in the `builtin/` part, and only in the commands that were not touched directly by your patch. Which means that all is fine and dandy, we are unlikely to introduce a code flow where the worktrees array is populated multiple times (when before, it would only have been generated only once). Very good. Ciao, Dscho