Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree

[Junio C Hamano <gitster@pobox.com>]

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

> I've cc'd Eric for a second opinion
>
> On 13/02/2026 22:29, Junio C Hamano wrote:
>> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
>> 
>>> diff --git a/path.c b/path.c
>>> index d726537622..4ac86e1e58 100644
>>> --- a/path.c
>>> +++ b/path.c
>>> @@ -408,9 +408,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,
>>>   				   const struct repository *repo,
>>>   				   const struct worktree *wt)
>>>   {
>>> -	if (!wt)
>>> -		strbuf_addstr(buf, repo->gitdir);
>>> -	else if (!wt->id)
>>> +	if (is_main_worktree(wt))
>>>   		strbuf_addstr(buf, repo->commondir);
>>>   	else
>>>   		repo_common_path_append(repo, buf, "worktrees/%s", wt->id);
>> 
>> This is curious.
>> 
>> We used to treat "wt==NULL" and "wt->id==NULL" differently.  Now we
>> use repo->commondir for both.  For the primary worktree, it ought to
>> be the same as repo->gitdir, so it should not matter, but makes me
>> wonder what the reason behind this difference in the original.
>> 
>> We have been assuming that wt==NULL and wt->id==NULL both meant the
>> same thing: "we are talking about the primary worktree".  But the
>> code around here before this patch seems to behave differently.  Is
>> our assumption incorrect and are we making a mistake by conflating
>> these two conditions into one?
>
> My understanding is that wt==NULL means "use the current worktree" and 
> wt->id==NULL means "this is the main worktree". That would explain why 
> we use repo->gitdir above when wt==NULL and repo->commondir when 
> wt->id==NULL, as repo->gitdir is the gitdir of the current worktree and 
> repo->commondir will be the gitdir of the main worktree. If we look at 
> the code in wt-status.c that's passing a NULL worktree it wants to know 
> about the status of the current worktree, not the main worktree.

Oh, boy.  If that is what wt==NULL means, the above confusion about
the original is perfectly cleared.  We have been operating under a
totally wrong assumption.

> I think that we should add a new function
>
> struct worktree *get_current_worktree(struct repository*);
>
> to worktree.c that constructs a struct worktree using repo->gitdir etc. 

Certainly.

> We should also think about whether we should 
> change wt_status_get_state() to take a "struct worktree *" rather than a 
> "struct repository *" instead (I've not looked at the callers to see if 
> that's sensible).
>
> With that, we can gradually clean up uses of wt==NULL in the rest of the 
> codebase overtime and eventually remove support for it from worktree.c 
> rather than having a big flag-day patch. I don't think we need to change 
> uses of wt-id==NULL.

OK.  I think wt->id==NULL vs wt->id=="/" is about correcting
inconsistencies between the worktree.c and refs.c and certainly can
be done in a separate patch.

We probably should think about how often we use the current one
(presumably almost all the time, given that even wt-status.c API
functions seem to take one), and if the current implementation is
the best way to signal, among a list of worktrees, which one is the
current and which one is the primary.

I do not mind too much about "the primary sits at the beginning of
the resulting list all the time" convention, but wt->is_current bit
looks like a disaster waiting to happen.  To find the current one,
you need to construct a full list and then iterate over the list to
find one with that bit on?  What if there is nobody with the bit or
more than one?  Are callers prepared to notice and report such bugs?

As you said, comparison between gitdir and commondir is sufficient,
then we can lose that bit.  One fewer thing that can go out of sync
takes us one step closer to a cleaner world.

Thanks.