Merge pull request #4035 from Mathjk/fix/aux-epipe

Fix panic on closed stdout in auxiliary output paths

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, see #4035 (@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)

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<T: serde::Serialize>(
     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(())
 }

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) {
+pub fn clear_assets(cache_dir: &Path) -> Result<()> {
-    clear_asset(cache_dir.join("themes.bin"), "theme set cache");
+    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("syntaxes.bin"), "syntax set cache")?;
-    clear_asset(cache_dir.join("metadata.yaml"), "metadata file");
+    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) {
+fn clear_asset(path: PathBuf, description: &str) -> Result<()> {
-    print!("Clearing {description} ... ");
+    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(())
 }

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(())
 }

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}");
+    write!(
-    io::stdout().flush().unwrap();
+        io::stdout(),
+        "{osc_command_for_setting_terminal_title}{safe_title}{osc_end_command}"
+    )?;
+    io::stdout().flush()?;
+    Ok(())
 }
 
 fn get_new_terminal_title(inputs: &Vec<Input>) -> String {
@@ -286,13 +290,13 @@ fn run_controller(inputs: Vec<Input>, 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::<Markdown>();
+    writeln!(io::stdout(), "{}", report.format::<Markdown>())?;
+    Ok(())
 }
 
 /// Returns `Err(..)` upon fatal errors. Otherwise, returns `Ok(true)` on full success and
@@ -382,21 +387,22 @@ fn run() -> Result<bool> {
 
     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::<String>("completion") {
-        match shell.as_str() {
+        let completion = match shell.as_str() {
-            "bash" => println!("{}", completions::BASH_COMPLETION),
+            "bash" => completions::BASH_COMPLETION,
-            "fish" => println!("{}", completions::FISH_COMPLETION),
+            "fish" => completions::FISH_COMPLETION,
-            "ps1" => println!("{}", completions::PS1_COMPLETION),
+            "ps1" => completions::PS1_COMPLETION,
-            "zsh" => println!("{}", completions::ZSH_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<bool> {
                 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()?;

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()