diff --git a/docs/internal/validation.md b/docs/internal/validation.md index 166db71..b1fc038 100644 --- a/docs/internal/validation.md +++ b/docs/internal/validation.md @@ -52,8 +52,11 @@ must close it when finished. The shared validation executor closes its tree on every return before product checks or build callbacks run. A metadata-only selection never starts the reader. Per-read contexts cancel an in-flight blob exchange without tying the reader lifetime to a short-lived caller context. -Until formatting and parsed-result reuse are in place, regular-file bytes stay -cached by object ID for the lifetime of the tree. +Regular-file reads return independent caller-owned byte slices and are not +retained by the tree. Symlink targets are retained as immutable owned strings +by blob object ID; explicitly reading a symlink still returns independent +bytes. The Go-format check caches only whether each successfully inspected +object is formatted, then reports every unformatted path that refers to it. The internal Markdown parser uses Goldmark to record inline, reference, and autolinks, images, titles, and their source use sites, and to build heading diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index c500f93..40dddc1 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -1,6 +1,6 @@ # Computational Efficiency Implementation Plan -**Status:** Stages 1–5 complete; Stages 6–11 pending. +**Status:** Stages 1–6 complete; Stages 7–11 pending. ## Purpose And Baseline @@ -359,6 +359,22 @@ buffers after formatting, and duplicate-content files retain per-path findings. Record the tradeoff for explicit repeated raw reads. Update validation mechanics with the new ownership and reuse choices. +**Completion:** Complete. Regular-file reads now return independent bytes and +are not retained by `CandidateTree`; repeated explicit reads therefore transfer +the blob again. The tree caches owned symlink-target strings by object ID, and +explicit symlink reads still return independent byte slices. Go formatting +caches only successful formatted/unformatted outcomes by object ID, checks +cancellation per path, and reports every unformatted path even when contents +repeat. Validation tests and the formatting/Markdown destination benchmarks +passed. On the same Go 1.26.8, Git 2.53.0, Linux x86_64 Intel Core i5-7260U +setup, three repetitions measured unique-file formatting at 75.0–75.9 ms loose +and 37.9–42.5 ms packed, repeated-ID formatting at 1.97–2.17 ms, and the +Markdown workload at 16.3–17.0 ms. Formatting used about 2.89 MB/op for unique +files and 121 KB/op for repeated IDs. The retained-memory observation was +129,496 bytes with the candidate and Markdown cache alive, compared with +1,385,040–1,385,616 bytes in the baseline fixture. This retains Markdown parser +data by path until Stage 7; it is a workload-specific heap observation. + ## Stage 7: Reuse Owned Markdown And Module Results **Owners:** [markdown.go](../../internal/validation/markdown.go), diff --git a/internal/validation/format_test.go b/internal/validation/format_test.go index c4563c7..08d203e 100644 --- a/internal/validation/format_test.go +++ b/internal/validation/format_test.go @@ -90,7 +90,6 @@ func TestCheckGoFormatPreservesParserAndInspectionCauses(t *testing.T) { repositoryDir: t.TempDir(), entries: []git.TreeEntry{{Path: "missing.go", ObjectID: strings.Repeat("0", 40), Mode: "100644", Type: "blob"}}, byPath: map[string]int{"missing.go": 0}, - blobs: make(map[string][]byte), } cleanupCandidateTree(t, tree) err := checkGoFormat(context.Background(), tree) @@ -104,6 +103,33 @@ func TestCheckGoFormatPreservesParserAndInspectionCauses(t *testing.T) { }) } +func TestCheckGoFormatReportsEveryPathForRepeatedUnformattedContent(t *testing.T) { + repository := newTreeRepository(t) + contents := []byte("package app\nfunc value(){return}\n") + writeTreeFile(t, repository, "first.go", contents) + writeTreeFile(t, repository, "nested/second.go", contents) + commit := commitCandidateTree(t, repository) + tree, err := ReadCandidateTree(context.Background(), repository, commit) + if err != nil { + t.Fatal(err) + } + cleanupCandidateTree(t, tree) + + err = checkGoFormat(context.Background(), tree) + var failure *Failure + if !errors.As(err, &failure) || failure.Check != GoFormat { + t.Fatalf("checkGoFormat() error = %v, want go-format findings", err) + } + if len(failure.Findings) != 2 || failure.Findings[0].Path != "first.go" || failure.Findings[1].Path != "nested/second.go" { + t.Fatalf("findings = %#v, want both paths in tree order", failure.Findings) + } + first, _ := tree.Entry("first.go") + second, _ := tree.Entry("nested/second.go") + if first.ObjectID != second.ObjectID { + t.Fatalf("fixture object IDs = %q and %q, want repeated content", first.ObjectID, second.ObjectID) + } +} + func TestRunGoFormatIsOptionalAndStopsAfterFinding(t *testing.T) { repository := newTreeRepository(t) writeTreeFile(t, repository, "unformatted.go", []byte("package fixture\nfunc Value( ){return 1}\n")) diff --git a/internal/validation/local_replacements.go b/internal/validation/local_replacements.go index 3b590c5..4fafd19 100644 --- a/internal/validation/local_replacements.go +++ b/internal/validation/local_replacements.go @@ -69,11 +69,11 @@ func readModuleDeclaration(ctx context.Context, tree *CandidateTree, entry git.T return contents, nil } - target, err := tree.ReadFile(ctx, entry.Path) + target, err := tree.readSymlinkTarget(ctx, entry) if err != nil { return nil, fmt.Errorf("read symlink declaration: %w", err) } - destination, err := tree.ResolveDestination(ctx, entry.Path, string(target)) + destination, err := tree.ResolveDestination(ctx, entry.Path, target) if err != nil { return nil, fmt.Errorf("resolve symlink declaration target %q: %w", target, err) } diff --git a/internal/validation/performance_test.go b/internal/validation/performance_test.go index d09bdec..ae8e440 100644 --- a/internal/validation/performance_test.go +++ b/internal/validation/performance_test.go @@ -229,7 +229,7 @@ func freshPerformanceTrees(tb testing.TB, fixture performanceTree, count int) [] trees := make([]*CandidateTree, count) for index := range trees { tree := *base - tree.blobs = make(map[string][]byte) + tree.symlinkTargets = make(map[string]string) trees[index] = &tree } if err := base.Close(); err != nil { diff --git a/internal/validation/tree.go b/internal/validation/tree.go index 54a56c1..5bcb7f2 100644 --- a/internal/validation/tree.go +++ b/internal/validation/tree.go @@ -13,14 +13,14 @@ import ( // CandidateTree is an inventory of one captured commit with blob contents // loaded lazily by object ID. type CandidateTree struct { - repositoryDir string - entries []git.TreeEntry - byPath map[string]int - directories map[string]struct{} - blobs map[string][]byte - blobReader *git.BlobReader - closed bool - closeErr error + repositoryDir string + entries []git.TreeEntry + byPath map[string]int + directories map[string]struct{} + blobReader *git.BlobReader + symlinkTargets map[string]string + closed bool + closeErr error } // ReadCandidateTree enumerates the captured commit without reading from the @@ -31,11 +31,11 @@ func ReadCandidateTree(ctx context.Context, repositoryDir, commit string) (*Cand return nil, err } tree := &CandidateTree{ - repositoryDir: repositoryDir, - entries: entries, - byPath: make(map[string]int, len(entries)), - directories: make(map[string]struct{}), - blobs: make(map[string][]byte), + repositoryDir: repositoryDir, + entries: entries, + byPath: make(map[string]int, len(entries)), + directories: make(map[string]struct{}), + symlinkTargets: make(map[string]string), } tree.directories["."] = struct{}{} for index, entry := range entries { @@ -52,8 +52,9 @@ func (tree *CandidateTree) Entries() []git.TreeEntry { return append([]git.TreeEntry(nil), tree.entries...) } -// ReadFile returns the committed blob bytes for path. Contents are cached by -// object ID for later reads from this tree. +// ReadFile returns newly allocated, caller-owned bytes for a committed blob. +// Regular-file content is not cached, so repeated reads request the object +// again through the tree's scoped blob reader. func (tree *CandidateTree) ReadFile(ctx context.Context, path string) ([]byte, error) { if tree.closed { return nil, fmt.Errorf("read committed path %q: candidate tree is closed", path) @@ -66,8 +67,41 @@ func (tree *CandidateTree) ReadFile(ctx context.Context, path string) ([]byte, e if entry.Type != "blob" { return nil, fmt.Errorf("committed path %q is a %s entry, not a blob", path, entry.Type) } - if contents, ok := tree.blobs[entry.ObjectID]; ok { - return append([]byte(nil), contents...), nil + if entry.Mode == "120000" { + target, err := tree.readSymlinkTarget(ctx, entry) + if err != nil { + return nil, err + } + return []byte(target), nil + } + return tree.readBlob(ctx, entry) +} + +func (tree *CandidateTree) readSymlinkTarget(ctx context.Context, entry git.TreeEntry) (string, error) { + if tree.closed { + return "", fmt.Errorf("read committed path %q: candidate tree is closed", entry.Path) + } + if entry.Type != "blob" || entry.Mode != "120000" { + return "", fmt.Errorf("committed path %q is not a symlink blob", entry.Path) + } + if target, ok := tree.symlinkTargets[entry.ObjectID]; ok { + return target, nil + } + contents, err := tree.readBlob(ctx, entry) + if err != nil { + return "", err + } + target := string(contents) + if tree.symlinkTargets == nil { + tree.symlinkTargets = make(map[string]string) + } + tree.symlinkTargets[entry.ObjectID] = target + return target, nil +} + +func (tree *CandidateTree) readBlob(ctx context.Context, entry git.TreeEntry) ([]byte, error) { + if tree.closed { + return nil, fmt.Errorf("read committed path %q: candidate tree is closed", entry.Path) } if tree.blobReader == nil { // The reader belongs to the tree and can outlive an individual read @@ -77,12 +111,8 @@ func (tree *CandidateTree) ReadFile(ctx context.Context, path string) ([]byte, e } contents, err := tree.blobReader.Read(ctx, entry.ObjectID) if err != nil { - return nil, fmt.Errorf("read committed path %q: %w", path, err) + return nil, fmt.Errorf("read committed path %q: %w", entry.Path, err) } - if tree.blobs == nil { - tree.blobs = make(map[string][]byte) - } - tree.blobs[entry.ObjectID] = append([]byte(nil), contents...) return contents, nil } @@ -179,11 +209,12 @@ func (tree *CandidateTree) resolveDestination(ctx context.Context, directory str if active[partial] { return Destination{Kind: DestinationSymlinkCycle, Path: partial}, nil } - linkText, err := tree.ReadFile(ctx, partial) + entry, _ := tree.Entry(partial) + linkText, err := tree.readSymlinkTarget(ctx, entry) if err != nil { return Destination{}, fmt.Errorf("read committed symlink %q: %w", partial, err) } - target := string(linkText) + target := linkText targetDirectory := directory if path.IsAbs(target) { relative, inside, err := tree.relativeAbsoluteDestination(target) diff --git a/internal/validation/tree_test.go b/internal/validation/tree_test.go index 747ba86..cb2a587 100644 --- a/internal/validation/tree_test.go +++ b/internal/validation/tree_test.go @@ -78,6 +78,56 @@ func TestReadCandidateTreeUsesCapturedCommitAndEnumeratesGitEntries(t *testing.T } } +func TestReadFileReturnsOwnedBytesAndCachesOnlySymlinkTargets(t *testing.T) { + repository := newTreeRepository(t) + writeTreeFile(t, repository, "source.txt", []byte("committed content")) + if err := os.Symlink("source.txt", filepath.Join(repository, "alias.txt")); err != nil { + t.Fatal(err) + } + commit := commitCandidateTree(t, repository) + tree, err := ReadCandidateTree(context.Background(), repository, commit) + if err != nil { + t.Fatal(err) + } + cleanupCandidateTree(t, tree) + + first, err := tree.ReadFile(context.Background(), "source.txt") + if err != nil { + t.Fatal(err) + } + first[0] = 'X' + second, err := tree.ReadFile(context.Background(), "source.txt") + if err != nil { + t.Fatal(err) + } + if string(second) != "committed content" { + t.Fatalf("second regular-file read = %q, want committed content", second) + } + if &first[0] == &second[0] { + t.Fatal("regular-file reads share a backing byte slice") + } + + linkFirst, err := tree.ReadFile(context.Background(), "alias.txt") + if err != nil { + t.Fatal(err) + } + linkFirst[0] = 'X' + linkSecond, err := tree.ReadFile(context.Background(), "alias.txt") + if err != nil { + t.Fatal(err) + } + if string(linkSecond) != "source.txt" { + t.Fatalf("second symlink read = %q, want source.txt", linkSecond) + } + if len(tree.symlinkTargets) != 1 { + t.Fatalf("cached symlink target count = %d, want 1", len(tree.symlinkTargets)) + } + destination, err := tree.ResolveDestination(context.Background(), "README.md", "alias.txt") + if err != nil || destination.Kind != DestinationEntry || destination.Path != "source.txt" { + t.Fatalf("ResolveDestination() = %#v, %v; want source.txt entry", destination, err) + } +} + func cleanupCandidateTree(tb testing.TB, tree *CandidateTree) { tb.Helper() tb.Cleanup(func() { _ = tree.Close() }) diff --git a/internal/validation/validation.go b/internal/validation/validation.go index f25cb8e..0386222 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -199,6 +199,7 @@ func checkWorkspaceFiles(tree *CandidateTree) error { func checkGoFormat(ctx context.Context, tree *CandidateTree) error { var findings []Finding + formattedByObjectID := make(map[string]bool) for _, entry := range tree.Entries() { if entry.Type != "blob" || (entry.Mode != "100644" && entry.Mode != "100755") || !strings.HasSuffix(entry.Path, ".go") { continue @@ -206,6 +207,12 @@ func checkGoFormat(ctx context.Context, tree *CandidateTree) error { if err := ctx.Err(); err != nil { return &Failure{Check: GoFormat, Cause: err} } + if formatted, ok := formattedByObjectID[entry.ObjectID]; ok { + if !formatted { + findings = append(findings, Finding{Path: entry.Path, Message: "file is not gofmt-formatted"}) + } + continue + } contents, err := tree.ReadFile(ctx, entry.Path) if err != nil { return &Failure{ @@ -228,7 +235,11 @@ func checkGoFormat(ctx context.Context, tree *CandidateTree) error { Cause: fmt.Errorf("format committed Go file %q: %w", entry.Path, err), } } - if !bytes.Equal(contents, formatted) { + isFormatted := bytes.Equal(contents, formatted) + formattedByObjectID[entry.ObjectID] = isFormatted + contents = nil + formatted = nil + if !isFormatted { findings = append(findings, Finding{Path: entry.Path, Message: "file is not gofmt-formatted"}) } }