Re: [PATCH 3/5] worktree.c: add is_worktree_locked()
On Sun, May 22, 2016 at 5:53 AM, Duy Nguyen wrote: > On Fri, May 13, 2016 at 11:52 PM, Eric Sunshine > wrote: >> 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. > > I'm keeping this as a separate function for now. I don't like > get_worktrees() doing extra work unless it has to. We soon will see > the complete picture of "git worktree" then we can merge it back to > get_worktrees() if it turns out checking "locked" is the common > operation. is_worktree_locked() then may become a thin wrapper to > access this "locked" field. is_worktree_locked() could also just go away since the 'struct worktree::locked' field would be non-NULL for locked, else NULL. >>> +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//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. > > I thought about updating prune too. But it is in a bit special > situation where it may need to consider invalid (or partly invalid) > worktrees as well. So far worktree api is about valid worktrees only > if I'm not mistaken and we probably should keep it that way, otherwise > all callers may need to check "is this worktree valid" all over the > place. Yes and no. In addition to suggesting to Mike that 'struct worktree' should have a 'locked' field, I also suggested a 'prunable' field which would indicate whether a worktree was a candidate for pruning. In fact, the field would be a 'const char *' where non-NULL would mean prunable and give a reason (one of the reasons already determined by builtin/worktree.c:prune_worktree()). Alternately it could be an enum. Like the 'locked' (or 'lock_reason') field, 'prunable' (or 'prune_reason') is likely the sort of information which should be presented by "git worktree list". And, as with 'locked', I think it makes sense for the 'prunable' field to be populated automatically by get_worktrees(). As for your concern about clients having to take care with valid vs. invalid worktrees, get_worktrees() could be updated to return all worktrees or only valid worktrees (for some definition of "valid"). "git worktree list" is an example of a client which would want to see all worktrees assuming its updated to show 'locked' and 'prunable' status. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] worktree.c: add is_worktree_locked()
On Fri, May 13, 2016 at 11:52 PM, Eric Sunshine wrote: > 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. I'm keeping this as a separate function for now. I don't like get_worktrees() doing extra work unless it has to. We soon will see the complete picture of "git worktree" then we can merge it back to get_worktrees() if it turns out checking "locked" is the common operation. is_worktree_locked() then may become a thin wrapper to access this "locked" field. >> +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//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. I thought about updating prune too. But it is in a bit special situation where it may need to consider invalid (or partly invalid) worktrees as well. So far worktree api is about valid worktrees only if I'm not mistaken and we probably should keep it that way, otherwise all callers may need to check "is this worktree valid" all over the place. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] worktree.c: add is_worktree_locked()
On Tue, May 10, 2016 at 10:17 AM, Nguyễn Thái Ngọc Duy
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
> ---
> 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//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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
