Skip to content
Open
15 changes: 9 additions & 6 deletions modules/git/ref.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,13 +228,16 @@ func (ref RefName) RefWebLinkPath() string {
return string(refType) + "/" + util.PathEscapeSegments(ref.ShortName())
}

func ParseRefSuffix(ref string) (string, string) {
func ParseRefSuffix(ref string) (refName, refSuffix string) {
// Partially support https://git-scm.com/docs/gitrevisions
if idx := strings.Index(ref, "@{"); idx != -1 {
return ref[:idx], ref[idx:]
suffixIdx := -1 // earliest suffix mark, so a combined suffix like "main~2^" stays intact
for _, mark := range []string{"@{", "^", "~"} {
if idx := strings.Index(ref, mark); idx != -1 && (suffixIdx == -1 || idx < suffixIdx) {
suffixIdx = idx
}
}
if idx := strings.Index(ref, "^"); idx != -1 {
return ref[:idx], ref[idx:]
if suffixIdx == -1 {
return ref, ""
}
return ref, ""
return ref[:suffixIdx], ref[suffixIdx:]
}
19 changes: 19 additions & 0 deletions modules/git/ref_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,22 @@ func TestRefWebLinkPath(t *testing.T) {
assert.Equal(t, "tag/foo", RefName("refs/tags/foo").RefWebLinkPath())
assert.Equal(t, "commit/c0ffee", RefName("c0ffee").RefWebLinkPath())
}

func TestParseRefSuffix(t *testing.T) {
cases := []struct {
ref, name, suffix string
}{
{"main", "main", ""},
{"main^", "main", "^"},
{"main^2", "main", "^2"},
{"main~3", "main", "~3"},
{"main@{yesterday}", "main", "@{yesterday}"},
{"main~2^", "main", "~2^"},
{"main^~2", "main", "^~2"},
}
for _, c := range cases {
name, suffix := ParseRefSuffix(c.ref)
assert.Equal(t, c.name, name, "ref: %s", c.ref)
assert.Equal(t, c.suffix, suffix, "ref: %s", c.ref)
}
}
4 changes: 3 additions & 1 deletion routers/api/v1/repo/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func CompareDiff(ctx *context.APIContext) {
// required: true
// - name: basehead
// in: path
// description: compare two refs as `base...head` (or `base..head`); refs may be branches, tags, full or short SHAs, including branch names that contain slashes.
// description: compare two refs as `base...head` (or `base..head`); refs may be branches, tags, full or short SHAs (including branch names that contain slashes), optionally with a `^` or `~N` revision suffix.
// type: string
// required: true
// - name: output
Expand All @@ -51,6 +51,8 @@ func CompareDiff(ctx *context.APIContext) {
// responses:
// "200":
// "$ref": "#/responses/Compare"
// "400":
// "$ref": "#/responses/error"
// "404":
// "$ref": "#/responses/notFound"

Expand Down
20 changes: 11 additions & 9 deletions routers/api/v1/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -1087,12 +1087,6 @@ func parseCompareInfo(ctx *context.APIContext, compareParam string) (result *git
baseRepo := ctx.Repo.Repository
compareReq := common.ParseCompareRouterParam(compareParam)

// remove the check when we support compare with carets
if compareReq.BaseOriRefSuffix != "" {
ctx.APIError(http.StatusBadRequest, "Unsupported comparison syntax: ref with suffix")
return nil, nil
}

_, headRepo, err := common.GetHeadOwnerAndRepo(ctx, baseRepo, compareReq)
switch {
case errors.Is(err, util.ErrInvalidArgument):
Expand Down Expand Up @@ -1152,10 +1146,18 @@ func parseCompareInfo(ctx *context.APIContext, compareParam string) (result *git
return nil, nil
}

baseRef := ctx.Repo.GitRepo.UnstableGuessRefByShortName(util.IfZero(compareReq.BaseOriRef, baseRepo.GetPullRequestTargetBranch(ctx)))
headRef := headGitRepo.UnstableGuessRefByShortName(util.IfZero(compareReq.HeadOriRef, headRepo.DefaultBranch))
baseRef, err := common.ResolveRefWithSuffix(ctx.Repo.GitRepo, util.IfZero(compareReq.BaseOriRef, baseRepo.GetPullRequestTargetBranch(ctx)), compareReq.BaseOriRefSuffix)
if err != nil {
ctx.APIErrorAuto(err)
return nil, nil
}
headRef, err := common.ResolveRefWithSuffix(headGitRepo, util.IfZero(compareReq.HeadOriRef, headRepo.DefaultBranch), compareReq.HeadOriRefSuffix)
if err != nil {
ctx.APIErrorAuto(err)
return nil, nil
}

log.Trace("Repo path: %q, base ref: %q->%q, head ref: %q->%q", ctx.Repo.Repository.RelativePath(), compareReq.BaseOriRef, baseRef, compareReq.HeadOriRef, headRef)
log.Trace("Repo path: %q, base ref: %q->%q, head ref: %q->%q", ctx.Repo.Repository.RelativePath(), compareReq.BaseOriRef+compareReq.BaseOriRefSuffix, baseRef, compareReq.HeadOriRef+compareReq.HeadOriRefSuffix, 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 Down
42 changes: 37 additions & 5 deletions routers/common/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ package common

import (
"context"
"regexp"
"strings"
"sync"

repo_model "gitea.dev/models/repo"
user_model "gitea.dev/models/user"
Expand All @@ -19,9 +21,10 @@ type CompareRouterReq struct {

CompareSeparator string

HeadOwner string
HeadRepoName string
HeadOriRef string
HeadOwner string
HeadRepoName string
HeadOriRef string
HeadOriRefSuffix string
}

func (cr *CompareRouterReq) DirectComparison() bool {
Expand Down Expand Up @@ -79,9 +82,11 @@ func ParseCompareRouterParam(routerParam string) *CompareRouterReq {
sep = ".."
basePart, headPart, ok = strings.Cut(routerParam, sep)
if !ok {
headOwnerName, headRepoName, headRef := parseHead(routerParam)
headOwnerName, headRepoName, headOriRef := parseHead(routerParam)
headOriRef, headOriRefSuffix := git.ParseRefSuffix(headOriRef)
return &CompareRouterReq{
HeadOriRef: headRef,
HeadOriRef: headOriRef,
HeadOriRefSuffix: headOriRefSuffix,
HeadOwner: headOwnerName,
HeadRepoName: headRepoName,
CompareSeparator: "...",
Expand All @@ -92,9 +97,36 @@ func ParseCompareRouterParam(routerParam string) *CompareRouterReq {
ci := &CompareRouterReq{CompareSeparator: sep}
ci.BaseOriRef, ci.BaseOriRefSuffix = git.ParseRefSuffix(basePart)
ci.HeadOwner, ci.HeadRepoName, ci.HeadOriRef = parseHead(headPart)
ci.HeadOriRef, ci.HeadOriRefSuffix = git.ParseRefSuffix(ci.HeadOriRef)
return ci
}

// validRefSuffix matches only ^/~ ancestry navigation. The ^{...}, @{...} and :path forms address
// other objects (trees, blobs) or reflog/upstream state that compare does not resolve, so they are rejected.
var validRefSuffix = sync.OnceValue(func() *regexp.Regexp {
return regexp.MustCompile(`^(?:[~^][0-9]*)+$`)
})

// ResolveRefWithSuffix resolves oriRef plus an optional revision suffix (^, ~N) to a RefName.
// A nil error guarantees a usable RefName: an unsupported suffix yields an invalid-argument error
// and an unresolvable ref yields a not-found error.
func ResolveRefWithSuffix(gitRepo *git.Repository, oriRef, refSuffix string) (git.RefName, error) {
if refSuffix == "" {
if refName := gitRepo.UnstableGuessRefByShortName(oriRef); refName != "" {
return refName, nil
}
return "", util.NewNotExistErrorf("ref %q does not exist", oriRef)
}
if !validRefSuffix().MatchString(refSuffix) {
return "", util.NewInvalidArgumentErrorf("unsupported ref suffix %q", refSuffix)
}
commit, err := gitRepo.GetCommit(oriRef + refSuffix)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Resolve suffixes after disambiguating refs

When a suffix is present this bypasses UnstableGuessRefByShortName and asks git to resolve the raw short name plus suffix. In repositories where a branch and tag share the same name, the non-suffixed compare path resolves the branch first, but compare/v1^...main feeds v1^ to git and git reports the ambiguous ref as missing, so users cannot compare the branch parent. Resolve oriRef to the branch/tag/SHA first and append the suffix to that full ref to preserve the existing branch-first semantics.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

There is a related edge case worth mentioning. When a branch and tag share a name, the suffix path uses the default git order instead of checking branches first (UnstableGuessRefByShortName), so name^ can pick the tag. I can fix it here, or open a follow-up if you want to keep this PR focused.

if err != nil {
return "", util.NewNotExistErrorf("ref %q does not exist", oriRef+refSuffix)
}
return git.RefNameFromCommit(commit.ID.String()), nil
}

// maxForkTraverseLevel defines the maximum levels to traverse when searching for the head repository.
const maxForkTraverseLevel = 10

Expand Down
49 changes: 49 additions & 0 deletions routers/common/compare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ package common
import (
"testing"

"gitea.dev/modules/util"

"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -97,9 +99,56 @@ func TestCompareRouterReq(t *testing.T) {
HeadOriRef: "develop",
},
},
{
input: "main...develop^",
CompareRouterReq: &CompareRouterReq{
BaseOriRef: "main",
CompareSeparator: "...",
HeadOriRef: "develop",
HeadOriRefSuffix: "^",
},
},
{
input: "main~2...develop",
CompareRouterReq: &CompareRouterReq{
BaseOriRef: "main",
BaseOriRefSuffix: "~2",
CompareSeparator: "...",
HeadOriRef: "develop",
},
},
{
input: "main...lunny/forked_repo:develop~3",
CompareRouterReq: &CompareRouterReq{
BaseOriRef: "main",
CompareSeparator: "...",
HeadOwner: "lunny",
HeadRepoName: "forked_repo",
HeadOriRef: "develop",
HeadOriRefSuffix: "~3",
},
},
{
input: "develop^",
CompareRouterReq: &CompareRouterReq{
CompareSeparator: "...",
HeadOriRef: "develop",
HeadOriRefSuffix: "^",
},
},
}

for _, c := range cases {
assert.Equal(t, c.CompareRouterReq, ParseCompareRouterParam(c.input), "input: %s", c.input)
}
}

func TestResolveRefWithSuffix(t *testing.T) {
// The ^{...}, @{...} and :path forms address non-commit objects or reflog state, so they are
// rejected before any repository access and a nil repo is fine here.
for _, refSuffix := range []string{"^{/Add}", "^{commit}", "@{upstream}", "~1:path"} {
ref, err := ResolveRefWithSuffix(nil, "branch", refSuffix)
assert.ErrorIs(t, err, util.ErrInvalidArgument, "suffix %q", refSuffix)
assert.Empty(t, ref, "suffix %q", refSuffix)
}
}
17 changes: 6 additions & 11 deletions routers/web/repo/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,11 +208,6 @@ func (cpi *comparePageInfoType) parseCompareInfo(ctx *context.Context, comparePa
// 1 Parse compare router param
compareReq := common.ParseCompareRouterParam(compareParam)

// remove the check when we support compare with carets
if compareReq.BaseOriRefSuffix != "" {
return util.NewInvalidArgumentErrorf("unsupported comparison syntax: ref with suffix")
}

// 2 get repository and owner for head
headOwner, headRepo, err := common.GetHeadOwnerAndRepo(ctx, baseRepo, compareReq)
if err != nil {
Expand Down Expand Up @@ -241,18 +236,18 @@ func (cpi *comparePageInfoType) parseCompareInfo(ctx *context.Context, comparePa
baseRefName := util.IfZero(compareReq.BaseOriRef, baseRepo.GetPullRequestTargetBranch(ctx))
headRefName := util.IfZero(compareReq.HeadOriRef, headRepo.DefaultBranch)

baseRef := ctx.Repo.GitRepo.UnstableGuessRefByShortName(baseRefName)
if baseRef == "" {
return util.NewNotExistErrorf("no base ref: %s", baseRefName)
baseRef, err := common.ResolveRefWithSuffix(ctx.Repo.GitRepo, baseRefName, compareReq.BaseOriRefSuffix)
if err != nil {
return err
}
headGitRepo, err := gitrepo.RepositoryFromRequestContextOrOpen(ctx, headRepo)
if err != nil {
return err
}

headRef := headGitRepo.UnstableGuessRefByShortName(headRefName)
if headRef == "" {
return util.NewNotExistErrorf("no head ref: %s", headRefName)
headRef, err := common.ResolveRefWithSuffix(headGitRepo, headRefName, compareReq.HeadOriRefSuffix)
if err != nil {
return err
}

ctx.Data["BaseName"] = baseRepo.OwnerName
Expand Down
5 changes: 4 additions & 1 deletion templates/swagger/v1_json.tmpl

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion templates/swagger/v1_openapi3_json.tmpl

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 34 additions & 0 deletions tests/integration/api_repo_compare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,40 @@ func TestAPICompareBranches(t *testing.T) {
assert.Len(t, apiResp.Commits, 1)
})

t.Run("CompareWithRefSuffix", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()

// remove-files-b^ is the parent of the tip, so the range drops the tip and ends at that parent
const parentSHA = "b67e43a07d48243a5f670ace063acd5e13f719df"
req := NewRequestf(t, "GET", "/api/v1/repos/user2/repo20/compare/add-csv...remove-files-b^").AddTokenAuth(token2)
resp := MakeRequest(t, req, http.StatusOK)
apiResp := DecodeJSON(t, resp, &api.Compare{})
assert.Equal(t, 1, apiResp.TotalCommits)
assert.Len(t, apiResp.Commits, 1)
assert.Equal(t, parentSHA, apiResp.Commits[0].SHA)

// the same suffix on the direct ".." comparison resolves to the same commit
req = NewRequestf(t, "GET", "/api/v1/repos/user2/repo20/compare/add-csv..remove-files-b^").AddTokenAuth(token2)
resp = MakeRequest(t, req, http.StatusOK)
apiResp = DecodeJSON(t, resp, &api.Compare{})
assert.Equal(t, 1, apiResp.TotalCommits)
assert.Equal(t, parentSHA, apiResp.Commits[0].SHA)

req = NewRequestf(t, "GET", "/api/v1/repos/user2/repo20/compare/add-csv~1...add-csv").AddTokenAuth(token2)
resp = MakeRequest(t, req, http.StatusOK)
apiResp = DecodeJSON(t, resp, &api.Compare{})
assert.Equal(t, 1, apiResp.TotalCommits)
assert.Len(t, apiResp.Commits, 1)

// a valid but unresolvable suffix is not found, while an unsupported suffix (^{...}) is a bad request
req = NewRequestf(t, "GET", "/api/v1/repos/user2/repo20/compare/add-csv...remove-files-b~50").AddTokenAuth(token2)
MakeRequest(t, req, http.StatusNotFound)
req = NewRequestf(t, "GET", "/api/v1/repos/user2/repo20/compare/add-csv...remove-files-b^{/Add}").AddTokenAuth(token2)
MakeRequest(t, req, http.StatusBadRequest)
req = NewRequestf(t, "GET", "/api/v1/repos/user2/repo20/compare/add-csv^{/Add}...remove-files-b").AddTokenAuth(token2)
MakeRequest(t, req, http.StatusBadRequest)
})

t.Run("CompareForkOnlyCommit", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()

Expand Down
Loading
Loading