diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 301e88b14e645..b0875123b7b8b 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1067,6 +1067,9 @@ LEVEL = Info ;; ;; In addition to testing patches using the three-way merge method, re-test conflicting patches with git apply ;TEST_CONFLICTING_PATCHES_WITH_GIT_APPLY = false +;; +;; Retarget child pull requests to the parent pull request branch target on merge of parent pull request. It only works on merged PRs where the head and base branch target the same repo. +;RETARGET_CHILDREN_ON_MERGE = true ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/docs/content/administration/config-cheat-sheet.en-us.md b/docs/content/administration/config-cheat-sheet.en-us.md index c9e99ea54fcef..eb9b8d1ae9a43 100644 --- a/docs/content/administration/config-cheat-sheet.en-us.md +++ b/docs/content/administration/config-cheat-sheet.en-us.md @@ -135,6 +135,7 @@ In addition, there is _`StaticRootPath`_ which can be set as a built-in at build - `POPULATE_SQUASH_COMMENT_WITH_COMMIT_MESSAGES`: **false**: In default squash-merge messages include the commit message of all commits comprising the pull request. - `ADD_CO_COMMITTER_TRAILERS`: **true**: Add co-authored-by and co-committed-by trailers to merge commit messages if committer does not match author. - `TEST_CONFLICTING_PATCHES_WITH_GIT_APPLY`: **false**: PR patches are tested using a three-way merge method to discover if there are conflicts. If this setting is set to **true**, conflicting patches will be retested using `git apply` - This was the previous behaviour in 1.18 (and earlier) but is somewhat inefficient. Please report if you find that this setting is required. +- `RETARGET_CHILDREN_ON_MERGE`: **true**: Retarget child pull requests to the parent pull request branch target on merge of parent pull request. It only works on merged PRs where the head and base branch target the same repo. ### Repository - Issue (`repository.issue`) diff --git a/modules/setting/repository.go b/modules/setting/repository.go index 9697a851d3ba9..a6f0ed8833589 100644 --- a/modules/setting/repository.go +++ b/modules/setting/repository.go @@ -85,6 +85,7 @@ var ( PopulateSquashCommentWithCommitMessages bool AddCoCommitterTrailers bool TestConflictingPatchesWithGitApply bool + RetargetChildrenOnMerge bool } `ini:"repository.pull-request"` // Issue Setting @@ -209,6 +210,7 @@ var ( PopulateSquashCommentWithCommitMessages bool AddCoCommitterTrailers bool TestConflictingPatchesWithGitApply bool + RetargetChildrenOnMerge bool }{ WorkInProgressPrefixes: []string{"WIP:", "[WIP]"}, // Same as GitHub. See @@ -223,6 +225,7 @@ var ( DefaultMergeMessageOfficialApproversOnly: true, PopulateSquashCommentWithCommitMessages: false, AddCoCommitterTrailers: true, + RetargetChildrenOnMerge: true, }, // Issue settings diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 34129ad5958c8..b1cb7011f1d4b 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -913,6 +913,10 @@ func MergePullRequest(ctx *context.APIContext) { } defer headRepo.Close() } + if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil { + ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err) + return + } if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil { switch { case git.IsErrBranchNotExist(err): diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 32d82b2a643cb..e36d7092af286 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1587,6 +1587,12 @@ func CleanUpPullRequest(ctx *context.Context) { func deleteBranch(ctx *context.Context, pr *issues_model.PullRequest, gitRepo *git.Repository) { fullBranchName := pr.HeadRepo.FullName() + ":" + pr.HeadBranch + + if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil { + ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) + return + } + if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, gitRepo, pr.HeadBranch); err != nil { switch { case git.IsErrBranchNotExist(err): diff --git a/services/pull/pull.go b/services/pull/pull.go index d1630f3792c07..930954bdfdfa1 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -546,6 +546,43 @@ func (errs errlist) Error() string { return "" } +// RetargetChildrenOnMerge retarget children pull requests on merge if possible +func RetargetChildrenOnMerge(ctx context.Context, doer *user_model.User, pr *issues_model.PullRequest) error { + if setting.Repository.PullRequest.RetargetChildrenOnMerge && pr.BaseRepoID == pr.HeadRepoID { + return RetargetBranchPulls(ctx, doer, pr.HeadRepoID, pr.HeadBranch, pr.BaseBranch) + } + return nil +} + +// RetargetBranchPulls change target branch for all pull requests whose base branch is the branch +// Both branch and targetBranch must be in the same repo (for security reasons) +func RetargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch, targetBranch string) error { + prs, err := issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch) + if err != nil { + return err + } + + if err := issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil { + return err + } + + var errs errlist + for _, pr := range prs { + if err = pr.Issue.LoadRepo(ctx); err != nil { + errs = append(errs, err) + } else if err = ChangeTargetBranch(ctx, pr, doer, targetBranch); err != nil && + !issues_model.IsErrIssueIsClosed(err) && !models.IsErrPullRequestHasMerged(err) && + !issues_model.IsErrPullRequestAlreadyExists(err) { + errs = append(errs, err) + } + } + + if len(errs) > 0 { + return errs + } + return nil +} + // CloseBranchPulls close all the pull requests who's head branch is the branch func CloseBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch string) error { prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repoID, branch) diff --git a/tests/integration/pull_create_test.go b/tests/integration/pull_create_test.go index a6ee0d9dfa1ef..029ea65d7190f 100644 --- a/tests/integration/pull_create_test.go +++ b/tests/integration/pull_create_test.go @@ -17,7 +17,7 @@ import ( "github.com/stretchr/testify/assert" ) -func testPullCreate(t *testing.T, session *TestSession, user, repo, branch, title string) *httptest.ResponseRecorder { +func testPullCreate(t *testing.T, session *TestSession, user, repo string, toSelf bool, targetBranch, sourceBranch, title string) *httptest.ResponseRecorder { req := NewRequest(t, "GET", path.Join(user, repo)) resp := session.MakeRequest(t, req, http.StatusOK) @@ -25,8 +25,21 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo, branch, titl htmlDoc := NewHTMLParser(t, resp.Body) link, exists := htmlDoc.doc.Find("#new-pull-request").Attr("href") assert.True(t, exists, "The template has changed") - if branch != "master" { - link = strings.Replace(link, ":master", ":"+branch, 1) + + targetUser := strings.Split(link, "/")[1] + if toSelf && targetUser != user { + link = strings.Replace(link, targetUser, user, 1) + } + + if targetBranch != "master" { + link = strings.Replace(link, "master...", targetBranch+"...", 1) + } + if sourceBranch != "master" { + if targetUser == user { + link = strings.Replace(link, "...master", "..."+sourceBranch, 1) + } else { + link = strings.Replace(link, ":master", ":"+sourceBranch, 1) + } } req = NewRequest(t, "GET", link) @@ -49,7 +62,7 @@ func TestPullCreate(t *testing.T) { session := loginUser(t, "user1") testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") - resp := testPullCreate(t, session, "user1", "repo1", "master", "This is a pull title") + resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title") // check the redirected URL url := test.RedirectURL(resp) @@ -77,7 +90,7 @@ func TestPullCreate_TitleEscape(t *testing.T) { session := loginUser(t, "user1") testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") - resp := testPullCreate(t, session, "user1", "repo1", "master", "XSS PR") + resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "XSS PR") // check the redirected URL url := test.RedirectURL(resp) @@ -142,7 +155,7 @@ func TestPullBranchDelete(t *testing.T) { testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testCreateBranch(t, session, "user1", "repo1", "branch/master", "master1", http.StatusSeeOther) testEditFile(t, session, "user1", "repo1", "master1", "README.md", "Hello, World (Edited)\n") - resp := testPullCreate(t, session, "user1", "repo1", "master1", "This is a pull title") + resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master1", "This is a pull title") // check the redirected URL url := test.RedirectURL(resp) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 35af47f802fd6..2aa6742a56ac0 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -35,16 +35,23 @@ import ( "github.com/stretchr/testify/assert" ) -func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum string, mergeStyle repo_model.MergeStyle) *httptest.ResponseRecorder { +func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum string, mergeStyle repo_model.MergeStyle, deleteBranch bool) *httptest.ResponseRecorder { req := NewRequest(t, "GET", path.Join(user, repo, "pulls", pullnum)) resp := session.MakeRequest(t, req, http.StatusOK) htmlDoc := NewHTMLParser(t, resp.Body) link := path.Join(user, repo, "pulls", pullnum, "merge") - req = NewRequestWithValues(t, "POST", link, map[string]string{ + + options := map[string]string{ "_csrf": htmlDoc.GetCSRF(), "do": string(mergeStyle), - }) + } + + if deleteBranch { + options["delete_branch_after_merge"] = "on" + } + + req = NewRequestWithValues(t, "POST", link, options) resp = session.MakeRequest(t, req, http.StatusOK) respJSON := struct { @@ -83,11 +90,11 @@ func TestPullMerge(t *testing.T) { testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") - resp := testPullCreate(t, session, "user1", "repo1", "master", "This is a pull title") + resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title") elem := strings.Split(test.RedirectURL(resp), "/") assert.EqualValues(t, "pulls", elem[3]) - testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge) + testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false) hookTasks, err = webhook.HookTasks(db.DefaultContext, 1, 1) assert.NoError(t, err) @@ -105,11 +112,11 @@ func TestPullRebase(t *testing.T) { testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") - resp := testPullCreate(t, session, "user1", "repo1", "master", "This is a pull title") + resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title") elem := strings.Split(test.RedirectURL(resp), "/") assert.EqualValues(t, "pulls", elem[3]) - testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleRebase) + testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleRebase, false) hookTasks, err = webhook.HookTasks(db.DefaultContext, 1, 1) assert.NoError(t, err) @@ -127,11 +134,11 @@ func TestPullRebaseMerge(t *testing.T) { testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") - resp := testPullCreate(t, session, "user1", "repo1", "master", "This is a pull title") + resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title") elem := strings.Split(test.RedirectURL(resp), "/") assert.EqualValues(t, "pulls", elem[3]) - testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleRebaseMerge) + testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleRebaseMerge, false) hookTasks, err = webhook.HookTasks(db.DefaultContext, 1, 1) assert.NoError(t, err) @@ -150,11 +157,11 @@ func TestPullSquash(t *testing.T) { testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited!)\n") - resp := testPullCreate(t, session, "user1", "repo1", "master", "This is a pull title") + resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title") elem := strings.Split(test.RedirectURL(resp), "/") assert.EqualValues(t, "pulls", elem[3]) - testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleSquash) + testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleSquash, false) hookTasks, err = webhook.HookTasks(db.DefaultContext, 1, 1) assert.NoError(t, err) @@ -168,11 +175,11 @@ func TestPullCleanUpAfterMerge(t *testing.T) { testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md", "Hello, World (Edited - TestPullCleanUpAfterMerge)\n") - resp := testPullCreate(t, session, "user1", "repo1", "feature/test", "This is a pull title") + resp := testPullCreate(t, session, "user1", "repo1", false, "master", "feature/test", "This is a pull title") elem := strings.Split(test.RedirectURL(resp), "/") assert.EqualValues(t, "pulls", elem[3]) - testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge) + testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false) // Check PR branch deletion resp = testPullCleanUp(t, session, elem[1], elem[2], elem[4]) @@ -203,7 +210,7 @@ func TestCantMergeWorkInProgress(t *testing.T) { testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") - resp := testPullCreate(t, session, "user1", "repo1", "master", "[wip] This is a pull title") + resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "[wip] This is a pull title") req := NewRequest(t, "GET", test.RedirectURL(resp)) resp = session.MakeRequest(t, req, http.StatusOK) @@ -435,3 +442,63 @@ func TestConflictChecking(t *testing.T) { assert.False(t, conflictingPR.Mergeable(db.DefaultContext)) }) } + +func TestPullRetargetChildOnBranchDelete(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + session := loginUser(t, "user1") + testEditFileToNewBranch(t, session, "user2", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - TestPullRetargetOnCleanup - base PR)\n") + testRepoFork(t, session, "user2", "repo1", "user1", "repo1") + testEditFileToNewBranch(t, session, "user1", "repo1", "base-pr", "child-pr", "README.md", "Hello, World\n(Edited - TestPullRetargetOnCleanup - base PR)\n(Edited - TestPullRetargetOnCleanup - child PR)") + + respBasePR := testPullCreate(t, session, "user2", "repo1", true, "master", "base-pr", "Base Pull Request") + elemBasePR := strings.Split(test.RedirectURL(respBasePR), "/") + assert.EqualValues(t, "pulls", elemBasePR[3]) + + respChildPR := testPullCreate(t, session, "user1", "repo1", false, "base-pr", "child-pr", "Child Pull Request") + elemChildPR := strings.Split(test.RedirectURL(respChildPR), "/") + assert.EqualValues(t, "pulls", elemChildPR[3]) + + testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true) + + // Check child PR + req := NewRequest(t, "GET", test.RedirectURL(respChildPR)) + resp := session.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + targetBranch := htmlDoc.doc.Find("#branch_target>a").Text() + prStatus := strings.TrimSpace(htmlDoc.doc.Find(".issue-title-meta>.issue-state-label").Text()) + + assert.EqualValues(t, "master", targetBranch) + assert.EqualValues(t, "Open", prStatus) + }) +} + +func TestPullDontRetargetChildOnWrongRepo(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + session := loginUser(t, "user1") + testRepoFork(t, session, "user2", "repo1", "user1", "repo1") + testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - TestPullDontRetargetChildOnWrongRepo - base PR)\n") + testEditFileToNewBranch(t, session, "user1", "repo1", "base-pr", "child-pr", "README.md", "Hello, World\n(Edited - TestPullDontRetargetChildOnWrongRepo - base PR)\n(Edited - TestPullDontRetargetChildOnWrongRepo - child PR)") + + respBasePR := testPullCreate(t, session, "user1", "repo1", false, "master", "base-pr", "Base Pull Request") + elemBasePR := strings.Split(test.RedirectURL(respBasePR), "/") + assert.EqualValues(t, "pulls", elemBasePR[3]) + + respChildPR := testPullCreate(t, session, "user1", "repo1", true, "base-pr", "child-pr", "Child Pull Request") + elemChildPR := strings.Split(test.RedirectURL(respChildPR), "/") + assert.EqualValues(t, "pulls", elemChildPR[3]) + + testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true) + + // Check child PR + req := NewRequest(t, "GET", test.RedirectURL(respChildPR)) + resp := session.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + targetBranch := htmlDoc.doc.Find("#branch_target>a").Text() + prStatus := strings.TrimSpace(htmlDoc.doc.Find(".issue-title-meta>.issue-state-label").Text()) + + assert.EqualValues(t, "base-pr", targetBranch) + assert.EqualValues(t, "Closed", prStatus) + }) +} diff --git a/tests/integration/repo_activity_test.go b/tests/integration/repo_activity_test.go index c8d0c46d6455e..792554db4bc93 100644 --- a/tests/integration/repo_activity_test.go +++ b/tests/integration/repo_activity_test.go @@ -22,16 +22,16 @@ func TestRepoActivity(t *testing.T) { // Create PRs (1 merged & 2 proposed) testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") - resp := testPullCreate(t, session, "user1", "repo1", "master", "This is a pull title") + resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title") elem := strings.Split(test.RedirectURL(resp), "/") assert.EqualValues(t, "pulls", elem[3]) - testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge) + testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false) testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feat/better_readme", "README.md", "Hello, World (Edited Again)\n") - testPullCreate(t, session, "user1", "repo1", "feat/better_readme", "This is a pull title") + testPullCreate(t, session, "user1", "repo1", false, "master", "feat/better_readme", "This is a pull title") testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feat/much_better_readme", "README.md", "Hello, World (Edited More)\n") - testPullCreate(t, session, "user1", "repo1", "feat/much_better_readme", "This is a pull title") + testPullCreate(t, session, "user1", "repo1", false, "master", "feat/much_better_readme", "This is a pull title") // Create issues (3 new issues) testNewIssue(t, session, "user2", "repo1", "Issue 1", "Description 1")