From d49910fd210b1732498d994abecbe6b12d8db3bb Mon Sep 17 00:00:00 2001
From: Aleksey Kladov <aleksey.kladov@gmail.com>
Date: Thu, 3 May 2018 13:50:05 +0300
Subject: [PATCH] Make alias handling more straightforward

Previously, `execute_subcommand` was called recursively, and each call
would `.configure` the `config` again. It worked, but seemed rather
fragile.

This commit handles aliases more explicitly, ensures that `.configure`
is called once and, as a bonus, adds a warning for when an alias is
shadowed by the built in.
---
 src/bin/cli.rs                        | 47 ++++++++++++++++++++-------
 tests/testsuite/cargo_alias_config.rs | 43 +++++++++++++++++++++---
 2 files changed, 74 insertions(+), 16 deletions(-)

diff --git a/src/bin/cli.rs b/src/bin/cli.rs
index dd33ce538..9fbd12cbc 100644
--- a/src/bin/cli.rs
+++ b/src/bin/cli.rs
@@ -68,9 +68,43 @@ Run with 'cargo -Z [FLAG] [SUBCOMMAND]'"
         return Ok(());
     }
 
+    let args = expand_aliases(config, args)?;
+
     execute_subcommand(config, args)
 }
 
+fn expand_aliases(
+    config: &mut Config,
+    args: ArgMatches<'static>,
+) -> Result<ArgMatches<'static>, CliError> {
+    if let (cmd, Some(args)) = args.subcommand() {
+        match (
+            commands::builtin_exec(cmd),
+            super::aliased_command(config, cmd)?,
+        ) {
+            (None, Some(mut alias)) => {
+                alias.extend(
+                    args.values_of("")
+                        .unwrap_or_default()
+                        .map(|s| s.to_string()),
+                );
+                let args = cli()
+                    .setting(AppSettings::NoBinaryName)
+                    .get_matches_from_safe(alias)?;
+                return expand_aliases(config, args);
+            }
+            (Some(_), Some(_)) => {
+                config.shell().warn(format!(
+                    "alias `{}` is ignored, because it is shadowed by a built in command",
+                    cmd
+                ))?;
+            }
+            (_, None) => {}
+        }
+    };
+    Ok(args)
+}
+
 fn execute_subcommand(config: &mut Config, args: ArgMatches) -> CliResult {
     let (cmd, subcommand_args) = match args.subcommand() {
         (cmd, Some(args)) => (cmd, args),
@@ -79,7 +113,7 @@ fn execute_subcommand(config: &mut Config, args: ArgMatches) -> CliResult {
             return Ok(());
         }
     };
-    
+
     let arg_target_dir = &subcommand_args.value_of_path("target-dir", config);
 
     config.configure(
@@ -101,17 +135,6 @@ fn execute_subcommand(config: &mut Config, args: ArgMatches) -> CliResult {
         return exec(config, subcommand_args);
     }
 
-    if let Some(mut alias) = super::aliased_command(config, cmd)? {
-        alias.extend(
-            subcommand_args.values_of("")
-                           .unwrap_or_default()
-                           .map(|s| s.to_string()),
-        );
-        let subcommand_args = cli()
-            .setting(AppSettings::NoBinaryName)
-            .get_matches_from_safe(alias)?;
-        return execute_subcommand(config, subcommand_args);
-    }
     let mut ext_args: Vec<&str> = vec![cmd];
     ext_args.extend(subcommand_args.values_of("").unwrap_or_default());
     super::execute_external_subcommand(config, cmd, &ext_args)
diff --git a/tests/testsuite/cargo_alias_config.rs b/tests/testsuite/cargo_alias_config.rs
index a9e3df703..3eb1a0bf6 100644
--- a/tests/testsuite/cargo_alias_config.rs
+++ b/tests/testsuite/cargo_alias_config.rs
@@ -23,8 +23,8 @@ fn alias_incorrect_config_type() {
     assert_that(
         p.cargo("b-cargo-test").arg("-v"),
         execs().with_status(101).with_stderr_contains(
-            "[ERROR] invalid configuration \
-for key `alias.b-cargo-test`
+            "\
+[ERROR] invalid configuration for key `alias.b-cargo-test`
 expected a list, but found a integer for [..]",
         ),
     );
@@ -77,9 +77,43 @@ fn alias_config() {
         .build();
 
     assert_that(
-        p.cargo("b-cargo-test").arg("-v"),
+        p.cargo("b-cargo-test -v"),
+        execs()
+            .with_status(0)
+            .with_stderr_contains(
+                "\
+[COMPILING] foo v0.5.0 [..]
+[RUNNING] `rustc --crate-name foo [..]",
+            )
+            .stream(),
+    );
+}
+
+#[test]
+fn recursive_alias() {
+    let p = project("foo")
+        .file("Cargo.toml", &basic_bin_manifest("foo"))
+        .file(
+            "src/main.rs",
+            r#"
+            fn main() {
+        }"#,
+        )
+        .file(
+            ".cargo/config",
+            r#"
+            [alias]
+            b-cargo-test = "build"
+            a-cargo-test = ["b-cargo-test", "-v"]
+        "#,
+        )
+        .build();
+
+    assert_that(
+        p.cargo("a-cargo-test"),
         execs().with_status(0).with_stderr_contains(
-            "[COMPILING] foo v0.5.0 [..]
+            "\
+[COMPILING] foo v0.5.0 [..]
 [RUNNING] `rustc --crate-name foo [..]",
         ),
     );
@@ -164,6 +198,7 @@ fn cant_shadow_builtin() {
         p.cargo("build"),
         execs().with_status(0).with_stderr(
             "\
+[WARNING] alias `build` is ignored, because it is shadowed by a built in command
 [COMPILING] foo v0.5.0 ([..])
 [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
 ",
-- 
2.30.2