diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json index 4f1c3904e2..3262b49f6c 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 9360ff1335..d978d24e8b 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 4e365f24ea..1e3eda9b61 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 f69f8a87b4..c279147399 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -208,7 +208,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 { @@ -216,9 +239,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 2a26759956..7d3b7538d6 100644 --- a/services/pull/merge_test.go +++ b/services/pull/merge_test.go @@ -6,7 +6,15 @@ package pull import ( "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" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_expandDefaultMergeMessage(t *testing.T) { @@ -90,3 +98,29 @@ func TestAddCommitMessageTailer(t *testing.T) { assert.Equal(t, "title\n\nTest-tailer: v1\nTest-tailer: v2", AddCommitMessageTrailer("title\n\nTest-tailer: v1", "Test-tailer", "v2")) assert.Equal(t, "title\n\nTest-tailer: v1\nTest-tailer: v2", AddCommitMessageTrailer("title\n\nTest-tailer: v1\n", "Test-tailer", "v2")) } + +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 75d1f98914..7bbab2751a 100644 --- a/tests/test_utils.go +++ b/tests/test_utils.go @@ -42,6 +42,7 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "xorm.io/xorm/convert" ) func exitf(format string, args ...any) { @@ -342,6 +343,7 @@ type DeclarativeRepoOptions struct { Name optional.Option[string] EnabledUnits optional.Option[[]unit_model.Type] DisabledUnits optional.Option[[]unit_model.Type] + UnitConfig optional.Option[map[unit_model.Type]convert.Conversion] Files optional.Option[[]*files_service.ChangeRepoFile] WikiBranch optional.Option[string] AutoInit optional.Option[bool] @@ -390,9 +392,14 @@ func CreateDeclarativeRepoWithOptions(t *testing.T, owner *user_model.User, opts enabledUnits = make([]repo_model.RepoUnit, len(units)) for i, unitType := range units { + var config convert.Conversion + if cfg, ok := opts.UnitConfig.Value()[unitType]; ok { + config = cfg + } enabledUnits[i] = repo_model.RepoUnit{ RepoID: repo.ID, Type: unitType, + Config: config, } } } @@ -470,6 +477,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)