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

Previous Next

Full log

View this message in rfc822 format

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 35418 <at> debbugs.gnu.org
Subject: 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.

Previous Next

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