Skip to content

Commit b091c99

Browse files
zeripathtechknowlogicklunny
authored
Comments on review should have the same sha (#13448)
* 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]> * Fix broken migration Signed-off-by: Andrew Thornton <[email protected]> * fix mssql Signed-off-by: Andrew Thornton <[email protected]> * Create temporary table because ... well MSSQL ... Signed-off-by: Andrew Thornton <[email protected]> * Create temporary table because ... well MSSQL ... Signed-off-by: Andrew Thornton <[email protected]> * Create temporary table because ... well MSSQL ... Signed-off-by: Andrew Thornton <[email protected]> * fix mssql Signed-off-by: Andrew Thornton <[email protected]> * move session within the batch Signed-off-by: Andrew Thornton <[email protected]> * regen the sqlcmd each time round the loop Signed-off-by: Andrew Thornton <[email protected]> * as per @lunny Signed-off-by: Andrew Thornton <[email protected]> Co-authored-by: techknowlogick <[email protected]> Co-authored-by: Lunny Xiao <[email protected]>
1 parent 1213301 commit b091c99

File tree

4 files changed

+183
-24
lines changed

4 files changed

+183
-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: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
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+
"fmt"
9+
"strconv"
10+
11+
"code.gitea.io/gitea/modules/log"
12+
"code.gitea.io/gitea/modules/setting"
13+
14+
"xorm.io/xorm"
15+
)
16+
17+
func updateCodeCommentReplies(x *xorm.Engine) error {
18+
type Comment struct {
19+
ID int64 `xorm:"pk autoincr"`
20+
CommitSHA string `xorm:"VARCHAR(40)"`
21+
Patch string `xorm:"TEXT patch"`
22+
Invalidated bool
23+
24+
// Not extracted but used in the below query
25+
Type int `xorm:"INDEX"`
26+
Line int64 // - previous line / + proposed line
27+
TreePath string
28+
ReviewID int64 `xorm:"index"`
29+
}
30+
31+
if err := x.Sync2(new(Comment)); err != nil {
32+
return err
33+
}
34+
35+
sqlSelect := `SELECT comment.id as id, first.commit_sha as commit_sha, first.patch as patch, first.invalidated as invalidated`
36+
sqlTail := ` FROM comment INNER JOIN (
37+
SELECT C.id, C.review_id, C.line, C.tree_path, C.patch, C.commit_sha, C.invalidated
38+
FROM comment AS C
39+
WHERE C.type = 21
40+
AND C.created_unix =
41+
(SELECT MIN(comment.created_unix)
42+
FROM comment
43+
WHERE comment.review_id = C.review_id
44+
AND comment.type = 21
45+
AND comment.line = C.line
46+
AND comment.tree_path = C.tree_path)
47+
) AS first
48+
ON comment.review_id = first.review_id
49+
AND comment.tree_path = first.tree_path AND comment.line = first.line
50+
WHERE comment.type = 21
51+
AND comment.id != first.id
52+
AND comment.commit_sha != first.commit_sha`
53+
54+
var sqlCmd string
55+
var start = 0
56+
var batchSize = 100
57+
sess := x.NewSession()
58+
defer sess.Close()
59+
for {
60+
if err := sess.Begin(); err != nil {
61+
return err
62+
}
63+
64+
if setting.Database.UseMSSQL {
65+
if _, err := sess.Exec(sqlSelect + " INTO #temp_comments" + sqlTail); err != nil {
66+
log.Error("unable to create temporary table")
67+
return err
68+
}
69+
}
70+
71+
var comments = make([]*Comment, 0, batchSize)
72+
73+
switch {
74+
case setting.Database.UseMySQL:
75+
sqlCmd = sqlSelect + sqlTail + " LIMIT " + strconv.Itoa(batchSize) + ", " + strconv.Itoa(start)
76+
case setting.Database.UsePostgreSQL:
77+
fallthrough
78+
case setting.Database.UseSQLite3:
79+
sqlCmd = sqlSelect + sqlTail + " LIMIT " + strconv.Itoa(batchSize) + " OFFSET " + strconv.Itoa(start)
80+
case setting.Database.UseMSSQL:
81+
sqlCmd = "SELECT TOP " + strconv.Itoa(batchSize) + " * FROM #temp_comments WHERE " +
82+
"(id NOT IN ( SELECT TOP " + strconv.Itoa(start) + " id FROM #temp_comments ORDER BY id )) ORDER BY id"
83+
default:
84+
return fmt.Errorf("Unsupported database type")
85+
}
86+
87+
if err := sess.SQL(sqlCmd).Find(&comments); err != nil {
88+
log.Error("failed to select: %v", err)
89+
return err
90+
}
91+
92+
for _, comment := range comments {
93+
if _, err := sess.Table("comment").ID(comment.ID).Cols("commit_sha", "patch", "invalidated").Update(comment); err != nil {
94+
log.Error("failed to update comment[%d]: %v %v", comment.ID, comment, err)
95+
return err
96+
}
97+
}
98+
99+
start += len(comments)
100+
101+
if err := sess.Commit(); err != nil {
102+
return err
103+
}
104+
if len(comments) < batchSize {
105+
break
106+
}
107+
}
108+
109+
return nil
110+
}

‎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)