diff --git a/models/git/branch.go b/models/git/branch.go index b74c7aa8b0..7954efb1cb 100644 --- a/models/git/branch.go +++ b/models/git/branch.go @@ -156,33 +156,27 @@ func init() { db.RegisterModel(new(RenamedBranch)) } -func GetBranch(ctx context.Context, repoID int64, branchName string) (*Branch, error) { +func getBranchWithDeleted(ctx context.Context, repoID int64, branchName string, deleted bool) (*Branch, error) { var branch Branch - has, err := db.GetEngine(ctx).Where("repo_id=?", repoID).And("name=?", branchName).Get(&branch) + has, err := db.GetEngine(ctx).Where("repo_id=?", repoID).And("name=?", branchName). + And("is_deleted=?", deleted).Get(&branch) if err != nil { return nil, err } else if !has { - return nil, ErrBranchNotExist{ - RepoID: repoID, - BranchName: branchName, - } + return nil, ErrBranchNotExist{RepoID: repoID, BranchName: branchName} } - // FIXME: this design is not right: it doesn't check `branch.IsDeleted`, it doesn't make sense to make callers to check IsDeleted again and again. - // It causes inconsistency with `GetBranches` and `git.GetBranch`, and will lead to strange bugs - // In the future, there should be 2 functions: `GetBranchExisting` and `GetBranchWithDeleted` return &branch, nil } -// IsBranchExist returns true if the branch exists in the repository. +// GetBranchExisting retrieves a branch that should exist in git repository (excluding soft-deleted ones in database) +func GetBranchExisting(ctx context.Context, repoID int64, branchName string) (*Branch, error) { + return getBranchWithDeleted(ctx, repoID, branchName, false) +} + +// IsBranchExist returns true if the branch should exist in the git repository func IsBranchExist(ctx context.Context, repoID int64, branchName string) (bool, error) { - var branch Branch - has, err := db.GetEngine(ctx).Where("repo_id=?", repoID).And("name=?", branchName).Get(&branch) - if err != nil { - return false, err - } else if !has { - return false, nil - } - return !branch.IsDeleted, nil + return db.GetEngine(ctx).Where("repo_id=?", repoID).And("name=?", branchName). + And("is_deleted=?", false).Exist(&Branch{}) } func GetBranches(ctx context.Context, repoID int64, branchNames []string, includeDeleted bool) ([]*Branch, error) { @@ -270,14 +264,6 @@ func UpdateBranch(ctx context.Context, repoID, pusherID int64, branchName string // MarkBranchAsDeleted marks branch as deleted func MarkBranchAsDeleted(ctx context.Context, repoID int64, branchName string, deletedByID int64) error { - branch, err := GetBranch(ctx, repoID, branchName) - if err != nil { - return err - } - if branch.IsDeleted { - return nil - } - cnt, err := db.GetEngine(ctx).Where("repo_id=? AND name=? AND is_deleted=?", repoID, branchName, false). Cols("is_deleted, deleted_by_id, deleted_unix"). Update(&Branch{ @@ -289,9 +275,12 @@ func MarkBranchAsDeleted(ctx context.Context, repoID int64, branchName string, d return err } if cnt == 0 { - return fmt.Errorf("branch %s not found or has been deleted", branchName) + if _, err := getBranchWithDeleted(ctx, repoID, branchName, true); err == nil { + return nil + } + return ErrBranchNotExist{RepoID: repoID, BranchName: branchName} } - return err + return nil } func RemoveDeletedBranchByID(ctx context.Context, repoID, branchID int64) error { @@ -491,19 +480,15 @@ func FindRecentlyPushedNewBranches(ctx context.Context, doer *user_model.User, o } var ignoredCommitIDs []string - baseDefaultBranch, err := GetBranch(ctx, opts.BaseRepo.ID, opts.BaseRepo.DefaultBranch) - if err != nil { - log.Warn("GetBranch:DefaultBranch: %v", err) - } else { + baseDefaultBranch, err := GetBranchExisting(ctx, opts.BaseRepo.ID, opts.BaseRepo.DefaultBranch) + if err == nil { ignoredCommitIDs = append(ignoredCommitIDs, baseDefaultBranch.CommitID) } baseDefaultTargetBranchName := opts.BaseRepo.MustGetUnit(ctx, unit.TypePullRequests).PullRequestsConfig().DefaultTargetBranch if baseDefaultTargetBranchName != "" && baseDefaultTargetBranchName != opts.BaseRepo.DefaultBranch { - baseDefaultTargetBranch, err := GetBranch(ctx, opts.BaseRepo.ID, baseDefaultTargetBranchName) - if err != nil { - log.Warn("GetBranch:DefaultTargetBranch: %v", err) - } else { + baseDefaultTargetBranch, err := GetBranchExisting(ctx, opts.BaseRepo.ID, baseDefaultTargetBranchName) + if err == nil { ignoredCommitIDs = append(ignoredCommitIDs, baseDefaultTargetBranch.CommitID) } } diff --git a/modules/repository/branch_test.go b/modules/repository/branch_test.go index e8f9bad463..069a3588ee 100644 --- a/modules/repository/branch_test.go +++ b/modules/repository/branch_test.go @@ -25,7 +25,7 @@ func TestSyncRepoBranches(t *testing.T) { assert.NoError(t, err) repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) assert.Equal(t, "sha1", repo.ObjectFormatName) - branch, err := git_model.GetBranch(t.Context(), 1, "master") + branch, err := git_model.GetBranchExisting(t.Context(), 1, "master") assert.NoError(t, err) assert.Equal(t, "master", branch.Name) } diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index fa6d364981..a96bb0e7f0 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -492,12 +492,8 @@ func viewSummaryBranchFromRun(ctx context.Context, run *actions_model.ActionRun, Link: run.RefLink(), } if refName.IsBranch() { - b, err := git_model.GetBranch(ctx, run.RepoID, refName.ShortName()) - if err != nil && !git_model.IsErrBranchNotExist(err) { - log.Error("GetBranch: %v", err) - } else if git_model.IsErrBranchNotExist(err) || (b != nil && b.IsDeleted) { - branch.IsDeleted = true - } + refBranchExists, _ := git_model.IsBranchExist(ctx, run.RepoID, refName.ShortName()) + branch.IsDeleted = !refBranchExists } return branch } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 12b1eca579..4d8bfeb7d1 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -173,14 +173,9 @@ func (prInfo *pullRequestViewInfo) setTemplateDataMergeTarget(ctx *context.Conte ctx.Data["BaseTarget"] = pull.BaseBranch headBranchLink := "" if pull.Flow == issues_model.PullRequestFlowGithub { - b, err := git_model.GetBranch(ctx, pull.HeadRepoID, pull.HeadBranch) - switch { - case err == nil: - if !b.IsDeleted { - headBranchLink = pull.GetHeadBranchLink(ctx) - } - case !git_model.IsErrBranchNotExist(err): - log.Error("GetBranch: %v", err) + headBranchExists, _ := git_model.IsBranchExist(ctx, pull.HeadRepoID, pull.HeadBranch) + if headBranchExists { + headBranchLink = pull.GetHeadBranchLink(ctx) } } ctx.Data["HeadBranchLink"] = headBranchLink diff --git a/routers/web/repo/view_home.go b/routers/web/repo/view_home.go index b09dbbeaac..fb3e27294e 100644 --- a/routers/web/repo/view_home.go +++ b/routers/web/repo/view_home.go @@ -34,23 +34,16 @@ func checkOutdatedBranch(ctx *context.Context) { if !(ctx.Repo.Permission.IsAdmin() || ctx.Repo.Permission.IsOwner()) { return } - - // get the head commit of the branch since ctx.Repo.CommitID is not always the head commit of `ctx.Repo.BranchName` - commit, err := ctx.Repo.GitRepo.GetBranchCommit(ctx, ctx.Repo.BranchName) - if err != nil { - log.Error("GetBranchCommitID: %v", err) - // Don't return an error page, as it can be rechecked the next time the user opens the page. + if !ctx.Repo.RefFullName.IsBranch() { return } - dbBranch, err := git_model.GetBranch(ctx, ctx.Repo.Repository.ID, ctx.Repo.BranchName) + dbBranch, err := git_model.GetBranchExisting(ctx, ctx.Repo.Repository.ID, ctx.Repo.RefFullName.ShortName()) if err != nil { - log.Error("GetBranch: %v", err) - // Don't return an error page, as it can be rechecked the next time the user opens the page. - return + return // ignore the error which can only be "not exists" } - if dbBranch.CommitID != commit.ID.String() { + if dbBranch.CommitID != ctx.Repo.CommitID { ctx.Flash.Warning(ctx.Tr("repo.error.broken_git_hook", "https://docs.gitea.com/help/faq#push-hook--webhook--actions-arent-running"), true) } } @@ -446,13 +439,13 @@ func Home(ctx *context.Context) { prepareFuncs := []func(*context.Context){ prepareClonePanel, prepareHomeSidebarRepoTopics, - checkOutdatedBranch, prepareToRenderDirOrFile(entry), prepareRecentlyPushedNewBranches, } if isTreePathRoot { prepareFuncs = append(prepareFuncs, + checkOutdatedBranch, prepareUpstreamDivergingInfo, prepareHomeSidebarLicenses, prepareHomeSidebarCitationFile(entry), diff --git a/services/actions/scoped_workflow_cache.go b/services/actions/scoped_workflow_cache.go index 6a02877ac2..7313a87369 100644 --- a/services/actions/scoped_workflow_cache.go +++ b/services/actions/scoped_workflow_cache.go @@ -38,7 +38,7 @@ func init() { // LoadParsedScopedWorkflows returns the source repo's parsed scoped workflows at its current default-branch HEAD. func LoadParsedScopedWorkflows(ctx context.Context, sourceRepo *repo_model.Repository) (sha string, parsed []*actions_module.ParsedScopedWorkflow, err error) { - branch, err := git_model.GetBranch(ctx, sourceRepo.ID, sourceRepo.DefaultBranch) + branch, err := git_model.GetBranchExisting(ctx, sourceRepo.ID, sourceRepo.DefaultBranch) if err != nil { return "", nil, fmt.Errorf("get source default branch: %w", err) } diff --git a/services/convert/pull.go b/services/convert/pull.go index cf968ee590..b5a39e17cf 100644 --- a/services/convert/pull.go +++ b/services/convert/pull.go @@ -410,7 +410,7 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs baseBranch, ok := baseBranchCache[pr.BaseBranch] if !ok { - baseBranch, err = git_model.GetBranch(ctx, baseRepo.ID, pr.BaseBranch) + baseBranch, err = git_model.GetBranchExisting(ctx, baseRepo.ID, pr.BaseBranch) if err == nil { baseBranchCache[pr.BaseBranch] = baseBranch } else if !git_model.IsErrBranchNotExist(err) { diff --git a/services/pull/check.go b/services/pull/check.go index 88a239b4ff..21b9d77ed7 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -368,7 +368,7 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com func getMergerForManuallyMergedPullRequest(ctx context.Context, pr *issues_model.PullRequest) (*user_model.User, error) { var errs []error - if branch, err := git_model.GetBranch(ctx, pr.BaseRepoID, pr.BaseBranch); err != nil { + if branch, err := git_model.GetBranchExisting(ctx, pr.BaseRepoID, pr.BaseBranch); err != nil { errs = append(errs, err) } else { err := branch.LoadPusher(ctx) // LoadPusher uses ghost for non-existing user diff --git a/services/repository/branch.go b/services/repository/branch.go index 666e1bf38b..7c259d5dc6 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -38,7 +38,7 @@ import ( // CreateNewBranch creates a new repository branch 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) + branch, err := git_model.GetBranchExisting(ctx, repo.ID, oldBranchName) if err != nil { return err } @@ -59,7 +59,7 @@ type Branch struct { // LoadBranches loads branches from the repository limited by page & pageSize. func LoadBranches(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, isDeletedBranch optional.Option[bool], keyword string, page, pageSize int) (defaultBranchOptional *Branch, _ []*Branch, _ int64, _ error) { - defaultDBBranchOptional, err := git_model.GetBranch(ctx, repo.ID, repo.DefaultBranch) + defaultDBBranchOptional, err := git_model.GetBranchExisting(ctx, repo.ID, repo.DefaultBranch) if err != nil && !errors.Is(err, util.ErrNotExist) { return nil, nil, 0, err } @@ -411,7 +411,7 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m return "target_exist", nil } - fromBranch, err := git_model.GetBranch(ctx, repo.ID, from) + fromBranch, err := git_model.GetBranchExisting(ctx, repo.ID, from) if err != nil { if git_model.IsErrBranchNotExist(err) { return "from_not_exist", nil @@ -492,15 +492,10 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m // UpdateBranch moves a branch reference to the provided commit. permission check should be done before calling this function. func UpdateBranch(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, doer *user_model.User, branchName, newCommitID, expectedOldCommitID string, force bool) error { - branch, err := git_model.GetBranch(ctx, repo.ID, branchName) + branch, err := git_model.GetBranchExisting(ctx, repo.ID, branchName) if err != nil { return err } - if branch.IsDeleted { - return git_model.ErrBranchNotExist{ - BranchName: branchName, - } - } if expectedOldCommitID != "" { expectedID, err := gitRepo.ConvertToGitID(ctx, expectedOldCommitID) @@ -764,24 +759,14 @@ type BranchDivergingInfo struct { // GetBranchDivergingInfo returns the information about the divergence of a patch branch to the base branch. func GetBranchDivergingInfo(ctx reqctx.RequestContext, baseRepo *repo_model.Repository, baseBranch string, headRepo *repo_model.Repository, headBranch string) (*BranchDivergingInfo, error) { - headGitBranch, err := git_model.GetBranch(ctx, headRepo.ID, headBranch) + headGitBranch, err := git_model.GetBranchExisting(ctx, headRepo.ID, headBranch) if err != nil { return nil, err } - if headGitBranch.IsDeleted { - return nil, git_model.ErrBranchNotExist{ - BranchName: headBranch, - } - } - baseGitBranch, err := git_model.GetBranch(ctx, baseRepo.ID, baseBranch) + baseGitBranch, err := git_model.GetBranchExisting(ctx, baseRepo.ID, baseBranch) if err != nil { return nil, err } - if baseGitBranch.IsDeleted { - return nil, git_model.ErrBranchNotExist{ - BranchName: baseBranch, - } - } info := &BranchDivergingInfo{} if headGitBranch.CommitID == baseGitBranch.CommitID { diff --git a/tests/integration/actions_schedule_test.go b/tests/integration/actions_schedule_test.go index 8508d87c60..67e1829c0d 100644 --- a/tests/integration/actions_schedule_test.go +++ b/tests/integration/actions_schedule_test.go @@ -42,7 +42,7 @@ func testScheduleUpdatePush(t *testing.T) { doTestScheduleUpdate(t, func(t *testing.T, u *url.URL, testContext APITestContext, user *user_model.User, repo *repo_model.Repository) (commitID, expectedSpec string) { newCron := "30 5 * * 1,3" pushScheduleChange(t, u, repo, newCron) - branch, err := git_model.GetBranch(t.Context(), repo.ID, repo.DefaultBranch) + branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) assert.NoError(t, err) return branch.CommitID, newCron }) @@ -187,13 +187,13 @@ func testScheduleUpdateMirrorSync(t *testing.T) { // update remote repo newCron := "30 5,17 * * 2,4" pushScheduleChange(t, u, repo, newCron) - repoDefaultBranch, err := git_model.GetBranch(t.Context(), repo.ID, repo.DefaultBranch) + repoDefaultBranch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) assert.NoError(t, err) // sync ok := mirror_service.SyncPullMirror(t.Context(), mirrorRepo.ID) assert.True(t, ok) - mirrorRepoDefaultBranch, err := git_model.GetBranch(t.Context(), mirrorRepo.ID, mirrorRepo.DefaultBranch) + mirrorRepoDefaultBranch, err := git_model.GetBranchExisting(t.Context(), mirrorRepo.ID, mirrorRepo.DefaultBranch) assert.NoError(t, err) assert.Equal(t, repoDefaultBranch.CommitID, mirrorRepoDefaultBranch.CommitID) @@ -215,7 +215,7 @@ func testScheduleUpdateArchiveAndUnarchive(t *testing.T) { doAPIEditRepository(testContext, &api.EditRepoOption{ Archived: new(false), })(t) - branch, err := git_model.GetBranch(t.Context(), repo.ID, repo.DefaultBranch) + branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) assert.NoError(t, err) return branch.CommitID, "@every 1m" }) @@ -230,7 +230,7 @@ func testScheduleUpdateDisableAndEnableActionsUnit(t *testing.T) { doAPIEditRepository(testContext, &api.EditRepoOption{ HasActions: new(true), })(t) - branch, err := git_model.GetBranch(t.Context(), repo.ID, repo.DefaultBranch) + branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) assert.NoError(t, err) return branch.CommitID, "@every 1m" }) diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index aa192490d5..5660e46cf7 100644 --- a/tests/integration/actions_trigger_test.go +++ b/tests/integration/actions_trigger_test.go @@ -480,7 +480,7 @@ jobs: gitRepo, err := git.OpenRepository(repo) assert.NoError(t, err) defer gitRepo.Close() - branch, err := git_model.GetBranch(t.Context(), repo.ID, repo.DefaultBranch) + branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) assert.NoError(t, err) // create a branch @@ -964,7 +964,7 @@ jobs: gitRepo, err := git.OpenRepository(repo) assert.NoError(t, err) defer gitRepo.Close() - branch, err := git_model.GetBranch(t.Context(), repo.ID, repo.DefaultBranch) + branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) assert.NoError(t, err) values := url.Values{} values.Set("ref", "main") @@ -1135,7 +1135,7 @@ jobs: gitRepo, err := git.OpenRepository(repo) assert.NoError(t, err) defer gitRepo.Close() - branch, err := git_model.GetBranch(t.Context(), repo.ID, repo.DefaultBranch) + branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) assert.NoError(t, err) values := url.Values{} values.Set("ref", "main") @@ -1226,7 +1226,7 @@ jobs: gitRepo, err := git.OpenRepository(repo) assert.NoError(t, err) defer gitRepo.Close() - branch, err := git_model.GetBranch(t.Context(), repo.ID, repo.DefaultBranch) + branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) assert.NoError(t, err) inputs := &api.CreateActionWorkflowDispatch{ Ref: "main", @@ -1312,7 +1312,7 @@ jobs: gitRepo, err := git.OpenRepository(repo) assert.NoError(t, err) defer gitRepo.Close() - branch, err := git_model.GetBranch(t.Context(), repo.ID, repo.DefaultBranch) + branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) assert.NoError(t, err) inputs := &api.CreateActionWorkflowDispatch{ Ref: "main", @@ -1640,7 +1640,7 @@ jobs: gitRepo, err := git.OpenRepository(repo) assert.NoError(t, err) defer gitRepo.Close() - branch, err := git_model.GetBranch(t.Context(), repo.ID, repo.DefaultBranch) + branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) assert.NoError(t, err) inputs = &api.CreateActionWorkflowDispatch{ Ref: "main", @@ -1811,7 +1811,7 @@ jobs: defer gitRepo.Close() // Get the commit ID of the default branch - branch, err := git_model.GetBranch(t.Context(), repo.ID, repo.DefaultBranch) + branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) assert.NoError(t, err) // create a branch @@ -1889,7 +1889,7 @@ jobs: defer gitRepo.Close() // Get the commit ID of the default branch - branch, err := git_model.GetBranch(t.Context(), repo.ID, repo.DefaultBranch) + branch, err := git_model.GetBranchExisting(t.Context(), repo.ID, repo.DefaultBranch) assert.NoError(t, err) // create a branch diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 57a766cfd2..16d6640298 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -242,7 +242,7 @@ func TestPullSquashWithHeadCommitID(t *testing.T) { resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title") repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"}) - headBranch, err := git_model.GetBranch(t.Context(), repo1.ID, "master") + headBranch, err := git_model.GetBranchExisting(t.Context(), repo1.ID, "master") assert.NoError(t, err) assert.NotNil(t, headBranch)