refactor(gitea): address review feedback on PR #90

- position.go: Replace O(n) maxPosition scan with O(1) lookup by
  tracking max position during map construction. Also eliminates
  shadowing of the builtin max identifier (Go 1.21+).
- position.go: Add comment clarifying +++ prefix ordering intent.
- adapter.go: Document diff-fetch tradeoff in PostReview.
- adapter_test.go: Remove extra blank line between test functions.
This commit is contained in:
claw
2026-05-12 13:57:44 -07:00
parent 701f0bed64
commit c86510b65d
3 changed files with 22 additions and 12 deletions
+5 -1
View File
@@ -150,7 +150,11 @@ func (a *Adapter) PostReview(ctx context.Context, owner, repo string, number int
var giteaComments []ReviewComment var giteaComments []ReviewComment
if len(req.Comments) > 0 { if len(req.Comments) > 0 {
// Fetch diff to build position → line number map // Fetch diff to build position → line number map.
// The diff is fetched unconditionally when comments exist. This adds latency
// for reviews with inline comments but keeps the implementation simple — caching
// the diff across calls would add complexity for minimal gain since PostReview
// is called at most once per review cycle.
diff, err := a.client.GetPullRequestDiff(ctx, owner, repo, number) diff, err := a.client.GetPullRequestDiff(ctx, owner, repo, number)
if err != nil { if err != nil {
return nil, fmt.Errorf("fetch diff for position translation: %w", err) return nil, fmt.Errorf("fetch diff for position translation: %w", err)
-1
View File
@@ -354,7 +354,6 @@ func TestAdapter_ListContents(t *testing.T) {
} }
} }
func TestAdapter_GetFileContent_RefRouting(t *testing.T) { func TestAdapter_GetFileContent_RefRouting(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// When ref is provided, the URL should contain ?ref= // When ref is provided, the URL should contain ?ref=
+17 -10
View File
@@ -12,6 +12,9 @@ type PositionMap struct {
// files maps filename → (position → new-file line number). // files maps filename → (position → new-file line number).
// Deletion lines are mapped to -1 (no new-file line). // Deletion lines are mapped to -1 (no new-file line).
files map[string]map[int]int files map[string]map[int]int
// maxPositions caches the highest position number per file,
// tracked during construction to avoid O(n) scans at translate time.
maxPositions map[string]int
} }
// Translate converts a GitHub diff-position to a new-file line number for a given file. // Translate converts a GitHub diff-position to a new-file line number for a given file.
@@ -53,15 +56,9 @@ func (pm *PositionMap) Translate(file string, position int) (int, error) {
} }
// maxPosition returns the highest position number for a file. // maxPosition returns the highest position number for a file.
// O(n) per call — acceptable since deletion-line fallback is rare and n is small (typical hunk size). // O(1) — the maximum is tracked during map construction.
func (pm *PositionMap) maxPosition(file string) int { func (pm *PositionMap) maxPosition(file string) int {
max := 0 return pm.maxPositions[file]
for pos := range pm.files[file] {
if pos > max {
max = pos
}
}
return max
} }
// BuildPositionToLineMap parses a unified diff and builds a PositionMap // BuildPositionToLineMap parses a unified diff and builds a PositionMap
@@ -74,7 +71,10 @@ func (pm *PositionMap) maxPosition(file string) int {
// - Position maps to the new file line number for additions and context lines // - Position maps to the new file line number for additions and context lines
// - Deletion lines have a position but no new-file line number (stored as -1) // - Deletion lines have a position but no new-file line number (stored as -1)
func BuildPositionToLineMap(diff string) *PositionMap { func BuildPositionToLineMap(diff string) *PositionMap {
pm := &PositionMap{files: make(map[string]map[int]int)} pm := &PositionMap{
files: make(map[string]map[int]int),
maxPositions: make(map[string]int),
}
lines := strings.Split(diff, "\n") lines := strings.Split(diff, "\n")
var currentFile string var currentFile string
@@ -82,7 +82,10 @@ func BuildPositionToLineMap(diff string) *PositionMap {
var newLine int var newLine int
for _, line := range lines { for _, line := range lines {
// Detect new file in diff // Detect new file in diff.
// "+++ b/" is checked before "+++ /dev/null" — the two prefixes are
// non-overlapping ("+++ /dev/null" does not start with "+++ b/"), so
// ordering is independent. Checking the common case first for clarity.
if strings.HasPrefix(line, "+++ b/") { if strings.HasPrefix(line, "+++ b/") {
currentFile = strings.TrimPrefix(line, "+++ b/") currentFile = strings.TrimPrefix(line, "+++ b/")
position = 0 position = 0
@@ -123,6 +126,7 @@ func BuildPositionToLineMap(diff string) *PositionMap {
// Parse hunk headers // Parse hunk headers
if strings.HasPrefix(line, "@@") && currentFile != "" { if strings.HasPrefix(line, "@@") && currentFile != "" {
position++ position++
pm.maxPositions[currentFile] = position
newLine = parseHunkStart(line) newLine = parseHunkStart(line)
continue continue
} }
@@ -141,15 +145,18 @@ func BuildPositionToLineMap(diff string) *PositionMap {
// Addition: has a new-file line number // Addition: has a new-file line number
position++ position++
pm.files[currentFile][position] = newLine pm.files[currentFile][position] = newLine
pm.maxPositions[currentFile] = position
newLine++ newLine++
} else if strings.HasPrefix(line, "-") { } else if strings.HasPrefix(line, "-") {
// Deletion: has a position but no new-file line number // Deletion: has a position but no new-file line number
position++ position++
pm.files[currentFile][position] = -1 pm.files[currentFile][position] = -1
pm.maxPositions[currentFile] = position
} else if strings.HasPrefix(line, " ") { } else if strings.HasPrefix(line, " ") {
// Context line // Context line
position++ position++
pm.files[currentFile][position] = newLine pm.files[currentFile][position] = newLine
pm.maxPositions[currentFile] = position
newLine++ newLine++
} }
} }