Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 43 additions & 18 deletions .claude/rules/caching-strategy.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<T>`
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<String> {
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<bool> {
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<String> {
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
})
}
```
15 changes: 15 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion src/commands/command_approval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
2 changes: 1 addition & 1 deletion src/commands/command_executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/commands/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1025,7 +1025,7 @@ pub fn handle_state_get(key: &str, refresh: bool, branch: Option<String>) -> 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)?;
Expand Down
3 changes: 1 addition & 2 deletions src/commands/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
12 changes: 6 additions & 6 deletions src/commands/hook_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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 {
Expand Down Expand Up @@ -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"))?;
Expand All @@ -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!(
Expand Down Expand Up @@ -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(),
)?;
Expand Down
4 changes: 2 additions & 2 deletions src/commands/list/ci_status.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
};

Expand Down Expand Up @@ -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;
};

Expand Down
15 changes: 8 additions & 7 deletions src/commands/list/collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down
11 changes: 7 additions & 4 deletions src/commands/list/collect_progressive_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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 },
Expand Down Expand Up @@ -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| {
Expand Down
4 changes: 2 additions & 2 deletions src/commands/step_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand All @@ -319,7 +319,7 @@ pub fn step_show_squash_prompt(
&target_branch,
&merge_base,
&subjects,
current_branch,
&current_branch,
repo_name,
config,
)?;
Expand Down
4 changes: 2 additions & 2 deletions src/commands/worktree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(_) => {
Expand Down Expand Up @@ -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 {
Expand Down
Loading