diff --git a/integrations/pull_merge_test.go b/integrations/pull_merge_test.go index f8e8c79a88..476f7ab52f 100644 --- a/integrations/pull_merge_test.go +++ b/integrations/pull_merge_test.go @@ -243,11 +243,11 @@ func TestCantMergeConflict(t *testing.T) { gitRepo, err := git.OpenRepository(git.DefaultContext, repo_model.RepoPath(user1.Name, repo1.Name)) assert.NoError(t, err) - err = pull.Merge(git.DefaultContext, pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "CONFLICT") + err = pull.Merge(pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "CONFLICT") assert.Error(t, err, "Merge should return an error due to conflict") assert.True(t, models.IsErrMergeConflicts(err), "Merge error is not a conflict error") - err = pull.Merge(git.DefaultContext, pr, user1, gitRepo, repo_model.MergeStyleRebase, "", "CONFLICT") + err = pull.Merge(pr, user1, gitRepo, repo_model.MergeStyleRebase, "", "CONFLICT") assert.Error(t, err, "Merge should return an error due to conflict") assert.True(t, models.IsErrRebaseConflicts(err), "Merge error is not a conflict error") gitRepo.Close() @@ -342,7 +342,7 @@ func TestCantMergeUnrelated(t *testing.T) { BaseBranch: "base", }).(*models.PullRequest) - err = pull.Merge(git.DefaultContext, pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "UNRELATED") + err = pull.Merge(pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "UNRELATED") assert.Error(t, err, "Merge should return an error due to unrelated") assert.True(t, models.IsErrMergeUnrelatedHistories(err), "Merge error is not a unrelated histories error") gitRepo.Close() diff --git a/models/branches.go b/models/branches.go index f4ded8588f..47fd3dc0a4 100644 --- a/models/branches.go +++ b/models/branches.go @@ -104,7 +104,7 @@ func (protectBranch *ProtectedBranch) CanUserPush(userID int64) bool { } // IsUserMergeWhitelisted checks if some user is whitelisted to merge to this branch -func IsUserMergeWhitelisted(protectBranch *ProtectedBranch, userID int64, permissionInRepo Permission) bool { +func IsUserMergeWhitelisted(ctx context.Context, protectBranch *ProtectedBranch, userID int64, permissionInRepo Permission) bool { if !protectBranch.EnableMergeWhitelist { // Then we need to fall back on whether the user has write permission return permissionInRepo.CanWrite(unit.TypeCode) @@ -118,7 +118,7 @@ func IsUserMergeWhitelisted(protectBranch *ProtectedBranch, userID int64, permis return false } - in, err := organization.IsUserInTeams(db.DefaultContext, userID, protectBranch.MergeWhitelistTeamIDs) + in, err := organization.IsUserInTeams(ctx, userID, protectBranch.MergeWhitelistTeamIDs) if err != nil { log.Error("IsUserInTeams: %v", err) return false @@ -159,16 +159,16 @@ func isUserOfficialReviewer(ctx context.Context, protectBranch *ProtectedBranch, } // HasEnoughApprovals returns true if pr has enough granted approvals. -func (protectBranch *ProtectedBranch) HasEnoughApprovals(pr *PullRequest) bool { +func (protectBranch *ProtectedBranch) HasEnoughApprovals(ctx context.Context, pr *PullRequest) bool { if protectBranch.RequiredApprovals == 0 { return true } - return protectBranch.GetGrantedApprovalsCount(pr) >= protectBranch.RequiredApprovals + return protectBranch.GetGrantedApprovalsCount(ctx, pr) >= protectBranch.RequiredApprovals } // GetGrantedApprovalsCount returns the number of granted approvals for pr. A granted approval must be authored by a user in an approval whitelist. -func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest) int64 { - sess := db.GetEngine(db.DefaultContext).Where("issue_id = ?", pr.IssueID). +func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(ctx context.Context, pr *PullRequest) int64 { + sess := db.GetEngine(ctx).Where("issue_id = ?", pr.IssueID). And("type = ?", ReviewTypeApprove). And("official = ?", true). And("dismissed = ?", false) @@ -185,11 +185,11 @@ func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest) } // MergeBlockedByRejectedReview returns true if merge is blocked by rejected reviews -func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(pr *PullRequest) bool { +func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(ctx context.Context, pr *PullRequest) bool { if !protectBranch.BlockOnRejectedReviews { return false } - rejectExist, err := db.GetEngine(db.DefaultContext).Where("issue_id = ?", pr.IssueID). + rejectExist, err := db.GetEngine(ctx).Where("issue_id = ?", pr.IssueID). And("type = ?", ReviewTypeReject). And("official = ?", true). And("dismissed = ?", false). @@ -204,11 +204,11 @@ func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(pr *PullReque // MergeBlockedByOfficialReviewRequests block merge because of some review request to official reviewer // of from official review -func (protectBranch *ProtectedBranch) MergeBlockedByOfficialReviewRequests(pr *PullRequest) bool { +func (protectBranch *ProtectedBranch) MergeBlockedByOfficialReviewRequests(ctx context.Context, pr *PullRequest) bool { if !protectBranch.BlockOnOfficialReviewRequests { return false } - has, err := db.GetEngine(db.DefaultContext).Where("issue_id = ?", pr.IssueID). + has, err := db.GetEngine(ctx).Where("issue_id = ?", pr.IssueID). And("type = ?", ReviewTypeRequest). And("official = ?", true). Exist(new(Review)) diff --git a/models/db/context.go b/models/db/context.go index 1cd23d453c..c823952cf6 100644 --- a/models/db/context.go +++ b/models/db/context.go @@ -103,7 +103,14 @@ func WithContext(f func(ctx *Context) error) error { } // WithTx represents executing database operations on a transaction -func WithTx(f func(ctx context.Context) error) error { +// you can optionally change the context to a parrent one +func WithTx(f func(ctx context.Context) error, stdCtx ...context.Context) error { + parentCtx := DefaultContext + if len(stdCtx) != 0 && stdCtx[0] != nil { + // TODO: make sure parent context has no open session + parentCtx = stdCtx[0] + } + sess := x.NewSession() defer sess.Close() if err := sess.Begin(); err != nil { @@ -111,7 +118,7 @@ func WithTx(f func(ctx context.Context) error) error { } if err := f(&Context{ - Context: DefaultContext, + Context: parentCtx, e: sess, }); err != nil { return err diff --git a/models/issue.go b/models/issue.go index 2a41cbc28e..98e64adafd 100644 --- a/models/issue.go +++ b/models/issue.go @@ -591,7 +591,7 @@ func ReplaceIssueLabels(issue *Issue, labels []*Label, doer *user_model.User) (e } // ReadBy sets issue to be read by given user. -func (issue *Issue) ReadBy(userID int64) error { +func (issue *Issue) ReadBy(ctx context.Context, userID int64) error { if err := UpdateIssueUserByRead(userID, issue.ID); err != nil { return err } @@ -635,7 +635,7 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use // Check for open dependencies if issue.IsClosed && issue.Repo.IsDependenciesEnabledCtx(ctx) { // only check if dependencies are enabled and we're about to close an issue, otherwise reopening an issue would fail when there are unsatisfied dependencies - noDeps, err := issueNoDependenciesLeft(e, issue) + noDeps, err := IssueNoDependenciesLeft(ctx, issue) if err != nil { return nil, err } @@ -693,13 +693,7 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use } // ChangeIssueStatus changes issue status to open or closed. -func ChangeIssueStatus(issue *Issue, doer *user_model.User, isClosed bool) (*Comment, error) { - ctx, committer, err := db.TxContext() - if err != nil { - return nil, err - } - defer committer.Close() - +func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed bool) (*Comment, error) { if err := issue.LoadRepo(ctx); err != nil { return nil, err } @@ -707,16 +701,7 @@ func ChangeIssueStatus(issue *Issue, doer *user_model.User, isClosed bool) (*Com return nil, err } - comment, err := changeIssueStatus(ctx, issue, doer, isClosed, false) - if err != nil { - return nil, err - } - - if err = committer.Commit(); err != nil { - return nil, fmt.Errorf("Commit: %v", err) - } - - return comment, nil + return changeIssueStatus(ctx, issue, doer, isClosed, false) } // ChangeIssueTitle changes the title of this issue, as the given user. @@ -787,16 +772,11 @@ func ChangeIssueRef(issue *Issue, doer *user_model.User, oldRef string) (err err } // AddDeletePRBranchComment adds delete branch comment for pull request issue -func AddDeletePRBranchComment(doer *user_model.User, repo *repo_model.Repository, issueID int64, branchName string) error { - issue, err := getIssueByID(db.GetEngine(db.DefaultContext), issueID) +func AddDeletePRBranchComment(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issueID int64, branchName string) error { + issue, err := getIssueByID(db.GetEngine(ctx), issueID) if err != nil { return err } - ctx, committer, err := db.TxContext() - if err != nil { - return err - } - defer committer.Close() opts := &CreateCommentOptions{ Type: CommentTypeDeleteBranch, Doer: doer, @@ -804,11 +784,8 @@ func AddDeletePRBranchComment(doer *user_model.User, repo *repo_model.Repository Issue: issue, OldRef: branchName, } - if _, err = CreateCommentCtx(ctx, opts); err != nil { - return err - } - - return committer.Commit() + _, err = CreateCommentCtx(ctx, opts) + return err } // UpdateIssueAttachments update attachments by UUIDs for the issue diff --git a/models/issue_comment.go b/models/issue_comment.go index 39c2818eed..ceea878662 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -284,14 +284,15 @@ type PushActionContent struct { // LoadIssue loads issue from database func (c *Comment) LoadIssue() (err error) { - return c.loadIssue(db.GetEngine(db.DefaultContext)) + return c.LoadIssueCtx(db.DefaultContext) } -func (c *Comment) loadIssue(e db.Engine) (err error) { +// LoadIssueCtx loads issue from database +func (c *Comment) LoadIssueCtx(ctx context.Context) (err error) { if c.Issue != nil { return nil } - c.Issue, err = getIssueByID(e, c.IssueID) + c.Issue, err = getIssueByID(db.GetEngine(ctx), c.IssueID) return } @@ -1126,7 +1127,7 @@ func UpdateComment(c *Comment, doer *user_model.User) error { if _, err := sess.ID(c.ID).AllCols().Update(c); err != nil { return err } - if err := c.loadIssue(sess); err != nil { + if err := c.LoadIssueCtx(ctx); err != nil { return err } if err := c.addCrossReferences(ctx, doer, true); err != nil { diff --git a/models/issue_dependency.go b/models/issue_dependency.go index d2c5785b90..b292db57e0 100644 --- a/models/issue_dependency.go +++ b/models/issue_dependency.go @@ -5,6 +5,8 @@ package models import ( + "context" + "code.gitea.io/gitea/models/db" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/timeutil" @@ -117,12 +119,8 @@ func issueDepExists(e db.Engine, issueID, depID int64) (bool, error) { } // IssueNoDependenciesLeft checks if issue can be closed -func IssueNoDependenciesLeft(issue *Issue) (bool, error) { - return issueNoDependenciesLeft(db.GetEngine(db.DefaultContext), issue) -} - -func issueNoDependenciesLeft(e db.Engine, issue *Issue) (bool, error) { - exists, err := e. +func IssueNoDependenciesLeft(ctx context.Context, issue *Issue) (bool, error) { + exists, err := db.GetEngine(ctx). Table("issue_dependency"). Select("issue.*"). Join("INNER", "issue", "issue.id = issue_dependency.dependency_id"). diff --git a/models/issue_dependency_test.go b/models/issue_dependency_test.go index 1789de5dc0..345a9077cd 100644 --- a/models/issue_dependency_test.go +++ b/models/issue_dependency_test.go @@ -7,6 +7,7 @@ package models import ( "testing" + "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" @@ -43,15 +44,15 @@ func TestCreateIssueDependency(t *testing.T) { _ = unittest.AssertExistsAndLoadBean(t, &Comment{Type: CommentTypeAddDependency, PosterID: user1.ID, IssueID: issue1.ID}) // Check if dependencies left is correct - left, err := IssueNoDependenciesLeft(issue1) + left, err := IssueNoDependenciesLeft(db.DefaultContext, issue1) assert.NoError(t, err) assert.False(t, left) // Close #2 and check again - _, err = ChangeIssueStatus(issue2, user1, true) + _, err = ChangeIssueStatus(db.DefaultContext, issue2, user1, true) assert.NoError(t, err) - left, err = IssueNoDependenciesLeft(issue1) + left, err = IssueNoDependenciesLeft(db.DefaultContext, issue1) assert.NoError(t, err) assert.True(t, left) diff --git a/models/issue_test.go b/models/issue_test.go index 5382c04f43..9b82fc80fb 100644 --- a/models/issue_test.go +++ b/models/issue_test.go @@ -443,12 +443,12 @@ func TestIssue_DeleteIssue(t *testing.T) { assert.NoError(t, err) err = CreateIssueDependency(user, issue1, issue2) assert.NoError(t, err) - left, err := IssueNoDependenciesLeft(issue1) + left, err := IssueNoDependenciesLeft(db.DefaultContext, issue1) assert.NoError(t, err) assert.False(t, left) err = DeleteIssue(&Issue{ID: 2}) assert.NoError(t, err) - left, err = IssueNoDependenciesLeft(issue1) + left, err = IssueNoDependenciesLeft(db.DefaultContext, issue1) assert.NoError(t, err) assert.True(t, left) } diff --git a/models/issue_xref.go b/models/issue_xref.go index 2647f9c656..cd1c122252 100644 --- a/models/issue_xref.go +++ b/models/issue_xref.go @@ -249,7 +249,7 @@ func (comment *Comment) addCrossReferences(stdCtx context.Context, doer *user_mo if comment.Type != CommentTypeCode && comment.Type != CommentTypeComment { return nil } - if err := comment.loadIssue(db.GetEngine(stdCtx)); err != nil { + if err := comment.LoadIssueCtx(stdCtx); err != nil { return err } ctx := &crossReferencesContext{ @@ -340,9 +340,9 @@ func (comment *Comment) RefIssueIdent() string { // \/ \/ |__| \/ \/ // ResolveCrossReferences will return the list of references to close/reopen by this PR -func (pr *PullRequest) ResolveCrossReferences() ([]*Comment, error) { +func (pr *PullRequest) ResolveCrossReferences(ctx context.Context) ([]*Comment, error) { unfiltered := make([]*Comment, 0, 5) - if err := db.GetEngine(db.DefaultContext). + if err := db.GetEngine(ctx). Where("ref_repo_id = ? AND ref_issue_id = ?", pr.Issue.RepoID, pr.Issue.ID). In("ref_action", []references.XRefAction{references.XRefActionCloses, references.XRefActionReopens}). OrderBy("id"). diff --git a/models/issue_xref_test.go b/models/issue_xref_test.go index 1ae5fb360f..677caa48ba 100644 --- a/models/issue_xref_test.go +++ b/models/issue_xref_test.go @@ -98,7 +98,7 @@ func TestXRef_ResolveCrossReferences(t *testing.T) { i1 := testCreateIssue(t, 1, 2, "title1", "content1", false) i2 := testCreateIssue(t, 1, 2, "title2", "content2", false) i3 := testCreateIssue(t, 1, 2, "title3", "content3", false) - _, err := ChangeIssueStatus(i3, d, true) + _, err := ChangeIssueStatus(db.DefaultContext, i3, d, true) assert.NoError(t, err) pr := testCreatePR(t, 1, 2, "titlepr", fmt.Sprintf("closes #%d", i1.Index)) @@ -118,7 +118,7 @@ func TestXRef_ResolveCrossReferences(t *testing.T) { c4 := testCreateComment(t, 1, 2, pr.Issue.ID, fmt.Sprintf("closes #%d", i3.Index)) r4 := unittest.AssertExistsAndLoadBean(t, &Comment{IssueID: i3.ID, RefIssueID: pr.Issue.ID, RefCommentID: c4.ID}).(*Comment) - refs, err := pr.ResolveCrossReferences() + refs, err := pr.ResolveCrossReferences(db.DefaultContext) assert.NoError(t, err) assert.Len(t, refs, 3) assert.Equal(t, rp.ID, refs[0].ID, "bad ref rp: %+v", refs[0]) diff --git a/models/pull.go b/models/pull.go index 437b202d3f..d056888130 100644 --- a/models/pull.go +++ b/models/pull.go @@ -227,23 +227,23 @@ func (pr *PullRequest) LoadProtectedBranchCtx(ctx context.Context) (err error) { } // GetDefaultMergeMessage returns default message used when merging pull request -func (pr *PullRequest) GetDefaultMergeMessage() (string, error) { +func (pr *PullRequest) GetDefaultMergeMessage(ctx context.Context) (string, error) { if pr.HeadRepo == nil { var err error - pr.HeadRepo, err = repo_model.GetRepositoryByID(pr.HeadRepoID) + pr.HeadRepo, err = repo_model.GetRepositoryByIDCtx(ctx, pr.HeadRepoID) if err != nil { return "", fmt.Errorf("GetRepositoryById[%d]: %v", pr.HeadRepoID, err) } } - if err := pr.LoadIssue(); err != nil { + if err := pr.LoadIssueCtx(ctx); err != nil { return "", fmt.Errorf("Cannot load issue %d for PR id %d: Error: %v", pr.IssueID, pr.ID, err) } - if err := pr.LoadBaseRepo(); err != nil { + if err := pr.LoadBaseRepoCtx(ctx); err != nil { return "", fmt.Errorf("LoadBaseRepo: %v", err) } issueReference := "#" - if pr.BaseRepo.UnitEnabled(unit.TypeExternalTracker) { + if pr.BaseRepo.UnitEnabledCtx(ctx, unit.TypeExternalTracker) { issueReference = "!" } @@ -337,14 +337,14 @@ func (pr *PullRequest) getReviewedByLines(writer io.Writer) error { } // GetDefaultSquashMessage returns default message used when squash and merging pull request -func (pr *PullRequest) GetDefaultSquashMessage() (string, error) { - if err := pr.LoadIssue(); err != nil { +func (pr *PullRequest) GetDefaultSquashMessage(ctx context.Context) (string, error) { + if err := pr.LoadIssueCtx(ctx); err != nil { return "", fmt.Errorf("LoadIssue: %v", err) } - if err := pr.LoadBaseRepo(); err != nil { + if err := pr.LoadBaseRepoCtx(ctx); err != nil { return "", fmt.Errorf("LoadBaseRepo: %v", err) } - if pr.BaseRepo.UnitEnabled(unit.TypeExternalTracker) { + if pr.BaseRepo.UnitEnabledCtx(ctx, unit.TypeExternalTracker) { return fmt.Sprintf("%s (!%d)", pr.Issue.Title, pr.Issue.Index), nil } return fmt.Sprintf("%s (#%d)", pr.Issue.Title, pr.Issue.Index), nil @@ -371,7 +371,7 @@ func (pr *PullRequest) IsEmpty() bool { } // SetMerged sets a pull request to merged and closes the corresponding issue -func (pr *PullRequest) SetMerged() (bool, error) { +func (pr *PullRequest) SetMerged(ctx context.Context) (bool, error) { if pr.HasMerged { return false, fmt.Errorf("PullRequest[%d] already merged", pr.Index) } @@ -380,12 +380,6 @@ func (pr *PullRequest) SetMerged() (bool, error) { } pr.HasMerged = true - - ctx, committer, err := db.TxContext() - if err != nil { - return false, err - } - defer committer.Close() sess := db.GetEngine(ctx) if _, err := sess.Exec("UPDATE `issue` SET `repo_id` = `repo_id` WHERE `id` = ?", pr.IssueID); err != nil { @@ -432,9 +426,6 @@ func (pr *PullRequest) SetMerged() (bool, error) { return false, fmt.Errorf("Failed to update pr[%d]: %v", pr.ID, err) } - if err := committer.Commit(); err != nil { - return false, fmt.Errorf("Commit: %v", err) - } return true, nil } diff --git a/models/pull_list.go b/models/pull_list.go index 055119a1e5..ca09e28a93 100644 --- a/models/pull_list.go +++ b/models/pull_list.go @@ -62,8 +62,8 @@ func GetUnmergedPullRequestsByHeadInfo(repoID int64, branch string) ([]*PullRequ // HasUnmergedPullRequestsByHeadInfo checks if there are open and not merged pull request // by given head information (repo and branch) -func HasUnmergedPullRequestsByHeadInfo(repoID int64, branch string) (bool, error) { - return db.GetEngine(db.DefaultContext). +func HasUnmergedPullRequestsByHeadInfo(ctx context.Context, repoID int64, branch string) (bool, error) { + return db.GetEngine(ctx). Where("head_repo_id = ? AND head_branch = ? AND has_merged = ? AND issue.is_closed = ? AND flow = ?", repoID, branch, false, false, PullRequestFlowGithub). Join("INNER", "issue", "issue.id = pull_request.issue_id"). diff --git a/models/pull_test.go b/models/pull_test.go index 6194ac0e24..92cf9a6524 100644 --- a/models/pull_test.go +++ b/models/pull_test.go @@ -110,11 +110,11 @@ func TestGetUnmergedPullRequest(t *testing.T) { func TestHasUnmergedPullRequestsByHeadInfo(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) - exist, err := HasUnmergedPullRequestsByHeadInfo(1, "branch2") + exist, err := HasUnmergedPullRequestsByHeadInfo(db.DefaultContext, 1, "branch2") assert.NoError(t, err) assert.Equal(t, true, exist) - exist, err = HasUnmergedPullRequestsByHeadInfo(1, "not_exist_branch") + exist, err = HasUnmergedPullRequestsByHeadInfo(db.DefaultContext, 1, "not_exist_branch") assert.NoError(t, err) assert.Equal(t, false, exist) } @@ -261,13 +261,13 @@ func TestPullRequest_GetDefaultMergeMessage_InternalTracker(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) pr := unittest.AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest) - msg, err := pr.GetDefaultMergeMessage() + msg, err := pr.GetDefaultMergeMessage(db.DefaultContext) assert.NoError(t, err) assert.Equal(t, "Merge pull request 'issue3' (#3) from branch2 into master", msg) pr.BaseRepoID = 1 pr.HeadRepoID = 2 - msg, err = pr.GetDefaultMergeMessage() + msg, err = pr.GetDefaultMergeMessage(db.DefaultContext) assert.NoError(t, err) assert.Equal(t, "Merge pull request 'issue3' (#3) from user2/repo1:branch2 into master", msg) } @@ -287,13 +287,22 @@ func TestPullRequest_GetDefaultMergeMessage_ExternalTracker(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &PullRequest{ID: 2, BaseRepo: baseRepo}).(*PullRequest) - msg, err := pr.GetDefaultMergeMessage() + msg, err := pr.GetDefaultMergeMessage(db.DefaultContext) assert.NoError(t, err) assert.Equal(t, "Merge pull request 'issue3' (!3) from branch2 into master", msg) pr.BaseRepoID = 1 pr.HeadRepoID = 2 - msg, err = pr.GetDefaultMergeMessage() + msg, err = pr.GetDefaultMergeMessage(db.DefaultContext) assert.NoError(t, err) assert.Equal(t, "Merge pull request 'issue3' (!3) from user2/repo1:branch2 into master", msg) } + +func TestPullRequest_GetDefaultSquashMessage(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + pr := unittest.AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest) + + msg, err := pr.GetDefaultSquashMessage(db.DefaultContext) + assert.NoError(t, err) + assert.Equal(t, "issue3 (#3)", msg) +} diff --git a/models/repo.go b/models/repo.go index 9c2fce8d3c..e934b24fb3 100644 --- a/models/repo.go +++ b/models/repo.go @@ -455,8 +455,8 @@ func CreateRepository(ctx context.Context, doer, u *user_model.User, repo *repo_ } } - if isAdmin, err := isUserRepoAdmin(ctx, repo, doer); err != nil { - return fmt.Errorf("isUserRepoAdmin: %v", err) + if isAdmin, err := IsUserRepoAdminCtx(ctx, repo, doer); err != nil { + return fmt.Errorf("IsUserRepoAdminCtx: %v", err) } else if !isAdmin { // Make creator repo admin if it wasn't assigned automatically if err = addCollaborator(ctx, repo, doer); err != nil { @@ -1215,7 +1215,7 @@ func DeleteDeployKey(ctx context.Context, doer *user_model.User, id int64) error if err != nil { return fmt.Errorf("GetRepositoryByID: %v", err) } - has, err := isUserRepoAdmin(ctx, repo, doer) + has, err := IsUserRepoAdminCtx(ctx, repo, doer) if err != nil { return fmt.Errorf("GetUserRepoPermission: %v", err) } else if !has { diff --git a/models/repo/repo.go b/models/repo/repo.go index fc72d36dac..8af6357bf3 100644 --- a/models/repo/repo.go +++ b/models/repo/repo.go @@ -302,8 +302,19 @@ func (repo *Repository) LoadUnits(ctx context.Context) (err error) { } // UnitEnabled if this repository has the given unit enabled -func (repo *Repository) UnitEnabled(tp unit.Type) bool { - if err := repo.LoadUnits(db.DefaultContext); err != nil { +func (repo *Repository) UnitEnabled(tp unit.Type) (result bool) { + if err := db.WithContext(func(ctx *db.Context) error { + result = repo.UnitEnabledCtx(ctx, tp) + return nil + }); err != nil { + log.Error("repo.UnitEnabled: %v", err) + } + return +} + +// UnitEnabled if this repository has the given unit enabled +func (repo *Repository) UnitEnabledCtx(ctx context.Context, tp unit.Type) bool { + if err := repo.LoadUnits(ctx); err != nil { log.Warn("Error loading repository (ID: %d) units: %s", repo.ID, err.Error()) } for _, unit := range repo.Units { diff --git a/models/repo_permission.go b/models/repo_permission.go index bc31b873f2..8ba6b86145 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -331,10 +331,11 @@ func IsUserRealRepoAdmin(repo *repo_model.Repository, user *user_model.User) (bo // IsUserRepoAdmin return true if user has admin right of a repo func IsUserRepoAdmin(repo *repo_model.Repository, user *user_model.User) (bool, error) { - return isUserRepoAdmin(db.DefaultContext, repo, user) + return IsUserRepoAdminCtx(db.DefaultContext, repo, user) } -func isUserRepoAdmin(ctx context.Context, repo *repo_model.Repository, user *user_model.User) (bool, error) { +// IsUserRepoAdminCtx return true if user has admin right of a repo +func IsUserRepoAdminCtx(ctx context.Context, repo *repo_model.Repository, user *user_model.User) (bool, error) { if user == nil || repo == nil { return false, nil } diff --git a/modules/convert/convert.go b/modules/convert/convert.go index bd06f4dbf4..3a12ed8f1f 100644 --- a/modules/convert/convert.go +++ b/modules/convert/convert.go @@ -87,7 +87,7 @@ func ToBranch(repo *repo_model.Repository, b *git.Branch, c *git.Commit, bp *mod return nil, err } branch.UserCanPush = bp.CanUserPush(user.ID) - branch.UserCanMerge = models.IsUserMergeWhitelisted(bp, user.ID, permission) + branch.UserCanMerge = models.IsUserMergeWhitelisted(db.DefaultContext, bp, user.ID, permission) } return branch, nil diff --git a/modules/git/commit_test.go b/modules/git/commit_test.go index aac7de5d46..fb8c22dfd3 100644 --- a/modules/git/commit_test.go +++ b/modules/git/commit_test.go @@ -67,6 +67,7 @@ empty commit` gitRepo, err := openRepositoryWithDefaultContext(filepath.Join(testReposDir, "repo1_bare")) assert.NoError(t, err) assert.NotNil(t, gitRepo) + defer gitRepo.Close() commitFromReader, err := CommitFromReader(gitRepo, sha, strings.NewReader(commitString)) assert.NoError(t, err) @@ -111,6 +112,7 @@ func TestHasPreviousCommit(t *testing.T) { repo, err := openRepositoryWithDefaultContext(bareRepo1Path) assert.NoError(t, err) + defer repo.Close() commit, err := repo.GetCommit("8006ff9adbf0cb94da7dad9e537e53817f9fa5c0") assert.NoError(t, err) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 9517802183..d6f349e332 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -726,7 +726,8 @@ func MergePullRequest(ctx *context.APIContext) { // "$ref": "#/responses/error" form := web.GetForm(ctx).(*forms.MergePullRequestForm) - pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) + + pr, err := models.GetPullRequestByIndexCtx(ctx, ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { if models.IsErrPullRequestNotExist(err) { ctx.NotFound("GetPullRequestByIndex", err) @@ -741,7 +742,7 @@ func MergePullRequest(ctx *context.APIContext) { return } - if err := pr.LoadIssue(); err != nil { + if err := pr.LoadIssueCtx(ctx); err != nil { ctx.Error(http.StatusInternalServerError, "LoadIssue", err) return } @@ -749,7 +750,7 @@ func MergePullRequest(ctx *context.APIContext) { if ctx.IsSigned { // Update issue-user. - if err = pr.Issue.ReadBy(ctx.Doer.ID); err != nil { + if err = pr.Issue.ReadBy(ctx, ctx.Doer.ID); err != nil { ctx.Error(http.StatusInternalServerError, "ReadBy", err) return } @@ -758,6 +759,7 @@ func MergePullRequest(ctx *context.APIContext) { manuallMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged force := form.ForceMerge != nil && *form.ForceMerge + // start with merging by checking if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, manuallMerge, force); err != nil { if errors.Is(err, pull_service.ErrIsClosed) { ctx.NotFound() @@ -798,15 +800,14 @@ func MergePullRequest(ctx *context.APIContext) { } // set defaults to propagate needed fields - if err := form.SetDefaults(pr); err != nil { + if err := form.SetDefaults(ctx, pr); err != nil { ctx.ServerError("SetDefaults", fmt.Errorf("SetDefaults: %v", err)) return } - if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, form.MergeTitleField); err != nil { + if err := pull_service.Merge(pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, form.MergeTitleField); err != nil { if models.IsErrInvalidMergeStyle(err) { ctx.Error(http.StatusMethodNotAllowed, "Invalid merge style", fmt.Errorf("%s is not allowed an allowed merge style for this repository", repo_model.MergeStyle(form.Do))) - return } else if models.IsErrMergeConflicts(err) { conflictError := err.(models.ErrMergeConflicts) ctx.JSON(http.StatusConflict, conflictError) @@ -818,28 +819,25 @@ func MergePullRequest(ctx *context.APIContext) { ctx.JSON(http.StatusConflict, conflictError) } else if git.IsErrPushOutOfDate(err) { ctx.Error(http.StatusConflict, "Merge", "merge push out of date") - return } else if models.IsErrSHADoesNotMatch(err) { ctx.Error(http.StatusConflict, "Merge", "head out of date") - return } else if git.IsErrPushRejected(err) { errPushRej := err.(*git.ErrPushRejected) if len(errPushRej.Message) == 0 { ctx.Error(http.StatusConflict, "Merge", "PushRejected without remote error message") - return + } else { + ctx.Error(http.StatusConflict, "Merge", "PushRejected with remote message: "+errPushRej.Message) } - ctx.Error(http.StatusConflict, "Merge", "PushRejected with remote message: "+errPushRej.Message) - return + } else { + ctx.Error(http.StatusInternalServerError, "Merge", err) } - ctx.Error(http.StatusInternalServerError, "Merge", err) return } - log.Trace("Pull request merged: %d", pr.ID) if form.DeleteBranchAfterMerge { // Don't cleanup when there are other PR's that use this branch as head branch. - exist, err := models.HasUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) + exist, err := models.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) if err != nil { ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) return @@ -873,7 +871,7 @@ func MergePullRequest(ctx *context.APIContext) { } return } - if err := models.AddDeletePRBranchComment(ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil { + if err := models.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil { // Do not fail here as branch has already been deleted log.Error("DeleteBranch: %v", err) } diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 731b36fc1e..d2203a1f99 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -311,7 +311,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID, refFullN // Now check if the user is allowed to merge PRs for this repository // Note: we can use ctx.perm and ctx.user directly as they will have been loaded above - allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, ctx.userPerm, ctx.user) + allowedMerge, err := pull_service.IsUserAllowedToMerge(ctx, pr, ctx.userPerm, ctx.user) if err != nil { log.Error("Error calculating if allowed to merge: %v", err) ctx.JSON(http.StatusInternalServerError, private.Response{ diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index a54ad3306b..cf919e0c32 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1295,7 +1295,7 @@ func ViewIssue(ctx *context.Context) { if ctx.IsSigned { // Update issue-user. - if err = issue.ReadBy(ctx.Doer.ID); err != nil { + if err = issue.ReadBy(ctx, ctx.Doer.ID); err != nil { ctx.ServerError("ReadBy", err) return } @@ -1557,7 +1557,7 @@ func ViewIssue(ctx *context.Context) { ctx.ServerError("GetUserRepoPermission", err) return } - ctx.Data["AllowMerge"], err = pull_service.IsUserAllowedToMerge(pull, perm, ctx.Doer) + ctx.Data["AllowMerge"], err = pull_service.IsUserAllowedToMerge(ctx, pull, perm, ctx.Doer) if err != nil { ctx.ServerError("IsUserAllowedToMerge", err) return @@ -1606,12 +1606,11 @@ func ViewIssue(ctx *context.Context) { if ctx.Doer != nil { showMergeInstructions = pull.ProtectedBranch.CanUserPush(ctx.Doer.ID) } - cnt := pull.ProtectedBranch.GetGrantedApprovalsCount(pull) - ctx.Data["IsBlockedByApprovals"] = !pull.ProtectedBranch.HasEnoughApprovals(pull) - ctx.Data["IsBlockedByRejection"] = pull.ProtectedBranch.MergeBlockedByRejectedReview(pull) - ctx.Data["IsBlockedByOfficialReviewRequests"] = pull.ProtectedBranch.MergeBlockedByOfficialReviewRequests(pull) + ctx.Data["IsBlockedByApprovals"] = !pull.ProtectedBranch.HasEnoughApprovals(ctx, pull) + ctx.Data["IsBlockedByRejection"] = pull.ProtectedBranch.MergeBlockedByRejectedReview(ctx, pull) + ctx.Data["IsBlockedByOfficialReviewRequests"] = pull.ProtectedBranch.MergeBlockedByOfficialReviewRequests(ctx, pull) ctx.Data["IsBlockedByOutdatedBranch"] = pull.ProtectedBranch.MergeBlockedByOutdatedBranch(pull) - ctx.Data["GrantedApprovals"] = cnt + ctx.Data["GrantedApprovals"] = pull.ProtectedBranch.GetGrantedApprovalsCount(ctx, pull) ctx.Data["RequireSigned"] = pull.ProtectedBranch.RequireSignedCommits ctx.Data["ChangedProtectedFiles"] = pull.ChangedProtectedFiles ctx.Data["IsBlockedByChangedProtectedFiles"] = len(pull.ChangedProtectedFiles) != 0 @@ -1641,7 +1640,7 @@ func ViewIssue(ctx *context.Context) { (!pull.HasMerged || ctx.Data["HeadBranchCommitID"] == ctx.Data["PullHeadCommitID"]) if isPullBranchDeletable && pull.HasMerged { - exist, err := models.HasUnmergedPullRequestsByHeadInfo(pull.HeadRepoID, pull.HeadBranch) + exist, err := models.HasUnmergedPullRequestsByHeadInfo(ctx, pull.HeadRepoID, pull.HeadBranch) if err != nil { ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) return diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 99faa54138..6cda560f3c 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -290,7 +290,7 @@ func checkPullInfo(ctx *context.Context) *models.Issue { if ctx.IsSigned { // Update issue-user. - if err = issue.ReadBy(ctx.Doer.ID); err != nil { + if err = issue.ReadBy(ctx, ctx.Doer.ID); err != nil { ctx.ServerError("ReadBy", err) return nil } @@ -866,6 +866,7 @@ func MergePullRequest(ctx *context.Context) { manuallMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged forceMerge := form.ForceMerge != nil && *form.ForceMerge + // start with merging by checking if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, manuallMerge, forceMerge); err != nil { if errors.Is(err, pull_service.ErrIsClosed) { if issue.IsPull { @@ -899,7 +900,6 @@ func MergePullRequest(ctx *context.Context) { } else { ctx.ServerError("WebCheck", err) } - return } @@ -909,14 +909,12 @@ func MergePullRequest(ctx *context.Context) { if models.IsErrInvalidMergeStyle(err) { ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option")) ctx.Redirect(issue.Link()) - return } else if strings.Contains(err.Error(), "Wrong commit ID") { ctx.Flash.Error(ctx.Tr("repo.pulls.wrong_commit_id")) ctx.Redirect(issue.Link()) - return + } else { + ctx.ServerError("MergedManually", err) } - - ctx.ServerError("MergedManually", err) return } @@ -925,16 +923,15 @@ func MergePullRequest(ctx *context.Context) { } // set defaults to propagate needed fields - if err := form.SetDefaults(pr); err != nil { + if err := form.SetDefaults(ctx, pr); err != nil { ctx.ServerError("SetDefaults", fmt.Errorf("SetDefaults: %v", err)) return } - if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, form.MergeTitleField); err != nil { + if err := pull_service.Merge(pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, form.MergeTitleField); err != nil { if models.IsErrInvalidMergeStyle(err) { ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option")) ctx.Redirect(issue.Link()) - return } else if models.IsErrMergeConflicts(err) { conflictError := err.(models.ErrMergeConflicts) flashError, err := ctx.RenderToString(tplAlertDetails, map[string]interface{}{ @@ -948,7 +945,6 @@ func MergePullRequest(ctx *context.Context) { } ctx.Flash.Error(flashError) ctx.Redirect(issue.Link()) - return } else if models.IsErrRebaseConflicts(err) { conflictError := err.(models.ErrRebaseConflicts) flashError, err := ctx.RenderToString(tplAlertDetails, map[string]interface{}{ @@ -962,22 +958,18 @@ func MergePullRequest(ctx *context.Context) { } ctx.Flash.Error(flashError) ctx.Redirect(issue.Link()) - return } else if models.IsErrMergeUnrelatedHistories(err) { log.Debug("MergeUnrelatedHistories error: %v", err) ctx.Flash.Error(ctx.Tr("repo.pulls.unrelated_histories")) ctx.Redirect(issue.Link()) - return } else if git.IsErrPushOutOfDate(err) { log.Debug("MergePushOutOfDate error: %v", err) ctx.Flash.Error(ctx.Tr("repo.pulls.merge_out_of_date")) ctx.Redirect(issue.Link()) - return } else if models.IsErrSHADoesNotMatch(err) { log.Debug("MergeHeadOutOfDate error: %v", err) ctx.Flash.Error(ctx.Tr("repo.pulls.head_out_of_date")) ctx.Redirect(issue.Link()) - return } else if git.IsErrPushRejected(err) { log.Debug("MergePushRejected error: %v", err) pushrejErr := err.(*git.ErrPushRejected) @@ -997,11 +989,12 @@ func MergePullRequest(ctx *context.Context) { ctx.Flash.Error(flashError) } ctx.Redirect(issue.Link()) - return + } else { + ctx.ServerError("Merge", err) } - ctx.ServerError("Merge", err) return } + log.Trace("Pull request merged: %d", pr.ID) if err := stopTimerIfAvailable(ctx.Doer, issue); err != nil { ctx.ServerError("CreateOrStopIssueStopwatch", err) @@ -1012,7 +1005,7 @@ func MergePullRequest(ctx *context.Context) { if form.DeleteBranchAfterMerge { // Don't cleanup when other pr use this branch as head branch - exist, err := models.HasUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) + exist, err := models.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) if err != nil { ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) return @@ -1193,7 +1186,7 @@ func CleanUpPullRequest(ctx *context.Context) { } // Don't cleanup when there are other PR's that use this branch as head branch. - exist, err := models.HasUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) + exist, err := models.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) if err != nil { ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) return @@ -1304,7 +1297,7 @@ func deleteBranch(ctx *context.Context, pr *models.PullRequest, gitRepo *git.Rep return } - if err := models.AddDeletePRBranchComment(ctx.Doer, pr.BaseRepo, pr.IssueID, pr.HeadBranch); err != nil { + if err := models.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.IssueID, pr.HeadBranch); err != nil { // Do not fail here as branch has already been deleted log.Error("DeleteBranch: %v", err) } diff --git a/services/asymkey/sign.go b/services/asymkey/sign.go index 876cb7c205..6b17c017fc 100644 --- a/services/asymkey/sign.go +++ b/services/asymkey/sign.go @@ -317,7 +317,7 @@ Loop: if protectedBranch == nil { return false, "", nil, &ErrWontSign{approved} } - if protectedBranch.GetGrantedApprovalsCount(pr) < 1 { + if protectedBranch.GetGrantedApprovalsCount(ctx, pr) < 1 { return false, "", nil, &ErrWontSign{approved} } case baseSigned: diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index f40e99a044..5c3adc1cd3 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -6,6 +6,7 @@ package forms import ( + stdContext "context" "net/http" "net/url" "strings" @@ -601,7 +602,7 @@ func (f *MergePullRequestForm) Validate(req *http.Request, errs binding.Errors) } // SetDefaults if not provided for mergestyle and commit message -func (f *MergePullRequestForm) SetDefaults(pr *models.PullRequest) (err error) { +func (f *MergePullRequestForm) SetDefaults(ctx stdContext.Context, pr *models.PullRequest) (err error) { if f.Do == "" { f.Do = "merge" } @@ -610,9 +611,9 @@ func (f *MergePullRequestForm) SetDefaults(pr *models.PullRequest) (err error) { if len(f.MergeTitleField) == 0 { switch f.Do { case "merge", "rebase-merge": - f.MergeTitleField, err = pr.GetDefaultMergeMessage() + f.MergeTitleField, err = pr.GetDefaultMergeMessage(ctx) case "squash": - f.MergeTitleField, err = pr.GetDefaultSquashMessage() + f.MergeTitleField, err = pr.GetDefaultSquashMessage(ctx) } } diff --git a/services/issue/status.go b/services/issue/status.go index 1af4508b09..d2b4fc303e 100644 --- a/services/issue/status.go +++ b/services/issue/status.go @@ -5,6 +5,8 @@ package issue import ( + "context" + "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/db" user_model "code.gitea.io/gitea/models/user" @@ -14,10 +16,16 @@ import ( // ChangeStatus changes issue status to open or closed. func ChangeStatus(issue *models.Issue, doer *user_model.User, closed bool) error { - comment, err := models.ChangeIssueStatus(issue, doer, closed) + return changeStatusCtx(db.DefaultContext, issue, doer, closed) +} + +// changeStatusCtx changes issue status to open or closed. +// TODO: if context is not db.DefaultContext we get a deadlock!!! +func changeStatusCtx(ctx context.Context, issue *models.Issue, doer *user_model.User, closed bool) error { + comment, err := models.ChangeIssueStatus(ctx, issue, doer, closed) if err != nil { if models.IsErrDependenciesLeft(err) && closed { - if err := models.FinishIssueStopwatchIfPossible(db.DefaultContext, doer, issue); err != nil { + if err := models.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil { log.Error("Unable to stop stopwatch for issue[%d]#%d: %v", issue.ID, issue.Index, err) } } @@ -25,7 +33,7 @@ func ChangeStatus(issue *models.Issue, doer *user_model.User, closed bool) error } if closed { - if err := models.FinishIssueStopwatchIfPossible(db.DefaultContext, doer, issue); err != nil { + if err := models.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil { return err } } diff --git a/services/pull/check.go b/services/pull/check.go index eb3b17d555..64eb93104e 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -14,6 +14,7 @@ import ( "strings" "code.gitea.io/gitea/models" + "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" @@ -59,70 +60,72 @@ func AddToTaskQueue(pr *models.PullRequest) { } // CheckPullMergable check if the pull mergable based on all conditions (branch protection, merge options, ...) -func CheckPullMergable(ctx context.Context, doer *user_model.User, perm *models.Permission, pr *models.PullRequest, manuallMerge, force bool) error { - if pr.HasMerged { - return ErrHasMerged - } +func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *models.Permission, pr *models.PullRequest, manuallMerge, force bool) error { + return db.WithTx(func(ctx context.Context) error { + if pr.HasMerged { + return ErrHasMerged + } - if err := pr.LoadIssue(); err != nil { - return err - } else if pr.Issue.IsClosed { - return ErrIsClosed - } + if err := pr.LoadIssueCtx(ctx); err != nil { + return err + } else if pr.Issue.IsClosed { + return ErrIsClosed + } - if allowedMerge, err := IsUserAllowedToMerge(pr, *perm, doer); err != nil { - return err - } else if !allowedMerge { - return ErrUserNotAllowedToMerge - } + if allowedMerge, err := IsUserAllowedToMerge(ctx, pr, *perm, doer); err != nil { + return err + } else if !allowedMerge { + return ErrUserNotAllowedToMerge + } - if manuallMerge { - // don't check rules to "auto merge", doer is going to mark this pull as merged manually - return nil - } + if manuallMerge { + // don't check rules to "auto merge", doer is going to mark this pull as merged manually + return nil + } - if pr.IsWorkInProgress() { - return ErrIsWorkInProgress - } + if pr.IsWorkInProgress() { + return ErrIsWorkInProgress + } - if !pr.CanAutoMerge() { - return ErrNotMergableState - } + if !pr.CanAutoMerge() { + return ErrNotMergableState + } - if pr.IsChecking() { - return ErrIsChecking - } + if pr.IsChecking() { + return ErrIsChecking + } - if err := CheckPullBranchProtections(ctx, pr, false); err != nil { - if models.IsErrDisallowedToMerge(err) { - if force { - if isRepoAdmin, err2 := models.IsUserRepoAdmin(pr.BaseRepo, doer); err2 != nil { - return err2 - } else if !isRepoAdmin { - return err + if err := CheckPullBranchProtections(ctx, pr, false); err != nil { + if models.IsErrDisallowedToMerge(err) { + if force { + if isRepoAdmin, err2 := models.IsUserRepoAdminCtx(ctx, pr.BaseRepo, doer); err2 != nil { + return err2 + } else if !isRepoAdmin { + return err + } } + } else { + return err } - } else { + } + + if _, err := isSignedIfRequired(ctx, pr, doer); err != nil { return err } - } - if _, err := isSignedIfRequired(ctx, pr, doer); err != nil { - return err - } + if noDeps, err := models.IssueNoDependenciesLeft(ctx, pr.Issue); err != nil { + return err + } else if !noDeps { + return ErrDependenciesLeft + } - if noDeps, err := models.IssueNoDependenciesLeft(pr.Issue); err != nil { - return err - } else if !noDeps { - return ErrDependenciesLeft - } - - return nil + return nil + }, stdCtx) } // isSignedIfRequired check if merge will be signed if required func isSignedIfRequired(ctx context.Context, pr *models.PullRequest, doer *user_model.User) (bool, error) { - if err := pr.LoadProtectedBranch(); err != nil { + if err := pr.LoadProtectedBranchCtx(ctx); err != nil { return false, err } @@ -266,7 +269,7 @@ func manuallyMerged(ctx context.Context, pr *models.PullRequest) bool { pr.Merger = merger pr.MergerID = merger.ID - if merged, err := pr.SetMerged(); err != nil { + if merged, err := pr.SetMerged(ctx); err != nil { log.Error("PullRequest[%d].setMerged : %v", pr.ID, err) return false } else if !merged { diff --git a/services/pull/merge.go b/services/pull/merge.go index 330dffd5c7..ad93b9779f 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -17,6 +17,7 @@ import ( "time" "code.gitea.io/gitea/models" + "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" @@ -34,11 +35,11 @@ import ( // Merge merges pull request to base repository. // Caller should check PR is ready to be merged (review and status checks) // FIXME: add repoWorkingPull make sure two merges does not happen at same time. -func Merge(ctx context.Context, pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) (err error) { - if err = pr.LoadHeadRepoCtx(ctx); err != nil { +func Merge(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) error { + if err := pr.LoadHeadRepo(); err != nil { log.Error("LoadHeadRepo: %v", err) return fmt.Errorf("LoadHeadRepo: %v", err) - } else if err = pr.LoadBaseRepoCtx(ctx); err != nil { + } else if err := pr.LoadBaseRepo(); err != nil { log.Error("LoadBaseRepo: %v", err) return fmt.Errorf("LoadBaseRepo: %v", err) } @@ -59,7 +60,9 @@ func Merge(ctx context.Context, pr *models.PullRequest, doer *user_model.User, b go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "") }() - pr.MergedCommitID, err = rawMerge(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message) + // TODO: make it able to do this in a database session + mergeCtx := context.Background() + pr.MergedCommitID, err = rawMerge(mergeCtx, pr, doer, mergeStyle, expectedHeadCommitID, message) if err != nil { return err } @@ -68,18 +71,18 @@ func Merge(ctx context.Context, pr *models.PullRequest, doer *user_model.User, b pr.Merger = doer pr.MergerID = doer.ID - if _, err := pr.SetMerged(); err != nil { + if _, err := pr.SetMerged(db.DefaultContext); err != nil { log.Error("setMerged [%d]: %v", pr.ID, err) } - if err := pr.LoadIssue(); err != nil { + if err := pr.LoadIssueCtx(db.DefaultContext); err != nil { log.Error("loadIssue [%d]: %v", pr.ID, err) } - if err := pr.Issue.LoadRepo(ctx); err != nil { + if err := pr.Issue.LoadRepo(db.DefaultContext); err != nil { log.Error("loadRepo for issue [%d]: %v", pr.ID, err) } - if err := pr.Issue.Repo.GetOwner(ctx); err != nil { + if err := pr.Issue.Repo.GetOwner(db.DefaultContext); err != nil { log.Error("GetOwner for issue repo [%d]: %v", pr.ID, err) } @@ -89,17 +92,17 @@ func Merge(ctx context.Context, pr *models.PullRequest, doer *user_model.User, b cache.Remove(pr.Issue.Repo.GetCommitsCountCacheKey(pr.BaseBranch, true)) // Resolve cross references - refs, err := pr.ResolveCrossReferences() + refs, err := pr.ResolveCrossReferences(db.DefaultContext) if err != nil { log.Error("ResolveCrossReferences: %v", err) return nil } for _, ref := range refs { - if err = ref.LoadIssue(); err != nil { + if err = ref.LoadIssueCtx(db.DefaultContext); err != nil { return err } - if err = ref.Issue.LoadRepo(ctx); err != nil { + if err = ref.Issue.LoadRepo(db.DefaultContext); err != nil { return err } close := ref.RefAction == references.XRefActionCloses @@ -112,7 +115,6 @@ func Merge(ctx context.Context, pr *models.PullRequest, doer *user_model.User, b } } } - return nil } @@ -504,6 +506,8 @@ func rawMerge(ctx context.Context, pr *models.PullRequest, doer *user_model.User } // Push back to upstream. + // TODO: this cause an api call to "/api/internal/hook/post-receive/...", + // that prevents us from doint the whole merge in one db transaction if err := pushCmd.Run(&git.RunOpts{ Env: env, Dir: tmpBasePath, @@ -645,17 +649,17 @@ func getDiffTree(ctx context.Context, repoPath, baseBranch, headBranch string) ( } // IsUserAllowedToMerge check if user is allowed to merge PR with given permissions and branch protections -func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *user_model.User) (bool, error) { +func IsUserAllowedToMerge(ctx context.Context, pr *models.PullRequest, p models.Permission, user *user_model.User) (bool, error) { if user == nil { return false, nil } - err := pr.LoadProtectedBranch() + err := pr.LoadProtectedBranchCtx(ctx) if err != nil { return false, err } - if (p.CanWrite(unit.TypeCode) && pr.ProtectedBranch == nil) || (pr.ProtectedBranch != nil && models.IsUserMergeWhitelisted(pr.ProtectedBranch, user.ID, p)) { + if (p.CanWrite(unit.TypeCode) && pr.ProtectedBranch == nil) || (pr.ProtectedBranch != nil && models.IsUserMergeWhitelisted(ctx, pr.ProtectedBranch, user.ID, p)) { return true, nil } @@ -668,7 +672,7 @@ func CheckPullBranchProtections(ctx context.Context, pr *models.PullRequest, ski return fmt.Errorf("LoadBaseRepo: %v", err) } - if err = pr.LoadProtectedBranch(); err != nil { + if err = pr.LoadProtectedBranchCtx(ctx); err != nil { return fmt.Errorf("LoadProtectedBranch: %v", err) } if pr.ProtectedBranch == nil { @@ -685,17 +689,17 @@ func CheckPullBranchProtections(ctx context.Context, pr *models.PullRequest, ski } } - if !pr.ProtectedBranch.HasEnoughApprovals(pr) { + if !pr.ProtectedBranch.HasEnoughApprovals(ctx, pr) { return models.ErrDisallowedToMerge{ Reason: "Does not have enough approvals", } } - if pr.ProtectedBranch.MergeBlockedByRejectedReview(pr) { + if pr.ProtectedBranch.MergeBlockedByRejectedReview(ctx, pr) { return models.ErrDisallowedToMerge{ Reason: "There are requested changes", } } - if pr.ProtectedBranch.MergeBlockedByOfficialReviewRequests(pr) { + if pr.ProtectedBranch.MergeBlockedByOfficialReviewRequests(ctx, pr) { return models.ErrDisallowedToMerge{ Reason: "There are official review requests", } @@ -721,52 +725,58 @@ func CheckPullBranchProtections(ctx context.Context, pr *models.PullRequest, ski } // MergedManually mark pr as merged manually -func MergedManually(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, commitID string) (err error) { - prUnit, err := pr.BaseRepo.GetUnit(unit.TypePullRequests) - if err != nil { - return - } - prConfig := prUnit.PullRequestsConfig() +func MergedManually(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, commitID string) error { + if err := db.WithTx(func(ctx context.Context) error { + prUnit, err := pr.BaseRepo.GetUnitCtx(ctx, unit.TypePullRequests) + if err != nil { + return err + } + prConfig := prUnit.PullRequestsConfig() - // Check if merge style is correct and allowed - if !prConfig.IsMergeStyleAllowed(repo_model.MergeStyleManuallyMerged) { - return models.ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: repo_model.MergeStyleManuallyMerged} - } + // Check if merge style is correct and allowed + if !prConfig.IsMergeStyleAllowed(repo_model.MergeStyleManuallyMerged) { + return models.ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: repo_model.MergeStyleManuallyMerged} + } - if len(commitID) < 40 { - return fmt.Errorf("Wrong commit ID") - } - - commit, err := baseGitRepo.GetCommit(commitID) - if err != nil { - if git.IsErrNotExist(err) { + if len(commitID) < 40 { return fmt.Errorf("Wrong commit ID") } - return - } - ok, err := baseGitRepo.IsCommitInBranch(commitID, pr.BaseBranch) - if err != nil { - return - } - if !ok { - return fmt.Errorf("Wrong commit ID") - } + commit, err := baseGitRepo.GetCommit(commitID) + if err != nil { + if git.IsErrNotExist(err) { + return fmt.Errorf("Wrong commit ID") + } + return err + } + commitID = commit.ID.String() - pr.MergedCommitID = commitID - pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix()) - pr.Status = models.PullRequestStatusManuallyMerged - pr.Merger = doer - pr.MergerID = doer.ID + ok, err := baseGitRepo.IsCommitInBranch(commitID, pr.BaseBranch) + if err != nil { + return err + } + if !ok { + return fmt.Errorf("Wrong commit ID") + } - merged := false - if merged, err = pr.SetMerged(); err != nil { - return - } else if !merged { - return fmt.Errorf("SetMerged failed") + pr.MergedCommitID = commitID + pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix()) + pr.Status = models.PullRequestStatusManuallyMerged + pr.Merger = doer + pr.MergerID = doer.ID + + merged := false + if merged, err = pr.SetMerged(ctx); err != nil { + return err + } else if !merged { + return fmt.Errorf("SetMerged failed") + } + return nil + }); err != nil { + return err } notification.NotifyMergePullRequest(pr, doer) - 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()) + log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commitID) return nil } diff --git a/services/pull/update.go b/services/pull/update.go index 2d845e8504..08ff1cedfc 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -116,13 +116,13 @@ func IsUserAllowedToUpdate(ctx context.Context, pull *models.PullRequest, user * return false, false, err } - mergeAllowed, err = IsUserAllowedToMerge(pr, headRepoPerm, user) + mergeAllowed, err = IsUserAllowedToMerge(ctx, pr, headRepoPerm, user) if err != nil { return false, false, err } if pull.AllowMaintainerEdit { - mergeAllowedMaintainer, err := IsUserAllowedToMerge(pr, baseRepoPerm, user) + mergeAllowedMaintainer, err := IsUserAllowedToMerge(ctx, pr, baseRepoPerm, user) if err != nil { return false, false, err } diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index fef868af1f..8c43ace32e 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -329,7 +329,7 @@ {{.CsrfTokenHtml}}
- +
@@ -375,7 +375,7 @@ {{.CsrfTokenHtml}}
- +
@@ -401,7 +401,7 @@ {{.CsrfTokenHtml}}
- +