-
Notifications
You must be signed in to change notification settings - Fork 47
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
Bug fixes #69
Bug fixes #69
Conversation
|
@@ -9,6 +9,7 @@ | |||
#include "../Watcher.hh" | |||
|
|||
#define CONVERT_TIME(ts) ((uint64_t)ts.tv_sec * 1000000000 + ts.tv_nsec) | |||
#define IGNORED_FLAGS (kFSEventStreamEventFlagItemIsHardlink | kFSEventStreamEventFlagItemIsLastHardlink | kFSEventStreamEventFlagItemIsSymlink | kFSEventStreamEventFlagItemIsDir | kFSEventStreamEventFlagItemIsFile) |
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.
It seems to me that this change could address what #57 already indicated but I think there are more flags that should be ignored, such as:
kFSEventStreamEventFlagMustScanSubDirs
kFSEventStreamEventFlagUserDropped
kFSEventStreamEventFlagKernelDropped
kFSEventStreamEventFlagEventIdsWrapped
kFSEventStreamEventFlagHistoryDone
kFSEventStreamEventFlagMount
kFSEventStreamEventFlagUnmount
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.
I believe we should actually handle those, not ignore them. We already ran into a case in parcel where writing many files quickly produces these flags and means we can't determine what changed accurately. I believe the plan is to emit a specific error code in those cases so the client can handle them. We'll work on that separately.
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.
Yeah that makes sense. I was more referring to the extra stat
call and handling from here:
watcher/src/macos/FSEventsBackend.cc
Lines 73 to 74 in 728b44a
// If multiple flags were set, then we need to call `stat` to determine if the file really exists. | |
// This helps disambiguate creates, updates, and deletes. |
In case none of the expected flags are set. I think for the ones mentioned you wouldn't have to do the stat
call to figure out the kind of event because the flags do not seem to indicate a file change.
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.
Yeah for now what happens is it emits a "create"
event for the root directory. Parcel currently uses that to know that it must re-build the entire project because the changes were actually unknown. When we replace that with a better error code, we can remove 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.
Just to clarify, you are saying that this call:
watcher/src/macos/FSEventsBackend.cc
Line 76 in cca5ca3
if (stat(paths[i], &file)) { |
Will end up calling stat
on the root for the event types in #69 (comment)?
I wonder how other components could deal with this. VS Code would certainly not be able to do anything useful other than accepting that some file changes got dropped.
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.
Yeah that's what happens. When events are dropped, macOS emits a single event on the root directory with those flags set. At the moment since they aren't handled specially, it results in a create event because of the stat. In the future we'll emit a specific error code so clients can re-scan appropriately.
Fixes #67. Fixes #65. Fixes #66.