fix: git diff blob excerpt (#38808)

1. refactor the legacy code and add more comments, remove the "+1/-1"
tricks, clarify the BuildBlobExcerptDiffSection behavior
2. fix a line-counting bug (see screenshot below)

modules/git/blob.go

@@ -49,20 +49,32 @@ func (b *Blob) GetBlobLineCount(ctx context.Context, w io.Writer) (size int64, c
 		return 0, 0, err
 	}
 	defer reader.Close()
+	return getBlobLineCount(reader, w)
+}
+
+func getBlobLineCount(r io.Reader, w io.Writer) (size int64, count int, _ error) {
 	buf := make([]byte, 32*1024)
-	size, count = 0, 1
+	size, count = 0, 0
-	lineSep := []byte{'\n'}
+	var lastChar byte
+	lineSep := []byte("\n")
 	for {
-		c, err := reader.Read(buf)
+		c, err := r.Read(buf)
 		size += int64(c)
 		if w != nil {
 			if _, err := w.Write(buf[:c]); err != nil {
 				return size, count, err
 			}
 		}
-		count += bytes.Count(buf[:c], lineSep)
+		if c > 0 {
+			count += bytes.Count(buf[:c], lineSep)
+			lastChar = buf[c-1]
+		}
 		switch {
 		case errors.Is(err, io.EOF):
+			if size > 0 && lastChar != '\n' {
+				// it should match "git diff" hunk line number. "a\nb" => 2 lines, "a\nb\n" => 2 lines
+				count++
+			}
 			return size, count, nil
 		case err != nil:
 			return size, count, err

modules/git/blob_test.go

@@ -7,6 +7,7 @@ package git
 import (
 	"io"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -56,3 +57,20 @@ func Benchmark_Blob_Data(b *testing.B) {
 		_ = r.Close()
 	}
 }
+
+func TestGetBlobLineCount(t *testing.T) {
+	size, count, err := getBlobLineCount(strings.NewReader(""), nil)
+	assert.NoError(t, err)
+	assert.EqualValues(t, 0, size)
+	assert.Equal(t, 0, count)
+
+	size, count, err = getBlobLineCount(strings.NewReader("\n"), nil)
+	assert.NoError(t, err)
+	assert.EqualValues(t, 1, size)
+	assert.Equal(t, 1, count)
+
+	size, count, err = getBlobLineCount(strings.NewReader("a\nb"), nil)
+	assert.NoError(t, err)
+	assert.EqualValues(t, 3, size)
+	assert.Equal(t, 2, count)
+}

services/gitdiff/gitdiff.go

@@ -83,37 +83,41 @@ type DiffLine struct {
 	cachedDiffInline *DiffInline
 }
 
-// DiffLineSectionInfo represents diff line section meta data
+// DiffLineSectionInfo represents diff line section metadata
 type DiffLineSectionInfo struct {
 	language *diffVarMutable[string]
 
 	Path string
 
-	// These line "idx" are 1-based line numbers
+	// These line "idx" are 1-based line numbers (inclusive)
 	// Left/Right refer to the left/right side of the diff:
 	//
-	// LastLeftIdx | LastRightIdx
+	//   LastLeftIdx | LastRightIdx   (the last rendered line number before this hunk)
-	// [up/down expander] @@ hunk info @@
+	//   [up/down/single expander] @@ hunk info @@
-	// LeftIdx     | RightIdx
+	//   LeftIdx     | RightIdx       (the next rendered line number after this hunk)
-
+	//   The hunk has LeftHunkSize lines on left side, RightHunkSize lines on right side.
-	LastLeftIdx  int
+	//
-	LastRightIdx int
-	LeftIdx      int
-	RightIdx     int
-
-	// Hunk sizes of the hidden lines
-	LeftHunkSize  int
-	RightHunkSize int
-
 	// For example:
-	// 17 | 31
+	//   17 | 31    diff line ...
-	// [up/down] @@ -40,23 +54,9 @@ ....
+	//   [up/down] @@ -40,23 +54,7 @@ ....
-	// 40 | 54
+	//   40 | 54    diff line ...
+	//     ...      diff line ...
+	//   62 | 60    diff line ...
+	//   (then file end or another hunk)
 	//
 	// In this case:
-	// LastLeftIdx = 17, LastRightIdx = 31
+	//   LastLeftIdx = 17, LastRightIdx = 31
-	// LeftHunkSize = 23, RightHunkSize = 9
+	//   (left lines 18-39, right lines 31-53 are hidden)
-	// LeftIdx = 40, RightIdx = 54
+	//   LeftIdx = 40, RightIdx = 54
+	//   LeftHunkSize = 23, RightHunkSize = 7
+	//   Left hunk ends at line 40+23-1=62 (23 lines), right: 54+7-1=60 (7 lines)
+
+	LastLeftIdx   int
+	LastRightIdx  int
+	LeftIdx       int
+	RightIdx      int
+	LeftHunkSize  int
+	RightHunkSize int
 
 	HiddenCommentIDs []int64 // IDs of hidden comments in this section
 }

services/gitdiff/gitdiff_excerpt.go

@@ -16,6 +16,7 @@ import (
 )
 
 type BlobExcerptOptions struct {
+	// More details in DiffLineSectionInfo struct
 	LastLeft      int
 	LastRight     int
 	LeftIndex     int
@@ -26,11 +27,11 @@ type BlobExcerptOptions struct {
 	Language      string
 }
 
-func fillExcerptLines(section *DiffSection, filePath string, reader io.Reader, lang string, idxLeft, idxRight, chunkSize int) error {
+func (diffSection *DiffSection) fillExcerptLines(reader io.Reader, leftStart, rightStart, chunkSize int) error {
 	buf := &bytes.Buffer{}
 	scanner := bufio.NewScanner(reader)
 	var diffLines []*DiffLine
-	for line := 0; line < idxRight+chunkSize; line++ {
+	for rightLineIdx := 1; rightLineIdx < rightStart+chunkSize; rightLineIdx++ {
 		if ok := scanner.Scan(); !ok {
 			break
 		}
@@ -39,12 +40,12 @@ func fillExcerptLines(section *DiffSection, filePath string, reader io.Reader, l
 			buf.WriteString(lineText)
 			buf.WriteByte('\n')
 		}
-		if line < idxRight {
+		if rightLineIdx < rightStart {
 			continue
 		}
 		diffLine := &DiffLine{
-			LeftIdx:  idxLeft + (line - idxRight) + 1,
+			LeftIdx:  leftStart + (rightLineIdx - rightStart),
-			RightIdx: line + 1,
+			RightIdx: rightLineIdx,
 			Type:     DiffLinePlain,
 			Content:  " " + lineText,
 		}
@@ -53,46 +54,53 @@ func fillExcerptLines(section *DiffSection, filePath string, reader io.Reader, l
 	if err := scanner.Err(); err != nil {
 		return fmt.Errorf("fillExcerptLines scan: %w", err)
 	}
-	section.Lines = diffLines
+	diffSection.Lines = diffLines
 	// DiffLinePlain always uses right lines
-	section.highlightedRightLines.value = highlightCodeLines(filePath, lang, []*DiffSection{section}, false /* right */, buf.Bytes())
+	diffSection.highlightedRightLines.value = highlightCodeLines(diffSection.FileName, diffSection.language.value, []*DiffSection{diffSection}, false /* right */, buf.Bytes())
 	return nil
 }
 
 func BuildBlobExcerptDiffSection(filePath string, reader io.Reader, opts BlobExcerptOptions) (*DiffSection, error) {
 	lastLeft, lastRight, idxLeft, idxRight := opts.LastLeft, opts.LastRight, opts.LeftIndex, opts.RightIndex
 	leftHunkSize, rightHunkSize, direction := opts.LeftHunkSize, opts.RightHunkSize, opts.Direction
-	language := opts.Language
 
-	chunkSize := BlobExcerptChunkSize
+	expandLimit := BlobExcerptChunkSize
 	section := &DiffSection{
-		language:              &diffVarMutable[string]{value: language},
+		language:              &diffVarMutable[string]{value: opts.Language},
 		highlightLexer:        &diffVarMutable[chroma.Lexer]{},
 		highlightedLeftLines:  &diffVarMutable[map[int]template.HTML]{},
 		highlightedRightLines: &diffVarMutable[map[int]template.HTML]{},
 		FileName:              filePath,
 	}
 	var err error
-	if direction == "up" && (idxLeft-lastLeft) > chunkSize {
+	remainingLines := idxRight - lastRight
-		idxLeft -= chunkSize
+	if direction == "up" && remainingLines > expandLimit {
-		idxRight -= chunkSize
+		idxLeft -= expandLimit
-		leftHunkSize += chunkSize
+		idxRight -= expandLimit
-		rightHunkSize += chunkSize
+		leftHunkSize += expandLimit
-		err = fillExcerptLines(section, filePath, reader, language, idxLeft-1, idxRight-1, chunkSize)
+		rightHunkSize += expandLimit
-	} else if direction == "down" && (idxLeft-lastLeft) > chunkSize {
+		err = section.fillExcerptLines(reader, idxLeft, idxRight, expandLimit)
-		err = fillExcerptLines(section, filePath, reader, language, lastLeft, lastRight, chunkSize)
+	} else if direction == "down" && remainingLines > expandLimit {
-		lastLeft += chunkSize
+		err = section.fillExcerptLines(reader, lastLeft+1, lastRight+1, expandLimit)
-		lastRight += chunkSize
+		lastLeft += expandLimit
-	} else {
+		lastRight += expandLimit
-		offset := -1
+	} else /* "single" or [ ("up" or "down") and (remainingLines <= expandLimit) ] */ {
-		if direction == "down" {
+		if direction == "up" || direction == "single" {
-			offset = 0
+			// if the direction is "up" or "single":
+			// * top: last=0, idx=11, chunk=11: line 11 is already rendered, line 0 can be considered as a "virtually rendered line"
+			//   * then need to expand line 10 lines (1-10), so "-1".
+			// * middle: last=100, idx=106, chunk=6: line 100 and 106 are both already rendered
+			//   * then need to expand 5 lines (101-105), so "-1".
+			expandLimit = remainingLines - 1
+		} else {
+			// if the direction is "down": either the hidden lines are too many in the middle (otherwise "single"), or are at the bottom
+			// * "last" line is already rendered, so just render the remaining lines from the next line
+			expandLimit = remainingLines
 		}
-		err = fillExcerptLines(section, filePath, reader, language, lastLeft, lastRight, idxRight-lastRight+offset)
+		err = section.fillExcerptLines(reader, lastLeft+1, lastRight+1, expandLimit)
-		leftHunkSize = 0
+		// now, the hidden lines are fewer than "expand limit", after expand, no hidden lines anymore,
-		rightHunkSize = 0
+		// no need to show new "expand buttons" (setting them to 0 will make GetExpandDirection returns "no direction")
-		idxLeft = lastLeft
+		leftHunkSize, rightHunkSize, idxLeft, idxRight = 0, 0, 0, 0
-		idxRight = lastRight
 	}
 	if err != nil {
 		return nil, err

services/gitdiff/gitdiff_render_test.go

@@ -98,8 +98,8 @@ func TestGetDiffForRender(t *testing.T) {
 			ExpandDirection: "down",
 			LastLeftIdx:     76,
 			LastRightIdx:    73,
-			LeftIdx:         104,
+			LeftIdx:         103, // left has 103 lines
-			RightIdx:        101,
+			RightIdx:        100, // right has 100 lines
 		},
 	}
 	for idx, exp := range expectedSections {