Mark PR reviews as stale at push and allow to dismiss stale approvals

models/branches.go

@@ -44,6 +44,7 @@ type ProtectedBranch struct {
 	ApprovalsWhitelistUserIDs []int64            `xorm:"JSON TEXT"`
 	ApprovalsWhitelistTeamIDs []int64            `xorm:"JSON TEXT"`
 	RequiredApprovals         int64              `xorm:"NOT NULL DEFAULT 0"`
+	DismissStaleApprovals     bool               `xorm:"NOT NULL DEFAULT false"`
 	CreatedUnix               timeutil.TimeStamp `xorm:"created"`
 	UpdatedUnix               timeutil.TimeStamp `xorm:"updated"`
 }
@@ -154,10 +155,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

models/migrations/migrations.go

@@ -288,6 +288,8 @@ var migrations = []Migration{
 	NewMigration("add user_id prefix to existing user avatar name", renameExistingUserAvatarName),
 	// v116 -> v117
 	NewMigration("Extend TrackedTimes", extendTrackedTimes),
+	// v117 -> v118
+	NewMigration("Add commit id and stale to reviews", addReviewCommitAndStale),
 }
 
 // Migrate database to current version

models/migrations/v117.go

@@ -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))
+}

models/review.go

@@ -53,7 +53,9 @@ type Review struct {
 	IssueID    int64  `xorm:"index"`
 	Content    string `xorm:"TEXT"`
 	// Official is a review made by an assigned approver (counts towards approval)
-	Official bool `xorm:"NOT NULL DEFAULT false"`
+	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
+}

modules/auth/repo_form.go

@@ -171,6 +171,7 @@ type ProtectBranchForm struct {
 	EnableApprovalsWhitelist bool
 	ApprovalsWhitelistUsers  string
 	ApprovalsWhitelistTeams  string
+	DismissStaleApprovals    bool
 }
 
 // Validate validates the fields
@@ -455,12 +456,13 @@ func (f *MergePullRequestForm) Validate(ctx *macaron.Context, errs binding.Error
 
 // CodeCommentForm form for adding code comments for PRs
 type CodeCommentForm struct {
-	Content  string `binding:"Required"`
-	Side     string `binding:"Required;In(previous,proposed)"`
-	Line     int64
-	TreePath string `form:"path" binding:"Required"`
-	IsReview bool   `form:"is_review"`
-	Reply    int64  `form:"reply"`
+	Content        string `binding:"Required"`
+	Side           string `binding:"Required;In(previous,proposed)"`
+	Line           int64
+	TreePath       string `form:"path" binding:"Required"`
+	IsReview       bool   `form:"is_review"`
+	Reply          int64  `form:"reply"`
+	LatestCommitID string
 }
 
 // Validate validates the fields
@@ -470,8 +472,9 @@ func (f *CodeCommentForm) Validate(ctx *macaron.Context, errs binding.Errors) bi
 
 // SubmitReviewForm for submitting a finished code review
 type SubmitReviewForm struct {
-	Content string
-	Type    string `binding:"Required;In(approve,comment,reject)"`
+	Content  string
+	Type     string `binding:"Required;In(approve,comment,reject)"`
+	CommitID string
 }
 
 // Validate validates the fields

modules/git/repo_compare.go

@@ -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)
+}

modules/repofiles/update.go

@@ -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)

options/locale/locale_en-US.ini

@@ -1411,6 +1411,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.

routers/repo/pull.go

@@ -822,7 +822,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)
 }
 

routers/repo/pull_review.go

@@ -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"))

routers/repo/setting_protected_branch.go

@@ -244,6 +244,7 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm)
 				approvalsWhitelistTeams, _ = base.StringsToInt64s(strings.Split(f.ApprovalsWhitelistTeams, ","))
 			}
 		}
+		protectBranch.DismissStaleApprovals = f.DismissStaleApprovals
 
 		err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{
 			UserIDs:          whitelistUsers,

services/pull/merge.go

@@ -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.

services/pull/pull.go

@@ -5,17 +5,23 @@
 package pull
 
 import (
+	"bufio"
+	"bytes"
 	"context"
 	"fmt"
 	"os"
 	"path"
+	"strings"
+	"time"
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/graceful"
 	"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 := 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: %v", err)
+	}
+	defer func() {
+		if err := headGitRepo.RemoveRemote(tmpRemote); err != nil {
+			log.Error("checkIfPRContentChanged: RemoveRemote: %s", 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?