bug#55590: [PATCH] 29.0.50; Eshell subcommands clobber pipelines and produce incorrect output

[Jim Porter <jporterbugs@gmail.com>]

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