bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input

bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input

[Leo]

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)

bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input

[Stefan Monnier]

> 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

bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input

[Leo]

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@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

bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input

[Stefan Monnier]

> Would something like the following acceptable? Thanks. Leo

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


        Stefan

bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input

[Leo]

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@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

bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input

[Stefan Monnier]

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

Looks good, please install it,


        Stefan

bug#7585: 23.2.90; [PATCH] fix eshell-previous-matching-input

[Chong Yidong]

Leo <sdl.web@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.