From 45c8197cf78248429bcaa5bdbf318c6c5060d7bc Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 1 Aug 2023 15:05:14 +0800 Subject: [PATCH 01/12] Add transaction when creating pull request created dirty data --- models/issues/pull.go | 5 ++--- services/pull/pull.go | 43 +++++++++++++++++++++++++++---------------- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index cedbb62c3cb59..ab89dfd4896d1 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -533,13 +533,12 @@ func (pr *PullRequest) SetMerged(ctx context.Context) (bool, error) { } // NewPullRequest creates new pull request with labels for repository. -func NewPullRequest(outerCtx context.Context, repo *repo_model.Repository, issue *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) { - ctx, committer, err := db.TxContext(outerCtx) +func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) { + ctx, committer, err := db.TxContext(ctx) if err != nil { return err } defer committer.Close() - ctx.WithContext(outerCtx) idx, err := db.GetNextResourceIndex(ctx, "issue_index", repo.ID) if err != nil { diff --git a/services/pull/pull.go b/services/pull/pull.go index cf49d2fe206ca..1a1f4e97c5a75 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -50,6 +50,12 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu pr.CommitsAhead = divergence.Ahead pr.CommitsBehind = divergence.Behind + ctx, committer, err := db.TxContext(ctx) + if err != nil { + return err + } + defer committer.Close() + if err := issues_model.NewPullRequest(ctx, repo, pull, labelIDs, uuids, pr); err != nil { return err } @@ -67,30 +73,16 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu // in the db and there is no way to cancel that transaction we have to proceed - therefore // create new context and work from there prCtx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("NewPullRequest: %s:%d", repo.FullName(), pr.Index)) - defer finished() - if pr.Flow == issues_model.PullRequestFlowGithub { err = PushToBaseRepo(prCtx, pr) } else { err = UpdateRef(prCtx, pr) } + finished() if err != nil { return err } - mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, pull, pull.Poster, pull.Content) - if err != nil { - return err - } - - notification.NotifyNewPullRequest(prCtx, pr, mentions) - if len(pull.Labels) > 0 { - notification.NotifyIssueChangeLabels(prCtx, pull.Poster, pull, pull.Labels, nil) - } - if pull.Milestone != nil { - notification.NotifyIssueChangeMilestone(prCtx, pull.Poster, pull, 0) - } - // add first push codes comment baseGitRepo, err := git.OpenRepository(prCtx, pr.BaseRepo.RepoPath()) if err != nil { @@ -125,14 +117,33 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu Content: string(dataJSON), } - _, _ = issue_service.CreateComment(ctx, ops) + if _, err = issue_service.CreateComment(ctx, ops); err != nil { + return err + } if !pr.IsWorkInProgress() { if err := issues_model.PullRequestCodeOwnersReview(ctx, pull, pr); err != nil { return err } } + } + if err := committer.Commit(); err != nil { + // TODO: cleanup + return err + } + + mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, pull, pull.Poster, pull.Content) + if err != nil { + return err + } + + notification.NotifyNewPullRequest(prCtx, pr, mentions) + if len(pull.Labels) > 0 { + notification.NotifyIssueChangeLabels(prCtx, pull.Poster, pull, pull.Labels, nil) + } + if pull.Milestone != nil { + notification.NotifyIssueChangeMilestone(prCtx, pull.Poster, pull, 0) } return nil From 3ac78386b7aedb6b1aa39347a3abfbbe11f0250f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 2 Aug 2023 09:00:00 +0800 Subject: [PATCH 02/12] Fix bug --- services/pull/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 1a1f4e97c5a75..09f1d94007b10 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -73,12 +73,12 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu // in the db and there is no way to cancel that transaction we have to proceed - therefore // create new context and work from there prCtx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("NewPullRequest: %s:%d", repo.FullName(), pr.Index)) + defer finished() if pr.Flow == issues_model.PullRequestFlowGithub { err = PushToBaseRepo(prCtx, pr) } else { err = UpdateRef(prCtx, pr) } - finished() if err != nil { return err } From b45479e757a2f4960f767a535fceffef8067c35d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 2 Aug 2023 17:31:41 +0800 Subject: [PATCH 03/12] Fix bug --- services/pull/pull.go | 112 ++++++++++++++++++++---------------------- 1 file changed, 54 insertions(+), 58 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 09f1d94007b10..eb9ce7fe2fd3b 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -50,85 +50,81 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu pr.CommitsAhead = divergence.Ahead pr.CommitsBehind = divergence.Behind - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - - if err := issues_model.NewPullRequest(ctx, repo, pull, labelIDs, uuids, pr); err != nil { - return err - } - - for _, assigneeID := range assigneeIDs { - if err := issue_service.AddAssigneeIfNotAssigned(ctx, pull, pull.Poster, assigneeID); err != nil { - return err - } - } - - pr.Issue = pull - pull.PullRequest = pr - // Now - even if the request context has been cancelled as the PR has been created // in the db and there is no way to cancel that transaction we have to proceed - therefore // create new context and work from there prCtx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("NewPullRequest: %s:%d", repo.FullName(), pr.Index)) defer finished() - if pr.Flow == issues_model.PullRequestFlowGithub { - err = PushToBaseRepo(prCtx, pr) - } else { - err = UpdateRef(prCtx, pr) - } - if err != nil { - return err - } - - // add first push codes comment - baseGitRepo, err := git.OpenRepository(prCtx, pr.BaseRepo.RepoPath()) - if err != nil { - return err - } - defer baseGitRepo.Close() - compareInfo, err := baseGitRepo.GetCompareInfo(pr.BaseRepo.RepoPath(), - git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName(), false, false) - if err != nil { - return err - } + if err := db.WithTx(ctx, func(ctx context.Context) error { + if err := issues_model.NewPullRequest(ctx, repo, pull, labelIDs, uuids, pr); err != nil { + return err + } - if len(compareInfo.Commits) > 0 { - data := issues_model.PushActionContent{IsForcePush: false} - data.CommitIDs = make([]string, 0, len(compareInfo.Commits)) - for i := len(compareInfo.Commits) - 1; i >= 0; i-- { - data.CommitIDs = append(data.CommitIDs, compareInfo.Commits[i].ID.String()) + for _, assigneeID := range assigneeIDs { + if err := issue_service.AddAssigneeIfNotAssigned(ctx, pull, pull.Poster, assigneeID); err != nil { + return err + } } - dataJSON, err := json.Marshal(data) + pr.Issue = pull + pull.PullRequest = pr + + if pr.Flow == issues_model.PullRequestFlowGithub { + err = PushToBaseRepo(prCtx, pr) + } else { + err = UpdateRef(prCtx, pr) + } if err != nil { return err } - ops := &issues_model.CreateCommentOptions{ - Type: issues_model.CommentTypePullRequestPush, - Doer: pull.Poster, - Repo: repo, - Issue: pr.Issue, - IsForcePush: false, - Content: string(dataJSON), + // add first push codes comment + baseGitRepo, err := git.OpenRepository(prCtx, pr.BaseRepo.RepoPath()) + if err != nil { + return err } + defer baseGitRepo.Close() - if _, err = issue_service.CreateComment(ctx, ops); err != nil { + compareInfo, err := baseGitRepo.GetCompareInfo(pr.BaseRepo.RepoPath(), + git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName(), false, false) + if err != nil { return err } - if !pr.IsWorkInProgress() { - if err := issues_model.PullRequestCodeOwnersReview(ctx, pull, pr); err != nil { + if len(compareInfo.Commits) > 0 { + data := issues_model.PushActionContent{IsForcePush: false} + data.CommitIDs = make([]string, 0, len(compareInfo.Commits)) + for i := len(compareInfo.Commits) - 1; i >= 0; i-- { + data.CommitIDs = append(data.CommitIDs, compareInfo.Commits[i].ID.String()) + } + + dataJSON, err := json.Marshal(data) + if err != nil { + return err + } + + ops := &issues_model.CreateCommentOptions{ + Type: issues_model.CommentTypePullRequestPush, + Doer: pull.Poster, + Repo: repo, + Issue: pr.Issue, + IsForcePush: false, + Content: string(dataJSON), + } + + if _, err = issue_service.CreateComment(ctx, ops); err != nil { return err } - } - } - if err := committer.Commit(); err != nil { + if !pr.IsWorkInProgress() { + if err := issues_model.PullRequestCodeOwnersReview(ctx, pull, pr); err != nil { + return err + } + } + } + return nil + }); err != nil { // TODO: cleanup return err } From 7e933b38029ac2a44ec97b225ba867aeff2aebc2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 3 Aug 2023 11:03:14 +0800 Subject: [PATCH 04/12] Fix bug --- services/pull/pull.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index eb9ce7fe2fd3b..b15daa7190cdd 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -71,9 +71,9 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu pull.PullRequest = pr if pr.Flow == issues_model.PullRequestFlowGithub { - err = PushToBaseRepo(prCtx, pr) + err = PushToBaseRepo(ctx, pr) } else { - err = UpdateRef(prCtx, pr) + err = UpdateRef(ctx, pr) } if err != nil { return err From 3b34db73a3dd99552addce974c99029079ac594a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 6 Aug 2023 21:23:47 +0800 Subject: [PATCH 05/12] Fix test bug --- models/issues/pull.go | 4 ++-- models/issues/review.go | 16 ++++++++-------- routers/web/repo/issue.go | 2 +- services/issue/assignee.go | 14 +++++++------- services/issue/issue.go | 2 +- services/pull/pull.go | 6 +++--- 6 files changed, 22 insertions(+), 22 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index ab89dfd4896d1..676224a3d6982 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -947,14 +947,14 @@ func PullRequestCodeOwnersReview(ctx context.Context, pull *Issue, pr *PullReque for _, u := range uniqUsers { if u.ID != pull.Poster.ID { - if _, err := AddReviewRequest(pull, u, pull.Poster); err != nil { + if _, err := AddReviewRequest(ctx, pull, u, pull.Poster); err != nil { log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err) return err } } } for _, t := range uniqTeams { - if _, err := AddTeamReviewRequest(pull, t, pull.Poster); err != nil { + if _, err := AddTeamReviewRequest(ctx, pull, t, pull.Poster); err != nil { log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err) return err } diff --git a/models/issues/review.go b/models/issues/review.go index cae3ef1d395e5..931f1a2ba72e4 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -560,8 +560,8 @@ func InsertReviews(reviews []*Review) error { } // AddReviewRequest add a review request from one reviewer -func AddReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment, error) { - ctx, committer, err := db.TxContext(db.DefaultContext) +func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User) (*Comment, error) { + ctx, committer, err := db.TxContext(ctx) if err != nil { return nil, err } @@ -615,8 +615,8 @@ func AddReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment, } // RemoveReviewRequest remove a review request from one reviewer -func RemoveReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment, error) { - ctx, committer, err := db.TxContext(db.DefaultContext) +func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User) (*Comment, error) { + ctx, committer, err := db.TxContext(ctx) if err != nil { return nil, err } @@ -676,8 +676,8 @@ func restoreLatestOfficialReview(ctx context.Context, issueID, reviewerID int64) } // AddTeamReviewRequest add a review request from one team -func AddTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) { - ctx, committer, err := db.TxContext(db.DefaultContext) +func AddTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) { + ctx, committer, err := db.TxContext(ctx) if err != nil { return nil, err } @@ -735,8 +735,8 @@ func AddTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_ } // RemoveTeamReviewRequest remove a review request from one team -func RemoveTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) { - ctx, committer, err := db.TxContext(db.DefaultContext) +func RemoveTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) { + ctx, committer, err := db.TxContext(ctx) if err != nil { return nil, err } diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 7bddabd10a680..488c97b0eb6ea 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -2290,7 +2290,7 @@ func UpdateIssueAssignee(ctx *context.Context) { return } - _, _, err = issue_service.ToggleAssignee(ctx, issue, ctx.Doer, assigneeID) + _, _, err = issue_service.ToggleAssigneeWithNotify(ctx, issue, ctx.Doer, assigneeID) if err != nil { ctx.ServerError("ToggleAssignee", err) return diff --git a/services/issue/assignee.go b/services/issue/assignee.go index 8fe35b560203e..9b0445d29f493 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -33,7 +33,7 @@ func DeleteNotPassedAssignee(ctx context.Context, issue *issues_model.Issue, doe if !found { // This function also does comments and hooks, which is why we call it separately instead of directly removing the assignees here - if _, _, err := ToggleAssignee(ctx, issue, doer, assignee.ID); err != nil { + if _, _, err := ToggleAssigneeWithNotify(ctx, issue, doer, assignee.ID); err != nil { return err } } @@ -42,8 +42,8 @@ func DeleteNotPassedAssignee(ctx context.Context, issue *issues_model.Issue, doe return nil } -// ToggleAssignee changes a user between assigned and not assigned for this issue, and make issue comment for it. -func ToggleAssignee(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (removed bool, comment *issues_model.Comment, err error) { +// ToggleAssigneeWithNoNotify changes a user between assigned and not assigned for this issue, and make issue comment for it. +func ToggleAssigneeWithNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (removed bool, comment *issues_model.Comment, err error) { removed, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID) if err != nil { return false, nil, err @@ -62,9 +62,9 @@ func ToggleAssignee(ctx context.Context, issue *issues_model.Issue, doer *user_m // ReviewRequest add or remove a review request from a user for this PR, and make comment for it. func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) { if isAdd { - comment, err = issues_model.AddReviewRequest(issue, reviewer, doer) + comment, err = issues_model.AddReviewRequest(ctx, issue, reviewer, doer) } else { - comment, err = issues_model.RemoveReviewRequest(issue, reviewer, doer) + comment, err = issues_model.RemoveReviewRequest(ctx, issue, reviewer, doer) } if err != nil { @@ -229,9 +229,9 @@ func IsValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team, // TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it. func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *organization.Team, isAdd bool) (comment *issues_model.Comment, err error) { if isAdd { - comment, err = issues_model.AddTeamReviewRequest(issue, reviewer, doer) + comment, err = issues_model.AddTeamReviewRequest(ctx, issue, reviewer, doer) } else { - comment, err = issues_model.RemoveTeamReviewRequest(issue, reviewer, doer) + comment, err = issues_model.RemoveTeamReviewRequest(ctx, issue, reviewer, doer) } if err != nil { diff --git a/services/issue/issue.go b/services/issue/issue.go index b6c6a26cbdc93..03e3bb55ee188 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -197,7 +197,7 @@ func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, do return repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name} } - _, _, err = ToggleAssignee(ctx, issue, doer, assigneeID) + _, _, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID) if err != nil { return err } diff --git a/services/pull/pull.go b/services/pull/pull.go index 73734b2fd9cf5..13cef7e7e8da6 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -134,12 +134,12 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu return err } - notification.NotifyNewPullRequest(prCtx, pr, mentions) + notification.NotifyNewPullRequest(ctx, pr, mentions) if len(pull.Labels) > 0 { - notification.NotifyIssueChangeLabels(prCtx, pull.Poster, pull, pull.Labels, nil) + notification.NotifyIssueChangeLabels(ctx, pull.Poster, pull, pull.Labels, nil) } if pull.Milestone != nil { - notification.NotifyIssueChangeMilestone(prCtx, pull.Poster, pull, 0) + notification.NotifyIssueChangeMilestone(ctx, pull.Poster, pull, 0) } return nil From e418ec5318a1dc8cd3def518a38ec3305cf8b169 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 6 Aug 2023 21:33:45 +0800 Subject: [PATCH 06/12] Fix test --- services/issue/issue.go | 16 ++++++++-------- services/pull/pull.go | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/services/issue/issue.go b/services/issue/issue.go index 03e3bb55ee188..53a495076ce14 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -27,7 +27,7 @@ func NewIssue(ctx context.Context, repo *repo_model.Repository, issue *issues_mo } for _, assigneeID := range assigneeIDs { - if err := AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID); err != nil { + if err := AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID, false); err != nil { return err } } @@ -128,7 +128,7 @@ func UpdateAssignees(ctx context.Context, issue *issues_model.Issue, oneAssignee // has access to the repo. for _, assignee := range allNewAssignees { // Extra method to prevent double adding (which would result in removing) - err = AddAssigneeIfNotAssigned(ctx, issue, doer, assignee.ID) + err = AddAssigneeIfNotAssigned(ctx, issue, doer, assignee.ID, true) if err != nil { return err } @@ -173,7 +173,7 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi // AddAssigneeIfNotAssigned adds an assignee only if he isn't already assigned to the issue. // Also checks for access of assigned user -func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (err error) { +func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64, notify bool) (err error) { assignee, err := user_model.GetUserByID(ctx, assigneeID) if err != nil { return err @@ -197,12 +197,12 @@ func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, do return repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name} } - _, _, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID) - if err != nil { - return err + if notify { + _, _, err = ToggleAssigneeWithNotify(ctx, issue, doer, assigneeID) + } else { + _, _, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID) } - - return nil + return err } // GetRefEndNamesAndURLs retrieves the ref end names (e.g. refs/heads/branch-name -> branch-name) diff --git a/services/pull/pull.go b/services/pull/pull.go index 13cef7e7e8da6..385e00c06adfb 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -62,7 +62,7 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu } for _, assigneeID := range assigneeIDs { - if err := issue_service.AddAssigneeIfNotAssigned(ctx, pull, pull.Poster, assigneeID); err != nil { + if err := issue_service.AddAssigneeIfNotAssigned(ctx, pull, pull.Poster, assigneeID, false); err != nil { return err } } From f21cd317a10ce99295420af90099d409d3fb3852 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 6 Aug 2023 22:32:14 +0800 Subject: [PATCH 07/12] Fix test --- services/issue/issue.go | 24 ++++++++++++------------ services/pull/pull.go | 15 ++++++++++++++- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/services/issue/issue.go b/services/issue/issue.go index 53a495076ce14..2e61787137dbb 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -27,7 +27,7 @@ func NewIssue(ctx context.Context, repo *repo_model.Repository, issue *issues_mo } for _, assigneeID := range assigneeIDs { - if err := AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID, false); err != nil { + if _, err := AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID, true); err != nil { return err } } @@ -128,7 +128,7 @@ func UpdateAssignees(ctx context.Context, issue *issues_model.Issue, oneAssignee // has access to the repo. for _, assignee := range allNewAssignees { // Extra method to prevent double adding (which would result in removing) - err = AddAssigneeIfNotAssigned(ctx, issue, doer, assignee.ID, true) + _, err = AddAssigneeIfNotAssigned(ctx, issue, doer, assignee.ID, true) if err != nil { return err } @@ -173,36 +173,36 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi // AddAssigneeIfNotAssigned adds an assignee only if he isn't already assigned to the issue. // Also checks for access of assigned user -func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64, notify bool) (err error) { +func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64, notify bool) (comment *issues_model.Comment, err error) { assignee, err := user_model.GetUserByID(ctx, assigneeID) if err != nil { - return err + return nil, err } // Check if the user is already assigned isAssigned, err := issues_model.IsUserAssignedToIssue(ctx, issue, assignee) if err != nil { - return err + return nil, err } if isAssigned { // nothing to to - return nil + return nil, nil } valid, err := access_model.CanBeAssigned(ctx, assignee, issue.Repo, issue.IsPull) if err != nil { - return err + return nil, err } if !valid { - return repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name} + return nil, repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name} } if notify { - _, _, err = ToggleAssigneeWithNotify(ctx, issue, doer, assigneeID) - } else { - _, _, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID) + _, comment, err = ToggleAssigneeWithNotify(ctx, issue, doer, assigneeID) + return comment, err } - return err + _, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID) + return comment, err } // GetRefEndNamesAndURLs retrieves the ref end names (e.g. refs/heads/branch-name -> branch-name) diff --git a/services/pull/pull.go b/services/pull/pull.go index 385e00c06adfb..c82f27b427f0d 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -56,15 +56,19 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu prCtx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("NewPullRequest: %s:%d", repo.FullName(), pr.Index)) defer finished() + assigneeCommentMap := make(map[int64]*issues_model.Comment) + if err := db.WithTx(ctx, func(ctx context.Context) error { if err := issues_model.NewPullRequest(ctx, repo, pull, labelIDs, uuids, pr); err != nil { return err } for _, assigneeID := range assigneeIDs { - if err := issue_service.AddAssigneeIfNotAssigned(ctx, pull, pull.Poster, assigneeID, false); err != nil { + comment, err := issue_service.AddAssigneeIfNotAssigned(ctx, pull, pull.Poster, assigneeID, false) + if err != nil { return err } + assigneeCommentMap[assigneeID] = comment } pr.Issue = pull @@ -141,6 +145,15 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu if pull.Milestone != nil { notification.NotifyIssueChangeMilestone(ctx, pull.Poster, pull, 0) } + if len(assigneeIDs) > 0 { + for _, assigneeID := range assigneeIDs { + assignee, err := user_model.GetUserByID(ctx, assigneeID) + if err != nil { + return ErrDependenciesLeft + } + notification.NotifyIssueChangeAssignee(ctx, pull.Poster, pull, assignee, false, assigneeCommentMap[assigneeID]) + } + } return nil } From 19faac29fa4465a435c336e4f1d7c29a101a4d2a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 6 Aug 2023 22:32:50 +0800 Subject: [PATCH 08/12] Fix test --- models/issues/pull_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 0990a3b870676..fa1f551adb936 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -88,14 +88,14 @@ func TestLoadRequestedReviewers(t *testing.T) { user1, err := user_model.GetUserByID(db.DefaultContext, 1) assert.NoError(t, err) - comment, err := issues_model.AddReviewRequest(issue, user1, &user_model.User{}) + comment, err := issues_model.AddReviewRequest(db.DefaultContext, issue, user1, &user_model.User{}) assert.NoError(t, err) assert.NotNil(t, comment) assert.NoError(t, pull.LoadRequestedReviewers(db.DefaultContext)) assert.Len(t, pull.RequestedReviewers, 1) - comment, err = issues_model.RemoveReviewRequest(issue, user1, &user_model.User{}) + comment, err = issues_model.RemoveReviewRequest(db.DefaultContext, issue, user1, &user_model.User{}) assert.NoError(t, err) assert.NotNil(t, comment) From a6548e538c6b227fa8c3964a8e575dcbdf96af82 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 7 Aug 2023 09:38:00 +0800 Subject: [PATCH 09/12] Add cleanup --- services/pull/pull.go | 91 +++++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 43 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index c82f27b427f0d..cf540bd93df22 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -38,7 +38,7 @@ import ( var pullWorkingPool = sync.NewExclusivePool() // NewPullRequest creates new pull request with labels for repository. -func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issues_model.Issue, labelIDs []int64, uuids []string, pr *issues_model.PullRequest, assigneeIDs []int64) error { +func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *issues_model.Issue, labelIDs []int64, uuids []string, pr *issues_model.PullRequest, assigneeIDs []int64) error { if err := TestPatch(pr); err != nil { return err } @@ -58,21 +58,28 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu assigneeCommentMap := make(map[int64]*issues_model.Comment) + // add first push codes comment + baseGitRepo, err := git.OpenRepository(prCtx, pr.BaseRepo.RepoPath()) + if err != nil { + return err + } + defer baseGitRepo.Close() + if err := db.WithTx(ctx, func(ctx context.Context) error { - if err := issues_model.NewPullRequest(ctx, repo, pull, labelIDs, uuids, pr); err != nil { + if err := issues_model.NewPullRequest(ctx, repo, issue, labelIDs, uuids, pr); err != nil { return err } for _, assigneeID := range assigneeIDs { - comment, err := issue_service.AddAssigneeIfNotAssigned(ctx, pull, pull.Poster, assigneeID, false) + comment, err := issue_service.AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID, false) if err != nil { return err } assigneeCommentMap[assigneeID] = comment } - pr.Issue = pull - pull.PullRequest = pr + pr.Issue = issue + issue.PullRequest = pr if pr.Flow == issues_model.PullRequestFlowGithub { err = PushToBaseRepo(ctx, pr) @@ -83,67 +90,65 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu return err } - // add first push codes comment - baseGitRepo, err := git.OpenRepository(prCtx, pr.BaseRepo.RepoPath()) + compareInfo, err := baseGitRepo.GetCompareInfo(pr.BaseRepo.RepoPath(), + git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName(), false, false) if err != nil { return err } - defer baseGitRepo.Close() + if len(compareInfo.Commits) == 0 { + return nil + } - compareInfo, err := baseGitRepo.GetCompareInfo(pr.BaseRepo.RepoPath(), - git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName(), false, false) + data := issues_model.PushActionContent{IsForcePush: false} + data.CommitIDs = make([]string, 0, len(compareInfo.Commits)) + for i := len(compareInfo.Commits) - 1; i >= 0; i-- { + data.CommitIDs = append(data.CommitIDs, compareInfo.Commits[i].ID.String()) + } + + dataJSON, err := json.Marshal(data) if err != nil { return err } - if len(compareInfo.Commits) > 0 { - data := issues_model.PushActionContent{IsForcePush: false} - data.CommitIDs = make([]string, 0, len(compareInfo.Commits)) - for i := len(compareInfo.Commits) - 1; i >= 0; i-- { - data.CommitIDs = append(data.CommitIDs, compareInfo.Commits[i].ID.String()) - } - - dataJSON, err := json.Marshal(data) - if err != nil { - return err - } + ops := &issues_model.CreateCommentOptions{ + Type: issues_model.CommentTypePullRequestPush, + Doer: issue.Poster, + Repo: repo, + Issue: pr.Issue, + IsForcePush: false, + Content: string(dataJSON), + } - ops := &issues_model.CreateCommentOptions{ - Type: issues_model.CommentTypePullRequestPush, - Doer: pull.Poster, - Repo: repo, - Issue: pr.Issue, - IsForcePush: false, - Content: string(dataJSON), - } + if _, err = issues_model.CreateComment(ctx, ops); err != nil { + return err + } - if _, err = issues_model.CreateComment(ctx, ops); err != nil { + if !pr.IsWorkInProgress() { + if err := issues_model.PullRequestCodeOwnersReview(ctx, issue, pr); err != nil { return err } - - if !pr.IsWorkInProgress() { - if err := issues_model.PullRequestCodeOwnersReview(ctx, pull, pr); err != nil { - return err - } - } } return nil }); err != nil { - // TODO: cleanup + // cleanup: this will only remove the reference, the real commit will be clean up when next GC + if err1 := baseGitRepo.RemoveReference(pr.GetGitRefName()); err1 != nil { + log.Error("RemoveReference: %v", err1) + } return err } + baseGitRepo.Close() - mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, pull, pull.Poster, pull.Content) + mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, issue.Poster, issue.Content) if err != nil { return err } notification.NotifyNewPullRequest(ctx, pr, mentions) - if len(pull.Labels) > 0 { - notification.NotifyIssueChangeLabels(ctx, pull.Poster, pull, pull.Labels, nil) + if len(issue.Labels) > 0 { + notification.NotifyIssueChangeLabels(ctx, issue.Poster, issue, issue.Labels, nil) } - if pull.Milestone != nil { - notification.NotifyIssueChangeMilestone(ctx, pull.Poster, pull, 0) + if issue.Milestone != nil { + notification.NotifyIssueChangeMilestone(ctx, issue.Poster, issue, 0) } if len(assigneeIDs) > 0 { for _, assigneeID := range assigneeIDs { @@ -151,7 +156,7 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu if err != nil { return ErrDependenciesLeft } - notification.NotifyIssueChangeAssignee(ctx, pull.Poster, pull, assignee, false, assigneeCommentMap[assigneeID]) + notification.NotifyIssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assigneeID]) } } From 42eb7cf22390f97d44b0af8cb8aed4a531a30ea3 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 7 Aug 2023 13:46:34 +0800 Subject: [PATCH 10/12] improve process tracking --- services/pull/pull.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index cf540bd93df22..147f5071d68e5 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -39,6 +39,10 @@ var pullWorkingPool = sync.NewExclusivePool() // NewPullRequest creates new pull request with labels for repository. func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *issues_model.Issue, labelIDs []int64, uuids []string, pr *issues_model.PullRequest, assigneeIDs []int64) error { + // add the creation of pull request to process management + ctx, _, finished := process.GetManager().AddContext(ctx, fmt.Sprintf("NewPullRequest: %s:%d", repo.FullName(), pr.Index)) + defer finished() + if err := TestPatch(pr); err != nil { return err } @@ -50,16 +54,10 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss pr.CommitsAhead = divergence.Ahead pr.CommitsBehind = divergence.Behind - // Now - even if the request context has been cancelled as the PR has been created - // in the db and there is no way to cancel that transaction we have to proceed - therefore - // create new context and work from there - prCtx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("NewPullRequest: %s:%d", repo.FullName(), pr.Index)) - defer finished() - assigneeCommentMap := make(map[int64]*issues_model.Comment) // add first push codes comment - baseGitRepo, err := git.OpenRepository(prCtx, pr.BaseRepo.RepoPath()) + baseGitRepo, err := git.OpenRepository(ctx, pr.BaseRepo.RepoPath()) if err != nil { return err } From ed249a0bb7e6baa85cf196af3cd9ca9ed6f07fac Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 7 Aug 2023 14:04:52 +0800 Subject: [PATCH 11/12] small optimization --- services/pull/patch.go | 9 +++++++-- services/pull/pull.go | 13 +++++++++++-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/services/pull/patch.go b/services/pull/patch.go index 9277355720dc5..688cbcc027db2 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -62,14 +62,19 @@ func TestPatch(pr *issues_model.PullRequest) error { ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("TestPatch: %s", pr)) defer finished() - // Clone base repo. prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) if err != nil { - log.Error("createTemporaryRepoForPR %-v: %v", pr, err) + if !git_model.IsErrBranchNotExist(err) { + log.Error("CreateTemporaryRepoForPR %-v: %v", pr, err) + } return err } defer cancel() + return testPatch(ctx, prCtx, pr) +} + +func testPatch(ctx context.Context, prCtx *prContext, pr *issues_model.PullRequest) error { gitRepo, err := git.OpenRepository(ctx, prCtx.tmpBasePath) if err != nil { return fmt.Errorf("OpenRepository: %w", err) diff --git a/services/pull/pull.go b/services/pull/pull.go index 147f5071d68e5..4f48519036dbf 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -43,11 +43,20 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss ctx, _, finished := process.GetManager().AddContext(ctx, fmt.Sprintf("NewPullRequest: %s:%d", repo.FullName(), pr.Index)) defer finished() - if err := TestPatch(pr); err != nil { + prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) + if err != nil { + if !git_model.IsErrBranchNotExist(err) { + log.Error("CreateTemporaryRepoForPR %-v: %v", pr, err) + } return err } + defer cancel() - divergence, err := GetDiverging(ctx, pr) + if err := testPatch(ctx, prCtx, pr); err != nil { + return err + } + + divergence, err := git.GetDivergingCommits(ctx, prCtx.tmpBasePath, baseBranch, trackingBranch) if err != nil { return err } From f1f2ab9c335cc6b48469b712dd983ce0a1c64ac6 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 7 Aug 2023 14:21:46 +0800 Subject: [PATCH 12/12] Remove unnecessary process tracking --- services/pull/pull.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 4f48519036dbf..0b6194b1436d8 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -26,7 +26,6 @@ import ( "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification" - "code.gitea.io/gitea/modules/process" repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/sync" @@ -39,10 +38,6 @@ var pullWorkingPool = sync.NewExclusivePool() // NewPullRequest creates new pull request with labels for repository. func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *issues_model.Issue, labelIDs []int64, uuids []string, pr *issues_model.PullRequest, assigneeIDs []int64) error { - // add the creation of pull request to process management - ctx, _, finished := process.GetManager().AddContext(ctx, fmt.Sprintf("NewPullRequest: %s:%d", repo.FullName(), pr.Index)) - defer finished() - prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) if err != nil { if !git_model.IsErrBranchNotExist(err) { @@ -143,7 +138,7 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss } return err } - baseGitRepo.Close() + baseGitRepo.Close() // close immediately to avoid notifications will open the repository again mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, issue.Poster, issue.Content) if err != nil {