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

From: [email-masked] (GNU bug Tracking System)
Date: Sun, 02 Apr 2023 22:10:02 +0000

Your bug report

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

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
62556: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=62556
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#62556: 29.0.60; [PATCH] Fix regression when calling 'eshell-command' with a pipeline in the background
From: Jim Porter <[email-masked]>
To: Eli Zaretskii <[email-masked]>
Cc: [email-masked]
Date: Sun, 2 Apr 2023 15:09:01 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

On 4/1/2023 10:31 AM, Jim Porter wrote:
> I'll make sure these issues are addressed in the patches for master and 
> then push them in a day or so, too.

Pushed to master as 093a360251a. Closing this now.

Hopefully this works on MS-Windows; I think I wrote it in a way where it 
should behave the same on all platforms. If not, just let me know, and 
I'll try to fix it.

Subject: 29.0.60; [PATCH] Fix regression when calling 'eshell-command' with a pipeline in the background
From: Jim Porter <[email-masked]>
To: [email-masked]
Date: Thu, 30 Mar 2023 21:18:10 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

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.

From 70b62fc6173ec25d6c4c82e86af7cb4bacc75baa Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
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

From bed83773f46c5fc72e9694139ddbc20ba0990c62 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
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

From e506ea46e24967fddff6d86ff9597686764d8244 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
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