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

Merging pull request causes 500 error. #7611

Closed
2 of 7 tasks
PershingSquare opened this issue Jul 25, 2019 · 3 comments · Fixed by #7615
Closed
2 of 7 tasks

Merging pull request causes 500 error. #7611

PershingSquare opened this issue Jul 25, 2019 · 3 comments · Fixed by #7615
Labels
Milestone

Comments

@PershingSquare
Copy link

PershingSquare commented Jul 25, 2019

Description

Every time I try to merge a pull request, I get a 500 server error. I have branch protection on master, with 1 approval required before merging.

Screenshots

what_the_page_looks_like

@zeripath
Copy link
Contributor

OK so looking at the gitea.log you've sent it appears that the panic is being caused at line 104 here:

gitea/models/branches.go

Lines 102 to 108 in 734aa96

// GetGrantedApprovalsCount returns the number of granted approvals for pr. A granted approval must be authored by a user in an approval whitelist.
func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest) int64 {
reviews, err := GetReviewersByPullID(pr.Issue.ID)
if err != nil {
log.Error("GetReviewersByPullID: %v", err)
return 0
}

This is in turn called from:

gitea/models/branches.go

Lines 94 to 100 in 734aa96

// HasEnoughApprovals returns true if pr has enough granted approvals.
func (protectBranch *ProtectedBranch) HasEnoughApprovals(pr *PullRequest) bool {
if protectBranch.RequiredApprovals == 0 {
return true
}
return protectBranch.GetGrantedApprovalsCount(pr) >= protectBranch.RequiredApprovals
}

Which is from line 99 here:

canPush := protectBranch.CanUserPush(userID)
if !canPush && prID > 0 {
pr, err := models.GetPullRequestByID(prID)
if err != nil {
log.Error("Unable to get PullRequest %d Error: %v", prID, err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": fmt.Sprintf("Unable to get PullRequest %d Error: %v", prID, err),
})
return
}
if !protectBranch.HasEnoughApprovals(pr) {
log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v and pr #%d does not have enough approvals", userID, branchName, repo, pr.Index)
ctx.JSON(http.StatusForbidden, map[string]interface{}{
"err": fmt.Sprintf("protected branch %s can not be pushed to and pr #%d does not have enough approvals", branchName, prID),
})
return
}

@zeripath
Copy link
Contributor

Now that means that either pr is nil here, or pr.Issue is nil.

@zeripath
Copy link
Contributor

OK, simple fix is to change pr.Issue.ID to pr.IssueID. However, when merging it shouldn't be pushing all of the branches so I'll look at that too.

zeripath added a commit to zeripath/gitea that referenced this issue Jul 25, 2019
Use pr.IssueID instead of pr.Issue.ID as Issue may not be loaded and is unnecessary
@lunny lunny added the type/bug label Jul 25, 2019
zeripath added a commit that referenced this issue Jul 25, 2019
* Fix panic in #7611

Use pr.IssueID instead of pr.Issue.ID as Issue may not be loaded and is unnecessary

* Only fetch the head branch from the remote
zeripath added a commit to zeripath/gitea that referenced this issue Jul 25, 2019
* Fix panic in go-gitea#7611

Use pr.IssueID instead of pr.Issue.ID as Issue may not be loaded and is unnecessary

* Only fetch the head branch from the remote
zeripath added a commit that referenced this issue Jul 25, 2019
* Fix panic in #7611

Use pr.IssueID instead of pr.Issue.ID as Issue may not be loaded and is unnecessary

* Only fetch the head branch from the remote
@lafriks lafriks added this to the 1.9.0 milestone Jul 25, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants