From c764355676eb6d67674d095f92576a85688fe6cb Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 21 Apr 2022 17:17:57 +0200 Subject: [PATCH] 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 --- modules/context/api.go | 81 +++++++++++------------ modules/context/repo.go | 25 +++++-- routers/api/v1/api.go | 36 +++++----- routers/api/v1/repo/blob.go | 3 +- routers/api/v1/repo/commits.go | 1 + routers/api/v1/repo/file.go | 24 ++----- routers/api/v1/repo/hook.go | 5 ++ routers/api/v1/repo/notes.go | 12 ++-- services/repository/files/content.go | 9 +-- services/repository/files/content_test.go | 9 ++- templates/swagger/v1_json.tmpl | 12 ++++ 11 files changed, 121 insertions(+), 96 deletions(-) diff --git a/modules/context/api.go b/modules/context/api.go index e5c2eeda0a..41d559f5b1 100644 --- a/modules/context/api.go +++ b/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) { diff --git a/modules/context/repo.go b/modules/context/repo.go index a7c9a982c4..4687434455 100644 --- a/modules/context/repo.go +++ b/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 diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index a430eb453a..aec2a6d7b2 100644 --- a/routers/api/v1/api.go +++ b/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()) }) diff --git a/routers/api/v1/repo/blob.go b/routers/api/v1/repo/blob.go index 19d893a68b..035f2dc1e1 100644 --- a/routers/api/v1/repo/blob.go +++ b/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) diff --git a/routers/api/v1/repo/commits.go b/routers/api/v1/repo/commits.go index b6c47e0685..c79c34ec42 100644 --- a/routers/api/v1/repo/commits.go +++ b/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, diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index a4811b370a..ed51f6f4df 100644 --- a/routers/api/v1/repo/file.go +++ b/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) diff --git a/routers/api/v1/repo/hook.go b/routers/api/v1/repo/hook.go index c79a1d6b13..7ec6cd88ab 100644 --- a/routers/api/v1/repo/hook.go +++ b/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" diff --git a/routers/api/v1/repo/notes.go b/routers/api/v1/repo/notes.go index f85883566f..bd8e27e40b 100644 --- a/routers/api/v1/repo/notes.go +++ b/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, ¬e) - if err != nil { + if err := git.GetNote(ctx, ctx.Repo.GitRepo, identifier, ¬e); 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 diff --git a/services/repository/files/content.go b/services/repository/files/content.go index 9037a84349..2237671a60 100644 --- a/services/repository/files/content.go +++ b/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 diff --git a/services/repository/files/content_test.go b/services/repository/files/content_test.go index 8a3e589bdf..342ebae329 100644 --- a/services/repository/files/content_test.go +++ b/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", diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index de74fd8fa3..0374a53a65 100644 --- a/templates/swagger/v1_json.tmpl +++ b/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": {