Re: master e9e807e: Don't remove notify descriptor that is already gone

["Mattias Engdegård" <mattiase@acm.org>]

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

16 apr. 2019 kl. 09.04 skrev Michael Albinus <michael.albinus@gmx.de>:

> I haven't tested thoroughly yet, but wouldn't it suffice if in
> auto-revert-notify-rm-watch there is just the test
> 
> (when (file-notify-valid-p auto-revert-notify-watch-descriptor)
> 
> instead of
> 
> (when auto-revert-notify-watch-descriptor

Thanks for reading my change. It is a fair question!

First of all, the descriptor wouldn't then be removed from `auto-revert-notify-watch-descriptor-hash-list' since that part is also guarded by the condition, but that's just a matter of rearranging code.

(Not only is `auto-revert-notify-watch-descriptor-hash-list' a mouthful, it is a bit misleading. It maps descriptors to lists of buffers. How about `auto-revert--buffers-by-watch-descriptor'?)

I'm not necessarily enamoured with `file-notify-valid-p'. It just tells whether there is, right now, a descriptor that looks like the argument, in use by someone, somewhere. These descriptors are reused, making the predicate dangerous to rely on, or requires brittle code that just knows that no reuse has occurred.

Slightly more robust would be to stop reusing descriptors: either made mutable, so that they can be invalidated, or made unique by using a counter. However, the basic design is still somewhat dubious: it tells us whether the descriptor is valid, but that just raises the question: why do we even have to ask? Correct code should understand its own invariants.

Now that you `mentioned auto-revert-notify-rm-watch', does it strike you as odd the way it does

    (maphash
     (lambda (key value)
       (when (equal key some-key)
         do-something))
     some-hashtable)

instead of using the hash table directly? Suggested patch to fix this attached.

(For that matter, the documentation doesn't say what mutation is permitted inside `maphash'. I can guess from the source.)

By the way, why don't we give each buffer in auto-revert-mode a unique descriptor, so that the table just maps descriptors to buffers, instead of to lists of buffers? It would simplify the code in many places, and it cannot be that common to have multiple buffers for the same file that it warrants the descriptor-sharing optimisation.

[-- Attachment #2: 0001-autorevert.el-auto-revert-notify-rm-watch-Simplify.patch --]
[-- Type: application/octet-stream, Size: 2009 bytes --]

From 00733fac960526e25e5f7e5d68be48d042d28315 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 16 Apr 2019 15:13:57 +0200
Subject: [PATCH] * autorevert.el (auto-revert-notify-rm-watch): Simplify.

---
 lisp/autorevert.el | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 1dc2f0eafd..cf9dc53f85 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -501,19 +501,17 @@ will use an up-to-date value of `auto-revert-interval'"
   "Disable file notification for current buffer's associated file.
 If REMOVE-DESCRIPTOR is non-nil, remove the corresponding notification
 descriptor; otherwise assume that it has already been removed."
-  (when auto-revert-notify-watch-descriptor
-    (maphash
-     (lambda (key value)
-       (when (equal key auto-revert-notify-watch-descriptor)
-	 (setq value (delete (current-buffer) value))
-	 (if value
-	     (puthash key value auto-revert-notify-watch-descriptor-hash-list)
-	   (remhash key auto-revert-notify-watch-descriptor-hash-list)
-           (when remove-descriptor
-	     (ignore-errors
-	       (file-notify-rm-watch auto-revert-notify-watch-descriptor))))))
-     auto-revert-notify-watch-descriptor-hash-list)
-    (remove-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch-callback t))
+  (let ((desc auto-revert-notify-watch-descriptor)
+        (table auto-revert-notify-watch-descriptor-hash-list))
+    (when desc
+      (let ((buffers (delq (current-buffer) (gethash desc table))))
+        (if buffers
+            (puthash desc buffers table)
+          (remhash desc table)))
+      (when remove-descriptor
+	(ignore-errors
+	  (file-notify-rm-watch desc)))
+      (remove-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch-callback t)))
   (setq auto-revert-notify-watch-descriptor nil
 	auto-revert-notify-modified-p nil))
 
-- 
2.20.1 (Apple Git-117)