bug#13579: 24.3.50; query-replace ressurrects previous minibuffer contents

bug#13579: 24.3.50; query-replace ressurrects previous minibuffer contents

[rcopley]

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


From "emacs -Q":
  M-x set-variable <return>
  enable-recursive-minibuffers <return>
  t <return> 
  M-x switch-to-butter
  C-a M-%
  t <return> 
  f <return>
  !
  ;; wait two seconds


Expected (in minibuffer): "swifch-to-buffer".
Actual (in minibutter): "sef-variable".




In GNU Emacs 24.3.50.1 (i386-mingw-nt6.2.9200)
 of 2013-01-28 on MACHINE
Bzr revision: 111617 monnier@iro.umontreal.ca-20130128200035-l5y6lyn7rerbmfjn
Windowing system distributor `Microsoft Corp.', version 6.2.9200
Configured using:
 `configure --with-gcc (4.7) --cflags -I c:/gnuwin32/include --ldflags
 -L c:/gnuwin32/lib'


Important settings:
  value of $LANG: ENG
  locale-coding-system: cp1252
  default enable-multibyte-characters: t


Major mode: Lisp Interaction


Minor modes in effect:
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t


Recent input:
M-x s e t - v a r <return> e n a b l e - r e c u r 
s i v e <return> t <return> M-x s w i t c h - b u t 
t e r C-a M-% t <return> f <return> ! C-g M-x r e p 
o r t - e m a c s - b u g <return>


Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Replaced 1 occurrence
Quit


Load-path shadows:
None found.


Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev
gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils cus-edit easymenu wid-edit cus-start cus-load
help-fns time-date tooltip ediff-hook vc-hooks lisp-float-type mwheel
dos-w32 ls-lisp w32-common-fns disp-table w32-win w32-vars tool-bar dnd
fontset image regexp-opt fringe tabulated-list newcomment lisp-mode
register page menu-bar rfn-eshadow timer select scroll-bar mouse
jit-lock font-lock syntax facemenu font-core frame cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese case-table epa-hook jka-cmpr-hook help simple abbrev
minibuffer button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process w32notify w32
multi-tty emacs)

[-- Attachment #2: Type: text/html, Size: 4121 bytes --]

bug#13579: 24.3.50; query-replace ressurrects previous minibuffer contents

[Juri Linkov]

> From "emacs -Q":
>   M-x set-variable <return>
>   enable-recursive-minibuffers <return>
>   t <return>
>   M-x switch-to-butter
>   C-a M-%
>   t <return>
>   f <return>
>   !
>   ;; wait two seconds
>
> Expected (in minibuffer): "swifch-to-buffer".
> Actual (in minibutter): "sef-variable".

Thanks for the report.  Initially `isearch-forward' is nil, so M-% tries
to search backward going through the minibuffer history (thus it uses the
global value of `isearch-forward' in `minibuffer-history-isearch-search').

It is evident now from your report that the search space for replacements
should be narrowed to the strict set of search functions defined in isearch.el.

This is a regression, so I propose to install the following patch to the
emacs-24 release branch:

=== modified file 'lisp/replace.el'
--- lisp/replace.el	2013-01-01 09:11:05 +0000
+++ lisp/replace.el	2013-01-30 06:03:10 +0000
@@ -1831,7 +1831,7 @@ (defun perform-replace (from-string repl
 		     replace-regexp-lax-whitespace)
 		    (isearch-case-fold-search case-fold-search)
 		    (isearch-forward t))
-		(isearch-search-fun))))
+		(isearch-search-fun-default))))
          (search-string from-string)
          (real-match-data nil)       ; The match data for the current match.
          (next-replacement nil)

bug#13579: 24.3.50; query-replace ressurrects previous minibuffer contents

[Drew Adams]

>  M-x switch-to-butter

Thank you!  That one made my day.

bug#13579: 24.3.50; query-replace ressurrects previous minibuffer contents

[Juri Linkov]

> It is evident now from your report that the search space for replacements
> should be narrowed to the strict set of search functions defined in isearch.el.

If someone wants to perform replacements through the minibuffer history,
this can be implemented as a new separate feature like multi-buffer
replacements (where going to the previous history element is like
going to the previous buffer in a sequence.)

> This is a regression, so I propose to install the following patch to the
> emacs-24 release branch:

`isearch-search-fun-default' prevents the search from going through
the history, but it fixes only part of the problem.  Another part of
the problem is that it tried to go backwards through the history
using the global nil value of `isearch-forward' and ignoring the
let-binding t of `isearch-forward' in `perform-replace'.

That's because `isearch-forward' is not lexically bound when the
lambda in `minibuffer-history-isearch-search' is evaluated.
Now `isearch-search-fun-default' should not use that lambda,
but there is another lambda in `isearch-search-fun-default'
used for `isearch-word' mode.  It causes another regression
when using delimited replacements in the minibuffer - the let-bindings
of isearch variables in `perform-replace' such as binding
`isearch-word' to `delimited-flag' are no more effective
at the moment when the lambda in `isearch-search-fun-default'
is evaluated.

This regression can be fixed by the following patch that will let-bind
isearch-related variables at the moment when the search function
is called:

=== modified file 'lisp/replace.el'
--- lisp/replace.el	2013-01-01 09:11:05 +0000
+++ lisp/replace.el	2013-01-31 00:04:17 +0000
@@ -1823,15 +1823,17 @@ (defun perform-replace (from-string repl
 	  (or (if regexp-flag
 		  replace-re-search-function
 		replace-search-function)
-	      (let ((isearch-regexp regexp-flag)
-		    (isearch-word delimited-flag)
-		    (isearch-lax-whitespace
-		     replace-lax-whitespace)
-		    (isearch-regexp-lax-whitespace
-		     replace-regexp-lax-whitespace)
-		    (isearch-case-fold-search case-fold-search)
-		    (isearch-forward t))
-		(isearch-search-fun))))
+	      (lambda (string &optional bound noerror count)
+		(let ((isearch-regexp regexp-flag)
+		      (isearch-word delimited-flag)
+		      (isearch-lax-whitespace
+		       replace-lax-whitespace)
+		      (isearch-regexp-lax-whitespace
+		       replace-regexp-lax-whitespace)
+		      (isearch-case-fold-search case-fold-search)
+		      (isearch-nonincremental t)
+		      (isearch-forward t))
+		  (funcall (isearch-search-fun-default) string bound noerror count)))))
          (search-string from-string)
          (real-match-data nil)       ; The match data for the current match.
          (next-replacement nil)

bug#13579: 24.3.50; query-replace ressurrects previous minibuffer contents

[Stefan Monnier]

> This regression can be fixed by the following patch that will let-bind
> isearch-related variables at the moment when the search function
> is called:

A better fix is to say that isearch-search-fun-function should return
a function that does not pay attention to isearch-* variables.


        Stefan

bug#13579: 24.3.50; query-replace ressurrects previous minibuffer contents

[Juri Linkov]

> A better fix is to say that isearch-search-fun-function should return
> a function that does not pay attention to isearch-* variables.

It is possible to construct a lambda for `isearch-word' in
`isearch-search-fun-default' that won't use isearch-* variables.
Rewriting more complex functions like `minibuffer-history-isearch-search'
would be more tricky.  And may be some external packages use global values too.
Also there are some variables that still will use the global values like
using `search-whitespace-regexp' in `search-forward-lax-whitespace'
(currently this shouldn't cause a problem since replace.el doesn't use
a replace-specific counterpart option for `search-whitespace-regexp';
introducing an option `replace-whitespace-regexp' might break this).
I'll try to do this in trunk.

Regarding fixing the regression in the emacs-24 branch, I propose a patch
that let-binds isearch-* variables in the same place where `search-function'
is funcalled.  I believe this is a more reliable and less error-prone fix.

=== modified file 'lisp/replace.el'
--- lisp/replace.el	2013-01-01 09:11:05 +0000
+++ lisp/replace.el	2013-02-01 00:15:31 +0000
@@ -1819,19 +1819,6 @@ (defun perform-replace (from-string repl
 	    case-fold-search))
          (nocasify (not (and case-replace case-fold-search)))
          (literal (or (not regexp-flag) (eq regexp-flag 'literal)))
-         (search-function
-	  (or (if regexp-flag
-		  replace-re-search-function
-		replace-search-function)
-	      (let ((isearch-regexp regexp-flag)
-		    (isearch-word delimited-flag)
-		    (isearch-lax-whitespace
-		     replace-lax-whitespace)
-		    (isearch-regexp-lax-whitespace
-		     replace-regexp-lax-whitespace)
-		    (isearch-case-fold-search case-fold-search)
-		    (isearch-forward t))
-		(isearch-search-fun))))
          (search-string from-string)
          (real-match-data nil)       ; The match data for the current match.
          (next-replacement nil)
@@ -1894,6 +1881,26 @@ (defun perform-replace (from-string repl
 	;; Loop finding occurrences that perhaps should be replaced.
 	(while (and keep-going
 		    (not (or (eobp) (and limit (>= (point) limit))))
+		    ;; Let-bind global isearch-* variables to values used
+		    ;; to search the next replacement.  These let-bindings
+		    ;; should be effective both at the time of calling
+		    ;; `isearch-search-fun-default' and also at the
+		    ;; time of funcalling `search-function'.
+		    (let* ((isearch-regexp regexp-flag)
+			   (isearch-word delimited-flag)
+			   (isearch-lax-whitespace
+			    replace-lax-whitespace)
+			   (isearch-regexp-lax-whitespace
+			    replace-regexp-lax-whitespace)
+			   (isearch-case-fold-search case-fold-search)
+			   (isearch-adjusted nil)
+			   (isearch-nonincremental t)
+			   (isearch-forward t)
+			   (search-function
+			    (or (if regexp-flag
+				    replace-re-search-function
+				  replace-search-function)
+				(isearch-search-fun-default))))
 		    ;; Use the next match if it is already known;
 		    ;; otherwise, search for a match after moving forward
 		    ;; one char if progress is required.
@@ -1926,7 +1933,7 @@ (defun perform-replace (from-string repl
 				       (replace-match-data
 					t real-match-data)
 				     (goto-char opoint)
-				     nil))))))
+				       nil)))))))
 
 	  ;; Record whether the match is nonempty, to avoid an infinite loop
 	  ;; repeatedly matching the same empty string.

bug#13579: 24.3.50; query-replace ressurrects previous minibuffer contents

[Stefan Monnier]

> I'll try to do this in trunk.

Thanks.

> Regarding fixing the regression in the emacs-24 branch, I propose a patch
> that let-binds isearch-* variables in the same place where `search-function'
> is funcalled.  I believe this is a more reliable and less error-prone fix.

That looks fine, yes, thank you,


        Stefan

bug#13579: 24.3.50; query-replace ressurrects previous minibuffer contents

[Juri Linkov]

Version: 24.2.92

> That looks fine, yes, thank you,

Fixed in emacs-24 branch.