fix: Bug switching to trunk with worktrees enabled#91
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR adds important safeguards for trunk branch and primary worktree handling. The changes are generally well-structured, but there are a few areas that need attention:
Key Findings
Critical Issues:
-
Primary worktree deletion logic (pkg/yas/branch.go:87): When a branch is in the primary worktree, the code falls through to
deleteNonWorktreeBranch(), which may fail if the branch is currently checked out. This needs explicit handling. -
Potential duplicate worktree creation (pkg/yas/worktrees.go:53): The conditional logic may allow creating a second worktree if a branch is already checked out in a different linked worktree (not primary). Git will reject this.
Improvements Needed:
- Missing test coverage for the new
BranchHasWorktree()function - Comment accuracy issues in
worktrees.gothat could mislead future maintainers - Complex conditional logic that would benefit from using the new
BranchHasWorktree()helper
Good Changes:
- Trunk branch deletion protection is excellent defensive programming
- Early return for trunk in
EnsureLinkedWorktreeForBranch()prevents "already used" errors - Primary worktree detection logic is sound
Recommended Actions
-
Add test coverage for edge cases, especially:
- Deleting a branch checked out in primary worktree
- Ensuring worktree when branch exists in another linked worktree
-
Fix the primary worktree deletion flow to handle the checked-out case
-
Simplify the worktree existence check using the new
BranchHasWorktree()helper
The core improvements are valuable, but the edge cases need handling before merge to avoid runtime errors.
| } | ||
| } | ||
|
|
||
| return false, nil |
There was a problem hiding this comment.
Consider adding a test for this function
The new BranchHasWorktree function is straightforward, but would benefit from unit tests covering:
- Branch exists in primary worktree
- Branch exists in linked worktree
- Branch doesn't exist in any worktree
- Error handling when
Worktrees()fails
This will help ensure the function behaves correctly across different scenarios.
| func (yas *YAS) DeleteBranch(branchName string, force bool) error { | ||
| // Refuse to delete trunk branch | ||
| if branchName == yas.cfg.TrunkBranch { | ||
| return errors.New("refusing to delete trunk branch") |
There was a problem hiding this comment.
Good safety check!
Adding validation to prevent deleting the trunk branch is excellent defensive programming. This prevents accidental data loss.
Optional improvement: Consider returning a more specific error type (e.g., ErrCannotDeleteTrunk) that could be checked with errors.Is() for better error handling by callers.
| } | ||
|
|
||
| if !isPrimary { | ||
| return yas.deleteWorktreeBranch(worktreePath, branchName, force) |
There was a problem hiding this comment.
Logic flow concern: Primary worktree branch deletion
When a branch is checked out in the primary worktree (isPrimary == true), the code falls through to deleteNonWorktreeBranch(). However, this seems incorrect because:
- The branch IS in a worktree (the primary one)
deleteNonWorktreeBranch()assumes no worktree and tries to delete the branch directly
This could fail if the branch is currently checked out. Should this case either:
- Return an error asking the user to switch to a different branch first?
- Automatically switch to trunk before deletion?
- Handle this case explicitly rather than falling through?
Test case needed: Try deleting a branch that's checked out in the primary worktree.
| // This also handles the case where trunk is already checked out in primary worktree, | ||
| // which would otherwise cause "branch is already used by worktree" error. | ||
| if branchName == yas.cfg.TrunkBranch { | ||
| return nil |
There was a problem hiding this comment.
Good edge case handling
Preventing worktree creation for the trunk branch makes sense and avoids the "branch is already used by worktree" error. This is a clean solution.
Minor suggestion: The comment could be more specific about why trunk should stay in primary:
// Don't create a worktree for the trunk branch - it must remain in the primary
// worktree as the stable base for the repository. Creating a linked worktree
// would cause "branch is already used by worktree" errors.| } | ||
|
|
||
| // Check if the existing worktree is the primary repo (not a separate worktree) | ||
| // Check if the found worktree is different from where we're running |
There was a problem hiding this comment.
Comment accuracy issue
The comment says "Check if the found worktree is different from where we're running" but the code actually checks if it's different from RepoDirectory, not necessarily "where we're running" (which would be the current working directory).
Suggested revision:
// Check if the existing worktree path differs from the repository root directoryThis is more precise and matches what the code actually does.
| // Worktree already exists for this branch in a DIFFERENT location than current directory. | ||
| // If the branch is just temporarily checked out in the current worktree (same as RepoDirectory), | ||
| // we still want to create a dedicated worktree for it. | ||
| if existingWorktreePath != "" && existingResolved != repoResolved { |
There was a problem hiding this comment.
Complex conditional logic needs clarification
This condition is doing a lot and the multi-line comment indicates it's handling a subtle case. The logic seems to be:
"Return early (don't create worktree) if a worktree exists AND it's not the current directory"
However, this is confusing because:
- If
existingWorktreePath == "", the condition short-circuits (correct) - If
existingResolved == repoResolved, we continue to create a new worktree
Potential issue: What happens if the branch is already checked out in a different linked worktree, not the primary? The code would try to create another worktree for the same branch, which git will reject.
Suggestion:
- Add a test case for: branch checked out in linked worktree A, try to ensure worktree from linked worktree B
- Consider using the new
BranchHasWorktree()helper for clarity:
hasWorktree, err := yas.git.BranchHasWorktree(branchName)
if err != nil {
return fmt.Errorf("failed to check if branch has worktree: %w", err)
}
if hasWorktree && existingResolved != repoResolved {
return nil // Branch already has a worktree elsewhere
}
There was a problem hiding this comment.
Summary
This PR successfully fixes the bug where yas br <trunk> would fail with "branch already used by worktree" when worktree-branch mode is enabled. The fix is well-implemented with good defensive programming.
Key Changes
✅ Core fix: EnsureLinkedWorktreeForBranch now skips worktree creation for trunk branch (pkg/yas/worktrees.go:28-29)
✅ Defensive check: DeleteBranch now refuses to delete trunk (pkg/yas/branch.go:50-52)
✅ Primary worktree handling: DeleteBranch correctly handles deletion when branch is in primary worktree (pkg/yas/branch.go:75-87)
✅ Excellent test coverage: Integration test documents bug and prevents regression (test/worktree_test.go:677-746)
✅ Improved comments: Better documentation of path comparison logic
Minor Notes
BranchHasWorktreefunction (pkg/gitexec/worktrees.go:135-149) is added but unused - consider removing or documenting its future purpose- Error message for trunk deletion could include the branch name for clarity
Verdict
The PR is well-structured, addresses the root cause correctly, and includes comprehensive testing. The code quality is high with clear comments explaining the intent. Ready to merge after considering the minor feedback above.
| } | ||
| } | ||
|
|
||
| return false, nil |
There was a problem hiding this comment.
The new BranchHasWorktree function looks correct but isn't actually used in this PR. Consider:
- If it's needed for future work, mention that in the PR description
- If it's not needed, remove it to keep the PR focused
The function itself is well-implemented with proper error handling and clear logic.
| func (yas *YAS) DeleteBranch(branchName string, force bool) error { | ||
| // Refuse to delete trunk branch | ||
| if branchName == yas.cfg.TrunkBranch { | ||
| return errors.New("refusing to delete trunk branch") |
There was a problem hiding this comment.
Good defensive check! This prevents accidentally deleting the trunk branch.
Minor suggestion: Consider using fmt.Errorf for consistency with other errors in the codebase:
| return errors.New("refusing to delete trunk branch") | |
| return fmt.Errorf("refusing to delete trunk branch: %s", yas.cfg.TrunkBranch) |
This provides more context about which branch is being protected.
| } | ||
|
|
||
| if !isPrimary { | ||
| return yas.deleteWorktreeBranch(worktreePath, branchName, force) |
There was a problem hiding this comment.
This logic change is subtle and important. The old code always tried to delete as a worktree branch if worktreePath != "". The new code now checks if it's the primary worktree first.
This correctly handles the case where trunk is checked out in the primary worktree. However, the logic flow might be clearer with an early return:
| return yas.deleteWorktreeBranch(worktreePath, branchName, force) | |
| if isPrimary { | |
| // Can't use git worktree remove on primary - fall through to regular deletion | |
| } else { | |
| return yas.deleteWorktreeBranch(worktreePath, branchName, force) | |
| } |
Actually, on second thought, your current implementation is fine and the comment helps clarify intent.
| // This also handles the case where trunk is already checked out in primary worktree, | ||
| // which would otherwise cause "branch is already used by worktree" error. | ||
| if branchName == yas.cfg.TrunkBranch { | ||
| return nil |
There was a problem hiding this comment.
Excellent fix! This is the core of the issue - preventing worktree creation for the trunk branch.
The comment clearly explains both:
- Why we don't create a worktree for trunk (it stays in primary)
- What error this prevents
This is a clean solution that handles the edge case at the right layer of abstraction.
| contentStr := string(content) | ||
| // Should switch to primary repo and run yas br main there | ||
| assert.Assert(t, cmp.Contains(contentStr, "yas br main")) | ||
| } |
There was a problem hiding this comment.
Excellent integration test! This test:
- ✅ Clearly documents the bug scenario in the function name and comments
- ✅ Sets up realistic conditions (worktree-branch config enabled)
- ✅ Tests the exact failure case (
yas br mainfrom a worktree) - ✅ Verifies both success and expected side effects (no worktree for main)
The test structure is clean and follows the existing patterns in the codebase. This will prevent regression of this bug.
| } | ||
|
|
||
| // Check if the existing worktree is the primary repo (not a separate worktree) | ||
| // Check if the found worktree is different from where we're running |
There was a problem hiding this comment.
These updated comments significantly improve clarity about what this check is actually doing. The old comment mentioned "primary repo" but that was misleading - this is about comparing paths regardless of which is primary.
The new wording "different from where we're running" is more accurate to the actual comparison being performed.
1 participant
No description provided.