Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not call "git diff" when listing PRs #33817

Merged
merged 3 commits into from
Mar 8, 2025
+31 −43
Merged

Do not call "git diff" when listing PRs #33817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52f7ca5
fix
wxiaoguang Mar 7, 2025
12989eb
Merge branch 'release/v1.23' into fix-pr-list-diff
wxiaoguang Mar 8, 2025
9796c73
Merge branch 'release/v1.23' into fix-pr-list-diff
wxiaoguang Mar 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions modules/structs/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@ type PullRequest struct {
Comments int `json:"comments"`
// number of review comments made on the diff of a PR review (not including comments on commits or issues in a PR)
ReviewComments int `json:"review_comments"`
Additions int `json:"additions"`
Deletions int `json:"deletions"`
ChangedFiles int `json:"changed_files"`

Additions *int `json:"additions,omitempty"`
Deletions *int `json:"deletions,omitempty"`
ChangedFiles *int `json:"changed_files,omitempty"`

HTMLURL string `json:"html_url"`
DiffURL string `json:"diff_url"`
Expand Down
20 changes: 3 additions & 17 deletions services/convert/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,9 +239,11 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u
// Calculate diff
startCommitID = pr.MergeBase

apiPullRequest.ChangedFiles, apiPullRequest.Additions, apiPullRequest.Deletions, err = gitRepo.GetDiffShortStat(startCommitID, endCommitID)
diffChangedFiles, diffAdditions, diffDeletions, err := gitRepo.GetDiffShortStat(startCommitID, endCommitID)
if err != nil {
log.Error("GetDiffShortStat: %v", err)
} else {
apiPullRequest.ChangedFiles, apiPullRequest.Additions, apiPullRequest.Deletions = &diffChangedFiles, &diffAdditions, &diffDeletions
}
}

Expand Down Expand Up @@ -459,12 +461,6 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs
return nil, err
}

// Outer scope variables to be used in diff calculation
var (
startCommitID string
endCommitID string
)

if git.IsErrBranchNotExist(err) {
headCommitID, err := headGitRepo.GetRefCommitID(apiPullRequest.Head.Ref)
if err != nil && !git.IsErrNotExist(err) {
Expand All @@ -473,7 +469,6 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs
}
if err == nil {
apiPullRequest.Head.Sha = headCommitID
endCommitID = headCommitID
}
} else {
commit, err := headBranch.GetCommit()
Expand All @@ -484,17 +479,8 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs
if err == nil {
apiPullRequest.Head.Ref = pr.HeadBranch
apiPullRequest.Head.Sha = commit.ID.String()
endCommitID = commit.ID.String()
}
}

// Calculate diff
startCommitID = pr.MergeBase

apiPullRequest.ChangedFiles, apiPullRequest.Additions, apiPullRequest.Deletions, err = gitRepo.GetDiffShortStat(startCommitID, endCommitID)
if err != nil {
log.Error("GetDiffShortStat: %v", err)
}
}

if len(apiPullRequest.Head.Sha) == 0 && len(apiPullRequest.Head.Ref) != 0 {
Expand Down
40 changes: 19 additions & 21 deletions tests/integration/api_pull_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,30 +51,30 @@ func TestAPIViewPulls(t *testing.T) {
assert.Empty(t, pull.RequestedReviewersTeams)
assert.EqualValues(t, 5, pull.RequestedReviewers[0].ID)
assert.EqualValues(t, 6, pull.RequestedReviewers[1].ID)
assert.EqualValues(t, 1, pull.ChangedFiles)

if assert.EqualValues(t, 5, pull.ID) {
resp = ctx.Session.MakeRequest(t, NewRequest(t, "GET", pull.DiffURL), http.StatusOK)
bs, err := io.ReadAll(resp.Body)
assert.NoError(t, err)
patch, err := gitdiff.ParsePatch(context.Background(), 1000, 5000, 10, bytes.NewReader(bs), "")
assert.NoError(t, err)
if assert.Len(t, patch.Files, pull.ChangedFiles) {
if assert.Len(t, patch.Files, 1) {
assert.Equal(t, "File-WoW", patch.Files[0].Name)
// FIXME: The old name should be empty if it's a file add type
assert.Equal(t, "File-WoW", patch.Files[0].OldName)
assert.EqualValues(t, pull.Additions, patch.Files[0].Addition)
assert.EqualValues(t, pull.Deletions, patch.Files[0].Deletion)
assert.EqualValues(t, 1, patch.Files[0].Addition)
assert.EqualValues(t, 0, patch.Files[0].Deletion)
assert.Equal(t, gitdiff.DiffFileAdd, patch.Files[0].Type)
}

t.Run(fmt.Sprintf("APIGetPullFiles_%d", pull.ID),
doAPIGetPullFiles(ctx, pull, func(t *testing.T, files []*api.ChangedFile) {
if assert.Len(t, files, pull.ChangedFiles) {
if assert.Len(t, files, 1) {
assert.Equal(t, "File-WoW", files[0].Filename)
assert.Empty(t, files[0].PreviousFilename)
assert.EqualValues(t, pull.Additions, files[0].Additions)
assert.EqualValues(t, pull.Deletions, files[0].Deletions)
assert.EqualValues(t, 1, files[0].Additions)
assert.EqualValues(t, 1, files[0].Changes)
assert.EqualValues(t, 0, files[0].Deletions)
assert.Equal(t, "added", files[0].Status)
}
}))
Expand All @@ -88,53 +88,51 @@ func TestAPIViewPulls(t *testing.T) {
assert.EqualValues(t, 4, pull.RequestedReviewers[1].ID)
assert.EqualValues(t, 2, pull.RequestedReviewers[2].ID)
assert.EqualValues(t, 5, pull.RequestedReviewers[3].ID)
assert.EqualValues(t, 1, pull.ChangedFiles)

if assert.EqualValues(t, 2, pull.ID) {
resp = ctx.Session.MakeRequest(t, NewRequest(t, "GET", pull.DiffURL), http.StatusOK)
bs, err := io.ReadAll(resp.Body)
assert.NoError(t, err)
patch, err := gitdiff.ParsePatch(context.Background(), 1000, 5000, 10, bytes.NewReader(bs), "")
assert.NoError(t, err)
if assert.Len(t, patch.Files, pull.ChangedFiles) {
if assert.Len(t, patch.Files, 1) {
assert.Equal(t, "README.md", patch.Files[0].Name)
assert.Equal(t, "README.md", patch.Files[0].OldName)
assert.EqualValues(t, pull.Additions, patch.Files[0].Addition)
assert.EqualValues(t, pull.Deletions, patch.Files[0].Deletion)
assert.EqualValues(t, 4, patch.Files[0].Addition)
assert.EqualValues(t, 1, patch.Files[0].Deletion)
assert.Equal(t, gitdiff.DiffFileChange, patch.Files[0].Type)
}

t.Run(fmt.Sprintf("APIGetPullFiles_%d", pull.ID),
doAPIGetPullFiles(ctx, pull, func(t *testing.T, files []*api.ChangedFile) {
if assert.Len(t, files, pull.ChangedFiles) {
if assert.Len(t, files, 1) {
assert.Equal(t, "README.md", files[0].Filename)
// FIXME: The PreviousFilename name should be the same as Filename if it's a file change
assert.Equal(t, "", files[0].PreviousFilename)
assert.EqualValues(t, pull.Additions, files[0].Additions)
assert.EqualValues(t, pull.Deletions, files[0].Deletions)
assert.EqualValues(t, 4, files[0].Additions)
assert.EqualValues(t, 1, files[0].Deletions)
assert.Equal(t, "changed", files[0].Status)
}
}))
}

pull = pulls[2]
pull = pulls[0]
assert.EqualValues(t, 1, pull.Poster.ID)
assert.Len(t, pull.RequestedReviewers, 1)
assert.Len(t, pull.RequestedReviewers, 2)
assert.Empty(t, pull.RequestedReviewersTeams)
assert.EqualValues(t, 1, pull.RequestedReviewers[0].ID)
assert.EqualValues(t, 0, pull.ChangedFiles)
assert.EqualValues(t, 5, pull.RequestedReviewers[0].ID)

if assert.EqualValues(t, 1, pull.ID) {
if assert.EqualValues(t, 5, pull.ID) {
resp = ctx.Session.MakeRequest(t, NewRequest(t, "GET", pull.DiffURL), http.StatusOK)
bs, err := io.ReadAll(resp.Body)
assert.NoError(t, err)
patch, err := gitdiff.ParsePatch(context.Background(), 1000, 5000, 10, bytes.NewReader(bs), "")
assert.NoError(t, err)
assert.EqualValues(t, pull.ChangedFiles, patch.NumFiles)
assert.EqualValues(t, 1, patch.NumFiles)

t.Run(fmt.Sprintf("APIGetPullFiles_%d", pull.ID),
doAPIGetPullFiles(ctx, pull, func(t *testing.T, files []*api.ChangedFile) {
assert.Len(t, files, pull.ChangedFiles)
assert.Len(t, files, 1)
}))
}
}
Expand Down
7 changes: 5 additions & 2 deletions tests/integration/repo_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (

"github.com/PuerkitoBio/goquery"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestNewWebHookLink(t *testing.T) {
Expand Down Expand Up @@ -378,12 +379,14 @@ func Test_WebhookPullRequest(t *testing.T) {

// 3. validate the webhook is triggered
assert.EqualValues(t, "pull_request", triggeredEvent)
assert.Len(t, payloads, 1)
require.Len(t, payloads, 1)
assert.EqualValues(t, "repo1", payloads[0].PullRequest.Base.Repository.Name)
assert.EqualValues(t, "user2/repo1", payloads[0].PullRequest.Base.Repository.FullName)
assert.EqualValues(t, "repo1", payloads[0].PullRequest.Head.Repository.Name)
assert.EqualValues(t, "user2/repo1", payloads[0].PullRequest.Head.Repository.FullName)
assert.EqualValues(t, 0, payloads[0].PullRequest.Additions)
assert.EqualValues(t, 0, *payloads[0].PullRequest.Additions)
assert.EqualValues(t, 0, *payloads[0].PullRequest.ChangedFiles)
assert.EqualValues(t, 0, *payloads[0].PullRequest.Deletions)
})
}

Expand Down