bug#35418: [PATCH] Don't poll auto-revert files that use notification

[Michael Albinus <michael.albinus@gmx.de>]

Eli Zaretskii <eliz@gnu.org> writes:

Hi,

>> > If you look at bug reports and discussions around the time this
>> > comment was written, you will find the descriptions of the use cases
>> > that caused this design.  AFAIR, the main problem was with inotify,
>> > not with w32notify.
>>
>> The inotify problems at the time seem to have stemmed from not using
>> unique notification descriptors. This was fixed some time ago
>> (158bb8555d etc, bug#26126).
>
> I'll let Michael decide on this.

Well, in inotify you still get undesired notifications. Like this:

--8<---------------cut here---------------start------------->8---
(write-region "foo" nil "/tmp/foo")
(add-name-to-file "/tmp/foo" "/tmp/bar" 'ok)

(inotify-add-watch "/tmp/foo" t (lambda (event) (message "inotify %S" event)))
=> (1 . 0)
(inotify-add-watch "/tmp/bar" t (lambda (event) (message "inotify %S" event)))
=> (1 . 1)


(write-region "foo" nil "/tmp/foo")
=> inotify ((1 . 0) (modify) "/tmp/foo" 0)
   inotify ((1 . 1) (modify) "/tmp/bar" 0)
   inotify ((1 . 0) (open) "/tmp/foo" 0)
   inotify ((1 . 1) (open) "/tmp/bar" 0)
   inotify ((1 . 0) (modify) "/tmp/foo" 0)
   inotify ((1 . 1) (modify) "/tmp/bar" 0)
   inotify ((1 . 0) (close-write) "/tmp/foo" 0)
   inotify ((1 . 1) (close-write) "/tmp/bar" 0)
--8<---------------cut here---------------end--------------->8---

However, in filenotify this is fixed:

--8<---------------cut here---------------start------------->8---
(file-notify-add-watch "/tmp/foo" '(change attribute-change)
                       (lambda (event) (message "file-notify %S" event)))
=> (2 . 0)
(file-notify-add-watch "/tmp/bar" '(change attribute-change)
                       (lambda (event) (message "file-notify %S" event)))
=> (2 . 1)

(write-region "foo" nil "/tmp/foo")
=> file-notify ((2 . 0) changed "/tmp/foo")
   inotify ((1 . 0) (modify) "/tmp/foo" 0)
   inotify ((1 . 1) (modify) "/tmp/bar" 0)
   inotify ((1 . 0) (open) "/tmp/foo" 0)
   inotify ((1 . 1) (open) "/tmp/bar" 0)
   file-notify ((2 . 0) changed "/tmp/foo")
   inotify ((1 . 0) (modify) "/tmp/foo" 0)
   inotify ((1 . 1) (modify) "/tmp/bar" 0)
   inotify ((1 . 0) (close-write) "/tmp/foo" 0)
   inotify ((1 . 1) (close-write) "/tmp/bar" 0)
--8<---------------cut here---------------end--------------->8---

Unrelated events for "/tmp/bar" are filtered out in
`file-notify-callback'. So yes, the inotify problems seem to be fixed.

>> Are you arguing that the default value of
>> auto-revert-notify-exclude-dir-regexp should not be extended in the
>> proposed way, or that the variable is fundamentally incompatible
>> with the patch?
>
> I'm questioning the usefulness of extending the default value, yes.
> But I don't have strong views on that.

We might extend this variable. *If* this regexp matches a file name, we
know that we need polling. But it is clear, that we cannot catch all
cases by just parsing file names.

(Btw, we should use the value of `mounted-file-systems', introduced in
Emacs 26.1, when initializing `auto-revert-notify-exclude-dir-regexp'.)

One alternative approach could be to analyze the file system device
number, as returned by `file-attributes'. By this, we could detect
mounted file systems.

But I don't believe that this information is always trustworty, given it
isn't used anywhere. And at least for remote files it doesn't tell you
anything. Furthermore, mounted file systems are not the only reason that
file notification doesn't work, and we need to poll.

> Thanks.

Best regards, Michael.