From c10b9d8685b506cd96c69701800abfd7dd7296d9 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 1 May 2022 08:54:35 -0700 Subject: [PATCH] cli: allow alias after global args, and recursive aliases 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 modifying the command defintion to have an "external subcommand" in the list of available commands. That make `clap` give us the remainder of the arguments when it runs into an unknown command. The first in the list will then be an alias or simply an unknown command. 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. With the new structure, it was easy to handle recursive alias definitions, so I added support for that too. Closes #292. --- CHANGELOG.md | 5 ++ src/commands.rs | 103 ++++++++++++++++++++++++---------- tests/test_alias.rs | 132 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 212 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd7560e221..44ae552093 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,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 @@ -111,6 +113,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=`, can now be passed before + an alias. + * Fixed relative path to the current directory in output to be `.` instead of empty string. diff --git a/src/commands.rs b/src/commands.rs index 12a00a3851..3ddc2c71f5 100644 --- a/src/commands.rs +++ b/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 ", version)] +#[clap( + name = "jj", + author = "Martin von Zweigbergk ", + version +)] #[clap(mut_arg("help", |arg| { arg .help("Print help information, more help with --help than with -h") @@ -1102,6 +1106,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), @@ -1139,6 +1164,9 @@ enum Commands { Git(GitArgs), Bench(BenchArgs), Debug(DebugArgs), + /// An alias or an unknown command + #[clap(external_subcommand)] + Alias(Vec), } /// Create a new repo in the given directory @@ -5034,34 +5062,53 @@ fn string_list_from_config(value: config::Value) -> Option> { } } -fn resolve_alias(settings: &UserSettings, args: Vec) -> Result, CommandError> { - if args.len() >= 2 { - let command_name = args[1].clone(); - match settings - .config() - .get::(&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 { + let mut resolved_aliases = HashSet::new(); + let mut string_args = string_args.to_vec(); + let mut earlier_global_args = None; + loop { + let mut args: Args = clap::Parser::parse_from(&string_args); + if let Some(earlier_global_args) = &earlier_global_args { + args.global_args.merge_from(earlier_global_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::(&format!("alias.{}", alias_name)) + { + Ok(value) => { + if let Some(alias_definition) = string_list_from_config(value) { + let executable_name = string_args[0].clone(); + string_args = vec![executable_name]; + string_args.extend(alias_definition); + string_args.extend_from_slice(&alias_args[1..]); + resolved_aliases.insert(alias_name.clone()); + earlier_global_args = Some(args.global_args.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 +5127,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 +5165,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"), } } diff --git a/tests/test_alias.rs b/tests/test_alias.rs index d9b4b0ba6b..c7aaf0a2a8 100644 --- a/tests/test_alias.rs +++ b/tests/test_alias.rs @@ -16,6 +16,138 @@ 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 + // 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();