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 1/4] 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 e5c2eeda0a334..41d559f5b1c5f 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 a7c9a982c42b2..4687434455a39 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 a430eb453aa9b..aec2a6d7b2c42 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 19d893a68b92e..035f2dc1e1de9 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 b6c47e0685186..c79c34ec42961 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 a4811b370a911..ed51f6f4df2a2 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 c79a1d6b13522..7ec6cd88aba63 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 f85883566f012..bd8e27e40bf40 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 9037a84349e5a..2237671a60cbb 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 8a3e589bdf9b1..342ebae329168 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 de74fd8fa399e..0374a53a6555c 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": { From 3ec1b6c2238c9eb46709091567eb2564aec86d99 Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 21 Apr 2022 16:05:53 +0000 Subject: [PATCH 2/4] Fix logging of Transfer API (#19456) - Use the correct fullname's in tracing calls. - Return correct function name in error. Co-authored-by: 6543 <6543@obermui.de> --- routers/api/v1/repo/transfer.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/routers/api/v1/repo/transfer.go b/routers/api/v1/repo/transfer.go index 7578fbd1873ce..241c578e608a6 100644 --- a/routers/api/v1/repo/transfer.go +++ b/routers/api/v1/repo/transfer.go @@ -104,14 +104,16 @@ func Transfer(ctx *context.APIContext) { ctx.Repo.GitRepo = nil } + oldFullname := ctx.Repo.Repository.FullName() + if err := repo_service.StartRepositoryTransfer(ctx.Doer, newOwner, ctx.Repo.Repository, teams); err != nil { if models.IsErrRepoTransferInProgress(err) { - ctx.Error(http.StatusConflict, "CreatePendingRepositoryTransfer", err) + ctx.Error(http.StatusConflict, "StartRepositoryTransfer", err) return } if repo_model.IsErrRepoAlreadyExist(err) { - ctx.Error(http.StatusUnprocessableEntity, "CreatePendingRepositoryTransfer", err) + ctx.Error(http.StatusUnprocessableEntity, "StartRepositoryTransfer", err) return } @@ -120,12 +122,12 @@ func Transfer(ctx *context.APIContext) { } if ctx.Repo.Repository.Status == repo_model.RepositoryPendingTransfer { - log.Trace("Repository transfer initiated: %s -> %s", ctx.Repo.Repository.FullName(), newOwner.Name) + log.Trace("Repository transfer initiated: %s -> %s", oldFullname, ctx.Repo.Repository.FullName()) ctx.JSON(http.StatusCreated, convert.ToRepo(ctx.Repo.Repository, perm.AccessModeAdmin)) return } - log.Trace("Repository transferred: %s -> %s", ctx.Repo.Repository.FullName(), newOwner.Name) + log.Trace("Repository transferred: %s -> %s", oldFullname, ctx.Repo.Repository.FullName()) ctx.JSON(http.StatusAccepted, convert.ToRepo(ctx.Repo.Repository, perm.AccessModeAdmin)) } From ebe569a268bbe71bf2bc30cd2829227700688b57 Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 21 Apr 2022 21:55:45 +0000 Subject: [PATCH 3/4] Set correct PR status on 3way on conflict checking (#19457) * Set correct PR status on 3way on conflict checking - When 3-way merge is enabled for conflict checking, it has a new interesting behavior that it doesn't return any error when it found a conflict, so we change the condition to not check for the error, but instead check if conflictedfiles is populated, this fixes a issue whereby PR status wasn't correctly on conflicted PR's. - Refactor the mergeable property(which was incorrectly set and lead me this bug) to be more maintainable. - Add a dedicated test for conflicting checking, so it should prevent future issues with this. * Fix linter --- integrations/pull_merge_test.go | 73 +++++++++++++++++++++++++++++++++ models/pull.go | 11 +++++ modules/convert/pull.go | 5 +-- services/pull/patch.go | 6 ++- 4 files changed, 89 insertions(+), 6 deletions(-) diff --git a/integrations/pull_merge_test.go b/integrations/pull_merge_test.go index d7cb042e9c127..f8e8c79a8864e 100644 --- a/integrations/pull_merge_test.go +++ b/integrations/pull_merge_test.go @@ -26,6 +26,8 @@ import ( "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/translation/i18n" "code.gitea.io/gitea/services/pull" + repo_service "code.gitea.io/gitea/services/repository" + files_service "code.gitea.io/gitea/services/repository/files" "github.com/stretchr/testify/assert" ) @@ -346,3 +348,74 @@ func TestCantMergeUnrelated(t *testing.T) { gitRepo.Close() }) } + +func TestConflictChecking(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).(*user_model.User) + + // Create new clean repo to test conflict checking. + baseRepo, err := repo_service.CreateRepository(user, user, models.CreateRepoOptions{ + Name: "conflict-checking", + Description: "Tempo repo", + AutoInit: true, + Readme: "Default", + DefaultBranch: "main", + }) + assert.NoError(t, err) + assert.NotEmpty(t, baseRepo) + + // create a commit on new branch. + _, err = files_service.CreateOrUpdateRepoFile(git.DefaultContext, baseRepo, user, &files_service.UpdateRepoFileOptions{ + TreePath: "important_file", + Message: "Add a important file", + Content: "Just a non-important file", + IsNewFile: true, + OldBranch: "main", + NewBranch: "important-secrets", + }) + assert.NoError(t, err) + + // create a commit on main branch. + _, err = files_service.CreateOrUpdateRepoFile(git.DefaultContext, baseRepo, user, &files_service.UpdateRepoFileOptions{ + TreePath: "important_file", + Message: "Add a important file", + Content: "Not the same content :P", + IsNewFile: true, + OldBranch: "main", + NewBranch: "main", + }) + assert.NoError(t, err) + + // create Pull to merge the important-secrets branch into main branch. + pullIssue := &models.Issue{ + RepoID: baseRepo.ID, + Title: "PR with conflict!", + PosterID: user.ID, + Poster: user, + IsPull: true, + } + + pullRequest := &models.PullRequest{ + HeadRepoID: baseRepo.ID, + BaseRepoID: baseRepo.ID, + HeadBranch: "important-secrets", + BaseBranch: "main", + HeadRepo: baseRepo, + BaseRepo: baseRepo, + Type: models.PullRequestGitea, + } + err = pull.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil) + assert.NoError(t, err) + + issue := unittest.AssertExistsAndLoadBean(t, &models.Issue{Title: "PR with conflict!"}).(*models.Issue) + conflictingPR, err := models.GetPullRequestByIssueID(issue.ID) + assert.NoError(t, err) + + // Ensure conflictedFiles is populated. + assert.Equal(t, 1, len(conflictingPR.ConflictedFiles)) + // Check if status is correct. + assert.Equal(t, models.PullRequestStatusConflict, conflictingPR.Status) + // Ensure that mergeable returns false + assert.False(t, conflictingPR.Mergeable()) + }) +} diff --git a/models/pull.go b/models/pull.go index 439005deb4264..ac44ebf0bea24 100644 --- a/models/pull.go +++ b/models/pull.go @@ -701,3 +701,14 @@ func (pr *PullRequest) GetHeadBranchHTMLURL() string { } return pr.HeadRepo.HTMLURL() + "/src/branch/" + util.PathEscapeSegments(pr.HeadBranch) } + +// Mergeable returns if the pullrequest is mergeable. +func (pr *PullRequest) Mergeable() bool { + // If a pull request isn't mergable if it's: + // - Being conflict checked. + // - Has a conflict. + // - Received a error while being conflict checked. + // - Is a work-in-progress pull request. + return pr.Status != PullRequestStatusChecking && pr.Status != PullRequestStatusConflict && + pr.Status != PullRequestStatusError && !pr.IsWorkInProgress() +} diff --git a/modules/convert/pull.go b/modules/convert/pull.go index 9c53afe8f38d0..3b39e3d2c1269 100644 --- a/modules/convert/pull.go +++ b/modules/convert/pull.go @@ -68,6 +68,7 @@ func ToAPIPullRequest(ctx context.Context, pr *models.PullRequest, doer *user_mo PatchURL: pr.Issue.PatchURL(), HasMerged: pr.HasMerged, MergeBase: pr.MergeBase, + Mergeable: pr.Mergeable(), Deadline: apiIssue.Deadline, Created: pr.Issue.CreatedUnix.AsTimePtr(), Updated: pr.Issue.UpdatedUnix.AsTimePtr(), @@ -191,10 +192,6 @@ func ToAPIPullRequest(ctx context.Context, pr *models.PullRequest, doer *user_mo } } - if pr.Status != models.PullRequestStatusChecking { - mergeable := !(pr.Status == models.PullRequestStatusConflict || pr.Status == models.PullRequestStatusError) && !pr.IsWorkInProgress() - apiPullRequest.Mergeable = mergeable - } if pr.HasMerged { apiPullRequest.Merged = pr.MergedUnix.AsTimePtr() apiPullRequest.MergedCommitID = &pr.MergedCommitID diff --git a/services/pull/patch.go b/services/pull/patch.go index f86141aa7aecf..f118ef33d022a 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -444,14 +444,16 @@ func checkConflicts(ctx context.Context, pr *models.PullRequest, gitRepo *git.Re }, }) - // 9. If there is a conflict the `git apply` command will return a non-zero error code - so there will be a positive error. - if err != nil { + // 9. Check if the found conflictedfiles is non-zero, "err" could be non-nil, so we should ignore it if we found conflicts. + // Note: `"err" could be non-nil` is due that if enable 3-way merge, it doesn't return any error on found conflicts. + if len(pr.ConflictedFiles) > 0 { if conflict { pr.Status = models.PullRequestStatusConflict log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles) return true, nil } + } else if err != nil { return false, fmt.Errorf("git apply --check: %v", err) } return false, nil From 0dfc2e55ea258d2b1a3cd86e2b6f27a481e495ff Mon Sep 17 00:00:00 2001 From: GiteaBot Date: Fri, 22 Apr 2022 00:10:36 +0000 Subject: [PATCH 4/4] [skip ci] Updated translations via Crowdin --- options/locale/locale_pt-BR.ini | 1 + 1 file changed, 1 insertion(+) diff --git a/options/locale/locale_pt-BR.ini b/options/locale/locale_pt-BR.ini index af6a293460737..99e526269cb71 100644 --- a/options/locale/locale_pt-BR.ini +++ b/options/locale/locale_pt-BR.ini @@ -1944,6 +1944,7 @@ settings.event_pull_request_review_desc=Pull request aprovado, rejeitado ou revi settings.event_pull_request_sync=Pull Request Sincronizado settings.event_pull_request_sync_desc=Pull request sincronizado. settings.event_package=Pacote +settings.event_package_desc=Pacote criado ou excluído em um repositório. settings.branch_filter=Filtro de branch settings.branch_filter_desc=Lista dos branches a serem considerados nos eventos push, criação de branch e exclusão de branch, especificados como padrão glob. Se estiver vazio ou for *, eventos para todos os branches serão relatados. Veja github.com/gobwas/glob documentação da sintaxe. Exemplos: master, {master,release*}. settings.active=Ativo