Add recursive watcher for Windows backend #540
Conversation
Recursive watches can be added by using a "/..." parameter, similar to
the Go command:
w.Add("dir") // Behaves as before.
w.Add("dir/...") // Watch dir and all paths underneath it.
w.Remove("dir") // Remove the watch for dir and, if
// recursive, all paths underneath it too
w.Remove("dir/...") // Behaves like just "dir" if the path was
// recursive, error otherwise (probably
// want to add recursive remove too at some
// point).
The advantage of using "/..." vs. an option is that it can be easily
specified in configuration files and the like; for example from a TOML
file:
[watches]
dirs = ["/tmp/one", "/tmp/two/..."]
Options for this were previously discussed at:
#339 (comment)
This should be expanded to other backends too; I started with Windows
because the implementation is the both the easiest and has the least
amount of control (just setting a boolean parameter), and we can focus
mostly on writing tests and documentation and the for it, and we can
then match the inotify and kqueue behaviour to the Windows one.
Fixes #21
| @@ -166,6 +174,8 @@ events=$(<<EOF | |||
| // you may get hundreds of Write events, so you | |||
| // probably want to wait until you've stopped receiving | |||
| // them (see the dedup example in cmd/fsnotify). | |||
| // Some systems may send Write event for directories | |||
| // when the directory content changes. | |||
There was a problem hiding this comment.
This behaviour is kind of annoying, IMO; if you create a new file or directory it sends a Write on the directory, too:
> fsnotify watch ./test/...
> md NEWDIR
19:36:19.7176 1 CREATE "test\\NEWDIR"
> cd NEWDIR
> md SUBDIR
19:36:29.4840 2 CREATE "test\\NEWDIR\\SUBDIR"
19:36:30.4888 3 WRITE "test\\NEWDIR"
> cd SUBDIR
> echo x >newfile
19:36:56.9763 4 CREATE "test\\NEWDIR\\SUBDIR\\newfile"
19:36:56.9786 5 WRITE "test\\NEWDIR\\SUBDIR\\newfile"
19:36:58.2154 6 WRITE "test\\NEWDIR\\SUBDIR"
> cd ../..
> echo asd >zxczxc
19:38:40.8707 7 CREATE "test\\zxczxc"
19:38:40.8741 8 WRITE "test\\zxczxc"
This is also the case with non-recursirve watches:
> fsnotify watch ./test2
> md asd
19:45:37.6129 1 CREATE "test2\\asd"
> echo zxc > zxczxc
19:45:42.9015 2 CREATE "test2\\zxczxc"
19:45:42.9045 3 WRITE "test2\\zxczxc"
> cd asd
> echo zxc > zxczxc
19:45:49.0316 4 WRITE "test2\\asd"
It's also inconsistent: you don't get this Write event on the root directory.
The reason is because it seems to use timestamps:
FILE_NOTIFY_CHANGE_LAST_WRITE
Any change to the last write-time of files in the watched directory or subtree
causes a change notification wait operation to return. The operating system
detects a change to the last write-time only when the file is written to the
disk. For operating systems that use extensive caching, detection occurs only
when the cache is sufficiently flushed.FILE_ACTION_MODIFIED
The file was modified. This can be a change in the time stamp or attributes.
I don't see any way to change or disable this. So ... it is what it is, I guess :-/
|
will review tomorrow if someone doesn't get to it first, thanks |
|
I will try to find some time to review this as soon as I can. |
| @@ -1,6 +1,10 @@ | |||
| //go:build windows | |||
| // +build windows | |||
|
|
|||
| // Windows backend based on ReadDirectoryChangesW() | |||
There was a problem hiding this comment.
RE: Godoc, might be worth changing all instances of "/some/path/..." to be "/some/path/... or X:\some\path\... depending upon platform" or similar
There was a problem hiding this comment.
I added a note in the "Windows notes" section for this. I think using "/some/path/... or X:\some\path... depending upon platform" on every case where a path is mentioned would get a bit long, and I don't really expect people to be confused by it. Besides, forward slashes work well on Windows, and have since at least Windows 2000 AFAIK so it's not even needed to use \ instead of /.
|
(Oops - submitted review without comment.) The reasoning for using |
Co-authored-by: Milas Bowman <milasb@gmail.com>
|
I'll probably merge this in the weekend, so if someone still wants to do a review: now's your chance 😅 |
|
@arp242 - this is recursive watcher still valid? README.md says that: If it is still valid, for Windows, should it be |
Recursive watches can be added by using a "/..." parameter, similar to the Go command:
The advantage of using "/..." vs. an option is that it can be easily specified in configuration files and the like; for example from a TOML file:
Options for this were previously discussed at:
#339 (comment)
This should be expanded to other backends too; I started with Windows because the implementation is the both the easiest and has the least amount of control (just setting a boolean parameter), and we can focus mostly on writing tests and documentation and the for it, and we can then match the inotify and kqueue behaviour to the Windows one.
Fixes #21