Re: [PATCH 3/7] worktree: teach worktree_lock_reason() to gently handle main worktree

> On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> The main worktree should not be locked and the worktree_lock_reason() API
>> is aware of this fact and avoids running the check code for the main
>> worktree. This checks is done via assert() macro, Therefore the caller
>
> s/Therefore/therefore/
>

>> needs to ensure the function is never called, usually by additional code.
>>
>> We can handle that case more gently by just returning false for the main
>> worktree and not bother checking if the "locked" file exists. This will
>> allowed further simplification from the caller as they will not need to
>> ensure the main worktree is never passed to the API.
>>
>> Teach worktree_lock_reason() to be more gently and just return false for
>> the main working tree.
>
> The situation is even a bit worse since the main worktree restriction
> isn't even documented. Here's a possible rewrite of the commit message
> which addresses that point too:
>
>     worktree_lock_reason() aborts with an assertion failure when
>     called on the main worktree since locking the main worktree is
>     nonsensical. Not only is this behavior undocumented, thus callers
>     might not even be aware that the call could potentially crash the
>     program, but it also forces clients to be extra careful:
>
>         if (!is_main_worktree(wt) && worktree_locked_reason(...))
>            ...
>
>     Since we know that locking makes no sense in the context of the
>     main worktree, we can simply return false for the main worktree,
>     thus making client code less complex by eliminating the need for
>     callers to have inside knowledge about the implementation:
>
>         if (worktree_locked_reason(...))
>             ...
>

>> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
>> ---
>> diff --git a/worktree.c b/worktree.c
>> @@ -225,9 +225,7 @@ int is_main_worktree(const struct worktree *wt)
>>  const char *worktree_lock_reason(struct worktree *wt)
>>  {
>> -       assert(!is_main_worktree(wt));
>> -
>> -       if (!wt->lock_reason_valid) {
>> +       if (!is_main_worktree(wt) && !wt->lock_reason_valid) {
>>                 struct strbuf path = STRBUF_INIT;
>
> As mentioned in my review of patch [2/7], this would be more idiomatic
> and easier to reason about if the function returns early for the main
> worktree case, thus freeing the reader from having to think about that
> case for the remainder of the code. So:
>
>     if (is_main_worktree(wt))
>         return NULL;
>     if (!wt->lock_reason_valid) {
>         ...
>
> Subjective, and not necessarily worth a re-roll, though.