From: Junio C Hamano Date: Fri, 13 Feb 2026 22:29:43 GMT Subject: Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree Message-ID: In-Reply-To: <20260213120529.15475-3-shreyanshpaliwalcmsmn@gmail.com> Shreyansh Paliwal writes: > - path = xstrdup(worktree_git_path(the_repository, wt, "index")); > + path = xstrdup(worktree_git_path(wt, "index")); We'll understand this change when we read the changes to worktree_git_path() later in this patch, I guess. I will not comment on the same changes around worktree_git_path() callers. > 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)"). 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? > @@ -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? > strbuf_add_real_path(&path, wt_gitdir); > strbuf_addstr(&path, "/HEAD"); > @@ -675,6 +678,7 @@ static int can_use_local_refs(const struct add_opts *opts) > strbuf_release(&path); > strbuf_release(&contents); > free(wt_gitdir); > + free_worktrees(worktrees); Anyway, we need to release the list of worktrees here. > @@ -1191,14 +1195,14 @@ static void validate_no_submodules(const struct worktree *wt) > > wt_gitdir = get_worktree_git_dir(wt); > > - if (is_directory(worktree_git_path(the_repository, wt, "modules"))) { > + if (is_directory(worktree_git_path(wt, "modules"))) { > - } else if (read_index_from(&istate, worktree_git_path(the_repository, wt, "index"), > + } else if (read_index_from(&istate, worktree_git_path(wt, "index"), Ditto on worktree_git_path(). > 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? > @@ -437,8 +435,10 @@ char *repo_git_path(struct repository *repo, > struct strbuf path = STRBUF_INIT; > va_list args; > va_start(args, fmt); > - repo_git_pathv(repo, NULL, &path, fmt, args); > + struct worktree **worktrees = get_worktrees_repo(repo); > + repo_git_pathv(repo, worktrees[0], &path, fmt, args); > va_end(args); > + free_worktrees(worktrees); The same "to pass the primary worktree, we need to grab everybody and pass the first one" pattern is repeated here. > @@ -448,8 +448,10 @@ const char *repo_git_path_append(struct repository *repo, > { > va_list args; > va_start(args, fmt); > - repo_git_pathv(repo, NULL, sb, fmt, args); > + struct worktree **worktrees = get_worktrees_repo(repo); > + repo_git_pathv(repo, worktrees[0], sb, fmt, args); > va_end(args); > + free_worktrees(worktrees); And again here. > @@ -460,8 +462,10 @@ const char *repo_git_path_replace(struct repository *repo, > va_list args; > strbuf_reset(sb); > va_start(args, fmt); > - repo_git_pathv(repo, NULL, sb, fmt, args); > + struct worktree **worktrees = get_worktrees_repo(repo); > + repo_git_pathv(repo, worktrees[0], sb, fmt, args); > va_end(args); > + free_worktrees(worktrees); And again here. > @@ -486,17 +490,12 @@ const char *mkpath(const char *fmt, ...) > return cleanup_path(pathname->buf); > } > > -const char *worktree_git_path(struct repository *r, > - const struct worktree *wt, const char *fmt, ...) > +const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...) > { Since we no longer use wt==NULL as the sign to work on the primary working tree, we can rely on wt->repo being a valid repository the caller wants to work with. > struct strbuf *pathname = get_pathname(); > va_list args; > - > - if (wt && wt->repo != r) > - BUG("worktree not connected to expected repository"); > - > va_start(args, fmt); > - repo_git_pathv(r, wt, pathname, fmt, args); > + repo_git_pathv(wt->repo, wt, pathname, fmt, args); > va_end(args); > return pathname->buf; > } > diff --git a/path.h b/path.h > index 0ec95a0b07..54e09b2883 100644 > --- a/path.h > +++ b/path.h > @@ -66,13 +66,10 @@ const char *repo_git_path_replace(struct repository *repo, > > /* > * Similar to repo_git_path() but can produce paths for a specified > - * worktree instead of current one. When no worktree is given, then the path is > - * computed relative to main worktree of the given repository. > + * worktree instead of current one. > */ > -const char *worktree_git_path(struct repository *r, > - const struct worktree *wt, > - const char *fmt, ...) > - __attribute__((format (printf, 3, 4))); > +const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...) > + __attribute__((format (printf, 2, 3))); Good. > diff --git a/refs.c b/refs.c > index 627b7f8698..98df2235e7 100644 > --- a/refs.c > +++ b/refs.c > @@ -2324,12 +2324,12 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt) > if (wt->is_current) > return get_main_ref_store(wt->repo); > > - id = wt->id ? wt->id : "/"; > + id = wt->id; > refs = lookup_ref_store_map(&wt->repo->worktree_ref_stores, id); > if (refs) > return refs; > > - if (wt->id) { > + if (!is_main_worktree(wt)) { > struct strbuf common_path = STRBUF_INIT; > repo_common_path_append(wt->repo, &common_path, > "worktrees/%s", wt->id); Good. The original uses local "id" as a pathname component (which could be "/"), and wt->id==NULL as the sign that it is the primary. The updated code is a faithful conversion of it to the new world order. > diff --git a/revision.c b/revision.c > index 9b131670f7..a9d4f796ed 100644 > --- a/revision.c > +++ b/revision.c > @@ -1730,12 +1730,14 @@ void add_reflogs_to_pending(struct rev_info *revs, unsigned flags) > > cb.all_revs = revs; > cb.all_flags = flags; > - cb.wt = NULL; > + struct worktree **worktrees = get_worktrees_repo(the_repository); > + cb.wt = worktrees[0]; > refs_for_each_reflog(get_main_ref_store(the_repository), > handle_one_reflog, &cb); > > if (!revs->single_worktree) > add_other_reflogs_to_pending(&cb); > + free_worktrees(worktrees); The same "to pass the primary worktree, we need to grab everybody and pass the first one" pattern strikes again. > 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. > @@ -199,12 +199,15 @@ static struct worktree **get_worktrees_internal(int skip_reading_head) > > struct worktree **get_worktrees(void) > { > - return get_worktrees_internal(0); > + return get_worktrees_repo(the_repository); > +} > +struct worktree **get_worktrees_repo(struct repository *repo) > +{ > + return get_worktrees_internal(repo, 0); > } OK. > @@ -1795,18 +1795,19 @@ void wt_status_get_state(struct repository *r, > struct stat st; > struct object_id oid; > enum replay_action action; > + struct worktree **worktrees = get_worktrees_repo(r); > > if (!stat(git_path_merge_head(r), &st)) { > - wt_status_check_rebase(NULL, state); > + wt_status_check_rebase(worktrees[0], state); > state->merge_in_progress = 1; > - } else if (wt_status_check_rebase(NULL, state)) { > + } else if (wt_status_check_rebase(worktrees[0], state)) { > ; /* all set */ > } else if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") && > !repo_get_oid(r, "CHERRY_PICK_HEAD", &oid)) { > state->cherry_pick_in_progress = 1; > oidcpy(&state->cherry_pick_head_oid, &oid); > } > - wt_status_check_bisect(NULL, state); > + wt_status_check_bisect(worktrees[0], state); > if (refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD") && > !repo_get_oid(r, "REVERT_HEAD", &oid)) { > state->revert_in_progress = 1; > @@ -1824,6 +1825,7 @@ void wt_status_get_state(struct repository *r, > if (get_detached_from) > wt_status_get_detached_from(r, state); > wt_status_check_sparse_checkout(r, state); > + free_worktrees(worktrees); > } The same "to pass the primary worktree, we need to grab everybody and pass the first one" pattern strikes yet another time. Thanks.