Allow code review comments display cross commits even if the head branch has a force-push

Author: lunny

models/issues/comment.go

@@ -1007,6 +1007,7 @@ type FindCommentsOptions struct {
 	RepoID      int64
 	IssueID     int64
 	ReviewID    int64
+	CommitSHA   string
 	Since       int64
 	Before      int64
 	Line        int64
@@ -1052,6 +1053,9 @@ func (opts FindCommentsOptions) ToConds() builder.Cond {
 	if opts.IsPull.Has() {
 		cond = cond.And(builder.Eq{"issue.is_pull": opts.IsPull.Value()})
 	}
+	if opts.CommitSHA != "" {
+		cond = cond.And(builder.Eq{"comment.commit_sha": opts.CommitSHA})
+	}
 	return cond
 }
 

models/issues/comment_code.go

@@ -9,46 +9,52 @@ import (
 
 	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/models/renderhelper"
+	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/markup/markdown"
 
 	"xorm.io/builder"
 )
 
 // CodeComments represents comments on code by using this structure: FILENAME -> LINE (+ == proposed; - == previous) -> COMMENTS
-type CodeComments map[string]map[int64][]*Comment
+type CodeComments map[string][]*Comment
+
+func (cc CodeComments) AllComments() []*Comment {
+	var allComments []*Comment
+	for _, comments := range cc {
+		allComments = append(allComments, comments...)
+	}
+	return allComments
+}
 
 // FetchCodeComments will return a 2d-map: ["Path"]["Line"] = Comments at line
-func FetchCodeComments(ctx context.Context, issue *Issue, currentUser *user_model.User, showOutdatedComments bool) (CodeComments, error) {
-	return fetchCodeCommentsByReview(ctx, issue, currentUser, nil, showOutdatedComments)
+func FetchCodeComments(ctx context.Context, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, showOutdatedComments bool) (CodeComments, error) {
+	return fetchCodeCommentsByReview(ctx, repo, issueID, currentUser, nil, showOutdatedComments)
 }
 
-func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, currentUser *user_model.User, review *Review, showOutdatedComments bool) (CodeComments, error) {
-	pathToLineToComment := make(CodeComments)
+func fetchCodeCommentsByReview(ctx context.Context, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, review *Review, showOutdatedComments bool) (CodeComments, error) {
+	codeCommentsPathMap := make(CodeComments)
 	if review == nil {
 		review = &Review{ID: 0}
 	}
 	opts := FindCommentsOptions{
 		Type:     CommentTypeCode,
-		IssueID:  issue.ID,
+		IssueID:  issueID,
 		ReviewID: review.ID,
 	}
 
-	comments, err := findCodeComments(ctx, opts, issue, currentUser, review, showOutdatedComments)
+	comments, err := FindCodeComments(ctx, opts, repo, currentUser, review, showOutdatedComments)
 	if err != nil {
 		return nil, err
 	}
 
 	for _, comment := range comments {
-		if pathToLineToComment[comment.TreePath] == nil {
-			pathToLineToComment[comment.TreePath] = make(map[int64][]*Comment)
-		}
-		pathToLineToComment[comment.TreePath][comment.Line] = append(pathToLineToComment[comment.TreePath][comment.Line], comment)
+		codeCommentsPathMap[comment.TreePath] = append(codeCommentsPathMap[comment.TreePath], comment)
 	}
-	return pathToLineToComment, nil
+	return codeCommentsPathMap, nil
 }
 
-func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issue, currentUser *user_model.User, review *Review, showOutdatedComments bool) ([]*Comment, error) {
+func FindCodeComments(ctx context.Context, opts FindCommentsOptions, repo *repo_model.Repository, currentUser *user_model.User, review *Review, showOutdatedComments bool) ([]*Comment, error) {
 	var comments CommentList
 	if review == nil {
 		review = &Review{ID: 0}
@@ -67,10 +73,6 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu
 		return nil, err
 	}
 
-	if err := issue.LoadRepo(ctx); err != nil {
-		return nil, err
-	}
-
 	if err := comments.LoadPosters(ctx); err != nil {
 		return nil, err
 	}
@@ -110,12 +112,12 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu
 			return nil, err
 		}
 
-		if err := comment.LoadReactions(ctx, issue.Repo); err != nil {
+		if err := comment.LoadReactions(ctx, repo); err != nil {
 			return nil, err
 		}
 
 		var err error
-		rctx := renderhelper.NewRenderContextRepoComment(ctx, issue.Repo, renderhelper.RepoCommentOptions{
+		rctx := renderhelper.NewRenderContextRepoComment(ctx, repo, renderhelper.RepoCommentOptions{
 			FootnoteContextID: strconv.FormatInt(comment.ID, 10),
 		})
 		if comment.RenderedContent, err = markdown.RenderString(rctx, comment.Content); err != nil {
@@ -124,14 +126,3 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu
 	}
 	return comments[:n], nil
 }
-
-// FetchCodeCommentsByLine fetches the code comments for a given treePath and line number
-func FetchCodeCommentsByLine(ctx context.Context, issue *Issue, currentUser *user_model.User, treePath string, line int64, showOutdatedComments bool) (CommentList, error) {
-	opts := FindCommentsOptions{
-		Type:     CommentTypeCode,
-		IssueID:  issue.ID,
-		TreePath: treePath,
-		Line:     line,
-	}
-	return findCodeComments(ctx, opts, issue, currentUser, nil, showOutdatedComments)
-}

models/issues/comment_test.go

@@ -68,15 +68,15 @@ func TestFetchCodeComments(t *testing.T) {
 
 	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
-	res, err := issues_model.FetchCodeComments(db.DefaultContext, issue, user, false)
+	res, err := issues_model.FetchCodeComments(db.DefaultContext, issue.Repo, issue.ID, user, false)
 	assert.NoError(t, err)
 	assert.Contains(t, res, "README.md")
 	assert.Contains(t, res["README.md"], int64(4))
 	assert.Len(t, res["README.md"][4], 1)
-	assert.Equal(t, int64(4), res["README.md"][4][0].ID)
+	assert.Equal(t, int64(4), res["README.md"][0].ID)
 
 	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
-	res, err = issues_model.FetchCodeComments(db.DefaultContext, issue, user2, false)
+	res, err = issues_model.FetchCodeComments(db.DefaultContext, issue.Repo, issue.ID, user2, false)
 	assert.NoError(t, err)
 	assert.Len(t, res, 1)
 }

models/issues/review.go

@@ -159,7 +159,7 @@ func (r *Review) LoadCodeComments(ctx context.Context) (err error) {
 	if err = r.LoadIssue(ctx); err != nil {
 		return err
 	}
-	r.CodeComments, err = fetchCodeCommentsByReview(ctx, r.Issue, nil, r, false)
+	r.CodeComments, err = fetchCodeCommentsByReview(ctx, r.Issue.Repo, r.Issue.ID, nil, r, false)
 	return err
 }
 

models/issues/review_test.go

@@ -48,7 +48,7 @@ func TestReview_LoadCodeComments(t *testing.T) {
 	assert.NoError(t, review.LoadAttributes(db.DefaultContext))
 	assert.NoError(t, review.LoadCodeComments(db.DefaultContext))
 	assert.Len(t, review.CodeComments, 1)
-	assert.Equal(t, int64(4), review.CodeComments["README.md"][int64(4)][0].Line)
+	assert.Equal(t, int64(4), review.CodeComments["README.md"][0].Line)
 }
 
 func TestReviewType_Icon(t *testing.T) {

modules/git/diff.go

@@ -15,6 +15,7 @@ import (
 	"strings"
 
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/util"
 )
 
 // RawDiffType type of a raw diff.
@@ -107,12 +108,16 @@ func ParseDiffHunkString(diffHunk string) (leftLine, leftHunk, rightLine, rightH
 	leftLine, _ = strconv.Atoi(leftRange[0][1:])
 	if len(leftRange) > 1 {
 		leftHunk, _ = strconv.Atoi(leftRange[1])
+	} else {
+		leftHunk = util.Iif(leftLine > 0, leftLine, -leftLine)
 	}
 	if len(ranges) > 1 {
 		rightRange := strings.Split(ranges[1], ",")
 		rightLine, _ = strconv.Atoi(rightRange[0])
 		if len(rightRange) > 1 {
 			rightHunk, _ = strconv.Atoi(rightRange[1])
+		} else {
+			rightHunk = rightLine
 		}
 	} else {
 		log.Debug("Parse line number failed: %v", diffHunk)
@@ -342,3 +347,55 @@ func GetAffectedFiles(repo *Repository, branchName, oldCommitID, newCommitID str
 
 	return affectedFiles, err
 }
+
+type HunkInfo struct {
+	LeftLine  int64 // Line number in the old file
+	LeftHunk  int64 // Number of lines in the old file
+	RightLine int64 // Line number in the new file
+	RightHunk int64 // Number of lines in the new file
+}
+
+// GetAffectedHunksForTwoCommitsSpecialFile returns the affected hunks between two commits for a special file
+// git diff --unified=0 abc123 def456 -- src/main.go
+func GetAffectedHunksForTwoCommitsSpecialFile(ctx context.Context, repoPath, oldCommitID, newCommitID, filePath string) ([]*HunkInfo, error) {
+	reader, writer := io.Pipe()
+	defer func() {
+		_ = reader.Close()
+		_ = writer.Close()
+	}()
+	go func() {
+		if err := NewCommand("diff", "--unified=0", "--no-color").
+			AddDynamicArguments(oldCommitID, newCommitID).
+			AddDashesAndList(filePath).
+			Run(ctx, &RunOpts{
+				Dir:    repoPath,
+				Stdout: writer,
+			}); err != nil {
+			_ = writer.CloseWithError(fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s, %s]: %w", repoPath, oldCommitID, newCommitID, filePath, err))
+			return
+		}
+		_ = writer.Close()
+	}()
+
+	scanner := bufio.NewScanner(reader)
+	hunks := make([]*HunkInfo, 0, 32)
+	for scanner.Scan() {
+		lof := scanner.Text()
+		if !strings.HasPrefix(lof, "@@") {
+			continue
+		}
+		// Parse the hunk header
+		leftLine, leftHunk, rightLine, rightHunk := ParseDiffHunkString(lof)
+		hunks = append([]*HunkInfo{}, &HunkInfo{
+			LeftLine:  int64(leftLine),
+			LeftHunk:  int64(leftHunk),
+			RightLine: int64(rightLine),
+			RightHunk: int64(rightHunk),
+		})
+	}
+	if scanner.Err() != nil {
+		return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s, %s]: %w", repoPath, oldCommitID, newCommitID, filePath, scanner.Err())
+	}
+
+	return hunks, nil
+}

modules/git/diff_test.go

@@ -181,4 +181,10 @@ func TestParseDiffHunkString(t *testing.T) {
 	assert.Equal(t, 3, leftHunk)
 	assert.Equal(t, 19, rightLine)
 	assert.Equal(t, 5, rightHunk)
+
+	leftLine, leftHunk, rightLine, rightHunk = ParseDiffHunkString("@@ -1 +0,0 @@")
+	assert.Equal(t, 1, leftLine)
+	assert.Equal(t, 1, leftHunk)
+	assert.Equal(t, 0, rightLine)
+	assert.Equal(t, 0, rightHunk)
 }

routers/api/v1/repo/pull_review.go

@@ -13,7 +13,6 @@ import (
 	"code.gitea.io/gitea/models/organization"
 	access_model "code.gitea.io/gitea/models/perm/access"
 	user_model "code.gitea.io/gitea/models/user"
-	"code.gitea.io/gitea/modules/gitrepo"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/routers/api/v1/utils"
@@ -329,14 +328,7 @@ func CreatePullReview(ctx *context.APIContext) {
 
 	// if CommitID is empty, set it as lastCommitID
 	if opts.CommitID == "" {
-		gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.Issue.Repo)
-		if err != nil {
-			ctx.APIErrorInternal(err)
-			return
-		}
-		defer closer.Close()
-
-		headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
+		headCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(pr.GetGitRefName())
 		if err != nil {
 			ctx.APIErrorInternal(err)
 			return
@@ -357,11 +349,12 @@ func CreatePullReview(ctx *context.APIContext) {
 			ctx.Repo.GitRepo,
 			pr.Issue,
 			line,
+			pr.MergeBase,
+			opts.CommitID,
 			c.Body,
 			c.Path,
 			true, // pending review
 			0,    // no reply
-			opts.CommitID,
 			nil,
 		); err != nil {
 			ctx.APIErrorInternal(err)

routers/web/repo/issue_view.go

@@ -730,28 +730,23 @@ func prepareIssueViewCommentsAndSidebarParticipants(ctx *context.Context, issue
 				}
 				comment.Review.Reviewer = user_model.NewGhostUser()
 			}
-			if err = comment.Review.LoadCodeComments(ctx); err != nil {
-				ctx.ServerError("Review.LoadCodeComments", err)
-				return
-			}
+
 			for _, codeComments := range comment.Review.CodeComments {
-				for _, lineComments := range codeComments {
-					for _, c := range lineComments {
-						// Check tag.
-						role, ok = marked[c.PosterID]
-						if ok {
-							c.ShowRole = role
-							continue
-						}
+				for _, c := range codeComments {
+					// Check tag.
+					role, ok = marked[c.PosterID]
+					if ok {
+						c.ShowRole = role
+						continue
+					}
 
-						c.ShowRole, err = roleDescriptor(ctx, issue.Repo, c.Poster, permCache, issue, c.HasOriginalAuthor())
-						if err != nil {
-							ctx.ServerError("roleDescriptor", err)
-							return
-						}
-						marked[c.PosterID] = c.ShowRole
-						participants = addParticipant(c.Poster, participants)
+					c.ShowRole, err = roleDescriptor(ctx, issue.Repo, c.Poster, permCache, issue, c.HasOriginalAuthor())
+					if err != nil {
+						ctx.ServerError("roleDescriptor", err)
+						return
 					}
+					marked[c.PosterID] = c.ShowRole
+					participants = addParticipant(c.Poster, participants)
 				}
 			}
 			if err = comment.LoadResolveDoer(ctx); err != nil {