From 83b2de3ea2a62a17bb349a808bb44e68157db57a Mon Sep 17 00:00:00 2001
From: Jiahao XU <Jiahao_XU@outlook.com>
Date: Mon, 21 Nov 2022 12:02:58 +1100
Subject: [PATCH] Enforce strict strategies parsing rule: Reject duplicate
 strategy in `--strategies` (#545)

`cargo-binstall` should reject duplicate strategy in `--strategies` as it is illegal input and it cannot be interpreted reasonably.

* Reject duplicate strategy in `--strategies`
* Optimize `compute_resolvers`: Use `Vec::retain`
   instead of `Iterator::filter` plus collecting into `Vec`.
* Optimize `compute_resolvers`: Reject `strategies.len() > Strategy::COUNT`
* Improve err msg for cases where user disabled all strategies
* Simplify `BinstallError::InvalidStrategies`: Takes `&'static str`
   instead of `&'static &'static str` since other variants are larger than 8B.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
---
 crates/bin/src/args.rs        |  1 +
 crates/bin/src/entry.rs       | 53 +++++++++++++++++++----------------
 crates/binstalk/src/errors.rs |  2 +-
 3 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/crates/bin/src/args.rs b/crates/bin/src/args.rs
index 0265c974..84c936ee 100644
--- a/crates/bin/src/args.rs
+++ b/crates/bin/src/args.rs
@@ -298,6 +298,7 @@ impl Default for RateLimit {
 
 /// Strategy for installing the package
 #[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, ValueEnum, EnumCount)]
+#[repr(u8)]
 pub enum Strategy {
     /// Attempt to download official pre-built artifacts using
     /// information provided in `Cargo.toml`.
diff --git a/crates/bin/src/entry.rs b/crates/bin/src/entry.rs
index e41efdbd..5ae562d4 100644
--- a/crates/bin/src/entry.rs
+++ b/crates/bin/src/entry.rs
@@ -185,22 +185,27 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) -
 
 /// Return (resolvers, cargo_install_fallback)
 fn compute_resolvers(
-    input_strategies: Vec<Strategy>,
+    mut strategies: Vec<Strategy>,
     mut disable_strategies: Vec<Strategy>,
 ) -> Result<(Vec<Resolver>, bool), BinstallError> {
-    // Compute strategies
-    let mut strategies = vec![];
+    let dup_strategy_err =
+        BinstallError::InvalidStrategies("--strategies should not contain duplicate strategy");
 
-    // Remove duplicate strategies
-    for strategy in input_strategies {
-        if strategies.len() == Strategy::COUNT {
-            // All variants of Strategy is present in strategies,
-            // there is no need to continue since all the remaining
-            // args.strategies must be present in stratetgies.
-            break;
-        }
-        if !strategies.contains(&strategy) {
-            strategies.push(strategy);
+    if strategies.len() > Strategy::COUNT {
+        // If len of strategies is larger than number of variants of Strategy,
+        // then there must be duplicates by pigeon hole principle.
+        return Err(dup_strategy_err);
+    }
+
+    // Whether specific variant of Strategy is present
+    let mut is_variant_present = [false; Strategy::COUNT];
+
+    for strategy in &strategies {
+        let index = *strategy as u8 as usize;
+        if is_variant_present[index] {
+            return Err(dup_strategy_err);
+        } else {
+            is_variant_present[index] = true;
         }
     }
 
@@ -213,22 +218,22 @@ fn compute_resolvers(
         ];
     }
 
-    let mut strategies: Vec<Strategy> = if !disable_strategies.is_empty() {
+    // Filter out all disabled strategies
+    if !disable_strategies.is_empty() {
         // Since order doesn't matter, we can sort it and remove all duplicates
         // to speedup checking.
         disable_strategies.sort_unstable();
         disable_strategies.dedup();
 
-        strategies
-            .into_iter()
-            .filter(|strategy| !disable_strategies.contains(strategy))
-            .collect()
-    } else {
-        strategies
-    };
+        // disable_strategies.len() <= Strategy::COUNT, of which is faster
+        // to just use [T]::contains rather than [T]::binary_search
+        strategies.retain(|strategy| !disable_strategies.contains(strategy));
 
-    if strategies.is_empty() {
-        return Err(BinstallError::InvalidStrategies(&"No strategy is provided"));
+        if strategies.is_empty() {
+            return Err(BinstallError::InvalidStrategies(
+                "You have disabled all strategies",
+            ));
+        }
     }
 
     let cargo_install_fallback = *strategies.last().unwrap() == Strategy::Compile;
@@ -243,7 +248,7 @@ fn compute_resolvers(
             Strategy::CrateMetaData => Ok(GhCrateMeta::new as Resolver),
             Strategy::QuickInstall => Ok(QuickInstall::new as Resolver),
             Strategy::Compile => Err(BinstallError::InvalidStrategies(
-                &"Compile strategy must be the last one",
+                "Compile strategy must be the last one",
             )),
         })
         .collect::<Result<Vec<_>, BinstallError>>()?;
diff --git a/crates/binstalk/src/errors.rs b/crates/binstalk/src/errors.rs
index 322be894..4fadd1d8 100644
--- a/crates/binstalk/src/errors.rs
+++ b/crates/binstalk/src/errors.rs
@@ -291,7 +291,7 @@ pub enum BinstallError {
     /// - Exit: 93
     #[error("Invalid strategies configured: {0}")]
     #[diagnostic(severity(error), code(binstall::strategies))]
-    InvalidStrategies(&'static &'static str),
+    InvalidStrategies(&'static str),
 
     /// Fallback to `cargo-install` is disabled.
     ///