Gather features recursively

3 files changed, 209 insertions, 28 deletions

diff --git a/src/features/mod.rs b/src/features/mod.rs
index 912bbb98..67506ef8 100644
--- a/src/features/mod.rs
+++ b/src/features/mod.rs
@@ -100,6 +100,22 @@ pub mod tests {
     }
 
     #[test]
+    fn test_builtin_feature_from_gitconfig() {
+        let git_config_contents = b"
+[delta]
+    navigate = true
+";
+        let git_config_path = "delta__test_builtin_feature_from_gitconfig.gitconfig";
+
+        assert_eq!(
+            make_options(&[], Some(git_config_contents), Some(git_config_path)).features,
+            "navigate"
+        );
+
+        remove_file(git_config_path).unwrap();
+    }
+
+    #[test]
     fn test_features_on_command_line_replace_features_in_gitconfig() {
         let git_config_contents = b"
 [delta]
@@ -152,6 +168,57 @@ pub mod tests {
     }
 
     #[test]
+    fn test_recursive_feature_gathering_1() {
+        let git_config_contents = b"
+[delta]
+    features = h g
+
+[delta \"a\"]
+    features = c b
+    diff-highlight = true
+
+[delta \"d\"]
+    features = f e
+    diff-so-fancy = true
+";
+        let git_config_path = "delta__test_feature_collection.gitconfig";
+
+        assert_eq!(
+            make_options(
+                &["--color-only", "--features", "d a"],
+                Some(git_config_contents),
+                Some(git_config_path),
+            )
+            .features,
+            "color-only diff-so-fancy f e d diff-highlight c b a"
+        );
+
+        remove_file(git_config_path).unwrap();
+    }
+
+    #[test]
+    fn test_recursive_feature_gathering_2() {
+        let git_config_contents = b"
+[delta]
+    features = feature-1
+
+[delta \"feature-1\"]
+    features = feature-2 feature-3
+
+[delta \"feature-2\"]
+    features = feature-4
+
+[delta \"feature-4\"]
+    minus-style = blue
+";
+        let git_config_path = "delta__test_recursive_features.gitconfig";
+        let opt = make_options(&["delta"], Some(git_config_contents), Some(git_config_path));
+        assert_eq!(opt.features, "feature-4 feature-2 feature-3 feature-1");
+
+        remove_file(git_config_path).unwrap();
+    }
+
+    #[test]
     fn test_main_section() {
         let git_config_contents = b"
 [delta]
diff --git a/src/features/navigate.rs b/src/features/navigate.rs
index 001c8bc5..8abef729 100644
--- a/src/features/navigate.rs
+++ b/src/features/navigate.rs
@@ -85,7 +85,7 @@ mod tests {
     fn test_navigate_activated_by_custom_feature() {
         let git_config_contents = b"
 [delta \"my-navigate-feature\"]
-    navigate = true
+    features = navigate
     file-modified-label = \"modified: \"
 ";
         let git_config_path = "delta__test_navigate_activated_by_custom_feature.gitconfig";
diff --git a/src/set_options.rs b/src/set_options.rs
index 9c8587af..53a353ac 100644
--- a/src/set_options.rs
+++ b/src/set_options.rs
@@ -1,13 +1,10 @@
-use std::collections::HashMap;
-use std::collections::HashSet;
-use structopt::clap;
+use std::collections::VecDeque;
 
-use itertools::Itertools;
+use structopt::clap;
 
 use crate::cli;
 use crate::config;
 use crate::features;
-use crate::get_option_value::get_option_value;
 use crate::git_config;
 
 macro_rules! set_options {
@@ -29,7 +26,12 @@ pub fn set_options(
     arg_matches: &clap::ArgMatches,
 ) {
     let builtin_features = features::make_builtin_features();
-    set_features(opt, git_config, &builtin_features);
+    opt.features = gather_features(
+        opt,
+        builtin_features.keys().into_iter().collect(),
+        git_config,
+    );
+
     // Handle options which default to an arbitrary git config value.
     // TODO: incorporate this logic into the set_options macro.
     if !config::user_supplied_option("whitespace-error-style", arg_matches) {
@@ -97,39 +99,151 @@ pub fn set_options(
     );
 }
 
-fn set_features(
-    opt: &mut cli::Opt,
-    git_config: &mut Option<git_config::GitConfig>,
-    builtin_features: &HashMap<String, features::BuiltinFeature>,
-) {
+/// Features are processed differently from all other options. The role of this function is to
+/// collect all configuration related to features and summarize it as a single list
+/// (space-separated string) of enabled features. The list is arranged in order of increasing
+/// priority in the sense that, when searching for a option value, one starts at the right-hand end
+/// and moves leftward, examining each feature in turn until a feature that associates a value with
+/// the option name is encountered. This search is documented in
+/// `get_option_value::get_option_value`.
+///
+/// The feature list comprises features deriving from the following sources, listed in order of
+/// decreasing priority:
+///
+/// 1. Suppose the command-line has `--features "a b"`. Then
+///    - `b`, followed by b's "ordered descendents"
+///    - `a`, followed by a's "ordered descendents"
+///
+/// 2. Suppose the command line enables two builtin features via `--navigate --diff-so-fancy`. Then
+///    - `diff-so-fancy`
+///    - `navigate`
+///
+/// 3. Suppose the main [delta] section has `features = d e`. Then
+///    - `e`, followed by e's "ordered descendents"
+///    - `d`, followed by d's "ordered descendents"
+///
+/// 4. Suppose the main [delta] section has `diff-highlight = true` followed by `color-only = true`.
+///    Then
+///    - `diff-highlight`
+///    - `color-only`
+///
+/// The "ordered descendents" of a feature `f` is a list of features obtained via a pre-order
+/// traversal of the feature tree rooted at `f`. This tree arises because it is allowed for a
+/// feature to contain a (key, value) pair that itself enables features.
+///
+/// If a feature has already been included at higher priority, and is encountered again, it is
+/// ignored.
+///
+/// Thus, for example:
+///
+/// delta --features "my-navigate-settings" --navigate   =>   "navigate my-navigate-settings"
+///
+/// In the following configuration, the feature names indicate their priority, with `a` having
+/// highest priority:
+///
+/// delta --g --features "d a"
+///
+/// [delta "a"]
+///     features = c b
+///
+/// [delta "d"]
+///     features = f e
+fn gather_features<'a>(
+    opt: &cli::Opt,
+    builtin_feature_names: Vec<&String>,
+    git_config: &Option<git_config::GitConfig>,
+) -> String {
+    let mut features = VecDeque::new();
+
+    // Gather features from command line.
+    if let Some(git_config) = git_config {
+        for feature in split_feature_string(&opt.features.to_lowercase()) {
+            gather_features_recursively(feature, &mut features, &builtin_feature_names, git_config);
+        }
+    } else {
+        for feature in split_feature_string(&opt.features.to_lowercase()) {
+            features.push_front(feature.to_string());
+        }
+    }
+
+    // Gather builtin feature flags supplied on command line.
+    // TODO: Iterate over programatically-obtained names of builtin features.
     if opt.color_only {
-        opt.features = format!("{} color-only", opt.features);
+        features.push_front("color-only".to_string());
     }
     if opt.diff_highlight {
-        opt.features = format!("{} diff-highlight", opt.features);
+        features.push_front("diff-highlight".to_string());
     }
     if opt.diff_so_fancy {
-        opt.features = format!("{} diff-so-fancy", opt.features);
+        features.push_front("diff-so-fancy".to_string());
     }
     if opt.navigate {
-        opt.features = format!("{} navigate", opt.features);
+        features.push_front("navigate".to_string());
     }
 
-    if let Some(more_features) =
-        get_option_value::<String>("features", builtin_features, opt, git_config)
-    {
-        opt.features = append_features(&opt.features, &more_features);
+    if let Some(git_config) = git_config {
+        // Gather features from [delta] section if --features was not passed.
+        if opt.features.is_empty() {
+            if let Some(feature_string) = git_config.get::<String>(&format!("delta.features")) {
+                for feature in split_feature_string(&feature_string.to_lowercase()) {
+                    gather_features_recursively(
+                        feature,
+                        &mut features,
+                        &builtin_feature_names,
+                        git_config,
+                    )
+                }
+            }
+        }
+        // Always gather builtin feature flags from [delta] section.
+        gather_builtin_features("delta", &mut features, &builtin_feature_names, git_config);
     }
+
+    Vec::<String>::from(features).join(" ")
 }
 
-fn append_features(features: &str, more_features: &str) -> String {
-    let feature_set: HashSet<_> = features.split_whitespace().collect();
+fn gather_features_recursively<'a>(
+    feature: &str,
+    features: &mut VecDeque<String>,
+    builtin_feature_names: &Vec<&String>,
+    git_config: &git_config::GitConfig,
+) {
+    features.push_front(feature.to_string());
+    if let Some(child_features) = git_config.get::<String>(&format!("delta.{}.features", feature)) {
+        for child_feature in split_feature_string(&child_features) {
+            if !features.contains(&child_feature.to_string()) {
+                gather_features_recursively(
+                    child_feature,
+                    features,
+                    builtin_feature_names,
+                    git_config,
+                )
+            }
+        }
+    }
+    gather_builtin_features(
+        &format!("delta.{}", feature),
+        features,
+        builtin_feature_names,
+        git_config,
+    );
+}
 
-    let more_features = more_features
-        .to_lowercase()
-        .split_whitespace()
-        .filter(|s| !feature_set.contains(s))
-        .join(" ");
+fn gather_builtin_features<'a>(
+    git_config_key: &str,
+    features: &mut VecDeque<String>,
+    builtin_feature_names: &Vec<&String>,
+    git_config: &git_config::GitConfig,
+) {
+    for feature in builtin_feature_names {
+        if let Some(value) = git_config.get::<bool>(&format!("{}.{}", git_config_key, feature)) {
+            if value {
+                features.push_front(feature.to_string());
+            }
+        }
+    }
+}
 
-    [features, &more_features].join(" ")
+fn split_feature_string(features: &str) -> impl Iterator<Item = &str> {
+    features.split_whitespace().rev()
 }