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

Abort merge if head has been updated before pressing merge #18032

Merged
Merged
Show file tree
Hide file tree
Changes from 5 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
6 changes: 3 additions & 3 deletions integrations/pull_merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,11 +241,11 @@ func TestCantMergeConflict(t *testing.T) {
gitRepo, err := git.OpenRepository(repo_model.RepoPath(user1.Name, repo1.Name))
assert.NoError(t, err)

err = pull.Merge(pr, user1, gitRepo, repo_model.MergeStyleMerge, "CONFLICT")
err = pull.Merge(pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "CONFLICT")
assert.Error(t, err, "Merge should return an error due to conflict")
assert.True(t, models.IsErrMergeConflicts(err), "Merge error is not a conflict error")

err = pull.Merge(pr, user1, gitRepo, repo_model.MergeStyleRebase, "CONFLICT")
err = pull.Merge(pr, user1, gitRepo, repo_model.MergeStyleRebase, "", "CONFLICT")
assert.Error(t, err, "Merge should return an error due to conflict")
assert.True(t, models.IsErrRebaseConflicts(err), "Merge error is not a conflict error")
gitRepo.Close()
Expand Down Expand Up @@ -329,7 +329,7 @@ func TestCantMergeUnrelated(t *testing.T) {
BaseBranch: "base",
}).(*models.PullRequest)

err = pull.Merge(pr, user1, gitRepo, repo_model.MergeStyleMerge, "UNRELATED")
err = pull.Merge(pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "UNRELATED")
assert.Error(t, err, "Merge should return an error due to unrelated")
assert.True(t, models.IsErrMergeUnrelatedHistories(err), "Merge error is not a unrelated histories error")
gitRepo.Close()
Expand Down
1 change: 1 addition & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -1506,6 +1506,7 @@ pulls.rebase_conflict_summary = Error Message
; </summary><code>%[2]s<br>%[3]s</code></details>
pulls.unrelated_histories = Merge Failed: The merge head and base do not share a common history. Hint: Try a different strategy
pulls.merge_out_of_date = Merge Failed: Whilst generating the merge, the base was updated. Hint: Try again.
pulls.head_out_of_date = Merge Failed: Whilst generating the merge, the head was updated. Hint: Try again.
pulls.push_rejected = Merge Failed: The push was rejected. Review the githooks for this repository.
pulls.push_rejected_summary = Full Rejection Message
pulls.push_rejected_no_message = Merge Failed: The push was rejected but there was no remote message.<br>Review the githooks for this repository
Expand Down
5 changes: 4 additions & 1 deletion routers/api/v1/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,7 @@ func MergePullRequest(ctx *context.APIContext) {
message += "\n\n" + form.MergeMessageField
}

if err := pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), message); err != nil {
if err := pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message); err != nil {
if models.IsErrInvalidMergeStyle(err) {
ctx.Error(http.StatusMethodNotAllowed, "Invalid merge style", fmt.Errorf("%s is not allowed an allowed merge style for this repository", repo_model.MergeStyle(form.Do)))
return
Expand All @@ -854,6 +854,9 @@ func MergePullRequest(ctx *context.APIContext) {
} else if git.IsErrPushOutOfDate(err) {
ctx.Error(http.StatusConflict, "Merge", "merge push out of date")
return
} else if models.IsErrSHADoesNotMatch(err) {
ctx.Error(http.StatusConflict, "Merge", "head out of date")
return
} else if git.IsErrPushRejected(err) {
errPushRej := err.(*git.ErrPushRejected)
if len(errPushRej.Message) == 0 {
Expand Down
7 changes: 6 additions & 1 deletion routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -933,7 +933,7 @@ func MergePullRequest(ctx *context.Context) {
return
}

if err = pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), message); err != nil {
if err = pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message); err != nil {
if models.IsErrInvalidMergeStyle(err) {
ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option"))
ctx.Redirect(issue.Link())
Expand Down Expand Up @@ -976,6 +976,11 @@ func MergePullRequest(ctx *context.Context) {
ctx.Flash.Error(ctx.Tr("repo.pulls.merge_out_of_date"))
ctx.Redirect(issue.Link())
return
} else if models.IsErrSHADoesNotMatch(err) {
log.Debug("MergeHeadOutOfDate error: %v", err)
ctx.Flash.Error(ctx.Tr("repo.pulls.head_out_of_date"))
ctx.Redirect(issue.Link())
return
} else if git.IsErrPushRejected(err) {
log.Debug("MergePushRejected error: %v", err)
pushrejErr := err.(*git.ErrPushRejected)
Expand Down
1 change: 1 addition & 0 deletions services/forms/repo_form.go
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,7 @@ type MergePullRequestForm struct {
MergeTitleField string
MergeMessageField string
MergeCommitID string // only used for manually-merged
HeadCommitID string `json:"head_commit_id,omitempty"`
ForceMerge *bool `json:"force_merge,omitempty"`
DeleteBranchAfterMerge bool `json:"delete_branch_after_merge,omitempty"`
}
Expand Down
20 changes: 17 additions & 3 deletions services/pull/merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import (
// Merge merges pull request to base repository.
// Caller should check PR is ready to be merged (review and status checks)
// FIXME: add repoWorkingPull make sure two merges does not happen at same time.
func Merge(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeStyle repo_model.MergeStyle, message string) (err error) {
func Merge(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) (err error) {
if err = pr.LoadHeadRepo(); err != nil {
log.Error("LoadHeadRepo: %v", err)
return fmt.Errorf("LoadHeadRepo: %v", err)
Expand All @@ -59,7 +59,7 @@ func Merge(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repos
go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "")
}()

pr.MergedCommitID, err = rawMerge(pr, doer, mergeStyle, message)
pr.MergedCommitID, err = rawMerge(pr, doer, mergeStyle, expectedHeadCommitID, message)
if err != nil {
return err
}
Expand Down Expand Up @@ -114,7 +114,7 @@ func Merge(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repos
}

// rawMerge perform the merge operation without changing any pull information in database
func rawMerge(pr *models.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, message string) (string, error) {
func rawMerge(pr *models.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) (string, error) {
err := git.LoadGitVersion()
if err != nil {
log.Error("git.LoadGitVersion: %v", err)
Expand All @@ -137,6 +137,20 @@ func rawMerge(pr *models.PullRequest, doer *user_model.User, mergeStyle repo_mod
trackingBranch := "tracking"
stagingBranch := "staging"

if expectedHeadCommitID != "" {
trackingCommitID, err := git.NewCommand("show-ref", "--hash", git.BranchPrefix+trackingBranch).RunInDir(tmpBasePath)
if err != nil {
log.Error("show-ref[%s] --hash refs/heads/trackingn: %v", tmpBasePath, git.BranchPrefix+trackingBranch, err)
return "", fmt.Errorf("getDiffTree: %v", err)
}
if strings.TrimSpace(trackingCommitID) != expectedHeadCommitID {
return "", models.ErrSHADoesNotMatch{
GivenSHA: expectedHeadCommitID,
CurrentSHA: trackingCommitID,
}
}
}

var outbuf, errbuf strings.Builder

// Enable sparse-checkout
Expand Down
2 changes: 1 addition & 1 deletion services/pull/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func Update(pull *models.PullRequest, doer *user_model.User, message string, reb
return fmt.Errorf("HeadBranch of PR %d is up to date", pull.Index)
}

_, err = rawMerge(pr, doer, style, message)
_, err = rawMerge(pr, doer, style, "", message)

defer func() {
if rebase {
Expand Down
5 changes: 5 additions & 0 deletions templates/repo/issue/view_content/pull.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@
<div class="ui form merge-fields" style="display: none">
<form action="{{.Link}}/merge" method="post">
{{.CsrfTokenHtml}}
<input type="hidden" name="head_commit_id" value="{{.PullHeadCommitID}}">
<div class="field">
<input type="text" name="merge_title_field" value="{{.Issue.PullRequest.GetDefaultMergeMessage}}">
</div>
Expand All @@ -352,6 +353,7 @@
<div class="ui form rebase-fields" style="display: none">
<form action="{{.Link}}/merge" method="post">
{{.CsrfTokenHtml}}
<input type="hidden" name="head_commit_id" value="{{.PullHeadCommitID}}">
<button class="ui green button" type="submit" name="do" value="rebase">
{{$.i18n.Tr "repo.pulls.rebase_merge_pull_request"}}
</button>
Expand All @@ -371,6 +373,7 @@
<div class="ui form rebase-merge-fields" style="display: none">
<form action="{{.Link}}/merge" method="post">
{{.CsrfTokenHtml}}
<input type="hidden" name="head_commit_id" value="{{.PullHeadCommitID}}">
<div class="field">
<input type="text" name="merge_title_field" value="{{.Issue.PullRequest.GetDefaultMergeMessage}}">
</div>
Expand All @@ -396,6 +399,7 @@
<div class="ui form squash-fields" style="display: none">
<form action="{{.Link}}/merge" method="post">
{{.CsrfTokenHtml}}
<input type="hidden" name="head_commit_id" value="{{.PullHeadCommitID}}">
<div class="field">
<input type="text" name="merge_title_field" value="{{.Issue.PullRequest.GetDefaultSquashMessage}}">
</div>
Expand All @@ -421,6 +425,7 @@
<div class="ui form manually-merged-fields" style="display: none">
<form action="{{.Link}}/merge" method="post">
{{.CsrfTokenHtml}}
<input type="hidden" name="head_commit_id" value="{{.PullHeadCommitID}}">
<div class="field">
<input type="text" name="merge_commit_id" placeholder="{{$.i18n.Tr "repo.pulls.merge_commit_id"}}">
</div>
Expand Down
4 changes: 4 additions & 0 deletions templates/swagger/v1_json.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -15674,6 +15674,10 @@
"force_merge": {
"type": "boolean",
"x-go-name": "ForceMerge"
},
"head_commit_id": {
"type": "string",
"x-go-name": "HeadCommitID"
}
},
"x-go-name": "MergePullRequestForm",
Expand Down