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

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

[Yegor Timoshenko]

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

GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.26)
 of 2018-02-25

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

[Jim Porter]

[-- 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

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

[Michael Albinus]

Jim Porter <jporterbugs@gmail.com> writes:

Hi Jim,

> 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.

(skip-unless (executable-find "echo"))

Best regards, Michael.

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

[Jim Porter]

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

On 1/18/2022 12:33 AM, Michael Albinus wrote:
> (skip-unless (executable-find "echo"))

Oh, right. I'd forgotten about `skip-unless'. Here's a patch with that 
added. Thanks.

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

From 5f2e3b12fd018ff64e4c8000d2e6fe293532e188 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 18 Jan 2022 10:04:22 -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 | 23 +++++++++++++
 2 files changed, 66 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..c4cb9bf485 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,13 @@ 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"
+  (skip-unless (executable-find "echo"))
+  (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

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

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

> Oh, right. I'd forgotten about `skip-unless'. Here's a patch with that
> added. Thanks.

Looks good to me; pushed to Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Jim Porter]

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

On 1/20/2022 5:38 AM, Lars Ingebrigtsen wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
> 
>> Oh, right. I'd forgotten about `skip-unless'. Here's a patch with that
>> added. Thanks.
> 
> Looks good to me; pushed to Emacs 29.

Drat. I just found bug#12689, which has a wider variety of test cases, 
and saw that I missed a pretty glaring case here:

   echo ${*echo hi}-there

That is, using a subcommand that gets concatenated to a constant string 
to form the argument (or other variations involving concatenation). Both 
before and after my prior fix, that would print "nil-there". With this 
new patch, it prints "hi-there" as expected.

This new patch should be considerably more robust, since it searches 
recursively for any `(eshell-as-subcommand FOO)' forms to check them. 
That way we don't require the command form to look *exactly* one way. I 
used a generator for this, since that's the clearest to my eyes, but I'm 
open to other implementations.

For completeness, this only fixes the first of two issues in bug#12689 
as described by Samer Masterson:

> There are two issues contained in this bug: eshell-plain-command doesn't
> wait for the process to finish before returning, and echo parses output
> from subcommands as lisp objects instead of as args.
The latter case is,

   echo ${*echo -e "foo\nbar"}-baz

which used to print "nil-baz" (or '("foo" "bar")-baz' if you use *echo 
in both spots). With my patch here, it always prints '("foo" 
"bar")-baz', which is at least wrong in a consistent way now. :)

[-- Attachment #2: 0001-Further-improve-determination-of-when-commands-can-b.patch --]
[-- Type: text/plain, Size: 3981 bytes --]

From e85ac190e432bcdabcd24431758e07bfbab385ab Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 20 Jan 2022 18:51:14 -0800
Subject: [PATCH] Further improve determination of when commands can be invoked
 directly

This covers the case when a subcommand is to be invoked in more places
than before, for example when a subcommand is concatenated in an
argument.

* lisp/eshell/esh-cmd.el (eshell--find-subcommands): New fuction.
(eshell--invoke-command-directly): Use 'eshell-find-subcommands'.

* test/lisp/eshell/eshell-tests.el
(eshell-test/interp-cmd-external-concat): New test.
---
 lisp/eshell/esh-cmd.el           | 28 +++++++++++++++++-----------
 test/lisp/eshell/eshell-tests.el |  7 +++++++
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 25e3a5a205..04d65df4f3 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -107,6 +107,7 @@
 (require 'esh-module)
 (require 'esh-io)
 (require 'esh-ext)
+(require 'generator)
 
 (eval-when-compile
   (require 'cl-lib)
@@ -903,6 +904,17 @@ pcomplete/eshell-mode/eshell-debug
   "Completion for the `debug' command."
   (while (pcomplete-here '("errors" "commands"))))
 
+(iter-defun eshell--find-subcommands (haystack)
+  "Recursively search for subcommand forms in HAYSTACK.
+This yields the SUBCOMMANDs when found in forms like
+\"(eshell-as-subcommand SUBCOMMAND)\"."
+  (dolist (elem haystack)
+    (cond
+     ((eq (car-safe elem) 'eshell-as-subcommand)
+      (iter-yield (cdr elem)))
+     ((listp elem)
+      (iter-yield-from (eshell--find-subcommands elem))))))
+
 (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.
@@ -916,8 +928,7 @@ eshell--invoke-command-directly
 * 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."
+* Any subcommands in ARGS 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)))
@@ -931,15 +942,10 @@ eshell--invoke-command-directly
 	         (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))))))))))
+             (iter-do (subcommand (eshell--find-subcommands args))
+               (unless (eshell--invoke-command-directly subcommand)
+                 (throw 'indirect-subcommand nil)))
+             t)))))
 
 (defun eshell-invoke-directly (command)
   "Determine whether the given COMMAND can be invoked directly.
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index c4cb9bf485..1a7ab0ab06 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -167,6 +167,13 @@ eshell-test/interp-cmd-external
    (eshell-command-result-p "echo ${*echo hi}"
                             "hi\n")))
 
+(ert-deftest eshell-test/interp-cmd-external-concat ()
+  "Interpolate command result from external command with concatenation"
+  (skip-unless (executable-find "echo"))
+  (with-temp-eshell
+   (eshell-command-result-p "echo ${echo hi}-${*echo there}"
+                            "hi-there\n")))
+
 (ert-deftest eshell-test/window-height ()
   "$LINES should equal (window-height)"
   (should (eshell-test-command-result "= $LINES (window-height)")))
-- 
2.25.1

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

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

> This new patch should be considerably more robust, since it searches
> recursively for any `(eshell-as-subcommand FOO)' forms to check
> them. That way we don't require the command form to look *exactly* one
> way. I used a generator for this, since that's the clearest to my
> eyes, but I'm open to other implementations.

Looks fine to me.  Pushed to Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no