From 0a5dc640a12d1c0475052b73a721056b53460275 Mon Sep 17 00:00:00 2001 From: Lauris BH Date: Wed, 21 Jun 2017 04:00:03 +0300 Subject: [PATCH] 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 --- integrations/editor_test.go | 43 +++++++++++ integrations/pull_create_test.go | 4 + integrations/pull_merge_test.go | 56 ++++++++++++++ routers/repo/branch.go | 59 -------------- routers/repo/issue.go | 18 +++-- routers/repo/pull.go | 127 +++++++++++++++++++++++++++++++ routers/routes/routes.go | 4 +- 7 files changed, 241 insertions(+), 70 deletions(-) diff --git a/integrations/editor_test.go b/integrations/editor_test.go index 24cb8f196..32eb07fa9 100644 --- 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") +} diff --git a/integrations/pull_create_test.go b/integrations/pull_create_test.go index 91f682fbd..9e9ba9eb3 100644 --- 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) diff --git a/integrations/pull_merge_test.go b/integrations/pull_merge_test.go index acb48bd78..da3103bd3 100644 --- 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) +} diff --git a/routers/repo/branch.go b/routers/repo/branch.go index d040f2a56..fcb6efd01 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 b5b620dbb..f3799fdb9 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 b710e9420..10e7449aa 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 c2f29613b..90944c25f 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() {