Skip to content

fix: prevent Ctrl+C from killing bat when paging without -K #3447

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.

Sign up for GitHub

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

Open
lmmx wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: prevent Ctrl+C from killing bat when paging without -K #3447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
## Bugfixes

- Fix negative values of N not being parsed in <N:M> line ranges without `=` flag value separator, see #3442 (@lmmx)
- Ignore SIGINT during paging to fix bug where less exited without `-K` arg, see issue #3444 and PR #3447 (@lmmx)

## Other
- Improve README documentation on pager options passed to less, see #3443 (@injust)
Expand Down
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ execute = { version = "0.2.13", optional = true }
terminal-colorsaurus = "1.0"
unicode-segmentation = "1.12.0"
itertools = "0.14.0"
signal-hook = "0.3.18"

[dependencies.git2]
version = "0.20"
Expand Down
49 changes: 42 additions & 7 deletions src/output.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,36 @@ use crate::paging::PagingMode;
#[cfg(feature = "paging")]
use crate::wrapping::WrappingMode;

#[cfg(all(unix, feature = "paging"))]
use std::sync::atomic::AtomicBool;
#[cfg(all(unix, feature = "paging"))]
use std::sync::Arc;
#[cfg(all(unix, feature = "paging"))]
struct IgnoreSigint {
_handle: signal_hook::SigId,
}

#[cfg(all(unix, feature = "paging"))]
impl IgnoreSigint {
fn new() -> Self {
// No-op SIGINT handler prevents pager process termination
let handle = signal_hook::flag::register(
signal_hook::consts::SIGINT,
Arc::new(AtomicBool::new(false)),
)
.expect("failed to ignore SIGINT");

Self { _handle: handle }
}
}

#[cfg(feature = "paging")]
pub struct PagerProc {
child: Child,
#[cfg(unix)]
_sigint_guard: IgnoreSigint,
}

#[cfg(feature = "paging")]
pub struct BuiltinPager {
pager: minus::Pager,
Expand Down Expand Up @@ -50,10 +80,9 @@ enum SingleScreenAction {
Nothing,
}

#[derive(Debug)]
pub enum OutputType {
#[cfg(feature = "paging")]
Pager(Child),
Pager(PagerProc),
Copy link
Collaborator

@keith-hall keith-hall Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this changes the public API (for bat as a library), so to be semver compatible, the next release including this cannot be a patch release

Copy link
Contributor Author

@lmmx lmmx Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, ok. Let me know if I can alleviate any of the friction to do with that. Also: if it’s not something you’re wholly behind don’t feel obliged to merge it.

I feel like it solves the bug in terms of UX but not correct way (i.e. bat stays alive). Whether there are implications to bat staying alive I’m not quite sure though. Unlike most programs bat isn’t going to be producing a stream of data (if I follow it correctly?)

I am not fully certain how to think about the case of bat reading a large file into a pager, I wouldn’t imagine that to be a runaway process you might want to Ctrl+C to stop and then stay in the pager to read the partial result of. But I might be wrong.

If that view is right though, bat living on and just ignoring SIGINT seems “basically fine”

#[cfg(feature = "paging")]
BuiltinPager(BuiltinPager),
Stdout(io::Stdout),
Expand Down Expand Up @@ -166,7 +195,13 @@ impl OutputType {

Ok(p.stdin(Stdio::piped())
.spawn()
.map(OutputType::Pager)
.map(|child| {
OutputType::Pager(PagerProc {
child,
#[cfg(unix)]
_sigint_guard: IgnoreSigint::new(),
})
})
.unwrap_or_else(|_| OutputType::stdout()))
}

Expand All @@ -187,8 +222,8 @@ impl OutputType {
pub fn handle<'a>(&'a mut self) -> Result<OutputHandle<'a>> {
Ok(match *self {
#[cfg(feature = "paging")]
OutputType::Pager(ref mut command) => OutputHandle::IoWrite(
command
OutputType::Pager(ref mut proc) => OutputHandle::IoWrite(
proc.child
.stdin
.as_mut()
.ok_or("Could not open stdin for pager")?,
Expand All @@ -204,8 +239,8 @@ impl OutputType {
impl Drop for OutputType {
fn drop(&mut self) {
match *self {
OutputType::Pager(ref mut command) => {
let _ = command.wait();
OutputType::Pager(ref mut proc) => {
let _ = proc.child.wait();
}
OutputType::BuiltinPager(ref mut pager) => {
if pager.handle.is_some() {
Expand Down