From 263a716cb52037f3e7e51f014f6c8cdfad6ae03d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 9 Apr 2024 11:43:17 +0800 Subject: [PATCH] Performance optimization for git push (#30104) 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`. --- cmd/hook.go | 37 +++++++----- modules/private/hook.go | 25 ++++---- routers/private/hook_post_receive.go | 85 ++++++++++++++-------------- services/agit/agit.go | 26 ++++++--- tests/integration/git_push_test.go | 11 ++++ 5 files changed, 110 insertions(+), 74 deletions(-) diff --git a/cmd/hook.go b/cmd/hook.go index 6a3358853d..c04591d79e 100644 --- a/cmd/hook.go +++ b/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 { - 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() + hookPrintResult(res.Message, res.Create, res.Branch, res.URL) } } +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 } diff --git a/modules/private/hook.go b/modules/private/hook.go index cab8c81224..79c3d48229 100644 --- a/modules/private/hook.go +++ b/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 - NewOID string - Ref string - OriginalRef git.RefName - IsForcePush bool - IsNotMatched bool - Err string + OldOID string + NewOID string + Ref string + OriginalRef git.RefName + IsForcePush bool + IsNotMatched bool + Err string + IsCreatePR bool + URL string + ShouldShowMessage bool + HeadBranch string } // HookPreReceive check whether the provided commits are allowed diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 101ae92302..769a68970d 100644 --- a/routers/private/hook_post_receive.go +++ b/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) - repo.IsTemplate = opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate, repo.IsTemplate) - if err := repo_model.UpdateRepositoryCols(ctx, repo, "is_private", "is_template"); err != nil { + pusher, err := user_model.GetUserByID(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), }) + 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 diff --git a/services/agit/agit.go b/services/agit/agit.go index eb3bafa906..52a70469e0 100644 --- a/services/agit/agit.go +++ b/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(), - OriginalRef: opts.RefFullNames[i], - OldOID: objectFormat.EmptyObjectID().String(), - NewOID: opts.NewCommitIDs[i], + Ref: pr.GetGitRefName(), + OriginalRef: opts.RefFullNames[i], + OldOID: objectFormat.EmptyObjectID().String(), + 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, - NewOID: opts.NewCommitIDs[i], - Ref: pr.GetGitRefName(), - OriginalRef: opts.RefFullNames[i], - IsForcePush: isForcePush, + OldOID: oldCommitID, + NewOID: opts.NewCommitIDs[i], + 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(ctx), }) } diff --git a/tests/integration/git_push_test.go b/tests/integration/git_push_test.go index 0a35724807..b37fb02444 100644 --- a/tests/integration/git_push_test.go +++ b/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