diff --git a/models/branches.go b/models/branches.go index 385817e4f..1932e06db 100644 --- a/models/branches.go +++ b/models/branches.go @@ -32,21 +32,23 @@ type ProtectedBranch struct { BranchName string `xorm:"UNIQUE(s)"` CanPush bool `xorm:"NOT NULL DEFAULT false"` EnableWhitelist bool - WhitelistUserIDs []int64 `xorm:"JSON TEXT"` - WhitelistTeamIDs []int64 `xorm:"JSON TEXT"` - EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"` - WhitelistDeployKeys bool `xorm:"NOT NULL DEFAULT false"` - MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"` - MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` - EnableStatusCheck bool `xorm:"NOT NULL DEFAULT false"` - StatusCheckContexts []string `xorm:"JSON TEXT"` - EnableApprovalsWhitelist bool `xorm:"NOT NULL DEFAULT false"` - ApprovalsWhitelistUserIDs []int64 `xorm:"JSON TEXT"` - ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` - RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"` - BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"` - CreatedUnix timeutil.TimeStamp `xorm:"created"` - UpdatedUnix timeutil.TimeStamp `xorm:"updated"` + WhitelistUserIDs []int64 `xorm:"JSON TEXT"` + WhitelistTeamIDs []int64 `xorm:"JSON TEXT"` + EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"` + WhitelistDeployKeys bool `xorm:"NOT NULL DEFAULT false"` + MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"` + MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` + EnableStatusCheck bool `xorm:"NOT NULL DEFAULT false"` + StatusCheckContexts []string `xorm:"JSON TEXT"` + EnableApprovalsWhitelist bool `xorm:"NOT NULL DEFAULT false"` + ApprovalsWhitelistUserIDs []int64 `xorm:"JSON TEXT"` + 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"` } // IsProtected returns if the branch is protected @@ -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 diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 73c9bc113..f26566b04 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -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 diff --git a/models/migrations/v118.go b/models/migrations/v118.go new file mode 100644 index 000000000..c79cbb8ae --- /dev/null +++ b/models/migrations/v118.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)) +} diff --git a/models/review.go b/models/review.go index bc7dfbcd1..2838cfa31 100644 --- a/models/review.go +++ b/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 +} diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index c87549af9..28615ebbc 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -172,6 +172,7 @@ type ProtectBranchForm struct { ApprovalsWhitelistUsers string ApprovalsWhitelistTeams string BlockOnRejectedReviews bool + DismissStaleApprovals bool } // Validate validates the fields @@ -456,12 +457,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 @@ -471,8 +473,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 diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 53b8af4bb..683ac7a7e 100644 --- a/modules/git/repo_compare.go +++ b/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) +} diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index 19c3d5b51..7ad4a4d38 100644 --- a/modules/repofiles/update.go +++ b/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) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index f6ff12250..a00df07d9 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -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. diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 1a5c4a036..93e98e0c3 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -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) } diff --git a/routers/repo/pull_review.go b/routers/repo/pull_review.go index b596d2578..558473eb0 100644 --- a/routers/repo/pull_review.go +++ b/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")) diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go index 8872c7471..da28ac50b 100644 --- a/routers/repo/setting_protected_branch.go +++ b/routers/repo/setting_protected_branch.go @@ -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, diff --git a/services/pull/merge.go b/services/pull/merge.go index 8aa38bf11..b38c2e72f 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 6be9c2da1..b459d81cf 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 5b453e87f..2bc824023 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 diff --git a/templates/repo/diff/comment_form.tmpl b/templates/repo/diff/comment_form.tmpl index 573e7d463..822b1e54f 100644 --- a/templates/repo/diff/comment_form.tmpl +++ b/templates/repo/diff/comment_form.tmpl @@ -4,6 +4,7 @@ {{end}}