Re: [BUG] comint-strip-ctrl-m doesn't function as documentation states

[Po Lu]

Jonathan <public@jds.work> writes:

> Hey folks,
>
> 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))=.
>
> #+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 brefity
>       )))
> #+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.
>
> 1. Update the documentation and leave as is. This is the simplest
> solution and would just require doc-string updates to indicate that
> =comint-strip-ctrl-m= is a "unique" filter function among the other
> filter functions that exist. This does not seem preferable to me.
>
> 2. Update the implementation of =comint-strip-ctrl-m= itself to
> conform it to the documented API. This would mean anything currently
> depending on it reading the =current-buffer= would break, and since
> there are plenty of unknowns in that regard, this also does not seem
> preferable.
>
> 3. Add a new version of the function with a different name that
> conforms to the documented API =comint-strip-ctrl-m-output= or
> something similar and deprecate the original.
>
> If we do decide to deprecate the original, I'm happy to include a
> deprecation warning and keep an eye on it popping up in core to ensure
> that we handle those issues over time.
>
> Any guidance would be useful. Thank you all for you're hard work.
>
> - Jonathan
>
> PS: Additional Context as promised:
>
> I was developing a package that runs SQL queries in a "hidden" SQLi
> buffer and so I needed to strip carriage return characters out of the
> output. Using this filter I had thought it would perform the task, but
> it did not. So digging through the documentation I discovered this
> error. I think it's pretty reasonable that filter functions conform to
> the documented api or should at least be noted otherwise.

Thanks.  Please direct bug reports to bug-gnu-emacs@gnu.org that the bug
tracker may record its progress: this list is for discussion that
directly concerns development only.