From 7f6b9a8867a718846328ce1095693e21d02c8b68 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Wed, 16 Jul 2025 08:04:38 +0200 Subject: [PATCH] fix: expanding exactly 20 lines between diff sections leaves visual artifact (#8519) Fixes #8488. The issue was caused by having exactly 20 lines of text between two diff sections, which is `BlobExcerptChunkSize`. The previous diff section ends at 530, the next diff section starts at 551, leaving 20 hidden lines. This triggered an off-by-one error in determining whether a synthetic section should be added to the render to include a new expander; it was falsely being triggered when it shouldn't have been. Mostly correct logic was already present in `GetExpandDirection`, so I changed the `ExcerptBlob` web rendering to use `GetExpandDirection` on whether to include the synthetic section. Then I covered `GetExpandDirection` with unit tests covering all boundary conditions, which discovered one other minor bug -- the case where exactly `BlobExcerptChunkSize` is hidden between two diff sections should never have displayed an "Up" and "Down" expansion, but just a single expander (as below). ![Untitled-2025-07-04-1538(1)](/attachments/05573c5e-3cd4-46d5-8c1f-ecdb28302a19) Note that *technically* the `ExcerptBlob` change is not covered by any new tests... but the relevant logic has been moved to somewhere more easily testable and fully covered. If this isn't covered enough for tests, let me know. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8519 Reviewed-by: Gusted Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- routers/web/repo/compare.go | 32 ++++--- services/gitdiff/gitdiff.go | 21 +++-- services/gitdiff/gitdiff_test.go | 157 +++++++++++++++++++++++++++++++ 3 files changed, 190 insertions(+), 20 deletions(-) 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