RepoAssignment ensure to close before overwrite (#19449) * check if GitRepo already open and close if * only run RepoAssignment once * refactor context helper for api to open GitRepo

Changes

11 changed files (+121/-96)

• modules/context/api.go
  • @@ -285,36 +285,6 @@ func APIContexter() func(http.Handler) http.Handler {
    } } - // ReferencesGitRepo injects the GitRepo into the Context - func ReferencesGitRepo(allowEmpty bool) func(ctx *APIContext) (cancel context.CancelFunc) { - return func(ctx *APIContext) (cancel context.CancelFunc) { - // Empty repository does not have reference information. - if !allowEmpty && ctx.Repo.Repository.IsEmpty { - return - } - - // For API calls. - if ctx.Repo.GitRepo == nil { - repoPath := repo_model.RepoPath(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name) - gitRepo, err := git.OpenRepository(ctx, repoPath) - if err != nil { - ctx.Error(http.StatusInternalServerError, "RepoRef Invalid repo "+repoPath, err) - return - } - ctx.Repo.GitRepo = gitRepo - // We opened it, we should close it - return func() { - // If it's been set to nil then assume someone else has closed it. - if ctx.Repo.GitRepo != nil { - ctx.Repo.GitRepo.Close() - } - } - } - - return - } - } - // NotFound handles 404s for APIContext // String will replace message, errors will be added to a slice func (ctx *APIContext) NotFound(objs ...interface{}) {
  • @@ -340,33 +310,62 @@ func (ctx *APIContext) NotFound(objs ...interface{}) {
    }) } - // RepoRefForAPI handles repository reference names when the ref name is not explicitly given - func RepoRefForAPI(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - ctx := GetAPIContext(req) + // ReferencesGitRepo injects the GitRepo into the Context + // you can optional skip the IsEmpty check + func ReferencesGitRepo(allowEmpty ...bool) func(ctx *APIContext) (cancel context.CancelFunc) { + return func(ctx *APIContext) (cancel context.CancelFunc) { // Empty repository does not have reference information. - if ctx.Repo.Repository.IsEmpty { + if ctx.Repo.Repository.IsEmpty && !(len(allowEmpty) != 0 && allowEmpty[0]) { return } - var err error - + // For API calls. if ctx.Repo.GitRepo == nil { repoPath := repo_model.RepoPath(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name) - ctx.Repo.GitRepo, err = git.OpenRepository(ctx, repoPath) + gitRepo, err := git.OpenRepository(ctx, repoPath) if err != nil { - ctx.InternalServerError(err) + ctx.Error(http.StatusInternalServerError, "RepoRef Invalid repo "+repoPath, err) return } + ctx.Repo.GitRepo = gitRepo // We opened it, we should close it - defer func() { + return func() { // If it's been set to nil then assume someone else has closed it. if ctx.Repo.GitRepo != nil { ctx.Repo.GitRepo.Close() } - }() + } + } + + return + } + } + + // RepoRefForAPI handles repository reference names when the ref name is not explicitly given + func RepoRefForAPI(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + ctx := GetAPIContext(req) + + if ctx.Repo.GitRepo == nil { + ctx.InternalServerError(fmt.Errorf("no open git repo")) + return + } + + if ref := ctx.FormTrim("ref"); len(ref) > 0 { + commit, err := ctx.Repo.GitRepo.GetCommit(ref) + if err != nil { + if git.IsErrNotExist(err) { + ctx.NotFound() + } else { + ctx.Error(http.StatusInternalServerError, "GetBlobByPath", err) + } + return + } + ctx.Repo.Commit = commit + return } + var err error refName := getRefName(ctx.Context, RepoRefAny) if ctx.Repo.GitRepo.IsBranchExist(refName) {
• modules/context/repo.go
  • @@ -221,13 +221,21 @@ func (r *Repository) FileExists(path, branch string) (bool, error) {
    // GetEditorconfig returns the .editorconfig definition if found in the // HEAD of the default repo branch. - func (r *Repository) GetEditorconfig() (*editorconfig.Editorconfig, error) { + func (r *Repository) GetEditorconfig(optCommit ...*git.Commit) (*editorconfig.Editorconfig, error) { if r.GitRepo == nil { return nil, nil } - commit, err := r.GitRepo.GetBranchCommit(r.Repository.DefaultBranch) - if err != nil { - return nil, err + var ( + err error + commit *git.Commit + ) + if len(optCommit) != 0 { + commit = optCommit[0] + } else { + commit, err = r.GitRepo.GetBranchCommit(r.Repository.DefaultBranch) + if err != nil { + return nil, err + } } treeEntry, err := commit.GetTreeEntryByPath(".editorconfig") if err != nil {
  • @@ -407,6 +415,12 @@ func RepoIDAssignment() func(ctx *Context) {
    // RepoAssignment returns a middleware to handle repository assignment func RepoAssignment(ctx *Context) (cancel context.CancelFunc) { + if _, repoAssignmentOnce := ctx.Data["repoAssignmentExecuted"]; repoAssignmentOnce { + log.Trace("RepoAssignment was exec already, skipping second call ...") + return + } + ctx.Data["repoAssignmentExecuted"] = true + var ( owner *user_model.User err error
  • @@ -602,6 +616,9 @@ func RepoAssignment(ctx *Context) (cancel context.CancelFunc) {
    ctx.ServerError("RepoAssignment Invalid repo "+repo_model.RepoPath(userName, repoName), err) return } + if ctx.Repo.GitRepo != nil { + ctx.Repo.GitRepo.Close() + } ctx.Repo.GitRepo = gitRepo // We opened it, we should close it
• routers/api/v1/api.go
  • @@ -796,7 +796,7 @@ func Routes() *web.Route {
    m.Combo("").Get(repo.GetHook). Patch(bind(api.EditHookOption{}), repo.EditHook). Delete(repo.DeleteHook) - m.Post("/tests", context.RepoRefForAPI, repo.TestHook) + m.Post("/tests", context.ReferencesGitRepo(), context.RepoRefForAPI, repo.TestHook) }) }, reqToken(), reqAdmin(), reqWebhooksEnabled()) m.Group("/collaborators", func() {
  • @@ -813,16 +813,16 @@ func Routes() *web.Route {
    Put(reqAdmin(), repo.AddTeam). Delete(reqAdmin(), repo.DeleteTeam) }, reqToken()) - m.Get("/raw/*", context.RepoRefForAPI, reqRepoReader(unit.TypeCode), repo.GetRawFile) + m.Get("/raw/*", context.ReferencesGitRepo(), context.RepoRefForAPI, reqRepoReader(unit.TypeCode), repo.GetRawFile) m.Get("/archive/*", reqRepoReader(unit.TypeCode), repo.GetArchive) m.Combo("/forks").Get(repo.ListForks). Post(reqToken(), reqRepoReader(unit.TypeCode), bind(api.CreateForkOption{}), repo.CreateFork) m.Group("/branches", func() { - m.Get("", context.ReferencesGitRepo(false), repo.ListBranches) - m.Get("/*", context.ReferencesGitRepo(false), repo.GetBranch) - m.Delete("/*", reqRepoWriter(unit.TypeCode), context.ReferencesGitRepo(false), repo.DeleteBranch) - m.Post("", reqRepoWriter(unit.TypeCode), context.ReferencesGitRepo(false), bind(api.CreateBranchRepoOption{}), repo.CreateBranch) - }, reqRepoReader(unit.TypeCode)) + m.Get("", repo.ListBranches) + m.Get("/*", repo.GetBranch) + m.Delete("/*", reqRepoWriter(unit.TypeCode), repo.DeleteBranch) + m.Post("", reqRepoWriter(unit.TypeCode), bind(api.CreateBranchRepoOption{}), repo.CreateBranch) + }, context.ReferencesGitRepo(), reqRepoReader(unit.TypeCode)) m.Group("/branch_protections", func() { m.Get("", repo.ListBranchProtections) m.Post("", bind(api.CreateBranchProtectionOption{}), repo.CreateBranchProtection)
  • @@ -941,10 +941,10 @@ func Routes() *web.Route {
    }) m.Group("/releases", func() { m.Combo("").Get(repo.ListReleases). - Post(reqToken(), reqRepoWriter(unit.TypeReleases), context.ReferencesGitRepo(false), bind(api.CreateReleaseOption{}), repo.CreateRelease) + Post(reqToken(), reqRepoWriter(unit.TypeReleases), context.ReferencesGitRepo(), bind(api.CreateReleaseOption{}), repo.CreateRelease) m.Group("/{id}", func() { m.Combo("").Get(repo.GetRelease). - Patch(reqToken(), reqRepoWriter(unit.TypeReleases), context.ReferencesGitRepo(false), bind(api.EditReleaseOption{}), repo.EditRelease). + Patch(reqToken(), reqRepoWriter(unit.TypeReleases), context.ReferencesGitRepo(), bind(api.EditReleaseOption{}), repo.EditRelease). Delete(reqToken(), reqRepoWriter(unit.TypeReleases), repo.DeleteRelease) m.Group("/assets", func() { m.Combo("").Get(repo.ListReleaseAttachments).
  • @@ -961,7 +961,7 @@ func Routes() *web.Route {
    }) }, reqRepoReader(unit.TypeReleases)) m.Post("/mirror-sync", reqToken(), reqRepoWriter(unit.TypeCode), repo.MirrorSync) - m.Get("/editorconfig/{filename}", context.RepoRefForAPI, reqRepoReader(unit.TypeCode), repo.GetEditorconfig) + m.Get("/editorconfig/{filename}", context.ReferencesGitRepo(), context.RepoRefForAPI, reqRepoReader(unit.TypeCode), repo.GetEditorconfig) m.Group("/pulls", func() { m.Combo("").Get(repo.ListPullRequests). Post(reqToken(), mustNotBeArchived, bind(api.CreatePullRequestOption{}), repo.CreatePullRequest)
  • @@ -992,13 +992,13 @@ func Routes() *web.Route {
    Delete(reqToken(), bind(api.PullReviewRequestOptions{}), repo.DeleteReviewRequests). Post(reqToken(), bind(api.PullReviewRequestOptions{}), repo.CreateReviewRequests) }) - }, mustAllowPulls, reqRepoReader(unit.TypeCode), context.ReferencesGitRepo(false)) + }, mustAllowPulls, reqRepoReader(unit.TypeCode), context.ReferencesGitRepo()) m.Group("/statuses", func() { m.Combo("/{sha}").Get(repo.GetCommitStatuses). Post(reqToken(), bind(api.CreateStatusOption{}), repo.NewCommitStatus) }, reqRepoReader(unit.TypeCode)) m.Group("/commits", func() { - m.Get("", context.ReferencesGitRepo(false), repo.GetAllCommits) + m.Get("", context.ReferencesGitRepo(), repo.GetAllCommits) m.Group("/{ref}", func() { m.Get("/status", repo.GetCombinedCommitStatusByRef) m.Get("/statuses", repo.GetCommitStatusesByRef)
  • @@ -1006,16 +1006,16 @@ func Routes() *web.Route {
    }, reqRepoReader(unit.TypeCode)) m.Group("/git", func() { m.Group("/commits", func() { - m.Get("/{sha}", context.ReferencesGitRepo(false), repo.GetSingleCommit) + m.Get("/{sha}", repo.GetSingleCommit) m.Get("/{sha}.{diffType:diff|patch}", repo.DownloadCommitDiffOrPatch) }) m.Get("/refs", repo.GetGitAllRefs) m.Get("/refs/*", repo.GetGitRefs) - m.Get("/trees/{sha}", context.RepoRefForAPI, repo.GetTree) - m.Get("/blobs/{sha}", context.RepoRefForAPI, repo.GetBlob) - m.Get("/tags/{sha}", context.RepoRefForAPI, repo.GetAnnotatedTag) + m.Get("/trees/{sha}", repo.GetTree) + m.Get("/blobs/{sha}", repo.GetBlob) + m.Get("/tags/{sha}", repo.GetAnnotatedTag) m.Get("/notes/{sha}", repo.GetNote) - }, reqRepoReader(unit.TypeCode)) + }, context.ReferencesGitRepo(), reqRepoReader(unit.TypeCode)) m.Post("/diffpatch", reqRepoWriter(unit.TypeCode), reqToken(), bind(api.ApplyDiffPatchFileOptions{}), repo.ApplyDiffPatch) m.Group("/contents", func() { m.Get("", repo.GetContentsList)
  • @@ -1035,7 +1035,7 @@ func Routes() *web.Route {
    Delete(reqToken(), repo.DeleteTopic) }, reqAdmin()) }, reqAnyRepoReader()) - m.Get("/issue_templates", context.ReferencesGitRepo(false), repo.GetIssueTemplates) + m.Get("/issue_templates", context.ReferencesGitRepo(), repo.GetIssueTemplates) m.Get("/languages", reqRepoReader(unit.TypeCode), repo.GetLanguages) }, repoAssignment()) })
• routers/api/v1/repo/blob.go
  • @@ -45,7 +45,8 @@ func GetBlob(ctx *context.APIContext) {
    ctx.Error(http.StatusBadRequest, "", "sha not provided") return } - if blob, err := files_service.GetBlobBySHA(ctx, ctx.Repo.Repository, sha); err != nil { + + if blob, err := files_service.GetBlobBySHA(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, sha); err != nil { ctx.Error(http.StatusBadRequest, "", err) } else { ctx.JSON(http.StatusOK, blob)
• routers/api/v1/repo/commits.go
  • @@ -269,6 +269,7 @@ func DownloadCommitDiffOrPatch(ctx *context.APIContext) {
    // "404": // "$ref": "#/responses/notFound" repoPath := repo_model.RepoPath(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name) + // TODO: use gitRepo from context if err := git.GetRawDiff( ctx, repoPath,
• routers/api/v1/repo/file.go
  • @@ -62,22 +62,7 @@ func GetRawFile(ctx *context.APIContext) {
    return } - commit := ctx.Repo.Commit - - if ref := ctx.FormTrim("ref"); len(ref) > 0 { - var err error - commit, err = ctx.Repo.GitRepo.GetCommit(ref) - if err != nil { - if git.IsErrNotExist(err) { - ctx.NotFound() - } else { - ctx.Error(http.StatusInternalServerError, "GetBlobByPath", err) - } - return - } - } - - blob, err := commit.GetBlobByPath(ctx.Repo.TreePath) + blob, err := ctx.Repo.Commit.GetBlobByPath(ctx.Repo.TreePath) if err != nil { if git.IsErrNotExist(err) { ctx.NotFound()
  • @@ -157,13 +142,18 @@ func GetEditorconfig(ctx *context.APIContext) {
    // description: filepath of file to get // type: string // required: true + // - name: ref + // in: query + // description: "The name of the commit/branch/tag. Default the repository’s default branch (usually master)" + // type: string + // required: false // responses: // 200: // description: success // "404": // "$ref": "#/responses/notFound" - ec, err := ctx.Repo.GetEditorconfig() + ec, err := ctx.Repo.GetEditorconfig(ctx.Repo.Commit) if err != nil { if git.IsErrNotExist(err) { ctx.NotFound(err)
• routers/api/v1/repo/hook.go
  • @@ -138,6 +138,11 @@ func TestHook(ctx *context.APIContext) {
    // type: integer // format: int64 // required: true + // - name: ref + // in: query + // description: "The name of the commit/branch/tag. Default the repository’s default branch (usually master)" + // type: string + // required: false // responses: // "204": // "$ref": "#/responses/empty"
• routers/api/v1/repo/notes.go
  • @@ -55,15 +55,13 @@ func GetNote(ctx *context.APIContext) {
    } func getNote(ctx *context.APIContext, identifier string) { - gitRepo, err := git.OpenRepository(ctx, ctx.Repo.Repository.RepoPath()) - if err != nil { - ctx.Error(http.StatusInternalServerError, "OpenRepository", err) + if ctx.Repo.GitRepo == nil { + ctx.InternalServerError(fmt.Errorf("no open git repo")) return } - defer gitRepo.Close() + var note git.Note - err = git.GetNote(ctx, gitRepo, identifier, &note) - if err != nil { + if err := git.GetNote(ctx, ctx.Repo.GitRepo, identifier, &note); err != nil { if git.IsErrNotExist(err) { ctx.NotFound(identifier) return
  • @@ -72,7 +70,7 @@ func getNote(ctx *context.APIContext, identifier string) {
    return } - cmt, err := convert.ToCommit(ctx.Repo.Repository, gitRepo, note.Commit, nil) + cmt, err := convert.ToCommit(ctx.Repo.Repository, ctx.Repo.GitRepo, note.Commit, nil) if err != nil { ctx.Error(http.StatusInternalServerError, "ToCommit", err) return
• services/repository/files/content.go
  • @@ -164,7 +164,7 @@ func GetContents(ctx context.Context, repo *repo_model.Repository, treePath, ref
    // Now populate the rest of the ContentsResponse based on entry type if entry.IsRegular() || entry.IsExecutable() { contentsResponse.Type = string(ContentTypeRegular) - if blobResponse, err := GetBlobBySHA(ctx, repo, entry.ID.String()); err != nil { + if blobResponse, err := GetBlobBySHA(ctx, repo, gitRepo, entry.ID.String()); err != nil { return nil, err } else if !forList { // We don't show the content if we are getting a list of FileContentResponses
  • @@ -220,12 +220,7 @@ func GetContents(ctx context.Context, repo *repo_model.Repository, treePath, ref
    } // GetBlobBySHA get the GitBlobResponse of a repository using a sha hash. - func GetBlobBySHA(ctx context.Context, repo *repo_model.Repository, sha string) (*api.GitBlobResponse, error) { - gitRepo, closer, err := git.RepositoryFromContextOrOpen(ctx, repo.RepoPath()) - if err != nil { - return nil, err - } - defer closer.Close() + func GetBlobBySHA(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, sha string) (*api.GitBlobResponse, error) { gitBlob, err := gitRepo.GetBlob(sha) if err != nil { return nil, err
• services/repository/files/content_test.go
  • @@ -8,7 +8,9 @@ import (
    "path/filepath" "testing" + repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/modules/git" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test"
  • @@ -234,7 +236,12 @@ func TestGetBlobBySHA(t *testing.T) {
    ctx.SetParams(":id", "1") ctx.SetParams(":sha", sha) - gbr, err := GetBlobBySHA(ctx, ctx.Repo.Repository, ctx.Params(":sha")) + gitRepo, err := git.OpenRepository(ctx, repo_model.RepoPath(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name)) + if err != nil { + t.Fail() + } + + gbr, err := GetBlobBySHA(ctx, ctx.Repo.Repository, gitRepo, ctx.Params(":sha")) expectedGBR := &api.GitBlobResponse{ Content: "dHJlZSAyYTJmMWQ0NjcwNzI4YTJlMTAwNDllMzQ1YmQ3YTI3NjQ2OGJlYWI2CmF1dGhvciB1c2VyMSA8YWRkcmVzczFAZXhhbXBsZS5jb20+IDE0ODk5NTY0NzkgLTA0MDAKY29tbWl0dGVyIEV0aGFuIEtvZW5pZyA8ZXRoYW50a29lbmlnQGdtYWlsLmNvbT4gMTQ4OTk1NjQ3OSAtMDQwMAoKSW5pdGlhbCBjb21taXQK", Encoding: "base64",
• templates/swagger/v1_json.tmpl
  • @@ -3668,6 +3668,12 @@
    "name": "filepath", "in": "path", "required": true + }, + { + "type": "string", + "description": "The name of the commit/branch/tag. Default the repository’s default branch (usually master)", + "name": "ref", + "in": "query" } ], "responses": {
  • @@ -4559,6 +4565,12 @@
    "name": "id", "in": "path", "required": true + }, + { + "type": "string", + "description": "The name of the commit/branch/tag. Default the repository’s default branch (usually master)", + "name": "ref", + "in": "query" } ], "responses": {