models/issue: improve quality and performance of NewIssue function

.travis.yml

@@ -4,6 +4,7 @@ go:
   - 1.4
   - 1.5
   - 1.6
+  - 1.7
 
 before_install:
   - sudo apt-get update -qq

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 tip version: 0.9.79 (see [Releases](https://github.com/gogits/gogs/releases) for binary versions)
+##### Current tip version: 0.9.80 (see [Releases](https://github.com/gogits/gogs/releases) for binary versions)
 
 | Web | UI  | Preview  |
 |:-------------:|:-------:|:-------:|

cmd/web.go

@@ -504,10 +504,9 @@ func runWeb(ctx *cli.Context) error {
 			m.Combo("/_new/*").Get(repo.NewFile).
 				Post(bindIgnErr(auth.EditRepoFileForm{}), repo.NewFilePost)
 			m.Post("/_preview/*", bindIgnErr(auth.EditPreviewDiffForm{}), repo.DiffPreviewPost)
-			m.Combo("/upload/*").Get(repo.UploadFile).
+			m.Combo("/_upload/*").Get(repo.UploadFile).
 				Post(bindIgnErr(auth.UploadRepoFileForm{}), repo.UploadFilePost)
 			m.Post("/_delete/*", bindIgnErr(auth.DeleteRepoFileForm{}), repo.DeleteFilePost)
-			m.Post("/branches", bindIgnErr(auth.NewBranchForm{}), repo.NewBranchPost)
 			// m.Post("/upload-file", repo.UploadFileToServer)
 			// m.Post("/upload-remove", bindIgnErr(auth.RemoveUploadFileForm{}), repo.RemoveUploadFileFromServer)
 		}, reqRepoWriter, context.RepoRef(), func(ctx *context.Context) {

gogs.go

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

models/issue.go

@@ -25,7 +25,6 @@ import (
 )
 
 var (
-	ErrWrongIssueCounter  = errors.New("Invalid number of issues for this milestone")
 	ErrMissingIssueNumber = errors.New("No issue number specified")
 )
 
@@ -580,80 +579,86 @@ func (issue *Issue) ChangeAssignee(doer *User, assigneeID int64) (err error) {
 	return nil
 }
 
-// It's caller's responsibility to create action.
-func newIssue(e *xorm.Session, repo *Repository, issue *Issue, labelIDs []int64, uuids []string, isPull bool) (err error) {
-	issue.Title = strings.TrimSpace(issue.Title)
-	issue.Index = repo.NextIssueIndex()
+type NewIssueOptions struct {
+	Repo        *Repository
+	Issue       *Issue
+	LableIDs    []int64
+	Attachments []string // In UUID format.
+	IsPull      bool
+}
 
-	if issue.AssigneeID > 0 {
-		// Silently drop invalid assignee
-		valid, err := hasAccess(e, &User{ID: issue.AssigneeID}, repo, ACCESS_MODE_WRITE)
+func newIssue(e *xorm.Session, opts *NewIssueOptions) (err error) {
+	opts.Issue.Title = strings.TrimSpace(opts.Issue.Title)
+	opts.Issue.Index = opts.Repo.NextIssueIndex()
+
+	if opts.Issue.AssigneeID > 0 {
+		// Silently drop invalid assignee.
+		valid, err := hasAccess(e, &User{ID: opts.Issue.AssigneeID}, opts.Repo, ACCESS_MODE_WRITE)
 		if err != nil {
-			return fmt.Errorf("hasAccess: %v", err)
+			return fmt.Errorf("hasAccess [user_id: %d, repo_id: %d]: %v", opts.Issue.AssigneeID, opts.Repo.ID, err)
 		} else if !valid {
-			issue.AssigneeID = 0
+			opts.Issue.AssigneeID = 0
 		}
 	}
 
-	if _, err = e.Insert(issue); err != nil {
+	if _, err = e.Insert(opts.Issue); err != nil {
 		return err
 	}
 
-	if isPull {
-		_, err = e.Exec("UPDATE `repository` SET num_pulls=num_pulls+1 WHERE id=?", issue.RepoID)
+	if opts.IsPull {
+		_, err = e.Exec("UPDATE `repository` SET num_pulls = num_pulls + 1 WHERE id = ?", opts.Issue.RepoID)
 	} else {
-		_, err = e.Exec("UPDATE `repository` SET num_issues=num_issues+1 WHERE id=?", issue.RepoID)
+		_, err = e.Exec("UPDATE `repository` SET num_issues = num_issues + 1 WHERE id = ?", opts.Issue.RepoID)
 	}
 	if err != nil {
 		return err
 	}
 
-	if len(labelIDs) > 0 {
+	if len(opts.LableIDs) > 0 {
 		// During the session, SQLite3 dirver cannot handle retrieve objects after update something.
 		// So we have to get all needed labels first.
-		labels := make([]*Label, 0, len(labelIDs))
-		if err = e.In("id", labelIDs).Find(&labels); err != nil {
-			return fmt.Errorf("find all labels: %v", err)
+		labels := make([]*Label, 0, len(opts.LableIDs))
+		if err = e.In("id", opts.LableIDs).Find(&labels); err != nil {
+			return fmt.Errorf("find all labels [label_ids: %v]: %v", opts.LableIDs, err)
 		}
 
 		for _, label := range labels {
-			if label.RepoID != repo.ID {
+			// Silently drop invalid labels.
+			if label.RepoID != opts.Repo.ID {
 				continue
 			}
 
-			if err = issue.addLabel(e, label); err != nil {
-				return fmt.Errorf("addLabel: %v", err)
+			if err = opts.Issue.addLabel(e, label); err != nil {
+				return fmt.Errorf("addLabel [id: %d]: %v", label.ID, err)
 			}
 		}
 	}
 
-	if issue.MilestoneID > 0 {
-		if err = changeMilestoneAssign(e, 0, issue); err != nil {
+	if opts.Issue.MilestoneID > 0 {
+		if err = changeMilestoneAssign(e, opts.Issue, -1); err != nil {
 			return err
 		}
 	}
 
-	if err = newIssueUsers(e, repo, issue); err != nil {
+	if err = newIssueUsers(e, opts.Repo, opts.Issue); err != nil {
 		return err
 	}
 
-	// Check attachments.
-	for _, uuid := range uuids {
-		attachment, err := getAttachmentByUUID(e, uuid)
+	if len(opts.Attachments) > 0 {
+		attachments, err := getAttachmentsByUUIDs(e, opts.Attachments)
 		if err != nil {
-			if IsErrAttachmentNotExist(err) {
-				continue
-			}
-			return fmt.Errorf("getAttachmentByUUID [%s]: %v", uuid, err)
+			return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %v", opts.Attachments, err)
 		}
-		attachment.IssueID = issue.ID
-		// No assign value could be 0, so ignore AllCols().
-		if _, err = e.Id(attachment.ID).Update(attachment); err != nil {
-			return fmt.Errorf("update attachment [%d]: %v", attachment.ID, err)
+
+		for i := 0; i < len(attachments); i++ {
+			attachments[i].IssueID = opts.Issue.ID
+			if _, err = e.Id(attachments[i].ID).Update(attachments[i]); err != nil {
+				return fmt.Errorf("update attachment [id: %d]: %v", attachments[i].ID, err)
+			}
 		}
 	}
 
-	return issue.loadAttributes(e)
+	return opts.Issue.loadAttributes(e)
 }
 
 // NewIssue creates new issue with labels for repository.
@@ -664,7 +669,12 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string)
 		return err
 	}
 
-	if err = newIssue(sess, repo, issue, labelIDs, uuids, false); err != nil {
+	if err = newIssue(sess, &NewIssueOptions{
+		Repo:        repo,
+		Issue:       issue,
+		LableIDs:    labelIDs,
+		Attachments: uuids,
+	}); err != nil {
 		return fmt.Errorf("newIssue: %v", err)
 	}
 
@@ -672,8 +682,7 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string)
 		return fmt.Errorf("Commit: %v", err)
 	}
 
-	// Notify watchers.
-	act := &Action{
+	if err = NotifyWatchers(&Action{
 		ActUserID:    issue.Poster.ID,
 		ActUserName:  issue.Poster.Name,
 		ActEmail:     issue.Poster.Email,
@@ -683,10 +692,10 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string)
 		RepoUserName: repo.Owner.Name,
 		RepoName:     repo.Name,
 		IsPrivate:    repo.IsPrivate,
-	}
-	if err = NotifyWatchers(act); err != nil {
+	}); err != nil {
 		log.Error(4, "NotifyWatchers: %v", err)
-	} else if err = issue.MailParticipants(); err != nil {
+	}
+	if err = issue.MailParticipants(); err != nil {
 		log.Error(4, "MailParticipants: %v", err)
 	}
 
@@ -855,37 +864,41 @@ type IssueUser struct {
 }
 
 func newIssueUsers(e *xorm.Session, repo *Repository, issue *Issue) error {
-	users, err := repo.GetAssignees()
+	assignees, err := repo.getAssignees(e)
 	if err != nil {
-		return err
+		return fmt.Errorf("getAssignees: %v", err)
+	}
+
+	// Poster can be anyone, append later if not one of assignees.
+	isPosterAssignee := false
+
+	// Leave a seat for poster itself to append later, but if poster is one of assignee
+	// and just waste 1 unit is cheaper than re-allocate memory once.
+	issueUsers := make([]*IssueUser, 0, len(assignees)+1)
+	for _, assignee := range assignees {
+		isPoster := assignee.ID == issue.PosterID
+		issueUsers = append(issueUsers, &IssueUser{
+			IssueID:    issue.ID,
+			RepoID:     repo.ID,
+			UID:        assignee.ID,
+			IsPoster:   isPoster,
+			IsAssigned: assignee.ID == issue.AssigneeID,
+		})
+		if !isPosterAssignee && isPoster {
+			isPosterAssignee = true
+		}
 	}
-
-	iu := &IssueUser{
-		IssueID: issue.ID,
-		RepoID:  repo.ID,
+	if !isPosterAssignee {
+		issueUsers = append(issueUsers, &IssueUser{
+			IssueID:  issue.ID,
+			RepoID:   repo.ID,
+			UID:      issue.PosterID,
+			IsPoster: true,
+		})
 	}
 
-	// Poster can be anyone.
-	isNeedAddPoster := true
-	for _, u := range users {
-		iu.ID = 0
-		iu.UID = u.ID
-		iu.IsPoster = iu.UID == issue.PosterID
-		if isNeedAddPoster && iu.IsPoster {
-			isNeedAddPoster = false
-		}
-		iu.IsAssigned = iu.UID == issue.AssigneeID
-		if _, err = e.Insert(iu); err != nil {
-			return err
-		}
-	}
-	if isNeedAddPoster {
-		iu.ID = 0
-		iu.UID = issue.PosterID
-		iu.IsPoster = true
-		if _, err = e.Insert(iu); err != nil {
-			return err
-		}
+	if _, err = e.Insert(issueUsers); err != nil {
+		return err
 	}
 	return nil
 }
@@ -1499,9 +1512,9 @@ func ChangeMilestoneIssueStats(issue *Issue) (err error) {
 	return sess.Commit()
 }
 
-func changeMilestoneAssign(e *xorm.Session, oldMid int64, issue *Issue) error {
-	if oldMid > 0 {
-		m, err := getMilestoneByID(e, oldMid)
+func changeMilestoneAssign(e *xorm.Session, issue *Issue, oldMilestoneID int64) error {
+	if oldMilestoneID > 0 {
+		m, err := getMilestoneByID(e, oldMilestoneID)
 		if err != nil {
 			return err
 		}
@@ -1513,7 +1526,7 @@ func changeMilestoneAssign(e *xorm.Session, oldMid int64, issue *Issue) error {
 
 		if err = updateMilestone(e, m); err != nil {
 			return err
-		} else if _, err = e.Exec("UPDATE `issue_user` SET milestone_id=0 WHERE issue_id=?", issue.ID); err != nil {
+		} else if _, err = e.Exec("UPDATE `issue_user` SET milestone_id = 0 WHERE issue_id = ?", issue.ID); err != nil {
 			return err
 		}
 	}
@@ -1529,13 +1542,9 @@ func changeMilestoneAssign(e *xorm.Session, oldMid int64, issue *Issue) error {
 			m.NumClosedIssues++
 		}
 
-		if m.NumIssues == 0 {
-			return ErrWrongIssueCounter
-		}
-
 		if err = updateMilestone(e, m); err != nil {
 			return err
-		} else if _, err = e.Exec("UPDATE `issue_user` SET milestone_id=? WHERE issue_id=?", m.ID, issue.ID); err != nil {
+		} else if _, err = e.Exec("UPDATE `issue_user` SET milestone_id = ? WHERE issue_id = ?", m.ID, issue.ID); err != nil {
 			return err
 		}
 	}
@@ -1544,14 +1553,14 @@ func changeMilestoneAssign(e *xorm.Session, oldMid int64, issue *Issue) error {
 }
 
 // ChangeMilestoneAssign changes assignment of milestone for issue.
-func ChangeMilestoneAssign(oldMid int64, issue *Issue) (err error) {
+func ChangeMilestoneAssign(issue *Issue, oldMilestoneID int64) (err error) {
 	sess := x.NewSession()
 	defer sess.Close()
 	if err = sess.Begin(); err != nil {
 		return err
 	}
 
-	if err = changeMilestoneAssign(sess, oldMid, issue); err != nil {
+	if err = changeMilestoneAssign(sess, issue, oldMilestoneID); err != nil {
 		return err
 	}
 	return sess.Commit()
@@ -1677,6 +1686,16 @@ func getAttachmentByUUID(e Engine, uuid string) (*Attachment, error) {
 	return attach, nil
 }
 
+func getAttachmentsByUUIDs(e Engine, uuids []string) ([]*Attachment, error) {
+	if len(uuids) == 0 {
+		return []*Attachment{}, nil
+	}
+
+	// Silently drop invalid uuids.
+	attachments := make([]*Attachment, 0, len(uuids))
+	return attachments, e.In("uuid", uuids).Find(&attachments)
+}
+
 // GetAttachmentByUUID returns attachment by given UUID.
 func GetAttachmentByUUID(uuid string) (*Attachment, error) {
 	return getAttachmentByUUID(x, uuid)

models/models.go

@@ -29,6 +29,7 @@ type Engine interface {
 	Find(interface{}, ...interface{}) error
 	Get(interface{}) (bool, error)
 	Id(interface{}) *xorm.Session
+	In(string, ...interface{}) *xorm.Session
 	Insert(...interface{}) (int64, error)
 	InsertOne(interface{}) (int64, error)
 	Iterate(interface{}, xorm.IterFunc) error

models/pull.go

@@ -388,7 +388,13 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str
 		return err
 	}
 
-	if err = newIssue(sess, repo, pull, labelIDs, uuids, true); err != nil {
+	if err = newIssue(sess, &NewIssueOptions{
+		Repo:        repo,
+		Issue:       pull,
+		LableIDs:    labelIDs,
+		Attachments: uuids,
+		IsPull:      true,
+	}); err != nil {
 		return fmt.Errorf("newIssue: %v", err)
 	}
 

models/repo.go

@@ -316,33 +316,43 @@ func (repo *Repository) DeleteWiki() {
 	}
 }
 
-// GetAssignees returns all users that have write access of repository.
-func (repo *Repository) GetAssignees() (_ []*User, err error) {
-	if err = repo.GetOwner(); err != nil {
+func (repo *Repository) getAssignees(e Engine) (_ []*User, err error) {
+	if err = repo.getOwner(e); err != nil {
 		return nil, err
 	}
 
 	accesses := make([]*Access, 0, 10)
-	if err = x.Where("repo_id=? AND mode>=?", repo.ID, ACCESS_MODE_WRITE).Find(&accesses); err != nil {
+	if err = e.Where("repo_id = ? AND mode >= ?", repo.ID, ACCESS_MODE_WRITE).Find(&accesses); err != nil {
 		return nil, err
 	}
+	if len(accesses) == 0 {
+		return []*User{}, nil
+	}
+
+	userIDs := make([]int64, len(accesses))
+	for i := 0; i < len(accesses); i++ {
+		userIDs[i] = accesses[i].UserID
+	}
 
-	users := make([]*User, 0, len(accesses)+1) // Just waste 1 unit does not matter.
+	// Leave a seat for owner itself to append later, but if owner is an organization
+	// and just waste 1 unit is cheaper than re-allocate memory once.
+	users := make([]*User, 0, len(userIDs)+1)
+	if err = e.In("id", userIDs).Find(&users); err != nil {
+		return nil, err
+	}
 	if !repo.Owner.IsOrganization() {
 		users = append(users, repo.Owner)
 	}
 
-	var u *User
-	for i := range accesses {
-		u, err = GetUserByID(accesses[i].UserID)
-		if err != nil {
-			return nil, err
-		}
-		users = append(users, u)
-	}
 	return users, nil
 }
 
+// GetAssignees returns all users that have write access and can be assigned to issues
+// of the repository,
+func (repo *Repository) GetAssignees() (_ []*User, err error) {
+	return repo.getAssignees(x)
+}
+
 // GetAssigneeByID returns the user that has write access of repository by given ID.
 func (repo *Repository) GetAssigneeByID(userID int64) (*User, error) {
 	return GetAssigneeByID(repo, userID)
@@ -420,6 +430,8 @@ func (repo *Repository) AllowsPulls() bool {
 	return repo.CanEnablePulls() && repo.EnablePulls
 }
 
+// FIXME: should have a mutex to prevent producing same index for two issues that are created
+// closely enough.
 func (repo *Repository) NextIssueIndex() int64 {
 	return int64(repo.NumIssues+repo.NumPulls) + 1
 }

modules/auth/repo_form.go

@@ -342,18 +342,3 @@ type DeleteRepoFileForm struct {
 func (f *DeleteRepoFileForm) Validate(ctx *macaron.Context, errs binding.Errors) binding.Errors {
 	return validate(errs, ctx.Data, f, ctx.Locale)
 }
-
-// __________                             .__
-// \______   \____________    ____   ____ |  |__
-//  |    |  _/\_  __ \__  \  /    \_/ ___\|  |  \
-//  |    |   \ |  | \// __ \|   |  \  \___|   Y  \
-//  |______  / |__|  (____  /___|  /\___  >___|  /
-//         \/             \/     \/     \/     \/
-type NewBranchForm struct {
-	OldBranchName string `binding:"Required;MaxSize(100)"`
-	BranchName    string `binding:"Required;MaxSize(100)"`
-}
-
-func (f *NewBranchForm) Validate(ctx *macaron.Context, errs binding.Errors) binding.Errors {
-	return validate(errs, ctx.Data, f, ctx.Locale)
-}

routers/api/v1/repo/issue.go

@@ -149,9 +149,9 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
 	}
 	if ctx.Repo.IsWriter() && form.Milestone != nil &&
 		issue.MilestoneID != *form.Milestone {
-		oldMid := issue.MilestoneID
+		oldMilestoneID := issue.MilestoneID
 		issue.MilestoneID = *form.Milestone
-		if err = models.ChangeMilestoneAssign(oldMid, issue); err != nil {
+		if err = models.ChangeMilestoneAssign(issue, oldMilestoneID); err != nil {
 			ctx.Error(500, "ChangeMilestoneAssign", err)
 			return
 		}

routers/repo/branch.go

@@ -5,13 +5,8 @@
 package repo
 
 import (
-	"github.com/gogits/gogs/models"
-	"github.com/gogits/gogs/modules/auth"
 	"github.com/gogits/gogs/modules/base"
 	"github.com/gogits/gogs/modules/context"
-	"github.com/gogits/gogs/modules/log"
-	"net/url"
-	"strings"
 )
 
 const (
@@ -34,48 +29,3 @@ func Branches(ctx *context.Context) {
 	ctx.Data["Branches"] = brs
 	ctx.HTML(200, BRANCH)
 }
-
-func NewBranchPost(ctx *context.Context, form auth.NewBranchForm) {
-	oldBranchName := form.OldBranchName
-	branchName := form.BranchName
-
-	if ctx.HasError() || !ctx.Repo.IsWriter() || branchName == oldBranchName {
-		ctx.Redirect(ctx.Repo.RepoLink + "/src/" + oldBranchName)
-		return
-	}
-
-	branchName = url.QueryEscape(strings.Replace(strings.Trim(branchName, " "), " ", "-", -1))
-
-	if _, err := ctx.Repo.Repository.GetBranch(branchName); err == nil {
-		ctx.Redirect(ctx.Repo.RepoLink + "/src/" + branchName)
-		return
-	}
-
-	if err := ctx.Repo.Repository.CreateNewBranch(ctx.User, oldBranchName, branchName); err != nil {
-		ctx.Handle(404, "repo.Branches(CreateNewBranch)", err)
-		log.Error(4, "%s: %v", "EditFile", err)
-		return
-	}
-
-	// Was successful, so now need to call models.CommitRepoAction() with the new commitID for webhooks and watchers
-	if branch, err := ctx.Repo.Repository.GetBranch(branchName); err != nil {
-		log.Error(4, "repo.Repository.GetBranch(%s): %v", branchName, err)
-	} else if commit, err := branch.GetCommit(); err != nil {
-		log.Error(4, "branch.GetCommit(): %v", err)
-	} else {
-		pc := &models.PushCommits{
-			Len:     1,
-			Commits: []*models.PushCommit{models.CommitToPushCommit(commit)},
-		}
-		oldCommitID := "0000000000000000000000000000000000000000" // New Branch so we use all 0s
-		newCommitID := commit.ID.String()
-		if err := models.CommitRepoAction(ctx.User.ID, ctx.Repo.Owner.ID, ctx.User.LowerName, ctx.Repo.Owner.Email,
-			ctx.Repo.Repository.ID, ctx.Repo.Owner.LowerName, ctx.Repo.Repository.Name, "refs/heads/"+branchName, pc,
-			oldCommitID, newCommitID); err != nil {
-			log.Error(4, "models.CommitRepoAction(branch = %s): %v", branchName, err)
-		}
-		models.HookQueue.Add(ctx.Repo.Repository.ID)
-	}
-
-	ctx.Redirect(ctx.Repo.RepoLink + "/src/" + branchName)
-}

routers/repo/issue.go

@@ -754,9 +754,9 @@ func UpdateIssueMilestone(ctx *context.Context) {
 		return
 	}
 
-	oldMid := issue.MilestoneID
-	mid := ctx.QueryInt64("id")
-	if oldMid == mid {
+	oldMilestoneID := issue.MilestoneID
+	milestoneID := ctx.QueryInt64("id")
+	if oldMilestoneID == milestoneID {
 		ctx.JSON(200, map[string]interface{}{
 			"ok": true,
 		})
@@ -764,8 +764,8 @@ func UpdateIssueMilestone(ctx *context.Context) {
 	}
 
 	// Not check for invalid milestone id and give responsibility to owners.
-	issue.MilestoneID = mid
-	if err := models.ChangeMilestoneAssign(oldMid, issue); err != nil {
+	issue.MilestoneID = milestoneID
+	if err := models.ChangeMilestoneAssign(issue, oldMilestoneID); err != nil {
 		ctx.Handle(500, "ChangeMilestoneAssign", err)
 		return
 	}

templates/.VERSION

@@ -1 +1 @@
-0.9.79.0815
+0.9.80.0815