From cbf05c3f795c1eed999dafe8d0757495e07f1ee2 Mon Sep 17 00:00:00 2001 From: a1012112796 <1012112796@qq.com> Date: Tue, 31 Aug 2021 22:03:45 +0800 Subject: [PATCH] Add option to update pull request by `rebase` (#16125) * add option to update pull request by `rebase` Signed-off-by: a1012112796 <1012112796@qq.com> --- integrations/pull_update_test.go | 28 +++++++++++ models/pull.go | 2 + options/locale/locale_en-US.ini | 3 +- routers/api/v1/repo/pull.go | 13 +++-- routers/web/repo/pull.go | 10 ++-- services/pull/merge.go | 17 ++++++- services/pull/update.go | 55 +++++++++++++++------ templates/repo/issue/view_content/pull.tmpl | 42 +++++++++++++++- templates/swagger/v1_json.tmpl | 10 ++++ web_src/js/index.js | 30 +++++++++++ 10 files changed, 184 insertions(+), 26 deletions(-) diff --git a/integrations/pull_update_test.go b/integrations/pull_update_test.go index 46a1e04b25..3e3b987b64 100644 --- a/integrations/pull_update_test.go +++ b/integrations/pull_update_test.go @@ -47,6 +47,34 @@ func TestAPIPullUpdate(t *testing.T) { }) } +func TestAPIPullUpdateByRebase(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + //Create PR to test + user := models.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) + org26 := models.AssertExistsAndLoadBean(t, &models.User{ID: 26}).(*models.User) + pr := createOutdatedPR(t, user, org26) + + //Test GetDiverging + diffCount, err := pull_service.GetDiverging(pr) + assert.NoError(t, err) + assert.EqualValues(t, 1, diffCount.Behind) + assert.EqualValues(t, 1, diffCount.Ahead) + assert.NoError(t, pr.LoadBaseRepo()) + assert.NoError(t, pr.LoadIssue()) + + session := loginUser(t, "user2") + token := getTokenForLoggedInUser(t, session) + req := NewRequestf(t, "POST", "/api/v1/repos/%s/%s/pulls/%d/update?style=rebase&token="+token, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, pr.Issue.Index) + session.MakeRequest(t, req, http.StatusOK) + + //Test GetDiverging after update + diffCount, err = pull_service.GetDiverging(pr) + assert.NoError(t, err) + assert.EqualValues(t, 0, diffCount.Behind) + assert.EqualValues(t, 1, diffCount.Ahead) + }) +} + func createOutdatedPR(t *testing.T, actor, forkOrg *models.User) *models.PullRequest { baseRepo, err := repo_service.CreateRepository(actor, actor, models.CreateRepoOptions{ Name: "repo-pr-update", diff --git a/models/pull.go b/models/pull.go index e8e815385d..ea8f289993 100644 --- a/models/pull.go +++ b/models/pull.go @@ -373,6 +373,8 @@ const ( MergeStyleSquash MergeStyle = "squash" // MergeStyleManuallyMerged pr has been merged manually, just mark it as merged directly MergeStyleManuallyMerged MergeStyle = "manually-merged" + // MergeStyleRebaseUpdate not a merge style, used to update pull head by rebase + MergeStyleRebaseUpdate MergeStyle = "rebase-update-only" ) // SetMerged sets a pull request to merged and closes the corresponding issue diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 76536f2d49..3462a15cca 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1468,7 +1468,8 @@ pulls.status_checks_failure = Some checks failed pulls.status_checks_error = Some checks reported errors pulls.status_checks_requested = Required pulls.status_checks_details = Details -pulls.update_branch = Update branch +pulls.update_branch = Update branch by merge +pulls.update_branch_rebase = Update branch by rebase pulls.update_branch_success = Branch update was successful pulls.update_not_allowed = You are not allowed to update branch pulls.outdated_with_base_branch = This branch is out-of-date with the base branch diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 0a903101c7..e493e720fb 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1065,6 +1065,11 @@ func UpdatePullRequest(ctx *context.APIContext) { // type: integer // format: int64 // required: true + // - name: style + // in: query + // description: how to update pull request + // type: string + // enum: [merge, rebase] // responses: // "200": // "$ref": "#/responses/empty" @@ -1111,13 +1116,15 @@ func UpdatePullRequest(ctx *context.APIContext) { return } - allowedUpdate, err := pull_service.IsUserAllowedToUpdate(pr, ctx.User) + rebase := ctx.FormString("style") == "rebase" + + allowedUpdateByMerge, allowedUpdateByRebase, err := pull_service.IsUserAllowedToUpdate(pr, ctx.User) if err != nil { ctx.Error(http.StatusInternalServerError, "IsUserAllowedToMerge", err) return } - if !allowedUpdate { + if (!allowedUpdateByMerge && !rebase) || (rebase && !allowedUpdateByRebase) { ctx.Status(http.StatusForbidden) return } @@ -1125,7 +1132,7 @@ func UpdatePullRequest(ctx *context.APIContext) { // default merge commit message message := fmt.Sprintf("Merge branch '%s' into %s", pr.BaseBranch, pr.HeadBranch) - if err = pull_service.Update(pr, ctx.User, message); err != nil { + if err = pull_service.Update(pr, ctx.User, message, rebase); err != nil { if models.IsErrMergeConflicts(err) { ctx.Error(http.StatusConflict, "Update", "merge failed because of conflict") return diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 8ff2ddf394..885ac3391a 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -450,7 +450,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare } if headBranchExist { - ctx.Data["UpdateAllowed"], err = pull_service.IsUserAllowedToUpdate(pull, ctx.User) + ctx.Data["UpdateAllowed"], ctx.Data["UpdateByRebaseAllowed"], err = pull_service.IsUserAllowedToUpdate(pull, ctx.User) if err != nil { ctx.ServerError("IsUserAllowedToUpdate", err) return nil @@ -724,6 +724,8 @@ func UpdatePullRequest(ctx *context.Context) { return } + rebase := ctx.FormString("style") == "rebase" + if err := issue.PullRequest.LoadBaseRepo(); err != nil { ctx.ServerError("LoadBaseRepo", err) return @@ -733,14 +735,14 @@ func UpdatePullRequest(ctx *context.Context) { return } - allowedUpdate, err := pull_service.IsUserAllowedToUpdate(issue.PullRequest, ctx.User) + allowedUpdateByMerge, allowedUpdateByRebase, err := pull_service.IsUserAllowedToUpdate(issue.PullRequest, ctx.User) if err != nil { ctx.ServerError("IsUserAllowedToMerge", err) return } // ToDo: add check if maintainers are allowed to change branch ... (need migration & co) - if !allowedUpdate { + if (!allowedUpdateByMerge && !rebase) || (rebase && !allowedUpdateByRebase) { ctx.Flash.Error(ctx.Tr("repo.pulls.update_not_allowed")) ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + fmt.Sprint(issue.Index)) return @@ -749,7 +751,7 @@ func UpdatePullRequest(ctx *context.Context) { // default merge commit message message := fmt.Sprintf("Merge branch '%s' into %s", issue.PullRequest.BaseBranch, issue.PullRequest.HeadBranch) - if err = pull_service.Update(issue.PullRequest, ctx.User, message); err != nil { + if err = pull_service.Update(issue.PullRequest, ctx.User, message, rebase); err != nil { if models.IsErrMergeConflicts(err) { conflictError := err.(models.ErrMergeConflicts) flashError, err := ctx.HTMLString(string(tplAlertDetails), map[string]interface{}{ diff --git a/services/pull/merge.go b/services/pull/merge.go index 7e6a214b87..ef797e1ca4 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -253,6 +253,8 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge } case models.MergeStyleRebase: fallthrough + case models.MergeStyleRebaseUpdate: + fallthrough case models.MergeStyleRebaseMerge: // Checkout head branch if err := git.NewCommand("checkout", "-b", stagingBranch, trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { @@ -305,6 +307,11 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge outbuf.Reset() errbuf.Reset() + // not need merge, just update by rebase. so skip + if mergeStyle == models.MergeStyleRebaseUpdate { + break + } + // Checkout base branch again if err := git.NewCommand("checkout", baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { log.Error("git checkout base prior to merge post staging rebase [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) @@ -410,8 +417,16 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge pr.ID, ) + var pushCmd *git.Command + if mergeStyle == models.MergeStyleRebaseUpdate { + // force push the rebase result to head brach + pushCmd = git.NewCommand("push", "-f", "head_repo", stagingBranch+":refs/heads/"+pr.HeadBranch) + } else { + pushCmd = git.NewCommand("push", "origin", baseBranch+":refs/heads/"+pr.BaseBranch) + } + // Push back to upstream. - if err := git.NewCommand("push", "origin", baseBranch+":refs/heads/"+pr.BaseBranch).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, &outbuf, &errbuf); err != nil { + if err := pushCmd.RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, &outbuf, &errbuf); err != nil { if strings.Contains(errbuf.String(), "non-fast-forward") { return "", &git.ErrPushOutOfDate{ StdOut: outbuf.String(), diff --git a/services/pull/update.go b/services/pull/update.go index c2c13845e3..4b4e67797e 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -13,13 +13,24 @@ import ( ) // Update updates pull request with base branch. -func Update(pull *models.PullRequest, doer *models.User, message string) error { - //use merge functions but switch repo's and branch's - pr := &models.PullRequest{ - HeadRepoID: pull.BaseRepoID, - BaseRepoID: pull.HeadRepoID, - HeadBranch: pull.BaseBranch, - BaseBranch: pull.HeadBranch, +func Update(pull *models.PullRequest, doer *models.User, message string, rebase bool) error { + var ( + pr *models.PullRequest + style models.MergeStyle + ) + + if rebase { + pr = pull + style = models.MergeStyleRebaseUpdate + } else { + //use merge functions but switch repo's and branch's + pr = &models.PullRequest{ + HeadRepoID: pull.BaseRepoID, + BaseRepoID: pull.HeadRepoID, + HeadBranch: pull.BaseBranch, + BaseBranch: pull.HeadBranch, + } + style = models.MergeStyleMerge } if pull.Flow == models.PullRequestFlowAGit { @@ -42,9 +53,13 @@ func Update(pull *models.PullRequest, doer *models.User, message string) error { return fmt.Errorf("HeadBranch of PR %d is up to date", pull.Index) } - _, err = rawMerge(pr, doer, models.MergeStyleMerge, message) + _, err = rawMerge(pr, doer, style, message) defer func() { + if rebase { + go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "") + return + } go AddTestPullRequestTask(doer, pr.HeadRepo.ID, pr.HeadBranch, false, "", "") }() @@ -52,17 +67,17 @@ func Update(pull *models.PullRequest, doer *models.User, message string) error { } // IsUserAllowedToUpdate check if user is allowed to update PR with given permissions and branch protections -func IsUserAllowedToUpdate(pull *models.PullRequest, user *models.User) (bool, error) { +func IsUserAllowedToUpdate(pull *models.PullRequest, user *models.User) (mergeAllowed, rebaseAllowed bool, err error) { if pull.Flow == models.PullRequestFlowAGit { - return false, nil + return false, false, nil } if user == nil { - return false, nil + return false, false, nil } headRepoPerm, err := models.GetUserRepoPermission(pull.HeadRepo, user) if err != nil { - return false, err + return false, false, err } pr := &models.PullRequest{ @@ -74,15 +89,25 @@ func IsUserAllowedToUpdate(pull *models.PullRequest, user *models.User) (bool, e err = pr.LoadProtectedBranch() if err != nil { - return false, err + return false, false, err + } + + // can't do rebase on protected branch because need force push + if pr.ProtectedBranch == nil { + rebaseAllowed = true } // Update function need push permission if pr.ProtectedBranch != nil && !pr.ProtectedBranch.CanUserPush(user.ID) { - return false, nil + return false, false, nil } - return IsUserAllowedToMerge(pr, headRepoPerm, user) + mergeAllowed, err = IsUserAllowedToMerge(pr, headRepoPerm, user) + if err != nil { + return false, false, err + } + + return mergeAllowed, rebaseAllowed, nil } // GetDiverging determines how many commits a PR is ahead or behind the PR base branch diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 15ce63b4d3..2e624a9fbf 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -281,7 +281,26 @@ {{$.i18n.Tr "repo.pulls.outdated_with_base_branch"}}
- {{if .UpdateAllowed}} + {{if and .UpdateAllowed .UpdateByRebaseAllowed }} +
+
+ + +
+ {{svg "octicon-triangle-down" 14 "dropdown icon"}} +
+
{{$.i18n.Tr "repo.pulls.update_branch"}}
+
{{$.i18n.Tr "repo.pulls.update_branch_rebase"}}
+
+
+
+
+ {{end}} + {{if and .UpdateAllowed (not .UpdateByRebaseAllowed)}}
{{.CsrfTokenHtml}}
- {{if .UpdateAllowed}} + {{if and .UpdateAllowed .UpdateByRebaseAllowed }} +
+
+ + +
+ {{svg "octicon-triangle-down" 14 "dropdown icon"}} +
+
{{$.i18n.Tr "repo.pulls.update_branch"}}
+
{{$.i18n.Tr "repo.pulls.update_branch_rebase"}}
+
+
+
+
+ {{end}} + {{if and .UpdateAllowed (not .UpdateByRebaseAllowed)}} {{.CsrfTokenHtml}}