diff --git a/.claude/rules/caching-strategy.md b/.claude/rules/caching-strategy.md index 73034151c..cce08a14a 100644 --- a/.claude/rules/caching-strategy.md +++ b/.claude/rules/caching-strategy.md @@ -12,17 +12,27 @@ Everything else (remote URLs, project config, branch metadata) is read-only. ## Caching Implementation -`Repository` uses `OnceCell` for per-instance caching. Since instances are short-lived (command duration), we can cache aggressively: +`Repository` uses a global cache keyed by `git_common_dir`. The cache is +shared across all `Repository` instances within a process. + +**Architecture note:** Using a HashMap keyed by `git_common_dir` is slightly +wasteful since every `Repository` instance must compute `git rev-parse +--git-common-dir` to look up the cache. However, tests require isolation (each +test creates different repos that need their own cached values), and the +HashMap approach is the simplest way to achieve this. Alternative approaches +(test-only cache clearing with RwLock instead of OnceCell) have their own +trade-offs. **Currently cached:** -- `git_common_dir()` — never changes -- `worktree_root()` — never changes +- `git_common_dir()` — cached per-instance (also used as HashMap key) +- `worktree_root()` — per-worktree, keyed by path - `worktree_base()` — derived from git_common_dir and is_bare - `is_bare()` — git config, doesn't change -- `current_branch()` — we don't switch branches within a worktree +- `current_branch()` — per-worktree, keyed by path - `project_identifier()` — derived from remote URL - `primary_remote()` — git config, doesn't change - `default_branch()` — from git config or detection, doesn't change +- `merge_base()` — keyed by (commit1, commit2) pair **Not cached (intentionally):** - `is_dirty()` — changes as we stage/commit @@ -31,23 +41,38 @@ Everything else (remote URLs, project config, branch metadata) is read-only. **Adding new cached methods:** 1. Add field to `RepoCache` struct: `field_name: OnceCell` -2. Use `get_or_try_init()` pattern for fallible initialization -3. Return `&T` (for references) or `.copied()`/`.cloned()` (for values) +2. Use `with_cache()` helper to access the shared cache +3. Return owned values (String, PathBuf, bool) ```rust -// Return reference (avoids allocation) -pub fn cached_ref(&self) -> anyhow::Result<&str> { - self.cache - .field_name - .get_or_try_init(|| { /* compute String */ }) - .map(String::as_str) +// For repo-wide values (same for all worktrees) +pub fn cached_value(&self) -> anyhow::Result { + self.with_cache(|cache| { + cache + .field_name + .get_or_init(|| { /* compute value */ }) + .clone() + }) } -// Return owned value (when caller needs ownership) -pub fn cached_value(&self) -> anyhow::Result { - self.cache - .field_name - .get_or_try_init(|| { /* compute bool */ }) - .copied() +// For per-worktree values (different per worktree path) +pub fn cached_per_worktree(&self) -> anyhow::Result { + let worktree_key = self.path.clone(); + + self.with_cache(|cache| { + // Check cache first + if let Ok(map) = cache.field_name.read() + && let Some(cached) = map.get(&worktree_key) + { + return cached.clone(); + } + + // Cache miss - compute and store + let result = /* compute value */; + if let Ok(mut map) = cache.field_name.write() { + map.insert(worktree_key, result.clone()); + } + result + }) } ``` diff --git a/Cargo.lock b/Cargo.lock index 7b5aa4b28..874b58499 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -703,6 +703,20 @@ dependencies = [ "syn", ] +[[package]] +name = "dashmap" +version = "6.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5041cc499144891f3790297212f32a74fb938e5136a14943f338ef9e0ae276cf" +dependencies = [ + "cfg-if", + "crossbeam-utils", + "hashbrown 0.14.5", + "lock_api", + "once_cell", + "parking_lot_core", +] + [[package]] name = "defer-drop" version = "1.3.0" @@ -3215,6 +3229,7 @@ dependencies = [ "criterion", "crossbeam-channel", "crossterm", + "dashmap", "dirs", "dunce", "env_logger", diff --git a/Cargo.toml b/Cargo.toml index 763c32790..3e05e48ae 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -119,6 +119,7 @@ urlencoding = "2.1" regex = "1.12" ignore = "0.4" reflink-copy = "0.1" +dashmap = "6.1.0" [target.'cfg(unix)'.dependencies] skim = "0.20" diff --git a/src/commands/command_approval.rs b/src/commands/command_approval.rs index 3633f89ae..c73c1485f 100644 --- a/src/commands/command_approval.rs +++ b/src/commands/command_approval.rs @@ -225,5 +225,5 @@ pub fn approve_hooks_filtered( } let project_id = ctx.repo.project_identifier()?; - approve_command_batch(&commands, project_id, ctx.config, ctx.yes, false) + approve_command_batch(&commands, &project_id, ctx.config, ctx.yes, false) } diff --git a/src/commands/command_executor.rs b/src/commands/command_executor.rs index b8ea5c7e5..65e2d15a5 100644 --- a/src/commands/command_executor.rs +++ b/src/commands/command_executor.rs @@ -111,7 +111,7 @@ pub fn build_hook_context( if let Ok(remote) = ctx.repo.primary_remote() { map.insert("remote".into(), remote.to_string()); // Add remote URL for conditional hook execution (e.g., GitLab vs GitHub) - if let Some(url) = ctx.repo.remote_url(remote) { + if let Some(url) = ctx.repo.remote_url(&remote) { map.insert("remote_url".into(), url); } if let Some(branch) = ctx.branch diff --git a/src/commands/config.rs b/src/commands/config.rs index 07f553332..41ea46293 100644 --- a/src/commands/config.rs +++ b/src/commands/config.rs @@ -1025,7 +1025,7 @@ pub fn handle_state_get(key: &str, refresh: bool, branch: Option) -> any } let has_upstream = repo.upstream_branch(&branch_name).ok().flatten().is_some(); - let ci_status = PrStatus::detect(&branch_name, &head, repo_root, has_upstream) + let ci_status = PrStatus::detect(&branch_name, &head, &repo_root, has_upstream) .map_or(super::list::ci_status::CiStatus::NoCI, |s| s.ci_status); let status_str: &'static str = ci_status.into(); crate::output::stdout(status_str)?; diff --git a/src/commands/context.rs b/src/commands/context.rs index 7be7eb7aa..dcb1cfaa1 100644 --- a/src/commands/context.rs +++ b/src/commands/context.rs @@ -57,8 +57,7 @@ impl CommandEnv { // Propagate git errors (broken repo, missing git) but allow None for detached HEAD let branch = repo .current_branch() - .context("Failed to determine current branch")? - .map(str::to_string); + .context("Failed to determine current branch")?; let config = WorktrunkConfig::load().context("Failed to load config")?; let repo_root = repo .worktree_base() diff --git a/src/commands/hook_commands.rs b/src/commands/hook_commands.rs index a0295597e..bf3bc8dde 100644 --- a/src/commands/hook_commands.rs +++ b/src/commands/hook_commands.rs @@ -292,7 +292,7 @@ pub fn add_approvals(show_all: bool) -> anyhow::Result<()> { let commands_to_approve = if !show_all { let unapproved: Vec<_> = commands .into_iter() - .filter(|cmd| !config.is_command_approved(project_id, &cmd.command.template)) + .filter(|cmd| !config.is_command_approved(&project_id, &cmd.command.template)) .collect(); if unapproved.is_empty() { @@ -309,7 +309,7 @@ pub fn add_approvals(show_all: bool) -> anyhow::Result<()> { // When show_all=true, we've already included all commands in commands_to_approve // When show_all=false, we've already filtered to unapproved commands // So we pass skip_approval_filter=true to prevent double-filtering - let approved = approve_command_batch(&commands_to_approve, project_id, &config, false, true)?; + let approved = approve_command_batch(&commands_to_approve, &project_id, &config, false, true)?; // Show result if approved { @@ -349,7 +349,7 @@ pub fn clear_approvals(global: bool) -> anyhow::Result<()> { let project_id = repo.project_identifier()?; // Check if project has any approvals - let had_approvals = config.projects.contains_key(project_id); + let had_approvals = config.projects.contains_key(&project_id); if !had_approvals { crate::output::print(info_message("No approvals to clear for this project"))?; @@ -359,12 +359,12 @@ pub fn clear_approvals(global: bool) -> anyhow::Result<()> { // Count approvals before removing let approval_count = config .projects - .get(project_id) + .get(&project_id) .map(|p| p.approved_commands.len()) .unwrap_or(0); config - .revoke_project(project_id, None) + .revoke_project(&project_id, None) .context("Failed to clear project approvals")?; crate::output::print(success_message(format!( @@ -419,7 +419,7 @@ pub fn handle_hook_show(hook_type_filter: Option<&str>, expanded: bool) -> anyho &repo, project_config.as_ref(), &config, - project_id, + project_id.as_deref(), filter, ctx.as_ref(), )?; diff --git a/src/commands/list/ci_status.rs b/src/commands/list/ci_status.rs index b0e4eb68e..93a9381bf 100644 --- a/src/commands/list/ci_status.rs +++ b/src/commands/list/ci_status.rs @@ -994,7 +994,7 @@ impl CachedCiStatus { Err(_) => return Vec::new(), }; - let Some(cache_dir) = Self::cache_dir(repo_root) else { + let Some(cache_dir) = Self::cache_dir(&repo_root) else { return Vec::new(); }; @@ -1028,7 +1028,7 @@ impl CachedCiStatus { Err(_) => return 0, }; - let Some(cache_dir) = Self::cache_dir(repo_root) else { + let Some(cache_dir) = Self::cache_dir(&repo_root) else { return 0; }; diff --git a/src/commands/list/collect.rs b/src/commands/list/collect.rs index 91380ab0f..458aedd73 100644 --- a/src/commands/list/collect.rs +++ b/src/commands/list/collect.rs @@ -253,8 +253,10 @@ impl TaskResult { } impl TaskKind { - /// Whether this task requires network I/O (slow). - fn is_network(self) -> bool { + /// Whether this task requires network access. + /// + /// Network tasks are sorted to run last to avoid blocking local tasks. + pub fn is_network(self) -> bool { matches!(self, TaskKind::CiStatus | TaskKind::UrlStatus) } } @@ -1083,11 +1085,10 @@ pub fn collect( )); } - // Sort: local git ops first, network ops (CI, URL) last. + // Sort work items: network tasks last to avoid blocking local operations all_work_items.sort_by_key(|item| item.kind.is_network()); - // Phase 2: Execute all work items in Rayon's thread pool - // Set thread-local timeout for each worker (used by shell_exec::run) + // Phase 2: Execute all work items in parallel all_work_items.into_par_iter().for_each(|item| { worktrunk::shell_exec::set_command_timeout(command_timeout); let result = item.execute(); @@ -1460,10 +1461,10 @@ pub fn populate_item( &tx, ); - // Sort: local git ops first, network ops last + // Sort: network tasks last work_items.sort_by_key(|item| item.kind.is_network()); - // Execute all work items in Rayon's thread pool + // Execute all tasks in parallel work_items.into_par_iter().for_each(|item| { let result = item.execute(); let _ = tx.send(result); diff --git a/src/commands/list/collect_progressive_impl.rs b/src/commands/list/collect_progressive_impl.rs index e36156c36..b3eefb7e6 100644 --- a/src/commands/list/collect_progressive_impl.rs +++ b/src/commands/list/collect_progressive_impl.rs @@ -19,7 +19,7 @@ use crossbeam_channel::Sender; use std::fmt::Display; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::sync::Arc; use worktrunk::git::{LineDiff, Repository, Worktree}; @@ -450,6 +450,7 @@ impl Task for AheadBehindTask { let (ahead, behind) = repo .ahead_behind(base, &ctx.commit_sha) .map_err(|e| ctx.error(Self::KIND, e))?; + Ok(TaskResult::AheadBehind { item_idx: ctx.item_idx, counts: AheadBehind { ahead, behind }, @@ -504,11 +505,12 @@ impl Task for HasFileChangesTask { has_file_changes: true, }); }; - let base = ctx.require_target(Self::KIND)?; + let target = ctx.require_target(Self::KIND)?; let repo = ctx.repo(); let has_file_changes = repo - .has_added_changes(branch, base) + .has_added_changes(branch, target) .map_err(|e| ctx.error(Self::KIND, e))?; + Ok(TaskResult::HasFileChanges { item_idx: ctx.item_idx, has_file_changes, @@ -571,6 +573,7 @@ impl Task for IsAncestorTask { let is_ancestor = repo .is_ancestor(&ctx.commit_sha, base) .map_err(|e| ctx.error(Self::KIND, e))?; + Ok(TaskResult::IsAncestor { item_idx: ctx.item_idx, is_ancestor, @@ -590,6 +593,7 @@ impl Task for BranchDiffTask { let diff = repo .branch_diff_stats(base, &ctx.commit_sha) .map_err(|e| ctx.error(Self::KIND, e))?; + Ok(TaskResult::BranchDiff { item_idx: ctx.item_idx, branch_diff: BranchDiffTotals { diff }, @@ -817,7 +821,6 @@ impl Task for CiStatusTask { let repo_path = repo .worktree_root() .ok() - .map(Path::to_path_buf) .unwrap_or_else(|| ctx.repo_path.clone()); let pr_status = ctx.branch.as_deref().and_then(|branch| { diff --git a/src/commands/step_commands.rs b/src/commands/step_commands.rs index 29c08da80..eb89e307a 100644 --- a/src/commands/step_commands.rs +++ b/src/commands/step_commands.rs @@ -299,7 +299,7 @@ pub fn step_show_squash_prompt( let target_branch = repo.resolve_target_branch(target)?; // Get current branch - let current_branch = repo.current_branch()?.unwrap_or("HEAD"); + let current_branch = repo.current_branch()?.unwrap_or_else(|| "HEAD".to_string()); // Get merge base with target branch let merge_base = repo.merge_base("HEAD", &target_branch)?; @@ -319,7 +319,7 @@ pub fn step_show_squash_prompt( &target_branch, &merge_base, &subjects, - current_branch, + ¤t_branch, repo_name, config, )?; diff --git a/src/commands/worktree.rs b/src/commands/worktree.rs index 516b787e7..0fc06b136 100644 --- a/src/commands/worktree.rs +++ b/src/commands/worktree.rs @@ -540,7 +540,7 @@ pub fn handle_switch( // Branch-first lookup: check if branch has a worktree anywhere match repo.worktree_for_branch(&resolved_branch)? { Some(existing_path) if existing_path.exists() => { - let _ = repo.record_switch_previous(new_previous); + let _ = repo.record_switch_previous(new_previous.as_deref()); return Ok(switch_to_existing(existing_path)); } Some(_) => { @@ -732,7 +732,7 @@ pub fn handle_switch( // (see main.rs switch handler for temporal locality) // Record successful switch in history for `wt switch -` support - let _ = repo.record_switch_previous(new_previous); + let _ = repo.record_switch_previous(new_previous.as_deref()); Ok(( SwitchResult::Created { diff --git a/src/git/repository/mod.rs b/src/git/repository/mod.rs index 2f263c29f..3511179c1 100644 --- a/src/git/repository/mod.rs +++ b/src/git/repository/mod.rs @@ -1,8 +1,10 @@ +use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::process::Command; -use std::sync::OnceLock; +use std::sync::{Arc, OnceLock, RwLock}; -use once_cell::sync::OnceCell; +use dashmap::DashMap; +use once_cell::sync::{Lazy, OnceCell}; use anyhow::{Context, bail}; use normalize_path::NormalizePath; @@ -17,6 +19,75 @@ use super::{ LineDiff, Worktree, }; +// ============================================================================ +// Global Cache +// ============================================================================ + +/// Global cache keyed by `git_common_dir`. +/// +/// NOTE: Using a HashMap is slightly wasteful since every Repository instance +/// must compute `git rev-parse --git-common-dir` to look up the cache. However, +/// tests require isolation (each test creates different repos that need their +/// own cached values), and the HashMap approach is the simplest way to achieve +/// this. Alternative approaches (test-only cache clearing with RwLock instead +/// of OnceCell) have their own trade-offs. +static REPO_CACHE: Lazy>>> = Lazy::new(Default::default); + +/// Get the shared cache for a repository, creating it if needed. +/// +/// Returns an Arc to the cache, allowing the caller to release the HashMap +/// lock before accessing cached values. This prevents deadlocks when cached +/// methods call each other. +fn get_cache(key: PathBuf) -> Arc { + // Try read-only first + { + let caches = REPO_CACHE.read().unwrap(); + if let Some(cache) = caches.get(&key) { + return Arc::clone(cache); + } + } + + // Cache miss - need write lock to insert + let mut caches = REPO_CACHE.write().unwrap(); + let cache = caches + .entry(key) + .or_insert_with(|| Arc::new(RepoCache::default())); + Arc::clone(cache) +} + +/// Cached data for a single repository. +/// +/// Contains: +/// - Repo-wide values (same for all worktrees): is_bare, default_branch, etc. +/// - Per-worktree values keyed by path: worktree_root, current_branch +/// +/// Wrapped in Arc to allow releasing the outer HashMap lock before accessing +/// cached values, avoiding deadlocks when cached methods call each other. +#[derive(Debug, Default)] +struct RepoCache { + // ========== Repo-wide values (same for all worktrees) ========== + /// Whether this is a bare repository + is_bare: OnceCell, + /// Default branch (main, master, etc.) + default_branch: OnceCell, + /// Primary remote name (usually "origin") + primary_remote: OnceCell, + /// Project identifier derived from remote URL + project_identifier: OnceCell, + /// Base path for worktrees (repo root for normal repos, bare repo path for bare) + worktree_base: OnceCell, + /// Project config (loaded from .config/wt.toml in main worktree) + project_config: OnceCell>, + /// Merge-base cache: (commit1, commit2) -> merge_base_sha + merge_base: DashMap<(String, String), String>, + + // ========== Per-worktree values (keyed by path) ========== + /// Worktree root paths: worktree_path -> canonicalized root + worktree_roots: DashMap, + /// Current branch per worktree: worktree_path -> branch name (None = detached HEAD) + current_branches: DashMap>, +} + /// Result of resolving a worktree name. /// /// Used by `resolve_worktree` to handle different resolution outcomes: @@ -57,28 +128,6 @@ fn base_path() -> &'static PathBuf { .unwrap_or_else(|| DEFAULT.get_or_init(|| PathBuf::from("."))) } -/// Cached values for expensive git queries. -/// -/// These values don't change during a process run, so we cache them -/// after the first lookup to avoid repeated git command spawns. -/// See `.claude/rules/caching-strategy.md` for what should/shouldn't be cached. -#[derive(Debug, Default)] -struct RepoCache { - git_common_dir: OnceCell, - worktree_root: OnceCell, - current_branch: OnceCell>, - primary_remote: OnceCell, - project_identifier: OnceCell, - /// Base path for worktrees (repo root for normal repos, bare repo path for bare repos) - worktree_base: OnceCell, - /// Whether this is a bare repository - is_bare: OnceCell, - /// Project config (loaded from .config/wt.toml, cached per Repository instance) - project_config: OnceCell>, - /// Default branch (main, master, etc.) - cached from git config or detection - default_branch: OnceCell, -} - /// Repository context for git operations. /// /// Provides a more ergonomic API than the `*_in(path, ...)` functions by @@ -97,7 +146,8 @@ struct RepoCache { #[derive(Debug)] pub struct Repository { path: PathBuf, - cache: RepoCache, + /// Per-instance cache of git_common_dir, used as key for global cache lookup. + git_common_dir: OnceCell, } impl Repository { @@ -105,7 +155,7 @@ impl Repository { pub fn at(path: impl Into) -> Self { Self { path: path.into(), - cache: RepoCache::default(), + git_common_dir: OnceCell::new(), } } @@ -122,6 +172,24 @@ impl Repository { &self.path } + /// Compute and cache the git common directory for this instance. + /// + /// This is the key used to look up the shared cache. + fn compute_git_common_dir(&self) -> anyhow::Result { + self.git_common_dir + .get_or_try_init(|| { + let stdout = self.run_command(&["rev-parse", "--git-common-dir"])?; + let path = PathBuf::from(stdout.trim()); + if path.is_relative() { + canonicalize(self.path.join(&path)) + .context("Failed to resolve git common directory") + } else { + Ok(path) + } + }) + .cloned() + } + /// Get the primary remote name for this repository. /// /// Returns a consistent value across all worktrees (not branch-specific). @@ -131,19 +199,20 @@ impl Repository { /// 2. Otherwise, get the first remote with a configured URL /// 3. Fall back to "origin" if no remotes exist /// - /// Result is cached for the lifetime of this Repository instance. + /// Result is cached in the shared repo cache (shared across all worktrees). /// /// [1]: https://git-scm.com/docs/git-config#Documentation/git-config.txt-checkoutdefaultRemote - pub fn primary_remote(&self) -> anyhow::Result<&str> { - self.cache + pub fn primary_remote(&self) -> anyhow::Result { + let cache = get_cache(self.compute_git_common_dir()?); + Ok(cache .primary_remote - .get_or_try_init(|| { + .get_or_init(|| { // Check git's checkout.defaultRemote config if let Ok(default_remote) = self.run_command(&["config", "checkout.defaultRemote"]) { let default_remote = default_remote.trim(); if !default_remote.is_empty() && self.remote_has_url(default_remote) { - return Ok(default_remote.to_string()); + return default_remote.to_string(); } } @@ -161,9 +230,9 @@ impl Repository { .map(|(name, _)| name) }); - Ok(first_remote.unwrap_or("origin").to_string()) + first_remote.unwrap_or("origin").to_string() }) - .map(String::as_str) + .clone()) } /// Check if a remote has a URL configured. @@ -234,27 +303,34 @@ impl Repository { } /// Get the current branch name, or None if in detached HEAD state. - /// Result is cached for the lifetime of this Repository instance. - pub fn current_branch(&self) -> anyhow::Result> { - self.cache - .current_branch - .get_or_try_init(|| { - let stdout = self.run_command(&["branch", "--show-current"])?; - let branch = stdout.trim(); - Ok(if branch.is_empty() { - None // Detached HEAD - } else { - Some(branch.to_string()) - }) + /// + /// Result is cached in the global shared cache (keyed by worktree path). + pub fn current_branch(&self) -> anyhow::Result> { + let cache = get_cache(self.compute_git_common_dir()?); + + Ok(cache + .current_branches + .entry(self.path.clone()) + .or_insert_with(|| { + self.run_command(&["branch", "--show-current"]) + .ok() + .and_then(|s| { + let branch = s.trim(); + if branch.is_empty() { + None // Detached HEAD + } else { + Some(branch.to_string()) + } + }) }) - .map(|opt| opt.as_deref()) + .clone()) } /// Get the current branch name, or error if in detached HEAD state. /// /// `action` describes what requires being on a branch (e.g., "merge"). pub fn require_current_branch(&self, action: &str) -> anyhow::Result { - self.current_branch()?.map(str::to_string).ok_or_else(|| { + self.current_branch()?.ok_or_else(|| { GitError::DetachedHead { action: Some(action.into()), } @@ -374,7 +450,7 @@ impl Repository { /// - `Err` if "-" but no previous branch in history pub fn resolve_worktree_name(&self, name: &str) -> anyhow::Result { match name { - "@" => self.current_branch()?.map(str::to_string).ok_or_else(|| { + "@" => self.current_branch()?.ok_or_else(|| { GitError::DetachedHead { action: Some("resolve '@' to current branch".into()), } @@ -461,8 +537,10 @@ impl Repository { /// 4. Infer from local branches if no remote /// /// Detection results are cached to `worktrunk.default-branch` for future calls. + /// Result is cached in the shared repo cache (shared across all worktrees). pub fn default_branch(&self) -> anyhow::Result { - self.cache + let cache = get_cache(self.compute_git_common_dir()?); + cache .default_branch .get_or_try_init(|| { // Fast path: check worktrunk's persistent cache (git config) @@ -491,12 +569,12 @@ impl Repository { // Try to get from the primary remote if let Ok(remote) = self.primary_remote() { // Try git's cache for this remote (e.g., origin/HEAD) - if let Ok(branch) = self.get_local_default_branch(remote) { + if let Ok(branch) = self.get_local_default_branch(&remote) { return Ok(branch); } // Query remote (no caching to git's remote HEAD - we only manage worktrunk's cache) - if let Ok(branch) = self.query_remote_default_branch(remote) { + if let Ok(branch) = self.query_remote_default_branch(&remote) { return Ok(branch); } } @@ -589,23 +667,11 @@ impl Repository { /// See [`--git-common-dir`][1] for details. /// /// Always returns an absolute path, resolving any relative paths returned by git. - /// Result is cached for the lifetime of this Repository instance. + /// Result is cached per Repository instance (also used as key for global cache). /// /// [1]: https://git-scm.com/docs/git-rev-parse#Documentation/git-rev-parse.txt---git-common-dir - pub fn git_common_dir(&self) -> anyhow::Result<&Path> { - self.cache - .git_common_dir - .get_or_try_init(|| { - let stdout = self.run_command(&["rev-parse", "--git-common-dir"])?; - let path = PathBuf::from(stdout.trim()); - if path.is_relative() { - canonicalize(self.path.join(&path)) - .context("Failed to resolve git common directory") - } else { - Ok(path) - } - }) - .map(PathBuf::as_path) + pub fn git_common_dir(&self) -> anyhow::Result { + self.compute_git_common_dir() } /// Get the directory where worktrunk background logs are stored. @@ -637,9 +703,10 @@ impl Repository { /// For bare repositories: the bare repository directory itself. /// /// This is the path that should be used when constructing worktree paths. - /// Result is cached for the lifetime of this Repository instance. + /// Result is cached in the global shared cache (same for all worktrees in a repo). pub fn worktree_base(&self) -> anyhow::Result { - self.cache + let cache = get_cache(self.compute_git_common_dir()?); + cache .worktree_base .get_or_try_init(|| { let git_common_dir = @@ -687,9 +754,10 @@ impl Repository { /// /// Bare repositories have no main worktree — all worktrees are linked /// worktrees at templated paths, including the default branch. - /// Result is cached for the lifetime of this Repository instance. + /// Result is cached in the shared repo cache (shared across all worktrees). pub fn is_bare(&self) -> anyhow::Result { - self.cache + let cache = get_cache(self.compute_git_common_dir()?); + cache .is_bare .get_or_try_init(|| { let output = self.run_command(&["config", "--bool", "core.bare"])?; @@ -728,16 +796,21 @@ impl Repository { /// /// Returns the canonicalized absolute path to the top-level directory of the /// current working tree. This could be the main worktree or a linked worktree. - /// Result is cached for the lifetime of this Repository instance. - pub fn worktree_root(&self) -> anyhow::Result<&Path> { - self.cache - .worktree_root - .get_or_try_init(|| { - let stdout = self.run_command(&["rev-parse", "--show-toplevel"])?; - let path = PathBuf::from(stdout.trim()); - canonicalize(&path).context("Failed to canonicalize worktree root") + /// Result is cached in the global shared cache (keyed by worktree path). + pub fn worktree_root(&self) -> anyhow::Result { + let cache = get_cache(self.compute_git_common_dir()?); + + Ok(cache + .worktree_roots + .entry(self.path.clone()) + .or_insert_with(|| { + self.run_command(&["rev-parse", "--show-toplevel"]) + .ok() + .map(|s| PathBuf::from(s.trim())) + .and_then(|p| canonicalize(&p).ok()) + .unwrap_or_else(|| self.path.clone()) }) - .map(PathBuf::as_path) + .clone()) } /// Check if this is a linked worktree (vs the main worktree). @@ -811,39 +884,29 @@ impl Repository { /// Check if a branch has file changes beyond the merge-base with target. /// - /// Uses [three-dot diff][1] (`target...branch`) which shows files changed from - /// merge-base to branch. Returns false when the diff is empty (no added changes). + /// Uses merge-base (cached) to find common ancestor, then two-dot diff to + /// check for file changes. Returns false when the diff is empty (no added changes). /// /// For orphan branches (no common ancestor with target), returns true since all /// their changes are unique. - /// - /// [1]: https://git-scm.com/docs/git-diff#Documentation/git-diff.txt-emgitdiffemltoptionsgtltcommitgtltcommitgt--telepathhellip pub fn has_added_changes(&self, branch: &str, target: &str) -> anyhow::Result { - use crate::shell_exec::run; - - // git diff --name-only target...branch shows files changed from merge-base to branch - let range = format!("{target}...{branch}"); - let mut cmd = std::process::Command::new("git"); - cmd.args(["diff", "--name-only", &range]); - cmd.current_dir(&self.path); - - let output = run(&mut cmd, Some(&self.logging_context())) - .with_context(|| format!("Failed to execute: git diff --name-only {range}"))?; - - if output.status.success() { - let stdout = String::from_utf8_lossy(&output.stdout); - Ok(!stdout.trim().is_empty()) - } else { - let stderr = String::from_utf8_lossy(&output.stderr); - // Orphan branches have no merge base with target - they definitely have unique changes. - // Alternative: pre-check with `git merge-base`, but that adds a git call for every - // branch. String matching here only costs extra for the rare orphan branch case. - if stderr.contains("no merge base") { - Ok(true) - } else { - bail!("{}", stderr.trim()) + // Try to get merge-base (cached). Orphan branches will fail here. + let merge_base = match self.merge_base(target, branch) { + Ok(base) => base, + Err(e) => { + // Check if it's an orphan branch (no common ancestor) + let msg = e.to_string(); + if msg.contains("no merge base") || msg.contains("Not a valid commit") { + return Ok(true); // Orphan branches have unique changes + } + return Err(e); } - } + }; + + // git diff --name-only merge_base..branch shows files changed from merge-base to branch + let range = format!("{merge_base}..{branch}"); + let output = self.run_command(&["diff", "--name-only", &range])?; + Ok(!output.trim().is_empty()) } /// Count commits between base and head. @@ -1041,21 +1104,30 @@ impl Repository { /// /// Returns (ahead, behind) where ahead is commits in head not in base, /// and behind is commits in base not in head. + /// + /// Uses `merge_base()` internally (which is cached) to compute the common + /// ancestor, then counts commits using two-dot syntax. This allows the + /// merge-base result to be reused across multiple operations. pub fn ahead_behind(&self, base: &str, head: &str) -> anyhow::Result<(usize, usize)> { - // Use single git call with --left-right --count for better performance - let range = format!("{}...{}", base, head); - let output = self.run_command(&["rev-list", "--left-right", "--count", &range])?; - - // Parse output: "\t" format - // Example: "5\t3" means 5 commits behind, 3 commits ahead - // git rev-list --left-right outputs left (base) first, then right (head) - let (behind_str, ahead_str) = output + // Get merge-base (cached in shared repo cache) + let merge_base = self.merge_base(base, head)?; + + // Count commits using two-dot syntax (faster when merge-base is cached) + // ahead = commits in head but not in merge_base + // behind = commits in base but not in merge_base + let ahead_output = + self.run_command(&["rev-list", "--count", &format!("{}..{}", merge_base, head)])?; + let behind_output = + self.run_command(&["rev-list", "--count", &format!("{}..{}", merge_base, base)])?; + + let ahead: usize = ahead_output .trim() - .split_once('\t') - .ok_or_else(|| anyhow::anyhow!("Unexpected rev-list output format: {}", output))?; - - let behind: usize = behind_str.parse().context("Failed to parse behind count")?; - let ahead: usize = ahead_str.parse().context("Failed to parse ahead count")?; + .parse() + .context("Failed to parse ahead count")?; + let behind: usize = behind_output + .trim() + .parse() + .context("Failed to parse behind count")?; Ok((ahead, behind)) } @@ -1249,13 +1321,20 @@ impl Repository { } } - /// Get line diff statistics between two refs (using three-dot diff for merge base). + /// Get line diff statistics between two refs. /// + /// Uses merge-base (cached) to find common ancestor, then two-dot diff + /// to get the stats. This allows the merge-base result to be reused + /// across multiple operations. pub fn branch_diff_stats(&self, base: &str, head: &str) -> anyhow::Result { // Limit concurrent diff operations to reduce mmap thrash on pack files let _guard = super::HEAVY_OPS_SEMAPHORE.acquire(); - let range = format!("{}...{}", base, head); + // Get merge-base (cached in shared repo cache) + let merge_base = self.merge_base(base, head)?; + + // Use two-dot syntax with the cached merge-base + let range = format!("{}..{}", merge_base, head); let stdout = self.run_command(&["diff", "--numstat", &range])?; LineDiff::from_numstat(&stdout) } @@ -1397,7 +1476,7 @@ impl Repository { local_branches.iter().map(|(n, _)| n.clone()).collect(); // Get remote branches with timestamps - let remote = self.primary_remote().unwrap_or("origin"); + let remote = self.primary_remote().unwrap_or_else(|_| "origin".into()); let remote_ref_path = format!("refs/remotes/{}/", remote); let remote_prefix = format!("{}/", remote); @@ -1471,9 +1550,29 @@ impl Repository { } /// Get the merge base between two commits. + /// + /// Results are cached in the shared repo cache to avoid redundant git commands + /// when multiple tasks need the same merge-base (e.g., parallel `wt list` tasks). + /// The cache key is normalized (sorted) since merge-base(A, B) == merge-base(B, A). pub fn merge_base(&self, commit1: &str, commit2: &str) -> anyhow::Result { - let output = self.run_command(&["merge-base", commit1, commit2])?; - Ok(output.trim().to_owned()) + // Normalize key order since merge-base is symmetric: merge-base(A, B) == merge-base(B, A) + let key = if commit1 <= commit2 { + (commit1.to_string(), commit2.to_string()) + } else { + (commit2.to_string(), commit1.to_string()) + }; + + let cache = get_cache(self.compute_git_common_dir()?); + + Ok(cache + .merge_base + .entry(key) + .or_insert_with(|| { + self.run_command(&["merge-base", commit1, commit2]) + .map(|output| output.trim().to_owned()) + .unwrap_or_default() + }) + .clone()) } /// Check if merging head into base would result in conflicts. @@ -1658,7 +1757,7 @@ impl Repository { /// Returns the refreshed default branch name. pub fn refresh_default_branch(&self) -> anyhow::Result { let remote = self.primary_remote()?; - let branch = self.query_remote_default_branch(remote)?; + let branch = self.query_remote_default_branch(&remote)?; // Update worktrunk's cache let _ = self.run_command(&["config", "worktrunk.default-branch", &branch]); Ok(branch) @@ -1735,15 +1834,16 @@ impl Repository { /// This identifier is used to track which commands have been approved /// for execution in this project. /// - /// Result is cached for the lifetime of this Repository instance. - pub fn project_identifier(&self) -> anyhow::Result<&str> { - self.cache + /// Result is cached in the shared repo cache (shared across all worktrees). + pub fn project_identifier(&self) -> anyhow::Result { + let cache = get_cache(self.compute_git_common_dir()?); + cache .project_identifier .get_or_try_init(|| { // Try to get the remote URL first let remote = self.primary_remote()?; - if let Some(url) = self.remote_url(remote) { + if let Some(url) = self.remote_url(&remote) { if let Some(parsed) = GitRemoteUrl::parse(url.trim()) { return Ok(parsed.project_identifier()); } @@ -1777,15 +1877,16 @@ impl Repository { Ok(repo_name.to_string()) }) - .map(String::as_str) + .cloned() } /// Load the project configuration (.config/wt.toml) if it exists. /// - /// Result is cached for the lifetime of this Repository instance. + /// Result is cached in the global shared cache (same config for all worktrees). /// Returns `None` if not in a worktree or if no config file exists. pub fn load_project_config(&self) -> anyhow::Result> { - self.cache + let cache = get_cache(self.compute_git_common_dir()?); + cache .project_config .get_or_try_init(|| { match self.worktree_root() { diff --git a/src/llm.rs b/src/llm.rs index db545998a..c529069d6 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -514,7 +514,7 @@ pub fn build_commit_prompt(config: &CommitGenerationConfig) -> anyhow::Result anyhow::Result