[PATCH] autorevert.el -- revert fix for Windows platform

[PATCH] autorevert.el -- revert fix for Windows platform

[Jari Aalto]

For some reason the `verify-visited-file-modtime' test in
auto-revert-buffers is not enough to notice a file change under
Windows / native Emacs 21.3. Here is patch to compare the bufer size
against files size on disk. It cures the revert problem.

The patch is against CVS 2007-03-22

2007-03-22 Thu Jari Aalto <jari aalto A T cante net>

	* autorevert.el
	(auto-revert-size-equal-p): New function.
	(auto-revert-handler): Use `auto-revert-size-equal-p' to
	test file change as well.

=== modified file 'autorevert.el'
--- autorevert.el       2007-03-22 11:05:43 +0000
+++ autorevert.el       2007-03-22 11:24:06 +0000
@@ -302,7 +302,6 @@
     (auto-revert-buffers)
     (setq auto-revert-tail-mode nil)))

-
 ;;;###autoload
 (defun turn-on-auto-revert-mode ()
   "Turn on Auto-Revert Mode.
@@ -396,6 +395,11 @@
        (not (memq major-mode
                  global-auto-revert-ignore-modes)))))

+(defun auto-revert-size-equal-p ()
+  (let ((bsize (buffer-size))
+        (fsize (nth 7 (file-attributes (buffer-file-name)))))
+    (eq bsize fsize)))
+
 (defun auto-revert-handler ()
   "Revert current buffer, if appropriate.
 This is an internal function used by Auto-Revert Mode."
@@ -405,7 +409,9 @@
            (or (and buffer-file-name
                     (not (file-remote-p buffer-file-name))
                     (file-readable-p buffer-file-name)
-                    (not (verify-visited-file-modtime buffer)))
+                     (or
+                      (not (verify-visited-file-modtime buffer))
+                      (not (auto-revert-size-equal-p))))
                (and (or auto-revert-mode
                         global-auto-revert-non-file-buffers)
                     revert-buffer-function

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Chong Yidong]

Jari Aalto <jari.aalto@cante.net> writes:

> For some reason the `verify-visited-file-modtime' test in
> auto-revert-buffers is not enough to notice a file change under
> Windows / native Emacs 21.3. Here is patch to compare the bufer size
> against files size on disk. It cures the revert problem.

That's not foolproof, since changes need not alter the file size.

Could you try to find out why verify-visited-file-modtime doesn't work
on Windows?  You said that the problem exists on Emacs 21---does it
still exist under Emacs 22?

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Lennart Borgman (gmail)]

Chong Yidong wrote:
> Jari Aalto <jari.aalto@cante.net> writes:
> 
>> For some reason the `verify-visited-file-modtime' test in
>> auto-revert-buffers is not enough to notice a file change under
>> Windows / native Emacs 21.3. Here is patch to compare the bufer size
>> against files size on disk. It cures the revert problem.
> 
> That's not foolproof, since changes need not alter the file size.
> 
> Could you try to find out why verify-visited-file-modtime doesn't work
> on Windows?  You said that the problem exists on Emacs 21---does it
> still exist under Emacs 22?


I did a test on latest CVS Emacs on w32:

- opened a file in Emacs.
- (verify-visited-file-modetime (current-buffer)) => t
- did "touch file" (outside of Emacs)

Now I got

- (verify-visited-file-modetime (current-buffer)) => nil

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Jason Rumney]

Lennart Borgman (gmail) wrote:
> I did a test on latest CVS Emacs on w32:
>
> - opened a file in Emacs.
> - (verify-visited-file-modetime (current-buffer)) => t
> - did "touch file" (outside of Emacs)
>
> Now I got
>
> - (verify-visited-file-modetime (current-buffer)) => nil

Looking at the code, this will probably fail on network drives that are 
accessed by UNC paths. The stat information is populated with dummy data 
in that case. I'm sure there is a reason for this, but maybe it will be 
possible to at least get the modification timestamp right.

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Eli Zaretskii]

> Date: Thu, 22 Mar 2007 17:49:21 +0000
> From: Jason Rumney <jasonr@gnu.org>
> Cc: Chong Yidong <cyd@stupidchicken.com>, Jari Aalto <jari.aalto@cante.net>,
> 	emacs-devel@gnu.org
> 
> Looking at the code, this will probably fail on network drives that are 
> accessed by UNC paths.

Jari, can you confirm that this problem happens for you only with UNC
style file names?

If not, please provide a self-contained test case that we could try to
see what is going wrong.

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Jari Aalto]

Eli Zaretskii <eliz@gnu.org> writes:
> > Looking at the code, this will probably fail on network drives
> > that are accessed by UNC paths.
> 
> Jari, can you confirm that this problem happens for you only with
> UNC style file names?

This happens in local PC disk. No UNC involved.

> If not, please provide a self-contained test case that we could try to
> see what is going wrong.

Have some process that keeps a file open while the data is being
written to it. The file is never closed (only if process terminated).
This is happens e.g with firewalls(*) that do not close the file in
between log writes.

Jari

(*) In Windows:
1. Download Jetico Personal Firewall 2.x, install, configure, start 
   http://www.jetico.com/ (Free)
2. Open Emacs, M-x global-auto-revert-mode
3. Load Jetico's firewall log C-x C-f path/to/jpfsrv.log

Watch the file grow on disk (Windows Eplorer), but no revert happens
in Emacs buffer.

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Jason Rumney]

Jari Aalto wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
>   
>>> Looking at the code, this will probably fail on network drives
>>> that are accessed by UNC paths.
>>>       
>> Jari, can you confirm that this problem happens for you only with
>> UNC style file names?
>>     
>
> This happens in local PC disk. No UNC involved.
>   

I went back and looked at the code I thought looked suspicious, and it 
turns out it only applies to the root of a UNC shared drive, not normal 
files on shared drives.

I will try to find out if other methods can be used that give more 
reliable modification timestamps.

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Jason Rumney]

It seems to me that auto-revert-tail-mode already exists to deal with 
this specific use case.

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[martin rudalics]

 > It seems to me that auto-revert-tail-mode already exists to deal with
 > this specific use case.

Doesn't this

(defun auto-revert-tail-handler ()
   (let ((size (nth 7 (file-attributes buffer-file-name)))
     ...
     (when (> size auto-revert-tail-pos)
           ...
	  (insert-file-contents file nil auto-revert-tail-pos size)))
       (undo-boundary)
       (setq auto-revert-tail-pos size)
       ...

suffer from the buffer-size file-size dichotomy as well?  Or are we on
the safe side here?

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Jason Rumney]

martin rudalics wrote:

> Doesn't this
>
> (defun auto-revert-tail-handler ()
>   (let ((size (nth 7 (file-attributes buffer-file-name)))
>     ...
>     (when (> size auto-revert-tail-pos)
>           ...
>       (insert-file-contents file nil auto-revert-tail-pos size)))
>       (undo-boundary)
>       (setq auto-revert-tail-pos size)
>       ...
>
> suffer from the buffer-size file-size dichotomy as well?  Or are we on
> the safe side here?

No. buffer-size is not used, both auto-revert-tail-pos and size are 
measuring bytes in the file, not characters in the buffer.

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Johan Bockgård]

Jason Rumney <jasonr@gnu.org> writes:

> No. buffer-size is not used, both auto-revert-tail-pos and size are
> measuring bytes in the file, not characters in the buffer.

There are however two occurrences of

      (set (make-local-variable 'auto-revert-tail-pos)
            (save-restriction (widen) (1- (point-max))))

in autorevert.el.

-- 
Johan Bockgård

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Jason Rumney]

Johan Bockgård wrote:
> Jason Rumney <jasonr@gnu.org> writes:
>
>   
>> No. buffer-size is not used, both auto-revert-tail-pos and size are
>> measuring bytes in the file, not characters in the buffer.
>>     
>
> There are however two occurrences of
>
>       (set (make-local-variable 'auto-revert-tail-pos)
>             (save-restriction (widen) (1- (point-max))))
>
> in autorevert.el.
>   

I think this patch should fix that. Though maybe we need to handle a 
user trying to use this mode on a buffer without an associated file.


*** autorevert.el    26 Jan 2007 22:30:51 +0000    1.56
--- autorevert.el    24 Mar 2007 11:42:53 +0000   
***************
*** 278,284 ****
  (add-hook 'find-file-hook
        (lambda ()
          (set (make-local-variable 'auto-revert-tail-pos)
!          (save-restriction (widen) (1- (point-max))))))
 
  ;; Functions:
 
--- 278,284 ----
  (add-hook 'find-file-hook
        (lambda ()
          (set (make-local-variable 'auto-revert-tail-pos)
!          (nth 7 (file-attributes buffer-file-name)))))
 
  ;; Functions:
 
***************
*** 341,347 ****
        (add-hook 'before-save-hook (lambda () (auto-revert-tail-mode 
0)) nil t)
        (or (local-variable-p 'auto-revert-tail-pos) ; don't lose prior 
position
        (set (make-local-variable 'auto-revert-tail-pos)
!            (save-restriction (widen) (1- (point-max)))))
        ;; let auto-revert-mode set up the mechanism for us if it isn't 
already
        (or auto-revert-mode
        (let ((auto-revert-tail-mode t))
--- 341,347 ----
        (add-hook 'before-save-hook (lambda () (auto-revert-tail-mode 
0)) nil t)
        (or (local-variable-p 'auto-revert-tail-pos) ; don't lose prior 
position
        (set (make-local-variable 'auto-revert-tail-pos)
!            (nth 7 (file-attributes buffer-file-name))))
        ;; let auto-revert-mode set up the mechanism for us if it isn't 
already
        (or auto-revert-mode
        (let ((auto-revert-tail-mode t))

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[martin rudalics]

 > No. buffer-size is not used, both auto-revert-tail-pos and size are
 > measuring bytes in the file, not characters in the buffer.

But `auto-revert-tail-mode' has this:

       (or (local-variable-p 'auto-revert-tail-pos) ; don't lose prior position
	  (set (make-local-variable 'auto-revert-tail-pos)
	       (save-restriction (widen) (1- (point-max)))))

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Eli Zaretskii]

> Date: Fri, 23 Mar 2007 20:36:45 +0000
> From: Jason Rumney <jasonr@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> I will try to find out if other methods can be used that give more 
> reliable modification timestamps.

My limited testing seems to indicate that, for a newly created file,
as long as it is not closed by the application that writes to it, all
3 of the file's times are identical, set to the time the file was
created, and do not change.  Only when the file is closed, the
last-write and last-access times are updated.

This is on Windows XP with NTFS volumes.

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Kim F. Storm]

IIRC, on Windows 98 with VFAT, the size of a newly created file
was 0 until the file was closed.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Eli Zaretskii]

> Cc: Jason Rumney <jasonr@gnu.org>,  emacs-devel@gnu.org,  jari.aalto@cante.net
> From: storm@cua.dk (Kim F. Storm)
> Date: Sat, 24 Mar 2007 23:56:31 +0100
> 
> IIRC, on Windows 98 with VFAT, the size of a newly created file
> was 0 until the file was closed.

Yes, I think so.  I don't have an easy access to Windows 9x anymore,
but I can test on a FAT32 volume if necessary.

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Eli Zaretskii]

> From: Jari Aalto <jari.aalto@cante.net>
> Date: 23 Mar 2007 19:31:41 +0200
> 
> Have some process that keeps a file open while the data is being
> written to it. The file is never closed (only if process terminated).
> This is happens e.g with firewalls(*) that do not close the file in
> between log writes.
> 
> Jari
> 
> (*) In Windows:
> 1. Download Jetico Personal Firewall 2.x, install, configure, start 
>    http://www.jetico.com/ (Free)
> 2. Open Emacs, M-x global-auto-revert-mode
> 3. Load Jetico's firewall log C-x C-f path/to/jpfsrv.log
> 
> Watch the file grow on disk (Windows Eplorer), but no revert happens
> in Emacs buffer.

In what ways is this specific to MS-Windows?

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Jari Aalto]

Chong Yidong <cyd@stupidchicken.com> writes:

> Jari Aalto <jari.aalto@cante.net> writes:
> 
> > For some reason the `verify-visited-file-modtime' test in
> > auto-revert-buffers is not enough to notice a file change under
> > Windows / native Emacs 21.3. Here is patch to compare the bufer size
> > against files size on disk. It cures the revert problem.
> 
> That's not foolproof, since changes need not alter the file size.

True. Do you know any use case of such a situation?

> Could you try to find out why verify-visited-file-modtime doesn't
> work on Windows? You said that the problem exists on Emacs 21---does
> it still exist under Emacs 22?

I could test more if you point me to a location where I can download
native windows version of 22.x

I downloaded only the lisp code from CVS to submit a patch that would apply
cleanly.

Jari

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Eli Zaretskii]

> From: Jari Aalto <jari.aalto@cante.net>
> Date: 23 Mar 2007 09:03:20 +0200
> 
> > That's not foolproof, since changes need not alter the file size.
> 
> True. Do you know any use case of such a situation?

Yes: replace one characters with another, and make no other changes.

> > Could you try to find out why verify-visited-file-modtime doesn't
> > work on Windows? You said that the problem exists on Emacs 21---does
> > it still exist under Emacs 22?
> 
> I could test more if you point me to a location where I can download
> native windows version of 22.x

Here's one place:

  http://ourcomments.org/Emacs/EmacsW32SetupUtilities.html

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Kim F. Storm]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jari Aalto <jari.aalto@cante.net>
>> Date: 23 Mar 2007 09:03:20 +0200
>> 
>> > That's not foolproof, since changes need not alter the file size.
>> 
>> True. Do you know any use case of such a situation?
>
> Yes: replace one characters with another, and make no other changes.

How often does an external file change in that way?

FWIW, auto-revert mode is often used to track changes to files that do
change size, e.g. log file, compilation output, etc, and in such cases,
Jari's patch is a significant improvement, even if it is not a general
solution to the problem.

For the release, I'd say Jari's patch is good enough!

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Jason Rumney]

Kim F. Storm wrote:
> For the release, I'd say Jari's patch is good enough!
>   

I'm not sure, do we really expect buffer-size to be the same as the 
number of bytes in the file, potentially in the presence of multibyte 
characters? This change is quite likely to break things if that 
assumption is not correct.

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Luc Teirlinck]

Jason Rumney wrote:

   I'm not sure, do we really expect buffer-size to be the same as the 
   number of bytes in the file, potentially in the presence of multibyte 
   characters?

Of course not and I actually got files for which the two are very
different.

   This change is quite likely to break things if that 
   assumption is not correct.

Could cause 100% CPU usage and continuous overshouting of other
minibuffer messages with revert messages, making Emacs completely
unusable.

On top of that, if Emacs has trouble checking when a file has changed
on disk on MS Windows, then how is this a problem that is specific to
Auto Revert Mode?  What about any other part of Emacs that needs to
know this?

Sincerely,

Luc Teirlinck.

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Eli Zaretskii]

> Date: Fri, 23 Mar 2007 13:48:01 -0500 (CDT)
> From: Luc Teirlinck <teirllm@dms.auburn.edu>
> Cc: eliz@gnu.org, jari.aalto@cante.net, emacs-devel@gnu.org, storm@cua.dk
> 
> On top of that, if Emacs has trouble checking when a file has changed
> on disk on MS Windows, then how is this a problem that is specific to
> Auto Revert Mode?

You are right: the proper change to fix this is in `stat' and other
related functions used by Emacs primitives to check for file changes.

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Stefan Monnier]

>> For the release, I'd say Jari's patch is good enough!

> I'm not sure, do we really expect buffer-size to be the same as the number
> of bytes in the file, potentially in the presence of multibyte characters?
> This change is quite likely to break things if that assumption is
> not correct.

Duh, I didn't even look at the patch.  So no clearly the patch is
unacceptable as is.  But the principle of checking whether the file changes
size is the thing I agreed to.


        Stefan

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Kim F. Storm]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> For the release, I'd say Jari's patch is good enough!
>
>> I'm not sure, do we really expect buffer-size to be the same as the number
>> of bytes in the file, potentially in the presence of multibyte characters?
>> This change is quite likely to break things if that assumption is
>> not correct.

>
> Duh, I didn't even look at the patch.  So no clearly the patch is
> unacceptable as is.  But the principle of checking whether the file changes
> size is the thing I agreed to.

Me too.

I didn't think deeply about the implications of using (buffer-size)
directly, but what about using (position-bytes (buffer-size)) instead?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Luc Teirlinck]

Kim Storm wrote:

   I didn't think deeply about the implications of using (buffer-size)
   directly, but what about using (position-bytes (buffer-size)) instead?

Just as wrong, and again, I have actually files for which
(position-bytes (buffer-size)) is very different from the size of the
file on disk.  The internal contents of an Emacs buffer are _not_ the
same as the contents of the file on disk, unless `find-file-literally'
was used (see for instance `(elisp)Coding Systems').

Sincerely,

Luc Teirlinck.
 LocalWords:  elisp

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Kim F. Storm]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> Kim Storm wrote:
>
>    I didn't think deeply about the implications of using (buffer-size)
>    directly, but what about using (position-bytes (buffer-size)) instead?
>
> Just as wrong, and again, I have actually files for which
> (position-bytes (buffer-size)) is very different from the size of the
> file on disk.  The internal contents of an Emacs buffer are _not_ the
> same as the contents of the file on disk, unless `find-file-literally'
> was used (see for instance `(elisp)Coding Systems').

I see, thanks!

Still, I don't see why auto-revert-mode cannot just track changes
to the file size, just like auto-revert-tail-mode does (i.e. store
the previous file size in buffer-local var auto-revert-tail-pos).

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Stefan Monnier]

> Still, I don't see why auto-revert-mode cannot just track changes
> to the file size, just like auto-revert-tail-mode does (i.e. store
> the previous file size in buffer-local var auto-revert-tail-pos).

I'd argue that we should replace buffer-file-number with
buffer-file-attributes.


        Stefan

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Stefan Monnier]

> For the release, I'd say Jari's patch is good enough!

Agreed.


        Stefan

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Eli Zaretskii]

> Cc: Jari Aalto <jari.aalto@cante.net>,  emacs-devel@gnu.org
> From: storm@cua.dk (Kim F. Storm)
> Date: Fri, 23 Mar 2007 16:47:42 +0100
> 
> For the release, I'd say Jari's patch is good enough!

I'm not convinced that a better patch is impossible.

Jari's patch tries to work around a problem that we do not yet
understand.  When we do understand it, then we could reason whether
his patch is the best solution we can reasonably provide.  Until then,
arguing about the patch is just hand-waiving without any real basis.

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Eli Zaretskii]

> From: Chong Yidong <cyd@stupidchicken.com>
> Date: Thu, 22 Mar 2007 11:00:32 -0400
> Cc: emacs-devel@gnu.org
> 
> Jari Aalto <jari.aalto@cante.net> writes:
> 
> > For some reason the `verify-visited-file-modtime' test in
> > auto-revert-buffers is not enough to notice a file change under
> > Windows / native Emacs 21.3. Here is patch to compare the bufer size
> > against files size on disk. It cures the revert problem.
> 
> That's not foolproof, since changes need not alter the file size.

Right, it's certainly not a good idea to install such a kludgey
solution.

Re: [PATCH] autorevert.el -- revert fix for Windows platform

[Richard Stallman]

Is this change needed in the Emacs in CVS?