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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 20690 in the body.
You can then email your comments to 20690 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Richard <youngfrog <at> members.fsf.org>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.0.50;
 query-replace: incorrect history when replacing the NUL character.
Date: Fri, 29 May 2015 10:41:42 +0200

[Message part 1 (text/plain, inline)]

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 :

[0001-Avoid-confusion-in-query-replace-history-when-replac.patch (text/x-diff, inline)]

From 5eecb757d03a9f6986fd82fad1f2413ff3983726 Mon Sep 17 00:00:00 2001
From: Nicolas Richard <youngfrog <at> 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

[0002-Add-test-for-previous-commit.patch (text/x-diff, inline)]

From 2daee12b2f1463a921a780f41ff00df670360b61 Mon Sep 17 00:00:00 2001
From: Nicolas Richard <youngfrog <at> 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

Message #8 received at 20690 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Nicolas Richard <youngfrog <at> members.fsf.org>
Cc: 20690 <at> debbugs.gnu.org
Subject: Re: bug#20690: 25.0.50;
 query-replace: incorrect history when replacing the NUL character.
Date: Mon, 01 Jun 2015 23:39:45 +0300

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

Message #13 received at 20690-done <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Richard <youngfrog <at> members.fsf.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 20690-done <at> debbugs.gnu.org
Subject: Re: bug#20690: 25.0.50;
 query-replace: incorrect history when replacing the NUL character.
Date: Tue, 02 Jun 2015 11:30:13 +0200

Juri Linkov <juri <at> 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.

Message #16 received at 20690 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Nicolas Richard <youngfrog <at> members.fsf.org>
Cc: 20690 <at> debbugs.gnu.org
Subject: Re: bug#20690: 25.0.50;
 query-replace: incorrect history when replacing the NUL character.
Date: Tue, 23 Jun 2015 01:59:54 +0300

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

Message #19 received at 20690 <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Richard <youngfrog <at> members.fsf.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 20690 <at> debbugs.gnu.org
Subject: Re: bug#20690: 25.0.50;
 query-replace: incorrect history when replacing the NUL character.
Date: Tue, 23 Jun 2015 07:11:54 +0200

Juri Linkov <juri <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.