Allow env vars to override config but not args

CHANGELOG.md

@@ -1,5 +1,7 @@
 # unreleased
 
+- Allow env vars to override config files, but not command-line arguments, see #1152, #1281, and #2381 (@aaronkollasch)
+
 ## Features
 
 - Implemented `-S` and `--chop-long-lines` flags as aliases for `--wrap=never`. See #2309 (@johnmatthiggins)

src/bin/bat/app.rs

@@ -1,13 +1,12 @@
 use std::collections::HashSet;
 use std::env;
 use std::path::{Path, PathBuf};
-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_opts_var, get_args_from_env_vars},
 };
 use clap::ArgMatches;
 
@@ -60,15 +59,20 @@ impl App {
             let mut cli_args = wild::args_os();
 
             // Read arguments from bats config file
-            let mut args = get_args_from_env_var()
+            let mut args = get_args_from_env_opts_var()
                 .unwrap_or_else(get_args_from_config_file)
                 .map_err(|_| "Could not parse configuration file")?;
 
             // Put the zero-th CLI argument (program name) first
             args.insert(0, cli_args.next().unwrap());
 
+            // env vars supersede config vars
+            get_args_from_env_vars()
+                .iter()
+                .for_each(|a| args.push(a.into()));
+
             // .. and the rest at the end
-            cli_args.for_each(|a| args.push(a));
+            cli_args.for_each(|a| args.push(a.into()));
 
             args
         };
@@ -203,7 +207,6 @@ impl App {
                 .matches
                 .get_one::<String>("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 {
@@ -216,7 +219,6 @@ impl App {
                 .matches
                 .get_one::<String>("theme")
                 .map(String::from)
-                .or_else(|| env::var("BAT_THEME").ok())
                 .map(|s| {
                     if s == "default" {
                         String::from(HighlightingAssets::default_theme())
@@ -321,16 +323,6 @@ impl App {
             } else if 0 < matches.get_count("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(StyleComponent::from_str)
-                            .collect::<Result<Vec<StyleComponent>>>()
-                    })
-                    .transpose()?;
-
                 matches
                     .get_one::<String>("style")
                     .map(|styles| {
@@ -340,7 +332,6 @@ impl App {
                             .filter_map(|style| style.ok())
                             .collect::<Vec<_>>()
                     })
-                    .or(env_style_components)
                     .unwrap_or_else(|| vec![StyleComponent::Default])
                     .into_iter()
                     .map(|style| style.components(self.interactive_output))

src/bin/bat/config.rs

@@ -117,7 +117,7 @@ pub fn get_args_from_config_file() -> Result<Vec<OsString>, shell_words::ParseEr
     get_args_from_str(&config)
 }
 
-pub fn get_args_from_env_var() -> Option<Result<Vec<OsString>, shell_words::ParseError>> {
+pub fn get_args_from_env_opts_var() -> Option<Result<Vec<OsString>, shell_words::ParseError>> {
     env::var("BAT_OPTS").ok().map(|s| get_args_from_str(&s))
 }
 
@@ -137,6 +137,19 @@ fn get_args_from_str(content: &str) -> Result<Vec<OsString>, shell_words::ParseE
         .collect())
 }
 
+pub fn get_args_from_env_vars() -> Vec<OsString> {
+    [
+        ("--tabs", "BAT_TABS"),
+        ("--theme", "BAT_THEME"),
+        ("--pager", "BAT_PAGER"),
+        ("--style", "BAT_STYLE"),
+    ]
+    .iter()
+    .filter_map(|(flag, key)| env::var(key).ok().map(|var| [flag.into(), var.into()]))
+    .flatten()
+    .collect()
+}
+
 #[test]
 fn empty() {
     let args = get_args_from_str("").unwrap();

tests/integration_tests.rs

@@ -508,6 +508,17 @@ fn pager_basic() {
         .stdout(predicate::eq("pager-output\n").normalize());
 }
 
+#[test]
+fn pager_basic_arg() {
+    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()
@@ -532,6 +543,45 @@ fn pager_disable() {
         .stdout(predicate::eq("hello world\n").normalize());
 }
 
+#[test]
+fn pager_arg_override_env() {
+    bat_with_config()
+        .env("BAT_CONFIG_PATH", "bat.conf")
+        .env("PAGER", "echo another-pager")
+        .env("BAT_PAGER", "echo other-pager")
+        .arg("--pager=echo pager-output")
+        .arg("--paging=always")
+        .arg("test.txt")
+        .assert()
+        .success()
+        .stdout(predicate::eq("pager-output\n").normalize());
+}
+
+#[test]
+fn pager_env_bat_pager_override_config() {
+    bat_with_config()
+        .env("BAT_CONFIG_PATH", "bat.conf")
+        .env("PAGER", "echo other-pager")
+        .env("BAT_PAGER", "echo pager-output")
+        .arg("--paging=always")
+        .arg("test.txt")
+        .assert()
+        .success()
+        .stdout(predicate::eq("pager-output\n").normalize());
+}
+
+#[test]
+fn pager_env_pager_nooverride_config() {
+    bat_with_config()
+        .env("BAT_CONFIG_PATH", "bat.conf")
+        .env("PAGER", "echo other-pager")
+        .arg("--paging=always")
+        .arg("test.txt")
+        .assert()
+        .success()
+        .stdout(predicate::eq("dummy-pager-from-config\n").normalize());
+}
+
 #[test]
 fn env_var_pager_value_bat() {
     bat()