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

Backport #30104 by @lunny

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`.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

cmd/hook.go

@@ -448,23 +448,26 @@ 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
-		}
-
-		fmt.Fprintln(os.Stderr, "")
-		if res.Create {
-			fmt.Fprintf(os.Stderr, "Create a new pull request for '%s':\n", res.Branch)
-			fmt.Fprintf(os.Stderr, "  %s\n", res.URL)
-		} else {
-			fmt.Fprint(os.Stderr, "Visit the existing pull request:\n")
-			fmt.Fprintf(os.Stderr, "  %s\n", res.URL)
-		}
-		fmt.Fprintln(os.Stderr, "")
-		os.Stderr.Sync()
 	}
 }
 
+func hookPrintResult(output, isCreate bool, branch, url string) {
+	if !output {
+		return
+	}
+	fmt.Fprintln(os.Stderr, "")
+	if isCreate {
+		fmt.Fprintf(os.Stderr, "Create a new pull request for '%s':\n", branch)
+		fmt.Fprintf(os.Stderr, "  %s\n", url)
+	} else {
+		fmt.Fprint(os.Stderr, "Visit the existing pull request:\n")
+		fmt.Fprintf(os.Stderr, "  %s\n", url)
+	}
+	fmt.Fprintln(os.Stderr, "")
+	os.Stderr.Sync()
+}
+
 func pushOptions() map[string]string {
 	opts := make(map[string]string)
 	if pushCount, err := strconv.Atoi(os.Getenv(private.GitPushOptionCount)); err == nil {
@@ -691,6 +694,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

@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/setting"
 )
 
@@ -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) optional.Option[bool] {
 	if val, ok := g[key]; ok {
 		if b, err := strconv.ParseBool(val); err == nil {
-			return b
+			return optional.Some(b)
 		}
 	}
-	return def
+	return optional.None[bool]()
 }
 
 // HookOptions represents the options for the Hook calls
@@ -87,13 +88,17 @@ type HookProcReceiveResult struct {
 
 // HookProcReceiveRefResult represents an individual result from ProcReceive
 type HookProcReceiveRefResult struct {
-	OldOID       string
+	OldOID            string
-	NewOID       string
+	NewOID            string
-	Ref          string
+	Ref               string
-	OriginalRef  git.RefName
+	OriginalRef       git.RefName
-	IsForcePush  bool
+	IsForcePush       bool
-	IsNotMatched bool
+	IsNotMatched      bool
-	Err          string
+	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,11 +6,12 @@ package private
 import (
 	"fmt"
 	"net/http"
-	"strconv"
 
 	git_model "code.gitea.io/gitea/models/git"
 	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"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/log"
@@ -159,8 +160,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.Has() || isTemplate.Has() {
 		// load the repository
 		if repo == nil {
 			repo = loadRepository(ctx, ownerName, repoName)
@@ -171,13 +174,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.Has() {
+			repo.IsPrivate = isPrivate.Value()
+			cols = append(cols, "is_private")
+		}
+
+		if isTemplate.Has() {
+			repo.IsTemplate = isTemplate.Value()
+			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
+			}
 		}
 	}
 
@@ -192,42 +231,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.DefaultFeatures.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(ctx),
-				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 !git.IsEmptyCommitID(newCommitID) && refFullName.IsBranch() {
 			// First ensure we have the repository loaded, we're allowed pulls requests and we can get the base repo

services/agit/agit.go

@@ -16,6 +16,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"
 )
@@ -145,10 +146,14 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
 			log.Trace("Pull request created: %d/%d", repo.ID, prIssue.ID)
 
 			results = append(results, private.HookProcReceiveRefResult{
-				Ref:         pr.GetGitRefName(),
+				Ref:               pr.GetGitRefName(),
-				OriginalRef: opts.RefFullNames[i],
+				OriginalRef:       opts.RefFullNames[i],
-				OldOID:      objectFormat.EmptyObjectID().String(),
+				OldOID:            objectFormat.EmptyObjectID().String(),
-				NewOID:      opts.NewCommitIDs[i],
+				NewOID:            opts.NewCommitIDs[i],
+				IsCreatePR:        true,
+				URL:               fmt.Sprintf("%s/pulls/%d", repo.HTMLURL(), pr.Index),
+				ShouldShowMessage: setting.Git.PullRequestPushMessage && repo.AllowsPulls(ctx),
+				HeadBranch:        headBranch,
 			})
 			continue
 		}
@@ -208,11 +213,14 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
 		isForcePush := comment != nil && comment.IsForcePush
 
 		results = append(results, private.HookProcReceiveRefResult{
-			OldOID:      oldCommitID,
+			OldOID:            oldCommitID,
-			NewOID:      opts.NewCommitIDs[i],
+			NewOID:            opts.NewCommitIDs[i],
-			Ref:         pr.GetGitRefName(),
+			Ref:               pr.GetGitRefName(),
-			OriginalRef: opts.RefFullNames[i],
+			OriginalRef:       opts.RefFullNames[i],
-			IsForcePush: isForcePush,
+			IsForcePush:       isForcePush,
+			IsCreatePR:        false,
+			URL:               fmt.Sprintf("%s/pulls/%d", repo.HTMLURL(), pr.Index),
+			ShouldShowMessage: setting.Git.PullRequestPushMessage && repo.AllowsPulls(ctx),
 		})
 	}
 

tests/integration/git_push_test.go

@@ -49,6 +49,17 @@ 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) (pushed, deleted []string) {
+			branchName := "branch-with-options"
+			doGitCreateBranch(gitPath, branchName)(t)
+			doGitPushTestRepository(gitPath, "origin", branchName, "-o", "repo.private=true", "-o", "repo.template=true")(t)
+			pushed = append(pushed, branchName)
+
+			return pushed, deleted
+		})
+	})
+
 	t.Run("Delete branches", func(t *testing.T) {
 		runTestGitPush(t, u, func(t *testing.T, gitPath string) (pushed, deleted []string) {
 			doGitPushTestRepository(gitPath, "origin", "master")(t) // make sure master is the default branch instead of a branch we are going to delete