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

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, May 22, 2016 at 5:53 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Fri, May 13, 2016 at 11:52 PM, Eric Sunshine <sunshine@sunshineco.com> 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/<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.