Skip to content

Commit 9c349a4

Browse files
65436543
authored
Move checks for pulls before merge into own function (#19271)
 This make checks in one single place so they dont differ and maintainer can not forget a check in one place while adding it to the other .... ( as it's atm ) Fix: * The API does ignore issue dependencies where Web does not * The API checks if "IsSignedIfRequired" where Web does not - UI probably do but nothing will some to craft custom requests * Default merge message is crafted a bit different between API and Web if not set on specific cases ...
1 parent f6145a6 commit 9c349a4

File tree

9 files changed

+217
-211
lines changed

9 files changed

+217
-211
lines changed

‎models/error.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -586,18 +586,18 @@ func (err ErrBranchesEqual) Error() string {
586586
return fmt.Sprintf("branches are equal [head: %sm base: %s]", err.HeadBranchName, err.BaseBranchName)
587587
}
588588

589-
// ErrNotAllowedToMerge represents an error that a branch is protected and the current user is not allowed to modify it.
590-
type ErrNotAllowedToMerge struct {
589+
// ErrDisallowedToMerge represents an error that a branch is protected and the current user is not allowed to modify it.
590+
type ErrDisallowedToMerge struct {
591591
Reason string
592592
}
593593

594-
// IsErrNotAllowedToMerge checks if an error is an ErrNotAllowedToMerge.
595-
func IsErrNotAllowedToMerge(err error) bool {
596-
_, ok := err.(ErrNotAllowedToMerge)
594+
// IsErrDisallowedToMerge checks if an error is an ErrDisallowedToMerge.
595+
func IsErrDisallowedToMerge(err error) bool {
596+
_, ok := err.(ErrDisallowedToMerge)
597597
return ok
598598
}
599599

600-
func (err ErrNotAllowedToMerge) Error() string {
600+
func (err ErrDisallowedToMerge) Error() string {
601601
return fmt.Sprintf("not allowed to merge [reason: %s]", err.Reason)
602602
}
603603

‎models/pull.go

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -222,22 +222,19 @@ func (pr *PullRequest) loadProtectedBranch(ctx context.Context) (err error) {
222222
}
223223

224224
// GetDefaultMergeMessage returns default message used when merging pull request
225-
func (pr *PullRequest) GetDefaultMergeMessage() string {
225+
func (pr *PullRequest) GetDefaultMergeMessage() (string, error) {
226226
if pr.HeadRepo == nil {
227227
var err error
228228
pr.HeadRepo, err = repo_model.GetRepositoryByID(pr.HeadRepoID)
229229
if err != nil {
230-
log.Error("GetRepositoryById[%d]: %v", pr.HeadRepoID, err)
231-
return ""
230+
return "", fmt.Errorf("GetRepositoryById[%d]: %v", pr.HeadRepoID, err)
232231
}
233232
}
234233
if err := pr.LoadIssue(); err != nil {
235-
log.Error("Cannot load issue %d for PR id %d: Error: %v", pr.IssueID, pr.ID, err)
236-
return ""
234+
return "", fmt.Errorf("Cannot load issue %d for PR id %d: Error: %v", pr.IssueID, pr.ID, err)
237235
}
238236
if err := pr.LoadBaseRepo(); err != nil {
239-
log.Error("LoadBaseRepo: %v", err)
240-
return ""
237+
return "", fmt.Errorf("LoadBaseRepo: %v", err)
241238
}
242239

243240
issueReference := "#"
@@ -246,10 +243,10 @@ func (pr *PullRequest) GetDefaultMergeMessage() string {
246243
}
247244

248245
if pr.BaseRepoID == pr.HeadRepoID {
249-
return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadBranch, pr.BaseBranch)
246+
return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadBranch, pr.BaseBranch), nil
250247
}
251248

252-
return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s:%s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseBranch)
249+
return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s:%s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseBranch), nil
253250
}
254251

255252
// ReviewCount represents a count of Reviews
@@ -335,19 +332,17 @@ func (pr *PullRequest) getReviewedByLines(writer io.Writer) error {
335332
}
336333

337334
// GetDefaultSquashMessage returns default message used when squash and merging pull request
338-
func (pr *PullRequest) GetDefaultSquashMessage() string {
335+
func (pr *PullRequest) GetDefaultSquashMessage() (string, error) {
339336
if err := pr.LoadIssue(); err != nil {
340-
log.Error("LoadIssue: %v", err)
341-
return ""
337+
return "", fmt.Errorf("LoadIssue: %v", err)
342338
}
343339
if err := pr.LoadBaseRepo(); err != nil {
344-
log.Error("LoadBaseRepo: %v", err)
345-
return ""
340+
return "", fmt.Errorf("LoadBaseRepo: %v", err)
346341
}
347342
if pr.BaseRepo.UnitEnabled(unit.TypeExternalTracker) {
348-
return fmt.Sprintf("%s (!%d)", pr.Issue.Title, pr.Issue.Index)
343+
return fmt.Sprintf("%s (!%d)", pr.Issue.Title, pr.Issue.Index), nil
349344
}
350-
return fmt.Sprintf("%s (#%d)", pr.Issue.Title, pr.Issue.Index)
345+
return fmt.Sprintf("%s (#%d)", pr.Issue.Title, pr.Issue.Index), nil
351346
}
352347

353348
// GetGitRefName returns git ref for hidden pull request branch

‎models/pull_test.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -261,11 +261,15 @@ func TestPullRequest_GetDefaultMergeMessage_InternalTracker(t *testing.T) {
261261
assert.NoError(t, unittest.PrepareTestDatabase())
262262
pr := unittest.AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest)
263263

264-
assert.Equal(t, "Merge pull request 'issue3' (#3) from branch2 into master", pr.GetDefaultMergeMessage())
264+
msg, err := pr.GetDefaultMergeMessage()
265+
assert.NoError(t, err)
266+
assert.Equal(t, "Merge pull request 'issue3' (#3) from branch2 into master", msg)
265267

266268
pr.BaseRepoID = 1
267269
pr.HeadRepoID = 2
268-
assert.Equal(t, "Merge pull request 'issue3' (#3) from user2/repo1:branch2 into master", pr.GetDefaultMergeMessage())
270+
msg, err = pr.GetDefaultMergeMessage()
271+
assert.NoError(t, err)
272+
assert.Equal(t, "Merge pull request 'issue3' (#3) from user2/repo1:branch2 into master", msg)
269273
}
270274

271275
func TestPullRequest_GetDefaultMergeMessage_ExternalTracker(t *testing.T) {
@@ -283,9 +287,13 @@ func TestPullRequest_GetDefaultMergeMessage_ExternalTracker(t *testing.T) {
283287

284288
pr := unittest.AssertExistsAndLoadBean(t, &PullRequest{ID: 2, BaseRepo: baseRepo}).(*PullRequest)
285289

286-
assert.Equal(t, "Merge pull request 'issue3' (!3) from branch2 into master", pr.GetDefaultMergeMessage())
290+
msg, err := pr.GetDefaultMergeMessage()
291+
assert.NoError(t, err)
292+
assert.Equal(t, "Merge pull request 'issue3' (!3) from branch2 into master", msg)
287293

288294
pr.BaseRepoID = 1
289295
pr.HeadRepoID = 2
290-
assert.Equal(t, "Merge pull request 'issue3' (!3) from user2/repo1:branch2 into master", pr.GetDefaultMergeMessage())
296+
msg, err = pr.GetDefaultMergeMessage()
297+
assert.NoError(t, err)
298+
assert.Equal(t, "Merge pull request 'issue3' (!3) from user2/repo1:branch2 into master", msg)
291299
}

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

Lines changed: 28 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -723,13 +723,12 @@ func MergePullRequest(ctx *context.APIContext) {
723723
return
724724
}
725725

726-
if err = pr.LoadHeadRepo(); err != nil {
726+
if err := pr.LoadHeadRepo(); err != nil {
727727
ctx.Error(http.StatusInternalServerError, "LoadHeadRepo", err)
728728
return
729729
}
730730

731-
err = pr.LoadIssue()
732-
if err != nil {
731+
if err := pr.LoadIssue(); err != nil {
733732
ctx.Error(http.StatusInternalServerError, "LoadIssue", err)
734733
return
735734
}
@@ -743,29 +742,33 @@ func MergePullRequest(ctx *context.APIContext) {
743742
}
744743
}
745744

746-
if pr.Issue.IsClosed {
747-
ctx.NotFound()
748-
return
749-
}
745+
manuallMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
746+
force := form.ForceMerge != nil && *form.ForceMerge
750747

751-
allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, ctx.Repo.Permission, ctx.Doer)
752-
if err != nil {
753-
ctx.Error(http.StatusInternalServerError, "IsUSerAllowedToMerge", err)
754-
return
755-
}
756-
if !allowedMerge {
757-
ctx.Error(http.StatusMethodNotAllowed, "Merge", "User not allowed to merge PR")
758-
return
759-
}
760-
761-
if pr.HasMerged {
762-
ctx.Error(http.StatusMethodNotAllowed, "PR already merged", "")
748+
if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, manuallMerge, force); err != nil {
749+
if errors.Is(err, pull_service.ErrIsClosed) {
750+
ctx.NotFound()
751+
} else if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) {
752+
ctx.Error(http.StatusMethodNotAllowed, "Merge", "User not allowed to merge PR")
753+
} else if errors.Is(err, pull_service.ErrHasMerged) {
754+
ctx.Error(http.StatusMethodNotAllowed, "PR already merged", "")
755+
} else if errors.Is(err, pull_service.ErrIsWorkInProgress) {
756+
ctx.Error(http.StatusMethodNotAllowed, "PR is a work in progress", "Work in progress PRs cannot be merged")
757+
} else if errors.Is(err, pull_service.ErrNotMergableState) {
758+
ctx.Error(http.StatusMethodNotAllowed, "PR not in mergeable state", "Please try again later")
759+
} else if models.IsErrDisallowedToMerge(err) {
760+
ctx.Error(http.StatusMethodNotAllowed, "PR is not ready to be merged", err)
761+
} else if asymkey_service.IsErrWontSign(err) {
762+
ctx.Error(http.StatusMethodNotAllowed, fmt.Sprintf("Protected branch %s requires signed commits but this merge would not be signed", pr.BaseBranch), err)
763+
} else {
764+
ctx.InternalServerError(err)
765+
}
763766
return
764767
}
765768

766769
// handle manually-merged mark
767-
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged {
768-
if err = pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
770+
if manuallMerge {
771+
if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
769772
if models.IsErrInvalidMergeStyle(err) {
770773
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)))
771774
return
@@ -781,63 +784,13 @@ func MergePullRequest(ctx *context.APIContext) {
781784
return
782785
}
783786

784-
if !pr.CanAutoMerge() {
785-
ctx.Error(http.StatusMethodNotAllowed, "PR not in mergeable state", "Please try again later")
787+
// set defaults to propagate needed fields
788+
if err := form.SetDefaults(pr); err != nil {
789+
ctx.ServerError("SetDefaults", fmt.Errorf("SetDefaults: %v", err))
786790
return
787791
}
788792

789-
if pr.IsWorkInProgress() {
790-
ctx.Error(http.StatusMethodNotAllowed, "PR is a work in progress", "Work in progress PRs cannot be merged")
791-
return
792-
}
793-
794-
if err := pull_service.CheckPRReadyToMerge(ctx, pr, false); err != nil {
795-
if !models.IsErrNotAllowedToMerge(err) {
796-
ctx.Error(http.StatusInternalServerError, "CheckPRReadyToMerge", err)
797-
return
798-
}
799-
if form.ForceMerge != nil && *form.ForceMerge {
800-
if isRepoAdmin, err := models.IsUserRepoAdmin(pr.BaseRepo, ctx.Doer); err != nil {
801-
ctx.Error(http.StatusInternalServerError, "IsUserRepoAdmin", err)
802-
return
803-
} else if !isRepoAdmin {
804-
ctx.Error(http.StatusMethodNotAllowed, "Merge", "Only repository admin can merge if not all checks are ok (force merge)")
805-
}
806-
} else {
807-
ctx.Error(http.StatusMethodNotAllowed, "PR is not ready to be merged", err)
808-
return
809-
}
810-
}
811-
812-
if _, err := pull_service.IsSignedIfRequired(ctx, pr, ctx.Doer); err != nil {
813-
if !asymkey_service.IsErrWontSign(err) {
814-
ctx.Error(http.StatusInternalServerError, "IsSignedIfRequired", err)
815-
return
816-
}
817-
ctx.Error(http.StatusMethodNotAllowed, fmt.Sprintf("Protected branch %s requires signed commits but this merge would not be signed", pr.BaseBranch), err)
818-
return
819-
}
820-
821-
if len(form.Do) == 0 {
822-
form.Do = string(repo_model.MergeStyleMerge)
823-
}
824-
825-
message := strings.TrimSpace(form.MergeTitleField)
826-
if len(message) == 0 {
827-
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleMerge {
828-
message = pr.GetDefaultMergeMessage()
829-
}
830-
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleSquash {
831-
message = pr.GetDefaultSquashMessage()
832-
}
833-
}
834-
835-
form.MergeMessageField = strings.TrimSpace(form.MergeMessageField)
836-
if len(form.MergeMessageField) > 0 {
837-
message += "\n\n" + form.MergeMessageField
838-
}
839-
840-
if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message); err != nil {
793+
if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, form.MergeTitleField); err != nil {
841794
if models.IsErrInvalidMergeStyle(err) {
842795
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)))
843796
return

‎routers/private/hook_pre_receive.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID, refFullN
340340

341341
// Check all status checks and reviews are ok
342342
if err := pull_service.CheckPRReadyToMerge(ctx, pr, true); err != nil {
343-
if models.IsErrNotAllowedToMerge(err) {
343+
if models.IsErrDisallowedToMerge(err) {
344344
log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", ctx.opts.UserID, branchName, repo, pr.Index, err.Error())
345345
ctx.JSON(http.StatusForbidden, private.Response{
346346
Err: fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, ctx.opts.PullRequestID, err.Error()),

0 commit comments

Comments
 (0)