mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-07-18 15:37:48 +00:00
This PR replaces the existing `skip-to` approach for rendering large diffs with paging over the changeset of a git diff, enabling a more efficient way to render the compare view for PR, compare, and commit views. ## Problem When a user opens a compare page for a large range of commits, Forgejo calculates the entire diff (potentially multiple times) Which consumes significant CPU time on the server and produces an enormous amount of HTML for the browser to parse and display. At some point the page becomes unusable or fails to load entirely. The previous `skip-to` approach attempted to mitigate this by allowing the user to jump to a specific file or section within the diff. However, this still required the diff process and prepare the full changeset up to that point. It did not fundamentally bound the amount of work done per request: the server could still spend excessive time calculating the Diff. As a result, `skip-to` was not a reliable solution for very large diffs. TLDR; `--skip-to` is an output-ordering/presentation option, not a performance optimization. [git docs](https://git-scm.com/docs/diff-options#Documentation/diff-options.txt---skip-tofile) ## Solution This change introduces paging over the changeset of a git diff. Rather than relying on `skip-to`, the diff is now split into manageable pages. Only a subset of the changed files is fetched, highlighted, and rendered at a time. ### Tests for Go changes - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I ran... - [x] `make pr-go` before pushing ### 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 - [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. <!--start release-notes-assistant--> ## Release notes <!--URL:https://codeberg.org/forgejo/forgejo--> - Security features - [PR](https://codeberg.org/forgejo/forgejo/pulls/12808): <!--number 12808 --><!--line 0 --><!--description ZG9uJ3QgcmVuZGVyIGxhcmdlIGNvbXBhcmUgZGlmZg==-->don't render large compare diff<!--description--> <!--end release-notes-assistant--> Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12808 Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
229 lines
5.5 KiB
Go
229 lines
5.5 KiB
Go
// Copyright 2021 The Gitea Authors. All rights reserved.
|
|
// SPDX-License-Identifier: MIT
|
|
|
|
package gitdiff
|
|
|
|
import (
|
|
"encoding/csv"
|
|
"strings"
|
|
"testing"
|
|
|
|
"forgejo.org/models/db"
|
|
csv_module "forgejo.org/modules/csv"
|
|
"forgejo.org/modules/setting"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
func TestCSVDiff(t *testing.T) {
|
|
cases := []struct {
|
|
diff string
|
|
base string
|
|
head string
|
|
cells [][]TableDiffCellType
|
|
}{
|
|
// case 0 - initial commit of a csv
|
|
{
|
|
diff: `diff --git a/unittest.csv b/unittest.csv
|
|
--- a/unittest.csv
|
|
+++ b/unittest.csv
|
|
@@ -0,0 +1,2 @@
|
|
+col1,col2
|
|
+a,a`,
|
|
base: "",
|
|
head: `col1,col2
|
|
a,a`,
|
|
cells: [][]TableDiffCellType{
|
|
{TableDiffCellAdd, TableDiffCellAdd},
|
|
{TableDiffCellAdd, TableDiffCellAdd},
|
|
},
|
|
},
|
|
// case 1 - adding 1 row at end
|
|
{
|
|
diff: `diff --git a/unittest.csv b/unittest.csv
|
|
--- a/unittest.csv
|
|
+++ b/unittest.csv
|
|
@@ -1,2 +1,3 @@
|
|
col1,col2
|
|
-a,a
|
|
+a,a
|
|
+b,b`,
|
|
base: `col1,col2
|
|
a,a`,
|
|
head: `col1,col2
|
|
a,a
|
|
b,b`,
|
|
cells: [][]TableDiffCellType{
|
|
{TableDiffCellUnchanged, TableDiffCellUnchanged},
|
|
{TableDiffCellUnchanged, TableDiffCellUnchanged},
|
|
{TableDiffCellAdd, TableDiffCellAdd},
|
|
},
|
|
},
|
|
// case 2 - row deleted
|
|
{
|
|
diff: `diff --git a/unittest.csv b/unittest.csv
|
|
--- a/unittest.csv
|
|
+++ b/unittest.csv
|
|
@@ -1,3 +1,2 @@
|
|
col1,col2
|
|
-a,a
|
|
b,b`,
|
|
base: `col1,col2
|
|
a,a
|
|
b,b`,
|
|
head: `col1,col2
|
|
b,b`,
|
|
cells: [][]TableDiffCellType{
|
|
{TableDiffCellUnchanged, TableDiffCellUnchanged},
|
|
{TableDiffCellDel, TableDiffCellDel},
|
|
{TableDiffCellUnchanged, TableDiffCellUnchanged},
|
|
},
|
|
},
|
|
// case 3 - row changed
|
|
{
|
|
diff: `diff --git a/unittest.csv b/unittest.csv
|
|
--- a/unittest.csv
|
|
+++ b/unittest.csv
|
|
@@ -1,2 +1,2 @@
|
|
col1,col2
|
|
-b,b
|
|
+b,c`,
|
|
base: `col1,col2
|
|
b,b`,
|
|
head: `col1,col2
|
|
b,c`,
|
|
cells: [][]TableDiffCellType{
|
|
{TableDiffCellUnchanged, TableDiffCellUnchanged},
|
|
{TableDiffCellUnchanged, TableDiffCellChanged},
|
|
},
|
|
},
|
|
// case 4 - all deleted
|
|
{
|
|
diff: `diff --git a/unittest.csv b/unittest.csv
|
|
--- a/unittest.csv
|
|
+++ b/unittest.csv
|
|
@@ -1,2 +0,0 @@
|
|
-col1,col2
|
|
-b,c`,
|
|
base: `col1,col2
|
|
b,c`,
|
|
head: "",
|
|
cells: [][]TableDiffCellType{
|
|
{TableDiffCellDel, TableDiffCellDel},
|
|
{TableDiffCellDel, TableDiffCellDel},
|
|
},
|
|
},
|
|
// case 5 - renames first column
|
|
{
|
|
diff: `diff --git a/unittest.csv b/unittest.csv
|
|
--- a/unittest.csv
|
|
+++ b/unittest.csv
|
|
@@ -1,3 +1,3 @@
|
|
-col1,col2,col3
|
|
+cola,col2,col3
|
|
a,b,c`,
|
|
base: `col1,col2,col3
|
|
a,b,c`,
|
|
head: `cola,col2,col3
|
|
a,b,c`,
|
|
cells: [][]TableDiffCellType{
|
|
{TableDiffCellDel, TableDiffCellAdd, TableDiffCellUnchanged, TableDiffCellUnchanged},
|
|
{TableDiffCellDel, TableDiffCellAdd, TableDiffCellUnchanged, TableDiffCellUnchanged},
|
|
},
|
|
},
|
|
// case 6 - inserts a column after first, deletes last column
|
|
{
|
|
diff: `diff --git a/unittest.csv b/unittest.csv
|
|
--- a/unittest.csv
|
|
+++ b/unittest.csv
|
|
@@ -1,2 +1,2 @@
|
|
-col1,col2,col3
|
|
-a,b,c
|
|
+col1,col1a,col2
|
|
+a,d,b`,
|
|
base: `col1,col2,col3
|
|
a,b,c`,
|
|
head: `col1,col1a,col2
|
|
a,d,b`,
|
|
cells: [][]TableDiffCellType{
|
|
{TableDiffCellUnchanged, TableDiffCellAdd, TableDiffCellDel, TableDiffCellMovedUnchanged},
|
|
{TableDiffCellUnchanged, TableDiffCellAdd, TableDiffCellDel, TableDiffCellMovedUnchanged},
|
|
},
|
|
},
|
|
// case 7 - deletes first column, inserts column after last
|
|
{
|
|
diff: `diff --git a/unittest.csv b/unittest.csv
|
|
--- a/unittest.csv
|
|
+++ b/unittest.csv
|
|
@@ -1,2 +1,2 @@
|
|
-col1,col2,col3
|
|
-a,b,c
|
|
+col2,col3,col4
|
|
+b,c,d`,
|
|
base: `col1,col2,col3
|
|
a,b,c`,
|
|
head: `col2,col3,col4
|
|
b,c,d`,
|
|
cells: [][]TableDiffCellType{
|
|
{TableDiffCellDel, TableDiffCellUnchanged, TableDiffCellUnchanged, TableDiffCellAdd},
|
|
{TableDiffCellDel, TableDiffCellUnchanged, TableDiffCellUnchanged, TableDiffCellAdd},
|
|
},
|
|
},
|
|
// case 8 - two columns deleted, 2 added
|
|
{
|
|
diff: `diff --git a/unittest.csv b/unittest.csv
|
|
--- a/unittest.csv
|
|
+++ b/unittest.csv
|
|
@@ -1,2 +1,2 @@
|
|
-col1,col2,col
|
|
-a,b,c
|
|
+col3,col4,col5
|
|
+c,d,e`,
|
|
base: `col1,col2,col3
|
|
a,b,c`,
|
|
head: `col3,col4,col5
|
|
c,d,e`,
|
|
cells: [][]TableDiffCellType{
|
|
{TableDiffCellDel, TableDiffCellMovedUnchanged, TableDiffCellDel, TableDiffCellAdd, TableDiffCellAdd},
|
|
{TableDiffCellDel, TableDiffCellMovedUnchanged, TableDiffCellDel, TableDiffCellAdd, TableDiffCellAdd},
|
|
},
|
|
},
|
|
}
|
|
|
|
for n, c := range cases {
|
|
diff, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, strings.NewReader(c.diff))
|
|
if err != nil {
|
|
t.Errorf("ParsePatch failed: %s", err)
|
|
}
|
|
|
|
var baseReader *csv.Reader
|
|
if len(c.base) > 0 {
|
|
baseReader, err = csv_module.CreateReaderAndDetermineDelimiter(nil, strings.NewReader(c.base))
|
|
if err != nil {
|
|
t.Errorf("CreateReaderAndDetermineDelimiter failed: %s", err)
|
|
}
|
|
}
|
|
var headReader *csv.Reader
|
|
if len(c.head) > 0 {
|
|
headReader, err = csv_module.CreateReaderAndDetermineDelimiter(nil, strings.NewReader(c.head))
|
|
if err != nil {
|
|
t.Errorf("CreateReaderAndDetermineDelimiter failed: %s", err)
|
|
}
|
|
}
|
|
|
|
result, err := CreateCsvDiff(diff.Files[0], baseReader, headReader)
|
|
require.NoError(t, err)
|
|
assert.Len(t, result, 1, "case %d: should be one section", n)
|
|
|
|
section := result[0]
|
|
assert.Len(t, section.Rows, len(c.cells), "case %d: should be %d rows", n, len(c.cells))
|
|
|
|
for i, row := range section.Rows {
|
|
assert.Len(t, row.Cells, len(c.cells[i]), "case %d: row %d should have %d cells", n, i, len(c.cells[i]))
|
|
for j, cell := range row.Cells {
|
|
assert.Equal(t, c.cells[i][j], cell.Type, "case %d: row %d cell %d should be equal", n, i, j)
|
|
}
|
|
}
|
|
}
|
|
}
|