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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 29, 2018, 04:01 UTC
Message-ID: <CAPig+cTTsbz1pygq6G281V+fR2VVMuchvy1Q1H-KEvJpjJ9ejg@mail.gmail.com>
In-Reply-To: <CAC05387mfDhJ5_=LyzxZZX09MoY1hsmSB1gseNeLCmMOUx2O4A@mail.gmail.com>

On Sun, Oct 28, 2018 at 9:11 PM Nickolai Belakovski <nbelakovski@gmail.com> wrote:

Show 10 quoted lines

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

Show 13 quoted lines

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