From: Shreyansh Paliwal Date: Sat, 14 Feb 2026 09:59:33 GMT Subject: Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree Message-ID: <20260214101045.515941-1-shreyanshpaliwalcmsmn@gmail.com> In-Reply-To: > > diff --git a/builtin/worktree.c b/builtin/worktree.c > > index fbdaf2eb2e..27c5889c89 100644 > > --- a/builtin/worktree.c > > +++ b/builtin/worktree.c > > @@ -328,6 +328,8 @@ static void check_candidate_path(const char *path, > > wt = find_worktree_by_path(worktrees, path); > > if (!wt) > > return; > > + if(is_main_worktree(wt)) > > + die(_("'%s' is the main worktree"), path); > > Style (missing SP between "if" and "(condition)"). my bad, will fix that. > Earlier, a failure from find_worktree_by_path(), presumably meaning > "you gave me this path, but that is not where any of our worktrees > live" gave wt==NULL and it silently returned. Could the function > returned wt==NULL to signal that the path is where the primary > worktree is? I guess not (it seems to give us a worktree instance > with wt->id==NULL). > > So, we never cared about wt being the primary worktree, but now we > care. Why do we need to? The function check_candidate_path() is basically to check the path we can add a worktree, so if we are receiving primary worktree it shouldn't proceed and further in the function we are calling delete_git_dir(wt->id) below which then further calls repo_common_path_append() with id. So to prevent feeding '/' to this we need to check if the worktree is main or not. > > @@ -660,7 +662,8 @@ static int can_use_local_refs(const struct add_opts *opts) > > if (!opts->quiet) { > > struct strbuf path = STRBUF_INIT; > > struct strbuf contents = STRBUF_INIT; > > - char *wt_gitdir = get_worktree_git_dir(NULL); > > + struct worktree **worktrees = get_worktrees_repo(the_repository); > > + char *wt_gitdir = get_worktree_git_dir(worktrees[0]); > > We used to pass NULL to get_worktree_git_dir() to ask about the > primary working tree, but the convention is no longer available. So > we use get_worktrees_repo(), presumably is a new function, that > gives all the worktrees honoring the "first one in the resulting > list is the primary one" convention, only to use the first element > in the list. > > I wonder if making worktree.c:get_main_worktree(), which is a file > scope static in worktree.c, available would allow us express this > logic more directly? Actually I thought about making get_main_worktree() available but then I saw helpers like get_worktrees() so I thought that get_worktrees_repo() would be better for every use case. But I agree since there are so many sites to access the main worktree, we can make get_main_worktree() available and use it instead of introducing get_worktrees_repo(). > > diff --git a/path.c b/path.c > > index d726537622..4ac86e1e58 100644 > > --- a/path.c > > +++ b/path.c > > @@ -408,9 +408,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf, > > const struct repository *repo, > > const struct worktree *wt) > > { > > - if (!wt) > > - strbuf_addstr(buf, repo->gitdir); > > - else if (!wt->id) > > + if (is_main_worktree(wt)) > > strbuf_addstr(buf, repo->commondir); > > else > > repo_common_path_append(repo, buf, "worktrees/%s", wt->id); > > This is curious. > > We used to treat "wt==NULL" and "wt->id==NULL" differently. Now we > use repo->commondir for both. For the primary worktree, it ought to > be the same as repo->gitdir, so it should not matter, but makes me > wonder what the reason behind this difference in the original. > > We have been assuming that wt==NULL and wt->id==NULL both meant the > same thing: "we are talking about the primary worktree". But the > code around here before this patch seems to behave differently. Is > our assumption incorrect and are we making a mistake by conflating > these two conditions into one? Yes it came into my mind as well. So if we check were strbuf_worktree_gitdir() is called from, there is only one function repo_git_pathv() which is mostly called with a NULL wt (primary worktree) and called once with an actual worktreee once inside worktree_git_path() which had some NULL indirect callers inside wt-status.c which again meant wt being primary. I think different usecases for wt and wt->id being NULL was just an oversight at this particular instance and both of them should refer to the main worktree. > > diff --git a/worktree.c b/worktree.c > > index b29934407f..1059c18115 100644 > > --- a/worktree.c > > +++ b/worktree.c > > @@ -91,16 +91,16 @@ static int is_main_worktree_bare(struct repository *repo) > > /** > > * get the main worktree > > */ > > -static struct worktree *get_main_worktree(int skip_reading_head) > > +static struct worktree *get_main_worktree(struct repository *repo, int skip_reading_head) > > { > > struct worktree *worktree = NULL; > > struct strbuf worktree_path = STRBUF_INIT; > > > > - strbuf_add_real_path(&worktree_path, repo_get_common_dir(the_repository)); > > + strbuf_add_real_path(&worktree_path, repo_get_common_dir(repo)); > > strbuf_strip_suffix(&worktree_path, "/.git"); > > > > CALLOC_ARRAY(worktree, 1); > > - worktree->repo = the_repository; > > + worktree->repo = repo; > > worktree->id = xstrdup("/"); > > worktree->path = strbuf_detach(&worktree_path, NULL); > > worktree->is_current = is_current_worktree(worktree); > > @@ -112,7 +112,7 @@ static struct worktree *get_main_worktree(int skip_reading_head) > > * This check is unnecessary if we're currently in the main worktree, > > * as prior checks already consulted all configs of the current worktree. > > */ > > - (!worktree->is_current && is_main_worktree_bare(the_repository)); > > + (!worktree->is_current && is_main_worktree_bare(repo)); > > > > if (!skip_reading_head) > > add_head_info(worktree); > > Weaning the code from depending on the_repository is mixed into the > refactoring, which makes me wonder if it is better done in a > separate patch. We seriously should consider making this function > externally visible, as so many callers want to get hold of it. > > I also wonder if "struct repository" wants to have a member that > points at the primary worktree instance, but I think I am getting > way ahead of myself. Yes agreed. I will in a seperate patch make get_main_worktree() available with a struct repository * argument instead of the_repository and then we can use it in all the places where we need to access the main worktree. And I think that if we are making get_main_worktree() with a struct repository * argument available, then we can skip adding a new member to struct repository and just call get_main_worktree() whenever we need to access the main worktree with respect to whatever instance of repository we are working with. Thanks for reviewing. Best, Shreyansh