Subject: 29.0.50; [PATCH] Inconsistencies with Eshell variable interpolation

From: Jim Porter <[email-masked]>
Date: Wed, 2 Mar 2022 22:35:22 -0800

There are a few related issues with how Eshell parses variable 
interpolations, especially when mixing with double-quotes. Starting from 
"emacs -Q --eval '(eshell)':

1. Quoted subcommands

   ~ $ echo ${echo hi}
   hi  ;; good, this is what we want
   ~ $ echo "${echo hi}"
   echo hi: command not found

In this case, Eshell gets confused by the double quotes and looks for 
the command named `echo hi', not the command named `echo' with the 
argument `hi'.

2. Complex list indexing

   ~ $ echo $PATH[: 0]
   /usr/local/bin  ;; good (the first element of your $PATH)
   ~ $ echo $PATH[":" 0]
   Wrong type argument: number-or-marker-p, (eshell-escape-arg ":")

Here, Eshell neglects to evaluate the subscript arguments, so only 
literal, unquoted values work. Note that the ":" in this case means 
"split the string $PATH with colon as a delimiter". The same happens 
with Elisp subexpressions:

   ~ $ echo $exec-path[0]
   /usr/local/bin  ;; good, again
   ~ $ echo $exec-path[$(+ 1 -1)]
   Wrong type argument: number-or-marker-p, (eshell-escape-arg (let 
((indices 'nil)) (eshell-command-to-value (eshell-lisp-command '(+ 1 -1)))))

3. Bare-string indexing

   ~ $ setq foo "fooXbarXbaz"
   fooXbarXbaz
   ~ $ echo $foo[X 0]
   (nil
     #("fooXbarXbaz" 0 11
     (escaped t)))

This is similar to the above case, but here Eshell thinks that "X" is a 
literal index to use. First, it splits `foo' into a list using space as 
a delimiter. Then gets confused when it uses a string as an index into a 
list, so it returns nil. The second element of the result is the 0th 
element of `foo'-as-a-list, aka the whole string since there are no 
spaces. (Just ignore for now the fact that the string "fooXbarXbaz" 
mysteriously gained an `escaped' property. That's a separate bug.)

This last one is less of a bug and more of a missing feature: if a bare 
(non-numeric) string is used as an index for a string variable, we know 
it *must* be a delimiter argument. Since we're indexing into a list, a 
string index doesn't really make sense.

--------------------

Attached is a patch series to fix these cases, as well as a lot more 
tests for Eshell variable interpolation, since this can get tricky. I've 
also updated the documentation to clarify how indexing works. I didn't 
initially realize that you could do quite so many complex things with 
it, so I figured it'd be good to explain in more detail so it's easier 
to discover.
From beea98cec6e56fcff7abc63eda00fecb448a3a8c Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Sun, 27 Feb 2022 18:34:22 -0800
Subject: [PATCH 1/5] Move Eshell variable interpolation tests to their own
 file

* test/lisp/eshell/eshell-tests.el (eshell-test/interp-cmd)
(eshell-test/interp-lisp, eshell-test/interp-temp-cmd)
(eshell-test/interp-concat, eshell-test/interp-concat-lisp)
(eshell-test/interp-concat2, eshell-test/interp-concat-lisp2)
(eshell-test/interp-cmd-external)
(eshell-test/interp-cmd-external-concat, eshell-test/window-height)
(eshell-test/window-width, eshell-test/last-result-var)
(eshell-test/last-result-var2, eshell-test/last-arg-var):
Move from here...

* test/lisp/eshell/esh-var-test.el (esh-var-test/interp-lisp)
(esh-var-test/interp-cmd, esh-var-test/interp-cmd-external)
(esh-var-test/interp-temp-cmd, esh-var-test/interp-concat-lisp)
(esh-var-test/interp-concat-lisp2, esh-var-test/interp-concat-cmd)
(esh-var-test/interp-concat-cmd2)
(esh-var-test/interp-concat-cmd-external, esh-var-test/window-height)
(esh-var-test/window-width, esh-var-test/last-result-var)
(esh-var-test/last-result-var2, esh-var-test/last-arg-var):
... to here.
---
 test/lisp/eshell/esh-var-tests.el | 111 ++++++++++++++++++++++++++++++
 test/lisp/eshell/eshell-tests.el  |  68 ------------------
 2 files changed, 111 insertions(+), 68 deletions(-)
 create mode 100644 test/lisp/eshell/esh-var-tests.el

diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el
new file mode 100644
index 0000000000..8d803e5ca4
--- /dev/null
+++ b/test/lisp/eshell/esh-var-tests.el
@@ -0,0 +1,111 @@
+;;; esh-var-tests.el --- esh-var test suite  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2022 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Tests for Eshell's variable handling.
+
+;;; Code:
+
+(require 'ert)
+(require 'esh-mode)
+(require 'eshell)
+
+(require 'eshell-tests-helpers
+         (expand-file-name "eshell-tests-helpers"
+                           (file-name-directory (or load-file-name
+                                                    default-directory))))
+
+;;; Tests:
+
+
+;; Variable interpolation
+
+(ert-deftest esh-var-test/interp-lisp ()
+  "Interpolate Lisp form evaluation"
+  (should (equal (eshell-test-command-result "+ $(+ 1 2) 3") 6)))
+
+(ert-deftest esh-var-test/interp-cmd ()
+  "Interpolate command result"
+  (should (equal (eshell-test-command-result "+ ${+ 1 2} 3") 6)))
+
+(ert-deftest esh-var-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 esh-var-test/interp-temp-cmd ()
+  "Interpolate command result redirected to temp file"
+  (should (equal (eshell-test-command-result "cat $<echo hi>") "hi")))
+
+(ert-deftest esh-var-test/interp-concat-lisp ()
+  "Interpolate and concat Lisp form"
+  (should (equal (eshell-test-command-result "+ $(+ 1 2)3 3") 36)))
+
+(ert-deftest esh-var-test/interp-concat-lisp2 ()
+  "Interpolate and concat two Lisp forms"
+  (should (equal (eshell-test-command-result "+ $(+ 1 2)$(+ 1 2) 3") 36)))
+
+(ert-deftest esh-var-test/interp-concat-cmd ()
+  "Interpolate and concat command"
+  (should (equal (eshell-test-command-result "+ ${+ 1 2}3 3") 36)))
+
+(ert-deftest esh-var-test/interp-concat-cmd2 ()
+  "Interpolate and concat two commands"
+  (should (equal (eshell-test-command-result "+ ${+ 1 2}${+ 1 2} 3") 36)))
+
+(ert-deftest esh-var-test/interp-concat-cmd-external ()
+  "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")))
+
+
+;; Built-in variables
+
+(ert-deftest esh-var-test/window-height ()
+  "$LINES should equal (window-height)"
+  (should (eshell-test-command-result "= $LINES (window-height)")))
+
+(ert-deftest esh-var-test/window-width ()
+  "$COLUMNS should equal (window-width)"
+  (should (eshell-test-command-result "= $COLUMNS (window-width)")))
+
+(ert-deftest esh-var-test/last-result-var ()
+  "Test using the \"last result\" ($$) variable"
+  (with-temp-eshell
+   (eshell-command-result-p "+ 1 2; + $$ 2"
+                            "3\n5\n")))
+
+(ert-deftest esh-var-test/last-result-var2 ()
+  "Test using the \"last result\" ($$) variable twice"
+  (with-temp-eshell
+   (eshell-command-result-p "+ 1 2; + $$ $$"
+                             "3\n6\n")))
+
+(ert-deftest esh-var-test/last-arg-var ()
+  "Test using the \"last arg\" ($_) variable"
+  (with-temp-eshell
+   (eshell-command-result-p "+ 1 2; + $_ 4"
+                             "3\n6\n")))
+
+;; esh-var-tests.el ends here
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index eff4edd62c..e31db07c61 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -85,48 +85,6 @@ eshell-test/subcommand-lisp
 e.g. \"{(+ 1 2)} 3\" => 3"
   (should (equal (eshell-test-command-result "{(+ 1 2)} 3") 3)))
 
-(ert-deftest eshell-test/interp-cmd ()
-  "Interpolate command result"
-  (should (equal (eshell-test-command-result "+ ${+ 1 2} 3") 6)))
-
-(ert-deftest eshell-test/interp-lisp ()
-  "Interpolate Lisp form evaluation"
-  (should (equal (eshell-test-command-result "+ $(+ 1 2) 3") 6)))
-
-(ert-deftest eshell-test/interp-temp-cmd ()
-  "Interpolate command result redirected to temp file"
-  (should (equal (eshell-test-command-result "cat $<echo hi>") "hi")))
-
-(ert-deftest eshell-test/interp-concat ()
-  "Interpolate and concat command"
-  (should (equal (eshell-test-command-result "+ ${+ 1 2}3 3") 36)))
-
-(ert-deftest eshell-test/interp-concat-lisp ()
-  "Interpolate and concat Lisp form"
-  (should (equal (eshell-test-command-result "+ $(+ 1 2)3 3") 36)))
-
-(ert-deftest eshell-test/interp-concat2 ()
-  "Interpolate and concat two commands"
-  (should (equal (eshell-test-command-result "+ ${+ 1 2}${+ 1 2} 3") 36)))
-
-(ert-deftest 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/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/pipe-headproc ()
   "Check that piping a non-process to a process command waits for the process"
   (skip-unless (executable-find "cat"))
@@ -152,32 +110,6 @@ eshell-test/pipe-headproc-stdin
    (eshell-wait-for-subprocess)
    (eshell-match-result "OLLEH\n")))
 
-(ert-deftest eshell-test/window-height ()
-  "$LINES should equal (window-height)"
-  (should (eshell-test-command-result "= $LINES (window-height)")))
-
-(ert-deftest eshell-test/window-width ()
-  "$COLUMNS should equal (window-width)"
-  (should (eshell-test-command-result "= $COLUMNS (window-width)")))
-
-(ert-deftest eshell-test/last-result-var ()
-  "Test using the \"last result\" ($$) variable"
-  (with-temp-eshell
-   (eshell-command-result-p "+ 1 2; + $$ 2"
-                            "3\n5\n")))
-
-(ert-deftest eshell-test/last-result-var2 ()
-  "Test using the \"last result\" ($$) variable twice"
-  (with-temp-eshell
-   (eshell-command-result-p "+ 1 2; + $$ $$"
-                             "3\n6\n")))
-
-(ert-deftest eshell-test/last-arg-var ()
-  "Test using the \"last arg\" ($_) variable"
-  (with-temp-eshell
-   (eshell-command-result-p "+ 1 2; + $_ 4"
-                             "3\n6\n")))
-
 (ert-deftest eshell-test/inside-emacs-var ()
   "Test presence of \"INSIDE_EMACS\" in subprocesses"
   (with-temp-eshell
-- 
2.25.1

From 7a64056e44a6f789fb2f58cca2c6876cfdcc107d Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Sat, 26 Feb 2022 20:55:22 -0800
Subject: [PATCH 2/5] Add a new macro to simplify parsing temporary Eshell
 command strings

This abstracts out the somewhat-unusual "insert&delete" logic in
'eshell-parse-command' so that it can be used elsewhere, and also
ensures that the deletion occurs even if an an error occurs.

* lisp/eshell/esh-cmd.el (eshell-with-temp-command): New macro.
(eshell-parse-command): Use it.
---
 lisp/eshell/esh-cmd.el | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index dceb061c8f..04b54d9d79 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -350,6 +350,36 @@ eshell-complete-lisp-symbols
 
 (defvar eshell--sep-terms)
 
+(defmacro eshell-with-temp-command (command &rest body)
+  "Narrow the buffer to COMMAND and execute the forms in BODY.
+COMMAND can either be a string, or a cons cell demarcating a
+buffer region.  If COMMAND is a string, temporarily insert it
+into the buffer before narrowing.  Point will be set to the
+beginning of the narrowed region.
+
+The value returned is the last form in BODY."
+  (declare (indent 1))
+  `(let ((cmd ,command))
+     (if (stringp cmd)
+         ;; Since parsing relies partly on buffer-local state
+         ;; (e.g. that of `eshell-parse-argument-hook'), we need to
+         ;; perform the parsing in the Eshell buffer.
+         (let ((begin (point)) end
+	       (inhibit-point-motion-hooks t))
+           (with-silent-modifications
+             (insert cmd)
+             (setq end (point))
+             (unwind-protect
+                 (save-restriction
+                   (narrow-to-region begin end)
+                   (goto-char begin)
+                   ,@body)
+               (delete-region begin end))))
+       (save-restriction
+         (narrow-to-region (car cmd) (cdr cmd))
+         (goto-char (car cmd))
+         ,@body))))
+
 (defun eshell-parse-command (command &optional args toplevel)
   "Parse the COMMAND, adding ARGS if given.
 COMMAND can either be a string, or a cons cell demarcating a buffer
@@ -361,15 +391,9 @@ eshell-parse-command
 	  (append
 	   (if (consp command)
 	       (eshell-parse-arguments (car command) (cdr command))
-	     (let ((here (point))
-		   (inhibit-point-motion-hooks t))
-               (with-silent-modifications
-                 ;; FIXME: Why not use a temporary buffer and avoid this
-                 ;; "insert&delete" business?  --Stef
-                 (insert command)
-                 (prog1
-                     (eshell-parse-arguments here (point))
-                   (delete-region here (point))))))
+             (eshell-with-temp-command command
+               (goto-char (point-max))
+               (eshell-parse-arguments (point-min) (point-max))))
 	   args))
 	 (commands
 	  (mapcar
-- 
2.25.1

From 64484ad751bfebc638afaf87b2cc8be63c550ba8 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Sun, 27 Feb 2022 21:04:30 -0800
Subject: [PATCH 3/5] Fix Eshell dollar interpolation inside of double-quotes

For example,

  echo "${echo hi}"

previously tried to run the program named 'echo hi', instead of 'echo'
with the argument 'hi'.

* lisp/eshell/esh-arg.el (eshell-parse-inner-double-quote):
New function.

* lisp/eshell/esh-var.el (eshell-parse-variable-ref): Support parsing
when wrapped in double-quiotes.

* test/lisp/eshell/esh-var-tests.el (esh-var-test/interp-var)
(esh-var-test/interp-quoted-var)
(esh-var-test/interp-quoted-var-concat)
(esh-var-test/quoted-interp-var)
(esh-var-test/quoted-interp-quoted-var)
(esh-var-test/quoted-interp-lisp, esh-var-test/quoted-interp-cmd)
(esh-var-test/quoted-interp-temp-cmd): New tests.
---
 lisp/eshell/esh-arg.el            | 24 +++++++++++++++
 lisp/eshell/esh-var.el            | 27 +++++++++++------
 test/lisp/eshell/esh-var-tests.el | 49 +++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+), 9 deletions(-)

diff --git a/lisp/eshell/esh-arg.el b/lisp/eshell/esh-arg.el
index 1a2f2a57e8..e19481c4ba 100644
--- a/lisp/eshell/esh-arg.el
+++ b/lisp/eshell/esh-arg.el
@@ -354,6 +354,30 @@ eshell-parse-double-quote
 		  (list 'eshell-escape-arg arg))))
 	  (goto-char (1+ end)))))))
 
+(defun eshell-parse-inner-double-quote (bound)
+  "Parse the inner part of a double quoted string.
+The string to parse starts at point and ends at BOUND.
+
+If Eshell is currently parsing a quoted string and there are any
+backslash-escaped characters, this will return the unescaped
+string, updating point to BOUND.  Otherwise, this returns nil and
+leaves point where it was."
+  (when eshell-current-quoted
+    (let (strings
+          (start (point))
+          (special-char
+           (rx-to-string
+            `(seq "\\" (group (any ,@eshell-special-chars-inside-quoting))))))
+      (while (re-search-forward special-char bound t)
+        (push (concat (buffer-substring start (match-beginning 0))
+                      (match-string 1))
+              strings)
+        (setq start (match-end 0)))
+      (when strings
+        (push (buffer-substring start bound) strings)
+        (goto-char bound)
+        (apply #'concat (nreverse strings))))))
+
 (defun eshell-parse-special-reference ()
   "Parse a special syntax reference, of the form `#<args>'.
 
diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index ee3ffbc647..24fdbde3cf 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -440,18 +440,16 @@ eshell-parse-variable-ref
     (let ((end (eshell-find-delimiter ?\{ ?\})))
       (if (not end)
           (throw 'eshell-incomplete ?\{)
+        (forward-char)
         (prog1
             `(eshell-convert
               (eshell-command-to-value
                (eshell-as-subcommand
-                ,(eshell-parse-command (cons (1+ (point)) end)))))
+                ,(let ((subcmd (or (eshell-parse-inner-double-quote end)
+                                   (cons (point) end)))
+                       (eshell-current-quoted nil))
+                   (eshell-parse-command subcmd)))))
           (goto-char (1+ end))))))
-   ((memq (char-after) '(?\' ?\"))
-    (let ((name (if (eq (char-after) ?\')
-                    (eshell-parse-literal-quote)
-                  (eshell-parse-double-quote))))
-      (if name
-	  `(eshell-get-variable ,(eval name) indices))))
    ((eq (char-after) ?\<)
     (let ((end (eshell-find-delimiter ?\< ?\>)))
       (if (not end)
@@ -463,7 +461,9 @@ eshell-parse-variable-ref
               `(let ((eshell-current-handles
                       (eshell-create-handles ,temp 'overwrite)))
                  (progn
-                   (eshell-as-subcommand ,(eshell-parse-command cmd))
+                   (eshell-as-subcommand
+                    ,(let ((eshell-current-quoted nil))
+                       (eshell-parse-command cmd)))
                    (ignore
                     (nconc eshell-this-command-hook
                            ;; Quote this lambda; it will be evaluated
@@ -478,9 +478,18 @@ eshell-parse-variable-ref
     (condition-case nil
         `(eshell-command-to-value
           (eshell-lisp-command
-           ',(read (current-buffer))))
+           ',(read (or (eshell-parse-inner-double-quote (point-max))
+                       (current-buffer)))))
       (end-of-file
        (throw 'eshell-incomplete ?\())))
+   ((looking-at (rx (or "'" "\"" "\\\"")))
+    (eshell-with-temp-command (or (eshell-parse-inner-double-quote (point-max))
+                                  (cons (point) (point-max)))
+      (let ((name (if (eq (char-after) ?\')
+                      (eshell-parse-literal-quote)
+                    (eshell-parse-double-quote))))
+        (when name
+	  `(eshell-get-variable ,(eval name) indices)))))
    ((assoc (char-to-string (char-after))
            eshell-variable-aliases-list)
     (forward-char)
diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el
index 8d803e5ca4..7ec6a97519 100644
--- a/test/lisp/eshell/esh-var-tests.el
+++ b/test/lisp/eshell/esh-var-tests.el
@@ -37,6 +37,25 @@
 
 ;; Variable interpolation
 
+(ert-deftest esh-var-test/interp-var ()
+  "Interpolate variable"
+  (should (equal (eshell-test-command-result "echo $user-login-name")
+                 user-login-name)))
+
+(ert-deftest esh-var-test/interp-quoted-var ()
+  "Interpolate quoted variable"
+  (should (equal (eshell-test-command-result "echo $'user-login-name'")
+                 user-login-name))
+  (should (equal (eshell-test-command-result "echo $\"user-login-name\"")
+                 user-login-name)))
+
+(ert-deftest esh-var-test/interp-quoted-var-concat ()
+  "Interpolate and concat quoted variable"
+  (should (equal (eshell-test-command-result "echo $'user-login-name'-foo")
+                 (concat user-login-name "-foo")))
+  (should (equal (eshell-test-command-result "echo $\"user-login-name\"-foo")
+                 (concat user-login-name "-foo"))))
+
 (ert-deftest esh-var-test/interp-lisp ()
   "Interpolate Lisp form evaluation"
   (should (equal (eshell-test-command-result "+ $(+ 1 2) 3") 6)))
@@ -79,6 +98,36 @@ esh-var-test/interp-concat-cmd-external
    (eshell-command-result-p "echo ${echo hi}-${*echo there}"
                             "hi-there\n")))
 
+(ert-deftest esh-var-test/quoted-interp-var ()
+  "Interpolate variable inside double-quotes"
+  (should (equal (eshell-test-command-result "echo \"$user-login-name\"")
+                 user-login-name)))
+
+(ert-deftest esh-var-test/quoted-interp-quoted-var ()
+  "Interpolate quoted variable inside double-quotes"
+  (should (equal (eshell-test-command-result
+                  "echo \"hi, $'user-login-name'\"")
+                 (concat "hi, " user-login-name)))
+  (should (equal (eshell-test-command-result
+                  "echo \"hi, $\\\"user-login-name\\\"\"")
+                 (concat "hi, " user-login-name))))
+
+(ert-deftest esh-var-test/quoted-interp-lisp ()
+  "Interpolate Lisp form evaluation inside double-quotes"
+  (should (equal (eshell-test-command-result
+                  "echo \"hi $(concat \\\"the\\\" \\\"re\\\")\"")
+                 "hi there")))
+
+(ert-deftest esh-var-test/quoted-interp-cmd ()
+  "Interpolate command result inside double-quotes"
+  (should (equal (eshell-test-command-result
+                  "echo \"hi ${echo \\\"there\\\"}\"")
+                 "hi there")))
+
+(ert-deftest esh-var-test/quoted-interp-temp-cmd ()
+  "Interpolate command result redirected to temp file inside double-quotes"
+  (should (equal (eshell-test-command-result "cat \"$<echo hi>\"") "hi")))
+
 
 ;; Built-in variables
 
-- 
2.25.1

From 3d93eba3ebe8396c0d349cad15f40e2c387f4e91 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Tue, 1 Mar 2022 18:36:08 -0800
Subject: [PATCH 4/5] Fix parsing of indices in Eshell expansions

Previously, more-complex index expansions, like '$var[":" 0]' or
'$var[$(expr) 0]' failed to parse correctly.

* lisp/eshell/esh-var.el (Commentary): Clarify indexing and length
expansions.
(eshell-parse-indices): Expand docstring and support parsing inside
double-quotes.
(eshell-eval-indices): New function.
(eshell-parse-variable): Use it.

* test/lisp/eshell/esh-var-tests.el (eshell-test-value): New defvar.
(esh-var-test/interp-var-indices,
(esh-var-test/interp-var-split-indices)
(esh-var-test/interp-var-string-split-indices)
(esh-var-test/interp-var-regexp-split-indices)
(esh-var-test/interp-var-assoc, esh-var-test/interp-var-length-list)
(esh-var-test/interp-var-length-string)
(esh-var-test/interp-var-length-alist)
(esh-var-test/quoted-interp-var-indices)
(esh-var-test/quoted-interp-var-split-indices)
(esh-var-test/quoted-interp-var-string-split-indices)
(esh-var-test/quoted-interp-var-regexp-split-indices)
(esh-var-test/quoted-interp-var-assoc)
(esh-var-test/quoted-interp-var-length-list)
(esh-var-test/quoted-interp-var-length-string)
(esh-var-test/quoted-interp-var-length-alist): New tests.

* doc/misc/eshell.texi (Dollars Expansion): Expand and reword
documentation for indexing and length expansions.
---
 doc/misc/eshell.texi              |  62 ++++++------
 lisp/eshell/esh-var.el            |  59 ++++++------
 test/lisp/eshell/esh-var-tests.el | 152 ++++++++++++++++++++++++++++++
 3 files changed, 212 insertions(+), 61 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index bbf8ca6b8b..3301a854eb 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -1022,11 +1022,6 @@ Dollars Expansion
 disambiguate the variable name when concatenating it with another
 value, such as @samp{$"@var{var}"-suffix}.
 
-@item $#@var{var}
-Expands to the length of the value bound to @var{var}.  Raises an error
-if the value is not a sequence
-(@pxref{Sequences Arrays Vectors, Sequences, , elisp, The Emacs Lisp Reference Manual}).
-
 @item $(@var{lisp})
 Expands to the result of evaluating the S-expression @code{(@var{lisp})}.  On
 its own, this is identical to just @code{(@var{lisp})}, but with the @code{$},
@@ -1041,34 +1036,35 @@ Dollars Expansion
 @command{@var{command}}, but writes the output to a temporary file and
 returns the file name.
 
-@item $@var{var}[i]
-Expands to the @code{i}th element of the value bound to @var{var}.  If
-the value is a string, it will be split at whitespace to make it a list.
-Again, raises an error if the value is not a sequence.
-
-@item $@var{var}[: i]
-As above, but now splitting occurs at the colon character.
-
-@item $@var{var}[: i j]
-As above, but instead of returning just a string, it now returns a list
-of two strings.  If the result is being interpolated into a larger
-string, this list will be flattened into one big string, with each
-element separated by a space.
-
-@item $@var{var}["\\\\" i]
-Separate on backslash characters.  Actually, the first argument -- if it
-doesn't have the form of a number, or a plain variable name -- can be
-any regular expression.  So to split on numbers, use
-@samp{$@var{var}["[0-9]+" 10 20]}.
-
-@item $@var{var}[hello]
-Calls @code{assoc} on @var{var} with @code{"hello"}, expecting it to be
-an alist (@pxref{Association List Type, Association Lists, , elisp,
-The Emacs Lisp Reference Manual}).
-
-@item $#@var{var}[hello]
-Returns the length of the @code{cdr} of the element of @var{var} whose
-car is equal to @code{"hello"}.
+@item $@var{expr}[@var{i...}]
+Expands to the @var{i}th element of the result of @var{expr}, an
+expression in one of the above forms listed here.  If multiple indices
+are supplied, this will return a list containing the elements for each
+index.  If @var{expr}'s value is a string, it will first be split at
+whitespace to make it a list.  If @var{expr}'s value is an alist
+(@pxref{Association List Type, Association Lists, , elisp, The Emacs
+Lisp Reference Manual}), this will call @code{assoc} on the result of
+@var{expr}, returning the @code{cdr} of the element of the result
+whose car is equal to @code{"i"}.  Raises an error if the value is not
+a sequence (@pxref{Sequences Arrays Vectors, Sequences, , elisp, The
+Emacs Lisp Reference Manual}).
+
+Multiple sets of indices can also be specified. For example, if
+@var{var} is a list of lists, @samp{$@var{var}[0][0]} is equivalent to
+@samp{(caar @var{var})}.
+
+@item $@var{expr}[@var{regexp} @var{i...}]
+As above (when @var{expr} expands to a string), but use @var{regexp}
+to split the string.  @var{regexp} can be any form other than a number
+or a plain variable name.  For example, @samp{$@var{var}[: 0]} will
+return the first element of a colon-delimited string.
+
+@item $#@var{expr}
+Expands to the length of the result of @var{expr}, an expression in
+one of the above forms.  For example, @samp{$#@var{var}} returns the
+length of the variable @var{var} and @samp{$#@var{var}[0]} returns the
+length of the first element of @var{var}.  Again, raises an error if
+the result of @var{expr} is not a sequence.
 
 @end table
 
diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index 24fdbde3cf..6f08a3fbc4 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -39,11 +39,6 @@
 ;;
 ;; Only "MYVAR" is part of the variable name in this case.
 ;;
-;;   $#VARIABLE
-;;
-;; Returns the length of the value of VARIABLE.  This could also be
-;; done using the `length' Lisp function.
-;;
 ;;   $(lisp)
 ;;
 ;; Returns result of Lisp evaluation.  Note: Used alone like this, it
@@ -61,38 +56,36 @@
 ;; Evaluates an eshell subcommand, redirecting the output to a
 ;; temporary file, and returning the file name.
 ;;
-;;   $ANYVAR[10]
+;;   $EXPR[10]
 ;;
-;; Return the 10th element of ANYVAR.  If ANYVAR's value is a string,
-;; it will be split in order to make it a list.  The splitting will
-;; occur at whitespace.
+;; Return the 10th element of $EXPR, which can be any dollar
+;; expression.  If $EXPR's value is a string, it will be split in
+;; order to make it a list.  The splitting will occur at whitespace.
 ;;
-;;   $ANYVAR[: 10]
+;;   $EXPR[10 20]
 ;;
-;; As above, except that splitting occurs at the colon now.
+;; As above, but instead of returning a single element, it now returns a
+;; list of two elements.
 ;;
-;;   $ANYVAR[: 10 20]
+;;   $EXPR[: 10]
 ;;
-;; As above, but instead of returning just a string, it now returns a
-;; list of two strings.  If the result is being interpolated into a
-;; larger string, this list will be flattened into one big string,
-;; with each element separated by a space.
+;; Like $EXPR[10], except that splitting occurs at the colon now.
 ;;
-;;   $ANYVAR["\\\\" 10]
+;;   $EXPR["\\\\" 10]
 ;;
 ;; Separate on backslash characters.  Actually, the first argument --
 ;; if it doesn't have the form of a number, or a plain variable name
 ;; -- can be any regular expression.  So to split on numbers, use
-;; '$ANYVAR["[0-9]+" 10 20]'.
+;; '$EXPR["[0-9]+" 10 20]'.
 ;;
-;;   $ANYVAR[hello]
+;;   $EXPR[hello]
 ;;
-;; Calls `assoc' on ANYVAR with 'hello', expecting it to be an alist.
+;; Calls `assoc' on $EXPR with 'hello', expecting it to be an alist.
 ;;
-;;   $#ANYVAR[hello]
+;;   $#EXPR
 ;;
-;; Returns the length of the cdr of the element of ANYVAR who car is
-;; equal to "hello".
+;; Returns the length of the value of $EXPR.  This could also be
+;; done using the `length' Lisp function.
 ;;
 ;; There are also a few special variables defined by Eshell.  '$$' is
 ;; the value of the last command (t or nil, in the case of an external
@@ -416,7 +409,7 @@ eshell-parse-variable
 		       (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 ',indices)) ,value))
+	  value `(let ((indices (eshell-eval-indices ',indices))) ,value))
     (if get-len
 	`(length ,value)
       value)))
@@ -504,19 +497,29 @@ eshell-parse-variable-ref
 (defvar eshell-glob-function)
 
 (defun eshell-parse-indices ()
-  "Parse and return a list of list of indices."
+  "Parse and return a list of index-lists.
+
+For example, \"[0 1][2]\" becomes:
+  ((\"0\" \"1\") (\"2\")."
   (let (indices)
     (while (eq (char-after) ?\[)
       (let ((end (eshell-find-delimiter ?\[ ?\])))
 	(if (not end)
 	    (throw 'eshell-incomplete ?\[)
 	  (forward-char)
-	  (let (eshell-glob-function)
-	    (setq indices (cons (eshell-parse-arguments (point) end)
-				indices)))
+          (eshell-with-temp-command (or (eshell-parse-inner-double-quote end)
+                                        (cons (point) end))
+	    (let (eshell-glob-function (eshell-current-quoted nil))
+	      (setq indices (cons (eshell-parse-arguments
+                                   (point-min) (point-max))
+				  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-get-variable (name &optional indices)
   "Get the value for the variable NAME."
   (let* ((alias (assoc name eshell-variable-aliases-list))
diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el
index 7ec6a97519..e679174939 100644
--- a/test/lisp/eshell/esh-var-tests.el
+++ b/test/lisp/eshell/esh-var-tests.el
@@ -32,6 +32,8 @@
                            (file-name-directory (or load-file-name
                                                     default-directory))))
 
+(defvar eshell-test-value nil)
+
 ;;; Tests:
 
 
@@ -56,6 +58,76 @@ esh-var-test/interp-quoted-var-concat
   (should (equal (eshell-test-command-result "echo $\"user-login-name\"-foo")
                  (concat user-login-name "-foo"))))
 
+(ert-deftest esh-var-test/interp-var-indices ()
+  "Interpolate list variable with indices"
+  (let ((eshell-test-value '("zero" "one" "two" "three" "four")))
+    (should (equal (eshell-test-command-result "echo $eshell-test-value[0]")
+                   "zero"))
+    (should (equal (eshell-test-command-result "echo $eshell-test-value[0 2]")
+                   '("zero" "two")))
+    (should (equal (eshell-test-command-result "echo $eshell-test-value[0 2 4]")
+                   '("zero" "two" "four")))))
+
+(ert-deftest esh-var-test/interp-var-split-indices ()
+  "Interpolate string variable with indices"
+  (let ((eshell-test-value "zero one two three four"))
+    (should (equal (eshell-test-command-result "echo $eshell-test-value[0]")
+                   "zero"))
+    (should (equal (eshell-test-command-result "echo $eshell-test-value[0 2]")
+                   '("zero" "two")))
+    (should (equal (eshell-test-command-result "echo $eshell-test-value[0 2 4]")
+                   '("zero" "two" "four")))))
+
+(ert-deftest esh-var-test/interp-var-string-split-indices ()
+  "Interpolate string variable with string splitter and indices"
+  (let ((eshell-test-value "zero:one:two:three:four"))
+    (should (equal (eshell-test-command-result "echo $eshell-test-value[: 0]")
+                   "zero"))
+    (should (equal (eshell-test-command-result "echo $eshell-test-value[: 0 2]")
+                   '("zero" "two")))))
+
+(ert-deftest esh-var-test/interp-var-regexp-split-indices ()
+  "Interpolate string variable with regexp splitter and indices"
+  (let ((eshell-test-value "zero:one!two:three!four"))
+    (should (equal (eshell-test-command-result
+                    "echo $eshell-test-value['[:!]' 0]")
+                   "zero"))
+    (should (equal (eshell-test-command-result
+                    "echo $eshell-test-value['[:!]' 0 2]")
+                   '("zero" "two")))
+    (should (equal (eshell-test-command-result
+                    "echo $eshell-test-value[\"[:!]\" 0]")
+                   "zero"))
+    (should (equal (eshell-test-command-result
+                    "echo $eshell-test-value[\"[:!]\" 0 2]")
+                   '("zero" "two")))))
+
+(ert-deftest esh-var-test/interp-var-assoc ()
+  "Interpolate alist variable with index"
+  (let ((eshell-test-value '(("foo" . 1))))
+    (should (eq (eshell-test-command-result "echo $eshell-test-value[foo]")
+                1))))
+
+(ert-deftest esh-var-test/interp-var-length-list ()
+  "Interpolate length of list variable"
+  (let ((eshell-test-value '((1 2) (3) (5 (6 7 8 9)))))
+    (should (eq (eshell-test-command-result "echo $#eshell-test-value") 3))
+    (should (eq (eshell-test-command-result "echo $#eshell-test-value[1]") 1))
+    (should (eq (eshell-test-command-result "echo $#eshell-test-value[2][1]")
+                4))))
+
+(ert-deftest esh-var-test/interp-var-length-string ()
+  "Interpolate length of string variable"
+  (let ((eshell-test-value "foobar"))
+    (should (eq (eshell-test-command-result "echo $#eshell-test-value") 6))))
+
+(ert-deftest esh-var-test/interp-var-length-alist ()
+  "Interpolate length of alist variable"
+  (let ((eshell-test-value '(("foo" . (1 2 3)))))
+    (should (eq (eshell-test-command-result "echo $#eshell-test-value") 1))
+    (should (eq (eshell-test-command-result "echo $#eshell-test-value[foo]")
+                3))))
+
 (ert-deftest esh-var-test/interp-lisp ()
   "Interpolate Lisp form evaluation"
   (should (equal (eshell-test-command-result "+ $(+ 1 2) 3") 6)))
@@ -112,6 +184,86 @@ esh-var-test/quoted-interp-quoted-var
                   "echo \"hi, $\\\"user-login-name\\\"\"")
                  (concat "hi, " user-login-name))))
 
+(ert-deftest esh-var-test/quoted-interp-var-indices ()
+  "Interpolate string variable with indices inside double-quotes"
+  (let ((eshell-test-value '("zero" "one" "two" "three" "four")))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value[0]\"")
+                   "zero"))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value[0 2]\"")
+                   '("zero" "two")))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value[0 2 4]\"")
+                   '("zero" "two" "four")))))
+
+(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"))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value[0]\"")
+                   "zero"))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value[0 2]\"")
+                   '("zero" "two")))))
+
+(ert-deftest esh-var-test/quoted-interp-var-string-split-indices ()
+  "Interpolate string variable with string splitter and indices
+inside double-quotes"
+  (let ((eshell-test-value "zero:one:two:three:four"))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value[: 0]\"")
+                   "zero"))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value[: 0 2]\"")
+                   '("zero" "two")))))
+
+(ert-deftest esh-var-test/quoted-interp-var-regexp-split-indices ()
+  "Interpolate string variable with regexp splitter and indices"
+  (let ((eshell-test-value "zero:one!two:three!four"))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value['[:!]' 0]\"")
+                   "zero"))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value['[:!]' 0 2]\"")
+                   '("zero" "two")))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value[\\\"[:!]\\\" 0]\"")
+                   "zero"))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value[\\\"[:!]\\\" 0 2]\"")
+                   '("zero" "two")))))
+
+(ert-deftest esh-var-test/quoted-interp-var-assoc ()
+  "Interpolate alist variable with index inside double-quotes"
+  (let ((eshell-test-value '(("foo" . 1))))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value[foo]\"")
+                   1))))
+
+(ert-deftest esh-var-test/quoted-interp-var-length-list ()
+  "Interpolate length of list variable inside double-quotes"
+  (let ((eshell-test-value '((1 2) (3) (5 (6 7 8 9)))))
+    (should (eq (eshell-test-command-result "echo \"$#eshell-test-value\"") 3))
+    (should (eq (eshell-test-command-result "echo \"$#eshell-test-value[1]\"")
+                1))
+    (should (eq (eshell-test-command-result
+                 "echo \"$#eshell-test-value[2][1]\"")
+                4))))
+
+(ert-deftest esh-var-test/quoted-interp-var-length-string ()
+  "Interpolate length of string variable inside double-quotes"
+  (let ((eshell-test-value "foobar"))
+    (should (eq (eshell-test-command-result "echo \"$#eshell-test-value\"")
+                6))))
+
+(ert-deftest esh-var-test/quoted-interp-var-length-alist ()
+  "Interpolate length of alist variable inside double-quotes"
+  (let ((eshell-test-value '(("foo" . (1 2 3)))))
+    (should (eq (eshell-test-command-result "echo \"$#eshell-test-value\"") 1))
+    (should (eq (eshell-test-command-result "echo \"$#eshell-test-value[foo]\"")
+                3))))
+
 (ert-deftest esh-var-test/quoted-interp-lisp ()
   "Interpolate Lisp form evaluation inside double-quotes"
   (should (equal (eshell-test-command-result
-- 
2.25.1

From 2d73ca67b844df6e1ed0319f5d454646d685caaa Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Tue, 1 Mar 2022 18:53:42 -0800
Subject: [PATCH 5/5] Allow splitting strings in Eshell expansions with "plain"
 strings

Since '$var[hello 0]' doesn't make sense when 'var' is a string, the
previous restriction was unnecessary.

* lisp/eshell/esh-var.el (Commentary): Update documentation.
(eshell-apply-indices): Allow "plain" strings to split strings.

* test/lisp/eshell/esh-var-test.el
(esh-var-test/interp-var-string-split-indices)
(esh-var-test/quoted-interp-var-string-split-indices): Update tests.

* doc/misc/eshell.texi (Dollars expansion): Update documentation.
---
 doc/misc/eshell.texi              |  6 +++---
 lisp/eshell/esh-var.el            | 17 +++++++----------
 test/lisp/eshell/esh-var-tests.el | 12 ++++++++++++
 3 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index 3301a854eb..5581e5cd9e 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -1055,9 +1055,9 @@ Dollars Expansion
 
 @item $@var{expr}[@var{regexp} @var{i...}]
 As above (when @var{expr} expands to a string), but use @var{regexp}
-to split the string.  @var{regexp} can be any form other than a number
-or a plain variable name.  For example, @samp{$@var{var}[: 0]} will
-return the first element of a colon-delimited string.
+to split the string.  @var{regexp} can be any form other than a
+number.  For example, @samp{$@var{var}[: 0]} will return the first
+element of a colon-delimited string.
 
 @item $#@var{expr}
 Expands to the length of the result of @var{expr}, an expression in
diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index 6f08a3fbc4..af89e35f55 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -74,9 +74,8 @@
 ;;   $EXPR["\\\\" 10]
 ;;
 ;; Separate on backslash characters.  Actually, the first argument --
-;; if it doesn't have the form of a number, or a plain variable name
-;; -- can be any regular expression.  So to split on numbers, use
-;; '$EXPR["[0-9]+" 10 20]'.
+;; if it doesn't have the form of a number -- can be any regular
+;; expression.  So to split on numbers, use '$EXPR["[0-9]+" 10 20]'.
 ;;
 ;;   $EXPR[hello]
 ;;
@@ -566,13 +565,11 @@ eshell-apply-indices
   (while indices
     (let ((refs (car indices)))
       (when (stringp value)
-	(let (separator)
-	  (if (not (or (not (stringp (caar indices)))
-		       (string-match
-			(concat "^" eshell-variable-name-regexp "$")
-			(caar indices))))
-	      (setq separator (caar indices)
-		    refs (cdr refs)))
+	(let (separator (index (caar indices)))
+          (when (and (stringp index)
+                     (not (get-text-property 0 'number index)))
+            (setq separator index
+                  refs (cdr refs)))
 	  (setq value
 		(mapcar #'eshell-convert
 			(split-string value separator)))))
diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el
index e679174939..d09dd614de 100644
--- a/test/lisp/eshell/esh-var-tests.el
+++ b/test/lisp/eshell/esh-var-tests.el
@@ -84,6 +84,11 @@ esh-var-test/interp-var-string-split-indices
     (should (equal (eshell-test-command-result "echo $eshell-test-value[: 0]")
                    "zero"))
     (should (equal (eshell-test-command-result "echo $eshell-test-value[: 0 2]")
+                   '("zero" "two"))))
+  (let ((eshell-test-value "zeroXoneXtwoXthreeXfour"))
+    (should (equal (eshell-test-command-result "echo $eshell-test-value[X 0]")
+                   "zero"))
+    (should (equal (eshell-test-command-result "echo $eshell-test-value[X 0 2]")
                    '("zero" "two")))))
 
 (ert-deftest esh-var-test/interp-var-regexp-split-indices ()
@@ -216,6 +221,13 @@ esh-var-test/quoted-interp-var-string-split-indices
                    "zero"))
     (should (equal (eshell-test-command-result
                     "echo \"$eshell-test-value[: 0 2]\"")
+                   '("zero" "two"))))
+  (let ((eshell-test-value "zeroXoneXtwoXthreeXfour"))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value[X 0]\"")
+                   "zero"))
+    (should (equal (eshell-test-command-result
+                    "echo \"$eshell-test-value[X 0 2]\"")
                    '("zero" "two")))))
 
 (ert-deftest esh-var-test/quoted-interp-var-regexp-split-indices ()
-- 
2.25.1