Skip to content

Commit 88680fd

Browse files
committed
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.
1 parent e6c362d commit 88680fd

File tree

5 files changed

+238
-3
lines changed

5 files changed

+238
-3
lines changed

cmd/cascade.go

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,17 @@ func doCascadeWithState(g *git.Git, cfg *config.Config, branches []*tree.Node, d
145145

146146
if !needsRebase {
147147
fmt.Printf("Restacking %s... %s\n", s.Branch(b.Name), s.Muted("already up to date"))
148+
149+
// Refresh fork point even when no rebase is needed. If the branch
150+
// was rebased outside gh-stack the stored fork point would be stale;
151+
// keeping it current prevents a future --onto rebase from replaying
152+
// too many commits.
153+
if !dryRun {
154+
parentTip, tipErr := g.GetTip(parent)
155+
if tipErr == nil {
156+
_ = cfg.SetForkPoint(b.Name, parentTip) //nolint:errcheck // best effort
157+
}
158+
}
148159
continue
149160
}
150161

@@ -159,11 +170,25 @@ func doCascadeWithState(g *git.Git, cfg *config.Config, branches []*tree.Node, d
159170
useOnto := false
160171

161172
if fpErr == nil && g.CommitExists(storedForkPoint) {
162-
// We have a valid stored fork point
163-
// Use --onto if the stored fork point differs from merge-base
164173
currentMergeBase, mbErr := g.GetMergeBase(b.Name, parent)
165174
if mbErr == nil && currentMergeBase != storedForkPoint {
166-
useOnto = true
175+
// Fork point differs from merge-base. Determine why:
176+
//
177+
// If the stored fork point is an ancestor of the merge-base,
178+
// it's just stale (e.g. branch was rebased outside gh-stack,
179+
// or fork point wasn't updated after a conflict resolution).
180+
// A simple rebase using the merge-base is correct; refresh the
181+
// fork point so it stays current.
182+
//
183+
// If the stored fork point is NOT an ancestor of the merge-base,
184+
// the parent's history was rewritten (squash merge, force push).
185+
// We need --onto with the stored fork point to identify the
186+
// correct commit range.
187+
if g.IsAncestor(storedForkPoint, currentMergeBase) {
188+
_ = cfg.SetForkPoint(b.Name, currentMergeBase) //nolint:errcheck // best effort
189+
} else {
190+
useOnto = true
191+
}
167192
}
168193
}
169194

cmd/continue.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,16 @@ func runContinue(cmd *cobra.Command, args []string) error {
6868
return err
6969
}
7070

71+
// Update fork point for the branch that just finished rebasing.
72+
// The cascade loop's fork point update only fires after a non-conflicting
73+
// rebase, so branches that hit a conflict never get their fork point
74+
// refreshed -- leaving it stale for future operations.
75+
if parent, parentErr := cfg.GetParent(st.Current); parentErr == nil {
76+
if parentTip, tipErr := g.GetTip(parent); tipErr == nil {
77+
_ = cfg.SetForkPoint(st.Current, parentTip) //nolint:errcheck // best effort
78+
}
79+
}
80+
7181
// Build tree to get node objects
7282
root, err := tree.Build(cfg)
7383
if err != nil {

e2e/cascade_test.go

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,3 +140,153 @@ func TestCascadeReturnsToOriginalBranch(t *testing.T) {
140140
env.AssertAncestor("feature-a", "feature-b")
141141
env.AssertAncestor("feature-b", "feature-c")
142142
}
143+
144+
func TestCascadeStaleForkPointFromManualRebase(t *testing.T) {
145+
// Reproduces the bug where a manual rebase outside gh-stack leaves the
146+
// fork point stale. On the next restack after main advances, the stale
147+
// fork point would trigger an --onto rebase that replays too many commits.
148+
env := NewTestEnv(t)
149+
env.MustRun("init")
150+
151+
env.MustRun("create", "feature-a")
152+
env.CreateCommit("feature a work")
153+
154+
// Advance main
155+
env.Git("checkout", "main")
156+
env.CreateCommit("main update 1")
157+
158+
// Manually rebase (outside gh-stack) -- fork point stays stale
159+
env.Git("checkout", "feature-a")
160+
env.Git("rebase", "main")
161+
162+
// Run restack while already up-to-date so fork point gets refreshed
163+
env.MustRun("restack")
164+
165+
// Advance main again
166+
env.Git("checkout", "main")
167+
env.CreateCommit("main update 2")
168+
169+
// This restack should use a simple rebase (not --onto with stale fork point)
170+
env.Git("checkout", "feature-a")
171+
result := env.MustRun("restack")
172+
173+
// Should NOT say "using fork point" -- that would mean the stale fork
174+
// point incorrectly triggered the --onto path
175+
if result.ContainsStdout("using fork point") {
176+
t.Error("restack should use simple rebase, not --onto with stale fork point")
177+
}
178+
179+
env.AssertAncestor("main", "feature-a")
180+
env.AssertNoRebaseInProgress()
181+
}
182+
183+
func TestCascadeStaleForkPointDetectedDuringRebase(t *testing.T) {
184+
// Even if the "already up to date" refresh was missed, the ancestor check
185+
// in the useOnto logic should prevent --onto with a stale fork point.
186+
env := NewTestEnv(t)
187+
env.MustRun("init")
188+
189+
env.MustRun("create", "feature-a")
190+
env.CreateCommit("feature a work")
191+
192+
// Record fork point before any manipulation
193+
originalFP := env.GetStackConfig("branch.feature-a.stackforkpoint")
194+
if originalFP == "" {
195+
t.Fatal("expected fork point to be set after create")
196+
}
197+
198+
// Advance main
199+
env.Git("checkout", "main")
200+
env.CreateCommit("main advance 1")
201+
env.CreateCommit("main advance 2")
202+
203+
// Manually rebase feature-a onto current main
204+
env.Git("checkout", "feature-a")
205+
env.Git("rebase", "main")
206+
207+
// Fork point is still the old value (stale)
208+
fpAfterManual := env.GetStackConfig("branch.feature-a.stackforkpoint")
209+
if fpAfterManual != originalFP {
210+
t.Fatal("expected fork point to still be the original (stale) value")
211+
}
212+
213+
// Advance main further
214+
env.Git("checkout", "main")
215+
env.CreateCommit("main advance 3")
216+
217+
// Restack from feature-a -- this triggers NeedsRebase=true with a stale
218+
// fork point that differs from merge-base. The fix should detect the stale
219+
// fork point is an ancestor of the merge-base and use a simple rebase.
220+
env.Git("checkout", "feature-a")
221+
result := env.MustRun("restack")
222+
223+
if result.ContainsStdout("using fork point") {
224+
t.Error("restack should NOT use --onto with a stale fork point that is an ancestor of merge-base")
225+
}
226+
227+
env.AssertAncestor("main", "feature-a")
228+
env.AssertNoRebaseInProgress()
229+
}
230+
231+
func TestCascadeForkPointUpdatedAfterContinue(t *testing.T) {
232+
env := NewTestEnv(t)
233+
env.MustRun("init")
234+
235+
// Create a stack that will conflict
236+
conflictFile := env.CreateStackWithConflict()
237+
238+
// Cascade will hit a conflict on feature-b
239+
result := env.Run("cascade")
240+
if result.Success() {
241+
t.Fatal("expected conflict")
242+
}
243+
244+
// Resolve and continue
245+
env.ResolveConflict(conflictFile)
246+
env.MustRun("continue")
247+
248+
// After continue, fork point for feature-a should be updated to main's tip.
249+
// (feature-a was the one that got rebased before the conflict on feature-b.)
250+
mainTip := env.BranchTip("main")
251+
featureAFP := env.GetStackConfig("branch.feature-a.stackforkpoint")
252+
if featureAFP != mainTip {
253+
t.Errorf("feature-a fork point after continue = %s, want main tip %s", featureAFP[:7], mainTip[:7])
254+
}
255+
256+
// feature-b should also have its fork point updated (to feature-a's tip)
257+
featureATip := env.BranchTip("feature-a")
258+
featureBFP := env.GetStackConfig("branch.feature-b.stackforkpoint")
259+
if featureBFP != featureATip {
260+
t.Errorf("feature-b fork point after continue = %s, want feature-a tip %s", featureBFP[:7], featureATip[:7])
261+
}
262+
}
263+
264+
func TestCascadeOntoUsedForRewrittenParent(t *testing.T) {
265+
// Verify that --onto IS used when the parent's history was actually
266+
// rewritten (not just a stale fork point).
267+
env := NewTestEnv(t)
268+
env.MustRun("init")
269+
270+
env.MustRun("create", "feature-a")
271+
env.CreateCommit("feature a work")
272+
273+
env.MustRun("create", "feature-b")
274+
env.CreateCommit("feature b work")
275+
276+
// Amend feature-a's commit (rewrites its history)
277+
env.Git("checkout", "feature-a")
278+
env.WriteFile("feature-a-amended.txt", "amended content")
279+
env.Git("add", ".")
280+
env.Git("commit", "--amend", "--no-edit")
281+
282+
// Restack from feature-a. feature-b's fork point (old feature-a tip)
283+
// is now on a different history line → --onto should be used.
284+
result := env.MustRun("restack")
285+
286+
if !result.ContainsStdout("using fork point") {
287+
t.Error("restack should use --onto when parent history was rewritten")
288+
}
289+
290+
env.AssertAncestor("feature-a", "feature-b")
291+
env.AssertNoRebaseInProgress()
292+
}

internal/git/git.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,13 @@ func (g *Git) CommitExists(sha string) bool {
178178
return err == nil
179179
}
180180

181+
// IsAncestor returns true if ancestor is a git ancestor of descendant.
182+
// Uses `git merge-base --is-ancestor` which returns exit code 0 when true.
183+
func (g *Git) IsAncestor(ancestor, descendant string) bool {
184+
err := g.runSilent("merge-base", "--is-ancestor", ancestor, descendant)
185+
return err == nil
186+
}
187+
181188
// HasUnmergedCommits returns true if the branch has commits not yet in upstream.
182189
// Uses git cherry to detect by diff content, which works for cherry-picks
183190
// where the commit SHAs differ but the content is the same.

internal/git/git_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,49 @@ func TestCommitExists(t *testing.T) {
378378
}
379379
}
380380

381+
func TestIsAncestor(t *testing.T) {
382+
dir := setupTestRepo(t)
383+
g := git.New(dir)
384+
385+
trunk, _ := g.CurrentBranch()
386+
initialTip, _ := g.GetTip(trunk)
387+
388+
// Add a commit to move trunk forward
389+
os.WriteFile(filepath.Join(dir, "new.txt"), []byte("new"), 0644)
390+
exec.Command("git", "-C", dir, "add", ".").Run()
391+
exec.Command("git", "-C", dir, "commit", "-m", "second commit").Run()
392+
secondTip, _ := g.GetTip(trunk)
393+
394+
// initial is ancestor of second
395+
if !g.IsAncestor(initialTip, secondTip) {
396+
t.Errorf("expected %s to be ancestor of %s", initialTip[:7], secondTip[:7])
397+
}
398+
399+
// second is NOT ancestor of initial
400+
if g.IsAncestor(secondTip, initialTip) {
401+
t.Errorf("expected %s to NOT be ancestor of %s", secondTip[:7], initialTip[:7])
402+
}
403+
404+
// commit is its own ancestor
405+
if !g.IsAncestor(initialTip, initialTip) {
406+
t.Errorf("expected commit to be its own ancestor")
407+
}
408+
409+
// divergent branches are not ancestors of each other
410+
exec.Command("git", "-C", dir, "checkout", "-b", "diverge", initialTip).Run()
411+
os.WriteFile(filepath.Join(dir, "diverge.txt"), []byte("diverge"), 0644)
412+
exec.Command("git", "-C", dir, "add", ".").Run()
413+
exec.Command("git", "-C", dir, "commit", "-m", "diverge commit").Run()
414+
divergeTip, _ := g.GetTip("diverge")
415+
416+
if g.IsAncestor(divergeTip, secondTip) {
417+
t.Error("divergent branch tip should not be ancestor of trunk tip")
418+
}
419+
if g.IsAncestor(secondTip, divergeTip) {
420+
t.Error("trunk tip should not be ancestor of divergent branch tip")
421+
}
422+
}
423+
381424
func TestRebaseOnto(t *testing.T) {
382425
dir := setupTestRepo(t)
383426
g := git.New(dir)

0 commit comments

Comments
 (0)