Make branch deletion URL more like GitHub's, fixes #1397 (#1994)

* Make branch deletion URL more like GitHub's, fixes #1397

* Add PR branch deletion integration test

* Do not allow deleting protected branch

* Change http error code to 403 if user has no write rights to repository

* Add check to not panic if forked repository has alrady been deleted

4 files changed, 138 insertions, 70 deletions

diff --git a/routers/repo/branch.go b/routers/repo/branch.go
index d040f2a560..fcb6efd010 100644
--- a/routers/repo/branch.go
+++ b/routers/repo/branch.go
@@ -5,11 +5,8 @@
 package repo
 
 import (
-	"code.gitea.io/git"
-	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/context"
-	"code.gitea.io/gitea/modules/log"
 )
 
 const (
@@ -33,59 +30,3 @@ func Branches(ctx *context.Context) {
 	ctx.Data["Branches"] = brs
 	ctx.HTML(200, tplBranch)
 }
-
-// DeleteBranchPost responses for delete merged branch
-func DeleteBranchPost(ctx *context.Context) {
-	branchName := ctx.Params(":name")
-	commitID := ctx.Query("commit")
-
-	defer func() {
-		redirectTo := ctx.Query("redirect_to")
-		if len(redirectTo) == 0 {
-			redirectTo = ctx.Repo.RepoLink
-		}
-
-		ctx.JSON(200, map[string]interface{}{
-			"redirect": redirectTo,
-		})
-	}()
-
-	fullBranchName := ctx.Repo.Owner.Name + "/" + branchName
-
-	if !ctx.Repo.GitRepo.IsBranchExist(branchName) || branchName == "master" {
-		ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName))
-		return
-	}
-
-	if len(commitID) > 0 {
-		branchCommitID, err := ctx.Repo.GitRepo.GetBranchCommitID(branchName)
-		if err != nil {
-			log.Error(4, "GetBranchCommitID: %v", err)
-			return
-		}
-
-		if branchCommitID != commitID {
-			ctx.Flash.Error(ctx.Tr("repo.branch.delete_branch_has_new_commits", fullBranchName))
-			return
-		}
-	}
-
-	if err := ctx.Repo.GitRepo.DeleteBranch(branchName, git.DeleteBranchOptions{
-		Force: true,
-	}); err != nil {
-		log.Error(4, "DeleteBranch: %v", err)
-		ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName))
-		return
-	}
-
-	issueID := ctx.QueryInt64("issue_id")
-	if issueID > 0 {
-		if err := models.AddDeletePRBranchComment(ctx.User, ctx.Repo.Repository, issueID, branchName); err != nil {
-			log.Error(4, "DeleteBranch: %v", err)
-			ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName))
-			return
-		}
-	}
-
-	ctx.Flash.Success(ctx.Tr("repo.branch.deletion_success", fullBranchName))
-}
diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index b5b620dbb3..f3799fdb99 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -647,19 +647,21 @@ func ViewIssue(ctx *context.Context) {
 		pull := issue.PullRequest
 		canDelete := false
 
-		if ctx.IsSigned && pull.HeadBranch != "master" {
+		if ctx.IsSigned {
 			if err := pull.GetHeadRepo(); err != nil {
 				log.Error(4, "GetHeadRepo: %v", err)
-			} else if ctx.User.IsWriterOfRepo(pull.HeadRepo) {
-				canDelete = true
-				deleteBranchURL := pull.HeadRepo.Link() + "/branches/" + pull.HeadBranch + "/delete"
-				ctx.Data["DeleteBranchLink"] = fmt.Sprintf("%s?commit=%s&redirect_to=%s&issue_id=%d",
-					deleteBranchURL, pull.MergedCommitID, ctx.Data["Link"], issue.ID)
-
+			} else if pull.HeadRepo != nil && pull.HeadBranch != pull.HeadRepo.DefaultBranch && ctx.User.IsWriterOfRepo(pull.HeadRepo) {
+				// Check if branch is not protected
+				if protected, err := pull.HeadRepo.IsProtectedBranch(pull.HeadBranch); err != nil {
+					log.Error(4, "IsProtectedBranch: %v", err)
+				} else if !protected {
+					canDelete = true
+					ctx.Data["DeleteBranchLink"] = ctx.Repo.RepoLink + "/pulls/" + com.ToStr(issue.Index) + "/cleanup"
+				}
 			}
 		}
 
-		ctx.Data["IsPullBranchDeletable"] = canDelete && git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch)
+		ctx.Data["IsPullBranchDeletable"] = canDelete && pull.HeadRepo != nil && git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch)
 	}
 
 	ctx.Data["Participants"] = participants
diff --git a/routers/repo/pull.go b/routers/repo/pull.go
index b710e94207..10e7449aa4 100644
--- a/routers/repo/pull.go
+++ b/routers/repo/pull.go
@@ -757,3 +757,130 @@ func TriggerTask(ctx *context.Context) {
 	go models.AddTestPullRequestTask(pusher, repo.ID, branch, true)
 	ctx.Status(202)
 }
+
+// CleanUpPullRequest responses for delete merged branch when PR has been merged
+func CleanUpPullRequest(ctx *context.Context) {
+	issue := checkPullInfo(ctx)
+	if ctx.Written() {
+		return
+	}
+
+	pr, err := models.GetPullRequestByIssueID(issue.ID)
+	if err != nil {
+		if models.IsErrPullRequestNotExist(err) {
+			ctx.Handle(404, "GetPullRequestByIssueID", nil)
+		} else {
+			ctx.Handle(500, "GetPullRequestByIssueID", err)
+		}
+		return
+	}
+
+	// Allow cleanup only for merged PR
+	if !pr.HasMerged {
+		ctx.Handle(404, "CleanUpPullRequest", nil)
+		return
+	}
+
+	if err = pr.GetHeadRepo(); err != nil {
+		ctx.Handle(500, "GetHeadRepo", err)
+		return
+	} else if pr.HeadRepo == nil {
+		// Forked repository has already been deleted
+		ctx.Handle(404, "CleanUpPullRequest", nil)
+		return
+	} else if pr.GetBaseRepo(); err != nil {
+		ctx.Handle(500, "GetBaseRepo", err)
+		return
+	} else if pr.HeadRepo.GetOwner(); err != nil {
+		ctx.Handle(500, "HeadRepo.GetOwner", err)
+		return
+	}
+
+	if !ctx.User.IsWriterOfRepo(pr.HeadRepo) {
+		ctx.Handle(403, "CleanUpPullRequest", nil)
+		return
+	}
+
+	fullBranchName := pr.HeadRepo.Owner.Name + "/" + pr.HeadBranch
+
+	gitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath())
+	if err != nil {
+		ctx.Handle(500, fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.RepoPath()), err)
+		return
+	}
+
+	gitBaseRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
+	if err != nil {
+		ctx.Handle(500, fmt.Sprintf("OpenRepository[%s]", pr.BaseRepo.RepoPath()), err)
+		return
+	}
+
+	defer func() {
+		ctx.JSON(200, map[string]interface{}{
+			"redirect": pr.BaseRepo.Link() + "/pulls/" + com.ToStr(issue.Index),
+		})
+	}()
+
+	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); err != nil || protected {
+		if err != nil {
+			log.Error(4, "HeadRepo.IsProtectedBranch: %v", err)
+		}
+		ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName))
+		return
+	}
+
+	// Check if branch has no new commits
+	if len(pr.MergedCommitID) > 0 {
+		branchCommitID, err := gitRepo.GetBranchCommitID(pr.HeadBranch)
+		if err != nil {
+			log.Error(4, "GetBranchCommitID: %v", err)
+			ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName))
+			return
+		}
+
+		commit, err := gitBaseRepo.GetCommit(pr.MergedCommitID)
+		if err != nil {
+			log.Error(4, "GetCommit: %v", err)
+			ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName))
+			return
+		}
+
+		isParent := false
+		for i := 0; i < commit.ParentCount(); i++ {
+			if parent, err := commit.Parent(i); err != nil {
+				log.Error(4, "Parent: %v", err)
+				ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName))
+				return
+			} else if parent.ID.String() == branchCommitID {
+				isParent = true
+				break
+			}
+		}
+
+		if !isParent {
+			ctx.Flash.Error(ctx.Tr("repo.branch.delete_branch_has_new_commits", fullBranchName))
+			return
+		}
+	}
+
+	if err := gitRepo.DeleteBranch(pr.HeadBranch, git.DeleteBranchOptions{
+		Force: true,
+	}); err != nil {
+		log.Error(4, "DeleteBranch: %v", err)
+		ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName))
+		return
+	}
+
+	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(4, "DeleteBranch: %v", err)
+	}
+
+	ctx.Flash.Success(ctx.Tr("repo.branch.deletion_success", fullBranchName))
+}
diff --git a/routers/routes/routes.go b/routers/routes/routes.go
index c2f29613b4..90944c25f7 100644
--- a/routers/routes/routes.go
+++ b/routers/routes/routes.go
@@ -562,9 +562,6 @@ func RegisterRoutes(m *macaron.Macaron) {
 			m.Get("/milestones", repo.Milestones)
 		}, context.RepoRef())
 
-		// m.Get("/branches", repo.Branches)
-		m.Post("/branches/:name/delete", reqSignIn, reqRepoWriter, repo.MustBeNotBare, repo.DeleteBranchPost)
-
 		m.Group("/wiki", func() {
 			m.Get("/?:page", repo.Wiki)
 			m.Get("/_pages", repo.WikiPages)
@@ -589,6 +586,7 @@ func RegisterRoutes(m *macaron.Macaron) {
 			m.Get("/commits", context.RepoRef(), repo.ViewPullCommits)
 			m.Get("/files", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.ViewPullFiles)
 			m.Post("/merge", reqRepoWriter, repo.MergePullRequest)
+			m.Post("/cleanup", context.RepoRef(), repo.CleanUpPullRequest)
 		}, repo.MustAllowPulls, context.CheckUnit(models.UnitTypePullRequests))
 
 		m.Group("", func() {