bug#61602: [PATCH]: comint-mode redirection

[Rah Guzar via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

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


Hi Ruijie,
   Thanks for the comments.

Ruijie Yu <ruijie@netyu.xyz> writes:

> Thanks for the patch.  First of all, when sending a patch(set) for
> Emacs, you need to run something like this:
>
>     $ git format-patch
>
> and send the generated file(s).  Take a look at its manpage and ask if
> you have any questions.  What you have sent is a "diff" file, which
> bears no commit messages.  At least in Emacs contributions, patches
> should usually come together with their commit messages.
>
> And there are guidelines on commit messages, see /CONTRIBUTE on
> emacs.git.

I have attached a patch which hopefully is made properly this time.

> Further in-line comments below.
>
> Rah Guzar via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> writes:
>
>>  This variable is only used if the variable
>> -`comint-use-prompt-regexp' is non-nil.
>> +`comint-use-prompt-regexp' is non-nil.  The exception to
>> +this is redirection.  Many commands including
>> +`comint-redirect-send-command-to-process' use it as
>> +`comint-redirect-finished-regexp'.
>
> This paragraph sounds a bit weird, but I don't know how to reword it.
> Maybe someone else can help.
>

I tried some rewording.

>>  (defvar comint-redirect-finished-regexp nil
>>    "Regular expression that determines when to stop redirection in Comint.
>>  When the redirection filter function is given output that matches this regexp,
>> -the output is inserted as usual, and redirection is completed.")
>> +the output is inserted as usual, and redirection is completed.
>> +This is an internal variable set by `comint-redirect-setup' and setting it
>> +directly has no effect.")
>
> If this is indeed a private variable, why does it contain no
> double-dashes in its name prior to your changes?
>
> Also, here and elsewhere, except for the first line, there should
> generally be one empty line between paragraphs.
>

It is not declared as a private variable but it is in effect used as
one, so documenting should prevent confusion. Some context: I wanted
to use redirection to display some documentation is a separate buffer.
However documentation sometimes contained the usual prompt and that
messed up the comint buffer. To get around it I setup the redirected
commands so that they had a custom prompt and tried to set c-r-f-r
accordingly but that didn't work and I had to fish around the comint
source to figure out why.

>> +(defvar comint-redirect-hook nil
>> +  "Hook run when a redirection finishes.")

> Does it make sense for a user to customize the hook?  If so, you should
> convert this variable into a `defcustom'.

This for better discoverability. I think comint-mode is too low-level for
most users to use it directly. But e.g. for those writing a derived mode
it is good to be aware that this hook exists without diving into source.

>> +If FINISHED-REGEXP is non-nil it is used as `comint-redirect-finished-regexp'
>> +instead of `comint-prompt-regexp'."
>
> Please clarify what "it" is.
>
> If you are referring to the change below from `cominit-prompt-regexp' to
> `(or finished-regexp comint-prompt-regexp)', then the current form is
> ambiguous, and maybe you should say something like this:
>
>     If F-R is non-nil, it is used as `c-r-f-r'.  Otherwise `c-p-r' is
>     used as `c-r-f-r'.
>

Thanks, I incorporated your suggestion.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Bug-61602-Tweak-comint-mode-redirection.patch --]
[-- Type: text/x-patch, Size: 3268 bytes --]

From ac814964e1885d005746e98f5010a2b400293810 Mon Sep 17 00:00:00 2001
From: Rahguzar <rahguzar@zohomail.eu>
Date: Fri, 12 May 2023 09:00:34 +0200
Subject: [PATCH] (Bug#61602) Tweak comint-mode redirection

* lisp/comint.el (redirection): New optional argument for
'comint-redirect-send-command-to-process' to explicitly
set the regexp to end redirection. Define 'comint-redirect-hook'
for easier discovery. Miscellaneous documentation fixes.
---
 lisp/comint.el | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index 682b555a33c..e6ce31f231c 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -161,7 +161,10 @@ comint-prompt-regexp
 Defaults to \"^\", the null string at BOL.
 
 This variable is only used if the variable
-`comint-use-prompt-regexp' is non-nil.
+`comint-use-prompt-regexp' is non-nil.  An exception to
+this is redirection.  Many commands including
+`comint-redirect-send-command-to-process' use `comint-prompt-regexp'
+to detect the end of redirection.
 
 Good choices:
   Canonical Lisp: \"^[^> \\n]*>+:? *\" (Lucid, franz, kcl, T, cscheme, oaklisp)
@@ -3637,7 +3640,13 @@ comint-redirect-output-buffer
 (defvar comint-redirect-finished-regexp nil
   "Regular expression that determines when to stop redirection in Comint.
 When the redirection filter function is given output that matches this regexp,
-the output is inserted as usual, and redirection is completed.")
+the output is inserted as usual, and redirection is completed.
+
+This is an internal variable set by `comint-redirect-setup' and setting it
+directly has no effect.")
+
+(defvar comint-redirect-hook nil
+  "Normal hook run when a redirection finishes.")
 
 (defvar comint-redirect-insert-matching-regexp nil
   "If non-nil, the text that ends a redirection is included in it.
@@ -3833,11 +3842,14 @@ comint-redirect-send-command
 
 ;;;###autoload
 (defun comint-redirect-send-command-to-process
-  (command output-buffer process echo &optional no-display)
+  (command output-buffer process echo &optional no-display finished-regexp)
   "Send COMMAND to PROCESS, with output to OUTPUT-BUFFER.
 With prefix arg, echo output in process buffer.
 
-If NO-DISPLAY is non-nil, do not show the output buffer."
+If NO-DISPLAY is non-nil, do not show the output buffer.
+FINISHED-REGEP is set as `comint-redirect-finished-regexp'
+when it is non-nil, otherwise `comint-prompt-regexp' is used
+as `comint-redirect-finished-regexp'."
   (interactive "sCommand: \nBOutput Buffer: \nbProcess Buffer: \nP")
   (let* (;; The process buffer
 	 (process-buffer (if (processp process)
@@ -3857,9 +3869,9 @@ comint-redirect-send-command-to-process
       ;; Set up for redirection
       (comint-redirect-setup
        output-buffer
-       (current-buffer)                 ; Comint Buffer
-       comint-prompt-regexp             ; Finished Regexp
-       echo)                            ; Echo input
+       (current-buffer)                          ; Comint Buffer
+       (or finished-regexp comint-prompt-regexp) ; Finished Regexp
+       echo)                                     ; Echo input
 
       ;; Set the filter.
       (add-function :around (process-filter proc) #'comint-redirect-filter)
-- 
2.40.1