On Tue, May 10, 2016 at 10:17 AM, Nguyễn Thái Ngọc Duy
<pclouds@xxxxxxxxx> wrote:
> 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@xxxxxxxxx>
> ---
> 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.
> +
> + 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()?
> + 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html