#1545 prevent duplicated refs of issues in single commit

models/action.go

@@ -183,14 +183,17 @@ func RenameRepoAction(actUser *User, oldRepoName string, repo *Repository) error
 	return renameRepoAction(x, actUser, oldRepoName, repo)
 }
 
+func issueIndexTrimRight(c rune) bool {
+	return !unicode.IsDigit(c)
+}
+
 // updateIssuesCommit checks if issues are manipulated by commit message.
 func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string, commits []*base.PushCommit) error {
 	for _, c := range commits {
+		refMarked := make(map[int64]bool)
 		for _, ref := range IssueReferenceKeywordsPat.FindAllString(c.Message, -1) {
-			ref := ref[strings.IndexByte(ref, byte(' '))+1:]
-			ref = strings.TrimRightFunc(ref, func(c rune) bool {
-				return !unicode.IsDigit(c)
-			})
+			ref = ref[strings.IndexByte(ref, byte(' '))+1:]
+			ref = strings.TrimRightFunc(ref, issueIndexTrimRight)
 
 			if len(ref) == 0 {
 				continue
@@ -199,10 +202,9 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 			// Add repo name if missing
 			if ref[0] == '#' {
 				ref = fmt.Sprintf("%s/%s%s", repoUserName, repoName, ref)
-			} else if strings.Contains(ref, "/") == false {
+			} else if !strings.Contains(ref, "/") {
 				// FIXME: We don't support User#ID syntax yet
 				// return ErrNotImplemented
-
 				continue
 			}
 
@@ -211,6 +213,11 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 				return err
 			}
 
+			if refMarked[issue.ID] {
+				continue
+			}
+			refMarked[issue.ID] = true
+
 			url := fmt.Sprintf("%s/%s/%s/commit/%s", setting.AppSubUrl, repoUserName, repoName, c.Sha1)
 			message := fmt.Sprintf(`<a href="%s">%s</a>`, url, c.Message)
 			if _, err = CreateComment(u, repo, issue, 0, 0, COMMENT_TYPE_COMMIT_REF, message, nil); err != nil {
@@ -218,11 +225,10 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 			}
 		}
 
+		refMarked = make(map[int64]bool)
 		for _, ref := range IssueCloseKeywordsPat.FindAllString(c.Message, -1) {
-			ref := ref[strings.IndexByte(ref, byte(' '))+1:]
-			ref = strings.TrimRightFunc(ref, func(c rune) bool {
-				return !unicode.IsDigit(c)
-			})
+			ref = ref[strings.IndexByte(ref, byte(' '))+1:]
+			ref = strings.TrimRightFunc(ref, issueIndexTrimRight)
 
 			if len(ref) == 0 {
 				continue
@@ -234,7 +240,6 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 			} else if strings.Contains(ref, "/") == false {
 				// We don't support User#ID syntax yet
 				// return ErrNotImplemented
-
 				continue
 			}
 
@@ -243,45 +248,24 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 				return err
 			}
 
-			if issue.RepoID == repo.ID {
-				if issue.IsClosed {
-					continue
-				}
-				issue.IsClosed = true
-
-				if err = issue.GetLabels(); err != nil {
-					return err
-				}
-				for _, label := range issue.Labels {
-					label.NumClosedIssues++
-
-					if err = UpdateLabel(label); err != nil {
-						return err
-					}
-				}
-
-				if err = UpdateIssue(issue); err != nil {
-					return err
-				} else if err = UpdateIssueUsersByStatus(issue.ID, issue.IsClosed); err != nil {
-					return err
-				}
-
-				if err = ChangeMilestoneIssueStats(issue); err != nil {
-					return err
-				}
-
-				// If commit happened in the referenced repository, it means the issue can be closed.
-				if _, err = CreateComment(u, repo, issue, 0, 0, COMMENT_TYPE_CLOSE, "", nil); err != nil {
-					return err
-				}
+			if refMarked[issue.ID] {
+				continue
+			}
+			refMarked[issue.ID] = true
+
+			if issue.RepoID != repo.ID || issue.IsClosed {
+				continue
+			}
+
+			if err = issue.ChangeStatus(u, true); err != nil {
+				return err
 			}
 		}
 
+		// It is conflict to have close and reopen at same time, so refsMarkd doesn't need to reinit here.
 		for _, ref := range IssueReopenKeywordsPat.FindAllString(c.Message, -1) {
-			ref := ref[strings.IndexByte(ref, byte(' '))+1:]
-			ref = strings.TrimRightFunc(ref, func(c rune) bool {
-				return !unicode.IsDigit(c)
-			})
+			ref = ref[strings.IndexByte(ref, byte(' '))+1:]
+			ref = strings.TrimRightFunc(ref, issueIndexTrimRight)
 
 			if len(ref) == 0 {
 				continue
@@ -293,7 +277,6 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 			} else if strings.Contains(ref, "/") == false {
 				// We don't support User#ID syntax yet
 				// return ErrNotImplemented
-
 				continue
 			}
 
@@ -302,37 +285,17 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 				return err
 			}
 
-			if issue.RepoID == repo.ID {
-				if !issue.IsClosed {
-					continue
-				}
-				issue.IsClosed = false
-
-				if err = issue.GetLabels(); err != nil {
-					return err
-				}
-				for _, label := range issue.Labels {
-					label.NumClosedIssues--
-
-					if err = UpdateLabel(label); err != nil {
-						return err
-					}
-				}
-
-				if err = UpdateIssue(issue); err != nil {
-					return err
-				} else if err = UpdateIssueUsersByStatus(issue.ID, issue.IsClosed); err != nil {
-					return err
-				}
-
-				if err = ChangeMilestoneIssueStats(issue); err != nil {
-					return err
-				}
-
-				// If commit happened in the referenced repository, it means the issue can be closed.
-				if _, err = CreateComment(u, repo, issue, 0, 0, COMMENT_TYPE_REOPEN, "", nil); err != nil {
-					return err
-				}
+			if refMarked[issue.ID] {
+				continue
+			}
+			refMarked[issue.ID] = true
+
+			if issue.RepoID != repo.ID || !issue.IsClosed {
+				continue
+			}
+
+			if err = issue.ChangeStatus(u, false); err != nil {
+				return err
 			}
 		}
 	}

models/issue.go

@@ -13,7 +13,6 @@ import (
 	"mime/multipart"
 	"os"
 	"path"
-	"strconv"
 	"strings"
 	"time"
 
@@ -384,25 +383,29 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string)
 
 // GetIssueByRef returns an Issue specified by a GFM reference.
 // See https://help.github.com/articles/writing-on-github#references for more information on the syntax.
-func GetIssueByRef(ref string) (issue *Issue, err error) {
-	var issueNumber int64
-	var repo *Repository
-
+func GetIssueByRef(ref string) (*Issue, error) {
 	n := strings.IndexByte(ref, byte('#'))
-
 	if n == -1 {
 		return nil, ErrMissingIssueNumber
 	}
 
-	if issueNumber, err = strconv.ParseInt(ref[n+1:], 10, 64); err != nil {
-		return
+	index, err := com.StrTo(ref[n+1:]).Int64()
+	if err != nil {
+		return nil, err
 	}
 
-	if repo, err = GetRepositoryByRef(ref[:n]); err != nil {
-		return
+	repo, err := GetRepositoryByRef(ref[:n])
+	if err != nil {
+		return nil, err
 	}
 
-	return GetIssueByIndex(repo.ID, issueNumber)
+	issue, err := GetIssueByIndex(repo.ID, index)
+	if err != nil {
+		return nil, err
+	}
+
+	issue.Repo = repo
+	return issue, nil
 }
 
 // GetIssueByIndex returns issue by given index in repository.

models/repo.go

@@ -1105,15 +1105,12 @@ func DeleteRepository(uid, repoID int64) error {
 // See https://help.github.com/articles/writing-on-github#references for more information on the syntax.
 func GetRepositoryByRef(ref string) (*Repository, error) {
 	n := strings.IndexByte(ref, byte('/'))
-
 	if n < 2 {
 		return nil, ErrInvalidReference
 	}
 
 	userName, repoName := ref[:n], ref[n+1:]
-
 	user, err := GetUserByName(userName)
-
 	if err != nil {
 		return nil, err
 	}