GNU bug report logs - #53715
29.0.50; [PATCH] Improve correctness of pipelines in Eshell

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 53715 in the body.
You can then email your comments to 53715 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: 29.0.50; [PATCH] Improve correctness of pipelines in Eshell
Date: Tue, 1 Feb 2022 19:32:10 -0800

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

From `emacs -Q', start Eshell, and first run the following:

  ~ $ *echo hi | echo bye

This results in:

  bye
  ~ $ hi

That is, `*echo' writes its output *after* the prompt is emitted. This 
is because `eshell-do-pipelines' reports that there's no process object 
to wait for, so Eshell thinks the prompt can be emitted immediately. I 
fixed that in the first patch by setting the `tailproc' in a different 
spot. With this change, the result is:

  bye
  hi
  ~ $

This may be a bit surprising, since you'd normally expect "echo bye" to 
read "hi" on its stdin and just ignore it. However, the builtin Eshell 
echo command doesn't consume stdin, so it just goes straight to stdout 
instead. It's in reverse order because Eshell runs the pipeline from 
right-to-left in order to hook up the pipes properly. For how the 
builtin Eshell echo works, I think this is the right behavior. (If 
people disagree, I can fix this, but not right now; I'm working on a 
patch series to properly support piping to Lisp functions in Eshell, and 
I can change it - or not - once that's done.)

Second, run this:

  ~ $ tr a-z A-Z | rev
  hello RET
  C-c C-d  ;; Send EOF

The result is:

  olleh  ;; should be "OLLEH"

This happens because Eshell only keeps track of the tail process when 
running an external process, so when sending stdin, it gets sent to the 
tail process (`rev'), not the head (`tr'). I fixed this in the second 
patch by recording both the head and tail processes and updating the 
various places that use the current process to use the right one.

[0001-Ensure-that-tailproc-is-set-for-the-last-process-in-.patch (text/plain, attachment)]

[0002-When-executing-an-Eshell-pipeline-send-input-to-the-.patch (text/plain, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 53715 <at> debbugs.gnu.org
Subject: Re: bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in
 Eshell
Date: Wed, 02 Feb 2022 19:10:22 +0100

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

> This happens because Eshell only keeps track of the tail process when
> running an external process, so when sending stdin, it gets sent to
> the tail process (`rev'), not the head (`tr'). I fixed this in the
> second patch by recording both the head and tail processes and
> updating the various places that use the current process to use the
> right one.

These patches seem to lead to a number of tests failing:

Test eshell-test/last-result-var2 condition:
    (void-variable eshell-last-async-proc)
   FAILED  22/34  eshell-test/last-result-var2 (0.001074 sec) at lisp/eshell/eshell-tests.el:164
   passed  23/34  eshell-test/lisp-command (0.000350 sec)
   passed  24/34  eshell-test/lisp-command-args (0.000308 sec)


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

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 53715 <at> debbugs.gnu.org
Subject: Re: bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in
 Eshell
Date: Wed, 2 Feb 2022 11:41:33 -0800

On 2/2/2022 10:10 AM, Lars Ingebrigtsen wrote:
> These patches seem to lead to a number of tests failing:
> 
> Test eshell-test/last-result-var2 condition:
>      (void-variable eshell-last-async-proc)
>     FAILED  22/34  eshell-test/last-result-var2 (0.001074 sec) at lisp/eshell/eshell-tests.el:164
>     passed  23/34  eshell-test/lisp-command (0.000350 sec)
>     passed  24/34  eshell-test/lisp-command-args (0.000308 sec)

Sorry, I forgot to mention that since the second patch updates a 
defsubst (`eshell-interactive-process'), you'll need to recompile all 
the files that might use it, so `make boostrap' or `touch 
lisp/eshell/*.el test/lisp/eshell/*.el && make'. That should make these 
tests pass.

Once this merges, I'll send a message to emacs-devel to let everyone 
know to recompile these files so there are no surprises when they try to 
use Eshell later on.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 53715 <at> debbugs.gnu.org
Subject: Re: bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in
 Eshell
Date: Wed, 02 Feb 2022 20:49:57 +0100

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

> Sorry, I forgot to mention that since the second patch updates a
> defsubst (`eshell-interactive-process'), you'll need to recompile all
> the files that might use it, so `make boostrap' or `touch
> lisp/eshell/*.el test/lisp/eshell/*.el && make'. That should make
> these tests pass.

I did remove all the .elc files in lisp/eshell and rebuilt, but the
tests still failed.  (I didn't try to delete the test elc files,
though.)

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

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 53715 <at> debbugs.gnu.org
Subject: Re: bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in
 Eshell
Date: Wed, 2 Feb 2022 13:01:16 -0800

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

On 2/2/2022 11:49 AM, Lars Ingebrigtsen wrote:
> Jim Porter <jporterbugs <at> gmail.com> writes:
> 
>> Sorry, I forgot to mention that since the second patch updates a
>> defsubst (`eshell-interactive-process'), you'll need to recompile all
>> the files that might use it, so `make boostrap' or `touch
>> lisp/eshell/*.el test/lisp/eshell/*.el && make'. That should make
>> these tests pass.
> 
> I did remove all the .elc files in lisp/eshell and rebuilt, but the
> tests still failed.  (I didn't try to delete the test elc files,
> though.)

Ah, I think I see the issue. I should have updated 
`eshell-wait-for-subprocess' in test/lisp/eshell/eshell-tests-helpers.el 
to use the new defsubst, which would have caused it to get recompiled. 
(Though manually recompiling it should also work.)

I've attached a fixed patch (the first one is the same; I only updated 
the second).

[0001-Ensure-that-tailproc-is-set-for-the-last-process-in-.patch (text/plain, attachment)]

[0002-When-executing-an-Eshell-pipeline-send-input-to-the-.patch (text/plain, attachment)]

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 53715 <at> debbugs.gnu.org
Subject: Re: bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in
 Eshell
Date: Wed, 2 Feb 2022 18:35:00 -0800

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

On 2/2/2022 1:01 PM, Jim Porter wrote:
> Ah, I think I see the issue. I should have updated 
> `eshell-wait-for-subprocess' in test/lisp/eshell/eshell-tests-helpers.el 
> to use the new defsubst, which would have caused it to get recompiled. 
> (Though manually recompiling it should also work.)
> 
> I've attached a fixed patch (the first one is the same; I only updated 
> the second).

Here's a small additional improvement that I hope is correct. The third 
patch here changes how eshell-tests-helpers.el is loaded to that it uses 
`require'. This reduces some of the boilerplate and will hopefully 
prevent issues with this file not getting recompiled when it's updated.

The first two patches are the same as before. I've just included them 
for completeness/ease of applying.

[0001-Ensure-that-tailproc-is-set-for-the-last-process-in-.patch (text/plain, attachment)]

[0002-When-executing-an-Eshell-pipeline-send-input-to-the-.patch (text/plain, attachment)]

[0003-Use-require-to-load-eshell-tests-helpers.patch (text/plain, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 53715 <at> debbugs.gnu.org
Subject: Re: bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in
 Eshell
Date: Thu, 03 Feb 2022 20:03:41 +0100

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

> Here's a small additional improvement that I hope is correct. The
> third patch here changes how eshell-tests-helpers.el is loaded to that
> it uses `require'. This reduces some of the boilerplate and will
> hopefully prevent issues with this file not getting recompiled when
> it's updated.
>
> The first two patches are the same as before. I've just included them
> for completeness/ease of applying.

Thanks; seems to work well for me, so I've now pushed them to Emacs 29.

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

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 53715 <at> debbugs.gnu.org
Subject: Re: bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in
 Eshell
Date: Fri, 4 Feb 2022 22:51:43 -0800

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

On 2/3/2022 11:03 AM, Lars Ingebrigtsen wrote:
> Jim Porter <jporterbugs <at> gmail.com> writes:
> 
>> Here's a small additional improvement that I hope is correct. The
>> third patch here changes how eshell-tests-helpers.el is loaded to that
>> it uses `require'. This reduces some of the boilerplate and will
>> hopefully prevent issues with this file not getting recompiled when
>> it's updated.
>>
>> The first two patches are the same as before. I've just included them
>> for completeness/ease of applying.
> 
> Thanks; seems to work well for me, so I've now pushed them to Emacs 29.

I found a bug in the second patch.

  emacs -Q --eval '(eshell)'
  ~ $ echo hi | *cat

This prints:

  ~ $ hi

That is, the output of the command is printed *after* the next prompt. 
That's because my patch wasn't smart enough about finding the "head" 
process in a pipeline. In "echo hi | *cat", the head process is "cat" 
(Eshell's builtin echo command doesn't create a process). In my old 
patch, it thought the head process was nil, which confused Eshell.

Here's a patch with a test to verify that things work correctly. Now the 
output is:

  hi~ $

(That's a bit ugly, but Eshell's builtin echo doesn't normally print a 
newline, so it's correct.)

[0001-Ensure-that-the-CAR-of-eshell-last-async-procs-alway.patch (text/plain, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 53715 <at> debbugs.gnu.org
Subject: Re: bug#53715: 29.0.50; [PATCH] Improve correctness of pipelines in
 Eshell
Date: Sat, 05 Feb 2022 07:59:22 +0100

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

> Here's a patch with a test to verify that things work correctly. Now
> the output is:

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