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

On Sun, May 22, 2016 at 5:53 AM, Duy Nguyen [off-list ref] wrote:

On Fri, May 13, 2016 at 11:52 PM, Eric Sunshine [off-list ref] wrote:

quoted

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.

quoted

quoted

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

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.