Comint-like history search for Eshell?

Comint-like history search for Eshell?

[Pengji Zhang]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

Hallo!

I hacked a small patch (attached) implementing a comint-like interface
for history search in Eshell, and I would like to know what people think
about it. You may type 'M-r' in Eshell to try the interface.

I myself always miss the 'M-r' in 'M-x shell' whenever I use 'M-r' in
Eshell. I tried 'eshell-isearch-{forward,backward}', but IMO they are a
bit rough -- they are implemented by inserting the whole history file
into the buffer and overriding Isearch keys.

This patch is mostly copied from comint.el (or simple.el), and it is
basically a proof of concept. Before I submit it to the bug tracker, I
would like to gather some input regarding the change. Thanks in advance!

Pengji

PS I am not subscribed to this list. Please CC me when replying. Thanks!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: eshell-hist-isearch.diff --]
[-- Type: text/x-patch, Size: 10380 bytes --]

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index fffd611c06f..df95706cbe0 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -195,6 +195,10 @@ eshell-hist-rebind-keys-alist
 			       (repeat :inline t sexp))
 		       (function :tag "Command"))))
 
+(defcustom eshell-hist-isearch nil
+  "Non-nil to Isearch in input history only."
+  :type 'boolean)
+
 ;;; Internal Variables:
 
 (defvar eshell-history-ring nil)
@@ -205,24 +209,12 @@ eshell-hist--new-items
   "The number of new history items that have not been written to
 file.  This variable is local in each eshell buffer.")
 
-(defvar-keymap eshell-isearch-map
-  :doc "Keymap used in isearch in Eshell."
-  :parent isearch-mode-map
-  "C-m"         #'eshell-isearch-return
-  "C-r"         #'eshell-isearch-repeat-backward
-  "C-s"         #'eshell-isearch-repeat-forward
-  "C-g"         #'eshell-isearch-abort
-  "<backspace>" #'eshell-isearch-delete-char
-  "<delete>"    #'eshell-isearch-delete-char
-  "C-c C-c"     #'eshell-isearch-cancel)
-
 (defvar-keymap eshell-hist-mode-map
   "<up>"     #'eshell-previous-matching-input-from-input
   "<down>"   #'eshell-next-matching-input-from-input
   "C-<up>"   #'eshell-previous-input
   "C-<down>" #'eshell-next-input
-  "M-r"      #'eshell-previous-matching-input
-  "M-s"      #'eshell-next-matching-input
+  "M-r"      #'eshell-hist-isearch-backward-regexp
   "C-c M-r"  #'eshell-previous-matching-input-from-input
   "C-c M-s"  #'eshell-next-matching-input-from-input
   "C-c C-l"  #'eshell-list-history
@@ -258,23 +250,12 @@ eshell-hist-initialize
 	      #'eshell-complete-history-reference nil t))
 
   (if (and (eshell-using-module 'eshell-rebind)
-	   (not eshell-non-interactive-p))
+           (not eshell-non-interactive-p))
       (let ((rebind-alist eshell-rebind-keys-alist))
         (setq-local eshell-rebind-keys-alist
-	      (append rebind-alist eshell-hist-rebind-keys-alist))
-        (setq-local search-invisible t)
-        (setq-local search-exit-option t)
-	(add-hook 'isearch-mode-hook
-                  (lambda ()
-                    (if (>= (point) eshell-last-output-end)
-                        (setq overriding-terminal-local-map
-                              eshell-isearch-map)))
-                  nil t)
-	(add-hook 'isearch-mode-end-hook
-                  (lambda ()
-                    (setq overriding-terminal-local-map nil))
-                  nil t))
+                    (append rebind-alist eshell-hist-rebind-keys-alist)))
     (eshell-hist-mode))
+  (add-hook 'isearch-mode-hook #'eshell-hist--isearch-setup nil t)
 
   (make-local-variable 'eshell-history-size)
   (or eshell-history-size
@@ -901,7 +882,7 @@ eshell-previous-matching-input
       (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))
+      (delete-region eshell-last-output-end (point-max))
       (insert-and-inherit (eshell-get-history pos)))))
 
 (defun eshell-next-matching-input (regexp arg)
@@ -981,70 +962,121 @@ eshell-return-to-prompt
 	  (set-marker eshell-last-output-end orig)
 	  (goto-char eshell-last-output-end))))))
 
-(defun eshell-prepare-for-search ()
-  "Make sure the old history file is at the beginning of the buffer."
-  (unless (get-text-property (point-min) 'history)
-    (save-excursion
-      (goto-char (point-min))
-      (let ((end (copy-marker (point) t)))
-	(insert-file-contents eshell-history-file-name)
-	(set-text-properties (point-min) end
-			     '(history t invisible t))))))
-
-(defun eshell-isearch-backward (&optional invert)
-  "Do incremental regexp search backward through past commands."
-  (interactive)
-  (let ((inhibit-read-only t))
-    (eshell-prepare-for-search)
-    (goto-char (point-max))
-    (set-marker eshell-last-output-end (point))
-    (delete-region (point) (point-max)))
-  (isearch-mode invert t 'eshell-return-to-prompt))
-
-(defun eshell-isearch-repeat-backward (&optional invert)
-  "Do incremental regexp search backward through past commands."
-  (interactive)
-  (let ((old-pos (get-text-property (1- (point-max))
-				    'last-search-pos)))
-    (when old-pos
-      (goto-char old-pos)
-      (if invert
-	  (end-of-line)
-	(backward-char)))
-    (setq isearch-forward invert)
-    (isearch-search-and-update)))
+(defvar-local eshell-hist--isearch-message-overlay nil)
+
+(defun eshell-hist--isearch-setup ()
+  (when (and (>= (point) eshell-last-output-end)
+             eshell-hist-isearch)
+    (setq isearch-message-prefix-add "history ")
+    (setq-local isearch-lazy-count nil
+                isearch-search-fun-function #'eshell-hist--isearch-search
+                isearch-message-function #'eshell-hist--isearch-message
+                isearch-wrap-function #'eshell-hist--isearch-wrap
+                isearch-push-state-function #'eshell-hist--isearch-push-state)
+    (add-hook 'isearch-mode-end-hook #'eshell-hist--isearch-end nil t)))
+
+(defun eshell-hist--isearch-end ()
+  (when (overlayp eshell-hist--isearch-message-overlay)
+    (delete-overlay eshell-hist--isearch-message-overlay))
+  (setq isearch-message-prefix-add nil)
+  (setq-local isearch-search-fun-function #'isearch-search-fun-default
+              isearch-message-function nil
+              isearch-wrap-function nil
+              isearch-push-state-function nil)
+  (setq isearch-opoint (point))
+  (kill-local-variable 'isearch-lazy-count)
+  (remove-hook 'isearch-mode-end-hook #'eshell-hist--isearch-end t)
+  (unless isearch-suspended
+    (custom-reevaluate-setting 'eshell-hist-isearch)))
+
+(defun eshell-hist--isearch-search ()
+  (lambda (string bound noerror)
+    (let ((search-fun (isearch-search-fun-default))
+          found)
+      (when (and bound isearch-forward (< (point) eshell-last-output-end))
+        (goto-char eshell-last-output-end))
+      (or
+       (funcall search-fun string
+                (if isearch-forward bound (line-beginning-position))
+                noerror)
+       (unless bound
+         (condition-case nil
+             (progn
+               (while (not found)
+                 (cond (isearch-forward
+                        (when (or (null eshell-history-index)
+                                  (eq eshell-history-index 0))
+                          (error "End of history; no next item"))
+                        (eshell-next-input 1)
+                        (goto-char (line-beginning-position)))
+                       (t
+                        (when (eq eshell-history-index
+                                  (1- (ring-length eshell-history-ring)))
+                          (error "Beginning of history; no preceding item"))
+                        (eshell-previous-input 1)
+                        (goto-char (point-max))))
+                 (setq isearch-barrier (point) isearch-opoint (point))
+                 (setq found (funcall search-fun string
+                                      (unless isearch-forward
+                                        (line-beginning-position))
+                                      noerror)))
+               (point))
+           (error nil)))))))
+
+(defun eshell-hist--isearch-message (&optional c-q-hack ellipsis)
+  (if (not (and isearch-success (not isearch-error)))
+      (isearch-message c-q-hack ellipsis)
+    (if (overlayp eshell-hist--isearch-message-overlay)
+        (move-overlay eshell-hist--isearch-message-overlay
+                      (save-excursion
+                        (goto-char (line-beginning-position))
+                        (forward-line 0)
+                        (point))
+                      (line-beginning-position))
+      (setq eshell-hist--isearch-message-overlay
+            (make-overlay (save-excursion
+                            (goto-char (line-beginning-position))
+                            (forward-line 0)
+                            (point))
+                          (line-beginning-position)))
+      (overlay-put eshell-hist--isearch-message-overlay 'evaporate t))
+    (overlay-put eshell-hist--isearch-message-overlay
+                 'display (isearch-message-prefix ellipsis isearch-nonincremental))))
+
+(defun eshell-hist--isearch-wrap ()
+  (let ((pos (if isearch-forward
+                 (1- (ring-length eshell-history-ring))
+               0)))
+    (setq eshell-history-index pos)
+    (delete-region eshell-last-output-end (point-max))
+    (insert-and-inherit (eshell-get-history pos))
+    (goto-char (if isearch-forward (line-beginning-position) (point-max)))))
+
+(defun eshell-hist--isearch-push-state ()
+  (let ((index eshell-history-index))
+    (lambda (_)
+      (delete-region eshell-last-output-end (point-max))
+      (and index (insert-and-inherit (eshell-get-history index))))))
+
+(defun eshell-hist-isearch-backward-regexp ()
+  (interactive nil eshell-mode)
+  (setq eshell-hist-isearch t)
+  (isearch-backward-regexp nil t))
+
+(defun eshell-hist-isearch-forward-regexp ()
+  (interactive nil eshell-mode)
+  (setq eshell-hist-isearch t)
+  (isearch-forward-regexp nil t))
+
+(defun eshell-isearch-backward ()
+  (interactive nil eshell-mode)
+  (setq eshell-hist-isearch t)
+  (isearch-backward nil t))
 
 (defun eshell-isearch-forward ()
-  "Do incremental regexp search backward through past commands."
-  (interactive)
-  (eshell-isearch-backward t))
-
-(defun eshell-isearch-repeat-forward ()
-  "Do incremental regexp search backward through past commands."
-  (interactive)
-  (eshell-isearch-repeat-backward t))
-
-(defun eshell-isearch-cancel ()
-  (interactive)
-  (goto-char eshell-last-output-end)
-  (delete-region (point) (point-max))
-  (call-interactively 'isearch-cancel))
-
-(defun eshell-isearch-abort ()
-  (interactive)
-  (goto-char eshell-last-output-end)
-  (delete-region (point) (point-max))
-  (call-interactively 'isearch-abort))
-
-(defun eshell-isearch-delete-char ()
-  (interactive)
-  (save-excursion
-  (isearch-delete-char)))
-
-(defun eshell-isearch-return ()
-  (interactive)
-  (isearch-done)
-  (eshell-send-input))
+  (interactive nil eshell-mode)
+  (setq eshell-hist-isearch t)
+  (isearch-forward nil t))
 
 (defun em-hist-unload-function ()
   (remove-hook 'kill-emacs-hook 'eshell-save-some-history))

Re: Comint-like history search for Eshell?

[James Thomas]

Pengji Zhang wrote:

> Hallo!
>
> I hacked a small patch (attached) implementing a comint-like interface
> for history search in Eshell, and I would like to know what people think
> about it. You may type 'M-r' in Eshell to try the interface.
>
> I myself always miss the 'M-r' in 'M-x shell' whenever I use 'M-r' in
> Eshell. I tried 'eshell-isearch-{forward,backward}', but IMO they are a
> bit rough -- they are implemented by inserting the whole history file
> into the buffer and overriding Isearch keys.
>
> This patch is mostly copied from comint.el (or simple.el), and it is
> basically a proof of concept. Before I submit it to the bug tracker, I
> would like to gather some input regarding the change. Thanks in advance!

Thanks! I don't intend to read the code, but I've applied it and started
testing. Haven't run into any issues for the past 5 minutes. :-)

Regards,
James

Re: Comint-like history search for Eshell?

[Jim Porter]

On 11/3/2024 5:23 AM, Pengji Zhang wrote:
> I hacked a small patch (attached) implementing a comint-like interface
> for history search in Eshell, and I would like to know what people think
> about it. You may type 'M-r' in Eshell to try the interface.

Thanks for the patch!

Just some general thoughts before I do a more-thorough review: as I see 
it, there are two competing ideas for how a user might look up a prior 
command: using Isearch (as in this patch) or using completion. Both have 
some similarities, but the latter has the advantage of letting you use 
the various completion frameworks for filtering/displaying the results.

I don't know if one or the other is "better" overall; I imagine which 
one a user would prefer just depends on individual preference. (That 
said, I think I'd probably use a completion-based command history if I 
had the option.)

For this patch in particular, I think it would probably make sense to at 
least see about leaving the door open for a completion-based history 
lookup in Eshell. I haven't thought very hard about what that would look 
like yet though...

Re: Comint-like history search for Eshell?

[Sean Whitton]

Hello,

On Sun 03 Nov 2024 at 03:09pm -08, Jim Porter wrote:

> On 11/3/2024 5:23 AM, Pengji Zhang wrote:
>> I hacked a small patch (attached) implementing a comint-like interface
>> for history search in Eshell, and I would like to know what people think
>> about it. You may type 'M-r' in Eshell to try the interface.
>
> Thanks for the patch!
>
> Just some general thoughts before I do a more-thorough review: as I see it,
> there are two competing ideas for how a user might look up a prior command:
> using Isearch (as in this patch) or using completion. Both have some
> similarities, but the latter has the advantage of letting you use the various
> completion frameworks for filtering/displaying the results.
>
> I don't know if one or the other is "better" overall; I imagine which one a
> user would prefer just depends on individual preference. (That said, I think
> I'd probably use a completion-based command history if I had the option.)
>
> For this patch in particular, I think it would probably make sense to at least
> see about leaving the door open for a completion-based history lookup in
> Eshell. I haven't thought very hard about what that would look like yet
> though...

I came up with one a few months ago in my init.el:
<https://git.spwhitton.name/dotfiles/tree/.emacs.d/init.el?id=eadfcea3abbad3f1c0a866e7061e288ecb580d09#n1653>

-- 
Sean Whitton

Re: Comint-like history search for Eshell?

[Juri Linkov]

> Just some general thoughts before I do a more-thorough review: as I see it,
> there are two competing ideas for how a user might look up a prior command:
> using Isearch (as in this patch) or using completion. Both have some
> similarities, but the latter has the advantage of letting you use the
> various completion frameworks for filtering/displaying the results.
>
> I don't know if one or the other is "better" overall; I imagine which one
> a user would prefer just depends on individual preference. (That said,
> I think I'd probably use a completion-based command history if I had the
> option.)
>
> For this patch in particular, I think it would probably make sense to at
> least see about leaving the door open for a completion-based history lookup
> in Eshell. I haven't thought very hard about what that would look like yet
> though...

The standard features that correspond to these two ways are
incremental search with 'C-s' (implemented in the proposed patch)
and occur with 'M-s o' (there is no occur-like interface for Eshell
history AFAIK, it exists only for the minibuffer history, and
in shell with 'comint-dynamic-list-input-ring').  However, having
a completion UI would be nice too.  For example, for shell I use:

```
(defun comint-complete-input-ring ()
  "Complete the input history as far as possible.
Like `minibuffer-complete-history' but completes on the comint history items."
  (interactive)
  (let* ((completions
          (if (and (ring-p comint-input-ring)
                   (not (ring-empty-p comint-input-ring)))
              (ring-elements comint-input-ring)
            (user-error "No history available")))
         (capf (lambda ()
                 (list (save-excursion (move-beginning-of-line 1) (point))
                       (point-max)
                       (lambda (string pred action)
                         (if (eq action 'metadata)
                             '(metadata (display-sort-function . identity)
                                        (cycle-sort-function . identity))
                           (complete-with-action action completions string pred))))))
         (completion-at-point-functions (list capf)))
    (completion-at-point)))
(define-key comint-mode-map [?\C-x up] 'comint-complete-input-ring)
```

So Eshell could have both as well: incremental search and completion.

Re: Comint-like history search for Eshell?

[Pengji Zhang]

Juri Linkov <juri@linkov.net> writes:

> The standard features that correspond to these two ways are
> incremental search with 'C-s' (implemented in the proposed patch)
> and occur with 'M-s o' (there is no occur-like interface for Eshell
> history AFAIK, it exists only for the minibuffer history, and
> in shell with 'comint-dynamic-list-input-ring').

In Eshell there is 'eshell-list-history', but it filters the history
using the bang expansion syntax, which is disabled by default now. In
Eshell, you may type 'C-c C-l' after

    $ !emacs

to get a buffer showing all history commands that start with 'emacs'. I
do not use bang expansion so I find it rarely useful.

> [...]
>
> So Eshell could have both as well: incremental search and completion.

I agree that we could implement both. For completion, if we are going to
let-bind 'completion-at-point-functions' as in Juri's and Sean's
commands (or use 'completion-in-region' as we do for the minibuffer),
then IMO there is nothing that hinders the implementation of an Isearch
interface.

Pengji

Re: Comint-like history search for Eshell?

[Juri Linkov]

>> So Eshell could have both as well: incremental search and completion.
>
> I agree that we could implement both. For completion, if we are going to
> let-bind 'completion-at-point-functions' as in Juri's and Sean's
> commands (or use 'completion-in-region' as we do for the minibuffer),
> then IMO there is nothing that hinders the implementation of an Isearch
> interface.

Ideally, a new key to invoke completion on history items
(e.g. the same key 'C-x up' like in the minibuffer)
should work like 'M-C-i' (in-buffer completion).

Re: Comint-like history search for Eshell?

[James Thomas]

Pengji Zhang wrote:

> I hacked a small patch (attached) implementing a comint-like interface
> for history search in Eshell, and I would like to know what people think
> about it. You may type 'M-r' in Eshell to try the interface.
>
> I myself always miss the 'M-r' in 'M-x shell' whenever I use 'M-r' in
> Eshell. I tried 'eshell-isearch-{forward,backward}', but IMO they are a
> bit rough -- they are implemented by inserting the whole history file
> into the buffer and overriding Isearch keys.

One thing is that those used to the effectively infinite
eshell-history-size previously, may be surprised. Like I was.

Regards,
James

Re: Comint-like history search for Eshell?

[James Thomas]

Pengji Zhang wrote:

> I hacked a small patch (attached) implementing a comint-like interface
> for history search in Eshell, and I would like to know what people think
> about it. You may type 'M-r' in Eshell to try the interface.

(Posting here as there seems to be no bug report as yet)

A small thing: With your patch, if you cancel after an M-r search (with
C-g) then the position in the history ring is left at the command that
was displayed, and not to the one at the start. Is this intentional?

--

Re: Comint-like history search for Eshell?

[Pengji Zhang]

James Thomas <jimjoe@gmx.net> writes:

> (Posting here as there seems to be no bug report as yet)

I am working on a new patch, and I will file a bug report later.

> A small thing: With your patch, if you cancel after an M-r search
> (with C-g) then the position in the history ring is left at the
> command that was displayed, and not to the one at the start. Is this
> intentional?

No, it is not. Thanks for reporting this! I have located the cause of
it, and I will fix it in the new patch.

Pengji

Re: Comint-like history search for Eshell?

[Pengji Zhang]

James Thomas <jimjoe@gmx.net> writes:

> (Posting here as there seems to be no bug report as yet)

I am working on a new patch, and I will file a bug report later.

> A small thing: With your patch, if you cancel after an M-r search
> (with C-g) then the position in the history ring is left at the
> command that was displayed, and not to the one at the start. Is this
> intentional?

No, it is not. Thanks for reporting this! I have located the cause of
it, and I will fix it in the new patch.

Pengji