Skip to content

branch: Close all the associated PRs when a branch is deleted. #5646

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

branch: Close all the associated PRs when a branch is deleted. #5646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d2957a
branch: Close all the associated PRs when a branch is deleted.
HarshitOnGitHub Jan 2, 2019
dbd2cd4
Merge branch 'master' into Issue_2466
techknowlogick May 7, 2019
70c1e28
update to use new logging
techknowlogick May 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions models/action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,8 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
case ActionDeleteBranch: // Delete Branch
isHookEventPush = true

CloseActivePullRequests(pusher, repo, refName)

if err = PrepareWebhooks(repo, HookEventDelete, &api.DeletePayload{
Ref: refName,
RefType: "branch",
Expand Down
2 changes: 1 addition & 1 deletion models/fixtures/pull_request.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
head_repo_id: 1
base_repo_id: 1
head_user_name: user1
head_branch: branch2
head_branch: develop
base_branch: master
merge_base: fedcba9876543210
has_merged: false
14 changes: 14 additions & 0 deletions models/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1803,6 +1803,15 @@ func (issue *Issue) getBlockingDependencies(e Engine) (issueDeps []*Issue, err e
Find(&issueDeps)
}

// Returns true if the issue has any open issue as its dependency.
func (issue *Issue) isBlocked(e Engine) (bool, error) {
count, err := e.Table("issue_dependency").
Join("INNER", "issue", "issue.id = issue_dependency.issue_id").
Where("dependency_id = ? AND issue.is_closed = ?", issue.ID, false).
Count()
return count > 0, err
}

// BlockedByDependencies finds all Dependencies an issue is blocked by
func (issue *Issue) BlockedByDependencies() ([]*Issue, error) {
return issue.getBlockedByDependencies(x)
Expand All @@ -1812,3 +1821,8 @@ func (issue *Issue) BlockedByDependencies() ([]*Issue, error) {
func (issue *Issue) BlockingDependencies() ([]*Issue, error) {
return issue.getBlockingDependencies(x)
}

// IsBlocked returns true if the issue has any open issue as its dependency.
func (issue *Issue) IsBlocked() (bool, error) {
return issue.isBlocked(x)
}
43 changes: 43 additions & 0 deletions models/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1527,3 +1527,46 @@ func TestPullRequests() {
func InitTestPullRequests() {
go TestPullRequests()
}

// CloseActivePullRequests closes the issue of all the active pull requests associated with a branch
func CloseActivePullRequests(doer *User, repo *Repository, branchName string) {
baseprs, err := GetUnmergedPullRequestsByBaseInfo(repo.ID, branchName)
if err != nil {
log.Error("Find pull requests [base_repo_id: %d, base_branch: %s]: %v", repo.ID, branchName, err)
return
}
headprs, err := GetUnmergedPullRequestsByHeadInfo(repo.ID, branchName)
if err != nil {
log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", repo.ID, branchName, err)
return
}
prs := append(baseprs, headprs...)
for statusChanged := true; statusChanged; {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not what I meant by sort the issues.

You have a list of pull-requests, prs. Consider two pull-requests p and q, say p<q if q has a dependency on p.

Assuming there are no cyclic dependencies, this < forms a partial order on the set of PRs in prs. You can then sort the list prs by this partial order. (There will be multiple valid sortations of the list but who cares.)

You should not have to ask the database potentially n^2 times whether an issue still has dependencies.

Further, this whole section should be done in a transaction - you're at risk of data changing under your feet here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I had understood what you meant in your comment but after looking at our code for issue dependency I found that it is indeed possible to create a cycle involving three or more issues like A->B->C->A, in this case sorting can't be done without first detecting and removing the issues involved in the cycle, right? If you are concerned about the database query I can remove that by the replacing the code with some less straightforward code, but shouldn't that database query be cheap since we are performing only a count operation and not actually loading the data?
I think we should fix the issue dependency code anyway to avoid having circular dependency, right?

Further, this whole section should be done in a transaction - you're at risk of data changing under your feet here.

Yeah sorry missed that, will change it. :)
Thanks for taking time to review! :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eurgh. I hadn't realised we weren't enforcing non-cyclicity - which I guess is hard without always forcing the transitive closure on dependency additional - and is itself prone to exponential blow up.

Ok. Let's think about this slightly differently - does it ever make sense to have an open pull request from a deleted branch? Probably not, even if that request has dependencies that it should prevent its closure - the fact that the code isn't there means it must be closed despite this.

So, if it doesn't break the database and the app layer to force it closed we should force it closed in spite of its dependencies. (I'm not sure for definite if this is the case - but I'd be surprised if it was the case.)

I guess therefore either issue.ChangeStatus should have additional optional argument to force or, there needs to be a ForceClose function. I could imagine a possible use for a ForceClose for cases where there has been a rogue user - although it might make sense in such situations to just delete everything they've done.

(I appreciate that I'm nitpicking over what will be usually one PR but people have a tendency to deliberately break stuff.)

statusChanged = false
for _, pr := range prs {
if err = pr.LoadIssue(); err != nil {
log.Error("LoadIssue: %v", err)
}
if pr.Issue.IsClosed {
continue
}
if blocked, err := pr.Issue.IsBlocked(); err != nil {
log.Error("IsBlocked: %v", err)
} else if !blocked {
err = pr.Issue.ChangeStatus(doer, true)
if err != nil {
log.Error("ChangeStatus: %v", err)
} else {
log.Trace("Issue [%d] status changed to closed", pr.IssueID)
statusChanged = true
}
}
}
}

for _, pr := range prs {
if !pr.Issue.IsClosed {
log.Warn("Issue [%d] status change blocked by existing dependencies.", pr.Issue.ID)
}
}
}
80 changes: 77 additions & 3 deletions models/pull_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func TestPullRequestsOldest(t *testing.T) {

func TestGetUnmergedPullRequest(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())
pr, err := GetUnmergedPullRequest(1, 1, "branch2", "master")
pr, err := GetUnmergedPullRequest(1, 1, "develop", "master")
assert.NoError(t, err)
assert.Equal(t, int64(2), pr.ID)

Expand All @@ -101,12 +101,12 @@ func TestGetUnmergedPullRequest(t *testing.T) {

func TestGetUnmergedPullRequestsByHeadInfo(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())
prs, err := GetUnmergedPullRequestsByHeadInfo(1, "branch2")
prs, err := GetUnmergedPullRequestsByHeadInfo(1, "develop")
assert.NoError(t, err)
assert.Len(t, prs, 1)
for _, pr := range prs {
assert.Equal(t, int64(1), pr.HeadRepoID)
assert.Equal(t, "branch2", pr.HeadBranch)
assert.Equal(t, "develop", pr.HeadBranch)
}
}

Expand Down Expand Up @@ -268,3 +268,77 @@ func TestPullRequest_GetWorkInProgressPrefixWorkInProgress(t *testing.T) {
pr.Issue.Title = "[wip] " + original
assert.Equal(t, "[wip]", pr.GetWorkInProgressPrefix())
}

func closeActivePullRequests(t *testing.T, pr *PullRequest, repo *Repository, branchName string) {
assert.NoError(t, repo.GetOwner())

CloseActivePullRequests(repo.Owner, repo, branchName)
pr.Issue = nil
pr.LoadIssue()
assert.Equal(t, true, pr.Issue.IsClosed)
}

func TestPullRequest_CloseActivePullRequests(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())

// Delete head branch.
pr := AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest)
pr.GetHeadRepo()
closeActivePullRequests(t, pr, pr.HeadRepo, pr.HeadBranch)

// Reopen pull request.
assert.NoError(t, pr.Issue.ChangeStatus(pr.HeadRepo.Owner, false))

// Delete base branch.
pr = AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest)
pr.GetBaseRepo()
closeActivePullRequests(t, pr, pr.BaseRepo, pr.BaseBranch)
}

func createTestPullRequest(t *testing.T, repo *Repository, user *User, baseBranch string, headBranch string) *PullRequest {
prIssue := &Issue{
RepoID: repo.ID,
Repo: repo,
Title: "test",
PosterID: user.ID,
Poster: user,
}
pr := &PullRequest{
HeadRepoID: repo.ID,
BaseRepoID: repo.ID,
HeadRepo: repo,
BaseRepo: repo,
HeadUserName: user.Name,
HeadBranch: headBranch,
BaseBranch: baseBranch,
Type: PullRequestGitea,
}
labelIDs := make([]int64, 0)
uuids := make([]string, 0)
patch := make([]byte, 0)
assigneeIDs := make([]int64, 0)
err := NewPullRequest(repo, prIssue, labelIDs, uuids, pr, patch, assigneeIDs)
assert.NoError(t, err)
return pr
}

func TestPullRequest_CloseActivePullRequestsDependent(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())

// Create a test pull request.
repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
user := AssertExistsAndLoadBean(t, &User{ID: 1}).(*User)
pr1 := AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest)
pr2 := createTestPullRequest(t, repo, user, "develop", "master")
pr1.LoadIssue()
pr2.LoadIssue()

// Add a dependency from pr2 to pr1.
err := CreateIssueDependency(user, pr2.Issue, pr1.Issue)
assert.NoError(t, err)

closeActivePullRequests(t, pr2, pr2.BaseRepo, pr2.BaseBranch)
pr1.Issue = nil
pr1.LoadIssue()
assert.Equal(t, true, pr1.Issue.IsClosed)
}