#1597 support pull requests in same repository

README.md

@@ -3,7 +3,7 @@ Gogs - Go Git Service [![Build Status](https://travis-ci.org/gogits/gogs.svg?bra
 
 ![](https://github.com/gogits/gogs/blob/master/public/img/gogs-large-resize.png?raw=true)
 
-##### Current version: 0.8.56
+##### Current version: 0.8.57
 
 | Web | UI  | Preview  |
 |:-------------:|:-------:|:-------:|

cmd/web.go

@@ -79,7 +79,7 @@ func checkVersion() {
 
 	// Check dependency version.
 	checkers := []VerChecker{
-		{"github.com/go-xorm/xorm", func() string { return xorm.Version }, "0.4.4.1029"},
+		{"github.com/go-xorm/xorm", func() string { return xorm.Version }, "0.5.2.0304"},
 		{"github.com/go-macaron/binding", binding.Version, "0.2.1"},
 		{"github.com/go-macaron/cache", cache.Version, "0.1.2"},
 		{"github.com/go-macaron/csrf", csrf.Version, "0.0.3"},
@@ -88,7 +88,7 @@ func checkVersion() {
 		{"github.com/go-macaron/toolbox", toolbox.Version, "0.1.0"},
 		{"gopkg.in/ini.v1", ini.Version, "1.8.4"},
 		{"gopkg.in/macaron.v1", macaron.Version, "0.8.0"},
-		{"github.com/gogits/git-module", git.Version, "0.2.8"},
+		{"github.com/gogits/git-module", git.Version, "0.2.9"},
 		{"github.com/gogits/go-gogs-client", gogs.Version, "0.7.3"},
 	}
 	for _, c := range checkers {

gogs.go

@@ -17,7 +17,7 @@ import (
 	"github.com/gogits/gogs/modules/setting"
 )
 
-const APP_VER = "0.8.56.0304"
+const APP_VER = "0.8.57.0304"
 
 func init() {
 	runtime.GOMAXPROCS(runtime.NumCPU())

models/pull.go

@@ -487,7 +487,7 @@ func (pr *PullRequest) UpdatePatch() (err error) {
 // FIXME: could fail after user force push head repo, should we always force push here?
 // FIXME: Only push branches that are actually updates?
 func (pr *PullRequest) PushToBaseRepo() (err error) {
-	log.Trace("PushToBaseRepo[%[1]d]: pushing commits to base repo 'refs/pull/%[1]d/head'", pr.ID)
+	log.Trace("PushToBaseRepo[%d]: pushing commits to base repo 'refs/pull/%d/head'", pr.BaseRepoID, pr.Index)
 
 	headRepoPath := pr.HeadRepo.RepoPath()
 	headGitRepo, err := git.OpenRepository(headRepoPath)

models/repo.go

@@ -184,6 +184,11 @@ type Repository struct {
 
 func (repo *Repository) AfterSet(colName string, _ xorm.Cell) {
 	switch colName {
+	case "default_branch":
+		// FIXME: use models migration to solve all at once.
+		if len(repo.DefaultBranch) == 0 {
+			repo.DefaultBranch = "master"
+		}
 	case "num_closed_issues":
 		repo.NumOpenIssues = repo.NumIssues - repo.NumClosedIssues
 	case "num_closed_pulls":

models/user.go

@@ -348,23 +348,26 @@ func (u *User) UploadAvatar(data []byte) error {
 
 // IsAdminOfRepo returns true if user has admin or higher access of repository.
 func (u *User) IsAdminOfRepo(repo *Repository) bool {
-	if err := repo.GetOwner(); err != nil {
-		log.Error(3, "GetOwner: %v", err)
-		return false
-	}
-
-	if repo.Owner.IsOrganization() {
+	if repo.MustOwner().IsOrganization() {
 		has, err := HasAccess(u, repo, ACCESS_MODE_ADMIN)
 		if err != nil {
 			log.Error(3, "HasAccess: %v", err)
-			return false
 		}
 		return has
 	}
 
 	return repo.IsOwnedBy(u.Id)
 }
 
+// CanWriteTo returns true if user has write access to given repository.
+func (u *User) CanWriteTo(repo *Repository) bool {
+	has, err := HasAccess(u, repo, ACCESS_MODE_WRITE)
+	if err != nil {
+		log.Error(3, "HasAccess: %v", err)
+	}
+	return has
+}
+
 // IsOrganization returns true if user is actually a organization.
 func (u *User) IsOrganization() bool {
 	return u.Type == ORGANIZATION

modules/middleware/repo.go

@@ -32,25 +32,6 @@ func RetrieveBaseRepo(ctx *Context, repo *models.Repository) {
 		ctx.Handle(500, "BaseRepo.GetOwner", err)
 		return
 	}
-
-	bsaeRepo := repo.BaseRepo
-	baseGitRepo, err := git.OpenRepository(models.RepoPath(bsaeRepo.Owner.Name, bsaeRepo.Name))
-	if err != nil {
-		ctx.Handle(500, "OpenRepository", err)
-		return
-	}
-	if len(bsaeRepo.DefaultBranch) > 0 && baseGitRepo.IsBranchExist(bsaeRepo.DefaultBranch) {
-		ctx.Data["BaseDefaultBranch"] = bsaeRepo.DefaultBranch
-	} else {
-		baseBranches, err := baseGitRepo.GetBranches()
-		if err != nil {
-			ctx.Handle(500, "GetBranches", err)
-			return
-		}
-		if len(baseBranches) > 0 {
-			ctx.Data["BaseDefaultBranch"] = baseBranches[0]
-		}
-	}
 }
 
 func RepoAssignment(args ...bool) macaron.Handler {
@@ -154,20 +135,38 @@ func RepoAssignment(args ...bool) macaron.Handler {
 		ctx.Data["Tags"] = tags
 		ctx.Repo.Repository.NumTags = len(tags)
 
+		ctx.Data["Title"] = owner.Name + "/" + repo.Name
+		ctx.Data["Repository"] = repo
+		ctx.Data["Owner"] = ctx.Repo.Repository.Owner
+		ctx.Data["IsRepositoryOwner"] = ctx.Repo.IsOwner()
+		ctx.Data["IsRepositoryAdmin"] = ctx.Repo.IsAdmin()
+		ctx.Data["IsRepositoryPusher"] = ctx.Repo.IsPusher()
+
 		if repo.IsFork {
 			RetrieveBaseRepo(ctx, repo)
 			if ctx.Written() {
 				return
 			}
 		}
 
-		ctx.Data["Title"] = owner.Name + "/" + repo.Name
-		ctx.Data["Repository"] = repo
-		ctx.Data["Owner"] = ctx.Repo.Repository.Owner
-		ctx.Data["IsRepositoryOwner"] = ctx.Repo.IsOwner()
-		ctx.Data["IsRepositoryAdmin"] = ctx.Repo.IsAdmin()
-		ctx.Data["IsRepositoryPusher"] = ctx.Repo.IsPusher()
-		ctx.Data["CanPullRequest"] = ctx.Repo.IsAdmin() && repo.BaseRepo != nil && repo.BaseRepo.AllowsPulls()
+		// People who have push access and propose a new pull request.
+		if ctx.Repo.IsPusher() {
+			// Pull request is allowed if this is a fork repository
+			// and base repository accepts pull requests.
+			if repo.BaseRepo != nil {
+				if repo.BaseRepo.AllowsPulls() {
+					ctx.Data["CanPullRequest"] = true
+					ctx.Data["BaseRepo"] = repo.BaseRepo
+				}
+			} else {
+				// Or, this is repository accepts pull requests between branches.
+				if repo.AllowsPulls() {
+					ctx.Data["CanPullRequest"] = true
+					ctx.Data["BaseRepo"] = repo
+					ctx.Data["IsBetweenBranches"] = true
+				}
+			}
+		}
 
 		ctx.Data["DisableSSH"] = setting.SSH.Disabled
 		ctx.Data["CloneLink"] = repo.CloneLink()

routers/repo/pull.go

@@ -414,35 +414,53 @@ func MergePullRequest(ctx *middleware.Context) {
 }
 
 func ParseCompareInfo(ctx *middleware.Context) (*models.User, *models.Repository, *git.Repository, *git.PullRequestInfo, string, string) {
-	// Get compare branch information.
+	// Get compared branches information
+	// format: <base branch>...[<head repo>:]<head branch>
+	// base<-head: master...head:feature
+	// same repo: master...feature
 	infos := strings.Split(ctx.Params("*"), "...")
 	if len(infos) != 2 {
+		log.Trace("Not enough compared branches information: %s", infos)
 		ctx.Handle(404, "CompareAndPullRequest", nil)
 		return nil, nil, nil, nil, "", ""
 	}
 
 	baseBranch := infos[0]
 	ctx.Data["BaseBranch"] = baseBranch
 
+	var (
+		headUser   *models.User
+		headBranch string
+		isSameRepo bool
+		err        error
+	)
+
+	// If there is no head repository, it means pull request between same repository.
 	headInfos := strings.Split(infos[1], ":")
-	if len(headInfos) != 2 {
-		ctx.Handle(404, "CompareAndPullRequest", nil)
-		return nil, nil, nil, nil, "", ""
-	}
-	headUsername := headInfos[0]
-	headBranch := headInfos[1]
-	ctx.Data["HeadBranch"] = headBranch
+	if len(headInfos) == 1 {
+		isSameRepo = true
+		headUser = ctx.Repo.Owner
+		headBranch = headInfos[0]
 
-	headUser, err := models.GetUserByName(headUsername)
-	if err != nil {
-		if models.IsErrUserNotExist(err) {
-			ctx.Handle(404, "GetUserByName", nil)
-		} else {
-			ctx.Handle(500, "GetUserByName", err)
+	} else if len(headInfos) == 2 {
+		headUser, err = models.GetUserByName(headInfos[0])
+		if err != nil {
+			if models.IsErrUserNotExist(err) {
+				ctx.Handle(404, "GetUserByName", nil)
+			} else {
+				ctx.Handle(500, "GetUserByName", err)
+			}
+			return nil, nil, nil, nil, "", ""
 		}
+		headBranch = headInfos[1]
+
+	} else {
+		ctx.Handle(404, "CompareAndPullRequest", nil)
 		return nil, nil, nil, nil, "", ""
 	}
 	ctx.Data["HeadUser"] = headUser
+	ctx.Data["HeadBranch"] = headBranch
+	ctx.Data["IsBetweenBranches"] = isSameRepo
 
 	repo := ctx.Repo.Repository
 
@@ -452,17 +470,23 @@ func ParseCompareInfo(ctx *middleware.Context) (*models.User, *models.Repository
 		return nil, nil, nil, nil, "", ""
 	}
 
-	// Check if current user has fork of repository.
+	// Check if current user has fork of repository or in the same repository.
 	headRepo, has := models.HasForkedRepo(headUser.Id, repo.ID)
-	if !has || (!ctx.User.IsAdminOfRepo(headRepo) && !ctx.User.IsAdmin) {
+	if (!has && !isSameRepo) || (!ctx.User.CanWriteTo(headRepo) && !ctx.User.IsAdmin) {
 		ctx.Handle(404, "HasForkedRepo", nil)
 		return nil, nil, nil, nil, "", ""
 	}
 
-	headGitRepo, err := git.OpenRepository(models.RepoPath(headUser.Name, headRepo.Name))
-	if err != nil {
-		ctx.Handle(500, "OpenRepository", err)
-		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, "", ""
+		}
 	}
 
 	// Check if head branch is valid.
@@ -648,8 +672,7 @@ func CompareAndPullRequestPost(ctx *middleware.Context, form auth.CreateIssueFor
 	if err := models.NewPullRequest(repo, pullIssue, labelIDs, attachments, pullRequest, patch); err != nil {
 		ctx.Handle(500, "NewPullRequest", err)
 		return
-	}
-	if err := pullRequest.PushToBaseRepo(); err != nil {
+	} else if err := pullRequest.PushToBaseRepo(); err != nil {
 		ctx.Handle(500, "PushToBaseRepo", err)
 		return
 	}

templates/.VERSION

@@ -1 +1 @@
-0.8.56.0304
+0.8.57.0304

templates/repo/home.tmpl

@@ -9,8 +9,7 @@
 		<div class="ui secondary menu">
 			{{if .CanPullRequest}}
 				<div class="fitted item">
-					{{ $baseRepo := .Repository.BaseRepo}}
-					<a href="{{AppSubUrl}}/{{$baseRepo.Owner.Name}}/{{$baseRepo.Name}}/compare/{{$.BaseDefaultBranch}}...{{$.Owner.Name}}:{{$.BranchName}}">
+					<a href="{{.BaseRepo.RepoLink}}/compare/{{.BaseRepo.DefaultBranch}}...{{if not .IsBetweenBranches}}{{$.Owner.Name}}:{{end}}{{$.BranchName}}">
 						<button class="ui green small button"><i class="octicon octicon-git-compare"></i></button>
 					</a>
 				</div>

templates/repo/issue/list.tmpl

@@ -8,7 +8,7 @@
 				{{if .PageIsIssueList}}
 					<a class="ui green button" href="{{.RepoLink}}/issues/new">{{.i18n.Tr "repo.issues.new"}}</a>
 				{{else}}
-					<a class="ui green button {{if not .HasForkedRepo}}disabled{{end}}" href="{{.RepoLink}}/compare/{{.BranchName}}...{{.SignedUserName}}:{{.BranchName}}">{{.i18n.Tr "repo.pulls.new"}}</a>
+					<a class="ui green button {{if not (or .CanPullRequest .HasForkedRepo)}}disabled{{end}}" href="{{.RepoLink}}/compare/{{.Repository.DefaultBranch}}...{{if not (eq .Owner.Name .SignedUserName)}}{{.SignedUserName}}:{{end}}{{.BranchName}}">{{.i18n.Tr "repo.pulls.new"}}</a>
 				{{end}}
 			</div>
 		</div>

templates/repo/pulls/compare.tmpl

@@ -21,7 +21,7 @@
 						</div>
 						<div class="scrolling menu">
 							{{range .Branches}}
-								<div class="item {{if eq $.BaseBranch .}}selected{{end}}" data-url="{{$.RepoLink}}/compare/{{.}}...{{$.HeadUser.Name}}:{{$.HeadBranch}}">{{.}}</div>
+								<div class="item {{if eq $.BaseBranch .}}selected{{end}}" data-url="{{$.RepoLink}}/compare/{{.}}...{{if not $.IsBetweenBranches}}{{$.HeadUser.Name}}:{{end}}{{$.HeadBranch}}">{{.}}</div>
 							{{end}}
 						</div>
 					</div>
@@ -39,7 +39,7 @@
 						</div>
 						<div class="scrolling menu">
 							{{range .HeadBranches}}
-								<div class="{{if eq $.HeadBranch .}}selected{{end}} item" data-url="{{$.RepoLink}}/compare/{{$.BaseBranch}}...{{$.HeadUser.Name}}:{{.}}">{{.}}</div>
+								<div class="{{if eq $.HeadBranch .}}selected{{end}} item" data-url="{{$.RepoLink}}/compare/{{$.BaseBranch}}...{{if not $.IsBetweenBranches}}{{$.HeadUser.Name}}:{{end}}{{.}}">{{.}}</div>
 							{{end}}
 						</div>
 					</div>