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

> 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.