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: 1362 bytes --]

On 12/21/2022 1:54 AM, Michael Albinus wrote:
> And when they want to use another value? (null-device) returns
> "/dev/null" for local default-directory's if you're not on MS
> Windows. On MS Windows, it returns "NUL".

Right. The intent is for the virtual name for the null device to be the 
same in Eshell no matter what the system really calls it. On non-MS 
systems, this shouldn't actually be necessary, since you could just 
write to the *real* /dev/null. The virtual target in Eshell is just so 
that /dev/null also works on MS Windows/DOS.

However, I would have thought that you could write to NUL on MS Windows 
without any special handling. The Emacs manual has this to say:

"[On MS Windows,] referencing any file whose name matches a DOS 
character device, such as NUL or LPT1 or PRN or CON, with or without any 
file-name extension, will always resolve to those character devices, in 
any directory. Therefore, only use such file names when you want to use 
the corresponding character device."

I'd expect that to mean that if you opened a buffer and tried to save it 
as "NUL", it would just work, but instead I get:

   Write error: Bad file descriptor, c:/NUL

With that in mind, here are two patches (one for 29 and one for master) 
to let Eshell handle both "/dev/null" and (on MS systems) "NUL". That 
way, users get the best of both worlds.

[-- Attachment #2: 29--0001-When-redirecting-to-the-null-device-in-Eshell-allow-.patch --]
[-- Type: text/plain, Size: 1381 bytes --]

From 3664f2bdfb64ae7d68b6be12e4ed6b027bdc2951 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 20 Dec 2022 16:20:50 -0800
Subject: [PATCH] When redirecting to the null device in Eshell, allow
 "/dev/null"

This is so that users can type "cmd ... > /dev/null" in Eshell no
matter what their system's null device is called.  This fixes a
regression from 67a8bdb90c9b5865b7f17290c7135b1a5458c36d.

Do not merge to master.

* lisp/eshell/esh-io.el (eshell-set-output-handle): Handle both
'null-device' and the literal "/dev/null".
---
 lisp/eshell/esh-io.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 4620565f857..b428f068472 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -342,7 +342,10 @@ eshell-set-output-handle
   (when target
     (let ((handles (or handles eshell-current-handles)))
       (if (and (stringp target)
-               (string= target (null-device)))
+               ;; Always treat "/dev/null" as the null device for
+               ;; compatibility, even if the system calls it something
+               ;; else.
+               (member target (list "/dev/null" (null-device))))
           (aset handles index nil)
         (let ((where (eshell-get-target target mode))
               (current (car (aref handles index))))
-- 
2.25.1


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

From afe81dc609802c9a3d3d64037d9d9df9a100cd0f 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'.
(eshell-get-target): Map 'null-device' to "/dev/null".

* 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            | 56 +++++++++++++++++---------------
 test/lisp/eshell/esh-io-tests.el | 33 +++++++++++++++++--
 2 files changed, 60 insertions(+), 29 deletions(-)

diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 58084db28a8..a1dfef07458 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.
@@ -458,6 +455,11 @@ eshell-get-target
   (setq mode (or mode 'insert))
   (cond
    ((stringp target)
+    ;; Always treat the `null-device' as the virtual target
+    ;; "/dev/null".  This way, systems that call their null device
+    ;; something else can use either form.
+    (when (string= target (null-device))
+      (setq target "/dev/null"))
     (let ((redir (assoc target eshell-virtual-targets)))
       (if redir
 	  (if (nth 2 redir)
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