cli: allow alias after global args

CHANGELOG.md

@@ -81,6 +81,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * `jj branch` can accept any number of branches to update, rather than just one.
 
+* Aliases can now call other aliases.
+
 ### Fixed bugs
 
 * When rebasing a conflict where one side modified a file and the other side
@@ -114,6 +116,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * You now get a proper error message instead of a crash when `$EDITOR` doesn't
   exist or exits with an error.
 
+* Global arguments, such as `--at-op=<operation>`, can now be passed before
+  an alias.
+
 * Fixed relative path to the current directory in output to be `.` instead of
   empty string.
 

src/commands.rs

@@ -1042,7 +1042,11 @@ fn update_working_copy(
 ///
 /// To get started, see the tutorial at https://github.com/martinvonz/jj/blob/main/docs/tutorial.md.
 #[derive(clap::Parser, Clone, Debug)]
-#[clap(author = "Martin von Zweigbergk <martinvonz@google.com>", version)]
+#[clap(
+    name = "jj",
+    author = "Martin von Zweigbergk <martinvonz@google.com>",
+    version
+)]
 #[clap(mut_arg("help", |arg| {
     arg
         .help("Print help information, more help with --help than with -h")
@@ -1139,6 +1143,9 @@ enum Commands {
     Git(GitArgs),
     Bench(BenchArgs),
     Debug(DebugArgs),
+    /// An alias or an unknown command
+    #[clap(external_subcommand)]
+    Alias(Vec<String>),
 }
 
 /// Create a new repo in the given directory
@@ -5034,34 +5041,48 @@ fn string_list_from_config(value: config::Value) -> Option<Vec<String>> {
     }
 }
 
-fn resolve_alias(settings: &UserSettings, args: Vec<String>) -> Result<Vec<String>, CommandError> {
-    if args.len() >= 2 {
-        let command_name = args[1].clone();
-        match settings
-            .config()
-            .get::<config::Value>(&format!("alias.{}", command_name))
-        {
-            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,
-                    )))
-                }
+fn parse_args(settings: &UserSettings, string_args: &[String]) -> Result<Args, CommandError> {
+    let mut resolved_aliases = HashSet::new();
+    let mut string_args = string_args.to_vec();
+    loop {
+        let args: Args = clap::Parser::parse_from(&string_args);
+        if let Commands::Alias(alias_args) = &args.command {
+            let alias_name = &alias_args[0];
+            if resolved_aliases.contains(alias_name) {
+                return Err(CommandError::UserError(format!(
+                    r#"Recursive alias definition involving "{alias_name}""#
+                )));
             }
-            Err(config::ConfigError::NotFound(_)) => {
-                // The command is not an alias
-                Ok(args)
+            match settings
+                .config()
+                .get::<config::Value>(&format!("alias.{}", alias_name))
+            {
+                Ok(value) => {
+                    if let Some(alias_definition) = string_list_from_config(value) {
+                        assert!(string_args.ends_with(alias_args.as_slice()));
+                        string_args.truncate(string_args.len() - alias_args.len());
+                        string_args.extend(alias_definition);
+                        string_args.extend_from_slice(&alias_args[1..]);
+                        resolved_aliases.insert(alias_name.clone());
+                    } else {
+                        return Err(CommandError::UserError(format!(
+                            r#"Alias definition for "{alias_name}" must be a string list"#
+                        )));
+                    }
+                }
+                Err(config::ConfigError::NotFound(_)) => {
+                    let mut app = Args::command();
+                    app.error(clap::ErrorKind::ArgumentNotFound, format!(
+                        r#"Found argument '{alias_name}' which wasn't expected, or isn't valid in this context"#
+                    )).exit();
+                }
+                Err(err) => {
+                    return Err(CommandError::from(err));
+                }
             }
-            Err(err) => Err(CommandError::from(err)),
+        } else {
+            return Ok(args);
         }
-    } else {
-        Ok(args)
     }
 }
 
@@ -5080,9 +5101,8 @@ where
         }
     }
 
-    let string_args = resolve_alias(ui.settings(), string_args)?;
+    let args = parse_args(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.global_args.clone());
     match &args.command {
         Commands::Init(sub_args) => cmd_init(ui, &command_helper, sub_args),
@@ -5119,6 +5139,7 @@ where
         Commands::Git(sub_args) => cmd_git(ui, &command_helper, sub_args),
         Commands::Bench(sub_args) => cmd_bench(ui, &command_helper, sub_args),
         Commands::Debug(sub_args) => cmd_debug(ui, &command_helper, sub_args),
+        Commands::Alias(_) => panic!("Unresolved alias"),
     }
 }
 

tests/test_alias.rs

@@ -16,6 +16,141 @@ use crate::common::TestEnvironment;
 
 pub mod common;
 
+#[test]
+fn test_alias_basic() {
+    let test_env = TestEnvironment::default();
+    test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
+    let repo_path = test_env.env_root().join("repo");
+
+    test_env.add_config(
+        br#"[alias]
+    b = ["log", "-r", "@", "-T", "branches"]
+    "#,
+    );
+    test_env.jj_cmd_success(&repo_path, &["branch", "my-branch"]);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["b"]);
+    insta::assert_snapshot!(stdout, @r###"
+    @ my-branch
+    ~ 
+    "###);
+}
+
+#[test]
+fn test_alias_calls_unknown_command() {
+    let test_env = TestEnvironment::default();
+    test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
+    let repo_path = test_env.env_root().join("repo");
+
+    test_env.add_config(
+        br#"[alias]
+    foo = ["nonexistent"]
+    "#,
+    );
+    // Should get an error about the unknown command
+    test_env.jj_cmd_cli_error(&repo_path, &["foo"]);
+}
+
+#[test]
+fn test_alias_cannot_override_builtin() {
+    let test_env = TestEnvironment::default();
+    test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
+    let repo_path = test_env.env_root().join("repo");
+
+    test_env.add_config(
+        br#"[alias]
+    log = ["rebase"]
+    "#,
+    );
+    // Alias should be ignored
+    let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-r", "root"]);
+    insta::assert_snapshot!(stdout, @r###"
+    o 000000000000 000000000000  1970-01-01 00:00:00.000 +00:00   
+
+    "###);
+}
+
+#[test]
+fn test_alias_recursive() {
+    let test_env = TestEnvironment::default();
+    test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
+    let repo_path = test_env.env_root().join("repo");
+
+    test_env.add_config(
+        br#"[alias]
+    foo = ["foo"]
+    bar = ["baz"]
+    baz = ["bar"]
+    "#,
+    );
+    // Alias should not cause infinite recursion or hang
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["foo"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Error: Recursive alias definition involving "foo"
+    "###);
+    // Also test with mutual recursion
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["bar"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Error: Recursive alias definition involving "bar"
+    "###);
+}
+
+#[test]
+fn test_alias_global_args_before_and_after() {
+    let test_env = TestEnvironment::default();
+    test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
+    let repo_path = test_env.env_root().join("repo");
+    test_env.add_config(
+        br#"[alias]
+    l = ["log", "-T", "commit_id"]
+    "#,
+    );
+    // Test the setup
+    let stdout = test_env.jj_cmd_success(&repo_path, &["l"]);
+    insta::assert_snapshot!(stdout, @r###"
+    @ 230dd059e1b059aefc0da06a2e5a7dbf22362f22
+    o 0000000000000000000000000000000000000000
+    "###);
+
+    // Can pass global args before
+    let stdout = test_env.jj_cmd_success(&repo_path, &["l", "--at-op", "@-"]);
+    insta::assert_snapshot!(stdout, @r###"
+    o 0000000000000000000000000000000000000000
+    "###);
+    // Can pass global args after
+    let stdout = test_env.jj_cmd_success(&repo_path, &["--at-op", "@-", "l"]);
+    insta::assert_snapshot!(stdout, @r###"
+    o 0000000000000000000000000000000000000000
+    "###);
+    // Test passing global args both before and after
+    let stdout = test_env.jj_cmd_success(&repo_path, &["--at-op", "abc123", "l", "--at-op", "@-"]);
+    insta::assert_snapshot!(stdout, @r###"
+    o 0000000000000000000000000000000000000000
+    "###);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["-R", "../nonexistent", "l", "-R", "."]);
+    insta::assert_snapshot!(stdout, @r###"
+    @ 230dd059e1b059aefc0da06a2e5a7dbf22362f22
+    o 0000000000000000000000000000000000000000
+    "###);
+}
+
+#[test]
+fn test_alias_global_args_in_definition() {
+    let test_env = TestEnvironment::default();
+    test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
+    let repo_path = test_env.env_root().join("repo");
+    test_env.add_config(
+        br#"[alias]
+    l = ["log", "-T", "commit_id", "--at-op", "@-"]
+    "#,
+    );
+
+    // The global argument in the alias is respected
+    let stdout = test_env.jj_cmd_success(&repo_path, &["l"]);
+    insta::assert_snapshot!(stdout, @r###"
+    o 0000000000000000000000000000000000000000
+    "###);
+}
+
 #[test]
 fn test_alias_invalid_definition() {
     let test_env = TestEnvironment::default();