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

[Jim Porter <jporterbugs@gmail.com>]

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