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

Previous Next

Full log

View this message in rfc822 format

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Mattias Engdegård <mattiase <at> acm.org>, 35418 <at> debbugs.gnu.org
Subject: bug#35418: [PATCH] Don't poll auto-revert files that use notification
Date: Sat, 27 Apr 2019 11:27:30 +0200

Eli Zaretskii <eliz <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.