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

		@@ -39,6 +39,8 @@ browser-only and the iOS build screen unable to say more than the log.
39	39	\| review (approve etc.) \| yes \| yes \| yes \|
40	40	\| resolve a thread \| yes \| yes \| yes \|
41	41	\| comment on a diff line \| yes \| yes \| yes \|
	42	\| suggest a change \| yes \| yes \| no \|
	43	\| apply a suggestion \| yes \| yes \| no \|
42	44	\| merge (all strategies) \| yes \| yes \| yes \|
43	45	\| merge when ready, cancel \| yes \| yes \| no \|
44	46	\| close \| yes \| yes \| yes \|
		@@ -480,6 +482,12 @@ now a page waiting to be built rather than a rule. A credential still
480	482	travels on stdin wherever it is set, since argv is world-readable in
481	483	/proc and the audit log keeps flag values.
482	484
	485	Applying a suggestion on a repository that requires signed commits is
	486	CLI-only by mechanism: the server has no key to sign the commit with,
	487	so =gitbay mr apply-suggestion= makes and signs it in a clone with the
	488	user's own git signing configuration and pushes it. The web shows that
	489	command in place of the button.
	490
483	491	Deleting or transferring a repository stays CLI-only on purpose, as
484	492	does deleting an organization and pruning merge request heads (=admin
485	493	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	566	anchors; =mr show= reports the unresolved count. Resolving is for the
567	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	611	* CI builds
570	612
571	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	12	layout puts old and new side by side, each column's line numbers
13	13	comment on their own side, and a narrow window stacks the rows as a
14	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	30	* v1.38.0 — 2026-09-29
17	31

cmd/gitbay/local.go +259

		@@ -6,12 +6,15 @@ import (
6	6	"os"
7	7	"os/exec"
8	8	"path/filepath"
	9	"strconv"
9	10	"strings"
10	11
	12	"github.com/spf13/cobra"
11	13	"golang.org/x/term"
12	14
13	15	"gitbay.org/gitbay/internal/cliconfig"
14	16	"gitbay.org/gitbay/internal/protocol"
	17	"gitbay.org/gitbay/internal/suggest"
15	18	"gitbay.org/gitbay/internal/toolpath"
16	19	)
17	20
		@@ -413,3 +416,259 @@ func worktreeDirty() (bool, error) {
413	416	}
414	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	683	pass("diff-comment", passOpts{server: []string{"mr", "diff-comment"}, needsRepo: true, stdinOK: true, editor: "comment"}),
684	684	pass("threads", passOpts{server: []string{"mr", "threads"}, needsRepo: true}),
685	685	pass("resolve", passOpts{server: []string{"mr", "resolve"}, needsRepo: true}),
	686	mrApplySuggestionCmd(),
686	687	pass("unresolve", passOpts{server: []string{"mr", "unresolve"}, needsRepo: true}),
687	688	review,
688	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	64	"milestone create": "create a milestone",
65	65	"milestone list": "list milestones with progress",
66	66	"milestone reopen": "reopen a milestone",
	67	"mr apply-suggestion": "commit a review thread's suggestion to the source branch",
67	68	"mr close": "close without merging",
68	69	"mr comment": "add a comment",
69	70	"mr create": "open a merge request",

e2e/cli_test.go +2

		@@ -16,6 +16,7 @@ type cli struct {
16	16	configDir string
17	17	inst *instance
18	18	key string
	19	env []string // appended last, so it overrides the defaults
19	20	}
20	21
21	22	func (c cli) run(t testing.T, dir, stdin string, args ...string) (string, string, int) {
		@@ -29,6 +30,7 @@ func (c cli) run(t testing.T, dir, stdin string, args ...string) (string, stri
29	30	"GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@example.test",
30	31	"EDITOR=", // no editor in tests: bodies come from flags
31	32	)
	33	cmd.Env = append(cmd.Env, c.env...)
32	34	if stdin != "" {
33	35	cmd.Stdin = strings.NewReader(stdin)
34	36	}

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

		@@ -72,6 +72,9 @@ func runCommitFile(c *Ctx, args []string) int {
72	72	"%s requires signed commits; this writes an unsigned one — push a signed commit instead",
73	73	repo.Path())
74	74	}
	75	if code := checkStorageQuota(c, repo); code >= 0 {
	76	return code
	77	}
75	78	// A commit carries an identity, and an unverified address is not one.
76	79	email, err := c.Store.PrimaryVerifiedEmail(c.User.ID)
77	80	if err != nil {

internal/control/diffcomment.go +104 −27

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

internal/control/quota.go +19

		@@ -81,6 +81,25 @@ func checkRepoQuota(c *Ctx) int {
81	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	103	func init() {
85	104	register(Command{Path: []string{"admin", "user", "limits"},
86	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

		@@ -211,6 +211,21 @@ func CommitFileChange(dir, branch, path string, content []byte, name, email, mes
211	211	parent = ""
212	212	}
213	213
	214	sha, err := CommitWithFile(dir, parent, path, "100644", content, name, email, message)
	215	if err != nil {
	216	return "", err
	217	}
	218	if err := UpdateRefCAS(dir, branchRef, sha, parent); err != nil {
	219	return "", fmt.Errorf("branch moved during edit; reload and retry: %w", err)
	220	}
	221	return sha, nil
	222	}
	223
	224	// CommitWithFile writes a commit on parent ("" for a root commit) whose
	225	// tree is parent's with content at path, as a file of mode (100644 or
	226	// 100755), authored and committed as name <email>. No ref moves: the
	227	// caller updates one, and enforces policy, since no hook runs.
	228	func CommitWithFile(dir, parent, path, mode string, content []byte, name, email, message string) (string, error) {
214	229	// Hash the new blob.
215	230	hb := exec.Command(toolpath.Look("git"), "-C", dir, "hash-object", "-w", "--stdin")
216	231	hb.Stdin = strings.NewReader(string(content))
		@@ -239,7 +254,7 @@ func CommitFileChange(dir, branch, path string, content []byte, name, email, mes
239	254	if out, err := rt.CombinedOutput(); err != nil {
240	255	return "", fmt.Errorf("read-tree: %v\n%s", err, out)
241	256	}
242		ui := exec.Command(toolpath.Look("git"), "-C", dir, "update-index", "--add", "--cacheinfo", "100644,"+blob+","+path)
	257	ui := exec.Command(toolpath.Look("git"), "-C", dir, "update-index", "--add", "--cacheinfo", mode+","+blob+","+path)
243	258	ui.Env = env
244	259	if out, err := ui.CombinedOutput(); err != nil {
245	260	return "", fmt.Errorf("update-index: %v\n%s", err, out)
		@@ -256,14 +271,7 @@ func CommitFileChange(dir, branch, path string, content []byte, name, email, mes
256	271	if parent != "" {
257	272	parents = []string{parent}
258	273	}
259		sha, err := CommitTree(dir, tree, parents, name, email, message)
260		if err != nil {
261		return "", err
262		}
263		if err := UpdateRefCAS(dir, branchRef, sha, parent); err != nil {
264		return "", fmt.Errorf("branch moved during edit; reload and retry: %w", err)
265		}
266		return sha, nil
	274	return CommitTree(dir, tree, parents, name, email, message)
267	275	}
268	276
269	277	// isEmptyRepo reports whether dir has no refs at all — a repository

internal/hookd/hookd.go +3 −200

		@@ -15,12 +15,9 @@ import (
15	15	"log/slog"
16	16	"net"
17	17	"os"
18		"path"
19	18	"path/filepath"
20	19	"strings"
21		"time"
22	20
23		"gitbay.org/gitbay/internal/ci"
24	21	"gitbay.org/gitbay/internal/config"
25	22	"gitbay.org/gitbay/internal/control"
26	23	"gitbay.org/gitbay/internal/gitutil"
		@@ -275,204 +272,10 @@ func (s *Server) releaseAnchors(repo store.Repo, updates []policy.RefUpdate) str
275	272	return ""
276	273	}
277	274
278		// postReceive applies the cross-repo MR effect: a push to a source branch
279		// refreshes refs/merge-requests/N/head in every target repo, by fetching —
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.
	275	// postReceive runs the ref-update work for a push; see
	276	// control.RefsUpdated, which server-side writes to a branch share.
282	277	func (s *Server) postReceive(req Request) {
283		pushedRepo, pushedRepoErr := s.st.RepoByID(req.RepoID)
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
	278	control.RefsUpdated(s.st, s.cfg, req.RepoID, req.UserID, req.Scope, req.Updates)
476	279	}
477	280
478	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	354	f.git(f.src, "tag", "v1")
355	355	f.sync()
356	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	358	}, []string{"—", "—", "—", "—", "queued", "—"}},
359	359
360	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	140	return
141	141	}
142	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	150	if r.FormValue("side") == "old" {
144	151	extra = append(extra, "--old")
145	152	}
		@@ -179,6 +186,17 @@ func (s Server) mrThreadSubmit(w http.ResponseWriter, r http.Request, u store.
179	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	200	// mrNewPage is the create form: branches to choose from, plus whatever
183	201	// the last attempt had in it so a refusal does not lose the draft.
184	202	type mrNewPage struct {

internal/httpd/routes.go +2

		@@ -225,6 +225,8 @@ func (s *Server) Routes() []Route {
225	225	Handler: s.checkOrigin(s.requireUser(s.mrThreadSubmit))},
226	226	Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/diff-comment", Mutating: true,
227	227	Handler: s.checkOrigin(s.requireUser(s.mrDiffCommentSubmit))},
	228	Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/suggestion", Mutating: true,
	229	Handler: s.checkOrigin(s.requireUser(s.mrSuggestionSubmit))},
228	230	Route{Method: "GET", Pattern: "/{owner}/{repo}/edit/{ref}/{path...}",
229	231	Handler: s.requireUser(s.editForm)},
230	232	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	39	"gitbay.org/gitbay/internal/gitutil"
40	40	"gitbay.org/gitbay/internal/sig"
41	41	"gitbay.org/gitbay/internal/store"
	42	"gitbay.org/gitbay/internal/suggest"
42	43	"gitbay.org/gitbay/internal/web"
43	44	)
44	45
		@@ -1494,6 +1495,38 @@ type diffThread struct {
1494	1495	Pending bool
1495	1496	CanResolve bool
1496	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	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	1545	// attachThreads injects review threads under their anchored diff lines;
1513	1546	// threads whose anchor no longer appears (stale after force-push, or on a
1514		// context line outside the current diff) are returned separately.
1515		func attachThreads(files []diffFile, comments []store.DiffComment, headSHA string, md ugcRenderer, rights reviewRights) ([]diffFile, []diffThread) {
	1547	// context line outside the current diff) are returned separately. A
	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	1551	type anchor struct {
1517	1552	path string
1518	1553	side string
		@@ -1525,10 +1560,15 @@ func attachThreads(files []diffFile, comments []store.DiffComment, headSHA strin
1525	1560	var order []int64
1526	1561	for _, cm := range comments {
1527	1562	if cm.ReplyTo == 0 {
	1563	body := cm.Body
	1564	if suggestions[cm.ID] != nil {
	1565	body = suggest.Strip(body)
	1566	}
1528	1567	threads[cm.ID] = &diffThread{ID: cm.ID, Resolved: cm.ResolvedBy, Stale: cm.HeadSHA != headSHA,
1529	1568	Pending: cm.Pending,
1530	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	1572	anchors[cm.ID] = anchor{cm.Path, cm.Side, cm.Line}
1533	1573	order = append(order, cm.ID)
1534	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	2227	md := s.ugcFor(r, p.Repo)
2188	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	2246	var detachedThreads []diffThread
2190	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	2249	if p.Viewer != "" {
2193	2250	markCompose(files, r.URL.Query())
2194	2251	}

internal/store/diffcomments.go +36 −12

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

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

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

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

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

internal/store/mrs_test.go +3 −3

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

internal/store/pending_test.go +5 −5

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

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

		@@ -1293,6 +1293,10 @@ a.authorlink:hover { color: var(--link); }
1293	1293	.thread textarea { width: 100%; }
1294	1294	.thread.composing p, .thread details.threadreply p { margin: var(--sp-2) 0 0; }
1295	1295	form.threadact { margin: var(--sp-1) 0 0; padding: 0 var(--sp-3) var(--sp-2); }
	1296	.thread .suggestion { margin: var(--sp-2) 0 0; }
	1297	.thread .suggestion table.difftable { margin: var(--sp-1) 0 0; }
	1298	.thread .suggestion table.difftable tr.add td,
	1299	.thread .suggestion table.difftable tr.del td { display: table-cell; }
1296	1300
1297	1301	nav.subtabs {
1298	1302	display: flex;

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

		@@ -219,6 +219,7 @@
219	219	<input type="hidden" name="path" value="{{.Path}}">
220	220	<input type="hidden" name="line" value="{{if eq .Class "del"}}{{.OldLine}}{{else}}{{.NewLine}}{{end}}">
221	221	<input type="hidden" name="side" value="{{if eq .Class "del"}}old{{else}}new{{end}}">
	222	{{if ne .Class "del"}}<p><label>From line <input type="number" name="start_line" min="1" max="{{.NewLine}}" placeholder="{{.NewLine}}"></label> to {{.NewLine}}; a <code>```suggestion</code> block proposes replacement lines</p>{{end}}
222	223	<p><textarea name="body" aria-label="Comment on {{.Path}}" rows="3" placeholder="Comment on this line" autofocus></textarea></p>
223	224	<p><button type="submit" class="btn">Comment</button>
224	225	<button type="submit" name="pending" value="on" class="btn">Add to review</button>
		@@ -235,6 +236,7 @@
235	236	<input type="hidden" name="path" value="{{.Path}}">
236	237	<input type="hidden" name="line" value="{{if eq .Class "del"}}{{.OldLine}}{{else}}{{.NewLine}}{{end}}">
237	238	<input type="hidden" name="side" value="{{if eq .Class "del"}}old{{else}}new{{end}}">
	239	{{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}}
238	240	<p><textarea name="body" aria-label="Comment on {{.Path}}" rows="3" placeholder="Comment on this line" autofocus></textarea></p>
239	241	<p><button type="submit" class="btn">Comment</button>
240	242	<button type="submit" name="pending" value="on" class="btn">Add to review</button>
		@@ -255,7 +257,7 @@
255	257	{{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}}">
256	258	{{if $t.Pending}}<p class="threadstate">pending — only you can see this until you submit your review</p>{{end}}
257	259	{{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}}
258		{{range $t.Comments}}<p class="commenthead"><strong>{{.Author}}</strong> <span class="when">{{when .CreatedAt}}</span></p><div class="rendered">{{.BodyHTML}}</div>{{end}}
	260	{{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}}
259	261	{{if .Viewer}}<details class="threadreply"><summary>Reply</summary>
260	262	<form method="post" action="{{.Base}}/diff-comment">
261	263	<input type="hidden" name="reply" value="{{$t.ID}}">
		@@ -264,3 +266,16 @@
264	266	</form></details>{{end}}
265	267	{{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}}
266	268	</div>{{end}}
	269
	270	{{/* suggestion renders the lines a thread's suggestion replaces and the
	271	ones it proposes, with the apply button, or the command to run where
	272	the server cannot sign the commit. */}}
	273	{{define "suggestion"}}{{$s := .S}}<div class="suggestion">
	274	<p class="threadstate">{{if $s.Outdated}}outdated suggestion: {{$s.Reason}}{{else}}suggested change{{end}}</p>
	275	<div class="tablewrap"><table class="difftable">
	276	{{range $s.Old}}<tr class="del"><td class="ln">{{.N}}</td><td class="src">{{.Text}}</td></tr>
	277	{{end}}{{range $s.New}}<tr class="add"><td class="ln">{{.N}}</td><td class="src">{{.Text}}</td></tr>
	278	{{end}}</table></div>
	279	{{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>
	280	{{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>
	281	{{end}}{{end}}</div>{{end}}