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

[Duy Nguyen]

On Fri, Apr 21, 2017 at 9:16 AM, Junio C Hamano  wrote:
>> +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(, "%s/.git", wt->path);
>> + if (!is_directory(sb.buf)) {
>> + strbuf_release();
>> + 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?

I'll save the error in a strbuf and let the caller decide how to
present them, which gives better context (e.g. "unable to move
worktree because ...")

>> + }
>> + 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.

This is almost like fsck for worktrees and for now only be checked
before we do destructive things to worktrees (moving, removing..).

Yeah we probably should do this at read time too (after checking if a
worktree is locked, and skip the next check because wt->path may not
exist). But we probably want to either make this function cheaper, or
we cache the worktree list. Probably the latter. It's on my todo list.
-- 
Duy

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   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 
> ---
>  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(, "%s/.git", wt->path);
> + if (!is_directory(sb.buf)) {
> + strbuf_release();
> + 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(, "%s/.git", wt->path);
> + if (!file_exists(sb.buf)) {
> + strbuf_release();
> + return report(quiet, _("'%s/.git' does not exist"), wt->path);
> + }
> +
> + path = xstrdup_or_null(read_gitfile_gently(sb.buf, ));
> + strbuf_release();
> + 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 **);