refactor(install): Shift crate argument parsing to clap

Author: epage

src/bin/cargo/commands/install.rs

@@ -10,11 +10,7 @@ use cargo_util::paths;
 pub fn cli() -> Command {
     subcommand("install")
         .about("Install a Rust binary. Default location is $HOME/.cargo/bin")
-        .arg(
-            Arg::new("crate")
-                .value_parser(clap::builder::NonEmptyStringValueParser::new())
-                .num_args(0..),
-        )
+        .arg(Arg::new("crate").value_parser(parse_crate).num_args(0..))
         .arg(
             opt("version", "Specify a version to install")
                 .alias("vers")
@@ -104,9 +100,10 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
 
     let version = args.get_one::<String>("version").map(String::as_str);
     let krates = args
-        .get_many::<String>("crate")
+        .get_many::<CrateVersion>("crate")
         .unwrap_or_default()
-        .map(|k| resolve_crate(k, version))
+        .cloned()
+        .map(|(krate, local_version)| resolve_crate(krate, local_version, version))
         .collect::<crate::CargoResult<Vec<_>>>()?;
 
     for (crate_name, _) in krates.iter() {
@@ -190,20 +187,42 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     Ok(())
 }
 
-fn resolve_crate<'k>(
-    mut krate: &'k str,
-    mut version: Option<&'k str>,
-) -> crate::CargoResult<(&'k str, Option<&'k str>)> {
-    if let Some((k, v)) = krate.split_once('@') {
-        if version.is_some() {
-            anyhow::bail!("cannot specify both `@{v}` and `--version`");
-        }
+type CrateVersion = (String, Option<String>);
+
+fn parse_crate(krate: &str) -> crate::CargoResult<CrateVersion> {
+    let (krate, version) = if let Some((k, v)) = krate.split_once('@') {
         if k.is_empty() {
             // by convention, arguments starting with `@` are response files
-            anyhow::bail!("missing crate name for `@{v}`");
+            anyhow::bail!("missing crate name before '@'");
         }
-        krate = k;
-        version = Some(v);
+        let krate = k.to_owned();
+        let version = Some(v.to_owned());
+        (krate, version)
+    } else {
+        let krate = krate.to_owned();
+        let version = None;
+        (krate, version)
+    };
+
+    if krate.is_empty() {
+        anyhow::bail!("crate name is empty");
     }
+
+    Ok((krate, version))
+}
+
+fn resolve_crate(
+    krate: String,
+    local_version: Option<String>,
+    version: Option<&str>,
+) -> crate::CargoResult<CrateVersion> {
+    let version = match (local_version, version) {
+        (Some(l), Some(g)) => {
+            anyhow::bail!("cannot specify both `@{l}` and `--version {g}`");
+        }
+        (Some(l), None) => Some(l),
+        (None, Some(g)) => Some(g.to_owned()),
+        (None, None) => None,
+    };
     Ok((krate, version))
 }

src/cargo/ops/cargo_install.rs

@@ -604,7 +604,7 @@ Consider enabling some of the needed features by passing, e.g., `--features=\"{e
 pub fn install(
     config: &Config,
     root: Option<&str>,
-    krates: Vec<(&str, Option<&str>)>,
+    krates: Vec<(String, Option<String>)>,
     source_id: SourceId,
     from_cwd: bool,
     opts: &ops::CompileOptions,
@@ -617,9 +617,9 @@ pub fn install(
 
     let (installed_anything, scheduled_error) = if krates.len() <= 1 {
         let (krate, vers) = krates
-            .into_iter()
+            .iter()
             .next()
-            .map(|(k, v)| (Some(k), v))
+            .map(|(k, v)| (Some(k.as_str()), v.as_deref()))
             .unwrap_or((None, None));
         let installable_pkg = InstallablePackage::new(
             config, root, map, krate, source_id, from_cwd, vers, opts, force, no_track, true,
@@ -637,18 +637,18 @@ pub fn install(
         let mut did_update = false;
 
         let pkgs_to_install: Vec<_> = krates
-            .into_iter()
+            .iter()
             .filter_map(|(krate, vers)| {
                 let root = root.clone();
                 let map = map.clone();
                 match InstallablePackage::new(
                     config,
                     root,
                     map,
-                    Some(krate),
+                    Some(krate.as_str()),
                     source_id,
                     from_cwd,
-                    vers,
+                    vers.as_deref(),
                     opts,
                     force,
                     no_track,
@@ -660,12 +660,12 @@ pub fn install(
                     }
                     Ok(None) => {
                         // Already installed
-                        succeeded.push(krate);
+                        succeeded.push(krate.as_str());
                         None
                     }
                     Err(e) => {
                         crate::display_error(&e, &mut config.shell());
-                        failed.push(krate);
+                        failed.push(krate.as_str());
                         // We assume an update was performed if we got an error.
                         did_update = true;
                         None

tests/testsuite/install.rs

@@ -1600,8 +1600,13 @@ fn inline_version_without_name() {
     pkg("foo", "0.1.2");
 
     cargo_process("install @0.1.1")
-        .with_status(101)
-        .with_stderr("error: missing crate name for `@0.1.1`")
+        .with_status(1)
+        .with_stderr(
+            "error: invalid value '@0.1.1' for '[crate]...': missing crate name before '@'
+
+For more information, try '--help'.
+",
+        )
         .run();
 }
 
@@ -1612,7 +1617,7 @@ fn inline_and_explicit_version() {
 
     cargo_process("install foo@0.1.1 --version 0.1.1")
         .with_status(101)
-        .with_stderr("error: cannot specify both `@0.1.1` and `--version`")
+        .with_stderr("error: cannot specify both `@0.1.1` and `--version 0.1.1`")
         .run();
 }
 
@@ -1827,7 +1832,7 @@ fn install_empty_argument() {
     cargo_process("install")
         .arg("")
         .with_status(1)
-        .with_stderr_contains("[ERROR] a value is required for '[crate]...' but none was supplied")
+        .with_stderr_contains("[ERROR] invalid value '' for '[crate]...': crate name is empty")
         .run();
 }