bug#54227: 29.0.50; [PATCH] Inconsistencies with Eshell variable interpolation

[Jim Porter <jporterbugs@gmail.com>]

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

On 3/3/2022 9:03 AM, Eli Zaretskii wrote:
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Wed, 2 Mar 2022 22:35:22 -0800
>>
>> +(defmacro eshell-with-temp-command (command &rest body)
>> +  "Narrow the buffer to COMMAND and execute the forms in BODY.
> 
> What does it mean to "narrow the buffer to COMMAND"?
> 
> Imagine that the user only sees this one line of the doc string --
> that actually happens in apropos commands.  How can such a user
> understand what this macro does?

The macro's job is to take an Eshell command (or some fragment thereof) 
and narrow the buffer so that it's just looking at that part. This is to 
make sure that whatever is called in the body knows where to start and 
stop looking.

I agree that this isn't very clear, but I had trouble coming up with a 
concise explanation. It's essentially a workaround for how Eshell 
expects things; a lot of the Eshell command parsing functions operate on 
a range of text in the buffer. Normally, if you wanted to use those 
functions with a temporary string, you'd use `with-temp-buffer' and 
insert the string there. That doesn't work here though, since Eshell 
uses lots of buffer-local state. This function tries to abstract that 
out in a way that's useful for a few different places in Eshell.

If you have any ideas about how to improve the wording, I'm happy to 
update it though. I'll try to keep thinking as well.

> 
>> +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.
> 
> After reading this several time and looking at the implementation, I'm
> beginning to think that COMMAND is not a good name for this argument.

Perhaps not. That comes from `eshell-parse-command' below, which takes a 
COMMAND argument of the same possible forms. There's probably a better 
name to use...

>> +(defun eshell-parse-inner-double-quote (bound)
[snip]
> 
> This seems to just unescape characters in the string?  If so, "parse"
> is not the best name for it, and the first line of the doc string
> should say "unescape", not "parse".

Fixed.

I also reworded the manual entries. Hopefully they're a bit better.

Finally, I made a very small tweak to how quoted variable expansions 
(like $"foo") are detected. The old code wasn't reporting the right 
error if you typed:

   echo $\"foo\"

That's not correct and it should be considered invalid syntax (which it 
is now).

[-- Attachment #2: 0001-Improve-wording-of-Eshell-variable-interpolation-cod.patch --]
[-- Type: text/plain, Size: 5254 bytes --]

From cbf43bb3d9fbb05f6a67d780cb8053ff5e0d700b Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 3 Mar 2022 09:37:25 -0800
Subject: [PATCH] Improve wording of Eshell variable interpolation
 code/documentation

* lisp/eshell/esh-arg.el (eshell-unescape-inner-double-quote): Rename
from 'eshell-parse-inner-double-quote'.

* lisp/eshell/esh-var.el (eshell-parse-variable-ref): Use
'eshell-unescape-inner-double-quote' and improve robustness of quoted
variable name matching.
(eshell-parse-indices): Use 'eshell-unescape-inner-double-quote'.

* doc/misc/eshell.texi (Dollars Expansion): Improve wording of
subscript notation.
---
 doc/misc/eshell.texi   | 19 ++++++++++---------
 lisp/eshell/esh-arg.el |  4 ++--
 lisp/eshell/esh-var.el | 14 ++++++++------
 3 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index 5581e5cd9e..47f8902d5a 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -1043,15 +1043,16 @@ Dollars Expansion
 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}).
+Lisp Reference Manual}), this will return the value associated with
+the key @code{"i"}.
 
-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})}.
+Multiple sets of indices can also be specified.  For example, if
+@var{var} is @samp{((1 2) (3 4))}, then @samp{$@var{var}[0][1]} will
+expand to @code{2}.
+
+Raises an error if the result of @var{expr} is not a string or a
+sequence (@pxref{Sequences Arrays Vectors, Sequences, , elisp, The
+Emacs Lisp Reference Manual}.)
 
 @item $@var{expr}[@var{regexp} @var{i...}]
 As above (when @var{expr} expands to a string), but use @var{regexp}
@@ -1064,7 +1065,7 @@ Dollars Expansion
 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.
+the result of @var{expr} is not a string or a sequence.
 
 @end table
 
diff --git a/lisp/eshell/esh-arg.el b/lisp/eshell/esh-arg.el
index e19481c4ba..a3c961f546 100644
--- a/lisp/eshell/esh-arg.el
+++ b/lisp/eshell/esh-arg.el
@@ -354,8 +354,8 @@ 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.
+(defun eshell-unescape-inner-double-quote (bound)
+  "Unescape 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
diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index af89e35f55..8746f2bb93 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -437,7 +437,7 @@ eshell-parse-variable-ref
             `(eshell-convert
               (eshell-command-to-value
                (eshell-as-subcommand
-                ,(let ((subcmd (or (eshell-parse-inner-double-quote end)
+                ,(let ((subcmd (or (eshell-unescape-inner-double-quote end)
                                    (cons (point) end)))
                        (eshell-current-quoted nil))
                    (eshell-parse-command subcmd)))))
@@ -470,13 +470,15 @@ eshell-parse-variable-ref
     (condition-case nil
         `(eshell-command-to-value
           (eshell-lisp-command
-           ',(read (or (eshell-parse-inner-double-quote (point-max))
+           ',(read (or (eshell-unescape-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)))
+   ((looking-at (rx-to-string
+                 `(or "'" ,(if eshell-current-quoted "\\\"" "\""))))
+    (eshell-with-temp-command
+        (or (eshell-unescape-inner-double-quote (point-max))
+            (cons (point) (point-max)))
       (let ((name (if (eq (char-after) ?\')
                       (eshell-parse-literal-quote)
                     (eshell-parse-double-quote))))
@@ -506,7 +508,7 @@ eshell-parse-indices
 	(if (not end)
 	    (throw 'eshell-incomplete ?\[)
 	  (forward-char)
-          (eshell-with-temp-command (or (eshell-parse-inner-double-quote end)
+          (eshell-with-temp-command (or (eshell-unescape-inner-double-quote end)
                                         (cons (point) end))
 	    (let (eshell-glob-function (eshell-current-quoted nil))
 	      (setq indices (cons (eshell-parse-arguments
-- 
2.25.1