vole
Clone
Read only, no credentials:
https://rodent.software/git/vole.git
Write over SSH with a registered key:
git clone ssh://git@0.0.0.0:2222/vole.git
A free and open-source forge that lives in your Git repo.
Read a change's file list without its patch, and never cut a diff
f64c77e4950c1759f336d3bc1849ead86e8b8ebf · Keenan Nemetz · 8d ago · parent 11720250
Read a change's file list without its patch, and never cut a diff The review tree, the file count and the landing summary were all parsed out of one `git diff` capped at 512 KiB. When a change carries a large file the cut lands inside it, and every file named after the cut is missing from the tree: reviewing #143, a 546 KB deletion hid the 296 files that came after it while the tab still said "6 changed". The cap existed to stop a page rendering every hunk, but the pages it guarded already render one file at a time. The inventory and the patch are separate reads now. DiffInventory runs `git diff --raw --numstat -z`, which names every file with its status and line counts in one walk of the trees and generates no hunks; the changed-file tree, the counts and the landing summary read that. The Files tab then asks for the one file it shows with DiffPaths, and the commit page keeps its per-file ShowPatchPath. Nothing is capped by bytes: a page is as large as the file it shows, and the inventory is the whole change however large it is. Compare still renders every file of a range at once, and is uncapped too, so a large comparison is one large page. Making that per-file is the next step if it turns out to matter. TestDiffInventoryCountsEveryFile covers the statuses, counts, renames and binary files the inventory has to report; TestRequestFilesListsEveryChangedFile is the regression for the tree that lost its files, and the commit page test now asserts the end of a large patch instead of its cut.
server/web/request_view.go +47−35
| @@ -601,15 +601,14 @@ func (w *Web) RequestLive(rw http.ResponseWriter, r *http.Request) { | |||
| 601 | 601 | // present, head reachable), because a page that says "clean" when it did not | |
| 602 | 602 | // check is worse than one that says nothing. | |
| 603 | 603 | func (w *Web) requestLanding(m *repo.Meta, req *metadata.Request) requestLandingView { | |
| 604 | files, _, alreadyMerged := w.requestDiff(m, req) | ||
| 605 | return w.landingFrom(m, req, files, alreadyMerged) | ||
| 604 | return w.landingFrom(m, req, w.reviewDiff(m, req)) | ||
| 606 | 605 | } | |
| 607 | 606 | ||
| 608 | // landingFrom is requestLanding for a caller that already has the diff: the full | ||
| 609 | // page needs the same files to attach inline comments, so it must not run the | ||
| 610 | // diff twice. | ||
| 611 | func (w *Web) landingFrom(m *repo.Meta, req *metadata.Request, files []diffFile, alreadyMerged bool) requestLandingView { | ||
| 612 | added, removed := diffSummary(files) | ||
| 607 | // landingFrom is requestLanding for a caller that already has the inventory: the | ||
| 608 | // full page needs the same file list for its tab count, so it must not read it | ||
| 609 | // twice. | ||
| 610 | func (w *Web) landingFrom(m *repo.Meta, req *metadata.Request, change reviewChange) requestLandingView { | ||
| 611 | added, removed := diffSummary(change.Files) | ||
| 613 | 612 | var conflicts []string | |
| 614 | 613 | mergeChecked := false | |
| 615 | 614 | if req.Open() && req.Head != "" { | |
| @@ -620,8 +619,8 @@ func (w *Web) landingFrom(m *repo.Meta, req *metadata.Request, files []diffFile, | |||
| 620 | 619 | } | |
| 621 | 620 | } | |
| 622 | 621 | return requestLandingView{ | |
| 623 | FileCount: len(files), Added: added, Removed: removed, | ||
| 624 | AlreadyMerged: alreadyMerged, Conflicts: conflicts, MergeChecked: mergeChecked, | ||
| 622 | FileCount: len(change.Files), Added: added, Removed: removed, | ||
| 623 | AlreadyMerged: change.Merged, Conflicts: conflicts, MergeChecked: mergeChecked, | ||
| 625 | 624 | } | |
| 626 | 625 | } | |
| 627 | 626 | ||
| @@ -640,12 +639,9 @@ func (w *Web) renderRequestPage(rw http.ResponseWriter, r *http.Request, errMsg | |||
| 640 | 639 | return | |
| 641 | 640 | } | |
| 642 | 641 | ||
| 643 | files, _, alreadyMerged := w.requestDiff(m, req) | ||
| 644 | 642 | comments := w.requestComments(m, req) | |
| 645 | attachRequestComments(files, comments) | ||
| 646 | |||
| 647 | 643 | sidebar := w.requestSidebar(r, m, req, m.Name+" · "+req.Title, errMsg). | |
| 648 | withLanding(w.landingFrom(m, req, files, alreadyMerged)) | ||
| 644 | withLanding(w.landingFrom(m, req, w.reviewDiff(m, req))) | ||
| 649 | 645 | if r.URL.Query().Get("merging") == "1" { | |
| 650 | 646 | sidebar.Notice = "Merging when the combined result passes. Use Cancel merge to stop waiting." | |
| 651 | 647 | } | |
| @@ -659,36 +655,53 @@ func (w *Web) renderRequestPage(rw http.ResponseWriter, r *http.Request, errMsg | |||
| 659 | 655 | }, status) | |
| 660 | 656 | } | |
| 661 | 657 | ||
| 662 | // requestDiff computes the diff a change request is reviewed as and answers whether | ||
| 663 | // it is already merged. The diff is cheap for one branch and always matches what the | ||
| 664 | // approver is being asked to approve. | ||
| 658 | // reviewChange is the inventory of what a change request changes: the range git | ||
| 659 | // is asked about, every file it touches with its status and counts, and whether | ||
| 660 | // the head has already landed. It is not a patch. The Files tab asks for the | ||
| 661 | // hunks of the one file it shows, so listing a change costs a walk of the trees | ||
| 662 | // and nothing is cut to fit a page. | ||
| 663 | type reviewChange struct { | ||
| 664 | From, To string | ||
| 665 | Files []diffFile | ||
| 666 | Merged bool | ||
| 667 | } | ||
| 668 | |||
| 669 | // reviewDiff lists what a change request changes and says whether it has already | ||
| 670 | // been merged, which is what the changed-file tree, the line counts and the | ||
| 671 | // landing summary read. | ||
| 665 | 672 | // | |
| 666 | // While the request is open that is the three-dot diff against the branch it targets. | ||
| 667 | // Once the head is in the base, though, that diff is empty by definition — the merge | ||
| 668 | // base of the two is the head itself — so the landing record is used instead: | ||
| 669 | // merged_base is the tip the request landed on, which is the "before" side of the | ||
| 670 | // change that landed. | ||
| 671 | func (w *Web) requestDiff(m *repo.Meta, req *metadata.Request) (files []diffFile, truncated, alreadyMerged bool) { | ||
| 673 | // While the request is open that is the three-dot range against the branch it | ||
| 674 | // targets. Once the head is in the base, though, that diff is empty by definition | ||
| 675 | // — the merge base of the two is the head itself — so the landing record is used | ||
| 676 | // instead: merged_base is the tip the request landed on, which is the "before" | ||
| 677 | // side of the change that landed. | ||
| 678 | func (w *Web) reviewDiff(m *repo.Meta, req *metadata.Request) reviewChange { | ||
| 679 | change := reviewChange{} | ||
| 672 | 680 | if req.Head == "" { | |
| 673 | return nil, false, false | ||
| 681 | return change | ||
| 674 | 682 | } | |
| 675 | from, to := req.Base+"..."+req.Head, "" | ||
| 683 | change.From = req.Base + "..." + req.Head | ||
| 676 | 684 | landed := req.State == "merged" | |
| 677 | 685 | if !landed { | |
| 678 | 686 | landed, _ = m.Git().IsAncestor(req.Head, req.Base) | |
| 679 | 687 | } | |
| 680 | 688 | if landed { | |
| 681 | alreadyMerged = true | ||
| 682 | from, to = mergedDiffRange(m.Git(), req) | ||
| 689 | change.Merged = true | ||
| 690 | change.From, change.To = mergedDiffRange(m.Git(), req) | ||
| 683 | 691 | } | |
| 684 | if from == "" { | ||
| 685 | return nil, false, alreadyMerged | ||
| 692 | if change.From == "" { | ||
| 693 | return change | ||
| 686 | 694 | } | |
| 687 | if patch, cut, err := m.Git().Diff(from, to, diffCap); err == nil { | ||
| 688 | files = parseDiff(patch) | ||
| 689 | truncated = cut | ||
| 690 | } | ||
| 691 | return files, truncated, alreadyMerged | ||
| 695 | changes, err := m.Git().DiffInventory(change.From, change.To) | ||
| 696 | if err != nil { | ||
| 697 | // A change whose files cannot be listed is shown without them rather than | ||
| 698 | // as an error page: the discussion, the landing and the merge controls | ||
| 699 | // still work, and the count says nothing instead of something wrong. | ||
| 700 | w.Logger("request %s: inventory: %v", req.ID, err) | ||
| 701 | return change | ||
| 702 | } | ||
| 703 | change.Files = changedFiles(changes) | ||
| 704 | return change | ||
| 692 | 705 | } | |
| 693 | 706 | ||
| 694 | 707 | // mergedDiffRange is the "before" and "after" of a landing, or two empty strings | |
| @@ -718,8 +731,7 @@ func mergedDiffRange(g *git.Repo, req *metadata.Request) (from, to string) { | |||
| 718 | 731 | // A comment on the old side of a rename names the old path, while the Files tree | |
| 719 | 732 | // keys the change by its new path. | |
| 720 | 733 | func (w *Web) reviewPathFor(m *repo.Meta, req *metadata.Request, path string) string { | |
| 721 | files, _, _ := w.requestDiff(m, req) | ||
| 722 | for _, f := range files { | ||
| 734 | for _, f := range w.reviewDiff(m, req).Files { | ||
| 723 | 735 | if f.NewPath == path || f.OldPath == path { | |
| 724 | 736 | return diffFilePath(f) | |
| 725 | 737 | } | |