Skip to content

feat(adopt): allow re-adoption of branches #117

Merged
boneskull merged 1 commit intomainfrom
issue-114
May 8, 2026
Merged

feat(adopt): allow re-adoption of branches #117
boneskull merged 1 commit intomainfrom
issue-114

Conversation

@boneskull
Copy link
Copy Markdown
Owner

@boneskull boneskull commented May 7, 2026

This change allows gh stack adopt to change the parent branch. Previously, it would error out if the current branch was already tracked.

Closes #114

@claude
Copy link
Copy Markdown

claude Bot commented May 7, 2026

Claude finished @boneskull's task in 1m 17s —— View job


Code Review

  • Gather context
  • Read changed files
  • Review logic in cmd/adopt.go
  • Review tests in cmd/adopt_test.go and e2e/adopt_orphan_test.go
  • Review README.md changes
  • Post review feedback

The implementation is clean and the core logic is correct. A few things worth flagging:

cmd/adopt.go — Logic

The structure looks good. The early exit for the no-op case (same parent) correctly uses WarningIcon(), fork point is recalculated on reparent, and cycle detection runs against the current tree state before mutation. One edge worth considering: the cycle check at adopt.go:89-96 walks ancestors of parent in the tree built from the current config state. Since the new parent hasn't been set yet, this is correct — but it also means the check only fires if parent is already in the tree (i.e. parentNode != nil). If parent is the trunk, parentNode will be nil and the loop is skipped entirely, which is safe because trunk can never be a descendant. No bug here, just worth documenting.

cmd/adopt_test.go — Unit Tests

TestAdoptNoOpWhenParentUnchanged is a dead-code test (adopt_test.go:69-97). It always passes trivially: the condition at line 92 (if parent == trunk) always holds (because GetParent was just asserted to return trunk on line 87), so the function always returns at line 94. The t.Error(...) on line 96 is unreachable. Fix this →

TestAdoptReparentsAlreadyTracked (adopt_test.go:40-67) tests cfg.SetParent directly, not runAdopt. It verifies that the config package can overwrite a parent — not that the adopt command's new logic (no-op detection, fork point update, success messaging) works correctly. This is consistent with other unit tests here, so it may be intentional, but it leaves the actual command behavior uncovered at the unit level.

e2e/adopt_orphan_test.go — E2E Tests

TestAdoptReparentsTrackedBranch is solid — exercises the full CLI path and checks stdout mentions both the old and new parent branch names.

TestAdoptNoOpWhenParentUnchangedE2E verifies exit code 0 on repeat adopt but doesn't assert the warning message in stdout. Minor, but asserting result.ContainsStdout("already tracked") or similar would make intent clearer.

Missing coverage: There's no test for cycle detection during reparent — e.g. trunk → A → B, then adopt --branch A B should fail with "would create a cycle". The cycle detection path already exists in the code but isn't exercised for the reparent case.

README.md

The added sentence is clear and accurate. No issues.


Overall: The implementation is correct and the e2e test TestAdoptReparentsTrackedBranch provides meaningful coverage of the key new behavior. The unit test issues above are the main things worth addressing before merge.

@boneskull boneskull changed the title feat(adopt): allow re-adoption of branches This change allows gh stack adopt to _change_ the parent branch. Previously, it would error out if the current branch was already tracked. feat(adopt): allow re-adoption of branches May 7, 2026
Copilot AI review requested due to automatic review settings May 7, 2026 23:02
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates gh stack adopt so that re-adopting an already-tracked branch updates its configured parent branch (instead of erroring), and adds coverage to ensure reparenting and cycle prevention work end-to-end.

Changes:

  • Allow adopt to reparent already-tracked branches and print a message indicating the old and new parent.
  • Make adopt a no-op (successful) when the requested parent is unchanged, with a warning message.
  • Add E2E coverage for reparenting, no-op behavior, and cycle detection; update README docs accordingly.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
cmd/adopt.go Implements re-adoption behavior (reparent/no-op) and user-facing messaging.
e2e/adopt_orphan_test.go Adds E2E tests for reparenting, no-op adopt, and cycle detection failures.
cmd/adopt_test.go Updates unit test to reflect reparenting semantics.
README.md Documents that adopt can update a tracked branch’s parent (or no-op if unchanged).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cmd/adopt.go
Comment on lines +100 to +104
// No-op: already tracked with the same parent
if alreadyTracked && oldParent == parent {
fmt.Printf("%s Branch %s is already tracked with parent %s\n", s.WarningIcon(), s.Branch(branchName), s.Branch(parent))
return nil
}
This change allows `gh stack adopt` to _change_ the parent branch. Previously, it would error out if the current branch was already tracked.
@boneskull boneskull merged commit 2b49c2b into main May 8, 2026
7 checks passed
@boneskull boneskull deleted the issue-114 branch May 8, 2026 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

adopt: should not fail if already tracked

2 participants