Allow entr to watch directories (fixes #470)

[ffevotte]

This PR extends the work done in #492 to allow entr to watch directories. It is also re-based on the current master, in order to benefit from the latest bug fixes in entr.

At this stage, everything works in the case where Revise.watching_files[] == false. The tests performed for entr on directories mirror those defined for entr on files. In particular, they check that:

1. file creations, modifications and deletions in the directory are correctly detected;
2. clustered notifications trigger the callback only once;
3. empty directories are correctly handled (i.e. they remain in the watch list even when all files in them are deleted).

Upcoming commits should complete this PR to handle the case where Revise.watching_files[] is set.

[ffevotte]

With this new version, tests should pass for entr on directories, even when watching_files[] = true.

Here are some points mentioned in #492, on which I'm following up here since this is the most up-to-date PR.

When I started to think this was more complicated I was starting to get concerned that the emptiness of latestfiles: if there are no individual files that we're individually watching in the directory, notify will never be called. Not sure, I might have been going down a rabbit hole.

I tried to explicitly test for the specific case where watched directories are empty, but eventually realized that this was probably not what you talked about. This prompted me to investigate how things currently work, in a more detailed way. The reason the current implementation works for directories is because watching a directory (say: /path/to/directory/, with a mandatory trailing slash) results in putting an entry like the following in Revise.watched_files:

"/path/to/directory" => WatchList(1.59112e9, Dict{String,Base.PkgId}(""=> [top-level]))

The "empty filename" entry corresponds to watching the directory itself (as opposed to watching a file in it). That "empty filename" entry is examined each time the directory is modified. And watch_files_via_dir finds that it is not an existing file, not because it does not exist, but because it is a directory. This triggers an event in the same way as if a file deletion had been detected.

So there is an explanation why things actually currently work here. But it could probably be argued that this whole mechanism is a bit brittle: relying on the file-deletion notification process to trigger directory notifications is probably not a very good idea. And a negative side-effect which already affects us is that an additional 0.1s delay is introduced in the loop, which is unnecessary in this case.

I think the correct way to handle things here would be to unconditionally push the fullpath to latestfiles in case it is a directory. If you agree to this, I'll add such an implementation to the PR. If you have other ideas, please do not hesitate to share them!

Sorry again to be slow to respond, I've been pretty consumed by #497.

No worries! I have not been particularly responsive either... I could (and probably should) have made this PR last week!

As of be0b61f, CI runs its tests on Julia>=1.3, linux, once with file-watching false and once with true. The ability to do Pkg.test(; test_args=["REVISE_TESTS_WATCH_FILES"]) was implemented in 94b029b. So we should be sensitive to any bugs. If you're running Julia >=1.3, you can try that locally too.

Yes, this is what I used to test my implementation on my Linux system. The reason why I am (probably needlessly) concerned about BSD systems is the following:

• In my understanding, there are differences between Linux and BSD that make it impossible to watch a directory on a BSD system. Will these differences affect the behavior of FileWatching.watch_file (particularly when it is called on a directory)? It this is the case, tests passing on a Linux system with watching_files[]=true might not tell much about the behavior of this feature on BSD systems.
• An other way to consider the previous point: if watching directories actually works on BSD systems by simply calling FileWatching.watch_file on the "directory as a file", wouldn't it be beneficial to use this and have only one mode of operation?

That being said, the current implementation seems to pass all tests in both modes. Maybe a pragmatic way of doing things would be to ship this as it is, assume that things work everywhere, and wait to see whether BSD users complain.

[timholy]

I think the correct way to handle things here would be to unconditionally push the fullpath to latestfiles in case it is a directory.

That sounds great to me! Thanks for digging into the mechanism here.

[ffevotte]

You're very welcome! I just added it (in the same commit: I didn't feel like such a small change deserved its own commit)

Please don't hesitate to let me know what you think (but there is no rush!)

[timholy]

All looks great, but what's the motivation for the notification?

[ffevotte]

Oops, sorry! I used this in my early tests and forgot to remove the line. I replaced it with a comment in the commit I just pushed.

Thanks for the thorough review!

[timholy]

Thank you so much for a wonderful contribution!