diff --git a/models/issues/issue.go b/models/issues/issue.go index fe347c271560e..1777fbb6a686f 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -46,23 +46,6 @@ func (err ErrIssueNotExist) Unwrap() error { return util.ErrNotExist } -// ErrIssueIsClosed represents a "IssueIsClosed" kind of error. -type ErrIssueIsClosed struct { - ID int64 - RepoID int64 - Index int64 -} - -// IsErrIssueIsClosed checks if an error is a ErrIssueNotExist. -func IsErrIssueIsClosed(err error) bool { - _, ok := err.(ErrIssueIsClosed) - return ok -} - -func (err ErrIssueIsClosed) Error() string { - return fmt.Sprintf("issue is closed [id: %d, repo_id: %d, index: %d]", err.ID, err.RepoID, err.Index) -} - // ErrNewIssueInsert is used when the INSERT statement in newIssue fails type ErrNewIssueInsert struct { OriginalError error @@ -78,22 +61,6 @@ func (err ErrNewIssueInsert) Error() string { return err.OriginalError.Error() } -// ErrIssueWasClosed is used when close a closed issue -type ErrIssueWasClosed struct { - ID int64 - Index int64 -} - -// IsErrIssueWasClosed checks if an error is a ErrIssueWasClosed. -func IsErrIssueWasClosed(err error) bool { - _, ok := err.(ErrIssueWasClosed) - return ok -} - -func (err ErrIssueWasClosed) Error() string { - return fmt.Sprintf("Issue [%d] %d was already closed", err.ID, err.Index) -} - var ErrIssueAlreadyChanged = util.NewInvalidArgumentErrorf("the issue is already changed") // Issue represents an issue or pull request of repository. diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index 479834045c6f9..7b3fe04aa5b10 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -28,38 +28,40 @@ import ( // UpdateIssueCols updates cols of issue func UpdateIssueCols(ctx context.Context, issue *Issue, cols ...string) error { - if _, err := db.GetEngine(ctx).ID(issue.ID).Cols(cols...).Update(issue); err != nil { - return err - } - return nil + _, err := db.GetEngine(ctx).ID(issue.ID).Cols(cols...).Update(issue) + return err } -func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed, isMergePull bool) (*Comment, error) { - // Reload the issue - currentIssue, err := GetIssueByID(ctx, issue.ID) - if err != nil { - return nil, err - } +// ErrIssueIsClosed is used when close a closed issue +type ErrIssueIsClosed struct { + ID int64 + RepoID int64 + Index int64 + IsPull bool +} - // Nothing should be performed if current status is same as target status - if currentIssue.IsClosed == isClosed { - if !issue.IsPull { - return nil, ErrIssueWasClosed{ - ID: issue.ID, - } - } - return nil, ErrPullWasClosed{ - ID: issue.ID, - } - } +// IsErrIssueIsClosed checks if an error is a ErrIssueIsClosed. +func IsErrIssueIsClosed(err error) bool { + _, ok := err.(ErrIssueIsClosed) + return ok +} - issue.IsClosed = isClosed - return doChangeIssueStatus(ctx, issue, doer, isMergePull) +func (err ErrIssueIsClosed) Error() string { + return fmt.Sprintf("%s [id: %d, repo_id: %d, index: %d] is already closed", util.Iif(err.IsPull, "Pull Request", "Issue"), err.ID, err.RepoID, err.Index) } -func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) { +func SetIssueAsClosed(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) { + if issue.IsClosed { + return nil, ErrIssueIsClosed{ + ID: issue.ID, + RepoID: issue.RepoID, + Index: issue.Index, + IsPull: issue.IsPull, + } + } + // Check for open dependencies - if issue.IsClosed && issue.Repo.IsDependenciesEnabled(ctx) { + if issue.Repo.IsDependenciesEnabled(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(ctx, issue) if err != nil { @@ -71,16 +73,63 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use } } - if issue.IsClosed { - issue.ClosedUnix = timeutil.TimeStampNow() - } else { - issue.ClosedUnix = 0 + issue.IsClosed = true + issue.ClosedUnix = timeutil.TimeStampNow() + + if cnt, err := db.GetEngine(ctx).ID(issue.ID).Cols("is_closed", "closed_unix"). + Where("is_closed = ?", false). + Update(issue); err != nil { + return nil, err + } else if cnt != 1 { + return nil, ErrIssueAlreadyChanged } - if err := UpdateIssueCols(ctx, issue, "is_closed", "closed_unix"); err != nil { + return updateIssueNumbers(ctx, issue, doer, util.Iif(isMergePull, CommentTypeMergePull, CommentTypeClose)) +} + +// ErrIssueIsOpen is used when reopen an opened issue +type ErrIssueIsOpen struct { + ID int64 + RepoID int64 + IsPull bool + Index int64 +} + +// IsErrIssueIsOpen checks if an error is a ErrIssueIsOpen. +func IsErrIssueIsOpen(err error) bool { + _, ok := err.(ErrIssueIsOpen) + return ok +} + +func (err ErrIssueIsOpen) Error() string { + return fmt.Sprintf("%s [id: %d, repo_id: %d, index: %d] is already open", util.Iif(err.IsPull, "Pull Request", "Issue"), err.ID, err.RepoID, err.Index) +} + +func setIssueAsReopen(ctx context.Context, issue *Issue, doer *user_model.User) (*Comment, error) { + if !issue.IsClosed { + return nil, ErrIssueIsOpen{ + ID: issue.ID, + RepoID: issue.RepoID, + Index: issue.Index, + IsPull: issue.IsPull, + } + } + + issue.IsClosed = false + issue.ClosedUnix = 0 + + if cnt, err := db.GetEngine(ctx).ID(issue.ID).Cols("is_closed", "closed_unix"). + Where("is_closed = ?", true). + Update(issue); err != nil { return nil, err + } else if cnt != 1 { + return nil, ErrIssueAlreadyChanged } + return updateIssueNumbers(ctx, issue, doer, CommentTypeReopen) +} + +func updateIssueNumbers(ctx context.Context, issue *Issue, doer *user_model.User, cmtType CommentType) (*Comment, error) { // Update issue count of labels if err := issue.LoadLabels(ctx); err != nil { return nil, err @@ -103,14 +152,6 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use return nil, err } - // New action comment - cmtType := CommentTypeClose - if !issue.IsClosed { - cmtType = CommentTypeReopen - } else if isMergePull { - cmtType = CommentTypeMergePull - } - return CreateComment(ctx, &CreateCommentOptions{ Type: cmtType, Doer: doer, @@ -134,7 +175,7 @@ func CloseIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comm } defer committer.Close() - comment, err := ChangeIssueStatus(ctx, issue, doer, true, false) + comment, err := SetIssueAsClosed(ctx, issue, doer, false) if err != nil { return nil, err } @@ -159,7 +200,7 @@ func ReopenIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Com } defer committer.Close() - comment, err := ChangeIssueStatus(ctx, issue, doer, false, false) + comment, err := setIssueAsReopen(ctx, issue, doer) if err != nil { return nil, err } diff --git a/models/issues/pull.go b/models/issues/pull.go index 786b2aa130d01..e3af00224dedb 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -80,22 +80,6 @@ func (err ErrPullRequestAlreadyExists) Unwrap() error { return util.ErrAlreadyExist } -// ErrPullWasClosed is used close a closed pull request -type ErrPullWasClosed struct { - ID int64 - Index int64 -} - -// IsErrPullWasClosed checks if an error is a ErrErrPullWasClosed. -func IsErrPullWasClosed(err error) bool { - _, ok := err.(ErrPullWasClosed) - return ok -} - -func (err ErrPullWasClosed) Error() string { - return fmt.Sprintf("Pull request [%d] %d was already closed", err.ID, err.Index) -} - // PullRequestType defines pull request type type PullRequestType int diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index af40cb3988c25..dba6aef9a3289 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -8,11 +8,9 @@ import ( "fmt" "net/http" - "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" - pull_model "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/cache" @@ -22,7 +20,7 @@ import ( "code.gitea.io/gitea/modules/private" repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" - timeutil "code.gitea.io/gitea/modules/timeutil" + "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" gitea_context "code.gitea.io/gitea/services/context" @@ -359,21 +357,9 @@ func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.H return } - pr.MergedCommitID = updates[len(updates)-1].NewCommitID - pr.MergedUnix = timeutil.TimeStampNow() - pr.Merger = pusher - pr.MergerID = pusher.ID - err = db.WithTx(ctx, func(ctx context.Context) error { - // Removing an auto merge pull and ignore if not exist - if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) { - return fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err) - } - if _, err := pull_service.SetMerged(ctx, pr); err != nil { - return fmt.Errorf("SetMerged failed: %s/%s Error: %v", ownerName, repoName, err) - } - return nil - }) - if err != nil { + // FIXME: Maybe we need a `PullRequestStatusMerged` status for PRs that are merged, currently we use the previous status + // here to keep it as before, that maybe PullRequestStatusMergeable + if _, err := pull_service.SetMerged(ctx, pr, updates[len(updates)-1].NewCommitID, timeutil.TimeStampNow(), pusher, pr.Status); err != nil { log.Error("Failed to update PR to merged: %v", err) ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{Err: "Failed to update PR to merged"}) } diff --git a/services/pull/check.go b/services/pull/check.go index f7aa8eb3695d2..e1adc3ca3bfa2 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -282,9 +282,6 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { return false } - pr.MergedCommitID = commit.ID.String() - pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix()) - pr.Status = issues_model.PullRequestStatusManuallyMerged merger, _ := user_model.GetUserByEmail(ctx, commit.Author.Email) // When the commit author is unknown set the BaseRepo owner as merger @@ -297,10 +294,8 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { } merger = pr.BaseRepo.Owner } - pr.Merger = merger - pr.MergerID = merger.ID - if merged, err := SetMerged(ctx, pr); err != nil { + if merged, err := SetMerged(ctx, pr, commit.ID.String(), timeutil.TimeStamp(commit.Author.When.Unix()), merger, issues_model.PullRequestStatusManuallyMerged); err != nil { log.Error("%-v setMerged : %v", pr, err) return false } else if !merged { diff --git a/services/pull/merge.go b/services/pull/merge.go index 879fe5a54085f..9c909ef7958b1 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -17,6 +17,7 @@ import ( git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" + pull_model "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" @@ -632,14 +633,8 @@ func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *use return fmt.Errorf("Wrong commit ID") } - pr.MergedCommitID = commitID - pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix()) - pr.Status = issues_model.PullRequestStatusManuallyMerged - pr.Merger = doer - pr.MergerID = doer.ID - var merged bool - if merged, err = SetMerged(ctx, pr); err != nil { + if merged, err = SetMerged(ctx, pr, commitID, timeutil.TimeStamp(commit.Author.When.Unix()), doer, issues_model.PullRequestStatusManuallyMerged); err != nil { return err } else if !merged { return fmt.Errorf("SetMerged failed") @@ -658,41 +653,35 @@ func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *use } // SetMerged sets a pull request to merged and closes the corresponding issue -func SetMerged(ctx context.Context, pr *issues_model.PullRequest) (bool, error) { +func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID string, mergedTimeStamp timeutil.TimeStamp, merger *user_model.User, mergeStatus issues_model.PullRequestStatus) (bool, error) { if pr.HasMerged { return false, fmt.Errorf("PullRequest[%d] already merged", pr.Index) } - if pr.MergedCommitID == "" || pr.MergedUnix == 0 || pr.Merger == nil { - return false, fmt.Errorf("unable to merge PullRequest[%d], some required fields are empty", pr.Index) - } pr.HasMerged = true - sess := db.GetEngine(ctx) + pr.MergedCommitID = mergedCommitID + pr.MergedUnix = mergedTimeStamp + pr.Merger = merger + pr.MergerID = merger.ID + pr.Status = mergeStatus + // reset the conflicted files as there cannot be any if we're merged + pr.ConflictedFiles = []string{} - if _, err := sess.Exec("UPDATE `issue` SET `repo_id` = `repo_id` WHERE `id` = ?", pr.IssueID); err != nil { - return false, err + if pr.MergedCommitID == "" || pr.MergedUnix == 0 || pr.Merger == nil { + return false, fmt.Errorf("unable to merge PullRequest[%d], some required fields are empty", pr.Index) } - if _, err := sess.Exec("UPDATE `pull_request` SET `issue_id` = `issue_id` WHERE `id` = ?", pr.ID); err != nil { + ctx, committer, err := db.TxContext(ctx) + if err != nil { return false, err } + defer committer.Close() pr.Issue = nil if err := pr.LoadIssue(ctx); err != nil { return false, err } - if tmpPr, err := issues_model.GetPullRequestByID(ctx, pr.ID); err != nil { - return false, err - } else if tmpPr.HasMerged { - if pr.Issue.IsClosed { - return false, nil - } - return false, fmt.Errorf("PullRequest[%d] already merged but it's associated issue [%d] is not closed", pr.Index, pr.IssueID) - } else if pr.Issue.IsClosed { - return false, fmt.Errorf("PullRequest[%d] already closed", pr.Index) - } - if err := pr.Issue.LoadRepo(ctx); err != nil { return false, err } @@ -701,16 +690,28 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest) (bool, error) return false, err } - if _, err := issues_model.ChangeIssueStatus(ctx, pr.Issue, pr.Merger, true, true); err != nil { - return false, fmt.Errorf("ChangeIssueStatus: %w", err) + // Removing an auto merge pull and ignore if not exist + if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) { + return false, fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", pr.ID, err) } - // reset the conflicted files as there cannot be any if we're merged - pr.ConflictedFiles = []string{} + // Set issue as closed + if _, err := issues_model.SetIssueAsClosed(ctx, pr.Issue, pr.Merger, true); err != nil { + return false, fmt.Errorf("ChangeIssueStatus: %w", err) + } // We need to save all of the data used to compute this merge as it may have already been changed by TestPatch. FIXME: need to set some state to prevent TestPatch from running whilst we are merging. - if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix, conflicted_files").Update(pr); err != nil { + if cnt, err := db.GetEngine(ctx).Where("id = ?", pr.ID). + And("has_merged = ?", false). + Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix, conflicted_files"). + Update(pr); err != nil { return false, fmt.Errorf("failed to update pr[%d]: %w", pr.ID, err) + } else if cnt != 1 { + return false, issues_model.ErrIssueAlreadyChanged + } + + if err := committer.Commit(); err != nil { + return false, err } return true, nil diff --git a/services/pull/pull.go b/services/pull/pull.go index 85c36bb16aff8..52abf35cec410 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -265,6 +265,7 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer ID: pr.Issue.ID, RepoID: pr.Issue.RepoID, Index: pr.Issue.Index, + IsPull: true, } } @@ -707,7 +708,7 @@ func CloseBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, var errs errlist for _, pr := range prs { - if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) { + if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrIssueIsClosed(err) && !issues_model.IsErrDependenciesLeft(err) { errs = append(errs, err) } } @@ -741,7 +742,7 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re if pr.BaseRepoID == repo.ID { continue } - if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrPullWasClosed(err) { + if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrIssueIsClosed(err) { errs = append(errs, err) } }