perf(vcs): read the reviewed commit from the checkout

docs/findings.md

@@ -2632,14 +2632,28 @@ each read was a separate forge request, made one at a time. The same tree holds
 the answers, so the requests did not depend on each other.
 
 Listing the tree and reading its files now overlap, at most eight requests in
-flight. Across the thirteen CI runs after the change the same tree, all 93
-directories and 396 files, reads in a median of 16.0s, from 7.3s to 40.5s. The
-widest values land on the most loaded runners, so the range is the number to
-keep; the baseline is the one pre-change run at 1m6s. Model batch time swings as
+flight. Across the thirteen CI runs after the change the same 93 directories
+read in a median of 16.0s, from 7.3s to 40.5s. The widest values land on the
+most loaded runners, so the range is the number to keep; the baseline is the
+one pre-change run at 1m6s. Model batch time swings as
 widely, 4s to 2m7s over the same runs with a median near 1m25s, so it is still
 the larger part of a review. What is left to measure is the fixed pre-batch
 step as a whole, not this read alone.
 
+The read still cost one forge request per file and directory, about 490, plus
+two more per file because a read that named no revision fetched the pull
+request and its head commit first. The Action already holds the history, so
+the provider now answers from the checkout when it has the reviewed commit:
+file contents from the commit's objects, listings from one `git ls-tree`, the
+head commit once per pull request. The read covers whatever the reviewed
+commit holds, so the file count rises as this branch adds tests: the
+concurrent runs held 396 or 397 files, the checkout runs 398 or 399. Those
+five reads covered all 93 directories in 290ms, 196ms, 277ms, 285ms and
+165ms, against 16.6s in the concurrent run before it and 1m6s before that.
+`design assembled` fell from 17.1s to a median near 0.9s. Five runs are not a
+distribution; the range from the earlier thirteen is why 16s was reported as a
+median and not a best case.
+
 `TestDesignSourceCaptureOverlapsRequestsAndStaysWithinTheLimit` pins the
 overlap as peak requests in flight rather than wall clock, so a loaded runner
 cannot fail it without a regression. Three siblings pin what the overlap must

internal/vcs/github.go

@@ -37,6 +37,9 @@ type GitHub struct {
 	// more than 300 files; with a checkout the diff is taken from git
 	// instead, which has no such limit.
 	Checkout string
+
+	// index caches each commit's tree read from Checkout.
+	index checkoutIndex
 }
 
 // GitHubOptions configure the provider.
@@ -299,11 +302,16 @@ func (g *GitHub) FileContent(ctx context.Context, ref Ref, path string) ([]byte,
 
 	sha := ref.Head
 	if sha == "" {
-		pr, err := g.PullRequest(ctx, ref)
-		if err != nil {
+		var err error
+		if sha, err = g.headSHA(ctx, ref); err != nil {
 			return nil, err
 		}
-		sha = pr.HeadSHA
+	}
+
+	if tree := g.checkoutTree(ctx, sha); tree != nil && tree.regular(path) {
+		if data, err := (&Local{Dir: g.Checkout}).FileContent(ctx, Ref{Head: sha}, path); err == nil {
+			return data, nil
+		}
 	}
 
 	// DownloadContents falls back to listing the parent even after a file 404,
@@ -361,11 +369,16 @@ func (g *GitHub) ListDir(ctx context.Context, ref Ref, dir string) ([]string, er
 
 	sha := ref.Head
 	if sha == "" {
-		pr, err := g.PullRequest(ctx, ref)
-		if err != nil {
+		var err error
+		if sha, err = g.headSHA(ctx, ref); err != nil {
 			return nil, err
 		}
-		sha = pr.HeadSHA
+	}
+
+	if tree := g.checkoutTree(ctx, sha); tree != nil {
+		if names, ok := tree.dirs[strings.Trim(dir, "/")]; ok {
+			return append([]string(nil), names...), nil
+		}
 	}
 
 	file, entries, resp, err := g.client.Repositories.GetContents(ctx, ref.Owner, ref.Repo, strings.Trim(dir, "/"),

internal/vcs/github_checkout.go

@@ -0,0 +1,181 @@
+package vcs
+
+import (
+	"bytes"
+	"context"
+	"fmt"
+	"log/slog"
+	"sort"
+	"strings"
+	"sync"
+)
+
+// checkoutTree is one commit's file list, read from the checkout in a single
+// git call. Directory listings and file modes are answered from it, so a
+// repository of ninety directories costs one process instead of ninety
+// requests.
+type checkoutTree struct {
+	modes map[string]string
+	dirs  map[string][]string
+}
+
+// checkoutIndex holds the trees read so far, keyed by commit.
+//
+// The lock guards the maps only. A tree is built outside it and then
+// published, so one slow git call does not stall reads of other commits; two
+// callers racing on an uncached commit read it twice and keep one.
+type checkoutIndex struct {
+	mu    sync.Mutex
+	trees map[string]*checkoutTree
+	heads map[string]string
+	// warned holds the commits already reported, so a checkout that keeps
+	// failing logs once and not once per file.
+	warned map[string]bool
+}
+
+// checkoutTree returns the commit's tree from the checkout, or nil when the
+// checkout cannot supply it. A nil answer sends the caller to the API.
+//
+// A commit the checkout does not hold is remembered, because asking again
+// cannot change the answer. A git call that failed after the commit was found
+// is not remembered: it may be transient, and caching it would send every
+// later read of the commit through the API.
+//
+// The commit is read from git objects and never the working tree: the review
+// is of the commit named, and the checkout may have something else out.
+func (g *GitHub) checkoutTree(ctx context.Context, sha string) *checkoutTree {
+	if g.Checkout == "" || sha == "" || strings.HasPrefix(sha, "-") {
+		return nil
+	}
+	g.index.mu.Lock()
+	t, seen := g.index.trees[sha]
+	g.index.mu.Unlock()
+	if seen {
+		return t
+	}
+	t, held, err := readCheckoutTree(ctx, g.Checkout, sha)
+	if err != nil {
+		if ctx.Err() != nil {
+			return nil
+		}
+		g.warnOnce(sha, "checkout could not be read; reading through the API", err)
+		if held {
+			return nil
+		}
+	}
+	g.index.mu.Lock()
+	defer g.index.mu.Unlock()
+	if kept, raced := g.index.trees[sha]; raced {
+		return kept
+	}
+	if g.index.trees == nil {
+		g.index.trees = map[string]*checkoutTree{}
+	}
+	g.index.trees[sha] = t
+	return t
+}
+
+// warnOnce logs a checkout problem the first time it is seen for a commit.
+func (g *GitHub) warnOnce(sha, msg string, err error) {
+	g.index.mu.Lock()
+	seen := g.index.warned[sha]
+	if g.index.warned == nil {
+		g.index.warned = map[string]bool{}
+	}
+	g.index.warned[sha] = true
+	g.index.mu.Unlock()
+	if !seen {
+		slog.Warn(msg, "checkout", g.Checkout, "commit", short(sha), "err", err)
+	}
+}
+
+// readCheckoutTree reads one commit's tree. held reports whether the checkout
+// has the commit: false with an error is a miss to remember, true with an
+// error is a failure that should be retried.
+func readCheckoutTree(ctx context.Context, dir, sha string) (t *checkoutTree, held bool, err error) {
+	local := &Local{Dir: dir}
+	if _, err := local.gitRaw(ctx, "cat-file", "-e", sha+"^{commit}"); err != nil {
+		return nil, false, fmt.Errorf("commit not in checkout: %w", err)
+	}
+	out, err := local.gitRaw(ctx, "ls-tree", "-r", "-t", "-z", sha)
+	if err != nil {
+		return nil, true, fmt.Errorf("list tree: %w", err)
+	}
+	t = &checkoutTree{modes: map[string]string{}, dirs: map[string][]string{}}
+	for _, rec := range bytes.Split(out, []byte{0}) {
+		meta, name, ok := strings.Cut(string(rec), "\t")
+		if !ok || name == "" {
+			continue
+		}
+		fields := strings.Fields(meta)
+		if len(fields) < 2 {
+			continue
+		}
+		parent := ""
+		if i := strings.LastIndex(name, "/"); i >= 0 {
+			parent = name[:i]
+		}
+		base := name[strings.LastIndex(name, "/")+1:]
+		switch fields[1] {
+		case "tree":
+			t.dirs[parent] = append(t.dirs[parent], base+"/")
+			if _, has := t.dirs[name]; !has {
+				t.dirs[name] = nil
+			}
+		case "blob":
+			t.modes[name] = fields[0]
+			// The contents API types a symlink as a file, so it is listed here
+			// too. Only reading one is refused, in regular below.
+			t.dirs[parent] = append(t.dirs[parent], base)
+		case "commit":
+			// A submodule is typed as a file by the contents API as well, and has
+			// no blob here to read.
+			t.modes[name] = fields[0]
+			t.dirs[parent] = append(t.dirs[parent], base)
+		}
+	}
+	for _, names := range t.dirs {
+		sort.Strings(names)
+	}
+	return t, true, nil
+}
+
+// regular reports whether path is a regular file in the tree. A symlink or a
+// submodule is not source, and the API path refuses them with its own error.
+func (t *checkoutTree) regular(path string) bool {
+	mode := t.modes[path]
+	return mode == "100644" || mode == "100755"
+}
+
+// headSHA is the pull request's head commit, asked for once per pull request.
+// It serves file and directory reads only. The approval check, Propose and its
+// final re-read call PullRequest themselves, so a push during the run is still
+// caught there.
+//
+// A read that names no revision used to fetch the pull request and its head
+// commit again for every file, so a review reading four hundred files made
+// well over a thousand requests. It also meant a push during the run could
+// split the reads across two commits.
+func (g *GitHub) headSHA(ctx context.Context, ref Ref) (string, error) {
+	key := ref.String()
+	g.index.mu.Lock()
+	sha, ok := g.index.heads[key]
+	g.index.mu.Unlock()
+	if ok {
+		return sha, nil
+	}
+	pr, err := g.PullRequest(ctx, ref)
+	if err != nil {
+		return "", err
+	}
+	if pr.HeadSHA == "" {
+		return "", nil
+	}
+	g.index.mu.Lock()
+	if g.index.heads == nil {
+		g.index.heads = map[string]string{}
+	}
+	g.index.heads[key] = pr.HeadSHA
+	g.index.mu.Unlock()
+	return pr.HeadSHA, nil
+}