bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Andrew Hyatt]

[-- Attachment #1: text-scale-patch --]
[-- Type: application/octet-stream, Size: 590 bytes --]

diff --git a/lisp/face-remap.el b/lisp/face-remap.el
index 00560f9d2e..df35d88933 100644
--- a/lisp/face-remap.el
+++ b/lisp/face-remap.el
@@ -134,6 +134,11 @@ face-remap-add-relative
     (let ((faces (cdr entry)))
       (if (symbolp faces)
 	  (setq faces (list faces)))
+      (setq face-remapping-alist
+	    (mapcar (lambda (kv)
+		      (if (eq (car kv) face)
+			  (cons face (face-remap-order (cons specs faces)))
+			kv)) face-remapping-alist))
       (setcdr entry (face-remap-order (cons specs faces)))
       ;; Force redisplay of this buffer.
       (force-mode-line-update))

[-- Attachment #2: Type: text/plain, Size: 2185 bytes --]


Hi all,

I noticed a bug recently where if I scale up fonts in an org 
capture buffer, it affects the original buffer, which keeps 
getting bigger and bigger every time org-capture is run.

However, you don't need org to reproduce this.  Here's a quick way 
to reproduce, which works with emacs -Q:

(require 'face-remap) (defun ash/big-font () 
  "Creates a font that is big enough for about 20 lines of text." 
  (interactive) (let ((text-scale-mode-amount (/ (frame-height) 
  20))) 
    (text-scale-mode 1))) 
 
(defun ash/reproduce-with-indirect-buffer () 
  (interactive) (let ((buf (get-buffer-create "*Orig buffer*"))) 
    (set-buffer buf) (variable-pitch-mode 1) ;; same way org mode 
    creates indirect buffer (set-buffer (make-indirect-buffer buf 
    "*Indirect buffer*" 'clone)) (ash/big-font-new) (kill-buffer 
    (current-buffer)))) 

Running ash/reproduce-with-indirect-buffer will increase the 
indirect buffer in size each time.  If you look at 
face-remapping-alist, it's clear that the original buffer's value 
is being altered by the indirect buffer.

I traced this down to the setcdr in face-remove-add-relative, 
which is incorrectly alterting a buffer local variable, by setting 
parts of a variable in a way that will affect the original 
variable.  Evidently making something buffer local doesn't clone 
it, it just copies the value, and so you can alter the original by 
operations such as setcdr. 

Assuming that the fixing how variables becomes buffer local is too 
much, I've fixed this in face-remap.el.  The patch is attached.

 
In GNU Emacs 29.0.50 (build 1, aarch64-apple-darwin21.2.0, NS 
appkit-2113.20 Version 12.1 (Build 21C52)) 
 of 2021-12-26 built on andrews-mbp.lan 
Repository revision: 978987f7ad58cd66fe51cefde53ba4771b189aeb 
Repository branch: master Windowing system distributor 'Apple', 
version 10.3.2113 System Description:  macOS 12.1
 
Configured using:
'configure --with-native-
Configured features:
ACL GLIB GNUTLS JSON LCMS2 LIBXML2 MODULES NATIVE_COMP NOTIFY 
KQUEUE NS
PDUMPER PNG RSVG SQLITE3 THREADS TOOLKIT_SCROLL_BARS XIM ZLIB
Important settings:
  value of $LANG: en_US.UTF-8
   
 locale-coding-system: utf-8-unix

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Eli Zaretskii]

> From: Andrew Hyatt <ahyatt@gmail.com>
> Date: Sun, 16 Jan 2022 00:13:26 -0500
> 
> I noticed a bug recently where if I scale up fonts in an org 
> capture buffer, it affects the original buffer, which keeps 
> getting bigger and bigger every time org-capture is run.
> 
> However, you don't need org to reproduce this.  Here's a quick way 
> to reproduce, which works with emacs -Q:
> 
> (require 'face-remap) (defun ash/big-font () 
>   "Creates a font that is big enough for about 20 lines of text." 
>   (interactive) (let ((text-scale-mode-amount (/ (frame-height) 
>   20))) 
>     (text-scale-mode 1))) 
>  
> (defun ash/reproduce-with-indirect-buffer () 
>   (interactive) (let ((buf (get-buffer-create "*Orig buffer*"))) 
>     (set-buffer buf) (variable-pitch-mode 1) ;; same way org mode 
>     creates indirect buffer (set-buffer (make-indirect-buffer buf 
>     "*Indirect buffer*" 'clone)) (ash/big-font-new) (kill-buffer 
>     (current-buffer)))) 
> 
> Running ash/reproduce-with-indirect-buffer will increase the 
> indirect buffer in size each time.  If you look at 
> face-remapping-alist, it's clear that the original buffer's value 
> is being altered by the indirect buffer.

I don't think I understand what I should see and pay attention to with
this recipe (and it includes several errors that took me some time to
fix, before I could run it), but isn't this because you used non-nil
CLONE argument to make-indirect-buffer?

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Andrew Hyatt]

On Sun, Jan 16, 2022 at 11:22 AM Eli Zaretskii <eliz@gnu.org> 
wrote: 

>> From: Andrew Hyatt <ahyatt@gmail.com> Date: Sun, 16 Jan 2022 
>> 00:13:26 -0500  I noticed a bug recently where if I scale up 
>> fonts in an org  capture buffer, it affects the original 
>> buffer, which keeps  getting bigger and bigger every time 
>> org-capture is run.   However, you don't need org to reproduce 
>> this.  Here's a quick way  to reproduce, which works with emacs 
>> -Q:  (require 'face-remap) (defun ash/big-font ()  
>>   "Creates a font that is big enough for about 20 lines of 
>>   text."  (interactive) (let ((text-scale-mode-amount (/ 
>>   (frame-height)  20)))  
>>     (text-scale-mode 1)))  
>>   
>> (defun ash/reproduce-with-indirect-buffer ()  
>>   (interactive) (let ((buf (get-buffer-create "*Orig 
>>   buffer*")))  
>>     (set-buffer buf) (variable-pitch-mode 1) ;; same way org 
>>     mode  creates indirect buffer (set-buffer 
>>     (make-indirect-buffer buf  "*Indirect buffer*" 'clone)) 
>>     (ash/big-font-new) (kill-buffer  (current-buffer))))  
>>  Running ash/reproduce-with-indirect-buffer will increase the 
>> indirect buffer in size each time.  If you look at 
>> face-remapping-alist, it's clear that the original buffer's 
>> value  is being altered by the indirect buffer. 
> 
> I don't think I understand what I should see and pay attention 
> to with this recipe (and it includes several errors that took me 
> some time to fix, before I could run it), but isn't this because 
> you used non-nil CLONE argument to make-indirect-buffer? 


Sorry for any errors, I simplified it a bit before I sent it out 
and perhaps made a few mistakes. What you should see, if you go 
the *Orig Buffer*, is that the font scale keeps increasing. If you 
look at the face-remapping-alist of that buffer, it will be 
growing with duplicate height specs, one for every time you called 
the ash/reproduce-with-indirect-buffer.

Yes, the non-nil CLONE argument is essential to reproduce. But 
that just means the indirect buffer will inherit the state from 
the direct buffer.  It doesn't imply that changes made to the 
indirect buffer's state will affect the original buffer, which is 
what is happening here.

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Eli Zaretskii]

> From: Andrew Hyatt <ahyatt@gmail.com>
> Cc: 53294@debbugs.gnu.org
> Date: Sun, 16 Jan 2022 09:43:16 -0500
> 
> Yes, the non-nil CLONE argument is essential to reproduce. But 
> that just means the indirect buffer will inherit the state from 
> the direct buffer.  It doesn't imply that changes made to the 
> indirect buffer's state will affect the original buffer, which is 
> what is happening here.

But "inheriting" the state means the indirect buffer gets the copy of
the variables of the original buffer, and that is not a deep copy,
AFAIU.

Stefan, any comments on this issue?

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Andrew Hyatt]

On Sun, Jan 16, 2022 at 05:00 PM Eli Zaretskii <eliz@gnu.org> 
wrote: 

>> From: Andrew Hyatt <ahyatt@gmail.com> Cc: 53294@debbugs.gnu.org 
>> Date: Sun, 16 Jan 2022 09:43:16 -0500  Yes, the non-nil CLONE 
>> argument is essential to reproduce. But  that just means the 
>> indirect buffer will inherit the state from  the direct buffer. 
>> It doesn't imply that changes made to the  indirect buffer's 
>> state will affect the original buffer, which is  what is 
>> happening here. 
> 
> But "inheriting" the state means the indirect buffer gets the 
> copy of the variables of the original buffer, and that is not a 
> deep copy, AFAIU. 
> 
> Stefan, any comments on this issue? 

Sorry, I may have been unclear. I'm not disagreeing - what you 
just said is correct. But, because of that, it's a bug for code to 
make an indirect buffer, then perform operations on it via setcdr 
or setf like things, which then will affect the original buffer's 
variables.

Alternatively, it'd perhaps would be a better fix to make the 
indirect buffer's copy a deep copy instead of the shallow copy it 
is now, so that these situations don't arise. I don't have a patch 
for that, though, but could create one if you thought it was a 
good idea.

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Eli Zaretskii]

> From: Andrew Hyatt <ahyatt@gmail.com>
> Cc: 53294@debbugs.gnu.org
> Date: Sun, 16 Jan 2022 10:28:40 -0500
> 
> > But "inheriting" the state means the indirect buffer gets the 
> > copy of the variables of the original buffer, and that is not a 
> > deep copy, AFAIU. 
> > 
> > Stefan, any comments on this issue? 
> 
> Sorry, I may have been unclear. I'm not disagreeing - what you 
> just said is correct. But, because of that, it's a bug for code to 
> make an indirect buffer, then perform operations on it via setcdr 
> or setf like things, which then will affect the original buffer's 
> variables.

I'm not sure it's a bug.  It could be a "feature", not limited to
face-remapping-alist, but instead affecting every buffer-local
variable that is not a simple scalar.

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > But "inheriting" the state means the indirect buffer gets the 
>> > copy of the variables of the original buffer, and that is not a 
>> > deep copy, AFAIU. 
>> > Stefan, any comments on this issue? 
>> Sorry, I may have been unclear. I'm not disagreeing - what you 
>> just said is correct. But, because of that, it's a bug for code to 
>> make an indirect buffer, then perform operations on it via setcdr 
>> or setf like things, which then will affect the original buffer's 
>> variables.
> I'm not sure it's a bug.  It could be a "feature", not limited to
> face-remapping-alist, but instead affecting every buffer-local
> variable that is not a simple scalar.

`make-indirect-buffer` can't do the right thing, indeed, because
different variables need different handling.  `clone-buffer` tries to
handle these issues by relying on `clone-buffer-hook` to handle the
various variables on a case-by-case basis, but `make-indirect-buffer`
doesn't come with such a hook.

Some ways we can fix this:

- In `face-remap.el`, refrain from modifying the `face-remapping-alist`
  by side-effects (i.e. avoid `delq`, `setcdr`, and friends).

- Add a `make-indirect-buffer-hook` and arrange for `face-remap.el` to
  add a function there that does a deep enough copy of
  `face-remapping-alist`.

- Remember that indirect buffers are an attractive nuisance and should
  be deprecated (but note that I suspect the same bug affects
  `clone-buffer` because it doesn't make a deep enough copy of
  `face-remapping-alist` either).


        Stefan

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Andrew Hyatt <ahyatt@gmail.com>,  53294@debbugs.gnu.org
> Date: Sun, 16 Jan 2022 12:11:39 -0500
> 
> Some ways we can fix this:
> 
> - In `face-remap.el`, refrain from modifying the `face-remapping-alist`
>   by side-effects (i.e. avoid `delq`, `setcdr`, and friends).
> 
> - Add a `make-indirect-buffer-hook` and arrange for `face-remap.el` to
>   add a function there that does a deep enough copy of
>   `face-remapping-alist`.
> 
> - Remember that indirect buffers are an attractive nuisance and should
>   be deprecated (but note that I suspect the same bug affects
>   `clone-buffer` because it doesn't make a deep enough copy of
>   `face-remapping-alist` either).

The last one tells me we are better with leaving this sleeping dog
lie.

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Andrew Hyatt]

On Sun, Jan 16, 2022 at 08:16 PM Eli Zaretskii <eliz@gnu.org> 
wrote: 

>> From: Stefan Monnier <monnier@iro.umontreal.ca> Cc: Andrew 
>> Hyatt <ahyatt@gmail.com>,  53294@debbugs.gnu.org Date: Sun, 16 
>> Jan 2022 12:11:39 -0500  Some ways we can fix this:  - In 
>> `face-remap.el`, refrain from modifying the 
>> `face-remapping-alist` 
>>   by side-effects (i.e. avoid `delq`, `setcdr`, and friends). 
>>  - Add a `make-indirect-buffer-hook` and arrange for 
>> `face-remap.el` to 
>>   add a function there that does a deep enough copy of 
>>   `face-remapping-alist`. 
>>  - Remember that indirect buffers are an attractive nuisance 
>> and should 
>>   be deprecated (but note that I suspect the same bug affects 
>>   `clone-buffer` because it doesn't make a deep enough copy of 
>>   `face-remapping-alist` either). 
> 
> The last one tells me we are better with leaving this sleeping 
> dog lie. 

I agree the use of indirect buffers is problematic, but this 
problem actually results in user-visible bugs. Anyone who likes 
their org-mode buffer to be variable-pitch, and likes their 
capture buffer to be in a larger font (both pretty reasonable 
things), will run into this problem.

My patch fixes this in a way whose only downside is that it would 
be less efficient notably when you have a lot of face-remappings. 
But it's not clear to me that face-remapping-alist ever gets so 
big or is changed so often that this would be a problem.

The only other option is to fix this in org-mode, but they are 
cloning their indirect buffer presumably so that the capture 
buffer looks and behaves like the parent buffer, which is 
reasonable. I'd have to break that, or maybe just add a hack to 
deep copy face-remapping-alist.  Both options seem a bit wrong.

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> - In `face-remap.el`, refrain from modifying the `face-remapping-alist`
>>   by side-effects (i.e. avoid `delq`, `setcdr`, and friends).
>> 
>> - Add a `make-indirect-buffer-hook` and arrange for `face-remap.el` to
>>   add a function there that does a deep enough copy of
>>   `face-remapping-alist`.
>> 
>> - Remember that indirect buffers are an attractive nuisance and should
>>   be deprecated (but note that I suspect the same bug affects
>>   `clone-buffer` because it doesn't make a deep enough copy of
>>   `face-remapping-alist` either).
>
> The last one tells me we are better with leaving this sleeping dog
> lie.

Actually, looking back at the code, I see we already have the needed
hook (tho called from `clone-indirect-buffer`), so I think the better
option is to change `make-indirect-buffer` so it runs
`clone-indirect-buffer-hook` (when called with a non-nil `clone` arg)
instead of having `clone-indirect-buffer` run it.  And then in
`face-remap.el` add a function to that hook which copies
`face-remapping-alist`.

An intermediate option is to change the Org code to use
`clone-indirect-buffer` instead of `make-indirect-buffer` (but that
still requires the `face-remap.el` addition to the hook).

[ Sorry Andrew for not noticing your earlier patch which does something
  similar to my first suggestion above.  ]

Andrew, would you be willing to write that patch?


        Stefan

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Andrew Hyatt]

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

On Mon, Jan 17, 2022 at 04:47 PM Stefan Monnier 
<monnier@iro.umontreal.ca> wrote: 

>>> - In `face-remap.el`, refrain from modifying the 
>>> `face-remapping-alist` 
>>>   by side-effects (i.e. avoid `delq`, `setcdr`, and friends). 
>>>  - Add a `make-indirect-buffer-hook` and arrange for 
>>> `face-remap.el` to 
>>>   add a function there that does a deep enough copy of 
>>>   `face-remapping-alist`. 
>>>  - Remember that indirect buffers are an attractive nuisance 
>>> and should 
>>>   be deprecated (but note that I suspect the same bug affects 
>>>   `clone-buffer` because it doesn't make a deep enough copy of 
>>>   `face-remapping-alist` either). 
>> 
>> The last one tells me we are better with leaving this sleeping 
>> dog lie. 
> 
> Actually, looking back at the code, I see we already have the 
> needed hook (tho called from `clone-indirect-buffer`), so I 
> think the better option is to change `make-indirect-buffer` so 
> it runs `clone-indirect-buffer-hook` (when called with a non-nil 
> `clone` arg) instead of having `clone-indirect-buffer` run it. 
> And then in `face-remap.el` add a function to that hook which 
> copies `face-remapping-alist`. 
> 
> An intermediate option is to change the Org code to use 
> `clone-indirect-buffer` instead of `make-indirect-buffer` (but 
> that still requires the `face-remap.el` addition to the hook). 
> 
> [ Sorry Andrew for not noticing your earlier patch which does 
> something 
>   similar to my first suggestion above.  ] 
> 
> Andrew, would you be willing to write that patch?
>  
>         Stefan 

That sounds reasonable, I can do that. In fact, I just did this 
based on your suggestions, and it does work well. The only weird 
thing is that I had to pull `clone-indirect-buffer-hook' into the 
c code, because that's where `make-indirect-buffer' is. Let me 
know if that seems wrong.

I've attached the patch, please let me know if there is an issue. 
I have commit access, so I can just commit it myself after your OK 
(if so, I'll wait for a week or so to see if Eli has a comment as 
well before checkin).


[-- Attachment #2: text-scale-patch-v2 --]
[-- Type: application/octet-stream, Size: 2895 bytes --]

diff --git a/lisp/face-remap.el b/lisp/face-remap.el
index 00560f9d2e..fc0edb7119 100644
--- a/lisp/face-remap.el
+++ b/lisp/face-remap.el
@@ -70,6 +70,14 @@ internal-lisp-face-attributes
    :foreground :background :stipple :overline :strike-through :box
    :font :inherit :fontset :distant-foreground :extend :vector])
 
+(defun face-attrs--make-indirect-safe ()
+  "Do a deep copy of `face-remapping-alist' to keep the original buffer safe."
+  (setq-local face-remapping-alist
+              (copy-tree
+               (buffer-local-value 'face-remapping-alist (buffer-base-buffer)))))
+
+(add-hook 'clone-indirect-buffer-hook #'face-attrs--make-indirect-safe)
+
 (defun face-attrs-more-relative-p (attrs1 attrs2)
   "Return true if ATTRS1 contains a greater number of relative
 face-attributes than ATTRS2.  A face attribute is considered
diff --git a/lisp/simple.el b/lisp/simple.el
index cbcde9fb8d..3ef9cca249 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9441,9 +9441,6 @@ function-key-map
 (defvar clone-buffer-hook nil
   "Normal hook to run in the new buffer at the end of `clone-buffer'.")
 
-(defvar clone-indirect-buffer-hook nil
-  "Normal hook to run in the new buffer at the end of `clone-indirect-buffer'.")
-
 (defun clone-process (process &optional newname)
   "Create a twin copy of PROCESS.
 If NEWNAME is nil, it defaults to PROCESS' name;
diff --git a/src/buffer.c b/src/buffer.c
index 10ac91915c..00646f8779 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -912,6 +912,10 @@ DEFUN ("make-indirect-buffer", Fmake_indirect_buffer, Smake_indirect_buffer,
       Fset (intern ("buffer-save-without-query"), Qnil);
       Fset (intern ("buffer-file-number"), Qnil);
       Fset (intern ("buffer-stale-function"), Qnil);
+      /* Cloned buffers need extra setup, to do things such as deep
+	 variable copies for list variables that might be mangled due
+	 to destructive operations in the indirect buffer. */
+      safe_run_hooks (Qclone_indirect_buffer_hook);
       set_buffer_internal_1 (old_b);
     }
 
@@ -5568,6 +5572,9 @@ syms_of_buffer (void)
 	pure_list (Qprotected_field, Qerror));
   Fput (Qprotected_field, Qerror_message,
 	build_pure_c_string ("Attempt to modify a protected field"));
+  
+  DEFSYM (Qclone_indirect_buffer_hook, "clone-indirect-buffer-hook");
+ 
 
   DEFVAR_PER_BUFFER ("tab-line-format",
 		     &BVAR (current_buffer, tab_line_format),
@@ -6392,6 +6399,10 @@ Functions (implicitly) running this hook are `get-buffer-create',
 This is the default.  If nil, auto-save file deletion is inhibited.  */);
   delete_auto_save_files = 1;
 
+  DEFVAR_LISP ("clone-indirect-buffer-hook", Vclone_indirect_buffer_hook,
+	       doc: /* Normal hook to run in the new buffer at the end of `clone-indirect-buffer'. */);
+  Vclone_indirect_buffer_hook = Qnil;
+
   defsubr (&Sbuffer_live_p);
   defsubr (&Sbuffer_list);
   defsubr (&Sget_buffer);

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> That sounds reasonable, I can do that. In fact, I just did this based on
> your suggestions, and it does work well.  The only weird thing is that I had
> to pull `clone-indirect-buffer-hook' into the c code, because that's where
> `make-indirect-buffer' is.

You could leave the `defvar` in Lisp and only pull the DEFSYM, but
yes, you need to pull at least part of it into C.

> Let me know if that seems wrong.

Nope.

> I've attached the patch, please let me know if there is an issue. I have
> commit access, so I can just commit it myself after your OK (if so, I'll
> wait for a week or so to see if Eli has a comment as well before checkin).

Comments below, thanks,


        Stefan


> diff --git a/lisp/face-remap.el b/lisp/face-remap.el
> index 00560f9d2e..fc0edb7119 100644
> --- a/lisp/face-remap.el
> +++ b/lisp/face-remap.el
> @@ -70,6 +70,14 @@ internal-lisp-face-attributes
>     :foreground :background :stipple :overline :strike-through :box
>     :font :inherit :fontset :distant-foreground :extend :vector])
>  
> +(defun face-attrs--make-indirect-safe ()
> +  "Do a deep copy of `face-remapping-alist' to keep the original buffer safe."
> +  (setq-local face-remapping-alist
> +              (copy-tree
> +               (buffer-local-value 'face-remapping-alist (buffer-base-buffer)))))

I think `mapcar #'copy-sequence` is slightly more correct than
`copy-tree`, and you shouldn't need
(buffer-local-value 'face-remapping-alist (buffer-base-buffer)) because
that buffer's value should already have been copied to the current buffer.

> @@ -912,6 +912,10 @@ DEFUN ("make-indirect-buffer", Fmake_indirect_buffer, Smake_indirect_buffer,
>        Fset (intern ("buffer-save-without-query"), Qnil);
>        Fset (intern ("buffer-file-number"), Qnil);
>        Fset (intern ("buffer-stale-function"), Qnil);
> +      /* Cloned buffers need extra setup, to do things such as deep
> +	 variable copies for list variables that might be mangled due
> +	 to destructive operations in the indirect buffer. */
> +      safe_run_hooks (Qclone_indirect_buffer_hook);

I think you want to use just `run_hook` here because there's no need to
catch errors.

More importantly, you need to remove the corresponding `run-hook` from
`clone-indirect-buffer`.


        Stefan

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Andrew Hyatt]

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

On Mon, Jan 17, 2022 at 09:47 PM Stefan Monnier 
<monnier@iro.umontreal.ca> wrote: 

>> That sounds reasonable, I can do that. In fact, I just did this 
>> based on your suggestions, and it does work well.  The only 
>> weird thing is that I had to pull `clone-indirect-buffer-hook' 
>> into the c code, because that's where `make-indirect-buffer' 
>> is. 
> 
> You could leave the `defvar` in Lisp and only pull the DEFSYM, 
> but yes, you need to pull at least part of it into C. 
> 
>> Let me know if that seems wrong. 
> 
> Nope. 
> 
>> I've attached the patch, please let me know if there is an 
>> issue. I have commit access, so I can just commit it myself 
>> after your OK (if so, I'll wait for a week or so to see if Eli 
>> has a comment as well before checkin). 
> 
> Comments below, thanks, 
>  
>         Stefan 
>  
>> diff --git a/lisp/face-remap.el b/lisp/face-remap.el index 
>> 00560f9d2e..fc0edb7119 100644 --- a/lisp/face-remap.el +++ 
>> b/lisp/face-remap.el @@ -70,6 +70,14 @@ 
>> internal-lisp-face-attributes 
>>     :foreground :background :stipple :overline :strike-through 
>>     :box :font :inherit :fontset :distant-foreground :extend 
>>     :vector]) 
>>   
>> +(defun face-attrs--make-indirect-safe () +  "Do a deep copy of 
>> `face-remapping-alist' to keep the original buffer safe."  + 
>> (setq-local face-remapping-alist +              (copy-tree + 
>> (buffer-local-value 'face-remapping-alist 
>> (buffer-base-buffer))))) 
> 
> I think `mapcar #'copy-sequence` is slightly more correct than 
> `copy-tree`, and you shouldn't need (buffer-local-value 
> 'face-remapping-alist (buffer-base-buffer)) because that 
> buffer's value should already have been copied to the current 
> buffer. 
> 
>> @@ -912,6 +912,10 @@ DEFUN ("make-indirect-buffer", 
>> Fmake_indirect_buffer, Smake_indirect_buffer, 
>>        Fset (intern ("buffer-save-without-query"), Qnil); Fset 
>>        (intern ("buffer-file-number"), Qnil); Fset (intern 
>>        ("buffer-stale-function"), Qnil); 
>> +      /* Cloned buffers need extra setup, to do things such as 
>> deep +	 variable copies for list variables that might be 
>> mangled due +	 to destructive operations in the indirect 
>> buffer. */ +      safe_run_hooks (Qclone_indirect_buffer_hook); 
> 
> I think you want to use just `run_hook` here because there's no 
> need to catch errors. 
> 
> More importantly, you need to remove the corresponding 
> `run-hook` from `clone-indirect-buffer`. 
>  
>         Stefan

Makes sense, thanks for the suggestions.  I've included the 
revised patch.

[-- Attachment #2: text-scale-patch-v3 --]
[-- Type: application/octet-stream, Size: 3178 bytes --]

diff --git a/lisp/face-remap.el b/lisp/face-remap.el
index 00560f9d2e..95dffcadd6 100644
--- a/lisp/face-remap.el
+++ b/lisp/face-remap.el
@@ -70,6 +70,13 @@ internal-lisp-face-attributes
    :foreground :background :stipple :overline :strike-through :box
    :font :inherit :fontset :distant-foreground :extend :vector])
 
+(defun face-attrs--make-indirect-safe ()
+  "Deep copy `face-remapping-alist' on cloning for safety."
+  (setq-local face-remapping-alist
+              (mapcar #'copy-sequence face-remapping-alist)))
+
+(add-hook 'clone-indirect-buffer-hook #'face-attrs--make-indirect-safe)
+
 (defun face-attrs-more-relative-p (attrs1 attrs2)
   "Return true if ATTRS1 contains a greater number of relative
 face-attributes than ATTRS2.  A face attribute is considered
diff --git a/lisp/simple.el b/lisp/simple.el
index cbcde9fb8d..8d0520d663 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9441,9 +9441,6 @@ function-key-map
 (defvar clone-buffer-hook nil
   "Normal hook to run in the new buffer at the end of `clone-buffer'.")
 
-(defvar clone-indirect-buffer-hook nil
-  "Normal hook to run in the new buffer at the end of `clone-indirect-buffer'.")
-
 (defun clone-process (process &optional newname)
   "Create a twin copy of PROCESS.
 If NEWNAME is nil, it defaults to PROCESS' name;
@@ -9596,8 +9593,6 @@ clone-indirect-buffer
       (setq newname (substring newname 0 (match-beginning 0))))
   (let* ((name (generate-new-buffer-name newname))
 	 (buffer (make-indirect-buffer (current-buffer) name t)))
-    (with-current-buffer buffer
-      (run-hooks 'clone-indirect-buffer-hook))
     (when display-flag
       (pop-to-buffer buffer nil norecord))
     buffer))
diff --git a/src/buffer.c b/src/buffer.c
index 10ac91915c..3b17d6c9ce 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -912,6 +912,10 @@ DEFUN ("make-indirect-buffer", Fmake_indirect_buffer, Smake_indirect_buffer,
       Fset (intern ("buffer-save-without-query"), Qnil);
       Fset (intern ("buffer-file-number"), Qnil);
       Fset (intern ("buffer-stale-function"), Qnil);
+      /* Cloned buffers need extra setup, to do things such as deep
+	 variable copies for list variables that might be mangled due
+	 to destructive operations in the indirect buffer. */
+      run_hook (Qclone_indirect_buffer_hook);
       set_buffer_internal_1 (old_b);
     }
 
@@ -5569,6 +5573,9 @@ syms_of_buffer (void)
   Fput (Qprotected_field, Qerror_message,
 	build_pure_c_string ("Attempt to modify a protected field"));
 
+  DEFSYM (Qclone_indirect_buffer_hook, "clone-indirect-buffer-hook");
+
+
   DEFVAR_PER_BUFFER ("tab-line-format",
 		     &BVAR (current_buffer, tab_line_format),
 		     Qnil,
@@ -6392,6 +6399,10 @@ Functions (implicitly) running this hook are `get-buffer-create',
 This is the default.  If nil, auto-save file deletion is inhibited.  */);
   delete_auto_save_files = 1;
 
+  DEFVAR_LISP ("clone-indirect-buffer-hook", Vclone_indirect_buffer_hook,
+	       doc: /* Normal hook to run in the new buffer at the end of `clone-indirect-buffer'. */);
+  Vclone_indirect_buffer_hook = Qnil;
+
   defsubr (&Sbuffer_live_p);
   defsubr (&Sbuffer_list);
   defsubr (&Sget_buffer);

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Makes sense, thanks for the suggestions.  I've included the revised patch.

Looks good to me, thanks,


        Stefan

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Lars Ingebrigtsen]

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

>> Makes sense, thanks for the suggestions.  I've included the revised patch.
>
> Looks good to me, thanks,

So I've pushed the patch to Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Anders Johansson]

Hi,

Stefan wrote:
> I think `mapcar #'copy-sequence` is slightly more correct than
> `copy-tree`,

This gives me  "Wrong type argument: listp, mini-modeline-mode-line-active"
For a face-remapping-alist that looks like:
((mode-line-active . mini-modeline-mode-line-active)
 (mode-line-inactive . mini-modeline-mode-line-inactive))

According to my reading of the docstring of face-remapping-alist, this
should be allowed, and does work (except for indirect buffers perhaps)

face-remap-add-relative would generate:
((mode-line-active mini-modeline-mode-line-active)
 (mode-line-inactive mini-modeline-mode-line-inactive))

mini-modeline (https://github.com/kiennq/emacs-mini-modeline/) sets
face-remapping-alist directly for various reasons. I guess this is not
really recommended. But this fix seems to cause problems in an
otherwise "correct" use of face-remapping-alist.

Best,
Anders Johansson

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Lars Ingebrigtsen]

Anders Johansson <mejlaandersj@gmail.com> writes:

> This gives me  "Wrong type argument: listp, mini-modeline-mode-line-active"
> For a face-remapping-alist that looks like:
> ((mode-line-active . mini-modeline-mode-line-active)
>  (mode-line-inactive . mini-modeline-mode-line-inactive))

Do you have a recipe to reproduce the problem, starting from "emacs -Q"?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53294: 29.0.50; Indirect font changes incorrectly affecting original buffer

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I think `mapcar #'copy-sequence` is slightly more correct than
>> `copy-tree`,
>
> This gives me  "Wrong type argument: listp, mini-modeline-mode-line-active"
> For a face-remapping-alist that looks like:
> ((mode-line-active . mini-modeline-mode-line-active)
>  (mode-line-inactive . mini-modeline-mode-line-inactive))

I pushed a patch to `master` which should fix it.  Can you confirm that
it fixes it for you?


        Stefan