From: Phillip Wood <phillip.wood123@gmail•com>
To: Junio C Hamano <gitster@pobox•com>
Cc: git@vger•kernel.org, Patrick Steinhardt <ps@pks•im>,
Eric Sunshine <sunshine@sunshineco•com>
Subject: Re: [PATCH v3 1/3] worktree: remove "the_repository" from is_current_worktree()
Date: Fri, 27 Mar 2026 16:40:15 +0000 [thread overview]
Message-ID: <317ed4f7-f88d-4415-bd25-b62b4a076728@gmail.com> (raw)
In-Reply-To: <xmqqy0jer3qu.fsf@gitster.g>
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.
>> - 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.
next prev parent reply other threads:[~2026-03-27 16:40 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-13 14:19 [PATCH 0/3] worktree: stop using "the_repository" in is_current_worktree() Phillip Wood
2026-03-13 14:19 ` [PATCH 1/3] worktree: remove "the_repository" from is_current_worktree() Phillip Wood
2026-03-13 14:19 ` [PATCH 2/3] worktree add: stop reading ".git/HEAD" Phillip Wood
2026-03-13 21:41 ` Junio C Hamano
2026-03-13 14:19 ` [PATCH 3/3] worktree: reject NULL worktree in get_worktree_git_dir() Phillip Wood
2026-03-13 21:42 ` Junio C Hamano
2026-03-14 20:09 ` Phillip Wood
2026-03-15 16:18 ` [PATCH v2 0/3] worktree: stop using "the_repository" in is_current_worktree() Phillip Wood
2026-03-15 16:18 ` [PATCH v2 1/3] worktree: remove "the_repository" from is_current_worktree() Phillip Wood
2026-03-16 7:38 ` Patrick Steinhardt
2026-03-16 16:22 ` Phillip Wood
2026-03-17 10:24 ` Phillip Wood
2026-03-23 9:41 ` Shreyansh Paliwal
2026-03-23 14:37 ` Phillip Wood
2026-03-23 17:05 ` Shreyansh Paliwal
2026-03-15 16:18 ` [PATCH v2 2/3] worktree add: stop reading ".git/HEAD" Phillip Wood
2026-03-16 7:39 ` Patrick Steinhardt
2026-03-15 16:18 ` [PATCH v2 3/3] worktree: reject NULL worktree in get_worktree_git_dir() Phillip Wood
2026-03-15 21:17 ` [PATCH v2 0/3] worktree: stop using "the_repository" in is_current_worktree() Junio C Hamano
2026-03-26 14:16 ` [PATCH v3 " Phillip Wood
2026-03-26 14:16 ` [PATCH v3 1/3] worktree: remove "the_repository" from is_current_worktree() Phillip Wood
2026-03-26 15:48 ` Junio C Hamano
2026-03-27 16:40 ` Phillip Wood [this message]
2026-03-27 17:07 ` Junio C Hamano
2026-04-02 15:10 ` Phillip Wood
2026-03-26 14:16 ` [PATCH v3 2/3] worktree add: stop reading ".git/HEAD" Phillip Wood
2026-03-26 14:16 ` [PATCH v3 3/3] worktree: reject NULL worktree in get_worktree_git_dir() Phillip Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=317ed4f7-f88d-4415-bd25-b62b4a076728@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--cc=ps@pks$(echo .)im \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox