From 2d2d85bba4dd5131e72db533c31aab423f86232e Mon Sep 17 00:00:00 2001 From: Unknwon Date: Fri, 4 Mar 2016 15:43:01 -0500 Subject: [PATCH] #1597 support pull requests in same repository --- README.md | 2 +- cmd/web.go | 4 +- gogs.go | 2 +- models/pull.go | 2 +- models/repo.go | 5 +++ models/user.go | 17 ++++---- modules/log/database.go | 68 ------------------------------- modules/middleware/repo.go | 51 ++++++++++++----------- routers/repo/pull.go | 67 ++++++++++++++++++++---------- templates/.VERSION | 2 +- templates/repo/home.tmpl | 3 +- templates/repo/issue/list.tmpl | 2 +- templates/repo/pulls/compare.tmpl | 4 +- 13 files changed, 95 insertions(+), 134 deletions(-) delete mode 100644 modules/log/database.go diff --git a/README.md b/README.md index 02dff0efc2..cd4a816131 100644 --- a/README.md +++ b/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 | |:-------------:|:-------:|:-------:| diff --git a/cmd/web.go b/cmd/web.go index 2e4989039c..7158f09a34 100644 --- a/cmd/web.go +++ b/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 { diff --git a/gogs.go b/gogs.go index 4ca4382f19..6cad5fe3c4 100644 --- a/gogs.go +++ b/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()) diff --git a/models/pull.go b/models/pull.go index 276dc1bcfd..77484b9562 100644 --- a/models/pull.go +++ b/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) diff --git a/models/repo.go b/models/repo.go index 17c0082cbd..3f631b6984 100644 --- a/models/repo.go +++ b/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": diff --git a/models/user.go b/models/user.go index 3264c0634b..6e7c27293d 100644 --- a/models/user.go +++ b/models/user.go @@ -348,16 +348,10 @@ 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 } @@ -365,6 +359,15 @@ func (u *User) IsAdminOfRepo(repo *Repository) bool { 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 diff --git a/modules/log/database.go b/modules/log/database.go deleted file mode 100644 index 5857cb6d66..0000000000 --- a/modules/log/database.go +++ /dev/null @@ -1,68 +0,0 @@ -// Copyright 2014 The Gogs Authors. All rights reserved. -// Use of this source code is governed by a MIT-style -// license that can be found in the LICENSE file. - -package log - -import ( - "encoding/json" - - "github.com/go-xorm/xorm" -) - -type Log struct { - Id int64 - Level int - Msg string `xorm:"TEXT"` -} - -// DatabaseWriter implements LoggerInterface and is used to log into database. -type DatabaseWriter struct { - Driver string `json:"driver"` - Conn string `json:"conn"` - Level int `json:"level"` - x *xorm.Engine -} - -func NewDatabase() LoggerInterface { - return &DatabaseWriter{Level: TRACE} -} - -// init database writer with json config. -// config like: -// { -// "driver": "mysql" -// "conn":"root:root@tcp(127.0.0.1:3306)/gogs?charset=utf8", -// "level": 0 -// } -// connection string is based on xorm. -func (d *DatabaseWriter) Init(jsonconfig string) (err error) { - if err = json.Unmarshal([]byte(jsonconfig), d); err != nil { - return err - } - d.x, err = xorm.NewEngine(d.Driver, d.Conn) - if err != nil { - return err - } - return d.x.Sync(new(Log)) -} - -// write message in database writer. -func (d *DatabaseWriter) WriteMsg(msg string, skip, level int) error { - if level < d.Level { - return nil - } - - _, err := d.x.Insert(&Log{Level: level, Msg: msg}) - return err -} - -func (_ *DatabaseWriter) Flush() { -} - -func (_ *DatabaseWriter) Destroy() { -} - -func init() { - Register("database", NewDatabase) -} diff --git a/modules/middleware/repo.go b/modules/middleware/repo.go index 002982656d..7c99d2d0c1 100644 --- a/modules/middleware/repo.go +++ b/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,6 +135,13 @@ 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() { @@ -161,13 +149,24 @@ func RepoAssignment(args ...bool) macaron.Handler { } } - 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() diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 58acb1758d..e42ac61250 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -414,9 +414,13 @@ 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<-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, "", "" } @@ -424,25 +428,39 @@ func ParseCompareInfo(ctx *middleware.Context) (*models.User, *models.Repository 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 } diff --git a/templates/.VERSION b/templates/.VERSION index b147b846a1..76f54f43fe 100644 --- a/templates/.VERSION +++ b/templates/.VERSION @@ -1 +1 @@ -0.8.56.0304 \ No newline at end of file +0.8.57.0304 \ No newline at end of file diff --git a/templates/repo/home.tmpl b/templates/repo/home.tmpl index 47d53bf13c..36752a6eab 100644 --- a/templates/repo/home.tmpl +++ b/templates/repo/home.tmpl @@ -9,8 +9,7 @@
{{if .CanPullRequest}}
- {{ $baseRepo := .Repository.BaseRepo}} - +
diff --git a/templates/repo/issue/list.tmpl b/templates/repo/issue/list.tmpl index 790a6572e4..b4e79ee9c6 100644 --- a/templates/repo/issue/list.tmpl +++ b/templates/repo/issue/list.tmpl @@ -8,7 +8,7 @@ {{if .PageIsIssueList}} {{.i18n.Tr "repo.issues.new"}} {{else}} - {{.i18n.Tr "repo.pulls.new"}} + {{.i18n.Tr "repo.pulls.new"}} {{end}}
diff --git a/templates/repo/pulls/compare.tmpl b/templates/repo/pulls/compare.tmpl index 110dfa46b3..30a2fd5b69 100644 --- a/templates/repo/pulls/compare.tmpl +++ b/templates/repo/pulls/compare.tmpl @@ -21,7 +21,7 @@
{{range .Branches}} -
{{.}}
+
{{.}}
{{end}}
@@ -39,7 +39,7 @@
{{range .HeadBranches}} -
{{.}}
+
{{.}}
{{end}}