From 2f548d34b9a6a5891917cca68babaf3a12468d9e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 22 Jan 2020 11:40:54 +0800 Subject: [PATCH 1/6] fix pull view when head repository or head branch missed and close related pull requests when delete branch --- modules/repofiles/update.go | 26 +++++++++++++++++----- routers/repo/pull.go | 15 +++++++------ services/pull/pull.go | 43 +++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 11 deletions(-) diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index 430a83093d49d..4c366469cc434 100644 --- a/modules/repofiles/update.go +++ b/modules/repofiles/update.go @@ -495,9 +495,20 @@ func PushUpdate(repo *models.Repository, branch string, opts PushUpdateOptions) return err } - log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name) + if opts.NewCommitID != git.EmptySHA { + 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, branch, pusher.Name) - go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, opts.OldCommitID, opts.NewCommitID) + go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, opts.OldCommitID, opts.NewCommitID) + } else { + // close all related pulls + 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,11 +555,16 @@ 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) + 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) + go pull_service.AddTestPullRequestTask(pusher, repo.ID, opts.Branch, true, opts.OldCommitID, opts.NewCommitID) + } else { + // close all related pulls + if err = pull_service.CloseBranchPulls(pusher, repo.ID, opts.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) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index c84174783a111..73c3d51e6d08e 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -343,12 +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) @@ -392,6 +386,15 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare } } + if headBranchExist { + 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 bc71e52213589..4586352fe93fd 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -355,3 +355,46 @@ 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 { + errs = append(errs, err) + } + } + + return errs +} From e804e140f3c97b7538589f5fd2e410e875f74e8a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 22 Jan 2020 12:13:26 +0800 Subject: [PATCH 2/6] fix pull view broken when head repository deleted --- modules/repofiles/update.go | 12 ++++-------- routers/repo/pull.go | 14 +++++++------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index 4c366469cc434..caf667fd07613 100644 --- a/modules/repofiles/update.go +++ b/modules/repofiles/update.go @@ -503,11 +503,9 @@ func PushUpdate(repo *models.Repository, branch string, opts PushUpdateOptions) 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) - } else { // close all related pulls - if err = pull_service.CloseBranchPulls(pusher, repo.ID, branch); err != nil { - log.Error("close related pull request failed: %v", err) - } + } 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 { @@ -559,11 +557,9 @@ func PushUpdates(repo *models.Repository, optsList []*PushUpdateOptions) error { 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) - } else { // close all related pulls - if err = pull_service.CloseBranchPulls(pusher, repo.ID, opts.Branch); err != nil { - log.Error("close related pull request failed: %v", err) - } + } else if err = pull_service.CloseBranchPulls(pusher, repo.ID, opts.Branch); err != nil { + log.Error("close related pull request failed: %v", err) } if err = models.WatchIfAuto(opts.PusherID, repo.ID, true); err != nil { diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 73c3d51e6d08e..655be2e82e993 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -343,13 +343,6 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare setMergeTarget(ctx, pull) - 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 @@ -387,6 +380,13 @@ 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) From 8d4f90ba3879b1467573caec2682c6ed300819c7 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 22 Jan 2020 13:05:25 +0800 Subject: [PATCH 3/6] close pull requests when head repositories deleted --- services/pull/pull.go | 28 ++++++++++++++++++++++++++++ services/repository/repository.go | 5 +++++ 2 files changed, 33 insertions(+) diff --git a/services/pull/pull.go b/services/pull/pull.go index 4586352fe93fd..ff90550f8c69d 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -398,3 +398,31 @@ func CloseBranchPulls(doer *models.User, repoID int64, branch string) error { return errs } + +// 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 { + errs = append(errs, err) + } + } + } + + return errs +} diff --git a/services/repository/repository.go b/services/repository/repository.go index eea8b352b4b51..f50b98b64099a 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 } From 98a5129c04c0aa80f7412a205d2212f3d46383b6 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 22 Jan 2020 14:29:05 +0800 Subject: [PATCH 4/6] Add tests for broken pull request head repository or branch --- integrations/pull_create_test.go | 54 ++++++++++++++++++++++++++++++++ models/pull.go | 6 +++- services/pull/pull.go | 11 +++++-- 3 files changed, 67 insertions(+), 4 deletions(-) diff --git a/integrations/pull_create_test.go b/integrations/pull_create_test.go index 84bcbff0dc6b8..d0c78a046c2e9 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 3ef631852ea98..4d466bd087763 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 is not exist: %v", pr.HeadRepoID, err) + } return "" } return pr.HeadRepo.OwnerName diff --git a/services/pull/pull.go b/services/pull/pull.go index ff90550f8c69d..53e3e1b467cef 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -395,8 +395,10 @@ func CloseBranchPulls(doer *models.User, repoID int64, branch string) error { errs = append(errs, err) } } - - return errs + if len(errs) > 0 { + return errs + } + return nil } // CloseRepoBranchesPulls close all pull requests which head branches are in the given repository @@ -424,5 +426,8 @@ func CloseRepoBranchesPulls(doer *models.User, repo *models.Repository) error { } } - return errs + if len(errs) > 0 { + return errs + } + return nil } From be8b1c64376e90fe9d875c136a9d63d03d7335df Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 22 Jan 2020 23:43:28 +0800 Subject: [PATCH 5/6] fix typo --- models/pull.go | 2 +- modules/repofiles/update.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/models/pull.go b/models/pull.go index 4d466bd087763..e9d0ab179ae85 100644 --- a/models/pull.go +++ b/models/pull.go @@ -70,7 +70,7 @@ func (pr *PullRequest) MustHeadUserName() string { if !IsErrRepoNotExist(err) { log.Error("LoadHeadRepo: %v", err) } else { - log.Warn("LoadHeadRepo %d but repository is not exist: %v", pr.HeadRepoID, err) + log.Warn("LoadHeadRepo %d but repository does not exist: %v", pr.HeadRepoID, err) } return "" } diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index caf667fd07613..5188529d1687a 100644 --- a/modules/repofiles/update.go +++ b/modules/repofiles/update.go @@ -495,7 +495,7 @@ func PushUpdate(repo *models.Repository, branch string, opts PushUpdateOptions) return err } - if opts.NewCommitID != git.EmptySHA { + 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) } From 74754bd3d4c1f13b8af8e51f4177278eb4045e4d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 23 Jan 2020 11:19:47 +0800 Subject: [PATCH 6/6] ignore special error when close pull request --- services/pull/pull.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 53e3e1b467cef..705eab06a2bed 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -363,7 +363,7 @@ func (errs errlist) Error() string { var buf strings.Builder for i, err := range errs { if i > 0 { - buf.WriteString(",") + buf.WriteString(", ") } buf.WriteString(err.Error()) } @@ -391,7 +391,7 @@ func CloseBranchPulls(doer *models.User, repoID int64, branch string) error { var errs errlist for _, pr := range prs { - if err = issue_service.ChangeStatus(pr.Issue, doer, true); err != nil { + if err = issue_service.ChangeStatus(pr.Issue, doer, true); err != nil && !models.IsErrIssueWasClosed(err) { errs = append(errs, err) } } @@ -420,7 +420,7 @@ func CloseRepoBranchesPulls(doer *models.User, repo *models.Repository) error { } for _, pr := range prs { - if err = issue_service.ChangeStatus(pr.Issue, doer, true); err != nil { + if err = issue_service.ChangeStatus(pr.Issue, doer, true); err != nil && !models.IsErrIssueWasClosed(err) { errs = append(errs, err) } }