From 1856b43b50c69e06b3ad0491879bd19164d5640d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 5 Dec 2023 00:26:38 -0800 Subject: [PATCH 1/8] Fix the possible branches sync failure when user push branches directly but not visit UI after upgrading from v1.20 -> v1.21 --- models/git/branch.go | 19 ++---------------- modules/repository/branch.go | 38 ++++++++++++++++++++++++++++++++++++ services/repository/push.go | 2 +- 3 files changed, 41 insertions(+), 18 deletions(-) diff --git a/models/git/branch.go b/models/git/branch.go index 6d50fb9fb6a1..cf37d3aa9255 100644 --- a/models/git/branch.go +++ b/models/git/branch.go @@ -207,8 +207,8 @@ 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). +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 +217,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 diff --git a/modules/repository/branch.go b/modules/repository/branch.go index 8daceecb44ff..933fb49e3841 100644 --- a/modules/repository/branch.go +++ b/modules/repository/branch.go @@ -5,6 +5,7 @@ package repository import ( "context" + "fmt" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" @@ -13,8 +14,45 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/timeutil" + "code.gitea.io/gitea/modules/util" ) +// 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 := 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 { // if branch does exist, so it's a normal update + 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 + totalBranches, err := db.Count[*git_model.Branch](ctx, &git_model.FindBranchOptions{ + RepoID: repoID, + IsDeletedBranch: util.OptionalBoolFalse, + }) + if err != nil { + return err + } + if totalBranches == 0 { + if _, err = SyncRepoBranches(ctx, repoID, pusherID); err != nil { + return fmt.Errorf("repo_module.SyncRepoBranches %d:%s failed: %v", repoID, branchName, err) + } + return nil + } + 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()), + }) +} + // SyncRepoBranches synchronizes branch table with repository branches func SyncRepoBranches(ctx context.Context, repoID, doerID int64) (int64, error) { repo, err := repo_model.GetRepositoryByID(ctx, repoID) diff --git a/services/repository/push.go b/services/repository/push.go index 391c8ad4ca7f..b8bec98c5d4f 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -259,7 +259,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 = repo_module.UpdateBranch(ctx, repo.ID, opts.PusherID, branch, newCommit); err != nil { return fmt.Errorf("git_model.UpdateBranch %s:%s failed: %v", repo.FullName(), branch, err) } From a036d149949b2748e6c2f6283169a0fff418c2b8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 5 Dec 2023 01:38:34 -0800 Subject: [PATCH 2/8] Fix lint --- models/git/branch_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) } From f413a5f162988965a1ccfeec71940c11bde35913 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 5 Dec 2023 11:08:06 -0800 Subject: [PATCH 3/8] Fix bugs --- models/git/branch.go | 11 ++++ modules/repository/branch.go | 2 +- routers/api/v1/repo/branch.go | 5 +- routers/web/repo/branch.go | 4 +- services/repository/branch.go | 57 +++++++------------ .../api_repo_get_contents_list_test.go | 9 +-- .../integration/api_repo_get_contents_test.go | 9 +-- 7 files changed, 48 insertions(+), 49 deletions(-) diff --git a/models/git/branch.go b/models/git/branch.go index cf37d3aa9255..3853ce745d1c 100644 --- a/models/git/branch.go +++ b/models/git/branch.go @@ -293,6 +293,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/modules/repository/branch.go b/modules/repository/branch.go index 933fb49e3841..8a7170eb5bc9 100644 --- a/modules/repository/branch.go +++ b/modules/repository/branch.go @@ -30,7 +30,7 @@ func UpdateBranch(ctx context.Context, repoID, pusherID int64, branchName string // 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 - totalBranches, err := db.Count[*git_model.Branch](ctx, &git_model.FindBranchOptions{ + totalBranches, err := db.Count[git_model.Branch](ctx, &git_model.FindBranchOptions{ RepoID: repoID, IsDeletedBranch: util.OptionalBoolFalse, }) diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index f2386a607e65..677105bdb506 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -251,12 +251,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 bc72d5f2eca1..a0bc1dadade8 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 735fa1a75681..4fcf938a6075 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -28,35 +28,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) { - err = repo.MustNotBeArchived() +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 } - // Check if branch name can be used - if err := checkBranchName(ctx, repo, branchName); 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 @@ -250,7 +228,7 @@ func checkBranchName(ctx context.Context, repo *repo_model.Repository, name stri } // 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 @@ -261,18 +239,27 @@ func CreateNewBranchFromCommit(ctx context.Context, doer *user_model.User, repo 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 + } + if err := repo_module.UpdateBranch(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/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 From a3d8eb105483a0e1356bd94a21d6632e734a8a88 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 6 Dec 2023 23:44:00 -0800 Subject: [PATCH 4/8] Use db.Exist instead of db.Count --- models/git/branch.go | 3 +-- modules/repository/branch.go | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/models/git/branch.go b/models/git/branch.go index 3853ce745d1c..ffd1d7ed164a 100644 --- a/models/git/branch.go +++ b/models/git/branch.go @@ -205,8 +205,7 @@ 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 +// 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"). diff --git a/modules/repository/branch.go b/modules/repository/branch.go index 8a7170eb5bc9..427c06076136 100644 --- a/modules/repository/branch.go +++ b/modules/repository/branch.go @@ -30,14 +30,14 @@ func UpdateBranch(ctx context.Context, repoID, pusherID int64, branchName string // 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 - totalBranches, err := db.Count[git_model.Branch](ctx, &git_model.FindBranchOptions{ + hasBranch, err := db.Exist[git_model.Branch](ctx, git_model.FindBranchOptions{ RepoID: repoID, IsDeletedBranch: util.OptionalBoolFalse, - }) + }.ToConds()) if err != nil { return err } - if totalBranches == 0 { + if !hasBranch { if _, err = SyncRepoBranches(ctx, repoID, pusherID); err != nil { return fmt.Errorf("repo_module.SyncRepoBranches %d:%s failed: %v", repoID, branchName, err) } From e6146c03101077766f3a3352700a297347fd7e0b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 7 Dec 2023 00:06:42 -0800 Subject: [PATCH 5/8] Exist doesn't need to check empty condition --- models/db/context.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/models/db/context.go b/models/db/context.go index 7b739f7e9f35..022e62967b76 100644 --- a/models/db/context.go +++ b/models/db/context.go @@ -200,10 +200,6 @@ func GetByID[T any](ctx context.Context, id int64) (object *T, exist bool, err e } 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) } From dc7828ed297ed4cd557044eed2cdff6447febc42 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 9 Dec 2023 15:02:04 +0800 Subject: [PATCH 6/8] Move function to service layer --- modules/repository/branch.go | 38 ---------------------------------- services/repository/branch.go | 39 ++++++++++++++++++++++++++++++++++- services/repository/push.go | 2 +- 3 files changed, 39 insertions(+), 40 deletions(-) diff --git a/modules/repository/branch.go b/modules/repository/branch.go index 427c06076136..8daceecb44ff 100644 --- a/modules/repository/branch.go +++ b/modules/repository/branch.go @@ -5,7 +5,6 @@ package repository import ( "context" - "fmt" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" @@ -14,45 +13,8 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/timeutil" - "code.gitea.io/gitea/modules/util" ) -// 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 := 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 { // if branch does exist, so it's a normal update - 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 = SyncRepoBranches(ctx, repoID, pusherID); err != nil { - return fmt.Errorf("repo_module.SyncRepoBranches %d:%s failed: %v", repoID, branchName, err) - } - return nil - } - 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()), - }) -} - // SyncRepoBranches synchronizes branch table with repository branches func SyncRepoBranches(ctx context.Context, repoID, doerID int64) (int64, error) { repo, err := repo_model.GetRepositoryByID(ctx, repoID) diff --git a/services/repository/branch.go b/services/repository/branch.go index 4fcf938a6075..ab7f83d08118 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" @@ -227,6 +228,42 @@ func checkBranchName(ctx context.Context, repo *repo_model.Repository, name stri return err } +// 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 := 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 { // if branch does exist, so it's a normal update + 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 + } + 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, gitRepo *git.Repository, commitID, branchName string) (err error) { err = repo.MustNotBeArchived() @@ -244,7 +281,7 @@ func CreateNewBranchFromCommit(ctx context.Context, doer *user_model.User, repo if err != nil { return err } - if err := repo_module.UpdateBranch(ctx, repo.ID, doer.ID, branchName, commit); err != nil { + if err := updateBranch(ctx, repo.ID, doer.ID, branchName, commit); err != nil { return err } diff --git a/services/repository/push.go b/services/repository/push.go index b8bec98c5d4f..ac9cc7a16d08 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -259,7 +259,7 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { commits.Commits = commits.Commits[:setting.UI.FeedMaxCommitNum] } - if err = repo_module.UpdateBranch(ctx, repo.ID, opts.PusherID, branch, newCommit); err != nil { + if err = updateBranch(ctx, repo.ID, opts.PusherID, branch, newCommit); err != nil { return fmt.Errorf("git_model.UpdateBranch %s:%s failed: %v", repo.FullName(), branch, err) } From c6874c70fe1300460421e6e44400d430e80ec5fd Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 9 Dec 2023 15:13:13 +0800 Subject: [PATCH 7/8] rename update branch function to syncBranchToDB --- models/db/context.go | 4 ++++ services/repository/branch.go | 16 +++++++++++----- services/repository/push.go | 2 +- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/models/db/context.go b/models/db/context.go index 022e62967b76..7b739f7e9f35 100644 --- a/models/db/context.go +++ b/models/db/context.go @@ -200,6 +200,10 @@ func GetByID[T any](ctx context.Context, id int64) (object *T, exist bool, err e } 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) } diff --git a/services/repository/branch.go b/services/repository/branch.go index ab7f83d08118..6dc286675f3d 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -228,14 +228,17 @@ func checkBranchName(ctx context.Context, repo *repo_model.Repository, name stri return err } -// 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 { +// 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 { // if branch does exist, so it's a normal update + if cnt > 0 { // This means branch does exist, so it's a normal update. It also means the branch has been synced. return nil } @@ -254,6 +257,8 @@ func updateBranch(ctx context.Context, repoID, pusherID int64, branchName string } 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, @@ -281,7 +286,8 @@ func CreateNewBranchFromCommit(ctx context.Context, doer *user_model.User, repo if err != nil { return err } - if err := updateBranch(ctx, repo.ID, doer.ID, branchName, commit); err != nil { + // 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 } diff --git a/services/repository/push.go b/services/repository/push.go index ac9cc7a16d08..b5388834c0bd 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -259,7 +259,7 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { commits.Commits = commits.Commits[:setting.UI.FeedMaxCommitNum] } - if err = 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) } From 72669dcdbc3f9bbac25c1566d5689e7f296e74c7 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 9 Dec 2023 15:50:08 +0800 Subject: [PATCH 8/8] Fix bug --- models/git/branch_list.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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) }