Skip to content

Commit 6e3aaa9

Browse files
lunnylunny
authored
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
1 parent 3f6ddd9 commit 6e3aaa9

File tree

5 files changed

+165
-73
lines changed

5 files changed

+165
-73
lines changed

‎cmd/hook.go‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -446,21 +446,24 @@ Gitea or set your environment appropriately.`, "")
446446

447447
func hookPrintResults(results []private.HookPostReceiveBranchResult) {
448448
for _, res := range results {
449-
if !res.Message {
450-
continue
451-
}
449+
hookPrintResult(res.Message, res.Create, res.Branch, res.URL)
450+
}
451+
}
452452

453-
fmt.Fprintln(os.Stderr, "")
454-
if res.Create {
455-
fmt.Fprintf(os.Stderr, "Create a new pull request for '%s':\n", res.Branch)
456-
fmt.Fprintf(os.Stderr, " %s\n", res.URL)
457-
} else {
458-
fmt.Fprint(os.Stderr, "Visit the existing pull request:\n")
459-
fmt.Fprintf(os.Stderr, " %s\n", res.URL)
460-
}
461-
fmt.Fprintln(os.Stderr, "")
462-
os.Stderr.Sync()
453+
func hookPrintResult(output, isCreate bool, branch, url string) {
454+
if !output {
455+
return
456+
}
457+
fmt.Fprintln(os.Stderr, "")
458+
if isCreate {
459+
fmt.Fprintf(os.Stderr, "Create a new pull request for '%s':\n", branch)
460+
fmt.Fprintf(os.Stderr, " %s\n", url)
461+
} else {
462+
fmt.Fprint(os.Stderr, "Visit the existing pull request:\n")
463+
fmt.Fprintf(os.Stderr, " %s\n", url)
463464
}
465+
fmt.Fprintln(os.Stderr, "")
466+
os.Stderr.Sync()
464467
}
465468

466469
func pushOptions() map[string]string {
@@ -688,6 +691,12 @@ Gitea or set your environment appropriately.`, "")
688691
}
689692
err = writeFlushPktLine(ctx, os.Stdout)
690693

694+
if err == nil {
695+
for _, res := range resp.Results {
696+
hookPrintResult(res.ShouldShowMessage, res.IsCreatePR, res.HeadBranch, res.URL)
697+
}
698+
}
699+
691700
return err
692701
}
693702

‎modules/private/hook.go‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212

1313
"code.gitea.io/gitea/modules/git"
1414
"code.gitea.io/gitea/modules/setting"
15+
"code.gitea.io/gitea/modules/util"
1516
)
1617

1718
// Git environment variables
@@ -32,13 +33,13 @@ const (
3233
)
3334

3435
// Bool checks for a key in the map and parses as a boolean
35-
func (g GitPushOptions) Bool(key string, def bool) bool {
36+
func (g GitPushOptions) Bool(key string) util.OptionalBool {
3637
if val, ok := g[key]; ok {
3738
if b, err := strconv.ParseBool(val); err == nil {
38-
return b
39+
return util.OptionalBoolOf(b)
3940
}
4041
}
41-
return def
42+
return util.OptionalBoolNone
4243
}
4344

4445
// HookOptions represents the options for the Hook calls
@@ -87,13 +88,17 @@ type HookProcReceiveResult struct {
8788

8889
// HookProcReceiveRefResult represents an individual result from ProcReceive
8990
type HookProcReceiveRefResult struct {
90-
OldOID string
91-
NewOID string
92-
Ref string
93-
OriginalRef git.RefName
94-
IsForcePush bool
95-
IsNotMatched bool
96-
Err string
91+
OldOID string
92+
NewOID string
93+
Ref string
94+
OriginalRef git.RefName
95+
IsForcePush bool
96+
IsNotMatched bool
97+
Err string
98+
IsCreatePR bool
99+
URL string
100+
ShouldShowMessage bool
101+
HeadBranch string
97102
}
98103

99104
// HookPreReceive check whether the provided commits are allowed

‎routers/private/hook_post_receive.go‎

Lines changed: 44 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,11 @@ package private
66
import (
77
"fmt"
88
"net/http"
9-
"strconv"
109

1110
issues_model "code.gitea.io/gitea/models/issues"
11+
access_model "code.gitea.io/gitea/models/perm/access"
1212
repo_model "code.gitea.io/gitea/models/repo"
13+
user_model "code.gitea.io/gitea/models/user"
1314
gitea_context "code.gitea.io/gitea/modules/context"
1415
"code.gitea.io/gitea/modules/git"
1516
"code.gitea.io/gitea/modules/log"
@@ -89,8 +90,10 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
8990
}
9091
}
9192

93+
isPrivate := opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate)
94+
isTemplate := opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate)
9295
// Handle Push Options
93-
if len(opts.GitPushOptions) > 0 {
96+
if !isPrivate.IsNone() || !isTemplate.IsNone() {
9497
// load the repository
9598
if repo == nil {
9699
repo = loadRepository(ctx, ownerName, repoName)
@@ -101,13 +104,49 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
101104
wasEmpty = repo.IsEmpty
102105
}
103106

104-
repo.IsPrivate = opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate, repo.IsPrivate)
105-
repo.IsTemplate = opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate, repo.IsTemplate)
106-
if err := repo_model.UpdateRepositoryCols(ctx, repo, "is_private", "is_template"); err != nil {
107+
pusher, err := user_model.GetUserByID(ctx, opts.UserID)
108+
if err != nil {
107109
log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err)
108110
ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
109111
Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err),
110112
})
113+
return
114+
}
115+
perm, err := access_model.GetUserRepoPermission(ctx, repo, pusher)
116+
if err != nil {
117+
log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err)
118+
ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
119+
Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err),
120+
})
121+
return
122+
}
123+
if !perm.IsOwner() && !perm.IsAdmin() {
124+
ctx.JSON(http.StatusNotFound, private.HookPostReceiveResult{
125+
Err: "Permissions denied",
126+
})
127+
return
128+
}
129+
130+
cols := make([]string, 0, len(opts.GitPushOptions))
131+
132+
if !isPrivate.IsNone() {
133+
repo.IsPrivate = isPrivate.IsTrue()
134+
cols = append(cols, "is_private")
135+
}
136+
137+
if !isTemplate.IsNone() {
138+
repo.IsTemplate = isTemplate.IsTrue()
139+
cols = append(cols, "is_template")
140+
}
141+
142+
if len(cols) > 0 {
143+
if err := repo_model.UpdateRepositoryCols(ctx, repo, cols...); err != nil {
144+
log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err)
145+
ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
146+
Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err),
147+
})
148+
return
149+
}
111150
}
112151
}
113152

@@ -122,42 +161,6 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
122161
refFullName := opts.RefFullNames[i]
123162
newCommitID := opts.NewCommitIDs[i]
124163

125-
// post update for agit pull request
126-
// FIXME: use pr.Flow to test whether it's an Agit PR or a GH PR
127-
if git.SupportProcReceive && refFullName.IsPull() {
128-
if repo == nil {
129-
repo = loadRepository(ctx, ownerName, repoName)
130-
if ctx.Written() {
131-
return
132-
}
133-
}
134-
135-
pullIndex, _ := strconv.ParseInt(refFullName.PullName(), 10, 64)
136-
if pullIndex <= 0 {
137-
continue
138-
}
139-
140-
pr, err := issues_model.GetPullRequestByIndex(ctx, repo.ID, pullIndex)
141-
if err != nil && !issues_model.IsErrPullRequestNotExist(err) {
142-
log.Error("Failed to get PR by index %v Error: %v", pullIndex, err)
143-
ctx.JSON(http.StatusInternalServerError, private.Response{
144-
Err: fmt.Sprintf("Failed to get PR by index %v Error: %v", pullIndex, err),
145-
})
146-
return
147-
}
148-
if pr == nil {
149-
continue
150-
}
151-
152-
results = append(results, private.HookPostReceiveBranchResult{
153-
Message: setting.Git.PullRequestPushMessage && repo.AllowsPulls(),
154-
Create: false,
155-
Branch: "",
156-
URL: fmt.Sprintf("%s/pulls/%d", repo.HTMLURL(), pr.Index),
157-
})
158-
continue
159-
}
160-
161164
// If we've pushed a branch (and not deleted it)
162165
if newCommitID != git.EmptySHA && refFullName.IsBranch() {
163166

‎services/agit/agit.go‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"code.gitea.io/gitea/modules/git"
1616
"code.gitea.io/gitea/modules/log"
1717
"code.gitea.io/gitea/modules/private"
18+
"code.gitea.io/gitea/modules/setting"
1819
notify_service "code.gitea.io/gitea/services/notify"
1920
pull_service "code.gitea.io/gitea/services/pull"
2021
)
@@ -149,10 +150,14 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
149150
log.Trace("Pull request created: %d/%d", repo.ID, prIssue.ID)
150151

151152
results = append(results, private.HookProcReceiveRefResult{
152-
Ref: pr.GetGitRefName(),
153-
OriginalRef: opts.RefFullNames[i],
154-
OldOID: git.EmptySHA,
155-
NewOID: opts.NewCommitIDs[i],
153+
Ref: pr.GetGitRefName(),
154+
OriginalRef: opts.RefFullNames[i],
155+
OldOID: git.EmptySHA,
156+
NewOID: opts.NewCommitIDs[i],
157+
IsCreatePR: true,
158+
URL: fmt.Sprintf("%s/pulls/%d", repo.HTMLURL(), pr.Index),
159+
ShouldShowMessage: setting.Git.PullRequestPushMessage && repo.AllowsPulls(),
160+
HeadBranch: headBranch,
156161
})
157162
continue
158163
}
@@ -214,11 +219,14 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
214219
isForcePush := comment != nil && comment.IsForcePush
215220

216221
results = append(results, private.HookProcReceiveRefResult{
217-
OldOID: oldCommitID,
218-
NewOID: opts.NewCommitIDs[i],
219-
Ref: pr.GetGitRefName(),
220-
OriginalRef: opts.RefFullNames[i],
221-
IsForcePush: isForcePush,
222+
OldOID: oldCommitID,
223+
NewOID: opts.NewCommitIDs[i],
224+
Ref: pr.GetGitRefName(),
225+
OriginalRef: opts.RefFullNames[i],
226+
IsForcePush: isForcePush,
227+
IsCreatePR: false,
228+
URL: fmt.Sprintf("%s/pulls/%d", repo.HTMLURL(), pr.Index),
229+
ShouldShowMessage: setting.Git.PullRequestPushMessage && repo.AllowsPulls(),
222230
})
223231
}
224232

‎tests/integration/git_push_test.go‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
// Copyright 2024 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package integration
5+
6+
import (
7+
"net/url"
8+
"testing"
9+
10+
"code.gitea.io/gitea/models/db"
11+
"code.gitea.io/gitea/models/unittest"
12+
user_model "code.gitea.io/gitea/models/user"
13+
"code.gitea.io/gitea/modules/git"
14+
repo_service "code.gitea.io/gitea/services/repository"
15+
16+
"github.com/stretchr/testify/require"
17+
)
18+
19+
func TestGitPush(t *testing.T) {
20+
onGiteaRun(t, testGitPush)
21+
}
22+
23+
func testGitPush(t *testing.T, u *url.URL) {
24+
t.Run("Push branch with options", func(t *testing.T) {
25+
runTestGitPush(t, u, func(t *testing.T, gitPath string) {
26+
branchName := "branch-with-options"
27+
doGitCreateBranch(gitPath, branchName)(t)
28+
doGitPushTestRepository(gitPath, "origin", branchName, "-o", "repo.private=true", "-o", "repo.template=true")(t)
29+
})
30+
})
31+
}
32+
33+
func runTestGitPush(t *testing.T, u *url.URL, gitOperation func(t *testing.T, gitPath string)) {
34+
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
35+
repo, err := repo_service.CreateRepository(db.DefaultContext, user, user, repo_service.CreateRepoOptions{
36+
Name: "repo-to-push",
37+
Description: "test git push",
38+
AutoInit: false,
39+
DefaultBranch: "main",
40+
IsPrivate: false,
41+
})
42+
require.NoError(t, err)
43+
require.NotEmpty(t, repo)
44+
45+
gitPath := t.TempDir()
46+
47+
doGitInitTestRepository(gitPath)(t)
48+
49+
oldPath := u.Path
50+
oldUser := u.User
51+
defer func() {
52+
u.Path = oldPath
53+
u.User = oldUser
54+
}()
55+
u.Path = repo.FullName() + ".git"
56+
u.User = url.UserPassword(user.LowerName, userPassword)
57+
58+
doGitAddRemote(gitPath, "origin", u)(t)
59+
60+
gitRepo, err := git.OpenRepository(git.DefaultContext, gitPath)
61+
require.NoError(t, err)
62+
defer gitRepo.Close()
63+
64+
gitOperation(t, gitPath)
65+
66+
require.NoError(t, repo_service.DeleteRepositoryDirectly(db.DefaultContext, user, user.ID, repo.ID))
67+
}

0 commit comments

Comments
 (0)