bug#74287: [PATCH] Rework history Isearch for Eshell

bug#74287: [PATCH] Rework history Isearch for Eshell

[Pengji Zhang]

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

Hello,

This patch brings a comint-like interface for history Isearch to Eshell.
To try it, type 'M-r' in Eshell, and search through the input history
ring incrementally.

Compared to the existing implementation, this patch integrates with
Isearch properly, like what we do for the minibuffer and comint modes.

There are relevant discussions (thank you all for the feedback!) in the
mailing list:

    https://lists.gnu.org/archive/html/emacs-devel/2024-11/msg00069.html

For Jim's concern, I do not think this implementation closes the door
for a history completion interface, as shown by Sean and Juri.

Thanks!

Pengji


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Rework-history-Isearch-for-Eshell.patch --]
[-- Type: text/x-patch, Size: 20739 bytes --]

From d455ec1bf1b49871cf866457df77f6ab45f5623f Mon Sep 17 00:00:00 2001
From: Pengji Zhang <me@pengjiz.com>
Date: Sun, 10 Nov 2024 08:50:27 +0800
Subject: [PATCH] Rework history Isearch for Eshell

This is to make history Isearch for Eshell similar to that of
'comint-mode', by hooking into Isearch properly instead of
defining new commands to emulate Isearch.

* lisp/eshell/em-hist.el (eshell-history-isearch): New user
option.
(eshell-goto-history, eshell--isearch-setup)
(eshell-history-isearch-end, eshell-history-isearch-search)
(eshell-history-isearch-message, eshell-history-isearch-wrap)
(eshell-history-isearch-push-state): New functions.
(eshell-isearch-backward-regexp, eshell-isearch-forward-regexp):
New commands.
(eshell--history-isearch-messasge-overlay)
(eshell--stored-incomplete-input): New internal variables.
(eshell-hist-mode-map): Bind 'M-r' to
'eshell-isearch-backward-regexp' and free 'M-s' binding for
normal in-buffer search commands.
(eshell-isearch-backward, eshell-isearch-forward): Use the new
way to start searching.
(eshell-hist-initialize): Use the new Isearch setup function.
(eshell-previous-matching-input): Use 'eshell-goto-history'.
Also inhibit messages when searching.
(eshell-isearch-map, eshell-isearch-repeat-backward)
(eshell-isearch-abort, eshell-isearch-delete-char)
(eshell-isearch-return, eshell-isearch-cancel)
(eshell-isearch-repeat-forward, eshell-test-imatch)
(eshell-return-to-prompt, eshell-prepare-for-search): Remove.
These are for the old history Isearch implementation.

* doc/misc/eshell.texi (History): Document changes.
* etc/NEWS: Annouce changes.
---
 doc/misc/eshell.texi   |  15 +-
 etc/NEWS               |  23 +++
 lisp/eshell/em-hist.el | 322 ++++++++++++++++++++++++-----------------
 3 files changed, 223 insertions(+), 137 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index ee4d0ca09c8..701137ea1b4 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -2661,10 +2661,10 @@ History
 
 @table @kbd
 @kindex M-r
-@kindex M-s
 @item M-r
-@itemx M-s
-History I-search.
+History I-search.  @kbd{M-r} starts an incremental search in input
+history.  While searching, type @kbd{C-r} to move to the previous match,
+and @kbd{C-s} to move to the next match in the input history.
 
 @kindex M-p
 @kindex M-n
@@ -2675,6 +2675,15 @@ History
 previous or next line that begins with that string.
 @end table
 
+@vindex eshell-history-isearch
+If you would like to use the default Isearch key-bindings to search
+through input history, you may customize @code{eshell-history-isearch}
+to @code{t}.  That makes, for example, @kbd{C-r} and @kbd{C-M-r} in an
+Eshell buffer search in input history only.  In addition, if the value
+of @code{eshell-history-isearch} is @code{dwim}, those commands search
+in the history when the point is after the last prompt, and search in
+the buffer when the point is before or within the last prompt.
+
 @node Extension modules
 @chapter Extension modules
 Eshell provides a facility for defining extension modules so that they
diff --git a/etc/NEWS b/etc/NEWS
index d1c7303f976..c1df6f78d9f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -288,6 +288,29 @@ This hook runs after an Eshell session has been fully initialized,
 immediately before running 'eshell-post-command-hook' for the first
 time.
 
++++
+*** Improved history Isearch.
+History Isearch in Eshell is reworked.  Two new commands
+'eshell-isearch-backward-regexp' and 'eshell-isearch-forward-regexp' are
+added for incrementally searching through the input history.
+'eshell-isearch-backward-regexp' is bound to 'M-r' by default, and 'M-s'
+is freed for normal search commands.  If you would like to restore the
+previous key-bindings for the non-incremental search commands, put in
+your configuration:
+
+    (keymap-set eshell-hist-mode-map "M-r"
+                #'eshell-previous-matching-input)
+    (keymap-set eshell-hist-mode-map "M-s"
+                #'eshell-next-matching-input)
+
++++
+*** New user option 'eshell-history-isearch'
+When 'eshell-history-isearch' is nil (the default), Isearch commands
+search in the buffer contents.  If you customize it to t, those commands
+only search in input history.  If you customize it to the symbol 'dwim',
+those commands search in input history only when the point is after the
+last prompt.
+
 ** SHR
 
 +++
diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index fffd611c06f..de986b05f48 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -34,7 +34,6 @@
 ;; Also, most of `comint-mode's keybindings are accepted:
 ;;
 ;;   M-r     ; search backward for a previous command by regexp
-;;   M-s     ; search forward for a previous command by regexp
 ;;   M-p     ; access the last command entered, repeatable
 ;;   M-n     ; access the first command entered, repeatable
 ;;
@@ -132,6 +131,17 @@ eshell-input-filter
                 (function :tag "Other function"))
   :risky t)
 
+(defcustom eshell-history-isearch nil
+  "Non-nil to Isearch in input history only.
+If t, usual Isearch keys like \\[isearch-forward] in Eshell search in
+the input history only.  If `dwim', Isearch in the input history when
+point is at the command line, otherwise search in the current Eshell
+buffer."
+  :type '(choice (const :tag "Don't search in input history" nil)
+                 (const :tag "Search histroy when point is on command line" dwim)
+                 (const :tag "Always search in input history" t))
+  :version "31.1")
+
 (defun eshell-hist--update-keymap (symbol value)
   "Update `eshell-hist-mode-map' for `eshell-hist-match-partial'."
   ;; Don't try to set this before it is bound.  See below.
@@ -204,25 +214,17 @@ eshell-save-history-index
 (defvar eshell-hist--new-items nil
   "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-local eshell--history-isearch-messasge-overlay nil
+  "Overlay for Isearch message when searching through input history.")
+(defvar-local eshell--stored-incomplete-input nil
+  "Stored input for history cycling.")
 
 (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-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
@@ -261,20 +263,9 @@ eshell-hist-initialize
 	   (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--isearch-setup nil t)
 
   (make-local-variable 'eshell-history-size)
   (or eshell-history-size
@@ -384,6 +375,23 @@ eshell-get-history
   "Get an input line from the history ring."
   (ring-ref (or ring eshell-history-ring) index))
 
+(defun eshell-goto-history (pos)
+  "Replace command line with the element at POS of history ring.
+Also update `eshell-history-index'.  As a special case, if POS is nil
+and `eshell--stored-incomplete-input' is a non-empty string, restore the
+saved input."
+  (when (null eshell-history-index)
+    (setq eshell--stored-incomplete-input
+          (buffer-substring-no-properties eshell-last-output-end
+                                          (point-max))))
+  (setq eshell-history-index pos)
+  ;; Can't use kill-region as it sets this-command
+  (delete-region eshell-last-output-end (point-max))
+  (if (and pos (not (ring-empty-p eshell-history-ring)))
+      (insert-and-inherit (eshell-get-history pos))
+    (when (> (length eshell--stored-incomplete-input) 0)
+      (insert-and-inherit eshell--stored-incomplete-input))))
+
 (defun eshell-add-input-to-history (input)
   "Add the string INPUT to the history ring.
 Input is entered into the input history ring, if the value of
@@ -897,12 +905,12 @@ eshell-previous-matching-input
     ;; 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)))))
+      (eshell-goto-history pos)
+      (unless (or (minibuffer-window-active-p (selected-window))
+                  ;; No messages for Isearch because it will show the
+                  ;; same messages (and more).
+                  isearch-mode)
+	(message "History item: %d" (- (ring-length eshell-history-ring) pos))))))
 
 (defun eshell-next-matching-input (regexp arg)
   "Search forwards through input history for match for REGEXP.
@@ -937,114 +945,160 @@ eshell-next-matching-input-from-input
   (interactive "p")
   (eshell-previous-matching-input-from-input (- arg)))
 
-(defun eshell-test-imatch ()
-  "If isearch match good, put point at the beginning and return non-nil."
-  (if (get-text-property (point) 'history)
-      (progn (beginning-of-line) t)
-    (let ((before (point)))
-      (beginning-of-line)
-      (if (and (not (bolp))
-	       (<= (point) before))
-	  t
-	(if isearch-forward
-	    (progn
-	      (end-of-line)
-	      (forward-char))
-	  (beginning-of-line)
-	  (backward-char))))))
-
-(defun eshell-return-to-prompt ()
-  "Once a search string matches, insert it at the end and go there."
-  (setq isearch-other-end nil)
-  (let ((found (eshell-test-imatch)) before)
-    (while (and (not found)
-		(setq before
-		      (funcall (if isearch-forward
-				   're-search-forward
-				 're-search-backward)
-			       isearch-string nil t)))
-      (setq found (eshell-test-imatch)))
-    (if (not found)
-	(progn
-	  (goto-char eshell-last-output-end)
-	  (delete-region (point) (point-max)))
-      (setq before (point))
-      (let ((text (buffer-substring-no-properties
-		   (point) (line-end-position)))
-	    (orig (marker-position eshell-last-output-end)))
-	(goto-char eshell-last-output-end)
-	(delete-region (point) (point-max))
-	(when (and text (> (length text) 0))
-	  (insert text)
-	  (put-text-property (1- (point)) (point)
-			     'last-search-pos before)
-	  (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-setup ()
+  "Set up Isearch to search the input history.
+Intended to be added to `isearch-mode-hook' in an Eshell buffer."
+  (when (and
+         ;; Eshell is busy running a foreground process
+         (not eshell-foreground-command)
+         (or (eq eshell-history-isearch t)
+             (and (eq eshell-history-isearch 'dwim)
+                  (>= (point) eshell-last-output-end))))
+    (setq isearch-message-prefix-add "history ")
+    (setq-local isearch-lazy-count nil)
+    (setq-local isearch-search-fun-function #'eshell-history-isearch-search
+                isearch-message-function #'eshell-history-isearch-message
+                isearch-wrap-function #'eshell-history-isearch-wrap
+                isearch-push-state-function #'eshell-history-isearch-push-state)
+    (add-hook 'isearch-mode-end-hook #'eshell-history-isearch-end nil t)))
+
+(defun eshell-history-isearch-end ()
+  "Clean up after terminating history Isearch."
+  (when (overlayp eshell--history-isearch-messasge-overlay)
+    (delete-overlay eshell--history-isearch-messasge-overlay))
+  (setq isearch-message-prefix-add nil)
+  (kill-local-variable 'isearch-lazy-count)
+  (setq-local isearch-search-fun-function #'isearch-search-fun-default
+              isearch-message-function nil
+              isearch-wrap-function nil
+              isearch-push-state-function nil)
+  (remove-hook 'isearch-mode-end-hook #'eshell-history-isearch-end t)
+  (setq isearch-opoint (point))
+  (unless isearch-suspended
+    (custom-reevaluate-setting 'eshell-history-isearch)))
+
+(defun eshell-history-isearch-search ()
+  "Return search function for Isearch in input history."
+  (lambda (string bound noerror)
+    (let ((search-fun (isearch-search-fun-default))
+          (found nil))
+      ;; Avoid highlighting matches in and before the last prompt
+      (when (and bound isearch-forward
+                 (< (point) eshell-last-output-end))
+        (goto-char eshell-last-output-end))
+      (or
+       ;; First search in the initial input
+       (funcall search-fun string
+                (if isearch-forward bound eshell-last-output-end)
+                noerror)
+       ;; Then search in the input history: put next/previous history
+       ;; element in the command line successively, then search the
+       ;; string in the command line.  Do this only when not
+       ;; lazy-highlighting (`bound' is nil).
+       (unless bound
+         (condition-case nil
+             (progn
+               (while (not found)
+                 (cond (isearch-forward
+                        ;; Signal an error explicitly to break
+                        (when (or (null eshell-history-index)
+                                  (eq eshell-history-index 0))
+                          (error "End of history; no next item"))
+                        (eshell-next-input 1)
+                        (goto-char eshell-last-output-end))
+                       (t
+                        ;; Signal an error explicitly to break
+                        (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))
+                 ;; After putting an history element in the command
+                 ;; line, search the string in them.
+                 (setq found (funcall search-fun string
+                                      (unless isearch-forward
+                                        eshell-last-output-end)
+                                      noerror)))
+               (point))
+           ;; Return when no next/preceding element error signaled
+           (error nil)))))))
+
+(defun eshell-history-isearch-message (&optional c-q-hack ellipsis)
+  "Display the input history search prompt.
+If there are no search errors, this function displays an overlay with
+the Isearch prompt which replaces the original Eshell prompt.
+Otherwise, it displays the standard Isearch message returned from the
+function `isearch-message'."
+  (if (not (and isearch-success (not isearch-error)))
+      ;; Use standard message function (which displays a message in the
+      ;; echo area) when not in command line, or search fails or has
+      ;; errors (like incomplete regexp).
+      (isearch-message c-q-hack ellipsis)
+    ;; Otherwise, use an overlay over the Eshell prompt.
+    (if (overlayp eshell--history-isearch-messasge-overlay)
+        (move-overlay eshell--history-isearch-messasge-overlay
+                      (save-excursion
+                        (goto-char eshell-last-output-end)
+                        (forward-line 0)
+                        (point))
+                      eshell-last-output-end)
+      (setq eshell--history-isearch-messasge-overlay
+            (make-overlay (save-excursion
+                            (goto-char eshell-last-output-end)
+                            (forward-line 0)
+                            (point))
+                          eshell-last-output-end))
+      (overlay-put eshell--history-isearch-messasge-overlay 'evaporate t))
+    (overlay-put eshell--history-isearch-messasge-overlay
+                 'display (isearch-message-prefix ellipsis
+                                                  isearch-nonincremental))
+    (if (and eshell-history-index (not ellipsis))
+        (message "History item: %d" (- (ring-length eshell-history-ring)
+                                       eshell-history-index))
+      (message ""))))
+
+(defun eshell-history-isearch-wrap ()
+  "Wrap the input history search."
+  (if isearch-forward
+      (eshell-goto-history (1- (ring-length eshell-history-ring)))
+    (eshell-goto-history nil))
+  (goto-char (if isearch-forward eshell-last-output-end (point-max))))
+
+(defun eshell-history-isearch-push-state ()
+  "Save a function restoring the state of input history search.
+Save `eshell-history-index' to the additional state parameter in the
+search status stack."
+  (let ((index eshell-history-index))
+    (lambda (_cmd)
+      (eshell-goto-history index))))
 
 (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)))
+  "Do incremental search backward through past commands."
+  (interactive nil eshell-mode)
+  (setq eshell-history-isearch t)
+  (if invert
+      (isearch-forward nil t)
+    (isearch-backward nil t)))
 
 (defun eshell-isearch-forward ()
-  "Do incremental regexp search backward through past commands."
-  (interactive)
+  "Do incremental search forward through past commands."
+  (interactive nil eshell-mode)
   (eshell-isearch-backward t))
 
-(defun eshell-isearch-repeat-forward ()
+(defun eshell-isearch-backward-regexp (&optional invert)
   "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-history-isearch t)
+  (if invert
+      (isearch-forward-regexp nil t)
+    (isearch-backward-regexp nil t)))
+
+(defun eshell-isearch-forward-regexp ()
+  "Do incremental regexp search forward through past commands."
+  (interactive nil eshell-mode)
+  (eshell-isearch-backward-regexp t))
 
 (defun em-hist-unload-function ()
   (remove-hook 'kill-emacs-hook 'eshell-save-some-history))
-- 
2.47.0

bug#74287: [PATCH] Rework history Isearch for Eshell

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pengji Zhang wrote:

> Hello,
>
> This patch brings a comint-like interface for history Isearch to Eshell.
> To try it, type 'M-r' in Eshell, and search through the input history
> ring incrementally.
>
> Compared to the existing implementation, this patch integrates with
> Isearch properly, like what we do for the minibuffer and comint modes.
>
> There are relevant discussions (thank you all for the feedback!) in the
> mailing list:
>
>     https://lists.gnu.org/archive/html/emacs-devel/2024-11/msg00069.html
>
> For Jim's concern, I do not think this implementation closes the door
> for a history completion interface, as shown by Sean and Juri.

Just wanna add a POC to the alternatives suggested. WDYT? Does not use
regexps for now, and C-. and C-, for cycling (rather than C-r and C-s):

(setq icomplete-in-buffer t)
(icomplete-mode 1)

(defun my/eshell-history-complete nil
  (interactive)
  (let ((icomplete-compute-delay 0)
	(completion-at-point-functions
	 '((lambda nil
	     `(,eshell-last-output-end ,(point) ,(append (cddr eshell-history-ring) nil) . nil)))))
    (completion-at-point)))

(define-key eshell-hist-mode-map (kbd "M-r") #'my/eshell-history-complete)

--

bug#74287: [PATCH] Rework history Isearch for Eshell

[Pengji Zhang]

James Thomas <jimjoe@gmx.net> writes:

> Just wanna add a POC to the alternatives suggested. WDYT? Does not use
> regexps for now, and C-. and C-, for cycling (rather than C-r and C-s):
>
> (setq icomplete-in-buffer t)
> (icomplete-mode 1)
>
> (defun my/eshell-history-complete nil
>   (interactive)
>   (let ((icomplete-compute-delay 0)
> 	(completion-at-point-functions
> 	 '((lambda nil
> 	     `(,eshell-last-output-end ,(point) ,(append (cddr eshell-history-ring) nil) . nil)))))
>     (completion-at-point)))
>
> (define-key eshell-hist-mode-map (kbd "M-r") #'my/eshell-history-complete)

Thanks! While I like the simplicity of your approach, I am not sure if
it is a good idea to depend on 'icomplete-mode'. People may have
different preferences for the in-buffer completion framework.

bug#74287: [PATCH] Rework history Isearch for Eshell

[Jim Porter]

On 11/9/2024 5:22 PM, Pengji Zhang wrote:
> Hello,
> 
> This patch brings a comint-like interface for history Isearch to Eshell.
> To try it, type 'M-r' in Eshell, and search through the input history
> ring incrementally.

 From inspection, this code all looks good to me. At least, it matches 
what Comint does; I don't know much about Isearch's implementation, so 
I'm just trusting that Comint is right here. Since this is a 
somewhat-large patch, I'm going to try and find some time this week to 
test it out so that I understand how it all works, and assuming I don't 
find any major problems, I'll merge it shortly after that.

Thanks for your contribution to Eshell (and Emacs as a whole)!

bug#74287: [PATCH] Rework history Isearch for Eshell

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jim Porter wrote:

> On 11/9/2024 5:22 PM, Pengji Zhang wrote:
>> Hello,
>> This patch brings a comint-like interface for history Isearch to
>> Eshell.
>> To try it, type 'M-r' in Eshell, and search through the input history
>> ring incrementally.
>
> From inspection, this code all looks good to me. At least, it matches
> what Comint does; I don't know much about Isearch's implementation, so
> I'm just trusting that Comint is right here. Since this is a
> somewhat-large patch, I'm going to try and find some time this week to
> test it out so that I understand how it all works, and assuming I
> don't find any major problems, I'll merge it shortly after that.

My 2c: I now think it's better to hold out for something with way less
code, using capf.

--

bug#74287: [PATCH] Rework history Isearch for Eshell

[Jim Porter]

On 11/26/2024 1:41 AM, James Thomas via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> My 2c: I now think it's better to hold out for something with way less
> code, using capf.

Like mentioned in the emacs-devel list, that's probably how *I'd* want a 
command history search implementation to use, but I also think that an 
Isearch interface makes a lot of sense as an option, depending on user 
preferences. As far as I can tell, we'd have to write different code for 
each of these styles (i.e. you can't just make Isearch use capf). Though 
as mentioned I'm not very familiar with Isearch's internals.

So long as this doesn't prevent a capf-based implementation (and I'm 
guessing it wouldn't, since that would just be a new option), I think we 
could merge this patch. I also prefer to have Shell and Eshell work the 
same way unless there's some compelling reason why they should be different.

bug#74287: [PATCH] Rework history Isearch for Eshell

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jim Porter wrote:

> On 11/26/2024 1:41 AM, James Thomas via Bug reports for GNU Emacs, the
> Swiss army knife of text editors wrote:
>> My 2c: I now think it's better to hold out for something with way less
>> code, using capf.
>
> Like mentioned in the emacs-devel list, that's probably how *I'd* want
> a command history search implementation to use, but I also think that
> an Isearch interface makes a lot of sense as an option, depending on
> user preferences. As far as I can tell, we'd have to write different
> code for each of these styles (i.e. you can't just make Isearch use
> capf). Though as mentioned I'm not very familiar with Isearch's
> internals.
>
> So long as this doesn't prevent a capf-based implementation (and I'm
> guessing it wouldn't, since that would just be a new option), I think
> we could merge this patch.

Okay. In that case, my preference would be for it to be a separate
module ala eshell-smart et. al., if possible; the motive being easing
the maintenance burden.

> I also prefer to have Shell and Eshell work the same way unless
> there's some compelling reason why they should be different.

Shell could change to be like eshell, for that, no? (like Wittgenstein's
bit with the ruler) ;-)

--

bug#74287: [PATCH] Rework history Isearch for Eshell

[Juri Linkov]

> Like mentioned in the emacs-devel list, that's probably how *I'd* want
> a command history search implementation to use, but I also think that an
> Isearch interface makes a lot of sense as an option, depending on user
> preferences. As far as I can tell, we'd have to write different code for
> each of these styles (i.e. you can't just make Isearch use capf). Though as
> mentioned I'm not very familiar with Isearch's internals.

I confirm that the patch uses the right Isearch's internals
to implement the search through Eshell history.

(There is only one typo in the variable name
'eshell--history-isearch-messasge-overlay'.)

> So long as this doesn't prevent a capf-based implementation (and I'm
> guessing it wouldn't, since that would just be a new option), I think we
> could merge this patch. I also prefer to have Shell and Eshell work the
> same way unless there's some compelling reason why they should be
> different.

Having Shell and Eshell work the same way is a nice goal.
There is a patch that implements completion for Shell in bug#74694.
Then Eshell could have something like this later as well.

bug#74287: [PATCH] Rework history Isearch for Eshell

[Pengji Zhang]

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

Juri Linkov <juri@linkov.net> writes:

> (There is only one typo in the variable name
> 'eshell--history-isearch-messasge-overlay'.)

Thanks! Corrected in the attached updated patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Updated patch --]
[-- Type: text/x-patch, Size: 20730 bytes --]

From 9df1d8eaf626c396b88fe616a6e89d5a8e7b963c Mon Sep 17 00:00:00 2001
From: Pengji Zhang <me@pengjiz.com>
Date: Thu, 5 Dec 2024 20:25:25 +0800
Subject: [PATCH] Rework history Isearch for Eshell

This is to make history Isearch for Eshell similar to that of
'comint-mode', by hooking into Isearch properly instead of
defining new commands to emulate Isearch.

* lisp/eshell/em-hist.el (eshell-history-isearch): New user
option.
(eshell-goto-history, eshell--isearch-setup)
(eshell-history-isearch-end, eshell-history-isearch-search)
(eshell-history-isearch-message, eshell-history-isearch-wrap)
(eshell-history-isearch-push-state): New functions.
(eshell-isearch-backward-regexp, eshell-isearch-forward-regexp):
New commands.
(eshell--history-isearch-messasge-overlay)
(eshell--stored-incomplete-input): New internal variables.
(eshell-hist-mode-map): Bind 'M-r' to
'eshell-isearch-backward-regexp' and free 'M-s' binding for
normal in-buffer search commands.
(eshell-isearch-backward, eshell-isearch-forward): Use the new
way to start searching.
(eshell-hist-initialize): Use the new Isearch setup function.
(eshell-previous-matching-input): Use 'eshell-goto-history'.
Also inhibit messages when searching.
(eshell-isearch-map, eshell-isearch-repeat-backward)
(eshell-isearch-abort, eshell-isearch-delete-char)
(eshell-isearch-return, eshell-isearch-cancel)
(eshell-isearch-repeat-forward, eshell-test-imatch)
(eshell-return-to-prompt, eshell-prepare-for-search): Remove.
These are for the old history Isearch implementation.

* doc/misc/eshell.texi (History): Document changes.
* etc/NEWS: Annouce changes.
---
 doc/misc/eshell.texi   |  15 +-
 etc/NEWS               |  23 +++
 lisp/eshell/em-hist.el | 322 ++++++++++++++++++++++++-----------------
 3 files changed, 223 insertions(+), 137 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index ee4d0ca09c8..701137ea1b4 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -2661,10 +2661,10 @@ History
 
 @table @kbd
 @kindex M-r
-@kindex M-s
 @item M-r
-@itemx M-s
-History I-search.
+History I-search.  @kbd{M-r} starts an incremental search in input
+history.  While searching, type @kbd{C-r} to move to the previous match,
+and @kbd{C-s} to move to the next match in the input history.
 
 @kindex M-p
 @kindex M-n
@@ -2675,6 +2675,15 @@ History
 previous or next line that begins with that string.
 @end table
 
+@vindex eshell-history-isearch
+If you would like to use the default Isearch key-bindings to search
+through input history, you may customize @code{eshell-history-isearch}
+to @code{t}.  That makes, for example, @kbd{C-r} and @kbd{C-M-r} in an
+Eshell buffer search in input history only.  In addition, if the value
+of @code{eshell-history-isearch} is @code{dwim}, those commands search
+in the history when the point is after the last prompt, and search in
+the buffer when the point is before or within the last prompt.
+
 @node Extension modules
 @chapter Extension modules
 Eshell provides a facility for defining extension modules so that they
diff --git a/etc/NEWS b/etc/NEWS
index d1c7303f976..c1df6f78d9f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -288,6 +288,29 @@ This hook runs after an Eshell session has been fully initialized,
 immediately before running 'eshell-post-command-hook' for the first
 time.
 
++++
+*** Improved history Isearch.
+History Isearch in Eshell is reworked.  Two new commands
+'eshell-isearch-backward-regexp' and 'eshell-isearch-forward-regexp' are
+added for incrementally searching through the input history.
+'eshell-isearch-backward-regexp' is bound to 'M-r' by default, and 'M-s'
+is freed for normal search commands.  If you would like to restore the
+previous key-bindings for the non-incremental search commands, put in
+your configuration:
+
+    (keymap-set eshell-hist-mode-map "M-r"
+                #'eshell-previous-matching-input)
+    (keymap-set eshell-hist-mode-map "M-s"
+                #'eshell-next-matching-input)
+
++++
+*** New user option 'eshell-history-isearch'
+When 'eshell-history-isearch' is nil (the default), Isearch commands
+search in the buffer contents.  If you customize it to t, those commands
+only search in input history.  If you customize it to the symbol 'dwim',
+those commands search in input history only when the point is after the
+last prompt.
+
 ** SHR
 
 +++
diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index fffd611c06f..c85e7704eca 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -34,7 +34,6 @@
 ;; Also, most of `comint-mode's keybindings are accepted:
 ;;
 ;;   M-r     ; search backward for a previous command by regexp
-;;   M-s     ; search forward for a previous command by regexp
 ;;   M-p     ; access the last command entered, repeatable
 ;;   M-n     ; access the first command entered, repeatable
 ;;
@@ -132,6 +131,17 @@ eshell-input-filter
                 (function :tag "Other function"))
   :risky t)
 
+(defcustom eshell-history-isearch nil
+  "Non-nil to Isearch in input history only.
+If t, usual Isearch keys like \\[isearch-forward] in Eshell search in
+the input history only.  If `dwim', Isearch in the input history when
+point is at the command line, otherwise search in the current Eshell
+buffer."
+  :type '(choice (const :tag "Don't search in input history" nil)
+                 (const :tag "Search histroy when point is on command line" dwim)
+                 (const :tag "Always search in input history" t))
+  :version "31.1")
+
 (defun eshell-hist--update-keymap (symbol value)
   "Update `eshell-hist-mode-map' for `eshell-hist-match-partial'."
   ;; Don't try to set this before it is bound.  See below.
@@ -204,25 +214,17 @@ eshell-save-history-index
 (defvar eshell-hist--new-items nil
   "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-local eshell--history-isearch-message-overlay nil
+  "Overlay for Isearch message when searching through input history.")
+(defvar-local eshell--stored-incomplete-input nil
+  "Stored input for history cycling.")
 
 (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-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
@@ -261,20 +263,9 @@ eshell-hist-initialize
 	   (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--isearch-setup nil t)
 
   (make-local-variable 'eshell-history-size)
   (or eshell-history-size
@@ -384,6 +375,23 @@ eshell-get-history
   "Get an input line from the history ring."
   (ring-ref (or ring eshell-history-ring) index))
 
+(defun eshell-goto-history (pos)
+  "Replace command line with the element at POS of history ring.
+Also update `eshell-history-index'.  As a special case, if POS is nil
+and `eshell--stored-incomplete-input' is a non-empty string, restore the
+saved input."
+  (when (null eshell-history-index)
+    (setq eshell--stored-incomplete-input
+          (buffer-substring-no-properties eshell-last-output-end
+                                          (point-max))))
+  (setq eshell-history-index pos)
+  ;; Can't use kill-region as it sets this-command
+  (delete-region eshell-last-output-end (point-max))
+  (if (and pos (not (ring-empty-p eshell-history-ring)))
+      (insert-and-inherit (eshell-get-history pos))
+    (when (> (length eshell--stored-incomplete-input) 0)
+      (insert-and-inherit eshell--stored-incomplete-input))))
+
 (defun eshell-add-input-to-history (input)
   "Add the string INPUT to the history ring.
 Input is entered into the input history ring, if the value of
@@ -897,12 +905,12 @@ eshell-previous-matching-input
     ;; 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)))))
+      (eshell-goto-history pos)
+      (unless (or (minibuffer-window-active-p (selected-window))
+                  ;; No messages for Isearch because it will show the
+                  ;; same messages (and more).
+                  isearch-mode)
+	(message "History item: %d" (- (ring-length eshell-history-ring) pos))))))
 
 (defun eshell-next-matching-input (regexp arg)
   "Search forwards through input history for match for REGEXP.
@@ -937,114 +945,160 @@ eshell-next-matching-input-from-input
   (interactive "p")
   (eshell-previous-matching-input-from-input (- arg)))
 
-(defun eshell-test-imatch ()
-  "If isearch match good, put point at the beginning and return non-nil."
-  (if (get-text-property (point) 'history)
-      (progn (beginning-of-line) t)
-    (let ((before (point)))
-      (beginning-of-line)
-      (if (and (not (bolp))
-	       (<= (point) before))
-	  t
-	(if isearch-forward
-	    (progn
-	      (end-of-line)
-	      (forward-char))
-	  (beginning-of-line)
-	  (backward-char))))))
-
-(defun eshell-return-to-prompt ()
-  "Once a search string matches, insert it at the end and go there."
-  (setq isearch-other-end nil)
-  (let ((found (eshell-test-imatch)) before)
-    (while (and (not found)
-		(setq before
-		      (funcall (if isearch-forward
-				   're-search-forward
-				 're-search-backward)
-			       isearch-string nil t)))
-      (setq found (eshell-test-imatch)))
-    (if (not found)
-	(progn
-	  (goto-char eshell-last-output-end)
-	  (delete-region (point) (point-max)))
-      (setq before (point))
-      (let ((text (buffer-substring-no-properties
-		   (point) (line-end-position)))
-	    (orig (marker-position eshell-last-output-end)))
-	(goto-char eshell-last-output-end)
-	(delete-region (point) (point-max))
-	(when (and text (> (length text) 0))
-	  (insert text)
-	  (put-text-property (1- (point)) (point)
-			     'last-search-pos before)
-	  (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-setup ()
+  "Set up Isearch to search the input history.
+Intended to be added to `isearch-mode-hook' in an Eshell buffer."
+  (when (and
+         ;; Eshell is busy running a foreground process
+         (not eshell-foreground-command)
+         (or (eq eshell-history-isearch t)
+             (and (eq eshell-history-isearch 'dwim)
+                  (>= (point) eshell-last-output-end))))
+    (setq isearch-message-prefix-add "history ")
+    (setq-local isearch-lazy-count nil)
+    (setq-local isearch-search-fun-function #'eshell-history-isearch-search
+                isearch-message-function #'eshell-history-isearch-message
+                isearch-wrap-function #'eshell-history-isearch-wrap
+                isearch-push-state-function #'eshell-history-isearch-push-state)
+    (add-hook 'isearch-mode-end-hook #'eshell-history-isearch-end nil t)))
+
+(defun eshell-history-isearch-end ()
+  "Clean up after terminating history Isearch."
+  (when (overlayp eshell--history-isearch-message-overlay)
+    (delete-overlay eshell--history-isearch-message-overlay))
+  (setq isearch-message-prefix-add nil)
+  (kill-local-variable 'isearch-lazy-count)
+  (setq-local isearch-search-fun-function #'isearch-search-fun-default
+              isearch-message-function nil
+              isearch-wrap-function nil
+              isearch-push-state-function nil)
+  (remove-hook 'isearch-mode-end-hook #'eshell-history-isearch-end t)
+  (setq isearch-opoint (point))
+  (unless isearch-suspended
+    (custom-reevaluate-setting 'eshell-history-isearch)))
+
+(defun eshell-history-isearch-search ()
+  "Return search function for Isearch in input history."
+  (lambda (string bound noerror)
+    (let ((search-fun (isearch-search-fun-default))
+          (found nil))
+      ;; Avoid highlighting matches in and before the last prompt
+      (when (and bound isearch-forward
+                 (< (point) eshell-last-output-end))
+        (goto-char eshell-last-output-end))
+      (or
+       ;; First search in the initial input
+       (funcall search-fun string
+                (if isearch-forward bound eshell-last-output-end)
+                noerror)
+       ;; Then search in the input history: put next/previous history
+       ;; element in the command line successively, then search the
+       ;; string in the command line.  Do this only when not
+       ;; lazy-highlighting (`bound' is nil).
+       (unless bound
+         (condition-case nil
+             (progn
+               (while (not found)
+                 (cond (isearch-forward
+                        ;; Signal an error explicitly to break
+                        (when (or (null eshell-history-index)
+                                  (eq eshell-history-index 0))
+                          (error "End of history; no next item"))
+                        (eshell-next-input 1)
+                        (goto-char eshell-last-output-end))
+                       (t
+                        ;; Signal an error explicitly to break
+                        (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))
+                 ;; After putting an history element in the command
+                 ;; line, search the string in them.
+                 (setq found (funcall search-fun string
+                                      (unless isearch-forward
+                                        eshell-last-output-end)
+                                      noerror)))
+               (point))
+           ;; Return when no next/preceding element error signaled
+           (error nil)))))))
+
+(defun eshell-history-isearch-message (&optional c-q-hack ellipsis)
+  "Display the input history search prompt.
+If there are no search errors, this function displays an overlay with
+the Isearch prompt which replaces the original Eshell prompt.
+Otherwise, it displays the standard Isearch message returned from the
+function `isearch-message'."
+  (if (not (and isearch-success (not isearch-error)))
+      ;; Use standard message function (which displays a message in the
+      ;; echo area) when not in command line, or search fails or has
+      ;; errors (like incomplete regexp).
+      (isearch-message c-q-hack ellipsis)
+    ;; Otherwise, use an overlay over the Eshell prompt.
+    (if (overlayp eshell--history-isearch-message-overlay)
+        (move-overlay eshell--history-isearch-message-overlay
+                      (save-excursion
+                        (goto-char eshell-last-output-end)
+                        (forward-line 0)
+                        (point))
+                      eshell-last-output-end)
+      (setq eshell--history-isearch-message-overlay
+            (make-overlay (save-excursion
+                            (goto-char eshell-last-output-end)
+                            (forward-line 0)
+                            (point))
+                          eshell-last-output-end))
+      (overlay-put eshell--history-isearch-message-overlay 'evaporate t))
+    (overlay-put eshell--history-isearch-message-overlay
+                 'display (isearch-message-prefix ellipsis
+                                                  isearch-nonincremental))
+    (if (and eshell-history-index (not ellipsis))
+        (message "History item: %d" (- (ring-length eshell-history-ring)
+                                       eshell-history-index))
+      (message ""))))
+
+(defun eshell-history-isearch-wrap ()
+  "Wrap the input history search."
+  (if isearch-forward
+      (eshell-goto-history (1- (ring-length eshell-history-ring)))
+    (eshell-goto-history nil))
+  (goto-char (if isearch-forward eshell-last-output-end (point-max))))
+
+(defun eshell-history-isearch-push-state ()
+  "Save a function restoring the state of input history search.
+Save `eshell-history-index' to the additional state parameter in the
+search status stack."
+  (let ((index eshell-history-index))
+    (lambda (_cmd)
+      (eshell-goto-history index))))
 
 (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)))
+  "Do incremental search backward through past commands."
+  (interactive nil eshell-mode)
+  (setq eshell-history-isearch t)
+  (if invert
+      (isearch-forward nil t)
+    (isearch-backward nil t)))
 
 (defun eshell-isearch-forward ()
-  "Do incremental regexp search backward through past commands."
-  (interactive)
+  "Do incremental search forward through past commands."
+  (interactive nil eshell-mode)
   (eshell-isearch-backward t))
 
-(defun eshell-isearch-repeat-forward ()
+(defun eshell-isearch-backward-regexp (&optional invert)
   "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-history-isearch t)
+  (if invert
+      (isearch-forward-regexp nil t)
+    (isearch-backward-regexp nil t)))
+
+(defun eshell-isearch-forward-regexp ()
+  "Do incremental regexp search forward through past commands."
+  (interactive nil eshell-mode)
+  (eshell-isearch-backward-regexp t))
 
 (defun em-hist-unload-function ()
   (remove-hook 'kill-emacs-hook 'eshell-save-some-history))
-- 
2.47.1

bug#74287: [PATCH] Rework history Isearch for Eshell

[Pengji Zhang]

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

Pengji Zhang <me@pengjiz.com> writes:

> Juri Linkov <juri@linkov.net> writes:
>
>> (There is only one typo in the variable name
>> 'eshell--history-isearch-messasge-overlay'.)
>
> Thanks! Corrected in the attached updated patch.

Forgot to update the commit message. Please disregard the previous patch
and check the one attached to this email instead. Sorry for the noise!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch updated to correct typos --]
[-- Type: text/x-patch, Size: 20729 bytes --]

From e5242e679090d999549805c30bc63685c1035439 Mon Sep 17 00:00:00 2001
From: Pengji Zhang <me@pengjiz.com>
Date: Thu, 5 Dec 2024 20:25:25 +0800
Subject: [PATCH] Rework history Isearch for Eshell

This is to make history Isearch for Eshell similar to that of
'comint-mode', by hooking into Isearch properly instead of
defining new commands to emulate Isearch.

* lisp/eshell/em-hist.el (eshell-history-isearch): New user
option.
(eshell-goto-history, eshell--isearch-setup)
(eshell-history-isearch-end, eshell-history-isearch-search)
(eshell-history-isearch-message, eshell-history-isearch-wrap)
(eshell-history-isearch-push-state): New functions.
(eshell-isearch-backward-regexp, eshell-isearch-forward-regexp):
New commands.
(eshell--history-isearch-message-overlay)
(eshell--stored-incomplete-input): New internal variables.
(eshell-hist-mode-map): Bind 'M-r' to
'eshell-isearch-backward-regexp' and free 'M-s' binding for
normal in-buffer search commands.
(eshell-isearch-backward, eshell-isearch-forward): Use the new
way to start searching.
(eshell-hist-initialize): Use the new Isearch setup function.
(eshell-previous-matching-input): Use 'eshell-goto-history'.
Also inhibit messages when searching.
(eshell-isearch-map, eshell-isearch-repeat-backward)
(eshell-isearch-abort, eshell-isearch-delete-char)
(eshell-isearch-return, eshell-isearch-cancel)
(eshell-isearch-repeat-forward, eshell-test-imatch)
(eshell-return-to-prompt, eshell-prepare-for-search): Remove.
These are for the old history Isearch implementation.

* doc/misc/eshell.texi (History): Document changes.
* etc/NEWS: Annouce changes.
---
 doc/misc/eshell.texi   |  15 +-
 etc/NEWS               |  23 +++
 lisp/eshell/em-hist.el | 322 ++++++++++++++++++++++++-----------------
 3 files changed, 223 insertions(+), 137 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index ee4d0ca09c8..701137ea1b4 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -2661,10 +2661,10 @@ History
 
 @table @kbd
 @kindex M-r
-@kindex M-s
 @item M-r
-@itemx M-s
-History I-search.
+History I-search.  @kbd{M-r} starts an incremental search in input
+history.  While searching, type @kbd{C-r} to move to the previous match,
+and @kbd{C-s} to move to the next match in the input history.
 
 @kindex M-p
 @kindex M-n
@@ -2675,6 +2675,15 @@ History
 previous or next line that begins with that string.
 @end table
 
+@vindex eshell-history-isearch
+If you would like to use the default Isearch key-bindings to search
+through input history, you may customize @code{eshell-history-isearch}
+to @code{t}.  That makes, for example, @kbd{C-r} and @kbd{C-M-r} in an
+Eshell buffer search in input history only.  In addition, if the value
+of @code{eshell-history-isearch} is @code{dwim}, those commands search
+in the history when the point is after the last prompt, and search in
+the buffer when the point is before or within the last prompt.
+
 @node Extension modules
 @chapter Extension modules
 Eshell provides a facility for defining extension modules so that they
diff --git a/etc/NEWS b/etc/NEWS
index d1c7303f976..c1df6f78d9f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -288,6 +288,29 @@ This hook runs after an Eshell session has been fully initialized,
 immediately before running 'eshell-post-command-hook' for the first
 time.
 
++++
+*** Improved history Isearch.
+History Isearch in Eshell is reworked.  Two new commands
+'eshell-isearch-backward-regexp' and 'eshell-isearch-forward-regexp' are
+added for incrementally searching through the input history.
+'eshell-isearch-backward-regexp' is bound to 'M-r' by default, and 'M-s'
+is freed for normal search commands.  If you would like to restore the
+previous key-bindings for the non-incremental search commands, put in
+your configuration:
+
+    (keymap-set eshell-hist-mode-map "M-r"
+                #'eshell-previous-matching-input)
+    (keymap-set eshell-hist-mode-map "M-s"
+                #'eshell-next-matching-input)
+
++++
+*** New user option 'eshell-history-isearch'
+When 'eshell-history-isearch' is nil (the default), Isearch commands
+search in the buffer contents.  If you customize it to t, those commands
+only search in input history.  If you customize it to the symbol 'dwim',
+those commands search in input history only when the point is after the
+last prompt.
+
 ** SHR
 
 +++
diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index fffd611c06f..c85e7704eca 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -34,7 +34,6 @@
 ;; Also, most of `comint-mode's keybindings are accepted:
 ;;
 ;;   M-r     ; search backward for a previous command by regexp
-;;   M-s     ; search forward for a previous command by regexp
 ;;   M-p     ; access the last command entered, repeatable
 ;;   M-n     ; access the first command entered, repeatable
 ;;
@@ -132,6 +131,17 @@ eshell-input-filter
                 (function :tag "Other function"))
   :risky t)
 
+(defcustom eshell-history-isearch nil
+  "Non-nil to Isearch in input history only.
+If t, usual Isearch keys like \\[isearch-forward] in Eshell search in
+the input history only.  If `dwim', Isearch in the input history when
+point is at the command line, otherwise search in the current Eshell
+buffer."
+  :type '(choice (const :tag "Don't search in input history" nil)
+                 (const :tag "Search histroy when point is on command line" dwim)
+                 (const :tag "Always search in input history" t))
+  :version "31.1")
+
 (defun eshell-hist--update-keymap (symbol value)
   "Update `eshell-hist-mode-map' for `eshell-hist-match-partial'."
   ;; Don't try to set this before it is bound.  See below.
@@ -204,25 +214,17 @@ eshell-save-history-index
 (defvar eshell-hist--new-items nil
   "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-local eshell--history-isearch-message-overlay nil
+  "Overlay for Isearch message when searching through input history.")
+(defvar-local eshell--stored-incomplete-input nil
+  "Stored input for history cycling.")
 
 (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-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
@@ -261,20 +263,9 @@ eshell-hist-initialize
 	   (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--isearch-setup nil t)
 
   (make-local-variable 'eshell-history-size)
   (or eshell-history-size
@@ -384,6 +375,23 @@ eshell-get-history
   "Get an input line from the history ring."
   (ring-ref (or ring eshell-history-ring) index))
 
+(defun eshell-goto-history (pos)
+  "Replace command line with the element at POS of history ring.
+Also update `eshell-history-index'.  As a special case, if POS is nil
+and `eshell--stored-incomplete-input' is a non-empty string, restore the
+saved input."
+  (when (null eshell-history-index)
+    (setq eshell--stored-incomplete-input
+          (buffer-substring-no-properties eshell-last-output-end
+                                          (point-max))))
+  (setq eshell-history-index pos)
+  ;; Can't use kill-region as it sets this-command
+  (delete-region eshell-last-output-end (point-max))
+  (if (and pos (not (ring-empty-p eshell-history-ring)))
+      (insert-and-inherit (eshell-get-history pos))
+    (when (> (length eshell--stored-incomplete-input) 0)
+      (insert-and-inherit eshell--stored-incomplete-input))))
+
 (defun eshell-add-input-to-history (input)
   "Add the string INPUT to the history ring.
 Input is entered into the input history ring, if the value of
@@ -897,12 +905,12 @@ eshell-previous-matching-input
     ;; 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)))))
+      (eshell-goto-history pos)
+      (unless (or (minibuffer-window-active-p (selected-window))
+                  ;; No messages for Isearch because it will show the
+                  ;; same messages (and more).
+                  isearch-mode)
+	(message "History item: %d" (- (ring-length eshell-history-ring) pos))))))
 
 (defun eshell-next-matching-input (regexp arg)
   "Search forwards through input history for match for REGEXP.
@@ -937,114 +945,160 @@ eshell-next-matching-input-from-input
   (interactive "p")
   (eshell-previous-matching-input-from-input (- arg)))
 
-(defun eshell-test-imatch ()
-  "If isearch match good, put point at the beginning and return non-nil."
-  (if (get-text-property (point) 'history)
-      (progn (beginning-of-line) t)
-    (let ((before (point)))
-      (beginning-of-line)
-      (if (and (not (bolp))
-	       (<= (point) before))
-	  t
-	(if isearch-forward
-	    (progn
-	      (end-of-line)
-	      (forward-char))
-	  (beginning-of-line)
-	  (backward-char))))))
-
-(defun eshell-return-to-prompt ()
-  "Once a search string matches, insert it at the end and go there."
-  (setq isearch-other-end nil)
-  (let ((found (eshell-test-imatch)) before)
-    (while (and (not found)
-		(setq before
-		      (funcall (if isearch-forward
-				   're-search-forward
-				 're-search-backward)
-			       isearch-string nil t)))
-      (setq found (eshell-test-imatch)))
-    (if (not found)
-	(progn
-	  (goto-char eshell-last-output-end)
-	  (delete-region (point) (point-max)))
-      (setq before (point))
-      (let ((text (buffer-substring-no-properties
-		   (point) (line-end-position)))
-	    (orig (marker-position eshell-last-output-end)))
-	(goto-char eshell-last-output-end)
-	(delete-region (point) (point-max))
-	(when (and text (> (length text) 0))
-	  (insert text)
-	  (put-text-property (1- (point)) (point)
-			     'last-search-pos before)
-	  (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-setup ()
+  "Set up Isearch to search the input history.
+Intended to be added to `isearch-mode-hook' in an Eshell buffer."
+  (when (and
+         ;; Eshell is busy running a foreground process
+         (not eshell-foreground-command)
+         (or (eq eshell-history-isearch t)
+             (and (eq eshell-history-isearch 'dwim)
+                  (>= (point) eshell-last-output-end))))
+    (setq isearch-message-prefix-add "history ")
+    (setq-local isearch-lazy-count nil)
+    (setq-local isearch-search-fun-function #'eshell-history-isearch-search
+                isearch-message-function #'eshell-history-isearch-message
+                isearch-wrap-function #'eshell-history-isearch-wrap
+                isearch-push-state-function #'eshell-history-isearch-push-state)
+    (add-hook 'isearch-mode-end-hook #'eshell-history-isearch-end nil t)))
+
+(defun eshell-history-isearch-end ()
+  "Clean up after terminating history Isearch."
+  (when (overlayp eshell--history-isearch-message-overlay)
+    (delete-overlay eshell--history-isearch-message-overlay))
+  (setq isearch-message-prefix-add nil)
+  (kill-local-variable 'isearch-lazy-count)
+  (setq-local isearch-search-fun-function #'isearch-search-fun-default
+              isearch-message-function nil
+              isearch-wrap-function nil
+              isearch-push-state-function nil)
+  (remove-hook 'isearch-mode-end-hook #'eshell-history-isearch-end t)
+  (setq isearch-opoint (point))
+  (unless isearch-suspended
+    (custom-reevaluate-setting 'eshell-history-isearch)))
+
+(defun eshell-history-isearch-search ()
+  "Return search function for Isearch in input history."
+  (lambda (string bound noerror)
+    (let ((search-fun (isearch-search-fun-default))
+          (found nil))
+      ;; Avoid highlighting matches in and before the last prompt
+      (when (and bound isearch-forward
+                 (< (point) eshell-last-output-end))
+        (goto-char eshell-last-output-end))
+      (or
+       ;; First search in the initial input
+       (funcall search-fun string
+                (if isearch-forward bound eshell-last-output-end)
+                noerror)
+       ;; Then search in the input history: put next/previous history
+       ;; element in the command line successively, then search the
+       ;; string in the command line.  Do this only when not
+       ;; lazy-highlighting (`bound' is nil).
+       (unless bound
+         (condition-case nil
+             (progn
+               (while (not found)
+                 (cond (isearch-forward
+                        ;; Signal an error explicitly to break
+                        (when (or (null eshell-history-index)
+                                  (eq eshell-history-index 0))
+                          (error "End of history; no next item"))
+                        (eshell-next-input 1)
+                        (goto-char eshell-last-output-end))
+                       (t
+                        ;; Signal an error explicitly to break
+                        (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))
+                 ;; After putting an history element in the command
+                 ;; line, search the string in them.
+                 (setq found (funcall search-fun string
+                                      (unless isearch-forward
+                                        eshell-last-output-end)
+                                      noerror)))
+               (point))
+           ;; Return when no next/preceding element error signaled
+           (error nil)))))))
+
+(defun eshell-history-isearch-message (&optional c-q-hack ellipsis)
+  "Display the input history search prompt.
+If there are no search errors, this function displays an overlay with
+the Isearch prompt which replaces the original Eshell prompt.
+Otherwise, it displays the standard Isearch message returned from the
+function `isearch-message'."
+  (if (not (and isearch-success (not isearch-error)))
+      ;; Use standard message function (which displays a message in the
+      ;; echo area) when not in command line, or search fails or has
+      ;; errors (like incomplete regexp).
+      (isearch-message c-q-hack ellipsis)
+    ;; Otherwise, use an overlay over the Eshell prompt.
+    (if (overlayp eshell--history-isearch-message-overlay)
+        (move-overlay eshell--history-isearch-message-overlay
+                      (save-excursion
+                        (goto-char eshell-last-output-end)
+                        (forward-line 0)
+                        (point))
+                      eshell-last-output-end)
+      (setq eshell--history-isearch-message-overlay
+            (make-overlay (save-excursion
+                            (goto-char eshell-last-output-end)
+                            (forward-line 0)
+                            (point))
+                          eshell-last-output-end))
+      (overlay-put eshell--history-isearch-message-overlay 'evaporate t))
+    (overlay-put eshell--history-isearch-message-overlay
+                 'display (isearch-message-prefix ellipsis
+                                                  isearch-nonincremental))
+    (if (and eshell-history-index (not ellipsis))
+        (message "History item: %d" (- (ring-length eshell-history-ring)
+                                       eshell-history-index))
+      (message ""))))
+
+(defun eshell-history-isearch-wrap ()
+  "Wrap the input history search."
+  (if isearch-forward
+      (eshell-goto-history (1- (ring-length eshell-history-ring)))
+    (eshell-goto-history nil))
+  (goto-char (if isearch-forward eshell-last-output-end (point-max))))
+
+(defun eshell-history-isearch-push-state ()
+  "Save a function restoring the state of input history search.
+Save `eshell-history-index' to the additional state parameter in the
+search status stack."
+  (let ((index eshell-history-index))
+    (lambda (_cmd)
+      (eshell-goto-history index))))
 
 (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)))
+  "Do incremental search backward through past commands."
+  (interactive nil eshell-mode)
+  (setq eshell-history-isearch t)
+  (if invert
+      (isearch-forward nil t)
+    (isearch-backward nil t)))
 
 (defun eshell-isearch-forward ()
-  "Do incremental regexp search backward through past commands."
-  (interactive)
+  "Do incremental search forward through past commands."
+  (interactive nil eshell-mode)
   (eshell-isearch-backward t))
 
-(defun eshell-isearch-repeat-forward ()
+(defun eshell-isearch-backward-regexp (&optional invert)
   "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-history-isearch t)
+  (if invert
+      (isearch-forward-regexp nil t)
+    (isearch-backward-regexp nil t)))
+
+(defun eshell-isearch-forward-regexp ()
+  "Do incremental regexp search forward through past commands."
+  (interactive nil eshell-mode)
+  (eshell-isearch-backward-regexp t))
 
 (defun em-hist-unload-function ()
   (remove-hook 'kill-emacs-hook 'eshell-save-some-history))
-- 
2.47.1

bug#74287: [PATCH] Rework history Isearch for Eshell

[Jim Porter]

On 12/5/2024 4:53 AM, Pengji Zhang wrote:
> Forgot to update the commit message. Please disregard the previous patch
> and check the one attached to this email instead. Sorry for the noise!

I finally had time to test this out. This seems to work well, except for 
one issue. In 'eshell-history-isearch-end' you have this bit:

> +  (unless isearch-suspended
> +    (custom-reevaluate-setting 'eshell-history-isearch)))

Unfortunately, this doesn't work well when using "(setopt 
eshell-history-isearch t)". The value gets reset to nil when exiting 
history Isearch, so the next time a user Isearches, they'll just get 
"regular" Isearch.

Is there a way we could change this so that users who use 'setopt' (or 
even 'setq'?) instead of the Customize package can enable this feature?

bug#74287: [PATCH] Rework history Isearch for Eshell

[Pengji Zhang]

Jim Porter <jporterbugs@gmail.com> writes:

> Is there a way we could change this so that users who use 'setopt' (or
> even 'setq'?) instead of the Customize package can enable this
> feature?

Indeed, it only works with 'M-x customize' (and friends). I guess we can
instead save the original value to another variable and restore from it.

Juri, since you wrote that for 'comint.el' in commit 71e458505f3, what
do you think? Thanks!

bug#74287: [PATCH] Rework history Isearch for Eshell

[Pengji Zhang]

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

Pengji Zhang <me@pengjiz.com> writes:

> Jim Porter <jporterbugs@gmail.com> writes:
>
>> Is there a way we could change this so that users who use 'setopt'
>> (or even 'setq'?) instead of the Customize package can enable this
>> feature?
>
> Indeed, it only works with 'M-x customize' (and friends). I guess we
> can instead save the original value to another variable and restore
> from it.

How about the change in the attachment? If it looks good to you, I will
merge it into the main patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: em-hist-restore-option.patch --]
[-- Type: text/x-patch, Size: 2190 bytes --]

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index c85e7704eca..4bcf434f6e4 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -218,6 +218,9 @@ eshell--history-isearch-message-overlay
   "Overlay for Isearch message when searching through input history.")
 (defvar-local eshell--stored-incomplete-input nil
   "Stored input for history cycling.")
+(defvar eshell--force-history-isearch nil
+  "Non-nil means to force searching in input history.
+If nil, respect the option `eshell-history-isearch'.")
 
 (defvar-keymap eshell-hist-mode-map
   "<up>"     #'eshell-previous-matching-input-from-input
@@ -951,7 +954,8 @@ eshell--isearch-setup
   (when (and
          ;; Eshell is busy running a foreground process
          (not eshell-foreground-command)
-         (or (eq eshell-history-isearch t)
+         (or eshell--force-history-isearch
+             (eq eshell-history-isearch t)
              (and (eq eshell-history-isearch 'dwim)
                   (>= (point) eshell-last-output-end))))
     (setq isearch-message-prefix-add "history ")
@@ -975,7 +979,7 @@ eshell-history-isearch-end
   (remove-hook 'isearch-mode-end-hook #'eshell-history-isearch-end t)
   (setq isearch-opoint (point))
   (unless isearch-suspended
-    (custom-reevaluate-setting 'eshell-history-isearch)))
+    (setq eshell--force-history-isearch nil)))
 
 (defun eshell-history-isearch-search ()
   "Return search function for Isearch in input history."
@@ -1077,7 +1081,7 @@ eshell-history-isearch-push-state
 (defun eshell-isearch-backward (&optional invert)
   "Do incremental search backward through past commands."
   (interactive nil eshell-mode)
-  (setq eshell-history-isearch t)
+  (setq eshell--force-history-isearch t)
   (if invert
       (isearch-forward nil t)
     (isearch-backward nil t)))
@@ -1090,7 +1094,7 @@ eshell-isearch-forward
 (defun eshell-isearch-backward-regexp (&optional invert)
   "Do incremental regexp search backward through past commands."
   (interactive nil eshell-mode)
-  (setq eshell-history-isearch t)
+  (setq eshell--force-history-isearch t)
   (if invert
       (isearch-forward-regexp nil t)
     (isearch-backward-regexp nil t)))

bug#74287: [PATCH] Rework history Isearch for Eshell

[Juri Linkov]

>> Is there a way we could change this so that users who use 'setopt' (or
>> even 'setq'?) instead of the Customize package can enable this
>> feature?
>
> Indeed, it only works with 'M-x customize' (and friends). I guess we can
> instead save the original value to another variable and restore from it.
>
> Juri, since you wrote that for 'comint.el' in commit 71e458505f3, what
> do you think? Thanks!

I confirm the same problem in comint.  The current solution was described
in bug#30187 as the least bad.  Fortunately, no one complained because
it works with saved customizations.  But still we need to fix it.

So do you propose to add an internal variable that duplicates the user option?

bug#74287: [PATCH] Rework history Isearch for Eshell

[Jim Porter]

On 12/9/2024 5:35 AM, Pengji Zhang wrote:
> How about the change in the attachment? If it looks good to you, I will
> merge it into the main patch.

This change works for me, so if Juri thinks it's ok, then let's do it 
this way. (Later, we can port this change to Comint to keep this code in 
sync.)

bug#74287: [PATCH] Rework history Isearch for Eshell

[Pengji Zhang]

Juri Linkov <juri@linkov.net> writes:

> [...]
>
> So do you propose to add an internal variable that duplicates the user
> option?

Yes, sort of. It does not fully duplicates the user option though.
Because we only need to force turning *on* history search, it is made to
override the user option only when it is non-nil. Basically, it is
something like:

--8<---------------cut here---------------start------------->8---
@@ -951,7 +954,8 @@ eshell--isearch-setup
   (when (and
          ;; Eshell is busy running a foreground process
          (not eshell-foreground-command)
-         (or (eq eshell-history-isearch t)
+         (or eshell--force-history-isearch
+             (eq eshell-history-isearch t)
              (and (eq eshell-history-isearch 'dwim)
                   (>= (point) eshell-last-output-end))))
     (setq isearch-message-prefix-add "history ")
@@ -975,7 +979,7 @@ eshell-history-isearch-end
   (remove-hook 'isearch-mode-end-hook #'eshell-history-isearch-end t)
   (setq isearch-opoint (point))
   (unless isearch-suspended
-    (custom-reevaluate-setting 'eshell-history-isearch)))
+    (setq eshell--force-history-isearch nil)))
--8<---------------cut here---------------end--------------->8---

bug#74287: [PATCH] Rework history Isearch for Eshell

[Juri Linkov]

>> So do you propose to add an internal variable that duplicates the user
>> option?
>
> Yes, sort of. It does not fully duplicates the user option though.
> Because we only need to force turning *on* history search, it is made to
> override the user option only when it is non-nil. Basically, it is
> something like:
>
> @@ -951,7 +954,8 @@ eshell--isearch-setup
>    (when (and
>           ;; Eshell is busy running a foreground process
>           (not eshell-foreground-command)
> -         (or (eq eshell-history-isearch t)
> +         (or eshell--force-history-isearch
> +             (eq eshell-history-isearch t)
>               (and (eq eshell-history-isearch 'dwim)
>                    (>= (point) eshell-last-output-end))))
>      (setq isearch-message-prefix-add "history ")
> @@ -975,7 +979,7 @@ eshell-history-isearch-end
>    (remove-hook 'isearch-mode-end-hook #'eshell-history-isearch-end t)
>    (setq isearch-opoint (point))
>    (unless isearch-suspended
> -    (custom-reevaluate-setting 'eshell-history-isearch)))
> +    (setq eshell--force-history-isearch nil)))

I agree this would be the right thing, because even in Isearch
we have variables that duplicate the user options such as e.g.
the option 'search-invisible' and the internal variable 'isearch-invisible'.

bug#74287: [PATCH] Rework history Isearch for Eshell

[Pengji Zhang]

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

Juri Linkov <juri@linkov.net> writes:

> I agree this would be the right thing, because even in Isearch we have
> variables that duplicate the user options such as e.g. the option
> 'search-invisible' and the internal variable 'isearch-invisible'.

Thanks!

Jim, please have a look at the attached updated patch, which includes
this change. Thank you!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Rework-history-Isearch-for-Eshell.patch --]
[-- Type: text/x-patch, Size: 20962 bytes --]

From 2cc9e260da56a5df24cf8932fb688143a356c150 Mon Sep 17 00:00:00 2001
From: Pengji Zhang <me@pengjiz.com>
Date: Tue, 10 Dec 2024 18:55:36 +0800
Subject: [PATCH] Rework history Isearch for Eshell

This is to make history Isearch for Eshell similar to that of
'comint-mode', by hooking into Isearch properly instead of
defining new commands to emulate Isearch.

* lisp/eshell/em-hist.el (eshell-history-isearch): New user
option.
(eshell-goto-history, eshell--isearch-setup)
(eshell-history-isearch-end, eshell-history-isearch-search)
(eshell-history-isearch-message, eshell-history-isearch-wrap)
(eshell-history-isearch-push-state): New functions.
(eshell-isearch-backward-regexp, eshell-isearch-forward-regexp):
New commands.
(eshell--history-isearch-message-overlay)
(eshell--stored-incomplete-input, eshell--force-history-isearch):
New internal variables.
(eshell-hist-mode-map): Bind 'M-r' to
'eshell-isearch-backward-regexp' and free 'M-s' binding for
normal in-buffer search commands.
(eshell-isearch-backward, eshell-isearch-forward): Use the new
way to start searching.
(eshell-hist-initialize): Use the new Isearch setup function.
(eshell-previous-matching-input): Use 'eshell-goto-history'.
Also inhibit messages when searching.
(eshell-isearch-map, eshell-isearch-repeat-backward)
(eshell-isearch-abort, eshell-isearch-delete-char)
(eshell-isearch-return, eshell-isearch-cancel)
(eshell-isearch-repeat-forward, eshell-test-imatch)
(eshell-return-to-prompt, eshell-prepare-for-search): Remove.
These are for the old history Isearch implementation.

* doc/misc/eshell.texi (History): Document changes.
* etc/NEWS: Annouce changes.
---
 doc/misc/eshell.texi   |  15 +-
 etc/NEWS               |  23 +++
 lisp/eshell/em-hist.el | 326 ++++++++++++++++++++++++-----------------
 3 files changed, 227 insertions(+), 137 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index ee4d0ca09c8..701137ea1b4 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -2661,10 +2661,10 @@ History
 
 @table @kbd
 @kindex M-r
-@kindex M-s
 @item M-r
-@itemx M-s
-History I-search.
+History I-search.  @kbd{M-r} starts an incremental search in input
+history.  While searching, type @kbd{C-r} to move to the previous match,
+and @kbd{C-s} to move to the next match in the input history.
 
 @kindex M-p
 @kindex M-n
@@ -2675,6 +2675,15 @@ History
 previous or next line that begins with that string.
 @end table
 
+@vindex eshell-history-isearch
+If you would like to use the default Isearch key-bindings to search
+through input history, you may customize @code{eshell-history-isearch}
+to @code{t}.  That makes, for example, @kbd{C-r} and @kbd{C-M-r} in an
+Eshell buffer search in input history only.  In addition, if the value
+of @code{eshell-history-isearch} is @code{dwim}, those commands search
+in the history when the point is after the last prompt, and search in
+the buffer when the point is before or within the last prompt.
+
 @node Extension modules
 @chapter Extension modules
 Eshell provides a facility for defining extension modules so that they
diff --git a/etc/NEWS b/etc/NEWS
index d1c7303f976..c1df6f78d9f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -288,6 +288,29 @@ This hook runs after an Eshell session has been fully initialized,
 immediately before running 'eshell-post-command-hook' for the first
 time.
 
++++
+*** Improved history Isearch.
+History Isearch in Eshell is reworked.  Two new commands
+'eshell-isearch-backward-regexp' and 'eshell-isearch-forward-regexp' are
+added for incrementally searching through the input history.
+'eshell-isearch-backward-regexp' is bound to 'M-r' by default, and 'M-s'
+is freed for normal search commands.  If you would like to restore the
+previous key-bindings for the non-incremental search commands, put in
+your configuration:
+
+    (keymap-set eshell-hist-mode-map "M-r"
+                #'eshell-previous-matching-input)
+    (keymap-set eshell-hist-mode-map "M-s"
+                #'eshell-next-matching-input)
+
++++
+*** New user option 'eshell-history-isearch'
+When 'eshell-history-isearch' is nil (the default), Isearch commands
+search in the buffer contents.  If you customize it to t, those commands
+only search in input history.  If you customize it to the symbol 'dwim',
+those commands search in input history only when the point is after the
+last prompt.
+
 ** SHR
 
 +++
diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index fffd611c06f..4bcf434f6e4 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -34,7 +34,6 @@
 ;; Also, most of `comint-mode's keybindings are accepted:
 ;;
 ;;   M-r     ; search backward for a previous command by regexp
-;;   M-s     ; search forward for a previous command by regexp
 ;;   M-p     ; access the last command entered, repeatable
 ;;   M-n     ; access the first command entered, repeatable
 ;;
@@ -132,6 +131,17 @@ eshell-input-filter
                 (function :tag "Other function"))
   :risky t)
 
+(defcustom eshell-history-isearch nil
+  "Non-nil to Isearch in input history only.
+If t, usual Isearch keys like \\[isearch-forward] in Eshell search in
+the input history only.  If `dwim', Isearch in the input history when
+point is at the command line, otherwise search in the current Eshell
+buffer."
+  :type '(choice (const :tag "Don't search in input history" nil)
+                 (const :tag "Search histroy when point is on command line" dwim)
+                 (const :tag "Always search in input history" t))
+  :version "31.1")
+
 (defun eshell-hist--update-keymap (symbol value)
   "Update `eshell-hist-mode-map' for `eshell-hist-match-partial'."
   ;; Don't try to set this before it is bound.  See below.
@@ -204,25 +214,20 @@ eshell-save-history-index
 (defvar eshell-hist--new-items nil
   "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-local eshell--history-isearch-message-overlay nil
+  "Overlay for Isearch message when searching through input history.")
+(defvar-local eshell--stored-incomplete-input nil
+  "Stored input for history cycling.")
+(defvar eshell--force-history-isearch nil
+  "Non-nil means to force searching in input history.
+If nil, respect the option `eshell-history-isearch'.")
 
 (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-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
@@ -261,20 +266,9 @@ eshell-hist-initialize
 	   (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--isearch-setup nil t)
 
   (make-local-variable 'eshell-history-size)
   (or eshell-history-size
@@ -384,6 +378,23 @@ eshell-get-history
   "Get an input line from the history ring."
   (ring-ref (or ring eshell-history-ring) index))
 
+(defun eshell-goto-history (pos)
+  "Replace command line with the element at POS of history ring.
+Also update `eshell-history-index'.  As a special case, if POS is nil
+and `eshell--stored-incomplete-input' is a non-empty string, restore the
+saved input."
+  (when (null eshell-history-index)
+    (setq eshell--stored-incomplete-input
+          (buffer-substring-no-properties eshell-last-output-end
+                                          (point-max))))
+  (setq eshell-history-index pos)
+  ;; Can't use kill-region as it sets this-command
+  (delete-region eshell-last-output-end (point-max))
+  (if (and pos (not (ring-empty-p eshell-history-ring)))
+      (insert-and-inherit (eshell-get-history pos))
+    (when (> (length eshell--stored-incomplete-input) 0)
+      (insert-and-inherit eshell--stored-incomplete-input))))
+
 (defun eshell-add-input-to-history (input)
   "Add the string INPUT to the history ring.
 Input is entered into the input history ring, if the value of
@@ -897,12 +908,12 @@ eshell-previous-matching-input
     ;; 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)))))
+      (eshell-goto-history pos)
+      (unless (or (minibuffer-window-active-p (selected-window))
+                  ;; No messages for Isearch because it will show the
+                  ;; same messages (and more).
+                  isearch-mode)
+	(message "History item: %d" (- (ring-length eshell-history-ring) pos))))))
 
 (defun eshell-next-matching-input (regexp arg)
   "Search forwards through input history for match for REGEXP.
@@ -937,114 +948,161 @@ eshell-next-matching-input-from-input
   (interactive "p")
   (eshell-previous-matching-input-from-input (- arg)))
 
-(defun eshell-test-imatch ()
-  "If isearch match good, put point at the beginning and return non-nil."
-  (if (get-text-property (point) 'history)
-      (progn (beginning-of-line) t)
-    (let ((before (point)))
-      (beginning-of-line)
-      (if (and (not (bolp))
-	       (<= (point) before))
-	  t
-	(if isearch-forward
-	    (progn
-	      (end-of-line)
-	      (forward-char))
-	  (beginning-of-line)
-	  (backward-char))))))
-
-(defun eshell-return-to-prompt ()
-  "Once a search string matches, insert it at the end and go there."
-  (setq isearch-other-end nil)
-  (let ((found (eshell-test-imatch)) before)
-    (while (and (not found)
-		(setq before
-		      (funcall (if isearch-forward
-				   're-search-forward
-				 're-search-backward)
-			       isearch-string nil t)))
-      (setq found (eshell-test-imatch)))
-    (if (not found)
-	(progn
-	  (goto-char eshell-last-output-end)
-	  (delete-region (point) (point-max)))
-      (setq before (point))
-      (let ((text (buffer-substring-no-properties
-		   (point) (line-end-position)))
-	    (orig (marker-position eshell-last-output-end)))
-	(goto-char eshell-last-output-end)
-	(delete-region (point) (point-max))
-	(when (and text (> (length text) 0))
-	  (insert text)
-	  (put-text-property (1- (point)) (point)
-			     'last-search-pos before)
-	  (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-setup ()
+  "Set up Isearch to search the input history.
+Intended to be added to `isearch-mode-hook' in an Eshell buffer."
+  (when (and
+         ;; Eshell is busy running a foreground process
+         (not eshell-foreground-command)
+         (or eshell--force-history-isearch
+             (eq eshell-history-isearch t)
+             (and (eq eshell-history-isearch 'dwim)
+                  (>= (point) eshell-last-output-end))))
+    (setq isearch-message-prefix-add "history ")
+    (setq-local isearch-lazy-count nil)
+    (setq-local isearch-search-fun-function #'eshell-history-isearch-search
+                isearch-message-function #'eshell-history-isearch-message
+                isearch-wrap-function #'eshell-history-isearch-wrap
+                isearch-push-state-function #'eshell-history-isearch-push-state)
+    (add-hook 'isearch-mode-end-hook #'eshell-history-isearch-end nil t)))
+
+(defun eshell-history-isearch-end ()
+  "Clean up after terminating history Isearch."
+  (when (overlayp eshell--history-isearch-message-overlay)
+    (delete-overlay eshell--history-isearch-message-overlay))
+  (setq isearch-message-prefix-add nil)
+  (kill-local-variable 'isearch-lazy-count)
+  (setq-local isearch-search-fun-function #'isearch-search-fun-default
+              isearch-message-function nil
+              isearch-wrap-function nil
+              isearch-push-state-function nil)
+  (remove-hook 'isearch-mode-end-hook #'eshell-history-isearch-end t)
+  (setq isearch-opoint (point))
+  (unless isearch-suspended
+    (setq eshell--force-history-isearch nil)))
+
+(defun eshell-history-isearch-search ()
+  "Return search function for Isearch in input history."
+  (lambda (string bound noerror)
+    (let ((search-fun (isearch-search-fun-default))
+          (found nil))
+      ;; Avoid highlighting matches in and before the last prompt
+      (when (and bound isearch-forward
+                 (< (point) eshell-last-output-end))
+        (goto-char eshell-last-output-end))
+      (or
+       ;; First search in the initial input
+       (funcall search-fun string
+                (if isearch-forward bound eshell-last-output-end)
+                noerror)
+       ;; Then search in the input history: put next/previous history
+       ;; element in the command line successively, then search the
+       ;; string in the command line.  Do this only when not
+       ;; lazy-highlighting (`bound' is nil).
+       (unless bound
+         (condition-case nil
+             (progn
+               (while (not found)
+                 (cond (isearch-forward
+                        ;; Signal an error explicitly to break
+                        (when (or (null eshell-history-index)
+                                  (eq eshell-history-index 0))
+                          (error "End of history; no next item"))
+                        (eshell-next-input 1)
+                        (goto-char eshell-last-output-end))
+                       (t
+                        ;; Signal an error explicitly to break
+                        (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))
+                 ;; After putting an history element in the command
+                 ;; line, search the string in them.
+                 (setq found (funcall search-fun string
+                                      (unless isearch-forward
+                                        eshell-last-output-end)
+                                      noerror)))
+               (point))
+           ;; Return when no next/preceding element error signaled
+           (error nil)))))))
+
+(defun eshell-history-isearch-message (&optional c-q-hack ellipsis)
+  "Display the input history search prompt.
+If there are no search errors, this function displays an overlay with
+the Isearch prompt which replaces the original Eshell prompt.
+Otherwise, it displays the standard Isearch message returned from the
+function `isearch-message'."
+  (if (not (and isearch-success (not isearch-error)))
+      ;; Use standard message function (which displays a message in the
+      ;; echo area) when not in command line, or search fails or has
+      ;; errors (like incomplete regexp).
+      (isearch-message c-q-hack ellipsis)
+    ;; Otherwise, use an overlay over the Eshell prompt.
+    (if (overlayp eshell--history-isearch-message-overlay)
+        (move-overlay eshell--history-isearch-message-overlay
+                      (save-excursion
+                        (goto-char eshell-last-output-end)
+                        (forward-line 0)
+                        (point))
+                      eshell-last-output-end)
+      (setq eshell--history-isearch-message-overlay
+            (make-overlay (save-excursion
+                            (goto-char eshell-last-output-end)
+                            (forward-line 0)
+                            (point))
+                          eshell-last-output-end))
+      (overlay-put eshell--history-isearch-message-overlay 'evaporate t))
+    (overlay-put eshell--history-isearch-message-overlay
+                 'display (isearch-message-prefix ellipsis
+                                                  isearch-nonincremental))
+    (if (and eshell-history-index (not ellipsis))
+        (message "History item: %d" (- (ring-length eshell-history-ring)
+                                       eshell-history-index))
+      (message ""))))
+
+(defun eshell-history-isearch-wrap ()
+  "Wrap the input history search."
+  (if isearch-forward
+      (eshell-goto-history (1- (ring-length eshell-history-ring)))
+    (eshell-goto-history nil))
+  (goto-char (if isearch-forward eshell-last-output-end (point-max))))
+
+(defun eshell-history-isearch-push-state ()
+  "Save a function restoring the state of input history search.
+Save `eshell-history-index' to the additional state parameter in the
+search status stack."
+  (let ((index eshell-history-index))
+    (lambda (_cmd)
+      (eshell-goto-history index))))
 
 (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)))
+  "Do incremental search backward through past commands."
+  (interactive nil eshell-mode)
+  (setq eshell--force-history-isearch t)
+  (if invert
+      (isearch-forward nil t)
+    (isearch-backward nil t)))
 
 (defun eshell-isearch-forward ()
-  "Do incremental regexp search backward through past commands."
-  (interactive)
+  "Do incremental search forward through past commands."
+  (interactive nil eshell-mode)
   (eshell-isearch-backward t))
 
-(defun eshell-isearch-repeat-forward ()
+(defun eshell-isearch-backward-regexp (&optional invert)
   "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--force-history-isearch t)
+  (if invert
+      (isearch-forward-regexp nil t)
+    (isearch-backward-regexp nil t)))
+
+(defun eshell-isearch-forward-regexp ()
+  "Do incremental regexp search forward through past commands."
+  (interactive nil eshell-mode)
+  (eshell-isearch-backward-regexp t))
 
 (defun em-hist-unload-function ()
   (remove-hook 'kill-emacs-hook 'eshell-save-some-history))
-- 
2.47.1

bug#74287: [PATCH] Rework history Isearch for Eshell

[Juri Linkov]

>> I agree this would be the right thing, because even in Isearch we have
>> variables that duplicate the user options such as e.g. the option
>> 'search-invisible' and the internal variable 'isearch-invisible'.
>
> Thanks!

Please also see the commit 98f8c5fb07dcb4e20b8ba6648bdabf5c0d3b5145
that fixed this in dired by using a buffer-local variable.
Do you agree it's a good fix?  Otherwise using a separate variable
is also fine.

bug#74287: [PATCH] Rework history Isearch for Eshell

[Pengji Zhang]

Juri Linkov <juri@linkov.net> writes:

>>> I agree this would be the right thing, because even in Isearch we
>>> have variables that duplicate the user options such as e.g. the
>>> option 'search-invisible' and the internal variable
>>> 'isearch-invisible'.
>>
>> Thanks!
>
> Please also see the commit 98f8c5fb07dcb4e20b8ba6648bdabf5c0d3b5145
> that fixed this in dired by using a buffer-local variable. Do you
> agree it's a good fix? Otherwise using a separate variable is also
> fine.

My concern with using a local variable is that users may also set the
user option locally. That is perhaps not a problem for eshell or dired,
but for comint, e.g., I may set 'comint-history-isearch' to 'dwim' only
in 'M-x shell' via the mode hook.

IMO, changing an internal variable feels a bit cleaner and safer than
manipulating a user option. So I would prefer using a separate variable.

bug#74287: [PATCH] Rework history Isearch for Eshell

[Juri Linkov]

>> Please also see the commit 98f8c5fb07dcb4e20b8ba6648bdabf5c0d3b5145
>> that fixed this in dired by using a buffer-local variable. Do you
>> agree it's a good fix? Otherwise using a separate variable is also
>> fine.
>
> My concern with using a local variable is that users may also set the
> user option locally. That is perhaps not a problem for eshell or dired,
> but for comint, e.g., I may set 'comint-history-isearch' to 'dwim' only
> in 'M-x shell' via the mode hook.

Indeed, this is a valid use case.

> IMO, changing an internal variable feels a bit cleaner and safer than
> manipulating a user option. So I would prefer using a separate variable.

I agree that a separate variable is better.  So now I added to comint.el
'comint--force-history-isearch' extracted from your Eshell patch.

Thanks for the fix!

bug#74287: [PATCH] Rework history Isearch for Eshell

[Jim Porter]

On 12/10/2024 3:01 AM, Pengji Zhang wrote:
> Jim, please have a look at the attached updated patch, which includes
> this change. Thank you!

Thanks for your work on this! I've now merged your patch to the master 
branch as 3959ea66448, so I'm closing this bug now. (Of course, if I 
missed something, just let me know.)