krz/gitbay

A CLI-first git forge. cli forge git self-hosted https://gitbay.org

Commit f4f897ed47

f4f897ed47b2a49534b455feedf54ed0c4c703dd

parent: 0e460978da

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-28 21:29 UTC 

web: range-diff page for a merge request's revisions

Ref #269

Layout: unified · split

internal/httpd/mrrangediff.go added +55
@@ -0,0 +1,55 @@
1package httpd
2
3import (
4 "net/http"
5 "strconv"
6
7 "gitbay.org/gitbay/internal/protocol"
8 "gitbay.org/gitbay/internal/store"
9)
10
11// mrRangeDiff renders what changed between two revisions of a merge
12// request — the same comparison `mr range-diff` prints on the CLI and
13// the iOS app already show — so a reviewer whose approval a force-push
14// staled can see what moved without leaving the browser (#269).
15func (s *Server) mrRangeDiff(w http.ResponseWriter, r *http.Request) {
16 p, ok := s.repoFor(w, r, "")
17 if !ok {
18 return
19 }
20 p.Tab = "merge requests"
21 n, err := strconv.ParseInt(r.PathValue("n"), 10, 64)
22 if err != nil {
23 s.notFound(w, r)
24 return
25 }
26 m, err := s.st.MRByNumber(p.Repo.ID, n)
27 if err != nil {
28 s.notFound(w, r)
29 return
30 }
31
32 viewer := s.webViewer(r)
33 argv := mrArgs(r, "range-diff")
34 if from := r.URL.Query().Get("from"); from != "" {
35 argv = append(argv, "--from", from)
36 }
37 if to := r.URL.Query().Get("to"); to != "" {
38 argv = append(argv, "--to", to)
39 }
40 out, msg, code := s.runControlCode(viewer, argv)
41 if code == protocol.ExitNotFound {
42 s.notFound(w, r)
43 return
44 }
45 errMsg := ""
46 if code != protocol.ExitOK {
47 errMsg = msg
48 }
49 s.render(w, "mrrangediff.html", struct {
50 repoPage
51 MR store.MR
52 Diff string
53 Error string
54 }{p, m, out, errMsg})
55}
internal/httpd/mrrangediff_test.go added +346
@@ -0,0 +1,346 @@
1package httpd
2
3import (
4 "net/http"
5 "net/http/httptest"
6 "os"
7 "os/exec"
8 "path/filepath"
9 "strconv"
10 "strings"
11 "testing"
12 "time"
13
14 "gitbay.org/gitbay/internal/config"
15 "gitbay.org/gitbay/internal/control"
16 "gitbay.org/gitbay/internal/store"
17 "gitbay.org/gitbay/internal/web"
18)
19
20// The range-diff page dispatches mr range-diff and renders its text
21// output, the same comparison the CLI and iOS already show (#269).
22func TestMRRangeDiffPageRendersCommandOutput(t *testing.T) {
23 st, err := store.Open(":memory:")
24 if err != nil {
25 t.Fatal(err)
26 }
27 defer st.Close()
28 if err := st.MigrateUp(); err != nil {
29 t.Fatal(err)
30 }
31 uid, err := st.CreateUser("alice", false)
32 if err != nil {
33 t.Fatal(err)
34 }
35 if _, err := st.CreateRepo("user", uid, "app", "public"); err != nil {
36 t.Fatal(err)
37 }
38
39 s := New(config.Default(), st)
40 req := httptest.NewRequest("GET", "/alice/app/mrs/1/range-diff", nil)
41 req.SetPathValue("owner", "alice")
42 req.SetPathValue("repo", "app")
43 req.SetPathValue("n", "1")
44 rr := httptest.NewRecorder()
45 s.mrRangeDiff(rr, req)
46
47 // No merge request 1 exists yet, so this must 404 rather than error.
48 if rr.Code != 404 {
49 t.Fatalf("status %d, body %s", rr.Code, rr.Body.String())
50 }
51}
52
53// rangeDiffGitEnv and rangeDiffGitRunner build real git history on disk,
54// the way internal/control's own build tests do, since mr range-diff
55// runs actual git commands against the repository's bare directory — a
56// store-only fixture cannot exercise it.
57func rangeDiffGitEnv() []string {
58 return append(os.Environ(),
59 "GIT_CONFIG_NOSYSTEM=1", "GIT_CONFIG_GLOBAL=/dev/null",
60 "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@example.test",
61 "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@example.test")
62}
63
64func rangeDiffGitRunner(t *testing.T) func(dir string, args ...string) string {
65 t.Helper()
66 env := rangeDiffGitEnv()
67 return func(dir string, args ...string) string {
68 t.Helper()
69 cmd := exec.Command("git", args...)
70 cmd.Dir = dir
71 cmd.Env = env
72 out, err := cmd.CombinedOutput()
73 if err != nil {
74 t.Fatalf("git %v: %v\n%s", args, err, out)
75 }
76 return string(out)
77 }
78}
79
80// rangeDiffFixture builds a private repository owned by alice with a
81// merge request that has two real revisions on disk — an "add b" commit,
82// then the same commit amended with one changed line — the same shape
83// e2e/rangediff_test.go:14-38 builds for the CLI. bob holds no access to
84// the repository, so he stands in for a non-reader.
85func rangeDiffFixture(t *testing.T) (st *store.Store, cfg config.Config, alice, bob store.User, repo store.Repo, n int64, v1, v2, title string) {
86 t.Helper()
87 root := t.TempDir()
88 st, err := store.Open(filepath.Join(root, "gitbay.db"))
89 if err != nil {
90 t.Fatal(err)
91 }
92 t.Cleanup(func() { st.Close() })
93 if err := st.MigrateUp(); err != nil {
94 t.Fatal(err)
95 }
96 aliceID, err := st.CreateUser("alice", false)
97 if err != nil {
98 t.Fatal(err)
99 }
100 bobID, err := st.CreateUser("bob", false)
101 if err != nil {
102 t.Fatal(err)
103 }
104 repoID, err := st.CreateRepo("user", aliceID, "secret", "private")
105 if err != nil {
106 t.Fatal(err)
107 }
108 repo, err = st.RepoByID(repoID)
109 if err != nil {
110 t.Fatal(err)
111 }
112
113 git := rangeDiffGitRunner(t)
114 dir := control.RepoDir(root, repo.OwnerName, repo.Name)
115 if err := os.MkdirAll(filepath.Dir(dir), 0o755); err != nil {
116 t.Fatal(err)
117 }
118 git(root, "init", "-q", "--bare", dir)
119
120 src := filepath.Join(root, "src")
121 git(root, "init", "-q", "-b", "main", "src")
122 os.WriteFile(filepath.Join(src, "a.txt"), []byte("one\n"), 0o644)
123 git(src, "add", ".")
124 git(src, "commit", "-q", "-m", "base")
125 git(src, "push", "-q", dir, "main")
126
127 git(src, "checkout", "-q", "-b", "feat")
128 os.WriteFile(filepath.Join(src, "b.txt"), []byte("alpha\nbeta\ngamma\n"), 0o644)
129 git(src, "add", ".")
130 git(src, "commit", "-q", "-m", "add b")
131 git(src, "push", "-q", dir, "feat")
132 v1 = strings.TrimSpace(git(src, "rev-parse", "HEAD"))
133
134 os.WriteFile(filepath.Join(src, "b.txt"), []byte("alpha\nbeta revised\ngamma\n"), 0o644)
135 git(src, "add", ".")
136 git(src, "commit", "-q", "--amend", "--no-edit")
137 git(src, "push", "-q", "--force", dir, "feat")
138 v2 = strings.TrimSpace(git(src, "rev-parse", "HEAD"))
139
140 title = "range diff of a secret plan"
141 n, err = st.CreateMR(repo.ID, aliceID, repo.ID, "feat", "main", title, "", v1, "md", false)
142 if err != nil {
143 t.Fatal(err)
144 }
145 mr, err := st.MRByNumber(repo.ID, n)
146 if err != nil {
147 t.Fatal(err)
148 }
149 if err := st.UpdateMRHead(mr.ID, v2, "", false); err != nil {
150 t.Fatal(err)
151 }
152
153 cfg = config.Default()
154 cfg.Server.Root = root
155 cfg.Web.Mode = "accounts"
156
157 alice = store.User{ID: aliceID, Username: "alice"}
158 bob = store.User{ID: bobID, Username: "bob"}
159 return
160}
161
162// loginCookie mints a real web session, the same as a browser login
163// would, so a handler under test reads a viewer through s.viewer /
164// s.webViewer exactly as it does in production.
165func loginCookie(t *testing.T, st *store.Store, userID int64) *http.Cookie {
166 t.Helper()
167 tok, hash, err := store.NewToken()
168 if err != nil {
169 t.Fatal(err)
170 }
171 if err := st.CreateWebSession(hash, userID, time.Hour); err != nil {
172 t.Fatal(err)
173 }
174 return &http.Cookie{Name: sessionCookie, Value: tok}
175}
176
177// The range-diff page must answer exactly as the MR page does: the owner
178// reads it, and a private repository is 404 — never 403, which would
179// confirm the namespace — for both an anonymous caller and a signed-in
180// user with no access (#269).
181func TestMRRangeDiffPagePrivateRepo(t *testing.T) {
182 st, cfg, alice, bob, repo, n, _, _, title := rangeDiffFixture(t)
183 s := New(cfg, st)
184
185 newReq := func(cookie *http.Cookie) (*httptest.ResponseRecorder, *http.Request) {
186 req := httptest.NewRequest("GET", "/alice/secret/mrs/"+strconv.FormatInt(n, 10)+"/range-diff", nil)
187 req.SetPathValue("owner", repo.OwnerName)
188 req.SetPathValue("repo", repo.Name)
189 req.SetPathValue("n", strconv.FormatInt(n, 10))
190 if cookie != nil {
191 req.AddCookie(cookie)
192 }
193 return httptest.NewRecorder(), req
194 }
195
196 t.Run("owner reads it", func(t *testing.T) {
197 rr, req := newReq(loginCookie(t, st, alice.ID))
198 s.mrRangeDiff(rr, req)
199 if rr.Code != 200 {
200 t.Fatalf("status %d, body %s", rr.Code, rr.Body.String())
201 }
202 if !strings.Contains(rr.Body.String(), title) {
203 t.Errorf("owner's page does not show the MR title %q:\n%s", title, rr.Body.String())
204 }
205 })
206
207 t.Run("anonymous gets 404 and no title", func(t *testing.T) {
208 rr, req := newReq(nil)
209 s.mrRangeDiff(rr, req)
210 if rr.Code != 404 {
211 t.Fatalf("status %d, want 404 (never 403), body %s", rr.Code, rr.Body.String())
212 }
213 if strings.Contains(rr.Body.String(), title) {
214 t.Errorf("404 body leaks the MR title %q:\n%s", title, rr.Body.String())
215 }
216 })
217
218 t.Run("a signed-in non-reader gets 404 and no title", func(t *testing.T) {
219 rr, req := newReq(loginCookie(t, st, bob.ID))
220 s.mrRangeDiff(rr, req)
221 if rr.Code != 404 {
222 t.Fatalf("status %d, want 404 (never 403), body %s", rr.Code, rr.Body.String())
223 }
224 if strings.Contains(rr.Body.String(), title) {
225 t.Errorf("404 body leaks the MR title %q:\n%s", title, rr.Body.String())
226 }
227 })
228}
229
230// --from/--to reach the control command as real argv: an unknown
231// revision is refused with not-found, and two real revisions render the
232// range-diff between exactly those two.
233func TestMRRangeDiffPageFromToQuery(t *testing.T) {
234 st, cfg, alice, _, repo, n, v1, v2, _ := rangeDiffFixture(t)
235 s := New(cfg, st)
236 cookie := loginCookie(t, st, alice.ID)
237
238 newReq := func(query string) (*httptest.ResponseRecorder, *http.Request) {
239 req := httptest.NewRequest("GET", "/alice/secret/mrs/"+strconv.FormatInt(n, 10)+"/range-diff"+query, nil)
240 req.SetPathValue("owner", repo.OwnerName)
241 req.SetPathValue("repo", repo.Name)
242 req.SetPathValue("n", strconv.FormatInt(n, 10))
243 req.AddCookie(cookie)
244 return httptest.NewRecorder(), req
245 }
246
247 t.Run("unknown revision is 404", func(t *testing.T) {
248 rr, req := newReq("?from=0000000000000000000000000000000000000000")
249 s.mrRangeDiff(rr, req)
250 if rr.Code != 404 {
251 t.Fatalf("status %d, want 404, body %s", rr.Code, rr.Body.String())
252 }
253 })
254
255 t.Run("two real revisions render the diff between them", func(t *testing.T) {
256 rr, req := newReq("?from=" + v1 + "&to=" + v2)
257 s.mrRangeDiff(rr, req)
258 if rr.Code != 200 {
259 t.Fatalf("status %d, body %s", rr.Code, rr.Body.String())
260 }
261 body := rr.Body.String()
262 if !strings.Contains(body, "beta revised") {
263 t.Errorf("range-diff does not show the changed line:\n%s", body)
264 }
265 })
266
267 t.Run("the same revision twice is a usage error rendered inline", func(t *testing.T) {
268 rr, req := newReq("?from=" + v1 + "&to=" + v1)
269 s.mrRangeDiff(rr, req)
270 if rr.Code != 200 {
271 t.Fatalf("status %d, want 200 (the error renders on the page), body %s", rr.Code, rr.Body.String())
272 }
273 if !strings.Contains(rr.Body.String(), "same revision") {
274 t.Errorf("page does not show the usage error:\n%s", rr.Body.String())
275 }
276 })
277}
278
279// mrRangeDiffPageData mirrors the anonymous struct mrRangeDiff renders
280// with, the way mrPageData mirrors mr's in mrpage_test.go:16-40.
281type mrRangeDiffPageData struct {
282 repoPage
283 MR store.MR
284 Diff string
285 Error string
286}
287
288func testRangeDiffMR() store.MR {
289 return store.MR{Number: 7, Title: "org native rendering", Author: "cmc"}
290}
291
292// The diff branch renders the command's raw text output, HTML-escaped —
293// it is not markup, and must not be treated as any.
294func TestMRRangeDiffTemplateEscapesDiff(t *testing.T) {
295 var sb strings.Builder
296 if err := web.Render(&sb, "mrrangediff.html", mrRangeDiffPageData{
297 repoPage: testRepoPage(), MR: testRangeDiffMR(),
298 Diff: `<script>alert("x")&</script>`,
299 }); err != nil {
300 t.Fatalf("render: %v", err)
301 }
302 out := sb.String()
303 if strings.Contains(out, "<script>") {
304 t.Errorf("diff output was not escaped:\n%s", out)
305 }
306 if !strings.Contains(out, "&lt;script&gt;") || !strings.Contains(out, "&amp;") {
307 t.Errorf("diff output is missing its escaped form:\n%s", out)
308 }
309}
310
311// One revision has nothing to compare; the page says so rather than
312// showing an empty <pre>.
313func TestMRRangeDiffTemplateOneRevision(t *testing.T) {
314 var sb strings.Builder
315 if err := web.Render(&sb, "mrrangediff.html", mrRangeDiffPageData{
316 repoPage: testRepoPage(), MR: testRangeDiffMR(),
317 }); err != nil {
318 t.Fatalf("render: %v", err)
319 }
320 out := sb.String()
321 if !strings.Contains(out, "Nothing to compare") {
322 t.Errorf("empty diff does not explain there is nothing to compare:\n%s", out)
323 }
324 if strings.Contains(out, `<pre class="code`) {
325 t.Errorf("empty diff still rendered a pre block:\n%s", out)
326 }
327}
328
329// A command refusal (same revision twice, in production) renders as a
330// page error, not a diff.
331func TestMRRangeDiffTemplateError(t *testing.T) {
332 var sb strings.Builder
333 if err := web.Render(&sb, "mrrangediff.html", mrRangeDiffPageData{
334 repoPage: testRepoPage(), MR: testRangeDiffMR(),
335 Error: "--from and --to are the same revision",
336 }); err != nil {
337 t.Fatalf("render: %v", err)
338 }
339 out := sb.String()
340 if !strings.Contains(out, "same revision") {
341 t.Errorf("error was not rendered:\n%s", out)
342 }
343 if strings.Contains(out, `<pre class="code`) {
344 t.Errorf("error state still rendered a diff block:\n%s", out)
345 }
346}
internal/httpd/routes.go +1
@@ -100,6 +100,7 @@ func (s *Server) Routes() []Route {
100 Route{Method: "GET", Pattern: "/{owner}/{repo}/issues/{n}", Handler: s.issue}, 100 Route{Method: "GET", Pattern: "/{owner}/{repo}/issues/{n}", Handler: s.issue},
101 Route{Method: "GET", Pattern: "/{owner}/{repo}/mrs", Handler: s.mrs}, 101 Route{Method: "GET", Pattern: "/{owner}/{repo}/mrs", Handler: s.mrs},
102 Route{Method: "GET", Pattern: "/{owner}/{repo}/mrs/{n}", Handler: s.mr}, 102 Route{Method: "GET", Pattern: "/{owner}/{repo}/mrs/{n}", Handler: s.mr},
103 Route{Method: "GET", Pattern: "/{owner}/{repo}/mrs/{n}/range-diff", Handler: s.mrRangeDiff},
103 ) 104 )
104 105
105 // The JSON API is its own opt-in surface, independent of web.mode. 106 // The JSON API is its own opt-in surface, independent of web.mode.
internal/web/templates/mrrangediff.html added +9
@@ -0,0 +1,9 @@
1{{define "width"}}wide{{end}}
2{{define "title"}}range-diff · !{{.MR.Number}} · {{.Repo.OwnerName}}/{{.Repo.Name}}{{end}}
3{{define "content"}}
4<h1>Range-diff <span class="issuenumber">!{{.MR.Number}}</span></h1>
5<p class="meta"><a href="/{{.Repo.OwnerName}}/{{.Repo.Name}}/mrs/{{.MR.Number}}">back to !{{.MR.Number}} {{.MR.Title}}</a></p>
6{{if .Error}}<p class="error" role="alert">{{.Error}}</p>
7{{else if .Diff}}<pre class="code buildlog" tabindex="0">{{.Diff}}</pre>
8{{else}}<p class="empty-note">Nothing to compare: this merge request has one revision.</p>{{end}}
9{{end}}
internal/web/web_test.go +1 −1
@@ -89,7 +89,7 @@ func TestWhenNamesTheZone(t *testing.T) {
89// none. The merge request page picks wide for its diff view, so it gets 89// none. The merge request page picks wide for its diff view, so it gets
90// a per-view define instead of a fixed one. 90// a per-view define instead of a fixed one.
91func TestMainWidthClass(t *testing.T) { 91func TestMainWidthClass(t *testing.T) {
92 wide := map[string]bool{"tree.html": true, "blob.html": true, "blame.html": true, "log.html": true, "commit.html": true, "compare.html": true, "builds.html": true, "build.html": true, "search.html": true, "globalsearch.html": true, "edit.html": true, "dashboard.html": true, "issues.html": true, "mrs.html": true, "explore.html": true, "notifications.html": true, "settings.html": true, "account.html": true, "admin.html": true, "adminusers.html": true} 92 wide := map[string]bool{"tree.html": true, "blob.html": true, "blame.html": true, "log.html": true, "commit.html": true, "compare.html": true, "builds.html": true, "build.html": true, "search.html": true, "globalsearch.html": true, "edit.html": true, "dashboard.html": true, "issues.html": true, "mrs.html": true, "mrrangediff.html": true, "explore.html": true, "notifications.html": true, "settings.html": true, "account.html": true, "admin.html": true, "adminusers.html": true}
93 bounded := map[string]bool{"landing.html": true, "fork.html": true, "login.html": true, "logout.html": true, "register.html": true, "registered.html": true, "new.html": true, "issuenew.html": true, "mrnew.html": true, "snippetnew.html": true, "privacy.html": true, "404.html": true} 93 bounded := map[string]bool{"landing.html": true, "fork.html": true, "login.html": true, "logout.html": true, "register.html": true, "registered.html": true, "new.html": true, "issuenew.html": true, "mrnew.html": true, "snippetnew.html": true, "privacy.html": true, "404.html": true}
94 perView := map[string]string{"mr.html": `{{define "width"}}{{if eq .View "diff"}}wide{{else}}reading{{end}}{{end}}`} 94 perView := map[string]string{"mr.html": `{{define "width"}}{{if eq .View "diff"}}wide{{else}}reading{{end}}{{end}}`}
95 for _, name := range Pages() { 95 for _, name := range Pages() {