From 154a996871bef4cd5972f94e5c75a336b37a8514 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 19 Oct 2020 21:53:12 +0100 Subject: [PATCH 1/2] When replying to an outdated comment it should not appear on the files page This happened because the comment took the latest commitID as its base instead of the reviewID that it was replying to. There was also no way of creating an already outdated comment - and a reply to a review on an outdated line should be outdated. Signed-off-by: Andrew Thornton --- models/issue_comment.go | 12 +++++ models/migrations/migrations.go | 2 + models/migrations/v156.go | 78 +++++++++++++++++++++++++++++++++ services/pull/review.go | 78 ++++++++++++++++++++++----------- 4 files changed, 145 insertions(+), 25 deletions(-) create mode 100644 models/migrations/v156.go diff --git a/models/issue_comment.go b/models/issue_comment.go index 5c053ec02a28a..1373dddcc9ad4 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -710,6 +710,7 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err RefAction: opts.RefAction, RefIsPull: opts.RefIsPull, IsForcePush: opts.IsForcePush, + Invalidated: opts.Invalidated, } if _, err = e.Insert(comment); err != nil { return nil, err @@ -876,6 +877,7 @@ type CreateCommentOptions struct { RefAction references.XRefAction RefIsPull bool IsForcePush bool + Invalidated bool } // CreateComment creates comment of issue or commit. @@ -951,6 +953,8 @@ type FindCommentsOptions struct { ReviewID int64 Since int64 Before int64 + Line int64 + TreePath string Type CommentType } @@ -974,6 +978,12 @@ func (opts *FindCommentsOptions) toConds() builder.Cond { if opts.Type != CommentTypeUnknown { cond = cond.And(builder.Eq{"comment.type": opts.Type}) } + if opts.Line > 0 { + cond = cond.And(builder.Eq{"comment.line": opts.Line}) + } + if len(opts.TreePath) > 0 { + cond = cond.And(builder.Eq{"comment.tree_path": opts.TreePath}) + } return cond } @@ -988,6 +998,8 @@ func findComments(e Engine, opts FindCommentsOptions) ([]*Comment, error) { sess = opts.setSessionPagination(sess) } + // WARNING: If you change this order you will need to fix createCodeComment + return comments, sess. Asc("comment.created_unix"). Asc("comment.id"). diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 0efcce30a921b..2eef103a845a7 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -246,6 +246,8 @@ var migrations = []Migration{ NewMigration("add timestamps to Star, Label, Follow, Watch and Collaboration", addTimeStamps), // v155 -> v156 NewMigration("add changed_protected_files column for pull_request table", addChangedProtectedFilesPullRequestColumn), + // v156 -> v157 + NewMigration("code comment replies should have the commitID of the review they are replying to", updateCodeCommentReplies), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v156.go b/models/migrations/v156.go new file mode 100644 index 0000000000000..d056ffe6dca5e --- /dev/null +++ b/models/migrations/v156.go @@ -0,0 +1,78 @@ +// Copyright 2020 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 ( + "code.gitea.io/gitea/modules/log" + + "xorm.io/xorm" +) + +func updateCodeCommentReplies(x *xorm.Engine) error { + type Comment struct { + ID int64 `xorm:"pk autoincr"` + CommitSHA string `xorm:"VARCHAR(40)"` + Patch string `xorm:"TEXT patch"` + Invalidated bool + + // Not extracted but used in the below query + Type int `xorm:"INDEX"` + Line int64 // - previous line / + proposed line + TreePath string + ReviewID int64 `xorm:"index"` + } + + if err := x.Sync2(new(Comment)); err != nil { + return err + } + + sess := x.NewSession() + defer sess.Close() + if err := sess.Begin(); err != nil { + return err + } + + var start = 0 + var batchSize = 100 + for { + var comments = make([]*Comment, 0, batchSize) + if err := sess.SQL(`SELECT comment.id as id, first.commit_sha as commit_sha, first.patch as patch, first.invalidated as invalidated + FROM comment INNER JOIN ( + SELECT C.id, C.review_id, C.line, C.tree_path, C.patch, C.commit_sha, C.invalidated + FROM comment AS C + WHERE C.type = 21 + AND C.created_unix = + (SELECT MIN(comment.created_unix) + FROM comment + WHERE comment.review_id = C.review_id + AND comment.type = 21 + AND comment.line = C.line + AND comment.tree_path = C.tree_path) + ) AS first + ON comment.review_id = first.review_id + AND comment.tree_path = first.tree_path AND comment.line = first.line + WHERE comment.type = 21 + AND comment.id != first.id + AND comment.commit_sha != first.commit_sha`).Limit(batchSize, start).Find(&comments); err != nil { + log.Error("failed to select: %v", err) + return err + } + + for _, comment := range comments { + if _, err := sess.Table("comment").Cols("commit_sha", "patch", "invalidated").Update(comment); err != nil { + log.Error("failed to update comment[%d]: %v %v", comment.ID, comment, err) + return err + } + } + + start += len(comments) + + if len(comments) < batchSize { + break + } + } + + return sess.Commit() +} diff --git a/services/pull/review.go b/services/pull/review.go index 99afdd73c2150..356e319003130 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -122,41 +122,69 @@ func createCodeComment(doer *models.User, repo *models.Repository, issue *models } defer gitRepo.Close() - // FIXME validate treePath - // Get latest commit referencing the commented line - // No need for get commit for base branch changes + invalidated := false + head := pr.GetGitRefName() if line > 0 { - commit, err := gitRepo.LineBlame(pr.GetGitRefName(), gitRepo.Path, treePath, uint(line)) - if err == nil { - commitID = commit.ID.String() - } else if !(strings.Contains(err.Error(), "exit status 128 - fatal: no such path") || notEnoughLines.MatchString(err.Error())) { - return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %v", pr.GetGitRefName(), gitRepo.Path, treePath, line, err) + if reviewID != 0 { + first, err := models.FindComments(models.FindCommentsOptions{ + ReviewID: reviewID, + Line: line, + TreePath: treePath, + Type: models.CommentTypeCode, + ListOptions: models.ListOptions{ + PageSize: 1, + Page: 1, + }, + }) + if err == nil && len(first) > 0 { + commitID = first[0].CommitSHA + invalidated = first[0].Invalidated + patch = first[0].Patch + } else if err != nil && !models.IsErrCommentNotExist(err) { + return nil, fmt.Errorf("Find first comment for %d line %d path %s. Error: %v", reviewID, line, treePath, err) + } else { + review, err := models.GetReviewByID(reviewID) + if err == nil && len(review.CommitID) > 0 { + head = review.CommitID + } else if err != nil && !models.IsErrReviewNotExist(err) { + return nil, fmt.Errorf("GetReviewByID %d. Error: %v", reviewID, err) + } + } + } + + if len(commitID) == 0 { + // FIXME validate treePath + // Get latest commit referencing the commented line + // No need for get commit for base branch changes + commit, err := gitRepo.LineBlame(head, gitRepo.Path, treePath, uint(line)) + if err == nil { + commitID = commit.ID.String() + } else if !(strings.Contains(err.Error(), "exit status 128 - fatal: no such path") || notEnoughLines.MatchString(err.Error())) { + return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %v", pr.GetGitRefName(), gitRepo.Path, treePath, line, err) + } } } // Only fetch diff if comment is review comment - if reviewID != 0 { - headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName()) - if err != nil { - return nil, fmt.Errorf("GetRefCommitID[%s]: %v", pr.GetGitRefName(), err) - } + if len(patch) == 0 && reviewID != 0 { patchBuf := new(bytes.Buffer) - if err := git.GetRepoRawDiffForFile(gitRepo, pr.MergeBase, headCommitID, git.RawDiffNormal, treePath, patchBuf); err != nil { - return nil, fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %v", err, gitRepo.Path, pr.MergeBase, headCommitID, treePath) + if err := git.GetRepoRawDiffForFile(gitRepo, pr.MergeBase, commitID, git.RawDiffNormal, treePath, patchBuf); err != nil { + return nil, fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %v", err, gitRepo.Path, pr.MergeBase, commitID, treePath) } patch = git.CutDiffAroundLine(patchBuf, int64((&models.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines) } return models.CreateComment(&models.CreateCommentOptions{ - Type: models.CommentTypeCode, - Doer: doer, - Repo: repo, - Issue: issue, - Content: content, - LineNum: line, - TreePath: treePath, - CommitSHA: commitID, - ReviewID: reviewID, - Patch: patch, + Type: models.CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: content, + LineNum: line, + TreePath: treePath, + CommitSHA: commitID, + ReviewID: reviewID, + Patch: patch, + Invalidated: invalidated, }) } From abc626d6209acb15a340c5c2bda4a112b401c0ae Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 20 Oct 2020 17:54:45 +0100 Subject: [PATCH 2/2] fix test Signed-off-by: Andrew Thornton --- services/pull/review.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/services/pull/review.go b/services/pull/review.go index 356e319003130..f0ee234a42419 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -167,9 +167,16 @@ func createCodeComment(doer *models.User, repo *models.Repository, issue *models // Only fetch diff if comment is review comment if len(patch) == 0 && reviewID != 0 { + if len(commitID) == 0 { + commitID, err = gitRepo.GetRefCommitID(pr.GetGitRefName()) + if err != nil { + return nil, fmt.Errorf("GetRefCommitID[%s]: %v", pr.GetGitRefName(), err) + } + } + patchBuf := new(bytes.Buffer) if err := git.GetRepoRawDiffForFile(gitRepo, pr.MergeBase, commitID, git.RawDiffNormal, treePath, patchBuf); err != nil { - return nil, fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %v", err, gitRepo.Path, pr.MergeBase, commitID, treePath) + return nil, fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %v", gitRepo.Path, pr.MergeBase, commitID, treePath, err) } patch = git.CutDiffAroundLine(patchBuf, int64((&models.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines) }