diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 59538d8a0e..90ce2e0a71 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -920,25 +920,29 @@ func ExcerptBlob(ctx *context.Context) { ctx.Error(http.StatusInternalServerError, "getExcerptLines") return } - if idxRight > lastRight { + + // After the "up" or "down" expansion, check if there's any remaining content in the diff and add a line that will + // be rendered into a new expander at either the top, or bottom. + lineSection := &gitdiff.DiffLine{ + Type: gitdiff.DiffLineSection, + SectionInfo: &gitdiff.DiffLineSectionInfo{ + Path: filePath, + LastLeftIdx: lastLeft, + LastRightIdx: lastRight, + LeftIdx: idxLeft, + RightIdx: idxRight, + LeftHunkSize: leftHunkSize, + RightHunkSize: rightHunkSize, + }, + } + if lineSection.GetExpandDirection() != gitdiff.DiffLineExpandNone { lineText := " " 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, - LastRightIdx: lastRight, - LeftIdx: idxLeft, - RightIdx: idxRight, - LeftHunkSize: leftHunkSize, - RightHunkSize: rightHunkSize, - }, - } + lineSection.Content = lineText + switch direction { case "up": section.Lines = append([]*gitdiff.DiffLine{lineSection}, section.Lines...) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 7033264f18..1f2a7f232f 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -85,11 +85,20 @@ type DiffLine struct { // DiffLineSectionInfo represents diff line section meta data type DiffLineSectionInfo struct { - Path string - LastLeftIdx int - LastRightIdx int - LeftIdx int - RightIdx int + Path string + + // Last(Left/Right)Idx do not directly relate to this diff section, but indicate the last line number in the + // previous diff section. Set to 0 for the first diff section of a file, and 1 for the first line of code in the + // file. + LastLeftIdx int + LastRightIdx int + + // (Left/Right)Idx are the first line number in this diff section + LeftIdx int + RightIdx int + + // Number of lines contained within each diff section. In the UI, these fields are set to 0 in cases where a + // section is being used as a placeholder at the end of a diff to allow expansion into the remainder of the file. LeftHunkSize int RightHunkSize int } @@ -157,7 +166,7 @@ func (d *DiffLine) GetExpandDirection() DiffLineExpandDirection { } if d.SectionInfo.LastLeftIdx <= 0 && d.SectionInfo.LastRightIdx <= 0 { return DiffLineExpandUp - } 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 DiffLineExpandUpDown } else if d.SectionInfo.LeftHunkSize <= 0 && d.SectionInfo.RightHunkSize <= 0 { return DiffLineExpandDown diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 695b177b8b..d4d1cd4460 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -717,6 +717,163 @@ func TestGetDiffFull(t *testing.T) { }) } +func TestDiffLine_GetExpandDirection(t *testing.T) { + tests := []struct { + name string + diffLine *DiffLine + expectedResult DiffLineExpandDirection + }{ + { + name: "non-section line - no expansion", + diffLine: &DiffLine{ + Type: DiffLineAdd, + SectionInfo: &DiffLineSectionInfo{}, + }, + expectedResult: DiffLineExpandNone, + }, + { + name: "nil section info - no expansion", + diffLine: &DiffLine{ + Type: DiffLineSection, + SectionInfo: nil, + }, + expectedResult: DiffLineExpandNone, + }, + { + name: "no lines between", + diffLine: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + // Previous section of the diff displayed up to line 530... + LastRightIdx: 530, + LastLeftIdx: 530, + // This section of the diff starts at line 531... + RightIdx: 531, + LeftIdx: 531, + }, + }, + // There are zero lines between 530 and 531, so we should have nothing to expand. + expectedResult: DiffLineExpandNone, + }, + { + name: "first diff section is the start of the file", + diffLine: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + // Last[...]Idx is set to zero when it's the first section in the file (and not 1, which would be + // the first section -is- the first line of the file). + LastRightIdx: 0, + LastLeftIdx: 0, + // The diff section is showing line 1, the top of th efile. + RightIdx: 1, + LeftIdx: 1, + }, + }, + // We're at the top of the file; no expansion. + expectedResult: DiffLineExpandNone, + }, + { + name: "first diff section doesn't start at the top of the file", + diffLine: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + // Last[...]Idx is set to zero when it's the first section in the file (and not 1, which would be + // the first section -is- the first line of the file). + LastRightIdx: 0, + LastLeftIdx: 0, + RightIdx: 531, + LeftIdx: 531, + }, + }, + // We're at the top of the diff but there's content above, so can only expand up. + expectedResult: DiffLineExpandUp, + }, + { + name: "middle of the file with single expansion", + diffLine: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + // Previous section ended at ~500... + LastRightIdx: 500, + LastLeftIdx: 500, + // Next section starts one line away... + RightIdx: 502, + LeftIdx: 502, + // The next block has content (> 0) + RightHunkSize: 50, + LeftHunkSize: 50, + }, + }, + // Can be expanded in a single direction, displaying the missing line (501). + expectedResult: DiffLineExpandSingle, + }, + { + name: "middle of the file with multi line expansion", + diffLine: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + // Previous section ended at ~500... + LastRightIdx: 500, + LastLeftIdx: 500, + // Lines 501-520 are hidden, exactly 20 lines, matching BlobExcerptChunkSize (20)... + RightIdx: 521, + LeftIdx: 521, + // The next block has content (> 0) + RightHunkSize: 50, + LeftHunkSize: 50, + }, + }, + // Can be expanded in a single direction, displaying all the hidden 20 lines. + expectedResult: DiffLineExpandSingle, + }, + { + name: "middle of the file with multi direction expansion", + diffLine: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + // Previous section ended at ~500... + LastRightIdx: 500, + LastLeftIdx: 500, + // Lines 501-521 are hidden, exactly 21 lines, exceeding BlobExcerptChunkSize (20)... + RightIdx: 522, + LeftIdx: 522, + // The next block has content (> 0) + RightHunkSize: 50, + LeftHunkSize: 50, + }, + }, + // Now can be expanded down to display from 501-520 (521 remains hidden), or up to display 502-521 (501 + // remains hidden). + expectedResult: DiffLineExpandUpDown, + }, + { + name: "end of the diff but still file content to display", + diffLine: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + // We had a previous diff section, of any size/location... + LastRightIdx: 200, + LastLeftIdx: 200, + RightIdx: 531, + LeftIdx: 531, + // Hunk size size 0 is a placeholder value for the end or beginning of a file... + RightHunkSize: 0, + LeftHunkSize: 0, + }, + }, + // Combination of conditions says we're at the end of the last diff section, can only expand down. + expectedResult: DiffLineExpandDown, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := tt.diffLine.GetExpandDirection() + assert.Equal(t, tt.expectedResult, result) + }) + } +} + func TestNoCrashes(t *testing.T) { type testcase struct { gitdiff string