Merged
Conversation
milas
force-pushed
the
milas/fix-windows-attrib
branch
3 times, most recently
from
878aa1b to
8a6b26b
Compare
Windows allows filtering file events to include changes to file attributes. However, when reporting the actual event, attribute changes are included as `FILE_ACTION_MODIFIED` so it's impossible to distinguish from file writes. This means the library's `op::Chmod` is never actually populated on Windows and so attribute changes aren't really usable. Since the API does not support specifying an event type filter as a caller, attribute changes on Windows are ignored rather than being misreported.
milas
force-pushed
the
milas/fix-windows-attrib
branch
from
8a6b26b to
f6dca35
Compare
Author
|
From MSDN docs on FILE_NOTIFY_INFORMATION (emphasis mine):
Given that the expected behavior from the library contract is to emit these as |
Author
|
Worth mentioning all tests including new one still pass on Windows 10 x64 with Go 1.16 for me |
nicks
approved these changes
Jun 30, 2021
Member
nicks
left a comment
nicks
left a comment
There was a problem hiding this comment.
LGTM
I might add a note to the effect of
"windows tends to have random security daemons that twiddle file attributes, generating spurious changes"
i could imagine some future person wanting to watch for file attribute changes on windows. But i agree that from a product standpoint, we can ignore them (especially since windows filesystems don't have an executable bit)
Author
|
Good call - I added some more context as well as a quick "notable changes" section in the README for anyone who ends up on this repo 🙃 |
milas
added a commit
to tilt-dev/tilt
that referenced
this pull request
Jul 1, 2021
Ignore file attribute changes on Windows. See tilt-dev/fsnotify#8 for details.
milas
added a commit
to tilt-dev/tilt
that referenced
this pull request
Jul 1, 2021
Ignore file attribute changes on Windows. See tilt-dev/fsnotify#8 for details.
milas
added a commit
to tilt-dev/tilt
that referenced
this pull request
Jul 1, 2021
Ignore file attribute changes on Windows. See tilt-dev/fsnotify#8 for details.
milas
added a commit
to tilt-dev/tilt
that referenced
this pull request
Jul 1, 2021
Ignore file attribute changes on Windows. See tilt-dev/fsnotify#8 for details.
arp242
added a commit
to fsnotify/fsnotify
that referenced
this pull request
Oct 14, 2022
On Windows a FILE_ACTION_MODIFIED event (i.e. a Write event) is triggered on file attribute changes, rather than some dedicates ATTRIBUTE_CHANGED event. Looking at the docs, I don't really see a way to distinguish between "real" write events and attribute changes. This is very odd, but seems to be how the ReadDirectoryChangesW() API works. The only way I can see to distinguish between the two events is to set up two filters: one with a FILE_NOTIFY_CHANGE_ATTRIBUTES, and one without. But that seems overly complex, and no one asked to get Chmod events for Windows; it's not really all that interesting on Windows anyway. The problem is that some software (anti-virus, backup software, etc.) can issue lots of attribute changes, causing a lot of "fake" Write events. So remove the FILE_NOTIFY_CHANGE_ATTRIBUTES and sysFSATTRIB flags. This was adapted from the tilt-dev/fsnotify fork: tilt-dev#8 Fixes #487
arp242
added a commit
to fsnotify/fsnotify
that referenced
this pull request
Oct 14, 2022
On Windows a FILE_ACTION_MODIFIED event (i.e. a Write event) is triggered on file attribute changes, rather than some dedicates ATTRIBUTE_CHANGED event. Looking at the docs, I don't really see a way to distinguish between "real" write events and attribute changes. This is very odd, but seems to be how the ReadDirectoryChangesW() API works. The only way I can see to distinguish between the two events is to set up two filters: one with a FILE_NOTIFY_CHANGE_ATTRIBUTES, and one without. But that seems overly complex, and no one asked to get Chmod events for Windows; it's not really all that interesting on Windows anyway. The problem is that some software (anti-virus, backup software, etc.) can issue lots of attribute changes, causing a lot of "fake" Write events. So remove the FILE_NOTIFY_CHANGE_ATTRIBUTES and sysFSATTRIB flags. This was adapted from the tilt-dev/fsnotify fork: tilt-dev#8 Fixes #487
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.
Windows allows filtering file events to include changes to file
attributes. However, when reporting the actual event, attribute
changes are included as
FILE_ACTION_MODIFIEDso it's impossibleto distinguish from file writes.
This means the library's
op::Chmodis never actually populatedon Windows and so attribute changes aren't really usable. Since
the API does not support specifying an event type filter as a
caller, attribute changes on Windows are ignored rather than being
misreported.