From 0453177b611cab92d8ee093012c5bc33d364b41d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 5 Mar 2025 11:15:40 -0800 Subject: [PATCH] Refactor: move part of updating protected branch logic to service layer (#33742) --- routers/api/v1/repo/branch.go | 38 +--------------- routers/web/repo/setting/protected_branch.go | 20 ++------ services/forms/repo_form.go | 7 --- services/pull/protected_branch.go | 48 ++++++++++++++++++++ 4 files changed, 52 insertions(+), 61 deletions(-) create mode 100644 services/pull/protected_branch.go diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 5430c1a266..f866f2048e 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -594,12 +594,6 @@ func CreateBranchProtection(ctx *context.APIContext) { return } - isPlainRule := !git_model.IsRuleNameSpecial(ruleName) - var isBranchExist bool - if isPlainRule { - isBranchExist = git.IsBranchExist(ctx.Req.Context(), ctx.Repo.Repository.RepoPath(), ruleName) - } - protectBranch, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, ruleName) if err != nil { ctx.APIErrorInternal(err) @@ -716,7 +710,7 @@ func CreateBranchProtection(ctx *context.APIContext) { BlockAdminMergeOverride: form.BlockAdminMergeOverride, } - if err := git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{ + if err := pull_service.CreateOrUpdateProtectedBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{ UserIDs: whitelistUsers, TeamIDs: whitelistTeams, ForcePushUserIDs: forcePushAllowlistUsers, @@ -730,36 +724,6 @@ func CreateBranchProtection(ctx *context.APIContext) { return } - if isBranchExist { - if err := pull_service.CheckPRsForBaseBranch(ctx, ctx.Repo.Repository, ruleName); err != nil { - ctx.APIErrorInternal(err) - return - } - } else { - if !isPlainRule { - if ctx.Repo.GitRepo == nil { - ctx.Repo.GitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, ctx.Repo.Repository) - if err != nil { - ctx.APIErrorInternal(err) - return - } - } - // FIXME: since we only need to recheck files protected rules, we could improve this - matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.Repository.ID, ruleName) - if err != nil { - ctx.APIErrorInternal(err) - return - } - - for _, branchName := range matchedBranches { - if err = pull_service.CheckPRsForBaseBranch(ctx, ctx.Repo.Repository, branchName); err != nil { - ctx.APIErrorInternal(err) - return - } - } - } - } - // Reload from db to get all whitelists bp, err := git_model.GetProtectedBranchRuleByName(ctx, ctx.Repo.Repository.ID, ruleName) if err != nil { diff --git a/routers/web/repo/setting/protected_branch.go b/routers/web/repo/setting/protected_branch.go index 6709bd115c..75de2ba1e7 100644 --- a/routers/web/repo/setting/protected_branch.go +++ b/routers/web/repo/setting/protected_branch.go @@ -261,7 +261,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) { protectBranch.BlockOnOutdatedBranch = f.BlockOnOutdatedBranch protectBranch.BlockAdminMergeOverride = f.BlockAdminMergeOverride - err = git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{ + if err = pull_service.CreateOrUpdateProtectedBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{ UserIDs: whitelistUsers, TeamIDs: whitelistTeams, ForcePushUserIDs: forcePushAllowlistUsers, @@ -270,25 +270,11 @@ func SettingsProtectedBranchPost(ctx *context.Context) { MergeTeamIDs: mergeWhitelistTeams, ApprovalsUserIDs: approvalsWhitelistUsers, ApprovalsTeamIDs: approvalsWhitelistTeams, - }) - if err != nil { - ctx.ServerError("UpdateProtectBranch", err) + }); err != nil { + ctx.ServerError("CreateOrUpdateProtectedBranch", err) return } - // FIXME: since we only need to recheck files protected rules, we could improve this - matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.Repository.ID, protectBranch.RuleName) - if err != nil { - ctx.ServerError("FindAllMatchedBranches", err) - return - } - for _, branchName := range matchedBranches { - if err = pull_service.CheckPRsForBaseBranch(ctx, ctx.Repo.Repository, branchName); err != nil { - ctx.ServerError("CheckPRsForBaseBranch", err) - return - } - } - ctx.Flash.Success(ctx.Tr("repo.settings.update_protect_branch_success", protectBranch.RuleName)) ctx.Redirect(fmt.Sprintf("%s/settings/branches?rule_name=%s", ctx.Repo.RepoLink, protectBranch.RuleName)) } diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 70019f3fa9..f07186117e 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -170,13 +170,6 @@ func (f *RepoSettingForm) Validate(req *http.Request, errs binding.Errors) bindi return middleware.Validate(errs, ctx.Data, f, ctx.Locale) } -// __________ .__ -// \______ \____________ ____ ____ | |__ -// | | _/\_ __ \__ \ / \_/ ___\| | \ -// | | \ | | \// __ \| | \ \___| Y \ -// |______ / |__| (____ /___| /\___ >___| / -// \/ \/ \/ \/ \/ - // ProtectBranchForm form for changing protected branch settings type ProtectBranchForm struct { RuleName string `binding:"Required"` diff --git a/services/pull/protected_branch.go b/services/pull/protected_branch.go new file mode 100644 index 0000000000..5f42629ddc --- /dev/null +++ b/services/pull/protected_branch.go @@ -0,0 +1,48 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package pull + +import ( + "context" + + git_model "code.gitea.io/gitea/models/git" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/modules/git" +) + +func CreateOrUpdateProtectedBranch(ctx context.Context, repo *repo_model.Repository, + protectBranch *git_model.ProtectedBranch, whitelistOptions git_model.WhitelistOptions, +) error { + err := git_model.UpdateProtectBranch(ctx, repo, protectBranch, whitelistOptions) + if err != nil { + return err + } + + isPlainRule := !git_model.IsRuleNameSpecial(protectBranch.RuleName) + var isBranchExist bool + if isPlainRule { + isBranchExist = git.IsBranchExist(ctx, repo.RepoPath(), protectBranch.RuleName) + } + + if isBranchExist { + if err := CheckPRsForBaseBranch(ctx, repo, protectBranch.RuleName); err != nil { + return err + } + } else { + if !isPlainRule { + // FIXME: since we only need to recheck files protected rules, we could improve this + matchedBranches, err := git_model.FindAllMatchedBranches(ctx, repo.ID, protectBranch.RuleName) + if err != nil { + return err + } + for _, branchName := range matchedBranches { + if err = CheckPRsForBaseBranch(ctx, repo, branchName); err != nil { + return err + } + } + } + } + + return nil +}