bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Jim Porter]

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

 From "emacs -Q --eval '(eshell)':

   ~ $ (eq 'foo nil)
   Expecting completion of delimiter ' ...

   M-: (unload-feature 'em-extpipe)

   ~ $ (eq 'foo nil)
   ;; No output, since it returns nil. This is correct.

(As an ironic twist, you'd normally be able to run "(unload-feature 
'em-extpipe)" as a command in Eshell, but this bug prevents that from 
working.)

I don't fully understand all the details of the code in 
lisp/eshell/em-extpipe.el yet, but I think this is the culprit:

                          (while (or (eshell-parse-lisp-argument)
                                     (eshell-parse-backslash)
                                     (eshell-parse-double-quote)
                                     (eshell-parse-literal-quote)))

In particular, since there are 3 single-quotes, 
`eshell-parse-literal-quote' throws an `eshell-incomplete' error, which 
gums up the works.

The attached patch resolves the issue for me, but I'm not sure if it's 
the best strategy. If possible, I think it would be better for 
`eshell-parse-external-pipeline' to solely focus on finding the external 
pipe operators ("*|", "*<", and "*>")[1] and then for 
`eshell-rewrite-external-pipeline' to prepare the command string to pass 
to sh. This would also have the advantage[2] of making it possible to 
support a richer set of Eshell features with external pipes, such as the 
following:

   ~ $ echo $(message "[%s]" "hi") *| cat
   zsh:1: command not found: message

(If you remove the "*", this outputs "[hi]", and it should be 
technically possible to make this work with external pipes too, provided 
it executes the Lisp code before generating the command string for sh.)

[1] See `eshell-parse-delimiter' for an example of how that might be 
implemented.
[2] Well, maybe there's room for debate on what the right behavior here is.

[-- Attachment #2: 0001-Make-Eshell-s-extpipe-more-lenient-when-looking-for-.patch --]
[-- Type: text/plain, Size: 2576 bytes --]

From fab48a28ff80f1c74f2d50d4f69edb056f88d119 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 27 Mar 2022 19:04:42 -0700
Subject: [PATCH] Make Eshell's extpipe more lenient when looking for its
 operators

This could cause some errors when executing Eshell commands with an
odd number of quotation marks, such as "(eq 'foo nil)".

* lisp/eshell/em-extpipe.el (eshell-parse-external-pipeline): Catch
'eshell-incomplete' errors.

* test/lisp/eshell/eshell-tests.el
(eshell-test/lisp-command-with-quote): New test.
---
 lisp/eshell/em-extpipe.el        | 9 +++++----
 test/lisp/eshell/eshell-tests.el | 4 ++++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/lisp/eshell/em-extpipe.el b/lisp/eshell/em-extpipe.el
index eb5b3bfe1d..e7b421982a 100644
--- a/lisp/eshell/em-extpipe.el
+++ b/lisp/eshell/em-extpipe.el
@@ -105,10 +105,11 @@ eshell-parse-external-pipeline
                        (if (re-search-forward pat next t)
                            (throw 'found (match-beginning 1))
                          (goto-char next)
-                         (while (or (eshell-parse-lisp-argument)
-                                    (eshell-parse-backslash)
-                                    (eshell-parse-double-quote)
-                                    (eshell-parse-literal-quote)))
+                         (catch 'eshell-incomplete
+                           (while (or (eshell-parse-lisp-argument)
+                                      (eshell-parse-backslash)
+                                      (eshell-parse-double-quote)
+                                      (eshell-parse-literal-quote))))
                          ;; Guard against an infinite loop if none of
                          ;; the parsers moved us forward.
                          (unless (or (> (point) next) (eobp))
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index e31db07c61..1e303f70e5 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -44,6 +44,10 @@ eshell-test/lisp-command
   "Test `eshell-command-result' with an elisp command."
   (should (equal (eshell-test-command-result "(+ 1 2)") 3)))
 
+(ert-deftest eshell-test/lisp-command-with-quote ()
+  "Test `eshell-command-result' with an elisp command containing a quote."
+  (should (equal (eshell-test-command-result "(eq 'foo nil)") nil)))
+
 (ert-deftest eshell-test/for-loop ()
   "Test `eshell-command-result' with a for loop.."
   (let ((process-environment (cons "foo" process-environment)))
-- 
2.25.1

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

> The attached patch resolves the issue for me, but I'm not sure if it's
> the best strategy. If possible, I think it would be better for
> `eshell-parse-external-pipeline' to solely focus on finding the
> external pipe operators ("*|", "*<", and "*>")[1] and then for
> `eshell-rewrite-external-pipeline' to prepare the command string to
> pass to sh. This would also have the advantage[2] of making it
> possible to support a richer set of Eshell features with external
> pipes, such as the following:

I think that sounds like a good idea (but I don't use eshell regularly,
so I don't really have much of an opinion here).  Perhaps John does;
added to the CCs.

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

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Jim Porter]

On 3/31/2022 4:46 AM, Lars Ingebrigtsen wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
> 
>> The attached patch resolves the issue for me, but I'm not sure if it's
>> the best strategy. If possible, I think it would be better for
>> `eshell-parse-external-pipeline' to solely focus on finding the
>> external pipe operators ("*|", "*<", and "*>")[1] and then for
>> `eshell-rewrite-external-pipeline' to prepare the command string to
>> pass to sh. This would also have the advantage[2] of making it
>> possible to support a richer set of Eshell features with external
>> pipes, such as the following:
> 
> I think that sounds like a good idea (but I don't use eshell regularly,
> so I don't really have much of an opinion here).  Perhaps John does;
> added to the CCs.

CCing Sean as well, who implemented the extpipe module.

(I probably should have done that in the initial report, but I'm not 
sure the right way to CC people when I'm mailing bug-gnu-emacs@; if the 
CCed person replies normally, wouldn't it make a new bug number? I'm 
sure this is in the debbugs documentation though, so I should sit down 
and read it sometime.)

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Eli Zaretskii]

> From: Jim Porter <jporterbugs@gmail.com>
> Date: Thu, 31 Mar 2022 09:26:28 -0700
> Cc: 54603@debbugs.gnu.org, John Wiegley <johnw@gnu.org>,
>  Sean Whitton <spwhitton@spwhitton.name>
> 
> (I probably should have done that in the initial report, but I'm not 
> sure the right way to CC people when I'm mailing bug-gnu-emacs@; if the 
> CCed person replies normally, wouldn't it make a new bug number?

No, not as long as they respond with the same Subject.

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Michael Albinus]

Jim Porter <jporterbugs@gmail.com> writes:

Hi Jim,

> (I probably should have done that in the initial report, but I'm not
> sure the right way to CC people when I'm mailing bug-gnu-emacs@; if
> the CCed person replies normally, wouldn't it make a new bug number?
> I'm sure this is in the debbugs documentation though, so I should sit
> down and read it sometime.)

In the initial report, use an "X-Debbugs-Cc" header. See
admin/notes/bugtracker.

Best regards, Michael.

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Sean Whitton]

Hello,

On Thu 31 Mar 2022 at 01:46pm +02, Lars Ingebrigtsen wrote:

> Jim Porter <jporterbugs@gmail.com> writes:
>
>> The attached patch resolves the issue for me, but I'm not sure if it's
>> the best strategy. If possible, I think it would be better for
>> `eshell-parse-external-pipeline' to solely focus on finding the
>> external pipe operators ("*|", "*<", and "*>")[1] and then for
>> `eshell-rewrite-external-pipeline' to prepare the command string to
>> pass to sh. This would also have the advantage[2] of making it
>> possible to support a richer set of Eshell features with external
>> pipes, such as the following:
>
> I think that sounds like a good idea (but I don't use eshell regularly,
> so I don't really have much of an opinion here).  Perhaps John does;
> added to the CCs.

As discussed down thread, this would kind of be a different feature --
one purpose of the extpipe syntax is to make it easy to have the
operating system shell interpret complex shell syntax rather than
Eshell.

Jim has an idea about making ordinary Eshell pipes automatically use the
external shell where possible, which would satisfy this usecase better
if it works out, I think.

-- 
Sean Whitton

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Sean Whitton]

Hello Jim,

On Sun 27 Mar 2022 at 07:21PM -07, Jim Porter wrote:

> The attached patch resolves the issue for me, but I'm not sure if it's
> the best strategy.

Thank you for looking into this.  I think however that the bug is not in
the em-extpipe.el, and so your patch is probably not the best fix.

The call to `eshell-parse-lisp-argument' is meant to handle precisely
your case.  It isn't doing atm, and I think it might actually be a bug
over in that function.  To see this, with point on the first nonblank
char of each of these lines, do 'M-: (eshell-parse-lisp-argument) RET':

    #'foo

    'foo

In the first case it successfully parses it and skips point forward, but
in the latter case it does not.  But I think it should, right?

> If possible, I think it would be better for
> `eshell-parse-external-pipeline' to solely focus on finding the
> external pipe operators ("*|", "*<", and "*>")[1] and then for
> `eshell-rewrite-external-pipeline' to prepare the command string to
> pass to sh. This would also have the advantage[2] of making it
> possible to support a richer set of Eshell features with external
> pipes, such as the following:
>
>    ~ $ echo $(message "[%s]" "hi") *| cat
>    zsh:1: command not found: message
>
> (If you remove the "*", this outputs "[hi]", and it should be
> technically possible to make this work with external pipes too, provided
> it executes the Lisp code before generating the command string for sh.)

In this case I would want the whole '$(message ..'  construction to go
to the external shell.  Although extpipe supports some combinations of
piping Eshell in and out of the external shell, fundamentally it's more
about making it easier to bypass Eshell features than to make complex
usage of them.  It's also simpler to understand as a user.  If you do
want more involved combinations of Eshell and external shell commands,
you can always do the 'sh -c' wrapping yourself.

-- 
Sean Whitton

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Jim Porter]

On 3/31/2022 11:26 AM, Sean Whitton wrote:
> The call to `eshell-parse-lisp-argument' is meant to handle precisely
> your case.  It isn't doing atm, and I think it might actually be a bug
> over in that function.  To see this, with point on the first nonblank
> char of each of these lines, do 'M-: (eshell-parse-lisp-argument) RET':
> 
>      #'foo
> 
>      'foo
> 
> In the first case it successfully parses it and skips point forward, but
> in the latter case it does not.  But I think it should, right?

I'm not so sure. That would mean "'foo" in "echo 'foo" is treated as a 
Lisp form, but Eshell expects you to use "#'" (or "(" or "`") to 
introduce a Lisp form. As I understand it, that's so that typos don't do 
such surprising things. There's a good chance that a user typing "echo 
'foo" actually meant "echo 'foo'".

Because of that, in general, once you've moved point to partway inside a 
Lisp form, you can't use `eshell-parse-lisp-argument'. Maybe that should 
be changed? It's a bit surprising that you have to sharp-quote symbols 
in Eshell if you're not already inside a Lisp form (especially when 
those symbols aren't necessarily functions). Still, the typo-protection 
of the current implementation seems like a good feature...

That said, this isn't the only situation where "unbalanced" single 
quotes can occur in an Eshell command. For example, see this command:

   echo $(list "one" "two")(:s'o'x')

This creates a list of two strings, and then performs a regexp 
substitution from "o" to "x", so the output is:

   ("xne" "twx")

Under Emacs 27/28, this works correctly, but it fails in 29 for the same 
reason as the original issue. The extpipe module could account for this 
and try to parse argument predicates/modifiers so that it knows when to 
stay out of things, but then what about configurations where that module 
is disabled? (And for that matter, a third-party Eshell module could 
cause conflicts in a similar manner.)

>> If possible, I think it would be better for
>> `eshell-parse-external-pipeline' to solely focus on finding the
>> external pipe operators ("*|", "*<", and "*>")[1] and then for
>> `eshell-rewrite-external-pipeline' to prepare the command string to
>> pass to sh. This would also have the advantage[2] of making it
>> possible to support a richer set of Eshell features with external
>> pipes, such as the following:
>>
>>     ~ $ echo $(message "[%s]" "hi") *| cat
>>     zsh:1: command not found: message
>>
>> (If you remove the "*", this outputs "[hi]", and it should be
>> technically possible to make this work with external pipes too, provided
>> it executes the Lisp code before generating the command string for sh.)
> 
> In this case I would want the whole '$(message ..'  construction to go
> to the external shell.  Although extpipe supports some combinations of
> piping Eshell in and out of the external shell, fundamentally it's more
> about making it easier to bypass Eshell features than to make complex
> usage of them.  It's also simpler to understand as a user.  If you do
> want more involved combinations of Eshell and external shell commands,
> you can always do the 'sh -c' wrapping yourself.

 From my point of view, since the only difference between using an 
"Eshell pipe" and an extpipe is that the pipe operator has a "*", I'd 
expect the commands to work largely the same, except that the extpipe is 
faster. When I see a command like 'echo $(message "[%s]" "hi") *| cat', 
I usually think of it as a two-phase operation: expansions/subcommands 
are expanded first, and then the final command is executed. In that 
model, the expansions would still be "in Eshell".

On the other hand, maybe there's enough practical use for passing the 
raw command string to `sh' that there should be a very simple way of 
invoking it (and without having to mess around with escaping interior 
quotes, as you would if you used `sh -c' manually). Maybe the parsing 
would be more robust if it used special sigils for the start/end of the 
external command? I suppose that's similar to your original proposal of 
using !! or || to introduce an external pipeline, so maybe it's not 
feasible to go this route.

Another possibility would be to keep the current behavior (or close to 
it), but to reconstruct the command to pass to `sh' during Eshell's 
rewrite phase. I'm not quite sure if that would actually work, but if it 
did, it would allow other argument parsers to run normally without 
extpipe needing to know what parsers to try. Perhaps if we kept around 
the substring that each argument parser consumed, it would be possible 
to reconstruct the relevant bits for extpipe's purposes?

More generally though, maybe there are really two different use cases?

1) Eshell's built-in pipelines are slow because they go through Emacs 
buffers.

2) It would be convenient to invoke a whole command (or some large part 
of a command) using `sh' syntax.

For (1), Eshell could opportunistically use external pipelines without 
any special syntax. It should be possible to tell just by looking at the 
parsed command form if "foo | bar" connects two external processes on 
the same host, and then perform the appropriate rewrite to connect the 
processes efficiently (e.g. using `sh -c'). This would happen after 
expansion of variables/subcommands, so to the user it would work just 
like any other Eshell command, but it would be faster.

For (2), we'd need a convenient syntax for forwarding some command 
string to `sh'. Something like your proposed !! or || syntax, or maybe 
something to wrap around part of a command? (Or maybe an even something 
like an interactive `eshell-externalize' function that replaces the 
selected region with the correct `sh' invocation?)

And finally, sorry for bringing up these issues months after bug#46351. 
At the time, I didn't really understand the internals of Eshell, so I 
didn't have anything of substance to say then. Since then I've delved a 
bit *too* deep into Eshell's internals while trying to prove to myself 
that my implementation of Lisp function pipelines is 
sufficiently-flexible. :)

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Sean Whitton]

Hello,

On Thu 31 Mar 2022 at 01:58PM -07, Jim Porter wrote:

> I'm not so sure. That would mean "'foo" in "echo 'foo" is treated as a
> Lisp form, but Eshell expects you to use "#'" (or "(" or "`") to
> introduce a Lisp form. As I understand it, that's so that typos don't do
> such surprising things. There's a good chance that a user typing "echo
> 'foo" actually meant "echo 'foo'".

Right okay.  We can just skip over entire Lisp forms when we find them.
I don't think there could be a non-highly esoteric shell command for
standard POSIX shells -- which is what this feature is for -- which that
would break.  Like this:

>> diff --git a/lisp/eshell/em-extpipe.el b/lisp/eshell/em-extpipe.el
>> index eb5b3bfe1d..0787ab791b 100644
>> --- a/lisp/eshell/em-extpipe.el
>> +++ b/lisp/eshell/em-extpipe.el
>> @@ -99,7 +99,7 @@ eshell-parse-external-pipeline
>>                       (let* ((found
>>                               (save-excursion
>>                                 (re-search-forward
>> -                                "\\(?:#?'\\|\"\\|\\\\\\)" bound t)))
>> +                                "\\(?:(\\|#?'\\|\"\\|\\\\\\)" bound t)))
>>                              (next (or (and found (match-beginning 0))
>>                                        bound)))
>>                         (if (re-search-forward pat next t)
>>

Something in my init.el is breaking the extpipe tests atm, but I ad hoc
tested one of your cases for this bug and it works.  Could you confirm?

> That said, this isn't the only situation where "unbalanced" single
> quotes can occur in an Eshell command. For example, see this command:
>
>    echo $(list "one" "two")(:s'o'x')
>
> This creates a list of two strings, and then performs a regexp
> substitution from "o" to "x", so the output is:
>
>    ("xne" "twx")
>
> Under Emacs 27/28, this works correctly, but it fails in 29 for the
> same reason as the original issue. The extpipe module could account
> for this and try to parse argument predicates/modifiers so that it
> knows when to stay out of things, but then what about configurations
> where that module is disabled? (And for that matter, a third-party
> Eshell module could cause conflicts in a similar manner.)

I think the correct thing is for extpipe to do something sensible with
all the Eshell syntax that is enabled by default.  We should call the
relevant parsing function to skip over these predicates/modifiers too.

If you're changing the list of modules then you might have to drop
extpipe as part of that process -- that's just inherent in how Eshell
modules work.  There's always going to be a limit to how much syntax you
can have enabled at once.

-- 
Sean Whitton

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Jim Porter]

On 3/31/2022 2:55 PM, Sean Whitton wrote:
> On Thu 31 Mar 2022 at 01:58PM -07, Jim Porter wrote:
> 
>> I'm not so sure. That would mean "'foo" in "echo 'foo" is treated as a
>> Lisp form, but Eshell expects you to use "#'" (or "(" or "`") to
>> introduce a Lisp form. As I understand it, that's so that typos don't do
>> such surprising things. There's a good chance that a user typing "echo
>> 'foo" actually meant "echo 'foo'".
> 
> Right okay.  We can just skip over entire Lisp forms when we find them.
> I don't think there could be a non-highly esoteric shell command for
> standard POSIX shells -- which is what this feature is for -- which that
> would break.  Like this:
> 
>>> diff --git a/lisp/eshell/em-extpipe.el b/lisp/eshell/em-extpipe.el
>>> index eb5b3bfe1d..0787ab791b 100644
>>> --- a/lisp/eshell/em-extpipe.el
>>> +++ b/lisp/eshell/em-extpipe.el
>>> @@ -99,7 +99,7 @@ eshell-parse-external-pipeline
>>>                        (let* ((found
>>>                                (save-excursion
>>>                                  (re-search-forward
>>> -                                "\\(?:#?'\\|\"\\|\\\\\\)" bound t)))
>>> +                                "\\(?:(\\|#?'\\|\"\\|\\\\\\)" bound t)))
>>>                               (next (or (and found (match-beginning 0))
>>>                                         bound)))
>>>                          (if (re-search-forward pat next t)
>>>
> 
> Something in my init.el is breaking the extpipe tests atm, but I ad hoc
> tested one of your cases for this bug and it works.  Could you confirm?

The first test case -- "(eq 'foo nil)" -- works, but the second doesn't:

   ~ $ echo $(list "one" "two")(:s'o'x')
   Invalid read syntax: ")", 1, 33

Similarly:

   ~ $ echo *(:gs'o'x')
   Invalid read syntax: ")", 1, 16

I guess this is because it's now trying to read the argument modifier as 
a Lisp form. Normally, `eshell-parse-lisp-argument' would ignore that 
bit, since the `(' starting the modifier isn't at the beginning of an 
argument. This could probably be resolved with some additional logic 
(setting `eshell-current-argument' as appropriate?), but I hope there's 
a cleaner way that doesn't involve reimplementing 
`eshell-parse-argument' within `eshell-parse-external-pipeline'.

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Sean Whitton]

Hello,

On Thu 31 Mar 2022 at 03:19pm -07, Jim Porter wrote:

> This could probably be resolved with some additional logic (setting
> `eshell-current-argument' as appropriate?), but I hope there's a
> cleaner way that doesn't involve reimplementing
> `eshell-parse-argument' within `eshell-parse-external-pipeline'.

Well, quite.

Thank you for testing.  In the meantime, I've thought it all through
some more, and I think that your original idea of catching the
'eshell-incomplete is the right thing, assuming all tests pass.

I think the error should be caught inside the `or', though?  The idea
would be that if eshell-incomplete is thrown within one of the
disjuncts, that disjunct should return nil.

-- 
Sean Whitton

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Jim Porter]

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

On 3/31/2022 3:48 PM, Sean Whitton wrote:
> Thank you for testing.  In the meantime, I've thought it all through
> some more, and I think that your original idea of catching the
> 'eshell-incomplete is the right thing, assuming all tests pass.
> 
> I think the error should be caught inside the `or', though?  The idea
> would be that if eshell-incomplete is thrown within one of the
> disjuncts, that disjunct should return nil.

Hmm, that's an interesting thought. Maybe this code could be more 
particular about what parse function it calls. Since each of the 
function calls here:

                          (while (or (eshell-parse-lisp-argument)
                                     (eshell-parse-backslash)
                                     (eshell-parse-double-quote)
                                     (eshell-parse-literal-quote)))

correspond to a particular token here (earlier in the source):

                                (re-search-forward
                                 "\\(?:(\\|#?'\\|\"\\|\\\\\\)" bound t)))

perhaps it would be better to match the function call to the 
corresponding token. That is, if we see a "#?", we call 
`eshell-parse-lisp-argument', and so on. See the attached patch, which 
works in my tests (and passes all the existing Eshell unit tests).

[-- Attachment #2: 0001-Make-Eshell-s-extpipe-more-lenient-when-looking-for-.patch --]
[-- Type: text/plain, Size: 3034 bytes --]

From d3ffc4b10cc37a8babc3048ef3507d4d74c5fc2f Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 27 Mar 2022 19:04:42 -0700
Subject: [PATCH] Make Eshell's extpipe more lenient when looking for its
 operators

This could cause some errors when executing Eshell commands with an
odd number of quotation marks, such as "(eq 'foo nil)".

* lisp/eshell/em-extpipe.el (eshell-parse-external-pipeline): Catch
'eshell-incomplete' errors and only call the parse function
corresponding to the token at point.

* test/lisp/eshell/eshell-tests.el
(eshell-test/lisp-command-with-quote): New test.
---
 lisp/eshell/em-extpipe.el        | 11 +++++++----
 test/lisp/eshell/eshell-tests.el |  4 ++++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/lisp/eshell/em-extpipe.el b/lisp/eshell/em-extpipe.el
index eb5b3bfe1d..c587f318bf 100644
--- a/lisp/eshell/em-extpipe.el
+++ b/lisp/eshell/em-extpipe.el
@@ -100,15 +100,18 @@ eshell-parse-external-pipeline
                              (save-excursion
                                (re-search-forward
                                 "\\(?:#?'\\|\"\\|\\\\\\)" bound t)))
+                            (token (and found (match-string 0)))
                             (next (or (and found (match-beginning 0))
                                       bound)))
                        (if (re-search-forward pat next t)
                            (throw 'found (match-beginning 1))
                          (goto-char next)
-                         (while (or (eshell-parse-lisp-argument)
-                                    (eshell-parse-backslash)
-                                    (eshell-parse-double-quote)
-                                    (eshell-parse-literal-quote)))
+                         (catch 'eshell-incomplete
+                           (pcase token
+                             ("#'" (eshell-parse-lisp-argument))
+                             ("\\" (eshell-parse-backslash))
+                             ("\"" (eshell-parse-double-quote))
+                             ("'"  (eshell-parse-literal-quote))))
                          ;; Guard against an infinite loop if none of
                          ;; the parsers moved us forward.
                          (unless (or (> (point) next) (eobp))
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index e31db07c61..1e303f70e5 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -44,6 +44,10 @@ eshell-test/lisp-command
   "Test `eshell-command-result' with an elisp command."
   (should (equal (eshell-test-command-result "(+ 1 2)") 3)))
 
+(ert-deftest eshell-test/lisp-command-with-quote ()
+  "Test `eshell-command-result' with an elisp command containing a quote."
+  (should (equal (eshell-test-command-result "(eq 'foo nil)") nil)))
+
 (ert-deftest eshell-test/for-loop ()
   "Test `eshell-command-result' with a for loop.."
   (let ((process-environment (cons "foo" process-environment)))
-- 
2.25.1

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Sean Whitton]

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

Hello,

On Thu 31 Mar 2022 at 04:31PM -07, Jim Porter wrote:

> On 3/31/2022 3:48 PM, Sean Whitton wrote:
>> 
>> I think the error should be caught inside the `or', though?  The idea
>> would be that if eshell-incomplete is thrown within one of the
>> disjuncts, that disjunct should return nil.
>
> Hmm, that's an interesting thought. Maybe this code could be more 
> particular about what parse function it calls. Since each of the 
> function calls here:
>
>                           (while (or (eshell-parse-lisp-argument)
>                                      (eshell-parse-backslash)
>                                      (eshell-parse-double-quote)
>                                      (eshell-parse-literal-quote)))
>
> correspond to a particular token here (earlier in the source):
>
>                                 (re-search-forward
>                                  "\\(?:(\\|#?'\\|\"\\|\\\\\\)" bound t)))
>
> perhaps it would be better to match the function call to the 
> corresponding token.

Thank you for this suggestion, but I think that findbeg1 is more
readable, and actually perhaps more efficient, if we maintain the 
(while (or ...)) structure.

So I would like to install the attached patch to resolve this bug.

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-em-extpipe-Catch-eshell-incomplete-thrown-while-pars.patch --]
[-- Type: text/x-patch, Size: 3500 bytes --]

From a66fd4c30779a64e56621d21e589bb35856a57ea Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Fri, 1 Apr 2022 14:05:07 -0700
Subject: [PATCH] em-extpipe: Catch eshell-incomplete thrown while parsing

* lisp/eshell/em-extpipe.el (em-extpipe--or-with-catch): New macro.
(eshell-parse-external-pipeline): Use new macro to treat
`eshell-incomplete' as a failure of the parse function to move us
forward (Bug#54603).  Thanks to Jim Porter <jporterbugs@gmail.com> for
the report and for help isolating the problem.

* test/lisp/eshell/eshell-tests.el
(eshell-test/lisp-command-with-quote): New test for Bug#54603, thanks
to Jim Porter <jporterbugs@gmail.com>.
---
 lisp/eshell/em-extpipe.el        | 22 ++++++++++++++++++----
 test/lisp/eshell/eshell-tests.el |  4 ++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/lisp/eshell/em-extpipe.el b/lisp/eshell/em-extpipe.el
index eb5b3bfe1d..3db1dea595 100644
--- a/lisp/eshell/em-extpipe.el
+++ b/lisp/eshell/em-extpipe.el
@@ -49,6 +49,19 @@ eshell-extpipe-initialize
   (add-hook 'eshell-pre-rewrite-command-hook
             #'eshell-rewrite-external-pipeline -20 t))
 
+(defmacro em-extpipe--or-with-catch (&rest disjuncts)
+  "Evaluate DISJUNCTS like `or' but catch `eshell-incomplete'.
+
+If `eshell-incomplete' is thrown during the evaluation of a
+disjunct, that disjunct yields nil."
+  (let ((result (gensym)))
+    `(let (,result)
+       (or ,@(cl-loop for disjunct in disjuncts collect
+                      `(if (catch 'eshell-incomplete
+                             (ignore (setq ,result ,disjunct)))
+                           nil
+                         ,result))))))
+
 (defun eshell-parse-external-pipeline ()
   "Parse a pipeline intended for execution by the external shell.
 
@@ -105,10 +118,11 @@ eshell-parse-external-pipeline
                        (if (re-search-forward pat next t)
                            (throw 'found (match-beginning 1))
                          (goto-char next)
-                         (while (or (eshell-parse-lisp-argument)
-                                    (eshell-parse-backslash)
-                                    (eshell-parse-double-quote)
-                                    (eshell-parse-literal-quote)))
+                         (while (em-extpipe--or-with-catch
+                                 (eshell-parse-lisp-argument)
+                                 (eshell-parse-backslash)
+                                 (eshell-parse-double-quote)
+                                 (eshell-parse-literal-quote)))
                          ;; Guard against an infinite loop if none of
                          ;; the parsers moved us forward.
                          (unless (or (> (point) next) (eobp))
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index e31db07c61..1e303f70e5 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -44,6 +44,10 @@ eshell-test/lisp-command
   "Test `eshell-command-result' with an elisp command."
   (should (equal (eshell-test-command-result "(+ 1 2)") 3)))
 
+(ert-deftest eshell-test/lisp-command-with-quote ()
+  "Test `eshell-command-result' with an elisp command containing a quote."
+  (should (equal (eshell-test-command-result "(eq 'foo nil)") nil)))
+
 (ert-deftest eshell-test/for-loop ()
   "Test `eshell-command-result' with a for loop.."
   (let ((process-environment (cons "foo" process-environment)))
-- 
2.30.2

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Jim Porter]

On 4/1/2022 2:16 PM, Sean Whitton wrote:
> Thank you for this suggestion, but I think that findbeg1 is more
> readable, and actually perhaps more efficient, if we maintain the
> (while (or ...)) structure.
> 
> So I would like to install the attached patch to resolve this bug.

Thanks, this patch looks good to me, though I didn't actually build with 
it. Just let me know if you'd like me to test it out.

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

>> Thank you for this suggestion, but I think that findbeg1 is more
>> readable, and actually perhaps more efficient, if we maintain the
>> (while (or ...)) structure.
>> So I would like to install the attached patch to resolve this bug.
>
> Thanks, this patch looks good to me, though I didn't actually build
> with it. Just let me know if you'd like me to test it out.

I've now pushed Sean's patch to Emacs 29.

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

bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks

[Sean Whitton]

Hello,

On Sat 02 Apr 2022 at 04:09pm +02, Lars Ingebrigtsen wrote:

> Jim Porter <jporterbugs@gmail.com> writes:
>
>>> Thank you for this suggestion, but I think that findbeg1 is more
>>> readable, and actually perhaps more efficient, if we maintain the
>>> (while (or ...)) structure.
>>> So I would like to install the attached patch to resolve this bug.
>>
>> Thanks, this patch looks good to me, though I didn't actually build
>> with it. Just let me know if you'd like me to test it out.
>
> I've now pushed Sean's patch to Emacs 29.

Sweet, thanks both!

-- 
Sean Whitton

Re: Eshell's external pipe module interferes with other argument parsing hooks

[Sean Whitton]

[switching debbugs -> emacs-devel for this subthread]

Hello,

On Thu 31 Mar 2022 at 01:58PM -07, Jim Porter wrote:

> Another possibility would be to keep the current behavior (or close to
> it), but to reconstruct the command to pass to `sh' during Eshell's
> rewrite phase. I'm not quite sure if that would actually work, but if it
> did, it would allow other argument parsers to run normally without
> extpipe needing to know what parsers to try. Perhaps if we kept around
> the substring that each argument parser consumed, it would be possible
> to reconstruct the relevant bits for extpipe's purposes?

Well, in your case (2), you don't want the other parsers to get a chance
to run -- that's the whole point.

> More generally though, maybe there are really two different use cases?
>
> 1) Eshell's built-in pipelines are slow because they go through Emacs
> buffers.
>
> 2) It would be convenient to invoke a whole command (or some large part
> of a command) using `sh' syntax.

These are both things that extpipe is meant to make easy, though I'm not
sure how separate they are -- often I want both.

> For (1), Eshell could opportunistically use external pipelines without
> any special syntax. It should be possible to tell just by looking at the
> parsed command form if "foo | bar" connects two external processes on
> the same host, and then perform the appropriate rewrite to connect the
> processes efficiently (e.g. using `sh -c'). This would happen after
> expansion of variables/subcommands, so to the user it would work just
> like any other Eshell command, but it would be faster.

This could just be added to Eshell right now, right?  Definitely useful.

> For (2), we'd need a convenient syntax for forwarding some command
> string to `sh'. Something like your proposed !! or || syntax, or maybe
> something to wrap around part of a command?

Yeah, extpipe's syntax covers most such cases but not quite all of them.

> (Or maybe an even something like an interactive `eshell-externalize'
> function that replaces the selected region with the correct `sh'
> invocation?)

I had something like this previously.  The problem is it makes editing
and re-running the command a fair bit more fiddly.

> And finally, sorry for bringing up these issues months after
> bug#46351.  At the time, I didn't really understand the internals of
> Eshell, so I didn't have anything of substance to say then. Since then
> I've delved a bit *too* deep into Eshell's internals while trying to
> prove to myself that my implementation of Lisp function pipelines is
> sufficiently-flexible. :)

No problem, but could I request that you spend a little more time
editing your messages for length?  And perhaps consider separating out
discussion of significant future possible enhancements from fixing bugs
with the existing code into separate bugs or ML threads, as I've done
with this message.  Thanks in advance :)

-- 
Sean Whitton

Re: Eshell's external pipe module interferes with other argument parsing hooks

[Jim Porter]

On 3/31/2022 2:56 PM, Sean Whitton wrote:
> On Thu 31 Mar 2022 at 01:58PM -07, Jim Porter wrote:
> 
>> Another possibility would be to keep the current behavior (or close to
>> it), but to reconstruct the command to pass to `sh' during Eshell's
>> rewrite phase. I'm not quite sure if that would actually work, but if it
>> did, it would allow other argument parsers to run normally without
>> extpipe needing to know what parsers to try. Perhaps if we kept around
>> the substring that each argument parser consumed, it would be possible
>> to reconstruct the relevant bits for extpipe's purposes?
> 
> Well, in your case (2), you don't want the other parsers to get a chance
> to run -- that's the whole point.

In practice, yes. However, the implementation could allow the other 
parsers to run, but then discard their parsed results during the rewrite 
phase if applicable. This is probably subject to a different set of 
unusual corner cases, but would allow other parsers to consume the parts 
of the command that they care about so that extpipe can just look for 
`*|' and friends. (A `*|' sequence inside quotes or something similar 
would already have been consumed by the time extpipe sees it.)

>> More generally though, maybe there are really two different use cases?
>>
>> 1) Eshell's built-in pipelines are slow because they go through Emacs
>> buffers.
>>
>> 2) It would be convenient to invoke a whole command (or some large part
>> of a command) using `sh' syntax.
> 
> These are both things that extpipe is meant to make easy, though I'm not
> sure how separate they are -- often I want both.
> 
>> For (1), Eshell could opportunistically use external pipelines without
>> any special syntax.
[snip]
> 
> This could just be added to Eshell right now, right?  Definitely useful.

Unless there's a reason for Eshell's current behavior that I'm not aware 
of, I can't think of any problems with doing this, so long as everything 
is escaped properly.

>> For (2), we'd need a convenient syntax for forwarding some command
>> string to `sh'. Something like your proposed !! or || syntax, or maybe
>> something to wrap around part of a command?
> 
> Yeah, extpipe's syntax covers most such cases but not quite all of them.

For the purposes of parsing, having the token that activates the extpipe 
module be at the beginning of the relevant portion would make things a 
lot easier. Then `eshell-parse-external-pipeline' can just check if 
that's the next token and if so, read until the end of the extpipe 
portion. That would eliminate all the complexity of trying to identify 
unquoted/literal `*|' operators.

In practice though, I'm happy with any syntax so long as the 
implementation is robust. If the current implementation using `*|' 
operators is significantly nicer to use (I don't have an opinion either 
way since I haven't used it enough), then we should stick with it, even 
if it makes it harder to implement.

> No problem, but could I request that you spend a little more time
> editing your messages for length?  And perhaps consider separating out
> discussion of significant future possible enhancements from fixing bugs
> with the existing code into separate bugs or ML threads, as I've done
> with this message.  Thanks in advance :)

I'll try. :) I usually prefer shorter messages myself, but I've had a 
hard time finding the right balance on the Emacs lists. Sometimes I 
think I'm keeping things focused, only to find that I haven't relayed 
enough information, and instead have just confused matters. Splitting 
these threads off is probably the right call though. Thanks.

Re: Eshell's external pipe module interferes with other argument parsing hooks

[Sean Whitton]

Hello,

On Thu 31 Mar 2022 at 04:11PM -07, Jim Porter wrote:

> In practice, yes. However, the implementation could allow the other
> parsers to run, but then discard their parsed results during the rewrite
> phase if applicable. This is probably subject to a different set of
> unusual corner cases, but would allow other parsers to consume the parts
> of the command that they care about so that extpipe can just look for
> `*|' and friends. (A `*|' sequence inside quotes or something similar
> would already have been consumed by the time extpipe sees it.)

Okay, I think I see what you mean.  Worth keeping in mind.

> For the purposes of parsing, having the token that activates the extpipe
> module be at the beginning of the relevant portion would make things a
> lot easier. Then `eshell-parse-external-pipeline' can just check if
> that's the next token and if so, read until the end of the extpipe
> portion. That would eliminate all the complexity of trying to identify
> unquoted/literal `*|' operators.
>
> In practice though, I'm happy with any syntax so long as the
> implementation is robust. If the current implementation using `*|'
> operators is significantly nicer to use (I don't have an opinion either
> way since I haven't used it enough), then we should stick with it, even
> if it makes it harder to implement.

I think it's pretty nice to use, indeed.  But one thing that isn't so
easy that I would additionally like to have is just converting a single
command, with no pipes or redirection, to use the external shell.  You
can do something like append "*| cat" to your command but that's not
nice.  One possibility discussed previously was to support putting || at
the very beginning of the command to request that the whole thing be
sent to the external shell.  I'm still thinking I might add that.

-- 
Sean Whitton

Re: Eshell's external pipe module interferes with other argument parsing hooks

[Jim Porter]

On 3/31/2022 4:11 PM, Jim Porter wrote:
> On 3/31/2022 2:56 PM, Sean Whitton wrote:
>> On Thu 31 Mar 2022 at 01:58PM -07, Jim Porter wrote:
>>> 1) Eshell's built-in pipelines are slow because they go through Emacs
>>> buffers.
[snip]
>>> For (1), Eshell could opportunistically use external pipelines without
>>> any special syntax.
> [snip]
>>
>> This could just be added to Eshell right now, right?  Definitely useful.
> 
> Unless there's a reason for Eshell's current behavior that I'm not aware 
> of, I can't think of any problems with doing this, so long as everything 
> is escaped properly.

On the subject of Eshell's built-in pipelines being slow, this might be 
of interest: 
<https://tdodge.consulting/blog/eshell/background-output-thread>. The 
author seems to have some interest in upstreaming the patch too[1].

I haven't had a chance to look at the patch in very much detail yet, but 
if this worked for all cases (apparently it's limited to PTY 
subprocesses for now, and I'm sure there are other cases where it would 
need to stay out of the way), it would be a huge improvement to 
"regular" Eshell pipes.

- Jim

[1] 
https://www.reddit.com/r/emacs/comments/usghki/comment/i93hkmv/?utm_source=reddit&utm_medium=web2x&context=3