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

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Oct 24, 2018 at 2:39 AM <nbelakovski@gmail.com> wrote:
> 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/