On Sun, Oct 28, 2018 at 4:03 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > On Sun, Oct 28, 2018 at 5:55 PM Nickolai Belakovski > > <nbelakovski@xxxxxxxxx> wrote: This was meant to be a reply to > > https://public-inbox.org/git/CAC05386F1X7TsPr6kgkuLWEwsmdiQ4VKTF5RxaHvzpkwbmXPBw@xxxxxxxxxxxxxx/T/#m8898c8f7c68e1ea234aca21cb2d7776b375c6f51, > > please look there for some more context. I think it both did and > > didn't get listed as a reply? In my mailbox I see two separate > > threads but in public-inbox.org/git it looks like it correctly got > > labelled as 1 thread. This whole mailing list thing is new to me, > > thanks for bearing with me as I figure it out :). > > Gmail threads messages entirely by subject; it doesn't pay attention > to In-Reply-To: or other headers for threading, which is why you see > two separate threads. public-inbox.org, on the other hand, does pay > attention to the headers, thus understands that all the messages > belong to the same thread. Gmail's behavior may be considered > anomalous. > Got it, thanks! > > Next time I'll make sure to change the subject line on updated > > patches as PATCH v2 (that's the convention, right?). > > That's correct. > (thumbs up) > > This is an improvement because it fixes an issue in which the fields > > lock_reason and lock_reason_valid of the worktree struct were not > > being populated. This is related to work I'm doing to add a worktree > > atom to ref-filter.c. > > Those fields are considered private/internal. They are not intended to > be accessed by calling code. (Unfortunately, only 'lock_reason' is > thus marked; 'lock_reason_valid' should be marked "internal".) Clients > are expected to retrieve the lock reason only through the provided > API, is_worktree_locked(). > > > I see your concerns about extra hits to the filesystem when calling > > get_worktrees and about users interested in lock_reason having to > > make extra calls. As regards hits to the filesystem, I could remove > > is_locked from the worktree struct entirely. To address the second > > concern, I could refactor worktree_locked_reason to return null if > > the wt is not locked. I would still want to keep is_worktree_locked > > around to provide a facility to check whether or not the worktree is > > locked without having to go get the reason. > > > > There's also been some concerns raised about caching. As I pointed > > out in the other thread, the current use cases for this information > > die upon accessing it, so caching is a moot point. For the use case > > of a worktree atom, caching would be relevant, but it could be done > > within ref-filter.c. Another option is to add the lock_reason back > > to the worktree struct and have two functions for populating it: > > get_worktrees_wo_lock_reason and get_worktrees_with_lock_reason. A > > bit more verbose, but it makes it clear to the caller what they're > > getting and what they're not getting. I might suggest starting with > > doing the caching within ref-filter.c first, and if more use cases > > appear for caching lock_reason we can consider the second option. It > > could also be get_worktrees and get_worktrees_wo_lock_reason, though > > I think most callers would be calling the latter name. > > > > So, my proposal for driving this patch to completion would be to: > > -remove is_locked from the worktree struct > > -refactor worktree_locked_reason to return null if the wt is not locked > > -refactor calls to is_locked within builtin/worktree.c to call > > either the refactored worktree_locked_reason or is_worktree_locked > > My impression, thus far, is that this all seems to be complicating > rather than simplifying. These changes also seem entirely unnecessary. > In [1], I made the observation that it seemed that your new ref-filter > atom could be implemented with the existing is_worktree_locked() API. > As far as I can tell, it can indeed be implemented without having to > make any changes to the worktree API or implementation at all. > > The worktree API is both compact and orthogonal, and I haven't yet > seen a compelling reason to change it. That said, though, the API > documentation in worktree.h may be lacking, even if the implementation > is not. I'll say a bit more about that below. > > > In addition to making the worktree code clearer, this patch fixes a > > bug in which the current is_worktree_locked over-eagerly sets > > lock_reason_valid. There are currently no consumers of > > lock_reason_valid within master, but obviously we should fix this > > before they appear :) > > As noted above, 'lock_reason_valid' is private/internal. It's an > accident that it is not annotated such (like 'lock_reason', which is > correctly annotated as "internal"). So, there should never be any > external consumers of that field. It also means that there is no bug > in the current code (as far as I can see) since that field is > correctly consulted (internally) to determine whether the lock reason > has been looked up yet. Thank you for explaining this. Looking at the code now it seems crystal clear, but, yea I clearly got on the wrong path initially. > > The missing "internal only" annotation is unfortunate since it may > have led you down this road of considering the implementation and API > broken. > > Moreover, the documentation for is_worktree_locked() apparently > doesn't convey strongly enough that it serves the dual purpose of (1) > telling you whether or not the worktree is locked, and (2) telling you > the reason it is locked. > > A patch which adds the missing "internal only" annotation to > 'lock_reason_valid', and which makes it easier to understand the dual > purpose of is_worktree_locked() would be welcome, especially if it > helps avoid such confusion in the future. > > Aside from that, it doesn't seem like worktree needs any changes for > the ref-filter atom you have in mind. (Don't interpret this > observation as me being averse to changes to the API; I'm open to > improvements, but haven't seen anything yet indicating a bug or > showing that the API is more difficult than it ought to be.) > > [1]: https://public-inbox.org/git/CAPig+cTvKd2DVu7wW_A31p_o7BaNJszu14kNRz9sqk8h45H4-g@xxxxxxxxxxxxxx/ You're right that these changes are not necessary in order to make a worktree atom. If there's no interest in this patch I'll withdraw it. I had found it really surprising that lock_reason was not populated when I was accessing it while working on the worktree atom. When digging into it, the "internal use" comment told me nothing, both because there's no convention (that I'm aware of) within C to mark fields as such and because it fails to direct the reader to is_worktree_locked. How about this, I can make a patch that changes the comment next to lock_reason to say "/* private - use is_worktree_locked */" (choosing the word "private" since it's a reserved keyword in C++ and other languages for implementation details that are meant to be inaccessible) and a comment next to lock_reason_valid that just says "/* private */"? I would also suggest renaming is_worktree_locked to worktree_lock_reason, the former makes me think the function is returning a boolean, whereas the latter more clearly conveys that a more detailed piece of information is being returned. Lemme know what you think.
