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

3 files changed, 122 insertions, 7 deletions

diff --git a/services/pull/merge.go b/services/pull/merge.go
index 8aa38bf11e..b38c2e72f2 100644
--- a/services/pull/merge.go
+++ b/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.
diff --git a/services/pull/pull.go b/services/pull/pull.go
index 6be9c2da17..b459d81cf7 100644
--- a/services/pull/pull.go
+++ b/services/pull/pull.go
@@ -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?
diff --git a/services/pull/review.go b/services/pull/review.go
index 5b453e87f4..2bc8240230 100644
--- a/services/pull/review.go
+++ b/services/pull/review.go
@@ -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)
+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
 	}
 
-	pr, err := issue.GetPullRequest()
+	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