diff --git a/cmd/cascade.go b/cmd/cascade.go index e43a139..f524e54 100644 --- a/cmd/cascade.go +++ b/cmd/cascade.go @@ -145,6 +145,17 @@ func doCascadeWithState(g *git.Git, cfg *config.Config, branches []*tree.Node, d if !needsRebase { fmt.Printf("Restacking %s... %s\n", s.Branch(b.Name), s.Muted("already up to date")) + + // Refresh fork point even when no rebase is needed. If the branch + // was rebased outside gh-stack the stored fork point would be stale; + // keeping it current prevents a future --onto rebase from replaying + // too many commits. + if !dryRun { + parentTip, tipErr := g.GetTip(parent) + if tipErr == nil { + _ = cfg.SetForkPoint(b.Name, parentTip) //nolint:errcheck // best effort + } + } continue } @@ -153,17 +164,47 @@ func doCascadeWithState(g *git.Git, cfg *config.Config, branches []*tree.Node, d continue } - // Check if we should use --onto rebase - // This is needed when parent has been rebased/amended since child was created + // Check if we should use --onto rebase. + // This is needed when the parent has been rebased/amended since the child was created. storedForkPoint, fpErr := cfg.GetForkPoint(b.Name) useOnto := false - if fpErr == nil && g.CommitExists(storedForkPoint) { - // We have a valid stored fork point - // Use --onto if the stored fork point differs from merge-base - currentMergeBase, mbErr := g.GetMergeBase(b.Name, parent) - if mbErr == nil && currentMergeBase != storedForkPoint { - useOnto = true + if fpErr == nil { + if !g.CommitExists(storedForkPoint) { + // The stored fork point no longer exists — it may have been + // garbage collected after a history rewrite or a sufficiently + // aggressive `git gc`. Without it we cannot use --onto, so we + // fall back to a plain rebase against the parent tip. If the + // parent's history was genuinely rewritten this fallback may + // produce spurious conflicts or silently re-apply commits that + // were already in the parent; the user should resolve conflicts + // as normal or re-run `gh stack restack` after history settles. + fmt.Printf(" %s\n", s.Muted(fmt.Sprintf( + "warning: stored fork point %s is no longer available (garbage collected?); falling back to simple rebase", + git.AbbrevSHA(storedForkPoint), + ))) + } else { + // Fork point is reachable — determine whether --onto is appropriate. + currentMergeBase, mbErr := g.GetMergeBase(b.Name, parent) + if mbErr == nil && currentMergeBase != storedForkPoint { + // Fork point differs from merge-base. Determine why: + // + // If the stored fork point is an ancestor of the merge-base, + // it's just stale (e.g. branch was rebased outside gh-stack, + // or fork point wasn't updated after a conflict resolution). + // A simple rebase using the merge-base is correct; refresh the + // fork point so it stays current. + // + // If the stored fork point is NOT an ancestor of the merge-base, + // the parent's history was rewritten (squash merge, force push). + // We need --onto with the stored fork point to identify the + // correct commit range. + if g.IsAncestor(storedForkPoint, currentMergeBase) { + _ = cfg.SetForkPoint(b.Name, currentMergeBase) //nolint:errcheck // best effort + } else { + useOnto = true + } + } } } diff --git a/cmd/continue.go b/cmd/continue.go index d1bf035..a123390 100644 --- a/cmd/continue.go +++ b/cmd/continue.go @@ -68,6 +68,16 @@ func runContinue(cmd *cobra.Command, args []string) error { return err } + // Update fork point for the branch that just finished rebasing. + // The cascade loop's fork point update only fires after a non-conflicting + // rebase, so branches that hit a conflict never get their fork point + // refreshed -- leaving it stale for future operations. + if parent, parentErr := cfg.GetParent(st.Current); parentErr == nil { + if parentTip, tipErr := g.GetTip(parent); tipErr == nil { + _ = cfg.SetForkPoint(st.Current, parentTip) //nolint:errcheck // best effort + } + } + // Build tree to get node objects root, err := tree.Build(cfg) if err != nil { diff --git a/e2e/cascade_test.go b/e2e/cascade_test.go index 43327f0..1667de8 100644 --- a/e2e/cascade_test.go +++ b/e2e/cascade_test.go @@ -140,3 +140,153 @@ func TestCascadeReturnsToOriginalBranch(t *testing.T) { env.AssertAncestor("feature-a", "feature-b") env.AssertAncestor("feature-b", "feature-c") } + +func TestCascadeStaleForkPointFromManualRebase(t *testing.T) { + // Reproduces the bug where a manual rebase outside gh-stack leaves the + // fork point stale. On the next restack after main advances, the stale + // fork point would trigger an --onto rebase that replays too many commits. + env := NewTestEnv(t) + env.MustRun("init") + + env.MustRun("create", "feature-a") + env.CreateCommit("feature a work") + + // Advance main + env.Git("checkout", "main") + env.CreateCommit("main update 1") + + // Manually rebase (outside gh-stack) -- fork point stays stale + env.Git("checkout", "feature-a") + env.Git("rebase", "main") + + // Run restack while already up-to-date so fork point gets refreshed + env.MustRun("restack") + + // Advance main again + env.Git("checkout", "main") + env.CreateCommit("main update 2") + + // This restack should use a simple rebase (not --onto with stale fork point) + env.Git("checkout", "feature-a") + result := env.MustRun("restack") + + // Should NOT say "using fork point" -- that would mean the stale fork + // point incorrectly triggered the --onto path + if result.ContainsStdout("using fork point") { + t.Error("restack should use simple rebase, not --onto with stale fork point") + } + + env.AssertAncestor("main", "feature-a") + env.AssertNoRebaseInProgress() +} + +func TestCascadeStaleForkPointDetectedDuringRebase(t *testing.T) { + // Even if the "already up to date" refresh was missed, the ancestor check + // in the useOnto logic should prevent --onto with a stale fork point. + env := NewTestEnv(t) + env.MustRun("init") + + env.MustRun("create", "feature-a") + env.CreateCommit("feature a work") + + // Record fork point before any manipulation + originalFP := env.GetStackConfig("branch.feature-a.stackforkpoint") + if originalFP == "" { + t.Fatal("expected fork point to be set after create") + } + + // Advance main + env.Git("checkout", "main") + env.CreateCommit("main advance 1") + env.CreateCommit("main advance 2") + + // Manually rebase feature-a onto current main + env.Git("checkout", "feature-a") + env.Git("rebase", "main") + + // Fork point is still the old value (stale) + fpAfterManual := env.GetStackConfig("branch.feature-a.stackforkpoint") + if fpAfterManual != originalFP { + t.Fatal("expected fork point to still be the original (stale) value") + } + + // Advance main further + env.Git("checkout", "main") + env.CreateCommit("main advance 3") + + // Restack from feature-a -- this triggers NeedsRebase=true with a stale + // fork point that differs from merge-base. The fix should detect the stale + // fork point is an ancestor of the merge-base and use a simple rebase. + env.Git("checkout", "feature-a") + result := env.MustRun("restack") + + if result.ContainsStdout("using fork point") { + t.Error("restack should NOT use --onto with a stale fork point that is an ancestor of merge-base") + } + + env.AssertAncestor("main", "feature-a") + env.AssertNoRebaseInProgress() +} + +func TestCascadeForkPointUpdatedAfterContinue(t *testing.T) { + env := NewTestEnv(t) + env.MustRun("init") + + // Create a stack that will conflict + conflictFile := env.CreateStackWithConflict() + + // Cascade will hit a conflict on feature-b + result := env.Run("cascade") + if result.Success() { + t.Fatal("expected conflict") + } + + // Resolve and continue + env.ResolveConflict(conflictFile) + env.MustRun("continue") + + // After continue, fork point for feature-a should be updated to main's tip. + // (feature-a was the one that got rebased before the conflict on feature-b.) + mainTip := env.BranchTip("main") + featureAFP := env.GetStackConfig("branch.feature-a.stackforkpoint") + if featureAFP != mainTip { + t.Errorf("feature-a fork point after continue = %s, want main tip %s", featureAFP[:7], mainTip[:7]) + } + + // feature-b should also have its fork point updated (to feature-a's tip) + featureATip := env.BranchTip("feature-a") + featureBFP := env.GetStackConfig("branch.feature-b.stackforkpoint") + if featureBFP != featureATip { + t.Errorf("feature-b fork point after continue = %s, want feature-a tip %s", featureBFP[:7], featureATip[:7]) + } +} + +func TestCascadeOntoUsedForRewrittenParent(t *testing.T) { + // Verify that --onto IS used when the parent's history was actually + // rewritten (not just a stale fork point). + env := NewTestEnv(t) + env.MustRun("init") + + env.MustRun("create", "feature-a") + env.CreateCommit("feature a work") + + env.MustRun("create", "feature-b") + env.CreateCommit("feature b work") + + // Amend feature-a's commit (rewrites its history) + env.Git("checkout", "feature-a") + env.WriteFile("feature-a-amended.txt", "amended content") + env.Git("add", ".") + env.Git("commit", "--amend", "--no-edit") + + // Restack from feature-a. feature-b's fork point (old feature-a tip) + // is now on a different history line → --onto should be used. + result := env.MustRun("restack") + + if !result.ContainsStdout("using fork point") { + t.Error("restack should use --onto when parent history was rewritten") + } + + env.AssertAncestor("feature-a", "feature-b") + env.AssertNoRebaseInProgress() +} diff --git a/internal/git/git.go b/internal/git/git.go index b5656a8..8a89c66 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -178,6 +178,13 @@ func (g *Git) CommitExists(sha string) bool { return err == nil } +// IsAncestor returns true if ancestor is a git ancestor of descendant. +// Uses `git merge-base --is-ancestor` which returns exit code 0 when true. +func (g *Git) IsAncestor(ancestor, descendant string) bool { + err := g.runSilent("merge-base", "--is-ancestor", ancestor, descendant) + return err == nil +} + // HasUnmergedCommits returns true if the branch has commits not yet in upstream. // Uses git cherry to detect by diff content, which works for cherry-picks // where the commit SHAs differ but the content is the same. diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 5fc45a6..8c7ddf7 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -378,6 +378,49 @@ func TestCommitExists(t *testing.T) { } } +func TestIsAncestor(t *testing.T) { + dir := setupTestRepo(t) + g := git.New(dir) + + trunk, _ := g.CurrentBranch() + initialTip, _ := g.GetTip(trunk) + + // Add a commit to move trunk forward + os.WriteFile(filepath.Join(dir, "new.txt"), []byte("new"), 0644) + exec.Command("git", "-C", dir, "add", ".").Run() + exec.Command("git", "-C", dir, "commit", "-m", "second commit").Run() + secondTip, _ := g.GetTip(trunk) + + // initial is ancestor of second + if !g.IsAncestor(initialTip, secondTip) { + t.Errorf("expected %s to be ancestor of %s", initialTip[:7], secondTip[:7]) + } + + // second is NOT ancestor of initial + if g.IsAncestor(secondTip, initialTip) { + t.Errorf("expected %s to NOT be ancestor of %s", secondTip[:7], initialTip[:7]) + } + + // commit is its own ancestor + if !g.IsAncestor(initialTip, initialTip) { + t.Errorf("expected commit to be its own ancestor") + } + + // divergent branches are not ancestors of each other + exec.Command("git", "-C", dir, "checkout", "-b", "diverge", initialTip).Run() + os.WriteFile(filepath.Join(dir, "diverge.txt"), []byte("diverge"), 0644) + exec.Command("git", "-C", dir, "add", ".").Run() + exec.Command("git", "-C", dir, "commit", "-m", "diverge commit").Run() + divergeTip, _ := g.GetTip("diverge") + + if g.IsAncestor(divergeTip, secondTip) { + t.Error("divergent branch tip should not be ancestor of trunk tip") + } + if g.IsAncestor(secondTip, divergeTip) { + t.Error("trunk tip should not be ancestor of divergent branch tip") + } +} + func TestRebaseOnto(t *testing.T) { dir := setupTestRepo(t) g := git.New(dir)