* bug#72018: 30.0.60; [PATCH] Don't emit a prompt when a background Eshell process is killed
@ 2024-07-09 18:04 Jim Porter
2024-07-10 11:16 ` Eli Zaretskii
0 siblings, 1 reply; 7+ messages in thread
From: Jim Porter @ 2024-07-09 18:04 UTC (permalink / raw)
To: 72018
[-- Attachment #1: Type: text/plain, Size: 442 bytes --]
Steps to reproduce:
emacs -Q -f eshell
$ sleep 100 &
$ kill -9 <sleep's process id>
After this, you'll see two Eshell prompts get emitted (one with a "[9]"
for the exit status of "sleep"). We don't want to emit prompts when
background processes die though.
This is a regression from Emacs 29, likely due to some changes I made to
improve support for complex background commands. Eli, is this ok to
merge to the release branch?
[-- Attachment #2: 0001-Don-t-emit-a-prompt-in-Eshell-when-a-background-comm.patch --]
[-- Type: text/plain, Size: 2785 bytes --]
From 3e5a702f0136cc6a071546a854aea54536c87783 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 9 Jul 2024 10:45:35 -0700
Subject: [PATCH] Don't emit a prompt in Eshell when a background command is
killed
* lisp/eshell/esh-cmd.el (eshell-resume-command): Check for
background-ness before resetting the prompt.
* test/lisp/eshell/esh-cmd-tests.el
(esh-cmd-test/background/simple-command): Make the regexp a bit
stricter.
(esh-cmd-test/background/kill): New test.
---
lisp/eshell/esh-cmd.el | 7 +++++--
test/lisp/eshell/esh-cmd-tests.el | 14 +++++++++++++-
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 0b3137127d2..d0de6e55ce6 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -1040,8 +1040,11 @@ eshell-resume-command
(not (string-match eshell-reset-signals status)))
(eshell-resume-eval command)
(eshell-remove-command command)
- (declare-function eshell-reset "esh-mode" (&optional no-hooks))
- (eshell-reset))))))
+ ;; Reset the prompt if the command we just aborted was in the
+ ;; foreground.
+ (unless (car command)
+ (declare-function eshell-reset "esh-mode" (&optional no-hooks))
+ (eshell-reset)))))))
(defun eshell-resume-eval (command)
"Destructively evaluate a COMMAND which may need to be deferred.
diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el
index 70e1901c169..d8124a19af6 100644
--- a/test/lisp/eshell/esh-cmd-tests.el
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -113,7 +113,7 @@ esh-cmd-test/background/simple-command
(with-temp-eshell
(eshell-match-command-output
(format "*echo hi > #<%s> &" bufname)
- (rx "[echo" (? ".exe") "] " (+ digit) "\n"))
+ (rx bos "[echo" (? ".exe") "] " (+ digit) "\n"))
(eshell-wait-for-subprocess t))
(should (equal (buffer-string) "hi\n"))))
@@ -129,6 +129,18 @@ esh-cmd-test/background/subcommand
(eshell-wait-for-subprocess t))
(should (equal (buffer-string) "olleh\n"))))
+(ert-deftest esh-cmd-test/background/kill ()
+ "Make sure that a background command that gets killed doesn't emit a prompt."
+ (skip-unless (executable-find "sleep"))
+ (let ((background-message (rx bos "[sleep" (? ".exe") "] " (+ digit) "\n")))
+ (with-temp-eshell
+ (eshell-match-command-output "*sleep 10 &" background-message)
+ (kill-process (caar eshell-process-list))
+ (eshell-wait-for-subprocess t)
+ ;; Ensure we didn't emit another prompt after killing the
+ ;; background process.
+ (should (eshell-match-output background-message)))))
+
\f
;; Lisp forms
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* bug#72018: 30.0.60; [PATCH] Don't emit a prompt when a background Eshell process is killed
2024-07-09 18:04 bug#72018: 30.0.60; [PATCH] Don't emit a prompt when a background Eshell process is killed Jim Porter
@ 2024-07-10 11:16 ` Eli Zaretskii
2024-07-10 16:16 ` Jim Porter
0 siblings, 1 reply; 7+ messages in thread
From: Eli Zaretskii @ 2024-07-10 11:16 UTC (permalink / raw)
To: Jim Porter; +Cc: 72018
> Date: Tue, 9 Jul 2024 11:04:05 -0700
> From: Jim Porter <jporterbugs@gmail.com>
>
> This is a regression from Emacs 29, likely due to some changes I made to
> improve support for complex background commands. Eli, is this ok to
> merge to the release branch?
I don't think I understand the essence of the change, and thus cannot
appreciate its effects enough to be able to answer this. What is the
significance of '(car command)' in this hunk:
> + ;; Reset the prompt if the command we just aborted was in the
> + ;; foreground.
> + (unless (car command)
> + (declare-function eshell-reset "esh-mode" (&optional no-hooks))
> + (eshell-reset)))))))
IOW, why '(car command)' is used as an indication of a fore/background
command? Also, why does the comment say "foreground" while your text
says we don't want the prompt if the killed program was in the
background?
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#72018: 30.0.60; [PATCH] Don't emit a prompt when a background Eshell process is killed
2024-07-10 11:16 ` Eli Zaretskii
@ 2024-07-10 16:16 ` Jim Porter
2024-07-10 17:34 ` Eli Zaretskii
0 siblings, 1 reply; 7+ messages in thread
From: Jim Porter @ 2024-07-10 16:16 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 72018
On 7/10/2024 4:16 AM, Eli Zaretskii wrote:
>> Date: Tue, 9 Jul 2024 11:04:05 -0700
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> This is a regression from Emacs 29, likely due to some changes I made to
>> improve support for complex background commands. Eli, is this ok to
>> merge to the release branch?
>
> I don't think I understand the essence of the change, and thus cannot
> appreciate its effects enough to be able to answer this. What is the
> significance of '(car command)' in this hunk:
'command' is a "command entry", and the result of
'eshell-commands-for-process', which returns a list of elements of the form:
(BACKGROUND FORM PROCESSES)
BACKGROUND is non-nil if the command is being run in the background.
>> + ;; Reset the prompt if the command we just aborted was in the
>> + ;; foreground.
>> + (unless (car command)
>> + (declare-function eshell-reset "esh-mode" (&optional no-hooks))
>> + (eshell-reset)))))))
>
> IOW, why '(car command)' is used as an indication of a fore/background
> command? Also, why does the comment say "foreground" while your text
> says we don't want the prompt if the killed program was in the
> background?
We want to reset the prompt (this just emits a new command prompt) for
foreground commands, but for background commands, we don't need to do
anything. Would it be clearer if I inverted the wording in the comment,
like, "Don't reset the prompt if the command we just aborted was in the
background"?
^ permalink raw reply [flat|nested] 7+ messages in thread* bug#72018: 30.0.60; [PATCH] Don't emit a prompt when a background Eshell process is killed
2024-07-10 16:16 ` Jim Porter
@ 2024-07-10 17:34 ` Eli Zaretskii
2024-07-10 19:55 ` Jim Porter
0 siblings, 1 reply; 7+ messages in thread
From: Eli Zaretskii @ 2024-07-10 17:34 UTC (permalink / raw)
To: Jim Porter; +Cc: 72018
> Date: Wed, 10 Jul 2024 09:16:11 -0700
> Cc: 72018@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
>
> On 7/10/2024 4:16 AM, Eli Zaretskii wrote:
> >
> > I don't think I understand the essence of the change, and thus cannot
> > appreciate its effects enough to be able to answer this. What is the
> > significance of '(car command)' in this hunk:
>
> 'command' is a "command entry", and the result of
> 'eshell-commands-for-process', which returns a list of elements of the form:
>
> (BACKGROUND FORM PROCESSES)
>
> BACKGROUND is non-nil if the command is being run in the background.
>
> >> + ;; Reset the prompt if the command we just aborted was in the
> >> + ;; foreground.
> >> + (unless (car command)
> >> + (declare-function eshell-reset "esh-mode" (&optional no-hooks))
> >> + (eshell-reset)))))))
> >
> > IOW, why '(car command)' is used as an indication of a fore/background
> > command? Also, why does the comment say "foreground" while your text
> > says we don't want the prompt if the killed program was in the
> > background?
>
> We want to reset the prompt (this just emits a new command prompt) for
> foreground commands, but for background commands, we don't need to do
> anything. Would it be clearer if I inverted the wording in the comment,
> like, "Don't reset the prompt if the command we just aborted was in the
> background"?
I think these subtleties just warrant more detailed comments, and then
we'll be fine.
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#72018: 30.0.60; [PATCH] Don't emit a prompt when a background Eshell process is killed
2024-07-10 17:34 ` Eli Zaretskii
@ 2024-07-10 19:55 ` Jim Porter
2024-07-11 4:34 ` Eli Zaretskii
0 siblings, 1 reply; 7+ messages in thread
From: Jim Porter @ 2024-07-10 19:55 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 72018
[-- Attachment #1: Type: text/plain, Size: 583 bytes --]
On 7/10/2024 10:34 AM, Eli Zaretskii wrote:
>> Date: Wed, 10 Jul 2024 09:16:11 -0700
>> Cc: 72018@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> We want to reset the prompt (this just emits a new command prompt) for
>> foreground commands, but for background commands, we don't need to do
>> anything. Would it be clearer if I inverted the wording in the comment,
>> like, "Don't reset the prompt if the command we just aborted was in the
>> background"?
>
> I think these subtleties just warrant more detailed comments, and then
> we'll be fine.
How about this?
[-- Attachment #2: 0001-Don-t-emit-a-prompt-in-Eshell-when-a-background-comm.patch --]
[-- Type: text/plain, Size: 3394 bytes --]
From 84fb997a94503e76a65a5b96301d9ff2baac1fc3 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 9 Jul 2024 10:45:35 -0700
Subject: [PATCH] Don't emit a prompt in Eshell when a background command is
killed
* lisp/eshell/esh-cmd.el (eshell-resume-command): Check for
background-ness before resetting the prompt.
* test/lisp/eshell/esh-cmd-tests.el
(esh-cmd-test/background/simple-command): Make the regexp a bit
stricter.
(esh-cmd-test/background/kill): New test.
---
lisp/eshell/esh-cmd.el | 11 +++++++++--
test/lisp/eshell/esh-cmd-tests.el | 14 +++++++++++++-
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 0b3137127d2..e97e4f6d067 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -1030,6 +1030,9 @@ eshell-resume-command
PROC is the process that invoked this from its sentinel, and
STATUS is its status."
(when proc
+ ;; Iterate over all the commands associated with this process. Each
+ ;; element is a list of the form (BACKGROUND FORM PROCESSES) (see
+ ;; `eshell-add-command').
(dolist (command (eshell-commands-for-process proc))
(unless (seq-some #'eshell-process-active-p (nth 2 command))
(setf (nth 2 command) nil) ; Clear processes from command.
@@ -1040,8 +1043,12 @@ eshell-resume-command
(not (string-match eshell-reset-signals status)))
(eshell-resume-eval command)
(eshell-remove-command command)
- (declare-function eshell-reset "esh-mode" (&optional no-hooks))
- (eshell-reset))))))
+ ;; Check if the command we just aborted is marked as a
+ ;; background command. If not, we need to reset the prompt so
+ ;; the user can enter another command.
+ (unless (car command)
+ (declare-function eshell-reset "esh-mode" (&optional no-hooks))
+ (eshell-reset)))))))
(defun eshell-resume-eval (command)
"Destructively evaluate a COMMAND which may need to be deferred.
diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el
index 70e1901c169..d8124a19af6 100644
--- a/test/lisp/eshell/esh-cmd-tests.el
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -113,7 +113,7 @@ esh-cmd-test/background/simple-command
(with-temp-eshell
(eshell-match-command-output
(format "*echo hi > #<%s> &" bufname)
- (rx "[echo" (? ".exe") "] " (+ digit) "\n"))
+ (rx bos "[echo" (? ".exe") "] " (+ digit) "\n"))
(eshell-wait-for-subprocess t))
(should (equal (buffer-string) "hi\n"))))
@@ -129,6 +129,18 @@ esh-cmd-test/background/subcommand
(eshell-wait-for-subprocess t))
(should (equal (buffer-string) "olleh\n"))))
+(ert-deftest esh-cmd-test/background/kill ()
+ "Make sure that a background command that gets killed doesn't emit a prompt."
+ (skip-unless (executable-find "sleep"))
+ (let ((background-message (rx bos "[sleep" (? ".exe") "] " (+ digit) "\n")))
+ (with-temp-eshell
+ (eshell-match-command-output "*sleep 10 &" background-message)
+ (kill-process (caar eshell-process-list))
+ (eshell-wait-for-subprocess t)
+ ;; Ensure we didn't emit another prompt after killing the
+ ;; background process.
+ (should (eshell-match-output background-message)))))
+
\f
;; Lisp forms
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* bug#72018: 30.0.60; [PATCH] Don't emit a prompt when a background Eshell process is killed
2024-07-10 19:55 ` Jim Porter
@ 2024-07-11 4:34 ` Eli Zaretskii
2024-07-11 23:44 ` Jim Porter
0 siblings, 1 reply; 7+ messages in thread
From: Eli Zaretskii @ 2024-07-11 4:34 UTC (permalink / raw)
To: Jim Porter; +Cc: 72018
> Date: Wed, 10 Jul 2024 12:55:25 -0700
> Cc: 72018@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
>
> On 7/10/2024 10:34 AM, Eli Zaretskii wrote:
> >> Date: Wed, 10 Jul 2024 09:16:11 -0700
> >> Cc: 72018@debbugs.gnu.org
> >> From: Jim Porter <jporterbugs@gmail.com>
> >>
> >> We want to reset the prompt (this just emits a new command prompt) for
> >> foreground commands, but for background commands, we don't need to do
> >> anything. Would it be clearer if I inverted the wording in the comment,
> >> like, "Don't reset the prompt if the command we just aborted was in the
> >> background"?
> >
> > I think these subtleties just warrant more detailed comments, and then
> > we'll be fine.
>
> How about this?
LGTM, thanks.
This is okay for emacs-30.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-07-11 23:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-09 18:04 bug#72018: 30.0.60; [PATCH] Don't emit a prompt when a background Eshell process is killed Jim Porter
2024-07-10 11:16 ` Eli Zaretskii
2024-07-10 16:16 ` Jim Porter
2024-07-10 17:34 ` Eli Zaretskii
2024-07-10 19:55 ` Jim Porter
2024-07-11 4:34 ` Eli Zaretskii
2024-07-11 23:44 ` Jim Porter
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).