Merge pull request #3507 from sharkdp/help_read_theme_from_config

Ensure --help respects valid config file

CHANGELOG.md

@@ -10,6 +10,7 @@
 - Fix negative values of N not being parsed in <N:M> line ranges without `=` flag value separator, see #3442 (@lmmx)
 - Fix broken Docker syntax preventing use of custom assets, see #3476 (@keith-hall)
 - Fix decorations being applied unexpectedly when piping. Now only line numbers explicitly required on the command line should be applied in auto decorations mode for `cat` compatibility. See #3496 (@keith-hall)
+- `--help` now correctly reads the config file for theme information etc. See #3507 (@keith-hall)
 
 ## Other
 - Improve README documentation on pager options passed to less, see #3443 (@injust)

src/bin/bat/app.rs

@@ -186,6 +186,20 @@ impl App {
         Ok(())
     }
 
+    /// Build argument list with env vars and CLI args (without config file)
+    fn build_args_without_config() -> Vec<std::ffi::OsString> {
+        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
+    }
+
     fn matches(interactive_output: bool) -> Result<ArgMatches> {
         // Check if we should skip config file processing for special arguments
         // that don't require full application setup (version, diagnostic)
@@ -196,63 +210,65 @@ impl App {
             )
         });
 
-        // Check if help was requested - help should go through the same code path
+        // Check if help was requested - help should read the config file but be
-        // but be forgiving of config file errors
+        // forgiving of invalid arguments (so configured theme etc. can be used)
         let help_requested =
             wild::args_os().any(|arg| matches!(arg.to_str(), Some("-h" | "--help")));
 
-        let args = if wild::args_os().nth(1) == Some("cache".into()) {
+        if wild::args_os().nth(1) == Some("cache".into()) {
             // Skip the config file and env vars
+            let args = wild::args_os().collect::<Vec<_>>();
+            return Ok(clap_app::build_app(interactive_output).get_matches_from(args));
+        }
 
-            wild::args_os().collect::<Vec<_>>()
+        if wild::args_os().any(|arg| arg == "--no-config") || should_skip_config {
-        } else if wild::args_os().any(|arg| arg == "--no-config") || should_skip_config {
             // Skip the arguments in bats config file when --no-config is present
             // or when user requests version or diagnostic information
+            let args = Self::build_args_without_config();
+            return Ok(clap_app::build_app(interactive_output).get_matches_from(args));
+        }
 
-            let mut cli_args = wild::args_os();
+        // Build arguments with config file
-            let mut args = get_args_from_env_vars();
+        let mut cli_args = wild::args_os();
 
-            // Put the zero-th CLI argument (program name) first
+        // Read arguments from bats config file
-            args.insert(0, cli_args.next().unwrap());
+        let config_args = match get_args_from_env_opts_var() {
-
+            Some(result) => result,
-            // .. and the rest at the end
+            None => get_args_from_config_file(),
-            cli_args.for_each(|a| args.push(a));
-
-            args
-        } else if help_requested {
-            // Help goes through the normal config path but only uses env vars for themes
-            // to avoid failing on invalid config options
-            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 (includes --help and other CLI args)
-            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 = match get_args_from_env_opts_var() {
-                Some(result) => result,
-                None => 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());
-
-            // .. and the rest at the end
-            cli_args.for_each(|a| args.push(a));
-            args
         };
 
-        Ok(clap_app::build_app(interactive_output).get_matches_from(args))
+        // For help, ignore config file parse errors (use empty config instead)
+        // For non-help, propagate the error
+        let mut args = if help_requested {
+            config_args.unwrap_or_default()
+        } else {
+            config_args.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());
+
+        // .. and the rest at the end
+        cli_args.for_each(|a| args.push(a));
+
+        // For help, try parsing with config, and if clap fails (e.g., invalid
+        // argument in config), fall back to parsing without config file args
+        if help_requested {
+            let app = clap_app::build_app(interactive_output);
+            match app.try_get_matches_from(args) {
+                Ok(matches) => Ok(matches),
+                Err(_) => {
+                    // Config has invalid arguments, fall back to just env vars + CLI args
+                    let fallback_args = Self::build_args_without_config();
+                    Ok(clap_app::build_app(interactive_output).get_matches_from(fallback_args))
+                }
+            }
+        } else {
+            Ok(clap_app::build_app(interactive_output).get_matches_from(args))
+        }
     }
 
     pub fn config(&self, inputs: &[Input]) -> Result<Config<'_>> {

tests/integration_tests.rs

@@ -1479,6 +1479,28 @@ fn help_works_with_invalid_config() {
         .stdout(predicate::str::contains("A cat(1) clone with wings"));
 }
 
+#[test]
+fn help_uses_valid_config() {
+    let tmp_dir = tempdir().expect("can create temporary directory");
+    let tmp_config_path = tmp_dir.path().join("valid-config.conf");
+
+    // Write a valid config file with an invalid theme (so we can see a warning)
+    std::fs::write(&tmp_config_path, "--theme=NonExistentThemeName123")
+        .expect("can write config file");
+
+    // --help should read the config file and try to use the theme
+    // (we'll see a warning about unknown theme. This is the easiest way to prove the theme is read from config.)
+    bat_with_config()
+        .env("BAT_CONFIG_PATH", tmp_config_path.to_str().unwrap())
+        .arg("--help")
+        .assert()
+        .success()
+        .stdout(predicate::str::contains(
+            "A cat(1) clone with syntax highlighting",
+        ))
+        .stderr(predicate::str::contains("Unknown theme"));
+}
+
 #[test]
 fn version_works_with_invalid_config() {
     let tmp_dir = tempdir().expect("can create temporary directory");