From 0279e8e63a79c124357f29a86e13fb6399e0a59b Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 26 Jan 2020 15:02:37 -0800 Subject: [PATCH] Fix using global options before an alias. --- crates/resolver-tests/tests/resolve.rs | 4 +- src/bin/cargo/cli.rs | 97 ++++++++++++++++++++------ src/cargo/util/config/mod.rs | 22 +++--- tests/testsuite/cargo_alias_config.rs | 16 +++++ tests/testsuite/config.rs | 5 +- 5 files changed, 104 insertions(+), 40 deletions(-) diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index 93e1ff4a1a9..2d2ac0fe32f 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -61,7 +61,7 @@ proptest! { config .configure( 1, - None, + false, None, false, false, @@ -569,7 +569,7 @@ fn test_resolving_minimum_version_with_transitive_deps() { config .configure( 1, - None, + false, None, false, false, diff --git a/src/bin/cargo/cli.rs b/src/bin/cargo/cli.rs index c30eeb40508..8646dfaf0e5 100644 --- a/src/bin/cargo/cli.rs +++ b/src/bin/cargo/cli.rs @@ -92,8 +92,11 @@ Run with 'cargo -Z [FLAG] [SUBCOMMAND]'" return Ok(()); } - let args = expand_aliases(config, args)?; - let (cmd, subcommand_args) = match args.subcommand() { + // Global args need to be extracted before expanding aliases because the + // clap code for extracting a subcommand discards global options + // (appearing before the subcommand). + let (expanded_args, global_args) = expand_aliases(config, args)?; + let (cmd, subcommand_args) = match expanded_args.subcommand() { (cmd, Some(args)) => (cmd, args), _ => { // No subcommand provided. @@ -101,7 +104,7 @@ Run with 'cargo -Z [FLAG] [SUBCOMMAND]'" return Ok(()); } }; - config_configure(config, &args, subcommand_args)?; + config_configure(config, &expanded_args, subcommand_args, global_args)?; super::init_git_transports(config); execute_subcommand(config, cmd, subcommand_args) @@ -129,7 +132,7 @@ pub fn get_version_string(is_verbose: bool) -> String { fn expand_aliases( config: &mut Config, args: ArgMatches<'static>, -) -> Result, CliError> { +) -> Result<(ArgMatches<'static>, GlobalArgs), CliError> { if let (cmd, Some(args)) = args.subcommand() { match ( commands::builtin_exec(cmd), @@ -148,41 +151,60 @@ fn expand_aliases( .unwrap_or_default() .map(|s| s.to_string()), ); - let args = cli() + // new_args strips out everything before the subcommand, so + // capture those global options now. + // Note that an alias to an external command will not receive + // these arguments. That may be confusing, but such is life. + let global_args = GlobalArgs::new(&args); + let new_args = cli() .setting(AppSettings::NoBinaryName) .get_matches_from_safe(alias)?; - return expand_aliases(config, args); + let (expanded_args, _) = expand_aliases(config, new_args)?; + return Ok((expanded_args, global_args)); } (_, None) => {} } }; - Ok(args) + Ok((args, GlobalArgs::default())) } fn config_configure( config: &mut Config, args: &ArgMatches<'_>, subcommand_args: &ArgMatches<'_>, + global_args: GlobalArgs, ) -> CliResult { let arg_target_dir = &subcommand_args.value_of_path("target-dir", config); - let config_args: Vec<&str> = args.values_of("config").unwrap_or_default().collect(); - let quiet = if args.is_present("quiet") || subcommand_args.is_present("quiet") { - Some(true) - } else { - None - }; + let verbose = global_args.verbose + args.occurrences_of("verbose") as u32; + // quiet is unusual because it is redefined in some subcommands in order + // to provide custom help text. + let quiet = + args.is_present("quiet") || subcommand_args.is_present("quiet") || global_args.quiet; + let global_color = global_args.color; // Extract so it can take reference. + let color = args + .value_of("color") + .or_else(|| global_color.as_ref().map(|s| s.as_ref())); + let frozen = args.is_present("frozen") || global_args.frozen; + let locked = args.is_present("locked") || global_args.locked; + let offline = args.is_present("offline") || global_args.offline; + let mut unstable_flags = global_args.unstable_flags; + if let Some(values) = args.values_of("unstable-features") { + unstable_flags.extend(values.map(|s| s.to_string())); + } + let mut config_args = global_args.config_args; + if let Some(values) = args.values_of("config") { + config_args.extend(values.map(|s| s.to_string())); + } config.configure( - args.occurrences_of("verbose") as u32, + verbose, quiet, - args.value_of("color"), - args.is_present("frozen"), - args.is_present("locked"), - args.is_present("offline"), + color, + frozen, + locked, + offline, arg_target_dir, - &args - .values_of_lossy("unstable-features") - .unwrap_or_default(), + &unstable_flags, &config_args, )?; Ok(()) @@ -202,6 +224,39 @@ fn execute_subcommand( super::execute_external_subcommand(config, cmd, &ext_args) } +#[derive(Default)] +struct GlobalArgs { + verbose: u32, + quiet: bool, + color: Option, + frozen: bool, + locked: bool, + offline: bool, + unstable_flags: Vec, + config_args: Vec, +} + +impl GlobalArgs { + fn new(args: &ArgMatches<'_>) -> GlobalArgs { + GlobalArgs { + verbose: args.occurrences_of("verbose") as u32, + quiet: args.is_present("quiet"), + color: args.value_of("color").map(|s| s.to_string()), + frozen: args.is_present("frozen"), + locked: args.is_present("locked"), + offline: args.is_present("offline"), + unstable_flags: args + .values_of_lossy("unstable-features") + .unwrap_or_default(), + config_args: args + .values_of("config") + .unwrap_or_default() + .map(|s| s.to_string()) + .collect(), + } + } +} + fn cli() -> App { App::new("cargo") .settings(&[ diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 105aea2a2c7..97d012c4df9 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -602,14 +602,14 @@ impl Config { pub fn configure( &mut self, verbose: u32, - quiet: Option, + quiet: bool, color: Option<&str>, frozen: bool, locked: bool, offline: bool, target_dir: &Option, unstable_flags: &[String], - cli_config: &[&str], + cli_config: &[String], ) -> CargoResult<()> { self.unstable_flags.parse(unstable_flags)?; if !cli_config.is_empty() { @@ -618,7 +618,7 @@ impl Config { self.merge_cli_args()?; } let extra_verbose = verbose >= 2; - let verbose = if verbose == 0 { None } else { Some(true) }; + let verbose = verbose != 0; #[derive(Deserialize, Default)] struct TermConfig { @@ -632,25 +632,19 @@ impl Config { let color = color.or_else(|| term.color.as_ref().map(|s| s.as_ref())); let verbosity = match (verbose, term.verbose, quiet) { - (Some(true), _, None) | (None, Some(true), None) => Verbosity::Verbose, + (true, _, false) | (_, Some(true), false) => Verbosity::Verbose, // Command line takes precedence over configuration, so ignore the // configuration.. - (None, _, Some(true)) => Verbosity::Quiet, + (false, _, true) => Verbosity::Quiet, // Can't pass both at the same time on the command line regardless // of configuration. - (Some(true), _, Some(true)) => { + (true, _, true) => { bail!("cannot set both --verbose and --quiet"); } - // Can't actually get `Some(false)` as a value from the command - // line, so just ignore them here to appease exhaustiveness checking - // in match statements. - (Some(false), _, _) - | (_, _, Some(false)) - | (None, Some(false), None) - | (None, None, None) => Verbosity::Normal, + (false, _, false) => Verbosity::Normal, }; let cli_target_dir = match target_dir.as_ref() { @@ -659,7 +653,7 @@ impl Config { }; self.shell().set_verbosity(verbosity); - self.shell().set_color_choice(color.map(|s| &s[..]))?; + self.shell().set_color_choice(color)?; self.extra_verbose = extra_verbose; self.frozen = frozen; self.locked = locked; diff --git a/tests/testsuite/cargo_alias_config.rs b/tests/testsuite/cargo_alias_config.rs index a5a83c0de98..ffa8c73bf6b 100644 --- a/tests/testsuite/cargo_alias_config.rs +++ b/tests/testsuite/cargo_alias_config.rs @@ -176,3 +176,19 @@ fn builtin_alias_takes_options() { p.cargo("r --example ex1 -- asdf").with_stdout("asdf").run(); } + +#[cargo_test] +fn global_options_with_alias() { + // Check that global options are passed through. + let p = project().file("src/lib.rs", "").build(); + + p.cargo("-v c") + .with_stderr( + "\ +[CHECKING] foo [..] +[RUNNING] `rustc [..] +[FINISHED] dev [..] +", + ) + .run(); +} diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index fdbc9339bca..61d24a881e6 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -76,17 +76,16 @@ impl ConfigBuilder { let homedir = paths::home(); let mut config = Config::new(shell, cwd, homedir); config.set_env(self.env.clone()); - let config_args: Vec<&str> = self.config_args.iter().map(AsRef::as_ref).collect(); config.configure( 0, - None, + false, None, false, false, false, &None, &self.unstable, - &config_args, + &self.config_args, )?; Ok(config) }