fix(orphan): allow orphaning branches whose parent link is invalid#123
Open
mvanhorn wants to merge 2 commits into
Open
fix(orphan): allow orphaning branches whose parent link is invalid#123mvanhorn wants to merge 2 commits into
mvanhorn wants to merge 2 commits into
Conversation
Per boneskull#116, if a tracked branch's recorded parent is missing or is itself untracked, tree.Build silently drops the parent-child wiring (the `continue` at internal/tree/tree.go:50). The branch's node stays in nodes[] but isn't connected to root, so tree.FindNode walking root's Children never reaches it and runOrphan reports the disconnected branch as "not tracked" - exactly the symptom in the issue. A disconnected branch IS tracked - the stackParent config entry is still there - so refusing to orphan it leaves the user stuck with a config entry they can't clean up through the CLI. When FindNode returns nil, fall back to ListTrackedBranches and treat the branch as orphan-able when its name appears there. The cleanup path is the same RemoveParent/RemovePR/RemoveForkPoint/RemovePRBase sequence the normal flow runs. The children check is safe to skip on this branch: tree.Build wires children up from parent links pointing AT this branch, and Build's parent-link miss already kept this branch out of any parent's Children list. So a disconnected branch with descendants would be caught the same way by Build - none of them appear as children. A new e2e test in e2e/orphan_test.go simulates the scenario by setting a stackParent that points at a missing branch, then runs orphan and asserts both that the command succeeds and that the stackParent config entry is cleared. Closes boneskull#116
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates gh stack orphan to treat a branch as orphan-able when its recorded stackParent points to a missing/untracked parent (i.e., the branch is tracked in git config but disconnected from the in-memory stack tree), addressing issue #116.
Changes:
- Add a fallback path in
orphanto allow cleanup whentree.FindNodecan’t locate a tracked branch due to a broken parent link. - Add an e2e regression test covering orphaning a disconnected tracked branch.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cmd/orphan.go |
Adds a fallback “tracked-but-disconnected” orphan path when FindNode returns nil. |
e2e/orphan_test.go |
Adds an e2e test to verify orphan succeeds for a disconnected tracked branch and clears stackParent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+76
to
+85
| // A disconnected branch has no children in the in-memory tree | ||
| // (children require a parent link from this branch, which Build | ||
| // would have wired up before disconnecting it), so drop straight | ||
| // into the cleanup path without the children check. | ||
| s := style.New() | ||
| _ = cfg.RemoveParent(branchName) //nolint:errcheck // best effort cleanup | ||
| _ = cfg.RemovePR(branchName) //nolint:errcheck // best effort cleanup | ||
| _ = cfg.RemoveForkPoint(branchName) //nolint:errcheck // best effort cleanup | ||
| _ = cfg.RemovePRBase(branchName) //nolint:errcheck // best effort cleanup | ||
| fmt.Printf("%s Orphaned %s\n", s.SuccessIcon(), s.Branch(branchName)) |
| if v := env.GetStackConfig("branch.feat-a.stackParent"); v != "" { | ||
| t.Errorf("expected feat-a stackParent cleared after orphan, got %q", v) | ||
| } | ||
| } |
Owner
|
@mvanhorn Are you able to work on this further? If not, I can take over the work. |
Address Copilot review feedback: a branch whose parent link is broken can still have tracked descendants wired up via git config, so the disconnected-branch fallback no longer shortcuts past the safety check. Rebuild the descendant subtree from tracked stackParent entries (with a cycle guard) and reuse the normal path: orphaning with descendants requires --force, and --force orphans the descendants too. Adds an e2e case for the disconnected-branch-with-descendants scenario.
Contributor
Author
|
Yes - sorry for the delay, picking this back up. Both Copilot points are addressed in 9911438: the disconnected-branch path no longer shortcuts past the children check. It rebuilds the branch's descendant subtree from the tracked stackParent entries (with a cycle guard) and falls through to the normal logic, so descendants require --force and get orphaned properly with it. Added the e2e case for a disconnected branch with tracked descendants (fails without --force, orphans the child with it). Full go test ./... passes locally. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix #116:
gh stack orphannow succeeds when the current branch's recorded parent is missing or itself untracked, instead of reporting the branch as "not tracked".Why this matters
The reporter asked the contributor to evaluate why the orphan check was misfiring, and what the consequences of removing it would be. Tracing through the path:
tree.Build(internal/tree/tree.go:46-54) silentlycontinues when a tracked branch's recorded parent is not itself in the tree map:The branch's own node is in
nodes[branch], but it is never wired into any parent'sChildrenslice and its ownParentpointer staysnil.tree.FindNode(root, branchName)(internal/tree/tree.go:76-87) walksroot.Childrenrecursively, so the disconnected node is unreachable - the function returnsnil.runOrphan(cmd/orphan.go:60-63) then translatesniltobranch %q is not tracked, which contradicts the git config that still recordsbranch.<name>.stackParent.The branch IS tracked. The link is just dangling. Treating that as "not tracked" leaves the user with an orphaned config entry the CLI cannot remove, which is the exact symptom #116 reports.
The fix preserves the existing safety check (a branch that has no
stackParentconfig key at all still hits the "not tracked" error) but falls back tocfg.ListTrackedBranches()whenFindNodereturns nil and the user is allowed to clean up the dangling entry. The children check is safe to skip on this branch: theBuildstep that disconnected this branch from root also kept it out of any descendant'sParentslot, so a disconnected node never has the kind of children that would silently take an orphan along for the ride.Testing
go test ./...is green (the newTestOrphanDisconnectedBranchIsAllowede2e case lands the regression net).golangci-lint run ./cmd/...reports0 issues.The new e2e test mirrors the exact scenario from #116: create
feat-aoffmain, then manually rewritebranch.feat-a.stackParentto point at a non-existent branch, then rungh stack orphan feat-aand assert the config entry is cleared. Without the patch the same test fails withbranch "feat-a" is not tracked.Closes #116