From e4e411821d985cf8f9007d9640909ab9ee271dd7 Mon Sep 17 00:00:00 2001 From: zeripath Date: Mon, 20 Dec 2021 00:32:54 +0000 Subject: [PATCH] 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 * adjust swagger Signed-off-by: Andrew Thornton * fix test Signed-off-by: Andrew Thornton * placate lint Signed-off-by: Andrew Thornton --- integrations/pull_merge_test.go | 6 +++--- options/locale/locale_en-US.ini | 1 + routers/api/v1/repo/pull.go | 5 ++++- routers/web/repo/pull.go | 7 ++++++- services/forms/repo_form.go | 1 + services/pull/merge.go | 20 +++++++++++++++++--- services/pull/update.go | 2 +- templates/repo/issue/view_content/pull.tmpl | 5 +++++ templates/swagger/v1_json.tmpl | 4 ++++ 9 files changed, 42 insertions(+), 9 deletions(-) diff --git a/integrations/pull_merge_test.go b/integrations/pull_merge_test.go index 812b5dd171..a0b87eeee8 100644 --- a/integrations/pull_merge_test.go +++ b/integrations/pull_merge_test.go @@ -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() @@ -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() diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 1d316bf90c..3b0092a5ef 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1506,6 +1506,7 @@ pulls.rebase_conflict_summary = Error Message ; %[2]s
%[3]s 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.
Review the githooks for this repository diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 4c774e8af7..2b7280d39b 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -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 @@ -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 { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 9445c1a48b..bf80a7e5df 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -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()) @@ -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) diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index c571951bb9..19b5a37664 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -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"` } diff --git a/services/pull/merge.go b/services/pull/merge.go index ab1f43a50a..e541495bef 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -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) @@ -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 } @@ -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) @@ -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 diff --git a/services/pull/update.go b/services/pull/update.go index 25c6d36308..8ca7e4cee7 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -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 { diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 5a682ea06b..bb80bba7c8 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -327,6 +327,7 @@