- Create vcs/util.go with GetAllFilesInPath and BuildLineToPositionMap - Create vcs/util_test.go with comprehensive tests for both functions - Remove review.ContentEntry type, replace with vcs.ContentEntry - Remove review.GiteaClient interface, replace with vcs.FileReader - Update review/repo_persona.go to use vcs.FileReader - Update review/repo_persona_test.go to use vcs.ContentEntry - Update cmd/review-bot/main.go adapter to implement vcs.FileReader - Add Number and Base fields to vcs.PullRequest - Add CommitStatus type to vcs/types.go - Add GetFileContentAtRef to vcs.PRReader interface - Add GetCommitStatuses to vcs.PRReader interface - Add DismissReview to vcs.Reviewer interface - Add stub implementations on gitea.Client for new interface methods Closes #84, Closes #85, Closes #86
This commit is contained in:
claw
@@ -10,6 +10,8 @@ type PRReader interface {
|
||||
GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error)
|
||||
GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error)
|
||||
GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error)
|
||||
GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error)
|
||||
GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error)
|
||||
}
|
||||
|
||||
// FileReader can fetch file contents and list directory entries.
|
||||
@@ -23,6 +25,7 @@ type Reviewer interface {
|
||||
PostReview(ctx context.Context, owner, repo string, number int, req ReviewRequest) (*Review, error)
|
||||
ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error)
|
||||
DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error
|
||||
DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error
|
||||
}
|
||||
|
||||
// Identity can report who the authenticated user is.
|
||||
|
||||
+18
-3
@@ -15,6 +15,11 @@ const (
|
||||
ReviewEventComment ReviewEvent = "COMMENT"
|
||||
)
|
||||
|
||||
// BaseRef identifies the target branch of a pull request.
|
||||
type BaseRef struct {
|
||||
Ref string `json:"ref"`
|
||||
}
|
||||
|
||||
// HeadRef identifies the source branch and latest commit of a pull request.
|
||||
type HeadRef struct {
|
||||
SHA string `json:"sha"`
|
||||
@@ -28,9 +33,11 @@ type UserInfo struct {
|
||||
|
||||
// PullRequest holds relevant PR metadata.
|
||||
type PullRequest struct {
|
||||
Title string `json:"title"`
|
||||
Body string `json:"body"`
|
||||
Head HeadRef `json:"head"`
|
||||
Number int `json:"number"`
|
||||
Title string `json:"title"`
|
||||
Body string `json:"body"`
|
||||
Head HeadRef `json:"head"`
|
||||
Base BaseRef `json:"base"`
|
||||
}
|
||||
|
||||
// ChangedFile represents a file modified in a PR.
|
||||
@@ -46,6 +53,14 @@ type ContentEntry struct {
|
||||
Type string `json:"type"` // "file" or "dir"
|
||||
}
|
||||
|
||||
// CommitStatus represents a single CI status entry for a commit.
|
||||
type CommitStatus struct {
|
||||
Status string `json:"status"`
|
||||
Context string `json:"context"`
|
||||
Description string `json:"description"`
|
||||
TargetURL string `json:"target_url"`
|
||||
}
|
||||
|
||||
// Review represents a pull request review.
|
||||
type Review struct {
|
||||
ID int64 `json:"id"`
|
||||
|
||||
+145
@@ -0,0 +1,145 @@
|
||||
package vcs
|
||||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"strings"
|
||||
)
|
||||
|
||||
// GetAllFilesInPath recursively fetches all file contents under a path using the
|
||||
// provided FileReader. Returns a map of filepath -> content for all files found.
|
||||
// If the path points to an empty directory, returns an empty map.
|
||||
func GetAllFilesInPath(ctx context.Context, client FileReader, owner, repo, path string) (map[string]string, error) {
|
||||
results := make(map[string]string)
|
||||
|
||||
entries, err := client.ListContents(ctx, owner, repo, path)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("list contents %q: %w", path, err)
|
||||
}
|
||||
|
||||
for _, entry := range entries {
|
||||
switch entry.Type {
|
||||
case "file":
|
||||
content, err := client.GetFileContent(ctx, owner, repo, entry.Path, "")
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("get file %q: %w", entry.Path, err)
|
||||
}
|
||||
results[entry.Path] = content
|
||||
case "dir":
|
||||
subResults, err := GetAllFilesInPath(ctx, client, owner, repo, entry.Path)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("recurse into %q: %w", entry.Path, err)
|
||||
}
|
||||
for k, v := range subResults {
|
||||
results[k] = v
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return results, nil
|
||||
}
|
||||
|
||||
// BuildLineToPositionMap parses a unified diff and returns a map of
|
||||
// filename -> (new line number -> diff position). The diff position is a
|
||||
// 1-indexed offset from the @@ hunk header line for each file.
|
||||
// Only lines that appear in the new file (context lines and additions) are mapped.
|
||||
// Deletion-only lines are not included.
|
||||
func BuildLineToPositionMap(diff string) map[string]map[int]int {
|
||||
result := make(map[string]map[int]int)
|
||||
|
||||
lines := strings.Split(diff, "\n")
|
||||
var currentFile string
|
||||
var position int
|
||||
var newLine int
|
||||
|
||||
for _, line := range lines {
|
||||
// Detect new file in diff
|
||||
if strings.HasPrefix(line, "+++ b/") {
|
||||
currentFile = strings.TrimPrefix(line, "+++ b/")
|
||||
position = 0
|
||||
newLine = 0
|
||||
if result[currentFile] == nil {
|
||||
result[currentFile] = make(map[int]int)
|
||||
}
|
||||
continue
|
||||
}
|
||||
|
||||
// Skip --- lines (old file header)
|
||||
if strings.HasPrefix(line, "--- ") {
|
||||
continue
|
||||
}
|
||||
|
||||
// Skip diff --git lines
|
||||
if strings.HasPrefix(line, "diff --git") {
|
||||
continue
|
||||
}
|
||||
|
||||
// Skip index lines
|
||||
if strings.HasPrefix(line, "index ") {
|
||||
continue
|
||||
}
|
||||
|
||||
// Parse hunk headers
|
||||
if strings.HasPrefix(line, "@@") {
|
||||
position++
|
||||
// Extract new file start line from @@ -a,b +c,d @@
|
||||
newLine = parseHunkNewStart(line)
|
||||
continue
|
||||
}
|
||||
|
||||
// We need a current file to map lines
|
||||
if currentFile == "" {
|
||||
continue
|
||||
}
|
||||
|
||||
// Process diff content lines
|
||||
if strings.HasPrefix(line, "+") {
|
||||
position++
|
||||
result[currentFile][newLine] = position
|
||||
newLine++
|
||||
} else if strings.HasPrefix(line, "-") {
|
||||
position++
|
||||
// Deletion lines don't map to new line numbers
|
||||
} else if strings.HasPrefix(line, " ") {
|
||||
// Context line (space-prefixed)
|
||||
if position > 0 {
|
||||
position++
|
||||
result[currentFile][newLine] = position
|
||||
newLine++
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return result
|
||||
}
|
||||
|
||||
// parseHunkNewStart extracts the new-file starting line number from a hunk header.
|
||||
// Format: @@ -old_start[,old_count] +new_start[,new_count] @@
|
||||
func parseHunkNewStart(hunkLine string) int {
|
||||
// Find the +N part
|
||||
plusIdx := strings.Index(hunkLine, "+")
|
||||
if plusIdx < 0 {
|
||||
return 1
|
||||
}
|
||||
rest := hunkLine[plusIdx+1:]
|
||||
|
||||
// Read digits until comma or space
|
||||
var numStr string
|
||||
for _, ch := range rest {
|
||||
if ch >= '0' && ch <= '9' {
|
||||
numStr += string(ch)
|
||||
} else {
|
||||
break
|
||||
}
|
||||
}
|
||||
|
||||
if numStr == "" {
|
||||
return 1
|
||||
}
|
||||
|
||||
n := 0
|
||||
for _, ch := range numStr {
|
||||
n = n*10 + int(ch-'0')
|
||||
}
|
||||
return n
|
||||
}
|
||||
@@ -0,0 +1,250 @@
|
||||
package vcs_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"testing"
|
||||
|
||||
"gitea.weiker.me/rodin/review-bot/vcs"
|
||||
)
|
||||
|
||||
// mockFileReader implements vcs.FileReader for testing.
|
||||
type mockFileReader struct {
|
||||
contents map[string][]vcs.ContentEntry // path -> entries
|
||||
files map[string]string // path -> content
|
||||
}
|
||||
|
||||
func (m *mockFileReader) GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error) {
|
||||
content, ok := m.files[path]
|
||||
if !ok {
|
||||
return "", fmt.Errorf("HTTP 404: file not found: %s", path)
|
||||
}
|
||||
return content, nil
|
||||
}
|
||||
|
||||
func (m *mockFileReader) ListContents(ctx context.Context, owner, repo, path string) ([]vcs.ContentEntry, error) {
|
||||
entries, ok := m.contents[path]
|
||||
if !ok {
|
||||
return nil, fmt.Errorf("HTTP 404: path not found: %s", path)
|
||||
}
|
||||
return entries, nil
|
||||
}
|
||||
|
||||
func TestGetAllFilesInPath(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
|
||||
t.Run("empty directory", func(t *testing.T) {
|
||||
client := &mockFileReader{
|
||||
contents: map[string][]vcs.ContentEntry{
|
||||
"src": {},
|
||||
},
|
||||
}
|
||||
result, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if len(result) != 0 {
|
||||
t.Errorf("expected empty map, got %d entries", len(result))
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("flat directory", func(t *testing.T) {
|
||||
client := &mockFileReader{
|
||||
contents: map[string][]vcs.ContentEntry{
|
||||
"src": {
|
||||
{Name: "main.go", Path: "src/main.go", Type: "file"},
|
||||
{Name: "util.go", Path: "src/util.go", Type: "file"},
|
||||
},
|
||||
},
|
||||
files: map[string]string{
|
||||
"src/main.go": "package main",
|
||||
"src/util.go": "package main\n// util",
|
||||
},
|
||||
}
|
||||
result, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if len(result) != 2 {
|
||||
t.Fatalf("expected 2 files, got %d", len(result))
|
||||
}
|
||||
if result["src/main.go"] != "package main" {
|
||||
t.Errorf("main.go content = %q", result["src/main.go"])
|
||||
}
|
||||
if result["src/util.go"] != "package main\n// util" {
|
||||
t.Errorf("util.go content = %q", result["src/util.go"])
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("nested directories", func(t *testing.T) {
|
||||
client := &mockFileReader{
|
||||
contents: map[string][]vcs.ContentEntry{
|
||||
"src": {
|
||||
{Name: "main.go", Path: "src/main.go", Type: "file"},
|
||||
{Name: "pkg", Path: "src/pkg", Type: "dir"},
|
||||
},
|
||||
"src/pkg": {
|
||||
{Name: "lib.go", Path: "src/pkg/lib.go", Type: "file"},
|
||||
{Name: "sub", Path: "src/pkg/sub", Type: "dir"},
|
||||
},
|
||||
"src/pkg/sub": {
|
||||
{Name: "deep.go", Path: "src/pkg/sub/deep.go", Type: "file"},
|
||||
},
|
||||
},
|
||||
files: map[string]string{
|
||||
"src/main.go": "package main",
|
||||
"src/pkg/lib.go": "package pkg",
|
||||
"src/pkg/sub/deep.go": "package sub",
|
||||
},
|
||||
}
|
||||
result, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if len(result) != 3 {
|
||||
t.Fatalf("expected 3 files, got %d", len(result))
|
||||
}
|
||||
if result["src/main.go"] != "package main" {
|
||||
t.Errorf("main.go content = %q", result["src/main.go"])
|
||||
}
|
||||
if result["src/pkg/lib.go"] != "package pkg" {
|
||||
t.Errorf("lib.go content = %q", result["src/pkg/lib.go"])
|
||||
}
|
||||
if result["src/pkg/sub/deep.go"] != "package sub" {
|
||||
t.Errorf("deep.go content = %q", result["src/pkg/sub/deep.go"])
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("mixed files and dirs", func(t *testing.T) {
|
||||
client := &mockFileReader{
|
||||
contents: map[string][]vcs.ContentEntry{
|
||||
"root": {
|
||||
{Name: "README.md", Path: "root/README.md", Type: "file"},
|
||||
{Name: "docs", Path: "root/docs", Type: "dir"},
|
||||
{Name: "config.yaml", Path: "root/config.yaml", Type: "file"},
|
||||
},
|
||||
"root/docs": {
|
||||
{Name: "guide.md", Path: "root/docs/guide.md", Type: "file"},
|
||||
},
|
||||
},
|
||||
files: map[string]string{
|
||||
"root/README.md": "# Hello",
|
||||
"root/config.yaml": "key: value",
|
||||
"root/docs/guide.md": "## Guide",
|
||||
},
|
||||
}
|
||||
result, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "root")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if len(result) != 3 {
|
||||
t.Fatalf("expected 3 files, got %d", len(result))
|
||||
}
|
||||
if result["root/README.md"] != "# Hello" {
|
||||
t.Errorf("README content = %q", result["root/README.md"])
|
||||
}
|
||||
if result["root/docs/guide.md"] != "## Guide" {
|
||||
t.Errorf("guide content = %q", result["root/docs/guide.md"])
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func TestBuildLineToPositionMap(t *testing.T) {
|
||||
t.Run("single hunk", func(t *testing.T) {
|
||||
diff := "diff --git a/file.go b/file.go\nindex abc..def 100644\n--- a/file.go\n+++ b/file.go\n@@ -1,3 +1,4 @@\n package main\n \n+// new comment\n func main() {}\n"
|
||||
result := vcs.BuildLineToPositionMap(diff)
|
||||
fileMap, ok := result["file.go"]
|
||||
if !ok {
|
||||
t.Fatal("expected file.go in result")
|
||||
}
|
||||
// Hunk header @@ is position 1
|
||||
// Line 1: " package main" -> position 2
|
||||
if fileMap[1] != 2 {
|
||||
t.Errorf("line 1 position = %d, want 2", fileMap[1])
|
||||
}
|
||||
// Line 2: " " (context) -> position 3
|
||||
if fileMap[2] != 3 {
|
||||
t.Errorf("line 2 position = %d, want 3", fileMap[2])
|
||||
}
|
||||
// Line 3: "+// new comment" -> position 4
|
||||
if fileMap[3] != 4 {
|
||||
t.Errorf("line 3 position = %d, want 4", fileMap[3])
|
||||
}
|
||||
// Line 4: " func main() {}" -> position 5
|
||||
if fileMap[4] != 5 {
|
||||
t.Errorf("line 4 position = %d, want 5", fileMap[4])
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("multi hunk", func(t *testing.T) {
|
||||
diff := "diff --git a/file.go b/file.go\n--- a/file.go\n+++ b/file.go\n@@ -1,3 +1,3 @@\n package main\n \n-// old\n+// new\n@@ -10,3 +10,4 @@\n func foo() {\n+\t// added\n \treturn\n }\n"
|
||||
result := vcs.BuildLineToPositionMap(diff)
|
||||
fileMap, ok := result["file.go"]
|
||||
if !ok {
|
||||
t.Fatal("expected file.go in result")
|
||||
}
|
||||
// First hunk: @@ is position 1
|
||||
// Line 1: " package main" -> position 2
|
||||
if fileMap[1] != 2 {
|
||||
t.Errorf("line 1 position = %d, want 2", fileMap[1])
|
||||
}
|
||||
// Line 3: "+// new" -> position 5 (after " ", "-// old" at pos 3,4)
|
||||
if fileMap[3] != 5 {
|
||||
t.Errorf("line 3 position = %d, want 5", fileMap[3])
|
||||
}
|
||||
// Second hunk: @@ is position 6
|
||||
// Line 10: " func foo() {" -> position 7
|
||||
if fileMap[10] != 7 {
|
||||
t.Errorf("line 10 position = %d, want 7", fileMap[10])
|
||||
}
|
||||
// Line 11: "+\t// added" -> position 8
|
||||
if fileMap[11] != 8 {
|
||||
t.Errorf("line 11 position = %d, want 8", fileMap[11])
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("deletion lines not in map", func(t *testing.T) {
|
||||
diff := "diff --git a/file.go b/file.go\n--- a/file.go\n+++ b/file.go\n@@ -1,4 +1,3 @@\n package main\n \n-// deleted line\n func main() {}\n"
|
||||
result := vcs.BuildLineToPositionMap(diff)
|
||||
fileMap, ok := result["file.go"]
|
||||
if !ok {
|
||||
t.Fatal("expected file.go in result")
|
||||
}
|
||||
// Line 1: " package main" -> position 2
|
||||
if fileMap[1] != 2 {
|
||||
t.Errorf("line 1 position = %d, want 2", fileMap[1])
|
||||
}
|
||||
// Line 3 in new file: " func main() {}" -> position 5 (after deletion at pos 4)
|
||||
if fileMap[3] != 5 {
|
||||
t.Errorf("line 3 position = %d, want 5", fileMap[3])
|
||||
}
|
||||
// Should only have 3 entries (lines 1, 2, 3 of new file)
|
||||
if len(fileMap) != 3 {
|
||||
t.Errorf("expected 3 mapped lines, got %d: %v", len(fileMap), fileMap)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("multiple files", func(t *testing.T) {
|
||||
diff := "diff --git a/a.go b/a.go\n--- a/a.go\n+++ b/a.go\n@@ -1,2 +1,3 @@\n package a\n \n+// file a\ndiff --git a/b.go b/b.go\n--- a/b.go\n+++ b/b.go\n@@ -1,2 +1,3 @@\n package b\n \n+// file b\n"
|
||||
result := vcs.BuildLineToPositionMap(diff)
|
||||
if len(result) != 2 {
|
||||
t.Fatalf("expected 2 files, got %d", len(result))
|
||||
}
|
||||
aMap, ok := result["a.go"]
|
||||
if !ok {
|
||||
t.Fatal("expected a.go in result")
|
||||
}
|
||||
bMap, ok := result["b.go"]
|
||||
if !ok {
|
||||
t.Fatal("expected b.go in result")
|
||||
}
|
||||
// a.go line 3: "+// file a" -> position 4
|
||||
if aMap[3] != 4 {
|
||||
t.Errorf("a.go line 3 position = %d, want 4", aMap[3])
|
||||
}
|
||||
// b.go line 3: "+// file b" -> position 4
|
||||
if bMap[3] != 4 {
|
||||
t.Errorf("b.go line 3 position = %d, want 4", bMap[3])
|
||||
}
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user