Re: [PATCH 1/6] worktree.c: add validate_worktree()

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This function is later used by "worktree move" and "worktree remove"
> to ensure that we have a good connection between the repository and
> the worktree. For example, if a worktree is moved manually, the
> worktree location recorded in $GIT_DIR/worktrees/.../gitdir is
> incorrect and we should not move that one.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  worktree.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  worktree.h |  5 +++++
>  2 files changed, 71 insertions(+)
>
> diff --git a/worktree.c b/worktree.c
> index bae787cf8d..40cc031ac9 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -294,6 +294,72 @@ const char *is_worktree_locked(struct worktree *wt)
>  	return wt->lock_reason;
>  }
>  
> +static int report(int quiet, const char *fmt, ...)
> +{
> +	va_list params;
> +
> +	if (quiet)
> +		return -1;
> +
> +	va_start(params, fmt);
> +	vfprintf(stderr, fmt, params);
> +	fputc('\n', stderr);
> +	va_end(params);
> +	return -1;
> +}
> +
> +int validate_worktree(const struct worktree *wt, int quiet)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	char *path;
> +	int err, ret;
> +
> +	if (is_main_worktree(wt)) {
> +		/*
> +		 * Main worktree using .git file to point to the
> +		 * repository would make it impossible to know where
> +		 * the actual worktree is if this function is executed
> +		 * from another worktree. No .git file support for now.
> +		 */
> +		strbuf_addf(&sb, "%s/.git", wt->path);
> +		if (!is_directory(sb.buf)) {
> +			strbuf_release(&sb);
> +			return report(quiet, _("'%s/.git' at main worktree is not the repository directory"),
> +				      wt->path);

These messages come without telling what they are.  Should they say
that these are errors?  Or does the severity depend on the caller,
i.e. why they want to know if a particular worktree is valid, and
sometimes these are errors and other times they are mere warnings?

> +		}
> +		return 0;
> +	}
> +
> +	/*
> +	 * Make sure "gitdir" file points to a real .git file and that
> +	 * file points back here.
> +	 */
> +	if (!is_absolute_path(wt->path))
> +		return report(quiet, _("'%s' file does not contain absolute path to the worktree location"),
> +			      git_common_path("worktrees/%s/gitdir", wt->id));

It makes me wonder if this kind of error reporting belongs to the
place where these are read (and a new wt is written out to the
filesystem, perhaps).  The programmer who wrote this code may have
known that wt->path is prepared by reading "worktrees/%s/gitdir" and
without doing real_path() or absolute_path() on the result when this
code was written, but nothing guarantees that to stay true over time
as the code evolves.

> +	strbuf_addf(&sb, "%s/.git", wt->path);
> +	if (!file_exists(sb.buf)) {
> +		strbuf_release(&sb);
> +		return report(quiet, _("'%s/.git' does not exist"), wt->path);
> +	}
> +
> +	path = xstrdup_or_null(read_gitfile_gently(sb.buf, &err));
> +	strbuf_release(&sb);
> +	if (!path)
> +		return report(quiet, _("'%s/.git' is not a .git file, error code %d"),
> +			      wt->path, err);

The above should do, at least for now.  It is unfortunate that no
existing code needs to turn READ_GITFILE_ERR_* into human readble
error message.

> +	ret = fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id)));
> +	free(path);
> +
> +	if (ret)
> +		return report(quiet, _("'%s' does not point back to '%s'"),
> +			      wt->path, git_common_path("worktrees/%s", wt->id));
> +
> +	return 0;
> +}
> +
>  int is_worktree_being_rebased(const struct worktree *wt,
>  			      const char *target)
>  {
> diff --git a/worktree.h b/worktree.h
> index 6bfb985203..33f7405e33 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -58,6 +58,11 @@ extern int is_main_worktree(const struct worktree *wt);
>  extern const char *is_worktree_locked(struct worktree *wt);
>  
>  /*
> + * Return zero if the worktree is in good condition.
> + */
> +extern int validate_worktree(const struct worktree *wt, int quiet);
> +
> +/*
>   * Free up the memory for worktree(s)
>   */
>  extern void free_worktrees(struct worktree **);