Fix merge bug in Config::load

Currently `Config::load` merges editor configs with
`merge_toml_values(global, local, 3)`

Besides the puzzling use of the magic number `3`, this is a bug,
because it concatenates arrays from the global config file with
those from the local config file.

For example `editor.shell=["bash", "-c"]` and
`editor.shell=["fish", "-c"]` would become
`editor.shell=["bash", "-c", "fish", "-c"]`.

This commit adds a new function `merge_toml_values_with_strategy` and
changes `Config::load` to call:
```
  merge_toml_values_with_strategy(
    global,
    local,
    &MergeStrategy {
      array: MergeMode::Never,
      table: MergeMode::Always,
    },
  )
```

For backward compatability, the original function `merge_toml_values`
calls:
```
  merge_toml_values_with_strategy(
    left,
    right,
    &MergeStrategy {
      array: MergeMode::MaxDepth(max_merge_depth),
      table: MergeMode::MaxDepth(max_merge_depth),
    },
  )
```
, so there should be no change in the way themes and languages.toml
are merged.

Also update doc string and add tests.

Author: thomie

helix-loader/src/lib.rs

@@ -152,39 +152,102 @@ pub fn default_log_file() -> PathBuf {
     cache_dir().join("helix.log")
 }
 
+pub struct MergeStrategy {
+    pub array: MergeMode,
+    pub table: MergeMode,
+}
+
+pub enum MergeMode {
+    Never,
+    Always,
+    MaxDepth(usize),
+}
+
+impl MergeMode {
+    pub fn should_merge(&self, depth: usize) -> bool {
+        match self {
+            MergeMode::Always => true,
+            MergeMode::MaxDepth(max_depth) => depth < *max_depth,
+            MergeMode::Never => false,
+        }
+    }
+}
+
 /// Merge two TOML documents, merging values from `right` onto `left`
 ///
-/// `merge_depth` sets the nesting depth up to which values are merged instead
-/// of overridden.
+/// `max_merge_depth` sets the nesting depth up to which values are merged
+/// instead of overridden.
+///
+/// When an array exists in both `left` and `right`, the merged array is formed
+/// by concatenating `left`'s elements with `right`'s. But if any elements share
+/// the same `name` field, they are merged recursively and included only once.
 ///
 /// When a table exists in both `left` and `right`, the merged table consists of
 /// all keys in `left`'s table unioned with all keys in `right` with the values
 /// of `right` being merged recursively onto values of `left`.
 ///
+/// Setting `max_merge_depth` is useful for TOML documents that use a
+/// top-level array of values, where the top-level arrays should be merged
+/// but nested arrays should act as overrides. For the `languages.toml`
+/// config for example, this means that you can specify a sub-set of
+/// languages in an overriding `languages.toml` but that nested arrays
+/// like Language Server arguments are replaced instead of merged.
+///
 /// `crate::merge_toml_values(a, b, 3)` combines, for example:
 ///
-/// b:
+/// a:
 /// ```toml
 /// [[language]]
 /// name = "toml"
+/// scope = "source.toml"
 /// language-server = { command = "taplo", args = ["lsp", "stdio"] }
 /// ```
-/// a:
+/// b:
 /// ```toml
 /// [[language]]
+/// name = "toml"
 /// language-server = { command = "/usr/bin/taplo" }
 /// ```
 ///
 /// into:
 /// ```toml
 /// [[language]]
 /// name = "toml"
+/// scope = "source.toml"
 /// language-server = { command = "/usr/bin/taplo" }
 /// ```
 ///
-/// thus it overrides the third depth-level of b with values of a if they exist,
+/// thus it overrides the third depth-level of a with values of b if they exist,
 /// but otherwise merges their values
-pub fn merge_toml_values(left: toml::Value, right: toml::Value, merge_depth: usize) -> toml::Value {
+pub fn merge_toml_values(
+    left: toml::Value,
+    right: toml::Value,
+    max_merge_depth: usize,
+) -> toml::Value {
+    merge_toml_values_with_strategy(
+        left,
+        right,
+        &MergeStrategy {
+            array: MergeMode::MaxDepth(max_merge_depth),
+            table: MergeMode::MaxDepth(max_merge_depth),
+        },
+    )
+}
+
+pub fn merge_toml_values_with_strategy(
+    left: toml::Value,
+    right: toml::Value,
+    strategy: &MergeStrategy,
+) -> toml::Value {
+    merge_toml_values_recursive(left, right, strategy, 0)
+}
+
+fn merge_toml_values_recursive(
+    left: toml::Value,
+    right: toml::Value,
+    strategy: &MergeStrategy,
+    depth: usize,
+) -> toml::Value {
     use toml::Value;
 
     fn get_name(v: &Value) -> Option<&str> {
@@ -193,7 +256,7 @@ pub fn merge_toml_values(left: toml::Value, right: toml::Value, merge_depth: usi
 
     match (left, right) {
         (Value::Array(mut left_items), Value::Array(right_items)) => {
-            if merge_depth > 0 {
+            if strategy.array.should_merge(depth) {
                 left_items.reserve(right_items.len());
                 for rvalue in right_items {
                     let lvalue = get_name(&rvalue)
@@ -202,7 +265,9 @@ pub fn merge_toml_values(left: toml::Value, right: toml::Value, merge_depth: usi
                         })
                         .map(|lpos| left_items.remove(lpos));
                     let mvalue = match lvalue {
-                        Some(lvalue) => merge_toml_values(lvalue, rvalue, merge_depth - 1),
+                        Some(lvalue) => {
+                            merge_toml_values_recursive(lvalue, rvalue, strategy, depth + 1)
+                        }
                         None => rvalue,
                     };
                     left_items.push(mvalue);
@@ -213,11 +278,12 @@ pub fn merge_toml_values(left: toml::Value, right: toml::Value, merge_depth: usi
             }
         }
         (Value::Table(mut left_map), Value::Table(right_map)) => {
-            if merge_depth > 0 {
+            if strategy.table.should_merge(depth) {
                 for (rname, rvalue) in right_map {
                     match left_map.remove(&rname) {
                         Some(lvalue) => {
-                            let merged_value = merge_toml_values(lvalue, rvalue, merge_depth - 1);
+                            let merged_value =
+                                merge_toml_values_recursive(lvalue, rvalue, strategy, depth + 1);
                             left_map.insert(rname, merged_value);
                         }
                         None => {

helix-term/src/config.rs

@@ -1,6 +1,6 @@
 use crate::keymap;
 use crate::keymap::{merge_keys, KeyTrie};
-use helix_loader::merge_toml_values;
+use helix_loader::{merge_toml_values_with_strategy, MergeMode, MergeStrategy};
 use helix_view::document::Mode;
 use serde::Deserialize;
 use std::collections::HashMap;
@@ -79,9 +79,16 @@ impl Config {
                     (None, Some(val)) | (Some(val), None) => {
                         val.try_into().map_err(ConfigLoadError::BadConfig)?
                     }
-                    (Some(global), Some(local)) => merge_toml_values(global, local, 3)
-                        .try_into()
-                        .map_err(ConfigLoadError::BadConfig)?,
+                    (Some(global), Some(local)) => merge_toml_values_with_strategy(
+                        global,
+                        local,
+                        &MergeStrategy {
+                            array: MergeMode::Never,
+                            table: MergeMode::Always,
+                        },
+                    )
+                    .try_into()
+                    .map_err(ConfigLoadError::BadConfig)?,
                 };
 
                 Config {
@@ -131,11 +138,42 @@ mod tests {
     use super::*;
 
     impl Config {
-        fn load_test(config: &str) -> Config {
-            Config::load(Ok(config.to_owned()), Err(ConfigLoadError::default())).unwrap()
+        fn load_test(global: &str, local: &str) -> Config {
+            Config::load(Ok(global.to_owned()), Ok(local.to_owned())).unwrap()
         }
     }
 
+    #[test]
+    fn should_merge_editor_config_tables() {
+        let global = r#"
+            [editor.statusline]
+            mode.insert = "INSERT"
+            mode.select = "SELECT"
+        "#;
+        let local = r#"
+            [editor.statusline]
+            mode.select = "VIS"
+        "#;
+        let config = Config::load_test(global, local);
+        assert_eq!(config.editor.statusline.mode.normal, "NOR"); // Default
+        assert_eq!(config.editor.statusline.mode.insert, "INSERT"); // Global
+        assert_eq!(config.editor.statusline.mode.select, "VIS"); // Local
+    }
+
+    #[test]
+    fn should_override_editor_config_arrays() {
+        let global = r#"
+            [editor]
+            shell = ["bash", "-c"]
+        "#;
+        let local = r#"
+            [editor]
+            shell = ["fish", "-c"]
+        "#;
+        let config = Config::load_test(global, local);
+        assert_eq!(config.editor.shell, ["fish", "-c"]);
+    }
+
     #[test]
     fn parsing_keymaps_config_file() {
         use crate::keymap;
@@ -166,7 +204,7 @@ mod tests {
         );
 
         assert_eq!(
-            Config::load_test(sample_keymaps),
+            Config::load_test(sample_keymaps, ""),
             Config {
                 keys,
                 ..Default::default()
@@ -177,7 +215,7 @@ mod tests {
     #[test]
     fn keys_resolve_to_correct_defaults() {
         // From serde default
-        let default_keys = Config::load_test("").keys;
+        let default_keys = Config::load_test("", "").keys;
         assert_eq!(default_keys, keymap::default());
 
         // From the Default trait