Skip to content
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.

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

Add option to update pull request by rebase #16125

Merged
merged 18 commits into from
Aug 31, 2021
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
28 changes: 28 additions & 0 deletions integrations/pull_update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 2 additions & 0 deletions models/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -1463,7 +1463,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
Expand Down
13 changes: 10 additions & 3 deletions routers/api/v1/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -1066,6 +1066,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"
Expand Down Expand Up @@ -1112,21 +1117,23 @@ func UpdatePullRequest(ctx *context.APIContext) {
return
}

allowedUpdate, err := pull_service.IsUserAllowedToUpdate(pr, ctx.User)
rebase := ctx.Form("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
}

// 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
Expand Down
10 changes: 6 additions & 4 deletions routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,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
Expand Down Expand Up @@ -728,6 +728,8 @@ func UpdatePullRequest(ctx *context.Context) {
return
}

rebase := ctx.Form("style") == "rebase"

if err := issue.PullRequest.LoadBaseRepo(); err != nil {
ctx.ServerError("LoadBaseRepo", err)
return
Expand All @@ -737,14 +739,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
Expand All @@ -753,7 +755,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{}{
Expand Down
17 changes: 16 additions & 1 deletion services/pull/merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved

// 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())
Expand Down Expand Up @@ -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)
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
} 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(),
Expand Down
55 changes: 40 additions & 15 deletions services/pull/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -42,27 +53,31 @@ 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, "", "")
}()

return err
}

// 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{
Expand All @@ -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
}

mergeAllowed, err = IsUserAllowedToMerge(pr, headRepoPerm, user)
if err != nil {
return false, false, err
}

return IsUserAllowedToMerge(pr, headRepoPerm, user)
return mergeAllowed, rebaseAllowed, nil
}

// GetDiverging determines how many commits a PR is ahead or behind the PR base branch
Expand Down
42 changes: 40 additions & 2 deletions templates/repo/issue/view_content/pull.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,26 @@
{{$.i18n.Tr "repo.pulls.outdated_with_base_branch"}}
</div>
<div class="item-section-right">
{{if .UpdateAllowed}}
{{if and .UpdateAllowed .UpdateByRebaseAllowed }}
<div class="dib">
<div class="ui buttons update-button">
<button class="ui button" data-do="{{.Link}}/update" data-redirect="{{.Link}}">
<span class="button-text">
{{$.i18n.Tr "repo.pulls.update_branch"}}
</span>
</button>

<div class="ui dropdown icon button no-text">
{{svg "octicon-triangle-down" 14 "dropdown icon"}}
<div class="menu">
<div class="item active selected" data-do="{{.Link}}/update">{{$.i18n.Tr "repo.pulls.update_branch"}}</div>
<div class="item" data-do="{{.Link}}/update?style=rebase">{{$.i18n.Tr "repo.pulls.update_branch_rebase"}}</div>
</div>
</div>
</div>
</div>
{{end}}
{{if and .UpdateAllowed (not .UpdateByRebaseAllowed)}}
<form action="{{.Link}}/update" method="post" class="ui update-branch-form">
{{.CsrfTokenHtml}}
<button class="ui compact button" data-do="update">
Expand Down Expand Up @@ -560,7 +579,26 @@
{{$.i18n.Tr "repo.pulls.outdated_with_base_branch"}}
</div>
<div>
{{if .UpdateAllowed}}
{{if and .UpdateAllowed .UpdateByRebaseAllowed }}
<div class="dib">
<div class="ui buttons update-button">
<button class="ui button" data-do="{{.Link}}/update" data-redirect="{{.Link}}">
<span class="button-text">
{{$.i18n.Tr "repo.pulls.update_branch"}}
</span>
</button>

<div class="ui dropdown icon button no-text">
{{svg "octicon-triangle-down" 14 "dropdown icon"}}
<div class="menu">
<div class="item active selected" data-do="{{.Link}}/update">{{$.i18n.Tr "repo.pulls.update_branch"}}</div>
<div class="item" data-do="{{.Link}}/update?style=rebase">{{$.i18n.Tr "repo.pulls.update_branch_rebase"}}</div>
</div>
</div>
</div>
</div>
{{end}}
{{if and .UpdateAllowed (not .UpdateByRebaseAllowed)}}
<form action="{{.Link}}/update" method="post">
{{.CsrfTokenHtml}}
<button class="ui compact button" data-do="update">
Expand Down
10 changes: 10 additions & 0 deletions templates/swagger/v1_json.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -8070,6 +8070,16 @@
"name": "index",
"in": "path",
"required": true
},
{
"enum": [
"merge",
"rebase"
],
"type": "string",
"description": "how to update pull request",
"name": "style",
"in": "query"
}
],
"responses": {
Expand Down
30 changes: 30 additions & 0 deletions web_src/js/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -1267,6 +1267,36 @@ async function initRepository() {
$('.instruct-toggle').show();
});

// Pull Request update button
const $pullUpdateButton = $('.update-button > button');
$pullUpdateButton.on('click', function (e) {
e.preventDefault();
const $this = $(this);
const redirect = $this.data('redirect');
$this.addClass('loading');
$.post($this.data('do'), {
_csrf: csrf
}).done((data) => {
if (data.redirect) {
window.location.href = data.redirect;
} else if (redirect) {
window.location.href = redirect;
} else {
window.location.reload();
}
});
});

$('.update-button > .dropdown').dropdown({
onChange(_text, _value, $choice) {
const $url = $choice.data('do');
if ($url) {
$pullUpdateButton.find('.button-text').text($choice.text());
$pullUpdateButton.data('do', $url);
}
}
});

initReactionSelector();
}

Expand Down