From f19b4b79677b90d99173378056d65edb43fc5775 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 10 Dec 2024 21:54:57 -0800 Subject: [PATCH] Refactor Parsing compare path parameters --- routers/api/v1/repo/compare.go | 93 +++++++++--- routers/api/v1/repo/pull.go | 191 ++++++------------------- routers/common/compare.go | 194 ++++++++++++++++++++++++- routers/common/compare_test.go | 120 ++++++++++++++++ routers/web/repo/compare.go | 253 +++++++-------------------------- routers/web/repo/pull.go | 4 +- 6 files changed, 484 insertions(+), 371 deletions(-) create mode 100644 routers/common/compare_test.go diff --git a/routers/api/v1/repo/compare.go b/routers/api/v1/repo/compare.go index 38e5330b3ac..6c4d428f228 100644 --- a/routers/api/v1/repo/compare.go +++ b/routers/api/v1/repo/compare.go @@ -4,12 +4,18 @@ package repo import ( + "errors" "net/http" - "strings" + access_model "code.gitea.io/gitea/models/perm/access" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/modules/log" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/routers/common" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" ) @@ -53,33 +59,84 @@ func CompareDiff(ctx *context.APIContext) { defer gitRepo.Close() } - infoPath := ctx.PathParam("*") - infos := []string{ctx.Repo.Repository.DefaultBranch, ctx.Repo.Repository.DefaultBranch} - if infoPath != "" { - infos = strings.SplitN(infoPath, "...", 2) - if len(infos) != 2 { - if infos = strings.SplitN(infoPath, "..", 2); len(infos) != 2 { - infos = []string{ctx.Repo.Repository.DefaultBranch, infoPath} + pathParam := ctx.PathParam("*") + baseRepo := ctx.Repo.Repository + ci, err := common.ParseComparePathParams(ctx, pathParam, baseRepo, ctx.Repo.GitRepo) + if err != nil { + switch { + case user_model.IsErrUserNotExist(err): + ctx.NotFound("GetUserByName") + case repo_model.IsErrRepoNotExist(err): + ctx.NotFound("GetRepositoryByOwnerAndName") + case errors.Is(err, util.ErrInvalidArgument): + ctx.NotFound("ParseComparePathParams") + default: + ctx.ServerError("GetRepositoryByOwnerAndName", err) + } + return + } + defer ci.Close() + + // remove the check when we support compare with carets + if ci.CaretTimes > 0 { + ctx.NotFound("Unsupported compare") + return + } + + if !ci.IsSameRepo() { + // user should have permission to read headrepo's codes + permHead, err := access_model.GetUserRepoPermission(ctx, ci.HeadRepo, ctx.Doer) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) + return + } + 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, + ci.HeadRepo, + permHead) } + ctx.NotFound("Can't read headRepo UnitTypeCode") + return } } - _, headGitRepo, ci, _, _ := parseCompareInfo(ctx, api.CreatePullRequestOption{ - Base: infos[0], - Head: infos[1], - }) - if ctx.Written() { + ctx.Repo.PullRequest.SameRepo = ci.IsSameRepo() + log.Trace("Repo path: %q, base branch: %q, head branch: %q", ctx.Repo.GitRepo.Path, ci.BaseOriRef, ci.HeadOriRef) + + // Check if current user has fork of repository or in the same repository. + /*headRepo := repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, ctx.Repo.Repository.ID) + if headRepo == nil && !ci.IsSameRepo() { + err := ctx.Repo.Repository.GetBaseRepo(ctx) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetBaseRepo", err) + return nil, 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, "", "" + } + // Assign headRepo so it can be used below. + headRepo = baseRepo.BaseRepo + }*/ + + ci.CompareInfo, err = ci.HeadGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), ci.BaseOriRef, ci.HeadOriRef, false, false) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetCompareInfo", err) return } - defer headGitRepo.Close() 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(ci.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(ci.CompareInfo.Commits); i++ { + apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, ci.CompareInfo.Commits[i], userCache, convert.ToCommitOptions{ Stat: true, Verification: verification, @@ -93,7 +150,7 @@ func CompareDiff(ctx *context.APIContext) { } ctx.JSON(http.StatusOK, &api.Compare{ - TotalCommits: len(ci.Commits), + TotalCommits: len(ci.CompareInfo.Commits), Commits: apiCommits, }) } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 86b204f51e2..fe23f89fbc6 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -28,8 +28,10 @@ import ( "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/timeutil" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/routers/api/v1/utils" + "code.gitea.io/gitea/routers/common" asymkey_service "code.gitea.io/gitea/services/asymkey" "code.gitea.io/gitea/services/automerge" "code.gitea.io/gitea/services/context" @@ -401,14 +403,48 @@ func CreatePullRequest(ctx *context.APIContext) { ) // Get repo/branch information - headRepo, headGitRepo, compareInfo, baseBranch, headBranch := parseCompareInfo(ctx, form) - if ctx.Written() { + ci, err := common.ParseComparePathParams(ctx, form.Base+"..."+form.Head, repo, ctx.Repo.GitRepo) + if err != nil { + switch { + case user_model.IsErrUserNotExist(err): + ctx.NotFound("GetUserByName") + case repo_model.IsErrRepoNotExist(err): + ctx.NotFound("GetRepositoryByOwnerAndName") + case errors.Is(err, util.ErrInvalidArgument): + ctx.NotFound("ParseComparePathParams") + default: + ctx.ServerError("GetRepositoryByOwnerAndName", err) + } return } - defer headGitRepo.Close() + defer ci.Close() + + if ci.IsPull() { + ctx.Error(http.StatusUnprocessableEntity, "Bad base or head refs", "Only support branch to branch comparison") + return + } + + if !ci.IsSameRepo() { + // user should have permission to read headrepo's codes + permHead, err := access_model.GetUserRepoPermission(ctx, ci.HeadRepo, ctx.Doer) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) + return + } + 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, + ci.HeadRepo, + permHead) + } + ctx.NotFound("Can't read headRepo UnitTypeCode") + 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, ci.HeadRepo.ID, ctx.Repo.Repository.ID, ci.HeadOriRef, ci.BaseOriRef, issues_model.PullRequestFlowGithub) if err != nil { if !issues_model.IsErrPullRequestNotExist(err) { ctx.Error(http.StatusInternalServerError, "GetUnmergedPullRequest", err) @@ -484,13 +520,13 @@ func CreatePullRequest(ctx *context.APIContext) { DeadlineUnix: deadlineUnix, } pr := &issues_model.PullRequest{ - HeadRepoID: headRepo.ID, + HeadRepoID: ci.HeadRepo.ID, BaseRepoID: repo.ID, - HeadBranch: headBranch, - BaseBranch: baseBranch, - HeadRepo: headRepo, + HeadBranch: ci.HeadOriRef, + BaseBranch: ci.BaseOriRef, + HeadRepo: ci.HeadRepo, BaseRepo: repo, - MergeBase: compareInfo.MergeBase, + MergeBase: ci.CompareInfo.MergeBase, Type: issues_model.PullRequestGitea, } @@ -1080,143 +1116,6 @@ 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 - - // Get compared branches information - // format: ...[:] - // base<-head: master...head:feature - // same repo: master...feature - - // TODO: Validate form first? - - baseBranch := form.Base - - var ( - headUser *user_model.User - headBranch string - isSameRepo bool - err error - ) - - // 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] - } else if len(headInfos) == 2 { - headUser, err = user_model.GetUserByName(ctx, headInfos[0]) - if err != nil { - if user_model.IsErrUserNotExist(err) { - ctx.NotFound("GetUserByName") - } else { - ctx.Error(http.StatusInternalServerError, "GetUserByName", err) - } - return nil, 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, "", "" - } - - 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, "", "" - } - - // 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) - if err != nil { - ctx.Error(http.StatusInternalServerError, "GetBaseRepo", err) - return nil, 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, "", "" - } - // Assign headRepo so it can be used below. - headRepo = baseRepo.BaseRepo - } - - var headGitRepo *git.Repository - if isSameRepo { - headRepo = ctx.Repo.Repository - headGitRepo = ctx.Repo.GitRepo - } else { - headGitRepo, err = gitrepo.OpenRepository(ctx, headRepo) - if err != nil { - ctx.Error(http.StatusInternalServerError, "OpenRepository", err) - return nil, nil, nil, "", "" - } - } - - // 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, "", "" - } - - // user should have permission to read headrepo's codes - permHead, err := access_model.GetUserRepoPermission(ctx, headRepo, ctx.Doer) - if err != nil { - headGitRepo.Close() - ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) - return nil, 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() - ctx.NotFound("Can't read headRepo UnitTypeCode") - return nil, nil, nil, "", "" - } - - // Check if head branch is valid. - if !headGitRepo.IsBranchExist(headBranch) && !headGitRepo.IsTagExist(headBranch) { - headGitRepo.Close() - ctx.NotFound() - return nil, nil, nil, "", "" - } - - compareInfo, err := headGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), baseBranch, headBranch, false, false) - if err != nil { - headGitRepo.Close() - ctx.Error(http.StatusInternalServerError, "GetCompareInfo", err) - return nil, nil, nil, "", "" - } - - return headRepo, headGitRepo, compareInfo, baseBranch, headBranch -} - // UpdatePullRequest merge PR's baseBranch into headBranch func UpdatePullRequest(ctx *context.APIContext) { // swagger:operation POST /repos/{owner}/{repo}/pulls/{index}/update repository repoUpdatePullRequest diff --git a/routers/common/compare.go b/routers/common/compare.go index 4d1cc2f0d89..d1b4327a1c7 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -4,18 +4,198 @@ package common import ( + "context" + "strings" + + git_model "code.gitea.io/gitea/models/git" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/modules/util" ) +type CompareRouter struct { + BaseOriRef string + BaseFullRef git.RefName + HeadOwnerName string + HeadRepoName string + HeadOriRef string + HeadFullRef git.RefName + CaretTimes int // ^ times after base ref + DotTimes int // 2(..) or 3(...) +} + +func (cr *CompareRouter) IsSameRepo() bool { + return cr.HeadOwnerName == "" && cr.HeadRepoName == "" +} + +func (cr *CompareRouter) IsSameRef() bool { + return cr.IsSameRepo() && cr.BaseOriRef == cr.HeadOriRef +} + +func (cr *CompareRouter) DirectComparison() bool { + return cr.DotTimes == 2 +} + +func parseBase(base string) (string, int) { + parts := strings.SplitN(base, "^", 2) + if len(parts) == 1 { + return base, 0 + } + return parts[0], len(parts[1]) + 1 +} + +func parseHead(head string) (string, string, string) { + paths := strings.SplitN(head, ":", 2) + if len(paths) == 1 { + return "", "", paths[0] + } + ownerRepo := strings.SplitN(paths[0], "/", 2) + if len(ownerRepo) == 1 { + return "", paths[0], paths[1] + } + return ownerRepo[0], ownerRepo[1], paths[1] +} + +func parseCompareRouter(router string) (*CompareRouter, error) { + var basePart, headPart string + dotTimes := 3 + parts := strings.Split(router, "...") + if len(parts) > 2 { + return nil, util.NewSilentWrapErrorf(util.ErrInvalidArgument, "invalid compare router: %s", router) + } + if len(parts) != 2 { + parts = strings.Split(router, "..") + if len(parts) == 1 { + headOwnerName, headRepoName, headRef := parseHead(router) + return &CompareRouter{ + HeadOriRef: headRef, + HeadOwnerName: headOwnerName, + HeadRepoName: headRepoName, + }, nil + } else if len(parts) > 2 { + return nil, util.NewSilentWrapErrorf(util.ErrInvalidArgument, "invalid compare router: %s", router) + } + dotTimes = 2 + } + basePart, headPart = parts[0], parts[1] + + baseRef, caretTimes := parseBase(basePart) + headOwnerName, headRepoName, headRef := parseHead(headPart) + + return &CompareRouter{ + BaseOriRef: baseRef, + HeadOriRef: headRef, + HeadOwnerName: headOwnerName, + HeadRepoName: headRepoName, + CaretTimes: caretTimes, + DotTimes: dotTimes, + }, nil +} + // CompareInfo represents the collected results from ParseCompareInfo type CompareInfo struct { - HeadUser *user_model.User - HeadRepo *repo_model.Repository - HeadGitRepo *git.Repository - CompareInfo *git.CompareInfo - BaseBranch string - HeadBranch string - DirectComparison bool + *CompareRouter + HeadUser *user_model.User + HeadRepo *repo_model.Repository + HeadGitRepo *git.Repository + CompareInfo *git.CompareInfo + close func() + IsBaseCommit bool + IsHeadCommit bool +} + +// display pull related information or not +func (ci *CompareInfo) IsPull() bool { + return ci.CaretTimes == 0 && !ci.DirectComparison() && + ci.BaseFullRef.IsBranch() && ci.HeadFullRef.IsBranch() +} + +func (ci *CompareInfo) Close() { + if ci.close != nil { + ci.close() + } +} + +func detectFullRef(ctx context.Context, repoID int64, gitRepo *git.Repository, oriRef string) (git.RefName, bool, error) { + b, err := git_model.GetBranch(ctx, repoID, oriRef) + if err != nil { + return "", false, err + } + if !b.IsDeleted { + return git.RefNameFromBranch(oriRef), false, nil + } + + rel, err := repo_model.GetRelease(ctx, repoID, oriRef) + if err != nil && !repo_model.IsErrReleaseNotExist(err) { + return "", false, err + } + if rel != nil && rel.Sha1 != "" { + return git.RefNameFromTag(oriRef), false, nil + } + + commitObjectID, err := gitRepo.ConvertToGitID(oriRef) + if err != nil { + return "", false, err + } + return git.RefName(commitObjectID.String()), true, nil +} + +func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *repo_model.Repository, baseGitRepo *git.Repository) (*CompareInfo, error) { + ci := &CompareInfo{} + var err error + + if pathParam == "" { + ci.HeadOriRef = baseRepo.DefaultBranch + } else { + ci.CompareRouter, err = parseCompareRouter(pathParam) + if err != nil { + return nil, err + } + } + if ci.BaseOriRef == "" { + ci.BaseOriRef = baseRepo.DefaultBranch + } + + if ci.IsSameRepo() { + ci.HeadUser = baseRepo.Owner + ci.HeadRepo = baseRepo + ci.HeadGitRepo = baseGitRepo + } else { + if ci.HeadOwnerName == baseRepo.Owner.Name { + ci.HeadUser = baseRepo.Owner + } else { + ci.HeadUser, err = user_model.GetUserByName(ctx, ci.HeadOwnerName) + if err != nil { + return nil, err + } + } + + ci.HeadRepo, err = repo_model.GetRepositoryByOwnerAndName(ctx, ci.HeadOwnerName, ci.HeadRepoName) + if err != nil { + return nil, err + } + ci.HeadRepo.Owner = ci.HeadUser + ci.HeadGitRepo, err = gitrepo.OpenRepository(ctx, ci.HeadRepo) + if err != nil { + return nil, err + } + ci.close = func() { + if ci.HeadGitRepo != nil { + ci.HeadGitRepo.Close() + } + } + } + + ci.BaseFullRef, ci.IsBaseCommit, err = detectFullRef(ctx, baseRepo.ID, baseGitRepo, ci.BaseOriRef) + if err != nil { + return nil, err + } + + ci.HeadFullRef, ci.IsHeadCommit, err = detectFullRef(ctx, ci.HeadRepo.ID, ci.HeadGitRepo, ci.HeadOriRef) + if err != nil { + return nil, err + } + return ci, nil } diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go new file mode 100644 index 00000000000..2341f807928 --- /dev/null +++ b/routers/common/compare_test.go @@ -0,0 +1,120 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package common + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCompareRouters(t *testing.T) { + kases := []struct { + router string + compareRouter *CompareRouter + }{ + { + router: "main...develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOriRef: "develop", + DotTimes: 3, + }, + }, + { + router: "main..develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOriRef: "develop", + DotTimes: 2, + }, + }, + { + router: "main^...develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOriRef: "develop", + CaretTimes: 1, + DotTimes: 3, + }, + }, + { + router: "main^^^^^...develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOriRef: "develop", + CaretTimes: 5, + DotTimes: 3, + }, + }, + { + router: "develop", + compareRouter: &CompareRouter{ + HeadOriRef: "develop", + }, + }, + { + router: "lunny/forked_repo:develop", + compareRouter: &CompareRouter{ + HeadOwnerName: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + }, + }, + { + router: "main...lunny/forked_repo:develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOwnerName: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + }, + }, + { + router: "main...lunny/forked_repo:develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOwnerName: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + }, + }, + { + router: "main^...lunny/forked_repo:develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOwnerName: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + CaretTimes: 1, + }, + }, + { + router: "v1.0...v1.1", + compareRouter: &CompareRouter{ + BaseOriRef: "v1.0", + HeadOriRef: "v1.1", + DotTimes: 3, + }, + }, + { + router: "8eb19a5ae19abae15c0666d4ab98906139a7f439...283c030497b455ecfa759d4649f9f8b45158742e", + compareRouter: &CompareRouter{ + BaseOriRef: "8eb19a5ae19abae15c0666d4ab98906139a7f439", + HeadOriRef: "283c030497b455ecfa759d4649f9f8b45158742e", + DotTimes: 3, + }, + }, + } + for _, kase := range kases { + t.Run(kase.router, func(t *testing.T) { + r, err := parseCompareRouter(kase.router) + assert.NoError(t, err) + assert.EqualValues(t, kase.compareRouter, r) + }) + } +} diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 278974bec3e..4279cf9b3a2 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -187,12 +187,8 @@ func setCsvCompareContext(ctx *context.Context) { } // ParseCompareInfo parse compare info between two commit for preparing comparing references +// Permission check for base repository's code read should be checked before invoking this function func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { - baseRepo := ctx.Repo.Repository - ci := &common.CompareInfo{} - - fileOnly := ctx.FormBool("file-only") - // Get compared branches information // A full compare url is of the form: // @@ -219,111 +215,51 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { // base<-head: master...head:feature // same repo: master...feature - var ( - isSameRepo bool - infoPath string - err error - ) + fileOnly := ctx.FormBool("file-only") + pathParam := ctx.PathParam("*") + baseRepo := ctx.Repo.Repository - infoPath = ctx.PathParam("*") - var infos []string - if infoPath == "" { - infos = []string{baseRepo.DefaultBranch, baseRepo.DefaultBranch} - } else { - infos = strings.SplitN(infoPath, "...", 2) - if len(infos) != 2 { - if infos = strings.SplitN(infoPath, "..", 2); len(infos) == 2 { - ci.DirectComparison = true - ctx.Data["PageIsComparePull"] = false - } else { - infos = []string{baseRepo.DefaultBranch, infoPath} - } + ci, err := common.ParseComparePathParams(ctx, pathParam, baseRepo, ctx.Repo.GitRepo) + if err != nil { + switch { + case user_model.IsErrUserNotExist(err): + ctx.NotFound("GetUserByName", nil) + case repo_model.IsErrRepoNotExist(err): + ctx.NotFound("GetRepositoryByOwnerAndName", nil) + case errors.Is(err, util.ErrInvalidArgument): + ctx.NotFound("ParseComparePathParams", nil) + default: + ctx.ServerError("GetRepositoryByOwnerAndName", err) } - } - - ctx.Data["BaseName"] = baseRepo.OwnerName - ci.BaseBranch = infos[0] - ctx.Data["BaseBranch"] = ci.BaseBranch - - // If there is no head repository, it means compare between same repository. - headInfos := strings.Split(infos[1], ":") - if len(headInfos) == 1 { - isSameRepo = true - ci.HeadUser = ctx.Repo.Owner - ci.HeadBranch = headInfos[0] - } else if len(headInfos) == 2 { - headInfosSplit := strings.Split(headInfos[0], "/") - if len(headInfosSplit) == 1 { - ci.HeadUser, err = user_model.GetUserByName(ctx, headInfos[0]) - if err != nil { - if user_model.IsErrUserNotExist(err) { - ctx.NotFound("GetUserByName", nil) - } else { - ctx.ServerError("GetUserByName", err) - } - return nil - } - ci.HeadBranch = headInfos[1] - isSameRepo = ci.HeadUser.ID == ctx.Repo.Owner.ID - if isSameRepo { - ci.HeadRepo = baseRepo - } - } else { - ci.HeadRepo, err = repo_model.GetRepositoryByOwnerAndName(ctx, headInfosSplit[0], headInfosSplit[1]) - if err != nil { - if repo_model.IsErrRepoNotExist(err) { - ctx.NotFound("GetRepositoryByOwnerAndName", nil) - } else { - ctx.ServerError("GetRepositoryByOwnerAndName", err) - } - return nil - } - if err := ci.HeadRepo.LoadOwner(ctx); err != nil { - if user_model.IsErrUserNotExist(err) { - ctx.NotFound("GetUserByName", nil) - } else { - ctx.ServerError("GetUserByName", err) - } - return nil - } - ci.HeadBranch = headInfos[1] - ci.HeadUser = ci.HeadRepo.Owner - isSameRepo = ci.HeadRepo.ID == ctx.Repo.Repository.ID - } - } else { - ctx.NotFound("CompareAndPullRequest", nil) return nil } - ctx.Data["HeadUser"] = ci.HeadUser - ctx.Data["HeadBranch"] = ci.HeadBranch - ctx.Repo.PullRequest.SameRepo = isSameRepo + defer ci.Close() - // Check if base branch is valid. - baseIsCommit := ctx.Repo.GitRepo.IsCommitExist(ci.BaseBranch) - baseIsBranch := ctx.Repo.GitRepo.IsBranchExist(ci.BaseBranch) - baseIsTag := ctx.Repo.GitRepo.IsTagExist(ci.BaseBranch) - - if !baseIsCommit && !baseIsBranch && !baseIsTag { - // Check if baseBranch is short sha commit hash - if baseCommit, _ := ctx.Repo.GitRepo.GetCommit(ci.BaseBranch); baseCommit != nil { - ci.BaseBranch = baseCommit.ID.String() - ctx.Data["BaseBranch"] = ci.BaseBranch - baseIsCommit = true - } else if ci.BaseBranch == ctx.Repo.GetObjectFormat().EmptyObjectID().String() { - if isSameRepo { - ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadBranch)) - } else { - ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadRepo.FullName()) + ":" + util.PathEscapeSegments(ci.HeadBranch)) - } - return nil - } else { - ctx.NotFound("IsRefExist", nil) - return nil - } + // remove the check when we support compare with carets + if ci.CaretTimes > 0 { + ctx.NotFound("Unsupported compare", nil) + return nil } - ctx.Data["BaseIsCommit"] = baseIsCommit - ctx.Data["BaseIsBranch"] = baseIsBranch - ctx.Data["BaseIsTag"] = baseIsTag + + if ci.BaseOriRef == ctx.Repo.GetObjectFormat().EmptyObjectID().String() { + if ci.IsSameRepo() { + ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadOriRef)) + } else { + ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadRepo.FullName()) + ":" + util.PathEscapeSegments(ci.HeadOriRef)) + } + return nil + } + + ctx.Data["PageIsComparePull"] = ci.IsPull() && ctx.Repo.CanReadIssuesOrPulls(true) + ctx.Data["BaseName"] = baseRepo.OwnerName + ctx.Data["BaseBranch"] = ci.BaseOriRef + ctx.Data["HeadUser"] = ci.HeadUser + ctx.Data["HeadBranch"] = ci.HeadOriRef + ctx.Repo.PullRequest.SameRepo = ci.IsSameRepo() + + ctx.Data["BaseIsCommit"] = ci.IsBaseCommit + ctx.Data["BaseIsBranch"] = ci.BaseFullRef.IsBranch() + ctx.Data["BaseIsTag"] = ci.BaseFullRef.IsTag() ctx.Data["IsPull"] = true // Now we have the repository that represents the base @@ -391,50 +327,15 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { } // 7. Otherwise if we're not the same repo and haven't found a repo give up - if !isSameRepo && !has { + if !ci.IsSameRepo() && !has { ctx.Data["PageIsComparePull"] = false } - // 8. Finally open the git repo - if isSameRepo { - ci.HeadRepo = ctx.Repo.Repository - ci.HeadGitRepo = ctx.Repo.GitRepo - } else if has { - ci.HeadGitRepo, err = gitrepo.OpenRepository(ctx, ci.HeadRepo) - if err != nil { - ctx.ServerError("OpenRepository", err) - return nil - } - defer ci.HeadGitRepo.Close() - } else { - ctx.NotFound("ParseCompareInfo", nil) - return nil - } - ctx.Data["HeadRepo"] = ci.HeadRepo ctx.Data["BaseCompareRepo"] = ctx.Repo.Repository - // Now we need to assert that the ctx.Doer has permission to read - // the baseRepo's code and pulls - // (NOT headRepo's) - permBase, err := access_model.GetUserRepoPermission(ctx, baseRepo, ctx.Doer) - if err != nil { - ctx.ServerError("GetUserRepoPermission", err) - return nil - } - if !permBase.CanRead(unit.TypeCode) { - if log.IsTrace() { - log.Trace("Permission Denied: User: %-v cannot read code in Repo: %-v\nUser in baseRepo has Permissions: %-+v", - ctx.Doer, - baseRepo, - permBase) - } - ctx.NotFound("ParseCompareInfo", nil) - return nil - } - // If we're not merging from the same repo: - if !isSameRepo { + if !ci.IsSameRepo() { // Assert ctx.Doer has permission to read headRepo's codes permHead, err := access_model.GetUserRepoPermission(ctx, ci.HeadRepo, ctx.Doer) if err != nil { @@ -501,60 +402,16 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { } } - // Check if head branch is valid. - headIsCommit := ci.HeadGitRepo.IsCommitExist(ci.HeadBranch) - headIsBranch := ci.HeadGitRepo.IsBranchExist(ci.HeadBranch) - headIsTag := ci.HeadGitRepo.IsTagExist(ci.HeadBranch) - if !headIsCommit && !headIsBranch && !headIsTag { - // Check if headBranch is short sha commit hash - if headCommit, _ := ci.HeadGitRepo.GetCommit(ci.HeadBranch); headCommit != nil { - ci.HeadBranch = headCommit.ID.String() - ctx.Data["HeadBranch"] = ci.HeadBranch - headIsCommit = true - } else { - ctx.NotFound("IsRefExist", nil) - return nil - } - } - ctx.Data["HeadIsCommit"] = headIsCommit - ctx.Data["HeadIsBranch"] = headIsBranch - ctx.Data["HeadIsTag"] = headIsTag + ctx.Data["HeadIsCommit"] = ci.IsHeadCommit + ctx.Data["HeadIsBranch"] = ci.HeadFullRef.IsBranch() + ctx.Data["HeadIsTag"] = ci.HeadFullRef.IsTag() - // Treat as pull request if both references are branches - if ctx.Data["PageIsComparePull"] == nil { - ctx.Data["PageIsComparePull"] = headIsBranch && baseIsBranch - } - - if ctx.Data["PageIsComparePull"] == true && !permBase.CanReadIssuesOrPulls(true) { - if log.IsTrace() { - log.Trace("Permission Denied: User: %-v cannot create/read pull requests in Repo: %-v\nUser in baseRepo has Permissions: %-+v", - ctx.Doer, - baseRepo, - permBase) - } - ctx.NotFound("ParseCompareInfo", nil) - return nil - } - - baseBranchRef := ci.BaseBranch - if baseIsBranch { - baseBranchRef = git.BranchPrefix + ci.BaseBranch - } else if baseIsTag { - baseBranchRef = git.TagPrefix + ci.BaseBranch - } - headBranchRef := ci.HeadBranch - if headIsBranch { - headBranchRef = git.BranchPrefix + ci.HeadBranch - } else if headIsTag { - headBranchRef = git.TagPrefix + ci.HeadBranch - } - - ci.CompareInfo, err = ci.HeadGitRepo.GetCompareInfo(baseRepo.RepoPath(), baseBranchRef, headBranchRef, ci.DirectComparison, fileOnly) + ci.CompareInfo, err = ci.HeadGitRepo.GetCompareInfo(baseRepo.RepoPath(), ci.BaseFullRef.String(), ci.HeadFullRef.String(), ci.DirectComparison(), fileOnly) if err != nil { ctx.ServerError("GetCompareInfo", err) return nil } - if ci.DirectComparison { + if ci.DirectComparison() { ctx.Data["BeforeCommitID"] = ci.CompareInfo.BaseCommitID } else { ctx.Data["BeforeCommitID"] = ci.CompareInfo.MergeBase @@ -582,14 +439,14 @@ func PrepareCompareDiff( ctx.Data["AfterCommitID"] = headCommitID - if (headCommitID == ci.CompareInfo.MergeBase && !ci.DirectComparison) || + if (headCommitID == ci.CompareInfo.MergeBase && !ci.DirectComparison()) || headCommitID == ci.CompareInfo.BaseCommitID { ctx.Data["IsNothingToCompare"] = true if unit, err := repo.GetUnit(ctx, unit.TypePullRequests); err == nil { config := unit.PullRequestsConfig() if !config.AutodetectManualMerge { - allowEmptyPr := !(ci.BaseBranch == ci.HeadBranch && ctx.Repo.Repository.Name == ci.HeadRepo.Name) + allowEmptyPr := !(ci.BaseOriRef == ci.HeadOriRef && ctx.Repo.Repository.Name == ci.HeadRepo.Name) ctx.Data["AllowEmptyPr"] = allowEmptyPr return !allowEmptyPr @@ -601,7 +458,7 @@ func PrepareCompareDiff( } beforeCommitID := ci.CompareInfo.MergeBase - if ci.DirectComparison { + if ci.DirectComparison() { beforeCommitID = ci.CompareInfo.BaseCommitID } @@ -622,7 +479,7 @@ func PrepareCompareDiff( MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters, MaxFiles: maxFiles, WhitespaceBehavior: whitespaceBehavior, - DirectComparison: ci.DirectComparison, + DirectComparison: ci.DirectComparison(), FileOnly: fileOnly, }, ctx.FormStrings("files")...) if err != nil { @@ -659,7 +516,7 @@ func PrepareCompareDiff( ctx.Data["content"] = strings.Join(body[1:], "\n") } } else { - title = ci.HeadBranch + title = ci.HeadOriRef } if len(title) > 255 { var trailer string @@ -720,7 +577,7 @@ func CompareDiff(ctx *context.Context) { ctx.Data["DirectComparison"] = ci.DirectComparison ctx.Data["OtherCompareSeparator"] = ".." ctx.Data["CompareSeparator"] = "..." - if ci.DirectComparison { + if ci.DirectComparison() { ctx.Data["CompareSeparator"] = ".." ctx.Data["OtherCompareSeparator"] = "..." } @@ -769,7 +626,7 @@ func CompareDiff(ctx *context.Context) { ctx.Data["HeadTags"] = headTags if ctx.Data["PageIsComparePull"] == true { - pr, err := issues_model.GetUnmergedPullRequest(ctx, ci.HeadRepo.ID, ctx.Repo.Repository.ID, ci.HeadBranch, ci.BaseBranch, issues_model.PullRequestFlowGithub) + pr, err := issues_model.GetUnmergedPullRequest(ctx, ci.HeadRepo.ID, ctx.Repo.Repository.ID, ci.HeadOriRef, ci.BaseOriRef, issues_model.PullRequestFlowGithub) if err != nil { if !issues_model.IsErrPullRequestNotExist(err) { ctx.ServerError("GetUnmergedPullRequest", err) @@ -802,7 +659,7 @@ func CompareDiff(ctx *context.Context) { afterCommitID := ctx.Data["AfterCommitID"].(string) separator := "..." - if ci.DirectComparison { + if ci.DirectComparison() { separator = ".." } ctx.Data["Title"] = "Comparing " + base.ShortSha(beforeCommitID) + separator + base.ShortSha(afterCommitID) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 9694ae845b1..fe0469e95ee 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1315,8 +1315,8 @@ func CompareAndPullRequestPost(ctx *context.Context) { pullRequest := &issues_model.PullRequest{ HeadRepoID: ci.HeadRepo.ID, BaseRepoID: repo.ID, - HeadBranch: ci.HeadBranch, - BaseBranch: ci.BaseBranch, + HeadBranch: ci.HeadOriRef, + BaseBranch: ci.BaseOriRef, HeadRepo: ci.HeadRepo, BaseRepo: repo, MergeBase: ci.CompareInfo.MergeBase,