unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#43714: 28.1: auto-revert code improvements [PATCH]
@ 2020-09-30  6:26 Boruch Baum
  2020-09-30 13:48 ` Lars Ingebrigtsen
  2020-09-30 16:51 ` bug#43714: [boruch_baum@gmx.com: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]] Boruch Baum
  0 siblings, 2 replies; 18+ messages in thread
From: Boruch Baum @ 2020-09-30  6:26 UTC (permalink / raw)
  To: 43714

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

In pursuing a 'best' solution for bug-report#43412, I've taken the
plunge and gotten an emacs 28 snapshot installed locally, and in
examining the code, saw things that didn't make sense to me or were
unnecessary or could just do with better readability. My testing
indicates no harm done by the changes, but they should be peer-reviewed
and undergo further testing. Nothing in the attached patch is aimed at
changing functionality, so you may want to reject it on the basis of "if
it ain't broke, don't fix it", but it does improve the code.

Just as a 'for example': I started on this because I saw that the code
was saving match data, which had me on a wild goose chase hunting for
why and where it was necessary - I found nothing, and removing the code
doesn't seem to have ill effect.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

[-- Attachment #2: auto-revert-28-cosmetic.patch --]
[-- Type: text/x-diff, Size: 6203 bytes --]

diff --git a/autorevert.el b/autorevert.el
index 011febf..de4407b 100644
--- a/autorevert.el
+++ b/autorevert.el
@@ -869,6 +869,62 @@ This is an internal function used by Auto-Revert Mode."
       (restore-buffer-modified-p modified)))
   (set-visited-file-modtime))

+(defun auto-revert--buffer-candidates ()
+  "Return a prioritized list of buffers to maybe auto-revert.
+The differences between this return value and the reference
+variable `auto-revert-buffer-list'. include: 1) this has more
+entries when in global-auto-revert-mode; 2) this prioritizes
+buffers not reverted last time due to user interruption. "
+  (let (remaining new
+        (bufs (delq nil
+               ;; Buffers with remote contents shall be reverted only
+               ;; if the connection is established already.
+                    (mapcar
+                     (lambda (buf)
+                       (and (buffer-live-p buf)
+                            (with-current-buffer buf
+                              (and
+                               (or (not (file-remote-p default-directory))
+                                   (file-remote-p default-directory nil t))
+                                   buf))))
+                     (auto-revert--polled-buffers)))))
+    ;; Partition `bufs' into two halves depending on whether or not
+    ;; the buffers are in `auto-revert-remaining-buffers'.  The two
+    ;; halves are then re-joined with the "remaining" buffers at the
+    ;; head of the list.
+    (dolist (buf auto-revert-remaining-buffers)
+      (if (memq buf bufs)
+          (push buf remaining)))
+    (dolist (buf bufs)
+      (if (not (memq buf remaining))
+          (push buf new)))
+    (nreverse (nconc new remaining))))
+
+(defun auto-revert-buffer (buf)
+  "Revert a single buffer.
+
+This is performed as specified by Auto-Revert and Global
+Auto-Revert Modes."
+  (if (not (buffer-live-p buf))
+    (auto-revert-remove-current-buffer buf)
+   (with-current-buffer buf
+     ;; Test if someone has turned off Auto-Revert Mode
+     ;; in a non-standard way, for example by changing
+     ;; major mode.
+     (and (not auto-revert-mode)
+          (not auto-revert-tail-mode)
+          (auto-revert-remove-current-buffer))
+     (when (auto-revert-active-p)
+       ;; Enable file notification.
+       ;; Don't bother creating a notifier for non-file buffers
+       ;; unless it explicitly indicates that this works.
+       (when (and auto-revert-use-notify
+                  (not auto-revert-notify-watch-descriptor)
+                  (or buffer-file-name
+                      buffer-auto-revert-by-notification))
+         (auto-revert-notify-add-watch))
+       (auto-revert-handler)))))
+
 (defun auto-revert-buffers ()
   "Revert buffers as specified by Auto-Revert and Global Auto-Revert Mode.

@@ -892,66 +948,17 @@ are checked first the next time this function is called.
 This function is also responsible for removing buffers no longer in
 Auto-Revert Mode from `auto-revert-buffer-list', and for canceling
 the timer when no buffers need to be checked."
-
-  (save-match-data
-    (let ((bufs (auto-revert--polled-buffers))
-	  remaining new)
-      ;; Buffers with remote contents shall be reverted only if the
-      ;; connection is established already.
-      (setq bufs (delq nil
-                       (mapcar
-                        (lambda (buf)
-                          (and (buffer-live-p buf)
-                               (with-current-buffer buf
-                                 (and
-                                  (or (not (file-remote-p default-directory))
-                                      (file-remote-p default-directory nil t))
-                                      buf))))
-                        bufs)))
-      ;; Partition `bufs' into two halves depending on whether or not
-      ;; the buffers are in `auto-revert-remaining-buffers'.  The two
-      ;; halves are then re-joined with the "remaining" buffers at the
-      ;; head of the list.
-      (dolist (buf auto-revert-remaining-buffers)
-	(if (memq buf bufs)
-	    (push buf remaining)))
-      (dolist (buf bufs)
-	(if (not (memq buf remaining))
-	    (push buf new)))
-      (setq bufs (nreverse (nconc new remaining)))
+    (let ((bufs (auto-revert--buffer-candidates)))
       (while (and bufs
 		  (not (and auto-revert-stop-on-user-input
 			    (input-pending-p))))
-	(let ((buf (car bufs)))
-          (if (not (buffer-live-p buf))
-              ;; Remove dead buffer from `auto-revert-buffer-list'.
-              (auto-revert-remove-current-buffer buf)
-            (with-current-buffer buf
-              ;; Test if someone has turned off Auto-Revert Mode
-              ;; in a non-standard way, for example by changing
-              ;; major mode.
-              (if (and (not auto-revert-mode)
-                       (not auto-revert-tail-mode)
-                       (memq buf auto-revert-buffer-list))
-                  (auto-revert-remove-current-buffer))
-              (when (auto-revert-active-p)
-                ;; Enable file notification.
-                ;; Don't bother creating a notifier for non-file buffers
-                ;; unless it explicitly indicates that this works.
-                (when (and auto-revert-use-notify
-                           (not auto-revert-notify-watch-descriptor)
-                           (or buffer-file-name
-                               buffer-auto-revert-by-notification))
-                  (auto-revert-notify-add-watch))
-                (auto-revert-handler)))))
-	(setq bufs (cdr bufs)))
+        (auto-revert-buffer (pop bufs)))
       (setq auto-revert-remaining-buffers bufs)
       ;; Check if we should cancel the timer.
       (unless (auto-revert--need-polling-p)
-        (if (timerp auto-revert-timer)
-            (cancel-timer auto-revert-timer))
-	(setq auto-revert-timer nil)))))
-
+        (when (timerp auto-revert-timer)
+          (cancel-timer auto-revert-timer))
+	(setq auto-revert-timer nil))))

 ;; The end:
 (provide 'autorevert)

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30  6:26 bug#43714: 28.1: auto-revert code improvements [PATCH] Boruch Baum
@ 2020-09-30 13:48 ` Lars Ingebrigtsen
  2020-09-30 14:25   ` Eli Zaretskii
  2020-09-30 15:09   ` Boruch Baum
  2020-09-30 16:51 ` bug#43714: [boruch_baum@gmx.com: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]] Boruch Baum
  1 sibling, 2 replies; 18+ messages in thread
From: Lars Ingebrigtsen @ 2020-09-30 13:48 UTC (permalink / raw)
  To: Boruch Baum; +Cc: 43714

Boruch Baum <boruch_baum@gmx.com> writes:

> Just as a 'for example': I started on this because I saw that the code
> was saving match data, which had me on a wild goose chase hunting for
> why and where it was necessary - I found nothing, and removing the code
> doesn't seem to have ill effect.

That's really hard to tell, though -- the reason for saving the match
data may be in hard-to-trigger circumstances, and the saving was
introduced in a commit of its own, so I'm guessing there was some
specific reason for it.

The commit message is super-helpful here:

commit 33512cbeb168764f83f5a740090ce40b4b948591
Author:     Luc Teirlinck <teirllm@auburn.edu>
AuthorDate: Wed Jun 1 20:51:03 2005 +0000

    (auto-revert-buffers): Use save-match-data.


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30 13:48 ` Lars Ingebrigtsen
@ 2020-09-30 14:25   ` Eli Zaretskii
  2020-09-30 15:28     ` Boruch Baum
  2020-09-30 17:21     ` Michael Albinus
  2020-09-30 15:09   ` Boruch Baum
  1 sibling, 2 replies; 18+ messages in thread
From: Eli Zaretskii @ 2020-09-30 14:25 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: boruch_baum, 43714

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Wed, 30 Sep 2020 15:48:05 +0200
> Cc: 43714@debbugs.gnu.org
> 
> commit 33512cbeb168764f83f5a740090ce40b4b948591
> Author:     Luc Teirlinck <teirllm@auburn.edu>
> AuthorDate: Wed Jun 1 20:51:03 2005 +0000
> 
>     (auto-revert-buffers): Use save-match-data.

It could be due to remote files (and file-handlers)?





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30 13:48 ` Lars Ingebrigtsen
  2020-09-30 14:25   ` Eli Zaretskii
@ 2020-09-30 15:09   ` Boruch Baum
  2020-09-30 15:12     ` Lars Ingebrigtsen
  2020-09-30 17:41     ` Glenn Morris
  1 sibling, 2 replies; 18+ messages in thread
From: Boruch Baum @ 2020-09-30 15:09 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 43714

On 2020-09-30 15:48, Lars Ingebrigtsen wrote:
> Boruch Baum <boruch_baum@gmx.com> writes:
>
> > Just as a 'for example': I started on this because I saw that the code
> > was saving match data, which had me on a wild goose chase hunting for
> > why and where it was necessary - I found nothing, and removing the code
> > doesn't seem to have ill effect.
>
> That's really hard to tell, though -- the reason for saving the match
> data may be in hard-to-trigger circumstances, and the saving was
> introduced in a commit of its own, so I'm guessing there was some
> specific reason for it.
>
> The commit message is super-helpful here:
>
> commit 33512cbeb168764f83f5a740090ce40b4b948591
> Author:     Luc Teirlinck <teirllm@auburn.edu>
> AuthorDate: Wed Jun 1 20:51:03 2005 +0000
>
>     (auto-revert-buffers): Use save-match-data.

Seriously now. Should I send an email to Luc, asking him what he was
thinking 15 years ago? Otherwise, I'd be willing to look over the
versions at that commit and it's HEAD^ if you can send them to me.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30 15:09   ` Boruch Baum
@ 2020-09-30 15:12     ` Lars Ingebrigtsen
  2020-09-30 17:41     ` Glenn Morris
  1 sibling, 0 replies; 18+ messages in thread
From: Lars Ingebrigtsen @ 2020-09-30 15:12 UTC (permalink / raw)
  To: Boruch Baum; +Cc: 43714

Boruch Baum <boruch_baum@gmx.com> writes:

> Seriously now. Should I send an email to Luc, asking him what he was
> thinking 15 years ago? Otherwise, I'd be willing to look over the
> versions at that commit and it's HEAD^ if you can send them to me.

Pulling down (and building) Emacs is super-duper easy:

sudo apt build-dep emacs
git clone git://git.savannah.gnu.org/emacs.git
cd emacs
make
./src/emacs &


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30 14:25   ` Eli Zaretskii
@ 2020-09-30 15:28     ` Boruch Baum
  2020-09-30 16:03       ` Eli Zaretskii
  2020-09-30 17:21     ` Michael Albinus
  1 sibling, 1 reply; 18+ messages in thread
From: Boruch Baum @ 2020-09-30 15:28 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Lars Ingebrigtsen, 43714

On 2020-09-30 17:25, Eli Zaretskii wrote:
> >     (auto-revert-buffers): Use save-match-data.
>
> It could be due to remote files (and file-handlers)?

But I don't see anywhere in the code itself being evaluated in
auto-revert.el that would produce its own match-data to potentially
disturb the pre-existing state.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30 15:28     ` Boruch Baum
@ 2020-09-30 16:03       ` Eli Zaretskii
  2020-09-30 16:59         ` Boruch Baum
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2020-09-30 16:03 UTC (permalink / raw)
  To: Boruch Baum; +Cc: larsi, 43714

> Date: Wed, 30 Sep 2020 11:28:27 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 43714@debbugs.gnu.org
> 
> On 2020-09-30 17:25, Eli Zaretskii wrote:
> > >     (auto-revert-buffers): Use save-match-data.
> >
> > It could be due to remote files (and file-handlers)?
> 
> But I don't see anywhere in the code itself being evaluated in
> auto-revert.el that would produce its own match-data to potentially
> disturb the pre-existing state.

That's what I meant: it could be hidden from plain sight.

Why is that a problem to leave save-match-data alone?  Does it do any
harm?





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: [boruch_baum@gmx.com: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]]
  2020-09-30  6:26 bug#43714: 28.1: auto-revert code improvements [PATCH] Boruch Baum
  2020-09-30 13:48 ` Lars Ingebrigtsen
@ 2020-09-30 16:51 ` Boruch Baum
  2020-09-30 16:54   ` Lars Ingebrigtsen
  1 sibling, 1 reply; 18+ messages in thread
From: Boruch Baum @ 2020-09-30 16:51 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 43714

On 2020-09-30 17:12, Lars Ingebrigtsen wrote:
> Boruch Baum <boruch_baum@gmx.com> writes:
>
> > Seriously now. Should I send an email to Luc, asking him what he was
> > thinking 15 years ago? Otherwise, I'd be willing to look over the
> > versions at that commit and it's HEAD^ if you can send them to me.
>
> Pulling down (and building) Emacs is super-duper easy:

No, it wasn't. I spent a lot of time last week failing to resolve the
ensuing dependency hell generated.

>
> sudo apt build-dep emacs
> git clone git://git.savannah.gnu.org/emacs.git
> cd emacs
> make
> ./src/emacs &

Maybe I wasn't clear. I meant just the file autorevert.el from the
commit and its HEAD^, not the entire emacs snapshot at those points.

Apart from that, all I need is probably less than 5kb of data. You're
asking me to pull in how many mega-bytes / giga-bytes of git for this?
Had I the extra space and computing power, I would have done it ages
ago (seven year old pentium...).

Another possibility would be if there is there a way to query the
savannah cgit web interface for that individual file at the snapshot and
its antecedent?

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: [boruch_baum@gmx.com: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]]
  2020-09-30 16:51 ` bug#43714: [boruch_baum@gmx.com: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]] Boruch Baum
@ 2020-09-30 16:54   ` Lars Ingebrigtsen
  2020-09-30 17:10     ` Boruch Baum
  0 siblings, 1 reply; 18+ messages in thread
From: Lars Ingebrigtsen @ 2020-09-30 16:54 UTC (permalink / raw)
  To: Boruch Baum; +Cc: 43714

Boruch Baum <boruch_baum@gmx.com> writes:

>> Pulling down (and building) Emacs is super-duper easy:
>
> No, it wasn't. I spent a lot of time last week failing to resolve the
> ensuing dependency hell generated.
>
>> sudo apt build-dep emacs
>> git clone git://git.savannah.gnu.org/emacs.git
>> cd emacs
>> make
>> ./src/emacs &
>
> Maybe I wasn't clear. I meant just the file autorevert.el from the
> commit and its HEAD^, not the entire emacs snapshot at those points.

Even if you don't want to build Emacs, "git clone" gives you the tree
and you can investigate whetever you want.

> Apart from that, all I need is probably less than 5kb of data. You're
> asking me to pull in how many mega-bytes / giga-bytes of git for this?
> Had I the extra space and computing power, I would have done it ages
> ago (seven year old pentium...).

I didn't know they were still making Pentium CPUs seven years ago.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30 16:03       ` Eli Zaretskii
@ 2020-09-30 16:59         ` Boruch Baum
  2020-09-30 17:01           ` Lars Ingebrigtsen
  0 siblings, 1 reply; 18+ messages in thread
From: Boruch Baum @ 2020-09-30 16:59 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, 43714

On 2020-09-30 19:03, Eli Zaretskii wrote:
> Why is that a problem to leave save-match-data alone?  Does it do any
> harm?

I explicitly said in the introduction to the patch that it wasn't
'fixing' anything broken, just cleaning things up (potentially /
hopefully). When I was researching how to best implement that "auto-revert
only if visible" feature, I found myself puzzling over some parts of the
code and came to clean it up to be better able to wok on it.

That said, You could leave the (save-match-data alone, and accept the other
changes; the other proposed changes are clearly independent.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30 16:59         ` Boruch Baum
@ 2020-09-30 17:01           ` Lars Ingebrigtsen
  2020-09-30 17:13             ` Boruch Baum
  0 siblings, 1 reply; 18+ messages in thread
From: Lars Ingebrigtsen @ 2020-09-30 17:01 UTC (permalink / raw)
  To: Boruch Baum; +Cc: 43714

Boruch Baum <boruch_baum@gmx.com> writes:

> That said, You could leave the (save-match-data alone, and accept the other
> changes; the other proposed changes are clearly independent.

Could you submit a patch with the save-match-data left intact?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: [boruch_baum@gmx.com: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]]
  2020-09-30 16:54   ` Lars Ingebrigtsen
@ 2020-09-30 17:10     ` Boruch Baum
  0 siblings, 0 replies; 18+ messages in thread
From: Boruch Baum @ 2020-09-30 17:10 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 43714

On 2020-09-30 18:54, Lars Ingebrigtsen wrote:
> Boruch Baum <boruch_baum@gmx.com> writes:
>
> Even if you don't want to build Emacs, "git clone" gives you the tree
> and you can investigate whetever you want.

Then you read my next paragraph...

> > Apart from that, all I need is probably less than 5kb of data. You're
> > asking me to pull in how many mega-bytes / giga-bytes of git for this?
> > Had I the extra space and computing power, I would have done it ages
> > ago (seven year old pentium...).
>
> I didn't know they were still making Pentium CPUs seven years ago.

dmidecode tells me its a pentium; I routinely forget all the detailed
hardware internals of my machine, but I remember purchasing it new seven
years ago.

So, returning to focus on the work. option 1: just flat out reject that
line of the patch; option 2: send me the old versions of the files;
option 3: tell me how to use the savannah cgit interface to pull the two
files.

This all being volunteer work, all options are of course themselves
optional.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30 17:01           ` Lars Ingebrigtsen
@ 2020-09-30 17:13             ` Boruch Baum
  0 siblings, 0 replies; 18+ messages in thread
From: Boruch Baum @ 2020-09-30 17:13 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 43714

On 2020-09-30 19:01, Lars Ingebrigtsen wrote:
> Boruch Baum <boruch_baum@gmx.com> writes:
>
> > That said, You could leave the (save-match-data alone, and accept the other
> > changes; the other proposed changes are clearly independent.
>
> Could you submit a patch with the save-match-data left intact?

Does that mean that the remainder is acceptable / accepted without
further discussion? If I'm going to make more submissions, I'd prefer to
minimize / aggregate the work.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30 14:25   ` Eli Zaretskii
  2020-09-30 15:28     ` Boruch Baum
@ 2020-09-30 17:21     ` Michael Albinus
  1 sibling, 0 replies; 18+ messages in thread
From: Michael Albinus @ 2020-09-30 17:21 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Lars Ingebrigtsen, boruch_baum, 43714

Eli Zaretskii <eliz@gnu.org> writes:

>> commit 33512cbeb168764f83f5a740090ce40b4b948591
>> Author:     Luc Teirlinck <teirllm@auburn.edu>
>> AuthorDate: Wed Jun 1 20:51:03 2005 +0000
>>
>>     (auto-revert-buffers): Use save-match-data.
>
> It could be due to remote files (and file-handlers)?

Could be the case. There is no promise that the file name handlers do
not change match data. And I'm kind of sure this save-match-data form
has been introduced after such an accident, that auto-revert has
corrupted somebody's match data.

So please keep it. It doesn't hurt.

Best regards, Michael.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30 15:09   ` Boruch Baum
  2020-09-30 15:12     ` Lars Ingebrigtsen
@ 2020-09-30 17:41     ` Glenn Morris
  2020-09-30 18:33       ` Boruch Baum
  1 sibling, 1 reply; 18+ messages in thread
From: Glenn Morris @ 2020-09-30 17:41 UTC (permalink / raw)
  To: Boruch Baum; +Cc: Lars Ingebrigtsen, 43714


The mailing list archives around the time of a commit are often informative.

https://lists.gnu.org/r/emacs-devel/2005-05/msg01414.html





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30 17:41     ` Glenn Morris
@ 2020-09-30 18:33       ` Boruch Baum
  2020-09-30 23:55         ` Lars Ingebrigtsen
  0 siblings, 1 reply; 18+ messages in thread
From: Boruch Baum @ 2020-09-30 18:33 UTC (permalink / raw)
  To: Glenn Morris; +Cc: Lars Ingebrigtsen, Michael Albinus, 43714

[-- Attachment #1: Type: text/plain, Size: 1029 bytes --]

On 2020-09-30 13:41, Glenn Morris wrote:
> The mailing list archives around the time of a commit are often informative.
> https://lists.gnu.org/r/emacs-devel/2005-05/msg01414.html

Absolutely great, Glenn!

An obvious follow-up observation: The patch that was applied was for
Luc's option #3, but it seems to me that the honest solution to the
problem with timers is his option #2, to put the fix in timers. Was that
ever done?

I'm guessing a decision at some point was made not to pursue Luc's
option #2 because the emacs documentation for timers[1] says, "If a
timer function calls functions that can change the match data, it should
save and restore the match data. See Saving Match Data". Reconsidering
that decision could be of benefit emacs-wide.

Attached is a new version of the patch, with the 'save-match-data'
restored. Thanks again, Glenn.

[1] https://www.gnu.org/software/emacs/manual/html_node/elisp/Timers.html

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

[-- Attachment #2: auto-revert-28-cosmetic.patch --]
[-- Type: text/x-diff, Size: 6170 bytes --]

diff --git a/autorevert.el b/autorevert.el
index 011febf..4ca8010 100644
--- a/autorevert.el
+++ b/autorevert.el
@@ -869,6 +869,62 @@ This is an internal function used by Auto-Revert Mode."
       (restore-buffer-modified-p modified)))
   (set-visited-file-modtime))

+(defun auto-revert--buffer-candidates ()
+  "Return a prioritized list of buffers to maybe auto-revert.
+The differences between this return value and the reference
+variable `auto-revert-buffer-list'. include: 1) this has more
+entries when in global-auto-revert-mode; 2) this prioritizes
+buffers not reverted last time due to user interruption. "
+  (let (remaining new
+        (bufs (delq nil
+               ;; Buffers with remote contents shall be reverted only
+               ;; if the connection is established already.
+                    (mapcar
+                     (lambda (buf)
+                       (and (buffer-live-p buf)
+                            (with-current-buffer buf
+                              (and
+                               (or (not (file-remote-p default-directory))
+                                   (file-remote-p default-directory nil t))
+                                   buf))))
+                     (auto-revert--polled-buffers)))))
+    ;; Partition `bufs' into two halves depending on whether or not
+    ;; the buffers are in `auto-revert-remaining-buffers'.  The two
+    ;; halves are then re-joined with the "remaining" buffers at the
+    ;; head of the list.
+    (dolist (buf auto-revert-remaining-buffers)
+      (if (memq buf bufs)
+          (push buf remaining)))
+    (dolist (buf bufs)
+      (if (not (memq buf remaining))
+          (push buf new)))
+    (nreverse (nconc new remaining))))
+
+(defun auto-revert-buffer (buf)
+  "Revert a single buffer.
+
+This is performed as specified by Auto-Revert and Global
+Auto-Revert Modes."
+  (if (not (buffer-live-p buf))
+    (auto-revert-remove-current-buffer buf)
+   (with-current-buffer buf
+     ;; Test if someone has turned off Auto-Revert Mode
+     ;; in a non-standard way, for example by changing
+     ;; major mode.
+     (and (not auto-revert-mode)
+          (not auto-revert-tail-mode)
+          (auto-revert-remove-current-buffer))
+     (when (auto-revert-active-p)
+       ;; Enable file notification.
+       ;; Don't bother creating a notifier for non-file buffers
+       ;; unless it explicitly indicates that this works.
+       (when (and auto-revert-use-notify
+                  (not auto-revert-notify-watch-descriptor)
+                  (or buffer-file-name
+                      buffer-auto-revert-by-notification))
+         (auto-revert-notify-add-watch))
+       (auto-revert-handler)))))
+
 (defun auto-revert-buffers ()
   "Revert buffers as specified by Auto-Revert and Global Auto-Revert Mode.

@@ -892,67 +948,19 @@ are checked first the next time this function is called.
 This function is also responsible for removing buffers no longer in
 Auto-Revert Mode from `auto-revert-buffer-list', and for canceling
 the timer when no buffers need to be checked."
-
   (save-match-data
-    (let ((bufs (auto-revert--polled-buffers))
-	  remaining new)
-      ;; Buffers with remote contents shall be reverted only if the
-      ;; connection is established already.
-      (setq bufs (delq nil
-                       (mapcar
-                        (lambda (buf)
-                          (and (buffer-live-p buf)
-                               (with-current-buffer buf
-                                 (and
-                                  (or (not (file-remote-p default-directory))
-                                      (file-remote-p default-directory nil t))
-                                      buf))))
-                        bufs)))
-      ;; Partition `bufs' into two halves depending on whether or not
-      ;; the buffers are in `auto-revert-remaining-buffers'.  The two
-      ;; halves are then re-joined with the "remaining" buffers at the
-      ;; head of the list.
-      (dolist (buf auto-revert-remaining-buffers)
-	(if (memq buf bufs)
-	    (push buf remaining)))
-      (dolist (buf bufs)
-	(if (not (memq buf remaining))
-	    (push buf new)))
-      (setq bufs (nreverse (nconc new remaining)))
+    (let ((bufs (auto-revert--buffer-candidates)))
       (while (and bufs
 		  (not (and auto-revert-stop-on-user-input
 			    (input-pending-p))))
-	(let ((buf (car bufs)))
-          (if (not (buffer-live-p buf))
-              ;; Remove dead buffer from `auto-revert-buffer-list'.
-              (auto-revert-remove-current-buffer buf)
-            (with-current-buffer buf
-              ;; Test if someone has turned off Auto-Revert Mode
-              ;; in a non-standard way, for example by changing
-              ;; major mode.
-              (if (and (not auto-revert-mode)
-                       (not auto-revert-tail-mode)
-                       (memq buf auto-revert-buffer-list))
-                  (auto-revert-remove-current-buffer))
-              (when (auto-revert-active-p)
-                ;; Enable file notification.
-                ;; Don't bother creating a notifier for non-file buffers
-                ;; unless it explicitly indicates that this works.
-                (when (and auto-revert-use-notify
-                           (not auto-revert-notify-watch-descriptor)
-                           (or buffer-file-name
-                               buffer-auto-revert-by-notification))
-                  (auto-revert-notify-add-watch))
-                (auto-revert-handler)))))
-	(setq bufs (cdr bufs)))
+        (auto-revert-buffer (pop bufs)))
       (setq auto-revert-remaining-buffers bufs)
       ;; Check if we should cancel the timer.
       (unless (auto-revert--need-polling-p)
-        (if (timerp auto-revert-timer)
-            (cancel-timer auto-revert-timer))
+        (when (timerp auto-revert-timer)
+          (cancel-timer auto-revert-timer))
 	(setq auto-revert-timer nil)))))

-
 ;; The end:
 (provide 'autorevert)


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30 18:33       ` Boruch Baum
@ 2020-09-30 23:55         ` Lars Ingebrigtsen
  2020-10-01  5:42           ` Boruch Baum
  0 siblings, 1 reply; 18+ messages in thread
From: Lars Ingebrigtsen @ 2020-09-30 23:55 UTC (permalink / raw)
  To: Boruch Baum; +Cc: Glenn Morris, Michael Albinus, 43714

Boruch Baum <boruch_baum@gmx.com> writes:

> Attached is a new version of the patch, with the 'save-match-data'
> restored. Thanks again, Glenn.

Now applied to Emacs 28 (with some changes).  As far as I can see, it's
just a refactoring of auto-revert-buffers?  Normally, we don't do pure
refactorings like this (because it makes code archaeology more
difficult), but this one seemed pretty nice.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#43714: 28.1: auto-revert code improvements [PATCH]
  2020-09-30 23:55         ` Lars Ingebrigtsen
@ 2020-10-01  5:42           ` Boruch Baum
  0 siblings, 0 replies; 18+ messages in thread
From: Boruch Baum @ 2020-10-01  5:42 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Glenn Morris, Michael Albinus, 43714

On 2020-10-01 01:55, Lars Ingebrigtsen wrote:
> Boruch Baum <boruch_baum@gmx.com> writes:
>
> Now applied to Emacs 28 (with some changes).  As far as I can see, it's
> just a refactoring of auto-revert-buffers?

Hope is, yes.

>  Normally, we don't do pure refactorings like this (because it makes
> code archaeology more difficult), but this one seemed pretty nice.

Thanks. I suspect that it may turn out to come in handy for bug #43412
'autorevert-only-if-visible', which will be a case of performing an
instant auto-revert on a single buffer (or small set of buffers) without
using a timer.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0





^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2020-10-01  5:42 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30  6:26 bug#43714: 28.1: auto-revert code improvements [PATCH] Boruch Baum
2020-09-30 13:48 ` Lars Ingebrigtsen
2020-09-30 14:25   ` Eli Zaretskii
2020-09-30 15:28     ` Boruch Baum
2020-09-30 16:03       ` Eli Zaretskii
2020-09-30 16:59         ` Boruch Baum
2020-09-30 17:01           ` Lars Ingebrigtsen
2020-09-30 17:13             ` Boruch Baum
2020-09-30 17:21     ` Michael Albinus
2020-09-30 15:09   ` Boruch Baum
2020-09-30 15:12     ` Lars Ingebrigtsen
2020-09-30 17:41     ` Glenn Morris
2020-09-30 18:33       ` Boruch Baum
2020-09-30 23:55         ` Lars Ingebrigtsen
2020-10-01  5:42           ` Boruch Baum
2020-09-30 16:51 ` bug#43714: [boruch_baum@gmx.com: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]] Boruch Baum
2020-09-30 16:54   ` Lars Ingebrigtsen
2020-09-30 17:10     ` Boruch Baum

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).