Sync branches to DB immediately when handle git hook calling (#29493)

Unlike other async processing in the queue, we should sync branches to the DB immediately when handling git hook calling. If it fails, users can see the error message in the output of the git command. It can avoid potential inconsistency issues, and help #29494. --------- Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
2024-03-06 16:47:52 +08:00 · 2024-03-06 16:47:52 +08:00 · 5bdf805e05
parent a2b0fb1a64
commit 5bdf805e05
5 changed files with 282 additions and 44 deletions
--- 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 {
--- a/routers/private/hook_post_receive.go
+++ b/routers/private/hook_post_receive.go
@ -8,9 +8,11 @@ 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"
+	"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 +29,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 +37,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 +94,63 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
 			})
 			return
 		}
+
+		branchesToSync := make([]*repo_module.PushUpdateOptions, 0, len(updates))
+		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 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 {
+				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 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
+			}
+		}
 	}

 	// Handle Push Options
--- a/services/repository/branch.go
+++ b/services/repository/branch.go
@ -221,44 +221,91 @@ 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,
-// 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
+// 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.
+	// 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")
 	}

-	// 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 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)
+		}
+
+		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
+		}
+
+		newBranches := make([]*git_model.Branch, 0, len(branchNames))
+
+		for i, branchName := range branchNames {
+			commitID := commitIDs[i]
+			branch, exist := branchMap[branchName]
+			if exist && branch.CommitID == commitID {
+				continue
+			}
+
+			commit, err := getCommit(branchName)
+			if err != nil {
+				return fmt.Errorf("get commit of %s failed: %v", branchName, err)
+			}
+
+			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
+			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()),
+			})
+		}
+
+		if len(newBranches) > 0 {
+			return db.Insert(ctx, newBranches)
 		}
 		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()),
 	})
 }

--- 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"
@ -259,10 +258,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
@ -275,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.
--- a/tests/integration/git_push_test.go
+++ 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 pushed, deleted
+		})
+	})
+
+	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 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
+			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 pushed, deleted
+		})
+	})
+}
+
+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))
+}