Re: [PATCH 1/2] wt-status: avoid passing NULL worktree

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

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> In preparation for removing the repository argument from
> worktree_git_path() add a function to construct a "struct worktree"
> from a "struct repository" and use that to avoid passing a NULL
> worktree to wt_status_check_bisect() and wt_status_check_rebase().

Hmph, I am afraid that

    "Construct a struct worktree from a struct repository"

is not quite sufficient.  A repository can have more than one
worktrees, so if you give a repository as a parameter, there needs a
way for the implementation of this helper function to identify which
one of them to construct a struct worktree for, and more importantly
for you as the caller to be able to expect which one the implementation
would pick, and what that particular worktree among many _means_ to you.

I know that the implementation uses repo->worktree but what does
that path mean in the world-view of the worktree API set?

I am guessing that it is what the worktree API calls "current", but
if so, perhaps the function should be explained with that word in
it, and the function name should also contain that word, no?

> +struct worktree *get_worktree_from_repository(struct repository *repo)
> +{
> +	struct worktree *wt = xcalloc(1, sizeof(*wt));
> +	char *gitdir = absolute_pathdup(repo->gitdir);
> +	char *commondir = absolute_pathdup(repo->commondir);
> +
> +	wt->repo = repo;
> +	if (repo->worktree)
> +		wt->path = absolute_pathdup(repo->worktree);

So, if the repository instance knows where the worktree is, we use
that to wt->path.  Otherwise wt->path is left NULL.

> +	wt->is_bare = !!repo->worktree;

I may be confused but don't we have one ! too many?  If we have a
worktree directory, "git checkout" would check the files there, and
that is not quite a "bare" repository, no?

> +	if (fspathcmp(gitdir, commondir))
> +		wt->id = xstrdup(find_last_dir_sep(commondir) + 1);

OK.  So gitdir and commondir would be the same for the primary and
for everybody else we'd have "id" as the last directory component of
the commondir.

> +	wt->is_current = is_current_worktree(wt);

Oh, so I guessed wrong and this is not about "current" worktree?
What does the directory pointed at by repo->worktree mean to the
callers of this function?  I somehow thought that is_current would
be always 1 here,.