Skip to content

Commit 154a996

Browse files
zeripathzeripath
committed
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 <[email protected]>
1 parent 5626811 commit 154a996

File tree

4 files changed

+145
-25
lines changed

4 files changed

+145
-25
lines changed

‎models/issue_comment.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -710,6 +710,7 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err
710710
RefAction: opts.RefAction,
711711
RefIsPull: opts.RefIsPull,
712712
IsForcePush: opts.IsForcePush,
713+
Invalidated: opts.Invalidated,
713714
}
714715
if _, err = e.Insert(comment); err != nil {
715716
return nil, err
@@ -876,6 +877,7 @@ type CreateCommentOptions struct {
876877
RefAction references.XRefAction
877878
RefIsPull bool
878879
IsForcePush bool
880+
Invalidated bool
879881
}
880882

881883
// CreateComment creates comment of issue or commit.
@@ -951,6 +953,8 @@ type FindCommentsOptions struct {
951953
ReviewID int64
952954
Since int64
953955
Before int64
956+
Line int64
957+
TreePath string
954958
Type CommentType
955959
}
956960

@@ -974,6 +978,12 @@ func (opts *FindCommentsOptions) toConds() builder.Cond {
974978
if opts.Type != CommentTypeUnknown {
975979
cond = cond.And(builder.Eq{"comment.type": opts.Type})
976980
}
981+
if opts.Line > 0 {
982+
cond = cond.And(builder.Eq{"comment.line": opts.Line})
983+
}
984+
if len(opts.TreePath) > 0 {
985+
cond = cond.And(builder.Eq{"comment.tree_path": opts.TreePath})
986+
}
977987
return cond
978988
}
979989

@@ -988,6 +998,8 @@ func findComments(e Engine, opts FindCommentsOptions) ([]*Comment, error) {
988998
sess = opts.setSessionPagination(sess)
989999
}
9901000

1001+
// WARNING: If you change this order you will need to fix createCodeComment
1002+
9911003
return comments, sess.
9921004
Asc("comment.created_unix").
9931005
Asc("comment.id").

‎models/migrations/migrations.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,8 @@ var migrations = []Migration{
246246
NewMigration("add timestamps to Star, Label, Follow, Watch and Collaboration", addTimeStamps),
247247
// v155 -> v156
248248
NewMigration("add changed_protected_files column for pull_request table", addChangedProtectedFilesPullRequestColumn),
249+
// v156 -> v157
250+
NewMigration("code comment replies should have the commitID of the review they are replying to", updateCodeCommentReplies),
249251
}
250252

251253
// GetCurrentDBVersion returns the current db version

‎models/migrations/v156.go‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
// Copyright 2020 The Gitea Authors. All rights reserved.
2+
// Use of this source code is governed by a MIT-style
3+
// license that can be found in the LICENSE file.
4+
5+
package migrations
6+
7+
import (
8+
"code.gitea.io/gitea/modules/log"
9+
10+
"xorm.io/xorm"
11+
)
12+
13+
func updateCodeCommentReplies(x *xorm.Engine) error {
14+
type Comment struct {
15+
ID int64 `xorm:"pk autoincr"`
16+
CommitSHA string `xorm:"VARCHAR(40)"`
17+
Patch string `xorm:"TEXT patch"`
18+
Invalidated bool
19+
20+
// Not extracted but used in the below query
21+
Type int `xorm:"INDEX"`
22+
Line int64 // - previous line / + proposed line
23+
TreePath string
24+
ReviewID int64 `xorm:"index"`
25+
}
26+
27+
if err := x.Sync2(new(Comment)); err != nil {
28+
return err
29+
}
30+
31+
sess := x.NewSession()
32+
defer sess.Close()
33+
if err := sess.Begin(); err != nil {
34+
return err
35+
}
36+
37+
var start = 0
38+
var batchSize = 100
39+
for {
40+
var comments = make([]*Comment, 0, batchSize)
41+
if err := sess.SQL(`SELECT comment.id as id, first.commit_sha as commit_sha, first.patch as patch, first.invalidated as invalidated
42+
FROM comment INNER JOIN (
43+
SELECT C.id, C.review_id, C.line, C.tree_path, C.patch, C.commit_sha, C.invalidated
44+
FROM comment AS C
45+
WHERE C.type = 21
46+
AND C.created_unix =
47+
(SELECT MIN(comment.created_unix)
48+
FROM comment
49+
WHERE comment.review_id = C.review_id
50+
AND comment.type = 21
51+
AND comment.line = C.line
52+
AND comment.tree_path = C.tree_path)
53+
) AS first
54+
ON comment.review_id = first.review_id
55+
AND comment.tree_path = first.tree_path AND comment.line = first.line
56+
WHERE comment.type = 21
57+
AND comment.id != first.id
58+
AND comment.commit_sha != first.commit_sha`).Limit(batchSize, start).Find(&comments); err != nil {
59+
log.Error("failed to select: %v", err)
60+
return err
61+
}
62+
63+
for _, comment := range comments {
64+
if _, err := sess.Table("comment").Cols("commit_sha", "patch", "invalidated").Update(comment); err != nil {
65+
log.Error("failed to update comment[%d]: %v %v", comment.ID, comment, err)
66+
return err
67+
}
68+
}
69+
70+
start += len(comments)
71+
72+
if len(comments) < batchSize {
73+
break
74+
}
75+
}
76+
77+
return sess.Commit()
78+
}

‎services/pull/review.go‎

Lines changed: 53 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -122,41 +122,69 @@ func createCodeComment(doer *models.User, repo *models.Repository, issue *models
122122
}
123123
defer gitRepo.Close()
124124

125-
// FIXME validate treePath
126-
// Get latest commit referencing the commented line
127-
// No need for get commit for base branch changes
125+
invalidated := false
126+
head := pr.GetGitRefName()
128127
if line > 0 {
129-
commit, err := gitRepo.LineBlame(pr.GetGitRefName(), gitRepo.Path, treePath, uint(line))
130-
if err == nil {
131-
commitID = commit.ID.String()
132-
} else if !(strings.Contains(err.Error(), "exit status 128 - fatal: no such path") || notEnoughLines.MatchString(err.Error())) {
133-
return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %v", pr.GetGitRefName(), gitRepo.Path, treePath, line, err)
128+
if reviewID != 0 {
129+
first, err := models.FindComments(models.FindCommentsOptions{
130+
ReviewID: reviewID,
131+
Line: line,
132+
TreePath: treePath,
133+
Type: models.CommentTypeCode,
134+
ListOptions: models.ListOptions{
135+
PageSize: 1,
136+
Page: 1,
137+
},
138+
})
139+
if err == nil && len(first) > 0 {
140+
commitID = first[0].CommitSHA
141+
invalidated = first[0].Invalidated
142+
patch = first[0].Patch
143+
} else if err != nil && !models.IsErrCommentNotExist(err) {
144+
return nil, fmt.Errorf("Find first comment for %d line %d path %s. Error: %v", reviewID, line, treePath, err)
145+
} else {
146+
review, err := models.GetReviewByID(reviewID)
147+
if err == nil && len(review.CommitID) > 0 {
148+
head = review.CommitID
149+
} else if err != nil && !models.IsErrReviewNotExist(err) {
150+
return nil, fmt.Errorf("GetReviewByID %d. Error: %v", reviewID, err)
151+
}
152+
}
153+
}
154+
155+
if len(commitID) == 0 {
156+
// FIXME validate treePath
157+
// Get latest commit referencing the commented line
158+
// No need for get commit for base branch changes
159+
commit, err := gitRepo.LineBlame(head, gitRepo.Path, treePath, uint(line))
160+
if err == nil {
161+
commitID = commit.ID.String()
162+
} else if !(strings.Contains(err.Error(), "exit status 128 - fatal: no such path") || notEnoughLines.MatchString(err.Error())) {
163+
return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %v", pr.GetGitRefName(), gitRepo.Path, treePath, line, err)
164+
}
134165
}
135166
}
136167

137168
// Only fetch diff if comment is review comment
138-
if reviewID != 0 {
139-
headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
140-
if err != nil {
141-
return nil, fmt.Errorf("GetRefCommitID[%s]: %v", pr.GetGitRefName(), err)
142-
}
169+
if len(patch) == 0 && reviewID != 0 {
143170
patchBuf := new(bytes.Buffer)
144-
if err := git.GetRepoRawDiffForFile(gitRepo, pr.MergeBase, headCommitID, git.RawDiffNormal, treePath, patchBuf); err != nil {
145-
return nil, fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %v", err, gitRepo.Path, pr.MergeBase, headCommitID, treePath)
171+
if err := git.GetRepoRawDiffForFile(gitRepo, pr.MergeBase, commitID, git.RawDiffNormal, treePath, patchBuf); err != nil {
172+
return nil, fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %v", err, gitRepo.Path, pr.MergeBase, commitID, treePath)
146173
}
147174
patch = git.CutDiffAroundLine(patchBuf, int64((&models.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines)
148175
}
149176
return models.CreateComment(&models.CreateCommentOptions{
150-
Type: models.CommentTypeCode,
151-
Doer: doer,
152-
Repo: repo,
153-
Issue: issue,
154-
Content: content,
155-
LineNum: line,
156-
TreePath: treePath,
157-
CommitSHA: commitID,
158-
ReviewID: reviewID,
159-
Patch: patch,
177+
Type: models.CommentTypeCode,
178+
Doer: doer,
179+
Repo: repo,
180+
Issue: issue,
181+
Content: content,
182+
LineNum: line,
183+
TreePath: treePath,
184+
CommitSHA: commitID,
185+
ReviewID: reviewID,
186+
Patch: patch,
187+
Invalidated: invalidated,
160188
})
161189
}
162190

0 commit comments

Comments
 (0)