* bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character.
@ 2015-05-29 8:41 Nicolas Richard
2015-06-01 20:39 ` Juri Linkov
2015-06-22 22:59 ` Juri Linkov
0 siblings, 2 replies; 5+ messages in thread
From: Nicolas Richard @ 2015-05-29 8:41 UTC (permalink / raw)
To: 20690
[-- 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character.
2015-05-29 8:41 bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character Nicolas Richard
@ 2015-06-01 20:39 ` Juri Linkov
2015-06-02 9:30 ` Nicolas Richard
2015-06-22 22:59 ` Juri Linkov
1 sibling, 1 reply; 5+ messages in thread
From: Juri Linkov @ 2015-06-01 20:39 UTC (permalink / raw)
To: Nicolas Richard; +Cc: 20690
> ;; 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character.
2015-06-01 20:39 ` Juri Linkov
@ 2015-06-02 9:30 ` Nicolas Richard
0 siblings, 0 replies; 5+ messages in thread
From: Nicolas Richard @ 2015-06-02 9:30 UTC (permalink / raw)
To: Juri Linkov; +Cc: 20690-done
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character.
2015-05-29 8:41 bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character Nicolas Richard
2015-06-01 20:39 ` Juri Linkov
@ 2015-06-22 22:59 ` Juri Linkov
2015-06-23 5:11 ` Nicolas Richard
1 sibling, 1 reply; 5+ messages in thread
From: Juri Linkov @ 2015-06-22 22:59 UTC (permalink / raw)
To: Nicolas Richard; +Cc: 20690
> +(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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character.
2015-06-22 22:59 ` Juri Linkov
@ 2015-06-23 5:11 ` Nicolas Richard
0 siblings, 0 replies; 5+ messages in thread
From: Nicolas Richard @ 2015-06-23 5:11 UTC (permalink / raw)
To: Juri Linkov; +Cc: 20690
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-06-23 5:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-29 8:41 bug#20690: 25.0.50; query-replace: incorrect history when replacing the NUL character Nicolas Richard
2015-06-01 20:39 ` Juri Linkov
2015-06-02 9:30 ` Nicolas Richard
2015-06-22 22:59 ` Juri Linkov
2015-06-23 5:11 ` Nicolas Richard
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.