routers/repo/pull: fix 404 on PR compare (#4074)

Due to recent code refactor, ctx.PullRequest is not initialized for
route repo.CompareAndPullRequest, which leads the UI thinks the
compare is not happening inside the same repository.

The current fix is to allow compare URL to include redundant head
user name so everything works fine again, but code logic isn't
as clean as before.

Made comments about possible future fix.

cmd/web.go

@@ -525,6 +525,10 @@ func runWeb(ctx *cli.Context) error {
 			ctx.Data["CommitsCount"] = ctx.Repo.CommitsCount
 		})
 
+		// FIXME: Should use ctx.Repo.PullRequest to unify template, currently we have inconsistent URL
+		// for PR in same repository. After select branch on the page, the URL contains redundant head user name.
+		// e.g. /org1/test-repo/compare/master...org1:develop
+		// which should be /org1/test-repo/compare/master...develop
 		m.Combo("/compare/*", repo.MustAllowPulls).Get(repo.CompareAndPullRequest).
 			Post(bindIgnErr(auth.CreateIssueForm{}), repo.CompareAndPullRequestPost)
 

gogs.go

@@ -16,7 +16,7 @@ import (
 	"github.com/gogits/gogs/modules/setting"
 )
 
-const APP_VER = "0.9.134.0208"
+const APP_VER = "0.9.135.0208"
 
 func init() {
 	setting.AppVer = APP_VER

routers/repo/pull.go

@@ -453,6 +453,7 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
 			return nil, nil, nil, nil, "", ""
 		}
 		headBranch = headInfos[1]
+		isSameRepo = headUser.ID == baseRepo.OwnerID
 
 	} else {
 		ctx.Handle(404, "CompareAndPullRequest", nil)
@@ -468,24 +469,30 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
 		return nil, nil, nil, nil, "", ""
 	}
 
-	// Check if current user has fork of repository or in the same repository.
-	headRepo, has := models.HasForkedRepo(headUser.ID, baseRepo.ID)
-	if !has && !isSameRepo {
-		log.Trace("ParseCompareInfo[%d]: does not have fork or in same repository", baseRepo.ID)
-		ctx.Handle(404, "ParseCompareInfo", nil)
-		return nil, nil, nil, nil, "", ""
-	}
+	var (
+		headRepo    *models.Repository
+		headGitRepo *git.Repository
+	)
+
+	// In case user included redundant head user name for comparison in same repository,
+	// no need to check the fork relation.
+	if !isSameRepo {
+		var has bool
+		headRepo, has = models.HasForkedRepo(headUser.ID, baseRepo.ID)
+		if !has {
+			log.Trace("ParseCompareInfo[%d]: does not have fork or in same repository", baseRepo.ID)
+			ctx.Handle(404, "ParseCompareInfo", nil)
+			return nil, nil, nil, nil, "", ""
+		}
 
-	var headGitRepo *git.Repository
-	if isSameRepo {
-		headRepo = ctx.Repo.Repository
-		headGitRepo = ctx.Repo.GitRepo
-	} else {
 		headGitRepo, err = git.OpenRepository(models.RepoPath(headUser.Name, headRepo.Name))
 		if err != nil {
 			ctx.Handle(500, "OpenRepository", err)
 			return nil, nil, nil, nil, "", ""
 		}
+	} else {
+		headRepo = ctx.Repo.Repository
+		headGitRepo = ctx.Repo.GitRepo
 	}
 
 	if !ctx.User.IsWriterOfRepo(headRepo) && !ctx.User.IsAdmin {

templates/.VERSION

@@ -1 +1 @@
-0.9.134.0208
+0.9.135.0208