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.