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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 35418 in the body.
You can then email your comments to 35418 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: bug-gnu-emacs <at> gnu.org
Cc: Michael Albinus <michael.albinus <at> gmx.de>
Subject: [PATCH] Don't poll auto-revert files that use notification
Date: Wed, 24 Apr 2019 20:14:46 +0200

[Message part 1 (text/plain, inline)]

It is a waste of power, on battery-powered devices in particular, to poll files in auto-revert mode periodically when change notification is used. The change is straightforward (attached patch); the main concern is whether the notification system is reliable enough.

In general, it probably is. There is a comment in w32notify.c about SMB-mounted file systems from Samba servers; while Samba does support notification nowadays, there are probably older systems still be deficient in that regard. However, isn't this what `auto-revert-notify-exclude-dir-regexp' is for? I'm not familiar with the way Emacs is used on Windows, but would adding something like

 (rx bos
     (or "\\\\" "//")
     (one-or-more (not (any "/:\\")))
     (any "/\\"))

to `auto-revert-notify-exclude-dir-regexp' be a good start?

Another note about what this patch does not do: global-auto-revert-mode will still use polling. This could be added later on, if there is a good place to hook into for buffer creation.

[0001-Don-t-poll-auto-revert-files-that-use-notification.patch (application/octet-stream, attachment)]

Message #8 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: michael.albinus <at> gmx.de, 35418 <at> debbugs.gnu.org
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Wed, 24 Apr 2019 21:58:18 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Wed, 24 Apr 2019 20:14:46 +0200
> Cc: Michael Albinus <michael.albinus <at> gmx.de>
> 
> It is a waste of power, on battery-powered devices in particular, to poll files in auto-revert mode periodically when change notification is used. The change is straightforward (attached patch); the main concern is whether the notification system is reliable enough.

The polling was added for a reason, and the reason was not reliability
of the notifications.  The reason is hinted upon in this comment:

  ;; If we have file notifications, we want to update the auto-revert buffers
  ;; immediately when a notification occurs. Since file updates can happen very
  ;; often, we want to skip some revert operations so that we don't spend all our
  ;; time reverting the buffer.
  ;;
  ;; We do this by reverting immediately in response to the first in a flurry of
  ;; notifications. We suppress subsequent notifications until the next time
  ;; `auto-revert-buffers' is called (this happens on a timer with a period set by
  ;; `auto-revert-interval').

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.

> In general, it probably is. There is a comment in w32notify.c about SMB-mounted file systems from Samba servers; while Samba does support notification nowadays, there are probably older systems still be deficient in that regard. However, isn't this what `auto-revert-notify-exclude-dir-regexp' is for? I'm not familiar with the way Emacs is used on Windows, but would adding something like
> 
>  (rx bos
>      (or "\\\\" "//")
>      (one-or-more (not (any "/:\\")))
>      (any "/\\"))
> 
> to `auto-revert-notify-exclude-dir-regexp' be a good start?

If you imply that Samba drives can be identified by the syntax of the
file name alone, then I don't think this is a valid assumption.  A
certain drive letter can be mapped to a Samba volume, and we can never
know that by looking just at the file name.

More generally, auto-revert-notify-exclude-dir-regexp is for any
situation where a filesystem doesn't cause notifications.  You will
find caveats about such issues in the documentation of every
notification system we support.

Thanks.

Message #11 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Wed, 24 Apr 2019 21:36:14 +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.

Inotify didn't work on mounted directories. I don't know whether this
has improved.

Gfile is agnostic to file systems being mounted or not. If inotify (used
internally) doesn't work, it uses polling. However, gfile has shown
instability; that's why it isn't the first choice anymore.

I don't remember the behavior of kqueue.

>> However, isn't this what `auto-revert-notify-exclude-dir-regexp' is
>> for? I'm not familiar with the way Emacs is used on Windows, but
>> would adding something like
>>
>>  (rx bos
>>      (or "\\\\" "//")
>>      (one-or-more (not (any "/:\\")))
>>      (any "/\\"))
>>
>> to `auto-revert-notify-exclude-dir-regexp' be a good start?
>
> If you imply that Samba drives can be identified by the syntax of the
> file name alone, then I don't think this is a valid assumption.  A
> certain drive letter can be mapped to a Samba volume, and we can never
> know that by looking just at the file name.
>
> More generally, auto-revert-notify-exclude-dir-regexp is for any
> situation where a filesystem doesn't cause notifications.  You will
> find caveats about such issues in the documentation of every
> notification system we support.

The default value of `auto-revert-notify-exclude-dir-regexp' tries to
identify common mount points, like /mnt or /media and alike. But like Eli
said already for Samba mounts, we cannot detect all of them by their
name.

What might be an alternative is to let the user decide. If we provide a
user option `auto-revert-dont-poll', a user could set it to t, and would
live with the consequences. If she tries to enable autorevert for a
mounted directory, which is not covered by file notifications, she might
be surprised.

> Thanks.

Best regards, Michael.

Message #14 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Mattias Engdegård <mattiase <at> acm.org>,
 35418 <at> debbugs.gnu.org
Subject: Re: [PATCH] Don't poll auto-revert files that use notification
Date: Wed, 24 Apr 2019 21:59:09 +0200

Mattias Engdegård <mattiase <at> acm.org> writes:

> It is a waste of power, on battery-powered devices in particular, to
> poll files in auto-revert mode periodically when change notification
> is used. The change is straightforward (attached patch); the main
> concern is whether the notification system is reliable enough.

An alternative approach is discussed at <https://debbugs.gnu.org/35418>:
set auto-revert-interval to a very large value (like most-positive-fixnum).

Sounds reasonable to me, if we adapt the documentation.

Best regards, Michael.

Message #17 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

24 apr. 2019 kl. 20.58 skrev Eli Zaretskii <eliz <at> gnu.org>:
> 
> The polling was added for a reason, and the reason was not reliability
> of the notifications.  The reason is hinted upon in this comment:
> 
>  ;; If we have file notifications, we want to update the auto-revert buffers
>  ;; immediately when a notification occurs. Since file updates can happen very
>  ;; often, we want to skip some revert operations so that we don't spend all our
>  ;; time reverting the buffer.
>  ;;
>  ;; We do this by reverting immediately in response to the first in a flurry of
>  ;; notifications. We suppress subsequent notifications until the next time
>  ;; `auto-revert-buffers' is called (this happens on a timer with a period set by
>  ;; `auto-revert-interval').

Thank you, interesting! In any case, that should not be a problem: the patch takes care of it in a more principled way, by the means of a timer. Currently, auto-revert is inhibited until next periodic poll, which can be anything between 0 and 5 seconds away. The patch sets this to a fixed value (2.5 s).

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

> If you imply that Samba drives can be identified by the syntax of the
> file name alone, then I don't think this is a valid assumption.  A
> certain drive letter can be mapped to a Samba volume, and we can never
> know that by looking just at the file name.

Certainly, but the intent was to add something like the attempts to identify network file systems on Unix machines:

   "^" (regexp-opt '("/afs/" "/media/" "/mnt" "/net/" "/tmp_mnt/"))

If that regexp is acceptable as rough heuristics on Unix, surely something like the regexp proposed, matching \\SOMETHING\, shouldn't be out of the question on Windows? Full precision cannot be attained, as you point out, but perhaps we can make life easier for the user.

> More generally, auto-revert-notify-exclude-dir-regexp is for any
> situation where a filesystem doesn't cause notifications.  You will
> find caveats about such issues in the documentation of every
> notification system we support.

Yes, that is my understanding as well. 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?

Message #20 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 35418 <at> debbugs.gnu.org
Subject: Re: [PATCH] Don't poll auto-revert files that use notification
Date: Thu, 25 Apr 2019 11:58:29 +0200

24 apr. 2019 kl. 21.59 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
> An alternative approach is discussed at <https://debbugs.gnu.org/35418>:
> set auto-revert-interval to a very large value (like most-positive-fixnum).

Sorry, did you mean a different bug number?

Message #23 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: michael.albinus <at> gmx.de, 35418 <at> debbugs.gnu.org
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Thu, 25 Apr 2019 13:04:57 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Thu, 25 Apr 2019 11:56:59 +0200
> Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
> 
> >  ;; If we have file notifications, we want to update the auto-revert buffers
> >  ;; immediately when a notification occurs. Since file updates can happen very
> >  ;; often, we want to skip some revert operations so that we don't spend all our
> >  ;; time reverting the buffer.
> >  ;;
> >  ;; We do this by reverting immediately in response to the first in a flurry of
> >  ;; notifications. We suppress subsequent notifications until the next time
> >  ;; `auto-revert-buffers' is called (this happens on a timer with a period set by
> >  ;; `auto-revert-interval').
> 
> Thank you, interesting! In any case, that should not be a problem: the patch takes care of it in a more principled way, by the means of a timer. Currently, auto-revert is inhibited until next periodic poll, which can be anything between 0 and 5 seconds away. The patch sets this to a fixed value (2.5 s).
> 
> > 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.

>    "^" (regexp-opt '("/afs/" "/media/" "/mnt" "/net/" "/tmp_mnt/"))
> 
> If that regexp is acceptable as rough heuristics on Unix, surely something like the regexp proposed, matching \\SOMETHING\, shouldn't be out of the question on Windows? Full precision cannot be attained, as you point out, but perhaps we can make life easier for the user.

on Windows, SOMETHING is just the name of the machine which exports
the drive, it can be anything.

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

Thanks.

Message #26 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 35418 <at> debbugs.gnu.org
Subject: Re: [PATCH] Don't poll auto-revert files that use notification
Date: Thu, 25 Apr 2019 13:04:42 +0200

Mattias Engdegård <mattiase <at> acm.org> writes:

>> An alternative approach is discussed at <https://debbugs.gnu.org/35418>:
>> set auto-revert-interval to a very large value (like most-positive-fixnum).
>
> Sorry, did you mean a different bug number?

Oops, I meant <https://emacs.stackexchange.com/a/50134/2427>. Cut'n'waste error, while
following Emacs Berlin meetup in parallel.

Best regards, Michael.

Message #29 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

25 apr. 2019 kl. 13.04 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
> Oops, I meant <https://emacs.stackexchange.com/a/50134/2427>. Cut'n'waste error, while
> following Emacs Berlin meetup in parallel.

A most understandable error!

Regarding the discussion you refer to, drastically raising auto-revert-interval cannot be a practical solution in the general case, nor was it likely proposed as such. Even with the patch, polling will be used in many cases, including: filenotify failure (from running out of file descriptors, for example); deleted file; and anything matching auto-revert-notify-exclude-dir-regexp.

Message #32 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

25 apr. 2019 kl. 12.04 skrev Eli Zaretskii <eliz <at> gnu.org>:
> 
>>   "^" (regexp-opt '("/afs/" "/media/" "/mnt" "/net/" "/tmp_mnt/"))
>> 
>> If that regexp is acceptable as rough heuristics on Unix, surely something like the regexp proposed, matching \\SOMETHING\, shouldn't be out of the question on Windows? Full precision cannot be attained, as you point out, but perhaps we can make life easier for the user.
> 
> on Windows, SOMETHING is just the name of the machine which exports
> the drive, it can be anything.

Right, it's just a path to a file or directory on a mounted remote file system.

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

Nor do I. It would avoid breaking auto-revert on some non-auto-revert-capable remote file systems on Windows, at the cost of increasing the auto-revert delay on some auto-revert-capable remote file systems.
The patch does not hinge on a change to the default value of that variable.

I forgot to mention that this patch should and will be accompanied by appropriate NEWS and documentation updates, to be written.

Message #35 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

24 apr. 2019 kl. 21.36 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
> Inotify didn't work on mounted directories. I don't know whether this
> has improved.

By 'work', do you mean receiving notification about changes made by the same machine or another machine? As far as I know, the NFS protocol has no means of propagating notifications, in contrast to SMB.

> What might be an alternative is to let the user decide. If we provide a
> user option `auto-revert-dont-poll', a user could set it to t, and would
> live with the consequences. If she tries to enable autorevert for a
> mounted directory, which is not covered by file notifications, she might
> be surprised.

That is a possibility, although I'm generally not too fond of user-adjustable behaviour of this sort. If I understand you right, you propose that the default value should be 'always poll'?
I would prefer the default to be 'avoid polling' -- I doubt that malfunctions will be common or serious if they occur -- but I suppose it is better than the status quo.

Message #38 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Message #41 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

>> Inotify didn't work on mounted directories. I don't know whether this
>> has improved.
>
> By 'work', do you mean receiving notification about changes made by
> the same machine or another machine? As far as I know, the NFS
> protocol has no means of propagating notifications, in contrast to
> SMB.

It fires notifications for changes made by the same machine:

--8<---------------cut here---------------start------------->8---
;; "/net/ford/albinus" is a mounted directory.
(write-region "foo" nil "/net/ford/albinus/tmp/foo")

(inotify-add-watch "/net/ford/albinus/tmp/foo"
		   t (lambda (event) (message "inotify %S" event)))
=> (3 . 0)

(write-region "foo" nil "/net/ford/albinus/tmp/foo")
=> inotify ((3 . 0) (modify) "/net/ford/albinus/tmp/foo" 0)
   inotify ((3 . 0) (open) "/net/ford/albinus/tmp/foo" 0)
   inotify ((3 . 0) (modify) "/net/ford/albinus/tmp/foo" 0)
   inotify ((3 . 0) (close-write) "/net/ford/albinus/tmp/foo" 0)
--8<---------------cut here---------------end--------------->8---

If I make a modification on the remote machine, nothing happens:

--8<---------------cut here---------------start------------->8---
;; Remote "/ssh:ford:/share/albinus" is the same as local "/net/ford/albinus".
(write-region "foo" nil "/ssh:ford:/share/albinus/tmp/foo")
--8<---------------cut here---------------end--------------->8---

>> What might be an alternative is to let the user decide. If we provide a
>> user option `auto-revert-dont-poll', a user could set it to t, and would
>> live with the consequences. If she tries to enable autorevert for a
>> mounted directory, which is not covered by file notifications, she might
>> be surprised.
>
> That is a possibility, although I'm generally not too fond of
> user-adjustable behaviour of this sort. If I understand you right, you
> propose that the default value should be 'always poll'?

The policy in Emacs is to set the default value to be compatible with
previous behavior. If time passes, and we see no drawback, the default
value could be changed. Usually, the next major E,acs version .

> I would prefer the default to be 'avoid polling' -- I doubt that
> malfunctions will be common or serious if they occur -- but I suppose
> it is better than the status quo.

Yes.

Best regards, Michael.

Message #44 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

> From: Michael Albinus <michael.albinus <at> gmx.de>
> Cc: Mattias Engdegård <mattiase <at> acm.org>,
>   35418 <at> debbugs.gnu.org
> Date: Sat, 27 Apr 2019 11:27:30 +0200
> 
> 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.

Only on Posix systems, right?

Message #47 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Eli Zaretskii <eliz <at> gnu.org> writes:

Hi Eli,

>> 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.
>
> Only on Posix systems, right?

Don't know. I haven't used the device number ever myself. And in Tramp,
I set it to a virtual value which is distinct from values used for local
file systems, or from values from other remote connections, w/o any
further checking.

Best regards, Michael.

Message #50 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

27 apr. 2019 kl. 11.27 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
> 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)

Thanks for the example!

I wouldn't call this undesired. Create a hard link to a file, ask for notification on both links, and then modify the file. Then both notifiers trigger, because someone has modified the file they were watching. The kqueue back-end behaves similarly.

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

Actually, it is (arguably) a bug. With two buffers referring to distinct hard links for the same file, surely we want a change in that file to trigger notification for both! (It's quite an exotic case, not the least because Emacs normally recognises hard links as if they were the same file name.)

However, with the kqueue back-end, file-notify watches do trigger for both, as expected.

The reason is that file-notify does not call inotify-add-watch on individual files, as in your example above, but on their containing directory ("/tmp" in your example). When monitoring a directory with two hard links to the same file, and the file is changed, inotify (sensibly) only reports a change to one of the links (the one employed for the change). Thus, the logic is in the Linux kernel, not in filenotify.

For kqueue it is different: here, changes to files are not reported when a watch is monitoring their directory, so filenotify.el sets kqueue watches on each file instead. The same could be done with inotify (and w32notify, if I read the code right), but watching directories has certain advantages.

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

Are you really sure that the inotify problems were really about changes to files with multiple hard links? It sounds very unlikely, and as showed above, the behaviour differs between back-ends.

If I were to guess, the problem was rather that the inotify back-end used to return the kernel-provided descriptor number, which is the same for the same directory: when /dir/a and /dir/b (distinct files, not hard links) both were watched, inotify would monitor /dir twice and give the same descriptor for both, with the ensuing chaos. This was subsequently fixed by making inotify return unique descriptors.

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

That variable contains "^//[^/]+/" on Windows, so we need to make up our minds about it.

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

Sort of; the interpretation is tricky, and as Eli commented, quite platform-specific.

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

What other reasons are you thinking about?

Message #53 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

27 apr. 2019 kl. 11.40 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
>> By 'work', do you mean receiving notification about changes made by
>> the same machine or another machine? As far as I know, the NFS
>> protocol has no means of propagating notifications, in contrast to
>> SMB.
> 
> It fires notifications for changes made by the same machine:
[...]
> If I make a modification on the remote machine, nothing happens:

Thank you for confirming that. The current state of remote notifications on Linux seems to be:

- could possibly be added to cifs in the future
- no explicit support in NFS but could possibly use NFS v4.1 directory delegations
- either requires VFS-level support
- no work in progress as far as I can tell

> The policy in Emacs is to set the default value to be compatible with
> previous behavior. If time passes, and we see no drawback, the default
> value could be changed. Usually, the next major E,acs version .

A most sensible policy, although reality isn't always black or white -- sometimes, a useful change outweighs a minor incompatibility.

Thanks again, I'll prepare a new patch with a defcustom.

Message #56 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Sat, 27 Apr 2019 18:19:36 +0200
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 35418 <at> debbugs.gnu.org
> 
> The reason is that file-notify does not call inotify-add-watch on individual files, as in your example above, but on their containing directory ("/tmp" in your example). When monitoring a directory with two hard links to the same file, and the file is changed, inotify (sensibly) only reports a change to one of the links (the one employed for the change). Thus, the logic is in the Linux kernel, not in filenotify.
> 
> For kqueue it is different: here, changes to files are not reported when a watch is monitoring their directory, so filenotify.el sets kqueue watches on each file instead. The same could be done with inotify (and w32notify, if I read the code right), but watching directories has certain advantages.

w32notify cannot watch a single file, because the Windows notification
machinery is directory-oriented, and reports all changes in the
directory.

Message #59 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

27 apr. 2019 kl. 18.52 skrev Eli Zaretskii <eliz <at> gnu.org>:
> 
> w32notify cannot watch a single file, because the Windows notification
> machinery is directory-oriented, and reports all changes in the
> directory.

Right; thanks for the correction.

Here is an updated patch. There is a new variable, `auto-revert-always-poll', which is t by default.
There is also a note in etc/NEWS. Does it merit a mention in the manual as well?

[0001-Don-t-poll-auto-revert-files-that-use-notification.patch (application/octet-stream, attachment)]

Message #62 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

> Actually, it is (arguably) a bug. With two buffers referring to
> distinct hard links for the same file, surely we want a change in that
> file to trigger notification for both! (It's quite an exotic case, not
> the least because Emacs normally recognises hard links as if they were
> the same file name.)

By design, in filenotify.el, we want see only events which are related
to the file *name*. If you want to be notified for both buffers, you
need to watch both file (names).

(Well, re-reading the docstring and the manual for `file-notify-add-watch',
this isn't said explicitly. Likely, we shall precise this.)

> However, with the kqueue back-end, file-notify watches do trigger for
> both, as expected.

Hmm, this is inconsistent. Worth a buig report?

> The reason is that file-notify does not call inotify-add-watch on
> individual files, as in your example above, but on their containing
> directory ("/tmp" in your example). When monitoring a directory with
> two hard links to the same file, and the file is changed, inotify
> (sensibly) only reports a change to one of the links (the one employed
> for the change). Thus, the logic is in the Linux kernel, not in
> filenotify.
>
> For kqueue it is different: here, changes to files are not reported
> when a watch is monitoring their directory, so filenotify.el sets
> kqueue watches on each file instead. The same could be done with
> inotify (and w32notify, if I read the code right), but watching
> directories has certain advantages.

It was a design decision, that filenotify.el implements directory
watching. Since kqueue does not support this, it must be emulated, somehow.

>> 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.
>
> Sort of; the interpretation is tricky, and as Eli commented, quite
> platform-specific.

I'm also not in favor of this approach, I just wanted to mention it.

>> 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.
>
> What other reasons are you thinking about?

The reasons you have already quoted somewhere else: sometimes, file
notification is not applicable; there are not enough descriptors left; a
file might have been deleted; a file notification process has been
killed silently; you name it ...

Best regards, Michael.

Message #65 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

> Here is an updated patch. There is a new variable,
> `auto-revert-always-poll', which is t by default.
> There is also a note in etc/NEWS. Does it merit a mention in the manual as well?

Yes, please.

> +(defcustom auto-revert-always-poll t
> +  "Non-nil to poll files even if notification is available.
> +
> +Set this variable to nil to save power by avoiding polling when
> +possible.  Files on file-systems that do not support file
> +notifications must match `auto-revert-notify-exclude-dir-regexp'
> +for Auto-Revert to work properly in this case.  This typically
> +includes network file systems on Unix-like machines, for files
> +that are modified from another computer.
> +
> +When non-nil, buffers in Auto-Revert Mode will always be polled
> +for changes to their files on disk every `auto-revert-interval'
> +seconds.
> +
> +In Global Auto-Revert Mode, polling is always done regardless of
> +the value of this variable."

I believe it shall be said, that this user option does not compete with
`auto-revert-use-notify'. Rather, polling is used additionally to file
notification. When `auto-revert-use-notify' is nil, the value of
`auto-revert-always-poll' doesn't matter; there will always be polling.

Saying this, the user option might need another name. What about
`auto-revert-also-poll'?

> +(defvar auto-revert--polled-buffers ()
> +  "List of buffers in Auto-Revert Mode that must be polled.
> +It contains the buffers in `auto-revert-buffer-list' whose
> +`auto-revert-notify-watch-descriptor' is nil.")

Is this variable needed? It is used only once in
`auto-revert--need-polling', and it could be computed easily by
(untested)

(delq nil
  (mapcar (lambda (buf)
            (and (or auto-revert-always-poll
                     (not auto-revert-notify-watch-descriptor))
                 buf))
          auto-revert-buffer-list))

`auto-revert--need-polling' shall always return the buffer list, also for
`global-auto-revert-mode'.

Otherwise, the patch might work. Let's try it.

Best regards, Michael.

Message #68 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

29 apr. 2019 kl. 09.53 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
>> Here is an updated patch. There is a new variable,
>> `auto-revert-always-poll', which is t by default.
>> There is also a note in etc/NEWS. Does it merit a mention in the manual as well?
> 
> Yes, please.

There is now a paragraph added to the manual.

By the way, the organisation of this part of the manual could be improved -- don't you agree?

There is a section called Reverting, which starts about `revert-buffer' but then goes on to talk about the auto-revert, global-auto-revert and auto-revert-tail modes and details about the mechanisms behind them: polling, intervals, notification.

Then there is a (sibling) section called Autorevert, which despite its name only talks about auto-reverting non-file buffers.

This can be reorganised in various ways. We could move all autorevert text to a sibling node to Reverting, or to one or more child nodes. In any case, such text shuffling should not be part of this patch.

> I believe it shall be said, that this user option does not compete with
> `auto-revert-use-notify'. Rather, polling is used additionally to file
> notification. When `auto-revert-use-notify' is nil, the value of
> `auto-revert-always-poll' doesn't matter; there will always be polling.

Good point; the doc string has been clarified.

> Saying this, the user option might need another name. What about
> `auto-revert-also-poll'?

Naming is always hard. I started with `auto-revert-avoid-polling' but wanted to avoid a negative name.
I tried `auto-revert-also-poll' but it somehow didn't feel right; not all buffers use notification.
It is nothing I feel strongly about, so if you do prefer that name I'll change, but I've kept the original name in the patch for now.

>> +(defvar auto-revert--polled-buffers ()
>> +  "List of buffers in Auto-Revert Mode that must be polled.
>> +It contains the buffers in `auto-revert-buffer-list' whose
>> +`auto-revert-notify-watch-descriptor' is nil.")
> 
> Is this variable needed? It is used only once in
> `auto-revert--need-polling', and it could be computed easily by

It is also used in `auto-revert-buffers', but you are quite right that it could be a function. I decided to maintain it as a derived state because it felt silly to replace O(1) code with O(N), and the invariant is clear enough (stated in its doc string). (Some of the places where the variable is updated are O(N) but less frequently executed.)

I can replace it with a function if you want, but the code didn't seem to gain much from doing so.

> `auto-revert--need-polling' shall always return the buffer list, also for
> `global-auto-revert-mode'.

Sorry, it was meant as a predicate and is only used as such.
Clarified by renaming it to `auto-revert--need-polling-p'.

Thank you very much for your review! Updated patch attached.

[0001-Don-t-poll-auto-revert-files-that-use-notification.patch (application/octet-stream, attachment)]

Message #71 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

29 apr. 2019 kl. 09.19 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
> By design, in filenotify.el, we want see only events which are related
> to the file *name*. If you want to be notified for both buffers, you
> need to watch both file (names).

Well yes, but you want a change to the file to be reported for both buffers, even if they watch different names, right?

Otherwise, it wouldn't make sense at all. If someone is watching a file, surely it is because changes to the contents of that file are of interest? Why would the name employed to carry out the changes matter?

I'm quite sure there is a simple misunderstanding here; probably my fault. And again, I don't think it matters much in practice since users are unlikely to have buffers for different hard links to the same file. Let's not waste too much time on this.

>> However, with the kqueue back-end, file-notify watches do trigger for
>> both, as expected.
> 
> Hmm, this is inconsistent. Worth a buig report?

Not really, because (a) multiple hard links are rare, (b) even more rare in Emacs, and (c) inotify isn't used that way by auto-revert (the directory is watched, not the files).

> It was a design decision, that filenotify.el implements directory
> watching. Since kqueue does not support this, it must be emulated, somehow.

Well, auto-revert only uses filenotify.el for watching changes to files (that is, the data corresponding to the names). How filenotify does that isn't very important. I suppose watching directories when possible has the advantages:

+ fewer (kernel-level) descriptors used if there are multiple files of interest in the same directory
+ notification about re-created previously removed files

with at least one disadvantage:

- changes to files not of interest have to be considered and rejected, spending more CPU and power. This can be non-trivial; consider looking at a single non-changing file in a very busy directory with files being added and removed all the time.

For kqueue and w32notify (and FSEvent) there isn't much choice.

>> What other reasons are you thinking about?
> 
> The reasons you have already quoted somewhere else: sometimes, file
> notification is not applicable; there are not enough descriptors left; a
> file might have been deleted; a file notification process has been
> killed silently; you name it ...

Thank you. Most of those cases should not cause any trouble -- except unreliable file notification processes, but since `auto-revert-remote-files' defaults to nil, it didn't look like a serious problem.

Message #74 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

> 29 apr. 2019 kl. 09.53 skrev Michael Albinus <michael.albinus <at> gmx.de>:
>> 
>>> Here is an updated patch. There is a new variable,
>>> `auto-revert-always-poll', which is t by default.
>>> There is also a note in etc/NEWS. Does it merit a mention in the
>>> manual as well?
>> 
>> Yes, please.
>
> There is now a paragraph added to the manual.
>
> By the way, the organisation of this part of the manual could be
> improved -- don't you agree?

I hardly disagree, this is always true :-)

> There is a section called Reverting, which starts about
> `revert-buffer' but then goes on to talk about the auto-revert,
> global-auto-revert and auto-revert-tail modes and details about the
> mechanisms behind them: polling, intervals, notification.
>
> Then there is a (sibling) section called Autorevert, which despite its
> name only talks about auto-reverting non-file buffers.
>
> This can be reorganised in various ways. We could move all autorevert
> text to a sibling node to Reverting, or to one or more child nodes. In
> any case, such text shuffling should not be part of this patch.

I would let it for you.

>> Saying this, the user option might need another name. What about
>> `auto-revert-also-poll'?
>
> Naming is always hard. I started with `auto-revert-avoid-polling' but
> wanted to avoid a negative name.
> I tried `auto-revert-also-poll' but it somehow didn't feel right; not
> all buffers use notification.
> It is nothing I feel strongly about, so if you do prefer that name
> I'll change, but I've kept the original name in the patch for now.

I have also hard times when choosing a proper name. Do what you believe
is best suited, unless Eli comes with something better. (It is my
experience over years, that he beats me always with better names.)

>> Is this variable needed? It is used only once in
>> `auto-revert--need-polling', and it could be computed easily by
>
> It is also used in `auto-revert-buffers', but you are quite right that
> it could be a function.

Yes, but the function as proposed would fit as well.

> I decided to maintain it as a derived state
> because it felt silly to replace O(1) code with O(N), and the
> invariant is clear enough (stated in its doc string). (Some of the
> places where the variable is updated are O(N) but less frequently
> executed.)

Yes, but is N large enough to experience the difference?

> I can replace it with a function if you want, but the code didn't seem
> to gain much from doing so.

There are several places you need to modify the variable. This gave me
the impression that one function would fit better, because if you need
to touch (set) avariable at several places, there are good chances to
miss it somewhere. I'm not saying you do in this case, it is just my
style to keep things together (in one function, for example).

>> `auto-revert--need-polling' shall always return the buffer list, also for
>> `global-auto-revert-mode'.
>
> Sorry, it was meant as a predicate and is only used as such.
> Clarified by renaming it to `auto-revert--need-polling-p'.

My proposal was to use it NOT as a predicate, but as a function
returning the buffer list.

> Thank you very much for your review! Updated patch attached.

Best regards, Michael.

Message #77 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

>> By design, in filenotify.el, we want see only events which are related
>> to the file *name*. If you want to be notified for both buffers, you
>> need to watch both file (names).
>
> Well yes, but you want a change to the file to be reported for both
> buffers, even if they watch different names, right?

You don't know first hand, which buffers contain the same file hard
linked together. This can be determined only via the inode and device
numbers; something we don't apply yet.

How do you know otherwise, that "/tmp/foo" and "/tmp/bar" are the same,
visited in different buffers?

> Otherwise, it wouldn't make sense at all. If someone is watching a
> file, surely it is because changes to the contents of that file are of
> interest? Why would the name employed to carry out the changes matter?

That's a desirable feature, I agree. But we haven't implemented it
yet. Likely, we shall say so in the doc.

> I'm quite sure there is a simple misunderstanding here; probably my
> fault. And again, I don't think it matters much in practice since
> users are unlikely to have buffers for different hard links to the
> same file. Let's not waste too much time on this.

Agreed. I won't change something in this respect, until there is a bug
report / feature request. And as said, maybe you could add a sentence
about in the manual.

> Thank you. Most of those cases should not cause any trouble -- except
> unreliable file notification processes, but since
> `auto-revert-remote-files' defaults to nil, it didn't look like a
> serious problem.

As Tramp maintainer, I always set `auto-revert-remote-files' to t :-)
So I care.

Best regards, Michael.

Message #80 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Mon, 29 Apr 2019 19:23:53 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Mon, 29 Apr 2019 13:06:50 +0200
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 35418 <at> debbugs.gnu.org
> 
> By the way, the organisation of this part of the manual could be improved -- don't you agree?

Could be.

> There is a section called Reverting, which starts about `revert-buffer' but then goes on to talk about the auto-revert, global-auto-revert and auto-revert-tail modes and details about the mechanisms behind them: polling, intervals, notification.
> 
> Then there is a (sibling) section called Autorevert, which despite its name only talks about auto-reverting non-file buffers.

You say "section" but the names you cite are node names, not section
names.  The latter are slightly more descriptive.

> This can be reorganised in various ways. We could move all autorevert text to a sibling node to Reverting, or to one or more child nodes. In any case, such text shuffling should not be part of this patch.

I think we should have sibling sections "Reverting" and "Autorevert",
with the latter describing both types of auto-reverting.  And
"Reverting" should have a cross-reference to "Autorevert" for
automatic reverting of file-visiting buffers.

Would you like to submit a patch to that effect?

> > Saying this, the user option might need another name. What about
> > `auto-revert-also-poll'?
> 
> Naming is always hard. I started with `auto-revert-avoid-polling' but wanted to avoid a negative name.
> I tried `auto-revert-also-poll' but it somehow didn't feel right; not all buffers use notification.
> It is nothing I feel strongly about, so if you do prefer that name I'll change, but I've kept the original name in the patch for now.

I actually think auto-revert-dont-poll is better, even though it's
negative.

Thanks.

Message #83 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: mattiase <at> acm.org, 35418 <at> debbugs.gnu.org
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Mon, 29 Apr 2019 19:24:59 +0300

> From: Michael Albinus <michael.albinus <at> gmx.de>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  35418 <at> debbugs.gnu.org
> Date: Mon, 29 Apr 2019 14:18:20 +0200
> 
> > Naming is always hard. I started with `auto-revert-avoid-polling' but
> > wanted to avoid a negative name.
> > I tried `auto-revert-also-poll' but it somehow didn't feel right; not
> > all buffers use notification.
> > It is nothing I feel strongly about, so if you do prefer that name
> > I'll change, but I've kept the original name in the patch for now.
> 
> I have also hard times when choosing a proper name. Do what you believe
> is best suited, unless Eli comes with something better. (It is my
> experience over years, that he beats me always with better names.)

Really?  I actually consider myself being bad with naming.  Let's see
if you like my suggestion this time.

Message #86 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

29 apr. 2019 kl. 14.18 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
>> I decided to maintain it as a derived state
>> because it felt silly to replace O(1) code with O(N), and the
>> invariant is clear enough (stated in its doc string). (Some of the
>> places where the variable is updated are O(N) but less frequently
>> executed.)
> 
> Yes, but is N large enough to experience the difference?

These things are tricky to measure, but obviously inefficient code just doesn't feel right to write. For example, generating a list just to see if it is non-empty, when that could be determined in a more straightforward way.

> My proposal was to use it NOT as a predicate, but as a function
> returning the buffer list.

Very well; here is an incremental patch (to make the differences clear). It's a compromise: the derived state is gone, but there are two functions: one for the list of buffers that need to be polled, and one for whether that list would be non-empty.

By the way, the patch now uses functions from cl-lib, not just macros. Is there any reason not to?

[0001-Eliminate-the-auto-revert-polled-buffers-variable.patch (application/octet-stream, attachment)]

Message #89 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

29 apr. 2019 kl. 14.26 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
> You don't know first hand, which buffers contain the same file hard
> linked together. This can be determined only via the inode and device
> numbers; something we don't apply yet.
> 
> How do you know otherwise, that "/tmp/foo" and "/tmp/bar" are the same,
> visited in different buffers?

Thanks, I think I understand what you are concerned about now.
It seems to work just as we expect it to (at least in kqueue and inotify):

A file has three hard links, /dir1/a, /dir1/b and /dir2/c. Then:

- A watch set on /dir1/a will report changes made to the file via any of the three links (kqueue, inotify).
- A watch set on /dir1 will report changes made to the file via /dir1/a and /dir1/b, but not /dir2/c (inotify).

Since filenotify would use /dir1 to watch /dir1/a with inotify, but /dir1/a with kqueue, the behaviour differs.
We could work around the problem by setting watches on files directly with inotify, but it's not worth the trouble or the other drawbacks (as mentioned earlier) for such an uncommon case.

> As Tramp maintainer, I always set `auto-revert-remote-files' to t :-)
> So I care.

Right, so I suppose a user like you would either:

(a) not set `auto-revert-always-poll' to nil
(b) trust remote file notification to work well enough, and if it fails, it's not a disaster (no data lost)
(c) add a pattern to `auto-revert-notify-exclude-dir-regexp' to disable particularly unreliable notifications

which sounds acceptable.

Message #92 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

29 apr. 2019 kl. 18.23 skrev Eli Zaretskii <eliz <at> gnu.org>:
> 
>> There is a section called Reverting, which starts about `revert-buffer' but then goes on to talk about the auto-revert, global-auto-revert and auto-revert-tail modes and details about the mechanisms behind them: polling, intervals, notification.
>> 
>> Then there is a (sibling) section called Autorevert, which despite its name only talks about auto-reverting non-file buffers.
> 
> You say "section" but the names you cite are node names, not section
> names.  The latter are slightly more descriptive.

Correct, thank you. (The node names attract the eyes since they are highlighted as links.)

>> This can be reorganised in various ways. We could move all autorevert text to a sibling node to Reverting, or to one or more child nodes. In any case, such text shuffling should not be part of this patch.
> 
> I think we should have sibling sections "Reverting" and "Autorevert",
> with the latter describing both types of auto-reverting.  And
> "Reverting" should have a cross-reference to "Autorevert" for
> automatic reverting of file-visiting buffers.
> 
> Would you like to submit a patch to that effect?

I'll see what I can do, once we are done with this particular patch.

>>> Saying this, the user option might need another name. What about
>>> `auto-revert-also-poll'?
>> 
>> Naming is always hard. I started with `auto-revert-avoid-polling' but wanted to avoid a negative name.
>> I tried `auto-revert-also-poll' but it somehow didn't feel right; not all buffers use notification.
>> It is nothing I feel strongly about, so if you do prefer that name I'll change, but I've kept the original name in the patch for now.
> 
> I actually think auto-revert-dont-poll is better, even though it's
> negative.

Then I'd prefer auto-revert-avoid-polling; 'don't poll' sounds definitive but we may still have to poll from time to time.

Message #95 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,
 
>> I actually think auto-revert-dont-poll is better, even though it's
>> negative.
>
> Then I'd prefer auto-revert-avoid-polling; 'don't poll' sounds
> definitive but we may still have to poll from time to time.

Sounds good to my ears.

Best regards, Michael.

Message #98 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

> We could work around the problem by setting watches on files directly
> with inotify, but it's not worth the trouble or the other drawbacks
> (as mentioned earlier) for such an uncommon case.

If you go back in history (for years), you will find in the archives
that we have tried different policies. None of them is perfect, so
unless we see serious problems, I propose to keep the things as they are.

>> As Tramp maintainer, I always set `auto-revert-remote-files' to t :-)
>> So I care.
>
> Right, so I suppose a user like you would either:
>
> (a) not set `auto-revert-always-poll' to nil
> (b) trust remote file notification to work well enough, and if it
> fails, it's not a disaster (no data lost)
> (c) add a pattern to `auto-revert-notify-exclude-dir-regexp' to
> disable particularly unreliable notifications
>
> which sounds acceptable.

Don't know what other people do, but I'll take (a). Since I have written
remote file notifications myself, likely I'm the one who has the least
trust for (b) :-) (c) could help, yes.

Best regards, Michael.

Message #101 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

> Very well; here is an incremental patch (to make the differences
> clear). It's a compromise: the derived state is gone, but there are
> two functions: one for the list of buffers that need to be polled, and
> one for whether that list would be non-empty.

Thanks, this goes to the right direction.

> By the way, the patch now uses functions from cl-lib, not just
> macros. Is there any reason not to?

No problem.

From my POV you could push it (with the final decision for the name from
Eli). If there are problems, people will react soon - that's my
experience with autorevert changes.

But we have autorevert-tests.el, so at least the important cases are
covered. I'm wondering if there are some tests which need to be added.

Best regards, Michael.

Message #104 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Zhang Haijun <ccsmile2008 <at> outlook.com>
To: "35418 <at> debbugs.gnu.org" <35418 <at> debbugs.gnu.org>
Subject: bug#35418: [PATCH] Don't poll auto-revert files that use notification
Date: Tue, 30 Apr 2019 01:03:21 +0000

> It was a design decision, that filenotify.el implements directory watching. Since kqueue does not support this, it must be emulated, somehow. 
> 

It seems that it is not true for kqueue on macOS 10.13.6.

Several weeks ago, I met a problem with emacs auto-revert. Some files in a directory can’t be auto reverted.  This directory was a soft link to another directory. I did some debug and  found that no event would be received if you use file watching for the files in a soft link directory. And use directory watching for these files worked well. So I modified filenotify.el like this:

diff --git a/lisp/filenotify.el b/lisp/filenotify.el
index 101ddb6be0..a4a0359328 100644
--- a/lisp/filenotify.el
+++ b/lisp/filenotify.el
@@ -363,7 +363,7 @@ file-notify-add-watch
       (setq desc (funcall
                   ;; kqueue does not report file changes in directory
                   ;; monitor.  So we must watch the file itself.
-                  func (if (eq file-notify--library 'kqueue) file dir)
+                  func (if (eq file-notify--library 'kqueue11) file dir)
                   l-flags 'file-notify-callback)))

     ;; Modify `file-notify-descriptors’.

It works well since then.

I don’t known from when the behavior of kqueue changed. There maybe need a user option to control whether to use file watching or directory watching for kqueue.

Message #107 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: mattiase <at> acm.org, 35418 <at> debbugs.gnu.org
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Tue, 30 Apr 2019 06:57:38 +0300

> From: Michael Albinus <michael.albinus <at> gmx.de>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  35418 <at> debbugs.gnu.org
> Date: Mon, 29 Apr 2019 22:17:23 +0200
> 
> >From my POV you could push it (with the final decision for the name from
> Eli).

I'm okay with the name.

Message #110 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Zhang Haijun <ccsmile2008 <at> outlook.com>
Cc: "35418 <at> debbugs.gnu.org" <35418 <at> debbugs.gnu.org>
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Tue, 30 Apr 2019 09:06:54 +0200

Zhang Haijun <ccsmile2008 <at> outlook.com> writes:

Hi,

> diff --git a/lisp/filenotify.el b/lisp/filenotify.el
> index 101ddb6be0..a4a0359328 100644
> --- a/lisp/filenotify.el
> +++ b/lisp/filenotify.el
> @@ -363,7 +363,7 @@ file-notify-add-watch
>        (setq desc (funcall
>                    ;; kqueue does not report file changes in directory
>                    ;; monitor.  So we must watch the file itself.
> -                  func (if (eq file-notify--library 'kqueue) file dir)
> +                  func (if (eq file-notify--library 'kqueue11) file dir)
>                    l-flags 'file-notify-callback)))
>
>      ;; Modify `file-notify-descriptors’.

I don't understand the patch. Symbol kqueue11 does not exist, so do you mean

+                  func dir

And have you applied the tests in filenotify-tests.el? Do all of them pass?

Best regards, Michael.

Message #113 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

30 apr. 2019 kl. 05.57 skrev Eli Zaretskii <eliz <at> gnu.org>:
> 
>> From: Michael Albinus <michael.albinus <at> gmx.de>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>,  35418 <at> debbugs.gnu.org
>> Date: Mon, 29 Apr 2019 22:17:23 +0200
>> 
>>> From my POV you could push it (with the final decision for the name from
>> Eli).
> 
> I'm okay with the name.

Thank you, pushed (c61bbb4c8e). What remains to be done:

- reorganise the manual as discussed before
- see what it would take to make the change work in global-auto-revert-mode

and of course fix any lingering concerns that may turn up.

Message #116 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

> Thank you, pushed (c61bbb4c8e). What remains to be done:

Thanks!

> and of course fix any lingering concerns that may turn up.

Let's start with this one: <https://emba.gnu.org/emacs/emacs/-/jobs/1587/raw>

Best regards, Michael.

Message #119 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

30 apr. 2019 kl. 14.59 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
> Let's start with this one: <https://emba.gnu.org/emacs/emacs/-/jobs/1587/raw>

Oh dear. Now fixed in the correct way, I hope. Thank you!

Message #122 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

> 30 apr. 2019 kl. 14.59 skrev Michael Albinus <michael.albinus <at> gmx.de>:
>> 
>> Let's start with this one: <https://emba.gnu.org/emacs/emacs/-/jobs/1587/raw>
>
> Oh dear. Now fixed in the correct way, I hope. Thank you!

Yes, looks better. You can always run a dedicated test like this:

--8<---------------cut here---------------start------------->8---
$ make -C test autorevert-tests
--8<---------------cut here---------------end--------------->8---

See also test/README.

Best regards, Michael.

Message #125 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Mon, 29 Apr 2019 20:58:15 +0200
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 35418 <at> debbugs.gnu.org
> 
> A file has three hard links, /dir1/a, /dir1/b and /dir2/c. Then:
> 
> - A watch set on /dir1/a will report changes made to the file via any of the three links (kqueue, inotify).
> - A watch set on /dir1 will report changes made to the file via /dir1/a and /dir1/b, but not /dir2/c (inotify).

Just FTR, w32notify reports changes made through any of the 3 links
when it watches dir1.  This is consistent with MS documentation, which
says that changing the file's data are reflected to all the hard links
immediately.

Message #128 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

mån 2019-04-29 klockan 19:23 +0300 skrev Eli Zaretskii:
> > 
> I think we should have sibling sections "Reverting" and "Autorevert",
> with the latter describing both types of auto-reverting.  And
> "Reverting" should have a cross-reference to "AutorevertAuto-" for
> automatic reverting of file-visiting buffers.

Here is a patch that does roughly that. I'm not entirely happy with the
old 'Auto-reverting non-buffer files' section, whose node name was just
'Autorevert'. I would have preferred it as a subsection to the new
auto-revert section, along with its existing two subsections, but since
its place isn't the same in the on-line and printed manuals, that
seemed technically tricky without duplicating a lot of text.

[0001-Reorganise-auto-revert-nodes-in-the-manual.patch (text/x-patch, attachment)]

Message #131 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Zhang Haijun <ccsmile2008 <at> outlook.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: "35418 <at> debbugs.gnu.org" <35418 <at> debbugs.gnu.org>
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Wed, 1 May 2019 02:17:32 +0000

> I don't understand the patch. Symbol kqueue11 does not exist, so do you mean
> 
> +                  func dir
Yes.

> 
> And have you applied the tests in filenotify-tests.el? Do all of them pass?
> 
> Best regards, Michael.

$ make -C test autorevert-tests
 ELC      lisp/autorevert-tests.elc
 GEN      lisp/autorevert-tests.log
Running 5 tests (2019-05-01 10:06:38+0800)
Reverting buffer `auto-revert-testqe4qed'.
  passed  1/5  auto-revert-test00-auto-revert-mode
(Shell command succeeded with no output)
Test auto-revert-test01-auto-revert-several-files backtrace:
 signal(ert-test-failed (((should (string-match "another text" (buffe
 ert-fail(((should (string-match "another text" (buffer-string))) :fo
 (if (unwind-protect (setq value-40 (apply fn-38 args-39)) (setq form
 (let (form-description-42) (if (unwind-protect (setq value-40 (apply
 (let ((value-40 (quote ert-form-evaluation-aborted-41))) (let (form-
 (let* ((fn-38 (function string-match)) (args-39 (condition-case err
 (save-current-buffer (set-buffer buf) (auto-revert--wait-for-revert
 (while --dolist-tail-- (setq buf (car --dolist-tail--)) (save-curren
 (let ((--dolist-tail-- (list buf1 buf2)) buf) (while --dolist-tail--
 (progn (write-region "any text" nil tmpfile1 nil (quote no-message))
 (unwind-protect (progn (write-region "any text" nil tmpfile1 nil (qu
 (let* ((auto-revert--messages "") (g30 (function (lambda (msg) (setq
 (unwind-protect (let* ((auto-revert--messages "") (g30 (function (la
 (let* ((cp (executable-find "cp")) (tmpdir1 (make-temp-file "auto-re
 (lambda nil (let* ((fn-23 (function executable-find)) (args-24 (cond
 ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
 ert-run-test(#s(ert-test :name auto-revert-test01-auto-revert-severa
 ert-run-or-rerun-test(#s(ert--stats :selector (not (tag :unstable))
 ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
 ert-run-tests-batch((not (tag :unstable)))
 ert-run-tests-batch-and-exit((not (tag :unstable)))
 eval((ert-run-tests-batch-and-exit (quote (not (tag :unstable)))))
 command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/autorevert-tests.el"
 command-line()
 normal-top-level()
Test auto-revert-test01-auto-revert-several-files condition:
   (ert-test-failed
    ((should
      (string-match "another text"
                    (buffer-string)))
     :form
     (string-match "another text" "any text")
     :value nil))
  FAILED  2/5  auto-revert-test01-auto-revert-several-files
Reverting buffer `auto-revert-test49vRly'.
Reverting buffer `auto-revert-test49vRly'.
Reverting buffer `auto-revert-test49vRly'.
  passed  3/5  auto-revert-test02-auto-revert-deleted-file
Reverting buffer `auto-revert-testvVIyEY'.
  passed  4/5  auto-revert-test03-auto-revert-tail-mode
ls does not support --dired; see `dired-use-ls-dired' for more details.
Reverting buffer `T'.
Reverting buffer `T'.
  passed  5/5  auto-revert-test04-auto-revert-mode-dired

Ran 5 tests, 4 results as expected, 1 unexpected (2019-05-01 10:07:23+0800)

1 unexpected results:
  FAILED  auto-revert-test01-auto-revert-several-files

make[1]: *** [lisp/autorevert-tests.log] Error 1
make: *** [lisp/autorevert-tests] Error 2

Message #134 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Zhang Haijun <ccsmile2008 <at> outlook.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: "35418 <at> debbugs.gnu.org" <35418 <at> debbugs.gnu.org>
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Wed, 1 May 2019 02:59:47 +0000

I haven’t run the test before. It seems that directory watching doesn’t work for file content only change. But it works for many common use cases while file watching doesn’t work.

1. Use vim to modify the file and save. It seems that vim will write to a temp file, then remove the original file and then rename the temp file to original file name.
2. Use git to switch branch.
3. For files in a soft link directory.

So directory watching is much more usefull for me than file watching in everyday use. 



> 在 2019年5月1日，上午10:15，张海君 <ccsmile2008 <at> outlook.com> 写道：
> 
> 
>> I don't understand the patch. Symbol kqueue11 does not exist, so do you mean
>> 
>> +                  func dir
> Yes.
> 
>> 
>> And have you applied the tests in filenotify-tests.el? Do all of them pass?
>> 
>> Best regards, Michael.
> 
> $ make -C test autorevert-tests
> ELC      lisp/autorevert-tests.elc
> GEN      lisp/autorevert-tests.log
> Running 5 tests (2019-05-01 10:06:38+0800)
> Reverting buffer `auto-revert-testqe4qed'.
>  passed  1/5  auto-revert-test00-auto-revert-mode
> (Shell command succeeded with no output)
> Test auto-revert-test01-auto-revert-several-files backtrace:
> signal(ert-test-failed (((should (string-match "another text" (buffe
> ert-fail(((should (string-match "another text" (buffer-string))) :fo
> (if (unwind-protect (setq value-40 (apply fn-38 args-39)) (setq form
> (let (form-description-42) (if (unwind-protect (setq value-40 (apply
> (let ((value-40 (quote ert-form-evaluation-aborted-41))) (let (form-
> (let* ((fn-38 (function string-match)) (args-39 (condition-case err
> (save-current-buffer (set-buffer buf) (auto-revert--wait-for-revert
> (while --dolist-tail-- (setq buf (car --dolist-tail--)) (save-curren
> (let ((--dolist-tail-- (list buf1 buf2)) buf) (while --dolist-tail--
> (progn (write-region "any text" nil tmpfile1 nil (quote no-message))
> (unwind-protect (progn (write-region "any text" nil tmpfile1 nil (qu
> (let* ((auto-revert--messages "") (g30 (function (lambda (msg) (setq
> (unwind-protect (let* ((auto-revert--messages "") (g30 (function (la
> (let* ((cp (executable-find "cp")) (tmpdir1 (make-temp-file "auto-re
> (lambda nil (let* ((fn-23 (function executable-find)) (args-24 (cond
> ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
> ert-run-test(#s(ert-test :name auto-revert-test01-auto-revert-severa
> ert-run-or-rerun-test(#s(ert--stats :selector (not (tag :unstable))
> ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
> ert-run-tests-batch((not (tag :unstable)))
> ert-run-tests-batch-and-exit((not (tag :unstable)))
> eval((ert-run-tests-batch-and-exit (quote (not (tag :unstable)))))
> command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/autorevert-tests.el"
> command-line()
> normal-top-level()
> Test auto-revert-test01-auto-revert-several-files condition:
>   (ert-test-failed
>    ((should
>      (string-match "another text"
>                    (buffer-string)))
>     :form
>     (string-match "another text" "any text")
>     :value nil))
>  FAILED  2/5  auto-revert-test01-auto-revert-several-files
> Reverting buffer `auto-revert-test49vRly'.
> Reverting buffer `auto-revert-test49vRly'.
> Reverting buffer `auto-revert-test49vRly'.
>  passed  3/5  auto-revert-test02-auto-revert-deleted-file
> Reverting buffer `auto-revert-testvVIyEY'.
>  passed  4/5  auto-revert-test03-auto-revert-tail-mode
> ls does not support --dired; see `dired-use-ls-dired' for more details.
> Reverting buffer `T'.
> Reverting buffer `T'.
>  passed  5/5  auto-revert-test04-auto-revert-mode-dired
> 
> Ran 5 tests, 4 results as expected, 1 unexpected (2019-05-01 10:07:23+0800)
> 
> 1 unexpected results:
>  FAILED  auto-revert-test01-auto-revert-several-files
> 
> make[1]: *** [lisp/autorevert-tests.log] Error 1
> make: *** [lisp/autorevert-tests] Error 2
> 

Message #137 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Zhang Haijun <ccsmile2008 <at> outlook.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: "35418 <at> debbugs.gnu.org" <35418 <at> debbugs.gnu.org>
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Wed, 1 May 2019 03:10:20 +0000

If both file watching and directory watching are used for one file, it will works for all use cases.

> 在 2019年5月1日，上午10:59，张海君 <ccsmile2008 <at> outlook.com> 写道：
> 
> I haven’t run the test before. It seems that directory watching doesn’t work for file content only change. But it works for many common use cases while file watching doesn’t work.
> 
> 1. Use vim to modify the file and save. It seems that vim will write to a temp file, then remove the original file and then rename the temp file to original file name.
> 2. Use git to switch branch.
> 3. For files in a soft link directory.
> 
> So directory watching is much more usefull for me than file watching in everyday use. 
> 
> 
> 
>> 在 2019年5月1日，上午10:15，张海君 <ccsmile2008 <at> outlook.com> 写道：
>> 
>> 
>>> I don't understand the patch. Symbol kqueue11 does not exist, so do you mean
>>> 
>>> +                  func dir
>> Yes.
>> 
>>> 
>>> And have you applied the tests in filenotify-tests.el? Do all of them pass?
>>> 
>>> Best regards, Michael.
>> 
>> $ make -C test autorevert-tests
>> ELC      lisp/autorevert-tests.elc
>> GEN      lisp/autorevert-tests.log
>> Running 5 tests (2019-05-01 10:06:38+0800)
>> Reverting buffer `auto-revert-testqe4qed'.
>> passed  1/5  auto-revert-test00-auto-revert-mode
>> (Shell command succeeded with no output)
>> Test auto-revert-test01-auto-revert-several-files backtrace:
>> signal(ert-test-failed (((should (string-match "another text" (buffe
>> ert-fail(((should (string-match "another text" (buffer-string))) :fo
>> (if (unwind-protect (setq value-40 (apply fn-38 args-39)) (setq form
>> (let (form-description-42) (if (unwind-protect (setq value-40 (apply
>> (let ((value-40 (quote ert-form-evaluation-aborted-41))) (let (form-
>> (let* ((fn-38 (function string-match)) (args-39 (condition-case err
>> (save-current-buffer (set-buffer buf) (auto-revert--wait-for-revert
>> (while --dolist-tail-- (setq buf (car --dolist-tail--)) (save-curren
>> (let ((--dolist-tail-- (list buf1 buf2)) buf) (while --dolist-tail--
>> (progn (write-region "any text" nil tmpfile1 nil (quote no-message))
>> (unwind-protect (progn (write-region "any text" nil tmpfile1 nil (qu
>> (let* ((auto-revert--messages "") (g30 (function (lambda (msg) (setq
>> (unwind-protect (let* ((auto-revert--messages "") (g30 (function (la
>> (let* ((cp (executable-find "cp")) (tmpdir1 (make-temp-file "auto-re
>> (lambda nil (let* ((fn-23 (function executable-find)) (args-24 (cond
>> ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
>> ert-run-test(#s(ert-test :name auto-revert-test01-auto-revert-severa
>> ert-run-or-rerun-test(#s(ert--stats :selector (not (tag :unstable))
>> ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
>> ert-run-tests-batch((not (tag :unstable)))
>> ert-run-tests-batch-and-exit((not (tag :unstable)))
>> eval((ert-run-tests-batch-and-exit (quote (not (tag :unstable)))))
>> command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/autorevert-tests.el"
>> command-line()
>> normal-top-level()
>> Test auto-revert-test01-auto-revert-several-files condition:
>>  (ert-test-failed
>>   ((should
>>     (string-match "another text"
>>                   (buffer-string)))
>>    :form
>>    (string-match "another text" "any text")
>>    :value nil))
>> FAILED  2/5  auto-revert-test01-auto-revert-several-files
>> Reverting buffer `auto-revert-test49vRly'.
>> Reverting buffer `auto-revert-test49vRly'.
>> Reverting buffer `auto-revert-test49vRly'.
>> passed  3/5  auto-revert-test02-auto-revert-deleted-file
>> Reverting buffer `auto-revert-testvVIyEY'.
>> passed  4/5  auto-revert-test03-auto-revert-tail-mode
>> ls does not support --dired; see `dired-use-ls-dired' for more details.
>> Reverting buffer `T'.
>> Reverting buffer `T'.
>> passed  5/5  auto-revert-test04-auto-revert-mode-dired
>> 
>> Ran 5 tests, 4 results as expected, 1 unexpected (2019-05-01 10:07:23+0800)
>> 
>> 1 unexpected results:
>> FAILED  auto-revert-test01-auto-revert-several-files
>> 
>> make[1]: *** [lisp/autorevert-tests.log] Error 1
>> make: *** [lisp/autorevert-tests] Error 2
>> 
> 

Message #140 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Wed, 01 May 2019 20:45:21 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Cc: michael.albinus <at> gmx.de, 35418 <at> debbugs.gnu.org
> Date: Tue, 30 Apr 2019 23:09:08 +0200
> 
> Here is a patch that does roughly that. I'm not entirely happy with the
> old 'Auto-reverting non-buffer files' section, whose node name was just
> 'Autorevert'. I would have preferred it as a subsection to the new
> auto-revert section, along with its existing two subsections, but since
> its place isn't the same in the on-line and printed manuals, that
> seemed technically tricky without duplicating a lot of text.

Didn't yet review the patch, but I don't understand the difficulty
with moving 'Auto-reverting non-buffer files' into Auto-revert.  Can
you explain what gets in the way?

Message #143 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Wed, 01 May 2019 21:41:38 +0200

[Message part 1 (text/plain, inline)]

ons 2019-05-01 klockan 20:45 +0300 skrev Eli Zaretskii:
> 
> Didn't yet review the patch, but I don't understand the difficulty
> with moving 'Auto-reverting non-buffer files' into Auto-revert.  Can
> you explain what gets in the way?

After applying the patch, the on-line manual would have the nodes

* Reverting (about reverting)
* Auto-revert (about auto-revert)
* Non-file buffers (about auto-reverting non-file buffers)
** Auto-reverting the buffer menu
** Auto-reverting Dired

but I'd rather have

* Reverting (about reverting)
* Auto-revert (about auto-revert, including non-buffer files)
** Auto-reverting the buffer menu
** Auto-reverting Dired

except that in the printed manual, the non-buffer part is a section of
its own. The attached patch hacks around it by removing @node and
@section from arevert-xtra.texi; perhaps it can be stomached.

[0001-Reorganise-auto-revert-nodes-in-the-manual.patch (text/x-patch, attachment)]

Message #146 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

In general it looks OK. Just some few comments:

> --- a/doc/emacs/emacs.texi
> +++ b/doc/emacs/emacs.texi
> @@ -443,9 +443,7 @@ Top
>  * Visiting::            Visiting a file prepares Emacs to edit the file.
>  * Saving::              Saving makes your changes permanent.
>  * Reverting::           Reverting cancels all the changes not saved.
> -@ifnottex
> -* Autorevert::          Auto Reverting non-file buffers.
> -@end ifnottex
> +* Auto-revert::         Keeping buffers automatically up-to-date.
>  * Auto Save::           Auto Save periodically protects against loss of data.

Please call the node "Auto Revert", like the following "Auto Save".

> +date, you can enable Auto-revert mode by typing @kbd{M-x auto-revert-mode}.

This shall be "Auto Revert mode" (or "Auto Revert Mode", don't know).

> +the end, use Auto-Revert Tail mode instead

dito, "Auto Revert Tail mode".

I know that it was called already like this. But it looks more
consistent to me, when changing it.

Best regards, Michael.

Message #149 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Zhang Haijun <ccsmile2008 <at> outlook.com>
Cc: "35418 <at> debbugs.gnu.org" <35418 <at> debbugs.gnu.org>
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Thu, 02 May 2019 14:24:55 +0200

Zhang Haijun <ccsmile2008 <at> outlook.com> writes:

>> I don't understand the patch. Symbol kqueue11 does not exist, so do you mean
>>
>> +                  func dir
> Yes.
>
>>
>> And have you applied the tests in filenotify-tests.el? Do all of them pass?
>>
>> Best regards, Michael.
>
> $ make -C test autorevert-tests
>  ELC      lisp/autorevert-tests.elc
>  GEN      lisp/autorevert-tests.log
> Running 5 tests (2019-05-01 10:06:38+0800)
> Reverting buffer `auto-revert-testqe4qed'.
>   passed  1/5  auto-revert-test00-auto-revert-mode
> (Shell command succeeded with no output)
> Test auto-revert-test01-auto-revert-several-files backtrace:
>  signal(ert-test-failed (((should (string-match "another text" (buffe

So this shows you cannot change filenotify.el as proposed.

Best regards, Michael.

Message #152 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Zhang Haijun <ccsmile2008 <at> outlook.com>
Cc: "35418 <at> debbugs.gnu.org" <35418 <at> debbugs.gnu.org>
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Thu, 02 May 2019 14:28:18 +0200

Zhang Haijun <ccsmile2008 <at> outlook.com> writes:

> I haven’t run the test before.

autorevert-tests run successfuly on my FreeBSD 10.2 system, with
unmodified Emacs sources from master.

> It seems that directory watching
> doesn’t work for file content only change. But it works for many
> common use cases while file watching doesn’t work.

We apply directory watching wherever possible. kqueue has its
restrictions here.


Best regards, Michael.

Message #155 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Zhang Haijun <ccsmile2008 <at> outlook.com>
Cc: "35418 <at> debbugs.gnu.org" <35418 <at> debbugs.gnu.org>
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Thu, 02 May 2019 14:30:10 +0200

Zhang Haijun <ccsmile2008 <at> outlook.com> writes:

> If both file watching and directory watching are used for one file, it
> will works for all use cases.

This doesn't fit into filenotify.el. Which watch descriptor do you
expect from `file-notify-add-watch'?

Best regards, Michael.

Message #158 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

tor 2019-05-02 klockan 14:18 +0200 skrev Michael Albinus:
> Mattias Engdegård <mattiase <at> acm.org> writes:
> 
> Please call the node "Auto Revert", like the following "Auto Save".

Done.

> > +date, you can enable Auto-revert mode by typing @kbd{M-x auto-
> > revert-mode}.
> 
> This shall be "Auto Revert mode" (or "Auto Revert Mode", don't know).

Done (I went with the former).

> > +the end, use Auto-Revert Tail mode instead
> 
> dito, "Auto Revert Tail mode".

Done.

> I know that it was called already like this. But it looks more
> consistent to me, when changing it.

Of course. New patch attached.

[0001-Reorganise-auto-revert-nodes-in-the-manual.patch (text/x-patch, attachment)]

Message #161 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

> Of course. New patch attached.

Thanks, LGTM.

Best regards, Michael.

Message #164 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Zhang Haijun <ccsmile2008 <at> outlook.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: "35418 <at> debbugs.gnu.org" <35418 <at> debbugs.gnu.org>
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Thu, 2 May 2019 13:24:53 +0000

It seems that file watching of kqueue is useless for me. The directory watching satisfies almost all of my needs. So I will continue to use my modified version.

> 在 2019年5月2日，下午8:30，Michael Albinus <michael.albinus <at> gmx.de> 写道：
> 
> Zhang Haijun <ccsmile2008 <at> outlook.com> writes:
> 
>> If both file watching and directory watching are used for one file, it
>> will works for all use cases.
> 
> This doesn't fit into filenotify.el. Which watch descriptor do you
> expect from `file-notify-add-watch'?
> 
> Best regards, Michael.

Message #167 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

2 maj 2019 kl. 15.02 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
> Thanks, LGTM.

Thank you. I'll give Eli a chance to comment before committing.
(I'm in no hurry; take your time.)

Message #170 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Fri, 03 May 2019 16:44:52 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 35418 <at> debbugs.gnu.org
> Date: Thu, 02 May 2019 14:53:20 +0200
> 
> * doc/emacs/files.texi:
> * doc/emacs/arevert-xtra.texi:
> * doc/emacs/buffers.texi:
> * doc/emacs/emacs.texi:
> * doc/emacs/emacs-xtra.texi:
> Add node 'Auto-revert' and move general information on that topic there.
> Sort paragraphs in that node in a rough least-to-most specific order.
> Include the old 'Autorevert' node into that node when building the
> on-line manual.

It is better to cite the node names in the log, unless doing that is
completely impractical.

> diff --git a/doc/emacs/arevert-xtra.texi b/doc/emacs/arevert-xtra.texi
> index cd7c1ff895..8cc5b053b5 100644
> --- a/doc/emacs/arevert-xtra.texi
> +++ b/doc/emacs/arevert-xtra.texi
> @@ -4,8 +4,7 @@
>  @c
>  @c This file is included either in emacs-xtra.texi (when producing the
>  @c printed version) or in the main Emacs manual (for the on-line version).
> -@node Autorevert
> -@section Auto Reverting Non-File Buffers
> +@c The including file must provide its own @node and @section lines.

So maybe we shouldn't remove the @node here?  How about making it a
subsection of "Auto Revert" instead?

> +@vindex auto-revert-remote-files
> +  These minor modes do not check or revert remote files, because that is

Which "these minor modes"?  Such wording is only appropriate when it
closely follows a list of modes, which is not the case here.  I think
it's better to enumerate the modes explicitly here.

> +usually too slow.  This behavior can be changed by setting the
> +variable @code{auto-revert-remote-files} to non-@code{nil}.
>  
>  @cindex file notifications
>  @vindex auto-revert-use-notify
> -  By default, Auto-Revert mode works using @dfn{file notifications},
> +@vindex auto-revert-interval
> +  By default, Auto Revert mode works using @dfn{file notifications},
>  whereby changes in the filesystem are reported to Emacs by the OS.
>  You can disable use of file notifications by customizing the variable
>  @code{auto-revert-use-notify} to a @code{nil} value, then Emacs will
> @@ -990,7 +1010,7 @@ Reverting
>  
>  @vindex auto-revert-avoid-polling
>  @vindex auto-revert-notify-exclude-dir-regexp
> -  By default, Auto-Revert mode will poll files for changes
> +  By default, Auto Revert mode will poll files for changes
>  periodically even when file notifications are used.  Such polling is
>  usually unnecessary, and turning it off may save power by relying on
   ^^^^^^^^^^^^^^^^^^^
I would say "unnecessary in many cases".  "usually" begs the question
why by default we do poll.  Bonus points for adding some hint about
what rare situations do need such polling, as I think this description
sounds like a small riddle without that, and doesn't allow people to
make an educated decision regarding whether they do or don't want the
polling turned off.

Thanks.

Message #173 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Fri, 03 May 2019 16:47:51 +0200

[Message part 1 (text/plain, inline)]

fre 2019-05-03 klockan 16:44 +0300 skrev Eli Zaretskii:
> > 
> It is better to cite the node names in the log, unless doing that is
> completely impractical.

Done. I found it a bit impractical in this case, but did so anyway.

> > -@node Autorevert
> > -@section Auto Reverting Non-File Buffers
> > +@c The including file must provide its own @node and @section
> > lines.
> 
> So maybe we shouldn't remove the @node here?  How about making it a
> subsection of "Auto Revert" instead?

The text in arevert-xtra has two subsections already. Then we would
have three subsections, where the first acts as a sort of prelude to
the two others. That might work for the on-line manual, but how would
it fit into emacs-xtra? What would the section be then?

> > +@vindex auto-revert-remote-files
> > +  These minor modes do not check or revert remote files, because
> > that is
> 
> Which "these minor modes"?  Such wording is only appropriate when it
> closely follows a list of modes, which is not the case here.  I think
> it's better to enumerate the modes explicitly here.

Replaced with 'The Auto Revert modes'; this should be readily
understood.

> >  periodically even when file notifications are used.  Such polling
> > is
> >  usually unnecessary, and turning it off may save power by relying
> > on
>    ^^^^^^^^^^^^^^^^^^^
> I would say "unnecessary in many cases".  "usually" begs the question
> why by default we do poll.  Bonus points for adding some hint about
> what rare situations do need such polling, as I think this
> description
> sounds like a small riddle without that, and doesn't allow people to
> make an educated decision regarding whether they do or don't want the
> polling turned off.

Done, but the hint you are asking for does come right after:

  [...] However,
  notification is ineffective on certain file systems; mainly network
  file system on Unix-like machines, where files can be altered from
  other machines.

which is the most important case that I'm aware of. (According to
Michael, Tramp notifications can be unreliable sometimes, but they have
to be enabled actively.)

Thanks for the review! Revised patch attached.

[0001-Reorganise-auto-revert-nodes-in-the-manual.patch (text/x-patch, attachment)]

Message #176 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Sat, 04 May 2019 12:04:28 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Cc: michael.albinus <at> gmx.de, 35418 <at> debbugs.gnu.org
> Date: Fri, 03 May 2019 16:47:51 +0200
> 
> > It is better to cite the node names in the log, unless doing that is
> > completely impractical.
> 
> Done. I found it a bit impractical in this case, but did so anyway.

Thanks, but why did you start the description of the change on a new
line?  Here:

> * doc/emacs/files.texi
> (Files): Adjust menu.
> (Reverting, Auto Revert):
> Add node `Auto Revert' and move general information on that topic there
> from `Reverting'.
> Sort paragraphs in `Auto Revert' in a rough least-to-most specific order.
> Include the old `Autorevert' text and subsections into that node when
> building the on-line manual.
> (Autorevert): Remove.

This should be formatted like this (indented 2 spaces for clarity):

  * doc/emacs/files.texi (Files): Adjust menu.
  (Reverting, Auto Revert): Add node `Auto Revert' and move general
  information on that topic there from `Reverting'.  Sort paragraphs
  in `Auto Revert' in a rough least-to-most specific order.  Include
  the old `Autorevert' text and subsections into that node when
  building the on-line manual.
  (Autorevert): Remove.

IOW, only when you describe changes to another node you should start
on a new line.  The above is the formatting produced by "C-x 4 a" and
its ilk.

> > > -@node Autorevert
> > > -@section Auto Reverting Non-File Buffers
> > > +@c The including file must provide its own @node and @section
> > > lines.
> > 
> > So maybe we shouldn't remove the @node here?  How about making it a
> > subsection of "Auto Revert" instead?
> 
> The text in arevert-xtra has two subsections already. Then we would
> have three subsections, where the first acts as a sort of prelude to
> the two others. That might work for the on-line manual, but how would
> it fit into emacs-xtra? What would the section be then?

No, I meant make "Autorevert" (under its new name) a subsection of
"Auto Revert", and the 2 subsections of "Autorevert" sub-subsections.
Or did I misunderstand the problem you were describing?

> > >  periodically even when file notifications are used.  Such polling
> > > is
> > >  usually unnecessary, and turning it off may save power by relying
> > > on
> >    ^^^^^^^^^^^^^^^^^^^
> > I would say "unnecessary in many cases".  "usually" begs the question
> > why by default we do poll.  Bonus points for adding some hint about
> > what rare situations do need such polling, as I think this
> > description
> > sounds like a small riddle without that, and doesn't allow people to
> > make an educated decision regarding whether they do or don't want the
> > polling turned off.
> 
> Done, but the hint you are asking for does come right after:
> 
>   [...] However,
>   notification is ineffective on certain file systems; mainly network
>   file system on Unix-like machines, where files can be altered from
>   other machines.

Yes, but that hint doesn't mention the polling.  I think it's
important to tell that polling is needed in these rare cases.

Thanks.

Message #179 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

lör 2019-05-04 klockan 12:04 +0300 skrev Eli Zaretskii:
> 
> IOW, only when you describe changes to another node you should start
> on a new line.  The above is the formatting produced by "C-x 4 a" and
> its ilk.

Reformatted.

> No, I meant make "Autorevert" (under its new name) a subsection of
> "Auto Revert", and the 2 subsections of "Autorevert" sub-subsections.
> Or did I misunderstand the problem you were describing?

Done. It seems that a subsection can be directly under a top node
without a section as intermediate step; I didn't know that.

> Yes, but that hint doesn't mention the polling.  I think it's
> important to tell that polling is needed in these rare cases.

Text added.

Thanks again; revised patch attached.

> 

[0001-Reorganise-auto-revert-nodes-in-the-manual.patch (text/x-patch, attachment)]

Message #182 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Sat, 04 May 2019 16:41:09 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Cc: michael.albinus <at> gmx.de, 35418 <at> debbugs.gnu.org
> Date: Sat, 04 May 2019 13:21:19 +0200
> 
> Thanks again; revised patch attached.

LGTM, thanks.

Message #185 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

> +  By default, Auto Revert mode will poll files for changes
> +periodically even when file notifications are used.  Polling is
> +unnecessary in many cases, and turning it off may save power by
> +relying on notifications only.  To do so, set the variable
>  @code{auto-revert-avoid-polling} to non-@code{nil}.  However,
>  notification is ineffective on certain file systems; mainly network
>  file system on Unix-like machines, where files can be altered from
> +other machines.  For such file systems, polling may be necessary.

Maybe we could also add the other major reason why polling is necessary:
file notification is not supported for all types of remote file systems
(Tramp).

Best regards, Michael.

Message #188 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: mattiase <at> acm.org, 35418 <at> debbugs.gnu.org
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Sat, 04 May 2019 20:08:38 +0300

> From: Michael Albinus <michael.albinus <at> gmx.de>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  35418 <at> debbugs.gnu.org
> Date: Sat, 04 May 2019 18:53:07 +0200
> 
> Maybe we could also add the other major reason why polling is necessary:
> file notification is not supported for all types of remote file systems
> (Tramp).

Yes, I think all the reasons should be mentioned, so that users who
disable polling know what they are/will be losing, and could make
educated decisions.

Message #191 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

4 maj 2019 kl. 18.53 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
> Maybe we could also add the other major reason why polling is necessary:
> file notification is not supported for all types of remote file systems
> (Tramp).

Is the problem that notification over Tramp may fail to activate or suddenly fail functioning, in both cases without any indication of error to file-notify?

If so, we should take care of this in code instead. Assuming the above, I suggest that file-notify consider all notification from file name handlers to be unreliable, and provide an interface to auto-revert, which will then keep polling those files.

Message #194 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

>> Maybe we could also add the other major reason why polling is necessary:
>> file notification is not supported for all types of remote file systems
>> (Tramp).
>
> Is the problem that notification over Tramp may fail to activate or
> suddenly fail functioning, in both cases without any indication of
> error to file-notify?

Yes we have both cases. But file-notify is told about.

The first case is that file notification cannot be activated. Several
Tramp methods do not support this, and they return nil for the
respective file-notify-add-watch call, as specified. auto-revert
understands this, and knows that the respective file must be handled by
polling.

The second case is, that file notification for a remote file ceases to
work. Since Tramp implements file notification as asynchronous
processes, this could happen if the respective process is killed on the
remote side, or if the connection between the local Emacs and the remote
host is broken temporarily. The respective process shall own a sentinel,
which sends a "stopped" event in this case. I've just checked the code;
this is not implemented. Will do.

> If so, we should take care of this in code instead. Assuming the
> above, I suggest that file-notify consider all notification from file
> name handlers to be unreliable, and provide an interface to
> auto-revert, which will then keep polling those files.

We have this interface already. For file-notify-add-watch it works
already as expected. For broken notifications, the interface is the
"stopped" event, which must (will be) implemented in Tramp.

Btw, I believe all this file notification vs polling behavior is not
covered yet in autorevert-tests.el. Would you like to add respective
test cases for local and remote files? Some days ago I've added tests
for remote files; you could use this mechanism as well.

Best regards, Michael.

Message #197 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Michael Albinus <michael.albinus <at> gmx.de> writes:

Hi Mattias,

> The second case is, that file notification for a remote file ceases to
> work. Since Tramp implements file notification as asynchronous
> processes, this could happen if the respective process is killed on the
> remote side, or if the connection between the local Emacs and the remote
> host is broken temporarily. The respective process shall own a sentinel,
> which sends a "stopped" event in this case. I've just checked the code;
> this is not implemented. Will do.

Implemented, pushed to master.

Best regards, Michael.

Message #200 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

4 maj 2019 kl. 22.31 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
>> The second case is, that file notification for a remote file ceases to
>> work. Since Tramp implements file notification as asynchronous
>> processes, this could happen if the respective process is killed on the
>> remote side, or if the connection between the local Emacs and the remote
>> host is broken temporarily. The respective process shall own a sentinel,
>> which sends a "stopped" event in this case. I've just checked the code;
>> this is not implemented. Will do.
> 
> Implemented, pushed to master.

Nice, thank you! Does this mean that we can trust Tramp sufficiently in this respect? That is, notifiers will either fail to be created, fail with a 'stopped' event, or work.

Message #203 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

> Nice, thank you! Does this mean that we can trust Tramp sufficiently
> in this respect? That is, notifiers will either fail to be created,
> fail with a 'stopped' event, or work.

Yes, I think so. Until the next bug report :-)

Best regards, Michael.

Message #206 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

5 maj 2019 kl. 10.22 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
>> Nice, thank you! Does this mean that we can trust Tramp sufficiently
>> in this respect? That is, notifiers will either fail to be created,
>> fail with a 'stopped' event, or work.
> 
> Yes, I think so. Until the next bug report :-)

Good. I've pushed the manual update.

What remains is avoiding polling in global-auto-revert-mode. I'll send a patch soon.

Message #209 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

Here is a detail that needs to be taken care of: we should not use notification on non-file buffers, since that is generally useless (consider the Buffer List buffer) and, with `auto-revert-avoid-polling', prevents these buffers from being polled and thus updated at all.

Patch attached. There is a hack for Dired buffers, since watching their directories happens to be just what we want. It would be nice to generalise this condition somehow, but meanwhile this will have to do, unless you can come up with something better.

A patch for global-auto-revert-mode will come shortly.

[0001-Don-t-use-file-notification-on-non-file-buffers.patch (application/octet-stream, attachment)]

Message #212 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Wed, 08 May 2019 11:47:39 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Wed, 8 May 2019 10:34:13 +0200
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 35418 <at> debbugs.gnu.org
> 
> +                ;; Don't bother creating a notifier for non-file buffers
> +                ;; if there is a custom `revert-buffer-function'.
> +                ;; An exception is made for Dired, since that mode works
> +                ;; well with notifiers.
>                  (when (and auto-revert-use-notify
> -                           (not auto-revert-notify-watch-descriptor))
> +                           (not auto-revert-notify-watch-descriptor)
> +                           (or buffer-file-name
> +                               (eq major-mode 'dired-mode)))

Is Dired the only exception from the rule?  Or is there a more general
indication that a non-file buffer may want to be automatically
reverted?  E.g., what about Info buffers? revert-buffer does work
there.

Thanks.

Message #215 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

8 maj 2019 kl. 10.47 skrev Eli Zaretskii <eliz <at> gnu.org>:

> Is Dired the only exception from the rule?  Or is there a more general
> indication that a non-file buffer may want to be automatically
> reverted?  E.g., what about Info buffers? revert-buffer does work
> there.

It's not about whether revert-buffer works, but whether notification on default-directory is a reliable replacement for polling.

Info actually isn't a good example, since it doesn't even work with polling: it has no special `buffer-stale-function', and therefore isn't able to tell when the buffer is out of date. Furthermore, notification on a directory may not indicate changes to any of the files in it and thus wouldn't be reliable anyway.

With the patch, auto-revert on non-file buffers will work where at all possible; it just won't use notification for buffers other than Dired.

Message #218 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

Actually, the comment added in the last patch was inaccurate and referred to a previous attempt.
Revised patch attached (the actual condition is unchanged).

[0001-Don-t-use-file-notification-on-non-file-buffers.patch (application/octet-stream, attachment)]

[Message part 3 (text/plain, inline)]

Message #221 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Wed, 08 May 2019 13:58:46 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Wed, 8 May 2019 12:18:30 +0200
> Cc: Michael Albinus <michael.albinus <at> gmx.de>, 35418 <at> debbugs.gnu.org
> 
> With the patch, auto-revert on non-file buffers will work where at all possible; it just won't use notification for buffers other than Dired.

I asked whether Dired is the only exception, or is there some more
general sign that a buffer could use notifications.  I'm still not
sure what the answer is to that question.  I don't have any doubt that
Dired will work with notifications, but is the only way to have
exceptions is by naming their major modes explicitly?

IOW, I wonder whether your proposed patch could be made more general
in some way.

Thanks.

Message #224 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

8 maj 2019 kl. 12.58 skrev Eli Zaretskii <eliz <at> gnu.org>:

> IOW, I wonder whether your proposed patch could be made more general
> in some way.

So do I, but since I could not come up with one, this ad-hoc solution appeared as a placeholder.
The code can be improved later on, but the patch is a strict improvement on the code in master.

An example of a generalisation: We could add a buffer-specific variable that tells autorevert that yes, this buffer can rely on directory notifications despite not having a buffer-file-name. All modes to which this applies would need to set that variable.

I don't think there is a passive condition, and furthermore, because of the nature of directory notifications, any mode that qualifies is likely to be some kind of variation on Dired: a buffer whose contents is determined by the set of files in its default-directory, but not their data.

Message #227 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Wed, 08 May 2019 15:35:14 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Wed, 8 May 2019 13:48:51 +0200
> Cc: Michael Albinus <michael.albinus <at> gmx.de>, 35418 <at> debbugs.gnu.org
> 
> the patch is a strict improvement on the code in master.

I have no doubt it is.

> An example of a generalisation: We could add a buffer-specific variable that tells autorevert that yes, this buffer can rely on directory notifications despite not having a buffer-file-name. All modes to which this applies would need to set that variable.
> 
> I don't think there is a passive condition, and furthermore, because of the nature of directory notifications, any mode that qualifies is likely to be some kind of variation on Dired: a buffer whose contents is determined by the set of files in its default-directory, but not their data.

OK, so how about adding such a variable as part of this improvement?

Or maybe it's better to have a special property on the mode's symbol?

Message #230 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

8 maj 2019 kl. 14.35 skrev Eli Zaretskii <eliz <at> gnu.org>:
> 
>> An example of a generalisation: We could add a buffer-specific variable that tells autorevert that yes, this buffer can rely on directory notifications despite not having a buffer-file-name. All modes to which this applies would need to set that variable.
>> 
>> I don't think there is a passive condition, and furthermore, because of the nature of directory notifications, any mode that qualifies is likely to be some kind of variation on Dired: a buffer whose contents is determined by the set of files in its default-directory, but not their data.
> 
> OK, so how about adding such a variable as part of this improvement?

If it's all the same to you, I'd like to keep it separate. This patch fixes a bug: some buffers, like Buffer Menu, are not updated automatically at all when `auto-revert-avoid-polling' is set.
What you are talking about is making some dired-like buffers update 2.5 s faster on average, if they set the right variable.

> Or maybe it's better to have a special property on the mode's symbol?

Maybe a variable goes better with `revert-buffer-function' and `buffer-stale-function' already in use by autorevert for related purposes, although I'm eager to hear what Michael has to say (about this and everything else).

Message #233 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

> I'm eager to hear what Michael has to say (about
> this and everything else).

I'm very busy @work just now; will react when possible. Latest on
weekend, hopefully earlier. Sorry.

Best regards, Michael.

Message #236 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Wed, 08 May 2019 16:28:41 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Wed, 8 May 2019 14:58:47 +0200
> Cc: Michael Albinus <michael.albinus <at> gmx.de>, 35418 <at> debbugs.gnu.org
> 
> > OK, so how about adding such a variable as part of this improvement?
> 
> If it's all the same to you, I'd like to keep it separate.

I don't understand why: the change as it is looks a bit unclean to me,
whereas the effort to make it cleaner and more future-proof is not a
substantial one.

But if you insist, I won't argue.

> What you are talking about is making some dired-like buffers update 2.5 s faster on average, if they set the right variable.
> 
> > Or maybe it's better to have a special property on the mode's symbol?
> 
> Maybe a variable goes better with `revert-buffer-function' and `buffer-stale-function' already in use by autorevert for related purposes

Maybe.  Those variables are used to name a function, whereas here we
are talking about a variable that will serve just as a flag.  So I
think this is different, though I don't have strong feelings.

Message #239 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

8 maj 2019 kl. 15.28 skrev Eli Zaretskii <eliz <at> gnu.org>:
>> 
>>> OK, so how about adding such a variable as part of this improvement?
>> 
>> If it's all the same to you, I'd like to keep it separate.
> 
> I don't understand why: the change as it is looks a bit unclean to me,
> whereas the effort to make it cleaner and more future-proof is not a
> substantial one.

How about dropping the Dired special case for the time being? Then the immediate bug is fixed, and all we need to worry about is the 2.5 second delay to Dired auto-revert updates, which we can fix separately in a manner of our choosing.

Simplified patch attached.

[0001-Don-t-use-file-notification-on-non-file-buffers.patch (application/octet-stream, attachment)]

Message #242 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Wed, 08 May 2019 20:24:59 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Wed, 8 May 2019 16:13:37 +0200
> Cc: Michael Albinus <michael.albinus <at> gmx.de>, 35418 <at> debbugs.gnu.org
> 
> >> If it's all the same to you, I'd like to keep it separate.
> > 
> > I don't understand why: the change as it is looks a bit unclean to me,
> > whereas the effort to make it cleaner and more future-proof is not a
> > substantial one.
> 
> How about dropping the Dired special case for the time being?

That's too drastic, IMO.  Feel free to push the original patch.

Thanks.

Message #245 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Wed, 08 May 2019 20:17:25 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

>> How about dropping the Dired special case for the time being?
>
> That's too drastic, IMO.  Feel free to push the original patch.

I agree with Eli, we would loose too much. A directory might not change
for hours, polling for this every 5 seconds is expensive. Think about a
remote directory, for example.

> Thanks.

Best regards, Michael.

PS: I hope to find tomorrow the time to review the patch.

Message #248 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

5 maj 2019 kl. 11.58 skrev Mattias Engdegård <mattiase <at> acm.org>:
> 
> What remains is avoiding polling in global-auto-revert-mode. I'll send a patch soon.

Here is that patch.

I understand that some people are queasy about using advice in code like this, and am open to suggestions about alternatives.
What the code needs is a reasonable (not necessarily bullet-proof) way to detect new file buffers and changes to buffer-file-name of those buffers. Monitoring `find-file-noselect' and `set-visited-file-name' turned out to be good enough.
For the former, it might be possible to get away with `after-change-major-mode-hook' instead (already used for non-file buffers).

[0001-Avoid-polling-in-global-auto-revert-mode.patch (application/octet-stream, attachment)]

Message #251 received at 35418 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 35418 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Thu, 09 May 2019 13:48:44 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Thu, 9 May 2019 12:00:17 +0200
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 35418 <at> debbugs.gnu.org
> 
> What the code needs is a reasonable (not necessarily bullet-proof) way to detect new file buffers and changes to buffer-file-name of those buffers.

Did you consider utilizing buffer-list-update-hook?

Message #254 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

9 maj 2019 kl. 12.48 skrev Eli Zaretskii <eliz <at> gnu.org>:
> 
> Did you consider utilizing buffer-list-update-hook?

It triggers far too often, including changes in the list order, creation and removal of temporary buffers, and so on. Furthermore it does not say what changed; a diff would have to be computed each time. Finally, neither the major mode nor buffer-file-name has been set at that point.

Message #257 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi,

> An example of a generalisation: We could add a buffer-specific
> variable that tells autorevert that yes, this buffer can rely on
> directory notifications despite not having a buffer-file-name. All
> modes to which this applies would need to set that variable.

I sympathize with this proposal. There shall be an indication that a
buffer could be auto-reverted by file notifications. This indication is
either a non-nil buffer-file-name, or a non-nil buffer-local variable
(let's call it buffer-auto-revert-by-file-notification-aware; I'm open
to any better name).

Non-file buffers which could be auto-reverted are those which provide a
buffer-stale-function. A short scan in vanilla Emacs shows
Buffer-menu-mode and dired-mode, which set buffer-stale-function.

Buffer-menu-mode does not use files, so it doesn't profit from file
notifications. One could write another kind of notification which fires
when buffers are created or deleted, but that's another story. And I
doubt it will be more useful than the current auto-reverting for buffer
lists.

So indeed, dired is left for vanilla Emacs. It shall set
buffer-auto-revert-by-file-notification-aware when a buffer is setup to
dired-mode. Other packages in the wild could do similar settings, think
about vc-dir or magit, which use their own machinery. Potentially, any
mode which uses (an own implementation of) revert-buffer, would be a
candidate for this kind of auto-revert.

Thinking about, I'm even not confident that a static value of this
indication is sufficient. In dired, it might be set to t when the dired
buffer is setup. But what if the dired buffer contains subdirectories?
Is it still possible to indicate this by file notifications over
default-directory? Don't know, maybe not, and the variable has to be set
to nil ...

Long story short: we shall start with dired, which sets a buffer-local
variable as indication, and we shall edocument this in the Elisp
manual. Let's see where we go.

Best regards, Michael.

Message #260 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

> I understand that some people are queasy about using advice in code
> like this, and am open to suggestions about alternatives.

It's not just being qeasy. It is a design rule, that advising functions
do not belong to core Emacs.

> What the code needs is a reasonable (not necessarily bullet-proof) way
> to detect new file buffers and changes to buffer-file-name of those
> buffers. Monitoring `find-file-noselect' and `set-visited-file-name'
> turned out to be good enough.
> For the former, it might be possible to get away with
> `after-change-major-mode-hook' instead (already used for non-file
> buffers).

There is `find-file-hook'. If we need to hook into
`set-visited-file-name', we shall create a new hook
`after-set-visited-file-name', and run it there.

> +(defvar-local global-auto-revert--tracked-buffer nil
> +  "Non-nil if buffer is handled by Global Auto-Revert mode.")
> +

Somehow, I'm not so comfortable with that name. Could we take
`auto-revert-global-mode'? It is similar to `auto-revert-mode' and
`auto-revert-tail-mode', with the disadvantage that there does not exist
such a mode.

Alternatively, we could create a local variable `global-autorevert-mode'
in buffers which are tracked, and check always for that local value
where it matters.

> +(defun auto-revert--find-file-noselect-advice (buffer)
> +  "Adopt BUFFER for Global Auto-Revert if appropriate.
> +Called with the return value of `find-file-noselect'."
> +  (auto-revert--global-add-buffer buffer)
> +  (auto-revert-set-timer)
> +  buffer)
> +
> +(defun auto-revert--after-change-major-mode ()
> +  "Adopt the current buffer for Global Auto-Revert if appropriate.
> +Called after the current buffer got a new major mode."
> +  (auto-revert--global-add-buffer (current-buffer))
> +  (auto-revert-set-timer))

These are almost identical. Make argument buffer optional, and it is
just one function.

Best regards, Michael.

Message #263 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

10 maj 2019 kl. 11.49 skrev Michael Albinus <michael.albinus <at> gmx.de>:

> There is `find-file-hook'. If we need to hook into
> `set-visited-file-name', we shall create a new hook
> `after-set-visited-file-name', and run it there.

Thank you, it looks like `find-file-hook' will do. It's `set-visited-file-name' that lacks a hook. We could add one, `after-visited-file-name-change-hook' say, and run it at the end of that function. It would come in handy for fixing the write-file bug, too.

That would suffice for this particular need, but we may contemplate some variations for general utility, such as passing the old value of buffer-file-name to the hook. It also wouldn't catch direct modifications of buffer-file-name, but that mostly happens in  special buffers that we don't want to autorevert anyway (?).

Perhaps we should exclude all buffers whose name start with a space from any kind of auto-revert, just in case.

>> +(defvar-local global-auto-revert--tracked-buffer nil
>> +  "Non-nil if buffer is handled by Global Auto-Revert mode.")
>> +
> 
> Somehow, I'm not so comfortable with that name. Could we take
> `auto-revert-global-mode'? It is similar to `auto-revert-mode' and
> `auto-revert-tail-mode', with the disadvantage that there does not exist
> such a mode.

Agreed, and I never liked that variable name much myself. What about `auto-revert--global-mode'? (More names in autorevert.el should have double dashes, but I suppose it was written before that convention came along.)

> Alternatively, we could create a local variable `global-autorevert-mode'
> in buffers which are tracked, and check always for that local value
> where it matters.

Possibly, but that sounds slightly more error-prone.

>> +(defun auto-revert--find-file-noselect-advice (buffer)
[..]
>> +(defun auto-revert--after-change-major-mode ()
> 
> These are almost identical. Make argument buffer optional, and it is
> just one function.

Good point, but the advice functions will probably be replaced anyway per your request; let's see what it looks like when that is done.

Message #266 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

> That would suffice for this particular need, but we may contemplate
> some variations for general utility, such as passing the old value of
> buffer-file-name to the hook. It also wouldn't catch direct
> modifications of buffer-file-name, but that mostly happens in special
> buffers that we don't want to autorevert anyway (?).

So we must document in the Elisp manual, that buffers, which want to
participate in global-auto-revert-mode after a renaming, shall change
the name via set-visited-file-name.

> Perhaps we should exclude all buffers whose name start with a space
> from any kind of auto-revert, just in case.

Agreed. Those buffers are special (internal) anyway, it's already tricky
to show them. Nobody needs auto-revert for invisible buffers :-)

> Agreed, and I never liked that variable name much myself. What about
> `auto-revert--global-mode'? (More names in autorevert.el should have
> double dashes, but I suppose it was written before that convention
> came along.)

D'accord.

>>> +(defun auto-revert--find-file-noselect-advice (buffer)
> [..]
>>> +(defun auto-revert--after-change-major-mode ()
>> 
>> These are almost identical. Make argument buffer optional, and it is
>> just one function.
>
> Good point, but the advice functions will probably be replaced anyway
> per your request; let's see what it looks like when that is done.

But they will convert to hook functions then ...

Best regards, Michael.

Message #269 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

9 maj 2019 kl. 13.50 skrev Michael Albinus <michael.albinus <at> gmx.de>:

> Thinking about, I'm even not confident that a static value of this
> indication is sufficient. In dired, it might be set to t when the dired
> buffer is setup. But what if the dired buffer contains subdirectories?
> Is it still possible to indicate this by file notifications over
> default-directory? Don't know, maybe not, and the variable has to be set
> to nil ...

There is a third possibility: a buffer requires polling, but could make use of notification. The distinction is only important when `auto-revert-avoid-polling' is set. For example, we could have, say, `buffer-auto-revert-by-notification' take the values nil, notify-only and notify-and-poll. This permits immediate updates for some changes while still not keeping stale information indefinitely.

On the other hand, maybe that sort of mode-specific complexity is better left to the modes themselves?

Message #272 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

>> Thinking about, I'm even not confident that a static value of this
>> indication is sufficient. In dired, it might be set to t when the dired
>> buffer is setup. But what if the dired buffer contains subdirectories?
>> Is it still possible to indicate this by file notifications over
>> default-directory? Don't know, maybe not, and the variable has to be set
>> to nil ...
>
> There is a third possibility: a buffer requires polling, but could
> make use of notification. The distinction is only important when
> `auto-revert-avoid-polling' is set. For example, we could have, say,
> `buffer-auto-revert-by-notification' take the values nil, notify-only
> and notify-and-poll. This permits immediate updates for some changes
> while still not keeping stale information indefinitely.

I haven't seen this requirement yet for any mode. Let's postpone this,
until there is a real request for this kind of distinction.

Furthermore, we shouldn't expect a deep knowledge from major mode
writers about the internals of auto-revert. People might have a hard
time to understand the difference between notify-only and
notify-and-poll.

Best regards, Michael.

Message #275 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

sön 2019-05-12 klockan 10:48 +0200 skrev Michael Albinus:
> 
> I haven't seen this requirement yet for any mode. Let's postpone
> this,
> until there is a real request for this kind of distinction.

Very well. Here is an updated patch, with a new buffer-local variable
controlling whether non-file buffers can rely on notification in
autorevert.

[0001-Don-t-use-file-notification-on-non-file-buffers.patch (text/x-patch, attachment)]

Message #278 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

fre 2019-05-10 klockan 14:43 +0200 skrev Michael Albinus:
> Mattias Engdegård <mattiase <at> acm.org> writes:
> 
> So we must document in the Elisp manual, that buffers, which want to
> participate in global-auto-revert-mode after a renaming, shall change
> the name via set-visited-file-name.

Are you sure about that? It sounds quite technical. But if you think it
is necessary, I'll add it.

> 
> > Perhaps we should exclude all buffers whose name start with a space
> > from any kind of auto-revert, just in case.
> 
> Agreed. Those buffers are special (internal) anyway, it's already
> tricky
> to show them. Nobody needs auto-revert for invisible buffers :-)

The revised patch now excludes such non-file buffers. I wonder if
buffers with file names should be excluded as well. They trivially
occur when visiting a file whose name starts with a space.

The new patch also has the tracking variable renamed to
`auto-revert--global-mode' and added a new hook,
`after-set-visited-file-name'; the advice calls are gone.

[0001-Avoid-polling-in-global-auto-revert-mode.patch (text/x-patch, attachment)]

Message #281 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

> --- a/doc/emacs/arevert-xtra.texi
> +++ b/doc/emacs/arevert-xtra.texi
>  
> +@vindex buffer-auto-revert-by-notification
> +Some non-file buffers can be updated reliably by file notification on
> +their default directory.  This can be indicated by setting
> +@code{buffer-auto-revert-by-notification} to a non-@code{nil} value in
> +that buffer, allowing Auto Revert to avoid periodic polling.  Such
> +notification does not include changes to files in that directory, only
> +to the directory itself.

Do we want to say that this is related to a major mode in general? That
means, that the mode function shall set this variable. And do we want to
mention dired-mode as example?
  
> --- a/etc/NEWS
> +++ b/etc/NEWS
>  
> +*** New variable 'buffer-auto-revert-by-notification'
> +Non-file buffers can explicitly declare that notification on their
> +default-directory is sufficient to know when updates are required by
> +setting the new variable `buffer-auto-revert-by-notification' to a
> +non-nil value in that buffer.  Auto Revert mode can use this
> +information to avoid polling the buffer periodically when
> +'auto-revert-avoid-polling' is non-nil.

Same remark. Mention major mode dependency.

Otherwise, the patch LGTM.

Best regards, Michael.

Message #284 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

>> So we must document in the Elisp manual, that buffers, which want to
>> participate in global-auto-revert-mode after a renaming, shall change
>> the name via set-visited-file-name.
>
> Are you sure about that? It sounds quite technical. But if you think it
> is necessary, I'll add it.

I'm not sure, of course. But how else can such a buffer participate, if
we don't poll anymore (and refresh the list of buffers to be watched)?
To be tested, I would say.

>> Agreed. Those buffers are special (internal) anyway, it's already
>> tricky to show them. Nobody needs auto-revert for invisible buffers
>> :-)
>
> The revised patch now excludes such non-file buffers. I wonder if
> buffers with file names should be excluded as well. They trivially
> occur when visiting a file whose name starts with a space.

`list-buffers' shows buffers with a leading space, if they are visiting a
file. If they don't visit a file, they are not listed.

I would say we shall apply the same rule.

The patch LGTM. Do you want also add some tests to autorevert-tests.el?

Best regards, Michael.

Message #287 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

mån 2019-05-13 klockan 15:35 +0200 skrev Michael Albinus:
> 
> Do we want to say that this is related to a major mode in general?
> That
> means, that the mode function shall set this variable. And do we want
> to
> mention dired-mode as example?

Good point, paragraph changed.

> Same remark. Mention major mode dependency.

Done.

Updated patch attached.

[0001-Don-t-use-file-notification-on-non-file-buffers.patch (text/x-patch, attachment)]

Message #290 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

> Updated patch attached.

Almost OK, but ...

> --- a/etc/NEWS
> +++ b/etc/NEWS

> +the new variable `buffer-auto-revert-by-notification' to a non-nil

... please quote like 'this'.

From my POV, this can be pushed.

Best regards, Michael.

Message #293 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

13 maj 2019 kl. 17.08 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
> The patch LGTM. Do you want also add some tests to autorevert-tests.el?

Thank you -- I added one, but have only access to kqueue at the moment so I haven't run it on anything else.

[0001-Avoid-polling-in-global-auto-revert-mode-bug-35418.patch (application/octet-stream, attachment)]

Message #296 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

> Thank you -- I added one, but have only access to kqueue at the moment
> so I haven't run it on anything else.

Thanks. I've applied it also for inotify, gfile, inotifywait and
gio-monitor. All tests passed, at least the local
ones. auto-revert-test05-global-notify-remote (see below) failed; this
one I could debug myself once the patch has landed in master. I suspect
timing issues.

Btw, you could always run the four tests on emba.gnu.org. Submit a
change (also a git branch would do), and the bot on emba.git.org would
run these test constellations for you. Same for filenotify-tests.

Comments:

> --- a/lisp/autorevert.el
> +++ b/lisp/autorevert.el

>    "Check if auto-revert is active (in current buffer or globally)."

Remove "or globally".

> --- a/test/lisp/autorevert-tests.el
> +++ b/test/lisp/autorevert-tests.el

> +(defun auto-revert-test--write-file (string file)
> +  (write-region string nil file nil 'no-message))
> +
> +(defun auto-revert-test--buffer-string (buffer)
> +  (with-current-buffer buffer
> +    (buffer-string)))

Pls add a docstring.

> +(ert-deftest auto-revert-test05-global-notify ()
> +  "Test global-auto-revert-mode without polling."

Quote `global-auto-revert-mode'. Check also, whether file notification
is enabled:

(skip-unless (or file-notify--library
                 (file-remote-p temporary-file-directory)))

> +  (let* ((auto-revert-avoid-polling t)

Enable file notification explicitly. You don't know, whether the user
has disabled it.

(let* ((auto-revert-use-notify t)
       (auto-revert-avoid-polling t)

> +         (auto-revert-interval 2)       ; To speed up the test.

Do we really want this? I prefer to test the unmodified package. If you
believe this takes too much time for ordinary "make check" calls, you
might tag the test case as :expensive-test, like
auto-revert-test01-auto-revert-several-files and
auto-revert-test02-auto-revert-deleted-file.

> +          (global-auto-revert-mode 1)   ; Turn it on.

Save the value of global-auto-revert-mode, and reset it in Cleanup. You
don't know the user's settings.

> +      (dolist (buf (list buf-1 buf-2 buf-3))
> +        (when (buffer-live-p buf)
> +          (kill-buffer buf)))

Why not (ignore-errors (kill-buffer buf)) ?

Add the remote test case:

(auto-revert--deftest-remote auto-revert-test05-global-notify
  "Test `global-auto-revert-mode' without polling for remote buffers."

Best regards, Michael.

Message #299 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

[Message part 1 (text/plain, inline)]

19 maj 2019 kl. 11.12 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
> Btw, you could always run the four tests on emba.gnu.org. Submit a
> change (also a git branch would do), and the bot on emba.git.org would
> run these test constellations for you. Same for filenotify-tests.

That's good to know -- I'll try that next time.

>>   "Check if auto-revert is active (in current buffer or globally)."
> 
> Remove "or globally".

Done.

>> +(defun auto-revert-test--write-file (string file)
>> +  (write-region string nil file nil 'no-message))
>> +
>> +(defun auto-revert-test--buffer-string (buffer)
>> +  (with-current-buffer buffer
>> +    (buffer-string)))
> 
> Pls add a docstring.

Added.

>> +(ert-deftest auto-revert-test05-global-notify ()
>> +  "Test global-auto-revert-mode without polling."
> 
> Quote `global-auto-revert-mode'. Check also, whether file notification
> is enabled:
> 
> (skip-unless (or file-notify--library
>                 (file-remote-p temporary-file-directory)))

Quoted, and check added.

>> +  (let* ((auto-revert-avoid-polling t)
> 
> Enable file notification explicitly. You don't know, whether the user
> has disabled it.
> 
> (let* ((auto-revert-use-notify t)
>       (auto-revert-avoid-polling t)

Done.

>> +         (auto-revert-interval 2)       ; To speed up the test.
> 
> Do we really want this? I prefer to test the unmodified package. If you
> believe this takes too much time for ordinary "make check" calls, you
> might tag the test case as :expensive-test, like
> auto-revert-test01-auto-revert-several-files and
> auto-revert-test02-auto-revert-deleted-file.

That was probably just me being impatient; I've removed that line and added :expensive-test.

>> +          (global-auto-revert-mode 1)   ; Turn it on.
> 
> Save the value of global-auto-revert-mode, and reset it in Cleanup. You
> don't know the user's settings.

Done.

>> +      (dolist (buf (list buf-1 buf-2 buf-3))
>> +        (when (buffer-live-p buf)
>> +          (kill-buffer buf)))
> 
> Why not (ignore-errors (kill-buffer buf)) ?

Using a condition felt more precise, but I have no strong opinion in this case. Changed to ignore-errors.

> Add the remote test case:
> 
> (auto-revert--deftest-remote auto-revert-test05-global-notify
>  "Test `global-auto-revert-mode' without polling for remote buffers."

Added.

Thanks for the tests and review! Revised patch attached.

[0001-Avoid-polling-in-global-auto-revert-mode-bug-35418.patch (application/octet-stream, attachment)]

Message #302 received at 35418 <at> debbugs.gnu.org (full text, mbox):

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

Mattias Engdegård <mattiase <at> acm.org> writes:

Hi Mattias,

>> Btw, you could always run the four tests on emba.gnu.org. Submit a
>> change (also a git branch would do), and the bot on emba.git.org would
>> run these test constellations for you. Same for filenotify-tests.
>
> That's good to know -- I'll try that next time.

You can see it just now :-)

From my POV, the patch is fine, and could be committed. Whether it is
successful will be told us by emba.gnu.org (and hydra.nixos.org).

This is the last change wrt bug#35418, isn't it? You could close the bug
then, if both CI systems do not report an error for autorevert-tests or
filenotify-tests.

Best regards, Michael.

Message #307 received at 35418-done <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 35418-done <at> debbugs.gnu.org
Subject: Re: bug#35418: [PATCH] Don't poll auto-revert files that use
 notification
Date: Mon, 20 May 2019 21:19:37 +0200

20 maj 2019 kl. 09.30 skrev Michael Albinus <michael.albinus <at> gmx.de>:
> 
> From my POV, the patch is fine, and could be committed. Whether it is
> successful will be told us by emba.gnu.org (and hydra.nixos.org).

Thanks for your diligence and patience; closing.

Previous Next

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