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

From: Nickolai Belakovski <nbelakovski@gmail.com>
Date: Oct 29, 2018, 05:45 UTC
Message-ID: <CAC05387rFq0yJ3nUVkb0jUyQy=EmZiCnBW9L53A6GS5=U0qUDg@mail.gmail.com>
In-Reply-To: <CAPig+cTTsbz1pygq6G281V+fR2VVMuchvy1Q1H-KEvJpjJ9ejg@mail.gmail.com>

On Sun, Oct 28, 2018 at 9:01 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 17 quoted lines

>
> On Sun, Oct 28, 2018 at 9:11 PM Nickolai Belakovski
> <nbelakovski@gmail.com> wrote:
> > 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.

Actually it's 3:2 in the current code for callers getting the reason out of the function vs callers checking the value of the pointer for null/not null. This leads to some rather unnatural looking code in the current repo like

reason = is_worktree_locked(wt);

I think it would look a lot more natural if it were "reason = worktree_lock_reason(wt)". The resulting if-statement wouldn't be too bad, IMO

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

To me, I would just go lookup the signature of worktree_lock_reason and see that it returns a pointer and I'd be satisfied with that. I could also infer that from looking at the code if I'm just skimming through. But if I see code like "reason = is_worktree_locked(wt)" I'm like hold on, what's going on here?! :P