Re: [RFC][PATCH 2/2] worktree: stop passing NULL as primary worktree

[Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>]

> > 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