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

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

[Felician Nemeth]

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.

Thank you.

In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
 of 2020-01-19 built on betli
Repository revision: 35a1a007bb7506c72ee6d9757a79014c679e7bae
Repository branch: emacs-27
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

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

[Juri Linkov]

X-Debbugs-CC: Stefan Monnier <monnier@iro.umontreal.ca>

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.

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

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

[Juri Linkov]

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?

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

[Stefan Monnier]

>> 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.

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`.


        Stefan

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

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

> 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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: buffer-file-name-for-mode.patch --]
[-- Type: text/x-diff, Size: 3838 bytes --]

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)

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

[Stefan Monnier]

> 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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 27 Jan 2020 20:30:31 -0500
> Cc: 39190@debbugs.gnu.org, Felician Nemeth <felician.nemeth@gmail.com>
> 
> > 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?

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

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

[Dmitry Gutov]

On 28.01.2020 16:58, Stefan Monnier wrote:
> 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.

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

[Juri Linkov]

>> 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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: juri@linkov.net,  39190@debbugs.gnu.org,  felician.nemeth@gmail.com
> 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?

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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: juri@linkov.net,  39190@debbugs.gnu.org,  felician.nemeth@gmail.com
> 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?

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

[Stefan Monnier]

> 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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: juri@linkov.net,  39190@debbugs.gnu.org,  felician.nemeth@gmail.com
> Date: Tue, 28 Jan 2020 18:17:23 -0500
> 
> > 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))

Ah, okay.

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

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.

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.  If nothing else, it will signal an
error if buffer-file-name-for-mode is nil.

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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: juri@linkov.net,  39190@debbugs.gnu.org,  felician.nemeth@gmail.com
> 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.

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

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

[Juri Linkov]

> And now that I think about it, maybe
>
>     (or buffer-file-name-for-mode buffer-file-name)

I encountered more problems with buffer-file-name-for-mode
because there are more unexpected places that expect buffer-file-name:

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  find-file-name-handler(nil file-name-sans-versions)
  file-name-sans-versions(nil)
  generic-mode-find-file-hook()
  diff-syntax-fontify-props(#("/dev/null" 0 9 (face (diff-file-header diff-header) fontified t)) "" (0 0) t)
  diff-syntax-fontify-hunk(1686 3081 t)
  diff-syntax-fontify(1686 3081)

because it relies on non-nil buffer-file-name:

(defun generic-mode-find-file-hook ()
  ...
  (when (and (eq major-mode 'fundamental-mode)
             (or (null generic-ignore-files-regexp)
                 (not (string-match-p
                       generic-ignore-files-regexp
                       (file-name-sans-versions buffer-file-name)))))
                                                ================

It would be hard to find all occurrences of buffer-file-name
where to add buffer-file-name-for-mode.

Maybe safer would be to use just (delay-mode-hooks (set-auto-mode))

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

[Stefan Monnier]

>   generic-mode-find-file-hook()

Add that to the list of reasons why I loathe generic.el (and think that
diff-mode shouldn't call it explicitly).


        Stefan

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

[Dmitry Gutov]

On 31.01.2020 1:50, Juri Linkov wrote:
> Maybe safer would be to use just (delay-mode-hooks (set-auto-mode))

That would be my choice as well.

BTW, xref--collect-matches uses buffer-file-name in a similar way, 
curious how a problem with Eglot (or etc) never came up.

Also, see the comment there why it couldn't use delay-mode-hooks at the 
time (maybe it can now, if we're okay with entirely unsupporting 
syntax-propertize-via-font-lock).

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

[Felician Nemeth]

> Now I tried to repeat this problem with eglot and got such backtrace:
[...]

Did you use the ELPA version or the git master?

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

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.

I haven't completely understood Stefan's second suggestion of
buffer-<foo>-filename, but if this means temporary diff buffers can be
detected similarly how save-some-buffers detects indirect buffers with
buffer-base-buffer, then I support the idea.

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

[Juri Linkov]

>> 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.

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

[Felician Nemeth]

>>> 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.

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

[Juri Linkov]

>> 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

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

[Felician Nemeth]

> 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.

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

[Juri Linkov]

>> 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.

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

[Felician Nemeth]

Juri Linkov <juri@linkov.net> writes:

>>> 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.

Maybe I wasn't clear enough.  I got no errors, but flymake incorrectly
fontified the buffer.  Anyway, I tried to look into why this happened
even after applying your patch and it turned out that
vc-find-revision-no-save also calls set-auto-mode.  So, I don't know if
the following patch is correct, but together with your patch it does
solve the original issue.

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index f64b6c0631..c50ba132e7 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2098,7 +2098,7 @@ vc-find-revision-no-save
                     ;; For non-interactive, skip any questions
                     (let ((enable-local-variables :safe) ;; to find `mode:'
                           (buffer-file-name file))
-                      (ignore-errors (set-auto-mode)))
+                      (ignore-errors (delay-mode-hooks (set-auto-mode))))
                   (normal-mode))
 	        (set-buffer-modified-p nil)
                 (setq buffer-read-only t))

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

[Stefan Monnier]

> vc-find-revision-no-save also calls set-auto-mode.  So, I don't know if
> the following patch is correct, but together with your patch it does
> solve the original issue.
>
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index f64b6c0631..c50ba132e7 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -2098,7 +2098,7 @@ vc-find-revision-no-save
>                      ;; For non-interactive, skip any questions
>                      (let ((enable-local-variables :safe) ;; to find `mode:'
>                            (buffer-file-name file))
> -                      (ignore-errors (set-auto-mode)))
> +                      (ignore-errors (delay-mode-hooks (set-auto-mode))))
>                    (normal-mode))
>  	        (set-buffer-modified-p nil)
>                  (setq buffer-read-only t))

How 'bout we first consolidate the two cases into one by introducing
a new function `set-auto-mode-for-filename` that both
vc-find-revision-no-save and diff-mode can use?


        Stefan

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

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

>> vc-find-revision-no-save also calls set-auto-mode.  So, I don't know if
>> the following patch is correct, but together with your patch it does
>> solve the original issue.
>>
>> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
>> index f64b6c0631..c50ba132e7 100644
>> --- a/lisp/vc/vc.el
>> +++ b/lisp/vc/vc.el
>> @@ -2098,7 +2098,7 @@ vc-find-revision-no-save
>>                      ;; For non-interactive, skip any questions
>>                      (let ((enable-local-variables :safe) ;; to find `mode:'
>>                            (buffer-file-name file))
>> -                      (ignore-errors (set-auto-mode)))
>> +                      (ignore-errors (delay-mode-hooks (set-auto-mode))))
>>                    (normal-mode))
>>  	        (set-buffer-modified-p nil)
>>                  (setq buffer-read-only t))
>
> How 'bout we first consolidate the two cases into one by introducing
> a new function `set-auto-mode-for-filename` that both
> vc-find-revision-no-save and diff-mode can use?

Actually vc-find-revision-no-save when called as a command (e.g. by the
key 'f' from log-view) still should run hooks because the buffer it creates
is not internal.

Only its call from diff-syntax-fontify-hunk should not run hooks:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: delay-mode-hooks.patch --]
[-- Type: text/x-diff, Size: 2450 bytes --]

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 2dbab80208..9cdd732923 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2635,11 +2635,11 @@ diff-syntax-fontify-hunk
                          ;; Don't re-initialize the buffer (which would throw
                          ;; away the previous fontification work).
                          (setq file nil)
-                       (setq buffer (ignore-errors
+                       (setq buffer (ignore-errors (delay-mode-hooks
                                       (vc-find-revision-no-save
                                        file revision
                                        diff-vc-backend
-                                       (get-buffer-create buffer-name)))))
+                                       (get-buffer-create buffer-name))))))
                      (when buffer
                        (with-current-buffer buffer
                          (diff-syntax-fontify-props file text line-nb))))))))
@@ -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
diff --git a/lisp/gnus/mm-view.el b/lisp/gnus/mm-view.el
index a6be04e313..e629674ea6 100644
--- a/lisp/gnus/mm-view.el
+++ b/lisp/gnus/mm-view.el
@@ -497,7 +497,7 @@ mm-display-inline-fontify
 	    (let ((auto-mode-alist
 		   (delq (rassq 'doc-view-mode-maybe auto-mode-alist)
 			 (copy-sequence auto-mode-alist))))
-	      (set-auto-mode)
+	      (delay-mode-hooks (set-auto-mode))
 	      (setq mode major-mode)))
 	  ;; Do not fontify if the guess mode is fundamental.
 	  (unless (eq major-mode 'fundamental-mode)
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 1a34456340..741ed73905 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1395,7 +1395,7 @@ xref--collect-matches
                 (inhibit-message t)
                 message-log-max)
             (ignore-errors
-              (set-auto-mode t)))
+              (delay-mode-hooks (set-auto-mode t))))
           (setq-local xref--temp-buffer-file-name file)
           (setq-local inhibit-read-only t)
           (erase-buffer))

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

[Dmitry Gutov]

On 03.02.2020 2:41, Juri Linkov wrote:
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -1395,7 +1395,7 @@ xref--collect-matches
>                   (inhibit-message t)
>                   message-log-max)
>               (ignore-errors
> -              (set-auto-mode t)))
> +              (delay-mode-hooks (set-auto-mode t))))

Nope.

There's even a comment above these lines explaining why I didn't use 
delay-mode-hooks there.

I've even mentioned it in this thread already.

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

[Juri Linkov]

>> @@ -1395,7 +1395,7 @@ xref--collect-matches
>>                   (inhibit-message t)
>>                   message-log-max)
>>               (ignore-errors
>> -              (set-auto-mode t)))
>> +              (delay-mode-hooks (set-auto-mode t))))
>
> Nope.
>
> There's even a comment above these lines explaining why I didn't use
> delay-mode-hooks there.
>
> I've even mentioned it in this thread already.

I hoped that maybe this was solved somehow already.
But if there is no problem in using xref with Eglot,
then let's leave it unchanged.

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

[Dmitry Gutov]

On 04.02.2020 1:44, Juri Linkov wrote:
> I hoped that maybe this was solved somehow already.
> But if there is no problem in using xref with Eglot,
> then let's leave it unchanged.

There is a related bug report. As long as we support 
syntax-propertize-via-font-lock, let's not change it (unless some 
related bug reports come).

Eglot should have its own xref backend, maybe that's why it doesn't 
trigger it.

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

[Juri Linkov]

> Maybe I wasn't clear enough.  I got no errors, but flymake incorrectly
> fontified the buffer.  Anyway, I tried to look into why this happened
> even after applying your patch and it turned out that
> vc-find-revision-no-save also calls set-auto-mode.  So, I don't know if
> the following patch is correct, but together with your patch it does
> solve the original issue.
>
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index f64b6c0631..c50ba132e7 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -2098,7 +2098,7 @@ vc-find-revision-no-save
>                      ;; For non-interactive, skip any questions
>                      (let ((enable-local-variables :safe) ;; to find `mode:'
>                            (buffer-file-name file))
> -                      (ignore-errors (set-auto-mode)))
> +                      (ignore-errors (delay-mode-hooks (set-auto-mode))))
>                    (normal-mode))
>  	        (set-buffer-modified-p nil)
>                  (setq buffer-read-only t))

vc-find-revision-no-save should not use delay-mode-hooks when called as
a command.  Could you please try again with the following patch:

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 2dbab80208..9cdd732923 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2635,11 +2635,11 @@ diff-syntax-fontify-hunk
                          ;; Don't re-initialize the buffer (which would throw
                          ;; away the previous fontification work).
                          (setq file nil)
-                       (setq buffer (ignore-errors
+                       (setq buffer (ignore-errors (delay-mode-hooks
                                       (vc-find-revision-no-save
                                        file revision
                                        diff-vc-backend
-                                       (get-buffer-create buffer-name)))))
+                                       (get-buffer-create buffer-name))))))
                      (when buffer
                        (with-current-buffer buffer
                          (diff-syntax-fontify-props file text line-nb))))))))
@@ -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

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

[Stefan Monnier]

> vc-find-revision-no-save should not use delay-mode-hooks when called as
> a command.

That's the problem with the current approach: we end up making decisions
which can't be justified.  Whether mode hooks should be run or not
depends on the hook and various other things.  What we *do* know, OTOH
is that `vc-find-revision-no-save`s buffer is not associated with
a file, so setting `buffer-file-name` can result in bugs.


        Stefan

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

[Juri Linkov]

>> vc-find-revision-no-save should not use delay-mode-hooks when called as
>> a command.
>
> That's the problem with the current approach: we end up making decisions
> which can't be justified.  Whether mode hooks should be run or not
> depends on the hook and various other things.  What we *do* know, OTOH
> is that `vc-find-revision-no-save`s buffer is not associated with
> a file, so setting `buffer-file-name` can result in bugs.

delay-mode-hooks was intended as the safest workaround to install in emacs-27.

Using alternatives of buffer-file-name are not straightforward
and need more testing in emacs-28.

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

[Juri Linkov]

>> That's the problem with the current approach: we end up making decisions
>> which can't be justified.  Whether mode hooks should be run or not
>> depends on the hook and various other things.  What we *do* know, OTOH
>> is that `vc-find-revision-no-save`s buffer is not associated with
>> a file, so setting `buffer-file-name` can result in bugs.
>
> delay-mode-hooks was intended as the safest workaround to install in emacs-27.

delay-mode-hooks is now in emacs-27.

> Using alternatives of buffer-file-name are not straightforward
> and need more testing in emacs-28.

I don't know how to handle this more properly because some hooks
should be run with buffer-file-name, e.g. generic-mode-find-file-hook
that uses buffer-file-name to add more font-lock highlighting.
But some hooks like in Eglot assume that buffer-file-name associates the
buffer with a file.

Also I tried to use indirect buffers:

(let ((buffer (make-indirect-buffer
               (current-buffer)
               (generate-new-buffer-name (buffer-name)))))
  (funcall
   (unwind-protect
       (with-current-buffer buffer
         (let ((buffer-file-name "/tmp/foo.el"))
           (set-auto-mode)
           major-mode))
     (kill-buffer buffer))))

but not sure if it's enough to just set its deduced mode.

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

[Felician Nemeth]

>> delay-mode-hooks was intended as the safest workaround to install in emacs-27.
>
> delay-mode-hooks is now in emacs-27.

This makes the original problem disappear.  Thank you.

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

[Juri Linkov]

>>> delay-mode-hooks was intended as the safest workaround to install in emacs-27.
>>
>> delay-mode-hooks is now in emacs-27.
>
> This makes the original problem disappear.  Thank you.

Unfortunately, this change broke diff-syntax font-lock of conf-space-mode -
font-lock from files in conf-space-mode is not displayed in diffs anymore.

First, these steps show how it used to work when set-auto-mode was called
from diff-syntax-fontify-props without wrapping in delay-mode-hooks macro:

- diff-syntax-fontify-props calls set-auto-mode
- in auto-mode-alist a file extension is associated with conf-mode-maybe
- conf-mode-maybe calls conf-mode
- conf-mode checks for the variable 'delay-mode-hooks':
- initially delay-mode-hooks is nil, so call conf-space-mode
- conf-space-mode calls its parent mode conf-mode again
- when conf-mode is called again, its delay-mode-hooks is t
  and it sets its mode variables.

Now the failure caused by wrapping (delay-mode-hooks (set-auto-mode))
in diff-syntax-fontify-props is because on the first call of conf-mode,
the variable 'delay-mode-hooks' is already t, not nil as it was before
the change.

All this due to such trick used in conf-mode with these comments:

(defun conf-mode ()
  ...
  ;; `conf-mode' plays two roles: it's the parent of several sub-modes
  ;; but it's also the function that chooses between those submodes.
  ;; To tell the difference between those two cases where the function
  ;; might be called, we check `delay-mode-hooks'.
  ;; (adopted from tex-mode.el)
  (if (not delay-mode-hooks)
      ;; try to guess sub-mode of conf-mode based on buffer content

So it detects the situation when define-derived-mode runs the parent
with delay-mode-hooks.

I have no idea what to do in this case.

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

[Stefan Monnier]

> (defun conf-mode ()
>   ...
>   ;; `conf-mode' plays two roles: it's the parent of several sub-modes
>   ;; but it's also the function that chooses between those submodes.
>   ;; To tell the difference between those two cases where the function
>   ;; might be called, we check `delay-mode-hooks'.
>   ;; (adopted from tex-mode.el)
>   (if (not delay-mode-hooks)
>       ;; try to guess sub-mode of conf-mode based on buffer content

The Right Thing™ to do is of course to split `conf-mode` into two
functions, one for the generic entry point and the other for the shared
parent mode.

How easy/realistic this is, I don't know.


        Stefan

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

[Juri Linkov]

>> (defun conf-mode ()
>>   ...
>>   ;; `conf-mode' plays two roles: it's the parent of several sub-modes
>>   ;; but it's also the function that chooses between those submodes.
>>   ;; To tell the difference between those two cases where the function
>>   ;; might be called, we check `delay-mode-hooks'.
>>   ;; (adopted from tex-mode.el)
>>   (if (not delay-mode-hooks)
>>       ;; try to guess sub-mode of conf-mode based on buffer content
>
> The Right Thing™ to do is of course to split `conf-mode` into two
> functions, one for the generic entry point and the other for the shared
> parent mode.

Maybe a simpler variant would be to use some other variable to distinguish
sub-mode and its parent, instead of relying on delay-mode-hooks?

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

[Stefan Monnier]

> Maybe a simpler variant would be to use some other variable to distinguish
> sub-mode and its parent, instead of relying on delay-mode-hooks?

That's kinda hard if you want to use define-derived-mode and we do: when
we call `conf-foo-mode` the first things it does is: bind
`delay-mode-hooks` and call the parent (`conf-mode`), so the only
difference between this call to the parent and a direct call to the
generic entry point is the binding of `delay-mode-hooks`.


        Stefan

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

[Stefan Monnier]

>> Maybe a simpler variant would be to use some other variable to distinguish
>> sub-mode and its parent, instead of relying on delay-mode-hooks?
>
> That's kinda hard if you want to use define-derived-mode and we do: when
> we call `conf-foo-mode` the first things it does is: bind
> `delay-mode-hooks` and call the parent (`conf-mode`), so the only
> difference between this call to the parent and a direct call to the
> generic entry point is the binding of `delay-mode-hooks`.

IOW, I think we should start with something like the patch below which
makes `conf-guess-mode` available, after which we can start fixing the
various callers to call `conf-guess-mode` instead.


        Stefan


diff --git a/lisp/textmodes/conf-mode.el b/lisp/textmodes/conf-mode.el
index 86db698043..b3325eeff6 100644
--- a/lisp/textmodes/conf-mode.el
+++ b/lisp/textmodes/conf-mode.el
@@ -351,7 +351,37 @@ conf-outline-level
 \f
 
 ;;;###autoload
-(defun conf-mode ()
+(defun conf-guess-mode ()
+  "Enable the conf-*-mode that seems most appropriate."
+  (let ((unix 0) (win 0) (equal 0) (colon 0) (space 0) (jp 0))
+    (save-excursion
+      (goto-char (point-min))
+      (while (not (eobp))
+	(skip-chars-forward " \t\f")
+	(cond ((eq (char-after) ?\#) (setq unix (1+ unix)))
+	      ((eq (char-after) ?\;) (setq win (1+ win)))
+	      ((eq (char-after) ?\[))	; nop
+	      ((eolp))			; nop
+	      ((eq (char-after) ?}))	; nop
+	      ;; recognize at most double spaces within names
+	      ((looking-at "[^ \t\n=:]+\\(?:  ?[^ \t\n=:]+\\)*[ \t]*[=:]")
+	       (if (eq (char-before (match-end 0)) ?=)
+		   (setq equal (1+ equal))
+		 (setq colon (1+ colon))))
+	      ((looking-at "/[/*]") (setq jp (1+ jp)))
+	      ((looking-at ".*{"))		; nop
+	      ((setq space (1+ space))))
+	(forward-line)))
+    (cond
+     ((> jp (max unix win 3)) (conf-javaprop-mode))
+     ((> colon (max equal space)) (conf-colon-mode))
+     ((> space (max equal colon)) (conf-space-mode))
+     ((or (> win unix) (and (= win unix) (eq system-type 'windows-nt)))
+      (conf-windows-mode))
+     (t (conf-unix-mode)))))
+
+;;;###autoload
+(define-derived-mode conf-mode nil "Conf[?]"
   "Mode for Unix and Windows Conf files and Java properties.
 Most conf files know only three kinds of constructs: parameter
 assignments optionally grouped into sections and comments.  Yet
@@ -372,7 +402,7 @@ conf-mode
 quite right.  Patches that clearly identify some special case,
 without breaking the general ones, are welcome.
 
-If instead you start this mode with the generic `conf-mode'
+If instead you start this mode with the generic `conf-guess-mode'
 command, it will parse the buffer.  It will generally well
 identify the first four cases listed below.  If the buffer
 doesn't have enough contents to decide, this is identical to
@@ -381,46 +411,13 @@ conf-mode
 `conf-ppd-mode' and `conf-xdefaults-mode'.
 
 \\{conf-mode-map}"
-
-  (interactive)
-  ;; `conf-mode' plays two roles: it's the parent of several sub-modes
-  ;; but it's also the function that chooses between those submodes.
-  ;; To tell the difference between those two cases where the function
-  ;; might be called, we check `delay-mode-hooks'.
-  ;; (adopted from tex-mode.el)
+  ;; `conf-mode' is the parent of several sub-modes but it's also sometimes
+  ;; abused as the function that chooses between those submodes.  They should
+  ;; call `conf-guess-mode' directly, but for historical reasons, we try to
+  ;; accommodate those misuses with this ugly hack.
   (if (not delay-mode-hooks)
-      ;; try to guess sub-mode of conf-mode based on buffer content
-      (let ((unix 0) (win 0) (equal 0) (colon 0) (space 0) (jp 0))
-	(save-excursion
-	  (goto-char (point-min))
-	  (while (not (eobp))
-	    (skip-chars-forward " \t\f")
-	    (cond ((eq (char-after) ?\#) (setq unix (1+ unix)))
-		  ((eq (char-after) ?\;) (setq win (1+ win)))
-		  ((eq (char-after) ?\[))	; nop
-		  ((eolp))			; nop
-		  ((eq (char-after) ?}))	; nop
-		  ;; recognize at most double spaces within names
-		  ((looking-at "[^ \t\n=:]+\\(?:  ?[^ \t\n=:]+\\)*[ \t]*[=:]")
-		   (if (eq (char-before (match-end 0)) ?=)
-		       (setq equal (1+ equal))
-		     (setq colon (1+ colon))))
-		  ((looking-at "/[/*]") (setq jp (1+ jp)))
-		  ((looking-at ".*{"))		; nop
-		  ((setq space (1+ space))))
-	    (forward-line)))
-	(cond
-         ((> jp (max unix win 3)) (conf-javaprop-mode))
-         ((> colon (max equal space)) (conf-colon-mode))
-         ((> space (max equal colon)) (conf-space-mode))
-         ((or (> win unix) (and (= win unix) (eq system-type 'windows-nt)))
-          (conf-windows-mode))
-         (t (conf-unix-mode))))
-
-    (kill-all-local-variables)
-    (use-local-map conf-mode-map)
-    (setq major-mode 'conf-mode
-	  mode-name "Conf[?]")
+      ;; Try to guess sub-mode of conf-mode based on buffer content.
+      (conf-guess-mode)
     (set (make-local-variable 'font-lock-defaults)
          '(conf-font-lock-keywords nil t nil nil))
     ;; Let newcomment.el decide this for itself.
@@ -438,8 +435,7 @@ conf-mode
 	    ;; [section]
 	    (nil "^[ \t]*\\[[ \t]*\\(.+\\)[ \t]*\\]" 1)
 	    ;; section { ... }
-	    (nil "^[ \t]*\\([^=:{} \t\n][^=:{}\n]+\\)[ \t\n]*{" 1)))
-    (run-mode-hooks 'conf-mode-hook)))
+	    (nil "^[ \t]*\\([^=:{} \t\n][^=:{}\n]+\\)[ \t\n]*{" 1)))))
 
 (defun conf-mode-initialize (comment &optional font-lock)
   "Initializations for sub-modes of `conf-mode'.

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

[Juri Linkov]

>> That's kinda hard if you want to use define-derived-mode and we do: when
>> we call `conf-foo-mode` the first things it does is: bind
>> `delay-mode-hooks` and call the parent (`conf-mode`), so the only
>> difference between this call to the parent and a direct call to the
>> generic entry point is the binding of `delay-mode-hooks`.
>
> IOW, I think we should start with something like the patch below which
> makes `conf-guess-mode` available, after which we can start fixing the
> various callers to call `conf-guess-mode` instead.

I tested this with additional patch that replaces `conf-mode` calls with
`conf-guess-mode`, and see that syntax font-lock now is fixed in diff-mode
on some conf files (not tested on all possible conf submodes).

diff --git a/lisp/files.el b/lisp/files.el
index 683f4a8ce7..3e1cf5e526 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2813,7 +2813,7 @@ auto-mode-alist
      ("/config\\.\\(?:bat\\|log\\)\\'" . fundamental-mode)
      ("/\\.\\(authinfo\\|netrc\\)\\'" . authinfo-mode)
      ;; Windows candidates may be opened case sensitively on Unix
-     ("\\.\\(?:[iI][nN][iI]\\|[lL][sS][tT]\\|[rR][eE][gG]\\|[sS][yY][sS]\\)\\'" . conf-mode)
+     ("\\.\\(?:[iI][nN][iI]\\|[lL][sS][tT]\\|[rR][eE][gG]\\|[sS][yY][sS]\\)\\'" . conf-guess-mode)
      ("\\.la\\'" . conf-unix-mode)
      ("\\.ppd\\'" . conf-ppd-mode)
      ("java.+\\.conf\\'" . conf-javaprop-mode)
@@ -2822,15 +2822,15 @@ auto-mode-alist
      ("\\.desktop\\'" . conf-desktop-mode)
      ("/\\.redshift.conf\\'" . conf-windows-mode)
      ("\\`/etc/\\(?:DIR_COLORS\\|ethers\\|.?fstab\\|.*hosts\\|lesskey\\|login\\.?de\\(?:fs\\|vperm\\)\\|magic\\|mtab\\|pam\\.d/.*\\|permissions\\(?:\\.d/.+\\)?\\|protocols\\|rpc\\|services\\)\\'" . conf-space-mode)
-     ("\\`/etc/\\(?:acpid?/.+\\|aliases\\(?:\\.d/.+\\)?\\|default/.+\\|group-?\\|hosts\\..+\\|inittab\\|ksysguarddrc\\|opera6rc\\|passwd-?\\|shadow-?\\|sysconfig/.+\\)\\'" . conf-mode)
+     ("\\`/etc/\\(?:acpid?/.+\\|aliases\\(?:\\.d/.+\\)?\\|default/.+\\|group-?\\|hosts\\..+\\|inittab\\|ksysguarddrc\\|opera6rc\\|passwd-?\\|shadow-?\\|sysconfig/.+\\)\\'" . conf-guess-mode)
      ;; ChangeLog.old etc.  Other change-log-mode entries are above;
      ;; this has lower priority to avoid matching changelog.sgml etc.
      ("[cC]hange[lL]og[-.][-0-9a-z]+\\'" . change-log-mode)
      ;; either user's dot-files or under /etc or some such
-     ("/\\.?\\(?:gitconfig\\|gnokiirc\\|hgrc\\|kde.*rc\\|mime\\.types\\|wgetrc\\)\\'" . conf-mode)
+     ("/\\.?\\(?:gitconfig\\|gnokiirc\\|hgrc\\|kde.*rc\\|mime\\.types\\|wgetrc\\)\\'" . conf-guess-mode)
      ;; alas not all ~/.*rc files are like this
-     ("/\\.\\(?:asound\\|enigma\\|fetchmail\\|gltron\\|gtk\\|hxplayer\\|mairix\\|mbsync\\|msmtp\\|net\\|neverball\\|nvidia-settings-\\|offlineimap\\|qt/.+\\|realplayer\\|reportbug\\|rtorrent\\.\\|screen\\|scummvm\\|sversion\\|sylpheed/.+\\|xmp\\)rc\\'" . conf-mode)
-     ("/\\.\\(?:gdbtkinit\\|grip\\|mpdconf\\|notmuch-config\\|orbital/.+txt\\|rhosts\\|tuxracer/options\\)\\'" . conf-mode)
+     ("/\\.\\(?:asound\\|enigma\\|fetchmail\\|gltron\\|gtk\\|hxplayer\\|mairix\\|mbsync\\|msmtp\\|net\\|neverball\\|nvidia-settings-\\|offlineimap\\|qt/.+\\|realplayer\\|reportbug\\|rtorrent\\.\\|screen\\|scummvm\\|sversion\\|sylpheed/.+\\|xmp\\)rc\\'" . conf-guess-mode)
+     ("/\\.\\(?:gdbtkinit\\|grip\\|mpdconf\\|notmuch-config\\|orbital/.+txt\\|rhosts\\|tuxracer/options\\)\\'" . conf-guess-mode)
      ("/\\.?X\\(?:default\\|resource\\|re\\)s\\>" . conf-xdefaults-mode)
      ("/X11.+app-defaults/\\|\\.ad\\'" . conf-xdefaults-mode)
      ("/X11.+locale/.+/Compose\\'" . conf-colon-mode)
@@ -2920,7 +2920,7 @@ conf-mode-maybe
 	  (goto-char (point-min))
 	  (looking-at "<\\?xml \\|<!-- \\|<!DOCTYPE ")))
       (xml-mode)
-    (conf-mode)))
+    (conf-guess-mode)))
 
 (defvar interpreter-mode-alist
   ;; Note: The entries for the modes defined in cc-mode.el (awk-mode

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

[Stefan Monnier]

> I tested this with additional patch that replaces `conf-mode` calls with
> `conf-guess-mode`, and see that syntax font-lock now is fixed in diff-mode
> on some conf files (not tested on all possible conf submodes).

This said.  Using delay-mode-hooks in the diff-syntax driver is an ugly
hack and is the real source of the problem, so splitting conf-mode into
two is basically a way to work around the breakage introduced by the hack.


        Stefan


> diff --git a/lisp/files.el b/lisp/files.el
> index 683f4a8ce7..3e1cf5e526 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -2813,7 +2813,7 @@ auto-mode-alist
>       ("/config\\.\\(?:bat\\|log\\)\\'" . fundamental-mode)
>       ("/\\.\\(authinfo\\|netrc\\)\\'" . authinfo-mode)
>       ;; Windows candidates may be opened case sensitively on Unix
> -     ("\\.\\(?:[iI][nN][iI]\\|[lL][sS][tT]\\|[rR][eE][gG]\\|[sS][yY][sS]\\)\\'" . conf-mode)
> +     ("\\.\\(?:[iI][nN][iI]\\|[lL][sS][tT]\\|[rR][eE][gG]\\|[sS][yY][sS]\\)\\'" . conf-guess-mode)
>       ("\\.la\\'" . conf-unix-mode)
>       ("\\.ppd\\'" . conf-ppd-mode)
>       ("java.+\\.conf\\'" . conf-javaprop-mode)
> @@ -2822,15 +2822,15 @@ auto-mode-alist
>       ("\\.desktop\\'" . conf-desktop-mode)
>       ("/\\.redshift.conf\\'" . conf-windows-mode)
>       ("\\`/etc/\\(?:DIR_COLORS\\|ethers\\|.?fstab\\|.*hosts\\|lesskey\\|login\\.?de\\(?:fs\\|vperm\\)\\|magic\\|mtab\\|pam\\.d/.*\\|permissions\\(?:\\.d/.+\\)?\\|protocols\\|rpc\\|services\\)\\'" . conf-space-mode)
> -     ("\\`/etc/\\(?:acpid?/.+\\|aliases\\(?:\\.d/.+\\)?\\|default/.+\\|group-?\\|hosts\\..+\\|inittab\\|ksysguarddrc\\|opera6rc\\|passwd-?\\|shadow-?\\|sysconfig/.+\\)\\'" . conf-mode)
> +     ("\\`/etc/\\(?:acpid?/.+\\|aliases\\(?:\\.d/.+\\)?\\|default/.+\\|group-?\\|hosts\\..+\\|inittab\\|ksysguarddrc\\|opera6rc\\|passwd-?\\|shadow-?\\|sysconfig/.+\\)\\'" . conf-guess-mode)
>       ;; ChangeLog.old etc.  Other change-log-mode entries are above;
>       ;; this has lower priority to avoid matching changelog.sgml etc.
>       ("[cC]hange[lL]og[-.][-0-9a-z]+\\'" . change-log-mode)
>       ;; either user's dot-files or under /etc or some such
> -     ("/\\.?\\(?:gitconfig\\|gnokiirc\\|hgrc\\|kde.*rc\\|mime\\.types\\|wgetrc\\)\\'" . conf-mode)
> +     ("/\\.?\\(?:gitconfig\\|gnokiirc\\|hgrc\\|kde.*rc\\|mime\\.types\\|wgetrc\\)\\'" . conf-guess-mode)
>       ;; alas not all ~/.*rc files are like this
> -     ("/\\.\\(?:asound\\|enigma\\|fetchmail\\|gltron\\|gtk\\|hxplayer\\|mairix\\|mbsync\\|msmtp\\|net\\|neverball\\|nvidia-settings-\\|offlineimap\\|qt/.+\\|realplayer\\|reportbug\\|rtorrent\\.\\|screen\\|scummvm\\|sversion\\|sylpheed/.+\\|xmp\\)rc\\'" . conf-mode)
> -     ("/\\.\\(?:gdbtkinit\\|grip\\|mpdconf\\|notmuch-config\\|orbital/.+txt\\|rhosts\\|tuxracer/options\\)\\'" . conf-mode)
> +     ("/\\.\\(?:asound\\|enigma\\|fetchmail\\|gltron\\|gtk\\|hxplayer\\|mairix\\|mbsync\\|msmtp\\|net\\|neverball\\|nvidia-settings-\\|offlineimap\\|qt/.+\\|realplayer\\|reportbug\\|rtorrent\\.\\|screen\\|scummvm\\|sversion\\|sylpheed/.+\\|xmp\\)rc\\'" . conf-guess-mode)
> +     ("/\\.\\(?:gdbtkinit\\|grip\\|mpdconf\\|notmuch-config\\|orbital/.+txt\\|rhosts\\|tuxracer/options\\)\\'" . conf-guess-mode)
>       ("/\\.?X\\(?:default\\|resource\\|re\\)s\\>" . conf-xdefaults-mode)
>       ("/X11.+app-defaults/\\|\\.ad\\'" . conf-xdefaults-mode)
>       ("/X11.+locale/.+/Compose\\'" . conf-colon-mode)
> @@ -2920,7 +2920,7 @@ conf-mode-maybe
>  	  (goto-char (point-min))
>  	  (looking-at "<\\?xml \\|<!-- \\|<!DOCTYPE ")))
>        (xml-mode)
> -    (conf-mode)))
> +    (conf-guess-mode)))
>  
>  (defvar interpreter-mode-alist
>    ;; Note: The entries for the modes defined in cc-mode.el (awk-mode

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

[Juri Linkov]

>> I tested this with additional patch that replaces `conf-mode` calls with
>> `conf-guess-mode`, and see that syntax font-lock now is fixed in diff-mode
>> on some conf files (not tested on all possible conf submodes).
>
> This said.  Using delay-mode-hooks in the diff-syntax driver is an ugly
> hack and is the real source of the problem, so splitting conf-mode into
> two is basically a way to work around the breakage introduced by the hack.

Indeed, but it seems it's too late to redesign this in the release
branch.  So unfontified conf-mode files in diff hunks is a trade-off
for not breaking the release.

As for changes in master, I tried something like this to use in
diff-syntax-fontify-props instead of direct set-auto-mode call:

(let ((buffer (make-indirect-buffer
               (current-buffer)
               (generate-new-buffer-name (buffer-name)))))
  (funcall
   (unwind-protect
       (with-current-buffer buffer
         (let ((buffer-file-name "/tmp/foo.el"))
           (set-auto-mode)
           major-mode))
     (kill-buffer buffer))))

It calls set-auto-mode in a temporary buffer.

Do you think this is the right direction?

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

[Juri Linkov]

> IOW, I think we should start with something like the patch below which
> makes `conf-guess-mode` available, after which we can start fixing the
> various callers to call `conf-guess-mode` instead.

Currently `conf-mode` is broken in master: visiting a conf file doesn't
activate `conf-mode`.  I don't know the right way to fix this, but this
patch demonstrates what basically needed to do to fix it, and it seems
something like this wrapper could be added:

(defun conf-guess-mode ()
  (funcall (conf--guess-mode)))

diff --git a/lisp/files.el b/lisp/files.el
index 8ce0187f5b..f0aa3447ac 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2813,7 +2813,7 @@ auto-mode-alist
      ("/config\\.\\(?:bat\\|log\\)\\'" . fundamental-mode)
      ("/\\.\\(authinfo\\|netrc\\)\\'" . authinfo-mode)
      ;; Windows candidates may be opened case sensitively on Unix
-     ("\\.\\(?:[iI][nN][iI]\\|[lL][sS][tT]\\|[rR][eE][gG]\\|[sS][yY][sS]\\)\\'" . conf-mode)
+     ("\\.\\(?:[iI][nN][iI]\\|[lL][sS][tT]\\|[rR][eE][gG]\\|[sS][yY][sS]\\)\\'" . (lambda () (funcall (conf--guess-mode))))
      ("\\.la\\'" . conf-unix-mode)
      ("\\.ppd\\'" . conf-ppd-mode)
      ("java.+\\.conf\\'" . conf-javaprop-mode)
@@ -2822,15 +2822,15 @@ auto-mode-alist
      ("\\.desktop\\'" . conf-desktop-mode)
      ("/\\.redshift.conf\\'" . conf-windows-mode)
      ("\\`/etc/\\(?:DIR_COLORS\\|ethers\\|.?fstab\\|.*hosts\\|lesskey\\|login\\.?de\\(?:fs\\|vperm\\)\\|magic\\|mtab\\|pam\\.d/.*\\|permissions\\(?:\\.d/.+\\)?\\|protocols\\|rpc\\|services\\)\\'" . conf-space-mode)
-     ("\\`/etc/\\(?:acpid?/.+\\|aliases\\(?:\\.d/.+\\)?\\|default/.+\\|group-?\\|hosts\\..+\\|inittab\\|ksysguarddrc\\|opera6rc\\|passwd-?\\|shadow-?\\|sysconfig/.+\\)\\'" . conf-mode)
+     ("\\`/etc/\\(?:acpid?/.+\\|aliases\\(?:\\.d/.+\\)?\\|default/.+\\|group-?\\|hosts\\..+\\|inittab\\|ksysguarddrc\\|opera6rc\\|passwd-?\\|shadow-?\\|sysconfig/.+\\)\\'" . (lambda () (funcall (conf--guess-mode))))
      ;; ChangeLog.old etc.  Other change-log-mode entries are above;
      ;; this has lower priority to avoid matching changelog.sgml etc.
      ("[cC]hange[lL]og[-.][-0-9a-z]+\\'" . change-log-mode)
      ;; either user's dot-files or under /etc or some such
-     ("/\\.?\\(?:gitconfig\\|gnokiirc\\|hgrc\\|kde.*rc\\|mime\\.types\\|wgetrc\\)\\'" . conf-mode)
+     ("/\\.?\\(?:gitconfig\\|gnokiirc\\|hgrc\\|kde.*rc\\|mime\\.types\\|wgetrc\\)\\'" . (lambda () (funcall (conf--guess-mode))))
      ;; alas not all ~/.*rc files are like this
-     ("/\\.\\(?:asound\\|enigma\\|fetchmail\\|gltron\\|gtk\\|hxplayer\\|mairix\\|mbsync\\|msmtp\\|net\\|neverball\\|nvidia-settings-\\|offlineimap\\|qt/.+\\|realplayer\\|reportbug\\|rtorrent\\.\\|screen\\|scummvm\\|sversion\\|sylpheed/.+\\|xmp\\)rc\\'" . conf-mode)
-     ("/\\.\\(?:gdbtkinit\\|grip\\|mpdconf\\|notmuch-config\\|orbital/.+txt\\|rhosts\\|tuxracer/options\\)\\'" . conf-mode)
+     ("/\\.\\(?:asound\\|enigma\\|fetchmail\\|gltron\\|gtk\\|hxplayer\\|mairix\\|mbsync\\|msmtp\\|net\\|neverball\\|nvidia-settings-\\|offlineimap\\|qt/.+\\|realplayer\\|reportbug\\|rtorrent\\.\\|screen\\|scummvm\\|sversion\\|sylpheed/.+\\|xmp\\)rc\\'" . (lambda () (funcall (conf--guess-mode))))
+     ("/\\.\\(?:gdbtkinit\\|grip\\|mpdconf\\|notmuch-config\\|orbital/.+txt\\|rhosts\\|tuxracer/options\\)\\'" . (lambda () (funcall (conf--guess-mode))))
      ("/\\.?X\\(?:default\\|resource\\|re\\)s\\>" . conf-xdefaults-mode)
      ("/X11.+app-defaults/\\|\\.ad\\'" . conf-xdefaults-mode)
      ("/X11.+locale/.+/Compose\\'" . conf-colon-mode)
@@ -2920,7 +2920,7 @@ conf-mode-maybe
 	  (goto-char (point-min))
 	  (looking-at "<\\?xml \\|<!-- \\|<!DOCTYPE ")))
       (xml-mode)
-    (conf-mode)))
+    (funcall (conf--guess-mode))))
 
 (defvar interpreter-mode-alist
   ;; Note: The entries for the modes defined in cc-mode.el (awk-mode

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

[Stefan Monnier]

> Currently `conf-mode` is broken in master: visiting a conf file doesn't
> activate `conf-mode`.

In my tests it does, but it only activates the parent-mode and not the
specific submode applicable to the file, indeed.

> I don't know the right way to fix this, but this patch demonstrates
> what basically needed to do to fix it, and it seems something like
> this wrapper could be added:
>
> (defun conf-guess-mode ()
>   (funcall (conf--guess-mode)))

That shouldn't be needed!
[ I.e. the right fix should arrange for that not to be necessary.
  I'm looking into it, thanks.  ]


        Stefan

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

[Juri Linkov]

>> (defun conf-guess-mode ()
>>   (funcall (conf--guess-mode)))
>
> That shouldn't be needed!
> [ I.e. the right fix should arrange for that not to be necessary.
>   I'm looking into it, thanks.  ]

BTW, I noticed another problem related to diff-syntax-fontify:
when diff-font-lock-syntax is customized to 'hunk-also',
and a patch displayed by Gnus contains a truncated
local variables list, e.g. only the first line

 ;; Local Variables:

is presented in diff context without the terminating
"End:" then opening such attachment patch raises the error:

   "Local variables list is not properly terminated"

Not sure if this is the right way to fix this, but it works:

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 8171a58515..96baea8526 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2724,7 +2724,7 @@ diff-syntax-fontify-props
           (buffer-file-name file))
       ;; Don't run hooks that might assume buffer-file-name
       ;; really associates buffer with a file (bug#39190).
-      (delay-mode-hooks (set-auto-mode))
+      (delay-mode-hooks (ignore-errors (set-auto-mode)))
       ;; FIXME: Is this really worth the trouble?
       (when (and (fboundp 'generic-mode-find-file-hook)
                  (memq #'generic-mode-find-file-hook

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

[Stefan Monnier]

> is presented in diff context without the terminating
> "End:" then opening such attachment patch raises the error:
>
>    "Local variables list is not properly terminated"

Really?  AFAICT the code does:

		  (unless (let ((case-fold-search t))
			    (re-search-forward
			     (concat prefix "[ \t]*End:[ \t]*" suffix)
			     nil t))
		    ;; This used to be an error, but really all it means is
		    ;; that this may simply not be a local-variables section,
		    ;; so just ignore it.
		    (message "Local variables list is not properly terminated"))

so it shouldn't signal an error but just emit a message.

> Not sure if this is the right way to fix this, but it works:

I think we should try and arrange for errors to really be "not normal",
and then use `with-demoted-errors`, yes.

But w.r.t file-local variables and diff-hunk, I think obeying "file-local
variables" doesn't make much sense when we only have a hunk to go on
since it's more common for the hunk not to include the actual file-local
vars, so if we find something it's likely to be a false-positive.


        Stefan


> diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
> index 8171a58515..96baea8526 100644
> --- a/lisp/vc/diff-mode.el
> +++ b/lisp/vc/diff-mode.el
> @@ -2724,7 +2724,7 @@ diff-syntax-fontify-props
>            (buffer-file-name file))
>        ;; Don't run hooks that might assume buffer-file-name
>        ;; really associates buffer with a file (bug#39190).
> -      (delay-mode-hooks (set-auto-mode))
> +      (delay-mode-hooks (ignore-errors (set-auto-mode)))
>        ;; FIXME: Is this really worth the trouble?
>        (when (and (fboundp 'generic-mode-find-file-hook)
>                   (memq #'generic-mode-find-file-hook

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

[Juri Linkov]

>> is presented in diff context without the terminating
>> "End:" then opening such attachment patch raises the error:
>>
>>    "Local variables list is not properly terminated"
>
> Really?  AFAICT the code does:
>
> 		  (unless (let ((case-fold-search t))
> 			    (re-search-forward
> 			     (concat prefix "[ \t]*End:[ \t]*" suffix)
> 			     nil t))
> 		    ;; This used to be an error, but really all it means is
> 		    ;; that this may simply not be a local-variables section,
> 		    ;; so just ignore it.
> 		    (message "Local variables list is not properly terminated"))
>
> so it shouldn't signal an error but just emit a message.

Sorry, actually this is what is displayed in the echo area after I added
ignore-errors.  But before adding ignore-errors, there was such backtrace:

Debugger entered--Lisp error: (error "Malformed local variable line: \"Local Variables:\"")
  signal(error ("Malformed local variable line: \"Local Variables:\""))
  error("Malformed local variable line: %S" "Local Variables:")
  hack-local-variables(t)
  set-auto-mode()
  (let ((delay-mode-hooks t)) (set-auto-mode))
  (progn (make-local-variable 'delay-mode-hooks) (let ((delay-mode-hooks t)) (set-auto-mode)))
  diff-syntax-fontify-props(#("a/lisp/dired-aux.el" ...
  diff-syntax-fontify-hunk(136 1334 t)
  diff-syntax-fontify(136 1334)
  diff--iterate-hunks(3750 #f(compiled-function (beg end) #<bytecode 0x877598e526a1ebd>))
  diff--font-lock-syntax(3750)
  font-lock-fontify-keywords-region(1 3750 nil)
  font-lock-default-fontify-region(1 3750 nil)
  font-lock-fontify-region(1 3750)
  font-lock-ensure()
  mm-display-inline-fontify(...
  mm-display-patch-inline(...
  mm-display-inline(...
  gnus-mime-display-single(...
  gnus-mime-display-part(...
  gnus-mime-display-mixed(...
  gnus-mime-display-part(...
  gnus-display-mime()
  gnus-article-prepare-display()
  gnus-article-prepare(165466 nil)
  gnus-summary-display-article(165466 nil)
  gnus-summary-select-article(nil nil pseudo)
  gnus-summary-scroll-up(1)
  funcall-interactively(gnus-summary-scroll-up 1)
  call-interactively(gnus-summary-scroll-up nil nil)
  command-execute(gnus-summary-scroll-up)

>> Not sure if this is the right way to fix this, but it works:
>
> I think we should try and arrange for errors to really be "not normal",
> and then use `with-demoted-errors`, yes.
>
> But w.r.t file-local variables and diff-hunk, I think obeying "file-local
> variables" doesn't make much sense when we only have a hunk to go on
> since it's more common for the hunk not to include the actual file-local
> vars, so if we find something it's likely to be a false-positive.

I'll try to set enable-local-variables to nil in case of HUNK-ONLY.