-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(exit): suspended jobs warning and sending kill signals #287
base: main
Are you sure you want to change the base?
Changes from all commits
b3f8044
d6ee07f
c03362b
bad26a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
use clap::Parser; | ||
|
||
use std::io::Write; | ||
|
||
use crate::{builtins, commands}; | ||
|
||
/// Exit the shell. | ||
|
@@ -14,6 +16,18 @@ impl builtins::Command for ExitCommand { | |
&self, | ||
context: commands::ExecutionContext<'_>, | ||
) -> Result<crate::builtins::ExitCode, crate::error::Error> { | ||
if !context.shell.jobs.jobs.is_empty() && context.shell.user_tried_exiting == 0 { | ||
writeln!(context.stdout(), "brush: You have suspended jobs.")?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor code note: would be nice to avoid hard-coding the display name of the shell. If I recall correctly |
||
|
||
context.shell.user_tried_exiting = 2; // get's decreased this input too so next input will be 1 | ||
|
||
return Ok(builtins::ExitCode::Custom(1)) | ||
} | ||
|
||
for job in &mut context.shell.jobs.jobs { | ||
job.kill(Some(nix::sys::signal::SIGHUP))?; | ||
} | ||
|
||
let code_8bit: u8; | ||
|
||
#[allow(clippy::cast_sign_loss)] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -408,9 +408,9 @@ impl Job { | |
} | ||
|
||
/// Kills the job. | ||
pub fn kill(&mut self) -> Result<(), error::Error> { | ||
pub fn kill(&mut self, signal: Option<nix::sys::signal::Signal>) -> Result<(), error::Error> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may run into challenges exposing a |
||
if let Some(pid) = self.get_process_group_id() { | ||
sys::signal::kill_process(pid) | ||
sys::signal::kill_process(pid, signal) | ||
} else { | ||
Err(error::Error::FailedToSendSignal) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,9 @@ pub struct Shell { | |
// Additional state | ||
/// The status of the last completed command. | ||
pub last_exit_status: u8, | ||
|
||
/// User tried exiting in the previous/current input if != 0. | ||
pub user_tried_exiting: u8, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason for this to be an integer instead of a simple bool flag? Are there cases in which you think we'd need to track how many times the user tried exiting? |
||
|
||
/// Clone depth from the original ancestor shell. | ||
pub depth: usize, | ||
|
@@ -100,6 +103,7 @@ impl Clone for Shell { | |
builtins: self.builtins.clone(), | ||
program_location_cache: self.program_location_cache.clone(), | ||
depth: self.depth + 1, | ||
user_tried_exiting: self.user_tried_exiting | ||
} | ||
} | ||
} | ||
|
@@ -191,6 +195,7 @@ impl Shell { | |
builtins: builtins::get_default_builtins(options), | ||
program_location_cache: pathcache::PathCache::default(), | ||
depth: 0, | ||
user_tried_exiting: 0 | ||
}; | ||
|
||
// TODO: Without this a script that sets extglob will fail because we | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, a
Ctrl+D
based exit won't actually invoke theexit
built-in. Do we need to make an additional change to make sure that path also works in the same way?