From f2d12f7b034e32d0e7cc7b60e7ad015af122db3f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 25 Jan 2020 10:48:22 +0800 Subject: [PATCH] Fix pull view when head repository or head branch missed and close related pull requests when delete head repository or head branch (#9927) * fix pull view when head repository or head branch missed and close related pull requests when delete branch * fix pull view broken when head repository deleted * close pull requests when head repositories deleted * Add tests for broken pull request head repository or branch * fix typo * ignore special error when close pull request Co-authored-by: Lauris BH --- integrations/pull_create_test.go | 54 ++++++++++++++++++++++ models/pull.go | 6 ++- modules/repofiles/update.go | 24 +++++++--- routers/repo/pull.go | 29 ++++++------ services/pull/pull.go | 76 +++++++++++++++++++++++++++++++ services/repository/repository.go | 5 ++ 6 files changed, 174 insertions(+), 20 deletions(-) diff --git a/integrations/pull_create_test.go b/integrations/pull_create_test.go index 84bcbff0d..d0c78a046 100644 --- a/integrations/pull_create_test.go +++ b/integrations/pull_create_test.go @@ -106,3 +106,57 @@ func TestPullCreate_TitleEscape(t *testing.T) { assert.Equal(t, "<u>XSS PR</u>", titleHTML) }) } + +func testUIDeleteBranch(t *testing.T, session *TestSession, ownerName, repoName, branchName string) { + relURL := "/" + path.Join(ownerName, repoName, "branches") + req := NewRequest(t, "GET", relURL) + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + req = NewRequestWithValues(t, "POST", relURL+"/delete", map[string]string{ + "_csrf": getCsrf(t, htmlDoc.doc), + "name": branchName, + }) + session.MakeRequest(t, req, http.StatusOK) +} + +func testDeleteRepository(t *testing.T, session *TestSession, ownerName, repoName string) { + relURL := "/" + path.Join(ownerName, repoName, "settings") + req := NewRequest(t, "GET", relURL) + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + req = NewRequestWithValues(t, "POST", relURL+"?action=delete", map[string]string{ + "_csrf": getCsrf(t, htmlDoc.doc), + "repo_name": repoName, + }) + session.MakeRequest(t, req, http.StatusFound) +} + +func TestPullBranchDelete(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + defer prepareTestEnv(t)() + + session := loginUser(t, "user1") + testRepoFork(t, session, "user2", "repo1", "user1", "repo1") + testCreateBranch(t, session, "user1", "repo1", "branch/master", "master1", http.StatusFound) + testEditFile(t, session, "user1", "repo1", "master1", "README.md", "Hello, World (Edited)\n") + resp := testPullCreate(t, session, "user1", "repo1", "master1", "This is a pull title") + + // check the redirected URL + url := resp.HeaderMap.Get("Location") + assert.Regexp(t, "^/user2/repo1/pulls/[0-9]*$", url) + req := NewRequest(t, "GET", url) + session.MakeRequest(t, req, http.StatusOK) + + // delete head branch and confirm pull page is ok + testUIDeleteBranch(t, session, "user1", "repo1", "master1") + req = NewRequest(t, "GET", url) + session.MakeRequest(t, req, http.StatusOK) + + // delete head repository and confirm pull page is ok + testDeleteRepository(t, session, "user1", "repo1") + req = NewRequest(t, "GET", url) + session.MakeRequest(t, req, http.StatusOK) + }) +} diff --git a/models/pull.go b/models/pull.go index 42d93bd54..1895030f7 100644 --- a/models/pull.go +++ b/models/pull.go @@ -67,7 +67,11 @@ type PullRequest struct { // MustHeadUserName returns the HeadRepo's username if failed return blank func (pr *PullRequest) MustHeadUserName() string { if err := pr.LoadHeadRepo(); err != nil { - log.Error("LoadHeadRepo: %v", err) + if !IsErrRepoNotExist(err) { + log.Error("LoadHeadRepo: %v", err) + } else { + log.Warn("LoadHeadRepo %d but repository does not exist: %v", pr.HeadRepoID, err) + } return "" } return pr.HeadRepo.OwnerName diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index 430a83093..5188529d1 100644 --- a/modules/repofiles/update.go +++ b/modules/repofiles/update.go @@ -495,9 +495,18 @@ func PushUpdate(repo *models.Repository, branch string, opts PushUpdateOptions) return err } - log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name) + if !isDelRef { + if err = models.RemoveDeletedBranch(repo.ID, opts.Branch); err != nil { + log.Error("models.RemoveDeletedBranch %s/%s failed: %v", repo.ID, opts.Branch, err) + } - go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, opts.OldCommitID, opts.NewCommitID) + log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name) + + go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, opts.OldCommitID, opts.NewCommitID) + // close all related pulls + } else if err = pull_service.CloseBranchPulls(pusher, repo.ID, branch); err != nil { + log.Error("close related pull request failed: %v", err) + } if err = models.WatchIfAuto(opts.PusherID, repo.ID, true); err != nil { log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err) @@ -544,12 +553,15 @@ func PushUpdates(repo *models.Repository, optsList []*PushUpdateOptions) error { if err = models.RemoveDeletedBranch(repo.ID, opts.Branch); err != nil { log.Error("models.RemoveDeletedBranch %s/%s failed: %v", repo.ID, opts.Branch, err) } + + log.Trace("TriggerTask '%s/%s' by %s", repo.Name, opts.Branch, pusher.Name) + + go pull_service.AddTestPullRequestTask(pusher, repo.ID, opts.Branch, true, opts.OldCommitID, opts.NewCommitID) + // close all related pulls + } else if err = pull_service.CloseBranchPulls(pusher, repo.ID, opts.Branch); err != nil { + log.Error("close related pull request failed: %v", err) } - log.Trace("TriggerTask '%s/%s' by %s", repo.Name, opts.Branch, pusher.Name) - - go pull_service.AddTestPullRequestTask(pusher, repo.ID, opts.Branch, true, opts.OldCommitID, opts.NewCommitID) - if err = models.WatchIfAuto(opts.PusherID, repo.ID, true); err != nil { log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err) } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index c84174783..655be2e82 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -343,19 +343,6 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare setMergeTarget(ctx, pull) - divergence, err := pull_service.GetDiverging(pull) - if err != nil { - ctx.ServerError("GetDiverging", err) - return nil - } - ctx.Data["Divergence"] = divergence - allowUpdate, err := pull_service.IsUserAllowedToUpdate(pull, ctx.User) - if err != nil { - ctx.ServerError("IsUserAllowedToUpdate", err) - return nil - } - ctx.Data["UpdateAllowed"] = allowUpdate - if err := pull.LoadProtectedBranch(); err != nil { ctx.ServerError("LoadProtectedBranch", err) return nil @@ -392,6 +379,22 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare } } + if headBranchExist { + allowUpdate, err := pull_service.IsUserAllowedToUpdate(pull, ctx.User) + if err != nil { + ctx.ServerError("IsUserAllowedToUpdate", err) + return nil + } + ctx.Data["UpdateAllowed"] = allowUpdate + + divergence, err := pull_service.GetDiverging(pull) + if err != nil { + ctx.ServerError("GetDiverging", err) + return nil + } + ctx.Data["Divergence"] = divergence + } + sha, err := baseGitRepo.GetRefCommitID(pull.GetGitRefName()) if err != nil { ctx.ServerError(fmt.Sprintf("GetRefCommitID(%s)", pull.GetGitRefName()), err) diff --git a/services/pull/pull.go b/services/pull/pull.go index bc71e5221..705eab06a 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -355,3 +355,79 @@ func PushToBaseRepo(pr *models.PullRequest) (err error) { return nil } + +type errlist []error + +func (errs errlist) Error() string { + if len(errs) > 0 { + var buf strings.Builder + for i, err := range errs { + if i > 0 { + buf.WriteString(", ") + } + buf.WriteString(err.Error()) + } + return buf.String() + } + return "" +} + +// CloseBranchPulls close all the pull requests who's head branch is the branch +func CloseBranchPulls(doer *models.User, repoID int64, branch string) error { + prs, err := models.GetUnmergedPullRequestsByHeadInfo(repoID, branch) + if err != nil { + return err + } + + prs2, err := models.GetUnmergedPullRequestsByBaseInfo(repoID, branch) + if err != nil { + return err + } + + prs = append(prs, prs2...) + if err := models.PullRequestList(prs).LoadAttributes(); err != nil { + return err + } + + var errs errlist + for _, pr := range prs { + if err = issue_service.ChangeStatus(pr.Issue, doer, true); err != nil && !models.IsErrIssueWasClosed(err) { + errs = append(errs, err) + } + } + if len(errs) > 0 { + return errs + } + return nil +} + +// CloseRepoBranchesPulls close all pull requests which head branches are in the given repository +func CloseRepoBranchesPulls(doer *models.User, repo *models.Repository) error { + branches, err := git.GetBranchesByPath(repo.RepoPath()) + if err != nil { + return err + } + + var errs errlist + for _, branch := range branches { + prs, err := models.GetUnmergedPullRequestsByHeadInfo(repo.ID, branch.Name) + if err != nil { + return err + } + + if err = models.PullRequestList(prs).LoadAttributes(); err != nil { + return err + } + + for _, pr := range prs { + if err = issue_service.ChangeStatus(pr.Issue, doer, true); err != nil && !models.IsErrIssueWasClosed(err) { + errs = append(errs, err) + } + } + } + + if len(errs) > 0 { + return errs + } + return nil +} diff --git a/services/repository/repository.go b/services/repository/repository.go index eea8b352b..f50b98b64 100644 --- a/services/repository/repository.go +++ b/services/repository/repository.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification" repo_module "code.gitea.io/gitea/modules/repository" + pull_service "code.gitea.io/gitea/services/pull" ) // CreateRepository creates a repository for the user/organization. @@ -49,6 +50,10 @@ func ForkRepository(doer, u *models.User, oldRepo *models.Repository, name, desc // DeleteRepository deletes a repository for a user or organization. func DeleteRepository(doer *models.User, repo *models.Repository) error { + if err := pull_service.CloseRepoBranchesPulls(doer, repo); err != nil { + log.Error("CloseRepoBranchesPulls failed: %v", err) + } + if err := models.DeleteRepository(doer, repo.OwnerID, repo.ID); err != nil { return err }