bug#22557: 25.0.90; Saving a buffer under auto-revert turns off file notifications

bug#22557: 25.0.90; Saving a buffer under auto-revert turns off file notifications

[Eli Zaretskii]

  emacs -Q
  C-x C-f SOME-FILE RET
  M-x auto-revert-mode RET
  M-: auto-revert-use-notify RET => t

Modify the buffer and save it with "C-x C-s".  Then:

  M-: auto-revert-use-notify RET => nil

This is on MS-Windows; the same recipe on GNU/Linux with inotify
doesn't have this problem.

Looking under the hood reveals that filenotify.el sends a 'stopped'
notification when Emacs renames SOME-FILE to SOME-FILE~, to back it
up.  In response, auto-revert-mode turns off file notifications and
falls back to polling.

It looks like the reason is this condition in filenotify.el:

        ;; Check for stopped.
        (setq
         stopped
         (or
          stopped
          (and
           (memq action '(deleted renamed))
           (= (length (cdr registered)) 1)
           (or
            (string-equal
             (file-name-nondirectory file)
	     (file-name-nondirectory (car registered)))
            (string-equal
             (file-name-nondirectory file)
             (car (cadr registered)))))))

The second call to string-equal is the culprit: the file names match,
so 'stopped' gets a non-nil value.

I don't understand the logic of this: at least with w32notify, when a
file is in auto-revert-mode, we watch the entire parent directory of
that file, so there's no need to send the 'stopped' event when the
file is renamed or deleted, only when the parent directory is deleted.

This second call to string-equal was added as part of kqueue
back-port, but it affects all the backends, and there's no comment or
anything in the log message that explains the rationale for this part
of the change, only a laconic "Add kqueue support."

What is the reason for this test, and should it perhaps be limited to
kqueue?



In GNU Emacs 25.0.90.2 (i686-pc-mingw32)
 of 2016-01-31 built on HOME-C4E4A596F7
Windowing system distributor 'Microsoft Corp.', version 5.1.2600
Configured using:
 'configure --prefix=/d/usr --enable-checking=yes,glyphs
 --with-wide-int --with-modules 'CFLAGS=-Og -gdwarf-4 -g3''

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND NOTIFY ACL GNUTLS LIBXML2 ZLIB
TOOLKIT_SCROLL_BARS MODULES

Important settings:
  value of $LANG: ENU
  locale-coding-system: cp1255

Major mode: Mail

Minor modes in effect:
  shell-dirtrack-mode: t
  flyspell-mode: t
  diff-auto-refine-mode: t
  desktop-save-mode: t
  save-place-mode: t
  show-paren-mode: t
  display-time-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  temp-buffer-resize-mode: t
  line-number-mode: t
  auto-fill-function: mail-mode-auto-fill
  abbrev-mode: t

Recent messages:
Mark saved where search started [2 times]
Mark set
Mark saved where search started
Mark set
Saving file d:/gnu/git/emacs/branch/lisp/filenotify.el...
Wrote d:/gnu/git/emacs/branch/lisp/filenotify.el
Mark set
Undo!
Saving file d:/gnu/git/emacs/branch/lisp/filenotify.el...
Wrote d:/gnu/git/emacs/branch/lisp/filenotify.el

Load-path shadows:
d:/usr/share/emacs/site-lisp/soap-inspect hides d:/usr/share/emacs/25.0.90/lisp/net/soap-inspect
d:/usr/share/emacs/site-lisp/soap-client hides d:/usr/share/emacs/25.0.90/lisp/net/soap-client

Features:
(descr-text iso-transl ebuff-menu pp shadow emacsbug ruby-mode smie
shell character-fold misearch multi-isearch rmailout url-util
url-parse url-vars rfc2104 network-stream nsm starttls tls gnutls
mail-extr smtpmail auth-source eieio eieio-core cl-macs password-cache
dabbrev mailalias sendmail shr-color color shr dom subr-x browse-url
cl-seq conf-mode arc-mode archive-mode vc-bzr org-element org-rmail
org-mhe org-irc org-info org-gnus org-docview doc-view image-mode
org-bibtex bibtex org-bbdb org-w3m org advice org-macro org-footnote
org-pcomplete pcomplete org-list org-faces org-entities org-version
ob-emacs-lisp ob ob-tangle ob-ref ob-lob ob-table ob-exp org-src
ob-keys ob-comint comint ansi-color ring ob-core ob-eval org-compat
org-macs org-loaddefs find-func cal-menu calendar cal-loaddefs
bat-mode make-mode parse-time vc-cvs generic vc-svn texinfo jka-compr
noutline outline bug-reference flyspell add-log info vc vc-dispatcher
vc-git diff-mode easy-mmode map seq byte-opt gv bytecomp byte-compile
cconv cl-extra qp rmailsum rmailmm message dired-x dired format-spec
rfc822 mml mml-sec epg epg-config gnus-util mm-decode mm-bodies
mm-encode mailabbrev gmm-utils mailheader mail-parse rfc2231 rmail
rfc2047 rfc2045 ietf-drums mm-util help-fns help-mode mail-prsvr
mail-utils desktop frameset server filecache mairix cus-edit cus-start
cus-load wid-edit saveplace midnight ispell generic-x cc-mode cc-fonts
easymenu cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine
cc-vars cc-defs cl-loaddefs pcase cl-lib paren battery time time-date
mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel dos-w32 ls-lisp disp-table w32-win w32-vars
term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list newcomment elisp-mode lisp-mode prog-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core frame cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese
eucjp-ms cp51932 hebrew greek romanian slovak czech european ethiopic
indian cyrillic chinese charscript case-table epa-hook jka-cmpr-hook
help simple abbrev minibuffer cl-preloaded nadvice loaddefs button
faces cus-face macroexp files text-properties overlay sha1 md5 base64
format env code-pages mule custom widget hashtable-print-readable
backquote w32notify w32 multi-tty make-network-process emacs)

Memory information:
((conses 16 1766138 246501)
 (symbols 56 51658 10)
 (miscs 48 3618 4548)
 (strings 16 128344 31235)
 (string-bytes 1 3335828)
 (vectors 16 42573)
 (vector-slots 8 1729654 252618)
 (floats 8 498 992)
 (intervals 40 338547 2583)
 (buffers 856 223))

bug#22557: 25.0.90; Saving a buffer under auto-revert turns off file notifications

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>   emacs -Q
>   C-x C-f SOME-FILE RET
>   M-x auto-revert-mode RET
>   M-: auto-revert-use-notify RET => t
>
> Modify the buffer and save it with "C-x C-s".  Then:
>
>   M-: auto-revert-use-notify RET => nil
>
> This is on MS-Windows; the same recipe on GNU/Linux with inotify
> doesn't have this problem.

For me the recipe fails in both cases. This is due to
`backup-by-copying' being nil; in case of t there's no problem.

But it shall work independent of this setting. This calls for a new test
case in test/automated/file-notify-tests.el. Will add it.

> Looking under the hood reveals that filenotify.el sends a 'stopped'
> notification when Emacs renames SOME-FILE to SOME-FILE~, to back it
> up.  In response, auto-revert-mode turns off file notifications and
> falls back to polling.
>
> It looks like the reason is this condition in filenotify.el:
>
>         ;; Check for stopped.
>         (setq
>          stopped
>          (or
>           stopped
>           (and
>            (memq action '(deleted renamed))
>            (= (length (cdr registered)) 1)
>            (or
>             (string-equal
>              (file-name-nondirectory file)
> 	     (file-name-nondirectory (car registered)))
>             (string-equal
>              (file-name-nondirectory file)
>              (car (cadr registered)))))))
>
> The second call to string-equal is the culprit: the file names match,
> so 'stopped' gets a non-nil value.

`stopped' shall be raised only in case of `deleted'. For `renamed', it
seems to be wrong. Will check.

> I don't understand the logic of this: at least with w32notify, when a
> file is in auto-revert-mode, we watch the entire parent directory of
> that file, so there's no need to send the 'stopped' event when the
> file is renamed or deleted, only when the parent directory is deleted.

In `auto-revert-notify-add-watch', only the file named by
`buffer-file-name' is watched. autorevert.el does not check, what the
underlying library of filenotify.el is able to do, watching single files
or whole directories. And as said, a renaming of a file should not turn
file notifications off. I will try to fix this.

> This second call to string-equal was added as part of kqueue
> back-port, but it affects all the backends, and there's no comment or
> anything in the log message that explains the rationale for this part
> of the change, only a laconic "Add kqueue support."

The changes were introduced in the scratch/kqueue branch. Later merged
to the master branch, later backported to the emacs-25 branch. Likely,
commit messages were lost this way.

I really hate it that we have no ChangeLog anymore, maintained by the
developers.

> What is the reason for this test, and should it perhaps be limited to
> kqueue?

Nope. If a file is supervised, and this is deleted on purpose, it
shouldn't come back under supervision if a file with the same name
appears later on.

There is the case of renaming files during backup, which could be seen
internally as delete+create file notification actions. This must be
handled, I agree.

Best regards, Michael.

bug#22557: 25.0.90; Saving a buffer under auto-revert turns off file notifications

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: 22557@debbugs.gnu.org
> Date: Fri, 05 Feb 2016 13:24:01 +0100
> 
> > I don't understand the logic of this: at least with w32notify, when a
> > file is in auto-revert-mode, we watch the entire parent directory of
> > that file, so there's no need to send the 'stopped' event when the
> > file is renamed or deleted, only when the parent directory is deleted.
> 
> In `auto-revert-notify-add-watch', only the file named by
> `buffer-file-name' is watched. autorevert.el does not check, what the
> underlying library of filenotify.el is able to do, watching single files
> or whole directories.

I don't think the problem is in autorevert.el.  The problem is in
filenotify.el, which is where 'stopped' is generated.  Filenotify does
know what it watches.

> And as said, a renaming of a file should not turn file notifications
> off. I will try to fix this.

Neither should deleting a file, IMO.  Otherwise you will lose during
saving, since that's what Emacs does then.

> I really hate it that we have no ChangeLog anymore, maintained by the
> developers.

Maybe we should re-introduce them.  git-merge-changelog makes merging
them all but free of problems, so perhaps we are paying a price that's
too heavy for eliminating ChangeLog.

bug#22557: 25.0.90; Saving a buffer under auto-revert turns off file notifications

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> And as said, a renaming of a file should not turn file notifications
>> off. I will try to fix this.
>
> Neither should deleting a file, IMO.  Otherwise you will lose during
> saving, since that's what Emacs does then.

That's what I meant. Deleting a file by intention shall take it off of
file notification. Deleting a file temporarily, due to backup,
shouldn't. I'll see how I could manage it.

Fortunately, a `deleted'+`created' sequence is already handled as
`renamed' in filenotify.el. I'll check how robust this is with all
supported backends.

>> I really hate it that we have no ChangeLog anymore, maintained by the
>> developers.
>
> Maybe we should re-introduce them.  git-merge-changelog makes merging
> them all but free of problems, so perhaps we are paying a price that's
> too heavy for eliminating ChangeLog.

+++++++++1

Best regards, Michael.

bug#22557: 25.0.90; Saving a buffer under auto-revert turns off file notifications

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't think the problem is in autorevert.el.  The problem is in
> filenotify.el, which is where 'stopped' is generated.  Filenotify does
> know what it watches.

I've committed a patch to the emacs-25 branch, which shall fix
this. There is also a new test file-notify-test07-backup for this
problems. W/o the fix in filenotify.el, this test failed. Now it
succeeds for me.

Could you pls cross-check it with your scenario on w32?

Best regards, Michael.

bug#22557: 25.0.90; Saving a buffer under auto-revert turns off file notifications

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: 22557@debbugs.gnu.org
> Date: Sun, 07 Feb 2016 19:35:08 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I don't think the problem is in autorevert.el.  The problem is in
> > filenotify.el, which is where 'stopped' is generated.  Filenotify does
> > know what it watches.
> 
> I've committed a patch to the emacs-25 branch, which shall fix
> this.

It does, thanks.

> There is also a new test file-notify-test07-backup for this
> problems. W/o the fix in filenotify.el, this test failed. Now it
> succeeds for me.

It fails for me on w32, I need 2 changed events, not one.  If this is
surprising, I can investigate more.

bug#22557: 25.0.90; Saving a buffer under auto-revert turns off file notifications

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> I've committed a patch to the emacs-25 branch, which shall fix
>> this.
>
> It does, thanks.

So I'm closing the bug.

>> There is also a new test file-notify-test07-backup for this
>> problems. W/o the fix in filenotify.el, this test failed. Now it
>> succeeds for me.
>
> It fails for me on w32, I need 2 changed events, not one.  If this is
> surprising, I can investigate more.

As said, there are good chances that I'll have access to an MS Windows
machine next week; I'll run all tests there.

I have seen with inotify that I'll get a changed and then an
attribute-changed event; the attribute-changed event is suppressed in
the test.

On MS Windows, there are no attribute-changed events IIUC; they are sent
also as changed events. Therefore, you see two changed events. This is
also the case in other tests, see the comments in
file-notify-test02-events. I'll try to verify this, and adjust the
expected events for file-notify-test07-backup on MS Windows.

Best regards, Michael.

bug#22557: 25.0.90; Saving a buffer under auto-revert turns off file notifications

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: 22557-done@debbugs.gnu.org
> Date: Sun, 07 Feb 2016 20:56:37 +0100
> 
> > It fails for me on w32, I need 2 changed events, not one.  If this is
> > surprising, I can investigate more.
> 
> As said, there are good chances that I'll have access to an MS Windows
> machine next week; I'll run all tests there.
> 
> I have seen with inotify that I'll get a changed and then an
> attribute-changed event; the attribute-changed event is suppressed in
> the test.
> 
> On MS Windows, there are no attribute-changed events IIUC; they are sent
> also as changed events. Therefore, you see two changed events.

Yes, this was also my conclusion.

> This is also the case in other tests, see the comments in
> file-notify-test02-events. I'll try to verify this, and adjust the
> expected events for file-notify-test07-backup on MS Windows.

OK, thanks.  Let me know if you need any help in these matters.

bug#22557: 25.0.90; Saving a buffer under auto-revert turns off file notifications

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> This is also the case in other tests, see the comments in
>> file-notify-test02-events. I'll try to verify this, and adjust the
>> expected events for file-notify-test07-backup on MS Windows.
>
> OK, thanks.  Let me know if you need any help in these matters.

I've fixed this in the emacs-25 branch. Surprisingly, the remote case
shows also two `changed' events when backing up by copying. Maybe this
is a sign that Tramp's implementation could be optimized, dunno. When
time permits I'll debug.

kqueue does not keep file notification when backing up by rename. That's
expected, because it cannot watch whole directories for changes. Maybe
I'll drop a short not in the manual.

Best regards, Michael.