mr: suggested changes in review comments !528

merged merged by cmc on 2026-09-29 05:36 UTC · krz/gitbay:mr-suggestions-288 into main

33 files changed, +2473 −263

Layout: unified · split

.gitbay/wiki/Parity.org +8

 | review (approve etc.)  | yes | yes | yes |
 | resolve a thread       | yes | yes | yes |
 | comment on a diff line | yes | yes | yes |
+| suggest a change       | yes | yes | no  |
+| apply a suggestion     | yes | yes | no  |
 | merge (all strategies) | yes | yes | yes |
 | merge when ready, cancel | yes | yes | no  |
 | close                  | yes | yes | yes |
 travels on stdin wherever it is set, since argv is world-readable in
 /proc and the audit log keeps flag values.
+Applying a suggestion on a repository that requires signed commits is
+CLI-only by mechanism: the server has no key to sign the commit with,
+so =gitbay mr apply-suggestion= makes and signs it in a clone with the
+user's own git signing configuration and pushes it. The web shows that
+command in place of the button.
 Deleting or transferring a repository stays CLI-only on purpose, as
 does deleting an organization and pruning merge request heads (=admin
 mr prune=): each removes or moves what clone URLs point at, and wants a

.gitbay/wiki/Users.org +42

	@@ -566,6 +566,48 @@ Threads render inline on the MR page. A force-push marks them stale
566	anchors; =mr show= reports the unresolved count. Resolving is for the	566	anchors; =mr show= reports the unresolved count. Resolving is for the
567	thread author, the MR author, or anyone with write.	567	thread author, the MR author, or anyone with write.
568		568
		569	A thread can span lines: =--start-line 12 --line 13= anchors it to
		570	lines 12 and 13 of the new file. A fenced =suggestion= block in the
		571	comment proposes replacement lines for that range; an empty block
		572	proposes deleting it.
		573
		574	#+begin_src sh
		575	gitbay mr diff-comment 4 --path main.go --start-line 12 --line 13 --file - < suggestion.md
		576	gitbay mr apply-suggestion 4 9 # commit thread 9's suggestion
		577	#+end_src
		578
		579	where =suggestion.md= is
		580
		581	#+begin_example
		582	one call does both
		583	```suggestion
		584	log.Printf("starting %s", name)
		585	```
		586	#+end_example
		587
		588	The page and =mr threads= show a suggestion as the lines it replaces
		589	and the lines it proposes; =mr threads --json= carries it as
		590	=suggestion= with the path, the line range, the commit and blob it was
		591	made against, the original and replacement text, =outdated= with a
		592	reason, and =apply= (=server= or =local=). A suggestion is outdated
		593	once the lines it replaces differ at the head from what it was made
		594	against, or the file is renamed or deleted; it cannot be applied then.
		595
		596	Applying commits the replacement to the source branch as the applying
		597	user, with a message naming the merge request and thread, and resolves
		598	the thread if the applier could resolve it by hand (the thread author,
		599	the MR author, or a writer of the target); otherwise the thread stays
		600	open and the output says so. It is a push: only the source branch's
		601	writers can apply (for a merge request from a fork, the fork's
		602	writers), the branch's protection, =require-mr= and the owner's storage
		603	quota apply, and a queued merge treats it as a push by the applier. The web's Apply suggestion button and =mr
		604	apply-suggestion= commit it on the server. The server cannot sign, so
		605	where the source or target requires signed commits (=apply= is
		606	=local=) the CLI fetches the source branch into the clone it runs in,
		607	builds the commit without touching the working tree, signs it with
		608	=git commit-tree -S= under your git signing configuration, pushes it,
		609	and resolves the thread; the page shows that command.
		610
569	* CI builds	611	* CI builds
570		612
571	A =.gitbay/ci.yml= in the repo runs jobs on every branch push:	613	A =.gitbay/ci.yml= in the repo runs jobs on every branch push:

CHANGELOG.org +14

	@@ -12,6 +12,20 @@ anything beyond "replace the binary and restart" is needed.
12	layout puts old and new side by side, each column's line numbers	12	layout puts old and new side by side, each column's line numbers
13	comment on their own side, and a narrow window stacks the rows as a	13	comment on their own side, and a narrow window stacks the rows as a
14	unified diff. Migration 0068 adds =users.diff_layout=. (#290)	14	unified diff. Migration 0068 adds =users.diff_layout=. (#290)
		15	- Suggested changes in review threads: a fenced =suggestion= block in
		16	an =mr diff-comment= body proposes replacement lines for the
		17	anchored range, and =--start-line= anchors a thread to a range.
		18	=mr threads= and the page show it as a diff, marked outdated once
		19	the lines change at the head or the file is renamed or deleted;
		20	the JSON carries it as =suggestion=. =mr apply-suggestion= and the
		21	page's button commit it to the source branch as the applier, under
		22	the branch's push policy and write rules; a queued merge sees it as
		23	a push. The thread is resolved when the applier could resolve it by
		24	hand, and otherwise stays open with the output saying so. On a
		25	repository requiring signed commits the CLI commits and signs it in
		26	the clone instead. =repo commit-file= and applying a suggestion
		27	refuse once the owner's storage quota is used up, as a push does.
		28	Migration 0069 (#288).
15		29
16	* v1.38.0 — 2026-09-29	30	* v1.38.0 — 2026-09-29
17		31

cmd/gitbay/local.go +259

	@@ -6,12 +6,15 @@ import (
6	"os"	6	"os"
7	"os/exec"	7	"os/exec"
8	"path/filepath"	8	"path/filepath"
		9	"strconv"
9	"strings"	10	"strings"
10		11
		12	"github.com/spf13/cobra"
11	"golang.org/x/term"	13	"golang.org/x/term"
12		14
13	"gitbay.org/gitbay/internal/cliconfig"	15	"gitbay.org/gitbay/internal/cliconfig"
14	"gitbay.org/gitbay/internal/protocol"	16	"gitbay.org/gitbay/internal/protocol"
		17	"gitbay.org/gitbay/internal/suggest"
15	"gitbay.org/gitbay/internal/toolpath"	18	"gitbay.org/gitbay/internal/toolpath"
16	)	19	)
17		20
	@@ -413,3 +416,259 @@ func worktreeDirty() (bool, error) {
413	}	416	}
414	return strings.TrimSpace(string(out)) != "", nil	417	return strings.TrimSpace(string(out)) != "", nil
415	}	418	}
		419
		420	// mrApplySuggestionCmd is `gitbay mr apply-suggestion`: the server's
		421	// command where the server can commit the suggestion, and a local commit
		422	// where it cannot.
		423	func mrApplySuggestionCmd() *cobra.Command {
		424	server := []string{"mr", "apply-suggestion"}
		425	return &cobra.Command{
		426	Use: "apply-suggestion",
		427	Short: summaries["mr apply-suggestion"],
		428	Annotations: map[string]string{
		429	serverPath: "mr apply-suggestion",
		430	stdinMode: "none",
		431	},
		432	DisableFlagParsing: true,
		433	RunE: func(cmd *cobra.Command, args []string) error {
		434	for _, a := range args {
		435	if a == "--help" \|\| a == "-h" {
		436	os.Exit(runServerHelp(passOpts{server: server}, cliPathOf(cmd)))
		437	}
		438	}
		439	os.Exit(cmdMRApplySuggestion(args))
		440	return nil
		441	},
		442	}
		443	}
		444
		445	// suggestionJSON is the suggestion `mr threads --json` carries.
		446	type suggestionJSON struct {
		447	Path string `json:"path"`
		448	StartLine int `json:"start_line"`
		449	EndLine int `json:"end_line"`
		450	Original string `json:"original"`
		451	Replacement string `json:"replacement"`
		452	Outdated bool `json:"outdated"`
		453	Reason string `json:"reason"`
		454	Apply string `json:"apply"`
		455	}
		456
		457	// cmdMRApplySuggestion implements `gitbay mr apply-suggestion [<owner/name>]
		458	// <n> <thread>`. The thread's suggestion says where it applies. Where the
		459	// server can commit it, the server's command does. On a repository
		460	// requiring signed commits the server has no key to sign with, so the
		461	// commit is made here, signed by whatever the user's git config signs
		462	// with (#288): the source branch is fetched into this clone's objects,
		463	// the anchored lines are checked against what the suggestion was made
		464	// against, the commit is built with plumbing (the working tree and
		465	// branches are not touched) and signed with commit-tree -S, and pushed
		466	// as a fast-forward, which fails if the branch moved meanwhile. Then the
		467	// thread is resolved.
		468	func cmdMRApplySuggestion(args []string) int {
		469	t, err := resolveTarget()
		470	if err != nil {
		471	fmt.Fprintln(os.Stderr, "gitbay:", err)
		472	return protocol.ExitFailure
		473	}
		474	asJSON := false
		475	var rest []string
		476	for _, a := range args {
		477	if a == "--json" {
		478	asJSON = true
		479	continue
		480	}
		481	rest = append(rest, a)
		482	}
		483	if args, err = withRepo(t, rest); err != nil {
		484	fmt.Fprintln(os.Stderr, "gitbay:", err)
		485	return protocol.ExitUsage
		486	}
		487	if len(args) != 3 {
		488	fmt.Fprintln(os.Stderr, "usage: gitbay mr apply-suggestion [<owner/name>] <n> <thread-id> [--json]")
		489	return protocol.ExitUsage
		490	}
		491	repo, n, thread := args[0], args[1], args[2]
		492	out, code := captureSSH(t, []string{"mr", "threads", repo, n, "--json"})
		493	if code != 0 {
		494	return code
		495	}
		496	var threads struct {
		497	Data []struct {
		498	ID int64 `json:"id"`
		499	Comments []struct {
		500	Author string `json:"author"`
		501	} `json:"comments"`
		502	Suggestion *suggestionJSON `json:"suggestion"`
		503	} `json:"data"`
		504	}
		505	if err := json.Unmarshal([]byte(out), &threads); err != nil {
		506	fmt.Fprintln(os.Stderr, "gitbay: reading threads:", err)
		507	return protocol.ExitProtocol
		508	}
		509	var author string
		510	var s *suggestionJSON
		511	found := false
		512	for _, th := range threads.Data {
		513	if strconv.FormatInt(th.ID, 10) == thread {
		514	found, s = true, th.Suggestion
		515	if len(th.Comments) > 0 {
		516	author = th.Comments[0].Author
		517	}
		518	}
		519	}
		520	switch {
		521	case !found:
		522	fmt.Fprintf(os.Stderr, "gitbay: no thread %s on %s!%s\n", thread, repo, n)
		523	return protocol.ExitNotFound
		524	case s == nil:
		525	fmt.Fprintf(os.Stderr, "gitbay: thread %s carries no suggestion\n", thread)
		526	return protocol.ExitUsage
		527	case s.Apply != "local":
		528	argv := []string{"mr", "apply-suggestion", repo, n, thread}
		529	if asJSON {
		530	argv = append(argv, "--json")
		531	}
		532	return runSSH(t, argv, strings.NewReader(""))
		533	case s.Outdated:
		534	fmt.Fprintf(os.Stderr, "gitbay: suggestion in thread %s is outdated: %s\n", thread, s.Reason)
		535	return protocol.ExitUsage
		536	}
		537	if _, code := gitOutput("", "rev-parse", "--git-dir"); code != 0 {
		538	fmt.Fprintln(os.Stderr, "gitbay: run this in a git clone; the commit is made and signed here")
		539	return protocol.ExitUsage
		540	}
		541
		542	out, code = captureSSH(t, []string{"mr", "show", repo, n, "--json"})
		543	if code != 0 {
		544	return code
		545	}
		546	var mr struct {
		547	Data struct {
		548	Source string `json:"source"`
		549	State string `json:"state"`
		550	} `json:"data"`
		551	}
		552	if err := json.Unmarshal([]byte(out), &mr); err != nil {
		553	fmt.Fprintln(os.Stderr, "gitbay: reading merge request:", err)
		554	return protocol.ExitProtocol
		555	}
		556	if mr.Data.State != "open" {
		557	fmt.Fprintf(os.Stderr, "gitbay: !%s is %s\n", n, mr.Data.State)
		558	return protocol.ExitUsage
		559	}
		560	srcRepo, branch := repo, mr.Data.Source
		561	if r, b, ok := strings.Cut(mr.Data.Source, ":"); ok {
		562	srcRepo, branch = r, b
		563	}
		564	url := t.inst.CloneURL(srcRepo)
		565	if len(t.inst.SSHOptions) > 0 {
		566	os.Setenv("GIT_SSH_COMMAND", "ssh "+strings.Join(quoteAll(t.inst.SSHOptions), " "))
		567	}
		568	if code := runGitLocal("fetch", "--quiet", url, "refs/heads/"+branch); code != 0 {
		569	return code
		570	}
		571	tip, code := gitOutput("", "rev-parse", "FETCH_HEAD^{commit}")
		572	if code != 0 {
		573	return code
		574	}
		575	entry, code := gitOutput("", "ls-tree", tip, "--", s.Path)
		576	mode, _, _ := strings.Cut(entry, " ")
		577	if code != 0 \|\| (mode != "100644" && mode != "100755") {
		578	fmt.Fprintf(os.Stderr, "gitbay: %s is not a regular file at the head of %s; it was renamed or deleted\n", s.Path, mr.Data.Source)
		579	return protocol.ExitUsage
		580	}
		581	content, code := gitRaw("", "", "cat-file", "blob", tip+":"+s.Path)
		582	if code != 0 {
		583	return code
		584	}
		585	if now, ok := suggest.Range([]byte(content), s.StartLine, s.EndLine); !ok \|\| string(now) != s.Original {
		586	fmt.Fprintf(os.Stderr, "gitbay: suggestion in thread %s is outdated: the lines it replaces have changed\n", thread)
		587	return protocol.ExitUsage
		588	}
		589	updated, err := suggest.Apply([]byte(content), s.StartLine, s.EndLine, suggest.FromText(s.Replacement))
		590	if err != nil {
		591	fmt.Fprintln(os.Stderr, "gitbay:", err)
		592	return protocol.ExitUsage
		593	}
		594	blob, code := gitOutput(string(updated), "hash-object", "-w", "--stdin")
		595	if code != 0 {
		596	return code
		597	}
		598	idx, err := os.CreateTemp("", "gitbay-index-*")
		599	if err != nil {
		600	fmt.Fprintln(os.Stderr, "gitbay:", err)
		601	return protocol.ExitFailure
		602	}
		603	idx.Close()
		604	defer os.Remove(idx.Name())
		605	indexEnv := "GIT_INDEX_FILE=" + idx.Name()
		606	if _, code := gitRaw(indexEnv, "", "read-tree", tip); code != 0 {
		607	return code
		608	}
		609	if _, code := gitRaw(indexEnv, "", "update-index", "--add", "--cacheinfo", mode+","+blob+","+s.Path); code != 0 {
		610	return code
		611	}
		612	tree, code := gitRaw(indexEnv, "", "write-tree")
		613	if code != 0 {
		614	return code
		615	}
		616	tree = strings.TrimSpace(tree)
		617	nr, _ := strconv.ParseInt(n, 10, 64)
		618	tn, _ := strconv.ParseInt(thread, 10, 64)
		619	sha, code := gitOutput(suggest.Message(repo, nr, tn, author), "commit-tree", "-S", tree, "-p", tip, "-F", "-")
		620	if code != 0 {
		621	fmt.Fprintln(os.Stderr, "gitbay: signing the commit failed; the repository requires signed commits, so set user.signingkey (and gpg.format) in git config")
		622	return code
		623	}
		624	if code := runGitLocal("push", "--quiet", url, sha+":refs/heads/"+branch); code != 0 {
		625	return code
		626	}
		627	// The commit has landed, so a thread that cannot be resolved (the
		628	// server's resolve rule is narrower than who can push) is a warning,
		629	// not a failure; captureSSH has already printed the server's reason.
		630	_, code = captureSSH(t, []string{"mr", "resolve", repo, n, thread})
		631	resolved := code == 0
		632	if asJSON {
		633	out, _ := json.Marshal(protocol.Envelope{ProtocolVersion: protocol.Version, Data: map[string]any{
		634	"thread": tn, "sha": sha, "source": mr.Data.Source, "branch": branch, "resolved": resolved}})
		635	fmt.Println(string(out))
		636	} else {
		637	fmt.Printf("applied thread %s to %s at %.10s", thread, mr.Data.Source, sha)
		638	if resolved {
		639	fmt.Print("; thread resolved")
		640	}
		641	fmt.Println()
		642	}
		643	if !resolved {
		644	fmt.Fprintf(os.Stderr, "gitbay: warning: thread %s is still open\n", thread)
		645	}
		646	return 0
		647	}
		648
		649	// gitOutput runs git with stdin and returns its stdout without the
		650	// trailing newline; git's stderr goes to the terminal.
		651	func gitOutput(stdin string, args ...string) (string, int) {
		652	out, code := gitRaw("", stdin, args...)
		653	return strings.TrimRight(out, "\n"), code
		654	}
		655
		656	// gitRaw runs git with one extra environment variable (none when env is
		657	// "") and returns its stdout as written.
		658	func gitRaw(env, stdin string, args ...string) (string, int) {
		659	cmd := exec.Command(toolpath.Look("git"), args...)
		660	if env != "" {
		661	cmd.Env = append(os.Environ(), env)
		662	}
		663	cmd.Stdin = strings.NewReader(stdin)
		664	cmd.Stderr = os.Stderr
		665	out, err := cmd.Output()
		666	if err != nil {
		667	if ee, ok := err.(*exec.ExitError); ok {
		668	return "", ee.ExitCode()
		669	}
		670	fmt.Fprintln(os.Stderr, "gitbay:", err)
		671	return "", protocol.ExitFailure
		672	}
		673	return string(out), 0
		674	}

cmd/gitbay/main.go +1

	@@ -683,6 +683,7 @@ func mrCmd() *cobra.Command {
683	pass("diff-comment", passOpts{server: []string{"mr", "diff-comment"}, needsRepo: true, stdinOK: true, editor: "comment"}),	683	pass("diff-comment", passOpts{server: []string{"mr", "diff-comment"}, needsRepo: true, stdinOK: true, editor: "comment"}),
684	pass("threads", passOpts{server: []string{"mr", "threads"}, needsRepo: true}),	684	pass("threads", passOpts{server: []string{"mr", "threads"}, needsRepo: true}),
685	pass("resolve", passOpts{server: []string{"mr", "resolve"}, needsRepo: true}),	685	pass("resolve", passOpts{server: []string{"mr", "resolve"}, needsRepo: true}),
		686	mrApplySuggestionCmd(),
686	pass("unresolve", passOpts{server: []string{"mr", "unresolve"}, needsRepo: true}),	687	pass("unresolve", passOpts{server: []string{"mr", "unresolve"}, needsRepo: true}),
687	review,	688	review,
688	pass("merge", passOpts{server: []string{"mr", "merge"}, needsRepo: true}),	689	pass("merge", passOpts{server: []string{"mr", "merge"}, needsRepo: true}),

cmd/gitbay/summaries_gen.go +1

	@@ -64,6 +64,7 @@ var summaries = map[string]string{
64	"milestone create": "create a milestone",	64	"milestone create": "create a milestone",
65	"milestone list": "list milestones with progress",	65	"milestone list": "list milestones with progress",
66	"milestone reopen": "reopen a milestone",	66	"milestone reopen": "reopen a milestone",
		67	"mr apply-suggestion": "commit a review thread's suggestion to the source branch",
67	"mr close": "close without merging",	68	"mr close": "close without merging",
68	"mr comment": "add a comment",	69	"mr comment": "add a comment",
69	"mr create": "open a merge request",	70	"mr create": "open a merge request",

e2e/cli_test.go +2

 	configDir string
 	inst      *instance
 	key       string
+	env       []string // appended last, so it overrides the defaults
 }
 func (c *cli) run(t *testing.T, dir, stdin string, args ...string) (string, string, int) {
 		"GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@example.test",
 		"EDITOR=", // no editor in tests: bodies come from flags
 	)
+	cmd.Env = append(cmd.Env, c.env...)
 	if stdin != "" {
 		cmd.Stdin = strings.NewReader(stdin)
 	}

e2e/suggestion_test.go added +213

@@ -0,0 +1,213 @@
	1	package e2e
	2
	3	import (
	4	"encoding/json"
	5	"fmt"
	6	"os"
	7	"path/filepath"
	8	"strings"
	9	"testing"
	10	)
	11
	12	// suggestionCLI is a CLI configured as key against inst.
	13	func suggestionCLI(t testing.T, inst instance, key string) *cli {
	14	t.Helper()
	15	c := &cli{bin: buildGitbayCLI(t), configDir: t.TempDir(), inst: inst, key: key}
	16	c.must(t, "", "", "remote", "add", "test", "127.0.0.1",
	17	"--port", fmt.Sprint(inst.port),
	18	"--ssh-option", "-i", "--ssh-option", key,
	19	"--ssh-option", "-oIdentitiesOnly=yes",
	20	"--ssh-option", "-oStrictHostKeyChecking=no",
	21	"--ssh-option", "-oUserKnownHostsFile="+filepath.Join(inst.sshDir, "kh"),
	22	"--ssh-option", "-oBatchMode=yes",
	23	"--default")
	24	return c
	25	}
	26
	27	// postSuggestion opens a thread on lib.txt line 2 of !1 in repo whose
	28	// suggestion replaces it with two lines, and returns the thread id.
	29	func postSuggestion(t testing.T, inst instance, key, repo string) string {
	30	t.Helper()
	31	body := "split this\n```suggestion\nTWO\nTWO AND A HALF\n```\n"
	32	out, errOut, code := inst.ssh(t, key, body, "mr", "diff-comment", repo, "1",
	33	"--path", "lib.txt", "--line", "2", "--file", "-", "--json")
	34	if code != 0 {
	35	t.Fatalf("diff-comment: exit %d %s", code, errOut)
	36	}
	37	var env struct {
	38	Data struct {
	39	Thread int64 `json:"thread"`
	40	} `json:"data"`
	41	}
	42	json.Unmarshal([]byte(out), &env)
	43	return fmt.Sprint(env.Data.Thread)
	44	}
	45
	46	type e2eThread struct {
	47	ID int64 `json:"id"`
	48	Resolved string `json:"resolved_by"`
	49	Suggestion *struct {
	50	StartLine int `json:"start_line"`
	51	EndLine int `json:"end_line"`
	52	Original string `json:"original"`
	53	Replacement string `json:"replacement"`
	54	Outdated bool `json:"outdated"`
	55	Apply string `json:"apply"`
	56	} `json:"suggestion"`
	57	}
	58
	59	func threadsOf(t testing.T, inst instance, key, repo string) []e2eThread {
	60	t.Helper()
	61	out, errOut, code := inst.ssh(t, key, "", "mr", "threads", repo, "1", "--json")
	62	if code != 0 {
	63	t.Fatalf("mr threads: %s", errOut)
	64	}
	65	var env struct {
	66	Data []e2eThread `json:"data"`
	67	}
	68	if err := json.Unmarshal([]byte(out), &env); err != nil {
	69	t.Fatalf("threads JSON: %v\n%s", err, out)
	70	}
	71	return env.Data
	72	}
	73
	74	// A reviewer's suggestion, applied by the author through the CLI, which
	75	// asks the server to commit it: the source branch gets one commit by the
	76	// author with the suggested lines, and the thread is resolved.
	77	func TestSuggestionAppliedByServer(t *testing.T) {
	78	t.Parallel()
	79	inst := startInstance(t)
	80	aliceKey := inst.newKey(t, "alice")
	81	bobKey := inst.newKey(t, "bob")
	82	inst.admin(t, "admin", "user", "create", "alice",
	83	"--key", aliceKey+".pub", "--email", "alice@example.test", "--verified")
	84	inst.admin(t, "admin", "user", "create", "bob", "--key", bobKey+".pub")
	85	c := suggestionCLI(t, inst, aliceKey)
	86	c.must(t, "", "", "repo", "create", "alice/lib")
	87
	88	env := inst.gitEnv(aliceKey)
	89	work := t.TempDir()
	90	mustGit(t, work, env, "clone", inst.sshURL("alice/lib"), "w")
	91	dir := filepath.Join(work, "w")
	92	mustGit(t, dir, env, "checkout", "-q", "-b", "main")
	93	os.WriteFile(filepath.Join(dir, "lib.txt"), []byte("one\n"), 0o644)
	94	mustGit(t, dir, env, "add", ".")
	95	mustGit(t, dir, env, "commit", "-q", "-m", "base")
	96	mustGit(t, dir, env, "push", "-q", "origin", "main")
	97	mustGit(t, dir, env, "checkout", "-q", "-b", "feat")
	98	os.WriteFile(filepath.Join(dir, "lib.txt"), []byte("one\ntwo\nthree\n"), 0o644)
	99	mustGit(t, dir, env, "commit", "-q", "-am", "more")
	100	mustGit(t, dir, env, "push", "-q", "origin", "feat")
	101	c.must(t, dir, "", "mr", "create", "alice/lib", "--source", "feat", "--target", "main", "--title", "more")
	102
	103	thread := postSuggestion(t, inst, bobKey, "alice/lib")
	104	th := threadsOf(t, inst, aliceKey, "alice/lib")
	105	if len(th) != 1 \|\| th[0].Suggestion == nil \|\| th[0].Suggestion.Original != "two\n" \|\|
	106	th[0].Suggestion.Replacement != "TWO\nTWO AND A HALF\n" \|\| th[0].Suggestion.Apply != "server" {
	107	t.Fatalf("threads = %+v", th)
	108	}
	109
	110	// bob reads the repository and cannot push to it, so he cannot apply.
	111	if _, errOut, code := inst.ssh(t, bobKey, "", "mr", "apply-suggestion", "alice/lib", "1", thread); code != 4 {
	112	t.Fatalf("reader applied a suggestion: exit %d %s", code, errOut)
	113	}
	114
	115	out, errOut, code := c.run(t, dir, "", "mr", "apply-suggestion", "1", thread, "--json")
	116	if code != 0 {
	117	t.Fatalf("apply-suggestion: exit %d\n%s\n%s", code, out, errOut)
	118	}
	119	if !strings.Contains(out, `"resolved":true`) {
	120	t.Errorf("apply-suggestion --json = %s", out)
	121	}
	122	mustGit(t, dir, env, "fetch", "-q", "origin", "feat")
	123	if got := mustGit(t, dir, env, "show", "FETCH_HEAD:lib.txt"); got != "one\nTWO\nTWO AND A HALF\nthree\n" {
	124	t.Fatalf("lib.txt = %q", got)
	125	}
	126	if who := mustGit(t, dir, env, "log", "-1", "--format=%an <%ae>", "FETCH_HEAD"); strings.TrimSpace(who) != "alice <alice@example.test>" {
	127	t.Errorf("commit by %q", who)
	128	}
	129	if th := threadsOf(t, inst, aliceKey, "alice/lib"); th[0].Resolved != "alice" \|\| !th[0].Suggestion.Outdated {
	130	t.Errorf("after apply: thread = %+v", th[0])
	131	}
	132	}
	133
	134	// On a repository requiring signed commits the server refuses, and the
	135	// CLI applies the suggestion in the clone with the user's own signing
	136	// key: the push passes the signed-commit check, and the working tree is
	137	// left as it was.
	138	func TestSuggestionAppliedLocallyWhenSigned(t *testing.T) {
	139	t.Parallel()
	140	inst := startInstance(t)
	141	aliceKey := inst.newKey(t, "alice")
	142	inst.admin(t, "admin", "user", "create", "alice",
	143	"--key", aliceKey+".pub", "--email", "alice@example.test", "--verified")
	144	c := suggestionCLI(t, inst, aliceKey)
	145	c.must(t, "", "", "repo", "create", "alice/sec")
	146	c.must(t, "", "", "repo", "settings", "require-signed", "alice/sec", "on")
	147
	148	// alice signs with her SSH key, as git's own config says to.
	149	ident := []string{"GIT_AUTHOR_NAME=alice", "GIT_AUTHOR_EMAIL=alice@example.test",
	150	"GIT_COMMITTER_NAME=alice", "GIT_COMMITTER_EMAIL=alice@example.test"}
	151	env := append(inst.gitEnv(aliceKey), ident...)
	152	c.env = ident
	153	work := t.TempDir()
	154	mustGit(t, work, env, "clone", inst.sshURL("alice/sec"), "w")
	155	dir := filepath.Join(work, "w")
	156	mustGit(t, dir, env, "config", "gpg.format", "ssh")
	157	mustGit(t, dir, env, "config", "user.signingkey", aliceKey)
	158	mustGit(t, dir, env, "config", "commit.gpgsign", "true")
	159	mustGit(t, dir, env, "checkout", "-q", "-b", "main")
	160	os.WriteFile(filepath.Join(dir, "lib.txt"), []byte("one\n"), 0o644)
	161	mustGit(t, dir, env, "add", ".")
	162	mustGit(t, dir, env, "commit", "-q", "-m", "base")
	163	mustGit(t, dir, env, "push", "-q", "origin", "main")
	164	mustGit(t, dir, env, "checkout", "-q", "-b", "feat")
	165	os.WriteFile(filepath.Join(dir, "lib.txt"), []byte("one\ntwo\nthree\n"), 0o644)
	166	mustGit(t, dir, env, "commit", "-q", "-am", "more")
	167	mustGit(t, dir, env, "push", "-q", "origin", "feat")
	168	c.must(t, dir, "", "mr", "create", "alice/sec", "--source", "feat", "--target", "main", "--title", "more")
	169	mustGit(t, dir, env, "checkout", "-q", "main")
	170
	171	thread := postSuggestion(t, inst, aliceKey, "alice/sec")
	172	if th := threadsOf(t, inst, aliceKey, "alice/sec"); th[0].Suggestion == nil \|\| th[0].Suggestion.Apply != "local" {
	173	t.Fatalf("threads = %+v, want a suggestion applied locally", th)
	174	}
	175	_, errOut, code := inst.ssh(t, aliceKey, "", "mr", "apply-suggestion", "alice/sec", "1", thread)
	176	if code != 4 \|\| !strings.Contains(errOut, "gitbay mr apply-suggestion alice/sec 1 "+thread) {
	177	t.Fatalf("server-side apply on a require-signed repository: exit %d %s", code, errOut)
	178	}
	179
	180	out, errOut, code := c.run(t, dir, "", "mr", "apply-suggestion", "1", thread, "--json")
	181	if code != 0 {
	182	t.Fatalf("local apply-suggestion: exit %d\n%s\n%s", code, out, errOut)
	183	}
	184	var res struct {
	185	Data struct {
	186	SHA string `json:"sha"`
	187	Branch string `json:"branch"`
	188	Resolved bool `json:"resolved"`
	189	} `json:"data"`
	190	}
	191	if err := json.Unmarshal([]byte(out), &res); err != nil \|\| res.Data.Branch != "feat" \|\| !res.Data.Resolved {
	192	t.Fatalf("local apply-suggestion --json = %s (%v)", out, err)
	193	}
	194	mustGit(t, dir, env, "fetch", "-q", "origin", "feat")
	195	if tip := strings.TrimSpace(mustGit(t, dir, env, "rev-parse", "FETCH_HEAD")); tip != res.Data.SHA {
	196	t.Errorf("reported sha %s, branch at %s", res.Data.SHA, tip)
	197	}
	198	if got := mustGit(t, dir, env, "show", "FETCH_HEAD:lib.txt"); got != "one\nTWO\nTWO AND A HALF\nthree\n" {
	199	t.Fatalf("lib.txt = %q", got)
	200	}
	201	if msg := mustGit(t, dir, env, "log", "-1", "--format=%B", "FETCH_HEAD"); !strings.Contains(msg, "Thread "+thread+" on alice/sec!1") {
	202	t.Errorf("commit message = %q", msg)
	203	}
	204	if branch := strings.TrimSpace(mustGit(t, dir, env, "branch", "--show-current")); branch != "main" {
	205	t.Errorf("checked-out branch moved to %q", branch)
	206	}
	207	if st := mustGit(t, dir, env, "status", "--porcelain"); st != "" {
	208	t.Errorf("working tree touched:\n%s", st)
	209	}
	210	if th := threadsOf(t, inst, aliceKey, "alice/sec"); th[0].Resolved != "alice" {
	211	t.Errorf("thread not resolved: %+v", th[0])
	212	}
	213	}

internal/control/commitfile.go +3

 			"%s requires signed commits; this writes an unsigned one — push a signed commit instead",
 			repo.Path())
 	}
+	if code := checkStorageQuota(c, repo); code >= 0 {
+		return code
+	}
 	// A commit carries an identity, and an unverified address is not one.
 	email, err := c.Store.PrimaryVerifiedEmail(c.User.ID)
 	if err != nil {

internal/control/diffcomment.go +104 −27

 	"gitbay.org/gitbay/internal/policy"
 	"gitbay.org/gitbay/internal/protocol"
 	"gitbay.org/gitbay/internal/store"
+	"gitbay.org/gitbay/internal/suggest"
 )
 func init() {
 	register(Command{Path: []string{"mr", "diff-comment"},
 		Summary: "comment on a diff line",
-		Usage:   "mr diff-comment <owner/name> <n> --path <file> --line <l> [--old] [--pending] [--reply <id>] [--message <m> | --file -]",
+		Usage:   "mr diff-comment <owner/name> <n> --path <file> --line <l> [--start-line <s>] [--old] [--pending] [--reply <id>] [--message <m> | --file -]",
 		Flags: []Flag{
 			{"--path", "<file>", "the file the comment is on", ""},
-			{"--line", "<l>", "the line the comment is on", ""},
+			{"--line", "<l>", "the line the comment is on, or the last line of a range", ""},
+			{"--start-line", "<s>", "the first line of a range ending at --line", ""},
 			{"--old", "", "the line is on the old side of the diff", ""},
 			{"--pending", "", "hold the comment for `mr review --comment`", ""},
 			{"--reply", "<id>", "reply to this thread instead of opening one", ""},
 		Examples: []string{
 			`mr diff-comment krz/gitbay 431 --path internal/control/build.go --line 42 --message "why is this a switch"`,
 			"mr diff-comment krz/gitbay 431 --reply 12 --file - < notes.md",
+			"mr diff-comment krz/gitbay 431 --path go.mod --start-line 3 --line 4 --file - < suggestion.md",
 		},
 		ReadsStdin: true, Run: runDiffComment})
 	register(Command{Path: []string{"mr", "threads"},
 }
 func runDiffComment(c *Ctx, args []string) int {
-	f, err := c.parseArgs(args, flagSpec{Values: []string{"--path", "--line", "--reply", "--message", "--file"},
+	f, err := c.parseArgs(args, flagSpec{Values: []string{"--path", "--line", "--start-line", "--reply", "--message", "--file"},
 		Bools: []string{"--old", "--pending"}, MaxPos: -1,
-		Usage: "mr diff-comment <owner/name> <n> --path <file> --line <l> [--old] [--pending] [--reply <id>] [--message <m> | --file -]"})
+		Usage: "mr diff-comment <owner/name> <n> --path <file> --line <l> [--start-line <s>] [--old] [--pending] [--reply <id>] [--message <m> | --file -]"})
 	if err != nil {
 		return c.fail(protocol.ExitUsage, "%v", err)
 	}
 	rest := f.Pos
 	path, message, file, old := f.Value("--path"), f.Value("--message"), f.Value("--file"), f.Has("--old")
-	var line, replyTo int64
+	var line, startLine, replyTo int64
 	if f.Has("--line") {
 		n, err := strconv.ParseInt(f.Value("--line"), 10, 64)
 		if err != nil || n < 1 {
 		}
 		line = n
 	}
+	if f.Has("--start-line") {
+		n, err := strconv.ParseInt(f.Value("--start-line"), 10, 64)
+		if err != nil || n < 1 || (line != 0 && n > line) {
+			return c.fail(protocol.ExitUsage, "--start-line must be a positive number no greater than --line")
+		}
+		startLine = n
+	}
 	if f.Has("--reply") {
 		n, err := strconv.ParseInt(f.Value("--reply"), 10, 64)
 		if err != nil || n < 1 {
 	if strings.TrimSpace(body) == "" {
 		return c.fail(protocol.ExitUsage, "empty comment; use --message or --file -")
 	}
+	if replyTo != 0 && startLine != 0 {
+		return c.fail(protocol.ExitUsage, "a reply takes its thread's lines; drop --start-line")
+	}
+	_, hasSuggestion, err := suggest.Parse(body)
+	if err != nil {
+		return c.fail(protocol.ExitUsage, "%v", err)
+	}
+	if hasSuggestion && replyTo != 0 {
+		return c.fail(protocol.ExitUsage, "a suggestion opens its own thread: post it with --path and --line, not --reply")
+	}
+	if hasSuggestion && old {
+		return c.fail(protocol.ExitUsage, "a suggestion replaces lines of the new file; drop --old")
+	}
 	side := "new"
 	if old {
 		if !slices.Contains(files, path) {
 			return c.fail(protocol.ExitUsage, "%s is not part of this merge request's diff", path)
 		}
+		if hasSuggestion {
+			first := firstNonZero(startLine, line)
+			content, _, err := readAnchored(dir, mr.HeadSHA, path)
+			if err != nil {
+				return c.fail(protocol.ExitUsage, "%v", err)
+			}
+			if _, ok := suggest.Range(content, int(first), int(line)); !ok {
+				return c.fail(protocol.ExitUsage, "%s has no lines %d-%d at the head", path, first, line)
+			}
+		}
 	}
 	pending := f.Has("--pending")
-	id, err := c.Store.AddDiffComment(mr.ID, c.User.ID, mr.HeadSHA, path, side, line, body, replyTo, pending)
+	id, err := c.Store.AddDiffComment(mr.ID, c.User.ID, mr.HeadSHA, path, side, line, startLine, body, replyTo, pending)
 	if err != nil {
 		if errors.Is(err, store.ErrNotFound) {
 			return c.fail(protocol.ExitNotFound, "%v", err)
 		CreatedAt string `json:"created_at"`
 	}
 	type threadOut struct {
-		ID       int64        `json:"id"`
+		ID         int64          `json:"id"`
-		Path     string       `json:"path"`
+		Path       string         `json:"path"`
-		Side     string       `json:"side"`
+		Side       string         `json:"side"`
-		Line     int64        `json:"line"`
+		StartLine  int64          `json:"start_line,omitempty"`
-		Stale    bool         `json:"stale"`
+		Line       int64          `json:"line"`
-		Resolved string       `json:"resolved_by,omitempty"`
+		Stale      bool           `json:"stale"`
-		Comments []commentOut `json:"comments"`
+		Resolved   string         `json:"resolved_by,omitempty"`
+		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 {
 		if cm.ReplyTo == 0 {
 			byRoot[cm.ID] = &threadOut{
-				ID: cm.ID, Path: cm.Path, Side: cm.Side, Line: cm.Line,
+				ID: cm.ID, Path: cm.Path, Side: cm.Side, StartLine: cm.StartLine, Line: cm.Line,
 				Stale: cm.HeadSHA != mr.HeadSHA, Resolved: cm.ResolvedBy,
-				Comments: []commentOut{{cm.ID, cm.Author, cm.Body, cm.CreatedAt}},
+				Suggestion: suggestions[cm.ID],
+				Comments:   []commentOut{{cm.ID, cm.Author, cm.Body, cm.CreatedAt}},
 			}
 			order = append(order, cm.ID)
 		} else if th, ok := byRoot[cm.ReplyTo]; ok {
 	for _, id := range order {
 		ds = append(ds, *byRoot[id])
 	}
-	_ = repo
 	return c.emit(ds, func(w io.Writer) {
 		for _, th := range ds {
 			marks := ""
 			if th.Stale {
 				marks += " [stale]"
 			}
-			fmt.Fprintf(w, "thread %d  %s:%d (%s)%s\n", th.ID, th.Path, th.Line, th.Side, marks)
+			lines := fmt.Sprint(th.Line)
-			for _, cm := range th.Comments {
+			if th.StartLine != 0 && th.StartLine != th.Line {
-				fmt.Fprintf(w, "  %s: %s\n", cm.Author, cm.Body)
+				lines = fmt.Sprintf("%d-%d", th.StartLine, th.Line)
+			}
+			fmt.Fprintf(w, "thread %d  %s:%s (%s)%s\n", th.ID, th.Path, lines, th.Side, marks)
+			for i, cm := range th.Comments {
+				body := cm.Body
+				if i == 0 && th.Suggestion != nil {
+					body = suggest.Strip(body)
+				}
+				if body != "" {
+					fmt.Fprintf(w, "  %s: %s\n", cm.Author, body)
+				}
+				if i == 0 && th.Suggestion != nil {
+					writeSuggestion(w, repo, mr, th.ID, cm.Author, th.Suggestion)
+				}
 			}
 		}
 	})
 }
+// writeSuggestion prints a suggestion as the diff it proposes and how to
+// apply it, or why it can no longer be applied.
+func writeSuggestion(w io.Writer, repo store.Repo, mr store.MR, thread int64, author string, s *SuggestionOut) {
+	switch {
+	case s.Outdated:
+		fmt.Fprintf(w, "  %s suggests (outdated: %s):\n", author, s.Reason)
+	default:
+		fmt.Fprintf(w, "  %s suggests (gitbay mr apply-suggestion %s %d %d):\n", author, repo.Path(), mr.Number, thread)
+	}
+	for _, l := range suggest.FromText(strings.ReplaceAll(s.Original, "\r\n", "\n")) {
+		fmt.Fprintf(w, "  - %s\n", l)
+	}
+	for _, l := range suggest.FromText(s.Replacement) {
+		fmt.Fprintf(w, "  + %s\n", l)
+	}
+}
 func setThreadResolved(c *Ctx, args []string, resolved bool) int {
 	if len(args) != 3 {
 		return c.usage()
 	if err != nil {
 		return c.fail(protocol.ExitUsage, "bad thread id %q", args[2])
 	}
-	// Thread author, MR author, or anyone with write may resolve.
+	ok, err := canResolveThread(c, repo, mr, threadID)
-	author, err := c.Store.DiffCommentAuthor(mr.ID, threadID)
 	if errors.Is(err, store.ErrNotFound) {
 		return c.fail(protocol.ExitNotFound, "no thread %d on %s!%d", threadID, repo.Path(), mr.Number)
 	}
 	if err != nil {
 		return c.fail(protocol.ExitFailure, "%v", err)
 	}
-	grant, err := c.Store.AccessRole(repo.ID, c.User.ID)
+	if !ok {
-	if err != nil {
+		return c.fail(protocol.ExitDenied, "%s", cannotResolve)
-		return c.fail(protocol.ExitFailure, "%v", err)
-	}
-	if author != c.User.ID && mr.Author != c.User.Username && !policy.CanWrite(c.User, repo, grant) {
-		return c.fail(protocol.ExitDenied, "only the thread author, the MR author, or users with write access can resolve threads")
 	}
 	if err := c.Store.SetThreadResolved(mr.ID, threadID, c.User.ID, resolved); err != nil {
 		if errors.Is(err, store.ErrNotFound) {
 	})
 }
+const cannotResolve = "only the thread author, the MR author, or users with write access can resolve threads"
+// canResolveThread reports whether the caller may resolve a thread: its
+// author, the MR author, or anyone with write on the target.
+func canResolveThread(c *Ctx, repo store.Repo, mr store.MR, threadID int64) (bool, error) {
+	author, err := c.Store.DiffCommentAuthor(mr.ID, threadID)
+	if err != nil {
+		return false, err
+	}
+	grant, err := c.Store.AccessRole(repo.ID, c.User.ID)
+	if err != nil {
+		return false, err
+	}
+	return author == c.User.ID || mr.Author == c.User.Username || policy.CanWrite(c.User, repo, grant), nil
+}
 func runMRResolve(c *Ctx, args []string) int   { return setThreadResolved(c, args, true) }
 func runMRUnresolve(c *Ctx, args []string) int { return setThreadResolved(c, args, false) }

internal/control/mergequeue_test.go +1 −1

	@@ -232,7 +232,7 @@ func TestWhenReadyReviewMerges(t *testing.T) {
232	// Resolving the last open thread merges the queued request.	232	// Resolving the last open thread merges the queued request.
233	func TestWhenReadyThreadResolveMerges(t *testing.T) {	233	func TestWhenReadyThreadResolveMerges(t *testing.T) {
234	f := newQueueFixture(t, func(s *store.RepoSettings) { s.RequireResolved = true })	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	if err != nil {	236	if err != nil {
237	t.Fatal(err)	237	t.Fatal(err)
238	}	238	}

internal/control/quota.go +19

	@@ -81,6 +81,25 @@ func checkRepoQuota(c *Ctx) int {
81	return -1	81	return -1
82	}	82	}
83		83
		84	// checkStorageQuota refuses a server-side write into repo once its
		85	// owner's storage quota is used up, the check sshd makes before a push.
		86	// Repositories an org owns have no quota.
		87	func checkStorageQuota(c *Ctx, repo store.Repo) int {
		88	if repo.OwnerKind != "user" {
		89	return -1
		90	}
		91	limit := ByteLimit(c.Store, limitsOf(c), repo.OwnerID)
		92	if limit <= 0 {
		93	return -1
		94	}
		95	if used := OwnedBytes(c.Store, c.Cfg.Server.Root, repo.OwnerID); used >= limit {
		96	return c.fail(protocol.ExitDenied,
		97	"%s's storage quota is used up (%d of %d bytes); delete something, or ask an admin to raise the limit",
		98	repo.OwnerName, used, limit)
		99	}
		100	return -1
		101	}
		102
84	func init() {	103	func init() {
85	register(Command{Path: []string{"admin", "user", "limits"},	104	register(Command{Path: []string{"admin", "user", "limits"},
86	Summary: "show or set an account's repository and storage caps (instance admins)",	105	Summary: "show or set an account's repository and storage caps (instance admins)",

internal/control/refsupdated.go added +212

@@ -0,0 +1,212 @@
	1	package control
	2
	3	import (
	4	"encoding/json"
	5	"fmt"
	6	"log/slog"
	7	"path"
	8	"strings"
	9	"time"
	10
	11	"gitbay.org/gitbay/internal/ci"
	12	"gitbay.org/gitbay/internal/config"
	13	"gitbay.org/gitbay/internal/gitutil"
	14	"gitbay.org/gitbay/internal/policy"
	15	"gitbay.org/gitbay/internal/store"
	16	)
	17
	18	// RefsUpdated is the work that follows a ref update in repoID by userID,
	19	// with a key or token of scope: post-receive runs it for every push, and
	20	// a server-side write to a branch (an applied suggestion) runs it after
	21	// its own ref update, so nothing a push triggers is skipped. A push to a
	22	// source branch refreshes refs/merge-requests/N/head in every target
	23	// repo, by fetching — the target owns the objects, so the MR outlives the
	24	// fork. This is the only place a push writes outside its own repository.
	25	func RefsUpdated(st *store.Store, cfg config.Config, repoID, userID int64, scope string, updates []policy.RefUpdate) {
	26	pushedRepo, pushedRepoErr := st.RepoByID(repoID)
	27	if pushedRepoErr == nil {
	28	adoptDefaultBranch(st, cfg, &pushedRepo, updates)
	29	}
	30	for _, u := range updates {
	31	// Every ref update is an event webhooks can subscribe to.
	32	st.RecordEvent(repoID, userID, "push", fmt.Sprintf(
	33	`{"ref":%q,"old":%q,"new":%q,"forced":%v,"deleted":%v}`,
	34	u.Ref, u.Old, u.New, u.IsForce, u.IsDelete))
	35
	36	// Any ref update — branch or tag — schedules the push mirrors.
	37	st.MarkMirrorsDirty(repoID, "push")
	38
	39	// Tag pushes run the tag-triggered CI jobs.
	40	if tag, ok := strings.CutPrefix(u.Ref, "refs/tags/"); ok && !u.IsDelete && pushedRepoErr == nil {
	41	QueueTagBuilds(st, cfg, pushedRepo, userID, tag, u.New)
	42	}
	43
	44	branch, ok := cutHeads(u.Ref)
	45	if !ok {
	46	continue
	47	}
	48	// Commits landing on the default branch act on issue references
	49	// in their messages (closes #N, plain #N).
	50	if pushedRepoErr == nil && branch == pushedRepo.DefaultBranch && !u.IsDelete {
	51	dir := RepoDir(cfg.Server.Root, pushedRepo.OwnerName, pushedRepo.Name)
	52	ProcessCommitMessages(st, dir, pushedRepo, userID, scope, u.Old, u.New)
	53	RecordLandedCommits(st, dir, pushedRepo, u.Old, u.New)
	54	}
	55	// A branch push with a .gitbay/ci.yml queues one build per job.
	56	if pushedRepoErr == nil && !u.IsDelete {
	57	QueueBranchBuilds(st, cfg.Server.Root, cfg.Server.SiteURL,
	58	pushedRepo, userID, branch, u.Old, u.New, time.Now())
	59	}
	60	if u.IsForce {
	61	st.Audit(userID, "push.forced", map[string]any{
	62	"repo": repoID, "ref": u.Ref, "old": u.Old, "new": u.New})
	63	}
	64	mrs, err := st.OpenMRsBySource(repoID, branch)
	65	if err != nil {
	66	slog.Error("post-receive: listing MRs", "err", err)
	67	continue
	68	}
	69	srcRepo, err := st.RepoByID(repoID)
	70	if err != nil {
	71	continue
	72	}
	73	srcDir := RepoDir(cfg.Server.Root, srcRepo.OwnerName, srcRepo.Name)
	74	for _, mr := range mrs {
	75	target, err := st.RepoByID(mr.RepoID)
	76	if err != nil {
	77	continue
	78	}
	79	if u.IsDelete {
	80	if mr.State == "open" {
	81	st.SetMRState(mr.ID, "source_gone")
	82	}
	83	if mr.QueuedAt != "" {
	84	TryQueuedMerge(st, cfg, mr.ID) // dequeues: the source is gone
	85	}
	86	continue // head ref retained: the diff stays viewable
	87	}
	88	dstDir := RepoDir(cfg.Server.Root, target.OwnerName, target.Name)
	89	headRef := fmt.Sprintf("refs/merge-requests/%d/head", mr.Number)
	90	if err := gitutil.FetchInto(dstDir, srcDir, u.New, headRef); err != nil {
	91	slog.Error("post-receive: refreshing MR head", "mr", mr.Number, "err", err)
	92	continue
	93	}
	94	// The merge base as it stands now, so a later range-diff
	95	// compares each revision against the target it was written
	96	// on rather than against today's. Best-effort: a base that
	97	// cannot be worked out costs precision, not the record.
	98	base, err := gitutil.MergeBase(dstDir, "refs/heads/"+mr.TargetRef, headRef)
	99	if err != nil {
	100	base = ""
	101	}
	102	if err := st.UpdateMRHead(mr.ID, u.New, base, sameChange(dstDir, mr, base, u.New)); err != nil {
	103	slog.Error("post-receive: recording MR head", "mr", mr.Number, "err", err)
	104	}
	105	if srcRepo.ID != target.ID {
	106	QueueMRBuilds(st, cfg.Server.Root, cfg.Server.SiteURL,
	107	target, userID, mr.Number, u.New)
	108	}
	109	if mr.State == "source_gone" {
	110	st.SetMRState(mr.ID, "open") // branch came back
	111	}
	112	// A queued merge stays queued across a push by someone who can
	113	// merge it, and the new head has to pass the gates on its own.
	114	if mr.QueuedAt != "" {
	115	QueuedMergePushed(st, cfg, mr.ID, userID, scope)
	116	}
	117	}
	118	}
	119	}
	120
	121	// adoptDefaultBranch moves an unborn HEAD to the first branch a push
	122	// creates. A repository is initialised with HEAD at the stored default,
	123	// and a first push of master or trunk left HEAD naming a branch that did
	124	// not exist: clones checked out nothing and every surface asked git for
	125	// a branch that was not there (#189). A push that includes the default
	126	// branch itself needs nothing.
	127	func adoptDefaultBranch(st store.Store, cfg config.Config, repo store.Repo, updates []policy.RefUpdate) {
	128	dir := RepoDir(cfg.Server.Root, repo.OwnerName, repo.Name)
	129	if _, err := gitutil.ResolveRef(dir, "refs/heads/"+repo.DefaultBranch); err == nil {
	130	return
	131	}
	132	for _, u := range updates {
	133	branch, ok := cutHeads(u.Ref)
	134	if !ok \|\| u.IsDelete \|\| !gitutil.ZeroSHA(u.Old) {
	135	continue
	136	}
	137	if err := gitutil.SetHead(dir, branch); err != nil {
	138	slog.Error("post-receive: moving HEAD", "repo", repo.Path(), "err", err)
	139	return
	140	}
	141	if err := st.UpdateDefaultBranch(repo.ID, branch); err != nil {
	142	slog.Error("post-receive: recording default branch", "repo", repo.Path(), "err", err)
	143	return
	144	}
	145	repo.DefaultBranch = branch
	146	return
	147	}
	148	}
	149
	150	// sameChange reports whether the new head proposes the diff the old one
	151	// did: the patch-id of each revision against its own merge base. A
	152	// rebase onto a moved target changes every sha and nothing about the
	153	// change, and the reviews of it should not go stale for that (#198).
	154	// Any doubt answers false, which is the old behaviour.
	155	func sameChange(dir string, mr store.MR, newBase, newHead string) bool {
	156	if mr.HeadSHA == "" \|\| newBase == "" \|\| mr.HeadSHA == newHead {
	157	return false
	158	}
	159	oldBase, err := gitutil.MergeBase(dir, "refs/heads/"+mr.TargetRef, mr.HeadSHA)
	160	if err != nil {
	161	return false
	162	}
	163	oldID, err := gitutil.PatchID(dir, oldBase, mr.HeadSHA)
	164	if err != nil \|\| oldID == "" {
	165	return false
	166	}
	167	newID, err := gitutil.PatchID(dir, newBase, newHead)
	168	return err == nil && newID == oldID
	169	}
	170
	171	// QueueTagBuilds runs the jobs whose tag pattern matches a pushed tag.
	172	// The build records the tag as its ref and the peeled commit as its sha,
	173	// so statuses land on the commit, not an annotated tag object.
	174	func QueueTagBuilds(st *store.Store, cfg config.Config, repo store.Repo, userID int64, tag, pushed string) {
	175	dir := RepoDir(cfg.Server.Root, repo.OwnerName, repo.Name)
	176	sha, err := gitutil.PeelToCommit(dir, pushed)
	177	if err != nil {
	178	return
	179	}
	180	raw, err := gitutil.ReadBlob(dir, sha, ci.ConfigPath, 1<<16)
	181	if err != nil {
	182	return
	183	}
	184	jobs, err := ci.Parse(raw)
	185	if err != nil {
	186	return // the branch push already reported ci/config
	187	}
	188	for _, j := range jobs {
	189	if j.Tags == "" {
	190	continue
	191	}
	192	if ok, _ := path.Match(j.Tags, tag); !ok {
	193	continue
	194	}
	195	steps, _ := json.Marshal(j.Steps)
	196	n, err := st.CreateBuild(repo.ID, j.Name, sha, tag, string(steps), j.Image, "", true)
	197	if err != nil {
	198	slog.Error("queueing tag build", "repo", repo.Path(), "job", j.Name, "err", err)
	199	continue
	200	}
	201	url := fmt.Sprintf("%s/%s/builds/%d", cfg.Server.SiteURL, repo.Path(), n)
	202	st.SetCommitStatus(repo.ID, sha, "ci/"+j.Name, "pending", "tag "+tag, url, userID)
	203	}
	204	}
	205
	206	func cutHeads(ref string) (string, bool) {
	207	const p = "refs/heads/"
	208	if len(ref) > len(p) && ref[:len(p)] == p {
	209	return ref[len(p):], true
	210	}
	211	return "", false
	212	}

internal/control/suggestion.go added +374

@@ -0,0 +1,374 @@
	1	package control
	2
	3	import (
	4	"bytes"
	5	"errors"
	6	"fmt"
	7	"io"
	8	"strconv"
	9
	10	"gitbay.org/gitbay/internal/gitutil"
	11	"gitbay.org/gitbay/internal/policy"
	12	"gitbay.org/gitbay/internal/protocol"
	13	"gitbay.org/gitbay/internal/store"
	14	"gitbay.org/gitbay/internal/suggest"
	15	)
	16
	17	func init() {
	18	register(Command{Path: []string{"mr", "apply-suggestion"},
	19	Summary: "commit a review thread's suggestion to the source branch",
	20	Usage: "mr apply-suggestion <owner/name> <n> <thread-id>",
	21	Examples: []string{"mr apply-suggestion krz/gitbay 431 12"},
	22	Run: runMRApplySuggestion})
	23	}
	24
	25	// SuggestionOut is the change a review thread's ```suggestion block
	26	// proposes: lines StartLine through EndLine of Path, as they were at
	27	// Commit (Original), replaced by Replacement. Both texts end every line
	28	// with its terminator, so "" is no lines. Apply is "server" where `mr
	29	// apply-suggestion` commits it, or "local" on a repository requiring
	30	// signed commits, where the CLI commits it with the user's own key.
	31	type SuggestionOut struct {
	32	Path string `json:"path"`
	33	StartLine int64 `json:"start_line"`
	34	EndLine int64 `json:"end_line"`
	35	Commit string `json:"commit"`
	36	Blob string `json:"blob,omitempty"`
	37	Original string `json:"original"`
	38	Replacement string `json:"replacement"`
	39	Outdated bool `json:"outdated"`
	40	Reason string `json:"reason,omitempty"`
	41	Apply string `json:"apply"`
	42	}
	43
	44	// maxSuggestionBytes bounds the file a suggestion rewrites, the same as
	45	// a single-file commit over the control plane.
	46	const maxSuggestionBytes = maxCommitFileBytes
	47
	48	// Reasons a suggestion cannot be applied at a head.
	49	const (
	50	reasonGone = "the file is not at the head: it was renamed or deleted"
	51	reasonChanged = "the lines it replaces have changed since it was made"
	52	reasonNoBase = "the commit it was made against is no longer available"
	53	)
	54
	55	// anchoredFiles reads the files suggestions anchor in, for one page or
	56	// listing: one ls-tree per (commit, path), and every blob through one
	57	// cat-file --batch process started on first use, so a merge request with
	58	// many suggestions on a file costs no more processes than one with a
	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.
	61	type anchoredFiles struct {
	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
	69	}
	70
	71	// anchoredCacheCap bounds the blobs one render keeps.
	72	const anchoredCacheCap = 8 << 20
	73
	74	type anchoredEntry struct {
	75	e gitutil.TreeEntry
	76	ok bool
	77	}
	78
	79	func newAnchoredFiles(dir string) *anchoredFiles {
	80	return &anchoredFiles{dir: dir, entries: map[[2]string]anchoredEntry{}, blobs: map[string][]byte{},
	81	cacheCap: anchoredCacheCap}
	82	}
	83
	84	func (f *anchoredFiles) close() {
	85	if f.batch != nil {
	86	f.batch.Close()
	87	}
	88	}
	89
	90	// read returns the regular file path at commit, with its mode and blob id.
	91	func (f *anchoredFiles) read(commit, path string) ([]byte, string, string, error) {
	92	key := [2]string{commit, path}
	93	ent, seen := f.entries[key]
	94	if !seen {
	95	f.spawned++
	96	ent.e, ent.ok = gitutil.StatPath(f.dir, commit, path)
	97	f.entries[key] = ent
	98	}
	99	e := ent.e
	100	if !ent.ok {
	101	return nil, "", "", fmt.Errorf("%s", reasonGone)
	102	}
	103	if e.Type != "blob" \|\| (e.Mode != "100644" && e.Mode != "100755") {
	104	return nil, "", "", fmt.Errorf("%s is not a regular file", path)
	105	}
	106	if e.Size > maxSuggestionBytes {
	107	return nil, "", "", fmt.Errorf("%s is larger than %d bytes", path, maxSuggestionBytes)
	108	}
	109	if content, ok := f.blobs[e.SHA]; ok {
	110	return content, e.Mode, e.SHA, nil
	111	}
	112	if f.batch == nil {
	113	b, err := gitutil.NewBlobBatch(f.dir)
	114	if err != nil {
	115	return nil, "", "", err
	116	}
	117	f.spawned++
	118	f.batch = b
	119	}
	120	content, err := f.batch.Read(e.SHA, maxSuggestionBytes)
	121	if err != nil {
	122	return nil, "", "", err
	123	}
	124	if f.cached+int64(len(content)) <= f.cacheCap {
	125	f.blobs[e.SHA] = content
	126	f.cached += int64(len(content))
	127	}
	128	return content, e.Mode, e.SHA, nil
	129	}
	130
	131	// readAnchored reads the regular file path at commit, with its mode.
	132	func readAnchored(dir, commit, path string) ([]byte, string, error) {
	133	f := newAnchoredFiles(dir)
	134	defer f.close()
	135	content, mode, _, err := f.read(commit, path)
	136	return content, mode, err
	137	}
	138
	139	// suggestionRange is a thread's first and last line.
	140	func suggestionRange(cm store.DiffComment) (int, int) {
	141	return int(firstNonZero(cm.StartLine, cm.Line)), int(cm.Line)
	142	}
	143
	144	// Suggestions are the suggestions the thread roots among comments carry,
	145	// by thread id, each checked against the merge request's head. The
	146	// original lines are read from the commit the comment was made on, in
	147	// the target repository, which holds every head the merge request has
	148	// had until gc prunes an abandoned one.
	149	func Suggestions(st store.Store, root string, repo store.Repo, mr store.MR, comments []store.DiffComment) map[int64]SuggestionOut {
	150	f := newAnchoredFiles(RepoDir(root, repo.OwnerName, repo.Name))
	151	defer f.close()
	152	return suggestionsWith(f, func() bool { return signedOnly(st, repo, mr) }, mr, comments)
	153	}
	154
	155	// suggestionsWith is Suggestions reading through f. signed is asked
	156	// once, and only when there is a suggestion.
	157	func suggestionsWith(f anchoredFiles, signed func() bool, mr store.MR, comments []store.DiffComment) map[int64]SuggestionOut {
	158	out := map[int64]*SuggestionOut{}
	159	apply := ""
	160	for _, cm := range comments {
	161	if cm.ReplyTo != 0 \|\| cm.Side != "new" {
	162	continue
	163	}
	164	lines, found, err := suggest.Parse(cm.Body)
	165	if err != nil \|\| !found {
	166	continue
	167	}
	168	if apply == "" {
	169	apply = "server"
	170	if signed() {
	171	apply = "local"
	172	}
	173	}
	174	start, end := suggestionRange(cm)
	175	s := &SuggestionOut{Path: cm.Path, StartLine: int64(start), EndLine: int64(end), Commit: cm.HeadSHA,
	176	Replacement: suggest.Text(lines), Apply: apply}
	177	out[cm.ID] = s
	178	base, _, blob, err := f.read(cm.HeadSHA, cm.Path)
	179	s.Blob = blob
	180	orig, ok := suggest.Range(base, start, end)
	181	if err != nil \|\| !ok {
	182	s.Outdated, s.Reason = true, reasonNoBase
	183	continue
	184	}
	185	s.Original = string(orig)
	186	s.Reason = anchorReason(f, mr.HeadSHA, s)
	187	s.Outdated = s.Reason != ""
	188	}
	189	return out
	190	}
	191
	192	// anchorReason says why s cannot be applied to the file at head, or ""
	193	// when the lines it replaces are still what it was made against.
	194	func anchorReason(f anchoredFiles, head string, s SuggestionOut) string {
	195	content, _, _, err := f.read(head, s.Path)
	196	if err != nil {
	197	return err.Error()
	198	}
	199	now, ok := suggest.Range(content, int(s.StartLine), int(s.EndLine))
	200	if !ok \|\| !bytes.Equal(now, []byte(s.Original)) {
	201	return reasonChanged
	202	}
	203	return ""
	204	}
	205
	206	// signedOnly reports whether the server may not commit a suggestion for
	207	// this merge request: the source branch or the target it merges into
	208	// requires signed commits, and the server has no key to sign with.
	209	func signedOnly(st *store.Store, repo store.Repo, mr store.MR) bool {
	210	if repo.Settings.RequireSignedCommits {
	211	return true
	212	}
	213	if mr.SourceRepoID == repo.ID {
	214	return false
	215	}
	216	src, err := st.RepoByID(mr.SourceRepoID)
	217	return err != nil \|\| src.Settings.RequireSignedCommits
	218	}
	219
	220	// runMRApplySuggestion commits a thread's suggestion to the merge
	221	// request's source branch as the caller, and resolves the thread.
	222	//
	223	// It is a push by the caller, and is held to what a push is: the caller
	224	// needs write on the source repository (for a fork, the fork's writers;
	225	// write on the target grants nothing there), the update goes through the
	226	// pre-receive ref policy (policy.CheckPush: protected branches,
	227	// require-mr) before a compare-and-swap ref update, and RefsUpdated then
	228	// does what post-receive does, which moves the merge request's head and
	229	// reaches a queued merge. The server has no signing key, so where either
	230	// repository requires signed commits it refuses, and the CLI applies the
	231	// suggestion locally instead.
	232	func runMRApplySuggestion(c *Ctx, args []string) int {
	233	if len(args) != 3 {
	234	return c.usage()
	235	}
	236	repo, mr, code := mrRef(c, args[:2], policy.CanRead)
	237	if code >= 0 {
	238	return code
	239	}
	240	if code := refuseArchived(c, repo); code >= 0 {
	241	return code
	242	}
	243	threadID, err := strconv.ParseInt(args[2], 10, 64)
	244	if err != nil {
	245	return c.fail(protocol.ExitUsage, "bad thread id %q", args[2])
	246	}
	247	cm, err := c.Store.DiffCommentByID(mr.ID, threadID)
	248	if errors.Is(err, store.ErrNotFound) \|\| (err == nil && cm.Pending && cm.Author != c.User.Username) {
	249	return c.fail(protocol.ExitNotFound, "no thread %d on %s!%d", threadID, repo.Path(), mr.Number)
	250	}
	251	if err != nil {
	252	return c.failErr(err)
	253	}
	254	if cm.ReplyTo != 0 {
	255	return c.fail(protocol.ExitUsage, "%d is a reply; name the thread root %d", threadID, cm.ReplyTo)
	256	}
	257	if cm.Pending {
	258	return c.fail(protocol.ExitUsage, "thread %d is in your unsubmitted review; submit it with `mr review` first", threadID)
	259	}
	260	if mr.State != "open" {
	261	return c.fail(protocol.ExitUsage, "%s!%d is %s", repo.Path(), mr.Number, mr.State)
	262	}
	263	s := Suggestions(c.Store, c.Cfg.Server.Root, repo, mr, []store.DiffComment{cm})[cm.ID]
	264	if s == nil {
	265	return c.fail(protocol.ExitUsage, "thread %d carries no suggestion", threadID)
	266	}
	267
	268	src, err := c.Store.RepoByID(mr.SourceRepoID)
	269	if err != nil {
	270	return c.failErr(err)
	271	}
	272	grant, err := c.Store.AccessRole(src.ID, c.User.ID)
	273	if err != nil {
	274	return c.failErr(err)
	275	}
	276	source := mr.SourceRef
	277	if src.ID != repo.ID {
	278	source = src.Path() + ":" + mr.SourceRef
	279	}
	280	if !policy.CanWrite(c.User, src, grant) {
	281	return c.fail(protocol.ExitDenied, "applying a suggestion pushes to %s; only its writers can", source)
	282	}
	283	if src.Settings.Archived {
	284	return c.fail(protocol.ExitDenied, "%s is archived and read-only", src.Path())
	285	}
	286	if mirrored, err := c.Store.PullMirrored(src.ID); err != nil {
	287	return c.failErr(err)
	288	} else if mirrored {
	289	return c.fail(protocol.ExitDenied, "%s is a pull mirror: its refs come from the upstream", src.Path())
	290	}
	291	if s.Apply == "local" {
	292	return c.fail(protocol.ExitDenied,
	293	"%s requires signed commits and the server cannot sign one; apply it from a clone, which commits with your own key: gitbay mr apply-suggestion %s %d %d",
	294	source, repo.Path(), mr.Number, threadID)
	295	}
	296	if code := checkStorageQuota(c, src); code >= 0 {
	297	return code
	298	}
	299	email, err := c.Store.PrimaryVerifiedEmail(c.User.ID)
	300	if err != nil {
	301	return c.failErr(err)
	302	}
	303	if email == "" {
	304	return c.fail(protocol.ExitDenied, "commits carry your identity: your account needs a verified primary email")
	305	}
	306
	307	srcDir := RepoDir(c.Cfg.Server.Root, src.OwnerName, src.Name)
	308	ref := "refs/heads/" + mr.SourceRef
	309	tip, err := gitutil.ResolveRef(srcDir, ref)
	310	if err != nil {
	311	return c.fail(protocol.ExitUsage, "the source branch %s is gone", source)
	312	}
	313	if s.Reason == reasonNoBase {
	314	return c.fail(protocol.ExitUsage, "suggestion in thread %d cannot be applied: %s", threadID, s.Reason)
	315	}
	316	files := newAnchoredFiles(srcDir)
	317	defer files.close()
	318	if reason := anchorReason(files, tip, s); reason != "" {
	319	return c.fail(protocol.ExitUsage, "suggestion in thread %d is outdated: %s", threadID, reason)
	320	}
	321	content, mode, _, err := files.read(tip, s.Path)
	322	if err != nil {
	323	return c.fail(protocol.ExitUsage, "%v", err)
	324	}
	325	updated, err := suggest.Apply(content, int(s.StartLine), int(s.EndLine), suggest.FromText(s.Replacement))
	326	if err != nil {
	327	return c.fail(protocol.ExitUsage, "%v", err)
	328	}
	329	if bytes.Equal(updated, content) {
	330	return c.fail(protocol.ExitUsage, "the suggestion in thread %d changes nothing", threadID)
	331	}
	332	message := suggest.Message(repo.Path(), mr.Number, threadID, cm.Author)
	333	sha, err := gitutil.CommitWithFile(srcDir, tip, s.Path, mode, updated, c.User.Username, email, message)
	334	if err != nil {
	335	return c.failErr(err)
	336	}
	337	updates := []policy.RefUpdate{{Ref: ref, Old: tip, New: sha}}
	338	if msg := policy.CheckPush(src, updates); msg != "" {
	339	return c.fail(protocol.ExitDenied, "%s", msg)
	340	}
	341	if err := gitutil.UpdateRefCAS(srcDir, ref, sha, tip); err != nil {
	342	return c.fail(protocol.ExitFailure, "the source branch moved; reload and retry")
	343	}
	344	RefsUpdated(c.Store, c.Cfg, src.ID, c.User.ID, c.Scope, updates)
	345
	346	// The commit has landed, so from here nothing fails the command: a
	347	// thread that cannot be resolved is left open and the output says so.
	348	resolved, warning := false, ""
	349	switch ok, err := canResolveThread(c, repo, mr, threadID); {
	350	case err != nil:
	351	warning = fmt.Sprintf("thread %d is still open: %v", threadID, err)
	352	case !ok:
	353	warning = fmt.Sprintf("thread %d is still open: %s", threadID, cannotResolve)
	354	default:
	355	if err := c.Store.SetThreadResolved(mr.ID, threadID, c.User.ID, true); err != nil {
	356	warning = fmt.Sprintf("thread %d is still open: %v", threadID, err)
	357	} else {
	358	resolved = true
	359	TryQueuedMerge(c.Store, c.Cfg, mr.ID)
	360	}
	361	}
	362	d := map[string]any{"thread": threadID, "sha": sha, "source": source, "resolved": resolved}
	363	if warning != "" {
	364	d["warning"] = warning
	365	}
	366	return c.emit(d, func(w io.Writer) {
	367	if resolved {
	368	fmt.Fprintf(w, "applied thread %d to %s at %.10s; thread resolved\n", threadID, source, sha)
	369	return
	370	}
	371	fmt.Fprintf(w, "applied thread %d to %s at %.10s\n", threadID, source, sha)
	372	fmt.Fprintln(c.Stderr, "warning:", warning)
	373	})
	374	}

internal/control/suggestion_test.go added +565

@@ -0,0 +1,565 @@
	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	c.Cfg.Limits.WriteRate = -1
	53	argv := []string{"mr", "diff-comment", f.repo.Path(), "1", "--path", path,
	54	"--start-line", strconv.Itoa(start), "--line", strconv.Itoa(end), "--file", "-"}
	55	if code := Dispatch(c, argv); code != protocol.ExitOK {
	56	f.t.Fatalf("diff-comment: exit %d, %s", code, errOut.String())
	57	}
	58	var env struct {
	59	Data struct {
	60	Thread int64 `json:"thread"`
	61	} `json:"data"`
	62	}
	63	if err := json.Unmarshal([]byte(out.String()), &env); err != nil {
	64	f.t.Fatal(err)
	65	}
	66	return strconv.Itoa(int(env.Data.Thread))
	67	}
	68
	69	type threadJSON struct {
	70	ID int64 `json:"id"`
	71	StartLine int64 `json:"start_line"`
	72	Line int64 `json:"line"`
	73	Resolved string `json:"resolved_by"`
	74	Suggestion *SuggestionOut `json:"suggestion"`
	75	}
	76
	77	// unlimited lifts the per-account write limit, which these tests would
	78	// otherwise spend for every test in the package that writes as uid 1.
	79	func unlimited(c *Ctx) { c.Cfg.Limits.WriteRate = -1 }
	80
	81	func (f *queueFixture) mustWrite(u store.User, argv ...string) string {
	82	f.t.Helper()
	83	code, out, errOut := f.runWith(u, unlimited, argv...)
	84	if code != protocol.ExitOK {
	85	f.t.Fatalf("%v: exit %d, %s", argv, code, errOut)
	86	}
	87	return out
	88	}
	89
	90	func (f *queueFixture) threads(u store.User) []threadJSON {
	91	f.t.Helper()
	92	out := f.mustRun(u, "mr", "threads", f.repo.Path(), "1", "--json")
	93	var env struct {
	94	Data []threadJSON `json:"data"`
	95	}
	96	if err := json.Unmarshal([]byte(out), &env); err != nil {
	97	f.t.Fatalf("threads JSON: %v\n%s", err, out)
	98	}
	99	return env.Data
	100	}
	101
	102	func (f queueFixture) suggestion(u store.User, thread string) SuggestionOut {
	103	f.t.Helper()
	104	for _, th := range f.threads(u) {
	105	if strconv.Itoa(int(th.ID)) == thread {
	106	return th.Suggestion
	107	}
	108	}
	109	f.t.Fatalf("no thread %s", thread)
	110	return nil
	111	}
	112
	113	// A suggestion over a range reads back from mr threads as structure:
	114	// the anchor, the commit and blob it was made against, the lines it
	115	// replaces and the replacement.
	116	func TestSuggestionInThreads(t *testing.T) {
	117	f := newSuggestFixture(t, nil)
	118	id := f.suggest(f.alice, "lib.txt", 2, 3, "TWO", "THREE", "extra")
	119	s := f.suggestion(f.alice, id)
	120	if s == nil {
	121	t.Fatal("thread carries no suggestion")
	122	}
	123	blob := strings.TrimSpace(f.git(f.dir, "rev-parse", f.headSHA+":lib.txt"))
	124	want := SuggestionOut{Path: "lib.txt", StartLine: 2, EndLine: 3, Commit: f.headSHA, Blob: blob,
	125	Original: "two\nthree\n", Replacement: "TWO\nTHREE\nextra\n", Apply: "server"}
	126	if *s != want {
	127	t.Fatalf("suggestion = %+v\nwant %+v", *s, want)
	128	}
	129	text := f.mustRun(f.alice, "mr", "threads", f.repo.Path(), "1")
	130	for _, w := range []string{"lib.txt:2-3", "- two", "+ THREE", "mr apply-suggestion alice/app 1 " + id} {
	131	if !strings.Contains(text, w) {
	132	t.Errorf("threads text lacks %q:\n%s", w, text)
	133	}
	134	}
	135	if strings.Contains(text, "```suggestion") {
	136	t.Errorf("threads text repeats the raw block:\n%s", text)
	137	}
	138	}
	139
	140	// The suggestion goes stale when the lines it replaces change, and not
	141	// when the file changes elsewhere.
	142	func TestSuggestionOutdated(t *testing.T) {
	143	f := newSuggestFixture(t, nil)
	144	id := f.suggest(f.alice, "lib.txt", 2, 2, "TWO")
	145	f.write("lib.txt", "one\ntwo\nthree\nfour\nFIVE\n")
	146	f.git(f.src, "commit", "-q", "-am", "elsewhere")
	147	f.moveHead()
	148	if s := f.suggestion(f.alice, id); s.Outdated {
	149	t.Fatalf("a change below the range outdated the suggestion: %+v", s)
	150	}
	151	f.write("lib.txt", "zero\none\ntwo\nthree\nfour\nFIVE\n")
	152	f.git(f.src, "commit", "-q", "-am", "shift")
	153	f.moveHead()
	154	if s := f.suggestion(f.alice, id); !s.Outdated \|\| s.Reason != reasonChanged {
	155	t.Fatalf("suggestion after its lines moved = %+v, want outdated", s)
	156	}
	157	f.git(f.src, "rm", "-q", "lib.txt")
	158	f.git(f.src, "commit", "-q", "-m", "gone")
	159	f.moveHead()
	160	if s := f.suggestion(f.alice, id); !s.Outdated \|\| s.Reason != reasonGone {
	161	t.Fatalf("suggestion on a deleted file = %+v, want outdated", s)
	162	}
	163	}
	164
	165	// On a repository requiring signed commits the server does not commit a
	166	// suggestion, and the thread says the CLI applies it locally.
	167	func TestSuggestionSignedIsLocal(t *testing.T) {
	168	f := newSuggestFixture(t, func(s *store.RepoSettings) { s.RequireSignedCommits = true })
	169	id := f.suggest(f.alice, "lib.txt", 1, 1, "ONE")
	170	if s := f.suggestion(f.alice, id); s.Apply != "local" {
	171	t.Fatalf("apply = %q, want local", s.Apply)
	172	}
	173	}
	174
	175	// What diff-comment refuses before storing a suggestion it could never
	176	// apply.
	177	func TestSuggestionRefusals(t *testing.T) {
	178	f := newSuggestFixture(t, nil)
	179	id := f.suggest(f.alice, "lib.txt", 1, 1, "ONE")
	180	run := func(body string, extra ...string) (int, string) {
	181	var out, errOut strings.Builder
	182	c := &Ctx{User: f.alice, Scope: "full", Store: f.st, Stdout: &out, Stderr: &errOut,
	183	Stdin: strings.NewReader(body)}
	184	c.Cfg.Server.Root = f.root
	185	c.Cfg.Limits.WriteRate = -1
	186	return Dispatch(c, append([]string{"mr", "diff-comment", f.repo.Path(), "1", "--file", "-"}, extra...)), errOut.String()
	187	}
	188	block := "```suggestion\nx\n```\n"
	189	cases := []struct {
	190	name string
	191	body string
	192	args []string
	193	want string
	194	}{
	195	{"old side", block, []string{"--path", "lib.txt", "--line", "1", "--old"}, "drop --old"},
	196	{"reply", block, []string{"--reply", id}, "its own thread"},
	197	{"past the end", block, []string{"--path", "lib.txt", "--start-line", "5", "--line", "6"}, "no lines 5-6"},
	198	{"unclosed", "```suggestion\nx\n", []string{"--path", "lib.txt", "--line", "1"}, "not closed"},
	199	{"start after line", "plain", []string{"--path", "lib.txt", "--start-line", "3", "--line", "2"}, "no greater than --line"},
	200	}
	201	for _, c := range cases {
	202	code, errOut := run(c.body, c.args...)
	203	if code != protocol.ExitUsage \|\| !strings.Contains(errOut, c.want) {
	204	t.Errorf("%s: exit %d %q, want usage with %q", c.name, code, errOut, c.want)
	205	}
	206	}
	207	}
	208
	209	// verified gives u a verified primary address, which a commit needs.
	210	func (f *queueFixture) verified(u store.User) {
	211	f.t.Helper()
	212	if err := f.st.AddEmail(u.ID, u.Username+"@example.test", "admin", true); err != nil {
	213	f.t.Fatal(err)
	214	}
	215	}
	216
	217	// applyOK applies thread as u and returns the new commit, checking what
	218	// every successful apply must have done: one commit on the old head by
	219	// u, naming the merge request and thread, the merge request moved to
	220	// it, and the thread resolved.
	221	func (f *queueFixture) applyOK(u store.User, thread string) string {
	222	f.t.Helper()
	223	old := strings.TrimSpace(f.git(f.dir, "rev-parse", "refs/heads/feature"))
	224	out := f.mustWrite(u, "mr", "apply-suggestion", f.repo.Path(), "1", thread, "--json")
	225	var env struct {
	226	Data struct {
	227	SHA string `json:"sha"`
	228	} `json:"data"`
	229	}
	230	json.Unmarshal([]byte(out), &env)
	231	sha := env.Data.SHA
	232	if tip := strings.TrimSpace(f.git(f.dir, "rev-parse", "refs/heads/feature")); tip != sha \|\| sha == "" {
	233	f.t.Fatalf("feature = %s, apply reported %q", tip, sha)
	234	}
	235	if parent := strings.TrimSpace(f.git(f.dir, "rev-parse", sha+"^")); parent != old {
	236	f.t.Fatalf("parent = %s, want the old head %s", parent, old)
	237	}
	238	meta := f.git(f.dir, "log", "-1", "--format=%an <%ae>\|%cn <%ce>\|%B", sha)
	239	who := u.Username + " <" + u.Username + "@example.test>"
	240	if !strings.HasPrefix(meta, who+"\|"+who+"\|") \|\| !strings.Contains(meta, "Thread "+thread+" on alice/app!1") {
	241	f.t.Fatalf("commit = %q", meta)
	242	}
	243	if mr := f.mr(); mr.HeadSHA != sha {
	244	f.t.Fatalf("MR head = %s, want %s", mr.HeadSHA, sha)
	245	}
	246	if head := strings.TrimSpace(f.git(f.dir, "rev-parse", mrHeadRef(1))); head != sha {
	247	f.t.Fatalf("MR head ref = %s, want %s", head, sha)
	248	}
	249	for _, th := range f.threads(u) {
	250	if strconv.Itoa(int(th.ID)) == thread && th.Resolved != u.Username {
	251	f.t.Fatalf("thread %s resolved by %q, want %s", thread, th.Resolved, u.Username)
	252	}
	253	}
	254	return sha
	255	}
	256
	257	func (f *queueFixture) file(sha, path string) string {
	258	f.t.Helper()
	259	return f.git(f.dir, "show", sha+":"+path)
	260	}
	261
	262	// Each shape of range: several lines to more, deletion, the last line of
	263	// a file with no final newline, and a CRLF file.
	264	func TestApplySuggestion(t *testing.T) {
	265	f := newSuggestFixture(t, nil)
	266	f.verified(f.alice)
	267	cases := []struct {
	268	path string
	269	start, end int
	270	lines []string
	271	want string
	272	}{
	273	{"lib.txt", 2, 3, []string{"TWO", "THREE", "3.5"}, "one\nTWO\nTHREE\n3.5\nfour\nfive\n"},
	274	{"lib.txt", 5, 6, nil, "one\nTWO\nTHREE\n3.5\n"},
	275	{"tail.txt", 2, 2, []string{"LAST", "more"}, "x\nLAST\nmore"},
	276	{"dos.txt", 2, 2, []string{"B", "B2"}, "a\r\nB\r\nB2\r\nc\r\n"},
	277	}
	278	for _, c := range cases {
	279	id := f.suggest(f.alice, c.path, c.start, c.end, c.lines...)
	280	sha := f.applyOK(f.alice, id)
	281	if got := f.file(sha, c.path); got != c.want {
	282	t.Errorf("%s %d-%d: file = %q, want %q", c.path, c.start, c.end, got, c.want)
	283	}
	284	}
	285	}
	286
	287	// A suggestion whose lines changed, or whose file is gone, is refused;
	288	// so is one on a repository requiring signed commits, with the command
	289	// that applies it locally.
	290	func TestApplySuggestionRefusals(t *testing.T) {
	291	f := newSuggestFixture(t, nil)
	292	f.verified(f.alice)
	293	stale := f.suggest(f.alice, "lib.txt", 1, 1, "ONE")
	294	gone := f.suggest(f.alice, "tail.txt", 1, 1, "X")
	295	plain := f.suggest(f.alice, "lib.txt", 2, 2, "two")
	296	f.write("lib.txt", "uno\ntwo\nthree\nfour\nfive\n")
	297	f.git(f.src, "rm", "-q", "tail.txt")
	298	f.git(f.src, "commit", "-q", "-am", "moved on")
	299	f.moveHead()
	300	cases := []struct {
	301	thread string
	302	code int
	303	want string
	304	}{
	305	{stale, protocol.ExitUsage, "outdated: " + reasonChanged},
	306	{gone, protocol.ExitUsage, reasonGone},
	307	{plain, protocol.ExitUsage, "changes nothing"},
	308	{"999", protocol.ExitNotFound, "no thread 999"},
	309	}
	310	for _, c := range cases {
	311	code, _, errOut := f.runWith(f.alice, unlimited, "mr", "apply-suggestion", f.repo.Path(), "1", c.thread)
	312	if code != c.code \|\| !strings.Contains(errOut, c.want) {
	313	t.Errorf("thread %s: exit %d %q, want %d with %q", c.thread, code, errOut, c.code, c.want)
	314	}
	315	}
	316
	317	if _, err := f.st.UpdateRepoSettings(f.repo.ID, func(s *store.RepoSettings) { s.RequireSignedCommits = true }); err != nil {
	318	t.Fatal(err)
	319	}
	320	ok := f.suggest(f.alice, "lib.txt", 2, 2, "TWO")
	321	code, _, errOut := f.runWith(f.alice, unlimited, "mr", "apply-suggestion", f.repo.Path(), "1", ok)
	322	if code != protocol.ExitDenied \|\| !strings.Contains(errOut, "gitbay mr apply-suggestion alice/app 1 "+ok) {
	323	t.Fatalf("signed repo: exit %d %q", code, errOut)
	324	}
	325	}
	326
	327	// The update is held to the pre-receive ref policy a push is: a source
	328	// branch that is protected under require-mr refuses it.
	329	func TestApplySuggestionHonoursRefPolicy(t *testing.T) {
	330	f := newSuggestFixture(t, func(s *store.RepoSettings) {
	331	s.ProtectedBranches = []string{"feature"}
	332	s.RequireMR = true
	333	})
	334	f.verified(f.alice)
	335	id := f.suggest(f.alice, "lib.txt", 1, 1, "ONE")
	336	before := strings.TrimSpace(f.git(f.dir, "rev-parse", "refs/heads/feature"))
	337	code, _, errOut := f.runWith(f.alice, unlimited, "mr", "apply-suggestion", f.repo.Path(), "1", id)
	338	if code != protocol.ExitDenied \|\| !strings.Contains(errOut, "merge requests only") {
	339	t.Fatalf("exit %d %q, want the require-mr refusal", code, errOut)
	340	}
	341	if after := strings.TrimSpace(f.git(f.dir, "rev-parse", "refs/heads/feature")); after != before {
	342	t.Fatal("refused apply moved the branch")
	343	}
	344	}
	345
	346	// Only the source branch's writers apply: a reader cannot. A thread in
	347	// an unsubmitted review is not applied, and to anyone but its author it
	348	// does not exist.
	349	func TestApplySuggestionNeedsWrite(t *testing.T) {
	350	f := newSuggestFixture(t, nil)
	351	carol := f.user("carol", "read")
	352	f.verified(carol)
	353	id := f.suggest(f.alice, "lib.txt", 1, 1, "ONE")
	354	code, _, errOut := f.runWith(carol, unlimited, "mr", "apply-suggestion", f.repo.Path(), "1", id)
	355	if code != protocol.ExitDenied \|\| !strings.Contains(errOut, "only its writers") {
	356	t.Fatalf("reader: exit %d %q", code, errOut)
	357	}
	358
	359	var out, stderr strings.Builder
	360	c := &Ctx{User: f.alice, Scope: "full", Store: f.st, Stdout: &out, Stderr: &stderr, JSON: true,
	361	Stdin: strings.NewReader("```suggestion\nONE\n```\n")}
	362	c.Cfg.Server.Root = f.root
	363	c.Cfg.Limits.WriteRate = -1
	364	if code := Dispatch(c, []string{"mr", "diff-comment", f.repo.Path(), "1", "--path", "lib.txt", "--line", "1", "--pending", "--file", "-"}); code != protocol.ExitOK {
	365	t.Fatalf("pending diff-comment: %s", stderr.String())
	366	}
	367	var env struct {
	368	Data struct {
	369	Thread int64 `json:"thread"`
	370	} `json:"data"`
	371	}
	372	json.Unmarshal([]byte(out.String()), &env)
	373	pending := strconv.Itoa(int(env.Data.Thread))
	374	if code, _, errOut := f.runWith(f.alice, unlimited, "mr", "apply-suggestion", f.repo.Path(), "1", pending); code != protocol.ExitUsage \|\| !strings.Contains(errOut, "unsubmitted") {
	375	t.Errorf("own pending thread: exit %d %q", code, errOut)
	376	}
	377	if code, _, _ := f.runWith(carol, unlimited, "mr", "apply-suggestion", f.repo.Path(), "1", pending); code != protocol.ExitNotFound {
	378	t.Errorf("someone else's pending thread: exit %d, want not found", code)
	379	}
	380	}
	381
	382	// Applying is a push by the applier, so a queued merge sees it: a
	383	// writer's apply keeps the queue, and resolving the thread it came from
	384	// lets the queued merge land on the new head.
	385	func TestApplySuggestionReachesQueuedMerge(t *testing.T) {
	386	f := newSuggestFixture(t, func(s *store.RepoSettings) { s.RequireResolved = true })
	387	f.verified(f.alice)
	388	id := f.suggest(f.alice, "lib.txt", 1, 1, "ONE")
	389	f.mustWrite(f.alice, "mr", "merge", f.repo.Path(), "1", "--when-ready")
	390	f.wantQueued("threads resolved")
	391	sha := f.applyOK(f.alice, id)
	392	f.wantMergedBy("alice")
	393	if main := strings.TrimSpace(f.git(f.dir, "rev-parse", "refs/heads/main")); main != sha {
	394	t.Fatalf("main = %s, want the applied commit %s", main, sha)
	395	}
	396	}
	397
	398	// On a merge request from a fork the source branch is the fork's, so its
	399	// writers apply and the target's do not. The fork writer's apply is a
	400	// push by someone who cannot merge into the target, which dequeues a
	401	// merge queued there.
	402	func TestApplySuggestionFork(t *testing.T) {
	403	f := newSuggestFixture(t, nil)
	404	bobID, err := f.st.CreateUser("bob", false)
	405	if err != nil {
	406	t.Fatal(err)
	407	}
	408	bob, _ := f.st.UserByID(bobID)
	409	f.verified(f.alice)
	410	f.verified(bob)
	411	forkID, err := f.st.CreateRepo("user", bobID, "app", "public")
	412	if err != nil {
	413	t.Fatal(err)
	414	}
	415	fork, _ := f.st.RepoByID(forkID)
	416	forkDir := RepoDir(f.root, fork.OwnerName, fork.Name)
	417	f.git(f.root, "clone", "-q", "--bare", f.src, forkDir)
	418	f.git(f.dir, "update-ref", mrHeadRef(2), f.headSHA)
	419	if _, err := f.st.CreateMR(f.repo.ID, bobID, forkID, "feature", "main", "forked", "", f.headSHA, "md", false); err != nil {
	420	t.Fatal(err)
	421	}
	422	var out, errOut strings.Builder
	423	c := &Ctx{User: f.alice, Scope: "full", Store: f.st, Stdout: &out, Stderr: &errOut, JSON: true,
	424	Stdin: strings.NewReader("```suggestion\nONE\n```\n")}
	425	c.Cfg.Server.Root = f.root
	426	c.Cfg.Limits.WriteRate = -1
	427	if code := Dispatch(c, []string{"mr", "diff-comment", f.repo.Path(), "2", "--path", "lib.txt", "--line", "1", "--file", "-"}); code != protocol.ExitOK {
	428	t.Fatalf("diff-comment: %s", errOut.String())
	429	}
	430	var env struct {
	431	Data struct {
	432	Thread int64 `json:"thread"`
	433	} `json:"data"`
	434	}
	435	json.Unmarshal([]byte(out.String()), &env)
	436	thread := strconv.Itoa(int(env.Data.Thread))
	437
	438	code, _, stderr := f.runWith(f.alice, unlimited, "mr", "apply-suggestion", f.repo.Path(), "2", thread)
	439	if code != protocol.ExitDenied \|\| !strings.Contains(stderr, "bob/app:feature") {
	440	t.Fatalf("target owner on a fork's branch: exit %d %q", code, stderr)
	441	}
	442
	443	if _, err := f.st.UpdateRepoSettings(f.repo.ID, func(s *store.RepoSettings) { s.RequireApprovals = 1 }); err != nil {
	444	t.Fatal(err)
	445	}
	446	f.mustWrite(f.alice, "mr", "merge", f.repo.Path(), "2", "--when-ready", "--strategy", "merge")
	447	f.mustWrite(bob, "mr", "apply-suggestion", f.repo.Path(), "2", thread)
	448	tip := strings.TrimSpace(f.git(forkDir, "rev-parse", "refs/heads/feature"))
	449	if got := f.git(forkDir, "show", tip+":lib.txt"); !strings.HasPrefix(got, "ONE\ntwo\n") {
	450	t.Fatalf("fork's lib.txt = %q", got)
	451	}
	452	mr, _ := f.st.MRByNumber(f.repo.ID, 2)
	453	if mr.HeadSHA != tip {
	454	t.Fatalf("MR head = %s, want the fork's new tip %s", mr.HeadSHA, tip)
	455	}
	456	if mr.QueuedAt != "" \|\| mr.State != "open" {
	457	t.Fatalf("queued merge after the fork writer's apply: state %s queued %q", mr.State, mr.QueuedAt)
	458	}
	459	cs, _ := f.st.ListMRComments(mr.ID)
	460	said := false
	461	for _, c := range cs {
	462	said = said \|\| (c.Kind == "system" && strings.Contains(c.Body, "bob pushed and cannot merge"))
	463	}
	464	if !said {
	465	t.Fatalf("timeline does not say why the merge was dequeued: %+v", cs)
	466	}
	467	// carol writes to the fork and is neither the thread's author, the
	468	// merge request's, nor a writer of the target: her apply lands and
	469	// leaves the thread open, saying so.
	470	carolID, _ := f.st.CreateUser("carol", false)
	471	carol, _ := f.st.UserByID(carolID)
	472	f.verified(carol)
	473	if err := f.st.GrantAccess(forkID, carolID, "write"); err != nil {
	474	t.Fatal(err)
	475	}
	476	out.Reset()
	477	errOut.Reset()
	478	c.Stdin = strings.NewReader("```suggestion\nTWO\n```\n")
	479	if code := Dispatch(c, []string{"mr", "diff-comment", f.repo.Path(), "2", "--path", "lib.txt", "--line", "2", "--file", "-"}); code != protocol.ExitOK {
	480	t.Fatalf("diff-comment: %s", errOut.String())
	481	}
	482	json.Unmarshal([]byte(out.String()), &env)
	483	second := strconv.Itoa(int(env.Data.Thread))
	484	stdout := f.mustWrite(carol, "mr", "apply-suggestion", f.repo.Path(), "2", second, "--json")
	485	if !strings.Contains(stdout, `"resolved":false`) \|\| !strings.Contains(stdout, "still open") {
	486	t.Fatalf("carol's apply = %s, want it applied and the thread left open", stdout)
	487	}
	488	if n, _ := f.st.UnresolvedThreadCount(mr.ID); n != 1 {
	489	t.Fatalf("unresolved threads = %d, want carol's left open", n)
	490	}
	491	}
	492
	493	// Reading suggestions costs git processes per file and commit, not per
	494	// thread: six suggestions on one file read it as one does, whether or
	495	// not the blobs fit the cache.
	496	func TestSuggestionsProcessCountPerFile(t *testing.T) {
	497	f := newSuggestFixture(t, nil)
	498	spawnedWith := func(cacheCap int64) int {
	499	t.Helper()
	500	comments, err := f.st.ListDiffComments(f.mr().ID, f.alice.ID)
	501	if err != nil {
	502	t.Fatal(err)
	503	}
	504	files := newAnchoredFiles(f.dir)
	505	files.cacheCap = cacheCap
	506	defer files.close()
	507	got := suggestionsWith(files, func() bool { return false }, f.mr(), comments)
	508	for _, s := range got {
	509	if s.Outdated {
	510	t.Fatalf("suggestion outdated: %+v", s)
	511	}
	512	}
	513	if files.cached > cacheCap {
	514	t.Fatalf("cached %d bytes past a cap of %d", files.cached, cacheCap)
	515	}
	516	return files.spawned
	517	}
	518	spawned := func() int { return spawnedWith(anchoredCacheCap) }
	519	f.suggest(f.alice, "lib.txt", 1, 1, "ONE")
	520	one := spawned()
	521	for i := 2; i <= 5; i++ {
	522	f.suggest(f.alice, "lib.txt", i, i, "X")
	523	}
	524	f.suggest(f.alice, "lib.txt", 1, 2, "Y")
	525	if six := spawned(); six != one \|\| one > 2 {
	526	t.Fatalf("git processes: %d for one suggestion, %d for six on the same file", one, six)
	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	}
	533	}
	534
	535	// A server-side write stops at the owner's storage quota as a push does:
	536	// both apply-suggestion and repo commit-file.
	537	func TestServerWritesHonourStorageQuota(t *testing.T) {
	538	f := newSuggestFixture(t, nil)
	539	f.verified(f.alice)
	540	id := f.suggest(f.alice, "lib.txt", 1, 1, "ONE")
	541	full := func(c *Ctx) {
	542	c.Cfg.Limits.WriteRate = -1
	543	c.Cfg.Limits.MaxBytesPerUser = 1
	544	}
	545	before := strings.TrimSpace(f.git(f.dir, "rev-parse", "refs/heads/feature"))
	546	code, _, errOut := f.runWith(f.alice, full, "mr", "apply-suggestion", f.repo.Path(), "1", id)
	547	if code != protocol.ExitDenied \|\| !strings.Contains(errOut, "storage quota is used up") {
	548	t.Errorf("apply-suggestion over quota: exit %d %q", code, errOut)
	549	}
	550	code, _, errOut = f.runWith(f.alice, func(c *Ctx) { full(c); c.Stdin = strings.NewReader("x\n") },
	551	"repo", "commit-file", f.repo.Path(), "new.txt", "--ref", "feature", "--file", "-")
	552	if code != protocol.ExitDenied \|\| !strings.Contains(errOut, "storage quota is used up") {
	553	t.Errorf("commit-file over quota: exit %d %q", code, errOut)
	554	}
	555	if after := strings.TrimSpace(f.git(f.dir, "rev-parse", "refs/heads/feature")); after != before {
	556	t.Fatal("a refused write moved the branch")
	557	}
	558	// Under the quota both go through.
	559	f.mustWrite(f.alice, "mr", "apply-suggestion", f.repo.Path(), "1", id)
	560	code, _, errOut = f.runWith(f.alice, func(c *Ctx) { unlimited(c); c.Stdin = strings.NewReader("x\n") },
	561	"repo", "commit-file", f.repo.Path(), "new.txt", "--ref", "feature", "--file", "-")
	562	if code != protocol.ExitOK {
	563	t.Errorf("commit-file under quota: exit %d %q", code, errOut)
	564	}
	565	}

internal/gitutil/batch.go added +81

@@ -0,0 +1,81 @@
	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	// 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	}
	70	data := make([]byte, size+1) // the object and its trailing newline
	71	if _, err := io.ReadFull(b.out, data); err != nil {
	72	return nil, err
	73	}
	74	return data[:size], nil
	75	}
	76
	77	// Close ends the process.
	78	func (b *BlobBatch) Close() error {
	79	b.in.Close()
	80	return b.cmd.Wait()
	81	}

internal/gitutil/batch_test.go added +53

@@ -0,0 +1,53 @@
	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 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	}
	44	if _, err := batch.Read(tree, 100); err == nil {
	45	t.Error("a tree was read as a blob")
	46	}
	47	if _, err := batch.Read(strings.Repeat("0", 40), 100); err == nil {
	48	t.Error("a missing object was read")
	49	}
	50	if got, err := batch.Read(a, 100); err != nil \|\| string(got) != "one\ntwo" {
	51	t.Fatalf("read after refusals = %q, %v", got, err)
	52	}
	53	}

internal/gitutil/merge.go +17 −9

 		parent = ""
 	}
+	sha, err := CommitWithFile(dir, parent, path, "100644", content, name, email, message)
+	if err != nil {
+		return "", err
+	}
+	if err := UpdateRefCAS(dir, branchRef, sha, parent); err != nil {
+		return "", fmt.Errorf("branch moved during edit; reload and retry: %w", err)
+	}
+	return sha, nil
+}
+// CommitWithFile writes a commit on parent ("" for a root commit) whose
+// tree is parent's with content at path, as a file of mode (100644 or
+// 100755), authored and committed as name <email>. No ref moves: the
+// caller updates one, and enforces policy, since no hook runs.
+func CommitWithFile(dir, parent, path, mode string, content []byte, name, email, message string) (string, error) {
 	// Hash the new blob.
 	hb := exec.Command(toolpath.Look("git"), "-C", dir, "hash-object", "-w", "--stdin")
 	hb.Stdin = strings.NewReader(string(content))
 	if out, err := rt.CombinedOutput(); err != nil {
 		return "", fmt.Errorf("read-tree: %v\n%s", err, out)
 	}
-	ui := exec.Command(toolpath.Look("git"), "-C", dir, "update-index", "--add", "--cacheinfo", "100644,"+blob+","+path)
+	ui := exec.Command(toolpath.Look("git"), "-C", dir, "update-index", "--add", "--cacheinfo", mode+","+blob+","+path)
 	ui.Env = env
 	if out, err := ui.CombinedOutput(); err != nil {
 		return "", fmt.Errorf("update-index: %v\n%s", err, out)
 	if parent != "" {
 		parents = []string{parent}
 	}
-	sha, err := CommitTree(dir, tree, parents, name, email, message)
+	return CommitTree(dir, tree, parents, name, email, message)
-	if err != nil {
-		return "", err
-	}
-	if err := UpdateRefCAS(dir, branchRef, sha, parent); err != nil {
-		return "", fmt.Errorf("branch moved during edit; reload and retry: %w", err)
-	}
-	return sha, nil
 }
 // isEmptyRepo reports whether dir has no refs at all — a repository

internal/hookd/hookd.go +3 −200

	@@ -15,12 +15,9 @@ import (
15	"log/slog"	15	"log/slog"
16	"net"	16	"net"
17	"os"	17	"os"
18	"path"
19	"path/filepath"	18	"path/filepath"
20	"strings"	19	"strings"
21	"time"
22		20
23	"gitbay.org/gitbay/internal/ci"
24	"gitbay.org/gitbay/internal/config"	21	"gitbay.org/gitbay/internal/config"
25	"gitbay.org/gitbay/internal/control"	22	"gitbay.org/gitbay/internal/control"
26	"gitbay.org/gitbay/internal/gitutil"	23	"gitbay.org/gitbay/internal/gitutil"
	@@ -275,204 +272,10 @@ func (s *Server) releaseAnchors(repo store.Repo, updates []policy.RefUpdate) str
275	return ""	272	return ""
276	}	273	}
277		274
278	// postReceive applies the cross-repo MR effect: a push to a source branch	275	// postReceive runs the ref-update work for a push; see
279	// refreshes refs/merge-requests/N/head in every target repo, by fetching —	276	// control.RefsUpdated, which server-side writes to a branch share.
280	// the target owns the objects, so the MR outlives the fork. This is the only
281	// place a hook writes outside its own repository.
282	func (s *Server) postReceive(req Request) {	277	func (s *Server) postReceive(req Request) {
283	pushedRepo, pushedRepoErr := s.st.RepoByID(req.RepoID)	278	control.RefsUpdated(s.st, s.cfg, req.RepoID, req.UserID, req.Scope, req.Updates)
284	if pushedRepoErr == nil {
285	s.adoptDefaultBranch(&pushedRepo, req.Updates)
286	}
287	for _, u := range req.Updates {
288	// Every ref update is an event webhooks can subscribe to.
289	s.st.RecordEvent(req.RepoID, req.UserID, "push", fmt.Sprintf(
290	`{"ref":%q,"old":%q,"new":%q,"forced":%v,"deleted":%v}`,
291	u.Ref, u.Old, u.New, u.IsForce, u.IsDelete))
292
293	// Any ref update — branch or tag — schedules the push mirrors.
294	s.st.MarkMirrorsDirty(req.RepoID, "push")
295
296	// Tag pushes run the tag-triggered CI jobs.
297	if tag, ok := strings.CutPrefix(u.Ref, "refs/tags/"); ok && !u.IsDelete && pushedRepoErr == nil {
298	s.queueTagBuilds(pushedRepo, req.UserID, tag, u.New)
299	}
300
301	branch, ok := cutHeads(u.Ref)
302	if !ok {
303	continue
304	}
305	// Commits landing on the default branch act on issue references
306	// in their messages (closes #N, plain #N).
307	if pushedRepoErr == nil && branch == pushedRepo.DefaultBranch && !u.IsDelete {
308	dir := control.RepoDir(s.cfg.Server.Root, pushedRepo.OwnerName, pushedRepo.Name)
309	control.ProcessCommitMessages(s.st, dir, pushedRepo, req.UserID, req.Scope, u.Old, u.New)
310	control.RecordLandedCommits(s.st, dir, pushedRepo, u.Old, u.New)
311	}
312	// A branch push with a .gitbay/ci.yml queues one build per job.
313	if pushedRepoErr == nil && !u.IsDelete {
314	s.queueBuilds(pushedRepo, req.UserID, branch, u.Old, u.New)
315	}
316	if u.IsForce {
317	s.st.Audit(req.UserID, "push.forced", map[string]any{
318	"repo": req.RepoID, "ref": u.Ref, "old": u.Old, "new": u.New})
319	}
320	mrs, err := s.st.OpenMRsBySource(req.RepoID, branch)
321	if err != nil {
322	slog.Error("post-receive: listing MRs", "err", err)
323	continue
324	}
325	srcRepo, err := s.st.RepoByID(req.RepoID)
326	if err != nil {
327	continue
328	}
329	srcDir := control.RepoDir(s.cfg.Server.Root, srcRepo.OwnerName, srcRepo.Name)
330	for _, mr := range mrs {
331	target, err := s.st.RepoByID(mr.RepoID)
332	if err != nil {
333	continue
334	}
335	if u.IsDelete {
336	if mr.State == "open" {
337	s.st.SetMRState(mr.ID, "source_gone")
338	}
339	if mr.QueuedAt != "" {
340	control.TryQueuedMerge(s.st, s.cfg, mr.ID) // dequeues: the source is gone
341	}
342	continue // head ref retained: the diff stays viewable
343	}
344	dstDir := control.RepoDir(s.cfg.Server.Root, target.OwnerName, target.Name)
345	headRef := fmt.Sprintf("refs/merge-requests/%d/head", mr.Number)
346	if err := gitutil.FetchInto(dstDir, srcDir, u.New, headRef); err != nil {
347	slog.Error("post-receive: refreshing MR head", "mr", mr.Number, "err", err)
348	continue
349	}
350	// The merge base as it stands now, so a later range-diff
351	// compares each revision against the target it was written
352	// on rather than against today's. Best-effort: a base that
353	// cannot be worked out costs precision, not the record.
354	base, err := gitutil.MergeBase(dstDir, "refs/heads/"+mr.TargetRef, headRef)
355	if err != nil {
356	base = ""
357	}
358	if err := s.st.UpdateMRHead(mr.ID, u.New, base, sameChange(dstDir, mr, base, u.New)); err != nil {
359	slog.Error("post-receive: recording MR head", "mr", mr.Number, "err", err)
360	}
361	if srcRepo.ID != target.ID {
362	control.QueueMRBuilds(s.st, s.cfg.Server.Root, s.cfg.Server.SiteURL,
363	target, req.UserID, mr.Number, u.New)
364	}
365	if mr.State == "source_gone" {
366	s.st.SetMRState(mr.ID, "open") // branch came back
367	}
368	// A queued merge stays queued across a push by someone who can
369	// merge it, and the new head has to pass the gates on its own.
370	if mr.QueuedAt != "" {
371	control.QueuedMergePushed(s.st, s.cfg, mr.ID, req.UserID, req.Scope)
372	}
373	}
374	}
375	}
376
377	// adoptDefaultBranch moves an unborn HEAD to the first branch a push
378	// creates. A repository is initialised with HEAD at the stored default,
379	// and a first push of master or trunk left HEAD naming a branch that did
380	// not exist: clones checked out nothing and every surface asked git for
381	// a branch that was not there (#189). A push that includes the default
382	// branch itself needs nothing.
383	func (s Server) adoptDefaultBranch(repo store.Repo, updates []policy.RefUpdate) {
384	dir := control.RepoDir(s.cfg.Server.Root, repo.OwnerName, repo.Name)
385	if _, err := gitutil.ResolveRef(dir, "refs/heads/"+repo.DefaultBranch); err == nil {
386	return
387	}
388	for _, u := range updates {
389	branch, ok := cutHeads(u.Ref)
390	if !ok \|\| u.IsDelete \|\| !gitutil.ZeroSHA(u.Old) {
391	continue
392	}
393	if err := gitutil.SetHead(dir, branch); err != nil {
394	slog.Error("post-receive: moving HEAD", "repo", repo.Path(), "err", err)
395	return
396	}
397	if err := s.st.UpdateDefaultBranch(repo.ID, branch); err != nil {
398	slog.Error("post-receive: recording default branch", "repo", repo.Path(), "err", err)
399	return
400	}
401	repo.DefaultBranch = branch
402	return
403	}
404	}
405
406	// sameChange reports whether the new head proposes the diff the old one
407	// did: the patch-id of each revision against its own merge base. A
408	// rebase onto a moved target changes every sha and nothing about the
409	// change, and the reviews of it should not go stale for that (#198).
410	// Any doubt answers false, which is the old behaviour.
411	func sameChange(dir string, mr store.MR, newBase, newHead string) bool {
412	if mr.HeadSHA == "" \|\| newBase == "" \|\| mr.HeadSHA == newHead {
413	return false
414	}
415	oldBase, err := gitutil.MergeBase(dir, "refs/heads/"+mr.TargetRef, mr.HeadSHA)
416	if err != nil {
417	return false
418	}
419	oldID, err := gitutil.PatchID(dir, oldBase, mr.HeadSHA)
420	if err != nil \|\| oldID == "" {
421	return false
422	}
423	newID, err := gitutil.PatchID(dir, newBase, newHead)
424	return err == nil && newID == oldID
425	}
426
427	// queueBuilds queues the push jobs for a branch update. The work is
428	// shared with the merge path, which moves a ref without reaching a hook.
429	func (s *Server) queueBuilds(repo store.Repo, userID int64, branch, old, sha string) {
430	control.QueueBranchBuilds(
431	s.st, s.cfg.Server.Root, s.cfg.Server.SiteURL,
432	repo, userID, branch, old, sha, time.Now())
433	}
434
435	// queueTagBuilds runs the jobs whose tag pattern matches a pushed tag.
436	// The build records the tag as its ref and the peeled commit as its sha,
437	// so statuses land on the commit, not an annotated tag object.
438	func (s *Server) queueTagBuilds(repo store.Repo, userID int64, tag, pushed string) {
439	dir := control.RepoDir(s.cfg.Server.Root, repo.OwnerName, repo.Name)
440	sha, err := gitutil.PeelToCommit(dir, pushed)
441	if err != nil {
442	return
443	}
444	raw, err := gitutil.ReadBlob(dir, sha, ci.ConfigPath, 1<<16)
445	if err != nil {
446	return
447	}
448	jobs, err := ci.Parse(raw)
449	if err != nil {
450	return // the branch push already reported ci/config
451	}
452	for _, j := range jobs {
453	if j.Tags == "" {
454	continue
455	}
456	if ok, _ := path.Match(j.Tags, tag); !ok {
457	continue
458	}
459	steps, _ := json.Marshal(j.Steps)
460	n, err := s.st.CreateBuild(repo.ID, j.Name, sha, tag, string(steps), j.Image, "", true)
461	if err != nil {
462	slog.Error("queueing tag build", "repo", repo.Path(), "job", j.Name, "err", err)
463	continue
464	}
465	url := fmt.Sprintf("%s/%s/builds/%d", s.cfg.Server.SiteURL, repo.Path(), n)
466	s.st.SetCommitStatus(repo.ID, sha, "ci/"+j.Name, "pending", "tag "+tag, url, userID)
467	}
468	}
469
470	func cutHeads(ref string) (string, bool) {
471	const p = "refs/heads/"
472	if len(ref) > len(p) && ref[:len(p)] == p {
473	return ref[len(p):], true
474	}
475	return "", false
476	}	279	}
477		280
478	// Ask sends one request from the hook process to the daemon. stream is	281	// Ask sends one request from the hook process to the daemon. stream is

internal/hookd/pushshapes_test.go +1 −1

	@@ -354,7 +354,7 @@ var pushShapes = []pushShape{
354	f.git(f.src, "tag", "v1")	354	f.git(f.src, "tag", "v1")
355	f.sync()	355	f.sync()
356	f.mark(f.base)	356	f.mark(f.base)
357	f.srv.queueTagBuilds(f.repo, f.uid, "v1", f.base)	357	control.QueueTagBuilds(f.st, f.srv.cfg, f.repo, f.uid, "v1", f.base)
358	}, []string{"—", "—", "—", "—", "queued", "—"}},	358	}, []string{"—", "—", "—", "—", "queued", "—"}},
359		359
360	{"schedule tick on the default branch", func(f *shapeFixture) {	360	{"schedule tick on the default branch", func(f *shapeFixture) {

internal/httpd/mractions.go +18

	@@ -140,6 +140,13 @@ func (s Server) mrDiffCommentSubmit(w http.ResponseWriter, r http.Request, u s
140	return	140	return
141	}	141	}
142	extra = []string{"--path", path, "--line", line}	142	extra = []string{"--path", path, "--line", line}
		143	if start := strings.TrimSpace(r.FormValue("start_line")); start != "" {
		144	if n, err := strconv.ParseInt(start, 10, 64); err != nil \|\| n < 1 {
		145	s.mrDiffRedirect(w, r, "the first line is a line number")
		146	return
		147	}
		148	extra = append(extra, "--start-line", start)
		149	}
143	if r.FormValue("side") == "old" {	150	if r.FormValue("side") == "old" {
144	extra = append(extra, "--old")	151	extra = append(extra, "--old")
145	}	152	}
	@@ -179,6 +186,17 @@ func (s Server) mrThreadSubmit(w http.ResponseWriter, r http.Request, u store.
179	s.done(w, r, code, msg, s.mrRedirect)	186	s.done(w, r, code, msg, s.mrRedirect)
180	}	187	}
181		188
		189	// mrSuggestionSubmit commits a thread's suggestion to the source branch.
		190	func (s Server) mrSuggestionSubmit(w http.ResponseWriter, r http.Request, u store.User) {
		191	id := strings.TrimSpace(r.FormValue("thread"))
		192	if _, err := strconv.ParseInt(id, 10, 64); err != nil {
		193	s.mrDiffRedirect(w, r, "bad thread id")
		194	return
		195	}
		196	_, msg, code := s.runControlCode(u, mrArgs(r, "apply-suggestion", id))
		197	s.done(w, r, code, msg, s.mrDiffRedirect)
		198	}
		199
182	// mrNewPage is the create form: branches to choose from, plus whatever	200	// mrNewPage is the create form: branches to choose from, plus whatever
183	// the last attempt had in it so a refusal does not lose the draft.	201	// the last attempt had in it so a refusal does not lose the draft.
184	type mrNewPage struct {	202	type mrNewPage struct {

internal/httpd/routes.go +2

 				Handler: s.checkOrigin(s.requireUser(s.mrThreadSubmit))},
 			Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/diff-comment", Mutating: true,
 				Handler: s.checkOrigin(s.requireUser(s.mrDiffCommentSubmit))},
+			Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/suggestion", Mutating: true,
+				Handler: s.checkOrigin(s.requireUser(s.mrSuggestionSubmit))},
 			Route{Method: "GET", Pattern: "/{owner}/{repo}/edit/{ref}/{path...}",
 				Handler: s.requireUser(s.editForm)},
 			Route{Method: "POST", Pattern: "/{owner}/{repo}/edit/{ref}/{path...}", Mutating: true,

internal/httpd/suggestion_test.go added +74

@@ -0,0 +1,74 @@
	1	package httpd
	2
	3	import (
	4	"html/template"
	5	"strings"
	6	"testing"
	7
	8	"gitbay.org/gitbay/internal/control"
	9	"gitbay.org/gitbay/internal/store"
	10	"gitbay.org/gitbay/internal/web"
	11	)
	12
	13	func renderSuggestion(t testing.T, sg control.SuggestionOut, canApply bool) string {
	14	t.Helper()
	15	view := newSuggestionView(sg, canApply, "gitbay mr apply-suggestion alice/app 1 7")
	16	var sb strings.Builder
	17	if err := web.Render(&sb, "mr.html", mrPageData{
	18	repoPage: testRepoPage(), MR: testMR("open"), View: "conversation",
	19	DetachedThreads: []diffThread{{ID: 7, Suggestion: view,
	20	Comments: []renderedComment{{Author: "bob", BodyHTML: template.HTML("<p>try</p>")}}}},
	21	}); err != nil {
	22	t.Fatalf("render: %v", err)
	23	}
	24	return sb.String()
	25	}
	26
	27	// A suggestion renders as the lines it replaces and the ones it proposes,
	28	// with a button for someone who can push to the source branch.
	29	func TestSuggestionRendersAsDiff(t *testing.T) {
	30	sg := &control.SuggestionOut{StartLine: 4, EndLine: 5, Original: "old a\r\nold b\r\n",
	31	Replacement: "new a\n", Apply: "server"}
	32	out := renderSuggestion(t, sg, true)
	33	for _, w := range []string{
	34	`<tr class="del"><td class="ln">4</td><td class="src">old a</td>`,
	35	`<tr class="del"><td class="ln">5</td><td class="src">old b</td>`,
	36	`<tr class="add"><td class="ln">4</td><td class="src">new a</td>`,
	37	`action="/krz/gitbay/mrs/42/suggestion"`, `name="thread" value="7"`, "Apply suggestion",
	38	} {
	39	if !strings.Contains(out, w) {
	40	t.Errorf("page lacks %q", w)
	41	}
	42	}
	43	if out := renderSuggestion(t, sg, false); strings.Contains(out, "Apply suggestion") {
	44	t.Error("apply button shown to someone who cannot push to the source branch")
	45	}
	46	}
	47
	48	// Where the server cannot sign, the page gives the command; an outdated
	49	// suggestion says why and offers neither.
	50	func TestSuggestionLocalAndOutdated(t *testing.T) {
	51	local := renderSuggestion(t, &control.SuggestionOut{StartLine: 1, EndLine: 1, Original: "a\n",
	52	Replacement: "b\n", Apply: "local"}, true)
	53	if !strings.Contains(local, "<code>gitbay mr apply-suggestion alice/app 1 7</code>") \|\| strings.Contains(local, "Apply suggestion</button>") {
	54	t.Error("require-signed suggestion does not give the CLI command in place of the button")
	55	}
	56	stale := renderSuggestion(t, &control.SuggestionOut{StartLine: 1, EndLine: 1, Original: "a\n",
	57	Replacement: "b\n", Apply: "server", Outdated: true, Reason: "the lines it replaces have changed"}, true)
	58	if !strings.Contains(stale, "outdated suggestion: the lines it replaces have changed") \|\| strings.Contains(stale, "Apply suggestion</button>") {
	59	t.Error("outdated suggestion is not marked, or still offers the button")
	60	}
	61	}
	62
	63	// The thread body renders without the raw block the diff stands in for.
	64	func TestAttachThreadsStripsSuggestionBlock(t *testing.T) {
	65	md := func(src, _ string) template.HTML { return template.HTML(src) }
	66	cm := store.DiffComment{ID: 3, Author: "bob", HeadSHA: "h", Path: "a.go", Side: "new", Line: 2,
	67	Body: "try this\n```suggestion\nx\n```\n"}
	68	view := &suggestionView{}
	69	_, detached := attachThreads(nil, []store.DiffComment{cm}, "h", md, reviewRights{},
	70	map[int64]*suggestionView{3: view})
	71	if len(detached) != 1 \|\| detached[0].Suggestion != view \|\| string(detached[0].Comments[0].BodyHTML) != "try this" {
	72	t.Fatalf("thread = %+v", detached)
	73	}
	74	}

internal/httpd/web.go +61 −4

	@@ -39,6 +39,7 @@ import (
39	"gitbay.org/gitbay/internal/gitutil"	39	"gitbay.org/gitbay/internal/gitutil"
40	"gitbay.org/gitbay/internal/sig"	40	"gitbay.org/gitbay/internal/sig"
41	"gitbay.org/gitbay/internal/store"	41	"gitbay.org/gitbay/internal/store"
		42	"gitbay.org/gitbay/internal/suggest"
42	"gitbay.org/gitbay/internal/web"	43	"gitbay.org/gitbay/internal/web"
43	)	44	)
44		45
	@@ -1494,6 +1495,38 @@ type diffThread struct {
1494	Pending bool	1495	Pending bool
1495	CanResolve bool	1496	CanResolve bool
1496	Comments []renderedComment	1497	Comments []renderedComment
		1498	Suggestion *suggestionView
		1499	}
		1500
		1501	// suggestionView is a thread's suggestion as the page shows it: the lines
		1502	// it replaces and the ones it proposes, numbered from Start, and whether
		1503	// the viewer can apply it here or needs the CLI.
		1504	type suggestionView struct {
		1505	Start int64
		1506	Old, New []suggestionLine
		1507	Outdated bool
		1508	Reason string
		1509	Local bool // the repositories require signed commits: apply from a clone
		1510	CanApply bool // the viewer can push to the source branch of an open MR
		1511	Command string // the CLI command that applies it
		1512	}
		1513
		1514	type suggestionLine struct {
		1515	N int64
		1516	Text string
		1517	}
		1518
		1519	// newSuggestionView lays out s for the page.
		1520	func newSuggestionView(s control.SuggestionOut, canApply bool, command string) suggestionView {
		1521	v := &suggestionView{Start: s.StartLine, Outdated: s.Outdated, Reason: s.Reason,
		1522	Local: s.Apply == "local", CanApply: canApply, Command: command}
		1523	for i, l := range suggest.FromText(strings.ReplaceAll(s.Original, "\r\n", "\n")) {
		1524	v.Old = append(v.Old, suggestionLine{s.StartLine + int64(i), l})
		1525	}
		1526	for i, l := range suggest.FromText(s.Replacement) {
		1527	v.New = append(v.New, suggestionLine{s.StartLine + int64(i), l})
		1528	}
		1529	return v
1497	}	1530	}
1498		1531
1499	// reviewRights decides which thread controls a viewer sees. mr resolve	1532	// reviewRights decides which thread controls a viewer sees. mr resolve
	@@ -1511,8 +1544,10 @@ func (r reviewRights) canResolve(threadAuthor string) bool {
1511		1544
1512	// attachThreads injects review threads under their anchored diff lines;	1545	// attachThreads injects review threads under their anchored diff lines;
1513	// threads whose anchor no longer appears (stale after force-push, or on a	1546	// threads whose anchor no longer appears (stale after force-push, or on a
1514	// context line outside the current diff) are returned separately.	1547	// context line outside the current diff) are returned separately. A
1515	func attachThreads(files []diffFile, comments []store.DiffComment, headSHA string, md ugcRenderer, rights reviewRights) ([]diffFile, []diffThread) {	1548	// thread root in suggestions renders its suggestion as a diff, and its
		1549	// body without the block.
		1550	func attachThreads(files []diffFile, comments []store.DiffComment, headSHA string, md ugcRenderer, rights reviewRights, suggestions map[int64]*suggestionView) ([]diffFile, []diffThread) {
1516	type anchor struct {	1551	type anchor struct {
1517	path string	1552	path string
1518	side string	1553	side string
	@@ -1525,10 +1560,15 @@ func attachThreads(files []diffFile, comments []store.DiffComment, headSHA strin
1525	var order []int64	1560	var order []int64
1526	for _, cm := range comments {	1561	for _, cm := range comments {
1527	if cm.ReplyTo == 0 {	1562	if cm.ReplyTo == 0 {
		1563	body := cm.Body
		1564	if suggestions[cm.ID] != nil {
		1565	body = suggest.Strip(body)
		1566	}
1528	threads[cm.ID] = &diffThread{ID: cm.ID, Resolved: cm.ResolvedBy, Stale: cm.HeadSHA != headSHA,	1567	threads[cm.ID] = &diffThread{ID: cm.ID, Resolved: cm.ResolvedBy, Stale: cm.HeadSHA != headSHA,
1529	Pending: cm.Pending,	1568	Pending: cm.Pending,
1530	CanResolve: rights.canResolve(cm.Author),	1569	CanResolve: rights.canResolve(cm.Author),
1531	Comments: []renderedComment{{Author: cm.Author, CreatedAt: cm.CreatedAt, BodyHTML: md(cm.Body, "md")}}}	1570	Suggestion: suggestions[cm.ID],
		1571	Comments: []renderedComment{{Author: cm.Author, CreatedAt: cm.CreatedAt, BodyHTML: md(body, "md")}}}
1532	anchors[cm.ID] = anchor{cm.Path, cm.Side, cm.Line}	1572	anchors[cm.ID] = anchor{cm.Path, cm.Side, cm.Line}
1533	order = append(order, cm.ID)	1573	order = append(order, cm.ID)
1534	} else if th, ok := threads[cm.ReplyTo]; ok {	1574	} else if th, ok := threads[cm.ReplyTo]; ok {
	@@ -2186,9 +2226,26 @@ func (s Server) mrPage(w http.ResponseWriter, r http.Request, previewForm stri
2186	}	2226	}
2187	md := s.ugcFor(r, p.Repo)	2227	md := s.ugcFor(r, p.Repo)
2188	canWrite := s.canWriteRepo(r, p.Repo)	2228	canWrite := s.canWriteRepo(r, p.Repo)
		2229	// Applying a suggestion pushes to the source branch, so the button
		2230	// follows write on the source repository, which for a fork is not
		2231	// the one this page is in.
		2232	canApply := false
		2233	if p.Viewer != "" && m.State == "open" {
		2234	if src, err := s.st.RepoByID(m.SourceRepoID); err == nil {
		2235	canApply = s.canWriteRepo(r, src)
		2236	}
		2237	}
		2238	suggestions := map[int64]*suggestionView{}
		2239	sgs := control.Suggestions(s.st, s.cfg.Server.Root, p.Repo, m, diffComments)
		2240	for _, cm := range diffComments {
		2241	if sg := sgs[cm.ID]; sg != nil {
		2242	suggestions[cm.ID] = newSuggestionView(sg, canApply && !cm.Pending,
		2243	fmt.Sprintf("gitbay mr apply-suggestion %s %d %d", p.Repo.Path(), m.Number, cm.ID))
		2244	}
		2245	}
2189	var detachedThreads []diffThread	2246	var detachedThreads []diffThread
2190	files, detachedThreads = attachThreads(files, diffComments, m.HeadSHA, md,	2247	files, detachedThreads = attachThreads(files, diffComments, m.HeadSHA, md,
2191	reviewRights{Viewer: p.Viewer, MRAuthor: m.Author, Write: canWrite})	2248	reviewRights{Viewer: p.Viewer, MRAuthor: m.Author, Write: canWrite}, suggestions)
2192	if p.Viewer != "" {	2249	if p.Viewer != "" {
2193	markCompose(files, r.URL.Query())	2250	markCompose(files, r.URL.Query())
2194	}	2251	}

internal/store/diffcomments.go +36 −12

 	Path       string
 	Side       string
 	Line       int64
+	StartLine  int64 // first line of a range ending at Line; 0 for Line alone
 	Body       string
 	ReplyTo    int64 // 0 for thread roots
 	ResolvedBy string
 }
 // AddDiffComment creates a thread root (replyTo 0) or a reply. Replies
-// inherit the root's anchor and must belong to the same MR.
+// inherit the root's anchor and must belong to the same MR. startLine is
-func (s *Store) AddDiffComment(mrID, authorID int64, headSHA, path, side string, line int64, body string, replyTo int64, pending bool) (int64, error) {
+// the first line of a range ending at line, or 0.
+func (s *Store) AddDiffComment(mrID, authorID int64, headSHA, path, side string, line, startLine int64, body string, replyTo int64, pending bool) (int64, error) {
 	if replyTo != 0 {
 		var rootMR int64
 		var rootReply sql.NullInt64
 			return 0, fmt.Errorf("reply to the thread root %d, not to a reply", rootReply.Int64)
 		}
 		err = s.DB.QueryRow(
-			"SELECT head_sha, path, side, line FROM mr_diff_comments WHERE id = ?", replyTo).
+			"SELECT head_sha, path, side, line, start_line FROM mr_diff_comments WHERE id = ?", replyTo).
-			Scan(&headSHA, &path, &side, &line)
+			Scan(&headSHA, &path, &side, &line, &startLine)
 		if err != nil {
 			return 0, err
 		}
 		reply = replyTo
 	}
 	res, err := s.DB.Exec(`
-		INSERT INTO mr_diff_comments (mr_id, author_id, head_sha, path, side, line, body, reply_to, pending)
+		INSERT INTO mr_diff_comments (mr_id, author_id, head_sha, path, side, line, start_line, body, reply_to, pending)
-		VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`,
+		VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`,
-		mrID, authorID, headSHA, path, side, line, body, reply, pending)
+		mrID, authorID, headSHA, path, side, line, startLine, body, reply, pending)
 	if err != nil {
 		return 0, err
 	}
 // anonymous reader, who sees only what is published.
 func (s *Store) ListDiffComments(mrID, viewer int64) ([]DiffComment, error) {
 	rows, err := s.DB.Query(`
-		SELECT c.id, u.username, c.head_sha, c.path, c.side, c.line, c.body,
+		SELECT `+diffCommentCols+`
-		       COALESCE(c.reply_to, 0), COALESCE(r.username, ''), c.pending, c.created_at
 		FROM mr_diff_comments c
 		JOIN users u ON u.id = c.author_id
 		LEFT JOIN users r ON r.id = c.resolved_by
 	defer rows.Close()
 	var out []DiffComment
 	for rows.Next() {
-		var c DiffComment
+		c, err := scanDiffComment(rows)
-		if err := rows.Scan(&c.ID, &c.Author, &c.HeadSHA, &c.Path, &c.Side, &c.Line, &c.Body,
+		if err != nil {
-			&c.ReplyTo, &c.ResolvedBy, &c.Pending, &c.CreatedAt); err != nil {
 			return nil, err
 		}
 		out = append(out, c)
 	return out, rows.Err()
 }
+const diffCommentCols = `c.id, u.username, c.head_sha, c.path, c.side, c.line, c.start_line, c.body,
+		       COALESCE(c.reply_to, 0), COALESCE(r.username, ''), c.pending, c.created_at`
+func scanDiffComment(row interface{ Scan(...any) error }) (DiffComment, error) {
+	var c DiffComment
+	err := row.Scan(&c.ID, &c.Author, &c.HeadSHA, &c.Path, &c.Side, &c.Line, &c.StartLine, &c.Body,
+		&c.ReplyTo, &c.ResolvedBy, &c.Pending, &c.CreatedAt)
+	return c, err
+}
+// DiffCommentByID returns one comment on an MR, published or pending.
+func (s *Store) DiffCommentByID(mrID, id int64) (DiffComment, error) {
+	c, err := scanDiffComment(s.DB.QueryRow(`
+		SELECT `+diffCommentCols+`
+		FROM mr_diff_comments c
+		JOIN users u ON u.id = c.author_id
+		LEFT JOIN users r ON r.id = c.resolved_by
+		WHERE c.mr_id = ? AND c.id = ?`, mrID, id))
+	if errors.Is(err, sql.ErrNoRows) {
+		return c, ErrNotFound
+	}
+	return c, err
+}
 // SetThreadResolved resolves or unresolves a thread root.
 func (s *Store) SetThreadResolved(mrID, rootID, byUser int64, resolved bool) error {
 	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	if err := s.AddMRSystemComment(mr1.ID, uid, "merged"); err != nil {	208	if err := s.AddMRSystemComment(mr1.ID, uid, "merged"); err != nil {
209	t.Fatal(err)	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	if err != nil {	212	if err != nil {
213	t.Fatal(err)	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	t.Fatal(err)	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	t.Fatal(err)	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	// else, until they submit.	35	// else, until they submit.
36	func TestPendingCommentsArePrivate(t *testing.T) {	36	func TestPendingCommentsArePrivate(t *testing.T) {
37	s, mrID, author, other := pendingFixture(t)	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	t.Fatal(err)	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	t.Fatal(err)	42	t.Fatal(err)
43	}	43	}
44		44
	@@ -66,7 +66,7 @@ func TestPendingCommentsArePrivate(t *testing.T) {
66	// so nobody else could resolve it.	66	// so nobody else could resolve it.
67	func TestPendingThreadsDoNotBlockMerges(t *testing.T) {	67	func TestPendingThreadsDoNotBlockMerges(t *testing.T) {
68	s, mrID, _, other := pendingFixture(t)	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	t.Fatal(err)	70	t.Fatal(err)
71	}	71	}
72	n, err := s.UnresolvedThreadCount(mrID)	72	n, err := s.UnresolvedThreadCount(mrID)
	@@ -88,12 +88,12 @@ func TestPendingThreadsDoNotBlockMerges(t *testing.T) {
88	func TestPublishAndDiscardPending(t *testing.T) {	88	func TestPublishAndDiscardPending(t *testing.T) {
89	s, mrID, author, other := pendingFixture(t)	89	s, mrID, author, other := pendingFixture(t)
90	for i := 0; i < 3; i++ {	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	t.Fatal(err)	92	t.Fatal(err)
93	}	93	}
94	}	94	}
95	// Another reviewer's batch is untouched by either operation.	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	t.Fatal(err)	97	t.Fatal(err)
98	}	98	}
99		99

internal/suggest/suggest.go added +176

@@ -0,0 +1,176 @@
	1	// Package suggest reads the replacement lines a review comment proposes
	2	// in a fenced suggestion block, and applies them to a file's anchored
	3	// line range. The server's apply and the CLI's local apply share it, so
	4	// both produce the same bytes.
	5	package suggest
	6
	7	import (
	8	"bytes"
	9	"errors"
	10	"fmt"
	11	"strings"
	12	)
	13
	14	// Parse returns the lines of the one ```suggestion block in body. found
	15	// is false when there is none. An empty block proposes deleting the
	16	// range; a block holding one empty line proposes a blank line.
	17	func Parse(body string) (lines []string, found bool, err error) {
	18	start, end, err := locate(body)
	19	if err != nil \|\| start < 0 {
	20	return nil, false, err
	21	}
	22	all := strings.Split(normalize(body), "\n")
	23	return append([]string{}, all[start+1:end]...), true, nil
	24	}
	25
	26	// Strip returns body without its suggestion block, for rendering the
	27	// prose around a suggestion that is shown as a diff instead.
	28	func Strip(body string) string {
	29	start, end, err := locate(body)
	30	if err != nil \|\| start < 0 {
	31	return body
	32	}
	33	all := strings.Split(normalize(body), "\n")
	34	return strings.TrimSpace(strings.Join(append(all[:start:start], all[end+1:]...), "\n"))
	35	}
	36
	37	func normalize(body string) string { return strings.ReplaceAll(body, "\r\n", "\n") }
	38
	39	// locate finds the suggestion block's opening and closing fence lines.
	40	// start is -1 when there is no block.
	41	func locate(body string) (start, end int, err error) {
	42	all := strings.Split(normalize(body), "\n")
	43	start = -1
	44	for i := 0; i < len(all); i++ {
	45	fence, ok := opening(all[i])
	46	if !ok {
	47	continue
	48	}
	49	if start >= 0 {
	50	return -1, -1, errors.New("a comment carries one suggestion block")
	51	}
	52	j := i + 1
	53	for ; j < len(all); j++ {
	54	if closing(all[j], fence) {
	55	break
	56	}
	57	}
	58	if j == len(all) {
	59	return -1, -1, errors.New("the suggestion block is not closed")
	60	}
	61	start, end = i, j
	62	i = j
	63	}
	64	return start, end, nil
	65	}
	66
	67	// opening reports whether line opens a suggestion block, and the length
	68	// of its backtick fence.
	69	func opening(line string) (int, bool) {
	70	t := strings.TrimLeft(line, " ")
	71	if len(line)-len(t) > 3 {
	72	return 0, false
	73	}
	74	n := len(t) - len(strings.TrimLeft(t, "`"))
	75	if n < 3 {
	76	return 0, false
	77	}
	78	return n, strings.TrimSpace(t[n:]) == "suggestion"
	79	}
	80
	81	func closing(line string, fence int) bool {
	82	t := strings.TrimSpace(line)
	83	return len(t) >= fence && strings.Trim(t, "`") == ""
	84	}
	85
	86	// Text is the replacement as one string, every line ending in a newline:
	87	// "" deletes the range and "\n" is one blank line.
	88	func Text(lines []string) string {
	89	var b strings.Builder
	90	for _, l := range lines {
	91	b.WriteString(l)
	92	b.WriteByte('\n')
	93	}
	94	return b.String()
	95	}
	96
	97	// FromText undoes Text.
	98	func FromText(s string) []string {
	99	if s == "" {
	100	return nil
	101	}
	102	return strings.Split(strings.TrimSuffix(s, "\n"), "\n")
	103	}
	104
	105	// split cuts content into lines, each keeping its terminator. The last
	106	// line has none when the file does not end in a newline.
	107	func split(content []byte) [][]byte {
	108	var out [][]byte
	109	for len(content) > 0 {
	110	i := bytes.IndexByte(content, '\n')
	111	if i < 0 {
	112	out = append(out, content)
	113	break
	114	}
	115	out = append(out, content[:i+1])
	116	content = content[i+1:]
	117	}
	118	return out
	119	}
	120
	121	// Range returns lines start through end (1-based, inclusive) of content
	122	// with their terminators, and false when the file is shorter than that.
	123	func Range(content []byte, start, end int) ([]byte, bool) {
	124	lines := split(content)
	125	if start < 1 \|\| end < start \|\| end > len(lines) {
	126	return nil, false
	127	}
	128	return bytes.Join(lines[start-1:end], nil), true
	129	}
	130
	131	// Apply replaces lines start through end of content with repl. The
	132	// replacement takes the line ending the file uses there, CRLF or LF, and
	133	// the last replacement line keeps whatever ended the range, so a range
	134	// at the end of a file with no final newline still has none.
	135	func Apply(content []byte, start, end int, repl []string) ([]byte, error) {
	136	lines := split(content)
	137	if start < 1 \|\| end < start {
	138	return nil, fmt.Errorf("bad line range %d-%d", start, end)
	139	}
	140	if end > len(lines) {
	141	return nil, fmt.Errorf("the file has %d lines; the suggestion ends at line %d", len(lines), end)
	142	}
	143	last := lines[end-1]
	144	eol, lastEOL := []byte("\n"), []byte{}
	145	switch {
	146	case bytes.HasSuffix(last, []byte("\r\n")):
	147	eol, lastEOL = []byte("\r\n"), []byte("\r\n")
	148	case bytes.HasSuffix(last, []byte("\n")):
	149	lastEOL = []byte("\n")
	150	case end > 1 && bytes.HasSuffix(lines[end-2], []byte("\r\n")):
	151	eol = []byte("\r\n")
	152	}
	153	var b bytes.Buffer
	154	for _, l := range lines[:start-1] {
	155	b.Write(l)
	156	}
	157	for i, r := range repl {
	158	b.WriteString(r)
	159	if i == len(repl)-1 {
	160	b.Write(lastEOL)
	161	} else {
	162	b.Write(eol)
	163	}
	164	}
	165	for _, l := range lines[end:] {
	166	b.Write(l)
	167	}
	168	return b.Bytes(), nil
	169	}
	170
	171	// Message is the commit message of an applied suggestion, naming the
	172	// merge request and the thread. The server and the CLI's local apply
	173	// both write it.
	174	func Message(repoPath string, mr, thread int64, author string) string {
	175	return fmt.Sprintf("Apply suggestion from %s\n\nThread %d on %s!%d.\n", author, thread, repoPath, mr)
	176	}

internal/suggest/suggest_test.go added +100

@@ -0,0 +1,100 @@
	1	package suggest
	2
	3	import (
	4	"reflect"
	5	"strings"
	6	"testing"
	7	)
	8
	9	func TestParse(t *testing.T) {
	10	cases := []struct {
	11	name string
	12	body string
	13	lines []string
	14	found bool
	15	err string
	16	}{
	17	{"none", "just prose\n```go\nx\n```\n", nil, false, ""},
	18	{"one line", "try this\n```suggestion\nreturn nil\n```\n", []string{"return nil"}, true, ""},
	19	{"several", "```suggestion\na\n\tb\n```", []string{"a", "\tb"}, true, ""},
	20	{"deletion", "drop it\n```suggestion\n```\n", []string{}, true, ""},
	21	{"blank line", "```suggestion\n\n```\n", []string{""}, true, ""},
	22	{"crlf body", "x\r\n```suggestion\r\nnew\r\n```\r\n", []string{"new"}, true, ""},
	23	{"longer fence", "````suggestion\n```\n````\n", []string{"```"}, true, ""},
	24	{"unclosed", "```suggestion\nnew\n", nil, false, "not closed"},
	25	{"two", "```suggestion\na\n```\n```suggestion\nb\n```\n", nil, false, "one suggestion"},
	26	}
	27	for _, c := range cases {
	28	lines, found, err := Parse(c.body)
	29	if c.err != "" {
	30	if err == nil \|\| !strings.Contains(err.Error(), c.err) {
	31	t.Errorf("%s: err = %v, want %q", c.name, err, c.err)
	32	}
	33	continue
	34	}
	35	if err != nil \|\| found != c.found \|\| (c.found && !reflect.DeepEqual(lines, c.lines)) {
	36	t.Errorf("%s: Parse = %q %v %v, want %q %v", c.name, lines, found, err, c.lines, c.found)
	37	}
	38	}
	39	}
	40
	41	func TestStrip(t *testing.T) {
	42	if got := Strip("use this\n```suggestion\nx\n```\nthanks"); got != "use this\nthanks" {
	43	t.Errorf("Strip = %q", got)
	44	}
	45	if got := Strip("```suggestion\nx\n```"); got != "" {
	46	t.Errorf("Strip of a bare block = %q", got)
	47	}
	48	}
	49
	50	func TestTextRoundTrip(t *testing.T) {
	51	for _, lines := range [][]string{nil, {""}, {"a"}, {"a", "", "b"}} {
	52	if got := FromText(Text(lines)); !reflect.DeepEqual(got, lines) && !(len(got) == 0 && len(lines) == 0) {
	53	t.Errorf("FromText(Text(%q)) = %q", lines, got)
	54	}
	55	}
	56	}
	57
	58	func TestApply(t *testing.T) {
	59	cases := []struct {
	60	name string
	61	content string
	62	start, end int
	63	repl []string
	64	want string
	65	err string
	66	}{
	67	{"one line", "a\nb\nc\n", 2, 2, []string{"B"}, "a\nB\nc\n", ""},
	68	{"range to more", "a\nb\nc\nd\n", 2, 3, []string{"x", "y", "z"}, "a\nx\ny\nz\nd\n", ""},
	69	{"range to fewer", "a\nb\nc\nd\n", 1, 3, []string{"x"}, "x\nd\n", ""},
	70	{"deletion", "a\nb\nc\n", 2, 2, nil, "a\nc\n", ""},
	71	{"delete all", "a\nb\n", 1, 2, nil, "", ""},
	72	{"eof no newline", "a\nb", 2, 2, []string{"B", "C"}, "a\nB\nC", ""},
	73	{"eof with newline", "a\nb\n", 2, 2, []string{"B"}, "a\nB\n", ""},
	74	{"crlf", "a\r\nb\r\nc\r\n", 2, 2, []string{"x", "y"}, "a\r\nx\r\ny\r\nc\r\n", ""},
	75	{"crlf eof no newline", "a\r\nb", 2, 2, []string{"x", "y"}, "a\r\nx\r\ny", ""},
	76	{"past eof", "a\nb\n", 2, 3, []string{"x"}, "", "has 2 lines"},
	77	{"bad range", "a\n", 2, 1, nil, "", "bad line range"},
	78	}
	79	for _, c := range cases {
	80	got, err := Apply([]byte(c.content), c.start, c.end, c.repl)
	81	if c.err != "" {
	82	if err == nil \|\| !strings.Contains(err.Error(), c.err) {
	83	t.Errorf("%s: err = %v, want %q", c.name, err, c.err)
	84	}
	85	continue
	86	}
	87	if err != nil \|\| string(got) != c.want {
	88	t.Errorf("%s: Apply = %q, %v; want %q", c.name, got, err, c.want)
	89	}
	90	}
	91	}
	92
	93	func TestRange(t *testing.T) {
	94	if got, ok := Range([]byte("a\r\nb\nc"), 2, 3); !ok \|\| string(got) != "b\nc" {
	95	t.Errorf("Range = %q %v", got, ok)
	96	}
	97	if _, ok := Range([]byte("a\n"), 1, 2); ok {
	98	t.Error("Range past the end reported ok")
	99	}
	100	}

internal/web/static/style.css +4

 .thread textarea { width: 100%; }
 .thread.composing p, .thread details.threadreply p { margin: var(--sp-2) 0 0; }
 form.threadact { margin: var(--sp-1) 0 0; padding: 0 var(--sp-3) var(--sp-2); }
+.thread .suggestion { margin: var(--sp-2) 0 0; }
+.thread .suggestion table.difftable { margin: var(--sp-1) 0 0; }
+.thread .suggestion table.difftable tr.add td,
+.thread .suggestion table.difftable tr.del td { display: table-cell; }
 nav.subtabs {
   display: flex;

internal/web/templates/layout.html +16 −1

     <input type="hidden" name="path" value="{{.Path}}">
     <input type="hidden" name="line" value="{{if eq .Class "del"}}{{.OldLine}}{{else}}{{.NewLine}}{{end}}">
     <input type="hidden" name="side" value="{{if eq .Class "del"}}old{{else}}new{{end}}">
+    {{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}}
     <p><textarea name="body" aria-label="Comment on {{.Path}}" rows="3" placeholder="Comment on this line" autofocus></textarea></p>
     <p><button type="submit" class="btn">Comment</button>
        <button type="submit" name="pending" value="on" class="btn">Add to review</button>
     <input type="hidden" name="path" value="{{.Path}}">
     <input type="hidden" name="line" value="{{if eq .Class "del"}}{{.OldLine}}{{else}}{{.NewLine}}{{end}}">
     <input type="hidden" name="side" value="{{if eq .Class "del"}}old{{else}}new{{end}}">
+    {{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}}
     <p><textarea name="body" aria-label="Comment on {{.Path}}" rows="3" placeholder="Comment on this line" autofocus></textarea></p>
     <p><button type="submit" class="btn">Comment</button>
        <button type="submit" name="pending" value="on" class="btn">Add to review</button>
 {{define "thread"}}{{$t := .T}}<div class="thread{{if $t.Resolved}} resolved{{end}}{{if $t.Pending}} pending{{end}}{{if .Class}} {{.Class}}{{end}}" id="thread-{{$t.ID}}">
 {{if $t.Pending}}<p class="threadstate">pending — only you can see this until you submit your review</p>{{end}}
 {{if or $t.Resolved (and .Class $t.Stale)}}<p class="threadstate">{{if and .Class $t.Stale}}stale{{end}}{{if $t.Resolved}}{{if and .Class $t.Stale}} · {{end}}resolved by {{$t.Resolved}}{{end}}</p>{{end}}
-{{range $t.Comments}}<p class="commenthead"><strong>{{.Author}}</strong> <span class="when">{{when .CreatedAt}}</span></p><div class="rendered">{{.BodyHTML}}</div>{{end}}
+{{range $i, $c := $t.Comments}}<p class="commenthead"><strong>{{.Author}}</strong> <span class="when">{{when .CreatedAt}}</span></p><div class="rendered">{{.BodyHTML}}</div>{{if and (eq $i 0) $t.Suggestion}}{{template "suggestion" dict "S" $t.Suggestion "T" $t.ID "Base" $.Base}}{{end}}{{end}}
 {{if .Viewer}}<details class="threadreply"><summary>Reply</summary>
 <form method="post" action="{{.Base}}/diff-comment">
   <input type="hidden" name="reply" value="{{$t.ID}}">
 </form></details>{{end}}
 {{if $t.CanResolve}}<form method="post" action="{{.Base}}/thread" class="threadact"><input type="hidden" name="thread" value="{{$t.ID}}"><button type="submit" name="action" value="{{if $t.Resolved}}unresolve{{else}}resolve{{end}}" class="linklike">{{if $t.Resolved}}Reopen thread{{else}}Resolve thread{{end}}</button></form>{{end}}
 </div>{{end}}
+{{/* suggestion renders the lines a thread's suggestion replaces and the
+     ones it proposes, with the apply button, or the command to run where
+     the server cannot sign the commit. */}}
+{{define "suggestion"}}{{$s := .S}}<div class="suggestion">
+<p class="threadstate">{{if $s.Outdated}}outdated suggestion: {{$s.Reason}}{{else}}suggested change{{end}}</p>
+<div class="tablewrap"><table class="difftable">
+{{range $s.Old}}<tr class="del"><td class="ln">{{.N}}</td><td class="src">{{.Text}}</td></tr>
+{{end}}{{range $s.New}}<tr class="add"><td class="ln">{{.N}}</td><td class="src">{{.Text}}</td></tr>
+{{end}}</table></div>
+{{if and $s.CanApply (not $s.Outdated)}}{{if $s.Local}}<p class="threadstate">This repository requires signed commits, which the server cannot make. Apply it from a clone: <code>{{$s.Command}}</code></p>
+{{else}}<form method="post" action="{{.Base}}/suggestion" class="threadact"><input type="hidden" name="thread" value="{{.T}}"><button type="submit" class="btn">Apply suggestion</button></form>
+{{end}}{{end}}</div>{{end}}