bug#69914: comint-strip-ctrl-m doesn't function as documentation states

[Jonathan]

My apologies. This completely dropped off my radar as a few life events took 
precedence over the past few months and took me away from this. Things are more 
settled now though.

I do agree this appears to be a common trend among the comint filter functions. 
I will get together a patch including a new version of =comint-strip-ctrl-m= 
named something different of course, and updating the documentation that I can 
track down.

I do just anticipate this to take a little time as this would be my first 
contribution to the project and I'm still learning my way around. If all this 
is amenable to you I'll move forward with my solution and get a patch sent in 
soon. 


On Thursday, April 18th, 2024 at 4:01 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> 
> 
> Ping! Ping! Any interest in pursuing this issue further? If so,
> could you please answer my questions below?
> 
> > Cc: 69914@debbugs.gnu.org
> > Date: Sat, 06 Apr 2024 11:58:51 +0300
> > From: Eli Zaretskii eliz@gnu.org
> > 
> > Ping! Could you please answer my questions below?
> > 
> > > Cc: 69914@debbugs.gnu.org
> > > Date: Sat, 23 Mar 2024 09:30:16 +0200
> > > From: Eli Zaretskii eliz@gnu.org
> > > 
> > > > Date: Wed, 20 Mar 2024 14:15:39 +0000
> > > > From: Jonathan via "Bug reports for GNU Emacs,
> > > > the Swiss army knife of text editors" bug-gnu-emacs@gnu.org
> > > > 
> > > > There appears to either be a bug or just inaccurate documentation of 
> > > > =comint-strip-ctrl-m=. At the very bottom, I've included some context 
> > > > about my use case by which I discovered this bug that may or may not be 
> > > > relevant to you. The documentation for that function states:
> > > > 
> > > > #+begin_quote
> > > > Strip trailing ^M characters from the current output group.
> > > > 
> > > > This function could be on comint-output-filter-functions or bound to a 
> > > > key.
> > > > #+end_quote
> > > > 
> > > > =comint-output-filter-functions= states the following:
> > > > 
> > > > #+begin_quote
> > > > ...These functions get one argument, a string containing the text as 
> > > > originally
> > > > inserted. Note that this might not be the same as the buffer contents 
> > > > between
> > > > comint-last-output-start and the buffer's process-mark, if other filter
> > > > functions have already modified the buffer.
> > > > #+end_quote
> > > > 
> > > > Looking at the implementation of =comint-strip-ctrl-m= it appears that 
> > > > it completely ignores the =string= argument and instead uses 
> > > > =(get-buffer-process (current-buffer))= in direct contradiction to the 
> > > > documentation.
> > > 
> > > Actually, AFAICT, almost all of the filtering functions intended for
> > > comint-output-filter-functions ignore its string argument. Isn't that
> > > so?
> > > 
> > > > #+begin_src emacs-lisp
> > > > (defun comint-strip-ctrl-m (&optional _string interactive)
> > > > "Strip trailing `^M' characters from the current output group. This 
> > > > function could be on` comint-output-filter-functions' or bound to a 
> > > > key."
> > > > (interactive (list nil t))
> > > > (let ((process (get-buffer-process (current-buffer))))
> > > > (if (not process)
> > > > ;; This function may be used in
> > > > ;; `comint-output-filter-functions', and in that case, if
> > > > ;; there's no process, then we should do nothing. If
> > > > ;; interactive, report an error.
> > > > (when interactive
> > > > (error "No process in the current buffer"))
> > > > ;;; rest omitted for brevity
> > > > )))
> > > > #+end_src
> > > > 
> > > > This represents unexpected and undocumented behavior, as you anticipate 
> > > > =comint-strip-ctrl-m= to behave like any other comint output filter 
> > > > functions. I'd like to propose 3 different possible solutions for a 
> > > > patch and would like input on which is preferred as this code was 
> > > > originally introduced in 1994. I can submit a patch once a solution has 
> > > > been determined.
> > > 
> > > Given that almost all the filter functions behave the same (unless you
> > > disagree), it sounds like ignoring the string is a de-facto standard
> > > behavior. So we should document that, and I guess adding a new
> > > function, without deprecating the existing one, is the most reasonable
> > > way ahead?