fix(git-shed): harden branch cleanup and add --gone-only - #75
Conversation
List merged locals via for-each-ref, fix stale parsing for + worktree lines, remove linked worktrees before branch delete, and tolerate per-branch delete failures.
Review Summary by QodoHarden git-shed merged branch cleanup and worktree handling
WalkthroughsDescription• Replace branch parsing with git for-each-ref to avoid malformed tokens • Add linked worktree removal before branch deletion operations • Fix stale branch detection to handle + prefix like * prefix • Implement per-branch error tolerance with graceful failure handling Diagramflowchart LR
A["Branch Detection"] -->|for-each-ref| B["Clean Branch Names"]
B -->|Check Worktrees| C["Remove Linked Worktrees"]
C -->|git branch -d/-D| D["Delete Branches"]
D -->|Error Handling| E["Continue on Failures"]
F["Stale Detection"] -->|Handle + prefix| G["Fixed Parsing"]
File Changes1. git-shed
|
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Code Review
This pull request enhances the git-shed script by adding the ability to automatically remove linked worktrees when deleting merged or stale branches. It introduces a helper function to identify and force-remove worktrees and refactors branch identification to use git for-each-ref for better reliability. Review feedback suggests improving the worktree detection logic to handle paths with spaces using the --porcelain flag and adding --no-color to git branch commands to prevent ANSI escape codes from breaking the parsing logic.
`remove_linked_worktree_for_branch` parsed `git worktree list` via
`awk '{print $1}'`, which truncates paths containing whitespace
(common on macOS / Windows) and collapses multiple matches into a
single multi-line value that was then passed as one argument to
`git worktree remove`. The `grep -F "[$branch]"` heuristic also
false-positives on bracketed path components.
Switch to `git worktree list --porcelain` and match by full ref name
(`refs/heads/$branch`), iterating distinct paths so multi-match
cases are handled correctly and whitespace is preserved.
Addresses Qodo (#75 r3250784699) and Gemini (#75 r3250779444).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/agentic_review |
|
Persistent review updated to latest commit 3c0a185 |
|
[Review-Convergence] Round 1: active
Cross-repo adaptations (bin, not ai4inputs-specs): no |
Bundle porcelain worktree removal with optional upstream-only cleanup, branch/worktree status output before deletes, and bats coverage for --gone-only.
|
[Review-Convergence] Round 2: blocked
Operator policy applied this round:
Code changes: none pushed this round. What I did:
Reviewer state at HEAD (
Unresolved review threads (paginated, totalCount cross-checked): 1 unresolved.
Next action: author resolves SC2001 on Anything surprising: the operator-cited URL Bulletin updated: #82 |
Replaces `echo "$var" | sed 's/^/ /'` with bash parameter expansion. Resolves shellcheck SC2001 on git-shed:134 and :173 that blocked CI after #75 added `--gone-only`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `[gone]` awk pattern fails when git colorizes output (e.g. `color.ui=always` in user config). `--no-color` makes the filter deterministic regardless of terminal/config state. Addresses gemini-code-assist review thread on PR #75: #75 (comment) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/gemini review |
|
/agentic_review |
|
Persistent review updated to latest commit 49126d6 |
There was a problem hiding this comment.
Code Review
This pull request introduces a --gone-only flag to the git-shed script and adds logic to automatically remove linked worktrees before branch deletion. Feedback suggests refining the worktree removal process to prevent accidental data loss by avoiding unconditional use of --force. It also recommends filtering out the current branch from deletion candidates and using LC_ALL=C for more reliable pattern matching in Git output.
The worktree-removal helper jumped straight to `git worktree remove --force`, which discards any uncommitted changes in the linked worktree without warning. Try a plain `git worktree remove` first so clean worktrees succeed safely; escalate to `--force` (dirty/locked) and then `--force --force` (administrative override) only when the previous step fails. Preserves uncommitted work by default while keeping the original cleanup behavior available as a fallback. Addresses Gemini review thread on git-shed:114.
`delete_merged_branches` could surface HEAD as a candidate when the current branch is also merged into the target. `git branch -d` always refuses to delete the current branch, so the script printed a warning and attempted a worktree-removal pass for a branch that could never be deleted. Filter the current branch up-front with `git branch --show-current`; safe in detached HEAD because `for-each-ref` never emits empty refnames. Addresses Gemini review thread on git-shed:125.
`delete_gone_branches` matched the `: gone]` token from `git branch -vv` output and treated the current-branch marker (`*`) the same as the other-worktree marker (`+`), surfacing HEAD as a deletion candidate even though `git branch -D` always refuses it. Set `LC_ALL=C` for defensive locale-independence on the input stream, and split the awk arm so only `+` rows print field 2 while `*` rows are skipped outright. Addresses Gemini review thread on git-shed:164.
The `-*)` case in the option parser rejected any dash-prefixed positional argument, making TARGET_BRANCH names like `-wip` unusable. Add the conventional `--` end-of-options marker so callers can pass dash-prefixed branch names (e.g. `git-shed -- -wip`). Reported by Qodo /agentic_review (issue-comment 4463230561, Bug 2).
`remove_linked_worktree_for_branch` ran `git rev-parse --show-toplevel` unconditionally, which exits 128 in a bare repository. Under `set -euo pipefail` that aborts the entire script before merged/gone branch cleanup completes. Bare repos have no working trees to remove, so guard the call: skip immediately when the repo is bare or when `--show-toplevel` is unavailable for any other reason. Reported by Qodo /agentic_review (issue-comment 4463230561, Bug 3).
|
[Operator-assisted assessment] Qodo summary (issue-comment 4463230561)Bug 1: Worktree path parsing broken — already resolvedConfirmed resolved. Current Bug 2: Dash branch name rejected — fixed
Bug 3: Bare repo hard-fails — fixed
CI failure on Run Tests (macos-latest)
Net change this session
Both pushed to |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the git-shed script by adding a --gone-only option, supporting dash-prefixed branch names, and implementing automatic removal of linked worktrees before branch deletion. The core logic has been refactored into functions for better maintainability, and new tests have been added to verify the changes. Review feedback highlights several opportunities to further harden the script against branch names containing whitespace, specifically by improving awk parsing logic and using while read loops instead of for loops to avoid word splitting issues.
|
/agentic_review |
|
Persistent review updated to latest commit 67f84f6 |
|
[Review-Convergence] Round 3: blocked
|
Address four Gemini Round 3 review findings on PR #75. Each affected site parsed or iterated branch/worktree refs in a way that breaks when a name contains an ASCII space (which `git check-ref-format` permits). - `remove_linked_worktree_for_branch`: compare full `branch` line via `substr($0, 8)` instead of `$2`, so names with spaces match. Addresses comment r3271510527. - `delete_gone_branches`: source stale branches from `git for-each-ref` with a `|` delimiter (mirroring `delete_merged_branches`), and exclude `$TARGET_BRANCH`. Replaces the fragile `git branch -vv | awk` field-split parse. Addresses comment r3271510532. - `delete_merged_branches`: iterate via `while IFS= read -r branch` over a `<<<"$merged_branches"` here-string, with an empty-line guard. Addresses comment r3271510538. - `delete_gone_branches`: same iteration fix for stale_branches. Addresses comment r3271510543. Verified: `bash -n`, `shellcheck`, and `make test SCRIPTS=git-shed PARALLEL=false` (11/11) all pass.
When HEAD is detached, `git branch --show-current` returns the empty string. Passing it directly into `grep -vxF -e "$current_branch"` makes grep match every line (since `-e ""` matches all input), and `-v` then filters every branch out, silently disabling merged-branch cleanup in detached-HEAD contexts (common in CI/rebase/bisect workflows). Only exclude `$current_branch` when it is non-empty. Add a regression test covering the detached-HEAD path. Resolves Qodo finding r3271553397.
|
/agentic_review |
|
/gemini review |
|
Persistent review updated to latest commit 248260b |
|
[Review-Convergence] Round 4: active
|
There was a problem hiding this comment.
Code Review
This pull request refactors the git-shed script to improve modularity and robustness. Key changes include the addition of a --gone-only flag to target branches with missing upstreams, support for dash-prefixed branch names via the -- separator, and automatic removal of linked worktrees before branch deletion. The logic has been modularized into functions, and handling for bare repositories and detached HEAD states has been improved. Review feedback suggests simplifying the branch exclusion logic using arrays and improving compatibility with older Git versions by replacing git branch --show-current with git symbolic-ref.
|
[Review-Convergence] Round 5: blocked
|
|
[Review-Convergence] Round 6: converged
|
Summary
git-shedfailed when listing merged branches checked out in another worktree (+prefix): the delete loop triedgit branch -d '+'. It also failed when deleting a merged branch still active in a linked worktree. Stale upstream parsing mis-read+lines the same way.This PR hardens cleanup end-to-end:
git for-each-ref --mergedagainstorigin/TARGET_BRANCH(fallback: local target) instead of parsinggit branch --mergedlinesgit branch -d/-D, usinggit worktree list --porcelain(paths with whitespace / multiple matches), with--force --forceretry on lock errors; skip branch on failure instead of aborting[gone]with both*and+prefixes ingit branch -vvgit branch -vandgit worktree listbefore cleanup--gone-only: skip the merged pass and only delete upstream-deleted branches (formerclean_gonescope)Closes #82 (review-convergence bulletin).
Test plan
SCRIPTS=git-shed PARALLEL=false ./tests/run-tests.sh(8/8)git-shed --dry-runon a repo with merged and[gone]branchesgit-shed --gone-only --dry-runskips merged listing