Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Oct 28, 2018 at 9:11 PM Nickolai Belakovski
<nbelakovski@gmail.com> wrote:
> On Sun, Oct 28, 2018 at 4:03 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > Aside from that, it doesn't seem like worktree needs any changes for
> > the ref-filter atom you have in mind. (Don't interpret this
> > observation as me being averse to changes to the API; I'm open to
> > improvements, but haven't seen anything yet indicating a bug or
> > showing that the API is more difficult than it ought to be.)
>
> You're right that these changes are not necessary in order to make a
> worktree atom.
> If there's no interest in this patch I'll withdraw it.

Withdrawing this patch seems reasonable.

> I had found it really surprising that lock_reason was not populated
> when I was accessing it while working on the worktree atom. When
> digging into it, the "internal use" comment told me nothing, both
> because there's no convention (that I'm aware of) within C to mark
> fields as such and because it fails to direct the reader to
> is_worktree_locked.
>
> How about this, I can make a patch that changes the comment next to
> lock_reason to say "/* private - use is_worktree_locked */" (choosing
> the word "private" since it's a reserved keyword in C++ and other
> languages for implementation details that are meant to be
> inaccessible) and a comment next to lock_reason_valid that just says
> "/* private */"?

A patch clarifying the "private" state of 'lock_reason' and
'lock_reason_valid' and pointing the reader at is_worktree_locked()
would be welcome.

One extra point: It might be a good idea to mention in the
documentation of is_worktree_locked() that, in addition to returning
NULL or non-NULL indicating not-locked or locked, the returned
lock-reason might very well be empty ("") when no reason was given by
the locker.

> I would also suggest renaming is_worktree_locked to
> worktree_lock_reason, the former makes me think the function is
> returning a boolean, whereas the latter more clearly conveys that a
> more detailed piece of information is being returned.

I think the "boolean"-sounding name was intentional since most
(current) callers only care about that; so, the following reads very
naturally for such callers:

    if (is_worktree_locked(wt))
        die(_("worktree locked; aborting"));

That said, I wouldn't necessarily oppose renaming the function, but I
also don't think it's particularly important to do so.