bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character.

bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character.

[Nicolas Richard]

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

From emacs -Q:

;; We do a query-replace to remove NUL characters and replace them by
;; an empty string:
 M-% [query-replace]
 C-q [quoted-insert]
 0 <return> <return> [exit-minibuffer]
 <return> [exit-minibuffer]
;; now we try to do it again, by relying on the history
 M-% [query-replace]
 <up> [previous-line-or-history-element]
 <return> [exit-minibuffer]
;; At this point, emacs asks what to replace "two NUL chars" with,
;; instead of removing NULs like the previous query-replace did.

In GNU Emacs 25.0.50.1 (i686-pc-linux-gnu, X toolkit, Xaw scroll bars)
 of 2015-05-07 on Aurora
Windowing system distributor `The X.Org Foundation', version 11.0.11501000
System Description:	Ubuntu 14.04.2 LTS

I suggest the following changes :

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-confusion-in-query-replace-history-when-replac.patch --]
[-- Type: text/x-diff, Size: 4261 bytes --]

From 5eecb757d03a9f6986fd82fad1f2413ff3983726 Mon Sep 17 00:00:00 2001
From: Nicolas Richard <youngfrog@members.fsf.org>
Date: Fri, 29 May 2015 10:32:05 +0200
Subject: [PATCH 1/2] Avoid confusion in query-replace history when replacing
 NUL chars.

* lisp/replace.el (query-replace--split-string): New function.
(query-replace-read-from): Rely on the 'separator' property
instead of searching for the NUL character.
---
 lisp/replace.el | 52 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/lisp/replace.el b/lisp/replace.el
index 8e71615..0ab9b83 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -136,6 +136,16 @@ See `replace-regexp' and `query-replace-regexp-eval'.")
 (defun query-replace-descr (string)
   (mapconcat 'isearch-text-char-description string ""))
 
+(defun query-replace--split-string (string)
+  "Split string STRING at a character with property `separator'"
+  (let* ((length (length string))
+         (split-pos (text-property-any 0 length 'separator t string)))
+    (if (not split-pos)
+        string
+      (cl-assert (not (text-property-any (1+ split-pos) length 'separator t string)))
+      (cons (substring-no-properties string 0 split-pos)
+            (substring-no-properties string (1+ split-pos) length)))))
+
 (defun query-replace-read-from (prompt regexp-flag)
   "Query and return the `from' argument of a query-replace operation.
 The return value can also be a pair (FROM . TO) indicating that the user
@@ -174,32 +184,30 @@ wants to replace FROM with TO."
 		  (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)))))
+		 (car (if regexp-flag regexp-search-ring search-ring)) t))))
+           (to))
       (if (and (zerop (length from)) query-replace-defaults)
 	  (cons (caar query-replace-defaults)
 		(query-replace-compile-replacement
 		 (cdar query-replace-defaults) regexp-flag))
-	(let* ((to (if (and (string-match separator from)
-			    (get-text-property (match-beginning 0) 'separator from))
-		       (substring-no-properties from (match-end 0))))
-	       (from (if to (substring-no-properties from 0 (match-beginning 0))
-		       (substring-no-properties from))))
-	  (add-to-history query-replace-from-history-variable from nil t)
-	  ;; Warn if user types \n or \t, but don't reject the input.
-	  (and regexp-flag
-	       (string-match "\\(\\`\\|[^\\]\\)\\(\\\\\\\\\\)*\\(\\\\[nt]\\)" from)
-	       (let ((match (match-string 3 from)))
-		 (cond
-		  ((string= match "\\n")
-		   (message "Note: `\\n' here doesn't match a newline; to do that, type C-q C-j instead"))
-		  ((string= match "\\t")
-		   (message "Note: `\\t' here doesn't match a tab; to do that, just type TAB")))
-		 (sit-for 2)))
-	  (if (not to)
-	      from
-	    (add-to-history query-replace-to-history-variable to nil t)
-	    (add-to-history 'query-replace-defaults (cons from to) nil t)
-	    (cons from (query-replace-compile-replacement to regexp-flag))))))))
+        (setq from (query-replace--split-string from))
+        (when (consp from) (setq to (cdr from) from (car from)))
+        (add-to-history query-replace-from-history-variable from nil t)
+        ;; Warn if user types \n or \t, but don't reject the input.
+        (and regexp-flag
+             (string-match "\\(\\`\\|[^\\]\\)\\(\\\\\\\\\\)*\\(\\\\[nt]\\)" from)
+             (let ((match (match-string 3 from)))
+               (cond
+                ((string= match "\\n")
+                 (message "Note: `\\n' here doesn't match a newline; to do that, type C-q C-j instead"))
+                ((string= match "\\t")
+                 (message "Note: `\\t' here doesn't match a tab; to do that, just type TAB")))
+               (sit-for 2)))
+        (if (not to)
+            from
+          (add-to-history query-replace-to-history-variable to nil t)
+          (add-to-history 'query-replace-defaults (cons from to) nil t)
+          (cons from (query-replace-compile-replacement to regexp-flag)))))))
 
 (defun query-replace-compile-replacement (to regexp-flag)
   "Maybe convert a regexp replacement TO to Lisp.
-- 
1.9.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-test-for-previous-commit.patch --]
[-- Type: text/x-diff, Size: 1997 bytes --]

From 2daee12b2f1463a921a780f41ff00df670360b61 Mon Sep 17 00:00:00 2001
From: Nicolas Richard <youngfrog@members.fsf.org>
Date: Fri, 29 May 2015 10:33:35 +0200
Subject: [PATCH 2/2] Add test for previous commit

* test/automated/replace-tests.el: New file.
(query-replace--split-string-tests): Add test for previous commit.
---
 test/automated/replace-tests.el | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 test/automated/replace-tests.el

diff --git a/test/automated/replace-tests.el b/test/automated/replace-tests.el
new file mode 100644
index 0000000..f4e474b
--- /dev/null
+++ b/test/automated/replace-tests.el
@@ -0,0 +1,35 @@
+;;; replace-tests.el --- tests for replace.el.
+
+;; Copyright (C) 2015 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert)
+
+(ert-deftest query-replace--split-string-tests ()
+  (let ((sep (propertize "\0" 'separator t)))
+    (dolist (before '("" "b"))
+      (dolist (after '("" "a"))
+        (should (equal
+                 (query-replace--split-string (concat before sep after))
+                 (cons before after)))
+        (should (equal
+                 (query-replace--split-string (concat before "\0" after))
+                 (concat before "\0" after)))))))
+
+;;; replace-tests.el ends here
-- 
1.9.1

bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character.

[Juri Linkov]

> ;; We do a query-replace to remove NUL characters and replace them by
> ;; an empty string:
>  M-% [query-replace]
>  C-q [quoted-insert]
>  0 <return> <return> [exit-minibuffer]
>  <return> [exit-minibuffer]
> ;; now we try to do it again, by relying on the history
>  M-% [query-replace]
>  <up> [previous-line-or-history-element]
>  <return> [exit-minibuffer]
> ;; At this point, emacs asks what to replace "two NUL chars" with,
> ;; instead of removing NULs like the previous query-replace did.
>
> I suggest the following changes :

Thank you for the fix!  I noticed that besides fixing the NUL character problem
your patch also changes the behaviour when there are no NUL characters, i.e.:

> +(defun query-replace--split-string (string)
> +  "Split string STRING at a character with property `separator'"
> +  (let* ((length (length string))
> +         (split-pos (text-property-any 0 length 'separator t string)))
> +    (if (not split-pos)
> +        string
           ======
This used to be (substring-no-properties string)

> -	(let* ((to (if (and (string-match separator from)
> -			    (get-text-property (match-beginning 0) 'separator from))
> -		       (substring-no-properties from (match-end 0))))
> -	       (from (if to (substring-no-properties from 0 (match-beginning 0))
> -		       (substring-no-properties from))))
                        =======================
Like you can see here above that stripes properties from the from string.

bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character.

[Nicolas Richard]

Juri Linkov <juri@linkov.net> writes:
>> +(defun query-replace--split-string (string)
>> +  "Split string STRING at a character with property `separator'"
>> +  (let* ((length (length string))
>> +         (split-pos (text-property-any 0 length 'separator t string)))
>> +    (if (not split-pos)
>> +        string
>            ======
> This used to be (substring-no-properties string)

Indeed, thanks for spotting this. I've corrected it and mentionned this
bug report.

pushed as
bc9d9bc7a8d56303595899cd66db67ef90d3a4cd and
c7695d0adb125a3817f3df015137287e801e457a

Nico.

bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character.

[Juri Linkov]

> +(defun query-replace--split-string (string)
> +  "Split string STRING at a character with property `separator'"
> +  (let* ((length (length string))
> +         (split-pos (text-property-any 0 length 'separator t string)))
> +    (if (not split-pos)
> +        string
> +      (cl-assert (not (text-property-any (1+ split-pos) length 'separator t string)))
> +      (cons (substring-no-properties string 0 split-pos)
> +            (substring-no-properties string (1+ split-pos) length)))))

Thanks to cl-assert it signaled an error with ‘M-% a RET RET C-g M-% M-p C-e z RET’
so I fixed it in 1b1b6644c8cb27539ca99e97ef2f352f411c06d8.

bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character.

[Nicolas Richard]

Juri Linkov <juri@linkov.net> writes:
> Thanks to cl-assert it signaled an error with ‘M-% a RET RET C-g M-% M-p C-e z RET’
> so I fixed it in 1b1b6644c8cb27539ca99e97ef2f352f411c06d8.

Thank you.

Nico.