cli: make invalid alias definition an error

CHANGELOG.md

@@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   restore to as an argument to `-o/--operation`. It is now a positional
   argument instead (i.e. `jj undo -o abc123` is now written `jj undo abc123`).
 
+* An alias that is not configured as a string list (e.g. `my-status = "status"`
+  instead of `my-status = ["status"]`) is now an error instead of a warning.
+
 ### New features
 
 * `jj rebase` now accepts a `--branch/-b <revision>` argument, which can be used

src/commands.rs

@@ -30,6 +30,7 @@ use std::time::Instant;
 use std::{fs, io};
 
 use clap::{ArgGroup, CommandFactory, Subcommand};
+use config::Value;
 use criterion::Criterion;
 use git2::{Oid, Repository};
 use itertools::Itertools;
@@ -93,6 +94,12 @@ impl From<std::io::Error> for CommandError {
     }
 }
 
+impl From<config::ConfigError> for CommandError {
+    fn from(err: config::ConfigError) -> Self {
+        CommandError::UserError(format!("Config error: {}", err))
+    }
+}
+
 impl From<BackendError> for CommandError {
     fn from(err: BackendError) -> Self {
         CommandError::UserError(format!("Unexpected error from store: {}", err))
@@ -4995,33 +5002,61 @@ fn cmd_git(ui: &mut Ui, command: &CommandHelper, args: &GitArgs) -> Result<(), C
     }
 }
 
-fn resolve_alias(ui: &mut Ui, args: Vec<String>) -> Vec<String> {
+fn string_list_from_config(value: config::Value) -> Option<Vec<String>> {
+    match value {
+        Value {
+            kind: config::ValueKind::Array(elements),
+            ..
+        } => {
+            let mut strings = vec![];
+            for arg in elements {
+                match arg {
+                    config::Value {
+                        kind: config::ValueKind::String(string_value),
+                        ..
+                    } => {
+                        strings.push(string_value);
+                    }
+                    _ => {
+                        return None;
+                    }
+                }
+            }
+            Some(strings)
+        }
+        _ => None,
+    }
+}
+
+fn resolve_alias(settings: &UserSettings, args: Vec<String>) -> Result<Vec<String>, CommandError> {
     if args.len() >= 2 {
         let command_name = args[1].clone();
-        if let Ok(alias_definition) = ui
-            .settings()
+        match settings
             .config()
-            .get_array(&format!("alias.{}", command_name))
+            .get::<config::Value>(&format!("alias.{}", command_name))
         {
-            let mut resolved_args = vec![args[0].clone()];
-            for arg in alias_definition {
-                match arg.into_string() {
-                    Ok(string_arg) => resolved_args.push(string_arg),
-                    Err(err) => {
-                        ui.write_error(&format!(
-                            "Warning: Ignoring bad alias definition: {:?}\n",
-                            err
-                        ))
-                        .unwrap();
-                        return args;
-                    }
+            Ok(value) => {
+                if let Some(alias_definition) = string_list_from_config(value) {
+                    let mut resolved_args = vec![args[0].clone()];
+                    resolved_args.extend(alias_definition);
+                    resolved_args.extend_from_slice(&args[2..]);
+                    Ok(resolved_args)
+                } else {
+                    Err(CommandError::UserError(format!(
+                        r#"Alias definition for "{}" must be a string list"#,
+                        command_name,
+                    )))
                 }
             }
-            resolved_args.extend_from_slice(&args[2..]);
-            return resolved_args;
+            Err(config::ConfigError::NotFound(_)) => {
+                // The command is not an alias
+                Ok(args)
+            }
+            Err(err) => Err(CommandError::from(err)),
         }
+    } else {
+        Ok(args)
     }
-    args
 }
 
 pub fn dispatch<I, T>(ui: &mut Ui, args: I) -> Result<(), CommandError>
@@ -5039,7 +5074,7 @@ where
         }
     }
 
-    let string_args = resolve_alias(ui, string_args);
+    let string_args = resolve_alias(ui.settings(), string_args)?;
     let app = Args::command();
     let args: Args = clap::Parser::parse_from(&string_args);
     let command_helper = CommandHelper::new(app, string_args, args.clone());

tests/test_alias.rs

@@ -0,0 +1,37 @@
+// Copyright 2022 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use crate::common::TestEnvironment;
+
+pub mod common;
+
+#[test]
+fn test_alias_invalid_definition() {
+    let test_env = TestEnvironment::default();
+
+    test_env.add_config(
+        br#"[alias]
+    non-list = 5
+    non-string-list = [7]
+    "#,
+    );
+    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
+    "###);
+    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
+    "###);
+}