fix(gitea): map hunk-header positions in BuildPositionToLineMap #104
@@ -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++ {
|
||||||
|
|
|||||||
if ln, exists := fileMap[p]; exists && ln > 0 {
|
if ln, exists := fileMap[p]; exists && ln > 0 {
|
||||||
return ln, nil
|
return ln, nil
|
||||||
}
|
}
|
||||||
|
sonnet-review-bot
commented
[NIT] The error-message disambiguation (hunk-header vs deletion) occurs after the forward-scan loop, but the check **[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.
|
|||||||
}
|
}
|
||||||
|
rodin
commented
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)
|
||||||
|
sonnet-review-bot
commented
[NIT] The error-message discrimination between lineNum==0 and lineNum==-1 is checked after the walk loop, meaning the **[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
|
||||||
|
|||||||
@@ -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
|
||||||
|
rodin
commented
This edge case IS tested — see 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
|
||||||
|
sonnet-review-bot
commented
[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")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
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.