fix(gitea): map hunk-header positions in BuildPositionToLineMap #104

Merged
aweiker merged 2 commits from review-bot-issue-97 into feature/github-support 2026-05-13 13:15:31 +00:00
2 changed files with 120 additions and 4 deletions
+11 -4
View File
@@ -11,6 +11,7 @@ import (
type PositionMap struct { 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).
// Hunk-header lines are mapped to 0 (no new-file line).
files map[string]map[int]int files map[string]map[int]int
// maxPositions caches the highest position number per file, // maxPositions caches the highest position number per file,
// tracked during construction to avoid O(n) scans at translate time. // tracked during construction to avoid O(n) scans at translate time.
@@ -19,8 +20,8 @@ type PositionMap struct {
// 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.
// Returns an error if the file is not in the diff or the position is out of range. // Returns an error if the file is not in the diff or the position is out of range.
// If the position targets a deletion line, it maps to the nearest non-deletion line below; // If the position targets a deletion or hunk-header line, it maps to the nearest
// if no such line exists, returns an error. // context/addition line below; if no such line exists, returns an error.
func (pm *PositionMap) Translate(file string, position int) (int, error) { func (pm *PositionMap) Translate(file string, position int) (int, error) {
if pm == nil || pm.files == nil { if pm == nil || pm.files == nil {
return 0, fmt.Errorf("empty position map") return 0, fmt.Errorf("empty position map")
@@ -41,14 +42,18 @@ func (pm *PositionMap) Translate(file string, position int) (int, error) {
} }
// lineNum == -1 means this position is a deletion line. // lineNum == -1 means this position is a deletion line.
// Map to the nearest non-deletion line below. // lineNum == 0 means this position is a hunk-header line.
if lineNum == -1 { // Both map to the nearest context/addition line below.
if lineNum <= 0 {
maxPos := pm.maxPosition(file) maxPos := pm.maxPosition(file)
for p := position + 1; p <= maxPos; p++ { for p := position + 1; p <= maxPos; p++ {
Review

Same reasoning as the earlier NIT on this topic: the post-loop position is the natural place for error differentiation. Computing it before the loop would add a variable that's only used in the failure path and separate the error logic from where the failure is detected. Current flow is: attempt resolution → explain failure. Keeping as-is.

Same reasoning as the earlier NIT on this topic: the post-loop position is the natural place for error differentiation. Computing it before the loop would add a variable that's only used in the failure path and separate the error logic from where the failure is detected. Current flow is: attempt resolution → explain failure. Keeping as-is.
if ln, exists := fileMap[p]; exists && ln > 0 { if ln, exists := fileMap[p]; exists && ln > 0 {
return ln, nil return ln, nil
} }
Review

[NIT] The error-message disambiguation (hunk-header vs deletion) occurs after the forward-scan loop, but the check if lineNum == 0 is evaluated only after the loop exhausts. This is correct and readable, but the two error messages could equally be computed before the loop (since lineNum doesn't change). Minor style point only — no functional issue.

**[NIT]** The error-message disambiguation (hunk-header vs deletion) occurs after the forward-scan loop, but the check `if lineNum == 0` is evaluated only after the loop exhausts. This is correct and readable, but the two error messages could equally be computed before the loop (since `lineNum` doesn't change). Minor style point only — no functional issue.
} }
Review

Acknowledged. The current structure reads top-to-bottom: scan forward for a resolution, then differentiate the error. Moving the check before the loop would require storing the message in a variable or duplicating the scan logic. The linear post-loop flow is clearer. Keeping as-is.

Acknowledged. The current structure reads top-to-bottom: scan forward for a resolution, then differentiate the error. Moving the check before the loop would require storing the message in a variable or duplicating the scan logic. The linear post-loop flow is clearer. Keeping as-is.
if lineNum == 0 {
return 0, fmt.Errorf("position %d targets a hunk-header line with no subsequent new-file line in %q", position, file)
Review

[NIT] The error-message discrimination between lineNum==0 and lineNum==-1 is checked after the walk loop, meaning the if lineNum == 0 branch is dead code for the hunk-header error path when there IS a subsequent line — that path returns early. The logic is still correct, but the structure is slightly misleading: the if/else at lines 53-57 would be clearer if written as two separate if lineNum == -1 / if lineNum == 0 fallbacks before the shared loop, or the error strings could simply be unified into one message since both cases have identical semantics from a caller perspective. Minor readability issue only.

**[NIT]** The error-message discrimination between lineNum==0 and lineNum==-1 is checked after the walk loop, meaning the `if lineNum == 0` branch is dead code for the hunk-header error path when there IS a subsequent line — that path returns early. The logic is still correct, but the structure is slightly misleading: the if/else at lines 53-57 would be clearer if written as two separate `if lineNum == -1` / `if lineNum == 0` fallbacks before the shared loop, or the error strings could simply be unified into one message since both cases have identical semantics from a caller perspective. Minor readability issue only.
}
return 0, fmt.Errorf("position %d targets a deletion line with no subsequent new-file line in %q", position, file) return 0, fmt.Errorf("position %d targets a deletion line with no subsequent new-file line in %q", position, file)
} }
@@ -70,6 +75,7 @@ func (pm *PositionMap) maxPosition(file string) int {
// - A new @@ hunk within the same file continues incrementing (does not reset) // - A new @@ hunk within the same file continues incrementing (does not reset)
// - 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)
// - Hunk-header lines have a position but no new-file line number (stored as 0)
func BuildPositionToLineMap(diff string) *PositionMap { func BuildPositionToLineMap(diff string) *PositionMap {
pm := &PositionMap{ pm := &PositionMap{
files: make(map[string]map[int]int), files: make(map[string]map[int]int),
@@ -126,6 +132,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.files[currentFile][position] = 0 // sentinel: hunk-header has no new-file line
pm.maxPositions[currentFile] = position pm.maxPositions[currentFile] = position
newLine = parseHunkStart(line) newLine = parseHunkStart(line)
continue continue
+109
View File
@@ -272,3 +272,112 @@ diff --git a/b.go b/b.go
t.Errorf("Translate(b.go, 3) = %d, want 2", got) t.Errorf("Translate(b.go, 3) = %d, want 2", got)
} }
} }
func TestTranslate_HunkHeaderPosition_SingleHunk(t *testing.T) {
// Position 1 is the @@ hunk-header line.
// It should resolve to the first context/addition line below (new line 16).
diff := `diff --git a/file.go b/file.go
index abc..def 100644
--- a/file.go
+++ b/file.go
@@ -16,4 +16,5 @@ func example() {
context line
-deleted line
+added line
context after
`
pm := BuildPositionToLineMap(diff)
got, err := pm.Translate("file.go", 1)
if err != nil {
t.Fatalf("Translate(file.go, 1): unexpected error: %v", err)
}
if got != 16 {
t.Errorf("Translate(file.go, 1) = %d, want 16 (first context/addition line in hunk)", got)
}
}
func TestTranslate_HunkHeaderPosition_MultiHunk(t *testing.T) {
// First hunk: @@ is pos 1, then " line1" (pos 2), "-old" (pos 3), "+new" (pos 4)
// Second hunk: @@ is pos 5, then " func foo() {" (pos 6), "+// added" (pos 7), etc.
// Translating position 5 (second @@) should resolve to new line 10.
diff := `diff --git a/file.go b/file.go
--- a/file.go
Review

This edge case IS tested — see TestTranslate_HunkHeaderAtEnd (line 361), which tests exactly the scenario where a hunk-header is the last position with no subsequent new-file line. That test covers the error path the NIT mentions.

This edge case IS tested — see `TestTranslate_HunkHeaderAtEnd` (line 361), which tests exactly the scenario where a hunk-header is the last position with no subsequent new-file line. That test covers the error path the NIT mentions.
+++ b/file.go
@@ -1,3 +1,3 @@ package main
line1
Review

[NIT] TestTranslate_HunkHeaderPosition_MultiHunk does not test the edge case where a hunk-header is the last position in the file (i.e., the error path for a hunk-header with no subsequent line). The deletion equivalent is covered by TestBuildPositionToLineMap_DeletionAtEnd but there is no corresponding test for the new hunk-header error path. Not a correctness issue since the code path is trivially reached from the shared loop, but worth noting for completeness.

**[NIT]** TestTranslate_HunkHeaderPosition_MultiHunk does not test the edge case where a hunk-header is the last position in the file (i.e., the error path for a hunk-header with no subsequent line). The deletion equivalent is covered by TestBuildPositionToLineMap_DeletionAtEnd but there is no corresponding test for the new hunk-header error path. Not a correctness issue since the code path is trivially reached from the shared loop, but worth noting for completeness.
-old
+new
@@ -10,3 +10,4 @@ func foo() {
func foo() {
+ // added
return
}
`
pm := BuildPositionToLineMap(diff)
// Position 5 is the second @@ hunk-header — should resolve to new line 10
got, err := pm.Translate("file.go", 5)
if err != nil {
t.Fatalf("Translate(file.go, 5): unexpected error: %v", err)
}
if got != 10 {
t.Errorf("Translate(file.go, 5) = %d, want 10 (first context/addition line in second hunk)", got)
}
// Also verify first hunk header at position 1 resolves to new line 1
got, err = pm.Translate("file.go", 1)
if err != nil {
t.Fatalf("Translate(file.go, 1): unexpected error: %v", err)
}
if got != 1 {
t.Errorf("Translate(file.go, 1) = %d, want 1 (first context/addition line in first hunk)", got)
}
}
func TestTranslate_HunkHeaderPosition_NewFile(t *testing.T) {
// New file: @@ -0,0 +1,3 @@ is position 1.
// Should resolve to new line 1 (the first addition).
diff := `diff --git a/new.go b/new.go
new file mode 100644
--- /dev/null
+++ b/new.go
@@ -0,0 +1,3 @@
+package main
+
+func init() {}
`
pm := BuildPositionToLineMap(diff)
got, err := pm.Translate("new.go", 1)
if err != nil {
t.Fatalf("Translate(new.go, 1): unexpected error: %v", err)
}
if got != 1 {
t.Errorf("Translate(new.go, 1) = %d, want 1 (first addition line)", got)
}
}
func TestTranslate_HunkHeaderAtEnd(t *testing.T) {
// A hunk-header at the last position with no subsequent new-file line should error.
// This is the hunk-header equivalent of TestBuildPositionToLineMap_DeletionAtEnd.
diff := `diff --git a/file.go b/file.go
--- a/file.go
+++ b/file.go
@@ -1,2 +1,2 @@ package main
line1
-old
+new
@@ -10,2 +10,1 @@ func foo() {
-removed
`
pm := BuildPositionToLineMap(diff)
// Position 5 is the second @@ hunk-header; the only line after it (pos 6) is a
// deletion (lineNum == -1), so there's no positive new-file line to resolve to.
// The hunk-header lookup should fail.
_, err := pm.Translate("file.go", 5)
if err == nil {
t.Error("expected error for hunk-header at end with no subsequent new-file line")
}
}