Improve commits list performance to reduce unnecessary database queries (#33528)

When listing commits, Gitea attempts to retrieve the actual user based
on the commit email. Querying users one by one from the database is
inefficient. This PR optimizes the process by batch querying users by
email, reducing the number of database queries.

models/asymkey/gpg_key_commit_verification.go

@@ -12,6 +12,7 @@ import (
 	"code.gitea.io/gitea/models/db"
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/container"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
@@ -71,21 +72,41 @@ const (
 )
 
 // ParseCommitsWithSignature checks if signaute of commits are corresponding to users gpg keys.
-func ParseCommitsWithSignature(ctx context.Context, oldCommits []*user_model.UserCommit, repoTrustModel repo_model.TrustModelType, isOwnerMemberCollaborator func(*user_model.User) (bool, error)) []*SignCommit {
+func ParseCommitsWithSignature(ctx context.Context, oldCommits []*user_model.UserCommit, repoTrustModel repo_model.TrustModelType, isOwnerMemberCollaborator func(*user_model.User) (bool, error)) ([]*SignCommit, error) {
 	newCommits := make([]*SignCommit, 0, len(oldCommits))
 	keyMap := map[string]bool{}
 
+	emails := make(container.Set[string])
 	for _, c := range oldCommits {
+		if c.Committer != nil {
+			emails.Add(c.Committer.Email)
+		}
+	}
+
+	emailUsers, err := user_model.GetUsersByEmails(ctx, emails.Values())
+	if err != nil {
+		return nil, err
+	}
+
+	for _, c := range oldCommits {
+		committer, ok := emailUsers[c.Committer.Email]
+		if !ok && c.Committer != nil {
+			committer = &user_model.User{
+				Name:  c.Committer.Name,
+				Email: c.Committer.Email,
+			}
+		}
+
 		signCommit := &SignCommit{
 			UserCommit:   c,
-			Verification: ParseCommitWithSignature(ctx, c.Commit),
+			Verification: parseCommitWithSignatureCommitter(ctx, c.Commit, committer),
 		}
 
 		_ = CalculateTrustStatus(signCommit.Verification, repoTrustModel, isOwnerMemberCollaborator, &keyMap)
 
 		newCommits = append(newCommits, signCommit)
 	}
-	return newCommits
+	return newCommits, nil
 }
 
 // ParseCommitWithSignature check if signature is good against keystore.
@@ -113,6 +134,10 @@ func ParseCommitWithSignature(ctx context.Context, c *git.Commit) *CommitVerific
 		}
 	}
 
+	return parseCommitWithSignatureCommitter(ctx, c, committer)
+}
+
+func parseCommitWithSignatureCommitter(ctx context.Context, c *git.Commit, committer *user_model.User) *CommitVerification {
 	// If no signature just report the committer
 	if c.Signature == nil {
 		return &CommitVerification{

models/git/commit_status.go

@@ -497,7 +497,7 @@ type SignCommitWithStatuses struct {
 }
 
 // ParseCommitsWithStatus checks commits latest statuses and calculates its worst status state
-func ParseCommitsWithStatus(ctx context.Context, oldCommits []*asymkey_model.SignCommit, repo *repo_model.Repository) []*SignCommitWithStatuses {
+func ParseCommitsWithStatus(ctx context.Context, oldCommits []*asymkey_model.SignCommit, repo *repo_model.Repository) ([]*SignCommitWithStatuses, error) {
 	newCommits := make([]*SignCommitWithStatuses, 0, len(oldCommits))
 
 	for _, c := range oldCommits {
@@ -506,15 +506,14 @@ func ParseCommitsWithStatus(ctx context.Context, oldCommits []*asymkey_model.Sig
 		}
 		statuses, _, err := GetLatestCommitStatus(ctx, repo.ID, commit.ID.String(), db.ListOptions{})
 		if err != nil {
-			log.Error("GetLatestCommitStatus: %v", err)
-		} else {
-			commit.Statuses = statuses
-			commit.Status = CalcCommitStatus(statuses)
+			return nil, err
 		}
 
+		commit.Statuses = statuses
+		commit.Status = CalcCommitStatus(statuses)
 		newCommits = append(newCommits, commit)
 	}
-	return newCommits
+	return newCommits, nil
 }
 
 // hashCommitStatusContext hash context
@@ -523,18 +522,23 @@ func hashCommitStatusContext(context string) string {
 }
 
 // ConvertFromGitCommit converts git commits into SignCommitWithStatuses
-func ConvertFromGitCommit(ctx context.Context, commits []*git.Commit, repo *repo_model.Repository) []*SignCommitWithStatuses {
-	return ParseCommitsWithStatus(ctx,
-		asymkey_model.ParseCommitsWithSignature(
+func ConvertFromGitCommit(ctx context.Context, commits []*git.Commit, repo *repo_model.Repository) ([]*SignCommitWithStatuses, error) {
+	validatedCommits, err := user_model.ValidateCommitsWithEmails(ctx, commits)
+	if err != nil {
+		return nil, err
+	}
+	signedCommits, err := asymkey_model.ParseCommitsWithSignature(
 		ctx,
-			user_model.ValidateCommitsWithEmails(ctx, commits),
+		validatedCommits,
 		repo.GetTrustModel(),
 		func(user *user_model.User) (bool, error) {
 			return repo_model.IsOwnerMemberCollaborator(ctx, repo, user.ID)
 		},
-		),
-		repo,
 	)
+	if err != nil {
+		return nil, err
+	}
+	return ParseCommitsWithStatus(ctx, signedCommits, repo)
 }
 
 // CommitStatusesHideActionsURL hide Gitea Actions urls

models/issues/comment.go

@@ -802,7 +802,10 @@ func (c *Comment) LoadPushCommits(ctx context.Context) (err error) {
 		}
 		defer closer.Close()
 
-		c.Commits = git_model.ConvertFromGitCommit(ctx, gitRepo.GetCommitsFromIDs(data.CommitIDs), c.Issue.Repo)
+		c.Commits, err = git_model.ConvertFromGitCommit(ctx, gitRepo.GetCommitsFromIDs(data.CommitIDs), c.Issue.Repo)
+		if err != nil {
+			return err
+		}
 		c.CommitsNum = int64(len(c.Commits))
 	}
 

models/user/user.go

@@ -1129,28 +1129,85 @@ func ValidateCommitWithEmail(ctx context.Context, c *git.Commit) *User {
 }
 
 // ValidateCommitsWithEmails checks if authors' e-mails of commits are corresponding to users.
-func ValidateCommitsWithEmails(ctx context.Context, oldCommits []*git.Commit) []*UserCommit {
+func ValidateCommitsWithEmails(ctx context.Context, oldCommits []*git.Commit) ([]*UserCommit, error) {
 	var (
-		emails     = make(map[string]*User)
 		newCommits = make([]*UserCommit, 0, len(oldCommits))
+		emailSet   = make(container.Set[string])
 	)
 	for _, c := range oldCommits {
-		var u *User
 		if c.Author != nil {
-			if v, ok := emails[c.Author.Email]; !ok {
-				u, _ = GetUserByEmail(ctx, c.Author.Email)
-				emails[c.Author.Email] = u
-			} else {
-				u = v
+			emailSet.Add(c.Author.Email)
+		}
 	}
+
+	emailUserMap, err := GetUsersByEmails(ctx, emailSet.Values())
+	if err != nil {
+		return nil, err
 	}
 
+	for _, c := range oldCommits {
+		user, ok := emailUserMap[c.Author.Email]
+		if !ok {
+			user = &User{
+				Name:  c.Author.Name,
+				Email: c.Author.Email,
+			}
+		}
 		newCommits = append(newCommits, &UserCommit{
-			User:   u,
+			User:   user,
 			Commit: c,
 		})
 	}
-	return newCommits
+	return newCommits, nil
+}
+
+func GetUsersByEmails(ctx context.Context, emails []string) (map[string]*User, error) {
+	if len(emails) == 0 {
+		return nil, nil
+	}
+
+	needCheckEmails := make(container.Set[string])
+	needCheckUserNames := make(container.Set[string])
+	for _, email := range emails {
+		if strings.HasSuffix(email, fmt.Sprintf("@%s", setting.Service.NoReplyAddress)) {
+			username := strings.TrimSuffix(email, fmt.Sprintf("@%s", setting.Service.NoReplyAddress))
+			needCheckUserNames.Add(username)
+		} else {
+			needCheckEmails.Add(strings.ToLower(email))
+		}
+	}
+
+	emailAddresses := make([]*EmailAddress, 0, len(needCheckEmails))
+	if err := db.GetEngine(ctx).In("lower_email", needCheckEmails.Values()).
+		And("is_activated=?", true).
+		Find(&emailAddresses); err != nil {
+		return nil, err
+	}
+	userIDs := make(container.Set[int64])
+	for _, email := range emailAddresses {
+		userIDs.Add(email.UID)
+	}
+	users, err := GetUsersByIDs(ctx, userIDs.Values())
+	if err != nil {
+		return nil, err
+	}
+
+	results := make(map[string]*User, len(emails))
+	for _, user := range users {
+		if user.KeepEmailPrivate {
+			results[user.LowerName+"@"+setting.Service.NoReplyAddress] = user
+		} else {
+			results[user.Email] = user
+		}
+	}
+	users = make([]*User, 0, len(needCheckUserNames))
+	if err := db.GetEngine(ctx).In("lower_name", needCheckUserNames.Values()).Find(&users); err != nil {
+		return nil, err
+	}
+	for _, user := range users {
+		results[user.LowerName+"@"+setting.Service.NoReplyAddress] = user
+	}
+	return results, nil
 }
 
 // GetUserByEmail returns the user object by given e-mail if exists.

routers/web/repo/blame.go

@@ -234,7 +234,12 @@ func processBlameParts(ctx *context.Context, blameParts []*git.BlamePart) map[st
 	}
 
 	// populate commit email addresses to later look up avatars.
-	for _, c := range user_model.ValidateCommitsWithEmails(ctx, commits) {
+	validatedCommits, err := user_model.ValidateCommitsWithEmails(ctx, commits)
+	if err != nil {
+		ctx.ServerError("ValidateCommitsWithEmails", err)
+		return nil
+	}
+	for _, c := range validatedCommits {
 		commitNames[c.ID.String()] = c
 	}
 

routers/web/repo/commit.go

@@ -80,7 +80,11 @@ func Commits(ctx *context.Context) {
 		ctx.ServerError("CommitsByRange", err)
 		return
 	}
-	ctx.Data["Commits"] = processGitCommits(ctx, commits)
+	ctx.Data["Commits"], err = processGitCommits(ctx, commits)
+	if err != nil {
+		ctx.ServerError("processGitCommits", err)
+		return
+	}
 	commitIDs := make([]string, 0, len(commits))
 	for _, c := range commits {
 		commitIDs = append(commitIDs, c.ID.String())
@@ -192,7 +196,11 @@ func SearchCommits(ctx *context.Context) {
 		return
 	}
 	ctx.Data["CommitCount"] = len(commits)
-	ctx.Data["Commits"] = processGitCommits(ctx, commits)
+	ctx.Data["Commits"], err = processGitCommits(ctx, commits)
+	if err != nil {
+		ctx.ServerError("processGitCommits", err)
+		return
+	}
 
 	ctx.Data["Keyword"] = query
 	if all {
@@ -235,7 +243,11 @@ func FileHistory(ctx *context.Context) {
 		ctx.ServerError("CommitsByFileAndRange", err)
 		return
 	}
-	ctx.Data["Commits"] = processGitCommits(ctx, commits)
+	ctx.Data["Commits"], err = processGitCommits(ctx, commits)
+	if err != nil {
+		ctx.ServerError("processGitCommits", err)
+		return
+	}
 
 	ctx.Data["Username"] = ctx.Repo.Owner.Name
 	ctx.Data["Reponame"] = ctx.Repo.Repository.Name
@@ -416,13 +428,16 @@ func RawDiff(ctx *context.Context) {
 	}
 }
 
-func processGitCommits(ctx *context.Context, gitCommits []*git.Commit) []*git_model.SignCommitWithStatuses {
-	commits := git_model.ConvertFromGitCommit(ctx, gitCommits, ctx.Repo.Repository)
+func processGitCommits(ctx *context.Context, gitCommits []*git.Commit) ([]*git_model.SignCommitWithStatuses, error) {
+	commits, err := git_model.ConvertFromGitCommit(ctx, gitCommits, ctx.Repo.Repository)
+	if err != nil {
+		return nil, err
+	}
 	if !ctx.Repo.CanRead(unit_model.TypeActions) {
 		for _, commit := range commits {
 			commit.Status.HideActionsURL(ctx)
 			git_model.CommitStatusesHideActionsURL(ctx, commit.Statuses)
 		}
 	}
-	return commits
+	return commits, nil
 }

routers/web/repo/compare.go

@@ -647,7 +647,11 @@ func PrepareCompareDiff(
 		return false
 	}
 
-	commits := processGitCommits(ctx, ci.CompareInfo.Commits)
+	commits, err := processGitCommits(ctx, ci.CompareInfo.Commits)
+	if err != nil {
+		ctx.ServerError("processGitCommits", err)
+		return false
+	}
 	ctx.Data["Commits"] = commits
 	ctx.Data["CommitCount"] = len(commits)
 

routers/web/repo/pull.go

@@ -631,7 +631,11 @@ func ViewPullCommits(ctx *context.Context) {
 	ctx.Data["Username"] = ctx.Repo.Owner.Name
 	ctx.Data["Reponame"] = ctx.Repo.Repository.Name
 
-	commits := processGitCommits(ctx, prInfo.Commits)
+	commits, err := processGitCommits(ctx, prInfo.Commits)
+	if err != nil {
+		ctx.ServerError("processGitCommits", err)
+		return
+	}
 	ctx.Data["Commits"] = commits
 	ctx.Data["CommitCount"] = len(commits)
 

routers/web/repo/wiki.go

@@ -437,7 +437,14 @@ func renderRevisionPage(ctx *context.Context) (*git.Repository, *git.TreeEntry)
 		ctx.ServerError("CommitsByFileAndRange", err)
 		return nil, nil
 	}
-	ctx.Data["Commits"] = git_model.ConvertFromGitCommit(ctx, commitsHistory, ctx.Repo.Repository)
+	ctx.Data["Commits"], err = git_model.ConvertFromGitCommit(ctx, commitsHistory, ctx.Repo.Repository)
+	if err != nil {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
+		ctx.ServerError("ConvertFromGitCommit", err)
+		return nil, nil
+	}
 
 	pager := context.NewPagination(int(commitsCount), setting.Git.CommitsRangeSize, page, 5)
 	pager.AddParamFromRequest(ctx.Req)

tests/integration/repo_commits_test.go

@@ -12,11 +12,14 @@ import (
 	"testing"
 
 	auth_model "code.gitea.io/gitea/models/auth"
+	"code.gitea.io/gitea/models/unittest"
+	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/tests"
 
+	"github.com/PuerkitoBio/goquery"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -35,6 +38,44 @@ func TestRepoCommits(t *testing.T) {
 	assert.NotEmpty(t, commitURL)
 }
 
+func Test_ReposGitCommitListNotMaster(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	// Login as User2.
+	session := loginUser(t, user.Name)
+
+	// Test getting commits (Page 1)
+	req := NewRequestf(t, "GET", "/%s/repo16/commits/branch/master", user.Name)
+	resp := session.MakeRequest(t, req, http.StatusOK)
+
+	doc := NewHTMLParser(t, resp.Body)
+	commits := []string{}
+	doc.doc.Find("#commits-table .commit-id-short").Each(func(i int, s *goquery.Selection) {
+		commitURL, exists := s.Attr("href")
+		assert.True(t, exists)
+		assert.NotEmpty(t, commitURL)
+		commits = append(commits, path.Base(commitURL))
+	})
+
+	assert.Len(t, commits, 3)
+	assert.EqualValues(t, "69554a64c1e6030f051e5c3f94bfbd773cd6a324", commits[0])
+	assert.EqualValues(t, "27566bd5738fc8b4e3fef3c5e72cce608537bd95", commits[1])
+	assert.EqualValues(t, "5099b81332712fe655e34e8dd63574f503f61811", commits[2])
+
+	userNames := []string{}
+	doc.doc.Find("#commits-table .author-wrapper").Each(func(i int, s *goquery.Selection) {
+		userPath, exists := s.Attr("href")
+		assert.True(t, exists)
+		assert.NotEmpty(t, userPath)
+		userNames = append(userNames, path.Base(userPath))
+	})
+
+	assert.Len(t, userNames, 3)
+	assert.EqualValues(t, "User2", userNames[0])
+	assert.EqualValues(t, "user21", userNames[1])
+	assert.EqualValues(t, "User2", userNames[2])
+}
+
 func doTestRepoCommitWithStatus(t *testing.T, state string, classes ...string) {
 	defer tests.PrepareTestEnv(t)()