Allow env vars to override config variables, but not flags

Parses env configurations into matches (for those that can be configured
via matches) instead of relying on dedicated env checks everywhere.

Fixes sharkdp#1152

src/bin/bat/app.rs

@@ -1,13 +1,12 @@
 use std::collections::HashSet;
 use std::env;
 use std::ffi::OsStr;
-use std::str::FromStr;
 
 use atty::{self, Stream};
 
 use crate::{
     clap_app,
-    config::{get_args_from_config_file, get_args_from_env_var},
+    config::{get_args_from_config_file, get_args_from_env_vars, get_args_from_opts_env_var},
 };
 use clap::ArgMatches;
 
@@ -49,28 +48,29 @@ impl App {
     }
 
     fn matches(interactive_output: bool) -> Result<ArgMatches<'static>> {
-        let args = if wild::args_os().nth(1) == Some("cache".into())
+        let mut cli_args = wild::args_os();
+        let mut args = if wild::args_os().nth(1) == Some("cache".into())
             || wild::args_os().any(|arg| arg == "--no-config")
         {
             // Skip the arguments in bats config file
-
-            wild::args_os().collect::<Vec<_>>()
+            Vec::new()
         } else {
-            let mut cli_args = wild::args_os();
-
             // Read arguments from bats config file
-            let mut args = get_args_from_env_var()
+            get_args_from_opts_env_var()
                 .unwrap_or_else(get_args_from_config_file)
-                .chain_err(|| "Could not parse configuration file")?;
+                .chain_err(|| "Could not parse configuration file")?
+        };
 
-            // Put the zero-th CLI argument (program name) first
-            args.insert(0, cli_args.next().unwrap());
+        args.append(
+            &mut get_args_from_env_vars()
+                .chain_err(|| "Could not get args from environment variables")?,
+        );
 
-            // .. and the rest at the end
-            cli_args.for_each(|a| args.push(a));
+        // Put the zero-th CLI argument (program name) first
+        args.insert(0, cli_args.next().unwrap());
 
-            args
-        };
+        // .. and the rest at the end
+        cli_args.for_each(|a| args.push(a));
 
         Ok(clap_app::build_app(interactive_output).get_matches_from(args))
     }
@@ -183,7 +183,6 @@ impl App {
                 .matches
                 .value_of("tabs")
                 .map(String::from)
-                .or_else(|| env::var("BAT_TABS").ok())
                 .and_then(|t| t.parse().ok())
                 .unwrap_or(
                     if style_components.plain() && paging_mode == PagingMode::Never {
@@ -196,7 +195,6 @@ impl App {
                 .matches
                 .value_of("theme")
                 .map(String::from)
-                .or_else(|| env::var("BAT_THEME").ok())
                 .map(|s| {
                     if s == "default" {
                         String::from(HighlightingAssets::default_theme())
@@ -296,16 +294,6 @@ impl App {
             } else if matches.is_present("plain") {
                 [StyleComponent::Plain].iter().cloned().collect()
             } else {
-                let env_style_components: Option<Vec<StyleComponent>> = env::var("BAT_STYLE")
-                    .ok()
-                    .map(|style_str| {
-                        style_str
-                            .split(',')
-                            .map(|x| StyleComponent::from_str(&x))
-                            .collect::<Result<Vec<StyleComponent>>>()
-                    })
-                    .transpose()?;
-
                 matches
                     .value_of("style")
                     .map(|styles| {
@@ -315,7 +303,6 @@ impl App {
                             .filter_map(|style| style.ok())
                             .collect::<Vec<_>>()
                     })
-                    .or(env_style_components)
                     .unwrap_or_else(|| vec![StyleComponent::Full])
                     .into_iter()
                     .map(|style| style.components(self.interactive_output))

src/bin/bat/config.rs

@@ -44,8 +44,7 @@ pub fn generate_config_file() -> bat::error::Result<()> {
         }
     }
 
-    let default_config =
-        r#"# This is `bat`s configuration file. Each line either contains a comment or
+    let default_config = r#"# This is `bat`s configuration file. Each line either contains a comment or
 # a command-line option that you want to pass to `bat` by default. You can
 # run `bat --help` to get a list of all possible configuration options.
 
@@ -89,7 +88,7 @@ pub fn get_args_from_config_file() -> Result<Vec<OsString>, shell_words::ParseEr
         .unwrap_or_else(|| vec![]))
 }
 
-pub fn get_args_from_env_var() -> Option<Result<Vec<OsString>, shell_words::ParseError>> {
+pub fn get_args_from_opts_env_var() -> Option<Result<Vec<OsString>, shell_words::ParseError>> {
     env::var("BAT_OPTS").ok().map(|s| get_args_from_str(&s))
 }
 
@@ -109,6 +108,24 @@ fn get_args_from_str(content: &str) -> Result<Vec<OsString>, shell_words::ParseE
         .collect())
 }
 
+pub fn get_args_from_env_vars() -> Result<Vec<OsString>, shell_words::ParseError> {
+    let env_arg_pairs = [
+        ("BAT_THEME", "theme"),
+        ("BAT_STYLE", "style"),
+        ("BAT_TABS", "tabs"),
+        ("BAT_PAGER", "pager"),
+    ];
+
+    Ok(env_arg_pairs
+        .iter()
+        .filter_map(|(env_var, arg)| {
+            env::var(env_var)
+                .ok()
+                .map(|s| OsString::from(format!("--{}={}", arg, s)))
+        })
+        .collect())
+}
+
 #[test]
 fn empty() {
     let args = get_args_from_str("").unwrap();
@@ -167,3 +184,13 @@ fn comments() {
         get_args_from_str(config).unwrap()
     );
 }
+
+#[test]
+fn env_var() {
+    env::set_var("BAT_THEME", "test");
+    env::set_var("BAT_PAGER", "more");
+    assert_eq!(
+        vec!["--theme=test", "--pager=more"],
+        get_args_from_env_vars().unwrap()
+    );
+}

src/output.rs

@@ -36,20 +36,16 @@ impl OutputType {
 
         let mut replace_arguments_to_less = false;
 
-        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_from_env = env::var("PAGER").ok().map(|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;
+            pager
+        });
 
         let pager_from_config = pager_from_config.map(|p| p.to_string());
 

tests/integration_tests.rs

@@ -381,6 +381,17 @@ fn pager_basic() {
         .stdout(predicate::eq("pager-output\n").normalize());
 }
 
+#[test]
+fn pager_config() {
+    bat()
+        .arg("--pager=echo pager-output")
+        .arg("--paging=always")
+        .arg("test.txt")
+        .assert()
+        .success()
+        .stdout(predicate::eq("pager-output\n").normalize());
+}
+
 #[test]
 fn pager_overwrite() {
     bat()
@@ -393,6 +404,19 @@ fn pager_overwrite() {
         .stdout(predicate::eq("pager-output\n").normalize());
 }
 
+#[test]
+fn pager_overwrite_overwrite() {
+    bat()
+        .env("PAGER", "echo other-pager")
+        .env("BAT_PAGER", "echo another-pager")
+        .arg("--pager=echo pager-output")
+        .arg("--paging=always")
+        .arg("test.txt")
+        .assert()
+        .success()
+        .stdout(predicate::eq("pager-output\n").normalize());
+}
+
 #[test]
 fn pager_disable() {
     bat()

tests/syntax-tests/source/Rust/output.rs

@@ -36,20 +36,16 @@ impl OutputType {
 
         let mut replace_arguments_to_less = false;
 
-        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_from_env = env::var("PAGER").ok().map(|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;
+            pager
+        });
 
         let pager_from_config = pager_from_config.map(|p| p.to_string());