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

[Andrew Hyatt]

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.

text-scale-patch-v3
Description: Binary data