Reject non-empty worktree targets before branch creation - #14244
Conversation
Validate fresh worktree paths before issue develop creates a linked branch, preventing orphaned remote branches when checkout cannot proceed. Co-authored-by: Copilot App <[email protected]>
There was a problem hiding this comment.
Pull request overview
Rejects non-empty worktree targets before linked branch creation, preventing orphaned branches.
Changes:
- Validates fresh worktree targets are empty.
- Preserves reuse of linked worktrees.
- Adds shared and command-level regression tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cmd/pr/shared/worktree.go |
Adds target emptiness validation. |
pkg/cmd/pr/shared/worktree_test.go |
Tests non-empty target rejection. |
pkg/cmd/issue/develop/develop_test.go |
Verifies rejection precedes branch creation. |
Review details
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
Protect linked worktree reuse from the fresh-target emptiness guard. Co-authored-by: Copilot App <[email protected]> Copilot-Session: 62a807cf-3d2d-42a5-a327-cc451efa7a59
babakks
left a comment
babakks
left a comment
There was a problem hiding this comment.
Thanks for the thorough follow-up here, @tidy-dev! 🙏
The fix looks solid. Putting the emptiness check in the shared ResolveWorktreeTarget keeps issue develop and pr checkout consistent, and gating it on !reuse correctly leaves existing linked worktrees reusable. Verifying that validation runs before CreateLinkedBranch (so no orphan linked branch is left behind) is exactly the right thing to prove, and the unit test that registers no branch mutation nails it.
The only thing I felt was missing is acceptance coverage for the non-empty target case. I'm going to push a couple of acceptance tests for that shortly (one under issue develop, one under pr checkout), so no action needed on your end there. Once those land I think this is good to go. 🚀
Add acceptance coverage for rejecting an existing non-empty directory as a worktree target. The issue develop case also asserts no orphan linked branch is left behind, and the pr checkout case asserts the early, clear error. Co-authored-by: Copilot <[email protected]>
|
Enabling auto-merge as the updated acceptance tests pass. |
Follow-up to #14136 (comment)
Description
gh issue develop --checkout --worktreecreated its linked branch before Git rejected an existing non-empty target directory. This could leave an orphaned linked branch without a local worktree.Reject existing non-empty directories while resolving a fresh worktree target, before
gh issue developcreates or reuses any linked branch. Existing linked worktrees remain reusable.How did you test this change?
Given an existing directory containing a file
When I target it with
gh issue develop --checkout --worktreeThen the command reports that the worktree path must be empty before creating a linked branch.
Given an empty or non-existent target directory
When I resolve it as a new worktree target
Then checkout proceeds as before.
Given an existing linked worktree
When I target its root
Then the worktree remains eligible for reuse even though it contains files.
Key points
The validation lives in the shared worktree resolver so
gh issue developandgh pr checkoutretain consistent target semantics. The non-empty check runs only after determining that the target is not an existing linked worktree.Notes for reviewers
Start with
pkg/cmd/pr/shared/worktree.go. The regression test inpkg/cmd/issue/develop/develop_test.godeliberately registers no linked-branch mutation, proving validation happens before branch creation.This follows up on the deferred edge case identified during review of #14136.
Authorship and follow-up
Who wrote this:
Who answers review comments: