Re: [BUG] Prompt appears in async shell results

[Matt <matt@excalamus.com>]

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

Friendly bump.

I saw no comments on the patch part of my previous message and fear it may have been missed due to my inclusion of the lengthy and separate topic, "bug? org-babel-comint-with-output return value type".

The patches fix the prompt appearing in async shell results and are ready to go, barring the concern I have about being unable to test how the fix may affect non-shell languages.

>  ---- 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? :)

--
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