Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files
On Thu, Oct 25, 2018 at 1:47 AM Nickolai Belakovski wrote: > The motivation for the change is some work that I'm doing to add a > worktree atom in ref-filter.c. I wanted that atom to be able to access > all fields of the worktree struct and noticed that lock_reason wasn't > getting populated so I figured I'd go and fix that. > > Reviewing this work in the context of your feedback and Junio's > previous comments, I think it makes sense to only have a field in the > struct indicating whether or not the worktree is locked, and have a > separate function for getting the reason. Is your new ref-filter atom going to be boolean-only or will it also have a form (or a separate atom) for retrieving the lock-reason? I imagine both could be desirable. In any event, implementation-wise, I would think that such an atom (or atoms) could be easily built with the existing worktree API (with its lazy-loading and caching), which might be an easy way forward since you wouldn't need this patch or the updated one you posted[1], thus no need to justify such a change. > Since the only cases in > which the reason is retrieved in the current codebase are cases where > the program immediately dies, caching seems a moot point. If your new atom has a form for retrieving the lock reason, then caching could potentially be beneficial(?). [1]: https://public-inbox.org/git/20181025055142.38077-1-nbelakov...@gmail.com/
Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files
The motivation for the change is some work that I'm doing to add a worktree atom in ref-filter.c. I wanted that atom to be able to access all fields of the worktree struct and noticed that lock_reason wasn't getting populated so I figured I'd go and fix that. I figured that since get_worktrees is already hitting the filesystem for the directory it wouldn't matter much if it also went and got the lock reason. Reviewing this work in the context of your feedback and Junio's previous comments, I think it makes sense to only have a field in the struct indicating whether or not the worktree is locked, and have a separate function for getting the reason. Since the only cases in which the reason is retrieved in the current codebase are cases where the program immediately dies, caching seems a moot point. I'll send an updated patch just after this message. Thanks for the feedback, happy to receive more. On Wed, Oct 24, 2018 at 1:11 AM Eric Sunshine wrote: > > On Wed, Oct 24, 2018 at 2:39 AM 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@gitster.mtv.corp.google.com/ > [2]: > https://public-inbox.org/git/xmqq4m9d0w6v@gitster.mtv.corp.google.com/
Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files
On Wed, Oct 24, 2018 at 2:39 AM 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@gitster.mtv.corp.google.com/ [2]: https://public-inbox.org/git/xmqq4m9d0w6v@gitster.mtv.corp.google.com/
