improve the compare page

modules/git/ref.go

@@ -220,3 +220,14 @@ func (ref RefName) RefWebLinkPath() string {
 	}
 	return string(refType) + "/" + util.PathEscapeSegments(ref.ShortName())
 }
+
+func ParseRefSuffix(ref string) (string, string) {
+	// Partially support https://git-scm.com/docs/gitrevisions
+	if idx := strings.Index(ref, "@{"); idx != -1 {
+		return ref[:idx], ref[idx:]
+	}
+	if idx := strings.Index(ref, "^"); idx != -1 {
+		return ref[:idx], ref[idx:]
+	}
+	return ref, ""
+}

options/locale/locale_en-US.json

@@ -1736,8 +1736,11 @@
   "repo.issues.reference_link": "Reference: %s",
   "repo.compare.compare_base": "base",
   "repo.compare.compare_head": "compare",
+  "repo.compare.title": "Comparing changes",
+  "repo.compare.description": "Choose two branches or tags to see what’s changed or to start a new pull request.",
   "repo.pulls.desc": "Enable pull requests and code reviews.",
   "repo.pulls.new": "New Pull Request",
+  "repo.pulls.new.description": "Discuss and review the changes in this comparison with others.",
   "repo.pulls.new.blocked_user": "Cannot create pull request because you are blocked by the repository owner.",
   "repo.pulls.new.must_collaborator": "You must be a collaborator to create pull request.",
   "repo.pulls.new.already_existed": "A pull request between these branches already exists",
@@ -1747,7 +1750,6 @@
   "repo.pulls.allow_edits_from_maintainers": "Allow edits from maintainers",
   "repo.pulls.allow_edits_from_maintainers_desc": "Users with write access to the base branch can also push to this branch",
   "repo.pulls.allow_edits_from_maintainers_err": "Updating failed",
-  "repo.pulls.compare_changes_desc": "Select the branch to merge into and the branch to pull from.",
   "repo.pulls.has_viewed_file": "Viewed",
   "repo.pulls.has_changed_since_last_review": "Changed since your last review",
   "repo.pulls.viewed_files_label": "%[1]d / %[2]d files viewed",

routers/api/v1/repo/pull.go

@@ -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
 	}
 

routers/common/compare.go

@@ -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
-}
-
-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
+	// 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 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.

routers/common/compare_test.go

@@ -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: "",
-			CompareRouterReq: &CompareRouterReq{
-				BaseOriRef: "",
-				HeadOriRef: "",
-				DotTimes:   0,
-			},
-		},
-		{
-			router: "main...develop",
-			CompareRouterReq: &CompareRouterReq{
-				BaseOriRef: "main",
-				HeadOriRef: "develop",
-				DotTimes:   3,
-			},
-		},
-		{
-			router: "main..develop",
-			CompareRouterReq: &CompareRouterReq{
-				BaseOriRef: "main",
-				HeadOriRef: "develop",
-				DotTimes:   2,
-			},
-		},
-		{
-			router: "main^...develop",
-			CompareRouterReq: &CompareRouterReq{
-				BaseOriRef: "main",
-				HeadOriRef: "develop",
-				CaretTimes: 1,
-				DotTimes:   3,
-			},
-		},
-		{
-			router: "main^^^^^...develop",
-			CompareRouterReq: &CompareRouterReq{
-				BaseOriRef: "main",
-				HeadOriRef: "develop",
-				CaretTimes: 5,
-				DotTimes:   3,
-			},
+			input:            "",
+			CompareRouterReq: &CompareRouterReq{},
 		},
 		{
-			router: "develop",
+			input: "v1.0...v1.1",
 			CompareRouterReq: &CompareRouterReq{
-				HeadOriRef: "develop",
-				DotTimes:   3,
+				BaseOriRef:       "v1.0",
+				CompareSeparator: "...",
+				HeadOriRef:       "v1.1",
 			},
 		},
 		{
-			router: "lunny/forked_repo:develop",
+			input: "main..develop",
 			CompareRouterReq: &CompareRouterReq{
-				HeadOwner:    "lunny",
-				HeadRepoName: "forked_repo",
-				HeadOriRef:   "develop",
-				DotTimes:     3,
+				BaseOriRef:       "main",
+				CompareSeparator: "..",
+				HeadOriRef:       "develop",
 			},
 		},
 		{
-			router: "main...lunny/forked_repo:develop",
+			input: "main^...develop",
 			CompareRouterReq: &CompareRouterReq{
-				BaseOriRef:   "main",
-				HeadOwner:    "lunny",
-				HeadRepoName: "forked_repo",
-				HeadOriRef:   "develop",
-				DotTimes:     3,
+				BaseOriRef:       "main",
+				BaseOriRefSuffix: "^",
+				CompareSeparator: "...",
+				HeadOriRef:       "develop",
 			},
 		},
 		{
-			router: "main...lunny/forked_repo:develop",
+			input: "main^^^^^...develop",
 			CompareRouterReq: &CompareRouterReq{
-				BaseOriRef:   "main",
-				HeadOwner:    "lunny",
-				HeadRepoName: "forked_repo",
-				HeadOriRef:   "develop",
-				DotTimes:     3,
+				BaseOriRef:       "main",
+				BaseOriRefSuffix: "^^^^^",
+				CompareSeparator: "...",
+				HeadOriRef:       "develop",
 			},
 		},
 		{
-			router: "main^...lunny/forked_repo:develop",
+			input: "develop",
 			CompareRouterReq: &CompareRouterReq{
-				BaseOriRef:   "main",
-				HeadOwner:    "lunny",
-				HeadRepoName: "forked_repo",
-				HeadOriRef:   "develop",
-				DotTimes:     3,
-				CaretTimes:   1,
+				CompareSeparator: "...",
+				HeadOriRef:       "develop",
 			},
 		},
 		{
-			router: "v1.0...v1.1",
+			input: "teabot:feature1",
 			CompareRouterReq: &CompareRouterReq{
-				BaseOriRef: "v1.0",
-				HeadOriRef: "v1.1",
-				DotTimes:   3,
+				CompareSeparator: "...",
+				HeadOwner:        "teabot",
+				HeadOriRef:       "feature1",
 			},
 		},
 		{
-			router: "teabot-patch-1...v0.0.1",
+			input: "lunny/forked_repo:develop",
 			CompareRouterReq: &CompareRouterReq{
-				BaseOriRef: "teabot-patch-1",
-				HeadOriRef: "v0.0.1",
-				DotTimes:   3,
+				CompareSeparator: "...",
+				HeadOwner:        "lunny",
+				HeadRepoName:     "forked_repo",
+				HeadOriRef:       "develop",
 			},
 		},
 		{
-			router: "teabot:feature1",
+			input: "main...lunny/forked_repo:develop",
 			CompareRouterReq: &CompareRouterReq{
-				HeadOwner:  "teabot",
-				HeadOriRef: "feature1",
-				DotTimes:   3,
+				BaseOriRef:       "main",
+				CompareSeparator: "...",
+				HeadOwner:        "lunny",
+				HeadRepoName:     "forked_repo",
+				HeadOriRef:       "develop",
 			},
 		},
 		{
-			router: "8eb19a5ae19abae15c0666d4ab98906139a7f439...283c030497b455ecfa759d4649f9f8b45158742e",
+			input: "main^...lunny/forked_repo:develop",
 			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)
 	}
 }