Add Close() method to gogitRepository (#8901)

In investigating #7947 it has become clear that the storage component of go-git repositories needs closing.

This PR adds this Close function and adds the Close functions as necessary.

In TransferOwnership the ctx.Repo.GitRepo is closed if it is open to help prevent the risk of multiple open files.

Fixes #7947

15 files changed, 46 insertions, 2 deletions

diff --git a/modules/git/blame.go b/modules/git/blame.go
index 548236b657..4f4343fe96 100644
--- a/modules/git/blame.go
+++ b/modules/git/blame.go
@@ -87,10 +87,11 @@ func (r *BlameReader) Close() error {
 
 // CreateBlameReader creates reader for given repository, commit and file
 func CreateBlameReader(repoPath, commitID, file string) (*BlameReader, error) {
-	_, err := OpenRepository(repoPath)
+	gitRepo, err := OpenRepository(repoPath)
 	if err != nil {
 		return nil, err
 	}
+	gitRepo.Close()
 
 	return createBlameReader(repoPath, GitExecutable, "blame", commitID, "--porcelain", "--", file)
 }
diff --git a/modules/git/blob_test.go b/modules/git/blob_test.go
index 66c046ecc8..9043de5955 100644
--- a/modules/git/blob_test.go
+++ b/modules/git/blob_test.go
@@ -37,6 +37,8 @@ THE SOFTWARE.
 `
 	repo, err := OpenRepository("../../.git")
 	assert.NoError(t, err)
+	defer repo.Close()
+
 	testBlob, err := repo.GetBlob("a8d4b49dd073a4a38a7e58385eeff7cc52568697")
 	assert.NoError(t, err)
 
@@ -55,6 +57,8 @@ func Benchmark_Blob_Data(b *testing.B) {
 	if err != nil {
 		b.Fatal(err)
 	}
+	defer repo.Close()
+
 	testBlob, err := repo.GetBlob("a8d4b49dd073a4a38a7e58385eeff7cc52568697")
 	if err != nil {
 		b.Fatal(err)
diff --git a/modules/git/commit_info_test.go b/modules/git/commit_info_test.go
index 71637d188a..ac7bc43c4e 100644
--- a/modules/git/commit_info_test.go
+++ b/modules/git/commit_info_test.go
@@ -77,6 +77,8 @@ func TestEntries_GetCommitsInfo(t *testing.T) {
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
 	bareRepo1, err := OpenRepository(bareRepo1Path)
 	assert.NoError(t, err)
+	defer bareRepo1.Close()
+
 	testGetCommitsInfo(t, bareRepo1)
 
 	clonedPath, err := cloneRepo(bareRepo1Path, testReposDir, "repo1_TestEntries_GetCommitsInfo")
@@ -84,6 +86,8 @@ func TestEntries_GetCommitsInfo(t *testing.T) {
 	defer os.RemoveAll(clonedPath)
 	clonedRepo1, err := OpenRepository(clonedPath)
 	assert.NoError(t, err)
+	defer clonedRepo1.Close()
+
 	testGetCommitsInfo(t, clonedRepo1)
 }
 
@@ -101,13 +105,16 @@ func BenchmarkEntries_GetCommitsInfo(b *testing.B) {
 	for _, benchmark := range benchmarks {
 		var commit *Commit
 		var entries Entries
+		var repo *Repository
 		if repoPath, err := cloneRepo(benchmark.url, benchmarkReposDir, benchmark.name); err != nil {
 			b.Fatal(err)
-		} else if repo, err := OpenRepository(repoPath); err != nil {
+		} else if repo, err = OpenRepository(repoPath); err != nil {
 			b.Fatal(err)
 		} else if commit, err = repo.GetBranchCommit("master"); err != nil {
+			repo.Close()
 			b.Fatal(err)
 		} else if entries, err = commit.Tree.ListEntries(); err != nil {
+			repo.Close()
 			b.Fatal(err)
 		}
 		entries.Sort()
@@ -120,5 +127,6 @@ func BenchmarkEntries_GetCommitsInfo(b *testing.B) {
 				}
 			}
 		})
+		repo.Close()
 	}
 }
diff --git a/modules/git/notes_test.go b/modules/git/notes_test.go
index bf010b9a71..b7939e6913 100644
--- a/modules/git/notes_test.go
+++ b/modules/git/notes_test.go
@@ -15,6 +15,7 @@ func TestGetNotes(t *testing.T) {
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
 	bareRepo1, err := OpenRepository(bareRepo1Path)
 	assert.NoError(t, err)
+	defer bareRepo1.Close()
 
 	note := Note{}
 	err = GetNote(bareRepo1, "95bb4d39648ee7e325106df01a621c530863a653", &note)
@@ -27,6 +28,7 @@ func TestGetNestedNotes(t *testing.T) {
 	repoPath := filepath.Join(testReposDir, "repo3_notes")
 	repo, err := OpenRepository(repoPath)
 	assert.NoError(t, err)
+	defer repo.Close()
 
 	note := Note{}
 	err = GetNote(repo, "3e668dbfac39cbc80a9ff9c61eb565d944453ba4", &note)
diff --git a/modules/git/repo.go b/modules/git/repo.go
index 80f6109772..ffca2524be 100644
--- a/modules/git/repo.go
+++ b/modules/git/repo.go
@@ -17,6 +17,7 @@ import (
 	"strings"
 	"time"
 
+	gitealog "code.gitea.io/gitea/modules/log"
 	"github.com/unknwon/com"
 	"gopkg.in/src-d/go-billy.v4/osfs"
 	gogit "gopkg.in/src-d/go-git.v4"
@@ -122,6 +123,16 @@ func OpenRepository(repoPath string) (*Repository, error) {
 	}, nil
 }
 
+// Close this repository, in particular close the underlying gogitStorage if this is not nil
+func (repo *Repository) Close() {
+	if repo == nil || repo.gogitStorage == nil {
+		return
+	}
+	if err := repo.gogitStorage.Close(); err != nil {
+		gitealog.Error("Error closing storage: %v", err)
+	}
+}
+
 // GoGitRepo gets the go-git repo representation
 func (repo *Repository) GoGitRepo() *gogit.Repository {
 	return repo.gogitRepo
diff --git a/modules/git/repo_blob_test.go b/modules/git/repo_blob_test.go
index 128a227829..52a124db2a 100644
--- a/modules/git/repo_blob_test.go
+++ b/modules/git/repo_blob_test.go
@@ -17,6 +17,7 @@ func TestRepository_GetBlob_Found(t *testing.T) {
 	repoPath := filepath.Join(testReposDir, "repo1_bare")
 	r, err := OpenRepository(repoPath)
 	assert.NoError(t, err)
+	defer r.Close()
 
 	testCases := []struct {
 		OID  string
@@ -44,6 +45,7 @@ func TestRepository_GetBlob_NotExist(t *testing.T) {
 	repoPath := filepath.Join(testReposDir, "repo1_bare")
 	r, err := OpenRepository(repoPath)
 	assert.NoError(t, err)
+	defer r.Close()
 
 	testCase := "0000000000000000000000000000000000000000"
 	testError := ErrNotExist{testCase, ""}
@@ -57,6 +59,7 @@ func TestRepository_GetBlob_NoId(t *testing.T) {
 	repoPath := filepath.Join(testReposDir, "repo1_bare")
 	r, err := OpenRepository(repoPath)
 	assert.NoError(t, err)
+	defer r.Close()
 
 	testCase := ""
 	testError := fmt.Errorf("Length must be 40: %s", testCase)
diff --git a/modules/git/repo_branch.go b/modules/git/repo_branch.go
index a2bf9ac973..e79bab76a6 100644
--- a/modules/git/repo_branch.go
+++ b/modules/git/repo_branch.go
@@ -108,6 +108,7 @@ func GetBranchesByPath(path string) ([]*Branch, error) {
 	if err != nil {
 		return nil, err
 	}
+	defer gitRepo.Close()
 
 	brs, err := gitRepo.GetBranches()
 	if err != nil {
diff --git a/modules/git/repo_branch_test.go b/modules/git/repo_branch_test.go
index 08736d702e..33d31aef68 100644
--- a/modules/git/repo_branch_test.go
+++ b/modules/git/repo_branch_test.go
@@ -15,6 +15,7 @@ func TestRepository_GetBranches(t *testing.T) {
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
 	bareRepo1, err := OpenRepository(bareRepo1Path)
 	assert.NoError(t, err)
+	defer bareRepo1.Close()
 
 	branches, err := bareRepo1.GetBranches()
 
@@ -29,6 +30,7 @@ func BenchmarkRepository_GetBranches(b *testing.B) {
 	if err != nil {
 		b.Fatal(err)
 	}
+	defer bareRepo1.Close()
 
 	for i := 0; i < b.N; i++ {
 		_, err := bareRepo1.GetBranches()
diff --git a/modules/git/repo_commit_test.go b/modules/git/repo_commit_test.go
index 6d8ee6453f..87dd6763b3 100644
--- a/modules/git/repo_commit_test.go
+++ b/modules/git/repo_commit_test.go
@@ -15,6 +15,7 @@ func TestRepository_GetCommitBranches(t *testing.T) {
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
 	bareRepo1, err := OpenRepository(bareRepo1Path)
 	assert.NoError(t, err)
+	defer bareRepo1.Close()
 
 	// these test case are specific to the repo1_bare test repo
 	testCases := []struct {
@@ -41,6 +42,7 @@ func TestGetTagCommitWithSignature(t *testing.T) {
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
 	bareRepo1, err := OpenRepository(bareRepo1Path)
 	assert.NoError(t, err)
+	defer bareRepo1.Close()
 
 	commit, err := bareRepo1.GetCommit("3ad28a9149a2864384548f3d17ed7f38014c9e8a")
 	assert.NoError(t, err)
@@ -54,6 +56,7 @@ func TestGetCommitWithBadCommitID(t *testing.T) {
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
 	bareRepo1, err := OpenRepository(bareRepo1Path)
 	assert.NoError(t, err)
+	defer bareRepo1.Close()
 
 	commit, err := bareRepo1.GetCommit("bad_branch")
 	assert.Nil(t, commit)
diff --git a/modules/git/repo_compare_test.go b/modules/git/repo_compare_test.go
index e194788773..def67fa87b 100644
--- a/modules/git/repo_compare_test.go
+++ b/modules/git/repo_compare_test.go
@@ -20,6 +20,7 @@ func TestGetFormatPatch(t *testing.T) {
 	defer os.RemoveAll(clonedPath)
 	repo, err := OpenRepository(clonedPath)
 	assert.NoError(t, err)
+	defer repo.Close()
 	rd, err := repo.GetFormatPatch("8d92fc95^", "8d92fc95")
 	assert.NoError(t, err)
 	patchb, err := ioutil.ReadAll(rd)
diff --git a/modules/git/repo_ref_test.go b/modules/git/repo_ref_test.go
index d32b34994c..303c496c1d 100644
--- a/modules/git/repo_ref_test.go
+++ b/modules/git/repo_ref_test.go
@@ -15,6 +15,7 @@ func TestRepository_GetRefs(t *testing.T) {
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
 	bareRepo1, err := OpenRepository(bareRepo1Path)
 	assert.NoError(t, err)
+	defer bareRepo1.Close()
 
 	refs, err := bareRepo1.GetRefs()
 
@@ -38,6 +39,7 @@ func TestRepository_GetRefsFiltered(t *testing.T) {
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
 	bareRepo1, err := OpenRepository(bareRepo1Path)
 	assert.NoError(t, err)
+	defer bareRepo1.Close()
 
 	refs, err := bareRepo1.GetRefsFiltered(TagPrefix)
 
diff --git a/modules/git/repo_stats_test.go b/modules/git/repo_stats_test.go
index 6fbcb7ac13..bc1f6a5662 100644
--- a/modules/git/repo_stats_test.go
+++ b/modules/git/repo_stats_test.go
@@ -16,6 +16,7 @@ func TestRepository_GetCodeActivityStats(t *testing.T) {
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
 	bareRepo1, err := OpenRepository(bareRepo1Path)
 	assert.NoError(t, err)
+	defer bareRepo1.Close()
 
 	timeFrom, err := time.Parse(time.RFC3339, "2016-01-01T00:00:00+00:00")
 	assert.NoError(t, err)
diff --git a/modules/git/repo_tag_test.go b/modules/git/repo_tag_test.go
index ab9742afc5..90f2b37358 100644
--- a/modules/git/repo_tag_test.go
+++ b/modules/git/repo_tag_test.go
@@ -16,6 +16,7 @@ func TestRepository_GetTags(t *testing.T) {
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
 	bareRepo1, err := OpenRepository(bareRepo1Path)
 	assert.NoError(t, err)
+	defer bareRepo1.Close()
 
 	tags, err := bareRepo1.GetTagInfos()
 	assert.NoError(t, err)
@@ -34,6 +35,7 @@ func TestRepository_GetTag(t *testing.T) {
 
 	bareRepo1, err := OpenRepository(clonedPath)
 	assert.NoError(t, err)
+	defer bareRepo1.Close()
 
 	lTagCommitID := "6fbd69e9823458e6c4a2fc5c0f6bc022b2f2acd1"
 	lTagName := "lightweightTag"
@@ -83,6 +85,7 @@ func TestRepository_GetAnnotatedTag(t *testing.T) {
 
 	bareRepo1, err := OpenRepository(clonedPath)
 	assert.NoError(t, err)
+	defer bareRepo1.Close()
 
 	lTagCommitID := "6fbd69e9823458e6c4a2fc5c0f6bc022b2f2acd1"
 	lTagName := "lightweightTag"
diff --git a/modules/git/repo_test.go b/modules/git/repo_test.go
index 15f5e3781c..0b6986764c 100644
--- a/modules/git/repo_test.go
+++ b/modules/git/repo_test.go
@@ -30,6 +30,7 @@ func TestRepoIsEmpty(t *testing.T) {
 	emptyRepo2Path := filepath.Join(testReposDir, "repo2_empty")
 	repo, err := OpenRepository(emptyRepo2Path)
 	assert.NoError(t, err)
+	defer repo.Close()
 	isEmpty, err := repo.IsEmpty()
 	assert.NoError(t, err)
 	assert.True(t, isEmpty)
diff --git a/modules/git/tree_entry_test.go b/modules/git/tree_entry_test.go
index c65a691ecf..e872900370 100644
--- a/modules/git/tree_entry_test.go
+++ b/modules/git/tree_entry_test.go
@@ -56,6 +56,7 @@ func TestEntriesCustomSort(t *testing.T) {
 func TestFollowLink(t *testing.T) {
 	r, err := OpenRepository("tests/repos/repo1_bare")
 	assert.NoError(t, err)
+	defer r.Close()
 
 	commit, err := r.GetCommit("37991dec2c8e592043f47155ce4808d4580f9123")
 	assert.NoError(t, err)