github-mirror/gitea
2
0
Fork 0
mirror of https://github.com/go-gitea/gitea.git synced 2024-12-19 01:15:47 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

Make API "compare" accept commit IDs (#32801)

Browse Source
This commit is contained in:
wxiaoguang 2024-12-12 16:10:09 +08:00 committed by GitHub
parent 01b1896bf5
commit 22bf2ca6ba
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 154 additions and 124 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
modules/git/commit.go
View File

@ -474,3 +474,17 @@ func (c *Commit) GetRepositoryDefaultPublicGPGKey(forceUpdate bool) (*GPGSetting
}
return c.repo.GetDefaultPublicGPGKey(forceUpdate)
}
func IsStringLikelyCommitID(objFmt ObjectFormat, s string, minLength ...int) bool {
minLen := util.OptionalArg(minLength, objFmt.FullLength())
if len(s) < minLen || len(s) > objFmt.FullLength() {
return false
}
for _, c := range s {
isHex := (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')
if !isHex {
return false
}
}
return true
}

4
modules/git/ref.go
View File

@ -142,7 +142,6 @@ func (ref RefName) RemoteName() string {
// ShortName returns the short name of the reference name
func (ref RefName) ShortName() string {
refName := string(ref)
if ref.IsBranch() {
return ref.BranchName()
}
@ -158,8 +157,7 @@ func (ref RefName) ShortName() string {
if ref.IsFor() {
return ref.ForBranchName()
}
return refName
return string(ref) // usually it is a commit ID
}
// RefGroup returns the group type of the reference

28
modules/git/repo_ref.go
View File

@ -61,3 +61,31 @@ func parseTags(refs []string) []string {
}
return results
}
// UnstableGuessRefByShortName does the best guess to see whether a "short name" provided by user is a branch, tag or commit.
// It could guess wrongly if the input is already ambiguous. For example:
// * "refs/heads/the-name" vs "refs/heads/refs/heads/the-name"
// * "refs/tags/1234567890" vs commit "1234567890"
// In most cases, it SHOULD AVOID using this function, unless there is an irresistible reason (eg: make API friendly to end users)
// If the function is used, the caller SHOULD CHECK the ref type carefully.
func (repo *Repository) UnstableGuessRefByShortName(shortName string) RefName {
if repo.IsBranchExist(shortName) {
return RefNameFromBranch(shortName)
}
if repo.IsTagExist(shortName) {
return RefNameFromTag(shortName)
}
if strings.HasPrefix(shortName, "refs/") {
if repo.IsReferenceExist(shortName) {
return RefName(shortName)
}
}
commit, err := repo.GetCommit(shortName)
if err == nil {
commitIDString := commit.ID.String()
if strings.HasPrefix(commitIDString, shortName) {
return RefName(commitIDString)
}
}
return ""
}

2
modules/globallock/globallock_test.go
View File

@ -64,7 +64,7 @@ func TestLockAndDo(t *testing.T) {
}
func testLockAndDo(t *testing.T) {
const concurrency = 1000
const concurrency = 50
ctx := context.Background()
count := 0

15
routers/api/v1/repo/compare.go
View File

@ -64,22 +64,19 @@ func CompareDiff(ctx *context.APIContext) {
}
}
_, headGitRepo, ci, _, _ := parseCompareInfo(ctx, api.CreatePullRequestOption{
Base: infos[0],
Head: infos[1],
})
compareResult, closer := parseCompareInfo(ctx, api.CreatePullRequestOption{Base: infos[0], Head: infos[1]})
if ctx.Written() {
return
}
defer headGitRepo.Close()
defer closer()
verification := ctx.FormString("verification") == "" || ctx.FormBool("verification")
files := ctx.FormString("files") == "" || ctx.FormBool("files")
apiCommits := make([]*api.Commit, 0, len(ci.Commits))
apiCommits := make([]*api.Commit, 0, len(compareResult.compareInfo.Commits))
userCache := make(map[string]*user_model.User)
for i := 0; i < len(ci.Commits); i++ {
apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, ci.Commits[i], userCache,
for i := 0; i < len(compareResult.compareInfo.Commits); i++ {
apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, compareResult.compareInfo.Commits[i], userCache,
convert.ToCommitOptions{
Stat: true,
Verification: verification,
@ -93,7 +90,7 @@ func CompareDiff(ctx *context.APIContext) {
}
ctx.JSON(http.StatusOK, &api.Compare{
TotalCommits: len(ci.Commits),
TotalCommits: len(compareResult.compareInfo.Commits),
Commits: apiCommits,
})
}

151
routers/api/v1/repo/pull.go
View File

@ -389,8 +389,7 @@ func CreatePullRequest(ctx *context.APIContext) {
form := *web.GetForm(ctx).(*api.CreatePullRequestOption)
if form.Head == form.Base {
ctx.Error(http.StatusUnprocessableEntity, "BaseHeadSame",
"Invalid PullRequest: There are no changes between the head and the base")
ctx.Error(http.StatusUnprocessableEntity, "BaseHeadSame", "Invalid PullRequest: There are no changes between the head and the base")
return
}
@ -401,14 +400,22 @@ func CreatePullRequest(ctx *context.APIContext) {
)
// Get repo/branch information
headRepo, headGitRepo, compareInfo, baseBranch, headBranch := parseCompareInfo(ctx, form)
compareResult, closer := parseCompareInfo(ctx, form)
if ctx.Written() {
return
}
defer headGitRepo.Close()
defer closer()
if !compareResult.baseRef.IsBranch() || !compareResult.headRef.IsBranch() {
ctx.Error(http.StatusUnprocessableEntity, "BaseHeadInvalidRefType", "Invalid PullRequest: base and head must be branches")
return
}
// Check if another PR exists with the same targets
existingPr, err := issues_model.GetUnmergedPullRequest(ctx, headRepo.ID, ctx.Repo.Repository.ID, headBranch, baseBranch, issues_model.PullRequestFlowGithub)
existingPr, err := issues_model.GetUnmergedPullRequest(ctx, compareResult.headRepo.ID, ctx.Repo.Repository.ID,
compareResult.headRef.ShortName(), compareResult.baseRef.ShortName(),
issues_model.PullRequestFlowGithub,
)
if err != nil {
if !issues_model.IsErrPullRequestNotExist(err) {
ctx.Error(http.StatusInternalServerError, "GetUnmergedPullRequest", err)
@ -484,13 +491,13 @@ func CreatePullRequest(ctx *context.APIContext) {
DeadlineUnix: deadlineUnix,
}
pr := &issues_model.PullRequest{
HeadRepoID: headRepo.ID,
HeadRepoID: compareResult.headRepo.ID,
BaseRepoID: repo.ID,
HeadBranch: headBranch,
BaseBranch: baseBranch,
HeadRepo: headRepo,
HeadBranch: compareResult.headRef.ShortName(),
BaseBranch: compareResult.baseRef.ShortName(),
HeadRepo: compareResult.headRepo,
BaseRepo: repo,
MergeBase: compareInfo.MergeBase,
MergeBase: compareResult.compareInfo.MergeBase,
Type: issues_model.PullRequestGitea,
}
@ -1080,32 +1087,32 @@ func MergePullRequest(ctx *context.APIContext) {
ctx.Status(http.StatusOK)
}
func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) (*repo_model.Repository, *git.Repository, *git.CompareInfo, string, string) {
baseRepo := ctx.Repo.Repository
type parseCompareInfoResult struct {
headRepo *repo_model.Repository
headGitRepo *git.Repository
compareInfo *git.CompareInfo
baseRef git.RefName
headRef git.RefName
}
// parseCompareInfo returns non-nil if it succeeds, it always writes to the context and returns nil if it fails
func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) (result *parseCompareInfoResult, closer func()) {
var err error
// Get compared branches information
// format: <base branch>...[<head repo>:]<head branch>
// base<-head: master...head:feature
// same repo: master...feature
baseRepo := ctx.Repo.Repository
baseRefToGuess := form.Base
// TODO: Validate form first?
baseBranch := form.Base
var (
headUser *user_model.User
headBranch string
isSameRepo bool
err error
)
headUser := ctx.Repo.Owner
headRefToGuess := form.Head
if headInfos := strings.Split(form.Head, ":"); len(headInfos) == 1 {
// If there is no head repository, it means pull request between same repository.
headInfos := strings.Split(form.Head, ":")
if len(headInfos) == 1 {
isSameRepo = true
headUser = ctx.Repo.Owner
headBranch = headInfos[0]
// Do nothing here because the head variables have been assigned above.
} else if len(headInfos) == 2 {
// There is a head repository (the head repository could also be the same base repo)
headRefToGuess = headInfos[1]
headUser, err = user_model.GetUserByName(ctx, headInfos[0])
if err != nil {
if user_model.IsErrUserNotExist(err) {
@ -1113,38 +1120,29 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
} else {
ctx.Error(http.StatusInternalServerError, "GetUserByName", err)
}
return nil, nil, nil, "", ""
return nil, nil
}
headBranch = headInfos[1]
// The head repository can also point to the same repo
isSameRepo = ctx.Repo.Owner.ID == headUser.ID
} else {
ctx.NotFound()
return nil, nil, nil, "", ""
return nil, nil
}
ctx.Repo.PullRequest.SameRepo = isSameRepo
log.Trace("Repo path: %q, base branch: %q, head branch: %q", ctx.Repo.GitRepo.Path, baseBranch, headBranch)
// Check if base branch is valid.
if !ctx.Repo.GitRepo.IsBranchExist(baseBranch) && !ctx.Repo.GitRepo.IsTagExist(baseBranch) {
ctx.NotFound("BaseNotExist")
return nil, nil, nil, "", ""
}
isSameRepo := ctx.Repo.Owner.ID == headUser.ID
// Check if current user has fork of repository or in the same repository.
headRepo := repo_model.GetForkedRepo(ctx, headUser.ID, baseRepo.ID)
if headRepo == nil && !isSameRepo {
err := baseRepo.GetBaseRepo(ctx)
err = baseRepo.GetBaseRepo(ctx)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetBaseRepo", err)
return nil, nil, nil, "", ""
return nil, nil
}
// Check if baseRepo's base repository is the same as headUser's repository.
if baseRepo.BaseRepo == nil || baseRepo.BaseRepo.OwnerID != headUser.ID {
log.Trace("parseCompareInfo[%d]: does not have fork or in same repository", baseRepo.ID)
ctx.NotFound("GetBaseRepo")
return nil, nil, nil, "", ""
return nil, nil
}
// Assign headRepo so it can be used below.
headRepo = baseRepo.BaseRepo
@ -1154,67 +1152,68 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
if isSameRepo {
headRepo = ctx.Repo.Repository
headGitRepo = ctx.Repo.GitRepo
closer = func() {} // no need to close the head repo because it shares the base repo
} else {
headGitRepo, err = gitrepo.OpenRepository(ctx, headRepo)
if err != nil {
ctx.Error(http.StatusInternalServerError, "OpenRepository", err)
return nil, nil, nil, "", ""
return nil, nil
}
closer = func() { _ = headGitRepo.Close() }
}
defer func() {
if result == nil && !isSameRepo {
_ = headGitRepo.Close()
}
}()
// user should have permission to read baseRepo's codes and pulls, NOT headRepo's
permBase, err := access_model.GetUserRepoPermission(ctx, baseRepo, ctx.Doer)
if err != nil {
headGitRepo.Close()
ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err)
return nil, nil, nil, "", ""
}
if !permBase.CanReadIssuesOrPulls(true) || !permBase.CanRead(unit.TypeCode) {
if log.IsTrace() {
log.Trace("Permission Denied: User %-v cannot create/read pull requests or cannot read code in Repo %-v\nUser in baseRepo has Permissions: %-+v",
ctx.Doer,
baseRepo,
permBase)
}
headGitRepo.Close()
ctx.NotFound("Can't read pulls or can't read UnitTypeCode")
return nil, nil, nil, "", ""
return nil, nil
}
// user should have permission to read headrepo's codes
if !permBase.CanReadIssuesOrPulls(true) || !permBase.CanRead(unit.TypeCode) {
log.Trace("Permission Denied: User %-v cannot create/read pull requests or cannot read code in Repo %-v\nUser in baseRepo has Permissions: %-+v", ctx.Doer, baseRepo, permBase)
ctx.NotFound("Can't read pulls or can't read UnitTypeCode")
return nil, nil
}
// user should have permission to read headRepo's codes
// TODO: could the logic be simplified if the headRepo is the same as the baseRepo? Need to think more about it.
permHead, err := access_model.GetUserRepoPermission(ctx, headRepo, ctx.Doer)
if err != nil {
headGitRepo.Close()
ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err)
return nil, nil, nil, "", ""
return nil, nil
}
if !permHead.CanRead(unit.TypeCode) {
if log.IsTrace() {
log.Trace("Permission Denied: User: %-v cannot read code in Repo: %-v\nUser in headRepo has Permissions: %-+v",
ctx.Doer,
headRepo,
permHead)
}
headGitRepo.Close()
log.Trace("Permission Denied: User: %-v cannot read code in Repo: %-v\nUser in headRepo has Permissions: %-+v", ctx.Doer, headRepo, permHead)
ctx.NotFound("Can't read headRepo UnitTypeCode")
return nil, nil, nil, "", ""
return nil, nil
}
// Check if head branch is valid.
if !headGitRepo.IsBranchExist(headBranch) && !headGitRepo.IsTagExist(headBranch) {
headGitRepo.Close()
baseRef := ctx.Repo.GitRepo.UnstableGuessRefByShortName(baseRefToGuess)
headRef := headGitRepo.UnstableGuessRefByShortName(headRefToGuess)
log.Trace("Repo path: %q, base ref: %q->%q, head ref: %q->%q", ctx.Repo.GitRepo.Path, baseRefToGuess, baseRef, headRefToGuess, headRef)
baseRefValid := baseRef.IsBranch() || baseRef.IsTag() || git.IsStringLikelyCommitID(git.ObjectFormatFromName(ctx.Repo.Repository.ObjectFormatName), baseRef.ShortName())
headRefValid := headRef.IsBranch() || headRef.IsTag() || git.IsStringLikelyCommitID(git.ObjectFormatFromName(headRepo.ObjectFormatName), headRef.ShortName())
// Check if base&head ref are valid.
if !baseRefValid || !headRefValid {
ctx.NotFound()
return nil, nil, nil, "", ""
return nil, nil
}
compareInfo, err := headGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), baseBranch, headBranch, false, false)
compareInfo, err := headGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), baseRef.ShortName(), headRef.ShortName(), false, false)
if err != nil {
headGitRepo.Close()
ctx.Error(http.StatusInternalServerError, "GetCompareInfo", err)
return nil, nil, nil, "", ""
return nil, nil
}
return headRepo, headGitRepo, compareInfo, baseBranch, headBranch
result = &parseCompareInfoResult{headRepo: headRepo, headGitRepo: headGitRepo, compareInfo: compareInfo, baseRef: baseRef, headRef: headRef}
return result, closer
}
// UpdatePullRequest merge PR's baseBranch into headBranch

2
routers/web/repo/issue.go
View File

@ -9,7 +9,6 @@ import (
"fmt"
"html/template"
"net/http"
"net/url"
"strconv"
"strings"
@ -114,7 +113,6 @@ func MustAllowPulls(ctx *context.Context) {
// User can send pull request if owns a forked repository.
if ctx.IsSigned && repo_model.HasForkedRepo(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID) {
ctx.Repo.PullRequest.Allowed = true
ctx.Repo.PullRequest.HeadInfoSubURL = url.PathEscape(ctx.Doer.Name) + ":" + util.PathEscapeSegments(ctx.Repo.BranchName)
}
}

28
services/context/repo.go
View File

@ -40,9 +40,8 @@ import (
// PullRequest contains information to make a pull request
type PullRequest struct {
BaseRepo *repo_model.Repository
Allowed bool
SameRepo bool
HeadInfoSubURL string // [<user>:]<branch> url segment
Allowed bool // it only used by the web tmpl: "PullRequestCtx.Allowed"
SameRepo bool // it only used by the web tmpl: "PullRequestCtx.SameRepo"
}
// Repository contains information to operate a repository
@ -401,6 +400,7 @@ func repoAssignment(ctx *Context, repo *repo_model.Repository) {
// RepoAssignment returns a middleware to handle repository assignment
func RepoAssignment(ctx *Context) context.CancelFunc {
if _, repoAssignmentOnce := ctx.Data["repoAssignmentExecuted"]; repoAssignmentOnce {
// FIXME: it should panic in dev/test modes to have a clear behavior
log.Trace("RepoAssignment was exec already, skipping second call ...")
return nil
}
@ -697,7 +697,6 @@ func RepoAssignment(ctx *Context) context.CancelFunc {
ctx.Data["BaseRepo"] = repo.BaseRepo
ctx.Repo.PullRequest.BaseRepo = repo.BaseRepo
ctx.Repo.PullRequest.Allowed = canPush
ctx.Repo.PullRequest.HeadInfoSubURL = url.PathEscape(ctx.Repo.Owner.Name) + ":" + util.PathEscapeSegments(ctx.Repo.BranchName)
} else if repo.AllowsPulls(ctx) {
// Or, this is repository accepts pull requests between branches.
canCompare = true
@ -705,7 +704,6 @@ func RepoAssignment(ctx *Context) context.CancelFunc {
ctx.Repo.PullRequest.BaseRepo = repo
ctx.Repo.PullRequest.Allowed = canPush
ctx.Repo.PullRequest.SameRepo = true
ctx.Repo.PullRequest.HeadInfoSubURL = util.PathEscapeSegments(ctx.Repo.BranchName)
}
ctx.Data["CanCompareOrPull"] = canCompare
ctx.Data["PullRequestCtx"] = ctx.Repo.PullRequest
@ -771,20 +769,6 @@ func getRefNameFromPath(repo *Repository, path string, isExist func(string) bool
return ""
}
func isStringLikelyCommitID(objFmt git.ObjectFormat, s string, minLength ...int) bool {
minLen := util.OptionalArg(minLength, objFmt.FullLength())
if len(s) < minLen || len(s) > objFmt.FullLength() {
return false
}
for _, c := range s {
isHex := (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')
if !isHex {
return false
}
}
return true
}
func getRefNameLegacy(ctx *Base, repo *Repository, optionalExtraRef ...string) (string, RepoRefType) {
extraRef := util.OptionalArg(optionalExtraRef)
reqPath := ctx.PathParam("*")
@ -799,7 +783,7 @@ func getRefNameLegacy(ctx *Base, repo *Repository, optionalExtraRef ...string) (
// For legacy support only full commit sha
parts := strings.Split(reqPath, "/")
if isStringLikelyCommitID(git.ObjectFormatFromName(repo.Repository.ObjectFormatName), parts[0]) {
if git.IsStringLikelyCommitID(git.ObjectFormatFromName(repo.Repository.ObjectFormatName), parts[0]) {
// FIXME: this logic is different from other types. Ideally, it should also try to GetCommit to check if it exists
repo.TreePath = strings.Join(parts[1:], "/")
return parts[0], RepoRefCommit
@ -849,7 +833,7 @@ func getRefName(ctx *Base, repo *Repository, pathType RepoRefType) string {
return getRefNameFromPath(repo, path, repo.GitRepo.IsTagExist)
case RepoRefCommit:
parts := strings.Split(path, "/")
if isStringLikelyCommitID(repo.GetObjectFormat(), parts[0], 7) {
if git.IsStringLikelyCommitID(repo.GetObjectFormat(), parts[0], 7) {
// FIXME: this logic is different from other types. Ideally, it should also try to GetCommit to check if it exists
repo.TreePath = strings.Join(parts[1:], "/")
return parts[0]
@ -985,7 +969,7 @@ func RepoRefByType(detectRefType RepoRefType, opts ...RepoRefByTypeOptions) func
return cancel
}
ctx.Repo.CommitID = ctx.Repo.Commit.ID.String()
} else if isStringLikelyCommitID(ctx.Repo.GetObjectFormat(), refName, 7) {
} else if git.IsStringLikelyCommitID(ctx.Repo.GetObjectFormat(), refName, 7) {
ctx.Repo.IsViewCommit = true
ctx.Repo.CommitID = refName

20
tests/integration/api_repo_compare_test.go
View File

@ -24,10 +24,9 @@ func TestAPICompareBranches(t *testing.T) {
session := loginUser(t, user.Name)
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
repoName := "repo20"
req := NewRequestf(t, "GET", "/api/v1/repos/user2/%s/compare/add-csv...remove-files-b", repoName).
AddTokenAuth(token)
t.Run("CompareBranches", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
req := NewRequestf(t, "GET", "/api/v1/repos/user2/repo20/compare/add-csv...remove-files-b").AddTokenAuth(token)
resp := MakeRequest(t, req, http.StatusOK)
var apiResp *api.Compare
@ -35,4 +34,17 @@ func TestAPICompareBranches(t *testing.T) {
assert.Equal(t, 2, apiResp.TotalCommits)
assert.Len(t, apiResp.Commits, 2)
})
t.Run("CompareCommits", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
req := NewRequestf(t, "GET", "/api/v1/repos/user2/repo20/compare/808038d2f71b0ab02099...c8e31bc7688741a5287f").AddTokenAuth(token)
resp := MakeRequest(t, req, http.StatusOK)
var apiResp *api.Compare
DecodeJSON(t, resp, &apiResp)
assert.Equal(t, 1, apiResp.TotalCommits)
assert.Len(t, apiResp.Commits, 1)
})
}

2
tests/integration/integration_test.go
View File

@ -440,7 +440,7 @@ func DecodeJSON(t testing.TB, resp *httptest.ResponseRecorder, v any) {
t.Helper()
decoder := json.NewDecoder(resp.Body)
assert.NoError(t, decoder.Decode(v))
require.NoError(t, decoder.Decode(v))
}
func VerifyJSONSchema(t testing.TB, resp *httptest.ResponseRecorder, schemaFile string) {