diff --git a/doc/includes/cli-reference.md b/doc/includes/cli-reference.md index c440781c..e5eba202 100644 --- a/doc/includes/cli-reference.md +++ b/doc/includes/cli-reference.md @@ -1600,6 +1600,27 @@ with the current comment body pre-filled. * `-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 delete {#gs-review-delete} + +``` +gs review delete (rm) ... [flags] +``` + +Delete draft comments + +Deletes one or more local draft comments. + +Use 'gs review list --draft-only' +to find the branch-local draft IDs. + +**Arguments** + +* `draft`: Draft comment IDs to delete. + +**Flags** + +* `-b`, `--branch=BRANCH`: Branch containing the drafts. Defaults to the current branch. + ### git-spice review resolve {#gs-review-resolve} ``` diff --git a/internal/handler/review/delete.go b/internal/handler/review/delete.go new file mode 100644 index 00000000..62bf1699 --- /dev/null +++ b/internal/handler/review/delete.go @@ -0,0 +1,28 @@ +package review + +import ( + "context" + "fmt" +) + +// DeleteDraftsRequest describes local review drafts to delete. +type DeleteDraftsRequest struct { + // Branch identifies the branch containing the drafts. + Branch string // required + + // IDs identify drafts within the branch. + IDs []DraftID // required +} + +// DeleteDrafts removes local review drafts. +func (h *DraftHandler) DeleteDrafts( + ctx context.Context, + req *DeleteDraftsRequest, +) error { + deleted, err := h.Store.DeleteReviewDrafts(ctx, req.Branch, req.IDs) + if err != nil { + return fmt.Errorf("delete draft comments: %w", err) + } + h.Log.Infof("Deleted %d draft comment(s).", deleted) + return nil +} diff --git a/internal/handler/review/handler.go b/internal/handler/review/handler.go index 0094949f..71daa596 100644 --- a/internal/handler/review/handler.go +++ b/internal/handler/review/handler.go @@ -64,6 +64,7 @@ var _ Service = (*spice.Service)(nil) // Store persists branch-local review drafts. type Store interface { AddReviewDraft(context.Context, string, review.Draft) (review.Draft, error) + DeleteReviewDrafts(context.Context, string, []review.DraftID) (int, error) LoadReviewDrafts(context.Context, string) ([]review.Draft, error) RemovePublishedReviewDrafts(context.Context, string, []review.Draft) error UpdateReviewDraftBody(context.Context, string, review.DraftID, string) error diff --git a/internal/handler/review/handler_test.go b/internal/handler/review/handler_test.go index 9fc7d3f8..be572fee 100644 --- a/internal/handler/review/handler_test.go +++ b/internal/handler/review/handler_test.go @@ -226,6 +226,31 @@ func TestDraftHandler_ReplaceDraftBody(t *testing.T) { require.NoError(t, err) } +func TestDraftHandler_DeleteDrafts(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockStore(ctrl) + handler := &DraftHandler{ + Log: silog.Nop(), + Store: store, + Editor: nil, + } + + store. + EXPECT(). + DeleteReviewDrafts( + gomock.Any(), + "feature", + []DraftID{1, 3}, + ). + Return(2, nil) + + err := handler.DeleteDrafts(t.Context(), &DeleteDraftsRequest{ + Branch: "feature", + IDs: []DraftID{1, 3}, + }) + require.NoError(t, err) +} + func TestHandler_LoadReviewData(t *testing.T) { ctrl := gomock.NewController(t) store := NewMockStore(ctrl) diff --git a/internal/handler/review/mocks_test.go b/internal/handler/review/mocks_test.go index 5f9dff06..662ba1af 100644 --- a/internal/handler/review/mocks_test.go +++ b/internal/handler/review/mocks_test.go @@ -208,6 +208,45 @@ func (c *MockStoreAddReviewDraftCall) DoAndReturn(f func(context.Context, string return c } +// DeleteReviewDrafts mocks base method. +func (m *MockStore) DeleteReviewDrafts(arg0 context.Context, arg1 string, arg2 []review.DraftID) (int, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteReviewDrafts", arg0, arg1, arg2) + ret0, _ := ret[0].(int) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// DeleteReviewDrafts indicates an expected call of DeleteReviewDrafts. +func (mr *MockStoreMockRecorder) DeleteReviewDrafts(arg0, arg1, arg2 any) *MockStoreDeleteReviewDraftsCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteReviewDrafts", reflect.TypeOf((*MockStore)(nil).DeleteReviewDrafts), arg0, arg1, arg2) + return &MockStoreDeleteReviewDraftsCall{Call: call} +} + +// MockStoreDeleteReviewDraftsCall wrap *gomock.Call +type MockStoreDeleteReviewDraftsCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockStoreDeleteReviewDraftsCall) Return(arg0 int, arg1 error) *MockStoreDeleteReviewDraftsCall { + c.Call = c.Call.Return(arg0, arg1) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockStoreDeleteReviewDraftsCall) Do(f func(context.Context, string, []review.DraftID) (int, error)) *MockStoreDeleteReviewDraftsCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockStoreDeleteReviewDraftsCall) DoAndReturn(f func(context.Context, string, []review.DraftID) (int, error)) *MockStoreDeleteReviewDraftsCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + // LoadReviewDrafts mocks base method. func (m *MockStore) LoadReviewDrafts(arg0 context.Context, arg1 string) ([]review.Draft, error) { m.ctrl.T.Helper() diff --git a/internal/spice/state/review_draft.go b/internal/spice/state/review_draft.go index baf26110..700c1025 100644 --- a/internal/spice/state/review_draft.go +++ b/internal/spice/state/review_draft.go @@ -61,6 +61,60 @@ func (s *Store) AddReviewDraft( return draft, nil } +// DeleteReviewDrafts atomically removes drafts by branch-local ID. +func (s *Store) DeleteReviewDrafts( + ctx context.Context, + branch string, + ids []review.DraftID, +) (int, error) { + requested := make(map[review.DraftID]struct{}, len(ids)) + for _, id := range ids { + requested[id] = struct{}{} + } + + statements := make([]jsonmut.Statement, 0, len(requested)) + for _, id := range slices.Sorted(maps.Keys(requested)) { + path := jsontext.Pointer("/drafts").AppendToken(id.String()) + statements = append(statements, + jsonmut.Lookup(path).Then(func(value jsontext.Value) jsonmut.Statement { + if len(value) == 0 { + return jsonmut.Fail[struct{}]( + &reviewDraftNotFoundError{ID: id}, + ) + } + return jsonmut.Delete(path) + }), + ) + } + + err := storage.UpdateJSON( + ctx, + s.db, + storage.JSONMutationRequest{ + Key: reviewDraftsJSON(branch), + IfMissing: jsontext.Value(`{}`), + Requires: []string{branchKey(branch)}, + Message: fmt.Sprintf("%v: delete review drafts", branch), + }, + jsonmut.Block(statements...), + ) + if notFound, ok := errors.AsType[*reviewDraftNotFoundError](err); ok { + return 0, notFound + } + if err != nil { + return 0, fmt.Errorf("delete review drafts: %w", err) + } + return len(requested), nil +} + +type reviewDraftNotFoundError struct { + ID review.DraftID +} + +func (e *reviewDraftNotFoundError) Error() string { + return fmt.Sprintf("draft comment %d not found", e.ID) +} + // UpdateReviewDraftBody atomically replaces one draft's body. func (s *Store) UpdateReviewDraftBody( ctx context.Context, diff --git a/internal/spice/state/review_draft_concurrency_test.go b/internal/spice/state/review_draft_concurrency_test.go index 352db850..328b7ecd 100644 --- a/internal/spice/state/review_draft_concurrency_test.go +++ b/internal/spice/state/review_draft_concurrency_test.go @@ -128,6 +128,61 @@ func TestReviewDraftsConcurrentEdit(t *testing.T) { assert.Equal(t, "Second edited", drafts[1].Body) } +func TestReviewDraftsConcurrentDeleteAndAdd(t *testing.T) { + ctx := t.Context() + stores, repo := newConcurrentReviewDraftStores(t) + _, err := stores[0].AddReviewDraft( + ctx, + "feat", + review.Draft{ + ID: 0, + Body: "First", + Anchor: review.Anchor{ + Path: "first.go", + StartLine: 1, + EndLine: 1, + }, + }, + ) + require.NoError(t, err) + + var added review.Draft + repo.pauseNextRefUpdates(2) + errs := runConcurrently( + func() error { + _, err := stores[0].DeleteReviewDrafts( + ctx, + "feat", + []review.DraftID{1}, + ) + return err + }, + func() (err error) { + added, err = stores[1].AddReviewDraft( + ctx, + "feat", + review.Draft{ + ID: 0, + Body: "Second", + Anchor: review.Anchor{ + Path: "second.go", + StartLine: 2, + EndLine: 2, + }, + }, + ) + return err + }, + ) + require.NoError(t, errs[0]) + require.NoError(t, errs[1]) + + drafts, err := stores[0].LoadReviewDrafts(ctx, "feat") + require.NoError(t, err) + require.NotNil(t, drafts) + assert.Equal(t, []review.Draft{added}, drafts) +} + func newConcurrentReviewDraftStores( t *testing.T, ) ([2]*state.Store, *pausingGitRepository) { diff --git a/internal/spice/state/review_draft_delete_test.go b/internal/spice/state/review_draft_delete_test.go new file mode 100644 index 00000000..e55de19c --- /dev/null +++ b/internal/spice/state/review_draft_delete_test.go @@ -0,0 +1,85 @@ +package state_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.abhg.dev/gs/internal/review" + "go.abhg.dev/gs/internal/spice/state" + "go.abhg.dev/gs/internal/spice/state/storage" +) + +func TestReviewDraftsDelete(t *testing.T) { + ctx := t.Context() + db := storage.NewDB(make(storage.MapBackend)) + store, err := state.InitStore(ctx, state.InitStoreRequest{ + DB: db, + Trunk: "main", + }) + require.NoError(t, err) + tx := store.BeginBranchTx() + require.NoError(t, tx.Upsert(ctx, state.UpsertRequest{ + Name: "feat", + Base: "main", + })) + require.NoError(t, tx.Commit(ctx, "track feat")) + + var added []review.Draft + for _, body := range []string{"First", "Second", "Third"} { + draft, err := store.AddReviewDraft( + ctx, + "feat", + review.Draft{ + ID: 0, + Body: body, + Anchor: review.Anchor{ + Path: "main.go", + StartLine: 1, + EndLine: 1, + }, + }, + ) + require.NoError(t, err) + added = append(added, draft) + } + + deleted, err := store.DeleteReviewDrafts( + ctx, + "feat", + []review.DraftID{1, 3, 3}, + ) + require.NoError(t, err) + assert.Equal(t, 2, deleted) + drafts, err := store.LoadReviewDrafts(ctx, "feat") + require.NoError(t, err) + require.NotNil(t, drafts) + assert.Equal(t, []review.Draft{added[1]}, drafts) + + _, err = store.DeleteReviewDrafts( + ctx, + "feat", + []review.DraftID{2, 99}, + ) + assert.EqualError(t, err, "draft comment 99 not found") + drafts, err = store.LoadReviewDrafts(ctx, "feat") + require.NoError(t, err) + assert.Equal(t, []review.Draft{added[1]}, drafts) +} + +func TestReviewDraftsDeleteFromUntrackedBranch(t *testing.T) { + ctx := t.Context() + db := storage.NewDB(make(storage.MapBackend)) + store, err := state.InitStore(ctx, state.InitStoreRequest{ + DB: db, + Trunk: "main", + }) + require.NoError(t, err) + + _, err = store.DeleteReviewDrafts( + ctx, + "feat", + []review.DraftID{1}, + ) + assert.ErrorIs(t, err, state.ErrNotExist) +} diff --git a/review.go b/review.go index 2c7b334a..804b76fb 100644 --- a/review.go +++ b/review.go @@ -20,6 +20,7 @@ type reviewCmd struct { 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"` + Delete reviewDeleteCmd `cmd:"" aliases:"rm" help:"Delete draft comments"` Resolve reviewResolveCmd `cmd:"" help:"Resolve a review thread"` Reopen reviewReopenCmd `cmd:"" help:"Reopen a resolved review thread"` } @@ -97,6 +98,7 @@ type ReviewHandler interface { type ReviewDraftHandler interface { SaveCommentDraft(context.Context, *review.CommentRequest) error SaveReplyDraft(context.Context, *review.ReplyRequest) error + DeleteDrafts(context.Context, *review.DeleteDraftsRequest) error ReplaceDraftBody(context.Context, *review.ReplaceDraftBodyRequest) error } diff --git a/review_delete.go b/review_delete.go new file mode 100644 index 00000000..741c9d1b --- /dev/null +++ b/review_delete.go @@ -0,0 +1,43 @@ +package main + +import ( + "context" + "fmt" + + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/handler/review" + "go.abhg.dev/gs/internal/text" +) + +type reviewDeleteCmd struct { + IDs []review.DraftID `arg:"" name:"draft" help:"Draft comment IDs to delete."` + Branch string `short:"b" placeholder:"BRANCH" predictor:"trackedBranches" help:"Branch containing the drafts. Defaults to the current branch."` +} + +func (*reviewDeleteCmd) Help() string { + return text.Dedent(` + Deletes one or more local draft comments. + + Use 'gs review list --draft-only' + to find the branch-local draft IDs. + `) +} + +func (cmd *reviewDeleteCmd) Run( + ctx context.Context, + wt *git.Worktree, + handler ReviewDraftHandler, +) error { + if cmd.Branch == "" { + branch, err := wt.CurrentBranch(ctx) + if err != nil { + return fmt.Errorf("get current branch: %w", err) + } + cmd.Branch = branch + } + + return handler.DeleteDrafts(ctx, &review.DeleteDraftsRequest{ + Branch: cmd.Branch, + IDs: cmd.IDs, + }) +} diff --git a/testdata/help/gs.txt b/testdata/help/gs.txt index cedd4c2e..f5cb8d1d 100644 --- a/testdata/help/gs.txt +++ b/testdata/help/gs.txt @@ -70,13 +70,14 @@ 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 + 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 delete (rm) Delete draft comments + review resolve Resolve a review thread + review reopen Reopen a resolved review thread Rebase rebase (rb) continue (c) Continue an interrupted operation diff --git a/testdata/help/review_delete.txt b/testdata/help/review_delete.txt new file mode 100644 index 00000000..54316424 --- /dev/null +++ b/testdata/help/review_delete.txt @@ -0,0 +1,21 @@ +Usage: gs review delete (rm) ... [flags] + +Delete draft comments + +Deletes one or more local draft comments. + +Use 'gs review list --draft-only' to find the branch-local draft IDs. + +Arguments: + ... Draft comment IDs to delete. + +Flags: + -b, --branch=BRANCH Branch containing the drafts. 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/script/review_delete.txt b/testdata/script/review_delete.txt new file mode 100644 index 00000000..b1c2546a --- /dev/null +++ b/testdata/script/review_delete.txt @@ -0,0 +1,53 @@ +# Delete local draft comments. + +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 +git add main.go +gs bc -m 'Add main' feature1 + +# draft two comments +gs review comment main.go:3 -m 'First comment.' +stderr 'Drafted comment 1' +gs review comment main.go:4 -m 'Second comment.' +stderr 'Drafted comment 2' + +# a missing ID leaves all drafts untouched +! gs review delete 1 99 +stderr 'draft comment 99 not found' +gs review list --draft-only +stderr 'First comment' +stderr 'Second comment' + +# delete both comments +gs review delete 1 2 +stderr 'Deleted 2 draft comment' +gs review list --draft-only +stderr 'No draft comments' + +# deleted IDs are not reused +gs review comment main.go:3 -m 'Third comment.' +stderr 'Drafted comment 3' + +-- repo/main.go -- +package main + +func main() { + println("hello") +}