From f7f627cee801c42a620583c92e70da69b04989e8 Mon Sep 17 00:00:00 2001 From: a1012112796 <1012112796@qq.com> Date: Mon, 3 Jan 2022 18:48:47 +0800 Subject: [PATCH 1/3] Don't delete branch if other PRs with this branch are open fix #18149 Signed-off-by: a1012112796 <1012112796@qq.com> --- routers/api/v1/repo/pull.go | 11 +++++++++++ routers/web/repo/issue.go | 14 +++++++++++++- routers/web/repo/pull.go | 22 ++++++++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 2b7280d39be0c..28c9bb11c3d4a 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -873,6 +873,17 @@ func MergePullRequest(ctx *context.APIContext) { log.Trace("Pull request merged: %d", pr.ID) if form.DeleteBranchAfterMerge { + // Don't cleanup when other pr use this branch as head branch + prs, err := models.GetUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) + if err != nil { + ctx.ServerError("GetUnmergedPullRequestsByHeadInfo", err) + return + } + if len(prs) > 0 { + ctx.Status(http.StatusOK) + return + } + var headRepo *git.Repository if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil { headRepo = ctx.Repo.GitRepo diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index d55b6c96b6aff..f9f903e113cf3 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1600,11 +1600,23 @@ func ViewIssue(ctx *context.Context) { } else { ctx.Data["WontSignReason"] = "not_signed_in" } - ctx.Data["IsPullBranchDeletable"] = canDelete && + + isPullBranchDeletable := canDelete && pull.HeadRepo != nil && git.IsBranchExist(ctx, pull.HeadRepo.RepoPath(), pull.HeadBranch) && (!pull.HasMerged || ctx.Data["HeadBranchCommitID"] == ctx.Data["PullHeadCommitID"]) + if isPullBranchDeletable && pull.HasMerged { + prs, err := models.GetUnmergedPullRequestsByHeadInfo(pull.HeadRepoID, pull.HeadBranch) + if err != nil { + ctx.ServerError("GetUnmergedPullRequestsByHeadInfo", err) + return + } + + isPullBranchDeletable = len(prs) == 0 + } + ctx.Data["IsPullBranchDeletable"] = isPullBranchDeletable + stillCanManualMerge := func() bool { if pull.HasMerged || issue.IsClosed || !ctx.IsSigned { return false diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 1a6a5e55e611f..b16e0da4f6d94 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1052,6 +1052,17 @@ func MergePullRequest(ctx *context.Context) { log.Trace("Pull request merged: %d", pr.ID) if form.DeleteBranchAfterMerge { + // Don't cleanup when other pr use this branch as head branch + prs, err := models.GetUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) + if err != nil { + ctx.ServerError("GetUnmergedPullRequestsByHeadInfo", err) + return + } + if len(prs) > 0 { + ctx.Redirect(issue.Link()) + return + } + var headRepo *git.Repository if ctx.Repo != nil && ctx.Repo.Repository != nil && pr.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil { headRepo = ctx.Repo.GitRepo @@ -1261,6 +1272,17 @@ func CleanUpPullRequest(ctx *context.Context) { return } + // Don't cleanup when other pr use this branch as head branch + prs, err := models.GetUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) + if err != nil { + ctx.ServerError("GetUnmergedPullRequestsByHeadInfo", err) + return + } + if len(prs) > 0 { + ctx.NotFound("CleanUpPullRequest", nil) + return + } + if err := pr.LoadHeadRepo(); err != nil { ctx.ServerError("LoadHeadRepo", err) return From 92466fe15b4d471ce10edb09f21ceb4b86ae48ad Mon Sep 17 00:00:00 2001 From: a1012112796 <1012112796@qq.com> Date: Mon, 3 Jan 2022 20:20:12 +0800 Subject: [PATCH 2/3] simplify --- models/pull_list.go | 10 ++++++++++ models/pull_test.go | 12 ++++++++++++ routers/api/v1/repo/pull.go | 6 +++--- routers/web/repo/issue.go | 6 +++--- routers/web/repo/pull.go | 12 ++++++------ 5 files changed, 34 insertions(+), 12 deletions(-) diff --git a/models/pull_list.go b/models/pull_list.go index 7c5b83ae5f275..c4621afbac3a8 100644 --- a/models/pull_list.go +++ b/models/pull_list.go @@ -59,6 +59,16 @@ func GetUnmergedPullRequestsByHeadInfo(repoID int64, branch string) ([]*PullRequ Find(&prs) } +// CheckUnmergedPullRequestsByHeadInfo check if the pull requests that are open and has not been merged +// by given head information (repo and branch) exist. +func CheckUnmergedPullRequestsByHeadInfo(repoID int64, branch string) (bool, error) { + return db.GetEngine(db.DefaultContext). + Where("head_repo_id = ? AND head_branch = ? AND has_merged = ? AND issue.is_closed = ? AND flow = ?", + repoID, branch, false, false, PullRequestFlowGithub). + Join("INNER", "issue", "issue.id = pull_request.issue_id"). + Exist(&PullRequest{}) +} + // GetUnmergedPullRequestsByBaseInfo returns all pull requests that are open and has not been merged // by given base information (repo and branch). func GetUnmergedPullRequestsByBaseInfo(repoID int64, branch string) ([]*PullRequest, error) { diff --git a/models/pull_test.go b/models/pull_test.go index f5e9d486ff79e..ec4a172f4d05e 100644 --- a/models/pull_test.go +++ b/models/pull_test.go @@ -107,6 +107,18 @@ func TestGetUnmergedPullRequest(t *testing.T) { assert.True(t, IsErrPullRequestNotExist(err)) } +func TestCheckUnmergedPullRequestsByHeadInfo(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + exist, err := CheckUnmergedPullRequestsByHeadInfo(1, "branch2") + assert.NoError(t, err) + assert.Equal(t, true, exist) + + exist, err = CheckUnmergedPullRequestsByHeadInfo(1, "not_exist_branch") + assert.NoError(t, err) + assert.Equal(t, false, exist) +} + func TestGetUnmergedPullRequestsByHeadInfo(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) prs, err := GetUnmergedPullRequestsByHeadInfo(1, "branch2") diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 28c9bb11c3d4a..2428f145db623 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -874,12 +874,12 @@ func MergePullRequest(ctx *context.APIContext) { if form.DeleteBranchAfterMerge { // Don't cleanup when other pr use this branch as head branch - prs, err := models.GetUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) + exist, err := models.CheckUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) if err != nil { - ctx.ServerError("GetUnmergedPullRequestsByHeadInfo", err) + ctx.ServerError("CheckUnmergedPullRequestsByHeadInfo", err) return } - if len(prs) > 0 { + if exist { ctx.Status(http.StatusOK) return } diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index f9f903e113cf3..2ff5982549e83 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1607,13 +1607,13 @@ func ViewIssue(ctx *context.Context) { (!pull.HasMerged || ctx.Data["HeadBranchCommitID"] == ctx.Data["PullHeadCommitID"]) if isPullBranchDeletable && pull.HasMerged { - prs, err := models.GetUnmergedPullRequestsByHeadInfo(pull.HeadRepoID, pull.HeadBranch) + exist, err := models.CheckUnmergedPullRequestsByHeadInfo(pull.HeadRepoID, pull.HeadBranch) if err != nil { - ctx.ServerError("GetUnmergedPullRequestsByHeadInfo", err) + ctx.ServerError("CheckUnmergedPullRequestsByHeadInfo", err) return } - isPullBranchDeletable = len(prs) == 0 + isPullBranchDeletable = !exist } ctx.Data["IsPullBranchDeletable"] = isPullBranchDeletable diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index b16e0da4f6d94..33ec4ffbc80c3 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1053,12 +1053,12 @@ func MergePullRequest(ctx *context.Context) { if form.DeleteBranchAfterMerge { // Don't cleanup when other pr use this branch as head branch - prs, err := models.GetUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) + exist, err := models.CheckUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) if err != nil { - ctx.ServerError("GetUnmergedPullRequestsByHeadInfo", err) + ctx.ServerError("CheckUnmergedPullRequestsByHeadInfo", err) return } - if len(prs) > 0 { + if exist { ctx.Redirect(issue.Link()) return } @@ -1273,12 +1273,12 @@ func CleanUpPullRequest(ctx *context.Context) { } // Don't cleanup when other pr use this branch as head branch - prs, err := models.GetUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) + exist, err := models.CheckUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) if err != nil { - ctx.ServerError("GetUnmergedPullRequestsByHeadInfo", err) + ctx.ServerError("CheckUnmergedPullRequestsByHeadInfo", err) return } - if len(prs) > 0 { + if exist { ctx.NotFound("CleanUpPullRequest", nil) return } From e0ccfefdc40937ba7951aa49580e404331241a88 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 3 Jan 2022 18:42:43 +0100 Subject: [PATCH 3/3] nits Co-authored-by: Gusted --- models/pull_list.go | 6 +++--- models/pull_test.go | 6 +++--- routers/api/v1/repo/pull.go | 6 +++--- routers/web/repo/issue.go | 4 ++-- routers/web/repo/pull.go | 10 +++++----- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/models/pull_list.go b/models/pull_list.go index c4621afbac3a8..9d4d42892883b 100644 --- a/models/pull_list.go +++ b/models/pull_list.go @@ -59,9 +59,9 @@ func GetUnmergedPullRequestsByHeadInfo(repoID int64, branch string) ([]*PullRequ Find(&prs) } -// CheckUnmergedPullRequestsByHeadInfo check if the pull requests that are open and has not been merged -// by given head information (repo and branch) exist. -func CheckUnmergedPullRequestsByHeadInfo(repoID int64, branch string) (bool, error) { +// HasUnmergedPullRequestsByHeadInfo checks if there are open and not merged pull request +// by given head information (repo and branch) +func HasUnmergedPullRequestsByHeadInfo(repoID int64, branch string) (bool, error) { return db.GetEngine(db.DefaultContext). Where("head_repo_id = ? AND head_branch = ? AND has_merged = ? AND issue.is_closed = ? AND flow = ?", repoID, branch, false, false, PullRequestFlowGithub). diff --git a/models/pull_test.go b/models/pull_test.go index ec4a172f4d05e..2567984cc1331 100644 --- a/models/pull_test.go +++ b/models/pull_test.go @@ -107,14 +107,14 @@ func TestGetUnmergedPullRequest(t *testing.T) { assert.True(t, IsErrPullRequestNotExist(err)) } -func TestCheckUnmergedPullRequestsByHeadInfo(t *testing.T) { +func TestHasUnmergedPullRequestsByHeadInfo(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) - exist, err := CheckUnmergedPullRequestsByHeadInfo(1, "branch2") + exist, err := HasUnmergedPullRequestsByHeadInfo(1, "branch2") assert.NoError(t, err) assert.Equal(t, true, exist) - exist, err = CheckUnmergedPullRequestsByHeadInfo(1, "not_exist_branch") + exist, err = HasUnmergedPullRequestsByHeadInfo(1, "not_exist_branch") assert.NoError(t, err) assert.Equal(t, false, exist) } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 2428f145db623..8297e35a17765 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -873,10 +873,10 @@ func MergePullRequest(ctx *context.APIContext) { log.Trace("Pull request merged: %d", pr.ID) if form.DeleteBranchAfterMerge { - // Don't cleanup when other pr use this branch as head branch - exist, err := models.CheckUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) + // Don't cleanup when there are other PR's that use this branch as head branch. + exist, err := models.HasUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) if err != nil { - ctx.ServerError("CheckUnmergedPullRequestsByHeadInfo", err) + ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) return } if exist { diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 2ff5982549e83..ea16de3950b7f 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1607,9 +1607,9 @@ func ViewIssue(ctx *context.Context) { (!pull.HasMerged || ctx.Data["HeadBranchCommitID"] == ctx.Data["PullHeadCommitID"]) if isPullBranchDeletable && pull.HasMerged { - exist, err := models.CheckUnmergedPullRequestsByHeadInfo(pull.HeadRepoID, pull.HeadBranch) + exist, err := models.HasUnmergedPullRequestsByHeadInfo(pull.HeadRepoID, pull.HeadBranch) if err != nil { - ctx.ServerError("CheckUnmergedPullRequestsByHeadInfo", err) + ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) return } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 33bc11cae6822..b40eb1ea170cd 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1052,9 +1052,9 @@ func MergePullRequest(ctx *context.Context) { if form.DeleteBranchAfterMerge { // Don't cleanup when other pr use this branch as head branch - exist, err := models.CheckUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) + exist, err := models.HasUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) if err != nil { - ctx.ServerError("CheckUnmergedPullRequestsByHeadInfo", err) + ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) return } if exist { @@ -1233,10 +1233,10 @@ func CleanUpPullRequest(ctx *context.Context) { return } - // Don't cleanup when other pr use this branch as head branch - exist, err := models.CheckUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) + // Don't cleanup when there are other PR's that use this branch as head branch. + exist, err := models.HasUnmergedPullRequestsByHeadInfo(pr.HeadRepoID, pr.HeadBranch) if err != nil { - ctx.ServerError("CheckUnmergedPullRequestsByHeadInfo", err) + ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) return } if exist {