Skip to content

Commit

Permalink
cli: allow alias after global args, and recursive aliases
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@martinvonz
martinvonz committed May 12, 2022
1 parent d0ecbaf commit 788831f
Show file tree
Hide file tree
Showing 3 changed files with 189 additions and 28 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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.

Expand Down
77 changes: 49 additions & 28 deletions src/commands.rs
View file
Expand Up @@ -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")
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}

Expand All @@ -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),
Expand Down Expand Up @@ -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"),
}
}

Expand Down
135 changes: 135 additions & 0 deletions tests/test_alias.rs
View file
Expand Up @@ -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();
Expand Down

0 comments on commit 788831f

Please sign in to comment.