Re: filenotify.el

["Rüdiger Sonderfeld" <ruediger@c-plusplus.de>]

Hi,

On Tuesday 25 June 2013 14:02:38 Michael Albinus wrote:
> I have written filenotify.el, which is intended as upper layer for
> gfilenotify.c, inotify.c and w32notify.c. This is a simplified
> interface, which offers just file change or file attribute change
> notifications. I believe this is sufficient for the use-cases in Emacs;
> if more fine granular notifications are needed, one could still use one
> of the low-level packages.

Thanks for your work so far.  I agree for the upper layer portability is most 
important and not fine granularity.

> Beside further tests and bug fixing, I plan to write ert tests as well
> as a Tramp file name handler. Documentation is also lacking. But these
> steps could be applied later, once there is an agreement about the
> interface.

Yes, absolutely.  There is a very basic test for inotify which could be 
adopted.

> Comments?

I have a few comments:

+   (or (and (featurep 'gfilenotify)
+ 	   (not (featurep 'inotify))
+ 	   (not (featurep 'w32notify)))
+       (and (not (featurep 'gfilenotify))
+ 	   (featurep 'inotify)
+ 	   (not (featurep 'w32notify)))
+       (and (not (featurep 'gfilenotify))
+ 	   (not (featurep 'inotify))
+ 	   (featurep 'w32notify)))

Why is the support exclusive?

Maybe we could set `file-notify-support' to the name of the low-level feature.  
This would provide an easy way to identify which one is used.

+ ;; Shouldn't this be `file-notify-error'?

Yes, this would be more consistent in my opinion.

+ (defun file-notify-event-file1-name (event)
...
+ 	((featurep 'inotify) (nth 4 event))

The event layout for inotify is (WATCH-DESCRIPTOR ASPECTS COOKIE NAME).  The 
new file name will be NAME in the `moved-to' event and it is connected with 
the COOKIE to the `moved-from' event.

+     ;; Check, that event is meant for us.
+     (unless (setq callback (cdr (gethash descriptor file-notify-
descriptors)))
+       (signal 'filewatch-error (list "Not a valid descriptor" descriptor)))

I'm not sure if this should be treated as an error.  There could be the case 
that a file event is still in the queue while the watch-descriptor is removed 
and it will still be delivered.

But I just saw that this behaviour was also changed in `file-notify-handle-
event' a while ago.  I think this needs a bit more discussion.

+ 	     ((memq 'moved-from action) 'deleted)
+ 	     ((memq 'moved-to action) 'created)
...
+ 	     ((eq 'renamed-from action) 'deleted)
+ 	     ((eq 'renamed-to action) 'created)))))
...
+     ;; Apply callback.
+     (when (and (stringp file) (eq action 'moved))
+       (funcall callback (list descriptor 'deleted file))
+       (setq action 'created file file1))
+     (when (and (stringp file) action)
+       (funcall callback (list descriptor action file)))))

I don't think this is very useful because there is no way for the user to 
connect both events.  Inotify provides COOKIE to do this.

Maybe we should introduce a moved-from/moved-to event using a cookie instead.  
This should be easier to implement for all APIs.

+ 	     ((memq 'move-self action) 'deleted)))

Are you sure this is correct?  If I remember correctly then inotify sets the 
file name to the new file name.  I'll look into it.

Regards
Rüdiger