GNU bug report logs - #72018
30.0.60; [PATCH] Don't emit a prompt when a background Eshell process is killed

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 72018 in the body.
You can then email your comments to 72018 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: 30.0.60; [PATCH] Don't emit a prompt when a background Eshell process
 is killed
Date: Tue, 9 Jul 2024 11:04:05 -0700

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

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?

[0001-Don-t-emit-a-prompt-in-Eshell-when-a-background-comm.patch (text/plain, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 72018 <at> debbugs.gnu.org
Subject: Re: bug#72018: 30.0.60;
 [PATCH] Don't emit a prompt when a background Eshell process is killed
Date: Wed, 10 Jul 2024 14:16:05 +0300

> Date: Tue, 9 Jul 2024 11:04:05 -0700
> From: Jim Porter <jporterbugs <at> 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?

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72018 <at> debbugs.gnu.org
Subject: Re: bug#72018: 30.0.60; [PATCH] Don't emit a prompt when a background
 Eshell process is killed
Date: Wed, 10 Jul 2024 09:16:11 -0700

On 7/10/2024 4:16 AM, Eli Zaretskii wrote:
>> Date: Tue, 9 Jul 2024 11:04:05 -0700
>> From: Jim Porter <jporterbugs <at> 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"?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 72018 <at> debbugs.gnu.org
Subject: Re: bug#72018: 30.0.60; [PATCH] Don't emit a prompt when a background
 Eshell process is killed
Date: Wed, 10 Jul 2024 20:34:57 +0300

> Date: Wed, 10 Jul 2024 09:16:11 -0700
> Cc: 72018 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> 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.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72018 <at> debbugs.gnu.org
Subject: Re: bug#72018: 30.0.60; [PATCH] Don't emit a prompt when a background
 Eshell process is killed
Date: Wed, 10 Jul 2024 12:55:25 -0700

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

On 7/10/2024 10:34 AM, Eli Zaretskii wrote:
>> Date: Wed, 10 Jul 2024 09:16:11 -0700
>> Cc: 72018 <at> debbugs.gnu.org
>> From: Jim Porter <jporterbugs <at> 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?

[0001-Don-t-emit-a-prompt-in-Eshell-when-a-background-comm.patch (text/plain, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 72018 <at> debbugs.gnu.org
Subject: Re: bug#72018: 30.0.60; [PATCH] Don't emit a prompt when a background
 Eshell process is killed
Date: Thu, 11 Jul 2024 07:34:16 +0300

> Date: Wed, 10 Jul 2024 12:55:25 -0700
> Cc: 72018 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> On 7/10/2024 10:34 AM, Eli Zaretskii wrote:
> >> Date: Wed, 10 Jul 2024 09:16:11 -0700
> >> Cc: 72018 <at> debbugs.gnu.org
> >> From: Jim Porter <jporterbugs <at> 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.

Message #25 received at 72018-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72018-done <at> debbugs.gnu.org
Subject: Re: bug#72018: 30.0.60; [PATCH] Don't emit a prompt when a background
 Eshell process is killed
Date: Thu, 11 Jul 2024 16:44:05 -0700

On 7/10/2024 9:34 PM, Eli Zaretskii wrote:
> LGTM, thanks.
> 
> This is okay for emacs-30.

Thanks. Pushed to the release branch as 0de0056fd6b, and closing this bug.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.