fix: prevent pull requests from being merged multiple times (#8842)

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8842
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

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 <code>%[2]s</code> into <code>%[3]s</code> %[4]s",
         "other": "merged %[1]d commits from <code>%[2]s</code> into <code>%[3]s</code> %[4]s"

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) {

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"))

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)

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)
+	}
+}

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)