[fix] Protect fresh worktrees from clean --merged (#335)#336
[fix] Protect fresh worktrees from clean --merged (#335)#336MohammadYusif wants to merge 2 commits into
Conversation
`git branch --merged <ref>` lists every branch reachable from the ref, including a brand-new worktree branch whose tip is the base commit (an ancestor, trivially "merged"). FindMergedCandidates added any such branch to the removal set with no check for whether it had commits of its own, so a freshly-created worktree could be silently removed and its branch force-deleted by the post-merge hook (which passes --force). Add an own-commits guard, HasOwnCommits, mirroring the empty-branch check IsSquashMerged already performs (merge-base == tip). The --merged ancestor path now skips branches that contributed nothing, while genuinely merged branches (squash and merge-commit) still have own commits and are removed. Add a regression test covering the fresh-worktree case.
| // This is used to protect such branches from being treated as "merged" by | ||
| // `git branch --merged`, which lists every branch reachable from the ref — | ||
| // including a brand-new branch whose tip *is* the base commit. | ||
| func HasOwnCommits(ctx context.Context, r Runner, mergeRef, branch string) (bool, error) { |
There was a problem hiding this comment.
Medium | FF-merged branches with real commits silently survive cleanup
A branch that was fast-forward merged (e.g. feature/xyz at commit C, FF-merged into main so both tips equal C) will have merge-base(main, feature/xyz) == tip == C. HasOwnCommits returns false, so it is never added as a removal candidate — even though the work was genuinely merged.
The godoc names this explicitly ("or one that was fast-forward-merged"), framing it as intentional, but users who rely on plain git merge (FF allowed) for worktree branches will find those branches silently surviving rimba clean --merged forever.
The root tension: merge-base == tip cannot distinguish "fresh branch" from "FF-merged branch". Suggestion: document this gap clearly in clean --help or a comment in FindMergedCandidates, so users are not surprised by stale FF-merged branches persisting.
| // This is used to protect such branches from being treated as "merged" by | ||
| // `git branch --merged`, which lists every branch reachable from the ref — | ||
| // including a brand-new branch whose tip *is* the base commit. | ||
| func HasOwnCommits(ctx context.Context, r Runner, mergeRef, branch string) (bool, error) { |
There was a problem hiding this comment.
Medium | No dedicated unit tests for HasOwnCommits in internal/git
IsSquashMerged has focused unit tests covering error paths (merge-base error, tip parse, etc.). HasOwnCommits is only exercised indirectly through FindMergedCandidates — there are no tests in internal/git/branch_test.go that exercise it directly.
Given this is new exported API and the project has a 97% coverage threshold, consider adding TestHasOwnCommits in internal/git/branch_test.go covering:
- Same SHA → returns
false, nil - Different SHA → returns
true, nil MergeBaseerror → returnsfalse, errorrev-parseerror → returnsfalse, error
| hasOwn, err := git.HasOwnCommits(ctx, r, mergeRef, e.Branch) | ||
| if err != nil { | ||
| result.Warnings = append(result.Warnings, fmt.Sprintf("skipped %s: own-commits check failed: %v", e.Branch, err)) | ||
| continue |
There was a problem hiding this comment.
Medium | Missing test for the HasOwnCommits-error → warning promotion path
TestFindMergedCandidatesSquashMergeError covers the squash path's error-to-warning promotion. The parallel path added here — mergedSet[branch] == true but HasOwnCommits fails (e.g. merge-base errors) — converts the error into result.Warnings and continues (lines 77-79). This branch has no test.
Suggested addition: TestFindMergedCandidatesMergedBranchOwnCommitsError — set up a branch in mergedSet, make the mock return an error for CmdMergeBase, assert len(result.Candidates) == 0 and len(result.Warnings) == 1. Without it, a regression that accidentally propagates the error instead of warning would not be caught.
| return false, err | ||
| } | ||
|
|
||
| tip, err := r.Run(ctx, cmdRevParse, flagVerify, branch) |
There was a problem hiding this comment.
Low | rev-parse --verify branch resolves ambiguously for short branch names
HasOwnCommits passes the bare branch name to rev-parse --verify branch. For a branch named the same as a tag, git disambiguates using its refspec priority order, and could resolve to the tag SHA — making the SHA comparison against merge-base unreliable.
BranchExists (line 54) consistently uses refs/heads/<branch> to be unambiguous. Consider:
tip, err := r.Run(ctx, cmdRevParse, flagVerify, refsHeadsPrefix+branch)(IsSquashMerged has the same pre-existing inconsistency, but fixing it there is out of scope for this PR.)
lugassawan
left a comment
lugassawan
left a comment
There was a problem hiding this comment.
Review Decision: COMMENT
Reviewed by reviewer (swe-workbench). Posted 4 inline comments, deduped 0.
…it tests
- Use `refs/heads/<branch>` instead of a bare branch name in the
rev-parse --verify call inside HasOwnCommits, matching the convention
already used by BranchExists (line 54) to avoid ambiguity when a tag
shares the branch name.
- Document the fast-forward-merge limitation in the HasOwnCommits godoc:
merge-base == tip cannot distinguish a fresh branch from an FF-merged
one, so FF-merged branches survive `rimba clean --merged`.
- Add TestHasOwnCommits{DifferentSHA,SameSHA,MergeBaseError,
RevParseError,UsesRefsHeadsPrefix} in internal/git/mock_runner_test.go,
covering all four code paths and verifying the refs/heads/ prefix.
- Add TestFindMergedCandidatesMergedBranchOwnCommitsError in
internal/operations/clean_test.go, covering the error→warning
promotion path when HasOwnCommits fails on a --merged hit.
|
Thanks for the thorough review! Addressed all four points in the latest push:
|
2 participants
Summary
rimba clean --mergedwas force-deleting freshly-created worktrees that had no commits of their own.FindMergedCandidates(internal/operations/clean.go) added any branch returned bygit branch --merged <ref>to the removal set with no own-commits guard.git branch --mergedlists every branch reachable from the ref, including a brand-new worktree branch whose tip is the base commit (a trivial ancestor). With the installedpost-mergehook (which passes--force), such fresh worktrees were silently removed and their branches force-deleted.IsSquashMerged) already guarded this exact case viamergeBase == tip; the plain--mergedancestor path did not.git.HasOwnCommits(true only when the branch tip differs from its merge-base with the ref) and wired it intoFindMergedCandidates, so a--mergedhit becomes a removal candidate only when the branch actually contributed commits. Fresh and fast-forward-merged branches are protected; squash- and merge-commit-merged branches still get removed. The guard lives in candidate finding, not removal, so--forcecannot bypass it.Test Plan
make test—TestFindMergedCandidates*pass; newTestFindMergedCandidatesFreshWorktreeNotRemovedfails without the guard, passes with itmake lintmake test-e2eCloses #335