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 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.
  • Loading branch information
@martinvonz
martinvonz committed May 9, 2022
1 parent c5a312b commit 2f48cd6
Show file tree
Hide file tree
Showing 3 changed files with 212 additions and 28 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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=<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
103 changes: 75 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 @@ -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),
Expand Down Expand Up @@ -1139,6 +1164,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 +5062,53 @@ 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();
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::<config::Value>(&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)
}
}

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

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

0 comments on commit 2f48cd6

Please sign in to comment.