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!
>
> Signed-off-by: Anders Kaseorg <andersk@xxxxxxx>
> ---
> 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
