refactor: split review pattern repository helpers

haasonsaas · haasonsaas · commit d721da251c65 · 2026-03-13T10:40:20.000-07:00
Made-with: Cursor
diff --git a/TODO.md b/TODO.md
@@ -10,19 +10,15 @@
 
 ## Immediate Queue
 
-- [ ] `src/review/context_helpers/pattern_repositories.rs`
-  - Split local path resolution from git-source resolution.
-  - Split git URL safety validation from clone/pull cache management.
-  - Isolate checkout cache hashing/path construction and shell-backed update helpers.
-
-## Review Backlog
-
 - [ ] `src/review/filters.rs`
   - Split comment-type classification and comment-type filtering.
   - Split vague-comment detection heuristics from vague-comment filtering.
   - Split explicit/adaptive feedback suppression decisions from filter application.
   - Split feedback-confidence lookup fallback order from confidence mutation.
   - Keep `apply_review_filters()` as a thin composition root.
+
+## Review Backlog
+
 - [ ] `src/review/feedback.rs`
   - Split feedback stats/data models from file-pattern derivation.
   - Split JSON persistence and atomic-write helpers from store mutation helpers.
diff --git a/src/review/context_helpers/pattern_repositories.rs b/src/review/context_helpers/pattern_repositories.rs
@@ -1,141 +1,22 @@
 use std::collections::HashMap;
-use std::hash::{Hash, Hasher};
-use std::path::{Path, PathBuf};
-use tracing::warn;
+use std::path::PathBuf;
 
-use crate::config;
+#[path = "pattern_repositories/checkout.rs"]
+mod checkout;
+#[path = "pattern_repositories/git.rs"]
+mod git;
+#[path = "pattern_repositories/local.rs"]
+mod local;
+#[path = "pattern_repositories/run.rs"]
+mod run;
 
 pub type PatternRepositoryMap = HashMap<String, PathBuf>;
 
-pub fn resolve_pattern_repositories(
-    config: &config::Config,
-    repo_root: &Path,
-) -> PatternRepositoryMap {
-    let mut resolved = HashMap::new();
-    if config.pattern_repositories.is_empty() {
-        return resolved;
-    }
-
-    for repo in &config.pattern_repositories {
-        if resolved.contains_key(&repo.source) {
-            continue;
-        }
-
-        let source_path = Path::new(&repo.source);
-        if source_path.is_absolute() && source_path.is_dir() {
-            if let Ok(path) = source_path.canonicalize() {
-                resolved.insert(repo.source.clone(), path);
-            }
-            continue;
-        }
-
-        let repo_relative = repo_root.join(&repo.source);
-        if repo_relative.is_dir() {
-            if let Ok(path) = repo_relative.canonicalize() {
-                resolved.insert(repo.source.clone(), path);
-            }
-            continue;
-        }
-
-        if is_git_source(&repo.source) {
-            if let Some(path) = prepare_pattern_repository_checkout(&repo.source) {
-                resolved.insert(repo.source.clone(), path);
-                continue;
-            }
-        }
-
-        warn!(
-            "Skipping pattern repository '{}' (not a readable local path or cloneable git source)",
-            repo.source
-        );
-    }
-
-    resolved
-}
-
-/// Check whether a source string looks like a git URL and uses an allowed scheme.
-/// Accepts `https://`, `ssh://`, `git@` (SSH shorthand), and `http://` (with `.git` suffix).
-fn is_safe_git_url(source: &str) -> bool {
-    source.starts_with("https://")
-        || source.starts_with("ssh://")
-        || source.starts_with("git@")
-        || (source.starts_with("http://") && source.ends_with(".git"))
-}
-
-fn is_git_source(source: &str) -> bool {
-    is_safe_git_url(source)
-}
-
-fn prepare_pattern_repository_checkout(source: &str) -> Option<PathBuf> {
-    use std::process::Command;
-
-    if !is_safe_git_url(source) {
-        warn!(
-            "Rejecting pattern repository '{}': only https://, ssh://, and git@ URLs are allowed",
-            source
-        );
-        return None;
-    }
-
-    let home_dir = dirs::home_dir()?;
-    let cache_root = home_dir.join(".diffscope").join("pattern_repositories");
-    if std::fs::create_dir_all(&cache_root).is_err() {
-        return None;
-    }
-
-    let mut hasher = std::collections::hash_map::DefaultHasher::new();
-    source.hash(&mut hasher);
-    let repo_dir = cache_root.join(format!("{:x}", hasher.finish()));
-
-    if repo_dir.is_dir() {
-        let pull_result = Command::new("git")
-            .arg("-C")
-            .arg(&repo_dir)
-            .arg("pull")
-            .arg("--ff-only")
-            .output();
-        if let Err(err) = pull_result {
-            warn!(
-                "Unable to update cached pattern repository {}: {}",
-                source, err
-            );
-        }
-    } else {
-        let clone_result = Command::new("git")
-            .arg("clone")
-            .arg("--depth")
-            .arg("1")
-            .arg(source)
-            .arg(&repo_dir)
-            .output();
-        match clone_result {
-            Ok(output) if output.status.success() => {}
-            Ok(output) => {
-                let stderr = String::from_utf8_lossy(&output.stderr);
-                warn!(
-                    "Failed to clone pattern repository {}: {}",
-                    source,
-                    stderr.trim()
-                );
-                return None;
-            }
-            Err(err) => {
-                warn!("Failed to clone pattern repository {}: {}", source, err);
-                return None;
-            }
-        }
-    }
-
-    if repo_dir.is_dir() {
-        Some(repo_dir)
-    } else {
-        None
-    }
-}
+pub use run::resolve_pattern_repositories;
 
 #[cfg(test)]
 mod tests {
-    use super::*;
+    use super::git::{is_git_source, is_safe_git_url};
 
     #[test]
     fn test_is_git_source_https() {
diff --git a/src/review/context_helpers/pattern_repositories/checkout.rs b/src/review/context_helpers/pattern_repositories/checkout.rs
@@ -0,0 +1,74 @@
+use std::hash::{Hash, Hasher};
+use std::path::PathBuf;
+
+use tracing::warn;
+
+use super::git::is_safe_git_url;
+
+pub(super) fn prepare_pattern_repository_checkout(source: &str) -> Option<PathBuf> {
+    use std::process::Command;
+
+    if !is_safe_git_url(source) {
+        warn!(
+            "Rejecting pattern repository '{}': only https://, ssh://, and git@ URLs are allowed",
+            source
+        );
+        return None;
+    }
+
+    let repo_dir = pattern_repository_cache_dir(source)?;
+    if repo_dir.is_dir() {
+        let pull_result = Command::new("git")
+            .arg("-C")
+            .arg(&repo_dir)
+            .arg("pull")
+            .arg("--ff-only")
+            .output();
+        if let Err(err) = pull_result {
+            warn!(
+                "Unable to update cached pattern repository {}: {}",
+                source, err
+            );
+        }
+    } else {
+        let clone_result = Command::new("git")
+            .arg("clone")
+            .arg("--depth")
+            .arg("1")
+            .arg(source)
+            .arg(&repo_dir)
+            .output();
+        match clone_result {
+            Ok(output) if output.status.success() => {}
+            Ok(output) => {
+                let stderr = String::from_utf8_lossy(&output.stderr);
+                warn!(
+                    "Failed to clone pattern repository {}: {}",
+                    source,
+                    stderr.trim()
+                );
+                return None;
+            }
+            Err(err) => {
+                warn!("Failed to clone pattern repository {}: {}", source, err);
+                return None;
+            }
+        }
+    }
+
+    if repo_dir.is_dir() {
+        Some(repo_dir)
+    } else {
+        None
+    }
+}
+
+fn pattern_repository_cache_dir(source: &str) -> Option<PathBuf> {
+    let home_dir = dirs::home_dir()?;
+    let cache_root = home_dir.join(".diffscope").join("pattern_repositories");
+    std::fs::create_dir_all(&cache_root).ok()?;
+
+    let mut hasher = std::collections::hash_map::DefaultHasher::new();
+    source.hash(&mut hasher);
+    Some(cache_root.join(format!("{:x}", hasher.finish())))
+}
diff --git a/src/review/context_helpers/pattern_repositories/git.rs b/src/review/context_helpers/pattern_repositories/git.rs
@@ -0,0 +1,10 @@
+pub(super) fn is_safe_git_url(source: &str) -> bool {
+    source.starts_with("https://")
+        || source.starts_with("ssh://")
+        || source.starts_with("git@")
+        || (source.starts_with("http://") && source.ends_with(".git"))
+}
+
+pub(super) fn is_git_source(source: &str) -> bool {
+    is_safe_git_url(source)
+}
diff --git a/src/review/context_helpers/pattern_repositories/local.rs b/src/review/context_helpers/pattern_repositories/local.rs
@@ -0,0 +1,15 @@
+use std::path::{Path, PathBuf};
+
+pub(super) fn resolve_local_repository_path(source: &str, repo_root: &Path) -> Option<PathBuf> {
+    let source_path = Path::new(source);
+    if source_path.is_absolute() && source_path.is_dir() {
+        return source_path.canonicalize().ok();
+    }
+
+    let repo_relative = repo_root.join(source);
+    if repo_relative.is_dir() {
+        return repo_relative.canonicalize().ok();
+    }
+
+    None
+}
diff --git a/src/review/context_helpers/pattern_repositories/run.rs b/src/review/context_helpers/pattern_repositories/run.rs
@@ -0,0 +1,45 @@
+use std::path::Path;
+
+use tracing::warn;
+
+use crate::config;
+
+use super::checkout::prepare_pattern_repository_checkout;
+use super::git::is_git_source;
+use super::local::resolve_local_repository_path;
+use super::PatternRepositoryMap;
+
+pub fn resolve_pattern_repositories(
+    config: &config::Config,
+    repo_root: &Path,
+) -> PatternRepositoryMap {
+    let mut resolved = PatternRepositoryMap::new();
+    if config.pattern_repositories.is_empty() {
+        return resolved;
+    }
+
+    for repo in &config.pattern_repositories {
+        if resolved.contains_key(&repo.source) {
+            continue;
+        }
+
+        if let Some(path) = resolve_local_repository_path(&repo.source, repo_root) {
+            resolved.insert(repo.source.clone(), path);
+            continue;
+        }
+
+        if is_git_source(&repo.source) {
+            if let Some(path) = prepare_pattern_repository_checkout(&repo.source) {
+                resolved.insert(repo.source.clone(), path);
+                continue;
+            }
+        }
+
+        warn!(
+            "Skipping pattern repository '{}' (not a readable local path or cloneable git source)",
+            repo.source
+        );
+    }
+
+    resolved
+}
