Re: [BUG] Prompt appears in async shell results

[Ihor Radchenko]

Matt  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 .
Support Org development at ,
or support my work at 

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  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 .
Support Org development at ,
or support my work at 

Re: [BUG] Prompt appears in async shell results

[Matt]

  On Sat, 23 Mar 2024 15:16:37 +0100  Ihor Radchenko  wrote --- 
 
 > Yup. Tests are passing on my side as well, and I had no comments.
 > So, we are waiting for other interested users to test.
 > For at least a week, so that people who check the mailing list on
 > weekend have a chance to see the request.

Cool.

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

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

Yup. Tests are passing on my side as well, and I had no comments.
So, we are waiting for other interested users to test.
For at least a week, so that people who check the mailing list on
weekend have a chance to see the request.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: [BUG] Prompt appears in async shell results

[Matt]

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 m...@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



0001-testing-lisp-test-ob-comint.el-Make-test-for-prompt-.patch
Description: Binary data


0002-lisp-ob-comint.el-Create-comint-prompt-filter.patch
Description: Binary data


0003-testing-lisp-test-ob-comint.el-Make-test-for-echo-fi.patch
Description: Binary data


0004-lisp-ob-comint.el-Create-comint-echo-filter.patch
Description: Binary data


0005-lisp-ob-comint.el-Refactor-org-babel-comint-with-out.patch
Description: Binary data


0006-testing-lisp-test-ob-shell.el-Test-async-prompt-remo.patch
Description: Binary data


0007-lisp-ob-comint.el-Fix-prompt-appearing-in-async-shel.patch
Description: Binary data

Re: [BUG] Prompt appears in async shell results

[Matt]

  On Mon, 19 Feb 2024 12:07:55 +0100  Ihor Radchenko  wrote ---
 > Matt m...@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



0001-testing-lisp-test-ob-comint.el-Make-test-for-prompt-.patch
Description: Binary data


0002-lisp-ob-comint.el-Create-comint-prompt-filter.patch
Description: Binary data


0003-testing-lisp-test-ob-comint.el-Make-test-for-echo-fi.patch
Description: Binary data


0004-lisp-ob-comint.el-Create-comint-echo-filter.patch
Description: Binary data


0005-lisp-ob-comint.el-Refactor-org-babel-comint-with-out.patch
Description: Binary data


0006-testing-lisp-test-ob-shell.el-Test-async-prompt-remo.patch
Description: Binary data


0007-lisp-ob-comint.el-Fix-prompt-appearing-in-async-shel.patch
Description: Binary data

Re: [BUG] Prompt appears in async shell results

[Ihor Radchenko]

Matt  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 .
Support Org development at ,
or support my work at 

Re: [BUG] Prompt appears in async shell results

[Matt]

  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.c...@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


v01-refactor-filter.diff
Description: Binary data

Re: [BUG] Prompt appears in async shell results

[Ihor Radchenko]

Matt  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 .
Support Org development at ,
or support my work at 

[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\"
,#+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