diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 98928c5adcf..d6dbce009cf 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -9,7 +9,6 @@ import ( "net/http" audit_model "gitea.dev/models/audit" - git_model "gitea.dev/models/git" issues_model "gitea.dev/models/issues" repo_model "gitea.dev/models/repo" "gitea.dev/modules/git" @@ -51,24 +50,18 @@ func hookPostReceiveCollectPushUpdates(opts *private.HookOptions, repo *repo_mod } func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts *private.HookOptions, repo *repo_model.Repository, updates []*repo_module.PushUpdateOptions) { - branchesToSync := make([]*repo_module.PushUpdateOptions, 0, len(updates)) + branchNames := make([]string, 0, len(updates)) for _, update := range updates { if !update.RefFullName.IsBranch() { continue } - if update.IsDelRef() { - if err := git_model.MarkBranchAsDeleted(ctx, repo.ID, update.RefFullName.BranchName(), update.PusherID); err != nil { - log.Error("failed to mark branch %s as deleted: %v", update.RefFullName, err) - continue - } - } else { - branchesToSync = append(branchesToSync, update) - // TODO: should we return the error and return the error when pushing? Currently it will log the error and not prevent the pushing + branchNames = append(branchNames, update.RefFullName.BranchName()) + if !update.IsDelRef() { pull_service.UpdatePullsRefs(ctx, repo, update) } } - if len(branchesToSync) == 0 { + if len(branchNames) == 0 { return } @@ -78,13 +71,7 @@ func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts return } - 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, gitRepo, branchNames, commitIDs); err != nil { + if err = repo_service.SyncBranchesToDB(ctx, repo.ID, opts.UserID, gitRepo, branchNames); err != nil { log.Error("failed to sync branch to DB: %v", err) } } diff --git a/services/repository/branch.go b/services/repository/branch.go index 71a6f2c2eae..320acdf8f12 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -18,6 +18,7 @@ import ( user_model "gitea.dev/models/user" "gitea.dev/modules/cache" "gitea.dev/modules/git" + "gitea.dev/modules/globallock" "gitea.dev/modules/graceful" "gitea.dev/modules/json" "gitea.dev/modules/log" @@ -304,7 +305,7 @@ func checkBranchName(ctx context.Context, gitRepo *git.Repository, name string) // 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, gitRepo *git.Repository, branchNames, commitIDs []string) error { +func SyncBranchesToDB(ctx context.Context, repoID, pusherID int64, gitRepo *git.Repository, branchNames []string) 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. @@ -316,27 +317,31 @@ func SyncBranchesToDB(ctx context.Context, repoID, pusherID int64, gitRepo *git. // For the first batch, it will hit optimization 3. // For other batches, it will hit optimization 4. - if len(branchNames) != len(commitIDs) { - return errors.New("branchNames and commitIDs length not match") + // In case there are concurrent pushes, we need to lock the sync operation for the same repository + // and always use the latest branch information in the database. + releaser, err := globallock.Lock(ctx, fmt.Sprintf("repo_branch_sync_%d", repoID)) + if err != nil { + return err } + defer releaser() return db.WithTx(ctx, func(ctx context.Context) error { - branches, err := git_model.GetBranches(ctx, repoID, branchNames, true) + dbBranches, err := git_model.GetBranches(ctx, repoID, branchNames, true) 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, + if len(dbBranches) == 0 { + // if user haven't visited 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{ + hasAnyActiveBranch, 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 !hasAnyActiveBranch { if _, err = repo_module.SyncRepoBranches(ctx, repoID, pusherID); err != nil { return fmt.Errorf("repo_module.SyncRepoBranches %d failed: %v", repoID, err) } @@ -344,26 +349,32 @@ func SyncBranchesToDB(ctx context.Context, repoID, pusherID int64, gitRepo *git. } } - branchMap := make(map[string]*git_model.Branch, len(branches)) - for _, branch := range branches { - branchMap[branch.Name] = branch + dbBranchMap := make(map[string]*git_model.Branch, len(dbBranches)) + for _, branch := range dbBranches { + dbBranchMap[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 && !branch.IsDeleted { + for _, branchName := range branchNames { + branch, existInDb := dbBranchMap[branchName] + commit, err := gitRepo.GetCommit(ctx, git.RefNameFromBranch(branchName).String()) + if git.IsErrNotExist(err) { + if existInDb && !branch.IsDeleted { + if err := git_model.MarkBranchAsDeleted(ctx, repoID, branchName, pusherID); err != nil { + log.Error("SyncBranchesToDB: mark branch %d:%s deleted: %v", repoID, branchName, err) + } + } + continue + } else if err != nil { + log.Error("SyncBranchesToDB: get commit id of %s: %v", branchName, err) + continue + } + if existInDb && branch.CommitID == commit.ID.String() && !branch.IsDeleted { continue } - commit, err := gitRepo.GetCommit(ctx, commitID) - if err != nil { - return fmt.Errorf("get commit of %s failed: %v", branchName, err) - } - - if exist { + if existInDb { 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) } diff --git a/services/repository/branch_test.go b/services/repository/branch_test.go index 95314521465..d695553efef 100644 --- a/services/repository/branch_test.go +++ b/services/repository/branch_test.go @@ -12,6 +12,7 @@ import ( "gitea.dev/models/unit" "gitea.dev/models/unittest" user_model "gitea.dev/models/user" + "gitea.dev/modules/git" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -42,3 +43,29 @@ func TestCanDeleteBranchDefaultTargets(t *testing.T) { assert.ErrorIs(t, CanDeleteBranch(ctx, repo, "branch2", owner), ErrBranchIsDefault) } + +func TestSyncBranchesToDBUsesGitState(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + ctx := t.Context() + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + gitRepo, err := git.OpenRepository(ctx, repo) + require.NoError(t, err) + defer gitRepo.Close() + + _, err = db.GetEngine(ctx).Where("repo_id=? AND name=?", repo.ID, "master").Update(&git_model.Branch{CommitID: "985f0301dba5e7b34be866819cd15ad3d8f508ee"}) + require.NoError(t, err) + _, err = db.GetEngine(ctx).Where("repo_id=? AND name=?", repo.ID, "foo").Cols("is_deleted").Update(&git_model.Branch{IsDeleted: false}) + require.NoError(t, err) + require.NoError(t, git_model.MarkBranchAsDeleted(ctx, repo.ID, "develop", 1)) + + require.NoError(t, SyncBranchesToDB(ctx, repo.ID, 2, gitRepo, []string{"master", "foo", "develop", "pr-to-update"})) + + loadBranch := func(name string) *git_model.Branch { + return unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo.ID, Name: name}) + } + assert.Equal(t, "65f1bf27bc3bf70f64657658635e66094edbcb4d", loadBranch("master").CommitID) + assert.True(t, loadBranch("foo").IsDeleted) + assert.False(t, loadBranch("develop").IsDeleted) + assert.Equal(t, "62fb502a7172d4453f0322a2cc85bddffa57f07a", loadBranch("pr-to-update").CommitID) +}