WIP Cache GetWorktreePath result to avoid repeated git commands#473
WIP Cache GetWorktreePath result to avoid repeated git commands#473
Conversation
Uses the same cwd-keyed caching pattern as paths.RepoRoot() to eliminate redundant `git rev-parse --show-toplevel` calls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: c79e31160fd3
PR SummaryLow Risk Overview Adds Written by Cursor Bugbot for commit c04fc78. Configure here. |
There was a problem hiding this comment.
Pull request overview
This PR adds a working-directory-keyed cache for strategy.GetWorktreePath() to reduce repeated git rev-parse --show-toplevel subprocess calls, following the same caching approach used elsewhere in the CLI for git path discovery.
Changes:
- Add an RWMutex-protected cache for
GetWorktreePath()keyed by current working directory. - Add
ClearWorktreePathCache()helper intended for tests that change directories.
cmd/entire/cli/strategy/common.go
Outdated
| cwd, err := os.Getwd() //nolint:forbidigo // needed for cache key | ||
| if err != nil { | ||
| cwd = "" | ||
| } |
There was a problem hiding this comment.
If os.Getwd() fails, cwd is set to "" and the function can still populate/consult the cache under an empty key. That can return a stale worktree path in later calls where Getwd continues to fail (e.g., deleted/unreadable CWD), potentially pointing operations at the wrong repo. Consider skipping both cache read and cache write when Getwd fails (treat it as an uncachable call), or return an explicit error instead of using "" as a cache key.
cmd/entire/cli/strategy/common.go
Outdated
| // worktreePathCache caches the worktree path to avoid repeated git commands. | ||
| // The cache is keyed by the current working directory to handle directory changes. | ||
| var ( | ||
| worktreePathMu sync.RWMutex | ||
| worktreePathCache string | ||
| worktreePathCacheDir string | ||
| ) |
There was a problem hiding this comment.
This introduces a second, independent cache for git rev-parse --show-toplevel even though paths.RepoRoot() already implements the same cwd-keyed caching and is already used in this package (e.g., OpenRepository). To avoid duplicated logic/state (and avoid running the same git command once per cache), consider implementing GetWorktreePath in terms of paths.RepoRoot (wrapping the error message if needed) and removing this separate cache.
| // GetWorktreePath returns the absolute path to the current worktree root. | ||
| // This is the working directory path, not the git directory. | ||
| // The result is cached per working directory. | ||
| func GetWorktreePath() (string, error) { | ||
| cwd, err := os.Getwd() //nolint:forbidigo // needed for cache key | ||
| if err != nil { | ||
| cwd = "" | ||
| } | ||
|
|
||
| // Check cache with read lock first | ||
| worktreePathMu.RLock() | ||
| if worktreePathCache != "" && worktreePathCacheDir == cwd { | ||
| cached := worktreePathCache | ||
| worktreePathMu.RUnlock() | ||
| return cached, nil | ||
| } | ||
| worktreePathMu.RUnlock() | ||
|
|
||
| // Cache miss - get worktree path and update cache with write lock | ||
| ctx := context.Background() | ||
| cmd := exec.CommandContext(ctx, "git", "rev-parse", "--show-toplevel") | ||
| output, err := cmd.Output() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get worktree path: %w", err) | ||
| } | ||
| return strings.TrimSpace(string(output)), nil | ||
|
|
||
| result := strings.TrimSpace(string(output)) | ||
|
|
||
| worktreePathMu.Lock() | ||
| worktreePathCache = result | ||
| worktreePathCacheDir = cwd | ||
| worktreePathMu.Unlock() | ||
|
|
||
| return result, nil | ||
| } |
There was a problem hiding this comment.
The new caching behavior in GetWorktreePath isn't covered by tests. Since this file already has unit tests, it would be useful to add one that proves the second call is served from cache (e.g., call once in a temp repo, then modify PATH so invoking git would fail, and verify the second call still succeeds; use ClearWorktreePathCache for isolation).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
cmd/entire/cli/strategy/common.go
Outdated
| worktreePathMu.Lock() | ||
| worktreePathCache = "" | ||
| worktreePathCacheDir = "" | ||
| worktreePathMu.Unlock() |
There was a problem hiding this comment.
Exported ClearWorktreePathCache function is never called anywhere
Low Severity
ClearWorktreePathCache is a new exported function that is never called anywhere in the codebase. The analogous ClearRepoRootCache is called in 6 places across benchmarks and tests, but ClearWorktreePathCache has zero callers. If any benchmarks or tests change directories and then invoke code paths that call GetWorktreePath, the stale cache won't be cleared, potentially leading to subtle test failures in the future.
Addresses three review findings: - Removes separate cwd-keyed cache that could return stale results when os.Getwd() fails (empty-key bug) - Eliminates duplicate cache/git-command since both functions run the same `git rev-parse --show-toplevel` - Adds tests proving cache behavior and correct worktree resolution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f4df4a730588
evisdren
changed the title
1 participant
Uses the same cwd-keyed caching pattern as paths.RepoRoot() to eliminate redundant
git rev-parse --show-toplevelcalls.Entire-Checkpoint: c79e31160fd3