From 1e723deb0cf1e83808e95618412727beaafd35bb Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sat, 28 Dec 2019 22:36:43 +0100 Subject: [PATCH 1/8] Check if push changes branch and mark reviews as stale --- models/review.go | 8 ++++ modules/git/repo_compare.go | 6 +++ modules/repofiles/update.go | 4 +- routers/repo/pull.go | 2 +- services/pull/merge.go | 2 +- services/pull/pull.go | 90 ++++++++++++++++++++++++++++++++++++- 6 files changed, 107 insertions(+), 5 deletions(-) diff --git a/models/review.go b/models/review.go index 493959e78e65a..1fa5f6153aae1 100644 --- a/models/review.go +++ b/models/review.go @@ -54,6 +54,7 @@ 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"` + Stale bool `xorm:"NOT NULL DEFAULT false"` CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` @@ -370,3 +371,10 @@ 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 +} diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 53b8af4bb4243..683ac7a7ee423 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 19c3d5b51c44b..7ad4a4d388ffc 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/routers/repo/pull.go b/routers/repo/pull.go index 418c2e9438bd5..08cf7410b94a5 100644 --- a/routers/repo/pull.go +++ b/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) } diff --git a/services/pull/merge.go b/services/pull/merge.go index 8aa38bf11eab7..b38c2e72f2766 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 6be9c2da176c0..82e95ea96e323 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,19 @@ func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSy } if err == nil { for _, pr := range prs { + if oldCommitID != "" && oldCommitID != git.EmptySHA && 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) + } + } + } + pr.Issue.PullRequest = pr notification.NotifyPullRequestSynchronized(doer, pr) } @@ -211,6 +230,75 @@ 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 { + return false, fmt.Errorf("GetDiffFromMergeBase: %v", err) + } + if err := headGitRepo.GetDiffFromMergeBase(base, newCommitID, diffAfter); err != nil { + 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? From 92b55e7c3961fdf798cc4e876869fe2144819cfa Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sat, 28 Dec 2019 22:37:29 +0100 Subject: [PATCH 2/8] Show old (stale) reviews with sandglass icon. --- templates/repo/issue/view_content/pull.tmpl | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 04f8a86cad30e..d41d99823bffa 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -13,6 +13,11 @@ {{else}}grey{{end}}"> + {{if .Stale}} + + + + {{end}} From fce82c48f939021acff358c14de9a24e1c71a5d7 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 29 Dec 2019 12:49:06 +0100 Subject: [PATCH 3/8] Save review commit id and mark as not stale if force pushed back. --- models/review.go | 24 +++++++++++++++++++---- modules/auth/repo_form.go | 5 +++-- routers/repo/pull_review.go | 3 ++- services/pull/pull.go | 5 ++++- services/pull/review.go | 30 ++++++++++++++++++++++++----- templates/repo/diff/new_review.tmpl | 1 + 6 files changed, 55 insertions(+), 13 deletions(-) diff --git a/models/review.go b/models/review.go index 1fa5f6153aae1..2fdcc08fee6df 100644 --- a/models/review.go +++ b/models/review.go @@ -53,8 +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"` - Stale 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"` @@ -166,6 +167,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) @@ -197,6 +200,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 @@ -255,7 +260,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, commitSHA string, stale bool) (*Review, *Comment, error) { sess := x.NewSession() defer sess.Close() if err := sess.Begin(); err != nil { @@ -292,6 +297,8 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin Reviewer: doer, Content: content, Official: official, + CommitID: commitSHA, + Stale: stale, }) if err != nil { return nil, nil, err @@ -319,8 +326,10 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin review.Issue = issue review.Content = content review.Type = reviewType + review.CommitID = commitSHA + 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 } } @@ -378,3 +387,10 @@ func MarkReviewsAsStale(issueID int64) (err error) { 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 26ed2bb692923..8c06cc8a9d767 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -470,8 +470,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/routers/repo/pull_review.go b/routers/repo/pull_review.go index b596d2578b6e3..81e37eff8a911 100644 --- a/routers/repo/pull_review.go +++ b/routers/repo/pull_review.go @@ -37,6 +37,7 @@ func CreateCodeComment(ctx *context.Context, form auth.CodeCommentForm) { comment, err := pull_service.CreateCodeComment( ctx.User, + ctx.Repo.GitRepo, issue, signedLine, form.Content, @@ -95,7 +96,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/services/pull/pull.go b/services/pull/pull.go index 82e95ea96e323..cfe526defa8cd 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -197,7 +197,7 @@ func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSy } if err == nil { for _, pr := range prs { - if oldCommitID != "" && oldCommitID != git.EmptySHA && newCommitID != "" && newCommitID != git.EmptySHA { + if newCommitID != "" && newCommitID != git.EmptySHA { changed, err := checkIfPRContentChanged(pr, oldCommitID, newCommitID) if err != nil { log.Error("checkIfPRContentChanged: %v", err) @@ -208,6 +208,9 @@ func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSy log.Error("MarkReviewsAsStale: %v", err) } } + if err := models.MarkReviewsAsNotStale(pr.IssueID, newCommitID); err != nil { + log.Error("MarkReviewsAsNotStale: %v", err) + } } pr.Issue.PullRequest = pr diff --git a/services/pull/review.go b/services/pull/review.go index 5b453e87f4416..a1139002c4f34 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) (*models.Comment, error) { var ( existsReview bool @@ -94,7 +94,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, "", ""); err != nil { return nil, err } } @@ -159,16 +159,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, commitSHA 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 == commitSHA { + stale = false + } else { + stale, err = checkIfPRContentChanged(pr, commitSHA, headCommitID) + if err != nil { + return nil, nil, err + } + } + } + + review, comm, err := models.SubmitReview(doer, issue, reviewType, content, commitSHA, stale) if err != nil { return nil, nil, err } + notification.NotifyPullRequestReview(pr, review, comm) return review, comm, nil diff --git a/templates/repo/diff/new_review.tmpl b/templates/repo/diff/new_review.tmpl index 68d8f893f2a34..4981c9ef48943 100644 --- a/templates/repo/diff/new_review.tmpl +++ b/templates/repo/diff/new_review.tmpl @@ -7,6 +7,7 @@
{{.CsrfTokenHtml}} +
{{$.i18n.Tr "repo.diff.review.header"}} From 1f4ccc973cde6276af859b33af167801bd2d05d9 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 29 Dec 2019 16:07:05 +0100 Subject: [PATCH 4/8] Add migration step --- models/migrations/migrations.go | 2 ++ models/migrations/v117.go | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 models/migrations/v117.go diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index e8bb3f16d460f..ea31d1ef85b2b 100644 --- a/models/migrations/migrations.go +++ b/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 diff --git a/models/migrations/v117.go b/models/migrations/v117.go new file mode 100644 index 0000000000000..9760a8ce4fb29 --- /dev/null +++ b/models/migrations/v117.go @@ -0,0 +1,19 @@ +// 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"` + } + + // Old reviews will have commit ID set to "" and not stale + return x.Sync2(new(Review)) +} From 74cd5a70b9dcbbeccba3901a6ef55aea890c2537 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 29 Dec 2019 21:57:59 +0100 Subject: [PATCH 5/8] Assume changed if old commit not found --- services/pull/pull.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index cfe526defa8cd..f39f2b566c6c3 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -273,9 +273,12 @@ func checkIfPRContentChanged(pr *models.PullRequest, oldCommitID, newCommitID st diffBefore := &bytes.Buffer{} diffAfter := &bytes.Buffer{} if err := headGitRepo.GetDiffFromMergeBase(base, oldCommitID, diffBefore); err != nil { - return false, fmt.Errorf("GetDiffFromMergeBase: %v", err) + // 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) } From bd32f941110d714cd1eb81354fd57bead79a278d Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Mon, 30 Dec 2019 17:07:39 +0100 Subject: [PATCH 6/8] Save commit id for complete review also when making code comments. --- models/review.go | 6 +++--- modules/auth/repo_form.go | 13 +++++++------ routers/repo/pull_review.go | 1 + services/pull/review.go | 13 +++++++------ templates/repo/diff/comment_form.tmpl | 1 + 5 files changed, 19 insertions(+), 15 deletions(-) diff --git a/models/review.go b/models/review.go index 2fdcc08fee6df..85b922368515d 100644 --- a/models/review.go +++ b/models/review.go @@ -260,7 +260,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, commitSHA string, stale bool) (*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 { @@ -297,7 +297,7 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm Reviewer: doer, Content: content, Official: official, - CommitID: commitSHA, + CommitID: commitID, Stale: stale, }) if err != nil { @@ -326,7 +326,7 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm review.Issue = issue review.Content = content review.Type = reviewType - review.CommitID = commitSHA + review.CommitID = commitID review.Stale = stale if _, err := sess.ID(review.ID).Cols("content, type, official, commit_id, stale").Update(review); err != nil { diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index 8c06cc8a9d767..eba19d6851f86 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -455,12 +455,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 diff --git a/routers/repo/pull_review.go b/routers/repo/pull_review.go index 81e37eff8a911..558473eb0839d 100644 --- a/routers/repo/pull_review.go +++ b/routers/repo/pull_review.go @@ -44,6 +44,7 @@ func CreateCodeComment(ctx *context.Context, form auth.CodeCommentForm) { form.TreePath, form.IsReview, form.Reply, + form.LatestCommitID, ) if err != nil { ctx.ServerError("CreateCodeComment", err) diff --git a/services/pull/review.go b/services/pull/review.go index a1139002c4f34..2bc8240230cbc 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, gitRepo *git.Repository, 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, gitRepo *git.Repository, issue *models Reviewer: doer, Issue: issue, Official: false, + CommitID: latestCommitID, }) if err != nil { return nil, err @@ -94,7 +95,7 @@ func CreateCodeComment(doer *models.User, gitRepo *git.Repository, issue *models if !isReview && !existsReview { // Submit the review we've just created so the comment shows up in the issue view - if _, _, err = SubmitReview(doer, gitRepo, issue, models.ReviewTypeComment, "", ""); err != nil { + if _, _, err = SubmitReview(doer, gitRepo, issue, models.ReviewTypeComment, "", latestCommitID); err != nil { return nil, err } } @@ -159,7 +160,7 @@ 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, gitRepo *git.Repository, issue *models.Issue, reviewType models.ReviewType, content, commitSHA string) (*models.Review, *models.Comment, error) { +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 @@ -174,17 +175,17 @@ func SubmitReview(doer *models.User, gitRepo *git.Repository, issue *models.Issu return nil, nil, err } - if headCommitID == commitSHA { + if headCommitID == commitID { stale = false } else { - stale, err = checkIfPRContentChanged(pr, commitSHA, headCommitID) + stale, err = checkIfPRContentChanged(pr, commitID, headCommitID) if err != nil { return nil, nil, err } } } - review, comm, err := models.SubmitReview(doer, issue, reviewType, content, commitSHA, stale) + review, comm, err := models.SubmitReview(doer, issue, reviewType, content, commitID, stale) if err != nil { return nil, nil, err } diff --git a/templates/repo/diff/comment_form.tmpl b/templates/repo/diff/comment_form.tmpl index 573e7d463830f..822b1e54f6759 100644 --- a/templates/repo/diff/comment_form.tmpl +++ b/templates/repo/diff/comment_form.tmpl @@ -4,6 +4,7 @@ {{end}} {{$.root.CsrfTokenHtml}} + From 72d99aad78c0c8b606de395b880eb233936d31af Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Thu, 2 Jan 2020 21:43:13 +0100 Subject: [PATCH 7/8] Add option to dismiss stale approvals --- models/branches.go | 10 +++++++--- models/migrations/v117.go | 9 ++++++++- modules/auth/repo_form.go | 1 + options/locale/locale_en-US.ini | 2 ++ routers/repo/setting_protected_branch.go | 1 + templates/repo/settings/protected_branch.tmpl | 7 +++++++ 6 files changed, 26 insertions(+), 4 deletions(-) diff --git a/models/branches.go b/models/branches.go index 21b23c75d92dc..72ba7c731e084 100644 --- a/models/branches.go +++ b/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 diff --git a/models/migrations/v117.go b/models/migrations/v117.go index 9760a8ce4fb29..c79cbb8ae3bb5 100644 --- a/models/migrations/v117.go +++ b/models/migrations/v117.go @@ -14,6 +14,13 @@ func addReviewCommitAndStale(x *xorm.Engine) error { 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 - return x.Sync2(new(Review)) + if err := x.Sync2(new(Review)); err != nil { + return err + } + return x.Sync2(new(ProtectedBranch)) } diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index eba19d6851f86..5ffeb005a1ebd 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -171,6 +171,7 @@ type ProtectBranchForm struct { EnableApprovalsWhitelist bool ApprovalsWhitelistUsers string ApprovalsWhitelistTeams string + DismissStaleApprovals bool } // Validate validates the fields diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 691190427148d..8bace4899f9c5 100644 --- a/options/locale/locale_en-US.ini +++ b/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. diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go index c279c94b1b64b..24c22b5125e79 100644 --- a/routers/repo/setting_protected_branch.go +++ b/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, diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index b6305861af1b6..166a1924a9957 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -204,6 +204,13 @@
{{end}}
+
+
+ + +

{{.i18n.Tr "repo.settings.dismiss_stale_approvals_desc"}}

+
+
From c076040a8c4b9a673b528769e8e475f61c404ce5 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 5 Jan 2020 08:25:21 +0100 Subject: [PATCH 8/8] Fix review comments. --- services/pull/pull.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index f39f2b566c6c3..b459d81cf79ae 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -255,13 +255,13 @@ func checkIfPRContentChanged(pr *models.PullRequest, oldCommitID, newCommitID st defer headGitRepo.Close() // Add a temporary remote. - tmpRemote := com.ToStr(time.Now().UnixNano()) + 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: %v", err) + 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", err) + log.Error("checkIfPRContentChanged: RemoveRemote: %s/%s-%s: %v", pr.HeadRepo.OwnerName, pr.HeadRepo.Name, tmpRemote, err) } }() // To synchronize repo and get a base ref