From: Jim Porter <jporterbugs@gmail.com>
To: Eli Zaretskii <eliz@gnu.org>
Cc: 59545@debbugs.gnu.org, milan.zimmermann@gmail.com,
michael.albinus@gmx.de
Subject: bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
Date: Sat, 24 Dec 2022 17:36:46 -0800 [thread overview]
Message-ID: <d1a43fb0-e1a8-45c8-2320-42c1501bf4f4@gmail.com> (raw)
In-Reply-To: <13f7119d-9960-1936-9014-2147e9bd1db9@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 817 bytes --]
On 12/23/2022 11:29 PM, Jim Porter wrote:
> I found a problem with the patch on master:
>
> ~ $ {echo hello; echo world} | rev
> olleh ;; "dlrow" is missing!
>
> This happens because the way I'm copying output handles around results
> in EOF being sent to "rev" after "echo hello".
Attached is a patch to fix this. I'm going to look into adding more test
cases if I can think of any before merging this.
I'll also see if I can fix the FIXME comment I added, but this is a part
of Eshell that's fairly brittle, and I think the *real* fix for that is
moving to running Eshell commands in a separate thread, as discussed on
emacs-devel. (I have a very WIP patch for this that already works
surprisingly well, but it's going to require a lot more work before it's
even worth making a feature branch.)
[-- Attachment #2: 0001-Fix-reference-counting-of-Eshell-I-O-handles.patch --]
[-- Type: text/plain, Size: 19366 bytes --]
From 7c2a087f7534d70893cd533c42a3a7c78682cb9a Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 24 Dec 2022 14:31:50 -0800
Subject: [PATCH] Fix reference-counting of Eshell I/O handles
This ensures that output targets in Eshell are only closed when Eshell
is actually done with them. In particular, this means that
"{ echo foo; echo bar } | rev" prints "raboof" as expected
(bug#59545).
* lisp/eshell/esh-io.el (eshell-create-handles): Structure the handles
differently so the targets and their ref-count can be shared.
(eshell-duplicate-handles): Reimplement this to share targets between
the original and new handle sets. Add STEAL-P argument.
(eshell-protect-handles, eshell-close-handles)
(eshell-set-output-handle, eshell-copy-output-handle)
(eshell-interactive-output-p, eshell-output-object): Account for
changes to the handle structure.
(eshell-get-targets): Remove. This only existed to make the previous
implementation of 'eshell-duplicate-handles' work.
* lisp/eshell/esh-cmd.el (eshell-with-copied-handles): New argument
STEAL-P.
(eshell-do-pipelines): Use STEAL-P for the last item in the pipeline.
(eshell-parse-command): Don't copy handles for the last command in the
list; explain why we can't use STEAL-P here.
* test/lisp/eshell/em-extpipe-tests.el (em-extpipe-tests--deftest)
* test/lisp/eshemm/em-tramp-tests.el (em-tramp-test/su-default)
(em-tramp-test/su-user, em-tramp-test/su-login)
(em-tramp-test/sudo-shell, em-tramp-test/sudo-user-shell)
(em-tramp-test/doas-shell, em-tramp-test/doas-user-shell): Account for
changes to the handle structure.
* test/lisp/eshell/esh-io-tests.el (esh-io-test/redirect-pipe): Split
into...
(esh-io-test/pipeline/default, esh-io-test/pipeline/all): ... these.
(esh-io-test/pipeline/subcommands): New test.
---
lisp/eshell/esh-cmd.el | 22 ++++--
lisp/eshell/esh-io.el | 106 +++++++++++++++------------
test/lisp/eshell/em-extpipe-tests.el | 2 +-
test/lisp/eshell/em-tramp-tests.el | 75 +++++++++----------
test/lisp/eshell/esh-io-tests.el | 23 ++++--
5 files changed, 127 insertions(+), 101 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 79957aeb416..4420657488b 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -419,10 +419,12 @@ eshell-parse-command
(let ((cmd commands))
(while cmd
;; Copy I/O handles so each full statement can manipulate them
- ;; if they like. As a small optimization, skip this for the
- ;; last top-level one; we won't use these handles again
- ;; anyway.
- (when (or (not toplevel) (cdr cmd))
+ ;; if they like. Skip this for the last command in the list
+ ;; though; we won't use these handles again anyway.
+ ;; FIXME: We could just call `eshell-with-copied-handles' with
+ ;; a non-nil STEAL-P argument here, except that this confuses
+ ;; Eshell's iterative evaluation when queuing input.
+ (when (cdr cmd)
(setcar cmd `(eshell-with-copied-handles ,(car cmd))))
(setq cmd (cdr cmd))))
(if toplevel
@@ -792,10 +794,12 @@ eshell-trap-errors
(defvar eshell-output-handle) ;Defined in esh-io.el.
(defvar eshell-error-handle) ;Defined in esh-io.el.
-(defmacro eshell-with-copied-handles (object)
- "Duplicate current I/O handles, so OBJECT works with its own copy."
+(defmacro eshell-with-copied-handles (object &optional steal-p)
+ "Duplicate current I/O handles, so OBJECT works with its own copy.
+If STEAL-P is non-nil, these new handles will be stolen from the
+current ones (see `eshell-duplicate-handles')."
`(let ((eshell-current-handles
- (eshell-duplicate-handles eshell-current-handles)))
+ (eshell-duplicate-handles eshell-current-handles ,steal-p)))
,object))
(define-obsolete-function-alias 'eshell-copy-handles
@@ -836,7 +840,9 @@ eshell-do-pipelines
(let ((proc ,(car pipeline)))
(set headproc (or proc (symbol-value headproc)))
(set tailproc (or (symbol-value tailproc) proc))
- proc))))))
+ proc)))
+ ;; 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.
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index f2bc87374c1..5002cc50dc3 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -302,35 +302,51 @@ eshell-create-handles
The result is a vector of file handles. Each handle is of the form:
- (TARGETS DEFAULT REF-COUNT)
+ ((TARGETS . REF-COUNT) DEFAULT)
-TARGETS is a list of destinations for output. DEFAULT is non-nil
-if handle has its initial default value (always t after calling
-this function). REF-COUNT is the number of references to this
-handle (initially 1); see `eshell-protect-handles' and
-`eshell-close-handles'."
+TARGETS is a list of destinations for output. REF-COUNT is the
+number of references to this handle (initially 1); see
+`eshell-protect-handles' and `eshell-close-handles'. DEFAULT is
+non-nil if handle has its initial default value (always t after
+calling this function)."
(let* ((handles (make-vector eshell-number-of-handles nil))
- (output-target (eshell-get-targets stdout output-mode))
- (error-target (if stderr
- (eshell-get-targets stderr error-mode)
- output-target)))
- (aset handles eshell-output-handle (list output-target t 1))
- (aset handles eshell-error-handle (list error-target t 1))
+ (output-target
+ (let ((target (eshell-get-target stdout output-mode)))
+ (cons (when target (list target)) 1)))
+ (error-target
+ (if stderr
+ (let ((target (eshell-get-target stderr error-mode)))
+ (cons (when target (list target)) 1))
+ (cl-incf (cdr output-target))
+ output-target)))
+ (aset handles eshell-output-handle (list output-target t))
+ (aset handles eshell-error-handle (list error-target t))
handles))
-(defun eshell-duplicate-handles (handles)
+(defun eshell-duplicate-handles (handles &optional steal-p)
"Create a duplicate of the file handles in HANDLES.
-This will copy the targets of each handle in HANDLES, setting the
-DEFAULT field to t (see `eshell-create-handles')."
- (eshell-create-handles
- (car (aref handles eshell-output-handle)) nil
- (car (aref handles eshell-error-handle)) nil))
+This uses the targets of each handle in HANDLES, incrementing its
+reference count by one (unless STEAL-P is non-nil). These
+targets are shared between the original set of handles and the
+new one, so the targets are only closed when the reference count
+drops to 0 (see `eshell-close-handles').
+
+This function also sets the DEFAULT field for each handle to
+t (see `eshell-create-handles'). Unlike the targets, this value
+is not shared with the original handles."
+ (let ((dup-handles (make-vector eshell-number-of-handles nil)))
+ (dotimes (idx eshell-number-of-handles)
+ (when-let ((handle (aref handles idx)))
+ (unless steal-p
+ (cl-incf (cdar handle)))
+ (aset dup-handles idx (list (car handle) t))))
+ dup-handles))
(defun eshell-protect-handles (handles)
"Protect the handles in HANDLES from a being closed."
(dotimes (idx eshell-number-of-handles)
(when-let ((handle (aref handles idx)))
- (setcar (nthcdr 2 handle) (1+ (nth 2 handle)))))
+ (cl-incf (cdar handle))))
handles)
(defun eshell-close-handles (&optional exit-code result handles)
@@ -351,26 +367,34 @@ eshell-close-handles
(let ((handles (or handles eshell-current-handles)))
(dotimes (idx eshell-number-of-handles)
(when-let ((handle (aref handles idx)))
- (setcar (nthcdr 2 handle) (1- (nth 2 handle)))
- (when (= (nth 2 handle) 0)
- (dolist (target (ensure-list (car (aref handles idx))))
+ (cl-assert (natnump (cdar handle)))
+ (when (and (> (cdar handle) 0)
+ (= (cl-decf (cdar handle)) 0))
+ (dolist (target (caar handle))
(eshell-close-target target (= eshell-last-command-status 0)))
- (setcar handle nil))))))
+ (setcar (car handle) nil))))))
(defun eshell-set-output-handle (index mode &optional target handles)
"Set handle INDEX for the current HANDLES to point to TARGET using MODE.
-If HANDLES is nil, use `eshell-current-handles'."
+If HANDLES is nil, use `eshell-current-handles'.
+
+If the handle is currently set to its default value (see
+`eshell-create-handles'), this will overwrite the targets with
+the new target. Otherwise, it will append the new target to the
+current list of targets."
(when target
(let* ((handles (or handles eshell-current-handles))
(handle (or (aref handles index)
- (aset handles index (list nil nil 1))))
- (defaultp (cadr handle))
- (current (unless defaultp (car handle))))
+ (aset handles index (list (cons nil 1) nil))))
+ (defaultp (cadr handle)))
+ (when defaultp
+ (cl-decf (cdar handle))
+ (setcar handle (cons nil 1)))
(catch 'eshell-null-device
- (let ((where (eshell-get-target target mode)))
+ (let ((current (caar handle))
+ (where (eshell-get-target target mode)))
(unless (member where current)
- (setq current (append current (list where))))))
- (setcar handle current)
+ (setcar (car handle) (append current (list where))))))
(setcar (cdr handle) nil))))
(defun eshell-copy-output-handle (index index-to-copy &optional handles)
@@ -378,10 +402,7 @@ eshell-copy-output-handle
If HANDLES is nil, use `eshell-current-handles'."
(let* ((handles (or handles eshell-current-handles))
(handle-to-copy (car (aref handles index-to-copy))))
- (setcar (aref handles index)
- (if (listp handle-to-copy)
- (copy-sequence handle-to-copy)
- handle-to-copy))))
+ (setcar (aref handles index) handle-to-copy)))
(defun eshell-set-all-output-handles (mode &optional target handles)
"Set output and error HANDLES to point to TARGET using MODE.
@@ -501,13 +522,6 @@ eshell-get-target
(error "Invalid redirection target: %s"
(eshell-stringify target)))))
-(defun eshell-get-targets (targets &optional mode)
- "Convert TARGETS into valid output targets.
-TARGETS can be a single raw target or a list thereof. MODE is either
-`overwrite', `append' or `insert'; if it is omitted or nil, it
-defaults to `insert'."
- (mapcar (lambda (i) (eshell-get-target i mode)) (ensure-list targets)))
-
(defun eshell-interactive-output-p (&optional index handles)
"Return non-nil if the specified handle is bound for interactive display.
HANDLES is the set of handles to check; if nil, use
@@ -519,9 +533,9 @@ eshell-interactive-output-p
(let ((handles (or handles eshell-current-handles))
(index (or index eshell-output-handle)))
(if (eq index 'all)
- (and (equal (car (aref handles eshell-output-handle)) '(t))
- (equal (car (aref handles eshell-error-handle)) '(t)))
- (equal (car (aref handles index)) '(t)))))
+ (and (equal (caar (aref handles eshell-output-handle)) '(t))
+ (equal (caar (aref handles eshell-error-handle)) '(t)))
+ (equal (caar (aref handles index)) '(t)))))
(defvar eshell-print-queue nil)
(defvar eshell-print-queue-count -1)
@@ -628,8 +642,8 @@ eshell-output-object
If HANDLE-INDEX is nil, output to `eshell-output-handle'.
HANDLES is the set of file handles to use; if nil, use
`eshell-current-handles'."
- (let ((targets (car (aref (or handles eshell-current-handles)
- (or handle-index eshell-output-handle)))))
+ (let ((targets (caar (aref (or handles eshell-current-handles)
+ (or handle-index eshell-output-handle)))))
(dolist (target targets)
(eshell-output-object-to-target object target))))
diff --git a/test/lisp/eshell/em-extpipe-tests.el b/test/lisp/eshell/em-extpipe-tests.el
index a2646a0296b..04e78279427 100644
--- a/test/lisp/eshell/em-extpipe-tests.el
+++ b/test/lisp/eshell/em-extpipe-tests.el
@@ -42,7 +42,7 @@ em-extpipe-tests--deftest
(shell-command-switch "-c"))
;; Strip `eshell-trap-errors'.
(should (equal ,expected
- (cadadr (eshell-parse-command input))))))
+ (cadr (eshell-parse-command input))))))
(with-substitute-for-temp (&rest body)
;; Substitute name of an actual temporary file and/or
;; buffer into `input'. The substitution logic is
diff --git a/test/lisp/eshell/em-tramp-tests.el b/test/lisp/eshell/em-tramp-tests.el
index 982a1eba279..6cc35ecdb1b 100644
--- a/test/lisp/eshell/em-tramp-tests.el
+++ b/test/lisp/eshell/em-tramp-tests.el
@@ -27,23 +27,21 @@ em-tramp-test/su-default
"Test Eshell `su' command with no arguments."
(should (equal
(catch 'eshell-replace-command (eshell/su))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:root@%s:%s"
- tramp-default-host default-directory))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/su:root@%s:%s"
+ tramp-default-host default-directory)))))))
(ert-deftest em-tramp-test/su-user ()
"Test Eshell `su' command with USER argument."
(should (equal
(catch 'eshell-replace-command (eshell/su "USER"))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:USER@%s:%s"
- tramp-default-host default-directory))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/su:USER@%s:%s"
+ tramp-default-host default-directory)))))))
(ert-deftest em-tramp-test/su-login ()
"Test Eshell `su' command with -/-l/--login option."
@@ -52,11 +50,10 @@ em-tramp-test/su-login
("-")))
(should (equal
(catch 'eshell-replace-command (apply #'eshell/su args))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:root@%s:~/" tramp-default-host)))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/su:root@%s:~/" tramp-default-host))))))))
(defun mock-eshell-named-command (&rest args)
"Dummy function to test Eshell `sudo' command rewriting."
@@ -94,23 +91,21 @@ em-tramp-test/sudo-shell
("-s")))
(should (equal
(catch 'eshell-replace-command (apply #'eshell/sudo args))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/sudo:root@%s:%s"
- tramp-default-host default-directory)))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/sudo:root@%s:%s"
+ tramp-default-host default-directory))))))))
(ert-deftest em-tramp-test/sudo-user-shell ()
"Test Eshell `sudo' command with -s and -u options."
(should (equal
(catch 'eshell-replace-command (eshell/sudo "-u" "USER" "-s"))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/sudo:USER@%s:%s"
- tramp-default-host default-directory))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/sudo:USER@%s:%s"
+ tramp-default-host default-directory)))))))
(ert-deftest em-tramp-test/doas-basic ()
"Test Eshell `doas' command with default user."
@@ -149,22 +144,20 @@ em-tramp-test/doas-shell
("-s")))
(should (equal
(catch 'eshell-replace-command (apply #'eshell/doas args))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/doas:root@%s:%s"
- tramp-default-host default-directory)))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/doas:root@%s:%s"
+ tramp-default-host default-directory))))))))
(ert-deftest em-tramp-test/doas-user-shell ()
"Test Eshell `doas' command with -s and -u options."
(should (equal
(catch 'eshell-replace-command (eshell/doas "-u" "USER" "-s"))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/doas:USER@%s:%s"
- tramp-default-host default-directory))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/doas:USER@%s:%s"
+ tramp-default-host default-directory)))))))
;;; em-tramp-tests.el ends here
diff --git a/test/lisp/eshell/esh-io-tests.el b/test/lisp/eshell/esh-io-tests.el
index 9a3c14f365f..0f09afa19e4 100644
--- a/test/lisp/eshell/esh-io-tests.el
+++ b/test/lisp/eshell/esh-io-tests.el
@@ -301,15 +301,28 @@ esh-io-test/redirect-copy-first
"stderr\n"))
(should (equal (buffer-string) "stdout\n"))))
-(ert-deftest esh-io-test/redirect-pipe ()
- "Check that \"redirecting\" to a pipe works."
- ;; `|' should only redirect stdout.
+\f
+;; Pipelines
+
+(ert-deftest esh-io-test/pipeline/default ()
+ "Check that `|' only pipes stdout."
+ (skip-unless (executable-find "rev"))
(eshell-command-result-equal "test-output | rev"
- "stderr\ntuodts\n")
- ;; `|&' should redirect stdout and stderr.
+ "stderr\ntuodts\n"))
+
+
+(ert-deftest esh-io-test/pipeline/all ()
+ "Check that `|&' only pipes stdout and stderr."
+ (skip-unless (executable-find "rev"))
(eshell-command-result-equal "test-output |& rev"
"tuodts\nrredts\n"))
+(ert-deftest esh-io-test/pipeline/subcommands ()
+ "Chek that all commands in a subcommand are properly piped."
+ (skip-unless (executable-find "rev"))
+ (eshell-command-result-equal "{echo foo; echo bar} | rev"
+ "raboof"))
+
\f
;; Virtual targets
--
2.25.1
next prev parent reply other threads:[~2022-12-25 1:36 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-24 15:49 bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file Milan Zimmermann
2022-12-21 0:18 ` Jim Porter
2022-12-21 0:29 ` Jim Porter
2022-12-21 9:54 ` Michael Albinus
2022-12-21 18:48 ` Jim Porter
2022-12-21 19:26 ` Eli Zaretskii
2022-12-22 1:20 ` Jim Porter
2022-12-22 20:02 ` Jim Porter
2022-12-24 7:29 ` Jim Porter
2022-12-25 1:36 ` Jim Porter [this message]
2022-12-25 21:49 ` Jim Porter
2022-12-26 19:50 ` Jim Porter
2022-12-30 6:40 ` Jim Porter
2022-12-21 12:01 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d1a43fb0-e1a8-45c8-2320-42c1501bf4f4@gmail.com \
--to=jporterbugs@gmail.com \
--cc=59545@debbugs.gnu.org \
--cc=eliz@gnu.org \
--cc=michael.albinus@gmx.de \
--cc=milan.zimmermann@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.