fix: properly handle errors when removing dirty worktrees #10
Conversation
This change dramatically simplifies the `wtp add` command interface by removing complex features and flags that added cognitive overhead without significant value for most users. ## Removed Features - `--detach` flag and detached HEAD support - `--track` flag (auto-tracking of remote branches is now automatic) - `--force` flag (users can use git worktree directly for edge cases) - `--cd`/`--no-cd` flags (shell integration handles directory switching) - `wtp add <worktree-name> <commit-ish>` pattern (ambiguous with branch names) ## New Simplified Interface - `wtp add <existing-branch>` - Create worktree from existing local/remote branch - `wtp add -b <new-branch>` - Create new branch and worktree from HEAD - `wtp add -b <new-branch> <commit>` - Create new branch from specific commit ## Benefits - Reduced complexity: Only 1 flag instead of 6 - Consistent with git-checkout pattern (`-b` for new branches) - Eliminates ambiguous argument parsing - Maintains all common workflows while removing edge cases - Better error messages and user guidance ## Implementation Details - Removed 100+ lines of complex argument parsing logic - Simplified test suite by removing tests for deleted features - Updated documentation and help text - Enhanced success messages with friendly emojis and guidance - Maintained backward compatibility for core use cases This change follows the principle that software should be easy to use correctly and hard to use incorrectly. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
The remove command now correctly shows error messages when attempting to: - Remove worktrees with uncommitted changes (without --force) - Remove branches with unmerged commits (without --force-branch) Previously, these errors were silently ignored due to incorrect error handling in the command executor. The fix ensures users receive helpful error messages suggesting the use of appropriate force flags. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThis update streamlines the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant AddCmd
participant Git
User->>CLI: wtp add [options] <branch> [commit-ish]
CLI->>AddCmd: Parse args, validate input
AddCmd->>Git: Resolve branch/tracking (if -b used)
AddCmd->>Git: git worktree add ...
Git-->>AddCmd: Worktree created or error
AddCmd->>CLI: Display enhanced success or error message
CLI-->>User: Output result
sequenceDiagram
participant User
participant CLI
participant RemoveCmd
participant Git
User->>CLI: wtp remove <worktree/branch>
CLI->>RemoveCmd: Parse args
RemoveCmd->>Git: git worktree remove ...
Git-->>RemoveCmd: Result (success or error)
RemoveCmd->>RemoveCmd: Inspect result for errors/output
RemoveCmd->>CLI: Display error or success message
CLI-->>User: Output result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes error handling in the wtp remove command to properly display error messages when attempting to remove worktrees with uncommitted changes. Previously, the command would incorrectly report success while leaving the worktree in place.
Key changes include:
- Fixed error handling to check individual command results rather than just overall execution status
- Updated error pattern matching to recognize the actual git error message "contains modified or untracked files"
- Added comprehensive test coverage for dirty worktree removal scenarios and branch removal with unmerged commits
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/wtp/remove.go | Fixed error handling in remove and branch removal functions to properly check command results |
| internal/errors/errors.go | Updated error pattern matching to recognize actual git error messages |
| cmd/wtp/remove_test.go | Added comprehensive tests for dirty worktree and unmerged branch removal scenarios |
| test/e2e/*.go | Updated end-to-end tests to reflect simplified interface changes |
| cmd/wtp/add.go | Simplified add command interface by removing complex flags and options |
| test/e2e/framework/assertions.go | Updated assertion to check for new friendly success message format |
| README.md | Updated documentation to reflect simplified command syntax |
| CLAUDE.md | Added documentation of recent interface simplification changes |
Comments suppressed due to low confidence (1)
cmd/wtp/add_test.go:27
- The test only checks for the 'branch' flag but the comment suggests this is testing the simplified interface. Consider adding a test to verify that removed flags (force, detach, track) are no longer present to ensure the simplification was complete.
// Check simplified flags exist (only -b/--branch remains)
| if err != nil { | ||
| return errors.WorktreeRemovalFailed(targetWorktree.Path, err) | ||
| } | ||
| if len(result.Results) > 0 && result.Results[0].Error != nil { |
There was a problem hiding this comment.
The error handling pattern for checking result.Results[0].Error is duplicated in both removeCommandWithCommandExecutor and removeBranchWithCommandExecutor. Consider extracting this logic into a helper function to reduce code duplication.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/wtp/add.go (1)
371-383: Consider normalizing paths before comparison.While the
isMainWorktreefunction usesfilepath.Absfor comparison, consider also usingfilepath.Cleanto normalize paths and handle edge cases like trailing slashes:func isMainWorktree(workTreePath, mainRepoPath string) bool { - absWorkTreePath, err := filepath.Abs(workTreePath) + absWorkTreePath, err := filepath.Abs(filepath.Clean(workTreePath)) if err != nil { return false } - absMainRepoPath, err := filepath.Abs(mainRepoPath) + absMainRepoPath, err := filepath.Abs(filepath.Clean(mainRepoPath)) if err != nil { return false } return absWorkTreePath == absMainRepoPath }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CLAUDE.md(2 hunks)README.md(2 hunks)cmd/wtp/add.go(8 hunks)cmd/wtp/add_test.go(6 hunks)cmd/wtp/remove.go(2 hunks)cmd/wtp/remove_test.go(2 hunks)internal/errors/errors.go(1 hunks)test/e2e/error_test.go(2 hunks)test/e2e/framework/assertions.go(1 hunks)test/e2e/remote_test.go(2 hunks)test/e2e/worktree_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
cmd/wtp/remove.go (2)
internal/command/types.go (1)
Command(4-8)internal/errors/errors.go (2)
WorktreeRemovalFailed(119-152)BranchRemovalFailed(154-177)
test/e2e/worktree_test.go (1)
test/e2e/framework/assertions.go (3)
AssertNoError(68-71)AssertError(73-76)AssertOutputContains(29-32)
test/e2e/error_test.go (1)
test/e2e/framework/assertions.go (1)
AssertTrue(144-147)
test/e2e/remote_test.go (2)
test/e2e/framework/framework.go (1)
NewTestEnvironment(25-38)test/e2e/framework/assertions.go (3)
AssertNoError(68-71)AssertWorktreeCreated(10-21)AssertWorktreeExists(109-122)
cmd/wtp/add_test.go (1)
internal/config/config.go (2)
Config(12-16)Defaults(19-21)
cmd/wtp/remove_test.go (1)
internal/command/types.go (1)
Result(11-15)
cmd/wtp/add.go (3)
internal/command/builders.go (1)
GitWorktreeAddOptions(4-9)internal/errors/errors.go (1)
BranchNameRequired(38-48)internal/git/repository.go (1)
NewRepository(16-21)
🔇 Additional comments (18)
internal/errors/errors.go (1)
129-129: LGTM: Error pattern updated to match actual Git output.The change from "working tree is dirty" to "contains modified or untracked files" aligns with Git's actual error message format, improving error detection accuracy for dirty worktrees.
cmd/wtp/remove.go (2)
155-166: Excellent error handling improvement.The enhanced error checking properly inspects individual command results in addition to executor errors. This addresses the core issue where worktree removal could appear to succeed while actually failing, ensuring users receive appropriate error messages for dirty worktrees.
196-207: Consistent error handling implementation.The branch removal function now uses the same improved error handling pattern as worktree removal, ensuring consistent and thorough error reporting across all removal operations.
README.md (1)
134-134: Inconsistency between changes and PR objectives.The documentation updates show changes to the
wtp addcommand interface (removing--trackflag), but the PR objectives specifically state this is about fixing error handling in thewtp removecommand for dirty worktrees. These appear to be unrelated changes that may have been bundled together inadvertently.Please verify if these
wtp addinterface changes are intentionally included in this PR or if they should be in a separate pull request focused on command interface simplification.Also applies to: 148-148
Likely an incorrect or invalid review comment.
test/e2e/framework/assertions.go (1)
13-15: Another inconsistency with PR objectives.This change adds support for a new worktree creation success message, but the PR objectives are specifically about fixing error handling when removing dirty worktrees. This appears to be unrelated functionality bundled in the same PR.
Likely an incorrect or invalid review comment.
test/e2e/worktree_test.go (1)
60-74: Test changes don't match PR objectives.The removal of DetachedHead test and addition of BranchConflict test are related to simplifying the
wtp addcommand interface, not fixing error handling in thewtp removecommand as stated in the PR objectives.While the test logic itself appears sound (testing that duplicate branch addition fails appropriately), these changes seem to belong in a different PR focused on command interface simplification rather than error handling fixes.
Likely an incorrect or invalid review comment.
test/e2e/error_test.go (1)
68-73: PR objectives and code changes appear misaligned.The changes in this file modify test expectations for the
wtp addcommand by removing references to--forceand--trackflags. However, the PR objectives state this PR is specifically about "fixing error handling in thewtp removecommand when removing dirty worktrees."These changes appear to be related to simplifying the
wtp addinterface rather than fixingwtp removeerror handling.Also applies to: 106-112
Likely an incorrect or invalid review comment.
test/e2e/remote_test.go (2)
46-47: Interface change looks correct but doesn't match PR objectives.The change from
--trackto-bflag usage is implemented correctly. However, this change is related to simplifying thewtp addinterface, not fixing error handling inwtp removeas stated in the PR objectives.Likely an incorrect or invalid review comment.
148-173: Test refactoring aligns with add command simplification.The test function rename and content changes correctly reflect the new simplified interface patterns. The tests properly verify:
- Creating new branches from remote branches
- Creating new branches from specific commits
However, these changes are unrelated to the stated PR objective of fixing
wtp removeerror handling.Likely an incorrect or invalid review comment.
CLAUDE.md (1)
5-98: Documentation describes majorwtp addchanges not mentioned in PR objectives.This extensive documentation section details a major refactoring of the
wtp addcommand, including:
- Removal of multiple flags (--detach, --track, --force, --cd/--no-cd)
- Introduction of a simplified interface
- Compatibility mode changes
However, the PR objectives explicitly state this PR is about "fixing error handling in the
wtp removecommand when removing dirty worktrees." There's no mention of these substantialwtp addchanges in the PR description.This appears to be either:
- Wrong PR objectives/description
- Wrong set of files included in this PR
- Multiple unrelated changes bundled together
Please clarify the actual scope of this PR.
Likely an incorrect or invalid review comment.
cmd/wtp/remove_test.go (2)
413-499: Excellent test coverage for dirty worktree removal scenarios.These tests properly verify the enhanced error handling for dirty worktrees:
- ✅ Removal without
--forcefails with clear error message- ✅ Error message includes helpful suggestion to use
--force- ✅ Removal with
--forcesucceedsThis aligns perfectly with the PR objectives to fix error handling when removing dirty worktrees.
501-596: Good coverage for branch removal with unmerged commits.The tests verify:
- ✅ Removal of unmerged branches fails without
--force-branch- ✅ Error message suggests using
--force-branch- ✅ Removal succeeds with the flag
Well-structured test cases that complement the dirty worktree scenarios.
cmd/wtp/add_test.go (2)
27-28: Code changes don't match PR objectives.While these test updates for the simplified
wtp addinterface are well-implemented:
- Proper validation of the new
-bflag pattern- Good test coverage for various scenarios
- Clear test names and assertions
They are completely unrelated to the PR's stated objective of "fixing error handling in the
wtp removecommand."Also applies to: 516-607
Likely an incorrect or invalid review comment.
709-805: Enhanced success messages with emojis are user-friendly.The updated success message format with emojis (✅, 📁, 🌿, 💡) provides a much better user experience. The tests thoroughly cover various scenarios including:
- Regular branches
- Main worktree (@)
- Detached HEAD
- Commit references
However, these UX improvements for
wtp addare outside the scope of the stated PR objectives.Likely an incorrect or invalid review comment.
cmd/wtp/add.go (4)
25-31: Clear and focused command interface improvements.The simplified usage text and examples make the command interface more intuitive and align well with the overall simplification effort mentioned in the PR summary.
118-151: Verify the branch name assignment logic when using resolved tracking.The logic at lines 137-140 assigns the first argument as the branch name when using resolved tracking without
-b. This seems correct, but please ensure this behavior is well-documented and tested, as it's a key part of the auto-tracking feature.if cmd.Args().Len() > 0 && opts.Branch == "" { // The first argument is the branch name when using resolved tracking without -b opts.Branch = cmd.Args().Get(0) }
312-312: Consistent error messaging with new interface.The updated error message correctly reflects the simplified command syntax.
406-439: Well-structured branch resolution logic.The
resolveBranchTrackingfunction cleanly implements the auto-tracking feature with proper error handling for ambiguous cases. The logic correctly skips resolution when creating a new branch with-bflag.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
wtp removecommand to properly display errors when removing dirty worktrees--with-branchflagProblem
The
wtp removecommand was not showing error messages when attempting to remove worktrees with uncommitted changes. Instead, it would incorrectly report success while the worktree remained.Solution
removeCommandWithCommandExecutorandremoveBranchWithCommandExecutorto check individual command resultsTest plan
Manual Test Results
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
wtp addto align syntax withgit worktree add.Bug Fixes
Documentation
wtp addcommand, removing outdated flags and clarifying usage.git worktree addbehavior.Tests
wtp addinterface and improved success/error messaging.Refactor
wtp addcommand interface by removing support for--detach,--track, and--forceflags, focusing on clearer usage patterns.