bug#34847: 27.0.50; auto-revert-buffers occasionally selects a killed buffer

[Basil L. Contovounesios]

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

> Something like this. But the final `auto-revert-remove-current-buffer'
> needs the buffer to be removed as the current one. So it is a bit more
> complex.
>
> I came up with the following patch:
>
> *** /tmp/ediffWnT0dx  2019-03-24 15:30:02.022068542 +0100
> --- /home/albinus/src/emacs/lisp/autorevert.el        2019-03-24 
> 15:26:41.756960378 +0100
> ***************
> *** 343,352 ****
>
>   ;; Functions:
>
> ! (defun auto-revert-remove-current-buffer ()
>     "Remove dead buffer from `auto-revert-buffer-list'."
>     (setq auto-revert-buffer-list
> !         (delq (current-buffer) auto-revert-buffer-list)))
>
>   ;;;###autoload
>   (define-minor-mode auto-revert-mode
> --- 343,352 ----
>
>   ;; Functions:
>
> ! (defun auto-revert-remove-current-buffer (&optional buffer)
>     "Remove dead buffer from `auto-revert-buffer-list'."
>     (setq auto-revert-buffer-list
> !         (delq (or buffer (current-buffer)) auto-revert-buffer-list)))
>
>   ;;;###autoload
>   (define-minor-mode auto-revert-mode

Can you please update the docstring?  E.g.

  "Remove BUFFER from `auto-revert-buffer-list'.
BUFFER defaults to `current-buffer'."

> ***************
> *** 772,781 ****
>         (setq bufs (delq nil
>                          (mapcar
>                           (lambda (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
> --- 772,783 ----
>         (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

Indentation seems a bit off here.  Note that you can use 'when' instead
of 'and' to fit more easily within 80 columns, if you prefer.

Otherwise LGTM.  Can you please also make the following change while
you're at it?

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index bc7c616ecb..d736f0d31c 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -509,7 +509,7 @@ auto-revert-notify-rm-watch
           (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))
+    (remove-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch t))
   (setq auto-revert-notify-watch-descriptor nil
        auto-revert-notify-modified-p nil))
 

It seems to have been left over from a past change:

* lisp/autorevert.el: Use lexical-binding.  Fix hook usage.
  2015-05-22 00:03:12 -0400
  
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=9e41e0bc6a6fdb7a01841db62d8de42d67be534a

> Could you pls check whether the patch works for you with magit? (I
> don't use magit myself)

Works for me; I hope to see it land soon.  See also my other message for
a related but more subtle problem: https://debbugs.gnu.org/34847#16

Thanks,

-- 
Basil