Refactor and Optimizations (#459)

* Refactor: Avoid parsing `package.version()` twice in `ops::resolve` and `ops::install`
* Optimize Resolution: Replace `Package<Meta>` with two `CompactStrings`: `name` and `version`
* Use `CompactString` for `BinstallError::CratesIoApi::crate_name`
* Use `CompactString` for `BinstallError::VersionParse::v`
* Use `CompactString` for `BinstallError::VersionReq::req`
* Use `CompactString` for `BinstallError::VersionUnavailable::crate_name`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

crates/binstalk/src/errors.rs

@@ -114,7 +114,7 @@ pub enum BinstallError {
         help("Check that the crate name you provided is correct.\nYou can also search for a matching crate at: https://lib.rs/search?q={crate_name}")
     )]
     CratesIoApi {
-        crate_name: String,
+        crate_name: CompactString,
         #[source]
         err: crates_io_api::Error,
     },
@@ -153,7 +153,7 @@ pub enum BinstallError {
     #[error("version string '{v}' is not semver")]
     #[diagnostic(severity(error), code(binstall::version::parse))]
     VersionParse {
-        v: String,
+        v: CompactString,
         #[source]
         err: semver::Error,
     },
@@ -170,7 +170,7 @@ pub enum BinstallError {
     #[error("version requirement '{req}' is not semver")]
     #[diagnostic(severity(error), code(binstall::version::requirement))]
     VersionReq {
-        req: String,
+        req: CompactString,
         #[source]
         err: semver::Error,
     },
@@ -194,7 +194,7 @@ pub enum BinstallError {
     #[error("no crate information available for '{crate_name}' version '{v}'")]
     #[diagnostic(severity(error), code(binstall::version::unavailable))]
     VersionUnavailable {
-        crate_name: String,
+        crate_name: CompactString,
         v: semver::Version,
     },
 

crates/binstalk/src/ops/install.rs

@@ -1,6 +1,5 @@
 use std::{borrow::Cow, env, ffi::OsStr, sync::Arc};
 
-use cargo_toml::Package;
 use compact_str::CompactString;
 use log::{debug, error, info};
 use tokio::{process::Command, task::block_in_place};
@@ -10,10 +9,7 @@ use crate::{
     bins,
     errors::BinstallError,
     helpers::jobserver_client::LazyJobserverClient,
-    manifests::{
+    manifests::crate_info::{CrateInfo, CrateSource},
-        cargo_toml_binstall::Meta,
-        crate_info::{CrateInfo, CrateSource},
-    },
 };
 
 pub async fn install(
@@ -25,26 +21,18 @@ pub async fn install(
         Resolution::AlreadyUpToDate => Ok(None),
         Resolution::Fetch {
             fetcher,
-            package,
+            new_version,
             name,
             version_req,
             bin_files,
         } => {
-            let current_version =
-                package
-                    .version()
-                    .parse()
-                    .map_err(|err| BinstallError::VersionParse {
-                        v: package.version().to_string(),
-                        err,
-                    })?;
             let target = fetcher.target().into();
 
             install_from_package(opts, bin_files).await.map(|option| {
                 option.map(|bins| CrateInfo {
                     name,
                     version_req,
-                    current_version,
+                    current_version: new_version,
                     source: CrateSource::cratesio_registry(),
                     target,
                     bins,
@@ -52,21 +40,26 @@ pub async fn install(
                 })
             })
         }
-        Resolution::InstallFromSource { package } => {
+        Resolution::InstallFromSource { name, version } => {
             let desired_targets = opts.desired_targets.get().await;
             let target = desired_targets
                 .first()
                 .ok_or(BinstallError::NoViableTargets)?;
 
             if !opts.dry_run {
-                install_from_source(&package, target, jobserver_client, opts.quiet, opts.force)
+                install_from_source(
+                    &name,
+                    &version,
+                    target,
+                    jobserver_client,
+                    opts.quiet,
+                    opts.force,
+                )
                 .await
                 .map(|_| None)
             } else {
                 info!(
-                    "Dry-run: running `cargo install {} --version {} --target {target}`",
+                    "Dry-run: running `cargo install {name} --version {version} --target {target}`",
-                    package.name,
-                    package.version()
                 );
                 Ok(None)
             }
@@ -103,7 +96,8 @@ async fn install_from_package(
 }
 
 async fn install_from_source(
-    package: &Package<Meta>,
+    name: &str,
+    version: &str,
     target: &str,
     lazy_jobserver_client: LazyJobserverClient,
     quiet: bool,
@@ -116,18 +110,16 @@ async fn install_from_source(
         .unwrap_or_else(|| Cow::Borrowed(OsStr::new("cargo")));
 
     debug!(
-        "Running `{} install {} --version {} --target {target}`",
+        "Running `{} install {name} --version {version} --target {target}`",
         cargo.to_string_lossy(),
-        package.name,
-        package.version()
     );
 
     let mut cmd = Command::new(cargo);
 
     cmd.arg("install")
-        .arg(&package.name)
+        .arg(name)
         .arg("--version")
-        .arg(package.version())
+        .arg(version)
         .arg("--target")
         .arg(target);
 

crates/binstalk/src/ops/resolve.rs

@@ -34,13 +34,14 @@ pub use version_ext::VersionReqExt;
 pub enum Resolution {
     Fetch {
         fetcher: Arc<dyn Fetcher>,
-        package: Package<Meta>,
+        new_version: Version,
         name: CompactString,
         version_req: CompactString,
         bin_files: Vec<bins::BinFile>,
     },
     InstallFromSource {
-        package: Package<Meta>,
+        name: CompactString,
+        version: CompactString,
     },
     AlreadyUpToDate,
 }
@@ -154,13 +155,13 @@ async fn resolve_inner(
         .package
         .ok_or_else(|| BinstallError::CargoTomlMissingPackage(crate_name.name.clone()))?;
 
-    if let Some(curr_version) = curr_version {
     let new_version =
         Version::parse(package.version()).map_err(|err| BinstallError::VersionParse {
-                v: package.version().to_string(),
+            v: package.version().to_compact_string(),
             err,
         })?;
 
+    if let Some(curr_version) = curr_version {
         if new_version == curr_version {
             info!(
                 "{} v{curr_version} is already installed, use --force to override",
@@ -246,7 +247,7 @@ async fn resolve_inner(
                         if !bin_files.is_empty() {
                             return Ok(Resolution::Fetch {
                                 fetcher,
-                                package,
+                                new_version,
                                 name: crate_name.name,
                                 version_req: version_req.to_compact_string(),
                                 bin_files,
@@ -279,7 +280,10 @@ async fn resolve_inner(
         }
     }
 
-    Ok(Resolution::InstallFromSource { package })
+    Ok(Resolution::InstallFromSource {
+        name: crate_name.name,
+        version: package.version().to_compact_string(),
+    })
 }
 
 ///  * `fetcher` - `fetcher.find()` must return `Ok(true)`.