fix: off-by-one errors in code comment patches (#39887)

Code comments next to a diff hunk could show one line too high in the
conversation view vs. in the code view that showed them on the correct
line.

- Treat the hunk end in `CutDiffAroundLine` as exclusive, so the line
after a hunk falls back to the unchanged-line context
- Don't count `\ No newline at end of file` markers as lines when
cutting the hunk
- Share the unchanged-line fallback with migrated review comments

modules/git/diff.go

@@ -5,6 +5,7 @@ package git
 
 import (
 	"bufio"
+	"cmp"
 	"context"
 	"fmt"
 	"io"
@@ -200,22 +201,18 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi
 			}
 			if old {
 				begin, _ = strconv.ParseInt(groups["beginOld"], 10, 64)
-				end, _ = strconv.ParseInt(groups["endOld"], 10, 64)
+				end, _ = strconv.ParseInt(cmp.Or(groups["endOld"], "1"), 10, 64)
 				// init otherLine with begin of opposite side
 				otherLine, _ = strconv.ParseInt(groups["beginNew"], 10, 64)
 			} else {
 				begin, _ = strconv.ParseInt(groups["beginNew"], 10, 64)
-				if groups["endNew"] != "" {
+				end, _ = strconv.ParseInt(cmp.Or(groups["endNew"], "1"), 10, 64)
-					end, _ = strconv.ParseInt(groups["endNew"], 10, 64)
-				} else {
-					end = 0
-				}
 				// init otherLine with begin of opposite side
 				otherLine, _ = strconv.ParseInt(groups["beginOld"], 10, 64)
 			}
 			end += begin // end is for real only the number of lines in hunk
 			// lof is between begin and end
-			if begin <= line && end >= line {
+			if begin <= line && line < end {
 				hunk = append(hunk, lof)
 				currentLine = begin
 				continue
@@ -265,27 +262,26 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi
 		oldBegin = otherLine
 		newBegin = currentLine
 	}
-	// headers + hunk header
-	newHunk := make([]string, headerLines)
-	// transfer existing headers
-	copy(newHunk, hunk[:headerLines])
-	// transfer last n lines
-	newHunk = append(newHunk, hunk[len(hunk)-numbersOfLine-1:]...)
 	// calculate newBegin, ... by counting lines
-	for i := len(hunk) - 1; i >= len(hunk)-numbersOfLine; i-- {
+	start := len(hunk)
-		switch hunk[i][0] {
+	for count := 0; count < numbersOfLine && start > headerLines+1; {
+		start--
+		switch hunk[start][0] {
 		case '+':
 			newBegin--
 			newNumOfLines++
 		case '-':
 			oldBegin--
 			oldNumOfLines++
+		case '\\':
+			continue
 		default:
 			oldBegin--
 			newBegin--
 			newNumOfLines++
 			oldNumOfLines++
 		}
+		count++
 	}
 
 	// "git diff" outputs "@@ -1 +1,3 @@" for "OLD" => "A\nB\nC"
@@ -294,9 +290,9 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi
 	// For example: "L1\nL2" => "A\nB", then the patch shows "L2" as line 1 on the left (deleted part)
 
 	// construct the new hunk header
-	newHunk[headerLines] = fmt.Sprintf("@@ -%d,%d +%d,%d @@",
+	hunk[headerLines] = fmt.Sprintf("@@ -%d,%d +%d,%d @@",
 		oldBegin, oldNumOfLines, newBegin, newNumOfLines)
-	return strings.Join(newHunk, "\n"), nil
+	return strings.Join(append(hunk[:headerLines+1], hunk[start:]...), "\n"), nil
 }
 
 // GetAffectedFiles returns the affected files between two commits

modules/git/diff_test.go

@@ -120,10 +120,18 @@ func TestCutDiffAroundLine(t *testing.T) {
 	assert.Empty(t, emptyResult)
 
 	// Line is out of scope
-	emptyResult, err = CutDiffAroundLine(strings.NewReader(exampleDiff), 434, false, 0)
+	emptyResult, err = CutDiffAroundLine(strings.NewReader(exampleDiff), 7, false, 3)
 	assert.NoError(t, err)
 	assert.Empty(t, emptyResult)
 
+	noNewlineDiff := "diff --git a/a b/a\n--- a/a\n+++ b/a\n@@ -1 +1 @@\n-a\n\\ No newline at end of file\n+a"
+	result, err = CutDiffAroundLine(strings.NewReader(noNewlineDiff), 1, false, 2)
+	assert.NoError(t, err)
+	assert.Equal(t, "diff --git a/a b/a\n--- a/a\n+++ b/a\n@@ -1,1 +1,1 @@\n-a\n\\ No newline at end of file\n+a", result)
+	result, err = CutDiffAroundLine(strings.NewReader(noNewlineDiff), 1, true, 2)
+	assert.NoError(t, err)
+	assert.Equal(t, "diff --git a/a b/a\n--- a/a\n+++ b/a\n@@ -1 +1 @@\n-a", result)
+
 	// Handle minus diffs properly
 	minusDiff, err := CutDiffAroundLine(strings.NewReader(breakingDiff), 2, false, 4)
 	assert.NoError(t, err)

services/gitdiff/gitdiff.go

@@ -1596,6 +1596,22 @@ func CommentAsDiff(ctx context.Context, c *issues_model.Comment) (*Diff, error)
 	return diff, nil
 }
 
+// GetCodeCommentPatch returns the code snippet stored with a code comment
+func GetCodeCommentPatch(ctx context.Context, gitRepo *git.Repository, mergeBase, headCommitID, treePath string, line int64) (string, error) {
+	patch, err := git.GetFileDiffCutAroundLine(ctx, gitRepo, mergeBase, headCommitID, treePath,
+		int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines,
+	)
+	contextCommitID := util.Iif(line < 0, mergeBase, headCommitID)
+	if err != nil || patch != "" || contextCommitID == "" {
+		return patch, err
+	}
+	patch, err = GeneratePatchForUnchangedLine(ctx, gitRepo, contextCommitID, treePath, line, setting.UI.CodeCommentLines)
+	if err != nil {
+		log.Debug("Unable to generate patch for unchanged line (file=%s, line=%d, commit=%s): %v", treePath, line, contextCommitID, err)
+	}
+	return patch, nil
+}
+
 // GeneratePatchForUnchangedLine creates a patch showing code context for an unchanged line
 func GeneratePatchForUnchangedLine(ctx context.Context, gitRepo *git.Repository, commitID, treePath string, line int64, contextLines int) (string, error) {
 	commit, err := gitRepo.GetCommit(ctx, commitID)

services/migrations/gitea_uploader.go

@@ -31,6 +31,7 @@ import (
 	"gitea.dev/modules/timeutil"
 	"gitea.dev/modules/uri"
 	"gitea.dev/modules/util"
+	"gitea.dev/services/gitdiff"
 	"gitea.dev/services/pull"
 	repo_service "gitea.dev/services/repository"
 )
@@ -905,10 +906,7 @@ func (g *GiteaLocalUploader) CreateReviews(ctx context.Context, reviews ...*base
 			// SECURITY: The TreePath must be cleaned! use relative path
 			comment.TreePath = util.PathJoinRel(comment.TreePath)
 
-			patch, _ := git.GetFileDiffCutAroundLine(ctx,
+			patch, _ := gitdiff.GetCodeCommentPatch(ctx, g.gitRepo, pr.MergeBase, headCommitID, comment.TreePath, int64(line+comment.Position-1))
-				g.gitRepo, pr.MergeBase, headCommitID, comment.TreePath,
-				int64((&issues_model.Comment{Line: int64(line + comment.Position - 1)}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines,
-			)
 
 			if comment.CreatedAt.IsZero() {
 				comment.CreatedAt = review.CreatedAt

services/pull/review.go

@@ -17,7 +17,6 @@ import (
 	"gitea.dev/modules/git/gitcmd"
 	"gitea.dev/modules/log"
 	"gitea.dev/modules/optional"
-	"gitea.dev/modules/setting"
 	"gitea.dev/modules/util"
 	"gitea.dev/services/gitdiff"
 	notify_service "gitea.dev/services/notify"
@@ -258,23 +257,10 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo
 			commitID = headCommitID
 		}
 
-		patch, err = git.GetFileDiffCutAroundLine(ctx,
+		patch, err = gitdiff.GetCodeCommentPatch(ctx, gitRepo, pr.MergeBase, headCommitID, treePath, line)
-			gitRepo, pr.MergeBase, headCommitID, treePath,
-			int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines,
-		)
 		if err != nil {
 			return nil, err
 		}
-
-		// If patch is still empty (unchanged line), generate code context
-		contextCommitID := util.Iif(line < 0, pr.MergeBase, headCommitID) // the line number is relative to the diff sides, not to the blamed commitID
-		if patch == "" && contextCommitID != "" {
-			patch, err = gitdiff.GeneratePatchForUnchangedLine(ctx, gitRepo, contextCommitID, treePath, line, setting.UI.CodeCommentLines)
-			if err != nil {
-				// Log the error but don't fail comment creation
-				log.Debug("Unable to generate patch for unchanged line (file=%s, line=%d, commit=%s): %v", treePath, line, contextCommitID, err)
-			}
-		}
 	}
 	return issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{
 		Type:        issues_model.CommentTypeCode,