refactor(git): clarify GetBranch behavior to make it only gets an existing branch (#38662)

`GetBranch` silently returned soft-deleted branches, contradicting
`IsBranchExist` and forcing callers to manually check `branch.IsDeleted`
everywhere.

Refactored it to `GetBranchExisting` to have a clear behavior: it only
returns the existing branch.

---------

Signed-off-by: Sudhanshu Singh <sudhanshuwriterblc@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

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{
+		return nil, ErrBranchNotExist{RepoID: repoID, BranchName: branchName}
-			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
+	return db.GetEngine(ctx).Where("repo_id=?", repoID).And("name=?", branchName).
-	has, err := db.GetEngine(ctx).Where("repo_id=?", repoID).And("name=?", branchName).Get(&branch)
+		And("is_deleted=?", false).Exist(&Branch{})
-	if err != nil {
-		return false, err
-	} else if !has {
-		return false, nil
-	}
-	return !branch.IsDeleted, nil
 }
 
 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)
+	baseDefaultBranch, err := GetBranchExisting(ctx, opts.BaseRepo.ID, opts.BaseRepo.DefaultBranch)
-	if err != nil {
+	if err == nil {
-		log.Warn("GetBranch:DefaultBranch: %v", err)
-	} else {
 		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)
+		baseDefaultTargetBranch, err := GetBranchExisting(ctx, opts.BaseRepo.ID, baseDefaultTargetBranchName)
-		if err != nil {
+		if err == nil {
-			log.Warn("GetBranch:DefaultTargetBranch: %v", err)
-		} else {
 			ignoredCommitIDs = append(ignoredCommitIDs, baseDefaultTargetBranch.CommitID)
 		}
 	}

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)
 }

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())
+		refBranchExists, _ := git_model.IsBranchExist(ctx, run.RepoID, refName.ShortName())
-		if err != nil && !git_model.IsErrBranchNotExist(err) {
+		branch.IsDeleted = !refBranchExists
-			log.Error("GetBranch: %v", err)
-		} else if git_model.IsErrBranchNotExist(err) || (b != nil && b.IsDeleted) {
-			branch.IsDeleted = true
-		}
 	}
 	return branch
 }

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)
+		headBranchExists, _ := git_model.IsBranchExist(ctx, pull.HeadRepoID, pull.HeadBranch)
-		switch {
+		if headBranchExists {
-		case err == nil:
+			headBranchLink = pull.GetHeadBranchLink(ctx)
-			if !b.IsDeleted {
-				headBranchLink = pull.GetHeadBranchLink(ctx)
-			}
-		case !git_model.IsErrBranchNotExist(err):
-			log.Error("GetBranch: %v", err)
 		}
 	}
 	ctx.Data["HeadBranchLink"] = headBranchLink

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
 	}
-
+	if !ctx.Repo.RefFullName.IsBranch() {
-	// 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.
 		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)
+		return // ignore the error which can only be "not exists"
-		// Don't return an error page, as it can be rechecked the next time the user opens the page.
-		return
 	}
 
-	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),

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)
 	}

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) {

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

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 {
+	baseGitBranch, err := git_model.GetBranchExisting(ctx, baseRepo.ID, baseBranch)
-		return nil, git_model.ErrBranchNotExist{
-			BranchName: headBranch,
-		}
-	}
-	baseGitBranch, err := git_model.GetBranch(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 {

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"
 	})

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

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)