Make sure all cache access is encapsulated in the ProtofetchGitCache

src/api/builder.rs

@@ -2,7 +2,7 @@ use std::{env, error::Error, path::PathBuf};
 
 use home::home_dir;
 
-use crate::{cache::ProtofetchGitCache, Protofetch};
+use crate::{git::cache::ProtofetchGitCache, Protofetch};
 
 #[derive(Default)]
 pub struct ProtofetchBuilder {

src/api/mod.rs

@@ -4,8 +4,8 @@ use std::{
 };
 
 use crate::{
-    cache::ProtofetchGitCache,
-    cli::command_handlers::{do_clean, do_clear_cache, do_fetch, do_init, do_lock, do_migrate},
+    cli::command_handlers::{do_clean, do_fetch, do_init, do_lock, do_migrate},
+    git::cache::ProtofetchGitCache,
 };
 
 mod builder;
@@ -89,6 +89,7 @@ impl Protofetch {
     }
 
     pub fn clear_cache(&self) -> Result<(), Box<dyn Error>> {
-        do_clear_cache(&self.cache)
+        self.cache.clear()?;
+        Ok(())
     }
 }

src/cache/git.rs

@@ -0,0 +1,33 @@
+use std::path::PathBuf;
+
+use crate::{
+    git::cache::ProtofetchGitCache,
+    model::protofetch::{Coordinate, DependencyName, RevisionSpecification},
+};
+
+use super::RepositoryCache;
+
+impl RepositoryCache for ProtofetchGitCache {
+    fn fetch(
+        &self,
+        coordinate: &Coordinate,
+        specification: &RevisionSpecification,
+        _commit_hash: &str,
+    ) -> anyhow::Result<()> {
+        self.clone_or_update(coordinate)?
+            .resolve_commit_hash(specification)?;
+        Ok(())
+    }
+
+    fn create_worktree(
+        &self,
+        coordinate: &Coordinate,
+        commit_hash: &str,
+        name: &DependencyName,
+    ) -> anyhow::Result<PathBuf> {
+        let path = self
+            .clone_or_update(coordinate)?
+            .create_worktree(name, commit_hash)?;
+        Ok(path)
+    }
+}

src/cache/mod.rs

@@ -0,0 +1,21 @@
+mod git;
+
+use std::path::PathBuf;
+
+use crate::model::protofetch::{Coordinate, DependencyName, RevisionSpecification};
+
+pub trait RepositoryCache {
+    fn fetch(
+        &self,
+        coordinate: &Coordinate,
+        specification: &RevisionSpecification,
+        commit_hash: &str,
+    ) -> anyhow::Result<()>;
+
+    fn create_worktree(
+        &self,
+        coordinate: &Coordinate,
+        commit_hash: &str,
+        name: &DependencyName,
+    ) -> anyhow::Result<PathBuf>;
+}

src/cli/command_handlers.rs

@@ -2,8 +2,8 @@ use log::{debug, info};
 
 use crate::{
     api::LockMode,
-    cache::ProtofetchGitCache,
     fetch,
+    git::cache::ProtofetchGitCache,
     model::{
         protodep::ProtodepDescriptor,
         protofetch::{lock::LockFile, Descriptor},
@@ -17,7 +17,6 @@ use std::{
 };
 
 const DEFAULT_OUTPUT_DIRECTORY_NAME: &str = "proto_src";
-const CACHE_WORKSPACES_DIRECTORY_NAME: &str = "dependencies";
 
 /// Handler to fetch command
 pub fn do_fetch(
@@ -32,18 +31,13 @@ pub fn do_fetch(
 
     let lockfile = do_lock(lock_mode, cache, root, module_file_name, lock_file_name)?;
 
-    let cache_dependencies_directory_path = cache.location.join(CACHE_WORKSPACES_DIRECTORY_NAME);
     let output_directory_name = output_directory_name
         .or_else(|| module_descriptor.proto_out_dir.as_ref().map(Path::new))
         .unwrap_or(Path::new(DEFAULT_OUTPUT_DIRECTORY_NAME));
-    fetch::fetch_sources(cache, &lockfile, &cache_dependencies_directory_path)?;
+    fetch::fetch_sources(cache, &lockfile)?;
 
     //Copy proto_out files to actual target
-    proto::copy_proto_files(
-        &root.join(output_directory_name),
-        &cache_dependencies_directory_path,
-        &lockfile,
-    )?;
+    proto::copy_proto_files(cache, &lockfile, &root.join(output_directory_name))?;
 
     Ok(())
 }
@@ -177,19 +171,6 @@ pub fn do_clean(
     Ok(())
 }
 
-pub fn do_clear_cache(cache: &ProtofetchGitCache) -> Result<(), Box<dyn Error>> {
-    if cache.location.exists() {
-        info!(
-            "Clearing protofetch repository cache {}.",
-            &cache.location.display()
-        );
-        std::fs::remove_dir_all(&cache.location)?;
-        Ok(())
-    } else {
-        Ok(())
-    }
-}
-
 fn load_module_descriptor(
     root: &Path,
     module_file_name: &Path,

src/fetch.rs

@@ -1,35 +1,28 @@
-use std::{
-    collections::BTreeMap,
-    path::{Path, PathBuf},
-    str::Utf8Error,
-};
+use std::{collections::BTreeMap, str::Utf8Error};
 
 use crate::{
-    cache::{CacheError, RepositoryCache},
+    cache::RepositoryCache,
     model::protofetch::{
         lock::{LockFile, LockedDependency},
         Dependency, DependencyName, Descriptor,
     },
-    proto_repository::ProtoRepository,
     resolver::ModuleResolver,
 };
-use log::{debug, error, info};
+use log::{error, info};
 use thiserror::Error;
 
 #[derive(Error, Debug)]
 pub enum FetchError {
     #[error("Error while fetching repo from cache: {0}")]
-    Cache(#[from] CacheError),
+    Cache(anyhow::Error),
     #[error("Git error: {0}")]
     GitError(#[from] git2::Error),
     #[error("Error while decoding utf8 bytes from blob: {0}")]
     BlobRead(#[from] Utf8Error),
     #[error("Error while parsing descriptor")]
     Parsing(#[from] crate::model::ParseError),
-    #[error("Bad output dir {0}")]
-    BadOutputDir(String),
     #[error("Error while processing protobuf repository: {0}")]
-    ProtoRepoError(#[from] crate::proto_repository::ProtoRepoError),
+    ProtoRepoError(#[from] crate::git::repository::ProtoRepoError),
     #[error("IO error: {0}")]
     IO(#[from] std::io::Error),
     #[error(transparent)]
@@ -114,47 +107,15 @@ pub fn lock(
     })
 }
 
-pub fn fetch_sources<Cache: RepositoryCache>(
-    cache: &Cache,
-    lockfile: &LockFile,
-    cache_src_dir: &Path,
-) -> Result<(), FetchError> {
+pub fn fetch_sources(cache: &impl RepositoryCache, lockfile: &LockFile) -> Result<(), FetchError> {
     info!("Fetching dependencies source files...");
-
-    if !cache_src_dir.exists() {
-        std::fs::create_dir_all(cache_src_dir)?;
+    for dep in &lockfile.dependencies {
+        cache
+            .fetch(&dep.coordinate, &dep.specification, &dep.commit_hash)
+            .map_err(FetchError::Cache)?;
     }
 
-    if cache_src_dir.is_dir() {
-        for dep in &lockfile.dependencies {
-            //If the dependency is already in the cache, we don't need to fetch it again
-            if cache_src_dir
-                .join(&dep.name.value)
-                .join(PathBuf::from(&dep.commit_hash))
-                .exists()
-            {
-                debug!("Skipping fetching {:?}. Already in cache", dep.name);
-                continue;
-            }
-            let repo = cache.clone_or_update(&dep.coordinate)?;
-            let work_tree_res = repo.create_worktrees(
-                &lockfile.module_name,
-                &dep.name,
-                &dep.commit_hash,
-                cache_src_dir,
-            );
-            if let Err(err) = work_tree_res {
-                error!("Error while trying to create worktrees {err}. \
-                Most likely the worktree sources have been deleted but the worktree metadata has not. \
-                Please delete the cache and run protofetch fetch again.")
-            }
-        }
-        Ok(())
-    } else {
-        Err(FetchError::BadOutputDir(
-            cache_src_dir.to_str().unwrap_or("").to_string(),
-        ))
-    }
+    Ok(())
 }
 
 #[cfg(test)]

src/cache.rs → src/git/cache.rs

@@ -1,22 +1,17 @@
 use std::path::{Path, PathBuf};
 
-use git2::Config;
-use git2::{build::RepoBuilder, Cred, CredentialType, FetchOptions, RemoteCallbacks, Repository};
-use log::trace;
+use git2::{
+    build::RepoBuilder, Config, Cred, CredentialType, FetchOptions, RemoteCallbacks, Repository,
+};
+use log::{info, trace};
 use thiserror::Error;
 
-use crate::{model::protofetch::Coordinate, proto_repository::ProtoGitRepository};
+use crate::{git::repository::ProtoGitRepository, model::protofetch::Coordinate};
 
-use crate::proto_repository::ProtoRepository;
-
-pub trait RepositoryCache {
-    type Repository: ProtoRepository;
-
-    fn clone_or_update(&self, entry: &Coordinate) -> Result<Self::Repository, CacheError>;
-}
+const WORKTREES_DIR: &str = "dependencies";
 
 pub struct ProtofetchGitCache {
-    pub location: PathBuf,
+    location: PathBuf,
     git_config: Config,
 }
 
@@ -30,25 +25,6 @@ pub enum CacheError {
     IO(#[from] std::io::Error),
 }
 
-impl RepositoryCache for ProtofetchGitCache {
-    type Repository = ProtoGitRepository;
-
-    fn clone_or_update(&self, entry: &Coordinate) -> Result<Self::Repository, CacheError> {
-        let repo = match self.get_entry(entry) {
-            None => self.clone_repo(entry)?,
-            Some(path) => {
-                let mut repo = self.open_entry(&path)?;
-
-                self.fetch(&mut repo)?;
-
-                repo
-            }
-        };
-
-        Ok(ProtoGitRepository::new(repo))
-    }
-}
-
 impl ProtofetchGitCache {
     pub fn new(location: PathBuf, git_config: Config) -> Result<ProtofetchGitCache, CacheError> {
         if location.exists() && location.is_dir() {
@@ -69,6 +45,35 @@ impl ProtofetchGitCache {
         }
     }
 
+    pub fn clear(&self) -> anyhow::Result<()> {
+        if self.location.exists() {
+            info!(
+                "Clearing protofetch repository cache {}.",
+                &self.location.display()
+            );
+            std::fs::remove_dir_all(&self.location)?;
+        }
+        Ok(())
+    }
+
+    pub fn clone_or_update(&self, entry: &Coordinate) -> Result<ProtoGitRepository, CacheError> {
+        let repo = match self.get_entry(entry) {
+            None => self.clone_repo(entry)?,
+            Some(path) => {
+                let mut repo = self.open_entry(&path)?;
+
+                self.fetch(&mut repo)?;
+
+                repo
+            }
+        };
+
+        Ok(ProtoGitRepository::new(
+            repo,
+            self.location.join(WORKTREES_DIR),
+        ))
+    }
+
     fn get_entry(&self, entry: &Coordinate) -> Option<PathBuf> {
         let mut full_path = self.location.clone();
         full_path.push(entry.as_path());

src/git/mod.rs

@@ -0,0 +1,2 @@
+pub mod cache;
+pub mod repository;