Auto merge of #7837 - ehuss:fix-global-opt-alias, r=alexcrichton

Fix using global options before an alias.

Options before an alias were being ignored (like `cargo -v b`).  The solution is to extract those global options before expanding an alias, and then merging it later.

An alternative to this is to try to avoid discarding the options during expansion, but I couldn't figure out a way to get the position of the subcommand in the argument list.  Clap only provides a way to get the arguments *following* the subcommand.

I also cleaned up some of the code in `Config::configure`, which was carrying some weird baggage from previous refactorings.

Fixes #7834

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,

src/bin/cargo/cli.rs

@@ -91,16 +91,19 @@ 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.
             cli().print_help()?;
             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)
@@ -128,7 +131,7 @@ pub fn get_version_string(is_verbose: bool) -> String {
 fn expand_aliases(
     config: &mut Config,
     args: ArgMatches<'static>,
-) -> Result<ArgMatches<'static>, CliError> {
+) -> Result<(ArgMatches<'static>, GlobalArgs), CliError> {
     if let (cmd, Some(args)) = args.subcommand() {
         match (
             commands::builtin_exec(cmd),
@@ -147,41 +150,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(())
@@ -201,6 +223,39 @@ fn execute_subcommand(
     super::execute_external_subcommand(config, cmd, &ext_args)
 }
 
+#[derive(Default)]
+struct GlobalArgs {
+    verbose: u32,
+    quiet: bool,
+    color: Option<String>,
+    frozen: bool,
+    locked: bool,
+    offline: bool,
+    unstable_flags: Vec<String>,
+    config_args: Vec<String>,
+}
+
+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(&[

src/cargo/util/config/mod.rs

@@ -602,14 +602,14 @@ impl Config {
     pub fn configure(
         &mut self,
         verbose: u32,
-        quiet: Option<bool>,
+        quiet: bool,
         color: Option<&str>,
         frozen: bool,
         locked: bool,
         offline: bool,
         target_dir: &Option<PathBuf>,
         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;

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();
+}

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)
     }