Change option value look-up algorithm

2 files changed, 55 insertions, 38 deletions

diff --git a/src/features/mod.rs b/src/features/mod.rs
index 61043a46..9bcccf0d 100644
--- a/src/features/mod.rs
+++ b/src/features/mod.rs
@@ -123,20 +123,13 @@ pub mod tests {
     fn test_feature() {
         let git_config_contents = b"
 [delta]
-    minus-style = blue
+
 
 [delta \"my-feature\"]
     minus-style = green
 ";
         let git_config_path = "delta__test_feature.gitconfig";
 
-        // Without --features the main section takes effect
-        assert_eq!(
-            make_config(&[], Some(git_config_contents), Some(git_config_path)).minus_style,
-            make_style("blue")
-        );
-
-        // With --features the feature takes effect
         assert_eq!(
             make_config(
                 &["--features", "my-feature"],
@@ -150,19 +143,23 @@ pub mod tests {
     }
 
     #[test]
-    fn test_multiple_features() {
+    fn test_main_section_overrides_feature() {
         let git_config_contents = b"
 [delta]
     minus-style = blue
 
 [delta \"my-feature-1\"]
     minus-style = green
-
-[delta \"my-feature-2\"]
-    minus-style = yellow
 ";
-        let git_config_path = "delta__test_multiple_features.gitconfig";
+        let git_config_path = "delta__test_main_section_overrides_feature.gitconfig";
+
+        // Without --features the main section takes effect
+        assert_eq!(
+            make_config(&[], Some(git_config_contents), Some(git_config_path)).minus_style,
+            make_style("blue")
+        );
 
+        // Event with --features the main section overrides the feature.
         assert_eq!(
             make_config(
                 &["--features", "my-feature-1"],
@@ -170,8 +167,24 @@ pub mod tests {
                 Some(git_config_path),
             )
             .minus_style,
-            make_style("green")
+            make_style("blue")
         );
+        remove_file(git_config_path).unwrap();
+    }
+
+    #[test]
+    fn test_multiple_features() {
+        let git_config_contents = b"
+[delta]
+
+
+[delta \"my-feature-1\"]
+    minus-style = green
+
+[delta \"my-feature-2\"]
+    minus-style = yellow
+";
+        let git_config_path = "delta__test_multiple_features.gitconfig";
 
         assert_eq!(
             make_config(
@@ -199,9 +212,6 @@ pub mod tests {
     #[test]
     fn test_invalid_features() {
         let git_config_contents = b"
-[delta]
-    minus-style = blue
-
 [delta \"my-feature-1\"]
     minus-style = green
 
@@ -210,6 +220,10 @@ pub mod tests {
 ";
         let git_config_path = "delta__test_invalid_features.gitconfig";
 
+        let default = make_config(&[], None, None).minus_style;
+        assert_ne!(default, make_style("green"));
+        assert_ne!(default, make_style("yellow"));
+
         assert_eq!(
             make_config(
                 &["--features", "my-feature-1"],
@@ -227,7 +241,7 @@ pub mod tests {
                 Some(git_config_path),
             )
             .minus_style,
-            make_style("blue")
+            default
         );
 
         assert_eq!(
@@ -307,14 +321,21 @@ pub mod tests {
             make_style("reverse red")
         );
 
-        // No command line argument; main [delta] section wins
+        // No command line argument or features; main [delta] section wins
         assert_eq!(
             make_config(&[], Some(git_config_contents), Some(git_config_path))
                 .whitespace_error_style,
             make_style("blue reverse")
         );
 
-        // No command line argument; feature section wins
+        // Feature contains key, but main [delta] section still wins.
+        // This is equivalent to
+        //
+        // [delta]
+        //     features = my-whitespace-error-style-feature
+        //     whitespace-error-style = blue reverse
+        //
+        // In this situation, the value from the feature is overridden.
         assert_eq!(
             make_config(
                 &["--features", "my-whitespace-error-style-feature"],
@@ -322,7 +343,7 @@ pub mod tests {
                 Some(git_config_path)
             )
             .whitespace_error_style,
-            make_style("reverse green")
+            make_style("reverse blue")
         );
 
         remove_file(git_config_path).unwrap();
diff --git a/src/get_option_value.rs b/src/get_option_value.rs
index 483e6153..5939258c 100644
--- a/src/get_option_value.rs
+++ b/src/get_option_value.rs
@@ -8,20 +8,20 @@ use ProvenancedOptionValue::*;
 
 /// Look up a value of type `T` associated with `option name`. The search rules are:
 ///
-/// 1. For each feature in the ordered list of enabled features:
+/// 1. If there is a value associated with `option_name` in the main [delta] git config
+///    section, then stop searching and return that value.
+///
+/// 2. For each feature in the ordered list of enabled features:
 ///
-///    1.1 Look-up the value, treating `feature` as a custom feature.
+///    2.1 Look-up the value, treating `feature` as a custom feature.
 ///        I.e., if there is a value associated with `option_name` in a git config section
 ///        named [delta "`feature`"] then stop searching and return that value.
 ///
-///    1.2 Look-up the value, treating `feature` as a builtin feature.
+///    2.2 Look-up the value, treating `feature` as a builtin feature.
 ///        I.e., if there is a value (not a default value) associated with `option_name` in a
 ///        builtin feature named `feature`, then stop searching and return that value.
 ///        Otherwise, record the default value and continue searching.
 ///
-/// 2. If there is a value associated with `option_name` in the main [delta] git config
-///    section, then stop searching and return that value.
-///
 /// 3. Return the last default value that was encountered.
 pub fn get_option_value<T>(
     option_name: &str,
@@ -51,7 +51,11 @@ pub trait GetOptionValue {
         Self: From<OptionValue>,
         Self: Into<OptionValue>,
     {
-        let mut default = None;
+        if let Some(git_config) = git_config {
+            if let Some(value) = git_config.get::<Self>(&format!("delta.{}", option_name)) {
+                return Some(value);
+            }
+        }
         if let Some(features) = &opt.features {
             for feature in features.to_lowercase().split_whitespace().rev() {
                 match Self::get_provenanced_value_for_feature(
@@ -61,22 +65,14 @@ pub trait GetOptionValue {
                     opt,
                     git_config,
                 ) {
-                    Some(GitConfigValue(value)) => {
+                    Some(GitConfigValue(value)) | Some(DefaultValue(value)) => {
                         return Some(value.into());
                     }
-                    Some(DefaultValue(value)) => {
-                        default = Some(value.into());
-                    }
                     None => {}
                 }
             }
         }
-        if let Some(git_config) = git_config {
-            if let Some(value) = git_config.get::<Self>(&format!("delta.{}", option_name)) {
-                return Some(value);
-            }
-        }
-        default
+        None
     }
 
     /// Return the value, or default value, associated with `option_name` under feature name