diff --git a/modules/git/ref.go b/modules/git/ref.go index 7d0bbcbae9bd2..ba69cf87ff396 100644 --- a/modules/git/ref.go +++ b/modules/git/ref.go @@ -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:] } diff --git a/modules/git/ref_test.go b/modules/git/ref_test.go index 5397191561290..5016eea9d5728 100644 --- a/modules/git/ref_test.go +++ b/modules/git/ref_test.go @@ -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) + } +} diff --git a/routers/api/v1/repo/compare.go b/routers/api/v1/repo/compare.go index ddd5a9bb21a8f..833e8af9ff52c 100644 --- a/routers/api/v1/repo/compare.go +++ b/routers/api/v1/repo/compare.go @@ -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 @@ -51,6 +51,8 @@ func CompareDiff(ctx *context.APIContext) { // responses: // "200": // "$ref": "#/responses/Compare" + // "400": + // "$ref": "#/responses/error" // "404": // "$ref": "#/responses/notFound" diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index d5c912d243011..2f2edd101b875 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -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): @@ -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()) diff --git a/routers/common/compare.go b/routers/common/compare.go index 667a32a9657e0..83990e4eb5407 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -5,7 +5,9 @@ package common import ( "context" + "regexp" "strings" + "sync" repo_model "gitea.dev/models/repo" user_model "gitea.dev/models/user" @@ -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 { @@ -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: "...", @@ -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) + 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 diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go index e4e24a03cfa09..fc47397b1ebaa 100644 --- a/routers/common/compare_test.go +++ b/routers/common/compare_test.go @@ -6,6 +6,8 @@ package common import ( "testing" + "gitea.dev/modules/util" + "github.com/stretchr/testify/assert" ) @@ -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) + } +} diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 5a9fa1c1d1301..adb51ab69c95c 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -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 { @@ -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 diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 37a358a26b675..c10c8af6d9297 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -8135,7 +8135,7 @@ }, { "type": "string", - "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.", "name": "basehead", "in": "path", "required": true @@ -8155,6 +8155,9 @@ "200": { "$ref": "#/responses/Compare" }, + "400": { + "$ref": "#/responses/error" + }, "404": { "$ref": "#/responses/notFound" } diff --git a/templates/swagger/v1_openapi3_json.tmpl b/templates/swagger/v1_openapi3_json.tmpl index 78295445268c7..6b851bea5f92e 100644 --- a/templates/swagger/v1_openapi3_json.tmpl +++ b/templates/swagger/v1_openapi3_json.tmpl @@ -19490,7 +19490,7 @@ } }, { - "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.", "in": "path", "name": "basehead", "required": true, @@ -19515,6 +19515,9 @@ "200": { "$ref": "#/components/responses/Compare" }, + "400": { + "$ref": "#/components/responses/error" + }, "404": { "$ref": "#/components/responses/notFound" } diff --git a/tests/integration/api_repo_compare_test.go b/tests/integration/api_repo_compare_test.go index 5bb8cf6bde426..d750e72062560 100644 --- a/tests/integration/api_repo_compare_test.go +++ b/tests/integration/api_repo_compare_test.go @@ -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)() diff --git a/tests/integration/compare_test.go b/tests/integration/compare_test.go index d5f5bc2cc2749..ff2928c4aa058 100644 --- a/tests/integration/compare_test.go +++ b/tests/integration/compare_test.go @@ -14,7 +14,10 @@ import ( "gitea.dev/models/unittest" user_model "gitea.dev/models/user" "gitea.dev/modules/git/gitcmd" + "gitea.dev/modules/gitrepo" "gitea.dev/modules/test" + "gitea.dev/modules/util" + "gitea.dev/routers/common" repo_service "gitea.dev/services/repository" "gitea.dev/tests" @@ -135,6 +138,73 @@ func TestCompareBranches(t *testing.T) { inspectCompare(t, htmlDoc, diffCount, diffChanges) } +func TestCompareWithRefSuffix(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + session := loginUser(t, "user2") + + // remove-files-b^ resolves to the tip's parent, so the test.txt added by the tip is excluded + req := NewRequest(t, "GET", "/user2/repo20/compare/add-csv...remove-files-b^") + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + inspectCompare(t, htmlDoc, 2, []string{"link_hi", "test.csv"}) + + // a suffix resolves to a commit rather than a branch, so the page offers no pull request to create + assert.Equal(t, 0, htmlDoc.doc.Find(".pullrequest-form").Length()) + + // the same suffix on the direct ".." comparison resolves to the same commit + req = NewRequest(t, "GET", "/user2/repo20/compare/add-csv..remove-files-b^") + resp = session.MakeRequest(t, req, http.StatusOK) + htmlDoc = NewHTMLParser(t, resp.Body) + inspectCompare(t, htmlDoc, 2, []string{"link_hi", "test.csv"}) + + // a ~N suffix on the base side resolves and renders the compare page, but also + // resolves to a commit rather than a branch, so no pull request form is offered + req = NewRequest(t, "GET", "/user2/repo20/compare/add-csv~1...remove-files-b") + resp = session.MakeRequest(t, req, http.StatusOK) + assert.True(t, test.IsNormalPageCompleted(resp.Body.String())) + htmlDoc = NewHTMLParser(t, resp.Body) + assert.Equal(t, 0, htmlDoc.doc.Find(".pullrequest-form").Length()) + + // the web handler folds an unsupported (^{...}) and an unresolvable (~50) suffix alike into 404 + for _, basehead := range []string{ + "add-csv...remove-files-b~50", + "add-csv...remove-files-b^{/Add}", + "add-csv^{/Add}...remove-files-b", + } { + req = NewRequest(t, "GET", "/user2/repo20/compare/"+basehead).SetHeader("Accept", "text/html") + resp = session.MakeRequest(t, req, http.StatusNotFound) + assert.True(t, test.IsNormalPageCompleted(resp.Body.String())) + } +} + +func TestResolveRefWithSuffixContract(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 31}) + gitRepo, err := gitrepo.OpenRepository(t.Context(), repo) + require.NoError(t, err) + defer gitRepo.Close() + + // a nil error guarantees a usable RefName + ref, err := common.ResolveRefWithSuffix(gitRepo, "add-csv", "^") + require.NoError(t, err) + assert.NotEmpty(t, ref) + // a ref resolved with a suffix must be a commit SHA, not a branch ref + // (branch refs would break "New Pull Request" logic) + assert.False(t, ref.IsBranch(), "ref with suffix must not resolve to a branch") + + // a missing ref and an unresolvable suffix both report not-found instead of an empty RefName + for _, tc := range []struct{ oriRef, suffix string }{ + {"does-not-exist", ""}, + {"add-csv", "~50"}, + } { + ref, err := common.ResolveRefWithSuffix(gitRepo, tc.oriRef, tc.suffix) + assert.ErrorIs(t, err, util.ErrNotExist, "ref %q suffix %q", tc.oriRef, tc.suffix) + assert.Empty(t, ref, "ref %q suffix %q", tc.oriRef, tc.suffix) + } +} + func TestCompareBranchesNoCommonMergeBase(t *testing.T) { defer tests.PrepareTestEnv(t)()