Refactor Branch struct in package modules/git (#33980)

The `Branch` struct in `modules/git` package is unnecessary. We can just
use a `string` to represent a branch

models/fixtures/branch.yml

@@ -93,3 +93,111 @@
   is_deleted: false
   deleted_by_id: 0
   deleted_unix: 0
+
+-
+  id: 16
+  repo_id: 16
+  name: 'master'
+  commit_id: '69554a64c1e6030f051e5c3f94bfbd773cd6a324'
+  commit_message: 'not signed commit'
+  commit_time: 1502042309
+  pusher_id: 2
+  is_deleted: false
+  deleted_by_id: 0
+  deleted_unix: 0
+
+-
+  id: 17
+  repo_id: 16
+  name: 'not-signed'
+  commit_id: '69554a64c1e6030f051e5c3f94bfbd773cd6a324'
+  commit_message: 'not signed commit'
+  commit_time: 1502042309
+  pusher_id: 2
+  is_deleted: false
+  deleted_by_id: 0
+  deleted_unix: 0
+
+-
+  id: 18
+  repo_id: 16
+  name: 'good-sign-not-yet-validated'
+  commit_id: '27566bd5738fc8b4e3fef3c5e72cce608537bd95'
+  commit_message: 'good signed commit (with not yet validated email)'
+  commit_time: 1502042234
+  pusher_id: 2
+  is_deleted: false
+  deleted_by_id: 0
+  deleted_unix: 0
+
+-
+  id: 19
+  repo_id: 16
+  name: 'good-sign'
+  commit_id: 'f27c2b2b03dcab38beaf89b0ab4ff61f6de63441'
+  commit_message: 'good signed commit'
+  commit_time: 1502042101
+  pusher_id: 2
+  is_deleted: false
+  deleted_by_id: 0
+  deleted_unix: 0
+
+-
+  id: 20
+  repo_id: 1
+  name: 'feature/1'
+  commit_id: '65f1bf27bc3bf70f64657658635e66094edbcb4d'
+  commit_message: 'Initial commit'
+  commit_time: 1489950479
+  pusher_id: 2
+  is_deleted: false
+  deleted_by_id: 0
+  deleted_unix: 0
+
+-
+  id: 21
+  repo_id: 49
+  name: 'master'
+  commit_id: 'aacbdfe9e1c4b47f60abe81849045fa4e96f1d75'
+  commit_message: "Add 'test/test.txt'"
+  commit_time: 1572535577
+  pusher_id: 2
+  is_deleted: false
+  deleted_by_id: 0
+  deleted_unix: 0
+
+-
+  id: 22
+  repo_id: 1
+  name: 'develop'
+  commit_id: '65f1bf27bc3bf70f64657658635e66094edbcb4d'
+  commit_message: "Initial commit"
+  commit_time: 1489927679
+  pusher_id: 1
+  is_deleted: false
+  deleted_by_id: 0
+  deleted_unix: 0
+
+-
+  id: 23
+  repo_id: 3
+  name: 'master'
+  commit_id: '2a47ca4b614a9f5a43abbd5ad851a54a616ffee6'
+  commit_message: "init project"
+  commit_time: 1497448461
+  pusher_id: 1
+  is_deleted: false
+  deleted_by_id: 0
+  deleted_unix: 0
+
+-
+  id: 24
+  repo_id: 3
+  name: 'test_branch'
+  commit_id: 'd22b4d4daa5be07329fcef6ed458f00cf3392da0'
+  commit_message: "test commit"
+  commit_time: 1602935385
+  pusher_id: 1
+  is_deleted: false
+  deleted_by_id: 0
+  deleted_unix: 0

modules/git/repo_branch.go

@@ -7,7 +7,6 @@ package git
 import (
 	"context"
 	"errors"
-	"fmt"
 	"strings"
 )
 
@@ -25,36 +24,6 @@ func IsBranchExist(ctx context.Context, repoPath, name string) bool {
 	return IsReferenceExist(ctx, repoPath, BranchPrefix+name)
 }
 
-// Branch represents a Git branch.
-type Branch struct {
-	Name string
-	Path string
-
-	gitRepo *Repository
-}
-
-// GetHEADBranch returns corresponding branch of HEAD.
-func (repo *Repository) GetHEADBranch() (*Branch, error) {
-	if repo == nil {
-		return nil, errors.New("nil repo")
-	}
-	stdout, _, err := NewCommand("symbolic-ref", "HEAD").RunStdString(repo.Ctx, &RunOpts{Dir: repo.Path})
-	if err != nil {
-		return nil, err
-	}
-	stdout = strings.TrimSpace(stdout)
-
-	if !strings.HasPrefix(stdout, BranchPrefix) {
-		return nil, fmt.Errorf("invalid HEAD branch: %v", stdout)
-	}
-
-	return &Branch{
-		Name:    stdout[len(BranchPrefix):],
-		Path:    stdout,
-		gitRepo: repo,
-	}, nil
-}
-
 func GetDefaultBranch(ctx context.Context, repoPath string) (string, error) {
 	stdout, _, err := NewCommand("symbolic-ref", "HEAD").RunStdString(ctx, &RunOpts{Dir: repoPath})
 	if err != nil {
@@ -67,37 +36,6 @@ func GetDefaultBranch(ctx context.Context, repoPath string) (string, error) {
 	return strings.TrimPrefix(stdout, BranchPrefix), nil
 }
 
-// GetBranch returns a branch by it's name
-func (repo *Repository) GetBranch(branch string) (*Branch, error) {
-	if !repo.IsBranchExist(branch) {
-		return nil, ErrBranchNotExist{branch}
-	}
-	return &Branch{
-		Path:    repo.Path,
-		Name:    branch,
-		gitRepo: repo,
-	}, nil
-}
-
-// GetBranches returns a slice of *git.Branch
-func (repo *Repository) GetBranches(skip, limit int) ([]*Branch, int, error) {
-	brs, countAll, err := repo.GetBranchNames(skip, limit)
-	if err != nil {
-		return nil, 0, err
-	}
-
-	branches := make([]*Branch, len(brs))
-	for i := range brs {
-		branches[i] = &Branch{
-			Path:    repo.Path,
-			Name:    brs[i],
-			gitRepo: repo,
-		}
-	}
-
-	return branches, countAll, nil
-}
-
 // DeleteBranchOptions Option(s) for delete branch
 type DeleteBranchOptions struct {
 	Force bool
@@ -147,11 +85,6 @@ func (repo *Repository) RemoveRemote(name string) error {
 	return err
 }
 
-// GetCommit returns the head commit of a branch
-func (branch *Branch) GetCommit() (*Commit, error) {
-	return branch.gitRepo.GetBranchCommit(branch.Name)
-}
-
 // RenameBranch rename a branch
 func (repo *Repository) RenameBranch(from, to string) error {
 	_, _, err := NewCommand("branch", "-m").AddDynamicArguments(from, to).RunStdString(repo.Ctx, &RunOpts{Dir: repo.Path})

modules/gitrepo/branch.go

@@ -11,14 +11,14 @@ import (
 
 // GetBranchesByPath returns a branch by its path
 // if limit = 0 it will not limit
-func GetBranchesByPath(ctx context.Context, repo Repository, skip, limit int) ([]*git.Branch, int, error) {
+func GetBranchesByPath(ctx context.Context, repo Repository, skip, limit int) ([]string, int, error) {
 	gitRepo, err := OpenRepository(ctx, repo)
 	if err != nil {
 		return nil, 0, err
 	}
 	defer gitRepo.Close()
 
-	return gitRepo.GetBranches(skip, limit)
+	return gitRepo.GetBranchNames(skip, limit)
 }
 
 func GetBranchCommitID(ctx context.Context, repo Repository, branch string) (string, error) {

routers/api/v1/repo/branch.go

@@ -59,17 +59,16 @@ func GetBranch(ctx *context.APIContext) {
 
 	branchName := ctx.PathParam("*")
 
-	branch, err := ctx.Repo.GitRepo.GetBranch(branchName)
+	exist, err := git_model.IsBranchExist(ctx, ctx.Repo.Repository.ID, branchName)
 	if err != nil {
-		if git.IsErrBranchNotExist(err) {
-			ctx.APIErrorNotFound(err)
-		} else {
 		ctx.APIErrorInternal(err)
-		}
+		return
+	} else if !exist {
+		ctx.APIErrorNotFound(err)
 		return
 	}
 
-	c, err := branch.GetCommit()
+	c, err := ctx.Repo.GitRepo.GetBranchCommit(branchName)
 	if err != nil {
 		ctx.APIErrorInternal(err)
 		return
@@ -81,7 +80,7 @@ func GetBranch(ctx *context.APIContext) {
 		return
 	}
 
-	br, err := convert.ToBranch(ctx, ctx.Repo.Repository, branch.Name, c, branchProtection, ctx.Doer, ctx.Repo.IsAdmin())
+	br, err := convert.ToBranch(ctx, ctx.Repo.Repository, branchName, c, branchProtection, ctx.Doer, ctx.Repo.IsAdmin())
 	if err != nil {
 		ctx.APIErrorInternal(err)
 		return
@@ -260,25 +259,19 @@ func CreateBranch(ctx *context.APIContext) {
 		return
 	}
 
-	branch, err := ctx.Repo.GitRepo.GetBranch(opt.BranchName)
-	if err != nil {
-		ctx.APIErrorInternal(err)
-		return
-	}
-
-	commit, err := branch.GetCommit()
+	commit, err := ctx.Repo.GitRepo.GetBranchCommit(opt.BranchName)
 	if err != nil {
 		ctx.APIErrorInternal(err)
 		return
 	}
 
-	branchProtection, err := git_model.GetFirstMatchProtectedBranchRule(ctx, ctx.Repo.Repository.ID, branch.Name)
+	branchProtection, err := git_model.GetFirstMatchProtectedBranchRule(ctx, ctx.Repo.Repository.ID, opt.BranchName)
 	if err != nil {
 		ctx.APIErrorInternal(err)
 		return
 	}
 
-	br, err := convert.ToBranch(ctx, ctx.Repo.Repository, branch.Name, commit, branchProtection, ctx.Doer, ctx.Repo.IsAdmin())
+	br, err := convert.ToBranch(ctx, ctx.Repo.Repository, opt.BranchName, commit, branchProtection, ctx.Doer, ctx.Repo.IsAdmin())
 	if err != nil {
 		ctx.APIErrorInternal(err)
 		return

routers/api/v1/repo/commits.go

@@ -179,13 +179,7 @@ func GetAllCommits(ctx *context.APIContext) {
 		var baseCommit *git.Commit
 		if len(sha) == 0 {
 			// no sha supplied - use default branch
-			head, err := ctx.Repo.GitRepo.GetHEADBranch()
-			if err != nil {
-				ctx.APIErrorInternal(err)
-				return
-			}
-
-			baseCommit, err = ctx.Repo.GitRepo.GetBranchCommit(head.Name)
+			baseCommit, err = ctx.Repo.GitRepo.GetBranchCommit(ctx.Repo.Repository.DefaultBranch)
 			if err != nil {
 				ctx.APIErrorInternal(err)
 				return

routers/web/repo/editor.go

@@ -668,7 +668,7 @@ func UploadFilePost(ctx *context.Context) {
 	}
 
 	if oldBranchName != branchName {
-		if _, err := ctx.Repo.GitRepo.GetBranch(branchName); err == nil {
+		if exist, err := git_model.IsBranchExist(ctx, ctx.Repo.Repository.ID, branchName); err == nil && exist {
 			ctx.Data["Err_NewBranchName"] = true
 			ctx.RenderWithErr(ctx.Tr("repo.editor.branch_already_exists", branchName), tplUploadFile, &form)
 			return
@@ -875,12 +875,11 @@ func GetUniquePatchBranchName(ctx *context.Context) string {
 	prefix := ctx.Doer.LowerName + "-patch-"
 	for i := 1; i <= 1000; i++ {
 		branchName := fmt.Sprintf("%s%d", prefix, i)
-		if _, err := ctx.Repo.GitRepo.GetBranch(branchName); err != nil {
-			if git.IsErrBranchNotExist(err) {
-				return branchName
-			}
+		if exist, err := git_model.IsBranchExist(ctx, ctx.Repo.Repository.ID, branchName); err != nil {
 			log.Error("GetUniquePatchBranchName: %v", err)
 			return ""
+		} else if !exist {
+			return branchName
 		}
 	}
 	return ""

routers/web/repo/view_home.go

@@ -269,7 +269,7 @@ func handleRepoEmptyOrBroken(ctx *context.Context) {
 		} else if reallyEmpty {
 			showEmpty = true // the repo is really empty
 			updateContextRepoEmptyAndStatus(ctx, true, repo_model.RepositoryReady)
-		} else if branches, _, _ := ctx.Repo.GitRepo.GetBranches(0, 1); len(branches) == 0 {
+		} else if branches, _, _ := ctx.Repo.GitRepo.GetBranchNames(0, 1); len(branches) == 0 {
 			showEmpty = true // it is not really empty, but there is no branch
 			// at the moment, other repo units like "actions" are not able to handle such case,
 			// so we just mark the repo as empty to prevent from displaying these units.

services/context/repo.go

@@ -817,9 +817,9 @@ func RepoRefByType(detectRefType git.RefType) func(*Context) {
 		if reqPath == "" {
 			refShortName = ctx.Repo.Repository.DefaultBranch
 			if !gitrepo.IsBranchExist(ctx, ctx.Repo.Repository, refShortName) {
-				brs, _, err := ctx.Repo.GitRepo.GetBranches(0, 1)
+				brs, _, err := ctx.Repo.GitRepo.GetBranchNames(0, 1)
 				if err == nil && len(brs) != 0 {
-					refShortName = brs[0].Name
+					refShortName = brs[0]
 				} else if len(brs) == 0 {
 					log.Error("No branches in non-empty repository %s", ctx.Repo.GitRepo.Path)
 				} else {

services/convert/pull.go

@@ -28,8 +28,8 @@ import (
 // Optional - Merger
 func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User) *api.PullRequest {
 	var (
-		baseBranch *git.Branch
-		headBranch *git.Branch
+		baseBranch string
+		headBranch string
 		baseCommit *git.Commit
 		err        error
 	)
@@ -150,16 +150,16 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u
 	}
 	defer gitRepo.Close()
 
-	baseBranch, err = gitRepo.GetBranch(pr.BaseBranch)
-	if err != nil && !git.IsErrBranchNotExist(err) {
+	exist, err := git_model.IsBranchExist(ctx, pr.BaseRepoID, pr.BaseBranch)
+	if err != nil {
 		log.Error("GetBranch[%s]: %v", pr.BaseBranch, err)
 		return nil
 	}
 
-	if err == nil {
-		baseCommit, err = baseBranch.GetCommit()
+	if exist {
+		baseCommit, err = gitRepo.GetBranchCommit(pr.BaseBranch)
 		if err != nil && !git.IsErrNotExist(err) {
-			log.Error("GetCommit[%s]: %v", baseBranch.Name, err)
+			log.Error("GetCommit[%s]: %v", baseBranch, err)
 			return nil
 		}
 
@@ -169,13 +169,6 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u
 	}
 
 	if pr.Flow == issues_model.PullRequestFlowAGit {
-		gitRepo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo)
-		if err != nil {
-			log.Error("OpenRepository[%s]: %v", pr.GetGitRefName(), err)
-			return nil
-		}
-		defer gitRepo.Close()
-
 		apiPullRequest.Head.Sha, err = gitRepo.GetRefCommitID(pr.GetGitRefName())
 		if err != nil {
 			log.Error("GetRefCommitID[%s]: %v", pr.GetGitRefName(), err)
@@ -203,8 +196,8 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u
 		}
 		defer headGitRepo.Close()
 
-		headBranch, err = headGitRepo.GetBranch(pr.HeadBranch)
-		if err != nil && !git.IsErrBranchNotExist(err) {
+		exist, err = git_model.IsBranchExist(ctx, pr.HeadRepoID, pr.HeadBranch)
+		if err != nil {
 			log.Error("GetBranch[%s]: %v", pr.HeadBranch, err)
 			return nil
 		}
@@ -215,7 +208,7 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u
 			endCommitID   string
 		)
 
-		if git.IsErrBranchNotExist(err) {
+		if !exist {
 			headCommitID, err := headGitRepo.GetRefCommitID(apiPullRequest.Head.Ref)
 			if err != nil && !git.IsErrNotExist(err) {
 				log.Error("GetCommit[%s]: %v", pr.HeadBranch, err)
@@ -226,9 +219,9 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u
 				endCommitID = headCommitID
 			}
 		} else {
-			commit, err := headBranch.GetCommit()
+			commit, err := headGitRepo.GetBranchCommit(pr.HeadBranch)
 			if err != nil && !git.IsErrNotExist(err) {
-				log.Error("GetCommit[%s]: %v", headBranch.Name, err)
+				log.Error("GetCommit[%s]: %v", headBranch, err)
 				return nil
 			}
 			if err == nil {

services/mirror/mirror_pull.go

@@ -437,7 +437,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo
 	}
 
 	for _, branch := range branches {
-		cache.Remove(m.Repo.GetCommitsCountCacheKey(branch.Name, true))
+		cache.Remove(m.Repo.GetCommitsCountCacheKey(branch, true))
 	}
 
 	m.UpdatedUnix = timeutil.TimeStampNow()

services/pull/pull.go

@@ -763,7 +763,7 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re
 
 	var errs []error
 	for _, branch := range branches {
-		prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repo.ID, branch.Name)
+		prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repo.ID, branch)
 		if err != nil {
 			return err
 		}

services/repository/files/patch.go

@@ -12,7 +12,6 @@ import (
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/git"
-	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/util"
@@ -62,29 +61,26 @@ func (opts *ApplyDiffPatchOptions) Validate(ctx context.Context, repo *repo_mode
 		opts.NewBranch = opts.OldBranch
 	}
 
-	gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, repo)
-	if err != nil {
-		return err
-	}
-	defer closer.Close()
-
 	// oldBranch must exist for this operation
-	if _, err := gitRepo.GetBranch(opts.OldBranch); err != nil {
+	if exist, err := git_model.IsBranchExist(ctx, repo.ID, opts.OldBranch); err != nil {
 		return err
+	} else if !exist {
+		return git_model.ErrBranchNotExist{
+			BranchName: opts.OldBranch,
+		}
 	}
 	// A NewBranch can be specified for the patch to be applied to.
 	// Check to make sure the branch does not already exist, otherwise we can't proceed.
 	// If we aren't branching to a new branch, make sure user can commit to the given branch
 	if opts.NewBranch != opts.OldBranch {
-		existingBranch, err := gitRepo.GetBranch(opts.NewBranch)
-		if existingBranch != nil {
+		exist, err := git_model.IsBranchExist(ctx, repo.ID, opts.NewBranch)
+		if err != nil {
+			return err
+		} else if exist {
 			return git_model.ErrBranchAlreadyExists{
 				BranchName: opts.NewBranch,
 			}
 		}
-		if err != nil && !git.IsErrBranchNotExist(err) {
-			return err
-		}
 	} else {
 		protectedBranch, err := git_model.GetFirstMatchProtectedBranchRule(ctx, repo.ID, opts.OldBranch)
 		if err != nil {

services/repository/files/update.go

@@ -107,8 +107,13 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use
 	defer closer.Close()
 
 	// oldBranch must exist for this operation
-	if _, err := gitRepo.GetBranch(opts.OldBranch); err != nil && !repo.IsEmpty {
+	if exist, err := git_model.IsBranchExist(ctx, repo.ID, opts.OldBranch); err != nil {
 		return nil, err
+	} else if !exist && !repo.IsEmpty {
+		return nil, git_model.ErrBranchNotExist{
+			RepoID:     repo.ID,
+			BranchName: opts.OldBranch,
+		}
 	}
 
 	var treePaths []string
@@ -145,15 +150,15 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use
 	// Check to make sure the branch does not already exist, otherwise we can't proceed.
 	// If we aren't branching to a new branch, make sure user can commit to the given branch
 	if opts.NewBranch != opts.OldBranch {
-		existingBranch, err := gitRepo.GetBranch(opts.NewBranch)
-		if existingBranch != nil {
+		exist, err := git_model.IsBranchExist(ctx, repo.ID, opts.NewBranch)
+		if err != nil {
+			return nil, err
+		}
+		if exist {
 			return nil, git_model.ErrBranchAlreadyExists{
 				BranchName: opts.NewBranch,
 			}
 		}
-		if err != nil && !git.IsErrBranchNotExist(err) {
-			return nil, err
-		}
 	} else if err := VerifyBranchProtection(ctx, repo, doer, opts.OldBranch, treePaths); err != nil {
 		return nil, err
 	}

services/repository/migrate.go

@@ -142,12 +142,12 @@ func MigrateRepositoryGitData(ctx context.Context, u *user_model.User,
 	if !repo.IsEmpty {
 		if len(repo.DefaultBranch) == 0 {
 			// Try to get HEAD branch and set it as default branch.
-			headBranch, err := gitRepo.GetHEADBranch()
+			headBranchName, err := git.GetDefaultBranch(ctx, repoPath)
 			if err != nil {
 				return repo, fmt.Errorf("GetHEADBranch: %w", err)
 			}
-			if headBranch != nil {
-				repo.DefaultBranch = headBranch.Name
+			if headBranchName != "" {
+				repo.DefaultBranch = headBranchName
 			}
 		}
 

tests/integration/api_branch_test.go

@@ -303,7 +303,7 @@ func TestAPICreateBranchWithSyncBranches(t *testing.T) {
 		RepoID: 1,
 	})
 	assert.NoError(t, err)
-	assert.Len(t, branches, 4)
+	assert.Len(t, branches, 6)
 
 	// make a broke repository with no branch on database
 	_, err = db.DeleteByBean(db.DefaultContext, git_model.Branch{RepoID: 1})
@@ -320,7 +320,7 @@ func TestAPICreateBranchWithSyncBranches(t *testing.T) {
 		RepoID: 1,
 	})
 	assert.NoError(t, err)
-	assert.Len(t, branches, 5)
+	assert.Len(t, branches, 7)
 
 	branches, err = db.Find[git_model.Branch](db.DefaultContext, git_model.FindBranchOptions{
 		RepoID:  1,

tests/integration/repofiles_change_test.go

@@ -4,6 +4,7 @@
 package integration
 
 import (
+	"fmt"
 	"net/url"
 	"path/filepath"
 	"strings"
@@ -490,7 +491,7 @@ func TestChangeRepoFilesErrors(t *testing.T) {
 			filesResponse, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, doer, opts)
 			assert.Error(t, err)
 			assert.Nil(t, filesResponse)
-			expectedError := "branch does not exist [name: " + opts.OldBranch + "]"
+			expectedError := fmt.Sprintf("branch does not exist [repo_id: %d name: %s]", repo.ID, opts.OldBranch)
 			assert.EqualError(t, err, expectedError)
 		})