From 87db4a47c8e22b7c2e4f2b9f9efc8df1e3622884 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 11 Dec 2023 14:16:56 +0800 Subject: [PATCH] Also sync DB branches on push if necessary (#28361) (#28403) Fix #28056 Backport #28361 This PR will check whether the repo has zero branch when pushing a branch. If that, it means this repository hasn't been synced. The reason caused that is after user upgrade from v1.20 -> v1.21, he just push branches without visit the repository user interface. Because all repositories routers will check whether a branches sync is necessary but push has not such check. For every repository, it has two states, synced or not synced. If there is zero branch for a repository, then it will be assumed as non-sync state. Otherwise, it's synced state. So if we think it's synced, we just need to update branch/insert new branch. Otherwise do a full sync. So that, for every push, there will be almost no extra load added. It's high performance than yours. For the implementation, we in fact 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. --- models/db/context.go | 9 ++ models/db/error.go | 18 ++++ models/git/branch.go | 33 +++--- models/git/branch_list.go | 8 +- models/git/branch_test.go | 2 +- models/repo/repo.go | 16 +++ routers/api/v1/repo/branch.go | 5 +- routers/web/repo/branch.go | 4 +- services/repository/branch.go | 102 ++++++++++++------ services/repository/push.go | 2 +- .../api_repo_get_contents_list_test.go | 9 +- .../integration/api_repo_get_contents_test.go | 9 +- 12 files changed, 148 insertions(+), 69 deletions(-) diff --git a/models/db/context.go b/models/db/context.go index 521857fae836..9f72b4355582 100644 --- a/models/db/context.go +++ b/models/db/context.go @@ -178,6 +178,15 @@ func GetByBean(ctx context.Context, bean any) (bool, error) { return GetEngine(ctx).Get(bean) } +func Exist[T any](ctx context.Context, cond builder.Cond) (bool, error) { + if !cond.IsValid() { + return false, ErrConditionRequired{} + } + + var bean T + return GetEngine(ctx).Where(cond).NoAutoCondition().Exist(&bean) +} + // DeleteByBean deletes all records according non-empty fields of the bean as conditions. func DeleteByBean(ctx context.Context, bean any) (int64, error) { return GetEngine(ctx).Delete(bean) diff --git a/models/db/error.go b/models/db/error.go index 665e970e17c1..f601a15c01ef 100644 --- a/models/db/error.go +++ b/models/db/error.go @@ -72,3 +72,21 @@ func (err ErrNotExist) Error() string { func (err ErrNotExist) Unwrap() error { return util.ErrNotExist } + +// ErrConditionRequired represents an error which require condition. +type ErrConditionRequired struct{} + +// IsErrConditionRequired checks if an error is an ErrConditionRequired +func IsErrConditionRequired(err error) bool { + _, ok := err.(ErrConditionRequired) + return ok +} + +func (err ErrConditionRequired) Error() string { + return "condition is required" +} + +// Unwrap unwraps this as a ErrNotExist err +func (err ErrConditionRequired) Unwrap() error { + return util.ErrInvalidArgument +} diff --git a/models/git/branch.go b/models/git/branch.go index 6d50fb9fb6a1..ffd1d7ed164a 100644 --- a/models/git/branch.go +++ b/models/git/branch.go @@ -205,10 +205,9 @@ func DeleteBranches(ctx context.Context, repoID, doerID int64, branchIDs []int64 }) } -// UpdateBranch updates the branch information in the database. If the branch exist, it will update latest commit of this branch information -// If it doest not exist, insert a new record into database -func UpdateBranch(ctx context.Context, repoID, pusherID int64, branchName string, commit *git.Commit) error { - cnt, err := db.GetEngine(ctx).Where("repo_id=? AND name=?", repoID, branchName). +// UpdateBranch updates the branch information in the database. +func UpdateBranch(ctx context.Context, repoID, pusherID int64, branchName string, commit *git.Commit) (int64, error) { + return db.GetEngine(ctx).Where("repo_id=? AND name=?", repoID, branchName). Cols("commit_id, commit_message, pusher_id, commit_time, is_deleted, updated_unix"). Update(&Branch{ CommitID: commit.ID.String(), @@ -217,21 +216,6 @@ func UpdateBranch(ctx context.Context, repoID, pusherID int64, branchName string CommitTime: timeutil.TimeStamp(commit.Committer.When.Unix()), IsDeleted: false, }) - if err != nil { - return err - } - if cnt > 0 { - return nil - } - - return db.Insert(ctx, &Branch{ - RepoID: repoID, - Name: branchName, - CommitID: commit.ID.String(), - CommitMessage: commit.Summary(), - PusherID: pusherID, - CommitTime: timeutil.TimeStamp(commit.Committer.When.Unix()), - }) } // AddDeletedBranch adds a deleted branch to the database @@ -308,6 +292,17 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, from, to str sess := db.GetEngine(ctx) + var branch Branch + exist, err := db.GetEngine(ctx).Where("repo_id=? AND name=?", repo.ID, from).Get(&branch) + if err != nil { + return err + } else if !exist || branch.IsDeleted { + return ErrBranchNotExist{ + RepoID: repo.ID, + BranchName: from, + } + } + // 1. update branch in database if n, err := sess.Where("repo_id=? AND name=?", repo.ID, from).Update(&Branch{ Name: to, diff --git a/models/git/branch_list.go b/models/git/branch_list.go index b5c1301a1dc0..2efe495264ab 100644 --- a/models/git/branch_list.go +++ b/models/git/branch_list.go @@ -73,7 +73,7 @@ type FindBranchOptions struct { Keyword string } -func (opts *FindBranchOptions) Cond() builder.Cond { +func (opts FindBranchOptions) ToConds() builder.Cond { cond := builder.NewCond() if opts.RepoID > 0 { cond = cond.And(builder.Eq{"repo_id": opts.RepoID}) @@ -92,7 +92,7 @@ func (opts *FindBranchOptions) Cond() builder.Cond { } func CountBranches(ctx context.Context, opts FindBranchOptions) (int64, error) { - return db.GetEngine(ctx).Where(opts.Cond()).Count(&Branch{}) + return db.GetEngine(ctx).Where(opts.ToConds()).Count(&Branch{}) } func orderByBranches(sess *xorm.Session, opts FindBranchOptions) *xorm.Session { @@ -108,7 +108,7 @@ func orderByBranches(sess *xorm.Session, opts FindBranchOptions) *xorm.Session { } func FindBranches(ctx context.Context, opts FindBranchOptions) (BranchList, error) { - sess := db.GetEngine(ctx).Where(opts.Cond()) + sess := db.GetEngine(ctx).Where(opts.ToConds()) if opts.PageSize > 0 && !opts.IsListAll() { sess = db.SetSessionPagination(sess, &opts.ListOptions) } @@ -119,7 +119,7 @@ func FindBranches(ctx context.Context, opts FindBranchOptions) (BranchList, erro } func FindBranchNames(ctx context.Context, opts FindBranchOptions) ([]string, error) { - sess := db.GetEngine(ctx).Select("name").Where(opts.Cond()) + sess := db.GetEngine(ctx).Select("name").Where(opts.ToConds()) if opts.PageSize > 0 && !opts.IsListAll() { sess = db.SetSessionPagination(sess, &opts.ListOptions) } diff --git a/models/git/branch_test.go b/models/git/branch_test.go index ba6902692792..07b243e5e6e9 100644 --- a/models/git/branch_test.go +++ b/models/git/branch_test.go @@ -37,7 +37,7 @@ func TestAddDeletedBranch(t *testing.T) { }, } - err := git_model.UpdateBranch(db.DefaultContext, repo.ID, secondBranch.PusherID, secondBranch.Name, commit) + _, err := git_model.UpdateBranch(db.DefaultContext, repo.ID, secondBranch.PusherID, secondBranch.Name, commit) assert.NoError(t, err) } diff --git a/models/repo/repo.go b/models/repo/repo.go index 5ebc7bfc2420..0b0c029993a9 100644 --- a/models/repo/repo.go +++ b/models/repo/repo.go @@ -47,6 +47,14 @@ func (err ErrUserDoesNotHaveAccessToRepo) Unwrap() error { return util.ErrPermissionDenied } +type ErrRepoIsArchived struct { + Repo *Repository +} + +func (err ErrRepoIsArchived) Error() string { + return fmt.Sprintf("%s is archived", err.Repo.LogString()) +} + var ( reservedRepoNames = []string{".", "..", "-"} reservedRepoPatterns = []string{"*.git", "*.wiki", "*.rss", "*.atom"} @@ -654,6 +662,14 @@ func (repo *Repository) GetTrustModel() TrustModelType { return trustModel } +// MustNotBeArchived returns ErrRepoIsArchived if the repo is archived +func (repo *Repository) MustNotBeArchived() error { + if repo.IsArchived { + return ErrRepoIsArchived{Repo: repo} + } + return nil +} + // __________ .__ __ // \______ \ ____ ______ ____ _____|__|/ |_ ___________ ___.__. // | _// __ \\____ \ / _ \/ ___/ \ __\/ _ \_ __ < | | diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index c851525c0f02..922f15f7499c 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -262,12 +262,11 @@ func CreateBranch(ctx *context.APIContext) { } } - err = repo_service.CreateNewBranchFromCommit(ctx, ctx.Doer, ctx.Repo.Repository, oldCommit.ID.String(), opt.BranchName) + err = repo_service.CreateNewBranchFromCommit(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, oldCommit.ID.String(), opt.BranchName) if err != nil { if git_model.IsErrBranchNotExist(err) { ctx.Error(http.StatusNotFound, "", "The old branch does not exist") - } - if models.IsErrTagAlreadyExists(err) { + } else if models.IsErrTagAlreadyExists(err) { ctx.Error(http.StatusConflict, "", "The branch with the same tag already exists.") } else if git_model.IsErrBranchAlreadyExists(err) || git.IsErrPushOutOfDate(err) { ctx.Error(http.StatusConflict, "", "The branch already exists.") diff --git a/routers/web/repo/branch.go b/routers/web/repo/branch.go index e0e27fd4823c..6afe4ad68a87 100644 --- a/routers/web/repo/branch.go +++ b/routers/web/repo/branch.go @@ -191,9 +191,9 @@ func CreateBranch(ctx *context.Context) { } err = release_service.CreateNewTag(ctx, ctx.Doer, ctx.Repo.Repository, target, form.NewBranchName, "") } else if ctx.Repo.IsViewBranch { - err = repo_service.CreateNewBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.BranchName, form.NewBranchName) + err = repo_service.CreateNewBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, ctx.Repo.BranchName, form.NewBranchName) } else { - err = repo_service.CreateNewBranchFromCommit(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.CommitID, form.NewBranchName) + err = repo_service.CreateNewBranchFromCommit(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, ctx.Repo.CommitID, form.NewBranchName) } if err != nil { if models.IsErrProtectedTagName(err) { diff --git a/services/repository/branch.go b/services/repository/branch.go index 011dc5568ec0..d56a0660c67c 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -20,6 +20,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/queue" repo_module "code.gitea.io/gitea/modules/repository" + "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" notify_service "code.gitea.io/gitea/services/notify" files_service "code.gitea.io/gitea/services/repository/files" @@ -28,30 +29,13 @@ import ( ) // CreateNewBranch creates a new repository branch -func CreateNewBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, oldBranchName, branchName string) (err error) { - // Check if branch name can be used - if err := checkBranchName(ctx, repo, branchName); err != nil { +func CreateNewBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, oldBranchName, branchName string) (err error) { + branch, err := git_model.GetBranch(ctx, repo.ID, oldBranchName) + if err != nil { return err } - if !git.IsBranchExist(ctx, repo.RepoPath(), oldBranchName) { - return git_model.ErrBranchNotExist{ - BranchName: oldBranchName, - } - } - - if err := git.Push(ctx, repo.RepoPath(), git.PushOptions{ - Remote: repo.RepoPath(), - Branch: fmt.Sprintf("%s%s:%s%s", git.BranchPrefix, oldBranchName, git.BranchPrefix, branchName), - Env: repo_module.PushingEnvironment(doer, repo), - }); err != nil { - if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) { - return err - } - return fmt.Errorf("push: %w", err) - } - - return nil + return CreateNewBranchFromCommit(ctx, doer, repo, gitRepo, branch.CommitID, branchName) } // Branch contains the branch information @@ -244,25 +228,81 @@ 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 + } + + // 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: util.OptionalBoolFalse, + }.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 + 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()), + }) +} + // CreateNewBranchFromCommit creates a new repository branch -func CreateNewBranchFromCommit(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, commit, branchName string) (err error) { +func CreateNewBranchFromCommit(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, commitID, branchName string) (err error) { + err = repo.MustNotBeArchived() + if err != nil { + return err + } + // Check if branch name can be used if err := checkBranchName(ctx, repo, branchName); err != nil { return err } - if err := git.Push(ctx, repo.RepoPath(), git.PushOptions{ - Remote: repo.RepoPath(), - Branch: fmt.Sprintf("%s:%s%s", commit, git.BranchPrefix, branchName), - Env: repo_module.PushingEnvironment(doer, repo), - }); err != nil { - if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) { + return db.WithTx(ctx, func(ctx context.Context) error { + commit, err := gitRepo.GetCommit(commitID) + if err != nil { + return err + } + // database operation should be done before git operation so that we can rollback if git operation failed + if err := syncBranchToDB(ctx, repo.ID, doer.ID, branchName, commit); err != nil { return err } - return fmt.Errorf("push: %w", err) - } - return nil + if err := git.Push(ctx, repo.RepoPath(), git.PushOptions{ + Remote: repo.RepoPath(), + Branch: fmt.Sprintf("%s:%s%s", commitID, git.BranchPrefix, branchName), + Env: repo_module.PushingEnvironment(doer, repo), + }); err != nil { + if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) { + return err + } + return fmt.Errorf("push: %w", err) + } + return nil + }) } // RenameBranch rename a branch diff --git a/services/repository/push.go b/services/repository/push.go index 97da45f52b4b..90aac95323c5 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -257,7 +257,7 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { commits.Commits = commits.Commits[:setting.UI.FeedMaxCommitNum] } - if err = git_model.UpdateBranch(ctx, repo.ID, opts.PusherID, branch, newCommit); err != nil { + 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) } diff --git a/tests/integration/api_repo_get_contents_list_test.go b/tests/integration/api_repo_get_contents_list_test.go index f3a5159115c1..7874eddfd4d1 100644 --- a/tests/integration/api_repo_get_contents_list_test.go +++ b/tests/integration/api_repo_get_contents_list_test.go @@ -70,15 +70,16 @@ func testAPIGetContentsList(t *testing.T, u *url.URL) { session = loginUser(t, user4.Name) token4 := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadRepository) - // Make a new branch in repo1 - newBranch := "test_branch" - err := repo_service.CreateNewBranch(git.DefaultContext, user2, repo1, repo1.DefaultBranch, newBranch) - assert.NoError(t, err) // Get the commit ID of the default branch gitRepo, err := git.OpenRepository(git.DefaultContext, repo1.RepoPath()) assert.NoError(t, err) defer gitRepo.Close() + // Make a new branch in repo1 + newBranch := "test_branch" + err = repo_service.CreateNewBranch(git.DefaultContext, user2, repo1, gitRepo, repo1.DefaultBranch, newBranch) + assert.NoError(t, err) + commitID, _ := gitRepo.GetBranchCommitID(repo1.DefaultBranch) // Make a new tag in repo1 newTag := "test_tag" diff --git a/tests/integration/api_repo_get_contents_test.go b/tests/integration/api_repo_get_contents_test.go index 709bbe082a5a..1d708a4cdb43 100644 --- a/tests/integration/api_repo_get_contents_test.go +++ b/tests/integration/api_repo_get_contents_test.go @@ -72,15 +72,16 @@ func testAPIGetContents(t *testing.T, u *url.URL) { session = loginUser(t, user4.Name) token4 := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadRepository) - // Make a new branch in repo1 - newBranch := "test_branch" - err := repo_service.CreateNewBranch(git.DefaultContext, user2, repo1, repo1.DefaultBranch, newBranch) - assert.NoError(t, err) // Get the commit ID of the default branch gitRepo, err := git.OpenRepository(git.DefaultContext, repo1.RepoPath()) assert.NoError(t, err) defer gitRepo.Close() + // Make a new branch in repo1 + newBranch := "test_branch" + err = repo_service.CreateNewBranch(git.DefaultContext, user2, repo1, gitRepo, repo1.DefaultBranch, newBranch) + assert.NoError(t, err) + commitID, err := gitRepo.GetBranchCommitID(repo1.DefaultBranch) assert.NoError(t, err) // Make a new tag in repo1