Merge pull request 'feat: Improve diffs generated by Forgejo' (#5110) from fnetx/better-diffs into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/5110
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

.forgejo/workflows/e2e.yml

@@ -22,7 +22,7 @@ jobs:
           go-version-file: "go.mod"
       - run: |
           apt-get -qq update
-          apt-get -qq install -q sudo
+          apt-get -qq install -q sudo git git-lfs
           sed -i -e 's/%sudo.*/%sudo   ALL=(ALL:ALL) NOPASSWD:ALL/' /etc/sudoers
           git config --add safe.directory '*'
           adduser --quiet --comment forgejo --disabled-password forgejo
@@ -30,6 +30,8 @@ jobs:
           chown -R forgejo:forgejo .
       - run: |
           su forgejo -c 'make deps-frontend frontend deps-backend'
+      - run: |
+          su forgejo -c 'make backend'
       - run: |
           su forgejo -c 'make generate test-e2e-sqlite'
         timeout-minutes: 40

routers/web/repo/issue_content_history.go

@@ -154,6 +154,7 @@ func GetContentHistoryDetail(ctx *context.Context) {
 	dmp := diffmatchpatch.New()
 	// `checklines=false` makes better diff result
 	diff := dmp.DiffMain(prevHistoryContentText, history.ContentText, false)
+	diff = dmp.DiffCleanupSemantic(diff)
 	diff = dmp.DiffCleanupEfficiency(diff)
 
 	// use chroma to render the diff html

services/gitdiff/highlightdiff.go

@@ -97,6 +97,7 @@ func (hcd *HighlightCodeDiff) diffWithHighlight(filename, language, codeA, codeB
 	convertedCodeB := hcd.ConvertToPlaceholders(string(highlightCodeB))
 
 	diffs := diffMatchPatch.DiffMain(convertedCodeA, convertedCodeB, true)
+	diffs = diffMatchPatch.DiffCleanupSemantic(diffs)
 	diffs = diffMatchPatch.DiffCleanupEfficiency(diffs)
 
 	for i := range diffs {

tests/e2e/.eslintrc.yaml

@@ -14,6 +14,7 @@ env:
 
 rules:
   playwright/no-conditional-in-test: [0]
+  playwright/no-conditional-expect: [0]
   playwright/no-networkidle: [0]
   playwright/no-skipped-test: [2, {allowConditional: true}]
   playwright/prefer-comparison-matcher: [2]

tests/e2e/debugserver_test.go

@@ -24,7 +24,8 @@ func TestDebugserver(t *testing.T) {
 	done := make(chan os.Signal, 1)
 	signal.Notify(done, syscall.SIGINT, syscall.SIGTERM)
 
-	onGiteaRun(t, func(*testing.T, *url.URL) {
+	onForgejoRun(t, func(*testing.T, *url.URL) {
+		defer DeclareGitRepos(t)()
 		fmt.Println(setting.AppURL)
 		<-done
 	})

tests/e2e/declare_repos_test.go

@@ -0,0 +1,83 @@
+// Copyright 2024 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package e2e
+
+import (
+	"fmt"
+	"strconv"
+	"strings"
+	"testing"
+	"time"
+
+	unit_model "code.gitea.io/gitea/models/unit"
+	"code.gitea.io/gitea/models/unittest"
+	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/git"
+	files_service "code.gitea.io/gitea/services/repository/files"
+	"code.gitea.io/gitea/tests"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type FileChanges [][]string
+
+// put your Git repo declarations in here
+// feel free to amend the helper function below or use the raw variant directly
+func DeclareGitRepos(t *testing.T) func() {
+	var cleanupFunctions []func()
+	cleanupFunctions = append(cleanupFunctions, newRepo(t, 2, "diff-test", FileChanges{
+		{"testfile", "hello", "hallo", "hola", "native", "ubuntu-latest", "- runs-on: ubuntu-latest", "- runs-on: debian-latest"},
+	}))
+
+	return func() {
+		for _, cleanup := range cleanupFunctions {
+			cleanup()
+		}
+	}
+}
+
+func newRepo(t *testing.T, userID int64, repoName string, fileChanges FileChanges) func() {
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: userID})
+	somerepo, _, cleanupFunc := tests.CreateDeclarativeRepo(t, user, repoName,
+		[]unit_model.Type{unit_model.TypeCode, unit_model.TypeIssues}, nil,
+		nil,
+	)
+
+	for _, file := range fileChanges {
+		changeLen := len(file)
+		for i := 1; i < changeLen; i++ {
+			operation := "create"
+			if i != 1 {
+				operation = "update"
+			}
+			resp, err := files_service.ChangeRepoFiles(git.DefaultContext, somerepo, user, &files_service.ChangeRepoFilesOptions{
+				Files: []*files_service.ChangeRepoFile{{
+					Operation:     operation,
+					TreePath:      file[0],
+					ContentReader: strings.NewReader(file[i]),
+				}},
+				Message:   fmt.Sprintf("Patch: %s-%s", file[0], strconv.Itoa(i)),
+				OldBranch: "main",
+				NewBranch: "main",
+				Author: &files_service.IdentityOptions{
+					Name:  user.Name,
+					Email: user.Email,
+				},
+				Committer: &files_service.IdentityOptions{
+					Name:  user.Name,
+					Email: user.Email,
+				},
+				Dates: &files_service.CommitDateOptions{
+					Author:    time.Now(),
+					Committer: time.Now(),
+				},
+			})
+			require.NoError(t, err)
+			assert.NotEmpty(t, resp)
+		}
+	}
+
+	return cleanupFunc
+}

tests/e2e/e2e_test.go

@@ -37,7 +37,7 @@ func TestMain(m *testing.M) {
 	graceful.InitManager(managerCtx)
 	defer cancel()
 
-	tests.InitTest(false)
+	tests.InitTest(true)
 	testE2eWebRoutes = routers.NormalRoutes()
 
 	os.Unsetenv("GIT_AUTHOR_NAME")
@@ -102,7 +102,8 @@ func TestE2e(t *testing.T) {
 
 		t.Run(testname, func(t *testing.T) {
 			// Default 2 minute timeout
-			onGiteaRun(t, func(*testing.T, *url.URL) {
+			onForgejoRun(t, func(*testing.T, *url.URL) {
+				defer DeclareGitRepos(t)()
 				thisTest := runArgs
 				thisTest = append(thisTest, path)
 				cmd := exec.Command(runArgs[0], thisTest...)

tests/e2e/repo-code.test.e2e.js

@@ -51,3 +51,29 @@ test('Line Range Selection', async ({browser}, workerInfo) => {
   await page.goto(`${filePath}#L1-L100`);
   await assertSelectedLines(page, ['1', '2', '3']);
 });
+
+test('Readable diff', async ({page}, workerInfo) => {
+  // remove this when the test covers more (e.g. accessibility scans or interactive behaviour)
+  test.skip(workerInfo.project.name !== 'firefox', 'This currently only tests the backend-generated HTML code and it is not necessary to test with multiple browsers.');
+  const expectedDiffs = [
+    {id: 'testfile-2', removed: 'e', added: 'a'},
+    {id: 'testfile-3', removed: 'allo', added: 'ola'},
+    {id: 'testfile-4', removed: 'hola', added: 'native'},
+    {id: 'testfile-5', removed: 'native', added: 'ubuntu-latest'},
+    {id: 'testfile-6', added: '- runs-on: '},
+    {id: 'testfile-7', removed: 'ubuntu', added: 'debian'},
+  ];
+  for (const thisDiff of expectedDiffs) {
+    const response = await page.goto('/user2/diff-test/commits/branch/main');
+    await expect(response?.status()).toBe(200); // Status OK
+    await page.getByText(`Patch: ${thisDiff.id}`).click();
+    if (thisDiff.removed) {
+      await expect(page.getByText(thisDiff.removed, {exact: true})).toHaveClass(/removed-code/);
+      await expect(page.getByText(thisDiff.removed, {exact: true})).toHaveCSS('background-color', 'rgb(252, 165, 165)');
+    }
+    if (thisDiff.added) {
+      await expect(page.getByText(thisDiff.added, {exact: true})).toHaveClass(/added-code/);
+      await expect(page.getByText(thisDiff.added, {exact: true})).toHaveCSS('background-color', 'rgb(134, 239, 172)');
+    }
+  }
+});

tests/e2e/utils_e2e_test.go

@@ -17,7 +17,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func onGiteaRunTB(t testing.TB, callback func(testing.TB, *url.URL), prepare ...bool) {
+func onForgejoRunTB(t testing.TB, callback func(testing.TB, *url.URL), prepare ...bool) {
 	if len(prepare) == 0 || prepare[0] {
 		defer tests.PrepareTestEnv(t, 1)()
 	}
@@ -49,8 +49,8 @@ func onGiteaRunTB(t testing.TB, callback func(testing.TB, *url.URL), prepare ...
 	callback(t, u)
 }
 
-func onGiteaRun(t *testing.T, callback func(*testing.T, *url.URL), prepare ...bool) {
-	onGiteaRunTB(t, func(t testing.TB, u *url.URL) {
+func onForgejoRun(t *testing.T, callback func(*testing.T, *url.URL), prepare ...bool) {
+	onForgejoRunTB(t, func(t testing.TB, u *url.URL) {
 		callback(t.(*testing.T), u)
 	}, prepare...)
 }