Conversation
|
Warning Review limit reached
Next review available in: 13 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change adds ChangesCleanup commands
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant cmd_clean
participant _is_branch_merged
participant Git
CLI->>cmd_clean: pass cleanup selectors and apply options
cmd_clean->>Git: inspect upstream and branch state
cmd_clean->>_is_branch_merged: check merge, rebase, and patch identity
_is_branch_merged->>Git: inspect commit and tree history
cmd_clean->>Git: remove worktrees and branches when apply is enabled
Git-->>CLI: return cleanup results
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
tests/smoke.sh (4)
486-493: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a dedicated origin repository for the
cleansection.This section mutates the shared
$ORIGIN: it deletesgone-branch, merges, cherry-picks and squash-commits ontomain. Everynew_containercall clones$ORIGIN, so any test section that runs after this one observes a different default branch history. That works today only becausecleanis the last section. A future reordering breaks earlier sections in a way that is hard to diagnose.Build a second origin under
$TMPfor this section, or add a comment that records the ordering dependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/smoke.sh` around lines 486 - 493, Isolate the clean test’s origin mutations from the shared repository to prevent order-dependent smoke tests. Update the clean section around new_container clean-c and its subsequent git operations to create and use a dedicated origin under $TMP, or explicitly document that this section must remain last if isolation is not feasible.
490-517: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
cleanfixture setup instead of discarding its output.Lines 493, 499-501, 507-509 and 515-517 route every setup command to
/dev/null 2>&1with no assertion. Each one can fail quietly. For example,git mergeandgit cherry-pickin$ORIGINrequire a clean worktree, andgit checkout -q mainfails if a previous section left$ORIGINon another branch. The suite then reports a confusing failure at line 527 rather than at the real cause.Wrap the merge, cherry-pick, squash and push steps in
assert_ok, or add oneassert_okper fixture that verifies the expected origin state, for example thatorigin/maincontains the squash commit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/smoke.sh` around lines 490 - 517, Assert every clean-fixture setup operation in the smoke test instead of suppressing failures. Update the origin checkout/merge, cherry-pick, squash merge/commit, branch push, and related fetch steps in the fixture setup around the merged-direct, merged-rebase, and merged-squash sections to use assert_ok with descriptive labels, preserving the existing commands and expected repository state.
525-539: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
cleantests do not cover three behaviors thatAGENTS.mdline 118 claims.The assertions check refs only. Three gaps remain:
- No assertion that
clean --applyremoved the worktree directories. Onlyrefs/heads/*is checked, so a broken_remove_worktreecall insidecmd_cleanwould pass.- No selector-only run.
AGENTS.mdline 118 lists--goneand--mergedseparately, but both are exercised only through the default that enables both.- No
TREES_RM_CMDrouting test forclean, whichAGENTS.mdline 118 also claims.💚 Proposed additions
assert_ok "clean --apply" bash "$T" clean --apply assert_fail "gone-branch deleted" git show-ref --verify --quiet refs/heads/gone-branch +assert_fail "gone-branch worktree removed" test -e gone-branch assert_fail "merged-direct deleted" git show-ref --verify --quiet refs/heads/merged-direct +assert_fail "merged-direct worktree removed" test -e merged-direct assert_fail "merged-rebase deleted" git show-ref --verify --quiet refs/heads/merged-rebase assert_fail "merged-squash deleted" git show-ref --verify --quiet refs/heads/merged-squash assert_ok "fresh-branch preserved after clean" git show-ref --verify --quiet refs/heads/fresh-branch +assert_ok "fresh-branch worktree preserved" test -d fresh-branch + +# Selector-only run plus TREES_RM_CMD routing through clean. +assert_ok "add clean-sel" bash "$T" add clean-sel +git -C "$ORIGIN" branch -D clean-sel >/dev/null 2>&1 +out=$(bash "$T" clean --merged 2>&1) +assert_not_contains "clean --merged skips gone section" "$out" "gone upstream" +assert_ok "clean --gone with TREES_RM_CMD" env TREES_RM_CMD="rm -rf" bash "$T" clean --gone --apply +assert_fail "clean --gone removed worktree" test -e clean-sel +assert_fail "clean --gone deleted branch" git show-ref --verify --quiet refs/heads/clean-selAdjust the
assert_not_containsneedle to the exact heading text emitted bycmd_clean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/smoke.sh` around lines 525 - 539, Extend the clean smoke tests around cmd_clean to verify that clean --apply removes each corresponding worktree directory, add separate --gone and --merged selector-only runs with assertions for their exact emitted heading text, and add a TREES_RM_CMD routing test for clean. Also update the existing assert_not_contains needle to match the exact heading emitted by cmd_clean while preserving the current ref and fresh-branch checks.
452-462: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the path target form of
rm.Every
rmtest passes a branch name.cmd_rmresolves a branch withgit show-refbefore it tests-d "$target", and each worktree directory here has the same name as its branch, so the branch arm always wins. The path arm atgit-treeslines 611-617 is never executed.AGENTS.mdline 117 claims coverage "by branch or path".Add a case that passes an unambiguous path, for example a slugged directory whose branch name contains
/, and a case that passes a directory that is not a worktree. The second case protects the removal guard I noted ongit-treeslines 611-637.💚 Proposed addition
+# Path target: a slugged directory whose name is not a branch name forces the +# path arm of cmd_rm. +assert_ok "create slashed branch worktree" bash "$T" add rm/by-path --no-push +assert_ok "rm by path" bash "$T" rm "$RM_C/rm-by-path" --apply +assert_fail "worktree removed by path" test -e rm-by-path +assert_fail "branch removed by path" git show-ref --verify --quiet refs/heads/rm/by-path + +# A plain directory is not a worktree and must be refused, even with a custom +# removal command that would otherwise delete it. +mkdir -p not-a-worktree +assert_fail "rm refuses non-worktree directory" env TREES_RM_CMD="rm -rf" bash "$T" rm not-a-worktree --apply +assert_ok "non-worktree directory intact" test -d not-a-worktree🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/smoke.sh` around lines 452 - 462, Add smoke-test coverage for the path-target branch of cmd_rm by creating a worktree whose directory path is unambiguously different from its slash-containing branch name, then verify dry-run and --apply removal using that path. Also add a case passing an existing non-worktree directory and assert rm refuses to remove it, preserving the guard behavior; keep the existing branch-target assertions unchanged.git-trees (4)
681-688: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
clean --applyreturns 0 even when removals fail.Both loops report a failed worktree removal or a failed branch deletion on stderr and then continue.
cmd_cleanalways ends withreturn 0. A caller that scriptsgit trees clean --applycannot detect partial failure.cmd_rmalready returns 1 when worktree removal fails, so the two commands report failure differently.Track a failure flag and return it.
♻️ Proposed change
- local apply=0 do_gone=0 do_merged=0 root def br p + local apply=0 do_gone=0 do_merged=0 root def br p failed=0if ! _remove_worktree "$p"; then echo " ! worktree remove failed — skipped branch delete for $br" >&2 + failed=1 continue figit branch -d "$br" 2>/dev/null || git branch -D "$br" 2>/dev/null \ - || echo " ! could not delete branch $br" >&2 + || { echo " ! could not delete branch $br" >&2; failed=1; }Apply the same two changes in the merged loop, then:
git worktree prune >/dev/null 2>&1 [ "$apply" -eq 0 ] && echo "(report only — pass --apply to execute)" - return 0 + return "$failed" }If you change the exit status, extend
tests/smoke.shto assert it.Also applies to: 706-713, 719-722
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@git-trees` around lines 681 - 688, Update cmd_clean to track whether any worktree removal or branch deletion fails, set the failure flag in both the primary and merged cleanup loops, and return a nonzero status when any failure occurred instead of always returning 0. Preserve the existing stderr messages and successful cleanup behavior, and extend tests/smoke.sh to assert the failure exit status.
74-80: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the patch-id scan the same way as the tree scan.
Step 3 limits its history walk with
git log -n 100. Step 4 runsgit log -p "$mb..origin/$def"with no limit. On a repository with a long-lived default branch, that generates the full patch text for every commit since the merge base, for every candidate branch that reaches step 4.cmd_cleancalls_is_branch_mergedonce per local branch, so the cost multiplies.♻️ Proposed change
# 4. Squash-merge patch-id match patch_id=$(git diff "$mb..$br" 2>/dev/null | git patch-id | awk '{print $1}') if [ -n "$patch_id" ]; then - if git log -p "$mb..origin/$def" 2>/dev/null | git patch-id | awk '{print $1}' | grep -qx "$patch_id"; then + # Same 100-commit window as the tree check above: recent history is where a + # squash merge of this branch would land, and an unbounded `log -p` is costly. + if git log -p -n 100 "$mb..origin/$def" 2>/dev/null | git patch-id | awk '{print $1}' | grep -qx "$patch_id"; then return 0 fi fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@git-trees` around lines 74 - 80, Update the patch-id scan in _is_branch_merged to bound its git log history walk with the same 100-commit limit used by the tree scan. Preserve the existing "$mb..origin/$def" range, patch-id extraction, and exact-match behavior while adding the limit before generating patch text.
606-606: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDiscard
rootinstead of assigning it incmd_rmandcmd_clean. These commands only need_root’s success status, but assign the path to an unusedrootlocal. Replace the assignments with_root >/dev/null || ...and removerootfrom thelocaldeclarations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@git-trees` at line 606, The cmd_rm and cmd_clean commands assign _root to an unused root variable. At git-trees lines 606 and 669, replace those assignments with _root >/dev/null while preserving the existing failure handling, and remove root from each command’s local declarations.
32-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the ShellCheck disable and rationale for intentional word splitting.
_remove_worktree()calls$TREES_RM_CMDunquoted so a multi-word value such asrm -rfbecomes command plus arguments, butshellcheck -s bash git-treesflags this as SC2086. Add the directive above the call and a short comment explaining that splitting is intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@git-trees` around lines 32 - 39, Add a ShellCheck SC2086 disable directive immediately above the unquoted $TREES_RM_CMD invocation in _remove_worktree(), along with a brief comment explaining that intentional word splitting supports multi-word commands such as “rm -rf”.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 117: Update the rm coverage description in AGENTS.md to remove the claim
about refusing deletion of unmerged branches and accurately describe the
implemented/tested behavior: unconditional forced branch deletion via git branch
-D, consistent with the existing -D documentation.
In `@git-trees`:
- Line 59: Update the arithmetic comparisons in the git branch checks around git
rev-list --count, including both the lines near 59 and 69, to default empty
command output to zero before evaluating with -gt. Preserve the existing
branch-flow behavior while preventing Bash integer-expression errors.
- Around line 48-55: Remove the unreachable inner check comparing br_commit with
mb in the ancestor branch of the merge-status logic. In the surrounding
branch-processing function, retain only the comparison against git rev-parse
origin/$def to determine the direct-merge result, preserving the behavior that
older zero-commit branches are reported as merged.
- Around line 611-637: Validate the resolved directory in the `elif [ -d
"$target" ]` path before allowing removal by confirming it appears in `git
worktree list`, rather than relying on `_branch_at` or the `br`/`path` guard.
Use this worktree-registration result as the gate for both report-only and apply
flows, while preserving support for detached-HEAD worktrees whose `br` is empty.
In `@README.md`:
- Line 262: Update the README descriptions of TREES_RM_CMD to warn that
configuring a custom removal command bypasses git worktree remove safety checks
for uncommitted changes and untracked files, potentially destroying work when
using rm --apply or clean --apply. Add the warning at each TREES_RM_CMD
description location.
In `@tests/smoke.sh`:
- Around line 464-471: Update the unmerged-worktree test comment and matching
AGENTS.md description to state that rm --apply deletes the unmerged branch via
forced removal. Wrap the setup commit in the existing assertion helper so commit
failure aborts the test and the -D escalation is genuinely exercised; use the
file’s established helper style for portability.
---
Nitpick comments:
In `@git-trees`:
- Around line 681-688: Update cmd_clean to track whether any worktree removal or
branch deletion fails, set the failure flag in both the primary and merged
cleanup loops, and return a nonzero status when any failure occurred instead of
always returning 0. Preserve the existing stderr messages and successful cleanup
behavior, and extend tests/smoke.sh to assert the failure exit status.
- Around line 74-80: Update the patch-id scan in _is_branch_merged to bound its
git log history walk with the same 100-commit limit used by the tree scan.
Preserve the existing "$mb..origin/$def" range, patch-id extraction, and
exact-match behavior while adding the limit before generating patch text.
- Line 606: The cmd_rm and cmd_clean commands assign _root to an unused root
variable. At git-trees lines 606 and 669, replace those assignments with _root
>/dev/null while preserving the existing failure handling, and remove root from
each command’s local declarations.
- Around line 32-39: Add a ShellCheck SC2086 disable directive immediately above
the unquoted $TREES_RM_CMD invocation in _remove_worktree(), along with a brief
comment explaining that intentional word splitting supports multi-word commands
such as “rm -rf”.
In `@tests/smoke.sh`:
- Around line 486-493: Isolate the clean test’s origin mutations from the shared
repository to prevent order-dependent smoke tests. Update the clean section
around new_container clean-c and its subsequent git operations to create and use
a dedicated origin under $TMP, or explicitly document that this section must
remain last if isolation is not feasible.
- Around line 490-517: Assert every clean-fixture setup operation in the smoke
test instead of suppressing failures. Update the origin checkout/merge,
cherry-pick, squash merge/commit, branch push, and related fetch steps in the
fixture setup around the merged-direct, merged-rebase, and merged-squash
sections to use assert_ok with descriptive labels, preserving the existing
commands and expected repository state.
- Around line 525-539: Extend the clean smoke tests around cmd_clean to verify
that clean --apply removes each corresponding worktree directory, add separate
--gone and --merged selector-only runs with assertions for their exact emitted
heading text, and add a TREES_RM_CMD routing test for clean. Also update the
existing assert_not_contains needle to match the exact heading emitted by
cmd_clean while preserving the current ref and fresh-branch checks.
- Around line 452-462: Add smoke-test coverage for the path-target branch of
cmd_rm by creating a worktree whose directory path is unambiguously different
from its slash-containing branch name, then verify dry-run and --apply removal
using that path. Also add a case passing an existing non-worktree directory and
assert rm refuses to remove it, preserving the guard behavior; keep the existing
branch-target assertions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b66d373-11a6-4185-8be6-0b06294c1b8e
📒 Files selected for processing (5)
.claude/agent-notes.mdAGENTS.mdREADME.mdgit-treestests/smoke.sh
Close a data-loss hole in `rm`, correct docs that contradicted the code, and cover the paths the suite claimed but never exercised. `cmd_rm`'s path arm accepted any directory. `git worktree remove` refuses a non-worktree on its own, but `TREES_RM_CMD` does not, so `TREES_RM_CMD="rm -rf" git trees rm . --apply` deleted the container root and the bare store. Gate the path arm on `git worktree list` registration rather than on the resolved branch, since a detached-HEAD worktree legitimately has none. Resolve with `pwd -P`: git records worktrees by physical path, so a symlinked parent (macOS /var) would fail both that check and `_branch_at`. Also in the script: drop an unreachable comparison in `_is_branch_merged` (once a branch is an ancestor, the merge base is that branch); default `rev-list --count` to 0 so a failure cannot leak bash's "integer expression expected"; bound the patch-id scan to the same 100 commits as the tree scan above it; return nonzero from `clean --apply` when a removal failed, matching `cmd_rm`. `AGENTS.md` claimed `rm` refused to delete unmerged branches while line 51 documented the `-d` -> `-D` escalation correctly; the tests asserted deletion. Correct the coverage lines and record the new constraints. Warn in the README that `TREES_RM_CMD` bypasses git's check for uncommitted changes and untracked files. Tests: assert the fixture commits and the `clean` origin setup, which were unasserted and could fail silently into confusing downstream failures; add the path arm (a slugged directory is the only shape that reaches it) and the non-worktree refusal; assert `clean --apply` removes worktree directories, not just refs; run each selector alone; route `clean` through `TREES_RM_CMD`; note that the section must stay last because it mutates the shared origin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Worked through all 14 findings (6 actionable, 8 nitpicks). Everything is in 2933845. FixedThe Writing the test for it surfaced a second bug in the guard itself: git records worktrees by physical path, so on macOS (
Tests: stale comment fixed; fixture commits and the whole Skipped
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/smoke.sh (2)
532-533: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the assertion label for the checkout step.
Line 532 labels the step "merge merged-direct into origin main", but line 533 runs
git checkout -q main. The merge happens at lines 534-535. A failure at line 533 prints a misleading message. Rename the label to match the command.♻️ Proposed change
-assert_ok "merge merged-direct into origin main" \ +assert_ok "checkout origin main for direct merge" \ in_dir "$ORIGIN" git -c advice.detachedHead=false checkout -q main🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/smoke.sh` around lines 532 - 533, Update the assertion label in the checkout step of the smoke test to describe checking out main, while leaving the subsequent merge assertion labels unchanged.
590-600: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the nonzero exit status of
clean --apply.
cmd_cleansetsfailed=1and returns it when a worktree removal or branch delete fails, and it continues with the other candidates (git-trees:667-741). The new tests only assert the success path. A regression that swallowsfailedwould stay undetected.Add one case that forces a removal failure, for example
TREES_RM_CMD="false", then assert thatclean --applyfails and that other candidates are still processed.# Failure path: a removal that fails must not abort the run, and the exit # status must be nonzero so `clean --apply` can be scripted. assert_ok "add clean-fail" bash "$T" add clean-fail assert_ok "delete clean-fail on origin" git -C "$ORIGIN" branch -D clean-fail assert_fail "clean --apply reports removal failure" \ env TREES_RM_CMD="false" bash "$T" clean --gone --apply assert_ok "branch kept when its worktree removal failed" \ git show-ref --verify --quiet refs/heads/clean-fail🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/smoke.sh` around lines 590 - 600, Add a failure-path case near the existing clean --apply assertions: create and remove the origin branch for a candidate such as clean-fail, run clean --gone --apply with TREES_RM_CMD set to false, and assert the command fails while processing continues and the branch remains when worktree removal fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 300-304: Update the README description of the two subcommands to
say they delete unmerged branches when --apply is provided, replacing the
ambiguous “unmerged work” wording while preserving the separate warning about
uncommitted changes.
---
Nitpick comments:
In `@tests/smoke.sh`:
- Around line 532-533: Update the assertion label in the checkout step of the
smoke test to describe checking out main, while leaving the subsequent merge
assertion labels unchanged.
- Around line 590-600: Add a failure-path case near the existing clean --apply
assertions: create and remove the origin branch for a candidate such as
clean-fail, run clean --gone --apply with TREES_RM_CMD set to false, and assert
the command fails while processing continues and the branch remains when
worktree removal fails.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 08d85c4a-4f36-4ce4-9d51-77dd9d3cebe9
📒 Files selected for processing (4)
AGENTS.mdREADME.mdgit-treestests/smoke.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- git-trees
| Both subcommands default to dry-run mode (report only) unless `--apply` is passed. | ||
| Both also delete unmerged work once `--apply` is given — `-d` escalates to `-D`. | ||
| If you have set `TREES_RM_CMD`, read the warning under [`git trees | ||
| rm`](#git-trees-rm-branchpath---apply) first: it removes git's check for | ||
| uncommitted changes. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Specify “unmerged branches,” not “unmerged work.”
The phrase can be confused with uncommitted changes. The following warning concerns uncommitted changes, while -d and -D control branch merge status. Use precise terms to avoid misunderstanding the deletion behavior.
Proposed wording
-Both also delete unmerged work once `--apply` is given — `-d` escalates to `-D`.
+Both can delete unmerged branches once `--apply` is given — branch deletion starts
+with `git branch -d` and escalates to `git branch -D`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Both subcommands default to dry-run mode (report only) unless `--apply` is passed. | |
| Both also delete unmerged work once `--apply` is given — `-d` escalates to `-D`. | |
| If you have set `TREES_RM_CMD`, read the warning under [`git trees | |
| rm`](#git-trees-rm-branchpath---apply) first: it removes git's check for | |
| uncommitted changes. | |
| Both subcommands default to dry-run mode (report only) unless `--apply` is passed. | |
| Both can delete unmerged branches once `--apply` is given — branch deletion starts | |
| with `git branch -d` and escalates to `git branch -D`. | |
| If you have set `TREES_RM_CMD`, read the warning under [`git trees | |
| rm`](`#git-trees-rm-branchpath---apply`) first: it removes git's check for | |
| uncommitted changes. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 300 - 304, Update the README description of the two
subcommands to say they delete unmerged branches when --apply is provided,
replacing the ambiguous “unmerged work” wording while preserving the separate
warning about uncommitted changes.
Close a data-loss hole in `rm`, correct docs that contradicted the code, and cover the paths the suite claimed but never exercised. `cmd_rm`'s path arm accepted any directory. `git worktree remove` refuses a non-worktree on its own, but `TREES_RM_CMD` does not, so `TREES_RM_CMD="rm -rf" git trees rm . --apply` deleted the container root and the bare store. Gate the path arm on `git worktree list` registration rather than on the resolved branch, since a detached-HEAD worktree legitimately has none. Resolve with `pwd -P`: git records worktrees by physical path, so a symlinked parent (macOS /var) would fail both that check and `_branch_at`. Also in the script: drop an unreachable comparison in `_is_branch_merged` (once a branch is an ancestor, the merge base is that branch); default `rev-list --count` to 0 so a failure cannot leak bash's "integer expression expected"; bound the patch-id scan to the same 100 commits as the tree scan above it; return nonzero from `clean --apply` when a removal failed, matching `cmd_rm`. `AGENTS.md` claimed `rm` refused to delete unmerged branches while line 51 documented the `-d` -> `-D` escalation correctly; the tests asserted deletion. Correct the coverage lines and record the new constraints. Warn in the README that `TREES_RM_CMD` bypasses git's check for uncommitted changes and untracked files. Tests: assert the fixture commits and the `clean` origin setup, which were unasserted and could fail silently into confusing downstream failures; add the path arm (a slugged directory is the only shape that reaches it) and the non-worktree refusal; assert `clean --apply` removes worktree directories, not just refs; run each selector alone; route `clean` through `TREES_RM_CMD`; note that the section must stay last because it mutates the shared origin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pwd -P requirement in cmd_rm is not obvious from the code and cost a debugging cycle to find. Record it alongside the upstream-inheritance pitfall so the next contributor does not rediscover it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Rebased onto Worth noting: CI had never run on this PR — not on my commits, and not on the original branch commit either. That was a consequence of the conflicted state, since GitHub won't run Full suite passes locally after the rebase, including the |
1 participant
Reintroduces
rmandcleansubcommands for single-worktree removal and batch cleanup of gone/merged branches.Summary by CodeRabbit
New Features
rmto preview or apply worktree and branch removal.cleanto identify and remove branches with gone upstreams or merged history.Documentation
Tests