On Sun, Oct 28, 2018 at 8:52 PM Junio C Hamano <gits...@pobox.com> wrote: > > > If the field "reason" should always be populated, there is *no* > reason why we need the "valid" boolean. They work as a pair to > realize lazy population of rarely used field. The lazy evaluation > technique is used as an optimization for common case, where majority > of operations do not care if worktrees are locked and if so why they > are locked, so that only rare operations that do want to find out > can ask "is this locked and why?" via is_worktree_locked() interface, > and at that point we lazily find it out by reading "locked" file. > > So it is by design that these fields are not always populated, but > are populated on demand as book-keeping info internal to the API's > implementation. It is not "an issue", and changing it is not a > "fix".
Having fields in a struct that are not populated by a getter function with no documentation indicating that they are not populated and no documentation explaining how to populate them is the issue here. > > In addition, if we have already checked, then we do not even do the > same check again. If in an earlier call we found out that a worktree > is not locked, we flip the _valid bit to true while setting _reason > to NULL, so that the next call can say "oh, that's not locked and we > can tell that without looking at the filesystem again" [*1*]. I clearly misunderstood the use case of the _valid flag, thanks for pointing it out. > > You are forcing the callers of get_worktrees() to pay the cost to > check, open and read the "why is this worktree locked?" file for all > worktrees, whether they care if these worktrees are locked or why > they are locked. Such a change can be an improvement *ONLY* if you > can demonstrate that in the current code most codepaths that call > get_worktrees() end up calling is_worktree_locked() on all worktrees > anyways. If that were the case, not having to lazily evaluate the > "locked"-ness, but always check upfront, would have a simplification > value, as either approach would be spending the same cost to open > and read these "locked" files. > > But I do not think it is the case. Outside builtin/worktree.c (and > you need to admit "git worktree" is a rather rare command in the > first place, so you shouldn't be optimizing for that if it hurts > other codepaths), builtin/branch.c wants to go to all worktrees and > update their HEAD when a branch is renamed (if the old HEAD is > pointing at the original name, of course), but that code won't care > if the worktree is locked at all. I do not think of any caller of > get_worktrees() that want to know if it is locked and why for each > and every one of them, and I'd be surprised if that *is* the > majority, but as a proposer to burden get_worktrees() with this > extra cost, you certainly would have audited the callers and made > sure it is worth making them pay the extra cost? > > If we are going to change anything around this area, I'd not be > surprised that the right move is to go in the opposite direction. > Right now, you cannot just get "is it locked?" boolean answer (which > can be obtained by a simple stat(2) call) without getting "why is it > locked?" (which takes open(2) & read(2) & close(2)), and if you are > planning a new application that wants to ask "is it locked?" a lot > without having to know the reason, you may want to make the lazy > evaluation even lazier by splitting _valid field into two (i.e. a > "do we know if this is locked?" valid bit covers "is_locked" bit, > and another "do we know why this is locked?" valid bit accompanies > "locked_reason" string). And the callers would ask two separate > questions: is_worktree_locked() that says true or false, and then > why_worktree_locked() that yields NULL or string (i.e. essentially > that is what we have as is_worktree_locked() today). Of course, > such a change must also be justified with a code audit to > demonstrate that only minority case of the callers of is-locked? > wants to know why > > > [Footnote] > > *1* The codepaths that want to know if a worktree is locked or not > (and wants to learn the reason) are so rare and concentrated in > builtin/worktree.c, and more importantly, they do not need to ask > the same question twice, so we can stop caching and make > is_worktree_locked() always go to the filesystem, I think, and that > may be a valid change _if_ we allow worktrees to be randomly locked > and unlocked while we are looking at them, but if we want to worry > about such concurrent and competing uses, we need a big > repository-wide lock anyway, and it is the least of our problems > that the current caching may go stale without getting invalidated. > The code will be racing against such concurrent processes even if > you made it to go to the filesystem all the time. > Basically, I already implemented most of what you're saying. The v2 proposal does force all callers of get_worktrees to check the lock status, but by calling stat, not open/read/close. That being said you're right that even forcing them to call stat when most don't care is imposing an extra cost for no gain. The v2 proposal no longer caches the lock reason (in fact it removes it from the worktree struct), since not only do current users have no need to ask for the lock_reason twice, none of them ask for it twice in the first place. The v2 proposal provides a standalone function for getting the actual reason (leaving it up to callers to cache the result if they like). I'd be up for removing is_locked from the struct as well and making a separate standalone function for that. Either way, I do see an issue with the current code that anybody who wants to know the lock status and/or lock reason of a worktree gets faced with a confusing, misleading, and opaque piece of code. I see two possible remedies: a) Remove these fields from the worktree struct and provide standalone functions for answering these questions. Pros are that we follow single responsibility principle with two standalone functions for doing so, and we make the route for answering these questions less circuitous (IMO). Cons are that we remove the caching currently in place since we're no longer storing this info in the struct, but then again that caching is not currently being used and can be implemented by callers if they really need it b) Update the comments in the code to state that lock_reason and lock_reason_valid are to be considered private fields and to use is_worktree_locked for populating them. Pros are that no actual code changes need to be made. Cons are that, IMO, it's still a strange piece of code in that it's doing some sort of quasi object oriented stuff in C, and if we can take the opportunity to make the code look a bit more canonical I think we should, but that's just my 2 cents. Of course there's also option c, which is that I leave this alone and just go back to making my worktree atom :)
