* bug#55590: 29.0.50; Eshell subcommands clobber pipelines and produce incorrect output
@ 2022-05-23 3:34 Jim Porter
2022-05-23 3:43 ` bug#55590: [PATCH] " Jim Porter
0 siblings, 1 reply; 6+ messages in thread
From: Jim Porter @ 2022-05-23 3:34 UTC (permalink / raw)
To: 55590
Starting from `emacs -Q -f eshell', enter the following command:
echo ${*echo hi} | rev
The output is:
~ $ ih
That is, the output of the command ended up *after* the prompt, when it
should be before. This usually happens in cases where Eshell thinks all
the subprocesses in a command have finished, but there's actually one
still running. In this case, it's because the ${SUBCOMMAND} clobbers the
headproc/tailproc values being recorded for the (outer) pipeline. Both
the top-level command and the subcommand call `eshell-execute-pipeline',
and the function isn't properly reentrant.
Patch forthcoming momentarily. I'm just getting a bug number first.
^ permalink raw reply [flat|nested] 6+ messages in thread* bug#55590: [PATCH] 29.0.50; Eshell subcommands clobber pipelines and produce incorrect output 2022-05-23 3:34 bug#55590: 29.0.50; Eshell subcommands clobber pipelines and produce incorrect output Jim Porter @ 2022-05-23 3:43 ` Jim Porter 2022-05-23 5:39 ` Sean Whitton 0 siblings, 1 reply; 6+ messages in thread From: Jim Porter @ 2022-05-23 3:43 UTC (permalink / raw) To: 55590 [-- Attachment #1: Type: text/plain, Size: 1176 bytes --] On 5/22/2022 8:34 PM, Jim Porter wrote: > Starting from `emacs -Q -f eshell', enter the following command: > > echo ${*echo hi} | rev > > The output is: > > ~ $ ih > > That is, the output of the command ended up *after* the prompt, when it > should be before. The attached patch fixes this issue. It might not be the *best* way to do it, but it's the least-invasive I could come up with. The `make-symbol' dance is a bit awkward, but Eshell already uses it for `eshell-command-to-value' / `eshell-do-command-to-value', so hopefully it's ok. I'm open to other solutions though. It's possible the bug is really in `eshell-do-eval', since I'd expect the original code to work, but `eshell-do-eval' is pretty tricky, and I don't want to poke at it too hard for fear that other things will break. Long-term, the best way to fix this might be to rip out `eshell-do-eval' entirely, which iteratively evaluates parts of Eshell commands (so as not to hang Emacs) and use the generator.el machinery instead. I looked into that briefly, and it seems like it would be quite a bit of work. And I don't fully understand generator.el's implementation yet anyway... [-- Attachment #2: 0001-Keep-subcommands-in-pipelines-from-clobbering-the-he.patch --] [-- Type: text/plain, Size: 3380 bytes --] From 95d6bcc370b098b524ffde25101f684d327a7584 Mon Sep 17 00:00:00 2001 From: Jim Porter <jporterbugs@gmail.com> Date: Sun, 22 May 2022 17:27:48 -0700 Subject: [PATCH] Keep subcommands in pipelines from clobbering the head/tail processes * lisp/eshell/esh-cmd.el (eshell-execute-pipeline): Use 'make-symbol' for headproc and tailproc. (eshell-do-pipelines, eshell-do-pipelines-synchronously): Adapt to the above. * test/lisp/eshell/eshell-tests.el (eshell-test/pipe-subcommand): New test (bug#55590). --- lisp/eshell/esh-cmd.el | 15 ++++++++++----- test/lisp/eshell/eshell-tests.el | 8 ++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el index 42616e7037..73c250632c 100644 --- a/lisp/eshell/esh-cmd.el +++ b/lisp/eshell/esh-cmd.el @@ -827,8 +827,8 @@ eshell-do-pipelines ((cdr pipeline) t) (t (quote 'last))))) (let ((proc ,(car pipeline))) - (setq headproc (or proc headproc)) - (setq tailproc (or tailproc proc)) + (set headproc (or proc (symbol-value headproc))) + (set tailproc (or (symbol-value tailproc) proc)) proc)))))) (defmacro eshell-do-pipelines-synchronously (pipeline) @@ -861,7 +861,7 @@ eshell-do-pipelines-synchronously (let ((result ,(car pipeline))) ;; tailproc gets the result of the last successful process in ;; the pipeline. - (setq tailproc (or result tailproc)) + (set tailproc (or result (symbol-value tailproc))) ,(if (cdr pipeline) `(eshell-do-pipelines-synchronously (quote ,(cdr pipeline)))) result)))) @@ -870,7 +870,11 @@ 'eshell-process-identity (defmacro eshell-execute-pipeline (pipeline) "Execute the commands in PIPELINE, connecting each to one another." - `(let ((eshell-in-pipeline-p t) headproc tailproc) + `(let ((eshell-in-pipeline-p t) + (headproc (make-symbol "headproc")) + (tailproc (make-symbol "tailproc"))) + (set headproc nil) + (set tailproc nil) (progn ,(if (fboundp 'make-process) `(eshell-do-pipelines ,pipeline) @@ -880,7 +884,8 @@ eshell-execute-pipeline (car (aref eshell-current-handles ,eshell-error-handle)) nil))) (eshell-do-pipelines-synchronously ,pipeline))) - (eshell-process-identity (cons headproc tailproc))))) + (eshell-process-identity (cons (symbol-value headproc) + (symbol-value tailproc)))))) (defmacro eshell-as-subcommand (command) "Execute COMMAND using a temp buffer. diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el index 7cdeb017e4..dcb703c73f 100644 --- a/test/lisp/eshell/eshell-tests.el +++ b/test/lisp/eshell/eshell-tests.el @@ -114,6 +114,14 @@ eshell-test/pipe-headproc-stdin (eshell-wait-for-subprocess) (eshell-match-result "OLLEH\n"))) +(ert-deftest eshell-test/pipe-subcommand () + "Check that piping with asynchronous subcommands works" + (skip-unless (and (executable-find "echo") + (executable-find "cat"))) + (with-temp-eshell + (eshell-command-result-p "echo ${*echo hi} | *cat" + "hi"))) + (ert-deftest eshell-test/redirect-buffer () "Check that piping to a buffer works" (with-temp-buffer -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#55590: [PATCH] 29.0.50; Eshell subcommands clobber pipelines and produce incorrect output 2022-05-23 3:43 ` bug#55590: [PATCH] " Jim Porter @ 2022-05-23 5:39 ` Sean Whitton 2022-05-23 15:41 ` bug#55590: [PATCH v2] " Jim Porter 0 siblings, 1 reply; 6+ messages in thread From: Sean Whitton @ 2022-05-23 5:39 UTC (permalink / raw) To: Jim Porter, 55590 Hello Jim, On Sun 22 May 2022 at 08:43pm -07, Jim Porter wrote: > On 5/22/2022 8:34 PM, Jim Porter wrote: >> Starting from `emacs -Q -f eshell', enter the following command: >> >> echo ${*echo hi} | rev >> >> The output is: >> >> ~ $ ih >> >> That is, the output of the command ended up *after* the prompt, when it >> should be before. > > The attached patch fixes this issue. It might not be the *best* way to > do it, but it's the least-invasive I could come up with. The > `make-symbol' dance is a bit awkward, but Eshell already uses it for > `eshell-command-to-value' / `eshell-do-command-to-value', so hopefully > it's ok. Couldn't you just defvar a couple of vars for this purpose? -- Sean Whitton ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#55590: [PATCH v2] 29.0.50; Eshell subcommands clobber pipelines and produce incorrect output 2022-05-23 5:39 ` Sean Whitton @ 2022-05-23 15:41 ` Jim Porter 2022-05-24 12:58 ` bug#55590: " Lars Ingebrigtsen 0 siblings, 1 reply; 6+ messages in thread From: Jim Porter @ 2022-05-23 15:41 UTC (permalink / raw) To: Sean Whitton, 55590 [-- Attachment #1: Type: text/plain, Size: 1329 bytes --] On 5/22/2022 10:39 PM, Sean Whitton wrote: > On Sun 22 May 2022 at 08:43pm -07, Jim Porter wrote: >> On 5/22/2022 8:34 PM, Jim Porter wrote: >>> Starting from `emacs -Q -f eshell', enter the following command: >>> >>> echo ${*echo hi} | rev >>> >>> The output is: >>> >>> ~ $ ih >>> >>> That is, the output of the command ended up *after* the prompt, when it >>> should be before. [snip] > > Couldn't you just defvar a couple of vars for this purpose? Do you mean just defvar-ing `headproc' and `tailproc' (and renaming them, of course)? That works for my example above, but falls apart for more complex cases such as: echo ${*echo hi | rev} | tr a-z A-Z The Eshell manual also says the following command is broken (output comes after the prompt). It works correctly with my patch, but just adding defvars isn't enough: for i in 1 2 3 { grep -q a b && *echo hi } | wc -l The "Command evaluation macros" comment section in lisp/eshell/esh-cmd.el indicates that there are some odd limitations on these macros, so that might be part of the issue. I'm still a bit confused as to why the dance in my patch is necessary, but I'm ok with any implementation that passes the tests. Attached is an updated patch with an additional test for the more-complex case and the manual entry about this bug removed. [-- Attachment #2: 0001-Keep-subcommands-in-pipelines-from-clobbering-the-he.patch --] [-- Type: text/plain, Size: 4720 bytes --] From 84677237af2b8fb74702b420c4f9214ca537339b Mon Sep 17 00:00:00 2001 From: Jim Porter <jporterbugs@gmail.com> Date: Sun, 22 May 2022 17:27:48 -0700 Subject: [PATCH] Keep subcommands in pipelines from clobbering the head/tail processes * lisp/eshell/esh-cmd.el (eshell-execute-pipeline): Use 'make-symbol' for headproc and tailproc. (eshell-do-pipelines, eshell-do-pipelines-synchronously): Adapt to the above. * test/lisp/eshell/eshell-tests.el (eshell-test/pipe-subcommand) (eshell-test/pipe-subcommand-with-pipe): New test. * doc/misc/eshell.texi (Bugs and ideas): Remove item about piping to process from loop; this commit fixes it (bug#55590). --- doc/misc/eshell.texi | 8 -------- lisp/eshell/esh-cmd.el | 15 ++++++++++----- test/lisp/eshell/eshell-tests.el | 16 ++++++++++++++++ 3 files changed, 26 insertions(+), 13 deletions(-) diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi index d35a642b62..85e5a4933f 100644 --- a/doc/misc/eshell.texi +++ b/doc/misc/eshell.texi @@ -1758,14 +1758,6 @@ Bugs and ideas function arg () @{ blah $* @} @end example -@item @samp{for i in 1 2 3 @{ grep -q a b && *echo has it @} | wc -l} outputs result after prompt - -In fact, piping to a process from a looping construct doesn't work in -general. If I change the call to @code{eshell-copy-handles} in -@code{eshell-rewrite-for-command} to use @code{eshell-protect}, it seems -to work, but the output occurs after the prompt is displayed. The whole -structured command thing is too complicated at present. - @item Pcomplete sometimes gets stuck You press @key{TAB}, but no completions appear, even though the diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el index 42616e7037..73c250632c 100644 --- a/lisp/eshell/esh-cmd.el +++ b/lisp/eshell/esh-cmd.el @@ -827,8 +827,8 @@ eshell-do-pipelines ((cdr pipeline) t) (t (quote 'last))))) (let ((proc ,(car pipeline))) - (setq headproc (or proc headproc)) - (setq tailproc (or tailproc proc)) + (set headproc (or proc (symbol-value headproc))) + (set tailproc (or (symbol-value tailproc) proc)) proc)))))) (defmacro eshell-do-pipelines-synchronously (pipeline) @@ -861,7 +861,7 @@ eshell-do-pipelines-synchronously (let ((result ,(car pipeline))) ;; tailproc gets the result of the last successful process in ;; the pipeline. - (setq tailproc (or result tailproc)) + (set tailproc (or result (symbol-value tailproc))) ,(if (cdr pipeline) `(eshell-do-pipelines-synchronously (quote ,(cdr pipeline)))) result)))) @@ -870,7 +870,11 @@ 'eshell-process-identity (defmacro eshell-execute-pipeline (pipeline) "Execute the commands in PIPELINE, connecting each to one another." - `(let ((eshell-in-pipeline-p t) headproc tailproc) + `(let ((eshell-in-pipeline-p t) + (headproc (make-symbol "headproc")) + (tailproc (make-symbol "tailproc"))) + (set headproc nil) + (set tailproc nil) (progn ,(if (fboundp 'make-process) `(eshell-do-pipelines ,pipeline) @@ -880,7 +884,8 @@ eshell-execute-pipeline (car (aref eshell-current-handles ,eshell-error-handle)) nil))) (eshell-do-pipelines-synchronously ,pipeline))) - (eshell-process-identity (cons headproc tailproc))))) + (eshell-process-identity (cons (symbol-value headproc) + (symbol-value tailproc)))))) (defmacro eshell-as-subcommand (command) "Execute COMMAND using a temp buffer. diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el index 7cdeb017e4..c0affed80a 100644 --- a/test/lisp/eshell/eshell-tests.el +++ b/test/lisp/eshell/eshell-tests.el @@ -114,6 +114,22 @@ eshell-test/pipe-headproc-stdin (eshell-wait-for-subprocess) (eshell-match-result "OLLEH\n"))) +(ert-deftest eshell-test/pipe-subcommand () + "Check that piping with an asynchronous subcommand works" + (skip-unless (and (executable-find "echo") + (executable-find "cat"))) + (with-temp-eshell + (eshell-command-result-p "echo ${*echo hi} | *cat" + "hi"))) + +(ert-deftest eshell-test/pipe-subcommand-with-pipe () + "Check that piping with an asynchronous subcommand with its own pipe works" + (skip-unless (and (executable-find "echo") + (executable-find "cat"))) + (with-temp-eshell + (eshell-command-result-p "echo ${*echo hi | *cat} | *cat" + "hi"))) + (ert-deftest eshell-test/redirect-buffer () "Check that piping to a buffer works" (with-temp-buffer -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#55590: 29.0.50; Eshell subcommands clobber pipelines and produce incorrect output 2022-05-23 15:41 ` bug#55590: [PATCH v2] " Jim Porter @ 2022-05-24 12:58 ` Lars Ingebrigtsen 2022-05-25 2:27 ` Jim Porter 0 siblings, 1 reply; 6+ messages in thread From: Lars Ingebrigtsen @ 2022-05-24 12:58 UTC (permalink / raw) To: Jim Porter; +Cc: 55590, Sean Whitton Jim Porter <jporterbugs@gmail.com> writes: > Attached is an updated patch with an additional test for the > more-complex case and the manual entry about this bug removed. Thanks; pushed to Emacs 29. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#55590: 29.0.50; Eshell subcommands clobber pipelines and produce incorrect output 2022-05-24 12:58 ` bug#55590: " Lars Ingebrigtsen @ 2022-05-25 2:27 ` Jim Porter 0 siblings, 0 replies; 6+ messages in thread From: Jim Porter @ 2022-05-25 2:27 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: Sean Whitton, 55590 On 5/24/2022 5:58 AM, Lars Ingebrigtsen wrote: > Jim Porter <jporterbugs@gmail.com> writes: > >> Attached is an updated patch with an additional test for the >> more-complex case and the manual entry about this bug removed. > > Thanks; pushed to Emacs 29. Thanks for merging; of course, if anyone has a cleaner implementation that passes the tests, I'd be happy to see that merge too. I don't quite understand all the intricacies of Eshell's command evaluation macros... ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-25 2:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-23 3:34 bug#55590: 29.0.50; Eshell subcommands clobber pipelines and produce incorrect output Jim Porter 2022-05-23 3:43 ` bug#55590: [PATCH] " Jim Porter 2022-05-23 5:39 ` Sean Whitton 2022-05-23 15:41 ` bug#55590: [PATCH v2] " Jim Porter 2022-05-24 12:58 ` bug#55590: " Lars Ingebrigtsen 2022-05-25 2:27 ` Jim Porter
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.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).