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

[Junio C Hamano <gitster@pobox.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.

Thanks.