From 628677fba1630de4875a3f7f8cafa9cae2ae45ed 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 makes `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. 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 | 77 ++++++++++++++++--------- tests/test_alias.rs | 135 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 189 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7558615926..e622527fc7 100644 --- a/CHANGELOG.md +++ b/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=`, 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..0c1bbe3787 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") @@ -1139,6 +1143,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 +5041,48 @@ 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(); + 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::(&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"), } } diff --git a/tests/test_alias.rs b/tests/test_alias.rs index d9b4b0ba6b..80a3e20b8c 100644 --- a/tests/test_alias.rs +++ b/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();