cli: allow alias after global args

Our support for aliases is very naively implemented; it assumes the
alias is the first argument in argv. It therefore fails to resolve
aliases after global arguments such as `--at-op`.

This patch fixes that by using doing an intial parse of the arguments
with a modified definition of the CLI passed to `clap`. The modified
definition has only global arguments, plus an "external
subcommand". That way we can convince `clap` to parse the global
arguments for us and give us the alias (or real command) and further
arguments back. Thanks to @epage for the suggestion on in
clap-rs/clap#3672. The only minor problem is that it doesn't seem
possible to tell `clap` to parse global arguments that come after the
external subcommand. To work around that, we manually merge any parsed
global argument before the alias with any parsed global arguments
after it.

Closes #292.

CHANGELOG.md

@@ -111,6 +111,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

@@ -1055,6 +1055,21 @@ struct Args {
     command: Commands,
 }
 
+/// Used for resolving aliases
+#[derive(clap::Parser, Clone, Debug)]
+struct AliasArgs {
+    #[clap(flatten)]
+    global_args: GlobalArgs,
+    #[clap(subcommand)]
+    command: AliasCommand,
+}
+
+#[derive(Subcommand, Debug, Clone)]
+enum AliasCommand {
+    #[clap(external_subcommand)]
+    Alias(Vec<String>),
+}
+
 #[derive(clap::Parser, Clone, Debug)]
 struct GlobalArgs {
     /// Path to repository to operate on
@@ -1102,6 +1117,27 @@ struct GlobalArgs {
     at_operation: String,
 }
 
+impl GlobalArgs {
+    fn merge_from(&mut self, other: &GlobalArgs) {
+        // Destructure `other`, so we're forced to update this code if new fields are
+        // added in the future.
+        let GlobalArgs {
+            repository,
+            no_commit_working_copy,
+            at_operation,
+        } = other;
+        if self.repository.is_none() {
+            self.repository = repository.clone();
+        }
+        if *no_commit_working_copy {
+            self.no_commit_working_copy = true;
+        }
+        if at_operation != "@" {
+            self.at_operation = at_operation.clone()
+        }
+    }
+}
+
 #[derive(Subcommand, Clone, Debug)]
 enum Commands {
     Init(InitArgs),
@@ -5034,34 +5070,41 @@ 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,
-                    )))
-                }
-            }
-            Err(config::ConfigError::NotFound(_)) => {
-                // The command is not an alias
+fn parse_args(settings: &UserSettings, string_args: &[String]) -> Result<Args, CommandError> {
+    let alias_args: AliasArgs = clap::Parser::parse_from(string_args);
+    let AliasCommand::Alias(command_args) = alias_args.command;
+    let command_name = command_args[0].clone();
+    match settings
+        .config()
+        .get::<config::Value>(&format!("alias.{}", command_name))
+    {
+        Ok(value) => {
+            if let Some(alias_definition) = string_list_from_config(value) {
+                let executable_name = string_args[0].clone();
+                let mut resolved_args = vec![executable_name];
+                resolved_args.extend(alias_definition);
+                resolved_args.extend_from_slice(&command_args[1..]);
+                // Re-parse the expanded alias and the remainder of the arguments.
+                // The way this works means that `jj --at-op=@ my-alias --at-op=@-` is allowed
+                // (and the second one wins).
+                let mut args: Args = clap::Parser::parse_from(&resolved_args);
+                // Merge in global arguments that came after the alias
+                args.global_args.merge_from(&alias_args.global_args);
+                // TODO: Resolve aliases that call other aliases
                 Ok(args)
+            } else {
+                Err(CommandError::UserError(format!(
+                    r#"Alias definition for "{}" must be a string list"#,
+                    command_name,
+                )))
             }
-            Err(err) => Err(CommandError::from(err)),
         }
-    } else {
-        Ok(args)
+        Err(config::ConfigError::NotFound(_)) => {
+            // The command is not an alias
+            let args: Args = clap::Parser::parse_from(string_args);
+            Ok(args)
+        }
+        Err(err) => Err(CommandError::from(err)),
     }
 }
 
@@ -5080,9 +5123,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),

tests/test_alias.rs

@@ -16,6 +16,98 @@ use crate::common::TestEnvironment;
 
 pub mod common;
 
+#[test]
+fn test_alias() {
+    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_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]
+    log = ["log", "-r", "root"]
+    "#,
+    );
+    // Alias should be resolved and not cause infinite recursion
+    let stdout = test_env.jj_cmd_success(&repo_path, &["log"]);
+    insta::assert_snapshot!(stdout, @r###"
+    o 000000000000 000000000000  1970-01-01 00:00:00.000 +00:00   
+
+    "###);
+}
+
+#[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
+    // TODO: This should ideally fail just like it does when there's no alias
+    // involved.
+    let stdout = test_env.jj_cmd_success(&repo_path, &["--at-op", "@", "l", "--at-op", "@-"]);
+    insta::assert_snapshot!(stdout, @r###"
+    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();