From 0ca13c5eaef536dade65f4587b9ce1d144111f87 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 21 Aug 2024 01:04:57 +0800 Subject: [PATCH 1/3] [PORT] Refactor the usage of batch catfile (gitea#31754) When opening a repository, it will call `ensureValidRepository` and also `CatFileBatch`. But sometimes these will not be used until repository closed. So it's a waste of CPU to invoke 3 times git command for every open repository. This PR removed all of these from `OpenRepository` but only kept checking whether the folder exists. When a batch is necessary, the necessary functions will be invoked. --- Conflict resolution: Because of the removal of go-git in (#4941) `_nogogit.go` files were either renamed or merged into the 'common' file. Git does handle the renames correctly, but for those that were merged has to be manually copied pasted over. The patch looks the same, 201 additions 90 deletions as the original patch. (cherry picked from commit c03baab678ba5b2e9d974aea147e660417f5d3f7) --- modules/git/batch.go | 46 +++++++++ modules/git/batch_reader.go | 12 +-- modules/git/blob.go | 15 ++- modules/git/commit_info.go | 5 +- modules/git/pipeline/lfs.go | 5 +- modules/git/repo_base.go | 96 ++++++++++--------- modules/git/repo_branch.go | 16 +++- modules/git/repo_commit.go | 21 +++- modules/git/repo_language_stats.go | 5 +- modules/git/repo_tag.go | 12 ++- modules/git/repo_tree.go | 5 +- modules/git/tree.go | 5 +- modules/git/tree_entry.go | 8 +- modules/indexer/code/bleve/bleve.go | 20 ++-- .../code/elasticsearch/elasticsearch.go | 20 ++-- 15 files changed, 201 insertions(+), 90 deletions(-) create mode 100644 modules/git/batch.go diff --git a/modules/git/batch.go b/modules/git/batch.go new file mode 100644 index 0000000000..3ec4f1ddcc --- /dev/null +++ b/modules/git/batch.go @@ -0,0 +1,46 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package git + +import ( + "bufio" + "context" +) + +type Batch struct { + cancel context.CancelFunc + Reader *bufio.Reader + Writer WriteCloserError +} + +func (repo *Repository) NewBatch(ctx context.Context) (*Batch, error) { + // Now because of some insanity with git cat-file not immediately failing if not run in a valid git directory we need to run git rev-parse first! + if err := ensureValidGitRepository(ctx, repo.Path); err != nil { + return nil, err + } + + var batch Batch + batch.Writer, batch.Reader, batch.cancel = catFileBatch(ctx, repo.Path) + return &batch, nil +} + +func (repo *Repository) NewBatchCheck(ctx context.Context) (*Batch, error) { + // Now because of some insanity with git cat-file not immediately failing if not run in a valid git directory we need to run git rev-parse first! + if err := ensureValidGitRepository(ctx, repo.Path); err != nil { + return nil, err + } + + var check Batch + check.Writer, check.Reader, check.cancel = catFileBatchCheck(ctx, repo.Path) + return &check, nil +} + +func (b *Batch) Close() { + if b.cancel != nil { + b.cancel() + b.Reader = nil + b.Writer = nil + b.cancel = nil + } +} diff --git a/modules/git/batch_reader.go b/modules/git/batch_reader.go index c988d6ab86..3b1a466b2e 100644 --- a/modules/git/batch_reader.go +++ b/modules/git/batch_reader.go @@ -26,10 +26,10 @@ type WriteCloserError interface { CloseWithError(err error) error } -// EnsureValidGitRepository runs git rev-parse in the repository path - thus ensuring that the repository is a valid repository. +// ensureValidGitRepository runs git rev-parse in the repository path - thus ensuring that the repository is a valid repository. // Run before opening git cat-file. // This is needed otherwise the git cat-file will hang for invalid repositories. -func EnsureValidGitRepository(ctx context.Context, repoPath string) error { +func ensureValidGitRepository(ctx context.Context, repoPath string) error { stderr := strings.Builder{} err := NewCommand(ctx, "rev-parse"). SetDescription(fmt.Sprintf("%s rev-parse [repo_path: %s]", GitExecutable, repoPath)). @@ -43,8 +43,8 @@ func EnsureValidGitRepository(ctx context.Context, repoPath string) error { return nil } -// CatFileBatchCheck opens git cat-file --batch-check in the provided repo and returns a stdin pipe, a stdout reader and cancel function -func CatFileBatchCheck(ctx context.Context, repoPath string) (WriteCloserError, *bufio.Reader, func()) { +// catFileBatchCheck opens git cat-file --batch-check in the provided repo and returns a stdin pipe, a stdout reader and cancel function +func catFileBatchCheck(ctx context.Context, repoPath string) (WriteCloserError, *bufio.Reader, func()) { batchStdinReader, batchStdinWriter := io.Pipe() batchStdoutReader, batchStdoutWriter := io.Pipe() ctx, ctxCancel := context.WithCancel(ctx) @@ -93,8 +93,8 @@ func CatFileBatchCheck(ctx context.Context, repoPath string) (WriteCloserError, return batchStdinWriter, batchReader, cancel } -// CatFileBatch opens git cat-file --batch in the provided repo and returns a stdin pipe, a stdout reader and cancel function -func CatFileBatch(ctx context.Context, repoPath string) (WriteCloserError, *bufio.Reader, func()) { +// catFileBatch opens git cat-file --batch in the provided repo and returns a stdin pipe, a stdout reader and cancel function +func catFileBatch(ctx context.Context, repoPath string) (WriteCloserError, *bufio.Reader, func()) { // We often want to feed the commits in order into cat-file --batch, followed by their trees and sub trees as necessary. // so let's create a batch stdin and stdout batchStdinReader, batchStdinWriter := io.Pipe() diff --git a/modules/git/blob.go b/modules/git/blob.go index 3de8ce8e90..2f02693428 100644 --- a/modules/git/blob.go +++ b/modules/git/blob.go @@ -28,9 +28,12 @@ type Blob struct { // DataAsync gets a ReadCloser for the contents of a blob without reading it all. // Calling the Close function on the result will discard all unread output. func (b *Blob) DataAsync() (io.ReadCloser, error) { - wr, rd, cancel := b.repo.CatFileBatch(b.repo.Ctx) + wr, rd, cancel, err := b.repo.CatFileBatch(b.repo.Ctx) + if err != nil { + return nil, err + } - _, err := wr.Write([]byte(b.ID.String() + "\n")) + _, err = wr.Write([]byte(b.ID.String() + "\n")) if err != nil { cancel() return nil, err @@ -66,9 +69,13 @@ func (b *Blob) Size() int64 { return b.size } - wr, rd, cancel := b.repo.CatFileBatchCheck(b.repo.Ctx) + wr, rd, cancel, err := b.repo.CatFileBatchCheck(b.repo.Ctx) + if err != nil { + log.Debug("error whilst reading size for %s in %s. Error: %v", b.ID.String(), b.repo.Path, err) + return 0 + } defer cancel() - _, err := wr.Write([]byte(b.ID.String() + "\n")) + _, err = wr.Write([]byte(b.ID.String() + "\n")) if err != nil { log.Debug("error whilst reading size for %s in %s. Error: %v", b.ID.String(), b.repo.Path, err) return 0 diff --git a/modules/git/commit_info.go b/modules/git/commit_info.go index a26d749320..3b34b7933a 100644 --- a/modules/git/commit_info.go +++ b/modules/git/commit_info.go @@ -129,7 +129,10 @@ func GetLastCommitForPaths(ctx context.Context, commit *Commit, treePath string, return nil, err } - batchStdinWriter, batchReader, cancel := commit.repo.CatFileBatch(ctx) + batchStdinWriter, batchReader, cancel, err := commit.repo.CatFileBatch(ctx) + if err != nil { + return nil, err + } defer cancel() commitsMap := map[string]*Commit{} diff --git a/modules/git/pipeline/lfs.go b/modules/git/pipeline/lfs.go index 55c49aaf3d..3407eb9838 100644 --- a/modules/git/pipeline/lfs.go +++ b/modules/git/pipeline/lfs.go @@ -67,7 +67,10 @@ func FindLFSFile(repo *git.Repository, objectID git.ObjectID) ([]*LFSResult, err // Next feed the commits in order into cat-file --batch, followed by their trees and sub trees as necessary. // so let's create a batch stdin and stdout - batchStdinWriter, batchReader, cancel := repo.CatFileBatch(repo.Ctx) + batchStdinWriter, batchReader, cancel, err := repo.CatFileBatch(repo.Ctx) + if err != nil { + return nil, err + } defer cancel() // We'll use a scanner for the revList because it's simpler than a bufio.Reader diff --git a/modules/git/repo_base.go b/modules/git/repo_base.go index 8c34efc2c7..5f17bc14f6 100644 --- a/modules/git/repo_base.go +++ b/modules/git/repo_base.go @@ -21,15 +21,11 @@ type Repository struct { gpgSettings *GPGSettings - batchInUse bool - batchCancel context.CancelFunc - batchReader *bufio.Reader - batchWriter WriteCloserError + batchInUse bool + batch *Batch - checkInUse bool - checkCancel context.CancelFunc - checkReader *bufio.Reader - checkWriter WriteCloserError + checkInUse bool + check *Batch Ctx context.Context LastCommitCache *LastCommitCache @@ -51,63 +47,75 @@ func OpenRepository(ctx context.Context, repoPath string) (*Repository, error) { return nil, errors.New("no such file or directory") } - // Now because of some insanity with git cat-file not immediately failing if not run in a valid git directory we need to run git rev-parse first! - if err := EnsureValidGitRepository(ctx, repoPath); err != nil { - return nil, err - } - - repo := &Repository{ + return &Repository{ Path: repoPath, tagCache: newObjectCache(), Ctx: ctx, - } - - repo.batchWriter, repo.batchReader, repo.batchCancel = CatFileBatch(ctx, repoPath) - repo.checkWriter, repo.checkReader, repo.checkCancel = CatFileBatchCheck(ctx, repoPath) - - return repo, nil + }, nil } // CatFileBatch obtains a CatFileBatch for this repository -func (repo *Repository) CatFileBatch(ctx context.Context) (WriteCloserError, *bufio.Reader, func()) { - if repo.batchCancel == nil || repo.batchInUse { - log.Debug("Opening temporary cat file batch for: %s", repo.Path) - return CatFileBatch(ctx, repo.Path) +func (repo *Repository) CatFileBatch(ctx context.Context) (WriteCloserError, *bufio.Reader, func(), error) { + if repo.batch == nil { + var err error + repo.batch, err = repo.NewBatch(ctx) + if err != nil { + return nil, nil, nil, err + } } - repo.batchInUse = true - return repo.batchWriter, repo.batchReader, func() { - repo.batchInUse = false + + if !repo.batchInUse { + repo.batchInUse = true + return repo.batch.Writer, repo.batch.Reader, func() { + repo.batchInUse = false + }, nil } + + log.Debug("Opening temporary cat file batch for: %s", repo.Path) + tempBatch, err := repo.NewBatch(ctx) + if err != nil { + return nil, nil, nil, err + } + return tempBatch.Writer, tempBatch.Reader, tempBatch.Close, nil } // CatFileBatchCheck obtains a CatFileBatchCheck for this repository -func (repo *Repository) CatFileBatchCheck(ctx context.Context) (WriteCloserError, *bufio.Reader, func()) { - if repo.checkCancel == nil || repo.checkInUse { - log.Debug("Opening temporary cat file batch-check for: %s", repo.Path) - return CatFileBatchCheck(ctx, repo.Path) +func (repo *Repository) CatFileBatchCheck(ctx context.Context) (WriteCloserError, *bufio.Reader, func(), error) { + if repo.check == nil { + var err error + repo.check, err = repo.NewBatchCheck(ctx) + if err != nil { + return nil, nil, nil, err + } } - repo.checkInUse = true - return repo.checkWriter, repo.checkReader, func() { - repo.checkInUse = false + + if !repo.checkInUse { + repo.checkInUse = true + return repo.check.Writer, repo.check.Reader, func() { + repo.checkInUse = false + }, nil } + + log.Debug("Opening temporary cat file batch-check for: %s", repo.Path) + tempBatchCheck, err := repo.NewBatchCheck(ctx) + if err != nil { + return nil, nil, nil, err + } + return tempBatchCheck.Writer, tempBatchCheck.Reader, tempBatchCheck.Close, nil } func (repo *Repository) Close() error { if repo == nil { return nil } - if repo.batchCancel != nil { - repo.batchCancel() - repo.batchReader = nil - repo.batchWriter = nil - repo.batchCancel = nil + if repo.batch != nil { + repo.batch.Close() + repo.batch = nil repo.batchInUse = false } - if repo.checkCancel != nil { - repo.checkCancel() - repo.checkCancel = nil - repo.checkReader = nil - repo.checkWriter = nil + if repo.check != nil { + repo.check.Close() + repo.check = nil repo.checkInUse = false } repo.LastCommitCache = nil diff --git a/modules/git/repo_branch.go b/modules/git/repo_branch.go index dff3a43fa3..7339c7db0d 100644 --- a/modules/git/repo_branch.go +++ b/modules/git/repo_branch.go @@ -169,9 +169,13 @@ func (repo *Repository) IsObjectExist(name string) bool { return false } - wr, rd, cancel := repo.CatFileBatchCheck(repo.Ctx) + wr, rd, cancel, err := repo.CatFileBatchCheck(repo.Ctx) + if err != nil { + log.Debug("Error writing to CatFileBatchCheck %v", err) + return false + } defer cancel() - _, err := wr.Write([]byte(name + "\n")) + _, err = wr.Write([]byte(name + "\n")) if err != nil { log.Debug("Error writing to CatFileBatchCheck %v", err) return false @@ -186,9 +190,13 @@ func (repo *Repository) IsReferenceExist(name string) bool { return false } - wr, rd, cancel := repo.CatFileBatchCheck(repo.Ctx) + wr, rd, cancel, err := repo.CatFileBatchCheck(repo.Ctx) + if err != nil { + log.Debug("Error writing to CatFileBatchCheck %v", err) + return false + } defer cancel() - _, err := wr.Write([]byte(name + "\n")) + _, err = wr.Write([]byte(name + "\n")) if err != nil { log.Debug("Error writing to CatFileBatchCheck %v", err) return false diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 2a325d3644..1f3d64fe03 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -536,9 +536,12 @@ func (repo *Repository) ResolveReference(name string) (string, error) { // GetRefCommitID returns the last commit ID string of given reference (branch or tag). func (repo *Repository) GetRefCommitID(name string) (string, error) { - wr, rd, cancel := repo.CatFileBatchCheck(repo.Ctx) + wr, rd, cancel, err := repo.CatFileBatchCheck(repo.Ctx) + if err != nil { + return "", err + } defer cancel() - _, err := wr.Write([]byte(name + "\n")) + _, err = wr.Write([]byte(name + "\n")) if err != nil { return "", err } @@ -564,12 +567,19 @@ func (repo *Repository) RemoveReference(name string) error { // IsCommitExist returns true if given commit exists in current repository. func (repo *Repository) IsCommitExist(name string) bool { + if err := ensureValidGitRepository(repo.Ctx, repo.Path); err != nil { + log.Error("IsCommitExist: %v", err) + return false + } _, _, err := NewCommand(repo.Ctx, "cat-file", "-e").AddDynamicArguments(name).RunStdString(&RunOpts{Dir: repo.Path}) return err == nil } func (repo *Repository) getCommit(id ObjectID) (*Commit, error) { - wr, rd, cancel := repo.CatFileBatch(repo.Ctx) + wr, rd, cancel, err := repo.CatFileBatch(repo.Ctx) + if err != nil { + return nil, err + } defer cancel() _, _ = wr.Write([]byte(id.String() + "\n")) @@ -646,7 +656,10 @@ func (repo *Repository) ConvertToGitID(commitID string) (ObjectID, error) { } } - wr, rd, cancel := repo.CatFileBatchCheck(repo.Ctx) + wr, rd, cancel, err := repo.CatFileBatchCheck(repo.Ctx) + if err != nil { + return nil, err + } defer cancel() _, err = wr.Write([]byte(commitID + "\n")) if err != nil { diff --git a/modules/git/repo_language_stats.go b/modules/git/repo_language_stats.go index 84638b3cef..37c23faf68 100644 --- a/modules/git/repo_language_stats.go +++ b/modules/git/repo_language_stats.go @@ -60,7 +60,10 @@ func mergeLanguageStats(stats map[string]int64) map[string]int64 { func (repo *Repository) GetLanguageStats(commitID string) (map[string]int64, error) { // We will feed the commit IDs in order into cat-file --batch, followed by blobs as necessary. // so let's create a batch stdin and stdout - batchStdinWriter, batchReader, cancel := repo.CatFileBatch(repo.Ctx) + batchStdinWriter, batchReader, cancel, err := repo.CatFileBatch(repo.Ctx) + if err != nil { + return nil, err + } defer cancel() writeID := func(id string) error { diff --git a/modules/git/repo_tag.go b/modules/git/repo_tag.go index d925d4a7d3..12b0c022cb 100644 --- a/modules/git/repo_tag.go +++ b/modules/git/repo_tag.go @@ -257,9 +257,12 @@ func (repo *Repository) GetTags(skip, limit int) (tags []string, err error) { // GetTagType gets the type of the tag, either commit (simple) or tag (annotated) func (repo *Repository) GetTagType(id ObjectID) (string, error) { - wr, rd, cancel := repo.CatFileBatchCheck(repo.Ctx) + wr, rd, cancel, err := repo.CatFileBatchCheck(repo.Ctx) + if err != nil { + return "", err + } defer cancel() - _, err := wr.Write([]byte(id.String() + "\n")) + _, err = wr.Write([]byte(id.String() + "\n")) if err != nil { return "", err } @@ -315,7 +318,10 @@ func (repo *Repository) getTag(tagID ObjectID, name string) (*Tag, error) { } // The tag is an annotated tag with a message. - wr, rd, cancel := repo.CatFileBatch(repo.Ctx) + wr, rd, cancel, err := repo.CatFileBatch(repo.Ctx) + if err != nil { + return nil, err + } defer cancel() if _, err := wr.Write([]byte(tagID.String() + "\n")); err != nil { diff --git a/modules/git/repo_tree.go b/modules/git/repo_tree.go index 79a7e50eb4..53d94d9d7d 100644 --- a/modules/git/repo_tree.go +++ b/modules/git/repo_tree.go @@ -68,7 +68,10 @@ func (repo *Repository) CommitTree(author, committer *Signature, tree *Tree, opt } func (repo *Repository) getTree(id ObjectID) (*Tree, error) { - wr, rd, cancel := repo.CatFileBatch(repo.Ctx) + wr, rd, cancel, err := repo.CatFileBatch(repo.Ctx) + if err != nil { + return nil, err + } defer cancel() _, _ = wr.Write([]byte(id.String() + "\n")) diff --git a/modules/git/tree.go b/modules/git/tree.go index 422fe68caa..5b06cbf359 100644 --- a/modules/git/tree.go +++ b/modules/git/tree.go @@ -41,7 +41,10 @@ func (t *Tree) ListEntries() (Entries, error) { } if t.repo != nil { - wr, rd, cancel := t.repo.CatFileBatch(t.repo.Ctx) + wr, rd, cancel, err := t.repo.CatFileBatch(t.repo.Ctx) + if err != nil { + return nil, err + } defer cancel() _, _ = wr.Write([]byte(t.ID.String() + "\n")) diff --git a/modules/git/tree_entry.go b/modules/git/tree_entry.go index b5dd801309..0d9cfd2258 100644 --- a/modules/git/tree_entry.go +++ b/modules/git/tree_entry.go @@ -47,9 +47,13 @@ func (te *TreeEntry) Size() int64 { return te.size } - wr, rd, cancel := te.ptree.repo.CatFileBatchCheck(te.ptree.repo.Ctx) + wr, rd, cancel, err := te.ptree.repo.CatFileBatchCheck(te.ptree.repo.Ctx) + if err != nil { + log.Debug("error whilst reading size for %s in %s. Error: %v", te.ID.String(), te.ptree.repo.Path, err) + return 0 + } defer cancel() - _, err := wr.Write([]byte(te.ID.String() + "\n")) + _, err = wr.Write([]byte(te.ID.String() + "\n")) if err != nil { log.Debug("error whilst reading size for %s in %s. Error: %v", te.ID.String(), te.ptree.repo.Path, err) return 0 diff --git a/modules/indexer/code/bleve/bleve.go b/modules/indexer/code/bleve/bleve.go index 66724a3445..cf9fcbd8b5 100644 --- a/modules/indexer/code/bleve/bleve.go +++ b/modules/indexer/code/bleve/bleve.go @@ -16,10 +16,10 @@ import ( "code.gitea.io/gitea/modules/analyze" "code.gitea.io/gitea/modules/charset" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/indexer/code/internal" indexer_internal "code.gitea.io/gitea/modules/indexer/internal" inner_bleve "code.gitea.io/gitea/modules/indexer/internal/bleve" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/typesniffer" @@ -193,21 +193,23 @@ func (b *Indexer) addDelete(filename string, repo *repo_model.Repository, batch func (b *Indexer) Index(ctx context.Context, repo *repo_model.Repository, sha string, changes *internal.RepoChanges) error { batch := inner_bleve.NewFlushingBatch(b.inner.Indexer, maxBatchSize) if len(changes.Updates) > 0 { - // Now because of some insanity with git cat-file not immediately failing if not run in a valid git directory we need to run git rev-parse first! - if err := git.EnsureValidGitRepository(ctx, repo.RepoPath()); err != nil { - log.Error("Unable to open git repo: %s for %-v: %v", repo.RepoPath(), repo, err) + r, err := gitrepo.OpenRepository(ctx, repo) + if err != nil { return err } - - batchWriter, batchReader, cancel := git.CatFileBatch(ctx, repo.RepoPath()) - defer cancel() + defer r.Close() + gitBatch, err := r.NewBatch(ctx) + if err != nil { + return err + } + defer gitBatch.Close() for _, update := range changes.Updates { - if err := b.addUpdate(ctx, batchWriter, batchReader, sha, update, repo, batch); err != nil { + if err := b.addUpdate(ctx, gitBatch.Writer, gitBatch.Reader, sha, update, repo, batch); err != nil { return err } } - cancel() + gitBatch.Close() } for _, filename := range changes.RemovedFilenames { if err := b.addDelete(filename, repo, batch); err != nil { diff --git a/modules/indexer/code/elasticsearch/elasticsearch.go b/modules/indexer/code/elasticsearch/elasticsearch.go index e4622fd66e..0bda180fac 100644 --- a/modules/indexer/code/elasticsearch/elasticsearch.go +++ b/modules/indexer/code/elasticsearch/elasticsearch.go @@ -15,11 +15,11 @@ import ( "code.gitea.io/gitea/modules/analyze" "code.gitea.io/gitea/modules/charset" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/indexer/code/internal" indexer_internal "code.gitea.io/gitea/modules/indexer/internal" inner_elasticsearch "code.gitea.io/gitea/modules/indexer/internal/elasticsearch" "code.gitea.io/gitea/modules/json" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/typesniffer" @@ -154,17 +154,19 @@ func (b *Indexer) addDelete(filename string, repo *repo_model.Repository) elasti func (b *Indexer) Index(ctx context.Context, repo *repo_model.Repository, sha string, changes *internal.RepoChanges) error { reqs := make([]elastic.BulkableRequest, 0) if len(changes.Updates) > 0 { - // Now because of some insanity with git cat-file not immediately failing if not run in a valid git directory we need to run git rev-parse first! - if err := git.EnsureValidGitRepository(ctx, repo.RepoPath()); err != nil { - log.Error("Unable to open git repo: %s for %-v: %v", repo.RepoPath(), repo, err) + r, err := gitrepo.OpenRepository(ctx, repo) + if err != nil { return err } - - batchWriter, batchReader, cancel := git.CatFileBatch(ctx, repo.RepoPath()) - defer cancel() + defer r.Close() + batch, err := r.NewBatch(ctx) + if err != nil { + return err + } + defer batch.Close() for _, update := range changes.Updates { - updateReqs, err := b.addUpdate(ctx, batchWriter, batchReader, sha, update, repo) + updateReqs, err := b.addUpdate(ctx, batch.Writer, batch.Reader, sha, update, repo) if err != nil { return err } @@ -172,7 +174,7 @@ func (b *Indexer) Index(ctx context.Context, repo *repo_model.Repository, sha st reqs = append(reqs, updateReqs...) } } - cancel() + batch.Close() } for _, filename := range changes.RemovedFilenames { From 24bbf051c3d1f1567dd8ea143ac7a4d3cafbd7b5 Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 26 Aug 2024 04:46:27 +0200 Subject: [PATCH 2/3] [TESTS] Add test for CatFileBatch(Check) --- modules/git/repo_base_test.go | 163 ++++++++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) create mode 100644 modules/git/repo_base_test.go diff --git a/modules/git/repo_base_test.go b/modules/git/repo_base_test.go new file mode 100644 index 0000000000..323b28f476 --- /dev/null +++ b/modules/git/repo_base_test.go @@ -0,0 +1,163 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package git + +import ( + "bufio" + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// This unit test relies on the implementation detail of CatFileBatch. +func TestCatFileBatch(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + repo, err := OpenRepository(ctx, "./tests/repos/repo1_bare") + require.NoError(t, err) + defer repo.Close() + + var wr WriteCloserError + var r *bufio.Reader + var cancel1 func() + t.Run("Request cat file batch", func(t *testing.T) { + assert.Nil(t, repo.batch) + wr, r, cancel1, err = repo.CatFileBatch(ctx) + require.NoError(t, err) + assert.NotNil(t, repo.batch) + assert.Equal(t, repo.batch.Writer, wr) + assert.True(t, repo.batchInUse) + }) + + t.Run("Request temporary cat file batch", func(t *testing.T) { + wr, r, cancel, err := repo.CatFileBatch(ctx) + require.NoError(t, err) + assert.NotEqual(t, repo.batch.Writer, wr) + + t.Run("Check temporary cat file batch", func(t *testing.T) { + _, err = wr.Write([]byte("95bb4d39648ee7e325106df01a621c530863a653" + "\n")) + require.NoError(t, err) + + sha, typ, size, err := ReadBatchLine(r) + require.NoError(t, err) + assert.Equal(t, "commit", typ) + assert.EqualValues(t, []byte("95bb4d39648ee7e325106df01a621c530863a653"), sha) + assert.EqualValues(t, 144, size) + }) + + cancel() + assert.True(t, repo.batchInUse) + }) + + t.Run("Check cached cat file batch", func(t *testing.T) { + _, err = wr.Write([]byte("95bb4d39648ee7e325106df01a621c530863a653" + "\n")) + require.NoError(t, err) + + sha, typ, size, err := ReadBatchLine(r) + require.NoError(t, err) + assert.Equal(t, "commit", typ) + assert.EqualValues(t, []byte("95bb4d39648ee7e325106df01a621c530863a653"), sha) + assert.EqualValues(t, 144, size) + }) + + t.Run("Cancel cached cat file batch", func(t *testing.T) { + cancel1() + assert.False(t, repo.batchInUse) + assert.NotNil(t, repo.batch) + }) + + t.Run("Request cached cat file batch", func(t *testing.T) { + wr, _, _, err := repo.CatFileBatch(ctx) + require.NoError(t, err) + assert.NotNil(t, repo.batch) + assert.Equal(t, repo.batch.Writer, wr) + assert.True(t, repo.batchInUse) + + t.Run("Close git repo", func(t *testing.T) { + require.NoError(t, repo.Close()) + assert.Nil(t, repo.batch) + }) + + _, err = wr.Write([]byte("95bb4d39648ee7e325106df01a621c530863a653" + "\n")) + require.Error(t, err) + }) +} + +// This unit test relies on the implementation detail of CatFileBatchCheck. +func TestCatFileBatchCheck(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + repo, err := OpenRepository(ctx, "./tests/repos/repo1_bare") + require.NoError(t, err) + defer repo.Close() + + var wr WriteCloserError + var r *bufio.Reader + var cancel1 func() + t.Run("Request cat file batch check", func(t *testing.T) { + assert.Nil(t, repo.check) + wr, r, cancel1, err = repo.CatFileBatchCheck(ctx) + require.NoError(t, err) + assert.NotNil(t, repo.check) + assert.Equal(t, repo.check.Writer, wr) + assert.True(t, repo.checkInUse) + }) + + t.Run("Request temporary cat file batch check", func(t *testing.T) { + wr, r, cancel, err := repo.CatFileBatchCheck(ctx) + require.NoError(t, err) + assert.NotEqual(t, repo.check.Writer, wr) + + t.Run("Check temporary cat file batch check", func(t *testing.T) { + _, err = wr.Write([]byte("test" + "\n")) + require.NoError(t, err) + + sha, typ, size, err := ReadBatchLine(r) + require.NoError(t, err) + assert.Equal(t, "tag", typ) + assert.EqualValues(t, []byte("3ad28a9149a2864384548f3d17ed7f38014c9e8a"), sha) + assert.EqualValues(t, 807, size) + }) + + cancel() + assert.True(t, repo.checkInUse) + }) + + t.Run("Check cached cat file batch check", func(t *testing.T) { + _, err = wr.Write([]byte("test" + "\n")) + require.NoError(t, err) + + sha, typ, size, err := ReadBatchLine(r) + require.NoError(t, err) + assert.Equal(t, "tag", typ) + assert.EqualValues(t, []byte("3ad28a9149a2864384548f3d17ed7f38014c9e8a"), sha) + assert.EqualValues(t, 807, size) + }) + + t.Run("Cancel cached cat file batch check", func(t *testing.T) { + cancel1() + assert.False(t, repo.checkInUse) + assert.NotNil(t, repo.check) + }) + + t.Run("Request cached cat file batch check", func(t *testing.T) { + wr, _, _, err := repo.CatFileBatchCheck(ctx) + require.NoError(t, err) + assert.NotNil(t, repo.check) + assert.Equal(t, repo.check.Writer, wr) + assert.True(t, repo.checkInUse) + + t.Run("Close git repo", func(t *testing.T) { + require.NoError(t, repo.Close()) + assert.Nil(t, repo.check) + }) + + _, err = wr.Write([]byte("test" + "\n")) + require.Error(t, err) + }) +} From b44dcf553cda964028c57cdf60ad8605d9000786 Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 26 Aug 2024 08:03:48 +0200 Subject: [PATCH 3/3] [TESTS] Fix usage of `LoadRepoCommit` It loads the Commit with a temporary open GitRepo. This is incorrect, the GitRepo should be open as long as the Commit can be used. This mainly removes the usage of this function as it's not needed. --- routers/api/v1/repo/hook_test.go | 6 +- routers/web/repo/editor_test.go | 12 +-- services/contexttest/context_tests.go | 27 ++++-- services/repository/files/content_test.go | 102 ++++++--------------- services/repository/files/diff_test.go | 19 ++-- services/repository/files/file_test.go | 16 +--- services/repository/files/tree_test.go | 1 - tests/integration/repo_view_test.go | 7 +- tests/integration/repofiles_change_test.go | 84 +++-------------- 9 files changed, 84 insertions(+), 190 deletions(-) diff --git a/routers/api/v1/repo/hook_test.go b/routers/api/v1/repo/hook_test.go index 37cf61c1ed..a8065e4a60 100644 --- a/routers/api/v1/repo/hook_test.go +++ b/routers/api/v1/repo/hook_test.go @@ -19,9 +19,11 @@ func TestTestHook(t *testing.T) { ctx, _ := contexttest.MockAPIContext(t, "user2/repo1/wiki/_pages") ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) contexttest.LoadUser(t, ctx, 2) + contexttest.LoadRepo(t, ctx, 1) + contexttest.LoadGitRepo(t, ctx) + defer ctx.Repo.GitRepo.Close() + contexttest.LoadRepoCommit(t, ctx) TestHook(ctx) assert.EqualValues(t, http.StatusNoContent, ctx.Resp.Status()) diff --git a/routers/web/repo/editor_test.go b/routers/web/repo/editor_test.go index 313fcfe33a..4d565b5fd6 100644 --- a/routers/web/repo/editor_test.go +++ b/routers/web/repo/editor_test.go @@ -6,6 +6,7 @@ package repo import ( "testing" + repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" @@ -45,7 +46,6 @@ func TestGetUniquePatchBranchName(t *testing.T) { ctx, _ := contexttest.MockContext(t, "user2/repo1") ctx.SetParams(":id", "1") contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) contexttest.LoadUser(t, ctx, 2) contexttest.LoadGitRepo(t, ctx) defer ctx.Repo.GitRepo.Close() @@ -57,15 +57,7 @@ func TestGetUniquePatchBranchName(t *testing.T) { func TestGetClosestParentWithFiles(t *testing.T) { unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() - - repo := ctx.Repo.Repository + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) branch := repo.DefaultBranch gitRepo, _ := gitrepo.OpenRepository(git.DefaultContext, repo) defer gitRepo.Close() diff --git a/services/contexttest/context_tests.go b/services/contexttest/context_tests.go index a4cc967a57..7c829f3598 100644 --- a/services/contexttest/context_tests.go +++ b/services/contexttest/context_tests.go @@ -136,14 +136,15 @@ func LoadRepoCommit(t *testing.T, ctx gocontext.Context) { assert.FailNow(t, "context is not *context.Context or *context.APIContext") } - gitRepo, err := gitrepo.OpenRepository(ctx, repo.Repository) - require.NoError(t, err) - defer gitRepo.Close() - branch, err := gitRepo.GetHEADBranch() + if repo.GitRepo == nil { + assert.FailNow(t, "must call LoadGitRepo") + } + + branch, err := repo.GitRepo.GetHEADBranch() require.NoError(t, err) assert.NotNil(t, branch) if branch != nil { - repo.Commit, err = gitRepo.GetBranchCommit(branch.Name) + repo.Commit, err = repo.GitRepo.GetBranchCommit(branch.Name) require.NoError(t, err) } } @@ -176,10 +177,20 @@ func LoadOrganization(t *testing.T, ctx gocontext.Context, orgID int64) { // LoadGitRepo load a git repo into a test context. Requires that ctx.Repo has // already been populated. -func LoadGitRepo(t *testing.T, ctx *context.Context) { - require.NoError(t, ctx.Repo.Repository.LoadOwner(ctx)) +func LoadGitRepo(t *testing.T, ctx gocontext.Context) { + var repo *context.Repository + switch ctx := ctx.(type) { + case *context.Context: + repo = ctx.Repo + case *context.APIContext: + repo = ctx.Repo + default: + assert.FailNow(t, "context is not *context.Context or *context.APIContext") + } + + require.NoError(t, repo.Repository.LoadOwner(ctx)) var err error - ctx.Repo.GitRepo, err = gitrepo.OpenRepository(ctx, ctx.Repo.Repository) + repo.GitRepo, err = gitrepo.OpenRepository(ctx, repo.Repository) require.NoError(t, err) } diff --git a/services/repository/files/content_test.go b/services/repository/files/content_test.go index 768d6d2f39..c22dcd2e8d 100644 --- a/services/repository/files/content_test.go +++ b/services/repository/files/content_test.go @@ -6,10 +6,11 @@ package files import ( "testing" + "code.gitea.io/gitea/models/db" + repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/modules/gitrepo" api "code.gitea.io/gitea/modules/structs" - "code.gitea.io/gitea/services/contexttest" _ "code.gitea.io/gitea/models/actions" @@ -53,27 +54,21 @@ func getExpectedReadmeContentsResponse() *api.ContentsResponse { func TestGetContents(t *testing.T) { unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) treePath := "README.md" - ref := ctx.Repo.Repository.DefaultBranch + ref := repo.DefaultBranch expectedContentsResponse := getExpectedReadmeContentsResponse() t.Run("Get README.md contents with GetContents(ctx, )", func(t *testing.T) { - fileContentResponse, err := GetContents(ctx, ctx.Repo.Repository, treePath, ref, false) + fileContentResponse, err := GetContents(db.DefaultContext, repo, treePath, ref, false) assert.EqualValues(t, expectedContentsResponse, fileContentResponse) require.NoError(t, err) }) t.Run("Get README.md contents with ref as empty string (should then use the repo's default branch) with GetContents(ctx, )", func(t *testing.T) { - fileContentResponse, err := GetContents(ctx, ctx.Repo.Repository, treePath, "", false) + fileContentResponse, err := GetContents(db.DefaultContext, repo, treePath, "", false) assert.EqualValues(t, expectedContentsResponse, fileContentResponse) require.NoError(t, err) }) @@ -81,16 +76,10 @@ func TestGetContents(t *testing.T) { func TestGetContentsOrListForDir(t *testing.T) { unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) treePath := "" // root dir - ref := ctx.Repo.Repository.DefaultBranch + ref := repo.DefaultBranch readmeContentsResponse := getExpectedReadmeContentsResponse() // because will be in a list, doesn't have encoding and content @@ -102,13 +91,13 @@ func TestGetContentsOrListForDir(t *testing.T) { } t.Run("Get root dir contents with GetContentsOrList(ctx, )", func(t *testing.T) { - fileContentResponse, err := GetContentsOrList(ctx, ctx.Repo.Repository, treePath, ref) + fileContentResponse, err := GetContentsOrList(db.DefaultContext, repo, treePath, ref) assert.EqualValues(t, expectedContentsListResponse, fileContentResponse) require.NoError(t, err) }) t.Run("Get root dir contents with ref as empty string (should then use the repo's default branch) with GetContentsOrList(ctx, )", func(t *testing.T) { - fileContentResponse, err := GetContentsOrList(ctx, ctx.Repo.Repository, treePath, "") + fileContentResponse, err := GetContentsOrList(db.DefaultContext, repo, treePath, "") assert.EqualValues(t, expectedContentsListResponse, fileContentResponse) require.NoError(t, err) }) @@ -116,27 +105,21 @@ func TestGetContentsOrListForDir(t *testing.T) { func TestGetContentsOrListForFile(t *testing.T) { unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) treePath := "README.md" - ref := ctx.Repo.Repository.DefaultBranch + ref := repo.DefaultBranch expectedContentsResponse := getExpectedReadmeContentsResponse() t.Run("Get README.md contents with GetContentsOrList(ctx, )", func(t *testing.T) { - fileContentResponse, err := GetContentsOrList(ctx, ctx.Repo.Repository, treePath, ref) + fileContentResponse, err := GetContentsOrList(db.DefaultContext, repo, treePath, ref) assert.EqualValues(t, expectedContentsResponse, fileContentResponse) require.NoError(t, err) }) t.Run("Get README.md contents with ref as empty string (should then use the repo's default branch) with GetContentsOrList(ctx, )", func(t *testing.T) { - fileContentResponse, err := GetContentsOrList(ctx, ctx.Repo.Repository, treePath, "") + fileContentResponse, err := GetContentsOrList(db.DefaultContext, repo, treePath, "") assert.EqualValues(t, expectedContentsResponse, fileContentResponse) require.NoError(t, err) }) @@ -144,28 +127,21 @@ func TestGetContentsOrListForFile(t *testing.T) { func TestGetContentsErrors(t *testing.T) { unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) - repo := ctx.Repo.Repository treePath := "README.md" ref := repo.DefaultBranch t.Run("bad treePath", func(t *testing.T) { badTreePath := "bad/tree.md" - fileContentResponse, err := GetContents(ctx, repo, badTreePath, ref, false) + fileContentResponse, err := GetContents(db.DefaultContext, repo, badTreePath, ref, false) require.EqualError(t, err, "object does not exist [id: , rel_path: bad]") assert.Nil(t, fileContentResponse) }) t.Run("bad ref", func(t *testing.T) { badRef := "bad_ref" - fileContentResponse, err := GetContents(ctx, repo, treePath, badRef, false) + fileContentResponse, err := GetContents(db.DefaultContext, repo, treePath, badRef, false) require.EqualError(t, err, "object does not exist [id: "+badRef+", rel_path: ]") assert.Nil(t, fileContentResponse) }) @@ -173,28 +149,21 @@ func TestGetContentsErrors(t *testing.T) { func TestGetContentsOrListErrors(t *testing.T) { unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) - repo := ctx.Repo.Repository treePath := "README.md" ref := repo.DefaultBranch t.Run("bad treePath", func(t *testing.T) { badTreePath := "bad/tree.md" - fileContentResponse, err := GetContentsOrList(ctx, repo, badTreePath, ref) + fileContentResponse, err := GetContentsOrList(db.DefaultContext, repo, badTreePath, ref) require.EqualError(t, err, "object does not exist [id: , rel_path: bad]") assert.Nil(t, fileContentResponse) }) t.Run("bad ref", func(t *testing.T) { badRef := "bad_ref" - fileContentResponse, err := GetContentsOrList(ctx, repo, treePath, badRef) + fileContentResponse, err := GetContentsOrList(db.DefaultContext, repo, treePath, badRef) require.EqualError(t, err, "object does not exist [id: "+badRef+", rel_path: ]") assert.Nil(t, fileContentResponse) }) @@ -202,17 +171,10 @@ func TestGetContentsOrListErrors(t *testing.T) { func TestGetContentsOrListOfEmptyRepos(t *testing.T) { unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "user30/empty") - ctx.SetParams(":id", "52") - contexttest.LoadRepo(t, ctx, 52) - contexttest.LoadUser(t, ctx, 30) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() - - repo := ctx.Repo.Repository + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 52}) t.Run("empty repo", func(t *testing.T) { - contents, err := GetContentsOrList(ctx, repo, "", "") + contents, err := GetContentsOrList(db.DefaultContext, repo, "", "") require.NoError(t, err) assert.Empty(t, contents) }) @@ -220,23 +182,13 @@ func TestGetContentsOrListOfEmptyRepos(t *testing.T) { func TestGetBlobBySHA(t *testing.T) { unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "user2/repo1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) - sha := "65f1bf27bc3bf70f64657658635e66094edbcb4d" - ctx.SetParams(":id", "1") - ctx.SetParams(":sha", sha) + gitRepo, err := gitrepo.OpenRepository(db.DefaultContext, repo) + require.NoError(t, err) + defer gitRepo.Close() - gitRepo, err := gitrepo.OpenRepository(ctx, ctx.Repo.Repository) - if err != nil { - t.Fail() - } - - gbr, err := GetBlobBySHA(ctx, ctx.Repo.Repository, gitRepo, ctx.Params(":sha")) + gbr, err := GetBlobBySHA(db.DefaultContext, repo, gitRepo, "65f1bf27bc3bf70f64657658635e66094edbcb4d") expectedGBR := &api.GitBlobResponse{ Content: "dHJlZSAyYTJmMWQ0NjcwNzI4YTJlMTAwNDllMzQ1YmQ3YTI3NjQ2OGJlYWI2CmF1dGhvciB1c2VyMSA8YWRkcmVzczFAZXhhbXBsZS5jb20+IDE0ODk5NTY0NzkgLTA0MDAKY29tbWl0dGVyIEV0aGFuIEtvZW5pZyA8ZXRoYW50a29lbmlnQGdtYWlsLmNvbT4gMTQ4OTk1NjQ3OSAtMDQwMAoKSW5pdGlhbCBjb21taXQK", Encoding: "base64", diff --git a/services/repository/files/diff_test.go b/services/repository/files/diff_test.go index 1ea4a170cc..95de10e07e 100644 --- a/services/repository/files/diff_test.go +++ b/services/repository/files/diff_test.go @@ -6,6 +6,7 @@ package files import ( "testing" + "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/modules/json" @@ -21,7 +22,6 @@ func TestGetDiffPreview(t *testing.T) { ctx, _ := contexttest.MockContext(t, "user2/repo1") ctx.SetParams(":id", "1") contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) contexttest.LoadUser(t, ctx, 2) contexttest.LoadGitRepo(t, ctx) defer ctx.Repo.GitRepo.Close() @@ -140,33 +140,26 @@ func TestGetDiffPreview(t *testing.T) { func TestGetDiffPreviewErrors(t *testing.T) { unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() - - branch := ctx.Repo.Repository.DefaultBranch + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + branch := repo.DefaultBranch treePath := "README.md" content := "# repo1\n\nDescription for repo1\nthis is a new line" t.Run("empty repo", func(t *testing.T) { - diff, err := GetDiffPreview(ctx, &repo_model.Repository{}, branch, treePath, content) + diff, err := GetDiffPreview(db.DefaultContext, &repo_model.Repository{}, branch, treePath, content) assert.Nil(t, diff) assert.EqualError(t, err, "repository does not exist [id: 0, uid: 0, owner_name: , name: ]") }) t.Run("bad branch", func(t *testing.T) { badBranch := "bad_branch" - diff, err := GetDiffPreview(ctx, ctx.Repo.Repository, badBranch, treePath, content) + diff, err := GetDiffPreview(db.DefaultContext, repo, badBranch, treePath, content) assert.Nil(t, diff) assert.EqualError(t, err, "branch does not exist [name: "+badBranch+"]") }) t.Run("empty treePath", func(t *testing.T) { - diff, err := GetDiffPreview(ctx, ctx.Repo.Repository, branch, "", content) + diff, err := GetDiffPreview(db.DefaultContext, repo, branch, "", content) assert.Nil(t, diff) assert.EqualError(t, err, "path is invalid [path: ]") }) diff --git a/services/repository/files/file_test.go b/services/repository/files/file_test.go index 2c6a169da1..7c387e2dd5 100644 --- a/services/repository/files/file_test.go +++ b/services/repository/files/file_test.go @@ -6,11 +6,12 @@ package files import ( "testing" + "code.gitea.io/gitea/models/db" + repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" - "code.gitea.io/gitea/services/contexttest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -99,23 +100,16 @@ func getExpectedFileResponse() *api.FileResponse { func TestGetFileResponseFromCommit(t *testing.T) { unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() - repo := ctx.Repo.Repository + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) branch := repo.DefaultBranch treePath := "README.md" - gitRepo, _ := gitrepo.OpenRepository(ctx, repo) + gitRepo, _ := gitrepo.OpenRepository(db.DefaultContext, repo) defer gitRepo.Close() commit, _ := gitRepo.GetBranchCommit(branch) expectedFileResponse := getExpectedFileResponse() - fileResponse, err := GetFileResponseFromCommit(ctx, repo, commit, branch, treePath) + fileResponse, err := GetFileResponseFromCommit(db.DefaultContext, repo, commit, branch, treePath) require.NoError(t, err) assert.EqualValues(t, expectedFileResponse, fileResponse) } diff --git a/services/repository/files/tree_test.go b/services/repository/files/tree_test.go index faa9b8e29e..9e5c5c1701 100644 --- a/services/repository/files/tree_test.go +++ b/services/repository/files/tree_test.go @@ -18,7 +18,6 @@ func TestGetTreeBySHA(t *testing.T) { unittest.PrepareTestEnv(t) ctx, _ := contexttest.MockContext(t, "user2/repo1") contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) contexttest.LoadUser(t, ctx, 2) contexttest.LoadGitRepo(t, ctx) defer ctx.Repo.GitRepo.Close() diff --git a/tests/integration/repo_view_test.go b/tests/integration/repo_view_test.go index 290686d554..7c280e2491 100644 --- a/tests/integration/repo_view_test.go +++ b/tests/integration/repo_view_test.go @@ -52,8 +52,13 @@ func createRepoAndGetContext(t *testing.T, files []string, deleteMdReadme bool) ctx, _ := contexttest.MockContext(t, "user1/readmetest") ctx.SetParams(":id", fmt.Sprint(repo.ID)) contexttest.LoadRepo(t, ctx, repo.ID) + contexttest.LoadGitRepo(t, ctx) contexttest.LoadRepoCommit(t, ctx) - return ctx, f + + return ctx, func() { + f() + ctx.Repo.GitRepo.Close() + } } func TestRepoView_FindReadme(t *testing.T) { diff --git a/tests/integration/repofiles_change_test.go b/tests/integration/repofiles_change_test.go index 7f5e17c2ce..9790b36183 100644 --- a/tests/integration/repofiles_change_test.go +++ b/tests/integration/repofiles_change_test.go @@ -12,11 +12,11 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" + 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/setting" api "code.gitea.io/gitea/modules/structs" - "code.gitea.io/gitea/services/contexttest" files_service "code.gitea.io/gitea/services/repository/files" "github.com/stretchr/testify/assert" @@ -247,16 +247,8 @@ func getExpectedFileResponseForRepofilesUpdate(commitID, filename, lastCommitSHA func TestChangeRepoFilesForCreate(t *testing.T) { // setup onGiteaRun(t, func(t *testing.T, u *url.URL) { - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() - - repo := ctx.Repo.Repository - doer := ctx.Doer + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) opts := getCreateRepoFilesOptions(repo) // test @@ -284,16 +276,8 @@ func TestChangeRepoFilesForCreate(t *testing.T) { func TestChangeRepoFilesForUpdate(t *testing.T) { // setup onGiteaRun(t, func(t *testing.T, u *url.URL) { - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() - - repo := ctx.Repo.Repository - doer := ctx.Doer + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) opts := getUpdateRepoFilesOptions(repo) // test @@ -318,16 +302,8 @@ func TestChangeRepoFilesForUpdate(t *testing.T) { func TestChangeRepoFilesForUpdateWithFileMove(t *testing.T) { // setup onGiteaRun(t, func(t *testing.T, u *url.URL) { - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() - - repo := ctx.Repo.Repository - doer := ctx.Doer + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) opts := getUpdateRepoFilesOptions(repo) opts.Files[0].FromTreePath = "README.md" opts.Files[0].TreePath = "README_new.md" // new file name, README_new.md @@ -369,16 +345,8 @@ func TestChangeRepoFilesForUpdateWithFileMove(t *testing.T) { func TestChangeRepoFilesWithoutBranchNames(t *testing.T) { // setup onGiteaRun(t, func(t *testing.T, u *url.URL) { - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() - - repo := ctx.Repo.Repository - doer := ctx.Doer + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) opts := getUpdateRepoFilesOptions(repo) opts.OldBranch = "" opts.NewBranch = "" @@ -405,15 +373,8 @@ func TestChangeRepoFilesForDelete(t *testing.T) { func testDeleteRepoFiles(t *testing.T, u *url.URL) { // setup unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() - repo := ctx.Repo.Repository - doer := ctx.Doer + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) opts := getDeleteRepoFilesOptions(repo) t.Run("Delete README.md file", func(t *testing.T) { @@ -444,16 +405,9 @@ func TestChangeRepoFilesForDeleteWithoutBranchNames(t *testing.T) { func testDeleteRepoFilesWithoutBranchNames(t *testing.T, u *url.URL) { // setup unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) - repo := ctx.Repo.Repository - doer := ctx.Doer opts := getDeleteRepoFilesOptions(repo) opts.OldBranch = "" opts.NewBranch = "" @@ -474,16 +428,8 @@ func testDeleteRepoFilesWithoutBranchNames(t *testing.T, u *url.URL) { func TestChangeRepoFilesErrors(t *testing.T) { // setup onGiteaRun(t, func(t *testing.T, u *url.URL) { - ctx, _ := contexttest.MockContext(t, "user2/repo1") - ctx.SetParams(":id", "1") - contexttest.LoadRepo(t, ctx, 1) - contexttest.LoadRepoCommit(t, ctx) - contexttest.LoadUser(t, ctx, 2) - contexttest.LoadGitRepo(t, ctx) - defer ctx.Repo.GitRepo.Close() - - repo := ctx.Repo.Repository - doer := ctx.Doer + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) t.Run("bad branch", func(t *testing.T) { opts := getUpdateRepoFilesOptions(repo)