Refactor: non-String builtin-preset values

3 files changed, 177 insertions, 155 deletions

diff --git a/src/git_config.rs b/src/git_config.rs
index dc6091e0..77f4c899 100644
--- a/src/git_config.rs
+++ b/src/git_config.rs
@@ -1,6 +1,5 @@
 #[cfg(test)]
 use std::path::Path;
-
 use std::process;
 
 use git2;
@@ -73,12 +72,6 @@ impl GitConfigGet for bool {
     }
 }
 
-impl GitConfigGet for i64 {
-    fn git_config_get(key: &str, git_config: &GitConfig) -> Option<Self> {
-        git_config.config.get_i64(key).ok()
-    }
-}
-
 impl GitConfigGet for usize {
     fn git_config_get(key: &str, git_config: &GitConfig) -> Option<Self> {
         match git_config.config.get_i64(key) {
diff --git a/src/preset.rs b/src/preset.rs
index b8d8f19d..52178493 100644
--- a/src/preset.rs
+++ b/src/preset.rs
@@ -14,14 +14,22 @@ use crate::git_config::GitConfig;
 /// A builtin preset is a named set of command line (option, value) pairs that is built in to
 /// delta. The implementation stores each value as a function, which allows the value (a) to depend
 /// dynamically on the value of other command line options, and (b) to be taken from git config.
-// Currently, all values in builtin presets are of type String.
-pub type BuiltinPreset<T> = HashMap<String, PresetValueFunction<T>>;
-type PresetValueFunction<T> = Box<dyn Fn(&cli::Opt, &Option<GitConfig>) -> T>;
+pub type BuiltinPreset = HashMap<String, PresetValueFunction>;
+
+type PresetValueFunction = Box<dyn Fn(&cli::Opt, &Option<GitConfig>) -> OptionValue>;
+
+pub enum OptionValue {
+    Boolean(bool),
+    Float(f64),
+    OptionString(Option<String>),
+    String(String),
+    Int(usize),
+}
 
 // Construct a 2-level hash map: (preset name) -> (option name) -> (value function). A value
 // function is a function that takes an Opt struct, and a git Config struct, and returns the value
 // for the option.
-pub fn make_builtin_presets() -> HashMap<String, BuiltinPreset<String>> {
+pub fn make_builtin_presets() -> HashMap<String, BuiltinPreset> {
     vec![
         (
             "diff-highlight".to_string(),
@@ -39,141 +47,211 @@ pub fn make_builtin_presets() -> HashMap<String, BuiltinPreset<String>> {
 /// The macro permits the values of a builtin preset to be specified as either (a) a git config
 /// entry or (b) a value, which may be computed from the other command line options (cli::Opt).
 macro_rules! builtin_preset {
-    ([$( ($option_name:expr, $git_config_key:expr, $opt:ident => $value:expr) ),*]) => {
+    ([$( ($option_name:expr, $type:ty, $git_config_key:expr, $opt:ident => $value:expr) ),*]) => {
         vec![$(
             (
                 $option_name.to_string(),
                 Box::new(move |$opt: &cli::Opt, git_config: &Option<GitConfig>| {
                     match (git_config, $git_config_key) {
-                        (Some(git_config), Some(git_config_key)) => git_config.get::<String>(git_config_key),
+                        (Some(git_config), Some(git_config_key)) => match git_config.get::<$type>(git_config_key) {
+                            Some(value) => Some(value.into()),
+                            _ => None,
+                        },
                         _ => None,
                     }
-                    .unwrap_or_else(|| $value)
-                }) as PresetValueFunction<String>
+                    .unwrap_or_else(|| $value.into())
+                }) as PresetValueFunction
             )
         ),*]
     }
 }
 
-fn _make_diff_highlight_preset<'a>(bold: bool) -> Vec<(String, PresetValueFunction<String>)> {
+fn _make_diff_highlight_preset<'a>(bold: bool) -> Vec<(String, PresetValueFunction)> {
     builtin_preset!([
         (
             "minus-style",
+            String,
             Some("color.diff.old"),
-            _opt => (if bold { "bold red" } else { "red" }).to_string()
+            _opt => if bold { "bold red" } else { "red" }
         ),
         (
             "minus-non-emph-style",
+            String,
             Some("color.diff-highlight.oldNormal"),
             opt => opt.minus_style.clone()
         ),
         (
             "minus-emph-style",
+            String,
             Some("color.diff-highlight.oldHighlight"),
             opt => format!("{} reverse", opt.minus_style)
         ),
         (
             "zero-style",
+            String,
             None,
-            _opt => "normal".to_string()
+            _opt => "normal"
         ),
         (
             "plus-style",
+            String,
             Some("color.diff.new"),
-            _opt => (if bold { "bold green" } else { "green" }).to_string()
+            _opt => if bold { "bold green" } else { "green" }
         ),
         (
             "plus-non-emph-style",
+            String,
             Some("color.diff-highlight.newNormal"),
             opt => opt.plus_style.clone()
         ),
         (
             "plus-emph-style",
+            String,
             Some("color.diff-highlight.newHighlight"),
             opt => format!("{} reverse", opt.plus_style)
         )
     ])
 }
 
-fn make_diff_highlight_preset() -> Vec<(String, PresetValueFunction<String>)> {
+fn make_diff_highlight_preset() -> Vec<(String, PresetValueFunction)> {
     _make_diff_highlight_preset(false)
 }
 
-fn make_diff_so_fancy_preset() -> Vec<(String, PresetValueFunction<String>)> {
+fn make_diff_so_fancy_preset() -> Vec<(String, PresetValueFunction)> {
     let mut preset = _make_diff_highlight_preset(true);
     preset.extend(builtin_preset!([
         (
             "minus-emph-style",
+            String,
             Some("color.diff-highlight.oldHighlight"),
-            _opt => "bold red 52".to_string()
+            _opt => "bold red 52"
         ),
         (
             "plus-emph-style",
+            String,
             Some("color.diff-highlight.newHighlight"),
-            _opt => "bold green 22".to_string()
+            _opt => "bold green 22"
         ),
         (
             "commit-style",
+            String,
             None,
-            _opt => "bold yellow".to_string()
+            _opt => "bold yellow"
         ),
         (
             "commit-decoration-style",
+            String,
             None,
-            _opt => "none".to_string()
+            _opt => "none"
         ),
         (
             "file-style",
+            String,
             Some("color.diff.meta"),
-            _opt => "11".to_string()
+            _opt => "11"
         ),
         (
             "file-decoration-style",
+            String,
             None,
-            _opt => "bold yellow ul ol".to_string()
+            _opt => "bold yellow ul ol"
         ),
         (
             "hunk-header-style",
+            String,
             Some("color.diff.frag"),
-            _opt => "bold syntax".to_string()
+            _opt => "bold syntax"
         ),
         (
             "hunk-header-decoration-style",
+            String,
             None,
-            _opt => "magenta box".to_string()
+            _opt => "magenta box"
         )
     ]));
     preset
 }
 
-// Currently the builtin presets only have String values. The trait is implemented for other types
-// out of necessity.
-pub trait GetValueFunctionFromBuiltinPreset {
-    fn get_value_function_from_builtin_preset<'a>(
-        _option_name: &str,
-        _builtin_preset: &'a BuiltinPreset<String>,
-    ) -> Option<&'a PresetValueFunction<Self>>
-    where
-        Self: Sized,
-    {
-        None
+impl From<bool> for OptionValue {
+    fn from(value: bool) -> Self {
+        OptionValue::Boolean(value)
+    }
+}
+
+impl From<OptionValue> for bool {
+    fn from(value: OptionValue) -> Self {
+        match value {
+            OptionValue::Boolean(value) => value,
+            _ => panic!(),
+        }
+    }
+}
+
+impl From<f64> for OptionValue {
+    fn from(value: f64) -> Self {
+        OptionValue::Float(value)
+    }
+}
+
+impl From<OptionValue> for f64 {
+    fn from(value: OptionValue) -> Self {
+        match value {
+            OptionValue::Float(value) => value,
+            _ => panic!(),
+        }
+    }
+}
+
+impl From<Option<String>> for OptionValue {
+    fn from(value: Option<String>) -> Self {
+        OptionValue::OptionString(value)
     }
 }
 
-impl GetValueFunctionFromBuiltinPreset for String {
-    fn get_value_function_from_builtin_preset<'a>(
-        option_name: &str,
-        builtin_preset: &'a BuiltinPreset<String>,
-    ) -> Option<&'a PresetValueFunction<String>> {
-        builtin_preset.get(option_name)
+impl From<OptionValue> for Option<String> {
+    fn from(value: OptionValue) -> Self {
+        match value {
+            OptionValue::OptionString(value) => value,
+            _ => panic!(),
+        }
     }
 }
 
-impl GetValueFunctionFromBuiltinPreset for bool {}
-impl GetValueFunctionFromBuiltinPreset for i64 {}
-impl GetValueFunctionFromBuiltinPreset for usize {}
-impl GetValueFunctionFromBuiltinPreset for f64 {}
-impl GetValueFunctionFromBuiltinPreset for Option<String> {}
+impl From<String> for OptionValue {
+    fn from(value: String) -> Self {
+        OptionValue::String(value)
+    }
+}
+
+impl From<&str> for OptionValue {
+    fn from(value: &str) -> Self {
+        value.to_string().into()
+    }
+}
+
+impl From<OptionValue> for String {
+    fn from(value: OptionValue) -> Self {
+        match value {
+            OptionValue::String(value) => value,
+            _ => panic!(),
+        }
+    }
+}
+
+impl From<usize> for OptionValue {
+    fn from(value: usize) -> Self {
+        OptionValue::Int(value)
+    }
+}
+
+impl From<OptionValue> for usize {
+    fn from(value: OptionValue) -> Self {
+        match value {
+            OptionValue::Int(value) => value,
+            _ => panic!(),
+        }
+    }
+}
 
 #[cfg(test)]
 mod tests {
diff --git a/src/set_options.rs b/src/set_options.rs
index fcd04914..229b0d48 100644
--- a/src/set_options.rs
+++ b/src/set_options.rs
@@ -4,7 +4,7 @@ use structopt::clap;
 use crate::cli;
 use crate::config;
 use crate::git_config::{self, GitConfigGet};
-use crate::preset::{self, GetValueFunctionFromBuiltinPreset};
+use crate::preset;
 
 // A type T implementing this trait gains a static method allowing an option value of type T to be
 // looked up, implementing delta's rules for looking up option values.
@@ -19,14 +19,14 @@ trait GetOptionValue {
     // 3. The value for n in the main git config section for delta (i.e. git config value delta.$n)
     fn get_option_value(
         option_name: &str,
-        builtin_presets: &HashMap<String, preset::BuiltinPreset<String>>,
+        builtin_presets: &HashMap<String, preset::BuiltinPreset>,
         opt: &cli::Opt,
         git_config: &mut Option<git_config::GitConfig>,
     ) -> Option<Self>
     where
         Self: Sized,
         Self: GitConfigGet,
-        Self: GetValueFunctionFromBuiltinPreset,
+        Self: From<preset::OptionValue>,
     {
         if let Some(presets) = &opt.presets {
             for preset in presets.to_lowercase().split_whitespace().rev() {
@@ -52,14 +52,14 @@ trait GetOptionValue {
     fn get_option_value_for_preset(
         option_name: &str,
         preset: &str,
-        builtin_presets: &HashMap<String, preset::BuiltinPreset<String>>,
+        builtin_presets: &HashMap<String, preset::BuiltinPreset>,
         opt: &cli::Opt,
         git_config: &mut Option<git_config::GitConfig>,
     ) -> Option<Self>
     where
         Self: Sized,
         Self: GitConfigGet,
-        Self: GetValueFunctionFromBuiltinPreset,
+        Self: From<preset::OptionValue>,
     {
         if let Some(git_config) = git_config {
             if let Some(value) =
@@ -69,84 +69,40 @@ trait GetOptionValue {
             }
         }
         if let Some(builtin_preset) = builtin_presets.get(preset) {
-            if let Some(value_function) =
-                Self::get_value_function_from_builtin_preset(option_name, builtin_preset)
-            {
-                return Some(value_function(opt, &git_config));
+            if let Some(value_function) = builtin_preset.get(option_name) {
+                return Some(value_function(opt, &git_config).into());
             }
         }
         return None;
     }
 }
 
+impl GetOptionValue for Option<String> {}
 impl GetOptionValue for String {}
-
-impl GetOptionValue for Option<String> {
-    fn get_option_value(
-        option_name: &str,
-        builtin_presets: &HashMap<String, preset::BuiltinPreset<String>>,
-        opt: &cli::Opt,
-        git_config: &mut Option<git_config::GitConfig>,
-    ) -> Option<Self> {
-        match get_option_value::<String>(option_name, builtin_presets, opt, git_config) {
-            Some(value) => Some(Some(value)),
-            None => None,
-        }
-    }
-}
-
 impl GetOptionValue for bool {}
-
-impl GetOptionValue for i64 {}
-
-impl GetOptionValue for usize {
-    fn get_option_value(
-        option_name: &str,
-        builtin_presets: &HashMap<String, preset::BuiltinPreset<String>>,
-        opt: &cli::Opt,
-        git_config: &mut Option<git_config::GitConfig>,
-    ) -> Option<Self> {
-        match get_option_value::<i64>(option_name, builtin_presets, opt, git_config) {
-            Some(value) => Some(value as usize),
-            None => None,
-        }
-    }
-}
-
-impl GetOptionValue for f64 {
-    fn get_option_value(
-        option_name: &str,
-        builtin_presets: &HashMap<String, preset::BuiltinPreset<String>>,
-        opt: &cli::Opt,
-        git_config: &mut Option<git_config::GitConfig>,
-    ) -> Option<Self> {
-        match get_option_value::<String>(option_name, builtin_presets, opt, git_config) {
-            Some(value) => value.parse::<f64>().ok(),
-            None => None,
-        }
-    }
-}
+impl GetOptionValue for f64 {}
+impl GetOptionValue for usize {}
 
 fn get_option_value<T>(
     option_name: &str,
-    builtin_presets: &HashMap<String, preset::BuiltinPreset<String>>,
+    builtin_presets: &HashMap<String, preset::BuiltinPreset>,
     opt: &cli::Opt,
     git_config: &mut Option<git_config::GitConfig>,
 ) -> Option<T>
 where
     T: GitConfigGet,
     T: GetOptionValue,
-    T: GetValueFunctionFromBuiltinPreset,
+    T: From<preset::OptionValue>,
 {
     T::get_option_value(option_name, builtin_presets, opt, git_config)
 }
 
 macro_rules! set_options {
-	([$( ($option_name:expr, $type:ty, $field_ident:ident) ),* ],
+	([$( ($option_name:expr, $field_ident:ident) ),* ],
      $opt:expr, $builtin_presets:expr, $git_config:expr, $arg_matches:expr) => {
         $(
             if !$crate::config::user_supplied_option($option_name, $arg_matches) {
-                if let Some(value) = get_option_value::<$type>($option_name, &$builtin_presets, $opt, $git_config) {
+                if let Some(value) = get_option_value($option_name, &$builtin_presets, $opt, $git_config) {
                     $opt.$field_ident = value;
                 }
             };
@@ -176,58 +132,53 @@ pub fn set_options(
     set_options!(
         [
             // --presets must be set first
-            ("presets", Option<String>, presets),
-            ("color-only", bool, color_only),
-            ("commit-decoration-style", String, commit_decoration_style),
-            ("commit-style", String, commit_style),
-            ("dark", bool, dark),
-            ("file-added-label", String, file_added_label),
-            ("file-decoration-style", String, file_decoration_style),
-            ("file-modified-label", String, file_modified_label),
-            ("file-removed-label", String, file_removed_label),
-            ("file-renamed-label", String, file_renamed_label),
-            ("file-style", String, file_style),
-            (
-                "hunk-header-decoration-style",
-                String,
-                hunk_header_decoration_style
-            ),
-            ("hunk-header-style", String, hunk_header_style),
-            ("keep-plus-minus-markers", bool, keep_plus_minus_markers),
-            ("light", bool, light),
-            ("max-line-distance", f64, max_line_distance),
+            ("presets", presets),
+            ("color-only", color_only),
+            ("commit-decoration-style", commit_decoration_style),
+            ("commit-style", commit_style),
+            ("dark", dark),
+            ("file-added-label", file_added_label),
+            ("file-decoration-style", file_decoration_style),
+            ("file-modified-label", file_modified_label),
+            ("file-removed-label", file_removed_label),
+            ("file-renamed-label", file_renamed_label),
+            ("file-style", file_style),
+            ("hunk-header-decoration-style", hunk_header_decoration_style),
+            ("hunk-header-style", hunk_header_style),
+            ("keep-plus-minus-markers", keep_plus_minus_markers),
+            ("light", light),
+            ("max-line-distance", max_line_distance),
             // Hack: minus-style must come before minus-*emph-style because the latter default
             // dynamically to the value of the former.
-            ("minus-style", String, minus_style),
-            ("minus-emph-style", String, minus_emph_style),
-            ("minus-empty-line-marker-style", String, minus_empty_line_marker_style),
-            ("minus-non-emph-style", String, minus_non_emph_style),
-            ("navigate", bool, navigate),
-            ("number", bool, show_line_numbers),
-            ("number-minus-format", String, number_minus_format),
+            ("minus-style", minus_style),
+            ("minus-emph-style", minus_emph_style),
             (
-                "number-minus-format-style",
-                String,
-                number_minus_format_style
+                "minus-empty-line-marker-style",
+                minus_empty_line_marker_style
             ),
-            ("number-minus-style", String, number_minus_style),
-            ("number-plus-format", String, number_plus_format),
-            ("number-plus-format-style", String, number_plus_format_style),
-            ("number-plus-style", String, number_plus_style),
-            ("paging-mode", String, paging_mode),
+            ("minus-non-emph-style", minus_non_emph_style),
+            ("navigate", navigate),
+            ("number", show_line_numbers),
+            ("number-minus-format", number_minus_format),
+            ("number-minus-format-style", number_minus_format_style),
+            ("number-minus-style", number_minus_style),
+            ("number-plus-format", number_plus_format),
+            ("number-plus-format-style", number_plus_format_style),
+            ("number-plus-style", number_plus_style),
+            ("paging-mode", paging_mode),
             // Hack: plus-style must come before plus-*emph-style because the latter default
             // dynamically to the value of the former.
-            ("plus-style", String, plus_style),
-            ("plus-emph-style", String, plus_emph_style),
-            ("plus-empty-line-marker-style", String, plus_empty_line_marker_style),
-            ("plus-non-emph-style", String, plus_non_emph_style),
-            ("syntax-theme", Option<String>, syntax_theme),
-            ("tabs", usize, tab_width),
-            ("true-color", String, true_color),
-            ("whitespace-error-style", String, whitespace_error_style),
-            ("width", Option<String>, width),
-            ("word-diff-regex", String, tokenization_regex),
-            ("zero-style", String, zero_style)
+            ("plus-style", plus_style),
+            ("plus-emph-style", plus_emph_style),
+            ("plus-empty-line-marker-style", plus_empty_line_marker_style),
+            ("plus-non-emph-style", plus_non_emph_style),
+            ("syntax-theme", syntax_theme),
+            ("tabs", tab_width),
+            ("true-color", true_color),
+            ("whitespace-error-style", whitespace_error_style),
+            ("width", width),
+            ("word-diff-regex", tokenization_regex),
+            ("zero-style", zero_style)
         ],
         opt,
         preset::make_builtin_presets(),