Improve checkBranchName (#17901)

The current implementation of checkBranchName is highly inefficient
involving opening the repository, the listing all of the branch names
checking them individually before then using using opened repo to get
the tags.

This PR avoids this by simply walking the references from show-ref
instead of opening the repository (in the nogogit case).

Signed-off-by: Andrew Thornton <art27@cantab.net>
This commit is contained in:
zeripath 2021-12-08 19:08:16 +00:00 committed by GitHub
parent b59875aa12
commit 9e6e1dc950
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 106 additions and 58 deletions

View file

@ -584,7 +584,7 @@ func RepoAssignment(ctx *Context) (cancel context.CancelFunc) {
} }
ctx.Data["Tags"] = tags ctx.Data["Tags"] = tags
brs, _, err := ctx.Repo.GitRepo.GetBranches(0, 0) brs, _, err := ctx.Repo.GitRepo.GetBranchNames(0, 0)
if err != nil { if err != nil {
ctx.ServerError("GetBranches", err) ctx.ServerError("GetBranches", err)
return return
@ -810,7 +810,7 @@ func RepoRefByType(refType RepoRefType, ignoreNotExistErr ...bool) func(*Context
if len(ctx.Params("*")) == 0 { if len(ctx.Params("*")) == 0 {
refName = ctx.Repo.Repository.DefaultBranch refName = ctx.Repo.Repository.DefaultBranch
if !ctx.Repo.GitRepo.IsBranchExist(refName) { if !ctx.Repo.GitRepo.IsBranchExist(refName) {
brs, _, err := ctx.Repo.GitRepo.GetBranches(0, 0) brs, _, err := ctx.Repo.GitRepo.GetBranchNames(0, 0)
if err != nil { if err != nil {
ctx.ServerError("GetBranches", err) ctx.ServerError("GetBranches", err)
return return

View file

@ -95,7 +95,12 @@ func GetBranchesByPath(path string, skip, limit int) ([]*Branch, int, error) {
} }
defer gitRepo.Close() defer gitRepo.Close()
brs, countAll, err := gitRepo.GetBranches(skip, limit) return gitRepo.GetBranches(skip, limit)
}
// 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 { if err != nil {
return nil, 0, err return nil, 0, err
} }
@ -103,9 +108,9 @@ func GetBranchesByPath(path string, skip, limit int) ([]*Branch, int, error) {
branches := make([]*Branch, len(brs)) branches := make([]*Branch, len(brs))
for i := range brs { for i := range brs {
branches[i] = &Branch{ branches[i] = &Branch{
Path: path, Path: repo.Path,
Name: brs[i], Name: brs[i],
gitRepo: gitRepo, gitRepo: repo,
} }
} }

View file

@ -9,6 +9,7 @@
package git package git
import ( import (
"context"
"strings" "strings"
"github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing"
@ -52,7 +53,7 @@ func (repo *Repository) IsBranchExist(name string) bool {
// GetBranches returns branches from the repository, skipping skip initial branches and // GetBranches returns branches from the repository, skipping skip initial branches and
// returning at most limit branches, or all branches if limit is 0. // returning at most limit branches, or all branches if limit is 0.
func (repo *Repository) GetBranches(skip, limit int) ([]string, int, error) { func (repo *Repository) GetBranchNames(skip, limit int) ([]string, int, error) {
var branchNames []string var branchNames []string
branches, err := repo.gogitRepo.Branches() branches, err := repo.gogitRepo.Branches()
@ -79,3 +80,26 @@ func (repo *Repository) GetBranches(skip, limit int) ([]string, int, error) {
return branchNames, count, nil return branchNames, count, nil
} }
// WalkReferences walks all the references from the repository
func WalkReferences(ctx context.Context, repoPath string, walkfn func(string) error) (int, error) {
repo, err := OpenRepositoryCtx(ctx, repoPath)
if err != nil {
return 0, err
}
defer repo.Close()
i := 0
iter, err := repo.gogitRepo.References()
if err != nil {
return i, err
}
defer iter.Close()
err = iter.ForEach(func(ref *plumbing.Reference) error {
err := walkfn(string(ref.Name()))
i++
return err
})
return i, err
}

View file

@ -61,14 +61,29 @@ func (repo *Repository) IsBranchExist(name string) bool {
return repo.IsReferenceExist(BranchPrefix + name) return repo.IsReferenceExist(BranchPrefix + name)
} }
// GetBranches returns branches from the repository, skipping skip initial branches and // GetBranchNames returns branches from the repository, skipping skip initial branches and
// returning at most limit branches, or all branches if limit is 0. // returning at most limit branches, or all branches if limit is 0.
func (repo *Repository) GetBranches(skip, limit int) ([]string, int, error) { func (repo *Repository) GetBranchNames(skip, limit int) ([]string, int, error) {
return callShowRef(repo.Ctx, repo.Path, BranchPrefix, "--heads", skip, limit) return callShowRef(repo.Ctx, repo.Path, BranchPrefix, "--heads", skip, limit)
} }
// WalkReferences walks all the references from the repository
func WalkReferences(ctx context.Context, repoPath string, walkfn func(string) error) (int, error) {
return walkShowRef(ctx, repoPath, "", 0, 0, walkfn)
}
// callShowRef return refs, if limit = 0 it will not limit // callShowRef return refs, if limit = 0 it will not limit
func callShowRef(ctx context.Context, repoPath, prefix, arg string, skip, limit int) (branchNames []string, countAll int, err error) { func callShowRef(ctx context.Context, repoPath, prefix, arg string, skip, limit int) (branchNames []string, countAll int, err error) {
countAll, err = walkShowRef(ctx, repoPath, arg, skip, limit, func(branchName string) error {
branchName = strings.TrimPrefix(branchName, prefix)
branchNames = append(branchNames, branchName)
return nil
})
return
}
func walkShowRef(ctx context.Context, repoPath, arg string, skip, limit int, walkfn func(string) error) (countAll int, err error) {
stdoutReader, stdoutWriter := io.Pipe() stdoutReader, stdoutWriter := io.Pipe()
defer func() { defer func() {
_ = stdoutReader.Close() _ = stdoutReader.Close()
@ -77,7 +92,11 @@ func callShowRef(ctx context.Context, repoPath, prefix, arg string, skip, limit
go func() { go func() {
stderrBuilder := &strings.Builder{} stderrBuilder := &strings.Builder{}
err := NewCommandContext(ctx, "show-ref", arg).RunInDirPipeline(repoPath, stdoutWriter, stderrBuilder) args := []string{"show-ref"}
if arg != "" {
args = append(args, arg)
}
err := NewCommandContext(ctx, args...).RunInDirPipeline(repoPath, stdoutWriter, stderrBuilder)
if err != nil { if err != nil {
if stderrBuilder.Len() == 0 { if stderrBuilder.Len() == 0 {
_ = stdoutWriter.Close() _ = stdoutWriter.Close()
@ -94,10 +113,10 @@ func callShowRef(ctx context.Context, repoPath, prefix, arg string, skip, limit
for i < skip { for i < skip {
_, isPrefix, err := bufReader.ReadLine() _, isPrefix, err := bufReader.ReadLine()
if err == io.EOF { if err == io.EOF {
return branchNames, i, nil return i, nil
} }
if err != nil { if err != nil {
return nil, 0, err return 0, err
} }
if !isPrefix { if !isPrefix {
i++ i++
@ -112,39 +131,42 @@ func callShowRef(ctx context.Context, repoPath, prefix, arg string, skip, limit
_, err = bufReader.ReadSlice(' ') _, err = bufReader.ReadSlice(' ')
} }
if err == io.EOF { if err == io.EOF {
return branchNames, i, nil return i, nil
} }
if err != nil { if err != nil {
return nil, 0, err return 0, err
} }
branchName, err := bufReader.ReadString('\n') branchName, err := bufReader.ReadString('\n')
if err == io.EOF { if err == io.EOF {
// This shouldn't happen... but we'll tolerate it for the sake of peace // This shouldn't happen... but we'll tolerate it for the sake of peace
return branchNames, i, nil return i, nil
} }
if err != nil { if err != nil {
return nil, i, err return i, err
} }
branchName = strings.TrimPrefix(branchName, prefix)
if len(branchName) > 0 { if len(branchName) > 0 {
branchName = branchName[:len(branchName)-1] branchName = branchName[:len(branchName)-1]
} }
branchNames = append(branchNames, branchName) err = walkfn(branchName)
if err != nil {
return i, err
}
i++ i++
} }
// count all refs // count all refs
for limit != 0 { for limit != 0 {
_, isPrefix, err := bufReader.ReadLine() _, isPrefix, err := bufReader.ReadLine()
if err == io.EOF { if err == io.EOF {
return branchNames, i, nil return i, nil
} }
if err != nil { if err != nil {
return nil, 0, err return 0, err
} }
if !isPrefix { if !isPrefix {
i++ i++
} }
} }
return branchNames, i, nil return i, nil
} }

View file

@ -17,21 +17,21 @@ func TestRepository_GetBranches(t *testing.T) {
assert.NoError(t, err) assert.NoError(t, err)
defer bareRepo1.Close() defer bareRepo1.Close()
branches, countAll, err := bareRepo1.GetBranches(0, 2) branches, countAll, err := bareRepo1.GetBranchNames(0, 2)
assert.NoError(t, err) assert.NoError(t, err)
assert.Len(t, branches, 2) assert.Len(t, branches, 2)
assert.EqualValues(t, 3, countAll) assert.EqualValues(t, 3, countAll)
assert.ElementsMatch(t, []string{"branch1", "branch2"}, branches) assert.ElementsMatch(t, []string{"branch1", "branch2"}, branches)
branches, countAll, err = bareRepo1.GetBranches(0, 0) branches, countAll, err = bareRepo1.GetBranchNames(0, 0)
assert.NoError(t, err) assert.NoError(t, err)
assert.Len(t, branches, 3) assert.Len(t, branches, 3)
assert.EqualValues(t, 3, countAll) assert.EqualValues(t, 3, countAll)
assert.ElementsMatch(t, []string{"branch1", "branch2", "master"}, branches) assert.ElementsMatch(t, []string{"branch1", "branch2", "master"}, branches)
branches, countAll, err = bareRepo1.GetBranches(5, 1) branches, countAll, err = bareRepo1.GetBranchNames(5, 1)
assert.NoError(t, err) assert.NoError(t, err)
assert.Len(t, branches, 0) assert.Len(t, branches, 0)
@ -48,7 +48,7 @@ func BenchmarkRepository_GetBranches(b *testing.B) {
defer bareRepo1.Close() defer bareRepo1.Close()
for i := 0; i < b.N; i++ { for i := 0; i < b.N; i++ {
_, _, err := bareRepo1.GetBranches(0, 0) _, _, err := bareRepo1.GetBranchNames(0, 0)
if err != nil { if err != nil {
b.Fatal(err) b.Fatal(err)
} }

View file

@ -165,14 +165,14 @@ func redirect(ctx *context.Context) {
// loadBranches loads branches from the repository limited by page & pageSize. // loadBranches loads branches from the repository limited by page & pageSize.
// NOTE: May write to context on error. // NOTE: May write to context on error.
func loadBranches(ctx *context.Context, skip, limit int) ([]*Branch, int) { func loadBranches(ctx *context.Context, skip, limit int) ([]*Branch, int) {
defaultBranch, err := repo_service.GetBranch(ctx.Repo.Repository, ctx.Repo.Repository.DefaultBranch) defaultBranch, err := ctx.Repo.GitRepo.GetBranch(ctx.Repo.Repository.DefaultBranch)
if err != nil { if err != nil {
log.Error("loadBranches: get default branch: %v", err) log.Error("loadBranches: get default branch: %v", err)
ctx.ServerError("GetDefaultBranch", err) ctx.ServerError("GetDefaultBranch", err)
return nil, 0 return nil, 0
} }
rawBranches, totalNumOfBranches, err := repo_service.GetBranches(ctx.Repo.Repository, skip, limit) rawBranches, totalNumOfBranches, err := ctx.Repo.GitRepo.GetBranches(skip, limit)
if err != nil { if err != nil {
log.Error("GetBranches: %v", err) log.Error("GetBranches: %v", err)
ctx.ServerError("GetBranches", err) ctx.ServerError("GetBranches", err)

View file

@ -660,7 +660,7 @@ func getBranchesAndTagsForRepo(repo *models.Repository) (branches, tags []string
} }
defer gitRepo.Close() defer gitRepo.Close()
branches, _, err = gitRepo.GetBranches(0, 0) branches, _, err = gitRepo.GetBranchNames(0, 0)
if err != nil { if err != nil {
return nil, nil, err return nil, nil, err
} }
@ -711,7 +711,7 @@ func CompareDiff(ctx *context.Context) {
return return
} }
headBranches, _, err := ci.HeadGitRepo.GetBranches(0, 0) headBranches, _, err := ci.HeadGitRepo.GetBranchNames(0, 0)
if err != nil { if err != nil {
ctx.ServerError("GetBranches", err) ctx.ServerError("GetBranches", err)
return return

View file

@ -690,7 +690,7 @@ func RetrieveRepoMetas(ctx *context.Context, repo *models.Repository, isPull boo
return nil return nil
} }
brs, _, err := ctx.Repo.GitRepo.GetBranches(0, 0) brs, _, err := ctx.Repo.GitRepo.GetBranchNames(0, 0)
if err != nil { if err != nil {
ctx.ServerError("GetBranches", err) ctx.ServerError("GetBranches", err)
return nil return nil

View file

@ -142,7 +142,7 @@ func adoptRepository(ctx context.Context, repoPath string, u *user_model.User, r
repo.DefaultBranch = strings.TrimPrefix(repo.DefaultBranch, git.BranchPrefix) repo.DefaultBranch = strings.TrimPrefix(repo.DefaultBranch, git.BranchPrefix)
} }
branches, _, _ := gitRepo.GetBranches(0, 0) branches, _, _ := gitRepo.GetBranchNames(0, 0)
found := false found := false
hasDefault := false hasDefault := false
hasMaster := false hasMaster := false

View file

@ -5,8 +5,10 @@
package repository package repository
import ( import (
"context"
"errors" "errors"
"fmt" "fmt"
"strings"
"code.gitea.io/gitea/models" "code.gitea.io/gitea/models"
user_model "code.gitea.io/gitea/models/user" user_model "code.gitea.io/gitea/models/user"
@ -20,7 +22,7 @@ import (
// CreateNewBranch creates a new repository branch // CreateNewBranch creates a new repository branch
func CreateNewBranch(doer *user_model.User, repo *models.Repository, oldBranchName, branchName string) (err error) { func CreateNewBranch(doer *user_model.User, repo *models.Repository, oldBranchName, branchName string) (err error) {
// Check if branch name can be used // Check if branch name can be used
if err := checkBranchName(repo, branchName); err != nil { if err := checkBranchName(git.DefaultContext, repo, branchName); err != nil {
return err return err
} }
@ -65,44 +67,39 @@ func GetBranches(repo *models.Repository, skip, limit int) ([]*git.Branch, int,
} }
// checkBranchName validates branch name with existing repository branches // checkBranchName validates branch name with existing repository branches
func checkBranchName(repo *models.Repository, name string) error { func checkBranchName(ctx context.Context, repo *models.Repository, name string) error {
gitRepo, err := git.OpenRepository(repo.RepoPath()) _, err := git.WalkReferences(ctx, repo.RepoPath(), func(refName string) error {
if err != nil { branchRefName := strings.TrimPrefix(refName, git.BranchPrefix)
return err switch {
} case branchRefName == name:
defer gitRepo.Close()
branches, _, err := GetBranches(repo, 0, 0)
if err != nil {
return err
}
for _, branch := range branches {
if branch.Name == name {
return models.ErrBranchAlreadyExists{ return models.ErrBranchAlreadyExists{
BranchName: branch.Name, BranchName: name,
} }
} else if (len(branch.Name) < len(name) && branch.Name+"/" == name[0:len(branch.Name)+1]) || // If branchRefName like a/b but we want to create a branch named a then we have a conflict
(len(branch.Name) > len(name) && name+"/" == branch.Name[0:len(name)+1]) { case strings.HasPrefix(branchRefName, name+"/"):
return models.ErrBranchNameConflict{ return models.ErrBranchNameConflict{
BranchName: branch.Name, BranchName: branchRefName,
} }
// Conversely if branchRefName like a but we want to create a branch named a/b then we also have a conflict
case strings.HasPrefix(name, branchRefName+"/"):
return models.ErrBranchNameConflict{
BranchName: branchRefName,
} }
} case refName == git.TagPrefix+name:
if _, err := gitRepo.GetTag(name); err == nil {
return models.ErrTagAlreadyExists{ return models.ErrTagAlreadyExists{
TagName: name, TagName: name,
} }
} }
return nil return nil
})
return err
} }
// CreateNewBranchFromCommit creates a new repository branch // CreateNewBranchFromCommit creates a new repository branch
func CreateNewBranchFromCommit(doer *user_model.User, repo *models.Repository, commit, branchName string) (err error) { func CreateNewBranchFromCommit(doer *user_model.User, repo *models.Repository, commit, branchName string) (err error) {
// Check if branch name can be used // Check if branch name can be used
if err := checkBranchName(repo, branchName); err != nil { if err := checkBranchName(git.DefaultContext, repo, branchName); err != nil {
return err return err
} }