GNU bug report logs - #7585
23.2.90; [PATCH] fix eshell-previous-matching-input

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 7585 in the body.
You can then email your comments to 7585 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: Leo <sdl.web <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 23.2.90; [PATCH] fix eshell-previous-matching-input
Date: Tue, 07 Dec 2010 18:51:07 +0000

There is a customisable variable eshell-hist-move-to-end when set to
nil, point is not guaranteed to be located behind
eshell-last-output-end. Thus blindly calling delete-region and
insert-and-inherit will generate an error in that case.


diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index 45fe050..defaf5a 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -837,6 +837,8 @@ With prefix argument N, search for Nth previous match.
 If N is negative, find the next or Nth next match."
   (interactive (eshell-regexp-arg "Previous input matching (regexp): "))
   (setq arg (eshell-search-arg arg))
+  (assert (<= eshell-last-output-end (point))
+	  nil "Point not located after prompt")
   (let ((pos (eshell-previous-matching-input-string-position regexp arg)))
     ;; Has a match been found?
     (if (null pos)

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo <sdl.web <at> gmail.com>
Cc: John Wiegley <jwiegley <at> gmail.com>, 7585 <at> debbugs.gnu.org
Subject: Re: bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input
Date: Mon, 13 Dec 2010 12:00:19 -0500

> There is a customisable variable eshell-hist-move-to-end when set to
> nil, point is not guaranteed to be located behind
> eshell-last-output-end. Thus blindly calling delete-region and
> insert-and-inherit will generate an error in that case.

The patch looks good as well, tho we usually prefer not to use `assert'
in this way (I'd keep assert for actual coding errors, so better use an
if test that then `signal's an error).


        Stefan

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

From: Leo <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: John Wiegley <jwiegley <at> gmail.com>, 7585 <at> debbugs.gnu.org
Subject: Re: bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input
Date: Mon, 13 Dec 2010 18:31:00 +0000

On 2010-12-13 17:00 +0000, Stefan Monnier wrote:
>> There is a customisable variable eshell-hist-move-to-end when set to
>> nil, point is not guaranteed to be located behind
>> eshell-last-output-end. Thus blindly calling delete-region and
>> insert-and-inherit will generate an error in that case.
>
> The patch looks good as well, tho we usually prefer not to use `assert'
> in this way (I'd keep assert for actual coding errors, so better use an
> if test that then `signal's an error).

Would something like the following acceptable? Thanks. Leo

2010-12-13  Leo <sdl.web <at> gmail.com>

	* eshell/em-hist.el (eshell-previous-matching-input): Check point
	is located after eshell prompt in case `eshell-hist-move-to-end'
	is nil.

 lisp/eshell/em-hist.el |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index 45fe050..47e0b2b 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -837,16 +837,18 @@ With prefix argument N, search for Nth previous match.
 If N is negative, find the next or Nth next match."
   (interactive (eshell-regexp-arg "Previous input matching (regexp): "))
   (setq arg (eshell-search-arg arg))
-  (let ((pos (eshell-previous-matching-input-string-position regexp arg)))
-    ;; Has a match been found?
-    (if (null pos)
-	(error "Not found")
-      (setq eshell-history-index pos)
-      (unless (minibuffer-window-active-p (selected-window))
-	(message "History item: %d" (- (ring-length eshell-history-ring) pos)))
-       ;; Can't use kill-region as it sets this-command
-      (delete-region eshell-last-output-end (point))
-      (insert-and-inherit (eshell-get-history pos)))))
+  (if (<= eshell-last-output-end (point))
+      (let ((pos (eshell-previous-matching-input-string-position regexp arg)))
+	;; Has a match been found?
+	(if (null pos)
+	    (error "Not found")
+	  (setq eshell-history-index pos)
+	  (unless (minibuffer-window-active-p (selected-window))
+	    (message "History item: %d" (- (ring-length eshell-history-ring) pos)))
+	  ;; Can't use kill-region as it sets this-command
+	  (delete-region eshell-last-output-end (point))
+	  (insert-and-inherit (eshell-get-history pos))))
+    (message "Point not located after prompt")))
 
 (defun eshell-next-matching-input (regexp arg)
   "Search forwards through input history for match for REGEXP.
-- 
1.7.3

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo <sdl.web <at> gmail.com>
Cc: John Wiegley <jwiegley <at> gmail.com>, 7585 <at> debbugs.gnu.org
Subject: Re: bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input
Date: Mon, 13 Dec 2010 15:42:39 -0500

> Would something like the following acceptable? Thanks. Leo

No, don't use `message': this is a real error, use `signal' or `error'.


        Stefan

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

From: Leo <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: John Wiegley <jwiegley <at> gmail.com>, 7585 <at> debbugs.gnu.org
Subject: Re: bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input
Date: Tue, 14 Dec 2010 06:13:00 +0000

On 2010-12-13 20:42 +0000, Stefan Monnier wrote:
>> Would something like the following acceptable? Thanks. Leo
>
> No, don't use `message': this is a real error, use `signal' or `error'.
>
>
>         Stefan

OK. Here is the whole thing with the change of `message' to `error'.

Without this patch, one usually gets an error like "Text is read-only"
when M-n/p. So the error is obscure. The patch makes the error more
useful.

2010-12-13  Leo <sdl.web <at> gmail.com>

	* eshell/em-hist.el (eshell-previous-matching-input): Check point
	is located after eshell prompt in case `eshell-hist-move-to-end'
	is nil.

 lisp/eshell/em-hist.el |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index 45fe050..47e0b2b 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -837,16 +837,18 @@ With prefix argument N, search for Nth previous match.
 If N is negative, find the next or Nth next match."
   (interactive (eshell-regexp-arg "Previous input matching (regexp): "))
   (setq arg (eshell-search-arg arg))
-  (let ((pos (eshell-previous-matching-input-string-position regexp arg)))
-    ;; Has a match been found?
-    (if (null pos)
-	(error "Not found")
-      (setq eshell-history-index pos)
-      (unless (minibuffer-window-active-p (selected-window))
-	(message "History item: %d" (- (ring-length eshell-history-ring) pos)))
-       ;; Can't use kill-region as it sets this-command
-      (delete-region eshell-last-output-end (point))
-      (insert-and-inherit (eshell-get-history pos)))))
+  (if (<= eshell-last-output-end (point))
+      (let ((pos (eshell-previous-matching-input-string-position regexp arg)))
+	;; Has a match been found?
+	(if (null pos)
+	    (error "Not found")
+	  (setq eshell-history-index pos)
+	  (unless (minibuffer-window-active-p (selected-window))
+	    (message "History item: %d" (- (ring-length eshell-history-ring) pos)))
+	  ;; Can't use kill-region as it sets this-command
+	  (delete-region eshell-last-output-end (point))
+	  (insert-and-inherit (eshell-get-history pos))))
+    (error "Point not located after prompt")))
 
 (defun eshell-next-matching-input (regexp arg)
   "Search forwards through input history for match for REGEXP.
-- 
1.7.3

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

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Leo <sdl.web <at> gmail.com>
Cc: John Wiegley <jwiegley <at> gmail.com>, 7585 <at> debbugs.gnu.org
Subject: Re: bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input
Date: Tue, 14 Dec 2010 14:46:38 -0500

> OK. Here is the whole thing with the change of `message' to `error'.

Looks good, please install it,


        Stefan

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

From: Chong Yidong <cyd <at> stupidchicken.com>
To: Leo <sdl.web <at> gmail.com>
Cc: John Wiegley <jwiegley <at> gmail.com>, 7585 <at> debbugs.gnu.org,
	Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input
Date: Fri, 17 Dec 2010 19:04:39 +0800

Leo <sdl.web <at> gmail.com> writes:

> OK. Here is the whole thing with the change of `message' to `error'.
>
> Without this patch, one usually gets an error like "Text is read-only"
> when M-n/p. So the error is obscure. The patch makes the error more
> useful.

Thanks; committed.

Previous Next

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