Re: filenotify.el

[Michael Albinus]

Rüdiger Sonderfeld <address@hidden> writes:

> Hi,

Hi Rüdiger,

> 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?

Just to be sure. The different low-level packages are not fully
compatible (event names differ, for example); it would be a nightmare
for filenotify.el when the different packages compete under the hood.

In general, it could be possible to apply several low-level file
notification libraries in parallel. But this wouldn't bring additional
features, so we should avoid this.

(Out of curiosity, I let compete file monitoring for "/tmp" and
"/ssh::/tmp" in parallel. Very funny.)

> 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.

Really? I don't see a serious difference between (featurep 'inotify) and
(eq file-notify-support 'inotify)

> + ;; Shouldn't this be `file-notify-error'?
>
> Yes, this would be more consistent in my opinion.

If nobody objects, I will change it here as well as in the low-level
packages, which use `file-error'.

> + (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.

Just now, this function is used only for gfilenotify, event `moved'. I
will remove the other two cases from that function.

> +     ;; 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.

I believe, that the callback function needs special care from the
callee. In `auto-revert-notify-handler', I wrap the sensitive code by
`ignore-errors'. For example.

But I agree, this might be discussed.

> +          ((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.

That's also my intention. But Glib's implementation does not implement
this for all backends, see the comment in
<https://developer.gnome.org/gio/2.36/GFile.html#GFileMonitorFlags-enum>.

The w32notify case I couldn't test yet.

It is on my todo list; once there is a robust implementation for all
low-level packages, we shall add a `renamed' event to be returned.

> +          ((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.

I'll check also.

> Regards
> Rüdiger

Best regards, Michael.