From f1feec1f4fcc09173f0dc7442d6ff9052419e372 Mon Sep 17 00:00:00 2001 From: Edmund Kohlwey Date: Sat, 13 Jun 2026 20:45:13 -0700 Subject: [PATCH] diffmap: Add diff position mapper Comment workflows need to translate working-tree line numbers into diff positions before a forge can accept inline comment requests. The diff mapper parses unified diff hunks into a small mapping API that callers can use without depending on any forge implementation. Keeping this package independent lets forge comment support and CLI state management adopt the mapper separately. Extracted from #1162 --- internal/diffmap/mapper.go | 281 ++++++++++++++++++++++++++++++++ internal/diffmap/mapper_test.go | 198 ++++++++++++++++++++++ 2 files changed, 479 insertions(+) create mode 100644 internal/diffmap/mapper.go create mode 100644 internal/diffmap/mapper_test.go diff --git a/internal/diffmap/mapper.go b/internal/diffmap/mapper.go new file mode 100644 index 000000000..d309eae77 --- /dev/null +++ b/internal/diffmap/mapper.go @@ -0,0 +1,281 @@ +// Package diffmap maps working-tree line numbers +// to diff hunk positions for inline code review comments. +package diffmap + +import ( + "bufio" + "bytes" + "errors" + "fmt" + "slices" + "strconv" + "strings" +) + +// Mapper maps file:line references to diff coordinates. +type Mapper struct { + // files maps file paths to their diff hunks. + files map[string]*fileDiff +} + +type fileDiff struct { + // hunks are the diff hunks for the file, + // in order of appearance. + hunks []hunk +} + +type hunk struct { + // newStart is the starting line number + // in the new version of the file. + newStart int + + // newCount is the number of lines + // in the new version of the hunk. + newCount int + + // oldStart is the starting line number + // in the old version of the file. + oldStart int + + // oldCount is the number of lines + // in the old version of the hunk. + oldCount int + + // lines are the diff lines in the hunk. + lines []diffLine +} + +type diffLine struct { + // op is the diff operation: ' ', '+', or '-'. + op byte + + // newLineNo is the line number in the new file. + // Zero for deleted lines. + newLineNo int + + // oldLineNo is the line number in the old file. + // Zero for added lines. + oldLineNo int +} + +// New creates a Mapper from unified diff output. +// The diff should be the output of git diff base...HEAD. +func New(diff []byte) (*Mapper, error) { + m := &Mapper{ + files: make(map[string]*fileDiff), + } + + scanner := bufio.NewScanner(bytes.NewReader(diff)) + var currentFile string + var currentHunk *hunk + + for scanner.Scan() { + line := scanner.Text() + + // Detect file header: "diff --git a/path b/path" + if strings.HasPrefix(line, "diff --git ") { + currentFile = "" + currentHunk = nil + continue + } + + // Detect new file path: "+++ b/path" + if path, ok := strings.CutPrefix(line, "+++ b/"); ok { + currentFile = path + if _, ok := m.files[currentFile]; !ok { + m.files[currentFile] = &fileDiff{} + } + continue + } + + // Detect rename/copy target: + // "rename to path" or "copy to path" + if path, ok := strings.CutPrefix(line, "rename to "); ok { + currentFile = path + if _, ok := m.files[currentFile]; !ok { + m.files[currentFile] = &fileDiff{} + } + continue + } + if path, ok := strings.CutPrefix(line, "copy to "); ok { + currentFile = path + if _, ok := m.files[currentFile]; !ok { + m.files[currentFile] = &fileDiff{} + } + continue + } + + // Skip if no file context yet. + if currentFile == "" { + continue + } + + // Detect hunk header: "@@ -old,count +new,count @@" + if strings.HasPrefix(line, "@@ ") { + h, err := parseHunkHeader(line) + if err != nil { + return nil, fmt.Errorf( + "parse hunk header %q: %w", + line, err, + ) + } + fd := m.files[currentFile] + fd.hunks = append(fd.hunks, h) + currentHunk = &fd.hunks[len(fd.hunks)-1] + continue + } + + // Process diff lines within a hunk. + if currentHunk == nil || len(line) == 0 { + continue + } + + op := line[0] + switch op { + case ' ': + // Context line: present in both old and new. + dl := diffLine{ + op: ' ', + oldLineNo: currentHunk.oldStart + countOp(currentHunk.lines, ' ', '-'), + newLineNo: currentHunk.newStart + countOp(currentHunk.lines, ' ', '+'), + } + currentHunk.lines = append(currentHunk.lines, dl) + case '+': + // Added line: only in new. + dl := diffLine{ + op: '+', + newLineNo: currentHunk.newStart + countOp(currentHunk.lines, ' ', '+'), + } + currentHunk.lines = append(currentHunk.lines, dl) + case '-': + // Deleted line: only in old. + dl := diffLine{ + op: '-', + oldLineNo: currentHunk.oldStart + countOp(currentHunk.lines, ' ', '-'), + } + currentHunk.lines = append(currentHunk.lines, dl) + case '\\': + // "\ No newline at end of file" — skip. + default: + // Unknown line type — skip. + } + } + + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("scan diff: %w", err) + } + + return m, nil +} + +// Map converts a working-tree file:line reference +// to diff coordinates suitable for forge inline comments. +// +// Returns the file path, the diff line number, +// and the side ("LEFT" or "RIGHT"). +// +// Returns an error if the file or line +// is not part of the diff. +func (m *Mapper) Map(file string, line int) ( + path string, diffLine int, side string, err error, +) { + fd, ok := m.files[file] + if !ok { + return "", 0, "", + fmt.Errorf("file %q not in diff", file) + } + + for _, h := range fd.hunks { + for _, dl := range h.lines { + if dl.newLineNo == line && + (dl.op == '+' || dl.op == ' ') { + return file, dl.newLineNo, "RIGHT", nil + } + } + } + + return "", 0, "", fmt.Errorf( + "line %d of %q not in diff", line, file, + ) +} + +// Files returns all file paths present in the diff. +func (m *Mapper) Files() []string { + var files []string + for f := range m.files { + files = append(files, f) + } + return files +} + +// parseHunkHeader parses "@@ -old,count +new,count @@". +func parseHunkHeader(line string) (hunk, error) { + // Strip "@@ " prefix and " @@..." suffix. + line = strings.TrimPrefix(line, "@@ ") + idx := strings.Index(line, " @@") + if idx < 0 { + return hunk{}, + errors.New("missing @@ terminator") + } + line = line[:idx] + + parts := strings.SplitN(line, " ", 2) + if len(parts) != 2 { + return hunk{}, + errors.New("expected old and new ranges") + } + + oldStart, oldCount, err := parseRange( + strings.TrimPrefix(parts[0], "-"), + ) + if err != nil { + return hunk{}, + fmt.Errorf("parse old range: %w", err) + } + + newStart, newCount, err := parseRange( + strings.TrimPrefix(parts[1], "+"), + ) + if err != nil { + return hunk{}, + fmt.Errorf("parse new range: %w", err) + } + + return hunk{ + oldStart: oldStart, + oldCount: oldCount, + newStart: newStart, + newCount: newCount, + }, nil +} + +// parseRange parses "start,count" or "start" (count=1). +func parseRange(s string) (start, count int, err error) { + parts := strings.SplitN(s, ",", 2) + start, err = strconv.Atoi(parts[0]) + if err != nil { + return 0, 0, + fmt.Errorf("parse start: %w", err) + } + if len(parts) == 1 { + return start, 1, nil + } + count, err = strconv.Atoi(parts[1]) + if err != nil { + return 0, 0, + fmt.Errorf("parse count: %w", err) + } + return start, count, nil +} + +// countOp counts lines in the hunk +// that match any of the given operations. +func countOp(lines []diffLine, ops ...byte) int { + n := 0 + for _, l := range lines { + if slices.Contains(ops, l.op) { + n++ + } + } + return n +} diff --git a/internal/diffmap/mapper_test.go b/internal/diffmap/mapper_test.go new file mode 100644 index 000000000..6abbb5fe1 --- /dev/null +++ b/internal/diffmap/mapper_test.go @@ -0,0 +1,198 @@ +package diffmap + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMapper_Map(t *testing.T) { + tests := []struct { + name string + diff string + file string + line int + wantPath string + wantLine int + wantSide string + wantErr string + }{ + { + name: "AddedLine", + diff: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -1,3 +1,4 @@ + package main + ++func hello() {} + func main() {} +`, + file: "main.go", + line: 3, + wantPath: "main.go", + wantLine: 3, + wantSide: "RIGHT", + }, + { + name: "ContextLine", + diff: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -1,3 +1,4 @@ + package main + ++func hello() {} + func main() {} +`, + file: "main.go", + line: 1, + wantPath: "main.go", + wantLine: 1, + wantSide: "RIGHT", + }, + { + name: "MultipleHunks", + diff: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -1,3 +1,4 @@ + package main + ++import "fmt" + func main() {} +@@ -10,3 +11,4 @@ + func foo() {} + ++func bar() {} + func baz() {} +`, + file: "main.go", + line: 13, + wantPath: "main.go", + wantLine: 13, + wantSide: "RIGHT", + }, + { + name: "FileNotInDiff", + diff: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -1,3 +1,4 @@ + package main + ++func hello() {} + func main() {} +`, + file: "other.go", + line: 1, + wantErr: `file "other.go" not in diff`, + }, + { + name: "LineNotInDiff", + diff: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -1,3 +1,4 @@ + package main + ++func hello() {} + func main() {} +`, + file: "main.go", + line: 100, + wantErr: `line 100 of "main.go" not in diff`, + }, + { + name: "MultipleFiles", + diff: `diff --git a/a.go b/a.go +--- a/a.go ++++ b/a.go +@@ -1,2 +1,3 @@ + package a ++func A() {} + +diff --git a/b.go b/b.go +--- a/b.go ++++ b/b.go +@@ -1,2 +1,3 @@ + package b ++func B() {} + +`, + file: "b.go", + line: 2, + wantPath: "b.go", + wantLine: 2, + wantSide: "RIGHT", + }, + { + name: "NewFile", + diff: `diff --git a/new.go b/new.go +new file mode 100644 +--- /dev/null ++++ b/new.go +@@ -0,0 +1,3 @@ ++package new ++ ++func New() {} +`, + file: "new.go", + line: 1, + wantPath: "new.go", + wantLine: 1, + wantSide: "RIGHT", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + m, err := New([]byte(tt.diff)) + require.NoError(t, err) + + path, line, side, err := m.Map(tt.file, tt.line) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.wantPath, path) + assert.Equal(t, tt.wantLine, line) + assert.Equal(t, tt.wantSide, side) + }) + } +} + +func TestMapper_Files(t *testing.T) { + diff := `diff --git a/a.go b/a.go +--- a/a.go ++++ b/a.go +@@ -1,2 +1,3 @@ + package a ++func A() {} + +diff --git a/b.go b/b.go +--- a/b.go ++++ b/b.go +@@ -1,2 +1,3 @@ + package b ++func B() {} + +` + m, err := New([]byte(diff)) + require.NoError(t, err) + + files := m.Files() + assert.Len(t, files, 2) + assert.Contains(t, files, "a.go") + assert.Contains(t, files, "b.go") +} + +func TestNew_EmptyDiff(t *testing.T) { + m, err := New([]byte("")) + require.NoError(t, err) + assert.Empty(t, m.Files()) +}