Merge all deleteBranch as one function and also fix bug when delete branch don't close related PRs (#16067)

* Fix bug when delete branch don't close related PRs

* Merge all deletebranch as one method

* Add missed branch.go

* fix comment

Co-authored-by: Lauris BH <lauris@nix.lv>

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,60 +117,18 @@ func DeleteBranch(ctx *context.APIContext) {
 
 	branchName := ctx.Params("*")
 
-	if ctx.Repo.Repository.DefaultBranch == branchName {
+	if err := repo_service.DeleteBranch(ctx.User, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName); err != nil {
+		switch {
+		case git.IsErrBranchNotExist(err):
+			ctx.NotFound(err)
+		case errors.Is(err, repo_service.ErrBranchIsDefault):
 			ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch"))
-		return
+		case errors.Is(err, repo_service.ErrBranchIsProtected):
-	}
-
-	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
+		default:
-	}
-
-	branch, err := repo_module.GetBranch(ctx.Repo.Repository, branchName)
-	if err != nil {
-		if git.IsErrBranchNotExist(err) {
-			ctx.NotFound(err)
-		} else {
-			ctx.Error(http.StatusInternalServerError, "GetBranch", 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
 		}
-
+		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)

routers/repo/branch.go

@@ -6,6 +6,7 @@
 package repo
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
 	"strings"
@@ -83,34 +84,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 := repo_service.DeleteBranch(ctx.User, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName); err != nil {
-	if err != nil {
+		switch {
-		log.Error("DeleteBranch: %v", err)
+		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))
-		return
+		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))
-	if isProtected {
+		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))
-		return
+		default:
-	}
+			log.Error("DeleteBranch: %v", err)
-
-	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
 	}
 
@@ -169,41 +159,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) {

routers/repo/pull.go

@@ -9,6 +9,7 @@ package repo
 import (
 	"container/list"
 	"crypto/subtle"
+	"errors"
 	"fmt"
 	"net/http"
 	"path"
@@ -21,7 +22,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,25 +1204,19 @@ func CleanUpPullRequest(ctx *context.Context) {
 		return
 	}
 
-	if err := gitRepo.DeleteBranch(pr.HeadBranch, git.DeleteBranchOptions{
+	if err := repo_service.DeleteBranch(ctx.User, pr.HeadRepo, gitRepo, pr.HeadBranch); err != nil {
-		Force: true,
+		switch {
-	}); err != nil {
+		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
 		}
-
+		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 {

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
+}