Skip to content

Commit 3cab3be

Browse files
zeripathtechknowlogick
zeripath
and
techknowlogick
authored
Replies to outdated code comments should also be outdated (#13217)
* 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]> * fix test Signed-off-by: Andrew Thornton <[email protected]> Co-authored-by: techknowlogick <[email protected]>
1 parent fb756e7 commit 3cab3be

File tree

4 files changed

+151
-24
lines changed

4 files changed

+151
-24
lines changed

‎models/issue_comment.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -712,6 +712,7 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err
712712
RefAction: opts.RefAction,
713713
RefIsPull: opts.RefIsPull,
714714
IsForcePush: opts.IsForcePush,
715+
Invalidated: opts.Invalidated,
715716
}
716717
if _, err = e.Insert(comment); err != nil {
717718
return nil, err
@@ -878,6 +879,7 @@ type CreateCommentOptions struct {
878879
RefAction references.XRefAction
879880
RefIsPull bool
880881
IsForcePush bool
882+
Invalidated bool
881883
}
882884

883885
// CreateComment creates comment of issue or commit.
@@ -953,6 +955,8 @@ type FindCommentsOptions struct {
953955
ReviewID int64
954956
Since int64
955957
Before int64
958+
Line int64
959+
TreePath string
956960
Type CommentType
957961
}
958962

@@ -976,6 +980,12 @@ func (opts *FindCommentsOptions) toConds() builder.Cond {
976980
if opts.Type != CommentTypeUnknown {
977981
cond = cond.And(builder.Eq{"comment.type": opts.Type})
978982
}
983+
if opts.Line > 0 {
984+
cond = cond.And(builder.Eq{"comment.line": opts.Line})
985+
}
986+
if len(opts.TreePath) > 0 {
987+
cond = cond.And(builder.Eq{"comment.tree_path": opts.TreePath})
988+
}
979989
return cond
980990
}
981991

@@ -990,6 +1000,8 @@ func findComments(e Engine, opts FindCommentsOptions) ([]*Comment, error) {
9901000
sess = opts.setSessionPagination(sess)
9911001
}
9921002

1003+
// WARNING: If you change this order you will need to fix createCodeComment
1004+
9931005
return comments, sess.
9941006
Asc("comment.created_unix").
9951007
Asc("comment.id").

‎models/migrations/migrations.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,8 @@ var migrations = []Migration{
250250
NewMigration("fix publisher ID for tag releases", fixPublisherIDforTagReleases),
251251
// v157 -> v158
252252
NewMigration("ensure repo topics are up-to-date", fixRepoTopics),
253+
// v158 -> v159
254+
NewMigration("code comment replies should have the commitID of the review they are replying to", updateCodeCommentReplies),
253255
}
254256

255257
// GetCurrentDBVersion returns the current db version

‎models/migrations/v158.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: 59 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -122,41 +122,76 @@ 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)
169+
if len(patch) == 0 && reviewID != 0 {
170+
if len(commitID) == 0 {
171+
commitID, err = gitRepo.GetRefCommitID(pr.GetGitRefName())
172+
if err != nil {
173+
return nil, fmt.Errorf("GetRefCommitID[%s]: %v", pr.GetGitRefName(), err)
174+
}
142175
}
176+
143177
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)
178+
if err := git.GetRepoRawDiffForFile(gitRepo, pr.MergeBase, commitID, git.RawDiffNormal, treePath, patchBuf); err != nil {
179+
return nil, fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %v", gitRepo.Path, pr.MergeBase, commitID, treePath, err)
146180
}
147181
patch = git.CutDiffAroundLine(patchBuf, int64((&models.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines)
148182
}
149183
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,
184+
Type: models.CommentTypeCode,
185+
Doer: doer,
186+
Repo: repo,
187+
Issue: issue,
188+
Content: content,
189+
LineNum: line,
190+
TreePath: treePath,
191+
CommitSHA: commitID,
192+
ReviewID: reviewID,
193+
Patch: patch,
194+
Invalidated: invalidated,
160195
})
161196
}
162197

0 commit comments

Comments
 (0)