[v12.0/forgejo] fix: prevent pull requests from being merged multiple times (#8862)

Backport of https://codeberg.org/forgejo/forgejo/pulls/8842

Contains a partial cherry-pick of 184e068f37, for the parts the PR depends on. The whole commit is way too involved to cherry-pick as a whole.

Co-authored-by: Danko Aleksejevs <danko@very.lv>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8862
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: BtbN <btbn@btbn.de>
Co-committed-by: BtbN <btbn@btbn.de>

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

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

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

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)