Re: [PATCH v3 1/3] worktree: remove "the_repository" from is_current_worktree()

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 26/03/2026 15:48, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>> 
>>> ... it calls get_workree_git_dir() which also depends on
>>> "the_repository". This means that even if "wt->path" matches
>>> "wt->repo->worktree" is_current_worktree(wt) will return false when
>>> "wt->repo" is not "the_repository". Consequently die_if_checked_out()
>>> will fail to skip such a worktree when checking if a branch is already
>>> checked out and may die errounously. Fix this by using the worktree's
>>> repository instance instead of "the_repository" when comparing gitdirs.
>> 
>> It sounds like the above is something we can write in a test to make
>> sure the code with this fix won't regress in the future.  Can we add
>> one?
>
> We'd need to create a struct worktree instance with a repository 
> instance that is not "the_repository" - I'm not sure we can do that in a 
> script.

Ah, so this is more of "futureproofing" than "fix"?  That is fine.
Thanks for clarifying.

>>> -	wt->is_current = is_current_worktree(wt);
>>> +	wt->is_current = true;
>>>   	add_head_info(wt);
>>>   
>>>   	free(gitdir);
>> 
>> I think I found the semantics of get_worktree_from_repository()
>> unclear when we discussed a different patch series, so I may have
>> asked the same question already, but what exactly does it mean to
>> "get worktree from repository", i.e., this function does?  A
>> repository can have one or more worktrees attached to it (that was
>> the whole point of introducing the worktree mechanism), so "I have
>> this repository, give me the worktree for it" is not a sensible
>> request.
>
> A repository can have more that one worktree but a "struct repository" 
> instance has "gitdir" and "worktree" members that point to a specific 
> worktree within that repository. For example
>
> 	repo_get_oid(repo, "HEAD", &oid);
>
> reads "HEAD" from a specific worktree within the repository.

And...?

I _think_ what I am frustrated about is the lack of description on
"what it means to be the worktree among many that is pointed by via
the .worktree member in the repository struct".  Does it correspond to
the worktree being "the current worktree the codepath is working on?"

>> The function's comment in <worktree.h> talks only at the
>> implementation level "construct a worktree struct from repo->gitdir
>> and repo->worktree" as if it is so obvious what the resulting
>> worktree struct means at a higher layer's point of view, which does
>> not help, either.
>
> That comes from me thinking of a struct repository as referring to a 
> specific worktree - would calling it 
> "get_worktree_from_repository_instance" be clearer?

It does not change the descriptive value of the name in any
meaningful way, so let's not do that.  If the answer to my "what
frustrates me" comment above is "yeah, we are getting the current
worktree", then renaming the function to include "current" in its
name would add descriptive value vastly, though.

> I feel I'm struggling to explain this clearly - I find this whole 
> discussion gets confusing because we have "struct worktree" and also a 
> "worktree" member of "struct repository" which means a "struct 
> repository" instance is tied to a specific worktree within the 
> repository. If "struct repository" only had a "commondir" member and no 
> "gitdir" or "worktree" members and we instead used "sturct worktree" to 
> refer to a specific worktree within a repository with functions like
>
> 	worktree_get_oid(wt, "HEAD", &oid);
>
> instead of
>
> 	repo_get_oid(repo, "HEAD", &oid);
>
> it might be clearer but that would be a very big change.

In short, am I hearing the worktree subsystem is not conceptually
clean and it would be a huge undertaking to clean it up?