Subject: bug#34765: 26.1; with-temp-buffer should not run buffer-list-update-hook

From: martin rudalics <[email-masked]>
Date: Tue, 23 Apr 2019 11:21:45 +0200

>  >> Maybe we should indeed rebind 'buffer-list-update-hook' around the
 >  >> 'generate-new-buffer' and 'kill-buffer' calls of 'with-temp-buffer'.
 >  >
 >  > Wouldn't that be a backward-incompatible change?
 >
 > It would be a backward-incompatible change.
 >
 >  > How long did we call
 >  > that hook for temporary buffers?
 >
 > Ever since that hook existed.
 >
 >  > Also, can generate-new-buffer and/or
 >  > kill-buffer run some hooks which might modify other, non-temporary
 >  > buffers?
 >
 > 'generate-new-buffer' calls 'get-buffer-create' and that one runs only
 > 'buffer-list-update-hook'.  'kill-buffer' runs its usual hooks and if
 > one of them runs 'kill-buffer' for another buffer we'd have a problem.
 >
 > An even more radical solution would be to never run
 > 'buffer-list-update-hook' for buffers whose name starts with a space.
 > Backward-incompatible as well but cleaner from a designer's POV.
 >
 > In either case it wouldn't help the OP since he probably (hopefully)
 > won't need a solution for Emacs 27 (where he should be able to use
 > 'window-selection-change-functions' instead) and we are certainly not
 > going to change this for Emacs 26.

Stefan asked me to add a variable 'inhibit-buffer-list-update-hook'
and I came up with the attached.  WDYT?

martin
diff --git a/lisp/subr.el b/lisp/subr.el
index f68f9dd419..388705b3de 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3569,13 +3569,16 @@ with-temp-buffer
 See also `with-temp-file' and `with-output-to-string'."
   (declare (indent 0) (debug t))
   (let ((temp-buffer (make-symbol "temp-buffer")))
-    `(let ((,temp-buffer (generate-new-buffer " *temp*")))
+    `(let ((,temp-buffer
+	    (let ((inhibit-buffer-list-update-hook t))
+              (generate-new-buffer " *temp*"))))
        ;; FIXME: kill-buffer can change current-buffer in some odd cases.
        (with-current-buffer ,temp-buffer
          (unwind-protect
 	     (progn ,@body)
            (and (buffer-name ,temp-buffer)
-                (kill-buffer ,temp-buffer)))))))
+	        (let ((inhibit-buffer-list-update-hook t))
+                  (kill-buffer ,temp-buffer))))))))
 
 (defmacro with-silent-modifications (&rest body)
   "Execute BODY, pretending it does not modify the buffer.
diff --git a/src/buffer.c b/src/buffer.c
index ab47748191..822b5262dc 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -500,6 +500,36 @@ get_truename_buffer (register Lisp_Object filename)
   return Qnil;
 }
 
+
+/**
+ * run_buffer_list_update_hook:
+ *
+ * Run any functions on 'buffer-list-update-hook'.  Do not run the
+ * functions when BUFFER is a buffer and its inhibit_buffer_hooks
+ * structure element is set.  Do not run any functions either when we
+ * are not allowed to run hooks or 'inhibit-buffer-list-update-hook'
+ * is non-nil.
+ *
+ * While running the functions, bind 'inhibit-buffer-list-update-hook'
+ * to t to avoid invoking ourselves recursively.
+ */
+static void
+run_buffer_list_update_hook (Lisp_Object buffer)
+{
+  if (!NILP (Vrun_hooks)
+      && (!BUFFERP (buffer) || !XBUFFER (buffer)->inhibit_buffer_hooks)
+      && !inhibit_buffer_list_update_hook)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+
+      record_unwind_current_buffer ();
+      specbind (Qinhibit_buffer_list_update_hook, Qt);
+      call1 (Vrun_hooks, Qbuffer_list_update_hook);
+      unbind_to (count, Qnil);
+    }
+}
+
+
 DEFUN ("get-buffer-create", Fget_buffer_create, Sget_buffer_create, 1, 1, 0,
        doc: /* Return the buffer specified by BUFFER-OR-NAME, creating a new one if needed.
 If BUFFER-OR-NAME is a string and a live buffer with that name exists,
@@ -600,9 +630,8 @@ even if it is dead.  The return value is never nil.  */)
   /* Put this in the alist of all live buffers.  */
   XSETBUFFER (buffer, b);
   Vbuffer_alist = nconc2 (Vbuffer_alist, list1 (Fcons (name, buffer)));
-  /* And run buffer-list-update-hook.  */
-  if (!NILP (Vrun_hooks) && !b->inhibit_buffer_hooks)
-    call1 (Vrun_hooks, Qbuffer_list_update_hook);
+  /* Run buffer-list-update-hook.  */
+  run_buffer_list_update_hook (buffer);
 
   return buffer;
 }
@@ -871,8 +900,7 @@ CLONE nil means the indirect buffer's state is reset to default values.  */)
     }
 
   /* Run buffer-list-update-hook.  */
-  if (!NILP (Vrun_hooks))
-    call1 (Vrun_hooks, Qbuffer_list_update_hook);
+  run_buffer_list_update_hook (Qnil);
 
   return buf;
 }
@@ -1499,8 +1527,7 @@ This does not change the name of the visited file (if any).  */)
     call0 (intern ("rename-auto-save-file"));
 
   /* Run buffer-list-update-hook.  */
-  if (!NILP (Vrun_hooks) && !current_buffer->inhibit_buffer_hooks)
-    call1 (Vrun_hooks, Qbuffer_list_update_hook);
+  run_buffer_list_update_hook (buf);
 
   /* Refetch since that last call may have done GC.  */
   return BVAR (current_buffer, name);
@@ -1938,8 +1965,7 @@ cleaning up all windows currently displaying the buffer to be killed. */)
   bset_undo_list (b, Qnil);
 
   /* Run buffer-list-update-hook.  */
-  if (!NILP (Vrun_hooks) && !b->inhibit_buffer_hooks)
-    call1 (Vrun_hooks, Qbuffer_list_update_hook);
+  run_buffer_list_update_hook (buffer);
 
   return Qt;
 }
@@ -1980,8 +2006,7 @@ record_buffer (Lisp_Object buffer)
   fset_buried_buffer_list (f, Fdelq (buffer, f->buried_buffer_list));
 
   /* Run buffer-list-update-hook.  */
-  if (!NILP (Vrun_hooks) && !XBUFFER (buffer)->inhibit_buffer_hooks)
-    call1 (Vrun_hooks, Qbuffer_list_update_hook);
+  run_buffer_list_update_hook (buffer);
 }

@@ -2019,8 +2044,7 @@ DEFUN ("bury-buffer-internal", Fbury_buffer_internal, Sbury_buffer_internal,
     (f, Fcons (buffer, Fdelq (buffer, f->buried_buffer_list)));
 
   /* Run buffer-list-update-hook.  */
-  if (!NILP (Vrun_hooks) && !XBUFFER (buffer)->inhibit_buffer_hooks)
-    call1 (Vrun_hooks, Qbuffer_list_update_hook);
+  run_buffer_list_update_hook (buffer);
 
   return Qnil;
 }
@@ -6268,12 +6292,16 @@ The function `kill-all-local-variables' runs this before doing anything else.  *
 	       doc: /* Hook run when the buffer list changes.
 Functions (implicitly) running this hook are `get-buffer-create',
 `make-indirect-buffer', `rename-buffer', `kill-buffer', `bury-buffer'
-and `select-window'.  Functions run by this hook should avoid calling
-`select-window' with a nil NORECORD argument or `with-temp-buffer'
-since either may lead to infinite recursion.  */);
+and `select-window'.  This hook is not run when
+`inhibit-buffer-list-update-hook' is non-nil.  */);
   Vbuffer_list_update_hook = Qnil;
   DEFSYM (Qbuffer_list_update_hook, "buffer-list-update-hook");
 
+  DEFVAR_BOOL ("inhibit-buffer-list-update-hook", inhibit_buffer_list_update_hook,
+	       doc: /* Non-nil means don't run `buffer-list-update-hook'.  */);
+  inhibit_buffer_list_update_hook = false;
+  DEFSYM (Qinhibit_buffer_list_update_hook, "inhibit-buffer-list-update-hook");
+
   defsubr (&Sbuffer_live_p);
   defsubr (&Sbuffer_list);
   defsubr (&Sget_buffer);