cli: add support for jj help alias

jyn514 · jyn514 · commit bdfcd0f1af3b · 2024-05-17T11:33:59.000-04:00
i consider rewriting the `help` command, but that breaks some other things (e.g. tab completion doesn't work, because `clap_complete` doesn't know about the aliases). instead, i found that i can add each alias to the `clap::Command` dynamically at runtime. this works! the help it shows is useful and easy to modify, and it's only about 25 lines of code. the problem is this breaks `resolve_aliases` pretty badly. the replacement loop uses *clap's* API to determine the arguments that come after the alias, and clap does not expose a way to get the raw arguments, only the parsed arguments. it happened to work before because the commands were all `External`, in which case clap doesn't do any parsing. i tried to just stop using clap's API, which simplifies the code a lot and works with the new dynamically added alias subcommands. but that breaks global flags that precede the alias, things like `jj --no-pager alias`. instead, i waited to add the aliases to `Command` until after replacing the string arguments. that breaks because `expand_args()` gets called twice, once before and once after processing `--repository`. so even if i do this afterwards in the `resolve_aliases` function, the *next* call will still break because it parses differently. to work around *that*, i clone the app before passing it to `expand_args`, so that the modifications don't affect the --repository parsing. it's kind of hacky, but i haven't found a simpler solution. --- ## future work * `jj alias -h` shows the help for the *resolved* alias, not the "Alias for ..." short help. that happens because the alias is replaced with its definition before passing it to get_matches. it would be nice to make it consistent with `help`. * `jj help` without arguments shows builtin aliases in a different format than user-defined aliases (builtins look like `[aliases: st]` in the short help). i tried to make these consistent, but [ran into trouble](clap-rs/clap#5497) that i'm not sure is fixable.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * `jj split` will now refuse to split an empty commit.
 
+* TOML type errors in alias definitions are now reported eagerly. Previously, they would not error unless you used the alias in a jj command.
+
 ### Deprecations
 
 - Attempting to alias a built-in command now gives a warning, rather than being silently ignored.
@@ -33,6 +35,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * `jj branch track` now show conflicts if there are some.
 
+* `jj help` and `jj util completion` now support aliases.
+
+
 ### Fixed bugs
 
 * When the working copy commit becomes immutable, a new one is automatically created on top of it 
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/cli/Cargo.toml b/cli/Cargo.toml
@@ -74,6 +74,7 @@ regex = { workspace = true }
 rpassword = { workspace = true }
 scm-record = { workspace = true }
 serde = { workspace = true }
+shlex = "1.3.0"
 slab = { workspace = true }
 strsim = { workspace = true }
 tempfile = { workspace = true }
diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs
@@ -14,7 +14,7 @@
 
 use core::fmt;
 use std::borrow::Cow;
-use std::collections::{BTreeMap, HashSet};
+use std::collections::{BTreeMap, HashMap, HashSet};
 use std::env::{self, ArgsOs, VarError};
 use std::ffi::OsString;
 use std::fmt::Debug;
@@ -2599,12 +2599,15 @@ fn resolve_default_command(
     Ok(string_args)
 }
 
-fn resolve_aliases(
+type AliasMap = HashMap<String, Vec<String>>;
+
+// NOTE: logging doesn't work in this function because we don't enable it until after expanding arguments.
+fn expand_aliases(
     ui: &Ui,
     config: &config::Config,
     app: &Command,
     mut string_args: Vec<String>,
-) -> Result<Vec<String>, CommandError> {
+) -> Result<(Vec<String>, AliasMap), CommandError> {
     let mut aliases_map = config.get_table("aliases")?;
     if let Ok(alias_map) = config.get_table("alias") {
         for (alias, definition) in alias_map {
@@ -2618,21 +2621,31 @@ fn resolve_aliases(
         }
     }
 
-    let mut resolved_aliases = HashSet::new();
+    let mut expanded_aliases = HashSet::new();
     let mut real_commands = HashSet::new();
     for command in app.get_subcommands() {
         real_commands.insert(command.get_name().to_string());
         for alias in command.get_all_aliases() {
             real_commands.insert(alias.to_string());
         }
     }
-    for alias in aliases_map.keys() {
-        if real_commands.contains(alias) {
+
+    let mut resolved_alias_map = HashMap::new();
+    for (alias_name, value) in aliases_map {
+        if real_commands.contains(&alias_name) {
             writeln!(
                 ui.warning_default(),
-                "Cannot define an alias that overrides the built-in command '{alias}'"
+                "Cannot define an alias that overrides the built-in command '{alias_name}'"
             )?;
+            continue;
         }
+
+        let Ok(alias_definition) = value.try_deserialize::<Vec<String>>() else {
+            return Err(user_error(format!(
+                r#"Alias definition for "{alias_name}" must be a string list"#
+            )));
+        };
+        resolved_alias_map.insert(alias_name, alias_definition);
     }
 
     loop {
@@ -2646,32 +2659,85 @@ fn resolve_aliases(
                     .unwrap_or_default()
                     .map(|arg| arg.to_str().unwrap().to_string())
                     .collect_vec();
-                if resolved_aliases.contains(&alias_name) {
+                if expanded_aliases.contains(&alias_name) {
                     return Err(user_error(format!(
                         r#"Recursive alias definition involving "{alias_name}""#
                     )));
                 }
-                if let Some(value) = aliases_map.remove(&alias_name) {
-                    if let Ok(alias_definition) = value.try_deserialize::<Vec<String>>() {
-                        assert!(string_args.ends_with(&alias_args));
-                        string_args.truncate(string_args.len() - 1 - alias_args.len());
-                        string_args.extend(alias_definition);
-                        string_args.extend_from_slice(&alias_args);
-                        resolved_aliases.insert(alias_name.clone());
-                        continue;
-                    } else {
-                        return Err(user_error(format!(
-                            r#"Alias definition for "{alias_name}" must be a string list"#
-                        )));
-                    }
+                if let Some(alias_definition) = resolved_alias_map.get(&alias_name) {
+                    assert!(string_args.ends_with(&alias_args));
+                    string_args.truncate(string_args.len() - 1 - alias_args.len());
+                    string_args.extend(alias_definition.clone());
+                    string_args.extend_from_slice(&alias_args);
+                    expanded_aliases.insert(alias_name.clone());
+                    continue;
                 } else {
                     // Not a real command and not an alias, so return what we've resolved so far
-                    return Ok(string_args);
+                    break;
                 }
             }
         }
         // No more alias commands, or hit unknown option
-        return Ok(string_args);
+        break;
+    }
+
+    Ok((string_args, resolved_alias_map))
+}
+
+fn add_alias_help(app: &mut Command, mut aliases: AliasMap) {
+    let render_definition = |def: &[String]| {
+        match shlex::try_join(def.into_iter().map(|s| &**s)) {
+            Ok(s) => format!("Alias for \"{s}\""),
+            Err(_) => format!("Alias for {def:?}"),
+        }
+    };
+
+    // Add each alias to `Command` so it shows up in help
+    // Aliases may be defined in terms of other aliases, and it's allowed for them to be defined in any order in the TOML config.
+    // Repeat this algorithm until we no longer make progress.
+    let mut progress;
+    let mut try_again_later = HashMap::new();
+    loop {
+        progress = false;
+        for (alias_name, alias_definition) in aliases.drain() {
+            // Find the innermost subcommand so we can use its help.
+            let mut subcmd = None;
+            for arg in &alias_definition {
+                if let Some(cmd) = subcmd.unwrap_or(&*app).find_subcommand(arg) {
+                    subcmd = Some(cmd);
+                } else {
+                    break;
+                }
+            }
+            let Some(subcmd) = subcmd else {
+                // We never found a valid subcommand. Skip this iteration; maybe it's an alias to another alias.
+                try_again_later.insert(alias_name, alias_definition);
+                continue;
+            };
+            progress = true;
+            let help = render_definition(&alias_definition);
+            let subcmd_alias = subcmd
+                .clone()
+                .name(alias_name)
+                // if we leave the built-in aliases, clap doesn't know whether to call the user alias or
+                // the built-in command
+                .alias(None)
+                .before_help(&help)
+                .before_long_help(help);
+            *app = std::mem::take(app).subcommand(subcmd_alias);
+        }
+        if !progress { break; }
+        aliases.extend(try_again_later.drain());
+    }
+    // We may still have some unhandled aliases. That means there's an alias with valid toml syntax,
+    // but points to a subcommand that doesn't exist. Add help for that too.
+    for (name, definition) in try_again_later {
+        let help = render_definition(&definition);
+        // TODO: we can't distinguish empty/option-only aliases from aliases without a subcommand :(
+        // This leaves the default "global options" help in case it does happen to be valid.
+        // Perhaps we should do `app.try_parse_matches` here so we can distinguish the two?
+        let subcmd = Command::new(name).about(help);
+        *app = std::mem::take(app).subcommand(subcmd);
     }
 }
 
@@ -2713,7 +2779,7 @@ pub fn expand_args(
     app: &Command,
     args_os: ArgsOs,
     config: &config::Config,
-) -> Result<Vec<String>, CommandError> {
+) -> Result<(Vec<String>, AliasMap), CommandError> {
     let mut string_args: Vec<String> = vec![];
     for arg_os in args_os {
         if let Some(string_arg) = arg_os.to_str() {
@@ -2724,7 +2790,7 @@ pub fn expand_args(
     }
 
     let string_args = resolve_default_command(ui, config, app, string_args)?;
-    resolve_aliases(ui, config, app, string_args)
+    expand_aliases(ui, config, app, string_args)
 }
 
 pub fn parse_args(
@@ -2903,7 +2969,7 @@ impl CliRunner {
 
     #[instrument(skip_all)]
     fn run_internal(
-        self,
+        mut self,
         ui: &mut Ui,
         mut layered_configs: LayeredConfigs,
     ) -> Result<(), CommandError> {
@@ -2929,7 +2995,11 @@ impl CliRunner {
         let config = layered_configs.merge();
         ui.reset(&config)?;
 
-        let string_args = expand_args(ui, &self.app, env::args_os(), &config)?;
+        // save this - since `expand_args` modifies app, it won't parse the same when we
+        // process -R
+        let original_app = self.app.clone();
+        let (string_args, aliases) = expand_args(ui, &self.app, env::args_os(), &config)?;
+        add_alias_help(&mut self.app, aliases);
         let (matches, args) = parse_args(
             ui,
             &self.app,
@@ -2959,7 +3029,7 @@ impl CliRunner {
         // If -R is specified, check if the expanded arguments differ. Aliases
         // can also be injected by --config-toml, but that's obviously wrong.
         if args.global_args.repository.is_some() {
-            let new_string_args = expand_args(ui, &self.app, env::args_os(), &config).ok();
+            let new_string_args = expand_args(ui, &original_app, env::args_os(), &config).ok().map(|(args, _)| args);
             if new_string_args.as_ref() != Some(&string_args) {
                 writeln!(
                     ui.warning_default(),
diff --git a/cli/tests/test_alias.rs b/cli/tests/test_alias.rs
@@ -249,17 +249,22 @@ fn test_alias_global_args_in_definition() {
 #[test]
 fn test_alias_invalid_definition() {
     let test_env = TestEnvironment::default();
-
     test_env.add_config(
         r#"[aliases]
     non-list = 5
-    non-string-list = [[]]
     "#,
     );
     let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["non-list"]);
     insta::assert_snapshot!(stderr, @r###"
     Error: Alias definition for "non-list" must be a string list
     "###);
+
+    let test_env = TestEnvironment::default();
+    test_env.add_config(
+        r#"[aliases]
+    non-string-list = [[]]
+    "#,
+    );
     let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["non-string-list"]);
     insta::assert_snapshot!(stderr, @r###"
     Error: Alias definition for "non-string-list" must be a string list
@@ -337,3 +342,88 @@ fn test_alias_in_repo_config() {
     aliases.l=["log", "-r@", "--no-graph", "-T\"user alias\\n\""]
     "###);
 }
+
+#[test]
+fn test_alias_help() {
+    let test_env = TestEnvironment::default();
+    test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]);
+    let repo_path = test_env.env_root().join("repo");
+
+    test_env.add_config(r#"aliases.b = ["log", "-r", "@", "-T", "branches"]"#);
+    test_env.add_config(r#"aliases.s = ["status"]"#);
+    test_env.add_config(r#"aliases.f = ["git", "fetch"]"#); // test nested subcommand
+    test_env.add_config(r#"aliases.b2 = ["b", "--no-graph"]"#); // test recursive subcommand
+    test_env.add_config(r#"aliases.empty = []"#);
+    test_env.add_config(r#"aliases.option-only = ["--no-pager"]"#);
+    test_env.add_config(r#"aliases.bad = ["this-command-does-not-exist"]"#);
+
+    let stdout = test_env.jj_cmd_success(&repo_path, &["help", "b"]);
+    insta::assert_snapshot!(stdout.lines().take(3).join("\n"), @r###"
+    Alias for "log -r @ -T branches"
+
+    Show revision history
+    "###);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["b", "-h"]);
+    insta::assert_snapshot!(stdout.lines().next().unwrap(), @"Show revision history");
+
+    let stdout = test_env.jj_cmd_success(&repo_path, &["help", "s"]);
+    insta::assert_snapshot!(stdout.lines().take(3).join("\n"), @r###"
+    Alias for "status"
+
+    Show high-level repo status
+    "###);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["s", "-h"]);
+    insta::assert_snapshot!(stdout.lines().next().unwrap(), @"Show high-level repo status");
+
+    let stdout = test_env.jj_cmd_success(&repo_path, &["help", "f"]);
+    insta::assert_snapshot!(stdout.lines().take(3).join("\n"), @r###"
+    Alias for "git fetch"
+
+    Fetch from a Git remote
+    "###);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["f", "-h"]);
+    insta::assert_snapshot!(stdout.lines().next().unwrap(), @"Fetch from a Git remote");
+
+    let stdout = test_env.jj_cmd_success(&repo_path, &["help", "b2"]);
+    insta::assert_snapshot!(stdout.lines().take(3).join("\n"), @r###"
+    Alias for "b --no-graph"
+
+    Show revision history
+    "###);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["b2", "-h"]);
+    insta::assert_snapshot!(stdout.lines().next().unwrap(), @"Show revision history");
+
+    let stdout = test_env.jj_cmd_success(&repo_path, &["help", "empty"]);
+    insta::assert_snapshot!(stdout.lines().take(3).join("\n"), @r###"
+    Alias for ""
+
+    Usage: jj empty [OPTIONS]
+    "###);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["empty", "-h"]);
+    insta::assert_snapshot!(stdout.lines().next().unwrap(), @"Jujutsu (An experimental VCS)");
+
+    let stdout = test_env.jj_cmd_success(&repo_path, &["help", "option-only"]);
+    insta::assert_snapshot!(stdout.lines().take(3).join("\n"), @r###"
+    Alias for "--no-pager"
+
+    Usage: jj option-only [OPTIONS]
+    "###);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["option-only", "-h"]);
+    insta::assert_snapshot!(stdout.lines().next().unwrap(), @"Jujutsu (An experimental VCS)");
+
+    let stdout = test_env.jj_cmd_success(&repo_path, &["help", "bad"]);
+    // TODO: this isn't ideal, see the comment in `resolve_aliases`
+    insta::assert_snapshot!(stdout.lines().take(3).join("\n"), @r###"
+    Alias for "this-command-does-not-exist"
+
+    Usage: jj bad [OPTIONS]
+    "###);
+    let stderr = test_env.jj_cmd_cli_error(&repo_path, &["bad", "-h"]);
+    insta::assert_snapshot!(stderr, @r###"
+    error: unrecognized subcommand 'this-command-does-not-exist'
+
+    Usage: jj [OPTIONS] <COMMAND>
+
+    For more information, try '--help'.
+    "###);
+}
