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@mit.edu>
> ---
>  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