diff --git a/CHANGELOG.md b/CHANGELOG.md index 27bd4f798..c2486a935 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Breaking changes +* Configuration variables are no longer "stringly" typed. For example, `true` is + not converted to a string `"true"`, and vice versa. + * The following configuration variables are now parsed strictly: `ui.progress-indicator`, `ui.quiet` diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 13f05f8bb..61e751c26 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -2694,22 +2694,19 @@ fn load_template_aliases( Ok(Some(table)) => table, Ok(None) => continue, Err(item) => { - // TODO: rewrite error construction after migrating to toml_edit - let error = item.clone().into_table().unwrap_err(); return Err(ConfigGetError::Type { name: table_name.to_string(), - error: error.into(), + error: format!("Expected a table, but is {}", item.type_name()).into(), source_path: layer.path.clone(), } .into()); } }; // TODO: remove sorting after migrating to toml_edit - for (decl, value) in table.iter().sorted_by_key(|&(decl, _)| decl) { - let r = value - .clone() - .into_string() - .map_err(|e| e.to_string()) + for (decl, item) in table.iter().sorted_by_key(|&(decl, _)| decl) { + let r = item + .as_str() + .ok_or_else(|| format!("Expected a string, but is {}", item.type_name())) .and_then(|v| aliases_map.insert(decl, v).map_err(|e| e.to_string())); if let Err(s) = r { writeln!( diff --git a/cli/src/commands/config/get.rs b/cli/src/commands/config/get.rs index 8db273f20..f77e92c4c 100644 --- a/cli/src/commands/config/get.rs +++ b/cli/src/commands/config/get.rs @@ -13,12 +13,10 @@ // limitations under the License. use std::io::Write as _; -use std::path::PathBuf; use clap_complete::ArgValueCandidates; -use jj_lib::config::ConfigError; -use jj_lib::config::ConfigGetError; use jj_lib::config::ConfigNamePathBuf; +use jj_lib::config::ConfigValue; use tracing::instrument; use crate::cli_util::CommandHelper; @@ -48,21 +46,24 @@ pub fn cmd_config_get( command: &CommandHelper, args: &ConfigGetArgs, ) -> Result<(), CommandError> { - let value = command.settings().get_value(&args.name)?; - let stringified = value.into_string().map_err(|err| -> CommandError { - match err { - ConfigError::Type { - origin, unexpected, .. - } => ConfigGetError::Type { - name: args.name.to_string(), - error: format!("Expected a value convertible to a string, but is {unexpected}") - .into(), - source_path: origin.map(PathBuf::from), + let stringified = command + .settings() + .get_value_with(&args.name, |value| match value { + // Remove extra formatting from a string value + ConfigValue::String(v) => Ok(v.into_value()), + // Print other values in TOML syntax (but whitespace trimmed) + ConfigValue::Integer(_) + | ConfigValue::Float(_) + | ConfigValue::Boolean(_) + | ConfigValue::Datetime(_) => Ok(value.decorated("", "").to_string()), + // TODO: maybe okay to just print array or table in TOML syntax? + ConfigValue::Array(_) => { + Err("Expected a value convertible to a string, but is an array") } - .into(), - err => err.into(), - } - })?; + ConfigValue::InlineTable(_) => { + Err("Expected a value convertible to a string, but is a table") + } + })?; writeln!(ui.stdout(), "{stringified}")?; Ok(()) } diff --git a/cli/src/commands/config/list.rs b/cli/src/commands/config/list.rs index 76862fecc..2009bc55c 100644 --- a/cli/src/commands/config/list.rs +++ b/cli/src/commands/config/list.rs @@ -22,7 +22,6 @@ use crate::cli_util::CommandHelper; use crate::command_error::CommandError; use crate::complete; use crate::config::resolved_config_values; -use crate::config::to_toml_value; use crate::config::AnnotatedValue; use crate::generic_templater::GenericTemplateLanguage; use crate::template_builder::TemplateLanguage as _; @@ -122,8 +121,9 @@ fn config_template_language() -> GenericTemplateLanguage<'static, AnnotatedValue }); language.add_keyword("value", |self_property| { // TODO: would be nice if we can provide raw dynamically-typed value + // .decorated("", "") to trim leading/trailing whitespace let out_property = - self_property.and_then(|annotated| Ok(to_toml_value(&annotated.value)?.to_string())); + self_property.map(|annotated| annotated.value.decorated("", "").to_string()); Ok(L::wrap_string(out_property)) }); language.add_keyword("overridden", |self_property| { diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs index 33a7f9696..cbbdedfa8 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -52,7 +52,6 @@ use crate::command_error::config_error; use crate::command_error::print_parse_diagnostics; use crate::command_error::CommandError; use crate::complete; -use crate::config::to_toml_value; use crate::config::CommandNameAndArgs; use crate::ui::Ui; @@ -470,7 +469,10 @@ fn get_tools_config(ui: &mut Ui, settings: &UserSettings) -> Result = settings diff --git a/cli/src/config.rs b/cli/src/config.rs index 96d8ce85e..a223bc1b2 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -51,32 +51,6 @@ pub fn parse_toml_value_or_bare_string(value_str: &str) -> toml_edit::Value { } } -pub fn to_toml_value(value: &ConfigValue) -> Result { - fn type_error(message: T) -> ConfigError { - ConfigError::Message(message.to_string()) - } - // It's unlikely that the config object contained unsupported values, but - // there's no guarantee. For example, values coming from environment - // variables might be big int. - match value.kind { - config::ValueKind::Nil => Err(type_error(format!("Unexpected value: {value}"))), - config::ValueKind::Boolean(v) => Ok(v.into()), - config::ValueKind::I64(v) => Ok(v.into()), - config::ValueKind::I128(v) => Ok(i64::try_from(v).map_err(type_error)?.into()), - config::ValueKind::U64(v) => Ok(i64::try_from(v).map_err(type_error)?.into()), - config::ValueKind::U128(v) => Ok(i64::try_from(v).map_err(type_error)?.into()), - config::ValueKind::Float(v) => Ok(v.into()), - config::ValueKind::String(ref v) => Ok(v.into()), - // TODO: Remove sorting when config crate maintains deterministic ordering. - config::ValueKind::Table(ref table) => table - .iter() - .sorted_by_key(|(k, _)| *k) - .map(|(k, v)| Ok((k, to_toml_value(v)?))) - .collect(), - config::ValueKind::Array(ref array) => array.iter().map(to_toml_value).collect(), - } -} - #[derive(Error, Debug)] pub enum ConfigEnvError { #[error(transparent)] @@ -118,24 +92,30 @@ pub fn resolved_config_values( }; let mut config_stack = vec![(filter_prefix.clone(), top_item)]; while let Some((name, item)) = config_stack.pop() { - match &item.kind { - config::ValueKind::Table(table) => { - // TODO: Remove sorting when config crate maintains deterministic ordering. - for (k, v) in table.iter().sorted_by_key(|(k, _)| *k).rev() { - let mut sub_name = name.clone(); - sub_name.push(k); - config_stack.push((sub_name, v)); - } - } - _ => { - config_vals.push(AnnotatedValue { - name, - value: item.to_owned(), - source: layer.source, - // Note: Value updated below. - is_overridden: false, - }); + // TODO: item.as_table() to print inline table as a value? + if let Some(table) = item.as_table_like() { + // table.iter() does not implement DoubleEndedIterator as of + // toml_edit 0.22.22. + let frame = config_stack.len(); + // TODO: Remove sorting + for (k, v) in table.iter().sorted_by_key(|(k, _)| *k) { + let mut sub_name = name.clone(); + sub_name.push(k); + config_stack.push((sub_name, v)); } + config_stack[frame..].reverse(); + } else { + let value = item + .clone() + .into_value() + .expect("Item::None should not exist in table"); + config_vals.push(AnnotatedValue { + name, + value, + source: layer.source, + // Note: Value updated below. + is_overridden: false, + }); } } } @@ -798,12 +778,13 @@ mod tests { }, ], ), - value: Value { - origin: None, - kind: String( - "base@user.email", - ), - }, + value: String( + Formatted { + value: "base@user.email", + repr: "default", + decor: Decor { .. }, + }, + ), source: EnvBase, is_overridden: true, }, @@ -824,12 +805,13 @@ mod tests { }, ], ), - value: Value { - origin: None, - kind: String( - "base-user-name", - ), - }, + value: String( + Formatted { + value: "base-user-name", + repr: "default", + decor: Decor { .. }, + }, + ), source: EnvBase, is_overridden: false, }, @@ -850,12 +832,13 @@ mod tests { }, ], ), - value: Value { - origin: None, - kind: String( - "repo@user.email", - ), - }, + value: String( + Formatted { + value: "repo@user.email", + repr: "default", + decor: Decor { .. }, + }, + ), source: Repo, is_overridden: false, }, @@ -897,12 +880,13 @@ mod tests { }, ], ), - value: Value { - origin: None, - kind: String( - "user-FOO", - ), - }, + value: String( + Formatted { + value: "user-FOO", + repr: "default", + decor: Decor { .. }, + }, + ), source: User, is_overridden: false, }, @@ -923,12 +907,13 @@ mod tests { }, ], ), - value: Value { - origin: None, - kind: String( - "repo-BAR", - ), - }, + value: String( + Formatted { + value: "repo-BAR", + repr: "default", + decor: Decor { .. }, + }, + ), source: Repo, is_overridden: false, }, diff --git a/cli/src/formatter.rs b/cli/src/formatter.rs index b8bb03a8b..f63bdab01 100644 --- a/cli/src/formatter.rs +++ b/cli/src/formatter.rs @@ -417,44 +417,41 @@ fn rules_from_config(config: &StackedConfig) -> Result { .split_whitespace() .map(ToString::to_string) .collect_vec(); + // TODO: report invalid type? let value = config.get_value(["colors", key])?; - match value.kind { - config::ValueKind::String(color_name) => { - let style = Style { - fg_color: Some(color_for_name_or_hex(&color_name).map_err(to_config_err)?), - bg_color: None, - bold: None, - underlined: None, - }; - result.push((labels, style)); + if let Some(color_name) = value.as_str() { + let style = Style { + fg_color: Some(color_for_name_or_hex(color_name).map_err(to_config_err)?), + bg_color: None, + bold: None, + underlined: None, + }; + result.push((labels, style)); + } else if let Some(style_table) = value.as_inline_table() { + let mut style = Style::default(); + if let Some(item) = style_table.get("fg") { + if let Some(color_name) = item.as_str() { + style.fg_color = + Some(color_for_name_or_hex(color_name).map_err(to_config_err)?); + } } - config::ValueKind::Table(style_table) => { - let mut style = Style::default(); - if let Some(value) = style_table.get("fg") { - if let config::ValueKind::String(color_name) = &value.kind { - style.fg_color = - Some(color_for_name_or_hex(color_name).map_err(to_config_err)?); - } + if let Some(item) = style_table.get("bg") { + if let Some(color_name) = item.as_str() { + style.bg_color = + Some(color_for_name_or_hex(color_name).map_err(to_config_err)?); } - if let Some(value) = style_table.get("bg") { - if let config::ValueKind::String(color_name) = &value.kind { - style.bg_color = - Some(color_for_name_or_hex(color_name).map_err(to_config_err)?); - } - } - if let Some(value) = style_table.get("bold") { - if let config::ValueKind::Boolean(value) = &value.kind { - style.bold = Some(*value); - } - } - if let Some(value) = style_table.get("underline") { - if let config::ValueKind::Boolean(value) = &value.kind { - style.underlined = Some(*value); - } - } - result.push((labels, style)); } - _ => {} + if let Some(item) = style_table.get("bold") { + if let Some(value) = item.as_bool() { + style.bold = Some(value); + } + } + if let Some(item) = style_table.get("underline") { + if let Some(value) = item.as_bool() { + style.underlined = Some(value); + } + } + result.push((labels, style)); } } Ok(result) diff --git a/cli/src/revset_util.rs b/cli/src/revset_util.rs index 32af07a20..892ed60b9 100644 --- a/cli/src/revset_util.rs +++ b/cli/src/revset_util.rs @@ -176,26 +176,22 @@ pub fn load_revset_aliases( Ok(Some(table)) => table, Ok(None) => continue, Err(item) => { - // TODO: rewrite error construction after migrating to toml_edit - let error = item.clone().into_table().unwrap_err(); return Err(ConfigGetError::Type { name: table_name.to_string(), - error: error.into(), + error: format!("Expected a table, but is {}", item.type_name()).into(), source_path: layer.path.clone(), } .into()); } }; // TODO: remove sorting after migrating to toml_edit - for (decl, value) in table.iter().sorted_by_key(|&(decl, _)| decl) { + for (decl, item) in table.iter().sorted_by_key(|&(decl, _)| decl) { warn_user_redefined_builtin(ui, layer.source, decl)?; - let r = value - .clone() - .into_string() - .map_err(|e| e.to_string()) + let r = item + .as_str() + .ok_or_else(|| format!("Expected a string, but is {}", item.type_name())) .and_then(|v| aliases_map.insert(decl, v).map_err(|e| e.to_string())); - if let Err(s) = r { writeln!( ui.warning_default(), diff --git a/cli/tests/test_alias.rs b/cli/tests/test_alias.rs index 916e5596a..75e5f96ef 100644 --- a/cli/tests/test_alias.rs +++ b/cli/tests/test_alias.rs @@ -230,20 +230,22 @@ fn test_alias_invalid_definition() { test_env.add_config( r#"[aliases] non-list = 5 - non-string-list = [[]] + non-string-list = [0] "#, ); let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["non-list"]); insta::assert_snapshot!(stderr.replace('\\', "/"), @r" Config error: Invalid type or value for aliases.non-list Caused by: invalid type: integer `5`, expected a sequence + Hint: Check the config file: $TEST_ENV/config/config0002.toml For help, see https://martinvonz.github.io/jj/latest/config/. "); let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["non-string-list"]); - insta::assert_snapshot!(stderr.replace('\\', "/"), @r" + insta::assert_snapshot!(stderr, @r" Config error: Invalid type or value for aliases.non-string-list - Caused by: invalid type: sequence, expected a string for key `[0]` in config/config0002.toml + Caused by: invalid type: integer `0`, expected a string + Hint: Check the config file: $TEST_ENV/config/config0002.toml For help, see https://martinvonz.github.io/jj/latest/config/. "); @@ -318,7 +320,5 @@ fn test_alias_in_repo_config() { repo2_path.to_str().unwrap(), ], ); - insta::assert_snapshot!(stdout, @r###" - aliases.l = ["log", "-r@", "--no-graph", '-T"user alias\n"'] - "###); + insta::assert_snapshot!(stdout, @r#"aliases.l = ['log', '-r@', '--no-graph', '-T"user alias\n"']"#); } diff --git a/cli/tests/test_config_command.rs b/cli/tests/test_config_command.rs index 06fd7b08c..161929e48 100644 --- a/cli/tests/test_config_command.rs +++ b/cli/tests/test_config_command.rs @@ -153,12 +153,12 @@ bar ); let stdout = test_env.jj_cmd_success(test_env.env_root(), &["config", "list", "multiline"]); - insta::assert_snapshot!(stdout, @r###" - multiline = """ + insta::assert_snapshot!(stdout, @r" + multiline = ''' foo bar - """ - "###); + ''' + "); let stdout = test_env.jj_cmd_success( test_env.env_root(), @@ -170,13 +170,13 @@ bar "--config-toml=multiline='single'", ], ); - insta::assert_snapshot!(stdout, @r###" - # multiline = """ + insta::assert_snapshot!(stdout, @r" + # multiline = ''' # foo # bar - # """ - multiline = "single" - "###); + # ''' + multiline = 'single' + "); } #[test] @@ -836,17 +836,18 @@ fn test_config_get() { "###); let stdout = test_env.jj_cmd_failure(test_env.env_root(), &["config", "get", "table.list"]); - insta::assert_snapshot!(stdout.replace('\\', "/"), @r" + insta::assert_snapshot!(stdout, @r" Config error: Invalid type or value for table.list - Caused by: Expected a value convertible to a string, but is sequence - Hint: Check the config file: config/config0002.toml + Caused by: Expected a value convertible to a string, but is an array + Hint: Check the config file: $TEST_ENV/config/config0002.toml For help, see https://martinvonz.github.io/jj/latest/config/. "); let stdout = test_env.jj_cmd_failure(test_env.env_root(), &["config", "get", "table"]); insta::assert_snapshot!(stdout, @r" Config error: Invalid type or value for table - Caused by: Expected a value convertible to a string, but is map + Caused by: Expected a value convertible to a string, but is a table + Hint: Check the config file: $TEST_ENV/config/config0003.toml For help, see https://martinvonz.github.io/jj/latest/config/. "); @@ -975,7 +976,8 @@ fn test_config_show_paths() { let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["st"]); insta::assert_snapshot!(stderr, @r" Config error: Invalid type or value for ui.paginate - Caused by: enum PaginationChoice does not have variant constructor :builtin + Caused by: unknown variant `:builtin`, expected `never` or `auto` + Hint: Check the config file: $TEST_ENV/config/config.toml For help, see https://martinvonz.github.io/jj/latest/config/. "); diff --git a/cli/tests/test_fix_command.rs b/cli/tests/test_fix_command.rs index 734d77459..c822efccf 100644 --- a/cli/tests/test_fix_command.rs +++ b/cli/tests/test_fix_command.rs @@ -45,11 +45,7 @@ fn init_with_fake_formatter(args: &[&str]) -> (TestEnvironment, PathBuf, impl Fn // When the test runs on Windows, backslashes in the path complicate things by // changing the double quotes to single quotes in the serialized TOML. snapshot.replace( - &if cfg!(windows) { - format!(r#"'{}'"#, formatter_path.to_str().unwrap()) - } else { - format!(r#""{}""#, formatter_path.to_str().unwrap()) - }, + &format!("'{}'", formatter_path.to_str().unwrap()), "", ) }) @@ -174,16 +170,16 @@ fn test_config_multiple_tools_with_same_name() { std::fs::write(repo_path.join("bar"), "Bar\n").unwrap(); let stderr = test_env.jj_cmd_failure(&repo_path, &["fix"]); - #[cfg(unix)] - insta::assert_snapshot!(stderr, @r###" - Config error: redefinition of table `fix.tools.my-tool` for key `fix.tools.my-tool` at line 6 column 9 in ../config/config0002.toml + insta::assert_snapshot!(stderr, @r" + Config error: TOML parse error at line 6, column 9 + | + 6 | [fix.tools.my-tool] + | ^ + invalid table header + duplicate key `my-tool` in table `fix.tools` + in $TEST_ENV/config/config0002.toml For help, see https://martinvonz.github.io/jj/latest/config/. - "###); - #[cfg(windows)] - insta::assert_snapshot!(stderr, @r###" - Config error: redefinition of table `fix.tools.my-tool` for key `fix.tools.my-tool` at line 6 column 9 in ..\config\config0002.toml - For help, see https://martinvonz.github.io/jj/latest/config/. - "###); + "); test_env.set_config_path("/dev/null".into()); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo", "-r", "@"]); @@ -259,6 +255,7 @@ fn test_config_tables_all_commands_missing() { insta::assert_snapshot!(stderr.replace('\\', "/"), @r" Config error: Invalid type or value for fix.tools.my-tool-missing-command-1 Caused by: missing field `command` + Hint: Check the config file: $TEST_ENV/config/config0002.toml For help, see https://martinvonz.github.io/jj/latest/config/. "); @@ -293,6 +290,7 @@ fn test_config_tables_some_commands_missing() { insta::assert_snapshot!(stderr.replace('\\', "/"), @r" Config error: Invalid type or value for fix.tools.my-tool-missing-command Caused by: missing field `command` + Hint: Check the config file: $TEST_ENV/config/config0002.toml For help, see https://martinvonz.github.io/jj/latest/config/. "); @@ -428,16 +426,16 @@ fn test_fix_empty_commit() { let (test_env, repo_path, redact) = init_with_fake_formatter(&["--uppercase"]); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 0 commits of 1 checked. Nothing changed. - "###); + "#); } #[test] @@ -449,18 +447,18 @@ fn test_fix_leaf_commit() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 1 commits of 1 checked. Working copy now at: rlvkpnrz 85ce8924 (no description set) Parent commit : qpvuntsm b2ca2bc5 (no description set) Added 0 files, modified 1 files, removed 0 files - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@-"]); insta::assert_snapshot!(content, @"unaffected"); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); @@ -482,18 +480,18 @@ fn test_fix_parent_commit() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "parent"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 3 commits of 3 checked. Working copy now at: mzvwutvl d30c8ae2 child2 | (no description set) Parent commit : qpvuntsm 70a4dae2 parent | (no description set) Added 0 files, modified 1 files, removed 0 files - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "parent"]); insta::assert_snapshot!(content, @"PARENT"); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "child1"]); @@ -516,15 +514,15 @@ fn test_fix_sibling_commit() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "child1"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 1 commits of 1 checked. - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "parent"]); insta::assert_snapshot!(content, @"parent"); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "child1"]); @@ -561,18 +559,18 @@ fn test_default_revset() { test_env.add_config(r#"revset-aliases."immutable_heads()" = "trunk2""#); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 3 commits of 3 checked. Working copy now at: yostqsxw dabc47b2 bar2 | (no description set) Parent commit : yqosqzyt 984b5924 bar1 | (no description set) Added 0 files, modified 1 files, removed 0 files - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "trunk1"]); insta::assert_snapshot!(content, @"trunk1"); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "trunk2"]); @@ -604,15 +602,15 @@ fn test_custom_default_revset() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 1 commits of 1 checked. - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "foo"]); insta::assert_snapshot!(content, @"foo"); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "bar"]); @@ -634,7 +632,7 @@ fn test_fix_immutable_commit() { Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Error: Commit e4b41a3ce243 is immutable @@ -654,16 +652,16 @@ fn test_fix_empty_file() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 0 commits of 1 checked. Nothing changed. - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); insta::assert_snapshot!(content, @""); } @@ -676,18 +674,18 @@ fn test_fix_some_paths() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@", "file1"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 1 commits of 1 checked. Working copy now at: qpvuntsm 54a90d2b (no description set) Parent commit : zzzzzzzz 00000000 (empty) (no description set) Added 0 files, modified 1 files, removed 0 files - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file1"]); insta::assert_snapshot!(content, @r###" FOO @@ -703,35 +701,35 @@ fn test_fix_cyclic() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--reverse"] + command = [, '--reverse'] patterns = ["all()"] Fixed 1 commits of 1 checked. Working copy now at: qpvuntsm bf5e6a5a (no description set) Parent commit : zzzzzzzz 00000000 (empty) (no description set) Added 0 files, modified 1 files, removed 0 files - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); insta::assert_snapshot!(content, @"tnetnoc\n"); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--reverse"] + command = [, '--reverse'] patterns = ["all()"] Fixed 1 commits of 1 checked. Working copy now at: qpvuntsm 0e2d20d6 (no description set) Parent commit : zzzzzzzz 00000000 (empty) (no description set) Added 0 files, modified 1 files, removed 0 files - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); insta::assert_snapshot!(content, @"content\n"); } @@ -760,18 +758,18 @@ fn test_deduplication() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "a"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase", "--tee", "$path-fixlog"] + command = [, '--uppercase', '--tee', '$path-fixlog'] patterns = ["all()"] Fixed 4 commits of 4 checked. Working copy now at: yqosqzyt cf770245 d | (no description set) Parent commit : mzvwutvl 370615a5 c | (empty) (no description set) Added 0 files, modified 1 files, removed 0 files - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "a"]); insta::assert_snapshot!(content, @"FOO\n"); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "b"]); @@ -806,16 +804,16 @@ fn test_executed_but_nothing_changed() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--tee", "$path-copy"] + command = [, '--tee', '$path-copy'] patterns = ["all()"] Fixed 0 commits of 1 checked. Nothing changed. - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); insta::assert_snapshot!(content, @"content\n"); let copy_content = std::fs::read_to_string(repo_path.join("file-copy").as_os_str()).unwrap(); @@ -829,16 +827,16 @@ fn test_failure() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--fail"] + command = [, '--fail'] patterns = ["all()"] Fixed 0 commits of 1 checked. Nothing changed. - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); insta::assert_snapshot!(content, @"content"); } @@ -853,18 +851,18 @@ fn test_stderr_success() { // of passing it through directly. let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--stderr", "error", "--stdout", "new content"] + command = [, '--stderr', 'error', '--stdout', 'new content'] patterns = ["all()"] errorFixed 1 commits of 1 checked. Working copy now at: qpvuntsm 487808ba (no description set) Parent commit : zzzzzzzz 00000000 (empty) (no description set) Added 0 files, modified 1 files, removed 0 files - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); insta::assert_snapshot!(content, @"new content"); } @@ -877,16 +875,16 @@ fn test_stderr_failure() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--stderr", "error", "--stdout", "new content", "--fail"] + command = [, '--stderr', 'error', '--stdout', 'new content', '--fail'] patterns = ["all()"] errorFixed 0 commits of 1 checked. Nothing changed. - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); insta::assert_snapshot!(content, @"old content"); } @@ -923,18 +921,18 @@ fn test_fix_file_types() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 1 commits of 1 checked. Working copy now at: qpvuntsm 6836a9e4 (no description set) Parent commit : zzzzzzzz 00000000 (empty) (no description set) Added 0 files, modified 1 files, removed 0 files - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); insta::assert_snapshot!(content, @"CONTENT"); } @@ -951,18 +949,18 @@ fn test_fix_executable() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 1 commits of 1 checked. Working copy now at: qpvuntsm fee78e99 (no description set) Parent commit : zzzzzzzz 00000000 (empty) (no description set) Added 0 files, modified 1 files, removed 0 files - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); insta::assert_snapshot!(content, @"CONTENT"); let executable = std::fs::metadata(&path).unwrap().permissions().mode() & 0o111; @@ -985,16 +983,16 @@ fn test_fix_trivial_merge_commit() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 0 commits of 1 checked. Nothing changed. - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file_a", "-r", "@"]); insta::assert_snapshot!(content, @"content a"); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file_b", "-r", "@"]); @@ -1023,11 +1021,11 @@ fn test_fix_adding_merge_commit() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 1 commits of 1 checked. @@ -1035,7 +1033,7 @@ fn test_fix_adding_merge_commit() { Parent commit : qpvuntsm 6e64e7a7 a | (no description set) Parent commit : kkmpptxz c536f264 b | (no description set) Added 0 files, modified 4 files, removed 0 files - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file_a", "-r", "@"]); insta::assert_snapshot!(content, @"CHANGE A"); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file_b", "-r", "@"]); @@ -1060,11 +1058,11 @@ fn test_fix_both_sides_of_conflict() { // fixed if we didn't fix the parents also. let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "a", "-s", "b"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 3 commits of 3 checked. @@ -1074,7 +1072,7 @@ fn test_fix_both_sides_of_conflict() { Added 0 files, modified 1 files, removed 0 files There are unresolved conflicts at these paths: file 2-sided conflict - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "a"]); insta::assert_snapshot!(content, @r###" CONTENT A @@ -1110,11 +1108,11 @@ fn test_fix_resolve_conflict() { // fixed if we didn't fix the parents also. let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "a", "-s", "b"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr), @r#" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase"] + command = [, '--uppercase'] patterns = ["all()"] Fixed 3 commits of 3 checked. @@ -1122,7 +1120,7 @@ fn test_fix_resolve_conflict() { Parent commit : qpvuntsm dd2721f1 a | (no description set) Parent commit : kkmpptxz 07c27a8e b | (no description set) Added 0 files, modified 1 files, removed 0 files - "###); + "#); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); insta::assert_snapshot!(content, @r###" CONTENT diff --git a/cli/tests/test_global_opts.rs b/cli/tests/test_global_opts.rs index b7d356440..5456963ed 100644 --- a/cli/tests/test_global_opts.rs +++ b/cli/tests/test_global_opts.rs @@ -586,10 +586,16 @@ fn test_invalid_config() { test_env.add_config("[section]key = value-missing-quotes"); let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["init", "repo"]); - insta::assert_snapshot!(stderr.replace('\\', "/"), @r###" - Config error: expected newline, found an identifier at line 1 column 10 in config/config0002.toml + insta::assert_snapshot!(stderr, @r" + Config error: TOML parse error at line 1, column 10 + | + 1 | [section]key = value-missing-quotes + | ^ + invalid table header + expected newline, `#` + in $TEST_ENV/config/config0002.toml For help, see https://martinvonz.github.io/jj/latest/config/. - "###); + "); } #[test] @@ -606,6 +612,7 @@ fn test_invalid_config_value() { insta::assert_snapshot!(stderr, @r" Config error: Invalid type or value for snapshot.auto-track Caused by: invalid type: sequence, expected a string + For help, see https://martinvonz.github.io/jj/latest/config/. "); } diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs index fa3f83ac6..968139e28 100644 --- a/cli/tests/test_log_command.rs +++ b/cli/tests/test_log_command.rs @@ -1326,7 +1326,8 @@ fn test_graph_styles() { ); insta::assert_snapshot!(stderr, @r" Config error: Invalid type or value for ui.graph.style - Caused by: enum GraphStyle does not have variant constructor unknown + Caused by: unknown variant `unknown`, expected one of `ascii`, `ascii-large`, `curved`, `square` + For help, see https://martinvonz.github.io/jj/latest/config/. "); } diff --git a/lib/src/config.rs b/lib/src/config.rs index 4516dcd08..0b37a2ee4 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -17,7 +17,7 @@ use std::borrow::Borrow; use std::convert::Infallible; use std::fmt; -use std::mem; +use std::fs; use std::ops::Range; use std::path::Path; use std::path::PathBuf; @@ -25,15 +25,20 @@ use std::slice; use std::str::FromStr; use itertools::Itertools as _; +use serde::de::IntoDeserializer as _; use serde::Deserialize; use thiserror::Error; +use toml_edit::DocumentMut; +use toml_edit::ImDocument; use crate::file_util::IoResultExt as _; +/// Config value or table node. +pub type ConfigItem = toml_edit::Item; /// Table of config key and value pairs. -pub type ConfigTable = config::Map; +pub type ConfigTable = toml_edit::Table; /// Generic config value. -pub type ConfigValue = config::Value; +pub type ConfigValue = toml_edit::Value; /// Error that can occur when accessing configuration. // TODO: will be replaced with our custom error type @@ -63,8 +68,20 @@ pub enum ConfigGetError { /// Error that can occur when updating config variable. #[derive(Debug, Error)] -#[error(transparent)] -pub struct ConfigUpdateError(pub ConfigError); +pub enum ConfigUpdateError { + /// Non-table value exists at parent path, which shouldn't be removed. + #[error("Would overwrite non-table value with parent table {name}")] + WouldOverwriteValue { + /// Dotted config name path. + name: String, + }, + /// Table exists at the path, which shouldn't be overwritten by a value. + #[error("Would overwrite entire table {name}")] + WouldOverwriteTable { + /// Dotted config name path. + name: String, + }, +} /// Extension methods for `Result`. pub trait ConfigGetResultExt { @@ -234,17 +251,17 @@ pub struct ConfigLayer { /// Source file path of this layer if any. pub path: Option, /// Configuration variables. - pub data: config::Config, + pub data: DocumentMut, } impl ConfigLayer { /// Creates new layer with empty data. pub fn empty(source: ConfigSource) -> Self { - Self::with_data(source, config::Config::default()) + Self::with_data(source, DocumentMut::new()) } /// Creates new layer with the configuration variables `data`. - pub fn with_data(source: ConfigSource, data: config::Config) -> Self { + pub fn with_data(source: ConfigSource, data: DocumentMut) -> Self { ConfigLayer { source, path: None, @@ -254,27 +271,26 @@ impl ConfigLayer { /// Parses TOML document `text` into new layer. pub fn parse(source: ConfigSource, text: &str) -> Result { - let data = config::Config::builder() - .add_source(config::File::from_str(text, config::FileFormat::Toml)) - .build()?; - Ok(Self::with_data(source, data)) + let data = ImDocument::parse(text).map_err(|err| ConfigError::FileParse { + uri: None, + cause: err.into(), + })?; + Ok(Self::with_data(source, data.into_mut())) } fn load_from_file(source: ConfigSource, path: PathBuf) -> Result { - // TODO: will be replaced with toml_edit::DocumentMut or ImDocument - let data = config::Config::builder() - .add_source( - config::File::from(path.clone()) - // TODO: The path should exist, but the config crate refuses - // to read a special file (e.g. /dev/null) as TOML. - .required(false) - .format(config::FileFormat::Toml), - ) - .build()?; + let text = fs::read_to_string(&path).map_err(|err| ConfigError::FileParse { + uri: Some(path.to_string_lossy().into_owned()), + cause: err.into(), + })?; + let data = ImDocument::parse(text).map_err(|err| ConfigError::FileParse { + uri: Some(path.to_string_lossy().into_owned()), + cause: err.into(), + })?; Ok(ConfigLayer { source, path: Some(path), - data, + data: data.into_mut(), }) } @@ -303,14 +319,16 @@ impl ConfigLayer { /// Looks up sub table by the `name` path. Returns `Some(table)` if a table /// was found at the path. Returns `Err(item)` if middle or leaf node wasn't /// a table. + // TODO: Perhaps, an inline table will be processed as a value, and this + // function will return &ConfigTable. pub fn look_up_table( &self, name: impl ToConfigNamePath, - ) -> Result, &config::Value> { + ) -> Result, &ConfigItem> { match self.look_up_item(name) { - Ok(Some(item)) => match &item.kind { - config::ValueKind::Table(table) => Ok(Some(table)), - _ => Err(item), + Ok(Some(item)) => match item.as_table_like() { + Some(table) => Ok(Some(table)), + None => Err(item), }, Ok(None) => Ok(None), Err(item) => Err(item), @@ -322,35 +340,53 @@ impl ConfigLayer { pub fn look_up_item( &self, name: impl ToConfigNamePath, - ) -> Result, &ConfigValue> { - look_up_item(&self.data.cache, name.into_name_path().borrow()) + ) -> Result, &ConfigItem> { + look_up_item(self.data.as_item(), name.into_name_path().borrow()) } - /// Sets new `value` to the `name` path. + /// Sets `new_value` to the `name` path. Returns old value if any. + /// + /// This function errors out if attempted to overwrite a non-table middle + /// node or a leaf table (in the same way as file/directory operation.) pub fn set_value( &mut self, - name: &str, // TODO: impl ToConfigNamePath - value: impl Into, - ) -> Result<(), ConfigUpdateError> { - self.data = config::Config::builder() - .add_source(mem::take(&mut self.data)) - .set_override(name, value) - .map_err(ConfigUpdateError)? - .build() - .map_err(ConfigUpdateError)?; - Ok(()) + name: impl ToConfigNamePath, + new_value: impl Into, + ) -> Result, ConfigUpdateError> { + let name = name.into_name_path(); + let name = name.borrow(); + let (parent_table, leaf_key) = ensure_parent_table(self.data.as_table_mut(), name) + .map_err(|keys| ConfigUpdateError::WouldOverwriteValue { + name: keys.join("."), + })?; + match parent_table.entry(leaf_key) { + toml_edit::Entry::Occupied(mut entry) => { + if !entry.get().is_value() { + return Err(ConfigUpdateError::WouldOverwriteTable { + name: name.to_string(), + }); + } + let old_item = entry.insert(toml_edit::value(new_value)); + Ok(Some(old_item.into_value().unwrap())) + } + toml_edit::Entry::Vacant(entry) => { + entry.insert(toml_edit::value(new_value)); + Ok(None) + } + } } } /// Looks up item from the `root_item`. Returns `Some(item)` if an item found at /// the path. Returns `Err(item)` if middle node wasn't a table. fn look_up_item<'a>( - root_item: &'a ConfigValue, + root_item: &'a ConfigItem, name: &ConfigNamePathBuf, -) -> Result, &'a ConfigValue> { +) -> Result, &'a ConfigItem> { let mut cur_item = root_item; - for key in name.components().map(toml_edit::Key::get) { - let config::ValueKind::Table(table) = &cur_item.kind else { + for key in name.components() { + // TODO: Should inline tables be addressed by dotted name path? + let Some(table) = cur_item.as_table_like() else { return Err(cur_item); }; cur_item = match table.get(key) { @@ -361,6 +397,21 @@ fn look_up_item<'a>( Ok(Some(cur_item)) } +/// Inserts tables down to the parent of the `name` path. Returns `Err(keys)` if +/// middle node exists at the prefix name `keys` and wasn't a table. +fn ensure_parent_table<'a, 'b>( + root_table: &'a mut ConfigTable, + name: &'b ConfigNamePathBuf, +) -> Result<(&'a mut ConfigTable, &'b toml_edit::Key), &'b [toml_edit::Key]> { + let mut keys = name.components(); + let leaf_key = keys.next_back().ok_or(&name.0[..])?; + let parent_table = keys.enumerate().try_fold(root_table, |table, (i, key)| { + let sub_item = table.entry(key).or_insert_with(toml_edit::table); + sub_item.as_table_mut().ok_or(&name.0[..=i]) + })?; + Ok((parent_table, leaf_key)) +} + /// Stack of configuration layers which can be merged as needed. #[derive(Clone, Debug)] pub struct StackedConfig { @@ -454,12 +505,12 @@ impl StackedConfig { &self, name: impl ToConfigNamePath, ) -> Result { - self.get_item_with(name, T::deserialize) + self.get_value_with(name, |value| T::deserialize(value.into_deserializer())) } /// Looks up value from all layers, merges sub fields as needed. pub fn get_value(&self, name: impl ToConfigNamePath) -> Result { - self.get_item_with::<_, Infallible>(name, Ok) + self.get_value_with::<_, Infallible>(name, Ok) } /// Looks up value from all layers, merges sub fields as needed, then @@ -469,8 +520,15 @@ impl StackedConfig { name: impl ToConfigNamePath, convert: impl FnOnce(ConfigValue) -> Result, ) -> Result { - // TODO: Item will be a different type than Value - self.get_item_with(name, convert) + self.get_item_with(name, |item| { + // Item variants other than Item::None can be converted to a Value, + // and Item::None is not a valid TOML type. See also the following + // thread: https://github.com/toml-rs/toml/issues/299 + let value = item + .into_value() + .expect("Item::None should not exist in loaded tables"); + convert(value) + }) } /// Looks up sub table from all layers, merges fields as needed. @@ -478,13 +536,18 @@ impl StackedConfig { /// Use `table_keys(prefix)` and `get([prefix, key])` instead if table /// values have to be converted to non-generic value type. pub fn get_table(&self, name: impl ToConfigNamePath) -> Result { - self.get(name) + self.get_item_with(name, |item| { + // TODO: Should inline table be converted to non-inline table? + // .into_table() does this conversion. + item.into_table() + .map_err(|item| format!("Expected a table, but is {}", item.type_name())) + }) } fn get_item_with>>( &self, name: impl ToConfigNamePath, - convert: impl FnOnce(ConfigValue) -> Result, + convert: impl FnOnce(ConfigItem) -> Result, ) -> Result { let name = name.into_name_path(); let name = name.borrow(); @@ -513,8 +576,7 @@ impl StackedConfig { to_merge .into_iter() .rev() - // TODO: remove sorting after migrating to toml_edit - .flat_map(|table| table.keys().sorted_unstable().map(|k| k.as_ref())) + .flat_map(|table| table.iter().map(|(k, _)| k)) .unique() } } @@ -524,7 +586,7 @@ impl StackedConfig { fn get_merged_item( layers: &[ConfigLayer], name: &ConfigNamePathBuf, -) -> Option<(ConfigValue, usize)> { +) -> Option<(ConfigItem, usize)> { let mut to_merge = Vec::new(); for (index, layer) in layers.iter().enumerate().rev() { let item = match layer.look_up_item(name) { @@ -532,7 +594,8 @@ fn get_merged_item( Ok(None) => continue, // parent is a table, but no value found Err(_) => break, // parent is not a table, shadows lower layers }; - if matches!(item.kind, config::ValueKind::Table(_)) { + // TODO: handle non-inline and inline tables differently? + if item.is_table_like() { to_merge.push((item, index)); } else if to_merge.is_empty() { return Some((item.clone(), index)); // no need to allocate vec @@ -557,7 +620,7 @@ fn get_merged_item( fn get_tables_to_merge<'a>( layers: &'a [ConfigLayer], name: &ConfigNamePathBuf, -) -> Vec<&'a ConfigTable> { +) -> Vec<&'a dyn toml_edit::TableLike> { let mut to_merge = Vec::new(); for layer in layers.iter().rev() { match layer.look_up_table(name) { @@ -570,22 +633,26 @@ fn get_tables_to_merge<'a>( } /// Merges `upper_item` fields into `lower_item` recursively. -fn merge_items(lower_item: &mut ConfigValue, upper_item: &ConfigValue) { - // TODO: If we switch to toml_edit, inline table will probably be treated as - // a value, not a table to be merged. For example, { fg = "red" } won't - // inherit other color parameters from the lower table. - let (config::ValueKind::Table(lower_table), config::ValueKind::Table(upper_table)) = - (&mut lower_item.kind, &upper_item.kind) +fn merge_items(lower_item: &mut ConfigItem, upper_item: &ConfigItem) { + // TODO: Inline table will probably be treated as a value, not a table to be + // merged. For example, { fg = "red" } won't inherit other color parameters + // from the lower table. + let (Some(lower_table), Some(upper_table)) = + (lower_item.as_table_like_mut(), upper_item.as_table_like()) else { // Not a table, the upper item wins. *lower_item = upper_item.clone(); return; }; - for (key, upper) in upper_table { - lower_table - .entry(key.clone()) - .and_modify(|lower| merge_items(lower, upper)) - .or_insert_with(|| upper.clone()); + for (key, upper) in upper_table.iter() { + match lower_table.entry(key) { + toml_edit::Entry::Occupied(entry) => { + merge_items(entry.into_mut(), upper); + } + toml_edit::Entry::Vacant(entry) => { + entry.insert(upper.clone()); + } + }; } } @@ -597,9 +664,61 @@ mod tests { use super::*; + #[test] + fn test_config_layer_set_value() { + let mut layer = ConfigLayer::empty(ConfigSource::User); + // Cannot overwrite the root table + assert_matches!( + layer.set_value(ConfigNamePathBuf::root(), 0), + Err(ConfigUpdateError::WouldOverwriteValue { name }) if name.is_empty() + ); + + // Insert some values + layer.set_value("foo", 1).unwrap(); + layer.set_value("bar.baz.blah", "2").unwrap(); + layer + .set_value("bar.qux", ConfigValue::from_iter([("inline", "table")])) + .unwrap(); + insta::assert_snapshot!(layer.data, @r#" + foo = 1 + + [bar] + qux = { inline = "table" } + + [bar.baz] + blah = "2" + "#); + + // Can overwrite value + layer + .set_value("foo", ConfigValue::from_iter(["new", "foo"])) + .unwrap(); + // Can overwrite inline table + layer.set_value("bar.qux", "new bar.qux").unwrap(); + // Cannot overwrite table + assert_matches!( + layer.set_value("bar", 0), + Err(ConfigUpdateError::WouldOverwriteTable { name }) if name == "bar" + ); + // Cannot overwrite value by table + assert_matches!( + layer.set_value("bar.baz.blah.blah", 0), + Err(ConfigUpdateError::WouldOverwriteValue { name }) if name == "bar.baz.blah" + ); + insta::assert_snapshot!(layer.data, @r#" + foo = ["new", "foo"] + + [bar] + qux = "new bar.qux" + + [bar.baz] + blah = "2" + "#); + } + #[test] fn test_stacked_config_layer_order() { - let empty_data = || config::Config::builder().build().unwrap(); + let empty_data = || DocumentMut::new(); let layer_sources = |config: &StackedConfig| { config .layers() @@ -686,10 +805,6 @@ mod tests { ConfigLayer::parse(ConfigSource::User, text).unwrap() } - fn parse_to_table(text: &str) -> ConfigTable { - new_user_layer(text).data.cache.into_table().unwrap() - } - #[test] fn test_stacked_config_get_simple_value() { let mut config = StackedConfig::empty(); @@ -726,6 +841,57 @@ mod tests { ); } + #[test] + fn test_stacked_config_get_table_as_value() { + let mut config = StackedConfig::empty(); + config.add_layer(new_user_layer(indoc! {" + a.b = { c = 'a.b.c #0' } + "})); + config.add_layer(new_user_layer(indoc! {" + a.d = ['a.d #1'] + "})); + + // Table can be converted to a value (so it can be deserialized to a + // structured value.) + insta::assert_snapshot!( + config.get_value("a").unwrap(), + @"{ b = { c = 'a.b.c #0' }, d = ['a.d #1'] }"); + } + + #[test] + fn test_stacked_config_get_inline_table() { + let mut config = StackedConfig::empty(); + config.add_layer(new_user_layer(indoc! {" + a.b = { c = 'a.b.c #0' } + "})); + config.add_layer(new_user_layer(indoc! {" + a.b = { d = 'a.b.d #1' } + "})); + + // Inline tables are merged. This might change later. + insta::assert_snapshot!( + config.get_value("a.b").unwrap(), + @" { c = 'a.b.c #0' , d = 'a.b.d #1' }"); + } + + #[test] + fn test_stacked_config_get_inline_non_inline_table() { + let mut config = StackedConfig::empty(); + config.add_layer(new_user_layer(indoc! {" + a.b = { c = 'a.b.c #0' } + "})); + config.add_layer(new_user_layer(indoc! {" + a.b.d = 'a.b.d #1' + "})); + + insta::assert_snapshot!( + config.get_value("a.b").unwrap(), + @" { c = 'a.b.c #0' , d = 'a.b.d #1'}"); + insta::assert_snapshot!( + config.get_table("a").unwrap(), + @"b = { c = 'a.b.c #0' , d = 'a.b.d #1'}"); + } + #[test] fn test_stacked_config_get_value_shadowing_table() { let mut config = StackedConfig::empty(); @@ -755,11 +921,7 @@ mod tests { config.add_layer(new_user_layer(indoc! {" a.b.c = 'a.b.c #1' "})); - - let expected = parse_to_table(indoc! {" - c = 'a.b.c #1' - "}); - assert_eq!(config.get_table("a.b").unwrap(), expected); + insta::assert_snapshot!(config.get_table("a.b").unwrap(), @"c = 'a.b.c #1'"); } #[test] @@ -775,14 +937,13 @@ mod tests { a.a.c = 'a.a.c #1' a.c = 'a.c #1' "})); - let expected = parse_to_table(indoc! {" - a.a = 'a.a.a #0' - a.b = 'a.a.b #1' - a.c = 'a.a.c #1' - b = 'a.b #0' - c = 'a.c #1' - "}); - assert_eq!(config.get_table("a").unwrap(), expected); + insta::assert_snapshot!(config.get_table("a").unwrap(), @r" + a.a = 'a.a.a #0' + a.b = 'a.a.b #1' + a.c = 'a.a.c #1' + b = 'a.b #0' + c = 'a.c #1' + "); assert_eq!(config.table_keys("a").collect_vec(), vec!["a", "b", "c"]); assert_eq!(config.table_keys("a.a").collect_vec(), vec!["a", "b", "c"]); assert_eq!(config.table_keys("a.b").collect_vec(), vec![""; 0]); @@ -804,10 +965,7 @@ mod tests { config.add_layer(new_user_layer(indoc! {" a.a.b = 'a.a.b #2' "})); - let expected = parse_to_table(indoc! {" - a.b = 'a.a.b #2' - "}); - assert_eq!(config.get_table("a").unwrap(), expected); + insta::assert_snapshot!(config.get_table("a").unwrap(), @"a.b = 'a.a.b #2'"); assert_eq!(config.table_keys("a").collect_vec(), vec!["a"]); assert_eq!(config.table_keys("a.a").collect_vec(), vec!["b"]); } @@ -827,11 +985,10 @@ mod tests { config.add_layer(new_user_layer(indoc! {" a.a.b = 'a.a.b #2' "})); - let expected = parse_to_table(indoc! {" - a.b = 'a.a.b #2' - b = 'a.b #0' - "}); - assert_eq!(config.get_table("a").unwrap(), expected); + insta::assert_snapshot!(config.get_table("a").unwrap(), @r" + a.b = 'a.a.b #2' + b = 'a.b #0' + "); assert_eq!(config.table_keys("a").collect_vec(), vec!["a", "b"]); assert_eq!(config.table_keys("a.a").collect_vec(), vec!["b"]); } @@ -850,11 +1007,8 @@ mod tests { config.add_layer(new_user_layer(indoc! {" a.a.b = 'a.a.b #2' "})); - let expected = parse_to_table(indoc! {" - b = 'a.a.b #2' - "}); // a is not under a.a, but it should still shadow lower layers - assert_eq!(config.get_table("a.a").unwrap(), expected); + insta::assert_snapshot!(config.get_table("a.a").unwrap(), @"b = 'a.a.b #2'"); assert_eq!(config.table_keys("a.a").collect_vec(), vec!["b"]); } } diff --git a/lib/src/settings.rs b/lib/src/settings.rs index 449a24898..e5351ba1d 100644 --- a/lib/src/settings.rs +++ b/lib/src/settings.rs @@ -374,10 +374,10 @@ impl TryFrom for HumanByteSize { type Error = &'static str; fn try_from(value: ConfigValue) -> Result { - if let Ok(n) = value.clone().into_int() { + if let Some(n) = value.as_integer() { let n = u64::try_from(n).map_err(|_| "Integer out of range")?; Ok(HumanByteSize(n)) - } else if let Ok(s) = value.into_string() { + } else if let Some(s) = value.as_str() { s.parse() } else { Err("Expected a positive integer or a string in '' form")