Replies to outdated code comments should also be outdated

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,78 @@
+// 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 (
+	"code.gitea.io/gitea/modules/log"
+
+	"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
+	}
+
+	sess := x.NewSession()
+	defer sess.Close()
+	if err := sess.Begin(); err != nil {
+		return err
+	}
+
+	var start = 0
+	var batchSize = 100
+	for {
+		var comments = make([]*Comment, 0, batchSize)
+		if err := sess.SQL(`SELECT comment.id as id, first.commit_sha as commit_sha, first.patch as patch, first.invalidated as invalidated
+		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`).Limit(batchSize, start).Find(&comments); err != nil {
+			log.Error("failed to select: %v", err)
+			return err
+		}
+
+		for _, comment := range comments {
+			if _, err := sess.Table("comment").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 len(comments) < batchSize {
+			break
+		}
+	}
+
+	return sess.Commit()
+}

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