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

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 30725 in the body.
You can then email your comments to 30725 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: Yegor Timoshenko <yegortimoshenko <at> riseup.net>
To: bug-gnu-emacs <at> gnu.org
Subject: eshell: built-ins do not handle command substitution
Date: Tue, 6 Mar 2018 04:34:33 +0000

In M-x eshell:

  $ which echo
  eshell/echo is a compiled Lisp function in `em-basic.el'.
  $ which *echo
  /run/current-system/sw/bin/echo
  $ echo ${mktemp -d}
  $ *echo ${mktemp -d}
  /tmp/tmp.UaiWQ0YPIX

GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.26)
 of 2018-02-25

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Yegor Timoshenko <yegortimoshenko <at> riseup.net>, bug-gnu-emacs <at> gnu.org
Subject: Re: eshell: built-ins do not handle command substitution
Date: Mon, 17 Jan 2022 11:41:47 -0800

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

On 3/5/2018 8:34 PM, Yegor Timoshenko wrote:
> In M-x eshell:
> 
>    $ which echo
>    eshell/echo is a compiled Lisp function in `em-basic.el'.
>    $ which *echo
>    /run/current-system/sw/bin/echo
>    $ echo ${mktemp -d}
>    $ *echo ${mktemp -d}
>    /tmp/tmp.UaiWQ0YPIX

I can see this bug with an even simpler case too: "echo ${*echo hi}".

It turns out that this is because `eshell-invoke-directly' thought that 
the above command was simple enough to, well, invoke directly. However, 
since "${mktemp -d}" or "${*echo hi}" create a subprocess, the command 
needs to be invoked *iteratively* by `eshell-eval-command'. The problem 
was that `eshell-invoke-directly' only checked the top-level command and 
didn't examine subcommands.

Attached is a patch that fixes this, plus a unit test (I've verified 
that the test fails without the patch and passes with it). Note that the 
test *does* rely on the system having an external "echo" command, but I 
think some of the tests in that file already rely on the presence of an 
external "sleep" command, so this should be ok. However, if it causes 
issues on some systems (MS Windows maybe?), just let me know and I can 
try to put a guard around the test so it doesn't run on such systems.

[0001-Consider-subcommands-when-deciding-to-invoke-Eshell-.patch (text/plain, attachment)]

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

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 30725 <at> debbugs.gnu.org, yegortimoshenko <at> riseup.net
Subject: Re: bug#30725: eshell: built-ins do not handle command substitution
Date: Tue, 18 Jan 2022 09:33:40 +0100

Jim Porter <jporterbugs <at> gmail.com> writes:

Hi Jim,

> Attached is a patch that fixes this, plus a unit test (I've verified
> that the test fails without the patch and passes with it). Note that
> the test *does* rely on the system having an external "echo" command,
> but I think some of the tests in that file already rely on the
> presence of an external "sleep" command, so this should be
> ok. However, if it causes issues on some systems (MS Windows maybe?),
> just let me know and I can try to put a guard around the test so it
> doesn't run on such systems.

(skip-unless (executable-find "echo"))

Best regards, Michael.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 30725 <at> debbugs.gnu.org, yegortimoshenko <at> riseup.net
Subject: Re: bug#30725: eshell: built-ins do not handle command substitution
Date: Tue, 18 Jan 2022 10:06:45 -0800

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

On 1/18/2022 12:33 AM, Michael Albinus wrote:
> (skip-unless (executable-find "echo"))

Oh, right. I'd forgotten about `skip-unless'. Here's a patch with that 
added. Thanks.

[0001-Consider-subcommands-when-deciding-to-invoke-Eshell-.patch (text/plain, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 30725 <at> debbugs.gnu.org, Michael Albinus <michael.albinus <at> gmx.de>,
 yegortimoshenko <at> riseup.net
Subject: Re: bug#30725: eshell: built-ins do not handle command substitution
Date: Thu, 20 Jan 2022 14:38:11 +0100

Jim Porter <jporterbugs <at> 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.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 30725 <at> debbugs.gnu.org, Michael Albinus <michael.albinus <at> gmx.de>,
 yegortimoshenko <at> riseup.net
Subject: Re: bug#30725: eshell: built-ins do not handle command substitution
Date: Thu, 20 Jan 2022 19:10:31 -0800

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

On 1/20/2022 5:38 AM, Lars Ingebrigtsen wrote:
> Jim Porter <jporterbugs <at> 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. :)

[0001-Further-improve-determination-of-when-commands-can-b.patch (text/plain, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 30725 <at> debbugs.gnu.org, Michael Albinus <michael.albinus <at> gmx.de>,
 yegortimoshenko <at> riseup.net
Subject: Re: bug#30725: eshell: built-ins do not handle command substitution
Date: Fri, 21 Jan 2022 10:32:27 +0100

Jim Porter <jporterbugs <at> gmail.com> writes:

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

Looks fine to me.  Pushed to Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

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