refactor(diff): move the commit page window and the one-file splice into the usecases
10 files · +120 -49
M internal/adapters/gitcli/diff.go +0 -2
| @@ -88,8 +88,6 @@ type rawEntry struct { | |||
| 88 | 88 | } | |
| 89 | 89 | ||
| 90 | 90 | // readDiff parses the output of diff-tree --raw --numstat -p. | |
| 91 | - | // | |
| 92 | - | //hexcheck:ignore suspicious-business-logic-in-adapter streams raw, numstat and patch sections; domain.DiffBudget decides window and hunk eligibility and accounts retained lines and bytes; adapter only skips sections and stops the process | |
| 93 | 91 | func readDiff(r *bufio.Reader, limits domain.DiffLimits) (domain.Diff, domain.DiffStats, error) { | |
| 94 | 92 | entries, err := readRaw(r) | |
| 95 | 93 | if err != nil { | |
| @@ -88,8 +88,6 @@ type rawEntry struct { | |||||
| 88 | } | 88 | } | ||
| 89 | 89 | ||||
| 90 | // readDiff parses the output of diff-tree --raw --numstat -p. | 90 | // readDiff parses the output of diff-tree --raw --numstat -p. | ||
| 91 | - | // | |||
| 92 | - | //hexcheck:ignore suspicious-business-logic-in-adapter streams raw, numstat and patch sections; domain.DiffBudget decides window and hunk eligibility and accounts retained lines and bytes; adapter only skips sections and stops the process | |||
| 93 | func readDiff(r *bufio.Reader, limits domain.DiffLimits) (domain.Diff, domain.DiffStats, error) { | 91 | func readDiff(r *bufio.Reader, limits domain.DiffLimits) (domain.Diff, domain.DiffStats, error) { | ||
| 94 | entries, err := readRaw(r) | 92 | entries, err := readRaw(r) | ||
| 95 | if err != nil { | 93 | if err != nil { | ||
M internal/adapters/web/commit.go +3 -25
| @@ -205,34 +205,12 @@ func diffNote(d domain.Diff) string { | |||
| 205 | 205 | return "this diff is incomplete: " + plural(gone, "file is", "files are") + " too large to show. the totals above count everything; clone the repository to read the rest." | |
| 206 | 206 | } | |
| 207 | 207 | ||
| 208 | - | // commitWindow is how many files of a commit get their hunks read with the | |
| 209 | - | // page; the files past it are collapsed, and load on their own. | |
| 210 | - | const commitWindow = 300 | |
| 211 | - | ||
| 212 | 208 | // commitDiff reads a commit for its page (and the compare page): every file | |
| 213 | - | // listed, the hunks of the first commitWindow files read, generated files | |
| 214 | - | // collapsed. Files past the window are collapsed too. With ?file=i (the link | |
| 209 | + | // listed, the hunks of the first files read, generated files and files past | |
| 210 | + | // the window collapsed (ports.GetCommitOptions.Page). With ?file=i (the link | |
| 215 | 211 | // of a page without scripts) file i is read in full and replaces its entry. | |
| 216 | 212 | func (s *Server) commitDiff(ctx context.Context, r *http.Request, repo domain.RepoName, rev string) (domain.CommitDetail, error) { | |
| 217 | - | res, err := s.getCommit(ctx, repo, rev, ports.GetCommitOptions{Files: &ports.FileWindow{From: 0, To: commitWindow}}) | |
| 218 | - | if err != nil { | |
| 219 | - | return res, err | |
| 220 | - | } | |
| 221 | - | for i := commitWindow; i < len(res.Diff.Files); i++ { | |
| 222 | - | if f := &res.Diff.Files[i]; !f.Binary && f.Omitted == domain.OmitNone && f.Additions+f.Deletions > 0 { | |
| 223 | - | f.Omitted = domain.OmitBudget | |
| 224 | - | } | |
| 225 | - | } | |
| 226 | - | if i, ok := fileParam(r, len(res.Diff.Files)); ok && res.Diff.Files[i].Omitted.Collapsed() { | |
| 227 | - | one, err := s.getCommit(ctx, repo, rev, ports.GetCommitOptions{Files: &ports.FileWindow{From: i, To: i + 1, Full: true}}) | |
| 228 | - | if err != nil { | |
| 229 | - | return res, err | |
| 230 | - | } | |
| 231 | - | if i < len(one.Diff.Files) { | |
| 232 | - | res.Diff.Files[i] = one.Diff.Files[i] | |
| 233 | - | } | |
| 234 | - | } | |
| 235 | - | return res, nil | |
| 213 | + | return s.getCommit(ctx, repo, rev, ports.GetCommitOptions{Page: true, Load: fileParam(r)}) | |
| 236 | 214 | } | |
| 237 | 215 | ||
| 238 | 216 | // commitLinks are the addresses a commit page loads its collapsed files from. | |
| @@ -205,34 +205,12 @@ func diffNote(d domain.Diff) string { | |||||
| 205 | return "this diff is incomplete: " + plural(gone, "file is", "files are") + " too large to show. the totals above count everything; clone the repository to read the rest." | 205 | return "this diff is incomplete: " + plural(gone, "file is", "files are") + " too large to show. the totals above count everything; clone the repository to read the rest." | ||
| 206 | } | 206 | } | ||
| 207 | 207 | ||||
| 208 | - | // commitWindow is how many files of a commit get their hunks read with the | |||
| 209 | - | // page; the files past it are collapsed, and load on their own. | |||
| 210 | - | const commitWindow = 300 | |||
| 211 | - | ||||
| 212 | // commitDiff reads a commit for its page (and the compare page): every file | 208 | // commitDiff reads a commit for its page (and the compare page): every file | ||
| 213 | - | // listed, the hunks of the first commitWindow files read, generated files | 209 | + | // listed, the hunks of the first files read, generated files and files past |
| 214 | - | // collapsed. Files past the window are collapsed too. With ?file=i (the link | 210 | + | // the window collapsed (ports.GetCommitOptions.Page). With ?file=i (the link |
| 215 | // of a page without scripts) file i is read in full and replaces its entry. | 211 | // of a page without scripts) file i is read in full and replaces its entry. | ||
| 216 | func (s *Server) commitDiff(ctx context.Context, r *http.Request, repo domain.RepoName, rev string) (domain.CommitDetail, error) { | 212 | func (s *Server) commitDiff(ctx context.Context, r *http.Request, repo domain.RepoName, rev string) (domain.CommitDetail, error) { | ||
| 217 | - | res, err := s.getCommit(ctx, repo, rev, ports.GetCommitOptions{Files: &ports.FileWindow{From: 0, To: commitWindow}}) | 213 | + | return s.getCommit(ctx, repo, rev, ports.GetCommitOptions{Page: true, Load: fileParam(r)}) |
| 218 | - | if err != nil { | |||
| 219 | - | return res, err | |||
| 220 | - | } | |||
| 221 | - | for i := commitWindow; i < len(res.Diff.Files); i++ { | |||
| 222 | - | if f := &res.Diff.Files[i]; !f.Binary && f.Omitted == domain.OmitNone && f.Additions+f.Deletions > 0 { | |||
| 223 | - | f.Omitted = domain.OmitBudget | |||
| 224 | - | } | |||
| 225 | - | } | |||
| 226 | - | if i, ok := fileParam(r, len(res.Diff.Files)); ok && res.Diff.Files[i].Omitted.Collapsed() { | |||
| 227 | - | one, err := s.getCommit(ctx, repo, rev, ports.GetCommitOptions{Files: &ports.FileWindow{From: i, To: i + 1, Full: true}}) | |||
| 228 | - | if err != nil { | |||
| 229 | - | return res, err | |||
| 230 | - | } | |||
| 231 | - | if i < len(one.Diff.Files) { | |||
| 232 | - | res.Diff.Files[i] = one.Diff.Files[i] | |||
| 233 | - | } | |||
| 234 | - | } | |||
| 235 | - | return res, nil | |||
| 236 | } | 214 | } | ||
| 237 | 215 | ||||
| 238 | // commitLinks are the addresses a commit page loads its collapsed files from. | 216 | // commitLinks are the addresses a commit page loads its collapsed files from. | ||
M internal/adapters/web/diff.go +7 -5
| @@ -490,6 +490,8 @@ func oneFileSrc(src string, i int) string { | |||
| 490 | 490 | ||
| 491 | 491 | // collapseFiles gives the collapsed files of a diff their load address, and | |
| 492 | 492 | // the files too large to show a link to their raw blob. Nothing is read. | |
| 493 | + | // | |
| 494 | + | //hexcheck:ignore suspicious-business-logic-in-adapter only maps each file's domain omission (Omitted.Collapsed) to a load link, a ?file= anchor or a raw link; it decides nothing and reads nothing | |
| 493 | 495 | func collapseFiles(out []DiffFile, files []domain.FileDiff, l fileLinks) { | |
| 494 | 496 | for i, f := range files { | |
| 495 | 497 | if i >= len(out) { | |
| @@ -521,14 +523,14 @@ func rawLink(f domain.FileDiff, l fileLinks) string { | |||
| 521 | 523 | } | |
| 522 | 524 | ||
| 523 | 525 | // fileParam reads ?file= of a page without scripts: the index of a collapsed | |
| 524 | - | // file to read in full, in [0, n). | |
| 525 | - | func fileParam(r *http.Request, n int) (int, bool) { | |
| 526 | + | // file to read in full; nil when absent or malformed. | |
| 527 | + | func fileParam(r *http.Request) *int { | |
| 526 | 528 | s := r.URL.Query().Get("file") | |
| 527 | 529 | i, err := strconv.Atoi(s) | |
| 528 | - | if err != nil || i < 0 || i >= n || strconv.Itoa(i) != s { | |
| 529 | - | return 0, false | |
| 530 | + | if err != nil || i < 0 || strconv.Itoa(i) != s { | |
| 531 | + | return nil | |
| 530 | 532 | } | |
| 531 | - | return i, true | |
| 533 | + | return &i | |
| 532 | 534 | } | |
| 533 | 535 | ||
| 534 | 536 | // fragmentView lays out a one-file fragment like the page that asks for it: | |
| @@ -490,6 +490,8 @@ func oneFileSrc(src string, i int) string { | |||||
| 490 | 490 | ||||
| 491 | // collapseFiles gives the collapsed files of a diff their load address, and | 491 | // collapseFiles gives the collapsed files of a diff their load address, and | ||
| 492 | // the files too large to show a link to their raw blob. Nothing is read. | 492 | // the files too large to show a link to their raw blob. Nothing is read. | ||
| 493 | + | // | |||
| 494 | + | //hexcheck:ignore suspicious-business-logic-in-adapter only maps each file's domain omission (Omitted.Collapsed) to a load link, a ?file= anchor or a raw link; it decides nothing and reads nothing | |||
| 493 | func collapseFiles(out []DiffFile, files []domain.FileDiff, l fileLinks) { | 495 | func collapseFiles(out []DiffFile, files []domain.FileDiff, l fileLinks) { | ||
| 494 | for i, f := range files { | 496 | for i, f := range files { | ||
| 495 | if i >= len(out) { | 497 | if i >= len(out) { | ||
| @@ -521,14 +523,14 @@ func rawLink(f domain.FileDiff, l fileLinks) string { | |||||
| 521 | } | 523 | } | ||
| 522 | 524 | ||||
| 523 | // fileParam reads ?file= of a page without scripts: the index of a collapsed | 525 | // fileParam reads ?file= of a page without scripts: the index of a collapsed | ||
| 524 | - | // file to read in full, in [0, n). | 526 | + | // file to read in full; nil when absent or malformed. |
| 525 | - | func fileParam(r *http.Request, n int) (int, bool) { | 527 | + | func fileParam(r *http.Request) *int { |
| 526 | s := r.URL.Query().Get("file") | 528 | s := r.URL.Query().Get("file") | ||
| 527 | i, err := strconv.Atoi(s) | 529 | i, err := strconv.Atoi(s) | ||
| 528 | - | if err != nil || i < 0 || i >= n || strconv.Itoa(i) != s { | 530 | + | if err != nil || i < 0 || strconv.Itoa(i) != s { |
| 529 | - | return 0, false | 531 | + | return nil |
| 530 | } | 532 | } | ||
| 531 | - | return i, true | 533 | + | return &i |
| 532 | } | 534 | } | ||
| 533 | 535 | ||||
| 534 | // fragmentView lays out a one-file fragment like the page that asks for it: | 536 | // fragmentView lays out a one-file fragment like the page that asks for it: | ||
M internal/adapters/web/mr.go +1 -16
| @@ -4,7 +4,6 @@ package web | |||
| 4 | 4 | ||
| 5 | 5 | import ( | |
| 6 | 6 | "context" | |
| 7 | - | "errors" | |
| 8 | 7 | "html" | |
| 9 | 8 | "html/template" | |
| 10 | 9 | "io" | |
| @@ -563,6 +562,7 @@ func (s *Server) mrPage(ctx context.Context, r *http.Request, since bool) (templ | |||
| 563 | 562 | } | |
| 564 | 563 | res, err := s.cfg.MergeRequests.Get(ctx, principal(ctx), d.repo.Name, n, ports.GetMergeRequestOptions{ | |
| 565 | 564 | SinceLastRead: since, Files: &ports.FileWindow{From: from, To: from + mrWindow}, | |
| 565 | + | Load: fileParam(r), // a page without scripts: ?file=i reads that collapsed file in full | |
| 566 | 566 | }) | |
| 567 | 567 | if err != nil { | |
| 568 | 568 | return nil, 0, err | |
| @@ -570,21 +570,6 @@ func (s *Server) mrPage(ctx context.Context, r *http.Request, since bool) (templ | |||
| 570 | 570 | if from > 0 && from >= len(res.Diff.Files) { | |
| 571 | 571 | return nil, 0, errNotFound | |
| 572 | 572 | } | |
| 573 | - | // A page without scripts: ?file=i reads that collapsed file in full. | |
| 574 | - | // Without a pinned base, or when the pair moved since the first read, the | |
| 575 | - | // page is shown unspliced. | |
| 576 | - | if i, ok := fileParam(r, len(res.Diff.Files)); ok && res.Diff.Files[i].Omitted.Collapsed() && !res.DiffBaseID.IsZero() { | |
| 577 | - | one, err := s.cfg.MergeRequests.Get(ctx, principal(ctx), d.repo.Name, n, ports.GetMergeRequestOptions{ | |
| 578 | - | SinceLastRead: since, DiffOnly: true, Base: res.DiffBaseID, Head: res.MergeRequest.HeadID, | |
| 579 | - | Files: &ports.FileWindow{From: i, To: i + 1, Full: true}, | |
| 580 | - | }) | |
| 581 | - | if err != nil && !errors.Is(err, domain.ErrConflict) { | |
| 582 | - | return nil, 0, err | |
| 583 | - | } | |
| 584 | - | if err == nil && i < len(one.Diff.Files) { | |
| 585 | - | res.Diff.Files[i] = one.Diff.Files[i] | |
| 586 | - | } | |
| 587 | - | } | |
| 588 | 573 | m := res.MergeRequest | |
| 589 | 574 | signed := SignedIn(ctx) | |
| 590 | 575 | open := m.State == domain.MRStateOpen | |
| @@ -4,7 +4,6 @@ package web | |||||
| 4 | 4 | ||||
| 5 | import ( | 5 | import ( | ||
| 6 | "context" | 6 | "context" | ||
| 7 | - | "errors" | |||
| 8 | "html" | 7 | "html" | ||
| 9 | "html/template" | 8 | "html/template" | ||
| 10 | "io" | 9 | "io" | ||
| @@ -563,6 +562,7 @@ func (s *Server) mrPage(ctx context.Context, r *http.Request, since bool) (templ | |||||
| 563 | } | 562 | } | ||
| 564 | res, err := s.cfg.MergeRequests.Get(ctx, principal(ctx), d.repo.Name, n, ports.GetMergeRequestOptions{ | 563 | res, err := s.cfg.MergeRequests.Get(ctx, principal(ctx), d.repo.Name, n, ports.GetMergeRequestOptions{ | ||
| 565 | SinceLastRead: since, Files: &ports.FileWindow{From: from, To: from + mrWindow}, | 564 | SinceLastRead: since, Files: &ports.FileWindow{From: from, To: from + mrWindow}, | ||
| 565 | + | Load: fileParam(r), // a page without scripts: ?file=i reads that collapsed file in full | |||
| 566 | }) | 566 | }) | ||
| 567 | if err != nil { | 567 | if err != nil { | ||
| 568 | return nil, 0, err | 568 | return nil, 0, err | ||
| @@ -570,21 +570,6 @@ func (s *Server) mrPage(ctx context.Context, r *http.Request, since bool) (templ | |||||
| 570 | if from > 0 && from >= len(res.Diff.Files) { | 570 | if from > 0 && from >= len(res.Diff.Files) { | ||
| 571 | return nil, 0, errNotFound | 571 | return nil, 0, errNotFound | ||
| 572 | } | 572 | } | ||
| 573 | - | // A page without scripts: ?file=i reads that collapsed file in full. | |||
| 574 | - | // Without a pinned base, or when the pair moved since the first read, the | |||
| 575 | - | // page is shown unspliced. | |||
| 576 | - | if i, ok := fileParam(r, len(res.Diff.Files)); ok && res.Diff.Files[i].Omitted.Collapsed() && !res.DiffBaseID.IsZero() { | |||
| 577 | - | one, err := s.cfg.MergeRequests.Get(ctx, principal(ctx), d.repo.Name, n, ports.GetMergeRequestOptions{ | |||
| 578 | - | SinceLastRead: since, DiffOnly: true, Base: res.DiffBaseID, Head: res.MergeRequest.HeadID, | |||
| 579 | - | Files: &ports.FileWindow{From: i, To: i + 1, Full: true}, | |||
| 580 | - | }) | |||
| 581 | - | if err != nil && !errors.Is(err, domain.ErrConflict) { | |||
| 582 | - | return nil, 0, err | |||
| 583 | - | } | |||
| 584 | - | if err == nil && i < len(one.Diff.Files) { | |||
| 585 | - | res.Diff.Files[i] = one.Diff.Files[i] | |||
| 586 | - | } | |||
| 587 | - | } | |||
| 588 | m := res.MergeRequest | 573 | m := res.MergeRequest | ||
| 589 | signed := SignedIn(ctx) | 574 | signed := SignedIn(ctx) | ||
| 590 | open := m.State == domain.MRStateOpen | 575 | open := m.State == domain.MRStateOpen | ||
M internal/domain/budget_test.go +18 -0
| @@ -240,3 +240,21 @@ func TestDiffBudgetAdmitAloneLines(t *testing.T) { | |||
| 240 | 240 | t.Fatalf("over the cap: read=%v why=%v, want OmitUnavailable", read, why) | |
| 241 | 241 | } | |
| 242 | 242 | } | |
| 243 | + | ||
| 244 | + | func TestDiffCollapseFrom(t *testing.T) { | |
| 245 | + | d := Diff{Files: []FileDiff{ | |
| 246 | + | {Additions: 1}, | |
| 247 | + | {Additions: 1}, | |
| 248 | + | {Binary: true, Additions: 1}, | |
| 249 | + | {Omitted: OmitLarge, Additions: 1}, | |
| 250 | + | {}, | |
| 251 | + | {Deletions: 2}, | |
| 252 | + | }} | |
| 253 | + | d.CollapseFrom(1) | |
| 254 | + | want := []FileOmission{OmitNone, OmitBudget, OmitNone, OmitLarge, OmitNone, OmitBudget} | |
| 255 | + | for i, f := range d.Files { | |
| 256 | + | if f.Omitted != want[i] { | |
| 257 | + | t.Errorf("file %d: omitted %v, want %v", i, f.Omitted, want[i]) | |
| 258 | + | } | |
| 259 | + | } | |
| 260 | + | } | |
| @@ -240,3 +240,21 @@ func TestDiffBudgetAdmitAloneLines(t *testing.T) { | |||||
| 240 | t.Fatalf("over the cap: read=%v why=%v, want OmitUnavailable", read, why) | 240 | t.Fatalf("over the cap: read=%v why=%v, want OmitUnavailable", read, why) | ||
| 241 | } | 241 | } | ||
| 242 | } | 242 | } | ||
| 243 | + | ||||
| 244 | + | func TestDiffCollapseFrom(t *testing.T) { | |||
| 245 | + | d := Diff{Files: []FileDiff{ | |||
| 246 | + | {Additions: 1}, | |||
| 247 | + | {Additions: 1}, | |||
| 248 | + | {Binary: true, Additions: 1}, | |||
| 249 | + | {Omitted: OmitLarge, Additions: 1}, | |||
| 250 | + | {}, | |||
| 251 | + | {Deletions: 2}, | |||
| 252 | + | }} | |||
| 253 | + | d.CollapseFrom(1) | |||
| 254 | + | want := []FileOmission{OmitNone, OmitBudget, OmitNone, OmitLarge, OmitNone, OmitBudget} | |||
| 255 | + | for i, f := range d.Files { | |||
| 256 | + | if f.Omitted != want[i] { | |||
| 257 | + | t.Errorf("file %d: omitted %v, want %v", i, f.Omitted, want[i]) | |||
| 258 | + | } | |||
| 259 | + | } | |||
| 260 | + | } | |||
M internal/domain/git.go +15 -0
| @@ -693,3 +693,18 @@ func (t *Tree) BoundTree() { | |||
| 693 | 693 | t.Entries, t.Truncated = t.Entries[:MaxTreeEntries], true | |
| 694 | 694 | } | |
| 695 | 695 | } | |
| 696 | + | ||
| 697 | + | // CommitPageFiles is how many files of a commit get their hunks read with its | |
| 698 | + | // page; the files past it are collapsed (see Diff.CollapseFrom) and load on | |
| 699 | + | // their own. | |
| 700 | + | const CommitPageFiles = 300 | |
| 701 | + | ||
| 702 | + | // CollapseFrom marks the files from index n on that have lines to show (not | |
| 703 | + | // binary, not already omitted, with changes) as OmitBudget: listed, not read. | |
| 704 | + | func (d *Diff) CollapseFrom(n int) { | |
| 705 | + | for i := max(n, 0); i < len(d.Files); i++ { | |
| 706 | + | if f := &d.Files[i]; !f.Binary && f.Omitted == OmitNone && f.Additions+f.Deletions > 0 { | |
| 707 | + | f.Omitted = OmitBudget | |
| 708 | + | } | |
| 709 | + | } | |
| 710 | + | } | |
| @@ -693,3 +693,18 @@ func (t *Tree) BoundTree() { | |||||
| 693 | t.Entries, t.Truncated = t.Entries[:MaxTreeEntries], true | 693 | t.Entries, t.Truncated = t.Entries[:MaxTreeEntries], true | ||
| 694 | } | 694 | } | ||
| 695 | } | 695 | } | ||
| 696 | + | ||||
| 697 | + | // CommitPageFiles is how many files of a commit get their hunks read with its | |||
| 698 | + | // page; the files past it are collapsed (see Diff.CollapseFrom) and load on | |||
| 699 | + | // their own. | |||
| 700 | + | const CommitPageFiles = 300 | |||
| 701 | + | ||||
| 702 | + | // CollapseFrom marks the files from index n on that have lines to show (not | |||
| 703 | + | // binary, not already omitted, with changes) as OmitBudget: listed, not read. | |||
| 704 | + | func (d *Diff) CollapseFrom(n int) { | |||
| 705 | + | for i := max(n, 0); i < len(d.Files); i++ { | |||
| 706 | + | if f := &d.Files[i]; !f.Binary && f.Omitted == OmitNone && f.Additions+f.Deletions > 0 { | |||
| 707 | + | f.Omitted = OmitBudget | |||
| 708 | + | } | |||
| 709 | + | } | |||
| 710 | + | } | |||
M internal/ports/dto.go +13 -0
| @@ -309,6 +309,11 @@ type GetMergeRequestOptions struct { | |||
| 309 | 309 | Base, Head domain.ObjectID | |
| 310 | 310 | // DiffOnly skips the commits of the request (Commits stays empty). | |
| 311 | 311 | DiffOnly bool | |
| 312 | + | // Load, when set with Files, is the index of a collapsed file Get reads in | |
| 313 | + | // full and splices into the diff in place of its collapsed entry (the page | |
| 314 | + | // of a client without scripts). Ignored when the index is out of range, | |
| 315 | + | // the file is not collapsed, or there is no diff. | |
| 316 | + | Load *int | |
| 312 | 317 | } | |
| 313 | 318 | ||
| 314 | 319 | // FileWindow is the range [From, To) of files whose hunks are read. | |
| @@ -327,6 +332,14 @@ type GetCommitOptions struct { | |||
| 327 | 332 | // GetMergeRequestOptions.Files does. Unset (the zero value), the diff is | |
| 328 | 333 | // bounded by domain.DefaultDiffLimits. | |
| 329 | 334 | Files *FileWindow | |
| 335 | + | // Page, without Files, reads the diff for a page: every file listed, the | |
| 336 | + | // hunks of the first domain.CommitPageFiles files read, generated files | |
| 337 | + | // collapsed, and the files past the window collapsed too | |
| 338 | + | // (domain.OmitBudget). Combined with Files it is domain.ErrInvalid. | |
| 339 | + | Page bool | |
| 340 | + | // Load is the index of a collapsed file read in full and spliced into the | |
| 341 | + | // diff, as GetMergeRequestOptions.Load. It requires Page. | |
| 342 | + | Load *int | |
| 330 | 343 | } | |
| 331 | 344 | ||
| 332 | 345 | // AddCommentParams describes the comment to add. | |
| @@ -309,6 +309,11 @@ type GetMergeRequestOptions struct { | |||||
| 309 | Base, Head domain.ObjectID | 309 | Base, Head domain.ObjectID | ||
| 310 | // DiffOnly skips the commits of the request (Commits stays empty). | 310 | // DiffOnly skips the commits of the request (Commits stays empty). | ||
| 311 | DiffOnly bool | 311 | DiffOnly bool | ||
| 312 | + | // Load, when set with Files, is the index of a collapsed file Get reads in | |||
| 313 | + | // full and splices into the diff in place of its collapsed entry (the page | |||
| 314 | + | // of a client without scripts). Ignored when the index is out of range, | |||
| 315 | + | // the file is not collapsed, or there is no diff. | |||
| 316 | + | Load *int | |||
| 312 | } | 317 | } | ||
| 313 | 318 | ||||
| 314 | // FileWindow is the range [From, To) of files whose hunks are read. | 319 | // FileWindow is the range [From, To) of files whose hunks are read. | ||
| @@ -327,6 +332,14 @@ type GetCommitOptions struct { | |||||
| 327 | // GetMergeRequestOptions.Files does. Unset (the zero value), the diff is | 332 | // GetMergeRequestOptions.Files does. Unset (the zero value), the diff is | ||
| 328 | // bounded by domain.DefaultDiffLimits. | 333 | // bounded by domain.DefaultDiffLimits. | ||
| 329 | Files *FileWindow | 334 | Files *FileWindow | ||
| 335 | + | // Page, without Files, reads the diff for a page: every file listed, the | |||
| 336 | + | // hunks of the first domain.CommitPageFiles files read, generated files | |||
| 337 | + | // collapsed, and the files past the window collapsed too | |||
| 338 | + | // (domain.OmitBudget). Combined with Files it is domain.ErrInvalid. | |||
| 339 | + | Page bool | |||
| 340 | + | // Load is the index of a collapsed file read in full and spliced into the | |||
| 341 | + | // diff, as GetMergeRequestOptions.Load. It requires Page. | |||
| 342 | + | Load *int | |||
| 330 | } | 343 | } | ||
| 331 | 344 | ||||
| 332 | // AddCommentParams describes the comment to add. | 345 | // AddCommentParams describes the comment to add. | ||
M internal/usecase/browse.go +20 -1
| @@ -339,7 +339,14 @@ func decodeCommitToken(tok string, path domain.TreePath) (domain.ObjectID, int, | |||
| 339 | 339 | // first parent, within the default diff limits or, with opts.Files, the limits | |
| 340 | 340 | // of that file window. An invalid window gives domain.ErrInvalid. | |
| 341 | 341 | func (b *Browse) GetCommit(ctx context.Context, p domain.Principal, repo domain.RepoName, rev domain.Revision, opts ports.GetCommitOptions) (domain.CommitDetail, error) { | |
| 342 | - | limits, err := windowLimits(opts.Files) | |
| 342 | + | if (opts.Page && opts.Files != nil) || (opts.Load != nil && !opts.Page) { | |
| 343 | + | return domain.CommitDetail{}, fmt.Errorf("%w: commit page options", domain.ErrInvalid) | |
| 344 | + | } | |
| 345 | + | win := opts.Files | |
| 346 | + | if opts.Page { | |
| 347 | + | win = &ports.FileWindow{From: 0, To: domain.CommitPageFiles} | |
| 348 | + | } | |
| 349 | + | limits, err := windowLimits(win) | |
| 343 | 350 | if err != nil { | |
| 344 | 351 | return domain.CommitDetail{}, err | |
| 345 | 352 | } | |
| @@ -350,6 +357,18 @@ func (b *Browse) GetCommit(ctx context.Context, p domain.Principal, repo domain. | |||
| 350 | 357 | if err != nil { | |
| 351 | 358 | return domain.CommitDetail{}, err | |
| 352 | 359 | } | |
| 360 | + | if opts.Page { | |
| 361 | + | d.Diff.CollapseFrom(domain.CommitPageFiles) | |
| 362 | + | if i := opts.Load; i != nil && *i >= 0 && *i < len(d.Diff.Files) && d.Diff.Files[*i].Omitted.Collapsed() { | |
| 363 | + | one, err := b.reader.GetCommit(ctx, repo, rev, domain.OneFileDiffLimits(*i)) | |
| 364 | + | if err != nil { | |
| 365 | + | return domain.CommitDetail{}, err | |
| 366 | + | } | |
| 367 | + | if *i < len(one.Diff.Files) { | |
| 368 | + | d.Diff.Files[*i] = one.Diff.Files[*i] | |
| 369 | + | } | |
| 370 | + | } | |
| 371 | + | } | |
| 353 | 372 | if b.verifier != nil { | |
| 354 | 373 | one := []domain.Commit{d.Commit} | |
| 355 | 374 | b.verify(ctx, repo, one) | |
| @@ -339,7 +339,14 @@ func decodeCommitToken(tok string, path domain.TreePath) (domain.ObjectID, int, | |||||
| 339 | // first parent, within the default diff limits or, with opts.Files, the limits | 339 | // first parent, within the default diff limits or, with opts.Files, the limits | ||
| 340 | // of that file window. An invalid window gives domain.ErrInvalid. | 340 | // of that file window. An invalid window gives domain.ErrInvalid. | ||
| 341 | func (b *Browse) GetCommit(ctx context.Context, p domain.Principal, repo domain.RepoName, rev domain.Revision, opts ports.GetCommitOptions) (domain.CommitDetail, error) { | 341 | func (b *Browse) GetCommit(ctx context.Context, p domain.Principal, repo domain.RepoName, rev domain.Revision, opts ports.GetCommitOptions) (domain.CommitDetail, error) { | ||
| 342 | - | limits, err := windowLimits(opts.Files) | 342 | + | if (opts.Page && opts.Files != nil) || (opts.Load != nil && !opts.Page) { |
| 343 | + | return domain.CommitDetail{}, fmt.Errorf("%w: commit page options", domain.ErrInvalid) | |||
| 344 | + | } | |||
| 345 | + | win := opts.Files | |||
| 346 | + | if opts.Page { | |||
| 347 | + | win = &ports.FileWindow{From: 0, To: domain.CommitPageFiles} | |||
| 348 | + | } | |||
| 349 | + | limits, err := windowLimits(win) | |||
| 343 | if err != nil { | 350 | if err != nil { | ||
| 344 | return domain.CommitDetail{}, err | 351 | return domain.CommitDetail{}, err | ||
| 345 | } | 352 | } | ||
| @@ -350,6 +357,18 @@ func (b *Browse) GetCommit(ctx context.Context, p domain.Principal, repo domain. | |||||
| 350 | if err != nil { | 357 | if err != nil { | ||
| 351 | return domain.CommitDetail{}, err | 358 | return domain.CommitDetail{}, err | ||
| 352 | } | 359 | } | ||
| 360 | + | if opts.Page { | |||
| 361 | + | d.Diff.CollapseFrom(domain.CommitPageFiles) | |||
| 362 | + | if i := opts.Load; i != nil && *i >= 0 && *i < len(d.Diff.Files) && d.Diff.Files[*i].Omitted.Collapsed() { | |||
| 363 | + | one, err := b.reader.GetCommit(ctx, repo, rev, domain.OneFileDiffLimits(*i)) | |||
| 364 | + | if err != nil { | |||
| 365 | + | return domain.CommitDetail{}, err | |||
| 366 | + | } | |||
| 367 | + | if *i < len(one.Diff.Files) { | |||
| 368 | + | d.Diff.Files[*i] = one.Diff.Files[*i] | |||
| 369 | + | } | |||
| 370 | + | } | |||
| 371 | + | } | |||
| 353 | if b.verifier != nil { | 372 | if b.verifier != nil { | ||
| 354 | one := []domain.Commit{d.Commit} | 373 | one := []domain.Commit{d.Commit} | ||
| 355 | b.verify(ctx, repo, one) | 374 | b.verify(ctx, repo, one) | ||
M internal/usecase/browse_test.go +33 -0
| @@ -9,6 +9,7 @@ import ( | |||
| 9 | 9 | "fmt" | |
| 10 | 10 | "io" | |
| 11 | 11 | "runtime" | |
| 12 | + | "slices" | |
| 12 | 13 | "strconv" | |
| 13 | 14 | "strings" | |
| 14 | 15 | "sync" | |
| @@ -635,6 +636,38 @@ func TestBrowseVerifiesCommits(t *testing.T) { | |||
| 635 | 636 | assert.Equal(t, 2, d.Stats.FilesChanged) | |
| 636 | 637 | }) | |
| 637 | 638 | ||
| 639 | + | t.Run("GetCommit Page collapses past the window and loads one file", func(t *testing.T) { | |
| 640 | + | t.Parallel() | |
| 641 | + | f, v := newFixture(t) | |
| 642 | + | v.EXPECT().VerifyCommits(ctx, pub.Name, mock.Anything).Return(nil).Maybe() | |
| 643 | + | f.meta.EXPECT().Get(ctx, pub.Name).Return(pub, nil).Maybe() | |
| 644 | + | files := make([]domain.FileDiff, domain.CommitPageFiles+2) | |
| 645 | + | for i := range files { | |
| 646 | + | files[i].Additions = 1 | |
| 647 | + | } | |
| 648 | + | win := domain.DefaultDiffLimits() | |
| 649 | + | win.MaxFiles, win.From, win.To, win.SkipGenerated = 0, 0, domain.CommitPageFiles, true | |
| 650 | + | f.reader.EXPECT().GetCommit(ctx, pub.Name, mock.Anything, win). | |
| 651 | + | Return(domain.CommitDetail{Diff: domain.Diff{Files: slices.Clone(files)}}, nil) | |
| 652 | + | i := domain.CommitPageFiles + 1 | |
| 653 | + | loaded := slices.Clone(files) | |
| 654 | + | loaded[i].Hunks = []domain.Hunk{{}} | |
| 655 | + | f.reader.EXPECT().GetCommit(ctx, pub.Name, mock.Anything, domain.OneFileDiffLimits(i)). | |
| 656 | + | Return(domain.CommitDetail{Diff: domain.Diff{Files: loaded}}, nil) | |
| 657 | + | ||
| 658 | + | d, err := f.browse.GetCommit(ctx, domain.Anonymous(), pub.Name, domain.Revision{}, ports.GetCommitOptions{Page: true, Load: &i}) | |
| 659 | + | require.NoError(t, err) | |
| 660 | + | assert.Equal(t, domain.OmitNone, d.Diff.Files[0].Omitted) | |
| 661 | + | assert.Equal(t, domain.OmitBudget, d.Diff.Files[domain.CommitPageFiles].Omitted) | |
| 662 | + | assert.Equal(t, domain.OmitNone, d.Diff.Files[i].Omitted) | |
| 663 | + | assert.NotEmpty(t, d.Diff.Files[i].Hunks) | |
| 664 | + | ||
| 665 | + | _, err = f.browse.GetCommit(ctx, domain.Anonymous(), pub.Name, domain.Revision{}, ports.GetCommitOptions{Load: &i}) | |
| 666 | + | assert.ErrorIs(t, err, domain.ErrInvalid) | |
| 667 | + | _, err = f.browse.GetCommit(ctx, domain.Anonymous(), pub.Name, domain.Revision{}, ports.GetCommitOptions{Page: true, Files: &ports.FileWindow{To: 1}}) | |
| 668 | + | assert.ErrorIs(t, err, domain.ErrInvalid) | |
| 669 | + | }) | |
| 670 | + | ||
| 638 | 671 | t.Run("nothing is verified for an empty page or a failed read", func(t *testing.T) { | |
| 639 | 672 | t.Parallel() | |
| 640 | 673 | f, _ := newFixture(t) // the verifier mock fails the test if called | |
| @@ -9,6 +9,7 @@ import ( | |||||
| 9 | "fmt" | 9 | "fmt" | ||
| 10 | "io" | 10 | "io" | ||
| 11 | "runtime" | 11 | "runtime" | ||
| 12 | + | "slices" | |||
| 12 | "strconv" | 13 | "strconv" | ||
| 13 | "strings" | 14 | "strings" | ||
| 14 | "sync" | 15 | "sync" | ||
| @@ -635,6 +636,38 @@ func TestBrowseVerifiesCommits(t *testing.T) { | |||||
| 635 | assert.Equal(t, 2, d.Stats.FilesChanged) | 636 | assert.Equal(t, 2, d.Stats.FilesChanged) | ||
| 636 | }) | 637 | }) | ||
| 637 | 638 | ||||
| 639 | + | t.Run("GetCommit Page collapses past the window and loads one file", func(t *testing.T) { | |||
| 640 | + | t.Parallel() | |||
| 641 | + | f, v := newFixture(t) | |||
| 642 | + | v.EXPECT().VerifyCommits(ctx, pub.Name, mock.Anything).Return(nil).Maybe() | |||
| 643 | + | f.meta.EXPECT().Get(ctx, pub.Name).Return(pub, nil).Maybe() | |||
| 644 | + | files := make([]domain.FileDiff, domain.CommitPageFiles+2) | |||
| 645 | + | for i := range files { | |||
| 646 | + | files[i].Additions = 1 | |||
| 647 | + | } | |||
| 648 | + | win := domain.DefaultDiffLimits() | |||
| 649 | + | win.MaxFiles, win.From, win.To, win.SkipGenerated = 0, 0, domain.CommitPageFiles, true | |||
| 650 | + | f.reader.EXPECT().GetCommit(ctx, pub.Name, mock.Anything, win). | |||
| 651 | + | Return(domain.CommitDetail{Diff: domain.Diff{Files: slices.Clone(files)}}, nil) | |||
| 652 | + | i := domain.CommitPageFiles + 1 | |||
| 653 | + | loaded := slices.Clone(files) | |||
| 654 | + | loaded[i].Hunks = []domain.Hunk{{}} | |||
| 655 | + | f.reader.EXPECT().GetCommit(ctx, pub.Name, mock.Anything, domain.OneFileDiffLimits(i)). | |||
| 656 | + | Return(domain.CommitDetail{Diff: domain.Diff{Files: loaded}}, nil) | |||
| 657 | + | ||||
| 658 | + | d, err := f.browse.GetCommit(ctx, domain.Anonymous(), pub.Name, domain.Revision{}, ports.GetCommitOptions{Page: true, Load: &i}) | |||
| 659 | + | require.NoError(t, err) | |||
| 660 | + | assert.Equal(t, domain.OmitNone, d.Diff.Files[0].Omitted) | |||
| 661 | + | assert.Equal(t, domain.OmitBudget, d.Diff.Files[domain.CommitPageFiles].Omitted) | |||
| 662 | + | assert.Equal(t, domain.OmitNone, d.Diff.Files[i].Omitted) | |||
| 663 | + | assert.NotEmpty(t, d.Diff.Files[i].Hunks) | |||
| 664 | + | ||||
| 665 | + | _, err = f.browse.GetCommit(ctx, domain.Anonymous(), pub.Name, domain.Revision{}, ports.GetCommitOptions{Load: &i}) | |||
| 666 | + | assert.ErrorIs(t, err, domain.ErrInvalid) | |||
| 667 | + | _, err = f.browse.GetCommit(ctx, domain.Anonymous(), pub.Name, domain.Revision{}, ports.GetCommitOptions{Page: true, Files: &ports.FileWindow{To: 1}}) | |||
| 668 | + | assert.ErrorIs(t, err, domain.ErrInvalid) | |||
| 669 | + | }) | |||
| 670 | + | ||||
| 638 | t.Run("nothing is verified for an empty page or a failed read", func(t *testing.T) { | 671 | t.Run("nothing is verified for an empty page or a failed read", func(t *testing.T) { | ||
| 639 | t.Parallel() | 672 | t.Parallel() | ||
| 640 | f, _ := newFixture(t) // the verifier mock fails the test if called | 673 | f, _ := newFixture(t) // the verifier mock fails the test if called | ||
M internal/usecase/mergerequests.go +10 -0
| @@ -621,6 +621,16 @@ func (m *MergeRequests) Get(ctx context.Context, p domain.Principal, name domain | |||
| 621 | 621 | if d.Stats, d.Diff, err = m.git.DiffCommits(ctx, name, base, head, limits); err != nil { | |
| 622 | 622 | return ports.MergeRequestDetail{}, err | |
| 623 | 623 | } | |
| 624 | + | // The page of a client without scripts reads one collapsed file in full. | |
| 625 | + | if i := opts.Load; i != nil && *i >= 0 && *i < len(d.Diff.Files) && d.Diff.Files[*i].Omitted.Collapsed() { | |
| 626 | + | _, one, err := m.git.DiffCommits(ctx, name, base, head, domain.OneFileDiffLimits(*i)) | |
| 627 | + | if err != nil { | |
| 628 | + | return ports.MergeRequestDetail{}, err | |
| 629 | + | } | |
| 630 | + | if *i < len(one.Files) { | |
| 631 | + | d.Diff.Files[*i] = one.Files[*i] | |
| 632 | + | } | |
| 633 | + | } | |
| 624 | 634 | return d, nil | |
| 625 | 635 | } | |
| 626 | 636 | ||
| @@ -621,6 +621,16 @@ func (m *MergeRequests) Get(ctx context.Context, p domain.Principal, name domain | |||||
| 621 | if d.Stats, d.Diff, err = m.git.DiffCommits(ctx, name, base, head, limits); err != nil { | 621 | if d.Stats, d.Diff, err = m.git.DiffCommits(ctx, name, base, head, limits); err != nil { | ||
| 622 | return ports.MergeRequestDetail{}, err | 622 | return ports.MergeRequestDetail{}, err | ||
| 623 | } | 623 | } | ||
| 624 | + | // The page of a client without scripts reads one collapsed file in full. | |||
| 625 | + | if i := opts.Load; i != nil && *i >= 0 && *i < len(d.Diff.Files) && d.Diff.Files[*i].Omitted.Collapsed() { | |||
| 626 | + | _, one, err := m.git.DiffCommits(ctx, name, base, head, domain.OneFileDiffLimits(*i)) | |||
| 627 | + | if err != nil { | |||
| 628 | + | return ports.MergeRequestDetail{}, err | |||
| 629 | + | } | |||
| 630 | + | if *i < len(one.Files) { | |||
| 631 | + | d.Diff.Files[*i] = one.Files[*i] | |||
| 632 | + | } | |||
| 633 | + | } | |||
| 624 | return d, nil | 634 | return d, nil | ||
| 625 | } | 635 | } | ||
| 626 | 636 | ||||