From a9b112b6fef8b63623e8b44d42a77220b0668e47 Mon Sep 17 00:00:00 2001 From: Mathjk <137137489+Mathjk@users.noreply.github.com> Date: Fri, 2 Oct 2026 02:23:08 +0800 Subject: [PATCH] Exit quietly on broken pipe in auxiliary output paths The main output path has exited with status 0 on EPIPE since #232, but auxiliary outputs still used println!/print!/io::stdout().flush().unwrap() and panicked (exit code 101) when the reader of a piped stdout went away early, e.g. 'bat --completion bash | head -n0'. Route them through the existing convention instead: write!/writeln! to io::stdout() and '?' so that Error::Io(BrokenPipe) reaches default_error_handler, which exits with status 0. This covers --completion, --config-file, --diagnostic (bugreport's print() becomes format() + writeln!), --generate-config-file, the 'cache' subcommand progress messages, the OSC title sequence of --set-terminal-title, and the 'built without feature' notices. Closes #4034 --- CHANGELOG.md | 1 + src/assets/build_assets.rs | 40 ++++++++++++++--------- src/bin/bat/assets.rs | 25 +++++++++------ src/bin/bat/config.rs | 12 ++++--- src/bin/bat/main.rs | 40 +++++++++++++---------- tests/integration_tests.rs | 65 ++++++++++++++++++++++++++++++++++++-- 6 files changed, 133 insertions(+), 50 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d5bcb2c..bfbafb3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ - Syntax highlighting for Python files using uv as script runner in shebang #3689 (@janlarres) ## Bugfixes +- Exit quietly with status 0 instead of panicking when stdout is closed early (broken pipe) on auxiliary output paths such as `--completion`, `--config-file`, `--diagnostic`, `--generate-config-file`, `--set-terminal-title` and `cache --build`/`--clear`, matching the main output path since #232. Closes #4034 (@Mathjk) - Track strikethrough (SGR 9 / 29) in the ANSI style tracker so it is re-emitted like bold, dim, italic, and underline, and is no longer dropped after the first highlighted token when displaying ANSI input with syntax highlighting, see #4015 (@mikehasa) - Allow boolean flags to be given more than once, so that a flag set in the config file can also be passed on the command line without erroring, see #3912 (@logarithmone1128) - Use parsed CLI arguments to detect number flags, see #3908 (@cuishuang) diff --git a/src/assets/build_assets.rs b/src/assets/build_assets.rs index 6d9c8e59..a05d3721 100644 --- a/src/assets/build_assets.rs +++ b/src/assets/build_assets.rs @@ -1,4 +1,5 @@ use std::convert::TryInto; +use std::io::{self, Write}; use std::path::Path; use syntect::highlighting::ThemeSet; @@ -24,7 +25,7 @@ pub fn build( let acknowledgements = build_acknowledgements(source_dir, include_acknowledgements)?; - print_unlinked_contexts(&syntax_set); + print_unlinked_contexts(&syntax_set)?; write_assets( &theme_set, @@ -46,16 +47,18 @@ fn build_theme_set(source_dir: &Path, include_integrated_assets: bool) -> Result if theme_dir.exists() { let res = theme_set.add_from_folder(&theme_dir); if let Err(err) = res { - println!( + writeln!( + io::stdout(), "Failed to load one or more themes from '{}' (reason: '{err}')", theme_dir.to_string_lossy(), - ); + )?; } } else { - println!( + writeln!( + io::stdout(), "No themes were found in '{}', using the default set", theme_dir.to_string_lossy() - ); + )?; } theme_set.try_into() @@ -78,23 +81,25 @@ fn build_syntax_set_builder( if syntax_dir.exists() { syntax_set_builder.add_from_folder(syntax_dir, true)?; } else { - println!( + writeln!( + io::stdout(), "No syntaxes were found in '{}', using the default set.", syntax_dir.to_string_lossy() - ); + )?; } Ok(syntax_set_builder) } -fn print_unlinked_contexts(syntax_set: &SyntaxSet) { +fn print_unlinked_contexts(syntax_set: &SyntaxSet) -> Result<()> { let missing_contexts = syntax_set.find_unlinked_contexts(); if !missing_contexts.is_empty() { - println!("Some referenced contexts could not be found!"); + writeln!(io::stdout(), "Some referenced contexts could not be found!")?; for context in missing_contexts { - println!("- {context}"); + writeln!(io::stdout(), "- {context}")?; } } + Ok(()) } fn write_assets( @@ -127,12 +132,13 @@ fn write_assets( )?; } - print!( + write!( + io::stdout(), "Writing metadata to folder {} ... ", target_dir.to_string_lossy() - ); + )?; crate::assets_metadata::AssetsMetadata::new(current_version).save_to_folder(target_dir)?; - println!("okay"); + writeln!(io::stdout(), "okay")?; Ok(()) } @@ -161,10 +167,14 @@ fn asset_to_cache( description: &str, compressed: bool, ) -> Result<()> { - print!("Writing {description} to {} ... ", path.to_string_lossy()); + write!( + io::stdout(), + "Writing {description} to {} ... ", + path.to_string_lossy() + )?; let contents = asset_to_contents(asset, description, compressed)?; std::fs::write(path, &contents[..]) .map_err(|_| format!("Could not save {description} to {}", path.to_string_lossy()))?; - println!("okay"); + writeln!(io::stdout(), "okay")?; Ok(()) } diff --git a/src/bin/bat/assets.rs b/src/bin/bat/assets.rs index 7f917088..94c858b5 100644 --- a/src/bin/bat/assets.rs +++ b/src/bin/bat/assets.rs @@ -1,5 +1,5 @@ use std::fs; -use std::io; +use std::io::{self, Write}; use std::path::Path; use std::path::PathBuf; @@ -9,10 +9,11 @@ use bat::assets::HighlightingAssets; use bat::assets_metadata::AssetsMetadata; use bat::error::*; -pub fn clear_assets(cache_dir: &Path) { - clear_asset(cache_dir.join("themes.bin"), "theme set cache"); - clear_asset(cache_dir.join("syntaxes.bin"), "syntax set cache"); - clear_asset(cache_dir.join("metadata.yaml"), "metadata file"); +pub fn clear_assets(cache_dir: &Path) -> Result<()> { + clear_asset(cache_dir.join("themes.bin"), "theme set cache")?; + clear_asset(cache_dir.join("syntaxes.bin"), "syntax set cache")?; + clear_asset(cache_dir.join("metadata.yaml"), "metadata file")?; + Ok(()) } pub fn assets_from_cache_or_binary( @@ -43,15 +44,19 @@ pub fn assets_from_cache_or_binary( Ok(custom_assets.unwrap_or_else(HighlightingAssets::from_binary)) } -fn clear_asset(path: PathBuf, description: &str) { - print!("Clearing {description} ... "); +fn clear_asset(path: PathBuf, description: &str) -> Result<()> { + write!(io::stdout(), "Clearing {description} ... ")?; match fs::remove_file(&path) { Err(err) if err.kind() == io::ErrorKind::NotFound => { - println!("skipped (not present)"); + writeln!(io::stdout(), "skipped (not present)")?; } Err(err) => { - println!("could not remove the cache file {path:?}: {err}"); + writeln!( + io::stdout(), + "could not remove the cache file {path:?}: {err}" + )?; } - Ok(_) => println!("okay"), + Ok(_) => writeln!(io::stdout(), "okay")?, } + Ok(()) } diff --git a/src/bin/bat/config.rs b/src/bin/bat/config.rs index 3e2fb72e..863aa3d7 100644 --- a/src/bin/bat/config.rs +++ b/src/bin/bat/config.rs @@ -32,12 +32,13 @@ pub fn config_file() -> PathBuf { pub fn generate_config_file() -> bat::error::Result<()> { let config_file = config_file(); if config_file.is_file() { - println!( + writeln!( + io::stdout(), "A config file already exists at: {}", config_file.to_string_lossy() - ); + )?; - print!("Overwrite? (y/N): "); + write!(io::stdout(), "Overwrite? (y/N): ")?; io::stdout().flush()?; let mut decision = String::new(); io::stdin().read_line(&mut decision)?; @@ -93,10 +94,11 @@ pub fn generate_config_file() -> bat::error::Result<()> { ) })?; - println!( + writeln!( + io::stdout(), "Success! Config file written to {}", config_file.to_string_lossy() - ); + )?; Ok(()) } diff --git a/src/bin/bat/main.rs b/src/bin/bat/main.rs index 705a28b9..359219be 100644 --- a/src/bin/bat/main.rs +++ b/src/bin/bat/main.rs @@ -74,9 +74,9 @@ fn run_cache_subcommand( #[cfg(feature = "build-assets")] build_assets(matches, config_dir, cache_dir)?; #[cfg(not(feature = "build-assets"))] - println!("bat has been built without the 'build-assets' feature. The 'cache --build' option is not available."); + writeln!(io::stdout(), "bat has been built without the 'build-assets' feature. The 'cache --build' option is not available.")?; } else if matches.get_flag("clear") { - clear_assets(cache_dir); + clear_assets(cache_dir)?; } Ok(()) @@ -262,13 +262,17 @@ pub fn list_themes( Ok(()) } -fn set_terminal_title_to(new_terminal_title: String) { +fn set_terminal_title_to(new_terminal_title: String) -> Result<()> { let osc_command_for_setting_terminal_title = "\x1b]0;"; let osc_end_command = "\x07"; // Prevent BEL/ESC/C1 bytes in the title from terminating or nesting the OSC. let safe_title = bat::sanitize_for_terminal(&new_terminal_title); - print!("{osc_command_for_setting_terminal_title}{safe_title}{osc_end_command}"); - io::stdout().flush().unwrap(); + write!( + io::stdout(), + "{osc_command_for_setting_terminal_title}{safe_title}{osc_end_command}" + )?; + io::stdout().flush()?; + Ok(()) } fn get_new_terminal_title(inputs: &Vec) -> String { @@ -286,13 +290,13 @@ fn run_controller(inputs: Vec, config: &Config, cache_dir: &Path) -> Resu let assets = assets_from_cache_or_binary(config.use_custom_assets, cache_dir)?; let controller = Controller::new(config, &assets); if config.paging_mode != PagingMode::Never && config.set_terminal_title { - set_terminal_title_to(get_new_terminal_title(&inputs)); + set_terminal_title_to(get_new_terminal_title(&inputs))?; } controller.run(inputs, None) } #[cfg(feature = "bugreport")] -fn invoke_bugreport(app: &App, cache_dir: &Path) { +fn invoke_bugreport(app: &App, cache_dir: &Path) -> Result<()> { use bugreport::{bugreport, collector::*, format::Markdown, report::ReportEntry}; struct ColorSchemeCollector; @@ -370,7 +374,8 @@ fn invoke_bugreport(app: &App, cache_dir: &Path) { )) }; - report.print::(); + writeln!(io::stdout(), "{}", report.format::())?; + Ok(()) } /// Returns `Err(..)` upon fatal errors. Otherwise, returns `Ok(true)` on full success and @@ -382,21 +387,22 @@ fn run() -> Result { if app.matches.get_flag("diagnostic") { #[cfg(feature = "bugreport")] - invoke_bugreport(&app, cache_dir); + invoke_bugreport(&app, cache_dir)?; #[cfg(not(feature = "bugreport"))] - println!("bat has been built without the 'bugreport' feature. The '--diagnostic' option is not available."); + writeln!(io::stdout(), "bat has been built without the 'bugreport' feature. The '--diagnostic' option is not available.")?; return Ok(true); } #[cfg(feature = "application")] if let Some(shell) = app.matches.get_one::("completion") { - match shell.as_str() { - "bash" => println!("{}", completions::BASH_COMPLETION), - "fish" => println!("{}", completions::FISH_COMPLETION), - "ps1" => println!("{}", completions::PS1_COMPLETION), - "zsh" => println!("{}", completions::ZSH_COMPLETION), + let completion = match shell.as_str() { + "bash" => completions::BASH_COMPLETION, + "fish" => completions::FISH_COMPLETION, + "ps1" => completions::PS1_COMPLETION, + "zsh" => completions::ZSH_COMPLETION, _ => unreachable!("No completion for shell '{shell}' available."), - } + }; + writeln!(io::stdout(), "{completion}")?; return Ok(true); } @@ -435,7 +441,7 @@ fn run() -> Result { list_themes(&config, config_dir, cache_dir, app.theme_options())?; Ok(true) } else if app.matches.get_flag("config-file") { - println!("{}", config_file().to_string_lossy()); + writeln!(io::stdout(), "{}", config_file().to_string_lossy())?; Ok(true) } else if app.matches.get_flag("generate-config-file") { generate_config_file()?; diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 5d682c8c..9463be37 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -25,10 +25,8 @@ mod unix { use unix::*; mod utils; -use utils::command::{bat, bat_with_config}; +use utils::command::{bat, bat_raw_command, bat_raw_command_with_config, bat_with_config}; -#[cfg(unix)] -use utils::command::bat_raw_command; use utils::mocked_pagers; const EXAMPLES_DIR: &str = "tests/examples"; @@ -60,6 +58,67 @@ fn stdin() { .stdout("foo\nbar\n"); } +/// Runs `bat` with `args` while its stdout is a pipe whose read end has +/// already been closed, so the very first write fails with `BrokenPipe` +/// (EPIPE on Unix, `ERROR_NO_DATA` on Windows). +fn assert_quiet_exit_when_stdout_is_closed(command: &mut std::process::Command, args: &[&str]) { + use std::process::Stdio; + + let (pipe_reader, pipe_writer) = std::io::pipe().expect("could not create a pipe"); + drop(pipe_reader); + + let output = command + .args(args) + .stdin(Stdio::null()) + .stdout(pipe_writer) + .stderr(Stdio::piped()) + .spawn() + .and_then(|child| child.wait_with_output()) + .expect("could not run bat"); + + assert!( + output.status.success(), + "bat {args:?} should exit quietly when stdout is closed, status: {}, stderr: {}", + output.status, + String::from_utf8_lossy(&output.stderr), + ); + assert!( + output.stderr.is_empty(), + "bat {args:?} should not print to stderr when stdout is closed, stderr: {}", + String::from_utf8_lossy(&output.stderr), + ); +} + +#[test] +fn auxiliary_output_does_not_panic_on_closed_stdout() { + // Auxiliary outputs used to be written via `println!`, which panics when + // the reader of a piped stdout is already gone (e.g. `bat --config-file | + // head -n 0`). The main output path has exited quietly on `BrokenPipe` + // since #232; auxiliary outputs should behave the same way. + assert_quiet_exit_when_stdout_is_closed(&mut bat_raw_command(), &["--config-file"]); + assert_quiet_exit_when_stdout_is_closed(&mut bat_raw_command(), &["--diagnostic"]); + + // The `cache` subcommand is only enabled when the working directory does + // not contain a file named `cache` (see `cache_clear`), and it conflicts + // with top-level flags like `--no-config`. Run it from a temporary + // directory that doubles as `BAT_CACHE_PATH`, leaving the real cache alone. + let temp_dir = tempdir().expect("could not create a temporary directory"); + let mut command = bat_raw_command_with_config(); + command + .current_dir(temp_dir.path()) + .env("BAT_CONFIG_PATH", temp_dir.path().join("config")) + .env("BAT_CACHE_PATH", temp_dir.path()); + assert_quiet_exit_when_stdout_is_closed(&mut command, &["cache", "--clear"]); +} + +#[cfg(feature = "application")] +#[test] +fn completion_does_not_panic_on_closed_stdout() { + for shell in ["bash", "fish", "ps1", "zsh"] { + assert_quiet_exit_when_stdout_is_closed(&mut bat_raw_command(), &["--completion", shell]); + } +} + #[test] fn concatenate() { bat()