Skip to content

Refactor compare router param parse and fix bugs #36105

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
lunny wants to merge 15 commits into
base: main
Choose a base branch
from
+558 −155
Open

Refactor compare router param parse and fix bugs #36105

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7fe0840
Refactor compare router param parse
lunny Dec 8, 2025
3c51f38
Refactor compare router param parse
lunny Dec 8, 2025
a2c8e9c
Fix test
lunny Dec 8, 2025
229b20b
Merge branch 'main' into lunny/refactor_compare
lunny Dec 8, 2025
44925e3
Update routers/common/compare_test.go
lunny Dec 9, 2025
8a83a64
Fix
lunny Dec 10, 2025
eaf8dff
Merge branch 'main' into lunny/refactor_compare
lunny Dec 10, 2025
0166880
Merge branch 'lunny/refactor_compare' of github.com:lunny/gitea into …
lunny Dec 10, 2025
dcb8e47
Fix bug
lunny Dec 10, 2025
e5dbcbd
Merge branch 'main' into lunny/refactor_compare
lunny Dec 10, 2025
aebb29c
Fix bug and add test
lunny Dec 10, 2025
c3c4e3c
remove unnecessary code
lunny Dec 10, 2025
220a908
Remove unnecessary permission check
lunny Dec 11, 2025
3bd496b
Prevent to create duplicated pull request
lunny Dec 11, 2025
6ed0c75
Merge branch 'main' into lunny/refactor_compare
lunny Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 1 addition & 13 deletions routers/api/v1/repo/compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package repo

import (
"net/http"
"strings"

user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/gitrepo"
Expand Down Expand Up @@ -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
}
Expand Down
111 changes: 64 additions & 47 deletions routers/api/v1/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -1065,61 +1067,76 @@ 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 branch>...[<head repo>:]<head branch>
// 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)
var headRepo *repo_model.Repository
if compareReq.HeadOwner == "" {
if compareReq.HeadRepoName != "" { // unsupported syntax
ctx.APIErrorNotFound()
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
headRepo = ctx.Repo.Repository
} else {
var headUser *user_model.User
if compareReq.HeadOwner == ctx.Repo.Owner.Name {
headUser = ctx.Repo.Owner
} else {
headUser, err = user_model.GetUserByName(ctx, compareReq.HeadOwner)
if err != nil {
if user_model.IsErrUserNotExist(err) {
ctx.APIErrorNotFound("GetUserByName")
} else {
ctx.APIErrorInternal(err)
}
return nil, nil
}
}
if compareReq.HeadRepoName == "" {
headRepo = repo_model.GetForkedRepo(ctx, headUser.ID, baseRepo.ID)
if headRepo == nil && headUser.ID != baseRepo.OwnerID {
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
}
} else {
if compareReq.HeadOwner == ctx.Repo.Owner.Name && compareReq.HeadRepoName == ctx.Repo.Repository.Name {
headRepo = ctx.Repo.Repository
} else {
headRepo, err = repo_model.GetRepositoryByName(ctx, headUser.ID, compareReq.HeadRepoName)
if err != nil {
if repo_model.IsErrRepoNotExist(err) {
ctx.APIErrorNotFound("GetRepositoryByName")
} else {
ctx.APIErrorInternal(err)
}
return nil, nil
}
}
}
// Assign headRepo so it can be used below.
headRepo = baseRepo.BaseRepo
}

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 {
Expand Down Expand Up @@ -1162,10 +1179,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())
Expand All @@ -1175,7 +1192,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
Expand Down
102 changes: 102 additions & 0 deletions routers/common/compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@
package common

import (
"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"
)

Expand All @@ -20,3 +23,102 @@ 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.CaretTimes == 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 branch>...[<head repo>:]<head branch>
// 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
}
Loading
Loading