finn/forgejo
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Add Close() method to gogitRepository (#8901)

Browse source 
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
This commit is contained in:
zeripath 2019-11-13 07:01:19 +00:00 • committed by GitHub
parent 7b97e04555
commit 722a7c902d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
75 changed files with 391 additions and 106 deletions
Download patch file Download diff file Expand all files Collapse all files

2
routers/api/v1/api.go
View file

@ -641,7 +641,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)

2
routers/api/v1/repo/commits.go
View file

@ -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 {

1
routers/api/v1/repo/file.go
View file

@ -95,6 +95,7 @@ func GetArchive(ctx *context.APIContext) {
return
}
ctx.Repo.GitRepo = gitRepo
defer gitRepo.Close()
repo.Download(ctx.Context)
}

2
routers/api/v1/repo/git_ref.go
View file

@ -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
}

7
routers/api/v1/repo/pull.go
View file

@ -195,6 +195,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)
@ -722,6 +723,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, "", ""
}
@ -732,6 +734,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, "", ""
}
@ -739,6 +742,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, "", ""
}
@ -749,18 +753,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, "", ""
}

2
routers/api/v1/repo/tag.go
View file

@ -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

11
routers/repo/compare.go
View file

@ -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

5
routers/repo/editor_test.go
View file

@ -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 := ""

6
routers/repo/pull.go
View file

@ -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 {

1
routers/repo/release_test.go
View file

@ -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()
}
}

10
routers/repo/setting.go
View file

@ -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)

70
routers/repo/wiki.go
View file

@ -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)
}

1
routers/repo/wiki_test.go
View file

@ -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()