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

[Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>]

> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
> 
> >> 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.

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.

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

* 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