[BUG] Prompt appears in async shell results

[BUG] Prompt appears in async shell results

[Matt]

* [BUG] Prompt appears in async shell results
** Minimal reproducible example
#+begin_src emacs-lisp
(org-babel-do-load-languages 'org-babel-load-languages '((shell . t)))
#+end_src

#+begin_src sh :results output :session *test* :async t
cd /tmp
echo "hello world"
#+end_src

#+RESULTS:
: org_babel_sh_prompt> hello world

or

#+begin_src sh :results output :session *test* :async t
# comment
# comment
#+end_src

#+RESULTS:
: org_babel_sh_prompt> org_babel_sh_prompt>

or

#+begin_src sh :results output :session *test* :async t
# print message
echo \"hello world\"
#+end_src

#+RESULTS:
: org_babel_sh_prompt> "hello world"

Interestingly, this returns without the prompt:

,#+begin_src sh :results output :session *test* :async t
echo "hello"
echo "world"
#+end_src

#+RESULTS:
: hello
: world

** Test
Here's a test that checks one of the MREs:
#+begin_src emacs-lisp
(ert-deftest test-ob-shell/session-async-removes-prompt-from-results ()
  "Test that async evaluation removes prompt from results."
  (let* ((session-name "test-ob-shell/session-async-removes-prompt-from-results")
         (kill-buffer-query-functions nil)
         (start-time (current-time))
         (wait-time (time-add start-time 3))
         uuid-placeholder)
    (org-test-with-temp-text
        (concat "#+begin_src sh :session " session-name " :async t
# print message
echo \"hello world\"<point>
,#+end_src")
      (setq uuid-placeholder (org-trim (org-babel-execute-src-block)))
      (catch 'too-long
        (while (string-match uuid-placeholder (buffer-string))
          (progn
            (sleep-for 0.01)
            (when (time-less-p wait-time (current-time))
              (throw 'too-long (ert-fail "Took too long to get result from callback"))))))
    (search-forward "#+results")
    (beginning-of-line 2)
    (if (should (string= ": hello world\n" (buffer-substring-no-properties (point) (point-max))))
          (kill-buffer session-name)))))
#+end_src

** Thoughts
A quick fix is:

#+begin_src diff
modified   lisp/ob-shell.el
@@ -289,7 +289,7 @@ See `org-babel-comint-async-indicator'.")
 (defun ob-shell-async-chunk-callback (string)
   "Filter applied to results before insertion.
 See `org-babel-comint-async-chunk-callback'."
-  (replace-regexp-in-string comint-prompt-regexp "" string))
+  (replace-regexp-in-string org-babel-sh-prompt "" string))

 (defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
   "Pass BODY to the Shell process in BUFFER.
#+end_src

I'm not sure this is the best way.

There are two ways I can think to look at it: how text is passed to the process and what's done with it afterward.

Regarding how text is passed to the process:

Blocks without :session and :async are sent via =process-file=.  Blocks with :session, including those with :async, are inserted into a process buffer and sent to the process with =comint-send-input=.  The details differ, but I think the general concept holds: a chunk of text is inserted into the process buffer and that chunk is then sent to the process.  This is in contrast to successive insert-send pairs.  AFAICT, there's no major difference in how text is passed to the process between :session only and :session with :async.

Regarding how process results are handled:

AFAIU, =process-file= interfaces directly with the process and no terminal emulation is involved.  So, there's no prompt to worry about.  Blocks with only :session go through =org-babel-comint-with-output= which filters out the prompt (https://git.savannah.gnu.org/cgit/emacs/org-mode.git/tree/lisp/ob-comint.el#n110).  Async block results go through =org-babel-comint-async-filter= which doesn't filter results (https://git.savannah.gnu.org/cgit/emacs/org-mode.git/tree/lisp/ob-comint.el#n212).

It seems to me that we should extract the filter from =org-babel-comint-with-output= and use it in both =org-babel-comint-with-output= and =org-babel-comint-async-filter=.  

Thoughts?

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

Re: [BUG] Prompt appears in async shell results

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> #+begin_src emacs-lisp
> (org-babel-do-load-languages 'org-babel-load-languages '((shell . t)))
> #+end_src
>
> #+begin_src sh :results output :session *test* :async t
> cd /tmp
> echo "hello world"
> #+end_src
>
> #+RESULTS:
> : org_babel_sh_prompt> hello world

Confirmed.

> ** Thoughts
> A quick fix is:
>
> #+begin_src diff
> modified   lisp/ob-shell.el
> @@ -289,7 +289,7 @@ See `org-babel-comint-async-indicator'.")
>  (defun ob-shell-async-chunk-callback (string)
>    "Filter applied to results before insertion.
>  See `org-babel-comint-async-chunk-callback'."
> -  (replace-regexp-in-string comint-prompt-regexp "" string))
> +  (replace-regexp-in-string org-babel-sh-prompt "" string))
> ...
> I'm not sure this is the best way.

It is not. It works by accident.
Other edge cases may appear with chunks containing parts of the prompt.

> It seems to me that we should extract the filter from =org-babel-comint-with-output= and use it in both =org-babel-comint-with-output= and =org-babel-comint-async-filter=.  

Yes. The right fix would be extracting the filter from
`org-babel-comint-with-output' and re-using it.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Prompt appears in async shell results

[Matt]

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


 ---- On Mon, 05 Feb 2024 15:00:18 +0100  Ihor Radchenko  wrote --- 
 
 > Yes. The right fix would be extracting the filter from
 > `org-babel-comint-with-output' and re-using it.

This message documents my progress so that I may switch focus to Bruno's patches in "Asynchronous blocks for everything" attention (https://list.orgmode.org/65cfa0d8.050a0220.cb569.ce34@mx.google.com/T/#u).

Attached is a failed attempt at extracting the filter in =org-babel-comint-with-output=.  It tries to extract the filter more-or-less directly:

1. take the filter code from =org-babel-comint-with-output= and put it
into a separate function, =org-babel-comint-process-output-filter=

2. call =org-babel-comint-process-output-filter= from
=org-babel-comint-with-output= and =org-babel-comint-async-filter=

The unmodified =org-babel-comint-with-output= has a comment that says, "Filter out prompts".  This is misleading.  The filter code does two things: removes prompts *and* removes echoed input.

The problem is the filter which removes echoes uses the body of the source block.  It's unclear how to give =org-babel-comint-async-filter= the block body.  =org-babel-comint-async-filter= is a =comint-output-filter-function= which receives a single input, "a string containing the text as originally inserted" in the comint buffer.

Thoughts:

- Split prompt filtering and input echoing into two filters
  + this seems to imply a =-hook= or =-functions= type implementation
  + where could input echo filter go?  Where has access to the block body?
-  creating a generic prompt filter duplicates =ob-shell-async-chunk-callback= or, more fundamentally, =org-babel-comint-async-chunk-callback=
- What would it take to consolidate output filtering?  In addition to prompt filtering and input echo filtering, ob-shell filters the =org-babel-sh-eoe-indicator=.  I'm sure there's other filtering that happens on block output.  Wouldn't it be nice if that were in a single place, like right before results are inserted?

Please feel free to provide feedback and suggestions.

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

[-- Attachment #2: v01-refactor-filter.diff --]
[-- Type: application/octet-stream, Size: 5443 bytes --]

diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index 1ec84e865..fd80880e9 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -74,6 +74,35 @@ This is useful when prompt unexpectedly changes."
       (setq comint-prompt-regexp org-babel-comint-prompt-regexp-old
             org-babel-comint-prompt-regexp-old tmp))))
 
+(defun org-babel-comint-process-output-filter (from-string &optional to-remove remove-echo)
+  "Remove string FROM-STRING from TO-REMOVE.
+
+TO-REMOVE defaults to `comint-prompt-regexp'.  REMOVE-ECHO
+removes echoed commands some shells may print and defaults to t."
+  (let* ((to-remove (or to-remove comint-prompt-regexp))
+         (remove-echo (or remove-echo t)))
+    ;; remove TO-REMOVE from FROM-STRING
+    (while (string-match-p to-remove from-string)
+      (setq from-string
+            (replace-regexp-in-string
+             (format
+              "\\(?:%s\\)?\\(?:%s\\)[ \t]*"
+              "org-babel-comint-process-output-filter-separator\n"
+              to-remove)
+             "org-babel-comint-process-output-filter-separator\n"
+             from-string)))
+    ;; remove echo
+    (when (and remove-echo from-string ; <-- FIXME from-string needs to be block body
+               (string-match
+                (replace-regexp-in-string
+                 "\n" "[\r\n]+" (regexp-quote (or from-string "")))
+                from-string))
+      (setq from-string (substring from-string (match-end 0))))
+    (delete "" (split-string
+                from-string
+                "org-babel-comint-process-output-filter-separator\n"))
+    ))
+
 (defmacro org-babel-comint-with-output (meta &rest body)
   "Evaluate BODY in BUFFER and return process output.
 Will wait until EOE-INDICATOR appears in the output, then return
@@ -139,31 +168,35 @@ or user `keyboard-quit' during execution of body."
 	 (goto-char (process-mark (get-buffer-process (current-buffer))))
 	 (insert dangling-text)
 
-         ;; Filter out prompts.
-         (while (string-match-p comint-prompt-regexp string-buffer)
-           (setq string-buffer
-                 (replace-regexp-in-string
-                  ;; Sometimes, we get multiple agglomerated
-                  ;; prompts together in a single output:
-                  ;; "prompt prompt prompt output"
-                  ;; Or even "<whitespace>prompt<whitespace>prompt ...>.
-                  ;; Remove them progressively, so that
-                  ;; possible "^" in the prompt regexp gets to
-                  ;; work as we remove the heading prompt
-                  ;; instance.
-                  (format "\\(?:%s\\)?\\(?:%s\\)[ \t]*" ,org-babel-comint-prompt-separator comint-prompt-regexp)
-                  ,org-babel-comint-prompt-separator
-                  string-buffer)))
-	 ;; remove echo'd FULL-BODY from input
-	 (when (and ,remove-echo ,full-body
-		    (string-match
-		     (replace-regexp-in-string
-		      "\n" "[\r\n]+" (regexp-quote (or ,full-body "")))
-		     string-buffer))
-	   (setq string-buffer (substring string-buffer (match-end 0))))
-         (delete "" (split-string
-                     string-buffer
-                     ,org-babel-comint-prompt-separator))))))
+         (org-babel-comint-process-output-filter string-buffer)
+         
+         ;; ;; Filter out prompts.
+         ;; (while (string-match-p comint-prompt-regexp string-buffer)
+         ;;   (setq string-buffer
+         ;;         (replace-regexp-in-string
+         ;;          ;; Sometimes, we get multiple agglomerated
+         ;;          ;; prompts together in a single output:
+         ;;          ;; "prompt prompt prompt output"
+         ;;          ;; Or even "<whitespace>prompt<whitespace>prompt ...>.
+         ;;          ;; Remove them progressively, so that
+         ;;          ;; possible "^" in the prompt regexp gets to
+         ;;          ;; work as we remove the heading prompt
+         ;;          ;; instance.
+         ;;          (format "\\(?:%s\\)?\\(?:%s\\)[ \t]*" ,org-babel-comint-prompt-separator comint-prompt-regexp)
+         ;;          ,org-babel-comint-prompt-separator
+         ;;          string-buffer)))
+	 ;; ;; remove echo'd FULL-BODY from input
+	 ;; (when (and ,remove-echo ,full-body
+	 ;;            (string-match
+	 ;;             (replace-regexp-in-string
+	 ;;              "\n" "[\r\n]+" (regexp-quote (or ,full-body "")))
+	 ;;             string-buffer))
+	 ;;   (setq string-buffer (substring string-buffer (match-end 0))))
+         ;; (delete "" (split-string
+         ;;             string-buffer
+         ;;             ,org-babel-comint-prompt-separator))
+
+         ))))
 
 (defun org-babel-comint-input-command (buffer cmd)
   "Pass CMD to BUFFER.
@@ -324,8 +357,9 @@ STRING contains the output originally inserted into the comint buffer."
 		      until (and (equal (match-string 1) "start")
 				 (equal (match-string 2) uuid))
 		      finally return (+ 1 (match-end 0)))))
-		   ;; Apply callback to clean up the result
-		   (res-str (funcall org-babel-comint-async-chunk-callback
+                   ;; Apply filter to clean up the result
+                   (res-str ;(funcall org-babel-comint-async-chunk-callback
+                             (org-babel-comint-process-output-filter
                                      res-str-raw)))
 	      ;; Search for uuid in associated org-buffers to insert results
 	      (cl-loop for buf in org-buffers

Re: [BUG] Prompt appears in async shell results

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> - Split prompt filtering and input echoing into two filters
>   + this seems to imply a =-hook= or =-functions= type implementation

Not necessarily. You may just split the filter into (1) prompt
remover; (2) body remover. The filter does not need to interact with
comint buffer and may simply be provided a regexp/string to be removed.
Then, the caller will be responsible to supply prompt regexp/body.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Prompt appears in async shell results

[Matt]

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


 ---- On Mon, 19 Feb 2024 12:07:55 +0100  Ihor Radchenko  wrote ---
 > Matt matt@excalamus.com> writes:
 >
 > > - Split prompt filtering and input echoing into two filters
 > >   + this seems to imply a =-hook= or =-functions= type implementation
 >
 > Not necessarily. You may just split the filter into (1) prompt
 > remover; (2) body remover. The filter does not need to interact with
 > comint buffer and may simply be provided a regexp/string to be removed.
 > Then, the caller will be responsible to supply prompt regexp/body.

Attached are patches which split the prompt and echo filtering into separate functions.  These are then used to refactor =org-babel-comint-with-output= and to remove the prompt from async results.  I also wrote tests, one for the reported bug and two for the filter functions.  Tests for the filter functions are in a new file, 'test-ob-comint.el'.

Two thoughts:

1. Can people please test that the changes to =org-babel-comint-with-output= haven't broken other languages?

The filter, which I extracted from =org-babel-comint-with-output=, works according to the test I wrote.  The test is based on ob-shell output.  However, =org-babel-comint-with-output= is used by other languages.  I wasn't sure if echoes showed up for other languages and I had no examples from other languages to work from.  The filter is untested for anything but shell, aside from running 'make test' (which returns no unexpected errors).  If no echo argument is given, the filter simply returns the string it would otherwise try to remove the echo from.  So, I suspect the worst outcome would be that echoes might start showing up in output.  But when have my predictions ever been right? :)

2. Legacy code woes

I'll start another thread to rant, er, discuss.

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode


[-- Attachment #2: 0001-testing-lisp-test-ob-comint.el-Make-test-for-prompt-.patch --]
[-- Type: application/octet-stream, Size: 2871 bytes --]

From 66cc514eb27a9c50fb0128fa30e55b7609bb9b18 Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Sun, 17 Mar 2024 16:47:22 +0100
Subject: [PATCH 1/7] testing/lisp/test-ob-comint.el: Make test for prompt
 filter

* test-ob-comint.el:  Make new file for comint tests.
(test-org-babel-comint/prompt-filter-removes-prompt): Test that the
prompt is removed from process buffer output.
---
 testing/lisp/test-ob-comint.el | 55 ++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 testing/lisp/test-ob-comint.el

diff --git a/testing/lisp/test-ob-comint.el b/testing/lisp/test-ob-comint.el
new file mode 100644
index 000000000..9193ef8f3
--- /dev/null
+++ b/testing/lisp/test-ob-comint.el
@@ -0,0 +1,55 @@
+;;; test-ob-comint.el  -*- lexical-binding: t; -*-
+
+;; Copyright (c) 2024 Matthew Trzcinski
+;; Authors: Matthew Trzcinski
+
+;; This file is not part of GNU Emacs.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+\f
+;;; Comment:
+
+;; See testing/README for how to run tests.
+
+\f
+;;; Requirements:
+
+\f
+;;; Code:
+(ert-deftest test-org-babel-comint/prompt-filter-removes-prompt ()
+  "Test that prompt is actually removed."
+  (let* ((prompt "org_babel_sh_prompt> ")
+         (results "org_babel_sh_prompt> echo 'ob_comint_async_shell_start_d78ac49f-dc8a-4c39-827c-c93225484d59'
+# print message
+echo \"hello world\"
+echo 'ob_comint_async_shell_end_d78ac49f-dc8a-4c39-827c-c93225484d59'
+ob_comint_async_shell_start_d78ac49f-dc8a-4c39-827c-c93225484d59
+org_babel_sh_prompt> org_babel_sh_prompt> \"hello world\"
+org_babel_sh_prompt> ob_comint_async_shell_end_d78ac49f-dc8a-4c39-827c-c93225484d59
+org_babel_sh_prompt> "))
+    (should (string=
+             (org-trim (string-join (mapcar #'org-trim (org-babel-comint--prompt-filter results prompt)) "\n") "\n")
+             "echo 'ob_comint_async_shell_start_d78ac49f-dc8a-4c39-827c-c93225484d59'
+# print message
+echo \"hello world\"
+echo 'ob_comint_async_shell_end_d78ac49f-dc8a-4c39-827c-c93225484d59'
+ob_comint_async_shell_start_d78ac49f-dc8a-4c39-827c-c93225484d59
+\"hello world\"
+ob_comint_async_shell_end_d78ac49f-dc8a-4c39-827c-c93225484d59"))))
+
+
+(provide 'test-ob-comint)
+
+;;; test-ob-comint.el ends here
-- 
2.41.0


[-- Attachment #3: 0002-lisp-ob-comint.el-Create-comint-prompt-filter.patch --]
[-- Type: application/octet-stream, Size: 1536 bytes --]

From ed5f3e358f2e32c12d3487de9ab17797370eaf81 Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Sun, 17 Mar 2024 16:54:33 +0100
Subject: [PATCH 2/7] lisp/ob-comint.el: Create comint prompt filter

* lisp/ob-comint.el (org-babel-comint--prompt-filter): Extract prompt
filtering logic from `org-babel-comint-with-output' into a new
function.
---
 lisp/ob-comint.el | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index 1ec84e865..882e19289 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -74,6 +74,19 @@ This is useful when prompt unexpectedly changes."
       (setq comint-prompt-regexp org-babel-comint-prompt-regexp-old
             org-babel-comint-prompt-regexp-old tmp))))
 
+(defun org-babel-comint--prompt-filter (string &optional prompt-regexp)
+  "Remove PROMPT-REGEXP from STRING.
+
+PROMPT-REGEXP defaults to `comint-prompt-regexp'."
+  (let* ((prompt-regexp (or prompt-regexp comint-prompt-regexp))
+         (separator "org-babel-comint--prompt-filter-separator\n"))
+    (while (string-match-p prompt-regexp string)
+      (setq string
+            (replace-regexp-in-string
+             (format "\\(?:%s\\)?\\(?:%s\\)[ \t]*" separator prompt-regexp)
+             separator string)))
+    (delete "" (split-string string separator))))
+
 (defmacro org-babel-comint-with-output (meta &rest body)
   "Evaluate BODY in BUFFER and return process output.
 Will wait until EOE-INDICATOR appears in the output, then return
-- 
2.41.0


[-- Attachment #4: 0003-testing-lisp-test-ob-comint.el-Make-test-for-echo-fi.patch --]
[-- Type: application/octet-stream, Size: 1962 bytes --]

From 44bccab228b9dee80672b3a3dd9d27b2cc26c4b3 Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Sun, 17 Mar 2024 16:56:43 +0100
Subject: [PATCH 3/7] testing/lisp/test-ob-comint.el: Make test for echo filter

* test-ob-comint.el:
(test-org-babel-comint/echo-filter-removes-echo): Test that echoed
input is removed from process buffer output.
---
 testing/lisp/test-ob-comint.el | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/testing/lisp/test-ob-comint.el b/testing/lisp/test-ob-comint.el
index 9193ef8f3..8f6def8ca 100644
--- a/testing/lisp/test-ob-comint.el
+++ b/testing/lisp/test-ob-comint.el
@@ -49,6 +49,26 @@ ob_comint_async_shell_start_d78ac49f-dc8a-4c39-827c-c93225484d59
 \"hello world\"
 ob_comint_async_shell_end_d78ac49f-dc8a-4c39-827c-c93225484d59"))))
 
+(ert-deftest test-org-babel-comint/echo-filter-removes-echo ()
+  "Test that echo is actually removed."
+  (let* ((echo "echo 'ob_comint_async_shell_start_d78ac49f-dc8a-4c39-827c-c93225484d59'
+# print message
+echo \"hello world\"
+echo 'ob_comint_async_shell_end_d78ac49f-dc8a-4c39-827c-c93225484d59'")
+         (result "org_babel_sh_prompt> echo 'ob_comint_async_shell_start_d78ac49f-dc8a-4c39-827c-c93225484d59'
+# print message
+echo \"hello world\"
+echo 'ob_comint_async_shell_end_d78ac49f-dc8a-4c39-827c-c93225484d59'
+ob_comint_async_shell_start_d78ac49f-dc8a-4c39-827c-c93225484d59
+org_babel_sh_prompt> org_babel_sh_prompt> \"hello world\"
+org_babel_sh_prompt> ob_comint_async_shell_end_d78ac49f-dc8a-4c39-827c-c93225484d59
+org_babel_sh_prompt> "))
+    (should (string=
+             (org-babel-comint--echo-filter result echo)
+             "\nob_comint_async_shell_start_d78ac49f-dc8a-4c39-827c-c93225484d59
+org_babel_sh_prompt> org_babel_sh_prompt> \"hello world\"
+org_babel_sh_prompt> ob_comint_async_shell_end_d78ac49f-dc8a-4c39-827c-c93225484d59
+org_babel_sh_prompt> "))))
 
 (provide 'test-ob-comint)
 
-- 
2.41.0


[-- Attachment #5: 0004-lisp-ob-comint.el-Create-comint-echo-filter.patch --]
[-- Type: application/octet-stream, Size: 1203 bytes --]

From c629cb7041d25480cba82812ba5da41329f4e435 Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Sun, 17 Mar 2024 16:58:52 +0100
Subject: [PATCH 4/7] lisp/ob-comint.el: Create comint echo filter

* lisp/ob-comint.el (org-babel-comint--echo-filter): Extract echo
filtering logic from `org-babel-comint-with-output' into a new
function.
---
 lisp/ob-comint.el | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index 882e19289..e8d8e7609 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -87,6 +87,15 @@ PROMPT-REGEXP defaults to `comint-prompt-regexp'."
              separator string)))
     (delete "" (split-string string separator))))
 
+(defun org-babel-comint--echo-filter (string &optional echo)
+  "Remove ECHO from STRING."
+  (and echo string
+       (string-match
+        (replace-regexp-in-string "\n" "[\r\n]+" (regexp-quote echo))
+        string)
+       (setq string (substring string (match-end 0))))
+  string)
+
 (defmacro org-babel-comint-with-output (meta &rest body)
   "Evaluate BODY in BUFFER and return process output.
 Will wait until EOE-INDICATOR appears in the output, then return
-- 
2.41.0


[-- Attachment #6: 0005-lisp-ob-comint.el-Refactor-org-babel-comint-with-out.patch --]
[-- Type: application/octet-stream, Size: 3632 bytes --]

From f9821b7ff50aeae096be2ebb55f6d920c0afa1e0 Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Sun, 17 Mar 2024 18:24:43 +0100
Subject: [PATCH 5/7] lisp/ob-comint.el: Refactor
 `org-babel-comint-with-output'

* lisp/ob-comint.el (org-babel-comint-with-output): Replace logic for
prompt and echo filtering with `org-babel-comint--prompt-filter' and
`org-babel-comint--echo-filter'.  Delete
`org-babel-comint-prompt-separator' variable and move related comment
to `org-babel-comint--prompt-filter'.
---
 lisp/ob-comint.el | 39 +++++++++------------------------------
 1 file changed, 9 insertions(+), 30 deletions(-)

diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index e8d8e7609..d13aacccc 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -79,6 +79,9 @@ This is useful when prompt unexpectedly changes."
 
 PROMPT-REGEXP defaults to `comint-prompt-regexp'."
   (let* ((prompt-regexp (or prompt-regexp comint-prompt-regexp))
+         ;; We need newline in case if we do progressive replacement
+         ;; of agglomerated comint prompts with `comint-prompt-regexp'
+         ;; containing ^.
          (separator "org-babel-comint--prompt-filter-separator\n"))
     (while (string-match-p prompt-regexp string)
       (setq string
@@ -112,12 +115,7 @@ or user `keyboard-quit' during execution of body."
   (let ((buffer (nth 0 meta))
 	(eoe-indicator (nth 1 meta))
 	(remove-echo (nth 2 meta))
-	(full-body (nth 3 meta))
-        (org-babel-comint-prompt-separator
-         ;; We need newline in case if we do progressive replacement
-         ;; of agglomerated comint prompts with `comint-prompt-regexp'
-         ;; containing ^.
-         "org-babel-comint-prompt-separator\n"))
+	(full-body (nth 3 meta)))
     `(org-babel-comint-in-buffer ,buffer
        (let* ((string-buffer "")
 	      (comint-output-filter-functions
@@ -161,31 +159,12 @@ or user `keyboard-quit' during execution of body."
 	 (goto-char (process-mark (get-buffer-process (current-buffer))))
 	 (insert dangling-text)
 
+         ;; remove echo'd FULL-BODY from input
+         (and ,remove-echo ,full-body
+              (setq string-buffer (org-babel-comint--echo-filter string-buffer ,full-body)))
+
          ;; Filter out prompts.
-         (while (string-match-p comint-prompt-regexp string-buffer)
-           (setq string-buffer
-                 (replace-regexp-in-string
-                  ;; Sometimes, we get multiple agglomerated
-                  ;; prompts together in a single output:
-                  ;; "prompt prompt prompt output"
-                  ;; Or even "<whitespace>prompt<whitespace>prompt ...>.
-                  ;; Remove them progressively, so that
-                  ;; possible "^" in the prompt regexp gets to
-                  ;; work as we remove the heading prompt
-                  ;; instance.
-                  (format "\\(?:%s\\)?\\(?:%s\\)[ \t]*" ,org-babel-comint-prompt-separator comint-prompt-regexp)
-                  ,org-babel-comint-prompt-separator
-                  string-buffer)))
-	 ;; remove echo'd FULL-BODY from input
-	 (when (and ,remove-echo ,full-body
-		    (string-match
-		     (replace-regexp-in-string
-		      "\n" "[\r\n]+" (regexp-quote (or ,full-body "")))
-		     string-buffer))
-	   (setq string-buffer (substring string-buffer (match-end 0))))
-         (delete "" (split-string
-                     string-buffer
-                     ,org-babel-comint-prompt-separator))))))
+         (org-babel-comint--prompt-filter string-buffer)))))
 
 (defun org-babel-comint-input-command (buffer cmd)
   "Pass CMD to BUFFER.
-- 
2.41.0


[-- Attachment #7: 0006-testing-lisp-test-ob-shell.el-Test-async-prompt-remo.patch --]
[-- Type: application/octet-stream, Size: 2036 bytes --]

From 4f943d1e72c1e32f02723a53d4d8f5ee201dfbbb Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Sun, 17 Mar 2024 18:27:11 +0100
Subject: [PATCH 6/7] testing/lisp/test-ob-shell.el: Test async prompt removal

* testing/lisp/test-ob-shell.el (test-ob-shell/session-async-results):
Create test verifying bug report that shell prompt appears in async
results.
---
 testing/lisp/test-ob-shell.el | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el
index 879555af0..8cebd8467 100644
--- a/testing/lisp/test-ob-shell.el
+++ b/testing/lisp/test-ob-shell.el
@@ -129,6 +129,30 @@ echo 2<point>
     (if (should (string= ": 1\n: 2\n" (buffer-substring-no-properties (point) (point-max))))
           (kill-buffer session-name)))))
 
+(ert-deftest test-ob-shell/session-async-results ()
+  "Test that async evaluation removes prompt from results."
+  (let* ((session-name "test-ob-shell/session-async-results")
+         (kill-buffer-query-functions nil)
+         (start-time (current-time))
+         (wait-time (time-add start-time 3))
+         uuid-placeholder)
+    (org-test-with-temp-text
+     (concat "#+begin_src sh :session " session-name " :async t
+# print message
+echo \"hello world\"<point>
+#+end_src")
+     (setq uuid-placeholder (org-trim (org-babel-execute-src-block)))
+     (catch 'too-long
+       (while (string-match uuid-placeholder (buffer-string))
+         (progn
+           (sleep-for 0.01)
+           (when (time-less-p wait-time (current-time))
+             (throw 'too-long (ert-fail "Took too long to get result from callback"))))))
+     (search-forward "#+results")
+     (beginning-of-line 2)
+     (if (should (string= ": hello world\n" (buffer-substring-no-properties (point) (point-max))))
+         (kill-buffer session-name)))))
+
 (ert-deftest test-ob-shell/generic-uses-no-arrays ()
   "Test generic serialization of array into a single string."
   (org-test-with-temp-text
-- 
2.41.0


[-- Attachment #8: 0007-lisp-ob-comint.el-Fix-prompt-appearing-in-async-shel.patch --]
[-- Type: application/octet-stream, Size: 1546 bytes --]

From abe14703390e9ae56829a118c3d4e488443f6845 Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Sun, 17 Mar 2024 19:04:39 +0100
Subject: [PATCH 7/7] lisp/ob-comint.el: Fix prompt appearing in async shell
 results

* lisp/ob-comint.el (org-babel-comint-async-filter): Call prompt
`org-babel-comint--prompt-filter'

Reported-by: "Matthew Trzcinski" <matt@excalamus.com>
Link: https://list.orgmode.org/18d753c1e8a.cfb3e1921191837.5665565128507976741@excalamus.com/
---
 lisp/ob-comint.el | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index d13aacccc..f2251892a 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -325,9 +325,10 @@ STRING contains the output originally inserted into the comint buffer."
 		      until (and (equal (match-string 1) "start")
 				 (equal (match-string 2) uuid))
 		      finally return (+ 1 (match-end 0)))))
-		   ;; Apply callback to clean up the result
-		   (res-str (funcall org-babel-comint-async-chunk-callback
-                                     res-str-raw)))
+                   ;; Remove prompt
+                   (res-promptless (org-trim (string-join (mapcar #'org-trim (org-babel-comint--prompt-filter res-str-raw)) "\n") "\n"))
+		   ;; Apply user callback
+		   (res-str (funcall org-babel-comint-async-chunk-callback res-promptless)))
 	      ;; Search for uuid in associated org-buffers to insert results
 	      (cl-loop for buf in org-buffers
 		       until (with-current-buffer buf
-- 
2.41.0

bug? org-babel-comint-with-output return value type

[Matt]

Looking at the last patch of https://list.orgmode.org/18e4deaa1bd.ff0f87fa694858.5955779335024266818@excalamus.com/, I would expect a reaction of something like, "WTF is all that weird org-trim string-join stuff on the prompt filter?"

---
 lisp/ob-comint.el | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index d13aacccc..f2251892a 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -325,9 +325,10 @@ STRING contains the output originally inserted into the comint buffer."
 		      until (and (equal (match-string 1) "start")
 				 (equal (match-string 2) uuid))
 		      finally return (+ 1 (match-end 0)))))
-		   ;; Apply callback to clean up the result
-		   (res-str (funcall org-babel-comint-async-chunk-callback
-                                     res-str-raw)))
+                   ;; Remove prompt
+                   (res-promptless (org-trim (string-join (mapcar #'org-trim (org-babel-comint--prompt-filter res-str-raw)) "\n") "\n"))
+		   ;; Apply user callback
+		   (res-str (funcall org-babel-comint-async-chunk-callback res-promptless)))
 	      ;; Search for uuid in associated org-buffers to insert results
 	      (cl-loop for buf in org-buffers
 		       until (with-current-buffer buf
-- 
2.41.0

The reason is that the current result of =org-babel-comint-with-output= is a list because the final expression is =delete=: https://git.savannah.gnu.org/cgit/emacs/org-mode.git/tree/lisp/ob-comint.el?id=46909a54e1a2ce0d948e94e0e19ff64af1a39eb9#n164

Why =org-babel-comint-with-output= returns a list is unclear to me.  The docstring for =org-babel-comint-with-output= says it should "return all process output".  AFAIK, process output is a string, always has been, always will be, and Babel has always gotten a substring of the process buffer (for that code path).  Yet for some reason, it was decided to have =org-babel-comint-with-output= return a list.  This goes back to the beginning, in 2009, when the final expression used =split-string= instead of =delete=: https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/lisp/org-babel-comint.el?id=dd0392a4f23b40fae7491c55aa44a2324248c103

You might ask, "But wouldn't returning a list require you to then convert it back into a string since =org-babel-comint-with-output= is really just a sentinel on process output?"  The answer is, "Yes, that's exactly what you'd need to do."

Call grep on =org-babel-comint-with-output= and you'll see in every case except the Ruby ':results value' results and Lua the ':results value' results, that indeed the return value of =org-babel-comint-with-output= is concatenated or the first string element taken.

| file          | line | result modified?         | result used? | comment                                                                      |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-ruby.el    |  263 | no                       | no           | used to send eoe-string                                                      |
|               |  272 | yes, mapconcat           | yes          | as returned value (for :results output)                                      |
|               |  281 | no                       | yes          | as returned value (for :results value)                                       |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-js.el      |  111 | yes, nth 1               | yes          | as returned value (for the default :results)                                 |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-shell.el   |  357 | yes, mapconcat           | yes          | as returned value (for session results of all types)                         |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-julia.el   |  327 | yes,  mapconcat          | yes          | as returned value (for :results output)                                      |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-lua.el     |  391 | yes, mapconcat           | yes          | as returned value (for :results output)                                      |
|               |  400 | no                       | yes          | as returned value (for :results value)                                       |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-haskell.el |  169 | yes, (cdr (reverse ...)) | yes          | as returned value? (for :results output) hard to tell, see lines 194 and 201 |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-octave.el  |  239 | yes, (cdr (reverse ...)) | yes          | as returned value (for :results output)                                      |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-ocaml.el   |   72 | yes, whoa...             | yes          | as returned value (for at least :results output)                             |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-R          |  460 | yes, mapconcat           | yes          | as returned value (for :results output)                                      |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|

It looks like the original implementation aggregated process buffer output, collecting it into a list.  It then passed this list on to a function that did, as predicted, (cdr (reverse ...)) to get a string.  https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=dd0392a4f23b40fae7491c55aa44a2324248c103

It seems like the return type of =org-babel-comint-with-output= is not what it should be.  It looks like it should be a string.

Am I missing something?

Obviously, changing the return type would be a big job, requiring updates to at least 10 files, and has the potential to introduce bugs.  However, as far as I can tell, the return type isn't appropriate and so requires some kind of workaround, of which there are currently three.  I guess the best argument I can come up with at the moment for taking on the task of changing the return type is that, assuming my understanding is correct, the current implementation is simply an inappropriate, or outdated, design choice that only adds complexity.  My hope is that Babel continues to support new languages.  As that happens, they will need to apply work arounds, further entrenching the design and making it harder to correct.

What are people's thoughts on changing the return type of =org-babel-comint-with-output=?

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

Re: bug? org-babel-comint-with-output return value type

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> Why =org-babel-comint-with-output= returns a list is unclear to me.
> The docstring for =org-babel-comint-with-output= says it should
> "return all process output". AFAIK, process output is a string, always
> has been, always will be, and Babel has always gotten a substring of
> the process buffer (for that code path). Yet for some reason, it was
> decided to have =org-babel-comint-with-output= return a list. This
> goes back to the beginning, in 2009, when the final expression used
> =split-string= instead of =delete=:
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/lisp/org-babel-comint.el?id=dd0392a4f23b40fae7491c55aa44a2324248c103
>
> ...
> It seems like the return type of =org-babel-comint-with-output= is not what it should be.  It looks like it should be a string.
>
> Am I missing something?

Inside `org-babel-comint-with-output', BODY may send multiple commands
to the shell; one by one. Their output will also arrive one by one,
separated by PROMPT. The return value is a list of these outputs.

If we were to concatenate this list, the information about which part of
the output belongs to which submitted command would be lost.

The docstring may indeed be improved.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Prompt appears in async shell results

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> 1. Can people please test that the changes to
> =org-babel-comint-with-output= haven't broken other languages?

Over a week have passed. No objections have been raised.
Feel free to apply the patches.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Prompt appears in async shell results

[Matt]

 ---- On Wed, 27 Mar 2024 11:59:06 +0100  Ihor Radchenko  wrote --- 

 > Over a week have passed. No objections have been raised.
 > Feel free to apply the patches.

Done.  See: https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=e9c288dfaccc2960e5b6889e6aabea700ad4e05a and parents.

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

Re: [BUG] Prompt appears in async shell results

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

>  ---- On Wed, 27 Mar 2024 11:59:06 +0100  Ihor Radchenko  wrote --- 
>
>  > Over a week have passed. No objections have been raised.
>  > Feel free to apply the patches.
>
> Done.  See: https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=e9c288dfaccc2960e5b6889e6aabea700ad4e05a and parents.

Fixed.
(this is a message to tracker)

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>