From: Phillip Wood Date: Mon, 16 Feb 2026 16:18:58 GMT Subject: Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree Message-ID: <66b0f03a-36ab-4305-814e-6d964f5d33c4@gmail.com> In-Reply-To: <20260215090815.46544-1-shreyanshpaliwalcmsmn@gmail.com> On 15/02/2026 08:56, Shreyansh Paliwal wrote: >> I've cc'd Eric for a second opinion >> >> On 13/02/2026 22:29, Junio C Hamano wrote: >>> Shreyansh Paliwal 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. >> >> 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. >> The worktree id is the last path component of repo->gitdir when the >> repo->gitdir and repo->commondir differ, otherwise it is NULL. Then we >> can use that function to get the current worktree rather than passing >> NULL when we call wt_status_check_{rebase,bisect} from >> wt_status_get_state(). 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. > > Thanks a lot for clarifying. This helps solve the doubt regarding the > different usage of !wt and !wt->id in strbuf_worktree_gitdir(). I realize > we have been under the wrong assumption about what wt == NULL represents. > > But I still have a few points where I’m a bit confused, > > If wt == NULL is meant to represent the current worktree, then what role > wt->is_current plays in the present implementation, and if they both > represent the same thing then wt->is_current wouldn't make sense if wt is > already NULL in the case of a current worktree. wt == NULL is a shorthand that callers can use if they don't have a struct worktree to pass, it does not replace wt->is_current when listing all worktrees with get_worktrees() which returns a NULL terminated list. > Beyond representation, I’m not quite understanding on how call sites are > logically differentiating on whether the intent is to 'operate on the > worktree we are in' or 'operate on the primary one'. We're nearly always interested in the current one. The primary worktree is special in that it cannot be moved or deleted with "git worktree" but git commands generally operate on the current worktree and occasionally check the state of other worktrees (for example to avoid checking out the same branch in two different worktrees). > And I think if we included both in struct repository (r->main_wt, r->current_wt) > so accessing either of them would be a whole lot easier and also would > prevent confusion in the future. It might be worth adding the current worktree (or probably the worktree that the struct repository refers to) to struct repository in the future but I think that is outside the scope of cleaning up wt-status.c Thanks Phillip > Let me know what you think. > > Best, > Shreyansh >