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
[off-list ref] 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(...))
            ...

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.