* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks @ 2022-03-28 2:21 Jim Porter 2022-03-31 11:46 ` Lars Ingebrigtsen 2022-03-31 18:26 ` Sean Whitton 0 siblings, 2 replies; 16+ messages in thread From: Jim Porter @ 2022-03-28 2:21 UTC (permalink / raw) To: 54603 [-- 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-03-28 2:21 bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks Jim Porter @ 2022-03-31 11:46 ` Lars Ingebrigtsen 2022-03-31 16:26 ` Jim Porter 2022-03-31 22:55 ` Sean Whitton 2022-03-31 18:26 ` Sean Whitton 1 sibling, 2 replies; 16+ messages in thread From: Lars Ingebrigtsen @ 2022-03-31 11:46 UTC (permalink / raw) To: Jim Porter; +Cc: 54603, John Wiegley 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-03-31 11:46 ` Lars Ingebrigtsen @ 2022-03-31 16:26 ` Jim Porter 2022-03-31 16:50 ` Eli Zaretskii 2022-03-31 17:35 ` Michael Albinus 2022-03-31 22:55 ` Sean Whitton 1 sibling, 2 replies; 16+ messages in thread From: Jim Porter @ 2022-03-31 16:26 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: 54603, John Wiegley, Sean Whitton 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.) ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-03-31 16:26 ` Jim Porter @ 2022-03-31 16:50 ` Eli Zaretskii 2022-03-31 17:35 ` Michael Albinus 1 sibling, 0 replies; 16+ messages in thread From: Eli Zaretskii @ 2022-03-31 16:50 UTC (permalink / raw) To: Jim Porter; +Cc: larsi, 54603, johnw, spwhitton > 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-03-31 16:26 ` Jim Porter 2022-03-31 16:50 ` Eli Zaretskii @ 2022-03-31 17:35 ` Michael Albinus 1 sibling, 0 replies; 16+ messages in thread From: Michael Albinus @ 2022-03-31 17:35 UTC (permalink / raw) To: Jim Porter; +Cc: Lars Ingebrigtsen, 54603, John Wiegley, Sean Whitton 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-03-31 11:46 ` Lars Ingebrigtsen 2022-03-31 16:26 ` Jim Porter @ 2022-03-31 22:55 ` Sean Whitton 1 sibling, 0 replies; 16+ messages in thread From: Sean Whitton @ 2022-03-31 22:55 UTC (permalink / raw) To: Lars Ingebrigtsen, Jim Porter; +Cc: 54603, John Wiegley 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-03-28 2:21 bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks Jim Porter 2022-03-31 11:46 ` Lars Ingebrigtsen @ 2022-03-31 18:26 ` Sean Whitton 2022-03-31 20:58 ` Jim Porter 1 sibling, 1 reply; 16+ messages in thread From: Sean Whitton @ 2022-03-31 18:26 UTC (permalink / raw) To: Jim Porter, 54603 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-03-31 18:26 ` Sean Whitton @ 2022-03-31 20:58 ` Jim Porter 2022-03-31 21:55 ` Sean Whitton 0 siblings, 1 reply; 16+ messages in thread From: Jim Porter @ 2022-03-31 20:58 UTC (permalink / raw) To: Sean Whitton, 54603 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. :) ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-03-31 20:58 ` Jim Porter @ 2022-03-31 21:55 ` Sean Whitton 2022-03-31 22:19 ` Jim Porter 0 siblings, 1 reply; 16+ messages in thread From: Sean Whitton @ 2022-03-31 21:55 UTC (permalink / raw) To: Jim Porter, 54603 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-03-31 21:55 ` Sean Whitton @ 2022-03-31 22:19 ` Jim Porter 2022-03-31 22:48 ` Sean Whitton 0 siblings, 1 reply; 16+ messages in thread From: Jim Porter @ 2022-03-31 22:19 UTC (permalink / raw) To: Sean Whitton, 54603 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'. ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-03-31 22:19 ` Jim Porter @ 2022-03-31 22:48 ` Sean Whitton 2022-03-31 23:31 ` Jim Porter 0 siblings, 1 reply; 16+ messages in thread From: Sean Whitton @ 2022-03-31 22:48 UTC (permalink / raw) To: Jim Porter, 54603 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-03-31 22:48 ` Sean Whitton @ 2022-03-31 23:31 ` Jim Porter 2022-04-01 21:16 ` Sean Whitton 0 siblings, 1 reply; 16+ messages in thread From: Jim Porter @ 2022-03-31 23:31 UTC (permalink / raw) To: Sean Whitton, 54603 [-- 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-03-31 23:31 ` Jim Porter @ 2022-04-01 21:16 ` Sean Whitton 2022-04-02 1:31 ` Jim Porter 0 siblings, 1 reply; 16+ messages in thread From: Sean Whitton @ 2022-04-01 21:16 UTC (permalink / raw) To: Jim Porter, 54603 [-- 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-04-01 21:16 ` Sean Whitton @ 2022-04-02 1:31 ` Jim Porter 2022-04-02 14:09 ` Lars Ingebrigtsen 0 siblings, 1 reply; 16+ messages in thread From: Jim Porter @ 2022-04-02 1:31 UTC (permalink / raw) To: Sean Whitton, 54603 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-04-02 1:31 ` Jim Porter @ 2022-04-02 14:09 ` Lars Ingebrigtsen 2022-04-02 15:52 ` Sean Whitton 0 siblings, 1 reply; 16+ messages in thread From: Lars Ingebrigtsen @ 2022-04-02 14:09 UTC (permalink / raw) To: Jim Porter; +Cc: 54603, Sean Whitton 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks 2022-04-02 14:09 ` Lars Ingebrigtsen @ 2022-04-02 15:52 ` Sean Whitton 0 siblings, 0 replies; 16+ messages in thread From: Sean Whitton @ 2022-04-02 15:52 UTC (permalink / raw) To: Lars Ingebrigtsen, Jim Porter; +Cc: 54603 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-04-02 15:52 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-03-28 2:21 bug#54603: 29.0.50; [PATCH] Eshell's external pipe module interferes with other argument parsing hooks Jim Porter 2022-03-31 11:46 ` Lars Ingebrigtsen 2022-03-31 16:26 ` Jim Porter 2022-03-31 16:50 ` Eli Zaretskii 2022-03-31 17:35 ` Michael Albinus 2022-03-31 22:55 ` Sean Whitton 2022-03-31 18:26 ` Sean Whitton 2022-03-31 20:58 ` Jim Porter 2022-03-31 21:55 ` Sean Whitton 2022-03-31 22:19 ` Jim Porter 2022-03-31 22:48 ` Sean Whitton 2022-03-31 23:31 ` Jim Porter 2022-04-01 21:16 ` Sean Whitton 2022-04-02 1:31 ` Jim Porter 2022-04-02 14:09 ` Lars Ingebrigtsen 2022-04-02 15:52 ` Sean Whitton
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).