Commit `5271aa8b0d`

5271aa8b0d040f1bb9dcfcffec98563966f34a26

parent: 9581d34d47

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-29 04:50 UTC

control: suggestion blocks in diff comments, line ranges, suggestions in mr threads

Ref #288

Layout: unified · split

internal/control/diffcomment.go +84 −19

		@@ -12,15 +12,17 @@ import (
12	12	"gitbay.org/gitbay/internal/policy"
13	13	"gitbay.org/gitbay/internal/protocol"
14	14	"gitbay.org/gitbay/internal/store"
	15	"gitbay.org/gitbay/internal/suggest"
15	16	)
16	17
17	18	func init() {
18	19	register(Command{Path: []string{"mr", "diff-comment"},
19	20	Summary: "comment on a diff line",
20		Usage: "mr diff-comment <owner/name> <n> --path <file> --line <l> [--old] [--pending] [--reply <id>] [--message <m> \| --file -]",
	21	Usage: "mr diff-comment <owner/name> <n> --path <file> --line <l> [--start-line <s>] [--old] [--pending] [--reply <id>] [--message <m> \| --file -]",
21	22	Flags: []Flag{
22	23	{"--path", "<file>", "the file the comment is on", ""},
23		{"--line", "<l>", "the line the comment is on", ""},
	24	{"--line", "<l>", "the line the comment is on, or the last line of a range", ""},
	25	{"--start-line", "<s>", "the first line of a range ending at --line", ""},
24	26	{"--old", "", "the line is on the old side of the diff", ""},
25	27	{"--pending", "", "hold the comment for `mr review --comment`", ""},
26	28	{"--reply", "<id>", "reply to this thread instead of opening one", ""},
		@@ -30,6 +32,7 @@ func init() {
30	32	Examples: []string{
31	33	`mr diff-comment krz/gitbay 431 --path internal/control/build.go --line 42 --message "why is this a switch"`,
32	34	"mr diff-comment krz/gitbay 431 --reply 12 --file - < notes.md",
	35	"mr diff-comment krz/gitbay 431 --path go.mod --start-line 3 --line 4 --file - < suggestion.md",
33	36	},
34	37	ReadsStdin: true, Run: runDiffComment})
35	38	register(Command{Path: []string{"mr", "threads"},
		@@ -50,15 +53,15 @@ func init() {
50	53	}
51	54
52	55	func runDiffComment(c *Ctx, args []string) int {
53		f, err := c.parseArgs(args, flagSpec{Values: []string{"--path", "--line", "--reply", "--message", "--file"},
	56	f, err := c.parseArgs(args, flagSpec{Values: []string{"--path", "--line", "--start-line", "--reply", "--message", "--file"},
54	57	Bools: []string{"--old", "--pending"}, MaxPos: -1,
55		Usage: "mr diff-comment <owner/name> <n> --path <file> --line <l> [--old] [--pending] [--reply <id>] [--message <m> \| --file -]"})
	58	Usage: "mr diff-comment <owner/name> <n> --path <file> --line <l> [--start-line <s>] [--old] [--pending] [--reply <id>] [--message <m> \| --file -]"})
56	59	if err != nil {
57	60	return c.fail(protocol.ExitUsage, "%v", err)
58	61	}
59	62	rest := f.Pos
60	63	path, message, file, old := f.Value("--path"), f.Value("--message"), f.Value("--file"), f.Has("--old")
61		var line, replyTo int64
	64	var line, startLine, replyTo int64
62	65	if f.Has("--line") {
63	66	n, err := strconv.ParseInt(f.Value("--line"), 10, 64)
64	67	if err != nil \|\| n < 1 {
		@@ -66,6 +69,13 @@ func runDiffComment(c *Ctx, args []string) int {
66	69	}
67	70	line = n
68	71	}
	72	if f.Has("--start-line") {
	73	n, err := strconv.ParseInt(f.Value("--start-line"), 10, 64)
	74	if err != nil \|\| n < 1 \|\| (line != 0 && n > line) {
	75	return c.fail(protocol.ExitUsage, "--start-line must be a positive number no greater than --line")
	76	}
	77	startLine = n
	78	}
69	79	if f.Has("--reply") {
70	80	n, err := strconv.ParseInt(f.Value("--reply"), 10, 64)
71	81	if err != nil \|\| n < 1 {
		@@ -90,6 +100,19 @@ func runDiffComment(c *Ctx, args []string) int {
90	100	if strings.TrimSpace(body) == "" {
91	101	return c.fail(protocol.ExitUsage, "empty comment; use --message or --file -")
92	102	}
	103	if replyTo != 0 && startLine != 0 {
	104	return c.fail(protocol.ExitUsage, "a reply takes its thread's lines; drop --start-line")
	105	}
	106	_, hasSuggestion, err := suggest.Parse(body)
	107	if err != nil {
	108	return c.fail(protocol.ExitUsage, "%v", err)
	109	}
	110	if hasSuggestion && replyTo != 0 {
	111	return c.fail(protocol.ExitUsage, "a suggestion opens its own thread: post it with --path and --line, not --reply")
	112	}
	113	if hasSuggestion && old {
	114	return c.fail(protocol.ExitUsage, "a suggestion replaces lines of the new file; drop --old")
	115	}
93	116
94	117	side := "new"
95	118	if old {
		@@ -113,10 +136,20 @@ func runDiffComment(c *Ctx, args []string) int {
113	136	if !slices.Contains(files, path) {
114	137	return c.fail(protocol.ExitUsage, "%s is not part of this merge request's diff", path)
115	138	}
	139	if hasSuggestion {
	140	first := firstNonZero(startLine, line)
	141	content, _, err := readAnchored(dir, mr.HeadSHA, path)
	142	if err != nil {
	143	return c.fail(protocol.ExitUsage, "%v", err)
	144	}
	145	if _, ok := suggest.Range(content, int(first), int(line)); !ok {
	146	return c.fail(protocol.ExitUsage, "%s has no lines %d-%d at the head", path, first, line)
	147	}
	148	}
116	149	}
117	150
118	151	pending := f.Has("--pending")
119		id, err := c.Store.AddDiffComment(mr.ID, c.User.ID, mr.HeadSHA, path, side, line, body, replyTo, pending)
	152	id, err := c.Store.AddDiffComment(mr.ID, c.User.ID, mr.HeadSHA, path, side, line, startLine, body, replyTo, pending)
120	153	if err != nil {
121	154	if errors.Is(err, store.ErrNotFound) {
122	155	return c.fail(protocol.ExitNotFound, "%v", err)
		@@ -175,22 +208,25 @@ func runMRThreads(c *Ctx, args []string) int {
175	208	CreatedAt string `json:"created_at"`
176	209	}
177	210	type threadOut struct {
178		ID int64 `json:"id"`
179		Path string `json:"path"`
180		Side string `json:"side"`
181		Line int64 `json:"line"`
182		Stale bool `json:"stale"`
183		Resolved string `json:"resolved_by,omitempty"`
184		Comments []commentOut `json:"comments"`
	211	ID int64 `json:"id"`
	212	Path string `json:"path"`
	213	Side string `json:"side"`
	214	StartLine int64 `json:"start_line,omitempty"`
	215	Line int64 `json:"line"`
	216	Stale bool `json:"stale"`
	217	Resolved string `json:"resolved_by,omitempty"`
	218	Suggestion *SuggestionOut `json:"suggestion,omitempty"`
	219	Comments []commentOut `json:"comments"`
185	220	}
186	221	byRoot := map[int64]*threadOut{}
187	222	var order []int64
188	223	for _, cm := range comments {
189	224	if cm.ReplyTo == 0 {
190	225	byRoot[cm.ID] = &threadOut{
191		ID: cm.ID, Path: cm.Path, Side: cm.Side, Line: cm.Line,
	226	ID: cm.ID, Path: cm.Path, Side: cm.Side, StartLine: cm.StartLine, Line: cm.Line,
192	227	Stale: cm.HeadSHA != mr.HeadSHA, Resolved: cm.ResolvedBy,
193		Comments: []commentOut{{cm.ID, cm.Author, cm.Body, cm.CreatedAt}},
	228	Suggestion: ThreadSuggestion(c.Store, c.Cfg.Server.Root, repo, mr, cm),
	229	Comments: []commentOut{{cm.ID, cm.Author, cm.Body, cm.CreatedAt}},
194	230	}
195	231	order = append(order, cm.ID)
196	232	} else if th, ok := byRoot[cm.ReplyTo]; ok {
		@@ -201,7 +237,6 @@ func runMRThreads(c *Ctx, args []string) int {
201	237	for _, id := range order {
202	238	ds = append(ds, *byRoot[id])
203	239	}
204		_ = repo
205	240	return c.emit(ds, func(w io.Writer) {
206	241	for _, th := range ds {
207	242	marks := ""
		@@ -211,14 +246,44 @@ func runMRThreads(c *Ctx, args []string) int {
211	246	if th.Stale {
212	247	marks += " [stale]"
213	248	}
214		fmt.Fprintf(w, "thread %d %s:%d (%s)%s\n", th.ID, th.Path, th.Line, th.Side, marks)
215		for _, cm := range th.Comments {
216		fmt.Fprintf(w, " %s: %s\n", cm.Author, cm.Body)
	249	lines := fmt.Sprint(th.Line)
	250	if th.StartLine != 0 && th.StartLine != th.Line {
	251	lines = fmt.Sprintf("%d-%d", th.StartLine, th.Line)
	252	}
	253	fmt.Fprintf(w, "thread %d %s:%s (%s)%s\n", th.ID, th.Path, lines, th.Side, marks)
	254	for i, cm := range th.Comments {
	255	body := cm.Body
	256	if i == 0 && th.Suggestion != nil {
	257	body = suggest.Strip(body)
	258	}
	259	if body != "" {
	260	fmt.Fprintf(w, " %s: %s\n", cm.Author, body)
	261	}
	262	if i == 0 && th.Suggestion != nil {
	263	writeSuggestion(w, repo, mr, th.ID, cm.Author, th.Suggestion)
	264	}
217	265	}
218	266	}
219	267	})
220	268	}
221	269
	270	// writeSuggestion prints a suggestion as the diff it proposes and how to
	271	// apply it, or why it can no longer be applied.
	272	func writeSuggestion(w io.Writer, repo store.Repo, mr store.MR, thread int64, author string, s *SuggestionOut) {
	273	switch {
	274	case s.Outdated:
	275	fmt.Fprintf(w, " %s suggests (outdated: %s):\n", author, s.Reason)
	276	default:
	277	fmt.Fprintf(w, " %s suggests (gitbay mr apply-suggestion %s %d %d):\n", author, repo.Path(), mr.Number, thread)
	278	}
	279	for _, l := range suggest.FromText(strings.ReplaceAll(s.Original, "\r\n", "\n")) {
	280	fmt.Fprintf(w, " - %s\n", l)
	281	}
	282	for _, l := range suggest.FromText(s.Replacement) {
	283	fmt.Fprintf(w, " + %s\n", l)
	284	}
	285	}
	286
222	287	func setThreadResolved(c *Ctx, args []string, resolved bool) int {
223	288	if len(args) != 3 {
224	289	return c.usage()

internal/control/mergequeue_test.go +1 −1

		@@ -232,7 +232,7 @@ func TestWhenReadyReviewMerges(t *testing.T) {
232	232	// Resolving the last open thread merges the queued request.
233	233	func TestWhenReadyThreadResolveMerges(t *testing.T) {
234	234	f := newQueueFixture(t, func(s *store.RepoSettings) { s.RequireResolved = true })
235		id, err := f.st.AddDiffComment(f.mr().ID, f.alice.ID, f.headSHA, "feature.txt", "new", 1, "why?", 0, false)
	235	id, err := f.st.AddDiffComment(f.mr().ID, f.alice.ID, f.headSHA, "feature.txt", "new", 1, 0, "why?", 0, false)
236	236	if err != nil {
237	237	t.Fatal(err)
238	238	}

internal/control/suggestion.go added +127

		@@ -0,0 +1,127 @@
	1	package control
	2
	3	import (
	4	"bytes"
	5	"fmt"
	6
	7	"gitbay.org/gitbay/internal/gitutil"
	8	"gitbay.org/gitbay/internal/store"
	9	"gitbay.org/gitbay/internal/suggest"
	10	)
	11
	12	// SuggestionOut is the change a review thread's ```suggestion block
	13	// proposes: lines StartLine through EndLine of Path, as they were at
	14	// Commit (Original), replaced by Replacement. Both texts end every line
	15	// with its terminator, so "" is no lines. Apply is "server" where `mr
	16	// apply-suggestion` commits it, or "local" on a repository requiring
	17	// signed commits, where the CLI commits it with the user's own key.
	18	type SuggestionOut struct {
	19	Path string `json:"path"`
	20	StartLine int64 `json:"start_line"`
	21	EndLine int64 `json:"end_line"`
	22	Commit string `json:"commit"`
	23	Blob string `json:"blob,omitempty"`
	24	Original string `json:"original"`
	25	Replacement string `json:"replacement"`
	26	Outdated bool `json:"outdated"`
	27	Reason string `json:"reason,omitempty"`
	28	Apply string `json:"apply"`
	29	}
	30
	31	// maxSuggestionBytes bounds the file a suggestion rewrites, the same as
	32	// a single-file commit over the control plane.
	33	const maxSuggestionBytes = maxCommitFileBytes
	34
	35	// Reasons a suggestion cannot be applied at a head.
	36	const (
	37	reasonGone = "the file is not at the head: it was renamed or deleted"
	38	reasonChanged = "the lines it replaces have changed since it was made"
	39	reasonNoBase = "the commit it was made against is no longer available"
	40	)
	41
	42	// readAnchored reads the regular file path at commit, with its mode.
	43	func readAnchored(dir, commit, path string) ([]byte, string, error) {
	44	e, ok := gitutil.StatPath(dir, commit, path)
	45	if !ok {
	46	return nil, "", fmt.Errorf("%s", reasonGone)
	47	}
	48	if e.Type != "blob" \|\| (e.Mode != "100644" && e.Mode != "100755") {
	49	return nil, "", fmt.Errorf("%s is not a regular file", path)
	50	}
	51	if e.Size > maxSuggestionBytes {
	52	return nil, "", fmt.Errorf("%s is larger than %d bytes", path, maxSuggestionBytes)
	53	}
	54	content, err := gitutil.ReadBlob(dir, commit, path, maxSuggestionBytes)
	55	if err != nil {
	56	return nil, "", err
	57	}
	58	return content, e.Mode, nil
	59	}
	60
	61	// suggestionRange is a thread's first and last line.
	62	func suggestionRange(cm store.DiffComment) (int, int) {
	63	return int(firstNonZero(cm.StartLine, cm.Line)), int(cm.Line)
	64	}
	65
	66	// ThreadSuggestion is the suggestion a thread root carries, checked
	67	// against the merge request's head, or nil when it carries none. The
	68	// original lines are read from the commit the comment was made on, in
	69	// the target repository, which holds every head the merge request has
	70	// had until gc prunes an abandoned one.
	71	func ThreadSuggestion(st store.Store, root string, repo store.Repo, mr store.MR, cm store.DiffComment) SuggestionOut {
	72	if cm.ReplyTo != 0 \|\| cm.Side != "new" {
	73	return nil
	74	}
	75	lines, found, err := suggest.Parse(cm.Body)
	76	if err != nil \|\| !found {
	77	return nil
	78	}
	79	start, end := suggestionRange(cm)
	80	s := &SuggestionOut{Path: cm.Path, StartLine: int64(start), EndLine: int64(end), Commit: cm.HeadSHA,
	81	Replacement: suggest.Text(lines), Apply: "server"}
	82	if signedOnly(st, repo, mr) {
	83	s.Apply = "local"
	84	}
	85	dir := RepoDir(root, repo.OwnerName, repo.Name)
	86	if e, ok := gitutil.StatPath(dir, cm.HeadSHA, cm.Path); ok {
	87	s.Blob = e.SHA
	88	}
	89	base, _, err := readAnchored(dir, cm.HeadSHA, cm.Path)
	90	orig, ok := suggest.Range(base, start, end)
	91	if err != nil \|\| !ok {
	92	s.Outdated, s.Reason = true, reasonNoBase
	93	return s
	94	}
	95	s.Original = string(orig)
	96	s.Reason = anchorReason(dir, mr.HeadSHA, s)
	97	s.Outdated = s.Reason != ""
	98	return s
	99	}
	100
	101	// anchorReason says why s cannot be applied to the file at head, or ""
	102	// when the lines it replaces are still what it was made against.
	103	func anchorReason(dir, head string, s *SuggestionOut) string {
	104	content, _, err := readAnchored(dir, head, s.Path)
	105	if err != nil {
	106	return err.Error()
	107	}
	108	now, ok := suggest.Range(content, int(s.StartLine), int(s.EndLine))
	109	if !ok \|\| !bytes.Equal(now, []byte(s.Original)) {
	110	return reasonChanged
	111	}
	112	return ""
	113	}
	114
	115	// signedOnly reports whether the server may not commit a suggestion for
	116	// this merge request: the source branch or the target it merges into
	117	// requires signed commits, and the server has no key to sign with.
	118	func signedOnly(st *store.Store, repo store.Repo, mr store.MR) bool {
	119	if repo.Settings.RequireSignedCommits {
	120	return true
	121	}
	122	if mr.SourceRepoID == repo.ID {
	123	return false
	124	}
	125	src, err := st.RepoByID(mr.SourceRepoID)
	126	return err != nil \|\| src.Settings.RequireSignedCommits
	127	}

internal/control/suggestion_test.go added +192

		@@ -0,0 +1,192 @@
	1	package control
	2
	3	import (
	4	"encoding/json"
	5	"strconv"
	6	"strings"
	7	"testing"
	8
	9	"gitbay.org/gitbay/internal/protocol"
	10	"gitbay.org/gitbay/internal/store"
	11	)
	12
	13	// suggestFixture is a queueFixture whose feature branch carries a
	14	// multi-line file, lib.txt, for suggestions to anchor in.
	15	func newSuggestFixture(t testing.T, set func(store.RepoSettings)) *queueFixture {
	16	t.Helper()
	17	f := newQueueFixture(t, set)
	18	f.write("lib.txt", "one\ntwo\nthree\nfour\nfive\n")
	19	f.write("dos.txt", "a\r\nb\r\nc\r\n")
	20	f.write("tail.txt", "x\nlast")
	21	f.git(f.src, "add", ".")
	22	f.git(f.src, "commit", "-q", "-m", "lib")
	23	f.moveHead()
	24	return f
	25	}
	26
	27	// moveHead pushes the fixture's feature branch to the bare repository
	28	// and points the merge request at it, as post-receive would.
	29	func (f *queueFixture) moveHead() {
	30	f.t.Helper()
	31	f.git(f.src, "push", "-q", "--force", f.dir, "feature")
	32	f.headSHA = strings.TrimSpace(f.git(f.src, "rev-parse", "HEAD"))
	33	f.git(f.dir, "update-ref", mrHeadRef(1), f.headSHA)
	34	if err := f.st.UpdateMRHead(f.mr().ID, f.headSHA, f.targetSH, false); err != nil {
	35	f.t.Fatal(err)
	36	}
	37	}
	38
	39	// suggest opens a thread on lib.txt start-end with a suggestion block
	40	// holding lines, returning the thread id.
	41	func (f *queueFixture) suggest(u store.User, path string, start, end int, lines ...string) string {
	42	f.t.Helper()
	43	body := "try this\n```suggestion\n" + strings.Join(lines, "\n")
	44	if len(lines) > 0 {
	45	body += "\n"
	46	}
	47	body += "```\n"
	48	var out, errOut strings.Builder
	49	c := &Ctx{User: u, Scope: "full", Store: f.st, Stdout: &out, Stderr: &errOut, JSON: true,
	50	Stdin: strings.NewReader(body)}
	51	c.Cfg.Server.Root = f.root
	52	argv := []string{"mr", "diff-comment", f.repo.Path(), "1", "--path", path,
	53	"--start-line", strconv.Itoa(start), "--line", strconv.Itoa(end), "--file", "-"}
	54	if code := Dispatch(c, argv); code != protocol.ExitOK {
	55	f.t.Fatalf("diff-comment: exit %d, %s", code, errOut.String())
	56	}
	57	var env struct {
	58	Data struct {
	59	Thread int64 `json:"thread"`
	60	} `json:"data"`
	61	}
	62	if err := json.Unmarshal([]byte(out.String()), &env); err != nil {
	63	f.t.Fatal(err)
	64	}
	65	return strconv.Itoa(int(env.Data.Thread))
	66	}
	67
	68	type threadJSON struct {
	69	ID int64 `json:"id"`
	70	StartLine int64 `json:"start_line"`
	71	Line int64 `json:"line"`
	72	Resolved string `json:"resolved_by"`
	73	Suggestion *SuggestionOut `json:"suggestion"`
	74	}
	75
	76	func (f *queueFixture) threads(u store.User) []threadJSON {
	77	f.t.Helper()
	78	out := f.mustRun(u, "mr", "threads", f.repo.Path(), "1", "--json")
	79	var env struct {
	80	Data []threadJSON `json:"data"`
	81	}
	82	if err := json.Unmarshal([]byte(out), &env); err != nil {
	83	f.t.Fatalf("threads JSON: %v\n%s", err, out)
	84	}
	85	return env.Data
	86	}
	87
	88	func (f queueFixture) suggestion(u store.User, thread string) SuggestionOut {
	89	f.t.Helper()
	90	for _, th := range f.threads(u) {
	91	if strconv.Itoa(int(th.ID)) == thread {
	92	return th.Suggestion
	93	}
	94	}
	95	f.t.Fatalf("no thread %s", thread)
	96	return nil
	97	}
	98
	99	// A suggestion over a range reads back from mr threads as structure:
	100	// the anchor, the commit and blob it was made against, the lines it
	101	// replaces and the replacement.
	102	func TestSuggestionInThreads(t *testing.T) {
	103	f := newSuggestFixture(t, nil)
	104	id := f.suggest(f.alice, "lib.txt", 2, 3, "TWO", "THREE", "extra")
	105	s := f.suggestion(f.alice, id)
	106	if s == nil {
	107	t.Fatal("thread carries no suggestion")
	108	}
	109	blob := strings.TrimSpace(f.git(f.dir, "rev-parse", f.headSHA+":lib.txt"))
	110	want := SuggestionOut{Path: "lib.txt", StartLine: 2, EndLine: 3, Commit: f.headSHA, Blob: blob,
	111	Original: "two\nthree\n", Replacement: "TWO\nTHREE\nextra\n", Apply: "server"}
	112	if *s != want {
	113	t.Fatalf("suggestion = %+v\nwant %+v", *s, want)
	114	}
	115	text := f.mustRun(f.alice, "mr", "threads", f.repo.Path(), "1")
	116	for _, w := range []string{"lib.txt:2-3", "- two", "+ THREE", "mr apply-suggestion alice/app 1 " + id} {
	117	if !strings.Contains(text, w) {
	118	t.Errorf("threads text lacks %q:\n%s", w, text)
	119	}
	120	}
	121	if strings.Contains(text, "```suggestion") {
	122	t.Errorf("threads text repeats the raw block:\n%s", text)
	123	}
	124	}
	125
	126	// The suggestion goes stale when the lines it replaces change, and not
	127	// when the file changes elsewhere.
	128	func TestSuggestionOutdated(t *testing.T) {
	129	f := newSuggestFixture(t, nil)
	130	id := f.suggest(f.alice, "lib.txt", 2, 2, "TWO")
	131	f.write("lib.txt", "one\ntwo\nthree\nfour\nFIVE\n")
	132	f.git(f.src, "commit", "-q", "-am", "elsewhere")
	133	f.moveHead()
	134	if s := f.suggestion(f.alice, id); s.Outdated {
	135	t.Fatalf("a change below the range outdated the suggestion: %+v", s)
	136	}
	137	f.write("lib.txt", "zero\none\ntwo\nthree\nfour\nFIVE\n")
	138	f.git(f.src, "commit", "-q", "-am", "shift")
	139	f.moveHead()
	140	if s := f.suggestion(f.alice, id); !s.Outdated \|\| s.Reason != reasonChanged {
	141	t.Fatalf("suggestion after its lines moved = %+v, want outdated", s)
	142	}
	143	f.git(f.src, "rm", "-q", "lib.txt")
	144	f.git(f.src, "commit", "-q", "-m", "gone")
	145	f.moveHead()
	146	if s := f.suggestion(f.alice, id); !s.Outdated \|\| s.Reason != reasonGone {
	147	t.Fatalf("suggestion on a deleted file = %+v, want outdated", s)
	148	}
	149	}
	150
	151	// On a repository requiring signed commits the server does not commit a
	152	// suggestion, and the thread says the CLI applies it locally.
	153	func TestSuggestionSignedIsLocal(t *testing.T) {
	154	f := newSuggestFixture(t, func(s *store.RepoSettings) { s.RequireSignedCommits = true })
	155	id := f.suggest(f.alice, "lib.txt", 1, 1, "ONE")
	156	if s := f.suggestion(f.alice, id); s.Apply != "local" {
	157	t.Fatalf("apply = %q, want local", s.Apply)
	158	}
	159	}
	160
	161	// What diff-comment refuses before storing a suggestion it could never
	162	// apply.
	163	func TestSuggestionRefusals(t *testing.T) {
	164	f := newSuggestFixture(t, nil)
	165	id := f.suggest(f.alice, "lib.txt", 1, 1, "ONE")
	166	run := func(body string, extra ...string) (int, string) {
	167	var out, errOut strings.Builder
	168	c := &Ctx{User: f.alice, Scope: "full", Store: f.st, Stdout: &out, Stderr: &errOut,
	169	Stdin: strings.NewReader(body)}
	170	c.Cfg.Server.Root = f.root
	171	return Dispatch(c, append([]string{"mr", "diff-comment", f.repo.Path(), "1", "--file", "-"}, extra...)), errOut.String()
	172	}
	173	block := "```suggestion\nx\n```\n"
	174	cases := []struct {
	175	name string
	176	body string
	177	args []string
	178	want string
	179	}{
	180	{"old side", block, []string{"--path", "lib.txt", "--line", "1", "--old"}, "drop --old"},
	181	{"reply", block, []string{"--reply", id}, "its own thread"},
	182	{"past the end", block, []string{"--path", "lib.txt", "--start-line", "5", "--line", "6"}, "no lines 5-6"},
	183	{"unclosed", "```suggestion\nx\n", []string{"--path", "lib.txt", "--line", "1"}, "not closed"},
	184	{"start after line", "plain", []string{"--path", "lib.txt", "--start-line", "3", "--line", "2"}, "no greater than --line"},
	185	}
	186	for _, c := range cases {
	187	code, errOut := run(c.body, c.args...)
	188	if code != protocol.ExitUsage \|\| !strings.Contains(errOut, c.want) {
	189	t.Errorf("%s: exit %d %q, want usage with %q", c.name, code, errOut, c.want)
	190	}
	191	}
	192	}

internal/store/diffcomments.go +36 −12

		@@ -13,6 +13,7 @@ type DiffComment struct {
13	13	Path string
14	14	Side string
15	15	Line int64
	16	StartLine int64 // first line of a range ending at Line; 0 for Line alone
16	17	Body string
17	18	ReplyTo int64 // 0 for thread roots
18	19	ResolvedBy string
		@@ -23,8 +24,9 @@ type DiffComment struct {
23	24	}
24	25
25	26	// AddDiffComment creates a thread root (replyTo 0) or a reply. Replies
26		// inherit the root's anchor and must belong to the same MR.
27		func (s *Store) AddDiffComment(mrID, authorID int64, headSHA, path, side string, line int64, body string, replyTo int64, pending bool) (int64, error) {
	27	// inherit the root's anchor and must belong to the same MR. startLine is
	28	// the first line of a range ending at line, or 0.
	29	func (s *Store) AddDiffComment(mrID, authorID int64, headSHA, path, side string, line, startLine int64, body string, replyTo int64, pending bool) (int64, error) {
28	30	if replyTo != 0 {
29	31	var rootMR int64
30	32	var rootReply sql.NullInt64
		@@ -43,8 +45,8 @@ func (s *Store) AddDiffComment(mrID, authorID int64, headSHA, path, side string,
43	45	return 0, fmt.Errorf("reply to the thread root %d, not to a reply", rootReply.Int64)
44	46	}
45	47	err = s.DB.QueryRow(
46		"SELECT head_sha, path, side, line FROM mr_diff_comments WHERE id = ?", replyTo).
47		Scan(&headSHA, &path, &side, &line)
	48	"SELECT head_sha, path, side, line, start_line FROM mr_diff_comments WHERE id = ?", replyTo).
	49	Scan(&headSHA, &path, &side, &line, &startLine)
48	50	if err != nil {
49	51	return 0, err
50	52	}
		@@ -54,9 +56,9 @@ func (s *Store) AddDiffComment(mrID, authorID int64, headSHA, path, side string,
54	56	reply = replyTo
55	57	}
56	58	res, err := s.DB.Exec(`
57		INSERT INTO mr_diff_comments (mr_id, author_id, head_sha, path, side, line, body, reply_to, pending)
58		VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`,
59		mrID, authorID, headSHA, path, side, line, body, reply, pending)
	59	INSERT INTO mr_diff_comments (mr_id, author_id, head_sha, path, side, line, start_line, body, reply_to, pending)
	60	VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`,
	61	mrID, authorID, headSHA, path, side, line, startLine, body, reply, pending)
60	62	if err != nil {
61	63	return 0, err
62	64	}
		@@ -68,8 +70,7 @@ func (s *Store) AddDiffComment(mrID, authorID int64, headSHA, path, side string,
68	70	// anonymous reader, who sees only what is published.
69	71	func (s *Store) ListDiffComments(mrID, viewer int64) ([]DiffComment, error) {
70	72	rows, err := s.DB.Query(`
71		SELECT c.id, u.username, c.head_sha, c.path, c.side, c.line, c.body,
72		COALESCE(c.reply_to, 0), COALESCE(r.username, ''), c.pending, c.created_at
	73	SELECT `+diffCommentCols+`
73	74	FROM mr_diff_comments c
74	75	JOIN users u ON u.id = c.author_id
75	76	LEFT JOIN users r ON r.id = c.resolved_by
		@@ -81,9 +82,8 @@ func (s *Store) ListDiffComments(mrID, viewer int64) ([]DiffComment, error) {
81	82	defer rows.Close()
82	83	var out []DiffComment
83	84	for rows.Next() {
84		var c DiffComment
85		if err := rows.Scan(&c.ID, &c.Author, &c.HeadSHA, &c.Path, &c.Side, &c.Line, &c.Body,
86		&c.ReplyTo, &c.ResolvedBy, &c.Pending, &c.CreatedAt); err != nil {
	85	c, err := scanDiffComment(rows)
	86	if err != nil {
87	87	return nil, err
88	88	}
89	89	out = append(out, c)
		@@ -91,6 +91,30 @@ func (s *Store) ListDiffComments(mrID, viewer int64) ([]DiffComment, error) {
91	91	return out, rows.Err()
92	92	}
93	93
	94	const diffCommentCols = `c.id, u.username, c.head_sha, c.path, c.side, c.line, c.start_line, c.body,
	95	COALESCE(c.reply_to, 0), COALESCE(r.username, ''), c.pending, c.created_at`
	96
	97	func scanDiffComment(row interface{ Scan(...any) error }) (DiffComment, error) {
	98	var c DiffComment
	99	err := row.Scan(&c.ID, &c.Author, &c.HeadSHA, &c.Path, &c.Side, &c.Line, &c.StartLine, &c.Body,
	100	&c.ReplyTo, &c.ResolvedBy, &c.Pending, &c.CreatedAt)
	101	return c, err
	102	}
	103
	104	// DiffCommentByID returns one comment on an MR, published or pending.
	105	func (s *Store) DiffCommentByID(mrID, id int64) (DiffComment, error) {
	106	c, err := scanDiffComment(s.DB.QueryRow(`
	107	SELECT `+diffCommentCols+`
	108	FROM mr_diff_comments c
	109	JOIN users u ON u.id = c.author_id
	110	LEFT JOIN users r ON r.id = c.resolved_by
	111	WHERE c.mr_id = ? AND c.id = ?`, mrID, id))
	112	if errors.Is(err, sql.ErrNoRows) {
	113	return c, ErrNotFound
	114	}
	115	return c, err
	116	}
	117
94	118	// SetThreadResolved resolves or unresolves a thread root.
95	119	func (s *Store) SetThreadResolved(mrID, rootID, byUser int64, resolved bool) error {
96	120	var q string

internal/store/migrations/0069_thread_range.down.sql added +1

		@@ -0,0 +1 @@
	1	ALTER TABLE mr_diff_comments DROP COLUMN start_line;

internal/store/migrations/0069_thread_range.up.sql added +4

		@@ -0,0 +1,4 @@
	1	-- A review thread may anchor to a range of lines ending at line: a
	2	-- suggestion replaces start_line through line. 0 means the thread is on
	3	-- line alone, which every existing row is.
	4	ALTER TABLE mr_diff_comments ADD COLUMN start_line INTEGER NOT NULL DEFAULT 0;

internal/store/mrs_test.go +3 −3

		@@ -208,14 +208,14 @@ func TestMRCommentCounts(t *testing.T) {
208	208	if err := s.AddMRSystemComment(mr1.ID, uid, "merged"); err != nil {
209	209	t.Fatal(err)
210	210	}
211		rootID, err := s.AddDiffComment(mr1.ID, uid, "abc123", "file.txt", "new", 1, "root", 0, false)
	211	rootID, err := s.AddDiffComment(mr1.ID, uid, "abc123", "file.txt", "new", 1, 0, "root", 0, false)
212	212	if err != nil {
213	213	t.Fatal(err)
214	214	}
215		if _, err := s.AddDiffComment(mr1.ID, uid, "abc123", "file.txt", "new", 1, "reply", rootID, false); err != nil {
	215	if _, err := s.AddDiffComment(mr1.ID, uid, "abc123", "file.txt", "new", 1, 0, "reply", rootID, false); err != nil {
216	216	t.Fatal(err)
217	217	}
218		if _, err := s.AddDiffComment(mr1.ID, uid, "abc123", "file.txt", "new", 2, "pending root", 0, true); err != nil {
	218	if _, err := s.AddDiffComment(mr1.ID, uid, "abc123", "file.txt", "new", 2, 0, "pending root", 0, true); err != nil {
219	219	t.Fatal(err)
220	220	}
221	221

internal/store/pending_test.go +5 −5

		@@ -35,10 +35,10 @@ func pendingFixture(t testing.T) (Store, int64, int64, int64) {
35	35	// else, until they submit.
36	36	func TestPendingCommentsArePrivate(t *testing.T) {
37	37	s, mrID, author, other := pendingFixture(t)
38		if _, err := s.AddDiffComment(mrID, other, "abc123", "a.go", "new", 3, "half a thought", 0, true); err != nil {
	38	if _, err := s.AddDiffComment(mrID, other, "abc123", "a.go", "new", 3, 0, "half a thought", 0, true); err != nil {
39	39	t.Fatal(err)
40	40	}
41		if _, err := s.AddDiffComment(mrID, other, "abc123", "a.go", "new", 9, "said out loud", 0, false); err != nil {
	41	if _, err := s.AddDiffComment(mrID, other, "abc123", "a.go", "new", 9, 0, "said out loud", 0, false); err != nil {
42	42	t.Fatal(err)
43	43	}
44	44
		@@ -66,7 +66,7 @@ func TestPendingCommentsArePrivate(t *testing.T) {
66	66	// so nobody else could resolve it.
67	67	func TestPendingThreadsDoNotBlockMerges(t *testing.T) {
68	68	s, mrID, _, other := pendingFixture(t)
69		if _, err := s.AddDiffComment(mrID, other, "abc123", "a.go", "new", 3, "pending", 0, true); err != nil {
	69	if _, err := s.AddDiffComment(mrID, other, "abc123", "a.go", "new", 3, 0, "pending", 0, true); err != nil {
70	70	t.Fatal(err)
71	71	}
72	72	n, err := s.UnresolvedThreadCount(mrID)
		@@ -88,12 +88,12 @@ func TestPendingThreadsDoNotBlockMerges(t *testing.T) {
88	88	func TestPublishAndDiscardPending(t *testing.T) {
89	89	s, mrID, author, other := pendingFixture(t)
90	90	for i := 0; i < 3; i++ {
91		if _, err := s.AddDiffComment(mrID, other, "abc123", "a.go", "new", int64(i+1), "note", 0, true); err != nil {
	91	if _, err := s.AddDiffComment(mrID, other, "abc123", "a.go", "new", int64(i+1), 0, "note", 0, true); err != nil {
92	92	t.Fatal(err)
93	93	}
94	94	}
95	95	// Another reviewer's batch is untouched by either operation.
96		if _, err := s.AddDiffComment(mrID, author, "abc123", "b.go", "new", 1, "mine", 0, true); err != nil {
	96	if _, err := s.AddDiffComment(mrID, author, "abc123", "b.go", "new", 1, 0, "mine", 0, true); err != nil {
97	97	t.Fatal(err)
98	98	}
99	99

Commit 5271aa8b0d

Commit `5271aa8b0d`