Merge pull request #3457 from abhinavcool42/fix/list-themes-pager-hang

fix: `--list-themes` pagination

Author: keith-hall

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ## Bugfixes
 
+- Fix hang when using `--list-themes` with an explicit pager, see #3457 (@abhinavcool42)
 - Fix negative values of N not being parsed in <N:M> line ranges without `=` flag value separator, see #3442 (@lmmx)
 
 ## Other

src/bin/bat/main.rs

@@ -16,7 +16,7 @@ use std::io::{BufReader, Write};
 use std::path::Path;
 use std::process;
 
-use bat::output::OutputType;
+use bat::output::{OutputHandle, OutputType};
 use bat::theme::DetectColorScheme;
 use nu_ansi_term::Color::Green;
 use nu_ansi_term::Style;
@@ -206,11 +206,10 @@ pub fn list_themes(
     config.language = Some("Rust");
     config.style_components = StyleComponents(style);
 
-    let mut output_type =
-        OutputType::from_mode(config.paging_mode, config.wrapping_mode, config.pager)?;
-    let mut writer = output_type.handle()?;
-
     let default_theme_name = default_theme(color_scheme(detect_color_scheme).unwrap_or_default());
+    let mut buf = String::new();
+    let mut handle = OutputHandle::FmtWrite(&mut buf);
+
     for theme in assets.themes() {
         let default_theme_info = if default_theme_name == theme {
             " (default)"
@@ -221,35 +220,39 @@ pub fn list_themes(
         } else {
             ""
         };
+
         if config.colored_output {
-            writeln!(
-                writer,
-                "Theme: {}{default_theme_info}\n",
+            handle.write_fmt(format_args!(
+                "{}{default_theme_info}\n\n",
                 Style::new().bold().paint(theme.to_string()),
-            )?;
+            ))?;
             config.theme = theme.to_string();
             Controller::new(&config, &assets)
-                .run(vec![theme_preview_file()], Some(&mut writer))
+                .run(vec![theme_preview_file()], Some(&mut handle))
                 .ok();
-            writeln!(writer)?;
+            handle.write_fmt(format_args!("\n"))?;
         } else if config.loop_through {
-            writeln!(writer, "{theme}")?;
+            handle.write_fmt(format_args!("{theme}\n"))?;
         } else {
-            writeln!(writer, "{theme}{default_theme_info}")?;
+            handle.write_fmt(format_args!("{theme}{default_theme_info}\n"))?;
         }
     }
 
     if config.colored_output {
-        writeln!(
-            writer,
+        handle.write_fmt(format_args!(
             "Further themes can be installed to '{}', \
             and are added to the cache with `bat cache --build`. \
             For more information, see:\n\n  \
             https://github.com/sharkdp/bat#adding-new-themes",
             config_dir.join("themes").to_string_lossy()
-        )?;
+        ))?;
     }
 
+    let mut output_type =
+        OutputType::from_mode(config.paging_mode, config.wrapping_mode, config.pager)?;
+    let mut writer = output_type.handle()?;
+    writer.write_fmt(format_args!("{buf}"))?;
+
     Ok(())
 }
 

src/controller.rs

@@ -50,10 +50,12 @@ impl Controller<'_> {
         output_handle: Option<&mut OutputHandle<'_>>,
         mut handle_error: impl FnMut(&Error, &mut dyn Write),
     ) -> Result<bool> {
-        let mut output_type;
+        // only create our own OutputType if no output handle was provided.
+        #[allow(unused_mut)]
+        let mut output_type_opt: Option<OutputType> = None;
 
         #[cfg(feature = "paging")]
-        {
+        if output_handle.is_none() {
             use crate::input::InputKind;
             use std::path::Path;
 
@@ -74,25 +76,35 @@ impl Controller<'_> {
 
             let wrapping_mode = self.config.wrapping_mode;
 
-            output_type = OutputType::from_mode(paging_mode, wrapping_mode, self.config.pager)?;
+            output_type_opt = Some(OutputType::from_mode(
+                paging_mode,
+                wrapping_mode,
+                self.config.pager,
+            )?);
         }
 
         #[cfg(not(feature = "paging"))]
-        {
-            output_type = OutputType::stdout();
+        if output_handle.is_none() {
+            output_type_opt = Some(OutputType::stdout());
         }
 
-        let attached_to_pager = output_type.is_pager();
+        let attached_to_pager = match (&output_handle, &output_type_opt) {
+            (Some(_), _) => true,
+            (None, Some(ot)) => ot.is_pager(),
+            (None, None) => false,
+        };
+
         let stdout_identifier = if cfg!(windows) || attached_to_pager {
             None
         } else {
             clircle::Identifier::stdout()
         };
 
-        let mut writer = match output_handle {
-            Some(OutputHandle::FmtWrite(w)) => OutputHandle::FmtWrite(w),
-            Some(OutputHandle::IoWrite(w)) => OutputHandle::IoWrite(w),
-            None => output_type.handle()?,
+        let mut writer = match (output_handle, &mut output_type_opt) {
+            (Some(OutputHandle::FmtWrite(w)), _) => OutputHandle::FmtWrite(w),
+            (Some(OutputHandle::IoWrite(w)), _) => OutputHandle::IoWrite(w),
+            (None, Some(ot)) => ot.handle()?,
+            (None, None) => unreachable!("No output handle and no output type available"),
         };
         let mut no_errors: bool = true;
         let stderr = io::stderr();

src/output.rs

@@ -141,16 +141,18 @@ impl OutputType {
                 //
                 // For newer versions (530 or 558 on Windows), we omit '--no-init' as it
                 // is not needed anymore.
-                match retrieve_less_version(&pager.bin) {
-                    None => {
-                        p.arg("--no-init");
-                    }
-                    Some(LessVersion::Less(version))
-                        if (version < 530 || (cfg!(windows) && version < 558)) =>
-                    {
-                        p.arg("--no-init");
+                if single_screen_action == SingleScreenAction::Quit {
+                    match retrieve_less_version(&pager.bin) {
+                        None => {
+                            p.arg("--no-init");
+                        }
+                        Some(LessVersion::Less(version))
+                            if (version < 530 || (cfg!(windows) && version < 558)) =>
+                        {
+                            p.arg("--no-init");
+                        }
+                        _ => {}
                     }
-                    _ => {}
                 }
             } else {
                 p.args(args);