GNU bug report logs - #60942
30.0.50; [PATCH] Indices in Eshell variable interpolation don't work with async subcommands

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 60942 in the body.
You can then email your comments to 60942 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [PATCH] Indices in Eshell variable interpolation don't work
 with async subcommands
Date: Wed, 18 Jan 2023 19:36:41 -0800

[Message part 1 (text/plain, inline)]

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.

[0001-Fix-evaluation-of-asynchronous-expansions-in-Eshell-.patch (text/plain, attachment)]

Message #8 received at 60942 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 60942 <at> debbugs.gnu.org
Subject: Re: bug#60942: 30.0.50;
 [PATCH] Indices in Eshell variable interpolation don't work with
 async subcommands
Date: Thu, 19 Jan 2023 08:49:34 +0200

> Date: Wed, 18 Jan 2023 19:36:41 -0800
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> 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.

Why do you remove a non-internal function?  We cannot possibly do that
if this is going to be installed on the emacs-29 branch.  But even if
you are going to install on master, why not leave that function alone?
Some code somewhere could be using it, and we don't usually remove
functions before a period of deprecation.

Message #11 received at 60942 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 60942 <at> debbugs.gnu.org
Subject: Re: bug#60942: 30.0.50; [PATCH] Indices in Eshell variable
 interpolation don't work with async subcommands
Date: Wed, 18 Jan 2023 23:37:12 -0800

On 1/18/2023 10:49 PM, Eli Zaretskii wrote:
> Why do you remove a non-internal function?  We cannot possibly do that
> if this is going to be installed on the emacs-29 branch.  But even if
> you are going to install on master, why not leave that function alone?
> Some code somewhere could be using it, and we don't usually remove
> functions before a period of deprecation.

I can keep 'eshell-eval-indices' around and mark it obsolete if you 
prefer; it wouldn't hurt anything. I could also fix this bug in that 
function, though it would be an inferior fix compared to the new 
'eshell-indices' function, so we definitely want that new function (or 
something very similar). I'm not sure fixing 'eshell-eval-indices' is 
worth it though, since I'd be very surprised if anyone called that 
function directly.

For context, 'eshell-eval-indices' is a function I added in Emacs 29 to 
fix some related issues with indices[1], but at the time I didn't fully 
understand Eshell's internals and so implemented the fix incorrectly 
(though it works for most common cases). It probably could have been 
marked as internal at the time, but Eshell doesn't seem to do that 
regularly, even for functions that external code would be very unlikely 
to find useful.


[1] In particular, the second case in my original message fails even 
more severely in Emacs 28:

  ~/config $ echo $exec-path[${echo 0}]
  Wrong type argument: number-or-marker-p, (eshell-escape-arg (let 
((indices 'nil)) (eshell-convert (eshell-command-to-value 
(eshell-as-subcommand (eshell-trap-errors (eshell-named-command "echo" 
(list #("0" 0 1 (number t))))))))))

Message #14 received at 60942 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 60942 <at> debbugs.gnu.org
Subject: Re: bug#60942: 30.0.50; [PATCH] Indices in Eshell variable
 interpolation don't work with async subcommands
Date: Thu, 19 Jan 2023 11:31:24 -0800

[Message part 1 (text/plain, inline)]

On 1/18/2023 11:37 PM, Jim Porter wrote:
> On 1/18/2023 10:49 PM, Eli Zaretskii wrote:
>> Why do you remove a non-internal function?  We cannot possibly do that
>> if this is going to be installed on the emacs-29 branch.  But even if
>> you are going to install on master, why not leave that function alone?
>> Some code somewhere could be using it, and we don't usually remove
>> functions before a period of deprecation.
> 
> I can keep 'eshell-eval-indices' around and mark it obsolete if you 
> prefer; it wouldn't hurt anything.

Here's a patch that does this. It doesn't try to fix 
'eshell-eval-indices', since people shouldn't use that anyway. (I also 
renamed the new 'eshell-indices' to 'eshell-prepare-indices' to be clearer.)

[0001-Fix-evaluation-of-asynchronous-expansions-in-Eshell-.patch (text/plain, attachment)]

Message #17 received at 60942 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 60942 <at> debbugs.gnu.org
Subject: Re: bug#60942: 30.0.50; [PATCH] Indices in Eshell variable
 interpolation don't work with async subcommands
Date: Thu, 19 Jan 2023 21:41:34 +0200

> Date: Thu, 19 Jan 2023 11:31:24 -0800
> From: Jim Porter <jporterbugs <at> gmail.com>
> Cc: 60942 <at> debbugs.gnu.org
> 
> On 1/18/2023 11:37 PM, Jim Porter wrote:
> > On 1/18/2023 10:49 PM, Eli Zaretskii wrote:
> >> Why do you remove a non-internal function?  We cannot possibly do that
> >> if this is going to be installed on the emacs-29 branch.  But even if
> >> you are going to install on master, why not leave that function alone?
> >> Some code somewhere could be using it, and we don't usually remove
> >> functions before a period of deprecation.
> > 
> > I can keep 'eshell-eval-indices' around and mark it obsolete if you 
> > prefer; it wouldn't hurt anything.
> 
> Here's a patch that does this. It doesn't try to fix 
> 'eshell-eval-indices', since people shouldn't use that anyway. (I also 
> renamed the new 'eshell-indices' to 'eshell-prepare-indices' to be clearer.)

Is this for master?  If so, okay.  Otherwise, you'll need to adjust
the version in the obsolescence declaration.

Thanks.

Message #20 received at 60942 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 60942 <at> debbugs.gnu.org
Subject: Re: bug#60942: 30.0.50; [PATCH] Indices in Eshell variable
 interpolation don't work with async subcommands
Date: Thu, 19 Jan 2023 12:20:48 -0800

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.

(If this were for the release branch, we could probably remove 
'eshell-eval-indices', since it was introduced in Emacs 29 anyway.)

Message #25 received at 60942-done <at> debbugs.gnu.org (full text, mbox):

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

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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.