diff --git a/.changes/unreleased/Added-20260514-072440.yaml b/.changes/unreleased/Added-20260514-072440.yaml new file mode 100644 index 000000000..b1f4012ec --- /dev/null +++ b/.changes/unreleased/Added-20260514-072440.yaml @@ -0,0 +1,3 @@ +kind: Added +body: 'submodule: gs branch fold merges submodule associations across the folded and base branches; conflicts prompt interactively or accept ''--module-branch==''.' +time: 2026-05-14T07:24:40.075455-04:00 diff --git a/.changes/unreleased/Added-20260514-072626.yaml b/.changes/unreleased/Added-20260514-072626.yaml new file mode 100644 index 000000000..53da91b92 --- /dev/null +++ b/.changes/unreleased/Added-20260514-072626.yaml @@ -0,0 +1,3 @@ +kind: Added +body: 'submodule: gs branch checkout (and friends) now switch tracked submodules to their recorded branches transactionally — on conflict, parent and already-switched submodules roll back to their original HEADs.' +time: 2026-05-14T07:26:26.543871-04:00 diff --git a/.changes/unreleased/Added-20260514-072627.yaml b/.changes/unreleased/Added-20260514-072627.yaml new file mode 100644 index 000000000..693475267 --- /dev/null +++ b/.changes/unreleased/Added-20260514-072627.yaml @@ -0,0 +1,3 @@ +kind: Added +body: 'submodule: gs branch create inherits recorded submodule associations from the parent branch as a baseline.' +time: 2026-05-14T07:26:27.764587-04:00 diff --git a/branch_create.go b/branch_create.go index 72911bb2f..39a70e491 100644 --- a/branch_create.go +++ b/branch_create.go @@ -325,9 +325,12 @@ func (cmd *branchCreateCmd) Run( } // Record submodule associations if a commit was made. + // Inherit from the parent branch as a baseline so a fresh child + // is consistent with its parent's submodule pinning unless the + // user has explicitly moved a submodule. if cmd.Commit { - if err := submoduleTracker.RecordBranchState( - ctx, branchName, + if err := submoduleTracker.RecordWithInheritance( + ctx, branchName, cmd.Target, ); err != nil { log.Warn( "Could not record submodule associations", diff --git a/branch_fold.go b/branch_fold.go index c0d18216e..7a999089c 100644 --- a/branch_fold.go +++ b/branch_fold.go @@ -7,6 +7,7 @@ import ( "go.abhg.dev/gs/internal/git" "go.abhg.dev/gs/internal/handler/checkout" + "go.abhg.dev/gs/internal/handler/submodule" "go.abhg.dev/gs/internal/silog" "go.abhg.dev/gs/internal/spice" "go.abhg.dev/gs/internal/spice/state" @@ -15,7 +16,8 @@ import ( ) type branchFoldCmd struct { - Branch string `placeholder:"NAME" help:"Name of the branch" predictor:"trackedBranches"` + Branch string `placeholder:"NAME" help:"Name of the branch" predictor:"trackedBranches"` + ModuleBranch map[string]string `name:"module-branch" placeholder:"PATH=BRANCH" help:"Per-submodule branch override for fold conflicts (repeatable)"` } func (*branchFoldCmd) Help() string { @@ -38,6 +40,7 @@ func (cmd *branchFoldCmd) Run( store *state.Store, svc *spice.Service, checkoutHandler CheckoutHandler, + submoduleApplier SubmoduleApplier, ) error { if cmd.Branch == "" { currentBranch, err := wt.CurrentBranch(ctx) @@ -101,8 +104,74 @@ func (cmd *branchFoldCmd) Run( return fmt.Errorf("peel to commit: %w", err) } + // Merge submodule associations from child into base. + // The child branch is being folded away; its sub-branch records + // should win when they differ from the base's, but conflicting + // per-sub records require explicit resolution. + type submoduleApplierWithMerge interface { + MergeAssociationsForFold( + ctx context.Context, req submodule.MergeFoldRequest, + ) (map[string]string, error) + } + merge, _ := submoduleApplier.(submoduleApplierWithMerge) + var resolvedSubs map[string]string + if merge != nil && b.Base != store.Trunk() { + // Trunk is not tracked in the store; nothing to merge against. + var resolveFn func(submodule.FoldConflict) (string, error) + if ui.Interactive(view) { + resolveFn = func(c submodule.FoldConflict) (string, error) { + var pick string + prompt := ui.NewSelect[string](). + WithValue(&pick). + WithOptions( + ui.SelectOption[string]{ + Label: c.ChildBranch, + Value: c.ChildBranch, + }, + ui.SelectOption[string]{ + Label: c.BaseBranch, + Value: c.BaseBranch, + }, + ). + WithTitle(fmt.Sprintf( + "Submodule %s: pick branch for %s", + c.Path, b.Base)). + WithDescription(fmt.Sprintf( + "Folding %s (records %s) into %s (records %s)", + cmd.Branch, c.ChildBranch, + b.Base, c.BaseBranch)) + if err := ui.Run(view, prompt); err != nil { + return "", err + } + return pick, nil + } + } + var err error + resolvedSubs, err = merge.MergeAssociationsForFold(ctx, submodule.MergeFoldRequest{ + Base: b.Base, + Child: cmd.Branch, + ModuleBranch: cmd.ModuleBranch, + Resolve: resolveFn, + }) + if err != nil { + return fmt.Errorf("merge submodule associations: %w", err) + } + } + tx := store.BeginBranchTx() + // Persist the merged submodule associations onto the base. + if len(resolvedSubs) > 0 { + if err := tx.Upsert(ctx, state.UpsertRequest{ + Name: b.Base, + Submodules: resolvedSubs, + }); err != nil { + return fmt.Errorf( + "set submodule associations on %v: %w", b.Base, err, + ) + } + } + // Change the base of all branches above us // to the base of the branch we are folding. aboves, err := svc.ListAbove(ctx, cmd.Branch) diff --git a/branch_onto.go b/branch_onto.go index a6a50511a..5d022aaad 100644 --- a/branch_onto.go +++ b/branch_onto.go @@ -8,6 +8,7 @@ import ( "go.abhg.dev/gs/internal/cli" "go.abhg.dev/gs/internal/git" "go.abhg.dev/gs/internal/handler/onto" + "go.abhg.dev/gs/internal/silog" "go.abhg.dev/gs/internal/spice" "go.abhg.dev/gs/internal/spice/state" "go.abhg.dev/gs/internal/text" @@ -116,14 +117,43 @@ func (cmd *branchOntoCmd) AfterApply( func (cmd *branchOntoCmd) Run( ctx context.Context, + log *silog.Logger, + wt *git.Worktree, handler OntoHandler, + submoduleApplier SubmoduleApplier, ) error { - return handler.BranchOnto(ctx, &onto.BranchRequest{ + // Snapshot parent HEAD before the checkout so a submodule apply + // failure rolls back cleanly. branch onto bypasses checkout.Handler + // (its VerifyRestacked path is not appropriate post-BranchOnto), + // so we apply submodule associations directly after the handler runs. + parentSnap, snapErr := wt.SnapshotHead(ctx) + if snapErr != nil { + log.Warn("Could not snapshot HEAD before checkout; "+ + "submodule rollback disabled", + "error", snapErr) + } + + if err := handler.BranchOnto(ctx, &onto.BranchRequest{ Branch: cmd.Branch, Onto: cmd.Onto, Restack: cmd.Restack, ContinueCommand: cmd.continueCommand(), - }) + }); err != nil { + return err + } + + if err := submoduleApplier.ApplyAssociations(ctx, cmd.Branch); err != nil { + if parentSnap != nil { + if rerr := wt.RestoreHead(ctx, parentSnap); rerr != nil { + log.Warn("Parent rollback failed after submodule conflict", + "target", parentSnap.Hash, + "error", rerr) + } + } + return fmt.Errorf("apply submodules: %w", err) + } + + return nil } func (cmd *branchOntoCmd) continueCommand() []string { diff --git a/branch_submodule.go b/branch_submodule.go index f7e5613a4..e7aebdc86 100644 --- a/branch_submodule.go +++ b/branch_submodule.go @@ -10,10 +10,19 @@ import ( // for the current worktree state. type SubmoduleTracker interface { RecordBranchState(ctx context.Context, branch string) error + RecordWithInheritance(ctx context.Context, branch, parentBranch string) error } var _ SubmoduleTracker = (*submodule.Tracker)(nil) +// SubmoduleApplier switches tracked submodules to the branches +// recorded for a parent branch, transactionally. +type SubmoduleApplier interface { + ApplyAssociations(ctx context.Context, parentBranch string) error +} + +var _ SubmoduleApplier = (*submodule.Applier)(nil) + type branchSubmoduleCmd struct { List branchSubmoduleListCmd `cmd:"" aliases:"ls" help:"List submodule branch associations"` Repoint branchSubmoduleRepointCmd `cmd:"" help:"Change submodule branch association for the current branch"` diff --git a/doc/includes/cli-reference.md b/doc/includes/cli-reference.md index 574870fc1..8b1beb377 100644 --- a/doc/includes/cli-reference.md +++ b/doc/includes/cli-reference.md @@ -310,9 +310,11 @@ Before merging, the stack is checked for branches whose base PR was already merged on the forge. Use --no-branch-check to skip this validation. -Before each merge, waits for CI checks to pass. -Use --build-timeout to configure the maximum wait -before failing if checks are not ready. +Before each merge, waits for merge readiness: +the forge must observe the pushed head +and report that the CR is ready to merge. +Use --ready-timeout to configure the maximum wait +before failing if merge readiness is not reached. By default, a branch failure skips that branch's upstack descendants, but independent sibling branches continue. @@ -321,12 +323,12 @@ Use --fail-fast to stop the queue after the first branch failure. **Flags** * `--method=METHOD` ([:material-wrench:{ .middle title="spice.merge.method" }](/cli/config.md#spicemergemethod)): Preferred merge method. One of 'merge', 'squash', and 'rebase'. -* `--build-timeout=30m` ([:material-wrench:{ .middle title="spice.merge.buildTimeout" }](/cli/config.md#spicemergebuildtimeout)): Max time to wait for CI checks before each merge. 0 means check once. +* `--ready-timeout=30m` ([:material-wrench:{ .middle title="spice.merge.readyTimeout" }](/cli/config.md#spicemergereadytimeout)): Max time to wait for merge readiness before each merge. 0 means check once. * `--no-branch-check`: Skip stale base validation before merging. * `--fail-fast`: Stop the merge queue after the first branch failure. * `--branch=NAME`: Branch whose stack to merge -**Configuration**: [spice.merge.buildTimeout](/cli/config.md#spicemergebuildtimeout), [spice.merge.method](/cli/config.md#spicemergemethod) +**Configuration**: [spice.merge.method](/cli/config.md#spicemergemethod), [spice.merge.readyTimeout](/cli/config.md#spicemergereadytimeout) ### git-spice stack restack {#gs-stack-restack} @@ -624,7 +626,7 @@ This command acts as a local merge queue: it merges one Change Request, waits for that merge to finish, restacks and updates the next Change Request, -waits for its CI checks to pass, +waits for merge readiness on the updated Change Request, and then repeats the process. For a stack like this: @@ -642,13 +644,15 @@ Before merging, the downstack is checked for branches whose base PR was already merged on the forge. Use --no-branch-check to skip this validation. -Before each merge, waits for CI checks to pass. -Use --build-timeout to configure the maximum wait +Before each merge, waits for merge readiness: +the forge must observe the pushed head +and report that the CR is ready to merge. +Use --ready-timeout to configure the maximum wait (default: 30m, 0 means fail immediately if not ready). Between merges, the command waits for each merge to complete, restacks and updates the next PR, -waits for CI checks on the updated PR, +waits for merge readiness on the updated PR, and syncs merged branch cleanup. Use --no-wait for single branch merging @@ -658,12 +662,12 @@ when you don't want to wait for the merge to propagate. **Flags** * `--method=METHOD` ([:material-wrench:{ .middle title="spice.merge.method" }](/cli/config.md#spicemergemethod)): Preferred merge method. One of 'merge', 'squash', and 'rebase'. -* `--build-timeout=30m` ([:material-wrench:{ .middle title="spice.merge.buildTimeout" }](/cli/config.md#spicemergebuildtimeout)): Max time to wait for CI checks before each merge. 0 means check once. +* `--ready-timeout=30m` ([:material-wrench:{ .middle title="spice.merge.readyTimeout" }](/cli/config.md#spicemergereadytimeout)): Max time to wait for merge readiness before each merge. 0 means check once. * `--no-wait`: Skip polling for a single branch merge to propagate. * `--no-branch-check`: Skip stale base validation before merging. * `--branch=NAME`: Branch to start merging from -**Configuration**: [spice.merge.buildTimeout](/cli/config.md#spicemergebuildtimeout), [spice.merge.method](/cli/config.md#spicemergemethod) +**Configuration**: [spice.merge.method](/cli/config.md#spicemergemethod), [spice.merge.readyTimeout](/cli/config.md#spicemergereadytimeout) ### git-spice downstack edit {#gs-downstack-edit} @@ -914,6 +918,7 @@ Use the --branch flag to target a different branch. **Flags** * `--branch=NAME`: Name of the branch +* `--module-branch=PATH=BRANCH`: Per-submodule branch override for fold conflicts (repeatable) ### git-spice branch split {#gs-branch-split} @@ -1133,16 +1138,18 @@ Use --branch to merge a different branch. The branch must be based directly on trunk. To merge a stacked branch, use 'gs downstack merge'. -Before merging, waits for CI checks to pass. -Use --build-timeout to configure the maximum wait. +Before merging, waits for merge readiness: +the forge must observe the pushed head +and report that the CR is ready to merge. +Use --ready-timeout to configure the maximum wait. **Flags** * `--method=METHOD` ([:material-wrench:{ .middle title="spice.merge.method" }](/cli/config.md#spicemergemethod)): Preferred merge method. One of 'merge', 'squash', and 'rebase'. -* `--build-timeout=30m` ([:material-wrench:{ .middle title="spice.merge.buildTimeout" }](/cli/config.md#spicemergebuildtimeout)): Max time to wait for CI checks before each merge. 0 means check once. +* `--ready-timeout=30m` ([:material-wrench:{ .middle title="spice.merge.readyTimeout" }](/cli/config.md#spicemergereadytimeout)): Max time to wait for merge readiness before each merge. 0 means check once. * `--branch=NAME`: Branch to merge -**Configuration**: [spice.merge.buildTimeout](/cli/config.md#spicemergebuildtimeout), [spice.merge.method](/cli/config.md#spicemergemethod) +**Configuration**: [spice.merge.method](/cli/config.md#spicemergemethod), [spice.merge.readyTimeout](/cli/config.md#spicemergereadytimeout) ### git-spice branch submit {#gs-branch-submit} diff --git a/internal/handler/checkout/handler.go b/internal/handler/checkout/handler.go index 6108f636d..50d614824 100644 --- a/internal/handler/checkout/handler.go +++ b/internal/handler/checkout/handler.go @@ -17,7 +17,7 @@ import ( "go.abhg.dev/gs/internal/spice/state" ) -//go:generate mockgen -destination mocks_test.go -package checkout -typed . GitRepository,GitWorktree,TrackHandler,Service,Store +//go:generate mockgen -destination mocks_test.go -package checkout -typed . GitRepository,GitWorktree,TrackHandler,Service,Store,SubmoduleApplier // Options defines options for checking out a branch. // These turn into command line flags, so be mindful of what you add here. @@ -40,6 +40,14 @@ type Store interface { type GitWorktree interface { DetachHead(ctx context.Context, commitish string) error CheckoutBranch(ctx context.Context, branch string) error + SnapshotHead(ctx context.Context) (*git.HeadSnapshot, error) + RestoreHead(ctx context.Context, snap *git.HeadSnapshot) error +} + +// SubmoduleApplier switches tracked submodules to the branches +// recorded for a parent branch. +type SubmoduleApplier interface { + ApplyAssociations(ctx context.Context, parentBranch string) error } // GitRepository provides access to the Git repository methods @@ -63,13 +71,14 @@ type Service interface { // Handler provides a central place for handling checkout operations. type Handler struct { - Stdout io.Writer // required - Log *silog.Logger // required - Store Store // required - Repository GitRepository // required - Worktree GitWorktree // required - Track TrackHandler // required - Service Service // required + Stdout io.Writer // required + Log *silog.Logger // required + Store Store // required + Repository GitRepository // required + Worktree GitWorktree // required + Track TrackHandler // required + Service Service // required + Submodule SubmoduleApplier // optional; nil disables submodule auto-checkout } // Request is a request to checkout a branch. @@ -174,10 +183,37 @@ func (h *Handler) CheckoutBranch(ctx context.Context, req *Request) error { return nil } + // Snapshot parent HEAD before any state-changing op so we can + // roll back if the submodule apply fails downstream. + var parentSnap *git.HeadSnapshot + if h.Submodule != nil { + var err error + parentSnap, err = h.Worktree.SnapshotHead(ctx) + if err != nil { + // Snapshot failure is non-fatal — proceed without rollback. + log.Warn("Could not snapshot HEAD before checkout; "+ + "submodule rollback disabled", + "error", err) + } + } + if err := h.Worktree.CheckoutBranch(ctx, branch); err != nil { return fmt.Errorf("checkout branch: %w", err) } + if h.Submodule != nil { + if err := h.Submodule.ApplyAssociations(ctx, branch); err != nil { + if parentSnap != nil { + if rerr := h.Worktree.RestoreHead(ctx, parentSnap); rerr != nil { + log.Warn("Parent rollback failed after submodule conflict", + "target", parentSnap.Hash, + "error", rerr) + } + } + return fmt.Errorf("apply submodules: %w", err) + } + } + if opts.Verbose { log.Infof("switched to branch: %s", branch) } diff --git a/internal/handler/checkout/mocks_test.go b/internal/handler/checkout/mocks_test.go index 4e6329752..c4a75d425 100644 --- a/internal/handler/checkout/mocks_test.go +++ b/internal/handler/checkout/mocks_test.go @@ -1,9 +1,9 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: go.abhg.dev/gs/internal/handler/checkout (interfaces: GitRepository,GitWorktree,TrackHandler,Service,Store) +// Source: go.abhg.dev/gs/internal/handler/checkout (interfaces: GitRepository,GitWorktree,TrackHandler,Service,Store,SubmoduleApplier) // // Generated by this command: // -// mockgen -destination mocks_test.go -package checkout -typed . GitRepository,GitWorktree,TrackHandler,Service,Store +// mockgen -destination mocks_test.go -package checkout -typed . GitRepository,GitWorktree,TrackHandler,Service,Store,SubmoduleApplier // // Package checkout is a generated GoMock package. @@ -258,6 +258,83 @@ func (c *MockGitWorktreeDetachHeadCall) DoAndReturn(f func(context.Context, stri return c } +// RestoreHead mocks base method. +func (m *MockGitWorktree) RestoreHead(ctx context.Context, snap *git.HeadSnapshot) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RestoreHead", ctx, snap) + ret0, _ := ret[0].(error) + return ret0 +} + +// RestoreHead indicates an expected call of RestoreHead. +func (mr *MockGitWorktreeMockRecorder) RestoreHead(ctx, snap any) *MockGitWorktreeRestoreHeadCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RestoreHead", reflect.TypeOf((*MockGitWorktree)(nil).RestoreHead), ctx, snap) + return &MockGitWorktreeRestoreHeadCall{Call: call} +} + +// MockGitWorktreeRestoreHeadCall wrap *gomock.Call +type MockGitWorktreeRestoreHeadCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockGitWorktreeRestoreHeadCall) Return(arg0 error) *MockGitWorktreeRestoreHeadCall { + c.Call = c.Call.Return(arg0) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockGitWorktreeRestoreHeadCall) Do(f func(context.Context, *git.HeadSnapshot) error) *MockGitWorktreeRestoreHeadCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockGitWorktreeRestoreHeadCall) DoAndReturn(f func(context.Context, *git.HeadSnapshot) error) *MockGitWorktreeRestoreHeadCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// SnapshotHead mocks base method. +func (m *MockGitWorktree) SnapshotHead(ctx context.Context) (*git.HeadSnapshot, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SnapshotHead", ctx) + ret0, _ := ret[0].(*git.HeadSnapshot) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// SnapshotHead indicates an expected call of SnapshotHead. +func (mr *MockGitWorktreeMockRecorder) SnapshotHead(ctx any) *MockGitWorktreeSnapshotHeadCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SnapshotHead", reflect.TypeOf((*MockGitWorktree)(nil).SnapshotHead), ctx) + return &MockGitWorktreeSnapshotHeadCall{Call: call} +} + +// MockGitWorktreeSnapshotHeadCall wrap *gomock.Call +type MockGitWorktreeSnapshotHeadCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockGitWorktreeSnapshotHeadCall) Return(arg0 *git.HeadSnapshot, arg1 error) *MockGitWorktreeSnapshotHeadCall { + c.Call = c.Call.Return(arg0, arg1) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockGitWorktreeSnapshotHeadCall) Do(f func(context.Context) (*git.HeadSnapshot, error)) *MockGitWorktreeSnapshotHeadCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockGitWorktreeSnapshotHeadCall) DoAndReturn(f func(context.Context) (*git.HeadSnapshot, error)) *MockGitWorktreeSnapshotHeadCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + // MockTrackHandler is a mock of TrackHandler interface. type MockTrackHandler struct { ctrl *gomock.Controller @@ -482,3 +559,65 @@ func (c *MockStoreTrunkCall) DoAndReturn(f func() string) *MockStoreTrunkCall { c.Call = c.Call.DoAndReturn(f) return c } + +// MockSubmoduleApplier is a mock of SubmoduleApplier interface. +type MockSubmoduleApplier struct { + ctrl *gomock.Controller + recorder *MockSubmoduleApplierMockRecorder + isgomock struct{} +} + +// MockSubmoduleApplierMockRecorder is the mock recorder for MockSubmoduleApplier. +type MockSubmoduleApplierMockRecorder struct { + mock *MockSubmoduleApplier +} + +// NewMockSubmoduleApplier creates a new mock instance. +func NewMockSubmoduleApplier(ctrl *gomock.Controller) *MockSubmoduleApplier { + mock := &MockSubmoduleApplier{ctrl: ctrl} + mock.recorder = &MockSubmoduleApplierMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockSubmoduleApplier) EXPECT() *MockSubmoduleApplierMockRecorder { + return m.recorder +} + +// ApplyAssociations mocks base method. +func (m *MockSubmoduleApplier) ApplyAssociations(ctx context.Context, parentBranch string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ApplyAssociations", ctx, parentBranch) + ret0, _ := ret[0].(error) + return ret0 +} + +// ApplyAssociations indicates an expected call of ApplyAssociations. +func (mr *MockSubmoduleApplierMockRecorder) ApplyAssociations(ctx, parentBranch any) *MockSubmoduleApplierApplyAssociationsCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ApplyAssociations", reflect.TypeOf((*MockSubmoduleApplier)(nil).ApplyAssociations), ctx, parentBranch) + return &MockSubmoduleApplierApplyAssociationsCall{Call: call} +} + +// MockSubmoduleApplierApplyAssociationsCall wrap *gomock.Call +type MockSubmoduleApplierApplyAssociationsCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockSubmoduleApplierApplyAssociationsCall) Return(arg0 error) *MockSubmoduleApplierApplyAssociationsCall { + c.Call = c.Call.Return(arg0) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockSubmoduleApplierApplyAssociationsCall) Do(f func(context.Context, string) error) *MockSubmoduleApplierApplyAssociationsCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockSubmoduleApplierApplyAssociationsCall) DoAndReturn(f func(context.Context, string) error) *MockSubmoduleApplierApplyAssociationsCall { + c.Call = c.Call.DoAndReturn(f) + return c +} diff --git a/internal/handler/submodule/applier.go b/internal/handler/submodule/applier.go index 1399b7046..64d9087f4 100644 --- a/internal/handler/submodule/applier.go +++ b/internal/handler/submodule/applier.go @@ -162,3 +162,123 @@ func (a *Applier) rollback( func (a *Applier) isExcluded(path string) bool { return slices.Contains(a.Exclude, path) } + +// MergeFoldRequest specifies inputs for [Applier.MergeAssociationsForFold]. +type MergeFoldRequest struct { + // Base is the branch that survives the fold (the destination). + Base string + + // Child is the branch being folded away. + Child string + + // ModuleBranch is an optional map of submodule path -> branch name + // that pre-resolves specific conflicts without prompting. + // Typically set from a CLI flag like --module-branch=path=branch. + ModuleBranch map[string]string + + // Resolve, if non-nil, is called for each unresolved conflict. + // It receives the conflict description and must return the chosen + // branch name. + // + // If Resolve is nil and ModuleBranch does not cover a conflict, + // the merge returns a [FoldConflictError] listing all unresolved + // conflicts. + Resolve func(FoldConflict) (string, error) +} + +// MergeAssociationsForFold computes the submodule association map that +// should be recorded on req.Base after folding req.Child into it. +// +// Per-path resolution: +// - both branches record nothing → omit the path. +// - only base records → keep base's value. +// - only child records → adopt child's value (tip-state wins). +// - both record the same value → keep it. +// - both record different values → consult req.ModuleBranch[path]; +// if absent, call req.Resolve; if that is nil, accumulate into a +// [FoldConflictError] and return it after scanning all conflicts. +// +// The returned map is suitable for [state.UpsertRequest.Submodules]: +// keys absent from the map are left unchanged; empty-string values +// would delete recorded entries (not produced by this method). +func (a *Applier) MergeAssociationsForFold( + ctx context.Context, req MergeFoldRequest, +) (map[string]string, error) { + baseSubs := map[string]string{} + if resp, err := a.Store.LookupBranch(ctx, req.Base); err == nil { + baseSubs = resp.Submodules + } else if !errors.Is(err, state.ErrNotExist) { + return nil, fmt.Errorf( + "lookup base %s: %w", req.Base, err, + ) + } + + childSubs := map[string]string{} + if resp, err := a.Store.LookupBranch(ctx, req.Child); err == nil { + childSubs = resp.Submodules + } else if !errors.Is(err, state.ErrNotExist) { + return nil, fmt.Errorf( + "lookup child %s: %w", req.Child, err, + ) + } + + pathSet := make(map[string]struct{}, len(baseSubs)+len(childSubs)) + for p := range baseSubs { + pathSet[p] = struct{}{} + } + for p := range childSubs { + pathSet[p] = struct{}{} + } + paths := make([]string, 0, len(pathSet)) + for p := range pathSet { + paths = append(paths, p) + } + slices.Sort(paths) + + resolved := make(map[string]string, len(paths)) + var conflicts []FoldConflict + + for _, p := range paths { + bv, hasB := baseSubs[p] + cv, hasC := childSubs[p] + + switch { + case !hasB && !hasC: + // nothing recorded; skip. + case hasB && !hasC: + resolved[p] = bv + case !hasB && hasC: + resolved[p] = cv + case bv == cv: + resolved[p] = bv + default: + // True conflict: both record, values differ. + if v, ok := req.ModuleBranch[p]; ok { + resolved[p] = v + continue + } + if req.Resolve != nil { + v, err := req.Resolve(FoldConflict{ + Path: p, + BaseBranch: bv, + ChildBranch: cv, + }) + if err != nil { + return nil, err + } + resolved[p] = v + continue + } + conflicts = append(conflicts, FoldConflict{ + Path: p, + BaseBranch: bv, + ChildBranch: cv, + }) + } + } + + if len(conflicts) > 0 { + return nil, &FoldConflictError{Conflicts: conflicts} + } + return resolved, nil +} diff --git a/internal/handler/submodule/merge_fold_test.go b/internal/handler/submodule/merge_fold_test.go new file mode 100644 index 000000000..28fa0225b --- /dev/null +++ b/internal/handler/submodule/merge_fold_test.go @@ -0,0 +1,152 @@ +package submodule_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.abhg.dev/gs/internal/handler/submodule" + "go.abhg.dev/gs/internal/silog/silogtest" + "go.abhg.dev/gs/internal/spice/state" +) + +// fakeStore is a minimal in-memory implementation of submodule.ApplierStore. +type fakeStore struct { + branches map[string]map[string]string +} + +func (s *fakeStore) LookupBranch( + _ context.Context, name string, +) (*state.LookupResponse, error) { + subs, ok := s.branches[name] + if !ok { + return nil, state.ErrNotExist + } + return &state.LookupResponse{Submodules: subs}, nil +} + +func TestMergeAssociationsForFold_disjointBase(t *testing.T) { + t.Parallel() + store := &fakeStore{branches: map[string]map[string]string{ + "base": {"libs/core": "sub-core"}, + "child": {"libs/util": "sub-util"}, + }} + a := &submodule.Applier{Log: silogtest.New(t), Store: store} + + resolved, err := a.MergeAssociationsForFold(t.Context(), submodule.MergeFoldRequest{ + Base: "base", + Child: "child", + }) + require.NoError(t, err) + assert.Equal(t, map[string]string{ + "libs/core": "sub-core", + "libs/util": "sub-util", + }, resolved) +} + +func TestMergeAssociationsForFold_childWins(t *testing.T) { + t.Parallel() + store := &fakeStore{branches: map[string]map[string]string{ + "base": {}, + "child": {"libs/core": "feat-x"}, + }} + a := &submodule.Applier{Log: silogtest.New(t), Store: store} + + resolved, err := a.MergeAssociationsForFold(t.Context(), submodule.MergeFoldRequest{ + Base: "base", + Child: "child", + }) + require.NoError(t, err) + assert.Equal(t, "feat-x", resolved["libs/core"]) +} + +func TestMergeAssociationsForFold_sameValueKept(t *testing.T) { + t.Parallel() + store := &fakeStore{branches: map[string]map[string]string{ + "base": {"libs/core": "feat-x"}, + "child": {"libs/core": "feat-x"}, + }} + a := &submodule.Applier{Log: silogtest.New(t), Store: store} + + resolved, err := a.MergeAssociationsForFold(t.Context(), submodule.MergeFoldRequest{ + Base: "base", + Child: "child", + }) + require.NoError(t, err) + assert.Equal(t, "feat-x", resolved["libs/core"]) +} + +func TestMergeAssociationsForFold_conflictFlag(t *testing.T) { + t.Parallel() + store := &fakeStore{branches: map[string]map[string]string{ + "base": {"libs/core": "main"}, + "child": {"libs/core": "feat-x"}, + }} + a := &submodule.Applier{Log: silogtest.New(t), Store: store} + + resolved, err := a.MergeAssociationsForFold(t.Context(), submodule.MergeFoldRequest{ + Base: "base", + Child: "child", + ModuleBranch: map[string]string{ + "libs/core": "feat-x", + }, + }) + require.NoError(t, err) + assert.Equal(t, "feat-x", resolved["libs/core"]) +} + +func TestMergeAssociationsForFold_conflictPrompt(t *testing.T) { + t.Parallel() + store := &fakeStore{branches: map[string]map[string]string{ + "base": {"libs/core": "main"}, + "child": {"libs/core": "feat-x"}, + }} + a := &submodule.Applier{Log: silogtest.New(t), Store: store} + + var prompted submodule.FoldConflict + resolved, err := a.MergeAssociationsForFold(t.Context(), submodule.MergeFoldRequest{ + Base: "base", + Child: "child", + Resolve: func(c submodule.FoldConflict) (string, error) { + prompted = c + return c.ChildBranch, nil + }, + }) + require.NoError(t, err) + assert.Equal(t, "feat-x", resolved["libs/core"]) + assert.Equal(t, "libs/core", prompted.Path) + assert.Equal(t, "main", prompted.BaseBranch) + assert.Equal(t, "feat-x", prompted.ChildBranch) +} + +func TestMergeAssociationsForFold_conflictUnresolvedFails(t *testing.T) { + t.Parallel() + store := &fakeStore{branches: map[string]map[string]string{ + "base": {"libs/core": "main"}, + "child": {"libs/core": "feat-x"}, + }} + a := &submodule.Applier{Log: silogtest.New(t), Store: store} + + _, err := a.MergeAssociationsForFold(t.Context(), submodule.MergeFoldRequest{ + Base: "base", + Child: "child", + }) + var conflictErr *submodule.FoldConflictError + require.ErrorAs(t, err, &conflictErr) + require.Len(t, conflictErr.Conflicts, 1) + assert.Equal(t, "libs/core", conflictErr.Conflicts[0].Path) +} + +func TestMergeAssociationsForFold_emptyOnBothMissing(t *testing.T) { + t.Parallel() + store := &fakeStore{branches: map[string]map[string]string{}} + a := &submodule.Applier{Log: silogtest.New(t), Store: store} + + resolved, err := a.MergeAssociationsForFold(t.Context(), submodule.MergeFoldRequest{ + Base: "base", + Child: "child", + }) + require.NoError(t, err) + assert.Empty(t, resolved) +} diff --git a/internal/handler/submodule/tracker.go b/internal/handler/submodule/tracker.go index 27ff2e228..027802cba 100644 --- a/internal/handler/submodule/tracker.go +++ b/internal/handler/submodule/tracker.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "maps" "slices" "go.abhg.dev/gs/internal/git" @@ -37,6 +38,62 @@ func (t *Tracker) RecordBranchState( return RecordAssociations(ctx, t.Store, branch, assocs) } +// RecordWithInheritance records submodule associations for branch +// using parentBranch's recorded associations as a baseline, then +// overlaying the current state of each submodule. +// +// The resulting map gives current state precedence: if a submodule +// is on a different branch than what parentBranch records, the new +// branch records the current state. If a submodule has no association +// in the current worktree (e.g., detached HEAD), the parent's value +// is preserved. +// +// This makes `gs bc` create a child branch that is consistent with +// its parent's submodule pinning by default without an extra step. +func (t *Tracker) RecordWithInheritance( + ctx context.Context, branch, parentBranch string, +) error { + inherited := make(map[string]string) + if parentBranch != "" { + resp, err := t.Store.LookupBranch(ctx, parentBranch) + switch { + case err == nil: + maps.Copy(inherited, resp.Submodules) + case errors.Is(err, state.ErrNotExist): + // Parent not tracked (e.g., trunk): no baseline to inherit. + default: + return fmt.Errorf( + "lookup parent branch %s: %w", parentBranch, err, + ) + } + } + + current, err := t.ResolveAssociations(ctx) + if err != nil { + return fmt.Errorf( + "resolve submodule associations: %w", err, + ) + } + for _, a := range current { + inherited[a.Path] = a.Branch + } + + if len(inherited) == 0 { + return nil + } + + tx := t.Store.BeginBranchTx() + if err := tx.Upsert(ctx, state.UpsertRequest{ + Name: branch, + Submodules: inherited, + }); err != nil { + return err + } + return tx.Commit(ctx, + "record submodule associations (with inheritance)", + ) +} + // GitWorktree is the subset of [git.Worktree] // needed by the tracker. type GitWorktree interface { diff --git a/internal/handler/submodule/tracker_test.go b/internal/handler/submodule/tracker_test.go index 0a467250b..5c3af8942 100644 --- a/internal/handler/submodule/tracker_test.go +++ b/internal/handler/submodule/tracker_test.go @@ -212,3 +212,107 @@ func TestTracker_RecordBranchState_noSubmodules( // Ensure fakeWorktree implements the interface. var _ submodule.GitWorktree = (*fakeWorktree)(nil) + +func TestTracker_RecordWithInheritance_inheritsThenOverlays(t *testing.T) { + t.Parallel() + + ctx := t.Context() + db := storage.NewDB(make(storage.MapBackend)) + store, err := state.InitStore(ctx, state.InitStoreRequest{ + DB: db, + Trunk: "main", + }) + require.NoError(t, err) + + // Set up the parent branch with two recorded submodules. + tx := store.BeginBranchTx() + require.NoError(t, tx.Upsert(ctx, state.UpsertRequest{ + Name: "parent", + Base: "main", + Submodules: map[string]string{ + "libs/core": "feat-core", + "libs/util": "feat-util", + }, + })) + require.NoError(t, tx.Commit(ctx, "set up parent")) + + // Current worktree has libs/util on a different branch. + wt := &fakeWorktree{ + subs: []git.Submodule{ + {Path: "libs/core"}, + {Path: "libs/util"}, + }, + branches: map[string]string{ + "libs/core": "feat-core", + "libs/util": "feat-util-v2", + }, + } + + tracker := submodule.Tracker{ + Log: silog.Nop(), + Worktree: wt, + Store: store, + } + + // Pre-create the child branch. + tx = store.BeginBranchTx() + require.NoError(t, tx.Upsert(ctx, state.UpsertRequest{ + Name: "child", + Base: "parent", + })) + require.NoError(t, tx.Commit(ctx, "create child")) + + require.NoError(t, + tracker.RecordWithInheritance(ctx, "child", "parent"), + ) + + resp, err := store.LookupBranch(ctx, "child") + require.NoError(t, err) + assert.Equal(t, map[string]string{ + "libs/core": "feat-core", // inherited from parent + "libs/util": "feat-util-v2", // overlayed by current state + }, resp.Submodules) +} + +func TestTracker_RecordWithInheritance_noParentRecord(t *testing.T) { + t.Parallel() + + ctx := t.Context() + db := storage.NewDB(make(storage.MapBackend)) + store, err := state.InitStore(ctx, state.InitStoreRequest{ + DB: db, + Trunk: "main", + }) + require.NoError(t, err) + + wt := &fakeWorktree{ + subs: []git.Submodule{{Path: "libs/core"}}, + branches: map[string]string{ + "libs/core": "feat-core", + }, + } + + tracker := submodule.Tracker{ + Log: silog.Nop(), + Worktree: wt, + Store: store, + } + + tx := store.BeginBranchTx() + require.NoError(t, tx.Upsert(ctx, state.UpsertRequest{ + Name: "feature-x", + Base: "main", + })) + require.NoError(t, tx.Commit(ctx, "create branch")) + + // Parent "main" is not tracked in store, so no inheritance. + require.NoError(t, + tracker.RecordWithInheritance(ctx, "feature-x", "main"), + ) + + resp, err := store.LookupBranch(ctx, "feature-x") + require.NoError(t, err) + assert.Equal(t, map[string]string{ + "libs/core": "feat-core", + }, resp.Submodules) +} diff --git a/main.go b/main.go index 031dd5955..0a8920631 100644 --- a/main.go +++ b/main.go @@ -455,6 +455,7 @@ func (cmd *mainCmd) AfterApply( wt *git.Worktree, svc *spice.Service, trackHandler TrackHandler, + submoduleApplier SubmoduleApplier, ) (CheckoutHandler, error) { return &checkout.Handler{ Stdout: kctx.Stdout, @@ -464,6 +465,7 @@ func (cmd *mainCmd) AfterApply( Worktree: wt, Track: trackHandler, Service: svc, + Submodule: submoduleApplier, }, nil }), kctx.BindSingletonProvider(func( @@ -536,6 +538,23 @@ func (cmd *mainCmd) AfterApply( Exclude: exclude, }, nil }), + kctx.BindSingletonProvider(func( + log *silog.Logger, + wt *git.Worktree, + store *state.Store, + cfg *spice.Config, + ) (SubmoduleApplier, error) { + var exclude []string + if cfg != nil { + exclude = cfg.SubmoduleExclusions() + } + return &submodule.Applier{ + Log: log, + Worktree: wt, + Store: store, + Exclude: exclude, + }, nil + }), kctx.BindSingletonProvider(func( log *silog.Logger, store *state.Store, diff --git a/testdata/help/branch_fold.txt b/testdata/help/branch_fold.txt index f9b93d181..93aa6f2e7 100644 --- a/testdata/help/branch_fold.txt +++ b/testdata/help/branch_fold.txt @@ -9,7 +9,9 @@ branch downstack. Use the --branch flag to target a different branch. Flags: - --branch=NAME Name of the branch + --branch=NAME Name of the branch + --module-branch=PATH=BRANCH Per-submodule branch override for fold + conflicts (repeatable) Global Flags: -h, --help Show help for the command diff --git a/testdata/script/branch_create_submodule_inherits.txt b/testdata/script/branch_create_submodule_inherits.txt new file mode 100644 index 000000000..848b8d7e3 --- /dev/null +++ b/testdata/script/branch_create_submodule_inherits.txt @@ -0,0 +1,64 @@ +# 'gs branch create' inherits submodule associations from the parent +# branch as a baseline, then overlays the current submodule state. + +as 'Test ' +at '2026-03-11T15:00:00Z' + +# Set up a submodule repo with two branches. +cd sub-repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init +git add feat.txt +gs branch create sub-feat -m 'Add feat' +gs trunk +git add fix.txt +gs branch create sub-fix -m 'Add fix' + +cd $WORK/repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init + +# Add the submodule. +git -c protocol.file.allow=always submodule add $WORK/sub-repo libs/core +git commit -m 'Add submodule' + +# Switch sub to sub-feat; create parent branch feat-x. +cd libs/core +git checkout sub-feat +cd $WORK/repo +git add file1.txt +gs branch create feat-x -m 'Add file1' + +# Without explicitly changing the submodule, create child branch feat-y. +# The child should inherit libs/core:sub-feat from the parent. +git add file2.txt +gs branch create feat-y -m 'Add file2' + +gs branch submodule list +stderr 'libs/core' +stderr 'sub-feat' + +# Now flip the submodule to sub-fix and create another child branch. +cd libs/core +git checkout sub-fix +cd $WORK/repo +git add file3.txt +gs branch create feat-z -m 'Add file3' + +# feat-z should record the new state, not parent's value. +gs branch submodule list +stderr 'libs/core' +stderr 'sub-fix' + +-- sub-repo/feat.txt -- +sub feature +-- sub-repo/fix.txt -- +sub fix +-- repo/file1.txt -- +parent file 1 +-- repo/file2.txt -- +parent file 2 +-- repo/file3.txt -- +parent file 3 diff --git a/testdata/script/submodule_checkout_applies.txt b/testdata/script/submodule_checkout_applies.txt new file mode 100644 index 000000000..a786a7bb5 --- /dev/null +++ b/testdata/script/submodule_checkout_applies.txt @@ -0,0 +1,68 @@ +# 'gs branch checkout' (and friends) switches each tracked submodule +# to the branch recorded for the parent branch. + +as 'Test ' +at '2026-03-11T15:00:00Z' + +# Set up a submodule repo with two branches. +cd sub-repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init +git add feat.txt +gs branch create sub-feat -m 'Add feat' +gs trunk +git add fix.txt +gs branch create sub-fix -m 'Add fix' + +# Set up the parent repo. +cd $WORK/repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init + +# Add the submodule. +git -c protocol.file.allow=always submodule add $WORK/sub-repo libs/core +git commit -m 'Add submodule' + +# Create branch 'feat-x' while sub is on sub-feat. +cd libs/core +git checkout sub-feat +cd $WORK/repo +git add file1.txt +gs branch create feat-x -m 'Add file1' + +# Create branch 'feat-y' (from trunk) while sub is on sub-fix. +gs trunk +cd libs/core +git checkout sub-fix +cd $WORK/repo +git add file2.txt +gs branch create feat-y -m 'Add file2' + +# Verify associations are recorded. +gs branch submodule list +stderr 'libs/core' +stderr 'sub-fix' + +# Switch to feat-x and verify the submodule follows. +gs branch checkout feat-x +cd libs/core +git rev-parse --abbrev-ref HEAD +stdout 'sub-feat' +cd $WORK/repo + +# Switch to feat-y and verify the submodule follows. +gs branch checkout feat-y +cd libs/core +git rev-parse --abbrev-ref HEAD +stdout 'sub-fix' + +-- sub-repo/feat.txt -- +sub feature +-- sub-repo/fix.txt -- +sub fix +-- repo/file1.txt -- +parent file 1 +-- repo/file2.txt -- +parent file 2 diff --git a/testdata/script/submodule_fold_inherit.txt b/testdata/script/submodule_fold_inherit.txt new file mode 100644 index 000000000..be5670f50 --- /dev/null +++ b/testdata/script/submodule_fold_inherit.txt @@ -0,0 +1,57 @@ +# 'gs branch fold' merges submodule associations from the folded +# branch into its base. Disjoint associations carry over without +# prompts. Identical values are preserved. + +as 'Test ' +at '2026-03-11T15:00:00Z' + +# Set up a submodule repo. +cd sub-repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init +git add feat.txt +gs branch create sub-feat -m 'Add feat' + +cd $WORK/repo +git init +git commit --allow-empty -m 'Initial commit' +gs repo init + +# Add the submodule. +git -c protocol.file.allow=always submodule add $WORK/sub-repo libs/core +git commit -m 'Add submodule' + +# Switch sub to sub-feat and create child branch. +cd libs/core +git checkout sub-feat +cd $WORK/repo + +git add file1.txt +gs branch create feat-a -m 'Add file1' + +git add file2.txt +gs branch create feat-b -m 'Add file2' + +# Verify feat-a and feat-b both record libs/core:sub-feat. +gs branch submodule list -b feat-a +stderr 'libs/core' +stderr 'sub-feat' +gs branch submodule list -b feat-b +stderr 'libs/core' +stderr 'sub-feat' + +# Fold feat-b into feat-a. They record the same value: no prompt. +gs branch fold + +# feat-a should still record libs/core:sub-feat. +gs branch submodule list -b feat-a +stderr 'libs/core' +stderr 'sub-feat' + +-- sub-repo/feat.txt -- +sub feature +-- repo/file1.txt -- +parent file 1 +-- repo/file2.txt -- +parent file 2