Add Close() method to gogitRepository (#8901) (#8956)

Backport #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

13 files changed, 114 insertions, 3 deletions

diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go
index 04ff91fbbf..10b827253d 100644
--- a/routers/api/v1/api.go
+++ b/routers/api/v1/api.go
@@ -638,7 +638,7 @@ func RegisterRoutes(m *macaron.Macaron) {
 				}, reqRepoReader(models.UnitTypeCode))
 				m.Group("/tags", func() {
 					m.Get("", repo.ListTags)
-				}, reqRepoReader(models.UnitTypeCode))
+				}, reqRepoReader(models.UnitTypeCode), context.ReferencesGitRepo(true))
 				m.Group("/keys", func() {
 					m.Combo("").Get(repo.ListDeployKeys).
 						Post(bind(api.CreateKeyOption{}), repo.CreateDeployKey)
diff --git a/routers/api/v1/repo/commits.go b/routers/api/v1/repo/commits.go
index 0156aaaa05..163a06a95e 100644
--- a/routers/api/v1/repo/commits.go
+++ b/routers/api/v1/repo/commits.go
@@ -51,6 +51,7 @@ func GetSingleCommit(ctx *context.APIContext) {
 		ctx.ServerError("OpenRepository", err)
 		return
 	}
+	defer gitRepo.Close()
 	commit, err := gitRepo.GetCommit(ctx.Params(":sha"))
 	if err != nil {
 		ctx.NotFoundOrServerError("GetCommit", git.IsErrNotExist, err)
@@ -113,6 +114,7 @@ func GetAllCommits(ctx *context.APIContext) {
 		ctx.ServerError("OpenRepository", err)
 		return
 	}
+	defer gitRepo.Close()
 
 	page := ctx.QueryInt("page")
 	if page <= 0 {
diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go
index ae20e1e96b..f23e7bca9e 100644
--- a/routers/api/v1/repo/file.go
+++ b/routers/api/v1/repo/file.go
@@ -95,6 +95,7 @@ func GetArchive(ctx *context.APIContext) {
 		return
 	}
 	ctx.Repo.GitRepo = gitRepo
+	defer gitRepo.Close()
 
 	repo.Download(ctx.Context)
 }
diff --git a/routers/api/v1/repo/git_ref.go b/routers/api/v1/repo/git_ref.go
index d7acc139f0..c2bcbb3603 100644
--- a/routers/api/v1/repo/git_ref.go
+++ b/routers/api/v1/repo/git_ref.go
@@ -76,6 +76,8 @@ func getGitRefs(ctx *context.APIContext, filter string) ([]*git.Reference, strin
 	if err != nil {
 		return nil, "OpenRepository", err
 	}
+	defer gitRepo.Close()
+
 	if len(filter) > 0 {
 		filter = "refs/" + filter
 	}
diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go
index 4d6a84d8f8..2601ab8339 100644
--- a/routers/api/v1/repo/pull.go
+++ b/routers/api/v1/repo/pull.go
@@ -194,6 +194,7 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption
 	if ctx.Written() {
 		return
 	}
+	defer headGitRepo.Close()
 
 	// Check if another PR exists with the same targets
 	existingPr, err := models.GetUnmergedPullRequest(headRepo.ID, ctx.Repo.Repository.ID, headBranch, baseBranch)
@@ -687,6 +688,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
 	// user should have permission to read baseRepo's codes and pulls, NOT headRepo's
 	permBase, err := models.GetUserRepoPermission(baseRepo, ctx.User)
 	if err != nil {
+		headGitRepo.Close()
 		ctx.ServerError("GetUserRepoPermission", err)
 		return nil, nil, nil, nil, "", ""
 	}
@@ -697,6 +699,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
 				baseRepo,
 				permBase)
 		}
+		headGitRepo.Close()
 		ctx.NotFound("Can't read pulls or can't read UnitTypeCode")
 		return nil, nil, nil, nil, "", ""
 	}
@@ -704,6 +707,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
 	// user should have permission to read headrepo's codes
 	permHead, err := models.GetUserRepoPermission(headRepo, ctx.User)
 	if err != nil {
+		headGitRepo.Close()
 		ctx.ServerError("GetUserRepoPermission", err)
 		return nil, nil, nil, nil, "", ""
 	}
@@ -714,18 +718,21 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
 				headRepo,
 				permHead)
 		}
+		headGitRepo.Close()
 		ctx.NotFound("Can't read headRepo UnitTypeCode")
 		return nil, nil, nil, nil, "", ""
 	}
 
 	// Check if head branch is valid.
 	if !headGitRepo.IsBranchExist(headBranch) {
+		headGitRepo.Close()
 		ctx.NotFound()
 		return nil, nil, nil, nil, "", ""
 	}
 
 	compareInfo, err := headGitRepo.GetCompareInfo(models.RepoPath(baseRepo.Owner.Name, baseRepo.Name), baseBranch, headBranch)
 	if err != nil {
+		headGitRepo.Close()
 		ctx.Error(500, "GetCompareInfo", err)
 		return nil, nil, nil, nil, "", ""
 	}
diff --git a/routers/api/v1/repo/tag.go b/routers/api/v1/repo/tag.go
index a802048285..12bac854ec 100644
--- a/routers/api/v1/repo/tag.go
+++ b/routers/api/v1/repo/tag.go
@@ -33,7 +33,7 @@ func ListTags(ctx *context.APIContext) {
 	// responses:
 	//   "200":
 	//     "$ref": "#/responses/TagList"
-	tags, err := ctx.Repo.Repository.GetTags()
+	tags, err := ctx.Repo.GitRepo.GetTagInfos()
 	if err != nil {
 		ctx.Error(500, "GetTags", err)
 		return
diff --git a/routers/repo/compare.go b/routers/repo/compare.go
index f8534f68b7..2b66d3818d 100644
--- a/routers/repo/compare.go
+++ b/routers/repo/compare.go
@@ -157,6 +157,7 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
 	// user should have permission to read baseRepo's codes and pulls, NOT headRepo's
 	permBase, err := models.GetUserRepoPermission(baseRepo, ctx.User)
 	if err != nil {
+		headGitRepo.Close()
 		ctx.ServerError("GetUserRepoPermission", err)
 		return nil, nil, nil, nil, "", ""
 	}
@@ -167,6 +168,7 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
 				baseRepo,
 				permBase)
 		}
+		headGitRepo.Close()
 		ctx.NotFound("ParseCompareInfo", nil)
 		return nil, nil, nil, nil, "", ""
 	}
@@ -174,6 +176,7 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
 	// user should have permission to read headrepo's codes
 	permHead, err := models.GetUserRepoPermission(headRepo, ctx.User)
 	if err != nil {
+		headGitRepo.Close()
 		ctx.ServerError("GetUserRepoPermission", err)
 		return nil, nil, nil, nil, "", ""
 	}
@@ -184,6 +187,7 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
 				headRepo,
 				permHead)
 		}
+		headGitRepo.Close()
 		ctx.NotFound("ParseCompareInfo", nil)
 		return nil, nil, nil, nil, "", ""
 	}
@@ -199,6 +203,7 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
 			ctx.Data["HeadBranch"] = headBranch
 			headIsCommit = true
 		} else {
+			headGitRepo.Close()
 			ctx.NotFound("IsRefExist", nil)
 			return nil, nil, nil, nil, "", ""
 		}
@@ -219,12 +224,14 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
 				baseRepo,
 				permBase)
 		}
+		headGitRepo.Close()
 		ctx.NotFound("ParseCompareInfo", nil)
 		return nil, nil, nil, nil, "", ""
 	}
 
 	compareInfo, err := headGitRepo.GetCompareInfo(models.RepoPath(baseRepo.Owner.Name, baseRepo.Name), baseBranch, headBranch)
 	if err != nil {
+		headGitRepo.Close()
 		ctx.ServerError("GetCompareInfo", err)
 		return nil, nil, nil, nil, "", ""
 	}
@@ -345,6 +352,7 @@ func CompareDiff(ctx *context.Context) {
 	if ctx.Written() {
 		return
 	}
+	defer headGitRepo.Close()
 
 	nothingToCompare := PrepareCompareDiff(ctx, headUser, headRepo, headGitRepo, compareInfo, baseBranch, headBranch)
 	if ctx.Written() {
diff --git a/routers/repo/editor_test.go b/routers/repo/editor_test.go
index ca00be74b7..ec7aee1e77 100644
--- a/routers/repo/editor_test.go
+++ b/routers/repo/editor_test.go
@@ -48,6 +48,8 @@ func TestGetUniquePatchBranchName(t *testing.T) {
 	test.LoadRepoCommit(t, ctx)
 	test.LoadUser(t, ctx, 2)
 	test.LoadGitRepo(t, ctx)
+	defer ctx.Repo.GitRepo.Close()
+
 	expectedBranchName := "user2-patch-1"
 	branchName := GetUniquePatchBranchName(ctx)
 	assert.Equal(t, expectedBranchName, branchName)
@@ -61,9 +63,12 @@ func TestGetClosestParentWithFiles(t *testing.T) {
 	test.LoadRepoCommit(t, ctx)
 	test.LoadUser(t, ctx, 2)
 	test.LoadGitRepo(t, ctx)
+	defer ctx.Repo.GitRepo.Close()
+
 	repo := ctx.Repo.Repository
 	branch := repo.DefaultBranch
 	gitRepo, _ := git.OpenRepository(repo.RepoPath())
+	defer gitRepo.Close()
 	commit, _ := gitRepo.GetBranchCommit(branch)
 	expectedTreePath := ""
 
diff --git a/routers/repo/pull.go b/routers/repo/pull.go
index 23d1718cf2..33bf1b2ed9 100644
--- a/routers/repo/pull.go
+++ b/routers/repo/pull.go
@@ -337,6 +337,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
 			ctx.ServerError("OpenRepository", err)
 			return nil
 		}
+		defer headGitRepo.Close()
 
 		headBranchExist = headGitRepo.IsBranchExist(pull.HeadBranch)
 
@@ -519,6 +520,7 @@ func ViewPullFiles(ctx *context.Context) {
 			ctx.ServerError("OpenRepository", err)
 			return
 		}
+		defer headGitRepo.Close()
 
 		headCommitID, err := headGitRepo.GetBranchCommitID(pull.HeadBranch)
 		if err != nil {
@@ -704,6 +706,7 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm)
 	if ctx.Written() {
 		return
 	}
+	defer headGitRepo.Close()
 
 	labelIDs, assigneeIDs, milestoneID := ValidateRepoMetas(ctx, form, true)
 	if ctx.Written() {
@@ -870,12 +873,14 @@ func CleanUpPullRequest(ctx *context.Context) {
 		ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.RepoPath()), err)
 		return
 	}
+	defer gitRepo.Close()
 
 	gitBaseRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
 	if err != nil {
 		ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.BaseRepo.RepoPath()), err)
 		return
 	}
+	defer gitBaseRepo.Close()
 
 	defer func() {
 		ctx.JSON(200, map[string]interface{}{
@@ -1004,6 +1009,7 @@ func DownloadPullPatch(ctx *context.Context) {
 		ctx.ServerError("OpenRepository", err)
 		return
 	}
+	defer headGitRepo.Close()
 
 	patch, err := headGitRepo.GetFormatPatch(pr.MergeBase, pr.HeadBranch)
 	if err != nil {
diff --git a/routers/repo/release_test.go b/routers/repo/release_test.go
index 524c1c7346..47d1a89b54 100644
--- a/routers/repo/release_test.go
+++ b/routers/repo/release_test.go
@@ -57,5 +57,6 @@ func TestNewReleasePost(t *testing.T) {
 			Title:       testCase.Form.Title,
 			Note:        testCase.Form.Content,
 		}, models.Cond("is_draft=?", len(testCase.Form.Draft) > 0))
+		ctx.Repo.GitRepo.Close()
 	}
 }
diff --git a/routers/repo/setting.go b/routers/repo/setting.go
index c74b273420..eb4f27e64b 100644
--- a/routers/repo/setting.go
+++ b/routers/repo/setting.go
@@ -71,6 +71,11 @@ func SettingsPost(ctx *context.Context, form auth.RepoSettingForm) {
 		// Check if repository name has been changed.
 		if repo.LowerName != strings.ToLower(newRepoName) {
 			isNameChanged = true
+			// Close the GitRepo if open
+			if ctx.Repo.GitRepo != nil {
+				ctx.Repo.GitRepo.Close()
+				ctx.Repo.GitRepo = nil
+			}
 			if err := models.ChangeRepositoryName(ctx.Repo.Owner, repo.Name, newRepoName); err != nil {
 				ctx.Data["Err_RepoName"] = true
 				switch {
@@ -378,6 +383,11 @@ func SettingsPost(ctx *context.Context, form auth.RepoSettingForm) {
 		}
 
 		oldOwnerID := ctx.Repo.Owner.ID
+		// Close the GitRepo if open
+		if ctx.Repo.GitRepo != nil {
+			ctx.Repo.GitRepo.Close()
+			ctx.Repo.GitRepo = nil
+		}
 		if err = models.TransferOwnership(ctx.User, newOwner, repo); err != nil {
 			if models.IsErrRepoAlreadyExist(err) {
 				ctx.RenderWithErr(ctx.Tr("repo.settings.new_owner_has_same_repo"), tplSettingsOptions, nil)
diff --git a/routers/repo/wiki.go b/routers/repo/wiki.go
index 02fbe4a1dd..6cf1943658 100644
--- a/routers/repo/wiki.go
+++ b/routers/repo/wiki.go
@@ -146,6 +146,9 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) {
 	// Get page list.
 	entries, err := commit.ListEntries()
 	if err != nil {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
 		ctx.ServerError("ListEntries", err)
 		return nil, nil
 	}
@@ -159,6 +162,9 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) {
 			if models.IsErrWikiInvalidFileName(err) {
 				continue
 			}
+			if wikiRepo != nil {
+				wikiRepo.Close()
+			}
 			ctx.ServerError("WikiFilenameToName", err)
 			return nil, nil
 		} else if wikiName == "_Sidebar" || wikiName == "_Footer" {
@@ -188,16 +194,25 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) {
 		ctx.Redirect(ctx.Repo.RepoLink + "/wiki/_pages")
 	}
 	if entry == nil || ctx.Written() {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
 		return nil, nil
 	}
 
 	sidebarContent, _, _, _ := wikiContentsByName(ctx, commit, "_Sidebar")
 	if ctx.Written() {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
 		return nil, nil
 	}
 
 	footerContent, _, _, _ := wikiContentsByName(ctx, commit, "_Footer")
 	if ctx.Written() {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
 		return nil, nil
 	}
 
@@ -218,6 +233,9 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) {
 func renderRevisionPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) {
 	wikiRepo, commit, err := findWikiRepoCommit(ctx)
 	if err != nil {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
 		if !git.IsErrNotExist(err) {
 			ctx.ServerError("GetBranchCommit", err)
 		}
@@ -241,6 +259,9 @@ func renderRevisionPage(ctx *context.Context) (*git.Repository, *git.TreeEntry)
 		ctx.Redirect(ctx.Repo.RepoLink + "/wiki/_pages")
 	}
 	if entry == nil || ctx.Written() {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
 		return nil, nil
 	}
 
@@ -263,6 +284,9 @@ func renderRevisionPage(ctx *context.Context) (*git.Repository, *git.TreeEntry)
 	// get Commit Count
 	commitsHistory, err := wikiRepo.CommitsByFileAndRangeNoFollow("master", pageFilename, page)
 	if err != nil {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
 		ctx.ServerError("CommitsByFileAndRangeNoFollow", err)
 		return nil, nil
 	}
@@ -279,13 +303,21 @@ func renderRevisionPage(ctx *context.Context) (*git.Repository, *git.TreeEntry)
 }
 
 func renderEditPage(ctx *context.Context) {
-	_, commit, err := findWikiRepoCommit(ctx)
+	wikiRepo, commit, err := findWikiRepoCommit(ctx)
 	if err != nil {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
 		if !git.IsErrNotExist(err) {
 			ctx.ServerError("GetBranchCommit", err)
 		}
 		return
 	}
+	defer func() {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
+	}()
 
 	// get requested pagename
 	pageName := models.NormalizeWikiName(ctx.Params(":page"))
@@ -327,8 +359,16 @@ func Wiki(ctx *context.Context) {
 
 	wikiRepo, entry := renderViewPage(ctx)
 	if ctx.Written() {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
 		return
 	}
+	defer func() {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
+	}()
 	if entry == nil {
 		ctx.Data["Title"] = ctx.Tr("repo.wiki")
 		ctx.HTML(200, tplWikiStart)
@@ -364,8 +404,16 @@ func WikiRevision(ctx *context.Context) {
 
 	wikiRepo, entry := renderRevisionPage(ctx)
 	if ctx.Written() {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
 		return
 	}
+	defer func() {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
+	}()
 	if entry == nil {
 		ctx.Data["Title"] = ctx.Tr("repo.wiki")
 		ctx.HTML(200, tplWikiStart)
@@ -397,11 +445,18 @@ func WikiPages(ctx *context.Context) {
 
 	wikiRepo, commit, err := findWikiRepoCommit(ctx)
 	if err != nil {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
 		return
 	}
 
 	entries, err := commit.ListEntries()
 	if err != nil {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
+
 		ctx.ServerError("ListEntries", err)
 		return
 	}
@@ -412,6 +467,10 @@ func WikiPages(ctx *context.Context) {
 		}
 		c, err := wikiRepo.GetCommitByPath(entry.Name())
 		if err != nil {
+			if wikiRepo != nil {
+				wikiRepo.Close()
+			}
+
 			ctx.ServerError("GetCommit", err)
 			return
 		}
@@ -420,6 +479,10 @@ func WikiPages(ctx *context.Context) {
 			if models.IsErrWikiInvalidFileName(err) {
 				continue
 			}
+			if wikiRepo != nil {
+				wikiRepo.Close()
+			}
+
 			ctx.ServerError("WikiFilenameToName", err)
 			return
 		}
@@ -431,6 +494,11 @@ func WikiPages(ctx *context.Context) {
 	}
 	ctx.Data["Pages"] = pages
 
+	defer func() {
+		if wikiRepo != nil {
+			wikiRepo.Close()
+		}
+	}()
 	ctx.HTML(200, tplWikiPages)
 }
 
diff --git a/routers/repo/wiki_test.go b/routers/repo/wiki_test.go
index 4687d24f6b..44fcd02035 100644
--- a/routers/repo/wiki_test.go
+++ b/routers/repo/wiki_test.go
@@ -23,6 +23,7 @@ const message = "Wiki commit message for unit tests"
 func wikiEntry(t *testing.T, repo *models.Repository, wikiName string) *git.TreeEntry {
 	wikiRepo, err := git.OpenRepository(repo.WikiPath())
 	assert.NoError(t, err)
+	defer wikiRepo.Close()
 	commit, err := wikiRepo.GetBranchCommit("master")
 	assert.NoError(t, err)
 	entries, err := commit.ListEntries()