bug#24580: 25.2.50; query-replace history doesn't work

[Mark Oteiza]

On 02/10/16 at 12:51pm, Mark Oteiza wrote:
> On 02/10/16 at 10:02am, Eli Zaretskii wrote:
> > > Date: Sun, 2 Oct 2016 00:17:15 -0400
> > > From: Mark Oteiza <mvoteiza@udel.edu>
> > > Cc: Marcin Borkowski <mbork@mbork.pl>, 24580@debbugs.gnu.org
> > > 
> > > > > - start emacs -Q
> > > > > - press `a' `C-b', `M-%', `a', `RET', `b', `RET', `!'
> > > > > - press `M-%', `M-p'
> > > > > 
> > > > > Emacs says "Beginning of history; no preceding item".
> > > > 
> > > > Mark, this regression was caused by the lexical-binding change in
> > > > replace.el.  Could you please take a look?
> > > 
> > > The problem lies (at least) in query-replace-read-from, where
> > > query-replace-from-to is let bound, and later read-from-minibuffer
> > > refers to the symbol 'query-replace-from-to.  At first sight, it looks
> > > like it could be fixed by putting some of the setup done by these
> > > bindings into the minibuffer setup hook.
> > 
> > Isn't there a less intrusive change to fix this?  A significant change
> > like the one you propose is more risky, but if there's no safer one, I
> > guess we should take it.
> 
> Perhaps just adding a (defvar query-replace-from-to-history …) will be
> sufficient.  That read-from-minibuffer and friends' HIST argument can
> only be a symbol is a bit frustrating.

Surely there is a better way.

diff --git a/lisp/replace.el b/lisp/replace.el
index 4256751..0a373db 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -90,6 +90,9 @@ query-replace-from-to-separator
   :type '(choice string (sexp :tag "Display specification"))
   :version "25.1")
 
+(defvar query-replace-from-to-history nil
+  "???")
+
 (defcustom query-replace-from-history-variable 'query-replace-history
   "History list to use for the FROM argument of `query-replace' commands.
 The value of this variable should be a symbol; that symbol
@@ -174,20 +177,7 @@ query-replace-read-from
              (propertize "\0"
                          'display query-replace-from-to-separator
                          'separator t)))
-          (query-replace-from-to-history
-           (append
-            (when separator
-              (mapcar (lambda (from-to)
-                        (concat (query-replace-descr (car from-to))
-                                separator
-                                (query-replace-descr (cdr from-to))))
-                      query-replace-defaults))
-            (symbol-value query-replace-from-history-variable)))
           (minibuffer-allow-text-properties t) ; separator uses text-properties
-          (prompt
-           (if (and query-replace-defaults separator)
-               (format "%s (default %s): " prompt (car 
query-replace-from-to-history))
-             (format "%s: " prompt)))
           (from
            ;; The save-excursion here is in case the user marks and copies
            ;; a region in order to specify the minibuffer input.
@@ -195,13 +185,26 @@ query-replace-read-from
            (save-excursion
               (minibuffer-with-setup-hook
                   (lambda ()
+                    (setq query-replace-from-to-history
+                          (append
+                           (when separator
+                             (mapcar (lambda (from-to)
+                                       (concat (query-replace-descr (car 
from-to))
+                                               separator
+                                               (query-replace-descr (cdr 
from-to))))
+                                     query-replace-defaults))
+                           (symbol-value query-replace-from-history-variable)))
                     (setq-local text-property-default-nonsticky
                                 (cons '(separator . t) 
text-property-default-nonsticky)))
-                (if regexp-flag
-                    (read-regexp prompt nil 'query-replace-from-to-history)
-                  (read-from-minibuffer
-                   prompt nil nil nil 'query-replace-from-to-history
-                   (car (if regexp-flag regexp-search-ring search-ring)) t)))))
+                (let ((prompt
+                       (if (and query-replace-defaults separator)
+                           (format "%s (default %s): " prompt (car 
query-replace-from-to-history))
+                         (format "%s: " prompt))))
+                  (if regexp-flag
+                      (read-regexp prompt nil 'query-replace-from-to-history)
+                    (read-from-minibuffer
+                     prompt nil nil nil 'query-replace-from-to-history
+                     (car (if regexp-flag regexp-search-ring search-ring)) 
t))))))
            (to))
       (if (and (zerop (length from)) query-replace-defaults)
          (cons (caar query-replace-defaults)