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

[Andrew Hyatt <ahyatt@gmail.com>]

[-- 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);