bug#62556: 29.0.60; [PATCH] Fix regression when calling 'eshell-command' with a pipeline in the background

[Jim Porter <jporterbugs@gmail.com>]

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

Starting from "emacs -Q":

   M-x eshell-command RET *echo hi | *cat & RET

You'll see the following error in the *Messages* buffer instead of the 
result:

   eshell-eval-command: Unmatched delimiter: (#<process echo> . 
#<process cat>)

This is a regression from my fix for bug#53715, which changed how
Eshell pipelines return the processes in the pipeline. Attached are some 
patches to fix it. One for the emacs-29 branch, and one for master.

Eli, is it ok to merge the patch for emacs-29? I tried to keep the 
change as minimal as possible for that branch.

The patch for master is a bit more extensive, and also fixes another 
issue where this would fail due to incorrect syntax in the Eshell 
command form:

   (eshell-command "*echo hi &" t)

Previously, it turned the command into "*echo hi & >>> 
#<current-buffer>", but that's not right; the "&" needs to go last.

For master, I also thought it would be nice to clean up 'eshell-command' 
slightly (see patch 0002); this just changes it to handle its arguments 
in the interactive spec.

[-- Attachment #2: emacs-29--0001-Fix-using-background-commands-in-eshell-command.patch --]
[-- Type: text/plain, Size: 4687 bytes --]

From 70b62fc6173ec25d6c4c82e86af7cb4bacc75baa Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 30 Mar 2023 17:39:24 -0700
Subject: [PATCH] Fix using background commands in 'eshell-command'

Do not merge to master.

This regressed due to the patch for bug#53715, which changed how
Eshell pipelines return the processes in the pipeline.

* lisp/eshell/esh-cmd.el (eshell-eval-command): Allow process-pairs.

* test/lisp/eshell/eshell-tests.el (eshell-test/eshell-command/simple)
(eshell-test/eshell-command/pipeline)
(eshell-test/eshell-command/background)
(eshell-test/eshell-command/background-pipeline): New tests.
---
 lisp/eshell/esh-cmd.el           |  8 ++++--
 test/lisp/eshell/eshell-tests.el | 47 ++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index f4ac384ccc5..706477a5f45 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -1032,18 +1032,20 @@ eshell-eval-command
     (setq eshell-current-command command)
     (let* ((delim (catch 'eshell-incomplete
                     (eshell-resume-eval)))
-           (val (car-safe delim)))
+           (val (car-safe delim))
+           (val-is-process (or (eshell-processp val)
+                               (eshell-process-pair-p val))))
       ;; If the return value of `eshell-resume-eval' is wrapped in a
       ;; list, it indicates that the command was run asynchronously.
       ;; In that case, unwrap the value before checking the delimiter
       ;; value.
       (if (and val
-               (not (eshell-processp val))
+               (not val-is-process)
                (not (eq val t)))
           (error "Unmatched delimiter: %S" val)
         ;; Eshell-command expect a list like (<process>) to know if the
         ;; command should be async or not.
-        (or (and (eshell-processp val) delim) val)))))
+        (or (and val-is-process delim) val)))))
 
 (defun eshell-resume-command (proc status)
   "Resume the current command when a process ends."
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index 3c4a8ec97ea..b57abe3226c 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -105,6 +105,53 @@ eshell-test/lisp-reset-in-pipeline
      (format template "format \"%s\" eshell-in-pipeline-p")
      "nil")))
 
+(ert-deftest eshell-test/eshell-command/simple ()
+  "Test that the `eshell-command' function writes to the current buffer."
+  (skip-unless (executable-find "echo"))
+  (ert-with-temp-directory eshell-directory-name
+    (let ((eshell-history-file-name nil))
+      (with-temp-buffer
+        (eshell-command "*echo hi" t)
+        (should (equal (buffer-string) "hi\n"))))))
+
+(ert-deftest eshell-test/eshell-command/pipeline ()
+  "Test that the `eshell-command' function writes to the current buffer.
+This test uses a pipeline for the command."
+  (skip-unless (and (executable-find "echo")
+                    (executable-find "cat")))
+  (ert-with-temp-directory eshell-directory-name
+    (let ((eshell-history-file-name nil))
+      (with-temp-buffer
+        (eshell-command "*echo hi | *cat" t)
+        (should (equal (buffer-string) "hi\n"))))))
+
+(ert-deftest eshell-test/eshell-command/background ()
+  "Test that `eshell-command' works for background commands."
+  (skip-unless (executable-find "echo"))
+  (ert-with-temp-directory eshell-directory-name
+    (let ((eshell-history-file-name nil))
+      ;; XXX: We can't write to the current buffer here, since
+      ;; `eshell-command' will produce an invalid command in that
+      ;; case.  Just make sure the command runs and produces an output
+      ;; buffer.
+      (eshell-command "*echo hi &")
+      (with-current-buffer "*Eshell Async Command Output*"
+        (goto-char (point-min))
+        (should (looking-at "\\[echo\\(<[0-9]+>\\)?\\]"))))))
+
+(ert-deftest eshell-test/eshell-command/background-pipeline ()
+  "Test that `eshell-command' works for background commands.
+This test uses a pipeline for the command."
+  (skip-unless (and (executable-find "echo")
+                    (executable-find "cat")))
+  (ert-with-temp-directory eshell-directory-name
+    (let ((eshell-history-file-name nil))
+      ;; XXX: As above, we can't write to the current buffer here.
+      (eshell-command "*echo hi | *cat &")
+      (with-current-buffer "*Eshell Async Command Output*"
+        (goto-char (point-min))
+        (should (looking-at "\\[cat\\(<[0-9]+>\\)?\\]"))))))
+
 (ert-deftest eshell-test/command-running-p ()
   "Modeline should show no command running"
   (with-temp-eshell
-- 
2.25.1


[-- Attachment #3: emacs-30--0001-Fix-using-background-commands-in-eshell-command.patch --]
[-- Type: text/plain, Size: 7579 bytes --]

From bed83773f46c5fc72e9694139ddbc20ba0990c62 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 30 Mar 2023 19:31:30 -0700
Subject: [PATCH 1/2] Fix using background commands in 'eshell-command'

This regressed due to the patch for bug#53715, which changed how
Eshell pipelines return the processes in the pipeline.

* lisp/eshell/esh-cmd.el (eshell-parse-command): When creating
background commands, wrap the process(es) in a cons cell whose CAR is
':eshell-background'.  This lets us use fewer heuristics...
(eshell-eval-command): ... here.  Additionally, keep the result and
the incomplete delimiter separate.

* lisp/eshell/eshell.el (eshell-command): Check ':eshell-background'
and use a more-robust method for setting the output target.

* test/lisp/eshell/eshell-tests.el (eshell-test/eshell-command/simple)
(eshell-test/eshell-command/pipeline)
(eshell-test/eshell-command/background)
(eshell-test/eshell-command/background-pipeline): New tests.
---
 lisp/eshell/esh-cmd.el           | 30 ++++++++++------------
 lisp/eshell/eshell.el            | 21 +++++----------
 test/lisp/eshell/eshell-tests.el | 44 ++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 30 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index d5237ee1f04..600a2c3af0b 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -421,7 +421,8 @@ eshell-parse-command
                            (string= (car eshell--sep-terms) ";"))
                        (eshell-parse-pipeline cmd)
                      `(eshell-do-subjob
-                       (list ,(eshell-parse-pipeline cmd)))))
+                       (cons :eshell-background
+                             ,(eshell-parse-pipeline cmd)))))
              (setq eshell--sep-terms (cdr eshell--sep-terms))
              (if eshell-in-pipeline-p
                  cmd
@@ -1030,7 +1031,12 @@ eshell-eval-argument
     (cadr result)))
 
 (defun eshell-eval-command (command &optional input)
-  "Evaluate the given COMMAND iteratively."
+  "Evaluate the given COMMAND iteratively.
+Return the process (or head and tail processes) created by
+COMMAND, if any.  If COMMAND is a background command, return the
+process(es) in a cons cell like:
+
+  (:eshell-background . PROCESS)"
   (if eshell-current-command
       ;; We can just stick the new command at the end of the current
       ;; one, and everything will happen as it should.
@@ -1046,20 +1052,12 @@ eshell-eval-command
            (erase-buffer)
            (insert "command: \"" input "\"\n")))
     (setq eshell-current-command command)
-    (let* ((delim (catch 'eshell-incomplete
-                    (eshell-resume-eval)))
-           (val (car-safe delim)))
-      ;; If the return value of `eshell-resume-eval' is wrapped in a
-      ;; list, it indicates that the command was run asynchronously.
-      ;; In that case, unwrap the value before checking the delimiter
-      ;; value.
-      (if (and val
-               (not (eshell-processp val))
-               (not (eq val t)))
-          (error "Unmatched delimiter: %S" val)
-        ;; Eshell-command expect a list like (<process>) to know if the
-        ;; command should be async or not.
-        (or (and (eshell-processp val) delim) val)))))
+    (let* (result
+           (delim (catch 'eshell-incomplete
+                    (ignore (setq result (eshell-resume-eval))))))
+      (when delim
+        (error "Unmatched delimiter: %S" delim))
+      result)))
 
 (defun eshell-resume-command (proc status)
   "Resume the current command when a process ends."
diff --git a/lisp/eshell/eshell.el b/lisp/eshell/eshell.el
index 7d2c0335db2..b71f283bf9f 100644
--- a/lisp/eshell/eshell.el
+++ b/lisp/eshell/eshell.el
@@ -290,25 +290,18 @@ eshell-command
 	    (eshell-add-input-to-history command)))))
   (unless command
     (error "No command specified!"))
-  ;; redirection into the current buffer is achieved by adding an
-  ;; output redirection to the end of the command, of the form
-  ;; 'COMMAND >>> #<buffer BUFFER>'.  This will not interfere with
-  ;; other redirections, since multiple redirections merely cause the
-  ;; output to be copied to multiple target locations
-  (if arg
-      (setq command
-	    (concat command
-		    (format " >>> #<buffer %s>"
-			    (buffer-name (current-buffer))))))
   (save-excursion
-    (let ((buf (set-buffer (generate-new-buffer " *eshell cmd*")))
+    (let ((stdout (if arg (current-buffer) t))
+          (buf (set-buffer (generate-new-buffer " *eshell cmd*")))
 	  (eshell-non-interactive-p t))
       (eshell-mode)
       (let* ((proc (eshell-eval-command
-		    (list 'eshell-commands
-			  (eshell-parse-command command))))
+                    `(let ((eshell-current-handles
+                            (eshell-create-handles ,stdout 'insert))
+                           (eshell-current-subjob-p))
+		       ,(eshell-parse-command command))))
 	     intr
-	     (bufname (if (and proc (listp proc))
+	     (bufname (if (eq (car-safe proc) :eshell-background)
 			  "*Eshell Async Command Output*"
 			(setq intr t)
 			"*Eshell Command Output*")))
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index 743cc28b9b5..390f75cfbb9 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -107,6 +107,50 @@ eshell-test/lisp-reset-in-pipeline
      (format template "format \"%s\" eshell-in-pipeline-p")
      "nil")))
 
+(ert-deftest eshell-test/eshell-command/simple ()
+  "Test that the `eshell-command' function writes to the current buffer."
+  (skip-unless (executable-find "echo"))
+  (ert-with-temp-directory eshell-directory-name
+    (let ((eshell-history-file-name nil))
+      (with-temp-buffer
+        (eshell-command "*echo hi" t)
+        (should (equal (buffer-string) "hi\n"))))))
+
+(ert-deftest eshell-test/eshell-command/pipeline ()
+  "Test that the `eshell-command' function writes to the current buffer.
+This test uses a pipeline for the command."
+  (skip-unless (and (executable-find "echo")
+                    (executable-find "cat")))
+  (ert-with-temp-directory eshell-directory-name
+    (let ((eshell-history-file-name nil))
+      (with-temp-buffer
+        (eshell-command "*echo hi | *cat" t)
+        (should (equal (buffer-string) "hi\n"))))))
+
+(ert-deftest eshell-test/eshell-command/background ()
+  "Test that `eshell-command' works for background commands."
+  (skip-unless (executable-find "echo"))
+  (ert-with-temp-directory eshell-directory-name
+    (let ((orig-processes (process-list))
+          (eshell-history-file-name nil))
+      (with-temp-buffer
+        (eshell-command "*echo hi &" t)
+        (eshell-wait-for (lambda () (equal (process-list) orig-processes)))
+        (should (equal (buffer-string) "hi\n"))))))
+
+(ert-deftest eshell-test/eshell-command/background-pipeline ()
+  "Test that `eshell-command' works for background commands.
+This test uses a pipeline for the command."
+  (skip-unless (and (executable-find "echo")
+                    (executable-find "cat")))
+  (ert-with-temp-directory eshell-directory-name
+    (let ((orig-processes (copy-tree (process-list)))
+          (eshell-history-file-name nil))
+      (with-temp-buffer
+        (eshell-command "*echo hi | *cat &" t)
+        (eshell-wait-for (lambda () (equal (process-list) orig-processes)))
+        (should (equal (buffer-string) "hi\n"))))))
+
 (ert-deftest eshell-test/command-running-p ()
   "Modeline should show no command running"
   (with-temp-eshell
-- 
2.25.1


[-- Attachment #4: emacs-30--0002-Use-the-interactive-spec-to-set-arguments-for-eshell.patch --]
[-- Type: text/plain, Size: 2807 bytes --]

From e506ea46e24967fddff6d86ff9597686764d8244 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 30 Mar 2023 19:38:30 -0700
Subject: [PATCH 2/2] Use the 'interactive' spec to set arguments for
 'eshell-command'

* lisp/eshell/eshell.el (eshell-read-command): New function...
(eshell-command): ... use it.  Additionally, require the COMMAND
argument, and rename ARG to TO-CURRENT-BUFFER.
---
 lisp/eshell/eshell.el | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/lisp/eshell/eshell.el b/lisp/eshell/eshell.el
index b71f283bf9f..15fc2ae6310 100644
--- a/lisp/eshell/eshell.el
+++ b/lisp/eshell/eshell.el
@@ -272,26 +272,28 @@ eshell-non-interactive-p
 
 (declare-function eshell-add-input-to-history "em-hist" (input))
 
-;;;###autoload
-(defun eshell-command (&optional command arg)
-  "Execute the Eshell command string COMMAND.
-With prefix ARG, insert output into the current buffer at point."
-  (interactive)
-  (unless arg
-    (setq arg current-prefix-arg))
-  (let ((eshell-non-interactive-p t))
+(defun eshell-read-command (&optional prompt)
+  "Read an Eshell command from the minibuffer, prompting with PROMPT."
+  (let ((prompt (or prompt "Emacs shell command: "))
+        (eshell-non-interactive-p t))
     ;; Enable `eshell-mode' only in this minibuffer.
     (minibuffer-with-setup-hook (lambda ()
                                   (eshell-mode)
                                   (eshell-command-mode +1))
-      (unless command
-        (setq command (read-from-minibuffer "Emacs shell command: "))
-	(if (eshell-using-module 'eshell-hist)
-	    (eshell-add-input-to-history command)))))
-  (unless command
-    (error "No command specified!"))
+      (let ((command (read-from-minibuffer prompt)))
+        (when (eshell-using-module 'eshell-hist)
+          (eshell-add-input-to-history command))
+        command))))
+
+;;;###autoload
+(defun eshell-command (command &optional to-current-buffer)
+  "Execute the Eshell command string COMMAND.
+If TO-CURRENT-BUFFER is non-nil (interactively, with the prefix
+argument), then insert output into the current buffer at point."
+  (interactive (list (eshell-read-command)
+                     current-prefix-arg))
   (save-excursion
-    (let ((stdout (if arg (current-buffer) t))
+    (let ((stdout (if to-current-buffer (current-buffer) t))
           (buf (set-buffer (generate-new-buffer " *eshell cmd*")))
 	  (eshell-non-interactive-p t))
       (eshell-mode)
@@ -319,7 +321,7 @@ eshell-command
 	  (while (and (bolp) (not (bobp)))
 	    (delete-char -1)))
 	(cl-assert (and buf (buffer-live-p buf)))
-	(unless arg
+	(unless to-current-buffer
 	  (let ((len (if (not intr) 2
 		       (count-lines (point-min) (point-max)))))
 	    (cond
-- 
2.25.1