Conversation
Entire-Checkpoint: 270d6b69d4fe
PR SummaryMedium Risk Overview Adds a new Written by Cursor Bugbot for commit f413d34. Configure here. |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the entire status command by replacing subprocess calls with direct filesystem reads and adding caching for git operations. The optimization targets two hot paths: resolveWorktreeBranch() (which previously spawned a git subprocess per worktree) and getGitCommonDir() (which now caches results per working directory). The PR also introduces a comprehensive benchmark infrastructure (benchutil package) to measure and track performance improvements, and updates Discord invite links in CONTRIBUTING.md.
Changes:
- Replaced
resolveWorktreeBranch()subprocess call with direct filesystem reads of.git/HEAD - Added caching to
getGitCommonDir()to avoid repeated git subprocess invocations - Introduced
benchutilpackage with test fixtures for realistic git repositories, transcripts, and session states - Added benchmark tasks to mise.toml and comprehensive benchmarks for status command and checkpoint operations
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/status.go | Rewrote resolveWorktreeBranch to read .git/HEAD directly from filesystem instead of spawning git subprocess |
| cmd/entire/cli/status_test.go | Added comprehensive tests for resolveWorktreeBranch covering regular repos, detached HEAD, worktrees with absolute/relative paths |
| cmd/entire/cli/session/state.go | Added caching to getGitCommonDir following the established RepoRoot pattern with RWMutex and per-directory cache |
| cmd/entire/cli/session/state_test.go | Added tests for git common dir caching behavior including cache hits, invalidation, and directory changes |
| cmd/entire/cli/setup_test.go | Added ClearGitCommonDirCache() call to test setup for proper isolation |
| cmd/entire/cli/bench_test.go | Added end-to-end benchmarks for status command with/without caching to measure optimization impact |
| cmd/entire/cli/benchutil/benchutil.go | New package providing test fixtures (repos, sessions, transcripts) for benchmark tests |
| cmd/entire/cli/benchutil/benchutil_test.go | Benchmarks for the benchutil helpers themselves |
| cmd/entire/cli/checkpoint/bench_test.go | Added benchmarks for WriteTemporary and WriteCommitted hot paths with various scenarios |
| mise.toml | Added bench, bench:cpu, and bench:mem tasks for running benchmarks with profiling |
| CONTRIBUTING.md | Updated Discord invite link |
Comments suppressed due to low confidence (4)
mise.toml:104
- There are several typos in the echo message. "find." should be "find ." (with space), and "/path/to/.cpu.prof" should be "/path/to/cpu.prof" (without the dot prefix).
run = "go test -bench=. -benchmem -run='^$' -cpuprofile=cpu.prof -timeout=10m ./... && echo 'CPU Profiles saved as cpu.prof in each benchmarked package directory. List them with : find. -name cpu.prof -print. View one with: go tool pprof -http=:8080 /path/to/.cpu.prof'"
mise.toml:108
- There's a typo in the echo message: "List with them:" should be "List them with:". Also, the path should be "/path/to/mem.prof" not with extra space.
run = "go test -bench=. -benchmem -run='^$' -memprofile=mem.prof -timeout=10m ./... && echo 'Memory profiles saved as mem.prof in each benchmarked package directory. List with them: find . -name mem.prof -print. View one with: go tool pprof -http=:8080 /path/to/mem.prof'"
cmd/entire/cli/bench_test.go:26
- The comment on line 26 states that resolveWorktreeBranch uses "git rev-parse --abbrev-ref HEAD" subprocess call, but this is no longer accurate after the optimization. The function now reads the HEAD file directly from the filesystem without spawning a subprocess. Consider updating this comment to reflect the current implementation, such as: "Filesystem read of .git/HEAD (resolveWorktreeBranch, per unique worktree)".
// - git rev-parse --abbrev-ref HEAD (resolveWorktreeBranch, per unique worktree)
cmd/entire/cli/benchutil/benchutil_test.go:97
- The code uses
b.Loop()which is a Go 1.24+ feature for benchmark loops. While this is valid for Go 1.25.6 (as specified in mise.toml and go.mod), most other benchmarks in this PR use the standardfor range b.Npattern (see bench_test.go and checkpoint/bench_test.go). Consider using the consistent patternfor range b.Nfor uniformity across the codebase, unless there's a specific reason to use the newer Loop() API.
func BenchmarkNewBenchRepo(b *testing.B) {
for b.Loop() {
NewBenchRepo(b, RepoOpts{})
}
}
func BenchmarkNewBenchRepo_Large(b *testing.B) {
for b.Loop() {
NewBenchRepo(b, RepoOpts{
FileCount: 50,
FileSizeLines: 500,
})
}
}
func BenchmarkSeedShadowBranch(b *testing.B) {
for b.Loop() {
b.StopTimer()
repo := NewBenchRepo(b, RepoOpts{FileCount: 10})
sessionID := repo.CreateSessionState(b, SessionOpts{})
b.StartTimer()
repo.SeedShadowBranch(b, sessionID, 5, 3)
}
}
func BenchmarkSeedMetadataBranch(b *testing.B) {
for b.Loop() {
b.StopTimer()
repo := NewBenchRepo(b, RepoOpts{FileCount: 10})
b.StartTimer()
repo.SeedMetadataBranch(b, 10)
}
}
func BenchmarkGenerateTranscript(b *testing.B) {
b.Run("Small_20msg", func(b *testing.B) {
for b.Loop() {
GenerateTranscript(TranscriptOpts{
MessageCount: 20,
AvgMessageBytes: 500,
})
}
})
b.Run("Medium_200msg", func(b *testing.B) {
for b.Loop() {
GenerateTranscript(TranscriptOpts{
MessageCount: 200,
AvgMessageBytes: 500,
})
}
})
b.Run("Large_2000msg", func(b *testing.B) {
for b.Loop() {
GenerateTranscript(TranscriptOpts{
MessageCount: 2000,
AvgMessageBytes: 500,
})
}
})
b.Run("WithToolUse", func(b *testing.B) {
files := []string{"src/main.go", "src/util.go", "src/handler.go"}
for b.Loop() {
GenerateTranscript(TranscriptOpts{
MessageCount: 200,
AvgMessageBytes: 500,
IncludeToolUse: true,
FilesTouched: files,
})
}
})
}
func BenchmarkCreateSessionState(b *testing.B) {
repo := NewBenchRepo(b, RepoOpts{FileCount: 10})
b.ResetTimer()
for b.Loop() {
repo.CreateSessionState(b, SessionOpts{
Phase: session.PhaseActive,
StepCount: 5,
FilesTouched: []string{"src/file_000.go", "src/file_001.go", "src/file_002.go"},
})
}
}
Mainly for posterity:" |
| ref := strings.TrimSpace(string(data)) | ||
|
|
||
| // Symbolic ref: "ref: refs/heads/<branch>" | ||
| if strings.HasPrefix(ref, "ref: refs/heads/") { |
There was a problem hiding this comment.
If the Git repository is using refStorage=reftable, HEAD will point to ref: refs/heads/.invalid, instead of the correct branch. In that specific case, it may be worth shelling out to git.
Refs:
There was a problem hiding this comment.
good call out - updated to support this case
Replaces shelling out to a git process by using a pure go function to find the HEAD branch name. Shaves another 10ms off
entire status.