emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [BUG] Prompt appears in async shell results
@ 2024-02-04 17:48 Matt
  2024-02-05 14:02 ` Ihor Radchenko
  0 siblings, 1 reply; 4+ messages in thread
From: Matt @ 2024-02-04 17:48 UTC (permalink / raw)
  To: emacs-orgmode

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



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [BUG] Prompt appears in async shell results
  2024-02-04 17:48 [BUG] Prompt appears in async shell results Matt
@ 2024-02-05 14:02 ` Ihor Radchenko
  2024-02-18 12:09   ` Matt
  0 siblings, 1 reply; 4+ messages in thread
From: Ihor Radchenko @ 2024-02-05 14:02 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [BUG] Prompt appears in async shell results
  2024-02-05 14:02 ` Ihor Radchenko
@ 2024-02-18 12:09   ` Matt
  2024-02-19 11:10     ` Ihor Radchenko
  0 siblings, 1 reply; 4+ messages in thread
From: Matt @ 2024-02-18 12:09 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [BUG] Prompt appears in async shell results
  2024-02-18 12:09   ` Matt
@ 2024-02-19 11:10     ` Ihor Radchenko
  0 siblings, 0 replies; 4+ messages in thread
From: Ihor Radchenko @ 2024-02-19 11:10 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2024-02-19 11:08 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-04 17:48 [BUG] Prompt appears in async shell results Matt
2024-02-05 14:02 ` Ihor Radchenko
2024-02-18 12:09   ` Matt
2024-02-19 11:10     ` Ihor Radchenko

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).