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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 8, 2026, 06:08 UTC
Message-ID: <xmqq4inrahti.fsf@gitster.g>
In-Reply-To: <20260208044450.34444-1-shreyanshpaliwalcmsmn@gmail.com>

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

Show 27 quoted lines

>> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
>> 
>> > -int wt_status_check_rebase(const struct worktree *wt,
>> > -			   struct wt_status_state *state)
>> > +int wt_status_check_rebase(struct repository *r,
>> > +	 			const struct worktree *wt,
>> > +			    struct wt_status_state *state)
>> 
>> Funny indentation.
>
> my bad, will fix it.
>
>> 
>> Besides, should we adding a yet another repository parameter to the
>> function?  The worktree wt knows what repository it belongs to.
>> 
>> > -int wt_status_check_bisect(const struct worktree *wt,
>> > +int wt_status_check_bisect(struct repository *r, 
>> > +			   struct worktree *wt,
>> >  			   struct wt_status_state *state)
>> 
>> Same comment about "r" vs "wt->repo" applies here.
>
> 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.

Thanks.