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