diff --git a/docs/internal/caching.md b/docs/internal/caching.md index b76308c8..15c20dad 100644 --- a/docs/internal/caching.md +++ b/docs/internal/caching.md @@ -14,11 +14,32 @@ is a bug factory. | Cache | Key | Value | Invalidator | Bound | |---|---|---|---|---| | `repos/git.AheadBehindCached` | (repo_id, base_oid, head_oid) | (ahead, behind) | OID change ⇒ different key; LRU eviction | 4096 entries | +| `repo/treecache` last-commits | (repo_id, commit_oid, subpath) | basename → last `Commit` | OID change ⇒ different key; 10 min TTL; LRU | 2048 entries | +| `repo/treecache` commit count | (repo_id, commit_oid) | `rev-list --count` int | same | 2048 entries | +| `repo/treecache` languages | (repo_id, commit_oid) | language → bytes (from `ls-tree -r`) | same | 512 entries | +| `repo/treecache` contributors | (repo_id, commit_oid) | author tally (from `log -n 500`) | same | 512 entries | Concrete uses: - `branchesList` (S20 deferral H4) — replaces N `git rev-list` invocations per page load with one cached lookup per branch. Single-flight collapses concurrent misses on hot branches. +- The repo home / code tab (`internal/web/handlers/repo/treecache`, + built once in `internal/web/repo_wiring.go`, threaded through + `repo.Deps.TreeCache`). It is the most-crawled page on the site and + `meta-externalagent` walks it anonymously; before this cache one + cold render of an 81-entry directory forked git 90 times. It now + forks 10 times cold and 6 warm, independent of entry count. See + `docs/internal/code-tab.md`. + +All four `treecache` entries are keyed on the **rendered commit OID**, +so there is no invalidation hook to wire and none to forget: a push +moves the ref, the OID changes, the new key misses, and the pre-push +entries age out on TTL and LRU eviction. The 10-minute TTL exists to +release memory from repos that stop being visited, not for +correctness. The heavier language and contributor caches take +capacity/4 slots because their values are not uniformly sized. +Sizing rationale and worst-case memory live in the package doc +comment. ## Planned caches (next iterations) @@ -27,7 +48,6 @@ back grow large enough to bench-justify the cache. | Cache | Key | Value | Invalidator | |---|---|---|---| -| Tree at root | (repo_id, ref_oid) | rendered ls-tree result | push:process bumps default-OID | | Ref list | (repo_id) | branches + tags | push:process | | File list (finder) | (repo_id, ref_oid) | flat path slice | push:process | | Default-branch OID | (repo_id) | OID string | push:process + default-branch swap | diff --git a/docs/internal/code-tab.md b/docs/internal/code-tab.md index ea5a1f9a..537ada9d 100644 --- a/docs/internal/code-tab.md +++ b/docs/internal/code-tab.md @@ -125,11 +125,50 @@ default Code tab so independently-created mirrors don't produce dead links. Unknown, external, absent, or malformed remotes stay as plain `name @ shortsha` rows. -The S17 ship excludes the htmx-driven "last commit per entry" column -that the spec describes — an extra round-trip we can add later without -a schema change. The current page renders the listing immediately. -**Deferred to S18 (commits-per-entry)** — the spec calls out this -deferral path; the tree template has the column slot ready. +### Last commit per entry + +Every row carries the most recent commit that touched it. The first +implementation ran one `git log -1 -- ` per entry, serially, so +a 100-entry directory forked git 100 times for a single anonymous +page view — and the repo home is the most-crawled page on the site. + +It is now **one** invocation for the whole directory +(`repogit.EntryLastCommits`, `internal/repos/git/lastcommit.go`): + +``` +git log --max-count=2000 --name-only --no-renames \ + --format=%H%h%an%ae%at%s [-- ] +``` + +read in reverse-chronological order off a pipe. The first time a path +under `dir//` appears, ``'s answer is that commit. Once +every listed entry has an answer the walk kills git mid-stream rather +than draining the rest of history, so the common case reads only as +far back as the directory's least-recently-touched entry. + +Two escape hatches keep the rendered output byte-identical to the +N-fork version: + +- **The 2,000-commit bound.** An entry untouched inside the bound + comes back unresolved and the handler runs the old per-path + `git log -1` for exactly that entry. +- **Quoted paths.** Git quotes paths containing a newline or a double + quote even under `core.quotePath=false`; those never compare equal + to an `ls-tree` basename, so they too fall through to the per-path + query. + +Merge commits emit no file list under `--name-only`, which matches +what `git log -1 -- ` reports anyway: the attribution lands on +the side-branch commit that actually changed the file. + +Measured on the handler test fixture +(`code_tree_forks_test.go`), one cold anonymous render of the root +tree: + +| Directory | Before | After (cold) | After (warm cache) | +|---|---|---|---| +| 6 entries | 15 forks | 10 forks | 6 forks | +| 81 entries | 90 forks | 10 forks | 6 forks | ## Check status indicators @@ -262,17 +301,58 @@ the floor S17 commits to. ## Caching -Currently **no caching layer**. Every request runs `git for-each-ref`, -`git ls-tree`, etc. That's fine for small-to-medium repos; the cost -shows up on big repos with deep trees. The S17 spec proposes a cache -keyed on `(repo_id, ref_oid, dir_path)` invalidated on push (S14's -`push:process` job is the right invalidation hook). - -**Deferred** — the cache is purely performance polish. When we hit a -real-world repo where it matters, wire it in: file `internal/cache/` -plus a callback in `worker/jobs/push_process.go`. The handlers already -take a per-request `policy.Cache` so adding a per-process git cache is -mechanically straightforward. +`internal/web/handlers/repo/treecache` is the in-process cache behind +the repo home / code tab. One `*treecache.Cache` per process, built in +`internal/web/repo_wiring.go` and threaded through +`repo.Deps.TreeCache` — the same shape as `httpcache.PageCache`. It +memoizes the four git reads a tree render performs whose answers +depend only on the commit being rendered: + +| Value | Key | Replaces | +|---|---|---| +| basename → last commit | (repo_id, commit_oid, subpath) | the single `log --name-only` walk | +| commit count | (repo_id, commit_oid) | `rev-list --count` | +| language → bytes | (repo_id, commit_oid) | recursive `ls-tree -r` | +| author tally | (repo_id, commit_oid) | `log -n 500` | + +**Invalidation is structural, not hooked.** Every key carries the +rendered commit OID, so a push produces a different key; the pre-push +entries are never served again and age out on the 10-minute TTL or LRU +eviction. Nothing in `push:process` has to remember to call anything. +Sizes: 2,048 entries for the two cheap caches, 512 for the two heavier +ones. `nil` disables the cache entirely (tests, degraded boot) and +every read simply falls through to git. + +Only the *derived* values are cached, never the raw walk output: the +recursive `ls-tree -r` on a large repo is megabytes of path text but +reduces to a handful of language byte counts, and the 500-commit +contributor walk reduces to one row per distinct author. Identity +resolution stays per-request — it reads the users table, and its +answer can change without any git ref moving. + +Two things are deliberately still uncached per request: `for-each-ref` +(the ref list is what resolves the URL in the first place, so there is +no OID to key on yet) and `ls-tree` for the directory itself (cheap, +and it is what produces the entry names the other caches are keyed +against). + +### Read deadlines + +Every read-only git invocation on these paths runs under +`repogit.ReadTimeout` (30 s, `internal/repos/git/exec.go`). +`context.WithTimeout` keeps the earlier of the two deadlines, so a +tighter request deadline still wins. Blob *streaming* is excluded — +its duration is bounded by the client's download speed, not by git. +Pack/transport (`internal/git/protocol`, `handlers/githttp`) is a +separate path and is untouched. + +### Counting git forks + +`repogit.ForkCount()` is a process-wide counter incremented by the one +helper every subprocess in `internal/repos/git` goes through. It is +the measurement lever for this page: tests read it before and after a +request and assert the delta is constant in the entry count rather +than linear in it. ## Pitfalls + protections @@ -302,12 +382,6 @@ mechanically straightforward. These items are spec deliverables we ship in a later pass: -* **Last-commit-per-entry column** with htmx lazy load and pre-walked - `git log --name-status` cache → wire into S18 (commit history) where - the same walk powers the per-file history page. -* **Tree caching keyed on (repo_id, ref_oid, dir_path)**, push-event - invalidation → wire into S36 (performance pass) once we have a real - workload to measure. * **Pagination at 1000 entries per directory** → cosmetic for huge trees; add when someone hits `node_modules`-grade inflation. * **Encoding detection for non-UTF-8 source files** → file reads are diff --git a/docs/internal/retro/2026-09-02-availability-sitrep.md b/docs/internal/retro/2026-09-02-availability-sitrep.md index 008036a3..454efcbb 100644 --- a/docs/internal/retro/2026-09-02-availability-sitrep.md +++ b/docs/internal/retro/2026-09-02-availability-sitrep.md @@ -163,9 +163,18 @@ The verification items below are still the operator's. bulk DELETE in a migration. 0129 adds the `day` indexes the purge needs (`repo_traffic_uniques` reuses its existing `created_at` index). See `docs/internal/repository-insights.md`. -- [ ] Cache per-entry last-commit for the code tab (single - `git log --name-only` walk, or an LRU keyed by tree OID) and - cache `rev-list --count` / recursive `ls-tree` per head OID +- [x] Cache per-entry last-commit for the code tab — one streamed + `git log --name-only` walk per directory + (`repogit.EntryLastCommits`) replacing one `git log -1` per + entry, plus an OID-keyed LRU + (`internal/web/handlers/repo/treecache`) over that, the + `rev-list --count`, the `ls-tree -r` language aggregate and the + `log -n 500` contributor tally. Measured on the handler test + fixture: a cold root-tree render of an 81-entry directory went + **90 git forks → 10**, and 6 warm; a 6-entry directory 15 → 10. + Fork counts are now constant in the entry count + (`repogit.ForkCount()` + `code_tree_forks_test.go`). Read-only + git calls on these paths also gained a 30 s deadline. - [x] `actionsobserver`: the `octet_length` sum now runs every 5 min on its own cadence; the count and queue-depth gauges stay at 15 s diff --git a/internal/repos/git/blame.go b/internal/repos/git/blame.go index 2cf7fcc8..49f528e0 100644 --- a/internal/repos/git/blame.go +++ b/internal/repos/git/blame.go @@ -7,7 +7,6 @@ import ( "context" "errors" "fmt" - "os/exec" "strconv" "strings" "time" @@ -83,7 +82,7 @@ func Blame(ctx context.Context, gitDir string, opts BlameOptions) ([]BlameChunk, return nil, ErrBlameTooLarge } - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + cmd := gitCmd(ctx, "-C", gitDir, "blame", "--line-porcelain", opts.Ref, "--", opts.Path) stdout, err := cmd.StdoutPipe() if err != nil { diff --git a/internal/repos/git/branchops.go b/internal/repos/git/branchops.go index 628e83a6..08fbc011 100644 --- a/internal/repos/git/branchops.go +++ b/internal/repos/git/branchops.go @@ -19,7 +19,7 @@ import ( // When base or head doesn't exist on the repo we surface the typed // ErrRefNotFound so callers can render "—" instead of a number. func AheadBehind(ctx context.Context, gitDir, base, head string) (ahead, behind int, err error) { - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + cmd := gitCmd(ctx, "-C", gitDir, "rev-list", "--left-right", "--count", base+"..."+head) out, runErr := cmd.Output() if runErr != nil { @@ -50,8 +50,8 @@ func CommitsBetween(ctx context.Context, gitDir, base, head string, max int) ([] const sep = "\x1f" const recordEnd = "\x1e" format := strings.Join([]string{"%H", "%h", "%an", "%ae", "%at", "%s"}, sep) + sep + "%b" + recordEnd - cmd := exec.CommandContext( - ctx, "git", "-C", gitDir, + cmd := gitCmd( + ctx, "-C", gitDir, "log", "--max-count="+strconv.Itoa(max), "--format="+format, @@ -78,7 +78,7 @@ func CommitsBetween(ctx context.Context, gitDir, base, head string, max int) ([] // passing it gives git's exact-match semantics (no off-by-one // races even when oldvalue happens to equal newvalue). func UpdateRefCAS(ctx context.Context, gitDir, ref, newOID, oldOID string) error { - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + cmd := gitCmd(ctx, "-C", gitDir, "update-ref", ref, newOID, oldOID) out, err := cmd.CombinedOutput() if err == nil { @@ -107,7 +107,7 @@ func DeleteBranch(ctx context.Context, gitDir, branch, oldOID string) error { if branch == "" || strings.HasPrefix(branch, "-") { return ErrRefNotFound } - check := exec.CommandContext(ctx, "git", "-C", gitDir, "check-ref-format", "--branch", branch) + check := gitCmd(ctx, "-C", gitDir, "check-ref-format", "--branch", branch) if out, err := check.CombinedOutput(); err != nil { return fmt.Errorf("check-ref-format %s: %w (%s)", branch, err, strings.TrimSpace(string(out))) } @@ -116,7 +116,7 @@ func DeleteBranch(ctx context.Context, gitDir, branch, oldOID string) error { if strings.TrimSpace(oldOID) != "" { args = append(args, oldOID) } - cmd := exec.CommandContext(ctx, "git", args...) + cmd := gitCmd(ctx, args...) out, err := cmd.CombinedOutput() if err == nil { return nil @@ -143,7 +143,7 @@ func DeleteBranch(ctx context.Context, gitDir, branch, oldOID string) error { // refspec just updates the dst ref. func FetchIntoNamespace(ctx context.Context, dstRepoDir, srcRepoDir, srcRef, dstRef string) error { refspec := srcRef + ":" + dstRef - cmd := exec.CommandContext(ctx, "git", "-C", dstRepoDir, + cmd := gitCmd(ctx, "-C", dstRepoDir, "fetch", "--quiet", "--no-tags", srcRepoDir, refspec) if out, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("fetch %s into %s: %w (%s)", srcRef, dstRef, err, strings.TrimSpace(string(out))) @@ -155,7 +155,7 @@ func FetchIntoNamespace(ctx context.Context, dstRepoDir, srcRepoDir, srcRef, dst // Used by the pre-receive force-push detector: a fast-forward is // `IsAncestor(old, new)`. func IsAncestor(ctx context.Context, gitDir, a, b string) (bool, error) { - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + cmd := gitCmd(ctx, "-C", gitDir, "merge-base", "--is-ancestor", a, b) err := cmd.Run() if err == nil { @@ -174,7 +174,7 @@ func IsAncestor(ctx context.Context, gitDir, a, b string) (bool, error) { // SetSymbolicRef updates HEAD (or any other symbolic ref) atomically. // Used by the default-branch change to point HEAD at the new branch. func SetSymbolicRef(ctx context.Context, gitDir, ref, target string) error { - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + cmd := gitCmd(ctx, "-C", gitDir, "symbolic-ref", ref, target) if out, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("symbolic-ref %s -> %s: %w (%s)", ref, target, err, out) diff --git a/internal/repos/git/exec.go b/internal/repos/git/exec.go new file mode 100644 index 00000000..6a966d4d --- /dev/null +++ b/internal/repos/git/exec.go @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package git + +import ( + "context" + "os/exec" + "sync/atomic" + "time" +) + +// Every git subprocess this package spawns goes through gitCmd so we +// have exactly one place that (a) counts forks and (b) can grow +// process-wide policy later (nice level, env scrubbing, tracing). +// +// The counter is the measurement lever for the code-tab work: a page +// that forks git O(entries) times shows up as a linear ForkCount +// delta, and the tests assert the delta is constant instead. +var forkCount atomic.Uint64 + +// ForkCount reports the cumulative number of git subprocesses this +// process has started through this package. It only ever increases; +// callers that want a per-operation number read it before and after +// and subtract. Exported for tests and the /metrics surface. +// +// Note the pack/transport paths (internal/git/protocol, +// internal/web/handlers/githttp) run their own `git upload-pack` / +// `receive-pack` processes and are deliberately NOT counted here — +// they are long-lived streams, not the short read forks this counter +// is about. +func ForkCount() uint64 { return forkCount.Load() } + +// ReadTimeout bounds a single read-only git invocation on a request +// path. Reads on this side of the package are local-disk object +// lookups: `ls-tree`, `rev-list --count`, `log`, `cat-file`. On the +// production box the slowest of these is a full recursive `ls-tree` +// on the largest repo, measured in tens of milliseconds; 30s is three +// orders of magnitude of headroom and exists purely so a wedged or +// pathological invocation cannot pin a request goroutine (and its +// git subprocess) forever when a crawler is walking every SHA. +// +// Blob streaming (StreamBlob) is deliberately excluded: its duration +// is bounded by the client's download speed, not by git. +const ReadTimeout = 30 * time.Second + +// gitCmd builds a git subprocess rooted at args and bumps ForkCount. +func gitCmd(ctx context.Context, args ...string) *exec.Cmd { + forkCount.Add(1) + // gitDir is RepoFS-validated at every call site and every + // user-controlled value is an argv element, never a shell string. + return exec.CommandContext(ctx, "git", args...) +} + +// readCtx derives a deadline-bounded context for a read-only git +// invocation. context.WithTimeout keeps the earlier of the two +// deadlines, so a caller with a tighter request deadline still wins. +// Callers must defer the returned cancel. +func readCtx(ctx context.Context) (context.Context, context.CancelFunc) { + return context.WithTimeout(ctx, ReadTimeout) +} diff --git a/internal/repos/git/lastcommit.go b/internal/repos/git/lastcommit.go new file mode 100644 index 00000000..cac1c51e --- /dev/null +++ b/internal/repos/git/lastcommit.go @@ -0,0 +1,228 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package git + +import ( + "bufio" + "bytes" + "context" + "fmt" + "strconv" + "strings" + "time" +) + +// DefaultLastCommitWalk bounds how many commits EntryLastCommits will +// walk before giving up on whatever is still unresolved. 2,000 commits +// of `git log --name-only` output streams in single-digit milliseconds +// on the boxes we run and covers the entire history of most +// directories; anything deeper is a directory whose files have not +// been touched in 2,000 commits, and the caller resolves those few +// stragglers with one targeted `git log -1 -- ` each. +const DefaultLastCommitWalk = 2000 + +// LastCommitOptions describes one "last commit per entry" resolution. +// +// Dir is the repo-relative directory being listed ("" for the root) +// and Names are its immediate children as `ls-tree` reported them +// (basenames, not full paths). Ref is any commit-ish; callers should +// pass the resolved commit OID when they intend to cache the result, +// so the cache key moves whenever history moves. +type LastCommitOptions struct { + Ref string + Dir string + Names []string + MaxCommits int // 0 → DefaultLastCommitWalk +} + +// EntryLastCommits resolves the most recent commit touching each named +// child of Dir using ONE `git log` walk instead of one `git log -1` +// per entry. +// +// The walk is `git log --name-only [-- ]` read in reverse +// chronological order: the first time a path under `dir//` +// appears, ``'s last commit is that commit. Once every requested +// name has an answer we kill git mid-stream rather than draining the +// rest of history, so the common case reads only as far back as the +// least-recently-touched entry in the directory. +// +// The returned map holds only the names that resolved. Entries missing +// from it were either never touched inside the walk bound, or live +// under a path shape git had to quote (embedded newline/quote); the +// caller falls back to a per-path `git log -1` for exactly those and +// so keeps byte-identical output to the N-fork version. +// +// A non-nil error still returns whatever resolved before the failure — +// partial results are usable, and the caller's per-path fallback +// covers the rest. +func EntryLastCommits(ctx context.Context, gitDir string, o LastCommitOptions) (map[string]Commit, error) { + found := make(map[string]Commit, len(o.Names)) + want := make(map[string]struct{}, len(o.Names)) + for _, n := range o.Names { + if n != "" { + want[n] = struct{}{} + } + } + if len(want) == 0 { + return found, nil + } + + maxCommits := o.MaxCommits + if maxCommits <= 0 { + maxCommits = DefaultLastCommitWalk + } + ref := o.Ref + if ref == "" { + ref = "HEAD" + } + + ctx, cancel := readCtx(ctx) + defer cancel() + // A second, manually-triggered cancel so we can stop git the moment + // the last entry resolves without waiting on the read deadline. + walkCtx, stop := context.WithCancel(ctx) + defer stop() + + args := []string{ + "-C", gitDir, + // Keep non-ASCII paths verbatim so they compare equal to the + // names ls-tree handed us. + "-c", "core.quotePath=false", + "log", + "--max-count=" + strconv.Itoa(maxCommits), + "--name-only", + // Rename detection would only re-label a path we already see + // under its new name, and it costs real CPU on large commits. + "--no-renames", + "--format=" + lastCommitFormat, + ref, + } + if o.Dir != "" { + args = append(args, "--", o.Dir) + } + + cmd := gitCmd(walkCtx, args...) + var stderr bytes.Buffer + cmd.Stderr = &stderr + stdout, err := cmd.StdoutPipe() + if err != nil { + return found, err + } + if err := cmd.Start(); err != nil { + return found, err + } + + prefix := "" + if o.Dir != "" { + prefix = o.Dir + "/" + } + + var ( + cur Commit + haveCur bool + complete bool + ) + sc := bufio.NewScanner(stdout) + // Paths and subjects are short, but a pathological commit subject + // shouldn't abort the walk with ErrTooLong. + sc.Buffer(make([]byte, 0, 64*1024), 1<<20) + for sc.Scan() { + line := sc.Text() + if strings.HasPrefix(line, lastCommitRecordSep) { + cur, haveCur = parseLastCommitHeader(line[len(lastCommitRecordSep):]) + continue + } + if line == "" || !haveCur { + continue + } + name, ok := immediateChild(prefix, line) + if !ok { + continue + } + if _, wanted := want[name]; !wanted { + continue + } + found[name] = cur + delete(want, name) + if len(want) == 0 { + complete = true + break + } + } + scanErr := sc.Err() + // Kill git before reaping ONLY when we stopped reading early: it is + // then still writing into a pipe nobody drains and Wait would + // block. On a clean EOF git has already closed stdout and is on its + // way out, so cancelling would turn a successful walk into a + // spurious "context canceled". + if complete || scanErr != nil { + stop() + } + waitErr := cmd.Wait() + + if complete { + // Early exit makes git's exit status meaningless (we killed it). + return found, nil + } + if scanErr != nil { + return found, fmt.Errorf("git log --name-only: %w", scanErr) + } + if waitErr != nil { + return found, fmt.Errorf("git log --name-only: %w: %s", + waitErr, strings.TrimSpace(stderr.String())) + } + return found, nil +} + +// lastCommitRecordSep (ASCII record separator) opens every commit +// header line so the scanner can tell headers from the `--name-only` +// path lines that follow them. Fields inside the header are split on +// ASCII unit separator, matching the rest of this package. +const ( + lastCommitRecordSep = "\x1e" + lastCommitFieldSep = "\x1f" + lastCommitFormat = lastCommitRecordSep + "%H" + lastCommitFieldSep + "%h" + + lastCommitFieldSep + "%an" + lastCommitFieldSep + "%ae" + + lastCommitFieldSep + "%at" + lastCommitFieldSep + "%s" +) + +// parseLastCommitHeader unpacks one header line. Body is intentionally +// left empty — the tree listing renders subjects only, and carrying +// bodies would multiply the cached payload for no rendered output. +func parseLastCommitHeader(line string) (Commit, bool) { + parts := strings.SplitN(line, lastCommitFieldSep, 6) + if len(parts) != 6 { + return Commit{}, false + } + ts, err := strconv.ParseInt(parts[4], 10, 64) + if err != nil { + return Commit{}, false + } + return Commit{ + OID: parts[0], + ShortOID: parts[1], + AuthorName: parts[2], + AuthorEmail: parts[3], + AuthorWhen: time.Unix(ts, 0).UTC(), + Subject: parts[5], + }, true +} + +// immediateChild maps a full repo-relative path from `--name-only` to +// the name of the listed directory's immediate child that contains it. +// Paths outside the listed directory return ok=false. +func immediateChild(prefix, p string) (string, bool) { + if prefix != "" { + if !strings.HasPrefix(p, prefix) { + return "", false + } + p = p[len(prefix):] + } + if i := strings.IndexByte(p, '/'); i >= 0 { + p = p[:i] + } + if p == "" { + return "", false + } + return p, true +} diff --git a/internal/repos/git/lastcommit_test.go b/internal/repos/git/lastcommit_test.go new file mode 100644 index 00000000..b95e36be --- /dev/null +++ b/internal/repos/git/lastcommit_test.go @@ -0,0 +1,328 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package git_test + +import ( + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "strconv" + "testing" + + gitops "github.com/tenseleyFlow/shithub/internal/repos/git" +) + +// commitStep is one commit in a fixture repo's history: a set of +// path→body writes (empty body means "delete"), plus its subject. +type commitStep struct { + subject string + write map[string]string + rename [2]string +} + +// buildHistoryRepo materializes a NON-bare repo with the given commit +// sequence and returns its .git dir. A worktree is the cheap way to +// script renames and deletions; every read helper under test takes a +// gitDir and doesn't care that an index exists next to it. +func buildHistoryRepo(t *testing.T, steps []commitStep) string { + t.Helper() + dir := t.TempDir() + run := func(args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(cmd.Environ(), + "GIT_AUTHOR_NAME=Test Author", "GIT_AUTHOR_EMAIL=test@example.com", + "GIT_COMMITTER_NAME=Test Author", "GIT_COMMITTER_EMAIL=test@example.com", + "GIT_AUTHOR_DATE=2026-01-01T00:00:00Z", "GIT_COMMITTER_DATE=2026-01-01T00:00:00Z", + ) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", args, err, out) + } + } + run("init", "-q", "--initial-branch=trunk") + for _, step := range steps { + for p, body := range step.write { + writeRepoFile(t, dir, p, body) + } + if step.rename[0] != "" { + run("mv", step.rename[0], step.rename[1]) + } + run("add", "-A") + run("commit", "-q", "-m", step.subject) + } + return filepath.Join(dir, ".git") +} + +func writeRepoFile(t *testing.T, dir, rel, body string) { + t.Helper() + full := filepath.Join(dir, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(full), 0o750); err != nil { + t.Fatalf("mkdir %s: %v", filepath.Dir(full), err) + } + if err := os.WriteFile(full, []byte(body), 0o600); err != nil { + t.Fatalf("write %s: %v", full, err) + } +} + +func TestEntryLastCommits_ResolvesRootChildrenInOneWalk(t *testing.T) { + gitDir := buildHistoryRepo(t, []commitStep{ + {subject: "c1 seed", write: map[string]string{ + "README.md": "one\n", "src/a.go": "a\n", "docs/guide.md": "g\n", + }}, + {subject: "c2 touch src", write: map[string]string{"src/a.go": "aa\n"}}, + {subject: "c3 touch readme", write: map[string]string{"README.md": "two\n"}}, + }) + + before := gitops.ForkCount() + got, err := gitops.EntryLastCommits(context.Background(), gitDir, gitops.LastCommitOptions{ + Ref: "trunk", + Names: []string{"README.md", "src", "docs"}, + }) + if err != nil { + t.Fatalf("EntryLastCommits: %v", err) + } + if forks := gitops.ForkCount() - before; forks != 1 { + t.Errorf("fork count = %d, want 1", forks) + } + want := map[string]string{ + "README.md": "c3 touch readme", + "src": "c2 touch src", + "docs": "c1 seed", + } + for name, subject := range want { + c, ok := got[name] + if !ok { + t.Errorf("%s unresolved", name) + continue + } + if c.Subject != subject { + t.Errorf("%s subject = %q, want %q", name, c.Subject, subject) + } + if len(c.OID) != 40 || c.ShortOID == "" || c.AuthorName != "Test Author" || + c.AuthorEmail != "test@example.com" || c.AuthorWhen.IsZero() { + t.Errorf("%s: incomplete commit %+v", name, c) + } + } +} + +func TestEntryLastCommits_NestedDirectoryAttributesDeepChanges(t *testing.T) { + t.Parallel() + gitDir := buildHistoryRepo(t, []commitStep{ + {subject: "c1 seed", write: map[string]string{ + "src/pkg/deep/f.go": "1\n", "src/top.go": "1\n", "other/x": "1\n", + }}, + {subject: "c2 deep", write: map[string]string{"src/pkg/deep/f.go": "2\n"}}, + {subject: "c3 unrelated", write: map[string]string{"other/x": "2\n"}}, + }) + + got, err := gitops.EntryLastCommits(context.Background(), gitDir, gitops.LastCommitOptions{ + Ref: "trunk", Dir: "src", Names: []string{"pkg", "top.go"}, + }) + if err != nil { + t.Fatalf("EntryLastCommits: %v", err) + } + // A change three levels down still attributes to the immediate + // child `pkg`, and the unrelated sibling commit is ignored. + if got["pkg"].Subject != "c2 deep" { + t.Errorf("pkg subject = %q, want %q", got["pkg"].Subject, "c2 deep") + } + if got["top.go"].Subject != "c1 seed" { + t.Errorf("top.go subject = %q, want %q", got["top.go"].Subject, "c1 seed") + } +} + +func TestEntryLastCommits_RenameAttributesToRenameCommit(t *testing.T) { + t.Parallel() + gitDir := buildHistoryRepo(t, []commitStep{ + {subject: "c1 seed", write: map[string]string{"old.txt": "x\n", "keep.txt": "k\n"}}, + {subject: "c2 rename", rename: [2]string{"old.txt", "new.txt"}}, + }) + + got, err := gitops.EntryLastCommits(context.Background(), gitDir, gitops.LastCommitOptions{ + Ref: "trunk", Names: []string{"new.txt", "keep.txt"}, + }) + if err != nil { + t.Fatalf("EntryLastCommits: %v", err) + } + if got["new.txt"].Subject != "c2 rename" { + t.Errorf("new.txt subject = %q, want %q", got["new.txt"].Subject, "c2 rename") + } + if got["keep.txt"].Subject != "c1 seed" { + t.Errorf("keep.txt subject = %q, want %q", got["keep.txt"].Subject, "c1 seed") + } + // The pre-rename name no longer exists in the tree, so callers + // never ask for it — and the walk must not invent an answer. + if _, ok := got["old.txt"]; ok { + t.Errorf("old.txt should not resolve; it is not a listed entry") + } +} + +func TestEntryLastCommits_UntouchedNameStaysUnresolved(t *testing.T) { + t.Parallel() + gitDir := buildHistoryRepo(t, []commitStep{ + {subject: "c1 seed", write: map[string]string{"a.txt": "a\n"}}, + }) + + got, err := gitops.EntryLastCommits(context.Background(), gitDir, gitops.LastCommitOptions{ + Ref: "trunk", Names: []string{"a.txt", "ghost.txt"}, + }) + if err != nil { + t.Fatalf("EntryLastCommits: %v", err) + } + if _, ok := got["a.txt"]; !ok { + t.Errorf("a.txt should resolve") + } + if _, ok := got["ghost.txt"]; ok { + t.Errorf("ghost.txt resolved but was never committed") + } +} + +func TestEntryLastCommits_WalkBoundLeavesStragglersUnresolved(t *testing.T) { + t.Parallel() + // `old.txt` is touched only by the very first commit; five newer + // commits touch `hot.txt`. A bound of 2 cannot reach far enough + // back, so `old.txt` must come back unresolved for the caller's + // per-path fallback. + steps := []commitStep{ + {subject: "c0 seed", write: map[string]string{"old.txt": "o\n", "hot.txt": "0\n"}}, + } + for i := 1; i <= 5; i++ { + steps = append(steps, commitStep{ + subject: "hot " + strconv.Itoa(i), + write: map[string]string{"hot.txt": strconv.Itoa(i) + "\n"}, + }) + } + gitDir := buildHistoryRepo(t, steps) + + got, err := gitops.EntryLastCommits(context.Background(), gitDir, gitops.LastCommitOptions{ + Ref: "trunk", Names: []string{"hot.txt", "old.txt"}, MaxCommits: 2, + }) + if err != nil { + t.Fatalf("EntryLastCommits: %v", err) + } + if got["hot.txt"].Subject != "hot 5" { + t.Errorf("hot.txt subject = %q, want %q", got["hot.txt"].Subject, "hot 5") + } + if _, ok := got["old.txt"]; ok { + t.Errorf("old.txt resolved despite a 2-commit walk bound") + } + + // Without the bound the same walk resolves both. + all, err := gitops.EntryLastCommits(context.Background(), gitDir, gitops.LastCommitOptions{ + Ref: "trunk", Names: []string{"hot.txt", "old.txt"}, + }) + if err != nil { + t.Fatalf("EntryLastCommits (unbounded): %v", err) + } + if all["old.txt"].Subject != "c0 seed" { + t.Errorf("old.txt subject = %q, want %q", all["old.txt"].Subject, "c0 seed") + } +} + +func TestEntryLastCommits_MatchesPerPathLogForEveryEntry(t *testing.T) { + t.Parallel() + // Parity harness: the single walk must agree with the N-fork + // `git log -1 -- ` it replaces, entry for entry. + gitDir := buildHistoryRepo(t, []commitStep{ + {subject: "c1 seed", write: map[string]string{ + "README.md": "1\n", "src/a.go": "1\n", "src/b.go": "1\n", + "docs/d1.md": "1\n", "vendor/lib/v.go": "1\n", + }}, + {subject: "c2 src", write: map[string]string{"src/b.go": "2\n"}}, + {subject: "c3 vendor", write: map[string]string{"vendor/lib/v.go": "2\n"}}, + {subject: "c4 docs", write: map[string]string{"docs/d1.md": "2\n"}}, + {subject: "c5 readme", write: map[string]string{"README.md": "2\n"}}, + }) + + names := []string{"README.md", "src", "docs", "vendor"} + got, err := gitops.EntryLastCommits(context.Background(), gitDir, gitops.LastCommitOptions{ + Ref: "trunk", Names: names, + }) + if err != nil { + t.Fatalf("EntryLastCommits: %v", err) + } + for _, name := range names { + commits, err := gitops.Log(context.Background(), gitDir, gitops.LogOptions{ + Ref: "trunk", MaxCount: 1, Path: name, + }) + if err != nil { + t.Fatalf("Log %s: %v", name, err) + } + if len(commits) != 1 { + t.Fatalf("Log %s returned %d commits", name, len(commits)) + } + if got[name].OID != commits[0].OID { + t.Errorf("%s: walk OID %q, per-path OID %q", name, got[name].OID, commits[0].OID) + } + if got[name].Subject != commits[0].Subject { + t.Errorf("%s: walk subject %q, per-path subject %q", name, got[name].Subject, commits[0].Subject) + } + } +} + +func TestEntryLastCommits_ForkCountIsConstantInEntryCount(t *testing.T) { + // The point of the whole exercise: 1 fork for 5 entries, 1 fork + // for 60. The old code shape was one `git log -1` per entry. + for _, n := range []int{5, 60} { + n := n + t.Run(fmt.Sprintf("entries=%d", n), func(t *testing.T) { + seed := map[string]string{} + names := make([]string, 0, n) + for i := 0; i < n; i++ { + name := fmt.Sprintf("f%03d.txt", i) + seed[name] = "x\n" + names = append(names, name) + } + gitDir := buildHistoryRepo(t, []commitStep{{subject: "seed", write: seed}}) + + before := gitops.ForkCount() + got, err := gitops.EntryLastCommits(context.Background(), gitDir, gitops.LastCommitOptions{ + Ref: "trunk", Names: names, + }) + if err != nil { + t.Fatalf("EntryLastCommits: %v", err) + } + if forks := gitops.ForkCount() - before; forks != 1 { + t.Errorf("fork count for %d entries = %d, want 1", n, forks) + } + if len(got) != n { + t.Errorf("resolved %d of %d entries", len(got), n) + } + }) + } +} + +func TestEntryLastCommits_NoNamesIsANoop(t *testing.T) { + gitDir := buildHistoryRepo(t, []commitStep{ + {subject: "c1", write: map[string]string{"a.txt": "a\n"}}, + }) + before := gitops.ForkCount() + got, err := gitops.EntryLastCommits(context.Background(), gitDir, gitops.LastCommitOptions{Ref: "trunk"}) + if err != nil { + t.Fatalf("EntryLastCommits: %v", err) + } + if len(got) != 0 { + t.Errorf("got %d entries, want 0", len(got)) + } + if forks := gitops.ForkCount() - before; forks != 0 { + t.Errorf("fork count = %d, want 0 (no names to resolve)", forks) + } +} + +func TestEntryLastCommits_BadRefReturnsError(t *testing.T) { + t.Parallel() + gitDir := buildHistoryRepo(t, []commitStep{ + {subject: "c1", write: map[string]string{"a.txt": "a\n"}}, + }) + got, err := gitops.EntryLastCommits(context.Background(), gitDir, gitops.LastCommitOptions{ + Ref: "no-such-ref", Names: []string{"a.txt"}, + }) + if err == nil { + t.Fatalf("expected an error for a missing ref") + } + if len(got) != 0 { + t.Errorf("got %d entries on a failed walk, want 0", len(got)) + } +} diff --git a/internal/repos/git/logops.go b/internal/repos/git/logops.go index 6a20dbf4..ce5d7471 100644 --- a/internal/repos/git/logops.go +++ b/internal/repos/git/logops.go @@ -48,6 +48,8 @@ type LogOptions struct { // unit-separators (\x1f), with body terminated by ASCII record-separator // (\x1e) so newlines inside the body don't break parsing. func Log(ctx context.Context, gitDir string, o LogOptions) ([]Commit, error) { + ctx, cancel := readCtx(ctx) + defer cancel() if o.MaxCount <= 0 { o.MaxCount = 30 } @@ -80,7 +82,7 @@ func Log(ctx context.Context, gitDir string, o LogOptions) ([]Commit, error) { args = append(args, "--", o.Path) } - cmd := exec.CommandContext(ctx, "git", args...) + cmd := gitCmd(ctx, args...) out, err := cmd.Output() if err != nil { return nil, wrapExecErr(err) @@ -90,7 +92,9 @@ func Log(ctx context.Context, gitDir string, o LogOptions) ([]Commit, error) { // CountCommits returns the number of commits reachable from ref. func CountCommits(ctx context.Context, gitDir, ref string) (int, error) { - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, "rev-list", "--count", ref) + ctx, cancel := readCtx(ctx) + defer cancel() + cmd := gitCmd(ctx, "-C", gitDir, "rev-list", "--count", ref) out, err := cmd.Output() if err != nil { return 0, wrapExecErr(err) @@ -104,7 +108,7 @@ func CountCommits(ctx context.Context, gitDir, ref string) (int, error) { // CommitExists reports whether sha resolves to a commit in this repository. func CommitExists(ctx context.Context, gitDir, sha string) (bool, error) { - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, "cat-file", "-e", sha+"^{commit}") + cmd := gitCmd(ctx, "-C", gitDir, "cat-file", "-e", sha+"^{commit}") if _, err := cmd.Output(); err != nil { var ee *exec.ExitError if errors.As(err, &ee) && isMissingGitObjectError(ee.Stderr) { @@ -133,7 +137,7 @@ func WeeklyCommitActivity(ctx context.Context, gitDir, ref string, bucketCount i end := weekStart.AddDate(0, 0, 7) //nolint:gosec // G204: gitDir is constrained by RepoFS path validation; ref is an argv value. - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, "log", + cmd := gitCmd(ctx, "-C", gitDir, "log", "--format=%ct", "--since="+start.Format(time.RFC3339), "--until="+end.Format(time.RFC3339), @@ -233,7 +237,7 @@ func GetCommit(ctx context.Context, gitDir, sha string) (CommitDetail, error) { "%cn", "%ce", "%ct", "%P", "%T", "%s", }, sep) + sep + "%B" - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + cmd := gitCmd(ctx, "-C", gitDir, "log", "-1", "--format="+format, sha, "--") out, err := cmd.Output() if err != nil { @@ -302,13 +306,13 @@ func DiffStat(ctx context.Context, gitDir, sha string) ([]FileChange, error) { // `--root` makes the initial (parentless) commit show its files // against the empty tree; without it diff-tree emits nothing for // root commits. - nsOut, err := exec.CommandContext(ctx, "git", "-C", gitDir, + nsOut, err := gitCmd(ctx, "-C", gitDir, "diff-tree", "-r", "--root", "--name-status", "--no-commit-id", "-M", "-C", sha).Output() if err != nil { return nil, wrapExecErr(err) } // --numstat: "\t\t" or "-\t-\t" for binary. - numOut, err := exec.CommandContext(ctx, "git", "-C", gitDir, + numOut, err := gitCmd(ctx, "-C", gitDir, "diff-tree", "-r", "--root", "--numstat", "--no-commit-id", "-M", "-C", sha).Output() if err != nil { return nil, wrapExecErr(err) @@ -390,7 +394,7 @@ func ChangedPaths(ctx context.Context, gitDir, before, after string) ([]string, if isZeroSHAGit(before) { return ListAllPaths(ctx, gitDir, after) } - out, err := exec.CommandContext(ctx, "git", "-C", gitDir, + out, err := gitCmd(ctx, "-C", gitDir, "diff", "--name-only", "-z", before+".."+after).Output() if err != nil { var ee *exec.ExitError diff --git a/internal/repos/git/mergeops.go b/internal/repos/git/mergeops.go index eb2996e4..b67c6207 100644 --- a/internal/repos/git/mergeops.go +++ b/internal/repos/git/mergeops.go @@ -18,7 +18,7 @@ import ( // ResolveRefOID returns the full SHA for `ref` via `git rev-parse`. // Returns ErrRefNotFound when git can't resolve. func ResolveRefOID(ctx context.Context, gitDir, ref string) (string, error) { - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, "rev-parse", "--verify", ref+"^{commit}") + cmd := gitCmd(ctx, "-C", gitDir, "rev-parse", "--verify", ref+"^{commit}") out, err := cmd.Output() if err != nil { var ee *exec.ExitError @@ -44,7 +44,7 @@ type MergeTreeResult struct { // merge (TreeOID set); exit 1 = conflicts (ConflictPaths populated). // Anything else is wrapped. func ProbeMerge(ctx context.Context, gitDir, baseOID, headOID string) (MergeTreeResult, error) { - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + cmd := gitCmd(ctx, "-C", gitDir, "merge-tree", "--write-tree", "--no-messages", baseOID, headOID) var stdout, stderr bytes.Buffer @@ -90,8 +90,8 @@ func CommitsBetweenDetail(ctx context.Context, gitDir, baseOID, headOID string, "%cn", "%ce", "%ct", "%s", }, sep) + sep + "%b" + recordEnd - cmd := exec.CommandContext( - ctx, "git", "-C", gitDir, + cmd := gitCmd( + ctx, "-C", gitDir, "log", "--reverse", "--max-count="+strconv.Itoa(max), "--format="+format, @@ -148,8 +148,8 @@ func parseCommitDetail(out []byte) []CommitDetail { // changes from merge-base to head). Status is git's letter code, // renames carry the old path as the second column. func FilesChangedBetween(ctx context.Context, gitDir, baseOID, headOID string) ([]PRFileChange, error) { - cmd := exec.CommandContext( - ctx, "git", "-C", gitDir, + cmd := gitCmd( + ctx, "-C", gitDir, "diff", "--name-status", "-M", "-C", baseOID+"..."+headOID, ) @@ -161,8 +161,8 @@ func FilesChangedBetween(ctx context.Context, gitDir, baseOID, headOID string) ( } return nil, wrapExecErr(err) } - cmd = exec.CommandContext( - ctx, "git", "-C", gitDir, + cmd = gitCmd( + ctx, "-C", gitDir, "diff", "--numstat", "-M", "-C", baseOID+"..."+headOID, ) @@ -286,7 +286,7 @@ func PerformMerge(ctx context.Context, opts MergeOptions) (MergeResult, error) { // Set up the worktree at base_oid (detached). Using detached HEAD // keeps the worktree from polluting the bare repo's branch refs; // we only push the resulting commit back to base_ref at the end. - addCmd := exec.CommandContext(ctx, "git", "-C", opts.GitDir, + addCmd := gitCmd(ctx, "-C", opts.GitDir, "worktree", "add", "--detach", wt, opts.BaseOID) if out, err := addCmd.CombinedOutput(); err != nil { return MergeResult{}, fmt.Errorf("worktree add: %w (%s)", err, out) @@ -317,7 +317,7 @@ func PerformMerge(ctx context.Context, opts MergeOptions) (MergeResult, error) { if opts.Body != "" { msg += "\n\n" + opts.Body } - mergeCmd := exec.CommandContext(ctx, "git", "-C", wt, + mergeCmd := gitCmd(ctx, "-C", wt, "merge", "--no-ff", "--no-edit", "-m", msg, opts.HeadOID) mergeCmd.Env = envBase if out, err := mergeCmd.CombinedOutput(); err != nil { @@ -327,7 +327,7 @@ func PerformMerge(ctx context.Context, opts MergeOptions) (MergeResult, error) { // `git merge --squash` stages the squashed change without // committing; `git commit` makes the squash commit with a // single author/committer pair. - squashCmd := exec.CommandContext(ctx, "git", "-C", wt, + squashCmd := gitCmd(ctx, "-C", wt, "merge", "--squash", opts.HeadOID) squashCmd.Env = envBase if out, err := squashCmd.CombinedOutput(); err != nil { @@ -337,7 +337,7 @@ func PerformMerge(ctx context.Context, opts MergeOptions) (MergeResult, error) { if opts.Body != "" { msg += "\n\n" + opts.Body } - commitCmd := exec.CommandContext(ctx, "git", "-C", wt, + commitCmd := gitCmd(ctx, "-C", wt, "commit", "-m", msg) commitCmd.Env = envBase if out, err := commitCmd.CombinedOutput(); err != nil { @@ -347,7 +347,7 @@ func PerformMerge(ctx context.Context, opts MergeOptions) (MergeResult, error) { // Replay head_oid onto base_oid. --rebase-merges off means we // flatten merge commits into linear history; this matches the // standard "rebase merge" UX. - rebaseCmd := exec.CommandContext(ctx, "git", "-C", wt, + rebaseCmd := gitCmd(ctx, "-C", wt, "rebase", "--onto", opts.BaseOID, opts.BaseOID, opts.HeadOID) rebaseCmd.Env = envBase if out, err := rebaseCmd.CombinedOutput(); err != nil { @@ -361,7 +361,7 @@ func PerformMerge(ctx context.Context, opts MergeOptions) (MergeResult, error) { } // Capture the resulting tip of HEAD in the worktree. - revOut, err := exec.CommandContext(ctx, "git", "-C", wt, "rev-parse", "HEAD").Output() + revOut, err := gitCmd(ctx, "-C", wt, "rev-parse", "HEAD").Output() if err != nil { return MergeResult{}, fmt.Errorf("rev-parse HEAD: %w", err) } @@ -369,7 +369,7 @@ func PerformMerge(ctx context.Context, opts MergeOptions) (MergeResult, error) { // Update base_ref atomically via update-ref, gated on the expected // old OID to defend against concurrent pushes during the merge. - updateCmd := exec.CommandContext(ctx, "git", "-C", opts.GitDir, + updateCmd := gitCmd(ctx, "-C", opts.GitDir, "update-ref", opts.BaseRef, newOID, opts.BaseOID) if out, err := updateCmd.CombinedOutput(); err != nil { return MergeResult{}, fmt.Errorf("update-ref %s: %w (%s)", opts.BaseRef, err, out) @@ -420,7 +420,7 @@ type UpdateBranchResult struct { // tests against a vapor endpoint. func UpdateBranchFromBase(ctx context.Context, opts UpdateBranchOptions) (UpdateBranchResult, error) { // Already up-to-date: base is an ancestor of head → nothing to do. - if out, err := exec.CommandContext(ctx, "git", "-C", opts.GitDir, + if out, err := gitCmd(ctx, "-C", opts.GitDir, "merge-base", "--is-ancestor", opts.BaseOID, opts.HeadOID).CombinedOutput(); err == nil { return UpdateBranchResult{}, ErrBranchAlreadyUpToDate } else { @@ -448,7 +448,7 @@ func UpdateBranchFromBase(ctx context.Context, opts UpdateBranchOptions) (Update // Worktree at head_oid (detached). We'll merge/rebase base into // it, then push the resulting tip to head_ref. - if out, err := exec.CommandContext(ctx, "git", "-C", opts.GitDir, + if out, err := gitCmd(ctx, "-C", opts.GitDir, "worktree", "add", "--detach", wt, opts.HeadOID).CombinedOutput(); err != nil { return UpdateBranchResult{}, fmt.Errorf("worktree add: %w (%s)", err, out) } @@ -471,7 +471,7 @@ func UpdateBranchFromBase(ctx context.Context, opts UpdateBranchOptions) (Update switch opts.Method { case "", "merge": msg := "Merge base into " + strings.TrimPrefix(opts.HeadRef, "refs/heads/") - mergeCmd := exec.CommandContext(ctx, "git", "-C", wt, + mergeCmd := gitCmd(ctx, "-C", wt, "merge", "--no-ff", "--no-edit", "-m", msg, opts.BaseOID) mergeCmd.Env = envBase if out, err := mergeCmd.CombinedOutput(); err != nil { @@ -479,7 +479,7 @@ func UpdateBranchFromBase(ctx context.Context, opts UpdateBranchOptions) (Update } case "rebase": // Replay head's commits onto base_oid. - rebaseCmd := exec.CommandContext(ctx, "git", "-C", wt, + rebaseCmd := gitCmd(ctx, "-C", wt, "rebase", "--onto", opts.BaseOID, opts.BaseOID, opts.HeadOID) rebaseCmd.Env = envBase if out, err := rebaseCmd.CombinedOutput(); err != nil { @@ -490,14 +490,14 @@ func UpdateBranchFromBase(ctx context.Context, opts UpdateBranchOptions) (Update return UpdateBranchResult{}, fmt.Errorf("unknown update-branch method %q", opts.Method) } - revOut, err := exec.CommandContext(ctx, "git", "-C", wt, "rev-parse", "HEAD").Output() + revOut, err := gitCmd(ctx, "-C", wt, "rev-parse", "HEAD").Output() if err != nil { return UpdateBranchResult{}, fmt.Errorf("rev-parse HEAD: %w", err) } newOID := strings.TrimSpace(string(revOut)) // Atomic update-ref CAS on the head ref. - if out, err := exec.CommandContext(ctx, "git", "-C", opts.GitDir, + if out, err := gitCmd(ctx, "-C", opts.GitDir, "update-ref", opts.HeadRef, newOID, opts.HeadOID).CombinedOutput(); err != nil { return UpdateBranchResult{}, fmt.Errorf("update-ref %s: %w (%s)", opts.HeadRef, err, out) } diff --git a/internal/repos/git/plumbing.go b/internal/repos/git/plumbing.go index b4d23189..09ed84be 100644 --- a/internal/repos/git/plumbing.go +++ b/internal/repos/git/plumbing.go @@ -108,7 +108,7 @@ func (ic InitialCommit) Build(ctx context.Context) (string, error) { // returns the resulting OID. func (ic InitialCommit) hashObject(ctx context.Context, body []byte) (string, error) { //nolint:gosec // G204: gitDir is constrained by storage.RepoFS path validation. - cmd := exec.CommandContext(ctx, "git", "-C", ic.GitDir, "hash-object", "-w", "--stdin") + cmd := gitCmd(ctx, "-C", ic.GitDir, "hash-object", "-w", "--stdin") cmd.Stdin = bytes.NewReader(body) out, err := cmd.Output() if err != nil { @@ -123,7 +123,7 @@ func (ic InitialCommit) hashObject(ctx context.Context, body []byte) (string, er func (ic InitialCommit) updateIndex(ctx context.Context, indexPath, oid, path string) error { cacheinfo := fmt.Sprintf("100644,%s,%s", oid, path) //nolint:gosec // G204: gitDir + path are validated upstream. - cmd := exec.CommandContext(ctx, "git", "-C", ic.GitDir, "update-index", "--add", "--cacheinfo", cacheinfo) + cmd := gitCmd(ctx, "-C", ic.GitDir, "update-index", "--add", "--cacheinfo", cacheinfo) cmd.Env = append(os.Environ(), "GIT_INDEX_FILE="+indexPath) if out, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("%w: %s", wrapExecErr(err), strings.TrimSpace(string(out))) @@ -134,7 +134,7 @@ func (ic InitialCommit) updateIndex(ctx context.Context, indexPath, oid, path st // writeTree turns the staged index into a tree object and returns its OID. func (ic InitialCommit) writeTree(ctx context.Context, indexPath string) (string, error) { //nolint:gosec // G204: gitDir validated upstream. - cmd := exec.CommandContext(ctx, "git", "-C", ic.GitDir, "write-tree") + cmd := gitCmd(ctx, "-C", ic.GitDir, "write-tree") cmd.Env = append(os.Environ(), "GIT_INDEX_FILE="+indexPath) out, err := cmd.Output() if err != nil { @@ -148,7 +148,7 @@ func (ic InitialCommit) writeTree(ctx context.Context, indexPath string) (string // both timestamps so the test suite gets deterministic OIDs. func (ic InitialCommit) commitTree(ctx context.Context, tree string) (string, error) { //nolint:gosec // G204: tree is git's stdout (40-char OID); gitDir validated. - cmd := exec.CommandContext(ctx, "git", "-C", ic.GitDir, "commit-tree", tree, "-m", ic.Message) + cmd := gitCmd(ctx, "-C", ic.GitDir, "commit-tree", tree, "-m", ic.Message) stamp := ic.When.Format(time.RFC3339) cmd.Env = append( os.Environ(), @@ -172,7 +172,7 @@ func (ic InitialCommit) commitTree(ctx context.Context, tree string) (string, er func (ic InitialCommit) updateRef(ctx context.Context, commit string) error { ref := "refs/heads/" + ic.Branch //nolint:gosec // G204: ref is constructed from a non-empty branch name we set. - cmd := exec.CommandContext(ctx, "git", "-C", ic.GitDir, "update-ref", ref, commit) + cmd := gitCmd(ctx, "-C", ic.GitDir, "update-ref", ref, commit) if out, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("%w: %s", wrapExecErr(err), strings.TrimSpace(string(out))) } @@ -193,7 +193,9 @@ type HeadCommit struct { // ref under refs/heads/. Used by the repo home view to fork between the // "quick setup" empty-state and the post-push view. func HasAnyBranch(ctx context.Context, gitDir string) (bool, error) { - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + ctx, cancel := readCtx(ctx) + defer cancel() + cmd := gitCmd(ctx, "-C", gitDir, "for-each-ref", "--count=1", "--format=%(refname)", "refs/heads/") out, err := cmd.Output() if err != nil { @@ -216,11 +218,13 @@ func HeadOf(ctx context.Context, gitDir, branch string) (HeadCommit, bool, error } func commitAt(ctx context.Context, gitDir, rev string) (HeadCommit, bool, error) { + ctx, cancel := readCtx(ctx) + defer cancel() // Single git invocation — %x1f is ASCII unit-separator, an unambiguous // delimiter that won't appear in commit subjects/authors. const sep = "\x1f" format := strings.Join([]string{"%H", "%s", "%an", "%ae", "%ct"}, sep) - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + cmd := gitCmd(ctx, "-C", gitDir, "log", "-1", "--format="+format, rev, "--") out, err := cmd.Output() if err != nil { diff --git a/internal/repos/git/remotes.go b/internal/repos/git/remotes.go index 2c2a885e..4161e35b 100644 --- a/internal/repos/git/remotes.go +++ b/internal/repos/git/remotes.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "os" - "os/exec" "path/filepath" "strings" ) @@ -49,7 +48,7 @@ func fetchRemoteHeadsAndTags(ctx context.Context, gitDir, remoteURL, token strin env = append(env, "GIT_ASKPASS="+askpass) } //nolint:gosec // G204: gitDir is RepoFS-derived at call sites; remoteURL is caller-allowlisted and passed as argv, not shell. - cmd := exec.CommandContext(ctx, "git", + cmd := gitCmd(ctx, "-c", "protocol.ext.allow=never", "-C", gitDir, "fetch", diff --git a/internal/repos/git/submodule.go b/internal/repos/git/submodule.go index 0cc3798d..00ceb520 100644 --- a/internal/repos/git/submodule.go +++ b/internal/repos/git/submodule.go @@ -24,6 +24,8 @@ type Submodule struct { // Submodules reads and parses :.gitmodules. Missing or non-blob files // return an empty map so callers can render plain gitlink rows. func Submodules(ctx context.Context, gitDir, ref string) (map[string]Submodule, error) { + ctx, cancel := readCtx(ctx) + defer cancel() kind, _, size, err := StatPath(ctx, gitDir, ref, ".gitmodules") if err != nil { if errors.Is(err, ErrPathNotFound) { diff --git a/internal/repos/git/treeops.go b/internal/repos/git/treeops.go index ed1d61cb..ef609163 100644 --- a/internal/repos/git/treeops.go +++ b/internal/repos/git/treeops.go @@ -33,7 +33,9 @@ type RefEntry struct { // ListRefs enumerates branches and tags. Empty repos return empty // slices, not an error. func ListRefs(ctx context.Context, gitDir string) (RefListing, error) { - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + ctx, cancel := readCtx(ctx) + defer cancel() + cmd := gitCmd(ctx, "-C", gitDir, "for-each-ref", "--format=%(refname)\x1f%(objectname)", "refs/heads/", "refs/tags/") out, err := cmd.Output() @@ -141,11 +143,13 @@ type TreeFile struct { // Returns an empty slice when the path doesn't exist or is itself a // blob — callers should fall back to BlobInfo. func LsTree(ctx context.Context, gitDir, ref, path string) ([]TreeEntry, error) { + ctx, cancel := readCtx(ctx) + defer cancel() target := ref + ":" + path if path == "" { target = ref + ":" } - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + cmd := gitCmd(ctx, "-C", gitDir, "ls-tree", "--long", "--full-tree", target) out, err := cmd.Output() if err != nil { @@ -203,7 +207,9 @@ func LsTree(ctx context.Context, gitDir, ref, path string) ([]TreeEntry, error) // keeps only blobs because callers use it for file-level summaries such // as language bars. func ListBlobs(ctx context.Context, gitDir, ref string) ([]TreeBlob, error) { - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + ctx, cancel := readCtx(ctx) + defer cancel() + cmd := gitCmd(ctx, "-C", gitDir, "ls-tree", "-r", "--long", "--full-tree", "-z", ref) out, err := cmd.Output() if err != nil { @@ -239,7 +245,9 @@ func ListBlobs(ctx context.Context, gitDir, ref string) ([]TreeBlob, error) { // git-reported byte size. Unlike ListBlobs, it keeps symlinks because // code search should still index the symlink path. func ListFiles(ctx context.Context, gitDir, ref string) ([]TreeFile, error) { - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + ctx, cancel := readCtx(ctx) + defer cancel() + cmd := gitCmd(ctx, "-C", gitDir, "ls-tree", "-r", "--long", "--full-tree", "-z", ref) out, err := cmd.Output() if err != nil { @@ -312,12 +320,14 @@ type BlobInfo struct { // the handler to decide whether to render tree or blob without a // second round-trip. func StatPath(ctx context.Context, gitDir, ref, path string) (kind TreeEntryKind, oid string, size int64, err error) { + ctx, cancel := readCtx(ctx) + defer cancel() target := ref + ":" + path if path == "" { target = ref + ":" } // `git cat-file -t :` returns the type. - tCmd := exec.CommandContext(ctx, "git", "-C", gitDir, "cat-file", "-t", target) + tCmd := gitCmd(ctx, "-C", gitDir, "cat-file", "-t", target) tOut, tErr := tCmd.Output() if tErr != nil { var ee *exec.ExitError @@ -344,7 +354,7 @@ func StatPath(ctx context.Context, gitDir, ref, path string) (kind TreeEntryKind return "", "", 0, fmt.Errorf("git: unexpected type %q", gitType) } - sCmd := exec.CommandContext(ctx, "git", "-C", gitDir, "cat-file", "-s", target) + sCmd := gitCmd(ctx, "-C", gitDir, "cat-file", "-s", target) sOut, err := sCmd.Output() if err != nil { return "", "", 0, wrapExecErr(err) @@ -361,16 +371,23 @@ func StatPath(ctx context.Context, gitDir, ref, path string) (kind TreeEntryKind // stream. Pass 0 for "no cap"; otherwise an oversize read returns // ErrBlobTooLarge. func ReadBlobBytes(ctx context.Context, gitDir, ref, path string, maxBytes int64) ([]byte, error) { + ctx, cancel := readCtx(ctx) target := ref + ":" + path - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, "cat-file", "-p", target) + cmd := gitCmd(ctx, "-C", gitDir, "cat-file", "-p", target) stdout, err := cmd.StdoutPipe() if err != nil { + cancel() return nil, err } if err := cmd.Start(); err != nil { + cancel() return nil, err } - defer func() { _ = cmd.Wait() }() + // Cancel BEFORE Wait: when the LimitReader stops short of the blob + // (an oversize file), git is still blocked writing into a full + // pipe and Wait alone would never return. Cancelling kills it, then + // Wait reaps. + defer func() { cancel(); _ = cmd.Wait() }() var r io.Reader = stdout if maxBytes > 0 { // LimitReader so giant blobs don't OOM us. @@ -390,7 +407,7 @@ func ReadBlobBytes(ctx context.Context, gitDir, ref, path string, maxBytes int64 // buffer; this lets the response stream as `git cat-file -p` produces. func StreamBlob(ctx context.Context, gitDir, ref, path string, w io.Writer) error { target := ref + ":" + path - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, "cat-file", "-p", target) + cmd := gitCmd(ctx, "-C", gitDir, "cat-file", "-p", target) cmd.Stdout = w var stderr bytes.Buffer cmd.Stderr = &stderr @@ -408,7 +425,9 @@ var ErrBlobTooLarge = errors.New("git: blob exceeds size cap") // out submodule-style entries (commit type) which shouldn't surface // in the file finder. func ListAllPaths(ctx context.Context, gitDir, ref string) ([]string, error) { - cmd := exec.CommandContext(ctx, "git", "-C", gitDir, + ctx, cancel := readCtx(ctx) + defer cancel() + cmd := gitCmd(ctx, "-C", gitDir, "ls-tree", "-r", "--full-tree", "--name-only", ref) out, err := cmd.Output() if err != nil { diff --git a/internal/web/handlers/repo/about_sidebar.go b/internal/web/handlers/repo/about_sidebar.go index 8778c92a..8b4e3029 100644 --- a/internal/web/handlers/repo/about_sidebar.go +++ b/internal/web/handlers/repo/about_sidebar.go @@ -14,6 +14,7 @@ import ( "github.com/tenseleyFlow/shithub/internal/repos/git" "github.com/tenseleyFlow/shithub/internal/repos/identity" reposdb "github.com/tenseleyFlow/shithub/internal/repos/sqlc" + "github.com/tenseleyFlow/shithub/internal/web/handlers/repo/treecache" ) type repoAboutData struct { @@ -68,11 +69,15 @@ type repoLanguageAggregate struct { size int64 } -func (h *Handlers) repoAbout(ctx context.Context, gitDir, ref, owner string, row reposdb.Repo, rootEntries []git.TreeEntry) repoAboutData { +// repoAbout builds the About sidebar. commitOID is the resolved OID +// of the commit being rendered; it keys the contributor and language +// caches, so a push (new OID) is what invalidates them. Pass "" to +// bypass the caches. +func (h *Handlers) repoAbout(ctx context.Context, cc *codeContext, commitOID string, rootEntries []git.TreeEntry) repoAboutData { return repoAboutData{ - Resources: repoAboutResources(owner, row.Name, ref, row, rootEntries), - Contributors: h.repoAboutContributors(ctx, gitDir, ref), - Languages: h.repoAboutLanguages(ctx, gitDir, ref, row), + Resources: repoAboutResources(cc.owner, cc.row.Name, cc.ref, cc.row, rootEntries), + Contributors: h.repoAboutContributors(ctx, cc, commitOID), + Languages: h.repoAboutLanguages(ctx, cc, commitOID), } } @@ -225,8 +230,48 @@ func repoOverviewDocumentLabel(resource repoAboutResource) string { return resource.Label } -func (h *Handlers) repoAboutContributors(ctx context.Context, gitDir, ref string) []repoAboutContributor { - commits, err := git.Log(ctx, gitDir, git.LogOptions{Ref: ref, MaxCount: 500}) +// contributorWalkDepth is how far back the About sidebar's +// contributor strip counts. It is a `git log -n 500` on every repo +// home view, so the tally it reduces to is cached per commit OID. +const contributorWalkDepth = 500 + +// repoAboutContributorTally runs the bounded walk and reduces it to +// one row per distinct author. Only this reduction is cached — +// identity resolution stays per-request because it reads the users +// table and its answer can change without any git ref moving. +func (h *Handlers) repoAboutContributorTally(ctx context.Context, cc *codeContext, commitOID string) ([]treecache.ContributorTally, error) { + return h.d.TreeCache.Contributors(ctx, + treecache.RevKey{RepoID: cc.row.ID, CommitOID: commitOID}, + func(ctx context.Context) ([]treecache.ContributorTally, error) { + commits, err := git.Log(ctx, cc.gitDir, git.LogOptions{Ref: cc.ref, MaxCount: contributorWalkDepth}) + if err != nil { + return nil, err + } + order := make([]string, 0, len(commits)) + byKey := map[string]*treecache.ContributorTally{} + for _, c := range commits { + key := strings.ToLower(strings.TrimSpace(c.AuthorEmail)) + if key == "" { + key = "name:" + strings.ToLower(strings.TrimSpace(c.AuthorName)) + } + tally, ok := byKey[key] + if !ok { + tally = &treecache.ContributorTally{Name: c.AuthorName, Email: c.AuthorEmail} + byKey[key] = tally + order = append(order, key) + } + tally.Count++ + } + out := make([]treecache.ContributorTally, 0, len(order)) + for _, key := range order { + out = append(out, *byKey[key]) + } + return out, nil + }) +} + +func (h *Handlers) repoAboutContributors(ctx context.Context, cc *codeContext, commitOID string) []repoAboutContributor { + tallies, err := h.repoAboutContributorTally(ctx, cc, commitOID) if err != nil { if h.d.Logger != nil { h.d.Logger.WarnContext(ctx, "repo about: contributors", "error", err) @@ -238,8 +283,8 @@ func (h *Handlers) repoAboutContributors(ctx context.Context, gitDir, ref string } byAuthor := map[string]*aggregate{} resolver := identity.New(h.d.Pool) - for _, c := range commits { - resolved := resolver.Resolve(ctx, c.AuthorEmail) + for _, c := range tallies { + resolved := resolver.Resolve(ctx, c.Email) key := "" contributor := repoAboutContributor{} if resolved.User { @@ -253,16 +298,16 @@ func (h *Handlers) repoAboutContributors(ctx context.Context, gitDir, ref string contributor.Label = resolved.Username } } else { - email := strings.ToLower(strings.TrimSpace(c.AuthorEmail)) - name := strings.ToLower(strings.TrimSpace(c.AuthorName)) + email := strings.ToLower(strings.TrimSpace(c.Email)) + name := strings.ToLower(strings.TrimSpace(c.Name)) if email != "" { key = "email:" + email } else if name != "" { key = "name:" + name } - contributor.Label = strings.TrimSpace(c.AuthorName) + contributor.Label = strings.TrimSpace(c.Name) if contributor.Label == "" { - contributor.Label = strings.TrimSpace(c.AuthorEmail) + contributor.Label = strings.TrimSpace(c.Email) } contributor.IdenticonSeed = resolved.IdenticonSeed } @@ -274,7 +319,7 @@ func (h *Handlers) repoAboutContributors(ctx context.Context, gitDir, ref string agg = &aggregate{contributor: contributor} byAuthor[key] = agg } - agg.contributor.Count++ + agg.contributor.Count += c.Count } contributors := make([]repoAboutContributor, 0, len(byAuthor)) @@ -297,40 +342,54 @@ func (h *Handlers) repoAboutContributors(ctx context.Context, gitDir, ref string return contributors } -func (h *Handlers) repoAboutLanguages(ctx context.Context, gitDir, ref string, row reposdb.Repo) []repoAboutLanguage { - blobs, err := git.ListBlobs(ctx, gitDir, ref) +// repoAboutLanguageBytes is the `git ls-tree -r`-derived language +// aggregate: one entry per language, in bytes. The recursive walk +// itself is never cached (it can be megabytes of path text on a big +// repo); its reduction to a handful of numbers is, keyed per commit +// OID. +func (h *Handlers) repoAboutLanguageBytes(ctx context.Context, cc *codeContext, commitOID string) (map[string]int64, error) { + return h.d.TreeCache.Languages(ctx, + treecache.RevKey{RepoID: cc.row.ID, CommitOID: commitOID}, + func(ctx context.Context) (map[string]int64, error) { + blobs, err := git.ListBlobs(ctx, cc.gitDir, cc.ref) + if err != nil { + return nil, err + } + byName := map[string]int64{} + for _, blob := range blobs { + if blob.Size <= 0 { + continue + } + name, _, ok := repoLanguageForPath(blob.Path) + if !ok { + continue + } + byName[name] += blob.Size + } + return byName, nil + }) +} + +func (h *Handlers) repoAboutLanguages(ctx context.Context, cc *codeContext, commitOID string) []repoAboutLanguage { + byName, err := h.repoAboutLanguageBytes(ctx, cc, commitOID) if err != nil { if h.d.Logger != nil { h.d.Logger.WarnContext(ctx, "repo about: languages", "error", err) } - return fallbackPrimaryLanguage(row) + return fallbackPrimaryLanguage(cc.row) } - byName := map[string]*repoLanguageAggregate{} var total int64 - for _, blob := range blobs { - if blob.Size <= 0 { - continue - } - name, color, ok := repoLanguageForPath(blob.Path) - if !ok { - continue - } - agg, ok := byName[name] - if !ok { - agg = &repoLanguageAggregate{name: name, color: color} - byName[name] = agg - } - agg.size += blob.Size - total += blob.Size + for _, size := range byName { + total += size } if total == 0 { - return fallbackPrimaryLanguage(row) + return fallbackPrimaryLanguage(cc.row) } ordered := make([]repoLanguageAggregate, 0, len(byName)) - for _, agg := range byName { - ordered = append(ordered, *agg) + for name, size := range byName { + ordered = append(ordered, repoLanguageAggregate{name: name, color: repoLanguageColor(name), size: size}) } sort.Slice(ordered, func(i, j int) bool { if ordered[i].size != ordered[j].size { diff --git a/internal/web/handlers/repo/code.go b/internal/web/handlers/repo/code.go index 47f5de41..f79ac2d0 100644 --- a/internal/web/handlers/repo/code.go +++ b/internal/web/handlers/repo/code.go @@ -21,6 +21,7 @@ import ( "github.com/tenseleyFlow/shithub/internal/repos/highlight" "github.com/tenseleyFlow/shithub/internal/repos/identity" reposdb "github.com/tenseleyFlow/shithub/internal/repos/sqlc" + "github.com/tenseleyFlow/shithub/internal/web/handlers/repo/treecache" "github.com/tenseleyFlow/shithub/internal/web/middleware" ) @@ -223,7 +224,7 @@ func (h *Handlers) renderRepoTree(w http.ResponseWriter, r *http.Request, cc *co if headFound { headCheckSummary = h.codeCommitCheckSummary(r.Context(), cc.owner, cc.row.Name, cc.row.ID, head.OID) } - commitCount, countErr := repogit.CountCommits(r.Context(), cc.gitDir, cc.ref) + commitCount, countErr := h.repoCommitCount(r.Context(), cc, head.OID) if countErr != nil { h.d.Logger.WarnContext(r.Context(), "code: CountCommits", "error", countErr) } @@ -236,7 +237,7 @@ func (h *Handlers) renderRepoTree(w http.ResponseWriter, r *http.Request, cc *co h.d.Logger.WarnContext(r.Context(), "code: about root LsTree", "error", rerr) } } - about := h.repoAbout(r.Context(), cc.gitDir, cc.ref, cc.owner, cc.row, aboutEntries) + about := h.repoAbout(r.Context(), cc, head.OID, aboutEntries) // Subdirectories keep GitHub's plain local README behavior. The root tree // gets GitHub-style overview document tabs that swap README, license, // contributing, security, and conduct files in the same card. @@ -263,7 +264,7 @@ func (h *Handlers) renderRepoTree(w http.ResponseWriter, r *http.Request, cc *co "Path": cc.subpath, "Crumbs": breadcrumbs(cc.owner, cc.row.Name, cc.ref, cc.subpath), "Entries": entries, - "EntryRows": h.codeTreeEntryRows(r.Context(), cc, entries), + "EntryRows": h.codeTreeEntryRows(r.Context(), cc, entries, head.OID), "Branches": cc.refs.Branches, "Tags": cc.refs.Tags, "Head": head, @@ -289,6 +290,17 @@ func (h *Handlers) renderRepoTree(w http.ResponseWriter, r *http.Request, cc *co }) } +// repoCommitCount is `git rev-list --count`, memoized per head OID. +// The repo home runs it on every view; the value can only change when +// the commit being rendered changes, which changes the cache key. +func (h *Handlers) repoCommitCount(ctx context.Context, cc *codeContext, commitOID string) (int, error) { + return h.d.TreeCache.CommitCount(ctx, + treecache.RevKey{RepoID: cc.row.ID, CommitOID: commitOID}, + func(ctx context.Context) (int, error) { + return repogit.CountCommits(ctx, cc.gitDir, cc.ref) + }) +} + func refNames(refs repogit.RefListing) []string { allNames := make([]string, 0, len(refs.Branches)+len(refs.Tags)) for _, b := range refs.Branches { diff --git a/internal/web/handlers/repo/code_tree_forks_test.go b/internal/web/handlers/repo/code_tree_forks_test.go new file mode 100644 index 00000000..2815720a --- /dev/null +++ b/internal/web/handlers/repo/code_tree_forks_test.go @@ -0,0 +1,278 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package repo + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "testing/fstest" + + "github.com/go-chi/chi/v5" + "github.com/jackc/pgx/v5/pgtype" + + gitops "github.com/tenseleyFlow/shithub/internal/repos/git" + reposdb "github.com/tenseleyFlow/shithub/internal/repos/sqlc" + "github.com/tenseleyFlow/shithub/internal/web/handlers/repo/treecache" + "github.com/tenseleyFlow/shithub/internal/web/render" +) + +// treeTemplatesFS is minimalTemplatesFS plus a repo/tree page that +// prints exactly the fields the real template renders from the +// last-commit column, so this test also pins the rendered output. +func treeTemplatesFS() fstest.MapFS { + fsys := minimalTemplatesFS() + fsys["repo/tree.html"] = &fstest.MapFile{Data: []byte( + `{{ define "page" }}` + + `{{ range .EntryRows }}ROW={{ .Entry.Name }}:{{ .LastFound }}:{{ .LastCommit.Subject }};{{ end }}` + + `COUNT={{ .CommitCount }};` + + `{{ end }}`)} + return fsys +} + +func newTreeFixture(t *testing.T) *repoFixture { + t.Helper() + return newRepoFixtureWithTemplates(t, treeTemplatesFS(), render.Options{}) +} + +func (f *repoFixture) treeMux() *chi.Mux { + mux := chi.NewRouter() + mux.Use(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + next.ServeHTTP(w, withViewer(r, anonymousViewer())) + }) + }) + f.handlers.MountCode(mux) + return mux +} + +// seedWideRepo materializes a bare repo on the fixture's RepoFS layout +// whose root tree has `entries` files plus a README, built over two +// commits so the last-commit column has something to resolve. History +// is scripted in a scratch worktree and cloned in bare, which is the +// cheapest way to get a multi-commit fixture. +func (f *repoFixture) seedWideRepo(t *testing.T, owner, name string, entries int) { + t.Helper() + gitDir, err := f.handlers.d.RepoFS.RepoPath(owner, name) + if err != nil { + t.Fatalf("RepoFS.RepoPath: %v", err) + } + src := t.TempDir() + run := func(dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(cmd.Environ(), + "GIT_AUTHOR_NAME=Test Author", "GIT_AUTHOR_EMAIL=test@example.com", + "GIT_COMMITTER_NAME=Test Author", "GIT_COMMITTER_EMAIL=test@example.com", + "GIT_AUTHOR_DATE=2026-01-01T00:00:00Z", "GIT_COMMITTER_DATE=2026-01-01T00:00:00Z", + ) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", args, err, out) + } + } + write := func(rel, body string) { + t.Helper() + if err := os.WriteFile(filepath.Join(src, rel), []byte(body), 0o600); err != nil { + t.Fatalf("write %s: %v", rel, err) + } + } + + run(src, "init", "-q", "--initial-branch=trunk") + write("README.md", "# demo\n") + for i := 0; i < entries; i++ { + write(fmt.Sprintf("f%04d.txt", i), "seed\n") + } + run(src, "add", "-A") + run(src, "commit", "-q", "-m", "seed") + write("f0000.txt", "touched\n") + run(src, "add", "-A") + run(src, "commit", "-q", "-m", "touch first entry") + + if out, err := exec.Command("git", "clone", "-q", "--bare", src, gitDir).CombinedOutput(); err != nil { + t.Fatalf("git clone --bare: %v: %s", err, out) + } +} + +// treeRepo creates a repo row + its bare git dir with `entries` files +// at the root and returns the row. +func (f *repoFixture) treeRepo(t *testing.T, name string, entries int) reposdb.Repo { + t.Helper() + row, err := reposdb.New().CreateRepo(context.Background(), f.pool, reposdb.CreateRepoParams{ + OwnerUserID: pgtype.Int8{Int64: f.owner.ID, Valid: true}, + Name: name, + Visibility: reposdb.RepoVisibilityPublic, + DefaultBranch: "trunk", + }) + if err != nil { + t.Fatalf("CreateRepo %s: %v", name, err) + } + f.seedWideRepo(t, f.owner.Username, name, entries) + return row +} + +// getTree issues one anonymous GET of the repo's root tree and returns +// the number of git subprocesses it forked, plus the response. +func getTree(t *testing.T, mux *chi.Mux, owner, name string) (uint64, *httptest.ResponseRecorder) { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/"+owner+"/"+name+"/tree/trunk", nil) + rw := httptest.NewRecorder() + before := gitops.ForkCount() + mux.ServeHTTP(rw, req) + forks := gitops.ForkCount() - before + if rw.Code != http.StatusOK { + t.Fatalf("status=%d, want 200; body=%s", rw.Code, rw.Body.String()) + } + return forks, rw +} + +// TestCodeTree_ForkCountIsIndependentOfEntryCount is the regression +// test for the availability campaign's Phase 3 CPU item: the code tab +// used to fork `git log -1` once per tree entry, so a crawler hitting +// a 100-entry directory cost ~100 git processes per anonymous request. +// +// NOT parallel: gitops.ForkCount is process-global, and Go guarantees +// sequential tests never overlap with parallel ones. +func TestCodeTree_ForkCountIsIndependentOfEntryCount(t *testing.T) { + f := newTreeFixture(t) + mux := f.treeMux() + + small := f.treeRepo(t, "narrow-repo", 5) + large := f.treeRepo(t, "wide-repo", 80) + + smallForks, smallResp := getTree(t, mux, f.owner.Username, small.Name) + largeForks, largeResp := getTree(t, mux, f.owner.Username, large.Name) + t.Logf("git forks per cold tree render: 6 entries = %d, 81 entries = %d", smallForks, largeForks) + + if smallForks != largeForks { + t.Errorf("forks scale with entry count: 6 entries = %d forks, 81 entries = %d forks", + smallForks, largeForks) + } + // Belt and braces: the constant itself has to stay small. The + // per-request reads are ListRefs, StatPath, LsTree, CommitAt, the + // single last-commit walk, rev-list --count, the contributor log, + // the recursive ls-tree, and the README reads. + const forkCeiling = 16 + if largeForks > forkCeiling { + t.Errorf("cold tree render forked git %d times, ceiling is %d", largeForks, forkCeiling) + } + + // Output parity: every entry still gets its last-commit cell, and + // the entry touched by the second commit reports that commit. + for _, body := range []string{smallResp.Body.String(), largeResp.Body.String()} { + if strings.Contains(body, ":false:") { + t.Errorf("some rows lost their last-commit cell: %s", body) + } + if !strings.Contains(body, "ROW=f0000.txt:true:touch first entry;") { + t.Errorf("f0000.txt should report the second commit: %s", body) + } + if !strings.Contains(body, "ROW=README.md:true:seed;") { + t.Errorf("README.md should report the seed commit: %s", body) + } + if !strings.Contains(body, "COUNT=2;") { + t.Errorf("commit count should be 2: %s", body) + } + } +} + +// TestCodeTree_WarmCacheDropsGitForks pins the second half of the fix: +// once the OID-keyed cache is warm, the repeat views a crawler +// generates skip the last-commit walk, rev-list --count, the +// contributor log, and the recursive ls-tree entirely. +func TestCodeTree_WarmCacheDropsGitForks(t *testing.T) { + f := newTreeFixture(t) + f.handlers.d.TreeCache = treecache.New(treecache.DefaultCapacity, treecache.DefaultTTL) + mux := f.treeMux() + row := f.treeRepo(t, "warm-repo", 40) + + coldForks, coldResp := getTree(t, mux, f.owner.Username, row.Name) + warmForks, warmResp := getTree(t, mux, f.owner.Username, row.Name) + t.Logf("git forks per tree render: cold = %d, warm = %d", coldForks, warmForks) + + if warmForks >= coldForks { + t.Errorf("warm render forked %d times vs %d cold; the cache is not being used", + warmForks, coldForks) + } + if got := f.handlers.d.TreeCache.Stats(); got.Hits == 0 { + t.Errorf("tree cache recorded no hits: %+v", got) + } + if warmResp.Body.String() != coldResp.Body.String() { + t.Errorf("warm body differs from cold body:\ncold=%s\nwarm=%s", + coldResp.Body.String(), warmResp.Body.String()) + } +} + +// TestCodeTree_CacheMissesAfterAPush proves the invalidation contract: +// the cache key carries the rendered commit OID, so a push produces a +// new key rather than serving the pre-push listing. +func TestCodeTree_CacheMissesAfterAPush(t *testing.T) { + f := newTreeFixture(t) + f.handlers.d.TreeCache = treecache.New(treecache.DefaultCapacity, treecache.DefaultTTL) + mux := f.treeMux() + row := f.treeRepo(t, "pushed-repo", 4) + + _, before := getTree(t, mux, f.owner.Username, row.Name) + if !strings.Contains(before.Body.String(), "ROW=f0000.txt:true:touch first entry;") { + t.Fatalf("unexpected pre-push body: %s", before.Body.String()) + } + + // Land a third commit straight onto the bare repo's branch. + gitDir, err := f.handlers.d.RepoFS.RepoPath(f.owner.Username, row.Name) + if err != nil { + t.Fatalf("RepoFS.RepoPath: %v", err) + } + pushCommit(t, gitDir, "f0000.txt", "pushed\n", "third commit") + + _, after := getTree(t, mux, f.owner.Username, row.Name) + if !strings.Contains(after.Body.String(), "ROW=f0000.txt:true:third commit;") { + t.Errorf("post-push render served a stale last-commit column: %s", after.Body.String()) + } + if !strings.Contains(after.Body.String(), "COUNT=3;") { + t.Errorf("post-push commit count should be 3: %s", after.Body.String()) + } +} + +// pushCommit writes one blob onto trunk in a bare repo using the same +// plumbing sequence the web editor uses, and moves the ref. +func pushCommit(t *testing.T, gitDir, path, body, message string) { + t.Helper() + indexFile := filepath.Join(t.TempDir(), "index") + git := func(args ...string) string { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", gitDir}, args...)...) + cmd.Env = append(cmd.Environ(), + "GIT_AUTHOR_NAME=Test Author", "GIT_AUTHOR_EMAIL=test@example.com", + "GIT_COMMITTER_NAME=Test Author", "GIT_COMMITTER_EMAIL=test@example.com", + "GIT_AUTHOR_DATE=2026-02-01T00:00:00Z", "GIT_COMMITTER_DATE=2026-02-01T00:00:00Z", + "GIT_INDEX_FILE="+indexFile, + ) + out, err := cmd.Output() + if err != nil { + t.Fatalf("git %v: %v", args, err) + } + return strings.TrimSpace(string(out)) + } + head := git("rev-parse", "refs/heads/trunk") + git("read-tree", head) + blob := gitStdin(t, gitDir, body, "hash-object", "-w", "--stdin") + git("update-index", "--add", "--cacheinfo", "100644,"+blob+","+path) + tree := git("write-tree") + commit := git("commit-tree", tree, "-p", head, "-m", message) + git("update-ref", "refs/heads/trunk", commit, head) +} + +func gitStdin(t *testing.T, gitDir, stdin string, args ...string) string { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", gitDir}, args...)...) + cmd.Stdin = strings.NewReader(stdin) + out, err := cmd.Output() + if err != nil { + t.Fatalf("git %v: %v", args, err) + } + return strings.TrimSpace(string(out)) +} diff --git a/internal/web/handlers/repo/code_tree_rows.go b/internal/web/handlers/repo/code_tree_rows.go index 6c13f3b4..137dcd99 100644 --- a/internal/web/handlers/repo/code_tree_rows.go +++ b/internal/web/handlers/repo/code_tree_rows.go @@ -8,6 +8,7 @@ import ( "strings" repogit "github.com/tenseleyFlow/shithub/internal/repos/git" + "github.com/tenseleyFlow/shithub/internal/web/handlers/repo/treecache" ) type codeTreeEntryRow struct { @@ -18,7 +19,22 @@ type codeTreeEntryRow struct { LastFound bool } -func (h *Handlers) codeTreeEntryRows(ctx context.Context, cc *codeContext, entries []repogit.TreeEntry) []codeTreeEntryRow { +// codeTreeEntryRows builds the tree listing's rows, including the +// last-commit column. +// +// The column used to cost one `git log -1 -- ` per entry, run +// serially: a 100-entry directory forked git 100 times for one +// anonymous page view. It is now one cached `git log --name-only` +// walk for the whole directory (see repogit.EntryLastCommits), with +// the per-path query kept only as the fallback for entries the walk +// could not resolve inside its commit bound. +// +// commitOID is the resolved OID of the commit being rendered and is +// the cache key's invalidation lever; pass "" (unresolvable ref) to +// bypass the cache. +func (h *Handlers) codeTreeEntryRows( + ctx context.Context, cc *codeContext, entries []repogit.TreeEntry, commitOID string, +) []codeTreeEntryRow { submodules := map[string]repogit.Submodule{} if hasSubmoduleEntries(entries) { var err error @@ -29,6 +45,8 @@ func (h *Handlers) codeTreeEntryRows(ctx context.Context, cc *codeContext, entri } } + lastCommits := h.entryLastCommits(ctx, cc, commitOID, entries) + rows := make([]codeTreeEntryRow, 0, len(entries)) for _, e := range entries { fullPath := joinPath(cc.subpath, e.Name) @@ -44,21 +62,79 @@ func (h *Handlers) codeTreeEntryRows(ctx context.Context, cc *codeContext, entri FullPath: fullPath, URL: entryURL, } - if commits, err := repogit.Log(ctx, cc.gitDir, repogit.LogOptions{ - Ref: cc.ref, - MaxCount: 1, - Path: fullPath, - }); err == nil && len(commits) > 0 { - row.LastCommit = commits[0] + if commit, ok := lastCommits[e.Name]; ok { + row.LastCommit = commit + row.LastFound = true + } else if commit, ok := h.entryLastCommitFallback(ctx, cc, fullPath); ok { + row.LastCommit = commit row.LastFound = true - } else if err != nil && h.d.Logger != nil { - h.d.Logger.WarnContext(ctx, "code: row history", "error", err, "path", fullPath) } rows = append(rows, row) } return rows } +// entryLastCommits resolves the whole directory's last-commit column +// in one walk, memoized on (repo, commit OID, subpath). A failure +// returns an empty map, which degrades to the old one-fork-per-entry +// behavior rather than dropping the column. +func (h *Handlers) entryLastCommits( + ctx context.Context, cc *codeContext, commitOID string, entries []repogit.TreeEntry, +) map[string]repogit.Commit { + if len(entries) == 0 { + return nil + } + names := make([]string, 0, len(entries)) + for _, e := range entries { + names = append(names, e.Name) + } + // Walk from the resolved OID when we have one so the walk and the + // cache key describe the same commit even if the ref moves + // mid-request. + walkRef := commitOID + if walkRef == "" { + walkRef = cc.ref + } + key := treecache.EntryKey{RepoID: cc.row.ID, CommitOID: commitOID, Subpath: cc.subpath} + out, err := h.d.TreeCache.LastCommits(ctx, key, func(ctx context.Context) (map[string]repogit.Commit, error) { + return repogit.EntryLastCommits(ctx, cc.gitDir, repogit.LastCommitOptions{ + Ref: walkRef, + Dir: cc.subpath, + Names: names, + }) + }) + if err != nil { + if h.d.Logger != nil { + h.d.Logger.WarnContext(ctx, "code: entry last commits", "error", err, "path", cc.subpath) + } + return nil + } + return out +} + +// entryLastCommitFallback is the per-path `git log -1` the walk +// replaces. It runs only for entries the walk left unresolved — +// entries untouched within the walk bound, or paths git had to quote. +func (h *Handlers) entryLastCommitFallback( + ctx context.Context, cc *codeContext, fullPath string, +) (repogit.Commit, bool) { + commits, err := repogit.Log(ctx, cc.gitDir, repogit.LogOptions{ + Ref: cc.ref, + MaxCount: 1, + Path: fullPath, + }) + if err != nil { + if h.d.Logger != nil { + h.d.Logger.WarnContext(ctx, "code: row history", "error", err, "path", fullPath) + } + return repogit.Commit{}, false + } + if len(commits) == 0 { + return repogit.Commit{}, false + } + return commits[0], true +} + func hasSubmoduleEntries(entries []repogit.TreeEntry) bool { for _, e := range entries { if e.Kind == repogit.EntrySubmod { diff --git a/internal/web/handlers/repo/repo.go b/internal/web/handlers/repo/repo.go index 0d8611b5..2c8fe08a 100644 --- a/internal/web/handlers/repo/repo.go +++ b/internal/web/handlers/repo/repo.go @@ -38,6 +38,7 @@ import ( "github.com/tenseleyFlow/shithub/internal/repos/templates" usersdb "github.com/tenseleyFlow/shithub/internal/users/sqlc" "github.com/tenseleyFlow/shithub/internal/web/handlers/repo/httpcache" + "github.com/tenseleyFlow/shithub/internal/web/handlers/repo/treecache" "github.com/tenseleyFlow/shithub/internal/web/middleware" "github.com/tenseleyFlow/shithub/internal/web/render" ) @@ -98,6 +99,14 @@ type Deps struct { // ETag layer is process-independent and stays correct // regardless. CommitsPageCache *httpcache.PageCache + // TreeCache memoizes the code tab's git reads (last-commit per + // tree entry, `rev-list --count`, the language aggregate, the + // contributor tally) per rendered commit OID. nil disables + // caching, which is what tests and degraded boot paths get: every + // call simply falls through to git. Production wires one per + // process from repo_wiring.go. There is no invalidation hook — + // the key carries the commit OID, so a push changes the key. + TreeCache *treecache.Cache } // Handlers is the registered handler set. Construct via New. diff --git a/internal/web/handlers/repo/treecache/treecache.go b/internal/web/handlers/repo/treecache/treecache.go new file mode 100644 index 00000000..57de3da8 --- /dev/null +++ b/internal/web/handlers/repo/treecache/treecache.go @@ -0,0 +1,200 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +// Package treecache is the in-process cache behind the repo home / +// code tab, the most-crawled page on the site. +// +// Rendering one directory listing used to cost, per request and with +// no caching at all: one `git log -1` per tree entry, one +// `git rev-list --count`, one `git log -n 500` for the contributor +// strip, and one recursive `git ls-tree -r` for the language bar. A +// crawler walking a 100-entry directory therefore forked git ~104 +// times per page view, anonymously. +// +// Every value here is keyed by the *commit OID* being rendered, so +// invalidation is structural: a push moves the ref, the OID changes, +// and the new key misses cleanly while the old entries age out via +// TTL and LRU eviction. There is no invalidation hook to forget to +// call — the only staleness window is a mutation that does NOT change +// the commit OID, which for these four values cannot happen. +// +// Construction follows the httpcache.PageCache pattern: exactly one +// Cache per process, built in internal/web/repo_wiring.go and passed +// through repo.Deps. Every method is nil-receiver safe so tests and +// degraded boot paths can leave it unwired and simply always miss. +package treecache + +import ( + "context" + "strconv" + "time" + + "github.com/tenseleyFlow/shithub/internal/cache/lru" + gitops "github.com/tenseleyFlow/shithub/internal/repos/git" +) + +// EntryKey identifies one directory listing's last-commit column: +// the repo, the commit being rendered, and the subpath inside it +// ("" for the repo root). +type EntryKey struct { + RepoID int64 + CommitOID string + Subpath string +} + +// RevKey identifies whole-revision data — anything derived from the +// commit as a whole rather than from one directory in it. +type RevKey struct { + RepoID int64 + CommitOID string +} + +// ContributorTally is one author's commit count within the bounded +// contributor walk. Caching the tally rather than the raw commit list +// keeps the entry proportional to the number of distinct authors +// (single digits for most repos) instead of to the 500-commit window, +// and leaves identity resolution — which reads the users table — on +// the per-request path where it belongs. +type ContributorTally struct { + Name string + Email string + Count int +} + +// Sizes are deliberately modest: this process shares a 3.9 GB box +// with Postgres and has a 1200 MiB GOMEMLIMIT, and the whole point of +// the availability campaign is to stop it from being the largest +// thing on the machine. +// +// - LastCommits: one entry per (repo, commit, directory). The value +// is a map of basename → Commit with no body, ≈150 B per row, so a +// typical 10-entry directory is ~1.5 KB and a 200-entry directory +// is ~30 KB. 2,048 entries is a few MB in practice and ~60 MB in +// an adversarial worst case that no real repo set produces. +// - CommitCounts: an int. 2,048 entries is noise. +// - Languages: a name → bytes map, ≤ ~20 rows after aggregation. +// - Contributors: one row per distinct author in the last 500 +// commits. In theory unbounded (500 commits could be 500 distinct +// authors), so this and Languages get capacity/4 slots. +// +// The 10-minute TTL is a backstop, not the correctness mechanism — +// the OID in the key is. It exists so a repo that stops being visited +// releases its memory instead of squatting an LRU slot until eviction +// pressure arrives. +const ( + // DefaultCapacity bounds the two cheap caches; the heavier + // language and contributor caches get DefaultCapacity/heavyCapRatio. + DefaultCapacity = 2048 + // DefaultTTL is the memory-release backstop, not the correctness + // mechanism — the commit OID in the key is. + DefaultTTL = 10 * time.Minute + + heavyCapRatio = 4 + minCapacity = 1 +) + +// Cache bundles the four code-tab caches. Each is an lru.Group, so a +// burst of concurrent misses on the same hot key (exactly what a +// crawler produces) collapses into one git invocation. +type Cache struct { + lastCommits *lru.Group[EntryKey, map[string]gitops.Commit] + commitCounts *lru.Group[RevKey, int] + languages *lru.Group[RevKey, map[string]int64] + contributors *lru.Group[RevKey, []ContributorTally] +} + +// New builds the process-wide cache. capacity bounds the two cheap +// caches; the heavier language and contributor caches get +// capacity/heavyCapRatio, floored at 1. A non-positive capacity or +// TTL returns nil, which every method treats as "always miss". +func New(capacity int, ttl time.Duration) *Cache { + if capacity <= 0 || ttl <= 0 { + return nil + } + heavy := capacity / heavyCapRatio + if heavy < minCapacity { + heavy = minCapacity + } + return &Cache{ + lastCommits: lru.NewGroup( + lru.NewWithTTL[EntryKey, map[string]gitops.Commit](capacity, ttl), entryKeyString), + commitCounts: lru.NewGroup( + lru.NewWithTTL[RevKey, int](capacity, ttl), revKeyString), + languages: lru.NewGroup( + lru.NewWithTTL[RevKey, map[string]int64](heavy, ttl), revKeyString), + contributors: lru.NewGroup( + lru.NewWithTTL[RevKey, []ContributorTally](heavy, ttl), revKeyString), + } +} + +func entryKeyString(k EntryKey) string { + return strconv.FormatInt(k.RepoID, 10) + "|" + k.CommitOID + "|" + k.Subpath +} + +func revKeyString(k RevKey) string { + return strconv.FormatInt(k.RepoID, 10) + "|" + k.CommitOID +} + +// LastCommits returns the cached last-commit-per-entry map for key, +// invoking fetch on a miss. Keys with an empty CommitOID bypass the +// cache entirely — an unresolvable ref has no stable identity to key +// on, and caching under "" would collide across revisions. +func (c *Cache) LastCommits( + ctx context.Context, key EntryKey, + fetch func(context.Context) (map[string]gitops.Commit, error), +) (map[string]gitops.Commit, error) { + if c == nil || key.CommitOID == "" { + return fetch(ctx) + } + return c.lastCommits.Do(ctx, key, fetch) +} + +// CommitCount returns the cached `rev-list --count` for key. +func (c *Cache) CommitCount( + ctx context.Context, key RevKey, fetch func(context.Context) (int, error), +) (int, error) { + if c == nil || key.CommitOID == "" { + return fetch(ctx) + } + return c.commitCounts.Do(ctx, key, fetch) +} + +// Languages returns the cached language → bytes aggregate for key. +// This is the `ls-tree -r`-derived value: the recursive walk itself is +// never cached, only the handful of numbers it reduces to. +func (c *Cache) Languages( + ctx context.Context, key RevKey, fetch func(context.Context) (map[string]int64, error), +) (map[string]int64, error) { + if c == nil || key.CommitOID == "" { + return fetch(ctx) + } + return c.languages.Do(ctx, key, fetch) +} + +// Contributors returns the cached author tally for key — the reduced +// form of the bounded `git log -n 500` the About sidebar runs. +func (c *Cache) Contributors( + ctx context.Context, key RevKey, fetch func(context.Context) ([]ContributorTally, error), +) ([]ContributorTally, error) { + if c == nil || key.CommitOID == "" { + return fetch(ctx) + } + return c.contributors.Do(ctx, key, fetch) +} + +// Stats reports the four caches' hit/miss/eviction counters, summed. +// Nil receiver returns the zero value so callers don't have to branch. +func (c *Cache) Stats() lru.Stats { + if c == nil { + return lru.Stats{} + } + var out lru.Stats + for _, s := range []lru.Stats{ + c.lastCommits.Stats(), c.commitCounts.Stats(), + c.languages.Stats(), c.contributors.Stats(), + } { + out.Hits += s.Hits + out.Misses += s.Misses + out.Evictions += s.Evictions + } + return out +} diff --git a/internal/web/handlers/repo/treecache/treecache_test.go b/internal/web/handlers/repo/treecache/treecache_test.go new file mode 100644 index 00000000..84be8fdb --- /dev/null +++ b/internal/web/handlers/repo/treecache/treecache_test.go @@ -0,0 +1,343 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package treecache + +import ( + "context" + "errors" + "sync" + "testing" + "time" + + gitops "github.com/tenseleyFlow/shithub/internal/repos/git" +) + +func commitMap(subject string) map[string]gitops.Commit { + return map[string]gitops.Commit{"README.md": {Subject: subject}} +} + +// countingFetch returns a fetch func plus a pointer to its call count. +func countingFetch(subject string) (func(context.Context) (map[string]gitops.Commit, error), *int) { + calls := 0 + return func(context.Context) (map[string]gitops.Commit, error) { + calls++ + return commitMap(subject), nil + }, &calls +} + +func TestLastCommits_SecondCallIsAHit(t *testing.T) { + t.Parallel() + c := New(8, time.Minute) + key := EntryKey{RepoID: 1, CommitOID: "aaa", Subpath: ""} + fetch, calls := countingFetch("first") + + for i := 0; i < 3; i++ { + got, err := c.LastCommits(context.Background(), key, fetch) + if err != nil { + t.Fatalf("LastCommits: %v", err) + } + if got["README.md"].Subject != "first" { + t.Fatalf("subject = %q, want %q", got["README.md"].Subject, "first") + } + } + if *calls != 1 { + t.Errorf("fetch called %d times, want 1", *calls) + } + // lru.Group re-checks the cache after winning the single-flight + // slot, so one cold key records two misses. + if s := c.Stats(); s.Hits != 2 || s.Misses != 2 { + t.Errorf("stats = %+v, want 2 hits / 2 misses", s) + } +} + +func TestLastCommits_NewCommitOIDMissesAndRefetches(t *testing.T) { + t.Parallel() + c := New(8, time.Minute) + oldKey := EntryKey{RepoID: 1, CommitOID: "old-oid"} + newKey := EntryKey{RepoID: 1, CommitOID: "new-oid"} + + oldFetch, oldCalls := countingFetch("before push") + if _, err := c.LastCommits(context.Background(), oldKey, oldFetch); err != nil { + t.Fatalf("LastCommits(old): %v", err) + } + // A push lands: the ref now points at a different commit, so the + // key changes and the cache must not serve the pre-push value. + newFetch, newCalls := countingFetch("after push") + got, err := c.LastCommits(context.Background(), newKey, newFetch) + if err != nil { + t.Fatalf("LastCommits(new): %v", err) + } + if got["README.md"].Subject != "after push" { + t.Errorf("subject = %q, want %q — stale entry served across an OID change", + got["README.md"].Subject, "after push") + } + if *oldCalls != 1 || *newCalls != 1 { + t.Errorf("fetch calls: old=%d new=%d, want 1 and 1", *oldCalls, *newCalls) + } +} + +func TestLastCommits_KeyComponentsAreIndependent(t *testing.T) { + t.Parallel() + c := New(8, time.Minute) + base := EntryKey{RepoID: 1, CommitOID: "oid", Subpath: "src"} + for _, k := range []EntryKey{ + base, + {RepoID: 2, CommitOID: "oid", Subpath: "src"}, + {RepoID: 1, CommitOID: "other", Subpath: "src"}, + {RepoID: 1, CommitOID: "oid", Subpath: "docs"}, + } { + fetch, calls := countingFetch("x") + if _, err := c.LastCommits(context.Background(), k, fetch); err != nil { + t.Fatalf("LastCommits(%+v): %v", k, err) + } + if *calls != 1 { + t.Errorf("key %+v collided with an earlier key (fetch calls = %d)", k, *calls) + } + } +} + +func TestLastCommits_EmptyOIDBypassesTheCache(t *testing.T) { + t.Parallel() + c := New(8, time.Minute) + key := EntryKey{RepoID: 1, CommitOID: ""} + fetch, calls := countingFetch("x") + for i := 0; i < 3; i++ { + if _, err := c.LastCommits(context.Background(), key, fetch); err != nil { + t.Fatalf("LastCommits: %v", err) + } + } + if *calls != 3 { + t.Errorf("fetch called %d times, want 3 (an unresolved ref must not be cached)", *calls) + } +} + +func TestLastCommits_TTLExpires(t *testing.T) { + t.Parallel() + c := New(8, 5*time.Millisecond) + key := EntryKey{RepoID: 1, CommitOID: "aaa"} + fetch, calls := countingFetch("x") + if _, err := c.LastCommits(context.Background(), key, fetch); err != nil { + t.Fatalf("LastCommits: %v", err) + } + time.Sleep(25 * time.Millisecond) + if _, err := c.LastCommits(context.Background(), key, fetch); err != nil { + t.Fatalf("LastCommits: %v", err) + } + if *calls != 2 { + t.Errorf("fetch called %d times, want 2 (TTL should have expired the entry)", *calls) + } +} + +func TestLastCommits_EvictsLeastRecentlyUsed(t *testing.T) { + t.Parallel() + c := New(2, time.Minute) + k1 := EntryKey{RepoID: 1, CommitOID: "a"} + k2 := EntryKey{RepoID: 1, CommitOID: "b"} + k3 := EntryKey{RepoID: 1, CommitOID: "c"} + f1, c1 := countingFetch("1") + f2, c2 := countingFetch("2") + f3, c3 := countingFetch("3") + ctx := context.Background() + mustFetch := func(k EntryKey, f func(context.Context) (map[string]gitops.Commit, error)) { + t.Helper() + if _, err := c.LastCommits(ctx, k, f); err != nil { + t.Fatalf("LastCommits(%+v): %v", k, err) + } + } + mustFetch(k1, f1) + mustFetch(k2, f2) + mustFetch(k3, f3) + mustFetch(k1, f1) // k1 was evicted → refetch + mustFetch(k3, f3) // k3 is still resident → hit + if *c1 != 2 { + t.Errorf("k1 fetches = %d, want 2 (should have been evicted)", *c1) + } + if *c2 != 1 || *c3 != 1 { + t.Errorf("k2/k3 fetches = %d/%d, want 1/1", *c2, *c3) + } +} + +func TestLastCommits_ErrorsAreNotCached(t *testing.T) { + t.Parallel() + c := New(8, time.Minute) + key := EntryKey{RepoID: 1, CommitOID: "aaa"} + boom := errors.New("git exploded") + calls := 0 + fetch := func(context.Context) (map[string]gitops.Commit, error) { + calls++ + if calls == 1 { + return nil, boom + } + return commitMap("recovered"), nil + } + if _, err := c.LastCommits(context.Background(), key, fetch); !errors.Is(err, boom) { + t.Fatalf("first call error = %v, want %v", err, boom) + } + got, err := c.LastCommits(context.Background(), key, fetch) + if err != nil { + t.Fatalf("second call: %v", err) + } + if got["README.md"].Subject != "recovered" { + t.Errorf("subject = %q; a transient failure must not poison the key", got["README.md"].Subject) + } +} + +func TestLastCommits_ConcurrentMissesCollapseToOneFetch(t *testing.T) { + t.Parallel() + c := New(8, time.Minute) + key := EntryKey{RepoID: 1, CommitOID: "aaa"} + release := make(chan struct{}) + var mu sync.Mutex + calls := 0 + fetch := func(context.Context) (map[string]gitops.Commit, error) { + mu.Lock() + calls++ + mu.Unlock() + <-release + return commitMap("x"), nil + } + + const racers = 16 + var wg sync.WaitGroup + wg.Add(racers) + for i := 0; i < racers; i++ { + go func() { + defer wg.Done() + _, _ = c.LastCommits(context.Background(), key, fetch) + }() + } + // Give the racers time to pile onto the single-flight slot, then + // let the one real fetch finish. + time.Sleep(20 * time.Millisecond) + close(release) + wg.Wait() + + mu.Lock() + defer mu.Unlock() + if calls != 1 { + t.Errorf("fetch called %d times under %d concurrent misses, want 1", calls, racers) + } +} + +func TestCommitCount_CachesPerRevision(t *testing.T) { + t.Parallel() + c := New(8, time.Minute) + calls := 0 + fetch := func(context.Context) (int, error) { calls++; return 42, nil } + key := RevKey{RepoID: 1, CommitOID: "aaa"} + for i := 0; i < 3; i++ { + got, err := c.CommitCount(context.Background(), key, fetch) + if err != nil { + t.Fatalf("CommitCount: %v", err) + } + if got != 42 { + t.Fatalf("count = %d, want 42", got) + } + } + if calls != 1 { + t.Errorf("fetch called %d times, want 1", calls) + } + // New head OID → new key → refetch. + if _, err := c.CommitCount(context.Background(), RevKey{RepoID: 1, CommitOID: "bbb"}, fetch); err != nil { + t.Fatalf("CommitCount(new oid): %v", err) + } + if calls != 2 { + t.Errorf("fetch called %d times after an OID change, want 2", calls) + } +} + +func TestLanguagesAndContributors_CacheAndInvalidateOnOIDChange(t *testing.T) { + t.Parallel() + c := New(8, time.Minute) + ctx := context.Background() + + langCalls := 0 + langFetch := func(context.Context) (map[string]int64, error) { + langCalls++ + return map[string]int64{"Go": 100}, nil + } + contribCalls := 0 + contribFetch := func(context.Context) ([]ContributorTally, error) { + contribCalls++ + return []ContributorTally{{Name: "A", Email: "a@e", Count: 3}}, nil + } + + old := RevKey{RepoID: 7, CommitOID: "old"} + fresh := RevKey{RepoID: 7, CommitOID: "new"} + for i := 0; i < 2; i++ { + if _, err := c.Languages(ctx, old, langFetch); err != nil { + t.Fatalf("Languages: %v", err) + } + if _, err := c.Contributors(ctx, old, contribFetch); err != nil { + t.Fatalf("Contributors: %v", err) + } + } + if langCalls != 1 || contribCalls != 1 { + t.Fatalf("cold fetches: lang=%d contrib=%d, want 1 and 1", langCalls, contribCalls) + } + if _, err := c.Languages(ctx, fresh, langFetch); err != nil { + t.Fatalf("Languages(new): %v", err) + } + if _, err := c.Contributors(ctx, fresh, contribFetch); err != nil { + t.Fatalf("Contributors(new): %v", err) + } + if langCalls != 2 || contribCalls != 2 { + t.Errorf("after OID change: lang=%d contrib=%d, want 2 and 2", langCalls, contribCalls) + } +} + +func TestNew_RejectsZeroOrNegativeInputs(t *testing.T) { + t.Parallel() + if got := New(0, time.Minute); got != nil { + t.Errorf("capacity 0 should return nil, got %+v", got) + } + if got := New(-1, time.Minute); got != nil { + t.Errorf("negative capacity should return nil, got %+v", got) + } + if got := New(8, 0); got != nil { + t.Errorf("ttl 0 should return nil, got %+v", got) + } +} + +func TestNew_TinyCapacityStillBuildsHeavyCaches(t *testing.T) { + t.Parallel() + // capacity/heavyCapRatio floors at 1 rather than panicking the + // underlying LRU with a zero capacity. + c := New(1, time.Minute) + if c == nil { + t.Fatal("New(1, ttl) = nil") + } + if _, err := c.Languages(context.Background(), RevKey{RepoID: 1, CommitOID: "a"}, + func(context.Context) (map[string]int64, error) { return nil, nil }); err != nil { + t.Fatalf("Languages: %v", err) + } +} + +func TestNilCache_AlwaysFetchesAndReportsZeroStats(t *testing.T) { + t.Parallel() + var c *Cache + ctx := context.Background() + calls := 0 + if _, err := c.LastCommits(ctx, EntryKey{RepoID: 1, CommitOID: "a"}, + func(context.Context) (map[string]gitops.Commit, error) { calls++; return nil, nil }); err != nil { + t.Fatalf("LastCommits: %v", err) + } + if _, err := c.CommitCount(ctx, RevKey{RepoID: 1, CommitOID: "a"}, + func(context.Context) (int, error) { calls++; return 0, nil }); err != nil { + t.Fatalf("CommitCount: %v", err) + } + if _, err := c.Languages(ctx, RevKey{RepoID: 1, CommitOID: "a"}, + func(context.Context) (map[string]int64, error) { calls++; return nil, nil }); err != nil { + t.Fatalf("Languages: %v", err) + } + if _, err := c.Contributors(ctx, RevKey{RepoID: 1, CommitOID: "a"}, + func(context.Context) ([]ContributorTally, error) { calls++; return nil, nil }); err != nil { + t.Fatalf("Contributors: %v", err) + } + if calls != 4 { + t.Errorf("nil cache made %d fetches, want 4", calls) + } + if got := c.Stats(); got.Hits != 0 || got.Misses != 0 || got.Evictions != 0 { + t.Errorf("nil-receiver Stats = %+v, want zero", got) + } +} diff --git a/internal/web/repo_wiring.go b/internal/web/repo_wiring.go index 307cca37..a70eded2 100644 --- a/internal/web/repo_wiring.go +++ b/internal/web/repo_wiring.go @@ -20,6 +20,7 @@ import ( "github.com/tenseleyFlow/shithub/internal/ratelimit" repoh "github.com/tenseleyFlow/shithub/internal/web/handlers/repo" "github.com/tenseleyFlow/shithub/internal/web/handlers/repo/httpcache" + "github.com/tenseleyFlow/shithub/internal/web/handlers/repo/treecache" "github.com/tenseleyFlow/shithub/internal/web/render" ) @@ -32,6 +33,16 @@ const ( commitsPageCacheTTL = 60 * time.Second ) +// Code-tab cache sizing. 2,048 entries covers the working set of a +// crawler walking every directory of every repo we host several times +// over; the 10-minute TTL is only the memory-release backstop, since +// correctness comes from the commit OID in the key. See +// docs/internal/caching.md and internal/web/handlers/repo/treecache. +const ( + treeCacheCap = treecache.DefaultCapacity + treeCacheTTL = treecache.DefaultTTL +) + // buildRepoHandlers wires the repo-create + empty-home handlers. The // bare repos live at cfg.Storage.ReposRoot (must be set; we refuse to // boot the repo surface without it). @@ -99,5 +110,6 @@ func buildRepoHandlers( }, BillingEnforce: cfg.Billing.Enforce, CommitsPageCache: httpcache.NewPageCache(commitsPageCacheCap, commitsPageCacheTTL), + TreeCache: treecache.New(treeCacheCap, treeCacheTTL), }) }