Skip to content

Move checks for pulls before merge into own function #19271

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Mar 31, 2022
Merged

Move checks for pulls before merge into own function #19271

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
d8e0686
move checks for branchprotection into own package
6543 Mar 30, 2022
6f15ec8
resolve todo now ...
6543 Mar 30, 2022
f17abd1
move & rename
6543 Mar 30, 2022
2a66e96
unexport if posible
6543 Mar 30, 2022
a1073bc
Merge branch 'main' into single-place-to-enforce-protected-branch-rules
6543 Mar 31, 2022
52cb7c4
wording
6543 Mar 31, 2022
f4b4a0b
Merge branch 'main' into single-place-to-enforce-protected-branch-rules
6543 Mar 31, 2022
c0e932a
improve SetDefaults & nits
6543 Mar 31, 2022
6959e27
NotAllowedToMerge -> DisallowedToMerge
6543 Mar 31, 2022
4775c3a
fix test
6543 Mar 31, 2022
8c6a54f
Merge branch 'main' into single-place-to-enforce-protected-branch-rules
6543 Mar 31, 2022
943d1f2
merge files
6543 Mar 31, 2022
f7b89a5
Merge branch 'master' into single-place-to-enforce-protected-branch-r…
6543 Mar 31, 2022
120d84d
use package "errors"
6543 Mar 31, 2022
d0a2312
merge files
6543 Mar 31, 2022
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
104 changes: 27 additions & 77 deletions routers/api/v1/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -723,13 +723,12 @@ func MergePullRequest(ctx *context.APIContext) {
return
}

if err = pr.LoadHeadRepo(); err != nil {
if err := pr.LoadHeadRepo(); err != nil {
ctx.Error(http.StatusInternalServerError, "LoadHeadRepo", err)
return
}

err = pr.LoadIssue()
if err != nil {
if err := pr.LoadIssue(); err != nil {
ctx.Error(http.StatusInternalServerError, "LoadIssue", err)
return
}
Expand All @@ -743,29 +742,33 @@ func MergePullRequest(ctx *context.APIContext) {
}
}

if pr.Issue.IsClosed {
ctx.NotFound()
return
}

allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, ctx.Repo.Permission, ctx.Doer)
if err != nil {
ctx.Error(http.StatusInternalServerError, "IsUSerAllowedToMerge", err)
return
}
if !allowedMerge {
ctx.Error(http.StatusMethodNotAllowed, "Merge", "User not allowed to merge PR")
return
}
manuallMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
force := form.ForceMerge != nil && *form.ForceMerge

if pr.HasMerged {
ctx.Error(http.StatusMethodNotAllowed, "PR already merged", "")
if err := pull_service.CheckPullProtection(ctx, ctx.Doer, &ctx.Repo.Permission, pr, manuallMerge, force); err != nil {
if pull_service.IsErrIsClosed(err) {
ctx.NotFound()
} else if pull_service.IsErrUserNotAllowedToMerge(err) {
ctx.Error(http.StatusMethodNotAllowed, "Merge", "User not allowed to merge PR")
} else if pull_service.IsErrHasMerged(err) {
ctx.Error(http.StatusMethodNotAllowed, "PR already merged", "")
} else if pull_service.IsErrIsWorkInProgress(err) {
ctx.Error(http.StatusMethodNotAllowed, "PR is a work in progress", "Work in progress PRs cannot be merged")
} else if pull_service.IsErrNotMergableState(err) {
ctx.Error(http.StatusMethodNotAllowed, "PR not in mergeable state", "Please try again later")
} else if models.IsErrNotAllowedToMerge(err) {
ctx.Error(http.StatusMethodNotAllowed, "PR is not ready to be merged", err)
} else if asymkey_service.IsErrWontSign(err) {
ctx.Error(http.StatusMethodNotAllowed, fmt.Sprintf("Protected branch %s requires signed commits but this merge would not be signed", pr.BaseBranch), err)
} else {
ctx.InternalServerError(err)
}
return
}

// handle manually-merged mark
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged {
if err = pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
if manuallMerge {
if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); 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 @@ -781,63 +784,10 @@ func MergePullRequest(ctx *context.APIContext) {
return
}

if !pr.CanAutoMerge() {
ctx.Error(http.StatusMethodNotAllowed, "PR not in mergeable state", "Please try again later")
return
}

if pr.IsWorkInProgress() {
ctx.Error(http.StatusMethodNotAllowed, "PR is a work in progress", "Work in progress PRs cannot be merged")
return
}

if err := pull_service.CheckPRReadyToMerge(ctx, pr, false); err != nil {
if !models.IsErrNotAllowedToMerge(err) {
ctx.Error(http.StatusInternalServerError, "CheckPRReadyToMerge", err)
return
}
if form.ForceMerge != nil && *form.ForceMerge {
if isRepoAdmin, err := models.IsUserRepoAdmin(pr.BaseRepo, ctx.Doer); err != nil {
ctx.Error(http.StatusInternalServerError, "IsUserRepoAdmin", err)
return
} else if !isRepoAdmin {
ctx.Error(http.StatusMethodNotAllowed, "Merge", "Only repository admin can merge if not all checks are ok (force merge)")
}
} else {
ctx.Error(http.StatusMethodNotAllowed, "PR is not ready to be merged", err)
return
}
}

if _, err := pull_service.IsSignedIfRequired(ctx, pr, ctx.Doer); err != nil {
if !asymkey_service.IsErrWontSign(err) {
ctx.Error(http.StatusInternalServerError, "IsSignedIfRequired", err)
return
}
ctx.Error(http.StatusMethodNotAllowed, fmt.Sprintf("Protected branch %s requires signed commits but this merge would not be signed", pr.BaseBranch), err)
return
}

if len(form.Do) == 0 {
form.Do = string(repo_model.MergeStyleMerge)
}

message := strings.TrimSpace(form.MergeTitleField)
if len(message) == 0 {
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleMerge {
message = pr.GetDefaultMergeMessage()
}
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleSquash {
message = pr.GetDefaultSquashMessage()
}
}

form.MergeMessageField = strings.TrimSpace(form.MergeMessageField)
if len(form.MergeMessageField) > 0 {
message += "\n\n" + form.MergeMessageField
}
// set defaults to propagate needed fields
form.SetDefaults(pr)

if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message); err != nil {
if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, form.MergeTitleField); 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 Down
133 changes: 43 additions & 90 deletions routers/web/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/modules/web/middleware"
"code.gitea.io/gitea/routers/utils"
asymkey_service "code.gitea.io/gitea/services/asymkey"
"code.gitea.io/gitea/services/forms"
"code.gitea.io/gitea/services/gitdiff"
pull_service "code.gitea.io/gitea/services/pull"
Expand Down Expand Up @@ -858,39 +859,53 @@ func MergePullRequest(ctx *context.Context) {
if ctx.Written() {
return
}
if issue.IsClosed {
if issue.IsPull {
ctx.Flash.Error(ctx.Tr("repo.pulls.is_closed"))
ctx.Redirect(issue.Link())
return
}
ctx.Flash.Error(ctx.Tr("repo.issues.closed_title"))
ctx.Redirect(issue.Link())
return
}

pr := issue.PullRequest
pr.Issue = issue
pr.Issue.Repo = ctx.Repo.Repository
manuallMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
forceMerge := form.ForceMerge != nil && *form.ForceMerge

allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, ctx.Repo.Permission, ctx.Doer)
if err != nil {
ctx.ServerError("IsUserAllowedToMerge", err)
return
}
if !allowedMerge {
ctx.Flash.Error(ctx.Tr("repo.pulls.update_not_allowed"))
ctx.Redirect(issue.Link())
return
}
if err := pull_service.CheckPullProtection(ctx, ctx.Doer, &ctx.Repo.Permission, pr, manuallMerge, forceMerge); err != nil {
if pull_service.IsErrIsClosed(err) {
if issue.IsPull {
ctx.Flash.Error(ctx.Tr("repo.pulls.is_closed"))
ctx.Redirect(issue.Link())
} else {
ctx.Flash.Error(ctx.Tr("repo.issues.closed_title"))
ctx.Redirect(issue.Link())
}
} else if pull_service.IsErrUserNotAllowedToMerge(err) {
ctx.Flash.Error(ctx.Tr("repo.pulls.update_not_allowed"))
ctx.Redirect(issue.Link())
} else if pull_service.IsErrHasMerged(err) {
ctx.Flash.Error(ctx.Tr("repo.pulls.has_merged"))
ctx.Redirect(issue.Link())
} else if pull_service.IsErrIsWorkInProgress(err) {
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_wip"))
ctx.Redirect(issue.Link())
} else if pull_service.IsErrNotMergableState(err) {
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready"))
ctx.Redirect(issue.Link())
} else if models.IsErrNotAllowedToMerge(err) {
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready"))
ctx.Redirect(issue.Link())
} else if asymkey_service.IsErrWontSign(err) {
ctx.Flash.Error(err.Error()) // has not translation ...
ctx.Redirect(issue.Link())
} else if pull_service.IsErrDependenciesLeft(err) {
ctx.Flash.Error(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
ctx.Redirect(issue.Link())
} else {
ctx.ServerError("WebCheck", err)
}

if pr.HasMerged {
ctx.Flash.Error(ctx.Tr("repo.pulls.has_merged"))
ctx.Redirect(issue.Link())
return
}

// handle manually-merged mark
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged {
if err = pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
if manuallMerge {
if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
if models.IsErrInvalidMergeStyle(err) {
ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option"))
ctx.Redirect(issue.Link())
Expand All @@ -909,72 +924,10 @@ func MergePullRequest(ctx *context.Context) {
return
}

if !pr.CanAutoMerge() {
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready"))
ctx.Redirect(issue.Link())
return
}

if pr.IsWorkInProgress() {
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_wip"))
ctx.Redirect(issue.Link())
return
}

if err := pull_service.CheckPRReadyToMerge(ctx, pr, false); err != nil {
if !models.IsErrNotAllowedToMerge(err) {
ctx.ServerError("Merge PR status", err)
return
}
if isRepoAdmin, err := models.IsUserRepoAdmin(pr.BaseRepo, ctx.Doer); err != nil {
ctx.ServerError("IsUserRepoAdmin", err)
return
} else if !isRepoAdmin {
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready"))
ctx.Redirect(issue.Link())
return
}
}

if ctx.HasError() {
ctx.Flash.Error(ctx.Data["ErrorMsg"].(string))
ctx.Redirect(issue.Link())
return
}

message := strings.TrimSpace(form.MergeTitleField)
if len(message) == 0 {
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleMerge {
message = pr.GetDefaultMergeMessage()
}
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleRebaseMerge {
message = pr.GetDefaultMergeMessage()
}
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleSquash {
message = pr.GetDefaultSquashMessage()
}
}

form.MergeMessageField = strings.TrimSpace(form.MergeMessageField)
if len(form.MergeMessageField) > 0 {
message += "\n\n" + form.MergeMessageField
}

pr.Issue = issue
pr.Issue.Repo = ctx.Repo.Repository

noDeps, err := models.IssueNoDependenciesLeft(issue)
if err != nil {
return
}

if !noDeps {
ctx.Flash.Error(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
ctx.Redirect(issue.Link())
return
}
// set defaults to propagate needed fields
form.SetDefaults(pr)

if err = pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message); err != nil {
if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, form.MergeTitleField); err != nil {
if models.IsErrInvalidMergeStyle(err) {
ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option"))
ctx.Redirect(issue.Link())
Expand Down
26 changes: 26 additions & 0 deletions services/forms/repo_form.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,32 @@ func (f *MergePullRequestForm) Validate(req *http.Request, errs binding.Errors)
return middleware.Validate(errs, ctx.Data, f, ctx.Locale)
}

// SetDefaults for options who n
func (f *MergePullRequestForm) SetDefaults(pr *models.PullRequest) {
if len(f.Do) == 0 {
f.Do = "merge"
}

f.MergeTitleField = strings.TrimSpace(f.MergeTitleField)
if len(f.MergeTitleField) == 0 {
switch f.Do {
case "merge":
f.MergeTitleField = pr.GetDefaultMergeMessage()
case "rebase-merge":
f.MergeTitleField = pr.GetDefaultMergeMessage()
case "squash":
f.MergeTitleField = pr.GetDefaultSquashMessage()

}
}

f.MergeMessageField = strings.TrimSpace(f.MergeMessageField)
if len(f.MergeMessageField) > 0 {
f.MergeTitleField += "\n\n" + f.MergeMessageField
f.MergeMessageField = ""
}
}

// CodeCommentForm form for adding code comments for PRs
type CodeCommentForm struct {
Origin string `binding:"Required;In(timeline,diff)"`
Expand Down
15 changes: 0 additions & 15 deletions services/pull/merge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,21 +660,6 @@ func getDiffTree(ctx context.Context, repoPath, baseBranch, headBranch string) (
return out.String(), nil
}

// IsSignedIfRequired check if merge will be signed if required
func IsSignedIfRequired(ctx context.Context, pr *models.PullRequest, doer *user_model.User) (bool, error) {
if err := pr.LoadProtectedBranch(); err != nil {
return false, err
}

if pr.ProtectedBranch == nil || !pr.ProtectedBranch.RequireSignedCommits {
return true, nil
}

sign, _, _, err := asymkey_service.SignMerge(ctx, pr, doer, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitRefName())

return sign, err
}

// IsUserAllowedToMerge check if user is allowed to merge PR with given permissions and branch protections
func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *user_model.User) (bool, error) {
if user == nil {
Expand Down
Loading