From: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
Date: Sat, 14 Feb 2026 09:54:21 GMT
Subject: Re: [RFC][PATCH 1/2] worktree: represent the primary worktree with '/' instead of NULL
Message-ID: <20260214095817.514765-1-shreyanshpaliwalcmsmn@gmail.com>
In-Reply-To: <xmqq7bsgl42j.fsf@gitster.g>

> > diff --git a/worktree.c b/worktree.c
> > index 9308389cb6..b29934407f 100644
> > --- a/worktree.c
> > +++ b/worktree.c
> > @@ -101,6 +101,7 @@ static struct worktree *get_main_worktree(int skip_reading_head)
> >
> >  	CALLOC_ARRAY(worktree, 1);
> >  	worktree->repo = the_repository;
> > +	worktree->id = xstrdup("/");
> >  	worktree->path = strbuf_detach(&worktree_path, NULL);
> >  	worktree->is_current = is_current_worktree(worktree);
> >  	worktree->is_bare = (is_bare_repository_cfg == 1) ||
>
> Presumably we left .id = NULL from CALLOC_ARRAY(), so this looks
> sensible.  When releasing resources from an instance of worktree,
> we'd blindly free(worktree->id) and in the old world, free(NULL)
> turned into no-op, and this xstrdup()'d copy will be freed in the
> new world, so there is nothing funny here, I hope?  This one, and
> the change to is_main_worktree() go together.

Hmm, I don't think free(worktree->id) should cause any issue in this case.

> > @@ -127,6 +128,8 @@ struct worktree *get_linked_worktree(const char *id
> >
> >  	if (!id)
> >  		die("Missing linked worktree name");
> > +	if (!strcmp(id, "/"))
> > +		die("'/' is reserved for primary worktree");
>
> Makes me wonder if this is a BUG not die; where does id come from?
>
> 	... goes and looks ...
>
> The only caller is worktree.c:get_worktrees_internal() and it is
> feeding d->d_name that came from readdir_skip_dot_and_dotdot(), so
> it cannot be "/".
>
> By the way, I suspect that get_linked_worktree() should become
> file-scope static, as there is no other caller.

Actually get_linked_worktee(), along with worktree.c: get_worktrees_internal()
is also called from builtin/worktree.c: add_worktree().
So at this point we should prefer die(), and if were to make
get_linked_worktree() static, then we can add a helper for external uses
maybe using a struct repository* instead of the_repository in the future.

> > @@ -629,6 +630,9 @@ static void repair_gitfile(struct worktree *wt,
> >  	char *path = NULL;
> >  	int err;
> >
> > +	if (is_main_worktree(wt))
> > +		goto done;
>
> This is a bit new.
>
> The original did not say
>
> 	if (!wt || !wt->id || !strcmp(wt->id, "/"))
> 		goto done;
>
> The only caller is iterating over the resulting list of worktrees
> returned from get_worktrees_internal(1) *BUT* it already skips the
> primary worktree (the function MUST return the primary one as the
> first one, and the callers MUST be aware of the convention).
>
> So I am not sure if the new check is even needed.  Or rather, this ...
>
> 	if (!wt || !wt->id || !strcmp(wt->id, "/"))
> 		BUG("why are you feeding me the primary worktree???");
>
> ... might be more appropriate, perhaps?  I dunno.

Actually this new check is to prevent accidently feeding '/' as wt->id in,

	path = repo_common_path(the_repository, "worktrees/%s", wt->id);

but you are right as of now its only caller skips the primary worktree
so we can just put a precautionary check, for the case if this function
is used somewhere in future like this,

    if(is_main_worktree(wt))
        BUG("repair_gitfile() called for the main worktree");