#2966 code cleanup

11 files changed, 138 insertions, 127 deletions

diff --git a/.gopmfile b/.gopmfile
index 0ea9eed96d..ab1550b9e4 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 6e71030c6b..c3b250e73e 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 a64dde1d87..f5bf62ed64 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 acc7b54569..1874c4d0a5 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 89c6389b0d..240e606776 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 1446056e5d..a90ebaf866 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 8abc063328..c914e0a56f 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 b83f7e66b9..fe967ae0a5 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 c3a80f989c..3957600b7b 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 9fe9cee06d..450143112f 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 98448cd5a0..179fe98081 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