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

[Jim Porter <jporterbugs@gmail.com>]

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