bug#30725: eshell: built-ins do not handle command substitution

[Jim Porter <jporterbugs@gmail.com>]

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

On 3/5/2018 8:34 PM, Yegor Timoshenko wrote:
> In M-x eshell:
> 
>    $ which echo
>    eshell/echo is a compiled Lisp function in `em-basic.el'.
>    $ which *echo
>    /run/current-system/sw/bin/echo
>    $ echo ${mktemp -d}
>    $ *echo ${mktemp -d}
>    /tmp/tmp.UaiWQ0YPIX

I can see this bug with an even simpler case too: "echo ${*echo hi}".

It turns out that this is because `eshell-invoke-directly' thought that 
the above command was simple enough to, well, invoke directly. However, 
since "${mktemp -d}" or "${*echo hi}" create a subprocess, the command 
needs to be invoked *iteratively* by `eshell-eval-command'. The problem 
was that `eshell-invoke-directly' only checked the top-level command and 
didn't examine subcommands.

Attached is a patch that fixes this, plus a unit test (I've verified 
that the test fails without the patch and passes with it). Note that the 
test *does* rely on the system having an external "echo" command, but I 
think some of the tests in that file already rely on the presence of an 
external "sleep" command, so this should be ok. However, if it causes 
issues on some systems (MS Windows maybe?), just let me know and I can 
try to put a guard around the test so it doesn't run on such systems.

[-- Attachment #2: 0001-Consider-subcommands-when-deciding-to-invoke-Eshell-.patch --]
[-- Type: text/plain, Size: 5810 bytes --]

From dd0bef6cf77bcc20f374f63003675218291a4638 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 17 Jan 2022 11:28:16 -0800
Subject: [PATCH] Consider subcommands when deciding to invoke Eshell command
 directly

When an Eshell command contains an asynchronous subcommand (such as
calling an external process), it must be evaluated iteratively.  See
bug#30725.

* lisp/eshell/esh-cmd.el (eshell-invoke-command): Move most of the
logic from here...
(eshell--invoke-command-directly): ... to here. Also add checks for
subcommands.

* test/lisp/eshell/eshell-tests.el (eshell-test--max-subprocess-time):
New variable.
(eshell-wait-for-subprocess): New function.
(eshell-command-result-p): Use 'eshell-wait-for-subprocess'.
(eshell-test/interp-cmd-external): New test.
---
 lisp/eshell/esh-cmd.el           | 57 ++++++++++++++++++++++++--------
 test/lisp/eshell/eshell-tests.el | 22 ++++++++++++
 2 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index a2d7d9431a..25e3a5a205 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -903,21 +903,50 @@ pcomplete/eshell-mode/eshell-debug
   "Completion for the `debug' command."
   (while (pcomplete-here '("errors" "commands"))))
 
+(defun eshell--invoke-command-directly (command)
+  "Determine whether the given COMMAND can be invoked directly.
+COMMAND should be a non-top-level Eshell command in parsed form.
+
+A command can be invoked directly if all of the following are true:
+
+* The command is of the form
+  \"(eshell-trap-errors (eshell-named-command NAME ARGS))\",
+  where ARGS is optional.
+
+* NAME is a string referring to an alias function and isn't a
+  complex command (see `eshell-complex-commands').
+
+* Any argument in ARGS that calls a subcommand can also be
+  invoked directly."
+  (when (and (eq (car command) 'eshell-trap-errors)
+             (eq (car (cadr command)) 'eshell-named-command))
+    (let ((name (cadr (cadr command)))
+          (args (cdr-safe (nth 2 (cadr command)))))
+      (and name (stringp name)
+	   (not (member name eshell-complex-commands))
+	   (catch 'simple
+	     (dolist (pred eshell-complex-commands t)
+	       (when (and (functionp pred)
+		          (funcall pred name))
+	         (throw 'simple nil))))
+	   (eshell-find-alias-function name)
+           (catch 'indirect-subcommand
+	     (dolist (arg args t)
+               (pcase arg
+                 (`(eshell-escape-arg
+                    (let ,_
+                      (eshell-convert
+                       (eshell-command-to-value
+                        (eshell-as-subcommand ,subcommand)))))
+                  (unless (eshell--invoke-command-directly subcommand)
+                    (throw 'indirect-subcommand nil))))))))))
+
 (defun eshell-invoke-directly (command)
-  (let ((base (cadr (nth 2 (nth 2 (cadr command))))) name)
-    (if (and (eq (car base) 'eshell-trap-errors)
-	     (eq (car (cadr base)) 'eshell-named-command))
-	(setq name (cadr (cadr base))))
-    (and name (stringp name)
-	 (not (member name eshell-complex-commands))
-	 (catch 'simple
-	   (progn
-	    (dolist (pred eshell-complex-commands)
-	      (if (and (functionp pred)
-		       (funcall pred name))
-		  (throw 'simple nil)))
-	    t))
-	 (eshell-find-alias-function name))))
+  "Determine whether the given COMMAND can be invoked directly.
+COMMAND should be a top-level Eshell command in parsed form, as
+produced by `eshell-parse-command'."
+  (let ((base (cadr (nth 2 (nth 2 (cadr command))))))
+    (eshell--invoke-command-directly base)))
 
 (defun eshell-eval-command (command &optional input)
   "Evaluate the given COMMAND iteratively."
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index aef1447907..9cc997c4cf 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -30,6 +30,10 @@
 (require 'esh-mode)
 (require 'eshell)
 
+(defvar eshell-test--max-subprocess-time 5
+  "The maximum amount of time to wait for a subprocess to finish, in seconds.
+See `eshell-wait-for-subprocess'.")
+
 (defmacro with-temp-eshell (&rest body)
   "Evaluate BODY in a temporary Eshell buffer."
   `(ert-with-temp-directory eshell-directory-name
@@ -44,6 +48,17 @@ with-temp-eshell
          (let (kill-buffer-query-functions)
            (kill-buffer eshell-buffer))))))
 
+(defun eshell-wait-for-subprocess ()
+  "Wait until there is no interactive subprocess running in Eshell.
+If this takes longer than `eshell-test--max-subprocess-time',
+raise an error."
+  (let ((start (current-time)))
+    (while (eshell-interactive-process)
+      (when (> (float-time (time-since start))
+               eshell-test--max-subprocess-time)
+        (error "timed out waiting for subprocess"))
+      (sit-for 0.1))))
+
 (defun eshell-insert-command (text &optional func)
   "Insert a command at the end of the buffer."
   (goto-char eshell-last-output-end)
@@ -59,6 +74,7 @@ eshell-match-result
 (defun eshell-command-result-p (text regexp &optional func)
   "Insert a command at the end of the buffer."
   (eshell-insert-command text func)
+  (eshell-wait-for-subprocess)
   (eshell-match-result regexp))
 
 (defvar eshell-history-file-name)
@@ -144,6 +160,12 @@ eshell-test/interp-concat-lisp2
   "Interpolate and concat two Lisp forms"
   (should (equal (eshell-test-command-result "+ $(+ 1 2)$(+ 1 2) 3") 36)))
 
+(ert-deftest eshell-test/interp-cmd-external ()
+  "Interpolate command result from external command"
+  (with-temp-eshell
+   (eshell-command-result-p "echo ${*echo hi}"
+                            "hi\n")))
+
 (ert-deftest eshell-test/window-height ()
   "$LINES should equal (window-height)"
   (should (eshell-test-command-result "= $LINES (window-height)")))
-- 
2.25.1