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

From: Phillip Wood <[email-masked]>
Date: Fri, 27 Mar 2026 16:40:15 +0000

On 26/03/2026 15:48, Junio C Hamano wrote:
> Phillip Wood <[email-masked]> 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.

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

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

> I am asking the above because I find this unconditional assignment
> of "true" utterly confusing.  Is it because "we created a brand new
> worktree structure that by definition is current out of a repository
> structure, so as far as the caller is concerned by definition it is
> what it is currently working on"?

Yes

> Stepping back a bit, how do "is_current_worktree(wt)" and
> "wt->is_current" relate to each other?

"wt->current" is the cached return value of is_current_worktree() (see 
the implementation of get_main_worktree() and get_linked_worktree())

>  Here is what happens in the
> former:
> 
>          static int is_current_worktree(struct worktree *wt)
>          {
>                  char *git_dir = absolute_pathdup(repo_get_git_dir(wt->repo));
>                  char *wt_git_dir = get_worktree_git_dir(wt);
>                  int is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));
>                  free(wt_git_dir);
>                  free(git_dir);
>                  return is_current;
>          }
> 
> and given the definition of get_worktree_git_dir(), which gives
> either the "common" .git directory for the primary worktree, or the
> worktree specific .git directory for the secondaries, the is_current
> being computed here sounds more like "is wt the primary worktree
> among the ones that are attached to the same repository?"

No, the primary worktree (which the code calls the "main" worktree) is 
orthogonal to the "current" worktree. The main worktree is the one whose 
gitdir is $GIT_COMMON_DIR i.e. the worktree created by "git init". The 
"current" worktree is the one whose gitdir is wt->repo->gitdir and may 
or may not be the "main" worktree.

> But given that there are API functions with "main" in their names,
> like is_main_worktree() and internal function get_main_worktree(),
> what I said apparently is not the intention of whoever built the
> worktree subsystem.  I am still confused... X-<.
> 
>> @@ -229,9 +229,9 @@ char *get_worktree_git_dir(const struct worktree *wt)
>>   	if (!wt)
>>   		return xstrdup(repo_get_git_dir(the_repository));
>>   	else if (!wt->id)
>> -		return xstrdup(repo_get_common_dir(the_repository));
>> +		return xstrdup(repo_get_common_dir(wt->repo));
>>   	else
>> -		return repo_common_path(the_repository, "worktrees/%s", wt->id);
>> +		return repo_common_path(wt->repo, "worktrees/%s", wt->id);
>>   }
>>   
>>   static struct worktree *find_worktree_by_suffix(struct worktree **list,
>> diff --git a/worktree.h b/worktree.h
>> index e450d1a3317..94ae58db973 100644
>> --- a/worktree.h
>> +++ b/worktree.h
>> @@ -16,7 +16,7 @@ struct worktree {
>>   	struct object_id head_oid;
>>   	int is_detached;
>>   	int is_bare;
>> -	int is_current;
>> +	int is_current;		/* does `path` match `repo->worktree` */
> 
> This new comment also talks only at the lowest implementation level.
> What significance does it have to the upper layer that calls into
> the API if `path` matches or does not match `repo->worktree` is
> totally unclear, at least to me.

Does it help to think about how "is_current" is used by functions like 
get_worktree_ref_store()? For the current worktree it can use the 
refstore from wt->repo, if it is not it needs to open the store for that 
worktree.

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.

Thanks

Phillip

>>   	int lock_reason_valid; /* private */
>>   	int prune_reason_valid; /* private */
>>   };
> 
> Thanks.