Conversation
The file watcher in darwin is recursive so adding and removing directories to watch was no-ops. One issue with that was that the filtering in cmd/govim/watcher.go that made sure files in directories that starts with '.' or '_' wasn't watched didn't apply in darwin. A consequence of that was that the test suite failed on darwin. We now apply a simple filter to added paths and suppress events for files outside those. Note that we do want to filter rather than configuring the file watcher to be non-recursive to avoid #492.
myitcv
reviewed
Feb 10, 2021
myitcv
left a comment
Member
There was a problem hiding this comment.
Added a couple of questions/thoughts on how we could do this differently.
| eventCh chan Event | ||
| es *fsevents.EventStream | ||
|
|
||
| // Darwin do recursive watching so we need to filter files in directories that |
| es *fsevents.EventStream | ||
|
|
||
| // Darwin do recursive watching so we need to filter files in directories that | ||
| // wasn't explicitly added. Note that it is desirable to watch recursively to avoid |
| // Darwin do recursive watching so we need to filter files in directories that | ||
| // wasn't explicitly added. Note that it is desirable to watch recursively to avoid | ||
| // a data race (#492). | ||
| watched map[string]bool // keyed by full path to directory |
Member
There was a problem hiding this comment.
Per our offline chant, I'm not clear why this is necessary. On Darwin, given the watcher is recursive (which is a good thing) we don't need to do the filepath.Walk that adds recursive watchers.
Instead, we can simply check on the event callback whether the event is for a file we care about. i.e. ignore . and _ prefixed directories, as well as submodules.
It's not perfect of course and there are loads of potential FS races... but that's the nature of the beast 😄
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The file watcher in darwin is recursive so adding and removing
directories to watch was no-ops. One issue with that was that the
filtering in cmd/govim/watcher.go that made sure files in directories
that starts with '.' or '_' wasn't watched didn't apply in darwin.
A consequence of that was that the test suite failed on darwin.
We now apply a simple filter to added paths and suppress events for
files outside those. Note that we do want to filter rather than
configuring the file watcher to be non-recursive to avoid #492.