bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file

[Jim Porter <jporterbugs@gmail.com>]

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

On 11/24/2022 7:49 AM, Milan Zimmermann wrote:
> Emacs 29 Eshell Bug: Sourcing 'redirect-echo.esh' and redirecting output
> to a file, results in the first echo string ('hello') showing in eshell,
> only the second ('there')(presumably because it is last) showing in the
> output file.

It turns out there's an even simpler way to reproduce this:

   ~ $ {echo hi; echo bye} > #<buf>
   hi  ;; Buffer "buf" now contains the string "bye".

Initially[1], I said that this was an issue with the implementation of 
'eshell-protect', but it turns out that it's actually an issue in an 
adjacent part of the Eshell I/O code. Specifically, every statement in 
Eshell gets its own set of default I/O handles, when it should actually 
inherit the handles from its parent. So in the example above, "echo hi" 
has the default I/O handles (pointing to the terminal), when its stdout 
handle should point to the buffer "buf".

Attached is a patch series to fix this, with a bunch of new tests. I 
also fixed a related issue where redirecting to /dev/null could clobber 
your other redirects. (There's *also* an issue that should be fixed for 
the release branch; I'll send that in a separate message.)

[1] https://lists.gnu.org/archive/html/emacs-devel/2022-11/msg01504.html

[-- Attachment #2: 0001-Add-eshell-duplicate-handles-to-return-a-copy-of-fil.patch --]
[-- Type: text/plain, Size: 9435 bytes --]

From 997c4e7e8343e87978fb6d921b0988c07dc6069d Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 19 Dec 2022 22:21:10 -0800
Subject: [PATCH 1/3] Add 'eshell-duplicate-handles' to return a copy of file
 handles

* lisp/eshell/esh-io.el (eshell-create-handles): Support creating with
multiple targets for stdout and/or stderr.  Make the targets for a
handle always be a list, and store whether the targets are the default
in a separate 'default' field.
(eshell-protect-handles, eshell-close-handles)
(eshell-copy-output-handle, eshell-interactive-output-p)
(eshell-output-object): Update for changes in 'eshell-create-handles'.
(eshell-duplicate-handles, eshell-get-targets): New functions.

* lisp/eshell/esh-cmd.el (eshell-copy-handles): Rename and alias to...
(eshell-with-copied-handles): ... this function, and use
'eshell-duplicate-handles'.
(eshell-execute-pipeline): Use 'eshell-duplicate-handles'.
---
 lisp/eshell/esh-cmd.el | 20 ++++------
 lisp/eshell/esh-io.el  | 83 ++++++++++++++++++++++++++----------------
 2 files changed, 60 insertions(+), 43 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 1fb84991120..03388236b06 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -788,16 +788,15 @@ eshell-trap-errors
 (defvar eshell-output-handle)           ;Defined in esh-io.el.
 (defvar eshell-error-handle)            ;Defined in esh-io.el.
 
-(defmacro eshell-copy-handles (object)
+(defmacro eshell-with-copied-handles (object)
   "Duplicate current I/O handles, so OBJECT works with its own copy."
   `(let ((eshell-current-handles
-	  (eshell-create-handles
-	   (car (aref eshell-current-handles
-		      eshell-output-handle)) nil
-	   (car (aref eshell-current-handles
-		      eshell-error-handle)) nil)))
+          (eshell-duplicate-handles eshell-current-handles)))
      ,object))
 
+(define-obsolete-function-alias 'eshell-copy-handles
+  #'eshell-with-copied-handles "30.1")
+
 (defmacro eshell-protect (object)
   "Protect I/O handles, so they aren't get closed after eval'ing OBJECT."
   `(progn
@@ -808,7 +807,7 @@ eshell-do-pipelines
   "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-copy-handles
+    `(eshell-with-copied-handles
       (progn
 	,(when (cdr pipeline)
 	   `(let ((nextproc
@@ -880,11 +879,8 @@ eshell-execute-pipeline
      (progn
        ,(if (fboundp 'make-process)
 	    `(eshell-do-pipelines ,pipeline)
-	  `(let ((tail-handles (eshell-create-handles
-				(car (aref eshell-current-handles
-					   ,eshell-output-handle)) nil
-				(car (aref eshell-current-handles
-					   ,eshell-error-handle)) nil)))
+          `(let ((tail-handles (eshell-duplicate-handles
+                                eshell-current-handles)))
 	     (eshell-do-pipelines-synchronously ,pipeline)))
        (eshell-process-identity (cons (symbol-value headproc)
                                       (symbol-value tailproc))))))
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 4620565f857..58084db28a8 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -291,25 +291,42 @@ eshell--apply-redirections
 (defun eshell-create-handles
   (stdout output-mode &optional stderr error-mode)
   "Create a new set of file handles for a command.
-The default location for standard output and standard error will go to
-STDOUT and STDERR, respectively.
-OUTPUT-MODE and ERROR-MODE are either `overwrite', `append' or `insert';
-a nil value of mode defaults to `insert'."
+The default target for standard output and standard error will
+go to STDOUT and STDERR, respectively.  OUTPUT-MODE and
+ERROR-MODE are either `overwrite', `append' or `insert'; a nil
+value of mode defaults to `insert'.
+
+The result is a vector of file handles.  Each handle is of the form:
+
+  (TARGETS DEFAULT REF-COUNT)
+
+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'."
   (let* ((handles (make-vector eshell-number-of-handles nil))
-         (output-target (eshell-get-target stdout output-mode))
+         (output-target (eshell-get-targets stdout output-mode))
          (error-target (if stderr
-                           (eshell-get-target stderr error-mode)
+                           (eshell-get-targets stderr error-mode)
                          output-target)))
-    (aset handles eshell-output-handle (cons output-target 1))
-    (aset handles eshell-error-handle (cons error-target 1))
+    (aset handles eshell-output-handle (list output-target t 1))
+    (aset handles eshell-error-handle (list error-target t 1))
     handles))
 
+(defun eshell-duplicate-handles (handles)
+  "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))
+
 (defun eshell-protect-handles (handles)
   "Protect the handles in HANDLES from a being closed."
   (dotimes (idx eshell-number-of-handles)
-    (when (aref handles idx)
-      (setcdr (aref handles idx)
-              (1+ (cdr (aref handles idx))))))
+    (when-let ((handle (aref handles idx)))
+      (setcar (nthcdr 2 handle) (1+ (nth 2 handle)))))
   handles)
 
 (defun eshell-close-handles (&optional exit-code result handles)
@@ -330,8 +347,8 @@ eshell-close-handles
   (let ((handles (or handles eshell-current-handles)))
     (dotimes (idx eshell-number-of-handles)
       (when-let ((handle (aref handles idx)))
-        (setcdr handle (1- (cdr handle)))
-	(when (= (cdr handle) 0)
+        (setcar (nthcdr 2 handle) (1- (nth 2 handle)))
+        (when (= (nth 2 handle) 0)
           (dolist (target (ensure-list (car (aref handles idx))))
             (eshell-close-target target (= eshell-last-command-status 0)))
           (setcar handle nil))))))
@@ -344,15 +361,17 @@ eshell-set-output-handle
       (if (and (stringp target)
                (string= target (null-device)))
           (aset handles index nil)
-        (let ((where (eshell-get-target target mode))
-              (current (car (aref handles index))))
-          (if (listp current)
+        (let* ((where (eshell-get-target target mode))
+               (handle (or (aref handles index)
+                           (aset handles index (list nil nil 1))))
+               (current (car handle))
+               (defaultp (cadr handle)))
+          (if (not defaultp)
               (unless (member where current)
                 (setq current (append current (list where))))
             (setq current (list where)))
-          (if (not (aref handles index))
-              (aset handles index (cons nil 1)))
-          (setcar (aref handles index) current))))))
+          (setcar handle current)
+          (setcar (cdr handle) nil))))))
 
 (defun eshell-copy-output-handle (index index-to-copy &optional handles)
   "Copy the handle INDEX-TO-COPY to INDEX for the current HANDLES.
@@ -482,6 +501,13 @@ 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
@@ -493,9 +519,9 @@ eshell-interactive-output-p
   (let ((handles (or handles eshell-current-handles))
         (index (or index eshell-output-handle)))
     (if (eq index 'all)
-        (and (eq (car (aref handles eshell-output-handle)) t)
-             (eq (car (aref handles eshell-error-handle)) t))
-      (eq (car (aref handles index)) t))))
+        (and (equal (car (aref handles eshell-output-handle)) '(t))
+             (equal (car (aref handles eshell-error-handle)) '(t)))
+      (equal (car (aref handles index)) '(t)))))
 
 (defvar eshell-print-queue nil)
 (defvar eshell-print-queue-count -1)
@@ -602,15 +628,10 @@ 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 ((target (car (aref (or handles eshell-current-handles)
-			   (or handle-index eshell-output-handle)))))
-    (if (listp target)
-        (while target
-	  (eshell-output-object-to-target object (car target))
-	  (setq target (cdr target)))
-      (eshell-output-object-to-target object target)
-      ;; Explicitly return nil to match the list case above.
-      nil)))
+  (let ((targets (car (aref (or handles eshell-current-handles)
+                            (or handle-index eshell-output-handle)))))
+    (dolist (target targets)
+      (eshell-output-object-to-target object target))))
 
 (provide 'esh-io)
 ;;; esh-io.el ends here
-- 
2.25.1


[-- Attachment #3: 0002-Fix-handling-of-output-handles-in-nested-Eshell-form.patch --]
[-- Type: text/plain, Size: 12878 bytes --]

From 43e9664a7a0c528a11e86dd6f2d07b3afe233f54 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 20 Dec 2022 09:39:07 -0800
Subject: [PATCH 2/3] Fix handling of output handles in nested Eshell forms

Previously, the output handles in nested forms would be reset to the
default, leading to wrong behavior for commands like

  {echo a; echo b} > file

"b" would be written to "file" as expected, but "a" would go to
standard output (bug#59545).

* lisp/eshell/esh-cmd.el (eshell-parse-command): Use
'eshell-with-copied-handles' for each statement within the whole
Eshell command.

* test/lisp/eshell/esh-io-tests.el (esh-io-test/redirect-subcommands)
(esh-io-test/redirect-subcommands/override)
(esh-io-test/redirect-subcommands/interpolated): New tests.

* test/lisp/eshell/em-script-tests.el
(em-script-test/source-script/redirect)
(em-script-test/source-script/redirect/dev-null): New tests.
(em-script-test/source-script, em-script-test/source-script/arg-vars)
(em-script-test/source-script/all-args-var): Tweak names/docstrings.

* test/lisp/eshell/em-extpipe-tests.el (em-extpipe-tests--deftest):
Skip over the newly-added 'eshell-with-copied-handles' form when
checking the parse results.

* test/lisp/eshell/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): Update
expected command forms.
---
 doc/misc/eshell.texi                 |  5 --
 lisp/eshell/esh-cmd.el               |  8 ++-
 test/lisp/eshell/em-extpipe-tests.el |  2 +-
 test/lisp/eshell/em-script-tests.el  | 32 ++++++++++--
 test/lisp/eshell/em-tramp-tests.el   | 75 +++++++++++++++-------------
 test/lisp/eshell/esh-io-tests.el     | 28 +++++++++++
 6 files changed, 103 insertions(+), 47 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index f9796d69a9a..118ee80acb9 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -2162,11 +2162,6 @@ Bugs and ideas
 
 @item Allow all Eshell buffers to share the same history and list-dir
 
-@item There is a problem with script commands that output to @file{/dev/null}
-
-If a script file, somewhere in the middle, uses @samp{> /dev/null},
-output from all subsequent commands is swallowed.
-
 @item Split up parsing of text after @samp{$} in @file{esh-var.el}
 
 Make it similar to the way that @file{esh-arg.el} is structured.
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 03388236b06..79957aeb416 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -418,8 +418,12 @@ eshell-parse-command
 	   (eshell-separate-commands terms "[&;]" nil 'eshell--sep-terms))))
     (let ((cmd commands))
       (while cmd
-	(if (cdr cmd)
-	    (setcar cmd `(eshell-commands ,(car 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))
+	  (setcar cmd `(eshell-with-copied-handles ,(car cmd))))
 	(setq cmd (cdr cmd))))
     (if toplevel
 	`(eshell-commands (progn
diff --git a/test/lisp/eshell/em-extpipe-tests.el b/test/lisp/eshell/em-extpipe-tests.el
index 04e78279427..a2646a0296b 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
-                              (cadr (eshell-parse-command input))))))
+                              (cadadr (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-script-tests.el b/test/lisp/eshell/em-script-tests.el
index b837d464ccd..f720f697c67 100644
--- a/test/lisp/eshell/em-script-tests.el
+++ b/test/lisp/eshell/em-script-tests.el
@@ -35,21 +35,43 @@
 ;;; Tests:
 
 (ert-deftest em-script-test/source-script ()
-  "Test sourcing script with no argumentss"
+  "Test sourcing a simple script."
   (ert-with-temp-file temp-file :text "echo hi"
     (with-temp-eshell
      (eshell-match-command-output (format "source %s" temp-file)
                                   "hi\n"))))
 
-(ert-deftest em-script-test/source-script-arg-vars ()
-  "Test sourcing script with $0, $1, ... variables"
+(ert-deftest em-script-test/source-script/redirect ()
+  "Test sourcing a script and redirecting its output."
+  (ert-with-temp-file temp-file
+    :text "echo hi\necho bye"
+    (eshell-with-temp-buffer bufname "old"
+      (with-temp-eshell
+       (eshell-match-command-output
+        (format "source %s > #<%s>" temp-file bufname)
+        "\\`\\'"))
+      (should (equal (buffer-string) "hibye")))))
+
+(ert-deftest em-script-test/source-script/redirect/dev-null ()
+  "Test sourcing a script and redirecting its output, including to /dev/null."
+  (ert-with-temp-file temp-file
+    :text "echo hi\necho bad > /dev/null\necho bye"
+    (eshell-with-temp-buffer bufname "old"
+      (with-temp-eshell
+       (eshell-match-command-output
+        (format "source %s > #<%s>" temp-file bufname)
+        "\\`\\'"))
+      (should (equal (buffer-string) "hibye")))))
+
+(ert-deftest em-script-test/source-script/arg-vars ()
+  "Test sourcing script with $0, $1, ... variables."
   (ert-with-temp-file temp-file :text "printnl $0 \"$1 $2\""
     (with-temp-eshell
      (eshell-match-command-output (format "source %s one two" temp-file)
                                   (format "%s\none two\n" temp-file)))))
 
-(ert-deftest em-script-test/source-script-all-args-var ()
-  "Test sourcing script with the $* variable"
+(ert-deftest em-script-test/source-script/all-args-var ()
+  "Test sourcing script with the $* variable."
   (ert-with-temp-file temp-file :text "printnl $*"
     (with-temp-eshell
      (eshell-match-command-output (format "source %s" temp-file)
diff --git a/test/lisp/eshell/em-tramp-tests.el b/test/lisp/eshell/em-tramp-tests.el
index 6cc35ecdb1b..982a1eba279 100644
--- a/test/lisp/eshell/em-tramp-tests.el
+++ b/test/lisp/eshell/em-tramp-tests.el
@@ -27,21 +27,23 @@ em-tramp-test/su-default
   "Test Eshell `su' command with no arguments."
   (should (equal
            (catch 'eshell-replace-command (eshell/su))
-           `(eshell-trap-errors
-             (eshell-named-command
-              "cd"
-              (list ,(format "/su:root@%s:%s"
-                             tramp-default-host default-directory)))))))
+           `(eshell-with-copied-handles
+             (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-trap-errors
-             (eshell-named-command
-              "cd"
-              (list ,(format "/su:USER@%s:%s"
-                             tramp-default-host default-directory)))))))
+           `(eshell-with-copied-handles
+             (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."
@@ -50,10 +52,11 @@ em-tramp-test/su-login
                   ("-")))
     (should (equal
              (catch 'eshell-replace-command (apply #'eshell/su args))
-             `(eshell-trap-errors
-               (eshell-named-command
-                "cd"
-                (list ,(format "/su:root@%s:~/" tramp-default-host))))))))
+             `(eshell-with-copied-handles
+               (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."
@@ -91,21 +94,23 @@ em-tramp-test/sudo-shell
                   ("-s")))
     (should (equal
              (catch 'eshell-replace-command (apply #'eshell/sudo args))
-             `(eshell-trap-errors
-               (eshell-named-command
-                "cd"
-                (list ,(format "/sudo:root@%s:%s"
-                               tramp-default-host default-directory))))))))
+             `(eshell-with-copied-handles
+               (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-trap-errors
-             (eshell-named-command
-              "cd"
-              (list ,(format "/sudo:USER@%s:%s"
-                             tramp-default-host default-directory)))))))
+           `(eshell-with-copied-handles
+             (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."
@@ -144,20 +149,22 @@ em-tramp-test/doas-shell
                   ("-s")))
     (should (equal
              (catch 'eshell-replace-command (apply #'eshell/doas args))
-             `(eshell-trap-errors
-               (eshell-named-command
-                "cd"
-                (list ,(format "/doas:root@%s:%s"
-                               tramp-default-host default-directory))))))))
+             `(eshell-with-copied-handles
+               (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-trap-errors
-             (eshell-named-command
-              "cd"
-              (list ,(format "/doas:USER@%s:%s"
-                             tramp-default-host default-directory)))))))
+           `(eshell-with-copied-handles
+             (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 37b234eaf06..ccf8ac1b9a1 100644
--- a/test/lisp/eshell/esh-io-tests.el
+++ b/test/lisp/eshell/esh-io-tests.el
@@ -146,6 +146,34 @@ esh-io-test/redirect-multiple/repeat
      (should (equal (buffer-string) "new"))
      (should (equal eshell-test-value "new")))))
 
+(ert-deftest esh-io-test/redirect-subcommands ()
+  "Check that redirecting subcommands applies to all subcommands."
+  (eshell-with-temp-buffer bufname "old"
+    (with-temp-eshell
+     (eshell-insert-command (format "{echo foo; echo bar} > #<%s>" bufname)))
+    (should (equal (buffer-string) "foobar"))))
+
+(ert-deftest esh-io-test/redirect-subcommands/override ()
+  "Check that redirecting subcommands applies to all subcommands.
+Include a redirect to another location in the subcommand to
+ensure only its statement is redirected."
+  (eshell-with-temp-buffer bufname "old"
+    (eshell-with-temp-buffer bufname-2 "also old"
+      (with-temp-eshell
+       (eshell-insert-command
+        (format "{echo foo; echo bar > #<%s>; echo baz} > #<%s>"
+                bufname-2 bufname)))
+      (should (equal (buffer-string) "bar")))
+    (should (equal (buffer-string) "foobaz"))))
+
+(ert-deftest esh-io-test/redirect-subcommands/interpolated ()
+  "Check that redirecting interpolated subcommands applies to all subcommands."
+  (eshell-with-temp-buffer bufname "old"
+    (with-temp-eshell
+     (eshell-insert-command
+      (format "echo ${echo foo; echo bar} > #<%s>" bufname)))
+    (should (equal (buffer-string) "foobar"))))
+
 \f
 ;; Redirecting specific handles
 
-- 
2.25.1


[-- Attachment #4: 0003-Simplify-handling-of-dev-null-redirection-in-Eshell.patch --]
[-- Type: text/plain, Size: 6302 bytes --]

From f591a65476a6283de8614fa71fe4ad3375b998a5 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 20 Dec 2022 13:47:20 -0800
Subject: [PATCH 3/3] Simplify handling of /dev/null redirection in Eshell

This also fixes an issue where "echo hi > foo > /dev/null" didn't
write to the file "foo".

* lisp/eshell/esh-io.el (eshell-virtual-targets): Add "/dev/null".
(eshell-set-output-handle): Handle 'eshell-null-device'.

* test/lisp/eshell/esh-io-tests.el
(esh-io-test/redirect-subcommands/dev-null)
(esh-io-test/virtual/dev-null, esh-io-test/virtual/dev-null/multiple):
New tests.
---
 lisp/eshell/esh-io.el            | 51 +++++++++++++++-----------------
 test/lisp/eshell/esh-io-tests.el | 33 +++++++++++++++++++--
 2 files changed, 55 insertions(+), 29 deletions(-)

diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 58084db28a8..dc433de09b0 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -116,16 +116,20 @@ eshell-print-queue-size
   :group 'eshell-io)
 
 (defcustom eshell-virtual-targets
-  '(("/dev/eshell" eshell-interactive-print nil)
+  '(;; This should be the literal string "/dev/null", not `null-device'.
+    ("/dev/null" (lambda (mode) (throw 'eshell-null-device t)) t)
+    ("/dev/eshell" eshell-interactive-print nil)
     ("/dev/kill" (lambda (mode)
-		   (if (eq mode 'overwrite)
-		       (kill-new ""))
-		   'eshell-kill-append) t)
+                   (when (eq mode 'overwrite)
+                     (kill-new ""))
+                   #'eshell-kill-append)
+     t)
     ("/dev/clip" (lambda (mode)
-		   (if (eq mode 'overwrite)
-		       (let ((select-enable-clipboard t))
-			 (kill-new "")))
-		   'eshell-clipboard-append) t))
+                   (when (eq mode 'overwrite)
+                     (let ((select-enable-clipboard t))
+                       (kill-new "")))
+                   #'eshell-clipboard-append)
+     t))
   "Map virtual devices name to Emacs Lisp functions.
 If the user specifies any of the filenames above as a redirection
 target, the function in the second element will be called.
@@ -138,10 +142,7 @@ eshell-virtual-targets
 
 The output function is then called repeatedly with single strings,
 which represents successive pieces of the output of the command, until nil
-is passed, meaning EOF.
-
-NOTE: /dev/null is handled specially as a virtual target, and should
-not be added to this variable."
+is passed, meaning EOF."
   :type '(repeat
 	  (list (string :tag "Target")
 		function
@@ -357,21 +358,17 @@ eshell-set-output-handle
   "Set handle INDEX for the current HANDLES to point to TARGET using MODE.
 If HANDLES is nil, use `eshell-current-handles'."
   (when target
-    (let ((handles (or handles eshell-current-handles)))
-      (if (and (stringp target)
-               (string= target (null-device)))
-          (aset handles index nil)
-        (let* ((where (eshell-get-target target mode))
-               (handle (or (aref handles index)
-                           (aset handles index (list nil nil 1))))
-               (current (car handle))
-               (defaultp (cadr handle)))
-          (if (not defaultp)
-              (unless (member where current)
-                (setq current (append current (list where))))
-            (setq current (list where)))
-          (setcar handle current)
-          (setcar (cdr handle) nil))))))
+    (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))))
+      (catch 'eshell-null-device
+        (let ((where (eshell-get-target target mode)))
+          (unless (member where current)
+            (setq current (append current (list where))))))
+      (setcar handle current)
+      (setcar (cdr handle) nil))))
 
 (defun eshell-copy-output-handle (index index-to-copy &optional handles)
   "Copy the handle INDEX-TO-COPY to INDEX for the current HANDLES.
diff --git a/test/lisp/eshell/esh-io-tests.el b/test/lisp/eshell/esh-io-tests.el
index ccf8ac1b9a1..9a3c14f365f 100644
--- a/test/lisp/eshell/esh-io-tests.el
+++ b/test/lisp/eshell/esh-io-tests.el
@@ -166,6 +166,17 @@ esh-io-test/redirect-subcommands/override
       (should (equal (buffer-string) "bar")))
     (should (equal (buffer-string) "foobaz"))))
 
+(ert-deftest esh-io-test/redirect-subcommands/dev-null ()
+  "Check that redirecting subcommands applies to all subcommands.
+Include a redirect to /dev/null to ensure it only applies to its
+statement."
+  (eshell-with-temp-buffer bufname "old"
+    (with-temp-eshell
+     (eshell-insert-command
+      (format "{echo foo; echo bar > /dev/null; echo baz} > #<%s>"
+              bufname)))
+    (should (equal (buffer-string) "foobaz"))))
+
 (ert-deftest esh-io-test/redirect-subcommands/interpolated ()
   "Check that redirecting interpolated subcommands applies to all subcommands."
   (eshell-with-temp-buffer bufname "old"
@@ -302,12 +313,30 @@ esh-io-test/redirect-pipe
 \f
 ;; Virtual targets
 
-(ert-deftest esh-io-test/virtual-dev-eshell ()
+(ert-deftest esh-io-test/virtual/dev-null ()
+  "Check that redirecting to /dev/null works."
+  (with-temp-eshell
+   (eshell-match-command-output "echo hi > /dev/null" "\\`\\'")))
+
+(ert-deftest esh-io-test/virtual/dev-null/multiple ()
+  "Check that redirecting to /dev/null works alongside other redirections."
+  (eshell-with-temp-buffer bufname "old"
+    (with-temp-eshell
+     (eshell-match-command-output
+      (format "echo new > /dev/null > #<%s>" bufname) "\\`\\'"))
+    (should (equal (buffer-string) "new")))
+  (eshell-with-temp-buffer bufname "old"
+    (with-temp-eshell
+     (eshell-match-command-output
+      (format "echo new > #<%s> > /dev/null" bufname) "\\`\\'"))
+    (should (equal (buffer-string) "new"))))
+
+(ert-deftest esh-io-test/virtual/dev-eshell ()
   "Check that redirecting to /dev/eshell works."
   (with-temp-eshell
    (eshell-match-command-output "echo hi > /dev/eshell" "hi")))
 
-(ert-deftest esh-io-test/virtual-dev-kill ()
+(ert-deftest esh-io-test/virtual/dev-kill ()
   "Check that redirecting to /dev/kill works."
   (with-temp-eshell
    (eshell-insert-command "echo one > /dev/kill")
-- 
2.25.1