Repository: sharkdp/bat (language: rust)

Issue:
Env var should take precedence over config
Hello,

i would like to set a default theme in my bat config file, but override it occasionally with the BAT_THEME env var. Currently it seems that if the theme is set in the config then env is ignored. This seems wrong to me, i think env should take precedence over the config file (as would usually be the case to allow customisation per shell instance).

Thanks,
Bram


Discussion / hints:
Thank you for reporting this.

> i think env should take precedence over the config file (as would usually be the case to allow customisation per shell instance).

Hm. Is this really the most common behavior of other command line programs? It does sound reasonable, but for some reason I thought it should be "environment variables < config file < command line arguments". But you might be right.
He's right: the correct priority is "config files < env vars < command line flags"
I believe you, but it would be great to see some actual examples.
`git` is a nice example...

e.g. the man page for `git-config` states:

```
The user.name and user.email variables determine what ends up in the
           author and committer field of commit objects. If you need the author or
           committer to be different, the author.name, author.email, committer.name
           or committer.email variables can be set. Also, all of these can be
           overridden by the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, GIT_COMMITTER_NAME,
           GIT_COMMITTER_EMAIL and EMAIL environment variables.
```

The environment should always take priority over config files. Git is full of other examples of this.

It makes sense if you think about it; config files are like global settings, env vars apply settings to the entire shell session, and command line args are per command settings.
Ok, thank you. In this case, let's make the necessary changes (and add some tests!). I'm afraid the change might not be completely trivial though.
If anyone wants to tackle this, make sure to also take a look at https://github.com/sharkdp/bat/pull/1281. I am closing that PR now to make the PR inbox cleaner.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d1355e211f..c37d935547 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,6 +3,7 @@
 ## Features
 
 - Implemented `-S` and `--chop-long-lines` flags as aliases for `--wrap=never`. See #2309 (@johnmatthiggins)
+- Breaking change: Environment variables can now override config file settings (but command-line arguments still have the highest precedence), see #1152, #1281, and #2381 (@aaronkollasch)
 
 ## Bugfixes
 
diff --git a/src/bin/bat/app.rs b/src/bin/bat/app.rs
index d38c529d11..58389beb2e 100644
--- a/src/bin/bat/app.rs
+++ b/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;
 
@@ -50,20 +49,34 @@ impl App {
     }
 
     fn matches(interactive_output: bool) -> Result<ArgMatches> {
-        let 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
+        let args = if wild::args_os().nth(1) == Some("cache".into()) {
+            // Skip the config file and env vars
 
             wild::args_os().collect::<Vec<_>>()
+        } else if wild::args_os().any(|arg| arg == "--no-config") {
+            // Skip the arguments in bats config file
+
+            let mut cli_args = wild::args_os();
+            let mut args = get_args_from_env_vars();
+
+            // Put the zero-th CLI argument (program name) first
+            args.insert(0, cli_args.next().unwrap());
+
+            // .. and the rest at the end
+            cli_args.for_each(|a| args.push(a));
+
+            args
         } else {
             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")?;
 
+            // Selected env vars supersede config vars
+            args.extend(get_args_from_env_vars());
+
             // Put the zero-th CLI argument (program name) first
             args.insert(0, cli_args.next().unwrap());
 
@@ -203,7 +216,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 +228,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 +332,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 +341,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))
diff --git a/src/bin/bat/config.rs b/src/bin/bat/config.rs
index 696edf9e9c..4abae4a69a 100644
--- a/src/bin/bat/config.rs
+++ b/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,20 @@ 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.to_string(), var]))
+    .flatten()
+    .map(|a| a.into())
+    .collect()
+}
+
 #[test]
 fn empty() {
     let args = get_args_from_str("").unwrap();


Test patch:
diff --git a/tests/examples/bat-tabs.conf b/tests/examples/bat-tabs.conf
new file mode 100644
index 0000000000..c172f95f33
--- /dev/null
+++ b/tests/examples/bat-tabs.conf
@@ -0,0 +1,1 @@
+--tabs=8
diff --git a/tests/examples/bat-theme.conf b/tests/examples/bat-theme.conf
new file mode 100644
index 0000000000..c39ad71346
--- /dev/null
+++ b/tests/examples/bat-theme.conf
@@ -0,0 +1,1 @@
+--theme=TwoDark
diff --git a/tests/examples/cache_source/syntaxes/c.sublime-syntax b/tests/examples/cache_source/syntaxes/c.sublime-syntax
new file mode 100644
index 0000000000..7f88e402be
--- /dev/null
+++ b/tests/examples/cache_source/syntaxes/c.sublime-syntax
@@ -0,0 +1,10 @@
+%YAML 1.2
+---
+name: C
+file_extensions: [c, h]
+scope: source.c
+
+contexts:
+  main:
+    - match: \b(if|else|for|while)\b
+      scope: keyword.control.c
diff --git a/tests/examples/cache_source/themes/example.tmTheme b/tests/examples/cache_source/themes/example.tmTheme
new file mode 100644
index 0000000000..94b40cfda7
--- /dev/null
+++ b/tests/examples/cache_source/themes/example.tmTheme
@@ -0,0 +1,45 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>name</key>
+	<string>example</string>
+	<key>settings</key>
+	<array>
+		<dict>
+			<key>settings</key>
+			<dict>
+				<key>background</key>
+				<string>#222222</string>
+				<key>caret</key>
+				<string>#979797</string>
+				<key>foreground</key>
+				<string>#F8F8F8</string>
+				<key>invisibles</key>
+				<string>#777777</string>
+				<key>lineHighlight</key>
+				<string>#000000</string>
+				<key>selection</key>
+				<string>#57CCBF</string>
+			</dict>
+		</dict>
+		<dict>
+			<key>name</key>
+			<string>Comment</string>
+			<key>scope</key>
+			<string>comment</string>
+			<key>settings</key>
+			<dict>
+				<key>foreground</key>
+				<string>#777777</string>
+			</dict>
+		</dict>
+	</array>
+	<key>uuid</key>
+	<string>0123-4567-89AB-CDEF</string>
+	<key>colorSpaceName</key>
+	<string>sRGB</string>
+	<key>semanticClass</key>
+	<string>theme</string>
+</dict>
+</plist>
diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs
index b7becf8b0f..a1c79e0287 100644
--- a/tests/integration_tests.rs
+++ b/tests/integration_tests.rs
@@ -477,6 +477,79 @@ fn tabs_8() {
         );
 }
 
+#[test]
+fn tabs_4_env_overrides_config() {
+    bat_with_config()
+        .env("BAT_CONFIG_PATH", "bat-tabs.conf")
+        .env("BAT_TABS", "4")
+        .arg("tabs.txt")
+        .arg("--style=plain")
+        .arg("--decorations=always")
+        .assert()
+        .success()
+        .stdout(
+            "    1   2   3   4
+1   ?
+22  ?
+333 ?
+4444    ?
+55555   ?
+666666  ?
+7777777 ?
+88888888    ?
+",
+        );
+}
+
+#[test]
+fn tabs_4_arg_overrides_env() {
+    bat_with_config()
+        .env("BAT_CONFIG_PATH", "bat-tabs.conf")
+        .env("BAT_TABS", "6")
+        .arg("tabs.txt")
+        .arg("--tabs=4")
+        .arg("--style=plain")
+        .arg("--decorations=always")
+        .assert()
+        .success()
+        .stdout(
+            "    1   2   3   4
+1   ?
+22  ?
+333 ?
+4444    ?
+55555   ?
+666666  ?
+7777777 ?
+88888888    ?
+",
+        );
+}
+
+#[test]
+fn tabs_4_arg_overrides_env_noconfig() {
+    bat()
+        .env("BAT_TABS", "6")
+        .arg("tabs.txt")
+        .arg("--tabs=4")
+        .arg("--style=plain")
+        .arg("--decorations=always")
+        .assert()
+        .success()
+        .stdout(
+            "    1   2   3   4
+1   ?
+22  ?
+333 ?
+4444    ?
+55555   ?
+666666  ?
+7777777 ?
+88888888    ?
+",
+        );
+}
+
 #[test]
 fn fail_non_existing() {
     bat().arg("non-existing-file").assert().failure();
@@ -508,6 +581,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 +616,58 @@ fn pager_disable() {
         .stdout(predicate::eq("hello world\n").normalize());
 }
 
+#[test]
+fn pager_arg_override_env_withconfig() {
+    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_arg_override_env_noconfig() {
+    bat()
+        .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()
@@ -756,6 +892,102 @@ fn config_read_arguments_from_file() {
         .stdout(predicate::eq("dummy-pager-from-config\n").normalize());
 }
 
+// Ignore this test for now as `bat cache --clear` only targets the default cache dir.
+// `bat cache --clear` must clear the `--target` dir for this test to pass.
+#[cfg(unix)]
+#[test]
+#[ignore]
+fn cache_clear() {
+    let src_dir = "cache_source";
+    let tmp_dir = tempdir().expect("can create temporary directory");
+    let themes_filename = "themes.bin";
+    let syntaxes_filename = "syntaxes.bin";
+    let metadata_filename = "metadata.yaml";
+    [themes_filename, syntaxes_filename, metadata_filename]
+        .iter()
+        .map(|filename| {
+            let fp = tmp_dir.path().join(filename);
+            let mut file = File::create(fp).expect("can create temporary file");
+            writeln!(file, "dummy content").expect("can write to file");
+        })
+        .count();
+
+    // Clear the targeted cache
+    // Include the BAT_CONFIG_PATH and BAT_THEME environment variables to ensure that
+    // options loaded from a config or the environment are not inserted
+    // before the cache subcommand, which would break it.
+    bat_with_config()
+        .current_dir(Path::new(EXAMPLES_DIR).join(src_dir))
+        .env("BAT_CONFIG_PATH", "bat.conf")
+        .env("BAT_THEME", "1337")
+        .arg("cache")
+        .arg("--clear")
+        .arg("--source")
+        .arg(".")
+        .arg("--target")
+        .arg(tmp_dir.path().to_str().unwrap())
+        .assert()
+        .success()
+        .stdout(
+            predicate::str::is_match(
+                "Clearing theme set cache ... okay
+Clearing syntax set cache ... okay
+Clearing metadata file ... okay",
+            )
+            .unwrap(),
+        );
+
+    // We expect these files to be removed
+    assert!(!tmp_dir.path().join(themes_filename).exists());
+    assert!(!tmp_dir.path().join(syntaxes_filename).exists());
+    assert!(!tmp_dir.path().join(metadata_filename).exists());
+}
+
+#[cfg(unix)]
+#[test]
+fn cache_build() {
+    let src_dir = "cache_source";
+    let tmp_dir = tempdir().expect("can create temporary directory");
+    let tmp_themes_path = tmp_dir.path().join("themes.bin");
+    let tmp_syntaxes_path = tmp_dir.path().join("syntaxes.bin");
+    let tmp_acknowledgements_path = tmp_dir.path().join("acknowledgements.bin");
+    let tmp_metadata_path = tmp_dir.path().join("metadata.yaml");
+
+    // Build the cache
+    // Include the BAT_CONFIG_PATH and BAT_THEME environment variables to ensure that
+    // options loaded from a config or the environment are not inserted
+    // before the cache subcommand, which would break it.
+    bat_with_config()
+        .current_dir(Path::new(EXAMPLES_DIR).join(src_dir))
+        .env("BAT_CONFIG_PATH", "bat.conf")
+        .env("BAT_THEME", "1337")
+        .arg("cache")
+        .arg("--build")
+        .arg("--blank")
+        .arg("--source")
+        .arg(".")
+        .arg("--target")
+        .arg(tmp_dir.path().to_str().unwrap())
+        .arg("--acknowledgements")
+        .assert()
+        .success()
+        .stdout(
+            predicate::str::is_match(
+                "Writing theme set to .*/themes.bin ... okay
+Writing syntax set to .*/syntaxes.bin ... okay
+Writing acknowledgements to .*/acknowledgements.bin ... okay
+Writing metadata to folder .* ... okay",
+            )
+            .unwrap(),
+        );
+
+    // Now we expect the files to exist. If they exist, we assume contents are correct
+    assert!(tmp_themes_path.exists());
+    assert!(tmp_syntaxes_path.exists());
+    assert!(tmp_acknowledgements_path.exists());
+    assert!(tmp_metadata_path.exists());
+}
+
 #[test]
 fn utf16() {
     // The output will be converted to UTF-8 with the leading UTF-16
@@ -981,6 +1213,35 @@ fn header_full_basic() {
         .stderr("");
 }
 
+#[test]
+fn header_env_basic() {
+    bat_with_config()
+        .env("BAT_STYLE", "header-filename,header-filesize")
+        .arg("test.txt")
+        .arg("--decorations=always")
+        .arg("-r=0:0")
+        .arg("--file-name=foo")
+        .assert()
+        .success()
+        .stdout("File: foo\nSize: 12 B\n")
+        .stderr("");
+}
+
+#[test]
+fn header_arg_overrides_env() {
+    bat_with_config()
+        .env("BAT_STYLE", "header-filesize")
+        .arg("test.txt")
+        .arg("--decorations=always")
+        .arg("--style=header-filename")
+        .arg("-r=0:0")
+        .arg("--file-name=foo")
+        .assert()
+        .success()
+        .stdout("File: foo\n")
+        .stderr("");
+}
+
 #[test]
 fn header_binary() {
     bat()
@@ -1540,6 +1801,64 @@ fn no_wrapping_with_chop_long_lines() {
     wrapping_test("--chop-long-lines", false);
 }
 
+#[test]
+fn theme_arg_overrides_env() {
+    bat()
+        .env("BAT_THEME", "TwoDark")
+        .arg("--paging=never")
+        .arg("--color=never")
+        .arg("--terminal-width=80")
+        .arg("--wrap=never")
+        .arg("--decorations=always")
+        .arg("--theme=ansi")
+        .arg("--style=plain")
+        .arg("--highlight-line=1")
+        .write_stdin("Ansi Underscore Test\nAnother Line")
+        .assert()
+        .success()
+        .stdout("\x1B[4mAnsi Underscore Test\n\x1B[24mAnother Line")
+        .stderr("");
+}
+
+#[test]
+fn theme_arg_overrides_env_withconfig() {
+    bat_with_config()
+        .env("BAT_CONFIG_PATH", "bat-theme.conf")
+        .env("BAT_THEME", "TwoDark")
+        .arg("--paging=never")
+        .arg("--color=never")
+        .arg("--terminal-width=80")
+        .arg("--wrap=never")
+        .arg("--decorations=always")
+        .arg("--theme=ansi")
+        .arg("--style=plain")
+        .arg("--highlight-line=1")
+        .write_stdin("Ansi Underscore Test\nAnother Line")
+        .assert()
+        .success()
+        .stdout("\x1B[4mAnsi Underscore Test\n\x1B[24mAnother Line")
+        .stderr("");
+}
+
+#[test]
+fn theme_env_overrides_config() {
+    bat_with_config()
+        .env("BAT_CONFIG_PATH", "bat-theme.conf")
+        .env("BAT_THEME", "ansi")
+        .arg("--paging=never")
+        .arg("--color=never")
+        .arg("--terminal-width=80")
+        .arg("--wrap=never")
+        .arg("--decorations=always")
+        .arg("--style=plain")
+        .arg("--highlight-line=1")
+        .write_stdin("Ansi Underscore Test\nAnother Line")
+        .assert()
+        .success()
+        .stdout("\x1B[4mAnsi Underscore Test\n\x1B[24mAnother Line")
+        .stderr("");
+}
+
 #[test]
 fn highlighting_is_skipped_on_long_lines() {
     let expected = "\u{1b}[38;5;231m{\u{1b}[0m\u{1b}[38;5;208m\"\u{1b}[0m\u{1b}[38;5;208mapi\u{1b}[0m\u{1b}[38;5;208m\"\u{1b}[0m\u{1b}[38;5;231m:\u{1b}[0m\n".to_owned() +