Fix diff blob excerpt expansion (#35922)

And add comments and tests

routers/web/repo/compare.go

@@ -9,7 +9,6 @@ import (
 	"encoding/csv"
 	"errors"
 	"fmt"
-	"html"
 	"io"
 	"net/http"
 	"net/url"
@@ -957,15 +956,9 @@ func ExcerptBlob(ctx *context.Context) {
 		ctx.HTTPError(http.StatusInternalServerError, "getExcerptLines")
 		return
 	}
-	if idxRight > lastRight {
+
-		lineText := " "
+	newLineSection := &gitdiff.DiffLine{
-		if rightHunkSize > 0 || leftHunkSize > 0 {
-			lineText = fmt.Sprintf("@@ -%d,%d +%d,%d @@\n", idxLeft, leftHunkSize, idxRight, rightHunkSize)
-		}
-		lineText = html.EscapeString(lineText)
-		lineSection := &gitdiff.DiffLine{
 		Type: gitdiff.DiffLineSection,
-			Content: lineText,
 		SectionInfo: &gitdiff.DiffLineSectionInfo{
 			Path:          filePath,
 			LastLeftIdx:   lastLeft,
@@ -976,11 +969,13 @@ func ExcerptBlob(ctx *context.Context) {
 			RightHunkSize: rightHunkSize,
 		},
 	}
+	if newLineSection.GetExpandDirection() != "" {
+		newLineSection.Content = fmt.Sprintf("@@ -%d,%d +%d,%d @@\n", idxLeft, leftHunkSize, idxRight, rightHunkSize)
 		switch direction {
 		case "up":
-			section.Lines = append([]*gitdiff.DiffLine{lineSection}, section.Lines...)
+			section.Lines = append([]*gitdiff.DiffLine{newLineSection}, section.Lines...)
 		case "down":
-			section.Lines = append(section.Lines, lineSection)
+			section.Lines = append(section.Lines, newLineSection)
 		}
 	}
 

services/gitdiff/gitdiff.go

@@ -83,13 +83,33 @@ type DiffLine struct {
 // DiffLineSectionInfo represents diff line section meta data
 type DiffLineSectionInfo struct {
 	Path string
+
+	// These line "idx" are 1-based line numbers
+	// Left/Right refer to the left/right side of the diff:
+	//
+	// LastLeftIdx | LastRightIdx
+	// [up/down expander] @@ hunk info @@
+	// LeftIdx     | RightIdx
+
 	LastLeftIdx  int
 	LastRightIdx int
 	LeftIdx      int
 	RightIdx     int
+
+	// Hunk sizes of the hidden lines
 	LeftHunkSize  int
 	RightHunkSize int
 
+	// For example:
+	// 17 | 31
+	// [up/down] @@ -40,23 +54,9 @@ ....
+	// 40 | 54
+	//
+	// In this case:
+	// LastLeftIdx = 17, LastRightIdx = 31
+	// LeftHunkSize = 23, RightHunkSize = 9
+	// LeftIdx = 40, RightIdx = 54
+
 	HiddenCommentIDs []int64 // IDs of hidden comments in this section
 }
 
@@ -158,13 +178,13 @@ func (d *DiffLine) getBlobExcerptQuery() string {
 	return query
 }
 
-func (d *DiffLine) getExpandDirection() string {
+func (d *DiffLine) GetExpandDirection() string {
 	if d.Type != DiffLineSection || d.SectionInfo == nil || d.SectionInfo.LeftIdx-d.SectionInfo.LastLeftIdx <= 1 || d.SectionInfo.RightIdx-d.SectionInfo.LastRightIdx <= 1 {
 		return ""
 	}
 	if d.SectionInfo.LastLeftIdx <= 0 && d.SectionInfo.LastRightIdx <= 0 {
 		return "up"
-	} else if d.SectionInfo.RightIdx-d.SectionInfo.LastRightIdx > BlobExcerptChunkSize && d.SectionInfo.RightHunkSize > 0 {
+	} else if d.SectionInfo.RightIdx-d.SectionInfo.LastRightIdx-1 > BlobExcerptChunkSize && d.SectionInfo.RightHunkSize > 0 {
 		return "updown"
 	} else if d.SectionInfo.LeftHunkSize <= 0 && d.SectionInfo.RightHunkSize <= 0 {
 		return "down"
@@ -202,13 +222,13 @@ func (d *DiffLine) RenderBlobExcerptButtons(fileNameHash string, data *DiffBlobE
 		content += htmlutil.HTMLFormat(`<span class="code-comment-more" data-tooltip-content="%s">%d</span>`, tooltip, len(d.SectionInfo.HiddenCommentIDs))
 	}
 
-	expandDirection := d.getExpandDirection()
+	expandDirection := d.GetExpandDirection()
-	if expandDirection == "up" || expandDirection == "updown" {
-		content += makeButton("up", "octicon-fold-up")
-	}
 	if expandDirection == "updown" || expandDirection == "down" {
 		content += makeButton("down", "octicon-fold-down")
 	}
+	if expandDirection == "up" || expandDirection == "updown" {
+		content += makeButton("up", "octicon-fold-up")
+	}
 	if expandDirection == "single" {
 		content += makeButton("single", "octicon-fold")
 	}

services/gitdiff/gitdiff_test.go

@@ -983,3 +983,126 @@ func TestDiffLine_RenderBlobExcerptButtons(t *testing.T) {
 		})
 	}
 }
+
+func TestDiffLine_GetExpandDirection(t *testing.T) {
+	cases := []struct {
+		name      string
+		diffLine  *DiffLine
+		direction string
+	}{
+		{
+			name:      "NotSectionLine",
+			diffLine:  &DiffLine{Type: DiffLineAdd, SectionInfo: &DiffLineSectionInfo{}},
+			direction: "",
+		},
+		{
+			name:      "NilSectionInfo",
+			diffLine:  &DiffLine{Type: DiffLineSection, SectionInfo: nil},
+			direction: "",
+		},
+		{
+			name: "NoHiddenLines",
+			// last block stops at line 100, next block starts at line 101, so no hidden lines, no expansion.
+			diffLine: &DiffLine{
+				Type: DiffLineSection,
+				SectionInfo: &DiffLineSectionInfo{
+					LastRightIdx: 100,
+					LastLeftIdx:  100,
+					RightIdx:     101,
+					LeftIdx:      101,
+				},
+			},
+			direction: "",
+		},
+		{
+			name: "FileHead",
+			diffLine: &DiffLine{
+				Type: DiffLineSection,
+				SectionInfo: &DiffLineSectionInfo{
+					LastRightIdx: 0, // LastXxxIdx = 0 means this is the first section in the file.
+					LastLeftIdx:  0,
+					RightIdx:     1,
+					LeftIdx:      1,
+				},
+			},
+			direction: "",
+		},
+		{
+			name: "FileHeadHiddenLines",
+			diffLine: &DiffLine{
+				Type: DiffLineSection,
+				SectionInfo: &DiffLineSectionInfo{
+					LastRightIdx: 0,
+					LastLeftIdx:  0,
+					RightIdx:     101,
+					LeftIdx:      101,
+				},
+			},
+			direction: "up",
+		},
+		{
+			name: "HiddenSingleHunk",
+			diffLine: &DiffLine{
+				Type: DiffLineSection,
+				SectionInfo: &DiffLineSectionInfo{
+					LastRightIdx:  100,
+					LastLeftIdx:   100,
+					RightIdx:      102,
+					LeftIdx:       102,
+					RightHunkSize: 1234, // non-zero dummy value
+					LeftHunkSize:  5678, // non-zero dummy value
+				},
+			},
+			direction: "single",
+		},
+		{
+			name: "HiddenSingleFullHunk",
+			// the hidden lines can exactly fit into one hunk
+			diffLine: &DiffLine{
+				Type: DiffLineSection,
+				SectionInfo: &DiffLineSectionInfo{
+					LastRightIdx:  100,
+					LastLeftIdx:   100,
+					RightIdx:      100 + BlobExcerptChunkSize + 1,
+					LeftIdx:       100 + BlobExcerptChunkSize + 1,
+					RightHunkSize: 1234, // non-zero dummy value
+					LeftHunkSize:  5678, // non-zero dummy value
+				},
+			},
+			direction: "single",
+		},
+		{
+			name: "HiddenUpDownHunks",
+			diffLine: &DiffLine{
+				Type: DiffLineSection,
+				SectionInfo: &DiffLineSectionInfo{
+					LastRightIdx:  100,
+					LastLeftIdx:   100,
+					RightIdx:      100 + BlobExcerptChunkSize + 2,
+					LeftIdx:       100 + BlobExcerptChunkSize + 2,
+					RightHunkSize: 1234, // non-zero dummy value
+					LeftHunkSize:  5678, // non-zero dummy value
+				},
+			},
+			direction: "updown",
+		},
+		{
+			name: "FileTail",
+			diffLine: &DiffLine{
+				Type: DiffLineSection,
+				SectionInfo: &DiffLineSectionInfo{
+					LastRightIdx:  100,
+					LastLeftIdx:   100,
+					RightIdx:      102,
+					LeftIdx:       102,
+					RightHunkSize: 0,
+					LeftHunkSize:  0,
+				},
+			},
+			direction: "down",
+		},
+	}
+	for _, c := range cases {
+		assert.Equal(t, c.direction, c.diffLine.GetExpandDirection(), "case %s expected direction: %s", c.name, c.direction)
+	}
+}