add support for jj help alias

jyn514 · jyn514 · commit 8c7bfab1caf5 · 2024-02-10T11:05:34.000-05: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.

---

this does have the downside that `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.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,10 +15,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * Attempting to alias a built-in command now gives a hard error, rather than being silently ignored.
 
+* Errors in alias definitions are now reported eagerly. Previously, they would not error unless you used the alias in a jj command.
+
 ### New features
 
 * Templates now support logical operators: `||`, `&&`, `!`
 
+* `jj help` and `jj util completion` now support aliases.
+
 ### Fixed bugs
 
 * On Windows, symlinks in the repo are now materialized as regular files in the
diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs
@@ -2684,7 +2684,7 @@ fn resolve_default_command(
 
 fn resolve_aliases(
     config: &config::Config,
-    app: &Command,
+    app: &mut Command,
     mut string_args: Vec<String>,
 ) -> Result<Vec<String>, CommandError> {
     let mut aliases_map = config.get_table("aliases")?;
@@ -2716,6 +2716,16 @@ fn resolve_aliases(
         }
     }
 
+    let mut resolved_alias_map = HashMap::new();
+    for (alias_name, value) in aliases_map {
+        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 {
         let app_clone = app.clone().allow_external_subcommands(true);
         let matches = app_clone.try_get_matches_from(&string_args).ok();
@@ -2732,28 +2742,47 @@ fn resolve_aliases(
                         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);
+                    resolved_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);
                 }
             }
         }
         // No more alias commands, or hit unknown option
-        return Ok(string_args);
+        break;
     }
+
+    // Add each alias to `Command` so it shows up in help
+    for (alias_name, alias_definition) in resolved_alias_map {
+        // Find the innermost subcommand so we can use its help.
+        let mut subcmd = &*app;
+        for arg in &alias_definition {
+            if let Some(cmd) = subcmd.find_subcommand(arg) {
+                subcmd = cmd;
+            } else {
+                break;
+            }
+        }
+        let help = format!("Alias for {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);
+    }
+
+    Ok(string_args)
 }
 
 /// Parse args that must be interpreted early, e.g. before printing help.
@@ -2788,7 +2817,7 @@ fn handle_early_args(
 
 pub fn expand_args(
     ui: &Ui,
-    app: &Command,
+    app: &mut Command,
     args_os: ArgsOs,
     config: &config::Config,
 ) -> Result<Vec<String>, CommandError> {
@@ -2999,7 +3028,7 @@ impl CliRunner {
 
     #[instrument(skip_all)]
     fn run_internal(
-        self,
+        mut self,
         ui: &mut Ui,
         mut layered_configs: LayeredConfigs,
     ) -> Result<(), CommandError> {
@@ -3021,7 +3050,10 @@ 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 mut original_app = self.app.clone();
+        let string_args = expand_args(ui, &mut self.app, env::args_os(), &config)?;
         let (matches, args) = parse_args(
             ui,
             &self.app,
@@ -3050,7 +3082,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, &mut original_app, env::args_os(), &config).ok();
             if new_string_args.as_ref() != Some(&string_args) {
                 writeln!(
                     ui.warning(),
diff --git a/cli/tests/test_alias.rs b/cli/tests/test_alias.rs
@@ -250,17 +250,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
@@ -338,3 +343,31 @@ 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"]"#);
+
+    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 commit history
+    "###);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["b", "-h"]);
+    insta::assert_snapshot!(stdout.lines().next().unwrap(), @"Show commit 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");
+}
