Also replace 'more' from PAGER with 'less'

But first do some quite significant refactorings to keep the code clean
and easy to understand.

CHANGELOG.md

@@ -11,7 +11,7 @@
 - Make ./tests/syntax-tests/regression_test.sh work on recent versions of macOS, see #1443 (@Enselic)
 - 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 `PAGER=most` (but not `BAT_PAGER` or `--pager`), silently use `less` instead since `most` does not support colors, see #1063 (@Enselic)
+- 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
 

src/output.rs

@@ -48,43 +48,33 @@ impl OutputType {
         wrapping_mode: WrappingMode,
         pager_from_config: Option<&str>,
     ) -> Result<Self> {
-        use std::ffi::OsString;
+        use crate::pager::{self, PagerKind, PagerSource};
-        use std::path::PathBuf;
         use std::process::{Command, Stdio};
-        use crate::pager::*;
 
-        let Pager { pager, source } = get_pager(pager_from_config);
+        let pager_opt =
+            pager::get_pager(pager_from_config).chain_err(|| "Could not parse pager command.")?;
 
-        let pagerflags =
+        let pager = match pager_opt {
-            shell_words::split(&pager).chain_err(|| "Could not parse pager command.")?;
+            Some(pager) => pager,
+            None => return Ok(OutputType::stdout()),
+        };
 
-        match pagerflags.split_first() {
+        if pager.kind == PagerKind::Bat {
-            Some((pager_name, pager_args)) => {
-                let mut pager_path = PathBuf::from(pager_name);
-                let mut args = pager_args;
-                let empty_args = vec![];
-
-                if pager_path.file_stem() == Some(&OsString::from("bat")) {
             return Err(ErrorKind::InvalidPagerValueBat.into());
         }
 
-                if pager_path.file_stem() == Some(&OsString::from("most")) && source == PagerSource::PagerEnvVar {
+        let mut p = Command::new(pager.bin);
-                    pager_path = PathBuf::from("less");
+        let args = pager.args;
-                    args = &empty_args;
-                }
 
-                let is_less = pager_path.file_stem() == Some(&OsString::from("less"));
+        if pager.kind == PagerKind::Less {
-
-                let mut process = if is_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 = source == PagerSource::PagerEnvVar;
+            let replace_arguments_to_less = pager.source == PagerSource::PagerEnvVar;
 
-                    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 {
@@ -106,9 +96,7 @@ impl OutputType {
                     None => {
                         p.arg("--no-init");
                     }
-                            Some(version)
+                    Some(version) if (version < 530 || (cfg!(windows) && version < 558)) => {
-                                if (version < 530 || (cfg!(windows) && version < 558)) =>
-                            {
                         p.arg("--no-init");
                     }
                     _ => {}
@@ -117,22 +105,15 @@ impl OutputType {
                 p.args(args);
             }
             p.env("LESSCHARSET", "UTF-8");
-                    p
         } else {
-                    let mut p = Command::new(&pager_path);
             p.args(args);
-                    p
         };
 
-                Ok(process
+        Ok(p.stdin(Stdio::piped())
-                    .stdin(Stdio::piped())
             .spawn()
             .map(OutputType::Pager)
             .unwrap_or_else(|_| OutputType::stdout()))
     }
-            None => Ok(OutputType::stdout()),
-        }
-    }
 
     pub(crate) fn stdout() -> Self {
         OutputType::Stdout(io::stdout())

src/pager.rs

@@ -1,3 +1,6 @@
+use shell_words::ParseError;
+use std::env;
+
 /// If we use a pager, this enum tells us from where we were told to use it.
 #[derive(Debug, PartialEq)]
 pub enum PagerSource {
@@ -14,31 +17,104 @@ pub enum PagerSource {
     Default,
 }
 
+/// We know about some pagers, for example 'less'. This is a list of all pagers we know about
+#[derive(Debug, PartialEq)]
+pub enum PagerKind {
+    /// The pager is ourselves
+    Bat,
+
+    /// less
+    Less,
+
+    /// more
+    More,
+
+    /// most
+    Most,
+
+    /// A pager we don't know about
+    Unknown,
+}
+
+impl PagerKind {
+    fn from_bin(bin: &str) -> PagerKind {
+        use std::ffi::OsStr;
+        use std::path::Path;
+
+        let stem = Path::new(bin)
+            .file_stem()
+            .unwrap_or_else(|| OsStr::new("unknown"));
+
+        if stem == OsStr::new("bat") {
+            PagerKind::Bat
+        } else if stem == OsStr::new("less") {
+            PagerKind::Less
+        } else if stem == OsStr::new("more") {
+            PagerKind::More
+        } else if stem == OsStr::new("most") {
+            PagerKind::Most
+        } else {
+            PagerKind::Unknown
+        }
+    }
+}
+
 /// A pager such as 'less', and from where we got it.
+#[derive(Debug)]
 pub struct Pager {
-    pub pager: String,
+    // The pager binary
+    pub bin: String,
+
+    // The pager binary arguments (that we might tweak)
+    pub args: Vec<String>,
+
+    // What pager this is
+    pub kind: PagerKind,
+
+    // From where this pager comes
     pub source: PagerSource,
 }
 
 impl Pager {
-    fn new(pager: &str, source: PagerSource) -> Pager {
+    fn new(bin: &str, args: &[String], kind: PagerKind, source: PagerSource) -> Pager {
         Pager {
-            pager: String::from(pager),
+            bin: String::from(bin),
+            args: args.to_vec(),
+            kind,
             source,
         }
     }
 }
 
 /// Returns what pager to use, after looking at both config and environment variables.
-pub fn get_pager(pager_from_config: Option<&str>) -> Pager {
+pub fn get_pager(config_pager: Option<&str>) -> Result<Option<Pager>, ParseError> {
-    match (
+    let bat_pager = env::var("BAT_PAGER");
-        pager_from_config,
+    let pager = env::var("PAGER");
-        std::env::var("BAT_PAGER"),
+
-        std::env::var("PAGER"),
+    let (cmd, source) = match (config_pager, &bat_pager, &pager) {
-    ) {
+        (Some(config_pager), _, _) => (config_pager, PagerSource::Config),
-        (Some(config), _, _) => Pager::new(config, PagerSource::Config),
+        (_, Ok(bat_pager), _) => (bat_pager.as_str(), PagerSource::BatPagerEnvVar),
-        (_, Ok(bat_pager), _) => Pager::new(&bat_pager, PagerSource::BatPagerEnvVar),
+        (_, _, Ok(pager)) => (pager.as_str(), PagerSource::PagerEnvVar),
-        (_, _, Ok(pager)) => Pager::new(&pager, PagerSource::PagerEnvVar),
+        _ => ("less", PagerSource::Default),
-        _ => Pager::new("less", PagerSource::Default),
+    };
+
+    let parts = shell_words::split(cmd)?;
+    match parts.split_first() {
+        Some((bin, args)) => {
+            let kind = PagerKind::from_bin(bin);
+
+            // 'more' and 'most' does not supports colors; automatically use 'less' instead
+            // if the problematic pager came from the generic PAGER env var
+            let no_color_support = kind == PagerKind::More || kind == PagerKind::Most;
+            let use_less_instead = no_color_support && source == PagerSource::PagerEnvVar;
+
+            Ok(Some(if use_less_instead {
+                let no_args = vec![];
+                Pager::new("less", &no_args, PagerKind::Less, PagerSource::PagerEnvVar)
+            } else {
+                Pager::new(bin, args, kind, source)
+            }))
+        }
+        None => Ok(None),
     }
 }