Subject: bug#60942: closed (Re: bug#60942: 30.0.50; [PATCH] Indices in Eshell variable interpolation don't work with async subcommands)

From: [email-masked] (GNU bug Tracking System)
Date: Fri, 20 Jan 2023 01:55:02 +0000

Your bug report

#60942: 30.0.50; [PATCH] Indices in Eshell variable interpolation don't work with async subcommands

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

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

Subject: Re: bug#60942: 30.0.50; [PATCH] Indices in Eshell variable interpolation don't work with async subcommands
From: Jim Porter <[email-masked]>
To: Eli Zaretskii <[email-masked]>
Cc: [email-masked]
Date: Thu, 19 Jan 2023 17:54:01 -0800
List-Id: <debbugs-submit.debbugs.gnu.org>

On 1/19/2023 12:20 PM, Jim Porter wrote:
> On 1/19/2023 11:41 AM, Eli Zaretskii wrote:
>> Is this for master?  If so, okay.  Otherwise, you'll need to adjust
>> the version in the obsolescence declaration.
> 
> Personally, I think just for master. The change isn't entirely 
> risk-free. This code is a bit tricky, and I'd want a fair amount of time 
> for people to identify bugs, just in case I regressed something.

And now merged to master as 54d5ea66c9. Closing this bug.

Subject: 30.0.50; [PATCH] Indices in Eshell variable interpolation don't work with async subcommands
From: Jim Porter <[email-masked]>
To: [email-masked]
Date: Wed, 18 Jan 2023 19:36:41 -0800
List-Id: <debbugs-submit.debbugs.gnu.org>

Starting from "emacs -Q -f eshell":

   ~ $ echo $exec-path[0]
   /usr/local/sbin
   ~ $ echo $exec-path[${echo 0}]
   /usr/local/sbin
   ~ $ echo $exec-path[${*echo 0}]
   ;; no output

This is because 'eshell-eval-indices' gets an S-expr describing code to 
evaluate for the indices, and it just passes that to 'eval'. That's not 
the right way to do things for Eshell: instead, we should rely on 
'eshell-do-eval', which properly handles asynchronous evaluation. That's 
required for working with external commands like "*echo" (which calls 
the real /bin/echo).

The attached patch fixes this by changing 'eshell-eval-indices' to 
'eshell-indices', which does some minimal transformations on the S-expr 
for the indices, and then uses it to build the final S-expr to pass to 
'eshell-do-eval'.

This could possibly go in Emacs 29, since it's a bugfix to add onto a 
previous bugfix (see commit 990f36fa10). However, I'd lean towards just 
merging to master; this is a fairly obscure issue, and we can't just fix 
*every* bug we find on the release branch, or the branch will never 
stabilize. If someone else thinks it's important enough to go on the 
release branch though, I won't argue.

From 1bbaf547d8d6668bca732e14dc190416c5b52671 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Wed, 18 Jan 2023 19:15:38 -0800
Subject: [PATCH] Fix evaluation of asynchronous expansions in Eshell indices

Previously, this code passed the indices to a separate function, which
called 'eval' on them, but it should instead make an S-expr that
'eshell-do-eval' can evaluate.

* lisp/eshell/esh-var.el (eshell-eval-indices): Rename to...
(eshell-indices): ... this, and adjust implementation to return a form
to evaluate via 'eshell-do-eval'.
(eshell-parse-variable): Use 'eshell-indices'.  Also, remove
irrelevant comment.
(eshell-parse-variable-ref): Fix quoting in docstring.
(eshell-parse-indices): Fix typo in docstring.

* test/lisp/eshell/esh-var-tests.el
(esh-var-test/interp-var-indices-subcommand)
(esh-var-test/quoted-interp-var-indices-subcommand): New tests.
---
 lisp/eshell/esh-var.el            | 15 +++++++--------
 test/lisp/eshell/esh-var-tests.el | 25 +++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index fd76a2c6f09..2da35222044 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -467,9 +467,7 @@ eshell-parse-variable
 	  indices (and (not (eobp))
 		       (eq (char-after) ?\[)
 		       (eshell-parse-indices))
-          ;; This is an expression that will be evaluated by `eshell-do-eval',
-          ;; which only support let-binding of dynamically-scoped vars
-	  value `(let ((indices (eshell-eval-indices ',indices))) ,value))
+          value `(let ((indices ,(eshell-indices indices))) ,value))
     (when get-len
       (setq value `(length ,value)))
     (when eshell-current-quoted
@@ -496,7 +494,7 @@ eshell-parse-variable-ref
 
   NAME          an environment or Lisp variable value
   \"LONG-NAME\"   disambiguates the length of the name
-  `LONG-NAME'   as above
+  \\='LONG-NAME\\='   as above
   {COMMAND}     result of command is variable's value
   (LISP-FORM)   result of Lisp form is variable's value
   <COMMAND>     write the output of command to a temporary file;
@@ -591,7 +589,7 @@ eshell-parse-indices
   "Parse and return a list of index-lists.
 
 For example, \"[0 1][2]\" becomes:
-  ((\"0\" \"1\") (\"2\")."
+  ((\"0\" \"1\") (\"2\"))."
   (let (indices)
     (while (eq (char-after) ?\[)
       (let ((end (eshell-find-delimiter ?\[ ?\])))
@@ -607,9 +605,10 @@ eshell-parse-indices
 	  (goto-char (1+ end)))))
     (nreverse indices)))
 
-(defun eshell-eval-indices (indices)
-  "Evaluate INDICES, a list of index-lists generated by `eshell-parse-indices'."
-  (mapcar (lambda (i) (mapcar #'eval i)) indices))
+(defun eshell-indices (indices)
+  "Prepare INDICES to be evaluated by Eshell.
+INDICES is a list of index-lists generated by `eshell-parse-indices'."
+  `(list ,@(mapcar (lambda (idx-list) (cons 'list idx-list)) indices)))
 
 (defun eshell-get-variable (name &optional indices quoted)
   "Get the value for the variable NAME.
diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el
index 0cc1b92266f..82324d72163 100644
--- a/test/lisp/eshell/esh-var-tests.el
+++ b/test/lisp/eshell/esh-var-tests.el
@@ -82,6 +82,17 @@ esh-var-test/interp-var-indices
     (eshell-command-result-equal "echo $eshell-test-value[0 2 4]"
                                  '("zero" "two" "four"))))
 
+(ert-deftest esh-var-test/interp-var-indices-subcommand ()
+  "Interpolate list variable with subcommand expansion for indices"
+  (skip-unless (executable-find "echo"))
+  (let ((eshell-test-value '("zero" "one" "two" "three" "four")))
+    (eshell-command-result-equal
+     "echo $eshell-test-value[${*echo 0}]"
+     "zero")
+    (eshell-command-result-equal
+     "echo $eshell-test-value[${*echo 0} ${*echo 2}]"
+     '("zero" "two"))))
+
 (ert-deftest esh-var-test/interp-var-split-indices ()
   "Interpolate string variable with indices"
   (let ((eshell-test-value "zero one two three four"))
@@ -271,6 +282,20 @@ esh-var-test/quoted-interp-var-indices
     (eshell-command-result-equal "echo \"$eshell-test-value[1 2 4]\""
                                  "(\"one\" \"two\" \"four\")")))
 
+(ert-deftest esh-var-test/quote-interp-var-indices-subcommand ()
+  "Interpolate list variable with subcommand expansion for indices
+inside double-quotes"
+  (skip-unless (executable-find "echo"))
+  (let ((eshell-test-value '("zero" "one" "two" "three" "four")))
+    (eshell-command-result-equal
+     "echo \"$eshell-test-value[${*echo 0}]\""
+     "zero")
+    ;; FIXME: These tests would use the 0th index like the other tests
+    ;; here, but see above.
+    (eshell-command-result-equal
+     "echo \"$eshell-test-value[${*echo 1} ${*echo 2}]\""
+     "(\"one\" \"two\")")))
+
 (ert-deftest esh-var-test/quoted-interp-var-split-indices ()
   "Interpolate string variable with indices inside double-quotes"
   (let ((eshell-test-value "zero one two three four"))
-- 
2.25.1