From 9aa580ce0e8791ddfcc1548c6a5ca516213bf785 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 5 Nov 2020 21:14:55 +0100 Subject: [PATCH] Replies to outdated code comments should also be outdated (#13217) (#13433) * 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 * fix test Signed-off-by: Andrew Thornton Co-authored-by: techknowlogick Co-authored-by: zeripath Co-authored-by: techknowlogick --- models/issue_comment.go | 12 ++++++ services/pull/review.go | 83 +++++++++++++++++++++++++++++------------ 2 files changed, 71 insertions(+), 24 deletions(-) diff --git a/models/issue_comment.go b/models/issue_comment.go index a5b6588fa..73cbb0566 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -725,6 +725,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 @@ -891,6 +892,7 @@ type CreateCommentOptions struct { RefAction references.XRefAction RefIsPull bool IsForcePush bool + Invalidated bool } // CreateComment creates comment of issue or commit. @@ -966,6 +968,8 @@ type FindCommentsOptions struct { ReviewID int64 Since int64 Before int64 + Line int64 + TreePath string Type CommentType } @@ -989,6 +993,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 } @@ -1003,6 +1013,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/services/pull/review.go b/services/pull/review.go index 99afdd73c..f0ee234a4 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -122,41 +122,76 @@ 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 { + 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, 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", gitRepo.Path, pr.MergeBase, commitID, treePath, err) } 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, }) }