diff --git a/.changes/unreleased/Added-20260512-100734.yaml b/.changes/unreleased/Added-20260512-100734.yaml new file mode 100644 index 000000000..0e23c46f3 --- /dev/null +++ b/.changes/unreleased/Added-20260512-100734.yaml @@ -0,0 +1,3 @@ +kind: Added +body: 'submit: Skip submission when a downstack base''s change has been merged (use --force to bypass)' +time: 2026-05-12T10:07:34.7611-04:00 diff --git a/downstack_submit.go b/downstack_submit.go index fc8fc7d61..44c0ca5a3 100644 --- a/downstack_submit.go +++ b/downstack_submit.go @@ -48,10 +48,12 @@ func (cmd *downstackSubmitCmd) Run( return errors.New("nothing to submit below trunk") } - downstacks, err := svc.ListDownstack(ctx, cmd.Branch) + graph, err := svc.BranchGraph(ctx, nil) if err != nil { - return fmt.Errorf("list downstack: %w", err) + return fmt.Errorf("build branch graph: %w", err) } + + downstacks := slices.Collect(graph.Downstack(cmd.Branch)) must.NotBeEmptyf(downstacks, "downstack cannot be empty") slices.Reverse(downstacks) @@ -61,5 +63,6 @@ func (cmd *downstackSubmitCmd) Run( Branches: downstacks, Options: &cmd.Options, BatchOptions: &cmd.BatchOptions, + BranchGraph: graph, }) } diff --git a/internal/handler/submit/handler.go b/internal/handler/submit/handler.go index 402b43861..1c5c088d0 100644 --- a/internal/handler/submit/handler.go +++ b/internal/handler/submit/handler.go @@ -65,9 +65,9 @@ var _ Store = (*state.Store)(nil) // Service provides access to the Spice service. type Service interface { + BranchGraph(context.Context, *spice.BranchGraphOptions) (*spice.BranchGraph, error) LoadBranches(context.Context) ([]spice.LoadBranchItem, error) VerifyRestacked(ctx context.Context, name string) error - LookupBranch(ctx context.Context, name string) (*spice.LookupBranchResponse, error) UnusedBranchName(ctx context.Context, remote string, branch string) (string, error) ListChangeTemplates(context.Context, string, forge.Repository) ([]*forge.ChangeTemplate, error) } @@ -470,6 +470,13 @@ type BatchRequest struct { Branches []string // required Options *Options BatchOptions *BatchOptions // required + + // BranchGraph is an optional graph already built by the command layer. + // + // Batch submit commands often build the graph to decide which branches are + // in scope. Reusing it here avoids loading the same branch state again for + // stale-base validation. + BranchGraph *spice.BranchGraph } // SubmitBatch submits a batch of branches to a remote repository, @@ -485,12 +492,25 @@ func (h *Handler) SubmitBatch(ctx context.Context, req *BatchRequest) error { opts.UpdateOnly = &batchOpts.UpdateOnlyDefault } + graph := req.BranchGraph + if graph == nil { + var err error + graph, err = h.Service.BranchGraph(ctx, nil) + if err != nil { + return fmt.Errorf("build branch graph: %w", err) + } + } + if err := h.checkStaleSubmissionBases(ctx, graph, req.Branches, opts); err != nil { + return err + } + var branchesToComment []string for _, branch := range req.Branches { // Shallow copy the options because submitBranch may modify them. opts := *opts status, err := h.submitBranch( ctx, + graph, branch, &submitOptions{Options: &opts}, ) @@ -528,6 +548,10 @@ type Request struct { // Options are the options for the submit operation. Options *Options // optional + + // BranchGraph is an optional graph already built by the command layer. + // If not provided, it may be loaded as required. + BranchGraph *spice.BranchGraph } // Submit submits a single branch to a remote repository, @@ -535,8 +559,21 @@ type Request struct { func (h *Handler) Submit(ctx context.Context, req *Request) error { opts := cmp.Or(req.Options, &Options{}) mergeConfiguredOptions(opts) + graph := req.BranchGraph + if graph == nil { + var err error + graph, err = h.Service.BranchGraph(ctx, nil) + if err != nil { + return fmt.Errorf("build branch graph: %w", err) + } + } + if err := h.checkStaleSubmissionBases(ctx, graph, []string{req.Branch}, opts); err != nil { + return err + } + status, err := h.submitBranch( ctx, + graph, req.Branch, &submitOptions{ Options: opts, @@ -582,6 +619,7 @@ type submitOptions struct { func (h *Handler) submitBranch( ctx context.Context, + graph *spice.BranchGraph, branchToSubmit string, opts *submitOptions, ) (status submitStatus, err error) { @@ -592,9 +630,9 @@ func (h *Handler) submitBranch( svc := h.Service log := h.Log - branch, err := svc.LookupBranch(ctx, branchToSubmit) - if err != nil { - return status, fmt.Errorf("lookup branch: %w", err) + branch, ok := graph.Lookup(branchToSubmit) + if !ok { + return status, fmt.Errorf("lookup branch: %w", state.ErrNotExist) } // Various code paths down below should call this @@ -634,9 +672,9 @@ func (h *Handler) submitBranch( // use that name instead. upstreamBase := branch.Base if branch.Base != h.Store.Trunk() { - baseBranch, err := svc.LookupBranch(ctx, branch.Base) - if err != nil { - return status, fmt.Errorf("lookup base branch: %w", err) + baseBranch, ok := graph.Lookup(branch.Base) + if !ok { + return status, fmt.Errorf("lookup base branch: %w", state.ErrNotExist) } upstreamBase = cmp.Or(baseBranch.UpstreamBranch, branch.Base) } diff --git a/internal/handler/submit/handler_test.go b/internal/handler/submit/handler_test.go index a935c1c58..2ea5c51fb 100644 --- a/internal/handler/submit/handler_test.go +++ b/internal/handler/submit/handler_test.go @@ -14,6 +14,7 @@ import ( "go.abhg.dev/gs/internal/forge/forgetest" "go.abhg.dev/gs/internal/silog" "go.abhg.dev/gs/internal/silog/silogtest" + "go.abhg.dev/gs/internal/spice" "go.abhg.dev/gs/internal/spice/state" "go.abhg.dev/gs/internal/ui" gomock "go.uber.org/mock/gomock" @@ -114,6 +115,93 @@ func TestHandler_pushRepositoryID_rejectsDifferentForge(t *testing.T) { assert.Contains(t, err.Error(), "different forge") } +func TestHandler_SubmitBatch_rejectsStaleBaseBeforeSubmitting(t *testing.T) { + mockCtrl := gomock.NewController(t) + + mockStore := NewMockStore(mockCtrl) + mockStore.EXPECT(). + Trunk(). + Return("main"). + AnyTimes() + + mockService := NewMockService(mockCtrl) + mockService.EXPECT(). + BranchGraph(gomock.Any(), gomock.Any()). + Return(buildStaleBaseTestGraph(t, "main", []spice.LoadBranchItem{ + {Name: "feat1", Base: "main", Change: submitFakeChange("pr-1")}, + {Name: "feat2", Base: "feat1", Change: submitFakeChange("pr-2")}, + }), nil) + + mockRemoteRepo := forgetest.NewMockRepository(mockCtrl) + mockRemoteRepo.EXPECT(). + ChangeStatuses(gomock.Any(), []forge.ChangeID{ + submitFakeChangeID("pr-1"), + }). + Return([]forge.ChangeStatus{ + {State: forge.ChangeMerged}, + }, nil) + + handler := &Handler{ + Log: silog.Nop(), + View: ui.NewFileView(&bytes.Buffer{}), + Repository: nil, + Worktree: nil, + Store: mockStore, + Service: mockService, + Browser: &browser.Noop{}, + FindRemote: func(context.Context) (state.Remote, error) { + return state.Remote{Upstream: "origin", Push: "origin"}, nil + }, + OpenRepository: func( + context.Context, + forge.Forge, + forge.RepositoryID, + ) (forge.Repository, error) { + return mockRemoteRepo, nil + }, + ResolveRepository: func( + context.Context, + string, + ) (forge.Forge, forge.RepositoryID, error) { + return forgetest.NewMockForge(mockCtrl), + stubRepositoryID("alice/repo"), nil + }, + } + + err := handler.SubmitBatch(t.Context(), &BatchRequest{ + Branches: []string{"feat2"}, + Options: &Options{}, + BatchOptions: &BatchOptions{}, + }) + + require.Error(t, err) + assert.ErrorContains(t, err, "1 branches with stale bases were found") + assert.ErrorContains(t, err, "gs repo sync") + assert.ErrorContains(t, err, "--force") +} + +func TestHandler_checkStaleSubmissionBases_forceSkipsValidation(t *testing.T) { + mockCtrl := gomock.NewController(t) + + handler := &Handler{ + Log: silog.Nop(), + View: ui.NewFileView(&bytes.Buffer{}), + Repository: nil, + Worktree: nil, + Store: NewMockStore(mockCtrl), + Service: NewMockService(mockCtrl), + Browser: &browser.Noop{}, + FindRemote: nil, + ResolveRepository: nil, + OpenRepository: nil, + } + + err := handler.checkStaleSubmissionBases( + t.Context(), nil, []string{"feat2"}, &Options{Force: true}, + ) + require.NoError(t, err) +} + func TestReviewersAddWhen_UnmarshalText(t *testing.T) { tests := []struct { name string @@ -252,16 +340,6 @@ func TestEffectiveReviewers(t *testing.T) { } } -type stubRepositoryID string - -func (id stubRepositoryID) String() string { - return string(id) -} - -func (id stubRepositoryID) ChangeURL(forge.ChangeID) string { - return string(id) -} - func TestLabelsAddWhen_UnmarshalText(t *testing.T) { tests := []struct { name string @@ -399,3 +477,40 @@ func TestEffectiveLabels(t *testing.T) { }) } } + +type stubRepositoryID string + +func (id stubRepositoryID) String() string { + return string(id) +} + +func (id stubRepositoryID) ChangeURL(forge.ChangeID) string { + return string(id) +} + +// submitFakeChangeMetadata is the minimal change metadata needed by submit +// handler tests that reason about stored branch state. +type submitFakeChangeMetadata struct { + id forge.ChangeID +} + +var _ forge.ChangeMetadata = (*submitFakeChangeMetadata)(nil) + +func (m *submitFakeChangeMetadata) ForgeID() string { return "test" } +func (m *submitFakeChangeMetadata) ChangeID() forge.ChangeID { + return m.id +} + +func (m *submitFakeChangeMetadata) NavigationCommentID() forge.ChangeCommentID { + return nil +} +func (m *submitFakeChangeMetadata) SetNavigationCommentID(forge.ChangeCommentID) {} + +// submitFakeChangeID identifies a fake change in submit handler tests. +type submitFakeChangeID string + +func (id submitFakeChangeID) String() string { return string(id) } + +func submitFakeChange(id string) forge.ChangeMetadata { + return &submitFakeChangeMetadata{id: submitFakeChangeID(id)} +} diff --git a/internal/handler/submit/mocks_test.go b/internal/handler/submit/mocks_test.go index 46add81a1..058e1bfcc 100644 --- a/internal/handler/submit/mocks_test.go +++ b/internal/handler/submit/mocks_test.go @@ -42,119 +42,119 @@ func (m *MockService) EXPECT() *MockServiceMockRecorder { return m.recorder } -// ListChangeTemplates mocks base method. -func (m *MockService) ListChangeTemplates(arg0 context.Context, arg1 string, arg2 forge.Repository) ([]*forge.ChangeTemplate, error) { +// BranchGraph mocks base method. +func (m *MockService) BranchGraph(arg0 context.Context, arg1 *spice.BranchGraphOptions) (*spice.BranchGraph, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ListChangeTemplates", arg0, arg1, arg2) - ret0, _ := ret[0].([]*forge.ChangeTemplate) + ret := m.ctrl.Call(m, "BranchGraph", arg0, arg1) + ret0, _ := ret[0].(*spice.BranchGraph) ret1, _ := ret[1].(error) return ret0, ret1 } -// ListChangeTemplates indicates an expected call of ListChangeTemplates. -func (mr *MockServiceMockRecorder) ListChangeTemplates(arg0, arg1, arg2 any) *MockServiceListChangeTemplatesCall { +// BranchGraph indicates an expected call of BranchGraph. +func (mr *MockServiceMockRecorder) BranchGraph(arg0, arg1 any) *MockServiceBranchGraphCall { mr.mock.ctrl.T.Helper() - call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListChangeTemplates", reflect.TypeOf((*MockService)(nil).ListChangeTemplates), arg0, arg1, arg2) - return &MockServiceListChangeTemplatesCall{Call: call} + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BranchGraph", reflect.TypeOf((*MockService)(nil).BranchGraph), arg0, arg1) + return &MockServiceBranchGraphCall{Call: call} } -// MockServiceListChangeTemplatesCall wrap *gomock.Call -type MockServiceListChangeTemplatesCall struct { +// MockServiceBranchGraphCall wrap *gomock.Call +type MockServiceBranchGraphCall struct { *gomock.Call } // Return rewrite *gomock.Call.Return -func (c *MockServiceListChangeTemplatesCall) Return(arg0 []*forge.ChangeTemplate, arg1 error) *MockServiceListChangeTemplatesCall { +func (c *MockServiceBranchGraphCall) Return(arg0 *spice.BranchGraph, arg1 error) *MockServiceBranchGraphCall { c.Call = c.Call.Return(arg0, arg1) return c } // Do rewrite *gomock.Call.Do -func (c *MockServiceListChangeTemplatesCall) Do(f func(context.Context, string, forge.Repository) ([]*forge.ChangeTemplate, error)) *MockServiceListChangeTemplatesCall { +func (c *MockServiceBranchGraphCall) Do(f func(context.Context, *spice.BranchGraphOptions) (*spice.BranchGraph, error)) *MockServiceBranchGraphCall { c.Call = c.Call.Do(f) return c } // DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockServiceListChangeTemplatesCall) DoAndReturn(f func(context.Context, string, forge.Repository) ([]*forge.ChangeTemplate, error)) *MockServiceListChangeTemplatesCall { +func (c *MockServiceBranchGraphCall) DoAndReturn(f func(context.Context, *spice.BranchGraphOptions) (*spice.BranchGraph, error)) *MockServiceBranchGraphCall { c.Call = c.Call.DoAndReturn(f) return c } -// LoadBranches mocks base method. -func (m *MockService) LoadBranches(arg0 context.Context) ([]spice.LoadBranchItem, error) { +// ListChangeTemplates mocks base method. +func (m *MockService) ListChangeTemplates(arg0 context.Context, arg1 string, arg2 forge.Repository) ([]*forge.ChangeTemplate, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "LoadBranches", arg0) - ret0, _ := ret[0].([]spice.LoadBranchItem) + ret := m.ctrl.Call(m, "ListChangeTemplates", arg0, arg1, arg2) + ret0, _ := ret[0].([]*forge.ChangeTemplate) ret1, _ := ret[1].(error) return ret0, ret1 } -// LoadBranches indicates an expected call of LoadBranches. -func (mr *MockServiceMockRecorder) LoadBranches(arg0 any) *MockServiceLoadBranchesCall { +// ListChangeTemplates indicates an expected call of ListChangeTemplates. +func (mr *MockServiceMockRecorder) ListChangeTemplates(arg0, arg1, arg2 any) *MockServiceListChangeTemplatesCall { mr.mock.ctrl.T.Helper() - call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LoadBranches", reflect.TypeOf((*MockService)(nil).LoadBranches), arg0) - return &MockServiceLoadBranchesCall{Call: call} + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListChangeTemplates", reflect.TypeOf((*MockService)(nil).ListChangeTemplates), arg0, arg1, arg2) + return &MockServiceListChangeTemplatesCall{Call: call} } -// MockServiceLoadBranchesCall wrap *gomock.Call -type MockServiceLoadBranchesCall struct { +// MockServiceListChangeTemplatesCall wrap *gomock.Call +type MockServiceListChangeTemplatesCall struct { *gomock.Call } // Return rewrite *gomock.Call.Return -func (c *MockServiceLoadBranchesCall) Return(arg0 []spice.LoadBranchItem, arg1 error) *MockServiceLoadBranchesCall { +func (c *MockServiceListChangeTemplatesCall) Return(arg0 []*forge.ChangeTemplate, arg1 error) *MockServiceListChangeTemplatesCall { c.Call = c.Call.Return(arg0, arg1) return c } // Do rewrite *gomock.Call.Do -func (c *MockServiceLoadBranchesCall) Do(f func(context.Context) ([]spice.LoadBranchItem, error)) *MockServiceLoadBranchesCall { +func (c *MockServiceListChangeTemplatesCall) Do(f func(context.Context, string, forge.Repository) ([]*forge.ChangeTemplate, error)) *MockServiceListChangeTemplatesCall { c.Call = c.Call.Do(f) return c } // DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockServiceLoadBranchesCall) DoAndReturn(f func(context.Context) ([]spice.LoadBranchItem, error)) *MockServiceLoadBranchesCall { +func (c *MockServiceListChangeTemplatesCall) DoAndReturn(f func(context.Context, string, forge.Repository) ([]*forge.ChangeTemplate, error)) *MockServiceListChangeTemplatesCall { c.Call = c.Call.DoAndReturn(f) return c } -// LookupBranch mocks base method. -func (m *MockService) LookupBranch(ctx context.Context, name string) (*spice.LookupBranchResponse, error) { +// LoadBranches mocks base method. +func (m *MockService) LoadBranches(arg0 context.Context) ([]spice.LoadBranchItem, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "LookupBranch", ctx, name) - ret0, _ := ret[0].(*spice.LookupBranchResponse) + ret := m.ctrl.Call(m, "LoadBranches", arg0) + ret0, _ := ret[0].([]spice.LoadBranchItem) ret1, _ := ret[1].(error) return ret0, ret1 } -// LookupBranch indicates an expected call of LookupBranch. -func (mr *MockServiceMockRecorder) LookupBranch(ctx, name any) *MockServiceLookupBranchCall { +// LoadBranches indicates an expected call of LoadBranches. +func (mr *MockServiceMockRecorder) LoadBranches(arg0 any) *MockServiceLoadBranchesCall { mr.mock.ctrl.T.Helper() - call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LookupBranch", reflect.TypeOf((*MockService)(nil).LookupBranch), ctx, name) - return &MockServiceLookupBranchCall{Call: call} + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LoadBranches", reflect.TypeOf((*MockService)(nil).LoadBranches), arg0) + return &MockServiceLoadBranchesCall{Call: call} } -// MockServiceLookupBranchCall wrap *gomock.Call -type MockServiceLookupBranchCall struct { +// MockServiceLoadBranchesCall wrap *gomock.Call +type MockServiceLoadBranchesCall struct { *gomock.Call } // Return rewrite *gomock.Call.Return -func (c *MockServiceLookupBranchCall) Return(arg0 *spice.LookupBranchResponse, arg1 error) *MockServiceLookupBranchCall { +func (c *MockServiceLoadBranchesCall) Return(arg0 []spice.LoadBranchItem, arg1 error) *MockServiceLoadBranchesCall { c.Call = c.Call.Return(arg0, arg1) return c } // Do rewrite *gomock.Call.Do -func (c *MockServiceLookupBranchCall) Do(f func(context.Context, string) (*spice.LookupBranchResponse, error)) *MockServiceLookupBranchCall { +func (c *MockServiceLoadBranchesCall) Do(f func(context.Context) ([]spice.LoadBranchItem, error)) *MockServiceLoadBranchesCall { c.Call = c.Call.Do(f) return c } // DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockServiceLookupBranchCall) DoAndReturn(f func(context.Context, string) (*spice.LookupBranchResponse, error)) *MockServiceLookupBranchCall { +func (c *MockServiceLoadBranchesCall) DoAndReturn(f func(context.Context) ([]spice.LoadBranchItem, error)) *MockServiceLoadBranchesCall { c.Call = c.Call.DoAndReturn(f) return c } diff --git a/internal/handler/submit/stale_base.go b/internal/handler/submit/stale_base.go new file mode 100644 index 000000000..7cd796a88 --- /dev/null +++ b/internal/handler/submit/stale_base.go @@ -0,0 +1,163 @@ +package submit + +import ( + "context" + "fmt" + + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/must" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/spice" +) + +// checkStaleSubmissionBases prevents submit from acting on a stack whose local +// base relationships are already obsolete on the forge. +// +// The submit path may push branches or edit CR bases before per-branch submit +// logic discovers that a downstack base was merged externally. This preflight +// checks every submitted branch's downstack first so the user can run +// 'gs repo sync' before any remote state is changed. +func (h *Handler) checkStaleSubmissionBases( + ctx context.Context, + graph *spice.BranchGraph, + branches []string, + opts *Options, +) error { + if opts.Force { + return nil + } + + candidates := staleBaseCandidates(graph, branches) + if len(candidates) == 0 { + return nil + } + + remoteRepo, err := h.upstreamRepository(ctx) + if err != nil { + return fmt.Errorf("open remote repository: %w", err) + } + + count, err := validateStaleBaseCandidates( + ctx, remoteRepo, h.Log, candidates, + ) + if err != nil { + return err + } + if count > 0 { + return fmt.Errorf( + "%d branches with stale bases were found; "+ + "run 'gs repo sync' first, "+ + "or use --force to submit anyway", + count, + ) + } + return nil +} + +// staleBaseCandidate identifies one local branch/base edge +// whose base CR may already be merged on the forge. +type staleBaseCandidate struct { + // Branch is the Branch whose base may be stale. + Branch string + + // Base is the downstack branch used as branch's Base. + Base string + + // ChangeID identifies base's published change on the forge. + ChangeID forge.ChangeID +} + +// staleBaseCandidates finds local branch/base edges +// that need forge status checks before submit. +// +// It walks each submitted branch's downstack, +// keeps only edges whose base branch has published change metadata, +// and de-duplicates shared downstacks by local base branch name. +func staleBaseCandidates( + graph *spice.BranchGraph, + branches []string, +) []staleBaseCandidate { + // Downstack yields the branch first, + // followed by each non-trunk base branch. + // Adjacent names therefore describe the local branch/base edges. + var candidates []staleBaseCandidate + for _, branch := range branches { + var child string + for base := range graph.Downstack(branch) { + if child != "" { + candidates = append(candidates, staleBaseCandidate{ + Branch: child, + Base: base, + // changeID filled below. + }) + } + child = base + } + } + + // Only base branches with published changes need forge checks. + // Multiple submitted branches may share a downstack, + // so de-duplicate by the local base branch name. + seen := make(map[string]struct{}) + verified := candidates[:0] + for _, candidate := range candidates { + baseItem, ok := graph.Lookup(candidate.Base) + if !ok || baseItem.Change == nil { + continue + } + + changeID := baseItem.Change.ChangeID() + if _, ok := seen[candidate.Base]; ok { + continue + } + seen[candidate.Base] = struct{}{} + + candidate.ChangeID = changeID + verified = append(verified, candidate) + } + return verified +} + +// validateStaleBaseCandidates checks candidate base CRs on the forge, +// logs a warning for each merged base it finds, +// and returns the number of stale local branch/base edges. +// +// The returned count is separated from transport or forge query errors +// so callers can report one aggregate submit-blocking error +// after all stale edges have been logged. +func validateStaleBaseCandidates( + ctx context.Context, + forgeRepo forge.Repository, + log *silog.Logger, + candidates []staleBaseCandidate, +) (int, error) { + if len(candidates) == 0 { + return 0, nil + } + + ids := make([]forge.ChangeID, len(candidates)) + for i, c := range candidates { + ids[i] = c.ChangeID + } + + statuses, err := forgeRepo.ChangeStatuses(ctx, ids) + if err != nil { + return 0, fmt.Errorf("query change states: %w", err) + } + must.BeEqualf(len(statuses), len(candidates), + "query change states: got %d statuses for %d changes", + len(statuses), len(candidates), + ) + + var count int + for i, c := range candidates { + if statuses[i].State == forge.ChangeMerged { + log.Warn("Branch has stale base", + "branch", c.Branch, + "base", c.Base, + ) + count++ + } + } + return count, nil +} diff --git a/internal/handler/submit/stale_base_test.go b/internal/handler/submit/stale_base_test.go new file mode 100644 index 000000000..ce468f633 --- /dev/null +++ b/internal/handler/submit/stale_base_test.go @@ -0,0 +1,175 @@ +package submit + +import ( + "bytes" + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/forge/forgetest" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/spice" +) + +func TestValidateStaleBaseCandidates_healthy(t *testing.T) { + ctrl := gomock.NewController(t) + + graph := buildStaleBaseTestGraph(t, "main", []spice.LoadBranchItem{ + {Name: "feat1", Base: "main", Change: submitFakeChange("pr-1")}, + {Name: "feat2", Base: "feat1", Change: submitFakeChange("pr-2")}, + }) + + mockRepo := forgetest.NewMockRepository(ctrl) + mockRepo.EXPECT(). + ChangeStatuses(gomock.Any(), []forge.ChangeID{ + submitFakeChangeID("pr-1"), + }). + Return([]forge.ChangeStatus{{State: forge.ChangeOpen}}, nil) + + count, err := validateStaleBaseCandidates( + t.Context(), + mockRepo, + silog.Nop(), + staleBaseCandidates(graph, []string{"feat2"}), + ) + require.NoError(t, err) + assert.Zero(t, count) +} + +func TestValidateStaleBaseCandidates_staleBase(t *testing.T) { + ctrl := gomock.NewController(t) + + graph := buildStaleBaseTestGraph(t, "main", []spice.LoadBranchItem{ + {Name: "feat1", Base: "main", Change: submitFakeChange("pr-1")}, + {Name: "feat2", Base: "feat1", Change: submitFakeChange("pr-2")}, + }) + + mockRepo := forgetest.NewMockRepository(ctrl) + mockRepo.EXPECT(). + ChangeStatuses(gomock.Any(), []forge.ChangeID{ + submitFakeChangeID("pr-1"), + }). + Return([]forge.ChangeStatus{{State: forge.ChangeMerged}}, nil) + + var logBuffer bytes.Buffer + count, err := validateStaleBaseCandidates( + t.Context(), + mockRepo, + silog.New(&logBuffer, nil), + staleBaseCandidates(graph, []string{"feat2"}), + ) + + require.NoError(t, err) + assert.Equal(t, 1, count) + assert.Contains(t, logBuffer.String(), "Branch has stale base") + assert.Contains(t, logBuffer.String(), "branch=feat2") + assert.Contains(t, logBuffer.String(), "base=feat1") +} + +func TestStaleBaseCandidates_baseWithoutChange(t *testing.T) { + graph := buildStaleBaseTestGraph(t, "main", []spice.LoadBranchItem{ + {Name: "feat1", Base: "main"}, + {Name: "feat2", Base: "feat1", Change: submitFakeChange("pr-2")}, + }) + + assert.Empty(t, staleBaseCandidates(graph, []string{"feat2"})) +} + +func TestStaleBaseCandidates_singleBranch(t *testing.T) { + graph := buildStaleBaseTestGraph(t, "main", []spice.LoadBranchItem{ + {Name: "feat1", Base: "main", Change: submitFakeChange("pr-1")}, + }) + + assert.Empty(t, staleBaseCandidates(graph, []string{"feat1"})) +} + +func TestValidateStaleBaseCandidates_deepStack(t *testing.T) { + ctrl := gomock.NewController(t) + + // trunk <- A <- B <- C; A is merged. + graph := buildStaleBaseTestGraph(t, "main", []spice.LoadBranchItem{ + {Name: "A", Base: "main", Change: submitFakeChange("pr-A")}, + {Name: "B", Base: "A", Change: submitFakeChange("pr-B")}, + {Name: "C", Base: "B", Change: submitFakeChange("pr-C")}, + }) + + // Both A and B are checked because they are bases of B and C. + mockRepo := forgetest.NewMockRepository(ctrl) + mockRepo.EXPECT(). + ChangeStatuses(gomock.Any(), gomock.Any()). + DoAndReturn(func( + _ context.Context, ids []forge.ChangeID, + ) ([]forge.ChangeStatus, error) { + statuses := make([]forge.ChangeStatus, len(ids)) + for i, id := range ids { + if id.String() == "pr-A" { + statuses[i].State = forge.ChangeMerged + } else { + statuses[i].State = forge.ChangeOpen + } + } + return statuses, nil + }) + + count, err := validateStaleBaseCandidates( + t.Context(), + mockRepo, + silog.Nop(), + staleBaseCandidates(graph, []string{"C"}), + ) + + require.NoError(t, err) + assert.Equal(t, 1, count) +} + +func TestStaleBaseCandidates_deduplicatesBaseStatuses(t *testing.T) { + graph := buildStaleBaseTestGraph(t, "main", []spice.LoadBranchItem{ + {Name: "feat1", Base: "main", Change: submitFakeChange("pr-1")}, + {Name: "feat2", Base: "feat1", Change: submitFakeChange("pr-2")}, + {Name: "feat3", Base: "feat2", Change: submitFakeChange("pr-3")}, + }) + + candidates := staleBaseCandidates(graph, []string{"feat3", "feat2"}) + + require.Len(t, candidates, 2) + assert.Equal(t, submitFakeChangeID("pr-2"), candidates[0].ChangeID) + assert.Equal(t, submitFakeChangeID("pr-1"), candidates[1].ChangeID) +} + +func buildStaleBaseTestGraph( + t *testing.T, + trunk string, + branches []spice.LoadBranchItem, +) *spice.BranchGraph { + t.Helper() + graph, err := spice.NewBranchGraph(t.Context(), &staleBaseBranchLoader{ + trunk: trunk, + branches: branches, + }, nil) + require.NoError(t, err) + return graph +} + +type staleBaseBranchLoader struct { + trunk string + branches []spice.LoadBranchItem +} + +func (l *staleBaseBranchLoader) Trunk() string { return l.trunk } + +func (l *staleBaseBranchLoader) LoadBranches( + context.Context, +) ([]spice.LoadBranchItem, error) { + return l.branches, nil +} + +func (*staleBaseBranchLoader) LookupWorktrees( + context.Context, + []string, +) (map[string]string, error) { + return nil, nil +} diff --git a/stack_submit.go b/stack_submit.go index 9f857eeae..ad2c2524a 100644 --- a/stack_submit.go +++ b/stack_submit.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "slices" "go.abhg.dev/gs/internal/git" "go.abhg.dev/gs/internal/handler/submit" @@ -35,9 +36,14 @@ func (cmd *stackSubmitCmd) Run( return fmt.Errorf("get current branch: %w", err) } - stack, err := svc.ListStack(ctx, currentBranch) + graph, err := svc.BranchGraph(ctx, nil) if err != nil { - return fmt.Errorf("list stack: %w", err) + return fmt.Errorf("build branch graph: %w", err) + } + + stack := slices.Collect(graph.Stack(currentBranch)) + if len(stack) == 0 { + stack = []string{currentBranch} } toSubmit := stack[:0] for _, branch := range stack { @@ -53,5 +59,6 @@ func (cmd *stackSubmitCmd) Run( Branches: toSubmit, Options: &cmd.Options, BatchOptions: &cmd.BatchOptions, + BranchGraph: graph, }) } diff --git a/testdata/script/branch_submit_stale_base.txt b/testdata/script/branch_submit_stale_base.txt new file mode 100644 index 000000000..1cb53db24 --- /dev/null +++ b/testdata/script/branch_submit_stale_base.txt @@ -0,0 +1,48 @@ +# 'branch submit' blocks when a downstack base has already been merged. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' + +# set up a fake GitHub remote +shamhub init +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a stack: main -> feature1 -> feature2 +git add feature1.txt +gs bc feature1 -m 'Add feature 1' +git add feature2.txt +gs bc feature2 -m 'Add feature 2' + +# submit the stack +gs downstack submit --fill +stderr 'Created #1' +stderr 'Created #2' + +# merge feature1 externally +shamhub merge alice/example 1 + +# branch submit should block: feature2's base is stale +! gs bs --fill +stderr 'base=feature1' +stderr '1 branches with stale bases were found' +stderr 'gs repo sync' +stderr '--force' + +# --force bypasses the stale base check +gs bs --fill --force +stderr '#2 is up-to-date' + +-- repo/feature1.txt -- +This is feature 1 +-- repo/feature2.txt -- +This is feature 2 diff --git a/testdata/script/downstack_submit_stale_base.txt b/testdata/script/downstack_submit_stale_base.txt new file mode 100644 index 000000000..2934953d5 --- /dev/null +++ b/testdata/script/downstack_submit_stale_base.txt @@ -0,0 +1,48 @@ +# 'downstack submit' blocks when a downstack base has already been merged. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' + +# set up a fake GitHub remote +shamhub init +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a stack: main -> feature1 -> feature2 +git add feature1.txt +gs bc feature1 -m 'Add feature 1' +git add feature2.txt +gs bc feature2 -m 'Add feature 2' + +# submit the stack +gs downstack submit --fill +stderr 'Created #1' +stderr 'Created #2' + +# merge feature1 externally +shamhub merge alice/example 1 + +# downstack submit should block: feature2's base is stale +! gs dss --fill +stderr 'base=feature1' +stderr '1 branches with stale bases were found' +stderr 'gs repo sync' +stderr '--force' + +# --force bypasses the stale base check +gs dss --fill --force +stderr '#2 is up-to-date' + +-- repo/feature1.txt -- +This is feature 1 +-- repo/feature2.txt -- +This is feature 2 diff --git a/testdata/script/stack_submit_stale_base.txt b/testdata/script/stack_submit_stale_base.txt new file mode 100644 index 000000000..3839cc83e --- /dev/null +++ b/testdata/script/stack_submit_stale_base.txt @@ -0,0 +1,48 @@ +# 'stack submit' blocks when a downstack base has already been merged. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' + +# set up a fake GitHub remote +shamhub init +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a stack: main -> feature1 -> feature2 +git add feature1.txt +gs bc feature1 -m 'Add feature 1' +git add feature2.txt +gs bc feature2 -m 'Add feature 2' + +# submit the stack +gs downstack submit --fill +stderr 'Created #1' +stderr 'Created #2' + +# merge feature1 externally +shamhub merge alice/example 1 + +# stack submit should block: feature2's base is stale +! gs stack submit --fill +stderr 'base=feature1' +stderr '1 branches with stale bases were found' +stderr 'gs repo sync' +stderr '--force' + +# --force bypasses the stale base check +gs stack submit --fill --force +stderr '#2 is up-to-date' + +-- repo/feature1.txt -- +This is feature 1 +-- repo/feature2.txt -- +This is feature 2 diff --git a/testdata/script/stale_base_closed_not_stale.txt b/testdata/script/stale_base_closed_not_stale.txt new file mode 100644 index 000000000..c268e3fc4 --- /dev/null +++ b/testdata/script/stale_base_closed_not_stale.txt @@ -0,0 +1,44 @@ +# A closed (rejected) base does not trigger stale base validation. +# Only merged bases are considered stale. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' + +# set up a fake GitHub remote +shamhub init +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a stack: main -> feature1 -> feature2 +git add feature1.txt +gs bc feature1 -m 'Add feature 1' +git add feature2.txt +gs bc feature2 -m 'Add feature 2' + +# submit the stack +gs downstack submit --fill +stderr 'Created #1' +stderr 'Created #2' + +# close (reject) feature1 without merging +shamhub reject alice/example 1 + +# stale base validation should NOT block: +# closed is not the same as merged +gs ss --fill +! stderr 'stale base' +stderr '#2 is up-to-date' + +-- repo/feature1.txt -- +This is feature 1 +-- repo/feature2.txt -- +This is feature 2 diff --git a/testdata/script/stale_base_deep_stack.txt b/testdata/script/stale_base_deep_stack.txt new file mode 100644 index 000000000..9a991340e --- /dev/null +++ b/testdata/script/stale_base_deep_stack.txt @@ -0,0 +1,54 @@ +# Stale base is detected in the middle of a deep stack. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' + +# set up a fake GitHub remote +shamhub init +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a deep stack: main -> feat1 -> feat2 -> feat3 +git add feature1.txt +gs bc feature1 -m 'Add feature 1' +git add feature2.txt +gs bc feature2 -m 'Add feature 2' +git add feature3.txt +gs bc feature3 -m 'Add feature 3' + +# submit the full stack +gs downstack submit --fill +stderr 'Created #1' +stderr 'Created #2' +stderr 'Created #3' + +# merge feature1 externally (middle of the stack) +shamhub merge alice/example 1 + +# from feat3, stack submit should detect feat1 as stale +# even though feat3's immediate base is feat2 +! gs ss --fill +stderr 'base=feature1' +stderr '1 branches with stale bases were found' +stderr 'gs repo sync' +stderr '--force' + +# --force bypasses the stale base check for submit +gs ss --fill --force +stderr '#3 is up-to-date' + +-- repo/feature1.txt -- +This is feature 1 +-- repo/feature2.txt -- +This is feature 2 +-- repo/feature3.txt -- +This is feature 3 diff --git a/testdata/script/stale_base_repo_sync_recovery.txt b/testdata/script/stale_base_repo_sync_recovery.txt new file mode 100644 index 000000000..2c72c0999 --- /dev/null +++ b/testdata/script/stale_base_repo_sync_recovery.txt @@ -0,0 +1,53 @@ +# Stale base is detected, then fixed by 'repo sync'. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' + +# set up a fake GitHub remote +shamhub init +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a stack: main -> feature1 -> feature2 +git add feature1.txt +gs bc feature1 -m 'Add feature 1' +git add feature2.txt +gs bc feature2 -m 'Add feature 2' + +# submit the stack +gs downstack submit --fill +stderr 'Created #1' +stderr 'Created #2' + +# merge feature1 externally +shamhub merge alice/example 1 + +# stack submit blocks: feature2's base is stale +! gs ss --fill +stderr 'base=feature1' +stderr '1 branches with stale bases were found' +stderr 'gs repo sync' + +# recover by syncing +gs repo sync +stderr 'feature1: #1 was merged' + +# feature2 is now restacked onto main. +# switch to feature2 and submit — should succeed +gs bco feature2 +gs ss --fill +stderr 'Updated #2' + +-- repo/feature1.txt -- +This is feature 1 +-- repo/feature2.txt -- +This is feature 2 diff --git a/testdata/script/upstack_submit_stale_base.txt b/testdata/script/upstack_submit_stale_base.txt new file mode 100644 index 000000000..f9d9e1356 --- /dev/null +++ b/testdata/script/upstack_submit_stale_base.txt @@ -0,0 +1,49 @@ +# 'upstack submit' blocks when a downstack base has already been merged. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' + +# set up a fake GitHub remote +shamhub init +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create a stack: main -> feature1 -> feature2 +git add feature1.txt +gs bc feature1 -m 'Add feature 1' +git add feature2.txt +gs bc feature2 -m 'Add feature 2' + +# submit the stack +gs downstack submit --fill +stderr 'Created #1' +stderr 'Created #2' + +# merge feature1 externally +shamhub merge alice/example 1 + +# upstack submit from feature2 should block: +# feature2's downstack includes feature1 (stale). +! gs upstack submit --fill +stderr 'base=feature1' +stderr '1 branches with stale bases were found' +stderr 'gs repo sync' +stderr '--force' + +# --force bypasses the stale base check +gs upstack submit --fill --force +stderr '#2 is up-to-date' + +-- repo/feature1.txt -- +This is feature 1 +-- repo/feature2.txt -- +This is feature 2 diff --git a/upstack_submit.go b/upstack_submit.go index 1c93d8865..872c68e85 100644 --- a/upstack_submit.go +++ b/upstack_submit.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" "go.abhg.dev/gs/internal/git" "go.abhg.dev/gs/internal/handler/submit" @@ -46,29 +47,18 @@ func (cmd *upstackSubmitCmd) Run( cmd.Branch = currentBranch } - if cmd.Branch != store.Trunk() { - b, err := svc.LookupBranch(ctx, cmd.Branch) - if err != nil { - return fmt.Errorf("lookup branch %v: %w", cmd.Branch, err) - } - - if b.Base != store.Trunk() { - base, err := svc.LookupBranch(ctx, b.Base) - if err != nil { - return fmt.Errorf("lookup base %v: %w", b.Base, err) - } + graph, err := svc.BranchGraph(ctx, nil) + if err != nil { + return fmt.Errorf("build branch graph: %w", err) + } - if base.Change == nil && cmd.Publish { - log.Errorf("%v: base (%v) has not been submitted", cmd.Branch, b.Base) - return errors.New("submit the base branch first") - } + if cmd.Branch != store.Trunk() { + if err := cmd.verifyBaseSubmitted(log, graph, store, cmd.Branch); err != nil { + return err } } - upstacks, err := svc.ListUpstack(ctx, cmd.Branch) - if err != nil { - return fmt.Errorf("list upstack: %w", err) - } + upstacks := slices.Collect(graph.Upstack(cmd.Branch)) // If running from trunk, exclude trunk from the list. // Trunk cannot be submitted but everything upstack can. @@ -82,5 +72,34 @@ func (cmd *upstackSubmitCmd) Run( Branches: upstacks, Options: &cmd.Options, BatchOptions: &cmd.BatchOptions, + BranchGraph: graph, }) } + +func (cmd *upstackSubmitCmd) verifyBaseSubmitted( + log *silog.Logger, + graph *spice.BranchGraph, + store *state.Store, + branch string, +) error { + b, ok := graph.Lookup(branch) + if !ok { + return fmt.Errorf("lookup branch %v: %w", branch, git.ErrNotExist) + } + + if b.Base == store.Trunk() { + // If base is trunk, this check doesn't apply. + return nil + } + + base, ok := graph.Lookup(b.Base) + if !ok { + return fmt.Errorf("lookup base %v: %w", b.Base, git.ErrNotExist) + } + + if base.Change == nil && cmd.Publish { + log.Errorf("%v: base (%v) has not been submitted", branch, b.Base) + return errors.New("submit the base branch first") + } + return nil +}