Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 24, 2018, 08:11 UTC
Message-ID: <CAPig+cRN_0VVe6dzhnmU73pgo-8ncPzmOx4bRrTBVvReLW6RfQ@mail.gmail.com>
In-Reply-To: <20181024063904.36096-1-nbelakovski@gmail.com>

On Wed, Oct 24, 2018 at 2:39 AM <nbelakovski@gmail.com> wrote:

Show 14 quoted lines

> lock_reason is now populated during the execution of get_worktrees
>
> is_worktree_locked has been simplified, renamed, and changed to internal
> linkage. It is simplified to only return the lock reason (or NULL in case
> there is no lock reason) and to not have any side effects on the inputs.
> As such it made sense to rename it since it only returns the reason.
>
> Since this function was now being used to populate the worktree struct's
> lock_reason field, it made sense to move the function to internal
> linkage and have callers refer to the lock_reason field. The
> lock_reason_valid field was removed since a NULL/non-NULL value of
> lock_reason accomplishes the same effect.
>
> Some unused variables within worktree source code were removed.

Thanks for the submission.

One thing which isn't clear from this commit message is _why_ this change is desirable at this time, aside from the obvious simplification of the code and client interaction (or perhaps those are the _why_?).

Although I had envisioned populating the "reason" field greedily in the way this patch does, not everyone agrees that doing so is desirable. In particular, Junio argued[1,2] for populating it lazily, which accounts for the current implementation. That's why I ask about the _why_ of this change since it will likely need to be justified in a such a way to convince Junio to change his mind.

Thanks.

[1]: https://public-inbox.org/git/xmqq8tyq5czn.fsf@gitster.mtv.corp.google.com/ [2]: https://public-inbox.org/git/xmqq4m9d0w6v.fsf@gitster.mtv.corp.google.com/