Cache push-commit comments in the database to improve load performance for pull requests containing a large number of pushed commits.

models/issues/comment.go

@@ -330,12 +330,6 @@ func init() {
 	db.RegisterModel(new(Comment))
 }
 
-// PushActionContent is content of push pull comment
-type PushActionContent struct {
-	IsForcePush bool     `json:"is_force_push"`
-	CommitIDs   []string `json:"commit_ids"`
-}
-
 // LoadIssue loads the issue reference for the comment
 func (c *Comment) LoadIssue(ctx context.Context) (err error) {
 	if c.Issue != nil {

routers/web/repo/issue_view.go

@@ -765,7 +765,7 @@ func prepareIssueViewCommentsAndSidebarParticipants(ctx *context.Context, issue
 			}
 		} else if comment.Type == issues_model.CommentTypePullRequestPush {
 			participants = addParticipant(comment.Poster, participants)
-			if err = issue_service.LoadCommentPushCommits(ctx, comment); err != nil {
+			if err = pull_service.LoadCommentPushCommits(ctx, comment); err != nil {
 				ctx.ServerError("LoadCommentPushCommits", err)
 				return
 			}

services/issue/comments.go

@@ -13,11 +13,7 @@ import (
 	access_model "code.gitea.io/gitea/models/perm/access"
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
-	"code.gitea.io/gitea/modules/gitrepo"
-	"code.gitea.io/gitea/modules/json"
-	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/timeutil"
-	git_service "code.gitea.io/gitea/services/git"
 	notify_service "code.gitea.io/gitea/services/notify"
 )
 
@@ -150,47 +146,3 @@ func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_m
 
 	return nil
 }
-
-// LoadCommentPushCommits Load push commits
-func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) error {
-	if c.Content == "" || c.Commits != nil || c.Type != issues_model.CommentTypePullRequestPush {
-		return nil
-	}
-
-	var data issues_model.PushActionContent
-	if err := json.Unmarshal([]byte(c.Content), &data); err != nil {
-		log.Debug("Unmarshal: %v", err) // no need to show 500 error to end user when the JSON is broken
-		return nil
-	}
-
-	c.IsForcePush = data.IsForcePush
-
-	if c.IsForcePush {
-		if len(data.CommitIDs) != 2 {
-			return nil
-		}
-		c.OldCommit, c.NewCommit = data.CommitIDs[0], data.CommitIDs[1]
-	} else {
-		if err := c.LoadIssue(ctx); err != nil {
-			return err
-		}
-		if err := c.Issue.LoadRepo(ctx); err != nil {
-			return err
-		}
-
-		gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, c.Issue.Repo)
-		if err != nil {
-			return err
-		}
-		defer closer.Close()
-
-		c.Commits, err = git_service.ConvertFromGitCommit(ctx, gitRepo.GetCommitsFromIDs(data.CommitIDs), c.Issue.Repo)
-		if err != nil {
-			log.Debug("ConvertFromGitCommit: %v", err) // no need to show 500 error to end user when the commit does not exist
-		} else {
-			c.CommitsNum = int64(len(c.Commits))
-		}
-	}
-
-	return nil
-}

services/mailer/notify.go

@@ -13,8 +13,8 @@ import (
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/log"
-	issue_service "code.gitea.io/gitea/services/issue"
 	notify_service "code.gitea.io/gitea/services/notify"
+	pull_service "code.gitea.io/gitea/services/pull"
 )
 
 type mailNotifier struct {
@@ -173,7 +173,7 @@ func (m *mailNotifier) PullRequestPushCommits(ctx context.Context, doer *user_mo
 		log.Error("comment.Issue.PullRequest.LoadBaseRepo: %v", err)
 		return
 	}
-	if err := issue_service.LoadCommentPushCommits(ctx, comment); err != nil {
+	if err := pull_service.LoadCommentPushCommits(ctx, comment); err != nil {
 		log.Error("comment.LoadPushCommits: %v", err)
 	}
 	m.CreateIssueComment(ctx, doer, comment.Issue.Repo, comment.Issue, comment, nil)

services/pull/comment.go

@@ -6,11 +6,15 @@ package pull
 import (
 	"context"
 
+	asymkey_model "code.gitea.io/gitea/models/asymkey"
 	issues_model "code.gitea.io/gitea/models/issues"
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/json"
+	"code.gitea.io/gitea/modules/log"
+	git_service "code.gitea.io/gitea/services/git"
 )
 
 // getCommitIDsFromRepo get commit IDs from repo in between oldCommitID and newCommitID
@@ -46,6 +50,140 @@ func getCommitIDsFromRepo(ctx context.Context, repo *repo_model.Repository, oldC
 	return commitIDs, err
 }
 
+type CachedCommitUser struct {
+	ID       int64
+	Name     string
+	FullName string
+	Email    string
+	Avatar   string
+}
+
+func convertUserToCachedCommitUser(u *user_model.User) *CachedCommitUser {
+	if u == nil {
+		return nil
+	}
+	return &CachedCommitUser{
+		ID:       u.ID,
+		Name:     u.Name,
+		FullName: u.FullName,
+		Email:    u.Email,
+		Avatar:   u.Avatar,
+	}
+}
+
+func convertCachedUserToUser(cu *CachedCommitUser) *user_model.User {
+	if cu == nil {
+		return nil
+	}
+	return &user_model.User{
+		ID:       cu.ID,
+		Name:     cu.Name,
+		FullName: cu.FullName,
+		Email:    cu.Email,
+		Avatar:   cu.Avatar,
+	}
+}
+
+// CachedCommit will be stored in database as part of push comment content to reduce
+// disk read when loading push commits later.
+// it will only keep necessary information to display in the timeline of the pull request
+type CachedCommit struct {
+	CommitID      string
+	Author        *git.Signature
+	Committer     *git.Signature
+	CommitMessage string
+	User          *CachedCommitUser
+
+	Verified                 bool
+	Warning                  bool
+	Reason                   string
+	SigningUser              *CachedCommitUser // if Verified, then SigningUser is non-nil
+	CommittingUser           *CachedCommitUser // if Verified, then CommittingUser is non-nil
+	SigningEmail             string
+	SigningGPGKeyID          string
+	SigningSSHKeyFingerprint string
+	TrustStatus              string
+}
+
+func convertCachedCommitsToGitCommits(cachedCommits []CachedCommit) []*asymkey_model.SignCommit {
+	var gitCommits []*asymkey_model.SignCommit
+	for _, cc := range cachedCommits {
+		objectID := git.MustIDFromString(cc.CommitID)
+		signedCommit := &asymkey_model.SignCommit{
+			UserCommit: &user_model.UserCommit{
+				User: convertCachedUserToUser(cc.User),
+				Commit: &git.Commit{
+					ID:            objectID,
+					Author:        cc.Author,
+					Committer:     cc.Committer,
+					CommitMessage: cc.CommitMessage,
+				},
+			},
+			Verification: &asymkey_model.CommitVerification{
+				Verified:       cc.Verified,
+				Warning:        cc.Warning,
+				Reason:         cc.Reason,
+				SigningEmail:   cc.SigningEmail,
+				TrustStatus:    cc.TrustStatus,
+				SigningUser:    convertCachedUserToUser(cc.SigningUser),
+				CommittingUser: convertCachedUserToUser(cc.CommittingUser),
+			},
+		}
+
+		if cc.SigningGPGKeyID != "" {
+			signedCommit.Verification.SigningKey = &asymkey_model.GPGKey{
+				KeyID: cc.SigningGPGKeyID,
+			}
+		} else if cc.SigningSSHKeyFingerprint != "" {
+			signedCommit.Verification.SigningSSHKey = &asymkey_model.PublicKey{
+				Fingerprint: cc.SigningSSHKeyFingerprint,
+			}
+		}
+
+		gitCommits = append(gitCommits, signedCommit)
+	}
+	return gitCommits
+}
+
+func convertGitCommitsToCachedCommits(commits []*asymkey_model.SignCommit) []CachedCommit {
+	var cachedCommits []CachedCommit
+	for _, c := range commits {
+		var signingGPGKeyID, signingSSHKeyFingerprint string
+		if c.Verification != nil {
+			if c.Verification.SigningKey != nil {
+				signingGPGKeyID = c.Verification.SigningKey.KeyID
+			} else if c.Verification.SigningSSHKey != nil {
+				signingSSHKeyFingerprint = c.Verification.SigningSSHKey.Fingerprint
+			}
+		}
+		cachedCommits = append(cachedCommits, CachedCommit{
+			CommitID:      c.ID.String(),
+			Author:        c.Author,
+			Committer:     c.Committer,
+			CommitMessage: c.CommitMessage,
+			User:          convertUserToCachedCommitUser(c.User),
+
+			Verified:                 c.Verification.Verified,
+			Warning:                  c.Verification.Warning,
+			Reason:                   c.Verification.Reason,
+			SigningUser:              convertUserToCachedCommitUser(c.Verification.SigningUser),
+			CommittingUser:           convertUserToCachedCommitUser(c.Verification.CommittingUser),
+			SigningEmail:             c.Verification.SigningEmail,
+			SigningGPGKeyID:          signingGPGKeyID,
+			SigningSSHKeyFingerprint: signingSSHKeyFingerprint,
+			TrustStatus:              c.Verification.TrustStatus,
+		})
+	}
+	return cachedCommits
+}
+
+// PushActionContent is content of push pull comment
+type PushActionContent struct {
+	IsForcePush   bool           `json:"is_force_push"`
+	CommitIDs     []string       `json:"commit_ids"`
+	CachedCommits []CachedCommit `json:"cached_commits"`
+}
+
 // CreatePushPullComment create push code to pull base comment
 func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *issues_model.PullRequest, oldCommitID, newCommitID string, isForcePush bool) (comment *issues_model.Comment, err error) {
 	if pr.HasMerged || oldCommitID == "" || newCommitID == "" {
@@ -60,7 +198,7 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss
 		Issue:       pr.Issue,
 	}
 
-	var data issues_model.PushActionContent
+	var data PushActionContent
 	if opts.IsForcePush {
 		data.CommitIDs = []string{oldCommitID, newCommitID}
 		data.IsForcePush = true
@@ -73,6 +211,28 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss
 		if len(data.CommitIDs) == 0 {
 			return nil, nil
 		}
+
+		gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.BaseRepo)
+		if err != nil {
+			return nil, err
+		}
+		defer closer.Close()
+
+		validatedCommits, err := user_model.ValidateCommitsWithEmails(ctx, gitRepo.GetCommitsFromIDs(data.CommitIDs))
+		if err != nil {
+			return nil, err
+		}
+		signedCommits, err := git_service.ParseCommitsWithSignature(
+			ctx,
+			pr.BaseRepo,
+			validatedCommits,
+			pr.BaseRepo.GetTrustModel(),
+		)
+		if err != nil {
+			return nil, err
+		}
+
+		data.CachedCommits = convertGitCommitsToCachedCommits(signedCommits)
 	}
 
 	dataJSON, err := json.Marshal(data)
@@ -85,3 +245,53 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss
 
 	return comment, err
 }
+
+// LoadCommentPushCommits Load push commits
+func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) error {
+	if c.Content == "" || c.Commits != nil || c.Type != issues_model.CommentTypePullRequestPush {
+		return nil
+	}
+
+	var data PushActionContent
+	if err := json.Unmarshal([]byte(c.Content), &data); err != nil {
+		log.Debug("Unmarshal: %v", err) // no need to show 500 error to end user when the JSON is broken
+		return nil
+	}
+
+	c.IsForcePush = data.IsForcePush
+
+	if c.IsForcePush {
+		if len(data.CommitIDs) != 2 {
+			return nil
+		}
+		c.OldCommit, c.NewCommit = data.CommitIDs[0], data.CommitIDs[1]
+	} else {
+		if err := c.LoadIssue(ctx); err != nil {
+			return err
+		}
+		if err := c.Issue.LoadRepo(ctx); err != nil {
+			return err
+		}
+
+		gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, c.Issue.Repo)
+		if err != nil {
+			return err
+		}
+		defer closer.Close()
+
+		if data.CachedCommits != nil {
+			convertedCommits := convertCachedCommitsToGitCommits(data.CachedCommits)
+			c.Commits, err = git_service.ParseCommitsWithStatus(ctx, convertedCommits, c.Issue.Repo)
+		} else {
+			c.Commits, err = git_service.ConvertFromGitCommit(ctx, gitRepo.GetCommitsFromIDs(data.CommitIDs), c.Issue.Repo)
+		}
+
+		if err != nil {
+			log.Debug("ConvertFromGitCommit: %v", err) // no need to show 500 error to end user when the commit does not exist
+		} else {
+			c.CommitsNum = int64(len(c.Commits))
+		}
+	}
+
+	return nil
+}

tests/integration/pull_comment_test.go

@@ -13,7 +13,7 @@ import (
 
 	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/unittest"
-	issues_service "code.gitea.io/gitea/services/issue"
+	pull_service "code.gitea.io/gitea/services/pull"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -70,7 +70,7 @@ func testPullCommentRebase(t *testing.T, u *url.URL, session *TestSession) {
 	})
 	require.NoError(t, err)
 	lastComment := comments[len(comments)-1]
-	assert.NoError(t, issues_service.LoadCommentPushCommits(t.Context(), lastComment))
+	assert.NoError(t, pull_service.LoadCommentPushCommits(t.Context(), lastComment))
 	assert.True(t, lastComment.IsForcePush)
 }