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

7 files changed, 103 insertions, 1 deletions

diff --git a/routers/repo/compare.go b/routers/repo/compare.go
index b9e14abfb8..e45f046435 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, "", ""
 	}
@@ -356,6 +363,8 @@ func parseBaseRepoInfo(ctx *context.Context, repo *models.Repository) error {
 	if err != nil {
 		return err
 	}
+	defer baseGitRepo.Close()
+
 	ctx.Data["BaseRepoBranches"], err = baseGitRepo.GetBranches()
 	if err != nil {
 		return err
@@ -369,6 +378,8 @@ func CompareDiff(ctx *context.Context) {
 	if ctx.Written() {
 		return
 	}
+	defer headGitRepo.Close()
+
 	if err := parseBaseRepoInfo(ctx, headRepo); err != nil {
 		ctx.ServerError("parseBaseRepoInfo", err)
 		return
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 8269717e57..67849f33e1 100644
--- a/routers/repo/pull.go
+++ b/routers/repo/pull.go
@@ -352,6 +352,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)
 
@@ -534,6 +535,7 @@ func ViewPullFiles(ctx *context.Context) {
 			ctx.ServerError("OpenRepository", err)
 			return
 		}
+		defer headGitRepo.Close()
 
 		headCommitID, err := headGitRepo.GetBranchCommitID(pull.HeadBranch)
 		if err != nil {
@@ -748,6 +750,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() {
@@ -913,12 +916,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{}{
@@ -1047,6 +1052,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 f699c1d685..fa215357d2 100644
--- a/routers/repo/setting.go
+++ b/routers/repo/setting.go
@@ -73,6 +73,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 {
@@ -379,6 +384,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()