Filters for GetAllCommits (#24568)

The `GetAllCommits` endpoint can be pretty slow, especially in repos
with a lot of commits. The issue is that it spends a lot of time
calculating information that may not be useful/needed by the user.

The `stat` param was previously added in #21337 to address this, by
allowing the user to disable the calculating stats for each commit. But
this has two issues:
1. The name `stat` is rather misleading, because disabling `stat`
disables the Stat **and** Files. This should be separated out into two
different params, because getting a list of affected files is much less
expensive than calculating the stats
2. There's still other costly information provided that the user may not
need, such as `Verification`

This PR, adds two parameters to the endpoint, `files` and `verification`
to allow the user to explicitly disable this information when listing
commits. The default behavior is true.

Author: mattwalo32

routers/api/v1/repo/commits.go

@@ -69,7 +69,7 @@ func getCommit(ctx *context.APIContext, identifier string) {
 		return
 	}
 
-	json, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, commit, nil, true)
+	json, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, commit, nil, convert.ToCommitOptions{Stat: true})
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "toCommit", err)
 		return
@@ -107,6 +107,14 @@ func GetAllCommits(ctx *context.APIContext) {
 	//   in: query
 	//   description: include diff stats for every commit (disable for speedup, default 'true')
 	//   type: boolean
+	// - name: verification
+	//   in: query
+	//   description: include verification for every commit (disable for speedup, default 'true')
+	//   type: boolean
+	// - name: files
+	//   in: query
+	//   description: include a list of affected files for every commit (disable for speedup, default 'true')
+	//   type: boolean
 	// - name: page
 	//   in: query
 	//   description: page number of results to return (1-based)
@@ -238,10 +246,18 @@ func GetAllCommits(ctx *context.APIContext) {
 	apiCommits := make([]*api.Commit, len(commits))
 
 	stat := ctx.FormString("stat") == "" || ctx.FormBool("stat")
+	verification := ctx.FormString("verification") == "" || ctx.FormBool("verification")
+	files := ctx.FormString("files") == "" || ctx.FormBool("files")
 
 	for i, commit := range commits {
 		// Create json struct
-		apiCommits[i], err = convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, commit, userCache, stat)
+		apiCommits[i], err = convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, commit, userCache,
+			convert.ToCommitOptions{
+				Stat:         stat,
+				Verification: verification,
+				Files:        files,
+			})
+
 		if err != nil {
 			ctx.Error(http.StatusInternalServerError, "toCommit", err)
 			return

routers/api/v1/repo/notes.go

@@ -78,7 +78,7 @@ func getNote(ctx *context.APIContext, identifier string) {
 		return
 	}
 
-	cmt, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, note.Commit, nil, true)
+	cmt, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, note.Commit, nil, convert.ToCommitOptions{Stat: true})
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "ToCommit", err)
 		return

routers/api/v1/repo/pull.go

@@ -1318,7 +1318,7 @@ func GetPullRequestCommits(ctx *context.APIContext) {
 
 	apiCommits := make([]*api.Commit, 0, end-start)
 	for i := start; i < end; i++ {
-		apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, baseGitRepo, commits[i], userCache, true)
+		apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, baseGitRepo, commits[i], userCache, convert.ToCommitOptions{Stat: true})
 		if err != nil {
 			ctx.ServerError("toCommit", err)
 			return

services/convert/git_commit.go

@@ -72,8 +72,14 @@ func ToPayloadCommit(ctx context.Context, repo *repo_model.Repository, c *git.Co
 	}
 }
 
+type ToCommitOptions struct {
+	Stat         bool
+	Verification bool
+	Files        bool
+}
+
 // ToCommit convert a git.Commit to api.Commit
-func ToCommit(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, commit *git.Commit, userCache map[string]*user_model.User, stat bool) (*api.Commit, error) {
+func ToCommit(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, commit *git.Commit, userCache map[string]*user_model.User, opts ToCommitOptions) (*api.Commit, error) {
 	var apiAuthor, apiCommitter *api.User
 
 	// Retrieve author and committer information
@@ -162,19 +168,24 @@ func ToCommit(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Rep
 				SHA:     commit.ID.String(),
 				Created: commit.Committer.When,
 			},
-			Verification: ToVerification(ctx, commit),
 		},
 		Author:    apiAuthor,
 		Committer: apiCommitter,
 		Parents:   apiParents,
 	}
 
+	// Retrieve verification for commit
+	if opts.Verification {
+		res.RepoCommit.Verification = ToVerification(ctx, commit)
+	}
+
 	// Retrieve files affected by the commit
-	if stat {
+	if opts.Files {
 		fileStatus, err := git.GetCommitFileStatus(gitRepo.Ctx, repo.RepoPath(), commit.ID.String())
 		if err != nil {
 			return nil, err
 		}
+
 		affectedFileList := make([]*api.CommitAffectedFiles, 0, len(fileStatus.Added)+len(fileStatus.Removed)+len(fileStatus.Modified))
 		for _, files := range [][]string{fileStatus.Added, fileStatus.Removed, fileStatus.Modified} {
 			for _, filename := range files {
@@ -184,14 +195,18 @@ func ToCommit(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Rep
 			}
 		}
 
+		res.Files = affectedFileList
+	}
+
+	// Get diff stats for commit
+	if opts.Stat {
 		diff, err := gitdiff.GetDiff(gitRepo, &gitdiff.DiffOptions{
 			AfterCommitID: commit.ID.String(),
 		})
 		if err != nil {
 			return nil, err
 		}
 
-		res.Files = affectedFileList
 		res.Stats = &api.CommitStats{
 			Total:     diff.TotalAddition + diff.TotalDeletion,
 			Additions: diff.TotalAddition,

templates/swagger/v1_json.tmpl

@@ -135,6 +135,27 @@ func TestAPIReposGitCommitListDifferentBranch(t *testing.T) {
 	compareCommitFiles(t, []string{"readme.md"}, apiData[0].Files)
 }
 
+func TestAPIReposGitCommitListWithoutSelectFields(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	// Login as User2.
+	session := loginUser(t, user.Name)
+	token := getTokenForLoggedInUser(t, session)
+
+	// Test getting commits without files, verification, and stats
+	req := NewRequestf(t, "GET", "/api/v1/repos/%s/repo16/commits?token="+token+"&sha=good-sign&stat=false&files=false&verification=false", user.Name)
+	resp := MakeRequest(t, req, http.StatusOK)
+
+	var apiData []api.Commit
+	DecodeJSON(t, resp, &apiData)
+
+	assert.Len(t, apiData, 1)
+	assert.Equal(t, "f27c2b2b03dcab38beaf89b0ab4ff61f6de63441", apiData[0].CommitMeta.SHA)
+	assert.Equal(t, (*api.CommitStats)(nil), apiData[0].Stats)
+	assert.Equal(t, (*api.PayloadCommitVerification)(nil), apiData[0].RepoCommit.Verification)
+	assert.Equal(t, ([]*api.CommitAffectedFiles)(nil), apiData[0].Files)
+}
+
 func TestDownloadCommitDiffOrPatch(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})