Re: [PATCH 3/5] worktree.c: add is_worktree_locked()

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 13, 2016, 16:52 UTC
Message-ID: <CAPig+cQR40u4nMEP1wz74ubA=wW5m+KiCZnpZwuWAyYvXsPHrw@mail.gmail.com>
In-Reply-To: <20160510141729.23063-3-pclouds@gmail.com>

On Tue, May 10, 2016 at 10:17 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 14 quoted lines

> This provides an API for checking if a worktree is locked. We need to
> check this to avoid double locking a worktree, or try to unlock one when
> it's not even locked.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -243,6 +243,24 @@ int is_main_worktree(const struct worktree *wt)
> +const char *is_worktree_locked(const struct worktree *wt)
> +{
> +       static struct strbuf sb = STRBUF_INIT;
> +
> +       if (!file_exists(git_common_path("worktrees/%s/locked", wt->id)))
> +               return NULL;

The git_common_path(...) invocation is textually lengthy and repeated three times in this function. If you instead assign the result to a variable (possibly xstrdup'ing it if needed), then the below strbuf_read_file() would likely fit on one line, thus be easier to read.

Show 5 quoted lines

> +
> +       strbuf_reset(&sb);
> +       if (strbuf_read_file(&sb,
> +                            git_common_path("worktrees/%s/locked", wt->id),
> +                            0) < 0)

It's too bad that strbuf_read_file() doesn't guarantee anything about 'errno', such as indicating that the file did not exist, in which case the !file_exists() check would not be needed, and a bit of raciness eliminated, but that's outside the scope of this series.

> +               die_errno(_("failed to read '%s'"),
> +                         git_common_path("worktrees/%s/locked", wt->id));
> +
> +       strbuf_rtrim(&sb);

Since this file is presumably human-editable (historically and at this point in the series) in order to specify the lock reason, should this be doing a full trim() rather than only rtrim()?

Show 8 quoted lines

> +       return sb.buf;
> +}
> diff --git a/worktree.h b/worktree.h
> @@ -40,6 +40,12 @@ extern struct worktree *find_worktree_by_path(struct worktree **list,
> +/*
> + * Return the reason string if the given worktree is locked. Return
> + * NULL otherwise.
> + */

Does this need to mention that the returned "locked reason" string is only guaranteed valid until the next invocation?

Actually, I recall that when I suggested the idea of 'struct worktree' and get_worktrees() to Mike that it would be natural for the structure to have a 'locked' (or 'locked_reason') field, in which case the reason could be stored there instead of in this static strbuf. In fact, my idea at the time was that get_worktrees() would populate that field automatically, rather than having to do so via a separate on-demand function call as in this patch.

> +extern const char *is_worktree_locked(const struct worktree *wt);

I was wondering if builtin/worktree.c:prune_worktree() should be updated to invoke this new function instead of consulting "worktrees/<id>/locked" manually, but I see that the entire "prune worktrees" functionality in builting/worktree.c first needs to be updated to the get_worktrees() API for that to happen.