improve the compare page

modules/git/ref.go

@@ -208,6 +208,10 @@ func (ref RefName) RefType() RefType {
 	return ""
 }
 
+func (ref RefName) IsCommit() bool {
+	return ref.RefType() == RefTypeCommit
+}
+
 // RefWebLinkPath returns a path for the reference that can be used in a web link:
 // * "branch/<branch_name>"
 // * "tag/<tag_name>"

options/locale/locale_en-US.json

@@ -1736,18 +1736,19 @@
   "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",
   "repo.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.",
   "repo.pulls.view": "View Pull Request",
-  "repo.pulls.compare_changes": "New Pull Request",
   "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/web/repo/compare.go

@@ -196,6 +196,7 @@ func ParseCompareInfo(ctx *context.Context) *git_service.CompareInfo {
 	baseRepo := ctx.Repo.Repository
 	fileOnly := ctx.FormBool("file-only")
 
+	// 1 Parse compare router param
 	compareReq, err := common.ParseCompareRouterParam(ctx.PathParam("*"))
 	switch {
 	case errors.Is(err, util.ErrInvalidArgument):
@@ -211,6 +212,7 @@ func ParseCompareInfo(ctx *context.Context) *git_service.CompareInfo {
 		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 +226,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 +354,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 +404,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 +420,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
@@ -668,13 +604,8 @@ 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
+	ctx.Data["CompareSeparator"] = ci.Separator() // To keep for template compatibility, otherwise use CompareInfo.Separator directly
 
 	nothingToCompare := PrepareCompareDiff(ctx, ci, gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)))
 	if ctx.Written() {

services/git/compare.go

@@ -39,6 +39,20 @@ func (ci *CompareInfo) IsSameRef() bool {
 	return ci.IsSameRepository() && ci.BaseRef == ci.HeadRef
 }
 
+func (ci *CompareInfo) Separator() string {
+	if ci.DirectComparison {
+		return ".."
+	}
+	return "..."
+}
+
+func (ci *CompareInfo) OtherSeparator() string {
+	if ci.DirectComparison {
+		return "..."
+	}
+	return ".."
+}
+
 // GetCompareInfo generates and returns compare information between base and head branches of repositories.
 func GetCompareInfo(ctx context.Context, baseRepo, headRepo *repo_model.Repository, headGitRepo *git.Repository, baseRef, headRef git.RefName, directComparison, fileOnly bool) (_ *CompareInfo, err error) {
 	var (

templates/repo/commits_ref_name.tmpl

@@ -0,0 +1,10 @@
+{{- if .IsCommit -}}
+	{{ShortSha .ShortName}}
+{{- else -}}
+	{{- if .IsBranch -}}
+		{{svg "octicon-git-branch"}}
+	{{- else if .IsTag -}}
+		{{svg "octicon-tag"}}
+	{{- end -}}
+	{{.ShortName}}
+{{- end -}}

templates/repo/commits_table.tmpl

@@ -10,9 +10,9 @@
 	</div>
 	{{if .IsDiffCompare}}
 		<div class="commits-table-right tw-whitespace-nowrap">
-			<a href="{{$.CommitRepoLink}}/commit/{{.BeforeCommitID | PathEscape}}" class="ui green sha label tw-mx-0">{{if not .BaseIsCommit}}{{if .BaseIsBranch}}{{svg "octicon-git-branch"}}{{else if .BaseIsTag}}{{svg "octicon-tag"}}{{end}}{{.BaseBranch}}{{else}}{{ShortSha .BaseBranch}}{{end}}</a>
-			...
-			<a href="{{$.CommitRepoLink}}/commit/{{.AfterCommitID | PathEscape}}" class="ui green sha label tw-mx-0">{{if not .HeadIsCommit}}{{if .HeadIsBranch}}{{svg "octicon-git-branch"}}{{else if .HeadIsTag}}{{svg "octicon-tag"}}{{end}}{{.HeadBranch}}{{else}}{{ShortSha .HeadBranch}}{{end}}</a>
+			<a href="{{$.CommitRepoLink}}/commit/{{.BeforeCommitID | PathEscape}}" class="ui green sha label tw-mx-0">{{template "repo/commits_ref_name" .CompareInfo.BaseRef}}</a>
+			{{.CompareInfo.Separator}}
+			<a href="{{$.CommitRepoLink}}/commit/{{.AfterCommitID | PathEscape}}" class="ui green sha label tw-mx-0">{{template "repo/commits_ref_name" .CompareInfo.HeadRef}}</a>
 		</div>
 	{{end}}
 </h4>

templates/repo/diff/compare.tmpl

@@ -5,8 +5,8 @@
 	<div class="ui container fluid padded">
 		<h2 class="ui header">
 			{{if and $.PageIsComparePull $.IsSigned (not .Repository.IsArchived)}}
-				{{ctx.Locale.Tr "repo.pulls.compare_changes"}}
-				<div class="sub header">{{ctx.Locale.Tr "repo.pulls.compare_changes_desc"}}</div>
+				{{ctx.Locale.Tr "repo.compare.title"}}
+				<div class="sub header">{{ctx.Locale.Tr "repo.compare.description"}}</div>
 			{{else}}
 				{{ctx.Locale.Tr "action.compare_commits_general"}}
 			{{end}}
@@ -99,7 +99,7 @@
 				</div>
 			</div>
 
-			<a href="{{.RepoLink}}/compare/{{PathEscapeSegments .BaseBranch}}{{.OtherCompareSeparator}}{{if not $.PullRequestCtx.SameRepo}}{{PathEscape $.HeadUser.Name}}/{{PathEscape $.HeadRepo.Name}}:{{end}}{{PathEscapeSegments $.HeadBranch}}" title="{{ctx.Locale.Tr "repo.pulls.switch_comparison_type"}}">{{svg "octicon-arrow-left" 16}}<div class="compare-separator">{{.CompareSeparator}}</div></a>
+			<a href="{{.RepoLink}}/compare/{{PathEscapeSegments .BaseBranch}}{{$.CompareInfo.OtherSeparator}}{{if not $.PullRequestCtx.SameRepo}}{{PathEscape $.HeadUser.Name}}/{{PathEscape $.HeadRepo.Name}}:{{end}}{{PathEscapeSegments $.HeadBranch}}" title="{{ctx.Locale.Tr "repo.pulls.switch_comparison_type"}}">{{svg "octicon-arrow-left" 16}}<div class="compare-separator">{{.CompareSeparator}}</div></a>
 
 			<div class="ui dropdown jump select-branch">
 				<div class="ui basic small button">
@@ -173,12 +173,12 @@
 
 		{{$showDiffBox := and .CommitCount (not .IsNothingToCompare)}}
 		{{if and .IsSigned .PageIsComparePull}}
-			{{$allowCreatePR := or $.AllowEmptyPr (not .IsNothingToCompare)}}
+			{{$allowCreatePR := and ($.CompareInfo.BaseRef.IsBranch) ($.CompareInfo.HeadRef.IsBranch) (not $.CompareInfo.DirectComparison) (or $.AllowEmptyPr (not .IsNothingToCompare))}}
 			{{if .IsNothingToCompare}}
 				<div class="ui segment">
 					{{if $allowCreatePR}}
 						{{ctx.Locale.Tr "repo.pulls.nothing_to_compare_and_allow_empty_pr"}}
-					{{else if and .HeadIsBranch .BaseIsBranch}}
+					{{else if and $.CompareInfo.BaseRef.IsBranch $.CompareInfo.HeadRef.IsBranch}}
 						{{ctx.Locale.Tr "repo.pulls.nothing_to_compare"}}
 					{{else}}
 						{{ctx.Locale.Tr "repo.pulls.nothing_to_compare_have_tag"}}
@@ -205,8 +205,11 @@
 					{{end}}
 				</div>
 			{{else if $allowCreatePR}}
-				<div class="ui info message pullrequest-form-toggle {{if .ExpandNewPrForm}}tw-hidden{{end}}">
-					<button class="ui button primary show-panel toggle" data-panel=".pullrequest-form-toggle, .pullrequest-form">{{ctx.Locale.Tr "repo.pulls.new"}}</button>
+				<div class="ui info message flex-text-block tw-justify-between pullrequest-form-toggle {{if .ExpandNewPrForm}}tw-hidden{{end}}">
+					<span class="issue-title tw-break-anywhere tw-text-left">
+						{{ctx.Locale.Tr "repo.pulls.new.description"}}
+					</span>
+					<a class="ui button primary show-panel toggle tw-text-right" data-panel=".pullrequest-form-toggle, .pullrequest-form">{{ctx.Locale.Tr "repo.pulls.new"}}</a>
 				</div>
 				<div class="pullrequest-form {{if not .ExpandNewPrForm}}tw-hidden{{end}}">
 					{{template "repo/issue/new_form" .}}