bug#62578: 30.0.50; [PATCH] Add regression tests for synchronous processes in Eshell

[Jim Porter <jporterbugs@gmail.com>]

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

On 4/1/2023 10:29 PM, Eli Zaretskii wrote:
> One minor nit: 'message' in a batch session prints to stderr, not to
> stdout.

!! Thanks for catching that. That actually means there's a small bug (or 
a missing feature) in Eshell: it doesn't support redirecting stdout and 
stderr to separate places when using synchronous subprocesses.

I've added a note about this to esh-proc.el (fixing it would probably be 
tricky), and changed 'message' in the tests to be 'princ'. See attached; 
I'll probably merge this tomorrow unless you catch any remaining issues.

[-- Attachment #2: 0001-Add-tests-for-synchronous-processes-in-Eshell.patch --]
[-- Type: text/plain, Size: 6337 bytes --]

From ec1a5b3cda09945cb4b74273f9568267de5bd96f Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 31 Mar 2023 21:32:44 -0700
Subject: [PATCH] ; Add tests for synchronous processes in Eshell

Normally, Eshell only uses synchronous processes on MS-DOS, so this is
hard to test.  To get around this, let the tests explicitly request
synchronous processes.

* lisp/eshell/esh-proc.el (eshell-supports-asynchronous-processes):
New variable...
(eshell-gather-process-output): ... use it, and remove some incorrect
code updating Eshell's internal markers (the async code path doesn't
do this, so neither should the sync path).

* lisp/eshell/esh-cmd.el (eshell-execute-pipeline): Use
'eshell-supports-asynchronous-processes'.

* test/lisp/eshell/esh-proc-tests.el
(esh-proc-test/emacs-command): New function.
(esh-proc-test/emacs-echo, esh-proc-test/emacs-upcase): New variables.
(esh-proc-test/synchronous-proc/simple/interactive)
(esh-proc-test/synchronous-proc/simple/command-result)
(esh-proc-test/synchronous-proc/pipeline/interactive)
(esh-proc-test/synchronous-proc/pipeline/command-result): New tests.
---
 lisp/eshell/esh-cmd.el             |  2 +-
 lisp/eshell/esh-proc.el            | 11 ++++---
 test/lisp/eshell/esh-proc-tests.el | 53 ++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index d5237ee1f04..b80596f2cad 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -892,7 +892,7 @@ eshell-execute-pipeline
      (set headproc nil)
      (set tailproc nil)
      (progn
-       ,(if (fboundp 'make-process)
+       ,(if eshell-supports-asynchronous-processes
 	    `(eshell-do-pipelines ,pipeline)
           `(let ((tail-handles (eshell-duplicate-handles
                                 eshell-current-handles)))
diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index a86e7502795..00e0c8014e1 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -97,6 +97,9 @@ eshell-kill-hook
 
 ;;; Internal Variables:
 
+(defvar eshell-supports-asynchronous-processes (fboundp 'make-process)
+  "Non-nil if Eshell can create asynchronous processes.")
+
 (defvar eshell-current-subjob-p nil)
 
 (defvar eshell-process-list nil
@@ -296,7 +299,7 @@ eshell-gather-process-output
                 (coding-system-change-eol-conversion locale-coding-system
                                                      'unix))))
     (cond
-     ((fboundp 'make-process)
+     (eshell-supports-asynchronous-processes
       (unless (or ;; FIXME: It's not currently possible to use a
                   ;; stderr process for remote files.
                   (file-remote-p default-directory)
@@ -367,6 +370,8 @@ eshell-gather-process-output
 	(erase-buffer)
 	(set-buffer oldbuf)
 	(run-hook-with-args 'eshell-exec-hook command)
+        ;; XXX: This doesn't support sending stdout and stderr to
+        ;; separate places.
 	(setq exit-status
 	      (apply #'call-process-region
 		     (append (list eshell-last-sync-output-start (point)
@@ -392,10 +397,6 @@ eshell-gather-process-output
 	    (setq lbeg lend)
 	    (set-buffer proc-buf))
 	  (set-buffer oldbuf))
-        (require 'esh-mode)
-        (declare-function eshell-update-markers "esh-mode" (pmark))
-        (defvar eshell-last-output-end)         ;Defined in esh-mode.el.
-	(eshell-update-markers eshell-last-output-end)
 	;; Simulate the effect of eshell-sentinel.
 	(eshell-close-handles
          (if (numberp exit-status) exit-status -1)
diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
index 8e02fbb5497..fa20efa71e1 100644
--- a/test/lisp/eshell/esh-proc-tests.el
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -191,6 +191,59 @@ esh-proc-test/pipeline-connection-type/last
      (unless (eq system-type 'windows-nt)
        "stdout\nstderr\n"))))
 
+\f
+;; Synchronous processes
+
+;; These tests check that synchronous subprocesses (only used on
+;; MS-DOS by default) work correctly.  To help them run on MS-DOS as
+;; well, we use the Emacs executable as our subprocess to test
+;; against; that way, users don't need to have GNU coreutils (or
+;; similar) installed.
+
+(defsubst esh-proc-test/emacs-command (command)
+  "Evaluate COMMAND in a new Emacs batch instance."
+  (mapconcat #'shell-quote-argument
+             `(,(expand-file-name invocation-name invocation-directory)
+               "-Q" "--batch" "--eval" ,(prin1-to-string command))
+             " "))
+
+(defvar esh-proc-test/emacs-echo
+  (esh-proc-test/emacs-command '(princ "hello\n"))
+  "A command that prints \"hello\" to stdout using Emacs.")
+
+(defvar esh-proc-test/emacs-upcase
+  (esh-proc-test/emacs-command
+   '(princ (upcase (concat (read-string "") "\n"))))
+  "A command that upcases the text from stdin using Emacs.")
+
+(ert-deftest esh-proc-test/synchronous-proc/simple/interactive ()
+  "Test that synchronous processes work in an interactive shell."
+  (let ((eshell-supports-asynchronous-processes nil))
+    (with-temp-eshell
+     (eshell-match-command-output esh-proc-test/emacs-echo
+                                  "\\`hello\n"))))
+
+(ert-deftest esh-proc-test/synchronous-proc/simple/command-result ()
+  "Test that synchronous processes work via `eshell-command-result'."
+  (let ((eshell-supports-asynchronous-processes nil))
+    (eshell-command-result-equal esh-proc-test/emacs-echo
+                                 "hello\n")))
+
+(ert-deftest esh-proc-test/synchronous-proc/pipeline/interactive ()
+  "Test that synchronous pipelines work in an interactive shell."
+  (let ((eshell-supports-asynchronous-processes nil))
+    (with-temp-eshell
+     (eshell-match-command-output (concat esh-proc-test/emacs-echo " | "
+                                          esh-proc-test/emacs-upcase)
+                                  "\\`HELLO\n"))))
+
+(ert-deftest esh-proc-test/synchronous-proc/pipeline/command-result ()
+  "Test that synchronous pipelines work via `eshell-command-result'."
+  (let ((eshell-supports-asynchronous-processes nil))
+    (eshell-command-result-equal (concat esh-proc-test/emacs-echo " | "
+                                          esh-proc-test/emacs-upcase)
+                                 "HELLO\n")))
+
 \f
 ;; Killing processes
 
-- 
2.25.1