Re: [PATCH v3 1/3] worktree: remove "the_repository" from is_current_worktree()

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

> ... it calls get_workree_git_dir() which also depends on
> "the_repository". This means that even if "wt->path" matches
> "wt->repo->worktree" is_current_worktree(wt) will return false when
> "wt->repo" is not "the_repository". Consequently die_if_checked_out()
> will fail to skip such a worktree when checking if a branch is already
> checked out and may die errounously. Fix this by using the worktree's
> repository instance instead of "the_repository" when comparing gitdirs.

It sounds like the above is something we can write in a test to make
sure the code with this fix won't regress in the future.  Can we add
one?

> The use of "the_repository" in is_current_wortree() comes from
> replacing get_git_dir() with repo_get_git_dir() in 246deeac951
> (environment: make `get_git_dir()` accept a repository, 2024-09-12). In
> get_worktree_git_dir() it comes from replacing git_common_path() with
> repo_common_path() in 07242c2a5af (path: drop `git_common_path()`
> in favor of `repo_common_path()`, 2025-02-07). In both cases the
> replacements appear to have been mechanical.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  worktree.c | 8 ++++----
>  worktree.h | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/worktree.c b/worktree.c
> index e9ff6e6ef2e..344ad0c031b 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -58,7 +58,7 @@ static void add_head_info(struct worktree *wt)
>  
>  static int is_current_worktree(struct worktree *wt)
>  {
> -	char *git_dir = absolute_pathdup(repo_get_git_dir(the_repository));
> +	char *git_dir = absolute_pathdup(repo_get_git_dir(wt->repo));
>  	char *wt_git_dir = get_worktree_git_dir(wt);
>  	int is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));
>  	free(wt_git_dir);
> @@ -78,7 +78,7 @@ struct worktree *get_worktree_from_repository(struct repository *repo)
>  	wt->is_bare = !repo->worktree;
>  	if (fspathcmp(gitdir, commondir))
>  		wt->id = xstrdup(find_last_dir_sep(gitdir) + 1);
> -	wt->is_current = is_current_worktree(wt);
> +	wt->is_current = true;
>  	add_head_info(wt);
>  
>  	free(gitdir);

I think I found the semantics of get_worktree_from_repository()
unclear when we discussed a different patch series, so I may have
asked the same question already, but what exactly does it mean to
"get worktree from repository", i.e., this function does?  A
repository can have one or more worktrees attached to it (that was
the whole point of introducing the worktree mechanism), so "I have
this repository, give me the worktree for it" is not a sensible
request.  The function's comment in <worktree.h> talks only at the
implementation level "construct a worktree struct from repo->gitdir
and repo->worktree" as if it is so obvious what the resulting
worktree struct means at a higher layer's point of view, which does
not help, either.

I am asking the above because I find this unconditional assignment
of "true" utterly confusing.  Is it because "we created a brand new
worktree structure that by definition is current out of a repository
structure, so as far as the caller is concerned by definition it is
what it is currently working on"?

Stepping back a bit, how do "is_current_worktree(wt)" and
"wt->is_current" relate to each other?  Here is what happens in the
former:

        static int is_current_worktree(struct worktree *wt)
        {
                char *git_dir = absolute_pathdup(repo_get_git_dir(wt->repo));
                char *wt_git_dir = get_worktree_git_dir(wt);
                int is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));
                free(wt_git_dir);
                free(git_dir);
                return is_current;
        }

and given the definition of get_worktree_git_dir(), which gives
either the "common" .git directory for the primary worktree, or the
worktree specific .git directory for the secondaries, the is_current
being computed here sounds more like "is wt the primary worktree
among the ones that are attached to the same repository?"

But given that there are API functions with "main" in their names,
like is_main_worktree() and internal function get_main_worktree(),
what I said apparently is not the intention of whoever built the
worktree subsystem.  I am still confused... X-<.

> @@ -229,9 +229,9 @@ char *get_worktree_git_dir(const struct worktree *wt)
>  	if (!wt)
>  		return xstrdup(repo_get_git_dir(the_repository));
>  	else if (!wt->id)
> -		return xstrdup(repo_get_common_dir(the_repository));
> +		return xstrdup(repo_get_common_dir(wt->repo));
>  	else
> -		return repo_common_path(the_repository, "worktrees/%s", wt->id);
> +		return repo_common_path(wt->repo, "worktrees/%s", wt->id);
>  }
>  
>  static struct worktree *find_worktree_by_suffix(struct worktree **list,
> diff --git a/worktree.h b/worktree.h
> index e450d1a3317..94ae58db973 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -16,7 +16,7 @@ struct worktree {
>  	struct object_id head_oid;
>  	int is_detached;
>  	int is_bare;
> -	int is_current;
> +	int is_current;		/* does `path` match `repo->worktree` */

This new comment also talks only at the lowest implementation level.
What significance does it have to the upper layer that calls into
the API if `path` matches or does not match `repo->worktree` is
totally unclear, at least to me.

>  	int lock_reason_valid; /* private */
>  	int prune_reason_valid; /* private */
>  };

Thanks.