From 1817eb5bd7eb70812cd76442e1c03279e1c5e5c0 Mon Sep 17 00:00:00 2001 From: oppiliappan Date: Wed, 14 May 2025 13:34:44 +0100 Subject: [PATCH] appview: pulls: refactor ResubmitPull --- appview/db/db.go | 69 +++ appview/db/pulls.go | 99 ++- appview/pages/templates/repo/pulls/pull.html | 25 +- appview/state/pull.go | 613 +++++++++++-------- patchutil/patchutil.go | 60 ++ 5 files changed, 604 insertions(+), 262 deletions(-) diff --git a/appview/db/db.go b/appview/db/db.go index b5339da..0a33de2 100644 --- a/appview/db/db.go +++ b/appview/db/db.go @@ -386,6 +386,75 @@ func Make(dbPath string) (*DB, error) { return err }) + // disable foreign-keys for the next migration + // NOTE: this cannot be done in a transaction, so it is run outside [0] + // + // [0]: https://sqlite.org/pragma.html#pragma_foreign_keys + db.Exec("pragma foreign_keys = off;") + runMigration(db, "recreate-pulls-column-for-stacking-support", func(tx *sql.Tx) error { + _, err := tx.Exec(` + -- disable fk to not delete submissions table + pragma foreign_keys = off; + + create table pulls_new ( + -- identifiers + id integer primary key autoincrement, + pull_id integer not null, + + -- at identifiers + repo_at text not null, + owner_did text not null, + rkey text not null, + + -- content + title text not null, + body text not null, + target_branch text not null, + state integer not null default 0 check (state in (0, 1, 2, 3)), -- closed, open, merged, deleted + + -- source info + source_branch text, + source_repo_at text, + + -- stacking + stack_id text, + change_id text, + parent_change_id text, + + -- meta + created text not null default (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')), + + -- constraints + unique(repo_at, pull_id), + foreign key (repo_at) references repos(at_uri) on delete cascade + ); + + insert into pulls_new ( + id, pull_id, + repo_at, owner_did, rkey, + title, body, target_branch, state, + source_branch, source_repo_at, + created + ) + select + id, pull_id, + repo_at, owner_did, rkey, + title, body, target_branch, state, + source_branch, source_repo_at, + created + FROM pulls; + + drop table pulls; + alter table pulls_new rename to pulls; + + -- reenable fk + pragma foreign_keys = on; + `) + return err + }) + db.Exec("pragma foreign_keys = on;") + +>>>>>>> Conflict 1 of 1 ends return &DB{db}, nil } diff --git a/appview/db/pulls.go b/appview/db/pulls.go index 63fae41..c25841d 100644 --- a/appview/db/pulls.go +++ b/appview/db/pulls.go @@ -22,6 +22,7 @@ const ( PullClosed PullState = iota PullOpen PullMerged + PullDeleted ) func (p PullState) String() string { @@ -32,6 +33,8 @@ func (p PullState) String() string { return "merged" case PullClosed: return "closed" + case PullDeleted: + return "deleted" default: return "closed" } @@ -46,6 +49,9 @@ func (p PullState) IsMerged() bool { func (p PullState) IsClosed() bool { return p == PullClosed } +func (p PullState) IsDelete() bool { + return p == PullDeleted +} type Pull struct { // ids @@ -77,6 +83,26 @@ type Pull struct { Repo *Repo } +func (p Pull) AsRecord() tangled.RepoPull { + var source *tangled.RepoPull_Source + if p.PullSource != nil { + s := p.PullSource.AsRecord() + source = &s + } + + record := tangled.RepoPull{ + Title: p.Title, + Body: &p.Body, + CreatedAt: p.Created.Format(time.RFC3339), + PullId: int64(p.PullId), + TargetRepo: p.RepoAt.String(), + TargetBranch: p.TargetBranch, + Patch: p.LatestPatch(), + Source: source, + } + return record +} + type PullSource struct { Branch string RepoAt *syntax.ATURI @@ -85,6 +111,19 @@ type PullSource struct { Repo *Repo } +func (p PullSource) AsRecord() tangled.RepoPull_Source { + var repoAt *string + if p.RepoAt != nil { + s := p.RepoAt.String() + repoAt = &s + } + record := tangled.RepoPull_Source{ + Branch: p.Branch, + Repo: repoAt, + } + return record +} + type PullSubmission struct { // ids ID int @@ -97,7 +136,7 @@ type PullSubmission struct { RoundNumber int Patch string Comments []PullComment - SourceRev string // include the rev that was used to create this submission: only for branch PRs + SourceRev string // include the rev that was used to create this submission: only for branch/fork PRs // meta Created time.Time @@ -434,7 +473,7 @@ func GetPulls(e Execer, filters ...filter) ([]*Pull, error) { inClause := strings.TrimSuffix(strings.Repeat("?, ", len(pulls)), ", ") submissionsQuery := fmt.Sprintf(` select - id, pull_id, round_number, patch + id, pull_id, round_number, patch, source_rev from pull_submissions where @@ -459,16 +498,22 @@ func GetPulls(e Execer, filters ...filter) ([]*Pull, error) { for submissionsRows.Next() { var s PullSubmission + var sourceRev sql.NullString err := submissionsRows.Scan( &s.ID, &s.PullId, &s.RoundNumber, &s.Patch, + &sourceRev, ) if err != nil { return nil, err } + if sourceRev.Valid { + s.SourceRev = sourceRev.String + } + if p, ok := pulls[s.PullId]; ok { p.Submissions = make([]*PullSubmission, s.RoundNumber+1) p.Submissions[s.RoundNumber] = &s @@ -839,7 +884,13 @@ func NewPullComment(e Execer, comment *PullComment) (int64, error) { } func SetPullState(e Execer, repoAt syntax.ATURI, pullId int, pullState PullState) error { - _, err := e.Exec(`update pulls set state = ? where repo_at = ? and pull_id = ?`, pullState, repoAt, pullId) + _, err := e.Exec( + `update pulls set state = ? where repo_at = ? and pull_id = ? and state <> ?`, + pullState, + repoAt, + pullId, + PullDeleted, // only update state of non-deleted pulls + ) return err } @@ -858,6 +909,11 @@ func MergePull(e Execer, repoAt syntax.ATURI, pullId int) error { return err } +func DeletePull(e Execer, repoAt syntax.ATURI, pullId int) error { + err := SetPullState(e, repoAt, pullId, PullDeleted) + return err +} + func ResubmitPull(e Execer, pull *Pull, newPatch, sourceRev string) error { newRoundNumber := len(pull.Submissions) _, err := e.Exec(` @@ -868,10 +924,33 @@ func ResubmitPull(e Execer, pull *Pull, newPatch, sourceRev string) error { return err } +func SetPullParentChangeId(e Execer, parentChangeId string, filters ...filter) error { + var conditions []string + var args []any + + args = append(args, parentChangeId) + + for _, filter := range filters { + conditions = append(conditions, filter.Condition()) + args = append(args, filter.arg) + } + + whereClause := "" + if conditions != nil { + whereClause = " where " + strings.Join(conditions, " and ") + } + + query := fmt.Sprintf("update pulls set parent_change_id = ? %s", whereClause) + _, err := e.Exec(query, args...) + + return err +} + type PullCount struct { - Open int - Merged int - Closed int + Open int + Merged int + Closed int + Deleted int } func GetPullCount(e Execer, repoAt syntax.ATURI) (PullCount, error) { @@ -879,18 +958,20 @@ func GetPullCount(e Execer, repoAt syntax.ATURI) (PullCount, error) { select count(case when state = ? then 1 end) as open_count, count(case when state = ? then 1 end) as merged_count, - count(case when state = ? then 1 end) as closed_count + count(case when state = ? then 1 end) as closed_count, + count(case when state = ? then 1 end) as deleted_count from pulls where repo_at = ?`, PullOpen, PullMerged, PullClosed, + PullDeleted, repoAt, ) var count PullCount - if err := row.Scan(&count.Open, &count.Merged, &count.Closed); err != nil { - return PullCount{0, 0, 0}, err + if err := row.Scan(&count.Open, &count.Merged, &count.Closed, &count.Deleted); err != nil { + return PullCount{0, 0, 0, 0}, err } return count, nil diff --git a/appview/pages/templates/repo/pulls/pull.html b/appview/pages/templates/repo/pulls/pull.html index 1f3f899..46eadd3 100644 --- a/appview/pages/templates/repo/pulls/pull.html +++ b/appview/pages/templates/repo/pulls/pull.html @@ -207,16 +207,23 @@ {{ i "triangle-alert" "w-4 h-4" }} merge conflicts detected - + {{ end }} {{ else if .MergeCheck }} diff --git a/appview/state/pull.go b/appview/state/pull.go index d0246b7..87b2053 100644 --- a/appview/state/pull.go +++ b/appview/state/pull.go @@ -10,6 +10,7 @@ import ( "net/http" "sort" "strconv" + "strings" "time" "tangled.sh/tangled.sh/core/api/tangled" @@ -21,6 +22,7 @@ import ( "tangled.sh/tangled.sh/core/patchutil" "tangled.sh/tangled.sh/core/types" + "github.com/bluekeyes/go-gitdiff/gitdiff" comatproto "github.com/bluesky-social/indigo/api/atproto" "github.com/bluesky-social/indigo/atproto/syntax" lexutil "github.com/bluesky-social/indigo/lex/util" @@ -47,7 +49,7 @@ func (s *State) PullActions(w http.ResponseWriter, r *http.Request) { } // can be nil if this pull is not stacked - stack := r.Context().Value("stack").(db.Stack) + stack, _ := r.Context().Value("stack").(db.Stack) roundNumberStr := chi.URLParam(r, "round") roundNumber, err := strconv.Atoi(roundNumberStr) @@ -63,7 +65,7 @@ func (s *State) PullActions(w http.ResponseWriter, r *http.Request) { mergeCheckResponse := s.mergeCheck(f, pull, stack) resubmitResult := pages.Unknown if user.Did == pull.OwnerDid { - resubmitResult = s.resubmitCheck(f, pull) + resubmitResult = s.resubmitCheck(f, pull, stack) } s.pages.PullActionsFragment(w, pages.PullActionsParams{ @@ -94,7 +96,7 @@ func (s *State) RepoSinglePull(w http.ResponseWriter, r *http.Request) { } // can be nil if this pull is not stacked - stack := r.Context().Value("stack").(db.Stack) + stack, _ := r.Context().Value("stack").(db.Stack) totalIdents := 1 for _, submission := range pull.Submissions { @@ -126,7 +128,7 @@ func (s *State) RepoSinglePull(w http.ResponseWriter, r *http.Request) { mergeCheckResponse := s.mergeCheck(f, pull, stack) resubmitResult := pages.Unknown if user != nil && user.Did == pull.OwnerDid { - resubmitResult = s.resubmitCheck(f, pull) + resubmitResult = s.resubmitCheck(f, pull, stack) } s.pages.RepoSinglePull(w, pages.RepoSinglePullParams{ @@ -169,14 +171,12 @@ func (s *State) mergeCheck(f *FullyResolvedRepo, pull *db.Pull, stack db.Stack) var mergeable db.Stack for _, p := range subStack { // stop at the first merged PR - if p.State == db.PullMerged { + if p.State == db.PullMerged || p.State == db.PullClosed { break } - // skip over closed PRs - // - // we will close PRs that are "removed" from a stack - if p.State != db.PullClosed { + // skip over deleted PRs + if p.State != db.PullDeleted { mergeable = append(mergeable, p) } } @@ -223,7 +223,7 @@ func (s *State) mergeCheck(f *FullyResolvedRepo, pull *db.Pull, stack db.Stack) return mergeCheckResponse } -func (s *State) resubmitCheck(f *FullyResolvedRepo, pull *db.Pull) pages.ResubmitResult { +func (s *State) resubmitCheck(f *FullyResolvedRepo, pull *db.Pull, stack db.Stack) pages.ResubmitResult { if pull.State == db.PullMerged || pull.PullSource == nil { return pages.Unknown } @@ -273,9 +273,15 @@ func (s *State) resubmitCheck(f *FullyResolvedRepo, pull *db.Pull) pages.Resubmi return pages.Unknown } - latestSubmission := pull.Submissions[pull.LastRoundNumber()] - if latestSubmission.SourceRev != result.Branch.Hash { - fmt.Println(latestSubmission.SourceRev, result.Branch.Hash) + latestSourceRev := pull.Submissions[pull.LastRoundNumber()].SourceRev + + if pull.IsStacked() && stack != nil { + top := stack[0] + latestSourceRev = top.Submissions[top.LastRoundNumber()].SourceRev + } + + if latestSourceRev != result.Branch.Hash { + log.Println(latestSourceRev, result.Branch.Hash) return pages.ShouldResubmit } @@ -650,6 +656,7 @@ func (s *State) NewPull(w http.ResponseWriter, r *http.Request) { RepoInfo: f.RepoInfo(s, user), Branches: result.Branches, }) + case http.MethodPost: title := r.FormValue("title") body := r.FormValue("body") @@ -884,11 +891,10 @@ func (s *State) createPullRequest( r, f, user, - title, body, targetBranch, + targetBranch, patch, sourceRev, pullSource, - recordPullSource, ) return } @@ -988,11 +994,10 @@ func (s *State) createStackedPulLRequest( r *http.Request, f *FullyResolvedRepo, user *oauth.User, - title, body, targetBranch string, + targetBranch string, patch string, sourceRev string, pullSource *db.PullSource, - recordPullSource *tangled.RepoPull_Source, ) { // run some necessary checks for stacked-prs first @@ -1005,97 +1010,49 @@ func (s *State) createStackedPulLRequest( formatPatches, err := patchutil.ExtractPatches(patch) if err != nil { + log.Println("failed to extract patches", err) s.pages.Notice(w, "pull", fmt.Sprintf("Failed to extract patches: %v", err)) return } // must have atleast 1 patch to begin with if len(formatPatches) == 0 { + log.Println("empty patches") s.pages.Notice(w, "pull", "No patches found in the generated format-patch.") return } - tx, err := s.db.BeginTx(r.Context(), nil) + // build a stack out of this patch + stackId := uuid.New() + stack, err := newStack(f, user, targetBranch, patch, pullSource, stackId.String()) if err != nil { - log.Println("failed to start tx") + log.Println("failed to create stack", err) + s.pages.Notice(w, "pull", fmt.Sprintf("Failed to create stack: %v", err)) + return + } + + client, err := s.oauth.AuthorizedClient(r) + if err != nil { + log.Println("failed to get authorized client", err) s.pages.Notice(w, "pull", "Failed to create pull request. Try again later.") return } - defer tx.Rollback() - // create a series of pull requests, and write records from them at once + // apply all record creations at once var writes []*comatproto.RepoApplyWrites_Input_Writes_Elem - - // the stack is identified by a UUID - stackId := uuid.New() - parentChangeId := "" - for _, fp := range formatPatches { - // all patches must have a jj change-id - changeId, err := fp.ChangeId() - if err != nil { - s.pages.Notice(w, "pull", "Stacking is only supported if all patches contain a change-id commit header.") - return - } - - title = fp.Title - body = fp.Body - rkey := appview.TID() - - // TODO: can we just use a format-patch string here? - initialSubmission := db.PullSubmission{ - Patch: fp.Raw, - SourceRev: sourceRev, - } - err = db.NewPull(tx, &db.Pull{ - Title: title, - Body: body, - TargetBranch: targetBranch, - OwnerDid: user.Did, - RepoAt: f.RepoAt, - Rkey: rkey, - Submissions: []*db.PullSubmission{ - &initialSubmission, - }, - PullSource: pullSource, - - StackId: stackId.String(), - ChangeId: changeId, - ParentChangeId: parentChangeId, - }) - if err != nil { - log.Println("failed to create pull request", err) - s.pages.Notice(w, "pull", "Failed to create pull request. Try again later.") - return - } - - record := tangled.RepoPull{ - Title: title, - TargetRepo: string(f.RepoAt), - TargetBranch: targetBranch, - Patch: fp.Raw, - Source: recordPullSource, - } - writes = append(writes, &comatproto.RepoApplyWrites_Input_Writes_Elem{ + for _, p := range stack { + record := p.AsRecord() + write := comatproto.RepoApplyWrites_Input_Writes_Elem{ RepoApplyWrites_Create: &comatproto.RepoApplyWrites_Create{ Collection: tangled.RepoPullNSID, - Rkey: &rkey, + Rkey: &p.Rkey, Value: &lexutil.LexiconTypeDecoder{ Val: &record, }, }, - }) - - parentChangeId = changeId - } - - client, err := s.oauth.AuthorizedClient(r) - if err != nil { - log.Println("failed to get authorized client", err) - s.pages.Notice(w, "pull", "Failed to create pull request. Try again later.") - return + } + writes = append(writes, &write) } - - // apply all record creations at once _, err = client.RepoApplyWrites(r.Context(), &comatproto.RepoApplyWrites_Input{ Repo: user.Did, Writes: writes, @@ -1107,6 +1064,23 @@ func (s *State) createStackedPulLRequest( } // create all pulls at once + tx, err := s.db.BeginTx(r.Context(), nil) + if err != nil { + log.Println("failed to start tx") + s.pages.Notice(w, "pull", "Failed to create pull request. Try again later.") + return + } + defer tx.Rollback() + + for _, p := range stack { + err = db.NewPull(tx, p) + if err != nil { + log.Println("failed to create pull request", err) + s.pages.Notice(w, "pull", "Failed to create pull request. Try again later.") + return + } + } + if err = tx.Commit(); err != nil { log.Println("failed to create pull request", err) s.pages.Notice(w, "pull", "Failed to create pull request. Try again later.") @@ -1371,68 +1345,7 @@ func (s *State) resubmitPatch(w http.ResponseWriter, r *http.Request) { patch := r.FormValue("patch") - if err = validateResubmittedPatch(pull, patch); err != nil { - s.pages.Notice(w, "resubmit-error", err.Error()) - return - } - - tx, err := s.db.BeginTx(r.Context(), nil) - if err != nil { - log.Println("failed to start tx") - s.pages.Notice(w, "resubmit-error", "Failed to create pull request. Try again later.") - return - } - defer tx.Rollback() - - err = db.ResubmitPull(tx, pull, patch, "") - if err != nil { - log.Println("failed to resubmit pull request", err) - s.pages.Notice(w, "resubmit-error", "Failed to resubmit pull request. Try again later.") - return - } - client, err := s.oauth.AuthorizedClient(r) - if err != nil { - log.Println("failed to get authorized client", err) - s.pages.Notice(w, "resubmit-error", "Failed to create pull request. Try again later.") - return - } - - ex, err := client.RepoGetRecord(r.Context(), "", tangled.RepoPullNSID, user.Did, pull.Rkey) - if err != nil { - // failed to get record - s.pages.Notice(w, "resubmit-error", "Failed to update pull, no record found on PDS.") - return - } - - _, err = client.RepoPutRecord(r.Context(), &comatproto.RepoPutRecord_Input{ - Collection: tangled.RepoPullNSID, - Repo: user.Did, - Rkey: pull.Rkey, - SwapRecord: ex.Cid, - Record: &lexutil.LexiconTypeDecoder{ - Val: &tangled.RepoPull{ - Title: pull.Title, - PullId: int64(pull.PullId), - TargetRepo: string(f.RepoAt), - TargetBranch: pull.TargetBranch, - Patch: patch, // new patch - }, - }, - }) - if err != nil { - log.Println("failed to update record", err) - s.pages.Notice(w, "resubmit-error", "Failed to update pull request on the PDS. Try again later.") - return - } - - if err = tx.Commit(); err != nil { - log.Println("failed to commit transaction", err) - s.pages.Notice(w, "resubmit-error", "Failed to resubmit pull.") - return - } - - s.pages.HxLocation(w, fmt.Sprintf("/%s/pulls/%d", f.OwnerSlashRepo(), pull.PullId)) - return + s.resubmitPullHelper(w, r, f, user, pull, patch, "") } func (s *State) resubmitBranch(w http.ResponseWriter, r *http.Request) { @@ -1480,77 +1393,7 @@ func (s *State) resubmitBranch(w http.ResponseWriter, r *http.Request) { sourceRev := comparison.Rev2 patch := comparison.Patch - if err = validateResubmittedPatch(pull, patch); err != nil { - s.pages.Notice(w, "resubmit-error", err.Error()) - return - } - - if sourceRev == pull.Submissions[pull.LastRoundNumber()].SourceRev { - s.pages.Notice(w, "resubmit-error", "This branch has not changed since the last submission.") - return - } - - tx, err := s.db.BeginTx(r.Context(), nil) - if err != nil { - log.Println("failed to start tx") - s.pages.Notice(w, "resubmit-error", "Failed to create pull request. Try again later.") - return - } - defer tx.Rollback() - - err = db.ResubmitPull(tx, pull, patch, sourceRev) - if err != nil { - log.Println("failed to create pull request", err) - s.pages.Notice(w, "resubmit-error", "Failed to create pull request. Try again later.") - return - } - client, err := s.oauth.AuthorizedClient(r) - if err != nil { - log.Println("failed to authorize client") - s.pages.Notice(w, "resubmit-error", "Failed to create pull request. Try again later.") - return - } - - ex, err := client.RepoGetRecord(r.Context(), "", tangled.RepoPullNSID, user.Did, pull.Rkey) - if err != nil { - // failed to get record - s.pages.Notice(w, "resubmit-error", "Failed to update pull, no record found on PDS.") - return - } - - recordPullSource := &tangled.RepoPull_Source{ - Branch: pull.PullSource.Branch, - } - _, err = client.RepoPutRecord(r.Context(), &comatproto.RepoPutRecord_Input{ - Collection: tangled.RepoPullNSID, - Repo: user.Did, - Rkey: pull.Rkey, - SwapRecord: ex.Cid, - Record: &lexutil.LexiconTypeDecoder{ - Val: &tangled.RepoPull{ - Title: pull.Title, - PullId: int64(pull.PullId), - TargetRepo: string(f.RepoAt), - TargetBranch: pull.TargetBranch, - Patch: patch, // new patch - Source: recordPullSource, - }, - }, - }) - if err != nil { - log.Println("failed to update record", err) - s.pages.Notice(w, "resubmit-error", "Failed to update pull request on the PDS. Try again later.") - return - } - - if err = tx.Commit(); err != nil { - log.Println("failed to commit transaction", err) - s.pages.Notice(w, "resubmit-error", "Failed to resubmit pull.") - return - } - - s.pages.HxLocation(w, fmt.Sprintf("/%s/pulls/%d", f.OwnerSlashRepo(), pull.PullId)) - return + s.resubmitPullHelper(w, r, f, user, pull, patch, sourceRev) } func (s *State) resubmitFork(w http.ResponseWriter, r *http.Request) { @@ -1623,16 +1466,54 @@ func (s *State) resubmitFork(w http.ResponseWriter, r *http.Request) { sourceRev := comparison.Rev2 patch := comparison.Patch - if err = validateResubmittedPatch(pull, patch); err != nil { - s.pages.Notice(w, "resubmit-error", err.Error()) + s.resubmitPullHelper(w, r, f, user, pull, patch, sourceRev) +} + +// validate a resubmission against a pull request +func validateResubmittedPatch(pull *db.Pull, patch string) error { + if patch == "" { + return fmt.Errorf("Patch is empty.") + } + + if patch == pull.LatestPatch() { + return fmt.Errorf("Patch is identical to previous submission.") + } + + if !patchutil.IsPatchValid(patch) { + return fmt.Errorf("Invalid patch format. Please provide a valid diff.") + } + + return nil +} + +func (s *State) resubmitPullHelper( + w http.ResponseWriter, + r *http.Request, + f *FullyResolvedRepo, + user *oauth.User, + pull *db.Pull, + patch string, + sourceRev string, +) { + if pull.IsStacked() { + log.Println("resubmitting stacked PR") + s.resubmitStackedPullHelper(w, r, f, user, pull, patch, pull.StackId) return } - if sourceRev == pull.Submissions[pull.LastRoundNumber()].SourceRev { - s.pages.Notice(w, "resubmit-error", "This branch has not changed since the last submission.") + if err := validateResubmittedPatch(pull, patch); err != nil { + s.pages.Notice(w, "resubmit-error", err.Error()) return } + // validate sourceRev if branch/fork based + if pull.IsBranchBased() || pull.IsForkBased() { + if sourceRev == pull.Submissions[pull.LastRoundNumber()].SourceRev { + s.pages.Notice(w, "resubmit-error", "This branch has not changed since the last submission.") + return + } + } + tx, err := s.db.BeginTx(r.Context(), nil) if err != nil { log.Println("failed to start tx") @@ -1649,7 +1530,7 @@ func (s *State) resubmitFork(w http.ResponseWriter, r *http.Request) { } client, err := s.oauth.AuthorizedClient(r) if err != nil { - log.Println("failed to get client") + log.Println("failed to authorize client") s.pages.Notice(w, "resubmit-error", "Failed to create pull request. Try again later.") return } @@ -1661,11 +1542,20 @@ func (s *State) resubmitFork(w http.ResponseWriter, r *http.Request) { return } - repoAt := pull.PullSource.RepoAt.String() - recordPullSource := &tangled.RepoPull_Source{ - Branch: pull.PullSource.Branch, - Repo: &repoAt, + var recordPullSource *tangled.RepoPull_Source + if pull.IsBranchBased() { + recordPullSource = &tangled.RepoPull_Source{ + Branch: pull.PullSource.Branch, + } + } + if pull.IsForkBased() { + repoAt := pull.PullSource.RepoAt.String() + recordPullSource = &tangled.RepoPull_Source{ + Branch: pull.PullSource.Branch, + Repo: &repoAt, + } } + _, err = client.RepoPutRecord(r.Context(), &comatproto.RepoPutRecord_Input{ Collection: tangled.RepoPullNSID, Repo: user.Did, @@ -1698,21 +1588,201 @@ func (s *State) resubmitFork(w http.ResponseWriter, r *http.Request) { return } -// validate a resubmission against a pull request -func validateResubmittedPatch(pull *db.Pull, patch string) error { - if patch == "" { - return fmt.Errorf("Patch is empty.") +func (s *State) resubmitStackedPullHelper( + w http.ResponseWriter, + r *http.Request, + f *FullyResolvedRepo, + user *oauth.User, + pull *db.Pull, + patch string, + stackId string, +) { + targetBranch := pull.TargetBranch + + origStack, _ := r.Context().Value("stack").(db.Stack) + newStack, err := newStack(f, user, targetBranch, patch, pull.PullSource, stackId) + if err != nil { + log.Println("failed to create resubmitted stack", err) + s.pages.Notice(w, "pull-merge-error", "Failed to merge pull request. Try again later.") + return } - if patch == pull.LatestPatch() { - return fmt.Errorf("Patch is identical to previous submission.") + // find the diff between the stacks, first, map them by changeId + origById := make(map[string]*db.Pull) + newById := make(map[string]*db.Pull) + for _, p := range origStack { + origById[p.ChangeId] = p + } + for _, p := range newStack { + newById[p.ChangeId] = p } - if !patchutil.IsPatchValid(patch) { - return fmt.Errorf("Invalid patch format. Please provide a valid diff.") + // commits that got deleted: corresponding pull is closed + // commits that got added: new pull is created + // commits that got updated: corresponding pull is resubmitted & new round begins + // + // for commits that were unchanged: no changes, parent-change-id is updated as necessary + additions := make(map[string]*db.Pull) + deletions := make(map[string]*db.Pull) + unchanged := make(map[string]struct{}) + updated := make(map[string]struct{}) + + // pulls in orignal stack but not in new one + for _, op := range origStack { + if _, ok := newById[op.ChangeId]; !ok { + deletions[op.ChangeId] = op + } } - return nil + // pulls in new stack but not in original one + for _, np := range newStack { + if _, ok := origById[np.ChangeId]; !ok { + additions[np.ChangeId] = np + } + } + + // NOTE: this loop can be written in any of above blocks, + // but is written separately in the interest of simpler code + for _, np := range newStack { + if op, ok := origById[np.ChangeId]; ok { + // pull exists in both stacks + // TODO: can we avoid reparse? + origFiles, _, _ := gitdiff.Parse(strings.NewReader(op.LatestPatch())) + newFiles, _, _ := gitdiff.Parse(strings.NewReader(np.LatestPatch())) + + patchutil.SortPatch(newFiles) + patchutil.SortPatch(origFiles) + + if patchutil.Equal(newFiles, origFiles) { + unchanged[op.ChangeId] = struct{}{} + } else { + updated[op.ChangeId] = struct{}{} + } + } + } + + tx, err := s.db.Begin() + if err != nil { + log.Println("failed to start transaction", err) + s.pages.Notice(w, "pull-resubmit-error", "Failed to resubmit pull request. Try again later.") + return + } + defer tx.Rollback() + + // pds updates to make + var writes []*comatproto.RepoApplyWrites_Input_Writes_Elem + + // deleted pulls are marked as deleted in the DB + for _, p := range deletions { + err := db.DeletePull(tx, p.RepoAt, p.PullId) + if err != nil { + log.Println("failed to delete pull", err, p.PullId) + s.pages.Notice(w, "pull-resubmit-error", "Failed to resubmit pull request. Try again later.") + return + } + writes = append(writes, &comatproto.RepoApplyWrites_Input_Writes_Elem{ + RepoApplyWrites_Delete: &comatproto.RepoApplyWrites_Delete{ + Collection: tangled.RepoPullNSID, + Rkey: p.Rkey, + }, + }) + } + + // new pulls are created + for _, p := range additions { + err := db.NewPull(tx, p) + if err != nil { + log.Println("failed to create pull", err, p.PullId) + s.pages.Notice(w, "pull-resubmit-error", "Failed to resubmit pull request. Try again later.") + return + } + + record := p.AsRecord() + writes = append(writes, &comatproto.RepoApplyWrites_Input_Writes_Elem{ + RepoApplyWrites_Create: &comatproto.RepoApplyWrites_Create{ + Collection: tangled.RepoPullNSID, + Rkey: &p.Rkey, + Value: &lexutil.LexiconTypeDecoder{ + Val: &record, + }, + }, + }) + } + + // updated pulls are, well, updated; to start a new round + for id := range updated { + op, _ := origById[id] + np, _ := newById[id] + + submission := np.Submissions[np.LastRoundNumber()] + + // resubmit the old pull + err := db.ResubmitPull(tx, op, submission.Patch, submission.SourceRev) + + if err != nil { + log.Println("failed to update pull", err, op.PullId) + s.pages.Notice(w, "pull-resubmit-error", "Failed to resubmit pull request. Try again later.") + return + } + + record := op.AsRecord() + record.Patch = submission.Patch + + writes = append(writes, &comatproto.RepoApplyWrites_Input_Writes_Elem{ + RepoApplyWrites_Update: &comatproto.RepoApplyWrites_Update{ + Collection: tangled.RepoPullNSID, + Rkey: op.Rkey, + Value: &lexutil.LexiconTypeDecoder{ + Val: &record, + }, + }, + }) + } + + // update parent-change-id relations for the entire stack + for _, p := range newStack { + err := db.SetPullParentChangeId( + tx, + p.ParentChangeId, + // these should be enough filters to be unique per-stack + db.Filter("repo_at", p.RepoAt.String()), + db.Filter("owner_did", p.OwnerDid), + db.Filter("change_id", p.ChangeId), + ) + + if err != nil { + log.Println("failed to update pull", err, p.PullId) + s.pages.Notice(w, "pull-resubmit-error", "Failed to resubmit pull request. Try again later.") + return + } + } + + err = tx.Commit() + if err != nil { + log.Println("failed to resubmit pull", err) + s.pages.Notice(w, "pull-resubmit-error", "Failed to resubmit pull request. Try again later.") + return + } + + client, err := s.oauth.AuthorizedClient(r) + if err != nil { + log.Println("failed to authorize client") + s.pages.Notice(w, "resubmit-error", "Failed to create pull request. Try again later.") + return + } + + _, err = client.RepoApplyWrites(r.Context(), &comatproto.RepoApplyWrites_Input{ + Repo: user.Did, + Writes: writes, + }) + if err != nil { + log.Println("failed to create stacked pull request", err) + s.pages.Notice(w, "pull", "Failed to create stacked pull request. Try again later.") + return + } + + s.pages.HxLocation(w, fmt.Sprintf("/%s/pulls/%d", f.OwnerSlashRepo(), pull.PullId)) + return } func (s *State) MergePull(w http.ResponseWriter, r *http.Request) { @@ -1745,16 +1815,13 @@ func (s *State) MergePull(w http.ResponseWriter, r *http.Request) { // collect the portion of the stack that is mergeable for _, p := range subStack { - // stop at the first merged PR - if p.State == db.PullMerged { + // stop at the first merged/closed PR + if p.State == db.PullMerged || p.State == db.PullClosed { break } - // skip over closed PRs - // - // TODO: we need a "deleted" state for such PRs, but without losing discussions - // we will close PRs that are "removed" from a stack - if p.State == db.PullClosed { + // skip over deleted PRs + if p.State == db.PullDeleted { continue } @@ -1806,10 +1873,11 @@ func (s *State) MergePull(w http.ResponseWriter, r *http.Request) { tx, err := s.db.Begin() if err != nil { - log.Printf("failed to start transcation", err) + log.Println("failed to start transcation", err) s.pages.Notice(w, "pull-merge-error", "Failed to merge pull request. Try again later.") return } + defer tx.Rollback() for _, p := range pullsToMerge { err := db.MergePull(tx, f.RepoAt, p.PullId) @@ -1865,6 +1933,7 @@ func (s *State) ClosePull(w http.ResponseWriter, r *http.Request) { s.pages.Notice(w, "pull-close", "Failed to close pull.") return } + defer tx.Rollback() var pullsToClose []*db.Pull pullsToClose = append(pullsToClose, pull) @@ -1932,6 +2001,7 @@ func (s *State) ReopenPull(w http.ResponseWriter, r *http.Request) { s.pages.Notice(w, "pull-reopen", "Failed to reopen pull.") return } + defer tx.Rollback() var pullsToReopen []*db.Pull pullsToReopen = append(pullsToReopen, pull) @@ -1963,3 +2033,58 @@ func (s *State) ReopenPull(w http.ResponseWriter, r *http.Request) { s.pages.HxLocation(w, fmt.Sprintf("/%s/pulls/%d", f.OwnerSlashRepo(), pull.PullId)) return } + +func newStack(f *FullyResolvedRepo, user *oauth.User, targetBranch, patch string, pullSource *db.PullSource, stackId string) (db.Stack, error) { + formatPatches, err := patchutil.ExtractPatches(patch) + if err != nil { + return nil, fmt.Errorf("Failed to extract patches: %v", err) + } + + // must have atleast 1 patch to begin with + if len(formatPatches) == 0 { + return nil, fmt.Errorf("No patches found in the generated format-patch.") + } + + // the stack is identified by a UUID + var stack db.Stack + parentChangeId := "" + for _, fp := range formatPatches { + // all patches must have a jj change-id + changeId, err := fp.ChangeId() + if err != nil { + return nil, fmt.Errorf("Stacking is only supported if all patches contain a change-id commit header.") + } + + title := fp.Title + body := fp.Body + rkey := appview.TID() + + initialSubmission := db.PullSubmission{ + Patch: fp.Raw, + SourceRev: fp.SHA, + } + pull := db.Pull{ + Title: title, + Body: body, + TargetBranch: targetBranch, + OwnerDid: user.Did, + RepoAt: f.RepoAt, + Rkey: rkey, + Submissions: []*db.PullSubmission{ + &initialSubmission, + }, + PullSource: pullSource, + Created: time.Now(), + + StackId: stackId, + ChangeId: changeId, + ParentChangeId: parentChangeId, + } + + stack = append(stack, &pull) + + parentChangeId = changeId + } + + return stack, nil +} diff --git a/patchutil/patchutil.go b/patchutil/patchutil.go index 2343acf..62fcc26 100644 --- a/patchutil/patchutil.go +++ b/patchutil/patchutil.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "regexp" + "slices" "strings" "github.com/bluekeyes/go-gitdiff/gitdiff" @@ -203,3 +204,62 @@ func Unified(oldText, oldFile, newText, newFile string) (string, error) { return string(output), nil } + +// are two patches identical +func Equal(a, b []*gitdiff.File) bool { + return slices.EqualFunc(a, b, func(x, y *gitdiff.File) bool { + // same pointer + if x == y { + return true + } + if x == nil || y == nil { + return x == y + } + + // compare file metadata + if x.OldName != y.OldName || x.NewName != y.NewName { + return false + } + if x.OldMode != y.OldMode || x.NewMode != y.NewMode { + return false + } + if x.IsNew != y.IsNew || x.IsDelete != y.IsDelete || x.IsCopy != y.IsCopy || x.IsRename != y.IsRename { + return false + } + + if len(x.TextFragments) != len(y.TextFragments) { + return false + } + + for i, xFrag := range x.TextFragments { + yFrag := y.TextFragments[i] + + // Compare fragment headers + if xFrag.OldPosition != yFrag.OldPosition || xFrag.OldLines != yFrag.OldLines || + xFrag.NewPosition != yFrag.NewPosition || xFrag.NewLines != yFrag.NewLines { + return false + } + + // Compare fragment changes + if len(xFrag.Lines) != len(yFrag.Lines) { + return false + } + + for j, xLine := range xFrag.Lines { + yLine := yFrag.Lines[j] + if xLine.Op != yLine.Op || xLine.Line != yLine.Line { + return false + } + } + } + + return true + }) +} + +// sort patch files in alphabetical order +func SortPatch(patch []*gitdiff.File) { + slices.SortFunc(patch, func(a, b *gitdiff.File) int { + return strings.Compare(bestName(a), bestName(b)) + }) +} -- 2.51.2