-
-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmd/govim: add directory filtering to darwin file watcher #1038
base: main
Are you sure you want to change the base?
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a couple of questions/thoughts on how we could do this differently.
|
||
type fswatcher struct { | ||
eventCh chan Event | ||
es *fsevents.EventStream | ||
|
||
// Darwin do recursive watching so we need to filter files in directories that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s/do/does/
|
||
type fswatcher struct { | ||
eventCh chan Event | ||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s/wasn't/weren't/
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
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.