Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

[Stefan Monnier]

> +  (when (or (string-match auto-revert-notify-exclude-dir-regexp
> +			  (expand-file-name default-directory))
> +	    (not (file-symlink-p buffer-file-name)))

I think this patch is acceptable, but if/when we try to write a generic
file-watcher Elisp API, it would be good to make this work (probably by
adding a watcher on the symlink's target).


        Stefan

Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Mon, 09 Dec 2013 21:30:53 -0500
> Cc: emacs-devel@gnu.org
> 
> > +  (when (or (string-match auto-revert-notify-exclude-dir-regexp
> > +			  (expand-file-name default-directory))
> > +	    (not (file-symlink-p buffer-file-name)))
> 
> I think this patch is acceptable, but if/when we try to write a generic
> file-watcher Elisp API, it would be good to make this work (probably by
> adding a watcher on the symlink's target).

It's up to the user to say whether she wants to watch the symlink or
its target, I think.

Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> I think this patch is acceptable, but if/when we try to write a generic
>> file-watcher Elisp API, it would be good to make this work (probably by
>> adding a watcher on the symlink's target).
>
> It's up to the user to say whether she wants to watch the symlink or
> its target, I think.

In general I agree. But Stefan was commenting on my change in
autorevert.el. Here it is obvious, that the target shall be watched.
I believe.

Best regards, Michael.

Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

[Davis Herring]

> In general I agree. But Stefan was commenting on my change in
> autorevert.el. Here it is obvious, that the target shall be watched.
> I believe.

It seems to me that this is a variation on "tail -f" vs. "tail -F"; you
might even want to watch both the symlink and its target if the target
might be written or the symlink redirected.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Stefan Monnier <monnier@IRO.UMontreal.CA>,  emacs-devel@gnu.org
> Date: Tue, 10 Dec 2013 05:58:18 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I think this patch is acceptable, but if/when we try to write a generic
> >> file-watcher Elisp API, it would be good to make this work (probably by
> >> adding a watcher on the symlink's target).
> >
> > It's up to the user to say whether she wants to watch the symlink or
> > its target, I think.
> 
> In general I agree. But Stefan was commenting on my change in
> autorevert.el. Here it is obvious, that the target shall be watched.
> I believe.

What if the target is a directory?

Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

[Stefan Monnier]

>> >> I think this patch is acceptable, but if/when we try to write a generic
>> >> file-watcher Elisp API, it would be good to make this work (probably by
>> >> adding a watcher on the symlink's target).
>> > It's up to the user to say whether she wants to watch the symlink or
>> > its target, I think.

autorevert cares about keeping the buffer in sync with the contents of
the file available at buffer-file-name.  So the user has already said
what she wants.

This need could occur in other circumstances as well: inotify and
friends usually work at a "shallow" level whereas many uses of
file-tracking care about the contents (hence following any indirections
like symlinks as deeply as needed).  So a good "notification" Elisp API
should probably provide the feature.

>> In general I agree. But Stefan was commenting on my change in
>> autorevert.el. Here it is obvious, that the target shall be watched.
>> I believe.
> What if the target is a directory?

I don't understand your question: whether the target is a directory or
not doesn't seem to make any difference.


        Stefan

Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Michael Albinus <michael.albinus@gmx.de>
>> Cc: Stefan Monnier <monnier@IRO.UMontreal.CA>,  emacs-devel@gnu.org
>> Date: Tue, 10 Dec 2013 05:58:18 +0100
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> >> I think this patch is acceptable, but if/when we try to write a generic
>> >> file-watcher Elisp API, it would be good to make this work (probably by
>> >> adding a watcher on the symlink's target).
>> >
>> > It's up to the user to say whether she wants to watch the symlink or
>> > its target, I think.
>>
>> In general I agree. But Stefan was commenting on my change in
>> autorevert.el. Here it is obvious, that the target shall be watched.
>> I believe.
>
> What if the target is a directory?

This is not relevant for autorevert.el and file notifications - if the
target is a directory, file notifications are not used (yet) in
autorevert.el. I've created Bug#16112 for this.

Best regards, Michael.

Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> +  (when (or (string-match auto-revert-notify-exclude-dir-regexp
>> +			  (expand-file-name default-directory))
>> +	    (not (file-symlink-p buffer-file-name)))
>
> I think this patch is acceptable, but if/when we try to write a generic
> file-watcher Elisp API, it would be good to make this work (probably by
> adding a watcher on the symlink's target).

That was my first idea, too. But when I tried to implement this, there
were too much performance penalties, which would eat the advantages of
file notifications. So I've decided to exclude symlinks, at least until
there's a better way to support symlinks.

As far as I am aware, inotify could support symbolic links and their
targets. For glib it seems to depend on the implementation. For kqueue
(*BSD, Mac OS X; candidate as further basic library) I don't know how
it is handled.

In short, it might be necessary to extend the filenotify.el API and the
underlying libraries in order to determine how to handle
symlinks. Nothing we should do days before the feature freeze, IMHO.

>         Stefan

Best regards, Michael.

Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Tue, 10 Dec 2013 05:37:06 +0100
> Cc: emacs-devel@gnu.org
> 
> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
> 
> >> +  (when (or (string-match auto-revert-notify-exclude-dir-regexp
> >> +			  (expand-file-name default-directory))
> >> +	    (not (file-symlink-p buffer-file-name)))
> >
> > I think this patch is acceptable, but if/when we try to write a generic
> > file-watcher Elisp API, it would be good to make this work (probably by
> > adding a watcher on the symlink's target).
> 
> That was my first idea, too. But when I tried to implement this, there
> were too much performance penalties, which would eat the advantages of
> file notifications.

Can you tell more about these performance problems?  Suppose we always
run the file name through file-truename, before submitting it to the
notification back-end -- what problems will this create?

> As far as I am aware, inotify could support symbolic links and their
> targets. For glib it seems to depend on the implementation. For kqueue
> (*BSD, Mac OS X; candidate as further basic library) I don't know how
> it is handled.

I'd prefer that we don't delegate such a delicate issue to the
back-end, the result is likely to be even more system dependencies
than what we have now.

Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

[Stefan Monnier]

> Can you tell more about these performance problems?  Suppose we always
> run the file name through file-truename, before submitting it to the
> notification back-end -- what problems will this create?

One obvious problem is if the user changes the symlink.


        Stefan

Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Michael Albinus <michael.albinus@gmx.de>, emacs-devel@gnu.org
> Date: Tue, 10 Dec 2013 23:38:47 -0500
> 
> > Can you tell more about these performance problems?  Suppose we always
> > run the file name through file-truename, before submitting it to the
> > notification back-end -- what problems will this create?
> 
> One obvious problem is if the user changes the symlink.

Right, but why is that a problem _performance-wise_?  So we probably
need to watch the parent directory of the symlink itself as well, so
what?

Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

[Stefan Monnier]

> Nothing we should do days before the feature freeze, IMHO.

100% agreement.  I just wanted to make sure we remember that what we
have is a workaround rather than a solution.


        Stefan

Re: [Emacs-diffs] trunk r115439: * autorevert.el (auto-revert-notify-add-watch): Do not handle symlinked files.

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> Nothing we should do days before the feature freeze, IMHO.
>
> 100% agreement.  I just wanted to make sure we remember that what we
> have is a workaround rather than a solution.

I've created Bug#16113 for this.

>         Stefan

Best regards, Michael.