Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree
- From
Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
- Date
- Feb 15, 2026, 08:56 UTC
- Message-ID
- <20260215090815.46544-1-shreyanshpaliwalcmsmn@gmail.com>
- In-Reply-To
- <ebc16a74-0555-4951-8ec6-ff7fce6b6fcc@gmail.com>
Show 60 quoted lines
> 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.
>
> 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.
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'.
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.
Let me know what you think.
Best, Shreyansh