fix(restack): prevent stale fork points from triggering incorrect --onto rebases (#83)

* fix(restack): prevent stale fork points from triggering incorrect --onto rebases

When a stored fork point fell behind the actual divergence point (e.g.
after a manual `git rebase` or a conflict-then-continue), the
`useOnto` logic would incorrectly trigger `git rebase --onto` with
the stale fork point as the upstream boundary. This replayed commits
already present in the parent, causing spurious conflicts and dropped
patches.

Three fixes:

- **Ancestor check in `useOnto` decision**: distinguish a stale fork
  point (ancestor of merge-base → use simple rebase) from a genuinely
  rewritten parent (divergent history → `--onto` is correct).
- **Refresh fork point on "already up to date"**: when `NeedsRebase`
  returns false, update the fork point to the parent tip so external
  rebases don't leave it permanently stale.
- **Update fork point in `continue`**: after resolving a conflict,
  the branch that was being rebased never had its fork point refreshed;
  now it does.

Adds `Git.IsAncestor()` helper and five new tests covering all three
paths plus a regression guard for the legitimate `--onto` case.

* fix(restack): handle GC'd fork points gracefully with a warning

When a stored fork point commit has been garbage collected (e.g. after
a history rewrite followed by aggressive `git gc`), the previous code
silently fell through to a plain rebase. Now it emits a warning so the
user knows why `--onto` wasn't used, then falls back to a simple
rebase as before.

Author: boneskull

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
+					}
+				}
 			}
 		}
 

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 {

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()
+}

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.

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)