diff options
Diffstat (limited to 'services/gitdiff')
19 files changed, 1111 insertions, 479 deletions
diff --git a/services/gitdiff/csv.go b/services/gitdiff/csv.go index 8db73c56a3..c10ee14490 100644 --- a/services/gitdiff/csv.go +++ b/services/gitdiff/csv.go @@ -134,7 +134,7 @@ func createCsvDiffSingle(reader *csv.Reader, celltype TableDiffCellType) ([]*Tab return nil, err } cells := make([]*TableDiffCell, len(row)) - for j := 0; j < len(row); j++ { + for j := range row { if celltype == TableDiffCellDel { cells[j] = &TableDiffCell{LeftCell: row[j], Type: celltype} } else { @@ -365,11 +365,11 @@ func getColumnMapping(baseCSVReader, headCSVReader *csvReader) ([]int, []int) { } // Loops through the baseRow and see if there is a match in the head row - for i := 0; i < len(baseRow); i++ { + for i := range baseRow { base2HeadColMap[i] = unmappedColumn baseCell, err := getCell(baseRow, i) if err == nil { - for j := 0; j < len(headRow); j++ { + for j := range headRow { if head2BaseColMap[j] == -1 { headCell, err := getCell(headRow, j) if err == nil && baseCell == headCell { @@ -390,7 +390,7 @@ func getColumnMapping(baseCSVReader, headCSVReader *csvReader) ([]int, []int) { // tryMapColumnsByContent tries to map missing columns by the content of the first lines. func tryMapColumnsByContent(baseCSVReader *csvReader, base2HeadColMap []int, headCSVReader *csvReader, head2BaseColMap []int) { - for i := 0; i < len(base2HeadColMap); i++ { + for i := range base2HeadColMap { headStart := 0 for base2HeadColMap[i] == unmappedColumn && headStart < len(head2BaseColMap) { if head2BaseColMap[headStart] == unmappedColumn { @@ -424,7 +424,7 @@ func getCell(row []string, column int) (string, error) { // countUnmappedColumns returns the count of unmapped columns. func countUnmappedColumns(mapping []int) int { count := 0 - for i := 0; i < len(mapping); i++ { + for i := range mapping { if mapping[i] == unmappedColumn { count++ } diff --git a/services/gitdiff/git_diff_tree.go b/services/gitdiff/git_diff_tree.go new file mode 100644 index 0000000000..ed94bfbfe4 --- /dev/null +++ b/services/gitdiff/git_diff_tree.go @@ -0,0 +1,250 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package gitdiff + +import ( + "bufio" + "context" + "errors" + "fmt" + "io" + "strconv" + "strings" + + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/log" +) + +type DiffTree struct { + Files []*DiffTreeRecord +} + +type DiffTreeRecord struct { + // Status is one of 'added', 'deleted', 'modified', 'renamed', 'copied', 'typechanged', 'unmerged', 'unknown' + Status string + + // For renames and copies, the percentage of similarity between the source and target of the move/rename. + Score uint8 + + HeadPath string + BasePath string + HeadMode git.EntryMode + BaseMode git.EntryMode + HeadBlobID string + BaseBlobID string +} + +// GetDiffTree returns the list of path of the files that have changed between the two commits. +// If useMergeBase is true, the diff will be calculated using the merge base of the two commits. +// This is the same behavior as using a three-dot diff in git diff. +func GetDiffTree(ctx context.Context, gitRepo *git.Repository, useMergeBase bool, baseSha, headSha string) (*DiffTree, error) { + gitDiffTreeRecords, err := runGitDiffTree(ctx, gitRepo, useMergeBase, baseSha, headSha) + if err != nil { + return nil, err + } + + return &DiffTree{ + Files: gitDiffTreeRecords, + }, nil +} + +func runGitDiffTree(ctx context.Context, gitRepo *git.Repository, useMergeBase bool, baseSha, headSha string) ([]*DiffTreeRecord, error) { + useMergeBase, baseCommitID, headCommitID, err := validateGitDiffTreeArguments(gitRepo, useMergeBase, baseSha, headSha) + if err != nil { + return nil, err + } + + cmd := git.NewCommand("diff-tree", "--raw", "-r", "--find-renames", "--root") + if useMergeBase { + cmd.AddArguments("--merge-base") + } + cmd.AddDynamicArguments(baseCommitID, headCommitID) + stdout, _, runErr := cmd.RunStdString(ctx, &git.RunOpts{Dir: gitRepo.Path}) + if runErr != nil { + log.Warn("git diff-tree: %v", runErr) + return nil, runErr + } + + return parseGitDiffTree(strings.NewReader(stdout)) +} + +func validateGitDiffTreeArguments(gitRepo *git.Repository, useMergeBase bool, baseSha, headSha string) (shouldUseMergeBase bool, resolvedBaseSha, resolvedHeadSha string, err error) { + // if the head is empty its an error + if headSha == "" { + return false, "", "", errors.New("headSha is empty") + } + + // if the head commit doesn't exist its and error + headCommit, err := gitRepo.GetCommit(headSha) + if err != nil { + return false, "", "", fmt.Errorf("failed to get commit headSha: %v", err) + } + headCommitID := headCommit.ID.String() + + // if the base is empty we should use the parent of the head commit + if baseSha == "" { + // if the headCommit has no parent we should use an empty commit + // this can happen when we are generating a diff against an orphaned commit + if headCommit.ParentCount() == 0 { + objectFormat, err := gitRepo.GetObjectFormat() + if err != nil { + return false, "", "", err + } + + // We set use merge base to false because we have no base commit + return false, objectFormat.EmptyTree().String(), headCommitID, nil + } + + baseCommit, err := headCommit.Parent(0) + if err != nil { + return false, "", "", fmt.Errorf("baseSha is '', attempted to use parent of commit %s, got error: %v", headCommit.ID.String(), err) + } + return useMergeBase, baseCommit.ID.String(), headCommitID, nil + } + + // try and get the base commit + baseCommit, err := gitRepo.GetCommit(baseSha) + // propagate the error if we couldn't get the base commit + if err != nil { + return useMergeBase, "", "", fmt.Errorf("failed to get base commit %s: %v", baseSha, err) + } + + return useMergeBase, baseCommit.ID.String(), headCommit.ID.String(), nil +} + +func parseGitDiffTree(gitOutput io.Reader) ([]*DiffTreeRecord, error) { + /* + The output of `git diff-tree --raw -r --find-renames` is of the form: + + :<old_mode> <new_mode> <old_sha> <new_sha> <status>\t<path> + + or for renames: + + :<old_mode> <new_mode> <old_sha> <new_sha> <status>\t<old_path>\t<new_path> + + See: <https://git-scm.com/docs/git-diff-tree#_raw_output_format> for more details + */ + results := make([]*DiffTreeRecord, 0) + + lines := bufio.NewScanner(gitOutput) + for lines.Scan() { + line := lines.Text() + + if len(line) == 0 { + continue + } + + record, err := parseGitDiffTreeLine(line) + if err != nil { + return nil, err + } + + results = append(results, record) + } + + if err := lines.Err(); err != nil { + return nil, err + } + + return results, nil +} + +func parseGitDiffTreeLine(line string) (*DiffTreeRecord, error) { + line = strings.TrimPrefix(line, ":") + splitSections := strings.SplitN(line, "\t", 2) + if len(splitSections) < 2 { + return nil, fmt.Errorf("unparsable output for diff-tree --raw: `%s`)", line) + } + + fields := strings.Fields(splitSections[0]) + if len(fields) < 5 { + return nil, fmt.Errorf("unparsable output for diff-tree --raw: `%s`, expected 5 space delimited values got %d)", line, len(fields)) + } + + baseMode, err := git.ParseEntryMode(fields[0]) + if err != nil { + return nil, err + } + + headMode, err := git.ParseEntryMode(fields[1]) + if err != nil { + return nil, err + } + + baseBlobID := fields[2] + headBlobID := fields[3] + + status, score, err := statusFromLetter(fields[4]) + if err != nil { + return nil, fmt.Errorf("unparsable output for diff-tree --raw: %s, error: %s", line, err) + } + + filePaths := strings.Split(splitSections[1], "\t") + + var headPath, basePath string + if status == "renamed" { + if len(filePaths) != 2 { + return nil, fmt.Errorf("unparsable output for diff-tree --raw: `%s`, expected 2 paths found %d", line, len(filePaths)) + } + basePath = filePaths[0] + headPath = filePaths[1] + } else { + basePath = filePaths[0] + headPath = filePaths[0] + } + + return &DiffTreeRecord{ + Status: status, + Score: score, + BaseMode: baseMode, + HeadMode: headMode, + BaseBlobID: baseBlobID, + HeadBlobID: headBlobID, + BasePath: basePath, + HeadPath: headPath, + }, nil +} + +func statusFromLetter(rawStatus string) (status string, score uint8, err error) { + if len(rawStatus) < 1 { + return "", 0, errors.New("empty status letter") + } + switch rawStatus[0] { + case 'A': + return "added", 0, nil + case 'D': + return "deleted", 0, nil + case 'M': + return "modified", 0, nil + case 'R': + score, err = tryParseStatusScore(rawStatus) + return "renamed", score, err + case 'C': + score, err = tryParseStatusScore(rawStatus) + return "copied", score, err + case 'T': + return "typechanged", 0, nil + case 'U': + return "unmerged", 0, nil + case 'X': + return "unknown", 0, nil + default: + return "", 0, fmt.Errorf("unknown status letter: '%s'", rawStatus) + } +} + +func tryParseStatusScore(rawStatus string) (uint8, error) { + if len(rawStatus) < 2 { + return 0, errors.New("status score missing") + } + + score, err := strconv.ParseUint(rawStatus[1:], 10, 8) + if err != nil { + return 0, fmt.Errorf("failed to parse status score: %w", err) + } else if score > 100 { + return 0, fmt.Errorf("status score out of range: %d", score) + } + + return uint8(score), nil +} diff --git a/services/gitdiff/git_diff_tree_test.go b/services/gitdiff/git_diff_tree_test.go new file mode 100644 index 0000000000..313d279e95 --- /dev/null +++ b/services/gitdiff/git_diff_tree_test.go @@ -0,0 +1,427 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package gitdiff + +import ( + "strings" + "testing" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/modules/git" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGitDiffTree(t *testing.T) { + test := []struct { + Name string + RepoPath string + BaseSha string + HeadSha string + useMergeBase bool + Expected *DiffTree + }{ + { + Name: "happy path", + RepoPath: "../../modules/git/tests/repos/repo5_pulls", + BaseSha: "72866af952e98d02a73003501836074b286a78f6", + HeadSha: "d8e0bbb45f200e67d9a784ce55bd90821af45ebd", + Expected: &DiffTree{ + Files: []*DiffTreeRecord{ + { + Status: "modified", + HeadPath: "LICENSE", + BasePath: "LICENSE", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeBlob, + HeadBlobID: "ee469963e76ae1bb7ee83d7510df2864e6c8c640", + BaseBlobID: "c996f4725be8fc8c1d1c776e58c97ddc5d03b336", + }, + { + Status: "modified", + HeadPath: "README.md", + BasePath: "README.md", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeBlob, + HeadBlobID: "9dfc0a6257d8eff526f0cfaf6a8ea950f55a9dba", + BaseBlobID: "074e590b8e64898b02beef03ece83f962c94f54c", + }, + }, + }, + }, + { + Name: "first commit (no parent)", + RepoPath: "../../modules/git/tests/repos/repo5_pulls", + HeadSha: "72866af952e98d02a73003501836074b286a78f6", + Expected: &DiffTree{ + Files: []*DiffTreeRecord{ + { + Status: "added", + HeadPath: ".gitignore", + BasePath: ".gitignore", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeNoEntry, + HeadBlobID: "f1c181ec9c5c921245027c6b452ecfc1d3626364", + BaseBlobID: "0000000000000000000000000000000000000000", + }, + { + Status: "added", + HeadPath: "LICENSE", + BasePath: "LICENSE", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeNoEntry, + HeadBlobID: "c996f4725be8fc8c1d1c776e58c97ddc5d03b336", + BaseBlobID: "0000000000000000000000000000000000000000", + }, + { + Status: "added", + HeadPath: "README.md", + BasePath: "README.md", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeNoEntry, + HeadBlobID: "074e590b8e64898b02beef03ece83f962c94f54c", + BaseBlobID: "0000000000000000000000000000000000000000", + }, + }, + }, + }, + { + Name: "first commit (no parent), merge base = true", + RepoPath: "../../modules/git/tests/repos/repo5_pulls", + HeadSha: "72866af952e98d02a73003501836074b286a78f6", + useMergeBase: true, + Expected: &DiffTree{ + Files: []*DiffTreeRecord{ + { + Status: "added", + HeadPath: ".gitignore", + BasePath: ".gitignore", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeNoEntry, + HeadBlobID: "f1c181ec9c5c921245027c6b452ecfc1d3626364", + BaseBlobID: "0000000000000000000000000000000000000000", + }, + { + Status: "added", + HeadPath: "LICENSE", + BasePath: "LICENSE", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeNoEntry, + HeadBlobID: "c996f4725be8fc8c1d1c776e58c97ddc5d03b336", + BaseBlobID: "0000000000000000000000000000000000000000", + }, + { + Status: "added", + HeadPath: "README.md", + BasePath: "README.md", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeNoEntry, + HeadBlobID: "074e590b8e64898b02beef03ece83f962c94f54c", + BaseBlobID: "0000000000000000000000000000000000000000", + }, + }, + }, + }, + { + Name: "base and head same", + RepoPath: "../../modules/git/tests/repos/repo5_pulls", + BaseSha: "ed8f4d2fa5b2420706580d191f5dd50c4e491f3f", + HeadSha: "ed8f4d2fa5b2420706580d191f5dd50c4e491f3f", + Expected: &DiffTree{ + Files: []*DiffTreeRecord{}, + }, + }, + { + Name: "useMergeBase false", + RepoPath: "../../modules/git/tests/repos/repo5_pulls", + BaseSha: "ed8f4d2fa5b2420706580d191f5dd50c4e491f3f", + HeadSha: "111cac04bd7d20301964e27a93698aabb5781b80", // this commit can be found on the update-readme branch + useMergeBase: false, + Expected: &DiffTree{ + Files: []*DiffTreeRecord{ + { + Status: "modified", + HeadPath: "LICENSE", + BasePath: "LICENSE", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeBlob, + HeadBlobID: "c996f4725be8fc8c1d1c776e58c97ddc5d03b336", + BaseBlobID: "ed5119b3c1f45547b6785bc03eac7f87570fa17f", + }, + + { + Status: "modified", + HeadPath: "README.md", + BasePath: "README.md", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeBlob, + HeadBlobID: "fb39771a8865c9a67f2ab9b616c854805664553c", + BaseBlobID: "9dfc0a6257d8eff526f0cfaf6a8ea950f55a9dba", + }, + }, + }, + }, + { + Name: "useMergeBase true", + RepoPath: "../../modules/git/tests/repos/repo5_pulls", + BaseSha: "ed8f4d2fa5b2420706580d191f5dd50c4e491f3f", + HeadSha: "111cac04bd7d20301964e27a93698aabb5781b80", // this commit can be found on the update-readme branch + useMergeBase: true, + Expected: &DiffTree{ + Files: []*DiffTreeRecord{ + { + Status: "modified", + HeadPath: "README.md", + BasePath: "README.md", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeBlob, + HeadBlobID: "fb39771a8865c9a67f2ab9b616c854805664553c", + BaseBlobID: "9dfc0a6257d8eff526f0cfaf6a8ea950f55a9dba", + }, + }, + }, + }, + { + Name: "no base set", + RepoPath: "../../modules/git/tests/repos/repo5_pulls", + HeadSha: "d8e0bbb45f200e67d9a784ce55bd90821af45ebd", // this commit can be found on the update-readme branch + useMergeBase: false, + Expected: &DiffTree{ + Files: []*DiffTreeRecord{ + { + Status: "modified", + HeadPath: "LICENSE", + BasePath: "LICENSE", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeBlob, + HeadBlobID: "ee469963e76ae1bb7ee83d7510df2864e6c8c640", + BaseBlobID: "ed5119b3c1f45547b6785bc03eac7f87570fa17f", + }, + }, + }, + }, + } + + for _, tt := range test { + t.Run(tt.Name, func(t *testing.T) { + gitRepo, err := git.OpenRepository(git.DefaultContext, tt.RepoPath) + assert.NoError(t, err) + defer gitRepo.Close() + + diffPaths, err := GetDiffTree(db.DefaultContext, gitRepo, tt.useMergeBase, tt.BaseSha, tt.HeadSha) + require.NoError(t, err) + + assert.Equal(t, tt.Expected, diffPaths) + }) + } +} + +func TestParseGitDiffTree(t *testing.T) { + test := []struct { + Name string + GitOutput string + Expected []*DiffTreeRecord + }{ + { + Name: "file change", + GitOutput: ":100644 100644 64e43d23bcd08db12563a0a4d84309cadb437e1a 5dbc7792b5bb228647cfcc8dfe65fc649119dedc M\tResources/views/curriculum/edit.blade.php", + Expected: []*DiffTreeRecord{ + { + Status: "modified", + HeadPath: "Resources/views/curriculum/edit.blade.php", + BasePath: "Resources/views/curriculum/edit.blade.php", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeBlob, + HeadBlobID: "5dbc7792b5bb228647cfcc8dfe65fc649119dedc", + BaseBlobID: "64e43d23bcd08db12563a0a4d84309cadb437e1a", + }, + }, + }, + { + Name: "file added", + GitOutput: ":000000 100644 0000000000000000000000000000000000000000 0063162fb403db15ceb0517b34ab782e4e58b619 A\tResources/views/class/index.blade.php", + Expected: []*DiffTreeRecord{ + { + Status: "added", + HeadPath: "Resources/views/class/index.blade.php", + BasePath: "Resources/views/class/index.blade.php", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeNoEntry, + HeadBlobID: "0063162fb403db15ceb0517b34ab782e4e58b619", + BaseBlobID: "0000000000000000000000000000000000000000", + }, + }, + }, + { + Name: "file deleted", + GitOutput: ":100644 000000 bac4286303c8c0017ea2f0a48c561ddcc0330a14 0000000000000000000000000000000000000000 D\tResources/views/classes/index.blade.php", + Expected: []*DiffTreeRecord{ + { + Status: "deleted", + HeadPath: "Resources/views/classes/index.blade.php", + BasePath: "Resources/views/classes/index.blade.php", + HeadMode: git.EntryModeNoEntry, + BaseMode: git.EntryModeBlob, + HeadBlobID: "0000000000000000000000000000000000000000", + BaseBlobID: "bac4286303c8c0017ea2f0a48c561ddcc0330a14", + }, + }, + }, + { + Name: "file renamed", + GitOutput: ":100644 100644 c8a055cfb45cd39747292983ad1797ceab40f5b1 97248f79a90aaf81fe7fd74b33c1cb182dd41783 R087\tDatabase/Seeders/AdminDatabaseSeeder.php\tDatabase/Seeders/AcademicDatabaseSeeder.php", + Expected: []*DiffTreeRecord{ + { + Status: "renamed", + Score: 87, + HeadPath: "Database/Seeders/AcademicDatabaseSeeder.php", + BasePath: "Database/Seeders/AdminDatabaseSeeder.php", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeBlob, + HeadBlobID: "97248f79a90aaf81fe7fd74b33c1cb182dd41783", + BaseBlobID: "c8a055cfb45cd39747292983ad1797ceab40f5b1", + }, + }, + }, + { + Name: "no changes", + GitOutput: ``, + Expected: []*DiffTreeRecord{}, + }, + { + Name: "multiple changes", + GitOutput: ":000000 100644 0000000000000000000000000000000000000000 db736b44533a840981f1f17b7029d0f612b69550 A\tHttp/Controllers/ClassController.php\n" + + ":100644 000000 9a4d2344d4d0145db7c91b3f3e123c74367d4ef4 0000000000000000000000000000000000000000 D\tHttp/Controllers/ClassesController.php\n" + + ":100644 100644 f060d6aede65d423f49e7dc248dfa0d8835ef920 b82c8e39a3602dedadb44669956d6eb5b6a7cc86 M\tHttp/Controllers/ProgramDirectorController.php\n", + Expected: []*DiffTreeRecord{ + { + Status: "added", + HeadPath: "Http/Controllers/ClassController.php", + BasePath: "Http/Controllers/ClassController.php", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeNoEntry, + HeadBlobID: "db736b44533a840981f1f17b7029d0f612b69550", + BaseBlobID: "0000000000000000000000000000000000000000", + }, + { + Status: "deleted", + HeadPath: "Http/Controllers/ClassesController.php", + BasePath: "Http/Controllers/ClassesController.php", + HeadMode: git.EntryModeNoEntry, + BaseMode: git.EntryModeBlob, + HeadBlobID: "0000000000000000000000000000000000000000", + BaseBlobID: "9a4d2344d4d0145db7c91b3f3e123c74367d4ef4", + }, + { + Status: "modified", + HeadPath: "Http/Controllers/ProgramDirectorController.php", + BasePath: "Http/Controllers/ProgramDirectorController.php", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeBlob, + HeadBlobID: "b82c8e39a3602dedadb44669956d6eb5b6a7cc86", + BaseBlobID: "f060d6aede65d423f49e7dc248dfa0d8835ef920", + }, + }, + }, + { + Name: "spaces in file path", + GitOutput: ":000000 100644 0000000000000000000000000000000000000000 db736b44533a840981f1f17b7029d0f612b69550 A\tHttp /Controllers/Class Controller.php\n" + + ":100644 000000 9a4d2344d4d0145db7c91b3f3e123c74367d4ef4 0000000000000000000000000000000000000000 D\tHttp/Cont rollers/Classes Controller.php\n" + + ":100644 100644 f060d6aede65d423f49e7dc248dfa0d8835ef920 b82c8e39a3602dedadb44669956d6eb5b6a7cc86 R010\tHttp/Controllers/Program Director Controller.php\tHttp/Cont rollers/ProgramDirectorController.php\n", + Expected: []*DiffTreeRecord{ + { + Status: "added", + HeadPath: "Http /Controllers/Class Controller.php", + BasePath: "Http /Controllers/Class Controller.php", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeNoEntry, + HeadBlobID: "db736b44533a840981f1f17b7029d0f612b69550", + BaseBlobID: "0000000000000000000000000000000000000000", + }, + { + Status: "deleted", + HeadPath: "Http/Cont rollers/Classes Controller.php", + BasePath: "Http/Cont rollers/Classes Controller.php", + HeadMode: git.EntryModeNoEntry, + BaseMode: git.EntryModeBlob, + HeadBlobID: "0000000000000000000000000000000000000000", + BaseBlobID: "9a4d2344d4d0145db7c91b3f3e123c74367d4ef4", + }, + { + Status: "renamed", + Score: 10, + HeadPath: "Http/Cont rollers/ProgramDirectorController.php", + BasePath: "Http/Controllers/Program Director Controller.php", + HeadMode: git.EntryModeBlob, + BaseMode: git.EntryModeBlob, + HeadBlobID: "b82c8e39a3602dedadb44669956d6eb5b6a7cc86", + BaseBlobID: "f060d6aede65d423f49e7dc248dfa0d8835ef920", + }, + }, + }, + { + Name: "file type changed", + GitOutput: ":100644 120000 344e0ca8aa791cc4164fb0ea645f334fd40d00f0 a7c2973de00bfdc6ca51d315f401b5199fe01dc3 T\twebpack.mix.js", + Expected: []*DiffTreeRecord{ + { + Status: "typechanged", + HeadPath: "webpack.mix.js", + BasePath: "webpack.mix.js", + HeadMode: git.EntryModeSymlink, + BaseMode: git.EntryModeBlob, + HeadBlobID: "a7c2973de00bfdc6ca51d315f401b5199fe01dc3", + BaseBlobID: "344e0ca8aa791cc4164fb0ea645f334fd40d00f0", + }, + }, + }, + } + + for _, tt := range test { + t.Run(tt.Name, func(t *testing.T) { + entries, err := parseGitDiffTree(strings.NewReader(tt.GitOutput)) + assert.NoError(t, err) + assert.Equal(t, tt.Expected, entries) + }) + } +} + +func TestGitDiffTreeErrors(t *testing.T) { + test := []struct { + Name string + RepoPath string + BaseSha string + HeadSha string + }{ + { + Name: "head doesn't exist", + RepoPath: "../../modules/git/tests/repos/repo5_pulls", + BaseSha: "f32b0a9dfd09a60f616f29158f772cedd89942d2", + HeadSha: "asdfasdfasdf", + }, + { + Name: "base doesn't exist", + RepoPath: "../../modules/git/tests/repos/repo5_pulls", + BaseSha: "asdfasdfasdf", + HeadSha: "f32b0a9dfd09a60f616f29158f772cedd89942d2", + }, + { + Name: "head not set", + RepoPath: "../../modules/git/tests/repos/repo5_pulls", + BaseSha: "f32b0a9dfd09a60f616f29158f772cedd89942d2", + }, + } + + for _, tt := range test { + t.Run(tt.Name, func(t *testing.T) { + gitRepo, err := git.OpenRepository(git.DefaultContext, tt.RepoPath) + assert.NoError(t, err) + defer gitRepo.Close() + + diffPaths, err := GetDiffTree(db.DefaultContext, gitRepo, true, tt.BaseSha, tt.HeadSha) + assert.Error(t, err) + assert.Nil(t, diffPaths) + }) + } +} diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index f046e59678..9964329876 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -25,12 +25,14 @@ import ( "code.gitea.io/gitea/modules/analyze" "code.gitea.io/gitea/modules/charset" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/git/attribute" "code.gitea.io/gitea/modules/highlight" "code.gitea.io/gitea/modules/lfs" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/translation" + "code.gitea.io/gitea/modules/util" "github.com/sergi/go-diff/diffmatchpatch" stdcharset "golang.org/x/net/html/charset" @@ -75,12 +77,12 @@ const ( // DiffLine represents a line difference in a DiffSection. type DiffLine struct { - LeftIdx int - RightIdx int - Match int + LeftIdx int // line number, 1-based + RightIdx int // line number, 1-based + Match int // the diff matched index. -1: no match. 0: plain and no need to match. >0: for add/del, "Lines" slice index of the other side Type DiffLineType Content string - Comments []*issues_model.Comment + Comments issues_model.CommentList // related PR code comments SectionInfo *DiffLineSectionInfo } @@ -95,9 +97,18 @@ type DiffLineSectionInfo struct { RightHunkSize int } +// DiffHTMLOperation is the HTML version of diffmatchpatch.Diff +type DiffHTMLOperation struct { + Type diffmatchpatch.Operation + HTML template.HTML +} + // BlobExcerptChunkSize represent max lines of excerpt const BlobExcerptChunkSize = 20 +// MaxDiffHighlightEntireFileSize is the maximum file size that will be highlighted with "entire file diff" +const MaxDiffHighlightEntireFileSize = 1 * 1024 * 1024 + // GetType returns the type of DiffLine. func (d *DiffLine) GetType() int { return int(d.Type) @@ -112,8 +123,9 @@ func (d *DiffLine) GetHTMLDiffLineType() string { return "del" case DiffLineSection: return "tag" + default: + return "same" } - return "same" } // CanComment returns whether a line can get commented @@ -192,89 +204,20 @@ func getLineContent(content string, locale translation.Locale) DiffInline { type DiffSection struct { file *DiffFile FileName string - Name string Lines []*DiffLine } -var ( - addedCodePrefix = []byte(`<span class="added-code">`) - removedCodePrefix = []byte(`<span class="removed-code">`) - codeTagSuffix = []byte(`</span>`) -) - -func diffToHTML(lineWrapperTags []string, diffs []diffmatchpatch.Diff, lineType DiffLineType) string { - buf := bytes.NewBuffer(nil) - // restore the line wrapper tags <span class="line"> and <span class="cl">, if necessary - for _, tag := range lineWrapperTags { - buf.WriteString(tag) - } - for _, diff := range diffs { - switch { - case diff.Type == diffmatchpatch.DiffEqual: - buf.WriteString(diff.Text) - case diff.Type == diffmatchpatch.DiffInsert && lineType == DiffLineAdd: - buf.Write(addedCodePrefix) - buf.WriteString(diff.Text) - buf.Write(codeTagSuffix) - case diff.Type == diffmatchpatch.DiffDelete && lineType == DiffLineDel: - buf.Write(removedCodePrefix) - buf.WriteString(diff.Text) - buf.Write(codeTagSuffix) - } - } - for range lineWrapperTags { - buf.WriteString("</span>") - } - return buf.String() -} - -// GetLine gets a specific line by type (add or del) and file line number -func (diffSection *DiffSection) GetLine(lineType DiffLineType, idx int) *DiffLine { - var ( - difference = 0 - addCount = 0 - delCount = 0 - matchDiffLine *DiffLine - ) - -LOOP: - for _, diffLine := range diffSection.Lines { - switch diffLine.Type { - case DiffLineAdd: - addCount++ - case DiffLineDel: - delCount++ - default: - if matchDiffLine != nil { - break LOOP - } - difference = diffLine.RightIdx - diffLine.LeftIdx - addCount = 0 - delCount = 0 - } - - switch lineType { - case DiffLineDel: - if diffLine.RightIdx == 0 && diffLine.LeftIdx == idx-difference { - matchDiffLine = diffLine - } - case DiffLineAdd: - if diffLine.LeftIdx == 0 && diffLine.RightIdx == idx+difference { - matchDiffLine = diffLine - } - } - } - - if addCount == delCount { - return matchDiffLine +func (diffSection *DiffSection) GetLine(idx int) *DiffLine { + if idx <= 0 { + return nil } - return nil + return diffSection.Lines[idx] } -var diffMatchPatch = diffmatchpatch.New() - -func init() { - diffMatchPatch.DiffEditCost = 100 +func defaultDiffMatchPatch() *diffmatchpatch.DiffMatchPatch { + dmp := diffmatchpatch.New() + dmp.DiffEditCost = 100 + return dmp } // DiffInline is a struct that has a content and escape status @@ -283,97 +226,125 @@ type DiffInline struct { Content template.HTML } -// DiffInlineWithUnicodeEscape makes a DiffInline with hidden unicode characters escaped +// DiffInlineWithUnicodeEscape makes a DiffInline with hidden Unicode characters escaped func DiffInlineWithUnicodeEscape(s template.HTML, locale translation.Locale) DiffInline { status, content := charset.EscapeControlHTML(s, locale) return DiffInline{EscapeStatus: status, Content: content} } -// DiffInlineWithHighlightCode makes a DiffInline with code highlight and hidden unicode characters escaped -func DiffInlineWithHighlightCode(fileName, language, code string, locale translation.Locale) DiffInline { - highlighted, _ := highlight.Code(fileName, language, code) - status, content := charset.EscapeControlHTML(highlighted, locale) - return DiffInline{EscapeStatus: status, Content: content} -} - -// GetComputedInlineDiffFor computes inline diff for the given line. -func (diffSection *DiffSection) GetComputedInlineDiffFor(diffLine *DiffLine, locale translation.Locale) DiffInline { +func (diffSection *DiffSection) getLineContentForRender(lineIdx int, diffLine *DiffLine, fileLanguage string, highlightLines map[int]template.HTML) template.HTML { + h, ok := highlightLines[lineIdx-1] + if ok { + return h + } + if diffLine.Content == "" { + return "" + } if setting.Git.DisableDiffHighlight { - return getLineContent(diffLine.Content[1:], locale) + return template.HTML(html.EscapeString(diffLine.Content[1:])) } + h, _ = highlight.Code(diffSection.FileName, fileLanguage, diffLine.Content[1:]) + return h +} - var ( - compareDiffLine *DiffLine - diff1 string - diff2 string - ) - - language := "" +func (diffSection *DiffSection) getDiffLineForRender(diffLineType DiffLineType, leftLine, rightLine *DiffLine, locale translation.Locale) DiffInline { + var fileLanguage string + var highlightedLeftLines, highlightedRightLines map[int]template.HTML + // when a "diff section" is manually prepared by ExcerptBlob, it doesn't have "file" information if diffSection.file != nil { - language = diffSection.file.Language + fileLanguage = diffSection.file.Language + highlightedLeftLines, highlightedRightLines = diffSection.file.highlightedLeftLines, diffSection.file.highlightedRightLines } + var lineHTML template.HTML + hcd := newHighlightCodeDiff() + if diffLineType == DiffLinePlain { + // left and right are the same, no need to do line-level diff + if leftLine != nil { + lineHTML = diffSection.getLineContentForRender(leftLine.LeftIdx, leftLine, fileLanguage, highlightedLeftLines) + } else if rightLine != nil { + lineHTML = diffSection.getLineContentForRender(rightLine.RightIdx, rightLine, fileLanguage, highlightedRightLines) + } + } else { + var diff1, diff2 template.HTML + if leftLine != nil { + diff1 = diffSection.getLineContentForRender(leftLine.LeftIdx, leftLine, fileLanguage, highlightedLeftLines) + } + if rightLine != nil { + diff2 = diffSection.getLineContentForRender(rightLine.RightIdx, rightLine, fileLanguage, highlightedRightLines) + } + if diff1 != "" && diff2 != "" { + // if only some parts of a line are changed, highlight these changed parts as "deleted/added". + lineHTML = hcd.diffLineWithHighlight(diffLineType, diff1, diff2) + } else { + // if left is empty or right is empty (a line is fully deleted or added), then we do not need to diff anymore. + // the tmpl code already adds background colors for these cases. + lineHTML = util.Iif(diffLineType == DiffLineDel, diff1, diff2) + } + } + return DiffInlineWithUnicodeEscape(lineHTML, locale) +} + +// GetComputedInlineDiffFor computes inline diff for the given line. +func (diffSection *DiffSection) GetComputedInlineDiffFor(diffLine *DiffLine, locale translation.Locale) DiffInline { // try to find equivalent diff line. ignore, otherwise switch diffLine.Type { case DiffLineSection: return getLineContent(diffLine.Content[1:], locale) case DiffLineAdd: - compareDiffLine = diffSection.GetLine(DiffLineDel, diffLine.RightIdx) - if compareDiffLine == nil { - return DiffInlineWithHighlightCode(diffSection.FileName, language, diffLine.Content[1:], locale) - } - diff1 = compareDiffLine.Content - diff2 = diffLine.Content + compareDiffLine := diffSection.GetLine(diffLine.Match) + return diffSection.getDiffLineForRender(DiffLineAdd, compareDiffLine, diffLine, locale) case DiffLineDel: - compareDiffLine = diffSection.GetLine(DiffLineAdd, diffLine.LeftIdx) - if compareDiffLine == nil { - return DiffInlineWithHighlightCode(diffSection.FileName, language, diffLine.Content[1:], locale) - } - diff1 = diffLine.Content - diff2 = compareDiffLine.Content - default: - if strings.IndexByte(" +-", diffLine.Content[0]) > -1 { - return DiffInlineWithHighlightCode(diffSection.FileName, language, diffLine.Content[1:], locale) - } - return DiffInlineWithHighlightCode(diffSection.FileName, language, diffLine.Content, locale) + compareDiffLine := diffSection.GetLine(diffLine.Match) + return diffSection.getDiffLineForRender(DiffLineDel, diffLine, compareDiffLine, locale) + default: // Plain + // TODO: there was an "if" check: `if diffLine.Content >strings.IndexByte(" +-", diffLine.Content[0]) > -1 { ... } else { ... }` + // no idea why it needs that check, it seems that the "if" should be always true, so try to simplify the code + return diffSection.getDiffLineForRender(DiffLinePlain, nil, diffLine, locale) } - - hcd := newHighlightCodeDiff() - diffRecord := hcd.diffWithHighlight(diffSection.FileName, language, diff1[1:], diff2[1:]) - // it seems that Gitea doesn't need the line wrapper of Chroma, so do not add them back - // if the line wrappers are still needed in the future, it can be added back by "diffToHTML(hcd.lineWrapperTags. ...)" - diffHTML := diffToHTML(nil, diffRecord, diffLine.Type) - return DiffInlineWithUnicodeEscape(template.HTML(diffHTML), locale) } // DiffFile represents a file diff. type DiffFile struct { - Name string - NameHash string - OldName string - Index int - Addition, Deletion int - Type DiffFileType - IsCreated bool - IsDeleted bool - IsBin bool - IsLFSFile bool - IsRenamed bool - IsAmbiguous bool - Sections []*DiffSection - IsIncomplete bool - IsIncompleteLineTooLong bool - IsProtected bool - IsGenerated bool - IsVendored bool + // only used internally to parse Ambiguous filenames + isAmbiguous bool + + // basic fields (parsed from diff result) + Name string + NameHash string + OldName string + Addition int + Deletion int + Type DiffFileType + Mode string + OldMode string + IsCreated bool + IsDeleted bool + IsBin bool + IsLFSFile bool + IsRenamed bool + IsSubmodule bool + // basic fields but for render purpose only + Sections []*DiffSection + IsIncomplete bool + IsIncompleteLineTooLong bool + + // will be filled by the extra loop in GitDiffForRender + Language string + IsGenerated bool + IsVendored bool + SubmoduleDiffInfo *SubmoduleDiffInfo // IsSubmodule==true, then there must be a SubmoduleDiffInfo + + // will be filled by route handler + IsProtected bool + + // will be filled by SyncUserSpecificDiff IsViewed bool // User specific HasChangedSinceLastReview bool // User specific - Language string - Mode string - OldMode string - IsSubmodule bool // if IsSubmodule==true, then there must be a SubmoduleDiffInfo - SubmoduleDiffInfo *SubmoduleDiffInfo + // for render purpose only, will be filled by the extra loop in GitDiffForRender + highlightedLeftLines map[int]template.HTML + highlightedRightLines map[int]template.HTML } // GetType returns type of diff file. @@ -381,18 +352,30 @@ func (diffFile *DiffFile) GetType() int { return int(diffFile.Type) } -// GetTailSection creates a fake DiffLineSection if the last section is not the end of the file -func (diffFile *DiffFile) GetTailSection(gitRepo *git.Repository, leftCommit, rightCommit *git.Commit) *DiffSection { - if len(diffFile.Sections) == 0 || diffFile.Type != DiffFileChange || diffFile.IsBin || diffFile.IsLFSFile { - return nil - } +type DiffLimitedContent struct { + LeftContent, RightContent *limitByteWriter +} +// GetTailSectionAndLimitedContent creates a fake DiffLineSection if the last section is not the end of the file +func (diffFile *DiffFile) GetTailSectionAndLimitedContent(leftCommit, rightCommit *git.Commit) (_ *DiffSection, diffLimitedContent DiffLimitedContent) { + var leftLineCount, rightLineCount int + diffLimitedContent = DiffLimitedContent{} + if diffFile.IsBin || diffFile.IsLFSFile { + return nil, diffLimitedContent + } + if (diffFile.Type == DiffFileDel || diffFile.Type == DiffFileChange) && leftCommit != nil { + leftLineCount, diffLimitedContent.LeftContent = getCommitFileLineCountAndLimitedContent(leftCommit, diffFile.OldName) + } + if (diffFile.Type == DiffFileAdd || diffFile.Type == DiffFileChange) && rightCommit != nil { + rightLineCount, diffLimitedContent.RightContent = getCommitFileLineCountAndLimitedContent(rightCommit, diffFile.OldName) + } + if len(diffFile.Sections) == 0 || diffFile.Type != DiffFileChange { + return nil, diffLimitedContent + } lastSection := diffFile.Sections[len(diffFile.Sections)-1] lastLine := lastSection.Lines[len(lastSection.Lines)-1] - leftLineCount := getCommitFileLineCount(leftCommit, diffFile.Name) - rightLineCount := getCommitFileLineCount(rightCommit, diffFile.Name) if leftLineCount <= lastLine.LeftIdx || rightLineCount <= lastLine.RightIdx { - return nil + return nil, diffLimitedContent } tailDiffLine := &DiffLine{ Type: DiffLineSection, @@ -406,7 +389,7 @@ func (diffFile *DiffFile) GetTailSection(gitRepo *git.Repository, leftCommit, ri }, } tailSection := &DiffSection{FileName: diffFile.Name, Lines: []*DiffLine{tailDiffLine}} - return tailSection + return tailSection, diffLimitedContent } // GetDiffFileName returns the name of the diff file, or its old name in case it was deleted @@ -438,26 +421,37 @@ func (diffFile *DiffFile) ModeTranslationKey(mode string) string { } } -func getCommitFileLineCount(commit *git.Commit, filePath string) int { +type limitByteWriter struct { + buf bytes.Buffer + limit int +} + +func (l *limitByteWriter) Write(p []byte) (n int, err error) { + if l.buf.Len()+len(p) > l.limit { + p = p[:l.limit-l.buf.Len()] + } + return l.buf.Write(p) +} + +func getCommitFileLineCountAndLimitedContent(commit *git.Commit, filePath string) (lineCount int, limitWriter *limitByteWriter) { blob, err := commit.GetBlobByPath(filePath) if err != nil { - return 0 + return 0, nil } - lineCount, err := blob.GetBlobLineCount() + w := &limitByteWriter{limit: MaxDiffHighlightEntireFileSize + 1} + lineCount, err = blob.GetBlobLineCount(w) if err != nil { - return 0 + return 0, nil } - return lineCount + return lineCount, w } // Diff represents a difference between two git trees. type Diff struct { - Start, End string - NumFiles int - TotalAddition, TotalDeletion int - Files []*DiffFile - IsIncomplete bool - NumViewedFiles int // user-specific + Start, End string + Files []*DiffFile + IsIncomplete bool + NumViewedFiles int // user-specific } // LoadComments loads comments into each line @@ -501,10 +495,7 @@ func ParsePatch(ctx context.Context, maxLines, maxLineCharacters, maxFiles int, // OK let's set a reasonable buffer size. // This should be at least the size of maxLineCharacters or 4096 whichever is larger. - readerSize := maxLineCharacters - if readerSize < 4096 { - readerSize = 4096 - } + readerSize := max(maxLineCharacters, 4096) input := bufio.NewReaderSize(reader, readerSize) line, err := input.ReadString('\n') @@ -528,13 +519,13 @@ parsingLoop: } if maxFiles > -1 && len(diff.Files) >= maxFiles { - lastFile := createDiffFile(diff, line) + lastFile := createDiffFile(line) diff.End = lastFile.Name diff.IsIncomplete = true break parsingLoop } - curFile = createDiffFile(diff, line) + curFile = createDiffFile(line) if skipping { if curFile.Name != skipToFile { line, err = skipToNextDiffHead(input) @@ -617,28 +608,28 @@ parsingLoop: case strings.HasPrefix(line, "rename from "): curFile.IsRenamed = true curFile.Type = DiffFileRename - if curFile.IsAmbiguous { + if curFile.isAmbiguous { curFile.OldName = prepareValue(line, "rename from ") } case strings.HasPrefix(line, "rename to "): curFile.IsRenamed = true curFile.Type = DiffFileRename - if curFile.IsAmbiguous { + if curFile.isAmbiguous { curFile.Name = prepareValue(line, "rename to ") - curFile.IsAmbiguous = false + curFile.isAmbiguous = false } case strings.HasPrefix(line, "copy from "): curFile.IsRenamed = true curFile.Type = DiffFileCopy - if curFile.IsAmbiguous { + if curFile.isAmbiguous { curFile.OldName = prepareValue(line, "copy from ") } case strings.HasPrefix(line, "copy to "): curFile.IsRenamed = true curFile.Type = DiffFileCopy - if curFile.IsAmbiguous { + if curFile.isAmbiguous { curFile.Name = prepareValue(line, "copy to ") - curFile.IsAmbiguous = false + curFile.isAmbiguous = false } case strings.HasPrefix(line, "new file"): curFile.Type = DiffFileAdd @@ -665,7 +656,7 @@ parsingLoop: curFile.IsBin = true case strings.HasPrefix(line, "--- "): // Handle ambiguous filenames - if curFile.IsAmbiguous { + if curFile.isAmbiguous { // The shortest string that can end up here is: // "--- a\t\n" without the quotes. // This line has a len() of 7 but doesn't contain a oldName. @@ -683,7 +674,7 @@ parsingLoop: // Otherwise do nothing with this line case strings.HasPrefix(line, "+++ "): // Handle ambiguous filenames - if curFile.IsAmbiguous { + if curFile.isAmbiguous { if len(line) > 6 && line[4] == 'b' { curFile.Name = line[6 : len(line)-1] if line[len(line)-2] == '\t' { @@ -695,12 +686,10 @@ parsingLoop: } else { curFile.Name = curFile.OldName } - curFile.IsAmbiguous = false + curFile.isAmbiguous = false } // Otherwise do nothing with this line, but now switch to parsing hunks lineBytes, isFragment, err := parseHunks(ctx, curFile, maxLines, maxLineCharacters, input) - diff.TotalAddition += curFile.Addition - diff.TotalDeletion += curFile.Deletion if err != nil { if err != io.EOF { return diff, err @@ -773,7 +762,6 @@ parsingLoop: } } - diff.NumFiles = len(diff.Files) return diff, nil } @@ -1011,7 +999,7 @@ func parseHunks(ctx context.Context, curFile *DiffFile, maxLines, maxLineCharact } } -func createDiffFile(diff *Diff, line string) *DiffFile { +func createDiffFile(line string) *DiffFile { // The a/ and b/ filenames are the same unless rename/copy is involved. // Especially, even for a creation or a deletion, /dev/null is not used // in place of the a/ or b/ filenames. @@ -1022,12 +1010,11 @@ func createDiffFile(diff *Diff, line string) *DiffFile { // // Path names are quoted if necessary. // - // This means that you should always be able to determine the file name even when there + // This means that you should always be able to determine the file name even when // there is potential ambiguity... // // but we can be simpler with our heuristics by just forcing git to prefix things nicely curFile := &DiffFile{ - Index: len(diff.Files) + 1, Type: DiffFileChange, Sections: make([]*DiffSection, 0, 10), } @@ -1039,7 +1026,7 @@ func createDiffFile(diff *Diff, line string) *DiffFile { curFile.OldName, oldNameAmbiguity = readFileName(rd) curFile.Name, newNameAmbiguity = readFileName(rd) if oldNameAmbiguity && newNameAmbiguity { - curFile.IsAmbiguous = true + curFile.isAmbiguous = true // OK we should bet that the oldName and the newName are the same if they can be made to be same // So we need to start again ... if (len(line)-len(cmdDiffHead)-1)%2 == 0 { @@ -1104,55 +1091,48 @@ type DiffOptions struct { MaxFiles int WhitespaceBehavior git.TrustedCmdArgs DirectComparison bool - FileOnly bool } -// GetDiff builds a Diff between two commits of a repository. +func guessBeforeCommitForDiff(gitRepo *git.Repository, beforeCommitID string, afterCommit *git.Commit) (actualBeforeCommit *git.Commit, actualBeforeCommitID git.ObjectID, err error) { + commitObjectFormat := afterCommit.ID.Type() + isBeforeCommitIDEmpty := beforeCommitID == "" || beforeCommitID == commitObjectFormat.EmptyObjectID().String() + + if isBeforeCommitIDEmpty && afterCommit.ParentCount() == 0 { + actualBeforeCommitID = commitObjectFormat.EmptyTree() + } else { + if isBeforeCommitIDEmpty { + actualBeforeCommit, err = afterCommit.Parent(0) + } else { + actualBeforeCommit, err = gitRepo.GetCommit(beforeCommitID) + } + if err != nil { + return nil, nil, err + } + actualBeforeCommitID = actualBeforeCommit.ID + } + return actualBeforeCommit, actualBeforeCommitID, nil +} + +// getDiffBasic builds a Diff between two commits of a repository. // Passing the empty string as beforeCommitID returns a diff from the parent commit. // The whitespaceBehavior is either an empty string or a git flag -func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) { +// Returned beforeCommit could be nil if the afterCommit doesn't have parent commit +func getDiffBasic(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (_ *Diff, beforeCommit, afterCommit *git.Commit, err error) { repoPath := gitRepo.Path - var beforeCommit *git.Commit - commit, err := gitRepo.GetCommit(opts.AfterCommitID) + afterCommit, err = gitRepo.GetCommit(opts.AfterCommitID) if err != nil { - return nil, err + return nil, nil, nil, err } - cmdCtx, cmdCancel := context.WithCancel(ctx) - defer cmdCancel() - - cmdDiff := git.NewCommand(cmdCtx) - objectFormat, err := gitRepo.GetObjectFormat() + beforeCommit, beforeCommitID, err := guessBeforeCommitForDiff(gitRepo, opts.BeforeCommitID, afterCommit) if err != nil { - return nil, err + return nil, nil, nil, err } - if (len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == objectFormat.EmptyObjectID().String()) && commit.ParentCount() == 0 { - cmdDiff.AddArguments("diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M"). - AddArguments(opts.WhitespaceBehavior...). - AddDynamicArguments(objectFormat.EmptyTree().String()). - AddDynamicArguments(opts.AfterCommitID) - } else { - actualBeforeCommitID := opts.BeforeCommitID - if len(actualBeforeCommitID) == 0 { - parentCommit, err := commit.Parent(0) - if err != nil { - return nil, err - } - actualBeforeCommitID = parentCommit.ID.String() - } - - cmdDiff.AddArguments("diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M"). - AddArguments(opts.WhitespaceBehavior...). - AddDynamicArguments(actualBeforeCommitID, opts.AfterCommitID) - opts.BeforeCommitID = actualBeforeCommitID - - beforeCommit, err = gitRepo.GetCommit(opts.BeforeCommitID) - if err != nil { - return nil, err - } - } + cmdDiff := git.NewCommand(). + AddArguments("diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M"). + AddArguments(opts.WhitespaceBehavior...) // In git 2.31, git diff learned --skip-to which we can use to shortcut skip to file // so if we are using at least this version of git we don't have to tell ParsePatch to do @@ -1163,8 +1143,12 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi parsePatchSkipToFile = "" } + cmdDiff.AddDynamicArguments(beforeCommitID.String(), opts.AfterCommitID) cmdDiff.AddDashesAndList(files...) + cmdCtx, cmdCancel := context.WithCancel(ctx) + defer cmdCancel() + reader, writer := io.Pipe() defer func() { _ = reader.Close() @@ -1173,7 +1157,7 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi go func() { stderr := &bytes.Buffer{} - if err := cmdDiff.Run(&git.RunOpts{ + if err := cmdDiff.Run(cmdCtx, &git.RunOpts{ Timeout: time.Duration(setting.Git.Timeout.Default) * time.Second, Dir: repoPath, Stdout: writer, @@ -1189,32 +1173,44 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi // Ensure the git process is killed if it didn't exit already cmdCancel() if err != nil { - return nil, fmt.Errorf("unable to ParsePatch: %w", err) + return nil, nil, nil, fmt.Errorf("unable to ParsePatch: %w", err) } diff.Start = opts.SkipTo + return diff, beforeCommit, afterCommit, nil +} + +func GetDiffForAPI(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) { + diff, _, _, err := getDiffBasic(ctx, gitRepo, opts, files...) + return diff, err +} - checker, deferable := gitRepo.CheckAttributeReader(opts.AfterCommitID) - defer deferable() +func GetDiffForRender(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) { + diff, beforeCommit, afterCommit, err := getDiffBasic(ctx, gitRepo, opts, files...) + if err != nil { + return nil, err + } + + checker, err := attribute.NewBatchChecker(gitRepo, opts.AfterCommitID, []string{attribute.LinguistVendored, attribute.LinguistGenerated, attribute.LinguistLanguage, attribute.GitlabLanguage}) + if err != nil { + return nil, err + } + defer checker.Close() for _, diffFile := range diff.Files { isVendored := optional.None[bool]() isGenerated := optional.None[bool]() - if checker != nil { - attrs, err := checker.CheckPath(diffFile.Name) - if err == nil { - isVendored = git.AttributeToBool(attrs, git.AttributeLinguistVendored) - isGenerated = git.AttributeToBool(attrs, git.AttributeLinguistGenerated) - - language := git.TryReadLanguageAttribute(attrs) - if language.Has() { - diffFile.Language = language.Value() - } + attrs, err := checker.CheckPath(diffFile.Name) + if err == nil { + isVendored, isGenerated = attrs.GetVendored(), attrs.GetGenerated() + language := attrs.GetLanguage() + if language.Has() { + diffFile.Language = language.Value() } } // Populate Submodule URLs if diffFile.SubmoduleDiffInfo != nil { - diffFile.SubmoduleDiffInfo.PopulateURL(diffFile, beforeCommit, commit) + diffFile.SubmoduleDiffInfo.PopulateURL(diffFile, beforeCommit, afterCommit) } if !isVendored.Has() { @@ -1226,76 +1222,80 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi isGenerated = optional.Some(analyze.IsGenerated(diffFile.Name)) } diffFile.IsGenerated = isGenerated.Value() - - tailSection := diffFile.GetTailSection(gitRepo, beforeCommit, commit) + tailSection, limitedContent := diffFile.GetTailSectionAndLimitedContent(beforeCommit, afterCommit) if tailSection != nil { diffFile.Sections = append(diffFile.Sections, tailSection) } - } - - if opts.FileOnly { - return diff, nil - } - stats, err := GetPullDiffStats(gitRepo, opts) - if err != nil { - return nil, err + if !setting.Git.DisableDiffHighlight { + if limitedContent.LeftContent != nil && limitedContent.LeftContent.buf.Len() < MaxDiffHighlightEntireFileSize { + diffFile.highlightedLeftLines = highlightCodeLines(diffFile, true /* left */, limitedContent.LeftContent.buf.String()) + } + if limitedContent.RightContent != nil && limitedContent.RightContent.buf.Len() < MaxDiffHighlightEntireFileSize { + diffFile.highlightedRightLines = highlightCodeLines(diffFile, false /* right */, limitedContent.RightContent.buf.String()) + } + } } - diff.NumFiles, diff.TotalAddition, diff.TotalDeletion = stats.NumFiles, stats.TotalAddition, stats.TotalDeletion - return diff, nil } -type PullDiffStats struct { +func highlightCodeLines(diffFile *DiffFile, isLeft bool, content string) map[int]template.HTML { + highlightedNewContent, _ := highlight.Code(diffFile.Name, diffFile.Language, content) + splitLines := strings.Split(string(highlightedNewContent), "\n") + lines := make(map[int]template.HTML, len(splitLines)) + // only save the highlighted lines we need, but not the whole file, to save memory + for _, sec := range diffFile.Sections { + for _, ln := range sec.Lines { + lineIdx := ln.LeftIdx + if !isLeft { + lineIdx = ln.RightIdx + } + if lineIdx >= 1 { + idx := lineIdx - 1 + if idx < len(splitLines) { + lines[idx] = template.HTML(splitLines[idx]) + } + } + } + } + return lines +} + +type DiffShortStat struct { NumFiles, TotalAddition, TotalDeletion int } -// GetPullDiffStats -func GetPullDiffStats(gitRepo *git.Repository, opts *DiffOptions) (*PullDiffStats, error) { +func GetDiffShortStat(gitRepo *git.Repository, beforeCommitID, afterCommitID string) (*DiffShortStat, error) { repoPath := gitRepo.Path - diff := &PullDiffStats{} - - separator := "..." - if opts.DirectComparison { - separator = ".." - } - - objectFormat, err := gitRepo.GetObjectFormat() + afterCommit, err := gitRepo.GetCommit(afterCommitID) if err != nil { return nil, err } - diffPaths := []string{opts.BeforeCommitID + separator + opts.AfterCommitID} - if len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == objectFormat.EmptyObjectID().String() { - diffPaths = []string{objectFormat.EmptyTree().String(), opts.AfterCommitID} + _, actualBeforeCommitID, err := guessBeforeCommitForDiff(gitRepo, beforeCommitID, afterCommit) + if err != nil { + return nil, err } - diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) - if err != nil && strings.Contains(err.Error(), "no merge base") { - // git >= 2.28 now returns an error if base and head have become unrelated. - // previously it would return the results of git diff --shortstat base head so let's try that... - diffPaths = []string{opts.BeforeCommitID, opts.AfterCommitID} - diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) - } + diff := &DiffShortStat{} + diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStatByCmdArgs(gitRepo.Ctx, repoPath, nil, actualBeforeCommitID.String(), afterCommitID) if err != nil { return nil, err } - return diff, nil } -// SyncAndGetUserSpecificDiff is like GetDiff, except that user specific data such as which files the given user has already viewed on the given PR will also be set -// Additionally, the database asynchronously is updated if files have changed since the last review -func SyncAndGetUserSpecificDiff(ctx context.Context, userID int64, pull *issues_model.PullRequest, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) { - diff, err := GetDiff(ctx, gitRepo, opts, files...) +// SyncUserSpecificDiff inserts user-specific data such as which files the user has already viewed on the given diff +// Additionally, the database is updated asynchronously if files have changed since the last review +func SyncUserSpecificDiff(ctx context.Context, userID int64, pull *issues_model.PullRequest, gitRepo *git.Repository, diff *Diff, opts *DiffOptions) (*pull_model.ReviewState, error) { + review, err := pull_model.GetNewestReviewState(ctx, userID, pull.ID) if err != nil { return nil, err } - review, err := pull_model.GetNewestReviewState(ctx, userID, pull.ID) - if err != nil || review == nil || review.UpdatedFiles == nil { - return diff, err + if review == nil || len(review.UpdatedFiles) == 0 { + return review, nil } latestCommit := opts.AfterCommitID @@ -1303,13 +1303,13 @@ func SyncAndGetUserSpecificDiff(ctx context.Context, userID int64, pull *issues_ latestCommit = pull.HeadBranch // opts.AfterCommitID is preferred because it handles PRs from forks correctly and the branch name doesn't } - changedFiles, err := gitRepo.GetFilesChangedBetween(review.CommitSHA, latestCommit) + changedFiles, errIgnored := gitRepo.GetFilesChangedBetween(review.CommitSHA, latestCommit) // There are way too many possible errors. // Examples are various git errors such as the commit the review was based on was gc'ed and hence doesn't exist anymore as well as unrecoverable errors where we should serve a 500 response // Due to the current architecture and physical limitation of needing to compare explicit error messages, we can only choose one approach without the code getting ugly // For SOME of the errors such as the gc'ed commit, it would be best to mark all files as changed // But as that does not work for all potential errors, we simply mark all files as unchanged and drop the error which always works, even if not as good as possible - if err != nil { + if errIgnored != nil { log.Error("Could not get changed files between %s and %s for pull request %d in repo with path %s. Assuming no changes. Error: %w", review.CommitSHA, latestCommit, pull.Index, gitRepo.Path, err) } @@ -1352,7 +1352,7 @@ outer: } } - return diff, nil + return review, nil } // CommentAsDiff returns c.Patch as *Diff @@ -1398,10 +1398,8 @@ func GetWhitespaceFlag(whitespaceBehavior string) git.TrustedCmdArgs { "ignore-eol": {"--ignore-space-at-eol"}, "show-all": nil, } - if flag, ok := whitespaceFlags[whitespaceBehavior]; ok { return flag } - log.Warn("unknown whitespace behavior: %q, default to 'show-all'", whitespaceBehavior) return nil } diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 1017d188dd..b84530043a 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -17,27 +17,10 @@ import ( "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/setting" - dmp "github.com/sergi/go-diff/diffmatchpatch" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestDiffToHTML(t *testing.T) { - assert.Equal(t, "foo <span class=\"added-code\">bar</span> biz", diffToHTML(nil, []dmp.Diff{ - {Type: dmp.DiffEqual, Text: "foo "}, - {Type: dmp.DiffInsert, Text: "bar"}, - {Type: dmp.DiffDelete, Text: " baz"}, - {Type: dmp.DiffEqual, Text: " biz"}, - }, DiffLineAdd)) - - assert.Equal(t, "foo <span class=\"removed-code\">bar</span> biz", diffToHTML(nil, []dmp.Diff{ - {Type: dmp.DiffEqual, Text: "foo "}, - {Type: dmp.DiffDelete, Text: "bar"}, - {Type: dmp.DiffInsert, Text: " baz"}, - {Type: dmp.DiffEqual, Text: " biz"}, - }, DiffLineDel)) -} - func TestParsePatch_skipTo(t *testing.T) { type testcase struct { name string @@ -182,16 +165,10 @@ diff --git "\\a/README.md" "\\b/README.md" } gotMarshaled, _ := json.MarshalIndent(got, "", " ") - if got.NumFiles != 1 { + if len(got.Files) != 1 { t.Errorf("ParsePath(%q) did not receive 1 file:\n%s", testcase.name, string(gotMarshaled)) return } - if got.TotalAddition != testcase.addition { - t.Errorf("ParsePath(%q) does not have correct totalAddition %d, wanted %d", testcase.name, got.TotalAddition, testcase.addition) - } - if got.TotalDeletion != testcase.deletion { - t.Errorf("ParsePath(%q) did not have correct totalDeletion %d, wanted %d", testcase.name, got.TotalDeletion, testcase.deletion) - } file := got.Files[0] if file.Addition != testcase.addition { t.Errorf("ParsePath(%q) does not have correct file addition %d, wanted %d", testcase.name, file.Addition, testcase.addition) @@ -407,16 +384,10 @@ index 6961180..9ba1a00 100644 } gotMarshaled, _ := json.MarshalIndent(got, "", " ") - if got.NumFiles != 1 { + if len(got.Files) != 1 { t.Errorf("ParsePath(%q) did not receive 1 file:\n%s", testcase.name, string(gotMarshaled)) return } - if got.TotalAddition != testcase.addition { - t.Errorf("ParsePath(%q) does not have correct totalAddition %d, wanted %d", testcase.name, got.TotalAddition, testcase.addition) - } - if got.TotalDeletion != testcase.deletion { - t.Errorf("ParsePath(%q) did not have correct totalDeletion %d, wanted %d", testcase.name, got.TotalDeletion, testcase.deletion) - } file := got.Files[0] if file.Addition != testcase.addition { t.Errorf("ParsePath(%q) does not have correct file addition %d, wanted %d", testcase.name, file.Addition, testcase.addition) @@ -445,7 +416,7 @@ index 0000000..6bb8f39 ` diffBuilder.WriteString(diff) - for i := 0; i < 35; i++ { + for i := range 35 { diffBuilder.WriteString("+line" + strconv.Itoa(i) + "\n") } diff = diffBuilder.String() @@ -482,11 +453,11 @@ index 0000000..6bb8f39 diffBuilder.Reset() diffBuilder.WriteString(diff) - for i := 0; i < 33; i++ { + for i := range 33 { diffBuilder.WriteString("+line" + strconv.Itoa(i) + "\n") } diffBuilder.WriteString("+line33") - for i := 0; i < 512; i++ { + for range 512 { diffBuilder.WriteString("0123456789ABCDEF") } diffBuilder.WriteByte('\n') @@ -628,23 +599,25 @@ func TestDiffLine_GetCommentSide(t *testing.T) { } func TestGetDiffRangeWithWhitespaceBehavior(t *testing.T) { - gitRepo, err := git.OpenRepository(git.DefaultContext, "./testdata/academic-module") + gitRepo, err := git.OpenRepository(t.Context(), "../../modules/git/tests/repos/repo5_pulls") require.NoError(t, err) defer gitRepo.Close() for _, behavior := range []git.TrustedCmdArgs{{"-w"}, {"--ignore-space-at-eol"}, {"-b"}, nil} { - diffs, err := GetDiff(db.DefaultContext, gitRepo, + diffs, err := GetDiffForAPI(t.Context(), gitRepo, &DiffOptions{ - AfterCommitID: "bd7063cc7c04689c4d082183d32a604ed27a24f9", - BeforeCommitID: "559c156f8e0178b71cb44355428f24001b08fc68", + AfterCommitID: "d8e0bbb45f200e67d9a784ce55bd90821af45ebd", + BeforeCommitID: "72866af952e98d02a73003501836074b286a78f6", MaxLines: setting.Git.MaxGitDiffLines, MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters, - MaxFiles: setting.Git.MaxGitDiffFiles, + MaxFiles: 1, WhitespaceBehavior: behavior, }) - assert.NoError(t, err, "Error when diff with %s", behavior) + require.NoError(t, err, "Error when diff with WhitespaceBehavior=%s", behavior) + assert.True(t, diffs.IsIncomplete) + assert.Len(t, diffs.Files, 1) for _, f := range diffs.Files { - assert.NotEmpty(t, f.Sections, "%s should have sections", f.Name) + assert.NotEmpty(t, f.Sections, "Diff file %q should have sections", f.Name) } } } diff --git a/services/gitdiff/highlightdiff.go b/services/gitdiff/highlightdiff.go index 35d4844550..e8be063e69 100644 --- a/services/gitdiff/highlightdiff.go +++ b/services/gitdiff/highlightdiff.go @@ -4,23 +4,24 @@ package gitdiff import ( + "bytes" + "html/template" "strings" - "code.gitea.io/gitea/modules/highlight" - "github.com/sergi/go-diff/diffmatchpatch" ) // token is a html tag or entity, eg: "<span ...>", "</span>", "<" func extractHTMLToken(s string) (before, token, after string, valid bool) { for pos1 := 0; pos1 < len(s); pos1++ { - if s[pos1] == '<' { + switch s[pos1] { + case '<': pos2 := strings.IndexByte(s[pos1:], '>') if pos2 == -1 { return "", "", s, false } return s[:pos1], s[pos1 : pos1+pos2+1], s[pos1+pos2+1:], true - } else if s[pos1] == '&' { + case '&': pos2 := strings.IndexByte(s[pos1:], ';') if pos2 == -1 { return "", "", s, false @@ -77,7 +78,7 @@ func (hcd *highlightCodeDiff) isInPlaceholderRange(r rune) bool { return hcd.placeholderBegin <= r && r < hcd.placeholderBegin+rune(hcd.placeholderMaxCount) } -func (hcd *highlightCodeDiff) collectUsedRunes(code string) { +func (hcd *highlightCodeDiff) collectUsedRunes(code template.HTML) { for _, r := range code { if hcd.isInPlaceholderRange(r) { // put the existing rune (used by code) in map, then this rune won't be used a placeholder anymore. @@ -86,27 +87,76 @@ func (hcd *highlightCodeDiff) collectUsedRunes(code string) { } } -func (hcd *highlightCodeDiff) diffWithHighlight(filename, language, codeA, codeB string) []diffmatchpatch.Diff { +func (hcd *highlightCodeDiff) diffLineWithHighlight(lineType DiffLineType, codeA, codeB template.HTML) template.HTML { + return hcd.diffLineWithHighlightWrapper(nil, lineType, codeA, codeB) +} + +func (hcd *highlightCodeDiff) diffLineWithHighlightWrapper(lineWrapperTags []string, lineType DiffLineType, codeA, codeB template.HTML) template.HTML { hcd.collectUsedRunes(codeA) hcd.collectUsedRunes(codeB) - highlightCodeA, _ := highlight.Code(filename, language, codeA) - highlightCodeB, _ := highlight.Code(filename, language, codeB) + convertedCodeA := hcd.convertToPlaceholders(codeA) + convertedCodeB := hcd.convertToPlaceholders(codeB) + + dmp := defaultDiffMatchPatch() + diffs := dmp.DiffMain(convertedCodeA, convertedCodeB, true) + diffs = dmp.DiffCleanupSemantic(diffs) + + buf := bytes.NewBuffer(nil) - convertedCodeA := hcd.convertToPlaceholders(string(highlightCodeA)) - convertedCodeB := hcd.convertToPlaceholders(string(highlightCodeB)) + // restore the line wrapper tags <span class="line"> and <span class="cl">, if necessary + for _, tag := range lineWrapperTags { + buf.WriteString(tag) + } - diffs := diffMatchPatch.DiffMain(convertedCodeA, convertedCodeB, true) - diffs = diffMatchPatch.DiffCleanupEfficiency(diffs) + addedCodePrefix := hcd.registerTokenAsPlaceholder(`<span class="added-code">`) + removedCodePrefix := hcd.registerTokenAsPlaceholder(`<span class="removed-code">`) + codeTagSuffix := hcd.registerTokenAsPlaceholder(`</span>`) - for i := range diffs { - hcd.recoverOneDiff(&diffs[i]) + if codeTagSuffix != 0 { + for _, diff := range diffs { + switch { + case diff.Type == diffmatchpatch.DiffEqual: + buf.WriteString(diff.Text) + case diff.Type == diffmatchpatch.DiffInsert && lineType == DiffLineAdd: + buf.WriteRune(addedCodePrefix) + buf.WriteString(diff.Text) + buf.WriteRune(codeTagSuffix) + case diff.Type == diffmatchpatch.DiffDelete && lineType == DiffLineDel: + buf.WriteRune(removedCodePrefix) + buf.WriteString(diff.Text) + buf.WriteRune(codeTagSuffix) + } + } + } else { + // placeholder map space is exhausted + for _, diff := range diffs { + take := diff.Type == diffmatchpatch.DiffEqual || (diff.Type == diffmatchpatch.DiffInsert && lineType == DiffLineAdd) || (diff.Type == diffmatchpatch.DiffDelete && lineType == DiffLineDel) + if take { + buf.WriteString(diff.Text) + } + } } - return diffs + for range lineWrapperTags { + buf.WriteString("</span>") + } + return hcd.recoverOneDiff(buf.String()) +} + +func (hcd *highlightCodeDiff) registerTokenAsPlaceholder(token string) rune { + placeholder, ok := hcd.tokenPlaceholderMap[token] + if !ok { + placeholder = hcd.nextPlaceholder() + if placeholder != 0 { + hcd.tokenPlaceholderMap[token] = placeholder + hcd.placeholderTokenMap[placeholder] = token + } + } + return placeholder } // convertToPlaceholders totally depends on Chroma's valid HTML output and its structure, do not use these functions for other purposes. -func (hcd *highlightCodeDiff) convertToPlaceholders(htmlCode string) string { +func (hcd *highlightCodeDiff) convertToPlaceholders(htmlContent template.HTML) string { var tagStack []string res := strings.Builder{} @@ -115,6 +165,7 @@ func (hcd *highlightCodeDiff) convertToPlaceholders(htmlCode string) string { var beforeToken, token string var valid bool + htmlCode := string(htmlContent) // the standard chroma highlight HTML is "<span class="line [hl]"><span class="cl"> ... </span></span>" for { beforeToken, token, htmlCode, valid = extractHTMLToken(htmlCode) @@ -151,14 +202,7 @@ func (hcd *highlightCodeDiff) convertToPlaceholders(htmlCode string) string { } // else: impossible // remember the placeholder and token in the map - placeholder, ok := hcd.tokenPlaceholderMap[tokenInMap] - if !ok { - placeholder = hcd.nextPlaceholder() - if placeholder != 0 { - hcd.tokenPlaceholderMap[tokenInMap] = placeholder - hcd.placeholderTokenMap[placeholder] = tokenInMap - } - } + placeholder := hcd.registerTokenAsPlaceholder(tokenInMap) if placeholder != 0 { res.WriteRune(placeholder) // use the placeholder to replace the token @@ -179,11 +223,11 @@ func (hcd *highlightCodeDiff) convertToPlaceholders(htmlCode string) string { return res.String() } -func (hcd *highlightCodeDiff) recoverOneDiff(diff *diffmatchpatch.Diff) { +func (hcd *highlightCodeDiff) recoverOneDiff(str string) template.HTML { sb := strings.Builder{} var tagStack []string - for _, r := range diff.Text { + for _, r := range str { token, ok := hcd.placeholderTokenMap[r] if !ok || token == "" { sb.WriteRune(r) // if the rune is not a placeholder, write it as it is @@ -217,6 +261,5 @@ func (hcd *highlightCodeDiff) recoverOneDiff(diff *diffmatchpatch.Diff) { } // else: impossible. every tag was pushed into the stack by the code above and is valid HTML opening tag } } - - diff.Text = sb.String() + return template.HTML(sb.String()) } diff --git a/services/gitdiff/highlightdiff_test.go b/services/gitdiff/highlightdiff_test.go index 545a060e20..aebe38ae7c 100644 --- a/services/gitdiff/highlightdiff_test.go +++ b/services/gitdiff/highlightdiff_test.go @@ -5,121 +5,82 @@ package gitdiff import ( "fmt" + "html/template" "strings" "testing" - "github.com/sergi/go-diff/diffmatchpatch" "github.com/stretchr/testify/assert" ) func TestDiffWithHighlight(t *testing.T) { - hcd := newHighlightCodeDiff() - diffs := hcd.diffWithHighlight( - "main.v", "", - " run('<>')\n", - " run(db)\n", - ) - - expected := ` <span class="n">run</span><span class="o">(</span><span class="removed-code"><span class="k">'</span><span class="o"><</span><span class="o">></span><span class="k">'</span></span><span class="o">)</span>` - output := diffToHTML(nil, diffs, DiffLineDel) - assert.Equal(t, expected, output) - - expected = ` <span class="n">run</span><span class="o">(</span><span class="added-code"><span class="n">db</span></span><span class="o">)</span>` - output = diffToHTML(nil, diffs, DiffLineAdd) - assert.Equal(t, expected, output) - - hcd = newHighlightCodeDiff() - hcd.placeholderTokenMap['O'] = "<span>" - hcd.placeholderTokenMap['C'] = "</span>" - diff := diffmatchpatch.Diff{} - - diff.Text = "OC" - hcd.recoverOneDiff(&diff) - assert.Equal(t, "<span></span>", diff.Text) - - diff.Text = "O" - hcd.recoverOneDiff(&diff) - assert.Equal(t, "<span></span>", diff.Text) - - diff.Text = "C" - hcd.recoverOneDiff(&diff) - assert.Equal(t, "", diff.Text) + t.Run("DiffLineAddDel", func(t *testing.T) { + hcd := newHighlightCodeDiff() + codeA := template.HTML(`x <span class="k">foo</span> y`) + codeB := template.HTML(`x <span class="k">bar</span> y`) + outDel := hcd.diffLineWithHighlight(DiffLineDel, codeA, codeB) + assert.Equal(t, `x <span class="k"><span class="removed-code">foo</span></span> y`, string(outDel)) + outAdd := hcd.diffLineWithHighlight(DiffLineAdd, codeA, codeB) + assert.Equal(t, `x <span class="k"><span class="added-code">bar</span></span> y`, string(outAdd)) + }) + + t.Run("CleanUp", func(t *testing.T) { + hcd := newHighlightCodeDiff() + codeA := template.HTML(`<span class="cm>this is a comment</span>`) + codeB := template.HTML(`<span class="cm>this is updated comment</span>`) + outDel := hcd.diffLineWithHighlight(DiffLineDel, codeA, codeB) + assert.Equal(t, `<span class="cm>this is <span class="removed-code">a</span> comment</span>`, string(outDel)) + outAdd := hcd.diffLineWithHighlight(DiffLineAdd, codeA, codeB) + assert.Equal(t, `<span class="cm>this is <span class="added-code">updated</span> comment</span>`, string(outAdd)) + }) + + t.Run("OpenCloseTags", func(t *testing.T) { + hcd := newHighlightCodeDiff() + hcd.placeholderTokenMap['O'], hcd.placeholderTokenMap['C'] = "<span>", "</span>" + assert.Equal(t, "<span></span>", string(hcd.recoverOneDiff("OC"))) + assert.Equal(t, "<span></span>", string(hcd.recoverOneDiff("O"))) + assert.Empty(t, string(hcd.recoverOneDiff("C"))) + }) } func TestDiffWithHighlightPlaceholder(t *testing.T) { hcd := newHighlightCodeDiff() - diffs := hcd.diffWithHighlight( - "main.js", "", - "a='\U00100000'", - "a='\U0010FFFD''", - ) - assert.Equal(t, "", hcd.placeholderTokenMap[0x00100000]) - assert.Equal(t, "", hcd.placeholderTokenMap[0x0010FFFD]) - - expected := fmt.Sprintf(`<span class="nx">a</span><span class="o">=</span><span class="s1">'</span><span class="removed-code">%s</span>'`, "\U00100000") - output := diffToHTML(hcd.lineWrapperTags, diffs, DiffLineDel) - assert.Equal(t, expected, output) + output := hcd.diffLineWithHighlight(DiffLineDel, "a='\U00100000'", "a='\U0010FFFD''") + assert.Empty(t, hcd.placeholderTokenMap[0x00100000]) + assert.Empty(t, hcd.placeholderTokenMap[0x0010FFFD]) + expected := fmt.Sprintf(`a='<span class="removed-code">%s</span>'`, "\U00100000") + assert.Equal(t, expected, string(output)) hcd = newHighlightCodeDiff() - diffs = hcd.diffWithHighlight( - "main.js", "", - "a='\U00100000'", - "a='\U0010FFFD'", - ) - expected = fmt.Sprintf(`<span class="nx">a</span><span class="o">=</span><span class="s1">'</span><span class="added-code">%s</span>'`, "\U0010FFFD") - output = diffToHTML(nil, diffs, DiffLineAdd) - assert.Equal(t, expected, output) + output = hcd.diffLineWithHighlight(DiffLineAdd, "a='\U00100000'", "a='\U0010FFFD'") + expected = fmt.Sprintf(`a='<span class="added-code">%s</span>'`, "\U0010FFFD") + assert.Equal(t, expected, string(output)) } func TestDiffWithHighlightPlaceholderExhausted(t *testing.T) { hcd := newHighlightCodeDiff() hcd.placeholderMaxCount = 0 - diffs := hcd.diffWithHighlight( - "main.js", "", - "'", - ``, - ) - output := diffToHTML(nil, diffs, DiffLineDel) - expected := fmt.Sprintf(`<span class="removed-code">%s#39;</span>`, "\uFFFD") - assert.Equal(t, expected, output) - - hcd = newHighlightCodeDiff() - hcd.placeholderMaxCount = 0 - diffs = hcd.diffWithHighlight( - "main.js", "", - "a < b", - "a > b", - ) - output = diffToHTML(nil, diffs, DiffLineDel) - expected = fmt.Sprintf(`a %s<span class="removed-code">l</span>t; b`, "\uFFFD") - assert.Equal(t, expected, output) - - output = diffToHTML(nil, diffs, DiffLineAdd) - expected = fmt.Sprintf(`a %s<span class="added-code">g</span>t; b`, "\uFFFD") - assert.Equal(t, expected, output) + placeHolderAmp := string(rune(0xFFFD)) + output := hcd.diffLineWithHighlight(DiffLineDel, `<span class="k"><</span>`, `<span class="k">></span>`) + assert.Equal(t, placeHolderAmp+"lt;", string(output)) + output = hcd.diffLineWithHighlight(DiffLineAdd, `<span class="k"><</span>`, `<span class="k">></span>`) + assert.Equal(t, placeHolderAmp+"gt;", string(output)) } func TestDiffWithHighlightTagMatch(t *testing.T) { - totalOverflow := 0 - for i := 0; i < 100; i++ { - hcd := newHighlightCodeDiff() - hcd.placeholderMaxCount = i - diffs := hcd.diffWithHighlight( - "main.js", "", - "a='1'", - "b='2'", - ) - totalOverflow += hcd.placeholderOverflowCount - - output := diffToHTML(nil, diffs, DiffLineDel) - c1 := strings.Count(output, "<span") - c2 := strings.Count(output, "</span") - assert.Equal(t, c1, c2) - - output = diffToHTML(nil, diffs, DiffLineAdd) - c1 = strings.Count(output, "<span") - c2 = strings.Count(output, "</span") - assert.Equal(t, c1, c2) + f := func(t *testing.T, lineType DiffLineType) { + totalOverflow := 0 + for i := 0; ; i++ { + hcd := newHighlightCodeDiff() + hcd.placeholderMaxCount = i + output := string(hcd.diffLineWithHighlight(lineType, `<span class="k"><</span>`, `<span class="k">></span>`)) + totalOverflow += hcd.placeholderOverflowCount + assert.Equal(t, strings.Count(output, "<span"), strings.Count(output, "</span")) + if hcd.placeholderOverflowCount == 0 { + break + } + } + assert.NotZero(t, totalOverflow) } - assert.NotZero(t, totalOverflow) + t.Run("DiffLineAdd", func(t *testing.T) { f(t, DiffLineAdd) }) + t.Run("DiffLineDel", func(t *testing.T) { f(t, DiffLineDel) }) } diff --git a/services/gitdiff/submodule_test.go b/services/gitdiff/submodule_test.go index 89f32c0e0c..152c5b7066 100644 --- a/services/gitdiff/submodule_test.go +++ b/services/gitdiff/submodule_test.go @@ -4,7 +4,6 @@ package gitdiff import ( - "context" "strings" "testing" @@ -204,7 +203,6 @@ index 0000000..68972a9 } for _, testcase := range tests { - testcase := testcase t.Run(testcase.name, func(t *testing.T) { diff, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), "") assert.NoError(t, err) @@ -224,13 +222,13 @@ func TestSubmoduleInfo(t *testing.T) { PreviousRefID: "aaaa", NewRefID: "bbbb", } - ctx := context.Background() + ctx := t.Context() assert.EqualValues(t, "1111", sdi.CommitRefIDLinkHTML(ctx, "1111")) assert.EqualValues(t, "aaaa...bbbb", sdi.CompareRefIDLinkHTML(ctx)) assert.EqualValues(t, "name", sdi.SubmoduleRepoLinkHTML(ctx)) sdi.SubmoduleFile = git.NewCommitSubmoduleFile("https://github.com/owner/repo", "1234") - assert.EqualValues(t, `<a href="https://github.com/owner/repo/commit/1111">1111</a>`, sdi.CommitRefIDLinkHTML(ctx, "1111")) + assert.EqualValues(t, `<a href="https://github.com/owner/repo/tree/1111">1111</a>`, sdi.CommitRefIDLinkHTML(ctx, "1111")) assert.EqualValues(t, `<a href="https://github.com/owner/repo/compare/aaaa...bbbb">aaaa...bbbb</a>`, sdi.CompareRefIDLinkHTML(ctx)) assert.EqualValues(t, `<a href="https://github.com/owner/repo">name</a>`, sdi.SubmoduleRepoLinkHTML(ctx)) } diff --git a/services/gitdiff/testdata/academic-module/HEAD b/services/gitdiff/testdata/academic-module/HEAD deleted file mode 100644 index cb089cd89a..0000000000 --- a/services/gitdiff/testdata/academic-module/HEAD +++ /dev/null @@ -1 +0,0 @@ -ref: refs/heads/master diff --git a/services/gitdiff/testdata/academic-module/config b/services/gitdiff/testdata/academic-module/config deleted file mode 100644 index 1bc26be514..0000000000 --- a/services/gitdiff/testdata/academic-module/config +++ /dev/null @@ -1,10 +0,0 @@ -[core] - repositoryformatversion = 0 - filemode = true - bare = false - logallrefupdates = true - ignorecase = true - precomposeunicode = true -[branch "master"] - remote = origin - merge = refs/heads/master diff --git a/services/gitdiff/testdata/academic-module/index b/services/gitdiff/testdata/academic-module/index Binary files differdeleted file mode 100644 index e712c906e3..0000000000 --- a/services/gitdiff/testdata/academic-module/index +++ /dev/null diff --git a/services/gitdiff/testdata/academic-module/logs/HEAD b/services/gitdiff/testdata/academic-module/logs/HEAD deleted file mode 100644 index 16b2e1c0f6..0000000000 --- a/services/gitdiff/testdata/academic-module/logs/HEAD +++ /dev/null @@ -1 +0,0 @@ -0000000000000000000000000000000000000000 bd7063cc7c04689c4d082183d32a604ed27a24f9 Lunny Xiao <xiaolunwen@gmail.com> 1574829684 +0800 clone: from https://try.gitea.io/shemgp-aiias/academic-module diff --git a/services/gitdiff/testdata/academic-module/logs/refs/heads/master b/services/gitdiff/testdata/academic-module/logs/refs/heads/master deleted file mode 100644 index 16b2e1c0f6..0000000000 --- a/services/gitdiff/testdata/academic-module/logs/refs/heads/master +++ /dev/null @@ -1 +0,0 @@ -0000000000000000000000000000000000000000 bd7063cc7c04689c4d082183d32a604ed27a24f9 Lunny Xiao <xiaolunwen@gmail.com> 1574829684 +0800 clone: from https://try.gitea.io/shemgp-aiias/academic-module diff --git a/services/gitdiff/testdata/academic-module/logs/refs/remotes/origin/HEAD b/services/gitdiff/testdata/academic-module/logs/refs/remotes/origin/HEAD deleted file mode 100644 index 16b2e1c0f6..0000000000 --- a/services/gitdiff/testdata/academic-module/logs/refs/remotes/origin/HEAD +++ /dev/null @@ -1 +0,0 @@ -0000000000000000000000000000000000000000 bd7063cc7c04689c4d082183d32a604ed27a24f9 Lunny Xiao <xiaolunwen@gmail.com> 1574829684 +0800 clone: from https://try.gitea.io/shemgp-aiias/academic-module diff --git a/services/gitdiff/testdata/academic-module/objects/pack/pack-597efbc3613c7ba790e33b178fd9fc1fe17b4245.idx b/services/gitdiff/testdata/academic-module/objects/pack/pack-597efbc3613c7ba790e33b178fd9fc1fe17b4245.idx Binary files differdeleted file mode 100644 index 4d759aa504..0000000000 --- a/services/gitdiff/testdata/academic-module/objects/pack/pack-597efbc3613c7ba790e33b178fd9fc1fe17b4245.idx +++ /dev/null diff --git a/services/gitdiff/testdata/academic-module/objects/pack/pack-597efbc3613c7ba790e33b178fd9fc1fe17b4245.pack b/services/gitdiff/testdata/academic-module/objects/pack/pack-597efbc3613c7ba790e33b178fd9fc1fe17b4245.pack Binary files differdeleted file mode 100644 index 2dc49cfded..0000000000 --- a/services/gitdiff/testdata/academic-module/objects/pack/pack-597efbc3613c7ba790e33b178fd9fc1fe17b4245.pack +++ /dev/null diff --git a/services/gitdiff/testdata/academic-module/packed-refs b/services/gitdiff/testdata/academic-module/packed-refs deleted file mode 100644 index 13b5611650..0000000000 --- a/services/gitdiff/testdata/academic-module/packed-refs +++ /dev/null @@ -1,2 +0,0 @@ -# pack-refs with: peeled fully-peeled sorted -bd7063cc7c04689c4d082183d32a604ed27a24f9 refs/remotes/origin/master diff --git a/services/gitdiff/testdata/academic-module/refs/heads/master b/services/gitdiff/testdata/academic-module/refs/heads/master deleted file mode 100644 index bd2b56eaf4..0000000000 --- a/services/gitdiff/testdata/academic-module/refs/heads/master +++ /dev/null @@ -1 +0,0 @@ -bd7063cc7c04689c4d082183d32a604ed27a24f9 diff --git a/services/gitdiff/testdata/academic-module/refs/remotes/origin/HEAD b/services/gitdiff/testdata/academic-module/refs/remotes/origin/HEAD deleted file mode 100644 index 6efe28fff8..0000000000 --- a/services/gitdiff/testdata/academic-module/refs/remotes/origin/HEAD +++ /dev/null @@ -1 +0,0 @@ -ref: refs/remotes/origin/master |