cli: replace uses of .get_table() to include better error indication

This will also help migrate to toml_edit, where a value type doesn't provide
a deserialization helper.

cli/src/cli_util.rs

@@ -3148,22 +3148,20 @@ fn resolve_aliases(
     app: &Command,
     mut string_args: Vec<String>,
 ) -> Result<Vec<String>, CommandError> {
-    let mut aliases_map = config.get_table("aliases")?;
+    let defined_aliases: HashSet<_> = config.table_keys("aliases").collect();
     let mut resolved_aliases = HashSet::new();
     let mut real_commands = HashSet::new();
     for command in app.get_subcommands() {
-        real_commands.insert(command.get_name().to_string());
+        real_commands.insert(command.get_name());
         for alias in command.get_all_aliases() {
-            real_commands.insert(alias.to_string());
+            real_commands.insert(alias);
         }
     }
-    for alias in aliases_map.keys() {
-        if real_commands.contains(alias) {
-            writeln!(
-                ui.warning_default(),
-                "Cannot define an alias that overrides the built-in command '{alias}'"
-            )?;
-        }
+    for alias in defined_aliases.intersection(&real_commands).sorted() {
+        writeln!(
+            ui.warning_default(),
+            "Cannot define an alias that overrides the built-in command '{alias}'"
+        )?;
     }
 
     loop {
@@ -3177,24 +3175,19 @@ fn resolve_aliases(
                     .unwrap_or_default()
                     .map(|arg| arg.to_str().unwrap().to_string())
                     .collect_vec();
-                if resolved_aliases.contains(&alias_name) {
+                if resolved_aliases.contains(&*alias_name) {
                     return Err(user_error(format!(
                         r#"Recursive alias definition involving "{alias_name}""#
                     )));
                 }
-                if let Some(value) = aliases_map.remove(&alias_name) {
-                    if let Ok(alias_definition) = value.try_deserialize::<Vec<String>>() {
-                        assert!(string_args.ends_with(&alias_args));
-                        string_args.truncate(string_args.len() - 1 - alias_args.len());
-                        string_args.extend(alias_definition);
-                        string_args.extend_from_slice(&alias_args);
-                        resolved_aliases.insert(alias_name.clone());
-                        continue;
-                    } else {
-                        return Err(user_error(format!(
-                            r#"Alias definition for "{alias_name}" must be a string list"#
-                        )));
-                    }
+                if let Some(&alias_name) = defined_aliases.get(&*alias_name) {
+                    let alias_definition: Vec<String> = config.get(["aliases", alias_name])?;
+                    assert!(string_args.ends_with(&alias_args));
+                    string_args.truncate(string_args.len() - 1 - alias_args.len());
+                    string_args.extend(alias_definition);
+                    string_args.extend_from_slice(&alias_args);
+                    resolved_aliases.insert(alias_name);
+                    continue;
                 } else {
                     // Not a real command and not an alias, so return what we've resolved so far
                     return Ok(string_args);

cli/src/commands/fix.rs

@@ -473,38 +473,36 @@ fn get_tools_config(ui: &mut Ui, settings: &UserSettings) -> Result<ToolsConfig,
             to_toml_value(&settings.get_value("fix.tool-command").unwrap()).unwrap()
         )?;
     }
-    if let Ok(tools_table) = settings.get_table("fix.tools") {
-        // Convert the map into a sorted vector early so errors are deterministic.
-        let mut tools: Vec<ToolConfig> = tools_table
-            .into_iter()
-            .sorted_by(|a, b| a.0.cmp(&b.0))
-            .map(|(name, value)| -> Result<ToolConfig, CommandError> {
-                let mut diagnostics = FilesetDiagnostics::new();
-                let tool: RawToolConfig = value.try_deserialize()?;
-                let expression = FilesetExpression::union_all(
-                    tool.patterns
-                        .iter()
-                        .map(|arg| {
-                            fileset::parse(
-                                &mut diagnostics,
-                                arg,
-                                &RepoPathUiConverter::Fs {
-                                    cwd: "".into(),
-                                    base: "".into(),
-                                },
-                            )
-                        })
-                        .try_collect()?,
-                );
-                print_parse_diagnostics(ui, &format!("In `fix.tools.{name}`"), &diagnostics)?;
-                Ok(ToolConfig {
-                    command: tool.command,
-                    matcher: expression.to_matcher(),
-                })
+    let tools: Vec<ToolConfig> = settings
+        .table_keys("fix.tools")
+        // Sort keys early so errors are deterministic.
+        .sorted()
+        .map(|name| -> Result<ToolConfig, CommandError> {
+            let mut diagnostics = FilesetDiagnostics::new();
+            let tool: RawToolConfig = settings.get(["fix", "tools", name])?;
+            let expression = FilesetExpression::union_all(
+                tool.patterns
+                    .iter()
+                    .map(|arg| {
+                        fileset::parse(
+                            &mut diagnostics,
+                            arg,
+                            &RepoPathUiConverter::Fs {
+                                cwd: "".into(),
+                                base: "".into(),
+                            },
+                        )
+                    })
+                    .try_collect()?,
+            );
+            print_parse_diagnostics(ui, &format!("In `fix.tools.{name}`"), &diagnostics)?;
+            Ok(ToolConfig {
+                command: tool.command,
+                matcher: expression.to_matcher(),
             })
-            .try_collect()?;
-        tools_config.tools.append(&mut tools);
-    }
+        })
+        .try_collect()?;
+    tools_config.tools.extend(tools);
     if tools_config.tools.is_empty() {
         // TODO: This is not a useful message when one or both fields are present but
         // have the wrong type. After removing `fix.tool-command`, it will be simpler to

cli/src/complete.rs

@@ -206,8 +206,7 @@ pub fn git_remotes() -> Vec<CompletionCandidate> {
 pub fn aliases() -> Vec<CompletionCandidate> {
     with_jj(|_, settings| {
         Ok(settings
-            .get_table("aliases")?
-            .into_keys()
+            .table_keys("aliases")
             // This is opinionated, but many people probably have several
             // single- or two-letter aliases they use all the time. These
             // aliases don't need to be completed and they would only clutter

cli/src/formatter.rs

@@ -407,8 +407,7 @@ impl<W: Write> ColorFormatter<W> {
 
 fn rules_from_config(config: &StackedConfig) -> Result<Rules, ConfigGetError> {
     let mut result = vec![];
-    let table = config.get_table("colors")?;
-    for (key, value) in table {
+    for key in config.table_keys("colors") {
         let to_config_err = |message: String| ConfigGetError::Type {
             name: format!("colors.'{key}'"),
             error: message.into(),
@@ -418,6 +417,7 @@ fn rules_from_config(config: &StackedConfig) -> Result<Rules, ConfigGetError> {
             .split_whitespace()
             .map(ToString::to_string)
             .collect_vec();
+        let value = config.get_value(["colors", key])?;
         match value.kind {
             config::ValueKind::String(color_name) => {
                 let style = Style {

cli/tests/test_alias.rs

@@ -234,13 +234,19 @@ fn test_alias_invalid_definition() {
     "#,
     );
     let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["non-list"]);
-    insta::assert_snapshot!(stderr, @r###"
-    Error: Alias definition for "non-list" must be a string 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, @r###"
-    Error: Alias definition for "non-string-list" must be a string list
-    "###);
+    insta::assert_snapshot!(stderr.replace('\\', "/"), @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
+    Hint: Check the config file: $TEST_ENV/config/config0002.toml
+    For help, see https://martinvonz.github.io/jj/latest/config/.
+    ");
 }
 
 #[test]

cli/tests/test_fix_command.rs

@@ -256,10 +256,12 @@ fn test_config_tables_all_commands_missing() {
     std::fs::write(repo_path.join("foo"), "foo\n").unwrap();
 
     let stderr = test_env.jj_cmd_failure(&repo_path, &["fix"]);
-    insta::assert_snapshot!(stderr, @r###"
-    Config error: missing field `command`
+    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/.
-    "###);
+    ");
 
     let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo", "-r", "@"]);
     insta::assert_snapshot!(content, @"foo\n");
@@ -288,10 +290,12 @@ fn test_config_tables_some_commands_missing() {
     std::fs::write(repo_path.join("foo"), "foo\n").unwrap();
 
     let stderr = test_env.jj_cmd_failure(&repo_path, &["fix"]);
-    insta::assert_snapshot!(stderr, @r###"
-    Config error: missing field `command`
+    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/.
-    "###);
+    ");
 
     let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo", "-r", "@"]);
     insta::assert_snapshot!(content, @"foo\n");