appview: pulls: enable merging stacked PRs

+152 -17

appview/db/pulls.go

··· 4 4 "database/sql" 5 5 "fmt" 6 6 "log" 7 + "slices" 7 8 "sort" 8 9 "strings" 9 10 "time" ··· 157 158 } 158 159 } 159 160 return false 161 + } 162 + 163 + func (p *Pull) IsStacked() bool { 164 + return p.StackId != "" 160 165 } 161 166 162 167 func (s PullSubmission) AsDiff(targetBranch string) ([]*gitdiff.File, error) { ··· 327 332 return pullId - 1, err 328 333 } 329 334 330 - func GetPulls(e Execer, repoAt syntax.ATURI, state PullState) ([]*Pull, error) { 335 + func GetPulls(e Execer, filters ...filter) ([]*Pull, error) { 331 336 pulls := make(map[int]*Pull) 332 337 333 - rows, err := e.Query(` 338 + var conditions []string 339 + var args []any 340 + for _, filter := range filters { 341 + conditions = append(conditions, filter.Condition()) 342 + args = append(args, filter.arg) 343 + } 344 + 345 + whereClause := "" 346 + if conditions != nil { 347 + whereClause = " where " + strings.Join(conditions, " and ") 348 + } 349 + 350 + query := fmt.Sprintf(` 334 351 select 335 352 owner_did, 353 + repo_at, 336 354 pull_id, 337 355 created, 338 356 title, ··· 341 359 body, 342 360 rkey, 343 361 source_branch, 344 - source_repo_at 362 + source_repo_at, 363 + stack_id, 364 + change_id, 365 + parent_change_id 345 366 from 346 367 pulls 347 - where 348 - repo_at = ? and state = ?`, repoAt, state) 368 + %s 369 + `, whereClause) 370 + 371 + rows, err := e.Query(query, args...) 349 372 if err != nil { 350 373 return nil, err 351 374 } ··· 354 377 for rows.Next() { 355 378 var pull Pull 356 379 var createdAt string 357 - var sourceBranch, sourceRepoAt sql.NullString 380 + var sourceBranch, sourceRepoAt, stackId, changeId, parentChangeId sql.NullString 358 381 err := rows.Scan( 359 382 &pull.OwnerDid, 383 + &pull.RepoAt, 360 384 &pull.PullId, 361 385 &createdAt, 362 386 &pull.Title, ··· 366 390 &pull.Rkey, 367 391 &sourceBranch, 368 392 &sourceRepoAt, 393 + &stackId, 394 + &changeId, 395 + &parentChangeId, 369 396 ) 370 397 if err != nil { 371 398 return nil, err ··· 390 417 } 391 418 } 392 419 420 + if stackId.Valid { 421 + pull.StackId = stackId.String 422 + } 423 + if changeId.Valid { 424 + pull.ChangeId = changeId.String 425 + } 426 + if parentChangeId.Valid { 427 + pull.ParentChangeId = parentChangeId.String 428 + } 429 + 393 430 pulls[pull.PullId] = &pull 394 431 } 395 432 ··· 397 434 inClause := strings.TrimSuffix(strings.Repeat("?, ", len(pulls)), ", ") 398 435 submissionsQuery := fmt.Sprintf(` 399 436 select 400 - id, pull_id, round_number 437 + id, pull_id, round_number, patch 401 438 from 402 439 pull_submissions 403 440 where 404 - repo_at = ? and pull_id in (%s) 405 - `, inClause) 441 + repo_at in (%s) and pull_id in (%s) 442 + `, inClause, inClause) 406 443 407 - args := make([]any, len(pulls)+1) 408 - args[0] = repoAt.String() 409 - idx := 1 444 + args = make([]any, len(pulls)*2) 445 + idx := 0 446 + for _, p := range pulls { 447 + args[idx] = p.RepoAt 448 + idx += 1 449 + } 410 450 for _, p := range pulls { 411 451 args[idx] = p.PullId 412 452 idx += 1 ··· 423 463 &s.ID, 424 464 &s.PullId, 425 465 &s.RoundNumber, 466 + &s.Patch, 426 467 ) 427 468 if err != nil { 428 469 return nil, err ··· 477 518 return nil, err 478 519 } 479 520 480 - orderedByDate := []*Pull{} 521 + orderedByPullId := []*Pull{} 481 522 for _, p := range pulls { 482 - orderedByDate = append(orderedByDate, p) 523 + orderedByPullId = append(orderedByPullId, p) 483 524 } 484 - sort.Slice(orderedByDate, func(i, j int) bool { 485 - return orderedByDate[i].Created.After(orderedByDate[j].Created) 525 + sort.Slice(orderedByPullId, func(i, j int) bool { 526 + return orderedByPullId[i].PullId > orderedByPullId[j].PullId 486 527 }) 487 528 488 - return orderedByDate, nil 529 + return orderedByPullId, nil 489 530 } 490 531 491 532 func GetPull(e Execer, repoAt syntax.ATURI, pullId int) (*Pull, error) { ··· 854 895 855 896 return count, nil 856 897 } 898 + 899 + type Stack []*Pull 900 + 901 + // change-id parent-change-id 902 + // 903 + // 4 w ,-------- z (TOP) 904 + // 3 z <----',------- y 905 + // 2 y <-----',------ x 906 + // 1 x <------' nil (BOT) 907 + // 908 + // `w` is parent of none, so it is the top of the stack 909 + func GetStack(e Execer, stackId string) (Stack, error) { 910 + unorderedPulls, err := GetPulls(e, Filter("stack_id", stackId)) 911 + if err != nil { 912 + return nil, err 913 + } 914 + // map of parent-change-id to pull 915 + changeIdMap := make(map[string]*Pull, len(unorderedPulls)) 916 + parentMap := make(map[string]*Pull, len(unorderedPulls)) 917 + for _, p := range unorderedPulls { 918 + changeIdMap[p.ChangeId] = p 919 + if p.ParentChangeId != "" { 920 + parentMap[p.ParentChangeId] = p 921 + } 922 + } 923 + 924 + // the top of the stack is the pull that is not a parent of any pull 925 + var topPull *Pull 926 + for _, maybeTop := range unorderedPulls { 927 + if _, ok := parentMap[maybeTop.ChangeId]; !ok { 928 + topPull = maybeTop 929 + break 930 + } 931 + } 932 + 933 + pulls := []*Pull{} 934 + for { 935 + pulls = append(pulls, topPull) 936 + if topPull.ParentChangeId != "" { 937 + if next, ok := changeIdMap[topPull.ParentChangeId]; ok { 938 + topPull = next 939 + } else { 940 + return nil, fmt.Errorf("failed to find parent pull request, stack is malformed") 941 + } 942 + } else { 943 + break 944 + } 945 + } 946 + 947 + return pulls, nil 948 + } 949 + 950 + // position of this pull in the stack 951 + func (stack Stack) Position(pull *Pull) int { 952 + return slices.IndexFunc(stack, func(p *Pull) bool { 953 + return p.ChangeId == pull.ChangeId 954 + }) 955 + } 956 + 957 + // all pulls below this pull (including self) in this stack 958 + // 959 + // nil if this pull does not belong to this stack 960 + func (stack Stack) Below(pull *Pull) Stack { 961 + position := stack.Position(pull) 962 + 963 + if position < 0 { 964 + return nil 965 + } 966 + 967 + return stack[position:] 968 + } 969 + 970 + // all pulls below this pull (excluding self) in this stack 971 + func (stack Stack) StrictlyBelow(pull *Pull) Stack { 972 + below := stack.Below(pull) 973 + 974 + if len(below) > 0 { 975 + return below[1:] 976 + } 977 + 978 + return nil 979 + } 980 + 981 + // the combined format-patches of all the newest submissions in this stack 982 + func (stack Stack) CombinedPatch() string { 983 + // go in reverse order because the bottom of the stack is the last element in the slice 984 + var combined strings.Builder 985 + for idx := range stack { 986 + pull := stack[len(stack)-1-idx] 987 + combined.WriteString(pull.LatestPatch()) 988 + combined.WriteString("\n") 989 + } 990 + return combined.String() 991 + }

+10

appview/state/middleware.go

··· 172 172 173 173 ctx := context.WithValue(r.Context(), "pull", pr) 174 174 175 + if pr.IsStacked() { 176 + stack, err := db.GetStack(s.db, pr.StackId) 177 + if err != nil { 178 + log.Println("failed to get stack", err) 179 + return 180 + } 181 + 182 + ctx = context.WithValue(ctx, "stack", stack) 183 + } 184 + 175 185 next.ServeHTTP(w, r.WithContext(ctx)) 176 186 }) 177 187 }

+137 -26

appview/state/pull.go

··· 46 46 return 47 47 } 48 48 49 + // can be nil if this pull is not stacked 50 + stack := r.Context().Value("stack").(db.Stack) 51 + 49 52 roundNumberStr := chi.URLParam(r, "round") 50 53 roundNumber, err := strconv.Atoi(roundNumberStr) 51 54 if err != nil { ··· 57 60 return 58 61 } 59 62 60 - mergeCheckResponse := s.mergeCheck(f, pull) 63 + mergeCheckResponse := s.mergeCheck(f, pull, stack) 61 64 resubmitResult := pages.Unknown 62 65 if user.Did == pull.OwnerDid { 63 66 resubmitResult = s.resubmitCheck(f, pull) ··· 90 93 return 91 94 } 92 95 96 + // can be nil if this pull is not stacked 97 + stack := r.Context().Value("stack").(db.Stack) 98 + 93 99 totalIdents := 1 94 100 for _, submission := range pull.Submissions { 95 101 totalIdents += len(submission.Comments) ··· 117 123 } 118 124 } 119 125 120 - mergeCheckResponse := s.mergeCheck(f, pull) 126 + mergeCheckResponse := s.mergeCheck(f, pull, stack) 121 127 resubmitResult := pages.Unknown 122 128 if user != nil && user.Did == pull.OwnerDid { 123 129 resubmitResult = s.resubmitCheck(f, pull) ··· 133 139 }) 134 140 } 135 141 136 - func (s *State) mergeCheck(f *FullyResolvedRepo, pull *db.Pull) types.MergeCheckResponse { 142 + func (s *State) mergeCheck(f *FullyResolvedRepo, pull *db.Pull, stack db.Stack) types.MergeCheckResponse { 137 143 if pull.State == db.PullMerged { 138 144 return types.MergeCheckResponse{} 139 145 } ··· 154 160 } 155 161 } 156 162 157 - resp, err := ksClient.MergeCheck([]byte(pull.LatestPatch()), f.OwnerDid(), f.RepoName, pull.TargetBranch) 163 + patch := pull.LatestPatch() 164 + if pull.IsStacked() { 165 + // combine patches of substack 166 + subStack := stack.Below(pull) 167 + 168 + // collect the portion of the stack that is mergeable 169 + var mergeable db.Stack 170 + for _, p := range subStack { 171 + // stop at the first merged PR 172 + if p.State == db.PullMerged { 173 + break 174 + } 175 + 176 + // skip over closed PRs 177 + // 178 + // we will close PRs that are "removed" from a stack 179 + if p.State != db.PullClosed { 180 + mergeable = append(mergeable, p) 181 + } 182 + } 183 + 184 + patch = mergeable.CombinedPatch() 185 + } 186 + 187 + resp, err := ksClient.MergeCheck([]byte(patch), f.OwnerDid(), f.RepoName, pull.TargetBranch) 158 188 if err != nil { 159 189 log.Println("failed to check for mergeability:", err) 160 190 return types.MergeCheckResponse{ ··· 417 447 return 418 448 } 419 449 420 - pulls, err := db.GetPulls(s.db, f.RepoAt, state) 450 + pulls, err := db.GetPulls( 451 + s.db, 452 + db.Filter("repo_at", f.RepoAt), 453 + db.Filter("state", state), 454 + ) 421 455 if err != nil { 422 456 log.Println("failed to get pulls", err) 423 457 s.pages.Notice(w, "pulls", "Failed to load pulls. Try again later.") ··· 1009 1043 1010 1044 // TODO: can we just use a format-patch string here? 1011 1045 initialSubmission := db.PullSubmission{ 1012 - Patch: fp.Patch(), 1046 + Patch: fp.Raw, 1013 1047 SourceRev: sourceRev, 1014 1048 } 1015 1049 err = db.NewPull(tx, &db.Pull{ ··· 1038 1072 Title: title, 1039 1073 TargetRepo: string(f.RepoAt), 1040 1074 TargetBranch: targetBranch, 1041 - Patch: fp.Patch(), 1075 + Patch: fp.Raw, 1042 1076 Source: recordPullSource, 1043 1077 } 1044 1078 writes = append(writes, &comatproto.RepoApplyWrites_Input_Writes_Elem{ ··· 1692 1726 pull, ok := r.Context().Value("pull").(*db.Pull) 1693 1727 if !ok { 1694 1728 log.Println("failed to get pull") 1695 - s.pages.Notice(w, "pull-error", "Failed to edit patch. Try again later.") 1729 + s.pages.Notice(w, "pull-merge-error", "Failed to merge patch. Try again later.") 1696 1730 return 1697 1731 } 1698 1732 1733 + var pullsToMerge db.Stack 1734 + pullsToMerge = append(pullsToMerge, pull) 1735 + if pull.IsStacked() { 1736 + stack, ok := r.Context().Value("stack").(db.Stack) 1737 + if !ok { 1738 + log.Println("failed to get stack") 1739 + s.pages.Notice(w, "pull-merge-error", "Failed to merge patch. Try again later.") 1740 + return 1741 + } 1742 + 1743 + // combine patches of substack 1744 + subStack := stack.Below(pull) 1745 + 1746 + // collect the portion of the stack that is mergeable 1747 + for _, p := range subStack { 1748 + // stop at the first merged PR 1749 + if p.State == db.PullMerged { 1750 + break 1751 + } 1752 + 1753 + // skip over closed PRs 1754 + // 1755 + // TODO: we need a "deleted" state for such PRs, but without losing discussions 1756 + // we will close PRs that are "removed" from a stack 1757 + if p.State == db.PullClosed { 1758 + continue 1759 + } 1760 + 1761 + pullsToMerge = append(pullsToMerge, p) 1762 + } 1763 + } 1764 + 1765 + patch := pullsToMerge.CombinedPatch() 1766 + 1699 1767 secret, err := db.GetRegistrationKey(s.db, f.Knot) 1700 1768 if err != nil { 1701 1769 log.Printf("no registration key found for domain %s: %s\n", f.Knot, err) ··· 1723 1791 } 1724 1792 1725 1793 // Merge the pull request 1726 - resp, err := ksClient.Merge([]byte(pull.LatestPatch()), f.OwnerDid(), f.RepoName, pull.TargetBranch, pull.Title, pull.Body, ident.Handle.String(), email.Address) 1794 + resp, err := ksClient.Merge([]byte(patch), f.OwnerDid(), f.RepoName, pull.TargetBranch, pull.Title, pull.Body, ident.Handle.String(), email.Address) 1727 1795 if err != nil { 1728 1796 log.Printf("failed to merge pull request: %s", err) 1729 1797 s.pages.Notice(w, "pull-merge-error", "Failed to merge pull request. Try again later.") 1730 1798 return 1731 1799 } 1732 1800 1733 - if resp.StatusCode == http.StatusOK { 1734 - err := db.MergePull(s.db, f.RepoAt, pull.PullId) 1801 + if resp.StatusCode != http.StatusOK { 1802 + log.Printf("knotserver returned non-OK status code for merge: %d", resp.StatusCode) 1803 + s.pages.Notice(w, "pull-merge-error", "Failed to merge pull request. Try again later.") 1804 + return 1805 + } 1806 + 1807 + tx, err := s.db.Begin() 1808 + if err != nil { 1809 + log.Printf("failed to start transcation", err) 1810 + s.pages.Notice(w, "pull-merge-error", "Failed to merge pull request. Try again later.") 1811 + return 1812 + } 1813 + 1814 + for _, p := range pullsToMerge { 1815 + err := db.MergePull(tx, f.RepoAt, p.PullId) 1735 1816 if err != nil { 1736 1817 log.Printf("failed to update pull request status in database: %s", err) 1737 1818 s.pages.Notice(w, "pull-merge-error", "Failed to merge pull request. Try again later.") 1738 1819 return 1739 1820 } 1740 - s.pages.HxLocation(w, fmt.Sprintf("/@%s/%s/pulls/%d", f.OwnerHandle(), f.RepoName, pull.PullId)) 1741 - } else { 1742 - log.Printf("knotserver returned non-OK status code for merge: %d", resp.StatusCode) 1821 + } 1822 + 1823 + err = tx.Commit() 1824 + if err != nil { 1825 + // TODO: this is unsound, we should also revert the merge from the knotserver here 1826 + log.Printf("failed to update pull request status in database: %s", err) 1743 1827 s.pages.Notice(w, "pull-merge-error", "Failed to merge pull request. Try again later.") 1828 + return 1744 1829 } 1830 + 1831 + s.pages.HxLocation(w, fmt.Sprintf("/@%s/%s/pulls/%d", f.OwnerHandle(), f.RepoName, pull.PullId)) 1745 1832 } 1746 1833 1747 1834 func (s *State) ClosePull(w http.ResponseWriter, r *http.Request) { ··· 1779 1866 return 1780 1867 } 1781 1868 1782 - // Close the pull in the database 1783 - err = db.ClosePull(tx, f.RepoAt, pull.PullId) 1784 - if err != nil { 1785 - log.Println("failed to close pull", err) 1786 - s.pages.Notice(w, "pull-close", "Failed to close pull.") 1787 - return 1869 + var pullsToClose []*db.Pull 1870 + pullsToClose = append(pullsToClose, pull) 1871 + 1872 + // if this PR is stacked, then we want to close all PRs below this one on the stack 1873 + if pull.IsStacked() { 1874 + stack := r.Context().Value("stack").(db.Stack) 1875 + subStack := stack.StrictlyBelow(pull) 1876 + pullsToClose = append(pullsToClose, subStack...) 1877 + } 1878 + 1879 + for _, p := range pullsToClose { 1880 + // Close the pull in the database 1881 + err = db.ClosePull(tx, f.RepoAt, p.PullId) 1882 + if err != nil { 1883 + log.Println("failed to close pull", err) 1884 + s.pages.Notice(w, "pull-close", "Failed to close pull.") 1885 + return 1886 + } 1788 1887 } 1789 1888 1790 1889 // Commit the transaction ··· 1834 1933 return 1835 1934 } 1836 1935 1837 - // Reopen the pull in the database 1838 - err = db.ReopenPull(tx, f.RepoAt, pull.PullId) 1839 - if err != nil { 1840 - log.Println("failed to reopen pull", err) 1841 - s.pages.Notice(w, "pull-reopen", "Failed to reopen pull.") 1842 - return 1936 + var pullsToReopen []*db.Pull 1937 + pullsToReopen = append(pullsToReopen, pull) 1938 + 1939 + // if this PR is stacked, then we want to reopen all PRs below this one on the stack 1940 + if pull.IsStacked() { 1941 + stack := r.Context().Value("stack").(db.Stack) 1942 + subStack := stack.StrictlyBelow(pull) 1943 + pullsToReopen = append(pullsToReopen, subStack...) 1944 + } 1945 + 1946 + for _, p := range pullsToReopen { 1947 + // Close the pull in the database 1948 + err = db.ReopenPull(tx, f.RepoAt, p.PullId) 1949 + if err != nil { 1950 + log.Println("failed to close pull", err) 1951 + s.pages.Notice(w, "pull-close", "Failed to close pull.") 1952 + return 1953 + } 1843 1954 } 1844 1955 1845 1956 // Commit the transaction

+1 -3

flake.nix

··· 49 49 inherit (gitignore.lib) gitignoreSource; 50 50 in { 51 51 overlays.default = final: prev: let 52 - goModHash = "sha256-TwlPge7vhVGmtNvYkHFFnZjJs2DWPUwPhCSBTCUYCtc="; 52 + goModHash = "sha256-CmBuvv3duQQoc8iTW4244w1rYLGeqMQS+qQ3wwReZZg="; 53 53 buildCmdPackage = name: 54 54 final.buildGoModule { 55 55 pname = name; ··· 156 156 pkgs.websocat 157 157 pkgs.tailwindcss 158 158 pkgs.nixos-shell 159 - pkgs.nodePackages.localtunnel 160 - pkgs.python312Packages.pyngrok 161 159 ]; 162 160 shellHook = '' 163 161 mkdir -p appview/pages/static/{fonts,icons}

+2 -9

patchutil/patchutil.go

··· 13 13 type FormatPatch struct { 14 14 Files []*gitdiff.File 15 15 *gitdiff.PatchHeader 16 - } 17 - 18 - // Extracts just the diff from this format-patch 19 - func (f FormatPatch) Patch() string { 20 - var b strings.Builder 21 - for _, p := range f.Files { 22 - b.WriteString(p.String()) 23 - } 24 - return b.String() 16 + Raw string 25 17 } 26 18 27 19 func (f FormatPatch) ChangeId() (string, error) { ··· 50 42 result = append(result, FormatPatch{ 51 43 Files: files, 52 44 PatchHeader: header, 45 + Raw: patch, 53 46 }) 54 47 } 55 48

Configure Feed

Configure Feed