Re: Upstreaming filenotify-recursive

[Michael Albinus]

Jethro Kuan <jethrokuan95@gmail.com> writes:

> Hello!

Hi Jethro,

I now found the time to dig into your code.

> I'd recently written a package that wraps filenotify somewhat
> transparently, providing a similar API to the built-in filenotify but it
> watches the directory recursively, spawning and destroying watchers as
> appropriately. It was suggested to me that this could be upstreamed to
> Emacs. The code is here:
>
> https://github.com/jethrokuan/filenotify-recursive

Which filenotify libraries do you have tested with? There are inotify,
kqueue, gfilenotify and w32notify as well as (for remote files)
inotifywait and gio. And for gfilenotify/gio, there are different
monitors like GFamFileMonitor, GFamDirectoryMonitor GInotifyFileMonitor,
GKqueueFileMonitor and GPollFileMonitor. All of them behave slightly
different.

Some of the native libraries allow even recursive watching of
directories, like inotifywait and (I believe) w32notify. This is not
implemented in Emacs' library wrappers, but for performance reasons, it
might be better to use this low level.

> I don't mind doing the work in getting this upstreamed, but I have a few
> concerns. Because of how filenotify works, filenotify-recursive uses own
> its global variables to keep track of the descriptors. It's best to
> think of a way to merge this functionality into filenotify.el itself,
> but I can't think of a good way right now. I'm open to any thoughts and
> suggestions here.

filenotify.el uses file-notify-descriptors for book-keeping:

--8<---------------cut here---------------start------------->8---
(file-notify-add-watch "~/.emacs.d" '(change) #'ignore)
=> (1 . 0)

(pp file-notify-descriptors)
=> #s(hash-table size 65 test equal rehash-size 1.5 rehash-threshold 0.8125 data
              ((1 . 0)
               #s(file-notify--watch "/home/albinus/.emacs.d" nil ignore)))
--8<---------------cut here---------------end--------------->8---

The hash key is the object returned by file-notify-add-watch, and the
value is a struct of type file-notify--watch.

In your library, you replace the callback by a closure for all watched
directories:

--8<---------------cut here---------------start------------->8---
(fnr-add-watch "~/.emacs.d" '(change) #'ignore)
=> "86fe4882-3d47-43a5-8841-4f5f2a85d558"

(pp file-notify-descriptors)
=> #s(hash-table size 145 test equal rehash-size 1.5 rehash-threshold 0.8125 
data
              ((1 . 0)
               #s(file-notify--watch "/home/albinus/.emacs.d" nil ignore)
               (1 . 1)
               #s(file-notify--watch "/home/albinus/.emacs.d" nil
                                     (closure
                                      ((callback . ignore)
                                       (uuid . 
"86fe4882-3d47-43a5-8841-4f5f2a85d558")
                                       cl-struct-fnr--watch-tags t)
                                      (event)
                                      (fnr--update-directory-watchers uuid 
event)
                                      (funcall callback event)))
               (2 . 0)
               #s(file-notify--watch "/home/albinus/.emacs.d/auto-save-list" nil
                                     (closure
                                      ((callback . ignore)
                                       (uuid . 
"86fe4882-3d47-43a5-8841-4f5f2a85d558")
                                       cl-struct-fnr--watch-tags t)
                                      (event)
                                      (fnr--update-directory-watchers uuid 
event)
                                      (funcall callback event)))
...

               (115 . 0)
               #s(file-notify--watch 
"/home/albinus/.emacs.d/url/cache/albinus/https/io/cloudimg/heise" nil
                                     (closure
                                      ((callback . ignore)
                                       (uuid . 
"86fe4882-3d47-43a5-8841-4f5f2a85d558")
                                       cl-struct-fnr--watch-tags t)
                                      (event)
                                      (fnr--update-directory-watchers uuid 
event)
                                      (funcall callback event)))))
--8<---------------cut here---------------end--------------->8---

IIUC, this is for calling fnr--update-directory-watchers. And the uuid
is used then in your own book-keeping variable fnr-descriptors. However,
this is not needed I believe. You could extend the file-notify--watch
struct by new slots parent and children, which refer to the watch which
has triggered watching the directory, and all the watches which are used
for subdirectories. Something like

--8<---------------cut here---------------start------------->8---
(cl-defstruct (fnr--watch (:include file-notify--watch))
  parent children)
--8<---------------cut here---------------end--------------->8---

Such objects would look in file-notify-descriptors as

--8<---------------cut here---------------start------------->8---
               (2 . 0)
               #s(file-notify--watch "/home/albinus/.emacs.d/auto-save-list" nil
                                     (closure
                                      ((callback . ignore)
                                       (uuid . 
"86fe4882-3d47-43a5-8841-4f5f2a85d558")
                                       cl-struct-fnr--watch-tags t)
                                      (event)
                                      (fnr--update-directory-watchers uuid 
event)
                                      (funcall callback event))
                                      ;; parent
                                      (1 . 1)
                                      ;; children
                                      ((10 . 0) (20 . 0) ...))
--8<---------------cut here---------------end--------------->8---

With this, you don't need uuids any longer, you could navigate in
file-notify-descriptors directly. Of course, you could also add other
slots as in the current definition of fnr--watch.

Some few comments on the code:

(defun fnr--subdirectories-recursively (dir &optional regexp predicate 
follow-symlinks)

This gives errors:

(fnr--subdirectories-recursively "/")
=> (file-error "Opening directory" "Permission denied" "//boot/efi")

I believe "//boot/efi" is a wrong file name. And not readable
subdirectories should be (silently) ignored.

      (unless (or (member file '("./" "../"))
                  (and regexp
                       (string-match regexp file)))

There is the defconst directory-files-no-dot-files-regexp which could be
used instead.

> Jethro

Best regards, Michael.