From ce2ade05e6124993312e2e8f988687d45598032e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 8 Jun 2021 00:27:41 +0800 Subject: [PATCH] Merge all deleteBranch as one function and also fix bug when delete branch don't close related PRs (#16067) (#16097) * Fix bug when delete branch don't close related PRs * Merge all deletebranch as one method Co-authored-by: Lauris BH --- routers/api/v1/repo/branch.go | 62 +++++----------------------- routers/repo/branch.go | 77 ++++++++--------------------------- routers/repo/pull.go | 46 ++++++--------------- services/repository/branch.go | 72 ++++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 146 deletions(-) create mode 100644 services/repository/branch.go diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 451fdcf51..85c1681df 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -6,6 +6,7 @@ package repo import ( + "errors" "fmt" "net/http" @@ -13,7 +14,6 @@ import ( "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/convert" "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/modules/log" repo_module "code.gitea.io/gitea/modules/repository" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/web" @@ -117,62 +117,20 @@ func DeleteBranch(ctx *context.APIContext) { branchName := ctx.Params("*") - if ctx.Repo.Repository.DefaultBranch == branchName { - ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch")) - return - } - - isProtected, err := ctx.Repo.Repository.IsProtectedBranch(branchName, ctx.User) - if err != nil { - ctx.InternalServerError(err) - return - } - if isProtected { - ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected")) - return - } - - branch, err := repo_module.GetBranch(ctx.Repo.Repository, branchName) - if err != nil { - if git.IsErrBranchNotExist(err) { + if err := repo_service.DeleteBranch(ctx.User, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName); err != nil { + switch { + case git.IsErrBranchNotExist(err): ctx.NotFound(err) - } else { - ctx.Error(http.StatusInternalServerError, "GetBranch", err) + case errors.Is(err, repo_service.ErrBranchIsDefault): + ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch")) + case errors.Is(err, repo_service.ErrBranchIsProtected): + ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected")) + default: + ctx.Error(http.StatusInternalServerError, "DeleteBranch", err) } return } - c, err := branch.GetCommit() - if err != nil { - ctx.Error(http.StatusInternalServerError, "GetCommit", err) - return - } - - if err := ctx.Repo.GitRepo.DeleteBranch(branchName, git.DeleteBranchOptions{ - Force: true, - }); err != nil { - ctx.Error(http.StatusInternalServerError, "DeleteBranch", err) - return - } - - // Don't return error below this - if err := repo_service.PushUpdate( - &repo_module.PushUpdateOptions{ - RefFullName: git.BranchPrefix + branchName, - OldCommitID: c.ID.String(), - NewCommitID: git.EmptySHA, - PusherID: ctx.User.ID, - PusherName: ctx.User.Name, - RepoUserName: ctx.Repo.Owner.Name, - RepoName: ctx.Repo.Repository.Name, - }); err != nil { - log.Error("Update: %v", err) - } - - if err := ctx.Repo.Repository.AddDeletedBranch(branchName, c.ID.String(), ctx.User.ID); err != nil { - log.Warn("AddDeletedBranch: %v", err) - } - ctx.Status(http.StatusNoContent) } diff --git a/routers/repo/branch.go b/routers/repo/branch.go index ac6b7a1be..019d9e997 100644 --- a/routers/repo/branch.go +++ b/routers/repo/branch.go @@ -6,6 +6,7 @@ package repo import ( + "errors" "fmt" "strings" @@ -82,34 +83,23 @@ func Branches(ctx *context.Context) { func DeleteBranchPost(ctx *context.Context) { defer redirect(ctx) branchName := ctx.Query("name") - if branchName == ctx.Repo.Repository.DefaultBranch { - log.Debug("DeleteBranch: Can't delete default branch '%s'", branchName) - ctx.Flash.Error(ctx.Tr("repo.branch.default_deletion_failed", branchName)) - return - } - isProtected, err := ctx.Repo.Repository.IsProtectedBranch(branchName, ctx.User) - if err != nil { - log.Error("DeleteBranch: %v", err) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", branchName)) - return - } + if err := repo_service.DeleteBranch(ctx.User, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName); err != nil { + switch { + case git.IsErrBranchNotExist(err): + log.Debug("DeleteBranch: Can't delete non existing branch '%s'", branchName) + ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", branchName)) + case errors.Is(err, repo_service.ErrBranchIsDefault): + log.Debug("DeleteBranch: Can't delete default branch '%s'", branchName) + ctx.Flash.Error(ctx.Tr("repo.branch.default_deletion_failed", branchName)) + case errors.Is(err, repo_service.ErrBranchIsProtected): + log.Debug("DeleteBranch: Can't delete protected branch '%s'", branchName) + ctx.Flash.Error(ctx.Tr("repo.branch.protected_deletion_failed", branchName)) + default: + log.Error("DeleteBranch: %v", err) + ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", branchName)) + } - if isProtected { - log.Debug("DeleteBranch: Can't delete protected branch '%s'", branchName) - ctx.Flash.Error(ctx.Tr("repo.branch.protected_deletion_failed", branchName)) - return - } - - if !ctx.Repo.GitRepo.IsBranchExist(branchName) { - log.Debug("DeleteBranch: Can't delete non existing branch '%s'", branchName) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", branchName)) - return - } - - if err := deleteBranch(ctx, branchName); err != nil { - log.Error("DeleteBranch: %v", err) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", branchName)) return } @@ -168,41 +158,6 @@ func redirect(ctx *context.Context) { }) } -func deleteBranch(ctx *context.Context, branchName string) error { - commit, err := ctx.Repo.GitRepo.GetBranchCommit(branchName) - if err != nil { - log.Error("GetBranchCommit: %v", err) - return err - } - - if err := ctx.Repo.GitRepo.DeleteBranch(branchName, git.DeleteBranchOptions{ - Force: true, - }); err != nil { - log.Error("DeleteBranch: %v", err) - return err - } - - // Don't return error below this - if err := repo_service.PushUpdate( - &repo_module.PushUpdateOptions{ - RefFullName: git.BranchPrefix + branchName, - OldCommitID: commit.ID.String(), - NewCommitID: git.EmptySHA, - PusherID: ctx.User.ID, - PusherName: ctx.User.Name, - RepoUserName: ctx.Repo.Owner.Name, - RepoName: ctx.Repo.Repository.Name, - }); err != nil { - log.Error("Update: %v", err) - } - - if err := ctx.Repo.Repository.AddDeletedBranch(branchName, commit.ID.String(), ctx.User.ID); err != nil { - log.Warn("AddDeletedBranch: %v", err) - } - - return nil -} - // loadBranches loads branches from the repository limited by page & pageSize. // NOTE: May write to context on error. func loadBranches(ctx *context.Context, skip, limit int) ([]*Branch, int) { diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 2ed47605f..5d7e70fec 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -9,6 +9,7 @@ package repo import ( "container/list" "crypto/subtle" + "errors" "fmt" "net/http" "path" @@ -22,7 +23,6 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification" - repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/upload" @@ -1186,20 +1186,6 @@ func CleanUpPullRequest(ctx *context.Context) { }) }() - if pr.HeadBranch == pr.HeadRepo.DefaultBranch || !gitRepo.IsBranchExist(pr.HeadBranch) { - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - return - } - - // Check if branch is not protected - if protected, err := pr.HeadRepo.IsProtectedBranch(pr.HeadBranch, ctx.User); err != nil || protected { - if err != nil { - log.Error("HeadRepo.IsProtectedBranch: %v", err) - } - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - return - } - // Check if branch has no new commits headCommitID, err := gitBaseRepo.GetRefCommitID(pr.GetGitRefName()) if err != nil { @@ -1218,27 +1204,21 @@ func CleanUpPullRequest(ctx *context.Context) { return } - if err := gitRepo.DeleteBranch(pr.HeadBranch, git.DeleteBranchOptions{ - Force: true, - }); err != nil { - log.Error("DeleteBranch: %v", err) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) + if err := repo_service.DeleteBranch(ctx.User, pr.HeadRepo, gitRepo, pr.HeadBranch); err != nil { + switch { + case git.IsErrBranchNotExist(err): + ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) + case errors.Is(err, repo_service.ErrBranchIsDefault): + ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) + case errors.Is(err, repo_service.ErrBranchIsProtected): + ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) + default: + log.Error("DeleteBranch: %v", err) + ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) + } return } - if err := repo_service.PushUpdate( - &repo_module.PushUpdateOptions{ - RefFullName: git.BranchPrefix + pr.HeadBranch, - OldCommitID: branchCommitID, - NewCommitID: git.EmptySHA, - PusherID: ctx.User.ID, - PusherName: ctx.User.Name, - RepoUserName: pr.HeadRepo.Owner.Name, - RepoName: pr.HeadRepo.Name, - }); err != nil { - log.Error("Update: %v", err) - } - if err := models.AddDeletePRBranchComment(ctx.User, pr.BaseRepo, issue.ID, pr.HeadBranch); err != nil { // Do not fail here as branch has already been deleted log.Error("DeleteBranch: %v", err) diff --git a/services/repository/branch.go b/services/repository/branch.go new file mode 100644 index 000000000..df07030be --- /dev/null +++ b/services/repository/branch.go @@ -0,0 +1,72 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package repository + +import ( + "errors" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/log" + repo_module "code.gitea.io/gitea/modules/repository" + pull_service "code.gitea.io/gitea/services/pull" +) + +// enmuerates all branch related errors +var ( + ErrBranchIsDefault = errors.New("branch is default") + ErrBranchIsProtected = errors.New("branch is protected") +) + +// DeleteBranch delete branch +func DeleteBranch(doer *models.User, repo *models.Repository, gitRepo *git.Repository, branchName string) error { + if branchName == repo.DefaultBranch { + return ErrBranchIsDefault + } + + isProtected, err := repo.IsProtectedBranch(branchName, doer) + if err != nil { + return err + } + + if isProtected { + return ErrBranchIsProtected + } + + commit, err := gitRepo.GetBranchCommit(branchName) + if err != nil { + return err + } + + if err := gitRepo.DeleteBranch(branchName, git.DeleteBranchOptions{ + Force: true, + }); err != nil { + return err + } + + if err := pull_service.CloseBranchPulls(doer, repo.ID, branchName); err != nil { + return err + } + + // Don't return error below this + if err := PushUpdate( + &repo_module.PushUpdateOptions{ + RefFullName: git.BranchPrefix + branchName, + OldCommitID: commit.ID.String(), + NewCommitID: git.EmptySHA, + PusherID: doer.ID, + PusherName: doer.Name, + RepoUserName: repo.OwnerName, + RepoName: repo.Name, + }); err != nil { + log.Error("Update: %v", err) + } + + if err := repo.AddDeletedBranch(branchName, commit.ID.String(), doer.ID); err != nil { + log.Warn("AddDeletedBranch: %v", err) + } + + return nil +}