bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in Eshell

[Jim Porter <jporterbugs@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

On 2/3/2022 11:03 AM, Lars Ingebrigtsen wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
> 
>> Here's a small additional improvement that I hope is correct. The
>> third patch here changes how eshell-tests-helpers.el is loaded to that
>> it uses `require'. This reduces some of the boilerplate and will
>> hopefully prevent issues with this file not getting recompiled when
>> it's updated.
>>
>> The first two patches are the same as before. I've just included them
>> for completeness/ease of applying.
> 
> Thanks; seems to work well for me, so I've now pushed them to Emacs 29.

I found a bug in the second patch.

   emacs -Q --eval '(eshell)'
   ~ $ echo hi | *cat

This prints:

   ~ $ hi

That is, the output of the command is printed *after* the next prompt. 
That's because my patch wasn't smart enough about finding the "head" 
process in a pipeline. In "echo hi | *cat", the head process is "cat" 
(Eshell's builtin echo command doesn't create a process). In my old 
patch, it thought the head process was nil, which confused Eshell.

Here's a patch with a test to verify that things work correctly. Now the 
output is:

   hi~ $

(That's a bit ugly, but Eshell's builtin echo doesn't normally print a 
newline, so it's correct.)

[-- Attachment #2: 0001-Ensure-that-the-CAR-of-eshell-last-async-procs-alway.patch --]
[-- Type: text/plain, Size: 2019 bytes --]

From 9cd08c8d66cfab977190398947d62b5e30441972 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 4 Feb 2022 22:41:39 -0800
Subject: [PATCH] Ensure that the CAR of 'eshell-last-async-procs' always
 points to a process

Previously, if a non-process was piped to a process, this could end up
being nil, which isn't correct.  'eshell-last-async-procs' should just
ignore non-process commands in a pipeline.

* lisp/eshell/esh-cmd.el (eshell-do-pipelines): Set 'headproc'
correctly.

* test/lisp/eshell/eshell-tests.el (eshell-test/pipe-headproc): New test.
---
 lisp/eshell/esh-cmd.el           | 3 +--
 test/lisp/eshell/eshell-tests.el | 7 +++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index e702de03a0..5819506cc0 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -800,8 +800,7 @@ eshell-do-pipelines
 		      ((cdr pipeline) t)
 		      (t (quote 'last)))))
           (let ((proc ,(car pipeline)))
-            ,(unless notfirst
-               '(setq headproc proc))
+            (setq headproc (or proc headproc))
             (setq tailproc (or tailproc proc))
             proc))))))
 
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index c5ca0a5485..d6ee1bdb17 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -123,6 +123,13 @@ eshell-test/interp-cmd-external-concat
    (eshell-command-result-p "echo ${echo hi}-${*echo there}"
                             "hi-there\n")))
 
+(ert-deftest eshell-test/pipe-headproc ()
+  "Check that piping a non-process to a process command waits for the process"
+  (skip-unless (executable-find "cat"))
+  (with-temp-eshell
+   (eshell-command-result-p "echo hi | *cat"
+                            "hi")))
+
 (ert-deftest eshell-test/pipe-tailproc ()
   "Check that piping a process to a non-process command waits for the process"
   (skip-unless (executable-find "echo"))
-- 
2.25.1