Repository: sharkdp/bat
Pull request #2929 (2024-04-07 01:07:24)

Merge `--style`s from different places using `+`/`-`
This commit aims to solve #1032, #1161, #1741, and #2894 by introducing a mechanism for merging the styles specified in the config file, `BAT_STYLE` environment variable, and the `--style` command-line argument.

This does so by treating each `--style` argument as a list of style components, and introducing a new `+` and `-` component prefix (e.g. `+numbers`, `-grid`) that allows `--style` lists to be merged together. When a style list consists only of prefixed components, it will be merged into the list that comes before it. If it contains any non-prefixed components, it will *replace* the list before it instead.

Ultimately, the style lists are merged into a final set of style components roughly following this pseudocode algorithm:

```rust
lists.fold(
    init=set(),
    |accumulator, list| {
        if list.components.has_any(|component| component.prefix is "") {
            accumulator.clear()
        }
        
        for component in list.components {
            if component.prefix == "-" {
                accumulator.remove_all(component.expand())
            } else {
                accumulator.add_all(component.expand())
            }
        }
        
    }
)
```

This preserves the existing behavior of `--style` taking precedence over `BAT_STYLE` or the config file, while also allowing users to add or remove styles from individual runs of `bat`.

Examples:

* `BAT_STYLE=full bat --style=plain` => (none)  
  Overriding the style from the config file or environment variable. (current behavior)
  ![image](https://github.com/sharkdp/bat/assets/32112321/124eec79-5274-4e53-8ffa-4face3465fd3)

* `bat --style=default,-header,-numbers` => `changes,grid,snip`  
  Removing components from the default/full style.
  ![image](https://github.com/sharkdp/bat/assets/32112321/14597ded-56e8-4a68-9174-0f2e3c56d754)

* `BAT_STYLE=full bat --style=-grid` => `changes,header-filename,header-filesize,numbers,snip`  
  Removing components specified from the config file or environment variable.
  ![image](https://github.com/sharkdp/bat/assets/32112321/680db652-8807-476b-873c-b0d9cda900e3)

* `BAT_STYLE=grid,numbers bat --style=+header` => `grid,numbers,header-filename`  
  Adding components from the config file or environment variable. (current behavior)
  ![image](https://github.com/sharkdp/bat/assets/32112321/98bc76e5-c5a0-432b-a4e3-14db566ef5d8)


## Questions & Answers

**Q. Why introduce `+component` instead of using `component` without the prefix?**  
**A.** In part due to technical limitations, and in part to avoid breaking users' existing configurations by preserving the original behavior of `--style`.

**Q. Why did you remove `overrides_with`?**  
**A.** It forced clap's argument matcher to discard all but the most recent occurrence of `--style`.

## Unresolved Questions

* ~~What should we do when there is only one `--style`, and it contains something like `+numbers`? It's currently treated like `--style=numbers`; would it be better to treat it like `default,+numbers`?~~

* ~~Why is [this test](https://github.com/sharkdp/bat/actions/runs/8585106737/job/23526154120?pr=2929) failing under the MSRV toolchain?~~

eth-p: Converting to a draft until I can figure out why one of the tests is failing when building with the MSRV toolchain.
einfachIrgendwer0815: The test run on the MSRV toolchain runs `cargo test` without the `git` feature. Therefore, the style component `full` does not contain `changes`, so less space gets allocated to the sidebar. I commented two suggestions that should fix the error.
eth-p: @einfachIrgendwer0815 Ah, that explains a lot! Thank you so much for pointing that out; it saved me a lot of debugging time. Rebased with your suggestion to fix the failing test :)
eth-p: Rebased on top of https://github.com/sharkdp/bat/commit/b7e44c76dc7bfe1d4bf6693ac20abdbbc48754b9.

Also made a new commit to answer the question of how `--style=+numbers`/`--style=-numbers` would behave if `--style` was only specified once. It was more consistent and intuitive to apply the change to the "default" style, rather than applying it to an empty set of component.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 610af60a..ea47b58f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,7 @@
 - `PrettyPrinter::squeeze_empty_lines` to support line squeezing for bat as a library, see #1441 (@eth-p) and #2665 (@einfachIrgendwer0815)
 - Syntax highlighting for JavaScript files that start with `#!/usr/bin/env bun` #2913 (@sharunkumar)
 - `bat --strip-ansi={never,always,auto}` to remove ANSI escape sequences from bat's input, see #2999 (@eth-p)
+- Add or remove individual style components without replacing all styles #2929 (@eth-p)
 
 ## Bugfixes
 
diff --git a/README.md b/README.md
index 1e42e501..016fe834 100644
--- a/README.md
+++ b/README.md
@@ -515,6 +515,16 @@ and line numbers but no grid and no file header. Set the `BAT_STYLE` environment
 variable to make these changes permanent or use `bat`s
 [configuration file](https://github.com/sharkdp/bat#configuration-file).
 
+>[!tip]
+> If you specify a default style in `bat`'s config file, you can change which components
+> are displayed during a single run of `bat` using the `--style` command-line argument.
+> By prefixing a component with `+` or `-`, it can be added or removed from the current style.
+>
+> For example, if your config contains `--style=full,-snip`, you can run bat with
+> `--style=-grid,+snip` to remove the grid and add back the `snip` component.
+> Or, if you want to override the styles completely, you use `--style=numbers` to
+> only show the line numbers.
+
 ### Adding new syntaxes / language definitions
 
 Should you find that a particular syntax is not available within `bat`, you can follow these
diff --git a/doc/long-help.txt b/doc/long-help.txt
index d9cdce39..2b03490f 100644
--- a/doc/long-help.txt
+++ b/doc/long-help.txt
@@ -134,6 +134,12 @@ Options:
           set a default style, add the '--style=".."' option to the configuration file or export the
           BAT_STYLE environment variable (e.g.: export BAT_STYLE="..").
           
+          When styles are specified in multiple places, the "nearest" set of styles take precedence.
+          The command-line arguments are the highest priority, followed by the BAT_STYLE environment
+          variable, and then the configuration file. If any set of styles consists entirely of
+          components prefixed with "+" or "-", it will modify the previous set of styles instead of
+          replacing them.
+          
           By default, the following components are enabled:
             changes, grid, header-filename, numbers, snip
           
diff --git a/src/bin/bat/app.rs b/src/bin/bat/app.rs
index 0d2600b2..d6628668 100644
--- a/src/bin/bat/app.rs
+++ b/src/bin/bat/app.rs
@@ -2,11 +2,13 @@ use std::collections::HashSet;
 use std::env;
 use std::io::IsTerminal;
 use std::path::{Path, PathBuf};
+use std::str::FromStr;
 
 use crate::{
     clap_app,
     config::{get_args_from_config_file, get_args_from_env_opts_var, get_args_from_env_vars},
 };
+use bat::style::StyleComponentList;
 use bat::StripAnsiMode;
 use clap::ArgMatches;
 
@@ -86,7 +88,6 @@ impl App {
 
             // .. and the rest at the end
             cli_args.for_each(|a| args.push(a));
-
             args
         };
 
@@ -364,34 +365,57 @@ impl App {
         Ok(file_input)
     }
 
+    fn forced_style_components(&self) -> Option<StyleComponents> {
+        // No components if `--decorations=never``.
+        if self
+            .matches
+            .get_one::<String>("decorations")
+            .map(|s| s.as_str())
+            == Some("never")
+        {
+            return Some(StyleComponents(HashSet::new()));
+        }
+
+        // Only line numbers if `--number`.
+        if self.matches.get_flag("number") {
+            return Some(StyleComponents(HashSet::from([
+                StyleComponent::LineNumbers,
+            ])));
+        }
+
+        // Plain if `--plain` is specified at least once.
+        if self.matches.get_count("plain") > 0 {
+            return Some(StyleComponents(HashSet::from([StyleComponent::Plain])));
+        }
+
+        // Default behavior.
+        None
+    }
+
     fn style_components(&self) -> Result<StyleComponents> {
         let matches = &self.matches;
-        let mut styled_components = StyleComponents(
-            if matches.get_one::<String>("decorations").map(|s| s.as_str()) == Some("never") {
-                HashSet::new()
-            } else if matches.get_flag("number") {
-                [StyleComponent::LineNumbers].iter().cloned().collect()
-            } else if 0 < matches.get_count("plain") {
-                [StyleComponent::Plain].iter().cloned().collect()
-            } else {
-                matches
-                    .get_one::<String>("style")
-                    .map(|styles| {
-                        styles
-                            .split(',')
-                            .map(|style| style.parse::<StyleComponent>())
-                            .filter_map(|style| style.ok())
-                            .collect::<Vec<_>>()
-                    })
-                    .unwrap_or_else(|| vec![StyleComponent::Default])
+        let mut styled_components = match self.forced_style_components() {
+            Some(forced_components) => forced_components,
+
+            // Parse the `--style` arguments and merge them.
+            None if matches.contains_id("style") => {
+                let lists = matches
+                    .get_many::<String>("style")
+                    .expect("styles present")
+                    .map(|v| StyleComponentList::from_str(v))
+                    .collect::<Result<Vec<StyleComponentList>>>()?;
+
+                StyleComponentList::to_components(lists, self.interactive_output, true)
+            }
+
+            // Use the default.
+            None => StyleComponents(HashSet::from_iter(
+                StyleComponent::Default
+                    .components(self.interactive_output)
                     .into_iter()
-                    .map(|style| style.components(self.interactive_output))
-                    .fold(HashSet::new(), |mut acc, components| {
-                        acc.extend(components.iter().cloned());
-                        acc
-                    })
-            },
-        );
+                    .cloned(),
+            )),
+        };
 
         // If `grid` is set, remove `rule` as it is a subset of `grid`, and print a warning.
         if styled_components.grid() && styled_components.0.remove(&StyleComponent::Rule) {
diff --git a/src/bin/bat/clap_app.rs b/src/bin/bat/clap_app.rs
index e70b1a5b..33dde980 100644
--- a/src/bin/bat/clap_app.rs
+++ b/src/bin/bat/clap_app.rs
@@ -1,9 +1,11 @@
+use bat::style::StyleComponentList;
 use clap::{
     crate_name, crate_version, value_parser, Arg, ArgAction, ArgGroup, ColorChoice, Command,
 };
 use once_cell::sync::Lazy;
 use std::env;
 use std::path::{Path, PathBuf};
+use std::str::FromStr;
 
 static VERSION: Lazy<String> = Lazy::new(|| {
     #[cfg(feature = "bugreport")]
@@ -419,34 +421,13 @@ pub fn build_app(interactive_output: bool) -> Command {
         .arg(
             Arg::new("style")
                 .long("style")
+                .action(ArgAction::Append)
                 .value_name("components")
-                .overrides_with("style")
-                .overrides_with("plain")
-                .overrides_with("number")
                 // Cannot use claps built in validation because we have to turn off clap's delimiters
                 .value_parser(|val: &str| {
-                    let mut invalid_vals = val.split(',').filter(|style| {
-                        !&[
-                            "auto",
-                            "full",
-                            "default",
-                            "plain",
-                            "header",
-                            "header-filename",
-                            "header-filesize",
-                            "grid",
-                            "rule",
-                            "numbers",
-                            "snip",
-                            #[cfg(feature = "git")]
-                            "changes",
-                        ].contains(style)
-                    });
-
-                    if let Some(invalid) = invalid_vals.next() {
-                        Err(format!("Unknown style, '{invalid}'"))
-                    } else {
-                        Ok(val.to_owned())
+                    match StyleComponentList::from_str(val) {
+                        Err(err) => Err(err),
+                        Ok(_) => Ok(val.to_owned()),
                     }
                 })
                 .help(
@@ -461,6 +442,12 @@ pub fn build_app(interactive_output: bool) -> Command {
                      pre-defined style ('full'). To set a default style, add the \
                      '--style=\"..\"' option to the configuration file or export the \
                      BAT_STYLE environment variable (e.g.: export BAT_STYLE=\"..\").\n\n\
+                     When styles are specified in multiple places, the \"nearest\" set \
+                     of styles take precedence. The command-line arguments are the highest \
+                     priority, followed by the BAT_STYLE environment variable, and then \
+                     the configuration file. If any set of styles consists entirely of \
+                     components prefixed with \"+\" or \"-\", it will modify the \
+                     previous set of styles instead of replacing them.\n\n\
                      By default, the following components are enabled:\n  \
                         changes, grid, header-filename, numbers, snip\n\n\
                      Possible values:\n\n  \
diff --git a/src/bin/bat/config.rs b/src/bin/bat/config.rs
index 9e38dfa4..6fa18f09 100644
--- a/src/bin/bat/config.rs
+++ b/src/bin/bat/config.rs
@@ -146,8 +146,11 @@ pub fn get_args_from_env_vars() -> Vec<OsString> {
         ("--style", "BAT_STYLE"),
     ]
     .iter()
-    .filter_map(|(flag, key)| env::var(key).ok().map(|var| [flag.to_string(), var]))
-    .flatten()
+    .filter_map(|(flag, key)| {
+        env::var(key)
+            .ok()
+            .map(|var| [flag.to_string(), var].join("="))
+    })
     .map(|a| a.into())
     .collect()
 }
diff --git a/src/style.rs b/src/style.rs
index 652b3743..b8d8b09f 100644
--- a/src/style.rs
+++ b/src/style.rs
@@ -138,3 +138,227 @@ impl StyleComponents {
         self.0.clear();
     }
 }
+
+#[derive(Debug, PartialEq)]
+enum ComponentAction {
+    Override,
+    Add,
+    Remove,
+}
+
+impl ComponentAction {
+    fn extract_from_str(string: &str) -> (ComponentAction, &str) {
+        match string.chars().next() {
+            Some('-') => (ComponentAction::Remove, string.strip_prefix('-').unwrap()),
+            Some('+') => (ComponentAction::Add, string.strip_prefix('+').unwrap()),
+            _ => (ComponentAction::Override, string),
+        }
+    }
+}
+
+/// A list of [StyleComponent] that can be parsed from a string.
+pub struct StyleComponentList(Vec<(ComponentAction, StyleComponent)>);
+
+impl StyleComponentList {
+    fn expand_into(&self, components: &mut HashSet<StyleComponent>, interactive_terminal: bool) {
+        for (action, component) in self.0.iter() {
+            let subcomponents = component.components(interactive_terminal);
+
+            use ComponentAction::*;
+            match action {
+                Override | Add => components.extend(subcomponents),
+                Remove => components.retain(|c| !subcomponents.contains(c)),
+            }
+        }
+    }
+
+    /// Returns `true` if any component in the list was not prefixed with `+` or `-`.
+    fn contains_override(&self) -> bool {
+        self.0.iter().any(|(a, _)| *a == ComponentAction::Override)
+    }
+
+    /// Combines multiple [StyleComponentList]s into a single [StyleComponents] set.
+    ///
+    /// ## Precedence
+    /// The most recent list will take precedence and override all previous lists
+    /// unless it only contains components prefixed with `-` or `+`. When this
+    /// happens, the list's components will be merged into the previous list.
+    ///
+    /// ## Example
+    /// ```text
+    /// [numbers,grid] + [header,changes]  -> [header,changes]
+    /// [numbers,grid] + [+header,-grid]   -> [numbers,header]
+    /// ```
+    ///
+    /// ## Parameters
+    ///  - `with_default`: If true, the styles lists will build upon the StyleComponent::Auto style.
+    pub fn to_components(
+        lists: impl IntoIterator<Item = StyleComponentList>,
+        interactive_terminal: bool,
+        with_default: bool,
+    ) -> StyleComponents {
+        let mut components: HashSet<StyleComponent> = HashSet::new();
+        if with_default {
+            components.extend(StyleComponent::Auto.components(interactive_terminal))
+        }
+
+        StyleComponents(lists.into_iter().fold(components, |mut components, list| {
+            if list.contains_override() {
+                components.clear();
+            }
+
+            list.expand_into(&mut components, interactive_terminal);
+            components
+        }))
+    }
+}
+
+impl Default for StyleComponentList {
+    fn default() -> Self {
+        StyleComponentList(vec![(ComponentAction::Override, StyleComponent::Default)])
+    }
+}
+
+impl FromStr for StyleComponentList {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        Ok(StyleComponentList(
+            s.split(",")
+                .map(|s| ComponentAction::extract_from_str(s)) // If the component starts with "-", it's meant to be removed
+                .map(|(a, s)| Ok((a, StyleComponent::from_str(s)?)))
+                .collect::<Result<Vec<(ComponentAction, StyleComponent)>>>()?,
+        ))
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use std::collections::HashSet;
+    use std::str::FromStr;
+
+    use super::ComponentAction::*;
+    use super::StyleComponent;
+    use super::StyleComponent::*;
+    use super::StyleComponentList;
+
+    #[test]
+    pub fn style_component_list_parse() {
+        assert_eq!(
+            StyleComponentList::from_str("grid,+numbers,snip,-snip,header")
+                .expect("no error")
+                .0,
+            vec![
+                (Override, Grid),
+                (Add, LineNumbers),
+                (Override, Snip),
+                (Remove, Snip),
+                (Override, Header),
+            ]
+        );
+
+        assert!(StyleComponentList::from_str("not-a-component").is_err());
+        assert!(StyleComponentList::from_str("grid,not-a-component").is_err());
+        assert!(StyleComponentList::from_str("numbers,-not-a-component").is_err());
+    }
+
+    #[test]
+    pub fn style_component_list_to_components() {
+        assert_eq!(
+            StyleComponentList::to_components(
+                vec![StyleComponentList::from_str("grid,numbers").expect("no error")],
+                false,
+                false
+            )
+            .0,
+            HashSet::from([Grid, LineNumbers])
+        );
+    }
+
+    #[test]
+    pub fn style_component_list_to_components_removes_negated() {
+        assert_eq!(
+            StyleComponentList::to_components(
+                vec![StyleComponentList::from_str("grid,numbers,-grid").expect("no error")],
+                false,
+                false
+            )
+            .0,
+            HashSet::from([LineNumbers])
+        );
+    }
+
+    #[test]
+    pub fn style_component_list_to_components_expands_subcomponents() {
+        assert_eq!(
+            StyleComponentList::to_components(
+                vec![StyleComponentList::from_str("full").expect("no error")],
+                false,
+                false
+            )
+            .0,
+            HashSet::from_iter(Full.components(true).to_owned())
+        );
+    }
+
+    #[test]
+    pub fn style_component_list_expand_negates_subcomponents() {
+        assert!(!StyleComponentList::to_components(
+            vec![StyleComponentList::from_str("full,-numbers").expect("no error")],
+            true,
+            false
+        )
+        .numbers());
+    }
+
+    #[test]
+    pub fn style_component_list_to_components_precedence_overrides_previous_lists() {
+        assert_eq!(
+            StyleComponentList::to_components(
+                vec![
+                    StyleComponentList::from_str("grid").expect("no error"),
+                    StyleComponentList::from_str("numbers").expect("no error"),
+                ],
+                false,
+                false
+            )
+            .0,
+            HashSet::from([LineNumbers])
+        );
+    }
+
+    #[test]
+    pub fn style_component_list_to_components_precedence_merges_previous_lists() {
+        assert_eq!(
+            StyleComponentList::to_components(
+                vec![
+                    StyleComponentList::from_str("grid,header").expect("no error"),
+                    StyleComponentList::from_str("-grid").expect("no error"),
+                    StyleComponentList::from_str("+numbers").expect("no error"),
+                ],
+                false,
+                false
+            )
+            .0,
+            HashSet::from([HeaderFilename, LineNumbers])
+        );
+    }
+
+    #[test]
+    pub fn style_component_list_default_builds_on_auto() {
+        assert_eq!(
+            StyleComponentList::to_components(
+                vec![StyleComponentList::from_str("-numbers").expect("no error"),],
+                true,
+                true
+            )
+            .0,
+            {
+                let mut expected: HashSet<StyleComponent> = HashSet::new();
+                expected.extend(Auto.components(true));
+                expected.remove(&LineNumbers);
+                expected
+            }
+        );
+    }
+}


Test patch:
diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs
index d6009361..8df4327c 100644
--- a/tests/integration_tests.rs
+++ b/tests/integration_tests.rs
@@ -2810,3 +2810,71 @@ fn strip_ansi_auto_does_not_strip_ansi_when_plain_text_by_option() {
 
     assert!(output.contains("\x1B[33mYellow"))
 }
+
+// Tests that style components can be removed with `-component`.
+#[test]
+fn style_components_can_be_removed() {
+    bat()
+        .arg({
+            #[cfg(not(feature = "git"))]
+            {
+                "--style=full,-grid"
+            }
+            #[cfg(feature = "git")]
+            {
+                "--style=full,-grid,-changes"
+            }
+        })
+        .arg("--decorations=always")
+        .arg("--color=never")
+        .write_stdin("test")
+        .assert()
+        .success()
+        .stdout("     STDIN\n     Size: -\n   1 test\n")
+        .stderr("");
+}
+
+// Tests that style components are chosen based on the rightmost `--style` argument.
+#[test]
+fn style_components_can_be_overidden() {
+    bat()
+        .arg("--style=full")
+        .arg("--style=header,numbers")
+        .arg("--decorations=always")
+        .arg("--color=never")
+        .write_stdin("test")
+        .assert()
+        .success()
+        .stdout("     STDIN\n   1 test\n")
+        .stderr("");
+}
+
+// Tests that style components can be merged across multiple `--style` arguments.
+#[test]
+fn style_components_will_merge() {
+    bat()
+        .arg("--style=header,grid")
+        .arg("--style=-grid,+numbers")
+        .arg("--decorations=always")
+        .arg("--color=never")
+        .write_stdin("test")
+        .assert()
+        .success()
+        .stdout("     STDIN\n   1 test\n")
+        .stderr("");
+}
+
+// Tests that style components can be merged with the `BAT_STYLE` environment variable.
+#[test]
+fn style_components_will_merge_with_env_var() {
+    bat()
+        .env("BAT_STYLE", "header,grid")
+        .arg("--style=-grid,+numbers")
+        .arg("--decorations=always")
+        .arg("--color=never")
+        .write_stdin("test")
+        .assert()
+        .success()
+        .stdout("     STDIN\n   1 test\n")
+        .stderr("");
+}