From bd1f54a531b2c1f8cc3da9005a37cb0e1df73ff8 Mon Sep 17 00:00:00 2001 From: oppiliappan Date: Tue, 23 Sep 2025 15:14:10 +0100 Subject: [PATCH] appview,knotserver: produce combined patch in comparisons Change-Id: tvqsxkkxqolvzmmqtyqslyxsqnxttkst 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 --- appview/db/db.go | 9 +++++ appview/db/pulls.go | 26 +++++++++----- appview/models/pull.go | 9 +++++ appview/pulls/pulls.go | 63 ++++++++++++--------------------- appview/repo/repo.go | 7 +++- knotserver/xrpc/repo_compare.go | 24 ++++++++++--- types/repo.go | 12 ++++--- 7 files changed, 92 insertions(+), 58 deletions(-) diff --git a/appview/db/db.go b/appview/db/db.go index a4b39560..1d79a317 100644 --- a/appview/db/db.go +++ b/appview/db/db.go @@ -954,6 +954,15 @@ func Make(dbPath string) (*DB, error) { return err }) + // 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, "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}, nil } diff --git a/appview/db/pulls.go b/appview/db/pulls.go index aad64fdc..c35311d7 100644 --- a/appview/db/pulls.go +++ b/appview/db/pulls.go @@ -87,9 +87,9 @@ func NewPull(tx *sql.Tx, pull *models.Pull) error { pull.ID = int(id) _, err = tx.Exec(` - insert into pull_submissions (pull_id, repo_at, round_number, patch, source_rev) + insert into pull_submissions (pull_id, repo_at, round_number, patch, combined, source_rev) values (?, ?, ?, ?, ?) - `, pull.PullId, pull.RepoAt, 0, pull.Submissions[0].Patch, pull.Submissions[0].SourceRev) + `, pull.PullId, pull.RepoAt, 0, pull.Submissions[0].Patch, pull.Submissions[0].Combined, pull.Submissions[0].SourceRev) return err } @@ -218,7 +218,7 @@ func GetPullsWithLimit(e Execer, limit int, filters ...filter) ([]*models.Pull, inClause := strings.TrimSuffix(strings.Repeat("?, ", len(pulls)), ", ") submissionsQuery := fmt.Sprintf(` select - id, pull_id, round_number, patch, created, source_rev + id, pull_id, round_number, patch, combined, created, source_rev from pull_submissions where @@ -243,13 +243,14 @@ func GetPullsWithLimit(e Execer, limit int, filters ...filter) ([]*models.Pull, for submissionsRows.Next() { var s models.PullSubmission - var sourceRev sql.NullString + var sourceRev, combined sql.NullString var createdAt string err := submissionsRows.Scan( &s.ID, &s.PullId, &s.RoundNumber, &s.Patch, + &combined, &createdAt, &sourceRev, ) @@ -267,6 +268,10 @@ func GetPullsWithLimit(e Execer, limit int, filters ...filter) ([]*models.Pull, s.SourceRev = sourceRev.String } + if combined.Valid { + s.Combined = combined.String + } + if p, ok := pulls[s.PullId]; ok { p.Submissions = make([]*models.PullSubmission, s.RoundNumber+1) p.Submissions[s.RoundNumber] = &s @@ -412,7 +417,7 @@ func GetPull(e Execer, repoAt syntax.ATURI, pullId int) (*models.Pull, error) { submissionsQuery := ` select - id, pull_id, repo_at, round_number, patch, created, source_rev + id, pull_id, repo_at, round_number, patch, combined, created, source_rev from pull_submissions where @@ -429,13 +434,14 @@ func GetPull(e Execer, repoAt syntax.ATURI, pullId int) (*models.Pull, error) { for submissionsRows.Next() { var submission models.PullSubmission var submissionCreatedStr string - var submissionSourceRev sql.NullString + var submissionSourceRev, submissionCombined sql.NullString err := submissionsRows.Scan( &submission.ID, &submission.PullId, &submission.RepoAt, &submission.RoundNumber, &submission.Patch, + &submissionCombined, &submissionCreatedStr, &submissionSourceRev, ) @@ -453,6 +459,10 @@ func GetPull(e Execer, repoAt syntax.ATURI, pullId int) (*models.Pull, error) { submission.SourceRev = submissionSourceRev.String } + if submissionCombined.Valid { + submission.Combined = submissionCombined.String + } + submissionsMap[submission.ID] = &submission } if err = submissionsRows.Close(); err != nil { @@ -674,10 +684,10 @@ func DeletePull(e Execer, repoAt syntax.ATURI, pullId int) error { return err } -func ResubmitPull(e Execer, pull *models.Pull, newPatch, sourceRev string) error { +func ResubmitPull(e Execer, pull *models.Pull, newPatch, combined, sourceRev string) error { newRoundNumber := len(pull.Submissions) _, err := e.Exec(` - insert into pull_submissions (pull_id, repo_at, round_number, patch, source_rev) + insert into pull_submissions (pull_id, repo_at, round_number, patch, combined, source_rev) values (?, ?, ?, ?, ?) `, pull.PullId, pull.RepoAt, newRoundNumber, newPatch, sourceRev) diff --git a/appview/models/pull.go b/appview/models/pull.go index 2826b00b..9688a1f9 100644 --- a/appview/models/pull.go +++ b/appview/models/pull.go @@ -134,6 +134,7 @@ type PullSubmission struct { // 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 @@ -221,6 +222,14 @@ func (s PullSubmission) AsFormatPatch() []types.FormatPatch { return patches } +func (s PullSubmission) CombinedPatch() string { + if s.Combined == "" { + return s.Patch + } + + return s.Combined +} + type Stack []*Pull // position of this pull in the stack diff --git a/appview/pulls/pulls.go b/appview/pulls/pulls.go index 77f35577..d5e057a1 100644 --- a/appview/pulls/pulls.go +++ b/appview/pulls/pulls.go @@ -135,23 +135,6 @@ func (s *Pulls) RepoSinglePull(w http.ResponseWriter, r *http.Request) { 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) resubmitResult := pages.Unknown if user != nil && user.Did == pull.OwnerDid { @@ -374,7 +357,7 @@ func (s *Pulls) RepoPullPatch(w http.ResponseWriter, r *http.Request) { return } - patch := pull.Submissions[roundIdInt].Patch + patch := pull.Submissions[roundIdInt].CombinedPatch() diff := patchutil.AsNiceDiff(patch, pull.TargetBranch) s.pages.RepoPullPatchPage(w, pages.RepoPullPatchParams{ @@ -425,14 +408,14 @@ func (s *Pulls) RepoPullInterdiff(w http.ResponseWriter, r *http.Request) { 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.") @@ -617,13 +600,6 @@ func (s *Pulls) PullComment(w http.ResponseWriter, r *http.Request) { 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) @@ -636,7 +612,7 @@ func (s *Pulls) PullComment(w http.ResponseWriter, r *http.Request) { Rkey: tid.TID(), Record: &lexutil.LexiconTypeDecoder{ Val: &tangled.RepoPullComment{ - Pull: string(pullAt), + Pull: pull.PullAt().String(), Body: body, CreatedAt: createdAt, }, @@ -884,7 +860,8 @@ func (s *Pulls) handleBranchBasedPull( } 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.") @@ -899,7 +876,7 @@ func (s *Pulls) handleBranchBasedPull( 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) { @@ -908,7 +885,7 @@ func (s *Pulls) handlePatchBasedPull(w http.ResponseWriter, r *http.Request, f * 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) { @@ -991,7 +968,8 @@ func (s *Pulls) handleForkBasedPull(w http.ResponseWriter, r *http.Request, f *r } 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.") @@ -1011,7 +989,7 @@ func (s *Pulls) handleForkBasedPull(w http.ResponseWriter, r *http.Request, f *r 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( @@ -1021,6 +999,7 @@ func (s *Pulls) createPullRequest( user *oauth.User, title, body, targetBranch string, patch string, + combined string, sourceRev string, pullSource *models.PullSource, recordPullSource *tangled.RepoPull_Source, @@ -1076,6 +1055,7 @@ func (s *Pulls) createPullRequest( rkey := tid.TID() initialSubmission := models.PullSubmission{ Patch: patch, + Combined: combined, SourceRev: sourceRev, } pull := &models.Pull{ @@ -1504,7 +1484,7 @@ func (s *Pulls) resubmitPatch(w http.ResponseWriter, r *http.Request) { 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) { @@ -1565,9 +1545,10 @@ func (s *Pulls) resubmitBranch(w http.ResponseWriter, r *http.Request) { } 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) { @@ -1660,9 +1641,10 @@ func (s *Pulls) resubmitFork(w http.ResponseWriter, r *http.Request) { 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 @@ -1689,6 +1671,7 @@ func (s *Pulls) resubmitPullHelper( user *oauth.User, pull *models.Pull, patch string, + combined string, sourceRev string, ) { if pull.IsStacked() { @@ -1718,7 +1701,7 @@ func (s *Pulls) resubmitPullHelper( } defer tx.Rollback() - err = db.ResubmitPull(tx, pull, patch, sourceRev) + err = db.ResubmitPull(tx, pull, patch, combined, sourceRev) if err != nil { log.Println("failed to create pull request", err) s.pages.Notice(w, "resubmit-error", "Failed to create pull request. Try again later.") @@ -1933,7 +1916,7 @@ func (s *Pulls) resubmitStackedPullHelper( submission := np.Submissions[np.LastRoundNumber()] // resubmit the old pull - err := db.ResubmitPull(tx, op, submission.Patch, submission.SourceRev) + err := db.ResubmitPull(tx, op, submission.Patch, submission.Combined, submission.SourceRev) if err != nil { log.Println("failed to update pull", err, op.PullId) diff --git a/appview/repo/repo.go b/appview/repo/repo.go index fb47ca4b..882cf3c2 100644 --- a/appview/repo/repo.go +++ b/appview/repo/repo.go @@ -2503,7 +2503,12 @@ func (rp *Repo) RepoCompare(w http.ResponseWriter, r *http.Request) { 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 index 6147e5f9..aeb23a1a 100644 --- a/knotserver/xrpc/repo_compare.go +++ b/knotserver/xrpc/repo_compare.go @@ -4,6 +4,7 @@ import ( "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 @@ func (x *Xrpc) RepoCompare(w http.ResponseWriter, r *http.Request) { 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) diff --git a/types/repo.go b/types/repo.go index e0d3d889..1c530aad 100644 --- 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 RepoCommitResponse struct { } 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 { -- 2.43.0