bug#71576: 30.0.50; [PATCH] Improve performance of Comint/Eshell password prompt handling

[Stefan Kangas]

Jim Porter <jporterbugs@gmail.com> writes:

> I've added NEWS entries for this, although maybe this isn't something
> that really needs to be announced. Still, I figured it was worth
> mentioning in the unlikely case that the new behavior could cause some
> problem with (very!) long password prompts.
>
> I'm also totally fine with letting this patch wait for Emacs 31 if
> there's any concern about the code. It's not a big change, but maybe
> it's worth erring on the side of stability.

Thanks.  I'd be okay with putting this patch in Emacs 30, but let's see
what other people think.

> From 197ec6368e6d2678f8f1601dc1a9800855df0943 Mon Sep 17 00:00:00 2001
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Sat, 15 Jun 2024 11:03:33 -0700
> Subject: [PATCH] Limit the amount of text we examine when looking for password
>  prompts
>
> Both Comint and Eshell do this, and it can significantly slow down
> commands that write a lot of output.
>
> * lisp/comint.el (comint-password-prompt-max-length): New option...
> (comint-watch-for-password-prompt): ... use it.  Additionally, use the
> matched result for the Emacs-based password prompt.
>
> * lisp/eshell/esh-mode.el (eshell-password-prompt-max-length): New
> option...
> (eshell-watch-for-password-prompt): ... use it.
>
> * etc/NEWS: Announce this change.
> ---
>  etc/NEWS                | 21 +++++++++++++++---
>  lisp/comint.el          | 49 +++++++++++++++++++++++++++--------------
>  lisp/eshell/esh-mode.el | 41 +++++++++++++++++++++++-----------
>  3 files changed, 78 insertions(+), 33 deletions(-)
>
> diff --git a/etc/NEWS b/etc/NEWS
> index b2fdbc4a88f..1cf5025910c 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1002,9 +1002,16 @@ more information on this notation.
>  ---
>  *** Performance improvements for interactive output in Eshell.
>  Interactive output in Eshell should now be significantly faster,
> -especially for built-in commands that can print large amounts of output
> -(e.g. "cat").  In addition, these commands can now update the display
> -periodically to show their progress.
> +especially for commands that can print large amounts of output
> +(e.g. "cat").  For external commands, Eshell saves time by only looking
> +for password prompts in the last 256 characters of each block of output.
> +To change the amount of text to examine, customize
> +'eshell-password-prompt-max-length'.
> +
> +---
> +*** Eshell built-in commands can now display progress.
> +Eshell built-in commands like "cat" and "ls" now update the display
> +periodically while running to show their progress.
>
>  +++
>  *** New special reference type '#<marker POSITION BUFFER>'.
> @@ -1160,6 +1167,14 @@ environment variable 'HISTFILE'.
>
>  In a 'shell' buffer, this user option is connection-local.
>
> +---
> +*** Performance improvements for interactive output.
> +Interactive output in Shell mode now scans more selectively for password
> +prompts by only examining the last 256 characters of each block of
> +output, reducing the time spent when printing large amounts of output.
> +To change the amount of text to examine, customize
> +'comint-password-prompt-max-length'.
> +
>  ** Make mode
>
>  *** The Makefile browser is now obsolete.
> diff --git a/lisp/comint.el b/lisp/comint.el
> index 3804932e01c..b8a12074fb7 100644
> --- a/lisp/comint.el
> +++ b/lisp/comint.el
> @@ -426,6 +426,18 @@ comint-password-prompt-regexp
>    :type 'regexp
>    :group 'comint)
>
> +(defcustom comint-password-prompt-max-length 256
> +  "The maximum amount of text to examine when matching password prompts.
> +When non-nil, only examine the last N characters of a block of output.
> +If nil, examine all the output.
> +
> +This is used by `comint-watch-for-password-prompt' to reduce the amount
> +of time spent searching for password prompts."
> +  :version "30.1"
> +  :type '(choice natnum
> +                 (const :tag "Examine all output" nil))
> +  :group 'comint)

If this is hardcoded in Tramp, are we sure that we need this as an
option?  I'd suggest making it into a defconst or defvar instead.

> +
>  ;; Here are the per-interpreter hooks.
>  (defvar comint-get-old-input (function comint-get-old-input-default)
>    "Function that returns old text in Comint mode.
> @@ -2563,23 +2575,26 @@ comint-watch-for-password-prompt
>  carriage returns (\\r) in STRING.
>
>  This function could be in the list `comint-output-filter-functions'."
> -  (when (let ((case-fold-search t))
> -       (string-match comint-password-prompt-regexp
> -                        (string-replace "\r" "" string)))
> -    ;; Use `run-at-time' in order not to pause execution of the
> -    ;; process filter with a minibuffer
> -    (run-at-time
> -     0 nil
> -     (lambda (current-buf)
> -       (with-current-buffer current-buf
> -         (let ((comint--prompt-recursion-depth
> -                (1+ comint--prompt-recursion-depth)))
> -           (if (> comint--prompt-recursion-depth 10)
> -               (message "Password prompt recursion too deep")
> -             (when (get-buffer-process (current-buffer))
> -               (comint-send-invisible
> -                (string-trim string "[ \n\r\t\v\f\b\a]+" "\n+")))))))
> -     (current-buffer))))
> +  (let ((string (string-limit string comint-password-prompt-max-length t))
> +        prompt)
> +    (when (let ((case-fold-search t))
> +            (string-match comint-password-prompt-regexp
> +                          (string-replace "\r" "" string)))
> +      (setq prompt (string-trim (match-string 0 string)
> +                                "[ \n\r\t\v\f\b\a]+" "\n+"))
> +      ;; Use `run-at-time' in order not to pause execution of the
> +      ;; process filter with a minibuffer
> +      (run-at-time
> +       0 nil
> +       (lambda (current-buf)
> +         (with-current-buffer current-buf
> +           (let ((comint--prompt-recursion-depth
> +                  (1+ comint--prompt-recursion-depth)))
> +             (if (> comint--prompt-recursion-depth 10)
> +                 (message "Password prompt recursion too deep")
> +               (when (get-buffer-process (current-buffer))
> +                 (comint-send-invisible prompt))))))
> +       (current-buffer)))))
>  
>  ;; Low-level process communication
>
> diff --git a/lisp/eshell/esh-mode.el b/lisp/eshell/esh-mode.el
> index ec1a07b7e2f..5603fef90a4 100644
> --- a/lisp/eshell/esh-mode.el
> +++ b/lisp/eshell/esh-mode.el
> @@ -182,6 +182,18 @@ eshell-password-prompt-regexp
>    :type 'regexp
>    :version "27.1")
>
> +(defcustom eshell-password-prompt-max-length 256
> +  "The maximum amount of text to examine when matching password prompts.
> +When non-nil, only examine the last N characters of a block of output.
> +If nil, examine all the output.
> +
> +This is used by `eshell-watch-for-password-prompt' to reduce the amount
> +of time spent searching for password prompts."
> +  :version "30.1"
> +  :type '(choice natnum
> +                 (const :tag "Examine all output" nil))
> +  :group 'comint)
> +
>  (defcustom eshell-skip-prompt-function nil
>    "A function called from beginning of line to skip the prompt."
>    :type '(choice (const nil) function))
> @@ -949,19 +961,22 @@ eshell-watch-for-password-prompt
>  This function could be in the list `eshell-output-filter-functions'."
>    (when (eshell-head-process)
>      (save-excursion
> -      (let ((case-fold-search t))
> -     (goto-char eshell-last-output-block-begin)
> -     (beginning-of-line)
> -     (if (re-search-forward eshell-password-prompt-regexp
> -                            eshell-last-output-end t)
> -            ;; Use `run-at-time' in order not to pause execution of
> -            ;; the process filter with a minibuffer
> -         (run-at-time
> -             0 nil
> -             (lambda (current-buf)
> -               (with-current-buffer current-buf
> -                 (eshell-send-invisible)))
> -             (current-buffer)))))))
> +      (goto-char (if eshell-password-prompt-max-length
> +                     (max eshell-last-output-block-begin
> +                          (- eshell-last-output-end
> +                             eshell-password-prompt-max-length))
> +                   eshell-last-output-block-begin))
> +      (when (let ((case-fold-search t))
> +              (re-search-forward eshell-password-prompt-regexp
> +                                 eshell-last-output-end t))

Could this be simplified using re-search-backward with the BOUND
argument instead?

> +        ;; Use `run-at-time' in order not to pause execution of the
> +        ;; process filter with a minibuffer.
> +        (run-at-time
> +         0 nil
> +         (lambda (current-buf)
> +           (with-current-buffer current-buf
> +             (eshell-send-invisible)))
> +         (current-buffer))))))
>
>  (custom-add-option 'eshell-output-filter-functions
>                  'eshell-watch-for-password-prompt)
> --
> 2.25.1