Re: [PATCH V2 2/3] wt-status: pass struct repository and wt_status through function parameters

From: Karthik Nayak <karthik.188@gmail.com>
Date: Feb 9, 2026, 09:02 UTC
Message-ID: <CAOLa=ZRaWA14sootWSPo5g4Yi4GBXf6HjdkdBY1Tt_+V0szCjg@mail.gmail.com>
In-Reply-To: <20260208152811.73213-1-shreyanshpaliwalcmsmn@gmail.com>

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

[snip]

Show 68 quoted lines

>> > Actually adding another repository parameter to both of these functions
>> > is needed because of the calls like wt_status_check_rebase(NULL, state)
>> > and wt_status_check_bisect(NULL, state) from wt_status_get_state().
>> > In the case where wt is NULL, accessing wt->repo can lead to issues.
>>
>> But stopping thought at that point is not a reasonable thing to do,
>> immediately after you notice that wt is sometimes NULL.  It merely
>> means that unconditionally dereferencing wt->repo without thinking
>> is not good enough, doesn't it?
>>
>> And what is the case where worktree is NULL?  What are we doing with
>> worktree set to NULL?  Is it when secondary worktrees do not come
>> into the picture at all and you can safely use the_repository?
>>
>>     ... goes and looks ...
>>
>> Ahh, I think the real culprit that needs cleaning up is the worktree
>> API, where they pass NULL to mean "the primary worktree that has its
>> .git/ directory at its natural place".  So it may not necessarily be
>> the_repository we are dealing with.  There is *no* such client code
>> right now, but we could imagine that a program that starts in a
>> repository visits the primary worktree of another repository and
>> asks the worktree status there, and once such a client code appears,
>> we need to be able to say "we are dealing with the primary worktree
>> for this repository".
>>
>> In the longer run, I think we should fix the worktree API so that
>> even for the primary worktree we will always have a non-NULL "struct
>> worktree" object, perhaps with its .id member set to NULL to signal
>> that it is the primary worktree, so that we do not have to have this
>> strange "we must pass repository redundantly even though we are
>> passing worktree" API elsewhere.  Not just this code you are making
>> worse, path.c:worktree_git_path() already is a victim of this
>> misdesign of the worktree API.  It has "if wt is given, then the r
>> parameter should be the same as wt->repo" nonsense, which we
>> wouldn't have had to have if we had a worktree object even for the
>> primary worktree,  Look at how ugly that code is, and weep X-<.
>>
>> And the same misdesign of the worktree API has caused your [1/3] to
>> pass 'r' but yet still depend on the_repository, which you had to
>> fix in [2/3], in this function.
>>
>> So, I dunno.  If you are ambitious, you may want to clean up the
>> worktree API before this series.  Alternatively you may be able to
>> punt on the parts of the wt-status that interact with worktree API,
>> and move the rest of wt-status less dependent on the_repository, but
>> I am not sure.
>
> Thank you very much for the detailed explanation and for pointing towards
> the bigger picture.
>
> From what I have understood, the worktree being NULL refers to the
> primary worktree (as it does not indicate which repository so it means in
> respect to the_repository). So if we want to access the primary worktree
> of a specific repository or even the local repository, NULL does not carry
> enough information.
> And obviously, using NULL as primary worktree introduces extra checks and
> measures as we saw in the previous discussion.
>
> I would be very interested (and the more logical step) to fixing worktree api
> first, and then revisiting the wt-status series on top of that, once the API
> makes it possible to rely on wt->repo without the NULL risks.
>
> So a possible in the worktree api cleanup approach could be,
>
> * Make primary worktree as an instance of struct worktree but seperate
> it by having a marker like id = NULL.
>

I would like to point out that we already have a function which provides a main worktree, see both `get_main_worktree()` & `is_main_worktree()`. In short, a worktree with id = NULL seems to be treated as the main worktree.

The harder part would be correcting all code where `struct worktree *` is passed and has special meaning for NULL vs non-NULL. See `strbuf_worktree_gitdir()` which also distinguishes between `wt == NULL`, `wt->id == NULL` and `wt->id != NULL`.

So cleanup would require identifying all such spots and fixing them too.

> * Add this primary worktree in the struct repository (e.g. repo->primary_wt).
>

This also is tricky. We currently already store all worktrees in the repository in `struct strmap worktree_ref_stores`. Here, for the main worktree we use '\' (see `get_worktree_ref_store()`). So perhaps we should formalize using `\` for the main worktree everywhere.

Show 8 quoted lines

> * Update/add functions, then find places that currently pass NULL
> and convert them to use primary worktree object instead.
>
> Let me know if I have the right understanding with this, and also would love
> to hear more guidance on the direction with this worktree api cleanup. Thanks.
>
> Best,
> Shreyansh

Karthik