diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json index 5d489a167a..55abe17f07 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -44,6 +44,7 @@ "relativetime.2months": "two months ago", "relativetime.1year": "last year", "relativetime.2years": "two years ago", + "repo.pulls.already_merged": "Merge failed: This pull request has already been merged.", "repo.pulls.merged_title_desc": { "one": "merged %[1]d commit from %[2]s into %[3]s %[4]s", "other": "merged %[1]d commits from %[2]s into %[3]s %[4]s" diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index e7ff533d6a..812468f6b4 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1016,6 +1016,9 @@ func MergePullRequest(ctx *context.APIContext) { } else if models.IsErrMergeUnrelatedHistories(err) { conflictError := err.(models.ErrMergeUnrelatedHistories) ctx.JSON(http.StatusConflict, conflictError) + } else if models.IsErrPullRequestHasMerged(err) { + conflictError := err.(models.ErrPullRequestHasMerged) + ctx.JSON(http.StatusConflict, conflictError) } else if git.IsErrPushOutOfDate(err) { ctx.Error(http.StatusConflict, "Merge", "merge push out of date") } else if models.IsErrSHADoesNotMatch(err) { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 48e132df5c..e0ad172ccc 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1445,6 +1445,10 @@ func MergePullRequest(ctx *context.Context) { log.Debug("MergeUnrelatedHistories error: %v", err) ctx.Flash.Error(ctx.Tr("repo.pulls.unrelated_histories")) ctx.JSONRedirect(issue.Link()) + } else if models.IsErrPullRequestHasMerged(err) { + log.Debug("MergePullRequestHasMerged error: %v", err) + ctx.Flash.Error(ctx.Tr("repo.pulls.already_merged")) + ctx.JSONRedirect(issue.Link()) } else if git.IsErrPushOutOfDate(err) { log.Debug("MergePushOutOfDate error: %v", err) ctx.Flash.Error(ctx.Tr("repo.pulls.merge_out_of_date")) diff --git a/services/pull/merge.go b/services/pull/merge.go index 55ce869497..a2542176f0 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -239,7 +239,30 @@ func AddCommitMessageTrailer(message, tailerKey, tailerValue string) string { // Merge merges pull request to base repository. // Caller should check PR is ready to be merged (review and status checks) func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string, wasAutoMerged bool) error { - if err := pr.LoadBaseRepo(ctx); err != nil { + pullWorkingPool.CheckIn(fmt.Sprint(pr.ID)) + defer pullWorkingPool.CheckOut(fmt.Sprint(pr.ID)) + + pr, err := issues_model.GetPullRequestByID(ctx, pr.ID) + if err != nil { + log.Error("Unable to load pull request itself: %v", err) + return fmt.Errorf("unable to load pull request itself: %w", err) + } + + if pr.HasMerged { + return models.ErrPullRequestHasMerged{ + ID: pr.ID, + IssueID: pr.IssueID, + HeadRepoID: pr.HeadRepoID, + BaseRepoID: pr.BaseRepoID, + HeadBranch: pr.HeadBranch, + BaseBranch: pr.BaseBranch, + } + } + + if err := pr.LoadIssue(ctx); err != nil { + log.Error("Unable to load issue: %v", err) + return fmt.Errorf("unable to load issue: %w", err) + } else if err := pr.LoadBaseRepo(ctx); err != nil { log.Error("Unable to load base repo: %v", err) return fmt.Errorf("unable to load base repo: %w", err) } else if err := pr.LoadHeadRepo(ctx); err != nil { @@ -247,9 +270,6 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U return fmt.Errorf("unable to load head repo: %w", err) } - pullWorkingPool.CheckIn(fmt.Sprint(pr.ID)) - defer pullWorkingPool.CheckOut(fmt.Sprint(pr.ID)) - prUnit, err := pr.BaseRepo.GetUnit(ctx, unit.TypePullRequests) if err != nil { log.Error("pr.BaseRepo.GetUnit(unit.TypePullRequests): %v", err) diff --git a/services/pull/merge_test.go b/services/pull/merge_test.go index 3684c2120a..aa033b8cdb 100644 --- a/services/pull/merge_test.go +++ b/services/pull/merge_test.go @@ -9,7 +9,12 @@ import ( "strings" "testing" + "forgejo.org/models" + issues_model "forgejo.org/models/issues" repo_model "forgejo.org/models/repo" + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + "forgejo.org/modules/gitrepo" "forgejo.org/modules/setting" "forgejo.org/modules/test" @@ -146,3 +151,29 @@ func TestLoadMergeMessageTemplates(t *testing.T) { require.NoError(t, LoadMergeMessageTemplates()) assert.Empty(t, mergeMessageTemplates) } + +func TestMergeMergedPR(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + require.NoError(t, pr.LoadBaseRepo(t.Context())) + + gitRepo, err := gitrepo.OpenRepository(t.Context(), pr.BaseRepo) + require.NoError(t, err) + defer gitRepo.Close() + + assert.True(t, pr.HasMerged) + pr.HasMerged = false + + err = Merge(t.Context(), pr, doer, gitRepo, repo_model.MergeStyleRebase, "", "I should not exist", false) + require.Error(t, err) + assert.True(t, models.IsErrPullRequestHasMerged(err)) + + if mergeErr, ok := err.(models.ErrPullRequestHasMerged); ok { + assert.Equal(t, pr.ID, mergeErr.ID) + assert.Equal(t, pr.IssueID, mergeErr.IssueID) + assert.Equal(t, pr.HeadBranch, mergeErr.HeadBranch) + assert.Equal(t, pr.BaseBranch, mergeErr.BaseBranch) + } +} diff --git a/tests/test_utils.go b/tests/test_utils.go index ee786624a2..d72ac476da 100644 --- a/tests/test_utils.go +++ b/tests/test_utils.go @@ -495,6 +495,25 @@ func CreateDeclarativeRepo(t *testing.T, owner *user_model.User, name string, en } if enabledUnits != nil { opts.EnabledUnits = optional.Some(enabledUnits) + + for _, unitType := range enabledUnits { + if unitType == unit_model.TypePullRequests { + opts.UnitConfig = optional.Some(map[unit_model.Type]convert.Conversion{ + unit_model.TypePullRequests: &repo_model.PullRequestsConfig{ + AllowMerge: true, + AllowRebase: true, + AllowRebaseMerge: true, + AllowSquash: true, + AllowFastForwardOnly: true, + AllowManualMerge: true, + AllowRebaseUpdate: true, + DefaultMergeStyle: repo_model.MergeStyleMerge, + DefaultUpdateStyle: repo_model.UpdateStyleMerge, + }, + }) + break + } + } } if disabledUnits != nil { opts.DisabledUnits = optional.Some(disabledUnits)