* bug#65795: 30.0.50; [PATCH] Keep track of all processes in Eshell pipelines
@ 2023-09-07 4:26 Jim Porter
2023-09-10 17:42 ` Jim Porter
0 siblings, 1 reply; 2+ messages in thread
From: Jim Porter @ 2023-09-07 4:26 UTC (permalink / raw)
To: 65795
[-- Attachment #1: Type: text/plain, Size: 781 bytes --]
Currently, Eshell only keeps track of the head and tail processes in a
pipeline. That works for most things, but it can cause issues with
Eshell thinking the pipeline is done too early. If the tail process ends
first, we assume the entire pipeline is done, when that's not always true.
These patches change things so that Eshell now keeps track of every
process in a pipeline and waits for them to be done before proceeding on
with the rest of Eshell's evaluation. That's less error-prone, and
matches the behavior of other shells.
(This also helps prepare the codebase for the addition of a new feature
for Eshell: job control. I have a mostly-working version of that
locally, so once this merges and I've finished the job control patches,
I'll post them to a new bug.)
[-- Attachment #2: 0001-Move-common-Eshell-pipeline-code-to-a-separate-funct.patch --]
[-- Type: text/plain, Size: 3597 bytes --]
From 627b85895590e75507fa329691f76a0a22daea7d Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 31 Mar 2023 16:57:18 -0700
Subject: [PATCH 1/5] ; Move common Eshell pipeline code to a separate function
* lisp/eshell/esh-cmd.el (eshell-do-pipelines)
(eshell-do-pipelines-synchronously): Move code for manipulating
deferrable command forms to...
(eshell--unmark-deferrable): ... here.
---
lisp/eshell/esh-cmd.el | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index ed2d6c71fc8..f6846345d7d 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -790,10 +790,25 @@ eshell-protect
(eshell-protect-handles eshell-current-handles)
,object))
+(defun eshell--unmark-deferrable (command)
+ "If COMMAND is (or ends with) a deferrable command, unmark it as such.
+This changes COMMAND in-place by converting function calls listed
+in `eshell-deferrable-commands' to their non-deferrable forms so
+that Eshell doesn't erroneously allow deferring it. For example,
+`eshell-named-command' becomes `eshell-named-command*', "
+ (let ((cmd command))
+ (when (memq (car cmd) '(let progn))
+ (setq cmd (car (last cmd))))
+ (when (memq (car cmd) eshell-deferrable-commands)
+ (setcar cmd (intern-soft
+ (concat (symbol-name (car cmd)) "*"))))
+ command))
+
(defmacro eshell-do-pipelines (pipeline &optional notfirst)
"Execute the commands in PIPELINE, connecting each to one another.
This macro calls itself recursively, with NOTFIRST non-nil."
(when (setq pipeline (cadr pipeline))
+ (eshell--unmark-deferrable (car pipeline))
`(eshell-with-copied-handles
(progn
,(when (cdr pipeline)
@@ -801,14 +816,6 @@ eshell-do-pipelines
(eshell-do-pipelines (quote ,(cdr pipeline)) t)))
(eshell-set-output-handle ,eshell-output-handle
'append nextproc)))
- ,(let ((head (car pipeline)))
- (if (memq (car head) '(let progn))
- (setq head (car (last head))))
- (when (memq (car head) eshell-deferrable-commands)
- (ignore
- (setcar head
- (intern-soft
- (concat (symbol-name (car head)) "*"))))))
;; First and last elements in a pipeline may need special treatment.
;; (Currently only eshell-ls-files uses 'last.)
;; Affects process-connection-type in eshell-gather-process-output.
@@ -828,20 +835,13 @@ eshell-do-pipelines-synchronously
Output of each command is passed as input to the next one in the pipeline.
This is used on systems where async subprocesses are not supported."
(when (setq pipeline (cadr pipeline))
+ ;; FIXME: is deferrable significant here?
+ (eshell--unmark-deferrable (car pipeline))
`(progn
,(when (cdr pipeline)
`(let ((output-marker ,(point-marker)))
(eshell-set-output-handle ,eshell-output-handle
'append output-marker)))
- ,(let ((head (car pipeline)))
- (if (memq (car head) '(let progn))
- (setq head (car (last head))))
- ;; FIXME: is deferrable significant here?
- (when (memq (car head) eshell-deferrable-commands)
- (ignore
- (setcar head
- (intern-soft
- (concat (symbol-name (car head)) "*"))))))
;; The last process in the pipe should get its handles
;; redirected as we found them before running the pipe.
,(if (null (cdr pipeline))
--
2.25.1
[-- Attachment #3: 0002-Make-Eshell-synchronous-pipeline-code-more-similar-t.patch --]
[-- Type: text/plain, Size: 3465 bytes --]
From 64edd87dc480f19400e30623111d4cc4867e9272 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 2 Apr 2023 21:54:32 -0700
Subject: [PATCH 2/5] ; Make Eshell synchronous pipeline code more similar to
asynchronous
* lisp/eshell/esh-cmd.el (eshell-do-pipelines-synchronously): Use
'eshell-with-copied-handles'.
(eshell-execute-pipeline): Remove now-unnecessary let-bindings.
---
lisp/eshell/esh-cmd.el | 40 +++++++++++++++++++---------------------
1 file changed, 19 insertions(+), 21 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index f6846345d7d..45176b332d8 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -838,39 +838,37 @@ eshell-do-pipelines-synchronously
;; FIXME: is deferrable significant here?
(eshell--unmark-deferrable (car pipeline))
`(progn
+ (eshell-with-copied-handles
+ (progn
+ ,(when (cdr pipeline)
+ `(let ((output-marker ,(point-marker)))
+ (eshell-set-output-handle ,eshell-output-handle
+ 'append output-marker)))
+ (let (;; XXX: `eshell-in-pipeline-p' has a different meaning
+ ;; for synchronous processes: it's non-nil only when
+ ;; piping *to* a process.
+ (eshell-in-pipeline-p ,(and (cdr pipeline) t)))
+ (let ((result ,(car pipeline)))
+ ;; `tailproc' gets the result of the last successful
+ ;; process in the pipeline.
+ (set tailproc (or result (symbol-value tailproc))))))
+ ;; Steal handles if this is the last item in the pipeline.
+ ,(null (cdr pipeline)))
,(when (cdr pipeline)
- `(let ((output-marker ,(point-marker)))
- (eshell-set-output-handle ,eshell-output-handle
- 'append output-marker)))
- ;; The last process in the pipe should get its handles
- ;; redirected as we found them before running the pipe.
- ,(if (null (cdr pipeline))
- '(progn
- (setq eshell-current-handles tail-handles)
- (setq eshell-in-pipeline-p nil)))
- (let ((result ,(car pipeline)))
- ;; tailproc gets the result of the last successful process in
- ;; the pipeline.
- (set tailproc (or result (symbol-value tailproc)))
- ,(if (cdr pipeline)
- `(eshell-do-pipelines-synchronously (quote ,(cdr pipeline))))
- result))))
+ `(eshell-do-pipelines-synchronously (quote ,(cdr pipeline)))))))
(defalias 'eshell-process-identity 'identity)
(defmacro eshell-execute-pipeline (pipeline)
"Execute the commands in PIPELINE, connecting each to one another."
- `(let ((eshell-in-pipeline-p t)
- (headproc (make-symbol "headproc"))
+ `(let ((headproc (make-symbol "headproc"))
(tailproc (make-symbol "tailproc")))
(set headproc nil)
(set tailproc nil)
(progn
,(if eshell-supports-asynchronous-processes
`(eshell-do-pipelines ,pipeline)
- `(let ((tail-handles (eshell-duplicate-handles
- eshell-current-handles)))
- (eshell-do-pipelines-synchronously ,pipeline)))
+ `(eshell-do-pipelines-synchronously ,pipeline))
(eshell-process-identity (cons (symbol-value headproc)
(symbol-value tailproc))))))
--
2.25.1
[-- Attachment #4: 0003-Collect-all-processes-in-an-Eshell-pipeline-not-just.patch --]
[-- Type: text/plain, Size: 10300 bytes --]
From f41436101706501a7ca8f15ffbd4b34fca31579c Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 2 Apr 2023 22:41:29 -0700
Subject: [PATCH 3/5] Collect all processes in an Eshell pipeline, not just the
head and tail
This has the extra benefit that Eshell now only considers a pipeline
to be done when *all* of its processes are done (previously, it
checked only the last one in the pipeline).
* lisp/eshell/esh-util.el (eshell-process-pair-p)
(eshell-make-process-pair): Rename to...
(eshell-process-list-p, eshell-make-process-list): ... these, and
handle lists of processes. Update callers.
* lisp/eshell/esh-cmd.el (eshell-head-process): Use 'car'.
(eshell-tail-process): Get the last element of the list.
(eshell-do-pipelines): Return a list of all processes in the pipeline.
(eshell-do-pipelines-synchronously): Return the result of the first
command (usually t or nil).
(eshell-execute-pipeline): Simplify.
(eshell-do-eval): Pass all processes to 'eshell/wait'.
---
lisp/eshell/esh-cmd.el | 109 +++++++++++++++++++---------------------
lisp/eshell/esh-util.el | 23 ++++-----
2 files changed, 62 insertions(+), 70 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 45176b332d8..a67b8abcc67 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -275,12 +275,9 @@ eshell-last-arguments
(defvar eshell-last-command-name nil)
(defvar eshell-last-async-procs nil
"The currently-running foreground process(es).
-When executing a pipeline, this is a cons cell whose CAR is the
-first process (usually reading from stdin) and whose CDR is the
-last process (usually writing to stdout). Otherwise, the CAR and
-CDR are the same process.
-
-When the process in the CDR completes, resume command evaluation.")
+When executing a pipeline, this is a list of all the pipeline's
+processes, with the first usually reading from stdin and last
+usually writing to stdout.")
(defvar eshell-allow-commands t
"If non-nil, allow evaluating command forms (including Lisp forms).
@@ -302,12 +299,12 @@ eshell-interactive-process-p
(defsubst eshell-head-process ()
"Return the currently running process at the head of any pipeline.
This only returns external (non-Lisp) processes."
- (car-safe eshell-last-async-procs))
+ (car eshell-last-async-procs))
(defsubst eshell-tail-process ()
"Return the currently running process at the tail of any pipeline.
This only returns external (non-Lisp) processes."
- (cdr-safe eshell-last-async-procs))
+ (car (last eshell-last-async-procs)))
(define-obsolete-function-alias 'eshell-interactive-process
'eshell-tail-process "29.1")
@@ -806,54 +803,56 @@ eshell--unmark-deferrable
(defmacro eshell-do-pipelines (pipeline &optional notfirst)
"Execute the commands in PIPELINE, connecting each to one another.
+Returns a list of the processes in the pipeline.
+
This macro calls itself recursively, with NOTFIRST non-nil."
(when (setq pipeline (cadr pipeline))
(eshell--unmark-deferrable (car pipeline))
`(eshell-with-copied-handles
- (progn
- ,(when (cdr pipeline)
- `(let ((nextproc
- (eshell-do-pipelines (quote ,(cdr pipeline)) t)))
- (eshell-set-output-handle ,eshell-output-handle
- 'append nextproc)))
- ;; First and last elements in a pipeline may need special treatment.
- ;; (Currently only eshell-ls-files uses 'last.)
- ;; Affects process-connection-type in eshell-gather-process-output.
- (let ((eshell-in-pipeline-p
- ,(cond ((not notfirst) (quote 'first))
- ((cdr pipeline) t)
- (t (quote 'last)))))
- (let ((proc ,(car pipeline)))
- (set headproc (or proc (symbol-value headproc)))
- (set tailproc (or (symbol-value tailproc) proc))
- proc)))
+ (let ((next-procs
+ ,(when (cdr pipeline)
+ `(eshell-do-pipelines (quote ,(cdr pipeline)) t)))
+ ;; First and last elements in a pipeline may need special
+ ;; treatment (currently only `eshell-ls-files' uses
+ ;; `last'). Affects `process-connection-type' in
+ ;; `eshell-gather-process-output'.
+ (eshell-in-pipeline-p
+ ,(cond ((not notfirst) (quote 'first))
+ ((cdr pipeline) t)
+ (t (quote 'last)))))
+ ,(when (cdr pipeline)
+ `(eshell-set-output-handle ,eshell-output-handle
+ 'append (car next-procs)))
+ (let ((proc ,(car pipeline)))
+ (cons proc next-procs)))
;; Steal handles if this is the last item in the pipeline.
,(null (cdr pipeline)))))
(defmacro eshell-do-pipelines-synchronously (pipeline)
"Execute the commands in PIPELINE in sequence synchronously.
-Output of each command is passed as input to the next one in the pipeline.
-This is used on systems where async subprocesses are not supported."
+This collects the output of each command in turn, passing it as
+input to the next one in the pipeline. Returns the result of the
+first command invocation in the pipeline (usually t or nil).
+
+This is used on systems where async subprocesses are not
+supported."
(when (setq pipeline (cadr pipeline))
;; FIXME: is deferrable significant here?
(eshell--unmark-deferrable (car pipeline))
- `(progn
- (eshell-with-copied-handles
- (progn
- ,(when (cdr pipeline)
- `(let ((output-marker ,(point-marker)))
- (eshell-set-output-handle ,eshell-output-handle
- 'append output-marker)))
- (let (;; XXX: `eshell-in-pipeline-p' has a different meaning
- ;; for synchronous processes: it's non-nil only when
- ;; piping *to* a process.
- (eshell-in-pipeline-p ,(and (cdr pipeline) t)))
- (let ((result ,(car pipeline)))
- ;; `tailproc' gets the result of the last successful
- ;; process in the pipeline.
- (set tailproc (or result (symbol-value tailproc))))))
- ;; Steal handles if this is the last item in the pipeline.
- ,(null (cdr pipeline)))
+ `(prog1
+ (eshell-with-copied-handles
+ (progn
+ ,(when (cdr pipeline)
+ `(let ((output-marker ,(point-marker)))
+ (eshell-set-output-handle ,eshell-output-handle
+ 'append output-marker)))
+ (let (;; XXX: `eshell-in-pipeline-p' has a different
+ ;; meaning for synchronous processes: it's non-nil
+ ;; only when piping *to* a process.
+ (eshell-in-pipeline-p ,(and (cdr pipeline) t)))
+ ,(car pipeline)))
+ ;; Steal handles if this is the last item in the pipeline.
+ ,(null (cdr pipeline)))
,(when (cdr pipeline)
`(eshell-do-pipelines-synchronously (quote ,(cdr pipeline)))))))
@@ -861,16 +860,10 @@ 'eshell-process-identity
(defmacro eshell-execute-pipeline (pipeline)
"Execute the commands in PIPELINE, connecting each to one another."
- `(let ((headproc (make-symbol "headproc"))
- (tailproc (make-symbol "tailproc")))
- (set headproc nil)
- (set tailproc nil)
- (progn
- ,(if eshell-supports-asynchronous-processes
- `(eshell-do-pipelines ,pipeline)
- `(eshell-do-pipelines-synchronously ,pipeline))
- (eshell-process-identity (cons (symbol-value headproc)
- (symbol-value tailproc))))))
+ `(eshell-process-identity
+ ,(if eshell-supports-asynchronous-processes
+ `(remove nil (eshell-do-pipelines ,pipeline))
+ `(eshell-do-pipelines-synchronously ,pipeline))))
(defmacro eshell-as-subcommand (command)
"Execute COMMAND as a subcommand.
@@ -1021,7 +1014,7 @@ eshell-resume-eval
(setq retval
(eshell-do-eval
eshell-current-command))))))
- (if (eshell-process-pair-p procs)
+ (if (eshell-process-list-p procs)
(ignore (setq eshell-last-async-procs procs))
(cadr retval)))))
(error
@@ -1228,10 +1221,10 @@ eshell-do-eval
(eshell-do-eval form synchronous-p))
(if-let (((memq (car form) eshell-deferrable-commands))
((not eshell-current-subjob-p))
- (procs (eshell-make-process-pair result)))
+ (procs (eshell-make-process-list result)))
(if synchronous-p
- (eshell/wait (cdr procs))
- (eshell-manipulate form "inserting ignore form"
+ (apply #'eshell/wait procs)
+ (eshell-manipulate form "inserting ignore form"
(setcar form 'ignore)
(setcdr form nil))
(throw 'eshell-defer procs))
diff --git a/lisp/eshell/esh-util.el b/lisp/eshell/esh-util.el
index d5a75b0d715..5134adeb7fb 100644
--- a/lisp/eshell/esh-util.el
+++ b/lisp/eshell/esh-util.el
@@ -730,19 +730,18 @@ eshell-processp
"If the `processp' function does not exist, PROC is not a process."
(and (fboundp 'processp) (processp proc)))
-(defun eshell-process-pair-p (procs)
- "Return non-nil if PROCS is a pair of process objects."
- (and (consp procs)
- (eshell-processp (car procs))
- (eshell-processp (cdr procs))))
-
-(defun eshell-make-process-pair (procs)
- "Make a pair of process objects from PROCS if possible.
-This represents the head and tail of a pipeline of processes,
-where the head and tail may be the same process."
+(defun eshell-process-list-p (procs)
+ "Return non-nil if PROCS is a list of process objects."
+ (and (listp procs)
+ (seq-every-p #'eshell-processp procs)))
+
+(defun eshell-make-process-list (procs)
+ "Make a list of process objects from PROCS if possible.
+PROCS can be a single process or a list thereof. If PROCS is
+anything else, return nil instead."
(pcase procs
- ((pred eshell-processp) (cons procs procs))
- ((pred eshell-process-pair-p) procs)))
+ ((pred eshell-processp) (list procs))
+ ((pred eshell-process-list-p) procs)))
;; (defun eshell-copy-file
;; (file newname &optional ok-if-already-exists keep-date)
--
2.25.1
[-- Attachment #5: 0004-Move-some-Eshell-tests-to-more-topical-files.patch --]
[-- Type: text/plain, Size: 8091 bytes --]
From 68ca271bf1315bb2f55f4bf48d7c8d8d448734b5 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 6 Sep 2023 17:00:59 -0700
Subject: [PATCH 4/5] ; Move some Eshell tests to more-topical files
eshell-tests.el should mainly be for code in eshell.el.
* test/lisp/eshell/eshell-tests.el (eshell-test/pipe-headproc)
(eshell-test/pipe-tailproc, eshell-test/pipe-subcommand)
(eshell-test/pipe-subcommand-with-pipe)
(eshell-test/subcommand-reset-in-pipeline)
(eshell-test/lisp-reset-in-pipeline): Move to...
* test/lisp/eshell/esh-cmd-tests.el
(esh-cmd-test/pipeline-wait/head-proc)
(esh-cmd-test/pipeline-wait/tail-proc)
(esh-cmd-test/pipeline-wait/subcommand)
(esh-cmd-test/pipeline-wait/subcommand-with-pipe)
(esh-cmd-test/reset-in-pipeline/subcommand)
(esh-cmd-test/reset-in-pipeline/lisp): ... here.
* test/lisp/eshell/eshell-tests.el (eshell-test/pipe-headproc-stdin):
Move to...
* test/lisp/eshell/esh-io-tests.el
(esh-io-test/pipeline/stdin-to-head): ... here.
---
test/lisp/eshell/esh-cmd-tests.el | 62 +++++++++++++++++++++++++++
test/lisp/eshell/esh-io-tests.el | 11 +++++
test/lisp/eshell/eshell-tests.el | 69 -------------------------------
3 files changed, 73 insertions(+), 69 deletions(-)
diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el
index a7208eb3a0b..3967910a53d 100644
--- a/test/lisp/eshell/esh-cmd-tests.el
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -137,6 +137,68 @@ esh-cmd-test/or-operator
(eshell-match-command-output "[ foo = bar ] || echo hi"
"hi\n")))
+\f
+;; Pipelines
+
+(ert-deftest esh-cmd-test/pipeline-wait/head-proc ()
+ "Check that piping a non-process to a process command waits for the process."
+ (skip-unless (executable-find "cat"))
+ (with-temp-eshell
+ (eshell-match-command-output "echo hi | *cat"
+ "hi")))
+
+(ert-deftest esh-cmd-test/pipeline-wait/tail-proc ()
+ "Check that piping a process to a non-process command waits for the process."
+ (skip-unless (executable-find "echo"))
+ (with-temp-eshell
+ (eshell-match-command-output "*echo hi | echo bye"
+ "bye\nhi\n")))
+
+(ert-deftest esh-cmd-test/pipeline-wait/subcommand ()
+ "Check that piping with an asynchronous subcommand waits for the subcommand."
+ (skip-unless (and (executable-find "echo")
+ (executable-find "cat")))
+ (with-temp-eshell
+ (eshell-match-command-output "echo ${*echo hi} | *cat"
+ "hi")))
+
+(ert-deftest esh-cmd-test/pipeline-wait/subcommand-with-pipe ()
+ "Check that piping with an asynchronous subcommand with its own pipe works.
+This should also wait for the subcommand."
+ (skip-unless (and (executable-find "echo")
+ (executable-find "cat")))
+ (with-temp-eshell
+ (eshell-match-command-output "echo ${*echo hi | *cat} | *cat"
+ "hi")))
+
+(ert-deftest esh-cmd-test/reset-in-pipeline/subcommand ()
+ "Check that subcommands reset `eshell-in-pipeline-p'."
+ (skip-unless (executable-find "cat"))
+ (dolist (template '("echo {%s} | *cat"
+ "echo ${%s} | *cat"
+ "*cat $<%s> | *cat"))
+ (eshell-command-result-equal
+ (format template "echo $eshell-in-pipeline-p")
+ nil)
+ (eshell-command-result-equal
+ (format template "echo | echo $eshell-in-pipeline-p")
+ "last")
+ (eshell-command-result-equal
+ (format template "echo $eshell-in-pipeline-p | echo")
+ "first")
+ (eshell-command-result-equal
+ (format template "echo | echo $eshell-in-pipeline-p | echo")
+ "t")))
+
+(ert-deftest esh-cmd-test/reset-in-pipeline/lisp ()
+ "Check that interpolated Lisp forms reset `eshell-in-pipeline-p'."
+ (skip-unless (executable-find "cat"))
+ (dolist (template '("echo (%s) | *cat"
+ "echo $(%s) | *cat"))
+ (eshell-command-result-equal
+ (format template "format \"%s\" eshell-in-pipeline-p")
+ "nil")))
+
\f
;; Control flow statements
diff --git a/test/lisp/eshell/esh-io-tests.el b/test/lisp/eshell/esh-io-tests.el
index ce80f3a8f08..0201b6ab650 100644
--- a/test/lisp/eshell/esh-io-tests.el
+++ b/test/lisp/eshell/esh-io-tests.el
@@ -334,6 +334,17 @@ esh-io-test/pipeline/subcommands
(eshell-match-command-output "{echo foo; echo bar} | rev"
"\\`raboof\n?")))
+(ert-deftest esh-io-test/pipeline/stdin-to-head ()
+ "Check that standard input is sent to the head process in a pipeline."
+ (skip-unless (and (executable-find "tr")
+ (executable-find "rev")))
+ (with-temp-eshell
+ (eshell-insert-command "tr a-z A-Z | rev")
+ (eshell-insert-command "hello")
+ (eshell-send-eof-to-process)
+ (eshell-wait-for-subprocess)
+ (should (eshell-match-output "OLLEH\n"))))
+
\f
;; Virtual targets
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index 46c9482ecf4..777c927c78e 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -38,75 +38,6 @@ eshell-test-value
;;; Tests:
-(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-match-command-output "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"))
- (with-temp-eshell
- (eshell-match-command-output "*echo hi | echo bye"
- "bye\nhi\n")))
-
-(ert-deftest eshell-test/pipe-headproc-stdin ()
- "Check that standard input is sent to the head process in a pipeline"
- (skip-unless (and (executable-find "tr")
- (executable-find "rev")))
- (with-temp-eshell
- (eshell-insert-command "tr a-z A-Z | rev")
- (eshell-insert-command "hello")
- (eshell-send-eof-to-process)
- (eshell-wait-for-subprocess)
- (should (eshell-match-output "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-match-command-output "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-match-command-output "echo ${*echo hi | *cat} | *cat"
- "hi")))
-
-(ert-deftest eshell-test/subcommand-reset-in-pipeline ()
- "Check that subcommands reset `eshell-in-pipeline-p'."
- (skip-unless (executable-find "cat"))
- (dolist (template '("echo {%s} | *cat"
- "echo ${%s} | *cat"
- "*cat $<%s> | *cat"))
- (eshell-command-result-equal
- (format template "echo $eshell-in-pipeline-p")
- nil)
- (eshell-command-result-equal
- (format template "echo | echo $eshell-in-pipeline-p")
- "last")
- (eshell-command-result-equal
- (format template "echo $eshell-in-pipeline-p | echo")
- "first")
- (eshell-command-result-equal
- (format template "echo | echo $eshell-in-pipeline-p | echo")
- "t")))
-
-(ert-deftest eshell-test/lisp-reset-in-pipeline ()
- "Check that interpolated Lisp forms reset `eshell-in-pipeline-p'."
- (skip-unless (executable-find "cat"))
- (dolist (template '("echo (%s) | *cat"
- "echo $(%s) | *cat"))
- (eshell-command-result-equal
- (format template "format \"%s\" eshell-in-pipeline-p")
- "nil")))
-
(ert-deftest eshell-test/eshell-command/simple ()
"Test that the `eshell-command' function writes to the current buffer."
(skip-unless (executable-find "echo"))
--
2.25.1
[-- Attachment #6: 0005-Wait-for-all-processes-in-a-pipeline-before-resuming.patch --]
[-- Type: text/plain, Size: 3039 bytes --]
From fea10a1585b1aed8f502c609bd8c7cb563486f64 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 6 Sep 2023 17:01:06 -0700
Subject: [PATCH 5/5] Wait for all processes in a pipeline before resuming an
Eshell command
Previously, we only waited until the tail process was finished, but
now, we wait for all of them. This is more consistent with other
shells, and prevents some cases of a process's output coming *after*
we continued past its pipeline.
* lisp/eshell/esh-cmd.el (eshell-resume-command): Simplify
conditionals, and check that all the foreground processes are dead
before resuming Eshell command.
* test/lisp/eshell/esh-cmd-tests.el
(esh-cmd-test/pipeline-wait/multi-proc): New test.
---
lisp/eshell/esh-cmd.el | 18 +++++++++++-------
test/lisp/eshell/esh-cmd-tests.el | 10 ++++++++++
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index a67b8abcc67..ca608e0e881 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -994,13 +994,17 @@ eshell-eval-command
result)))
(defun eshell-resume-command (proc status)
- "Resume the current command when a process ends."
- (when proc
- (unless (or (not (stringp status))
- (string= "stopped" status)
- (string-match eshell-reset-signals status))
- (if (eq proc (eshell-tail-process))
- (eshell-resume-eval)))))
+ "Resume the current command when a pipeline ends."
+ (when (and proc
+ ;; Make sure STATUS is something we want to handle.
+ (stringp status)
+ (not (string= "stopped" status))
+ (not (string-match eshell-reset-signals status))
+ ;; Make sure PROC is one of our foreground processes and
+ ;; that all of those processes are now dead.
+ (member proc eshell-last-async-procs)
+ (not (seq-some #'process-live-p eshell-last-async-procs)))
+ (eshell-resume-eval)))
(defun eshell-resume-eval ()
"Destructively evaluate a form which may need to be deferred."
diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el
index 3967910a53d..d625b8a6a5d 100644
--- a/test/lisp/eshell/esh-cmd-tests.el
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -154,6 +154,16 @@ esh-cmd-test/pipeline-wait/tail-proc
(eshell-match-command-output "*echo hi | echo bye"
"bye\nhi\n")))
+(ert-deftest esh-cmd-test/pipeline-wait/multi-proc ()
+ "Check that a pipeline waits for all its processes before returning."
+ (skip-unless (and (executable-find "echo")
+ (executable-find "sh")
+ (executable-find "rev")))
+ (with-temp-eshell
+ (eshell-match-command-output
+ "*echo hello | sh -c 'sleep 1; rev' 1>&2 | *echo goodbye"
+ "goodbye\nolleh\n")))
+
(ert-deftest esh-cmd-test/pipeline-wait/subcommand ()
"Check that piping with an asynchronous subcommand waits for the subcommand."
(skip-unless (and (executable-find "echo")
--
2.25.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* bug#65795: 30.0.50; [PATCH] Keep track of all processes in Eshell pipelines
2023-09-07 4:26 bug#65795: 30.0.50; [PATCH] Keep track of all processes in Eshell pipelines Jim Porter
@ 2023-09-10 17:42 ` Jim Porter
0 siblings, 0 replies; 2+ messages in thread
From: Jim Porter @ 2023-09-10 17:42 UTC (permalink / raw)
To: 65795-done
Version: 30.1
On 9/6/2023 9:26 PM, Jim Porter wrote:
> These patches change things so that Eshell now keeps track of every
> process in a pipeline and waits for them to be done before proceeding on
> with the rest of Eshell's evaluation. That's less error-prone, and
> matches the behavior of other shells.
Merged to master as 2ec41c174f9. Closing this now.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-09-10 17:42 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-07 4:26 bug#65795: 30.0.50; [PATCH] Keep track of all processes in Eshell pipelines Jim Porter
2023-09-10 17:42 ` 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).