From 5dcb6873051734fcce18d91c2cc89edace2abb71 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kh=E1=BA=A3i?= <hvksmr1996@gmail.com>
Date: Tue, 28 Nov 2023 07:12:49 +0700
Subject: [PATCH] refactor: use utf8 strings as mem cache keys (#222)

* fix: clippy

* refactor: use utf8 strings as mem cache keys

* refactor: use cleaned_entry_path directly
---
 .../package-manager/src/create_cas_files.rs   |  3 +--
 .../src/create_virtual_dir_by_snapshot.rs     |  3 +--
 crates/tarball/src/lib.rs                     | 27 +++++++++----------
 3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/crates/package-manager/src/create_cas_files.rs b/crates/package-manager/src/create_cas_files.rs
index 72fb2ba60..fef6095e2 100644
--- a/crates/package-manager/src/create_cas_files.rs
+++ b/crates/package-manager/src/create_cas_files.rs
@@ -5,7 +5,6 @@ use pacquet_npmrc::PackageImportMethod;
 use rayon::prelude::*;
 use std::{
     collections::HashMap,
-    ffi::OsString,
     path::{Path, PathBuf},
 };
 
@@ -22,7 +21,7 @@ pub enum CreateCasFilesError {
 pub fn create_cas_files(
     import_method: PackageImportMethod,
     dir_path: &Path,
-    cas_paths: &HashMap<OsString, PathBuf>,
+    cas_paths: &HashMap<String, PathBuf>,
 ) -> Result<(), CreateCasFilesError> {
     assert_eq!(
         import_method,
diff --git a/crates/package-manager/src/create_virtual_dir_by_snapshot.rs b/crates/package-manager/src/create_virtual_dir_by_snapshot.rs
index d61077645..4eb02f97f 100644
--- a/crates/package-manager/src/create_virtual_dir_by_snapshot.rs
+++ b/crates/package-manager/src/create_virtual_dir_by_snapshot.rs
@@ -5,7 +5,6 @@ use pacquet_lockfile::{DependencyPath, PackageSnapshot};
 use pacquet_npmrc::PackageImportMethod;
 use std::{
     collections::HashMap,
-    ffi::OsString,
     fs, io,
     path::{Path, PathBuf},
 };
@@ -14,7 +13,7 @@ use std::{
 #[must_use]
 pub struct CreateVirtualDirBySnapshot<'a> {
     pub virtual_store_dir: &'a Path,
-    pub cas_paths: &'a HashMap<OsString, PathBuf>,
+    pub cas_paths: &'a HashMap<String, PathBuf>,
     pub import_method: PackageImportMethod,
     pub dependency_path: &'a DependencyPath,
     pub package_snapshot: &'a PackageSnapshot,
diff --git a/crates/tarball/src/lib.rs b/crates/tarball/src/lib.rs
index 0a549db63..d312971df 100644
--- a/crates/tarball/src/lib.rs
+++ b/crates/tarball/src/lib.rs
@@ -1,6 +1,5 @@
 use std::{
     collections::HashMap,
-    ffi::OsString,
     io::{Cursor, Read},
     path::PathBuf,
     sync::Arc,
@@ -77,7 +76,7 @@ pub enum CacheValue {
     /// The package is being processed.
     InProgress(Arc<Notify>),
     /// The package is saved.
-    Available(Arc<HashMap<OsString, PathBuf>>),
+    Available(Arc<HashMap<String, PathBuf>>),
 }
 
 /// Internal in-memory cache of tarballs.
@@ -115,7 +114,7 @@ impl<'a> DownloadTarballToStore<'a> {
     pub async fn run_with_mem_cache(
         self,
         mem_cache: &'a MemCache,
-    ) -> Result<Arc<HashMap<OsString, PathBuf>>, TarballError> {
+    ) -> Result<Arc<HashMap<String, PathBuf>>, TarballError> {
         let &DownloadTarballToStore { package_url, .. } = &self;
 
         // QUESTION: I see no copying from existing store_dir, is there such mechanism?
@@ -154,7 +153,7 @@ impl<'a> DownloadTarballToStore<'a> {
     }
 
     /// Execute the subroutine without an in-memory cache.
-    pub async fn run_without_mem_cache(&self) -> Result<HashMap<OsString, PathBuf>, TarballError> {
+    pub async fn run_without_mem_cache(&self) -> Result<HashMap<String, PathBuf>, TarballError> {
         let &DownloadTarballToStore {
             http_client,
             store_dir,
@@ -208,7 +207,7 @@ impl<'a> DownloadTarballToStore<'a> {
                 .filter(|entry| !entry.as_ref().unwrap().header().entry_type().is_dir());
 
             let ((_, Some(capacity)) | (capacity, None)) = entries.size_hint();
-            let mut cas_paths = HashMap::<OsString, PathBuf>::with_capacity(capacity);
+            let mut cas_paths = HashMap::<String, PathBuf>::with_capacity(capacity);
             let mut pkg_files_idx = PackageFilesIndex { files: HashMap::with_capacity(capacity) };
 
             for entry in entries {
@@ -222,18 +221,18 @@ impl<'a> DownloadTarballToStore<'a> {
                 entry.read_to_end(&mut buffer).unwrap();
 
                 let entry_path = entry.path().unwrap();
-                let cleaned_entry_path =
-                    entry_path.components().skip(1).collect::<PathBuf>().into_os_string();
+                let cleaned_entry_path = entry_path
+                    .components()
+                    .skip(1)
+                    .collect::<PathBuf>()
+                    .into_os_string()
+                    .into_string()
+                    .expect("entry path must be valid UTF-8");
                 let (file_path, file_hash) = store_dir
                     .write_cas_file(&buffer, file_is_executable)
                     .map_err(TarballError::WriteCasFile)?;
 
-                let tarball_index_key = cleaned_entry_path
-                    .to_str()
-                    .expect("entry path must be valid UTF-8") // TODO: propagate this error, provide more information
-                    .to_string(); // TODO: convert cleaned_entry_path to String too.
-
-                if let Some(previous) = cas_paths.insert(cleaned_entry_path, file_path) {
+                if let Some(previous) = cas_paths.insert(cleaned_entry_path.clone(), file_path) {
                     tracing::warn!(?previous, "Duplication detected. Old entry has been ejected");
                 }
 
@@ -247,7 +246,7 @@ impl<'a> DownloadTarballToStore<'a> {
                     size: file_size,
                 };
 
-                if let Some(previous) = pkg_files_idx.files.insert(tarball_index_key, file_attrs) {
+                if let Some(previous) = pkg_files_idx.files.insert(cleaned_entry_path, file_attrs) {
                     tracing::warn!(?previous, "Duplication detected. Old entry has been ejected");
                 }
             }