From: Karthik Nayak <karthik.188@gmail.com>
Date: Tue, 17 Feb 2026 17:46:21 GMT
Subject: Re: [PATCH 1/2] wt-status: avoid passing NULL worktree
Message-ID: <CAOLa=ZQKLqFn4w3s7PD87FZ_120gohoqKX5c3uLKo2vASsbxfA@mail.gmail.com>
In-Reply-To: <409871a7d521b76c9eb811d3c49747e04de8defc.1771258688.git.phillip.wood@dunelm.org.uk>

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> In preparation for removing the repository argument from
> worktree_git_path() add a function to construct a "struct worktree"
> from a "struct repository" and use that to avoid passing a NULL
> worktree to wt_status_check_bisect() and wt_status_check_rebase().
>

Okay this makes sense, I'm curious how 'wt->id = NULL' is going to be
handled. Let's see

> wt_status_check_bisect() and wt_status_check_rebase() have the following
> callers:
>
>  - branch.c:prepare_checked_out_branches() which loops over all
>    worktrees.
>
>  - worktree.c:is_worktree_being_rebased() which is called from
>    builtin/branch.c:reject_rebase_or_bisect_branch() that loops over all
>    worktrees and worktree.c:is_shared_symref() which dereferences wt
>    earlier in the function.
>
>  - wt-status:wt_status_get_state() which is updated to avoid passing a
>    NULL worktree by this patch.
>
> This updates the only callers that pass a NULL worktree to
> worktree_git_path().
>

I was thinking surely there must be other places where we also pass NULL
for worktree, but doesn't seem like there are any such instances.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  worktree.c  | 20 ++++++++++++++++++++
>  worktree.h  |  5 ++++-
>  wt-status.c | 15 ++++++++++++---
>  3 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/worktree.c b/worktree.c
> index 9308389cb6f..fd182c319b7 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -66,6 +66,26 @@ static int is_current_worktree(struct worktree *wt)
>  	return is_current;
>  }
>
> +struct worktree *get_worktree_from_repository(struct repository *repo)
> +{
> +	struct worktree *wt = xcalloc(1, sizeof(*wt));
> +	char *gitdir = absolute_pathdup(repo->gitdir);
> +	char *commondir = absolute_pathdup(repo->commondir);
> +
> +	wt->repo = repo;
> +	if (repo->worktree)
> +		wt->path = absolute_pathdup(repo->worktree);

Shouldn't this always be set? I guess my question is, will
`repo->worktree` ever be NULL?

> +	wt->is_bare = !!repo->worktree;
> +	if (fspathcmp(gitdir, commondir))
> +		wt->id = xstrdup(find_last_dir_sep(commondir) + 1);

So here we continue to treat NULL as the main worktree. Okay.

> +	wt->is_current = is_current_worktree(wt);

Since we're getting the worktree from the repo, shouldn't this be
'true'?

> +	add_head_info(wt);
> +
> +	free(gitdir);
> +	free(commondir);
> +	return wt;
> +}
> +
>  /*
>  * When in a secondary worktree, and when extensions.worktreeConfig
>  * is true, only $commondir/config and $commondir/worktrees/<id>/
> diff --git a/worktree.h b/worktree.h
> index e4bcccdc0ae..b162bbabd50 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -38,7 +38,10 @@ struct worktree **get_worktrees(void);
>   */
>  struct worktree **get_worktrees_without_reading_head(void);
>
> -/*
> +/* Construct a struct worktree from a struct repository */
> +struct worktree *get_worktree_from_repository(struct repository *repo);
> +
> + /*

Nit: extra space?

>   * Returns 1 if linked worktrees exist, 0 otherwise.
>   */
>  int submodule_uses_worktrees(const char *path);

[snip]