Commit `65ec67cc47`

65ec67cc474948e6af63c1007c2c3cfed8f29764

parent: 421defaffc

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-29 05:20 UTC

control: read suggestion anchors once per file and commit, blobs through one cat-file --batch

Ref #288

Layout: unified · split

internal/control/diffcomment.go +2 −1

 		Suggestion *SuggestionOut `json:"suggestion,omitempty"`
 		Comments   []commentOut   `json:"comments"`
 	}
+	suggestions := Suggestions(c.Store, c.Cfg.Server.Root, repo, mr, comments)
 	byRoot := map[int64]*threadOut{}
 	var order []int64
 	for _, cm := range comments {
 			byRoot[cm.ID] = &threadOut{
 				ID: cm.ID, Path: cm.Path, Side: cm.Side, StartLine: cm.StartLine, Line: cm.Line,
 				Stale: cm.HeadSHA != mr.HeadSHA, Resolved: cm.ResolvedBy,
-				Suggestion: ThreadSuggestion(c.Store, c.Cfg.Server.Root, repo, mr, cm),
+				Suggestion: suggestions[cm.ID],
 				Comments:   []commentOut{{cm.ID, cm.Author, cm.Body, cm.CreatedAt}},
 			}
 			order = append(order, cm.ID)

internal/control/suggestion.go +115 −45

	@@ -52,23 +52,78 @@ const (
52	reasonNoBase = "the commit it was made against is no longer available"	52	reasonNoBase = "the commit it was made against is no longer available"
53	)	53	)
54		54
55	// readAnchored reads the regular file path at commit, with its mode.	55	// anchoredFiles reads the files suggestions anchor in, for one page or
56	func readAnchored(dir, commit, path string) ([]byte, string, error) {	56	// listing: one ls-tree per (commit, path), and every blob through one
57	e, ok := gitutil.StatPath(dir, commit, path)	57	// cat-file --batch process started on first use, so a merge request with
58	if !ok {	58	// many suggestions on a file costs no more processes than one with a
59	return nil, "", fmt.Errorf("%s", reasonGone)	59	// single suggestion.
		60	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
		66	}
		67
		68	type anchoredEntry struct {
		69	e gitutil.TreeEntry
		70	ok bool
		71	}
		72
		73	func newAnchoredFiles(dir string) *anchoredFiles {
		74	return &anchoredFiles{dir: dir, entries: map[[2]string]anchoredEntry{}, blobs: map[string][]byte{}}
		75	}
		76
		77	func (f *anchoredFiles) close() {
		78	if f.batch != nil {
		79	f.batch.Close()
		80	}
		81	}
		82
		83	// read returns the regular file path at commit, with its mode and blob id.
		84	func (f *anchoredFiles) read(commit, path string) ([]byte, string, string, error) {
		85	key := [2]string{commit, path}
		86	ent, seen := f.entries[key]
		87	if !seen {
		88	f.spawned++
		89	ent.e, ent.ok = gitutil.StatPath(f.dir, commit, path)
		90	f.entries[key] = ent
		91	}
		92	e := ent.e
		93	if !ent.ok {
		94	return nil, "", "", fmt.Errorf("%s", reasonGone)
60	}	95	}
61	if e.Type != "blob" \|\| (e.Mode != "100644" && e.Mode != "100755") {	96	if e.Type != "blob" \|\| (e.Mode != "100644" && e.Mode != "100755") {
62	return nil, "", fmt.Errorf("%s is not a regular file", path)	97	return nil, "", "", fmt.Errorf("%s is not a regular file", path)
63	}	98	}
64	if e.Size > maxSuggestionBytes {	99	if e.Size > maxSuggestionBytes {
65	return nil, "", fmt.Errorf("%s is larger than %d bytes", path, maxSuggestionBytes)	100	return nil, "", "", fmt.Errorf("%s is larger than %d bytes", path, maxSuggestionBytes)
		101	}
		102	if content, ok := f.blobs[e.SHA]; ok {
		103	return content, e.Mode, e.SHA, nil
		104	}
		105	if f.batch == nil {
		106	b, err := gitutil.NewBlobBatch(f.dir)
		107	if err != nil {
		108	return nil, "", "", err
		109	}
		110	f.spawned++
		111	f.batch = b
66	}	112	}
67	content, err := gitutil.ReadBlob(dir, commit, path, maxSuggestionBytes)	113	content, err := f.batch.Read(e.SHA, maxSuggestionBytes)
68	if err != nil {	114	if err != nil {
69	return nil, "", err	115	return nil, "", "", err
70	}	116	}
71	return content, e.Mode, nil	117	f.blobs[e.SHA] = content
		118	return content, e.Mode, e.SHA, nil
		119	}
		120
		121	// readAnchored reads the regular file path at commit, with its mode.
		122	func readAnchored(dir, commit, path string) ([]byte, string, error) {
		123	f := newAnchoredFiles(dir)
		124	defer f.close()
		125	content, mode, _, err := f.read(commit, path)
		126	return content, mode, err
72	}	127	}
73		128
74	// suggestionRange is a thread's first and last line.	129	// suggestionRange is a thread's first and last line.
	@@ -76,45 +131,58 @@ func suggestionRange(cm store.DiffComment) (int, int) {
76	return int(firstNonZero(cm.StartLine, cm.Line)), int(cm.Line)	131	return int(firstNonZero(cm.StartLine, cm.Line)), int(cm.Line)
77	}	132	}
78		133
79	// ThreadSuggestion is the suggestion a thread root carries, checked	134	// Suggestions are the suggestions the thread roots among comments carry,
80	// against the merge request's head, or nil when it carries none. The	135	// by thread id, each checked against the merge request's head. The
81	// original lines are read from the commit the comment was made on, in	136	// original lines are read from the commit the comment was made on, in
82	// the target repository, which holds every head the merge request has	137	// the target repository, which holds every head the merge request has
83	// had until gc prunes an abandoned one.	138	// had until gc prunes an abandoned one.
84	func ThreadSuggestion(st store.Store, root string, repo store.Repo, mr store.MR, cm store.DiffComment) SuggestionOut {	139	func Suggestions(st store.Store, root string, repo store.Repo, mr store.MR, comments []store.DiffComment) map[int64]SuggestionOut {
85	if cm.ReplyTo != 0 \|\| cm.Side != "new" {	140	f := newAnchoredFiles(RepoDir(root, repo.OwnerName, repo.Name))
86	return nil	141	defer f.close()
87	}	142	return suggestionsWith(f, func() bool { return signedOnly(st, repo, mr) }, mr, comments)
88	lines, found, err := suggest.Parse(cm.Body)	143	}
89	if err != nil \|\| !found {	144
90	return nil	145	// suggestionsWith is Suggestions reading through f. signed is asked
91	}	146	// once, and only when there is a suggestion.
92	start, end := suggestionRange(cm)	147	func suggestionsWith(f anchoredFiles, signed func() bool, mr store.MR, comments []store.DiffComment) map[int64]SuggestionOut {
93	s := &SuggestionOut{Path: cm.Path, StartLine: int64(start), EndLine: int64(end), Commit: cm.HeadSHA,	148	out := map[int64]*SuggestionOut{}
94	Replacement: suggest.Text(lines), Apply: "server"}	149	apply := ""
95	if signedOnly(st, repo, mr) {	150	for _, cm := range comments {
96	s.Apply = "local"	151	if cm.ReplyTo != 0 \|\| cm.Side != "new" {
97	}	152	continue
98	dir := RepoDir(root, repo.OwnerName, repo.Name)	153	}
99	if e, ok := gitutil.StatPath(dir, cm.HeadSHA, cm.Path); ok {	154	lines, found, err := suggest.Parse(cm.Body)
100	s.Blob = e.SHA	155	if err != nil \|\| !found {
101	}	156	continue
102	base, _, err := readAnchored(dir, cm.HeadSHA, cm.Path)	157	}
103	orig, ok := suggest.Range(base, start, end)	158	if apply == "" {
104	if err != nil \|\| !ok {	159	apply = "server"
105	s.Outdated, s.Reason = true, reasonNoBase	160	if signed() {
106	return s	161	apply = "local"
107	}	162	}
108	s.Original = string(orig)	163	}
109	s.Reason = anchorReason(dir, mr.HeadSHA, s)	164	start, end := suggestionRange(cm)
110	s.Outdated = s.Reason != ""	165	s := &SuggestionOut{Path: cm.Path, StartLine: int64(start), EndLine: int64(end), Commit: cm.HeadSHA,
111	return s	166	Replacement: suggest.Text(lines), Apply: apply}
		167	out[cm.ID] = s
		168	base, _, blob, err := f.read(cm.HeadSHA, cm.Path)
		169	s.Blob = blob
		170	orig, ok := suggest.Range(base, start, end)
		171	if err != nil \|\| !ok {
		172	s.Outdated, s.Reason = true, reasonNoBase
		173	continue
		174	}
		175	s.Original = string(orig)
		176	s.Reason = anchorReason(f, mr.HeadSHA, s)
		177	s.Outdated = s.Reason != ""
		178	}
		179	return out
112	}	180	}
113		181
114	// anchorReason says why s cannot be applied to the file at head, or ""	182	// anchorReason says why s cannot be applied to the file at head, or ""
115	// when the lines it replaces are still what it was made against.	183	// when the lines it replaces are still what it was made against.
116	func anchorReason(dir, head string, s *SuggestionOut) string {	184	func anchorReason(f anchoredFiles, head string, s SuggestionOut) string {
117	content, _, err := readAnchored(dir, head, s.Path)	185	content, _, _, err := f.read(head, s.Path)
118	if err != nil {	186	if err != nil {
119	return err.Error()	187	return err.Error()
120	}	188	}
	@@ -182,7 +250,7 @@ func runMRApplySuggestion(c *Ctx, args []string) int {
182	if mr.State != "open" {	250	if mr.State != "open" {
183	return c.fail(protocol.ExitUsage, "%s!%d is %s", repo.Path(), mr.Number, mr.State)	251	return c.fail(protocol.ExitUsage, "%s!%d is %s", repo.Path(), mr.Number, mr.State)
184	}	252	}
185	s := ThreadSuggestion(c.Store, c.Cfg.Server.Root, repo, mr, cm)	253	s := Suggestions(c.Store, c.Cfg.Server.Root, repo, mr, []store.DiffComment{cm})[cm.ID]
186	if s == nil {	254	if s == nil {
187	return c.fail(protocol.ExitUsage, "thread %d carries no suggestion", threadID)	255	return c.fail(protocol.ExitUsage, "thread %d carries no suggestion", threadID)
188	}	256	}
	@@ -232,10 +300,12 @@ func runMRApplySuggestion(c *Ctx, args []string) int {
232	if s.Reason == reasonNoBase {	300	if s.Reason == reasonNoBase {
233	return c.fail(protocol.ExitUsage, "suggestion in thread %d cannot be applied: %s", threadID, s.Reason)	301	return c.fail(protocol.ExitUsage, "suggestion in thread %d cannot be applied: %s", threadID, s.Reason)
234	}	302	}
235	if reason := anchorReason(srcDir, tip, s); reason != "" {	303	files := newAnchoredFiles(srcDir)
		304	defer files.close()
		305	if reason := anchorReason(files, tip, s); reason != "" {
236	return c.fail(protocol.ExitUsage, "suggestion in thread %d is outdated: %s", threadID, reason)	306	return c.fail(protocol.ExitUsage, "suggestion in thread %d is outdated: %s", threadID, reason)
237	}	307	}
238	content, mode, err := readAnchored(srcDir, tip, s.Path)	308	content, mode, _, err := files.read(tip, s.Path)
239	if err != nil {	309	if err != nil {
240	return c.fail(protocol.ExitUsage, "%v", err)	310	return c.fail(protocol.ExitUsage, "%v", err)
241	}	311	}

internal/control/suggestion_test.go +32 −1

	@@ -182,7 +182,7 @@ func TestSuggestionRefusals(t *testing.T) {
182	c := &Ctx{User: f.alice, Scope: "full", Store: f.st, Stdout: &out, Stderr: &errOut,	182	c := &Ctx{User: f.alice, Scope: "full", Store: f.st, Stdout: &out, Stderr: &errOut,
183	Stdin: strings.NewReader(body)}	183	Stdin: strings.NewReader(body)}
184	c.Cfg.Server.Root = f.root	184	c.Cfg.Server.Root = f.root
185	c.Cfg.Limits.WriteRate = -1	185	c.Cfg.Limits.WriteRate = -1
186	return Dispatch(c, append([]string{"mr", "diff-comment", f.repo.Path(), "1", "--file", "-"}, extra...)), errOut.String()	186	return Dispatch(c, append([]string{"mr", "diff-comment", f.repo.Path(), "1", "--file", "-"}, extra...)), errOut.String()
187	}	187	}
188	block := "```suggestion\nx\n```\n"	188	block := "```suggestion\nx\n```\n"
	@@ -465,3 +465,34 @@ func TestApplySuggestionFork(t *testing.T) {
465	t.Fatalf("timeline does not say why the merge was dequeued: %+v", cs)	465	t.Fatalf("timeline does not say why the merge was dequeued: %+v", cs)
466	}	466	}
467	}	467	}
		468
		469	// Reading suggestions costs git processes per file and commit, not per
		470	// thread: six suggestions on one file read it as one does.
		471	func TestSuggestionsProcessCountPerFile(t *testing.T) {
		472	f := newSuggestFixture(t, nil)
		473	spawned := func() int {
		474	t.Helper()
		475	comments, err := f.st.ListDiffComments(f.mr().ID, f.alice.ID)
		476	if err != nil {
		477	t.Fatal(err)
		478	}
		479	files := newAnchoredFiles(f.dir)
		480	defer files.close()
		481	got := suggestionsWith(files, func() bool { return false }, f.mr(), comments)
		482	for _, s := range got {
		483	if s.Outdated {
		484	t.Fatalf("suggestion outdated: %+v", s)
		485	}
		486	}
		487	return files.spawned
		488	}
		489	f.suggest(f.alice, "lib.txt", 1, 1, "ONE")
		490	one := spawned()
		491	for i := 2; i <= 5; i++ {
		492	f.suggest(f.alice, "lib.txt", i, i, "X")
		493	}
		494	f.suggest(f.alice, "lib.txt", 1, 2, "Y")
		495	if six := spawned(); six != one \|\| one > 2 {
		496	t.Fatalf("git processes: %d for one suggestion, %d for six on the same file", one, six)
		497	}
		498	}

internal/gitutil/batch.go added +76

@@ -0,0 +1,76 @@
	1	package gitutil
	2
	3	import (
	4	"bufio"
	5	"fmt"
	6	"io"
	7	"os/exec"
	8	"strconv"
	9	"strings"
	10
	11	"gitbay.org/gitbay/internal/toolpath"
	12	)
	13
	14	// BlobBatch reads blobs by object id through one `git cat-file --batch`
	15	// process, for a caller reading many blobs in one request.
	16	type BlobBatch struct {
	17	cmd *exec.Cmd
	18	in io.WriteCloser
	19	out *bufio.Reader
	20	}
	21
	22	// NewBlobBatch starts the cat-file process in dir. Close ends it.
	23	func NewBlobBatch(dir string) (*BlobBatch, error) {
	24	cmd := exec.Command(toolpath.Look("git"), "-C", dir, "cat-file", "--batch")
	25	in, err := cmd.StdinPipe()
	26	if err != nil {
	27	return nil, err
	28	}
	29	out, err := cmd.StdoutPipe()
	30	if err != nil {
	31	return nil, err
	32	}
	33	if err := cmd.Start(); err != nil {
	34	return nil, err
	35	}
	36	return &BlobBatch{cmd: cmd, in: in, out: bufio.NewReader(out)}, nil
	37	}
	38
	39	// Read returns the blob oid, refusing one larger than limit bytes.
	40	func (b *BlobBatch) Read(oid string, limit int64) ([]byte, error) {
	41	if strings.ContainsAny(oid, " \n") {
	42	return nil, fmt.Errorf("bad object id %q", oid)
	43	}
	44	if _, err := io.WriteString(b.in, oid+"\n"); err != nil {
	45	return nil, err
	46	}
	47	head, err := b.out.ReadString('\n')
	48	if err != nil {
	49	return nil, err
	50	}
	51	f := strings.Fields(head)
	52	if len(f) != 3 {
	53	return nil, fmt.Errorf("cat-file %s: %s", oid, strings.TrimSpace(head))
	54	}
	55	size, err := strconv.ParseInt(f[2], 10, 64)
	56	if err != nil {
	57	return nil, fmt.Errorf("cat-file %s: %s", oid, strings.TrimSpace(head))
	58	}
	59	data := make([]byte, size+1) // the object and its trailing newline
	60	if _, err := io.ReadFull(b.out, data); err != nil {
	61	return nil, err
	62	}
	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	return data[:size], nil
	70	}
	71
	72	// Close ends the process.
	73	func (b *BlobBatch) Close() error {
	74	b.in.Close()
	75	return b.cmd.Wait()
	76	}

internal/gitutil/batch_test.go added +50

@@ -0,0 +1,50 @@
	1	package gitutil
	2
	3	import (
	4	"os"
	5	"os/exec"
	6	"strings"
	7	"testing"
	8	)
	9
	10	func TestBlobBatch(t *testing.T) {
	11	dir := t.TempDir()
	12	run := func(stdin string, args ...string) string {
	13	t.Helper()
	14	cmd := exec.Command("git", append([]string{"-C", dir}, args...)...)
	15	cmd.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_NOSYSTEM=1")
	16	cmd.Stdin = strings.NewReader(stdin)
	17	out, err := cmd.CombinedOutput()
	18	if err != nil {
	19	t.Fatalf("git %v: %v\n%s", args, err, out)
	20	}
	21	return strings.TrimSpace(string(out))
	22	}
	23	run("", "init", "-q", "--bare")
	24	a := run("one\ntwo", "hash-object", "-w", "--stdin")
	25	b := run("", "hash-object", "-w", "--stdin")
	26	tree := run("100644 blob "+a+"\tf\n", "mktree")
	27	batch, err := NewBlobBatch(dir)
	28	if err != nil {
	29	t.Fatal(err)
	30	}
	31	defer batch.Close()
	32	for _, c := range []struct{ oid, want string }{{a, "one\ntwo"}, {b, ""}, {a, "one\ntwo"}} {
	33	got, err := batch.Read(c.oid, 100)
	34	if err != nil \|\| string(got) != c.want {
	35	t.Fatalf("Read(%s) = %q, %v; want %q", c.oid, got, err, c.want)
	36	}
	37	}
	38	if _, err := batch.Read(a, 3); err == nil {
	39	t.Error("a blob over the limit was read")
	40	}
	41	if _, err := batch.Read(tree, 100); err == nil {
	42	t.Error("a tree was read as a blob")
	43	}
	44	if _, err := batch.Read(strings.Repeat("0", 40), 100); err == nil {
	45	t.Error("a missing object was read")
	46	}
	47	if got, err := batch.Read(a, 100); err != nil \|\| string(got) != "one\ntwo" {
	48	t.Fatalf("read after refusals = %q, %v", got, err)
	49	}
	50	}

internal/httpd/web.go +2 −1

 		}
 	}
 	suggestions := map[int64]*suggestionView{}
+	sgs := control.Suggestions(s.st, s.cfg.Server.Root, p.Repo, m, diffComments)
 	for _, cm := range diffComments {
-		if sg := control.ThreadSuggestion(s.st, s.cfg.Server.Root, p.Repo, m, cm); sg != nil {
+		if sg := sgs[cm.ID]; sg != nil {
 			suggestions[cm.ID] = newSuggestionView(sg, canApply && !cm.Pending,
 				fmt.Sprintf("gitbay mr apply-suggestion %s %d %d", p.Repo.Path(), m.Number, cm.ID))
 		}

Commit 65ec67cc47

Commit `65ec67cc47`