Some improvements

Author: lunny

routers/private/hook_pre_receive.go

@@ -21,7 +21,9 @@ import (
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/private"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web"
+	"code.gitea.io/gitea/services/agit"
 	gitea_context "code.gitea.io/gitea/services/context"
 	pull_service "code.gitea.io/gitea/services/pull"
 )
@@ -452,40 +454,18 @@ func preReceiveFor(ctx *preReceiveContext, refFullName git.RefName) {
 		return
 	}
 
-	baseBranchName := refFullName.ForBranchName()
-
-	baseBranchExist, err := git_model.IsBranchExist(ctx, ctx.Repo.Repository.ID, baseBranchName)
+	_, _, err := agit.GetAgitBranchInfo(ctx, ctx.Repo.Repository.ID, refFullName.ForBranchName())
 	if err != nil {
-		ctx.JSON(http.StatusInternalServerError, private.Response{
-			Err: err.Error(),
-		})
-		return
-	}
-
-	if !baseBranchExist {
-		for p, v := range baseBranchName {
-			if v != '/' || p == len(baseBranchName)-1 {
-				continue
-			}
-			baseBranchExist, err = git_model.IsBranchExist(ctx, ctx.Repo.Repository.ID, baseBranchName[:p])
-			if err != nil {
-				ctx.JSON(http.StatusInternalServerError, private.Response{
-					Err: err.Error(),
-				})
-				return
-			}
-			if baseBranchExist {
-				break
-			}
+		if !errors.Is(err, util.ErrNotExist) {
+			ctx.JSON(http.StatusForbidden, private.Response{
+				UserMsg: fmt.Sprintf("Unexpected ref: %s", refFullName),
+			})
+		} else {
+			ctx.JSON(http.StatusInternalServerError, private.Response{
+				Err: err.Error(),
+			})
 		}
 	}
-
-	if !baseBranchExist {
-		ctx.JSON(http.StatusForbidden, private.Response{
-			UserMsg: fmt.Sprintf("Unexpected ref: %s", refFullName),
-		})
-		return
-	}
 }
 
 func generateGitEnv(opts *private.HookOptions) (env []string) {

services/agit/agit.go

@@ -6,6 +6,7 @@ package agit
 import (
 	"context"
 	"encoding/base64"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -32,6 +33,34 @@ func parseAgitPushOptionValue(s string) string {
 	return s
 }
 
+func GetAgitBranchInfo(ctx context.Context, repoID int64, baseBranchName string) (string, string, error) {
+	baseBranchExist, err := git_model.IsBranchExist(ctx, repoID, baseBranchName)
+	if err != nil {
+		return "", "", err
+	}
+	if baseBranchExist {
+		return baseBranchName, "", nil
+	}
+
+	// try match <target-branch>/<topic-branch>
+	// refs/for have been trimmed to get baseBranchName
+	for p, v := range baseBranchName {
+		if v != '/' {
+			continue
+		}
+
+		baseBranchExist, err := git_model.IsBranchExist(ctx, repoID, baseBranchName[:p])
+		if err != nil {
+			return "", "", err
+		}
+		if baseBranchExist {
+			return baseBranchName[:p], baseBranchName[p+1:], nil
+		}
+	}
+
+	return "", "", util.NewNotExistErrorf("base branch does not exist")
+}
+
 // ProcReceive handle proc receive work
 func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, opts *private.HookOptions) ([]private.HookProcReceiveRefResult, error) {
 	results := make([]private.HookProcReceiveRefResult, 0, len(opts.OldCommitIDs))
@@ -70,17 +99,19 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
 			continue
 		}
 
-		baseBranchName := opts.RefFullNames[i].ForBranchName()
-		currentTopicBranch := ""
-		if exist, _ := git_model.IsBranchExist(ctx, repo.ID, baseBranchName); !exist {
-			// try match refs/for/<target-branch>/<topic-branch>
-			for p, v := range baseBranchName {
-				if v == '/' && gitrepo.IsBranchExist(ctx, repo, baseBranchName[:p]) && p != len(baseBranchName)-1 {
-					currentTopicBranch = baseBranchName[p+1:]
-					baseBranchName = baseBranchName[:p]
-					break
-				}
+		baseBranchName, currentTopicBranch, err := GetAgitBranchInfo(ctx, repo.ID, opts.RefFullNames[i].ForBranchName())
+		if err != nil {
+			if !errors.Is(err, util.ErrNotExist) {
+				return nil, fmt.Errorf("failed to get branch information. Error: %w", err)
 			}
+			// If branch does not exist, we can continue
+			results = append(results, private.HookProcReceiveRefResult{
+				OriginalRef: opts.RefFullNames[i],
+				OldOID:      opts.OldCommitIDs[i],
+				NewOID:      opts.NewCommitIDs[i],
+				Err:         "base-branch does not exist",
+			})
+			continue
 		}
 
 		if len(topicBranch) == 0 && len(currentTopicBranch) == 0 {

services/agit/agit_test.go

@@ -6,11 +6,35 @@ package agit
 import (
 	"testing"
 
+	"code.gitea.io/gitea/models/unittest"
+	"code.gitea.io/gitea/modules/util"
+
 	"github.com/stretchr/testify/assert"
 )
 
+func TestMain(m *testing.M) {
+	unittest.MainTest(m)
+}
+
 func TestParseAgitPushOptionValue(t *testing.T) {
 	assert.Equal(t, "a", parseAgitPushOptionValue("a"))
 	assert.Equal(t, "a", parseAgitPushOptionValue("{base64}YQ=="))
 	assert.Equal(t, "{base64}invalid value", parseAgitPushOptionValue("{base64}invalid value"))
 }
+
+func TestGetAgitBranchInfo(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	_, _, err := GetAgitBranchInfo(t.Context(), 1, "non-exist-basebranch")
+	assert.ErrorIs(t, err, util.ErrNotExist)
+
+	baseBranch, currentTopicBranch, err := GetAgitBranchInfo(t.Context(), 1, "master")
+	assert.NoError(t, err)
+	assert.Equal(t, "master", baseBranch)
+	assert.Empty(t, currentTopicBranch)
+
+	baseBranch, currentTopicBranch, err = GetAgitBranchInfo(t.Context(), 1, "master/topicbranch")
+	assert.NoError(t, err)
+	assert.Equal(t, "master", baseBranch)
+	assert.Equal(t, "topicbranch", currentTopicBranch)
+}