mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2025-07-23 19:49:40 +02:00
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).  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/<pull request number>.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 <gusted@noreply.codeberg.org> Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net> Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
This commit is contained in:
parent
ac43750ada
commit
7f6b9a8867
3 changed files with 190 additions and 20 deletions
|
@ -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
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue