Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 13, 2026, 22:29 UTC
- Message-ID
- <xmqqcy28jmzs.fsf@gitster.g>
- In-Reply-To
- <20260213120529.15475-3-shreyanshpaliwalcmsmn@gmail.com>
Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> 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.
Show 10 quoted lines
> 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?
Show 7 quoted lines
> @@ -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?
Show 7 quoted lines
> 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.
Show 8 quoted lines
> @@ -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().
Show 15 quoted lines
> 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?
Show 9 quoted lines
> @@ -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.
Show 9 quoted lines
> @@ -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.
Show 9 quoted lines
> @@ -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.
Show 8 quoted lines
> @@ -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.
Show 30 quoted lines
> 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.
Show 19 quoted lines
> 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.
Show 17 quoted lines
> 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.
Show 33 quoted lines
> 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.
Show 11 quoted lines
> @@ -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.
Show 29 quoted lines
> @@ -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.