bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

classic Classic list List threaded Threaded
39 messages Options
12
Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Juri Linkov-2
X-Debbugs-CC: Stefan Monnier <[hidden email]>

Stefan, do you think diff-syntax-fontify-props should let-bind
after-change-major-mode-hook to nil to not allow running this hook
on an internal buffer.  Or it's the responsibility of the eglot package
to check for the leading space in the buffer name when its
after-change-major-mode-hook is called?

> If I read correctly, diff-syntax-fontify-props sets buffer-file-name of
> a temporary buffer to an existing one.  This is not necessarily a bug,
> but it definitely looks strange that we have two buffers with different
> contents and the same buffer-file-name.
>
> Eglot (in ELPA) runs into problems because of this
> (https://github.com/joaotavora/eglot/pull/233).
>
> Is there a recommended way to check in an after-change-major-mode-hook
> whether the current buffer is or isn't a temporary diff buffer?  For
> example, to check if first character of buffer-name is space.



Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Stefan Monnier
> Stefan, do you think diff-syntax-fontify-props should let-bind
> after-change-major-mode-hook to nil to not allow running this hook
> on an internal buffer.

No, that's definitely not the right fix.  It's at best a weak
workaround, but it won't handle the case where the code that depends on
`buffer-file-name` is placed on `foo-mode-hook` rather than on
`after-change-major-mode-hook`.

>> If I read correctly, diff-syntax-fontify-props sets buffer-file-name of
>> a temporary buffer to an existing one.  This is not necessarily a bug,
>> but it definitely looks strange that we have two buffers with different
>> contents and the same buffer-file-name.

Yes, it's definitely borderline.  I remember feeling a bit uneasy about
it at the time.  Maybe a better fix would be to be able to pass the
file-name to `set-auto-mode` (or some new function created for that
purpose) as an argument (instead of passing it via dynamic scoping in
`buffer-file-name`).

BTW, I see somewhat similar code in `xref--collect-matches`, so maybe
there's a need for a more general solution (haven't looked any further,
but I'd be surprised if there aren't other cases).

> Or it's the responsibility of the eglot package to check for the
> leading space in the buffer name when its after-change-major-mode-hook
> is called?

The eglot package could protect itself from such things by making its
`after-change-major-mode-hook` just add the buffer to a global var, and
then process this global var in `post-command-hook` or in a timer.


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Juri Linkov-2
Now I tried to repeat this problem with eglot and got such backtrace:

Debugger entered--Lisp error: (wrong-type-argument arrayp nil)
  substring(nil 0 1)
  file-truename(nil)
  eglot--path-to-uri(nil)
  eglot--TextDocumentIdentifier()
  eglot--signal-textDocument/didClose()
  kill-buffer(#<buffer  *temp*-492063>)
  (and (buffer-name temp-buffer) (kill-buffer temp-buffer))
  (unwind-protect (progn (insert text) (diff-syntax-fontify-props ...
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn (insert text) (diff-syntax-fontify-props ...
  (let ((temp-buffer (generate-new-buffer " *temp*"))) (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn (insert text) ...
  (cond ((and file diff-default-directory ...) ...) ...)
  (let ((file (car (diff-hunk-file-names old)))) ...)
  (or (if (and diff-vc-backend (not (eq diff-font-lock-syntax 'hunk-only))) ...) ...)
  (let* ((hunk (buffer-substring-no-properties beg end)) ...) ...
  diff-syntax-fontify-hunk(10530 14909 t)
  diff-syntax-fontify(10530 14909)
  #f(compiled-function (beg end) #<bytecode 0x1cb64d70b7d4e0e5>)(10530 14909)
  diff--iterate-hunks(12505 #f(compiled-function (beg end) #<bytecode 0x1cb64d70b7d4e0e5>))
  diff--font-lock-syntax(12505)
  font-lock-fontify-keywords-region(11965 12505 nil)
  font-lock-default-fontify-region(11980 12480 nil)
  font-lock-fontify-region(11980 12480)
  #f(compiled-function (fun) #<bytecode 0x303b13b2afd8864>)(font-lock-fontify-region)
  run-hook-wrapped(#f(compiled-function (fun) #<bytecode 0x303b13b2afd8864>) font-lock-fontify-region)
  jit-lock--run-functions(11980 12480)
  jit-lock-fontify-now(11980 12480)
  jit-lock-function(11980)
  redisplay_internal\ \(C\ function\)()

>> Stefan, do you think diff-syntax-fontify-props should let-bind
>> after-change-major-mode-hook to nil to not allow running this hook
>> on an internal buffer.
>
> No, that's definitely not the right fix.  It's at best a weak
> workaround, but it won't handle the case where the code that depends on
> `buffer-file-name` is placed on `foo-mode-hook` rather than on
> `after-change-major-mode-hook`.

In fact, eglot recommends adding such hooks:

  (add-hook 'foo-mode-hook 'eglot-ensure)

where eglot-ensure relies on buffer-file-name, so such fix is not possible indeed.

>>> If I read correctly, diff-syntax-fontify-props sets buffer-file-name of
>>> a temporary buffer to an existing one.  This is not necessarily a bug,
>>> but it definitely looks strange that we have two buffers with different
>>> contents and the same buffer-file-name.
>
> Yes, it's definitely borderline.  I remember feeling a bit uneasy about
> it at the time.  Maybe a better fix would be to be able to pass the
> file-name to `set-auto-mode` (or some new function created for that
> purpose) as an argument (instead of passing it via dynamic scoping in
> `buffer-file-name`).

The problem is that the right value of buffer-file-name is required
also by other more deep functions that set local variables like
dir-locals-collect-variables.

> BTW, I see somewhat similar code in `xref--collect-matches`, so maybe
> there's a need for a more general solution (haven't looked any further,
> but I'd be surprised if there aren't other cases).

Another example is mm-display-inline-fontify in gnus/mm-view.el,
so the problem is more widespread, and perhaps not much could be done
more than already using temporary buffers as an indication
for external packages that these buffers are for internal use only.

>> Or it's the responsibility of the eglot package to check for the
>> leading space in the buffer name when its after-change-major-mode-hook
>> is called?
>
> The eglot package could protect itself from such things by making its
> `after-change-major-mode-hook` just add the buffer to a global var, and
> then process this global var in `post-command-hook` or in a timer.

Felician, do you think that the eglot package should take more care
to protect itself, so this report could be closed?



Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Juri Linkov-2
> Passing it down as an arg is indeed problematic.

> Maybe we should just add yet-another buffer-<foo>-filename and change
> the corresponding places to so that instead of consulting
>
>     buffer-file-name
>
> they consult
>
>     (or buffer-file-name buffer-<foo>-filename)
>
> Maybe this could also be used for things like `vc-revision-other-window`.
This requires more testing, but at least such patch
basically would look like this:


diff --git a/lisp/files.el b/lisp/files.el
index 683f4a8ce7..e2721f43fc 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -3080,6 +3080,8 @@ magic-mode-regexp-match-limit
   "Upper limit on `magic-mode-alist' regexp matches.
 Also applies to `magic-fallback-mode-alist'.")
 
+(defvar-local buffer-file-name-for-mode nil)
+
 (defun set-auto-mode (&optional keep-mode-if-same)
   "Select major mode appropriate for current buffer.
 
@@ -3197,11 +3199,10 @@ set-auto-mode
   (set-auto-mode-0 done keep-mode-if-same)))
     ;; Next compare the filename against the entries in auto-mode-alist.
     (unless done
-      (if buffer-file-name
-  (let ((name buffer-file-name)
- (remote-id (file-remote-p buffer-file-name))
- (case-insensitive-p (file-name-case-insensitive-p
-     buffer-file-name)))
+      (if (or buffer-file-name buffer-file-name-for-mode)
+  (let* ((name (or buffer-file-name buffer-file-name-for-mode))
+ (remote-id (file-remote-p name))
+ (case-insensitive-p (file-name-case-insensitive-p name)))
     ;; Remove backup-suffixes from file name.
     (setq name (file-name-sans-versions name))
     ;; Remove remote file name identification.
@@ -3459,6 +3460,8 @@ hack-local-variables-confirm
     (let ((name (cond (dir-name)
       (buffer-file-name
        (file-name-nondirectory buffer-file-name))
+      (buffer-file-name-for-mode
+       (file-name-nondirectory buffer-file-name-for-mode))
       ((concat "buffer " (buffer-name)))))
   (offer-save (and (eq enable-local-variables t)
    unsafe-vars))
diff --git a/lisp/gnus/mm-view.el b/lisp/gnus/mm-view.el
index a6be04e313..0c2b985956 100644
--- a/lisp/gnus/mm-view.el
+++ b/lisp/gnus/mm-view.el
@@ -482,7 +482,7 @@ mm-display-inline-fontify
         ;; aren't a problem any more.  So we could probably get rid of this
         ;; setting now, but it seems harmless and potentially still useful.
  (set (make-local-variable 'font-lock-mode-hook) nil)
-        (setq buffer-file-name (mm-handle-filename handle))
+        (setq buffer-file-name-for-mode (mm-handle-filename handle))
  (with-demoted-errors
     (if mode
                 (save-window-excursion
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 1a34456340..4290eeb66a 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1391,7 +1391,7 @@ xref--collect-matches
           (insert-file-contents file nil 0 200)
           ;; Can't (setq-local delay-mode-hooks t) because of
           ;; bug#23272, but the performance penalty seems minimal.
-          (let ((buffer-file-name file)
+          (let ((buffer-file-name-for-mode file)
                 (inhibit-message t)
                 message-log-max)
             (ignore-errors
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 2dbab80208..cff0a5a103 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2718,7 +2718,7 @@ diff-syntax-fontify-props
     ;; temp buffer.
     (cl-assert (null buffer-file-name))
     (let ((enable-local-variables :safe) ;; to find `mode:'
-          (buffer-file-name file))
+          (buffer-file-name-for-mode file))
       (set-auto-mode)
       ;; FIXME: Is this really worth the trouble?
       (when (and (fboundp 'generic-mode-find-file-hook)
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index f64b6c0631..79f49ab68b 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2097,7 +2099,7 @@ vc-find-revision-no-save
                 (if buffer
                     ;; For non-interactive, skip any questions
                     (let ((enable-local-variables :safe) ;; to find `mode:'
-                          (buffer-file-name file))
+                          (buffer-file-name-for-mode file))
                       (ignore-errors (set-auto-mode)))
                   (normal-mode))
         (set-buffer-modified-p nil)
Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Juri Linkov-2
In reply to this post by Juri Linkov-2
>> Now I tried to repeat this problem with eglot and got such backtrace:
> [...]
>
> Did you use the ELPA version or the git master?

The backtrace was from the ELPA version, but trying it
in the git master signals a different error:

Error in post-command-hook:
(error "[eglot] Can't guess mode to manage for ` *diff-syntax:...

> Having read your discussion I have a feeling that this issue affects
> more than just Eglot.  Nevertheless we plan a feature that postpones the
> activation of Eglot and this feature is somewhat similar to Stefan's
> first suggestion.  So if the only concern is Eglot, then this issue can
> be closed.

Since it will take much time when the fix in Emacs will appear in a released
version, a workaround needs to be added to Eglot immediately.



Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Stefan Monnier
In reply to this post by Juri Linkov-2
> This requires more testing, but at least such patch
> basically would look like this:

This looks pretty good to me.  I wonder what Eli thinks about it.

> +(defvar-local buffer-file-name-for-mode nil)

Clearly we'd need a docstring here.

> +      (if (or buffer-file-name buffer-file-name-for-mode)

And now that I think about it, maybe

    (or buffer-file-name-for-mode buffer-file-name)

would be preferable (in case we want to set the major mode of a file
buffer based o a different file name).


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Eli Zaretskii
> From: Stefan Monnier <[hidden email]>
> Date: Mon, 27 Jan 2020 20:30:31 -0500
> Cc: [hidden email], Felician Nemeth <[hidden email]>
>
> > This requires more testing, but at least such patch
> > basically would look like this:
>
> This looks pretty good to me.  I wonder what Eli thinks about it.

I wasn't following this thread closely, so I don't have a clear idea
what problems does this change try to solve.  Could you humor me with
a summary, please?  Why isn't buffer-file-name enough?



Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Stefan Monnier
> I wasn't following this thread closely, so I don't have a clear idea
> what problems does this change try to solve.  Could you humor me with
> a summary, please?  Why isn't buffer-file-name enough?

Some packages set `buffer-file-name` in temp-buffers in order to
activate the major mode of the corresponding file.  As you can see in
the patch it's used in various circumstances.

Most of the time this is harmless because it's transient, but it is
fundamentally a lie (it more or less claims that this temp buffer holds
the content of that file, even though it's not the case (not only
because the content doesn't match, but the buffer is usually not fully
set up as a proper file buffer, it can lead to get-file-buffer returning
the wrong buffer, ...) and even though it's transient it can cause
problems because hooks are run during this time (e.g. major mode hooks)
which may take action under the mistaken assumption that this buffer
really correspond to the file.

In the original bug report the problem was that the major mode activated
`eglot-mode` in this temp buffer.  There are many ways for this to
create problems.  The immediate error can be avoided in eglot of course,
but I think the problem goes deeper and we should fix it by making it
possible to set the major mode without having to lie about
`buffer-file-name`.


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Felician Nemeth
In reply to this post by Juri Linkov-2
>>> Now I tried to repeat this problem with eglot and got such backtrace:
>> [...]
>>
>> Did you use the ELPA version or the git master?
>
> The backtrace was from the ELPA version, but trying it
> in the git master signals a different error:
>
> Error in post-command-hook:
> (error "[eglot] Can't guess mode to manage for ` *diff-syntax:...

That's strange.  I was able to reproduce the original bug report
following this recipe:
https://github.com/joaotavora/eglot/pull/233#issuecomment-466691992
I wonder if your setup is different is some way.

>> Having read your discussion I have a feeling that this issue affects
>> more than just Eglot.  Nevertheless we plan a feature that postpones the
>> activation of Eglot and this feature is somewhat similar to Stefan's
>> first suggestion.  So if the only concern is Eglot, then this issue can
>> be closed.
>
> Since it will take much time when the fix in Emacs will appear in a released
> version, a workaround needs to be added to Eglot immediately.

OK, but this feature ("Better syntax highlighting of Diff hunks") will
first appear in Emacs 27.



Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Eli Zaretskii
In reply to this post by Stefan Monnier
> From: Stefan Monnier <[hidden email]>
> Cc: [hidden email],  [hidden email],  [hidden email]
> Date: Tue, 28 Jan 2020 08:58:32 -0500
>
> Some packages set `buffer-file-name` in temp-buffers in order to
> activate the major mode of the corresponding file.  As you can see in
> the patch it's used in various circumstances.
>
> Most of the time this is harmless because it's transient, but it is
> fundamentally a lie (it more or less claims that this temp buffer holds
> the content of that file, even though it's not the case (not only
> because the content doesn't match, but the buffer is usually not fully
> set up as a proper file buffer, it can lead to get-file-buffer returning
> the wrong buffer, ...) and even though it's transient it can cause
> problems because hooks are run during this time (e.g. major mode hooks)
> which may take action under the mistaken assumption that this buffer
> really correspond to the file.
>
> In the original bug report the problem was that the major mode activated
> `eglot-mode` in this temp buffer.  There are many ways for this to
> create problems.  The immediate error can be avoided in eglot of course,
> but I think the problem goes deeper and we should fix it by making it
> possible to set the major mode without having to lie about
> `buffer-file-name`.

Thanks.

So this is because those modes set buffer-file-name instead of
activating the major mode directly?  And by setting buffer-file-name
they by side effect tell unrelated features that this buffer is
associated with a file?

If so, then I'm not sure I understand the solution.  The offending
modes will have to be modified to use this new variable instead of
buffer-file-name, no?  And if we have to modify them, then why not do
TRT while at that, i.e. call the major mode directly instead of
setting some variable?  Or what am I missing?



Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Stefan Monnier
> So this is because those modes

They're not "modes", really, they're features that create a temp buffer
related to file FOO and need to set this buffer to the same mode as file FOO
(e.g. this happens in vc-revision-other-window, or also in diff-mode
where we grab a hunk's text and want to put it in its corresponding
major mode to give it mode-specific highlighting).

> set buffer-file-name instead of activating the major mode directly?

Right: they don't know which major mode should be used, so they set
buffer-file-name and then call set-auto-mode (which looks up
auto-mode-alist and directory local vars appropriately).

> And by setting buffer-file-name they by side effect tell unrelated
> features that this buffer is associated with a file?

Indeed.

> If so, then I'm not sure I understand the solution.  The offending
> modes will have to be modified to use this new variable instead of
> buffer-file-name, no?

That's what the patch does, yes (modulo the fact that they're not
really "modes").

> And if we have to modify them, then why not do TRT while at that,
> i.e. call the major mode directly instead of setting some variable?

We set the variable in order to find out what mode to use.


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Eli Zaretskii
> From: Stefan Monnier <[hidden email]>
> Cc: [hidden email],  [hidden email],  [hidden email]
> Date: Tue, 28 Jan 2020 15:12:03 -0500
>
> > And if we have to modify them, then why not do TRT while at that,
> > i.e. call the major mode directly instead of setting some variable?
>
> We set the variable in order to find out what mode to use.

Sorry, I don't understand.  I meant to ask why the code which sets
this new variable cannot call the major mode function instead?



Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Juri Linkov-2
In reply to this post by Stefan Monnier
>> it can cause
>> problems because hooks are run during this time (e.g. major mode hooks)
>> which may take action under the mistaken assumption that this buffer
>> really correspond to the file
>
> I think diff-mode could also use delay-mode-hooks and then run them (or
> not) outside of that let binding.

Interesting idea, I see no problem with this:

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 2dbab80208..9035f7643a 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2719,7 +2719,7 @@ diff-syntax-fontify-props
     (cl-assert (null buffer-file-name))
     (let ((enable-local-variables :safe) ;; to find `mode:'
           (buffer-file-name file))
-      (set-auto-mode)
+      (delay-mode-hooks (set-auto-mode))
       ;; FIXME: Is this really worth the trouble?
       (when (and (fboundp 'generic-mode-find-file-hook)
                  (memq #'generic-mode-find-file-hook



Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Stefan Monnier
In reply to this post by Eli Zaretskii
> Sorry, I don't understand.  I meant to ask why the code which sets
> this new variable cannot call the major mode function instead?

The code does basically:

    (let ((buffer-file-name FOO))
      (set-auto-mode))

How could it "call the major mode function instead"?


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Stefan Monnier
> But then did you consider alternatives to yet another magic
> buffer-local variable?  Two possibilities come to mind:
>
>  . change set-auto-mode to accept another optional argument, the file
>    name to use to look up the mode
>
>  . perform look up of auto-mode-alist, then invoke the mode directly

The issue is that we also want to obey dir-locals and both above options
seem to become more invasive once we try and handle those (the first
above option is the first one I proposed, since I prefer
lexically-scoped args over dynamically-scoped vars ;-)

> Also, setting the pseudo-filename is not guaranteed to turn on the
> mode according to that file name.  Not sure if this matters in these
> cases.

Not sure what you mean here.

> And finally, I cannot say that I like this part of the patch:
>
>   @@ -3459,6 +3460,8 @@ hack-local-variables-confirm
>        (let ((name (cond (dir-name)
> (buffer-file-name
> (file-name-nondirectory buffer-file-name))
>   +      (buffer-file-name-for-mode
>   +       (file-name-nondirectory buffer-file-name-for-mode))
> ((concat "buffer " (buffer-name)))))
>    (offer-save (and (eq enable-local-variables t)
>     unsafe-vars))
>
> If buffer-file-name-for-mode is not really a file name, we shouldn't
> call file-name-nondirectory on it.

It is supposed to be a file name.  It's only that the buffer is not
supposed to be the buffer corresponding to that file.

> If nothing else, it will signal an
> error if buffer-file-name-for-mode is nil.

That code is predicated on `buffer-file-name-for-mode` being
non-nil, AFAICT, so I think we're OK in this regard.


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Juri Linkov-2
In reply to this post by Felician Nemeth
>> Since it will take much time when the fix in Emacs will appear in a released
>> version, a workaround needs to be added to Eglot immediately.
>
> OK, but this feature ("Better syntax highlighting of Diff hunks") will
> first appear in Emacs 27.

Right, so it needs to be fixed in Emacs 27.

Could you please try to reproduce the issue using Eglot with the
following minimal patch.  If it really fixes the bug then it
should be committed to Emacs 27 immediately, and more changes
could be added later.

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 2dbab80208..9035f7643a 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2719,7 +2719,7 @@ diff-syntax-fontify-props
     (cl-assert (null buffer-file-name))
     (let ((enable-local-variables :safe) ;; to find `mode:'
           (buffer-file-name file))
-      (set-auto-mode)
+      (delay-mode-hooks (set-auto-mode))
       ;; FIXME: Is this really worth the trouble?
       (when (and (fboundp 'generic-mode-find-file-hook)
                  (memq #'generic-mode-find-file-hook





Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Eli Zaretskii
In reply to this post by Stefan Monnier
> From: Stefan Monnier <[hidden email]>
> Cc: [hidden email],  [hidden email],  [hidden email]
> Date: Wed, 29 Jan 2020 16:33:31 -0500
>
> > But then did you consider alternatives to yet another magic
> > buffer-local variable?  Two possibilities come to mind:
> >
> >  . change set-auto-mode to accept another optional argument, the file
> >    name to use to look up the mode
> >
> >  . perform look up of auto-mode-alist, then invoke the mode directly
>
> The issue is that we also want to obey dir-locals and both above options
> seem to become more invasive once we try and handle those (the first
> above option is the first one I proposed, since I prefer
> lexically-scoped args over dynamically-scoped vars ;-)

What is the problem of the first alternative wrt dir-locals?  I don't
think I understand that.

> > Also, setting the pseudo-filename is not guaranteed to turn on the
> > mode according to that file name.  Not sure if this matters in these
> > cases.
>
> Not sure what you mean here.

set-auto-mode looks on other mode-determining stuff, before looking at
the file name.

> > And finally, I cannot say that I like this part of the patch:
> >
> >   @@ -3459,6 +3460,8 @@ hack-local-variables-confirm
> >        (let ((name (cond (dir-name)
> > (buffer-file-name
> > (file-name-nondirectory buffer-file-name))
> >   +      (buffer-file-name-for-mode
> >   +       (file-name-nondirectory buffer-file-name-for-mode))
> > ((concat "buffer " (buffer-name)))))
> >    (offer-save (and (eq enable-local-variables t)
> >     unsafe-vars))
> >
> > If buffer-file-name-for-mode is not really a file name, we shouldn't
> > call file-name-nondirectory on it.
>
> It is supposed to be a file name.  It's only that the buffer is not
> supposed to be the buffer corresponding to that file.

Yes, but having leading directories in it feels... unclean, even more
than just having this variable.

> > If nothing else, it will signal an
> > error if buffer-file-name-for-mode is nil.
>
> That code is predicated on `buffer-file-name-for-mode` being
> non-nil, AFAICT, so I think we're OK in this regard.

Non-nil doesn't mean it's a string.



Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Stefan Monnier
>> >  . change set-auto-mode to accept another optional argument, the file
>> >    name to use to look up the mode
>> >
>> >  . perform look up of auto-mode-alist, then invoke the mode directly
>>
>> The issue is that we also want to obey dir-locals and both above options
>> seem to become more invasive once we try and handle those (the first
>> above option is the first one I proposed, since I prefer
>> lexically-scoped args over dynamically-scoped vars ;-)
>
> What is the problem of the first alternative wrt dir-locals?  I don't
> think I understand that.

As I wrote: it's more invasive because you have to pass that extra arg
through various functions.

>> > Also, setting the pseudo-filename is not guaranteed to turn on the
>> > mode according to that file name.  Not sure if this matters in these
>> > cases.
>> Not sure what you mean here.
> set-auto-mode looks on other mode-determining stuff, before looking at
> the file name.

Ah, right.  This is a side-issue, yes: in cases like
`vc-revision-other-window` it does exactly what we want (i.e. it takes
advantage of the buffer's content (file-local vars, magic strings, ...)
to guess the mode), but in cases like diff-mode it can be detrimental
(since we don't have the full file content in that case, but only
a chunk of it, which means that things like file-local vars not only
will usually not be found but can even be incorrectly found).

I don't think it makes much difference to the current discussion, tho
(it's largely orthogonal).

>> > And finally, I cannot say that I like this part of the patch:
>> >
>> >   @@ -3459,6 +3460,8 @@ hack-local-variables-confirm
>> >        (let ((name (cond (dir-name)
>> > (buffer-file-name
>> > (file-name-nondirectory buffer-file-name))
>> >   +      (buffer-file-name-for-mode
>> >   +       (file-name-nondirectory buffer-file-name-for-mode))
>> > ((concat "buffer " (buffer-name)))))
>> >    (offer-save (and (eq enable-local-variables t)
>> >     unsafe-vars))
>> >
>> > If buffer-file-name-for-mode is not really a file name, we shouldn't
>> > call file-name-nondirectory on it.
>>
>> It is supposed to be a file name.  It's only that the buffer is not
>> supposed to be the buffer corresponding to that file.
>
> Yes, but having leading directories in it feels... unclean,

Why so?  The leading directories are crucial for dir-locals support, and
they can also be important for auto-mode-alist.

>> That code is predicated on `buffer-file-name-for-mode` being
>> non-nil, AFAICT, so I think we're OK in this regard.
> Non-nil doesn't mean it's a string.

If someone sets it to non-nil and not a string, they'll get what
they deserve.  Same holds already for `buffer-file-name`, so it's not
a new problem introduced with this change.


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Felician Nemeth
In reply to this post by Juri Linkov-2
> Could you please try to reproduce the issue using Eglot with the
> following minimal patch.  If it really fixes the bug then it
> should be committed to Emacs 27 immediately, and more changes
> could be added later.

Unfortunately the minimal patch doesn't fix the issue.

Case 1. When Eglot is started manually, then the recipe fails in an
after-change-major-mode-hook where the buffer-file-name is non-nil for
the temporary diff buffer.

Case 2. With (add-to-list 'python-mode-hook 'eglot-ensure) the recipe
fails in a post-command-hook, because eglot-ensure has this:

      (when buffer-file-name
        (add-hook 'post-command-hook #'maybe-connect 'append nil)))))

And buffer-file-name is non-nil for the diff buffer during the execution
of python-mode-hook.  But during the execution of maybe-connect in the
post-command-hook, the buffer-file-name is nil and that leads to an
error.



Reply | Threaded
Open this post in threaded view
|

bug#39190: 28.0.50; two buffers with same buffer-file-name (diff-syntax-fontify-props)

Juri Linkov-2
>> Could you please try to reproduce the issue using Eglot with the
>> following minimal patch.  If it really fixes the bug then it
>> should be committed to Emacs 27 immediately, and more changes
>> could be added later.
>
> Unfortunately the minimal patch doesn't fix the issue.
>
> Case 1. When Eglot is started manually, then the recipe fails in an
> after-change-major-mode-hook where the buffer-file-name is non-nil for
> the temporary diff buffer.

Strange, when I tested the minimal patch, there were no errors anymore.

Before the patch, I got this error in the git master when trying
to use 'C-c C-r' (diff-reverse-direction) in the diff buffer:

  Debugger entered--Lisp error: (wrong-type-argument arrayp nil)
    substring(nil 0 1)
    file-truename(nil)

But after applying the patch, after-change-major-mode-hook is not called
anymore in the temporary diff buffer.

> Case 2. With (add-to-list 'python-mode-hook 'eglot-ensure) the recipe
> fails in a post-command-hook, because eglot-ensure has this:
>
>       (when buffer-file-name
>         (add-hook 'post-command-hook #'maybe-connect 'append nil)))))
>
> And buffer-file-name is non-nil for the diff buffer during the execution
> of python-mode-hook.  But during the execution of maybe-connect in the
> post-command-hook, the buffer-file-name is nil and that leads to an
> error.

I guess Eglot should have more safeguarding, i.e. the same check
'when buffer-file-name' could be added to 'maybe-connect' as well.



12