From 29564a891b0e854ef51fdf08d4e3e04544f71b14 Mon Sep 17 00:00:00 2001 From: kite Date: Sat, 23 May 2026 00:36:51 +0800 Subject: [PATCH] refactor(diff): Simplify hunk resolution logic with helper functions This change refactors the `resolveFromHunk` function by introducing several helper functions (`extractSideLines`, `matchConsecutive`) to improve readability and maintainability. The new implementation attempts to match against both new and old sides of the diff hunk, making comment resolution more robust. --- internal/diff/resolver.go | 114 ++++--- internal/diff/resolver_test.go | 534 +++++++++++++++++++++++++++++++++ 2 files changed, 606 insertions(+), 42 deletions(-) diff --git a/internal/diff/resolver.go b/internal/diff/resolver.go index d5353c6..da60158 100644 --- a/internal/diff/resolver.go +++ b/internal/diff/resolver.go @@ -54,11 +54,17 @@ func ResolveLineNumbers(comments []model.LlmComment, diffs []model.Diff) []model return result } -// resolveFromHunk tries to find the startLine/endLine by matching ExistingCode -// against "from" side lines (context + deleted) in the diff hunks. -// Returns true on success (comments fields are mutated in place). +// indexedLine pairs a normalized line with its absolute file line number. +type indexedLine struct { + lineNum int + content string +} + +// resolveFromHunk tries to find startLine/endLine by matching ExistingCode +// against hunk lines. It tries the new-side first (context + added lines → +// new-file line numbers), then falls back to old-side (context + deleted → +// old-file line numbers). func resolveFromHunk(d *model.Diff, cm *model.LlmComment) bool { - // TODO: re-track with llm hunks := ParseHunks(d.Diff) if len(hunks) == 0 { return false @@ -70,53 +76,77 @@ func resolveFromHunk(d *model.Diff, cm *model.LlmComment) bool { } for i := range hunks { - hunk := &hunks[i] - offset := 0 // tracks position within old-file range + newSide := extractSideLines(&hunks[i], true) + if start, end, ok := matchConsecutive(newSide, targetLines); ok { + cm.StartLine = start + cm.EndLine = end + return true + } + } - lines := hunk.Lines - for j := 0; j < len(lines); j++ { - line := lines[j] - - switch line.Type { - case HunkAdded: - // Added lines are TO-side only; don't affect old-file offset - continue - case HunkContext, HunkDeleted: - // Both are FROM-side candidates - if matchAt(lines, j, targetLines) { - startLine := hunk.OldStart + offset - endLine := startLine + len(targetLines) - 1 - cm.StartLine = startLine - cm.EndLine = endLine - return true - } - offset++ - } + for i := range hunks { + oldSide := extractSideLines(&hunks[i], false) + if start, end, ok := matchConsecutive(oldSide, targetLines); ok { + cm.StartLine = start + cm.EndLine = end + return true } } return false } -// matchAt checks whether targetLines[i] matches the normalized content of -// lines[startIndex+i] for all i, where each matched line must be a "from" side -// line (context or deleted). -func matchAt(lines []HunkLine, startIndex int, targetLines []string) bool { - for i, target := range targetLines { - idx := startIndex + i - if idx >= len(lines) { - return false - } - l := lines[idx] - // Must be a "from" side line - if l.Type != HunkContext && l.Type != HunkDeleted { - return false - } - if normalizeLine(l.Content) != target { - return false +// extractSideLines extracts one side of the diff from a hunk. +// When newSide is true, returns context+added lines with new-file line numbers. +// When newSide is false, returns context+deleted lines with old-file line numbers. +func extractSideLines(hunk *Hunk, newSide bool) []indexedLine { + var result []indexedLine + oldLine := hunk.OldStart + newLine := hunk.NewStart + + for _, l := range hunk.Lines { + switch l.Type { + case HunkContext: + if newSide { + result = append(result, indexedLine{newLine, normalizeLine(l.Content)}) + } else { + result = append(result, indexedLine{oldLine, normalizeLine(l.Content)}) + } + oldLine++ + newLine++ + case HunkAdded: + if newSide { + result = append(result, indexedLine{newLine, normalizeLine(l.Content)}) + } + newLine++ + case HunkDeleted: + if !newSide { + result = append(result, indexedLine{oldLine, normalizeLine(l.Content)}) + } + oldLine++ } } - return true + return result +} + +// matchConsecutive scans sideLines for a consecutive run matching all targetLines. +func matchConsecutive(sideLines []indexedLine, targetLines []string) (startLine, endLine int, found bool) { + if len(targetLines) == 0 || len(sideLines) < len(targetLines) { + return 0, 0, false + } + for i := 0; i <= len(sideLines)-len(targetLines); i++ { + matched := true + for j, target := range targetLines { + if sideLines[i+j].content != target { + matched = false + break + } + } + if matched { + return sideLines[i].lineNum, sideLines[i+len(targetLines)-1].lineNum, true + } + } + return 0, 0, false } // resolveFromFileContent scans the new file content line-by-line for consecutive diff --git a/internal/diff/resolver_test.go b/internal/diff/resolver_test.go index a305429..dd8effd 100644 --- a/internal/diff/resolver_test.go +++ b/internal/diff/resolver_test.go @@ -207,6 +207,280 @@ line2`) } } +// --------------------------------------------------------------------------- +// extractSideLines unit tests +// --------------------------------------------------------------------------- + +func TestExtractSideLines_NewSide(t *testing.T) { + hunk := Hunk{ + OldStart: 10, OldCount: 3, + NewStart: 10, NewCount: 4, + Lines: []HunkLine{ + {HunkContext, " ctx := r.Context()"}, + {HunkDeleted, ` log.Print("old")`}, + {HunkAdded, ` log.Printf("new: %s", r.URL)`}, + {HunkContext, " err := process(ctx)"}, + }, + } + + got := extractSideLines(&hunk, true) + + want := []indexedLine{ + {10, `ctx := r.Context()`}, + {11, `log.Printf("new: %s", r.URL)`}, + {12, `err := process(ctx)`}, + } + + if len(got) != len(want) { + t.Fatalf("new-side: expected %d lines, got %d", len(want), len(got)) + } + for i := range want { + if got[i].lineNum != want[i].lineNum || got[i].content != want[i].content { + t.Errorf("new-side[%d]: got {%d, %q}, want {%d, %q}", + i, got[i].lineNum, got[i].content, want[i].lineNum, want[i].content) + } + } +} + +func TestExtractSideLines_OldSide(t *testing.T) { + hunk := Hunk{ + OldStart: 10, OldCount: 3, + NewStart: 10, NewCount: 4, + Lines: []HunkLine{ + {HunkContext, " ctx := r.Context()"}, + {HunkDeleted, ` log.Print("old")`}, + {HunkAdded, ` log.Printf("new: %s", r.URL)`}, + {HunkContext, " err := process(ctx)"}, + }, + } + + got := extractSideLines(&hunk, false) + + want := []indexedLine{ + {10, `ctx := r.Context()`}, + {11, `log.Print("old")`}, + {12, `err := process(ctx)`}, + } + + if len(got) != len(want) { + t.Fatalf("old-side: expected %d lines, got %d", len(want), len(got)) + } + for i := range want { + if got[i].lineNum != want[i].lineNum || got[i].content != want[i].content { + t.Errorf("old-side[%d]: got {%d, %q}, want {%d, %q}", + i, got[i].lineNum, got[i].content, want[i].lineNum, want[i].content) + } + } +} + +func TestExtractSideLines_DivergentStartLines(t *testing.T) { + hunk := Hunk{ + OldStart: 5, OldCount: 2, + NewStart: 8, NewCount: 3, + Lines: []HunkLine{ + {HunkContext, "A"}, + {HunkAdded, "B"}, + {HunkContext, "C"}, + }, + } + + newSide := extractSideLines(&hunk, true) + if len(newSide) != 3 { + t.Fatalf("expected 3 new-side lines, got %d", len(newSide)) + } + // new-side: A(8), B(9), C(10) + wantNew := []int{8, 9, 10} + for i, w := range wantNew { + if newSide[i].lineNum != w { + t.Errorf("new-side[%d].lineNum = %d, want %d", i, newSide[i].lineNum, w) + } + } + + oldSide := extractSideLines(&hunk, false) + if len(oldSide) != 2 { + t.Fatalf("expected 2 old-side lines, got %d", len(oldSide)) + } + // old-side: A(5), C(6) + wantOld := []int{5, 6} + for i, w := range wantOld { + if oldSide[i].lineNum != w { + t.Errorf("old-side[%d].lineNum = %d, want %d", i, oldSide[i].lineNum, w) + } + } +} + +func TestExtractSideLines_OnlyAdded(t *testing.T) { + hunk := Hunk{ + OldStart: 1, OldCount: 0, + NewStart: 1, NewCount: 2, + Lines: []HunkLine{ + {HunkAdded, "line1"}, + {HunkAdded, "line2"}, + }, + } + + newSide := extractSideLines(&hunk, true) + if len(newSide) != 2 { + t.Fatalf("expected 2 new-side lines, got %d", len(newSide)) + } + + oldSide := extractSideLines(&hunk, false) + if len(oldSide) != 0 { + t.Errorf("expected 0 old-side lines, got %d", len(oldSide)) + } +} + +func TestExtractSideLines_OnlyDeleted(t *testing.T) { + hunk := Hunk{ + OldStart: 3, OldCount: 2, + NewStart: 3, NewCount: 0, + Lines: []HunkLine{ + {HunkDeleted, "old1"}, + {HunkDeleted, "old2"}, + }, + } + + oldSide := extractSideLines(&hunk, false) + if len(oldSide) != 2 { + t.Fatalf("expected 2 old-side lines, got %d", len(oldSide)) + } + if oldSide[0].lineNum != 3 || oldSide[1].lineNum != 4 { + t.Errorf("expected line nums 3,4, got %d,%d", oldSide[0].lineNum, oldSide[1].lineNum) + } + + newSide := extractSideLines(&hunk, true) + if len(newSide) != 0 { + t.Errorf("expected 0 new-side lines, got %d", len(newSide)) + } +} + +// --------------------------------------------------------------------------- +// matchConsecutive unit tests +// --------------------------------------------------------------------------- + +func TestMatchConsecutive_SingleLine(t *testing.T) { + lines := []indexedLine{{5, "hello"}, {6, "world"}, {7, "foo"}} + start, end, ok := matchConsecutive(lines, []string{"world"}) + if !ok || start != 6 || end != 6 { + t.Errorf("single-line: got (%d, %d, %v), want (6, 6, true)", start, end, ok) + } +} + +func TestMatchConsecutive_MultiLine(t *testing.T) { + lines := []indexedLine{{1, "a"}, {2, "b"}, {3, "c"}, {4, "d"}} + start, end, ok := matchConsecutive(lines, []string{"b", "c"}) + if !ok || start != 2 || end != 3 { + t.Errorf("multi-line: got (%d, %d, %v), want (2, 3, true)", start, end, ok) + } +} + +func TestMatchConsecutive_NoMatch(t *testing.T) { + lines := []indexedLine{{1, "a"}, {2, "b"}} + _, _, ok := matchConsecutive(lines, []string{"x"}) + if ok { + t.Errorf("expected no match") + } +} + +func TestMatchConsecutive_FirstMatchWins(t *testing.T) { + lines := []indexedLine{{10, "x"}, {11, "y"}, {20, "x"}, {21, "y"}} + start, end, ok := matchConsecutive(lines, []string{"x", "y"}) + if !ok || start != 10 || end != 11 { + t.Errorf("first match: got (%d, %d, %v), want (10, 11, true)", start, end, ok) + } +} + +func TestMatchConsecutive_TargetLongerThanLines(t *testing.T) { + lines := []indexedLine{{1, "a"}} + _, _, ok := matchConsecutive(lines, []string{"a", "b"}) + if ok { + t.Errorf("expected no match when target is longer") + } +} + +func TestMatchConsecutive_EmptySideLines(t *testing.T) { + _, _, ok := matchConsecutive(nil, []string{"a"}) + if ok { + t.Errorf("expected no match on empty side lines") + } +} + +func TestMatchConsecutive_MatchAtEnd(t *testing.T) { + lines := []indexedLine{{1, "a"}, {2, "b"}, {3, "c"}} + start, end, ok := matchConsecutive(lines, []string{"b", "c"}) + if !ok || start != 2 || end != 3 { + t.Errorf("match-at-end: got (%d, %d, %v), want (2, 3, true)", start, end, ok) + } +} + +func TestMatchConsecutive_MatchAtStart(t *testing.T) { + lines := []indexedLine{{1, "a"}, {2, "b"}, {3, "c"}} + start, end, ok := matchConsecutive(lines, []string{"a", "b"}) + if !ok || start != 1 || end != 2 { + t.Errorf("match-at-start: got (%d, %d, %v), want (1, 2, true)", start, end, ok) + } +} + +func TestMatchConsecutive_ExactFull(t *testing.T) { + lines := []indexedLine{{1, "a"}, {2, "b"}} + start, end, ok := matchConsecutive(lines, []string{"a", "b"}) + if !ok || start != 1 || end != 2 { + t.Errorf("exact-full: got (%d, %d, %v), want (1, 2, true)", start, end, ok) + } +} + +// --------------------------------------------------------------------------- +// resolveFromHunk integration tests +// --------------------------------------------------------------------------- + +func TestResolveFromHunk_AddedLines(t *testing.T) { + raw := `diff --git a/test.go b/test.go +--- a/test.go ++++ b/test.go +@@ -3,3 +3,5 @@ + func main() { ++ x := 1 ++ y := 2 + fmt.Println("hello") + }` + + diffs := []model.Diff{{NewPath: "test.go", Diff: raw}} + comments := []model.LlmComment{ + {Path: "test.go", ExistingCode: ` x := 1 + y := 2`}, + } + + result := ResolveLineNumbers(comments, diffs) + cm := result[0] + if cm.StartLine != 4 || cm.EndLine != 5 { + t.Errorf("added lines: expected 4..5, got %d..%d", cm.StartLine, cm.EndLine) + } +} + +func TestResolveFromHunk_OldSideAcrossAddedLines(t *testing.T) { + raw := `diff --git a/test.go b/test.go +--- a/test.go ++++ b/test.go +@@ -5,3 +5,4 @@ + x := 1 ++ z := 99 + y := 2 + }` + + diffs := []model.Diff{{NewPath: "test.go", Diff: raw}} + comments := []model.LlmComment{ + {Path: "test.go", ExistingCode: ` x := 1 + y := 2`}, + } + + result := ResolveLineNumbers(comments, diffs) + cm := result[0] + // Old-side extracts [x:=1(5), y:=2(6), }(7)] — consecutive after skipping added line + if cm.StartLine != 5 || cm.EndLine != 6 { + t.Errorf("old-side across added: expected 5..6, got %d..%d", cm.StartLine, cm.EndLine) + } +} + func TestResolveFromHunk_ContextLinesOnly(t *testing.T) { // When existing_code matches context (unchanged) lines rather than deleted ones raw := `diff --git a/test.go b/test.go @@ -232,3 +506,263 @@ func TestResolveFromHunk_ContextLinesOnly(t *testing.T) { t.Errorf("expected line 4, got %d", cm.StartLine) } } + +func TestResolveFromHunk_SingleAddedLine(t *testing.T) { + raw := `diff --git a/test.go b/test.go +--- a/test.go ++++ b/test.go +@@ -1,2 +1,3 @@ + package main ++import "fmt" + func main() {}` + + diffs := []model.Diff{{NewPath: "test.go", Diff: raw}} + comments := []model.LlmComment{ + {Path: "test.go", ExistingCode: `import "fmt"`}, + } + + result := ResolveLineNumbers(comments, diffs) + cm := result[0] + if cm.StartLine != 2 || cm.EndLine != 2 { + t.Errorf("single added line: expected 2..2, got %d..%d", cm.StartLine, cm.EndLine) + } +} + +func TestResolveFromHunk_NewSidePriority(t *testing.T) { + // "fmt.Println" appears on both sides (context line). + // OldStart differs from NewStart → verifies new-side wins with new-file line number. + raw := `diff --git a/test.go b/test.go +--- a/test.go ++++ b/test.go +@@ -5,3 +8,4 @@ + func main() { + fmt.Println("hello") ++ fmt.Println("world") + }` + + diffs := []model.Diff{{NewPath: "test.go", Diff: raw}} + comments := []model.LlmComment{ + {Path: "test.go", ExistingCode: ` fmt.Println("hello")`}, + } + + result := ResolveLineNumbers(comments, diffs) + cm := result[0] + // new-side: func main(8), fmt.Println("hello")(9), fmt.Println("world")(10), }(11) + // old-side: func main(5), fmt.Println("hello")(6), }(7) + // new-side wins → line 9 + if cm.StartLine != 9 { + t.Errorf("new-side priority: expected 9, got %d", cm.StartLine) + } +} + +func TestResolveFromHunk_MultiHunkMatchInSecond(t *testing.T) { + raw := `diff --git a/test.go b/test.go +--- a/test.go ++++ b/test.go +@@ -2,3 +2,3 @@ + func foo() { +- old1() ++ new1() + } +@@ -20,3 +20,4 @@ + func bar() { ++ added_in_bar() + existing() + }` + + diffs := []model.Diff{{NewPath: "test.go", Diff: raw}} + comments := []model.LlmComment{ + {Path: "test.go", ExistingCode: " added_in_bar()"}, + } + + result := ResolveLineNumbers(comments, diffs) + cm := result[0] + // Second hunk @@ -20,3 +20,4: new-side → func bar(20), added_in_bar(21), existing(22), }(23) + if cm.StartLine != 21 || cm.EndLine != 21 { + t.Errorf("multi-hunk second: expected 21..21, got %d..%d", cm.StartLine, cm.EndLine) + } +} + +func TestResolveFromHunk_AddedWithContext(t *testing.T) { + raw := `diff --git a/test.go b/test.go +--- a/test.go ++++ b/test.go +@@ -10,3 +10,5 @@ + func process() { ++ validate() ++ transform() + save() + }` + + diffs := []model.Diff{{NewPath: "test.go", Diff: raw}} + // LLM provides added line + surrounding context + comments := []model.LlmComment{ + {Path: "test.go", ExistingCode: ` validate() + transform() + save()`}, + } + + result := ResolveLineNumbers(comments, diffs) + cm := result[0] + // new-side: process(10), validate(11), transform(12), save(13), }(14) + if cm.StartLine != 11 || cm.EndLine != 13 { + t.Errorf("added+context: expected 11..13, got %d..%d", cm.StartLine, cm.EndLine) + } +} + +func TestResolveFromHunk_NewSideAcrossDeletedLines(t *testing.T) { + raw := `diff --git a/test.go b/test.go +--- a/test.go ++++ b/test.go +@@ -5,4 +5,3 @@ + a := 1 +- unused := 0 + b := 2 + }` + + diffs := []model.Diff{{NewPath: "test.go", Diff: raw}} + // Target spans two context lines that have a deleted line between them in the hunk. + // In the new file they are consecutive. + comments := []model.LlmComment{ + {Path: "test.go", ExistingCode: ` a := 1 + b := 2`}, + } + + result := ResolveLineNumbers(comments, diffs) + cm := result[0] + // new-side: a:=1(5), b:=2(6), }(7) — deleted line skipped, consecutive + if cm.StartLine != 5 || cm.EndLine != 6 { + t.Errorf("new-side across deleted: expected 5..6, got %d..%d", cm.StartLine, cm.EndLine) + } +} + +// --------------------------------------------------------------------------- +// ResolveLineNumbers integration tests (additional scenarios) +// --------------------------------------------------------------------------- + +func TestResolveLineNumbers_AlreadyResolved(t *testing.T) { + diffs := []model.Diff{{NewPath: "test.go", Diff: testDiff}} + comments := []model.LlmComment{ + {Path: "test.go", ExistingCode: `log.Print("handling request")`, StartLine: 99, EndLine: 99}, + } + + result := ResolveLineNumbers(comments, diffs) + if result[0].StartLine != 99 || result[0].EndLine != 99 { + t.Errorf("already resolved: expected 99..99, got %d..%d", result[0].StartLine, result[0].EndLine) + } +} + +func TestResolveLineNumbers_MultipleCommentsOnSameFile(t *testing.T) { + raw := `diff --git a/test.go b/test.go +--- a/test.go ++++ b/test.go +@@ -1,4 +1,6 @@ + package main ++import "fmt" ++import "os" + func main() { +- old() ++ new() + }` + + diffs := []model.Diff{{NewPath: "test.go", Diff: raw}} + comments := []model.LlmComment{ + {Path: "test.go", ExistingCode: `import "fmt"`}, + {Path: "test.go", ExistingCode: `import "os"`}, + {Path: "test.go", ExistingCode: " old()"}, + } + + result := ResolveLineNumbers(comments, diffs) + // comment 0: added import "fmt" → new-side line 2 + if result[0].StartLine != 2 || result[0].EndLine != 2 { + t.Errorf("comment[0]: expected 2..2, got %d..%d", result[0].StartLine, result[0].EndLine) + } + // comment 1: added import "os" → new-side line 3 + if result[1].StartLine != 3 || result[1].EndLine != 3 { + t.Errorf("comment[1]: expected 3..3, got %d..%d", result[1].StartLine, result[1].EndLine) + } + // comment 2: deleted old() → old-side; @@ -1,4: old-side lines: package(1), main(2), old(3), }(4) + if result[2].StartLine != 3 || result[2].EndLine != 3 { + t.Errorf("comment[2]: expected 3..3, got %d..%d", result[2].StartLine, result[2].EndLine) + } +} + +func TestResolveLineNumbers_OldPathMapping(t *testing.T) { + raw := `diff --git a/old_name.go b/new_name.go +--- a/old_name.go ++++ b/new_name.go +@@ -1,3 +1,3 @@ + package main +-func oldFunc() {} ++func newFunc() {}` + + diffs := []model.Diff{{OldPath: "old_name.go", NewPath: "new_name.go", Diff: raw}} + // Comment references old path — should still resolve via diffByPath[oldPath] + comments := []model.LlmComment{ + {Path: "old_name.go", ExistingCode: "func oldFunc() {}"}, + } + + result := ResolveLineNumbers(comments, diffs) + cm := result[0] + if cm.StartLine != 2 || cm.EndLine != 2 { + t.Errorf("old path mapping: expected 2..2, got %d..%d", cm.StartLine, cm.EndLine) + } +} + +func TestResolveLineNumbers_MixedStrategies(t *testing.T) { + raw := `diff --git a/test.go b/test.go +--- a/test.go ++++ b/test.go +@@ -5,3 +5,4 @@ + func foo() { ++ newLine() + bar() + }` + + diffs := []model.Diff{ + { + NewPath: "test.go", Diff: raw, + NewFileContent: "package main\nimport \"fmt\"\n\nfunc helper() {}\nfunc foo() {\n newLine()\n bar()\n}", + }, + } + comments := []model.LlmComment{ + {Path: "test.go", ExistingCode: " newLine()"}, // hunk new-side + {Path: "test.go", ExistingCode: "func helper() {}"}, // not in hunk, file content fallback + {Path: "test.go", ExistingCode: "this_does_not_exist_anywhere()"}, // no match + } + + result := ResolveLineNumbers(comments, diffs) + + if result[0].StartLine != 6 || result[0].EndLine != 6 { + t.Errorf("hunk match: expected 6..6, got %d..%d", result[0].StartLine, result[0].EndLine) + } + if result[1].StartLine != 4 || result[1].EndLine != 4 { + t.Errorf("file content fallback: expected 4..4, got %d..%d", result[1].StartLine, result[1].EndLine) + } + if result[2].StartLine != 0 || result[2].EndLine != 0 { + t.Errorf("no match: expected 0..0, got %d..%d", result[2].StartLine, result[2].EndLine) + } +} + +func TestResolveLineNumbers_DiffMarkerInExistingCode(t *testing.T) { + raw := `diff --git a/test.go b/test.go +--- a/test.go ++++ b/test.go +@@ -1,2 +1,3 @@ + x := 1 ++y := 2 + z := 3` + + diffs := []model.Diff{{NewPath: "test.go", Diff: raw}} + // LLM may include the '+' marker from diff output + comments := []model.LlmComment{ + {Path: "test.go", ExistingCode: "+y := 2"}, + } + + result := ResolveLineNumbers(comments, diffs) + cm := result[0] + // normalizeLine strips leading '+', so "+y := 2" → "y := 2" matches + if cm.StartLine != 2 || cm.EndLine != 2 { + t.Errorf("diff marker in existing_code: expected 2..2, got %d..%d", cm.StartLine, cm.EndLine) + } +}