From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 18 Feb 2026 14:18:50 GMT
Subject: Re: [PATCH 1/2] wt-status: avoid passing NULL worktree
Message-ID: <f03fa5a8-b408-4b4e-a254-b0a39b87e636@gmail.com>
In-Reply-To: <xmqqa4x7cile.fsf@gitster.g>

On 17/02/2026 18:47, Junio C Hamano wrote:
> 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().
> 
> Hmph, I am afraid that
> 
>      "Construct a struct worktree from a struct repository"
> 
> is not quite sufficient.  A repository can have more than one
> worktrees, so if you give a repository as a parameter, there needs a
> way for the implementation of this helper function to identify which
> one of them to construct a struct worktree for, and more importantly
> for you as the caller to be able to expect which one the implementation
> would pick, and what that particular worktree among many _means_ to you.
> 
> I know that the implementation uses repo->worktree but what does
> that path mean in the world-view of the worktree API set?

While a repository can have multiple worktrees, a "struct repository" 
points to a particular worktree within that repository via the gitdir 
and worktree members. I'll try and make it clearer that the function 
returns a struct worktree corresponding to those members.

> I am guessing that it is what the worktree API calls "current", but
> if so, perhaps the function should be explained with that word in
> it, and the function name should also contain that word, no?

That's what I thought initially. However is_current_worktree() is 
defined in terms of "the_repository" rather than "wt->repo". That means 
all the struct worktrees within a single process agree on the "current" 
worktree but it is suprising that if "wt->path" matches 
"wt->repo->worktree" it is not necessarily the "current" worktree. I'm 
not sure if we want to change the definition of is_current_worktree() to 
use "wt->repo" rather than "the_repository", but if we do I think we can 
do that separately.

>> +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);
> 
> So, if the repository instance knows where the worktree is, we use
> that to wt->path.  Otherwise wt->path is left NULL.

That's actually a bug, we should be using repo->gitdir when the 
repository is bare.

>> +	wt->is_bare = !!repo->worktree;
> 
> I may be confused but don't we have one ! too many?  If we have a
> worktree directory, "git checkout" would check the files there, and
> that is not quite a "bare" repository, no?

Yes, it should be "wt->is_bare = !repo->worktree;"

>> +	if (fspathcmp(gitdir, commondir))
>> +		wt->id = xstrdup(find_last_dir_sep(commondir) + 1);
> 
> OK.  So gitdir and commondir would be the same for the primary and
> for everybody else we'd have "id" as the last directory component of
> the commondir.
> 
>> +	wt->is_current = is_current_worktree(wt);
> 
> Oh, so I guessed wrong and this is not about "current" worktree?
> What does the directory pointed at by repo->worktree mean to the
> callers of this function?  I somehow thought that is_current would
> be always 1 here,.

As explained above "repo->worktree" means nothing to 
is_current_worktree() because it uses "the_repository" instead of 
"wt->repo".

I'll re-roll with the fixes above and a bit more detail in the commit 
message about is_current_worktree() and the that the "struct worktree" 
instance corresponds to worktree that uses repo->gitdir

Thanks

Phillip