Skip to content

Commit c6a07ee

Browse files
wxiaoguangwxiaoguang
committed
do not run "git diff --shortstat" for pull list
1 parent 40bd121 commit c6a07ee

File tree

5 files changed

+44
-56
lines changed

5 files changed

+44
-56
lines changed

‎modules/git/repo_compare.go

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -174,17 +174,8 @@ func (repo *Repository) GetDiffNumChangedFiles(base, head string, directComparis
174174
return w.numLines, nil
175175
}
176176

177-
// GetDiffShortStat counts number of changed files, number of additions and deletions
178-
func (repo *Repository) GetDiffShortStat(base, head string) (numFiles, totalAdditions, totalDeletions int, err error) {
179-
numFiles, totalAdditions, totalDeletions, err = GetDiffShortStatByCmdArgs(repo.Ctx, repo.Path, nil, base+"..."+head)
180-
if err != nil && strings.Contains(err.Error(), "no merge base") {
181-
return GetDiffShortStatByCmdArgs(repo.Ctx, repo.Path, nil, base, head)
182-
}
183-
return numFiles, totalAdditions, totalDeletions, err
184-
}
185-
186177
// GetDiffShortStatByCmdArgs counts number of changed files, number of additions and deletions
187-
// TODO: there are already 2 other different "GetDiffShortStat" functions in code base, they do similar things, need to refactor in the future
178+
// TODO: it can be merged with another "GetDiffShortStat" in the future
188179
func GetDiffShortStatByCmdArgs(ctx context.Context, repoPath string, trustedArgs TrustedCmdArgs, dynamicArgs ...string) (numFiles, totalAdditions, totalDeletions int, err error) {
189180
// Now if we call:
190181
// $ git diff --shortstat 1ebb35b98889ff77299f24d82da426b434b0cca0...788b8b1440462d477f45b0088875

‎modules/structs/pull.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,13 @@ type PullRequest struct {
2525
Draft bool `json:"draft"`
2626
IsLocked bool `json:"is_locked"`
2727
Comments int `json:"comments"`
28+
2829
// number of review comments made on the diff of a PR review (not including comments on commits or issues in a PR)
29-
ReviewComments int `json:"review_comments"`
30-
Additions int `json:"additions"`
31-
Deletions int `json:"deletions"`
32-
ChangedFiles int `json:"changed_files"`
30+
ReviewComments int `json:"review_comments,omitempty"`
31+
32+
Additions *int `json:"additions,omitempty"`
33+
Deletions *int `json:"deletions,omitempty"`
34+
ChangedFiles *int `json:"changed_files,omitempty"`
3335

3436
HTMLURL string `json:"html_url"`
3537
DiffURL string `json:"diff_url"`

‎services/convert/pull.go

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,10 @@ import (
1717
"code.gitea.io/gitea/modules/git"
1818
"code.gitea.io/gitea/modules/gitrepo"
1919
"code.gitea.io/gitea/modules/log"
20+
"code.gitea.io/gitea/modules/setting"
2021
api "code.gitea.io/gitea/modules/structs"
2122
"code.gitea.io/gitea/modules/util"
23+
"code.gitea.io/gitea/services/gitdiff"
2224
)
2325

2426
// ToAPIPullRequest assumes following fields have been assigned with valid values:
@@ -239,9 +241,13 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u
239241
// Calculate diff
240242
startCommitID = pr.MergeBase
241243

242-
apiPullRequest.ChangedFiles, apiPullRequest.Additions, apiPullRequest.Deletions, err = gitRepo.GetDiffShortStat(startCommitID, endCommitID)
244+
diffShortStats, err := gitdiff.GetDiffShortStat(gitRepo, startCommitID, endCommitID)
243245
if err != nil {
244246
log.Error("GetDiffShortStat: %v", err)
247+
} else {
248+
apiPullRequest.ChangedFiles = &diffShortStats.NumFiles
249+
apiPullRequest.Additions = &diffShortStats.TotalAddition
250+
apiPullRequest.Deletions = &diffShortStats.TotalDeletion
245251
}
246252
}
247253

@@ -462,12 +468,6 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs
462468
return nil, err
463469
}
464470

465-
// Outer scope variables to be used in diff calculation
466-
var (
467-
startCommitID string
468-
endCommitID string
469-
)
470-
471471
if git.IsErrBranchNotExist(err) {
472472
headCommitID, err := headGitRepo.GetRefCommitID(apiPullRequest.Head.Ref)
473473
if err != nil && !git.IsErrNotExist(err) {
@@ -476,7 +476,6 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs
476476
}
477477
if err == nil {
478478
apiPullRequest.Head.Sha = headCommitID
479-
endCommitID = headCommitID
480479
}
481480
} else {
482481
commit, err := headBranch.GetCommit()
@@ -487,19 +486,8 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs
487486
if err == nil {
488487
apiPullRequest.Head.Ref = pr.HeadBranch
489488
apiPullRequest.Head.Sha = commit.ID.String()
490-
endCommitID = commit.ID.String()
491489
}
492490
}
493-
494-
// Calculate diff
495-
startCommitID = pr.MergeBase
496-
497-
// FIXME: it causes performance regressions, because in many cases end users do not need these information
498-
// But "git diff --shortstat" is slow on large repositories, this call makes the API slow
499-
apiPullRequest.ChangedFiles, apiPullRequest.Additions, apiPullRequest.Deletions, err = gitRepo.GetDiffShortStat(startCommitID, endCommitID)
500-
if err != nil {
501-
log.Error("GetDiffShortStat: %v", err)
502-
}
503491
}
504492

505493
if len(apiPullRequest.Head.Sha) == 0 && len(apiPullRequest.Head.Ref) != 0 {
@@ -520,6 +508,12 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs
520508
apiPullRequest.MergedBy = ToUser(ctx, pr.Merger, nil)
521509
}
522510

511+
// Do not provide "ChangeFiles/Additions/Deletions" for the PR list, because the "diff" is quite slow
512+
// If callers are interested in these values, they should do a separate request to get the PR details
513+
if apiPullRequest.ChangedFiles != nil || apiPullRequest.Additions != nil || apiPullRequest.Deletions != nil {
514+
setting.PanicInDevOrTesting("ChangedFiles/Additions/Deletions should not be set in PR list")
515+
}
516+
523517
apiPullRequests = append(apiPullRequests, apiPullRequest)
524518
}
525519

‎tests/integration/api_pull_test.go

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -50,30 +50,30 @@ func TestAPIViewPulls(t *testing.T) {
5050
assert.Empty(t, pull.RequestedReviewersTeams)
5151
assert.EqualValues(t, 5, pull.RequestedReviewers[0].ID)
5252
assert.EqualValues(t, 6, pull.RequestedReviewers[1].ID)
53-
assert.EqualValues(t, 1, pull.ChangedFiles)
5453

5554
if assert.EqualValues(t, 5, pull.ID) {
5655
resp = ctx.Session.MakeRequest(t, NewRequest(t, "GET", pull.DiffURL), http.StatusOK)
5756
bs, err := io.ReadAll(resp.Body)
5857
assert.NoError(t, err)
5958
patch, err := gitdiff.ParsePatch(t.Context(), 1000, 5000, 10, bytes.NewReader(bs), "")
6059
assert.NoError(t, err)
61-
if assert.Len(t, patch.Files, pull.ChangedFiles) {
60+
if assert.Len(t, patch.Files, 1) {
6261
assert.Equal(t, "File-WoW", patch.Files[0].Name)
6362
// FIXME: The old name should be empty if it's a file add type
6463
assert.Equal(t, "File-WoW", patch.Files[0].OldName)
65-
assert.EqualValues(t, pull.Additions, patch.Files[0].Addition)
66-
assert.EqualValues(t, pull.Deletions, patch.Files[0].Deletion)
64+
assert.EqualValues(t, 1, patch.Files[0].Addition)
65+
assert.EqualValues(t, 0, patch.Files[0].Deletion)
6766
assert.Equal(t, gitdiff.DiffFileAdd, patch.Files[0].Type)
6867
}
6968

7069
t.Run(fmt.Sprintf("APIGetPullFiles_%d", pull.ID),
7170
doAPIGetPullFiles(ctx, pull, func(t *testing.T, files []*api.ChangedFile) {
72-
if assert.Len(t, files, pull.ChangedFiles) {
71+
if assert.Len(t, files, 1) {
7372
assert.Equal(t, "File-WoW", files[0].Filename)
7473
assert.Empty(t, files[0].PreviousFilename)
75-
assert.EqualValues(t, pull.Additions, files[0].Additions)
76-
assert.EqualValues(t, pull.Deletions, files[0].Deletions)
74+
assert.EqualValues(t, 1, files[0].Additions)
75+
assert.EqualValues(t, 1, files[0].Changes)
76+
assert.EqualValues(t, 0, files[0].Deletions)
7777
assert.Equal(t, "added", files[0].Status)
7878
}
7979
}))
@@ -87,53 +87,51 @@ func TestAPIViewPulls(t *testing.T) {
8787
assert.EqualValues(t, 4, pull.RequestedReviewers[1].ID)
8888
assert.EqualValues(t, 2, pull.RequestedReviewers[2].ID)
8989
assert.EqualValues(t, 5, pull.RequestedReviewers[3].ID)
90-
assert.EqualValues(t, 1, pull.ChangedFiles)
9190

9291
if assert.EqualValues(t, 2, pull.ID) {
9392
resp = ctx.Session.MakeRequest(t, NewRequest(t, "GET", pull.DiffURL), http.StatusOK)
9493
bs, err := io.ReadAll(resp.Body)
9594
assert.NoError(t, err)
9695
patch, err := gitdiff.ParsePatch(t.Context(), 1000, 5000, 10, bytes.NewReader(bs), "")
9796
assert.NoError(t, err)
98-
if assert.Len(t, patch.Files, pull.ChangedFiles) {
97+
if assert.Len(t, patch.Files, 1) {
9998
assert.Equal(t, "README.md", patch.Files[0].Name)
10099
assert.Equal(t, "README.md", patch.Files[0].OldName)
101-
assert.EqualValues(t, pull.Additions, patch.Files[0].Addition)
102-
assert.EqualValues(t, pull.Deletions, patch.Files[0].Deletion)
100+
assert.EqualValues(t, 4, patch.Files[0].Addition)
101+
assert.EqualValues(t, 1, patch.Files[0].Deletion)
103102
assert.Equal(t, gitdiff.DiffFileChange, patch.Files[0].Type)
104103
}
105104

106105
t.Run(fmt.Sprintf("APIGetPullFiles_%d", pull.ID),
107106
doAPIGetPullFiles(ctx, pull, func(t *testing.T, files []*api.ChangedFile) {
108-
if assert.Len(t, files, pull.ChangedFiles) {
107+
if assert.Len(t, files, 1) {
109108
assert.Equal(t, "README.md", files[0].Filename)
110109
// FIXME: The PreviousFilename name should be the same as Filename if it's a file change
111110
assert.Equal(t, "", files[0].PreviousFilename)
112-
assert.EqualValues(t, pull.Additions, files[0].Additions)
113-
assert.EqualValues(t, pull.Deletions, files[0].Deletions)
111+
assert.EqualValues(t, 4, files[0].Additions)
112+
assert.EqualValues(t, 1, files[0].Deletions)
114113
assert.Equal(t, "changed", files[0].Status)
115114
}
116115
}))
117116
}
118117

119-
pull = pulls[2]
118+
pull = pulls[0]
120119
assert.EqualValues(t, 1, pull.Poster.ID)
121-
assert.Len(t, pull.RequestedReviewers, 1)
120+
assert.Len(t, pull.RequestedReviewers, 2)
122121
assert.Empty(t, pull.RequestedReviewersTeams)
123-
assert.EqualValues(t, 1, pull.RequestedReviewers[0].ID)
124-
assert.EqualValues(t, 0, pull.ChangedFiles)
122+
assert.EqualValues(t, 5, pull.RequestedReviewers[0].ID)
125123

126-
if assert.EqualValues(t, 1, pull.ID) {
124+
if assert.EqualValues(t, 5, pull.ID) {
127125
resp = ctx.Session.MakeRequest(t, NewRequest(t, "GET", pull.DiffURL), http.StatusOK)
128126
bs, err := io.ReadAll(resp.Body)
129127
assert.NoError(t, err)
130128
patch, err := gitdiff.ParsePatch(t.Context(), 1000, 5000, 10, bytes.NewReader(bs), "")
131129
assert.NoError(t, err)
132-
assert.Len(t, patch.Files, pull.ChangedFiles)
130+
assert.Len(t, patch.Files, 1)
133131

134132
t.Run(fmt.Sprintf("APIGetPullFiles_%d", pull.ID),
135133
doAPIGetPullFiles(ctx, pull, func(t *testing.T, files []*api.ChangedFile) {
136-
assert.Len(t, files, pull.ChangedFiles)
134+
assert.Len(t, files, 1)
137135
}))
138136
}
139137
}

‎tests/integration/repo_webhook_test.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424

2525
"github.com/PuerkitoBio/goquery"
2626
"github.com/stretchr/testify/assert"
27+
"github.com/stretchr/testify/require"
2728
)
2829

2930
func TestNewWebHookLink(t *testing.T) {
@@ -377,12 +378,14 @@ func Test_WebhookPullRequest(t *testing.T) {
377378

378379
// 3. validate the webhook is triggered
379380
assert.EqualValues(t, "pull_request", triggeredEvent)
380-
assert.Len(t, payloads, 1)
381+
require.Len(t, payloads, 1)
381382
assert.EqualValues(t, "repo1", payloads[0].PullRequest.Base.Repository.Name)
382383
assert.EqualValues(t, "user2/repo1", payloads[0].PullRequest.Base.Repository.FullName)
383384
assert.EqualValues(t, "repo1", payloads[0].PullRequest.Head.Repository.Name)
384385
assert.EqualValues(t, "user2/repo1", payloads[0].PullRequest.Head.Repository.FullName)
385-
assert.EqualValues(t, 0, payloads[0].PullRequest.Additions)
386+
assert.EqualValues(t, 0, *payloads[0].PullRequest.Additions)
387+
assert.EqualValues(t, 1, *payloads[0].PullRequest.ChangedFiles)
388+
assert.EqualValues(t, 1, *payloads[0].PullRequest.Deletions)
386389
})
387390
}
388391

0 commit comments

Comments
 (0)