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

Previous Next

Full log

View this message in rfc822 format

From: help-debbugs <at> gnu.org (GNU bug Tracking System)
To: Nicolas Richard <youngfrog <at> members.fsf.org>
Subject: bug#20690: closed (Re: bug#20690: 25.0.50; query-replace:
 incorrect history when replacing the NUL character.)
Date: Tue, 02 Jun 2015 09:36:04 +0000

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

Your bug report

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

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to 20690 <at> debbugs.gnu.org.

-- 
20690: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=20690
GNU Bug Tracking System
Contact help-debbugs <at> gnu.org with problems

[Message part 2 (message/rfc822, inline)]

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 part 3 (message/rfc822, inline)]

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 4 (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

Previous Next

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