From ee2a55977b03049cc4f4f3003b2c1eb9c60fec01 Mon Sep 17 00:00:00 2001 From: Edmund Kohlwey Date: Thu, 14 May 2026 09:33:52 -0400 Subject: [PATCH 1/2] branch comment: Add diff mapping and staging Review comments are entered against working-tree lines, while forges anchor them to positions in a change diff. Add `diffmap.Mapper` to bridge those coordinates. Persist staged comments by branch so upstack commands can assemble and edit a review before submission. --- internal/diffmap/mapper.go | 281 ++++++++++++++++++++ internal/diffmap/mapper_test.go | 198 ++++++++++++++ internal/spice/state/staged_comment.go | 121 +++++++++ internal/spice/state/staged_comment_test.go | 128 +++++++++ 4 files changed, 728 insertions(+) create mode 100644 internal/diffmap/mapper.go create mode 100644 internal/diffmap/mapper_test.go create mode 100644 internal/spice/state/staged_comment.go create mode 100644 internal/spice/state/staged_comment_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()) +} diff --git a/internal/spice/state/staged_comment.go b/internal/spice/state/staged_comment.go new file mode 100644 index 000000000..11079e783 --- /dev/null +++ b/internal/spice/state/staged_comment.go @@ -0,0 +1,121 @@ +package state + +import ( + "context" + "errors" + "fmt" + "path" + + "go.abhg.dev/gs/internal/spice/state/storage" +) + +// _stagedCommentsDir is the directory holding staged comments +// for branches that have not yet been submitted as reviews. +const _stagedCommentsDir = "staged-comments" + +// StagedComment is a draft inline comment +// waiting to be batch-submitted as part of a review. +type StagedComment struct { + // ID is a local auto-increment identifier + // unique within the branch's staged comments. + ID int `json:"id"` + + // File is the file path relative to the repository root. + File string `json:"file"` + + // Line is the line number in the new version of the file. + Line int `json:"line"` + + // Body is the markdown body of the comment. + Body string `json:"body"` + + // ThreadID is set when replying to an existing thread. + // The format is forge-specific. + ThreadID string `json:"threadID,omitempty"` +} + +// StagedComments is the collection of staged comments +// for a branch. +type StagedComments struct { + // NextID is the next ID to assign + // to a new staged comment. + NextID int `json:"nextID"` + + // Comments are the staged comments. + Comments []StagedComment `json:"comments"` +} + +func (s *Store) stagedCommentsJSON(branch string) string { + return path.Join(_stagedCommentsDir, branch) +} + +// SaveStagedComments saves the staged comments +// for the given branch. +// If staged comments already exist for the branch, +// they will be overwritten. +func (s *Store) SaveStagedComments( + ctx context.Context, + branch string, + comments *StagedComments, +) error { + err := s.db.Set( + ctx, + s.stagedCommentsJSON(branch), + comments, + fmt.Sprintf( + "%v: save staged comments", branch, + ), + ) + if err != nil { + return fmt.Errorf( + "set staged comments: %w", err, + ) + } + return nil +} + +// LoadStagedComments retrieves staged comments +// for the given branch. +// Returns nil if no staged comments exist. +func (s *Store) LoadStagedComments( + ctx context.Context, + branch string, +) (*StagedComments, error) { + var comments StagedComments + err := s.db.Get( + ctx, + s.stagedCommentsJSON(branch), + &comments, + ) + if err != nil { + if errors.Is(err, storage.ErrNotExist) { + return nil, nil + } + return nil, fmt.Errorf( + "get staged comments: %w", err, + ) + } + return &comments, nil +} + +// ClearStagedComments removes staged comments +// for the given branch. +// This is a no-op if no staged comments exist. +func (s *Store) ClearStagedComments( + ctx context.Context, + branch string, +) error { + err := s.db.Delete( + ctx, + s.stagedCommentsJSON(branch), + fmt.Sprintf( + "%v: clear staged comments", branch, + ), + ) + if err != nil { + return fmt.Errorf( + "delete staged comments: %w", err, + ) + } + return nil +} diff --git a/internal/spice/state/staged_comment_test.go b/internal/spice/state/staged_comment_test.go new file mode 100644 index 000000000..0f8da8bc6 --- /dev/null +++ b/internal/spice/state/staged_comment_test.go @@ -0,0 +1,128 @@ +package state_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.abhg.dev/gs/internal/silog/silogtest" + "go.abhg.dev/gs/internal/spice/state" + "go.abhg.dev/gs/internal/spice/state/storage" +) + +func TestStagedComments(t *testing.T) { + ctx := t.Context() + db := storage.NewDB(make(storage.MapBackend)) + + _, err := state.InitStore(ctx, state.InitStoreRequest{ + DB: db, + Trunk: "main", + }) + require.NoError(t, err) + + store, err := state.OpenStore(ctx, db, silogtest.New(t)) + require.NoError(t, err) + + t.Run("LoadEmpty", func(t *testing.T) { + got, err := store.LoadStagedComments(ctx, "feat") + require.NoError(t, err) + assert.Nil(t, got) + }) + + t.Run("SaveAndLoad", func(t *testing.T) { + comments := &state.StagedComments{ + NextID: 3, + Comments: []state.StagedComment{ + { + ID: 1, + File: "main.go", + Line: 42, + Body: "Consider using a const here.", + }, + { + ID: 2, + File: "handler.go", + Line: 15, + Body: "I agree with your suggestion.", + ThreadID: "thread-abc", + }, + }, + } + + err := store.SaveStagedComments(ctx, "feat", comments) + require.NoError(t, err) + + got, err := store.LoadStagedComments(ctx, "feat") + require.NoError(t, err) + require.NotNil(t, got) + + assert.Equal(t, 3, got.NextID) + assert.Len(t, got.Comments, 2) + assert.Equal(t, "main.go", got.Comments[0].File) + assert.Equal(t, 42, got.Comments[0].Line) + assert.Equal(t, "thread-abc", got.Comments[1].ThreadID) + }) + + t.Run("Overwrite", func(t *testing.T) { + comments := &state.StagedComments{ + NextID: 2, + Comments: []state.StagedComment{ + {ID: 1, File: "new.go", Line: 1, Body: "New comment"}, + }, + } + + err := store.SaveStagedComments(ctx, "feat", comments) + require.NoError(t, err) + + got, err := store.LoadStagedComments(ctx, "feat") + require.NoError(t, err) + require.NotNil(t, got) + + assert.Len(t, got.Comments, 1) + assert.Equal(t, "new.go", got.Comments[0].File) + }) + + t.Run("Clear", func(t *testing.T) { + err := store.ClearStagedComments(ctx, "feat") + require.NoError(t, err) + + got, err := store.LoadStagedComments(ctx, "feat") + require.NoError(t, err) + assert.Nil(t, got) + }) + + t.Run("ClearNonExistent", func(t *testing.T) { + // Clearing a branch with no staged comments + // should not error. + err := store.ClearStagedComments(ctx, "nonexistent") + require.NoError(t, err) + }) + + t.Run("MultipleBranches", func(t *testing.T) { + commentsA := &state.StagedComments{ + NextID: 2, + Comments: []state.StagedComment{ + {ID: 1, File: "a.go", Line: 1, Body: "A"}, + }, + } + commentsB := &state.StagedComments{ + NextID: 2, + Comments: []state.StagedComment{ + {ID: 1, File: "b.go", Line: 2, Body: "B"}, + }, + } + + require.NoError(t, + store.SaveStagedComments(ctx, "branch-a", commentsA)) + require.NoError(t, + store.SaveStagedComments(ctx, "branch-b", commentsB)) + + gotA, err := store.LoadStagedComments(ctx, "branch-a") + require.NoError(t, err) + assert.Equal(t, "a.go", gotA.Comments[0].File) + + gotB, err := store.LoadStagedComments(ctx, "branch-b") + require.NoError(t, err) + assert.Equal(t, "b.go", gotB.Comments[0].File) + }) +} From d2f51574d3178853656ef1f0d9b663f787663232 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sun, 23 Aug 2026 21:17:19 -0700 Subject: [PATCH 2/2] reviewdiff: Parse review comment patches Review comment anchors need postimage membership before submission, while ShamHub staleness needs to know whether an old-side line was deleted. The previous mapper coupled those questions to working-tree coordinates and always returned the right side. Parse Git patches into explicit membership and deletion queries. Line ranges must fit within one postimage fragment, and deletion ranges report only removed old-side lines. Staged-comment persistence moves to the command branch that owns its lifecycle. --- go.mod | 1 + go.sum | 2 + internal/diffmap/mapper.go | 281 -------------------- internal/diffmap/mapper_test.go | 198 -------------- internal/reviewdiff/patch.go | 145 ++++++++++ internal/reviewdiff/patch_test.go | 103 +++++++ internal/spice/state/staged_comment.go | 121 --------- internal/spice/state/staged_comment_test.go | 128 --------- 8 files changed, 251 insertions(+), 728 deletions(-) delete mode 100644 internal/diffmap/mapper.go delete mode 100644 internal/diffmap/mapper_test.go create mode 100644 internal/reviewdiff/patch.go create mode 100644 internal/reviewdiff/patch_test.go delete mode 100644 internal/spice/state/staged_comment.go delete mode 100644 internal/spice/state/staged_comment_test.go diff --git a/go.mod b/go.mod index c85fc93d8..0961edf62 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( charm.land/bubbletea/v2 v2.0.6 charm.land/lipgloss/v2 v2.0.3 github.com/alecthomas/kong v1.15.0 + github.com/bluekeyes/go-gitdiff v0.9.0 github.com/buildkite/shellwords v1.0.1 github.com/charmbracelet/colorprofile v0.4.3 github.com/charmbracelet/x/ansi v0.11.7 diff --git a/go.sum b/go.sum index ec549a5c9..4386b8033 100644 --- a/go.sum +++ b/go.sum @@ -16,6 +16,8 @@ github.com/aymanbagabas/go-osc52/v2 v2.0.1 h1:HwpRHbFMcZLEVr42D4p7XBqjyuxQH5SMiE github.com/aymanbagabas/go-osc52/v2 v2.0.1/go.mod h1:uYgXzlJ7ZpABp8OJ+exZzJJhRNQ2ASbcXHWsFqH8hp8= github.com/aymanbagabas/go-udiff v0.4.1 h1:OEIrQ8maEeDBXQDoGCbbTTXYJMYRCRO1fnodZ12Gv5o= github.com/aymanbagabas/go-udiff v0.4.1/go.mod h1:0L9PGwj20lrtmEMeyw4WKJ/TMyDtvAoK9bf2u/mNo3w= +github.com/bluekeyes/go-gitdiff v0.9.0 h1:w+O6lkRBOqfGcwF0Lf6FFHQrhmxM0hCJW5+rbilGuSs= +github.com/bluekeyes/go-gitdiff v0.9.0/go.mod h1:WWAk1Mc6EgWarCrPFO+xeYlujPu98VuLW3Tu+B/85AE= github.com/buildkite/shellwords v1.0.1 h1:88OjMbEBf+EliVB0tizXJynpAM2CKOvYwepg5n8O70M= github.com/buildkite/shellwords v1.0.1/go.mod h1:so0eQnTxgbo58CTYX+4BCx5UuMzvRha9dcKdCKl6NV4= github.com/charmbracelet/colorprofile v0.4.3 h1:QPa1IWkYI+AOB+fE+mg/5/4HRMZcaXex9t5KX76i20Q= diff --git a/internal/diffmap/mapper.go b/internal/diffmap/mapper.go deleted file mode 100644 index d309eae77..000000000 --- a/internal/diffmap/mapper.go +++ /dev/null @@ -1,281 +0,0 @@ -// 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 deleted file mode 100644 index 6abbb5fe1..000000000 --- a/internal/diffmap/mapper_test.go +++ /dev/null @@ -1,198 +0,0 @@ -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()) -} diff --git a/internal/reviewdiff/patch.go b/internal/reviewdiff/patch.go new file mode 100644 index 000000000..1c6b2bdee --- /dev/null +++ b/internal/reviewdiff/patch.go @@ -0,0 +1,145 @@ +// Package reviewdiff answers review-comment questions about a Git patch. +package reviewdiff + +import ( + "bytes" + "fmt" + + "github.com/bluekeyes/go-gitdiff/gitdiff" +) + +// Patch describes the files and line ranges represented by a Git patch. +// +// Postimage queries use destination paths and line numbers. Deletion queries +// use source paths and line numbers because those lines no longer exist in the +// postimage. +type Patch struct { + files map[string][]lineRange + deletions map[string][]lineRange +} + +// Parse parses a Git patch for review-comment queries. +func Parse(src []byte) (*Patch, error) { + files, _, err := gitdiff.Parse(bytes.NewReader(src)) + if err != nil { + return nil, fmt.Errorf("parse Git patch: %w", err) + } + + patch := &Patch{ + files: make(map[string][]lineRange), + deletions: make(map[string][]lineRange), + } + for _, file := range files { + // A destination path is enough to make a file commentable, including + // binary, rename-only, and mode-only changes without text fragments. + if file.NewName != "" { + if _, ok := patch.files[file.NewName]; !ok { + patch.files[file.NewName] = nil + } + } + + for _, fragment := range file.TextFragments { + if file.NewName != "" && fragment.NewLines > 0 { + patch.files[file.NewName] = append( + patch.files[file.NewName], + lineRange{ + start: fragment.NewPosition, + end: fragment.NewPosition + fragment.NewLines - 1, + }, + ) + } + + // Fragment ranges include context. Walk the old-side cursor so + // deletion queries distinguish removed lines from nearby context. + oldLine := fragment.OldPosition + for _, line := range fragment.Lines { + switch line.Op { + case gitdiff.OpContext: + oldLine++ + case gitdiff.OpDelete: + if file.OldName != "" { + patch.deletions[file.OldName] = appendLine( + patch.deletions[file.OldName], + oldLine, + ) + } + oldLine++ + case gitdiff.OpAdd: + } + } + } + } + + return patch, nil +} + +// ContainsFile reports whether path exists in the patch postimage. +func (p *Patch) ContainsFile(path string) bool { + _, ok := p.files[path] + return ok +} + +// ContainsLine reports whether the postimage line is in a patch fragment. +func (p *Patch) ContainsLine(path string, line int) bool { + return p.ContainsLineRange(path, line, line) +} + +// ContainsLineRange reports whether every line in the inclusive postimage +// range is in one patch fragment. +func (p *Patch) ContainsLineRange(path string, start, end int) bool { + if start <= 0 || end < start { + return false + } + + for _, fragment := range p.files[path] { + if fragment.contains(int64(start), int64(end)) { + return true + } + } + return false +} + +// DeletesLine reports whether the patch deletes the source line. +func (p *Patch) DeletesLine(path string, line int) bool { + return p.DeletesLineRange(path, line, line) +} + +// DeletesLineRange reports whether the patch deletes any source line in the +// inclusive range. +func (p *Patch) DeletesLineRange(path string, start, end int) bool { + if start <= 0 || end < start { + return false + } + + for _, deletion := range p.deletions[path] { + if deletion.overlaps(int64(start), int64(end)) { + return true + } + } + return false +} + +// lineRange is an inclusive interval in one side of a file diff. +type lineRange struct { + start int64 + end int64 +} + +func (r lineRange) contains(start, end int64) bool { + return r.start <= start && end <= r.end +} + +func (r lineRange) overlaps(start, end int64) bool { + return r.start <= end && start <= r.end +} + +// appendLine adds a deleted line while coalescing consecutive lines into a +// compact range. +func appendLine(ranges []lineRange, line int64) []lineRange { + if len(ranges) == 0 || ranges[len(ranges)-1].end+1 != line { + return append(ranges, lineRange{start: line, end: line}) + } + + ranges[len(ranges)-1].end = line + return ranges +} diff --git a/internal/reviewdiff/patch_test.go b/internal/reviewdiff/patch_test.go new file mode 100644 index 000000000..6c237e47b --- /dev/null +++ b/internal/reviewdiff/patch_test.go @@ -0,0 +1,103 @@ +package reviewdiff_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.abhg.dev/gs/internal/reviewdiff" +) + +func TestPatchContains(t *testing.T) { + patch, err := reviewdiff.Parse([]byte(`diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -1,4 +1,5 @@ + package main + ++import "fmt" + func main() {} + +@@ -10,3 +11,4 @@ func helper() { + work() ++ fmt.Println("done") + } + +diff --git "a/with space.go" "b/with space.go" +--- "a/with space.go" ++++ "b/with space.go" +@@ -1 +1,2 @@ + package spaced ++var Added = true + +diff --git a/old.go b/new.go +similarity index 100% +rename from old.go +rename to new.go + +diff --git a/script.sh b/script.sh +old mode 100644 +new mode 100755 +`)) + require.NoError(t, err) + + assert.True(t, patch.ContainsFile("main.go")) + assert.True(t, patch.ContainsFile("with space.go")) + assert.True(t, patch.ContainsFile("new.go")) + assert.True(t, patch.ContainsFile("script.sh")) + assert.False(t, patch.ContainsFile("old.go")) + assert.False(t, patch.ContainsFile("missing.go")) + + assert.True(t, patch.ContainsLine("main.go", 3)) + assert.True(t, patch.ContainsLine("main.go", 13)) + assert.False(t, patch.ContainsLine("main.go", 8)) + assert.False(t, patch.ContainsLine("main.go", 0)) + assert.True(t, patch.ContainsLine("with space.go", 2)) + + assert.True(t, patch.ContainsLineRange("main.go", 1, 5)) + assert.True(t, patch.ContainsLineRange("main.go", 11, 14)) + assert.False(t, patch.ContainsLineRange("main.go", 5, 11)) + assert.False(t, patch.ContainsLineRange("main.go", 4, 2)) +} + +func TestPatchDeletes(t *testing.T) { + patch, err := reviewdiff.Parse([]byte(`diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -2,5 +2,4 @@ package main + context +-delete one +-delete two ++replacement + more context + last context +diff --git a/old.go b/new.go +similarity index 80% +rename from old.go +rename to new.go +@@ -10,3 +10,2 @@ + keep +-removed + keep too +`)) + require.NoError(t, err) + + assert.True(t, patch.DeletesLine("main.go", 3)) + assert.True(t, patch.DeletesLine("main.go", 4)) + assert.False(t, patch.DeletesLine("main.go", 2)) + assert.False(t, patch.DeletesLine("main.go", 5)) + assert.True(t, patch.DeletesLineRange("main.go", 1, 3)) + assert.False(t, patch.DeletesLineRange("main.go", 5, 7)) + assert.False(t, patch.DeletesLineRange("main.go", 0, 4)) + + assert.True(t, patch.DeletesLine("old.go", 11)) + assert.False(t, patch.DeletesLine("new.go", 11)) +} + +func TestParseError(t *testing.T) { + _, err := reviewdiff.Parse([]byte(`detached fragment +@@ -1 +1 @@ +`)) + require.Error(t, err) + assert.ErrorContains(t, err, "parse Git patch") +} diff --git a/internal/spice/state/staged_comment.go b/internal/spice/state/staged_comment.go deleted file mode 100644 index 11079e783..000000000 --- a/internal/spice/state/staged_comment.go +++ /dev/null @@ -1,121 +0,0 @@ -package state - -import ( - "context" - "errors" - "fmt" - "path" - - "go.abhg.dev/gs/internal/spice/state/storage" -) - -// _stagedCommentsDir is the directory holding staged comments -// for branches that have not yet been submitted as reviews. -const _stagedCommentsDir = "staged-comments" - -// StagedComment is a draft inline comment -// waiting to be batch-submitted as part of a review. -type StagedComment struct { - // ID is a local auto-increment identifier - // unique within the branch's staged comments. - ID int `json:"id"` - - // File is the file path relative to the repository root. - File string `json:"file"` - - // Line is the line number in the new version of the file. - Line int `json:"line"` - - // Body is the markdown body of the comment. - Body string `json:"body"` - - // ThreadID is set when replying to an existing thread. - // The format is forge-specific. - ThreadID string `json:"threadID,omitempty"` -} - -// StagedComments is the collection of staged comments -// for a branch. -type StagedComments struct { - // NextID is the next ID to assign - // to a new staged comment. - NextID int `json:"nextID"` - - // Comments are the staged comments. - Comments []StagedComment `json:"comments"` -} - -func (s *Store) stagedCommentsJSON(branch string) string { - return path.Join(_stagedCommentsDir, branch) -} - -// SaveStagedComments saves the staged comments -// for the given branch. -// If staged comments already exist for the branch, -// they will be overwritten. -func (s *Store) SaveStagedComments( - ctx context.Context, - branch string, - comments *StagedComments, -) error { - err := s.db.Set( - ctx, - s.stagedCommentsJSON(branch), - comments, - fmt.Sprintf( - "%v: save staged comments", branch, - ), - ) - if err != nil { - return fmt.Errorf( - "set staged comments: %w", err, - ) - } - return nil -} - -// LoadStagedComments retrieves staged comments -// for the given branch. -// Returns nil if no staged comments exist. -func (s *Store) LoadStagedComments( - ctx context.Context, - branch string, -) (*StagedComments, error) { - var comments StagedComments - err := s.db.Get( - ctx, - s.stagedCommentsJSON(branch), - &comments, - ) - if err != nil { - if errors.Is(err, storage.ErrNotExist) { - return nil, nil - } - return nil, fmt.Errorf( - "get staged comments: %w", err, - ) - } - return &comments, nil -} - -// ClearStagedComments removes staged comments -// for the given branch. -// This is a no-op if no staged comments exist. -func (s *Store) ClearStagedComments( - ctx context.Context, - branch string, -) error { - err := s.db.Delete( - ctx, - s.stagedCommentsJSON(branch), - fmt.Sprintf( - "%v: clear staged comments", branch, - ), - ) - if err != nil { - return fmt.Errorf( - "delete staged comments: %w", err, - ) - } - return nil -} diff --git a/internal/spice/state/staged_comment_test.go b/internal/spice/state/staged_comment_test.go deleted file mode 100644 index 0f8da8bc6..000000000 --- a/internal/spice/state/staged_comment_test.go +++ /dev/null @@ -1,128 +0,0 @@ -package state_test - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "go.abhg.dev/gs/internal/silog/silogtest" - "go.abhg.dev/gs/internal/spice/state" - "go.abhg.dev/gs/internal/spice/state/storage" -) - -func TestStagedComments(t *testing.T) { - ctx := t.Context() - db := storage.NewDB(make(storage.MapBackend)) - - _, err := state.InitStore(ctx, state.InitStoreRequest{ - DB: db, - Trunk: "main", - }) - require.NoError(t, err) - - store, err := state.OpenStore(ctx, db, silogtest.New(t)) - require.NoError(t, err) - - t.Run("LoadEmpty", func(t *testing.T) { - got, err := store.LoadStagedComments(ctx, "feat") - require.NoError(t, err) - assert.Nil(t, got) - }) - - t.Run("SaveAndLoad", func(t *testing.T) { - comments := &state.StagedComments{ - NextID: 3, - Comments: []state.StagedComment{ - { - ID: 1, - File: "main.go", - Line: 42, - Body: "Consider using a const here.", - }, - { - ID: 2, - File: "handler.go", - Line: 15, - Body: "I agree with your suggestion.", - ThreadID: "thread-abc", - }, - }, - } - - err := store.SaveStagedComments(ctx, "feat", comments) - require.NoError(t, err) - - got, err := store.LoadStagedComments(ctx, "feat") - require.NoError(t, err) - require.NotNil(t, got) - - assert.Equal(t, 3, got.NextID) - assert.Len(t, got.Comments, 2) - assert.Equal(t, "main.go", got.Comments[0].File) - assert.Equal(t, 42, got.Comments[0].Line) - assert.Equal(t, "thread-abc", got.Comments[1].ThreadID) - }) - - t.Run("Overwrite", func(t *testing.T) { - comments := &state.StagedComments{ - NextID: 2, - Comments: []state.StagedComment{ - {ID: 1, File: "new.go", Line: 1, Body: "New comment"}, - }, - } - - err := store.SaveStagedComments(ctx, "feat", comments) - require.NoError(t, err) - - got, err := store.LoadStagedComments(ctx, "feat") - require.NoError(t, err) - require.NotNil(t, got) - - assert.Len(t, got.Comments, 1) - assert.Equal(t, "new.go", got.Comments[0].File) - }) - - t.Run("Clear", func(t *testing.T) { - err := store.ClearStagedComments(ctx, "feat") - require.NoError(t, err) - - got, err := store.LoadStagedComments(ctx, "feat") - require.NoError(t, err) - assert.Nil(t, got) - }) - - t.Run("ClearNonExistent", func(t *testing.T) { - // Clearing a branch with no staged comments - // should not error. - err := store.ClearStagedComments(ctx, "nonexistent") - require.NoError(t, err) - }) - - t.Run("MultipleBranches", func(t *testing.T) { - commentsA := &state.StagedComments{ - NextID: 2, - Comments: []state.StagedComment{ - {ID: 1, File: "a.go", Line: 1, Body: "A"}, - }, - } - commentsB := &state.StagedComments{ - NextID: 2, - Comments: []state.StagedComment{ - {ID: 1, File: "b.go", Line: 2, Body: "B"}, - }, - } - - require.NoError(t, - store.SaveStagedComments(ctx, "branch-a", commentsA)) - require.NoError(t, - store.SaveStagedComments(ctx, "branch-b", commentsB)) - - gotA, err := store.LoadStagedComments(ctx, "branch-a") - require.NoError(t, err) - assert.Equal(t, "a.go", gotA.Comments[0].File) - - gotB, err := store.LoadStagedComments(ctx, "branch-b") - require.NoError(t, err) - assert.Equal(t, "b.go", gotB.Comments[0].File) - }) -}