From d0777ab0bf5dcddf8731421fbb5ad5bffce11227 Mon Sep 17 00:00:00 2001 From: Andrew Lee Date: Sun, 14 Jun 2026 15:57:40 -0700 Subject: [PATCH] refactor: drop undo's command-label coupling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit undo_op.go was the one place the otherwise command-agnostic undo engine branched on a literal command label (if entry.Label == "rename"), flagged as debt in its own comment. That branch only recomputed the post-undo checkout for a renamed current branch — but the generic restore already lands HEAD on entry.CurrentBranch (the branch checked out when the command ran), which for a current-branch rename is the restored old name and for a create undo is the parent. The undo oracle in the model test even defines correct post-undo HEAD as entry.CurrentBranch, so a label-specific derivation could only ever agree with it. So drop the label branch and the two helpers it used (restoredRenameTarget, missingRestoredRef), now dead, and rely on the generic restore. Add an s==nil rename-recovery test for the cmd loadErr path (Undo called with a nil state), where the generic entry.CurrentBranch restore still lands on the old name. The full engine suite — including the random invariant model that fuzzes rename+undo and asserts HEAD == entry.CurrentBranch — stays green. --- internal/stack/undo_op.go | 48 +++++----------------------------- internal/stack/undo_op_test.go | 32 +++++++++++++++++++++++ 2 files changed, 38 insertions(+), 42 deletions(-) diff --git a/internal/stack/undo_op.go b/internal/stack/undo_op.go index 2a8e97a..67b0b98 100644 --- a/internal/stack/undo_op.go +++ b/internal/stack/undo_op.go @@ -54,14 +54,12 @@ func Undo(env Env, s *State, entry *UndoEntry) (*OpResult, error) { } } if cur, err := g.CurrentBranch(); err == nil && cur == name { - // The rename target is still derived from the entry label; the - // follow-up is to derive it from the state diff instead. - if entry.Label == "rename" { - checkoutAfterRestore = restoredRenameTarget(&prev, s, name) - if checkoutAfterRestore == "" { - checkoutAfterRestore = missingRestoredRef(g, entry) - } - } + // HEAD is on a branch we are about to delete; move it to target (the + // parent, or trunk) so the branch can be removed. The final landing + // branch is restored below from entry.CurrentBranch (the branch that + // was checked out when the command ran) — for a current-branch rename + // that is the restored old name, so no command-label coupling is + // needed here. if !g.BranchExists(target) { sha, ok := entry.Refs[target] if !ok { @@ -140,40 +138,6 @@ func checkoutBlockedByLocalChanges(err error) bool { return strings.Contains(msg, "local changes") || strings.Contains(msg, "would be overwritten") } -// restoredRenameTarget picks the branch to check out after undoing a rename: -// the snapshot name that the current state no longer tracks (or the snapshot -// trunk when the trunk itself was renamed). -func restoredRenameTarget(prev, current *State, deleted string) string { - if current == nil { - return "" - } - if current.Trunk == deleted && prev.Trunk != current.Trunk { - return prev.Trunk - } - for name := range prev.Branches { - if !current.IsTracked(name) { - return name - } - } - return "" -} - -// missingRestoredRef returns the single recorded ref whose branch is currently -// missing, or "" when there is not exactly one. -func missingRestoredRef(g Git, entry *UndoEntry) string { - var missing []string - for name := range entry.Refs { - if !g.BranchExists(name) { - missing = append(missing, name) - } - } - sort.Strings(missing) - if len(missing) == 1 { - return missing[0] - } - return "" -} - // branchCreatedByEntry reports whether name was created by the command the // entry snapshots: it is listed in CreatedBranches, or absent from the // local-branch list captured before the command ran. diff --git a/internal/stack/undo_op_test.go b/internal/stack/undo_op_test.go index 85dced5..4a369ce 100644 --- a/internal/stack/undo_op_test.go +++ b/internal/stack/undo_op_test.go @@ -125,6 +125,38 @@ func TestUndoRenameRestoresOldNameAndChecksItOut(t *testing.T) { assertUndoRestored(t, f, s, entry) } +// TestUndoRenameWithNilStateRecoversOldName covers the cmd loadErr path (Undo is +// called with s==nil when the state can't be loaded): the generic current-branch +// restore must still land on the restored old name (entry.CurrentBranch), with no +// loaded state to consult. +func TestUndoRenameWithNilStateRecoversOldName(t *testing.T) { + f, s, env := newEnvState() + mkBranch(t, env, s, f, "main", "a") + mkBranch(t, env, s, f, "a", "b") + if err := f.Checkout("a"); err != nil { + t.Fatal(err) + } + entry := mustSnapshot(t, s, f, "rename") + if _, err := Rename(env, s, "a", "z"); err != nil { + t.Fatalf("rename: %v", err) + } + // FinalizeUndo records the created branch on the persisted entry; with s==nil + // it is the only signal that "z" must be deleted. + entry.CreatedBranches = []string{"z"} + if _, err := Undo(env, nil, entry); err != nil { // s==nil: Load failed + t.Fatalf("undo (nil state): %v", err) + } + if f.BranchExists("z") { + t.Fatal("undo left the renamed branch z behind") + } + if !f.BranchExists("a") { + t.Fatal("undo did not restore the old branch name a") + } + if f.head != "a" { + t.Fatalf("HEAD = %q after undoing rename with nil state, want a", f.head) + } +} + func TestUndoDeleteResurrectsBranchFromSnapshotRef(t *testing.T) { f, s, env := newEnvState() mkBranch(t, env, s, f, "main", "a")