Don't close issues via commits on non-default branch. (#5622)

Adds a small check to close the issues only if the referencing commits
are on the default branch.

Fixes: #2314.

models/action.go

@@ -476,59 +476,40 @@ func getIssueFromRef(repo *Repository, ref string) (*Issue, error) {
 	return issue, nil
 }
 
-// UpdateIssuesCommit checks if issues are manipulated by commit message.
-func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit) error {
-	// Commits are appended in the reverse order.
-	for i := len(commits) - 1; i >= 0; i-- {
-		c := commits[i]
-
-		refMarked := make(map[int64]bool)
-		for _, ref := range issueReferenceKeywordsPat.FindAllString(c.Message, -1) {
+func changeIssueStatus(repo *Repository, doer *User, ref string, refMarked map[int64]bool, status bool) error {
 	issue, err := getIssueFromRef(repo, ref)
 	if err != nil {
 		return err
 	}
 
 	if issue == nil || refMarked[issue.ID] {
-				continue
-			}
-			refMarked[issue.ID] = true
-
-			message := fmt.Sprintf(`<a href="%s/commit/%s">%s</a>`, repo.Link(), c.Sha1, c.Message)
-			if err = CreateRefComment(doer, repo, issue, message, c.Sha1); err != nil {
-				return err
-			}
-		}
-
-		refMarked = make(map[int64]bool)
-		// FIXME: can merge this one and next one to a common function.
-		for _, ref := range issueCloseKeywordsPat.FindAllString(c.Message, -1) {
-			issue, err := getIssueFromRef(repo, ref)
-			if err != nil {
-				return err
-			}
-
-			if issue == nil || refMarked[issue.ID] {
-				continue
+		return nil
 	}
 	refMarked[issue.ID] = true
 
-			if issue.RepoID != repo.ID || issue.IsClosed {
-				continue
+	if issue.RepoID != repo.ID || issue.IsClosed == status {
+		return nil
 	}
 
 	issue.Repo = repo
-			if err = issue.ChangeStatus(doer, true); err != nil {
+	if err = issue.ChangeStatus(doer, status); err != nil {
 		// Don't return an error when dependencies are open as this would let the push fail
 		if IsErrDependenciesLeft(err) {
 			return nil
 		}
 		return err
 	}
+	return nil
 }
 
-		// It is conflict to have close and reopen at same time, so refsMarked doesn't need to reinit here.
-		for _, ref := range issueReopenKeywordsPat.FindAllString(c.Message, -1) {
+// UpdateIssuesCommit checks if issues are manipulated by commit message.
+func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, branchName string) error {
+	// Commits are appended in the reverse order.
+	for i := len(commits) - 1; i >= 0; i-- {
+		c := commits[i]
+
+		refMarked := make(map[int64]bool)
+		for _, ref := range issueReferenceKeywordsPat.FindAllString(c.Message, -1) {
 			issue, err := getIssueFromRef(repo, ref)
 			if err != nil {
 				return err
@@ -539,12 +520,27 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit) err
 			}
 			refMarked[issue.ID] = true
 
-			if issue.RepoID != repo.ID || !issue.IsClosed {
+			message := fmt.Sprintf(`<a href="%s/commit/%s">%s</a>`, repo.Link(), c.Sha1, c.Message)
+			if err = CreateRefComment(doer, repo, issue, message, c.Sha1); err != nil {
+				return err
+			}
+		}
+
+		// Change issue status only if the commit has been pushed to the default branch.
+		if repo.DefaultBranch != branchName {
 			continue
 		}
 
-			issue.Repo = repo
-			if err = issue.ChangeStatus(doer, false); err != nil {
+		refMarked = make(map[int64]bool)
+		for _, ref := range issueCloseKeywordsPat.FindAllString(c.Message, -1) {
+			if err := changeIssueStatus(repo, doer, ref, refMarked, true); err != nil {
+				return err
+			}
+		}
+
+		// It is conflict to have close and reopen at same time, so refsMarked doesn't need to reinit here.
+		for _, ref := range issueReopenKeywordsPat.FindAllString(c.Message, -1) {
+			if err := changeIssueStatus(repo, doer, ref, refMarked, false); err != nil {
 				return err
 			}
 		}
@@ -609,7 +605,7 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
 			opts.Commits.CompareURL = repo.ComposeCompareURL(opts.OldCommitID, opts.NewCommitID)
 		}
 
-		if err = UpdateIssuesCommit(pusher, repo, opts.Commits.Commits); err != nil {
+		if err = UpdateIssuesCommit(pusher, repo, opts.Commits.Commits, refName); err != nil {
 			log.Error(4, "updateIssuesCommit: %v", err)
 		}
 	}

models/action_test.go

@@ -227,10 +227,37 @@ func TestUpdateIssuesCommit(t *testing.T) {
 
 	AssertNotExistsBean(t, commentBean)
 	AssertNotExistsBean(t, &Issue{RepoID: repo.ID, Index: 2}, "is_closed=1")
-	assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits))
+	assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch))
 	AssertExistsAndLoadBean(t, commentBean)
 	AssertExistsAndLoadBean(t, issueBean, "is_closed=1")
 	CheckConsistencyFor(t, &Action{})
+
+	// Test that push to a non-default branch closes no issue.
+	pushCommits = []*PushCommit{
+		{
+			Sha1:           "abcdef1",
+			CommitterEmail: "user2@example.com",
+			CommitterName:  "User Two",
+			AuthorEmail:    "user4@example.com",
+			AuthorName:     "User Four",
+			Message:        "close #1",
+		},
+	}
+	repo = AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
+	commentBean = &Comment{
+		Type:      CommentTypeCommitRef,
+		CommitSHA: "abcdef1",
+		PosterID:  user.ID,
+		IssueID:   6,
+	}
+	issueBean = &Issue{RepoID: repo.ID, Index: 1}
+
+	AssertNotExistsBean(t, commentBean)
+	AssertNotExistsBean(t, &Issue{RepoID: repo.ID, Index: 1}, "is_closed=1")
+	assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, "non-existing-branch"))
+	AssertExistsAndLoadBean(t, commentBean)
+	AssertNotExistsBean(t, issueBean, "is_closed=1")
+	CheckConsistencyFor(t, &Action{})
 }
 
 func testCorrectRepoAction(t *testing.T, opts CommitRepoActionOptions, actionBean *Action) {