From 1d65f8cad52b0d5d63aeb9051336c3b3b3d6b536 Mon Sep 17 00:00:00 2001 From: Harshit Bansal Date: Wed, 2 Jan 2019 23:36:08 +0530 Subject: [PATCH 1/2] issue: Don't close issues via commits on non-default branch. Adds a small check to close the issues only if the referencing commits are on the default branch. Fixes: #2314. --- models/action.go | 9 +++++++-- models/action_test.go | 29 ++++++++++++++++++++++++++++- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/models/action.go b/models/action.go index cde0fb080eba7..316313d35addc 100644 --- a/models/action.go +++ b/models/action.go @@ -477,7 +477,7 @@ func getIssueFromRef(repo *Repository, ref string) (*Issue, error) { } // 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,6 +500,11 @@ 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) { @@ -609,7 +614,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) } } diff --git a/models/action_test.go b/models/action_test.go index d0e0a5d8fab1f..0310b0ad5d074 100644 --- a/models/action_test.go +++ b/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) { From 8660b18901b0a2e23b4f158ffc0879f6868e60e9 Mon Sep 17 00:00:00 2001 From: Harshit Bansal Date: Thu, 3 Jan 2019 00:30:03 +0530 Subject: [PATCH 2/2] refactor: Extract `changeIssueStatus()` function. --- models/action.go | 65 +++++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 37 deletions(-) diff --git a/models/action.go b/models/action.go index 316313d35addc..d917c3774614f 100644 --- a/models/action.go +++ b/models/action.go @@ -476,6 +476,32 @@ 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, branchName string) error { // Commits are appended in the reverse order. @@ -506,50 +532,15 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, bra } 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 } }