Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pull view when head repository or head branch missed and close related pull requests when delete head repository or head branch #9927

Merged
merged 9 commits into from
Jan 25, 2020
54 changes: 54 additions & 0 deletions integrations/pull_create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
6 changes: 5 additions & 1 deletion models/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 17 additions & 5 deletions modules/repofiles/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
lafriks marked this conversation as resolved.
Show resolved Hide resolved
lunny marked this conversation as resolved.
Show resolved Hide resolved
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)
// 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)
Expand Down Expand Up @@ -544,11 +553,14 @@ func PushUpdates(repo *models.Repository, optsList []*PushUpdateOptions) error {
if err = models.RemoveDeletedBranch(repo.ID, opts.Branch); err != nil {
lunny marked this conversation as resolved.
Show resolved Hide resolved
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)
// 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)
}

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)
Expand Down
29 changes: 16 additions & 13 deletions routers/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
76 changes: 76 additions & 0 deletions services/pull/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
lunny marked this conversation as resolved.
Show resolved Hide resolved
}
}
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)
lunny marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

if len(errs) > 0 {
return errs
}
return nil
}
5 changes: 5 additions & 0 deletions services/repository/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
}
Expand Down