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
2017-06-21 04:00:03 +03:00 · 2017-06-21 04:00:03 +03:00 · 0a5dc640a1
commit 0a5dc640a1
parent 6db387a21e
7 changed files with 241 additions and 70 deletions
--- a/integrations/editor_test.go
+++ b/integrations/editor_test.go
@ -126,8 +126,51 @@ func testEditFile(t *testing.T, session *TestSession, user, repo, branch, filePa
 	return resp
 }

+func testEditFileToNewBranch(t *testing.T, session *TestSession, user, repo, branch, targetBranch, filePath string) *TestResponse {
+
+	newContent := "Hello, World (Edited)\n"
+
+	// Get to the 'edit this file' page
+	req := NewRequest(t, "GET", path.Join(user, repo, "_edit", branch, filePath))
+	resp := session.MakeRequest(t, req)
+	assert.EqualValues(t, http.StatusOK, resp.HeaderCode)
+
+	htmlDoc := NewHTMLParser(t, resp.Body)
+	lastCommit := htmlDoc.GetInputValueByName("last_commit")
+	assert.NotEmpty(t, lastCommit)
+
+	// Submit the edits
+	req = NewRequestWithValues(t, "POST", path.Join(user, repo, "_edit", branch, filePath),
+		map[string]string{
+			"_csrf":           htmlDoc.GetCSRF(),
+			"last_commit":     lastCommit,
+			"tree_path":       filePath,
+			"content":         newContent,
+			"commit_choice":   "commit-to-new-branch",
+			"new_branch_name": targetBranch,
+		},
+	)
+	req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
+	resp = session.MakeRequest(t, req)
+	assert.EqualValues(t, http.StatusFound, resp.HeaderCode)
+
+	// Verify the change
+	req = NewRequest(t, "GET", path.Join(user, repo, "raw", targetBranch, filePath))
+	resp = session.MakeRequest(t, req)
+	assert.EqualValues(t, http.StatusOK, resp.HeaderCode)
+	assert.EqualValues(t, newContent, string(resp.Body))
+
+	return resp
+}
+
 func TestEditFile(t *testing.T) {
 	prepareTestEnv(t)
 	session := loginUser(t, "user2")
 	testEditFile(t, session, "user2", "repo1", "master", "README.md")
 }
+
+func TestEditFileToNewBranch(t *testing.T) {
+	prepareTestEnv(t)
+	session := loginUser(t, "user2")
+	testEditFileToNewBranch(t, session, "user2", "repo1", "master", "feature/test", "README.md")
+}
--- a/integrations/pull_create_test.go
+++ b/integrations/pull_create_test.go
@ -7,6 +7,7 @@ package integrations
 import (
 	"net/http"
 	"path"
+	"strings"
 	"testing"

 	"github.com/stretchr/testify/assert"
@ -21,6 +22,9 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo, branch strin
 	htmlDoc := NewHTMLParser(t, resp.Body)
 	link, exists := htmlDoc.doc.Find("button.ui.green.small.button").Parent().Attr("href")
 	assert.True(t, exists, "The template has changed")
+	if branch != "master" {
+		link = strings.Replace(link, ":master", ":"+branch, 1)
+	}

 	req = NewRequest(t, "GET", link)
 	resp = session.MakeRequest(t, req)
--- a/integrations/pull_merge_test.go
+++ b/integrations/pull_merge_test.go
@ -32,6 +32,25 @@ func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum strin
 	return resp
 }

+func testPullCleanUp(t *testing.T, session *TestSession, user, repo, pullnum string) *TestResponse {
+	req := NewRequest(t, "GET", path.Join(user, repo, "pulls", pullnum))
+	resp := session.MakeRequest(t, req)
+	assert.EqualValues(t, http.StatusOK, resp.HeaderCode)
+
+	// Click the little green button to craete a pull
+	htmlDoc := NewHTMLParser(t, resp.Body)
+	link, exists := htmlDoc.doc.Find(".comments .merge .delete-button").Attr("data-url")
+	assert.True(t, exists, "The template has changed")
+	req = NewRequestWithValues(t, "POST", link, map[string]string{
+		"_csrf": htmlDoc.GetCSRF(),
+	})
+	req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
+	resp = session.MakeRequest(t, req)
+	assert.EqualValues(t, http.StatusOK, resp.HeaderCode)
+
+	return resp
+}
+
 func TestPullMerge(t *testing.T) {
 	prepareTestEnv(t)
 	session := loginUser(t, "user1")
@ -46,3 +65,40 @@ func TestPullMerge(t *testing.T) {
 	assert.EqualValues(t, "pulls", elem[3])
 	testPullMerge(t, session, elem[1], elem[2], elem[4])
 }
+
+func TestPullCleanUpAfterMerge(t *testing.T) {
+	prepareTestEnv(t)
+	session := loginUser(t, "user1")
+	testRepoFork(t, session)
+	testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md")
+
+	resp := testPullCreate(t, session, "user1", "repo1", "feature/test")
+	redirectedURL := resp.Headers["Location"]
+	assert.NotEmpty(t, redirectedURL, "Redirected URL is not found")
+
+	elem := strings.Split(redirectedURL[0], "/")
+	assert.EqualValues(t, "pulls", elem[3])
+	testPullMerge(t, session, elem[1], elem[2], elem[4])
+
+	// Check PR branch deletion
+	resp = testPullCleanUp(t, session, elem[1], elem[2], elem[4])
+	respJSON := struct {
+		Redirect string
+	}{}
+	DecodeJSON(t, resp, &respJSON)
+
+	assert.NotEmpty(t, respJSON.Redirect, "Redirected URL is not found")
+
+	elem = strings.Split(respJSON.Redirect, "/")
+	assert.EqualValues(t, "pulls", elem[3])
+
+	// Check branch deletion result
+	req := NewRequest(t, "GET", respJSON.Redirect)
+	resp = session.MakeRequest(t, req)
+	assert.EqualValues(t, http.StatusOK, resp.HeaderCode)
+
+	htmlDoc := NewHTMLParser(t, resp.Body)
+	resultMsg := htmlDoc.doc.Find(".ui.message>p").Text()
+
+	assert.EqualValues(t, "user1/feature/test has been deleted.", resultMsg)
+}
--- 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))
-}
--- 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
--- 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))
+}
--- 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() {