* bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in Eshell
@ 2022-02-02 3:32 Jim Porter
2022-02-02 18:10 ` Lars Ingebrigtsen
0 siblings, 1 reply; 9+ messages in thread
From: Jim Porter @ 2022-02-02 3:32 UTC (permalink / raw)
To: 53715
[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]
From `emacs -Q', start Eshell, and first run the following:
~ $ *echo hi | echo bye
This results in:
bye
~ $ hi
That is, `*echo' writes its output *after* the prompt is emitted. This
is because `eshell-do-pipelines' reports that there's no process object
to wait for, so Eshell thinks the prompt can be emitted immediately. I
fixed that in the first patch by setting the `tailproc' in a different
spot. With this change, the result is:
bye
hi
~ $
This may be a bit surprising, since you'd normally expect "echo bye" to
read "hi" on its stdin and just ignore it. However, the builtin Eshell
echo command doesn't consume stdin, so it just goes straight to stdout
instead. It's in reverse order because Eshell runs the pipeline from
right-to-left in order to hook up the pipes properly. For how the
builtin Eshell echo works, I think this is the right behavior. (If
people disagree, I can fix this, but not right now; I'm working on a
patch series to properly support piping to Lisp functions in Eshell, and
I can change it - or not - once that's done.)
Second, run this:
~ $ tr a-z A-Z | rev
hello RET
C-c C-d ;; Send EOF
The result is:
olleh ;; should be "OLLEH"
This happens because Eshell only keeps track of the tail process when
running an external process, so when sending stdin, it gets sent to the
tail process (`rev'), not the head (`tr'). I fixed this in the second
patch by recording both the head and tail processes and updating the
various places that use the current process to use the right one.
[-- Attachment #2: 0001-Ensure-that-tailproc-is-set-for-the-last-process-in-.patch --]
[-- Type: text/plain, Size: 2699 bytes --]
From 6f15832f537035d26d072595bc59ff8795c9550f Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 27 Jan 2022 23:13:36 -0800
Subject: [PATCH 01/11] Ensure that tailproc is set for the last process in an
Eshell pipeline
In particular, this used to fail for pipelines where the last process
in the pipeline came from the first element of the pipeline. This
could happen when a process was piped to an ordinary Lisp function,
like in '*echo hi | echo bye'.
* lisp/eshell/esh-cmd.el (eshell-do-pipelines): Set the tailproc even
for the first process in the pipeline.
* test/lisp/eshell/eshell-tests.el (eshell-test/pipe-tailproc): New
test.
---
lisp/eshell/esh-cmd.el | 7 ++++---
test/lisp/eshell/eshell-tests.el | 7 +++++++
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 04d65df4f3..14139896dd 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -764,8 +764,7 @@ eshell-do-pipelines
(eshell-set-output-handle ,eshell-output-handle
'append nextproc)
(eshell-set-output-handle ,eshell-error-handle
- 'append nextproc)
- (setq tailproc (or tailproc nextproc))))
+ 'append nextproc)))
,(let ((head (car pipeline)))
(if (memq (car head) '(let progn))
(setq head (car (last head))))
@@ -781,7 +780,9 @@ eshell-do-pipelines
,(cond ((not notfirst) (quote 'first))
((cdr pipeline) t)
(t (quote 'last)))))
- ,(car pipeline))))))
+ (let ((proc ,(car pipeline)))
+ (setq tailproc (or tailproc proc))
+ proc))))))
(defmacro eshell-do-pipelines-synchronously (pipeline)
"Execute the commands in PIPELINE in sequence synchronously.
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index 542815df80..7658d5f551 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -129,6 +129,13 @@ eshell-test/interp-cmd-external-concat
(eshell-command-result-p "echo ${echo hi}-${*echo there}"
"hi-there\n")))
+(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-command-result-p "*echo hi | echo bye"
+ "bye\nhi\n")))
+
(ert-deftest eshell-test/window-height ()
"$LINES should equal (window-height)"
(should (eshell-test-command-result "= $LINES (window-height)")))
--
2.25.1
[-- Attachment #3: 0002-When-executing-an-Eshell-pipeline-send-input-to-the-.patch --]
[-- Type: text/plain, Size: 16506 bytes --]
From b81b8f3d05faace8de6537d13540f2783815572f Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 30 Jan 2022 18:53:53 -0800
Subject: [PATCH 02/11] When executing an Eshell pipeline, send input to the
first process
Previously, input was sent to the last process in the pipeline,
resulting in unexpected behavior when running commands like
'tr a-z A-Z | rev'.
* lisp/eshell/esh-util.el (eshell-process-pair-p)
(eshell-make-process-pair): New functions.
* lisp/eshell/esh-cmd.el (eshell-last-async-proc): Rename to...
(eshell-last-async-procs): ... this, and store a pair of processes.
(eshell-interactive-process): Replace with...
(eshell-interactive-process-p, eshell-head-process)
(eshell-tail-process): ... these.
(eshell-cmd-initialize): Set 'eshell-last-async-procs'.
(eshell-do-pipelines): Set 'headproc'.
(eshell-execute-pipeline): Return 'headproc' and 'tailproc'.
(eshell-resume-eval): Use 'eshell-last-async-procs'.
(eshell-do-eval): Make sure we work with a pair of processes.
* lisp/eshell/esh-proc.el (eshell-send-eof-to-process): Move from
here...
* lisp/eshell/esh-mode.el (eshell-send-eof-to-process): ... to here,
and only send EOF to the head process.
* lisp/eshell/em-cmpl.el (eshell-complete-parse-arguments)
* lisp/eshell/esh-mode.el (eshell-intercept-commands)
(eshell-watch-for-password-prompt):
Use 'eshell-interactive-process-p'.
* lisp/eshell/em-rebind.el (eshell-delchar-or-maybe-eof)
* lisp/eshell/em-term.el (eshell-term-send-raw-string)
* lisp/eshell/esh-mode.el (eshell-self-insert-command)
(eshell-send-input, eshell-send-invisible):
Use 'eshell-head-process'.
* lisp/eshell/esh-cmd.el (eshell-as-subcommand):
Use 'eshell-tail-process'.
* lisp/eshell/eshell.el (eshell-command):
Use 'eshell-interactive-process-p' and 'eshell-tail-process'.
* test/lisp/eshell/eshell-tests.el (eshell-test/pipe-headproc-stdin):
New test.
---
lisp/eshell/em-cmpl.el | 2 +-
lisp/eshell/em-rebind.el | 2 +-
lisp/eshell/em-term.el | 2 +-
lisp/eshell/esh-cmd.el | 66 +++++++++++++++++++++-----------
lisp/eshell/esh-io.el | 2 +-
lisp/eshell/esh-mode.el | 28 +++++++++-----
lisp/eshell/esh-proc.el | 11 +-----
lisp/eshell/esh-util.el | 14 +++++++
lisp/eshell/eshell.el | 6 +--
test/lisp/eshell/eshell-tests.el | 11 ++++++
10 files changed, 96 insertions(+), 48 deletions(-)
diff --git a/lisp/eshell/em-cmpl.el b/lisp/eshell/em-cmpl.el
index c6a51b1793..b79475f6e0 100644
--- a/lisp/eshell/em-cmpl.el
+++ b/lisp/eshell/em-cmpl.el
@@ -314,7 +314,7 @@ eshell-completion-help
(defun eshell-complete-parse-arguments ()
"Parse the command line arguments for `pcomplete-argument'."
(when (and eshell-no-completion-during-jobs
- (eshell-interactive-process))
+ (eshell-interactive-process-p))
(insert-and-inherit "\t")
(throw 'pcompleted t))
(let ((end (point-marker))
diff --git a/lisp/eshell/em-rebind.el b/lisp/eshell/em-rebind.el
index f24758d4e3..2b56c9e844 100644
--- a/lisp/eshell/em-rebind.el
+++ b/lisp/eshell/em-rebind.el
@@ -238,7 +238,7 @@ eshell-delchar-or-maybe-eof
Sends an EOF only if point is at the end of the buffer and there is no
input."
(interactive "p")
- (let ((proc (eshell-interactive-process)))
+ (let ((proc (eshell-head-process)))
(if (eobp)
(cond
((/= (point) eshell-last-output-end)
diff --git a/lisp/eshell/em-term.el b/lisp/eshell/em-term.el
index e34c5ae47c..d150c07b03 100644
--- a/lisp/eshell/em-term.el
+++ b/lisp/eshell/em-term.el
@@ -224,7 +224,7 @@ eshell-term-sentinel
; (defun eshell-term-send-raw-string (chars)
; (goto-char eshell-last-output-end)
-; (process-send-string (eshell-interactive-process) chars))
+; (process-send-string (eshell-head-process) chars))
; (defun eshell-term-send-raw ()
; "Send the last character typed through the terminal-emulator
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 14139896dd..e702de03a0 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -279,14 +279,33 @@ eshell-in-pipeline-p
(defvar eshell-in-subcommand-p nil)
(defvar eshell-last-arguments nil)
(defvar eshell-last-command-name nil)
-(defvar eshell-last-async-proc nil
- "When this foreground process completes, resume command evaluation.")
+(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.")
;;; Functions:
-(defsubst eshell-interactive-process ()
- "Return currently running command process, if non-Lisp."
- eshell-last-async-proc)
+(defsubst eshell-interactive-process-p ()
+ "Return non-nil if there is a currently running command process."
+ eshell-last-async-procs)
+
+(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))
+
+(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))
+
+(define-obsolete-function-alias 'eshell-interactive-process
+ 'eshell-tail-process "29.1")
(defun eshell-cmd-initialize () ;Called from `eshell-mode' via intern-soft!
"Initialize the Eshell command processing module."
@@ -295,7 +314,7 @@ eshell-cmd-initialize
(setq-local eshell-command-arguments nil)
(setq-local eshell-last-arguments nil)
(setq-local eshell-last-command-name nil)
- (setq-local eshell-last-async-proc nil)
+ (setq-local eshell-last-async-procs nil)
(add-hook 'eshell-kill-hook #'eshell-resume-command nil t)
@@ -306,7 +325,7 @@ eshell-cmd-initialize
(add-hook 'eshell-post-command-hook
(lambda ()
(setq eshell-current-command nil
- eshell-last-async-proc nil))
+ eshell-last-async-procs nil))
nil t)
(add-hook 'eshell-parse-argument-hook
@@ -781,6 +800,8 @@ eshell-do-pipelines
((cdr pipeline) t)
(t (quote 'last)))))
(let ((proc ,(car pipeline)))
+ ,(unless notfirst
+ '(setq headproc proc))
(setq tailproc (or tailproc proc))
proc))))))
@@ -823,7 +844,7 @@ 'eshell-process-identity
(defmacro eshell-execute-pipeline (pipeline)
"Execute the commands in PIPELINE, connecting each to one another."
- `(let ((eshell-in-pipeline-p t) tailproc)
+ `(let ((eshell-in-pipeline-p t) headproc tailproc)
(progn
,(if (fboundp 'make-process)
`(eshell-do-pipelines ,pipeline)
@@ -833,7 +854,7 @@ eshell-execute-pipeline
(car (aref eshell-current-handles
,eshell-error-handle)) nil)))
(eshell-do-pipelines-synchronously ,pipeline)))
- (eshell-process-identity tailproc))))
+ (eshell-process-identity (cons headproc tailproc)))))
(defmacro eshell-as-subcommand (command)
"Execute COMMAND using a temp buffer.
@@ -993,24 +1014,24 @@ eshell-resume-command
(unless (or (not (stringp status))
(string= "stopped" status)
(string-match eshell-reset-signals status))
- (if (eq proc (eshell-interactive-process))
+ (if (eq proc (eshell-tail-process))
(eshell-resume-eval)))))
(defun eshell-resume-eval ()
"Destructively evaluate a form which may need to be deferred."
(eshell-condition-case err
(progn
- (setq eshell-last-async-proc nil)
+ (setq eshell-last-async-procs nil)
(when eshell-current-command
(let* (retval
- (proc (catch 'eshell-defer
+ (procs (catch 'eshell-defer
(ignore
(setq retval
(eshell-do-eval
eshell-current-command))))))
- (if (eshell-processp proc)
- (ignore (setq eshell-last-async-proc proc))
- (cadr retval)))))
+ (if (eshell-process-pair-p procs)
+ (ignore (setq eshell-last-async-procs procs))
+ (cadr retval)))))
(error
(error (error-message-string err)))))
@@ -1173,17 +1194,16 @@ eshell-do-eval
(setcar form (car new-form))
(setcdr form (cdr new-form)))
(eshell-do-eval form synchronous-p))
- (if (and (memq (car form) eshell-deferrable-commands)
- (not eshell-current-subjob-p)
- result
- (eshell-processp result))
- (if synchronous-p
- (eshell/wait result)
+ (if-let (((memq (car form) eshell-deferrable-commands))
+ ((not eshell-current-subjob-p))
+ (procs (eshell-make-process-pair result)))
+ (if synchronous-p
+ (eshell/wait (cdr procs))
(eshell-manipulate "inserting ignore form"
(setcar form 'ignore)
(setcdr form nil))
- (throw 'eshell-defer result))
- (list 'quote result))))))))))))
+ (throw 'eshell-defer procs))
+ (list 'quote result))))))))))))
;; command invocation
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 2e0f312f4a..8e6463eac2 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -485,7 +485,7 @@ eshell-output-object-to-target
((eshell-processp target)
(when (eq (process-status target) 'run)
(unless (stringp object)
- (setq object (eshell-stringify object)))
+ (setq object (eshell-stringify object)))
(process-send-string target object)))
((consp target)
diff --git a/lisp/eshell/esh-mode.el b/lisp/eshell/esh-mode.el
index 8302eefe1e..59c8f8034f 100644
--- a/lisp/eshell/esh-mode.el
+++ b/lisp/eshell/esh-mode.el
@@ -423,13 +423,13 @@ eshell-toggle-direct-send
(defun eshell-self-insert-command ()
(interactive)
(process-send-string
- (eshell-interactive-process)
+ (eshell-head-process)
(char-to-string (if (symbolp last-command-event)
(get last-command-event 'ascii-character)
last-command-event))))
(defun eshell-intercept-commands ()
- (when (and (eshell-interactive-process)
+ (when (and (eshell-interactive-process-p)
(not (and (integerp last-input-event)
(memq last-input-event '(?\C-x ?\C-c)))))
(let ((possible-events (where-is-internal this-command))
@@ -595,13 +595,13 @@ eshell-send-input
newline."
(interactive "P")
;; Note that the input string does not include its terminal newline.
- (let ((proc-running-p (and (eshell-interactive-process)
+ (let ((proc-running-p (and (eshell-head-process)
(not queue-p)))
(inhibit-point-motion-hooks t)
(inhibit-modification-hooks t))
(unless (and proc-running-p
(not (eq (process-status
- (eshell-interactive-process))
+ (eshell-head-process))
'run)))
(if (or proc-running-p
(>= (point) eshell-last-output-end))
@@ -627,8 +627,8 @@ eshell-send-input
(if (or eshell-send-direct-to-subprocesses
(= eshell-last-input-start eshell-last-input-end))
(unless no-newline
- (process-send-string (eshell-interactive-process) "\n"))
- (process-send-region (eshell-interactive-process)
+ (process-send-string (eshell-head-process) "\n"))
+ (process-send-region (eshell-head-process)
eshell-last-input-start
eshell-last-input-end)))
(if (= eshell-last-output-end (point))
@@ -665,6 +665,16 @@ eshell-send-input
(run-hooks 'eshell-post-command-hook)
(insert-and-inherit input)))))))))
+(defun eshell-send-eof-to-process ()
+ "Send EOF to the currently-running \"head\" process."
+ (interactive)
+ (require 'esh-mode)
+ (declare-function eshell-send-input "esh-mode"
+ (&optional use-region queue-p no-newline))
+ (eshell-send-input nil nil t)
+ (when (eshell-head-process)
+ (process-send-eof (eshell-head-process))))
+
(defsubst eshell-kill-new ()
"Add the last input text to the kill ring."
(kill-ring-save eshell-last-input-start eshell-last-input-end))
@@ -924,9 +934,9 @@ eshell-send-invisible
(interactive) ; Don't pass str as argument, to avoid snooping via C-x ESC ESC
(let ((str (read-passwd
(format "%s Password: "
- (process-name (eshell-interactive-process))))))
+ (process-name (eshell-head-process))))))
(if (stringp str)
- (process-send-string (eshell-interactive-process)
+ (process-send-string (eshell-head-process)
(concat str "\n"))
(message "Warning: text will be echoed"))))
@@ -937,7 +947,7 @@ eshell-watch-for-password-prompt
`eshell-password-prompt-regexp'.
This function could be in the list `eshell-output-filter-functions'."
- (when (eshell-interactive-process)
+ (when (eshell-interactive-process-p)
(save-excursion
(let ((case-fold-search t))
(goto-char eshell-last-output-block-begin)
diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index 5ed692fb5a..bb2136c06c 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -101,6 +101,8 @@ eshell-current-subjob-p
(defvar eshell-process-list nil
"A list of the current status of subprocesses.")
+(declare-function eshell-send-eof-to-process "esh-mode")
+
(defvar-keymap eshell-proc-mode-map
"C-c M-i" #'eshell-insert-process
"C-c C-c" #'eshell-interrupt-process
@@ -542,14 +544,5 @@ eshell-quit-process
; ;; `eshell-resume-eval'.
; (eshell-kill-process-function nil "continue")))
-(defun eshell-send-eof-to-process ()
- "Send EOF to process."
- (interactive)
- (require 'esh-mode)
- (declare-function eshell-send-input "esh-mode"
- (&optional use-region queue-p no-newline))
- (eshell-send-input nil nil t)
- (eshell-process-interact 'process-send-eof))
-
(provide 'esh-proc)
;;; esh-proc.el ends here
diff --git a/lisp/eshell/esh-util.el b/lisp/eshell/esh-util.el
index 0e04dbc7c9..788404fc43 100644
--- a/lisp/eshell/esh-util.el
+++ b/lisp/eshell/esh-util.el
@@ -609,6 +609,20 @@ 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."
+ (pcase procs
+ ((pred eshell-processp) (cons procs procs))
+ ((pred eshell-process-pair-p) procs)))
+
;; (defun eshell-copy-file
;; (file newname &optional ok-if-already-exists keep-date)
;; "Copy FILE to NEWNAME. See docs for `copy-file'."
diff --git a/lisp/eshell/eshell.el b/lisp/eshell/eshell.el
index 5c356e8928..2c472a2afa 100644
--- a/lisp/eshell/eshell.el
+++ b/lisp/eshell/eshell.el
@@ -332,9 +332,9 @@ eshell-command
;; make the output as attractive as possible, with no
;; extraneous newlines
(when intr
- (if (eshell-interactive-process)
- (eshell-wait-for-process (eshell-interactive-process)))
- (cl-assert (not (eshell-interactive-process)))
+ (if (eshell-interactive-process-p)
+ (eshell-wait-for-process (eshell-tail-process)))
+ (cl-assert (not (eshell-interactive-process-p)))
(goto-char (point-max))
(while (and (bolp) (not (bobp)))
(delete-char -1)))
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index 7658d5f551..3b1bbe7188 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -136,6 +136,17 @@ eshell-test/pipe-tailproc
(eshell-command-result-p "*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)
+ (eshell-match-result "OLLEH\n")))
+
(ert-deftest eshell-test/window-height ()
"$LINES should equal (window-height)"
(should (eshell-test-command-result "= $LINES (window-height)")))
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in Eshell
2022-02-02 3:32 bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in Eshell Jim Porter
@ 2022-02-02 18:10 ` Lars Ingebrigtsen
2022-02-02 19:41 ` Jim Porter
0 siblings, 1 reply; 9+ messages in thread
From: Lars Ingebrigtsen @ 2022-02-02 18:10 UTC (permalink / raw)
To: Jim Porter; +Cc: 53715
Jim Porter <jporterbugs@gmail.com> writes:
> This happens because Eshell only keeps track of the tail process when
> running an external process, so when sending stdin, it gets sent to
> the tail process (`rev'), not the head (`tr'). I fixed this in the
> second patch by recording both the head and tail processes and
> updating the various places that use the current process to use the
> right one.
These patches seem to lead to a number of tests failing:
Test eshell-test/last-result-var2 condition:
(void-variable eshell-last-async-proc)
FAILED 22/34 eshell-test/last-result-var2 (0.001074 sec) at lisp/eshell/eshell-tests.el:164
passed 23/34 eshell-test/lisp-command (0.000350 sec)
passed 24/34 eshell-test/lisp-command-args (0.000308 sec)
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in Eshell
2022-02-02 18:10 ` Lars Ingebrigtsen
@ 2022-02-02 19:41 ` Jim Porter
2022-02-02 19:49 ` Lars Ingebrigtsen
0 siblings, 1 reply; 9+ messages in thread
From: Jim Porter @ 2022-02-02 19:41 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: 53715
On 2/2/2022 10:10 AM, Lars Ingebrigtsen wrote:
> These patches seem to lead to a number of tests failing:
>
> Test eshell-test/last-result-var2 condition:
> (void-variable eshell-last-async-proc)
> FAILED 22/34 eshell-test/last-result-var2 (0.001074 sec) at lisp/eshell/eshell-tests.el:164
> passed 23/34 eshell-test/lisp-command (0.000350 sec)
> passed 24/34 eshell-test/lisp-command-args (0.000308 sec)
Sorry, I forgot to mention that since the second patch updates a
defsubst (`eshell-interactive-process'), you'll need to recompile all
the files that might use it, so `make boostrap' or `touch
lisp/eshell/*.el test/lisp/eshell/*.el && make'. That should make these
tests pass.
Once this merges, I'll send a message to emacs-devel to let everyone
know to recompile these files so there are no surprises when they try to
use Eshell later on.
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in Eshell
2022-02-02 19:41 ` Jim Porter
@ 2022-02-02 19:49 ` Lars Ingebrigtsen
2022-02-02 21:01 ` Jim Porter
0 siblings, 1 reply; 9+ messages in thread
From: Lars Ingebrigtsen @ 2022-02-02 19:49 UTC (permalink / raw)
To: Jim Porter; +Cc: 53715
Jim Porter <jporterbugs@gmail.com> writes:
> Sorry, I forgot to mention that since the second patch updates a
> defsubst (`eshell-interactive-process'), you'll need to recompile all
> the files that might use it, so `make boostrap' or `touch
> lisp/eshell/*.el test/lisp/eshell/*.el && make'. That should make
> these tests pass.
I did remove all the .elc files in lisp/eshell and rebuilt, but the
tests still failed. (I didn't try to delete the test elc files,
though.)
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in Eshell
2022-02-02 19:49 ` Lars Ingebrigtsen
@ 2022-02-02 21:01 ` Jim Porter
2022-02-03 2:35 ` Jim Porter
0 siblings, 1 reply; 9+ messages in thread
From: Jim Porter @ 2022-02-02 21:01 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: 53715
[-- Attachment #1: Type: text/plain, Size: 874 bytes --]
On 2/2/2022 11:49 AM, Lars Ingebrigtsen wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
>
>> Sorry, I forgot to mention that since the second patch updates a
>> defsubst (`eshell-interactive-process'), you'll need to recompile all
>> the files that might use it, so `make boostrap' or `touch
>> lisp/eshell/*.el test/lisp/eshell/*.el && make'. That should make
>> these tests pass.
>
> I did remove all the .elc files in lisp/eshell and rebuilt, but the
> tests still failed. (I didn't try to delete the test elc files,
> though.)
Ah, I think I see the issue. I should have updated
`eshell-wait-for-subprocess' in test/lisp/eshell/eshell-tests-helpers.el
to use the new defsubst, which would have caused it to get recompiled.
(Though manually recompiling it should also work.)
I've attached a fixed patch (the first one is the same; I only updated
the second).
[-- Attachment #2: 0001-Ensure-that-tailproc-is-set-for-the-last-process-in-.patch --]
[-- Type: text/plain, Size: 2697 bytes --]
From 6f15832f537035d26d072595bc59ff8795c9550f Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 27 Jan 2022 23:13:36 -0800
Subject: [PATCH 1/2] Ensure that tailproc is set for the last process in an
Eshell pipeline
In particular, this used to fail for pipelines where the last process
in the pipeline came from the first element of the pipeline. This
could happen when a process was piped to an ordinary Lisp function,
like in '*echo hi | echo bye'.
* lisp/eshell/esh-cmd.el (eshell-do-pipelines): Set the tailproc even
for the first process in the pipeline.
* test/lisp/eshell/eshell-tests.el (eshell-test/pipe-tailproc): New
test.
---
lisp/eshell/esh-cmd.el | 7 ++++---
test/lisp/eshell/eshell-tests.el | 7 +++++++
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 04d65df4f3..14139896dd 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -764,8 +764,7 @@ eshell-do-pipelines
(eshell-set-output-handle ,eshell-output-handle
'append nextproc)
(eshell-set-output-handle ,eshell-error-handle
- 'append nextproc)
- (setq tailproc (or tailproc nextproc))))
+ 'append nextproc)))
,(let ((head (car pipeline)))
(if (memq (car head) '(let progn))
(setq head (car (last head))))
@@ -781,7 +780,9 @@ eshell-do-pipelines
,(cond ((not notfirst) (quote 'first))
((cdr pipeline) t)
(t (quote 'last)))))
- ,(car pipeline))))))
+ (let ((proc ,(car pipeline)))
+ (setq tailproc (or tailproc proc))
+ proc))))))
(defmacro eshell-do-pipelines-synchronously (pipeline)
"Execute the commands in PIPELINE in sequence synchronously.
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index 542815df80..7658d5f551 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -129,6 +129,13 @@ eshell-test/interp-cmd-external-concat
(eshell-command-result-p "echo ${echo hi}-${*echo there}"
"hi-there\n")))
+(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-command-result-p "*echo hi | echo bye"
+ "bye\nhi\n")))
+
(ert-deftest eshell-test/window-height ()
"$LINES should equal (window-height)"
(should (eshell-test-command-result "= $LINES (window-height)")))
--
2.25.1
[-- Attachment #3: 0002-When-executing-an-Eshell-pipeline-send-input-to-the-.patch --]
[-- Type: text/plain, Size: 17310 bytes --]
From 433ea7b6c50a856746e6e1a3b9bd7289a343aebc Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 30 Jan 2022 18:53:53 -0800
Subject: [PATCH 2/2] When executing an Eshell pipeline, send input to the
first process
Previously, input was sent to the last process in the pipeline,
resulting in unexpected behavior when running commands like
'tr a-z A-Z | rev'.
* lisp/eshell/esh-util.el (eshell-process-pair-p)
(eshell-make-process-pair): New functions.
* lisp/eshell/esh-cmd.el (eshell-last-async-proc): Rename to...
(eshell-last-async-procs): ... this, and store a pair of processes.
(eshell-interactive-process): Replace with...
(eshell-interactive-process-p, eshell-head-process)
(eshell-tail-process): ... these.
(eshell-cmd-initialize): Set 'eshell-last-async-procs'.
(eshell-do-pipelines): Set 'headproc'.
(eshell-execute-pipeline): Return 'headproc' and 'tailproc'.
(eshell-resume-eval): Use 'eshell-last-async-procs'.
(eshell-do-eval): Make sure we work with a pair of processes.
* lisp/eshell/esh-proc.el (eshell-send-eof-to-process): Move from
here...
* lisp/eshell/esh-mode.el (eshell-send-eof-to-process): ... to here,
and only send EOF to the head process.
* lisp/eshell/em-cmpl.el (eshell-complete-parse-arguments)
* lisp/eshell/esh-mode.el (eshell-intercept-commands)
(eshell-watch-for-password-prompt):
Use 'eshell-interactive-process-p'.
* lisp/eshell/em-rebind.el (eshell-delchar-or-maybe-eof)
* lisp/eshell/em-term.el (eshell-term-send-raw-string)
* lisp/eshell/esh-mode.el (eshell-self-insert-command)
(eshell-send-input, eshell-send-invisible):
Use 'eshell-head-process'.
* lisp/eshell/esh-cmd.el (eshell-as-subcommand):
Use 'eshell-tail-process'.
* lisp/eshell/eshell.el (eshell-command):
* test/lisp/eshell/eshell-tests-helpers.el
(eshell-wait-for-subprocess):
Use 'eshell-interactive-process-p' and 'eshell-tail-process'.
* test/lisp/eshell/eshell-tests.el (eshell-test/pipe-headproc-stdin):
New test.
---
lisp/eshell/em-cmpl.el | 2 +-
lisp/eshell/em-rebind.el | 2 +-
lisp/eshell/em-term.el | 2 +-
lisp/eshell/esh-cmd.el | 66 +++++++++++++++---------
lisp/eshell/esh-io.el | 2 +-
lisp/eshell/esh-mode.el | 28 ++++++----
lisp/eshell/esh-proc.el | 11 +---
lisp/eshell/esh-util.el | 14 +++++
lisp/eshell/eshell.el | 6 +--
test/lisp/eshell/eshell-tests-helpers.el | 2 +-
test/lisp/eshell/eshell-tests.el | 11 ++++
11 files changed, 97 insertions(+), 49 deletions(-)
diff --git a/lisp/eshell/em-cmpl.el b/lisp/eshell/em-cmpl.el
index c6a51b1793..b79475f6e0 100644
--- a/lisp/eshell/em-cmpl.el
+++ b/lisp/eshell/em-cmpl.el
@@ -314,7 +314,7 @@ eshell-completion-help
(defun eshell-complete-parse-arguments ()
"Parse the command line arguments for `pcomplete-argument'."
(when (and eshell-no-completion-during-jobs
- (eshell-interactive-process))
+ (eshell-interactive-process-p))
(insert-and-inherit "\t")
(throw 'pcompleted t))
(let ((end (point-marker))
diff --git a/lisp/eshell/em-rebind.el b/lisp/eshell/em-rebind.el
index f24758d4e3..2b56c9e844 100644
--- a/lisp/eshell/em-rebind.el
+++ b/lisp/eshell/em-rebind.el
@@ -238,7 +238,7 @@ eshell-delchar-or-maybe-eof
Sends an EOF only if point is at the end of the buffer and there is no
input."
(interactive "p")
- (let ((proc (eshell-interactive-process)))
+ (let ((proc (eshell-head-process)))
(if (eobp)
(cond
((/= (point) eshell-last-output-end)
diff --git a/lisp/eshell/em-term.el b/lisp/eshell/em-term.el
index e34c5ae47c..d150c07b03 100644
--- a/lisp/eshell/em-term.el
+++ b/lisp/eshell/em-term.el
@@ -224,7 +224,7 @@ eshell-term-sentinel
; (defun eshell-term-send-raw-string (chars)
; (goto-char eshell-last-output-end)
-; (process-send-string (eshell-interactive-process) chars))
+; (process-send-string (eshell-head-process) chars))
; (defun eshell-term-send-raw ()
; "Send the last character typed through the terminal-emulator
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 14139896dd..e702de03a0 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -279,14 +279,33 @@ eshell-in-pipeline-p
(defvar eshell-in-subcommand-p nil)
(defvar eshell-last-arguments nil)
(defvar eshell-last-command-name nil)
-(defvar eshell-last-async-proc nil
- "When this foreground process completes, resume command evaluation.")
+(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.")
;;; Functions:
-(defsubst eshell-interactive-process ()
- "Return currently running command process, if non-Lisp."
- eshell-last-async-proc)
+(defsubst eshell-interactive-process-p ()
+ "Return non-nil if there is a currently running command process."
+ eshell-last-async-procs)
+
+(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))
+
+(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))
+
+(define-obsolete-function-alias 'eshell-interactive-process
+ 'eshell-tail-process "29.1")
(defun eshell-cmd-initialize () ;Called from `eshell-mode' via intern-soft!
"Initialize the Eshell command processing module."
@@ -295,7 +314,7 @@ eshell-cmd-initialize
(setq-local eshell-command-arguments nil)
(setq-local eshell-last-arguments nil)
(setq-local eshell-last-command-name nil)
- (setq-local eshell-last-async-proc nil)
+ (setq-local eshell-last-async-procs nil)
(add-hook 'eshell-kill-hook #'eshell-resume-command nil t)
@@ -306,7 +325,7 @@ eshell-cmd-initialize
(add-hook 'eshell-post-command-hook
(lambda ()
(setq eshell-current-command nil
- eshell-last-async-proc nil))
+ eshell-last-async-procs nil))
nil t)
(add-hook 'eshell-parse-argument-hook
@@ -781,6 +800,8 @@ eshell-do-pipelines
((cdr pipeline) t)
(t (quote 'last)))))
(let ((proc ,(car pipeline)))
+ ,(unless notfirst
+ '(setq headproc proc))
(setq tailproc (or tailproc proc))
proc))))))
@@ -823,7 +844,7 @@ 'eshell-process-identity
(defmacro eshell-execute-pipeline (pipeline)
"Execute the commands in PIPELINE, connecting each to one another."
- `(let ((eshell-in-pipeline-p t) tailproc)
+ `(let ((eshell-in-pipeline-p t) headproc tailproc)
(progn
,(if (fboundp 'make-process)
`(eshell-do-pipelines ,pipeline)
@@ -833,7 +854,7 @@ eshell-execute-pipeline
(car (aref eshell-current-handles
,eshell-error-handle)) nil)))
(eshell-do-pipelines-synchronously ,pipeline)))
- (eshell-process-identity tailproc))))
+ (eshell-process-identity (cons headproc tailproc)))))
(defmacro eshell-as-subcommand (command)
"Execute COMMAND using a temp buffer.
@@ -993,24 +1014,24 @@ eshell-resume-command
(unless (or (not (stringp status))
(string= "stopped" status)
(string-match eshell-reset-signals status))
- (if (eq proc (eshell-interactive-process))
+ (if (eq proc (eshell-tail-process))
(eshell-resume-eval)))))
(defun eshell-resume-eval ()
"Destructively evaluate a form which may need to be deferred."
(eshell-condition-case err
(progn
- (setq eshell-last-async-proc nil)
+ (setq eshell-last-async-procs nil)
(when eshell-current-command
(let* (retval
- (proc (catch 'eshell-defer
+ (procs (catch 'eshell-defer
(ignore
(setq retval
(eshell-do-eval
eshell-current-command))))))
- (if (eshell-processp proc)
- (ignore (setq eshell-last-async-proc proc))
- (cadr retval)))))
+ (if (eshell-process-pair-p procs)
+ (ignore (setq eshell-last-async-procs procs))
+ (cadr retval)))))
(error
(error (error-message-string err)))))
@@ -1173,17 +1194,16 @@ eshell-do-eval
(setcar form (car new-form))
(setcdr form (cdr new-form)))
(eshell-do-eval form synchronous-p))
- (if (and (memq (car form) eshell-deferrable-commands)
- (not eshell-current-subjob-p)
- result
- (eshell-processp result))
- (if synchronous-p
- (eshell/wait result)
+ (if-let (((memq (car form) eshell-deferrable-commands))
+ ((not eshell-current-subjob-p))
+ (procs (eshell-make-process-pair result)))
+ (if synchronous-p
+ (eshell/wait (cdr procs))
(eshell-manipulate "inserting ignore form"
(setcar form 'ignore)
(setcdr form nil))
- (throw 'eshell-defer result))
- (list 'quote result))))))))))))
+ (throw 'eshell-defer procs))
+ (list 'quote result))))))))))))
;; command invocation
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 2e0f312f4a..8e6463eac2 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -485,7 +485,7 @@ eshell-output-object-to-target
((eshell-processp target)
(when (eq (process-status target) 'run)
(unless (stringp object)
- (setq object (eshell-stringify object)))
+ (setq object (eshell-stringify object)))
(process-send-string target object)))
((consp target)
diff --git a/lisp/eshell/esh-mode.el b/lisp/eshell/esh-mode.el
index 8302eefe1e..59c8f8034f 100644
--- a/lisp/eshell/esh-mode.el
+++ b/lisp/eshell/esh-mode.el
@@ -423,13 +423,13 @@ eshell-toggle-direct-send
(defun eshell-self-insert-command ()
(interactive)
(process-send-string
- (eshell-interactive-process)
+ (eshell-head-process)
(char-to-string (if (symbolp last-command-event)
(get last-command-event 'ascii-character)
last-command-event))))
(defun eshell-intercept-commands ()
- (when (and (eshell-interactive-process)
+ (when (and (eshell-interactive-process-p)
(not (and (integerp last-input-event)
(memq last-input-event '(?\C-x ?\C-c)))))
(let ((possible-events (where-is-internal this-command))
@@ -595,13 +595,13 @@ eshell-send-input
newline."
(interactive "P")
;; Note that the input string does not include its terminal newline.
- (let ((proc-running-p (and (eshell-interactive-process)
+ (let ((proc-running-p (and (eshell-head-process)
(not queue-p)))
(inhibit-point-motion-hooks t)
(inhibit-modification-hooks t))
(unless (and proc-running-p
(not (eq (process-status
- (eshell-interactive-process))
+ (eshell-head-process))
'run)))
(if (or proc-running-p
(>= (point) eshell-last-output-end))
@@ -627,8 +627,8 @@ eshell-send-input
(if (or eshell-send-direct-to-subprocesses
(= eshell-last-input-start eshell-last-input-end))
(unless no-newline
- (process-send-string (eshell-interactive-process) "\n"))
- (process-send-region (eshell-interactive-process)
+ (process-send-string (eshell-head-process) "\n"))
+ (process-send-region (eshell-head-process)
eshell-last-input-start
eshell-last-input-end)))
(if (= eshell-last-output-end (point))
@@ -665,6 +665,16 @@ eshell-send-input
(run-hooks 'eshell-post-command-hook)
(insert-and-inherit input)))))))))
+(defun eshell-send-eof-to-process ()
+ "Send EOF to the currently-running \"head\" process."
+ (interactive)
+ (require 'esh-mode)
+ (declare-function eshell-send-input "esh-mode"
+ (&optional use-region queue-p no-newline))
+ (eshell-send-input nil nil t)
+ (when (eshell-head-process)
+ (process-send-eof (eshell-head-process))))
+
(defsubst eshell-kill-new ()
"Add the last input text to the kill ring."
(kill-ring-save eshell-last-input-start eshell-last-input-end))
@@ -924,9 +934,9 @@ eshell-send-invisible
(interactive) ; Don't pass str as argument, to avoid snooping via C-x ESC ESC
(let ((str (read-passwd
(format "%s Password: "
- (process-name (eshell-interactive-process))))))
+ (process-name (eshell-head-process))))))
(if (stringp str)
- (process-send-string (eshell-interactive-process)
+ (process-send-string (eshell-head-process)
(concat str "\n"))
(message "Warning: text will be echoed"))))
@@ -937,7 +947,7 @@ eshell-watch-for-password-prompt
`eshell-password-prompt-regexp'.
This function could be in the list `eshell-output-filter-functions'."
- (when (eshell-interactive-process)
+ (when (eshell-interactive-process-p)
(save-excursion
(let ((case-fold-search t))
(goto-char eshell-last-output-block-begin)
diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index 5ed692fb5a..bb2136c06c 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -101,6 +101,8 @@ eshell-current-subjob-p
(defvar eshell-process-list nil
"A list of the current status of subprocesses.")
+(declare-function eshell-send-eof-to-process "esh-mode")
+
(defvar-keymap eshell-proc-mode-map
"C-c M-i" #'eshell-insert-process
"C-c C-c" #'eshell-interrupt-process
@@ -542,14 +544,5 @@ eshell-quit-process
; ;; `eshell-resume-eval'.
; (eshell-kill-process-function nil "continue")))
-(defun eshell-send-eof-to-process ()
- "Send EOF to process."
- (interactive)
- (require 'esh-mode)
- (declare-function eshell-send-input "esh-mode"
- (&optional use-region queue-p no-newline))
- (eshell-send-input nil nil t)
- (eshell-process-interact 'process-send-eof))
-
(provide 'esh-proc)
;;; esh-proc.el ends here
diff --git a/lisp/eshell/esh-util.el b/lisp/eshell/esh-util.el
index 0e04dbc7c9..788404fc43 100644
--- a/lisp/eshell/esh-util.el
+++ b/lisp/eshell/esh-util.el
@@ -609,6 +609,20 @@ 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."
+ (pcase procs
+ ((pred eshell-processp) (cons procs procs))
+ ((pred eshell-process-pair-p) procs)))
+
;; (defun eshell-copy-file
;; (file newname &optional ok-if-already-exists keep-date)
;; "Copy FILE to NEWNAME. See docs for `copy-file'."
diff --git a/lisp/eshell/eshell.el b/lisp/eshell/eshell.el
index 5c356e8928..2c472a2afa 100644
--- a/lisp/eshell/eshell.el
+++ b/lisp/eshell/eshell.el
@@ -332,9 +332,9 @@ eshell-command
;; make the output as attractive as possible, with no
;; extraneous newlines
(when intr
- (if (eshell-interactive-process)
- (eshell-wait-for-process (eshell-interactive-process)))
- (cl-assert (not (eshell-interactive-process)))
+ (if (eshell-interactive-process-p)
+ (eshell-wait-for-process (eshell-tail-process)))
+ (cl-assert (not (eshell-interactive-process-p)))
(goto-char (point-max))
(while (and (bolp) (not (bobp)))
(delete-char -1)))
diff --git a/test/lisp/eshell/eshell-tests-helpers.el b/test/lisp/eshell/eshell-tests-helpers.el
index 77f5313d57..f3fbe90356 100644
--- a/test/lisp/eshell/eshell-tests-helpers.el
+++ b/test/lisp/eshell/eshell-tests-helpers.el
@@ -53,7 +53,7 @@ eshell-wait-for-subprocess
If this takes longer than `eshell-test--max-subprocess-time',
raise an error."
(let ((start (current-time)))
- (while (eshell-interactive-process)
+ (while (eshell-interactive-process-p)
(when (> (float-time (time-since start))
eshell-test--max-subprocess-time)
(error "timed out waiting for subprocess"))
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index 7658d5f551..3b1bbe7188 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -136,6 +136,17 @@ eshell-test/pipe-tailproc
(eshell-command-result-p "*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)
+ (eshell-match-result "OLLEH\n")))
+
(ert-deftest eshell-test/window-height ()
"$LINES should equal (window-height)"
(should (eshell-test-command-result "= $LINES (window-height)")))
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in Eshell
2022-02-02 21:01 ` Jim Porter
@ 2022-02-03 2:35 ` Jim Porter
2022-02-03 19:03 ` Lars Ingebrigtsen
0 siblings, 1 reply; 9+ messages in thread
From: Jim Porter @ 2022-02-03 2:35 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: 53715
[-- Attachment #1: Type: text/plain, Size: 783 bytes --]
On 2/2/2022 1:01 PM, Jim Porter wrote:
> Ah, I think I see the issue. I should have updated
> `eshell-wait-for-subprocess' in test/lisp/eshell/eshell-tests-helpers.el
> to use the new defsubst, which would have caused it to get recompiled.
> (Though manually recompiling it should also work.)
>
> I've attached a fixed patch (the first one is the same; I only updated
> the second).
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.
[-- Attachment #2: 0001-Ensure-that-tailproc-is-set-for-the-last-process-in-.patch --]
[-- Type: text/plain, Size: 2697 bytes --]
From bda5d1dbe322b8fdcac601753f74d1a62d9b256c Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 27 Jan 2022 23:13:36 -0800
Subject: [PATCH 1/3] Ensure that tailproc is set for the last process in an
Eshell pipeline
In particular, this used to fail for pipelines where the last process
in the pipeline came from the first element of the pipeline. This
could happen when a process was piped to an ordinary Lisp function,
like in '*echo hi | echo bye'.
* lisp/eshell/esh-cmd.el (eshell-do-pipelines): Set the tailproc even
for the first process in the pipeline.
* test/lisp/eshell/eshell-tests.el (eshell-test/pipe-tailproc): New
test.
---
lisp/eshell/esh-cmd.el | 7 ++++---
test/lisp/eshell/eshell-tests.el | 7 +++++++
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 04d65df4f3..14139896dd 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -764,8 +764,7 @@ eshell-do-pipelines
(eshell-set-output-handle ,eshell-output-handle
'append nextproc)
(eshell-set-output-handle ,eshell-error-handle
- 'append nextproc)
- (setq tailproc (or tailproc nextproc))))
+ 'append nextproc)))
,(let ((head (car pipeline)))
(if (memq (car head) '(let progn))
(setq head (car (last head))))
@@ -781,7 +780,9 @@ eshell-do-pipelines
,(cond ((not notfirst) (quote 'first))
((cdr pipeline) t)
(t (quote 'last)))))
- ,(car pipeline))))))
+ (let ((proc ,(car pipeline)))
+ (setq tailproc (or tailproc proc))
+ proc))))))
(defmacro eshell-do-pipelines-synchronously (pipeline)
"Execute the commands in PIPELINE in sequence synchronously.
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index 542815df80..7658d5f551 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -129,6 +129,13 @@ eshell-test/interp-cmd-external-concat
(eshell-command-result-p "echo ${echo hi}-${*echo there}"
"hi-there\n")))
+(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-command-result-p "*echo hi | echo bye"
+ "bye\nhi\n")))
+
(ert-deftest eshell-test/window-height ()
"$LINES should equal (window-height)"
(should (eshell-test-command-result "= $LINES (window-height)")))
--
2.25.1
[-- Attachment #3: 0002-When-executing-an-Eshell-pipeline-send-input-to-the-.patch --]
[-- Type: text/plain, Size: 17310 bytes --]
From 78b8cc926e978f9e60efe4193f1f29019090455a Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 30 Jan 2022 18:53:53 -0800
Subject: [PATCH 2/3] When executing an Eshell pipeline, send input to the
first process
Previously, input was sent to the last process in the pipeline,
resulting in unexpected behavior when running commands like
'tr a-z A-Z | rev'.
* lisp/eshell/esh-util.el (eshell-process-pair-p)
(eshell-make-process-pair): New functions.
* lisp/eshell/esh-cmd.el (eshell-last-async-proc): Rename to...
(eshell-last-async-procs): ... this, and store a pair of processes.
(eshell-interactive-process): Replace with...
(eshell-interactive-process-p, eshell-head-process)
(eshell-tail-process): ... these.
(eshell-cmd-initialize): Set 'eshell-last-async-procs'.
(eshell-do-pipelines): Set 'headproc'.
(eshell-execute-pipeline): Return 'headproc' and 'tailproc'.
(eshell-resume-eval): Use 'eshell-last-async-procs'.
(eshell-do-eval): Make sure we work with a pair of processes.
* lisp/eshell/esh-proc.el (eshell-send-eof-to-process): Move from
here...
* lisp/eshell/esh-mode.el (eshell-send-eof-to-process): ... to here,
and only send EOF to the head process.
* lisp/eshell/em-cmpl.el (eshell-complete-parse-arguments)
* lisp/eshell/esh-mode.el (eshell-intercept-commands)
(eshell-watch-for-password-prompt):
Use 'eshell-interactive-process-p'.
* lisp/eshell/em-rebind.el (eshell-delchar-or-maybe-eof)
* lisp/eshell/em-term.el (eshell-term-send-raw-string)
* lisp/eshell/esh-mode.el (eshell-self-insert-command)
(eshell-send-input, eshell-send-invisible):
Use 'eshell-head-process'.
* lisp/eshell/esh-cmd.el (eshell-as-subcommand):
Use 'eshell-tail-process'.
* lisp/eshell/eshell.el (eshell-command):
* test/lisp/eshell/eshell-tests-helpers.el
(eshell-wait-for-subprocess):
Use 'eshell-interactive-process-p' and 'eshell-tail-process'.
* test/lisp/eshell/eshell-tests.el (eshell-test/pipe-headproc-stdin):
New test.
---
lisp/eshell/em-cmpl.el | 2 +-
lisp/eshell/em-rebind.el | 2 +-
lisp/eshell/em-term.el | 2 +-
lisp/eshell/esh-cmd.el | 66 +++++++++++++++---------
lisp/eshell/esh-io.el | 2 +-
lisp/eshell/esh-mode.el | 28 ++++++----
lisp/eshell/esh-proc.el | 11 +---
lisp/eshell/esh-util.el | 14 +++++
lisp/eshell/eshell.el | 6 +--
test/lisp/eshell/eshell-tests-helpers.el | 2 +-
test/lisp/eshell/eshell-tests.el | 11 ++++
11 files changed, 97 insertions(+), 49 deletions(-)
diff --git a/lisp/eshell/em-cmpl.el b/lisp/eshell/em-cmpl.el
index c6a51b1793..b79475f6e0 100644
--- a/lisp/eshell/em-cmpl.el
+++ b/lisp/eshell/em-cmpl.el
@@ -314,7 +314,7 @@ eshell-completion-help
(defun eshell-complete-parse-arguments ()
"Parse the command line arguments for `pcomplete-argument'."
(when (and eshell-no-completion-during-jobs
- (eshell-interactive-process))
+ (eshell-interactive-process-p))
(insert-and-inherit "\t")
(throw 'pcompleted t))
(let ((end (point-marker))
diff --git a/lisp/eshell/em-rebind.el b/lisp/eshell/em-rebind.el
index f24758d4e3..2b56c9e844 100644
--- a/lisp/eshell/em-rebind.el
+++ b/lisp/eshell/em-rebind.el
@@ -238,7 +238,7 @@ eshell-delchar-or-maybe-eof
Sends an EOF only if point is at the end of the buffer and there is no
input."
(interactive "p")
- (let ((proc (eshell-interactive-process)))
+ (let ((proc (eshell-head-process)))
(if (eobp)
(cond
((/= (point) eshell-last-output-end)
diff --git a/lisp/eshell/em-term.el b/lisp/eshell/em-term.el
index e34c5ae47c..d150c07b03 100644
--- a/lisp/eshell/em-term.el
+++ b/lisp/eshell/em-term.el
@@ -224,7 +224,7 @@ eshell-term-sentinel
; (defun eshell-term-send-raw-string (chars)
; (goto-char eshell-last-output-end)
-; (process-send-string (eshell-interactive-process) chars))
+; (process-send-string (eshell-head-process) chars))
; (defun eshell-term-send-raw ()
; "Send the last character typed through the terminal-emulator
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 14139896dd..e702de03a0 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -279,14 +279,33 @@ eshell-in-pipeline-p
(defvar eshell-in-subcommand-p nil)
(defvar eshell-last-arguments nil)
(defvar eshell-last-command-name nil)
-(defvar eshell-last-async-proc nil
- "When this foreground process completes, resume command evaluation.")
+(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.")
;;; Functions:
-(defsubst eshell-interactive-process ()
- "Return currently running command process, if non-Lisp."
- eshell-last-async-proc)
+(defsubst eshell-interactive-process-p ()
+ "Return non-nil if there is a currently running command process."
+ eshell-last-async-procs)
+
+(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))
+
+(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))
+
+(define-obsolete-function-alias 'eshell-interactive-process
+ 'eshell-tail-process "29.1")
(defun eshell-cmd-initialize () ;Called from `eshell-mode' via intern-soft!
"Initialize the Eshell command processing module."
@@ -295,7 +314,7 @@ eshell-cmd-initialize
(setq-local eshell-command-arguments nil)
(setq-local eshell-last-arguments nil)
(setq-local eshell-last-command-name nil)
- (setq-local eshell-last-async-proc nil)
+ (setq-local eshell-last-async-procs nil)
(add-hook 'eshell-kill-hook #'eshell-resume-command nil t)
@@ -306,7 +325,7 @@ eshell-cmd-initialize
(add-hook 'eshell-post-command-hook
(lambda ()
(setq eshell-current-command nil
- eshell-last-async-proc nil))
+ eshell-last-async-procs nil))
nil t)
(add-hook 'eshell-parse-argument-hook
@@ -781,6 +800,8 @@ eshell-do-pipelines
((cdr pipeline) t)
(t (quote 'last)))))
(let ((proc ,(car pipeline)))
+ ,(unless notfirst
+ '(setq headproc proc))
(setq tailproc (or tailproc proc))
proc))))))
@@ -823,7 +844,7 @@ 'eshell-process-identity
(defmacro eshell-execute-pipeline (pipeline)
"Execute the commands in PIPELINE, connecting each to one another."
- `(let ((eshell-in-pipeline-p t) tailproc)
+ `(let ((eshell-in-pipeline-p t) headproc tailproc)
(progn
,(if (fboundp 'make-process)
`(eshell-do-pipelines ,pipeline)
@@ -833,7 +854,7 @@ eshell-execute-pipeline
(car (aref eshell-current-handles
,eshell-error-handle)) nil)))
(eshell-do-pipelines-synchronously ,pipeline)))
- (eshell-process-identity tailproc))))
+ (eshell-process-identity (cons headproc tailproc)))))
(defmacro eshell-as-subcommand (command)
"Execute COMMAND using a temp buffer.
@@ -993,24 +1014,24 @@ eshell-resume-command
(unless (or (not (stringp status))
(string= "stopped" status)
(string-match eshell-reset-signals status))
- (if (eq proc (eshell-interactive-process))
+ (if (eq proc (eshell-tail-process))
(eshell-resume-eval)))))
(defun eshell-resume-eval ()
"Destructively evaluate a form which may need to be deferred."
(eshell-condition-case err
(progn
- (setq eshell-last-async-proc nil)
+ (setq eshell-last-async-procs nil)
(when eshell-current-command
(let* (retval
- (proc (catch 'eshell-defer
+ (procs (catch 'eshell-defer
(ignore
(setq retval
(eshell-do-eval
eshell-current-command))))))
- (if (eshell-processp proc)
- (ignore (setq eshell-last-async-proc proc))
- (cadr retval)))))
+ (if (eshell-process-pair-p procs)
+ (ignore (setq eshell-last-async-procs procs))
+ (cadr retval)))))
(error
(error (error-message-string err)))))
@@ -1173,17 +1194,16 @@ eshell-do-eval
(setcar form (car new-form))
(setcdr form (cdr new-form)))
(eshell-do-eval form synchronous-p))
- (if (and (memq (car form) eshell-deferrable-commands)
- (not eshell-current-subjob-p)
- result
- (eshell-processp result))
- (if synchronous-p
- (eshell/wait result)
+ (if-let (((memq (car form) eshell-deferrable-commands))
+ ((not eshell-current-subjob-p))
+ (procs (eshell-make-process-pair result)))
+ (if synchronous-p
+ (eshell/wait (cdr procs))
(eshell-manipulate "inserting ignore form"
(setcar form 'ignore)
(setcdr form nil))
- (throw 'eshell-defer result))
- (list 'quote result))))))))))))
+ (throw 'eshell-defer procs))
+ (list 'quote result))))))))))))
;; command invocation
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 2e0f312f4a..8e6463eac2 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -485,7 +485,7 @@ eshell-output-object-to-target
((eshell-processp target)
(when (eq (process-status target) 'run)
(unless (stringp object)
- (setq object (eshell-stringify object)))
+ (setq object (eshell-stringify object)))
(process-send-string target object)))
((consp target)
diff --git a/lisp/eshell/esh-mode.el b/lisp/eshell/esh-mode.el
index 8302eefe1e..59c8f8034f 100644
--- a/lisp/eshell/esh-mode.el
+++ b/lisp/eshell/esh-mode.el
@@ -423,13 +423,13 @@ eshell-toggle-direct-send
(defun eshell-self-insert-command ()
(interactive)
(process-send-string
- (eshell-interactive-process)
+ (eshell-head-process)
(char-to-string (if (symbolp last-command-event)
(get last-command-event 'ascii-character)
last-command-event))))
(defun eshell-intercept-commands ()
- (when (and (eshell-interactive-process)
+ (when (and (eshell-interactive-process-p)
(not (and (integerp last-input-event)
(memq last-input-event '(?\C-x ?\C-c)))))
(let ((possible-events (where-is-internal this-command))
@@ -595,13 +595,13 @@ eshell-send-input
newline."
(interactive "P")
;; Note that the input string does not include its terminal newline.
- (let ((proc-running-p (and (eshell-interactive-process)
+ (let ((proc-running-p (and (eshell-head-process)
(not queue-p)))
(inhibit-point-motion-hooks t)
(inhibit-modification-hooks t))
(unless (and proc-running-p
(not (eq (process-status
- (eshell-interactive-process))
+ (eshell-head-process))
'run)))
(if (or proc-running-p
(>= (point) eshell-last-output-end))
@@ -627,8 +627,8 @@ eshell-send-input
(if (or eshell-send-direct-to-subprocesses
(= eshell-last-input-start eshell-last-input-end))
(unless no-newline
- (process-send-string (eshell-interactive-process) "\n"))
- (process-send-region (eshell-interactive-process)
+ (process-send-string (eshell-head-process) "\n"))
+ (process-send-region (eshell-head-process)
eshell-last-input-start
eshell-last-input-end)))
(if (= eshell-last-output-end (point))
@@ -665,6 +665,16 @@ eshell-send-input
(run-hooks 'eshell-post-command-hook)
(insert-and-inherit input)))))))))
+(defun eshell-send-eof-to-process ()
+ "Send EOF to the currently-running \"head\" process."
+ (interactive)
+ (require 'esh-mode)
+ (declare-function eshell-send-input "esh-mode"
+ (&optional use-region queue-p no-newline))
+ (eshell-send-input nil nil t)
+ (when (eshell-head-process)
+ (process-send-eof (eshell-head-process))))
+
(defsubst eshell-kill-new ()
"Add the last input text to the kill ring."
(kill-ring-save eshell-last-input-start eshell-last-input-end))
@@ -924,9 +934,9 @@ eshell-send-invisible
(interactive) ; Don't pass str as argument, to avoid snooping via C-x ESC ESC
(let ((str (read-passwd
(format "%s Password: "
- (process-name (eshell-interactive-process))))))
+ (process-name (eshell-head-process))))))
(if (stringp str)
- (process-send-string (eshell-interactive-process)
+ (process-send-string (eshell-head-process)
(concat str "\n"))
(message "Warning: text will be echoed"))))
@@ -937,7 +947,7 @@ eshell-watch-for-password-prompt
`eshell-password-prompt-regexp'.
This function could be in the list `eshell-output-filter-functions'."
- (when (eshell-interactive-process)
+ (when (eshell-interactive-process-p)
(save-excursion
(let ((case-fold-search t))
(goto-char eshell-last-output-block-begin)
diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index 5ed692fb5a..bb2136c06c 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -101,6 +101,8 @@ eshell-current-subjob-p
(defvar eshell-process-list nil
"A list of the current status of subprocesses.")
+(declare-function eshell-send-eof-to-process "esh-mode")
+
(defvar-keymap eshell-proc-mode-map
"C-c M-i" #'eshell-insert-process
"C-c C-c" #'eshell-interrupt-process
@@ -542,14 +544,5 @@ eshell-quit-process
; ;; `eshell-resume-eval'.
; (eshell-kill-process-function nil "continue")))
-(defun eshell-send-eof-to-process ()
- "Send EOF to process."
- (interactive)
- (require 'esh-mode)
- (declare-function eshell-send-input "esh-mode"
- (&optional use-region queue-p no-newline))
- (eshell-send-input nil nil t)
- (eshell-process-interact 'process-send-eof))
-
(provide 'esh-proc)
;;; esh-proc.el ends here
diff --git a/lisp/eshell/esh-util.el b/lisp/eshell/esh-util.el
index 0e04dbc7c9..788404fc43 100644
--- a/lisp/eshell/esh-util.el
+++ b/lisp/eshell/esh-util.el
@@ -609,6 +609,20 @@ 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."
+ (pcase procs
+ ((pred eshell-processp) (cons procs procs))
+ ((pred eshell-process-pair-p) procs)))
+
;; (defun eshell-copy-file
;; (file newname &optional ok-if-already-exists keep-date)
;; "Copy FILE to NEWNAME. See docs for `copy-file'."
diff --git a/lisp/eshell/eshell.el b/lisp/eshell/eshell.el
index 5c356e8928..2c472a2afa 100644
--- a/lisp/eshell/eshell.el
+++ b/lisp/eshell/eshell.el
@@ -332,9 +332,9 @@ eshell-command
;; make the output as attractive as possible, with no
;; extraneous newlines
(when intr
- (if (eshell-interactive-process)
- (eshell-wait-for-process (eshell-interactive-process)))
- (cl-assert (not (eshell-interactive-process)))
+ (if (eshell-interactive-process-p)
+ (eshell-wait-for-process (eshell-tail-process)))
+ (cl-assert (not (eshell-interactive-process-p)))
(goto-char (point-max))
(while (and (bolp) (not (bobp)))
(delete-char -1)))
diff --git a/test/lisp/eshell/eshell-tests-helpers.el b/test/lisp/eshell/eshell-tests-helpers.el
index 77f5313d57..f3fbe90356 100644
--- a/test/lisp/eshell/eshell-tests-helpers.el
+++ b/test/lisp/eshell/eshell-tests-helpers.el
@@ -53,7 +53,7 @@ eshell-wait-for-subprocess
If this takes longer than `eshell-test--max-subprocess-time',
raise an error."
(let ((start (current-time)))
- (while (eshell-interactive-process)
+ (while (eshell-interactive-process-p)
(when (> (float-time (time-since start))
eshell-test--max-subprocess-time)
(error "timed out waiting for subprocess"))
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index 7658d5f551..3b1bbe7188 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -136,6 +136,17 @@ eshell-test/pipe-tailproc
(eshell-command-result-p "*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)
+ (eshell-match-result "OLLEH\n")))
+
(ert-deftest eshell-test/window-height ()
"$LINES should equal (window-height)"
(should (eshell-test-command-result "= $LINES (window-height)")))
--
2.25.1
[-- Attachment #4: 0003-Use-require-to-load-eshell-tests-helpers.patch --]
[-- Type: text/plain, Size: 3149 bytes --]
From a178fd197cf53f7731e1b0c6f9af32a144185ab5 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 2 Feb 2022 18:26:50 -0800
Subject: [PATCH 3/3] Use 'require' to load eshell-tests-helpers
* test/lisp/eshell/eshell-tests.el
* test/lisp/eshell/em-extpipe-tests.el: Load eshell-tests-helpers with
'require'.
* test/lisp/eshell/eshell-tests-helpers.el (eshell-history-file-name):
Define this here so individual test files don't have to.
---
test/lisp/eshell/em-extpipe-tests.el | 12 ++++--------
test/lisp/eshell/eshell-tests-helpers.el | 2 ++
test/lisp/eshell/eshell-tests.el | 14 ++++----------
3 files changed, 10 insertions(+), 18 deletions(-)
diff --git a/test/lisp/eshell/em-extpipe-tests.el b/test/lisp/eshell/em-extpipe-tests.el
index 0879ad5b0c..a1d15fe73a 100644
--- a/test/lisp/eshell/em-extpipe-tests.el
+++ b/test/lisp/eshell/em-extpipe-tests.el
@@ -28,14 +28,10 @@
(require 'ert)
(require 'ert-x)
(require 'em-extpipe)
-(eval-and-compile
- (load (expand-file-name "eshell-tests-helpers"
- (file-name-directory (or load-file-name
- default-directory)))))
-
-(defvar eshell-history-file-name)
-(defvar eshell-test--max-subprocess-time)
-(declare-function eshell-command-result-p "eshell-tests-helpers")
+(require 'eshell-tests-helpers
+ (expand-file-name "eshell-tests-helpers"
+ (file-name-directory (or load-file-name
+ default-directory))))
(defmacro em-extpipe-tests--deftest (name input &rest body)
(declare (indent 2))
diff --git a/test/lisp/eshell/eshell-tests-helpers.el b/test/lisp/eshell/eshell-tests-helpers.el
index f3fbe90356..33cdd60113 100644
--- a/test/lisp/eshell/eshell-tests-helpers.el
+++ b/test/lisp/eshell/eshell-tests-helpers.el
@@ -30,6 +30,8 @@
(require 'esh-mode)
(require 'eshell)
+(defvar eshell-history-file-name nil)
+
(defvar eshell-test--max-subprocess-time 5
"The maximum amount of time to wait for a subprocess to finish, in seconds.
See `eshell-wait-for-subprocess'.")
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index 3b1bbe7188..c5ca0a5485 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -29,16 +29,10 @@
(require 'ert-x)
(require 'esh-mode)
(require 'eshell)
-(eval-and-compile
- (load (expand-file-name "eshell-tests-helpers"
- (file-name-directory (or load-file-name
- default-directory)))))
-
-(defvar eshell-history-file-name)
-(defvar eshell-test--max-subprocess-time)
-(declare-function eshell-insert-command "eshell-tests-helpers")
-(declare-function eshell-match-result "eshell-tests-helpers")
-(declare-function eshell-command-result-p "eshell-tests-helpers")
+(require 'eshell-tests-helpers
+ (expand-file-name "eshell-tests-helpers"
+ (file-name-directory (or load-file-name
+ default-directory))))
;;; Tests:
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in Eshell
2022-02-03 2:35 ` Jim Porter
@ 2022-02-03 19:03 ` Lars Ingebrigtsen
2022-02-05 6:51 ` Jim Porter
0 siblings, 1 reply; 9+ messages in thread
From: Lars Ingebrigtsen @ 2022-02-03 19:03 UTC (permalink / raw)
To: Jim Porter; +Cc: 53715
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.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in Eshell
2022-02-03 19:03 ` Lars Ingebrigtsen
@ 2022-02-05 6:51 ` Jim Porter
2022-02-05 6:59 ` Lars Ingebrigtsen
0 siblings, 1 reply; 9+ messages in thread
From: Jim Porter @ 2022-02-05 6:51 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: 53715
[-- 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in Eshell
2022-02-05 6:51 ` Jim Porter
@ 2022-02-05 6:59 ` Lars Ingebrigtsen
0 siblings, 0 replies; 9+ messages in thread
From: Lars Ingebrigtsen @ 2022-02-05 6:59 UTC (permalink / raw)
To: Jim Porter; +Cc: 53715
Jim Porter <jporterbugs@gmail.com> writes:
> Here's a patch with a test to verify that things work correctly. Now
> the output is:
Thanks; pushed to Emacs 29.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-02-05 6:59 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-02 3:32 bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in Eshell Jim Porter
2022-02-02 18:10 ` Lars Ingebrigtsen
2022-02-02 19:41 ` Jim Porter
2022-02-02 19:49 ` Lars Ingebrigtsen
2022-02-02 21:01 ` Jim Porter
2022-02-03 2:35 ` Jim Porter
2022-02-03 19:03 ` Lars Ingebrigtsen
2022-02-05 6:51 ` Jim Porter
2022-02-05 6:59 ` Lars Ingebrigtsen
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).