File watch support in autorevert.el

File watch support in autorevert.el

[Michael Albinus]

Hi,

As a proof of concept, I have installed a patch in autorevert.el
implementing file watches.  This shall work for `auto-revert-mode' and
`global-auto-revert-mode' for buffers visiting files.
`auto-revert-tail-mode' is not supported (yet).

The implementation uses `inotify-*-watch' functions. I have also added
the calls for `w32-*-watch' functions, but this is untested.

I'll add the documentation and a NEWS entry once this feature has been
proven useful.

Any feedback is welcome.

Best regards, Michael.

Re: File watch support in autorevert.el

[Stefan Monnier]

> As a proof of concept, I have installed a patch in autorevert.el
> implementing file watches.  This shall work for `auto-revert-mode' and
> `global-auto-revert-mode' for buffers visiting files.
> `auto-revert-tail-mode' is not supported (yet).

Thanks, that sounds like a very good start.  But please make it
optional, for those users who want to use auto-revert-mode on files
accessed over NFS and other protocols where inotify doesn't work.


        Stefan

Re: File watch support in autorevert.el

[Lennart Borgman]

On Thu, Jan 10, 2013 at 6:11 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> As a proof of concept, I have installed a patch in autorevert.el
>> implementing file watches.  This shall work for `auto-revert-mode' and
>> `global-auto-revert-mode' for buffers visiting files.
>> `auto-revert-tail-mode' is not supported (yet).
>
> Thanks, that sounds like a very good start.  But please make it
> optional, for those users who want to use auto-revert-mode on files
> accessed over NFS and other protocols where inotify doesn't work.

Nice. One of the first thing I suggested long ago while trying to get
file watching into Emacs ;-)

Re: File watch support in autorevert.el

[Michael Albinus]

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

>> As a proof of concept, I have installed a patch in autorevert.el
>> implementing file watches.  This shall work for `auto-revert-mode' and
>> `global-auto-revert-mode' for buffers visiting files.
>> `auto-revert-tail-mode' is not supported (yet).
>
> Thanks, that sounds like a very good start.  But please make it
> optional, for those users who want to use auto-revert-mode on files
> accessed over NFS and other protocols where inotify doesn't work.

Done. There's a now user option `auto-revert-use-notify', which can be
set to nil in order to suppress file watches. Maybe it would be more
useful, if autorevert would decide per file, whether to trust on file
watches, or not. For example, one could check whether *-add-watch
returns a nil watch descriptor. In this case, there shall be a fallback
to the current implementation. Will see.

After that, I will check what to do with `auto-revert-tail-mode'.

And there is still the open point what to do with the timer. The current
implementation uses the file watches to mark buffers which need to be
reverted. The revert operation itself is triggered by the timer.

One could invoke the revert operation directly when the file watch event
arrives. But the timer would still be needed. For example for these
files which do not support file watches (see above), or for buffers
which are not linked with a visiting file. Therefore, it might not be
necessary to invoke the revert operation directly from the incoming
event.

>         Stefan

Best regards, Michael.

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Thu, 10 Jan 2013 15:28:27 +0100
> 
> As a proof of concept, I have installed a patch in autorevert.el
> implementing file watches.  This shall work for `auto-revert-mode' and
> `global-auto-revert-mode' for buffers visiting files.
> `auto-revert-tail-mode' is not supported (yet).

Thanks.

> The implementation uses `inotify-*-watch' functions. I have also added
> the calls for `w32-*-watch' functions, but this is untested.

The functions' names are w32notify-*-watch.  I fixed that in the trunk
revision 111482.  I also did some simple testing, and it seems to
generally work (but see the problems described below).

> Any feedback is welcome.

My feedback is as follows:

 . The code as written is too naive: it blindly assumes that every
   single notification reported by the filesystem for a given watch is
   necessarily the one requested in the auto-revert-notify-add-watch
   call.  But that assumption is false, at least on Windows, where the
   implementation actually watches events to the entire parent
   directory of the file we are interested in.  So Emacs reverts the
   file whenever _any_ file in the same directory was changed.  I
   believe similar problems can happen with inotify, albeit much more
   rarely.  For that reason, I think auto-revert-notify-handler should
   filter events by ASPECTS/ACTION member, and on Windows also by FILE
   member of the event.

 . It isn't clear to me that using IN_CLOSE_WRITE with inotify is TRT:
   AFAIU, that would mean we only revert a file when the application
   writing to it closes its descriptor.  IOW, if the application makes
   several changes to the file during a prolonged operation, and
   doesn't close and reopen the file in between, we will only see the
   changes at the end, but not during the operation.  Wouldn't it be
   better to use IN_MODIFY instead?

 . At least on Windows, turning on auto-revert-mode and then modifying
   and saving the file announces that it was auto-reverted.  This
   didn't happen with the auto-revert method that doesn't use file
   notifications.  Is this a bug?

 . I believe some of the features added to autorevert.el, such as a
   hash list of watch descriptors, should be in some infrastructure
   with appropriate APIs.

Re: File watch support in autorevert.el

[Stefan Monnier]

>  . The code as written is too naive: it blindly assumes that every
>    single notification reported by the filesystem for a given watch is
>    necessarily the one requested in the auto-revert-notify-add-watch
>    call.  But that assumption is false, at least on Windows, where the
>    implementation actually watches events to the entire parent
>    directory of the file we are interested in.  So Emacs reverts the
>    file whenever _any_ file in the same directory was changed.  I
>    believe similar problems can happen with inotify, albeit much more
>    rarely.  For that reason, I think auto-revert-notify-handler should
>    filter events by ASPECTS/ACTION member, and on Windows also by FILE
>    member of the event.

That seems like a good candidate for something the common API could
hide/provide, I think.

>  . It isn't clear to me that using IN_CLOSE_WRITE with inotify is TRT:
>    AFAIU, that would mean we only revert a file when the application
>    writing to it closes its descriptor.  IOW, if the application makes
>    several changes to the file during a prolonged operation, and
>    doesn't close and reopen the file in between, we will only see the
>    changes at the end, but not during the operation.  Wouldn't it be
>    better to use IN_MODIFY instead?

For auto-revert-tail-mode, IN_CLOSE_WRITE is definitely insufficient
since the common use case is when we watch a log file, so the CLOSE may
never happen.

The non-inotify code does something akin to the IN_MODIFY, indeed.
But at the same time, it's often preferable to wait a bit longer for the
application to finish writing the new version of the file.  I think the
perfect behavior lies somewhere in-between: when we get an
IN_CLOSE_WRITE, we should revert immediately, but when we get an
IN_MODIFY we should revert "soon".


        Stefan

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Michael Albinus <michael.albinus@gmx.de>,  emacs-devel@gnu.org
> Date: Fri, 11 Jan 2013 09:34:33 -0500
> 
> >  . The code as written is too naive: it blindly assumes that every
> >    single notification reported by the filesystem for a given watch is
> >    necessarily the one requested in the auto-revert-notify-add-watch
> >    call.  But that assumption is false, at least on Windows, where the
> >    implementation actually watches events to the entire parent
> >    directory of the file we are interested in.  So Emacs reverts the
> >    file whenever _any_ file in the same directory was changed.  I
> >    believe similar problems can happen with inotify, albeit much more
> >    rarely.  For that reason, I think auto-revert-notify-handler should
> >    filter events by ASPECTS/ACTION member, and on Windows also by FILE
> >    member of the event.
> 
> That seems like a good candidate for something the common API could
> hide/provide, I think.

I agree.

> >  . It isn't clear to me that using IN_CLOSE_WRITE with inotify is TRT:
> >    AFAIU, that would mean we only revert a file when the application
> >    writing to it closes its descriptor.  IOW, if the application makes
> >    several changes to the file during a prolonged operation, and
> >    doesn't close and reopen the file in between, we will only see the
> >    changes at the end, but not during the operation.  Wouldn't it be
> >    better to use IN_MODIFY instead?
> 
> For auto-revert-tail-mode, IN_CLOSE_WRITE is definitely insufficient
> since the common use case is when we watch a log file, so the CLOSE may
> never happen.
> 
> The non-inotify code does something akin to the IN_MODIFY, indeed.

I would suggest adding the 'size' filter as well, because Windows
doesn't update the last write time on every write to a file, only
after many writes.  (It does similar filtering with updating the
directory entry of the file, so 'size' alone will not do.)

> But at the same time, it's often preferable to wait a bit longer for the
> application to finish writing the new version of the file.  I think the
> perfect behavior lies somewhere in-between: when we get an
> IN_CLOSE_WRITE, we should revert immediately, but when we get an
> IN_MODIFY we should revert "soon".

You mean, with a timer?

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> >  . It isn't clear to me that using IN_CLOSE_WRITE with inotify is TRT:
>> >    AFAIU, that would mean we only revert a file when the application
>> >    writing to it closes its descriptor.  IOW, if the application makes
>> >    several changes to the file during a prolonged operation, and
>> >    doesn't close and reopen the file in between, we will only see the
>> >    changes at the end, but not during the operation.  Wouldn't it be
>> >    better to use IN_MODIFY instead?
>> 
>> For auto-revert-tail-mode, IN_CLOSE_WRITE is definitely insufficient
>> since the common use case is when we watch a log file, so the CLOSE may
>> never happen.

I can speak only for the inotify case. According to my tests,
IN_CLOSE_WRITE will always happen once a file has been written on the
filesystem. See for example (commands have been applied in different shells):

--8<---------------cut here---------------start------------->8---
~$ echo xxx >>~/tmp/123

~$ inotifywait -mq ~/tmp/123
/home/albinmic/tmp/123 OPEN 
/home/albinmic/tmp/123 MODIFY 
/home/albinmic/tmp/123 CLOSE_WRITE,CLOSE 
--8<---------------cut here---------------end--------------->8---

Do you (Stefan?) have a use case where just IN_MODIFY has been fired,
w/o a corresponding IN_CLOSE_WRITE?

> I would suggest adding the 'size' filter as well, because Windows
> doesn't update the last write time on every write to a file, only
> after many writes.  (It does similar filtering with updating the
> directory entry of the file, so 'size' alone will not do.)

Hmm, as said already I have almost no chance to test it under Windows ...
I would let this implementation to somebody else.

>> But at the same time, it's often preferable to wait a bit longer for the
>> application to finish writing the new version of the file.  I think the
>> perfect behavior lies somewhere in-between: when we get an
>> IN_CLOSE_WRITE, we should revert immediately, but when we get an
>> IN_MODIFY we should revert "soon".
>
> You mean, with a timer?

The timer is still active. The file watch handler just marks buffers
where the related file has been changed. Revert happens when the timer
goes through the buffers markes via `auto-revert-active-p'. That sounds
like an acceptable compromise.

Best regards, Michael.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>  . The code as written is too naive: it blindly assumes that every
>    single notification reported by the filesystem for a given watch is
>    necessarily the one requested in the auto-revert-notify-add-watch
>    call.  But that assumption is false, at least on Windows, where the
>    implementation actually watches events to the entire parent
>    directory of the file we are interested in.  So Emacs reverts the
>    file whenever _any_ file in the same directory was changed.  I
>    believe similar problems can happen with inotify, albeit much more
>    rarely.  For that reason, I think auto-revert-notify-handler should
>    filter events by ASPECTS/ACTION member, and on Windows also by FILE
>    member of the event.

Will do for the inotify case. It is a simple bit easier, because you can
install a file watch for exactly one file, and you can expect it returns
for that file only.

This will be different, when we implement `dired-buffer-stale-p' based
on file watches.

In general, something like IN_DELETE* and IN_MOVE* must be handled
properly. That's clearly missing in my first shot.

>  . It isn't clear to me that using IN_CLOSE_WRITE with inotify is TRT:
>    AFAIU, that would mean we only revert a file when the application
>    writing to it closes its descriptor.  IOW, if the application makes
>    several changes to the file during a prolonged operation, and
>    doesn't close and reopen the file in between, we will only see the
>    changes at the end, but not during the operation.  Wouldn't it be
>    better to use IN_MODIFY instead?

See my other message. I believe IN_CLOSE_WRITE is sufficient for the
inotify case, but I might be wrong. I would need a test case which shows
it.

>  . At least on Windows, turning on auto-revert-mode and then modifying
>    and saving the file announces that it was auto-reverted.  This
>    didn't happen with the auto-revert method that doesn't use file
>    notifications.  Is this a bug?

I have an old Emacs instance, w/o support of inotify in
autorevert.el. There I see the same message.

>  . I believe some of the features added to autorevert.el, such as a
>    hash list of watch descriptors, should be in some infrastructure
>    with appropriate APIs.

Yes. During these tests we want to identify these features. When
possible, I will try to add more general functions in autorevert.el,
which could be extracted later for a general API.

Best regards, Michael.

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  emacs-devel@gnu.org
> Date: Fri, 11 Jan 2013 16:01:14 +0100
> 
> >> For auto-revert-tail-mode, IN_CLOSE_WRITE is definitely insufficient
> >> since the common use case is when we watch a log file, so the CLOSE may
> >> never happen.
> 
> I can speak only for the inotify case. According to my tests,
> IN_CLOSE_WRITE will always happen once a file has been written on the
> filesystem. See for example (commands have been applied in different shells):

The documentation (inotify(7) page) clearly says

    IN_CLOSE_WRITE

    File opened for writing was closed

> --8<---------------cut here---------------start------------->8---
> ~$ echo xxx >>~/tmp/123
> 
> ~$ inotifywait -mq ~/tmp/123
> /home/albinmic/tmp/123 OPEN 
> /home/albinmic/tmp/123 MODIFY 
> /home/albinmic/tmp/123 CLOSE_WRITE,CLOSE 
> --8<---------------cut here---------------end--------------->8---
> 
> Do you (Stefan?) have a use case where just IN_MODIFY has been fired,
> w/o a corresponding IN_CLOSE_WRITE?

Maybe I'm missing something, but the above example is not the issue at
hand.  The issue at hand is to have several separate writes to a file
with some significant time between them.  AFAIU, in the above example
the file is written once and then closed.

> > I would suggest adding the 'size' filter as well, because Windows
> > doesn't update the last write time on every write to a file, only
> > after many writes.  (It does similar filtering with updating the
> > directory entry of the file, so 'size' alone will not do.)
> 
> Hmm, as said already I have almost no chance to test it under Windows ...
> I would let this implementation to somebody else.

So now we will have features which, although equivalent, are only
implemented on the platform of the implementer's choice?  If so, what
good is it to have the same feature implemented on different
platforms?

Re: File watch support in autorevert.el

[Stefan Monnier]

>> But at the same time, it's often preferable to wait a bit longer for the
>> application to finish writing the new version of the file.  I think the
>> perfect behavior lies somewhere in-between: when we get an
>> IN_CLOSE_WRITE, we should revert immediately, but when we get an
>> IN_MODIFY we should revert "soon".
> You mean, with a timer?

Yes.


        Stefan

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: emacs-devel@gnu.org
> Date: Fri, 11 Jan 2013 16:18:46 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >  . The code as written is too naive: it blindly assumes that every
> >    single notification reported by the filesystem for a given watch is
> >    necessarily the one requested in the auto-revert-notify-add-watch
> >    call.  But that assumption is false, at least on Windows, where the
> >    implementation actually watches events to the entire parent
> >    directory of the file we are interested in.  So Emacs reverts the
> >    file whenever _any_ file in the same directory was changed.  I
> >    believe similar problems can happen with inotify, albeit much more
> >    rarely.  For that reason, I think auto-revert-notify-handler should
> >    filter events by ASPECTS/ACTION member, and on Windows also by FILE
> >    member of the event.
> 
> Will do for the inotify case. It is a simple bit easier, because you can
> install a file watch for exactly one file, and you can expect it returns
> for that file only.

You cannot expect that, not in general, because there's no such
promise in the docs of these interfaces.  That's why the interface
returns to you the full information about the transaction.  I don't
think it's wise to ignore that information.

> >  . It isn't clear to me that using IN_CLOSE_WRITE with inotify is TRT:
> >    AFAIU, that would mean we only revert a file when the application
> >    writing to it closes its descriptor.  IOW, if the application makes
> >    several changes to the file during a prolonged operation, and
> >    doesn't close and reopen the file in between, we will only see the
> >    changes at the end, but not during the operation.  Wouldn't it be
> >    better to use IN_MODIFY instead?
> 
> See my other message. I believe IN_CLOSE_WRITE is sufficient for the
> inotify case, but I might be wrong. I would need a test case which shows
> it.

Since the documentation clearly says this event is generated when the
file is _closed_, I wonder why a test case is needed.

> >  . At least on Windows, turning on auto-revert-mode and then modifying
> >    and saving the file announces that it was auto-reverted.  This
> >    didn't happen with the auto-revert method that doesn't use file
> >    notifications.  Is this a bug?
> 
> I have an old Emacs instance, w/o support of inotify in
> autorevert.el. There I see the same message.

I don't see this in Emacs 24.2.92 on Windows.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> Hmm, as said already I have almost no chance to test it under Windows ...
>> I would let this implementation to somebody else.
>
> So now we will have features which, although equivalent, are only
> implemented on the platform of the implementer's choice?  If so, what
> good is it to have the same feature implemented on different
> platforms?

It's not "my choice". I simply don't use MS Windows, and so I cannot do
anything for MS Windows but adding code w/o any test.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> Maybe I'm missing something, but the above example is not the issue at
> hand.  The issue at hand is to have several separate writes to a file
> with some significant time between them.  AFAIU, in the above example
> the file is written once and then closed.

You are right, I was thinking the wrong direction. 

So we must use `modify' (and maybe `close-write') for inotify, and
`size' and `last-write-time' for w32notify. The problem is, that the
events are coming randomly, and one must decide whether the correct
handling has been performed already.

It does not make sense to revert a file twice, just because 2 different
events indicate the same change. Yes, the timer makes sense.

Best regards, Michael.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> You cannot expect that, not in general, because there's no such
> promise in the docs of these interfaces.  That's why the interface
> returns to you the full information about the transaction.  I don't
> think it's wise to ignore that information.

At least for now, inotify.el returns nil as file name in the event ...

Well, reading the INOTIFY(7) manpage it can happen, that an existing
watch can change. In that case, it could indeed return notifications for
another file. So we shall check the file name once it is returned.

OTOH, do we want to allow a redefinition of a running watch?

>> >  . At least on Windows, turning on auto-revert-mode and then modifying
>> >    and saving the file announces that it was auto-reverted.  This
>> >    didn't happen with the auto-revert method that doesn't use file
>> >    notifications.  Is this a bug?
>>
>> I have an old Emacs instance, w/o support of inotify in
>> autorevert.el. There I see the same message.
>
> I don't see this in Emacs 24.2.92 on Windows.

I see this message with 

GNU Emacs 24.2.91.3 (x86_64-unknown-linux-gnu, GTK+ Version 2.24.13)
 of 2013-01-04 on detlef

Best regards, Michael.

Re: File watch support in autorevert.el

[Stefan Monnier]

> I can speak only for the inotify case. According to my tests,
> IN_CLOSE_WRITE will always happen once a file has been written on the
> filesystem. See for example (commands have been applied in different shells):
> --8<---------------cut here---------------start------------->8---
> ~$ echo xxx >>~/tmp/123
> ~$ inotifywait -mq ~/tmp/123
> /home/albinmic/tmp/123 OPEN 
> /home/albinmic/tmp/123 MODIFY 
> /home/albinmic/tmp/123 CLOSE_WRITE,CLOSE 
> --8<---------------cut here---------------end--------------->8---
> Do you (Stefan?) have a use case where just IN_MODIFY has been fired,
> w/o a corresponding IN_CLOSE_WRITE?

Try:

  while sleep 60; do echo hello; done >> ~/tmp/123

Look ma, no close!

But you don't need a script: just try auto-revert-mode on a log written
by syslogd.

Admittedly, these cases are presumably clients of auto-revert-tail-mode,
but auto-revert-mode should handle them correctly.


        Stefan

Re: File watch support in autorevert.el

[Stefan Monnier]

> Will do for the inotify case. It is a simple bit easier, because you can
> install a file watch for exactly one file, and you can expect it returns
> for that file only.

BTW, what happens if the file gets overwritten without touching its
inode (e.g. use auto-revert-mode on ~/foo and then do "mv ~/bar ~/foo"
and then "echo toto >>~/foo")?

You'll presumably get some notification of the "mv" itself, but will you
subsequently get the notification of the "echo" (which is now modifying
another inode than the original ~/foo)?


        Stefan

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: emacs-devel@gnu.org
> Date: Fri, 11 Jan 2013 17:31:15 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > You cannot expect that, not in general, because there's no such
> > promise in the docs of these interfaces.  That's why the interface
> > returns to you the full information about the transaction.  I don't
> > think it's wise to ignore that information.
> 
> At least for now, inotify.el returns nil as file name in the event ...

Probably because some events don't provide a file name.

> OTOH, do we want to allow a redefinition of a running watch?

Not sure, probably not.

> >> >  . At least on Windows, turning on auto-revert-mode and then modifying
> >> >    and saving the file announces that it was auto-reverted.  This
> >> >    didn't happen with the auto-revert method that doesn't use file
> >> >    notifications.  Is this a bug?
> >>
> >> I have an old Emacs instance, w/o support of inotify in
> >> autorevert.el. There I see the same message.
> >
> > I don't see this in Emacs 24.2.92 on Windows.
> 
> I see this message with 
> 
> GNU Emacs 24.2.91.3 (x86_64-unknown-linux-gnu, GTK+ Version 2.24.13)
>  of 2013-01-04 on detlef

Probably because the underlying primitives (file-attributes etc.)
behave differently on GNU/Linux and on Windows.

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Fri, 11 Jan 2013 17:09:07 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Hmm, as said already I have almost no chance to test it under Windows ...
> >> I would let this implementation to somebody else.
> >
> > So now we will have features which, although equivalent, are only
> > implemented on the platform of the implementer's choice?  If so, what
> > good is it to have the same feature implemented on different
> > platforms?
> 
> It's not "my choice". I simply don't use MS Windows, and so I cannot do
> anything for MS Windows but adding code w/o any test.

If we have a single API that hides the OS dependencies, you will not
need to write 2 versions of the same code, but just one.  Testing that
on platforms that you don't use will then be up to the users of those
platforms, like always in Emacs.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>  . At least on Windows, turning on auto-revert-mode and then modifying
>    and saving the file announces that it was auto-reverted.  This
>    didn't happen with the auto-revert method that doesn't use file
>    notifications.  Is this a bug?

Finally, I found a bug. Should be fixed now.

Best regards, Michael.

Re: File watch support in autorevert.el

[Michael Albinus]

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

> Try:
>
>   while sleep 60; do echo hello; done >> ~/tmp/123
>
> Look ma, no close!
>
> But you don't need a script: just try auto-revert-mode on a log written
> by syslogd.
>
> Admittedly, these cases are presumably clients of auto-revert-tail-mode,
> but auto-revert-mode should handle them correctly.

Should be OK now for all 3 modes: global-auto-revert-mode,
auto-revert-mode and auto-revert-tail-mode (support for the last one is
just committed). For the inotify case, I've changed to IN_MODIFY as
supervised event. For w32notify, I have added size as proposed by Eli.

>         Stefan

Best regards, Michael.

Re: File watch support in autorevert.el

[Michael Albinus]

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

>> Will do for the inotify case. It is a simple bit easier, because you can
>> install a file watch for exactly one file, and you can expect it returns
>> for that file only.
>
> BTW, what happens if the file gets overwritten without touching its
> inode (e.g. use auto-revert-mode on ~/foo and then do "mv ~/bar ~/foo"
> and then "echo toto >>~/foo")?
>
> You'll presumably get some notification of the "mv" itself, but will you
> subsequently get the notification of the "echo" (which is now modifying
> another inode than the original ~/foo)?

No further notifications. I've realized it just now, because this is the
scenario when you save a file with `backup-by-copying' set to nil. You
will loose further notifications, because the file was moved away.

Tested in the inotify case. For w32notify, I don't know.

>         Stefan

Best regards, Michael.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>  . For that reason, I think auto-revert-notify-handler should
>    filter events by ASPECTS/ACTION member, and on Windows also by FILE
>    member of the event.

I have added first checks. Hopefully, they do not break the w32notify case.

Best regards, Michael.

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Fri, 11 Jan 2013 23:43:56 +0100
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> > Try:
> >
> >   while sleep 60; do echo hello; done >> ~/tmp/123
> >
> > Look ma, no close!
> >
> > But you don't need a script: just try auto-revert-mode on a log written
> > by syslogd.
> >
> > Admittedly, these cases are presumably clients of auto-revert-tail-mode,
> > but auto-revert-mode should handle them correctly.
> 
> Should be OK now for all 3 modes: global-auto-revert-mode,
> auto-revert-mode and auto-revert-tail-mode (support for the last one is
> just committed). For the inotify case, I've changed to IN_MODIFY as
> supervised event. For w32notify, I have added size as proposed by Eli.

Thanks.  I tried auto-revert-mode and auto-revert-tail-mode on
MS-Windows, and they both seem to work as expected (after I fixed the
minor issues described in my other mail).

I see only one problem: if you manually "M-x revert-buffer RET", a new
file watch is added, although the old one is still active.  AFAICS,
you rely on the watch descriptor local var be non-nil for avoiding
this, so perhaps revert-buffer reverts that variable as well?  Or is
this a Windows-only problem?

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: emacs-devel@gnu.org
> Date: Sat, 12 Jan 2013 00:01:07 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >  . For that reason, I think auto-revert-notify-handler should
> >    filter events by ASPECTS/ACTION member, and on Windows also by FILE
> >    member of the event.
> 
> I have added first checks. Hopefully, they do not break the w32notify case.

I needed to make 2 changes to get this to work on Windows, see trunk
revision 111499.  Actually, I don't understand how it worked with
inotify before my changes, since you were looking for 'modify' in the
descriptor rather than in the action.  Did I miss something?

Btw, should we perhaps do something more sophisticated than
string-equal to compare file names?  On Windows, we probably should
compare case-insensitively, but what about file-truename and friends
on Posix platforms?

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Fri, 11 Jan 2013 23:47:41 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> >> Will do for the inotify case. It is a simple bit easier, because you can
> >> install a file watch for exactly one file, and you can expect it returns
> >> for that file only.
> >
> > BTW, what happens if the file gets overwritten without touching its
> > inode (e.g. use auto-revert-mode on ~/foo and then do "mv ~/bar ~/foo"
> > and then "echo toto >>~/foo")?
> >
> > You'll presumably get some notification of the "mv" itself, but will you
> > subsequently get the notification of the "echo" (which is now modifying
> > another inode than the original ~/foo)?
> 
> No further notifications. I've realized it just now, because this is the
> scenario when you save a file with `backup-by-copying' set to nil. You
> will loose further notifications, because the file was moved away.
> 
> Tested in the inotify case. For w32notify, I don't know.

Windows is at advantage here, since it watches the entire directory.
So when the new "foo" is created and gets updated, the notifications
come in and the file is auto-reverted.

Perhaps the inotify implementation should watch the parent directory
instead of the file.

The rename notification doesn't come in, because we didn't request
it.  To request it (if we need that), wee should add 'file-name' to
the list of w32notify filters.  AFAIU, with inotify we need to use
'move' to get notifications about renames.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> I needed to make 2 changes to get this to work on Windows, see trunk
> revision 111499.  Actually, I don't understand how it worked with
> inotify before my changes, since you were looking for 'modify' in the
> descriptor rather than in the action.  Did I miss something?

You're right, I've introduced a bug here at the very end of my tests
last night.

> Btw, should we perhaps do something more sophisticated than
> string-equal to compare file names?  On Windows, we probably should
> compare case-insensitively, but what about file-truename and friends
> on Posix platforms?

What about `file-equal-p'? Like

--8<---------------cut here---------------start------------->8---
~/src/emacs> bzr diff lisp/autorevert.el
=== modified file 'lisp/autorevert.el'
--- lisp/autorevert.el	2013-01-12 11:25:39 +0000
+++ lisp/autorevert.el	2013-01-12 13:06:19 +0000
@@ -531,17 +531,11 @@
 	(when (featurep 'inotify) (cl-assert (memq 'modify action)))
 	(when (featurep 'w32notify) (cl-assert (eq 'modified action)))
 	(cl-assert (bufferp buffer))
-	(when (stringp file)
-	  (cl-assert (string-equal
-		      ;; w32notify returns the basename of the file
-		      ;; without its leading directories; inotify
-		      ;; returns its full absolute file name.
-                      (file-name-nondirectory (directory-file-name file))
-                      (file-name-nondirectory (directory-file-name
-					       (buffer-file-name buffer))))))
-
-	;; Mark buffer modified.
 	(with-current-buffer buffer
+	  (when (and (stringp file) (stringp buffer-file-name))
+	    (cl-assert (file-equal file buffer-file-name)))
+
+	  ;; Mark buffer modified.
 	  (setq auto-revert-notify-modified-p t))))))
 
 (defun auto-revert-active-p ()
--8<---------------cut here---------------end--------------->8---

The disadvantage might be performance, because `file-equal-p' performs
operations on the filesystem, like `file-attributes' etc. When all files
in a directory are watched, there will be a lot of events to be ignored.
Is this acceptable?

Best regards, Michael.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> Windows is at advantage here, since it watches the entire directory.
> So when the new "foo" is created and gets updated, the notifications
> come in and the file is auto-reverted.
>
> Perhaps the inotify implementation should watch the parent directory
> instead of the file.

I was thinking about, but I have no idea on performance issues when
notifications come in from all files in a large directory. OTOH, it
might ease the implementation, becauses we would have the same scenario
for inotify and w32notify.

One problem just now is that we expect one watch descriptor per
buffer. If we watch several files in the same directory, this approach
might fail. Will check.

Best regards, Michael.

Re: File watch support in autorevert.el

[Michael Albinus]

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

> +	    (cl-assert (file-equal file buffer-file-name)))
                                  ^-p

Best regards, Michael.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> I see only one problem: if you manually "M-x revert-buffer RET", a new
> file watch is added, although the old one is still active.  AFAICS,
> you rely on the watch descriptor local var be non-nil for avoiding
> this, so perhaps revert-buffer reverts that variable as well?  Or is
> this a Windows-only problem?

Fixed in revno#111500.

Best regards, Michael.

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: emacs-devel@gnu.org
> Date: Sat, 12 Jan 2013 14:08:42 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I needed to make 2 changes to get this to work on Windows, see trunk
> > revision 111499.  Actually, I don't understand how it worked with
> > inotify before my changes, since you were looking for 'modify' in the
> > descriptor rather than in the action.  Did I miss something?
> 
> You're right, I've introduced a bug here at the very end of my tests
> last night.
> 
> > Btw, should we perhaps do something more sophisticated than
> > string-equal to compare file names?  On Windows, we probably should
> > compare case-insensitively, but what about file-truename and friends
> > on Posix platforms?
> 
> What about `file-equal-p'?

Can't use that on Windows, because FILE in the notification is just
the basename.  If we want to be able to do that, w32notify will have
to prepend the directory that is being watched to the file names in
the notifications.

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Sat, 12 Jan 2013 14:14:36 +0100
> 
> > Perhaps the inotify implementation should watch the parent directory
> > instead of the file.
> 
> I was thinking about, but I have no idea on performance issues when
> notifications come in from all files in a large directory.

How is this different from receiving a lot of output from a
subprocess?  Emacs already copes with that, and file notifications use
the same mechanisms as process output reception.  So I wouldn't worry
about that.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> What about `file-equal-p'?
>
> Can't use that on Windows, because FILE in the notification is just
> the basename.  If we want to be able to do that, w32notify will have
> to prepend the directory that is being watched to the file names in
> the notifications.

`default-directory' is set properly, it shall work also on Windows. Or
do I miss something?

Best regards, Michael.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> I was thinking about, but I have no idea on performance issues when
>> notifications come in from all files in a large directory.
>
> How is this different from receiving a lot of output from a
> subprocess?  Emacs already copes with that, and file notifications use
> the same mechanisms as process output reception.  So I wouldn't worry
> about that.

OK, I'll check. I will also try to get performance figures on large
directories with a lot of notifications.

What I fear a little bit is the remote file case, which I also have in
mind. Will test this as well.

Best regards, Michael.

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: emacs-devel@gnu.org
> Date: Sat, 12 Jan 2013 15:12:58 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> What about `file-equal-p'?
> >
> > Can't use that on Windows, because FILE in the notification is just
> > the basename.  If we want to be able to do that, w32notify will have
> > to prepend the directory that is being watched to the file names in
> > the notifications.
> 
> `default-directory' is set properly, it shall work also on Windows.

For the autorevert use case, yes, it will work.  But in general, no
one said that the file being watched is necessarily in the
default-directory of the buffer.

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Sat, 12 Jan 2013 14:34:20 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I see only one problem: if you manually "M-x revert-buffer RET", a new
> > file watch is added, although the old one is still active.  AFAICS,
> > you rely on the watch descriptor local var be non-nil for avoiding
> > this, so perhaps revert-buffer reverts that variable as well?  Or is
> > this a Windows-only problem?
> 
> Fixed in revno#111500.

Thanks, it is fixed indeed.

Tracing what happens when I type "M-x revert-buffer RET yes RET", I
see that the watch is removed (as result of kill-buffer-hook) and then
a new watch is started.  Is this intended?

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> For the autorevert use case, yes, it will work.  But in general, no
> one said that the file being watched is necessarily in the
> default-directory of the buffer.
For the general API, I'm still in favor to add the file being watched to
the watch descriptor. I'll need it anyway for the Tramp file name
handler, and it would serve here as well. But this we can discuss when
it comes to the general API.

For the time being, we could use file-equal-p instead of string-equal in
autorevert.el.

Best regards, Michael.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> Tracing what happens when I type "M-x revert-buffer RET yes RET", I
> see that the watch is removed (as result of kill-buffer-hook) and then
> a new watch is started.  Is this intended?

No, it's not intended. But the current implementation using
kill-buffer-hook leads to this effect. I'll check, whether we could
optimize.

Best regards, Michael.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> Tracing what happens when I type "M-x revert-buffer RET yes RET", I
> see that the watch is removed (as result of kill-buffer-hook) and then
> a new watch is started.  Is this intended?

Should be fixed in revno#111542.

When I enter my next free time slot, I'll try to change the
implementation watching directories only also for the inotify case.

And I do not know, what happens with w32notify in the current
implementation, if you start auto-reverting of 2 files located in the
same directory. Does it work, or are there errors?

Best regards, Michael.

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Thu, 17 Jan 2013 10:38:58 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> Hi Eli,
> 
> > Tracing what happens when I type "M-x revert-buffer RET yes RET", I
> > see that the watch is removed (as result of kill-buffer-hook) and then
> > a new watch is started.  Is this intended?
> 
> Should be fixed in revno#111542.

Thanks, it is indeed fixed according to my limited testing.

> When I enter my next free time slot, I'll try to change the
> implementation watching directories only also for the inotify case.

Thanks.

> And I do not know, what happens with w32notify in the current
> implementation, if you start auto-reverting of 2 files located in the
> same directory. Does it work, or are there errors?

It works, after I change this:

  (defvar auto-revert-notify-watch-descriptor nil

to use defvar-local instead.  Without that,
auto-revert-notify-watch-descriptor is globally visible, so no more
than a single file can be autoreverted via notifications, even if the
other files are in different directories.

Other than that, why did you think it might not work?  Each file gets
its own watch machinery and resources, and while both watches are
reporting the same low-level events, the filtering by file name in
autorevert.el does its job well, and each buffer gets only the events
it is interested in.

Re: File watch support in autorevert.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> And I do not know, what happens with w32notify in the current
>> implementation, if you start auto-reverting of 2 files located in the
>> same directory. Does it work, or are there errors?
>
> It works, after I change this:
>
>   (defvar auto-revert-notify-watch-descriptor nil
>
> to use defvar-local instead.  Without that,
> auto-revert-notify-watch-descriptor is globally visible, so no more
> than a single file can be autoreverted via notifications, even if the
> other files are in different directories.

Ahh, thanks. This was damaged with the patch of last Saturday by
accident; I've fixed it in the trunk.

> Other than that, why did you think it might not work?  Each file gets
> its own watch machinery and resources, and while both watches are
> reporting the same low-level events, the filtering by file name in
> autorevert.el does its job well, and each buffer gets only the events
> it is interested in.

The point is that every file being watched should have a different watch
descriptor. Otherwise, an *-rm-watch command for a given file would stop
watching all files in the same directory.

Good to know that there's no problem with w32notify. For inotify, I need
to implement such mechanism, likely.

Best regards, Michael.

Re: File watch support in autorevert.el

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Thu, 17 Jan 2013 20:19:49 +0100
> 
> The point is that every file being watched should have a different watch
> descriptor. Otherwise, an *-rm-watch command for a given file would stop
> watching all files in the same directory.
> 
> Good to know that there's no problem with w32notify. For inotify, I need
> to implement such mechanism, likely.

Ah, I missed the fact that inotify_add_watch will return the same
handle when called with a file name that is already watched.  Now I
see what you mean.