Skip to content

Commit acf1fec

Browse files
committed
Persist fold-branch removal when a post-fold cascade rebase conflicts
ContinueApply removes the folded branch from the in-memory stack after a fold-down cherry-pick is resolved, but a subsequent cascade rebase conflict only saved the modify state file, not the stack metadata. On the next --continue the on-disk metadata (folded branch still present) was re-read, and because ConflictType is now "rebase" the fold-removal block was skipped, so the final save resurrected the folded branch as a phantom entry pointing at an orphaned tip. Persist the stack file alongside the state file on a cascade-rebase conflict, mirroring ApplyPlan's save-on-conflict, so the fold removal survives recovery. Adds an end-to-end regression test covering the fold-then-cascade-conflict path across two --continue calls.
1 parent 65b738f commit acf1fec

2 files changed

Lines changed: 98 additions & 0 deletions

File tree

internal/modify/apply.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -916,6 +916,16 @@ func ContinueApply(
916916
state.AffectsPRs = affectsPRs
917917
_ = SaveState(gitDir, state)
918918

919+
// Persist the stack metadata so far. A fold-down removes the
920+
// folded branch from the in-memory stack (above) before the
921+
// cascade rebase runs. If we don't save it here, the next
922+
// --continue re-reads the on-disk metadata (folded branch still
923+
// present) and — because ConflictType is now "rebase" — skips the
924+
// fold-removal block, silently resurrecting the folded branch as a
925+
// phantom entry. Mirrors ApplyPlan's save-on-conflict.
926+
if saveErr := stack.SaveWithLock(gitDir, sf, lock); saveErr != nil {
927+
cfg.Warningf("failed to save stack metadata: %v", saveErr)
928+
}
919929
cfg.Warningf("Conflict rebasing %s", branchName)
920930
if files, ferr := git.ConflictedFiles(); ferr == nil {
921931
for _, f := range files {

internal/modify/apply_test.go

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1329,6 +1329,94 @@ func TestContinueApply_SubsequentConflictBecomesRebase(t *testing.T) {
13291329
assert.Equal(t, "C", got.ConflictBranch)
13301330
}
13311331

1332+
// Regression test for the review on PR #167: after an initial fold-down
1333+
// (cherry-pick) conflict is resolved, a subsequent cascade rebase conflict must
1334+
// persist the fold-branch removal to disk. Otherwise the next --continue
1335+
// re-reads stale on-disk metadata and — because ConflictType is now "rebase" —
1336+
// skips the fold-removal step, silently resurrecting the folded branch as a
1337+
// phantom entry once recovery completes.
1338+
func TestContinueApply_FoldThenCascadeConflict_DoesNotResurrectFoldedBranch(t *testing.T) {
1339+
s := stack.Stack{
1340+
Trunk: stack.BranchRef{Branch: "main"},
1341+
Branches: []stack.BranchRef{
1342+
{Branch: "A"},
1343+
{Branch: "B"},
1344+
{Branch: "C"},
1345+
},
1346+
}
1347+
1348+
gitDir := t.TempDir()
1349+
writeTestStackFile(t, gitDir, s)
1350+
1351+
// State written by ApplyPlan when the fold-down of B into A conflicts on
1352+
// cherry-pick. B is still present in the on-disk metadata at this point.
1353+
state := &StateFile{
1354+
SchemaVersion: 1,
1355+
StackName: "main",
1356+
StackIndex: 0,
1357+
Phase: PhaseConflict,
1358+
ConflictType: "cherry_pick",
1359+
ConflictBranch: "B",
1360+
FoldBranch: "B",
1361+
FoldTarget: "A",
1362+
RemainingBranches: []string{"A", "C"},
1363+
OriginalBranch: "A",
1364+
OriginalRefs: map[string]string{"A": "sha-main", "C": "sha-A-old"},
1365+
}
1366+
require.NoError(t, SaveState(gitDir, state))
1367+
1368+
mock := newApplyMock(gitDir, map[string]string{
1369+
"main": "sha-main", "A": "sha-A", "B": "sha-B", "C": "sha-C",
1370+
})
1371+
mock.CherryPickContinueFn = func() error { return nil }
1372+
mock.IsRebaseInProgressFn = func() bool { return true }
1373+
mock.RebaseContinueFn = func(git.RebaseOpts) error { return nil }
1374+
// C conflicts on its first rebase attempt, then succeeds (user resolved it).
1375+
cRebases := 0
1376+
mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error {
1377+
if branch == "C" {
1378+
cRebases++
1379+
if cRebases == 1 {
1380+
return assert.AnError
1381+
}
1382+
}
1383+
return nil
1384+
}
1385+
mock.ConflictedFilesFn = func() ([]string, error) { return []string{"c.go"}, nil }
1386+
1387+
restore := git.SetOps(mock)
1388+
defer restore()
1389+
1390+
cfg, _, _ := config.NewTestConfig()
1391+
defer cfg.Out.Close()
1392+
defer cfg.Err.Close()
1393+
1394+
// First --continue: finishes the fold, then conflicts rebasing C.
1395+
err := ContinueApply(cfg, gitDir, noopUpdateBaseSHAs)
1396+
require.Error(t, err)
1397+
1398+
// The fold-branch removal must already be persisted on disk, even though
1399+
// the cascade hit a conflict.
1400+
afterFirst, err := stack.Load(gitDir)
1401+
require.NoError(t, err)
1402+
assert.Equal(t, -1, afterFirst.Stacks[0].IndexOf("B"),
1403+
"folded branch B must not be present on disk after the cascade conflict")
1404+
1405+
// Second --continue: rebase resolves and recovery completes.
1406+
err = ContinueApply(cfg, gitDir, noopUpdateBaseSHAs)
1407+
require.NoError(t, err)
1408+
1409+
final, err := stack.Load(gitDir)
1410+
require.NoError(t, err)
1411+
names := make([]string, len(final.Stacks[0].Branches))
1412+
for i, b := range final.Stacks[0].Branches {
1413+
names[i] = b.Branch
1414+
}
1415+
assert.Equal(t, []string{"A", "C"}, names,
1416+
"folded branch B must stay removed after recovery completes")
1417+
assert.False(t, StateExists(gitDir), "state should be cleared after successful recovery")
1418+
}
1419+
13321420
// ─── Unwind restores renamed branch ─────────────────────────────────────────
13331421

13341422
func TestUnwind_RestoresRenamedBranch(t *testing.T) {

0 commit comments

Comments
 (0)