From 7084570f2045913863d208066e3374d8fd8dfe46 Mon Sep 17 00:00:00 2001 From: Sean Stangl Date: Sat, 11 Dec 2021 15:09:10 -0700 Subject: [PATCH 1/6] Display alias target on 'cargo help `. Closes #10138 Previously, `cargo help ` resolved the alias and displayed the help for the targeted subcommand. For example, if `br` were aliased to `build --release`, `cargo help br` would display the manpage for cargo-build. With this patch, it will print "'br' is aliased to 'build --release'". --- src/bin/cargo/commands/help.rs | 42 ++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/src/bin/cargo/commands/help.rs b/src/bin/cargo/commands/help.rs index 597c2c5883b..215fef33808 100644 --- a/src/bin/cargo/commands/help.rs +++ b/src/bin/cargo/commands/help.rs @@ -1,6 +1,6 @@ use crate::aliased_command; use cargo::util::errors::CargoResult; -use cargo::Config; +use cargo::{drop_println, Config}; use cargo_util::paths::resolve_executable; use flate2::read::GzDecoder; use std::ffi::OsString; @@ -46,8 +46,16 @@ fn try_help(config: &Config) -> CargoResult { Some(s) => s, None => return Ok(false), }; - // Check if this is a built-in command (or alias); - let subcommand = match check_alias(config, subcommand) { + + // Check if this is an alias. If so, just display the alias information. + if let Some(argv) = check_alias(config, subcommand) { + let alias = argv.join(" "); + drop_println!(config, "'{}' is aliased to '{}'", subcommand, alias); + return Ok(true); + } + + // If not an alias, this should be a built-in subcommand. + let subcommand = match check_builtin(subcommand) { Some(s) => s, None => return Ok(false), }; @@ -73,26 +81,26 @@ fn try_help(config: &Config) -> CargoResult { Ok(true) } -/// Checks if the given subcommand is a built-in command (possibly via an alias). +/// Checks if the given subcommand is an alias. /// -/// Returns None if it is not a built-in command. -fn check_alias(config: &Config, subcommand: &str) -> Option { - if super::builtin_exec(subcommand).is_some() { - return Some(subcommand.to_string()); - } +/// Returns None if it is not an alias. +fn check_alias(config: &Config, subcommand: &str) -> Option> { match aliased_command(config, subcommand) { - Ok(Some(alias)) => { - let alias = alias.into_iter().next()?; - if super::builtin_exec(&alias).is_some() { - Some(alias) - } else { - None - } - } + Ok(Some(alias)) => Some(alias), _ => None, } } +/// Checks if the given subcommand is a built-in command (not via an alias). +/// +/// Returns None if it is not a built-in command. +fn check_builtin(subcommand: &str) -> Option { + match super::builtin_exec(subcommand) { + Some(_) => Some(subcommand.to_string()), + None => None, + } +} + /// Extracts the given man page from the compressed archive. /// /// Returns None if the command wasn't found. From e77b254923831db33da09b6d5d458df3382edfca Mon Sep 17 00:00:00 2001 From: Sean Stangl Date: Sat, 11 Dec 2021 15:31:48 -0700 Subject: [PATCH 2/6] Update help::help_alias() to assert the new behavior --- tests/testsuite/help.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/tests/testsuite/help.rs b/tests/testsuite/help.rs index ed91c5fcf27..0871d7f4b73 100644 --- a/tests/testsuite/help.rs +++ b/tests/testsuite/help.rs @@ -110,6 +110,20 @@ fn help_with_man_and_path( assert_eq!(stdout, contents); } +fn help_with_stdout_and_path(subcommand: &str, path: &Path) -> String { + let output = process(&cargo_exe()) + .arg("help") + .arg(subcommand) + .env("PATH", path) + .exec_with_output() + .unwrap(); + assert!(output.status.success()); + let stderr = from_utf8(&output.stderr).unwrap(); + assert_eq!(stderr, ""); + let stdout = from_utf8(&output.stdout).unwrap(); + stdout.to_string() +} + #[cargo_test] fn help_man() { // Checks that `help command` displays the man page using the given command. @@ -124,7 +138,8 @@ fn help_man() { #[cargo_test] fn help_alias() { // Check that `help some_alias` will resolve. - help_with_man_and_path("", "b", "build", Path::new("")); + let out = help_with_stdout_and_path("b", Path::new("")); + assert_eq!(out, "'b' is aliased to 'build'\n"); let config = paths::root().join(".cargo/config"); fs::create_dir_all(config.parent().unwrap()).unwrap(); @@ -136,7 +151,8 @@ fn help_alias() { "#, ) .unwrap(); - help_with_man_and_path("", "my-alias", "build", Path::new("")); + let out = help_with_stdout_and_path("my-alias", Path::new("")); + assert_eq!(out, "'my-alias' is aliased to 'build --release'\n"); } #[cargo_test] From dc0bc58445293eb549387706f784492a662c018a Mon Sep 17 00:00:00 2001 From: Sean Stangl Date: Sat, 11 Dec 2021 20:55:38 -0700 Subject: [PATCH 3/6] Update src/bin/cargo/commands/help.rs Co-authored-by: Weihang Lo --- src/bin/cargo/commands/help.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/bin/cargo/commands/help.rs b/src/bin/cargo/commands/help.rs index 215fef33808..30abc93aebc 100644 --- a/src/bin/cargo/commands/help.rs +++ b/src/bin/cargo/commands/help.rs @@ -94,11 +94,8 @@ fn check_alias(config: &Config, subcommand: &str) -> Option> { /// Checks if the given subcommand is a built-in command (not via an alias). /// /// Returns None if it is not a built-in command. -fn check_builtin(subcommand: &str) -> Option { - match super::builtin_exec(subcommand) { - Some(_) => Some(subcommand.to_string()), - None => None, - } +fn check_builtin(subcommand: &str) -> Option<&str> { + super::builtin_exec(subcommand).map(|_| subcommand) } /// Extracts the given man page from the compressed archive. From 8a1af7014ac3964f1e8c1743c67990081c0540ae Mon Sep 17 00:00:00 2001 From: Sean Stangl Date: Sat, 11 Dec 2021 20:58:46 -0700 Subject: [PATCH 4/6] Simplify check_alias() using flatten() Co-authored-by: Weihang Lo --- src/bin/cargo/commands/help.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/bin/cargo/commands/help.rs b/src/bin/cargo/commands/help.rs index 30abc93aebc..cd4e686a33d 100644 --- a/src/bin/cargo/commands/help.rs +++ b/src/bin/cargo/commands/help.rs @@ -85,10 +85,7 @@ fn try_help(config: &Config) -> CargoResult { /// /// Returns None if it is not an alias. fn check_alias(config: &Config, subcommand: &str) -> Option> { - match aliased_command(config, subcommand) { - Ok(Some(alias)) => Some(alias), - _ => None, - } + aliased_command(config, subcommand).ok().flatten() } /// Checks if the given subcommand is a built-in command (not via an alias). From 6df4b1ef71f55a668c523d9ed84b268aede315cc Mon Sep 17 00:00:00 2001 From: Sean Stangl Date: Sun, 12 Dec 2021 10:00:23 -0700 Subject: [PATCH 5/6] Update behavior to pass-through simple aliases This changes the behavior so that simple aliases that directly alias a subcommand (with no arguments) pass-through to that subcommand, while complex aliases (with arguments) show the alias. So for example, `cargo help b` will show the manpage for `cargo-build`, while `cargo help my-alias`, aliased to `build --release`, will show "`my-alias` is aliased to `build --release`". --- src/bin/cargo/commands/help.rs | 27 ++++++++++++++++----------- tests/testsuite/help.rs | 15 ++++++++++----- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/bin/cargo/commands/help.rs b/src/bin/cargo/commands/help.rs index cd4e686a33d..1adc129db96 100644 --- a/src/bin/cargo/commands/help.rs +++ b/src/bin/cargo/commands/help.rs @@ -15,14 +15,14 @@ const COMPRESSED_MAN: &[u8] = include_bytes!(concat!(env!("OUT_DIR"), "/man.tgz" /// This runs before clap processing, because it needs to intercept the `help` /// command if a man page is available. /// -/// Returns `true` if a man page was displayed. In this case, Cargo should -/// exit. +/// Returns `true` if help information was successfully displayed to the user. +/// In this case, Cargo should exit. pub fn handle_embedded_help(config: &Config) -> bool { match try_help(config) { Ok(true) => true, Ok(false) => false, Err(e) => { - log::warn!("man failed: {:?}", e); + log::warn!("help failed: {:?}", e); false } } @@ -47,18 +47,23 @@ fn try_help(config: &Config) -> CargoResult { None => return Ok(false), }; - // Check if this is an alias. If so, just display the alias information. - if let Some(argv) = check_alias(config, subcommand) { - let alias = argv.join(" "); - drop_println!(config, "'{}' is aliased to '{}'", subcommand, alias); - return Ok(true); - } + let subcommand = match check_alias(config, subcommand) { + // If this alias is more than a simple subcommand pass-through, show the alias. + Some(argv) if argv.len() > 1 => { + let alias = argv.join(" "); + drop_println!(config, "`{}` is aliased to `{}`", subcommand, alias); + return Ok(true); + } + // Otherwise, resolve the alias into its subcommand. + Some(argv) => argv[0].clone(), + None => subcommand.to_string(), + }; - // If not an alias, this should be a built-in subcommand. - let subcommand = match check_builtin(subcommand) { + let subcommand = match check_builtin(&subcommand) { Some(s) => s, None => return Ok(false), }; + if resolve_executable(Path::new("man")).is_ok() { let man = match extract_man(&subcommand, "1") { Some(man) => man, diff --git a/tests/testsuite/help.rs b/tests/testsuite/help.rs index 0871d7f4b73..7cad8ba25ae 100644 --- a/tests/testsuite/help.rs +++ b/tests/testsuite/help.rs @@ -138,8 +138,7 @@ fn help_man() { #[cargo_test] fn help_alias() { // Check that `help some_alias` will resolve. - let out = help_with_stdout_and_path("b", Path::new("")); - assert_eq!(out, "'b' is aliased to 'build'\n"); + help_with_man_and_path("", "b", "build", Path::new("")); let config = paths::root().join(".cargo/config"); fs::create_dir_all(config.parent().unwrap()).unwrap(); @@ -147,12 +146,18 @@ fn help_alias() { config, r#" [alias] - my-alias = ["build", "--release"] + simple-alias = ["build"] + complex-alias = ["build", "--release"] "#, ) .unwrap(); - let out = help_with_stdout_and_path("my-alias", Path::new("")); - assert_eq!(out, "'my-alias' is aliased to 'build --release'\n"); + + // Because `simple-alias` aliases a subcommand with no arguments, help shows the manpage. + help_with_man_and_path("", "simple-alias", "build", Path::new("")); + + // Help for `complex-alias` displays the full alias command. + let out = help_with_stdout_and_path("complex-alias", Path::new("")); + assert_eq!(out, "`complex-alias` is aliased to `build --release`\n"); } #[cargo_test] From 4c66d183611d9104fecb79da14e2dce46869e286 Mon Sep 17 00:00:00 2001 From: Sean Stangl Date: Sun, 12 Dec 2021 13:29:31 -0700 Subject: [PATCH 6/6] fix panic if an alias is defined to "" --- crates/cargo-test-support/src/lib.rs | 9 +++++++++ src/bin/cargo/commands/help.rs | 6 +++++- tests/testsuite/help.rs | 9 ++++++++- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index a4c4153bad2..5280ea0efd2 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -803,6 +803,15 @@ impl Execs { } } + #[track_caller] + pub fn run_expect_error(&mut self) { + self.ran = true; + let p = (&self.process_builder).clone().unwrap(); + if self.match_process(&p).is_ok() { + panic!("test was expected to fail, but succeeded running {}", p); + } + } + /// Runs the process, checks the expected output, and returns the first /// JSON object on stdout. #[track_caller] diff --git a/src/bin/cargo/commands/help.rs b/src/bin/cargo/commands/help.rs index 1adc129db96..caaa0c5a496 100644 --- a/src/bin/cargo/commands/help.rs +++ b/src/bin/cargo/commands/help.rs @@ -55,7 +55,11 @@ fn try_help(config: &Config) -> CargoResult { return Ok(true); } // Otherwise, resolve the alias into its subcommand. - Some(argv) => argv[0].clone(), + Some(argv) => { + // An alias with an empty argv can be created via `"empty-alias" = ""`. + let first = argv.get(0).map(String::as_str).unwrap_or(subcommand); + first.to_string() + } None => subcommand.to_string(), }; diff --git a/tests/testsuite/help.rs b/tests/testsuite/help.rs index 7cad8ba25ae..fdb527e761a 100644 --- a/tests/testsuite/help.rs +++ b/tests/testsuite/help.rs @@ -146,12 +146,19 @@ fn help_alias() { config, r#" [alias] - simple-alias = ["build"] + empty-alias = "" + simple-alias = "build" complex-alias = ["build", "--release"] "#, ) .unwrap(); + // The `empty-alias` returns an error. + cargo_process("help empty-alias") + .env("PATH", Path::new("")) + .with_stderr_contains("[..]The subcommand 'empty-alias' wasn't recognized[..]") + .run_expect_error(); + // Because `simple-alias` aliases a subcommand with no arguments, help shows the manpage. help_with_man_and_path("", "simple-alias", "build", Path::new(""));