Commit 39ca1bd78c
39ca1bd78c8159113c4af37cccf5d3640aec4c3e
parent: 714995cd86
Verified · cmc
cmc <hello@cleberg.net> · 2026-09-29 05:30 UTC
gitutil, control: BlobBatch skips a refused object without reading it in; suggestion blob cache capped at 8 MiB; range input on the split-layout compose form
Ref #288
Layout: unified · split
internal/control/suggestion.go
+18 −8
| @@ -56,22 +56,29 @@ const ( |
| 56 | 56 | // listing: one ls-tree per (commit, path), and every blob through one |
| 57 | 57 | // cat-file --batch process started on first use, so a merge request with |
| 58 | 58 | // many suggestions on a file costs no more processes than one with a |
| 59 | | // single suggestion. |
| 59 | // single suggestion. Blobs are kept by id up to cacheCap bytes in all; |
| 60 | // past that one is read again through the same process. |
| 60 | 61 | type anchoredFiles struct { |
| 61 | | dir string |
| 62 | | entries map[[2]string]anchoredEntry |
| 63 | | blobs map[string][]byte |
| 64 | | batch *gitutil.BlobBatch |
| 65 | | spawned int // git processes started, for the test that bounds it |
| 62 | dir string |
| 63 | entries map[[2]string]anchoredEntry |
| 64 | blobs map[string][]byte |
| 65 | cached int64 |
| 66 | cacheCap int64 |
| 67 | batch *gitutil.BlobBatch |
| 68 | spawned int // git processes started, for the test that bounds it |
| 66 | 69 | } |
| 67 | 70 | |
| 71 | // anchoredCacheCap bounds the blobs one render keeps. |
| 72 | const anchoredCacheCap = 8 << 20 |
| 73 | |
| 68 | 74 | type anchoredEntry struct { |
| 69 | 75 | e gitutil.TreeEntry |
| 70 | 76 | ok bool |
| 71 | 77 | } |
| 72 | 78 | |
| 73 | 79 | func newAnchoredFiles(dir string) *anchoredFiles { |
| 74 | | return &anchoredFiles{dir: dir, entries: map[[2]string]anchoredEntry{}, blobs: map[string][]byte{}} |
| 80 | return &anchoredFiles{dir: dir, entries: map[[2]string]anchoredEntry{}, blobs: map[string][]byte{}, |
| 81 | cacheCap: anchoredCacheCap} |
| 75 | 82 | } |
| 76 | 83 | |
| 77 | 84 | func (f *anchoredFiles) close() { |
| @@ -114,7 +121,10 @@ func (f *anchoredFiles) read(commit, path string) ([]byte, string, string, error |
| 114 | 121 | if err != nil { |
| 115 | 122 | return nil, "", "", err |
| 116 | 123 | } |
| 117 | | f.blobs[e.SHA] = content |
| 124 | if f.cached+int64(len(content)) <= f.cacheCap { |
| 125 | f.blobs[e.SHA] = content |
| 126 | f.cached += int64(len(content)) |
| 127 | } |
| 118 | 128 | return content, e.Mode, e.SHA, nil |
| 119 | 129 | } |
| 120 | 130 | |
internal/control/suggestion_test.go
+13 −2
| @@ -491,16 +491,18 @@ func TestApplySuggestionFork(t *testing.T) { |
| 491 | 491 | } |
| 492 | 492 | |
| 493 | 493 | // Reading suggestions costs git processes per file and commit, not per |
| 494 | | // thread: six suggestions on one file read it as one does. |
| 494 | // thread: six suggestions on one file read it as one does, whether or |
| 495 | // not the blobs fit the cache. |
| 495 | 496 | func TestSuggestionsProcessCountPerFile(t *testing.T) { |
| 496 | 497 | f := newSuggestFixture(t, nil) |
| 497 | | spawned := func() int { |
| 498 | spawnedWith := func(cacheCap int64) int { |
| 498 | 499 | t.Helper() |
| 499 | 500 | comments, err := f.st.ListDiffComments(f.mr().ID, f.alice.ID) |
| 500 | 501 | if err != nil { |
| 501 | 502 | t.Fatal(err) |
| 502 | 503 | } |
| 503 | 504 | files := newAnchoredFiles(f.dir) |
| 505 | files.cacheCap = cacheCap |
| 504 | 506 | defer files.close() |
| 505 | 507 | got := suggestionsWith(files, func() bool { return false }, f.mr(), comments) |
| 506 | 508 | for _, s := range got { |
| @@ -508,8 +510,12 @@ func TestSuggestionsProcessCountPerFile(t *testing.T) { |
| 508 | 510 | t.Fatalf("suggestion outdated: %+v", s) |
| 509 | 511 | } |
| 510 | 512 | } |
| 513 | if files.cached > cacheCap { |
| 514 | t.Fatalf("cached %d bytes past a cap of %d", files.cached, cacheCap) |
| 515 | } |
| 511 | 516 | return files.spawned |
| 512 | 517 | } |
| 518 | spawned := func() int { return spawnedWith(anchoredCacheCap) } |
| 513 | 519 | f.suggest(f.alice, "lib.txt", 1, 1, "ONE") |
| 514 | 520 | one := spawned() |
| 515 | 521 | for i := 2; i <= 5; i++ { |
| @@ -519,6 +525,11 @@ func TestSuggestionsProcessCountPerFile(t *testing.T) { |
| 519 | 525 | if six := spawned(); six != one || one > 2 { |
| 520 | 526 | t.Fatalf("git processes: %d for one suggestion, %d for six on the same file", one, six) |
| 521 | 527 | } |
| 528 | // With no room to cache, blobs are read again through the same |
| 529 | // process, and every suggestion still reads right. |
| 530 | if none := spawnedWith(0); none != one { |
| 531 | t.Fatalf("git processes with no cache: %d, want %d", none, one) |
| 532 | } |
| 522 | 533 | } |
| 523 | 534 | |
| 524 | 535 | // A server-side write stops at the owner's storage quota as a push does: |
internal/gitutil/batch.go
+11 −6
| @@ -56,16 +56,21 @@ func (b *BlobBatch) Read(oid string, limit int64) ([]byte, error) { |
| 56 | 56 | if err != nil { |
| 57 | 57 | return nil, fmt.Errorf("cat-file %s: %s", oid, strings.TrimSpace(head)) |
| 58 | 58 | } |
| 59 | // A refused object is still on the stream, with its trailing |
| 60 | // newline; skipping it keeps the next Read in step. |
| 61 | if f[1] != "blob" || size > limit { |
| 62 | if _, err := io.CopyN(io.Discard, b.out, size+1); err != nil { |
| 63 | return nil, err |
| 64 | } |
| 65 | if f[1] != "blob" { |
| 66 | return nil, fmt.Errorf("%s is a %s, not a blob", oid, f[1]) |
| 67 | } |
| 68 | return nil, fmt.Errorf("%s is larger than %d bytes", oid, limit) |
| 69 | } |
| 59 | 70 | data := make([]byte, size+1) // the object and its trailing newline |
| 60 | 71 | if _, err := io.ReadFull(b.out, data); err != nil { |
| 61 | 72 | return nil, err |
| 62 | 73 | } |
| 63 | | if f[1] != "blob" { |
| 64 | | return nil, fmt.Errorf("%s is a %s, not a blob", oid, f[1]) |
| 65 | | } |
| 66 | | if size > limit { |
| 67 | | return nil, fmt.Errorf("%s is larger than %d bytes", oid, limit) |
| 68 | | } |
| 69 | 74 | return data[:size], nil |
| 70 | 75 | } |
| 71 | 76 | |
internal/gitutil/batch_test.go
+3
| @@ -38,6 +38,9 @@ func TestBlobBatch(t *testing.T) { |
| 38 | 38 | if _, err := batch.Read(a, 3); err == nil { |
| 39 | 39 | t.Error("a blob over the limit was read") |
| 40 | 40 | } |
| 41 | if got, err := batch.Read(b, 100); err != nil || string(got) != "" { |
| 42 | t.Fatalf("read after a blob over the limit = %q, %v", got, err) |
| 43 | } |
| 41 | 44 | if _, err := batch.Read(tree, 100); err == nil { |
| 42 | 45 | t.Error("a tree was read as a blob") |
| 43 | 46 | } |
internal/web/static/style.css
+2
| @@ -1295,6 +1295,8 @@ a.authorlink:hover { color: var(--link); } |
| 1295 | 1295 | form.threadact { margin: var(--sp-1) 0 0; padding: 0 var(--sp-3) var(--sp-2); } |
| 1296 | 1296 | .thread .suggestion { margin: var(--sp-2) 0 0; } |
| 1297 | 1297 | .thread .suggestion table.difftable { margin: var(--sp-1) 0 0; } |
| 1298 | .thread .suggestion table.difftable tr.add td, |
| 1299 | .thread .suggestion table.difftable tr.del td { display: table-cell; } |
| 1298 | 1300 | |
| 1299 | 1301 | nav.subtabs { |
| 1300 | 1302 | display: flex; |
internal/web/templates/layout.html
+1
| @@ -219,6 +219,7 @@ |
| 219 | 219 | <input type="hidden" name="path" value="{{.Path}}"> |
| 220 | 220 | <input type="hidden" name="line" value="{{if eq .Class "del"}}{{.OldLine}}{{else}}{{.NewLine}}{{end}}"> |
| 221 | 221 | <input type="hidden" name="side" value="{{if eq .Class "del"}}old{{else}}new{{end}}"> |
| 222 | {{if ne .Class "del"}}<p><label>From line <input type="number" name="start_line" min="1" max="{{.NewLine}}" placeholder="{{.NewLine}}"></label> to {{.NewLine}}; a <code>```suggestion</code> block proposes replacement lines</p>{{end}} |
| 222 | 223 | <p><textarea name="body" aria-label="Comment on {{.Path}}" rows="3" placeholder="Comment on this line" autofocus></textarea></p> |
| 223 | 224 | <p><button type="submit" class="btn">Comment</button> |
| 224 | 225 | <button type="submit" name="pending" value="on" class="btn">Add to review</button> |