mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2025-10-19 02:43:18 +00:00
feat: simplify GetPullRequestFiles (#9740)
`GetPullRequestFiles` is the API route handler to get the files that are changed in a pull request, it has to know the start commit and end commit to diff for (so it can gather the changed file and other information). The end commit is clear, the pr ref (`ref/pull/xxx/head`). However the start commit has to be computed, it is the merge base commit between the base branch and pr ref. However if the pr was merged, then we should use the `pr.MergeBase` as it's possible the `pr.BaseBranch` no longer exists. Instead of doing this computation via `GetCompareInfo` that also does some other computations, compute the merge base directly ourselves in this function, if no merge base exists then fallback to the base reference (this is the same behavior as in `GetCompareInfo`). The only difference is that in the case of the fallback we don't convert the base ref to a commit ID, this is not necessary as the call to `git-diff` will accept any valid reference. So technically we could drop the call to `baseGitRepo.GetRefCommitID()` as well, but that's left for another time to keep the change minimal. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9740 Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org> Co-authored-by: Gusted <postmaster@gusted.xyz> Co-committed-by: Gusted <postmaster@gusted.xyz>
This commit is contained in:
Gusted
Earl Warren
parent
6726861e49
commit
0a7e438e43
2 changed files with 142 additions and 51 deletions
|
|
@ -1593,26 +1593,22 @@ func GetPullRequestFiles(ctx *context.APIContext) {
|
|||
|
||||
baseGitRepo := ctx.Repo.GitRepo
|
||||
|
||||
var prInfo *git.CompareInfo
|
||||
baseRef := pr.BaseBranch
|
||||
if pr.HasMerged {
|
||||
prInfo, err = baseGitRepo.GetCompareInfo(pr.BaseRepo.RepoPath(), pr.MergeBase, pr.GetGitRefName(), true, false)
|
||||
} else {
|
||||
prInfo, err = baseGitRepo.GetCompareInfo(pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitRefName(), true, false)
|
||||
baseRef = pr.MergeBase
|
||||
}
|
||||
startCommitID, err := baseGitRepo.GetMergeBaseSimple(baseRef, pr.GetGitRefName())
|
||||
if err != nil {
|
||||
ctx.ServerError("GetCompareInfo", err)
|
||||
return
|
||||
// No merge base exists, fallback to use base reference.
|
||||
startCommitID = baseRef
|
||||
}
|
||||
|
||||
headCommitID, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName())
|
||||
endCommitID, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName())
|
||||
if err != nil {
|
||||
ctx.ServerError("GetRefCommitID", err)
|
||||
return
|
||||
}
|
||||
|
||||
startCommitID := prInfo.MergeBase
|
||||
endCommitID := headCommitID
|
||||
|
||||
maxLines := setting.Git.MaxGitDiffLines
|
||||
|
||||
// FIXME: If there are too many files in the repo, may cause some unpredictable issues.
|
||||
|
|
|
|||
|
|
@ -1,13 +1,15 @@
|
|||
// Copyright 2017 The Gitea Authors. All rights reserved.
|
||||
// Copyright 2025 The Forgejo Authors. All rights reserved.
|
||||
// SPDX-License-Identifier: MIT
|
||||
|
||||
package integration
|
||||
|
||||
import (
|
||||
"cmp"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"slices"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
|
|
@ -31,40 +33,151 @@ import (
|
|||
func TestAPIViewPulls(t *testing.T) {
|
||||
defer tests.PrepareTestEnv(t)()
|
||||
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
|
||||
owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
|
||||
|
||||
ctx := NewAPITestContext(t, "user2", repo.Name, auth_model.AccessTokenScopeReadRepository)
|
||||
|
||||
req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/pulls?state=all", owner.Name, repo.Name).
|
||||
AddTokenAuth(ctx.Token)
|
||||
req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/pulls?state=all", repo.OwnerName, repo.Name).AddTokenAuth(ctx.Token)
|
||||
resp := ctx.Session.MakeRequest(t, req, http.StatusOK)
|
||||
|
||||
var pulls []*api.PullRequest
|
||||
DecodeJSON(t, resp, &pulls)
|
||||
expectedLen := unittest.GetCount(t, &issues_model.Issue{RepoID: repo.ID}, unittest.Cond("is_pull = ?", true))
|
||||
assert.Len(t, pulls, expectedLen)
|
||||
if assert.Len(t, pulls, 3) {
|
||||
slices.SortFunc(pulls, func(a, b *api.PullRequest) int {
|
||||
return cmp.Compare(a.ID, b.ID)
|
||||
})
|
||||
|
||||
pull := pulls[0]
|
||||
if assert.EqualValues(t, 5, pull.ID) {
|
||||
resp = ctx.Session.MakeRequest(t, NewRequest(t, "GET", pull.DiffURL), http.StatusOK)
|
||||
_, err := io.ReadAll(resp.Body)
|
||||
require.NoError(t, err)
|
||||
// TODO: use diff to generate stats to test against
|
||||
assert.EqualValues(t, 1, pulls[0].ID)
|
||||
assert.EqualValues(t, 2, pulls[0].Index)
|
||||
|
||||
t.Run(fmt.Sprintf("APIGetPullFiles_%d", pull.ID),
|
||||
doAPIGetPullFiles(ctx, pull, func(t *testing.T, files []*api.ChangedFile) {
|
||||
if assert.Len(t, files, 1) {
|
||||
assert.Equal(t, "File-WoW", files[0].Filename)
|
||||
assert.Empty(t, files[0].PreviousFilename)
|
||||
assert.Equal(t, 1, files[0].Additions)
|
||||
assert.Equal(t, 1, files[0].Changes)
|
||||
assert.Equal(t, 0, files[0].Deletions)
|
||||
assert.Equal(t, "added", files[0].Status)
|
||||
}
|
||||
}))
|
||||
assert.EqualValues(t, 2, pulls[1].ID)
|
||||
assert.EqualValues(t, 3, pulls[1].Index)
|
||||
|
||||
assert.EqualValues(t, 5, pulls[2].ID)
|
||||
assert.EqualValues(t, 5, pulls[2].Index)
|
||||
}
|
||||
}
|
||||
|
||||
func TestAPIPullsFiles(t *testing.T) {
|
||||
defer tests.PrepareTestEnv(t)()
|
||||
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
|
||||
|
||||
ctx := NewAPITestContext(t, "user2", repo.Name, auth_model.AccessTokenScopeReadRepository)
|
||||
|
||||
t.Run("Pull 1", func(t *testing.T) {
|
||||
req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/pulls/2/files", repo.OwnerName, repo.Name).AddTokenAuth(ctx.Token)
|
||||
resp := ctx.Session.MakeRequest(t, req, http.StatusOK)
|
||||
|
||||
var changedFiles []*api.ChangedFile
|
||||
DecodeJSON(t, resp, &changedFiles)
|
||||
|
||||
assert.Empty(t, changedFiles)
|
||||
assert.Equal(t, "0", resp.Header().Get("X-Total-Count"))
|
||||
assert.Equal(t, "false", resp.Header().Get("X-HasMore"))
|
||||
})
|
||||
|
||||
t.Run("Pull 2", func(t *testing.T) {
|
||||
req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/pulls/3/files", repo.OwnerName, repo.Name).AddTokenAuth(ctx.Token)
|
||||
resp := ctx.Session.MakeRequest(t, req, http.StatusOK)
|
||||
|
||||
var changedFiles []*api.ChangedFile
|
||||
DecodeJSON(t, resp, &changedFiles)
|
||||
|
||||
if assert.Len(t, changedFiles, 2) {
|
||||
assert.Equal(t, "2", resp.Header().Get("X-Total-Count"))
|
||||
assert.Equal(t, "false", resp.Header().Get("X-HasMore"))
|
||||
|
||||
assert.Equal(t, "3", changedFiles[0].Filename)
|
||||
assert.Empty(t, changedFiles[0].PreviousFilename)
|
||||
assert.Equal(t, "added", changedFiles[0].Status)
|
||||
assert.Equal(t, 1, changedFiles[0].Changes)
|
||||
assert.Equal(t, 1, changedFiles[0].Additions)
|
||||
assert.Equal(t, 0, changedFiles[0].Deletions)
|
||||
assert.Equal(t, setting.AppURL+"api/v1/repos/user2/repo1/contents/3?ref=5f22f7d0d95d614d25a5b68592adb345a4b5c7fd", changedFiles[0].ContentsURL)
|
||||
assert.Equal(t, setting.AppURL+"user2/repo1/raw/commit/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd/3", changedFiles[0].RawURL)
|
||||
assert.Equal(t, setting.AppURL+"user2/repo1/src/commit/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd/3", changedFiles[0].HTMLURL)
|
||||
|
||||
assert.Equal(t, "iso-8859-1.txt", changedFiles[1].Filename)
|
||||
assert.Empty(t, changedFiles[1].PreviousFilename)
|
||||
assert.Equal(t, "added", changedFiles[1].Status)
|
||||
assert.Equal(t, 10, changedFiles[1].Changes)
|
||||
assert.Equal(t, 10, changedFiles[1].Additions)
|
||||
assert.Equal(t, 0, changedFiles[1].Deletions)
|
||||
assert.Equal(t, setting.AppURL+"api/v1/repos/user2/repo1/contents/iso-8859-1.txt?ref=5f22f7d0d95d614d25a5b68592adb345a4b5c7fd", changedFiles[1].ContentsURL)
|
||||
assert.Equal(t, setting.AppURL+"user2/repo1/raw/commit/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd/iso-8859-1.txt", changedFiles[1].RawURL)
|
||||
assert.Equal(t, setting.AppURL+"user2/repo1/src/commit/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd/iso-8859-1.txt", changedFiles[1].HTMLURL)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("Pull 5", func(t *testing.T) {
|
||||
req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/pulls/5/files", repo.OwnerName, repo.Name).AddTokenAuth(ctx.Token)
|
||||
resp := ctx.Session.MakeRequest(t, req, http.StatusOK)
|
||||
|
||||
var changedFiles []*api.ChangedFile
|
||||
DecodeJSON(t, resp, &changedFiles)
|
||||
|
||||
if assert.Len(t, changedFiles, 1) {
|
||||
assert.Equal(t, "1", resp.Header().Get("X-Total-Count"))
|
||||
assert.Equal(t, "false", resp.Header().Get("X-HasMore"))
|
||||
|
||||
assert.Equal(t, "File-WoW", changedFiles[0].Filename)
|
||||
assert.Empty(t, changedFiles[0].PreviousFilename)
|
||||
assert.Equal(t, "added", changedFiles[0].Status)
|
||||
assert.Equal(t, 1, changedFiles[0].Changes)
|
||||
assert.Equal(t, 1, changedFiles[0].Additions)
|
||||
assert.Equal(t, 0, changedFiles[0].Deletions)
|
||||
assert.Equal(t, setting.AppURL+"api/v1/repos/user2/repo1/contents/File-WoW?ref=62fb502a7172d4453f0322a2cc85bddffa57f07a", changedFiles[0].ContentsURL)
|
||||
assert.Equal(t, setting.AppURL+"user2/repo1/raw/commit/62fb502a7172d4453f0322a2cc85bddffa57f07a/File-WoW", changedFiles[0].RawURL)
|
||||
assert.Equal(t, setting.AppURL+"user2/repo1/src/commit/62fb502a7172d4453f0322a2cc85bddffa57f07a/File-WoW", changedFiles[0].HTMLURL)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("Pull 7", func(t *testing.T) {
|
||||
req := NewRequest(t, "GET", "/api/v1/repos/user2/commitsonpr/pulls/1/files?limit=3").AddTokenAuth(ctx.Token)
|
||||
resp := ctx.Session.MakeRequest(t, req, http.StatusOK)
|
||||
|
||||
var changedFiles []*api.ChangedFile
|
||||
DecodeJSON(t, resp, &changedFiles)
|
||||
|
||||
if assert.Len(t, changedFiles, 3) {
|
||||
assert.Equal(t, "10", resp.Header().Get("X-Total-Count"))
|
||||
assert.Equal(t, "true", resp.Header().Get("X-HasMore"))
|
||||
assert.Equal(t, "1", resp.Header().Get("X-Page"))
|
||||
assert.Equal(t, "3", resp.Header().Get("X-PerPage"))
|
||||
assert.Equal(t, "4", resp.Header().Get("X-PageCount"))
|
||||
|
||||
assert.Equal(t, "test1.txt", changedFiles[0].Filename)
|
||||
assert.Empty(t, changedFiles[0].PreviousFilename)
|
||||
assert.Equal(t, "added", changedFiles[0].Status)
|
||||
assert.Equal(t, 1, changedFiles[0].Changes)
|
||||
assert.Equal(t, 1, changedFiles[0].Additions)
|
||||
assert.Equal(t, 0, changedFiles[0].Deletions)
|
||||
assert.Equal(t, setting.AppURL+"api/v1/repos/user2/commitsonpr/contents/test1.txt?ref=9b93963cf6de4dc33f915bb67f192d099c301f43", changedFiles[0].ContentsURL)
|
||||
assert.Equal(t, setting.AppURL+"user2/commitsonpr/raw/commit/9b93963cf6de4dc33f915bb67f192d099c301f43/test1.txt", changedFiles[0].RawURL)
|
||||
assert.Equal(t, setting.AppURL+"user2/commitsonpr/src/commit/9b93963cf6de4dc33f915bb67f192d099c301f43/test1.txt", changedFiles[0].HTMLURL)
|
||||
|
||||
assert.Equal(t, "test10.txt", changedFiles[1].Filename)
|
||||
assert.Empty(t, changedFiles[1].PreviousFilename)
|
||||
assert.Equal(t, "added", changedFiles[1].Status)
|
||||
assert.Equal(t, 1, changedFiles[1].Changes)
|
||||
assert.Equal(t, 1, changedFiles[1].Additions)
|
||||
assert.Equal(t, 0, changedFiles[1].Deletions)
|
||||
assert.Equal(t, setting.AppURL+"api/v1/repos/user2/commitsonpr/contents/test10.txt?ref=9b93963cf6de4dc33f915bb67f192d099c301f43", changedFiles[1].ContentsURL)
|
||||
assert.Equal(t, setting.AppURL+"user2/commitsonpr/raw/commit/9b93963cf6de4dc33f915bb67f192d099c301f43/test10.txt", changedFiles[1].RawURL)
|
||||
assert.Equal(t, setting.AppURL+"user2/commitsonpr/src/commit/9b93963cf6de4dc33f915bb67f192d099c301f43/test10.txt", changedFiles[1].HTMLURL)
|
||||
|
||||
assert.Equal(t, "test2.txt", changedFiles[2].Filename)
|
||||
assert.Empty(t, changedFiles[2].PreviousFilename)
|
||||
assert.Equal(t, "added", changedFiles[2].Status)
|
||||
assert.Equal(t, 1, changedFiles[2].Changes)
|
||||
assert.Equal(t, 1, changedFiles[2].Additions)
|
||||
assert.Equal(t, 0, changedFiles[2].Deletions)
|
||||
assert.Equal(t, setting.AppURL+"api/v1/repos/user2/commitsonpr/contents/test2.txt?ref=9b93963cf6de4dc33f915bb67f192d099c301f43", changedFiles[2].ContentsURL)
|
||||
assert.Equal(t, setting.AppURL+"user2/commitsonpr/raw/commit/9b93963cf6de4dc33f915bb67f192d099c301f43/test2.txt", changedFiles[2].RawURL)
|
||||
assert.Equal(t, setting.AppURL+"user2/commitsonpr/src/commit/9b93963cf6de4dc33f915bb67f192d099c301f43/test2.txt", changedFiles[2].HTMLURL)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func TestAPIViewPullsByBaseHead(t *testing.T) {
|
||||
defer tests.PrepareTestEnv(t)()
|
||||
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
|
||||
|
|
@ -305,24 +418,6 @@ func TestAPIForkDifferentName(t *testing.T) {
|
|||
MakeRequest(t, req, http.StatusCreated)
|
||||
}
|
||||
|
||||
func doAPIGetPullFiles(ctx APITestContext, pr *api.PullRequest, callback func(*testing.T, []*api.ChangedFile)) func(*testing.T) {
|
||||
return func(t *testing.T) {
|
||||
req := NewRequest(t, http.MethodGet, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/files", ctx.Username, ctx.Reponame, pr.Index)).
|
||||
AddTokenAuth(ctx.Token)
|
||||
if ctx.ExpectedCode == 0 {
|
||||
ctx.ExpectedCode = http.StatusOK
|
||||
}
|
||||
resp := ctx.Session.MakeRequest(t, req, ctx.ExpectedCode)
|
||||
|
||||
files := make([]*api.ChangedFile, 0, 1)
|
||||
DecodeJSON(t, resp, &files)
|
||||
|
||||
if callback != nil {
|
||||
callback(t, files)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestAPIPullDeleteBranchPerms(t *testing.T) {
|
||||
onApplicationRun(t, func(t *testing.T, giteaURL *url.URL) {
|
||||
user2Session := loginUser(t, "user2")
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue