From 808807f5bfc8a894113e7666939685a8d5cf69af 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 | 38 +++++++++++--------- 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, 95 insertions(+), 67 deletions(-) diff --git a/appview/db/db.go b/appview/db/db.go index 8cd3fac9..21f2c20c 100644 --- a/appview/db/db.go +++ b/appview/db/db.go @@ -1097,6 +1097,15 @@ func Make(ctx context.Context, dbPath string) (*DB, error) { }) 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 index cacb6c42..a32079ca 100644 --- a/appview/db/pulls.go +++ b/appview/db/pulls.go @@ -90,9 +90,9 @@ func NewPull(tx *sql.Tx, pull *models.Pull) error { 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 @@ func GetPullSubmissions(e Execer, filters ...filter) (map[syntax.ATURI][]*models pull_at, round_number, patch, + combined, created, source_rev from @@ -332,28 +333,31 @@ func GetPullSubmissions(e Execer, filters ...filter) (map[syntax.ATURI][]*models 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 + } + + if submissionSourceRev.Valid { + submission.SourceRev = submissionSourceRev.String } - submission.Created = createdTime - if sourceRev.Valid { - submission.SourceRev = sourceRev.String + if submissionCombined.Valid { + submission.Combined = submissionCombined.String } submissionMap[submission.ID] = &submission @@ -590,12 +594,12 @@ 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_at, round_number, patch, source_rev) - values (?, ?, ?, ?) - `, pull.PullAt(), newRoundNumber, newPatch, sourceRev) + insert into pull_submissions (pull_at, round_number, patch, combined, source_rev) + values (?, ?, ?, ?, ?) + `, pull.PullAt(), newRoundNumber, newPatch, combined, sourceRev) return err } diff --git a/appview/models/pull.go b/appview/models/pull.go index 3902e100..7e65ffbe 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 @@ -263,6 +264,14 @@ func (s *PullSubmission) Participants() []string { return participants } +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 d9dd9da3..f48e3f5f 100644 --- a/appview/pulls/pulls.go +++ b/appview/pulls/pulls.go @@ -146,23 +146,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) branchDeleteStatus := s.branchDeleteStatus(r, f, pull) resubmitResult := pages.Unknown @@ -460,7 +443,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{ @@ -511,14 +494,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.") @@ -720,13 +703,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) @@ -739,7 +715,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, }, @@ -987,7 +963,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.") @@ -1002,7 +979,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) { @@ -1011,7 +988,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) { @@ -1094,7 +1071,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.") @@ -1114,7 +1092,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( @@ -1124,6 +1102,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, @@ -1183,6 +1162,7 @@ func (s *Pulls) createPullRequest( rkey := tid.TID() initialSubmission := models.PullSubmission{ Patch: patch, + Combined: combined, SourceRev: sourceRev, } pull := &models.Pull{ @@ -1612,7 +1592,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) { @@ -1673,9 +1653,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) { @@ -1768,9 +1749,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 @@ -1797,6 +1779,7 @@ func (s *Pulls) resubmitPullHelper( user *oauth.User, pull *models.Pull, patch string, + combined string, sourceRev string, ) { if pull.IsStacked() { @@ -1826,7 +1809,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.") @@ -2042,7 +2025,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 8e87320c..6917dd4d 100644 --- a/appview/repo/repo.go +++ b/appview/repo/repo.go @@ -2565,7 +2565,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 3f479089..18bfef2e 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