Merge pull request #32 from wxiaoguang/lunny/transaction_merge

Some code improvements and add tests

Author: lunny

cmd/hook.go

@@ -352,7 +352,7 @@ Gitea or set your environment appropriately.`, "")
 		GitQuarantinePath:               os.Getenv(private.GitQuarantinePath),
 		GitPushOptions:                  pushOptions(),
 		PullRequestID:                   prID,
-		PushTrigger:                     os.Getenv(repo_module.EnvPushTrigger),
+		PushTrigger:                     repo_module.PushTrigger(os.Getenv(repo_module.EnvPushTrigger)),
 	}
 	oldCommitIDs := make([]string, hookBatchSize)
 	newCommitIDs := make([]string, hookBatchSize)

modules/private/hook.go

@@ -12,6 +12,7 @@ import (
 
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/optional"
+	"code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/setting"
 )
 
@@ -54,7 +55,7 @@ type HookOptions struct {
 	GitQuarantinePath               string
 	GitPushOptions                  GitPushOptions
 	PullRequestID                   int64
-	PushTrigger                     string
+	PushTrigger                     repository.PushTrigger
 	DeployKeyID                     int64 // if the pusher is a DeployKey, then UserID is the repo's org user.
 	IsUncyclo                          bool
 	ActionPerm                      int

modules/repository/env.go

@@ -34,7 +34,8 @@ const (
 type PushTrigger string
 
 const (
-	PushTriggerPRMergeToBase PushTrigger = "pr-merge-to-base"
+	PushTriggerPRMergeToBase    PushTrigger = "pr-merge-to-base"
+	PushTriggerPRUpdateWithBase PushTrigger = "pr-update-with-base"
 )
 
 // InternalPushingEnvironment returns an os environment to switch off hooks on push

routers/private/hook_post_receive.go

@@ -164,9 +164,11 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
 	}
 
 	// handle pull request merging, a pull request action should push at least 1 commit
-	handlePullRequestMerging(ctx, opts, ownerName, repoName, updates)
-	if ctx.Written() {
-		return
+	if opts.PushTrigger == repo_module.PushTriggerPRMergeToBase {
+		handlePullRequestMerging(ctx, opts, ownerName, repoName, updates)
+		if ctx.Written() {
+			return
+		}
 	}
 
 	isPrivate := opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate)
@@ -183,9 +185,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
 			wasEmpty = repo.IsEmpty
 		}
 
-		pusher, err := cache.GetWithContextCache(ctx, contextCachePusherKey, opts.UserID, func() (*user_model.User, error) {
-			return user_model.GetUserByID(ctx, opts.UserID)
-		})
+		pusher, err := loadContextCacheUser(ctx, opts.UserID)
 		if err != nil {
 			log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err)
 			ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
@@ -321,55 +321,55 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
 	})
 }
 
-const contextCachePusherKey = "hook_post_receive_pusher"
+func loadContextCacheUser(ctx context.Context, id int64) (*user_model.User, error) {
+	return cache.GetWithContextCache(ctx, "hook_post_receive_user", id, func() (*user_model.User, error) {
+		return user_model.GetUserByID(ctx, id)
+	})
+}
 
 // handlePullRequestMerging handle pull request merging, a pull request action should only push 1 commit
 func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.HookOptions, ownerName, repoName string, updates []*repo_module.PushUpdateOptions) {
-	if opts.PushTrigger != string(repo_module.PushTriggerPRMergeToBase) || len(updates) < 1 {
+	if len(updates) == 0 {
+		ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
+			Err: fmt.Sprintf("Pushing a merged PR (pr:%d) no commits pushed ", opts.PullRequestID),
+		})
 		return
 	}
+	if len(updates) != 1 && !setting.IsProd {
+		// it shouldn't happen
+		panic(fmt.Sprintf("Pushing a merged PR (pr:%d) should only push 1 commit, but got %d", opts.PullRequestID, len(updates)))
+	}
 
-	// Get the pull request
 	pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID)
 	if err != nil {
 		log.Error("GetPullRequestByID[%d]: %v", opts.PullRequestID, err)
-		ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
-			Err: fmt.Sprintf("GetPullRequestByID[%d]: %v", opts.PullRequestID, err),
-		})
+		ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{Err: "GetPullRequestByID failed"})
 		return
 	}
 
-	pusher, err := cache.GetWithContextCache(ctx, contextCachePusherKey, opts.UserID, func() (*user_model.User, error) {
-		return user_model.GetUserByID(ctx, opts.UserID)
-	})
+	pusher, err := loadContextCacheUser(ctx, opts.UserID)
 	if err != nil {
 		log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err)
-		ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
-			Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err),
-		})
+		ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{Err: "Load pusher user failed"})
 		return
 	}
 
 	pr.MergedCommitID = updates[len(updates)-1].NewCommitID
 	pr.MergedUnix = timeutil.TimeStampNow()
 	pr.Merger = pusher
-	pr.MergerID = opts.UserID
-
-	if err := db.WithTx(ctx, func(ctx context.Context) error {
+	pr.MergerID = pusher.ID
+	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 := pr.SetMerged(ctx); err != nil {
-			return fmt.Errorf("Failed to SetMerged: %s/%s Error: %v", ownerName, repoName, err)
+			return fmt.Errorf("SetMerged failed: %s/%s Error: %v", ownerName, repoName, err)
 		}
 		return nil
-	}); err != nil {
-		log.Error("%v", err)
-		ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
-			Err: err.Error(),
-		})
-		return
+	})
+	if 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"})
 	}
 }

routers/private/hook_post_receive_test.go

@@ -0,0 +1,37 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package private
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/db"
+	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/models/unittest"
+	"code.gitea.io/gitea/modules/private"
+	repo_module "code.gitea.io/gitea/modules/repository"
+	"code.gitea.io/gitea/services/contexttest"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestHandlePullRequestMerging(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	pr, err := issues_model.GetUnmergedPullRequest(db.DefaultContext, 1, 1, "branch2", "master", issues_model.PullRequestFlowGithub)
+	assert.NoError(t, err)
+	assert.NoError(t, pr.LoadBaseRepo(db.DefaultContext))
+	ctx, resp := contexttest.MockPrivateContext(t, "/")
+	handlePullRequestMerging(ctx, &private.HookOptions{
+		PullRequestID: pr.ID,
+		UserID:        2,
+	}, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, []*repo_module.PushUpdateOptions{
+		{NewCommitID: "01234567"},
+	})
+	assert.Equal(t, 0, len(resp.Body.String()))
+	pr, err = issues_model.GetPullRequestByID(db.DefaultContext, pr.ID)
+	assert.NoError(t, err)
+	assert.True(t, pr.HasMerged)
+	assert.EqualValues(t, "01234567", pr.MergedCommitID)
+	// TODO: test the removal of auto merge
+}

services/contexttest/context_tests.go

@@ -94,6 +94,19 @@ func MockAPIContext(t *testing.T, reqPath string) (*context.APIContext, *httptes
 	return ctx, resp
 }
 
+func MockPrivateContext(t *testing.T, reqPath string) (*context.PrivateContext, *httptest.ResponseRecorder) {
+	resp := httptest.NewRecorder()
+	req := mockRequest(t, reqPath)
+	base, baseCleanUp := context.NewBaseContext(resp, req)
+	base.Data = middleware.GetContextData(req.Context())
+	base.Locale = &translation.MockLocale{}
+	ctx := &context.PrivateContext{Base: base}
+	_ = baseCleanUp // during test, it doesn't need to do clean up. TODO: this can be improved later
+	chiCtx := chi.NewRouteContext()
+	ctx.Base.AppendContextValue(chi.RouteCtxKey, chiCtx)
+	return ctx, resp
+}
+
 // LoadRepo load a repo into a test context.
 func LoadRepo(t *testing.T, ctx gocontext.Context, repoID int64) {
 	var doer *user_model.User

services/pull/update.go

@@ -15,6 +15,7 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/repository"
 )
 
 // Update updates pull request with base branch.
@@ -72,7 +73,7 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.
 		BaseBranch: pr.HeadBranch,
 	}
 
-	_, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, "")
+	_, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, repository.PushTriggerPRUpdateWithBase)
 
 	defer func() {
 		go AddTestPullRequestTask(doer, reversePR.HeadRepo.ID, reversePR.HeadBranch, false, "", "")