diff --git a/models/user/user.go b/models/user/user.go index 925be83713..1797d3eefc 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -1461,3 +1461,15 @@ func GetUserOrOrgIDByName(ctx context.Context, name string) (int64, error) { } return id, nil } + +// GetUserOrOrgByName returns the user or org by name +func GetUserOrOrgByName(ctx context.Context, name string) (*User, error) { + var u User + has, err := db.GetEngine(ctx).Where("lower_name = ?", strings.ToLower(name)).Get(&u) + if err != nil { + return nil, err + } else if !has { + return nil, ErrUserNotExist{Name: name} + } + return &u, nil +} diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 981d9de2f8..6ac612a481 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1862,6 +1862,7 @@ pulls.desc = Enable pull requests and code reviews. pulls.new = New Pull Request pulls.new.blocked_user = Cannot create pull request because you are blocked by the repository owner. pulls.new.must_collaborator = You must be a collaborator to create pull request. +pulls.new.already_existed = A pull request between these branches already exists pulls.edit.already_changed = Unable to save changes to the pull request. It appears the content has already been changed by another user. Please refresh the page and try editing again to avoid overwriting their changes. pulls.view = View Pull Request pulls.compare_changes = New Pull Request diff --git a/routers/api/v1/repo/compare.go b/routers/api/v1/repo/compare.go index 6d427c8073..9472bb7d3f 100644 --- a/routers/api/v1/repo/compare.go +++ b/routers/api/v1/repo/compare.go @@ -5,7 +5,6 @@ package repo import ( "net/http" - "strings" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/gitrepo" @@ -52,18 +51,7 @@ func CompareDiff(ctx *context.APIContext) { } } - 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} - } - } - } - - compareResult, closer := parseCompareInfo(ctx, api.CreatePullRequestOption{Base: infos[0], Head: infos[1]}) + compareResult, closer := parseCompareInfo(ctx, ctx.PathParam("*")) if ctx.Written() { return } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 073c784242..afa3e21860 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" @@ -413,7 +415,7 @@ func CreatePullRequest(ctx *context.APIContext) { ) // Get repo/branch information - compareResult, closer := parseCompareInfo(ctx, form) + compareResult, closer := parseCompareInfo(ctx, form.Base+".."+form.Head) if ctx.Written() { return } @@ -1065,61 +1067,37 @@ type parseCompareInfoResult struct { } // 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<-head: master...head:feature - // same repo: master...feature +func parseCompareInfo(ctx *context.APIContext, compareParam string) (result *parseCompareInfoResult, closer func()) { baseRepo := ctx.Repo.Repository - baseRefToGuess := form.Base - - 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. - // 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) { - ctx.APIErrorNotFound("GetUserByName") - } else { - ctx.APIErrorInternal(err) - } - return nil, nil - } - } else { + compareReq, err := common.ParseCompareRouterParam(compareParam) + if err != nil { ctx.APIErrorNotFound() return 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) - if err != nil { - ctx.APIErrorInternal(err) - 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.APIErrorNotFound("GetBaseRepo") - return nil, nil - } - // Assign headRepo so it can be used below. - headRepo = baseRepo.BaseRepo + // remove the check when we support compare with carets + if compareReq.CaretTimes > 0 { + ctx.APIErrorNotFound("Unsupported compare syntax with carets") + return nil, nil } + _, headRepo, err := common.GetHeadOwnerAndRepo(ctx, baseRepo, compareReq) + switch { + case errors.Is(err, util.ErrInvalidArgument): + ctx.APIError(http.StatusBadRequest, err.Error()) + return nil, nil + case err != nil: + ctx.APIErrorInternal(err) + return nil, nil + case user_model.IsErrUserNotExist(err) || repo_model.IsErrRepoNotExist(err): + ctx.APIErrorNotFound() + return nil, nil + } + + isSameRepo := baseRepo.ID == headRepo.ID + var headGitRepo *git.Repository 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 { @@ -1143,9 +1121,9 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) return nil, nil } - 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.APIErrorNotFound("Can't read pulls or can't read UnitTypeCode") + if !permBase.CanRead(unit.TypeCode) { + log.Trace("Permission Denied: User %-v cannot read code in Repo %-v\nUser in baseRepo has Permissions: %-+v", ctx.Doer, baseRepo, permBase) + ctx.APIErrorNotFound("can't read baseRepo UnitTypeCode") return nil, nil } @@ -1162,10 +1140,10 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) return nil, nil } - baseRef := ctx.Repo.GitRepo.UnstableGuessRefByShortName(baseRefToGuess) - headRef := headGitRepo.UnstableGuessRefByShortName(headRefToGuess) + baseRef := ctx.Repo.GitRepo.UnstableGuessRefByShortName(util.Iif(compareReq.BaseOriRef == "", baseRepo.DefaultBranch, compareReq.BaseOriRef)) + headRef := headGitRepo.UnstableGuessRefByShortName(util.Iif(compareReq.HeadOriRef == "", headRepo.DefaultBranch, compareReq.HeadOriRef)) - log.Trace("Repo path: %q, base ref: %q->%q, head ref: %q->%q", ctx.Repo.Repository.RelativePath(), baseRefToGuess, baseRef, headRefToGuess, headRef) + log.Trace("Repo path: %q, base ref: %q->%q, head ref: %q->%q", ctx.Repo.Repository.RelativePath(), compareReq.BaseOriRef, baseRef, compareReq.HeadOriRef, 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()) @@ -1175,7 +1153,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) return nil, nil } - compareInfo, err := pull_service.GetCompareInfo(ctx, baseRepo, headRepo, headGitRepo, baseRef.ShortName(), headRef.ShortName(), false, false) + compareInfo, err := pull_service.GetCompareInfo(ctx, baseRepo, headRepo, headGitRepo, baseRef.ShortName(), headRef.ShortName(), compareReq.DirectComparison(), false) if err != nil { ctx.APIErrorInternal(err) return nil, nil diff --git a/routers/common/compare.go b/routers/common/compare.go index fda31a07ba..06badd5408 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -4,15 +4,19 @@ package common import ( + "context" + "strings" + 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/util" pull_service "code.gitea.io/gitea/services/pull" ) // CompareInfo represents the collected results from ParseCompareInfo type CompareInfo struct { - HeadUser *user_model.User + HeadOwner *user_model.User HeadRepo *repo_model.Repository HeadGitRepo *git.Repository CompareInfo *pull_service.CompareInfo @@ -20,3 +24,194 @@ type CompareInfo struct { HeadBranch string DirectComparison bool } + +type CompareRouterReq struct { + BaseOriRef string + HeadOwner string + HeadRepoName string + HeadOriRef string + CaretTimes int // ^ times after base ref + DotTimes int +} + +func (cr *CompareRouterReq) DirectComparison() bool { + return cr.DotTimes == 2 || cr.DotTimes == 0 +} + +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] +} + +// ParseCompareRouterParam Get compare information from the router parameter. +// A full compare url is of the form: +// +// 1. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headBranch} +// 2. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}:{:headBranch} +// 3. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}/{:headRepoName}:{:headBranch} +// 4. /{:baseOwner}/{:baseRepoName}/compare/{:headBranch} +// 5. /{:baseOwner}/{:baseRepoName}/compare/{:headOwner}:{:headBranch} +// 6. /{:baseOwner}/{:baseRepoName}/compare/{:headOwner}/{:headRepoName}:{:headBranch} +// +// Here we obtain the infoPath "{:baseBranch}...[{:headOwner}/{:headRepoName}:]{:headBranch}" as ctx.PathParam("*") +// with the :baseRepo in ctx.Repo. +// +// Note: Generally :headRepoName is not provided here - we are only passed :headOwner. +// +// How do we determine the :headRepo? +// +// 1. If :headOwner is not set then the :headRepo = :baseRepo +// 2. If :headOwner is set - then look for the fork of :baseRepo owned by :headOwner +// 3. But... :baseRepo could be a fork of :headOwner's repo - so check that +// 4. Now, :baseRepo and :headRepos could be forks of the same repo - so check that +// +// format: ...[:] +// base<-head: master...head:feature +// same repo: master...feature +func ParseCompareRouterParam(routerParam string) (*CompareRouterReq, error) { + if routerParam == "" { + return &CompareRouterReq{}, nil + } + + var basePart, headPart string + dotTimes := 3 + parts := strings.Split(routerParam, "...") + if len(parts) > 2 { + return nil, util.NewInvalidArgumentErrorf("invalid compare router: %s", routerParam) + } + if len(parts) != 2 { + parts = strings.Split(routerParam, "..") + if len(parts) == 1 { + headOwnerName, headRepoName, headRef := parseHead(routerParam) + return &CompareRouterReq{ + HeadOriRef: headRef, + HeadOwner: headOwnerName, + HeadRepoName: headRepoName, + DotTimes: dotTimes, + }, nil + } else if len(parts) > 2 { + return nil, util.NewInvalidArgumentErrorf("invalid compare router: %s", routerParam) + } + dotTimes = 2 + } + basePart, headPart = parts[0], parts[1] + + baseRef, caretTimes := parseBase(basePart) + headOwnerName, headRepoName, headRef := parseHead(headPart) + + return &CompareRouterReq{ + BaseOriRef: baseRef, + HeadOriRef: headRef, + HeadOwner: headOwnerName, + HeadRepoName: headRepoName, + CaretTimes: caretTimes, + DotTimes: dotTimes, + }, nil +} + +// maxForkTraverseLevel defines the maximum levels to traverse when searching for the head repository. +const maxForkTraverseLevel = 10 + +// FindHeadRepo tries to find the head repository based on the base repository and head user ID. +func FindHeadRepo(ctx context.Context, baseRepo *repo_model.Repository, headUserID int64) (*repo_model.Repository, error) { + if baseRepo.IsFork { + curRepo := baseRepo + for curRepo.OwnerID != headUserID { // We assume the fork deepth is not too deep. + if err := curRepo.GetBaseRepo(ctx); err != nil { + return nil, err + } + if curRepo.BaseRepo == nil { + return findHeadRepoFromRootBase(ctx, curRepo, headUserID, maxForkTraverseLevel) + } + curRepo = curRepo.BaseRepo + } + return curRepo, nil + } + + return findHeadRepoFromRootBase(ctx, baseRepo, headUserID, maxForkTraverseLevel) +} + +func findHeadRepoFromRootBase(ctx context.Context, baseRepo *repo_model.Repository, headUserID int64, traverseLevel int) (*repo_model.Repository, error) { + if traverseLevel == 0 { + return nil, nil + } + repo, err := repo_model.GetUserFork(ctx, baseRepo.ID, headUserID) + if err != nil { + return nil, err + } + if repo != nil { + return repo, nil + } + + firstLevelForkedRepos, err := repo_model.GetRepositoriesByForkID(ctx, baseRepo.ID) + if err != nil { + return nil, err + } + for _, repo := range firstLevelForkedRepos { + forked, err := findHeadRepoFromRootBase(ctx, repo, headUserID, traverseLevel-1) + if err != nil { + return nil, err + } + if forked != nil { + return forked, nil + } + } + return nil, nil +} + +func GetHeadOwnerAndRepo(ctx context.Context, baseRepo *repo_model.Repository, compareReq *CompareRouterReq) (headOwner *user_model.User, headRepo *repo_model.Repository, err error) { + if compareReq.HeadOwner == "" { + if compareReq.HeadRepoName != "" { // unsupported syntax + return nil, nil, util.ErrorWrap(util.ErrInvalidArgument, "head owner must be specified when head repo name is given") + } + + return baseRepo.Owner, baseRepo, nil + } + + if compareReq.HeadOwner == baseRepo.Owner.Name { + headOwner = baseRepo.Owner + } else { + headOwner, err = user_model.GetUserOrOrgByName(ctx, compareReq.HeadOwner) + if err != nil { + return nil, nil, err + } + } + if compareReq.HeadRepoName == "" { + if headOwner.ID == baseRepo.OwnerID { + headRepo = baseRepo + } else { + headRepo, err = FindHeadRepo(ctx, baseRepo, headOwner.ID) + if err != nil { + return nil, nil, err + } + if headRepo == nil { + return nil, nil, util.ErrorWrap(util.ErrInvalidArgument, "the user %s does not have a fork of the base repository", headOwner.Name) + } + } + } else { + if compareReq.HeadOwner == baseRepo.Owner.Name && compareReq.HeadRepoName == baseRepo.Name { + headRepo = baseRepo + } else { + headRepo, err = repo_model.GetRepositoryByName(ctx, headOwner.ID, compareReq.HeadRepoName) + if err != nil { + return nil, nil, err + } + } + } + return headOwner, headRepo, nil +} diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go new file mode 100644 index 0000000000..a55f6607ae --- /dev/null +++ b/routers/common/compare_test.go @@ -0,0 +1,151 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package common + +import ( + "testing" + + "code.gitea.io/gitea/models/unittest" + + "github.com/stretchr/testify/assert" +) + +func TestCompareRouterReq(t *testing.T) { + unittest.PrepareTestEnv(t) + + kases := []struct { + router string + CompareRouterReq *CompareRouterReq + }{ + { + router: "", + CompareRouterReq: &CompareRouterReq{ + BaseOriRef: "", + HeadOriRef: "", + DotTimes: 0, + }, + }, + { + router: "main...develop", + CompareRouterReq: &CompareRouterReq{ + BaseOriRef: "main", + HeadOriRef: "develop", + DotTimes: 3, + }, + }, + { + router: "main..develop", + CompareRouterReq: &CompareRouterReq{ + BaseOriRef: "main", + HeadOriRef: "develop", + DotTimes: 2, + }, + }, + { + router: "main^...develop", + CompareRouterReq: &CompareRouterReq{ + BaseOriRef: "main", + HeadOriRef: "develop", + CaretTimes: 1, + DotTimes: 3, + }, + }, + { + router: "main^^^^^...develop", + CompareRouterReq: &CompareRouterReq{ + BaseOriRef: "main", + HeadOriRef: "develop", + CaretTimes: 5, + DotTimes: 3, + }, + }, + { + router: "develop", + CompareRouterReq: &CompareRouterReq{ + HeadOriRef: "develop", + DotTimes: 3, + }, + }, + { + router: "lunny/forked_repo:develop", + CompareRouterReq: &CompareRouterReq{ + HeadOwner: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + }, + }, + { + router: "main...lunny/forked_repo:develop", + CompareRouterReq: &CompareRouterReq{ + BaseOriRef: "main", + HeadOwner: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + }, + }, + { + router: "main...lunny/forked_repo:develop", + CompareRouterReq: &CompareRouterReq{ + BaseOriRef: "main", + HeadOwner: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + }, + }, + { + router: "main^...lunny/forked_repo:develop", + CompareRouterReq: &CompareRouterReq{ + BaseOriRef: "main", + HeadOwner: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + CaretTimes: 1, + }, + }, + { + router: "v1.0...v1.1", + CompareRouterReq: &CompareRouterReq{ + BaseOriRef: "v1.0", + HeadOriRef: "v1.1", + DotTimes: 3, + }, + }, + { + router: "teabot-patch-1...v0.0.1", + CompareRouterReq: &CompareRouterReq{ + BaseOriRef: "teabot-patch-1", + HeadOriRef: "v0.0.1", + DotTimes: 3, + }, + }, + { + router: "teabot:feature1", + CompareRouterReq: &CompareRouterReq{ + HeadOwner: "teabot", + HeadOriRef: "feature1", + DotTimes: 3, + }, + }, + { + router: "8eb19a5ae19abae15c0666d4ab98906139a7f439...283c030497b455ecfa759d4649f9f8b45158742e", + CompareRouterReq: &CompareRouterReq{ + BaseOriRef: "8eb19a5ae19abae15c0666d4ab98906139a7f439", + HeadOriRef: "283c030497b455ecfa759d4649f9f8b45158742e", + DotTimes: 3, + }, + }, + } + + for _, kase := range kases { + t.Run(kase.router, func(t *testing.T) { + r, err := ParseCompareRouterParam(kase.router) + assert.NoError(t, err) + assert.Equal(t, kase.CompareRouterReq, r) + }) + } +} diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 7750278a8d..605e910eb5 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -195,111 +195,40 @@ func setCsvCompareContext(ctx *context.Context) { 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: - // - // 1. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headBranch} - // 2. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}:{:headBranch} - // 3. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}/{:headRepoName}:{:headBranch} - // 4. /{:baseOwner}/{:baseRepoName}/compare/{:headBranch} - // 5. /{:baseOwner}/{:baseRepoName}/compare/{:headOwner}:{:headBranch} - // 6. /{:baseOwner}/{:baseRepoName}/compare/{:headOwner}/{:headRepoName}:{:headBranch} - // - // Here we obtain the infoPath "{:baseBranch}...[{:headOwner}/{:headRepoName}:]{:headBranch}" as ctx.PathParam("*") - // with the :baseRepo in ctx.Repo. - // - // Note: Generally :headRepoName is not provided here - we are only passed :headOwner. - // - // How do we determine the :headRepo? - // - // 1. If :headOwner is not set then the :headRepo = :baseRepo - // 2. If :headOwner is set - then look for the fork of :baseRepo owned by :headOwner - // 3. But... :baseRepo could be a fork of :headOwner's repo - so check that - // 4. Now, :baseRepo and :headRepos could be forks of the same repo - so check that - // - // format: ...[:] - // base<-head: master...head:feature - // same repo: master...feature - - var ( - isSameRepo bool - infoPath string - err error - ) - - 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} - } - } + compareReq, err := common.ParseCompareRouterParam(ctx.PathParam("*")) + if err != nil { + ctx.ServerError("GetUserByName", err) + return nil + } + // remove the check when we support compare with carets + if compareReq.CaretTimes > 0 { + ctx.HTTPError(http.StatusBadRequest, "Unsupported compare syntax with carets") + return nil } - 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(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(nil) - } else { - ctx.ServerError("GetRepositoryByOwnerAndName", err) - } - return nil - } - if err := ci.HeadRepo.LoadOwner(ctx); err != nil { - if user_model.IsErrUserNotExist(err) { - ctx.NotFound(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 { + ci.HeadOwner, ci.HeadRepo, err = common.GetHeadOwnerAndRepo(ctx, baseRepo, compareReq) + switch { + case errors.Is(err, util.ErrInvalidArgument): + ctx.HTTPError(http.StatusBadRequest, err.Error()) + return nil + case err != nil: + ctx.ServerError("GetHeadOwnerAndRepo", err) + return nil + case user_model.IsErrUserNotExist(err) || repo_model.IsErrRepoNotExist(err): ctx.NotFound(nil) return nil } - ctx.Data["HeadUser"] = ci.HeadUser + + ci.BaseBranch = util.Iif(compareReq.BaseOriRef == "", baseRepo.DefaultBranch, compareReq.BaseOriRef) + ci.HeadBranch = util.Iif(compareReq.HeadOriRef == "", ci.HeadRepo.DefaultBranch, compareReq.HeadOriRef) + ci.DirectComparison = compareReq.DirectComparison() + isSameRepo := baseRepo.ID == ci.HeadRepo.ID + + ctx.Data["BaseName"] = baseRepo.OwnerName + ctx.Data["BaseBranch"] = ci.BaseBranch + ctx.Data["HeadUser"] = ci.HeadOwner ctx.Data["HeadBranch"] = ci.HeadBranch ctx.Repo.PullRequest.SameRepo = isSameRepo @@ -371,27 +300,27 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { has := ci.HeadRepo != nil // 3. If the base is a forked from "RootRepo" and the owner of // the "RootRepo" is the :headUser - set headRepo to that - if !has && rootRepo != nil && rootRepo.OwnerID == ci.HeadUser.ID { + if !has && rootRepo != nil && rootRepo.OwnerID == ci.HeadOwner.ID { ci.HeadRepo = rootRepo has = true } // 4. If the ctx.Doer has their own fork of the baseRepo and the headUser is the ctx.Doer // set the headRepo to the ownFork - if !has && ownForkRepo != nil && ownForkRepo.OwnerID == ci.HeadUser.ID { + if !has && ownForkRepo != nil && ownForkRepo.OwnerID == ci.HeadOwner.ID { ci.HeadRepo = ownForkRepo has = true } // 5. If the headOwner has a fork of the baseRepo - use that if !has { - ci.HeadRepo = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ID) + ci.HeadRepo = repo_model.GetForkedRepo(ctx, ci.HeadOwner.ID, baseRepo.ID) has = ci.HeadRepo != nil } // 6. If the baseRepo is a fork and the headUser has a fork of that use that if !has && baseRepo.IsFork { - ci.HeadRepo = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ForkID) + ci.HeadRepo = repo_model.GetForkedRepo(ctx, ci.HeadOwner.ID, baseRepo.ForkID) has = ci.HeadRepo != nil } @@ -706,10 +635,10 @@ func PrepareCompareDiff( } ctx.Data["title"] = title - ctx.Data["Username"] = ci.HeadUser.Name + ctx.Data["Username"] = ci.HeadOwner.Name ctx.Data["Reponame"] = ci.HeadRepo.Name - setCompareContext(ctx, beforeCommit, headCommit, ci.HeadUser.Name, repo.Name) + setCompareContext(ctx, beforeCommit, headCommit, ci.HeadOwner.Name, repo.Name) return false } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 4353e00840..488389e204 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1340,6 +1340,17 @@ func CompareAndPullRequestPost(ctx *context.Context) { return } + // Check if a pull request already exists with the same head and base branch. + pr, err := issues_model.GetUnmergedPullRequest(ctx, ci.HeadRepo.ID, repo.ID, ci.HeadBranch, ci.BaseBranch, issues_model.PullRequestFlowGithub) + if err != nil && !issues_model.IsErrPullRequestNotExist(err) { + ctx.ServerError("GetUnmergedPullRequest", err) + return + } + if pr != nil { + ctx.JSONError(ctx.Tr("repo.pulls.new.already_existed")) + return + } + content := form.Content if filename := ctx.Req.Form.Get("template-file"); filename != "" { if template, err := issue_template.UnmarshalFromRepo(ctx.Repo.GitRepo, ctx.Repo.Repository.DefaultBranch, filename); err == nil { diff --git a/tests/integration/pull_create_test.go b/tests/integration/pull_create_test.go index ddafdf33b8..ff60b70cf9 100644 --- a/tests/integration/pull_create_test.go +++ b/tests/integration/pull_create_test.go @@ -4,6 +4,7 @@ package integration import ( + "encoding/base64" "fmt" "net/http" "net/http/httptest" @@ -17,7 +18,9 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/modules/git/gitcmd" + api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -153,8 +156,16 @@ func TestPullCreate(t *testing.T) { url := test.RedirectURL(resp) assert.Regexp(t, "^/user2/repo1/pulls/[0-9]*$", url) + // test create the pull request again and it should fail now + link := "/user2/repo1/compare/master...user1/repo1:master" + req := NewRequestWithValues(t, "POST", link, map[string]string{ + "_csrf": GetUserCSRFToken(t, session), + "title": "This is a pull title", + }) + session.MakeRequest(t, req, http.StatusBadRequest) + // check .diff can be accessed and matches performed change - req := NewRequest(t, "GET", url+".diff") + req = NewRequest(t, "GET", url+".diff") resp = session.MakeRequest(t, req, http.StatusOK) assert.Regexp(t, `\+Hello, World \(Edited\)`, resp.Body) assert.Regexp(t, "^diff", resp.Body) @@ -295,6 +306,95 @@ func TestPullCreatePrFromBaseToFork(t *testing.T) { }) } +func TestCreatePullRequestFromNestedOrgForks(t *testing.T) { + onGiteaRun(t, func(t *testing.T, _ *url.URL) { + session := loginUser(t, "user1") + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteOrganization) + + const ( + baseOrg = "test-fork-org1" + midForkOrg = "test-fork-org2" + leafForkOrg = "test-fork-org3" + repoName = "test-fork-repo" + patchBranch = "teabot-patch-1" + ) + + createOrg := func(name string) { + req := NewRequestWithJSON(t, "POST", "/api/v1/orgs", &api.CreateOrgOption{ + UserName: name, + Visibility: "public", + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + } + + createOrg(baseOrg) + createOrg(midForkOrg) + createOrg(leafForkOrg) + + req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/orgs/%s/repos", baseOrg), &api.CreateRepoOption{ + Name: repoName, + AutoInit: true, + DefaultBranch: "main", + Private: false, + Readme: "Default", + }).AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusCreated) + var baseRepo api.Repository + DecodeJSON(t, resp, &baseRepo) + assert.Equal(t, "main", baseRepo.DefaultBranch) + + forkIntoOrg := func(srcOrg, dstOrg string) api.Repository { + req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/forks", srcOrg, repoName), &api.CreateForkOption{ + Organization: util.ToPointer(dstOrg), + }).AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusAccepted) + var forkRepo api.Repository + DecodeJSON(t, resp, &forkRepo) + assert.NotNil(t, forkRepo.Owner) + if forkRepo.Owner != nil { + assert.Equal(t, dstOrg, forkRepo.Owner.UserName) + } + return forkRepo + } + + forkIntoOrg(baseOrg, midForkOrg) + forkIntoOrg(midForkOrg, leafForkOrg) + + req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s", leafForkOrg, repoName, "patch-from-org3.txt"), &api.CreateFileOptions{ + FileOptions: api.FileOptions{ + BranchName: "main", + NewBranchName: patchBranch, + Message: "create patch from org3", + }, + ContentBase64: base64.StdEncoding.EncodeToString([]byte("patch content")), + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + + prPayload := map[string]string{ + "head": fmt.Sprintf("%s:%s", leafForkOrg, patchBranch), + "base": "main", + "title": "test creating pull from test-fork-org3 to test-fork-org1", + } + req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/pulls", baseOrg, repoName), prPayload).AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusCreated) + var pr api.PullRequest + DecodeJSON(t, resp, &pr) + assert.Equal(t, prPayload["title"], pr.Title) + if assert.NotNil(t, pr.Head) { + assert.Equal(t, patchBranch, pr.Head.Ref) + if assert.NotNil(t, pr.Head.Repository) { + assert.Equal(t, fmt.Sprintf("%s/%s", leafForkOrg, repoName), pr.Head.Repository.FullName) + } + } + if assert.NotNil(t, pr.Base) { + assert.Equal(t, "main", pr.Base.Ref) + if assert.NotNil(t, pr.Base.Repository) { + assert.Equal(t, fmt.Sprintf("%s/%s", baseOrg, repoName), pr.Base.Repository.FullName) + } + } + }) +} + func TestPullCreateParallel(t *testing.T) { onGiteaRun(t, func(t *testing.T, u *url.URL) { sessionFork := loginUser(t, "user1")