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

[Duy Nguyen <pclouds@gmail.com>]

On Fri, Apr 21, 2017 at 9:16 AM, Junio C Hamano <gitster@pobox.com> 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(&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?

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