diff --git a/cmd/rebase.go b/cmd/rebase.go index 9b79efdc..3b4d4827 100644 --- a/cmd/rebase.go +++ b/cmd/rebase.go @@ -36,6 +36,10 @@ type rebaseState struct { OntoOldBase string `json:"ontoOldBase,omitempty"` CommitterDateIsAuthorDate bool `json:"committerDateIsAuthorDate,omitempty"` NoTrunk bool `json:"noTrunk,omitempty"` + TrunkRef string `json:"trunkRef,omitempty"` + TrunkSHA string `json:"trunkSha,omitempty"` + StartIndex int `json:"startIndex,omitempty"` + EndIndex int `json:"endIndex,omitempty"` } const rebaseStateFile = "gh-stack-rebase-state" @@ -125,6 +129,7 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { return ErrSilent } + var trunk trunkTarget if !opts.noTrunk { // Resolve remote for fetch and trunk comparison remote, err := pickRemote(cfg, currentBranch, opts.remote) @@ -135,22 +140,16 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { return ErrSilent } - if err := git.Fetch(remote); err != nil { - cfg.Warningf("Failed to fetch %s: %v", remote, err) - } else { - cfg.Successf("Fetched %s", remote) + trunk, err = resolveTrunkTarget(cfg, s, remote, currentBranch) + if err != nil { + return err } - // Ensure trunk exists locally before fast-forward or cascade rebase. - if err := ensureLocalTrunk(cfg, s.Trunk.Branch, remote); err != nil { - cfg.Errorf("%s", err) + // Fast-forward stack branches that are behind their remote tracking branch. + if err := git.FetchBranches(remote, activeBranchNames(s)); err != nil { + cfg.Errorf("failed to fetch stack branches from %s: %v", remote, err) return ErrSilent } - - // Fast-forward trunk so the cascade rebase targets the latest upstream. - fastForwardTrunk(cfg, s.Trunk.Branch, remote, currentBranch) - - // Fast-forward stack branches that are behind their remote tracking branch. fastForwardBranches(cfg, s, remote, currentBranch) } @@ -222,10 +221,16 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { NeedsOnto: needsOnto, OntoOldBase: ontoOldBase, CommitterDateIsAuthorDate: opts.committerDateIsAuthorDate, + TrunkRef: trunk.Ref, }) if rebaseResult.Err != nil { cfg.Errorf("%v", rebaseResult.Err) + if rebaseResult.Rebased { + restoreRebaseRefs(cfg, currentBranch, originalRefs) + } else { + _ = git.CheckoutBranch(currentBranch) + } return ErrSilent } @@ -242,6 +247,10 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { OntoOldBase: rebaseResult.OntoOldBase, CommitterDateIsAuthorDate: opts.committerDateIsAuthorDate, NoTrunk: opts.noTrunk, + TrunkRef: trunk.Ref, + TrunkSHA: trunk.SHA, + StartIndex: startIdx, + EndIndex: endIdx, } if err := saveRebaseState(gitDir, state); err != nil { cfg.Warningf("failed to save rebase state: %s", err) @@ -259,6 +268,14 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { _ = git.CheckoutBranch(currentBranch) + if unstacked := verifyStacked(s, trunk.Ref, startIdx, endIdx); len(unstacked) > 0 { + reportUnstacked(cfg, trunk.Ref, unstacked) + if rebaseResult.Rebased { + restoreRebaseRefs(cfg, currentBranch, originalRefs) + } + return ErrSilent + } + updateBaseSHAs(s) _ = syncStackPRs(cfg, s) @@ -284,7 +301,7 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { if opts.noTrunk { cfg.Printf("%s rebased locally (without trunk)", rangeDesc) } else { - cfg.Printf("%s rebased locally with %s", rangeDesc, s.Trunk.Branch) + cfg.Printf("%s rebased locally with %s", rangeDesc, trunk.Describe()) } cfg.Printf("To push up your changes, run `%s`", cfg.ColorCyan("gh stack push")) @@ -314,6 +331,14 @@ func continueRebase(cfg *config.Config, gitDir string) error { if s == nil { return fmt.Errorf("no stack found for branch %s", state.OriginalBranch) } + trunkRef := state.TrunkRef + if trunkRef == "" { + trunkRef = s.Trunk.Branch + } + trunkBase := state.TrunkSHA + if trunkBase == "" { + trunkBase = trunkRef + } // Refresh PR state before selecting the base and cascading the remaining // branches. The queued flag is transient (not persisted), so it was lost @@ -343,7 +368,7 @@ func continueRebase(cfg *config.Config, gitDir string) error { var baseBranch string if state.UseOnto { // The --onto path targets the first non-merged ancestor, or trunk. - baseBranch = s.Trunk.Branch + baseBranch = trunkRef for j := state.CurrentBranchIndex - 1; j >= 0; j-- { if !s.Branches[j].IsMerged() { baseBranch = s.Branches[j].Branch @@ -353,7 +378,7 @@ func continueRebase(cfg *config.Config, gitDir string) error { } else if state.CurrentBranchIndex > 0 { baseBranch = s.Branches[state.CurrentBranchIndex-1].Branch } else { - baseBranch = s.Trunk.Branch + baseBranch = trunkRef } cfg.Successf("Rebased %s onto %s", conflictBranch, baseBranch) @@ -385,10 +410,13 @@ func continueRebase(cfg *config.Config, gitDir string) error { NeedsOnto: state.UseOnto, OntoOldBase: state.OntoOldBase, CommitterDateIsAuthorDate: state.CommitterDateIsAuthorDate, + TrunkRef: trunkBase, }) if result.Err != nil { cfg.Errorf("%v", result.Err) + restoreRebaseRefs(cfg, state.OriginalBranch, state.OriginalRefs) + clearRebaseState(gitDir) return ErrSilent } @@ -414,9 +442,23 @@ func continueRebase(cfg *config.Config, gitDir string) error { } } - clearRebaseState(gitDir) _ = git.CheckoutBranch(state.OriginalBranch) + verifyStart, verifyEnd := state.StartIndex, state.EndIndex + if verifyEnd <= verifyStart { + verifyStart, verifyEnd = 0, len(s.Branches) + if state.NoTrunk { + verifyStart = 1 + } + } + if unstacked := verifyStacked(s, trunkBase, verifyStart, verifyEnd); len(unstacked) > 0 { + reportUnstacked(cfg, trunkRef, unstacked) + restoreRebaseRefs(cfg, state.OriginalBranch, state.OriginalRefs) + clearRebaseState(gitDir) + return ErrSilent + } + + clearRebaseState(gitDir) updateBaseSHAs(s) _ = syncStackPRs(cfg, s) @@ -425,8 +467,10 @@ func continueRebase(cfg *config.Config, gitDir string) error { if state.NoTrunk { cfg.Printf("All branches in stack rebased locally (without trunk)") + } else if state.TrunkSHA != "" { + cfg.Printf("All branches in stack rebased locally with %s (%s)", trunkRef, short(state.TrunkSHA)) } else { - cfg.Printf("All branches in stack rebased locally with %s", s.Trunk.Branch) + cfg.Printf("All branches in stack rebased locally with %s", trunkRef) } cfg.Printf("To push up your changes and open/update the stack of PRs, run `%s`", cfg.ColorCyan("gh stack submit")) diff --git a/cmd/rebase_test.go b/cmd/rebase_test.go index 683bfe36..c8adc0a4 100644 --- a/cmd/rebase_test.go +++ b/cmd/rebase_test.go @@ -44,9 +44,9 @@ func newRebaseMock(tmpDir string, currentBranch string) *git.MockOps { } return "sha-" + ref, nil }, - IsAncestorFn: func(a, d string) (bool, error) { return true, nil }, - FetchFn: func(string) error { return nil }, - EnableRerereFn: func() error { return nil }, + IsAncestorFn: func(a, d string) (bool, error) { return true, nil }, + FetchFn: func(string) error { return nil }, + EnableRerereFn: func() error { return nil }, IsRebaseInProgressFn: func() bool { return false }, } } @@ -1293,11 +1293,7 @@ func TestRebase_FastForwardsBranchFromRemote(t *testing.T) { return "sha-" + ref, nil } mock.IsAncestorFn = func(a, d string) (bool, error) { - // b1-local is ancestor of b1-remote → can fast-forward - if a == "b1-local-sha" && d == "b1-remote-sha" { - return true, nil - } - return false, nil + return true, nil } mock.UpdateBranchRefFn = func(branch, sha string) error { updateBranchRefCalls = append(updateBranchRefCalls, struct{ branch, sha string }{branch, sha}) @@ -1415,7 +1411,11 @@ func TestRebase_BranchDiverged_NoFF(t *testing.T) { } // Neither is ancestor of the other — diverged mock.IsAncestorFn = func(a, d string) (bool, error) { - return false, nil + if (a == "b1-local-sha" && d == "b1-remote-sha") || + (a == "b1-remote-sha" && d == "b1-local-sha") { + return false, nil + } + return true, nil } mock.UpdateBranchRefFn = func(string, string) error { updateBranchRefCalls++ diff --git a/cmd/sync.go b/cmd/sync.go index 34bbbc05..c9aeda7b 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -107,9 +107,11 @@ func runSync(cfg *config.Config, opts *syncOptions) error { // Fetch trunk + active branches so tracking refs are current for // fast-forward detection (Step 2) and --force-with-lease (Step 4). - fetchTargets := append([]string{s.Trunk.Branch}, activeBranchNames(s)...) - _ = git.FetchBranches(remote, fetchTargets) - cfg.Successf("Fetched latest changes from %s", remote) + normalizeStackTrunk(cfg, s, remote) + if err := git.FetchBranches(remote, activeBranchNames(s)); err != nil { + cfg.Errorf("failed to fetch stack branches from %s: %v", remote, err) + return ErrSilent + } // --- Step 1b: Reconcile remote-ahead stack changes --- // Pull in branches for PRs that were added to the stack on GitHub, or @@ -139,19 +141,19 @@ func runSync(cfg *config.Config, opts *syncOptions) error { currentBranch = cb } - // --- Step 2: Fast-forward trunk --- - trunk := s.Trunk.Branch - trunkUpdated := fastForwardTrunk(cfg, trunk, remote, currentBranch) + // --- Step 2: Resolve trunk --- + trunk, err := resolveTrunkTarget(cfg, s, remote, currentBranch) + if err != nil { + return err + } // --- Step 2b: Fast-forward stack branches behind their remote tracking branch --- updatedBranches := fastForwardBranches(cfg, s, remote, currentBranch) - branchesUpdated := len(updatedBranches) > 0 // --- Step 3: Cascade rebase --- - // Rebase if trunk or any branch moved, or if the stack is stale - // (branches not yet rebased onto their parent's current tip). - needsRebase := trunkUpdated || branchesUpdated || stackNeedsRebase(s) + needsRebase := trunk.Moved || len(updatedBranches) > 0 || stackNeedsRebase(s, trunk.Ref) rebased := false + var originalRefs map[string]string if needsRebase { cfg.Printf("") cfg.Printf("Rebasing stack ...") @@ -159,7 +161,7 @@ func runSync(cfg *config.Config, opts *syncOptions) error { // Sync PR state to detect merged PRs before rebasing. _ = syncStackPRs(cfg, s) - originalRefs, err := resolveOriginalRefs(s) + originalRefs, err = resolveOriginalRefs(s) if err != nil { cfg.Warningf("Could not resolve branch SHAs — skipping rebase: %v", err) } else { @@ -169,11 +171,16 @@ func runSync(cfg *config.Config, opts *syncOptions) error { Branches: s.Branches, StartAbsIdx: 0, OriginalRefs: originalRefs, + TrunkRef: trunk.Ref, }) if result.Err != nil { cfg.Errorf("%v", result.Err) - _ = git.CheckoutBranch(currentBranch) + if result.Rebased { + restoreRebaseRefs(cfg, currentBranch, originalRefs) + } else { + _ = git.CheckoutBranch(currentBranch) + } stack.SaveNonBlocking(gitDir, sf) return ErrSilent } @@ -204,6 +211,16 @@ func runSync(cfg *config.Config, opts *syncOptions) error { _ = git.CheckoutBranch(currentBranch) } + if unstacked := verifyStacked(s, trunk.Ref, 0, len(s.Branches)); len(unstacked) > 0 { + _ = git.CheckoutBranch(currentBranch) + reportUnstacked(cfg, trunk.Ref, unstacked) + if rebased && originalRefs != nil { + restoreRebaseRefs(cfg, currentBranch, originalRefs) + } + stack.SaveNonBlocking(gitDir, sf) + return ErrSilent + } + // --- Step 4: Push --- cfg.Printf("") branches := activeBranchNames(s) @@ -329,7 +346,7 @@ func runSync(cfg *config.Config, opts *syncOptions) error { } } if needsSwitch { - switchTarget := trunk + switchTarget := trunk.Branch for _, b := range s.Branches { if !b.IsSkipped() { switchTarget = b.Branch @@ -385,6 +402,7 @@ func runSync(cfg *config.Config, opts *syncOptions) error { // unavailable, or a divergence). Report only what actually happened. cfg.Successf("Branches synced") } + cfg.Printf(" Stacked on %s", trunk.Describe()) return nil } @@ -392,6 +410,12 @@ func runSync(cfg *config.Config, opts *syncOptions) error { func restoreBranches(originalRefs map[string]string) []string { var errors []string for branch, sha := range originalRefs { + if !git.BranchExists(branch) { + continue + } + if currentSHA, err := git.RevParse(branch); err == nil && currentSHA == sha { + continue + } if err := git.CheckoutBranch(branch); err != nil { errors = append(errors, fmt.Sprintf("checkout %s: %s", branch, err)) continue @@ -403,6 +427,12 @@ func restoreBranches(originalRefs map[string]string) []string { return errors } +func restoreRebaseRefs(cfg *config.Config, originalBranch string, originalRefs map[string]string) { + restoreErrors := restoreBranches(originalRefs) + _ = git.CheckoutBranch(originalBranch) + reportRestoreStatus(cfg, restoreErrors) +} + // reportRestoreStatus prints whether branch restoration succeeded or partially failed. func reportRestoreStatus(cfg *config.Config, restoreErrors []string) { if len(restoreErrors) > 0 { diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 4f76d8e7..442aef59 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -139,10 +139,10 @@ func TestSync_TrunkUpToDate_StackStale(t *testing.T) { } return "sha-" + ref, nil } - // Stack branches are NOT rebased onto trunk — parent is not an ancestor. + // Stack branches are NOT rebased onto trunk until the cascade runs. + rebased := false mock.IsAncestorFn = func(a, d string) (bool, error) { - // main is NOT an ancestor of b1 → stack is stale - if a == "main" && d == "b1" { + if a == "main" && d == "b1" && !rebased { return false, nil } return true, nil @@ -150,10 +150,12 @@ func TestSync_TrunkUpToDate_StackStale(t *testing.T) { mock.CheckoutBranchFn = func(string) error { return nil } mock.RebaseFn = func(base string, opts git.RebaseOpts) error { rebaseCalls = append(rebaseCalls, rebaseCall{branch: "(rebase)" + base}) + rebased = true return nil } mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + rebased = true return nil } mock.PushFn = func(remote string, branches []string, force, atomic bool) error { @@ -298,10 +300,13 @@ func TestSync_TrunkFastForward_WhenOnTrunk(t *testing.T) { if ref == "origin/main" { return "remote-sha", nil } + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } return "sha-" + ref, nil } mock.IsAncestorFn = func(a, d string) (bool, error) { - return a == "local-sha" && d == "remote-sha", nil + return true, nil } mock.MergeFFFn = func(target string) error { mergeFFCalls = append(mergeFFCalls, target) @@ -470,6 +475,11 @@ func TestSync_RebaseConflict_RestoresAll(t *testing.T) { var checkouts []string currentBranch := "b1" abortCalled := false + branchSHAs := map[string]string{ + "b1": "sha-b1", + "b2": "sha-b2", + "b3": "sha-b3", + } mock := newSyncMock(tmpDir, "b1") mock.RevParseFn = func(ref string) (string, error) { @@ -479,6 +489,9 @@ func TestSync_RebaseConflict_RestoresAll(t *testing.T) { if ref == "origin/main" { return "remote-sha", nil } + if sha, ok := branchSHAs[ref]; ok { + return sha, nil + } return "sha-" + ref, nil } mock.IsAncestorFn = func(a, d string) (bool, error) { @@ -490,7 +503,10 @@ func TestSync_RebaseConflict_RestoresAll(t *testing.T) { currentBranch = name return nil } - mock.RebaseFn = func(string, git.RebaseOpts) error { return nil } // b1 succeeds + mock.RebaseFn = func(string, git.RebaseOpts) error { + branchSHAs["b1"] = "rebased-b1" + return nil + } mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { if branch == "b2" { return fmt.Errorf("conflict") @@ -503,6 +519,7 @@ func TestSync_RebaseConflict_RestoresAll(t *testing.T) { } mock.ResetHardFn = func(ref string) error { resets = append(resets, resetCall{currentBranch, ref}) + branchSHAs[currentBranch] = ref return nil } @@ -523,14 +540,15 @@ func TestSync_RebaseConflict_RestoresAll(t *testing.T) { assert.Contains(t, output, "Conflict detected") assert.Contains(t, output, "gh stack rebase") - // All branches should be restored + // The branch rewritten before the conflict should be restored. Unchanged + // branches are left alone. resetMap := make(map[string]string) for _, r := range resets { resetMap[r.branch] = r.sha } assert.Equal(t, "sha-b1", resetMap["b1"]) - assert.Equal(t, "sha-b2", resetMap["b2"]) - assert.Equal(t, "sha-b3", resetMap["b3"]) + assert.NotContains(t, resetMap, "b2") + assert.NotContains(t, resetMap, "b3") _ = abortCalled // RebaseAbort is called if IsRebaseInProgress returns true } @@ -625,7 +643,7 @@ func TestSync_PushForceFlagDependsOnRebase(t *testing.T) { return "sha-" + ref, nil } mock.IsAncestorFn = func(a, d string) (bool, error) { - return a == "local-sha" && d == "remote-sha", nil + return true, nil } mock.UpdateBranchRefFn = func(string, string) error { return nil } } else { @@ -938,7 +956,7 @@ func TestSync_PushFailureAfterRebase(t *testing.T) { return "sha-" + ref, nil } mock.IsAncestorFn = func(a, d string) (bool, error) { - return a == "local-sha" && d == "remote-sha", nil + return true, nil } mock.UpdateBranchRefFn = func(string, string) error { return nil } mock.CheckoutBranchFn = func(string) error { return nil } @@ -1006,10 +1024,7 @@ func TestSync_BranchFastForward_TriggersRebase(t *testing.T) { return "sha-" + ref, nil } mock.IsAncestorFn = func(a, d string) (bool, error) { - if a == "b1-local-sha" && d == "b1-remote-sha" { - return true, nil - } - return false, nil + return true, nil } mock.MergeFFFn = func(target string) error { mergeFFCalls = append(mergeFFCalls, target) @@ -1096,13 +1111,7 @@ func TestSync_BranchFastForward_WithTrunkUpdate(t *testing.T) { return "sha-" + ref, nil } mock.IsAncestorFn = func(a, d string) (bool, error) { - if a == "trunk-local" && d == "trunk-remote" { - return true, nil - } - if a == "b2-local" && d == "b2-remote" { - return true, nil - } - return false, nil + return true, nil } mock.UpdateBranchRefFn = func(branch, sha string) error { updateBranchRefCalls = append(updateBranchRefCalls, struct{ branch, sha string }{branch, sha}) diff --git a/cmd/trunk_target_test.go b/cmd/trunk_target_test.go new file mode 100644 index 00000000..ef864824 --- /dev/null +++ b/cmd/trunk_target_test.go @@ -0,0 +1,519 @@ +package cmd + +import ( + "errors" + "io" + "os" + "path/filepath" + "testing" + + "github.com/github/gh-stack/internal/config" + "github.com/github/gh-stack/internal/git" + "github.com/github/gh-stack/internal/stack" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func trunkTargetMock(localSHA, remoteSHA string) *git.MockOps { + return &git.MockOps{ + BranchExistsFn: func(string) bool { return true }, + RevParseFn: func(ref string) (string, error) { + switch ref { + case "main": + return localSHA, nil + case "origin/main": + return remoteSHA, nil + default: + return "sha-" + ref, nil + } + }, + } +} + +func TestNormalizeTrunkBranch(t *testing.T) { + t.Run("strips the selected remote prefix", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + BranchExistsFn: func(string) bool { return false }, + }) + defer restore() + + assert.Equal(t, "main", normalizeTrunkBranch("origin/main", "origin")) + }) + + t.Run("preserves a real local branch with the remote prefix", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + BranchExistsFn: func(name string) bool { return name == "origin/main" }, + }) + defer restore() + + assert.Equal(t, "origin/main", normalizeTrunkBranch("origin/main", "origin")) + }) +} + +func TestResolveTrunkTarget(t *testing.T) { + t.Run("normalizes a remote-qualified trunk before fetching", func(t *testing.T) { + mock := trunkTargetMock("same", "same") + mock.BranchExistsFn = func(name string) bool { return name == "main" } + var fetchedBranch string + mock.FetchBranchFn = func(remote, branch string) error { + assert.Equal(t, "origin", remote) + fetchedBranch = branch + return nil + } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + s := &stack.Stack{Trunk: stack.BranchRef{Branch: "origin/main"}} + target, err := resolveTrunkTarget(cfg, s, "origin", "b1") + + require.NoError(t, err) + assert.Equal(t, "main", fetchedBranch) + assert.Equal(t, "main", s.Trunk.Branch) + assert.Equal(t, "main", target.Ref) + }) + + t.Run("falls back to fetched remote ref when local trunk cannot move", func(t *testing.T) { + mock := trunkTargetMock("local", "remote") + mock.IsAncestorFn = func(a, d string) (bool, error) { + return a == "local" && d == "remote", nil + } + mock.UpdateBranchRefFn = func(string, string) error { + return errors.New("branch is checked out in another worktree") + } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + target, err := resolveTrunkTarget(cfg, &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + }, "origin", "b1") + + require.NoError(t, err) + assert.Equal(t, "origin/main", target.Ref) + assert.Equal(t, "remote", target.SHA) + }) + + t.Run("keeps local trunk when it contains fetched remote tip", func(t *testing.T) { + mock := trunkTargetMock("local-ahead", "remote") + mock.IsAncestorFn = func(a, d string) (bool, error) { + return a == "remote" && d == "local-ahead", nil + } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + target, err := resolveTrunkTarget(cfg, &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + }, "origin", "b1") + + require.NoError(t, err) + assert.Equal(t, "main", target.Ref) + assert.Equal(t, "local-ahead", target.SHA) + }) + + t.Run("uses intentional local-only trunk", func(t *testing.T) { + mock := trunkTargetMock("local", "") + mock.FetchBranchFn = func(string, string) error { + return git.ErrRemoteBranchNotFound + } + mock.UpstreamRemoteFn = func(string) (string, error) { return "", errors.New("unset") } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + target, err := resolveTrunkTarget(cfg, &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + }, "origin", "b1") + + require.NoError(t, err) + assert.Equal(t, "main", target.Ref) + assert.Equal(t, "local", target.SHA) + }) + + t.Run("fails when tracked trunk was deleted", func(t *testing.T) { + mock := trunkTargetMock("local", "") + mock.FetchBranchFn = func(string, string) error { + return git.ErrRemoteBranchNotFound + } + mock.UpstreamRemoteFn = func(string) (string, error) { return "origin", nil } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + _, err := resolveTrunkTarget(cfg, &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + }, "origin", "b1") + + assert.ErrorIs(t, err, ErrSilent) + }) + + t.Run("fails closed on transport error", func(t *testing.T) { + mock := trunkTargetMock("local", "cached") + mock.FetchBranchFn = func(string, string) error { + return errors.New("network unavailable") + } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + _, err := resolveTrunkTarget(cfg, &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + }, "origin", "b1") + + assert.ErrorIs(t, err, ErrSilent) + }) +} + +func TestVerifyStackedUsesResolvedTrunk(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}}, + } + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(a, d string) (bool, error) { + return a == "main" && d == "b1", nil + }, + }) + defer restore() + + assert.Empty(t, verifyStacked(s, "main", 0, 1)) + assert.Equal(t, []string{"b1"}, verifyStacked(s, "origin/main", 0, 1)) +} + +func TestVerifyStackedKeepsQueuedBranchAsParent(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1}}, + {Branch: "b2"}, + }, + } + s.Branches[0].Queued = true + + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(a, d string) (bool, error) { + return a == "b1" && d == "b2", nil + }, + }) + defer restore() + + assert.Empty(t, verifyStacked(s, "new-main", 0, 2), + "downstream branches remain stacked on queued branches while trunk moves") +} + +func TestRebase_FetchFailureStopsBeforeCascade(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}}, + }) + + rebaseCalls := 0 + mock := newRebaseMock(tmpDir, "b1") + mock.BranchExistsFn = func(string) bool { return true } + mock.FetchBranchFn = func(string, string) error { return errors.New("network unavailable") } + mock.RebaseFn = func(string, git.RebaseOpts) error { rebaseCalls++; return nil } + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + output, _ := io.ReadAll(errR) + assert.ErrorIs(t, err, ErrSilent) + assert.Zero(t, rebaseCalls) + assert.Contains(t, string(output), "failed to fetch trunk branch") + assert.NotContains(t, string(output), "rebased locally") +} + +func TestRebase_StartErrorDoesNotWriteRecoveryState(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}}, + }) + + mock := newRebaseMock(tmpDir, "b1") + mock.BranchExistsFn = func(string) bool { return true } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(string, git.RebaseOpts) error { + return &git.RebaseStartError{Err: errors.New("branch is checked out elsewhere")} + } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + assert.ErrorIs(t, err, ErrSilent) + _, statErr := os.Stat(filepath.Join(tmpDir, rebaseStateFile)) + assert.True(t, os.IsNotExist(statErr)) +} + +func TestRebase_LaterStartErrorRestoresEarlierBranches(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + }, + }) + + branchSHAs := map[string]string{"b1": "old-b1", "b2": "old-b2"} + currentBranch := "b1" + var resets []resetCall + + mock := newRebaseMock(tmpDir, currentBranch) + mock.BranchExistsFn = func(string) bool { return true } + mock.RevParseFn = func(ref string) (string, error) { + if ref == "main" || ref == "origin/main" { + return "trunk", nil + } + if sha, ok := branchSHAs[ref]; ok { + return sha, nil + } + if len(ref) > len("origin/") && ref[:len("origin/")] == "origin/" { + return branchSHAs[ref[len("origin/"):]], nil + } + return "sha-" + ref, nil + } + mock.CheckoutBranchFn = func(branch string) error { + currentBranch = branch + return nil + } + mock.RebaseFn = func(string, git.RebaseOpts) error { + branchSHAs["b1"] = "rebased-b1" + return nil + } + mock.RebaseOntoFn = func(string, string, string, git.RebaseOpts) error { + return &git.RebaseStartError{Err: errors.New("branch is checked out elsewhere")} + } + mock.ResetHardFn = func(ref string) error { + resets = append(resets, resetCall{currentBranch, ref}) + branchSHAs[currentBranch] = ref + return nil + } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + assert.ErrorIs(t, err, ErrSilent) + assert.Equal(t, "old-b1", branchSHAs["b1"]) + assert.Equal(t, "old-b2", branchSHAs["b2"]) + assert.Equal(t, []resetCall{{branch: "b1", sha: "old-b1"}}, resets) +} + +func TestSync_LaterStartErrorRestoresEarlierBranches(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + }, + }) + + branchSHAs := map[string]string{"b1": "old-b1", "b2": "old-b2"} + currentBranch := "b1" + pushes := 0 + + mock := newSyncMock(tmpDir, currentBranch) + mock.RevParseFn = func(ref string) (string, error) { + if ref == "main" || ref == "origin/main" { + return "trunk", nil + } + if sha, ok := branchSHAs[ref]; ok { + return sha, nil + } + if len(ref) > len("origin/") && ref[:len("origin/")] == "origin/" { + return branchSHAs[ref[len("origin/"):]], nil + } + return "sha-" + ref, nil + } + stacked := false + mock.IsAncestorFn = func(a, d string) (bool, error) { + if a == "main" && d == "b1" { + return stacked, nil + } + return true, nil + } + mock.CheckoutBranchFn = func(branch string) error { + currentBranch = branch + return nil + } + mock.RebaseFn = func(string, git.RebaseOpts) error { + branchSHAs["b1"] = "rebased-b1" + stacked = true + return nil + } + mock.RebaseOntoFn = func(string, string, string, git.RebaseOpts) error { + return &git.RebaseStartError{Err: errors.New("branch is checked out elsewhere")} + } + mock.ResetHardFn = func(ref string) error { + branchSHAs[currentBranch] = ref + return nil + } + mock.PushFn = func(string, []string, bool, bool) error { + pushes++ + return nil + } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + cmd := SyncCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + assert.ErrorIs(t, err, ErrSilent) + assert.Equal(t, "old-b1", branchSHAs["b1"]) + assert.Equal(t, "old-b2", branchSHAs["b2"]) + assert.Zero(t, pushes) +} + +func TestRebase_ContinueVerificationFailureRestoresAndClearsState(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + }) + + state := &rebaseState{ + CurrentBranchIndex: 1, + ConflictBranch: "b2", + RemainingBranches: []string{"b3"}, + OriginalBranch: "b1", + OriginalRefs: map[string]string{ + "b1": "old-b1", + "b2": "old-b2", + "b3": "old-b3", + }, + TrunkRef: "main", + TrunkSHA: "trunk", + StartIndex: 0, + EndIndex: 3, + } + require.NoError(t, saveRebaseState(tmpDir, state)) + + branchSHAs := map[string]string{"b1": "old-b1", "b2": "old-b2", "b3": "old-b3"} + currentBranch := "b2" + rebaseInProgress := true + cascadeDone := false + + mock := newRebaseMock(tmpDir, currentBranch) + mock.BranchExistsFn = func(string) bool { return true } + mock.RevParseFn = func(ref string) (string, error) { + if sha, ok := branchSHAs[ref]; ok { + return sha, nil + } + return "sha-" + ref, nil + } + mock.IsRebaseInProgressFn = func() bool { return rebaseInProgress } + mock.RebaseContinueFn = func(git.RebaseOpts) error { + rebaseInProgress = false + branchSHAs["b2"] = "rebased-b2" + return nil + } + mock.IsAncestorFn = func(a, d string) (bool, error) { + if a == "b2" && d == "b3" { + return !cascadeDone, nil + } + return true, nil + } + mock.RebaseOntoFn = func(string, string, string, git.RebaseOpts) error { + cascadeDone = true + branchSHAs["b3"] = "rebased-b3" + return nil + } + mock.CheckoutBranchFn = func(branch string) error { + currentBranch = branch + return nil + } + mock.ResetHardFn = func(ref string) error { + branchSHAs[currentBranch] = ref + return nil + } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetArgs([]string{"--continue"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + assert.ErrorIs(t, err, ErrSilent) + assert.Equal(t, "old-b2", branchSHAs["b2"]) + assert.Equal(t, "old-b3", branchSHAs["b3"]) + _, statErr := os.Stat(filepath.Join(tmpDir, rebaseStateFile)) + assert.True(t, os.IsNotExist(statErr), "terminal verification failure must clear stale continuation state") +} + +func TestSync_UnstackedCascadeDoesNotPush(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + }, + }) + + pushes := 0 + mock := newSyncMock(tmpDir, "b1") + mock.RevParseFn = func(ref string) (string, error) { + switch ref { + case "main": + return "local", nil + case "origin/main": + return "remote", nil + default: + return "sha-" + ref, nil + } + } + mock.IsAncestorFn = func(a, d string) (bool, error) { + if a == "local" && d == "remote" { + return true, nil + } + if a == "main" && d == "b1" { + return false, nil + } + return true, nil + } + mock.UpdateBranchRefFn = func(string, string) error { return nil } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(string, git.RebaseOpts) error { return nil } + mock.RebaseOntoFn = func(string, string, string, git.RebaseOpts) error { return nil } + mock.PushFn = func(string, []string, bool, bool) error { pushes++; return nil } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + cmd := SyncCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + assert.ErrorIs(t, err, ErrSilent) + assert.Zero(t, pushes) +} diff --git a/cmd/utils.go b/cmd/utils.go index 1c5dc139..514bf080 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -909,57 +909,125 @@ func ensureLocalTrunk(cfg *config.Config, trunk, remote string) error { return nil } -// fastForwardTrunk fast-forwards the trunk branch to match its remote tracking -// branch. Returns true if trunk was updated. -func fastForwardTrunk(cfg *config.Config, trunk, remote, currentBranch string) bool { - // If the local trunk branch doesn't exist, there's nothing to - // fast-forward. Callers should use ensureLocalTrunk beforehand if - // they need trunk to be resolvable as a local ref. - if !git.BranchExists(trunk) { - return false +func normalizeTrunkBranch(trunk, remote string) string { + if remote == "" || git.BranchExists(trunk) { + return trunk } + if stripped, ok := strings.CutPrefix(trunk, remote+"/"); ok && stripped != "" { + return stripped + } + return trunk +} - localSHA, remoteSHA := "", "" - trunkRefs, trunkErr := git.RevParseMulti([]string{trunk, remote + "/" + trunk}) - if trunkErr == nil { - localSHA, remoteSHA = trunkRefs[0], trunkRefs[1] +func normalizeStackTrunk(cfg *config.Config, s *stack.Stack, remote string) { + trunk := normalizeTrunkBranch(s.Trunk.Branch, remote) + if trunk == s.Trunk.Branch { + return } + cfg.Warningf("Stack trunk %q is remote-qualified — using %q", s.Trunk.Branch, trunk) + s.Trunk.Branch = trunk +} - if trunkErr != nil { - cfg.Warningf("Could not compare trunk %s with remote — skipping trunk update", trunk) - return false +type trunkTarget struct { + Branch string + Ref string + SHA string + Moved bool +} + +func (t trunkTarget) Describe() string { + return fmt.Sprintf("%s (%s)", t.Ref, short(t.SHA)) +} + +// resolveTrunkTarget fetches the trunk explicitly, then returns the ref the +// cascade must use. Updating the local trunk is best-effort; the fetched remote +// ref remains the source of truth when the local branch is stale or immovable. +func resolveTrunkTarget(cfg *config.Config, s *stack.Stack, remote, currentBranch string) (trunkTarget, error) { + normalizeStackTrunk(cfg, s, remote) + trunk := s.Trunk.Branch + remoteRef := remote + "/" + trunk + + if err := git.FetchBranch(remote, trunk); err != nil { + if errors.Is(err, git.ErrRemoteBranchNotFound) { + return trunkWithoutRemote(cfg, trunk, remote) + } + cfg.Errorf("failed to fetch trunk branch %s from %s: %v", trunk, remote, err) + return trunkTarget{}, ErrSilent } - if localSHA == remoteSHA { - cfg.Successf("Trunk %s is already up to date", trunk) - return false + remoteSHA, err := git.RevParse(remoteRef) + if err != nil { + cfg.Errorf("could not resolve fetched trunk %s: %v", remoteRef, err) + return trunkTarget{}, ErrSilent + } + cfg.Successf("Fetched latest %s from %s", trunk, remote) + + if !git.BranchExists(trunk) { + if err := git.CreateBranch(trunk, remoteRef); err != nil { + cfg.Errorf("could not create local trunk branch %s from %s: %v", trunk, remoteRef, err) + return trunkTarget{}, ErrSilent + } + cfg.Successf("Created local trunk branch %s from %s", trunk, remoteRef) + return trunkTarget{Branch: trunk, Ref: trunk, SHA: remoteSHA, Moved: true}, nil } - isAncestor, err := git.IsAncestor(localSHA, remoteSHA) + localSHA, err := git.RevParse(trunk) if err != nil { - cfg.Warningf("Could not determine fast-forward status for %s: %v", trunk, err) - return false + cfg.Errorf("could not resolve local trunk branch %s: %v", trunk, err) + return trunkTarget{}, ErrSilent } - if !isAncestor { - cfg.Warningf("Trunk %s has diverged from %s — skipping trunk update", trunk, remote) - cfg.Printf(" Local and remote %s have diverged. Resolve manually.", trunk) - return false + if localSHA == remoteSHA { + cfg.Successf("Trunk %s is already up to date", trunk) + return trunkTarget{Branch: trunk, Ref: trunk, SHA: localSHA}, nil } - if currentBranch == trunk { - if err := git.MergeFF(remote + "/" + trunk); err != nil { - cfg.Warningf("Failed to fast-forward %s: %v", trunk, err) - return false - } + canFastForward, ffErr := git.IsAncestor(localSHA, remoteSHA) + if ffErr == nil && canFastForward { + var updateErr error + if currentBranch == trunk { + updateErr = git.MergeFF(remoteRef) + } else { + updateErr = git.UpdateBranchRef(trunk, remoteSHA) + } + if updateErr == nil { + cfg.Successf("Trunk %s fast-forwarded to %s", trunk, short(remoteSHA)) + return trunkTarget{Branch: trunk, Ref: trunk, SHA: remoteSHA, Moved: true}, nil + } + cfg.Warningf("Could not update local %s: %v", trunk, updateErr) + } else if ffErr != nil { + cfg.Warningf("Could not determine fast-forward status for %s: %v", trunk, ffErr) + } else if isAncestor, ancErr := git.IsAncestor(remoteSHA, localSHA); ancErr == nil && isAncestor { + // Keep unpushed local trunk commits when they already contain the + // fetched remote tip. + cfg.Successf("Trunk %s is ahead of %s — using the local branch", trunk, remoteRef) + return trunkTarget{Branch: trunk, Ref: trunk, SHA: localSHA}, nil } else { - if err := git.UpdateBranchRef(trunk, remoteSHA); err != nil { - cfg.Warningf("Failed to fast-forward %s: %v", trunk, err) - return false - } + cfg.Warningf("Local %s has diverged from %s", trunk, remoteRef) } - cfg.Successf("Trunk %s fast-forwarded to %s", trunk, short(remoteSHA)) - return true + cfg.Printf(" Rebasing the stack onto %s instead; local %s is unchanged.", remoteRef, trunk) + return trunkTarget{Branch: trunk, Ref: remoteRef, SHA: remoteSHA}, nil +} + +func trunkWithoutRemote(cfg *config.Config, trunk, remote string) (trunkTarget, error) { + if !git.BranchExists(trunk) { + cfg.Errorf("trunk branch %s exists neither locally nor on %s", trunk, remote) + return trunkTarget{}, ErrSilent + } + if trackedRemote, err := git.UpstreamRemote(trunk); err == nil && trackedRemote != "" { + cfg.Errorf("%s no longer exists on %s", trunk, remote) + cfg.Printf(" Re-point the stack at an existing trunk branch, or use `%s`.", + cfg.ColorCyan("gh stack rebase --no-trunk")) + return trunkTarget{}, ErrSilent + } + + localSHA, err := git.RevParse(trunk) + if err != nil { + cfg.Errorf("could not resolve local trunk branch %s: %v", trunk, err) + return trunkTarget{}, ErrSilent + } + cfg.Warningf("Trunk %s only exists locally — %s has no such branch", trunk, remote) + return trunkTarget{Branch: trunk, Ref: trunk, SHA: localSHA}, nil } // cascadeRebaseOpts holds parameters for a cascade rebase across a range of @@ -973,6 +1041,14 @@ type cascadeRebaseOpts struct { NeedsOnto bool OntoOldBase string CommitterDateIsAuthorDate bool + TrunkRef string +} + +func (o cascadeRebaseOpts) trunkRef() string { + if o.TrunkRef != "" { + return o.TrunkRef + } + return o.Stack.Trunk.Branch } // cascadeRebaseResult describes the outcome of a cascade rebase. @@ -999,13 +1075,14 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { originalRefs := opts.OriginalRefs result := cascadeRebaseResult{} rebaseOpts := git.RebaseOpts{CommitterDateIsAuthorDate: opts.CommitterDateIsAuthorDate} + trunkRef := opts.trunkRef() for i, br := range opts.Branches { absIdx := opts.StartAbsIdx + i var base string if absIdx == 0 { - base = s.Trunk.Branch + base = trunkRef } else { base = s.Branches[absIdx-1].Branch } @@ -1034,7 +1111,7 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { if needsOnto { // Find --onto target: first non-merged ancestor, or trunk. Queued // ancestors keep their commits, so they are valid --onto targets. - newBase := s.Trunk.Branch + newBase := trunkRef for j := absIdx - 1; j >= 0; j-- { if !s.Branches[j].IsMerged() { newBase = s.Branches[j].Branch @@ -1054,6 +1131,12 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { } if err := git.RebaseOnto(newBase, actualOldBase, br.Branch, rebaseOpts); err != nil { + if git.IsRebaseStartError(err) { + return cascadeRebaseResult{ + Rebased: result.Rebased, + Err: fmt.Errorf("could not start rebase of %s onto %s: %w", br.Branch, newBase, err), + } + } remaining := make([]string, 0, len(opts.Branches)-i-1) for j := i + 1; j < len(opts.Branches); j++ { remaining = append(remaining, opts.Branches[j].Branch) @@ -1088,6 +1171,12 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { } if rebaseErr != nil { + if git.IsRebaseStartError(rebaseErr) { + return cascadeRebaseResult{ + Rebased: result.Rebased, + Err: fmt.Errorf("could not start rebase of %s onto %s: %w", br.Branch, base, rebaseErr), + } + } remaining := make([]string, 0, len(opts.Branches)-i-1) for j := i + 1; j < len(opts.Branches); j++ { remaining = append(remaining, opts.Branches[j].Branch) @@ -1112,29 +1201,48 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { return result } -// stackNeedsRebase returns true if any active branch in the stack is not based -// on its parent's current tip. This detects when the stack needs rebasing even -// if trunk was not updated in the current run. -func stackNeedsRebase(s *stack.Stack) bool { - trunk := s.Trunk.Branch - for i, br := range s.Branches { +// verifyStacked returns active branches in the requested range that do not +// contain their effective parent. +func verifyStacked(s *stack.Stack, trunkRef string, startIdx, endIdx int) []string { + if trunkRef == "" { + trunkRef = s.Trunk.Branch + } + if startIdx < 0 { + startIdx = 0 + } + if endIdx > len(s.Branches) { + endIdx = len(s.Branches) + } + + var unstacked []string + for i := startIdx; i < endIdx; i++ { + br := s.Branches[i] if br.IsSkipped() { continue } - // Find the nearest non-skipped parent. - parent := trunk + parent := trunkRef for j := i - 1; j >= 0; j-- { - if !s.Branches[j].IsSkipped() { + if !s.Branches[j].IsMerged() { parent = s.Branches[j].Branch break } } isAnc, err := git.IsAncestor(parent, br.Branch) if err != nil || !isAnc { - return true + unstacked = append(unstacked, br.Branch) } } - return false + return unstacked +} + +func stackNeedsRebase(s *stack.Stack, trunkRef string) bool { + return len(verifyStacked(s, trunkRef, 0, len(s.Branches))) > 0 +} + +func reportUnstacked(cfg *config.Config, trunkRef string, unstacked []string) { + cfg.Errorf("rebase did not leave these branches on their expected parents: %s", + strings.Join(unstacked, ", ")) + cfg.Printf(" Trunk target: %s", trunkRef) } // resolvePR resolves a user-provided target to a stack and branch using diff --git a/cmd/utils_test.go b/cmd/utils_test.go index feb89fc9..d95e3591 100644 --- a/cmd/utils_test.go +++ b/cmd/utils_test.go @@ -682,7 +682,7 @@ func TestStackNeedsRebase_AllCurrent(t *testing.T) { restore := git.SetOps(mock) defer restore() - assert.False(t, stackNeedsRebase(s), "stack should not need rebase when all branches are current") + assert.False(t, stackNeedsRebase(s, ""), "stack should not need rebase when all branches are current") } func TestStackNeedsRebase_FirstBranchStale(t *testing.T) { @@ -705,7 +705,7 @@ func TestStackNeedsRebase_FirstBranchStale(t *testing.T) { restore := git.SetOps(mock) defer restore() - assert.True(t, stackNeedsRebase(s), "stack should need rebase when first branch is stale") + assert.True(t, stackNeedsRebase(s, ""), "stack should need rebase when first branch is stale") } func TestStackNeedsRebase_SkipsMergedBranches(t *testing.T) { @@ -725,7 +725,7 @@ func TestStackNeedsRebase_SkipsMergedBranches(t *testing.T) { restore := git.SetOps(mock) defer restore() - assert.False(t, stackNeedsRebase(s), "should skip merged branches and find stack up to date") + assert.False(t, stackNeedsRebase(s, ""), "should skip merged branches and find stack up to date") } // setTestRepo sets RepoOverride so tests don't depend on real git context. diff --git a/internal/git/git.go b/internal/git/git.go index 678d13f8..b8c43e4e 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -2,6 +2,7 @@ package git import ( "context" + "errors" "fmt" "os" "os/exec" @@ -64,6 +65,40 @@ func runInteractive(args ...string) error { return cmd.Run() } +// RebaseStartError indicates that git rejected a rebase before creating any +// rebase state. There is nothing to continue or abort in this case. +type RebaseStartError struct { + Err error +} + +func (e *RebaseStartError) Error() string { + return e.Err.Error() +} + +func (e *RebaseStartError) Unwrap() error { + return e.Err +} + +func IsRebaseStartError(err error) bool { + var startErr *RebaseStartError + return errors.As(err, &startErr) +} + +func runRebaseCommand(args []string, opts RebaseOpts) error { + if IsRebaseInProgress() { + return &RebaseStartError{Err: errors.New("a rebase is already in progress")} + } + err := runSilent(args...) + if err == nil { + return nil + } + err = tryAutoResolveRebase(err, opts) + if err != nil && !IsRebaseInProgress() { + return &RebaseStartError{Err: err} + } + return err +} + // rebaseContinueOnce runs a single git rebase --continue without auto-resolve. func rebaseContinueOnce(opts RebaseOpts) error { args := []string{"rebase"} @@ -83,6 +118,9 @@ func rebaseContinueOnce(opts RebaseOpts) error { func tryAutoResolveRebase(originalErr error, opts RebaseOpts) error { for i := 0; i < 1000; i++ { if !IsRebaseInProgress() { + if i == 0 { + return originalErr + } return nil } conflicts, err := ConflictedFiles() @@ -134,6 +172,11 @@ func Fetch(remote string) error { return ops.Fetch(remote) } +// FetchBranch fetches one branch into its remote-tracking ref. +func FetchBranch(remote, branch string) error { + return ops.FetchBranch(remote, branch) +} + // FetchBranches fetches specific branches from a remote, // updating their tracking refs. func FetchBranches(remote string, branches []string) error { @@ -348,6 +391,11 @@ func SetUpstreamTracking(branch, remote string) error { return ops.SetUpstreamTracking(branch, remote) } +// UpstreamRemote returns the remote configured as a branch's upstream. +func UpstreamRemote(branch string) (string, error) { + return ops.UpstreamRemote(branch) +} + // MergeFF fast-forwards the currently checked-out branch using a merge. func MergeFF(target string) error { return ops.MergeFF(target) diff --git a/internal/git/gitops.go b/internal/git/gitops.go index 523c8e68..6a495802 100644 --- a/internal/git/gitops.go +++ b/internal/git/gitops.go @@ -17,6 +17,11 @@ type RebaseOpts struct { CommitterDateIsAuthorDate bool } +// ErrRemoteBranchNotFound indicates that a requested branch does not exist on +// the remote. It is distinct from transport, authentication, and other fetch +// failures. +var ErrRemoteBranchNotFound = errors.New("remote branch not found") + // Ops defines the interface for git operations used by commands. // The package-level functions are the default production implementation. // Tests can substitute a mock via SetOps(). @@ -27,6 +32,7 @@ type Ops interface { BranchExists(name string) bool CheckoutBranch(name string) error Fetch(remote string) error + FetchBranch(remote, branch string) error FetchBranches(remote string, branches []string) error DefaultBranch() (string, error) CreateBranch(name, base string) error @@ -59,6 +65,7 @@ type Ops interface { DeleteTrackingRef(remote, branch string) error ResetHard(ref string) error SetUpstreamTracking(branch, remote string) error + UpstreamRemote(branch string) (string, error) MergeFF(target string) error UpdateBranchRef(branch, sha string) error StageAll() error @@ -123,6 +130,17 @@ func (d *defaultOps) Fetch(remote string) error { return client.Fetch(context.Background(), remote, "") } +func (d *defaultOps) FetchBranch(remote, branch string) error { + refspec := fmt.Sprintf("+refs/heads/%s:refs/remotes/%s/%s", branch, remote, branch) + if err := runSilent("fetch", remote, refspec); err != nil { + if isMissingRemoteRefError(err) { + return fmt.Errorf("%w: %s/%s", ErrRemoteBranchNotFound, remote, branch) + } + return err + } + return nil +} + func (d *defaultOps) FetchBranches(remote string, branches []string) error { if len(branches) == 0 { return nil @@ -142,11 +160,22 @@ func (d *defaultOps) FetchBranches(remote string, branches []string) error { } // Fallback: one branch may be absent on the remote or deleted since // the last fetch. Fetch individually so one missing branch doesn't - // block the rest. Per-branch failure is expected and tolerated. + // block the rest, while still surfacing real fetch failures. + var fetchErr error for _, rs := range refspecs { - _ = runSilent("fetch", remote, rs) + err := runSilent("fetch", remote, rs) + if err == nil || isMissingRemoteRefError(err) { + continue + } + if fetchErr == nil { + fetchErr = fmt.Errorf("fetching from %s: %w", remote, err) + } } - return nil + return fetchErr +} + +func isMissingRemoteRefError(err error) bool { + return err != nil && strings.Contains(err.Error(), "couldn't find remote ref") } func (d *defaultOps) DefaultBranch() (string, error) { @@ -245,11 +274,7 @@ func (d *defaultOps) Rebase(base string, opts RebaseOpts) error { args = append(args, "--committer-date-is-author-date") } args = append(args, base) - err := runSilent(args...) - if err == nil { - return nil - } - return tryAutoResolveRebase(err, opts) + return runRebaseCommand(args, opts) } func (d *defaultOps) EnableRerere() error { @@ -302,11 +327,7 @@ func (d *defaultOps) RebaseOnto(newBase, oldBase, branch string, opts RebaseOpts args = append(args, "--committer-date-is-author-date") } args = append(args, "--onto", newBase, oldBase, branch) - err := runSilent(args...) - if err == nil { - return nil - } - return tryAutoResolveRebase(err, opts) + return runRebaseCommand(args, opts) } func (d *defaultOps) RebaseContinue(opts RebaseOpts) error { @@ -564,6 +585,10 @@ func (d *defaultOps) SetUpstreamTracking(branch, remote string) error { return runSilent("branch", "--set-upstream-to="+remote+"/"+branch, branch) } +func (d *defaultOps) UpstreamRemote(branch string) (string, error) { + return run("config", "--get", "branch."+branch+".remote") +} + func (d *defaultOps) MergeFF(target string) error { return runSilent("merge", "--ff-only", target) } diff --git a/internal/git/mock_ops.go b/internal/git/mock_ops.go index 9c2ce434..5bc52768 100644 --- a/internal/git/mock_ops.go +++ b/internal/git/mock_ops.go @@ -12,6 +12,7 @@ type MockOps struct { BranchExistsFn func(string) bool CheckoutBranchFn func(string) error FetchFn func(string) error + FetchBranchFn func(string, string) error FetchBranchesFn func(string, []string) error DefaultBranchFn func() (string, error) CreateBranchFn func(string, string) error @@ -44,6 +45,7 @@ type MockOps struct { DeleteTrackingRefFn func(string, string) error ResetHardFn func(string) error SetUpstreamTrackingFn func(string, string) error + UpstreamRemoteFn func(string) (string, error) MergeFFFn func(string) error UpdateBranchRefFn func(string, string) error StageAllFn func() error @@ -106,6 +108,13 @@ func (m *MockOps) Fetch(remote string) error { return nil } +func (m *MockOps) FetchBranch(remote, branch string) error { + if m.FetchBranchFn != nil { + return m.FetchBranchFn(remote, branch) + } + return nil +} + func (m *MockOps) FetchBranches(remote string, branches []string) error { if m.FetchBranchesFn != nil { return m.FetchBranchesFn(remote, branches) @@ -339,6 +348,13 @@ func (m *MockOps) SetUpstreamTracking(branch, remote string) error { return nil } +func (m *MockOps) UpstreamRemote(branch string) (string, error) { + if m.UpstreamRemoteFn != nil { + return m.UpstreamRemoteFn(branch) + } + return "", nil +} + func (m *MockOps) MergeFF(target string) error { if m.MergeFFFn != nil { return m.MergeFFFn(target) diff --git a/internal/git/rebase_start_test.go b/internal/git/rebase_start_test.go new file mode 100644 index 00000000..bd6d12f8 --- /dev/null +++ b/internal/git/rebase_start_test.go @@ -0,0 +1,87 @@ +package git + +import ( + "errors" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func rebaseTestRepo(t *testing.T) string { + t.Helper() + _, clone := setupBareAndClone(t) + + gitExec(t, clone, "checkout", "-b", "feature") + writeFile(t, clone, "feature.txt", "feature") + gitExec(t, clone, "add", ".") + gitExec(t, clone, "commit", "-m", "feature") + + gitExec(t, clone, "checkout", "main") + writeFile(t, clone, "main.txt", "main update") + gitExec(t, clone, "add", ".") + gitExec(t, clone, "commit", "-m", "main update") + gitExec(t, clone, "checkout", "feature") + + return clone +} + +func TestIntegration_RebaseRefusedBeforeStart(t *testing.T) { + clone := rebaseTestRepo(t) + restore := withGitDir(t, clone) + defer restore() + + writeFile(t, clone, "feature.txt", "uncommitted") + + err := Rebase("main", RebaseOpts{}) + require.Error(t, err) + assert.True(t, IsRebaseStartError(err)) + assert.False(t, IsRebaseInProgress()) +} + +func TestIntegration_RebaseConflictIsNotStartError(t *testing.T) { + clone := rebaseTestRepo(t) + restore := withGitDir(t, clone) + defer restore() + + writeFile(t, clone, "init.txt", "feature version") + gitExec(t, clone, "add", "init.txt") + gitExec(t, clone, "commit", "-m", "feature conflict") + + gitExec(t, clone, "checkout", "main") + writeFile(t, clone, "init.txt", "main version") + gitExec(t, clone, "add", "init.txt") + gitExec(t, clone, "commit", "-m", "main conflict") + gitExec(t, clone, "checkout", "feature") + + err := Rebase("main", RebaseOpts{}) + require.Error(t, err) + assert.False(t, IsRebaseStartError(err)) + assert.True(t, IsRebaseInProgress()) + gitExec(t, clone, "rebase", "--abort") +} + +func TestIntegration_FetchBranchRejectsDeletedRemoteBranchWithStaleTrackingRef(t *testing.T) { + bare, clone := setupBareAndClone(t) + + gitExec(t, clone, "checkout", "-b", "feature-trunk") + writeFile(t, clone, "feature.txt", "feature") + gitExec(t, clone, "add", ".") + gitExec(t, clone, "commit", "-m", "feature trunk") + gitExec(t, clone, "push", "origin", "feature-trunk") + staleSHA := gitExec(t, clone, "rev-parse", "origin/feature-trunk") + + other := filepath.Join(t.TempDir(), "other") + gitExec(t, ".", "clone", bare, other) + gitExec(t, other, "push", "origin", "--delete", "feature-trunk") + + restore := withGitDir(t, clone) + defer restore() + + err := FetchBranch("origin", "feature-trunk") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrRemoteBranchNotFound)) + assert.Equal(t, staleSHA, gitExec(t, clone, "rev-parse", "origin/feature-trunk"), + "the stale tracking ref still exists, so callers must trust the fetch result") +} diff --git a/internal/modify/apply.go b/internal/modify/apply.go index 6aafc31d..d7621287 100644 --- a/internal/modify/apply.go +++ b/internal/modify/apply.go @@ -584,6 +584,13 @@ func ApplyPlan( } if err := git.RebaseOnto(newBase, oldBase, b.Branch, git.RebaseOpts{}); err != nil { + if git.IsRebaseStartError(err) { + if saveErr := stack.SaveWithLock(gitDir, sf, lock); saveErr != nil { + cfg.Warningf("failed to save stack metadata: %v", saveErr) + } + return nil, nil, fmt.Errorf("could not start rebase of %s onto %s: %w", b.Branch, newBase, err) + } + conflict := &modifyview.ConflictInfo{ Branch: b.Branch, } @@ -815,8 +822,12 @@ func ContinueApply( affectsPRs = true } - // Finish the in-progress git operation (rebase or cherry-pick) - if state.ConflictType == "cherry_pick" { + remainingBranches := state.RemainingBranches + + // Finish the in-progress git operation, or resume at a rebase that was + // previously refused before it could start. + switch state.ConflictType { + case "cherry_pick": if err := git.CherryPickContinue(); err != nil { return fmt.Errorf("cherry-pick continue failed — resolve remaining conflicts and try again: %w", err) } @@ -827,7 +838,7 @@ func ContinueApply( if foldIdx >= 0 && foldIdx < len(s.Branches) { s.Branches = append(s.Branches[:foldIdx], s.Branches[foldIdx+1:]...) } - } else { + case "", "rebase": // Rebase conflict if git.IsRebaseInProgress() { if err := git.RebaseContinue(git.RebaseOpts{}); err != nil { @@ -835,10 +846,14 @@ func ContinueApply( } } cfg.Successf("Rebased %s", state.ConflictBranch) + case "rebase_start": + remainingBranches = append([]string{state.ConflictBranch}, remainingBranches...) + default: + return fmt.Errorf("unknown modify conflict type %q", state.ConflictType) } // Continue cascading rebase for remaining branches - for _, branchName := range state.RemainingBranches { + for _, branchName := range remainingBranches { idx := s.IndexOf(branchName) if idx < 0 { cfg.Warningf("branch %s no longer in stack, skipping", branchName) @@ -878,10 +893,35 @@ func ContinueApply( } if err := git.RebaseOnto(newBase, oldBase, b.Branch, git.RebaseOpts{}); err != nil { + if git.IsRebaseStartError(err) { + remaining := make([]string, 0) + foundCurrent := false + for _, rn := range remainingBranches { + if rn == branchName { + foundCurrent = true + continue + } + if foundCurrent { + remaining = append(remaining, rn) + } + } + state.ConflictBranch = branchName + state.ConflictType = "rebase_start" + state.RemainingBranches = remaining + state.AffectsPRs = affectsPRs + if saveErr := SaveState(gitDir, state); saveErr != nil { + cfg.Warningf("failed to update modify state: %v", saveErr) + } + if saveErr := stack.SaveWithLock(gitDir, sf, lock); saveErr != nil { + cfg.Warningf("failed to save stack metadata: %v", saveErr) + } + return fmt.Errorf("could not start rebase of %s onto %s: %w", b.Branch, newBase, err) + } + // Another conflict — update state and bail remaining := make([]string, 0) foundCurrent := false - for _, rn := range state.RemainingBranches { + for _, rn := range remainingBranches { if rn == branchName { foundCurrent = true continue diff --git a/internal/modify/apply_test.go b/internal/modify/apply_test.go index 0ad6059c..fc805aa0 100644 --- a/internal/modify/apply_test.go +++ b/internal/modify/apply_test.go @@ -2,6 +2,7 @@ package modify import ( "encoding/json" + "errors" "os" "path/filepath" "testing" @@ -1417,6 +1418,82 @@ func TestContinueApply_FoldThenCascadeConflict_DoesNotResurrectFoldedBranch(t *t assert.False(t, StateExists(gitDir), "state should be cleared after successful recovery") } +func TestContinueApply_RebaseStartErrorPersistsRetryState(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "A"}, + {Branch: "B"}, + {Branch: "C"}, + }, + } + + gitDir := t.TempDir() + writeTestStackFile(t, gitDir, s) + + state := &StateFile{ + SchemaVersion: 1, + StackName: "main", + StackIndex: 0, + Phase: PhaseConflict, + ConflictType: "cherry_pick", + ConflictBranch: "B", + FoldBranch: "B", + FoldTarget: "A", + RemainingBranches: []string{"A", "C"}, + OriginalBranch: "A", + OriginalRefs: map[string]string{"A": "sha-main", "C": "sha-A-old"}, + } + require.NoError(t, SaveState(gitDir, state)) + + mock := newApplyMock(gitDir, map[string]string{ + "main": "sha-main", "A": "sha-A", "B": "sha-B", "C": "sha-C", + }) + cherryPickContinues := 0 + mock.CherryPickContinueFn = func() error { + cherryPickContinues++ + return nil + } + cRebases := 0 + mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { + if branch == "C" { + cRebases++ + if cRebases == 1 { + return &git.RebaseStartError{Err: errors.New("branch is checked out elsewhere")} + } + } + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + defer cfg.Out.Close() + defer cfg.Err.Close() + + err := ContinueApply(cfg, gitDir, noopUpdateBaseSHAs) + require.Error(t, err) + + retryState, err := LoadState(gitDir) + require.NoError(t, err) + require.NotNil(t, retryState) + assert.Equal(t, "rebase_start", retryState.ConflictType) + assert.Equal(t, "C", retryState.ConflictBranch) + assert.Empty(t, retryState.RemainingBranches) + + afterFirst, err := stack.Load(gitDir) + require.NoError(t, err) + assert.Equal(t, -1, afterFirst.Stacks[0].IndexOf("B"), + "completed fold metadata must be saved before waiting to retry the rebase") + + err = ContinueApply(cfg, gitDir, noopUpdateBaseSHAs) + require.NoError(t, err) + assert.Equal(t, 1, cherryPickContinues, "retry must not repeat the completed cherry-pick") + assert.Equal(t, 2, cRebases, "retry must restart the refused rebase") + assert.False(t, StateExists(gitDir)) +} + // ─── Unwind restores renamed branch ───────────────────────────────────────── func TestUnwind_RestoresRenamedBranch(t *testing.T) {