From 898d826c985e456e949a4cfc0ccf62559eff8649 Mon Sep 17 00:00:00 2001 From: oppiliappan Date: Tue, 23 Sep 2025 14:14:10 +0000 Subject: [PATCH] appview,knotserver: produce combined patch in comparisons this is calculated by the knotserver in sh.tangled.repo.compare and cached by the appview in pull submissions, this cannot be calculated on the appview side with just the format-patch because this calculation requires a git-index. Signed-off-by: oppiliappan --- types/repo.go | 12 +++++++----- appview/db/db.go | 9 +++++++++ appview/db/pulls.go | 38 +++++++++++++++++++++----------------- appview/models/pull.go | 27 +++++++++++++++++++-------- appview/pulls/pulls.go | 67 ++++++++++++++++++++++++++----------------------------------------- appview/repo/repo.go | 7 ++++++- knotserver/xrpc/repo_compare.go | 24 ++++++++++++++++++++---- 7 file(s) changed, 108 insertion(s)(+), 76 deletion(s)(-) diff --git a/types/repo.go b/types/repo.go --- a/types/repo.go +++ b/types/repo.go @@ -1,6 +1,7 @@ package types import ( + "github.com/bluekeyes/go-gitdiff/gitdiff" "github.com/go-git/go-git/v5/plumbing/object" ) @@ -33,11 +34,12 @@ } type RepoFormatPatchResponse struct { - Rev1 string `json:"rev1,omitempty"` - Rev2 string `json:"rev2,omitempty"` - FormatPatch []FormatPatch `json:"format_patch,omitempty"` - MergeBase string `json:"merge_base,omitempty"` // deprecated - Patch string `json:"patch,omitempty"` + Rev1 string `json:"rev1,omitempty"` + Rev2 string `json:"rev2,omitempty"` + FormatPatch []FormatPatch `json:"format_patch,omitempty"` + FormatPatchRaw string `json:"patch,omitempty"` + CombinedPatch []*gitdiff.File `json:"combined_patch,omitempty"` + CombinedPatchRaw string `json:"combined_patch_raw,omitempty"` } type RepoTreeResponse struct { diff --git a/appview/db/db.go b/appview/db/db.go --- a/appview/db/db.go +++ b/appview/db/db.go @@ -1097,6 +1097,15 @@ }) conn.ExecContext(ctx, "pragma foreign_keys = on;") + // knots may report the combined patch for a comparison, we can store that on the appview side + // (but not on the pds record), because calculating the combined patch requires a git index + runMigration(conn, logger, "add-combined-column-submissions", func(tx *sql.Tx) error { + _, err := tx.Exec(` + alter table pull_submissions add column combined text; + `) + return err + }) + return &DB{ db, logger, diff --git a/appview/db/pulls.go b/appview/db/pulls.go --- a/appview/db/pulls.go +++ b/appview/db/pulls.go @@ -90,9 +90,9 @@ pull.ID = int(id) _, err = tx.Exec(` - insert into pull_submissions (pull_at, round_number, patch, source_rev) - values (?, ?, ?, ?) - `, pull.PullAt(), 0, pull.Submissions[0].Patch, pull.Submissions[0].SourceRev) + insert into pull_submissions (pull_at, round_number, patch, combined, source_rev) + values (?, ?, ?, ?, ?) + `, pull.PullAt(), 0, pull.Submissions[0].Patch, pull.Submissions[0].Combined, pull.Submissions[0].SourceRev) return err } @@ -313,6 +313,7 @@ pull_at, round_number, patch, + combined, created, source_rev from @@ -332,28 +333,31 @@ for rows.Next() { var submission models.PullSubmission - var createdAt string - var sourceRev sql.NullString + var submissionCreatedStr string + var submissionSourceRev, submissionCombined sql.NullString err := rows.Scan( &submission.ID, &submission.PullAt, &submission.RoundNumber, &submission.Patch, - &createdAt, - &sourceRev, + &submissionCombined, + &submissionCreatedStr, + &submissionSourceRev, ) if err != nil { return nil, err } - createdTime, err := time.Parse(time.RFC3339, createdAt) - if err != nil { - return nil, err + if t, err := time.Parse(time.RFC3339, submissionCreatedStr); err == nil { + submission.Created = t } - submission.Created = createdTime - if sourceRev.Valid { - submission.SourceRev = sourceRev.String + if submissionSourceRev.Valid { + submission.SourceRev = submissionSourceRev.String + } + + if submissionCombined.Valid { + submission.Combined = submissionCombined.String } submissionMap[submission.ID] = &submission @@ -590,11 +594,11 @@ return err } -func ResubmitPull(e Execer, pullAt syntax.ATURI, newRoundNumber int, newPatch string, newSourceRev string) error { +func ResubmitPull(e Execer, pullAt syntax.ATURI, newRoundNumber int, newPatch string, combinedPatch string, newSourceRev string) error { _, err := e.Exec(` - insert into pull_submissions (pull_at, round_number, patch, source_rev) - values (?, ?, ?, ?) - `, pullAt, newRoundNumber, newPatch, newSourceRev) + insert into pull_submissions (pull_at, round_number, patch, combined, source_rev) + values (?, ?, ?, ?, ?) + `, pullAt, newRoundNumber, newPatch, combinedPatch, newSourceRev) return err } diff --git a/appview/models/pull.go b/appview/models/pull.go --- a/appview/models/pull.go +++ b/appview/models/pull.go @@ -125,6 +125,7 @@ // content RoundNumber int Patch string + Combined string Comments []PullComment SourceRev string // include the rev that was used to create this submission: only for branch/fork PRs @@ -150,22 +151,24 @@ Created time.Time } +func (p *Pull) LastRoundNumber() int { + return len(p.Submissions) - 1 +} + +func (p *Pull) LatestSubmission() *PullSubmission { + return p.Submissions[p.LastRoundNumber()] +} + func (p *Pull) LatestPatch() string { - latestSubmission := p.Submissions[p.LastRoundNumber()] - return latestSubmission.Patch + return p.LatestSubmission().Patch } func (p *Pull) LatestSha() string { - latestSubmission := p.Submissions[p.LastRoundNumber()] - return latestSubmission.SourceRev + return p.LatestSubmission().SourceRev } func (p *Pull) PullAt() syntax.ATURI { return syntax.ATURI(fmt.Sprintf("at://%s/%s/%s", p.OwnerDid, tangled.RepoPullNSID, p.Rkey)) -} - -func (p *Pull) LastRoundNumber() int { - return len(p.Submissions) - 1 } func (p *Pull) IsPatchBased() bool { @@ -252,6 +255,14 @@ } return participants +} + +func (s PullSubmission) CombinedPatch() string { + if s.Combined == "" { + return s.Patch + } + + return s.Combined } type Stack []*Pull diff --git a/appview/pulls/pulls.go b/appview/pulls/pulls.go --- a/appview/pulls/pulls.go +++ b/appview/pulls/pulls.go @@ -145,23 +145,6 @@ stack, _ := r.Context().Value("stack").(models.Stack) abandonedPulls, _ := r.Context().Value("abandonedPulls").([]*models.Pull) - totalIdents := 1 - for _, submission := range pull.Submissions { - totalIdents += len(submission.Comments) - } - - identsToResolve := make([]string, totalIdents) - - // populate idents - identsToResolve[0] = pull.OwnerDid - idx := 1 - for _, submission := range pull.Submissions { - for _, comment := range submission.Comments { - identsToResolve[idx] = comment.OwnerDid - idx += 1 - } - } - mergeCheckResponse := s.mergeCheck(r, f, pull, stack) branchDeleteStatus := s.branchDeleteStatus(r, f, pull) resubmitResult := pages.Unknown @@ -459,7 +442,7 @@ return } - patch := pull.Submissions[roundIdInt].Patch + patch := pull.Submissions[roundIdInt].CombinedPatch() diff := patchutil.AsNiceDiff(patch, pull.TargetBranch) s.pages.RepoPullPatchPage(w, pages.RepoPullPatchParams{ @@ -510,14 +493,14 @@ return } - currentPatch, err := patchutil.AsDiff(pull.Submissions[roundIdInt].Patch) + currentPatch, err := patchutil.AsDiff(pull.Submissions[roundIdInt].CombinedPatch()) if err != nil { log.Println("failed to interdiff; current patch malformed") s.pages.Notice(w, fmt.Sprintf("interdiff-error-%d", roundIdInt), "Failed to calculate interdiff; current patch is invalid.") return } - previousPatch, err := patchutil.AsDiff(pull.Submissions[roundIdInt-1].Patch) + previousPatch, err := patchutil.AsDiff(pull.Submissions[roundIdInt-1].CombinedPatch()) if err != nil { log.Println("failed to interdiff; previous patch malformed") s.pages.Notice(w, fmt.Sprintf("interdiff-error-%d", roundIdInt), "Failed to calculate interdiff; previous patch is invalid.") @@ -719,13 +702,6 @@ createdAt := time.Now().Format(time.RFC3339) - pullAt, err := db.GetPullAt(s.db, f.RepoAt(), pull.PullId) - if err != nil { - log.Println("failed to get pull at", err) - s.pages.Notice(w, "pull-comment", "Failed to create comment.") - return - } - client, err := s.oauth.AuthorizedClient(r) if err != nil { log.Println("failed to get authorized client", err) @@ -738,7 +714,7 @@ Rkey: tid.TID(), Record: &lexutil.LexiconTypeDecoder{ Val: &tangled.RepoPullComment{ - Pull: string(pullAt), + Pull: pull.PullAt().String(), Body: body, CreatedAt: createdAt, }, @@ -986,7 +962,8 @@ } sourceRev := comparison.Rev2 - patch := comparison.Patch + patch := comparison.FormatPatchRaw + combined := comparison.CombinedPatchRaw if !patchutil.IsPatchValid(patch) { s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.") @@ -1001,7 +978,7 @@ Sha: comparison.Rev2, } - s.createPullRequest(w, r, f, user, title, body, targetBranch, patch, sourceRev, pullSource, recordPullSource, isStacked) + s.createPullRequest(w, r, f, user, title, body, targetBranch, patch, combined, sourceRev, pullSource, recordPullSource, isStacked) } func (s *Pulls) handlePatchBasedPull(w http.ResponseWriter, r *http.Request, f *reporesolver.ResolvedRepo, user *oauth.User, title, body, targetBranch, patch string, isStacked bool) { @@ -1010,7 +987,7 @@ return } - s.createPullRequest(w, r, f, user, title, body, targetBranch, patch, "", nil, nil, isStacked) + s.createPullRequest(w, r, f, user, title, body, targetBranch, patch, "", "", nil, nil, isStacked) } func (s *Pulls) handleForkBasedPull(w http.ResponseWriter, r *http.Request, f *reporesolver.ResolvedRepo, user *oauth.User, forkRepo string, title, body, targetBranch, sourceBranch string, isStacked bool) { @@ -1093,7 +1070,8 @@ } sourceRev := comparison.Rev2 - patch := comparison.Patch + patch := comparison.FormatPatchRaw + combined := comparison.CombinedPatchRaw if !patchutil.IsPatchValid(patch) { s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.") @@ -1113,7 +1091,7 @@ Sha: sourceRev, } - s.createPullRequest(w, r, f, user, title, body, targetBranch, patch, sourceRev, pullSource, recordPullSource, isStacked) + s.createPullRequest(w, r, f, user, title, body, targetBranch, patch, combined, sourceRev, pullSource, recordPullSource, isStacked) } func (s *Pulls) createPullRequest( @@ -1123,6 +1101,7 @@ user *oauth.User, title, body, targetBranch string, patch string, + combined string, sourceRev string, pullSource *models.PullSource, recordPullSource *tangled.RepoPull_Source, @@ -1182,6 +1161,7 @@ rkey := tid.TID() initialSubmission := models.PullSubmission{ Patch: patch, + Combined: combined, SourceRev: sourceRev, } pull := &models.Pull{ @@ -1611,7 +1591,7 @@ patch := r.FormValue("patch") - s.resubmitPullHelper(w, r, f, user, pull, patch, "") + s.resubmitPullHelper(w, r, f, user, pull, patch, "", "") } func (s *Pulls) resubmitBranch(w http.ResponseWriter, r *http.Request) { @@ -1672,9 +1652,10 @@ } sourceRev := comparison.Rev2 - patch := comparison.Patch + patch := comparison.FormatPatchRaw + combined := comparison.CombinedPatchRaw - s.resubmitPullHelper(w, r, f, user, pull, patch, sourceRev) + s.resubmitPullHelper(w, r, f, user, pull, patch, combined, sourceRev) } func (s *Pulls) resubmitFork(w http.ResponseWriter, r *http.Request) { @@ -1767,9 +1748,10 @@ comparison := forkComparison sourceRev := comparison.Rev2 - patch := comparison.Patch + patch := comparison.FormatPatchRaw + combined := comparison.CombinedPatchRaw - s.resubmitPullHelper(w, r, f, user, pull, patch, sourceRev) + s.resubmitPullHelper(w, r, f, user, pull, patch, combined, sourceRev) } // validate a resubmission against a pull request @@ -1796,6 +1778,7 @@ user *oauth.User, pull *models.Pull, patch string, + combined string, sourceRev string, ) { if pull.IsStacked() { @@ -1829,7 +1812,8 @@ newRoundNumber := len(pull.Submissions) newPatch := patch newSourceRev := sourceRev - err = db.ResubmitPull(tx, pullAt, newRoundNumber, newPatch, newSourceRev) + combinedPatch := combined + err = db.ResubmitPull(tx, pullAt, newRoundNumber, newPatch, combinedPatch, newSourceRev) if err != nil { log.Println("failed to create pull request", err) s.pages.Notice(w, "resubmit-error", "Failed to create pull request. Try again later.") @@ -2024,9 +2008,9 @@ pullAt := op.PullAt() newRoundNumber := len(op.Submissions) newPatch := np.LatestPatch() + combinedPatch := np.LatestSubmission().Combined newSourceRev := np.LatestSha() - err := db.ResubmitPull(tx, pullAt, newRoundNumber, newPatch, newSourceRev) - + err := db.ResubmitPull(tx, pullAt, newRoundNumber, newPatch, combinedPatch, newSourceRev) if err != nil { log.Println("failed to update pull", err, op.PullId) s.pages.Notice(w, "pull-resubmit-error", "Failed to resubmit pull request. Try again later.") @@ -2374,6 +2358,7 @@ initialSubmission := models.PullSubmission{ Patch: fp.Raw, SourceRev: fp.SHA, + Combined: fp.Raw, } pull := models.Pull{ Title: title, diff --git a/appview/repo/repo.go b/appview/repo/repo.go --- a/appview/repo/repo.go +++ b/appview/repo/repo.go @@ -2565,7 +2565,12 @@ return } - diff := patchutil.AsNiceDiff(formatPatch.Patch, base) + var diff types.NiceDiff + if formatPatch.CombinedPatchRaw != "" { + diff = patchutil.AsNiceDiff(formatPatch.CombinedPatchRaw, base) + } else { + diff = patchutil.AsNiceDiff(formatPatch.FormatPatchRaw, base) + } repoinfo := f.RepoInfo(user) diff --git a/knotserver/xrpc/repo_compare.go b/knotserver/xrpc/repo_compare.go --- a/knotserver/xrpc/repo_compare.go +++ b/knotserver/xrpc/repo_compare.go @@ -4,6 +4,7 @@ "fmt" "net/http" + "github.com/bluekeyes/go-gitdiff/gitdiff" "tangled.org/core/knotserver/git" "tangled.org/core/types" xrpcerr "tangled.org/core/xrpc/errors" @@ -71,11 +72,26 @@ return } + var combinedPatch []*gitdiff.File + var combinedPatchRaw string + // we need the combined patch + if len(formatPatch) >= 2 { + diffTree, err := gr.DiffTree(commit1, commit2) + if err != nil { + x.Logger.Error("error comparing revisions", "msg", err.Error()) + } else { + combinedPatch = diffTree.Diff + combinedPatchRaw = diffTree.Patch + } + } + response := types.RepoFormatPatchResponse{ - Rev1: commit1.Hash.String(), - Rev2: commit2.Hash.String(), - FormatPatch: formatPatch, - Patch: rawPatch, + Rev1: commit1.Hash.String(), + Rev2: commit2.Hash.String(), + FormatPatch: formatPatch, + FormatPatchRaw: rawPatch, + CombinedPatch: combinedPatch, + CombinedPatchRaw: combinedPatchRaw, } writeJson(w, response) -- tangled.sh