bug#54136: 29.0.50; Eshell emits extra prompts when killing processes in some cases

bug#54136: 29.0.50; Eshell emits extra prompts when killing processes in some cases

[Jim Porter]

(Patch forthcoming; I'm just getting a bug number first...)

Sometimes, when killing processes in Eshell (especially in pipelines), 
Eshell will emit *two* prompts instead of one. Starting with "emacs -Q 
--eval '(eshell)'...

Problem #1: Killing pipelines
-----------------------------

   ~ $ sh -c 'while true; do echo y; sleep 1; done' | sh -c 'while true; 
do read NAME; done'
   C-c C-c  ; or C-c C-k

The result is ("|" is the point):

   interrupted
   ~ $
   ~ $ |

What's happening here is one prompt is being emitted per process in the 
pipeline. It should only emit one prompt total.


Problem #2: Killing the head of a pipeline
------------------------------------------

   ~ $ sh -c 'while true; do sleep 1; done' | sh -c 'while read NAME; do 
echo =${NAME}=; done'
   M-: (kill-process (eshell-head-process)) RET

The result is:

   ~ $ =killed=
   |

In this case, Eshell writes the head process's exit status message to 
its stdout handle (i.e. to the tail process's stdin). It should either 
write directly to the terminal, or not write at all. I went with the 
latter, since the former would mean that we write "interrupted" N times 
in problem #1 above.


Problem #3: Killing a background process
----------------------------------------

    ~ $ sh -c 'while true; do sleep 1; done' &

This outputs, as expected:

   [sh] 12345
   ~ $

Now call `(kill-process (caar eshell-process-list))'. The result is:

   ~ $ killed
   ~ $ |

Here, Eshell writes the exit status to the terminal, but does so in an 
awkward spot, and then emits a new prompt. I think it should just avoid 
printing anything here. Information about the killed process is also 
shown in the minibuffer, so the user already has a place (and a better 
one at that!) to see what happened.

bug#54136: 29.0.50; Eshell emits extra prompts when killing processes in some cases

[Jim Porter]

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

On 2/23/2022 9:11 PM, Jim Porter wrote:
> (Patch forthcoming; I'm just getting a bug number first...)

Ok, here's the patch with some tests.

[-- Attachment #2: 0001-Don-t-superfluously-emit-prompts-when-terminating-pr.patch --]
[-- Type: text/plain, Size: 5244 bytes --]

From 52530c208d1b06cfbf527d77425bbbae1666b345 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 23 Feb 2022 20:43:38 -0800
Subject: [PATCH] Don't superfluously emit prompts when terminating processes
 in Eshell

* lisp/eshell/esh-proc.el (eshell-kill-process-function): Only reset
the prompt if PROC is writing to the terminal.
(eshell-sentinel): Only write the exit message if PROC is writing to
the terminal (bug#54136).

* test/lisp/eshell/esh-proc-tests.el (esh-proc-test/kill-pipeline)
(esh-proc-test/kill-pipeline-head)
(esh-proc-test/kill-background-process): New tests.
---
 lisp/eshell/esh-proc.el            | 16 ++++++++---
 test/lisp/eshell/esh-proc-tests.el | 46 ++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index d7d22d2a9e..70426ccaf2 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -102,6 +102,7 @@ eshell-process-list
   "A list of the current status of subprocesses.")
 
 (declare-function eshell-send-eof-to-process "esh-mode")
+(declare-function eshell-tail-process "esh-cmd")
 
 (defvar-keymap eshell-proc-mode-map
   "C-c M-i"  #'eshell-insert-process
@@ -119,7 +120,9 @@ eshell-kill-process-function
 PROC and STATUS to functions on the latter."
   ;; Was there till 24.1, but it is not optional.
   (remove-hook 'eshell-kill-hook #'eshell-reset-after-proc)
-  (eshell-reset-after-proc status)
+  ;; Only reset the prompt if this process is running interactively.
+  (when (eq proc (eshell-tail-process))
+    (eshell-reset-after-proc status))
   (run-hook-with-args 'eshell-kill-hook proc status))
 
 (define-minor-mode eshell-proc-mode
@@ -414,7 +417,7 @@ eshell-sentinel
   (when (buffer-live-p (process-buffer proc))
     (with-current-buffer (process-buffer proc)
       (unwind-protect
-	  (let* ((entry (assq proc eshell-process-list)))
+          (let ((entry (assq proc eshell-process-list)))
 ;	    (if (not entry)
 ;		(error "Sentinel called for unowned process `%s'"
 ;		       (process-name proc))
@@ -422,8 +425,13 @@ eshell-sentinel
 	      (unwind-protect
 		  (progn
 		    (unless (string= string "run")
-		      (unless (string-match "^\\(finished\\|exited\\)" string)
-			(eshell-insertion-filter proc string))
+                      ;; Write the exit message if the status is
+                      ;; abnormal and the process is already writing
+                      ;; to the terminal.
+                      (when (and (eq proc (eshell-tail-process))
+                                 (not (string-match "^\\(finished\\|exited\\)"
+                                                    string)))
+                        (funcall (process-filter proc) proc string))
                       (let ((handles (nth 1 entry))
                             (str (prog1 (nth 3 entry)
                                    (setf (nth 3 entry) nil)))
diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
index e7ea6c00d6..a8be0f8030 100644
--- a/test/lisp/eshell/esh-proc-tests.el
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -43,3 +43,49 @@ esh-proc-test/sigpipe-exits-process
     "y\n")
    (eshell-wait-for-subprocess t)
    (should (eq (process-list) nil))))
+
+(ert-deftest esh-proc-test/kill-pipeline ()
+  "Test that killing a pipeline of processes only emits a single
+prompt.  See bug#54136."
+  (skip-unless (and (executable-find "sh")
+                    (executable-find "echo")
+                    (executable-find "sleep")))
+  (with-temp-eshell
+   (eshell-insert-command
+    (concat "sh -c 'while true; do echo y; sleep 1; done' | "
+            "sh -c 'while true; do read NAME; done'"))
+   (let ((output-start (eshell-beginning-of-output)))
+     (eshell-kill-process)
+     (eshell-wait-for-subprocess t)
+     (should (equal (buffer-substring-no-properties
+                     output-start (eshell-end-of-output))
+                    "killed\n")))))
+
+(ert-deftest esh-proc-test/kill-pipeline-head ()
+  "Test that killing the first process in a pipeline doesn't
+write the exit status to the pipe.  See bug#54136."
+  (skip-unless (and (executable-find "sh")
+                    (executable-find "echo")
+                    (executable-find "sleep")))
+  (with-temp-eshell
+   (eshell-insert-command
+    (concat "sh -c 'while true; sleep 1; done' | "
+            "sh -c 'while read NAME; do echo =${NAME}=; done'"))
+   (let ((output-start (eshell-beginning-of-output)))
+     (kill-process (eshell-head-process))
+     (eshell-wait-for-subprocess t)
+     (should (equal (buffer-substring-no-properties
+                     output-start (eshell-end-of-output))
+                    "")))))
+
+(ert-deftest esh-proc-test/kill-background-process ()
+  "Test that killing a background process doesn't emit a new
+prompt.  See bug#54136."
+  (skip-unless (and (executable-find "sh")
+                    (executable-find "sleep")))
+  (with-temp-eshell
+   (eshell-insert-command "sh -c 'while true; do sleep 1; done' &")
+   (kill-process (caar eshell-process-list))
+   ;; Give `eshell-sentinel' a chance to run.
+   (sit-for 0.1)
+   (eshell-match-result "\\[sh\\] [[:digit:]]+\n")))
-- 
2.25.1

bug#54136: 29.0.50; Eshell emits extra prompts when killing processes in some cases

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

> Ok, here's the patch with some tests.

Thanks; pushed to Emacs 29.

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

bug#54136: 29.0.50; Eshell emits extra prompts when killing processes in some cases

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 24 Feb 2022 10:35:32 +0100
> Cc: 54136@debbugs.gnu.org
> 
> Jim Porter <jporterbugs@gmail.com> writes:
> 
> > Ok, here's the patch with some tests.
> 
> Thanks; pushed to Emacs 29.

The new tests fail on MS-Windows.  Does the patch below look right?
I'm quite sure about the "sh" vs "sh.exe" part, but what about the
"killed" vs "interrupt" part? any idea why this happens on MS-Windows?

diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
index a8be0f8..3f67961 100644
--- a/test/lisp/eshell/esh-proc-tests.el
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -59,7 +59,7 @@ esh-proc-test/kill-pipeline
      (eshell-wait-for-subprocess t)
      (should (equal (buffer-substring-no-properties
                      output-start (eshell-end-of-output))
-                    "killed\n")))))
+                    (or "interrupt\n" "killed\n"))))))
 
 (ert-deftest esh-proc-test/kill-pipeline-head ()
   "Test that killing the first process in a pipeline doesn't
@@ -88,4 +88,4 @@ esh-proc-test/kill-background-process
    (kill-process (caar eshell-process-list))
    ;; Give `eshell-sentinel' a chance to run.
    (sit-for 0.1)
-   (eshell-match-result "\\[sh\\] [[:digit:]]+\n")))
+   (eshell-match-result "\\[sh\\(\\.exe\\)?\\] [[:digit:]]+\n")))

bug#54136: 29.0.50; Eshell emits extra prompts when killing processes in some cases

[Jim Porter]

On 2/24/2022 3:03 AM, Eli Zaretskii wrote:
>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Date: Thu, 24 Feb 2022 10:35:32 +0100
>> Cc: 54136@debbugs.gnu.org
>>
>> Thanks; pushed to Emacs 29.
> 
> The new tests fail on MS-Windows.  Does the patch below look right?

Thanks, and sorry for the breakage.

> I'm quite sure about the "sh" vs "sh.exe" part, but what about the
> "killed" vs "interrupt" part? any idea why this happens on MS-Windows?

I think this makes sense. POSIX-style signal handling on MS Windows is 
only approximate, so I'm not surprised it produces some surprising 
results like that. Maybe it's possible to pass the signal code through 
the process exit status more accurately, but that's probably only a 
partial solution at best (due to the API differences), and at worst 
could break Emacs's subprocess handling for MS Windows in subtle ways.

Another solution would be to replace the call to `eshell-kill-process' 
in `esh-proc-test/kill-pipeline' with `eshell-interrupt-process'. Then 
the exit status should be "interrupt\n" on all platforms. I've verified 
that that works ok on GNU/Linux, but it's possible that it runs into 
some other issue on MS Windows.

This is sort of just working around the issue, but I also think there's 
an argument that `eshell-interrupt-process' is the more "natural" 
function to call, since it corresponds to a user hitting ^C in the shell 
(or C-c C-c in Eshell).

bug#54136: 29.0.50; Eshell emits extra prompts when killing processes in some cases

[Eli Zaretskii]

> Cc: 54136@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Thu, 24 Feb 2022 10:55:48 -0800
> 
> > I'm quite sure about the "sh" vs "sh.exe" part, but what about the
> > "killed" vs "interrupt" part? any idea why this happens on MS-Windows?
> 
> I think this makes sense. POSIX-style signal handling on MS Windows is 
> only approximate, so I'm not surprised it produces some surprising 
> results like that. Maybe it's possible to pass the signal code through 
> the process exit status more accurately, but that's probably only a 
> partial solution at best (due to the API differences), and at worst 
> could break Emacs's subprocess handling for MS Windows in subtle ways.
> 
> Another solution would be to replace the call to `eshell-kill-process' 
> in `esh-proc-test/kill-pipeline' with `eshell-interrupt-process'. Then 
> the exit status should be "interrupt\n" on all platforms. I've verified 
> that that works ok on GNU/Linux, but it's possible that it runs into 
> some other issue on MS Windows.
> 
> This is sort of just working around the issue, but I also think there's 
> an argument that `eshell-interrupt-process' is the more "natural" 
> function to call, since it corresponds to a user hitting ^C in the shell 
> (or C-c C-c in Eshell).

OK, so I installed my changes.

Thanks.

bug#54136: 29.0.50; Eshell emits extra prompts when killing processes in some cases

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> OK, so I installed my changes.

The test now fails on Debian:

Test esh-proc-test/kill-pipeline condition:
    (ert-test-failed
     ((should
       (equal
	(buffer-substring-no-properties output-start ...)
	(or "interrupt\n" "killed\n")))
      :form
      (equal "killed\n" "interrupt\n")
      :value nil :explanation
      (arrays-of-different-length 7 10 "killed\n" "interrupt\n" first-mismatch-at 0)))
   FAILED  2/4  esh-proc-test/kill-pipeline (0.109668 sec) at lisp/eshell/esh-proc-tests.el:47
   passed  3/4  esh-proc-test/kill-pipeline-head (0.112142 sec)
   passed  4/4  esh-proc-test/sigpipe-exits-process (1.010359 sec)

Ran 4 tests, 3 results as expected, 1 unexpected (2022-02-24 21:06:48+0100, 1.446135 sec)

1 unexpected results:
   FAILED  esh-proc-test/kill-pipeline

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

bug#54136: 29.0.50; Eshell emits extra prompts when killing processes in some cases

[Jim Porter]

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

On 2/24/2022 12:07 PM, Lars Ingebrigtsen wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>> OK, so I installed my changes.
> 
> The test now fails on Debian:
> 
> Test esh-proc-test/kill-pipeline condition:
>      (ert-test-failed
>       ((should
>         (equal
> 	(buffer-substring-no-properties output-start ...)
> 	(or "interrupt\n" "killed\n")))
>        :form
>        (equal "killed\n" "interrupt\n")
>        :value nil :explanation
>        (arrays-of-different-length 7 10 "killed\n" "interrupt\n" first-mismatch-at 0)))
>     FAILED  2/4  esh-proc-test/kill-pipeline (0.109668 sec) at lisp/eshell/esh-proc-tests.el:47
>     passed  3/4  esh-proc-test/kill-pipeline-head (0.112142 sec)
>     passed  4/4  esh-proc-test/sigpipe-exits-process (1.010359 sec)
> 
> Ran 4 tests, 3 results as expected, 1 unexpected (2022-02-24 21:06:48+0100, 1.446135 sec)
> 
> 1 unexpected results:
>     FAILED  esh-proc-test/kill-pipeline

Here's a patch for that. It works for me on GNU/Linux, and hopefully 
it's ok on MS Windows too.

[-- Attachment #2: 0001-Fix-Eshell-process-tests-to-hopefully-work-on-all-pl.patch --]
[-- Type: text/plain, Size: 1370 bytes --]

From 0b91535f7ba49b4e864aa735c8a83ee73b5de118 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 24 Feb 2022 16:57:44 -0800
Subject: [PATCH] Fix Eshell process tests to (hopefully) work on all platforms

* test/lisp/eshell/esh-proc-tests.el (esh-proc-test/kill-pipeline):
Fix test.
---
 test/lisp/eshell/esh-proc-tests.el | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
index e19eaf9779..8cd893ce57 100644
--- a/test/lisp/eshell/esh-proc-tests.el
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -57,10 +57,11 @@ esh-proc-test/kill-pipeline
    (let ((output-start (eshell-beginning-of-output)))
      (eshell-kill-process)
      (eshell-wait-for-subprocess t)
-     (should (equal (buffer-substring-no-properties
-                     output-start (eshell-end-of-output))
-                    ;; "interrupt\n" is for MS-Windows.
-                    (or "interrupt\n" "killed\n"))))))
+     (should (string-match-p
+              ;; "interrupt\n" is for MS-Windows.
+              (rx (or "interrupt\n" "killed\n"))
+              (buffer-substring-no-properties
+               output-start (eshell-end-of-output)))))))
 
 (ert-deftest esh-proc-test/kill-pipeline-head ()
   "Test that killing the first process in a pipeline doesn't
-- 
2.25.1

bug#54136: 29.0.50; Eshell emits extra prompts when killing processes in some cases

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

> Here's a patch for that. It works for me on GNU/Linux, and hopefully
> it's ok on MS Windows too.

Thanks; pushed to the trunk.

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

bug#54136: 29.0.50; Eshell emits extra prompts when killing processes in some cases

[Eli Zaretskii]

> Cc: 54136@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Thu, 24 Feb 2022 17:04:42 -0800
> 
> On 2/24/2022 12:07 PM, Lars Ingebrigtsen wrote:
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> >> OK, so I installed my changes.
> > 
> > The test now fails on Debian:
> > 
> > Test esh-proc-test/kill-pipeline condition:
> >      (ert-test-failed
> >       ((should
> >         (equal
> > 	(buffer-substring-no-properties output-start ...)
> > 	(or "interrupt\n" "killed\n")))
> >        :form
> >        (equal "killed\n" "interrupt\n")
> >        :value nil :explanation
> >        (arrays-of-different-length 7 10 "killed\n" "interrupt\n" first-mismatch-at 0)))
> >     FAILED  2/4  esh-proc-test/kill-pipeline (0.109668 sec) at lisp/eshell/esh-proc-tests.el:47
> >     passed  3/4  esh-proc-test/kill-pipeline-head (0.112142 sec)
> >     passed  4/4  esh-proc-test/sigpipe-exits-process (1.010359 sec)
> > 
> > Ran 4 tests, 3 results as expected, 1 unexpected (2022-02-24 21:06:48+0100, 1.446135 sec)
> > 
> > 1 unexpected results:
> >     FAILED  esh-proc-test/kill-pipeline
> 
> Here's a patch for that. It works for me on GNU/Linux, and hopefully 
> it's ok on MS Windows too.

Thanks, and sorry for the thinko.  (I thought posting the patch would
have that caught, but I guess no one reads my patches seriously.)

bug#54136: 29.0.50; Eshell emits extra prompts when killing processes in some cases

[Jim Porter]

On 2/24/2022 11:09 PM, Eli Zaretskii wrote:
>> Here's a patch for that. It works for me on GNU/Linux, and hopefully
>> it's ok on MS Windows too.
> 
> Thanks, and sorry for the thinko.  (I thought posting the patch would
> have that caught, but I guess no one reads my patches seriously.)

Indeed, sorry about that. I spent an hour reading the Emacs's C sources 
and digging through Microsoft docs on subprocesses to try and understand 
why the exit message differed, but neglected the obvious step of "read 
the actual patch". D'oh.