From 0694ab51ea9d6b7de522b0ec0ab91097a0550240 Mon Sep 17 00:00:00 2001 From: Edmund Kohlwey Date: Tue, 12 May 2026 09:25:18 -0400 Subject: [PATCH 1/2] submit: validate downstack against forge before submit Before submitting, check whether any branch in the downstack has a base whose change has already been merged on the forge. If so, refuse to submit and instruct the user to run 'gs repo sync' first (or to bypass with --no-branch-check). This catches a class of confusion where an external merge (UI or auto-merge) has left local state and forge state in disagreement, and a subsequent submit would push to a base that no longer makes sense. Adds an optional-forge-repository DI binding so that submit commands that don't need a forge (e.g. --no-publish) don't trigger an eager remote-resolution prompt. --- .../unreleased/Added-20260512-100734.yaml | 3 + branch_submit.go | 44 ++++- doc/includes/cli-reference.md | 4 + downstack_submit.go | 7 + internal/spice/validate.go | 110 +++++++++++++ internal/spice/validate_test.go | 152 ++++++++++++++++++ main.go | 37 +++++ remote.go | 10 ++ stack_submit.go | 7 + testdata/help/branch_submit.txt | 1 + testdata/help/downstack_submit.txt | 1 + testdata/help/stack_submit.txt | 1 + testdata/help/upstack_submit.txt | 1 + testdata/script/branch_submit_stale_base.txt | 47 ++++++ .../script/downstack_submit_stale_base.txt | 47 ++++++ testdata/script/stack_submit_stale_base.txt | 47 ++++++ .../script/stale_base_closed_not_stale.txt | 44 +++++ testdata/script/stale_base_deep_stack.txt | 53 ++++++ .../script/stale_base_repo_sync_recovery.txt | 52 ++++++ testdata/script/upstack_submit_stale_base.txt | 48 ++++++ upstack_submit.go | 55 +++++-- 21 files changed, 755 insertions(+), 16 deletions(-) create mode 100644 .changes/unreleased/Added-20260512-100734.yaml create mode 100644 internal/spice/validate.go create mode 100644 internal/spice/validate_test.go create mode 100644 testdata/script/branch_submit_stale_base.txt create mode 100644 testdata/script/downstack_submit_stale_base.txt create mode 100644 testdata/script/stack_submit_stale_base.txt create mode 100644 testdata/script/stale_base_closed_not_stale.txt create mode 100644 testdata/script/stale_base_deep_stack.txt create mode 100644 testdata/script/stale_base_repo_sync_recovery.txt create mode 100644 testdata/script/upstack_submit_stale_base.txt diff --git a/.changes/unreleased/Added-20260512-100734.yaml b/.changes/unreleased/Added-20260512-100734.yaml new file mode 100644 index 000000000..6d597db53 --- /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 --no-branch-check to bypass)' +time: 2026-05-12T10:07:34.7611-04:00 diff --git a/branch_submit.go b/branch_submit.go index 63ab015bf..dee964052 100644 --- a/branch_submit.go +++ b/branch_submit.go @@ -2,10 +2,13 @@ package main import ( "context" + "errors" "fmt" + "go.abhg.dev/gs/internal/forge" "go.abhg.dev/gs/internal/git" "go.abhg.dev/gs/internal/handler/submit" + "go.abhg.dev/gs/internal/spice" "go.abhg.dev/gs/internal/text" ) @@ -13,7 +16,8 @@ import ( type submitOptions struct { submit.Options - NoWeb bool `help:"Alias for --web=false."` + NoWeb bool `help:"Alias for --web=false."` + NoBranchCheck bool `help:"Skip stale base validation before submitting."` // TODO: Other creation options e.g.: // - milestone @@ -91,6 +95,8 @@ type SubmitHandler interface { func (cmd *branchSubmitCmd) Run( ctx context.Context, wt *git.Worktree, + svc *spice.Service, + forgeRepo *optionalForgeRepository, submitHandler SubmitHandler, ) error { if cmd.NoWeb { @@ -105,6 +111,12 @@ func (cmd *branchSubmitCmd) Run( cmd.Branch = currentBranch } + if err := cmd.checkDownstack( + ctx, svc, forgeRepo.Repository, cmd.Branch, + ); err != nil { + return err + } + return submitHandler.Submit(ctx, &submit.Request{ Branch: cmd.Branch, Title: cmd.Title, @@ -112,3 +124,33 @@ func (cmd *branchSubmitCmd) Run( Options: &cmd.Options, }) } + +// checkDownstack validates that no branch in the downstack +// has a base whose forge change was already merged. +func (opts *submitOptions) checkDownstack( + ctx context.Context, + svc *spice.Service, + forgeRepo forge.Repository, + branch string, +) error { + if opts.NoBranchCheck { + return nil + } + + graph, err := svc.BranchGraph(ctx, nil) + if err != nil { + return fmt.Errorf("build branch graph: %w", err) + } + + err = spice.ValidateDownstack(ctx, graph, forgeRepo, branch) + var staleErr *spice.StaleBaseError + if errors.As(err, &staleErr) { + return fmt.Errorf( + "%s has stale base %s (already merged); "+ + "run 'gs repo sync' first, "+ + "or use --no-branch-check to bypass", + staleErr.Branch, staleErr.Base, + ) + } + return err +} diff --git a/doc/includes/cli-reference.md b/doc/includes/cli-reference.md index 1aee40d86..fc2522a88 100644 --- a/doc/includes/cli-reference.md +++ b/doc/includes/cli-reference.md @@ -277,6 +277,7 @@ only if there are multiple CRs in the stack. * `-r`, `--reviewer=REVIEWER,...`: Add reviewers to the change request. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `-a`, `--assign=ASSIGNEE,...`: Assign the change request to these users. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `--no-web`: Alias for --web=false. +* `--no-branch-check`: Skip stale base validation before submitting. **Configuration**: [spice.submit.assignees](/cli/config.md#spicesubmitassignees), [spice.submit.draft](/cli/config.md#spicesubmitdraft), [spice.submit.label](/cli/config.md#spicesubmitlabel), [spice.submit.listTemplatesTimeout](/cli/config.md#spicesubmitlisttemplatestimeout), [spice.submit.navigationComment](/cli/config.md#spicesubmitnavigationcomment), [spice.submit.navigationComment.downstack](/cli/config.md#spicesubmitnavigationcommentdownstack), [spice.submit.navigationCommentStyle.marker](/cli/config.md#spicesubmitnavigationcommentstylemarker), [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.updateOnly](/cli/config.md#spicesubmitupdateonly), [spice.submit.web](/cli/config.md#spicesubmitweb) @@ -399,6 +400,7 @@ only if there are multiple CRs in the stack. * `-r`, `--reviewer=REVIEWER,...`: Add reviewers to the change request. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `-a`, `--assign=ASSIGNEE,...`: Assign the change request to these users. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `--no-web`: Alias for --web=false. +* `--no-branch-check`: Skip stale base validation before submitting. * `--branch=NAME`: Branch to start at **Configuration**: [spice.submit.assignees](/cli/config.md#spicesubmitassignees), [spice.submit.draft](/cli/config.md#spicesubmitdraft), [spice.submit.label](/cli/config.md#spicesubmitlabel), [spice.submit.listTemplatesTimeout](/cli/config.md#spicesubmitlisttemplatestimeout), [spice.submit.navigationComment](/cli/config.md#spicesubmitnavigationcomment), [spice.submit.navigationComment.downstack](/cli/config.md#spicesubmitnavigationcommentdownstack), [spice.submit.navigationCommentStyle.marker](/cli/config.md#spicesubmitnavigationcommentstylemarker), [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.updateOnly](/cli/config.md#spicesubmitupdateonly), [spice.submit.web](/cli/config.md#spicesubmitweb) @@ -554,6 +556,7 @@ only if there are multiple CRs in the stack. * `-r`, `--reviewer=REVIEWER,...`: Add reviewers to the change request. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `-a`, `--assign=ASSIGNEE,...`: Assign the change request to these users. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `--no-web`: Alias for --web=false. +* `--no-branch-check`: Skip stale base validation before submitting. * `--branch=NAME`: Branch to start at **Configuration**: [spice.submit.assignees](/cli/config.md#spicesubmitassignees), [spice.submit.draft](/cli/config.md#spicesubmitdraft), [spice.submit.label](/cli/config.md#spicesubmitlabel), [spice.submit.listTemplatesTimeout](/cli/config.md#spicesubmitlisttemplatestimeout), [spice.submit.navigationComment](/cli/config.md#spicesubmitnavigationcomment), [spice.submit.navigationComment.downstack](/cli/config.md#spicesubmitnavigationcommentdownstack), [spice.submit.navigationCommentStyle.marker](/cli/config.md#spicesubmitnavigationcommentstylemarker), [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.updateOnly](/cli/config.md#spicesubmitupdateonly), [spice.submit.web](/cli/config.md#spicesubmitweb) @@ -1033,6 +1036,7 @@ only if there are multiple CRs in the stack. * `-r`, `--reviewer=REVIEWER,...`: Add reviewers to the change request. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `-a`, `--assign=ASSIGNEE,...`: Assign the change request to these users. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `--no-web`: Alias for --web=false. +* `--no-branch-check`: Skip stale base validation before submitting. * `--title=TITLE`: Title of the change request * `--body=BODY`: Body of the change request * `--branch=NAME`: Branch to submit diff --git a/downstack_submit.go b/downstack_submit.go index fc8fc7d61..d5bb65584 100644 --- a/downstack_submit.go +++ b/downstack_submit.go @@ -34,6 +34,7 @@ func (cmd *downstackSubmitCmd) Run( wt *git.Worktree, store *state.Store, svc *spice.Service, + forgeRepo *optionalForgeRepository, submitHandler SubmitHandler, ) error { if cmd.Branch == "" { @@ -48,6 +49,12 @@ func (cmd *downstackSubmitCmd) Run( return errors.New("nothing to submit below trunk") } + if err := cmd.checkDownstack( + ctx, svc, forgeRepo.Repository, cmd.Branch, + ); err != nil { + return err + } + downstacks, err := svc.ListDownstack(ctx, cmd.Branch) if err != nil { return fmt.Errorf("list downstack: %w", err) diff --git a/internal/spice/validate.go b/internal/spice/validate.go new file mode 100644 index 000000000..d02432dc3 --- /dev/null +++ b/internal/spice/validate.go @@ -0,0 +1,110 @@ +package spice + +import ( + "context" + "fmt" + + "go.abhg.dev/gs/internal/forge" +) + +// StaleBaseError indicates a branch whose base +// was merged on the forge but not yet rebased onto trunk. +type StaleBaseError struct { + // Branch is the branch with the stale base. + Branch string + + // Base is the merged base branch. + Base string +} + +func (e *StaleBaseError) Error() string { + return fmt.Sprintf( + "%s has stale base %s (already merged)", + e.Branch, e.Base, + ) +} + +// staleBaseCandidate is a branch whose base has a published +// change that needs forge state verification. +type staleBaseCandidate struct { + branch string + base string + changeID forge.ChangeID +} + +// ValidateDownstack checks that no branch in the downstack +// has a base whose forge change has already been merged. +// Returns a *StaleBaseError if a stale base is detected. +// +// If forgeRepo is nil (e.g. unsupported forge), validation is skipped. +func ValidateDownstack( + ctx context.Context, + graph *BranchGraph, + forgeRepo forge.Repository, + branch string, +) error { + if forgeRepo == nil { + return nil + } + candidates := collectStaleCandidates(graph, branch) + if len(candidates) == 0 { + return nil + } + return checkStaleStates(ctx, forgeRepo, candidates) +} + +// collectStaleCandidates walks the downstack +// and returns branches whose base has a published change. +func collectStaleCandidates( + graph *BranchGraph, + branch string, +) []staleBaseCandidate { + trunk := graph.Trunk() + var candidates []staleBaseCandidate + for name := range graph.Downstack(branch) { + item, ok := graph.Lookup(name) + if !ok || item.Base == trunk { + continue + } + + baseItem, ok := graph.Lookup(item.Base) + if !ok || baseItem.Change == nil { + continue + } + + candidates = append(candidates, staleBaseCandidate{ + branch: name, + base: item.Base, + changeID: baseItem.Change.ChangeID(), + }) + } + return candidates +} + +// checkStaleStates queries the forge for change states +// and returns a *StaleBaseError if any base is merged. +func checkStaleStates( + ctx context.Context, + forgeRepo forge.Repository, + candidates []staleBaseCandidate, +) error { + 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 fmt.Errorf("query change states: %w", err) + } + + for i, c := range candidates { + if statuses[i].State == forge.ChangeMerged { + return &StaleBaseError{ + Branch: c.branch, + Base: c.base, + } + } + } + return nil +} diff --git a/internal/spice/validate_test.go b/internal/spice/validate_test.go new file mode 100644 index 000000000..600e7fb2a --- /dev/null +++ b/internal/spice/validate_test.go @@ -0,0 +1,152 @@ +package spice + +import ( + "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" +) + +// fakeChangeMetadata is a minimal forge.ChangeMetadata +// for use in tests. +type fakeChangeMetadata struct { + id forge.ChangeID +} + +var _ forge.ChangeMetadata = (*fakeChangeMetadata)(nil) + +func (m *fakeChangeMetadata) ForgeID() string { return "test" } +func (m *fakeChangeMetadata) ChangeID() forge.ChangeID { return m.id } +func (m *fakeChangeMetadata) NavigationCommentID() forge.ChangeCommentID { return nil } +func (m *fakeChangeMetadata) SetNavigationCommentID(forge.ChangeCommentID) {} + +// fakeChangeID is a string-based ChangeID for testing. +type fakeChangeID string + +func (f fakeChangeID) String() string { return string(f) } + +func TestValidateDownstack_healthy(t *testing.T) { + ctrl := gomock.NewController(t) + + graph := buildTestGraph(t, "main", []LoadBranchItem{ + {Name: "feat1", Base: "main", Change: fakeChange("pr-1")}, + {Name: "feat2", Base: "feat1", Change: fakeChange("pr-2")}, + }) + + mockRepo := forgetest.NewMockRepository(ctrl) + mockRepo.EXPECT(). + ChangeStatuses(gomock.Any(), []forge.ChangeID{fakeChangeID("pr-1")}). + Return([]forge.ChangeStatus{{State: forge.ChangeOpen}}, nil) + + err := ValidateDownstack(t.Context(), graph, mockRepo, "feat2") + require.NoError(t, err) +} + +func TestValidateDownstack_staleBase(t *testing.T) { + ctrl := gomock.NewController(t) + + graph := buildTestGraph(t, "main", []LoadBranchItem{ + {Name: "feat1", Base: "main", Change: fakeChange("pr-1")}, + {Name: "feat2", Base: "feat1", Change: fakeChange("pr-2")}, + }) + + mockRepo := forgetest.NewMockRepository(ctrl) + mockRepo.EXPECT(). + ChangeStatuses(gomock.Any(), []forge.ChangeID{fakeChangeID("pr-1")}). + Return([]forge.ChangeStatus{{State: forge.ChangeMerged}}, nil) + + err := ValidateDownstack(t.Context(), graph, mockRepo, "feat2") + + var staleErr *StaleBaseError + require.ErrorAs(t, err, &staleErr) + assert.Equal(t, "feat2", staleErr.Branch) + assert.Equal(t, "feat1", staleErr.Base) +} + +func TestValidateDownstack_baseWithoutChange(t *testing.T) { + graph := buildTestGraph(t, "main", []LoadBranchItem{ + {Name: "feat1", Base: "main"}, + {Name: "feat2", Base: "feat1", Change: fakeChange("pr-2")}, + }) + + // No forge call expected: feat1 has no published change. + err := ValidateDownstack( + t.Context(), graph, nil /* unused */, "feat2", + ) + require.NoError(t, err) +} + +func TestValidateDownstack_singleBranch(t *testing.T) { + graph := buildTestGraph(t, "main", []LoadBranchItem{ + {Name: "feat1", Base: "main", Change: fakeChange("pr-1")}, + }) + + // No forge call expected: base is trunk. + err := ValidateDownstack( + t.Context(), graph, nil /* unused */, "feat1", + ) + require.NoError(t, err) +} + +func TestValidateDownstack_deepStack(t *testing.T) { + ctrl := gomock.NewController(t) + + // trunk <- A <- B <- C; A is merged. + graph := buildTestGraph(t, "main", []LoadBranchItem{ + {Name: "A", Base: "main", Change: fakeChange("pr-A")}, + {Name: "B", Base: "A", Change: fakeChange("pr-B")}, + {Name: "C", Base: "B", Change: fakeChange("pr-C")}, + }) + + // Both A and B are checked (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 + }) + + err := ValidateDownstack(t.Context(), graph, mockRepo, "C") + + var staleErr *StaleBaseError + require.ErrorAs(t, err, &staleErr) + // B's base is A which is merged. + assert.Equal(t, "B", staleErr.Branch) + assert.Equal(t, "A", staleErr.Base) +} + +// buildTestGraph constructs a BranchGraph from the given branches. +func buildTestGraph( + t *testing.T, + trunk string, + branches []LoadBranchItem, +) *BranchGraph { + t.Helper() + graph, err := NewBranchGraph(t.Context(), &branchLoaderStub{ + trunk: trunk, + branches: branches, + }, nil) + require.NoError(t, err) + return graph +} + +// fakeChange creates a minimal ChangeMetadata +// for the given change ID. +func fakeChange(id string) forge.ChangeMetadata { + return &fakeChangeMetadata{id: fakeChangeID(id)} +} diff --git a/main.go b/main.go index aedac02bc..235baf616 100644 --- a/main.go +++ b/main.go @@ -457,6 +457,43 @@ func (cmd *mainCmd) AfterApply(ctx context.Context, kctx *kong.Context, logger * }, }, nil }), + kctx.BindSingletonProvider(func( + _ *silog.Logger, + _ ui.View, + store *state.Store, + secretStash secret.Stash, + forges *forge.Registry, + repo *git.Repository, + ) (*optionalForgeRepository, error) { + // Use the cached remote if one is already set. + // Don't call ensureRemote eagerly: that would + // run prompts before the command has a chance + // to decide whether it needs a forge at all + // (e.g. submit --no-publish). + remote, err := store.Remote() + if err != nil { + if errors.Is(err, state.ErrNotExist) { + // Defer remote resolution to the + // command itself; treat as "no + // supported forge available." + return &optionalForgeRepository{}, nil + } + return nil, fmt.Errorf("get remote: %w", err) + } + remoteRepo, err := openRemoteRepositorySilent( + ctx, secretStash, forges, repo, remote.Upstream, + ) + if err != nil { + var unsupported *unsupportedForgeError + if errors.As(err, &unsupported) { + return &optionalForgeRepository{}, nil + } + return nil, err + } + return &optionalForgeRepository{ + Repository: remoteRepo, + }, nil + }), kctx.BindSingletonProvider(func( log *silog.Logger, worktree *git.Worktree, diff --git a/remote.go b/remote.go index f230c56f8..35df1d83d 100644 --- a/remote.go +++ b/remote.go @@ -108,6 +108,16 @@ func openForgeRepository( return f.OpenRepository(ctx, tok, repoID) } +// optionalForgeRepository wraps a forge.Repository that may be unsupported. +// When the remote is not a supported forge, Repository is nil +// but the wrapper itself is non-nil so kong DI can inject it. +// +// Commands that need forge access should check Repository for nil and +// short-circuit accordingly (e.g. submit --no-publish skips validation). +type optionalForgeRepository struct { + forge.Repository +} + func resolveRemoteRepository( ctx context.Context, log *silog.Logger, diff --git a/stack_submit.go b/stack_submit.go index 9f857eeae..8f3bf4358 100644 --- a/stack_submit.go +++ b/stack_submit.go @@ -28,6 +28,7 @@ func (cmd *stackSubmitCmd) Run( wt *git.Worktree, store *state.Store, svc *spice.Service, + forgeRepo *optionalForgeRepository, submitHandler SubmitHandler, ) error { currentBranch, err := wt.CurrentBranch(ctx) @@ -35,6 +36,12 @@ func (cmd *stackSubmitCmd) Run( return fmt.Errorf("get current branch: %w", err) } + if err := cmd.checkDownstack( + ctx, svc, forgeRepo.Repository, currentBranch, + ); err != nil { + return err + } + stack, err := svc.ListStack(ctx, currentBranch) if err != nil { return fmt.Errorf("list stack: %w", err) diff --git a/testdata/help/branch_submit.txt b/testdata/help/branch_submit.txt index 73d34acf8..632a41719 100644 --- a/testdata/help/branch_submit.txt +++ b/testdata/help/branch_submit.txt @@ -49,6 +49,7 @@ Flags: -a, --assign=ASSIGNEE,... Assign the change request to these users. Pass multiple times or separate with commas. --no-web Alias for --web=false. + --no-branch-check Skip stale base validation before submitting. --title=TITLE Title of the change request --body=BODY Body of the change request --branch=NAME Branch to submit diff --git a/testdata/help/downstack_submit.txt b/testdata/help/downstack_submit.txt index 956ed1ad7..1ff54f40f 100644 --- a/testdata/help/downstack_submit.txt +++ b/testdata/help/downstack_submit.txt @@ -50,6 +50,7 @@ Flags: -a, --assign=ASSIGNEE,... Assign the change request to these users. Pass multiple times or separate with commas. --no-web Alias for --web=false. + --no-branch-check Skip stale base validation before submitting. --branch=NAME Branch to start at Global Flags: diff --git a/testdata/help/stack_submit.txt b/testdata/help/stack_submit.txt index 1a76ad80b..069967a4f 100644 --- a/testdata/help/stack_submit.txt +++ b/testdata/help/stack_submit.txt @@ -49,6 +49,7 @@ Flags: -a, --assign=ASSIGNEE,... Assign the change request to these users. Pass multiple times or separate with commas. --no-web Alias for --web=false. + --no-branch-check Skip stale base validation before submitting. Global Flags: -h, --help Show help for the command diff --git a/testdata/help/upstack_submit.txt b/testdata/help/upstack_submit.txt index fe39b137a..2b4d269cf 100644 --- a/testdata/help/upstack_submit.txt +++ b/testdata/help/upstack_submit.txt @@ -52,6 +52,7 @@ Flags: -a, --assign=ASSIGNEE,... Assign the change request to these users. Pass multiple times or separate with commas. --no-web Alias for --web=false. + --no-branch-check Skip stale base validation before submitting. --branch=NAME Branch to start at Global Flags: diff --git a/testdata/script/branch_submit_stale_base.txt b/testdata/script/branch_submit_stale_base.txt new file mode 100644 index 000000000..3307d3663 --- /dev/null +++ b/testdata/script/branch_submit_stale_base.txt @@ -0,0 +1,47 @@ +# '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 'stale base feature1' +stderr 'gs repo sync' +stderr 'no-branch-check' + +# --no-branch-check bypasses the stale base check +gs bs --fill --no-branch-check +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..a8e2f97c2 --- /dev/null +++ b/testdata/script/downstack_submit_stale_base.txt @@ -0,0 +1,47 @@ +# '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 'stale base feature1' +stderr 'gs repo sync' +stderr 'no-branch-check' + +# --no-branch-check bypasses the stale base check +gs dss --fill --no-branch-check +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..67fd5138f --- /dev/null +++ b/testdata/script/stack_submit_stale_base.txt @@ -0,0 +1,47 @@ +# '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 'stale base feature1' +stderr 'gs repo sync' +stderr 'no-branch-check' + +# --no-branch-check bypasses the stale base check +gs stack submit --fill --no-branch-check +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..8169d4aea --- /dev/null +++ b/testdata/script/stale_base_deep_stack.txt @@ -0,0 +1,53 @@ +# 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 'stale base feature1' +stderr 'gs repo sync' +stderr 'no-branch-check' + +# --no-branch-check bypasses the stale base check for submit +gs ss --fill --no-branch-check +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..692615081 --- /dev/null +++ b/testdata/script/stale_base_repo_sync_recovery.txt @@ -0,0 +1,52 @@ +# 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 'stale base feature1' +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..1083d65b8 --- /dev/null +++ b/testdata/script/upstack_submit_stale_base.txt @@ -0,0 +1,48 @@ +# '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 'stale base feature1' +stderr 'gs repo sync' +stderr 'no-branch-check' + +# --no-branch-check bypasses the stale base check +gs upstack submit --fill --no-branch-check +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..359700a29 100644 --- a/upstack_submit.go +++ b/upstack_submit.go @@ -36,6 +36,7 @@ func (cmd *upstackSubmitCmd) Run( wt *git.Worktree, store *state.Store, svc *spice.Service, + forgeRepo *optionalForgeRepository, submitHandler SubmitHandler, ) error { if cmd.Branch == "" { @@ -46,22 +47,17 @@ 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) - } + if err := cmd.checkDownstack( + ctx, svc, forgeRepo.Repository, cmd.Branch, + ); err != nil { + return 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( + ctx, log, svc, store, cmd.Branch, + ); err != nil { + return err } } @@ -84,3 +80,32 @@ func (cmd *upstackSubmitCmd) Run( BatchOptions: &cmd.BatchOptions, }) } + +func (cmd *upstackSubmitCmd) verifyBaseSubmitted( + ctx context.Context, + log *silog.Logger, + svc *spice.Service, + store *state.Store, + branch string, +) error { + b, err := svc.LookupBranch(ctx, branch) + if err != nil { + return fmt.Errorf("lookup branch %v: %w", branch, err) + } + + if b.Base == store.Trunk() { + return nil + } + + base, err := svc.LookupBranch(ctx, b.Base) + if err != nil { + return fmt.Errorf("lookup base %v: %w", b.Base, err) + } + + 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 +} From 6b9c3ff860c08712cf05b7be9d596b328d620e59 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sun, 17 May 2026 11:36:16 -0700 Subject: [PATCH 2/2] submit: Centralize stale base checks Submit should reject stale downstack bases through one handler-level preflight instead of spreading the same forge validation through each submit command. This moves the check into the submit handler, where the complete submission scope is already known. Batch submit commands build one branch graph for their scoped branches, then pass that graph through the request so branch lookup and downstack walking reuse the same local topology. The handler collects the submitted branches and their downstacks, de-duplicates their forge change IDs, and fetches CR status in one batch before any submit work mutates state. The validation now belongs to submit-specific flow instead of the shared spice package. Branch and stack submit modes no longer have a separate `--no-branch-check` bypass; they use the existing `--force` escape hatch and tell callers to use it when the stale-base preflight blocks submission. Navigation comments still build their own graph after submitBranch updates local branch metadata, because their comments must reflect the post-submit change IDs and upstream branch names. Unit tests cover the handler preflight and graph reuse, and script tests cover stale-base rejection and `--force` bypasses across branch, stack, downstack, and upstack submit flows. --- .../unreleased/Added-20260512-100734.yaml | 2 +- branch_submit.go | 44 +---- doc/includes/cli-reference.md | 4 - downstack_submit.go | 14 +- internal/handler/submit/handler.go | 52 +++++- internal/handler/submit/handler_test.go | 115 ++++++++++++ internal/handler/submit/mocks_test.go | 78 ++++---- internal/handler/submit/stale_base.go | 163 ++++++++++++++++ internal/handler/submit/stale_base_test.go | 175 ++++++++++++++++++ internal/spice/validate.go | 110 ----------- internal/spice/validate_test.go | 152 --------------- main.go | 37 ---- remote.go | 10 - stack_submit.go | 16 +- testdata/help/branch_submit.txt | 1 - testdata/help/downstack_submit.txt | 1 - testdata/help/stack_submit.txt | 1 - testdata/help/upstack_submit.txt | 1 - testdata/script/branch_submit_stale_base.txt | 9 +- .../script/downstack_submit_stale_base.txt | 9 +- testdata/script/stack_submit_stale_base.txt | 9 +- testdata/script/stale_base_deep_stack.txt | 9 +- .../script/stale_base_repo_sync_recovery.txt | 3 +- testdata/script/upstack_submit_stale_base.txt | 9 +- upstack_submit.go | 38 ++-- 25 files changed, 595 insertions(+), 467 deletions(-) create mode 100644 internal/handler/submit/stale_base.go create mode 100644 internal/handler/submit/stale_base_test.go delete mode 100644 internal/spice/validate.go delete mode 100644 internal/spice/validate_test.go diff --git a/.changes/unreleased/Added-20260512-100734.yaml b/.changes/unreleased/Added-20260512-100734.yaml index 6d597db53..0e23c46f3 100644 --- a/.changes/unreleased/Added-20260512-100734.yaml +++ b/.changes/unreleased/Added-20260512-100734.yaml @@ -1,3 +1,3 @@ kind: Added -body: 'submit: Skip submission when a downstack base''s change has been merged (use --no-branch-check to bypass)' +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/branch_submit.go b/branch_submit.go index dee964052..63ab015bf 100644 --- a/branch_submit.go +++ b/branch_submit.go @@ -2,13 +2,10 @@ package main import ( "context" - "errors" "fmt" - "go.abhg.dev/gs/internal/forge" "go.abhg.dev/gs/internal/git" "go.abhg.dev/gs/internal/handler/submit" - "go.abhg.dev/gs/internal/spice" "go.abhg.dev/gs/internal/text" ) @@ -16,8 +13,7 @@ import ( type submitOptions struct { submit.Options - NoWeb bool `help:"Alias for --web=false."` - NoBranchCheck bool `help:"Skip stale base validation before submitting."` + NoWeb bool `help:"Alias for --web=false."` // TODO: Other creation options e.g.: // - milestone @@ -95,8 +91,6 @@ type SubmitHandler interface { func (cmd *branchSubmitCmd) Run( ctx context.Context, wt *git.Worktree, - svc *spice.Service, - forgeRepo *optionalForgeRepository, submitHandler SubmitHandler, ) error { if cmd.NoWeb { @@ -111,12 +105,6 @@ func (cmd *branchSubmitCmd) Run( cmd.Branch = currentBranch } - if err := cmd.checkDownstack( - ctx, svc, forgeRepo.Repository, cmd.Branch, - ); err != nil { - return err - } - return submitHandler.Submit(ctx, &submit.Request{ Branch: cmd.Branch, Title: cmd.Title, @@ -124,33 +112,3 @@ func (cmd *branchSubmitCmd) Run( Options: &cmd.Options, }) } - -// checkDownstack validates that no branch in the downstack -// has a base whose forge change was already merged. -func (opts *submitOptions) checkDownstack( - ctx context.Context, - svc *spice.Service, - forgeRepo forge.Repository, - branch string, -) error { - if opts.NoBranchCheck { - return nil - } - - graph, err := svc.BranchGraph(ctx, nil) - if err != nil { - return fmt.Errorf("build branch graph: %w", err) - } - - err = spice.ValidateDownstack(ctx, graph, forgeRepo, branch) - var staleErr *spice.StaleBaseError - if errors.As(err, &staleErr) { - return fmt.Errorf( - "%s has stale base %s (already merged); "+ - "run 'gs repo sync' first, "+ - "or use --no-branch-check to bypass", - staleErr.Branch, staleErr.Base, - ) - } - return err -} diff --git a/doc/includes/cli-reference.md b/doc/includes/cli-reference.md index fc2522a88..1aee40d86 100644 --- a/doc/includes/cli-reference.md +++ b/doc/includes/cli-reference.md @@ -277,7 +277,6 @@ only if there are multiple CRs in the stack. * `-r`, `--reviewer=REVIEWER,...`: Add reviewers to the change request. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `-a`, `--assign=ASSIGNEE,...`: Assign the change request to these users. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `--no-web`: Alias for --web=false. -* `--no-branch-check`: Skip stale base validation before submitting. **Configuration**: [spice.submit.assignees](/cli/config.md#spicesubmitassignees), [spice.submit.draft](/cli/config.md#spicesubmitdraft), [spice.submit.label](/cli/config.md#spicesubmitlabel), [spice.submit.listTemplatesTimeout](/cli/config.md#spicesubmitlisttemplatestimeout), [spice.submit.navigationComment](/cli/config.md#spicesubmitnavigationcomment), [spice.submit.navigationComment.downstack](/cli/config.md#spicesubmitnavigationcommentdownstack), [spice.submit.navigationCommentStyle.marker](/cli/config.md#spicesubmitnavigationcommentstylemarker), [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.updateOnly](/cli/config.md#spicesubmitupdateonly), [spice.submit.web](/cli/config.md#spicesubmitweb) @@ -400,7 +399,6 @@ only if there are multiple CRs in the stack. * `-r`, `--reviewer=REVIEWER,...`: Add reviewers to the change request. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `-a`, `--assign=ASSIGNEE,...`: Assign the change request to these users. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `--no-web`: Alias for --web=false. -* `--no-branch-check`: Skip stale base validation before submitting. * `--branch=NAME`: Branch to start at **Configuration**: [spice.submit.assignees](/cli/config.md#spicesubmitassignees), [spice.submit.draft](/cli/config.md#spicesubmitdraft), [spice.submit.label](/cli/config.md#spicesubmitlabel), [spice.submit.listTemplatesTimeout](/cli/config.md#spicesubmitlisttemplatestimeout), [spice.submit.navigationComment](/cli/config.md#spicesubmitnavigationcomment), [spice.submit.navigationComment.downstack](/cli/config.md#spicesubmitnavigationcommentdownstack), [spice.submit.navigationCommentStyle.marker](/cli/config.md#spicesubmitnavigationcommentstylemarker), [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.updateOnly](/cli/config.md#spicesubmitupdateonly), [spice.submit.web](/cli/config.md#spicesubmitweb) @@ -556,7 +554,6 @@ only if there are multiple CRs in the stack. * `-r`, `--reviewer=REVIEWER,...`: Add reviewers to the change request. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `-a`, `--assign=ASSIGNEE,...`: Assign the change request to these users. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `--no-web`: Alias for --web=false. -* `--no-branch-check`: Skip stale base validation before submitting. * `--branch=NAME`: Branch to start at **Configuration**: [spice.submit.assignees](/cli/config.md#spicesubmitassignees), [spice.submit.draft](/cli/config.md#spicesubmitdraft), [spice.submit.label](/cli/config.md#spicesubmitlabel), [spice.submit.listTemplatesTimeout](/cli/config.md#spicesubmitlisttemplatestimeout), [spice.submit.navigationComment](/cli/config.md#spicesubmitnavigationcomment), [spice.submit.navigationComment.downstack](/cli/config.md#spicesubmitnavigationcommentdownstack), [spice.submit.navigationCommentStyle.marker](/cli/config.md#spicesubmitnavigationcommentstylemarker), [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.updateOnly](/cli/config.md#spicesubmitupdateonly), [spice.submit.web](/cli/config.md#spicesubmitweb) @@ -1036,7 +1033,6 @@ only if there are multiple CRs in the stack. * `-r`, `--reviewer=REVIEWER,...`: Add reviewers to the change request. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `-a`, `--assign=ASSIGNEE,...`: Assign the change request to these users. Pass multiple times or separate with commas. :material-tag:{ title="Released in version" }[v0.21.0](/changelog.md#v0.21.0) * `--no-web`: Alias for --web=false. -* `--no-branch-check`: Skip stale base validation before submitting. * `--title=TITLE`: Title of the change request * `--body=BODY`: Body of the change request * `--branch=NAME`: Branch to submit diff --git a/downstack_submit.go b/downstack_submit.go index d5bb65584..44c0ca5a3 100644 --- a/downstack_submit.go +++ b/downstack_submit.go @@ -34,7 +34,6 @@ func (cmd *downstackSubmitCmd) Run( wt *git.Worktree, store *state.Store, svc *spice.Service, - forgeRepo *optionalForgeRepository, submitHandler SubmitHandler, ) error { if cmd.Branch == "" { @@ -49,16 +48,12 @@ func (cmd *downstackSubmitCmd) Run( return errors.New("nothing to submit below trunk") } - if err := cmd.checkDownstack( - ctx, svc, forgeRepo.Repository, cmd.Branch, - ); err != nil { - return err - } - - 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) @@ -68,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 a4dbbf98f..7dd8cd256 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) } @@ -411,6 +411,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, @@ -426,12 +433,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}, ) @@ -469,6 +489,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, @@ -476,8 +500,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, @@ -523,6 +560,7 @@ type submitOptions struct { func (h *Handler) submitBranch( ctx context.Context, + graph *spice.BranchGraph, branchToSubmit string, opts *submitOptions, ) (status submitStatus, err error) { @@ -533,9 +571,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 @@ -575,9 +613,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 73966ed8e..804e3de02 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 @@ -261,3 +349,30 @@ func (id stubRepositoryID) String() string { 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/internal/spice/validate.go b/internal/spice/validate.go deleted file mode 100644 index d02432dc3..000000000 --- a/internal/spice/validate.go +++ /dev/null @@ -1,110 +0,0 @@ -package spice - -import ( - "context" - "fmt" - - "go.abhg.dev/gs/internal/forge" -) - -// StaleBaseError indicates a branch whose base -// was merged on the forge but not yet rebased onto trunk. -type StaleBaseError struct { - // Branch is the branch with the stale base. - Branch string - - // Base is the merged base branch. - Base string -} - -func (e *StaleBaseError) Error() string { - return fmt.Sprintf( - "%s has stale base %s (already merged)", - e.Branch, e.Base, - ) -} - -// staleBaseCandidate is a branch whose base has a published -// change that needs forge state verification. -type staleBaseCandidate struct { - branch string - base string - changeID forge.ChangeID -} - -// ValidateDownstack checks that no branch in the downstack -// has a base whose forge change has already been merged. -// Returns a *StaleBaseError if a stale base is detected. -// -// If forgeRepo is nil (e.g. unsupported forge), validation is skipped. -func ValidateDownstack( - ctx context.Context, - graph *BranchGraph, - forgeRepo forge.Repository, - branch string, -) error { - if forgeRepo == nil { - return nil - } - candidates := collectStaleCandidates(graph, branch) - if len(candidates) == 0 { - return nil - } - return checkStaleStates(ctx, forgeRepo, candidates) -} - -// collectStaleCandidates walks the downstack -// and returns branches whose base has a published change. -func collectStaleCandidates( - graph *BranchGraph, - branch string, -) []staleBaseCandidate { - trunk := graph.Trunk() - var candidates []staleBaseCandidate - for name := range graph.Downstack(branch) { - item, ok := graph.Lookup(name) - if !ok || item.Base == trunk { - continue - } - - baseItem, ok := graph.Lookup(item.Base) - if !ok || baseItem.Change == nil { - continue - } - - candidates = append(candidates, staleBaseCandidate{ - branch: name, - base: item.Base, - changeID: baseItem.Change.ChangeID(), - }) - } - return candidates -} - -// checkStaleStates queries the forge for change states -// and returns a *StaleBaseError if any base is merged. -func checkStaleStates( - ctx context.Context, - forgeRepo forge.Repository, - candidates []staleBaseCandidate, -) error { - 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 fmt.Errorf("query change states: %w", err) - } - - for i, c := range candidates { - if statuses[i].State == forge.ChangeMerged { - return &StaleBaseError{ - Branch: c.branch, - Base: c.base, - } - } - } - return nil -} diff --git a/internal/spice/validate_test.go b/internal/spice/validate_test.go deleted file mode 100644 index 600e7fb2a..000000000 --- a/internal/spice/validate_test.go +++ /dev/null @@ -1,152 +0,0 @@ -package spice - -import ( - "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" -) - -// fakeChangeMetadata is a minimal forge.ChangeMetadata -// for use in tests. -type fakeChangeMetadata struct { - id forge.ChangeID -} - -var _ forge.ChangeMetadata = (*fakeChangeMetadata)(nil) - -func (m *fakeChangeMetadata) ForgeID() string { return "test" } -func (m *fakeChangeMetadata) ChangeID() forge.ChangeID { return m.id } -func (m *fakeChangeMetadata) NavigationCommentID() forge.ChangeCommentID { return nil } -func (m *fakeChangeMetadata) SetNavigationCommentID(forge.ChangeCommentID) {} - -// fakeChangeID is a string-based ChangeID for testing. -type fakeChangeID string - -func (f fakeChangeID) String() string { return string(f) } - -func TestValidateDownstack_healthy(t *testing.T) { - ctrl := gomock.NewController(t) - - graph := buildTestGraph(t, "main", []LoadBranchItem{ - {Name: "feat1", Base: "main", Change: fakeChange("pr-1")}, - {Name: "feat2", Base: "feat1", Change: fakeChange("pr-2")}, - }) - - mockRepo := forgetest.NewMockRepository(ctrl) - mockRepo.EXPECT(). - ChangeStatuses(gomock.Any(), []forge.ChangeID{fakeChangeID("pr-1")}). - Return([]forge.ChangeStatus{{State: forge.ChangeOpen}}, nil) - - err := ValidateDownstack(t.Context(), graph, mockRepo, "feat2") - require.NoError(t, err) -} - -func TestValidateDownstack_staleBase(t *testing.T) { - ctrl := gomock.NewController(t) - - graph := buildTestGraph(t, "main", []LoadBranchItem{ - {Name: "feat1", Base: "main", Change: fakeChange("pr-1")}, - {Name: "feat2", Base: "feat1", Change: fakeChange("pr-2")}, - }) - - mockRepo := forgetest.NewMockRepository(ctrl) - mockRepo.EXPECT(). - ChangeStatuses(gomock.Any(), []forge.ChangeID{fakeChangeID("pr-1")}). - Return([]forge.ChangeStatus{{State: forge.ChangeMerged}}, nil) - - err := ValidateDownstack(t.Context(), graph, mockRepo, "feat2") - - var staleErr *StaleBaseError - require.ErrorAs(t, err, &staleErr) - assert.Equal(t, "feat2", staleErr.Branch) - assert.Equal(t, "feat1", staleErr.Base) -} - -func TestValidateDownstack_baseWithoutChange(t *testing.T) { - graph := buildTestGraph(t, "main", []LoadBranchItem{ - {Name: "feat1", Base: "main"}, - {Name: "feat2", Base: "feat1", Change: fakeChange("pr-2")}, - }) - - // No forge call expected: feat1 has no published change. - err := ValidateDownstack( - t.Context(), graph, nil /* unused */, "feat2", - ) - require.NoError(t, err) -} - -func TestValidateDownstack_singleBranch(t *testing.T) { - graph := buildTestGraph(t, "main", []LoadBranchItem{ - {Name: "feat1", Base: "main", Change: fakeChange("pr-1")}, - }) - - // No forge call expected: base is trunk. - err := ValidateDownstack( - t.Context(), graph, nil /* unused */, "feat1", - ) - require.NoError(t, err) -} - -func TestValidateDownstack_deepStack(t *testing.T) { - ctrl := gomock.NewController(t) - - // trunk <- A <- B <- C; A is merged. - graph := buildTestGraph(t, "main", []LoadBranchItem{ - {Name: "A", Base: "main", Change: fakeChange("pr-A")}, - {Name: "B", Base: "A", Change: fakeChange("pr-B")}, - {Name: "C", Base: "B", Change: fakeChange("pr-C")}, - }) - - // Both A and B are checked (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 - }) - - err := ValidateDownstack(t.Context(), graph, mockRepo, "C") - - var staleErr *StaleBaseError - require.ErrorAs(t, err, &staleErr) - // B's base is A which is merged. - assert.Equal(t, "B", staleErr.Branch) - assert.Equal(t, "A", staleErr.Base) -} - -// buildTestGraph constructs a BranchGraph from the given branches. -func buildTestGraph( - t *testing.T, - trunk string, - branches []LoadBranchItem, -) *BranchGraph { - t.Helper() - graph, err := NewBranchGraph(t.Context(), &branchLoaderStub{ - trunk: trunk, - branches: branches, - }, nil) - require.NoError(t, err) - return graph -} - -// fakeChange creates a minimal ChangeMetadata -// for the given change ID. -func fakeChange(id string) forge.ChangeMetadata { - return &fakeChangeMetadata{id: fakeChangeID(id)} -} diff --git a/main.go b/main.go index 235baf616..aedac02bc 100644 --- a/main.go +++ b/main.go @@ -457,43 +457,6 @@ func (cmd *mainCmd) AfterApply(ctx context.Context, kctx *kong.Context, logger * }, }, nil }), - kctx.BindSingletonProvider(func( - _ *silog.Logger, - _ ui.View, - store *state.Store, - secretStash secret.Stash, - forges *forge.Registry, - repo *git.Repository, - ) (*optionalForgeRepository, error) { - // Use the cached remote if one is already set. - // Don't call ensureRemote eagerly: that would - // run prompts before the command has a chance - // to decide whether it needs a forge at all - // (e.g. submit --no-publish). - remote, err := store.Remote() - if err != nil { - if errors.Is(err, state.ErrNotExist) { - // Defer remote resolution to the - // command itself; treat as "no - // supported forge available." - return &optionalForgeRepository{}, nil - } - return nil, fmt.Errorf("get remote: %w", err) - } - remoteRepo, err := openRemoteRepositorySilent( - ctx, secretStash, forges, repo, remote.Upstream, - ) - if err != nil { - var unsupported *unsupportedForgeError - if errors.As(err, &unsupported) { - return &optionalForgeRepository{}, nil - } - return nil, err - } - return &optionalForgeRepository{ - Repository: remoteRepo, - }, nil - }), kctx.BindSingletonProvider(func( log *silog.Logger, worktree *git.Worktree, diff --git a/remote.go b/remote.go index 35df1d83d..f230c56f8 100644 --- a/remote.go +++ b/remote.go @@ -108,16 +108,6 @@ func openForgeRepository( return f.OpenRepository(ctx, tok, repoID) } -// optionalForgeRepository wraps a forge.Repository that may be unsupported. -// When the remote is not a supported forge, Repository is nil -// but the wrapper itself is non-nil so kong DI can inject it. -// -// Commands that need forge access should check Repository for nil and -// short-circuit accordingly (e.g. submit --no-publish skips validation). -type optionalForgeRepository struct { - forge.Repository -} - func resolveRemoteRepository( ctx context.Context, log *silog.Logger, diff --git a/stack_submit.go b/stack_submit.go index 8f3bf4358..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" @@ -28,7 +29,6 @@ func (cmd *stackSubmitCmd) Run( wt *git.Worktree, store *state.Store, svc *spice.Service, - forgeRepo *optionalForgeRepository, submitHandler SubmitHandler, ) error { currentBranch, err := wt.CurrentBranch(ctx) @@ -36,15 +36,14 @@ func (cmd *stackSubmitCmd) Run( return fmt.Errorf("get current branch: %w", err) } - if err := cmd.checkDownstack( - ctx, svc, forgeRepo.Repository, currentBranch, - ); err != nil { - return err + graph, err := svc.BranchGraph(ctx, nil) + if err != nil { + return fmt.Errorf("build branch graph: %w", err) } - stack, err := svc.ListStack(ctx, currentBranch) - if err != nil { - return fmt.Errorf("list stack: %w", err) + stack := slices.Collect(graph.Stack(currentBranch)) + if len(stack) == 0 { + stack = []string{currentBranch} } toSubmit := stack[:0] for _, branch := range stack { @@ -60,5 +59,6 @@ func (cmd *stackSubmitCmd) Run( Branches: toSubmit, Options: &cmd.Options, BatchOptions: &cmd.BatchOptions, + BranchGraph: graph, }) } diff --git a/testdata/help/branch_submit.txt b/testdata/help/branch_submit.txt index 632a41719..73d34acf8 100644 --- a/testdata/help/branch_submit.txt +++ b/testdata/help/branch_submit.txt @@ -49,7 +49,6 @@ Flags: -a, --assign=ASSIGNEE,... Assign the change request to these users. Pass multiple times or separate with commas. --no-web Alias for --web=false. - --no-branch-check Skip stale base validation before submitting. --title=TITLE Title of the change request --body=BODY Body of the change request --branch=NAME Branch to submit diff --git a/testdata/help/downstack_submit.txt b/testdata/help/downstack_submit.txt index 1ff54f40f..956ed1ad7 100644 --- a/testdata/help/downstack_submit.txt +++ b/testdata/help/downstack_submit.txt @@ -50,7 +50,6 @@ Flags: -a, --assign=ASSIGNEE,... Assign the change request to these users. Pass multiple times or separate with commas. --no-web Alias for --web=false. - --no-branch-check Skip stale base validation before submitting. --branch=NAME Branch to start at Global Flags: diff --git a/testdata/help/stack_submit.txt b/testdata/help/stack_submit.txt index 069967a4f..1a76ad80b 100644 --- a/testdata/help/stack_submit.txt +++ b/testdata/help/stack_submit.txt @@ -49,7 +49,6 @@ Flags: -a, --assign=ASSIGNEE,... Assign the change request to these users. Pass multiple times or separate with commas. --no-web Alias for --web=false. - --no-branch-check Skip stale base validation before submitting. Global Flags: -h, --help Show help for the command diff --git a/testdata/help/upstack_submit.txt b/testdata/help/upstack_submit.txt index 2b4d269cf..fe39b137a 100644 --- a/testdata/help/upstack_submit.txt +++ b/testdata/help/upstack_submit.txt @@ -52,7 +52,6 @@ Flags: -a, --assign=ASSIGNEE,... Assign the change request to these users. Pass multiple times or separate with commas. --no-web Alias for --web=false. - --no-branch-check Skip stale base validation before submitting. --branch=NAME Branch to start at Global Flags: diff --git a/testdata/script/branch_submit_stale_base.txt b/testdata/script/branch_submit_stale_base.txt index 3307d3663..1cb53db24 100644 --- a/testdata/script/branch_submit_stale_base.txt +++ b/testdata/script/branch_submit_stale_base.txt @@ -33,12 +33,13 @@ shamhub merge alice/example 1 # branch submit should block: feature2's base is stale ! gs bs --fill -stderr 'stale base feature1' +stderr 'base=feature1' +stderr '1 branches with stale bases were found' stderr 'gs repo sync' -stderr 'no-branch-check' +stderr '--force' -# --no-branch-check bypasses the stale base check -gs bs --fill --no-branch-check +# --force bypasses the stale base check +gs bs --fill --force stderr '#2 is up-to-date' -- repo/feature1.txt -- diff --git a/testdata/script/downstack_submit_stale_base.txt b/testdata/script/downstack_submit_stale_base.txt index a8e2f97c2..2934953d5 100644 --- a/testdata/script/downstack_submit_stale_base.txt +++ b/testdata/script/downstack_submit_stale_base.txt @@ -33,12 +33,13 @@ shamhub merge alice/example 1 # downstack submit should block: feature2's base is stale ! gs dss --fill -stderr 'stale base feature1' +stderr 'base=feature1' +stderr '1 branches with stale bases were found' stderr 'gs repo sync' -stderr 'no-branch-check' +stderr '--force' -# --no-branch-check bypasses the stale base check -gs dss --fill --no-branch-check +# --force bypasses the stale base check +gs dss --fill --force stderr '#2 is up-to-date' -- repo/feature1.txt -- diff --git a/testdata/script/stack_submit_stale_base.txt b/testdata/script/stack_submit_stale_base.txt index 67fd5138f..3839cc83e 100644 --- a/testdata/script/stack_submit_stale_base.txt +++ b/testdata/script/stack_submit_stale_base.txt @@ -33,12 +33,13 @@ shamhub merge alice/example 1 # stack submit should block: feature2's base is stale ! gs stack submit --fill -stderr 'stale base feature1' +stderr 'base=feature1' +stderr '1 branches with stale bases were found' stderr 'gs repo sync' -stderr 'no-branch-check' +stderr '--force' -# --no-branch-check bypasses the stale base check -gs stack submit --fill --no-branch-check +# --force bypasses the stale base check +gs stack submit --fill --force stderr '#2 is up-to-date' -- repo/feature1.txt -- diff --git a/testdata/script/stale_base_deep_stack.txt b/testdata/script/stale_base_deep_stack.txt index 8169d4aea..9a991340e 100644 --- a/testdata/script/stale_base_deep_stack.txt +++ b/testdata/script/stale_base_deep_stack.txt @@ -37,12 +37,13 @@ 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 'stale base feature1' +stderr 'base=feature1' +stderr '1 branches with stale bases were found' stderr 'gs repo sync' -stderr 'no-branch-check' +stderr '--force' -# --no-branch-check bypasses the stale base check for submit -gs ss --fill --no-branch-check +# --force bypasses the stale base check for submit +gs ss --fill --force stderr '#3 is up-to-date' -- repo/feature1.txt -- diff --git a/testdata/script/stale_base_repo_sync_recovery.txt b/testdata/script/stale_base_repo_sync_recovery.txt index 692615081..2c72c0999 100644 --- a/testdata/script/stale_base_repo_sync_recovery.txt +++ b/testdata/script/stale_base_repo_sync_recovery.txt @@ -33,7 +33,8 @@ shamhub merge alice/example 1 # stack submit blocks: feature2's base is stale ! gs ss --fill -stderr 'stale base feature1' +stderr 'base=feature1' +stderr '1 branches with stale bases were found' stderr 'gs repo sync' # recover by syncing diff --git a/testdata/script/upstack_submit_stale_base.txt b/testdata/script/upstack_submit_stale_base.txt index 1083d65b8..f9d9e1356 100644 --- a/testdata/script/upstack_submit_stale_base.txt +++ b/testdata/script/upstack_submit_stale_base.txt @@ -34,12 +34,13 @@ shamhub merge alice/example 1 # upstack submit from feature2 should block: # feature2's downstack includes feature1 (stale). ! gs upstack submit --fill -stderr 'stale base feature1' +stderr 'base=feature1' +stderr '1 branches with stale bases were found' stderr 'gs repo sync' -stderr 'no-branch-check' +stderr '--force' -# --no-branch-check bypasses the stale base check -gs upstack submit --fill --no-branch-check +# --force bypasses the stale base check +gs upstack submit --fill --force stderr '#2 is up-to-date' -- repo/feature1.txt -- diff --git a/upstack_submit.go b/upstack_submit.go index 359700a29..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" @@ -36,7 +37,6 @@ func (cmd *upstackSubmitCmd) Run( wt *git.Worktree, store *state.Store, svc *spice.Service, - forgeRepo *optionalForgeRepository, submitHandler SubmitHandler, ) error { if cmd.Branch == "" { @@ -47,24 +47,18 @@ func (cmd *upstackSubmitCmd) Run( cmd.Branch = currentBranch } - if err := cmd.checkDownstack( - ctx, svc, forgeRepo.Repository, cmd.Branch, - ); err != nil { - return err + graph, err := svc.BranchGraph(ctx, nil) + if err != nil { + return fmt.Errorf("build branch graph: %w", err) } if cmd.Branch != store.Trunk() { - if err := cmd.verifyBaseSubmitted( - ctx, log, svc, store, cmd.Branch, - ); err != nil { + 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. @@ -78,33 +72,33 @@ func (cmd *upstackSubmitCmd) Run( Branches: upstacks, Options: &cmd.Options, BatchOptions: &cmd.BatchOptions, + BranchGraph: graph, }) } func (cmd *upstackSubmitCmd) verifyBaseSubmitted( - ctx context.Context, log *silog.Logger, - svc *spice.Service, + graph *spice.BranchGraph, store *state.Store, branch string, ) error { - b, err := svc.LookupBranch(ctx, branch) - if err != nil { - return fmt.Errorf("lookup branch %v: %w", branch, err) + 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, err := svc.LookupBranch(ctx, b.Base) - if err != nil { - return fmt.Errorf("lookup base %v: %w", b.Base, err) + 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) + log.Errorf("%v: base (%v) has not been submitted", branch, b.Base) return errors.New("submit the base branch first") } return nil