public/gitea
1
0
Fork 0
mirror of https://github.com/go-gitea/gitea.git synced 2026-02-07 09:49:41 +09:00
Code Issues Packages Projects Releases Wiki Activity

improve the compare page (#36261)

Browse Source 
- The compare page head title should be `compare` but not `new pull
request`.
- Use `UnstableGuessRefByShortName` instead of duplicated functions
calls.
- Direct-compare, tags, commits compare will not display `New Pull
Request` button any more.

The new screenshot
<img width="1459" height="391" alt="image"
src="https://github.com/user-attachments/assets/64e9b070-9c0b-41d1-b4b8-233b96270e1b"
/>

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
This commit is contained in:
Lunny Xiao
2026-01-01 10:32:19 -08:00
committed by GitHub
parent 98981eb749
commit 8373f7deb3
10 changed files with 208 additions and 327 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -1062,19 +1062,11 @@ func MergePullRequest(ctx *context.APIContext) {
// parseCompareInfo returns non-nil if it succeeds, it always writes to the context and returns nil if it fails
func parseCompareInfo(ctx *context.APIContext, compareParam string) (result *git_service.CompareInfo, closer func()) {
baseRepo := ctx.Repo.Repository
compareReq, err := common.ParseCompareRouterParam(compareParam)
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
}
compareReq := common.ParseCompareRouterParam(compareParam)
// remove the check when we support compare with carets
if compareReq.CaretTimes > 0 {
ctx.APIError(http.StatusBadRequest, "Unsupported compare syntax with carets")
if compareReq.BaseOriRefSuffix != "" {
ctx.APIError(http.StatusBadRequest, "Unsupported comparison syntax: ref with suffix")
return nil, nil
}

72
routers/common/compare.go
View File

@@ -9,31 +9,28 @@ import (
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"
)
type CompareRouterReq struct {
BaseOriRef string
BaseOriRef string
BaseOriRefSuffix string
CompareSeparator 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
// FIXME: the design of "DirectComparison" is wrong, it loses the information of `^`
// To correctly handle the comparison, developers should use `ci.CompareSeparator` directly, all "DirectComparison" related code should be rewritten.
return cr.CompareSeparator == ".."
}
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) {
func parseHead(head string) (headOwnerName, headRepoName, headRef string) {
paths := strings.SplitN(head, ":", 2)
if len(paths) == 1 {
return "", "", paths[0]
@@ -48,6 +45,7 @@ func parseHead(head string) (string, string, string) {
// ParseCompareRouterParam Get compare information from the router parameter.
// A full compare url is of the form:
//
// 0. /{:baseOwner}/{:baseRepoName}/compare
// 1. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headBranch}
// 2. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}:{:headBranch}
// 3. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}/{:headRepoName}:{:headBranch}
@@ -70,45 +68,31 @@ func parseHead(head string) (string, string, string) {
// format: <base branch>...[<head repo>:]<head branch>
// base<-head: master...head:feature
// same repo: master...feature
func ParseCompareRouterParam(routerParam string) (*CompareRouterReq, error) {
func ParseCompareRouterParam(routerParam string) *CompareRouterReq {
if routerParam == "" {
return &CompareRouterReq{}, nil
return &CompareRouterReq{}
}
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 {
sep := "..."
basePart, headPart, ok := strings.Cut(routerParam, sep)
if !ok {
sep = ".."
basePart, headPart, ok = strings.Cut(routerParam, sep)
if !ok {
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)
HeadOriRef: headRef,
HeadOwner: headOwnerName,
HeadRepoName: headRepoName,
CompareSeparator: "...",
}
}
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
ci := &CompareRouterReq{CompareSeparator: sep}
ci.BaseOriRef, ci.BaseOriRefSuffix = git.ParseRefSuffix(basePart)
ci.HeadOwner, ci.HeadRepoName, ci.HeadOriRef = parseHead(headPart)
return ci
}
// maxForkTraverseLevel defines the maximum levels to traverse when searching for the head repository.

148
routers/common/compare_test.go
View File

@@ -6,146 +6,100 @@ 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
cases := []struct {
input string
CompareRouterReq *CompareRouterReq
}{
{
router: "",
input: "",
CompareRouterReq: &CompareRouterReq{},
},
{
input: "v1.0...v1.1",
CompareRouterReq: &CompareRouterReq{
BaseOriRef: "",
HeadOriRef: "",
DotTimes: 0,
BaseOriRef: "v1.0",
CompareSeparator: "...",
HeadOriRef: "v1.1",
},
},
{
router: "main...develop",
input: "main..develop",
CompareRouterReq: &CompareRouterReq{
BaseOriRef: "main",
HeadOriRef: "develop",
DotTimes: 3,
BaseOriRef: "main",
CompareSeparator: "..",
HeadOriRef: "develop",
},
},
{
router: "main..develop",
input: "main^...develop",
CompareRouterReq: &CompareRouterReq{
BaseOriRef: "main",
HeadOriRef: "develop",
DotTimes: 2,
BaseOriRef: "main",
BaseOriRefSuffix: "^",
CompareSeparator: "...",
HeadOriRef: "develop",
},
},
{
router: "main^...develop",
input: "main^^^^^...develop",
CompareRouterReq: &CompareRouterReq{
BaseOriRef: "main",
HeadOriRef: "develop",
CaretTimes: 1,
DotTimes: 3,
BaseOriRef: "main",
BaseOriRefSuffix: "^^^^^",
CompareSeparator: "...",
HeadOriRef: "develop",
},
},
{
router: "main^^^^^...develop",
input: "develop",
CompareRouterReq: &CompareRouterReq{
BaseOriRef: "main",
HeadOriRef: "develop",
CaretTimes: 5,
DotTimes: 3,
CompareSeparator: "...",
HeadOriRef: "develop",
},
},
{
router: "develop",
input: "teabot:feature1",
CompareRouterReq: &CompareRouterReq{
HeadOriRef: "develop",
DotTimes: 3,
CompareSeparator: "...",
HeadOwner: "teabot",
HeadOriRef: "feature1",
},
},
{
router: "lunny/forked_repo:develop",
input: "lunny/forked_repo:develop",
CompareRouterReq: &CompareRouterReq{
HeadOwner: "lunny",
HeadRepoName: "forked_repo",
HeadOriRef: "develop",
DotTimes: 3,
CompareSeparator: "...",
HeadOwner: "lunny",
HeadRepoName: "forked_repo",
HeadOriRef: "develop",
},
},
{
router: "main...lunny/forked_repo:develop",
input: "main...lunny/forked_repo:develop",
CompareRouterReq: &CompareRouterReq{
BaseOriRef: "main",
HeadOwner: "lunny",
HeadRepoName: "forked_repo",
HeadOriRef: "develop",
DotTimes: 3,
BaseOriRef: "main",
CompareSeparator: "...",
HeadOwner: "lunny",
HeadRepoName: "forked_repo",
HeadOriRef: "develop",
},
},
{
router: "main...lunny/forked_repo:develop",
input: "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,
BaseOriRef: "main",
BaseOriRefSuffix: "^",
CompareSeparator: "...",
HeadOwner: "lunny",
HeadRepoName: "forked_repo",
HeadOriRef: "develop",
},
},
}
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)
})
for _, c := range cases {
assert.Equal(t, c.CompareRouterReq, ParseCompareRouterParam(c.input), "input: %s", c.input)
}
}

209
routers/web/repo/compare.go
View File

@@ -196,21 +196,16 @@ func ParseCompareInfo(ctx *context.Context) *git_service.CompareInfo {
baseRepo := ctx.Repo.Repository
fileOnly := ctx.FormBool("file-only")
compareReq, err := common.ParseCompareRouterParam(ctx.PathParam("*"))
switch {
case errors.Is(err, util.ErrInvalidArgument):
ctx.HTTPError(http.StatusBadRequest, err.Error())
return nil
case err != nil:
ctx.ServerError("ParseCompareRouterParam", err)
return nil
}
// 1 Parse compare router param
compareReq := common.ParseCompareRouterParam(ctx.PathParam("*"))
// remove the check when we support compare with carets
if compareReq.CaretTimes > 0 {
ctx.HTTPError(http.StatusBadRequest, "Unsupported compare syntax with carets")
if compareReq.BaseOriRefSuffix != "" {
ctx.HTTPError(http.StatusBadRequest, "Unsupported comparison syntax: ref with suffix")
return nil
}
// 2 get repository and owner for head
headOwner, headRepo, err := common.GetHeadOwnerAndRepo(ctx, baseRepo, compareReq)
switch {
case errors.Is(err, util.ErrInvalidArgument):
@@ -224,45 +219,66 @@ func ParseCompareInfo(ctx *context.Context) *git_service.CompareInfo {
return nil
}
baseBranch := util.IfZero(compareReq.BaseOriRef, baseRepo.DefaultBranch)
headBranch := util.IfZero(compareReq.HeadOriRef, headRepo.DefaultBranch)
isSameRepo := baseRepo.ID == headRepo.ID
ctx.Data["BaseName"] = baseRepo.OwnerName
ctx.Data["BaseBranch"] = baseBranch
ctx.Data["HeadUser"] = headOwner
ctx.Data["HeadBranch"] = headBranch
ctx.Repo.PullRequest.SameRepo = isSameRepo
// 3 permission check
// base repository's code unit read permission check has been done on web.go
permBase := ctx.Repo.Permission
// Check if base branch is valid.
baseIsCommit := ctx.Repo.GitRepo.IsCommitExist(baseBranch)
baseIsBranch, _ := git_model.IsBranchExist(ctx, ctx.Repo.Repository.ID, baseBranch)
baseIsTag := gitrepo.IsTagExist(ctx, ctx.Repo.Repository, baseBranch)
if !baseIsCommit && !baseIsBranch && !baseIsTag {
// Check if baseBranch is short sha commit hash
if baseCommit, _ := ctx.Repo.GitRepo.GetCommit(baseBranch); baseCommit != nil {
baseBranch = baseCommit.ID.String()
ctx.Data["BaseBranch"] = baseBranch
baseIsCommit = true
} else if baseBranch == ctx.Repo.GetObjectFormat().EmptyObjectID().String() {
if isSameRepo {
ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(headBranch))
} else {
ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(headRepo.FullName()) + ":" + util.PathEscapeSegments(headBranch))
}
// If we're not merging from the same repo:
if !isSameRepo {
// Assert ctx.Doer has permission to read headRepo's codes
permHead, err := access_model.GetUserRepoPermission(ctx, headRepo, ctx.Doer)
if err != nil {
ctx.ServerError("GetUserRepoPermission", err)
return nil
} else {
}
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)
}
ctx.NotFound(nil)
return nil
}
ctx.Data["CanWriteToHeadRepo"] = permHead.CanWrite(unit.TypeCode)
}
ctx.Data["BaseIsCommit"] = baseIsCommit
ctx.Data["BaseIsBranch"] = baseIsBranch
ctx.Data["BaseIsTag"] = baseIsTag
ctx.Data["IsPull"] = true
// Now we have the repository that represents the base
// 4 get base and head refs
baseRefName := util.IfZero(compareReq.BaseOriRef, baseRepo.DefaultBranch)
headRefName := util.IfZero(compareReq.HeadOriRef, headRepo.DefaultBranch)
baseRef := ctx.Repo.GitRepo.UnstableGuessRefByShortName(baseRefName)
if baseRef == "" {
ctx.NotFound(nil)
return nil
}
var headGitRepo *git.Repository
if isSameRepo {
headGitRepo = ctx.Repo.GitRepo
} else {
headGitRepo, err = gitrepo.OpenRepository(ctx, headRepo)
if err != nil {
ctx.ServerError("OpenRepository", err)
return nil
}
defer headGitRepo.Close()
}
headRef := headGitRepo.UnstableGuessRefByShortName(headRefName)
if headRef == "" {
ctx.NotFound(nil)
return nil
}
ctx.Data["BaseName"] = baseRepo.OwnerName
ctx.Data["BaseBranch"] = baseRef.ShortName() // for legacy templates
ctx.Data["HeadUser"] = headOwner
ctx.Data["HeadBranch"] = headRef.ShortName() // for legacy templates
ctx.Repo.PullRequest.SameRepo = isSameRepo
ctx.Data["IsPull"] = true
// The current base and head repositories and branches may not
// actually be the intended branches that the user wants to
@@ -331,64 +347,9 @@ func ParseCompareInfo(ctx *context.Context) *git_service.CompareInfo {
ctx.Data["PageIsComparePull"] = false
}
// 8. Finally open the git repo
var headGitRepo *git.Repository
if isSameRepo {
headGitRepo = ctx.Repo.GitRepo
} else if has {
headGitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, headRepo)
if err != nil {
ctx.ServerError("RepositoryFromRequestContextOrOpen", err)
return nil
}
} else {
ctx.NotFound(nil)
return nil
}
ctx.Data["HeadRepo"] = 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(nil)
return nil
}
// If we're not merging from the same repo:
if !isSameRepo {
// Assert ctx.Doer has permission to read headRepo's codes
permHead, err := access_model.GetUserRepoPermission(ctx, headRepo, ctx.Doer)
if err != nil {
ctx.ServerError("GetUserRepoPermission", err)
return 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)
}
ctx.NotFound(nil)
return nil
}
ctx.Data["CanWriteToHeadRepo"] = permHead.CanWrite(unit.TypeCode)
}
// If we have a rootRepo and it's different from:
// 1. the computed base
// 2. the computed head
@@ -436,28 +397,9 @@ func ParseCompareInfo(ctx *context.Context) *git_service.CompareInfo {
}
}
// Check if head branch is valid.
headIsCommit := headGitRepo.IsCommitExist(headBranch)
headIsBranch, _ := git_model.IsBranchExist(ctx, headRepo.ID, headBranch)
headIsTag := gitrepo.IsTagExist(ctx, headRepo, headBranch)
if !headIsCommit && !headIsBranch && !headIsTag {
// Check if headBranch is short sha commit hash
if headCommit, _ := headGitRepo.GetCommit(headBranch); headCommit != nil {
headBranch = headCommit.ID.String()
ctx.Data["HeadBranch"] = headBranch
headIsCommit = true
} else {
ctx.NotFound(nil)
return nil
}
}
ctx.Data["HeadIsCommit"] = headIsCommit
ctx.Data["HeadIsBranch"] = headIsBranch
ctx.Data["HeadIsTag"] = headIsTag
// Treat as pull request if both references are branches
if ctx.Data["PageIsComparePull"] == nil {
ctx.Data["PageIsComparePull"] = headIsBranch && baseIsBranch && permBase.CanReadIssuesOrPulls(true)
ctx.Data["PageIsComparePull"] = baseRef.IsBranch() && headRef.IsBranch() && permBase.CanReadIssuesOrPulls(true)
}
if ctx.Data["PageIsComparePull"] == true && !permBase.CanReadIssuesOrPulls(true) {
@@ -471,20 +413,7 @@ func ParseCompareInfo(ctx *context.Context) *git_service.CompareInfo {
return nil
}
baseBranchRef := git.RefName(baseBranch)
if baseIsBranch {
baseBranchRef = git.RefNameFromBranch(baseBranch)
} else if baseIsTag {
baseBranchRef = git.RefNameFromTag(baseBranch)
}
headBranchRef := git.RefName(headBranch)
if headIsBranch {
headBranchRef = git.RefNameFromBranch(headBranch)
} else if headIsTag {
headBranchRef = git.RefNameFromTag(headBranch)
}
compareInfo, err := git_service.GetCompareInfo(ctx, baseRepo, headRepo, headGitRepo, baseBranchRef, headBranchRef, compareReq.DirectComparison(), fileOnly)
compareInfo, err := git_service.GetCompareInfo(ctx, baseRepo, headRepo, headGitRepo, baseRef, headRef, compareReq.DirectComparison(), fileOnly)
if err != nil {
ctx.ServerError("GetCompareInfo", err)
return nil
@@ -517,7 +446,7 @@ func PrepareCompareDiff(
ctx.Data["TitleQuery"] = newPrFormTitle
ctx.Data["BodyQuery"] = newPrFormBody
if (headCommitID == ci.MergeBase && !ci.DirectComparison) ||
if (headCommitID == ci.MergeBase && !ci.DirectComparison()) ||
headCommitID == ci.BaseCommitID {
ctx.Data["IsNothingToCompare"] = true
if unit, err := repo.GetUnit(ctx, unit.TypePullRequests); err == nil {
@@ -534,7 +463,7 @@ func PrepareCompareDiff(
}
beforeCommitID := ci.MergeBase
if ci.DirectComparison {
if ci.DirectComparison() {
beforeCommitID = ci.BaseCommitID
}
@@ -555,7 +484,7 @@ func PrepareCompareDiff(
MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters,
MaxFiles: maxFiles,
WhitespaceBehavior: whitespaceBehavior,
DirectComparison: ci.DirectComparison,
DirectComparison: ci.DirectComparison(),
}, ctx.FormStrings("files")...)
if err != nil {
ctx.ServerError("GetDiff", err)
@@ -668,13 +597,7 @@ func CompareDiff(ctx *context.Context) {
ctx.Data["PageIsViewCode"] = true
ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
ctx.Data["DirectComparison"] = ci.DirectComparison
ctx.Data["OtherCompareSeparator"] = ".."
ctx.Data["CompareSeparator"] = "..."
if ci.DirectComparison {
ctx.Data["CompareSeparator"] = ".."
ctx.Data["OtherCompareSeparator"] = "..."
}
ctx.Data["CompareInfo"] = ci
nothingToCompare := PrepareCompareDiff(ctx, ci, gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)))
if ctx.Written() {
@@ -751,11 +674,7 @@ func CompareDiff(ctx *context.Context) {
beforeCommitID := ctx.Data["BeforeCommitID"].(string)
afterCommitID := ctx.Data["AfterCommitID"].(string)
separator := "..."
if ci.DirectComparison {
separator = ".."
}
ctx.Data["Title"] = "Comparing " + base.ShortSha(beforeCommitID) + separator + base.ShortSha(afterCommitID)
ctx.Data["Title"] = "Comparing " + base.ShortSha(beforeCommitID) + ci.CompareSeparator + base.ShortSha(afterCommitID)
ctx.Data["IsDiffCompare"] = true