GNU bug report logs - #61602
29.0.60; comint-mode redirection

Previous Next

Full log

Message #20 received at 61602 <at> debbugs.gnu.org (full text, mbox):

From: Ruijie Yu <ruijie <at> netyu.xyz>
To: Rah Guzar <rahguzar <at> zohomail.eu>
Cc: 61602 <at> debbugs.gnu.org
Subject: Re: bug#61602: [PATCH]: comint-mode redirection
Date: Fri, 12 May 2023 10:16:48 +0800

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.

Further in-line comments below.

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

> Dear Emacs Maintainers,
>    A while back I sent a patch that addresses the points in this bug report. I
> have since received the confirmation that I have completed the copyright
> paperwork, so I am bringing it to your attention again. This is my first time
> contributing so please let me know if I should do somethings differently or if
> changes are needed.
>
> Thanks,
> Rah Guzar
>
> [2. text/x-patch; comint-redirect.patch]...

> diff --git a/lisp/comint.el b/lisp/comint.el
> index 682b555a33c..98f4d315d64 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.  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.

>  Good choices:
>    Canonical Lisp: \"^[^> \\n]*>+:? *\" (Lucid, franz, kcl, T, cscheme, oaklisp)
> @@ -3637,7 +3640,12 @@ 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.")

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.

> +
> +(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'.

>  (defvar comint-redirect-insert-matching-regexp nil
>    "If non-nil, the text that ends a redirection is included in it.
> @@ -3833,11 +3841,13 @@ 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.
> +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'.

>    (interactive "sCommand: \nBOutput Buffer: \nbProcess Buffer: \nP")
>    (let* (;; The process buffer
>  	 (process-buffer (if (processp process)
> @@ -3858,7 +3868,7 @@ comint-redirect-send-command-to-process
>        (comint-redirect-setup
>         output-buffer
>         (current-buffer)                 ; Comint Buffer
> -       comint-prompt-regexp             ; Finished Regexp
> +       (or finished-regexp comint-prompt-regexp)             ; Finished Regexp
>         echo)                            ; Echo input
>  
>        ;; Set the filter.

-- 
Best,


RY

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.