On Sun, Oct 28, 2018 at 9:01 PM Eric Sunshine <sunsh...@sunshineco.com> wrote:
>
> On Sun, Oct 28, 2018 at 9:11 PM Nickolai Belakovski
> <nbelakov...@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
