Performance optimization for git push (#30104) (#30354)

Agit returned result should be from `ProcReceive` hook but not
`PostReceive` hook. Then for all non-agit pull requests, it will not
check the pull requests for every pushing `refs/pull/%d/head`.

Backport #30104

cmd/hook.go

@@ -446,21 +446,24 @@ Gitea or set your environment appropriately.`, "")
 
 func hookPrintResults(results []private.HookPostReceiveBranchResult) {
 	for _, res := range results {
-		if !res.Message {
+		hookPrintResult(res.Message, res.Create, res.Branch, res.URL)
-			continue
 	}
+}
 
+func hookPrintResult(output, isCreate bool, branch, url string) {
+	if !output {
+		return
+	}
 	fmt.Fprintln(os.Stderr, "")
-		if res.Create {
+	if isCreate {
-			fmt.Fprintf(os.Stderr, "Create a new pull request for '%s':\n", res.Branch)
+		fmt.Fprintf(os.Stderr, "Create a new pull request for '%s':\n", branch)
-			fmt.Fprintf(os.Stderr, "  %s\n", res.URL)
+		fmt.Fprintf(os.Stderr, "  %s\n", url)
 	} else {
 		fmt.Fprint(os.Stderr, "Visit the existing pull request:\n")
-			fmt.Fprintf(os.Stderr, "  %s\n", res.URL)
+		fmt.Fprintf(os.Stderr, "  %s\n", url)
 	}
 	fmt.Fprintln(os.Stderr, "")
 	os.Stderr.Sync()
-	}
 }
 
 func pushOptions() map[string]string {
@@ -688,6 +691,12 @@ Gitea or set your environment appropriately.`, "")
 	}
 	err = writeFlushPktLine(ctx, os.Stdout)
 
+	if err == nil {
+		for _, res := range resp.Results {
+			hookPrintResult(res.ShouldShowMessage, res.IsCreatePR, res.HeadBranch, res.URL)
+		}
+	}
+
 	return err
 }
 

modules/private/hook.go

@@ -12,6 +12,7 @@ import (
 
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 )
 
 // Git environment variables
@@ -32,13 +33,13 @@ const (
 )
 
 // Bool checks for a key in the map and parses as a boolean
-func (g GitPushOptions) Bool(key string, def bool) bool {
+func (g GitPushOptions) Bool(key string) util.OptionalBool {
 	if val, ok := g[key]; ok {
 		if b, err := strconv.ParseBool(val); err == nil {
-			return b
+			return util.OptionalBoolOf(b)
 		}
 	}
-	return def
+	return util.OptionalBoolNone
 }
 
 // HookOptions represents the options for the Hook calls
@@ -94,6 +95,10 @@ type HookProcReceiveRefResult struct {
 	IsForcePush       bool
 	IsNotMatched      bool
 	Err               string
+	IsCreatePR        bool
+	URL               string
+	ShouldShowMessage bool
+	HeadBranch        string
 }
 
 // HookPreReceive check whether the provided commits are allowed

routers/private/hook_post_receive.go

@@ -6,10 +6,11 @@ package private
 import (
 	"fmt"
 	"net/http"
-	"strconv"
 
 	issues_model "code.gitea.io/gitea/models/issues"
+	access_model "code.gitea.io/gitea/models/perm/access"
 	repo_model "code.gitea.io/gitea/models/repo"
+	user_model "code.gitea.io/gitea/models/user"
 	gitea_context "code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
@@ -89,8 +90,10 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
 		}
 	}
 
+	isPrivate := opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate)
+	isTemplate := opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate)
 	// Handle Push Options
-	if len(opts.GitPushOptions) > 0 {
+	if !isPrivate.IsNone() || !isTemplate.IsNone() {
 		// load the repository
 		if repo == nil {
 			repo = loadRepository(ctx, ownerName, repoName)
@@ -101,13 +104,49 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
 			wasEmpty = repo.IsEmpty
 		}
 
-		repo.IsPrivate = opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate, repo.IsPrivate)
+		pusher, err := user_model.GetUserByID(ctx, opts.UserID)
-		repo.IsTemplate = opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate, repo.IsTemplate)
+		if err != nil {
-		if err := repo_model.UpdateRepositoryCols(ctx, repo, "is_private", "is_template"); 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),
 			})
+			return
+		}
+		perm, err := access_model.GetUserRepoPermission(ctx, repo, pusher)
+		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),
+			})
+			return
+		}
+		if !perm.IsOwner() && !perm.IsAdmin() {
+			ctx.JSON(http.StatusNotFound, private.HookPostReceiveResult{
+				Err: "Permissions denied",
+			})
+			return
+		}
+
+		cols := make([]string, 0, len(opts.GitPushOptions))
+
+		if !isPrivate.IsNone() {
+			repo.IsPrivate = isPrivate.IsTrue()
+			cols = append(cols, "is_private")
+		}
+
+		if !isTemplate.IsNone() {
+			repo.IsTemplate = isTemplate.IsTrue()
+			cols = append(cols, "is_template")
+		}
+
+		if len(cols) > 0 {
+			if err := repo_model.UpdateRepositoryCols(ctx, repo, cols...); 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),
+				})
+				return
+			}
 		}
 	}
 
@@ -122,42 +161,6 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
 		refFullName := opts.RefFullNames[i]
 		newCommitID := opts.NewCommitIDs[i]
 
-		// post update for agit pull request
-		// FIXME: use pr.Flow to test whether it's an Agit PR or a GH PR
-		if git.SupportProcReceive && refFullName.IsPull() {
-			if repo == nil {
-				repo = loadRepository(ctx, ownerName, repoName)
-				if ctx.Written() {
-					return
-				}
-			}
-
-			pullIndex, _ := strconv.ParseInt(refFullName.PullName(), 10, 64)
-			if pullIndex <= 0 {
-				continue
-			}
-
-			pr, err := issues_model.GetPullRequestByIndex(ctx, repo.ID, pullIndex)
-			if err != nil && !issues_model.IsErrPullRequestNotExist(err) {
-				log.Error("Failed to get PR by index %v Error: %v", pullIndex, err)
-				ctx.JSON(http.StatusInternalServerError, private.Response{
-					Err: fmt.Sprintf("Failed to get PR by index %v Error: %v", pullIndex, err),
-				})
-				return
-			}
-			if pr == nil {
-				continue
-			}
-
-			results = append(results, private.HookPostReceiveBranchResult{
-				Message: setting.Git.PullRequestPushMessage && repo.AllowsPulls(),
-				Create:  false,
-				Branch:  "",
-				URL:     fmt.Sprintf("%s/pulls/%d", repo.HTMLURL(), pr.Index),
-			})
-			continue
-		}
-
 		// If we've pushed a branch (and not deleted it)
 		if newCommitID != git.EmptySHA && refFullName.IsBranch() {
 

services/agit/agit.go

@@ -15,6 +15,7 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/private"
+	"code.gitea.io/gitea/modules/setting"
 	notify_service "code.gitea.io/gitea/services/notify"
 	pull_service "code.gitea.io/gitea/services/pull"
 )
@@ -153,6 +154,10 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
 				OriginalRef:       opts.RefFullNames[i],
 				OldOID:            git.EmptySHA,
 				NewOID:            opts.NewCommitIDs[i],
+				IsCreatePR:        true,
+				URL:               fmt.Sprintf("%s/pulls/%d", repo.HTMLURL(), pr.Index),
+				ShouldShowMessage: setting.Git.PullRequestPushMessage && repo.AllowsPulls(),
+				HeadBranch:        headBranch,
 			})
 			continue
 		}
@@ -219,6 +224,9 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
 			Ref:               pr.GetGitRefName(),
 			OriginalRef:       opts.RefFullNames[i],
 			IsForcePush:       isForcePush,
+			IsCreatePR:        false,
+			URL:               fmt.Sprintf("%s/pulls/%d", repo.HTMLURL(), pr.Index),
+			ShouldShowMessage: setting.Git.PullRequestPushMessage && repo.AllowsPulls(),
 		})
 	}
 

tests/integration/git_push_test.go

@@ -0,0 +1,67 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"net/url"
+	"testing"
+
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/models/unittest"
+	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/git"
+	repo_service "code.gitea.io/gitea/services/repository"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestGitPush(t *testing.T) {
+	onGiteaRun(t, testGitPush)
+}
+
+func testGitPush(t *testing.T, u *url.URL) {
+	t.Run("Push branch with options", func(t *testing.T) {
+		runTestGitPush(t, u, func(t *testing.T, gitPath string) {
+			branchName := "branch-with-options"
+			doGitCreateBranch(gitPath, branchName)(t)
+			doGitPushTestRepository(gitPath, "origin", branchName, "-o", "repo.private=true", "-o", "repo.template=true")(t)
+		})
+	})
+}
+
+func runTestGitPush(t *testing.T, u *url.URL, gitOperation func(t *testing.T, gitPath string)) {
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	repo, err := repo_service.CreateRepository(db.DefaultContext, user, user, repo_service.CreateRepoOptions{
+		Name:          "repo-to-push",
+		Description:   "test git push",
+		AutoInit:      false,
+		DefaultBranch: "main",
+		IsPrivate:     false,
+	})
+	require.NoError(t, err)
+	require.NotEmpty(t, repo)
+
+	gitPath := t.TempDir()
+
+	doGitInitTestRepository(gitPath)(t)
+
+	oldPath := u.Path
+	oldUser := u.User
+	defer func() {
+		u.Path = oldPath
+		u.User = oldUser
+	}()
+	u.Path = repo.FullName() + ".git"
+	u.User = url.UserPassword(user.LowerName, userPassword)
+
+	doGitAddRemote(gitPath, "origin", u)(t)
+
+	gitRepo, err := git.OpenRepository(git.DefaultContext, gitPath)
+	require.NoError(t, err)
+	defer gitRepo.Close()
+
+	gitOperation(t, gitPath)
+
+	require.NoError(t, repo_service.DeleteRepositoryDirectly(db.DefaultContext, user, user.ID, repo.ID))
+}