From 9a925efef6a0e6dfcd2d4317b4a1eee8752928b8 Mon Sep 17 00:00:00 2001 From: Seongmin Lee Date: Wed, 1 Jul 2026 03:05:36 +0900 Subject: [PATCH] appview: ref based PR Signed-off-by: Seongmin Lee --- appview/db/db.go | 42 + appview/db/entity_state_test.go | 17 +- appview/db/focus.go | 7 +- appview/db/notifications.go | 12 +- appview/db/profile.go | 2 +- appview/db/pulls.go | 792 +++++------------- appview/indexer/pulls/indexer.go | 29 +- appview/ingester.go | 169 ++-- appview/labels/labels.go | 6 +- appview/middleware/middleware.go | 44 - .../migration/backfill_entity_state_test.go | 6 +- appview/models/entity_state.go | 19 + appview/models/pull.go | 535 ++++-------- appview/notify/db/db.go | 5 +- appview/notify/db/db_test.go | 4 +- appview/notify/posthog/notifier.go | 18 +- appview/notify/webhook/notifier.go | 26 +- appview/oauth/scopes.go | 1 + appview/pages/compose_parse_test.go | 324 ------- appview/pages/funcmap.go | 16 +- appview/pages/pages.go | 334 ++++++-- .../fragments/line-quote-button.html | 56 +- appview/pages/templates/layouts/base.html | 2 +- .../pages/templates/repo/fragments/diff.html | 6 +- .../repo/pulls/fragments/composediff.html | 84 ++ .../templates/repo/pulls/fragments/diff.html | 334 ++++++++ .../repo/pulls/fragments/diffFile.html | 18 + .../repo/pulls/fragments/diffSettings.html | 29 + .../repo/pulls/fragments/pullActions.html | 35 +- .../pulls/fragments/pullCompareBranches.html | 2 +- .../pulls/fragments/pullCompareForks.html | 2 +- .../fragments/pullCompareForksBranches.html | 2 +- .../repo/pulls/fragments/pullComposeHost.html | 10 +- .../repo/pulls/fragments/pullPatchUpload.html | 2 +- .../repo/pulls/fragments/pullResubmit.html | 4 +- .../repo/pulls/fragments/pullStepDetails.html | 1 + .../repo/pulls/fragments/pullStepReview.html | 418 ++------- .../repo/pulls/fragments/pullStepSource.html | 42 +- appview/pages/templates/repo/pulls/pull.html | 629 -------------- appview/pages/templates/repo/pulls/pulls.html | 51 +- .../pages/templates/repo/pulls/single.html | 652 ++++++++++++++ appview/pages/url.go | 4 +- appview/pulls/actions.go | 160 ++++ appview/pulls/compose.go | 516 +++++------- appview/pulls/compose_helpers_test.go | 463 ---------- appview/pulls/create.go | 545 +++--------- appview/pulls/diff.go | 168 ++++ appview/pulls/diff_helpers.go | 380 +++++++++ appview/pulls/edit.go | 16 +- appview/pulls/interdiff.go | 120 +++ appview/pulls/labels.go | 127 ++- appview/pulls/lifecycle.go | 57 +- appview/pulls/list.go | 80 +- appview/pulls/merge.go | 129 ++- appview/pulls/middleware.go | 45 + appview/pulls/opengraph.go | 28 +- appview/pulls/pull2.go | 280 ------- appview/pulls/pulls.go | 45 +- appview/pulls/resubmit.go | 641 ++------------ appview/pulls/resubmit_check_test.go | 73 +- appview/pulls/router.go | 33 +- appview/pulls/single.go | 729 ++++++++-------- appview/pulls/state.go | 32 +- appview/pulls/trigger_ci.go | 53 +- appview/repo/feed.go | 22 +- appview/state/profile.go | 4 +- appview/timeline/timeline.go | 7 +- cmd/interdiff/main.go | 38 - input.css | 53 ++ patchutil/interdiff.go | 325 ------- patchutil/patchutil_test.go | 9 - spindle/tapclient.go | 57 +- types/commit.go | 7 + 73 files changed, 4050 insertions(+), 5983 deletions(-) delete mode 100644 appview/pages/compose_parse_test.go create mode 100644 appview/pages/templates/repo/pulls/fragments/composediff.html create mode 100644 appview/pages/templates/repo/pulls/fragments/diff.html create mode 100644 appview/pages/templates/repo/pulls/fragments/diffFile.html create mode 100644 appview/pages/templates/repo/pulls/fragments/diffSettings.html delete mode 100644 appview/pages/templates/repo/pulls/pull.html create mode 100644 appview/pages/templates/repo/pulls/single.html create mode 100644 appview/pulls/actions.go delete mode 100644 appview/pulls/compose_helpers_test.go create mode 100644 appview/pulls/diff.go create mode 100644 appview/pulls/diff_helpers.go create mode 100644 appview/pulls/interdiff.go create mode 100644 appview/pulls/middleware.go delete mode 100644 appview/pulls/pull2.go delete mode 100644 cmd/interdiff/main.go delete mode 100644 patchutil/interdiff.go diff --git a/appview/db/db.go b/appview/db/db.go index b344e09f..96e0f7f6 100644 --- a/appview/db/db.go +++ b/appview/db/db.go @@ -2501,6 +2501,48 @@ func Make(ctx context.Context, dbPath string) (*DB, error) { return err }) + orm.RunMigration(conn, logger, "ref-based-pr", func(tx *sql.Tx) error { + _, err := tx.Exec(` + ALTER TABLE pulls ADD COLUMN cid TEXT NOT NULL DEFAULT ''; + + -- TODO(boltless): set source_repo_did to '' + + CREATE TABLE pull_versions ( + pull_at TEXT NOT NULL, + id INTEGER NOT NULL, -- PR local version id + head TEXT NOT NULL, -- head commit ID + base TEXT NOT NULL, -- base commit ID (used on interdiff) + created TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')), + legacy INTEGER NOT NULL DEFAULT 0, -- + + UNIQUE(pull_at, id), + FOREIGN KEY (pull_at) REFERENCES pulls(at_uri) ON DELETE CASCADE + ); + + -- set source_repo_did to repo_did for all branch based PRs + UPDATE pulls + SET source_repo_did = repo_did + WHERE coalesce(source_branch, '') <> ''; + + INSERT INTO pull_versions ( + pull_at, + id, + head, + base, + created + ) + SELECT + pull_at, + round_number, + coalesce(source_rev, ''), + '', + created + FROM pull_submissions; + -- we keep 'pull_submissions' table just in case. + `) + return err + }) + return &DB{ db, logger, diff --git a/appview/db/entity_state_test.go b/appview/db/entity_state_test.go index d58c439c..8646f69f 100644 --- a/appview/db/entity_state_test.go +++ b/appview/db/entity_state_test.go @@ -39,14 +39,17 @@ func seedPull(t *testing.T, d *DB, repo *models.Repo, did, rkey string) *models. } pull := &models.Pull{ RepoDid: syntax.DID(repo.RepoDid), - OwnerDid: did, - Rkey: rkey, + OwnerDid: syntax.DID(did), + Rkey: syntax.RecordKey(rkey), Title: "title", Body: "body", TargetBranch: "main", State: models.PullOpen, + Versions: []models.PullVersion{ + {Head: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, + }, } - if err := PutPull(tx, pull); err != nil { + if err := PutPull(t.Context(), tx, pull, nil); err != nil { t.Fatalf("PutPull: %v", err) } if err := tx.Commit(); err != nil { @@ -120,11 +123,11 @@ func issueOpen(t *testing.T, d *DB, subject syntax.ATURI) bool { func pullStateOf(t *testing.T, d *DB, subject syntax.ATURI) models.PullState { t.Helper() - pulls, err := GetPulls(d, orm.FilterEq("at_uri", subject)) - if err != nil || len(pulls) != 1 { - t.Fatalf("GetPulls: %v len %d", err, len(pulls)) + pull, err := GetPull(t.Context(), d, orm.FilterEq("at_uri", subject)) + if err != nil { + t.Fatalf("GetPulls: %v", err) } - return pulls[0].State + return pull.State } func issueRec(did, rkey string, subject syntax.ATURI, v models.StateValue, micros int64) models.StateRecord { diff --git a/appview/db/focus.go b/appview/db/focus.go index e4d3f182..4adf2d45 100644 --- a/appview/db/focus.go +++ b/appview/db/focus.go @@ -7,6 +7,7 @@ import ( "strings" "time" + "github.com/bluesky-social/indigo/atproto/syntax" "tangled.org/core/appview/models" ) @@ -170,12 +171,12 @@ func GetNextFocusItem(e Execer, did string) (*models.NotificationWithEntity, err } if pId.Valid { - pull.ID = int(pId.Int64) + pull.ID = pId.Int64 if pOwnerDid.Valid { - pull.OwnerDid = pOwnerDid.String + pull.OwnerDid = syntax.DID(pOwnerDid.String) } if pPullId.Valid { - pull.PullId = int(pPullId.Int64) + pull.PullId = pPullId.Int64 } if pTitle.Valid { pull.Title = pTitle.String diff --git a/appview/db/notifications.go b/appview/db/notifications.go index b55d8f7e..a2d6a721 100644 --- a/appview/db/notifications.go +++ b/appview/db/notifications.go @@ -251,12 +251,12 @@ func GetNotificationsWithEntities(e Execer, page pagination.Page, filters ...orm // populate pull if present if pId.Valid { - pull.ID = int(pId.Int64) + pull.ID = pId.Int64 if pOwnerDid.Valid { - pull.OwnerDid = pOwnerDid.String + pull.OwnerDid = syntax.DID(pOwnerDid.String) } if pPullId.Valid { - pull.PullId = int(pPullId.Int64) + pull.PullId = pPullId.Int64 } if pTitle.Valid { pull.Title = pTitle.String @@ -738,12 +738,12 @@ func GetPendingNotificationsForEmailDigest(e Execer, recipientDid string, olderT } if pId.Valid { - pull.ID = int(pId.Int64) + pull.ID = pId.Int64 if pOwnerDid.Valid { - pull.OwnerDid = pOwnerDid.String + pull.OwnerDid = syntax.DID(pOwnerDid.String) } if pPullId.Valid { - pull.PullId = int(pPullId.Int64) + pull.PullId = pPullId.Int64 } if pTitle.Valid { pull.Title = pTitle.String diff --git a/appview/db/profile.go b/appview/db/profile.go index 8219577f..b897749b 100644 --- a/appview/db/profile.go +++ b/appview/db/profile.go @@ -23,7 +23,7 @@ func MakeProfileTimeline(e Execer, forDid string) (*models.ProfileTimeline, erro now := time.Now() timeframe := fmt.Sprintf("-%d months", TimeframeMonths) - pulls, err := GetPullsByOwnerDid(e, forDid, timeframe) + pulls, err := GetPullsByOwnerDid(e, syntax.DID(forDid), timeframe) if err != nil { return nil, fmt.Errorf("error getting pulls by owner did: %w", err) } diff --git a/appview/db/pulls.go b/appview/db/pulls.go index bd2b28cd..5631fe7e 100644 --- a/appview/db/pulls.go +++ b/appview/db/pulls.go @@ -1,9 +1,8 @@ package db import ( - "cmp" + "context" "database/sql" - "errors" "fmt" "maps" "slices" @@ -12,54 +11,14 @@ import ( "time" "github.com/bluesky-social/indigo/atproto/syntax" - lexutil "github.com/bluesky-social/indigo/lex/util" - "github.com/ipfs/go-cid" "tangled.org/core/appview/models" "tangled.org/core/appview/pagination" "tangled.org/core/orm" - "tangled.org/core/sets" ) -func comparePullSource(existing, new *models.PullSource) bool { - if existing == nil && new == nil { - return true - } - if existing == nil || new == nil { - return false - } - if existing.Branch != new.Branch { - return false - } - if existing.RepoDid == nil && new.RepoDid == nil { - return true - } - if existing.RepoDid == nil || new.RepoDid == nil { - return false - } - return *existing.RepoDid == *new.RepoDid -} - -func compareSubmissions(existing, new []*models.PullSubmission) bool { - if len(existing) != len(new) { - return false - } - for i := range existing { - if existing[i].Blob.Ref.String() != new[i].Blob.Ref.String() { - return false - } - if existing[i].Blob.MimeType != new[i].Blob.MimeType { - return false - } - if existing[i].Blob.Size != new[i].Blob.Size { - return false - } - } - return true -} - -func PutPull(tx *sql.Tx, pull *models.Pull) error { +func PutPull(ctx context.Context, tx *sql.Tx, pull *models.Pull, references []syntax.ATURI) error { // ensure sequence exists - _, err := tx.Exec(` + _, err := tx.ExecContext(ctx, ` insert or ignore into repo_pull_seqs (repo_did, next_pull_id) values (?, 1) `, pull.RepoDid) @@ -67,229 +26,136 @@ func PutPull(tx *sql.Tx, pull *models.Pull) error { return err } - pulls, err := GetPulls( - tx, - orm.FilterEq("owner_did", pull.OwnerDid), - orm.FilterEq("rkey", pull.Rkey), - ) - switch { - case err != nil: + var exists bool + if err := tx.QueryRowContext(ctx, + `select exists (select 1 from pulls where at_uri = ?)`, + pull.AtUri(), + ).Scan(&exists); err != nil { return err - case len(pulls) == 0: - return createNewPull(tx, pull) - case len(pulls) != 1: // should be unreachable - return fmt.Errorf("invalid number of pulls returned: %d", len(pulls)) - default: - existingPull := pulls[0] - if existingPull.State == models.PullMerged { - return nil - } - - dependentOnEqual := (existingPull.DependentOn == nil && pull.DependentOn == nil) || - (existingPull.DependentOn != nil && pull.DependentOn != nil && *existingPull.DependentOn == *pull.DependentOn) - - pullSourceEqual := comparePullSource(existingPull.PullSource, pull.PullSource) - submissionsEqual := compareSubmissions(existingPull.Submissions, pull.Submissions) - - if existingPull.Title == pull.Title && - existingPull.Body == pull.Body && - existingPull.TargetBranch == pull.TargetBranch && - existingPull.RepoDid == pull.RepoDid && - dependentOnEqual && - pullSourceEqual && - submissionsEqual { - return nil - } - - isLonger := len(existingPull.Submissions) < len(pull.Submissions) - if isLonger { - isAppendOnly := compareSubmissions(existingPull.Submissions, pull.Submissions[:len(existingPull.Submissions)]) - if !isAppendOnly { - return fmt.Errorf("the new pull does not treat submissions as append-only") - } - } else if !submissionsEqual { - return fmt.Errorf("the new pull does not treat submissions as append-only") - } - - pull.ID = existingPull.ID - pull.PullId = existingPull.PullId - return updatePull(tx, pull, existingPull) } -} -func createNewPull(tx *sql.Tx, pull *models.Pull) error { - _, err := tx.Exec(` - insert or ignore into repo_pull_seqs (repo_did, next_pull_id) - values (?, 1) - `, pull.RepoDid) - if err != nil { - return err - } - - var nextId int - err = tx.QueryRow(` - update repo_pull_seqs - set next_pull_id = next_pull_id + 1 - where repo_did = ? - returning next_pull_id - 1 - `, pull.RepoDid).Scan(&nextId) - if err != nil { - return err - } - - pull.PullId = nextId - pull.State = models.PullOpen - - var sourceBranch, sourceRepoDid *string - if pull.PullSource != nil { - sourceBranch = &pull.PullSource.Branch - if pull.PullSource.RepoDid != nil { - x := string(*pull.PullSource.RepoDid) - sourceRepoDid = &x + if !exists { + // assign new ID for a PR + if err := tx.QueryRowContext(ctx, + `update repo_pull_seqs + set next_pull_id = next_pull_id + 1 + where repo_did = ? + returning next_pull_id - 1`, + pull.RepoDid, + ).Scan(&pull.PullId); err != nil { + return err } } - result, err := tx.Exec( - ` - insert into pulls ( - repo_did, + result, err := tx.ExecContext(ctx, + `insert into pulls ( owner_did, + rkey, + cid, + repo_did, pull_id, title, - target_branch, body, - rkey, - state, - dependent_on, + target_branch, + source_repo_did, source_branch, - source_repo_did + created, + state ) - values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, - pull.RepoDid, + values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + on conflict(at_uri) do update set + cid = excluded.cid, + repo_did = excluded.repo_did, + title = excluded.title, + body = excluded.body, + target_branch = excluded.target_branch, + source_repo_did = excluded.source_repo_did, + source_branch = excluded.source_branch, + created = excluded.created, + state = excluded.state + where pulls.cid is not excluded.cid`, pull.OwnerDid, + pull.Rkey, + pull.Cid, + pull.RepoDid, pull.PullId, pull.Title, - pull.TargetBranch, pull.Body, - pull.Rkey, + pull.TargetBranch, + pull.SourceRepo, + pull.SourceBranch, + pull.Created.Format(time.RFC3339), pull.State, - pull.DependentOn, - sourceBranch, - sourceRepoDid, ) if err != nil { - return err + return fmt.Errorf("inserting pr: %w", err) } - // Set the database primary key ID id, err := result.LastInsertId() if err != nil { return err } - pull.ID = int(id) - - for i, s := range pull.Submissions { - _, err = tx.Exec(` - insert into pull_submissions ( - pull_at, - round_number, - patch, - combined, - source_rev, - patch_blob_ref, - patch_blob_mime, - patch_blob_size - ) - values (?, ?, ?, ?, ?, ?, ?, ?) - `, - pull.AtUri(), - i, - s.Patch, - s.Combined, - s.SourceRev, - s.Blob.Ref.String(), - s.Blob.MimeType, - s.Blob.Size, - ) - if err != nil { - return err + pull.ID = id + + // delete all existing versions + if _, err := tx.ExecContext(ctx, + `delete from pull_versions where pull_at = ?`, + pull.AtUri(), + ); err != nil { + return fmt.Errorf("deleting old pr versions: %w", err) + } + + // re-create all versions + if len(pull.Versions) > 0 { + pullAt := pull.AtUri() + var sb strings.Builder + sb.WriteString(`insert into pull_versions (pull_at, id, head, base, created) values `) + args := make([]any, 0, len(pull.Versions)*5) + for i, v := range pull.Versions { + if i > 0 { + sb.WriteString(", ") + } + sb.WriteString("(?, ?, ?, ?, ?)") + args = append(args, pullAt, v.ID, v.Head, v.Base, v.Created.Format(time.RFC3339)) + } + if _, err := tx.ExecContext(ctx, sb.String(), args...); err != nil { + return fmt.Errorf("inserting pr versions: %w", err) } } - if err := putReferences(tx, pull.AtUri(), pull.References); err != nil { + // update references when comment is updated + if err := putReferences(tx, pull.AtUri(), references); err != nil { return fmt.Errorf("put reference_links: %w", err) } return nil } -func updatePull(tx *sql.Tx, pull *models.Pull, existingPull *models.Pull) error { - var sourceBranch, sourceRepoDid *string - if pull.PullSource != nil { - sourceBranch = &pull.PullSource.Branch - if pull.PullSource.RepoDid != nil { - x := string(*pull.PullSource.RepoDid) - sourceRepoDid = &x - } - } +func SubmitPullVersion(ctx context.Context, q Execer, pullAt syntax.ATURI, version models.PullVersion) error { + _, err := q.ExecContext(ctx, + `insert into pull_versions (pull_at, id, head, base, created) + values (?, ?, ?, ?, ?)`, + pullAt, + version.ID, + version.Head, + version.Base, + version.Created.Format(time.RFC3339), + ) + return err +} - _, err := tx.Exec(` - update pulls set - title = ?, - body = ?, - target_branch = ?, - dependent_on = ?, - source_branch = ?, - source_repo_did = ? - where owner_did = ? and rkey = ? - `, pull.Title, pull.Body, pull.TargetBranch, pull.DependentOn, sourceBranch, sourceRepoDid, pull.OwnerDid, pull.Rkey) +func GetPull(ctx context.Context, q Execer, filters ...orm.Filter) (*models.Pull, error) { + pulls, err := GetPullsPaginated(ctx, q, pagination.Page{Limit: 1}, filters...) if err != nil { - return err - } - - // insert new submissions (append-only) - for i := len(existingPull.Submissions); i < len(pull.Submissions); i++ { - s := pull.Submissions[i] - _, err = tx.Exec(` - insert into pull_submissions ( - pull_at, - round_number, - patch, - combined, - source_rev, - patch_blob_ref, - patch_blob_mime, - patch_blob_size - ) - values (?, ?, ?, ?, ?, ?, ?, ?) - `, - pull.AtUri(), - i, - s.Patch, - s.Combined, - s.SourceRev, - s.Blob.Ref.String(), - s.Blob.MimeType, - s.Blob.Size, - ) - if err != nil { - return err - } + return nil, err } - - if err := putReferences(tx, pull.AtUri(), pull.References); err != nil { - return fmt.Errorf("put reference_links: %w", err) + if len(pulls) == 0 { + return nil, sql.ErrNoRows } - return nil -} - -func NextPullId(e Execer, repoDid string) (int, error) { - var pullId int - err := e.QueryRow(`select next_pull_id from repo_pull_seqs where repo_did = ?`, repoDid).Scan(&pullId) - return pullId - 1, err + return pulls[0], nil } -func GetPullsPaginated(e Execer, page pagination.Page, filters ...orm.Filter) ([]*models.Pull, error) { +func GetPullsPaginated(ctx context.Context, q Execer, page pagination.Page, filters ...orm.Filter) ([]*models.Pull, error) { pulls := make(map[syntax.ATURI]*models.Pull) var conditions []string @@ -316,17 +182,17 @@ func GetPullsPaginated(e Execer, page pagination.Page, filters ...orm.Filter) ([ select id, owner_did, + rkey, + cid, repo_did, pull_id, - created, title, - state, - target_branch, body, - rkey, - source_branch, + target_branch, source_repo_did, - dependent_on + source_branch, + created, + state from pulls %s @@ -335,7 +201,7 @@ func GetPullsPaginated(e Execer, page pagination.Page, filters ...orm.Filter) ([ %s `, whereClause, pageClause) - rows, err := e.Query(query, args...) + rows, err := q.QueryContext(ctx, query, args...) if err != nil { return nil, err } @@ -344,271 +210,189 @@ func GetPullsPaginated(e Execer, page pagination.Page, filters ...orm.Filter) ([ for rows.Next() { var pull models.Pull var createdAt string - var sourceBranch, sourceRepoDid, dependentOn sql.NullString + var sourceRepo, sourceBranch sql.NullString err := rows.Scan( &pull.ID, &pull.OwnerDid, + &pull.Rkey, + &pull.Cid, &pull.RepoDid, &pull.PullId, - &createdAt, &pull.Title, - &pull.State, - &pull.TargetBranch, &pull.Body, - &pull.Rkey, + &pull.TargetBranch, + &sourceRepo, &sourceBranch, - &sourceRepoDid, - &dependentOn, + &createdAt, + &pull.State, ) if err != nil { - return nil, err + return nil, fmt.Errorf("scanning row: %w", err) } createdTime, err := time.Parse(time.RFC3339, createdAt) if err != nil { - return nil, err + return nil, fmt.Errorf("parsing created: %w", err) } pull.Created = createdTime - if sourceBranch.Valid { - pull.PullSource = &models.PullSource{ - Branch: sourceBranch.String, - } - if sourceRepoDid.Valid { - sourceRepoDidParsed, err := syntax.ParseDID(sourceRepoDid.String) - if err != nil { - return nil, err - } - pull.PullSource.RepoDid = &sourceRepoDidParsed - } + if sourceRepo.Valid { + pull.SourceRepo = syntax.DID(sourceRepo.String) + } else { + // fallback to pull.target.repo + pull.SourceRepo = pull.RepoDid } - if dependentOn.Valid { - x := syntax.ATURI(dependentOn.String) - pull.DependentOn = &x + if sourceBranch.Valid { + pull.SourceBranch = &sourceBranch.String } pulls[pull.AtUri()] = &pull } - - var pullAts []syntax.ATURI - for _, p := range pulls { - pullAts = append(pullAts, p.AtUri()) - } - submissionsMap, err := GetPullSubmissions(e, orm.FilterIn("pull_at", pullAts)) - if err != nil { - return nil, fmt.Errorf("failed to get submissions: %w", err) + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("scanning rows: %w", err) } - for pullAt, submissions := range submissionsMap { - if p, ok := pulls[pullAt]; ok { - p.Submissions = submissions - } - } + pullAts := slices.Collect(maps.Keys(pulls)) - // collect allLabels for each issue - allLabels, err := GetLabels(e, orm.FilterIn("subject", pullAts)) + versionsMap, err := ListVersions(ctx, q, pullAts) if err != nil { - return nil, fmt.Errorf("failed to query labels: %w", err) - } - for pullAt, labels := range allLabels { - if p, ok := pulls[pullAt]; ok { - p.Labels = labels - } + return nil, fmt.Errorf("querying versions: %w", err) } - // build up reverse mappings: p.Repo and p.PullSource.Repo - var repoDids []syntax.DID - for _, p := range pulls { - repoDids = append(repoDids, p.RepoDid) - if p.PullSource != nil && p.PullSource.RepoDid != nil { - repoDids = append(repoDids, *p.PullSource.RepoDid) + for pullAt, p := range pulls { + if versions, ok := versionsMap[pullAt]; ok { + p.Versions = versions + } else { + return nil, fmt.Errorf("find 0 versions for PR %s", pullAt) } } - repos, err := GetRepos(e, orm.FilterIn("repo_did", repoDids)) - if err != nil && !errors.Is(err, sql.ErrNoRows) { - return nil, fmt.Errorf("failed to get repos: %w", err) - } - - repoMap := make(map[syntax.DID]*models.Repo) - for _, r := range repos { - repoMap[syntax.DID(r.RepoDid)] = &r - } + // collect reverse repos + { + repoDids := make([]string, 0, len(pulls)) + for _, issue := range pulls { + repoDids = append(repoDids, string(issue.RepoDid)) + } - for _, p := range pulls { - if repo, ok := repoMap[p.RepoDid]; ok { - p.Repo = repo + repos, err := GetRepos(q, orm.FilterIn("repo_did", repoDids)) + if err != nil { + return nil, fmt.Errorf("failed to build repo mappings: %w", err) + } + repoMap := make(map[syntax.DID]*models.Repo) + for i := range repos { + repoMap[syntax.DID(repos[i].RepoDid)] = &repos[i] } - if p.PullSource != nil && p.PullSource.RepoDid != nil { - if sourceRepo, ok := repoMap[*p.PullSource.RepoDid]; ok { - p.PullSource.Repo = sourceRepo + + for pullAt, p := range pulls { + if r, ok := repoMap[p.RepoDid]; ok { + p.Repo = r + } else { + delete(pulls, pullAt) } } } - allReferences, err := GetReferencesAll(e, orm.FilterIn("from_at", pullAts)) - if err != nil { - return nil, fmt.Errorf("failed to query reference_links: %w", err) - } - for pullAt, references := range allReferences { - if pull, ok := pulls[pullAt]; ok { - pull.References = references + // collect allLabels for each PR + { + allLabels, err := GetLabels(q, orm.FilterIn("subject", pullAts)) + if err != nil { + return nil, fmt.Errorf("failed to query labels: %w", err) + } + for pullAt, labels := range allLabels { + if pull, ok := pulls[pullAt]; ok { + pull.Labels = labels + } } } - orderedByPullId := []*models.Pull{} + orderedById := []*models.Pull{} for _, p := range pulls { - orderedByPullId = append(orderedByPullId, p) + orderedById = append(orderedById, p) } - sort.Slice(orderedByPullId, func(i, j int) bool { - return orderedByPullId[i].PullId > orderedByPullId[j].PullId + sort.Slice(orderedById, func(i, j int) bool { + return orderedById[i].PullId > orderedById[j].PullId }) - return orderedByPullId, nil -} - -func GetPulls(e Execer, filters ...orm.Filter) ([]*models.Pull, error) { - return GetPullsPaginated(e, pagination.Page{}, filters...) -} - -func GetPull(e Execer, filters ...orm.Filter) (*models.Pull, error) { - pulls, err := GetPullsPaginated(e, pagination.Page{Limit: 1}, filters...) - if err != nil { - return nil, err - } - if len(pulls) == 0 { - return nil, sql.ErrNoRows - } - - return pulls[0], nil + return orderedById, nil } // mapping from pull -> pull submissions -func GetPullSubmissions(e Execer, filters ...orm.Filter) (map[syntax.ATURI][]*models.PullSubmission, error) { - var conditions []string - var args []any - for _, filter := range filters { - conditions = append(conditions, filter.Condition()) - args = append(args, filter.Arg()...) - } - - whereClause := "" - if conditions != nil { - whereClause = " where " + strings.Join(conditions, " and ") - } +func ListVersions(ctx context.Context, q Execer, pullAts []syntax.ATURI) (map[syntax.ATURI][]models.PullVersion, error) { + filter := orm.FilterIn("pull_at", pullAts) query := fmt.Sprintf(` select - id, pull_at, - round_number, - patch, - combined, - created, - source_rev, - patch_blob_ref, - patch_blob_mime, - patch_blob_size - from - pull_submissions - %s - order by - round_number asc - `, whereClause) - - rows, err := e.Query(query, args...) + id, + head, + base, + created + from pull_versions + where %s + order by id asc + `, filter.Condition()) + + rows, err := q.QueryContext(ctx, query, filter.Arg()...) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to query: %w", err) } defer rows.Close() - pullMap := make(map[syntax.ATURI][]*models.PullSubmission) + versionsMap := make(map[syntax.ATURI][]models.PullVersion) for rows.Next() { - var submission models.PullSubmission - var submissionCreatedStr string - var submissionSourceRev, submissionCombined sql.Null[string] - var patchBlobRef, patchBlobMime sql.Null[string] - var patchBlobSize sql.Null[int64] + var version models.PullVersion + var pullAt syntax.ATURI + var createdAt string err := rows.Scan( - &submission.ID, - &submission.PullAt, - &submission.RoundNumber, - &submission.Patch, - &submissionCombined, - &submissionCreatedStr, - &submissionSourceRev, - &patchBlobRef, - &patchBlobMime, - &patchBlobSize, + &pullAt, + &version.ID, + &version.Head, + &version.Base, + &createdAt, ) if err != nil { - return nil, err - } - - if t, err := time.Parse(time.RFC3339, submissionCreatedStr); err == nil { - submission.Created = t - } - - if submissionSourceRev.Valid { - submission.SourceRev = submissionSourceRev.V - } - - if submissionCombined.Valid { - submission.Combined = submissionCombined.V + return nil, fmt.Errorf("scanning row: %w", err) } - if patchBlobRef.Valid { - submission.Blob.Ref = lexutil.LexLink(cid.MustParse(patchBlobRef.V)) - } - - if patchBlobMime.Valid { - submission.Blob.MimeType = patchBlobMime.V - } - - if patchBlobSize.Valid { - submission.Blob.Size = patchBlobSize.V + createdTime, err := time.Parse(time.RFC3339, createdAt) + if err != nil { + return nil, fmt.Errorf("parsing created: %w", err) } + version.Created = createdTime - pullMap[submission.PullAt] = append(pullMap[submission.PullAt], &submission) + versionsMap[pullAt] = append(versionsMap[pullAt], version) } - if err := rows.Err(); err != nil { - return nil, err + return nil, fmt.Errorf("scanning rows: %w", err) } - // Get comments for all submissions using GetComments - pullAts := slices.Collect(maps.Keys(pullMap)) - comments, err := GetComments(e, orm.FilterIn("subject_uri", pullAts)) + comments, err := GetComments(q, orm.FilterIn("subject_uri", pullAts)) if err != nil { return nil, fmt.Errorf("failed to get pull comments: %w", err) } for _, comment := range comments { - if comment.PullRoundIdx != nil { - roundIdx := *comment.PullRoundIdx - if submissions, ok := pullMap[syntax.ATURI(comment.Subject.Uri)]; ok { - if roundIdx < len(submissions) { - submission := submissions[roundIdx] - submission.Comments = append(submission.Comments, comment) - } + if comment.PullRoundIdx == nil { + continue + } + versionIdx := *comment.PullRoundIdx + if versions, ok := versionsMap[syntax.ATURI(comment.Subject.Uri)]; ok { + if versionIdx >= len(versions) { + continue } + versions[versionIdx].Comments = append(versions[versionIdx].Comments, comment) } } - // sort each one by round number - for _, s := range pullMap { - slices.SortFunc(s, func(a, b *models.PullSubmission) int { - return cmp.Compare(a.RoundNumber, b.RoundNumber) - }) - } + // TODO: reverse-map version.Comments - return pullMap, nil + return versionsMap, nil } // timeframe here is directly passed into the sql query filter, and any // timeframe in the past should be negative; e.g.: "-3 months" -func GetPullsByOwnerDid(e Execer, did, timeframe string) ([]models.Pull, error) { +func GetPullsByOwnerDid(e Execer, did syntax.DID, timeframe string) ([]models.Pull, error) { var pulls []models.Pull rows, err := e.Query(` @@ -683,7 +467,7 @@ func GetPullsByOwnerDid(e Execer, did, timeframe string) ([]models.Pull, error) } // use with transaction -func SetPullsState(e Execer, pullState models.PullState, filters ...orm.Filter) error { +func setPullsState(e Execer, pullState models.PullState, filters ...orm.Filter) error { var conditions []string var args []any @@ -707,67 +491,19 @@ func SetPullsState(e Execer, pullState models.PullState, filters ...orm.Filter) } func ClosePulls(e Execer, filters ...orm.Filter) error { - return SetPullsState(e, models.PullClosed, filters...) + return setPullsState(e, models.PullClosed, filters...) } func ReopenPulls(e Execer, filters ...orm.Filter) error { - return SetPullsState(e, models.PullOpen, filters...) + return setPullsState(e, models.PullOpen, filters...) } func MergePulls(e Execer, filters ...orm.Filter) error { - return SetPullsState(e, models.PullMerged, filters...) + return setPullsState(e, models.PullMerged, filters...) } func AbandonPulls(e Execer, filters ...orm.Filter) error { - return SetPullsState(e, models.PullAbandoned, filters...) -} - -func ResubmitPull( - e Execer, - pullAt syntax.ATURI, - newRoundNumber int, - newPatch string, - combinedPatch string, - newSourceRev string, - blob *lexutil.LexBlob, -) error { - _, err := e.Exec(` - insert into pull_submissions ( - pull_at, - round_number, - patch, - combined, - source_rev, - patch_blob_ref, - patch_blob_mime, - patch_blob_size - ) - values (?, ?, ?, ?, ?, ?, ?, ?) - `, pullAt, newRoundNumber, newPatch, combinedPatch, newSourceRev, blob.Ref.String(), blob.MimeType, blob.Size) - - return err -} - -func SetDependentOn(e Execer, dependentOn syntax.ATURI, filters ...orm.Filter) error { - var conditions []string - var args []any - - args = append(args, dependentOn) - - 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 dependent_on = ? %s", whereClause) - _, err := e.Exec(query, args...) - - return err + return setPullsState(e, models.PullAbandoned, filters...) } func GetPullCount(e Execer, repoDid string) (models.PullCount, error) { @@ -793,119 +529,3 @@ func GetPullCount(e Execer, repoDid string) (models.PullCount, error) { return count, nil } - -// change-id dependent_on -// -// 4 w ,-------- at_uri(z) (TOP) -// 3 z <----',------- at_uri(y) -// 2 y <-----',------ at_uri(x) -// 1 x <------' nil (BOT) -// -// `w` has no dependents, so it is the top of the stack -// -// this unfortunately does a db query for *each* pull of the stack, -// ideally this would be a recursive query, but in the interest of implementation simplicity, -// we took the less performant route -// -// TODO: make this less bad -func GetStack(e Execer, atUri syntax.ATURI) (models.Stack, error) { - // first get the pull for the given at-uri - pull, err := GetPull(e, orm.FilterEq("at_uri", atUri)) - if err != nil { - return nil, err - } - - // Collect all pulls in the stack by traversing up and down - allPulls := []*models.Pull{pull} - visited := sets.New[syntax.ATURI]() - - // Traverse up to find all dependents - current := pull - for { - dependent, err := GetPull(e, - orm.FilterEq("dependent_on", current.AtUri()), - orm.FilterNotEq("state", models.PullAbandoned), - ) - if err != nil || dependent == nil { - break - } - if visited.Contains(dependent.AtUri()) { - return allPulls, fmt.Errorf("circular dependency detected in stack") - } - allPulls = append(allPulls, dependent) - visited.Insert(dependent.AtUri()) - current = dependent - } - - // Traverse down to find all dependencies - current = pull - for current.DependentOn != nil { - dependency, err := GetPull( - e, - orm.FilterEq("at_uri", current.DependentOn), - orm.FilterNotEq("state", models.PullAbandoned), - ) - - if err != nil { - return allPulls, fmt.Errorf("failed to find parent pull request, stack is malformed, missing PR: %s", current.DependentOn) - } - if visited.Contains(dependency.AtUri()) { - return allPulls, fmt.Errorf("circular dependency detected in stack") - } - allPulls = append(allPulls, dependency) - visited.Insert(dependency.AtUri()) - current = dependency - } - - // sort the list: find the top and build ordered list - atUriMap := make(map[syntax.ATURI]*models.Pull, len(allPulls)) - dependentMap := make(map[syntax.ATURI]*models.Pull, len(allPulls)) - - for _, p := range allPulls { - atUriMap[p.AtUri()] = p - if p.DependentOn != nil { - dependentMap[*p.DependentOn] = p - } - } - - // the top of the stack is the pull that no other pull depends on - var topPull *models.Pull - for _, maybeTop := range allPulls { - if _, ok := dependentMap[maybeTop.AtUri()]; !ok { - topPull = maybeTop - break - } - } - - pulls := []*models.Pull{} - for { - pulls = append(pulls, topPull) - if topPull.DependentOn != nil { - if next, ok := atUriMap[*topPull.DependentOn]; ok { - topPull = next - } else { - return pulls, fmt.Errorf("failed to find parent pull request, stack is malformed") - } - } else { - break - } - } - - return pulls, nil -} - -func GetAbandonedPulls(e Execer, atUri syntax.ATURI) ([]*models.Pull, error) { - stack, err := GetStack(e, atUri) - if err != nil { - return nil, err - } - - var abandoned []*models.Pull - for _, p := range stack { - if p.State == models.PullAbandoned { - abandoned = append(abandoned, p) - } - } - - return abandoned, nil -} diff --git a/appview/indexer/pulls/indexer.go b/appview/indexer/pulls/indexer.go index 34bc78db..7d6aa623 100644 --- a/appview/indexer/pulls/indexer.go +++ b/appview/indexer/pulls/indexer.go @@ -20,6 +20,7 @@ import ( "tangled.org/core/appview/indexer/base36" bleveutil "tangled.org/core/appview/indexer/bleve" "tangled.org/core/appview/models" + "tangled.org/core/appview/pagination" tlog "tangled.org/core/log" ) @@ -164,16 +165,16 @@ func openIndexer(ctx context.Context, path string, version int) (bleve.Index, er func PopulateIndexer(ctx context.Context, ix *Indexer, e db.Execer) error { l := tlog.FromContext(ctx) - - pulls, err := db.GetPulls(e) - if err != nil { - return err - } - count := len(pulls) - err = ix.Index(ctx, pulls...) - if err != nil { - return err - } + count := 0 + err := pagination.IterateAll( + func(page pagination.Page) ([]*models.Pull, error) { + return db.GetPullsPaginated(ctx, e, page) + }, + func(pulls []*models.Pull) error { + count += len(pulls) + return ix.Index(ctx, pulls...) + }, + ) l.Info("pulls indexed", "count", count) return err } @@ -194,13 +195,13 @@ type pullData struct { func makePullData(pull *models.Pull) *pullData { return &pullData{ - ID: int64(pull.ID), - RepoDid: string(pull.RepoDid), - PullID: pull.PullId, + ID: pull.ID, + RepoDid: pull.RepoDid.String(), + PullID: int(pull.PullId), Title: pull.Title, Body: pull.Body, State: pull.State.String(), - AuthorDid: pull.OwnerDid, + AuthorDid: pull.OwnerDid.String(), Labels: pull.Labels.LabelNames(), LabelValues: pull.Labels.LabelNameValues(), } diff --git a/appview/ingester.go b/appview/ingester.go index cf2d6037..40202d32 100644 --- a/appview/ingester.go +++ b/appview/ingester.go @@ -1,18 +1,17 @@ package appview import ( - "bytes" "context" "database/sql" "encoding/json" "errors" "fmt" - "io" "log/slog" "net/http" "net/url" "slices" "strings" + "sync" "time" @@ -1449,8 +1448,9 @@ func (i *Ingester) ingestIssue(ctx context.Context, e *jmodels.Event, l *slog.Lo } func (i *Ingester) ingestPull(ctx context.Context, e *jmodels.Event, l *slog.Logger) error { - did := e.Did - rkey := e.Commit.RKey + did := syntax.DID(e.Did) + rkey := syntax.RecordKey(e.Commit.RKey) + cid := syntax.CID(e.Commit.CID) var err error @@ -1466,85 +1466,94 @@ func (i *Ingester) ingestPull(ctx context.Context, e *jmodels.Event, l *slog.Log return err } - ownerId, err := i.IdResolver.ResolveIdent(ctx, did) - if err != nil { - l.Error("failed to resolve did", "err", err) - return err - } - - // go through and fetch all blobs in parallel - blobs := make([]io.Reader, len(record.Rounds)) - - g, gctx := errgroup.WithContext(ctx) - - for idx, b := range record.Rounds { - g.Go(func() error { - // for some reason, a blob is empty - if b.PatchBlob == nil { - return fmt.Errorf("missing patchBlob in round %d", idx) + versions, err := func() ([]models.PullVersion, error) { + if len(record.Versions) > 0 { + versions := make([]models.PullVersion, len(record.Versions)) + var err error + for i, v := range record.Versions { + versions[i], err = models.PullVersionFromRecord(i, v) + if err != nil { + return nil, fmt.Errorf("versions[%d]: %w", i, err) + } } + return versions, nil + } - ownerPds := ownerId.PDSEndpoint() - url, _ := url.Parse(fmt.Sprintf("%s/xrpc/com.atproto.sync.getBlob", ownerPds)) - q := url.Query() - q.Set("cid", b.PatchBlob.Ref.String()) - q.Set("did", did) - url.RawQuery = q.Encode() - - req, err := http.NewRequestWithContext(gctx, http.MethodGet, url.String(), nil) - if err != nil { - l.Error("failed to create request") - return err - } - req.Header.Set("Content-Type", "application/json") + if len(record.Rounds) == 0 { + return nil, nil + } - resp, err := http.DefaultClient.Do(req) - if err != nil { - l.Error("failed to make request") - return err - } - defer resp.Body.Close() + ownerId, err := i.IdResolver.Directory().LookupDID(ctx, did) + if err != nil { + return nil, fmt.Errorf("failed to resolve did: %w", err) + } - var buf bytes.Buffer - if _, err := io.Copy(&buf, io.LimitReader(resp.Body, 16<<20)); err != nil { - return fmt.Errorf("failed to read blob in round %d: %w", idx, err) - } - blobs[idx] = &buf + // go through and fetch all blobs in parallel + versions := make([]models.PullVersion, len(record.Rounds)) + var mu sync.Mutex + + g, gctx := errgroup.WithContext(ctx) + + for idx, b := range record.Rounds { + g.Go(func() error { + // for some reason, a blob is empty + if b.PatchBlob == nil { + return fmt.Errorf("missing patchBlob in round %d", idx) + } + + ownerPds := ownerId.PDSEndpoint() + url, _ := url.Parse(fmt.Sprintf("%s/xrpc/com.atproto.sync.getBlob", ownerPds)) + q := url.Query() + q.Set("cid", b.PatchBlob.Ref.String()) + q.Set("did", did.String()) + url.RawQuery = q.Encode() + + req, err := http.NewRequestWithContext(gctx, http.MethodGet, url.String(), nil) + if err != nil { + return fmt.Errorf("versions[%d]: failed to create request: %w", idx, err) + } + req.Header.Set("Content-Type", "application/json") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + return fmt.Errorf("versions[%d]: failed to make request: %w", idx, err) + } + defer resp.Body.Close() + + version, err := models.PullVersionFromLegacy(idx, b, resp.Body) + if err != nil { + return fmt.Errorf("versions[%d]: %w", idx, err) + } + + mu.Lock() + versions[idx] = version + mu.Unlock() - return nil - }) - } + return nil + }) + } - if err := g.Wait(); err != nil { - return err + if err := g.Wait(); err != nil { + return nil, err + } + + return versions, nil + }() + if err != nil { + return fmt.Errorf("failed pares pull versions: %w", err) } - pull, err := models.PullFromRecord(did, rkey, record, blobs) + pull, err := models.PullFromRecord(did, rkey, cid, record, versions) if err != nil { return fmt.Errorf("failed to parse pull from record: %w", err) } if err := pull.Validate(); err != nil { return fmt.Errorf("failed to validate pull: %w", err) } - if pull.DependentOn != nil { - if err := func() error { - dependentPull, err := db.GetPull( - i.Db, - orm.FilterEq("dependent_on", pull.DependentOn.String()), - ) - if errors.Is(err, sql.ErrNoRows) { - return nil - } - if err != nil { - return fmt.Errorf("failed to fetch pulls with same dependency: %w", err) - } - if dependentPull.AtUri() == pull.AtUri() { - return nil - } - return fmt.Errorf("another pull already depends on %s, which would form a DAG, this is presently disallowed", pull.DependentOn.String()) - }(); err != nil { - return fmt.Errorf("failed to validate pull stack: %w", err) - } + + var references []syntax.ATURI + if pull.Body != "" { + _, references = i.MentionsResolver.Resolve(ctx, pull.Body) } tx, err := i.Db.BeginTx(ctx, nil) @@ -1554,7 +1563,7 @@ func (i *Ingester) ingestPull(ctx context.Context, e *jmodels.Event, l *slog.Log } defer tx.Rollback() - err = db.PutPull(tx, pull) + err = db.PutPull(ctx, tx, pull, references) if err != nil { l.Error("failed to create pull", "err", err) return err @@ -1626,7 +1635,7 @@ func (i *Ingester) authorizeStateRecord(ctx context.Context, repo *models.Repo, type stateIngestSpec struct { subjectNSID string parse func(did, rkey string, raw json.RawMessage) (models.StateRecord, error) - findSubject func(e db.Execer, subject syntax.ATURI) (repo *models.Repo, authorDid string, found bool, err error) + findSubject func(ctx context.Context, e db.Execer, subject syntax.ATURI) (repo *models.Repo, authorDid string, found bool, err error) put func(tx *sql.Tx, rec models.StateRecord) (syntax.ATURI, error) resolve func(tx *sql.Tx, subject syntax.ATURI) error recompute func(tx *sql.Tx, subject syntax.ATURI) error @@ -1642,7 +1651,7 @@ var issueStateSpec = stateIngestSpec{ } return models.IssueStateFromRecord(did, rkey, record) }, - findSubject: func(e db.Execer, subject syntax.ATURI) (*models.Repo, string, bool, error) { + findSubject: func(ctx context.Context, e db.Execer, subject syntax.ATURI) (*models.Repo, string, bool, error) { issues, err := db.GetIssues(e, orm.FilterEq("at_uri", subject)) if err != nil { return nil, "", false, err @@ -1667,15 +1676,15 @@ var pullStatusSpec = stateIngestSpec{ } return models.PullStatusFromRecord(did, rkey, record) }, - findSubject: func(e db.Execer, subject syntax.ATURI) (*models.Repo, string, bool, error) { - pulls, err := db.GetPulls(e, orm.FilterEq("at_uri", subject)) + findSubject: func(ctx context.Context, e db.Execer, subject syntax.ATURI) (*models.Repo, string, bool, error) { + pull, err := db.GetPull(ctx, e, orm.FilterEq("at_uri", subject)) if err != nil { return nil, "", false, err } - if len(pulls) != 1 || pulls[0].Repo == nil { + if pull.Repo == nil { return nil, "", false, nil } - return pulls[0].Repo, pulls[0].OwnerDid, true, nil + return pull.Repo, pull.OwnerDid.String(), true, nil }, put: db.PutPullStatus, resolve: db.ResolvePullStatus, @@ -1709,7 +1718,7 @@ func (i *Ingester) applyStateRecord(ctx context.Context, did, rkey, nsid string, return fmt.Errorf("state subject is not %s: %s", spec.subjectNSID, rec.Subject) } - repo, authorDid, found, err := spec.findSubject(i.Db, rec.Subject) + repo, authorDid, found, err := spec.findSubject(ctx, i.Db, rec.Subject) if err != nil { return fmt.Errorf("failed to look up state subject: %w", err) } @@ -2123,7 +2132,7 @@ func (i *Ingester) ingestLabelOp(ctx context.Context, e *jmodels.Event, l *slog. return nil } -func (i *Ingester) findLabelSubjectRepo(subject syntax.ATURI) (*models.Repo, bool, error) { +func (i *Ingester) findLabelSubjectRepo(ctx context.Context, subject syntax.ATURI) (*models.Repo, bool, error) { var spec stateIngestSpec switch subject.Collection() { case tangled.RepoIssueNSID: @@ -2133,7 +2142,7 @@ func (i *Ingester) findLabelSubjectRepo(subject syntax.ATURI) (*models.Repo, boo default: return nil, false, fmt.Errorf("unsupported label subject: %s", subject.Collection()) } - repo, _, found, err := spec.findSubject(i.Db, subject) + repo, _, found, err := spec.findSubject(ctx, i.Db, subject) return repo, found, err } @@ -2148,7 +2157,7 @@ func (i *Ingester) applyLabelOpRecord(ctx context.Context, did, rkey string, raw return i.parkStateRecord(ctx, did, rkey, tangled.LabelOpNSID, subject, raw, l) } - repo, found, err := i.findLabelSubjectRepo(subject) + repo, found, err := i.findLabelSubjectRepo(ctx, subject) if err != nil { return err } diff --git a/appview/labels/labels.go b/appview/labels/labels.go index 0a131193..59db106f 100644 --- a/appview/labels/labels.go +++ b/appview/labels/labels.go @@ -282,9 +282,9 @@ func (l *Labels) PerformLabelOp(w http.ResponseWriter, r *http.Request) { } } if subject.Collection() == tangled.RepoPullNSID { - pulls, err := db.GetPulls(l.db, orm.FilterEq("at_uri", subjectUri)) - if err == nil && len(pulls) == 1 { - l.notifier.NewPullLabelOp(r.Context(), syntax.DID(did), pulls[0], validLabelOps) + pull, err := db.GetPull(r.Context(), l.db, orm.FilterEq("at_uri", subjectUri)) + if err == nil { + l.notifier.NewPullLabelOp(r.Context(), syntax.DID(did), pull, validLabelOps) } } diff --git a/appview/middleware/middleware.go b/appview/middleware/middleware.go index 4a1bb890..086247b3 100644 --- a/appview/middleware/middleware.go +++ b/appview/middleware/middleware.go @@ -368,50 +368,6 @@ func (mw Middleware) CanonicalizeRepoURL() middlewareFunc { } } -// middleware that is tacked on top of /{user}/{repo}/pulls/{pull} -func (mw Middleware) ResolvePull() middlewareFunc { - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - l := mw.logger.With("middleware", "ResolvePull") - f, err := mw.repoResolver.Resolve(r) - if err != nil { - l.Error("failed to fully resolve repo", "err", err) - w.WriteHeader(http.StatusNotFound) - mw.pages.ErrorKnot404(w) - return - } - - prId := chi.URLParam(r, "pull") - prIdInt, err := strconv.Atoi(prId) - if err != nil { - l.Error("failed to parse pr id", "err", err) - mw.pages.Error404(w) - return - } - - pr, err := db.GetPull(mw.db, orm.FilterEq("repo_did", f.RepoDid), orm.FilterEq("pull_id", prIdInt)) - if err != nil { - l.Error("failed to get pull and comments", "err", err) - mw.pages.Error404(w) - return - } - - ctx := context.WithValue(r.Context(), "pull", pr) - - stack, err := db.GetStack(mw.db, pr.AtUri()) - if err != nil { - l.Error("failed to get stack", "err", err) - mw.pages.Error404(w) - return - } - - ctx = context.WithValue(ctx, "stack", stack) - - next.ServeHTTP(w, r.WithContext(ctx)) - }) - } -} - // middleware that is tacked on top of /{user}/{repo}/issues/{issue} func (mw Middleware) ResolveIssue(next http.Handler) http.Handler { l := mw.logger.With("middleware", "ResolveIssue") diff --git a/appview/migration/backfill_entity_state_test.go b/appview/migration/backfill_entity_state_test.go index 6848b9c2..1958e680 100644 --- a/appview/migration/backfill_entity_state_test.go +++ b/appview/migration/backfill_entity_state_test.go @@ -214,14 +214,14 @@ func seedPullRow(t *testing.T, d *db.DB, repoDid, author, rkey string) *models.P } pull := &models.Pull{ RepoDid: syntax.DID(repoDid), - OwnerDid: author, - Rkey: rkey, + OwnerDid: syntax.DID(author), + Rkey: syntax.RecordKey(rkey), Title: "title", Body: "body", TargetBranch: "main", State: models.PullOpen, } - if err := db.PutPull(tx, pull); err != nil { + if err := db.PutPull(t.Context(), tx, pull, nil); err != nil { t.Fatalf("PutPull: %v", err) } if err := tx.Commit(); err != nil { diff --git a/appview/models/entity_state.go b/appview/models/entity_state.go index ce980ad5..d4bafeab 100644 --- a/appview/models/entity_state.go +++ b/appview/models/entity_state.go @@ -90,6 +90,25 @@ func AsIssueStateRecord(subject syntax.ATURI, value StateValue, createdAt time.T }, nil } +func AsPullStatusRecord(subject syntax.ATURI, value StateValue, createdAt time.Time) (tangled.RepoPullStatus, error) { + var variant string + switch value { + case StateOpen: + variant = tangled.RepoPullStatusOpen + case StateClosed: + variant = tangled.RepoPullStatusClosed + case StateMerged: + variant = tangled.RepoPullStatusMerged + default: + return tangled.RepoPullStatus{}, fmt.Errorf("invalid pull status: %q", value) + } + return tangled.RepoPullStatus{ + Pull: subject.String(), + Status: variant, + CreatedAt: createdAt.UTC().Format(syntax.AtprotoDatetimeLayout), + }, nil +} + func AsPullStatusRecords(subjects []syntax.ATURI, value StateValue, createdAt time.Time) ([]tangled.RepoPullStatus, error) { var variant string switch value { diff --git a/appview/models/pull.go b/appview/models/pull.go index daaab828..5019786f 100644 --- a/appview/models/pull.go +++ b/appview/models/pull.go @@ -3,9 +3,10 @@ package models import ( "bytes" "compress/gzip" + "encoding/hex" "fmt" "io" - "log" + "maps" "slices" "strings" "time" @@ -13,10 +14,8 @@ import ( "tangled.org/core/api/tangled" "tangled.org/core/appview/pages/markup/sanitizer" "tangled.org/core/patchutil" - "tangled.org/core/types" "github.com/bluesky-social/indigo/atproto/syntax" - lexutil "github.com/bluesky-social/indigo/lex/util" ) type PullState int @@ -57,115 +56,97 @@ func (p PullState) IsAbandoned() bool { } type Pull struct { - // ids - ID int - PullId int - - // at ids + ID int64 // appview-local PR id. Used for quick referencing + OwnerDid syntax.DID + Rkey syntax.RecordKey + Cid syntax.CID RepoDid syntax.DID - OwnerDid string - Rkey string + PullId int64 - // content Title string Body string TargetBranch string - State PullState - Submissions []*PullSubmission - Mentions []syntax.DID - References []syntax.ATURI - - // stacking - DependentOn *syntax.ATURI + SourceRepo syntax.DID + SourceBranch *string + Versions []PullVersion + Created time.Time - // meta - Created time.Time - PullSource *PullSource + State PullState // optionally, populate this when querying for reverse mappings Labels LabelState Repo *Repo } -func (p *Pull) SourceRepoDid() syntax.DID { - if p.PullSource != nil && p.PullSource.RepoDid != nil { - return *p.PullSource.RepoDid - } - return p.RepoDid +func (p *Pull) AtUri() syntax.ATURI { + return syntax.ATURI(fmt.Sprintf("at://%s/%s/%s", p.OwnerDid, tangled.RepoPullNSID, p.Rkey)) } -// NOTE: This method does not include patch blob in returned atproto record -func (p Pull) AsRecord() tangled.RepoPull { - mentions := make([]string, len(p.Mentions)) - for i, did := range p.Mentions { - mentions[i] = string(did) +func (p *Pull) AsRecord() tangled.RepoPull { + sourceRepo := p.SourceRepo.String() + var sourceBranch string + if p.SourceBranch != nil { + sourceBranch = *p.SourceBranch } - references := make([]string, len(p.References)) - for i, uri := range p.References { - references[i] = string(uri) - } - - rounds := make([]*tangled.RepoPull_Round, len(p.Submissions)) - for i, submission := range p.Submissions { - rounds[i] = submission.AsRecord() - } - - var dependentOn *string - if p.DependentOn != nil { - x := p.DependentOn.String() - dependentOn = &x + versions := make([]*tangled.RepoPull_Version, len(p.Versions)) + for i, v := range p.Versions { + var base *string + if v.Base != "" { + base = &v.Base + } + versions[i] = &tangled.RepoPull_Version{ + Base: base, + Head: v.Head, + CreatedAt: v.Created.Format(time.RFC3339), + } } - return tangled.RepoPull{ - Title: p.Title, - Body: &p.Body, - Mentions: mentions, - References: references, - CreatedAt: p.Created.Format(time.RFC3339), + Title: p.Title, + Body: &p.Body, Target: &tangled.RepoPull_Target{ - Repo: string(p.RepoDid), + Repo: p.RepoDid.String(), Branch: p.TargetBranch, }, - Rounds: rounds, - Source: p.PullSource.AsRecord(), - DependentOn: dependentOn, + Source: &tangled.RepoPull_Source{ + Repo: &sourceRepo, + Branch: sourceBranch, + }, + Versions: versions, + CreatedAt: p.Created.Format(time.RFC3339), } } -func (pull *Pull) Validate() error { - if len(pull.Submissions) == 0 { - return fmt.Errorf("pull must have at least one submission") +func (p *Pull) Validate() error { + if len(p.Versions) == 0 { + return fmt.Errorf("pull must have at least one version") } - latestSubmission := pull.LatestSubmission() - if latestSubmission == nil { - return fmt.Errorf("pull must have a valid latest submission") + if p.Title == "" { + return fmt.Errorf("pull title is empty (required for non-format-patch pulls)") + } + if st := strings.TrimSpace(sanitizer.SanitizeDescription(p.Title)); st == "" { + return fmt.Errorf("title is empty after HTML sanitization") } - isFormatPatch := patchutil.IsFormatPatch(latestSubmission.Patch) - - // title and body can only be empty if the patch is a format-patch - if !isFormatPatch { - if pull.Title == "" { - return fmt.Errorf("pull title is empty (required for non-format-patch pulls)") - } - - if pull.Body == "" { - return fmt.Errorf("pull body is empty (required for non-format-patch pulls)") - } - - if st := strings.TrimSpace(sanitizer.SanitizeDescription(pull.Title)); st == "" { - return fmt.Errorf("title is empty after HTML sanitization") + for i, version := range p.Versions { + if err := version.Validate(); err != nil { + return fmt.Errorf("versions[%d]: %w", i, err) } + } + return nil +} - if sb := strings.TrimSpace(sanitizer.SanitizeDefault(pull.Body)); sb == "" { - return fmt.Errorf("body is empty after HTML sanitization") - } +func (v *PullVersion) Validate() error { + if v.Base != "" && !IsHash(v.Base) { + return fmt.Errorf("invalid base commit id: %q", v.Base) + } + if !IsHash(v.Head) { + return fmt.Errorf("invalid head commit id: %q", v.Head) } return nil } -func PullFromRecord(did, rkey string, record tangled.RepoPull, blobs []io.Reader) (*Pull, error) { +func PullFromRecord(did syntax.DID, rkey syntax.RecordKey, cid syntax.CID, record tangled.RepoPull, versions []PullVersion) (*Pull, error) { created, err := time.Parse(time.RFC3339, record.CreatedAt) if err != nil { return nil, fmt.Errorf("invalid createdAt: %w", err) @@ -176,12 +157,12 @@ func PullFromRecord(did, rkey string, record tangled.RepoPull, blobs []io.Reader body = *record.Body } - var mentions []syntax.DID - for _, m := range record.Mentions { - if did, err := syntax.ParseDID(m); err == nil { - mentions = append(mentions, did) - } - } + // var mentions []syntax.DID + // for _, m := range record.Mentions { + // if did, err := syntax.ParseDID(m); err == nil { + // mentions = append(mentions, did) + // } + // } var targetRepoDid syntax.DID var targetBranch string @@ -194,90 +175,89 @@ func PullFromRecord(did, rkey string, record tangled.RepoPull, blobs []io.Reader targetBranch = record.Target.Branch } - var pullSource *PullSource + var sourceRepo syntax.DID + var sourceBranch *string if record.Source != nil { - pullSource = &PullSource{ - Branch: record.Source.Branch, - } - if record.Source.Repo != nil { did, err := syntax.ParseDID(*record.Source.Repo) if err != nil { return nil, fmt.Errorf("invalid source.repo did: %w", err) } - pullSource.RepoDid = &did + sourceRepo = did } - } - - var dependentOn *syntax.ATURI - if record.DependentOn != nil { - uri, err := syntax.ParseATURI(*record.DependentOn) - if err != nil { - return nil, fmt.Errorf("invalid dependentOn aturi: %w", err) + if record.Source.Branch != "" { + sourceBranch = new(string) + *sourceBranch = record.Source.Branch } - dependentOn = &uri - } - - var submissions []*PullSubmission - for i, s := range record.Rounds { - var blob io.Reader - if i < len(blobs) { - blob = blobs[i] - } - submission, err := PullSubmissionFromRecord(did, rkey, i, s, blob) - if err != nil { - return nil, fmt.Errorf("invalid pull round at index %d: %w", i, err) - } - submissions = append(submissions, submission) } return &Pull{ - RepoDid: targetRepoDid, - OwnerDid: did, - Rkey: rkey, + ID: -1, // uninitialized + OwnerDid: did, + Rkey: rkey, + Cid: cid, + RepoDid: targetRepoDid, + PullId: 0, // uninitialized + Title: record.Title, Body: body, TargetBranch: targetBranch, - PullSource: pullSource, - State: PullOpen, - Submissions: submissions, + SourceRepo: sourceRepo, + SourceBranch: sourceBranch, + Versions: versions, Created: created, - DependentOn: dependentOn, + State: PullOpen, // default to open }, nil } -func PullSubmissionFromRecord(did, rkey string, roundNumber int, round *tangled.RepoPull_Round, blob io.Reader) (*PullSubmission, error) { - created, err := time.Parse(time.RFC3339, round.CreatedAt) +func PullVersionFromRecord(idx int, record *tangled.RepoPull_Version) (PullVersion, error) { + created, err := time.Parse(time.RFC3339, record.CreatedAt) if err != nil { - return nil, fmt.Errorf("invalid createdAt: %w", err) + return PullVersion{}, fmt.Errorf("invalid createdAt: %w", err) } - var patch, sourceRev string - if blob != nil { - p, err := extractGzip(blob) - if err != nil { - return nil, fmt.Errorf("failed to extract gzip: %w", err) - } - patch = p - if patchutil.IsFormatPatch(p) { - patches, err := patchutil.ExtractPatches(p) - if err != nil { - return nil, fmt.Errorf("failed to extract patches: %w", err) - } + var base string + if record.Base != nil { + base = *record.Base + } + return PullVersion{ + ID: idx, + Base: base, + Head: record.Head, + Created: created, + }, nil +} - for _, part := range patches { - sourceRev = part.SHA - } - } +func PullVersionFromLegacy(idx int, record *tangled.RepoPull_Round, reader io.Reader) (PullVersion, error) { + created, err := time.Parse(time.RFC3339, record.CreatedAt) + if err != nil { + return PullVersion{}, fmt.Errorf("invalid createdAt: %w", err) + } + + patch, err := extractGzip(reader) + if err != nil { + return PullVersion{}, fmt.Errorf("failed to extract gzip: %w", err) + } + if !patchutil.IsFormatPatch(patch) { + return PullVersion{}, fmt.Errorf("only format-patch patch is supported") } - return &PullSubmission{ - PullAt: syntax.ATURI(fmt.Sprintf("at://%s/%s/%s", did, tangled.RepoPullNSID, rkey)), - RoundNumber: roundNumber, - Blob: *round.PatchBlob, - Created: created, - Patch: patch, - SourceRev: sourceRev, + var sourceRev string + patches, err := patchutil.ExtractPatches(patch) + if err != nil { + return PullVersion{}, fmt.Errorf("failed to extract patches: %w", err) + } + for _, part := range patches { + sourceRev = part.SHA + } + if sourceRev == "" { + return PullVersion{}, fmt.Errorf("source rev is missing") + } + return PullVersion{ + ID: idx, + Base: "", + Head: sourceRev, + Created: created, }, nil } @@ -304,263 +284,71 @@ func (s *PullSource) AsRecord() *tangled.RepoPull_Source { } } -type PullSubmission struct { - // ids - ID int - - // at ids - PullAt syntax.ATURI - - // content - RoundNumber int - Blob lexutil.LexBlob - Patch string - Combined string - Comments []Comment - SourceRev string // include the rev that was used to create this submission: only for branch/fork PRs - - // meta +type PullVersion struct { + ID int + Head string // head commit ID + Base string // base commit ID (for combined interdiff) Created time.Time + + // reverse mappings + Comments []Comment } func (p *Pull) TotalComments() int { total := 0 - for _, s := range p.Submissions { + for _, s := range p.Versions { total += len(s.Comments) } return total } -func (p *Pull) LastRoundNumber() int { - return len(p.Submissions) - 1 +func (p *Pull) GetVersion(id int) (PullVersion, bool) { + for _, version := range p.Versions { + if version.ID == id { + return version, true + } + } + return PullVersion{}, false } -func (p *Pull) LatestSubmission() *PullSubmission { - return p.Submissions[p.LastRoundNumber()] +func (p *Pull) LatestVersionNumber() int { + return len(p.Versions) - 1 } -func (p *Pull) LatestPatch() string { - return p.LatestSubmission().Patch +func (p *Pull) LatestVersion() PullVersion { + return p.Versions[p.LatestVersionNumber()] } func (p *Pull) LatestSha() string { - return p.LatestSubmission().SourceRev -} - -func (p *Pull) AtUri() syntax.ATURI { - return syntax.ATURI(fmt.Sprintf("at://%s/%s/%s", p.OwnerDid, tangled.RepoPullNSID, p.Rkey)) -} - -func (p *Pull) IsPatchBased() bool { - return p.PullSource == nil -} - -func (p *Pull) IsBranchBased() bool { - if p.PullSource != nil { - if p.PullSource.RepoDid != nil { - return *p.PullSource.RepoDid == p.RepoDid - } - // no repo specified - return true - } - return false + return p.LatestVersion().Head } func (p *Pull) IsForkBased() bool { - if p.PullSource != nil { - if p.PullSource.RepoDid != nil { - // make sure repos are different - return *p.PullSource.RepoDid != p.RepoDid - } - } - return false + return p.RepoDid != p.SourceRepo } func (p *Pull) Participants() []syntax.DID { - participantSet := make(map[syntax.DID]struct{}) - participants := []syntax.DID{} - - addParticipant := func(did syntax.DID) { - if _, exists := participantSet[did]; !exists { - participantSet[did] = struct{}{} - participants = append(participants, did) - } - } + participants := make(map[syntax.DID]struct{}) - addParticipant(syntax.DID(p.OwnerDid)) + participants[p.OwnerDid] = struct{}{} - for _, s := range p.Submissions { - for _, sp := range s.Participants() { - addParticipant(syntax.DID(sp)) + for _, v := range p.Versions { + for _, sp := range v.Participants() { + participants[sp] = struct{}{} } } - return participants -} - -func (s PullSubmission) IsFormatPatch() bool { - return patchutil.IsFormatPatch(s.Patch) -} - -func (s PullSubmission) AsFormatPatch() []types.FormatPatch { - patches, err := patchutil.ExtractPatches(s.Patch) - if err != nil { - log.Println("error extracting patches from submission:", err) - return []types.FormatPatch{} - } - - return patches -} - -// empty if invalid, not otherwise -func (s PullSubmission) ChangeId() string { - patches := s.AsFormatPatch() - if len(patches) != 1 { - return "" - } - - c, err := patches[0].ChangeId() - if err != nil { - return "" - } - - return c + return slices.Collect(maps.Keys(participants)) } -func (s *PullSubmission) Participants() []string { - participantSet := make(map[string]struct{}) - participants := []string{} - - addParticipant := func(did string) { - if _, exists := participantSet[did]; !exists { - participantSet[did] = struct{}{} - participants = append(participants, did) - } - } - - addParticipant(s.PullAt.Authority().String()) +func (s *PullVersion) Participants() []syntax.DID { + participants := make(map[syntax.DID]struct{}) for _, c := range s.Comments { - addParticipant(c.Did.String()) + participants[c.Did] = struct{}{} } - return participants -} - -func (s PullSubmission) CombinedPatch() string { - if s.Combined == "" { - return s.Patch - } - - return s.Combined -} - -func (s *PullSubmission) GetBlob() *lexutil.LexBlob { - if !s.Blob.Ref.Defined() { - return nil - } - - return &s.Blob -} - -func (s *PullSubmission) AsRecord() *tangled.RepoPull_Round { - return &tangled.RepoPull_Round{ - CreatedAt: s.Created.Format(time.RFC3339), - PatchBlob: s.GetBlob(), - } -} - -type Stack []*Pull - -// position of this pull in the stack -func (stack Stack) Position(pull *Pull) int { - return slices.IndexFunc(stack, func(p *Pull) bool { - return p.AtUri() == pull.AtUri() - }) -} - -// all pulls below this pull (including self) in this stack -// -// nil if this pull does not belong to this stack -func (stack Stack) Below(pull *Pull) Stack { - position := stack.Position(pull) - - if position < 0 { - return nil - } - - return stack[position:] -} - -// all pulls below this pull (excluding self) in this stack -func (stack Stack) StrictlyBelow(pull *Pull) Stack { - below := stack.Below(pull) - - if len(below) > 0 { - return below[1:] - } - - return nil -} - -// all pulls above this pull (including self) in this stack -func (stack Stack) Above(pull *Pull) Stack { - position := stack.Position(pull) - - if position < 0 { - return nil - } - - return stack[:position+1] -} - -// all pulls below this pull (excluding self) in this stack -func (stack Stack) StrictlyAbove(pull *Pull) Stack { - above := stack.Above(pull) - - if len(above) > 0 { - return above[:len(above)-1] - } - - return nil -} - -// the combined format-patches of all the newest submissions in this stack -func (stack Stack) CombinedPatch() string { - // go in reverse order because the bottom of the stack is the last element in the slice - var combined strings.Builder - for idx := range stack { - pull := stack[len(stack)-1-idx] - combined.WriteString(pull.LatestPatch()) - combined.WriteString("\n") - } - return combined.String() -} - -// filter out PRs that are "active" -// -// PRs that are still open are active -func (stack Stack) Mergeable() Stack { - var mergeable Stack - - for _, p := range stack { - // stop at the first merged PR - if p.State == PullMerged || p.State == PullClosed { - break - } - - // skip over abandoned PRs - if p.State != PullAbandoned { - mergeable = append(mergeable, p) - } - } - - return mergeable -} - -type BranchDeleteStatus struct { - Repo *Repo - Branch string + return slices.Collect(maps.Keys(participants)) } func extractGzip(blob io.Reader) (string, error) { @@ -581,3 +369,14 @@ func extractGzip(blob io.Reader) (string, error) { return b.String(), nil } + +func IsHash(s string) bool { + switch len(s) { + case 40: // SHA1 + case 64: // SHA2 + default: + return false + } + _, err := hex.DecodeString(s) + return err == nil +} diff --git a/appview/notify/db/db.go b/appview/notify/db/db.go index a5a80a2d..ff94ed18 100644 --- a/appview/notify/db/db.go +++ b/appview/notify/db/db.go @@ -158,8 +158,7 @@ func (n *databaseNotifier) NewComment(ctx context.Context, comment *models.Comme ) case tangled.RepoPullNSID: - pull, err := db.GetPull( - n.db, + pull, err := db.GetPull(ctx, n.db, orm.FilterEq("owner_did", subjectAt.Authority()), orm.FilterEq("rkey", subjectAt.RecordKey()), ) @@ -381,7 +380,7 @@ func (n *databaseNotifier) NewPull(ctx context.Context, pull *models.Pull) { recipients.Insert(c.SubjectDid) } - actorDid := syntax.DID(pull.OwnerDid) + actorDid := pull.OwnerDid eventType := models.NotificationTypePullCreated entityType := "pull" entityId := pull.AtUri().String() diff --git a/appview/notify/db/db_test.go b/appview/notify/db/db_test.go index e043b857..abf8c975 100644 --- a/appview/notify/db/db_test.go +++ b/appview/notify/db/db_test.go @@ -129,7 +129,7 @@ func seedPull(t *testing.T, d *appviewdb.DB, authorDid, repoDid string) *models. t.Helper() pull := &models.Pull{ RepoDid: syntax.DID(repoDid), - OwnerDid: authorDid, + OwnerDid: syntax.DID(authorDid), Rkey: "pullrkey", Title: "test", Body: "body", @@ -140,7 +140,7 @@ func seedPull(t *testing.T, d *appviewdb.DB, authorDid, repoDid string) *models. if err != nil { t.Fatalf("Begin: %v", err) } - if err := appviewdb.PutPull(tx, pull); err != nil { + if err := appviewdb.PutPull(t.Context(), tx, pull, nil); err != nil { t.Fatalf("PutPull: %v", err) } if err := tx.Commit(); err != nil { diff --git a/appview/notify/posthog/notifier.go b/appview/notify/posthog/notifier.go index 6a35ca5f..d1e2a876 100644 --- a/appview/notify/posthog/notifier.go +++ b/appview/notify/posthog/notifier.go @@ -96,7 +96,7 @@ func (n *posthogNotifier) NewIssue(ctx context.Context, issue *models.Issue, men func (n *posthogNotifier) NewPull(ctx context.Context, pull *models.Pull) { err := n.client.Enqueue(posthog.Capture{ - DistinctId: pull.OwnerDid, + DistinctId: pull.OwnerDid.String(), Event: "new_pull", Properties: posthog.Properties{ "repo_did": string(pull.RepoDid), @@ -108,20 +108,6 @@ func (n *posthogNotifier) NewPull(ctx context.Context, pull *models.Pull) { } } -func (n *posthogNotifier) NewPullClosed(ctx context.Context, pull *models.Pull) { - err := n.client.Enqueue(posthog.Capture{ - DistinctId: pull.OwnerDid, - Event: "pull_closed", - Properties: posthog.Properties{ - "repo_did": string(pull.RepoDid), - "pull_id": pull.PullId, - }, - }) - if err != nil { - log.Println("failed to enqueue posthog event:", err) - } -} - func (n *posthogNotifier) NewFollow(ctx context.Context, follow *models.Follow) { err := n.client.Enqueue(posthog.Capture{ DistinctId: follow.UserDid, @@ -247,7 +233,7 @@ func (n *posthogNotifier) NewPullState(ctx context.Context, actor syntax.DID, pu return } err := n.client.Enqueue(posthog.Capture{ - DistinctId: pull.OwnerDid, + DistinctId: pull.OwnerDid.String(), Event: event, Properties: posthog.Properties{ "repo_did": string(pull.RepoDid), diff --git a/appview/notify/webhook/notifier.go b/appview/notify/webhook/notifier.go index bf0ebe9c..92598196 100644 --- a/appview/notify/webhook/notifier.go +++ b/appview/notify/webhook/notifier.go @@ -108,7 +108,7 @@ func (w *Notifier) NewPullState(ctx context.Context, actor syntax.DID, pull *mod if !ok { return } - w.pullRequestEvent(ctx, event, action, actor.String(), pull) + w.pullRequestEvent(ctx, event, action, actor, pull) } // pullStateEvent maps a pull's state to the webhook event announcing the @@ -126,7 +126,7 @@ func pullStateEvent(state models.PullState) (models.WebhookEvent, string, bool) } } -func (w *Notifier) pullRequestEvent(ctx context.Context, event models.WebhookEvent, action, sender string, pull *models.Pull) { +func (w *Notifier) pullRequestEvent(ctx context.Context, event models.WebhookEvent, action string, sender syntax.DID, pull *models.Pull) { // pull request events originate from http handlers, whose context is // canceled as soon as the handler returns; detach so in-flight // deliveries are not cut short @@ -160,32 +160,32 @@ func (w *Notifier) pullRequestEvent(ctx context.Context, event models.WebhookEve } } -func buildPullRequestPayload(action string, repo *models.Repo, pull *models.Pull, sender, baseUrl string) *models.WebhookPullRequestPayload { +func buildPullRequestPayload(action string, repo *models.Repo, pull *models.Pull, sender syntax.DID, baseUrl string) *models.WebhookPullRequestPayload { htmlUrl := fmt.Sprintf("%s/%s/%s/pulls/%d", baseUrl, repo.Did, repo.Slug(), pull.PullId) pullRequest := models.WebhookPullRequest{ - Number: pull.PullId, + Number: int(pull.PullId), Title: pull.Title, Body: pull.Body, State: pull.State.String(), TargetBranch: pull.TargetBranch, - Owner: models.WebhookUser{Did: pull.OwnerDid}, + Owner: models.WebhookUser{Did: pull.OwnerDid.String()}, HtmlUrl: htmlUrl, CreatedAt: pull.Created.Format(time.RFC3339), } - if len(pull.Submissions) > 0 { - pullRequest.RoundNumber = pull.LastRoundNumber() - pullRequest.PatchUrl = fmt.Sprintf("%s/round/%d.patch", htmlUrl, pull.LastRoundNumber()) + if len(pull.Versions) > 0 { + pullRequest.RoundNumber = pull.LatestVersionNumber() + pullRequest.PatchUrl = fmt.Sprintf("%s/%d.patch", htmlUrl, pull.LatestVersionNumber()) } - if pull.PullSource != nil { + if pull.SourceBranch != nil { source := &models.WebhookPullRequestSource{ - Branch: pull.PullSource.Branch, + Branch: *pull.SourceBranch, } - if len(pull.Submissions) > 0 { + if len(pull.Versions) > 0 { source.Sha = pull.LatestSha() } if pull.IsForkBased() { - source.Repo = pull.PullSource.RepoDid.String() + source.Repo = pull.SourceRepo.String() } pullRequest.Source = source } @@ -194,7 +194,7 @@ func buildPullRequestPayload(action string, repo *models.Repo, pull *models.Pull Action: action, PullRequest: pullRequest, Repository: buildWebhookRepository(repo), - Sender: models.WebhookUser{Did: sender}, + Sender: models.WebhookUser{Did: sender.String()}, } } diff --git a/appview/oauth/scopes.go b/appview/oauth/scopes.go index 655cd8d4..08c41aff 100644 --- a/appview/oauth/scopes.go +++ b/appview/oauth/scopes.go @@ -34,6 +34,7 @@ var TangledScopes = []string{ "rpc:sh.tangled.ci.triggerPipeline?aud=*", "rpc:sh.tangled.ci.cancelPipeline?aud=*", "rpc:sh.tangled.git.keepCommit?aud=*", + "rpc:sh.tangled.git.mergeCommit?aud=*", "rpc:sh.tangled.repo.addCollaborator?aud=*", "rpc:sh.tangled.repo.addSecret?aud=*", "rpc:sh.tangled.repo.create?aud=*", diff --git a/appview/pages/compose_parse_test.go b/appview/pages/compose_parse_test.go deleted file mode 100644 index 9735ec12..00000000 --- a/appview/pages/compose_parse_test.go +++ /dev/null @@ -1,324 +0,0 @@ -package pages - -import ( - "bytes" - "io" - "log/slog" - "strings" - "testing" - - "tangled.org/core/appview/config" - "tangled.org/core/appview/models" - "tangled.org/core/appview/pages/repoinfo" - "tangled.org/core/patchutil" - "tangled.org/core/types" -) - -func TestPullComposeTemplatesParse(t *testing.T) { - cfg := &config.Config{} - p := NewPages(cfg, nil, nil, nil, slog.New(slog.NewTextHandler(io.Discard, nil))) - - cases := []struct { - name string - stack []string - }{ - {"new.html via repo base", []string{"layouts/base", "layouts/repobase", "repo/pulls/new"}}, - {"pullComposeHost", []string{"repo/pulls/fragments/pullComposeHost"}}, - {"pullStepSource", []string{"repo/pulls/fragments/pullStepSource"}}, - {"pullStepReview", []string{"repo/pulls/fragments/pullStepReview"}}, - {"pullStepDetails", []string{"repo/pulls/fragments/pullStepDetails"}}, - {"pullCompareForks", []string{"repo/pulls/fragments/pullCompareForks"}}, - {"pullCompareBranches", []string{"repo/pulls/fragments/pullCompareBranches"}}, - {"pullCompareForksBranches", []string{"repo/pulls/fragments/pullCompareForksBranches"}}, - {"pull.html via repo base", []string{"layouts/base", "layouts/repobase", "repo/pulls/pull"}}, - {"pullComment", []string{"fragments/comment/pullComment"}}, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - if _, err := p.rawParse(c.stack...); err != nil { - t.Fatalf("parse %v: %v", c.stack, err) - } - }) - } -} - -func TestPullComposeHostRender(t *testing.T) { - cfg := &config.Config{} - p := NewPages(cfg, nil, nil, nil, slog.New(slog.NewTextHandler(io.Discard, nil))) - - base := RepoNewPullParams{ - RepoInfo: repoinfo.RepoInfo{ - OwnerDid: "did:plc:test", - Name: "test-repo", - }, - } - - for _, source := range []Source{"", SourceBranch, SourceFork, SourcePatch} { - for _, stacked := range []bool{false, true} { - if source == SourcePatch && stacked { - continue - } - params := base - params.Source = source - params.IsStacked = stacked - name := string(source) - if name == "" { - name = "default" - } - if stacked { - name += "-stacked" - } - t.Run(name, func(t *testing.T) { - if err := p.PullComposeHostFragment(io.Discard, params); err != nil { - t.Fatalf("render source=%q stacked=%v: %v", source, stacked, err) - } - }) - } - } -} - -func TestPullComposeHostRenderWithData(t *testing.T) { - cfg := &config.Config{} - p := NewPages(cfg, nil, nil, nil, slog.New(slog.NewTextHandler(io.Discard, nil))) - - sampleBranches := []types.Branch{ - {Reference: types.Reference{Name: "feature"}}, - {Reference: types.Reference{Name: "main"}, IsDefault: true}, - } - - formatPatch := `From 1111111111111111111111111111111111111111 Mon Sep 11 00:00:00 2001 -From: Test -Date: Tue, 1 Jan 2020 00:00:00 +0000 -Subject: [PATCH] example commit - ---- - a.txt | 1 + - 1 file changed, 1 insertion(+) - -diff --git a/a.txt b/a.txt -index 0000000..1111111 100644 ---- a/a.txt -+++ b/a.txt -@@ -0,0 +1 @@ -+hello -` - patches, err := patchutil.ExtractPatches(formatPatch) - if err != nil { - t.Fatalf("extract patches: %v", err) - } - comparison := &types.RepoFormatPatchResponse{ - FormatPatchRaw: formatPatch, - FormatPatch: patches, - } - diff := patchutil.AsNiceDiff(formatPatch, "main") - - params := RepoNewPullParams{ - RepoInfo: repoinfo.RepoInfo{ - OwnerDid: "did:plc:test", - Name: "test-repo", - }, - Branches: sampleBranches, - SourceBranches: []types.Branch{sampleBranches[0]}, - ForkBranches: []types.Branch{sampleBranches[0]}, - Source: SourceBranch, - SourceBranch: "feature", - TargetBranch: "main", - Comparison: comparison, - Diff: &diff, - } - - if err := p.PullComposeHostFragment(io.Discard, params); err != nil { - t.Fatalf("render with data: %v", err) - } - - params.IsStacked = true - if err := p.PullComposeHostFragment(io.Discard, params); err != nil { - t.Fatalf("render stacked: %v", err) - } - - params.PrefillError = "branch not found" - params.Comparison = nil - params.Diff = nil - if err := p.PullComposeHostFragment(io.Discard, params); err != nil { - t.Fatalf("render with prefill error: %v", err) - } - - bugDef := &models.LabelDefinition{ - Did: "did:plc:test", - Rkey: "bug", - Name: "bug", - ValueType: models.ValueType{Type: models.ConcreteTypeNull}, - Scope: []string{"sh.tangled.repo.pull"}, - } - priorityDef := &models.LabelDefinition{ - Did: "did:plc:test", - Rkey: "priority", - Name: "priority", - ValueType: models.ValueType{Type: models.ConcreteTypeString, Enum: []string{"low", "med", "high"}}, - Scope: []string{"sh.tangled.repo.pull"}, - } - assigneeDef := &models.LabelDefinition{ - Did: "did:plc:test", - Rkey: "assignee", - Name: "assignee", - ValueType: models.ValueType{Type: models.ConcreteTypeString, Format: models.ValueTypeFormatDid}, - Scope: []string{"sh.tangled.repo.pull"}, - Multiple: true, - } - labelDefs := map[string]*models.LabelDefinition{ - bugDef.AtUri().String(): bugDef, - priorityDef.AtUri().String(): priorityDef, - assigneeDef.AtUri().String(): assigneeDef, - } - - pushRepoInfo := repoinfo.RepoInfo{ - OwnerDid: "did:plc:test", - Name: "test-repo", - Roles: repoinfo.RolesInRepo{Roles: []string{"repo:push"}}, - } - params = RepoNewPullParams{ - RepoInfo: pushRepoInfo, - Branches: sampleBranches, - SourceBranches: []types.Branch{sampleBranches[0]}, - Source: SourceBranch, - SourceBranch: "feature", - TargetBranch: "main", - Comparison: comparison, - Diff: &diff, - LabelDefs: labelDefs, - LabelState: models.NewLabelState(), - } - if err := p.PullComposeHostFragment(io.Discard, params); err != nil { - t.Fatalf("render with labels: %v", err) - } - - params.IsStacked = true - if err := p.PullComposeHostFragment(io.Discard, params); err != nil { - t.Fatalf("render stacked with labels: %v", err) - } - - params.StackedDiffs = []StackedDiff{{ - Diff: &diff, - Opts: types.DiffOpts{Split: true, RefreshUrl: "/r", Target: "#stack-diff-x", Field: "stackSplit[x]"}, - }} - if err := p.PullComposeHostFragment(io.Discard, params); err != nil { - t.Fatalf("render stacked with per-commit diffs: %v", err) - } -} - -func TestPullComposeLabelStateRoundTrip(t *testing.T) { - cfg := &config.Config{} - p := NewPages(cfg, nil, nil, nil, slog.New(slog.NewTextHandler(io.Discard, nil))) - - sampleBranches := []types.Branch{ - {Reference: types.Reference{Name: "feature"}}, - {Reference: types.Reference{Name: "main"}, IsDefault: true}, - } - - bugDef := &models.LabelDefinition{ - Did: "did:plc:test", Rkey: "bug", Name: "bug", - ValueType: models.ValueType{Type: models.ConcreteTypeNull}, - Scope: []string{"sh.tangled.repo.pull"}, - } - priorityDef := &models.LabelDefinition{ - Did: "did:plc:test", Rkey: "priority", Name: "priority", - ValueType: models.ValueType{Type: models.ConcreteTypeString, Enum: []string{"low", "med", "high"}}, - Scope: []string{"sh.tangled.repo.pull"}, - } - bugKey := bugDef.AtUri().String() - priorityKey := priorityDef.AtUri().String() - labelDefs := map[string]*models.LabelDefinition{ - bugKey: bugDef, - priorityKey: priorityDef, - } - - state := models.NewLabelState() - actx := &models.LabelApplicationCtx{Defs: labelDefs} - for _, op := range []models.LabelOp{ - {OperandKey: bugKey, OperandValue: "null", Operation: models.LabelOperationAdd}, - {OperandKey: priorityKey, OperandValue: "high", Operation: models.LabelOperationAdd}, - } { - if err := actx.ApplyLabelOp(state, op); err != nil { - t.Fatalf("seed state: %v", err) - } - } - - formatPatch := `From 1111111111111111111111111111111111111111 Mon Sep 11 00:00:00 2001 -From: Test -Date: Tue, 1 Jan 2020 00:00:00 +0000 -Subject: [PATCH] example commit - ---- - a.txt | 1 + - 1 file changed, 1 insertion(+) - -diff --git a/a.txt b/a.txt -index 0000000..1111111 100644 ---- a/a.txt -+++ b/a.txt -@@ -0,0 +1 @@ -+hello -` - patches, err := patchutil.ExtractPatches(formatPatch) - if err != nil { - t.Fatalf("extract patches: %v", err) - } - comparison := &types.RepoFormatPatchResponse{ - FormatPatchRaw: formatPatch, - FormatPatch: patches, - } - - params := RepoNewPullParams{ - RepoInfo: repoinfo.RepoInfo{ - OwnerDid: "did:plc:test", - Name: "test-repo", - Roles: repoinfo.RolesInRepo{Roles: []string{"repo:push"}}, - }, - Branches: sampleBranches, - SourceBranches: []types.Branch{sampleBranches[0]}, - Source: SourceBranch, - SourceBranch: "feature", - TargetBranch: "main", - Comparison: comparison, - LabelDefs: labelDefs, - LabelState: state, - } - - var buf bytes.Buffer - if err := p.PullComposeHostFragment(&buf, params); err != nil { - t.Fatalf("render: %v", err) - } - out := buf.String() - for _, want := range []string{ - `value="null" checked`, - `value="high" checked`, - } { - if !strings.Contains(out, want) { - t.Errorf("missing pre-selection %q", want) - } - } -} - -func TestParseSource(t *testing.T) { - cases := []struct { - in string - want Source - wantOk bool - }{ - {"branch", SourceBranch, true}, - {"BRANCH", SourceBranch, true}, - {"fork", SourceFork, true}, - {"patch", SourcePatch, true}, - {"", "", false}, - {"method", "", false}, - {"strategy", "", false}, - {"unknown", "", false}, - } - for _, c := range cases { - got, ok := ParseSource(c.in) - if got != c.want || ok != c.wantOk { - t.Errorf("ParseSource(%q) = %q, %v; want %q, %v", c.in, got, ok, c.want, c.wantOk) - } - } -} diff --git a/appview/pages/funcmap.go b/appview/pages/funcmap.go index 0b5fd231..fa8370b4 100644 --- a/appview/pages/funcmap.go +++ b/appview/pages/funcmap.go @@ -137,7 +137,7 @@ func (p *Pages) funcMap() template.FuncMap { return "" } // GetPull's reverse-mapping already populates pull.Repo - pull, err := db.GetPull(p.db, orm.FilterEq("at_uri", pullAtStr)) + pull, err := db.GetPull(context.Background(), p.db, orm.FilterEq("at_uri", pullAtStr)) if err != nil || pull == nil || pull.Repo == nil { return "" } @@ -150,12 +150,7 @@ func (p *Pages) funcMap() template.FuncMap { return s[:30] + "…" }, // short prefix of a commit hash or jj change id, safe on short input - "shortId": func(s string) string { - if len(s) <= 8 { - return s - } - return s[:8] - }, + "shortId": shortId, "splitOn": func(s, sep string) []string { return strings.Split(s, sep) }, @@ -617,6 +612,13 @@ func (p *Pages) funcMap() template.FuncMap { } } +func shortId(s string) string { + if len(s) <= 8 { + return s + } + return s[:8] +} + func primaryHandle(r *idresolver.Resolver, s string) string { identity, err := r.ResolveIdent(context.Background(), s) if err != nil || identity.Handle.IsInvalidHandle() { diff --git a/appview/pages/pages.go b/appview/pages/pages.go index 726687d4..f61c6d0e 100644 --- a/appview/pages/pages.go +++ b/appview/pages/pages.go @@ -22,12 +22,14 @@ import ( "tangled.org/core/appview/commitverify" "tangled.org/core/appview/config" "tangled.org/core/appview/db" + "tangled.org/core/appview/filetree" "tangled.org/core/appview/models" "tangled.org/core/appview/oauth" "tangled.org/core/appview/pages/markup" "tangled.org/core/appview/pages/markup/sanitizer" "tangled.org/core/appview/pages/repoinfo" "tangled.org/core/appview/pagination" + gitmirrorv1 "tangled.org/core/gitmirror/proto/gen" "tangled.org/core/idresolver" "tangled.org/core/types" @@ -1375,7 +1377,7 @@ func (p *Pages) IssueSubscribeFragment(w io.Writer, params IssueSubscribeParams) type PullSubscribeParams struct { RepoInfo repoinfo.RepoInfo - PullId int + PullId int64 IsSubscribed *bool } @@ -1429,33 +1431,73 @@ type StackedDiff struct { type RepoNewPullParams struct { BaseParams - RepoInfo repoinfo.RepoInfo - Branches []types.Branch - SourceBranches []types.Branch - ForkBranches []types.Branch - Forks []models.Repo - Source Source - SourceBranch string - TargetBranch string - Fork string - Patch string - Title string - Body string - TitleDirty bool - BodyDirty bool - IsStacked bool - Comparison *types.RepoFormatPatchResponse - Diff *types.NiceDiff - DiffOpts types.DiffOpts - StackedDiffs []StackedDiff - MergeCheck *types.MergeCheckResponse - StackTitles map[string]string - StackBodies map[string]string - PrefillError string - Active string - LabelDefs map[string]*models.LabelDefinition - LabelState models.LabelState - StackLabelStates map[string]models.LabelState + RepoInfo repoinfo.RepoInfo + Active string + PrefillError string + + // step 1. choose source + // TODO: replace to RepoNewPull_StepSourceParams + Branches []types.Branch + SourceBranches []types.Branch + ForkBranches []types.Branch + Forks []models.Repo + // selected values + Source Source // source kind + TargetBranch string + Fork string // fork repo DID + SourceBranch string + Patch string + + // step 2. review changes + StepReviewParams *RepoNewPull_StepReviewParams // optional step 2 params + + // step 3. fill details + // TODO: replace to RepoNewPull_StepDetailsParams + Title string + Body string + TitleDirty bool // flag to avoid overwriting users input + BodyDirty bool + MergeCheck MergeCheckParams + LabelDefs map[string]*models.LabelDefinition + LabelState models.LabelState +} + +func (p RepoNewPullParams) SourceRepo() string { + if p.Fork != "" { + return p.Fork + } + return p.RepoInfo.RepoDid +} + +type RepoNewPull_StepSourceParams struct { + Branches []types.Branch + SourceBranches []types.Branch + ForkBranches []types.Branch + Forks []models.Repo + ErrorMsg string + // selected values + Source Source // source kind + TargetBranch string + Fork string // fork repo DID + SourceBranch string + Patch string +} + +type RepoNewPull_StepReviewParams struct { + Commits []types.Commit +} + +type RepoNewPull_StepDetailsParams struct { + Title string + Body string + TitleDirty bool + BodyDirty bool +} + +type MergeCheckParams struct { + IsConflicted bool + Conflicts []*gitmirrorv1.MergeConflict + Error string } func (p *Pages) RepoNewPull(w io.Writer, params RepoNewPullParams) error { @@ -1489,7 +1531,6 @@ type RepoPullsParams struct { FilterState string FilterQuery string BaseFilterQuery string - Stacks []models.Stack Pipelines map[string]types.Pipeline LabelDefs map[string]*models.LabelDefinition Page pagination.Page @@ -1520,40 +1561,23 @@ func (r ResubmitResult) Unknown() bool { return r == Unknown } -type RepoSinglePullParams struct { - BaseParams - RepoInfo repoinfo.RepoInfo - Active string - Pull *models.Pull - Stack models.Stack - Backlinks []models.RichReferenceLink - BranchDeleteStatus *models.BranchDeleteStatus - MergeCheck types.MergeCheckResponse - ResubmitCheck ResubmitResult - Pipelines map[string]types.Pipeline - Diff types.DiffRenderer - DiffOpts types.DiffOpts - ActiveRound int - IsInterdiff bool - - Reactions map[syntax.ATURI]map[models.ReactionKind]models.ReactionDisplayData - UserReacted map[syntax.ATURI]map[models.ReactionKind]bool - - LabelDefs map[string]*models.LabelDefinition - VouchRelationships map[syntax.DID]*models.VouchRelationship - VouchSkips map[syntax.DID]bool - - // IsSubscribed is nil when not logged in, true when subscribed, false when explicitly unsubscribed. - IsSubscribed *bool +type BranchDeleteStatus struct { + Repo *models.Repo + Branch string } type PullPageBaseParams struct { BaseParams - Pull *models.Pull + RepoInfo repoinfo.RepoInfo + Pull *models.Pull Backlinks []models.RichReferenceLink - Comments []models.Comment Commits []types.Commit // all commits between .. + Pipelines map[string]types.Pipeline + + MergeCheck MergeCheckParams + ResubmitCheck ResubmitResult + BranchDeleteStatus *BranchDeleteStatus LabelDefs map[string]*models.LabelDefinition Reactions map[syntax.ATURI]map[models.ReactionKind]models.ReactionDisplayData @@ -1561,19 +1585,50 @@ type PullPageBaseParams struct { VouchRelationships map[syntax.DID]*models.VouchRelationship VouchSkips map[syntax.DID]bool + // IsSubscribed is nil when not logged in, true when subscribed, false when explicitly unsubscribed. + IsSubscribed *bool + // diff, branch-delete-status, merge-check, resubmit-check, pipelines will be lazy-loaded. } // /pulls/123/2/1a2b3c..d4e5f6 type PullDiffParams struct { PullPageBaseParams - Version int - BaseCommitId string - HeadCommitId string + VersionId int + + DiffParams DiffParams_Diff + IsDiffBase bool + IsDiffHead bool ErrorMsg string } +func (p PullDiffParams) ActiveVersionId() int { + return p.VersionId +} + +func (p PullDiffParams) ActiveCommitId() string { + return p.DiffParams.Head +} + +func (p PullDiffParams) IsInterdiff() bool { + return false +} + +func (p PullDiffParams) DisplayDiffBase() string { + if p.IsDiffBase { + return "base" + } + return shortId(p.DiffParams.Base) +} + +func (p PullDiffParams) DisplayDiffHead() string { + if p.IsDiffHead { + return "head" + } + return shortId(p.DiffParams.Head) +} + // /pulls/123/1..2/abcdef type PullInterdiffParams struct { PullPageBaseParams @@ -1581,31 +1636,159 @@ type PullInterdiffParams struct { Version2 int ChangeId string // optional change-id filter + DiffParams DiffParams + ActiveCommitId string + ErrorMsg string } -func (p *Pages) PullDiff(w io.Writer, params PullDiffParams) error { - panic("unimplemented") +func (p PullInterdiffParams) ActiveVersionId() int { + return p.Version2 } -func (p *Pages) PullInterdiff(w io.Writer, params PullInterdiffParams) error { - panic("unimplemented") +func (p PullInterdiffParams) IsInterdiff() bool { + return true } -func (p *Pages) RepoSinglePull(w io.Writer, params RepoSinglePullParams) error { - params.Active = "pulls" - return p.executeRepo("repo/pulls/pull", w, params) +type DiffParams struct { + Diff *DiffParams_Diff + Interdiff *DiffParams_Interdiff } -type PullResubmitParams struct { - BaseParams - RepoInfo repoinfo.RepoInfo - Pull *models.Pull - SubmissionId int +type DiffParams_Diff struct { + Base string + Head string +} + +type DiffParams_Interdiff struct { + From DiffParams_Diff + To DiffParams_Diff +} + +// DiffLine is one row of a unified (inline) diff. Old/New are 1-based line numbers into the +// base/head blob, or 0 when that side has no line here. Content is pre-rendered, safe HTML. +type DiffLine struct { + Op string // " " context, "-" removed, "+" added + Old int + New int + Content template.HTML +} + +// DiffCell is one side of a side-by-side row. Kind is "ctx", "del", "add", or "empty" (a +// blank padding cell). Num is the 1-based line number, or 0 when empty. +type DiffCell struct { + Kind string + Num int + Content template.HTML +} + +// DiffRow is one side-by-side row: the left (base) and right (head) cells. +type DiffRow struct { + Left DiffCell + Right DiffCell +} + +// DiffHunk holds a hunk's rows; exactly one of Lines (unified) / Rows (split) is populated, +// depending on PullDiffFragmentParams.Split. +type DiffHunk struct { + Lines []DiffLine + Rows []DiffRow +} + +func (h DiffHunk) AtFileStart() bool { + if len(h.Rows) > 0 { + r := h.Rows[0] + return r.Left.Num == 1 || r.Right.Num == 1 + } + if len(h.Lines) > 0 { + l := h.Lines[0] + return l.Old == 1 || l.New == 1 + } + return false +} + +// DiffFile is one changed file. Note is set (and Hunks empty) for binary/submodule files. +type DiffFile struct { + Path string + Note string + Hunks []DiffHunk +} + +type PullDiffFragmentParams struct { + BaseRepo syntax.DID + HeadRepo syntax.DID + DiffBase string + DiffHead string + DiffUrl string + Unified bool + Files []DiffFile + + ErrorMsg string +} + +func (f *DiffFile) Id() string { + return f.Path +} + +func (f *DiffFile) Stats() types.DiffFileStat { + var ins, del int64 + for _, hunk := range f.Hunks { + for _, line := range hunk.Lines { + switch line.Op { + case "+": + ins++ + case "-": + del++ + } + } + for _, row := range hunk.Rows { + if row.Left.Kind == "del" { + del++ + } + if row.Right.Kind == "add" { + ins++ + } + } + } + return types.DiffFileStat{ + Insertions: ins, + Deletions: del, + } +} + +func (p PullDiffFragmentParams) FileTree() *filetree.FileTreeNode { + fs := make([]string, len(p.Files)) + for i, s := range p.Files { + fs[i] = s.Id() + } + return filetree.FileTree(fs) +} + +func (p PullDiffFragmentParams) Stats() types.DiffStat { + var stat types.DiffStat + for _, df := range p.Files { + fileStats := df.Stats() + stat.Insertions += fileStats.Insertions + stat.Deletions += fileStats.Deletions + } + stat.FilesChanged = len(p.Files) + return stat +} + +func (p *Pages) PullDiff(w io.Writer, params PullDiffParams) error { + return p.executeRepo("repo/pulls/single", w, params) +} + +func (p *Pages) PullInterdiff(w io.Writer, params PullInterdiffParams) error { + return p.executeRepo("repo/pulls/single", w, params) +} + +func (p *Pages) PullDiffFragment(w io.Writer, params PullDiffFragmentParams) error { + return p.executePlain("repo/pulls/fragments/diff", w, params) } -func (p *Pages) PullResubmitFragment(w io.Writer, params PullResubmitParams) error { - return p.executePlain("repo/pulls/fragments/pullResubmit", w, params) +func (p *Pages) PullComposeDiffFragment(w io.Writer, params PullDiffFragmentParams) error { + return p.executePlain("repo/pulls/fragments/composediff", w, params) } type PullActionsParams struct { @@ -1613,10 +1796,9 @@ type PullActionsParams struct { RepoInfo repoinfo.RepoInfo Pull *models.Pull RoundNumber int - MergeCheck types.MergeCheckResponse + MergeCheck MergeCheckParams ResubmitCheck ResubmitResult - BranchDeleteStatus *models.BranchDeleteStatus - Stack models.Stack + BranchDeleteStatus *BranchDeleteStatus // Workflow warning state for fork-based pulls without a pipeline on the // latest commit. WorkflowsChanged and ChangedWorkflowFiles are computed diff --git a/appview/pages/templates/fragments/line-quote-button.html b/appview/pages/templates/fragments/line-quote-button.html index 52199f53..be3da838 100644 --- a/appview/pages/templates/fragments/line-quote-button.html +++ b/appview/pages/templates/fragments/line-quote-button.html @@ -21,34 +21,46 @@ Array.from(document.querySelectorAll('form[hx-post="/comment"] textarea')) .find(ta => ta.offsetParent !== null) || null; - const lineOf = (el) => - el?.closest?.('span[id*="-O"]') - || el?.closest?.('span[id*="-N"]'); + const lineOf = (el) => el?.closest?.('[id*="-O"], [id*="-N"]'); - const anchorOf = (el) => { - const link = el.querySelector('a[href^="#"]'); - return link ? link.getAttribute('href').slice(1) : el.id || null; - }; + const anchorOf = (el) => + el.querySelector('a[href^="#"]')?.getAttribute('href').slice(1) ?? null; const fileOf = (el) => { const d = el.closest('details[id^="file-"]'); return d ? d.id.replace(/^file-/, '') : null; }; - const lineNumOf = (el) => anchorOf(el)?.match(/(\d+)(?:-[ON]?\d+)?$/)?.[1]; - - const columnOf = (el) => el.closest('.flex-col'); + const lineNumOf = (el) => anchorOf(el)?.match(/-[ON](\d+)$/)?.[1]; + + // Column key: split has two flex sides per row (left = first child / -O, + // right = last child / -N); unified is a single column per file. + const columnOf = (el) => { + const file = fileOf(el); + if (file == null) return null; + const side = el.classList.contains('diff-side') + ? (el === el.parentElement.firstElementChild ? 'L' : 'R') + : 'U'; + return file + '-' + side; + }; - const linesInColumn = (col) => - Array.from(col.querySelectorAll('span[id*="-O"], span[id*="-N"]')) - .filter(s => s.querySelector('a[href^="#"]')); + const linesInColumn = (el) => { + const diff = el.closest('.diff'); + if (!diff) return []; + if (el.classList.contains('diff-side')) { + const pos = el === el.parentElement.firstElementChild + ? ':first-child' : ':last-child'; + return Array.from(diff.querySelectorAll(`.diff-line > .diff-side[id]${pos}`)); + } + return Array.from(diff.querySelectorAll('.diff-line[id]')); + }; let dragLines = null; const rangeBetween = (a, b) => { const col = columnOf(a); if (!col || col !== columnOf(b)) return []; - const all = dragLines || linesInColumn(col); + const all = dragLines || linesInColumn(a); const ai = all.indexOf(a); const bi = all.indexOf(b); if (ai === -1 || bi === -1) return []; @@ -73,18 +85,7 @@ if (hash.startsWith('comment-') || hash.startsWith('round-')) return; const parts = hash.split('~'); const startEl = document.getElementById(parts[0]); - - if (!startEl) { - const params = new URLSearchParams(window.location.search); - const hasCombined = parts.some(p => /-O\d+-N\d+$/.test(p)); - if (hasCombined && params.get('diff') !== 'unified') { - params.set('diff', 'unified'); - window.location.replace( - `${window.location.pathname}?${params}${window.location.hash}` - ); - } - return; - } + if (!startEl) return; const endEl = parts.length === 2 ? document.getElementById(parts[1]) : startEl; if (!endEl) return; @@ -180,8 +181,7 @@ e.preventDefault(); dragging = true; dragAnchor = dragCurrent = hoverTarget; - const col = columnOf(hoverTarget); - dragLines = col ? linesInColumn(col) : null; + dragLines = linesInColumn(hoverTarget); applyHl(dragAnchor, dragCurrent, 'line-quote-hl'); btn.style.pointerEvents = 'none'; document.body.style.userSelect = 'none'; diff --git a/appview/pages/templates/layouts/base.html b/appview/pages/templates/layouts/base.html index 30424eb4..31837722 100644 --- a/appview/pages/templates/layouts/base.html +++ b/appview/pages/templates/layouts/base.html @@ -110,7 +110,7 @@ {{ block "topbarLayout" . }} -
+
{{ if .LoggedInUser }} {{ end }} diff --git a/appview/pages/templates/repo/pulls/fragments/composediff.html b/appview/pages/templates/repo/pulls/fragments/composediff.html new file mode 100644 index 00000000..04674570 --- /dev/null +++ b/appview/pages/templates/repo/pulls/fragments/composediff.html @@ -0,0 +1,84 @@ +{{ define "repo/pulls/fragments/composediff" }} + +
+ +
+ + + {{ template "repo/fragments/diffStatPill" .Stats }} + {{ $count := .Stats.FilesChanged }} + + +
+ + + + + {{ template "repo/pulls/fragments/diffSettings" + (dict "DiffUrl" (printf "%s?baseRepo=%s&base=%s&headRepo=%s&head=%s" .DiffUrl .BaseRepo .DiffBase .HeadRepo .DiffHead) + "Unified" .Unified) }} + +
+
+ +
+ {{ range .Files }} + {{ template "repo/pulls/fragments/diffFile" . }} + {{ end }} +
+
+
+ +{{ end }} diff --git a/appview/pages/templates/repo/pulls/fragments/diff.html b/appview/pages/templates/repo/pulls/fragments/diff.html new file mode 100644 index 00000000..f9c756b4 --- /dev/null +++ b/appview/pages/templates/repo/pulls/fragments/diff.html @@ -0,0 +1,334 @@ +{{ define "repo/pulls/fragments/diff" }} +
+ {{ template "repo/fragments/fileTree" .FileTree }} +
+
+ {{ template "repo/fragments/diffStatPill" .Stats }} +
+
+ {{ template "repo/pulls/fragments/diffSettings" + (dict "DiffUrl" .DiffUrl + "Unified" .Unified) }} +
+ +
+ {{ if .ErrorMsg }} +
+ {{ .ErrorMsg }} +
+ {{ else if .Files }} + {{ range .Files }} +
+
+ +
+
+ {{ i "chevron-right" "size-4" }} + + {{ template "repo/fragments/diffStatPill" .Stats }} + {{ .Path }} +
+
+ +
+
+ + +
+ {{ if .Note }} +
{{ .Note }}
+ {{ else if $.Unified }} + {{ template "diffUnified" . }} + {{ else }} + {{ template "diffSplit" . }} + {{ end }} +
+
+
+ {{ end }} + {{ else }} +
+ No change between two revisions. +
+ {{ end }} +
+ {{ template "activeFileHighlightScript" }} + {{ template "fragments/line-quote-button" }} + {{ template "reviewStateScript" }} +{{ end }} + +{{ define "diffUnified" }} +
+ {{ $name := .Id }} + {{- range .Hunks -}} + {{- if not .AtFileStart -}}
···
{{- end -}} + {{- range $i, $line := .Lines -}} + {{- $lineId := "" -}} + {{- if ge .New 0 -}} + {{- $lineId = printf "%s-N%d" $name .New -}} + {{- else -}} + {{- $lineId = printf "%s-O%d" $name .Old -}} + {{- end -}} + {{- $cls := "" -}} + {{- if eq .Op "+" -}} + {{- $cls = "add" -}} + {{- else if eq .Op "-" -}} + {{- $cls = "del" -}} + {{- end -}} + + {{- end -}} + {{- end -}} +
+{{ end }} + +{{ define "diffSplit" }} +
+ {{ $name := .Id }} + {{- range .Hunks -}} + {{- if not .AtFileStart -}}
···
{{- end -}} + {{- range $i, $row := .Rows -}} +
+ {{- template "diffSplitSide" (list $name "O" $row.Left) -}} + {{- template "diffSplitSide" (list $name "N" $row.Right) -}} +
+ {{- end -}} + {{- end -}} +
+{{ end }} + +{{ define "diffSplitSide" }} + {{- $name := index . 0 -}} + {{- $side := index . 1 -}} + {{- $line := index . 2 -}} + {{- $lineId := printf "%s-%s%d" $name $side $line.Num -}} + {{- $cls := "" -}} + {{- $mark := "" -}} + {{- if eq $line.Kind "del" }}{{ $cls = "del" }}{{ $mark = "-" -}} + {{- else if eq $line.Kind "add" }}{{ $cls = "add" }}{{ $mark = "+" -}} + {{- else if eq $line.Kind "empty" }}{{ $cls = "empty" -}} + {{- end -}} +
+ {{ if gt $line.Num 0 -}} + {{ $line.Num }} + {{- else -}} + + {{- end }} + {{ $mark }} +
{{ $line.Content }}
+
+{{ end }} + +{{ define "activeFileHighlightScript" }} + +{{ end }} + +{{ define "reviewStateScript" }} + +{{ end }} diff --git a/appview/pages/templates/repo/pulls/fragments/diffFile.html b/appview/pages/templates/repo/pulls/fragments/diffFile.html new file mode 100644 index 00000000..1367de37 --- /dev/null +++ b/appview/pages/templates/repo/pulls/fragments/diffFile.html @@ -0,0 +1,18 @@ +{{ define "repo/pulls/fragments/diffFile" }} +
+ +
+
+ {{ i "chevron-right" "size-4" }} + + {{ template "repo/fragments/diffStatPill" .Stats }} + {{ .Path }} +
+
+
+ +
+ 
todo: diff content
+
+
+{{ end }} diff --git a/appview/pages/templates/repo/pulls/fragments/diffSettings.html b/appview/pages/templates/repo/pulls/fragments/diffSettings.html new file mode 100644 index 00000000..c1e45feb --- /dev/null +++ b/appview/pages/templates/repo/pulls/fragments/diffSettings.html @@ -0,0 +1,29 @@ +{{ define "repo/pulls/fragments/diffSettings" }} +{{ $diffUrl := .DiffUrl }} +{{ $unified := .Unified }} +
+ + +
+{{ end }} diff --git a/appview/pages/templates/repo/pulls/fragments/pullActions.html b/appview/pages/templates/repo/pulls/fragments/pullActions.html index 6f4abf2e..f80c187c 100644 --- a/appview/pages/templates/repo/pulls/fragments/pullActions.html +++ b/appview/pages/templates/repo/pulls/fragments/pullActions.html @@ -1,19 +1,8 @@ {{ define "repo/pulls/fragments/pullActions" }} - {{ $lastIdx := sub (len .Pull.Submissions) 1 }} + {{ $lastIdx := .Pull.LatestVersionNumber }} {{ $roundNumber := .RoundNumber }} - {{ $stack := .Stack }} {{ $loading := .Loading }} - {{ $totalPulls := sub 0 1 }} - {{ $below := sub 0 1 }} - {{ $stackCount := "" }} - {{ if (gt (len .Stack) 1) }} - {{ $totalPulls = len $stack }} - {{ $below = $stack.Below .Pull }} - {{ $mergeable := len $below.Mergeable }} - {{ $stackCount = printf "%d/%d" $mergeable $totalPulls }} - {{ end }} - {{ $isPushAllowed := .RepoInfo.Roles.IsPushAllowed }} {{ $isMerged := .Pull.State.IsMerged }} {{ $isClosed := .Pull.State.IsClosed }} @@ -21,14 +10,13 @@ {{ $isConflicted := and .MergeCheck (or .MergeCheck.Error .MergeCheck.IsConflicted) }} {{ $isPullAuthor := and .LoggedInUser (eq .LoggedInUser.Did .Pull.OwnerDid) }} {{ $isLastRound := eq $roundNumber $lastIdx }} - {{ $isSameRepoBranch := .Pull.IsBranchBased }} {{ $isUpToDate := .ResubmitCheck.No }} {{ $isForkBased := .Pull.IsForkBased }} {{ $showRunCI := and (not $loading) $isPushAllowed $isOpen $isLastRound $isForkBased (not .HasPipeline) (ne .RepoInfo.Spindle "") }}
+ {{ template "fragments/markdownEditor" (dict "Name" "body" @@ -111,18 +100,14 @@ {{ i "git-merge" "w-4 h-4 inline group-[.htmx-request]:hidden" }} {{ i "loader-circle" "w-4 h-4 animate-spin hidden group-[.htmx-request]:inline" }} {{ end }} - Merge{{if $stackCount}} {{$stackCount}}{{end}} + Merge {{ end }} {{ if and $isPullAuthor $isOpen $isLastRound }}