From 795d8cbe2b9260e66a57c04794cb8b50961d5a45 Mon Sep 17 00:00:00 2001 From: Viktor Kuzmin Date: Sun, 8 May 2022 23:46:00 +0300 Subject: [PATCH 1/4] Try to keep branch PRs open when branch is deleted cause of it's PR merged and closed, fixes #18408 --- routers/api/v1/repo/pull.go | 7 +++++++ routers/web/repo/pull.go | 9 +++++++++ services/pull/pull.go | 28 ++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 9b5ec0b3f8ed6..070a18195c7fa 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -904,6 +904,13 @@ func MergePullRequest(ctx *context.APIContext) { } defer headRepo.Close() } + if pr.BaseRepoID == pr.HeadRepoID { + if err := pull_service.RebaseBranchPulls(ctx, ctx.Doer, pr.HeadRepoID, pr.HeadBranch, pr.BaseBranch); err != nil { + log.Error("RebaseBranchPulls: %v", err) + ctx.Error(http.StatusInternalServerError, "RebaseBranchPulls", 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 4f99687738247..42529f7c0de97 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1399,6 +1399,15 @@ func CleanUpPullRequest(ctx *context.Context) { func deleteBranch(ctx *context.Context, pr *issues_model.PullRequest, gitRepo *git.Repository) { fullBranchName := pr.HeadRepo.FullName() + ":" + pr.HeadBranch + + if pr.BaseRepoID == pr.HeadRepoID { + if err := pull_service.RebaseBranchPulls(ctx, ctx.Doer, pr.HeadRepoID, pr.HeadBranch, pr.BaseBranch); err != nil { + log.Error("RebaseBranchPulls: %v", err) + 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 e1d5a6f86dd42..30cdd7e94f04f 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -497,6 +497,34 @@ func (errs errlist) Error() string { return "" } +// RebaseBranchPulls change target branch for all pull requests who's base branch is the branch +func RebaseBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch, targetBranch string) error { + prs, err := models.GetUnmergedPullRequestsByBaseInfo(repoID, branch) + if err != nil { + return err + } + + if err := models.PullRequestList(prs).LoadAttributes(); err != nil { + return err + } + + var errs errlist + for _, pr := range prs { + if err = pr.Issue.LoadAttributes(); err != nil { + errs = append(errs, err) + } else if err = ChangeTargetBranch(ctx, pr, doer, targetBranch); err != nil && + !models.IsErrIssueIsClosed(err) && !models.IsErrPullRequestHasMerged(err) && + !models.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(doer *user_model.User, repoID int64, branch string) error { prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(repoID, branch, false) From 660b3d2cba3913aa33b77eea1f7e9d57fdab463e Mon Sep 17 00:00:00 2001 From: Viktor Kuzmin Date: Mon, 9 May 2022 00:09:55 +0300 Subject: [PATCH 2/4] Make retarget optional, keep default behaviour without option --- modules/setting/repository.go | 2 ++ routers/api/v1/repo/pull.go | 2 +- routers/web/repo/pull.go | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/modules/setting/repository.go b/modules/setting/repository.go index bae3c658a4855..67fb71d9566fa 100644 --- a/modules/setting/repository.go +++ b/modules/setting/repository.go @@ -84,6 +84,7 @@ var ( PopulateSquashCommentWithCommitMessages bool AddCoCommitterTrailers bool TestConflictingPatchesWithGitApply bool + RetargetChildsOnClose bool } `ini:"repository.pull-request"` // Issue Setting @@ -207,6 +208,7 @@ var ( PopulateSquashCommentWithCommitMessages bool AddCoCommitterTrailers bool TestConflictingPatchesWithGitApply bool + RetargetChildsOnClose bool }{ WorkInProgressPrefixes: []string{"WIP:", "[WIP]"}, // Same as GitHub. See diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 070a18195c7fa..cc5aa4f696349 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -904,7 +904,7 @@ func MergePullRequest(ctx *context.APIContext) { } defer headRepo.Close() } - if pr.BaseRepoID == pr.HeadRepoID { + if setting.Repository.PullRequest.RetargetChildsOnClose && pr.BaseRepoID == pr.HeadRepoID { if err := pull_service.RebaseBranchPulls(ctx, ctx.Doer, pr.HeadRepoID, pr.HeadBranch, pr.BaseBranch); err != nil { log.Error("RebaseBranchPulls: %v", err) ctx.Error(http.StatusInternalServerError, "RebaseBranchPulls", err) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 42529f7c0de97..8b5bc5ced51be 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1400,7 +1400,7 @@ func CleanUpPullRequest(ctx *context.Context) { func deleteBranch(ctx *context.Context, pr *issues_model.PullRequest, gitRepo *git.Repository) { fullBranchName := pr.HeadRepo.FullName() + ":" + pr.HeadBranch - if pr.BaseRepoID == pr.HeadRepoID { + if setting.Repository.PullRequest.RetargetChildsOnClose && pr.BaseRepoID == pr.HeadRepoID { if err := pull_service.RebaseBranchPulls(ctx, ctx.Doer, pr.HeadRepoID, pr.HeadBranch, pr.BaseBranch); err != nil { log.Error("RebaseBranchPulls: %v", err) ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) From c7f03bb98b9075f47bc05074a8799736ee66009c Mon Sep 17 00:00:00 2001 From: Viktor Kuzmin Date: Sun, 26 Mar 2023 17:15:53 +0300 Subject: [PATCH 3/4] Cleanup error handling, add docs --- custom/conf/app.example.ini | 3 +++ .../config-cheat-sheet.en-us.md | 1 + modules/setting/repository.go | 5 +++-- routers/api/v1/repo/pull.go | 10 ++++----- routers/web/repo/pull.go | 10 ++++----- services/pull/pull.go | 22 +++++++++++++------ 6 files changed, 30 insertions(+), 21 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index d1cfcd70e5850..3c18a491cebef 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1062,6 +1062,9 @@ ROUTER = console ;; ;; 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 the child pull request to the parent pull request branch target +;RETARGET_CHILDREN_ON_MERGE = false ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/docs/content/doc/administration/config-cheat-sheet.en-us.md b/docs/content/doc/administration/config-cheat-sheet.en-us.md index 7f31a27f15066..0cbba4a425d0e 100644 --- a/docs/content/doc/administration/config-cheat-sheet.en-us.md +++ b/docs/content/doc/administration/config-cheat-sheet.en-us.md @@ -137,6 +137,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`: **false**: Retarget the child pull request to the parent pull request branch target. ### Repository - Issue (`repository.issue`) diff --git a/modules/setting/repository.go b/modules/setting/repository.go index 67fb71d9566fa..f9b66b7d91ec4 100644 --- a/modules/setting/repository.go +++ b/modules/setting/repository.go @@ -84,7 +84,7 @@ var ( PopulateSquashCommentWithCommitMessages bool AddCoCommitterTrailers bool TestConflictingPatchesWithGitApply bool - RetargetChildsOnClose bool + RetargetChildrenOnMerge bool } `ini:"repository.pull-request"` // Issue Setting @@ -208,7 +208,7 @@ var ( PopulateSquashCommentWithCommitMessages bool AddCoCommitterTrailers bool TestConflictingPatchesWithGitApply bool - RetargetChildsOnClose bool + RetargetChildrenOnMerge bool }{ WorkInProgressPrefixes: []string{"WIP:", "[WIP]"}, // Same as GitHub. See @@ -223,6 +223,7 @@ var ( DefaultMergeMessageOfficialApproversOnly: true, PopulateSquashCommentWithCommitMessages: false, AddCoCommitterTrailers: true, + RetargetChildrenOnMerge: false, }, // Issue settings diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index cc5aa4f696349..198b8f48023e2 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -904,12 +904,10 @@ func MergePullRequest(ctx *context.APIContext) { } defer headRepo.Close() } - if setting.Repository.PullRequest.RetargetChildsOnClose && pr.BaseRepoID == pr.HeadRepoID { - if err := pull_service.RebaseBranchPulls(ctx, ctx.Doer, pr.HeadRepoID, pr.HeadBranch, pr.BaseBranch); err != nil { - log.Error("RebaseBranchPulls: %v", err) - ctx.Error(http.StatusInternalServerError, "RebaseBranchPulls", err) - return - } + if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil { + log.Error("RetargetChildrenOnMerge: %v", err) + ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err) + return } if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil { switch { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 8b5bc5ced51be..31bd86feb3212 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1400,12 +1400,10 @@ func CleanUpPullRequest(ctx *context.Context) { func deleteBranch(ctx *context.Context, pr *issues_model.PullRequest, gitRepo *git.Repository) { fullBranchName := pr.HeadRepo.FullName() + ":" + pr.HeadBranch - if setting.Repository.PullRequest.RetargetChildsOnClose && pr.BaseRepoID == pr.HeadRepoID { - if err := pull_service.RebaseBranchPulls(ctx, ctx.Doer, pr.HeadRepoID, pr.HeadBranch, pr.BaseBranch); err != nil { - log.Error("RebaseBranchPulls: %v", err) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - return - } + if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil { + log.Error("RetargetChildrenOnMerge: %v", err) + 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 { diff --git a/services/pull/pull.go b/services/pull/pull.go index 30cdd7e94f04f..7227e32745f69 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -497,24 +497,32 @@ func (errs errlist) Error() string { return "" } -// RebaseBranchPulls change target branch for all pull requests who's base branch is the branch -func RebaseBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch, targetBranch string) error { - prs, err := models.GetUnmergedPullRequestsByBaseInfo(repoID, branch) +// 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 who's base branch is the branch +func RetargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch, targetBranch string) error { + prs, err := issues_model.GetUnmergedPullRequestsByBaseInfo(repoID, branch) if err != nil { return err } - if err := models.PullRequestList(prs).LoadAttributes(); err != nil { + if err := issues_model.PullRequestList(prs).LoadAttributes(); err != nil { return err } var errs errlist for _, pr := range prs { - if err = pr.Issue.LoadAttributes(); err != nil { + if err = pr.Issue.LoadAttributes(ctx); err != nil { errs = append(errs, err) } else if err = ChangeTargetBranch(ctx, pr, doer, targetBranch); err != nil && - !models.IsErrIssueIsClosed(err) && !models.IsErrPullRequestHasMerged(err) && - !models.IsErrPullRequestAlreadyExists(err) { + !issues_model.IsErrIssueIsClosed(err) && !models.IsErrPullRequestHasMerged(err) && + !issues_model.IsErrPullRequestAlreadyExists(err) { errs = append(errs, err) } } From 1e270a495e46b25af0412e8e7ccd3c2e464532cd Mon Sep 17 00:00:00 2001 From: Viktor Kuzmin Date: Sun, 26 Mar 2023 19:52:14 +0300 Subject: [PATCH 4/4] Docs and logs cleanup --- custom/conf/app.example.ini | 2 +- docs/content/doc/administration/config-cheat-sheet.en-us.md | 2 +- routers/api/v1/repo/pull.go | 1 - routers/web/repo/pull.go | 1 - 4 files changed, 2 insertions(+), 4 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 3c18a491cebef..1c54077cac612 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1063,7 +1063,7 @@ ROUTER = console ;; 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 the child pull request to the parent pull request branch target +;; Retarget child pull requests to the parent pull request branch target on merge of parent pull request ;RETARGET_CHILDREN_ON_MERGE = false ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/docs/content/doc/administration/config-cheat-sheet.en-us.md b/docs/content/doc/administration/config-cheat-sheet.en-us.md index 0cbba4a425d0e..f1e498f07eaca 100644 --- a/docs/content/doc/administration/config-cheat-sheet.en-us.md +++ b/docs/content/doc/administration/config-cheat-sheet.en-us.md @@ -137,7 +137,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`: **false**: Retarget the child pull request to the parent pull request branch target. +- `RETARGET_CHILDREN_ON_MERGE`: **false**: Retarget child pull requests to the parent pull request branch target on merge of parent pull request. ### Repository - Issue (`repository.issue`) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 198b8f48023e2..b3f3a53ef3c41 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -905,7 +905,6 @@ func MergePullRequest(ctx *context.APIContext) { defer headRepo.Close() } if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil { - log.Error("RetargetChildrenOnMerge: %v", err) ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err) return } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 31bd86feb3212..e170f48a93630 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1401,7 +1401,6 @@ func deleteBranch(ctx *context.Context, pr *issues_model.PullRequest, gitRepo *g fullBranchName := pr.HeadRepo.FullName() + ":" + pr.HeadBranch if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil { - log.Error("RetargetChildrenOnMerge: %v", err) ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) return }