Fix #1063: Do not use 'more' or 'most' PAGER, as they are not compatible with bats output

CHANGELOG.md

@@ -13,6 +13,7 @@
 - VimL syntax highlighting fix, see #1450 (@esensar)
 - Print an 'Invalid syntax theme settings' error message if a custom theme is broken, see #614 (@Enselic)
 - If plain mode is set and wrap is not explicitly opted in, long lines will no be truncated, see #1426
+- If `PAGER` (but not `BAT_PAGER` or `--pager`) is `more` or `most`, silently use `less` instead to ensure support for colors, see #1063 (@Enselic)
 
 ## Other
 

Cargo.toml

@@ -76,6 +76,7 @@ default-features = false
 [dev-dependencies]
 tempdir = "0.3"
 assert_cmd = "1.0.2"
+serial_test = "0.5.1"
 predicates = "1.0.6"
 wait-timeout = "0.2.0"
 

src/lib.rs

@@ -33,6 +33,8 @@ mod less;
 pub mod line_range;
 mod output;
 #[cfg(feature = "paging")]
+mod pager;
+#[cfg(feature = "paging")]
 pub(crate) mod paging;
 mod preprocessor;
 mod pretty_printer;

src/output.rs

@@ -48,100 +48,71 @@ impl OutputType {
         wrapping_mode: WrappingMode,
         pager_from_config: Option<&str>,
     ) -> Result<Self> {
-        use std::env;
-        use std::ffi::OsString;
-        use std::path::PathBuf;
+        use crate::pager::{self, PagerKind, PagerSource};
         use std::process::{Command, Stdio};
 
-        let mut replace_arguments_to_less = false;
+        let pager_opt =
+            pager::get_pager(pager_from_config).chain_err(|| "Could not parse pager command.")?;
 
-        let pager_from_env = match (env::var("BAT_PAGER"), env::var("PAGER")) {
-            (Ok(bat_pager), _) => Some(bat_pager),
-            (_, Ok(pager)) => {
-                // less needs to be called with the '-R' option in order to properly interpret the
-                // ANSI color sequences printed by bat. If someone has set PAGER="less -F", we
-                // therefore need to overwrite the arguments and add '-R'.
-                //
-                // We only do this for PAGER (as it is not specific to 'bat'), not for BAT_PAGER
-                // or bats '--pager' command line option.
-                replace_arguments_to_less = true;
-                Some(pager)
-            }
-            _ => None,
+        let pager = match pager_opt {
+            Some(pager) => pager,
+            None => return Ok(OutputType::stdout()),
         };
 
-        let pager_from_config = pager_from_config.map(|p| p.to_string());
-
-        if pager_from_config.is_some() {
-            replace_arguments_to_less = false;
+        if pager.kind == PagerKind::Bat {
+            return Err(ErrorKind::InvalidPagerValueBat.into());
         }
 
-        let pager = pager_from_config
-            .or(pager_from_env)
-            .unwrap_or_else(|| String::from("less"));
-
-        let pagerflags =
-            shell_words::split(&pager).chain_err(|| "Could not parse pager command.")?;
-
-        match pagerflags.split_first() {
-            Some((pager_name, args)) => {
-                let pager_path = PathBuf::from(pager_name);
+        let mut p = Command::new(pager.bin);
+        let args = pager.args;
+
+        if pager.kind == PagerKind::Less {
+            // less needs to be called with the '-R' option in order to properly interpret the
+            // ANSI color sequences printed by bat. If someone has set PAGER="less -F", we
+            // therefore need to overwrite the arguments and add '-R'.
+            //
+            // We only do this for PAGER (as it is not specific to 'bat'), not for BAT_PAGER
+            // or bats '--pager' command line option.
+            let replace_arguments_to_less = pager.source == PagerSource::EnvVarPager;
+
+            if args.is_empty() || replace_arguments_to_less {
+                p.arg("--RAW-CONTROL-CHARS");
+                if single_screen_action == SingleScreenAction::Quit {
+                    p.arg("--quit-if-one-screen");
+                }
 
-                if pager_path.file_stem() == Some(&OsString::from("bat")) {
-                    return Err(ErrorKind::InvalidPagerValueBat.into());
+                if wrapping_mode == WrappingMode::NoWrapping(true) {
+                    p.arg("--chop-long-lines");
                 }
 
-                let is_less = pager_path.file_stem() == Some(&OsString::from("less"));
-
-                let mut process = if is_less {
-                    let mut p = Command::new(&pager_path);
-                    if args.is_empty() || replace_arguments_to_less {
-                        p.arg("--RAW-CONTROL-CHARS");
-                        if single_screen_action == SingleScreenAction::Quit {
-                            p.arg("--quit-if-one-screen");
-                        }
-
-                        if wrapping_mode == WrappingMode::NoWrapping(true) {
-                            p.arg("--chop-long-lines");
-                        }
-
-                        // Passing '--no-init' fixes a bug with '--quit-if-one-screen' in older
-                        // versions of 'less'. Unfortunately, it also breaks mouse-wheel support.
-                        //
-                        // See: http://www.greenwoodsoftware.com/less/news.530.html
-                        //
-                        // For newer versions (530 or 558 on Windows), we omit '--no-init' as it
-                        // is not needed anymore.
-                        match retrieve_less_version() {
-                            None => {
-                                p.arg("--no-init");
-                            }
-                            Some(version)
-                                if (version < 530 || (cfg!(windows) && version < 558)) =>
-                            {
-                                p.arg("--no-init");
-                            }
-                            _ => {}
-                        }
-                    } else {
-                        p.args(args);
+                // Passing '--no-init' fixes a bug with '--quit-if-one-screen' in older
+                // versions of 'less'. Unfortunately, it also breaks mouse-wheel support.
+                //
+                // See: http://www.greenwoodsoftware.com/less/news.530.html
+                //
+                // For newer versions (530 or 558 on Windows), we omit '--no-init' as it
+                // is not needed anymore.
+                match retrieve_less_version() {
+                    None => {
+                        p.arg("--no-init");
+                    }
+                    Some(version) if (version < 530 || (cfg!(windows) && version < 558)) => {
+                        p.arg("--no-init");
                     }
-                    p.env("LESSCHARSET", "UTF-8");
-                    p
-                } else {
-                    let mut p = Command::new(&pager_path);
-                    p.args(args);
-                    p
-                };
-
-                Ok(process
-                    .stdin(Stdio::piped())
-                    .spawn()
-                    .map(OutputType::Pager)
-                    .unwrap_or_else(|_| OutputType::stdout()))
+                    _ => {}
+                }
+            } else {
+                p.args(args);
             }
-            None => Ok(OutputType::stdout()),
-        }
+            p.env("LESSCHARSET", "UTF-8");
+        } else {
+            p.args(args);
+        };
+
+        Ok(p.stdin(Stdio::piped())
+            .spawn()
+            .map(OutputType::Pager)
+            .unwrap_or_else(|_| OutputType::stdout()))
     }
 
     pub(crate) fn stdout() -> Self {