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

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

Fixes: #2314.

models/action.go

@@ -476,8 +476,34 @@ func getIssueFromRef(repo *Repository, ref string) (*Issue, error) {
  return issue, nil
 }
 
+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] {
+ return nil
+ }
+ refMarked[issue.ID] = true
+
+ if issue.RepoID != repo.ID || issue.IsClosed == status {
+ return nil
+ }
+
+ issue.Repo = repo
+ 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
+}
+
 // UpdateIssuesCommit checks if issues are manipulated by commit message.
-func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit) error {
+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]
@@ -500,51 +526,21 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit) err
  }
  }
 
+ // Change issue status only if the commit has been pushed to the default branch.
+ if repo.DefaultBranch != branchName {
+ continue
+ }
+
  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
- }
- refMarked[issue.ID] = true
-
- if issue.RepoID != repo.ID || issue.IsClosed {
- continue
- }
-
- issue.Repo = repo
- if err = issue.ChangeStatus(doer, true); err != nil {
- // Don't return an error when dependencies are open as this would let the push fail
- if IsErrDependenciesLeft(err) {
- return nil
- }
+ 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) {
- issue, err := getIssueFromRef(repo, ref)
- if err != nil {
- return err
- }
-
- if issue == nil || refMarked[issue.ID] {
- continue
- }
- refMarked[issue.ID] = true
-
- if issue.RepoID != repo.ID || !issue.IsClosed {
- continue
- }
-
- issue.Repo = repo
- if err = issue.ChangeStatus(doer, false); err != nil {
+ 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: "[email protected]",
+ CommitterName: "User Two",
+ AuthorEmail: "[email protected]",
+ 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) {