From 6b98d58906e4df063a2af59f97bd74c1c4cc1c84 Mon Sep 17 00:00:00 2001 From: Unknwon Date: Fri, 26 Aug 2016 13:40:53 -0700 Subject: [PATCH] #2966 code cleanup --- .gopmfile | 2 +- README.md | 2 +- glide.lock | 2 +- gogs.go | 2 +- models/error.go | 5 +- models/issue.go | 121 ++++++++++++++++----------- models/issue_comment.go | 46 ++++++++-- routers/api/v1/repo/issue.go | 2 +- routers/api/v1/repo/issue_comment.go | 80 +++++------------- routers/repo/issue.go | 1 - templates/.VERSION | 2 +- 11 files changed, 138 insertions(+), 127 deletions(-) diff --git a/.gopmfile b/.gopmfile index 0ea9eed96..ab1550b9e 100644 --- a/.gopmfile +++ b/.gopmfile @@ -19,7 +19,7 @@ github.com/go-xorm/xorm = commit:c6c7056 github.com/gogits/chardet = commit:2404f77 github.com/gogits/cron = commit:7f3990a github.com/gogits/git-module = commit:7b206b5 -github.com/gogits/go-gogs-client = commit:2ffd470 +github.com/gogits/go-gogs-client = commit:c52f7ee github.com/issue9/identicon = commit:d36b545 github.com/jaytaylor/html2text = commit:52d9b78 github.com/kardianos/minwinsvc = commit:cad6b2b diff --git a/README.md b/README.md index 6e71030c6..c3b250e73 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 tip version: 0.9.85 (see [Releases](https://github.com/gogits/gogs/releases) for binary versions) +##### Current tip version: 0.9.86 (see [Releases](https://github.com/gogits/gogs/releases) for binary versions) | Web | UI | Preview | |:-------------:|:-------:|:-------:| diff --git a/glide.lock b/glide.lock index a64dde1d8..f5bf62ed6 100644 --- a/glide.lock +++ b/glide.lock @@ -43,7 +43,7 @@ imports: - name: github.com/gogits/git-module version: 7b206b529a09ae8cfa1df52a6c0cdd2612cfc6fc - name: github.com/gogits/go-gogs-client - version: 2ffd4704c6f37d7fb10110450fe035fa6df08db8 + version: c52f7ee0cc58d3cd6e379025552873a8df6de322 - name: github.com/issue9/identicon version: d36b54562f4cf70c83653e13dc95c220c79ef521 - name: github.com/jaytaylor/html2text diff --git a/gogs.go b/gogs.go index acc7b5456..1874c4d0a 100644 --- a/gogs.go +++ b/gogs.go @@ -17,7 +17,7 @@ import ( "github.com/gogits/gogs/modules/setting" ) -const APP_VER = "0.9.85.0824" +const APP_VER = "0.9.86.0826" func init() { runtime.GOMAXPROCS(runtime.NumCPU()) diff --git a/models/error.go b/models/error.go index 89c6389b0..240e60677 100644 --- a/models/error.go +++ b/models/error.go @@ -526,7 +526,8 @@ func (err ErrPullRequestNotExist) Error() string { // \/ \/ \/ \/ \/ type ErrCommentNotExist struct { - ID int64 + ID int64 + IssueID int64 } func IsErrCommentNotExist(err error) bool { @@ -535,7 +536,7 @@ func IsErrCommentNotExist(err error) bool { } func (err ErrCommentNotExist) Error() string { - return fmt.Sprintf("comment does not exist [id: %d]", err.ID) + return fmt.Sprintf("comment does not exist [id: %d, issue_id: %d]", err.ID, err.IssueID) } // .____ ___. .__ diff --git a/models/issue.go b/models/issue.go index 1446056e5..a90ebaf86 100644 --- a/models/issue.go +++ b/models/issue.go @@ -73,70 +73,55 @@ func (issue *Issue) BeforeUpdate() { } func (issue *Issue) AfterSet(colName string, _ xorm.Cell) { - var err error switch colName { - case "id": - issue.Attachments, err = GetAttachmentsByIssueID(issue.ID) - if err != nil { - log.Error(3, "GetAttachmentsByIssueID[%d]: %v", issue.ID, err) - } - - issue.Comments, err = GetCommentsByIssueID(issue.ID) - if err != nil { - log.Error(3, "GetCommentsByIssueID[%d]: %v", issue.ID, err) - } + case "deadline_unix": + issue.Deadline = time.Unix(issue.DeadlineUnix, 0).Local() + case "created_unix": + issue.Created = time.Unix(issue.CreatedUnix, 0).Local() + case "updated_unix": + issue.Updated = time.Unix(issue.UpdatedUnix, 0).Local() + } +} - issue.Labels, err = GetLabelsByIssueID(issue.ID) +func (issue *Issue) loadAttributes(e Engine) (err error) { + if issue.Repo == nil { + issue.Repo, err = getRepositoryByID(e, issue.RepoID) if err != nil { - log.Error(3, "GetLabelsByIssueID[%d]: %v", issue.ID, err) + return fmt.Errorf("getRepositoryByID [%d]: %v", issue.RepoID, err) } + } - case "poster_id": - issue.Poster, err = GetUserByID(issue.PosterID) + if issue.Poster == nil { + issue.Poster, err = getUserByID(e, issue.PosterID) if err != nil { if IsErrUserNotExist(err) { issue.PosterID = -1 issue.Poster = NewGhostUser() } else { - log.Error(3, "GetUserByID[%d]: %v", issue.ID, err) + return fmt.Errorf("getUserByID.(poster) [%d]: %v", issue.PosterID, err) } return } + } - case "milestone_id": - if issue.MilestoneID == 0 { - return - } - - issue.Milestone, err = GetMilestoneByRepoID(issue.RepoID, issue.MilestoneID) + if issue.Labels == nil { + issue.Labels, err = getLabelsByIssueID(e, issue.ID) if err != nil { - log.Error(3, "GetMilestoneById[%d]: %v", issue.ID, err) - } - - case "assignee_id": - if issue.AssigneeID == 0 { - return + return fmt.Errorf("getLabelsByIssueID [%d]: %v", issue.ID, err) } + } - issue.Assignee, err = GetUserByID(issue.AssigneeID) + if issue.Milestone == nil && issue.MilestoneID > 0 { + issue.Milestone, err = getMilestoneByRepoID(e, issue.RepoID, issue.MilestoneID) if err != nil { - log.Error(3, "GetUserByID[%d]: %v", issue.ID, err) + return fmt.Errorf("getMilestoneByRepoID [repo_id: %d, milestone_id: %d]: %v", issue.RepoID, issue.MilestoneID, err) } - - case "deadline_unix": - issue.Deadline = time.Unix(issue.DeadlineUnix, 0).Local() - case "created_unix": - issue.Created = time.Unix(issue.CreatedUnix, 0).Local() - case "updated_unix": - issue.Updated = time.Unix(issue.UpdatedUnix, 0).Local() } -} -func (issue *Issue) loadAttributes(e Engine) (err error) { - if issue.Repo == nil { - issue.Repo, err = getRepositoryByID(e, issue.RepoID) + if issue.Assignee == nil && issue.AssigneeID > 0 { + issue.Assignee, err = getUserByID(e, issue.AssigneeID) if err != nil { - return fmt.Errorf("getRepositoryByID [%d]: %v", issue.RepoID, err) + return fmt.Errorf("getUserByID.(assignee) [%d]: %v", issue.AssigneeID, err) } } @@ -148,6 +133,20 @@ func (issue *Issue) loadAttributes(e Engine) (err error) { } } + if issue.Attachments == nil { + issue.Attachments, err = getAttachmentsByIssueID(e, issue.ID) + if err != nil { + return fmt.Errorf("getAttachmentsByIssueID [%d]: %v", issue.ID, err) + } + } + + if issue.Comments == nil { + issue.Comments, err = getCommentsByIssueID(e, issue.ID) + if err != nil { + return fmt.Errorf("getCommentsByIssueID [%d]: %v", issue.ID, err) + } + } + return nil } @@ -757,8 +756,8 @@ func GetIssueByRef(ref string) (*Issue, error) { return issue, issue.LoadAttributes() } -// GetIssueByIndex returns issue by given index in repository. -func GetIssueByIndex(repoID, index int64) (*Issue, error) { +// GetIssueByIndex returns raw issue without loading attributes by index in a repository. +func GetRawIssueByIndex(repoID, index int64) (*Issue, error) { issue := &Issue{ RepoID: repoID, Index: index, @@ -769,6 +768,15 @@ func GetIssueByIndex(repoID, index int64) (*Issue, error) { } else if !has { return nil, ErrIssueNotExist{0, repoID, index} } + return issue, nil +} + +// GetIssueByIndex returns issue by index in a repository. +func GetIssueByIndex(repoID, index int64) (*Issue, error) { + issue, err := GetRawIssueByIndex(repoID, index) + if err != nil { + return nil, err + } return issue, issue.LoadAttributes() } @@ -868,7 +876,18 @@ func Issues(opts *IssuesOptions) ([]*Issue, error) { } issues := make([]*Issue, 0, setting.UI.IssuePagingNum) - return issues, sess.Find(&issues) + if err := sess.Find(&issues); err != nil { + return nil, fmt.Errorf("Find: %v", err) + } + + // FIXME: use IssueList to improve performance. + for i := range issues { + if err := issues[i].LoadAttributes(); err != nil { + return nil, fmt.Errorf("LoadAttributes [%d]: %v", issues[i].ID, err) + } + } + + return issues, nil } // .___ ____ ___ @@ -1412,7 +1431,7 @@ func getMilestoneByRepoID(e Engine, repoID, id int64) (*Milestone, error) { return m, nil } -// GetWebhookByRepoID returns milestone of repository by given ID. +// GetWebhookByRepoID returns the milestone in a repository. func GetMilestoneByRepoID(repoID, id int64) (*Milestone, error) { return getMilestoneByRepoID(x, repoID, id) } @@ -1721,10 +1740,14 @@ func GetAttachmentByUUID(uuid string) (*Attachment, error) { return getAttachmentByUUID(x, uuid) } -// GetAttachmentsByIssueID returns all attachments for given issue by ID. -func GetAttachmentsByIssueID(issueID int64) ([]*Attachment, error) { +func getAttachmentsByIssueID(e Engine, issueID int64) ([]*Attachment, error) { attachments := make([]*Attachment, 0, 10) - return attachments, x.Where("issue_id=? AND comment_id=0", issueID).Find(&attachments) + return attachments, e.Where("issue_id = ? AND comment_id = 0", issueID).Find(&attachments) +} + +// GetAttachmentsByIssueID returns all attachments of an issue. +func GetAttachmentsByIssueID(issueID int64) ([]*Attachment, error) { + return getAttachmentsByIssueID(x, issueID) } // GetAttachmentsByCommentID returns all attachments if comment by given ID. diff --git a/models/issue_comment.go b/models/issue_comment.go index 8abc06332..c914e0a56 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -11,6 +11,7 @@ import ( "github.com/Unknwon/com" "github.com/go-xorm/xorm" + api "github.com/gogits/go-gogs-client" "github.com/gogits/gogs/modules/log" @@ -59,6 +60,8 @@ type Comment struct { Created time.Time `xorm:"-"` CreatedUnix int64 + Updated time.Time `xorm:"-"` + UpdatedUnix int64 // Reference issue in commit message CommitSHA string `xorm:"VARCHAR(40)"` @@ -71,6 +74,11 @@ type Comment struct { func (c *Comment) BeforeInsert() { c.CreatedUnix = time.Now().Unix() + c.UpdatedUnix = c.CreatedUnix +} + +func (c *Comment) BeforeUpdate() { + c.UpdatedUnix = time.Now().Unix() } func (c *Comment) AfterSet(colName string, _ xorm.Cell) { @@ -94,6 +102,8 @@ func (c *Comment) AfterSet(colName string, _ xorm.Cell) { } case "created_unix": c.Created = time.Unix(c.CreatedUnix, 0).Local() + case "updated_unix": + c.Updated = time.Unix(c.UpdatedUnix, 0).Local() } } @@ -105,16 +115,14 @@ func (c *Comment) AfterDelete() { } } -// APIFormat convert Comment struct to api.Comment struct func (c *Comment) APIFormat() *api.Comment { - apiComment := &api.Comment{ + return &api.Comment{ ID: c.ID, Poster: c.Poster.APIFormat(), Body: c.Content, Created: c.Created, + Updated: c.Updated, } - - return apiComment } // HashTag returns unique hash tag for comment. @@ -341,15 +349,32 @@ func GetCommentByID(id int64) (*Comment, error) { if err != nil { return nil, err } else if !has { - return nil, ErrCommentNotExist{id} + return nil, ErrCommentNotExist{id, 0} } return c, nil } -// GetCommentsByIssueID returns all comments of issue by given ID. -func GetCommentsByIssueID(issueID int64) ([]*Comment, error) { +func getCommentsByIssueIDSince(e Engine, issueID, since int64) ([]*Comment, error) { comments := make([]*Comment, 0, 10) - return comments, x.Where("issue_id=?", issueID).Asc("created_unix").Find(&comments) + sess := e.Where("issue_id = ?", issueID).Asc("created_unix") + if since > 0 { + sess.And("created_unix >= ?", since) + } + return comments, sess.Find(&comments) +} + +func getCommentsByIssueID(e Engine, issueID int64) ([]*Comment, error) { + return getCommentsByIssueIDSince(e, issueID, -1) +} + +// GetCommentsByIssueID returns all comments of an issue. +func GetCommentsByIssueID(issueID int64) ([]*Comment, error) { + return getCommentsByIssueID(x, issueID) +} + +// GetCommentsByIssueID returns a list of comments of an issue since a given time point. +func GetCommentsByIssueIDSince(issueID, since int64) ([]*Comment, error) { + return getCommentsByIssueIDSince(x, issueID, since) } // UpdateComment updates information of comment. @@ -358,10 +383,13 @@ func UpdateComment(c *Comment) error { return err } -// DeleteCommentByID deletes a comment by given ID. +// DeleteCommentByID deletes the comment by given ID. func DeleteCommentByID(id int64) error { comment, err := GetCommentByID(id) if err != nil { + if IsErrCommentNotExist(err) { + return nil + } return err } diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index b83f7e66b..fe967ae0a 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -25,9 +25,9 @@ func ListIssues(ctx *context.APIContext) { return } + // FIXME: use IssueList to improve performance. apiIssues := make([]*api.Issue, len(issues)) for i := range issues { - // FIXME: use IssueList to improve performance. if err = issues[i].LoadAttributes(); err != nil { ctx.Error(500, "LoadAttributes", err) return diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index c3a80f989..3957600b7 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -10,112 +10,72 @@ import ( "github.com/gogits/gogs/models" "github.com/gogits/gogs/modules/context" - "github.com/gogits/gogs/modules/log" ) -const ( - ISO8601Format = "2006-01-02T15:04:05Z" -) - -// ListIssueComments list comments on an issue func ListIssueComments(ctx *context.APIContext) { var since time.Time - var withSince bool + if len(ctx.Query("since")) > 0 { + since, _ = time.Parse(time.RFC3339, ctx.Query("since")) + } - // we get the issue instead of comments directly - // because to get comments we need to gets issue first, - // and there is already comments in the issue - issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) + // comments,err:=models.GetCommentsByIssueIDSince(, since) + issue, err := models.GetRawIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { - ctx.Error(500, "Comments", err) + ctx.Error(500, "GetRawIssueByIndex", err) return } - // parse `since`, by default we don't use `since` - if len(ctx.Query("since")) > 0 { - var err error - since, err = time.Parse(ISO8601Format, ctx.Query("since")) - if err == nil { - withSince = true - } + comments, err := models.GetCommentsByIssueIDSince(issue.ID, since.Unix()) + if err != nil { + ctx.Error(500, "GetCommentsByIssueIDSince", err) + return } - apiComments := []*api.Comment{} - for _, comment := range issue.Comments { - if withSince && !comment.Created.After(since) { - continue - } - apiComments = append(apiComments, comment.APIFormat()) + apiComments := make([]*api.Comment, len(comments)) + for i := range comments { + apiComments[i] = comments[i].APIFormat() } - ctx.JSON(200, &apiComments) } -// CreateIssueComment create comment on an issue func CreateIssueComment(ctx *context.APIContext, form api.CreateIssueCommentOption) { - // check issue issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { - ctx.Error(500, "Comments", err) + ctx.Error(500, "GetIssueByIndex", err) return } - comment := &models.Comment{ - Content: form.Body, - } - - if len(form.Body) == 0 { - ctx.Handle(400, "CreateIssueComment:empty content", err) - return - } - - // create comment - comment, err = models.CreateIssueComment(ctx.User, ctx.Repo.Repository, issue, form.Body, []string{}) + comment, err := models.CreateIssueComment(ctx.User, ctx.Repo.Repository, issue, form.Body, nil) if err != nil { - ctx.Handle(500, "CreateIssueComment", err) + ctx.Error(500, "CreateIssueComment", err) return } - log.Trace("Comment created: %d/%d/%d", ctx.Repo.Repository.ID, issue.ID, comment.ID) - - // Refetch from database to assign some automatic values - comment, err = models.GetCommentByID(comment.ID) - if err != nil { - log.Info("Failed to refetch comment:%v", err) - } ctx.JSON(201, comment.APIFormat()) } -// EditIssueComment edits an issue comment func EditIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption) { comment, err := models.GetCommentByID(ctx.ParamsInt64(":id")) if err != nil { if models.IsErrCommentNotExist(err) { ctx.Error(404, "GetCommentByID", err) } else { - ctx.Handle(500, "GetCommentByID", err) + ctx.Error(500, "GetCommentByID", err) } return } if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.IsAdmin()) { - ctx.Error(403, "edit comment", err) + ctx.Status(403) return } else if comment.Type != models.COMMENT_TYPE_COMMENT { - ctx.Error(204, "edit comment", err) + ctx.Status(204) return } comment.Content = form.Body - if len(comment.Content) == 0 { - ctx.JSON(200, map[string]interface{}{ - "content": "", - }) - return - } - if err := models.UpdateComment(comment); err != nil { - ctx.Handle(500, "UpdateComment", err) + ctx.Error(500, "UpdateComment", err) return } ctx.JSON(200, comment.APIFormat()) diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 9fe9cee06..450143112 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -166,7 +166,6 @@ func Issues(ctx *context.Context) { pager := paginater.New(total, setting.UI.IssuePagingNum, page, 5) ctx.Data["Page"] = pager - // Get issues. issues, err := models.Issues(&models.IssuesOptions{ UserID: uid, AssigneeID: assigneeID, diff --git a/templates/.VERSION b/templates/.VERSION index 98448cd5a..179fe9808 100644 --- a/templates/.VERSION +++ b/templates/.VERSION @@ -1 +1 @@ -0.9.85.0824 \ No newline at end of file +0.9.86.0826 \ No newline at end of file