From 430692cff81888263c803cc099903b26bbb69080 Mon Sep 17 00:00:00 2001
From: jyn <github@jyn.dev>
Date: Fri, 9 Feb 2024 17:38:43 -0500
Subject: [PATCH] give a warning when trying to redefine a built-in command

previously, aliases to built-in commands were silently ignored. this matches git's behavior, but seems unhelpful, especially if the user doesn't know that a command with that name already exists.
give a warning rather than silently ignoring it.
---
 CHANGELOG.md            |  2 ++
 cli/src/cli_util.rs     | 12 +++++++++++-
 cli/tests/test_alias.rs |  9 ++++-----
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7eaf1c33b0..fc2ad32ea0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Deprecations
 
+- Attempting to alias a built-in command now gives a warning, rather than being silently ignored.
+
 ### New features
 
 * `jj branch list`/`tag list` now accept `-T`/`--template` option. The tag list
diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs
index 2acb4d4f8d..885b300f28 100644
--- a/cli/src/cli_util.rs
+++ b/cli/src/cli_util.rs
@@ -2600,6 +2600,7 @@ fn resolve_default_command(
 }
 
 fn resolve_aliases(
+    ui: &Ui,
     config: &config::Config,
     app: &Command,
     mut string_args: Vec<String>,
@@ -2616,6 +2617,7 @@ fn resolve_aliases(
             }
         }
     }
+
     let mut resolved_aliases = HashSet::new();
     let mut real_commands = HashSet::new();
     for command in app.get_subcommands() {
@@ -2624,6 +2626,14 @@ fn resolve_aliases(
             real_commands.insert(alias.to_string());
         }
     }
+    for alias in aliases_map.keys() {
+        if real_commands.contains(alias) {
+            writeln!(ui.warning_default(),
+                "Cannot define an alias that overrides the built-in command '{alias}'"
+            )?;
+        }
+    }
+
     loop {
         let app_clone = app.clone().allow_external_subcommands(true);
         let matches = app_clone.try_get_matches_from(&string_args).ok();
@@ -2713,7 +2723,7 @@ pub fn expand_args(
     }
 
     let string_args = resolve_default_command(ui, config, app, string_args)?;
-    resolve_aliases(config, app, string_args)
+    resolve_aliases(ui, config, app, string_args)
 }
 
 pub fn parse_args(
diff --git a/cli/tests/test_alias.rs b/cli/tests/test_alias.rs
index d54b198191..692657f027 100644
--- a/cli/tests/test_alias.rs
+++ b/cli/tests/test_alias.rs
@@ -164,11 +164,10 @@ fn test_alias_cannot_override_builtin() {
     let repo_path = test_env.env_root().join("repo");
 
     test_env.add_config(r#"aliases.log = ["rebase"]"#);
-    // Alias should be ignored
-    let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-r", "root()"]);
-    insta::assert_snapshot!(stdout, @r###"
-    ◉  zzzzzzzz root() 00000000
-    "###);
+    // Alias should give a warning
+    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["log", "-r", "root()"]);
+    insta::assert_snapshot!(stdout, @"◉  zzzzzzzz root() 00000000\n");
+    insta::assert_snapshot!(stderr, @"Warning: Cannot define an alias that overrides the built-in command 'log'\n");
 }
 
 #[test]