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 22 commits into
base: main
Choose a base branch
from
+537 −172
Open

Refactor compare router param parse and fix bugs #36105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 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
6453e4a
some improvements
lunny Dec 12, 2025
64d6010
Merge branch 'main' into lunny/refactor_compare
lunny Dec 12, 2025
eb40447
remove duplicated code
lunny Dec 12, 2025
56e2b67
Merge branch 'main' into lunny/refactor_compare
lunny Dec 12, 2025
9868b2f
Don't return nilnil
lunny Dec 13, 2025
7ceeab1
Merge branch 'lunny/refactor_compare' of github.com:lunny/gitea into …
lunny Dec 13, 2025
a92efc1
Merge branch 'main' into lunny/refactor_compare
lunny Dec 13, 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
12 changes: 12 additions & 0 deletions models/user/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
1 change: 1 addition & 0 deletions options/locale/locale_en-US.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
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
84 changes: 31 additions & 53 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,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 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)
return nil, nil
}
// remove the check when we support compare with carets
if compareReq.CaretTimes > 0 {
ctx.APIErrorNotFound("Unsupported compare syntax with carets")
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
_, 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()
Comment on lines +1089 to +1093
Copy link
Contributor

@wxiaoguang wxiaoguang Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it right?

Copy link
Member Author

@lunny lunny Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no fork repository found in this user, It seems no other status code is better than 404.

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 {
Expand All @@ -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
}

Expand All @@ -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())
Expand All @@ -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
Expand Down
Loading