From 16d83b9db28277add817d2967147134f1731804f Mon Sep 17 00:00:00 2001 From: Edmund Kohlwey Date: Thu, 14 May 2026 09:33:52 -0400 Subject: [PATCH 01/11] state: Persist staged review comments Store single-line review comments by branch so branch comment commands can assemble, edit, and submit a review across invocations. Local integer IDs address drafts before the forge assigns review thread and comment IDs. The persisted shape intentionally retains the original single-line anchor contract. File-level and line-range staging remain a separate design decision. --- internal/spice/state/staged_comment.go | 121 ++++++++++++++++++ internal/spice/state/staged_comment_test.go | 128 ++++++++++++++++++++ 2 files changed, 249 insertions(+) create mode 100644 internal/spice/state/staged_comment.go create mode 100644 internal/spice/state/staged_comment_test.go 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 e3890ae25a5ca7f08e35fc363b8808b0cce5e839 Mon Sep 17 00:00:00 2001 From: Edmund Kohlwey Date: Thu, 14 May 2026 09:33:17 -0400 Subject: [PATCH 02/11] branch comment: Add review comment commands Add `gs branch comment` for posting and managing review feedback. The command group can list remote threads, stage and batch comments into a review, reply to or edit comments, and resolve threads. Forge IDs are opaque values rather than strings. Recover IDs from `ReviewRepository.ListReviewThreads` before reply, edit, or resolution operations. --- .../unreleased/Added-20260314-155204.yaml | 3 + branch.go | 3 + branch_comment.go | 62 +++ branch_comment_add.go | 178 ++++++++ branch_comment_edit.go | 224 ++++++++++ branch_comment_list.go | 381 ++++++++++++++++++ branch_comment_resolve.go | 110 +++++ branch_comment_stage.go | 177 ++++++++ branch_comment_submit_staged.go | 191 +++++++++ doc/includes/cli-reference.md | 160 ++++++++ doc/includes/cli-shorthands.md | 2 + internal/git/diff_wt.go | 15 + testdata/help/branch_comment_add.txt | 27 ++ testdata/help/branch_comment_edit.txt | 29 ++ testdata/help/branch_comment_list.txt | 28 ++ testdata/help/branch_comment_resolve.txt | 24 ++ testdata/help/branch_comment_stage.txt | 29 ++ .../help/branch_comment_submit-staged.txt | 25 ++ testdata/help/gs.txt | 40 +- 19 files changed, 1693 insertions(+), 15 deletions(-) create mode 100644 .changes/unreleased/Added-20260314-155204.yaml create mode 100644 branch_comment.go create mode 100644 branch_comment_add.go create mode 100644 branch_comment_edit.go create mode 100644 branch_comment_list.go create mode 100644 branch_comment_resolve.go create mode 100644 branch_comment_stage.go create mode 100644 branch_comment_submit_staged.go create mode 100644 testdata/help/branch_comment_add.txt create mode 100644 testdata/help/branch_comment_edit.txt create mode 100644 testdata/help/branch_comment_list.txt create mode 100644 testdata/help/branch_comment_resolve.txt create mode 100644 testdata/help/branch_comment_stage.txt create mode 100644 testdata/help/branch_comment_submit-staged.txt diff --git a/.changes/unreleased/Added-20260314-155204.yaml b/.changes/unreleased/Added-20260314-155204.yaml new file mode 100644 index 000000000..ffd94f42f --- /dev/null +++ b/.changes/unreleased/Added-20260314-155204.yaml @@ -0,0 +1,3 @@ +kind: Added +body: 'branch comment: Add commands for managing change request review comments from the CLI' +time: 2026-03-14T15:52:04.220487-04:00 diff --git a/branch.go b/branch.go index 160587832..e359605ba 100644 --- a/branch.go +++ b/branch.go @@ -38,6 +38,9 @@ type branchCmd struct { // Pull request management Merge branchMergeCmd `cmd:"" aliases:"m" experiment:"merge" help:"Merge a branch into trunk"` Submit branchSubmitCmd `cmd:"" aliases:"s" help:"Submit a branch"` + + // Comment management + Comment branchCommentCmd `cmd:"" aliases:"cmt" help:"Manage change request comments"` } // BranchPromptConfig defines configuration for the branch tree prompt diff --git a/branch_comment.go b/branch_comment.go new file mode 100644 index 000000000..4f82f440f --- /dev/null +++ b/branch_comment.go @@ -0,0 +1,62 @@ +package main + +import ( + "context" + "fmt" + + "go.abhg.dev/gs/internal/forge" +) + +type branchCommentCmd struct { + List branchCommentListCmd `cmd:"" aliases:"ls" help:"List comments on a change request"` + Stage branchCommentStageCmd `cmd:"" help:"Stage an inline comment for batch submission"` + Add branchCommentAddCmd `cmd:"" help:"Post an inline comment immediately"` + SubmitStaged branchCommentSubmitStagedCmd `cmd:"" aliases:"ss" help:"Submit all staged comments as a review"` + Resolve branchCommentResolveCmd `cmd:"" help:"Resolve or unresolve a review thread"` + Edit branchCommentEditCmd `cmd:"" help:"Edit a comment"` +} + +// loadReviewThreadIDs indexes the forge's native thread identifiers by their +// command-line representation. ReviewThreadID is intentionally opaque, so a +// command must recover the provider-owned value before replying to or resolving +// a thread named by the user. +func loadReviewThreadIDs( + ctx context.Context, + repo forge.ReviewRepository, + changeID forge.ChangeID, +) (map[string]forge.ReviewThreadID, error) { + ids := make(map[string]forge.ReviewThreadID) + for thread, err := range repo.ListReviewThreads(ctx, changeID) { + if err != nil { + return nil, fmt.Errorf("list review threads: %w", err) + } + ids[thread.ID.String()] = thread.ID + } + return ids, nil +} + +// reviewThreadID resolves a user-supplied thread string to the opaque ID value +// returned by the current forge. +func reviewThreadID( + ids map[string]forge.ReviewThreadID, + id string, +) (forge.ReviewThreadID, error) { + threadID, ok := ids[id] + if !ok { + return nil, fmt.Errorf("review thread %q not found", id) + } + return threadID, nil +} + +// reviewThreadSide translates diffmap's textual side into the shared review +// model used by forge implementations. +func reviewThreadSide(side string) (forge.ReviewThreadSide, error) { + switch side { + case "RIGHT": + return forge.ReviewThreadSideRight, nil + case "LEFT": + return forge.ReviewThreadSideLeft, nil + default: + return 0, fmt.Errorf("unknown review thread side %q", side) + } +} diff --git a/branch_comment_add.go b/branch_comment_add.go new file mode 100644 index 000000000..b301935e2 --- /dev/null +++ b/branch_comment_add.go @@ -0,0 +1,178 @@ +package main + +import ( + "context" + "errors" + "fmt" + "strings" + + "go.abhg.dev/gs/internal/diffmap" + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/spice" + "go.abhg.dev/gs/internal/spice/state" + "go.abhg.dev/gs/internal/text" +) + +type branchCommentAddCmd struct { + FileAndLine string `arg:"" optional:"" help:"File and line in the form file.go:42."` + Message string `short:"m" placeholder:"MSG" help:"Comment body. Opens editor if not provided."` + Respond string `placeholder:"THREAD_ID" help:"Thread ID to reply to instead of starting a new thread."` + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch to add comment for. Defaults to current branch."` +} + +func (*branchCommentAddCmd) Help() string { + return text.Dedent(` + Posts an inline comment immediately + on the change request for the current branch. + Provide the file and line number as file.go:42. + + If no message is given with -m, an editor is opened. + + Use --respond to reply to an existing thread + instead of starting a new one. + `) +} + +func (cmd *branchCommentAddCmd) Run( + ctx context.Context, + log *silog.Logger, + wt *git.Worktree, + svc *spice.Service, + repo *git.Repository, + forgeRepo forge.Repository, +) error { + branch := cmd.Branch + if branch == "" { + var err error + branch, err = wt.CurrentBranch(ctx) + if err != nil { + return fmt.Errorf("get current branch: %w", err) + } + } + + var file string + var line int + if cmd.Respond == "" { + if cmd.FileAndLine == "" { + return errors.New( + "file:line argument is required " + + "unless --respond is used", + ) + } + var err error + file, line, err = parseFileAndLine(cmd.FileAndLine) + if err != nil { + return err + } + } + + body := cmd.Message + if body == "" { + var err error + body, err = editCommentBody( + ctx, repo, "" /* initial */) + if err != nil { + return err + } + } + if strings.TrimSpace(body) == "" { + return errors.New("empty comment body, aborting") + } + + b, err := svc.LookupBranch(ctx, branch) + if err != nil { + if errors.Is(err, state.ErrNotExist) { + return fmt.Errorf( + "branch not tracked: %s", branch, + ) + } + return fmt.Errorf("get branch: %w", err) + } + + if b.Change == nil { + return fmt.Errorf( + "no change request for %s; "+ + "submit the branch first with "+ + "'gs branch submit'", + branch, + ) + } + + reviewRepo, ok := forgeRepo.(forge.ReviewRepository) + if !ok { + return errors.New( + "forge does not support review comments", + ) + } + + req := forge.SubmitReviewCommentRequest{ + Body: body, + } + + if cmd.Respond != "" { + threadIDs, err := loadReviewThreadIDs( + ctx, reviewRepo, b.Change.ChangeID(), + ) + if err != nil { + return err + } + req.ReplyTo, err = reviewThreadID(threadIDs, cmd.Respond) + if err != nil { + return err + } + } else { + // New comments are entered in working-tree coordinates. Translate the + // location to the reviewed diff before handing it to the forge. + diff, err := wt.DiffBranchBytes(ctx, b.Base, branch) + if err != nil { + return fmt.Errorf("get diff: %w", err) + } + + mapper, err := diffmap.New(diff) + if err != nil { + return fmt.Errorf("parse diff: %w", err) + } + + path, diffLine, side, err := mapper.Map(file, line) + if err != nil { + return fmt.Errorf( + "map %s:%d to diff: %w", + file, line, err, + ) + } + + reviewSide, err := reviewThreadSide(side) + if err != nil { + return err + } + req.Path = path + req.Range = forge.ReviewThreadLine(diffLine) + req.Side = reviewSide + } + + result, err := reviewRepo.SubmitReview( + ctx, + b.Change.ChangeID(), + forge.SubmitReviewRequest{ + Comments: []forge.SubmitReviewCommentRequest{req}, + }, + ) + if err != nil { + return fmt.Errorf("post review comment: %w", err) + } + if len(result.Comments) != 1 { + return fmt.Errorf( + "post review comment: forge returned %d comment results", + len(result.Comments), + ) + } + + log.Infof( + "Posted comment %s on %s.", + result.Comments[0].ThreadID.String(), + b.Change.ChangeID(), + ) + return nil +} diff --git a/branch_comment_edit.go b/branch_comment_edit.go new file mode 100644 index 000000000..c7ac8ed3d --- /dev/null +++ b/branch_comment_edit.go @@ -0,0 +1,224 @@ +package main + +import ( + "context" + "errors" + "fmt" + "strconv" + "strings" + + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/spice" + "go.abhg.dev/gs/internal/spice/state" + "go.abhg.dev/gs/internal/text" +) + +type branchCommentEditCmd struct { + ID string `arg:"" help:"Comment ID to edit. Use 'sc-N' for staged comments or a forge comment ID."` + Message string `short:"m" placeholder:"MSG" help:"New comment body. Opens editor if not provided."` + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch whose comments to edit. Defaults to current branch."` +} + +func (*branchCommentEditCmd) Help() string { + return text.Dedent(` + Edits the body of a comment. + + For staged comments (sc-N prefix), + the comment is updated in the local staging area. + + For forge comments, the comment is updated + on the remote forge. + + If no message is given with -m, an editor is opened + with the current comment body pre-filled. + `) +} + +func (cmd *branchCommentEditCmd) Run( + ctx context.Context, + log *silog.Logger, + wt *git.Worktree, + svc *spice.Service, + store *state.Store, + repo *git.Repository, + forgeRepo forge.Repository, +) error { + branch := cmd.Branch + if branch == "" { + var err error + branch, err = wt.CurrentBranch(ctx) + if err != nil { + return fmt.Errorf("get current branch: %w", err) + } + } + + // Handle staged comment edits. + if scID, ok := parseStagedCommentID(cmd.ID); ok { + return cmd.editStaged( + ctx, log, store, repo, branch, scID, + ) + } + + // Handle forge comment edits. + return cmd.editForge( + ctx, log, wt, svc, repo, forgeRepo, branch, + ) +} + +func (cmd *branchCommentEditCmd) editStaged( + ctx context.Context, + log *silog.Logger, + store *state.Store, + repo *git.Repository, + branch string, + scID int, +) error { + staged, err := store.LoadStagedComments(ctx, branch) + if err != nil { + return fmt.Errorf("load staged comments: %w", err) + } + if staged == nil { + staged = &state.StagedComments{} + } + + idx := -1 + for i, c := range staged.Comments { + if c.ID == scID { + idx = i + break + } + } + if idx < 0 { + return fmt.Errorf( + "staged comment sc-%d not found", scID, + ) + } + + body := cmd.Message + if body == "" { + var err error + body, err = editCommentBody( + ctx, repo, staged.Comments[idx].Body, + ) + if err != nil { + return err + } + } + if strings.TrimSpace(body) == "" { + return errors.New("empty comment body, aborting") + } + + staged.Comments[idx].Body = body + if err := store.SaveStagedComments( + ctx, branch, staged, + ); err != nil { + return fmt.Errorf("save staged comments: %w", err) + } + + log.Infof("Updated staged comment sc-%d.", scID) + return nil +} + +func (cmd *branchCommentEditCmd) editForge( + ctx context.Context, + log *silog.Logger, + _ *git.Worktree, + svc *spice.Service, + repo *git.Repository, + forgeRepo forge.Repository, + branch string, +) error { + b, err := svc.LookupBranch(ctx, branch) + if err != nil { + if errors.Is(err, state.ErrNotExist) { + return fmt.Errorf( + "branch not tracked: %s", branch, + ) + } + return fmt.Errorf("get branch: %w", err) + } + + if b.Change == nil { + return fmt.Errorf( + "no change request for %s", branch, + ) + } + + reviewRepo, ok := forgeRepo.(forge.ReviewRepository) + if !ok { + return errors.New( + "forge does not support review comments", + ) + } + + editor, ok := forgeRepo.(forge.ReviewCommentEditor) + if !ok { + return errors.New( + "forge does not support review comment editing", + ) + } + + // Recover the provider-owned ID and current body from the listed thread. + var target *forge.ReviewComment + for thread, err := range reviewRepo.ListReviewThreads( + ctx, b.Change.ChangeID(), + ) { + if err != nil { + return fmt.Errorf("list review threads: %w", err) + } + for i := range thread.Comments { + comment := &thread.Comments[i] + if comment.ID.String() == cmd.ID { + target = comment + break + } + } + if target != nil { + break + } + } + if target == nil { + return fmt.Errorf( + "comment %s not found", cmd.ID, + ) + } + + body := cmd.Message + if body == "" { + var err error + body, err = editCommentBody( + ctx, repo, target.Body, + ) + if err != nil { + return err + } + } + if strings.TrimSpace(body) == "" { + return errors.New("empty comment body, aborting") + } + + if err := editor.UpdateReviewComment( + ctx, target.ID, body, + ); err != nil { + return fmt.Errorf("edit comment: %w", err) + } + + log.Infof("Updated comment %s.", cmd.ID) + return nil +} + +// parseStagedCommentID parses "sc-N" into integer N. +// Returns (N, true) on success, (0, false) otherwise. +func parseStagedCommentID(s string) (int, bool) { + after, found := strings.CutPrefix(s, "sc-") + if !found { + return 0, false + } + id, err := strconv.Atoi(after) + if err != nil { + return 0, false + } + return id, true +} diff --git a/branch_comment_list.go b/branch_comment_list.go new file mode 100644 index 000000000..ea5db4b46 --- /dev/null +++ b/branch_comment_list.go @@ -0,0 +1,381 @@ +package main + +import ( + "bufio" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "strings" + "time" + + "github.com/alecthomas/kong" + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/sliceutil" + "go.abhg.dev/gs/internal/spice" + "go.abhg.dev/gs/internal/spice/state" + "go.abhg.dev/gs/internal/text" +) + +type branchCommentListCmd struct { + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch to list comments for. Defaults to current branch."` + Staged bool `help:"Show only staged comments."` + Unresolved bool `help:"Show only unresolved comments."` + JSON bool `name:"json" released:"unreleased" help:"Write to stdout as a stream of JSON objects."` +} + +func (*branchCommentListCmd) Help() string { + return text.Dedent(` + Lists comments on the change request + associated with the current branch. + Use --branch to target a different branch. + + Staged comments that have not yet been submitted + are shown with an 'sc-N' prefix. + + Use --staged to show only staged comments. + Use --unresolved to show only unresolved comments. + + With --json, prints output to stdout + as a stream of JSON objects. + `) +} + +func (cmd *branchCommentListCmd) Run( + ctx context.Context, + kctx *kong.Context, + log *silog.Logger, + wt *git.Worktree, + svc *spice.Service, + store *state.Store, + forgeRepo forge.Repository, +) error { + branch, err := cmd.resolveBranch(ctx, wt) + if err != nil { + return err + } + + staged, forgeComments, err := cmd.loadComments( + ctx, log, svc, store, forgeRepo, branch, + ) + if err != nil { + return err + } + + if cmd.JSON { + return cmd.writeJSON( + kctx.Stdout, staged, forgeComments, + ) + } + return cmd.writeText(log, branch, staged, forgeComments) +} + +func (cmd *branchCommentListCmd) resolveBranch( + ctx context.Context, wt *git.Worktree, +) (string, error) { + if cmd.Branch != "" { + return cmd.Branch, nil + } + branch, err := wt.CurrentBranch(ctx) + if err != nil { + return "", fmt.Errorf("get current branch: %w", err) + } + return branch, nil +} + +func (cmd *branchCommentListCmd) loadComments( + ctx context.Context, + log *silog.Logger, + svc *spice.Service, + store *state.Store, + forgeRepo forge.Repository, + branch string, +) ([]*state.StagedComment, []*listedReviewComment, error) { + staged, err := loadStagedComments(ctx, store, branch) + if err != nil { + return nil, nil, err + } + + if cmd.Staged { + return staged, nil, nil + } + + forgeComments, err := loadForgeComments( + ctx, log, svc, forgeRepo, branch, + ) + if err != nil { + return nil, nil, err + } + + return staged, cmd.filterForge(forgeComments), nil +} + +func loadStagedComments( + ctx context.Context, + store *state.Store, + branch string, +) ([]*state.StagedComment, error) { + staged, err := store.LoadStagedComments(ctx, branch) + if err != nil { + return nil, fmt.Errorf( + "load staged comments: %w", err, + ) + } + if staged == nil { + return nil, nil + } + + refs := make( + []*state.StagedComment, len(staged.Comments), + ) + for i := range staged.Comments { + refs[i] = &staged.Comments[i] + } + return refs, nil +} + +func loadForgeComments( + ctx context.Context, + log *silog.Logger, + svc *spice.Service, + forgeRepo forge.Repository, + branch string, +) ([]*listedReviewComment, error) { + b, err := svc.LookupBranch(ctx, branch) + if err != nil { + if errors.Is(err, state.ErrNotExist) { + return nil, fmt.Errorf( + "branch not tracked: %s", branch, + ) + } + return nil, fmt.Errorf("get branch: %w", err) + } + + if b.Change == nil { + log.Infof( + "No change request found for %s.", branch, + ) + return nil, nil + } + + reviewRepo, ok := forgeRepo.(forge.ReviewRepository) + if !ok { + log.Infof( + "Forge does not support review comments.", + ) + return nil, nil + } + + threads, err := sliceutil.CollectErr( + reviewRepo.ListReviewThreads(ctx, b.Change.ChangeID()), + ) + if err != nil { + return nil, fmt.Errorf( + "list review threads: %w", err, + ) + } + + var comments []*listedReviewComment + for _, thread := range threads { + for i := range thread.Comments { + comments = append(comments, &listedReviewComment{ + Thread: thread, + Comment: &thread.Comments[i], + }) + } + } + return comments, nil +} + +// listedReviewComment keeps a comment paired with the thread that owns its +// location and status. +type listedReviewComment struct { + Thread *forge.ReviewThread + Comment *forge.ReviewComment +} + +func (cmd *branchCommentListCmd) filterForge( + comments []*listedReviewComment, +) []*listedReviewComment { + if !cmd.Unresolved { + return comments + } + + var filtered []*listedReviewComment + for _, c := range comments { + if c.Thread.Resolved == nil || !*c.Thread.Resolved { + filtered = append(filtered, c) + } + } + return filtered +} + +// writeText prints comments in human-readable format. +func (cmd *branchCommentListCmd) writeText( + log *silog.Logger, + branch string, + staged []*state.StagedComment, + forgeComments []*listedReviewComment, +) error { + if len(staged) > 0 { + log.Infof("Staged comments:") + for _, c := range staged { + writeStagedText(log, c) + } + } + + if cmd.Staged && len(staged) == 0 { + log.Infof("No staged comments for %s.", branch) + return nil + } + + if len(forgeComments) > 0 { + log.Infof("Comments:") + for _, c := range forgeComments { + writeForgeText(log, c) + } + } + + if len(forgeComments) == 0 && len(staged) == 0 { + log.Infof("No comments on %s.", branch) + } + return nil +} + +func writeStagedText( + log *silog.Logger, c *state.StagedComment, +) { + location := fmt.Sprintf("%s:%d", c.File, c.Line) + if c.ThreadID != "" { + location = "reply:" + c.ThreadID + } + log.Infof(" sc-%-4d %s", c.ID, location) + writeBodyIndented(log, c.Body) +} + +func writeForgeText( + log *silog.Logger, c *listedReviewComment, +) { + location := fmt.Sprintf( + "%s:%d", c.Thread.Path, c.Thread.Range.StartLine, + ) + threadInfo := "" + if c.Thread.ID != nil { + threadInfo = " [" + c.Thread.ID.String() + "]" + } + log.Infof( + " %-12s %s %s %s%s", + c.Comment.ID.String(), location, c.Comment.Author, + commentStatus(c), threadInfo, + ) + writeBodyIndented(log, c.Comment.Body) +} + +func writeBodyIndented(log *silog.Logger, body string) { + for line := range strings.SplitSeq(body, "\n") { + log.Infof(" %s", line) + } +} + +func commentStatus(c *listedReviewComment) string { + if c.Thread.Outdated != nil && *c.Thread.Outdated { + return "outdated" + } + if c.Thread.Resolved != nil && *c.Thread.Resolved { + return "resolved" + } + return "open" +} + +// writeJSON encodes comments as NDJSON to stdout. +func (cmd *branchCommentListCmd) writeJSON( + w io.Writer, + staged []*state.StagedComment, + forgeComments []*listedReviewComment, +) (retErr error) { + bufw := bufio.NewWriter(w) + defer func() { + retErr = errors.Join(retErr, bufw.Flush()) + }() + + enc := json.NewEncoder(bufw) + for _, c := range staged { + if err := enc.Encode(stagedToJSON(c)); err != nil { + return fmt.Errorf("encode staged: %w", err) + } + } + for _, c := range forgeComments { + if err := enc.Encode(forgeToJSON(c)); err != nil { + return fmt.Errorf("encode forge: %w", err) + } + } + return nil +} + +func stagedToJSON(c *state.StagedComment) jsonComment { + return jsonComment{ + Kind: "staged", + ID: fmt.Sprintf("sc-%d", c.ID), + Path: c.File, + Line: c.Line, + Body: c.Body, + ThreadID: c.ThreadID, + } +} + +func forgeToJSON(c *listedReviewComment) jsonComment { + var createdAt *time.Time + if !c.Comment.CreatedAt.IsZero() { + createdAt = &c.Comment.CreatedAt + } + return jsonComment{ + Kind: "forge", + ID: c.Comment.ID.String(), + Path: c.Thread.Path, + Line: c.Thread.Range.StartLine, + Body: c.Comment.Body, + ThreadID: c.Thread.ID.String(), + Author: c.Comment.Author, + Status: commentStatus(c), + CreatedAt: createdAt, + } +} + +// jsonComment is the JSON representation +// of a comment for --json output. +type jsonComment struct { + // Kind is "staged" or "forge". + Kind string `json:"kind"` + + // ID is the comment identifier. + // For staged comments: "sc-N". + // For forge comments: forge-specific ID. + ID string `json:"id"` + + // Path is the file path relative to the repo root. + Path string `json:"path,omitempty"` + + // Line is the line number in the file. + Line int `json:"line,omitempty"` + + // Body is the full markdown body of the comment. + Body string `json:"body"` + + // ThreadID is the thread identifier, if any. + ThreadID string `json:"threadID,omitempty"` + + // Author is the username of the comment author. + // Only set for forge comments. + Author string `json:"author,omitempty"` + + // Status is "open", "resolved", or "outdated". + // Only set for forge comments. + Status string `json:"status,omitempty"` + + // CreatedAt is the time the comment was created. + // Only set for forge comments. + CreatedAt *time.Time `json:"createdAt,omitempty"` +} diff --git a/branch_comment_resolve.go b/branch_comment_resolve.go new file mode 100644 index 000000000..944a6686e --- /dev/null +++ b/branch_comment_resolve.go @@ -0,0 +1,110 @@ +package main + +import ( + "context" + "errors" + "fmt" + + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/spice" + "go.abhg.dev/gs/internal/spice/state" + "go.abhg.dev/gs/internal/text" +) + +type branchCommentResolveCmd struct { + ThreadID string `arg:"" help:"Thread ID to resolve."` + Unresolve bool `help:"Unresolve the thread instead of resolving it."` + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch whose change request contains the thread. Defaults to current branch."` +} + +func (*branchCommentResolveCmd) Help() string { + return text.Dedent(` + Resolves a review thread on the change request + for the current branch. + + Use --unresolve to mark the thread as unresolved. + + The thread ID is shown in 'gs branch comment list'. + `) +} + +func (cmd *branchCommentResolveCmd) Run( + ctx context.Context, + log *silog.Logger, + wt *git.Worktree, + svc *spice.Service, + forgeRepo forge.Repository, +) error { + branch := cmd.Branch + if branch == "" { + var err error + branch, err = wt.CurrentBranch(ctx) + if err != nil { + return fmt.Errorf("get current branch: %w", err) + } + } + + // Verify branch has a change request. + b, err := svc.LookupBranch(ctx, branch) + if err != nil { + if errors.Is(err, state.ErrNotExist) { + return fmt.Errorf( + "branch not tracked: %s", branch, + ) + } + return fmt.Errorf("get branch: %w", err) + } + + if b.Change == nil { + return fmt.Errorf( + "no change request for %s", branch, + ) + } + + reviewRepo, ok := forgeRepo.(forge.ReviewRepository) + if !ok { + return errors.New( + "forge does not support review comments", + ) + } + + resolver, ok := forgeRepo.(forge.ReviewThreadResolver) + if !ok { + return errors.New( + "forge does not support review thread resolution", + ) + } + + threadIDs, err := loadReviewThreadIDs( + ctx, reviewRepo, b.Change.ChangeID(), + ) + if err != nil { + return err + } + threadID, err := reviewThreadID(threadIDs, cmd.ThreadID) + if err != nil { + return err + } + + if cmd.Unresolve { + if err := resolver.UnresolveReviewThread( + ctx, threadID, + ); err != nil { + return fmt.Errorf( + "unresolve thread: %w", err, + ) + } + log.Infof("Unresolved thread %s.", cmd.ThreadID) + } else { + if err := resolver.ResolveReviewThread( + ctx, threadID, + ); err != nil { + return fmt.Errorf("resolve thread: %w", err) + } + log.Infof("Resolved thread %s.", cmd.ThreadID) + } + + return nil +} diff --git a/branch_comment_stage.go b/branch_comment_stage.go new file mode 100644 index 000000000..b5eb87eec --- /dev/null +++ b/branch_comment_stage.go @@ -0,0 +1,177 @@ +package main + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/spice/state" + "go.abhg.dev/gs/internal/text" + "go.abhg.dev/gs/internal/xec" +) + +type branchCommentStageCmd struct { + FileAndLine string `arg:"" optional:"" help:"File and line in the form file.go:42."` + Message string `short:"m" placeholder:"MSG" help:"Comment body. Opens editor if not provided."` + Respond string `placeholder:"THREAD_ID" help:"Thread ID to reply to instead of starting a new thread."` + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch to stage comment for. Defaults to current branch."` +} + +func (*branchCommentStageCmd) Help() string { + return text.Dedent(` + Stages an inline comment for later batch submission. + Provide the file and line number as file.go:42. + + If no message is given with -m, an editor is opened. + + Use --respond to reply to an existing thread + instead of starting a new one. + + Staged comments are submitted together with + 'gs branch comment submit-staged'. + `) +} + +func (cmd *branchCommentStageCmd) Run( + ctx context.Context, + log *silog.Logger, + wt *git.Worktree, + store *state.Store, + repo *git.Repository, +) error { + branch := cmd.Branch + if branch == "" { + var err error + branch, err = wt.CurrentBranch(ctx) + if err != nil { + return fmt.Errorf("get current branch: %w", err) + } + } + + var file string + var line int + if cmd.Respond == "" { + if cmd.FileAndLine == "" { + return errors.New( + "file:line argument is required " + + "unless --respond is used", + ) + } + var err error + file, line, err = parseFileAndLine(cmd.FileAndLine) + if err != nil { + return err + } + } + + body := cmd.Message + if body == "" { + var err error + body, err = editCommentBody( + ctx, repo, "" /* initial */) + if err != nil { + return err + } + } + if strings.TrimSpace(body) == "" { + return errors.New("empty comment body, aborting") + } + + staged, err := store.LoadStagedComments(ctx, branch) + if err != nil { + return fmt.Errorf("load staged comments: %w", err) + } + if staged == nil { + staged = &state.StagedComments{NextID: 1} + } + + comment := state.StagedComment{ + ID: staged.NextID, + File: file, + Line: line, + Body: body, + ThreadID: cmd.Respond, + } + staged.Comments = append(staged.Comments, comment) + staged.NextID++ + + if err := store.SaveStagedComments( + ctx, branch, staged, + ); err != nil { + return fmt.Errorf("save staged comments: %w", err) + } + + if cmd.Respond != "" { + log.Infof( + "Staged reply sc-%d to thread %s.", + comment.ID, cmd.Respond, + ) + } else { + log.Infof( + "Staged comment sc-%d on %s:%d.", + comment.ID, file, line, + ) + } + return nil +} + +// parseFileAndLine parses a "file.go:42" argument +// into file and line components. +func parseFileAndLine(s string) (string, int, error) { + idx := strings.LastIndex(s, ":") + if idx < 0 { + return "", 0, fmt.Errorf( + "expected file:line format, got %q", s, + ) + } + file := s[:idx] + line, err := strconv.Atoi(s[idx+1:]) + if err != nil { + return "", 0, fmt.Errorf( + "invalid line number in %q: %w", s, err, + ) + } + if line <= 0 { + return "", 0, fmt.Errorf( + "line number must be positive, got %d", line, + ) + } + return file, line, nil +} + +// editCommentBody opens an editor for the user +// to write a comment body. +// initial is pre-filled text (may be empty). +func editCommentBody( + ctx context.Context, + repo *git.Repository, + initial string, +) (string, error) { + tmpFile := filepath.Join( + os.TempDir(), "GS_COMMENT_EDITMSG", + ) + if err := os.WriteFile( + tmpFile, []byte(initial), 0o644, + ); err != nil { + return "", fmt.Errorf("write temp file: %w", err) + } + defer func() { _ = os.Remove(tmpFile) }() + + editor := gitEditor(ctx, repo) + cmd := xec.EditCommand(editor, tmpFile) + if err := cmd.Run(); err != nil { + return "", fmt.Errorf("run editor: %w", err) + } + + content, err := os.ReadFile(tmpFile) + if err != nil { + return "", fmt.Errorf("read temp file: %w", err) + } + return string(content), nil +} diff --git a/branch_comment_submit_staged.go b/branch_comment_submit_staged.go new file mode 100644 index 000000000..3b204d2be --- /dev/null +++ b/branch_comment_submit_staged.go @@ -0,0 +1,191 @@ +package main + +import ( + "context" + "errors" + "fmt" + + "go.abhg.dev/gs/internal/diffmap" + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/spice" + "go.abhg.dev/gs/internal/spice/state" + "go.abhg.dev/gs/internal/text" +) + +type branchCommentSubmitStagedCmd struct { + Body string `placeholder:"BODY" help:"Overall review body."` + Approve bool `help:"Mark the review as approved."` + RequestChanges bool `name:"request-changes" help:"Mark the review as requesting changes."` + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch to submit staged comments for. Defaults to current branch."` +} + +func (*branchCommentSubmitStagedCmd) Help() string { + return text.Dedent(` + Submits all staged comments for the current branch + as a single review on the change request. + + Use --approve or --request-changes + to set the review event type. + Defaults to a comment-only review. + + Use --body to add an overall review body. + `) +} + +func (cmd *branchCommentSubmitStagedCmd) Run( + ctx context.Context, + log *silog.Logger, + wt *git.Worktree, + svc *spice.Service, + store *state.Store, + forgeRepo forge.Repository, +) error { + branch := cmd.Branch + if branch == "" { + var err error + branch, err = wt.CurrentBranch(ctx) + if err != nil { + return fmt.Errorf("get current branch: %w", err) + } + } + + staged, err := store.LoadStagedComments(ctx, branch) + if err != nil { + return fmt.Errorf("load staged comments: %w", err) + } + if staged == nil { + staged = &state.StagedComments{} + } + + if len(staged.Comments) == 0 { + log.Infof("No staged comments to submit.") + return nil + } + + b, err := svc.LookupBranch(ctx, branch) + if err != nil { + if errors.Is(err, state.ErrNotExist) { + return fmt.Errorf( + "branch not tracked: %s", branch, + ) + } + return fmt.Errorf("get branch: %w", err) + } + + if b.Change == nil { + return fmt.Errorf( + "no change request for %s; "+ + "submit the branch first with "+ + "'gs branch submit'", + branch, + ) + } + + reviewRepo, ok := forgeRepo.(forge.ReviewRepository) + if !ok { + return errors.New( + "forge does not support review comments", + ) + } + + // Build diff map for coordinate translation. + diff, err := wt.DiffBranchBytes(ctx, b.Base, branch) + if err != nil { + return fmt.Errorf("get diff: %w", err) + } + + mapper, err := diffmap.New(diff) + if err != nil { + return fmt.Errorf("parse diff: %w", err) + } + + var threadIDs map[string]forge.ReviewThreadID + for _, comment := range staged.Comments { + if comment.ThreadID == "" { + continue + } + threadIDs, err = loadReviewThreadIDs( + ctx, reviewRepo, b.Change.ChangeID(), + ) + if err != nil { + return err + } + break + } + + // Map staged comments to forge requests. + var comments []forge.SubmitReviewCommentRequest + for _, sc := range staged.Comments { + if sc.ThreadID != "" { + // Thread reply: no coordinate mapping needed. + threadID, err := reviewThreadID(threadIDs, sc.ThreadID) + if err != nil { + return fmt.Errorf("sc-%d: %w", sc.ID, err) + } + comments = append(comments, + forge.SubmitReviewCommentRequest{ + Body: sc.Body, + ReplyTo: threadID, + }, + ) + continue + } + + path, diffLine, side, err := mapper.Map( + sc.File, sc.Line, + ) + if err != nil { + return fmt.Errorf( + "sc-%d: map %s:%d to diff: %w", + sc.ID, sc.File, sc.Line, err, + ) + } + + reviewSide, err := reviewThreadSide(side) + if err != nil { + return fmt.Errorf("sc-%d: %w", sc.ID, err) + } + comments = append(comments, + forge.SubmitReviewCommentRequest{ + Path: path, + Range: forge.ReviewThreadLine(diffLine), + Body: sc.Body, + Side: reviewSide, + }, + ) + } + + disposition := forge.ReviewDispositionNone + if cmd.Approve { + disposition = forge.ReviewDispositionApprove + } else if cmd.RequestChanges { + disposition = forge.ReviewDispositionRequestChanges + } + + if _, err := reviewRepo.SubmitReview( + ctx, + b.Change.ChangeID(), + forge.SubmitReviewRequest{ + Body: cmd.Body, + Disposition: disposition, + Comments: comments, + }, + ); err != nil { + return fmt.Errorf("submit review: %w", err) + } + + if err := store.ClearStagedComments( + ctx, branch, + ); err != nil { + return fmt.Errorf("clear staged comments: %w", err) + } + + log.Infof( + "Submitted %d comment(s) as review on %s.", + len(comments), + b.Change.ChangeID(), + ) + return nil +} diff --git a/doc/includes/cli-reference.md b/doc/includes/cli-reference.md index 7255a6cb9..8ee1c803f 100644 --- a/doc/includes/cli-reference.md +++ b/doc/includes/cli-reference.md @@ -1277,6 +1277,166 @@ only if there are multiple CRs in the stack. **Configuration**: [spice.submit.assignees](/cli/config.md#spicesubmitassignees), [spice.submit.draft](/cli/config.md#spicesubmitdraft), [spice.submit.labels](/cli/config.md#spicesubmitlabels), [spice.submit.labels.addWhen](/cli/config.md#spicesubmitlabelsaddwhen), [spice.submit.listTemplatesTimeout](/cli/config.md#spicesubmitlisttemplatestimeout), [spice.submit.navigationComment](/cli/config.md#spicesubmitnavigationcomment), [spice.submit.navigationComment.downstack](/cli/config.md#spicesubmitnavigationcommentdownstack), [spice.submit.navigationComment.trunkComparison](/cli/config.md#spicesubmitnavigationcommenttrunkcomparison), [spice.submit.navigationCommentStyle.marker](/cli/config.md#spicesubmitnavigationcommentstylemarker), [spice.submit.navigationCommentStyle.trunkComparisonText](/cli/config.md#spicesubmitnavigationcommentstyletrunkcomparisontext), [spice.submit.navigationCommentSync](/cli/config.md#spicesubmitnavigationcommentsync), [spice.submit.publish](/cli/config.md#spicesubmitpublish), [spice.submit.reviewers](/cli/config.md#spicesubmitreviewers), [spice.submit.reviewers.addWhen](/cli/config.md#spicesubmitreviewersaddwhen), [spice.submit.skipRestackCheck](/cli/config.md#spicesubmitskiprestackcheck), [spice.submit.template](/cli/config.md#spicesubmittemplate), [spice.submit.web](/cli/config.md#spicesubmitweb) +### git-spice branch comment list {#gs-branch-comment-list} + +``` +gs branch (b) comment (cmt) list (ls) [flags] +``` + +List comments on a change request + +Lists comments on the change request +associated with the current branch. +Use --branch to target a different branch. + +Staged comments that have not yet been submitted +are shown with an 'sc-N' prefix. + +Use --staged to show only staged comments. +Use --unresolved to show only unresolved comments. + +With --json, prints output to stdout +as a stream of JSON objects. + +**Flags** + +* `-b`, `--branch=BRANCH`: Branch to list comments for. Defaults to current branch. +* `--staged`: Show only staged comments. +* `--unresolved`: Show only unresolved comments. +* `--json`: Write to stdout as a stream of JSON objects. :material-tag-hidden:{ title="Released in version" }Unreleased + +### git-spice branch comment stage {#gs-branch-comment-stage} + +``` +gs branch (b) comment (cmt) stage [] [flags] +``` + +Stage an inline comment for batch submission + +Stages an inline comment for later batch submission. +Provide the file and line number as file.go:42. + +If no message is given with -m, an editor is opened. + +Use --respond to reply to an existing thread +instead of starting a new one. + +Staged comments are submitted together with +'gs branch comment submit-staged'. + +**Arguments** + +* `file-and-line`: File and line in the form file.go:42. + +**Flags** + +* `-m`, `--message=MSG`: Comment body. Opens editor if not provided. +* `--respond=THREAD_ID`: Thread ID to reply to instead of starting a new thread. +* `-b`, `--branch=BRANCH`: Branch to stage comment for. Defaults to current branch. + +### git-spice branch comment add {#gs-branch-comment-add} + +``` +gs branch (b) comment (cmt) add [] [flags] +``` + +Post an inline comment immediately + +Posts an inline comment immediately +on the change request for the current branch. +Provide the file and line number as file.go:42. + +If no message is given with -m, an editor is opened. + +Use --respond to reply to an existing thread +instead of starting a new one. + +**Arguments** + +* `file-and-line`: File and line in the form file.go:42. + +**Flags** + +* `-m`, `--message=MSG`: Comment body. Opens editor if not provided. +* `--respond=THREAD_ID`: Thread ID to reply to instead of starting a new thread. +* `-b`, `--branch=BRANCH`: Branch to add comment for. Defaults to current branch. + +### git-spice branch comment submit-staged {#gs-branch-comment-submit-staged} + +``` +gs branch (b) comment (cmt) submit-staged (ss) [flags] +``` + +Submit all staged comments as a review + +Submits all staged comments for the current branch +as a single review on the change request. + +Use --approve or --request-changes +to set the review event type. +Defaults to a comment-only review. + +Use --body to add an overall review body. + +**Flags** + +* `--body=BODY`: Overall review body. +* `--approve`: Mark the review as approved. +* `--request-changes`: Mark the review as requesting changes. +* `-b`, `--branch=BRANCH`: Branch to submit staged comments for. Defaults to current branch. + +### git-spice branch comment resolve {#gs-branch-comment-resolve} + +``` +gs branch (b) comment (cmt) resolve [flags] +``` + +Resolve or unresolve a review thread + +Resolves a review thread on the change request +for the current branch. + +Use --unresolve to mark the thread as unresolved. + +The thread ID is shown in 'gs branch comment list'. + +**Arguments** + +* `thread-id`: Thread ID to resolve. + +**Flags** + +* `--unresolve`: Unresolve the thread instead of resolving it. +* `-b`, `--branch=BRANCH`: Branch whose change request contains the thread. Defaults to current branch. + +### git-spice branch comment edit {#gs-branch-comment-edit} + +``` +gs branch (b) comment (cmt) edit [flags] +``` + +Edit a comment + +Edits the body of a comment. + +For staged comments (sc-N prefix), +the comment is updated in the local staging area. + +For forge comments, the comment is updated +on the remote forge. + +If no message is given with -m, an editor is opened +with the current comment body pre-filled. + +**Arguments** + +* `id`: Comment ID to edit. Use 'sc-N' for staged comments or a forge comment ID. + +**Flags** + +* `-m`, `--message=MSG`: New comment body. Opens editor if not provided. +* `-b`, `--branch=BRANCH`: Branch whose comments to edit. Defaults to current branch. + ## Commit ### git-spice commit create {#gs-commit-create} diff --git a/doc/includes/cli-shorthands.md b/doc/includes/cli-shorthands.md index 064cad722..b3cb86a0e 100644 --- a/doc/includes/cli-shorthands.md +++ b/doc/includes/cli-shorthands.md @@ -1,6 +1,8 @@ | **Shorthand** | **Long form** | | --- | --- | | gs bc | [gs branch create](/cli/reference.md#gs-branch-create) | +| gs bcmtls | [gs branch comment list](/cli/reference.md#gs-branch-comment-list) | +| gs bcmtss | [gs branch comment submit-staged](/cli/reference.md#gs-branch-comment-submit-staged) | | gs bco | [gs branch checkout](/cli/reference.md#gs-branch-checkout) | | gs bd | [gs branch delete](/cli/reference.md#gs-branch-delete) | | gs bdi | [gs branch diff](/cli/reference.md#gs-branch-diff) | diff --git a/internal/git/diff_wt.go b/internal/git/diff_wt.go index a3a805de9..f1686410b 100644 --- a/internal/git/diff_wt.go +++ b/internal/git/diff_wt.go @@ -19,3 +19,18 @@ func (w *Worktree) DiffBranch(ctx context.Context, base, head string) error { } return nil } + +// DiffBranchBytes returns the unified diff output +// between base and head using triple-dot syntax. +func (w *Worktree) DiffBranchBytes( + ctx context.Context, + base, head string, +) ([]byte, error) { + out, err := w.gitCmd( + ctx, "diff", base+"..."+head, + ).Output() + if err != nil { + return nil, fmt.Errorf("diff: %w", err) + } + return out, nil +} diff --git a/testdata/help/branch_comment_add.txt b/testdata/help/branch_comment_add.txt new file mode 100644 index 000000000..a3a596440 --- /dev/null +++ b/testdata/help/branch_comment_add.txt @@ -0,0 +1,27 @@ +Usage: gs branch (b) comment (cmt) add [] [flags] + +Post an inline comment immediately + +Posts an inline comment immediately on the change request for the current +branch. Provide the file and line number as file.go:42. + +If no message is given with -m, an editor is opened. + +Use --respond to reply to an existing thread instead of starting a new one. + +Arguments: + [] File and line in the form file.go:42. + +Flags: + -m, --message=MSG Comment body. Opens editor if not provided. + --respond=THREAD_ID Thread ID to reply to instead of starting a new + thread. + -b, --branch=BRANCH Branch to add comment for. Defaults to current + branch. + +Global Flags: + -h, --help Show help for the command + --version Print version information and quit + -v, --verbose Enable verbose output ($GIT_SPICE_VERBOSE) + -C, --dir=DIR Change to DIR before doing anything + --[no-]prompt Whether to prompt for missing information diff --git a/testdata/help/branch_comment_edit.txt b/testdata/help/branch_comment_edit.txt new file mode 100644 index 000000000..26ef35744 --- /dev/null +++ b/testdata/help/branch_comment_edit.txt @@ -0,0 +1,29 @@ +Usage: gs branch (b) comment (cmt) edit [flags] + +Edit a comment + +Edits the body of a comment. + +For staged comments (sc-N prefix), the comment is updated in the local staging +area. + +For forge comments, the comment is updated on the remote forge. + +If no message is given with -m, an editor is opened with the current comment +body pre-filled. + +Arguments: + Comment ID to edit. Use 'sc-N' for staged comments or a forge comment + ID. + +Flags: + -m, --message=MSG New comment body. Opens editor if not provided. + -b, --branch=BRANCH Branch whose comments to edit. Defaults to current + branch. + +Global Flags: + -h, --help Show help for the command + --version Print version information and quit + -v, --verbose Enable verbose output ($GIT_SPICE_VERBOSE) + -C, --dir=DIR Change to DIR before doing anything + --[no-]prompt Whether to prompt for missing information diff --git a/testdata/help/branch_comment_list.txt b/testdata/help/branch_comment_list.txt new file mode 100644 index 000000000..2ad4dabf5 --- /dev/null +++ b/testdata/help/branch_comment_list.txt @@ -0,0 +1,28 @@ +Usage: gs branch (b) comment (cmt) list (ls) [flags] + +List comments on a change request + +Lists comments on the change request associated with the current branch. +Use --branch to target a different branch. + +Staged comments that have not yet been submitted are shown with an 'sc-N' +prefix. + +Use --staged to show only staged comments. Use --unresolved to show only +unresolved comments. + +With --json, prints output to stdout as a stream of JSON objects. + +Flags: + -b, --branch=BRANCH Branch to list comments for. Defaults to current + branch. + --staged Show only staged comments. + --unresolved Show only unresolved comments. + --json Write to stdout as a stream of JSON objects. + +Global Flags: + -h, --help Show help for the command + --version Print version information and quit + -v, --verbose Enable verbose output ($GIT_SPICE_VERBOSE) + -C, --dir=DIR Change to DIR before doing anything + --[no-]prompt Whether to prompt for missing information diff --git a/testdata/help/branch_comment_resolve.txt b/testdata/help/branch_comment_resolve.txt new file mode 100644 index 000000000..c4ee13472 --- /dev/null +++ b/testdata/help/branch_comment_resolve.txt @@ -0,0 +1,24 @@ +Usage: gs branch (b) comment (cmt) resolve [flags] + +Resolve or unresolve a review thread + +Resolves a review thread on the change request for the current branch. + +Use --unresolve to mark the thread as unresolved. + +The thread ID is shown in 'gs branch comment list'. + +Arguments: + Thread ID to resolve. + +Flags: + --unresolve Unresolve the thread instead of resolving it. + -b, --branch=BRANCH Branch whose change request contains the thread. + Defaults to current branch. + +Global Flags: + -h, --help Show help for the command + --version Print version information and quit + -v, --verbose Enable verbose output ($GIT_SPICE_VERBOSE) + -C, --dir=DIR Change to DIR before doing anything + --[no-]prompt Whether to prompt for missing information diff --git a/testdata/help/branch_comment_stage.txt b/testdata/help/branch_comment_stage.txt new file mode 100644 index 000000000..d8d4e7d2b --- /dev/null +++ b/testdata/help/branch_comment_stage.txt @@ -0,0 +1,29 @@ +Usage: gs branch (b) comment (cmt) stage [] [flags] + +Stage an inline comment for batch submission + +Stages an inline comment for later batch submission. Provide the file and line +number as file.go:42. + +If no message is given with -m, an editor is opened. + +Use --respond to reply to an existing thread instead of starting a new one. + +Staged comments are submitted together with 'gs branch comment submit-staged'. + +Arguments: + [] File and line in the form file.go:42. + +Flags: + -m, --message=MSG Comment body. Opens editor if not provided. + --respond=THREAD_ID Thread ID to reply to instead of starting a new + thread. + -b, --branch=BRANCH Branch to stage comment for. Defaults to current + branch. + +Global Flags: + -h, --help Show help for the command + --version Print version information and quit + -v, --verbose Enable verbose output ($GIT_SPICE_VERBOSE) + -C, --dir=DIR Change to DIR before doing anything + --[no-]prompt Whether to prompt for missing information diff --git a/testdata/help/branch_comment_submit-staged.txt b/testdata/help/branch_comment_submit-staged.txt new file mode 100644 index 000000000..6d6b6c339 --- /dev/null +++ b/testdata/help/branch_comment_submit-staged.txt @@ -0,0 +1,25 @@ +Usage: gs branch (b) comment (cmt) submit-staged (ss) [flags] + +Submit all staged comments as a review + +Submits all staged comments for the current branch as a single review on the +change request. + +Use --approve or --request-changes to set the review event type. Defaults to a +comment-only review. + +Use --body to add an overall review body. + +Flags: + --body=BODY Overall review body. + --approve Mark the review as approved. + --request-changes Mark the review as requesting changes. + -b, --branch=BRANCH Branch to submit staged comments for. Defaults to + current branch. + +Global Flags: + -h, --help Show help for the command + --version Print version information and quit + -v, --verbose Enable verbose output ($GIT_SPICE_VERBOSE) + -C, --dir=DIR Change to DIR before doing anything + --[no-]prompt Whether to prompt for missing information diff --git a/testdata/help/gs.txt b/testdata/help/gs.txt index 43406fde2..68df72df1 100644 --- a/testdata/help/gs.txt +++ b/testdata/help/gs.txt @@ -46,21 +46,31 @@ Stack downstack (ds) restack (r) Restack a branch and its downstack Branch - branch (b) track (tr) Track a branch - branch (b) untrack (untr) Forget a tracked branch - branch (b) checkout (co) Switch to a branch - branch (b) create (c) Create a new branch - branch (b) delete (d,rm) Delete branches - branch (b) fold (fo) Merge a branch into its base - branch (b) split (sp) Split a branch on commits - branch (b) squash (sq) Squash a branch into one commit - branch (b) edit (e) Edit the commits in a branch - branch (b) rename (rn,mv) Rename a branch - branch (b) restack (r) Restack a branch - branch (b) onto (on) Move a branch onto another branch - branch (b) diff (di) Show diff between a branch and its base - branch (b) merge (m) Merge a branch into trunk - branch (b) submit (s) Submit a branch + branch (b) track (tr) Track a branch + branch (b) untrack (untr) Forget a tracked branch + branch (b) checkout (co) Switch to a branch + branch (b) create (c) Create a new branch + branch (b) delete (d,rm) Delete branches + branch (b) fold (fo) Merge a branch into its base + branch (b) split (sp) Split a branch on commits + branch (b) squash (sq) Squash a branch into one commit + branch (b) edit (e) Edit the commits in a branch + branch (b) rename (rn,mv) Rename a branch + branch (b) restack (r) Restack a branch + branch (b) onto (on) Move a branch onto another branch + branch (b) diff (di) Show diff between a branch and its base + branch (b) merge (m) Merge a branch into trunk + branch (b) submit (s) Submit a branch + branch (b) comment (cmt) list (ls) + List comments on a change request + branch (b) comment (cmt) stage + Stage an inline comment for batch submission + branch (b) comment (cmt) add Post an inline comment immediately + branch (b) comment (cmt) submit-staged (ss) + Submit all staged comments as a review + branch (b) comment (cmt) resolve + Resolve or unresolve a review thread + branch (b) comment (cmt) edit Edit a comment Commit commit (c) create (c) Create a new commit From 2ec106af3796a4532d9bf524c5942458187a7417 Mon Sep 17 00:00:00 2001 From: Edmund Kohlwey Date: Thu, 14 May 2026 09:32:45 -0400 Subject: [PATCH 03/11] branch comment: Test review comment workflows Exercise `gs branch comment` workflows against ShamHub. The scripts keep command parsing, staged state, diff mapping, and forge operations inside one end-to-end boundary. They cover immediate and staged submission, listing and JSON output, staged editing, and review-thread discovery. --- testdata/script/branch_comment_add.txt | 38 +++++++++++ .../script/branch_comment_edit_staged.txt | 44 +++++++++++++ testdata/script/branch_comment_list.txt | 47 ++++++++++++++ testdata/script/branch_comment_list_json.txt | 64 +++++++++++++++++++ testdata/script/branch_comment_resolve.txt | 40 ++++++++++++ .../script/branch_comment_stage_submit.txt | 59 +++++++++++++++++ 6 files changed, 292 insertions(+) create mode 100644 testdata/script/branch_comment_add.txt create mode 100644 testdata/script/branch_comment_edit_staged.txt create mode 100644 testdata/script/branch_comment_list.txt create mode 100644 testdata/script/branch_comment_list_json.txt create mode 100644 testdata/script/branch_comment_resolve.txt create mode 100644 testdata/script/branch_comment_stage_submit.txt diff --git a/testdata/script/branch_comment_add.txt b/testdata/script/branch_comment_add.txt new file mode 100644 index 000000000..6dae366a6 --- /dev/null +++ b/testdata/script/branch_comment_add.txt @@ -0,0 +1,38 @@ +# Post an inline comment immediately with 'branch comment add'. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init + +# set up a fake GitHub remote +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a branch with a file and submit it +git add handler.go +gs bc -m 'Add handler' feature1 +gs branch submit --fill +stderr 'Created #' + +# post an inline comment immediately +gs branch comment add handler.go:5 -m 'This needs a context parameter.' +stderr 'Posted comment' + +-- repo/handler.go -- +package main + +import "fmt" + +func handle() { + fmt.Println("handling") +} diff --git a/testdata/script/branch_comment_edit_staged.txt b/testdata/script/branch_comment_edit_staged.txt new file mode 100644 index 000000000..cdd4ddaa0 --- /dev/null +++ b/testdata/script/branch_comment_edit_staged.txt @@ -0,0 +1,44 @@ +# Edit a staged comment before submission. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init + +# set up a fake GitHub remote +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a branch with a file and submit it +git add main.go +gs bc -m 'Add main' feature1 +gs branch submit --fill +stderr 'Created #' + +# stage a comment +gs branch comment stage main.go:3 -m 'Original comment.' +stderr 'Staged comment sc-1' + +# edit the staged comment with -m +gs branch comment edit sc-1 -m 'Updated comment.' +stderr 'Updated staged comment sc-1' + +# verify the edit took effect by listing staged +gs branch comment list --staged +stderr 'Updated comment' + +-- repo/main.go -- +package main + +func main() { + println("hello") +} diff --git a/testdata/script/branch_comment_list.txt b/testdata/script/branch_comment_list.txt new file mode 100644 index 000000000..939cc0f7c --- /dev/null +++ b/testdata/script/branch_comment_list.txt @@ -0,0 +1,47 @@ +# List comments on a change request. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init + +# set up a fake GitHub remote +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a branch with a file and submit it +git add main.go +gs bc -m 'Add main' feature1 +gs branch submit --fill +stderr 'Created #' + +# add an inline comment so there is something to list +gs branch comment add main.go:3 -m 'Consider using a constant.' +stderr 'Posted comment' + +# list should show the comment +gs branch comment list +stderr 'Comments:' +stderr 'main.go:3' +stderr 'Consider using a constant' + +# list on a branch with no CR should say so +gs bc -m 'Another branch' feature2 +gs branch comment list +stderr 'No change request' + +-- repo/main.go -- +package main + +func main() { + println("hello") +} diff --git a/testdata/script/branch_comment_list_json.txt b/testdata/script/branch_comment_list_json.txt new file mode 100644 index 000000000..f54035082 --- /dev/null +++ b/testdata/script/branch_comment_list_json.txt @@ -0,0 +1,64 @@ +# List comments on a change request with --json output. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init + +# set up a fake GitHub remote +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a branch with a file and submit it +git add main.go +gs bc -m 'Add main' feature1 +gs branch submit --fill +stderr 'Created #' + +# add an inline comment so there is something to list +gs branch comment add main.go:3 -m 'Consider using a constant here because it would make the code much more maintainable and readable.' +stderr 'Posted comment' + +# --json should output NDJSON to stdout with forge comment +gs branch comment list --json +stdout '"kind":"forge"' +stdout '"body":"Consider using a constant here because it would make the code much more maintainable and readable."' +stdout '"path":"main.go"' +stdout '"line":3' +stdout '"status":"open"' + +# text mode shows the full body without truncation +gs branch comment list +stderr 'Consider using a constant here because it would make the code much more maintainable and readable.' + +# stage a comment +gs branch comment stage main.go:5 -m 'Add error handling here.' +stderr 'Staged comment' + +# --staged --json should include only staged comments +gs branch comment list --staged --json +cmpenvJSON stdout $WORK/golden/staged_only.json + +# --json without --staged includes both staged and forge +gs branch comment list --json +stdout '"kind":"staged"' +stdout '"kind":"forge"' + +-- repo/main.go -- +package main + +func main() { + println("hello") +} + +-- golden/staged_only.json -- +{"kind":"staged","id":"sc-1","path":"main.go","line":5,"body":"Add error handling here."} diff --git a/testdata/script/branch_comment_resolve.txt b/testdata/script/branch_comment_resolve.txt new file mode 100644 index 000000000..59df68aa8 --- /dev/null +++ b/testdata/script/branch_comment_resolve.txt @@ -0,0 +1,40 @@ +# Resolve and unresolve a review thread. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init + +# set up a fake GitHub remote +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a branch with a file and submit it +git add util.go +gs bc -m 'Add util' feature1 +gs branch submit --fill +stderr 'Created #' + +# post an inline comment to create a thread +gs branch comment add util.go:3 -m 'Rename this function.' +stderr 'Posted comment' + +# get the thread ID from the list output +gs branch comment list +stderr 'thread-' + +-- repo/util.go -- +package main + +func doStuff() { + // stuff +} diff --git a/testdata/script/branch_comment_stage_submit.txt b/testdata/script/branch_comment_stage_submit.txt new file mode 100644 index 000000000..4409fb17d --- /dev/null +++ b/testdata/script/branch_comment_stage_submit.txt @@ -0,0 +1,59 @@ +# Stage inline comments and submit them as a review. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init + +# set up a fake GitHub remote +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a branch with a file and submit it +git add feature.go +gs bc -m 'Add feature' feature1 +gs branch submit --fill +stderr 'Created #' + +# stage a comment on the file +gs branch comment stage feature.go:3 -m 'Consider renaming this function.' +stderr 'Staged comment sc-1 on feature.go:3' + +# stage another comment +gs branch comment stage feature.go:7 -m 'Add error handling here.' +stderr 'Staged comment sc-2 on feature.go:7' + +# list staged comments +gs branch comment list --staged +stderr 'sc-1' +stderr 'feature.go:3' +stderr 'sc-2' +stderr 'feature.go:7' + +# submit staged comments as a review +gs branch comment submit-staged +stderr 'Submitted 2 comment' + +# staged comments should be cleared after submit +gs branch comment list --staged +stderr 'No staged comments' + +-- repo/feature.go -- +package main + +func doWork() { + // does some work +} + +func handleRequest() { + // handles a request +} From ee26e30cd3fbb366d3d9230aa985f8fa32a97717 Mon Sep 17 00:00:00 2001 From: Edmund Kohlwey Date: Mon, 1 Jun 2026 11:27:33 -0700 Subject: [PATCH 04/11] branch comment: Test review thread staleness Exercise the user-visible outdated status after a later ShamHub revision deletes one anchored line while preserving another. Cover both the human-readable list and extension-facing JSON output. --- testdata/script/branch_comment_stale.txt | 75 ++++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 testdata/script/branch_comment_stale.txt diff --git a/testdata/script/branch_comment_stale.txt b/testdata/script/branch_comment_stale.txt new file mode 100644 index 000000000..d6b3743b2 --- /dev/null +++ b/testdata/script/branch_comment_stale.txt @@ -0,0 +1,75 @@ +# Verifies that the 'outdated' flag on inline comments is computed +# automatically from the diff between the comment's commit and the +# change's current head — not just a static seed value. +# +# A comment becomes outdated when the line it was anchored to is +# changed in a subsequent commit. Comments on other lines stay live. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +cd repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init + +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +git add main.go +gs bc -m 'Add main' feature1 +gs branch submit --fill +stderr 'Created #' + +# Two inline comments at different lines on the same head commit. +gs branch comment add main.go:4 -m 'Comment on line 4.' +stderr 'Posted comment' +gs branch comment add main.go:5 -m 'Comment on line 5.' +stderr 'Posted comment' + +# Both comments are fresh: neither is outdated yet. +gs branch comment list +! stderr 'outdated' + +# Amend the file so line 4 changes but line 5 does not, then +# update the change with 'gs ca' + 'gs branch submit --force'. +cp $WORK/extra/main-edited.go main.go +git add main.go +gs ca --no-edit +gs branch submit --force + +# Comment 1 (on line 4) is now outdated; comment 2 (on line 5) +# is not. The list output reflects this: +gs branch comment list +stderr 'Comment on line 4' +stderr 'outdated' +stderr 'Comment on line 5' + +# Sanity: the JSON list (used by the VSCode extension) reports +# outdated as the per-comment status. Comment 1 outdated, comment +# 2 still open. +gs branch comment list --json +stdout '"body":"Comment on line 4\."' +stdout '"body":"Comment on line 5\."' +stdout '"status":"outdated"' +stdout '"status":"open"' + +-- repo/main.go -- +package main + +func main() { + println("hello") + println("world") +} +-- extra/main-edited.go -- +package main + +func main() { + println("greetings") + println("world") +} From eed57f99aaa90bee13e7c6e32ddb56285d94887f Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sun, 23 Aug 2026 21:27:01 -0700 Subject: [PATCH 05/11] branch comment: Use review diff anchors Branch comment locations are coordinates in the selected branch postimage. Check single-line anchors against `reviewdiff.Patch` before submission, retain the requested path and line, and submit them on the right side of the review diff. This removes the working-tree mapping and textual side conversion from immediate and staged submission. Staged state remains single-line; file and range staging are deferred to their own contract. --- branch_comment.go | 13 ---------- branch_comment_add.go | 27 +++++++++------------ branch_comment_submit_staged.go | 31 +++++++++--------------- testdata/script/branch_comment_stale.txt | 8 +++--- 4 files changed, 27 insertions(+), 52 deletions(-) diff --git a/branch_comment.go b/branch_comment.go index 4f82f440f..11f3c1219 100644 --- a/branch_comment.go +++ b/branch_comment.go @@ -47,16 +47,3 @@ func reviewThreadID( } return threadID, nil } - -// reviewThreadSide translates diffmap's textual side into the shared review -// model used by forge implementations. -func reviewThreadSide(side string) (forge.ReviewThreadSide, error) { - switch side { - case "RIGHT": - return forge.ReviewThreadSideRight, nil - case "LEFT": - return forge.ReviewThreadSideLeft, nil - default: - return 0, fmt.Errorf("unknown review thread side %q", side) - } -} diff --git a/branch_comment_add.go b/branch_comment_add.go index b301935e2..7bdba1fa9 100644 --- a/branch_comment_add.go +++ b/branch_comment_add.go @@ -6,9 +6,9 @@ import ( "fmt" "strings" - "go.abhg.dev/gs/internal/diffmap" "go.abhg.dev/gs/internal/forge" "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/reviewdiff" "go.abhg.dev/gs/internal/silog" "go.abhg.dev/gs/internal/spice" "go.abhg.dev/gs/internal/spice/state" @@ -123,33 +123,28 @@ func (cmd *branchCommentAddCmd) Run( return err } } else { - // New comments are entered in working-tree coordinates. Translate the - // location to the reviewed diff before handing it to the forge. + // New comments use the selected branch's postimage coordinates. Check + // that the requested line belongs to its review diff before submission. diff, err := wt.DiffBranchBytes(ctx, b.Base, branch) if err != nil { return fmt.Errorf("get diff: %w", err) } - mapper, err := diffmap.New(diff) + patch, err := reviewdiff.Parse(diff) if err != nil { return fmt.Errorf("parse diff: %w", err) } - - path, diffLine, side, err := mapper.Map(file, line) - if err != nil { + if !patch.ContainsLine(file, line) { return fmt.Errorf( - "map %s:%d to diff: %w", - file, line, err, + "review diff does not contain %s:%d", + file, + line, ) } - reviewSide, err := reviewThreadSide(side) - if err != nil { - return err - } - req.Path = path - req.Range = forge.ReviewThreadLine(diffLine) - req.Side = reviewSide + req.Path = file + req.Range = forge.ReviewThreadLine(line) + req.Side = forge.ReviewThreadSideRight } result, err := reviewRepo.SubmitReview( diff --git a/branch_comment_submit_staged.go b/branch_comment_submit_staged.go index 3b204d2be..759177b33 100644 --- a/branch_comment_submit_staged.go +++ b/branch_comment_submit_staged.go @@ -5,9 +5,9 @@ import ( "errors" "fmt" - "go.abhg.dev/gs/internal/diffmap" "go.abhg.dev/gs/internal/forge" "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/reviewdiff" "go.abhg.dev/gs/internal/silog" "go.abhg.dev/gs/internal/spice" "go.abhg.dev/gs/internal/spice/state" @@ -90,13 +90,14 @@ func (cmd *branchCommentSubmitStagedCmd) Run( ) } - // Build diff map for coordinate translation. + // Staged roots use the selected branch's postimage coordinates. Parse the + // review diff once so every root can be checked before anything is sent. diff, err := wt.DiffBranchBytes(ctx, b.Base, branch) if err != nil { return fmt.Errorf("get diff: %w", err) } - mapper, err := diffmap.New(diff) + patch, err := reviewdiff.Parse(diff) if err != nil { return fmt.Errorf("parse diff: %w", err) } @@ -115,11 +116,9 @@ func (cmd *branchCommentSubmitStagedCmd) Run( break } - // Map staged comments to forge requests. var comments []forge.SubmitReviewCommentRequest for _, sc := range staged.Comments { if sc.ThreadID != "" { - // Thread reply: no coordinate mapping needed. threadID, err := reviewThreadID(threadIDs, sc.ThreadID) if err != nil { return fmt.Errorf("sc-%d: %w", sc.ID, err) @@ -133,26 +132,20 @@ func (cmd *branchCommentSubmitStagedCmd) Run( continue } - path, diffLine, side, err := mapper.Map( - sc.File, sc.Line, - ) - if err != nil { + if !patch.ContainsLine(sc.File, sc.Line) { return fmt.Errorf( - "sc-%d: map %s:%d to diff: %w", - sc.ID, sc.File, sc.Line, err, + "sc-%d: review diff does not contain %s:%d", + sc.ID, + sc.File, + sc.Line, ) } - - reviewSide, err := reviewThreadSide(side) - if err != nil { - return fmt.Errorf("sc-%d: %w", sc.ID, err) - } comments = append(comments, forge.SubmitReviewCommentRequest{ - Path: path, - Range: forge.ReviewThreadLine(diffLine), + Path: sc.File, + Range: forge.ReviewThreadLine(sc.Line), Body: sc.Body, - Side: reviewSide, + Side: forge.ReviewThreadSideRight, }, ) } diff --git a/testdata/script/branch_comment_stale.txt b/testdata/script/branch_comment_stale.txt index d6b3743b2..a821f0b9a 100644 --- a/testdata/script/branch_comment_stale.txt +++ b/testdata/script/branch_comment_stale.txt @@ -1,9 +1,9 @@ -# Verifies that the 'outdated' flag on inline comments is computed +# Verifies that the 'outdated' flag on review comments is computed # automatically from the diff between the comment's commit and the -# change's current head — not just a static seed value. +# change's current head, not just a static seed value. # -# A comment becomes outdated when the line it was anchored to is -# changed in a subsequent commit. Comments on other lines stay live. +# A comment becomes outdated when a subsequent commit deletes or replaces its +# anchored line. Comments on other lines stay live. as 'Test ' at '2024-04-05T16:40:32Z' From 78d82adc999796d052f259e9d166001ca419efec Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sun, 30 Aug 2026 13:08:25 -0700 Subject: [PATCH 06/11] review: Unify comment workflows Move review-comment management from gs branch comment to the top-level gs review namespace. Comments and replies now default to local drafts and use --no-draft for immediate posting. Publish sends drafts as one review, while list, edit, resolve, and reopen expose the remaining workflows with numeric draft references. Keep the draft default fixed and retain the existing ID lifecycle. Persistent allocation, deletion, configurable defaults, and staged file or range anchors remain separate review units. --- .../unreleased/Added-20260314-155204.yaml | 2 +- branch.go | 3 - branch_comment.go | 49 --- branch_comment_add.go | 173 --------- branch_comment_edit.go | 224 ------------ branch_comment_resolve.go | 110 ------ branch_comment_stage.go | 177 ---------- doc/includes/cli-reference.md | 333 +++++++++--------- doc/includes/cli-shorthands.md | 2 - main.go | 1 + review.go | 86 +++++ review_comment.go | 262 ++++++++++++++ review_edit.go | 78 ++++ branch_comment_list.go => review_list.go | 47 ++- ...ment_submit_staged.go => review_publish.go | 24 +- review_reply.go | 87 +++++ review_resolution.go | 133 +++++++ testdata/help/branch_comment_add.txt | 27 -- testdata/help/branch_comment_stage.txt | 29 -- testdata/help/gs.txt | 49 ++- testdata/help/review_comment.txt | 27 ++ ...ranch_comment_edit.txt => review_edit.txt} | 16 +- ...ranch_comment_list.txt => review_list.txt} | 13 +- ...t_submit-staged.txt => review_publish.txt} | 10 +- testdata/help/review_reopen.txt | 21 ++ testdata/help/review_reply.txt | 26 ++ ...comment_resolve.txt => review_resolve.txt} | 13 +- .../script/branch_comment_stage_submit.txt | 59 ---- ...nch_comment_add.txt => review_comment.txt} | 16 +- ...omment_edit_staged.txt => review_edit.txt} | 18 +- ...ranch_comment_list.txt => review_list.txt} | 6 +- ...ent_list_json.txt => review_list_json.txt} | 28 +- testdata/script/review_publish.txt | 57 +++ ...comment_resolve.txt => review_resolve.txt} | 17 +- ...nch_comment_stale.txt => review_stale.txt} | 10 +- 35 files changed, 1088 insertions(+), 1145 deletions(-) delete mode 100644 branch_comment.go delete mode 100644 branch_comment_add.go delete mode 100644 branch_comment_edit.go delete mode 100644 branch_comment_resolve.go delete mode 100644 branch_comment_stage.go create mode 100644 review.go create mode 100644 review_comment.go create mode 100644 review_edit.go rename branch_comment_list.go => review_list.go (88%) rename branch_comment_submit_staged.go => review_publish.go (84%) create mode 100644 review_reply.go create mode 100644 review_resolution.go delete mode 100644 testdata/help/branch_comment_add.txt delete mode 100644 testdata/help/branch_comment_stage.txt create mode 100644 testdata/help/review_comment.txt rename testdata/help/{branch_comment_edit.txt => review_edit.txt} (56%) rename testdata/help/{branch_comment_list.txt => review_list.txt} (71%) rename testdata/help/{branch_comment_submit-staged.txt => review_publish.txt} (68%) create mode 100644 testdata/help/review_reopen.txt create mode 100644 testdata/help/review_reply.txt rename testdata/help/{branch_comment_resolve.txt => review_resolve.txt} (52%) delete mode 100644 testdata/script/branch_comment_stage_submit.txt rename testdata/script/{branch_comment_add.txt => review_comment.txt} (54%) rename testdata/script/{branch_comment_edit_staged.txt => review_edit.txt} (60%) rename testdata/script/{branch_comment_list.txt => review_list.txt} (88%) rename testdata/script/{branch_comment_list_json.txt => review_list_json.txt} (62%) create mode 100644 testdata/script/review_publish.txt rename testdata/script/{branch_comment_resolve.txt => review_resolve.txt} (62%) rename testdata/script/{branch_comment_stale.txt => review_stale.txt} (90%) diff --git a/.changes/unreleased/Added-20260314-155204.yaml b/.changes/unreleased/Added-20260314-155204.yaml index ffd94f42f..54a978f25 100644 --- a/.changes/unreleased/Added-20260314-155204.yaml +++ b/.changes/unreleased/Added-20260314-155204.yaml @@ -1,3 +1,3 @@ kind: Added -body: 'branch comment: Add commands for managing change request review comments from the CLI' +body: 'Add commands for drafting, publishing, and managing review comments' time: 2026-03-14T15:52:04.220487-04:00 diff --git a/branch.go b/branch.go index e359605ba..160587832 100644 --- a/branch.go +++ b/branch.go @@ -38,9 +38,6 @@ type branchCmd struct { // Pull request management Merge branchMergeCmd `cmd:"" aliases:"m" experiment:"merge" help:"Merge a branch into trunk"` Submit branchSubmitCmd `cmd:"" aliases:"s" help:"Submit a branch"` - - // Comment management - Comment branchCommentCmd `cmd:"" aliases:"cmt" help:"Manage change request comments"` } // BranchPromptConfig defines configuration for the branch tree prompt diff --git a/branch_comment.go b/branch_comment.go deleted file mode 100644 index 11f3c1219..000000000 --- a/branch_comment.go +++ /dev/null @@ -1,49 +0,0 @@ -package main - -import ( - "context" - "fmt" - - "go.abhg.dev/gs/internal/forge" -) - -type branchCommentCmd struct { - List branchCommentListCmd `cmd:"" aliases:"ls" help:"List comments on a change request"` - Stage branchCommentStageCmd `cmd:"" help:"Stage an inline comment for batch submission"` - Add branchCommentAddCmd `cmd:"" help:"Post an inline comment immediately"` - SubmitStaged branchCommentSubmitStagedCmd `cmd:"" aliases:"ss" help:"Submit all staged comments as a review"` - Resolve branchCommentResolveCmd `cmd:"" help:"Resolve or unresolve a review thread"` - Edit branchCommentEditCmd `cmd:"" help:"Edit a comment"` -} - -// loadReviewThreadIDs indexes the forge's native thread identifiers by their -// command-line representation. ReviewThreadID is intentionally opaque, so a -// command must recover the provider-owned value before replying to or resolving -// a thread named by the user. -func loadReviewThreadIDs( - ctx context.Context, - repo forge.ReviewRepository, - changeID forge.ChangeID, -) (map[string]forge.ReviewThreadID, error) { - ids := make(map[string]forge.ReviewThreadID) - for thread, err := range repo.ListReviewThreads(ctx, changeID) { - if err != nil { - return nil, fmt.Errorf("list review threads: %w", err) - } - ids[thread.ID.String()] = thread.ID - } - return ids, nil -} - -// reviewThreadID resolves a user-supplied thread string to the opaque ID value -// returned by the current forge. -func reviewThreadID( - ids map[string]forge.ReviewThreadID, - id string, -) (forge.ReviewThreadID, error) { - threadID, ok := ids[id] - if !ok { - return nil, fmt.Errorf("review thread %q not found", id) - } - return threadID, nil -} diff --git a/branch_comment_add.go b/branch_comment_add.go deleted file mode 100644 index 7bdba1fa9..000000000 --- a/branch_comment_add.go +++ /dev/null @@ -1,173 +0,0 @@ -package main - -import ( - "context" - "errors" - "fmt" - "strings" - - "go.abhg.dev/gs/internal/forge" - "go.abhg.dev/gs/internal/git" - "go.abhg.dev/gs/internal/reviewdiff" - "go.abhg.dev/gs/internal/silog" - "go.abhg.dev/gs/internal/spice" - "go.abhg.dev/gs/internal/spice/state" - "go.abhg.dev/gs/internal/text" -) - -type branchCommentAddCmd struct { - FileAndLine string `arg:"" optional:"" help:"File and line in the form file.go:42."` - Message string `short:"m" placeholder:"MSG" help:"Comment body. Opens editor if not provided."` - Respond string `placeholder:"THREAD_ID" help:"Thread ID to reply to instead of starting a new thread."` - Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch to add comment for. Defaults to current branch."` -} - -func (*branchCommentAddCmd) Help() string { - return text.Dedent(` - Posts an inline comment immediately - on the change request for the current branch. - Provide the file and line number as file.go:42. - - If no message is given with -m, an editor is opened. - - Use --respond to reply to an existing thread - instead of starting a new one. - `) -} - -func (cmd *branchCommentAddCmd) Run( - ctx context.Context, - log *silog.Logger, - wt *git.Worktree, - svc *spice.Service, - repo *git.Repository, - forgeRepo forge.Repository, -) error { - branch := cmd.Branch - if branch == "" { - var err error - branch, err = wt.CurrentBranch(ctx) - if err != nil { - return fmt.Errorf("get current branch: %w", err) - } - } - - var file string - var line int - if cmd.Respond == "" { - if cmd.FileAndLine == "" { - return errors.New( - "file:line argument is required " + - "unless --respond is used", - ) - } - var err error - file, line, err = parseFileAndLine(cmd.FileAndLine) - if err != nil { - return err - } - } - - body := cmd.Message - if body == "" { - var err error - body, err = editCommentBody( - ctx, repo, "" /* initial */) - if err != nil { - return err - } - } - if strings.TrimSpace(body) == "" { - return errors.New("empty comment body, aborting") - } - - b, err := svc.LookupBranch(ctx, branch) - if err != nil { - if errors.Is(err, state.ErrNotExist) { - return fmt.Errorf( - "branch not tracked: %s", branch, - ) - } - return fmt.Errorf("get branch: %w", err) - } - - if b.Change == nil { - return fmt.Errorf( - "no change request for %s; "+ - "submit the branch first with "+ - "'gs branch submit'", - branch, - ) - } - - reviewRepo, ok := forgeRepo.(forge.ReviewRepository) - if !ok { - return errors.New( - "forge does not support review comments", - ) - } - - req := forge.SubmitReviewCommentRequest{ - Body: body, - } - - if cmd.Respond != "" { - threadIDs, err := loadReviewThreadIDs( - ctx, reviewRepo, b.Change.ChangeID(), - ) - if err != nil { - return err - } - req.ReplyTo, err = reviewThreadID(threadIDs, cmd.Respond) - if err != nil { - return err - } - } else { - // New comments use the selected branch's postimage coordinates. Check - // that the requested line belongs to its review diff before submission. - diff, err := wt.DiffBranchBytes(ctx, b.Base, branch) - if err != nil { - return fmt.Errorf("get diff: %w", err) - } - - patch, err := reviewdiff.Parse(diff) - if err != nil { - return fmt.Errorf("parse diff: %w", err) - } - if !patch.ContainsLine(file, line) { - return fmt.Errorf( - "review diff does not contain %s:%d", - file, - line, - ) - } - - req.Path = file - req.Range = forge.ReviewThreadLine(line) - req.Side = forge.ReviewThreadSideRight - } - - result, err := reviewRepo.SubmitReview( - ctx, - b.Change.ChangeID(), - forge.SubmitReviewRequest{ - Comments: []forge.SubmitReviewCommentRequest{req}, - }, - ) - if err != nil { - return fmt.Errorf("post review comment: %w", err) - } - if len(result.Comments) != 1 { - return fmt.Errorf( - "post review comment: forge returned %d comment results", - len(result.Comments), - ) - } - - log.Infof( - "Posted comment %s on %s.", - result.Comments[0].ThreadID.String(), - b.Change.ChangeID(), - ) - return nil -} diff --git a/branch_comment_edit.go b/branch_comment_edit.go deleted file mode 100644 index c7ac8ed3d..000000000 --- a/branch_comment_edit.go +++ /dev/null @@ -1,224 +0,0 @@ -package main - -import ( - "context" - "errors" - "fmt" - "strconv" - "strings" - - "go.abhg.dev/gs/internal/forge" - "go.abhg.dev/gs/internal/git" - "go.abhg.dev/gs/internal/silog" - "go.abhg.dev/gs/internal/spice" - "go.abhg.dev/gs/internal/spice/state" - "go.abhg.dev/gs/internal/text" -) - -type branchCommentEditCmd struct { - ID string `arg:"" help:"Comment ID to edit. Use 'sc-N' for staged comments or a forge comment ID."` - Message string `short:"m" placeholder:"MSG" help:"New comment body. Opens editor if not provided."` - Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch whose comments to edit. Defaults to current branch."` -} - -func (*branchCommentEditCmd) Help() string { - return text.Dedent(` - Edits the body of a comment. - - For staged comments (sc-N prefix), - the comment is updated in the local staging area. - - For forge comments, the comment is updated - on the remote forge. - - If no message is given with -m, an editor is opened - with the current comment body pre-filled. - `) -} - -func (cmd *branchCommentEditCmd) Run( - ctx context.Context, - log *silog.Logger, - wt *git.Worktree, - svc *spice.Service, - store *state.Store, - repo *git.Repository, - forgeRepo forge.Repository, -) error { - branch := cmd.Branch - if branch == "" { - var err error - branch, err = wt.CurrentBranch(ctx) - if err != nil { - return fmt.Errorf("get current branch: %w", err) - } - } - - // Handle staged comment edits. - if scID, ok := parseStagedCommentID(cmd.ID); ok { - return cmd.editStaged( - ctx, log, store, repo, branch, scID, - ) - } - - // Handle forge comment edits. - return cmd.editForge( - ctx, log, wt, svc, repo, forgeRepo, branch, - ) -} - -func (cmd *branchCommentEditCmd) editStaged( - ctx context.Context, - log *silog.Logger, - store *state.Store, - repo *git.Repository, - branch string, - scID int, -) error { - staged, err := store.LoadStagedComments(ctx, branch) - if err != nil { - return fmt.Errorf("load staged comments: %w", err) - } - if staged == nil { - staged = &state.StagedComments{} - } - - idx := -1 - for i, c := range staged.Comments { - if c.ID == scID { - idx = i - break - } - } - if idx < 0 { - return fmt.Errorf( - "staged comment sc-%d not found", scID, - ) - } - - body := cmd.Message - if body == "" { - var err error - body, err = editCommentBody( - ctx, repo, staged.Comments[idx].Body, - ) - if err != nil { - return err - } - } - if strings.TrimSpace(body) == "" { - return errors.New("empty comment body, aborting") - } - - staged.Comments[idx].Body = body - if err := store.SaveStagedComments( - ctx, branch, staged, - ); err != nil { - return fmt.Errorf("save staged comments: %w", err) - } - - log.Infof("Updated staged comment sc-%d.", scID) - return nil -} - -func (cmd *branchCommentEditCmd) editForge( - ctx context.Context, - log *silog.Logger, - _ *git.Worktree, - svc *spice.Service, - repo *git.Repository, - forgeRepo forge.Repository, - branch string, -) error { - b, err := svc.LookupBranch(ctx, branch) - if err != nil { - if errors.Is(err, state.ErrNotExist) { - return fmt.Errorf( - "branch not tracked: %s", branch, - ) - } - return fmt.Errorf("get branch: %w", err) - } - - if b.Change == nil { - return fmt.Errorf( - "no change request for %s", branch, - ) - } - - reviewRepo, ok := forgeRepo.(forge.ReviewRepository) - if !ok { - return errors.New( - "forge does not support review comments", - ) - } - - editor, ok := forgeRepo.(forge.ReviewCommentEditor) - if !ok { - return errors.New( - "forge does not support review comment editing", - ) - } - - // Recover the provider-owned ID and current body from the listed thread. - var target *forge.ReviewComment - for thread, err := range reviewRepo.ListReviewThreads( - ctx, b.Change.ChangeID(), - ) { - if err != nil { - return fmt.Errorf("list review threads: %w", err) - } - for i := range thread.Comments { - comment := &thread.Comments[i] - if comment.ID.String() == cmd.ID { - target = comment - break - } - } - if target != nil { - break - } - } - if target == nil { - return fmt.Errorf( - "comment %s not found", cmd.ID, - ) - } - - body := cmd.Message - if body == "" { - var err error - body, err = editCommentBody( - ctx, repo, target.Body, - ) - if err != nil { - return err - } - } - if strings.TrimSpace(body) == "" { - return errors.New("empty comment body, aborting") - } - - if err := editor.UpdateReviewComment( - ctx, target.ID, body, - ); err != nil { - return fmt.Errorf("edit comment: %w", err) - } - - log.Infof("Updated comment %s.", cmd.ID) - return nil -} - -// parseStagedCommentID parses "sc-N" into integer N. -// Returns (N, true) on success, (0, false) otherwise. -func parseStagedCommentID(s string) (int, bool) { - after, found := strings.CutPrefix(s, "sc-") - if !found { - return 0, false - } - id, err := strconv.Atoi(after) - if err != nil { - return 0, false - } - return id, true -} diff --git a/branch_comment_resolve.go b/branch_comment_resolve.go deleted file mode 100644 index 944a6686e..000000000 --- a/branch_comment_resolve.go +++ /dev/null @@ -1,110 +0,0 @@ -package main - -import ( - "context" - "errors" - "fmt" - - "go.abhg.dev/gs/internal/forge" - "go.abhg.dev/gs/internal/git" - "go.abhg.dev/gs/internal/silog" - "go.abhg.dev/gs/internal/spice" - "go.abhg.dev/gs/internal/spice/state" - "go.abhg.dev/gs/internal/text" -) - -type branchCommentResolveCmd struct { - ThreadID string `arg:"" help:"Thread ID to resolve."` - Unresolve bool `help:"Unresolve the thread instead of resolving it."` - Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch whose change request contains the thread. Defaults to current branch."` -} - -func (*branchCommentResolveCmd) Help() string { - return text.Dedent(` - Resolves a review thread on the change request - for the current branch. - - Use --unresolve to mark the thread as unresolved. - - The thread ID is shown in 'gs branch comment list'. - `) -} - -func (cmd *branchCommentResolveCmd) Run( - ctx context.Context, - log *silog.Logger, - wt *git.Worktree, - svc *spice.Service, - forgeRepo forge.Repository, -) error { - branch := cmd.Branch - if branch == "" { - var err error - branch, err = wt.CurrentBranch(ctx) - if err != nil { - return fmt.Errorf("get current branch: %w", err) - } - } - - // Verify branch has a change request. - b, err := svc.LookupBranch(ctx, branch) - if err != nil { - if errors.Is(err, state.ErrNotExist) { - return fmt.Errorf( - "branch not tracked: %s", branch, - ) - } - return fmt.Errorf("get branch: %w", err) - } - - if b.Change == nil { - return fmt.Errorf( - "no change request for %s", branch, - ) - } - - reviewRepo, ok := forgeRepo.(forge.ReviewRepository) - if !ok { - return errors.New( - "forge does not support review comments", - ) - } - - resolver, ok := forgeRepo.(forge.ReviewThreadResolver) - if !ok { - return errors.New( - "forge does not support review thread resolution", - ) - } - - threadIDs, err := loadReviewThreadIDs( - ctx, reviewRepo, b.Change.ChangeID(), - ) - if err != nil { - return err - } - threadID, err := reviewThreadID(threadIDs, cmd.ThreadID) - if err != nil { - return err - } - - if cmd.Unresolve { - if err := resolver.UnresolveReviewThread( - ctx, threadID, - ); err != nil { - return fmt.Errorf( - "unresolve thread: %w", err, - ) - } - log.Infof("Unresolved thread %s.", cmd.ThreadID) - } else { - if err := resolver.ResolveReviewThread( - ctx, threadID, - ); err != nil { - return fmt.Errorf("resolve thread: %w", err) - } - log.Infof("Resolved thread %s.", cmd.ThreadID) - } - - return nil -} diff --git a/branch_comment_stage.go b/branch_comment_stage.go deleted file mode 100644 index b5eb87eec..000000000 --- a/branch_comment_stage.go +++ /dev/null @@ -1,177 +0,0 @@ -package main - -import ( - "context" - "errors" - "fmt" - "os" - "path/filepath" - "strconv" - "strings" - - "go.abhg.dev/gs/internal/git" - "go.abhg.dev/gs/internal/silog" - "go.abhg.dev/gs/internal/spice/state" - "go.abhg.dev/gs/internal/text" - "go.abhg.dev/gs/internal/xec" -) - -type branchCommentStageCmd struct { - FileAndLine string `arg:"" optional:"" help:"File and line in the form file.go:42."` - Message string `short:"m" placeholder:"MSG" help:"Comment body. Opens editor if not provided."` - Respond string `placeholder:"THREAD_ID" help:"Thread ID to reply to instead of starting a new thread."` - Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch to stage comment for. Defaults to current branch."` -} - -func (*branchCommentStageCmd) Help() string { - return text.Dedent(` - Stages an inline comment for later batch submission. - Provide the file and line number as file.go:42. - - If no message is given with -m, an editor is opened. - - Use --respond to reply to an existing thread - instead of starting a new one. - - Staged comments are submitted together with - 'gs branch comment submit-staged'. - `) -} - -func (cmd *branchCommentStageCmd) Run( - ctx context.Context, - log *silog.Logger, - wt *git.Worktree, - store *state.Store, - repo *git.Repository, -) error { - branch := cmd.Branch - if branch == "" { - var err error - branch, err = wt.CurrentBranch(ctx) - if err != nil { - return fmt.Errorf("get current branch: %w", err) - } - } - - var file string - var line int - if cmd.Respond == "" { - if cmd.FileAndLine == "" { - return errors.New( - "file:line argument is required " + - "unless --respond is used", - ) - } - var err error - file, line, err = parseFileAndLine(cmd.FileAndLine) - if err != nil { - return err - } - } - - body := cmd.Message - if body == "" { - var err error - body, err = editCommentBody( - ctx, repo, "" /* initial */) - if err != nil { - return err - } - } - if strings.TrimSpace(body) == "" { - return errors.New("empty comment body, aborting") - } - - staged, err := store.LoadStagedComments(ctx, branch) - if err != nil { - return fmt.Errorf("load staged comments: %w", err) - } - if staged == nil { - staged = &state.StagedComments{NextID: 1} - } - - comment := state.StagedComment{ - ID: staged.NextID, - File: file, - Line: line, - Body: body, - ThreadID: cmd.Respond, - } - staged.Comments = append(staged.Comments, comment) - staged.NextID++ - - if err := store.SaveStagedComments( - ctx, branch, staged, - ); err != nil { - return fmt.Errorf("save staged comments: %w", err) - } - - if cmd.Respond != "" { - log.Infof( - "Staged reply sc-%d to thread %s.", - comment.ID, cmd.Respond, - ) - } else { - log.Infof( - "Staged comment sc-%d on %s:%d.", - comment.ID, file, line, - ) - } - return nil -} - -// parseFileAndLine parses a "file.go:42" argument -// into file and line components. -func parseFileAndLine(s string) (string, int, error) { - idx := strings.LastIndex(s, ":") - if idx < 0 { - return "", 0, fmt.Errorf( - "expected file:line format, got %q", s, - ) - } - file := s[:idx] - line, err := strconv.Atoi(s[idx+1:]) - if err != nil { - return "", 0, fmt.Errorf( - "invalid line number in %q: %w", s, err, - ) - } - if line <= 0 { - return "", 0, fmt.Errorf( - "line number must be positive, got %d", line, - ) - } - return file, line, nil -} - -// editCommentBody opens an editor for the user -// to write a comment body. -// initial is pre-filled text (may be empty). -func editCommentBody( - ctx context.Context, - repo *git.Repository, - initial string, -) (string, error) { - tmpFile := filepath.Join( - os.TempDir(), "GS_COMMENT_EDITMSG", - ) - if err := os.WriteFile( - tmpFile, []byte(initial), 0o644, - ); err != nil { - return "", fmt.Errorf("write temp file: %w", err) - } - defer func() { _ = os.Remove(tmpFile) }() - - editor := gitEditor(ctx, repo) - cmd := xec.EditCommand(editor, tmpFile) - if err := cmd.Run(); err != nil { - return "", fmt.Errorf("run editor: %w", err) - } - - content, err := os.ReadFile(tmpFile) - if err != nil { - return "", fmt.Errorf("read temp file: %w", err) - } - return string(content), nil -} diff --git a/doc/includes/cli-reference.md b/doc/includes/cli-reference.md index 8ee1c803f..d61ca2655 100644 --- a/doc/includes/cli-reference.md +++ b/doc/includes/cli-reference.md @@ -1277,166 +1277,6 @@ only if there are multiple CRs in the stack. **Configuration**: [spice.submit.assignees](/cli/config.md#spicesubmitassignees), [spice.submit.draft](/cli/config.md#spicesubmitdraft), [spice.submit.labels](/cli/config.md#spicesubmitlabels), [spice.submit.labels.addWhen](/cli/config.md#spicesubmitlabelsaddwhen), [spice.submit.listTemplatesTimeout](/cli/config.md#spicesubmitlisttemplatestimeout), [spice.submit.navigationComment](/cli/config.md#spicesubmitnavigationcomment), [spice.submit.navigationComment.downstack](/cli/config.md#spicesubmitnavigationcommentdownstack), [spice.submit.navigationComment.trunkComparison](/cli/config.md#spicesubmitnavigationcommenttrunkcomparison), [spice.submit.navigationCommentStyle.marker](/cli/config.md#spicesubmitnavigationcommentstylemarker), [spice.submit.navigationCommentStyle.trunkComparisonText](/cli/config.md#spicesubmitnavigationcommentstyletrunkcomparisontext), [spice.submit.navigationCommentSync](/cli/config.md#spicesubmitnavigationcommentsync), [spice.submit.publish](/cli/config.md#spicesubmitpublish), [spice.submit.reviewers](/cli/config.md#spicesubmitreviewers), [spice.submit.reviewers.addWhen](/cli/config.md#spicesubmitreviewersaddwhen), [spice.submit.skipRestackCheck](/cli/config.md#spicesubmitskiprestackcheck), [spice.submit.template](/cli/config.md#spicesubmittemplate), [spice.submit.web](/cli/config.md#spicesubmitweb) -### git-spice branch comment list {#gs-branch-comment-list} - -``` -gs branch (b) comment (cmt) list (ls) [flags] -``` - -List comments on a change request - -Lists comments on the change request -associated with the current branch. -Use --branch to target a different branch. - -Staged comments that have not yet been submitted -are shown with an 'sc-N' prefix. - -Use --staged to show only staged comments. -Use --unresolved to show only unresolved comments. - -With --json, prints output to stdout -as a stream of JSON objects. - -**Flags** - -* `-b`, `--branch=BRANCH`: Branch to list comments for. Defaults to current branch. -* `--staged`: Show only staged comments. -* `--unresolved`: Show only unresolved comments. -* `--json`: Write to stdout as a stream of JSON objects. :material-tag-hidden:{ title="Released in version" }Unreleased - -### git-spice branch comment stage {#gs-branch-comment-stage} - -``` -gs branch (b) comment (cmt) stage [] [flags] -``` - -Stage an inline comment for batch submission - -Stages an inline comment for later batch submission. -Provide the file and line number as file.go:42. - -If no message is given with -m, an editor is opened. - -Use --respond to reply to an existing thread -instead of starting a new one. - -Staged comments are submitted together with -'gs branch comment submit-staged'. - -**Arguments** - -* `file-and-line`: File and line in the form file.go:42. - -**Flags** - -* `-m`, `--message=MSG`: Comment body. Opens editor if not provided. -* `--respond=THREAD_ID`: Thread ID to reply to instead of starting a new thread. -* `-b`, `--branch=BRANCH`: Branch to stage comment for. Defaults to current branch. - -### git-spice branch comment add {#gs-branch-comment-add} - -``` -gs branch (b) comment (cmt) add [] [flags] -``` - -Post an inline comment immediately - -Posts an inline comment immediately -on the change request for the current branch. -Provide the file and line number as file.go:42. - -If no message is given with -m, an editor is opened. - -Use --respond to reply to an existing thread -instead of starting a new one. - -**Arguments** - -* `file-and-line`: File and line in the form file.go:42. - -**Flags** - -* `-m`, `--message=MSG`: Comment body. Opens editor if not provided. -* `--respond=THREAD_ID`: Thread ID to reply to instead of starting a new thread. -* `-b`, `--branch=BRANCH`: Branch to add comment for. Defaults to current branch. - -### git-spice branch comment submit-staged {#gs-branch-comment-submit-staged} - -``` -gs branch (b) comment (cmt) submit-staged (ss) [flags] -``` - -Submit all staged comments as a review - -Submits all staged comments for the current branch -as a single review on the change request. - -Use --approve or --request-changes -to set the review event type. -Defaults to a comment-only review. - -Use --body to add an overall review body. - -**Flags** - -* `--body=BODY`: Overall review body. -* `--approve`: Mark the review as approved. -* `--request-changes`: Mark the review as requesting changes. -* `-b`, `--branch=BRANCH`: Branch to submit staged comments for. Defaults to current branch. - -### git-spice branch comment resolve {#gs-branch-comment-resolve} - -``` -gs branch (b) comment (cmt) resolve [flags] -``` - -Resolve or unresolve a review thread - -Resolves a review thread on the change request -for the current branch. - -Use --unresolve to mark the thread as unresolved. - -The thread ID is shown in 'gs branch comment list'. - -**Arguments** - -* `thread-id`: Thread ID to resolve. - -**Flags** - -* `--unresolve`: Unresolve the thread instead of resolving it. -* `-b`, `--branch=BRANCH`: Branch whose change request contains the thread. Defaults to current branch. - -### git-spice branch comment edit {#gs-branch-comment-edit} - -``` -gs branch (b) comment (cmt) edit [flags] -``` - -Edit a comment - -Edits the body of a comment. - -For staged comments (sc-N prefix), -the comment is updated in the local staging area. - -For forge comments, the comment is updated -on the remote forge. - -If no message is given with -m, an editor is opened -with the current comment body pre-filled. - -**Arguments** - -* `id`: Comment ID to edit. Use 'sc-N' for staged comments or a forge comment ID. - -**Flags** - -* `-m`, `--message=MSG`: New comment body. Opens editor if not provided. -* `-b`, `--branch=BRANCH`: Branch whose comments to edit. Defaults to current branch. - ## Commit ### git-spice commit create {#gs-commit-create} @@ -1625,6 +1465,179 @@ This command requires at least Git 2.45. **Configuration**: [spice.commitPick.restack](/cli/config.md#spicecommitpickrestack) +## Review + +### git-spice review comment {#gs-review-comment} + +``` +gs review comment [flags] +``` + +Draft or post a review comment + +Adds a review comment to the change request +for the current branch. +Provide the file and line number as file.go:42. + +Comments are saved as local drafts by default. +Use --no-draft to post immediately. + +If no message is given with -m, an editor is opened. + +**Arguments** + +* `file-and-line`: File and line in the form file.go:42. + +**Flags** + +* `-m`, `--message=MSG`: Comment body. Opens editor if not provided. +* `--[no-]draft`: Save the comment as a local draft instead of posting it. +* `-b`, `--branch=BRANCH`: Branch to comment on. Defaults to the current branch. + +### git-spice review reply {#gs-review-reply} + +``` +gs review reply [flags] +``` + +Draft or post a reply to a review thread + +Replies to a review thread on the change request +for the current branch. + +Replies are saved as local drafts by default. +Use --no-draft to post immediately. + +If no message is given with -m, an editor is opened. + +**Arguments** + +* `thread-id`: Thread ID to reply to. + +**Flags** + +* `-m`, `--message=MSG`: Reply body. Opens editor if not provided. +* `--[no-]draft`: Save the reply as a local draft instead of posting it. +* `-b`, `--branch=BRANCH`: Branch containing the thread. Defaults to the current branch. + +### git-spice review publish {#gs-review-publish} + +``` +gs review publish [flags] +``` + +Publish draft comments as a review + +Publishes all draft comments for the current branch +as a single review on the change request. + +Use --approve or --request-changes +to set the review event type. +Defaults to a comment-only review. + +Use --body to add an overall review body. + +**Flags** + +* `--body=BODY`: Overall review body. +* `--approve`: Mark the review as approved. +* `--request-changes`: Mark the review as requesting changes. +* `-b`, `--branch=BRANCH`: Branch whose draft comments to publish. Defaults to the current branch. + +### git-spice review list {#gs-review-list} + +``` +gs review list (ls) [flags] +``` + +List review comments + +Lists comments on the change request +associated with the current branch. +Use --branch to target a different branch. + +Draft comments are identified by a branch-local integer. + +Use --draft-only to show only draft comments. +Use --unresolved to show only unresolved comments. + +With --json, prints output to stdout +as a stream of JSON objects. + +**Flags** + +* `-b`, `--branch=BRANCH`: Branch to list comments for. Defaults to the current branch. +* `--draft-only`: Show only draft comments. +* `--unresolved`: Show only unresolved comments. +* `--json`: Write to stdout as a stream of JSON objects. :material-tag-hidden:{ title="Released in version" }Unreleased + +### git-spice review edit {#gs-review-edit} + +``` +gs review edit [flags] +``` + +Edit a draft comment + +Edits a local draft comment. + +Use 'gs review list --draft-only' +to find the branch-local draft ID. + +If no message is given with -m, an editor is opened +with the current comment body pre-filled. + +**Arguments** + +* `id`: Draft comment ID to edit. + +**Flags** + +* `-m`, `--message=MSG`: New comment body. Opens editor if not provided. +* `-b`, `--branch=BRANCH`: Branch containing the draft. Defaults to the current branch. + +### git-spice review resolve {#gs-review-resolve} + +``` +gs review resolve [flags] +``` + +Resolve a review thread + +Resolves a review thread on the change request +for the current branch. + +The thread ID is shown in 'gs review list'. + +**Arguments** + +* `thread-id`: Thread ID to resolve. + +**Flags** + +* `-b`, `--branch=BRANCH`: Branch containing the thread. Defaults to the current branch. + +### git-spice review reopen {#gs-review-reopen} + +``` +gs review reopen [flags] +``` + +Reopen a resolved review thread + +Reopens a resolved review thread on the change request +for the current branch. + +The thread ID is shown in 'gs review list'. + +**Arguments** + +* `thread-id`: Thread ID to reopen. + +**Flags** + +* `-b`, `--branch=BRANCH`: Branch containing the thread. Defaults to the current branch. + ## Rebase ### git-spice rebase continue {#gs-rebase-continue} diff --git a/doc/includes/cli-shorthands.md b/doc/includes/cli-shorthands.md index b3cb86a0e..064cad722 100644 --- a/doc/includes/cli-shorthands.md +++ b/doc/includes/cli-shorthands.md @@ -1,8 +1,6 @@ | **Shorthand** | **Long form** | | --- | --- | | gs bc | [gs branch create](/cli/reference.md#gs-branch-create) | -| gs bcmtls | [gs branch comment list](/cli/reference.md#gs-branch-comment-list) | -| gs bcmtss | [gs branch comment submit-staged](/cli/reference.md#gs-branch-comment-submit-staged) | | gs bco | [gs branch checkout](/cli/reference.md#gs-branch-checkout) | | gs bd | [gs branch delete](/cli/reference.md#gs-branch-delete) | | gs bdi | [gs branch diff](/cli/reference.md#gs-branch-diff) | diff --git a/main.go b/main.go index e39fcdab7..b3344400b 100644 --- a/main.go +++ b/main.go @@ -319,6 +319,7 @@ type mainCmd struct { Branch branchCmd `cmd:"" aliases:"b" group:"Branch"` Commit commitCmd `cmd:"" aliases:"c" group:"Commit"` + Review reviewCmd `cmd:"" group:"Review" help:"Manage change request reviews"` Rebase rebaseCmd `cmd:"" aliases:"rb" group:"Rebase"` diff --git a/review.go b/review.go new file mode 100644 index 000000000..591091357 --- /dev/null +++ b/review.go @@ -0,0 +1,86 @@ +package main + +import ( + "context" + "errors" + "fmt" + + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/spice" + "go.abhg.dev/gs/internal/spice/state" +) + +type reviewCmd struct { + Comment reviewCommentCmd `cmd:"" help:"Draft or post a review comment"` + Reply reviewReplyCmd `cmd:"" help:"Draft or post a reply to a review thread"` + Publish reviewPublishCmd `cmd:"" help:"Publish draft comments as a review"` + List reviewListCmd `cmd:"" aliases:"ls" help:"List review comments"` + Edit reviewEditCmd `cmd:"" help:"Edit a draft comment"` + Resolve reviewResolveCmd `cmd:"" help:"Resolve a review thread"` + Reopen reviewReopenCmd `cmd:"" help:"Reopen a resolved review thread"` +} + +// reviewRepositoryForBranch resolves the change and review-capable repository +// shared by review commands that operate on remote state. +func reviewRepositoryForBranch( + ctx context.Context, + svc *spice.Service, + forgeRepo forge.Repository, + branch string, +) (*spice.LookupBranchResponse, forge.ReviewRepository, error) { + b, err := svc.LookupBranch(ctx, branch) + if err != nil { + if errors.Is(err, state.ErrNotExist) { + return nil, nil, fmt.Errorf("branch not tracked: %s", branch) + } + return nil, nil, fmt.Errorf("get branch: %w", err) + } + if b.Change == nil { + return nil, nil, fmt.Errorf( + "no change request for %s; "+ + "submit the branch first with "+ + "'gs branch submit'", + branch, + ) + } + + reviewRepo, ok := forgeRepo.(forge.ReviewRepository) + if !ok { + return nil, nil, errors.New( + "forge does not support review comments", + ) + } + return b, reviewRepo, nil +} + +// loadReviewThreadIDs indexes the forge's native thread identifiers by their +// command-line representation. ReviewThreadID is intentionally opaque, so a +// command must recover the provider-owned value before replying to or resolving +// a thread named by the user. +func loadReviewThreadIDs( + ctx context.Context, + repo forge.ReviewRepository, + changeID forge.ChangeID, +) (map[string]forge.ReviewThreadID, error) { + ids := make(map[string]forge.ReviewThreadID) + for thread, err := range repo.ListReviewThreads(ctx, changeID) { + if err != nil { + return nil, fmt.Errorf("list review threads: %w", err) + } + ids[thread.ID.String()] = thread.ID + } + return ids, nil +} + +// reviewThreadID resolves a user-supplied thread string to the opaque ID value +// returned by the current forge. +func reviewThreadID( + ids map[string]forge.ReviewThreadID, + id string, +) (forge.ReviewThreadID, error) { + threadID, ok := ids[id] + if !ok { + return nil, fmt.Errorf("review thread %q not found", id) + } + return threadID, nil +} diff --git a/review_comment.go b/review_comment.go new file mode 100644 index 000000000..548e24612 --- /dev/null +++ b/review_comment.go @@ -0,0 +1,262 @@ +package main + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/reviewdiff" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/spice" + "go.abhg.dev/gs/internal/spice/state" + "go.abhg.dev/gs/internal/text" + "go.abhg.dev/gs/internal/xec" +) + +type reviewCommentCmd struct { + FileAndLine string `arg:"" help:"File and line in the form file.go:42."` + Message string `short:"m" placeholder:"MSG" help:"Comment body. Opens editor if not provided."` + Draft bool `negatable:"" default:"true" help:"Save the comment as a local draft instead of posting it."` + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch to comment on. Defaults to the current branch."` +} + +func (*reviewCommentCmd) Help() string { + return text.Dedent(` + Adds a review comment to the change request + for the current branch. + Provide the file and line number as file.go:42. + + Comments are saved as local drafts by default. + Use --no-draft to post immediately. + + If no message is given with -m, an editor is opened. + `) +} + +func (cmd *reviewCommentCmd) Run( + ctx context.Context, + log *silog.Logger, + wt *git.Worktree, + svc *spice.Service, + store *state.Store, + repo *git.Repository, + forgeRepo forge.Repository, +) error { + branch, err := reviewBranch(ctx, wt, cmd.Branch) + if err != nil { + return err + } + + file, line, err := parseFileAndLine(cmd.FileAndLine) + if err != nil { + return err + } + body, err := reviewCommentBody(ctx, repo, cmd.Message, "") + if err != nil { + return err + } + + if cmd.Draft { + return saveReviewDraft(ctx, log, store, branch, state.StagedComment{ + File: file, + Line: line, + Body: body, + }) + } + + b, reviewRepo, err := reviewRepositoryForBranch( + ctx, svc, forgeRepo, branch, + ) + if err != nil { + return err + } + diff, err := wt.DiffBranchBytes(ctx, b.Base, branch) + if err != nil { + return fmt.Errorf("get diff: %w", err) + } + patch, err := reviewdiff.Parse(diff) + if err != nil { + return fmt.Errorf("parse diff: %w", err) + } + if !patch.ContainsLine(file, line) { + return fmt.Errorf( + "review diff does not contain %s:%d", + file, + line, + ) + } + + return postReviewComment( + ctx, + log, + reviewRepo, + b.Change.ChangeID(), + forge.SubmitReviewCommentRequest{ + Path: file, + Range: forge.ReviewThreadLine(line), + Body: body, + Side: forge.ReviewThreadSideRight, + }, + ) +} + +func reviewBranch( + ctx context.Context, + wt *git.Worktree, + branch string, +) (string, error) { + if branch != "" { + return branch, nil + } + branch, err := wt.CurrentBranch(ctx) + if err != nil { + return "", fmt.Errorf("get current branch: %w", err) + } + return branch, nil +} + +func reviewCommentBody( + ctx context.Context, + repo *git.Repository, + message string, + initial string, +) (string, error) { + body := message + if body == "" { + var err error + body, err = editReviewCommentBody(ctx, repo, initial) + if err != nil { + return "", err + } + } + if strings.TrimSpace(body) == "" { + return "", errors.New("empty comment body, aborting") + } + return body, nil +} + +func saveReviewDraft( + ctx context.Context, + log *silog.Logger, + store *state.Store, + branch string, + comment state.StagedComment, +) error { + staged, err := store.LoadStagedComments(ctx, branch) + if err != nil { + return fmt.Errorf("load draft comments: %w", err) + } + if staged == nil { + staged = &state.StagedComments{NextID: 1} + } + + comment.ID = staged.NextID + staged.Comments = append(staged.Comments, comment) + staged.NextID++ + if err := store.SaveStagedComments(ctx, branch, staged); err != nil { + return fmt.Errorf("save draft comments: %w", err) + } + + if comment.ThreadID != "" { + log.Infof( + "Drafted reply %d to thread %s.", + comment.ID, + comment.ThreadID, + ) + } else { + log.Infof( + "Drafted comment %d on %s:%d.", + comment.ID, + comment.File, + comment.Line, + ) + } + return nil +} + +func postReviewComment( + ctx context.Context, + log *silog.Logger, + reviewRepo forge.ReviewRepository, + changeID forge.ChangeID, + comment forge.SubmitReviewCommentRequest, +) error { + result, err := reviewRepo.SubmitReview( + ctx, + changeID, + forge.SubmitReviewRequest{ + Comments: []forge.SubmitReviewCommentRequest{comment}, + }, + ) + if err != nil { + return fmt.Errorf("post review comment: %w", err) + } + if len(result.Comments) != 1 { + return fmt.Errorf( + "post review comment: forge returned %d comment results", + len(result.Comments), + ) + } + + log.Infof( + "Posted comment %s on %s.", + result.Comments[0].ThreadID.String(), + changeID, + ) + return nil +} + +// parseFileAndLine parses a file.go:42 argument into its path and line. +func parseFileAndLine(value string) (string, int, error) { + idx := strings.LastIndex(value, ":") + if idx < 0 { + return "", 0, fmt.Errorf( + "expected file:line format, got %q", value, + ) + } + file := value[:idx] + line, err := strconv.Atoi(value[idx+1:]) + if err != nil { + return "", 0, fmt.Errorf( + "invalid line number in %q: %w", value, err, + ) + } + if line <= 0 { + return "", 0, fmt.Errorf( + "line number must be positive, got %d", line, + ) + } + return file, line, nil +} + +// editReviewCommentBody opens the configured editor with initial as its +// starting contents and returns the edited comment body. +func editReviewCommentBody( + ctx context.Context, + repo *git.Repository, + initial string, +) (string, error) { + tmpFile := filepath.Join(os.TempDir(), "GS_REVIEW_EDITMSG") + if err := os.WriteFile(tmpFile, []byte(initial), 0o644); err != nil { + return "", fmt.Errorf("write temp file: %w", err) + } + defer func() { _ = os.Remove(tmpFile) }() + + editor := gitEditor(ctx, repo) + cmd := xec.EditCommand(editor, tmpFile) + if err := cmd.Run(); err != nil { + return "", fmt.Errorf("run editor: %w", err) + } + + content, err := os.ReadFile(tmpFile) + if err != nil { + return "", fmt.Errorf("read temp file: %w", err) + } + return string(content), nil +} diff --git a/review_edit.go b/review_edit.go new file mode 100644 index 000000000..a30c4cf67 --- /dev/null +++ b/review_edit.go @@ -0,0 +1,78 @@ +package main + +import ( + "context" + "fmt" + + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/spice/state" + "go.abhg.dev/gs/internal/text" +) + +type reviewEditCmd struct { + ID int `arg:"" help:"Draft comment ID to edit."` + Message string `short:"m" placeholder:"MSG" help:"New comment body. Opens editor if not provided."` + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch containing the draft. Defaults to the current branch."` +} + +func (*reviewEditCmd) Help() string { + return text.Dedent(` + Edits a local draft comment. + + Use 'gs review list --draft-only' + to find the branch-local draft ID. + + If no message is given with -m, an editor is opened + with the current comment body pre-filled. + `) +} + +func (cmd *reviewEditCmd) Run( + ctx context.Context, + log *silog.Logger, + wt *git.Worktree, + store *state.Store, + repo *git.Repository, +) error { + branch, err := reviewBranch(ctx, wt, cmd.Branch) + if err != nil { + return err + } + + staged, err := store.LoadStagedComments(ctx, branch) + if err != nil { + return fmt.Errorf("load draft comments: %w", err) + } + if staged == nil { + staged = &state.StagedComments{} + } + + idx := -1 + for i, comment := range staged.Comments { + if comment.ID == cmd.ID { + idx = i + break + } + } + if idx < 0 { + return fmt.Errorf("draft comment %d not found", cmd.ID) + } + + body, err := reviewCommentBody( + ctx, + repo, + cmd.Message, + staged.Comments[idx].Body, + ) + if err != nil { + return err + } + staged.Comments[idx].Body = body + if err := store.SaveStagedComments(ctx, branch, staged); err != nil { + return fmt.Errorf("save draft comments: %w", err) + } + + log.Infof("Updated draft comment %d.", cmd.ID) + return nil +} diff --git a/branch_comment_list.go b/review_list.go similarity index 88% rename from branch_comment_list.go rename to review_list.go index ea5db4b46..a64c3a5c9 100644 --- a/branch_comment_list.go +++ b/review_list.go @@ -20,23 +20,22 @@ import ( "go.abhg.dev/gs/internal/text" ) -type branchCommentListCmd struct { - Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch to list comments for. Defaults to current branch."` - Staged bool `help:"Show only staged comments."` +type reviewListCmd struct { + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch to list comments for. Defaults to the current branch."` + DraftOnly bool `name:"draft-only" help:"Show only draft comments."` Unresolved bool `help:"Show only unresolved comments."` JSON bool `name:"json" released:"unreleased" help:"Write to stdout as a stream of JSON objects."` } -func (*branchCommentListCmd) Help() string { +func (*reviewListCmd) Help() string { return text.Dedent(` Lists comments on the change request associated with the current branch. Use --branch to target a different branch. - Staged comments that have not yet been submitted - are shown with an 'sc-N' prefix. + Draft comments are identified by a branch-local integer. - Use --staged to show only staged comments. + Use --draft-only to show only draft comments. Use --unresolved to show only unresolved comments. With --json, prints output to stdout @@ -44,7 +43,7 @@ func (*branchCommentListCmd) Help() string { `) } -func (cmd *branchCommentListCmd) Run( +func (cmd *reviewListCmd) Run( ctx context.Context, kctx *kong.Context, log *silog.Logger, @@ -73,7 +72,7 @@ func (cmd *branchCommentListCmd) Run( return cmd.writeText(log, branch, staged, forgeComments) } -func (cmd *branchCommentListCmd) resolveBranch( +func (cmd *reviewListCmd) resolveBranch( ctx context.Context, wt *git.Worktree, ) (string, error) { if cmd.Branch != "" { @@ -86,7 +85,7 @@ func (cmd *branchCommentListCmd) resolveBranch( return branch, nil } -func (cmd *branchCommentListCmd) loadComments( +func (cmd *reviewListCmd) loadComments( ctx context.Context, log *silog.Logger, svc *spice.Service, @@ -99,7 +98,7 @@ func (cmd *branchCommentListCmd) loadComments( return nil, nil, err } - if cmd.Staged { + if cmd.DraftOnly { return staged, nil, nil } @@ -120,9 +119,7 @@ func loadStagedComments( ) ([]*state.StagedComment, error) { staged, err := store.LoadStagedComments(ctx, branch) if err != nil { - return nil, fmt.Errorf( - "load staged comments: %w", err, - ) + return nil, fmt.Errorf("load draft comments: %w", err) } if staged == nil { return nil, nil @@ -197,7 +194,7 @@ type listedReviewComment struct { Comment *forge.ReviewComment } -func (cmd *branchCommentListCmd) filterForge( +func (cmd *reviewListCmd) filterForge( comments []*listedReviewComment, ) []*listedReviewComment { if !cmd.Unresolved { @@ -214,21 +211,21 @@ func (cmd *branchCommentListCmd) filterForge( } // writeText prints comments in human-readable format. -func (cmd *branchCommentListCmd) writeText( +func (cmd *reviewListCmd) writeText( log *silog.Logger, branch string, staged []*state.StagedComment, forgeComments []*listedReviewComment, ) error { if len(staged) > 0 { - log.Infof("Staged comments:") + log.Infof("Draft comments:") for _, c := range staged { writeStagedText(log, c) } } - if cmd.Staged && len(staged) == 0 { - log.Infof("No staged comments for %s.", branch) + if cmd.DraftOnly && len(staged) == 0 { + log.Infof("No draft comments for %s.", branch) return nil } @@ -252,7 +249,7 @@ func writeStagedText( if c.ThreadID != "" { location = "reply:" + c.ThreadID } - log.Infof(" sc-%-4d %s", c.ID, location) + log.Infof(" %-4d %s", c.ID, location) writeBodyIndented(log, c.Body) } @@ -291,7 +288,7 @@ func commentStatus(c *listedReviewComment) string { } // writeJSON encodes comments as NDJSON to stdout. -func (cmd *branchCommentListCmd) writeJSON( +func (cmd *reviewListCmd) writeJSON( w io.Writer, staged []*state.StagedComment, forgeComments []*listedReviewComment, @@ -317,8 +314,8 @@ func (cmd *branchCommentListCmd) writeJSON( func stagedToJSON(c *state.StagedComment) jsonComment { return jsonComment{ - Kind: "staged", - ID: fmt.Sprintf("sc-%d", c.ID), + Kind: "draft", + ID: fmt.Sprintf("%d", c.ID), Path: c.File, Line: c.Line, Body: c.Body, @@ -347,11 +344,11 @@ func forgeToJSON(c *listedReviewComment) jsonComment { // jsonComment is the JSON representation // of a comment for --json output. type jsonComment struct { - // Kind is "staged" or "forge". + // Kind is "draft" or "forge". Kind string `json:"kind"` // ID is the comment identifier. - // For staged comments: "sc-N". + // For draft comments: a branch-local integer encoded as a string. // For forge comments: forge-specific ID. ID string `json:"id"` diff --git a/branch_comment_submit_staged.go b/review_publish.go similarity index 84% rename from branch_comment_submit_staged.go rename to review_publish.go index 759177b33..dd3335879 100644 --- a/branch_comment_submit_staged.go +++ b/review_publish.go @@ -14,16 +14,16 @@ import ( "go.abhg.dev/gs/internal/text" ) -type branchCommentSubmitStagedCmd struct { +type reviewPublishCmd struct { Body string `placeholder:"BODY" help:"Overall review body."` Approve bool `help:"Mark the review as approved."` RequestChanges bool `name:"request-changes" help:"Mark the review as requesting changes."` - Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch to submit staged comments for. Defaults to current branch."` + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch whose draft comments to publish. Defaults to the current branch."` } -func (*branchCommentSubmitStagedCmd) Help() string { +func (*reviewPublishCmd) Help() string { return text.Dedent(` - Submits all staged comments for the current branch + Publishes all draft comments for the current branch as a single review on the change request. Use --approve or --request-changes @@ -34,7 +34,7 @@ func (*branchCommentSubmitStagedCmd) Help() string { `) } -func (cmd *branchCommentSubmitStagedCmd) Run( +func (cmd *reviewPublishCmd) Run( ctx context.Context, log *silog.Logger, wt *git.Worktree, @@ -53,14 +53,14 @@ func (cmd *branchCommentSubmitStagedCmd) Run( staged, err := store.LoadStagedComments(ctx, branch) if err != nil { - return fmt.Errorf("load staged comments: %w", err) + return fmt.Errorf("load draft comments: %w", err) } if staged == nil { staged = &state.StagedComments{} } if len(staged.Comments) == 0 { - log.Infof("No staged comments to submit.") + log.Infof("No draft comments to publish.") return nil } @@ -90,7 +90,7 @@ func (cmd *branchCommentSubmitStagedCmd) Run( ) } - // Staged roots use the selected branch's postimage coordinates. Parse the + // Draft roots use the selected branch's postimage coordinates. Parse the // review diff once so every root can be checked before anything is sent. diff, err := wt.DiffBranchBytes(ctx, b.Base, branch) if err != nil { @@ -121,7 +121,7 @@ func (cmd *branchCommentSubmitStagedCmd) Run( if sc.ThreadID != "" { threadID, err := reviewThreadID(threadIDs, sc.ThreadID) if err != nil { - return fmt.Errorf("sc-%d: %w", sc.ID, err) + return fmt.Errorf("draft %d: %w", sc.ID, err) } comments = append(comments, forge.SubmitReviewCommentRequest{ @@ -134,7 +134,7 @@ func (cmd *branchCommentSubmitStagedCmd) Run( if !patch.ContainsLine(sc.File, sc.Line) { return fmt.Errorf( - "sc-%d: review diff does not contain %s:%d", + "draft %d: review diff does not contain %s:%d", sc.ID, sc.File, sc.Line, @@ -172,11 +172,11 @@ func (cmd *branchCommentSubmitStagedCmd) Run( if err := store.ClearStagedComments( ctx, branch, ); err != nil { - return fmt.Errorf("clear staged comments: %w", err) + return fmt.Errorf("clear draft comments: %w", err) } log.Infof( - "Submitted %d comment(s) as review on %s.", + "Published %d comment(s) as review on %s.", len(comments), b.Change.ChangeID(), ) diff --git a/review_reply.go b/review_reply.go new file mode 100644 index 000000000..926a7da75 --- /dev/null +++ b/review_reply.go @@ -0,0 +1,87 @@ +package main + +import ( + "context" + + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/spice" + "go.abhg.dev/gs/internal/spice/state" + "go.abhg.dev/gs/internal/text" +) + +type reviewReplyCmd struct { + ThreadID string `arg:"" help:"Thread ID to reply to."` + Message string `short:"m" placeholder:"MSG" help:"Reply body. Opens editor if not provided."` + Draft bool `negatable:"" default:"true" help:"Save the reply as a local draft instead of posting it."` + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch containing the thread. Defaults to the current branch."` +} + +func (*reviewReplyCmd) Help() string { + return text.Dedent(` + Replies to a review thread on the change request + for the current branch. + + Replies are saved as local drafts by default. + Use --no-draft to post immediately. + + If no message is given with -m, an editor is opened. + `) +} + +func (cmd *reviewReplyCmd) Run( + ctx context.Context, + log *silog.Logger, + wt *git.Worktree, + svc *spice.Service, + store *state.Store, + repo *git.Repository, + forgeRepo forge.Repository, +) error { + branch, err := reviewBranch(ctx, wt, cmd.Branch) + if err != nil { + return err + } + body, err := reviewCommentBody(ctx, repo, cmd.Message, "") + if err != nil { + return err + } + + if cmd.Draft { + return saveReviewDraft(ctx, log, store, branch, state.StagedComment{ + Body: body, + ThreadID: cmd.ThreadID, + }) + } + + b, reviewRepo, err := reviewRepositoryForBranch( + ctx, svc, forgeRepo, branch, + ) + if err != nil { + return err + } + threadIDs, err := loadReviewThreadIDs( + ctx, + reviewRepo, + b.Change.ChangeID(), + ) + if err != nil { + return err + } + threadID, err := reviewThreadID(threadIDs, cmd.ThreadID) + if err != nil { + return err + } + + return postReviewComment( + ctx, + log, + reviewRepo, + b.Change.ChangeID(), + forge.SubmitReviewCommentRequest{ + Body: body, + ReplyTo: threadID, + }, + ) +} diff --git a/review_resolution.go b/review_resolution.go new file mode 100644 index 000000000..bb0cd19b6 --- /dev/null +++ b/review_resolution.go @@ -0,0 +1,133 @@ +package main + +import ( + "context" + "errors" + "fmt" + + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/spice" + "go.abhg.dev/gs/internal/text" +) + +type reviewResolveCmd struct { + ThreadID string `arg:"" help:"Thread ID to resolve."` + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch containing the thread. Defaults to the current branch."` +} + +func (*reviewResolveCmd) Help() string { + return text.Dedent(` + Resolves a review thread on the change request + for the current branch. + + The thread ID is shown in 'gs review list'. + `) +} + +func (cmd *reviewResolveCmd) Run( + ctx context.Context, + log *silog.Logger, + wt *git.Worktree, + svc *spice.Service, + forgeRepo forge.Repository, +) error { + return setReviewThreadResolved( + ctx, + log, + wt, + svc, + forgeRepo, + cmd.Branch, + cmd.ThreadID, + true, + ) +} + +type reviewReopenCmd struct { + ThreadID string `arg:"" help:"Thread ID to reopen."` + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch containing the thread. Defaults to the current branch."` +} + +func (*reviewReopenCmd) Help() string { + return text.Dedent(` + Reopens a resolved review thread on the change request + for the current branch. + + The thread ID is shown in 'gs review list'. + `) +} + +func (cmd *reviewReopenCmd) Run( + ctx context.Context, + log *silog.Logger, + wt *git.Worktree, + svc *spice.Service, + forgeRepo forge.Repository, +) error { + return setReviewThreadResolved( + ctx, + log, + wt, + svc, + forgeRepo, + cmd.Branch, + cmd.ThreadID, + false, + ) +} + +func setReviewThreadResolved( + ctx context.Context, + log *silog.Logger, + wt *git.Worktree, + svc *spice.Service, + forgeRepo forge.Repository, + branch string, + thread string, + resolved bool, +) error { + branch, err := reviewBranch(ctx, wt, branch) + if err != nil { + return err + } + b, reviewRepo, err := reviewRepositoryForBranch( + ctx, svc, forgeRepo, branch, + ) + if err != nil { + return err + } + resolver, ok := forgeRepo.(forge.ReviewThreadResolver) + if !ok { + return errors.New( + "forge does not support review thread resolution", + ) + } + + threadIDs, err := loadReviewThreadIDs( + ctx, + reviewRepo, + b.Change.ChangeID(), + ) + if err != nil { + return err + } + threadID, err := reviewThreadID(threadIDs, thread) + if err != nil { + return err + } + + if resolved { + if err := resolver.ResolveReviewThread(ctx, threadID); err != nil { + return fmt.Errorf("resolve thread: %w", err) + } + log.Infof("Resolved thread %s.", thread) + return nil + } + if err := resolver.UnresolveReviewThread(ctx, threadID); err != nil { + return fmt.Errorf("reopen thread: %w", err) + } + log.Infof("Reopened thread %s.", thread) + return nil +} diff --git a/testdata/help/branch_comment_add.txt b/testdata/help/branch_comment_add.txt deleted file mode 100644 index a3a596440..000000000 --- a/testdata/help/branch_comment_add.txt +++ /dev/null @@ -1,27 +0,0 @@ -Usage: gs branch (b) comment (cmt) add [] [flags] - -Post an inline comment immediately - -Posts an inline comment immediately on the change request for the current -branch. Provide the file and line number as file.go:42. - -If no message is given with -m, an editor is opened. - -Use --respond to reply to an existing thread instead of starting a new one. - -Arguments: - [] File and line in the form file.go:42. - -Flags: - -m, --message=MSG Comment body. Opens editor if not provided. - --respond=THREAD_ID Thread ID to reply to instead of starting a new - thread. - -b, --branch=BRANCH Branch to add comment for. Defaults to current - branch. - -Global Flags: - -h, --help Show help for the command - --version Print version information and quit - -v, --verbose Enable verbose output ($GIT_SPICE_VERBOSE) - -C, --dir=DIR Change to DIR before doing anything - --[no-]prompt Whether to prompt for missing information diff --git a/testdata/help/branch_comment_stage.txt b/testdata/help/branch_comment_stage.txt deleted file mode 100644 index d8d4e7d2b..000000000 --- a/testdata/help/branch_comment_stage.txt +++ /dev/null @@ -1,29 +0,0 @@ -Usage: gs branch (b) comment (cmt) stage [] [flags] - -Stage an inline comment for batch submission - -Stages an inline comment for later batch submission. Provide the file and line -number as file.go:42. - -If no message is given with -m, an editor is opened. - -Use --respond to reply to an existing thread instead of starting a new one. - -Staged comments are submitted together with 'gs branch comment submit-staged'. - -Arguments: - [] File and line in the form file.go:42. - -Flags: - -m, --message=MSG Comment body. Opens editor if not provided. - --respond=THREAD_ID Thread ID to reply to instead of starting a new - thread. - -b, --branch=BRANCH Branch to stage comment for. Defaults to current - branch. - -Global Flags: - -h, --help Show help for the command - --version Print version information and quit - -v, --verbose Enable verbose output ($GIT_SPICE_VERBOSE) - -C, --dir=DIR Change to DIR before doing anything - --[no-]prompt Whether to prompt for missing information diff --git a/testdata/help/gs.txt b/testdata/help/gs.txt index 68df72df1..cedd4c2e7 100644 --- a/testdata/help/gs.txt +++ b/testdata/help/gs.txt @@ -46,31 +46,21 @@ Stack downstack (ds) restack (r) Restack a branch and its downstack Branch - branch (b) track (tr) Track a branch - branch (b) untrack (untr) Forget a tracked branch - branch (b) checkout (co) Switch to a branch - branch (b) create (c) Create a new branch - branch (b) delete (d,rm) Delete branches - branch (b) fold (fo) Merge a branch into its base - branch (b) split (sp) Split a branch on commits - branch (b) squash (sq) Squash a branch into one commit - branch (b) edit (e) Edit the commits in a branch - branch (b) rename (rn,mv) Rename a branch - branch (b) restack (r) Restack a branch - branch (b) onto (on) Move a branch onto another branch - branch (b) diff (di) Show diff between a branch and its base - branch (b) merge (m) Merge a branch into trunk - branch (b) submit (s) Submit a branch - branch (b) comment (cmt) list (ls) - List comments on a change request - branch (b) comment (cmt) stage - Stage an inline comment for batch submission - branch (b) comment (cmt) add Post an inline comment immediately - branch (b) comment (cmt) submit-staged (ss) - Submit all staged comments as a review - branch (b) comment (cmt) resolve - Resolve or unresolve a review thread - branch (b) comment (cmt) edit Edit a comment + branch (b) track (tr) Track a branch + branch (b) untrack (untr) Forget a tracked branch + branch (b) checkout (co) Switch to a branch + branch (b) create (c) Create a new branch + branch (b) delete (d,rm) Delete branches + branch (b) fold (fo) Merge a branch into its base + branch (b) split (sp) Split a branch on commits + branch (b) squash (sq) Squash a branch into one commit + branch (b) edit (e) Edit the commits in a branch + branch (b) rename (rn,mv) Rename a branch + branch (b) restack (r) Restack a branch + branch (b) onto (on) Move a branch onto another branch + branch (b) diff (di) Show diff between a branch and its base + branch (b) merge (m) Merge a branch into trunk + branch (b) submit (s) Submit a branch Commit commit (c) create (c) Create a new commit @@ -79,6 +69,15 @@ Commit commit (c) fixup (f) Fixup a commit below the current commit commit (c) pick (p) Cherry-pick a commit +Review + review comment Draft or post a review comment + review reply Draft or post a reply to a review thread + review publish Publish draft comments as a review + review list (ls) List review comments + review edit Edit a draft comment + review resolve Resolve a review thread + review reopen Reopen a resolved review thread + Rebase rebase (rb) continue (c) Continue an interrupted operation rebase (rb) abort (a) Abort an operation diff --git a/testdata/help/review_comment.txt b/testdata/help/review_comment.txt new file mode 100644 index 000000000..d34df319c --- /dev/null +++ b/testdata/help/review_comment.txt @@ -0,0 +1,27 @@ +Usage: gs review comment [flags] + +Draft or post a review comment + +Adds a review comment to the change request for the current branch. Provide the +file and line number as file.go:42. + +Comments are saved as local drafts by default. Use --no-draft to post +immediately. + +If no message is given with -m, an editor is opened. + +Arguments: + File and line in the form file.go:42. + +Flags: + -m, --message=MSG Comment body. Opens editor if not provided. + --[no-]draft Save the comment as a local draft instead of posting + it. + -b, --branch=BRANCH Branch to comment on. Defaults to the current branch. + +Global Flags: + -h, --help Show help for the command + --version Print version information and quit + -v, --verbose Enable verbose output ($GIT_SPICE_VERBOSE) + -C, --dir=DIR Change to DIR before doing anything + --[no-]prompt Whether to prompt for missing information diff --git a/testdata/help/branch_comment_edit.txt b/testdata/help/review_edit.txt similarity index 56% rename from testdata/help/branch_comment_edit.txt rename to testdata/help/review_edit.txt index 26ef35744..7337e0344 100644 --- a/testdata/help/branch_comment_edit.txt +++ b/testdata/help/review_edit.txt @@ -1,24 +1,20 @@ -Usage: gs branch (b) comment (cmt) edit [flags] +Usage: gs review edit [flags] -Edit a comment +Edit a draft comment -Edits the body of a comment. +Edits a local draft comment. -For staged comments (sc-N prefix), the comment is updated in the local staging -area. - -For forge comments, the comment is updated on the remote forge. +Use 'gs review list --draft-only' to find the branch-local draft ID. If no message is given with -m, an editor is opened with the current comment body pre-filled. Arguments: - Comment ID to edit. Use 'sc-N' for staged comments or a forge comment - ID. + Draft comment ID to edit. Flags: -m, --message=MSG New comment body. Opens editor if not provided. - -b, --branch=BRANCH Branch whose comments to edit. Defaults to current + -b, --branch=BRANCH Branch containing the draft. Defaults to the current branch. Global Flags: diff --git a/testdata/help/branch_comment_list.txt b/testdata/help/review_list.txt similarity index 71% rename from testdata/help/branch_comment_list.txt rename to testdata/help/review_list.txt index 2ad4dabf5..69ce7b2e9 100644 --- a/testdata/help/branch_comment_list.txt +++ b/testdata/help/review_list.txt @@ -1,22 +1,21 @@ -Usage: gs branch (b) comment (cmt) list (ls) [flags] +Usage: gs review list (ls) [flags] -List comments on a change request +List review comments Lists comments on the change request associated with the current branch. Use --branch to target a different branch. -Staged comments that have not yet been submitted are shown with an 'sc-N' -prefix. +Draft comments are identified by a branch-local integer. -Use --staged to show only staged comments. Use --unresolved to show only +Use --draft-only to show only draft comments. Use --unresolved to show only unresolved comments. With --json, prints output to stdout as a stream of JSON objects. Flags: - -b, --branch=BRANCH Branch to list comments for. Defaults to current + -b, --branch=BRANCH Branch to list comments for. Defaults to the current branch. - --staged Show only staged comments. + --draft-only Show only draft comments. --unresolved Show only unresolved comments. --json Write to stdout as a stream of JSON objects. diff --git a/testdata/help/branch_comment_submit-staged.txt b/testdata/help/review_publish.txt similarity index 68% rename from testdata/help/branch_comment_submit-staged.txt rename to testdata/help/review_publish.txt index 6d6b6c339..6cfe6e13a 100644 --- a/testdata/help/branch_comment_submit-staged.txt +++ b/testdata/help/review_publish.txt @@ -1,8 +1,8 @@ -Usage: gs branch (b) comment (cmt) submit-staged (ss) [flags] +Usage: gs review publish [flags] -Submit all staged comments as a review +Publish draft comments as a review -Submits all staged comments for the current branch as a single review on the +Publishes all draft comments for the current branch as a single review on the change request. Use --approve or --request-changes to set the review event type. Defaults to a @@ -14,8 +14,8 @@ Flags: --body=BODY Overall review body. --approve Mark the review as approved. --request-changes Mark the review as requesting changes. - -b, --branch=BRANCH Branch to submit staged comments for. Defaults to - current branch. + -b, --branch=BRANCH Branch whose draft comments to publish. Defaults to + the current branch. Global Flags: -h, --help Show help for the command diff --git a/testdata/help/review_reopen.txt b/testdata/help/review_reopen.txt new file mode 100644 index 000000000..ee7b2206d --- /dev/null +++ b/testdata/help/review_reopen.txt @@ -0,0 +1,21 @@ +Usage: gs review reopen [flags] + +Reopen a resolved review thread + +Reopens a resolved review thread on the change request for the current branch. + +The thread ID is shown in 'gs review list'. + +Arguments: + Thread ID to reopen. + +Flags: + -b, --branch=BRANCH Branch containing the thread. Defaults to the current + branch. + +Global Flags: + -h, --help Show help for the command + --version Print version information and quit + -v, --verbose Enable verbose output ($GIT_SPICE_VERBOSE) + -C, --dir=DIR Change to DIR before doing anything + --[no-]prompt Whether to prompt for missing information diff --git a/testdata/help/review_reply.txt b/testdata/help/review_reply.txt new file mode 100644 index 000000000..5c365ec7a --- /dev/null +++ b/testdata/help/review_reply.txt @@ -0,0 +1,26 @@ +Usage: gs review reply [flags] + +Draft or post a reply to a review thread + +Replies to a review thread on the change request for the current branch. + +Replies are saved as local drafts by default. Use --no-draft to post +immediately. + +If no message is given with -m, an editor is opened. + +Arguments: + Thread ID to reply to. + +Flags: + -m, --message=MSG Reply body. Opens editor if not provided. + --[no-]draft Save the reply as a local draft instead of posting it. + -b, --branch=BRANCH Branch containing the thread. Defaults to the current + branch. + +Global Flags: + -h, --help Show help for the command + --version Print version information and quit + -v, --verbose Enable verbose output ($GIT_SPICE_VERBOSE) + -C, --dir=DIR Change to DIR before doing anything + --[no-]prompt Whether to prompt for missing information diff --git a/testdata/help/branch_comment_resolve.txt b/testdata/help/review_resolve.txt similarity index 52% rename from testdata/help/branch_comment_resolve.txt rename to testdata/help/review_resolve.txt index c4ee13472..70c10371f 100644 --- a/testdata/help/branch_comment_resolve.txt +++ b/testdata/help/review_resolve.txt @@ -1,20 +1,17 @@ -Usage: gs branch (b) comment (cmt) resolve [flags] +Usage: gs review resolve [flags] -Resolve or unresolve a review thread +Resolve a review thread Resolves a review thread on the change request for the current branch. -Use --unresolve to mark the thread as unresolved. - -The thread ID is shown in 'gs branch comment list'. +The thread ID is shown in 'gs review list'. Arguments: Thread ID to resolve. Flags: - --unresolve Unresolve the thread instead of resolving it. - -b, --branch=BRANCH Branch whose change request contains the thread. - Defaults to current branch. + -b, --branch=BRANCH Branch containing the thread. Defaults to the current + branch. Global Flags: -h, --help Show help for the command diff --git a/testdata/script/branch_comment_stage_submit.txt b/testdata/script/branch_comment_stage_submit.txt deleted file mode 100644 index 4409fb17d..000000000 --- a/testdata/script/branch_comment_stage_submit.txt +++ /dev/null @@ -1,59 +0,0 @@ -# Stage inline comments and submit them as a review. - -as 'Test ' -at '2024-04-05T16:40:32Z' - -# setup -cd repo -git init -git commit --allow-empty -m 'Initial commit' -gs repo init - -# set up a fake GitHub remote -shamhub-setup -shamhub new origin alice/example.git -shamhub register alice -git push origin main - -env SHAMHUB_USERNAME=alice -gs auth login - -# create a branch with a file and submit it -git add feature.go -gs bc -m 'Add feature' feature1 -gs branch submit --fill -stderr 'Created #' - -# stage a comment on the file -gs branch comment stage feature.go:3 -m 'Consider renaming this function.' -stderr 'Staged comment sc-1 on feature.go:3' - -# stage another comment -gs branch comment stage feature.go:7 -m 'Add error handling here.' -stderr 'Staged comment sc-2 on feature.go:7' - -# list staged comments -gs branch comment list --staged -stderr 'sc-1' -stderr 'feature.go:3' -stderr 'sc-2' -stderr 'feature.go:7' - -# submit staged comments as a review -gs branch comment submit-staged -stderr 'Submitted 2 comment' - -# staged comments should be cleared after submit -gs branch comment list --staged -stderr 'No staged comments' - --- repo/feature.go -- -package main - -func doWork() { - // does some work -} - -func handleRequest() { - // handles a request -} diff --git a/testdata/script/branch_comment_add.txt b/testdata/script/review_comment.txt similarity index 54% rename from testdata/script/branch_comment_add.txt rename to testdata/script/review_comment.txt index 6dae366a6..821b49088 100644 --- a/testdata/script/branch_comment_add.txt +++ b/testdata/script/review_comment.txt @@ -1,4 +1,4 @@ -# Post an inline comment immediately with 'branch comment add'. +# Draft and post a review comment or reply. as 'Test ' at '2024-04-05T16:40:32Z' @@ -24,8 +24,18 @@ gs bc -m 'Add handler' feature1 gs branch submit --fill stderr 'Created #' -# post an inline comment immediately -gs branch comment add handler.go:5 -m 'This needs a context parameter.' +# comments are drafts by default +gs review comment handler.go:5 -m 'This needs a context parameter.' +stderr 'Drafted comment 1' + +# --no-draft posts a comment immediately +gs review comment handler.go:5 -m 'This needs a context parameter.' --no-draft +stderr 'Posted comment' + +# replies follow the same draft convention +gs review reply thread-2 -m 'Agreed.' +stderr 'Drafted reply 2' +gs review reply thread-2 -m 'Agreed.' --no-draft stderr 'Posted comment' -- repo/handler.go -- diff --git a/testdata/script/branch_comment_edit_staged.txt b/testdata/script/review_edit.txt similarity index 60% rename from testdata/script/branch_comment_edit_staged.txt rename to testdata/script/review_edit.txt index cdd4ddaa0..5c6dac749 100644 --- a/testdata/script/branch_comment_edit_staged.txt +++ b/testdata/script/review_edit.txt @@ -1,4 +1,4 @@ -# Edit a staged comment before submission. +# Edit a draft comment before publication. as 'Test ' at '2024-04-05T16:40:32Z' @@ -24,16 +24,16 @@ gs bc -m 'Add main' feature1 gs branch submit --fill stderr 'Created #' -# stage a comment -gs branch comment stage main.go:3 -m 'Original comment.' -stderr 'Staged comment sc-1' +# draft a comment +gs review comment main.go:3 -m 'Original comment.' +stderr 'Drafted comment 1' -# edit the staged comment with -m -gs branch comment edit sc-1 -m 'Updated comment.' -stderr 'Updated staged comment sc-1' +# edit the draft comment with -m +gs review edit 1 -m 'Updated comment.' +stderr 'Updated draft comment 1' -# verify the edit took effect by listing staged -gs branch comment list --staged +# verify the edit took effect by listing drafts +gs review list --draft-only stderr 'Updated comment' -- repo/main.go -- diff --git a/testdata/script/branch_comment_list.txt b/testdata/script/review_list.txt similarity index 88% rename from testdata/script/branch_comment_list.txt rename to testdata/script/review_list.txt index 939cc0f7c..bda00cbc8 100644 --- a/testdata/script/branch_comment_list.txt +++ b/testdata/script/review_list.txt @@ -25,18 +25,18 @@ gs branch submit --fill stderr 'Created #' # add an inline comment so there is something to list -gs branch comment add main.go:3 -m 'Consider using a constant.' +gs review comment main.go:3 -m 'Consider using a constant.' --no-draft stderr 'Posted comment' # list should show the comment -gs branch comment list +gs review list stderr 'Comments:' stderr 'main.go:3' stderr 'Consider using a constant' # list on a branch with no CR should say so gs bc -m 'Another branch' feature2 -gs branch comment list +gs review list stderr 'No change request' -- repo/main.go -- diff --git a/testdata/script/branch_comment_list_json.txt b/testdata/script/review_list_json.txt similarity index 62% rename from testdata/script/branch_comment_list_json.txt rename to testdata/script/review_list_json.txt index f54035082..438d1702d 100644 --- a/testdata/script/branch_comment_list_json.txt +++ b/testdata/script/review_list_json.txt @@ -25,11 +25,11 @@ gs branch submit --fill stderr 'Created #' # add an inline comment so there is something to list -gs branch comment add main.go:3 -m 'Consider using a constant here because it would make the code much more maintainable and readable.' +gs review comment main.go:3 -m 'Consider using a constant here because it would make the code much more maintainable and readable.' --no-draft stderr 'Posted comment' # --json should output NDJSON to stdout with forge comment -gs branch comment list --json +gs review list --json stdout '"kind":"forge"' stdout '"body":"Consider using a constant here because it would make the code much more maintainable and readable."' stdout '"path":"main.go"' @@ -37,20 +37,20 @@ stdout '"line":3' stdout '"status":"open"' # text mode shows the full body without truncation -gs branch comment list +gs review list stderr 'Consider using a constant here because it would make the code much more maintainable and readable.' -# stage a comment -gs branch comment stage main.go:5 -m 'Add error handling here.' -stderr 'Staged comment' +# draft a comment +gs review comment main.go:5 -m 'Add error handling here.' +stderr 'Drafted comment' -# --staged --json should include only staged comments -gs branch comment list --staged --json -cmpenvJSON stdout $WORK/golden/staged_only.json +# --draft-only --json should include only draft comments +gs review list --draft-only --json +cmpenvJSON stdout $WORK/golden/draft_only.json -# --json without --staged includes both staged and forge -gs branch comment list --json -stdout '"kind":"staged"' +# --json without --draft-only includes both draft and forge comments +gs review list --json +stdout '"kind":"draft"' stdout '"kind":"forge"' -- repo/main.go -- @@ -60,5 +60,5 @@ func main() { println("hello") } --- golden/staged_only.json -- -{"kind":"staged","id":"sc-1","path":"main.go","line":5,"body":"Add error handling here."} +-- golden/draft_only.json -- +{"kind":"draft","id":"1","path":"main.go","line":5,"body":"Add error handling here."} diff --git a/testdata/script/review_publish.txt b/testdata/script/review_publish.txt new file mode 100644 index 000000000..51d6e1e37 --- /dev/null +++ b/testdata/script/review_publish.txt @@ -0,0 +1,57 @@ +# Draft comments and publish them as a review. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init + +# set up a fake GitHub remote +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a branch with a file and submit it +git add feature.go +gs bc -m 'Add feature' feature1 +gs branch submit --fill +stderr 'Created #' + +# draft a comment on the file +gs review comment feature.go:3 -m 'Consider renaming this function.' +stderr 'Drafted comment 1 on feature.go:3' + +# draft another comment +gs review comment feature.go:7 -m 'Add error handling here.' +stderr 'Drafted comment 2 on feature.go:7' + +# list draft comments +gs review list --draft-only +stderr 'feature.go:3' +stderr 'feature.go:7' + +# publish draft comments as a review +gs review publish +stderr 'Published 2 comment' + +# draft comments should be cleared after publication +gs review list --draft-only +stderr 'No draft comments' + +-- repo/feature.go -- +package main + +func doWork() { + // does some work +} + +func handleRequest() { + // handles a request +} diff --git a/testdata/script/branch_comment_resolve.txt b/testdata/script/review_resolve.txt similarity index 62% rename from testdata/script/branch_comment_resolve.txt rename to testdata/script/review_resolve.txt index 59df68aa8..721098ad4 100644 --- a/testdata/script/branch_comment_resolve.txt +++ b/testdata/script/review_resolve.txt @@ -1,4 +1,4 @@ -# Resolve and unresolve a review thread. +# Resolve and reopen a review thread. as 'Test ' at '2024-04-05T16:40:32Z' @@ -25,12 +25,19 @@ gs branch submit --fill stderr 'Created #' # post an inline comment to create a thread -gs branch comment add util.go:3 -m 'Rename this function.' +gs review comment util.go:3 -m 'Rename this function.' --no-draft stderr 'Posted comment' -# get the thread ID from the list output -gs branch comment list -stderr 'thread-' +# resolve and reopen the deterministic ShamHub thread +gs review resolve thread-2 +stderr 'Resolved thread thread-2' +gs review list +stderr 'resolved' + +gs review reopen thread-2 +stderr 'Reopened thread thread-2' +gs review list +stderr 'open' -- repo/util.go -- package main diff --git a/testdata/script/branch_comment_stale.txt b/testdata/script/review_stale.txt similarity index 90% rename from testdata/script/branch_comment_stale.txt rename to testdata/script/review_stale.txt index a821f0b9a..980ab9094 100644 --- a/testdata/script/branch_comment_stale.txt +++ b/testdata/script/review_stale.txt @@ -27,13 +27,13 @@ gs branch submit --fill stderr 'Created #' # Two inline comments at different lines on the same head commit. -gs branch comment add main.go:4 -m 'Comment on line 4.' +gs review comment main.go:4 -m 'Comment on line 4.' --no-draft stderr 'Posted comment' -gs branch comment add main.go:5 -m 'Comment on line 5.' +gs review comment main.go:5 -m 'Comment on line 5.' --no-draft stderr 'Posted comment' # Both comments are fresh: neither is outdated yet. -gs branch comment list +gs review list ! stderr 'outdated' # Amend the file so line 4 changes but line 5 does not, then @@ -45,7 +45,7 @@ gs branch submit --force # Comment 1 (on line 4) is now outdated; comment 2 (on line 5) # is not. The list output reflects this: -gs branch comment list +gs review list stderr 'Comment on line 4' stderr 'outdated' stderr 'Comment on line 5' @@ -53,7 +53,7 @@ stderr 'Comment on line 5' # Sanity: the JSON list (used by the VSCode extension) reports # outdated as the per-comment status. Comment 1 outdated, comment # 2 still open. -gs branch comment list --json +gs review list --json stdout '"body":"Comment on line 4\."' stdout '"body":"Comment on line 5\."' stdout '"status":"outdated"' From 6e563bcf604c27b7c11dd767b1d4f770a157ae4e Mon Sep 17 00:00:00 2001 From: Edmund Kohlwey Date: Mon, 1 Jun 2026 14:27:42 -0400 Subject: [PATCH 07/11] branch comment: Support review comment scopes `gs branch comment add` now accepts a file, an inclusive `file:start-end` range, or `--pr` for an unanchored review body. Single-line `file:line` anchors remain supported. `--pr` uses `SubmitReviewRequest.Body`. Because `ListReviewThreads` does not return review bodies, PR-level comments do not appear in `gs branch comment list`. --- doc/includes/cli-reference.md | 12 +- review_comment.go | 183 +++++++++++++++++++++++++----- testdata/help/review_comment.txt | 15 ++- testdata/script/review_scopes.txt | 70 ++++++++++++ 4 files changed, 247 insertions(+), 33 deletions(-) create mode 100644 testdata/script/review_scopes.txt diff --git a/doc/includes/cli-reference.md b/doc/includes/cli-reference.md index d61ca2655..37e0d3e5a 100644 --- a/doc/includes/cli-reference.md +++ b/doc/includes/cli-reference.md @@ -1470,14 +1470,19 @@ This command requires at least Git 2.45. ### git-spice review comment {#gs-review-comment} ``` -gs review comment [flags] +gs review comment [] [flags] ``` Draft or post a review comment Adds a review comment to the change request for the current branch. -Provide the file and line number as file.go:42. +The anchor controls the comment scope: + + file.go:42 anchored to that line + file.go:42-50 anchored to that line range + file.go anchored to the file + (empty) + --pr not anchored to a file Comments are saved as local drafts by default. Use --no-draft to post immediately. @@ -1486,11 +1491,12 @@ If no message is given with -m, an editor is opened. **Arguments** -* `file-and-line`: File and line in the form file.go:42. +* `anchor`: Comment anchor: file.go, file.go:42, or file.go:42-50. Omit with --pr. **Flags** * `-m`, `--message=MSG`: Comment body. Opens editor if not provided. +* `--pr`: Post an unanchored change request comment. * `--[no-]draft`: Save the comment as a local draft instead of posting it. * `-b`, `--branch=BRANCH`: Branch to comment on. Defaults to the current branch. diff --git a/review_comment.go b/review_comment.go index 548e24612..566c9c555 100644 --- a/review_comment.go +++ b/review_comment.go @@ -20,17 +20,23 @@ import ( ) type reviewCommentCmd struct { - FileAndLine string `arg:"" help:"File and line in the form file.go:42."` - Message string `short:"m" placeholder:"MSG" help:"Comment body. Opens editor if not provided."` - Draft bool `negatable:"" default:"true" help:"Save the comment as a local draft instead of posting it."` - Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch to comment on. Defaults to the current branch."` + Anchor string `arg:"" optional:"" help:"Comment anchor: file.go, file.go:42, or file.go:42-50. Omit with --pr."` + Message string `short:"m" placeholder:"MSG" help:"Comment body. Opens editor if not provided."` + PR bool `name:"pr" help:"Post an unanchored change request comment."` + Draft bool `negatable:"" default:"true" help:"Save the comment as a local draft instead of posting it."` + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch to comment on. Defaults to the current branch."` } func (*reviewCommentCmd) Help() string { return text.Dedent(` Adds a review comment to the change request for the current branch. - Provide the file and line number as file.go:42. + The anchor controls the comment scope: + + file.go:42 anchored to that line + file.go:42-50 anchored to that line range + file.go anchored to the file + (empty) + --pr not anchored to a file Comments are saved as local drafts by default. Use --no-draft to post immediately. @@ -53,7 +59,7 @@ func (cmd *reviewCommentCmd) Run( return err } - file, line, err := parseFileAndLine(cmd.FileAndLine) + anchor, err := parseReviewCommentAnchor(cmd.Anchor, cmd.PR) if err != nil { return err } @@ -63,9 +69,14 @@ func (cmd *reviewCommentCmd) Run( } if cmd.Draft { + if anchor.PR || anchor.StartLine == 0 || anchor.StartLine != anchor.EndLine { + return errors.New( + "draft comments require a single-line file:line anchor", + ) + } return saveReviewDraft(ctx, log, store, branch, state.StagedComment{ - File: file, - Line: line, + File: anchor.Path, + Line: anchor.StartLine, Body: body, }) } @@ -76,20 +87,48 @@ func (cmd *reviewCommentCmd) Run( if err != nil { return err } - diff, err := wt.DiffBranchBytes(ctx, b.Base, branch) - if err != nil { - return fmt.Errorf("get diff: %w", err) + if anchor.PR { + if _, err := reviewRepo.SubmitReview( + ctx, + b.Change.ChangeID(), + forge.SubmitReviewRequest{Body: body}, + ); err != nil { + return fmt.Errorf("post review comment: %w", err) + } + log.Infof("Posted comment on %s.", b.Change.ChangeID()) + return nil } - patch, err := reviewdiff.Parse(diff) - if err != nil { - return fmt.Errorf("parse diff: %w", err) + + comment := forge.SubmitReviewCommentRequest{ + Path: anchor.Path, + Body: body, } - if !patch.ContainsLine(file, line) { - return fmt.Errorf( - "review diff does not contain %s:%d", - file, - line, - ) + if anchor.StartLine > 0 { + diff, err := wt.DiffBranchBytes(ctx, b.Base, branch) + if err != nil { + return fmt.Errorf("get diff: %w", err) + } + patch, err := reviewdiff.Parse(diff) + if err != nil { + return fmt.Errorf("parse diff: %w", err) + } + if !patch.ContainsLineRange( + anchor.Path, + anchor.StartLine, + anchor.EndLine, + ) { + return fmt.Errorf( + "review diff does not contain %s:%d-%d", + anchor.Path, + anchor.StartLine, + anchor.EndLine, + ) + } + comment.Range = forge.ReviewThreadRange{ + StartLine: anchor.StartLine, + EndLine: anchor.EndLine, + } + comment.Side = forge.ReviewThreadSideRight } return postReviewComment( @@ -97,15 +136,52 @@ func (cmd *reviewCommentCmd) Run( log, reviewRepo, b.Change.ChangeID(), - forge.SubmitReviewCommentRequest{ - Path: file, - Range: forge.ReviewThreadLine(line), - Body: body, - Side: forge.ReviewThreadSideRight, - }, + comment, ) } +// reviewCommentAnchor is the parsed location accepted by review comment. +// PR distinguishes an unanchored review body from a file-level thread, whose +// line range is also zero. +type reviewCommentAnchor struct { + PR bool + Path string + StartLine int + EndLine int +} + +func parseReviewCommentAnchor( + value string, + pr bool, +) (reviewCommentAnchor, error) { + if pr { + if value != "" { + return reviewCommentAnchor{}, fmt.Errorf( + "--pr takes no anchor argument, got %q", value, + ) + } + return reviewCommentAnchor{PR: true}, nil + } + if value == "" { + return reviewCommentAnchor{}, errors.New( + "comment anchor is required unless --pr is used", + ) + } + if !strings.Contains(value, ":") { + return reviewCommentAnchor{Path: value}, nil + } + + file, start, end, err := parseFileAndRange(value) + if err != nil { + return reviewCommentAnchor{}, err + } + return reviewCommentAnchor{ + Path: file, + StartLine: start, + EndLine: end, + }, nil +} + func reviewBranch( ctx context.Context, wt *git.Worktree, @@ -235,6 +311,61 @@ func parseFileAndLine(value string) (string, int, error) { return file, line, nil } +// parseFileAndRange parses file.go:42 or file.go:42-50. +// The returned end equals start for a single-line anchor. +func parseFileAndRange( + value string, +) (file string, start, end int, err error) { + idx := strings.LastIndex(value, ":") + if idx < 0 { + return "", 0, 0, fmt.Errorf( + "expected file:line or file:start-end, got %q", value, + ) + } + file = value[:idx] + lineSpec := value[idx+1:] + + before, after, hasRange := strings.Cut(lineSpec, "-") + if !hasRange { + start, err = strconv.Atoi(lineSpec) + if err != nil { + return "", 0, 0, fmt.Errorf( + "invalid line number in %q: %w", value, err, + ) + } + if start <= 0 { + return "", 0, 0, fmt.Errorf( + "line number must be positive, got %d", start, + ) + } + return file, start, start, nil + } + + start, err = strconv.Atoi(before) + if err != nil { + return "", 0, 0, fmt.Errorf( + "invalid range start in %q: %w", value, err, + ) + } + end, err = strconv.Atoi(after) + if err != nil { + return "", 0, 0, fmt.Errorf( + "invalid range end in %q: %w", value, err, + ) + } + if start <= 0 || end <= 0 { + return "", 0, 0, fmt.Errorf( + "line numbers must be positive in %q", value, + ) + } + if end <= start { + return "", 0, 0, fmt.Errorf( + "range end must be greater than start in %q", value, + ) + } + return file, start, end, nil +} + // editReviewCommentBody opens the configured editor with initial as its // starting contents and returns the edited comment body. func editReviewCommentBody( diff --git a/testdata/help/review_comment.txt b/testdata/help/review_comment.txt index d34df319c..7f120eeac 100644 --- a/testdata/help/review_comment.txt +++ b/testdata/help/review_comment.txt @@ -1,9 +1,14 @@ -Usage: gs review comment [flags] +Usage: gs review comment [] [flags] Draft or post a review comment -Adds a review comment to the change request for the current branch. Provide the -file and line number as file.go:42. +Adds a review comment to the change request for the current branch. The anchor +controls the comment scope: + + file.go:42 anchored to that line + file.go:42-50 anchored to that line range + file.go anchored to the file + (empty) + --pr not anchored to a file Comments are saved as local drafts by default. Use --no-draft to post immediately. @@ -11,10 +16,12 @@ immediately. If no message is given with -m, an editor is opened. Arguments: - File and line in the form file.go:42. + [] Comment anchor: file.go, file.go:42, or file.go:42-50. Omit with + --pr. Flags: -m, --message=MSG Comment body. Opens editor if not provided. + --pr Post an unanchored change request comment. --[no-]draft Save the comment as a local draft instead of posting it. -b, --branch=BRANCH Branch to comment on. Defaults to the current branch. diff --git a/testdata/script/review_scopes.txt b/testdata/script/review_scopes.txt new file mode 100644 index 000000000..582dd3574 --- /dev/null +++ b/testdata/script/review_scopes.txt @@ -0,0 +1,70 @@ +# Verifies the three comment scopes: line, file, and pr. +# Each scope corresponds to a different positional-argument form on +# 'gs review comment', and surfaces through the review-thread API +# the extension uses. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +cd repo +git init +git commit --allow-empty -m 'Initial commit' + +gs repo init +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +git add main.go +gs bc -m 'Add main' feature1 +gs branch submit --fill +stderr 'Created #' + +# Line scope: existing form. +gs review comment main.go:4 -m 'Line-scope comment.' --no-draft +stderr 'Posted comment' + +# File scope: path with no ':'. +gs review comment main.go -m 'File-scope comment.' --no-draft +stderr 'Posted comment' + +# PR scope: --pr flag, no positional argument. +gs review comment --pr -m 'PR-scope comment.' --no-draft +stderr 'Posted comment' + +# Thread comments are listed with the right anchor fields: +# - line-scope: path + line +# - file-scope: path, no line +# +# The text-mode output makes the missing-line / missing-path +# distinction easiest to assert: line entries print as +# "path:line", and file entries print as "path:0". +# +# PR-scope comments are review bodies, not review threads. The forge +# API can submit them but does not include them in ListReviewThreads. +gs review list +stderr 'main\.go:4' +stderr 'main\.go:0' +stderr 'Line-scope comment' +stderr 'File-scope comment' +! stderr 'PR-scope comment' + +# --pr with a positional argument is rejected. +! gs review comment --pr unwanted.go -m 'oops' --no-draft +stderr '--pr takes no anchor argument' + +# Missing anchor without --pr is rejected. +! gs review comment -m 'oops' --no-draft +stderr 'comment anchor is required' + +-- repo/main.go -- +package main + +func main() { + println("hello") + println("world") +} From 55db57a2d19f471d536d594074f060db5cf5579e Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sun, 23 Aug 2026 21:32:31 -0700 Subject: [PATCH 08/11] branch comment: Limit scopes to review threads Accept immediate review comments anchored to a whole file, one line, or an inclusive line range. Parse those forms into a typed review-thread range and check the complete anchor against the selected branch patch before submission. Drop `--pr` because review bodies are not returned by `ListReviewThreads` and overlap ordinary change comments. Staged comments retain their existing single-line syntax until their persistence and lifetime contracts are decided. --- doc/includes/cli-reference.md | 4 +- review_comment.go | 114 +++++++++++------------------- testdata/help/review_comment.txt | 5 +- testdata/script/review_scopes.txt | 83 ++++++++++++++++------ 4 files changed, 105 insertions(+), 101 deletions(-) diff --git a/doc/includes/cli-reference.md b/doc/includes/cli-reference.md index 37e0d3e5a..f8657355d 100644 --- a/doc/includes/cli-reference.md +++ b/doc/includes/cli-reference.md @@ -1482,7 +1482,6 @@ The anchor controls the comment scope: file.go:42 anchored to that line file.go:42-50 anchored to that line range file.go anchored to the file - (empty) + --pr not anchored to a file Comments are saved as local drafts by default. Use --no-draft to post immediately. @@ -1491,12 +1490,11 @@ If no message is given with -m, an editor is opened. **Arguments** -* `anchor`: Comment anchor: file.go, file.go:42, or file.go:42-50. Omit with --pr. +* `anchor`: Comment anchor: file.go, file.go:42, or file.go:42-50. **Flags** * `-m`, `--message=MSG`: Comment body. Opens editor if not provided. -* `--pr`: Post an unanchored change request comment. * `--[no-]draft`: Save the comment as a local draft instead of posting it. * `-b`, `--branch=BRANCH`: Branch to comment on. Defaults to the current branch. diff --git a/review_comment.go b/review_comment.go index 566c9c555..cdd6f19e9 100644 --- a/review_comment.go +++ b/review_comment.go @@ -20,9 +20,8 @@ import ( ) type reviewCommentCmd struct { - Anchor string `arg:"" optional:"" help:"Comment anchor: file.go, file.go:42, or file.go:42-50. Omit with --pr."` + Anchor string `arg:"" optional:"" help:"Comment anchor: file.go, file.go:42, or file.go:42-50."` Message string `short:"m" placeholder:"MSG" help:"Comment body. Opens editor if not provided."` - PR bool `name:"pr" help:"Post an unanchored change request comment."` Draft bool `negatable:"" default:"true" help:"Save the comment as a local draft instead of posting it."` Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch to comment on. Defaults to the current branch."` } @@ -36,7 +35,6 @@ func (*reviewCommentCmd) Help() string { file.go:42 anchored to that line file.go:42-50 anchored to that line range file.go anchored to the file - (empty) + --pr not anchored to a file Comments are saved as local drafts by default. Use --no-draft to post immediately. @@ -59,7 +57,7 @@ func (cmd *reviewCommentCmd) Run( return err } - anchor, err := parseReviewCommentAnchor(cmd.Anchor, cmd.PR) + anchor, err := parseReviewCommentAnchor(cmd.Anchor) if err != nil { return err } @@ -69,14 +67,14 @@ func (cmd *reviewCommentCmd) Run( } if cmd.Draft { - if anchor.PR || anchor.StartLine == 0 || anchor.StartLine != anchor.EndLine { + if anchor.Range.IsZero() || anchor.Range.StartLine != anchor.Range.EndLine { return errors.New( "draft comments require a single-line file:line anchor", ) } return saveReviewDraft(ctx, log, store, branch, state.StagedComment{ File: anchor.Path, - Line: anchor.StartLine, + Line: anchor.Range.StartLine, Body: body, }) } @@ -87,48 +85,31 @@ func (cmd *reviewCommentCmd) Run( if err != nil { return err } - if anchor.PR { - if _, err := reviewRepo.SubmitReview( - ctx, - b.Change.ChangeID(), - forge.SubmitReviewRequest{Body: body}, - ); err != nil { - return fmt.Errorf("post review comment: %w", err) - } - log.Infof("Posted comment on %s.", b.Change.ChangeID()) - return nil + diff, err := wt.DiffBranchBytes(ctx, b.Base, branch) + if err != nil { + return fmt.Errorf("get diff: %w", err) } - - comment := forge.SubmitReviewCommentRequest{ - Path: anchor.Path, - Body: body, + patch, err := reviewdiff.Parse(diff) + if err != nil { + return fmt.Errorf("parse diff: %w", err) } - if anchor.StartLine > 0 { - diff, err := wt.DiffBranchBytes(ctx, b.Base, branch) - if err != nil { - return fmt.Errorf("get diff: %w", err) - } - patch, err := reviewdiff.Parse(diff) - if err != nil { - return fmt.Errorf("parse diff: %w", err) - } - if !patch.ContainsLineRange( + if anchor.Range.IsZero() && !patch.ContainsFile(anchor.Path) { + return fmt.Errorf( + "review diff does not contain file %q", anchor.Path, - anchor.StartLine, - anchor.EndLine, - ) { - return fmt.Errorf( - "review diff does not contain %s:%d-%d", - anchor.Path, - anchor.StartLine, - anchor.EndLine, - ) - } - comment.Range = forge.ReviewThreadRange{ - StartLine: anchor.StartLine, - EndLine: anchor.EndLine, - } - comment.Side = forge.ReviewThreadSideRight + ) + } + if !anchor.Range.IsZero() && !patch.ContainsLineRange( + anchor.Path, + anchor.Range.StartLine, + anchor.Range.EndLine, + ) { + return fmt.Errorf( + "review diff does not contain %s:%d-%d", + anchor.Path, + anchor.Range.StartLine, + anchor.Range.EndLine, + ) } return postReviewComment( @@ -136,36 +117,25 @@ func (cmd *reviewCommentCmd) Run( log, reviewRepo, b.Change.ChangeID(), - comment, + forge.SubmitReviewCommentRequest{ + Path: anchor.Path, + Range: anchor.Range, + Body: body, + Side: forge.ReviewThreadSideRight, + }, ) } -// reviewCommentAnchor is the parsed location accepted by review comment. -// PR distinguishes an unanchored review body from a file-level thread, whose -// line range is also zero. +// reviewCommentAnchor is the parsed file or inclusive line range accepted by +// review comment. A zero range identifies the whole file. type reviewCommentAnchor struct { - PR bool - Path string - StartLine int - EndLine int + Path string + Range forge.ReviewThreadRange } -func parseReviewCommentAnchor( - value string, - pr bool, -) (reviewCommentAnchor, error) { - if pr { - if value != "" { - return reviewCommentAnchor{}, fmt.Errorf( - "--pr takes no anchor argument, got %q", value, - ) - } - return reviewCommentAnchor{PR: true}, nil - } +func parseReviewCommentAnchor(value string) (reviewCommentAnchor, error) { if value == "" { - return reviewCommentAnchor{}, errors.New( - "comment anchor is required unless --pr is used", - ) + return reviewCommentAnchor{}, errors.New("comment anchor is required") } if !strings.Contains(value, ":") { return reviewCommentAnchor{Path: value}, nil @@ -176,9 +146,11 @@ func parseReviewCommentAnchor( return reviewCommentAnchor{}, err } return reviewCommentAnchor{ - Path: file, - StartLine: start, - EndLine: end, + Path: file, + Range: forge.ReviewThreadRange{ + StartLine: start, + EndLine: end, + }, }, nil } diff --git a/testdata/help/review_comment.txt b/testdata/help/review_comment.txt index 7f120eeac..4141f0b23 100644 --- a/testdata/help/review_comment.txt +++ b/testdata/help/review_comment.txt @@ -8,7 +8,6 @@ controls the comment scope: file.go:42 anchored to that line file.go:42-50 anchored to that line range file.go anchored to the file - (empty) + --pr not anchored to a file Comments are saved as local drafts by default. Use --no-draft to post immediately. @@ -16,12 +15,10 @@ immediately. If no message is given with -m, an editor is opened. Arguments: - [] Comment anchor: file.go, file.go:42, or file.go:42-50. Omit with - --pr. + [] Comment anchor: file.go, file.go:42, or file.go:42-50. Flags: -m, --message=MSG Comment body. Opens editor if not provided. - --pr Post an unanchored change request comment. --[no-]draft Save the comment as a local draft instead of posting it. -b, --branch=BRANCH Branch to comment on. Defaults to the current branch. diff --git a/testdata/script/review_scopes.txt b/testdata/script/review_scopes.txt index 582dd3574..ba8b1de0c 100644 --- a/testdata/script/review_scopes.txt +++ b/testdata/script/review_scopes.txt @@ -1,7 +1,4 @@ -# Verifies the three comment scopes: line, file, and pr. -# Each scope corresponds to a different positional-argument form on -# 'gs review comment', and surfaces through the review-thread API -# the extension uses. +# Verifies line, line-range, and file review-comment anchors. as 'Test ' at '2024-04-05T16:40:32Z' @@ -28,36 +25,27 @@ stderr 'Created #' gs review comment main.go:4 -m 'Line-scope comment.' --no-draft stderr 'Posted comment' -# File scope: path with no ':'. -gs review comment main.go -m 'File-scope comment.' --no-draft +# Line-range scope: inclusive start and end lines. +gs review comment main.go:4-5 -m 'Range-scope comment.' --no-draft stderr 'Posted comment' -# PR scope: --pr flag, no positional argument. -gs review comment --pr -m 'PR-scope comment.' --no-draft +# File scope: path with no ':'. +gs review comment main.go -m 'File-scope comment.' --no-draft stderr 'Posted comment' -# Thread comments are listed with the right anchor fields: -# - line-scope: path + line -# - file-scope: path, no line -# -# The text-mode output makes the missing-line / missing-path -# distinction easiest to assert: line entries print as -# "path:line", and file entries print as "path:0". -# -# PR-scope comments are review bodies, not review threads. The forge -# API can submit them but does not include them in ListReviewThreads. +# Text output lists the line and file-level anchors. gs review list stderr 'main\.go:4' stderr 'main\.go:0' stderr 'Line-scope comment' +stderr 'Range-scope comment' stderr 'File-scope comment' -! stderr 'PR-scope comment' -# --pr with a positional argument is rejected. -! gs review comment --pr unwanted.go -m 'oops' --no-draft -stderr '--pr takes no anchor argument' +# ShamHub's stable YAML dump preserves the complete inclusive range. +shamhub dump reviews 1 +cmp stdout $WORK/golden/reviews.yaml -# Missing anchor without --pr is rejected. +# Missing anchor is rejected. ! gs review comment -m 'oops' --no-draft stderr 'comment anchor is required' @@ -68,3 +56,52 @@ func main() { println("hello") println("world") } +-- golden/reviews.yaml -- +changes: + - change: 1 + submissions: + - submitter: alice + disposition: comment + commentIDs: + - 2 + - submitter: alice + disposition: comment + commentIDs: + - 3 + - submitter: alice + disposition: comment + commentIDs: + - 4 + threads: + - id: thread-2 + path: main.go + range: + start: 4 + end: 4 + side: right + resolved: false + outdated: false + comments: + - id: 2 + author: alice + body: Line-scope comment. + - id: thread-3 + path: main.go + range: + start: 4 + end: 5 + side: right + resolved: false + outdated: false + comments: + - id: 3 + author: alice + body: Range-scope comment. + - id: thread-4 + path: main.go + resolved: false + outdated: false + comments: + - id: 4 + author: alice + body: File-scope comment. From ebe67f3281837ac8f515a6635be6413ed2408617 Mon Sep 17 00:00:00 2001 From: Edmund Kohlwey Date: Tue, 2 Jun 2026 15:39:51 -0400 Subject: [PATCH 09/11] branch comment: Extend JSON review output `gs branch comment list --json` now includes `scope`, `side`, `range`, `commitSHA`, `resolved`, and `stale` for editor integrations. Resolution and staleness are omitted when a forge returns `nil` for unsupported state. The existing `status` field remains for compatibility. --- review_list.go | 59 +++++++++++++- testdata/script/review_list_json.txt | 2 +- testdata/script/review_list_json_extended.txt | 78 +++++++++++++++++++ 3 files changed, 134 insertions(+), 5 deletions(-) create mode 100644 testdata/script/review_list_json_extended.txt diff --git a/review_list.go b/review_list.go index a64c3a5c9..f109fa92b 100644 --- a/review_list.go +++ b/review_list.go @@ -313,14 +313,18 @@ func (cmd *reviewListCmd) writeJSON( } func stagedToJSON(c *state.StagedComment) jsonComment { - return jsonComment{ + comment := jsonComment{ Kind: "draft", ID: fmt.Sprintf("%d", c.ID), - Path: c.File, - Line: c.Line, Body: c.Body, ThreadID: c.ThreadID, } + if c.ThreadID == "" { + comment.Scope = "line" + comment.Path = c.File + comment.Line = c.Line + } + return comment } func forgeToJSON(c *listedReviewComment) jsonComment { @@ -328,17 +332,36 @@ func forgeToJSON(c *listedReviewComment) jsonComment { if !c.Comment.CreatedAt.IsZero() { createdAt = &c.Comment.CreatedAt } - return jsonComment{ + + scope := "line" + if c.Thread.Range.IsZero() { + scope = "file" + } + comment := jsonComment{ Kind: "forge", ID: c.Comment.ID.String(), + Scope: scope, Path: c.Thread.Path, Line: c.Thread.Range.StartLine, + CommitSHA: c.Thread.CommitHash.String(), Body: c.Comment.Body, ThreadID: c.Thread.ID.String(), Author: c.Comment.Author, + Resolved: c.Thread.Resolved, + Stale: c.Thread.Outdated, Status: commentStatus(c), CreatedAt: createdAt, } + if scope == "line" { + comment.Side = c.Thread.Side.String() + if c.Thread.Range.StartLine != c.Thread.Range.EndLine { + comment.Range = &jsonCommentRange{ + Start: c.Thread.Range.StartLine, + End: c.Thread.Range.EndLine, + } + } + } + return comment } // jsonComment is the JSON representation @@ -352,12 +375,26 @@ type jsonComment struct { // For forge comments: forge-specific ID. ID string `json:"id"` + // Scope is "file" or "line". + // It is omitted for draft replies, which inherit their thread's scope. + Scope string `json:"scope,omitempty"` + // Path is the file path relative to the repo root. Path string `json:"path,omitempty"` // Line is the line number in the file. Line int `json:"line,omitempty"` + // Range is set when a line comment spans more than one line. + Range *jsonCommentRange `json:"range,omitempty"` + + // Side is the diff side for a line comment. + Side string `json:"side,omitempty"` + + // CommitSHA is the reviewed revision against which the thread was created. + // It is empty when the forge does not expose that revision. + CommitSHA string `json:"commitSHA,omitempty"` + // Body is the full markdown body of the comment. Body string `json:"body"` @@ -368,6 +405,14 @@ type jsonComment struct { // Only set for forge comments. Author string `json:"author,omitempty"` + // Resolved reports whether the thread is resolved. + // It is omitted when the forge does not expose resolution state. + Resolved *bool `json:"resolved,omitempty"` + + // Stale reports whether the thread belongs to an earlier revision. + // It is omitted when the forge does not expose outdated state. + Stale *bool `json:"stale,omitempty"` + // Status is "open", "resolved", or "outdated". // Only set for forge comments. Status string `json:"status,omitempty"` @@ -376,3 +421,9 @@ type jsonComment struct { // Only set for forge comments. CreatedAt *time.Time `json:"createdAt,omitempty"` } + +// jsonCommentRange is an inclusive multi-line range in JSON output. +type jsonCommentRange struct { + Start int `json:"start"` + End int `json:"end"` +} diff --git a/testdata/script/review_list_json.txt b/testdata/script/review_list_json.txt index 438d1702d..4dbb6e49c 100644 --- a/testdata/script/review_list_json.txt +++ b/testdata/script/review_list_json.txt @@ -61,4 +61,4 @@ func main() { } -- golden/draft_only.json -- -{"kind":"draft","id":"1","path":"main.go","line":5,"body":"Add error handling here."} +{"kind":"draft","id":"1","scope":"line","path":"main.go","line":5,"body":"Add error handling here."} diff --git a/testdata/script/review_list_json_extended.txt b/testdata/script/review_list_json_extended.txt new file mode 100644 index 000000000..e6a471b8f --- /dev/null +++ b/testdata/script/review_list_json_extended.txt @@ -0,0 +1,78 @@ +# Verifies the extended 'gs review list --json' shape that +# the VSCode extension consumes. Covers: +# - scope, side, resolved (always emitted as bool), stale, +# range (multi-line), kind, status (legacy) + +as 'Test ' +at '2024-04-05T16:40:32Z' + +cd repo +git init +git commit --allow-empty -m 'Initial commit' + +gs repo init +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +git add main.go +gs bc -m 'Add main' feature1 +gs branch submit --fill +stderr 'Created #' + +# Post a single-line comment. +gs review comment main.go:3 -m 'Single-line comment.' --no-draft +stderr 'Posted comment' + +# Post a multi-line range comment spanning lines 4-5. +gs review comment main.go:4-5 -m 'Range comment.' --no-draft +stderr 'Posted comment' + +# Single-line comment: scope, side, resolved (false), stale (false). +gs review list --json +stdout '"scope":"line"' +stdout '"side":"right"' +stdout '"commitSHA":"[0-9a-f]{40}"' +stdout '"resolved":false' +stdout '"stale":false' +stdout '"status":"open"' +stdout '"body":"Single-line comment."' +stdout '"body":"Range comment."' + +# Range comment: range:{start,end} present. +gs review list --json +stdout '"range":{"start":4,"end":5}' + +# Resolve the first review thread. +gs review resolve thread-2 +gs review list --json +stdout '"resolved":true' +stdout '"status":"resolved"' + +# Change a line covered by the second comment, then update the change. +cp $WORK/extra/main-edited.go main.go +git add main.go +gs ca --no-edit +gs branch submit --force +gs review list --json +stdout '"stale":true' +stdout '"status":"outdated"' + +-- repo/main.go -- +package main + +func main() { + println("hello") + println("world") +} +-- extra/main-edited.go -- +package main + +func main() { + println("greetings") + println("world") +} From e759b9071ab12ab85710ee03174fd2e44ee2aed2 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sun, 23 Aug 2026 21:37:14 -0700 Subject: [PATCH 10/11] branch comment: Verify extended JSON records Compare complete deterministic JSON records for inclusive ranges and file-level threads instead of matching fragments of serialized output. A focused conversion test also preserves omission of resolution, staleness, commit, and timestamp fields when the forge does not expose them. --- review_list.go | 3 +- review_list_test.go | 58 +++++++++++++++++++ testdata/script/review_list_json_extended.txt | 53 +++-------------- testdata/script/review_list_json_file.txt | 35 +++++++++++ 4 files changed, 102 insertions(+), 47 deletions(-) create mode 100644 review_list_test.go create mode 100644 testdata/script/review_list_json_file.txt diff --git a/review_list.go b/review_list.go index f109fa92b..2797e0e29 100644 --- a/review_list.go +++ b/review_list.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "strconv" "strings" "time" @@ -315,7 +316,7 @@ func (cmd *reviewListCmd) writeJSON( func stagedToJSON(c *state.StagedComment) jsonComment { comment := jsonComment{ Kind: "draft", - ID: fmt.Sprintf("%d", c.ID), + ID: strconv.Itoa(c.ID), Body: c.Body, ThreadID: c.ThreadID, } diff --git a/review_list_test.go b/review_list_test.go new file mode 100644 index 000000000..a8868d3bf --- /dev/null +++ b/review_list_test.go @@ -0,0 +1,58 @@ +package main + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.abhg.dev/gs/internal/forge" +) + +func TestReviewListJSONUnsupportedThreadState(t *testing.T) { + var stdout bytes.Buffer + err := new(reviewListCmd).writeJSON( + &stdout, + nil, + []*listedReviewComment{ + { + Thread: &forge.ReviewThread{ + ID: testReviewThreadID("thread-1"), + Path: "review.go", + Range: forge.ReviewThreadLine(3), + Side: forge.ReviewThreadSideRight, + }, + Comment: &forge.ReviewComment{ + ID: testReviewCommentID("comment-1"), + Body: "Consider a constant.", + Author: "reviewer", + }, + }, + }, + ) + require.NoError(t, err) + assert.JSONEq(t, `{ + "kind": "forge", + "id": "comment-1", + "scope": "line", + "path": "review.go", + "line": 3, + "side": "right", + "body": "Consider a constant.", + "threadID": "thread-1", + "author": "reviewer", + "status": "open" + }`, stdout.String()) +} + +type testReviewThreadID string + +func (id testReviewThreadID) String() string { + return string(id) +} + +type testReviewCommentID string + +func (id testReviewCommentID) String() string { + return string(id) +} diff --git a/testdata/script/review_list_json_extended.txt b/testdata/script/review_list_json_extended.txt index e6a471b8f..5817c4907 100644 --- a/testdata/script/review_list_json_extended.txt +++ b/testdata/script/review_list_json_extended.txt @@ -1,7 +1,4 @@ -# Verifies the extended 'gs review list --json' shape that -# the VSCode extension consumes. Covers: -# - scope, side, resolved (always emitted as bool), stale, -# range (multi-line), kind, status (legacy) +# Reports a complete range-thread record for editor integrations. as 'Test ' at '2024-04-05T16:40:32Z' @@ -24,43 +21,12 @@ gs bc -m 'Add main' feature1 gs branch submit --fill stderr 'Created #' -# Post a single-line comment. -gs review comment main.go:3 -m 'Single-line comment.' --no-draft -stderr 'Posted comment' +# Seed every supported state so one record exposes the complete JSON shape. +shamhub review comment post --id 100 --author alice --path main.go --range 4:5 --side right --resolved --outdated alice/example 1 'Range comment.' +stdout '100' -# Post a multi-line range comment spanning lines 4-5. -gs review comment main.go:4-5 -m 'Range comment.' --no-draft -stderr 'Posted comment' - -# Single-line comment: scope, side, resolved (false), stale (false). -gs review list --json -stdout '"scope":"line"' -stdout '"side":"right"' -stdout '"commitSHA":"[0-9a-f]{40}"' -stdout '"resolved":false' -stdout '"stale":false' -stdout '"status":"open"' -stdout '"body":"Single-line comment."' -stdout '"body":"Range comment."' - -# Range comment: range:{start,end} present. gs review list --json -stdout '"range":{"start":4,"end":5}' - -# Resolve the first review thread. -gs review resolve thread-2 -gs review list --json -stdout '"resolved":true' -stdout '"status":"resolved"' - -# Change a line covered by the second comment, then update the change. -cp $WORK/extra/main-edited.go main.go -git add main.go -gs ca --no-edit -gs branch submit --force -gs review list --json -stdout '"stale":true' -stdout '"status":"outdated"' +cmpenvJSON stdout $WORK/golden/range.json -- repo/main.go -- package main @@ -69,10 +35,5 @@ func main() { println("hello") println("world") } --- extra/main-edited.go -- -package main - -func main() { - println("greetings") - println("world") -} +-- golden/range.json -- +{"kind":"forge","id":"100","scope":"line","path":"main.go","line":4,"range":{"start":4,"end":5},"side":"right","commitSHA":"4105957e3fefee17df99ae8581ed9c412dd150c2","body":"Range comment.","threadID":"thread-100","author":"alice","resolved":true,"stale":true,"status":"outdated","createdAt":"2024-04-05T16:40:32Z"} diff --git a/testdata/script/review_list_json_file.txt b/testdata/script/review_list_json_file.txt new file mode 100644 index 000000000..1ced59928 --- /dev/null +++ b/testdata/script/review_list_json_file.txt @@ -0,0 +1,35 @@ +# Reports file-level threads without line-only JSON fields. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +cd repo +git init +git commit --allow-empty -m 'Initial commit' + +gs repo init +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +git add main.go +gs bc -m 'Add main' feature1 +gs branch submit --fill +stderr 'Created #' + +shamhub review comment post --id 200 --author alice --path main.go alice/example 1 'File comment.' +stdout '200' + +gs review list --json +cmpenvJSON stdout $WORK/golden/file.json + +-- repo/main.go -- +package main + +func main() {} +-- golden/file.json -- +{"kind":"forge","id":"200","scope":"file","path":"main.go","commitSHA":"7e287bba2ffb67941482f25ec290a94becd22ea7","body":"File comment.","threadID":"thread-200","author":"alice","resolved":false,"stale":false,"status":"open","createdAt":"2024-04-05T16:40:32Z"} From 6a7577edf83816ea18e5ef308f867059a7f204a5 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sun, 30 Aug 2026 15:50:10 -0700 Subject: [PATCH 11/11] review: Remove obsolete line parser --- review_comment.go | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/review_comment.go b/review_comment.go index cdd6f19e9..b47b1c48e 100644 --- a/review_comment.go +++ b/review_comment.go @@ -260,29 +260,6 @@ func postReviewComment( return nil } -// parseFileAndLine parses a file.go:42 argument into its path and line. -func parseFileAndLine(value string) (string, int, error) { - idx := strings.LastIndex(value, ":") - if idx < 0 { - return "", 0, fmt.Errorf( - "expected file:line format, got %q", value, - ) - } - file := value[:idx] - line, err := strconv.Atoi(value[idx+1:]) - if err != nil { - return "", 0, fmt.Errorf( - "invalid line number in %q: %w", value, err, - ) - } - if line <= 0 { - return "", 0, fmt.Errorf( - "line number must be positive, got %d", line, - ) - } - return file, line, nil -} - // parseFileAndRange parses file.go:42 or file.go:42-50. // The returned end equals start for a single-line anchor. func parseFileAndRange(