From 97e3347894a8fd4b7a66e2335b3ece9a020c25c8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 Jan 2025 00:57:31 -0800 Subject: [PATCH 1/7] Add missed transaction on setmerged --- models/issues/issue_update.go | 75 ++++++++++++---------------- routers/private/hook_post_receive.go | 2 + services/pull/check.go | 2 + services/pull/merge.go | 38 ++++++-------- 4 files changed, 51 insertions(+), 66 deletions(-) diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index 479834045c6f9..43491a0a1e375 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -28,38 +28,13 @@ 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 -} - -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 - } - - // 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, - } - } - - issue.IsClosed = isClosed - return doChangeIssueStatus(ctx, issue, doer, isMergePull) + _, err := db.GetEngine(ctx).ID(issue.ID).Cols(cols...).Update(issue) + return err } -func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) { +func closeIssue(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) { // 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 +46,36 @@ 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)) +} + +func SetIssueAsReopen(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) { + 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 +98,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 +121,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 := closeIssue(ctx, issue, doer, false) if err != nil { return nil, err } @@ -159,7 +146,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, false) if err != nil { return nil, err } diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index af40cb3988c25..bfceabe72ec1b 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -363,6 +363,8 @@ func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.H pr.MergedUnix = timeutil.TimeStampNow() pr.Merger = pusher pr.MergerID = pusher.ID + // reset the conflicted files as there cannot be any if we're merged + pr.ConflictedFiles = []string{} 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) { diff --git a/services/pull/check.go b/services/pull/check.go index f7aa8eb3695d2..e2d25149558d8 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -299,6 +299,8 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { } pr.Merger = merger pr.MergerID = merger.ID + // reset the conflicted files as there cannot be any if we're merged + pr.ConflictedFiles = []string{} if merged, err := SetMerged(ctx, pr); err != nil { log.Error("%-v setMerged : %v", pr, err) diff --git a/services/pull/merge.go b/services/pull/merge.go index 879fe5a54085f..5b3fe5eb9d2b0 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -637,6 +637,8 @@ func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *use pr.Status = issues_model.PullRequestStatusManuallyMerged pr.Merger = doer pr.MergerID = doer.ID + // reset the conflicted files as there cannot be any if we're merged + pr.ConflictedFiles = []string{} var merged bool if merged, err = SetMerged(ctx, pr); err != nil { @@ -667,32 +669,18 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest) (bool, error) } pr.HasMerged = true - sess := db.GetEngine(ctx) - if _, err := sess.Exec("UPDATE `issue` SET `repo_id` = `repo_id` WHERE `id` = ?", pr.IssueID); err != nil { - return false, err - } - - 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 +689,22 @@ 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 { + if _, err := issues_model.SetIssueAsReopen(ctx, pr.Issue, pr.Merger, true); err != nil { return false, fmt.Errorf("ChangeIssueStatus: %w", err) } - // reset the conflicted files as there cannot be any if we're merged - pr.ConflictedFiles = []string{} - // 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 From 011ad7403f30b7381d5c9a88a38fd99c9f5f13a2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 Jan 2025 01:05:40 -0800 Subject: [PATCH 2/7] Add errors --- models/issues/issue.go | 16 ------- models/issues/issue_update.go | 86 +++++++++++++++++++++++++++++++++++ models/issues/pull.go | 16 ------- 3 files changed, 86 insertions(+), 32 deletions(-) diff --git a/models/issues/issue.go b/models/issues/issue.go index fe347c271560e..c0e5928ed0ef4 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -78,22 +78,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 43491a0a1e375..6fc0df3cb2634 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -32,7 +32,50 @@ func UpdateIssueCols(ctx context.Context, issue *Issue, cols ...string) error { return err } +// 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) +} + +// 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) +} + func closeIssue(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) { + if issue.IsClosed { + if !issue.IsPull { + return nil, ErrIssueWasClosed{ + ID: issue.ID, + } + } + return nil, ErrPullWasClosed{ + ID: issue.ID, + } + } + // Check for open dependencies 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 @@ -60,7 +103,50 @@ func closeIssue(ctx context.Context, issue *Issue, doer *user_model.User, isMerg return updateIssueNumbers(ctx, issue, doer, util.Iif(isMergePull, CommentTypeMergePull, CommentTypeClose)) } +// ErrIssueWasOpened is used when reopen an opened issue +type ErrIssueWasOpened struct { + ID int64 + Index int64 +} + +// IsErrIssueWasOpened checks if an error is a ErrIssueWasOpened. +func IsErrIssueWasOpened(err error) bool { + _, ok := err.(ErrIssueWasOpened) + return ok +} + +func (err ErrIssueWasOpened) Error() string { + return fmt.Sprintf("Issue [%d] %d was already opened", err.ID, err.Index) +} + +// ErrPullWasOpened is used reopen an opened pull request +type ErrPullWasOpened struct { + ID int64 + Index int64 +} + +// ErrPullWasOpened checks if an error is a ErrPullWasOpened. +func IsErrPullWasOpened(err error) bool { + _, ok := err.(ErrPullWasOpened) + return ok +} + +func (err ErrPullWasOpened) Error() string { + return fmt.Sprintf("Pull request [%d] %d was already opened", err.ID, err.Index) +} + func SetIssueAsReopen(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) { + if !issue.IsClosed { + if !issue.IsPull { + return nil, ErrIssueWasOpened{ + ID: issue.ID, + } + } + return nil, ErrPullWasOpened{ + ID: issue.ID, + } + } + issue.IsClosed = false issue.ClosedUnix = 0 diff --git a/models/issues/pull.go b/models/issues/pull.go index 8c43eb19ddfda..8a7c7eabad527 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 From 26f645523919eca39809a86e8e9cba7f95575fb3 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 Jan 2025 12:27:33 -0800 Subject: [PATCH 3/7] Some improvements --- models/issues/issue_update.go | 74 +++++++--------------------- routers/private/hook_post_receive.go | 14 +----- services/pull/merge.go | 9 +++- services/pull/pull.go | 4 +- 4 files changed, 29 insertions(+), 72 deletions(-) diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index 6fc0df3cb2634..a3ae0a232f1c6 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -34,8 +34,9 @@ func UpdateIssueCols(ctx context.Context, issue *Issue, cols ...string) error { // ErrIssueWasClosed is used when close a closed issue type ErrIssueWasClosed struct { - ID int64 - Index int64 + ID int64 + Index int64 + IsPull bool } // IsErrIssueWasClosed checks if an error is a ErrIssueWasClosed. @@ -45,34 +46,14 @@ func IsErrIssueWasClosed(err error) bool { } func (err ErrIssueWasClosed) Error() string { - return fmt.Sprintf("Issue [%d] %d was already closed", err.ID, err.Index) + return fmt.Sprintf("%s [%d] %d was already closed", util.Iif(err.IsPull, "Pull Request", "Issue"), err.ID, err.Index) } -// 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) -} - -func closeIssue(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 { - if !issue.IsPull { - return nil, ErrIssueWasClosed{ - ID: issue.ID, - } - } - return nil, ErrPullWasClosed{ - ID: issue.ID, + return nil, ErrIssueWasClosed{ + ID: issue.ID, + IsPull: issue.IsPull, } } @@ -105,8 +86,9 @@ func closeIssue(ctx context.Context, issue *Issue, doer *user_model.User, isMerg // ErrIssueWasOpened is used when reopen an opened issue type ErrIssueWasOpened struct { - ID int64 - Index int64 + ID int64 + IsPull bool + Index int64 } // IsErrIssueWasOpened checks if an error is a ErrIssueWasOpened. @@ -116,34 +98,14 @@ func IsErrIssueWasOpened(err error) bool { } func (err ErrIssueWasOpened) Error() string { - return fmt.Sprintf("Issue [%d] %d was already opened", err.ID, err.Index) + return fmt.Sprintf("%s [%d] %d was already opened", util.Iif(err.IsPull, "Pull Request", "Issue"), err.ID, err.Index) } -// ErrPullWasOpened is used reopen an opened pull request -type ErrPullWasOpened struct { - ID int64 - Index int64 -} - -// ErrPullWasOpened checks if an error is a ErrPullWasOpened. -func IsErrPullWasOpened(err error) bool { - _, ok := err.(ErrPullWasOpened) - return ok -} - -func (err ErrPullWasOpened) Error() string { - return fmt.Sprintf("Pull request [%d] %d was already opened", err.ID, err.Index) -} - -func SetIssueAsReopen(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) { +func setIssueAsReopen(ctx context.Context, issue *Issue, doer *user_model.User) (*Comment, error) { if !issue.IsClosed { - if !issue.IsPull { - return nil, ErrIssueWasOpened{ - ID: issue.ID, - } - } - return nil, ErrPullWasOpened{ - ID: issue.ID, + return nil, ErrIssueWasOpened{ + ID: issue.ID, + IsPull: issue.IsPull, } } @@ -207,7 +169,7 @@ func CloseIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comm } defer committer.Close() - comment, err := closeIssue(ctx, issue, doer, false) + comment, err := SetIssueAsClosed(ctx, issue, doer, false) if err != nil { return nil, err } @@ -232,7 +194,7 @@ func ReopenIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Com } defer committer.Close() - comment, err := SetIssueAsReopen(ctx, issue, doer, false) + comment, err := setIssueAsReopen(ctx, issue, doer) if err != nil { return nil, err } diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index bfceabe72ec1b..d38b184ea7547 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" @@ -365,17 +363,7 @@ func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.H pr.MergerID = pusher.ID // reset the conflicted files as there cannot be any if we're merged pr.ConflictedFiles = []string{} - 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 { + if _, err := pull_service.SetMerged(ctx, pr); 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/merge.go b/services/pull/merge.go index 5b3fe5eb9d2b0..f8dab8fcd9319 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" @@ -689,7 +690,13 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest) (bool, error) return false, err } - if _, err := issues_model.SetIssueAsReopen(ctx, pr.Issue, pr.Merger, true); err != nil { + // 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) + } + + // Set issue as closed + if _, err := issues_model.SetIssueAsClosed(ctx, pr.Issue, pr.Merger, true); err != nil { return false, fmt.Errorf("ChangeIssueStatus: %w", err) } diff --git a/services/pull/pull.go b/services/pull/pull.go index 85c36bb16aff8..1c6e8f66573d0 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -707,7 +707,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.IsErrIssueWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) { errs = append(errs, err) } } @@ -741,7 +741,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.IsErrIssueWasClosed(err) { errs = append(errs, err) } } From 11e634ea6640b75e9600dd6d0f59ed96c94a11f0 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 Jan 2025 14:01:09 -0800 Subject: [PATCH 4/7] Fix bug --- models/issues/issue_update.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index a3ae0a232f1c6..f70827896f9b5 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -74,7 +74,7 @@ func SetIssueAsClosed(ctx context.Context, issue *Issue, doer *user_model.User, issue.ClosedUnix = timeutil.TimeStampNow() if cnt, err := db.GetEngine(ctx).ID(issue.ID).Cols("is_closed", "closed_unix"). - Where("is_closed == ?", false). + Where("is_closed = ?", false). Update(issue); err != nil { return nil, err } else if cnt != 1 { @@ -113,7 +113,7 @@ func setIssueAsReopen(ctx context.Context, issue *Issue, doer *user_model.User) issue.ClosedUnix = 0 if cnt, err := db.GetEngine(ctx).ID(issue.ID).Cols("is_closed", "closed_unix"). - Where("is_closed == ?", true). + Where("is_closed = ?", true). Update(issue); err != nil { return nil, err } else if cnt != 1 { From a956456763cf2f07cb153bb0d3de970923718f37 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 Jan 2025 14:52:26 -0800 Subject: [PATCH 5/7] refactor setmerge parameters --- routers/private/hook_post_receive.go | 10 ++-------- services/pull/check.go | 9 +-------- services/pull/merge.go | 24 ++++++++++++------------ 3 files changed, 15 insertions(+), 28 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index d38b184ea7547..262714bec656b 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -20,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" @@ -357,13 +357,7 @@ 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 - // reset the conflicted files as there cannot be any if we're merged - pr.ConflictedFiles = []string{} - if _, err := pull_service.SetMerged(ctx, pr); err != nil { + 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 e2d25149558d8..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,12 +294,8 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { } merger = pr.BaseRepo.Owner } - pr.Merger = merger - pr.MergerID = merger.ID - // reset the conflicted files as there cannot be any if we're merged - pr.ConflictedFiles = []string{} - 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 f8dab8fcd9319..9c909ef7958b1 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -633,16 +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 - // reset the conflicted files as there cannot be any if we're merged - pr.ConflictedFiles = []string{} - 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") @@ -661,16 +653,24 @@ 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) } + + pr.HasMerged = true + 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 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 - ctx, committer, err := db.TxContext(ctx) if err != nil { return false, err From 47c1516e64cf6bc2dc9ef17477e8beb5c9a5bf4c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 Jan 2025 18:09:49 -0800 Subject: [PATCH 6/7] Fix error descriptions --- models/issues/issue.go | 17 ---------------- models/issues/issue_update.go | 38 ++++++++++++++++++++--------------- services/pull/pull.go | 5 +++-- 3 files changed, 25 insertions(+), 35 deletions(-) diff --git a/models/issues/issue.go b/models/issues/issue.go index c0e5928ed0ef4..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 diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index f70827896f9b5..7b3fe04aa5b10 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -32,27 +32,30 @@ func UpdateIssueCols(ctx context.Context, issue *Issue, cols ...string) error { return err } -// ErrIssueWasClosed is used when close a closed issue -type ErrIssueWasClosed struct { +// ErrIssueIsClosed is used when close a closed issue +type ErrIssueIsClosed struct { ID int64 + RepoID int64 Index int64 IsPull bool } -// IsErrIssueWasClosed checks if an error is a ErrIssueWasClosed. -func IsErrIssueWasClosed(err error) bool { - _, ok := err.(ErrIssueWasClosed) +// IsErrIssueIsClosed checks if an error is a ErrIssueIsClosed. +func IsErrIssueIsClosed(err error) bool { + _, ok := err.(ErrIssueIsClosed) return ok } -func (err ErrIssueWasClosed) Error() string { - return fmt.Sprintf("%s [%d] %d was already closed", util.Iif(err.IsPull, "Pull Request", "Issue"), err.ID, err.Index) +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 SetIssueAsClosed(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) { if issue.IsClosed { - return nil, ErrIssueWasClosed{ + return nil, ErrIssueIsClosed{ ID: issue.ID, + RepoID: issue.RepoID, + Index: issue.Index, IsPull: issue.IsPull, } } @@ -84,27 +87,30 @@ func SetIssueAsClosed(ctx context.Context, issue *Issue, doer *user_model.User, return updateIssueNumbers(ctx, issue, doer, util.Iif(isMergePull, CommentTypeMergePull, CommentTypeClose)) } -// ErrIssueWasOpened is used when reopen an opened issue -type ErrIssueWasOpened struct { +// ErrIssueIsOpen is used when reopen an opened issue +type ErrIssueIsOpen struct { ID int64 + RepoID int64 IsPull bool Index int64 } -// IsErrIssueWasOpened checks if an error is a ErrIssueWasOpened. -func IsErrIssueWasOpened(err error) bool { - _, ok := err.(ErrIssueWasOpened) +// IsErrIssueIsOpen checks if an error is a ErrIssueIsOpen. +func IsErrIssueIsOpen(err error) bool { + _, ok := err.(ErrIssueIsOpen) return ok } -func (err ErrIssueWasOpened) Error() string { - return fmt.Sprintf("%s [%d] %d was already opened", util.Iif(err.IsPull, "Pull Request", "Issue"), err.ID, err.Index) +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, ErrIssueWasOpened{ + return nil, ErrIssueIsOpen{ ID: issue.ID, + RepoID: issue.RepoID, + Index: issue.Index, IsPull: issue.IsPull, } } diff --git a/services/pull/pull.go b/services/pull/pull.go index 1c6e8f66573d0..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.IsErrIssueWasClosed(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.IsErrIssueWasClosed(err) { + if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrIssueIsClosed(err) { errs = append(errs, err) } } From 4eff10f8d1d8ba6777a4fa2ef10a02911ff0c7aa Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 Jan 2025 18:18:22 -0800 Subject: [PATCH 7/7] Add some comments --- routers/private/hook_post_receive.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 262714bec656b..dba6aef9a3289 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -357,6 +357,8 @@ func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.H return } + // 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"})