bug#69056: 30.0.50; history-add-new-input and recursive minibuffers

[Eshel Yaron via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

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

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

>> Thanks, that's what I thought too.  Here's an attempt do just that:
>
> Looks pretty good.  I do have some comments/questions:
>
>> @@ -902,6 +903,9 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
>>    /* Don't allow the user to undo past this point.  */
>>    bset_undo_list (current_buffer, Qnil);
>>
>> +  /* Cache the buffer-local value. */
>> +  nohist = NILP (find_symbol_value (Qhistory_add_new_input));
>
> Why not use `Vhistory_add_new_input`?

Good question, I guess for some reason I assumed that `NILP (Vfoo)`
doesn't check the buffer-local value like `find_symbol_value (Qfoo)`
does...

> [ Also, it's not really "cache" (which implies it impacts only
>   performance).  More like "remember".  ]

>> @@ -965,7 +969,7 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
>>    /* Add the value to the appropriate history list, if any.  This is
>>       done after the previous buffer has been made current again, in
>>       case the history variable is buffer-local.  */
>> -  if (! (NILP (Vhistory_add_new_input) || NILP (histstring)))
>> +  if (! (nohist || NILP (histstring)))
>>      call2 (Qadd_to_history, histvar, histstring);
>>
>>    /* If Lisp form desired instead of string, parse it.  */
>
> IIUC this change is needed because by the time we get here the
> buffer-local value of `history-add-new-input` has been flushed by
> `minibuffer-inactive-mode` called by `read_minibuf_unwind`,
> itself run by the `unbind_to` a few lines above.
> So maybe we can simplify this by just moving the above 2 lines before
> the `unbind_to`, WDYT?

Oh, that's much simpler indeed.  And it seems to work just as well.
Here's an updated patch (v2):


[-- Attachment #2: v2-0001-Use-buffer-local-value-of-history-add-new-input-i.patch --]
[-- Type: text/x-patch, Size: 8987 bytes --]

From 66fc69ff23911aa6e91e290a71568cf14d076d58 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Sun, 11 Feb 2024 16:18:48 +0100
Subject: [PATCH v2] Use buffer local value of 'history-add-new-input' in
 minibuffer

Avoid let-binding 'history-add-new-input', since that affects all nested
recursive minibuffers, and instead use a buffer-local setting in the
appropriate minibuffer.

* src/minibuf.c (read_minibuf): Use 'history-add-new-input' local value.

* lisp/isearch.el (isearch-edit-string)
* lisp/replace.el (query-replace-read-from)
(query-replace-read-to, read-regexp)
* lisp/simple.el (read-from-kill-ring): Set 'history-add-new-input'
locally in the minibuffer, instead of let-binding it.

* doc/lispref/minibuf.texi (Minibuffer History): Update.  (Bug#69056)
---
 doc/lispref/minibuf.texi |  6 +++---
 lisp/isearch.el          | 12 +++++++-----
 lisp/replace.el          | 36 +++++++++++++++++++-----------------
 lisp/simple.el           |  4 ++--
 src/minibuf.c            | 12 ++++++------
 5 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/doc/lispref/minibuf.texi b/doc/lispref/minibuf.texi
index aa27de72ba0..2e8b21d7040 100644
--- a/doc/lispref/minibuf.texi
+++ b/doc/lispref/minibuf.texi
@@ -700,9 +700,9 @@ Minibuffer History
 @end defun
 
 @defvar history-add-new-input
-If the value of this variable is @code{nil}, standard functions that
-read from the minibuffer don't add new elements to the history list.
-This lets Lisp programs explicitly manage input history by using
+If the value of this variable is @code{nil} in a minibuffer, Emacs
+doesn't add new elements to the history list of that minibuffer.  This
+lets Lisp programs explicitly manage input history by using
 @code{add-to-history}.  The default value is @code{t}.
 @end defvar
 
diff --git a/lisp/isearch.el b/lisp/isearch.el
index a139a6fb84e..814ab919d5e 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1844,10 +1844,6 @@ isearch-edit-string
   (interactive)
   (with-isearch-suspended
    (let* ((message-log-max nil)
-	  ;; Don't add a new search string to the search ring here
-	  ;; in `read-from-minibuffer'. It should be added only
-	  ;; by `isearch-update-ring' called from `isearch-done'.
-	  (history-add-new-input nil)
 	  ;; Binding minibuffer-history-symbol to nil is a work-around
 	  ;; for some incompatibility with gmhist.
 	  (minibuffer-history-symbol)
@@ -1855,7 +1851,13 @@ isearch-edit-string
 	  (minibuffer-allow-text-properties t))
      (setq isearch-new-string
 	   (minibuffer-with-setup-hook
-               (minibuffer-lazy-highlight-setup)
+               (let ((setup (minibuffer-lazy-highlight-setup)))
+                 (lambda ()
+                   ;; Don't add a new search string to the search ring here
+	           ;; in `read-from-minibuffer'. It should be added only
+	           ;; by `isearch-update-ring' called from `isearch-done'.
+	           (setq-local history-add-new-input nil)
+                   (funcall setup)))
              (read-from-minibuffer
 	      (isearch-message-prefix nil isearch-nonincremental)
 	      (cons isearch-string (1+ (or (isearch-fail-pos)
diff --git a/lisp/replace.el b/lisp/replace.el
index fa460a16063..61a1cc7714c 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -205,8 +205,7 @@ query-replace-read-from
 Prompt with PROMPT.  REGEXP-FLAG non-nil means the response should be a regexp.
 The return value can also be a pair (FROM . TO) indicating that the user
 wants to replace FROM with TO."
-  (let* ((history-add-new-input nil)
-         (separator-string
+  (let* ((separator-string
           (when query-replace-from-to-separator
             ;; Check if the first non-whitespace char is displayable
             (if (char-displayable-p
@@ -254,7 +253,8 @@ query-replace-read-from
                 (lambda ()
                   (setq-local text-property-default-nonsticky
                               (append '((separator . t) (face . t))
-                                      text-property-default-nonsticky)))
+                                      text-property-default-nonsticky)
+                              history-add-new-input nil))
               (if regexp-flag
                   (read-regexp
                    (if query-replace-read-from-regexp-default
@@ -342,11 +342,13 @@ query-replace-read-to
 should a regexp."
   (query-replace-compile-replacement
    (save-excursion
-     (let* ((history-add-new-input nil)
-	    (to (read-from-minibuffer
-		 (format "%s %s with: " prompt (query-replace-descr from))
-		 nil nil nil
-		 query-replace-to-history-variable from t)))
+     (let* ((to
+             (minibuffer-with-setup-hook
+                 (lambda () (setq-local history-add-new-input nil))
+                 (read-from-minibuffer
+	          (format "%s %s with: " prompt (query-replace-descr from))
+	          nil nil nil
+	          query-replace-to-history-variable from t))))
        (add-to-history query-replace-to-history-variable to nil t)
        (add-to-history 'query-replace-defaults (cons from to) nil t)
        to))
@@ -903,18 +905,18 @@ read-regexp
 	 (suggestions (if (listp defaults) defaults (list defaults)))
 	 (suggestions (append suggestions (read-regexp-suggestions)))
 	 (suggestions (delete-dups (delq nil (delete "" suggestions))))
-	 ;; Do not automatically add default to the history for empty input.
-	 (history-add-new-input nil)
          ;; `read-regexp--case-fold' dynamically bound and may be
          ;; altered by `M-c'.
          (read-regexp--case-fold case-fold-search)
-	 (input (read-from-minibuffer
-                 (if (string-match-p ":[ \t]*\\'" prompt)
-                     prompt
-                   (format-prompt prompt (and (length> default 0)
-                                              (query-replace-descr default))))
-		 nil read-regexp-map
-                 nil (or history 'regexp-history) suggestions t))
+	 (input (minibuffer-with-setup-hook
+                    (lambda () (setq-local history-add-new-input nil))
+                  (read-from-minibuffer
+                   (if (string-match-p ":[ \t]*\\'" prompt)
+                       prompt
+                     (format-prompt prompt (and (length> default 0)
+                                                (query-replace-descr default))))
+		   nil read-regexp-map
+                   nil (or history 'regexp-history) suggestions t)))
          (result (if (equal input "")
 	             ;; Return the default value when the user enters
 	             ;; empty input.
diff --git a/lisp/simple.el b/lisp/simple.el
index 9a33049f4ca..c5e7f24961c 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -6405,8 +6405,7 @@ read-from-kill-ring
 PROMPT is a string to prompt with."
   ;; `current-kill' updates `kill-ring' with a possible interprogram-paste
   (current-kill 0)
-  (let* ((history-add-new-input nil)
-         (history-pos (when yank-from-kill-ring-rotate
+  (let* ((history-pos (when yank-from-kill-ring-rotate
                         (- (length kill-ring)
                            (length kill-ring-yank-pointer))))
          (ellipsis (if (char-displayable-p ?…) "…" "..."))
@@ -6441,6 +6440,7 @@ read-from-kill-ring
                   read-from-kill-ring-history)))
     (minibuffer-with-setup-hook
         (lambda ()
+          (setq-local history-add-new-input nil)
           ;; Allow ‘SPC’ to be self-inserting
           (use-local-map
            (let ((map (make-sparse-keymap)))
diff --git a/src/minibuf.c b/src/minibuf.c
index 7c0c9799a60..88993659b90 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -948,6 +948,12 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   else
     histstring = Qnil;
 
+  /* Add the value to the appropriate history list, if any.  This is
+     done after the previous buffer has been made current again, in
+     case the history variable is buffer-local.  */
+  if (! (NILP (Vhistory_add_new_input) || NILP (histstring)))
+    call2 (Qadd_to_history, histvar, histstring);
+
   /* The appropriate frame will get selected
      in set-window-configuration.  */
   unbind_to (count, Qnil);
@@ -962,12 +968,6 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
 		      calling_frame))))
     call2 (Qselect_frame_set_input_focus, calling_frame, Qnil);
 
-  /* Add the value to the appropriate history list, if any.  This is
-     done after the previous buffer has been made current again, in
-     case the history variable is buffer-local.  */
-  if (! (NILP (Vhistory_add_new_input) || NILP (histstring)))
-    call2 (Qadd_to_history, histvar, histstring);
-
   /* If Lisp form desired instead of string, parse it.  */
   if (expflag)
     val = string_to_object (val, defalt);
-- 
2.42.0