Comments on review should have the same sha

models/issue_comment.go

@@ -712,6 +712,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
@@ -878,6 +879,7 @@ type CreateCommentOptions struct {
 	RefAction        references.XRefAction
 	RefIsPull        bool
 	IsForcePush      bool
+	Invalidated      bool
 }
 
 // CreateComment creates comment of issue or commit.
@@ -953,6 +955,8 @@ type FindCommentsOptions struct {
 	ReviewID int64
 	Since    int64
 	Before   int64
+	Line     int64
+	TreePath string
 	Type     CommentType
 }
 
@@ -976,6 +980,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
 }
 
@@ -990,6 +1000,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").

models/migrations/migrations.go

@@ -250,6 +250,8 @@ var migrations = []Migration{
 	NewMigration("fix publisher ID for tag releases", fixPublisherIDforTagReleases),
 	// v157 -> v158
 	NewMigration("ensure repo topics are up-to-date", fixRepoTopics),
+	// v158 -> v159
+	NewMigration("code comment replies should have the commitID of the review they are replying to", updateCodeCommentReplies),
 }
 
 // GetCurrentDBVersion returns the current db version

models/migrations/v158.go

@@ -0,0 +1,110 @@
+// 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 (
+	"fmt"
+	"strconv"
+
+	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/setting"
+
+	"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
+	}
+
+	sqlSelect := `SELECT comment.id as id, first.commit_sha as commit_sha, first.patch as patch, first.invalidated as invalidated`
+	sqlTail := ` 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`
+
+	var sqlCmd string
+	var start = 0
+	var batchSize = 100
+	sess := x.NewSession()
+	defer sess.Close()
+	for {
+		if err := sess.Begin(); err != nil {
+			return err
+		}
+
+		if setting.Database.UseMSSQL {
+			if _, err := sess.Exec(sqlSelect + " INTO #temp_comments" + sqlTail); err != nil {
+				log.Error("unable to create temporary table")
+				return err
+			}
+		}
+
+		var comments = make([]*Comment, 0, batchSize)
+
+		switch {
+		case setting.Database.UseMySQL:
+			sqlCmd = sqlSelect + sqlTail + " LIMIT " + strconv.Itoa(batchSize) + ", " + strconv.Itoa(start)
+		case setting.Database.UsePostgreSQL:
+			fallthrough
+		case setting.Database.UseSQLite3:
+			sqlCmd = sqlSelect + sqlTail + " LIMIT " + strconv.Itoa(batchSize) + " OFFSET " + strconv.Itoa(start)
+		case setting.Database.UseMSSQL:
+			sqlCmd = "SELECT TOP " + strconv.Itoa(batchSize) + " * FROM #temp_comments WHERE " +
+				"(id NOT IN ( SELECT TOP " + strconv.Itoa(start) + " id FROM #temp_comments ORDER BY id )) ORDER BY id"
+		default:
+			return fmt.Errorf("Unsupported database type")
+		}
+
+		if err := sess.SQL(sqlCmd).Find(&comments); err != nil {
+			log.Error("failed to select: %v", err)
+			return err
+		}
+
+		for _, comment := range comments {
+			if _, err := sess.Table("comment").ID(comment.ID).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 err := sess.Commit(); err != nil {
+			return err
+		}
+		if len(comments) < batchSize {
+			break
+		}
+	}
+
+	return nil
+}

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,
 	})
 }