Mark PR reviews as stale at push and allow to dismiss stale approvals (#9532)

Fix #5997.

If a push causes the patch/diff of a PR towards target branch to change, all existing reviews for the PR will be set and shown as stale.
New branch protection option to dismiss stale approvals are added.
To show that a review is not based on the latest PR changes, an hourglass is shown
This commit is contained in:
David Svantesson 2020-01-09 02:47:45 +01:00 committed by zeripath
parent 5b2d9333f1
commit 25531c71a7
18 changed files with 244 additions and 43 deletions

View File

@ -45,6 +45,8 @@ type ProtectedBranch struct {
ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"`
RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"`
BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"`
DismissStaleApprovals bool `xorm:"NOT NULL DEFAULT false"`
CreatedUnix timeutil.TimeStamp `xorm:"created"`
UpdatedUnix timeutil.TimeStamp `xorm:"updated"`
}
@ -155,10 +157,13 @@ func (protectBranch *ProtectedBranch) HasEnoughApprovals(pr *PullRequest) bool {
// 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 {
approvals, err := x.Where("issue_id = ?", pr.IssueID).
sess := x.Where("issue_id = ?", pr.IssueID).
And("type = ?", ReviewTypeApprove).
And("official = ?", true).
Count(new(Review))
And("official = ?", true)
if protectBranch.DismissStaleApprovals {
sess = sess.And("stale = ?", false)
}
approvals, err := sess.Count(new(Review))
if err != nil {
log.Error("GetGrantedApprovalsCount: %v", err)
return 0

View File

@ -290,6 +290,8 @@ var migrations = []Migration{
NewMigration("Extend TrackedTimes", extendTrackedTimes),
// v117 -> v118
NewMigration("Add block on rejected reviews branch protection", addBlockOnRejectedReviews),
// v118 -> v119
NewMigration("Add commit id and stale to reviews", addReviewCommitAndStale),
}
// Migrate database to current version

26
models/migrations/v118.go Normal file
View File

@ -0,0 +1,26 @@
// Copyright 2019 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.
package migrations
import (
"xorm.io/xorm"
)
func addReviewCommitAndStale(x *xorm.Engine) error {
type Review struct {
CommitID string `xorm:"VARCHAR(40)"`
Stale bool `xorm:"NOT NULL DEFAULT false"`
}
type ProtectedBranch struct {
DismissStaleApprovals bool `xorm:"NOT NULL DEFAULT false"`
}
// Old reviews will have commit ID set to "" and not stale
if err := x.Sync2(new(Review)); err != nil {
return err
}
return x.Sync2(new(ProtectedBranch))
}

View File

@ -54,6 +54,8 @@ type Review struct {
Content string `xorm:"TEXT"`
// Official is a review made by an assigned approver (counts towards approval)
Official bool `xorm:"NOT NULL DEFAULT false"`
CommitID string `xorm:"VARCHAR(40)"`
Stale bool `xorm:"NOT NULL DEFAULT false"`
CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
@ -169,6 +171,8 @@ type CreateReviewOptions struct {
Issue *Issue
Reviewer *User
Official bool
CommitID string
Stale bool
}
// IsOfficialReviewer check if reviewer can make official reviews in issue (counts towards required approvals)
@ -200,6 +204,8 @@ func createReview(e Engine, opts CreateReviewOptions) (*Review, error) {
ReviewerID: opts.Reviewer.ID,
Content: opts.Content,
Official: opts.Official,
CommitID: opts.CommitID,
Stale: opts.Stale,
}
if _, err := e.Insert(review); err != nil {
return nil, err
@ -258,7 +264,7 @@ func IsContentEmptyErr(err error) bool {
}
// SubmitReview creates a review out of the existing pending review or creates a new one if no pending review exist
func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content string) (*Review, *Comment, error) {
func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, commitID string, stale bool) (*Review, *Comment, error) {
sess := x.NewSession()
defer sess.Close()
if err := sess.Begin(); err != nil {
@ -295,6 +301,8 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin
Reviewer: doer,
Content: content,
Official: official,
CommitID: commitID,
Stale: stale,
})
if err != nil {
return nil, nil, err
@ -322,8 +330,10 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin
review.Issue = issue
review.Content = content
review.Type = reviewType
review.CommitID = commitID
review.Stale = stale
if _, err := sess.ID(review.ID).Cols("content, type, official").Update(review); err != nil {
if _, err := sess.ID(review.ID).Cols("content, type, official, commit_id, stale").Update(review); err != nil {
return nil, nil, err
}
}
@ -374,3 +384,17 @@ func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) {
return reviews, nil
}
// MarkReviewsAsStale marks existing reviews as stale
func MarkReviewsAsStale(issueID int64) (err error) {
_, err = x.Exec("UPDATE `review` SET stale=? WHERE issue_id=?", true, issueID)
return
}
// MarkReviewsAsNotStale marks existing reviews as not stale for a giving commit SHA
func MarkReviewsAsNotStale(issueID int64, commitID string) (err error) {
_, err = x.Exec("UPDATE `review` SET stale=? WHERE issue_id=? AND commit_id=?", false, issueID, commitID)
return
}

View File

@ -172,6 +172,7 @@ type ProtectBranchForm struct {
ApprovalsWhitelistUsers string
ApprovalsWhitelistTeams string
BlockOnRejectedReviews bool
DismissStaleApprovals bool
}
// Validate validates the fields
@ -462,6 +463,7 @@ type CodeCommentForm struct {
TreePath string `form:"path" binding:"Required"`
IsReview bool `form:"is_review"`
Reply int64 `form:"reply"`
LatestCommitID string
}
// Validate validates the fields
@ -473,6 +475,7 @@ func (f *CodeCommentForm) Validate(ctx *macaron.Context, errs binding.Errors) bi
type SubmitReviewForm struct {
Content string
Type string `binding:"Required;In(approve,comment,reject)"`
CommitID string
}
// Validate validates the fields

View File

@ -112,3 +112,9 @@ func (repo *Repository) GetPatch(base, head string, w io.Writer) error {
return NewCommand("format-patch", "--binary", "--stdout", base+"..."+head).
RunInDirPipeline(repo.Path, w, nil)
}
// GetDiffFromMergeBase generates and return patch data from merge base to head
func (repo *Repository) GetDiffFromMergeBase(base, head string, w io.Writer) error {
return NewCommand("diff", "-p", "--binary", base+"..."+head).
RunInDirPipeline(repo.Path, w, nil)
}

View File

@ -477,7 +477,7 @@ func PushUpdate(repo *models.Repository, branch string, opts PushUpdateOptions)
log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name)
go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true)
go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, opts.OldCommitID, opts.NewCommitID)
if err = models.WatchIfAuto(opts.PusherID, repo.ID, true); err != nil {
log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err)
@ -528,7 +528,7 @@ func PushUpdates(repo *models.Repository, optsList []*PushUpdateOptions) error {
log.Trace("TriggerTask '%s/%s' by %s", repo.Name, opts.Branch, pusher.Name)
go pull_service.AddTestPullRequestTask(pusher, repo.ID, opts.Branch, true)
go pull_service.AddTestPullRequestTask(pusher, repo.ID, opts.Branch, true, opts.OldCommitID, opts.NewCommitID)
if err = models.WatchIfAuto(opts.PusherID, repo.ID, true); err != nil {
log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err)

View File

@ -1413,6 +1413,8 @@ settings.protect_approvals_whitelist_enabled = Restrict approvals to whitelisted
settings.protect_approvals_whitelist_enabled_desc = Only reviews from whitelisted users or teams will count to the required approvals. Without approval whitelist, reviews from anyone with write access count to the required approvals.
settings.protect_approvals_whitelist_users = Whitelisted reviewers:
settings.protect_approvals_whitelist_teams = Whitelisted teams for reviews:
settings.dismiss_stale_approvals = Dismiss stale approvals
settings.dismiss_stale_approvals_desc = When new commits that change the content of the pull request are pushed to the branch, old approvals will be dismissed.
settings.add_protected_branch = Enable protection
settings.delete_protected_branch = Disable protection
settings.update_protect_branch_success = Branch protection for branch '%s' has been updated.

View File

@ -841,7 +841,7 @@ func TriggerTask(ctx *context.Context) {
log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name)
go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true)
go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, "", "")
ctx.Status(202)
}

View File

@ -37,12 +37,14 @@ func CreateCodeComment(ctx *context.Context, form auth.CodeCommentForm) {
comment, err := pull_service.CreateCodeComment(
ctx.User,
ctx.Repo.GitRepo,
issue,
signedLine,
form.Content,
form.TreePath,
form.IsReview,
form.Reply,
form.LatestCommitID,
)
if err != nil {
ctx.ServerError("CreateCodeComment", err)
@ -95,7 +97,7 @@ func SubmitReview(ctx *context.Context, form auth.SubmitReviewForm) {
}
}
_, comm, err := pull_service.SubmitReview(ctx.User, issue, reviewType, form.Content)
_, comm, err := pull_service.SubmitReview(ctx.User, ctx.Repo.GitRepo, issue, reviewType, form.Content, form.CommitID)
if err != nil {
if models.IsContentEmptyErr(err) {
ctx.Flash.Error(ctx.Tr("repo.issues.review.content.empty"))

View File

@ -245,6 +245,7 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm)
}
}
protectBranch.BlockOnRejectedReviews = f.BlockOnRejectedReviews
protectBranch.DismissStaleApprovals = f.DismissStaleApprovals
err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{
UserIDs: whitelistUsers,

View File

@ -64,7 +64,7 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor
}
defer func() {
go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false)
go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "")
}()
// Clone base repo.

View File

@ -5,10 +5,14 @@
package pull
import (
"bufio"
"bytes"
"context"
"fmt"
"os"
"path"
"strings"
"time"
"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/git"
@ -16,6 +20,8 @@ import (
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/notification"
issue_service "code.gitea.io/gitea/services/issue"
"github.com/unknwon/com"
)
// NewPullRequest creates new pull request with labels for repository.
@ -168,7 +174,7 @@ func addHeadRepoTasks(prs []*models.PullRequest) {
// AddTestPullRequestTask adds new test tasks by given head/base repository and head/base branch,
// and generate new patch for testing as needed.
func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSync bool) {
func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSync bool, oldCommitID, newCommitID string) {
log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", repoID, branch)
graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) {
// There is no sensible way to shut this down ":-("
@ -191,6 +197,22 @@ func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSy
}
if err == nil {
for _, pr := range prs {
if newCommitID != "" && newCommitID != git.EmptySHA {
changed, err := checkIfPRContentChanged(pr, oldCommitID, newCommitID)
if err != nil {
log.Error("checkIfPRContentChanged: %v", err)
}
if changed {
// Mark old reviews as stale if diff to mergebase has changed
if err := models.MarkReviewsAsStale(pr.IssueID); err != nil {
log.Error("MarkReviewsAsStale: %v", err)
}
}
if err := models.MarkReviewsAsNotStale(pr.IssueID, newCommitID); err != nil {
log.Error("MarkReviewsAsNotStale: %v", err)
}
}
pr.Issue.PullRequest = pr
notification.NotifyPullRequestSynchronized(doer, pr)
}
@ -211,6 +233,78 @@ func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSy
})
}
// checkIfPRContentChanged checks if diff to target branch has changed by push
// A commit can be considered to leave the PR untouched if the patch/diff with its merge base is unchanged
func checkIfPRContentChanged(pr *models.PullRequest, oldCommitID, newCommitID string) (hasChanged bool, err error) {
if err = pr.GetHeadRepo(); err != nil {
return false, fmt.Errorf("GetHeadRepo: %v", err)
} else if pr.HeadRepo == nil {
// corrupt data assumed changed
return true, nil
}
if err = pr.GetBaseRepo(); err != nil {
return false, fmt.Errorf("GetBaseRepo: %v", err)
}
headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath())
if err != nil {
return false, fmt.Errorf("OpenRepository: %v", err)
}
defer headGitRepo.Close()
// Add a temporary remote.
tmpRemote := "checkIfPRContentChanged-" + com.ToStr(time.Now().UnixNano())
if err = headGitRepo.AddRemote(tmpRemote, models.RepoPath(pr.BaseRepo.MustOwner().Name, pr.BaseRepo.Name), true); err != nil {
return false, fmt.Errorf("AddRemote: %s/%s-%s: %v", pr.HeadRepo.OwnerName, pr.HeadRepo.Name, tmpRemote, err)
}
defer func() {
if err := headGitRepo.RemoveRemote(tmpRemote); err != nil {
log.Error("checkIfPRContentChanged: RemoveRemote: %s/%s-%s: %v", pr.HeadRepo.OwnerName, pr.HeadRepo.Name, tmpRemote, err)
}
}()
// To synchronize repo and get a base ref
_, base, err := headGitRepo.GetMergeBase(tmpRemote, pr.BaseBranch, pr.HeadBranch)
if err != nil {
return false, fmt.Errorf("GetMergeBase: %v", err)
}
diffBefore := &bytes.Buffer{}
diffAfter := &bytes.Buffer{}
if err := headGitRepo.GetDiffFromMergeBase(base, oldCommitID, diffBefore); err != nil {
// If old commit not found, assume changed.
log.Debug("GetDiffFromMergeBase: %v", err)
return true, nil
}
if err := headGitRepo.GetDiffFromMergeBase(base, newCommitID, diffAfter); err != nil {
// New commit should be found
return false, fmt.Errorf("GetDiffFromMergeBase: %v", err)
}
diffBeforeLines := bufio.NewScanner(diffBefore)
diffAfterLines := bufio.NewScanner(diffAfter)
for diffBeforeLines.Scan() && diffAfterLines.Scan() {
if strings.HasPrefix(diffBeforeLines.Text(), "index") && strings.HasPrefix(diffAfterLines.Text(), "index") {
// file hashes can change without the diff changing
continue
} else if strings.HasPrefix(diffBeforeLines.Text(), "@@") && strings.HasPrefix(diffAfterLines.Text(), "@@") {
// the location of the difference may change
continue
} else if !bytes.Equal(diffBeforeLines.Bytes(), diffAfterLines.Bytes()) {
return true, nil
}
}
if diffBeforeLines.Scan() || diffAfterLines.Scan() {
// Diffs not of equal length
return true, nil
}
return false, nil
}
// PushToBaseRepo pushes commits from branches of head repository to
// corresponding branches of base repository.
// FIXME: Only push branches that are actually updates?

View File

@ -18,7 +18,7 @@ import (
)
// CreateCodeComment creates a comment on the code line
func CreateCodeComment(doer *models.User, issue *models.Issue, line int64, content string, treePath string, isReview bool, replyReviewID int64) (*models.Comment, error) {
func CreateCodeComment(doer *models.User, gitRepo *git.Repository, issue *models.Issue, line int64, content string, treePath string, isReview bool, replyReviewID int64, latestCommitID string) (*models.Comment, error) {
var (
existsReview bool
@ -73,6 +73,7 @@ func CreateCodeComment(doer *models.User, issue *models.Issue, line int64, conte
Reviewer: doer,
Issue: issue,
Official: false,
CommitID: latestCommitID,
})
if err != nil {
return nil, err
@ -94,7 +95,7 @@ func CreateCodeComment(doer *models.User, issue *models.Issue, line int64, conte
if !isReview && !existsReview {
// Submit the review we've just created so the comment shows up in the issue view
if _, _, err = SubmitReview(doer, issue, models.ReviewTypeComment, ""); err != nil {
if _, _, err = SubmitReview(doer, gitRepo, issue, models.ReviewTypeComment, "", latestCommitID); err != nil {
return nil, err
}
}
@ -159,16 +160,36 @@ func createCodeComment(doer *models.User, repo *models.Repository, issue *models
}
// SubmitReview creates a review out of the existing pending review or creates a new one if no pending review exist
func SubmitReview(doer *models.User, issue *models.Issue, reviewType models.ReviewType, content string) (*models.Review, *models.Comment, error) {
review, comm, err := models.SubmitReview(doer, issue, reviewType, content)
if err != nil {
return nil, nil, err
}
func SubmitReview(doer *models.User, gitRepo *git.Repository, issue *models.Issue, reviewType models.ReviewType, content, commitID string) (*models.Review, *models.Comment, error) {
pr, err := issue.GetPullRequest()
if err != nil {
return nil, nil, err
}
var stale bool
if reviewType != models.ReviewTypeApprove && reviewType != models.ReviewTypeReject {
stale = false
} else {
headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
if err != nil {
return nil, nil, err
}
if headCommitID == commitID {
stale = false
} else {
stale, err = checkIfPRContentChanged(pr, commitID, headCommitID)
if err != nil {
return nil, nil, err
}
}
}
review, comm, err := models.SubmitReview(doer, issue, reviewType, content, commitID, stale)
if err != nil {
return nil, nil, err
}
notification.NotifyPullRequestReview(pr, review, comm)
return review, comm, nil

View File

@ -4,6 +4,7 @@
{{end}}
<form class="ui form {{if $.hidden}}hide comment-form comment-form-reply{{end}}" action="{{$.root.Issue.HTMLURL}}/files/reviews/comments" method="post">
{{$.root.CsrfTokenHtml}}
<input type="hidden" name="latest_commit_id" value="{{$.root.AfterCommitID}}"/>
<input type="hidden" name="side" value="{{if $.Side}}{{$.Side}}{{end}}">
<input type="hidden" name="line" value="{{if $.Line}}{{$.Line}}{{end}}">
<input type="hidden" name="path" value="{{if $.File}}{{$.File}}{{end}}">

View File

@ -7,6 +7,7 @@
<div class="ui clearing segment">
<form class="ui form" action="{{.Link}}/reviews/submit" method="post">
{{.CsrfTokenHtml}}
<input type="hidden" name="commit_id" value="{{.AfterCommitID}}"/>
<i class="ui right floated link icon close"></i>
<div class="header">
{{$.i18n.Tr "repo.diff.review.header"}}

View File

@ -13,6 +13,11 @@
{{else}}grey{{end}}">
<span class="octicon octicon-{{.Type.Icon}}"></span>
</span>
{{if .Stale}}
<span class="type-icon text grey">
<i class="octicon icon fa-hourglass-end"></i>
</span>
{{end}}
<a class="ui avatar image" href="{{.Reviewer.HomeLink}}">
<img src="{{.Reviewer.RelAvatarLink}}">
</a>

View File

@ -211,6 +211,14 @@
<p class="help">{{.i18n.Tr "repo.settings.block_rejected_reviews_desc"}}</p>
</div>
</div>
<div class="field">
<div class="ui checkbox">
<input name="dismiss_stale_approvals" type="checkbox" {{if .Branch.DismissStaleApprovals}}checked{{end}}>
<label for="dismiss_stale_approvals">{{.i18n.Tr "repo.settings.dismiss_stale_approvals"}}</label>
<p class="help">{{.i18n.Tr "repo.settings.dismiss_stale_approvals_desc"}}</p>
</div>
</div>
</div>
<div class="ui divider"></div>