Skip to content

Commit e4e4118

Browse files
zeripathzeripath
authored
Abort merge if head has been updated before pressing merge (#18032)
* Abort merge if head has been updated before pressing merge It is possible that a PR head may be pushed to between the merge page being shown and the merge button being pressed. Pass the current expected head in as a parameter and cancel the merge if it has changed. Fix #18028 Signed-off-by: Andrew Thornton <[email protected]> * adjust swagger Signed-off-by: Andrew Thornton <[email protected]> * fix test Signed-off-by: Andrew Thornton <[email protected]> * placate lint Signed-off-by: Andrew Thornton <[email protected]>
1 parent b24a965 commit e4e4118

File tree

9 files changed

+42
-9
lines changed

9 files changed

+42
-9
lines changed

‎integrations/pull_merge_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -241,11 +241,11 @@ func TestCantMergeConflict(t *testing.T) {
241241
gitRepo, err := git.OpenRepository(repo_model.RepoPath(user1.Name, repo1.Name))
242242
assert.NoError(t, err)
243243

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

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

332-
err = pull.Merge(pr, user1, gitRepo, repo_model.MergeStyleMerge, "UNRELATED")
332+
err = pull.Merge(pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "UNRELATED")
333333
assert.Error(t, err, "Merge should return an error due to unrelated")
334334
assert.True(t, models.IsErrMergeUnrelatedHistories(err), "Merge error is not a unrelated histories error")
335335
gitRepo.Close()

‎options/locale/locale_en-US.ini

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1506,6 +1506,7 @@ pulls.rebase_conflict_summary = Error Message
15061506
; </summary><code>%[2]s<br>%[3]s</code></details>
15071507
pulls.unrelated_histories = Merge Failed: The merge head and base do not share a common history. Hint: Try a different strategy
15081508
pulls.merge_out_of_date = Merge Failed: Whilst generating the merge, the base was updated. Hint: Try again.
1509+
pulls.head_out_of_date = Merge Failed: Whilst generating the merge, the head was updated. Hint: Try again.
15091510
pulls.push_rejected = Merge Failed: The push was rejected. Review the githooks for this repository.
15101511
pulls.push_rejected_summary = Full Rejection Message
15111512
pulls.push_rejected_no_message = Merge Failed: The push was rejected but there was no remote message.<br>Review the githooks for this repository

‎routers/api/v1/repo/pull.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -838,7 +838,7 @@ func MergePullRequest(ctx *context.APIContext) {
838838
message += "\n\n" + form.MergeMessageField
839839
}
840840

841-
if err := pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), message); err != nil {
841+
if err := pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message); err != nil {
842842
if models.IsErrInvalidMergeStyle(err) {
843843
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)))
844844
return
@@ -854,6 +854,9 @@ func MergePullRequest(ctx *context.APIContext) {
854854
} else if git.IsErrPushOutOfDate(err) {
855855
ctx.Error(http.StatusConflict, "Merge", "merge push out of date")
856856
return
857+
} else if models.IsErrSHADoesNotMatch(err) {
858+
ctx.Error(http.StatusConflict, "Merge", "head out of date")
859+
return
857860
} else if git.IsErrPushRejected(err) {
858861
errPushRej := err.(*git.ErrPushRejected)
859862
if len(errPushRej.Message) == 0 {

‎routers/web/repo/pull.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -933,7 +933,7 @@ func MergePullRequest(ctx *context.Context) {
933933
return
934934
}
935935

936-
if err = pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), message); err != nil {
936+
if err = pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message); err != nil {
937937
if models.IsErrInvalidMergeStyle(err) {
938938
ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option"))
939939
ctx.Redirect(issue.Link())
@@ -976,6 +976,11 @@ func MergePullRequest(ctx *context.Context) {
976976
ctx.Flash.Error(ctx.Tr("repo.pulls.merge_out_of_date"))
977977
ctx.Redirect(issue.Link())
978978
return
979+
} else if models.IsErrSHADoesNotMatch(err) {
980+
log.Debug("MergeHeadOutOfDate error: %v", err)
981+
ctx.Flash.Error(ctx.Tr("repo.pulls.head_out_of_date"))
982+
ctx.Redirect(issue.Link())
983+
return
979984
} else if git.IsErrPushRejected(err) {
980985
log.Debug("MergePushRejected error: %v", err)
981986
pushrejErr := err.(*git.ErrPushRejected)

‎services/forms/repo_form.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -571,6 +571,7 @@ type MergePullRequestForm struct {
571571
MergeTitleField string
572572
MergeMessageField string
573573
MergeCommitID string // only used for manually-merged
574+
HeadCommitID string `json:"head_commit_id,omitempty"`
574575
ForceMerge *bool `json:"force_merge,omitempty"`
575576
DeleteBranchAfterMerge bool `json:"delete_branch_after_merge,omitempty"`
576577
}

‎services/pull/merge.go

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ import (
3434
// Merge merges pull request to base repository.
3535
// Caller should check PR is ready to be merged (review and status checks)
3636
// FIXME: add repoWorkingPull make sure two merges does not happen at same time.
37-
func Merge(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeStyle repo_model.MergeStyle, message string) (err error) {
37+
func Merge(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) (err error) {
3838
if err = pr.LoadHeadRepo(); err != nil {
3939
log.Error("LoadHeadRepo: %v", err)
4040
return fmt.Errorf("LoadHeadRepo: %v", err)
@@ -59,7 +59,7 @@ func Merge(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repos
5959
go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "")
6060
}()
6161

62-
pr.MergedCommitID, err = rawMerge(pr, doer, mergeStyle, message)
62+
pr.MergedCommitID, err = rawMerge(pr, doer, mergeStyle, expectedHeadCommitID, message)
6363
if err != nil {
6464
return err
6565
}
@@ -114,7 +114,7 @@ func Merge(pr *models.PullRequest, doer *user_model.User, baseGitRepo *git.Repos
114114
}
115115

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

140+
if expectedHeadCommitID != "" {
141+
trackingCommitID, err := git.NewCommand("show-ref", "--hash", git.BranchPrefix+trackingBranch).RunInDir(tmpBasePath)
142+
if err != nil {
143+
log.Error("show-ref[%s] --hash refs/heads/trackingn: %v", tmpBasePath, git.BranchPrefix+trackingBranch, err)
144+
return "", fmt.Errorf("getDiffTree: %v", err)
145+
}
146+
if strings.TrimSpace(trackingCommitID) != expectedHeadCommitID {
147+
return "", models.ErrSHADoesNotMatch{
148+
GivenSHA: expectedHeadCommitID,
149+
CurrentSHA: trackingCommitID,
150+
}
151+
}
152+
}
153+
140154
var outbuf, errbuf strings.Builder
141155

142156
// Enable sparse-checkout

‎services/pull/update.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ func Update(pull *models.PullRequest, doer *user_model.User, message string, reb
5555
return fmt.Errorf("HeadBranch of PR %d is up to date", pull.Index)
5656
}
5757

58-
_, err = rawMerge(pr, doer, style, message)
58+
_, err = rawMerge(pr, doer, style, "", message)
5959

6060
defer func() {
6161
if rebase {

‎templates/repo/issue/view_content/pull.tmpl

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,7 @@
327327
<div class="ui form merge-fields" style="display: none">
328328
<form action="{{.Link}}/merge" method="post">
329329
{{.CsrfTokenHtml}}
330+
<input type="hidden" name="head_commit_id" value="{{.PullHeadCommitID}}">
330331
<div class="field">
331332
<input type="text" name="merge_title_field" value="{{.Issue.PullRequest.GetDefaultMergeMessage}}">
332333
</div>
@@ -352,6 +353,7 @@
352353
<div class="ui form rebase-fields" style="display: none">
353354
<form action="{{.Link}}/merge" method="post">
354355
{{.CsrfTokenHtml}}
356+
<input type="hidden" name="head_commit_id" value="{{.PullHeadCommitID}}">
355357
<button class="ui green button" type="submit" name="do" value="rebase">
356358
{{$.i18n.Tr "repo.pulls.rebase_merge_pull_request"}}
357359
</button>
@@ -371,6 +373,7 @@
371373
<div class="ui form rebase-merge-fields" style="display: none">
372374
<form action="{{.Link}}/merge" method="post">
373375
{{.CsrfTokenHtml}}
376+
<input type="hidden" name="head_commit_id" value="{{.PullHeadCommitID}}">
374377
<div class="field">
375378
<input type="text" name="merge_title_field" value="{{.Issue.PullRequest.GetDefaultMergeMessage}}">
376379
</div>
@@ -396,6 +399,7 @@
396399
<div class="ui form squash-fields" style="display: none">
397400
<form action="{{.Link}}/merge" method="post">
398401
{{.CsrfTokenHtml}}
402+
<input type="hidden" name="head_commit_id" value="{{.PullHeadCommitID}}">
399403
<div class="field">
400404
<input type="text" name="merge_title_field" value="{{.Issue.PullRequest.GetDefaultSquashMessage}}">
401405
</div>
@@ -421,6 +425,7 @@
421425
<div class="ui form manually-merged-fields" style="display: none">
422426
<form action="{{.Link}}/merge" method="post">
423427
{{.CsrfTokenHtml}}
428+
<input type="hidden" name="head_commit_id" value="{{.PullHeadCommitID}}">
424429
<div class="field">
425430
<input type="text" name="merge_commit_id" placeholder="{{$.i18n.Tr "repo.pulls.merge_commit_id"}}">
426431
</div>

‎templates/swagger/v1_json.tmpl

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15674,6 +15674,10 @@
1567415674
"force_merge": {
1567515675
"type": "boolean",
1567615676
"x-go-name": "ForceMerge"
15677+
},
15678+
"head_commit_id": {
15679+
"type": "string",
15680+
"x-go-name": "HeadCommitID"
1567715681
}
1567815682
},
1567915683
"x-go-name": "MergePullRequestForm",

0 commit comments

Comments
 (0)