From 6e3aaa997549b83935241e486caf811793c88aea Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 10 Apr 2024 14:12:19 +0800 Subject: [PATCH] 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 | 37 +++++++----- modules/private/hook.go | 25 ++++---- routers/private/hook_post_receive.go | 85 ++++++++++++++-------------- services/agit/agit.go | 26 ++++++--- tests/integration/git_push_test.go | 67 ++++++++++++++++++++++ 5 files changed, 166 insertions(+), 74 deletions(-) create mode 100644 tests/integration/git_push_test.go diff --git a/cmd/hook.go b/cmd/hook.go index f38fd8831b..6c5a09b2e7 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -446,23 +446,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 { @@ -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 } diff --git a/modules/private/hook.go b/modules/private/hook.go index 23e03896e4..3dc78019cd 100644 --- a/modules/private/hook.go +++ b/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 @@ -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 0c9a2a10fa..1233d3db27 100644 --- a/routers/private/hook_post_receive.go +++ b/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) - 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.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() { diff --git a/services/agit/agit.go b/services/agit/agit.go index a39034b025..a7c97be7f4 100644 --- a/services/agit/agit.go +++ b/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" ) @@ -149,10 +150,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: git.EmptySHA, - NewOID: opts.NewCommitIDs[i], + Ref: pr.GetGitRefName(), + 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 } @@ -214,11 +219,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(), }) } diff --git a/tests/integration/git_push_test.go b/tests/integration/git_push_test.go new file mode 100644 index 0000000000..a64c2953c5 --- /dev/null +++ b/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)) +}