From 6bef77313c78dd0e47d741bfeb3724de07feffd7 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Thu, 29 Feb 2024 18:26:06 +0800 Subject: [PATCH 01/11] feat: sync to db in hook --- routers/private/hook_post_receive.go | 45 +++++++++++++++++++++++++++- services/repository/branch.go | 4 +-- services/repository/push.go | 4 --- 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 4eafe3923dfb..cba91ef85495 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -11,6 +11,7 @@ import ( issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/private" repo_module "code.gitea.io/gitea/modules/repository" @@ -27,6 +28,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { // We don't rely on RepoAssignment here because: // a) we don't need the git repo in this function + // OUT OF DATE: we do need the git repo to sync the branch to the db now. // b) our update function will likely change the repository in the db so we will need to refresh it // c) we don't always need the repo @@ -34,7 +36,11 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { repoName := ctx.Params(":repo") // defer getting the repository at this point - as we should only retrieve it if we're going to call update - var repo *repo_model.Repository + var ( + repo *repo_model.Repository + gitRepo *git.Repository + ) + defer gitRepo.Close() // it's safe to call Close on a nil pointer updates := make([]*repo_module.PushUpdateOptions, 0, len(opts.OldCommitIDs)) wasEmpty := false @@ -87,6 +93,43 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { }) return } + + for _, update := range updates { + if !update.RefFullName.IsBranch() { + continue + } + if repo == nil { + repo = loadRepository(ctx, ownerName, repoName) + if ctx.Written() { + return + } + wasEmpty = repo.IsEmpty + } + if gitRepo == nil { + var err error + gitRepo, err = gitrepo.OpenRepository(ctx, repo) + if err != nil { + log.Error("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err), + }) + return + } + } + commit, err := gitRepo.GetCommit(update.NewCommitID) + if err != nil { + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to get commit %s in repository: %s/%s Error: %v", update.NewCommitID, ownerName, repoName, err), + }) + return + } + if err := repo_service.SyncBranchToDB(ctx, repo.ID, update.PusherID, update.RefFullName.BranchName(), commit); err != nil { + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to sync branch to DB in repository: %s/%s Error: %v", ownerName, repoName, err), + }) + return + } + } } // Handle Push Options diff --git a/services/repository/branch.go b/services/repository/branch.go index ec41173da8c4..6c39ef01219c 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -222,12 +222,12 @@ func checkBranchName(ctx context.Context, repo *repo_model.Repository, name stri return err } -// syncBranchToDB sync the branch information in the database. It will try to update the branch first, +// SyncBranchToDB sync the branch information in the database. It will try to update the branch first, // if updated success with affect records > 0, then all are done. Because that means the branch has been in the database. // If no record is affected, that means the branch does not exist in database. So there are two possibilities. // One is this is a new branch, then we just need to insert the record. Another is the branches haven't been synced, // then we need to sync all the branches into database. -func syncBranchToDB(ctx context.Context, repoID, pusherID int64, branchName string, commit *git.Commit) error { +func SyncBranchToDB(ctx context.Context, repoID, pusherID int64, branchName string, commit *git.Commit) error { cnt, err := git_model.UpdateBranch(ctx, repoID, pusherID, branchName, commit) if err != nil { return fmt.Errorf("git_model.UpdateBranch %d:%s failed: %v", repoID, branchName, err) diff --git a/services/repository/push.go b/services/repository/push.go index 9aaf0e1c9bca..577e54d087e9 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -259,10 +259,6 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { commits.Commits = commits.Commits[:setting.UI.FeedMaxCommitNum] } - if err = syncBranchToDB(ctx, repo.ID, opts.PusherID, branch, newCommit); err != nil { - return fmt.Errorf("git_model.UpdateBranch %s:%s failed: %v", repo.FullName(), branch, err) - } - notify_service.PushCommits(ctx, pusher, repo, opts, commits) // Cache for big repository From 3d6882822f73c15191c6c7b970da272e0281c36b Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 1 Mar 2024 09:57:06 +0800 Subject: [PATCH 02/11] Update routers/private/hook_post_receive.go Co-authored-by: Lunny Xiao --- routers/private/hook_post_receive.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index cba91ef85495..0e0e16584ae2 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -116,12 +116,15 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { return } } - commit, err := gitRepo.GetCommit(update.NewCommitID) - if err != nil { - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + var commit *git.Commit + if !opts.IsDelRef() { + commit, err := gitRepo.GetCommit(update.NewCommitID) + if err != nil { + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ Err: fmt.Sprintf("Failed to get commit %s in repository: %s/%s Error: %v", update.NewCommitID, ownerName, repoName, err), - }) - return + }) + return + } } if err := repo_service.SyncBranchToDB(ctx, repo.ID, update.PusherID, update.RefFullName.BranchName(), commit); err != nil { ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ From 4fb19f7376c648bcbf1587e36433c68badb231ea Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 1 Mar 2024 10:00:42 +0800 Subject: [PATCH 03/11] fix: ignore deleted --- routers/private/hook_post_receive.go | 40 ++++++++++++++-------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 0e0e16584ae2..80e26218e116 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -105,33 +105,33 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } wasEmpty = repo.IsEmpty } - if gitRepo == nil { - var err error - gitRepo, err = gitrepo.OpenRepository(ctx, repo) + + if !update.IsDelRef() { + if gitRepo == nil { + var err error + gitRepo, err = gitrepo.OpenRepository(ctx, repo) + if err != nil { + log.Error("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err), + }) + return + } + } + commit, err := gitRepo.GetCommit(update.NewCommitID) if err != nil { - log.Error("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err) ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err), + Err: fmt.Sprintf("Failed to get commit %s in repository: %s/%s Error: %v", update.NewCommitID, ownerName, repoName, err), }) return } - } - var commit *git.Commit - if !opts.IsDelRef() { - commit, err := gitRepo.GetCommit(update.NewCommitID) - if err != nil { - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to get commit %s in repository: %s/%s Error: %v", update.NewCommitID, ownerName, repoName, err), - }) - return + if err := repo_service.SyncBranchToDB(ctx, repo.ID, update.PusherID, update.RefFullName.BranchName(), commit); err != nil { + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to sync branch to DB in repository: %s/%s Error: %v", ownerName, repoName, err), + }) + return } } - if err := repo_service.SyncBranchToDB(ctx, repo.ID, update.PusherID, update.RefFullName.BranchName(), commit); err != nil { - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to sync branch to DB in repository: %s/%s Error: %v", ownerName, repoName, err), - }) - return - } } } From 62d3cfae1d9f33ee19005481eabbcf9590f305b0 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 1 Mar 2024 14:10:06 +0800 Subject: [PATCH 04/11] test: debug log --- routers/private/hook_post_receive.go | 9 +++++++++ routers/private/hook_pre_receive.go | 4 ++++ 2 files changed, 13 insertions(+) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 80e26218e116..4f7335dca2d4 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "strconv" + "time" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" @@ -24,6 +25,9 @@ import ( // HookPostReceive updates services and users func HookPostReceive(ctx *gitea_context.PrivateContext) { + // TODO: remove debug code + log.Info("Debug git hook, HookPostReceive: %v", time.Now().Format(time.RFC3339Nano)) + opts := web.GetForm(ctx).(*private.HookOptions) // We don't rely on RepoAssignment here because: @@ -93,6 +97,8 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { }) return } + // TODO: remove debug code + log.Info("Debug git hook, before sync %v branches: %v", len(updates), time.Now().Format(time.RFC3339Nano)) for _, update := range updates { if !update.RefFullName.IsBranch() { @@ -133,6 +139,9 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } } } + + // TODO: remove debug code + log.Info("Debug git hook, after sync %v branches: %v", len(updates), time.Now().Format(time.RFC3339Nano)) } // Handle Push Options diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 32ec3003e265..0b0985643cd8 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "os" + "time" "code.gitea.io/gitea/models" asymkey_model "code.gitea.io/gitea/models/asymkey" @@ -103,6 +104,9 @@ func (ctx *preReceiveContext) AssertCreatePullRequest() bool { // HookPreReceive checks whether a individual commit is acceptable func HookPreReceive(ctx *gitea_context.PrivateContext) { + // TODO: remove debug code + log.Info("Debug git hook, HookPreReceive: %v", time.Now().Format(time.RFC3339Nano)) + opts := web.GetForm(ctx).(*private.HookOptions) ourCtx := &preReceiveContext{ From a844be575a3f674578cbb8dab51bb65cb7c1b16d Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 1 Mar 2024 15:04:25 +0800 Subject: [PATCH 05/11] Revert "test: debug log" This reverts commit 62d3cfae1d9f33ee19005481eabbcf9590f305b0. --- routers/private/hook_post_receive.go | 9 --------- routers/private/hook_pre_receive.go | 4 ---- 2 files changed, 13 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 4f7335dca2d4..80e26218e116 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -7,7 +7,6 @@ import ( "fmt" "net/http" "strconv" - "time" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" @@ -25,9 +24,6 @@ import ( // HookPostReceive updates services and users func HookPostReceive(ctx *gitea_context.PrivateContext) { - // TODO: remove debug code - log.Info("Debug git hook, HookPostReceive: %v", time.Now().Format(time.RFC3339Nano)) - opts := web.GetForm(ctx).(*private.HookOptions) // We don't rely on RepoAssignment here because: @@ -97,8 +93,6 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { }) return } - // TODO: remove debug code - log.Info("Debug git hook, before sync %v branches: %v", len(updates), time.Now().Format(time.RFC3339Nano)) for _, update := range updates { if !update.RefFullName.IsBranch() { @@ -139,9 +133,6 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } } } - - // TODO: remove debug code - log.Info("Debug git hook, after sync %v branches: %v", len(updates), time.Now().Format(time.RFC3339Nano)) } // Handle Push Options diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 0b0985643cd8..32ec3003e265 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -7,7 +7,6 @@ import ( "fmt" "net/http" "os" - "time" "code.gitea.io/gitea/models" asymkey_model "code.gitea.io/gitea/models/asymkey" @@ -104,9 +103,6 @@ func (ctx *preReceiveContext) AssertCreatePullRequest() bool { // HookPreReceive checks whether a individual commit is acceptable func HookPreReceive(ctx *gitea_context.PrivateContext) { - // TODO: remove debug code - log.Info("Debug git hook, HookPreReceive: %v", time.Now().Format(time.RFC3339Nano)) - opts := web.GetForm(ctx).(*private.HookOptions) ourCtx := &preReceiveContext{ From 75950d0743ef08344922c4a55be7ecab5c475e77 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 1 Mar 2024 15:21:20 +0800 Subject: [PATCH 06/11] feat: add deleted branches --- routers/private/hook_post_receive.go | 11 ++++++++++- services/repository/push.go | 5 ----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 80e26218e116..8ebc7306b401 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -8,6 +8,7 @@ import ( "net/http" "strconv" + git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/git" @@ -106,7 +107,15 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { wasEmpty = repo.IsEmpty } - if !update.IsDelRef() { + if update.IsDelRef() { + if err := git_model.AddDeletedBranch(ctx, repo.ID, update.RefFullName.BranchName(), update.PusherID); err != nil { + log.Error("Failed to add deleted branch: %s/%s Error: %v", ownerName, repoName, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to add deleted branch: %s/%s Error: %v", ownerName, repoName, err), + }) + return + } + } else { if gitRepo == nil { var err error gitRepo, err = gitrepo.OpenRepository(ctx, repo) diff --git a/services/repository/push.go b/services/repository/push.go index 577e54d087e9..89a3127902a5 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -11,7 +11,6 @@ import ( "time" "code.gitea.io/gitea/models/db" - git_model "code.gitea.io/gitea/models/git" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/cache" @@ -271,10 +270,6 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { // close all related pulls log.Error("close related pull request failed: %v", err) } - - if err := git_model.AddDeletedBranch(ctx, repo.ID, branch, pusher.ID); err != nil { - return fmt.Errorf("AddDeletedBranch %s:%s failed: %v", repo.FullName(), branch, err) - } } // Even if user delete a branch on a repository which he didn't watch, he will be watch that. From 49f224a10378e2ce8377f0c4395d6e8368e61c67 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 1 Mar 2024 17:56:21 +0800 Subject: [PATCH 07/11] fix: improve performance of SyncBranchesToDB --- models/git/branch.go | 5 ++ routers/private/hook_post_receive.go | 47 ++++++----- services/repository/branch.go | 112 +++++++++++++++++++-------- 3 files changed, 114 insertions(+), 50 deletions(-) diff --git a/models/git/branch.go b/models/git/branch.go index db02fc9b28bb..fa0781fed1d5 100644 --- a/models/git/branch.go +++ b/models/git/branch.go @@ -158,6 +158,11 @@ func GetBranch(ctx context.Context, repoID int64, branchName string) (*Branch, e return &branch, nil } +func GetBranches(ctx context.Context, repoID int64, branchNames []string) ([]*Branch, error) { + branches := make([]*Branch, 0, len(branchNames)) + return branches, db.GetEngine(ctx).Where("repo_id=?", repoID).In("name", branchNames).Find(&branches) +} + func AddBranches(ctx context.Context, branches []*Branch) error { for _, branch := range branches { if _, err := db.GetEngine(ctx).Insert(branch); err != nil { diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 8ebc7306b401..4043448e525a 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -95,6 +95,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { return } + var branchesToSync = make([]*repo_module.PushUpdateOptions, 0, len(updates)) for _, update := range updates { if !update.RefFullName.IsBranch() { continue @@ -116,31 +117,39 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { return } } else { - if gitRepo == nil { - var err error - gitRepo, err = gitrepo.OpenRepository(ctx, repo) - if err != nil { - log.Error("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err), - }) - return - } - } - commit, err := gitRepo.GetCommit(update.NewCommitID) + branchesToSync = append(branchesToSync, update) + } + } + if len(branchesToSync) > 0 { + if gitRepo == nil { + var err error + gitRepo, err = gitrepo.OpenRepository(ctx, repo) if err != nil { + log.Error("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err) ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to get commit %s in repository: %s/%s Error: %v", update.NewCommitID, ownerName, repoName, err), - }) - return - } - if err := repo_service.SyncBranchToDB(ctx, repo.ID, update.PusherID, update.RefFullName.BranchName(), commit); err != nil { - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to sync branch to DB in repository: %s/%s Error: %v", ownerName, repoName, err), + Err: fmt.Sprintf("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err), }) return } } + + var ( + branchNames = make([]string, 0, len(branchesToSync)) + commitIDs = make([]string, 0, len(branchesToSync)) + ) + for _, update := range branchesToSync { + branchNames = append(branchNames, update.RefFullName.BranchName()) + commitIDs = append(commitIDs, update.NewCommitID) + } + + if err := repo_service.SyncBranchesToDB(ctx, repo.ID, opts.UserID, branchNames, commitIDs, func(commitID string) (*git.Commit, error) { + return gitRepo.GetCommit(commitID) + }); err != nil { + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to sync branch to DB in repository: %s/%s Error: %v", ownerName, repoName, err), + }) + return + } } } diff --git a/services/repository/branch.go b/services/repository/branch.go index 6c39ef01219c..51eed062e543 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -222,44 +222,94 @@ func checkBranchName(ctx context.Context, repo *repo_model.Repository, name stri return err } -// SyncBranchToDB sync the branch information in the database. It will try to update the branch first, +// SyncBranchesToDB sync the branch information in the database. It will try to update the branch first, // if updated success with affect records > 0, then all are done. Because that means the branch has been in the database. // If no record is affected, that means the branch does not exist in database. So there are two possibilities. // One is this is a new branch, then we just need to insert the record. Another is the branches haven't been synced, // then we need to sync all the branches into database. -func SyncBranchToDB(ctx context.Context, repoID, pusherID int64, branchName string, commit *git.Commit) error { - cnt, err := git_model.UpdateBranch(ctx, repoID, pusherID, branchName, commit) - if err != nil { - return fmt.Errorf("git_model.UpdateBranch %d:%s failed: %v", repoID, branchName, err) - } - if cnt > 0 { // This means branch does exist, so it's a normal update. It also means the branch has been synced. - return nil - } +func SyncBranchesToDB(ctx context.Context, repoID, pusherID int64, branchNames []string, commitIDs []string, getCommit func(commitID string) (*git.Commit, error)) error { + // Some designs that make the code look strange but are made for performance optimization purposes: + // 1. Sync branches in a batch to reduce the number of DB queries. + // 2. Lazy load commit information since it may be not necessary. + // 3. Exit early if synced all branches of git repo when there's no branch in DB. + // 4. Check the branches in DB if they are already synced. + // + // If the user pushes many branches at once, the Git hook will call the internal API in batches, rather than all at once. + // See https://github.com/go-gitea/gitea/blob/cb52b17f92e2d2293f7c003649743464492bca48/cmd/hook.go#L27 + // For the first batch, it will hit optimization 3. + // For other batches, it will hit optimization 4. + + if len(branchNames) != len(commitIDs) { + return fmt.Errorf("branchNames and commitIDs length not match") + } + + return db.WithTx(ctx, func(ctx context.Context) error { + branches, err := git_model.GetBranches(ctx, repoID, branchNames) + if err != nil { + return fmt.Errorf("git_model.GetBranches: %v", err) + } + branchMap := make(map[string]*git_model.Branch, len(branches)) + for _, branch := range branches { + branchMap[branch.Name] = branch + } - // if user haven't visit UI but directly push to a branch after upgrading from 1.20 -> 1.21, - // we cannot simply insert the branch but need to check we have branches or not - hasBranch, err := db.Exist[git_model.Branch](ctx, git_model.FindBranchOptions{ - RepoID: repoID, - IsDeletedBranch: optional.Some(false), - }.ToConds()) - if err != nil { - return err - } - if !hasBranch { - if _, err = repo_module.SyncRepoBranches(ctx, repoID, pusherID); err != nil { - return fmt.Errorf("repo_module.SyncRepoBranches %d:%s failed: %v", repoID, branchName, err) + hasBranch := len(branches) > 0 + + newBranches := make([]*git_model.Branch, 0, len(branchNames)) + + for i, branchName := range branchNames { + commitID := commitIDs[i] + if branch, ok := branchMap[branchName]; ok && branch.CommitID == commitID { + continue + } + + commit, err := getCommit(branchName) + if err != nil { + return fmt.Errorf("get commit of %s failed: %v", branchName, err) + } + + cnt, err := git_model.UpdateBranch(ctx, repoID, pusherID, branchName, commit) + if err != nil { + return fmt.Errorf("git_model.UpdateBranch %d:%s failed: %v", repoID, branchName, err) + } + if cnt > 0 { // This means branch does exist, so it's a normal update. It also means the branch has been synced. + hasBranch = true + continue + } + + if !hasBranch { + // if user haven't visit UI but directly push to a branch after upgrading from 1.20 -> 1.21, + // we cannot simply insert the branch but need to check we have branches or not + hasBranch, err = db.Exist[git_model.Branch](ctx, git_model.FindBranchOptions{ + RepoID: repoID, + IsDeletedBranch: optional.Some(false), + }.ToConds()) + if err != nil { + return err + } + if !hasBranch { + if _, err = repo_module.SyncRepoBranches(ctx, repoID, pusherID); err != nil { + return fmt.Errorf("repo_module.SyncRepoBranches %d:%s failed: %v", repoID, branchName, err) + } + return nil + } + } + + // if database have branches but not this branch, it means this is a new branch + newBranches = append(newBranches, &git_model.Branch{ + RepoID: repoID, + Name: branchName, + CommitID: commit.ID.String(), + CommitMessage: commit.Summary(), + PusherID: pusherID, + CommitTime: timeutil.TimeStamp(commit.Committer.When.Unix()), + }) } - return nil - } - // if database have branches but not this branch, it means this is a new branch - return db.Insert(ctx, &git_model.Branch{ - RepoID: repoID, - Name: branchName, - CommitID: commit.ID.String(), - CommitMessage: commit.Summary(), - PusherID: pusherID, - CommitTime: timeutil.TimeStamp(commit.Committer.When.Unix()), + if len(newBranches) > 0 { + return db.Insert(ctx, newBranches) + } + return nil }) } From 0cfa28f0a47fe9e23f6610db91ca05f26f345ff4 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 1 Mar 2024 21:29:52 +0800 Subject: [PATCH 08/11] chore: lint --- routers/private/hook_post_receive.go | 2 +- services/repository/branch.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 4043448e525a..c5504126f865 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -95,7 +95,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { return } - var branchesToSync = make([]*repo_module.PushUpdateOptions, 0, len(updates)) + branchesToSync := make([]*repo_module.PushUpdateOptions, 0, len(updates)) for _, update := range updates { if !update.RefFullName.IsBranch() { continue diff --git a/services/repository/branch.go b/services/repository/branch.go index 51eed062e543..7bbdb4f8d2e5 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -227,7 +227,7 @@ func checkBranchName(ctx context.Context, repo *repo_model.Repository, name stri // If no record is affected, that means the branch does not exist in database. So there are two possibilities. // One is this is a new branch, then we just need to insert the record. Another is the branches haven't been synced, // then we need to sync all the branches into database. -func SyncBranchesToDB(ctx context.Context, repoID, pusherID int64, branchNames []string, commitIDs []string, getCommit func(commitID string) (*git.Commit, error)) error { +func SyncBranchesToDB(ctx context.Context, repoID, pusherID int64, branchNames, commitIDs []string, getCommit func(commitID string) (*git.Commit, error)) error { // Some designs that make the code look strange but are made for performance optimization purposes: // 1. Sync branches in a batch to reduce the number of DB queries. // 2. Lazy load commit information since it may be not necessary. From d1bb3e0084be3d26cca632f389d7831eba5f1600 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 4 Mar 2024 11:48:19 +0800 Subject: [PATCH 09/11] fix: check if never synced --- services/repository/branch.go | 61 +++++++++++++++++------------------ 1 file changed, 29 insertions(+), 32 deletions(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index 7bbdb4f8d2e5..68c2b0390dba 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -222,11 +222,10 @@ func checkBranchName(ctx context.Context, repo *repo_model.Repository, name stri return err } -// SyncBranchesToDB sync the branch information in the database. It will try to update the branch first, -// if updated success with affect records > 0, then all are done. Because that means the branch has been in the database. -// If no record is affected, that means the branch does not exist in database. So there are two possibilities. -// One is this is a new branch, then we just need to insert the record. Another is the branches haven't been synced, -// then we need to sync all the branches into database. +// SyncBranchesToDB sync the branch information in the database. +// It will check whether the branches of the repository have never been synced before. +// If so, it will sync all branches of the repository. +// Otherwise, it will sync the branches that need to be updated. func SyncBranchesToDB(ctx context.Context, repoID, pusherID int64, branchNames, commitIDs []string, getCommit func(commitID string) (*git.Commit, error)) error { // Some designs that make the code look strange but are made for performance optimization purposes: // 1. Sync branches in a batch to reduce the number of DB queries. @@ -248,18 +247,36 @@ func SyncBranchesToDB(ctx context.Context, repoID, pusherID int64, branchNames, if err != nil { return fmt.Errorf("git_model.GetBranches: %v", err) } + + if len(branches) == 0 { + // if user haven't visit UI but directly push to a branch after upgrading from 1.20 -> 1.21, + // we cannot simply insert the branch but need to check we have branches or not + hasBranch, err := db.Exist[git_model.Branch](ctx, git_model.FindBranchOptions{ + RepoID: repoID, + IsDeletedBranch: optional.Some(false), + }.ToConds()) + if err != nil { + return err + } + if !hasBranch { + if _, err = repo_module.SyncRepoBranches(ctx, repoID, pusherID); err != nil { + return fmt.Errorf("repo_module.SyncRepoBranches %d failed: %v", repoID, err) + } + return nil + } + } + branchMap := make(map[string]*git_model.Branch, len(branches)) for _, branch := range branches { branchMap[branch.Name] = branch } - hasBranch := len(branches) > 0 - newBranches := make([]*git_model.Branch, 0, len(branchNames)) for i, branchName := range branchNames { commitID := commitIDs[i] - if branch, ok := branchMap[branchName]; ok && branch.CommitID == commitID { + branch, exist := branchMap[branchName] + if exist && branch.CommitID == commitID { continue } @@ -268,31 +285,11 @@ func SyncBranchesToDB(ctx context.Context, repoID, pusherID int64, branchNames, return fmt.Errorf("get commit of %s failed: %v", branchName, err) } - cnt, err := git_model.UpdateBranch(ctx, repoID, pusherID, branchName, commit) - if err != nil { - return fmt.Errorf("git_model.UpdateBranch %d:%s failed: %v", repoID, branchName, err) - } - if cnt > 0 { // This means branch does exist, so it's a normal update. It also means the branch has been synced. - hasBranch = true - continue - } - - if !hasBranch { - // if user haven't visit UI but directly push to a branch after upgrading from 1.20 -> 1.21, - // we cannot simply insert the branch but need to check we have branches or not - hasBranch, err = db.Exist[git_model.Branch](ctx, git_model.FindBranchOptions{ - RepoID: repoID, - IsDeletedBranch: optional.Some(false), - }.ToConds()) - if err != nil { - return err - } - if !hasBranch { - if _, err = repo_module.SyncRepoBranches(ctx, repoID, pusherID); err != nil { - return fmt.Errorf("repo_module.SyncRepoBranches %d:%s failed: %v", repoID, branchName, err) - } - return nil + if exist { + if _, err := git_model.UpdateBranch(ctx, repoID, pusherID, branchName, commit); err != nil { + return fmt.Errorf("git_model.UpdateBranch %d:%s failed: %v", repoID, branchName, err) } + return nil } // if database have branches but not this branch, it means this is a new branch From 7d7bdebda3f9ed743a734bf64d0622c1bf926e8a Mon Sep 17 00:00:00 2001 From: Jason Song Date: Tue, 5 Mar 2024 15:00:13 +0800 Subject: [PATCH 10/11] test: TestGitPush --- tests/integration/git_push_test.go | 131 +++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 tests/integration/git_push_test.go diff --git a/tests/integration/git_push_test.go b/tests/integration/git_push_test.go new file mode 100644 index 000000000000..75af0f858ce4 --- /dev/null +++ b/tests/integration/git_push_test.go @@ -0,0 +1,131 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "fmt" + "net/url" + "testing" + + "code.gitea.io/gitea/models/db" + git_model "code.gitea.io/gitea/models/git" + "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/assert" + "github.com/stretchr/testify/require" +) + +func TestGitPush(t *testing.T) { + onGiteaRun(t, testGitPush) +} + +func testGitPush(t *testing.T, u *url.URL) { + t.Run("Push branches at once", func(t *testing.T) { + runTestGitPush(t, u, func(t *testing.T, gitPath string) (pushed, deleted []string) { + for i := 0; i < 100; i++ { + branchName := fmt.Sprintf("branch-%d", i) + pushed = append(pushed, branchName) + doGitCreateBranch(gitPath, branchName)(t) + } + pushed = append(pushed, "master") + doGitPushTestRepository(gitPath, "origin", "--all")(t) + return + }) + }) + + t.Run("Push branches one by one", func(t *testing.T) { + runTestGitPush(t, u, func(t *testing.T, gitPath string) (pushed, deleted []string) { + for i := 0; i < 100; i++ { + branchName := fmt.Sprintf("branch-%d", i) + doGitCreateBranch(gitPath, branchName)(t) + doGitPushTestRepository(gitPath, "origin", branchName)(t) + pushed = append(pushed, branchName) + } + return + }) + }) + + 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 + pushed = append(pushed, "master") + + for i := 0; i < 100; i++ { + branchName := fmt.Sprintf("branch-%d", i) + pushed = append(pushed, branchName) + doGitCreateBranch(gitPath, branchName)(t) + } + doGitPushTestRepository(gitPath, "origin", "--all")(t) + + for i := 0; i < 10; i++ { + branchName := fmt.Sprintf("branch-%d", i) + doGitPushTestRepository(gitPath, "origin", "--delete", branchName)(t) + deleted = append(deleted, branchName) + } + return + }) + }) +} + +func runTestGitPush(t *testing.T, u *url.URL, gitOperation func(t *testing.T, gitPath string) (pushed, deleted []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() + + pushedBranches, deletedBranches := gitOperation(t, gitPath) + + dbBranches := make([]*git_model.Branch, 0) + require.NoError(t, db.GetEngine(db.DefaultContext).Where("repo_id=?", repo.ID).Find(&dbBranches)) + assert.Equalf(t, len(pushedBranches), len(dbBranches), "mismatched number of branches in db") + dbBranchesMap := make(map[string]*git_model.Branch, len(dbBranches)) + for _, branch := range dbBranches { + dbBranchesMap[branch.Name] = branch + } + + deletedBranchesMap := make(map[string]bool, len(deletedBranches)) + for _, branchName := range deletedBranches { + deletedBranchesMap[branchName] = true + } + + for _, branchName := range pushedBranches { + branch, ok := dbBranchesMap[branchName] + deleted := deletedBranchesMap[branchName] + assert.True(t, ok, "branch %s not found in database", branchName) + assert.Equal(t, deleted, branch.IsDeleted, "IsDeleted of %s is %v, but it's expected to be %v", branchName, branch.IsDeleted, deleted) + commitID, err := gitRepo.GetBranchCommitID(branchName) + require.NoError(t, err) + assert.Equal(t, commitID, branch.CommitID) + } + + require.NoError(t, repo_service.DeleteRepositoryDirectly(db.DefaultContext, user, repo.ID)) +} From b204dbb87676a455aa90c90f09210687406c9839 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Tue, 5 Mar 2024 15:23:15 +0800 Subject: [PATCH 11/11] chore: lint --- tests/integration/git_push_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/integration/git_push_test.go b/tests/integration/git_push_test.go index 75af0f858ce4..cb2910b175e8 100644 --- a/tests/integration/git_push_test.go +++ b/tests/integration/git_push_test.go @@ -33,7 +33,7 @@ func testGitPush(t *testing.T, u *url.URL) { } pushed = append(pushed, "master") doGitPushTestRepository(gitPath, "origin", "--all")(t) - return + return pushed, deleted }) }) @@ -45,7 +45,7 @@ func testGitPush(t *testing.T, u *url.URL) { doGitPushTestRepository(gitPath, "origin", branchName)(t) pushed = append(pushed, branchName) } - return + return pushed, deleted }) }) @@ -66,7 +66,7 @@ func testGitPush(t *testing.T, u *url.URL) { doGitPushTestRepository(gitPath, "origin", "--delete", branchName)(t) deleted = append(deleted, branchName) } - return + return pushed, deleted }) }) }