From d6ff2012ee7c503147f736a048cfcc331d78bf94 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Mon, 19 Mar 2018 12:25:42 -0400 Subject: [PATCH 01/32] Update issues when a pull request is merged --- models/action.go | 18 +++++++++++++----- models/action_test.go | 3 ++- models/pull.go | 11 ++++++----- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/models/action.go b/models/action.go index 9050be22a6403..1d7497808444b 100644 --- a/models/action.go +++ b/models/action.go @@ -724,8 +724,12 @@ func TransferRepoAction(doer, oldOwner *User, repo *Repository) error { return transferRepoAction(x, doer, oldOwner, repo) } -func mergePullRequestAction(e Engine, doer *User, repo *Repository, issue *Issue) error { - return notifyWatchers(e, &Action{ +func mergePullRequestAction(e Engine, doer *User, repo *Repository, issue *Issue, commits *PushCommits) (err error) { + if err = UpdateIssuesCommit(doer, repo, commits.Commits); err != nil { + log.Error(4, "updateIssuesCommit: %v", err) + } + + if err = notifyWatchers(e, &Action{ ActUserID: doer.ID, ActUser: doer, OpType: ActionMergePullRequest, @@ -733,12 +737,16 @@ func mergePullRequestAction(e Engine, doer *User, repo *Repository, issue *Issue RepoID: repo.ID, Repo: repo, IsPrivate: repo.IsPrivate, - }) + }); err != nil { + return fmt.Errorf("notifyWatchers: %v", err) + } + + return nil } // MergePullRequestAction adds new action for merging pull request. -func MergePullRequestAction(actUser *User, repo *Repository, pull *Issue) error { - return mergePullRequestAction(x, actUser, repo, pull) +func MergePullRequestAction(actUser *User, repo *Repository, pull *Issue, commits *PushCommits) error { + return mergePullRequestAction(x, actUser, repo, pull, commits) } func mirrorSyncAction(e Engine, opType ActionType, repo *Repository, refName string, data []byte) error { diff --git a/models/action_test.go b/models/action_test.go index d0e0a5d8fab1f..5c0c7a3e3f061 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -379,6 +379,7 @@ func TestMergePullRequestAction(t *testing.T) { repo := AssertExistsAndLoadBean(t, &Repository{ID: 1, OwnerID: user.ID}).(*Repository) repo.Owner = user issue := AssertExistsAndLoadBean(t, &Issue{ID: 3, RepoID: repo.ID}).(*Issue) + commits := &PushCommits{0, make([]*PushCommit, 0), "", nil} actionBean := &Action{ OpType: ActionMergePullRequest, @@ -389,7 +390,7 @@ func TestMergePullRequestAction(t *testing.T) { IsPrivate: repo.IsPrivate, } AssertNotExistsBean(t, actionBean) - assert.NoError(t, MergePullRequestAction(user, repo, issue)) + assert.NoError(t, MergePullRequestAction(user, repo, issue, commits)) AssertExistsAndLoadBean(t, actionBean) CheckConsistencyFor(t, &Action{}) } diff --git a/models/pull.go b/models/pull.go index 0041f83dc820c..ef1e03b380699 100644 --- a/models/pull.go +++ b/models/pull.go @@ -452,10 +452,6 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository, mergeStyle log.Error(4, "setMerged [%d]: %v", pr.ID, err) } - if err = MergePullRequestAction(doer, pr.Issue.Repo, pr.Issue); err != nil { - log.Error(4, "MergePullRequestAction [%d]: %v", pr.ID, err) - } - // Reset cached commit count cache.Remove(pr.Issue.Repo.GetCommitsCountCacheKey(pr.BaseBranch, true)) @@ -495,13 +491,18 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository, mergeStyle if mergeStyle == MergeStyleMerge { l.PushFront(mergeCommit) } + commits := ListToPushCommits(l) + + if err = MergePullRequestAction(doer, pr.Issue.Repo, pr.Issue, commits); err != nil { + log.Error(4, "MergePullRequestAction [%d]: %v", pr.ID, err) + } p := &api.PushPayload{ Ref: git.BranchPrefix + pr.BaseBranch, Before: pr.MergeBase, After: mergeCommit.ID.String(), CompareURL: setting.AppURL + pr.BaseRepo.ComposeCompareURL(pr.MergeBase, pr.MergedCommitID), - Commits: ListToPushCommits(l).ToAPIPayloadCommits(pr.BaseRepo.HTMLURL()), + Commits: commits.ToAPIPayloadCommits(pr.BaseRepo.HTMLURL()), Repo: pr.BaseRepo.APIFormat(mode), Pusher: pr.HeadRepo.MustOwner().APIFormat(), Sender: doer.APIFormat(), From 224f0fd21e6f7ac5e1f1aa5c72da6b426837ef65 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Tue, 20 Mar 2018 20:23:42 -0400 Subject: [PATCH 02/32] Factor out detection of reference, close, reopen keywords into a more reusable function --- models/action.go | 127 +++++++++++++++++++++++++++-------------------- 1 file changed, 72 insertions(+), 55 deletions(-) diff --git a/models/action.go b/models/action.go index 1d7497808444b..28604142aa251 100644 --- a/models/action.go +++ b/models/action.go @@ -52,14 +52,29 @@ const ( ActionMirrorSyncDelete // 20 ) +// KeywordsFoundMaskType represents the bitmask of types of keywords found in a message. +type KeywordsFoundMaskType int + +// Possible bitmask types for keywords that can be found. +const ( + KeywordsFoundReference KeywordsFoundMaskType = 1 << iota // 1 + KeywordsFoundReopen // 2 + KeywordsFoundClose // 4 +) + +// IssueKeywordsToFind represents a pairing of a pattern to use to find keywords in message and the keywords bitmask value. +type IssueKeywordsToFind struct { + Pattern *regexp.Regexp + KeywordsFoundMask KeywordsFoundMaskType +} + var ( // Same as Github. See // https://help.github.com/articles/closing-issues-via-commit-messages issueCloseKeywords = []string{"close", "closes", "closed", "fix", "fixes", "fixed", "resolve", "resolves", "resolved"} issueReopenKeywords = []string{"reopen", "reopens", "reopened"} - issueCloseKeywordsPat, issueReopenKeywordsPat *regexp.Regexp - issueReferenceKeywordsPat *regexp.Regexp + issueKeywordsToFind []*IssueKeywordsToFind ) const issueRefRegexpStr = `(?:\S+/\S=)?#\d+` @@ -69,9 +84,21 @@ func assembleKeywordsPattern(words []string) string { } func init() { - issueCloseKeywordsPat = regexp.MustCompile(assembleKeywordsPattern(issueCloseKeywords)) - issueReopenKeywordsPat = regexp.MustCompile(assembleKeywordsPattern(issueReopenKeywords)) - issueReferenceKeywordsPat = regexp.MustCompile(issueRefRegexpStr) + // populate with details to find keywords for reference, reopen, close + issueKeywordsToFind = []*IssueKeywordsToFind{ + &IssueKeywordsToFind{ + Pattern: regexp.MustCompile(issueRefRegexpStr), + KeywordsFoundMask: KeywordsFoundReference, + }, + &IssueKeywordsToFind{ + Pattern: regexp.MustCompile(assembleKeywordsPattern(issueReopenKeywords)), + KeywordsFoundMask: KeywordsFoundReopen, + }, + &IssueKeywordsToFind{ + Pattern: regexp.MustCompile(assembleKeywordsPattern(issueCloseKeywords)), + KeywordsFoundMask: KeywordsFoundClose, + }, + } } // Action represents user operation type and other information to @@ -438,74 +465,64 @@ 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) { +// findIssueReferencesInString iterates over the keywords to find in a message and accumulates the findings into refs +func findIssueReferencesInString(message string, repo *Repository) (map[int64]KeywordsFoundMaskType, error) { + refs := make(map[int64]KeywordsFoundMaskType) + for _, kwToFind := range issueKeywordsToFind { + for _, ref := range kwToFind.Pattern.FindAllString(message, -1) { issue, err := getIssueFromRef(repo, ref) if err != nil { - return err - } - - if issue == nil || refMarked[issue.ID] { - continue + return nil, err } - refMarked[issue.ID] = true - message := fmt.Sprintf(`%s`, repo.Link(), c.Sha1, c.Message) - if err = CreateRefComment(doer, repo, issue, message, c.Sha1); err != nil { - return err + if issue != nil { + refs[issue.ID] |= kwToFind.KeywordsFoundMask } } + } + return refs, nil +} - 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 - } +// 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] - if err = issue.ChangeStatus(doer, repo, true); 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 - } + refs, err := findIssueReferencesInString(c.Message, repo) + if 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) + for id, mask := range refs { + issue, err := GetIssueByID(id) if err != nil { return err } - - if issue == nil || refMarked[issue.ID] { + if issue == nil { continue } - refMarked[issue.ID] = true - if issue.RepoID != repo.ID || !issue.IsClosed { - continue + if (mask & KeywordsFoundReference) == KeywordsFoundReference { + message := fmt.Sprintf(`%s`, repo.Link(), c.Sha1, c.Message) + if err = CreateRefComment(doer, repo, issue, message, c.Sha1); err != nil { + return err + } } - if err = issue.ChangeStatus(doer, repo, false); err != nil { - return err + // take no action if both KeywordsFoundClose and KeywordsFoundOpen are set + if (mask & (KeywordsFoundReopen|KeywordsFoundClose)) == KeywordsFoundClose { + if issue.RepoID == repo.ID && !issue.IsClosed { + if err = issue.ChangeStatus(doer, repo, true); err != nil { + return err + } + } + } else if (mask & (KeywordsFoundReopen|KeywordsFoundClose)) == KeywordsFoundReopen { + if issue.RepoID == repo.ID && issue.IsClosed { + if err = issue.ChangeStatus(doer, repo, false); err != nil { + return err + } + } } } } From d418d9240d75c65931b7d7f5c53675844768a39e Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Fri, 23 Mar 2018 09:48:23 -0400 Subject: [PATCH 03/32] Only change issue status when commits have been merged --- models/action.go | 38 ++++++++++++++++++++------------------ models/action_test.go | 2 +- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/models/action.go b/models/action.go index 28604142aa251..9e8806aeaec38 100644 --- a/models/action.go +++ b/models/action.go @@ -484,7 +484,7 @@ func findIssueReferencesInString(message string, repo *Repository) (map[int64]Ke } // 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, commitsAreMerged bool) error { // Commits are appended in the reverse order. for i := len(commits) - 1; i >= 0; i-- { c := commits[i] @@ -510,17 +510,19 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit) err } } - // take no action if both KeywordsFoundClose and KeywordsFoundOpen are set - if (mask & (KeywordsFoundReopen|KeywordsFoundClose)) == KeywordsFoundClose { - if issue.RepoID == repo.ID && !issue.IsClosed { - if err = issue.ChangeStatus(doer, repo, true); err != nil { - return err + if commitsAreMerged { + // take no action if both KeywordsFoundClose and KeywordsFoundOpen are set + if (mask & (KeywordsFoundReopen|KeywordsFoundClose)) == KeywordsFoundClose { + if issue.RepoID == repo.ID && !issue.IsClosed { + if err = issue.ChangeStatus(doer, repo, true); err != nil { + return err + } } - } - } else if (mask & (KeywordsFoundReopen|KeywordsFoundClose)) == KeywordsFoundReopen { - if issue.RepoID == repo.ID && issue.IsClosed { - if err = issue.ChangeStatus(doer, repo, false); err != nil { - return err + } else if (mask & (KeywordsFoundReopen|KeywordsFoundClose)) == KeywordsFoundReopen { + if issue.RepoID == repo.ID && issue.IsClosed { + if err = issue.ChangeStatus(doer, repo, false); err != nil { + return err + } } } } @@ -586,7 +588,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, true); err != nil { log.Error(4, "updateIssuesCommit: %v", err) } } @@ -741,16 +743,12 @@ func TransferRepoAction(doer, oldOwner *User, repo *Repository) error { return transferRepoAction(x, doer, oldOwner, repo) } -func mergePullRequestAction(e Engine, doer *User, repo *Repository, issue *Issue, commits *PushCommits) (err error) { - if err = UpdateIssuesCommit(doer, repo, commits.Commits); err != nil { - log.Error(4, "updateIssuesCommit: %v", err) - } - +func mergePullRequestAction(e Engine, doer *User, repo *Repository, pull *Issue, commits *PushCommits) (err error) { if err = notifyWatchers(e, &Action{ ActUserID: doer.ID, ActUser: doer, OpType: ActionMergePullRequest, - Content: fmt.Sprintf("%d|%s", issue.Index, issue.Title), + Content: fmt.Sprintf("%d|%s", pull.Index, pull.Title), RepoID: repo.ID, Repo: repo, IsPrivate: repo.IsPrivate, @@ -763,6 +761,10 @@ func mergePullRequestAction(e Engine, doer *User, repo *Repository, issue *Issue // MergePullRequestAction adds new action for merging pull request. func MergePullRequestAction(actUser *User, repo *Repository, pull *Issue, commits *PushCommits) error { + if err := UpdateIssuesCommit(actUser, repo, commits.Commits, true); err != nil { + log.Error(4, "updateIssuesCommit: %v", err) + } + return mergePullRequestAction(x, actUser, repo, pull, commits) } diff --git a/models/action_test.go b/models/action_test.go index 5c0c7a3e3f061..519cba0b5cfd9 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -227,7 +227,7 @@ 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, true)) AssertExistsAndLoadBean(t, commentBean) AssertExistsAndLoadBean(t, issueBean, "is_closed=1") CheckConsistencyFor(t, &Action{}) From 06fcda97dffd3f28ffcef27e10b8f0ec84bea923 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Fri, 23 Mar 2018 10:07:48 -0400 Subject: [PATCH 04/32] Add issue updating from commit messages in new pull request --- models/action.go | 27 ++++++++++++++++++++++++++ models/pull.go | 50 ++++++++++++++++++++++++++++++++++-------------- 2 files changed, 63 insertions(+), 14 deletions(-) diff --git a/models/action.go b/models/action.go index 9e8806aeaec38..e741d30c99f24 100644 --- a/models/action.go +++ b/models/action.go @@ -768,6 +768,33 @@ func MergePullRequestAction(actUser *User, repo *Repository, pull *Issue, commit return mergePullRequestAction(x, actUser, repo, pull, commits) } +func newPullRequestAction(e Engine, doer *User, repo *Repository, pull *Issue, commits *PushCommits) (err error) { + if err = NotifyWatchers(&Action{ + ActUserID: doer.ID, + ActUser: doer, + OpType: ActionCreatePullRequest, + Content: fmt.Sprintf("%d|%s", pull.Index, pull.Title), + RepoID: repo.ID, + Repo: repo, + IsPrivate: repo.IsPrivate, + }); err != nil { + log.Error(4, "NotifyWatchers: %v", err) + } else if err = pull.MailParticipants(); err != nil { + log.Error(4, "MailParticipants: %v", err) + } + + return nil +} + +// NewPullRequestAction adds new action for merging pull request. +func NewPullRequestAction(actUser *User, repo *Repository, pull *Issue, commits *PushCommits) error { + if err := UpdateIssuesCommit(actUser, repo, commits.Commits, false); err != nil { + log.Error(4, "updateIssuesCommit: %v", err) + } + + return newPullRequestAction(x, actUser, repo, pull, commits) +} + func mirrorSyncAction(e Engine, opType ActionType, repo *Repository, refName string, data []byte) error { if err := notifyWatchers(e, &Action{ ActUserID: repo.OwnerID, diff --git a/models/pull.go b/models/pull.go index ef1e03b380699..e7eb3c8ccc8a8 100644 --- a/models/pull.go +++ b/models/pull.go @@ -779,23 +779,45 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str UpdateIssueIndexer(pull.ID) - if err = NotifyWatchers(&Action{ - ActUserID: pull.Poster.ID, - ActUser: pull.Poster, - OpType: ActionCreatePullRequest, - Content: fmt.Sprintf("%d|%s", pull.Index, pull.Title), - RepoID: repo.ID, - Repo: repo, - IsPrivate: repo.IsPrivate, - }); err != nil { - log.Error(4, "NotifyWatchers: %v", err) - } else if err = pull.MailParticipants(); err != nil { - log.Error(4, "MailParticipants: %v", err) - } - pr.Issue = pull pull.PullRequest = pr mode, _ := AccessLevel(pull.Poster, repo) + + var ( + baseBranch *Branch + headBranch *Branch + baseCommit *git.Commit + headCommit *git.Commit + baseGitRepo *git.Repository + ) + if baseBranch, err = pr.BaseRepo.GetBranch(pr.BaseBranch); err != nil { + return nil + } + if baseCommit, err = baseBranch.GetCommit(); err != nil { + return nil + } + if headBranch, err = pr.HeadRepo.GetBranch(pr.HeadBranch); err != nil { + return nil + } + if headCommit, err = headBranch.GetCommit(); err != nil { + return nil + } + if baseGitRepo, err = git.OpenRepository(pr.BaseRepo.RepoPath()); err != nil { + log.Error(4, "OpenRepository", err) + return nil + } + + l, err := baseGitRepo.CommitsBetweenIDs(headCommit.ID.String(), baseCommit.ID.String()) + if err != nil { + log.Error(4, "CommitsBetweenIDs: %v", err) + return nil + } + + commits := ListToPushCommits(l) + if err = NewPullRequestAction(pull.Poster, repo, pull, commits); err != nil { + log.Error(4, "NewPullRequestAction [%d]: %v", pr.ID, err) + } + if err = PrepareWebhooks(repo, HookEventPullRequest, &api.PullRequestPayload{ Action: api.HookIssueOpened, Index: pull.Index, From c1dac9c2f7c18baff194ca7a0d0d7291cf0aa7b7 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Tue, 27 Mar 2018 12:53:29 -0400 Subject: [PATCH 05/32] Clean up {Merge,New}PullRequestAction and add UpdatePullRequestAction --- models/action.go | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/models/action.go b/models/action.go index e741d30c99f24..20faed28f5ab4 100644 --- a/models/action.go +++ b/models/action.go @@ -589,7 +589,7 @@ func CommitRepoAction(opts CommitRepoActionOptions) error { } if err = UpdateIssuesCommit(pusher, repo, opts.Commits.Commits, true); err != nil { - log.Error(4, "updateIssuesCommit: %v", err) + log.Error(4, "UpdateIssuesCommit: %v", err) } } @@ -743,8 +743,13 @@ func TransferRepoAction(doer, oldOwner *User, repo *Repository) error { return transferRepoAction(x, doer, oldOwner, repo) } -func mergePullRequestAction(e Engine, doer *User, repo *Repository, pull *Issue, commits *PushCommits) (err error) { - if err = notifyWatchers(e, &Action{ +// MergePullRequestAction adds new action for merging pull request. +func MergePullRequestAction(doer *User, repo *Repository, pull *Issue, commits *PushCommits) error { + if err := UpdateIssuesCommit(doer, repo, commits.Commits, true); err != nil { + log.Error(4, "UpdateIssuesCommit: %v", err) + } + + if err := notifyWatchers(x, &Action{ ActUserID: doer.ID, ActUser: doer, OpType: ActionMergePullRequest, @@ -759,17 +764,13 @@ func mergePullRequestAction(e Engine, doer *User, repo *Repository, pull *Issue, return nil } -// MergePullRequestAction adds new action for merging pull request. -func MergePullRequestAction(actUser *User, repo *Repository, pull *Issue, commits *PushCommits) error { - if err := UpdateIssuesCommit(actUser, repo, commits.Commits, true); err != nil { - log.Error(4, "updateIssuesCommit: %v", err) +// NewPullRequestAction adds new action for creating a new pull request. +func NewPullRequestAction(doer *User, repo *Repository, pull *Issue, commits *PushCommits) error { + if err := UpdateIssuesCommit(doer, repo, commits.Commits, false); err != nil { + log.Error(4, "UpdateIssuesCommit: %v", err) } - return mergePullRequestAction(x, actUser, repo, pull, commits) -} - -func newPullRequestAction(e Engine, doer *User, repo *Repository, pull *Issue, commits *PushCommits) (err error) { - if err = NotifyWatchers(&Action{ + if err := NotifyWatchers(&Action{ ActUserID: doer.ID, ActUser: doer, OpType: ActionCreatePullRequest, @@ -779,20 +780,20 @@ func newPullRequestAction(e Engine, doer *User, repo *Repository, pull *Issue, c IsPrivate: repo.IsPrivate, }); err != nil { log.Error(4, "NotifyWatchers: %v", err) - } else if err = pull.MailParticipants(); err != nil { + } else if err := pull.MailParticipants(); err != nil { log.Error(4, "MailParticipants: %v", err) } return nil } -// NewPullRequestAction adds new action for merging pull request. -func NewPullRequestAction(actUser *User, repo *Repository, pull *Issue, commits *PushCommits) error { - if err := UpdateIssuesCommit(actUser, repo, commits.Commits, false); err != nil { - log.Error(4, "updateIssuesCommit: %v", err) +// UpdatePullRequestAction adds new action for updating or merging a pull request. +func UpdatePullRequestAction(doer *User, repo *Repository, pull *Issue, commits *PushCommits) error { + if err := UpdateIssuesCommit(doer, repo, commits.Commits, false); err != nil { + log.Error(4, "UpdateIssuesCommit: %v", err) } - return newPullRequestAction(x, actUser, repo, pull, commits) + return nil } func mirrorSyncAction(e Engine, opType ActionType, repo *Repository, refName string, data []byte) error { From 32c9209d346c859e6476340ebc48e9a2d769391f Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Tue, 27 Mar 2018 12:57:09 -0400 Subject: [PATCH 06/32] Add issue updating from commits being tracked by pull requests --- models/update.go | 68 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/models/update.go b/models/update.go index 0f71cd1e709a6..0a94624c7e651 100644 --- a/models/update.go +++ b/models/update.go @@ -68,7 +68,7 @@ type PushUpdateOptions struct { // PushUpdate must be called for any push actions in order to // generates necessary push action history feeds. func PushUpdate(branch string, opt PushUpdateOptions) error { - repo, err := pushUpdate(opt) + repo, err := pushUpdate(branch, opt) if err != nil { return err } @@ -184,7 +184,7 @@ func pushUpdateAddTag(repo *Repository, gitRepo *git.Repository, tagName string) return nil } -func pushUpdate(opts PushUpdateOptions) (repo *Repository, err error) { +func pushUpdate(branch string, opts PushUpdateOptions) (repo *Repository, err error) { isNewRef := opts.OldCommitID == git.EmptySHA isDelRef := opts.NewCommitID == git.EmptySHA if isNewRef && isDelRef { @@ -278,5 +278,69 @@ func pushUpdate(opts PushUpdateOptions) (repo *Repository, err error) { }); err != nil { return nil, fmt.Errorf("CommitRepoAction: %v", err) } + + // create actions that update pull requests tracking the branch that was pushed to + prs, err := GetUnmergedPullRequestsByHeadInfo(repo.ID, branch) + if err != nil { + log.Error(4, "Find pull requests [head_repo_id: %d, head_branch: %s]: %v", repo.ID, branch, err) + } else { + pusher, err := GetUserByID(opts.PusherID) + if err != nil { + return nil, fmt.Errorf("GetUserByID: %v", err) + } + + for _, pr := range prs { + if err = pr.GetHeadRepo(); err != nil { + log.Error(4, "GetHeadRepo: %v", err) + continue + } else if err = pr.GetBaseRepo(); err != nil { + log.Error(4, "GetBaseRepo: %v", err) + continue + } + + var ( + baseBranch *Branch + headBranch *Branch + baseCommit *git.Commit + headCommit *git.Commit + headGitRepo *git.Repository + ) + if baseBranch, err = pr.BaseRepo.GetBranch(pr.BaseBranch); err != nil { + log.Error(4, "BaseRepo.GetBranch: %v", err) + continue + } + if baseCommit, err = baseBranch.GetCommit(); err != nil { + log.Error(4, "baseBranch.GetCommit: %v", err) + continue + } + if headBranch, err = pr.HeadRepo.GetBranch(pr.HeadBranch); err != nil { + log.Error(4, "HeadRepo.GetBranch: %v", err) + continue + } + if headCommit, err = headBranch.GetCommit(); err != nil { + log.Error(4, "headRepo.GetCommit: %v", err) + continue + } + + // NOTICE: this is using pr.HeadRepo rather than pr.BaseRepo to get commits since they are going to be pushed in a go routine + if headGitRepo, err = git.OpenRepository(pr.HeadRepo.RepoPath()); err != nil { + log.Error(4, "OpenRepository", err) + continue + } + + l, err := headGitRepo.CommitsBetweenIDs(headCommit.ID.String(), baseCommit.ID.String()) + if err != nil { + log.Error(4, "CommitsBetweenIDs: %v", err) + continue + } + + commits := ListToPushCommits(l) + if err = UpdatePullRequestAction(pusher, pr.BaseRepo, pr.Issue, commits); err != nil { + log.Error(4, "UpdatePullRequestAction [%d]: %v", pr.ID, err) + continue + } + } + } + return repo, nil } From bb65b7fdcc4652262958b7512fdc50b415f712e2 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Tue, 27 Mar 2018 13:05:14 -0400 Subject: [PATCH 07/32] Fix code formatting --- models/action.go | 10 +++++----- models/pull.go | 8 ++++---- models/update.go | 8 ++++---- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/models/action.go b/models/action.go index 20faed28f5ab4..117742f489115 100644 --- a/models/action.go +++ b/models/action.go @@ -86,15 +86,15 @@ func assembleKeywordsPattern(words []string) string { func init() { // populate with details to find keywords for reference, reopen, close issueKeywordsToFind = []*IssueKeywordsToFind{ - &IssueKeywordsToFind{ + { Pattern: regexp.MustCompile(issueRefRegexpStr), KeywordsFoundMask: KeywordsFoundReference, }, - &IssueKeywordsToFind{ + { Pattern: regexp.MustCompile(assembleKeywordsPattern(issueReopenKeywords)), KeywordsFoundMask: KeywordsFoundReopen, }, - &IssueKeywordsToFind{ + { Pattern: regexp.MustCompile(assembleKeywordsPattern(issueCloseKeywords)), KeywordsFoundMask: KeywordsFoundClose, }, @@ -512,13 +512,13 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, com if commitsAreMerged { // take no action if both KeywordsFoundClose and KeywordsFoundOpen are set - if (mask & (KeywordsFoundReopen|KeywordsFoundClose)) == KeywordsFoundClose { + if (mask & (KeywordsFoundReopen | KeywordsFoundClose)) == KeywordsFoundClose { if issue.RepoID == repo.ID && !issue.IsClosed { if err = issue.ChangeStatus(doer, repo, true); err != nil { return err } } - } else if (mask & (KeywordsFoundReopen|KeywordsFoundClose)) == KeywordsFoundReopen { + } else if (mask & (KeywordsFoundReopen | KeywordsFoundClose)) == KeywordsFoundReopen { if issue.RepoID == repo.ID && issue.IsClosed { if err = issue.ChangeStatus(doer, repo, false); err != nil { return err diff --git a/models/pull.go b/models/pull.go index e7eb3c8ccc8a8..38bdc517d41b4 100644 --- a/models/pull.go +++ b/models/pull.go @@ -784,10 +784,10 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str mode, _ := AccessLevel(pull.Poster, repo) var ( - baseBranch *Branch - headBranch *Branch - baseCommit *git.Commit - headCommit *git.Commit + baseBranch *Branch + headBranch *Branch + baseCommit *git.Commit + headCommit *git.Commit baseGitRepo *git.Repository ) if baseBranch, err = pr.BaseRepo.GetBranch(pr.BaseBranch); err != nil { diff --git a/models/update.go b/models/update.go index 0a94624c7e651..65435861f49e0 100644 --- a/models/update.go +++ b/models/update.go @@ -299,10 +299,10 @@ func pushUpdate(branch string, opts PushUpdateOptions) (repo *Repository, err er } var ( - baseBranch *Branch - headBranch *Branch - baseCommit *git.Commit - headCommit *git.Commit + baseBranch *Branch + headBranch *Branch + baseCommit *git.Commit + headCommit *git.Commit headGitRepo *git.Repository ) if baseBranch, err = pr.BaseRepo.GetBranch(pr.BaseBranch); err != nil { From cd174460c18728b3630d564897ac12f65f4d1628 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Mon, 2 Apr 2018 10:02:52 -0400 Subject: [PATCH 08/32] Rename {Update -> Commit}PullRequestAction to be more accurately named --- models/action.go | 4 ++-- models/update.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/models/action.go b/models/action.go index 117742f489115..81237a02a84bf 100644 --- a/models/action.go +++ b/models/action.go @@ -787,8 +787,8 @@ func NewPullRequestAction(doer *User, repo *Repository, pull *Issue, commits *Pu return nil } -// UpdatePullRequestAction adds new action for updating or merging a pull request. -func UpdatePullRequestAction(doer *User, repo *Repository, pull *Issue, commits *PushCommits) error { +// CommitPullRequestAction adds new action for pushed commits tracked by a pull request. +func CommitPullRequestAction(doer *User, repo *Repository, commits *PushCommits) error { if err := UpdateIssuesCommit(doer, repo, commits.Commits, false); err != nil { log.Error(4, "UpdateIssuesCommit: %v", err) } diff --git a/models/update.go b/models/update.go index 65435861f49e0..ffa9cfab1eade 100644 --- a/models/update.go +++ b/models/update.go @@ -335,8 +335,8 @@ func pushUpdate(branch string, opts PushUpdateOptions) (repo *Repository, err er } commits := ListToPushCommits(l) - if err = UpdatePullRequestAction(pusher, pr.BaseRepo, pr.Issue, commits); err != nil { - log.Error(4, "UpdatePullRequestAction [%d]: %v", pr.ID, err) + if err = CommitPullRequestAction(pusher, pr.BaseRepo, commits); err != nil { + log.Error(4, "CommitPullRequestAction [%d]: %v", pr.ID, err) continue } } From d9ea59676d499f113f2549c7f7ecd6792bbae3b1 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Mon, 2 Apr 2018 10:10:13 -0400 Subject: [PATCH 09/32] Factor out creation of reference comments --- models/action.go | 2 +- models/issue_comment.go | 41 +++++++++++++++++++++++++++++++---------- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/models/action.go b/models/action.go index 81237a02a84bf..d17873e076f16 100644 --- a/models/action.go +++ b/models/action.go @@ -505,7 +505,7 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, com if (mask & KeywordsFoundReference) == KeywordsFoundReference { message := fmt.Sprintf(`%s`, repo.Link(), c.Sha1, c.Message) - if err = CreateRefComment(doer, repo, issue, message, c.Sha1); err != nil { + if err = CreateCommitRefComment(doer, repo, issue, message, c.Sha1); err != nil { return err } } diff --git a/models/issue_comment.go b/models/issue_comment.go index 96b162ca196b2..ead1e205dffb0 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -127,7 +127,8 @@ type Comment struct { CreatedUnix util.TimeStamp `xorm:"INDEX created"` UpdatedUnix util.TimeStamp `xorm:"INDEX updated"` - // Reference issue in commit message + // Reference issue in commit message, comments, issues, or pull requests + // the commit SHA for commit refs otherwise a SHA of a unique reference identifier CommitSHA string `xorm:"VARCHAR(40)"` Attachments []*Attachment `xorm:"-"` @@ -862,17 +863,17 @@ func CreateCodeComment(doer *User, repo *Repository, issue *Issue, content, tree }) } -// CreateRefComment creates a commit reference comment to issue. -func CreateRefComment(doer *User, repo *Repository, issue *Issue, content, commitSHA string) error { - if len(commitSHA) == 0 { - return fmt.Errorf("cannot create reference with empty commit SHA") +// createRefComment creates a commit, comment, issue, or pull request reference comment to issue. +func createRefComment(doer *User, repo *Repository, issue *Issue, content, refSHA string, commentType CommentType) error { + if len(refSHA) == 0 { + return fmt.Errorf("cannot create reference with empty SHA") } - // Check if same reference from same commit has already existed. + // Check if same reference from same issue and comment has already existed. has, err := x.Get(&Comment{ - Type: CommentTypeCommitRef, + Type: commentType, IssueID: issue.ID, - CommitSHA: commitSHA, + CommitSHA: refSHA, }) if err != nil { return fmt.Errorf("check reference comment: %v", err) @@ -881,16 +882,36 @@ func CreateRefComment(doer *User, repo *Repository, issue *Issue, content, commi } _, err = CreateComment(&CreateCommentOptions{ - Type: CommentTypeCommitRef, + Type: commentType, Doer: doer, Repo: repo, Issue: issue, - CommitSHA: commitSHA, + CommitSHA: refSHA, Content: content, }) return err } +// CreateCommitRefComment creates a commit reference comment to issue. +func CreateCommitRefComment(doer *User, repo *Repository, issue *Issue, content, commitSHA string) error { + return createRefComment(doer, repo, issue, content, commitSHA, CommentTypeCommitRef) +} + +// CreateCommentRefComment creates a comment reference comment to issue. +func CreateCommentRefComment(doer *User, repo *Repository, issue *Issue, content, refSHA string) error { + return createRefComment(doer, repo, issue, content, refSHA, CommentTypeCommentRef) +} + +// CreateIssueRefComment creates a comment reference comment to issue. +func CreateIssueRefComment(doer *User, repo *Repository, issue *Issue, content, refSHA string) error { + return createRefComment(doer, repo, issue, content, refSHA, CommentTypeIssueRef) +} + +// CreatePullRefComment creates a comment reference comment to issue. +func CreatePullRefComment(doer *User, repo *Repository, issue *Issue, content, refSHA string) error { + return createRefComment(doer, repo, issue, content, refSHA, CommentTypePullRef) +} + // GetCommentByID returns the comment by given ID. func GetCommentByID(id int64) (*Comment, error) { c := new(Comment) From 29dacbd97c6558ff86e4ccae13fb37b6ea8174c2 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Mon, 2 Apr 2018 10:19:20 -0400 Subject: [PATCH 10/32] Add the UpdateIssuesComment function The UpdateIssuesComment function handles referencing and manipulation of issues from comments, issues, and pull requests --- models/action.go | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/models/action.go b/models/action.go index d17873e076f16..4422561f2ca40 100644 --- a/models/action.go +++ b/models/action.go @@ -483,6 +483,67 @@ func findIssueReferencesInString(message string, repo *Repository) (map[int64]Ke return refs, nil } +// UpdateIssuesComment checks if issues are manipulated by a comment +func UpdateIssuesComment(doer *User, repo *Repository, commentIssue *Issue, comment *Comment, canOpenClose bool) error { + var refString string + if comment != nil { + refString = comment.Content + } else { + refString = commentIssue.Title + ": " + commentIssue.Content + } + + refs, err := findIssueReferencesInString(refString, repo) + if err != nil { + return err + } + + for id, mask := range refs { + issue, err := GetIssueByID(id) + if err != nil { + return err + } + if issue == nil || issue.ID == commentIssue.ID { + continue + } + + if (mask & KeywordsFoundReference) == KeywordsFoundReference { + uniqueID := fmt.Sprintf("%d", commentIssue.ID) + if comment != nil { + uniqueID += fmt.Sprintf("@%d", comment.ID) + } + + if comment != nil { + err = CreateCommentRefComment(doer, repo, issue, fmt.Sprintf(`%d`, comment.ID), base.EncodeSha1(uniqueID)) + } else if commentIssue.IsPull { + err = CreatePullRefComment(doer, repo, issue, fmt.Sprintf(`%d`, commentIssue.ID), base.EncodeSha1(uniqueID)) + } else { + err = CreateIssueRefComment(doer, repo, issue, fmt.Sprintf(`%d`, commentIssue.ID), base.EncodeSha1(uniqueID)) + } + if err != nil { + return err + } + } + + if canOpenClose { + // take no action if both KeywordsFoundClose and KeywordsFoundOpen are set + if (mask & (KeywordsFoundReopen | KeywordsFoundClose)) == KeywordsFoundClose { + if issue.RepoID == repo.ID && !issue.IsClosed { + if err = issue.ChangeStatus(doer, repo, true); err != nil { + return err + } + } + } else if (mask & (KeywordsFoundReopen | KeywordsFoundClose)) == KeywordsFoundReopen { + if issue.RepoID == repo.ID && issue.IsClosed { + if err = issue.ChangeStatus(doer, repo, false); err != nil { + return err + } + } + } + } + } + return nil +} + // UpdateIssuesCommit checks if issues are manipulated by commit message. func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, commitsAreMerged bool) error { // Commits are appended in the reverse order. From ff00d31489320e0c26bbe86e33bf753e74ded3fe Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Mon, 2 Apr 2018 10:28:46 -0400 Subject: [PATCH 11/32] Hook up the UpdateIssuesComment function to various actions The UpdateIssuesComment function is now being called when: - a pull request is merged, so the pull request title or description can open/close issues - a new pull request is created, so it can reference an issue from its title or description - a new issue is created, so it can reference an issue from its title or description - a pull request or issue title or description is updated - a comment is created or updated, so it can reference an issue from its title or description --- models/action.go | 32 ++++++++++++++++++++++++++++++++ models/issue.go | 12 ++++++++++++ models/issue_comment.go | 13 +++++++++++++ 3 files changed, 57 insertions(+) diff --git a/models/action.go b/models/action.go index 4422561f2ca40..0b6b939ea4c08 100644 --- a/models/action.go +++ b/models/action.go @@ -810,6 +810,10 @@ func MergePullRequestAction(doer *User, repo *Repository, pull *Issue, commits * log.Error(4, "UpdateIssuesCommit: %v", err) } + if err := UpdateIssuesComment(doer, repo, pull, nil, true); err != nil { + log.Error(4, "UpdateIssuesCommit: %v", err) + } + if err := notifyWatchers(x, &Action{ ActUserID: doer.ID, ActUser: doer, @@ -831,6 +835,10 @@ func NewPullRequestAction(doer *User, repo *Repository, pull *Issue, commits *Pu log.Error(4, "UpdateIssuesCommit: %v", err) } + if err := UpdateIssuesComment(doer, repo, pull, nil, false); err != nil { + log.Error(4, "UpdateIssuesCommit: %v", err) + } + if err := NotifyWatchers(&Action{ ActUserID: doer.ID, ActUser: doer, @@ -854,6 +862,30 @@ func CommitPullRequestAction(doer *User, repo *Repository, commits *PushCommits) log.Error(4, "UpdateIssuesCommit: %v", err) } + // no action added + + return nil +} + +// CreateOrUpdateCommentAction adds new action when creating or updating a comment. +func CreateOrUpdateCommentAction(doer *User, repo *Repository, issue *Issue, comment *Comment) error { + if err := UpdateIssuesComment(doer, repo, issue, comment, false); err != nil { + log.Error(4, "UpdateIssuesComment: %v", err) + } + + // no action added + + return nil +} + +// CreateOrUpdateIssueAction adds new action when creating a new issue or pull request. +func CreateOrUpdateIssueAction(doer *User, repo *Repository, issue *Issue) error { + if err := UpdateIssuesComment(doer, repo, issue, nil, false); err != nil { + log.Error(4, "UpdateIssuesComment: %v", err) + } + + // no action added + return nil } diff --git a/models/issue.go b/models/issue.go index 88e96e5706092..cdde362a614b9 100644 --- a/models/issue.go +++ b/models/issue.go @@ -829,6 +829,10 @@ func (issue *Issue) ChangeTitle(doer *User, title string) (err error) { go HookQueue.Add(issue.RepoID) } + if err = CreateOrUpdateIssueAction(doer, issue.Repo, issue); err != nil { + return fmt.Errorf("CreateOrUpdateIssueAction: %v", err) + } + return nil } @@ -894,6 +898,10 @@ func (issue *Issue) ChangeContent(doer *User, content string) (err error) { go HookQueue.Add(issue.RepoID) } + if err = CreateOrUpdateIssueAction(doer, issue.Repo, issue); err != nil { + return fmt.Errorf("CreateOrUpdateIssueAction: %v", err) + } + return nil } @@ -1068,6 +1076,10 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []in UpdateIssueIndexer(issue.ID) + if err = CreateOrUpdateIssueAction(issue.Poster, issue.Repo, issue); err != nil { + return fmt.Errorf("CreateOrUpdateIssueAction: %v", err) + } + if err = NotifyWatchers(&Action{ ActUserID: issue.Poster.ID, ActUser: issue.Poster, diff --git a/models/issue_comment.go b/models/issue_comment.go index ead1e205dffb0..5d64cfd6f96a9 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -778,6 +778,10 @@ func CreateComment(opts *CreateCommentOptions) (comment *Comment, err error) { if opts.Type == CommentTypeComment { UpdateIssueIndexer(opts.Issue.ID) + + if err = CreateOrUpdateCommentAction(comment.Poster, opts.Repo, opts.Issue, comment); err != nil { + return nil, err + } } return comment, nil } @@ -1002,6 +1006,15 @@ func UpdateComment(doer *User, c *Comment, oldContent string) error { return err } else if c.Type == CommentTypeComment { UpdateIssueIndexer(c.IssueID) + + issue, err := GetIssueByID(c.IssueID) + if err != nil { + return err + } + + if err = CreateOrUpdateCommentAction(c.Poster, issue.Repo, issue, c); err != nil { + return err + } } if err := c.LoadIssue(); err != nil { From 83d08c2ae38f5a6ede60cb6094b707478d1f9a76 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Mon, 2 Apr 2018 10:35:59 -0400 Subject: [PATCH 12/32] Add the LoadReference function Add the LoadReference function to prepare data members of reference types of comments for frontend rendering. Also prepares for the transition of commit reference comments into a new message format. --- models/issue_comment.go | 105 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/models/issue_comment.go b/models/issue_comment.go index 5d64cfd6f96a9..8f20704e0bf26 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -20,6 +20,7 @@ import ( api "code.gitea.io/sdk/gitea" + "code.gitea.io/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/markup" "code.gitea.io/gitea/modules/util" @@ -128,6 +129,11 @@ type Comment struct { UpdatedUnix util.TimeStamp `xorm:"INDEX updated"` // Reference issue in commit message, comments, issues, or pull requests + RefExists bool + RefIssue *Issue + RefComment *Comment + RefMessage string + RefURL string // the commit SHA for commit refs otherwise a SHA of a unique reference identifier CommitSHA string `xorm:"VARCHAR(40)"` @@ -264,6 +270,105 @@ func (c *Comment) EventTag() string { return "event-" + com.ToStr(c.ID) } +// LoadReference if comment.Type is CommentType{Issue,Commit,Comment,Pull}Ref, then load RefIssue, RefComment +func (c *Comment) LoadReference() error { + if c.Type == CommentTypeIssueRef || c.Type == CommentTypePullRef { + issueID := int64(0) + n, err := fmt.Sscanf(c.Content, "%d", &issueID) + if err != nil { + return err + } + + if n == 1 { + refIssue, err := GetIssueByID(issueID) + if err != nil { + return err + } + + pullOrIssue := "issues" + if refIssue.IsPull { + pullOrIssue = "pulls" + } + + c.RefIssue = refIssue + c.RefURL = fmt.Sprintf("%s/%s/%d", refIssue.Repo.Link(), pullOrIssue, refIssue.Index) + c.RefExists = true + } + } else if c.Type == CommentTypeCommitRef { + if strings.HasPrefix(c.Content, ``) + contentParts := strings.SplitN(content, `">`, 2) + + if len(contentParts) == 2 { + c.RefURL = contentParts[0] + c.RefMessage = contentParts[1] + c.RefExists = true + } + } else { + // this is a new style commit ref + contentParts := strings.SplitN(c.Content, " ", 2) + if len(contentParts) == 2 { + repoID := int64(0) + n, err := fmt.Sscanf(contentParts[0], "%d", &repoID) + if err != nil { + return err + } + + if n == 1 { + refRepo, err := GetRepositoryByID(repoID) + if err != nil { + return err + } + + gitRepo, err := git.OpenRepository(refRepo.RepoPath()) + if err != nil { + return err + } + + refCommit, err := gitRepo.GetCommit(contentParts[1][:40]) + if err != nil { + return err + } + + c.RefURL = fmt.Sprintf("%s/commit/%s", refRepo.Link(), refCommit.ID.String()) + c.RefMessage = refCommit.CommitMessage + c.RefExists = true + } + } + } + } else if c.Type == CommentTypeCommentRef { + commentID := int64(0) + n, err := fmt.Sscanf(c.Content, "%d", &commentID) + if err != nil { + return err + } + + if n == 1 { + refComment, err := GetCommentByID(commentID) + if err != nil { + return err + } + + refIssue, err := GetIssueByID(refComment.IssueID) + if err != nil { + return err + } + + pullOrIssue := "issues" + if refIssue.IsPull { + pullOrIssue = "pulls" + } + + c.RefIssue = refIssue + c.RefComment = refComment + c.RefURL = fmt.Sprintf("%s/%s/%d#%s", refIssue.Repo.Link(), pullOrIssue, refIssue.Index, refComment.HashTag()) + c.RefExists = true + } + } + return nil +} + // LoadLabel if comment.Type is CommentTypeLabel, then load Label func (c *Comment) LoadLabel() error { var label Label From ce0333f0ee922106c56f24d04238391fc679cdb8 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Mon, 2 Apr 2018 10:43:38 -0400 Subject: [PATCH 13/32] Hook up frontend templates to render reference comments Commit features: - renders reference comments only if their source still exists (except for old style commit refs) - allows for more complex rendering options, e.g. issue or commit status, quoted comments, etc. - properly handles updates to issue/PR titles, movement of repositories, etc. NOTE: Only the en_US locale is provided by this commit. --- options/locale/locale_en-US.ini | 3 ++ routers/repo/issue.go | 4 ++ .../repo/issue/view_content/comments.tmpl | 43 ++++++++++++++++++- 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 1a748e8b209ec..02a65f66c84b4 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -736,7 +736,10 @@ issues.reopen_comment_issue = Comment and Reopen issues.create_comment = Comment issues.closed_at = `closed %[2]s` issues.reopened_at = `reopened %[2]s` +issues.issue_ref_at = `referenced this issue from an issue %[2]s` issues.commit_ref_at = `referenced this issue from a commit %[2]s` +issues.comment_ref_at = `referenced this issue from a comment %[2]s` +issues.pull_ref_at = `referenced this issue from a pull request %[2]s` issues.poster = Poster issues.collaborator = Collaborator issues.owner = Owner diff --git a/routers/repo/issue.go b/routers/repo/issue.go index dd1c5cfa56c9f..2fcc457a090e0 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -725,6 +725,10 @@ func ViewIssue(ctx *context.Context) { if !isAdded && !issue.IsPoster(comment.Poster.ID) { participants = append(participants, comment.Poster) } + } else if comment.Type == models.CommentTypeIssueRef || comment.Type == models.CommentTypeCommitRef || comment.Type == models.CommentTypeCommentRef || comment.Type == models.CommentTypePullRef { + if err = comment.LoadReference(); err != nil { + continue + } } else if comment.Type == models.CommentTypeLabel { if err = comment.LoadLabel(); err != nil { ctx.ServerError("LoadLabel", err) diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 953aa4c2f02db..49702094b783b 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -81,7 +81,20 @@ {{.Poster.Name}} {{$.i18n.Tr "repo.issues.closed_at" .EventTag $createdStr | Safe}} - {{else if eq .Type 4}} + {{else if and (eq .Type 3) .RefExists}} +
+ + + + + {{.Poster.Name}} {{$.i18n.Tr "repo.issues.issue_ref_at" .EventTag $createdStr | Safe}} + +
+ + {{.RefIssue.Title | Escape}} (#{{.RefIssue.Index}}) +
+
+ {{else if and (eq .Type 4) .RefExists}}
@@ -91,7 +104,33 @@
- {{.Content | Str2html}} + {{.RefMessage | Escape}} ({{ShortSha .CommitSHA}}) +
+
+ {{else if and (eq .Type 5) .RefExists}} +
+ + + + + {{.Poster.Name}} {{$.i18n.Tr "repo.issues.comment_ref_at" .EventTag $createdStr | Safe}} + +
+ + {{.RefIssue.Title | Escape}} (#{{.RefIssue.Index}}) +
+
+ {{else if and (eq .Type 6) .RefExists}} +
+ + + + + {{.Poster.Name}} {{$.i18n.Tr "repo.issues.pull_ref_at" .EventTag $createdStr | Safe}} + +
+ + {{.RefIssue.Title | Escape}} (#{{.RefIssue.Index}})
{{else if eq .Type 7}} From c6a7ce5ae3179f78d62ca325a287660d64130d35 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Mon, 2 Apr 2018 10:48:14 -0400 Subject: [PATCH 14/32] Change commit ref comments to use a more parse-able form of content --- models/action.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/action.go b/models/action.go index 0b6b939ea4c08..d62b5b078dfb6 100644 --- a/models/action.go +++ b/models/action.go @@ -565,7 +565,7 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, com } if (mask & KeywordsFoundReference) == KeywordsFoundReference { - message := fmt.Sprintf(`%s`, repo.Link(), c.Sha1, c.Message) + message := fmt.Sprintf("%d %s", repo.ID, c.Sha1) if err = CreateCommitRefComment(doer, repo, issue, message, c.Sha1); err != nil { return err } From 3d72103a8e9d6c4f17bb026d0018907da833adad Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Sat, 7 Apr 2018 10:33:51 -0400 Subject: [PATCH 15/32] Applied suggested code changes * better commented KeywordsFound bitmask constants * populate issueKeywordsToFind variable at point of definition rather than in init() * add needed `xorm:"-"` statements to the fields that were added to Comment * also move calculation of uniqueID out of loop --- models/action.go | 32 ++++++++++++++------------------ models/issue_comment.go | 10 +++++----- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/models/action.go b/models/action.go index d62b5b078dfb6..9f5e581b88fc1 100644 --- a/models/action.go +++ b/models/action.go @@ -57,9 +57,9 @@ type KeywordsFoundMaskType int // Possible bitmask types for keywords that can be found. const ( - KeywordsFoundReference KeywordsFoundMaskType = 1 << iota // 1 - KeywordsFoundReopen // 2 - KeywordsFoundClose // 4 + KeywordsFoundReference KeywordsFoundMaskType = 1 << iota // 1 = 1 << 0 + KeywordsFoundReopen // 2 = 1 << 1 + KeywordsFoundClose // 4 = 1 << 2 ) // IssueKeywordsToFind represents a pairing of a pattern to use to find keywords in message and the keywords bitmask value. @@ -74,16 +74,6 @@ var ( issueCloseKeywords = []string{"close", "closes", "closed", "fix", "fixes", "fixed", "resolve", "resolves", "resolved"} issueReopenKeywords = []string{"reopen", "reopens", "reopened"} - issueKeywordsToFind []*IssueKeywordsToFind -) - -const issueRefRegexpStr = `(?:\S+/\S=)?#\d+` - -func assembleKeywordsPattern(words []string) string { - return fmt.Sprintf(`(?i)(?:%s) %s`, strings.Join(words, "|"), issueRefRegexpStr) -} - -func init() { // populate with details to find keywords for reference, reopen, close issueKeywordsToFind = []*IssueKeywordsToFind{ { @@ -99,6 +89,12 @@ func init() { KeywordsFoundMask: KeywordsFoundClose, }, } +) + +const issueRefRegexpStr = `(?:\S+/\S=)?#\d+` + +func assembleKeywordsPattern(words []string) string { + return fmt.Sprintf(`(?i)(?:%s) %s`, strings.Join(words, "|"), issueRefRegexpStr) } // Action represents user operation type and other information to @@ -492,6 +488,11 @@ func UpdateIssuesComment(doer *User, repo *Repository, commentIssue *Issue, comm refString = commentIssue.Title + ": " + commentIssue.Content } + uniqueID := fmt.Sprintf("%d", commentIssue.ID) + if comment != nil { + uniqueID += fmt.Sprintf("@%d", comment.ID) + } + refs, err := findIssueReferencesInString(refString, repo) if err != nil { return err @@ -507,11 +508,6 @@ func UpdateIssuesComment(doer *User, repo *Repository, commentIssue *Issue, comm } if (mask & KeywordsFoundReference) == KeywordsFoundReference { - uniqueID := fmt.Sprintf("%d", commentIssue.ID) - if comment != nil { - uniqueID += fmt.Sprintf("@%d", comment.ID) - } - if comment != nil { err = CreateCommentRefComment(doer, repo, issue, fmt.Sprintf(`%d`, comment.ID), base.EncodeSha1(uniqueID)) } else if commentIssue.IsPull { diff --git a/models/issue_comment.go b/models/issue_comment.go index 8f20704e0bf26..e1a75922c925e 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -129,11 +129,11 @@ type Comment struct { UpdatedUnix util.TimeStamp `xorm:"INDEX updated"` // Reference issue in commit message, comments, issues, or pull requests - RefExists bool - RefIssue *Issue - RefComment *Comment - RefMessage string - RefURL string + RefExists bool `xorm:"-"` + RefIssue *Issue `xorm:"-"` + RefComment *Comment `xorm:"-"` + RefMessage string `xorm:"-"` + RefURL string `xorm:"-"` // the commit SHA for commit refs otherwise a SHA of a unique reference identifier CommitSHA string `xorm:"VARCHAR(40)"` From cf174ea955437ad1c0fb4802987f56310d007fd9 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Sat, 7 Apr 2018 10:45:49 -0400 Subject: [PATCH 16/32] Improve code coverage of TestUpdateIssuesCommit --- models/action_test.go | 405 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 362 insertions(+), 43 deletions(-) diff --git a/models/action_test.go b/models/action_test.go index 519cba0b5cfd9..0f9d4d136fe53 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -7,6 +7,7 @@ import ( "testing" "code.gitea.io/git" + "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/setting" "github.com/stretchr/testify/assert" @@ -185,52 +186,370 @@ func Test_getIssueFromRef(t *testing.T) { } func TestUpdateIssuesCommit(t *testing.T) { - assert.NoError(t, PrepareTestDatabase()) - pushCommits := []*PushCommit{ - { - Sha1: "abcdef1", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user4@example.com", - AuthorName: "User Four", - Message: "start working on #FST-1, #1", - }, - { - Sha1: "abcdef2", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "a plain message", - }, - { - Sha1: "abcdef2", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "close #2", - }, - } + for _, commitsAreMerged := range []bool{false, true} { + // if commits were not merged then issue should not change status + isOpen := "is_closed!=1" + isClosed := "is_closed=1" + if !commitsAreMerged { + isClosed = isOpen + } - user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) - repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) - repo.Owner = user + assert.NoError(t, PrepareTestDatabase()) + user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) + repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) + repo.Owner = user - commentBean := &Comment{ - Type: CommentTypeCommitRef, - CommitSHA: "abcdef1", - PosterID: user.ID, - IssueID: 1, - } - issueBean := &Issue{RepoID: repo.ID, Index: 2} + // test re-open of already open issue + pushCommits := []*PushCommit{ + { + Sha1: "abcdef1", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "reoopen #2", + }, + } + commentBean := []*Comment{ + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[0].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + } + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) + AssertExistsAndLoadBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + CheckConsistencyFor(t, &Action{}) + + // test simultaneous open and close on an already open issue + pushCommits = []*PushCommit{ + { + Sha1: "abcdef2", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "reopen #2 and the close #2", + }, + } + commentBean = []*Comment{ + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[0].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + } + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) + AssertExistsAndLoadBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + CheckConsistencyFor(t, &Action{}) + + // test close of an open issue + pushCommits = []*PushCommit{ + { + Sha1: "abcdef3", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "closes #2", + }, + } + commentBean = []*Comment{ + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[0].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + } + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) + AssertExistsAndLoadBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + CheckConsistencyFor(t, &Action{}) + + // test close of an already closed issue + pushCommits = []*PushCommit{ + { + Sha1: "abcdef4", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "close #2", + }, + } + commentBean = []*Comment{ + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[0].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + } + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) + AssertExistsAndLoadBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + CheckConsistencyFor(t, &Action{}) + + // test simultaneous open and close on a closed issue + pushCommits = []*PushCommit{ + { + Sha1: "abcdef5", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "close #2 and reopen #2", + }, + } + commentBean = []*Comment{ + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[0].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + } + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) + AssertExistsAndLoadBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + CheckConsistencyFor(t, &Action{}) + + // test referencing an closed issue + pushCommits = []*PushCommit{ + { + Sha1: "abcdef6", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "for details on how to open, see #2", + }, + } + commentBean = []*Comment{ + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[0].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + } + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) + AssertExistsAndLoadBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + CheckConsistencyFor(t, &Action{}) + + // test re-open a closed issue + pushCommits = []*PushCommit{ + { + Sha1: "abcdef7", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "reopens #2", + }, + } + commentBean = []*Comment{ + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[0].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + } + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) + AssertExistsAndLoadBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + CheckConsistencyFor(t, &Action{}) + + // test referencing an open issue + pushCommits = []*PushCommit{ + { + Sha1: "abcdef8", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "for details on how to close, see #2", + }, + } + commentBean = []*Comment{ + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[0].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + } + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) + AssertExistsAndLoadBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + CheckConsistencyFor(t, &Action{}) + + // test close-then-open commit order + pushCommits = []*PushCommit{ + { + Sha1: "abcdef10", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "reopened #2", + }, + { + Sha1: "abcdef9", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "fixes #2", + }, + } + commentBean = []*Comment{ + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[0].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[1].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + } + AssertNotExistsBean(t, commentBean[0]) + AssertNotExistsBean(t, commentBean[1]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) + AssertExistsAndLoadBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, commentBean[1]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + CheckConsistencyFor(t, &Action{}) + + // test open-then-close commit order + pushCommits = []*PushCommit{ + { + Sha1: "abcdef12", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "resolved #2", + }, + { + Sha1: "abcdef11", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "reopened #2", + }, + } + commentBean = []*Comment{ + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[0].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[1].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + } + AssertNotExistsBean(t, commentBean[0]) + AssertNotExistsBean(t, commentBean[1]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) + AssertExistsAndLoadBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, commentBean[1]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + CheckConsistencyFor(t, &Action{}) + + // test more complex commit pattern + pushCommits = []*PushCommit{ + { + Sha1: "abcdef15", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user4@example.com", + AuthorName: "User Four", + Message: "start working on #FST-1, #1", + }, + { + Sha1: "abcdef14", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "reopen #2", + }, + { + Sha1: "abcdef13", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "close #2", + }, + } + commentBean = []*Comment{ + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[0].Sha1, + PosterID: user.ID, + IssueID: 1, + }, + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[1].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + { + Type: CommentTypeCommitRef, + CommitSHA: pushCommits[2].Sha1, + PosterID: user.ID, + IssueID: 2, + }, + } - AssertNotExistsBean(t, commentBean) - AssertNotExistsBean(t, &Issue{RepoID: repo.ID, Index: 2}, "is_closed=1") - assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, true)) - AssertExistsAndLoadBean(t, commentBean) - AssertExistsAndLoadBean(t, issueBean, "is_closed=1") - CheckConsistencyFor(t, &Action{}) + AssertNotExistsBean(t, commentBean[0]) + AssertNotExistsBean(t, commentBean[1]) + AssertNotExistsBean(t, commentBean[2]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) + AssertExistsAndLoadBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, commentBean[1]) + AssertExistsAndLoadBean(t, commentBean[2]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + CheckConsistencyFor(t, &Action{}) + } } func testCorrectRepoAction(t *testing.T, opts CommitRepoActionOptions, actionBean *Action) { From 943be3d45af4844c39aba7ccd8d6f85fea08655b Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Sat, 7 Apr 2018 11:12:12 -0400 Subject: [PATCH 17/32] Add test for referencing issues in comments --- models/action_test.go | 72 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/models/action_test.go b/models/action_test.go index 0f9d4d136fe53..7ab4141a64db1 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -185,6 +185,78 @@ func Test_getIssueFromRef(t *testing.T) { } } +func TestUpdateIssuesCommentComments(t *testing.T) { + isOpen := "is_closed!=1" + + assert.NoError(t, PrepareTestDatabase()) + user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) + repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) + repo.Owner = user + + commentIssue := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}).(*Issue) + refIssue1 := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}).(*Issue) + refIssue2 := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}).(*Issue) + + comment := Comment{ + ID: 123456789, + Type: CommentTypeComment, + PosterID: user.ID, + Poster: user, + IssueID: commentIssue.ID, + Content: "this is a comment that mentions #2 and #1 too", + } + + commentBean := []*Comment{ + { + Type: CommentTypeCommentRef, + CommitSHA: base.EncodeSha1(fmt.Sprintf("%d@%d", commentIssue.ID, comment.ID)), + PosterID: user.ID, + IssueID: commentIssue.ID, + }, + { + Type: CommentTypeCommentRef, + CommitSHA: base.EncodeSha1(fmt.Sprintf("%d@%d", commentIssue.ID, comment.ID)), + PosterID: user.ID, + IssueID: refIssue1.ID, + }, + { + Type: CommentTypeCommentRef, + CommitSHA: base.EncodeSha1(fmt.Sprintf("%d@%d", commentIssue.ID, comment.ID)), + PosterID: user.ID, + IssueID: refIssue2.ID, + }, + } + + // test comment referencing issue including self-referencing + AssertNotExistsBean(t, commentBean[0]) + AssertNotExistsBean(t, commentBean[1]) + AssertNotExistsBean(t, commentBean[2]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) + assert.NoError(t, UpdateIssuesComment(user, repo, commentIssue, &comment, false)) + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, commentBean[1]) + AssertNotExistsBean(t, commentBean[2]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) + CheckConsistencyFor(t, &Action{}) + + // test comment updating issue reference + comment.Content = "this is a comment that mentions #2 and #3 too" + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, commentBean[1]) + AssertNotExistsBean(t, commentBean[2]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) + assert.NoError(t, UpdateIssuesComment(user, repo, commentIssue, &comment, false)) + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, commentBean[1]) + AssertExistsAndLoadBean(t, commentBean[2]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) + CheckConsistencyFor(t, &Action{}) +} + func TestUpdateIssuesCommit(t *testing.T) { for _, commitsAreMerged := range []bool{false, true} { // if commits were not merged then issue should not change status From 9f5b9ea17778776510030f50fa65cd8510a68656 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Sat, 7 Apr 2018 11:13:32 -0400 Subject: [PATCH 18/32] Add test for updating issues from issues --- models/action_test.go | 87 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/models/action_test.go b/models/action_test.go index 7ab4141a64db1..14d116874a4dc 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -185,6 +185,93 @@ func Test_getIssueFromRef(t *testing.T) { } } +func TestUpdateIssuesCommentIssues(t *testing.T) { + for _, canOpenClose := range []bool{false, true} { + // if cannot open or close then issue should not change status + isOpen := "is_closed!=1" + isClosed := "is_closed=1" + if !canOpenClose { + isClosed = isOpen + } + + assert.NoError(t, PrepareTestDatabase()) + user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) + repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) + repo.Owner = user + + commentIssue := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}).(*Issue) + refIssue1 := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}).(*Issue) + refIssue2 := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}).(*Issue) + + commentBean := []*Comment{ + { + Type: CommentTypeIssueRef, + CommitSHA: base.EncodeSha1(fmt.Sprintf("%d", commentIssue.ID)), + PosterID: user.ID, + IssueID: commentIssue.ID, + }, + { + Type: CommentTypeIssueRef, + CommitSHA: base.EncodeSha1(fmt.Sprintf("%d", commentIssue.ID)), + PosterID: user.ID, + IssueID: refIssue1.ID, + }, + { + Type: CommentTypeIssueRef, + CommitSHA: base.EncodeSha1(fmt.Sprintf("%d", commentIssue.ID)), + PosterID: user.ID, + IssueID: refIssue2.ID, + }, + } + + // test issue/pull request closing multiple issues + commentIssue.Title = "close #2" + commentIssue.Content = "close #3" + AssertNotExistsBean(t, commentBean[0]) + AssertNotExistsBean(t, commentBean[1]) + AssertNotExistsBean(t, commentBean[2]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) + assert.NoError(t, UpdateIssuesComment(user, repo, commentIssue, nil, canOpenClose)) + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, commentBean[1]) + AssertExistsAndLoadBean(t, commentBean[2]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isClosed) + CheckConsistencyFor(t, &Action{}) + + // test issue/pull request re-opening multiple issues + commentIssue.Title = "reopen #2" + commentIssue.Content = "reopen #3" + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, commentBean[1]) + AssertExistsAndLoadBean(t, commentBean[2]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isClosed) + assert.NoError(t, UpdateIssuesComment(user, repo, commentIssue, nil, canOpenClose)) + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, commentBean[1]) + AssertExistsAndLoadBean(t, commentBean[2]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) + CheckConsistencyFor(t, &Action{}) + + // test issue/pull request mixing re-opening and closing issue and self-referencing issue + commentIssue.Title = "reopen #2" + commentIssue.Content = "close #2 and reference #1" + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, commentBean[1]) + AssertExistsAndLoadBean(t, commentBean[2]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + assert.NoError(t, UpdateIssuesComment(user, repo, commentIssue, nil, canOpenClose)) + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, commentBean[1]) + AssertExistsAndLoadBean(t, commentBean[2]) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + CheckConsistencyFor(t, &Action{}) + } +} + func TestUpdateIssuesCommentComments(t *testing.T) { isOpen := "is_closed!=1" From 98e12cebc6ece48c52e9cb8802657dad4cfe0006 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Sat, 7 Apr 2018 11:17:20 -0400 Subject: [PATCH 19/32] Update TestCreateComment to check referenced issue --- models/issue_comment_test.go | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/models/issue_comment_test.go b/models/issue_comment_test.go index 91dd5c17322dd..d98cdd18e2d86 100644 --- a/models/issue_comment_test.go +++ b/models/issue_comment_test.go @@ -14,9 +14,27 @@ import ( func TestCreateComment(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) - issue := AssertExistsAndLoadBean(t, &Issue{}).(*Issue) - repo := AssertExistsAndLoadBean(t, &Repository{ID: issue.RepoID}).(*Repository) - doer := AssertExistsAndLoadBean(t, &User{ID: repo.OwnerID}).(*User) + doer := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) + repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) + repo.Owner = doer + + issue := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}).(*Issue) + refIssue := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}).(*Issue) + + commentBean := []*Comment{ + { + Type: CommentTypeCommentRef, + PosterID: doer.ID, + IssueID: issue.ID, + }, + { + Type: CommentTypeCommentRef, + PosterID: doer.ID, + IssueID: refIssue.ID, + }, + } + AssertNotExistsBean(t, commentBean[0]) + AssertNotExistsBean(t, commentBean[1]) now := time.Now().Unix() comment, err := CreateComment(&CreateCommentOptions{ @@ -24,20 +42,28 @@ func TestCreateComment(t *testing.T) { Doer: doer, Repo: repo, Issue: issue, - Content: "Hello", + Content: "Hello, this comment references issue #2", }) assert.NoError(t, err) then := time.Now().Unix() assert.EqualValues(t, CommentTypeComment, comment.Type) - assert.EqualValues(t, "Hello", comment.Content) + assert.EqualValues(t, "Hello, this comment references issue #2", comment.Content) assert.EqualValues(t, issue.ID, comment.IssueID) assert.EqualValues(t, doer.ID, comment.PosterID) AssertInt64InRange(t, now, then, int64(comment.CreatedUnix)) AssertExistsAndLoadBean(t, comment) // assert actually added to DB + AssertNotExistsBean(t, commentBean[0]) + AssertExistsAndLoadBean(t, commentBean[1]) updatedIssue := AssertExistsAndLoadBean(t, &Issue{ID: issue.ID}).(*Issue) AssertInt64InRange(t, now, then, int64(updatedIssue.UpdatedUnix)) + + err = commentBean[1].LoadReference() + assert.NoError(t, err) + if assert.NotNil(t, commentBean[1].RefIssue) { + assert.EqualValues(t, issue.ID, commentBean[1].RefIssue.ID) + } } func TestFetchCodeComments(t *testing.T) { From af7c75b30eb22388732e59c032837e1ba3542893 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Wed, 23 May 2018 11:37:15 -0400 Subject: [PATCH 20/32] Correct error message --- models/action.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/action.go b/models/action.go index 9f5e581b88fc1..2dc37f7d59655 100644 --- a/models/action.go +++ b/models/action.go @@ -807,7 +807,7 @@ func MergePullRequestAction(doer *User, repo *Repository, pull *Issue, commits * } if err := UpdateIssuesComment(doer, repo, pull, nil, true); err != nil { - log.Error(4, "UpdateIssuesCommit: %v", err) + log.Error(4, "UpdateIssuesComment: %v", err) } if err := notifyWatchers(x, &Action{ @@ -832,7 +832,7 @@ func NewPullRequestAction(doer *User, repo *Repository, pull *Issue, commits *Pu } if err := UpdateIssuesComment(doer, repo, pull, nil, false); err != nil { - log.Error(4, "UpdateIssuesCommit: %v", err) + log.Error(4, "UpdateIssuesComment: %v", err) } if err := NotifyWatchers(&Action{ From 845495088f713e31bfdb52b5f92ceb0ae15960b5 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Fri, 25 May 2018 09:01:03 -0400 Subject: [PATCH 21/32] Trigger MergePullRequestAction when PR is manually merged --- models/action.go | 8 +++++--- models/pull.go | 5 +++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/models/action.go b/models/action.go index 2dc37f7d59655..816233412dfca 100644 --- a/models/action.go +++ b/models/action.go @@ -800,10 +800,12 @@ func TransferRepoAction(doer, oldOwner *User, repo *Repository) error { return transferRepoAction(x, doer, oldOwner, repo) } -// MergePullRequestAction adds new action for merging pull request. +// MergePullRequestAction adds new action for merging pull request (including manually merged pull requests). func MergePullRequestAction(doer *User, repo *Repository, pull *Issue, commits *PushCommits) error { - if err := UpdateIssuesCommit(doer, repo, commits.Commits, true); err != nil { - log.Error(4, "UpdateIssuesCommit: %v", err) + if commits != nil { + if err := UpdateIssuesCommit(doer, repo, commits.Commits, true); err != nil { + log.Error(4, "UpdateIssuesCommit: %v", err) + } } if err := UpdateIssuesComment(doer, repo, pull, nil, true); err != nil { diff --git a/models/pull.go b/models/pull.go index 38bdc517d41b4..7f370c31e04b2 100644 --- a/models/pull.go +++ b/models/pull.go @@ -588,6 +588,11 @@ func (pr *PullRequest) manuallyMerged() bool { return false } log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String()) + + if err = MergePullRequestAction(pr.Merger, pr.Issue.Repo, pr.Issue, nil); err != nil { + log.Error(4, "MergePullRequestAction [%d]: %v", pr.ID, err) + } + return true } return false From 5e48595747f4fbe537d6ffea272b549bd59f3cbc Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Fri, 6 Jul 2018 08:20:25 -0400 Subject: [PATCH 22/32] Change variable names from KeywordsFound* to Keyword* --- models/action.go | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/models/action.go b/models/action.go index 816233412dfca..d82dc728b71d9 100644 --- a/models/action.go +++ b/models/action.go @@ -52,20 +52,20 @@ const ( ActionMirrorSyncDelete // 20 ) -// KeywordsFoundMaskType represents the bitmask of types of keywords found in a message. -type KeywordsFoundMaskType int +// KeywordMaskType represents the bitmask of types of keywords found in a message. +type KeywordMaskType int // Possible bitmask types for keywords that can be found. const ( - KeywordsFoundReference KeywordsFoundMaskType = 1 << iota // 1 = 1 << 0 - KeywordsFoundReopen // 2 = 1 << 1 - KeywordsFoundClose // 4 = 1 << 2 + KeywordReference KeywordMaskType = 1 << iota // 1 = 1 << 0 + KeywordReopen // 2 = 1 << 1 + KeywordClose // 4 = 1 << 2 ) // IssueKeywordsToFind represents a pairing of a pattern to use to find keywords in message and the keywords bitmask value. type IssueKeywordsToFind struct { - Pattern *regexp.Regexp - KeywordsFoundMask KeywordsFoundMaskType + Pattern *regexp.Regexp + KeywordMask KeywordMaskType } var ( @@ -77,16 +77,16 @@ var ( // populate with details to find keywords for reference, reopen, close issueKeywordsToFind = []*IssueKeywordsToFind{ { - Pattern: regexp.MustCompile(issueRefRegexpStr), - KeywordsFoundMask: KeywordsFoundReference, + Pattern: regexp.MustCompile(issueRefRegexpStr), + KeywordMask: KeywordReference, }, { - Pattern: regexp.MustCompile(assembleKeywordsPattern(issueReopenKeywords)), - KeywordsFoundMask: KeywordsFoundReopen, + Pattern: regexp.MustCompile(assembleKeywordsPattern(issueReopenKeywords)), + KeywordMask: KeywordReopen, }, { - Pattern: regexp.MustCompile(assembleKeywordsPattern(issueCloseKeywords)), - KeywordsFoundMask: KeywordsFoundClose, + Pattern: regexp.MustCompile(assembleKeywordsPattern(issueCloseKeywords)), + KeywordMask: KeywordClose, }, } ) @@ -462,8 +462,8 @@ func getIssueFromRef(repo *Repository, ref string) (*Issue, error) { } // findIssueReferencesInString iterates over the keywords to find in a message and accumulates the findings into refs -func findIssueReferencesInString(message string, repo *Repository) (map[int64]KeywordsFoundMaskType, error) { - refs := make(map[int64]KeywordsFoundMaskType) +func findIssueReferencesInString(message string, repo *Repository) (map[int64]KeywordMaskType, error) { + refs := make(map[int64]KeywordMaskType) for _, kwToFind := range issueKeywordsToFind { for _, ref := range kwToFind.Pattern.FindAllString(message, -1) { issue, err := getIssueFromRef(repo, ref) @@ -472,7 +472,7 @@ func findIssueReferencesInString(message string, repo *Repository) (map[int64]Ke } if issue != nil { - refs[issue.ID] |= kwToFind.KeywordsFoundMask + refs[issue.ID] |= kwToFind.KeywordMask } } } @@ -507,7 +507,7 @@ func UpdateIssuesComment(doer *User, repo *Repository, commentIssue *Issue, comm continue } - if (mask & KeywordsFoundReference) == KeywordsFoundReference { + if (mask & KeywordReference) == KeywordReference { if comment != nil { err = CreateCommentRefComment(doer, repo, issue, fmt.Sprintf(`%d`, comment.ID), base.EncodeSha1(uniqueID)) } else if commentIssue.IsPull { @@ -521,14 +521,14 @@ func UpdateIssuesComment(doer *User, repo *Repository, commentIssue *Issue, comm } if canOpenClose { - // take no action if both KeywordsFoundClose and KeywordsFoundOpen are set - if (mask & (KeywordsFoundReopen | KeywordsFoundClose)) == KeywordsFoundClose { + // take no action if both KeywordClose and KeywordOpen are set + if (mask & (KeywordReopen | KeywordClose)) == KeywordClose { if issue.RepoID == repo.ID && !issue.IsClosed { if err = issue.ChangeStatus(doer, repo, true); err != nil { return err } } - } else if (mask & (KeywordsFoundReopen | KeywordsFoundClose)) == KeywordsFoundReopen { + } else if (mask & (KeywordReopen | KeywordClose)) == KeywordReopen { if issue.RepoID == repo.ID && issue.IsClosed { if err = issue.ChangeStatus(doer, repo, false); err != nil { return err @@ -560,7 +560,7 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, com continue } - if (mask & KeywordsFoundReference) == KeywordsFoundReference { + if (mask & KeywordReference) == KeywordReference { message := fmt.Sprintf("%d %s", repo.ID, c.Sha1) if err = CreateCommitRefComment(doer, repo, issue, message, c.Sha1); err != nil { return err @@ -568,14 +568,14 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, com } if commitsAreMerged { - // take no action if both KeywordsFoundClose and KeywordsFoundOpen are set - if (mask & (KeywordsFoundReopen | KeywordsFoundClose)) == KeywordsFoundClose { + // take no action if both KeywordClose and KeywordOpen are set + if (mask & (KeywordReopen | KeywordClose)) == KeywordClose { if issue.RepoID == repo.ID && !issue.IsClosed { if err = issue.ChangeStatus(doer, repo, true); err != nil { return err } } - } else if (mask & (KeywordsFoundReopen | KeywordsFoundClose)) == KeywordsFoundReopen { + } else if (mask & (KeywordReopen | KeywordClose)) == KeywordReopen { if issue.RepoID == repo.ID && issue.IsClosed { if err = issue.ChangeStatus(doer, repo, false); err != nil { return err From 7c8204a9ae6fe13a3abac2fd0095931f3c4de180 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Fri, 6 Jul 2018 08:31:13 -0400 Subject: [PATCH 23/32] Move common status changing logic into a function --- models/action.go | 50 +++++++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/models/action.go b/models/action.go index d82dc728b71d9..166b7b61770ca 100644 --- a/models/action.go +++ b/models/action.go @@ -479,6 +479,26 @@ func findIssueReferencesInString(message string, repo *Repository) (map[int64]Ke return refs, nil } +// changeIssueStatus encapsulates the logic for changing the status of an issue based on what keywords are marked in the keyword mask +func changeIssueStatus(mask KeywordMaskType, doer *User, repo *Repository, issue *Issue) error { + // take no action if both KeywordClose and KeywordOpen are set + switch mask & (KeywordReopen | KeywordClose) { + case KeywordClose: + if issue.RepoID == repo.ID && !issue.IsClosed { + if err := issue.ChangeStatus(doer, repo, true); err != nil { + return err + } + } + case KeywordReopen: + if issue.RepoID == repo.ID && issue.IsClosed { + if err := issue.ChangeStatus(doer, repo, false); err != nil { + return err + } + } + } + return nil +} + // UpdateIssuesComment checks if issues are manipulated by a comment func UpdateIssuesComment(doer *User, repo *Repository, commentIssue *Issue, comment *Comment, canOpenClose bool) error { var refString string @@ -521,19 +541,8 @@ func UpdateIssuesComment(doer *User, repo *Repository, commentIssue *Issue, comm } if canOpenClose { - // take no action if both KeywordClose and KeywordOpen are set - if (mask & (KeywordReopen | KeywordClose)) == KeywordClose { - if issue.RepoID == repo.ID && !issue.IsClosed { - if err = issue.ChangeStatus(doer, repo, true); err != nil { - return err - } - } - } else if (mask & (KeywordReopen | KeywordClose)) == KeywordReopen { - if issue.RepoID == repo.ID && issue.IsClosed { - if err = issue.ChangeStatus(doer, repo, false); err != nil { - return err - } - } + if err = changeIssueStatus(mask, doer, repo, issue); err != nil { + return err } } } @@ -568,19 +577,8 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, com } if commitsAreMerged { - // take no action if both KeywordClose and KeywordOpen are set - if (mask & (KeywordReopen | KeywordClose)) == KeywordClose { - if issue.RepoID == repo.ID && !issue.IsClosed { - if err = issue.ChangeStatus(doer, repo, true); err != nil { - return err - } - } - } else if (mask & (KeywordReopen | KeywordClose)) == KeywordReopen { - if issue.RepoID == repo.ID && issue.IsClosed { - if err = issue.ChangeStatus(doer, repo, false); err != nil { - return err - } - } + if err = changeIssueStatus(mask, doer, repo, issue); err != nil { + return err } } } From 1768f33d92422a0510f0bd994e34f9445e27d239 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Fri, 6 Jul 2018 08:48:21 -0400 Subject: [PATCH 24/32] Change "x := int64(0)" statements to "var x int64" --- models/issue_comment.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/models/issue_comment.go b/models/issue_comment.go index e1a75922c925e..752a7ddfe1761 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -273,7 +273,7 @@ func (c *Comment) EventTag() string { // LoadReference if comment.Type is CommentType{Issue,Commit,Comment,Pull}Ref, then load RefIssue, RefComment func (c *Comment) LoadReference() error { if c.Type == CommentTypeIssueRef || c.Type == CommentTypePullRef { - issueID := int64(0) + var issueID int64 n, err := fmt.Sscanf(c.Content, "%d", &issueID) if err != nil { return err @@ -309,7 +309,7 @@ func (c *Comment) LoadReference() error { // this is a new style commit ref contentParts := strings.SplitN(c.Content, " ", 2) if len(contentParts) == 2 { - repoID := int64(0) + var repoID int64 n, err := fmt.Sscanf(contentParts[0], "%d", &repoID) if err != nil { return err @@ -338,7 +338,7 @@ func (c *Comment) LoadReference() error { } } } else if c.Type == CommentTypeCommentRef { - commentID := int64(0) + var commentID int64 n, err := fmt.Sscanf(c.Content, "%d", &commentID) if err != nil { return err From a398702b5b29c064ef1dd7bd3e4050cc808f83c9 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Tue, 14 Aug 2018 10:47:39 -0400 Subject: [PATCH 25/32] Refactor reference comment related code - Move all code related to getting/setting the uniqueness SHA identifier of a reference comment into issue_comment.go - Prepare the reference comment code to handle references to issues in other repos - Clean up redundant and confusing code, remove use of deprecated ShortSHA function - Consistent formatting for issue-linking comments --- models/action.go | 42 +++---- models/issue_comment.go | 118 ++++++++---------- .../repo/issue/view_content/comments.tmpl | 6 +- 3 files changed, 74 insertions(+), 92 deletions(-) diff --git a/models/action.go b/models/action.go index 166b7b61770ca..2b880066dedd6 100644 --- a/models/action.go +++ b/models/action.go @@ -480,20 +480,20 @@ func findIssueReferencesInString(message string, repo *Repository) (map[int64]Ke } // changeIssueStatus encapsulates the logic for changing the status of an issue based on what keywords are marked in the keyword mask -func changeIssueStatus(mask KeywordMaskType, doer *User, repo *Repository, issue *Issue) error { +func changeIssueStatus(mask KeywordMaskType, doer *User, issue *Issue) error { // take no action if both KeywordClose and KeywordOpen are set - switch mask & (KeywordReopen | KeywordClose) { + switch mask { case KeywordClose: - if issue.RepoID == repo.ID && !issue.IsClosed { - if err := issue.ChangeStatus(doer, repo, true); err != nil { - return err + if err := issue.ChangeStatus(doer, issue.Repo, true); err != nil { + // Don't return the error when dependencies are still open as this would cause a push, merge, etc. to fail + if IsErrDependenciesLeft(err) { + return nil } + return err } case KeywordReopen: - if issue.RepoID == repo.ID && issue.IsClosed { - if err := issue.ChangeStatus(doer, repo, false); err != nil { - return err - } + if err := issue.ChangeStatus(doer, issue.Repo, false); err != nil { + return err } } return nil @@ -508,11 +508,6 @@ func UpdateIssuesComment(doer *User, repo *Repository, commentIssue *Issue, comm refString = commentIssue.Title + ": " + commentIssue.Content } - uniqueID := fmt.Sprintf("%d", commentIssue.ID) - if comment != nil { - uniqueID += fmt.Sprintf("@%d", comment.ID) - } - refs, err := findIssueReferencesInString(refString, repo) if err != nil { return err @@ -529,19 +524,20 @@ func UpdateIssuesComment(doer *User, repo *Repository, commentIssue *Issue, comm if (mask & KeywordReference) == KeywordReference { if comment != nil { - err = CreateCommentRefComment(doer, repo, issue, fmt.Sprintf(`%d`, comment.ID), base.EncodeSha1(uniqueID)) + err = CreateCommentRefComment(doer, issue, commentIssue.ID, comment.ID) } else if commentIssue.IsPull { - err = CreatePullRefComment(doer, repo, issue, fmt.Sprintf(`%d`, commentIssue.ID), base.EncodeSha1(uniqueID)) + err = CreatePullRefComment(doer, issue, commentIssue.ID) } else { - err = CreateIssueRefComment(doer, repo, issue, fmt.Sprintf(`%d`, commentIssue.ID), base.EncodeSha1(uniqueID)) + err = CreateIssueRefComment(doer, issue, commentIssue.ID) } if err != nil { return err } } - if canOpenClose { - if err = changeIssueStatus(mask, doer, repo, issue); err != nil { + // only change issue status if this is a pull request in the issue's repo + if canOpenClose && comment == nil && commentIssue.IsPull && repo.ID == issue.RepoID { + if err = changeIssueStatus(mask&(KeywordReopen|KeywordClose), doer, issue); err != nil { return err } } @@ -570,14 +566,14 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, com } if (mask & KeywordReference) == KeywordReference { - message := fmt.Sprintf("%d %s", repo.ID, c.Sha1) - if err = CreateCommitRefComment(doer, repo, issue, message, c.Sha1); err != nil { + if err = CreateCommitRefComment(doer, issue, repo.ID, c.Sha1); err != nil { return err } } - if commitsAreMerged { - if err = changeIssueStatus(mask, doer, repo, issue); err != nil { + // only change issue status if the commit is merged to the issue's repo + if commitsAreMerged && repo.ID == issue.RepoID { + if err = changeIssueStatus(mask&(KeywordReopen|KeywordClose), doer, issue); err != nil { return err } } diff --git a/models/issue_comment.go b/models/issue_comment.go index 752a7ddfe1761..ece7a4d6b10b6 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -11,9 +11,6 @@ import ( "fmt" "strings" - "code.gitea.io/git" - "code.gitea.io/gitea/modules/markup/markdown" - "code.gitea.io/gitea/modules/setting" "github.com/Unknwon/com" "github.com/go-xorm/builder" "github.com/go-xorm/xorm" @@ -21,8 +18,11 @@ import ( api "code.gitea.io/sdk/gitea" "code.gitea.io/git" + "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/markup" + "code.gitea.io/gitea/modules/markup/markdown" + "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" ) @@ -128,13 +128,15 @@ type Comment struct { CreatedUnix util.TimeStamp `xorm:"INDEX created"` UpdatedUnix util.TimeStamp `xorm:"INDEX updated"` - // Reference issue in commit message, comments, issues, or pull requests - RefExists bool `xorm:"-"` - RefIssue *Issue `xorm:"-"` - RefComment *Comment `xorm:"-"` - RefMessage string `xorm:"-"` - RefURL string `xorm:"-"` - // the commit SHA for commit refs otherwise a SHA of a unique reference identifier + // Reference issue in commit, comments, issues, or pull requests + RefExists bool `xorm:"-"` + RefIssue *Issue `xorm:"-"` + RefComment *Comment `xorm:"-"` + RefCommitMessage string `xorm:"-"` + RefCommitShortSHA string `xorm:"-"` + RefURL string `xorm:"-"` + // CommentType*Ref types use this as a unique reference identifier to prevent duplicate references + // other types use it for the commit SHA CommitSHA string `xorm:"VARCHAR(40)"` Attachments []*Attachment `xorm:"-"` @@ -285,56 +287,39 @@ func (c *Comment) LoadReference() error { return err } - pullOrIssue := "issues" - if refIssue.IsPull { - pullOrIssue = "pulls" - } - c.RefIssue = refIssue - c.RefURL = fmt.Sprintf("%s/%s/%d", refIssue.Repo.Link(), pullOrIssue, refIssue.Index) + c.RefURL = refIssue.HTMLURL() c.RefExists = true } } else if c.Type == CommentTypeCommitRef { - if strings.HasPrefix(c.Content, ``) - contentParts := strings.SplitN(content, `">`, 2) - - if len(contentParts) == 2 { - c.RefURL = contentParts[0] - c.RefMessage = contentParts[1] - c.RefExists = true + contentParts := strings.SplitN(c.Content, " ", 2) + if len(contentParts) == 2 { + var repoID int64 + n, err := fmt.Sscanf(contentParts[0], "%d", &repoID) + if err != nil { + return err } - } else { - // this is a new style commit ref - contentParts := strings.SplitN(c.Content, " ", 2) - if len(contentParts) == 2 { - var repoID int64 - n, err := fmt.Sscanf(contentParts[0], "%d", &repoID) + + if n == 1 { + refRepo, err := GetRepositoryByID(repoID) if err != nil { return err } - if n == 1 { - refRepo, err := GetRepositoryByID(repoID) - if err != nil { - return err - } - - gitRepo, err := git.OpenRepository(refRepo.RepoPath()) - if err != nil { - return err - } - - refCommit, err := gitRepo.GetCommit(contentParts[1][:40]) - if err != nil { - return err - } - - c.RefURL = fmt.Sprintf("%s/commit/%s", refRepo.Link(), refCommit.ID.String()) - c.RefMessage = refCommit.CommitMessage - c.RefExists = true + gitRepo, err := git.OpenRepository(refRepo.RepoPath()) + if err != nil { + return err + } + + refCommit, err := gitRepo.GetCommit(contentParts[1][:40]) + if err != nil { + return err } + + c.RefCommitMessage = refCommit.CommitMessage + c.RefCommitShortSHA = base.TruncateString(refCommit.ID.String(), 10) + c.RefURL = fmt.Sprintf("%s/commit/%s", refRepo.Link(), refCommit.ID.String()) + c.RefExists = true } } } else if c.Type == CommentTypeCommentRef { @@ -355,14 +340,9 @@ func (c *Comment) LoadReference() error { return err } - pullOrIssue := "issues" - if refIssue.IsPull { - pullOrIssue = "pulls" - } - c.RefIssue = refIssue c.RefComment = refComment - c.RefURL = fmt.Sprintf("%s/%s/%d#%s", refIssue.Repo.Link(), pullOrIssue, refIssue.Index, refComment.HashTag()) + c.RefURL = refComment.HTMLURL() c.RefExists = true } } @@ -883,7 +863,9 @@ func CreateComment(opts *CreateCommentOptions) (comment *Comment, err error) { if opts.Type == CommentTypeComment { UpdateIssueIndexer(opts.Issue.ID) + } + if opts.Type == CommentTypeComment || opts.Type == CommentTypeCode { if err = CreateOrUpdateCommentAction(comment.Poster, opts.Repo, opts.Issue, comment); err != nil { return nil, err } @@ -973,7 +955,7 @@ func CreateCodeComment(doer *User, repo *Repository, issue *Issue, content, tree } // createRefComment creates a commit, comment, issue, or pull request reference comment to issue. -func createRefComment(doer *User, repo *Repository, issue *Issue, content, refSHA string, commentType CommentType) error { +func createRefComment(doer *User, issue *Issue, content string, refSHA string, commentType CommentType) error { if len(refSHA) == 0 { return fmt.Errorf("cannot create reference with empty SHA") } @@ -993,7 +975,7 @@ func createRefComment(doer *User, repo *Repository, issue *Issue, content, refSH _, err = CreateComment(&CreateCommentOptions{ Type: commentType, Doer: doer, - Repo: repo, + Repo: issue.Repo, Issue: issue, CommitSHA: refSHA, Content: content, @@ -1002,23 +984,27 @@ func createRefComment(doer *User, repo *Repository, issue *Issue, content, refSH } // CreateCommitRefComment creates a commit reference comment to issue. -func CreateCommitRefComment(doer *User, repo *Repository, issue *Issue, content, commitSHA string) error { - return createRefComment(doer, repo, issue, content, commitSHA, CommentTypeCommitRef) +func CreateCommitRefComment(doer *User, issue *Issue, commitRepoID int64, commitSHA string) error { + content := fmt.Sprintf("%d %s", commitRepoID, commitSHA) + return createRefComment(doer, issue, content, base.EncodeSha1(content), CommentTypeCommitRef) } // CreateCommentRefComment creates a comment reference comment to issue. -func CreateCommentRefComment(doer *User, repo *Repository, issue *Issue, content, refSHA string) error { - return createRefComment(doer, repo, issue, content, refSHA, CommentTypeCommentRef) +func CreateCommentRefComment(doer *User, issue *Issue, commentIssueID int64, commentID int64) error { + content := fmt.Sprintf("%d", commentID) + return createRefComment(doer, issue, content, base.EncodeSha1(content), CommentTypeCommentRef) } // CreateIssueRefComment creates a comment reference comment to issue. -func CreateIssueRefComment(doer *User, repo *Repository, issue *Issue, content, refSHA string) error { - return createRefComment(doer, repo, issue, content, refSHA, CommentTypeIssueRef) +func CreateIssueRefComment(doer *User, issue *Issue, commentIssueID int64) error { + content := fmt.Sprintf(`%d`, commentIssueID) + return createRefComment(doer, issue, content, base.EncodeSha1(content), CommentTypeIssueRef) } // CreatePullRefComment creates a comment reference comment to issue. -func CreatePullRefComment(doer *User, repo *Repository, issue *Issue, content, refSHA string) error { - return createRefComment(doer, repo, issue, content, refSHA, CommentTypePullRef) +func CreatePullRefComment(doer *User, issue *Issue, commentIssueID int64) error { + content := fmt.Sprintf(`%d`, commentIssueID) + return createRefComment(doer, issue, content, base.EncodeSha1(content), CommentTypePullRef) } // GetCommentByID returns the comment by given ID. diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 49702094b783b..e04298b66bf73 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -104,7 +104,7 @@
- {{.RefMessage | Escape}} ({{ShortSha .CommitSHA}}) + {{.RefCommitMessage | Escape}} ({{.RefCommitShortSHA | Escape}})
{{else if and (eq .Type 5) .RefExists}} @@ -282,7 +282,7 @@
- #{{.DependentIssue.Index}} {{.DependentIssue.Title}} + {{.DependentIssue.Title | Escape}} (#{{.DependentIssue.Index}})
{{else if eq .Type 20}} @@ -296,7 +296,7 @@
- #{{.DependentIssue.Index}} {{.DependentIssue.Title}} + {{.DependentIssue.Title | Escape}} (#{{.DependentIssue.Index}})
{{else if eq .Type 22}} From 65efac56e8e9ee9bca98f155191633923416eae8 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Wed, 15 Aug 2018 12:04:01 -0400 Subject: [PATCH 26/32] Update issue referencing comment tests --- models/action_test.go | 431 ++++++++++++++++-------------------------- 1 file changed, 158 insertions(+), 273 deletions(-) diff --git a/models/action_test.go b/models/action_test.go index 14d116874a4dc..b962cdc2830ae 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -199,25 +199,29 @@ func TestUpdateIssuesCommentIssues(t *testing.T) { repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) repo.Owner = user - commentIssue := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}).(*Issue) - refIssue1 := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}).(*Issue) + commentIssue := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}).(*Issue) + refIssue1 := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}).(*Issue) refIssue2 := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}).(*Issue) + assert.EqualValues(t, true, commentIssue.IsPull) + assert.EqualValues(t, false, refIssue1.IsClosed) + assert.EqualValues(t, false, refIssue2.IsClosed) + // TODO: comments are loaded and then this doesnt work... commentBean := []*Comment{ { - Type: CommentTypeIssueRef, + Type: CommentTypePullRef, CommitSHA: base.EncodeSha1(fmt.Sprintf("%d", commentIssue.ID)), PosterID: user.ID, IssueID: commentIssue.ID, }, { - Type: CommentTypeIssueRef, + Type: CommentTypePullRef, CommitSHA: base.EncodeSha1(fmt.Sprintf("%d", commentIssue.ID)), PosterID: user.ID, IssueID: refIssue1.ID, }, { - Type: CommentTypeIssueRef, + Type: CommentTypePullRef, CommitSHA: base.EncodeSha1(fmt.Sprintf("%d", commentIssue.ID)), PosterID: user.ID, IssueID: refIssue2.ID, @@ -225,49 +229,49 @@ func TestUpdateIssuesCommentIssues(t *testing.T) { } // test issue/pull request closing multiple issues - commentIssue.Title = "close #2" + commentIssue.Title = "close #1" commentIssue.Content = "close #3" AssertNotExistsBean(t, commentBean[0]) AssertNotExistsBean(t, commentBean[1]) AssertNotExistsBean(t, commentBean[2]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) assert.NoError(t, UpdateIssuesComment(user, repo, commentIssue, nil, canOpenClose)) AssertNotExistsBean(t, commentBean[0]) AssertExistsAndLoadBean(t, commentBean[1]) AssertExistsAndLoadBean(t, commentBean[2]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isClosed) AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isClosed) CheckConsistencyFor(t, &Action{}) // test issue/pull request re-opening multiple issues - commentIssue.Title = "reopen #2" + commentIssue.Title = "reopen #1" commentIssue.Content = "reopen #3" AssertNotExistsBean(t, commentBean[0]) AssertExistsAndLoadBean(t, commentBean[1]) AssertExistsAndLoadBean(t, commentBean[2]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isClosed) AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isClosed) assert.NoError(t, UpdateIssuesComment(user, repo, commentIssue, nil, canOpenClose)) AssertNotExistsBean(t, commentBean[0]) AssertExistsAndLoadBean(t, commentBean[1]) AssertExistsAndLoadBean(t, commentBean[2]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) CheckConsistencyFor(t, &Action{}) // test issue/pull request mixing re-opening and closing issue and self-referencing issue - commentIssue.Title = "reopen #2" - commentIssue.Content = "close #2 and reference #1" + commentIssue.Title = "reopen #3" + commentIssue.Content = "close #3 and reference #2" AssertNotExistsBean(t, commentBean[0]) AssertExistsAndLoadBean(t, commentBean[1]) AssertExistsAndLoadBean(t, commentBean[2]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) assert.NoError(t, UpdateIssuesComment(user, repo, commentIssue, nil, canOpenClose)) AssertNotExistsBean(t, commentBean[0]) AssertExistsAndLoadBean(t, commentBean[1]) AssertExistsAndLoadBean(t, commentBean[2]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) CheckConsistencyFor(t, &Action{}) } } @@ -280,9 +284,12 @@ func TestUpdateIssuesCommentComments(t *testing.T) { repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) repo.Owner = user - commentIssue := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}).(*Issue) - refIssue1 := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}).(*Issue) + commentIssue := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}).(*Issue) + refIssue1 := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}).(*Issue) refIssue2 := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}).(*Issue) + assert.EqualValues(t, true, commentIssue.IsPull) + assert.EqualValues(t, false, refIssue1.IsClosed) + assert.EqualValues(t, false, refIssue2.IsClosed) comment := Comment{ ID: 123456789, @@ -290,60 +297,91 @@ func TestUpdateIssuesCommentComments(t *testing.T) { PosterID: user.ID, Poster: user, IssueID: commentIssue.ID, - Content: "this is a comment that mentions #2 and #1 too", + Content: "", } commentBean := []*Comment{ { Type: CommentTypeCommentRef, - CommitSHA: base.EncodeSha1(fmt.Sprintf("%d@%d", commentIssue.ID, comment.ID)), + CommitSHA: base.EncodeSha1(fmt.Sprintf("%d", comment.ID)), PosterID: user.ID, IssueID: commentIssue.ID, }, { Type: CommentTypeCommentRef, - CommitSHA: base.EncodeSha1(fmt.Sprintf("%d@%d", commentIssue.ID, comment.ID)), + CommitSHA: base.EncodeSha1(fmt.Sprintf("%d", comment.ID)), PosterID: user.ID, IssueID: refIssue1.ID, }, { Type: CommentTypeCommentRef, - CommitSHA: base.EncodeSha1(fmt.Sprintf("%d@%d", commentIssue.ID, comment.ID)), + CommitSHA: base.EncodeSha1(fmt.Sprintf("%d", comment.ID)), PosterID: user.ID, IssueID: refIssue2.ID, }, } // test comment referencing issue including self-referencing + comment.Content = "this is a comment that mentions #1 and #2 too" AssertNotExistsBean(t, commentBean[0]) AssertNotExistsBean(t, commentBean[1]) AssertNotExistsBean(t, commentBean[2]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) assert.NoError(t, UpdateIssuesComment(user, repo, commentIssue, &comment, false)) AssertNotExistsBean(t, commentBean[0]) AssertExistsAndLoadBean(t, commentBean[1]) AssertNotExistsBean(t, commentBean[2]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) CheckConsistencyFor(t, &Action{}) // test comment updating issue reference - comment.Content = "this is a comment that mentions #2 and #3 too" + comment.Content = "this is a comment that mentions #1 and #3 too" AssertNotExistsBean(t, commentBean[0]) AssertExistsAndLoadBean(t, commentBean[1]) AssertNotExistsBean(t, commentBean[2]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) assert.NoError(t, UpdateIssuesComment(user, repo, commentIssue, &comment, false)) AssertNotExistsBean(t, commentBean[0]) AssertExistsAndLoadBean(t, commentBean[1]) AssertExistsAndLoadBean(t, commentBean[2]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 3}, isOpen) CheckConsistencyFor(t, &Action{}) } +type Message struct { + Message string + Sha1 string + IssueID int64 +} + +func preparePushCommits(userID int64, repoID int64, messages []*Message) ([]*PushCommit, []*Comment) { + var pushCommits []*PushCommit + var commentBean []*Comment + + for _, message := range messages { + pushCommits = append(pushCommits, &PushCommit{ + Sha1: message.Sha1, + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: message.Message, + }) + commentBean = append(commentBean, &Comment{ + Type: CommentTypeCommitRef, + CommitSHA: base.EncodeSha1(fmt.Sprintf("%d %s", repoID, message.Sha1)), + PosterID: userID, + IssueID: message.IssueID, + }) + } + + return pushCommits, commentBean +} + func TestUpdateIssuesCommit(t *testing.T) { for _, commitsAreMerged := range []bool{false, true} { // if commits were not merged then issue should not change status @@ -358,355 +396,202 @@ func TestUpdateIssuesCommit(t *testing.T) { repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) repo.Owner = user + commitIssue := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}).(*Issue) + refIssue := AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}).(*Issue) + assert.EqualValues(t, true, commitIssue.IsPull) + assert.EqualValues(t, false, refIssue.IsClosed) + // test re-open of already open issue - pushCommits := []*PushCommit{ - { - Sha1: "abcdef1", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "reoopen #2", - }, - } - commentBean := []*Comment{ + pushCommits, commentBean := preparePushCommits(user.ID, repo.ID, []*Message{ { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[0].Sha1, - PosterID: user.ID, - IssueID: 2, + Message: "reopen #1", + Sha1: "abcdef1", + IssueID: refIssue.ID, }, - } + }) AssertNotExistsBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) AssertExistsAndLoadBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) CheckConsistencyFor(t, &Action{}) // test simultaneous open and close on an already open issue - pushCommits = []*PushCommit{ - { - Sha1: "abcdef2", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "reopen #2 and the close #2", - }, - } - commentBean = []*Comment{ + pushCommits, commentBean = preparePushCommits(user.ID, repo.ID, []*Message{ { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[0].Sha1, - PosterID: user.ID, - IssueID: 2, + Message: "reopen #1 and then close #1", + Sha1: "abcdef2", + IssueID: refIssue.ID, }, - } + }) AssertNotExistsBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) AssertExistsAndLoadBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) CheckConsistencyFor(t, &Action{}) // test close of an open issue - pushCommits = []*PushCommit{ + pushCommits, commentBean = preparePushCommits(user.ID, repo.ID, []*Message{ { - Sha1: "abcdef3", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "closes #2", + Message: "closes #1", + Sha1: "abcdef3", + IssueID: refIssue.ID, }, - } - commentBean = []*Comment{ - { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[0].Sha1, - PosterID: user.ID, - IssueID: 2, - }, - } + }) AssertNotExistsBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) AssertExistsAndLoadBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isClosed) CheckConsistencyFor(t, &Action{}) // test close of an already closed issue - pushCommits = []*PushCommit{ + pushCommits, commentBean = preparePushCommits(user.ID, repo.ID, []*Message{ { - Sha1: "abcdef4", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "close #2", + Message: "closes #1", + Sha1: "abcdef4", + IssueID: refIssue.ID, }, - } - commentBean = []*Comment{ - { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[0].Sha1, - PosterID: user.ID, - IssueID: 2, - }, - } + }) AssertNotExistsBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isClosed) assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) AssertExistsAndLoadBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isClosed) CheckConsistencyFor(t, &Action{}) // test simultaneous open and close on a closed issue - pushCommits = []*PushCommit{ - { - Sha1: "abcdef5", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "close #2 and reopen #2", - }, - } - commentBean = []*Comment{ + pushCommits, commentBean = preparePushCommits(user.ID, repo.ID, []*Message{ { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[0].Sha1, - PosterID: user.ID, - IssueID: 2, + Message: "close #1 and reopen #1", + Sha1: "abcdef5", + IssueID: refIssue.ID, }, - } + }) AssertNotExistsBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isClosed) assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) AssertExistsAndLoadBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isClosed) CheckConsistencyFor(t, &Action{}) // test referencing an closed issue - pushCommits = []*PushCommit{ + pushCommits, commentBean = preparePushCommits(user.ID, repo.ID, []*Message{ { - Sha1: "abcdef6", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "for details on how to open, see #2", + Message: "for details on how to open, see #1", + Sha1: "abcdef6", + IssueID: refIssue.ID, }, - } - commentBean = []*Comment{ - { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[0].Sha1, - PosterID: user.ID, - IssueID: 2, - }, - } + }) AssertNotExistsBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isClosed) assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) AssertExistsAndLoadBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isClosed) CheckConsistencyFor(t, &Action{}) // test re-open a closed issue - pushCommits = []*PushCommit{ + pushCommits, commentBean = preparePushCommits(user.ID, repo.ID, []*Message{ { - Sha1: "abcdef7", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "reopens #2", + Message: "reopens #1", + Sha1: "abcdef7", + IssueID: refIssue.ID, }, - } - commentBean = []*Comment{ - { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[0].Sha1, - PosterID: user.ID, - IssueID: 2, - }, - } + }) AssertNotExistsBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isClosed) assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) AssertExistsAndLoadBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) CheckConsistencyFor(t, &Action{}) // test referencing an open issue - pushCommits = []*PushCommit{ + pushCommits, commentBean = preparePushCommits(user.ID, repo.ID, []*Message{ { - Sha1: "abcdef8", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "for details on how to close, see #2", + Message: "for details on how to close, see #1", + Sha1: "abcdef8", + IssueID: refIssue.ID, }, - } - commentBean = []*Comment{ - { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[0].Sha1, - PosterID: user.ID, - IssueID: 2, - }, - } + }) AssertNotExistsBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) AssertExistsAndLoadBean(t, commentBean[0]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) CheckConsistencyFor(t, &Action{}) // test close-then-open commit order - pushCommits = []*PushCommit{ - { - Sha1: "abcdef10", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "reopened #2", - }, - { - Sha1: "abcdef9", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "fixes #2", - }, - } - commentBean = []*Comment{ + pushCommits, commentBean = preparePushCommits(user.ID, repo.ID, []*Message{ { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[0].Sha1, - PosterID: user.ID, - IssueID: 2, + Message: "reopened #1", + Sha1: "abcdef10", + IssueID: refIssue.ID, }, { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[1].Sha1, - PosterID: user.ID, - IssueID: 2, + Message: "fixes #1", + Sha1: "abcdef9", + IssueID: refIssue.ID, }, - } + }) AssertNotExistsBean(t, commentBean[0]) AssertNotExistsBean(t, commentBean[1]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) AssertExistsAndLoadBean(t, commentBean[0]) AssertExistsAndLoadBean(t, commentBean[1]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) CheckConsistencyFor(t, &Action{}) // test open-then-close commit order - pushCommits = []*PushCommit{ + pushCommits, commentBean = preparePushCommits(user.ID, repo.ID, []*Message{ { - Sha1: "abcdef12", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "resolved #2", + Message: "resolved #1", + Sha1: "abcdef12", + IssueID: refIssue.ID, }, { - Sha1: "abcdef11", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "reopened #2", + Message: "reopened #1", + Sha1: "abcdef11", + IssueID: refIssue.ID, }, - } - commentBean = []*Comment{ - { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[0].Sha1, - PosterID: user.ID, - IssueID: 2, - }, - { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[1].Sha1, - PosterID: user.ID, - IssueID: 2, - }, - } + }) AssertNotExistsBean(t, commentBean[0]) AssertNotExistsBean(t, commentBean[1]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) AssertExistsAndLoadBean(t, commentBean[0]) AssertExistsAndLoadBean(t, commentBean[1]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isClosed) CheckConsistencyFor(t, &Action{}) // test more complex commit pattern - pushCommits = []*PushCommit{ - { - Sha1: "abcdef15", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user4@example.com", - AuthorName: "User Four", - Message: "start working on #FST-1, #1", - }, + pushCommits, commentBean = preparePushCommits(user.ID, repo.ID, []*Message{ { - Sha1: "abcdef14", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "reopen #2", + Message: "start working on #FST-1, #2", + Sha1: "abcdef15", + IssueID: commitIssue.ID, }, { - Sha1: "abcdef13", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "close #2", - }, - } - commentBean = []*Comment{ - { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[0].Sha1, - PosterID: user.ID, - IssueID: 1, - }, - { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[1].Sha1, - PosterID: user.ID, - IssueID: 2, + Message: "reopen #1", + Sha1: "abcdef14", + IssueID: refIssue.ID, }, { - Type: CommentTypeCommitRef, - CommitSHA: pushCommits[2].Sha1, - PosterID: user.ID, - IssueID: 2, + Message: "close #1", + Sha1: "abcdef13", + IssueID: refIssue.ID, }, - } - + }) AssertNotExistsBean(t, commentBean[0]) AssertNotExistsBean(t, commentBean[1]) AssertNotExistsBean(t, commentBean[2]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isClosed) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isClosed) assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, commitsAreMerged)) AssertExistsAndLoadBean(t, commentBean[0]) AssertExistsAndLoadBean(t, commentBean[1]) AssertExistsAndLoadBean(t, commentBean[2]) - AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 2}, isOpen) + AssertExistsAndLoadBean(t, &Issue{RepoID: repo.ID, Index: 1}, isOpen) CheckConsistencyFor(t, &Action{}) } } From 0da27367f92e52bf5fd92bcdf18fc643aea2dffd Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Thu, 16 Aug 2018 11:21:33 -0400 Subject: [PATCH 27/32] Push pull request commits to base before trying to use them --- models/pull.go | 4 ++++ routers/api/v1/repo/pull.go | 3 --- routers/repo/pull.go | 3 --- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/models/pull.go b/models/pull.go index 7f370c31e04b2..e49e3cf920532 100644 --- a/models/pull.go +++ b/models/pull.go @@ -782,6 +782,10 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str return fmt.Errorf("Commit: %v", err) } + if err := pr.PushToBaseRepo(); err != nil { + return fmt.Errorf("PushToBaseRepo: %v", err) + } + UpdateIssueIndexer(pull.ID) pr.Issue = pull diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 9fbadcd076682..5ac1fce5946f2 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -294,9 +294,6 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption } ctx.Error(500, "NewPullRequest", err) return - } else if err := pr.PushToBaseRepo(); err != nil { - ctx.Error(500, "PushToBaseRepo", err) - return } notification.NotifyNewPullRequest(pr) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 4adfb96e748be..dd23ed2aa0b37 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -894,9 +894,6 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) } ctx.ServerError("NewPullRequest", err) return - } else if err := pullRequest.PushToBaseRepo(); err != nil { - ctx.ServerError("PushToBaseRepo", err) - return } notification.NotifyNewPullRequest(pullRequest) From 05c469115105f3bd72ed107441080f34fcfc8b72 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Fri, 17 Aug 2018 11:11:24 -0400 Subject: [PATCH 28/32] Don't exit early in NewPullRequest --- models/pull.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/models/pull.go b/models/pull.go index e49e3cf920532..e4e2b6bce5c38 100644 --- a/models/pull.go +++ b/models/pull.go @@ -800,26 +800,24 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str baseGitRepo *git.Repository ) if baseBranch, err = pr.BaseRepo.GetBranch(pr.BaseBranch); err != nil { - return nil + return fmt.Errorf("GetBranch: %v", err) } if baseCommit, err = baseBranch.GetCommit(); err != nil { - return nil + return fmt.Errorf("GetCommit: %v", err) } if headBranch, err = pr.HeadRepo.GetBranch(pr.HeadBranch); err != nil { - return nil + return fmt.Errorf("GetBranch: %v", err) } if headCommit, err = headBranch.GetCommit(); err != nil { - return nil + return fmt.Errorf("GetCommit: %v", err) } if baseGitRepo, err = git.OpenRepository(pr.BaseRepo.RepoPath()); err != nil { - log.Error(4, "OpenRepository", err) - return nil + return fmt.Errorf("OpenRepository: %v", err) } l, err := baseGitRepo.CommitsBetweenIDs(headCommit.ID.String(), baseCommit.ID.String()) if err != nil { - log.Error(4, "CommitsBetweenIDs: %v", err) - return nil + return fmt.Errorf("CommitsBetweenIDs: %v", err) } commits := ListToPushCommits(l) From 18e63c19c060e048ebc32553e845adb733ae079f Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Mon, 20 Aug 2018 10:16:33 -0400 Subject: [PATCH 29/32] Complete addition of code comments and review issue referencing comments --- models/action.go | 12 ++- models/issue_comment.go | 101 +++++++++++------- options/locale/locale_en-US.ini | 2 + routers/repo/issue.go | 7 +- .../repo/issue/view_content/comments.tmpl | 26 +++++ 5 files changed, 106 insertions(+), 42 deletions(-) diff --git a/models/action.go b/models/action.go index 2b880066dedd6..17cb4dc2acf61 100644 --- a/models/action.go +++ b/models/action.go @@ -499,10 +499,14 @@ func changeIssueStatus(mask KeywordMaskType, doer *User, issue *Issue) error { return nil } -// UpdateIssuesComment checks if issues are manipulated by a comment +// UpdateIssuesComment checks if issues are manipulated by a regular comment, code comment, or review comment (also issue/pull request title and description) func UpdateIssuesComment(doer *User, repo *Repository, commentIssue *Issue, comment *Comment, canOpenClose bool) error { var refString string if comment != nil { + if comment.Type != CommentTypeComment && comment.Type != CommentTypeCode && comment.Type != CommentTypeReview { + return nil + } + refString = comment.Content } else { refString = commentIssue.Title + ": " + commentIssue.Content @@ -523,8 +527,12 @@ func UpdateIssuesComment(doer *User, repo *Repository, commentIssue *Issue, comm } if (mask & KeywordReference) == KeywordReference { - if comment != nil { + if comment != nil && comment.Type == CommentTypeComment { err = CreateCommentRefComment(doer, issue, commentIssue.ID, comment.ID) + } else if comment != nil && comment.Type == CommentTypeCode { + err = CreateCodeRefComment(doer, issue, commentIssue.ID, comment.ID) + } else if comment != nil && comment.Type == CommentTypeReview { + err = CreateReviewRefComment(doer, issue, commentIssue.ID, comment.ID) } else if commentIssue.IsPull { err = CreatePullRefComment(doer, issue, commentIssue.ID) } else { diff --git a/models/issue_comment.go b/models/issue_comment.go index ece7a4d6b10b6..d7102175e5dda 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -37,50 +37,54 @@ const ( // Enumerate all the comment types const ( // Plain comment, can be associated with a commit (CommitID > 0) and a line (LineNum > 0) - CommentTypeComment CommentType = iota - CommentTypeReopen - CommentTypeClose + CommentTypeComment CommentType = iota // 0 + CommentTypeReopen // 1 + CommentTypeClose // 2 // References. - CommentTypeIssueRef + CommentTypeIssueRef // 3 // Reference from a commit (not part of a pull request) - CommentTypeCommitRef + CommentTypeCommitRef // 4 // Reference from a comment - CommentTypeCommentRef + CommentTypeCommentRef // 5 // Reference from a pull request - CommentTypePullRef + CommentTypePullRef // 6 // Labels changed - CommentTypeLabel + CommentTypeLabel // 7 // Milestone changed - CommentTypeMilestone + CommentTypeMilestone // 8 // Assignees changed - CommentTypeAssignees + CommentTypeAssignees // 9 // Change Title - CommentTypeChangeTitle + CommentTypeChangeTitle // 10 // Delete Branch - CommentTypeDeleteBranch + CommentTypeDeleteBranch // 11 // Start a stopwatch for time tracking - CommentTypeStartTracking + CommentTypeStartTracking // 12 // Stop a stopwatch for time tracking - CommentTypeStopTracking + CommentTypeStopTracking // 13 // Add time manual for time tracking - CommentTypeAddTimeManual + CommentTypeAddTimeManual // 14 // Cancel a stopwatch for time tracking - CommentTypeCancelTracking + CommentTypeCancelTracking // 15 // Added a due date - CommentTypeAddedDeadline + CommentTypeAddedDeadline // 16 // Modified the due date - CommentTypeModifiedDeadline + CommentTypeModifiedDeadline // 17 // Removed a due date - CommentTypeRemovedDeadline + CommentTypeRemovedDeadline // 18 // Dependency added - CommentTypeAddDependency + CommentTypeAddDependency // 19 //Dependency removed - CommentTypeRemoveDependency + CommentTypeRemoveDependency // 20 // Comment a line of code - CommentTypeCode + CommentTypeCode // 21 // Reviews a pull request by giving general feedback - CommentTypeReview + CommentTypeReview // 22 + // Reference from a code review + CommentTypeReviewRef // 23 + // Reference from a code comment + CommentTypeCodeRef // 24 ) // CommentTag defines comment tag type @@ -272,7 +276,7 @@ func (c *Comment) EventTag() string { return "event-" + com.ToStr(c.ID) } -// LoadReference if comment.Type is CommentType{Issue,Commit,Comment,Pull}Ref, then load RefIssue, RefComment +// LoadReference if comment.Type is CommentType{Issue,Commit,Comment,Pull,Review,Code}Ref, then load RefIssue, RefComment, etc. func (c *Comment) LoadReference() error { if c.Type == CommentTypeIssueRef || c.Type == CommentTypePullRef { var issueID int64 @@ -322,7 +326,7 @@ func (c *Comment) LoadReference() error { c.RefExists = true } } - } else if c.Type == CommentTypeCommentRef { + } else if c.Type == CommentTypeCommentRef || c.Type == CommentTypeReviewRef || c.Type == CommentTypeCodeRef { var commentID int64 n, err := fmt.Sscanf(c.Content, "%d", &commentID) if err != nil { @@ -865,11 +869,12 @@ func CreateComment(opts *CreateCommentOptions) (comment *Comment, err error) { UpdateIssueIndexer(opts.Issue.ID) } - if opts.Type == CommentTypeComment || opts.Type == CommentTypeCode { + if opts.Type == CommentTypeComment || opts.Type == CommentTypeCode || opts.Type == CommentTypeReview { if err = CreateOrUpdateCommentAction(comment.Poster, opts.Repo, opts.Issue, comment); err != nil { return nil, err } } + return comment, nil } @@ -991,22 +996,37 @@ func CreateCommitRefComment(doer *User, issue *Issue, commitRepoID int64, commit // CreateCommentRefComment creates a comment reference comment to issue. func CreateCommentRefComment(doer *User, issue *Issue, commentIssueID int64, commentID int64) error { - content := fmt.Sprintf("%d", commentID) - return createRefComment(doer, issue, content, base.EncodeSha1(content), CommentTypeCommentRef) + return createCommentRefComment(doer, issue, commentIssueID, commentID, CommentTypeCommentRef) } -// CreateIssueRefComment creates a comment reference comment to issue. +// CreateIssueRefComment creates an issue reference comment to issue. func CreateIssueRefComment(doer *User, issue *Issue, commentIssueID int64) error { content := fmt.Sprintf(`%d`, commentIssueID) return createRefComment(doer, issue, content, base.EncodeSha1(content), CommentTypeIssueRef) } -// CreatePullRefComment creates a comment reference comment to issue. +// CreatePullRefComment creates a pull request reference comment to issue. func CreatePullRefComment(doer *User, issue *Issue, commentIssueID int64) error { content := fmt.Sprintf(`%d`, commentIssueID) return createRefComment(doer, issue, content, base.EncodeSha1(content), CommentTypePullRef) } +// CreateReviewRefComment creates a review reference comment to issue. +func CreateReviewRefComment(doer *User, issue *Issue, commentIssueID int64, commentID int64) error { + return createCommentRefComment(doer, issue, commentIssueID, commentID, CommentTypeReviewRef) +} + +// CreateCodeRefComment creates a code comment reference comment to issue. +func CreateCodeRefComment(doer *User, issue *Issue, commentIssueID int64, commentID int64) error { + return createCommentRefComment(doer, issue, commentIssueID, commentID, CommentTypeCodeRef) +} + +// createCommentRefComment is the common implementation for Create{Comment,Review,Code}RefComment functions +func createCommentRefComment(doer *User, issue *Issue, commentIssueID int64, commentID int64, commentType CommentType) error { + content := fmt.Sprintf(`%d`, commentID) + return createRefComment(doer, issue, content, base.EncodeSha1(content), commentType) +} + // GetCommentByID returns the comment by given ID. func GetCommentByID(id int64) (*Comment, error) { c := new(Comment) @@ -1097,15 +1117,6 @@ func UpdateComment(doer *User, c *Comment, oldContent string) error { return err } else if c.Type == CommentTypeComment { UpdateIssueIndexer(c.IssueID) - - issue, err := GetIssueByID(c.IssueID) - if err != nil { - return err - } - - if err = CreateOrUpdateCommentAction(c.Poster, issue.Repo, issue, c); err != nil { - return err - } } if err := c.LoadIssue(); err != nil { @@ -1116,6 +1127,18 @@ func UpdateComment(doer *User, c *Comment, oldContent string) error { } mode, _ := AccessLevel(doer, c.Issue.Repo) + + issue, err := GetIssueByID(c.IssueID) + if err != nil { + return err + } + + if c.Type == CommentTypeComment || c.Type == CommentTypeCode || c.Type == CommentTypeReview { + if err = CreateOrUpdateCommentAction(c.Poster, issue.Repo, issue, c); err != nil { + return err + } + } + if err := PrepareWebhooks(c.Issue.Repo, HookEventIssueComment, &api.IssueCommentPayload{ Action: api.HookIssueCommentEdited, Issue: c.Issue.APIFormat(), diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 02a65f66c84b4..b2219754f86f3 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -740,6 +740,8 @@ issues.issue_ref_at = `referenced this issue from an issue %[2]s` issues.comment_ref_at = `referenced this issue from a comment %[2]s` issues.pull_ref_at = `referenced this issue from a pull request %[2]s` +issues.review_ref_at = `referenced this issue from a review of a pull request %[2]s` +issues.code_ref_at = `referenced this issue from a comment on a pull request diff %[2]s` issues.poster = Poster issues.collaborator = Collaborator issues.owner = Owner diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 2fcc457a090e0..716414e23d488 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -725,7 +725,12 @@ func ViewIssue(ctx *context.Context) { if !isAdded && !issue.IsPoster(comment.Poster.ID) { participants = append(participants, comment.Poster) } - } else if comment.Type == models.CommentTypeIssueRef || comment.Type == models.CommentTypeCommitRef || comment.Type == models.CommentTypeCommentRef || comment.Type == models.CommentTypePullRef { + } else if comment.Type == models.CommentTypeIssueRef || + comment.Type == models.CommentTypeCommitRef || + comment.Type == models.CommentTypeCommentRef || + comment.Type == models.CommentTypePullRef || + comment.Type == models.CommentTypeReviewRef || + comment.Type == models.CommentTypeCodeRef { if err = comment.LoadReference(); err != nil { continue } diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index e04298b66bf73..4138da68cd306 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -133,6 +133,32 @@ {{.RefIssue.Title | Escape}} (#{{.RefIssue.Index}}) + {{else if and (eq .Type 23) .RefExists}} +
+ + + + + {{.Poster.Name}} {{$.i18n.Tr "repo.issues.review_ref_at" .EventTag $createdStr | Safe}} + +
+ + {{.RefIssue.Title | Escape}} (#{{.RefIssue.Index}}) +
+
+ {{else if and (eq .Type 24) .RefExists}} +
+ + + + + {{.Poster.Name}} {{$.i18n.Tr "repo.issues.code_ref_at" .EventTag $createdStr | Safe}} + +
+ + {{.RefIssue.Title | Escape}} (#{{.RefIssue.Index}}) +
+
{{else if eq .Type 7}} {{if .Label}}
From ee360eb60467ae88bec7f108a9bc04f297b52dec Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Mon, 20 Aug 2018 12:56:56 -0400 Subject: [PATCH 30/32] Fix to only get commits along head branch --- models/pull.go | 7 ++++++- models/update.go | 8 +++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/models/pull.go b/models/pull.go index e4e2b6bce5c38..15088c8469a5a 100644 --- a/models/pull.go +++ b/models/pull.go @@ -815,7 +815,12 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str return fmt.Errorf("OpenRepository: %v", err) } - l, err := baseGitRepo.CommitsBetweenIDs(headCommit.ID.String(), baseCommit.ID.String()) + mergeBase, err := baseGitRepo.GetMergeBase(baseCommit.ID.String(), headCommit.ID.String()) + if err != nil { + return fmt.Errorf("GetMergeBase: %v", err) + } + + l, err := baseGitRepo.CommitsBetweenIDs(headCommit.ID.String(), mergeBase) if err != nil { return fmt.Errorf("CommitsBetweenIDs: %v", err) } diff --git a/models/update.go b/models/update.go index ffa9cfab1eade..1773d3287eabc 100644 --- a/models/update.go +++ b/models/update.go @@ -328,7 +328,13 @@ func pushUpdate(branch string, opts PushUpdateOptions) (repo *Repository, err er continue } - l, err := headGitRepo.CommitsBetweenIDs(headCommit.ID.String(), baseCommit.ID.String()) + mergeBase, err := headGitRepo.GetMergeBase(baseCommit.ID.String(), headCommit.ID.String()) + if err != nil { + log.Error(4, "GetMergeBase: %v", err) + continue + } + + l, err := headGitRepo.CommitsBetweenIDs(headCommit.ID.String(), mergeBase) if err != nil { log.Error(4, "CommitsBetweenIDs: %v", err) continue From 7e2fbdb4caa7a772239317d99679b79fd3da1591 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Mon, 20 Aug 2018 13:20:24 -0400 Subject: [PATCH 31/32] Code cleanup --- models/issue_comment.go | 1 - 1 file changed, 1 deletion(-) diff --git a/models/issue_comment.go b/models/issue_comment.go index d7102175e5dda..f52241c5dbc8d 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -1127,7 +1127,6 @@ func UpdateComment(doer *User, c *Comment, oldContent string) error { } mode, _ := AccessLevel(doer, c.Issue.Repo) - issue, err := GetIssueByID(c.IssueID) if err != nil { return err From ac30d9493dac197373b4ec7b1ff944f1a2899b84 Mon Sep 17 00:00:00 2001 From: Keith Rutkowski Date: Mon, 20 Aug 2018 12:59:14 -0400 Subject: [PATCH 32/32] Add reformatAndAddReferenceComments migration v75 --- models/migrations/migrations.go | 2 + models/migrations/v75.go | 280 ++++++++++++++++++++++++++++++++ 2 files changed, 282 insertions(+) create mode 100644 models/migrations/v75.go diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 6c28989c2e80a..b62cb9cdbb2a1 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -202,6 +202,8 @@ var migrations = []Migration{ NewMigration("add must_change_password column for users table", addMustChangePassword), // v74 -> v75 NewMigration("add approval whitelists to protected branches", addApprovalWhitelistsToProtectedBranches), + // v75 -> v76 + NewMigration("reformat old-style reference comments, add new-style reference comments", reformatAndAddReferenceComments), } // Migrate database to current version diff --git a/models/migrations/v75.go b/models/migrations/v75.go new file mode 100644 index 0000000000000..eae5cb6c4da1e --- /dev/null +++ b/models/migrations/v75.go @@ -0,0 +1,280 @@ +// Copyright 2018 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "container/list" + "fmt" + "strings" + + "code.gitea.io/git" + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/base" + "code.gitea.io/gitea/modules/util" + "github.com/go-xorm/xorm" +) + +func reformatAndAddReferenceComments(x *xorm.Engine) error { + const batchSize = 100 + + sess := x.NewSession() + defer sess.Close() + + if err := sess.Begin(); err != nil { + return err + } + + var maxCommentID int64 + + // create an repo lookup to map the repo's full name to the repository object + var repos []*models.Repository + repoLookup := make(map[string]*models.Repository) + if err := x.Find(&repos); err != nil { + return err + } + for _, repo := range repos { + repoLookup[repo.FullName()] = repo + } + + // create an issue updated lookup to save the current UpdateUnix values since UpdatedUnix gets updated as we migrate + var issues []*models.Issue + issueUpdatedLookup := make(map[int64]util.TimeStamp) + if err := x.Find(&issues); err != nil { + return err + } + for _, issue := range issues { + issueUpdatedLookup[issue.ID] = issue.UpdatedUnix + } + + // convert or remove old-style commit reference comments + for refCommentStart := 0; ; refCommentStart += batchSize { + refComments := make([]*models.Comment, 0, batchSize) + if err := x.Where("comment.type = ?", models.CommentTypeCommitRef).Limit(batchSize, refCommentStart).Find(&refComments); err != nil { + return err + } + if len(refComments) == 0 { + break + } + + for _, refComment := range refComments { + if err := refComment.LoadReference(); err != nil { + // if load failed, parse the content into a repo link and a commit SHA... + if strings.HasPrefix(refComment.Content, ``) + contentParts := strings.SplitN(content, `">`, 2) + if len(contentParts) == 2 { + linkParts := strings.SplitN(contentParts[0], `/commit/`, 2) + if len(linkParts) == 2 && len(linkParts[1]) == 40 { + repoLinkParts := strings.Split(linkParts[0], "/") + commitSha1 := linkParts[1] + // ...then check if the repo/commit exist... + if len(repoLinkParts) >= 2 { + repoFullName := strings.Join(repoLinkParts[len(repoLinkParts)-2:], "/") + refRepo := repoLookup[repoFullName] + if refRepo != nil { + gitRepo, err := git.OpenRepository(refRepo.RepoPath()) + if err == nil { + refCommit, err := gitRepo.GetCommit(commitSha1) + if err == nil { + // ...and update the ref comment to the new-style + refComment.Content = fmt.Sprintf("%d %s", refRepo.ID, refCommit.ID.String()) + refComment.CommitSHA = base.EncodeSha1(refComment.Content) + + if _, err := x.ID(refComment.ID).AllCols().Update(refComment); err == nil { + // continue if successful in order to skip over the delete below + continue + } + } + } + } + } + } + } + } + + // delete the comment if unable to convert it + if _, err := x.Delete(refComment); err != nil { + return err + } + } + } + } + + // for every issue and every comment, try to process the title+contents as ref only, no open/close action + for issueStart := 0; ; issueStart += batchSize { + issues = make([]*models.Issue, 0, batchSize) + if err := x.Limit(batchSize, issueStart).Find(&issues); err != nil { + return err + } + if len(issues) == 0 { + break + } + + if err := models.IssueList(issues).LoadAttributes(); err != nil { + continue + } + + for _, issue := range issues { + if _, err := x.Table("comment").Select("coalesce(max(comment.id), 0)").Get(&maxCommentID); err != nil { + return err + } + + if err := models.UpdateIssuesComment(issue.Poster, issue.Repo, issue, nil, false); err != nil { + continue + } + + // try to handle the commit messages if this is a pull request + for once := true; once && issue.IsPull; once = false { + var ( + err error + pr *models.PullRequest + baseBranch *models.Branch + headBranch *models.Branch + baseCommit *git.Commit + headCommit *git.Commit + baseGitRepo *git.Repository + ) + + if pr, err = issue.GetPullRequest(); err != nil { + continue + } + + if err = pr.GetBaseRepo(); err != nil { + continue + } + + if err = pr.GetHeadRepo(); err != nil { + continue + } + + if baseBranch, err = pr.BaseRepo.GetBranch(pr.BaseBranch); err != nil { + continue + } + if baseCommit, err = baseBranch.GetCommit(); err != nil { + continue + } + if headBranch, err = pr.HeadRepo.GetBranch(pr.HeadBranch); err != nil { + continue + } + if headCommit, err = headBranch.GetCommit(); err != nil { + continue + } + if baseGitRepo, err = git.OpenRepository(pr.BaseRepo.RepoPath()); err != nil { + continue + } + + mergeBase, err := baseGitRepo.GetMergeBase(baseCommit.ID.String(), headCommit.ID.String()) + if err != nil { + continue + } + + l, err := baseGitRepo.CommitsBetweenIDs(headCommit.ID.String(), mergeBase) + if err != nil { + continue + } + + commits := models.ListToPushCommits(l) + if err := models.UpdateIssuesCommit(issue.Poster, pr.BaseRepo, commits.Commits, false); err != nil { + continue + } + } + + if _, err := x.Exec("UPDATE `comment` SET `created_unix` = ?, `updated_unix` = ? WHERE `id` > ?", issueUpdatedLookup[issue.ID], issueUpdatedLookup[issue.ID], maxCommentID); err != nil { + return err + } + + // try to handle all comments on this issue + for commentStart := 0; ; commentStart += batchSize { + comments := make([]*models.Comment, 0, batchSize) + if err := x.Limit(batchSize, commentStart).Find(&comments); err != nil { + return err + } + if len(comments) == 0 { + break + } + + for _, comment := range comments { + if comment.Type != models.CommentTypeComment && comment.Type != models.CommentTypeCode && comment.Type != models.CommentTypeReview { + continue + } + + if _, err := x.Table("comment").Select("coalesce(max(comment.id), 0)").Get(&maxCommentID); err != nil { + return err + } + + if err := comment.LoadIssue(); err != nil { + continue + } + + if err := models.UpdateIssuesComment(comment.Poster, issue.Repo, issue, comment, false); err != nil { + continue + } + + if _, err := x.Exec("UPDATE `comment` SET `created_unix` = ?, `updated_unix` = ? WHERE `id` > ?", comment.UpdatedUnix, comment.UpdatedUnix, maxCommentID); err != nil { + return err + } + } + } + } + } + + // for every repo, try to process the commits an all branches for issue references + for _, repo := range repoLookup { + if _, err := x.Table("comment").Select("coalesce(max(comment.id), 0)").Get(&maxCommentID); err != nil { + return err + } + + var ( + err error + gitRepo *git.Repository + branches []*models.Branch + defaultBranch *models.Branch + defaultCommit *git.Commit + ) + + if gitRepo, err = git.OpenRepository(repo.RepoPath()); err != nil { + continue + } + + if defaultBranch, err = repo.GetBranch(repo.DefaultBranch); err != nil { + continue + } + if defaultCommit, err = defaultBranch.GetCommit(); err != nil { + continue + } + + if branches, err = repo.GetBranches(); err != nil { + continue + } + + for _, branch := range branches { + var branchCommit *git.Commit + if branchCommit, err = branch.GetCommit(); err != nil { + continue + } + + var l *list.List + if branch.Name == repo.DefaultBranch { + l, err = branchCommit.CommitsBefore() + } else { + l, err = gitRepo.CommitsBetweenIDs(branchCommit.ID.String(), defaultCommit.ID.String()) + } + if err != nil { + continue + } + + commits := models.ListToPushCommits(l) + if err := models.UpdateIssuesCommit(repo.MustOwner(), repo, commits.Commits, false); err != nil { + continue + } + } + + if _, err := x.Exec("UPDATE `comment` SET `created_unix` = ?, `updated_unix` = ? WHERE `id` > ?", repo.UpdatedUnix, repo.UpdatedUnix, maxCommentID); err != nil { + return err + } + } + + return sess.Commit() +}