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

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]

1. emacs -Q
2. (setq enable-recursive-minibuffers t)
3. M-y
4. In the minibuffer (with the prompt "Yank from kill-ring: "),
   type M-x calendar RET (or any other command).
5. M-x M-p
   Expected: "calendar" is inserted in the minibuffer.
   Observed: error saying "Beginning of history; no preceding item".

The problem is that the minibuffer history of M-x isn't recorded when
you invoke M-x from within the minibuffer of read-from-kill-ring (M-y).
The reason is that read-from-kill-ring let binds history-add-new-input,
and that affects all recursive minibuffers as well, so no minibuffer
history is recorded until you exit the first (non-recursive) minibuffer.

AFAICT This issue affects all uses history-add-new-input, unfortunately,
not only read-from-kill-ring, since it's always used via let-bindings.

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

[Eli Zaretskii]

> Date: Sun, 11 Feb 2024 16:54:43 +0100
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> 
> 1. emacs -Q
> 2. (setq enable-recursive-minibuffers t)
> 3. M-y
> 4. In the minibuffer (with the prompt "Yank from kill-ring: "),
>    type M-x calendar RET (or any other command).
> 5. M-x M-p
>    Expected: "calendar" is inserted in the minibuffer.
>    Observed: error saying "Beginning of history; no preceding item".
> 
> The problem is that the minibuffer history of M-x isn't recorded when
> you invoke M-x from within the minibuffer of read-from-kill-ring (M-y).
> The reason is that read-from-kill-ring let binds history-add-new-input,
> and that affects all recursive minibuffers as well, so no minibuffer
> history is recorded until you exit the first (non-recursive) minibuffer.
> 
> AFAICT This issue affects all uses history-add-new-input, unfortunately,
> not only read-from-kill-ring, since it's always used via let-bindings.

I'm not sure we should be interested in fixing this.  Recursive
minibuffers are not supposed to start a completely new command loop
unaffected by whatever was before it, so we shouldn't try.  Even if
this particular case is solved (which I'm not sure we can), there are
a legion of other similar situations, where something let-bound by a
command entering the minibuffer affects all the recursive minibuffers.
Let-binding in commands that prompt users is ubiquitous in Emacs.

It's easy enough to work around the problem: C-g (perhaps more than
once), then start afresh.

So I tend to close this as wontfix.

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]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sun, 11 Feb 2024 16:54:43 +0100
>> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>>
>> 1. emacs -Q
>> 2. (setq enable-recursive-minibuffers t)
>> 3. M-y
>> 4. In the minibuffer (with the prompt "Yank from kill-ring: "),
>>    type M-x calendar RET (or any other command).
>> 5. M-x M-p
>>    Expected: "calendar" is inserted in the minibuffer.
>>    Observed: error saying "Beginning of history; no preceding item".
>>
>> The problem is that the minibuffer history of M-x isn't recorded when
>> you invoke M-x from within the minibuffer of read-from-kill-ring (M-y).
>> The reason is that read-from-kill-ring let binds history-add-new-input,
>> and that affects all recursive minibuffers as well, so no minibuffer
>> history is recorded until you exit the first (non-recursive) minibuffer.
>>
>> AFAICT This issue affects all uses history-add-new-input, unfortunately,
>> not only read-from-kill-ring, since it's always used via let-bindings.
>
> I'm not sure we should be interested in fixing this.  Recursive
> minibuffers are not supposed to start a completely new command loop
> unaffected by whatever was before it, so we shouldn't try.

I see that, but the problem, IMO, is that there's nothing telling you
that you're in this state of not recording minibuffer history.  You
likely won't know that you're using a command that let-binds
history-add-new-input when you enter a recursive minibuffer, and losing
all minibuffer history from commands you invoked in the recursive edit
may come as an unpleasant surprise.

> Even if this particular case is solved (which I'm not sure we can),
> there are a legion of other similar situations, where something
> let-bound by a command entering the minibuffer affects all the
> recursive minibuffers.  Let-binding in commands that prompt users is
> ubiquitous in Emacs.

Indeed, this issue is possibly broader.  Often the solution is to use
minibuffer-setup-hook to bind a variable buffer-locally in a minibuffer,
rather than let-binding it (affecting all recursive minibuffers).  For
history-add-new-input this is slightly trickier since read_minibuf
checks the value of this variable only after the minibuffer is exited.
I'm experimenting with a possible solution where we change read_minibuf
to grab the buffer-local value of this variable from the minibuffer, and
change all users of history-add-new-input to set it buffer-locally
instead of let-binding it.  Works pretty well, but it doesn't cover
third party code that uses this variable, naturally.

> It's easy enough to work around the problem: C-g (perhaps more than
> once), then start afresh.
>
> So I tend to close this as wontfix.

All right, fair enough.

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

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 69056@debbugs.gnu.org
> Date: Sun, 11 Feb 2024 18:42:49 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'm not sure we should be interested in fixing this.  Recursive
> > minibuffers are not supposed to start a completely new command loop
> > unaffected by whatever was before it, so we shouldn't try.
> 
> I see that, but the problem, IMO, is that there's nothing telling you
> that you're in this state of not recording minibuffer history.  You
> likely won't know that you're using a command that let-binds
> history-add-new-input when you enter a recursive minibuffer, and losing
> all minibuffer history from commands you invoked in the recursive edit
> may come as an unpleasant surprise.

We should probably document this caveat.  enable-recursive-minibuffers
is an advanced feature, not recommended to newbies.

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

[Eli Zaretskii]

> Cc: 69056@debbugs.gnu.org
> Date: Sun, 11 Feb 2024 19:50:21 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Eshel Yaron <me@eshelyaron.com>
> > Cc: 69056@debbugs.gnu.org
> > Date: Sun, 11 Feb 2024 18:42:49 +0100
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > > I'm not sure we should be interested in fixing this.  Recursive
> > > minibuffers are not supposed to start a completely new command loop
> > > unaffected by whatever was before it, so we shouldn't try.
> > 
> > I see that, but the problem, IMO, is that there's nothing telling you
> > that you're in this state of not recording minibuffer history.  You
> > likely won't know that you're using a command that let-binds
> > history-add-new-input when you enter a recursive minibuffer, and losing
> > all minibuffer history from commands you invoked in the recursive edit
> > may come as an unpleasant surprise.
> 
> We should probably document this caveat.  enable-recursive-minibuffers
> is an advanced feature, not recommended to newbies.

Stefan & Stefan, any comments or suggestions, beyond documenting this
caveat?

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

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

> 1. emacs -Q
> 2. (setq enable-recursive-minibuffers t)
> 3. M-y
> 4. In the minibuffer (with the prompt "Yank from kill-ring: "),
>    type M-x calendar RET (or any other command).
> 5. M-x M-p
>    Expected: "calendar" is inserted in the minibuffer.
>    Observed: error saying "Beginning of history; no preceding item".
>
> The problem is that the minibuffer history of M-x isn't recorded when
> you invoke M-x from within the minibuffer of read-from-kill-ring (M-y).
> The reason is that read-from-kill-ring let binds history-add-new-input,
> and that affects all recursive minibuffers as well, so no minibuffer
> history is recorded until you exit the first (non-recursive) minibuffer.

I suspect this can bite in more cases, including cases which don't
involve setting `enable-recursive-minibuffers` to t.
We should probably change the way `history-add-new-input` works so it's
attached to a particular minibuffer rather than being dynbound.


        Stefan

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]

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

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

>> 1. emacs -Q
>> 2. (setq enable-recursive-minibuffers t)
>> 3. M-y
>> 4. In the minibuffer (with the prompt "Yank from kill-ring: "),
>>    type M-x calendar RET (or any other command).
>> 5. M-x M-p
>>    Expected: "calendar" is inserted in the minibuffer.
>>    Observed: error saying "Beginning of history; no preceding item".
>>
>> The problem is that the minibuffer history of M-x isn't recorded when
>> you invoke M-x from within the minibuffer of read-from-kill-ring (M-y).
>> The reason is that read-from-kill-ring let binds history-add-new-input,
>> and that affects all recursive minibuffers as well, so no minibuffer
>> history is recorded until you exit the first (non-recursive) minibuffer.
>
> I suspect this can bite in more cases, including cases which don't
> involve setting `enable-recursive-minibuffers` to t.
> We should probably change the way `history-add-new-input` works so it's
> attached to a particular minibuffer rather than being dynbound.

Thanks, that's what I thought too.  Here's an attempt do just that:


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

From 35c7cf51102b5625a46bdb0dc5f7f2299659f699 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Sun, 11 Feb 2024 16:18:48 +0100
Subject: [PATCH] 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.
---
 doc/lispref/minibuf.texi |  6 +++---
 lisp/isearch.el          | 12 +++++++-----
 lisp/replace.el          | 36 +++++++++++++++++++-----------------
 lisp/simple.el           |  4 ++--
 src/minibuf.c            |  7 ++++++-
 5 files changed, 37 insertions(+), 28 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..b6126fe5c87 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -585,6 +585,7 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   /* String to add to the history.  */
   Lisp_Object histstring;
   Lisp_Object histval;
+  bool nohist = false;
 
   Lisp_Object empty_minibuf;
 
@@ -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));
+
   recursive_edit_1 ();
 
   /* If cursor is on the minibuffer line,
@@ -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.  */
@@ -2311,6 +2315,7 @@ syms_of_minibuf (void)
   Fset (Qcustom_variable_history, Qnil);
 
   DEFSYM (Qminibuffer_history, "minibuffer-history");
+  DEFSYM (Qhistory_add_new_input, "history-add-new-input");
   DEFSYM (Qbuffer_name_history, "buffer-name-history");
   Fset (Qbuffer_name_history, Qnil);
 
-- 
2.42.0

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

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

> 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`?
[ 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?


        Stefan

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]

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

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

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

>> 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...

The handling of Vfoo is quite delicate, but it does give you the value
in the current-buffer (i.e. they're changed as needed whenever we go
through `set_buffer`).

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

LGTM.
Eli&Stefan, any objection?


        Stefan

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

[Eli Zaretskii]

> Cc: 69056@debbugs.gnu.org
> Date: Thu, 15 Feb 2024 12:56:43 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > Thanks, that's what I thought too.  Here's an attempt do just that:
> 
> Looks pretty good.

Is this really worthwhile?  It might solve some problems with commands
invoked from the recursive edit, but it doesn't solve all of them,
because the value of history-add-new-input is still set in that
minibuffer.  And it introduces tricky effects due to the variable
being buffer-local for any code that let-binds history-add-new-input,
and could potentially break something because of that.

I'm afraid I don't like this change, for those reasons.

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

[Eli Zaretskii]

> Cc: 69056@debbugs.gnu.org
> Date: Thu, 15 Feb 2024 14:20:08 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> >> 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...
> 
> The handling of Vfoo is quite delicate, but it does give you the value
> in the current-buffer (i.e. they're changed as needed whenever we go
> through `set_buffer`).
> 
> > Oh, that's much simpler indeed.  And it seems to work just as well.
> > Here's an updated patch (v2):
> 
> LGTM.
> Eli&Stefan, any objection?

Yes, see my other message.  I feel like we are making an effort to
change the internals, which runs the usual risk of breaking things,
for very little gain.  The more general issue with let-binding
variables around APIs that enter the minibuffer stays, so I see little
sense to fix just this one problem in an incomplete way that could on
top of that break existing code.

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

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

> for very little gain.  The more general issue with let-binding
> variables around APIs that enter the minibuffer stays, so I see little

I agree that it would be good to tackle this more general problem.

Basically, should we treat recursive-edits as if they were run in a sort
of separate thread (with the original thread blocked until the new
thread exits)?

I think we can't do that in general, since I think we sometimes do want
let-bindings performed around `read-from-minibuffer` to affect the
command executed inside the minibuffer.  But we should maybe experiment
with it to get a clearer idea of where we do want/need it and where
we don't.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: me@eshelyaron.com,  69056@debbugs.gnu.org
> Date: Thu, 15 Feb 2024 14:54:07 -0500
> 
> > for very little gain.  The more general issue with let-binding
> > variables around APIs that enter the minibuffer stays, so I see little
> 
> I agree that it would be good to tackle this more general problem.
> 
> Basically, should we treat recursive-edits as if they were run in a sort
> of separate thread (with the original thread blocked until the new
> thread exits)?

I don't think we can do that by default.  We need some evidence that
this is the intent.

> I think we can't do that in general, since I think we sometimes do want
> let-bindings performed around `read-from-minibuffer` to affect the
> command executed inside the minibuffer.

Exactly.  And doing so is a very wide-spread paradigm in Emacs.  Which
is one reason why enable-recursive-minibuffers is not turned on by
default, and should be considered an advanced feature for users who
"know what they are doing" and are ready to sustain the consequences.

> But we should maybe experiment with it to get a clearer idea of
> where we do want/need it and where we don't.

Perhaps.  But I'd like to hear concrete ideas for such experiments
before I have an opinion on their value.