Enhance branch-cleanup with multi-layered merge detection #129
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mtnbikenc The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @mtnbikenc. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note
|
| Cohort / File(s) | Change Summary |
|---|---|
Branch detection logic overhaul plugins/git/commands/branch-cleanup.md |
Adds check_if_merged() bash function that evaluates branches using four methods (standard, merge-commit, content-identical, cherry-picked) and returns merged:<method> or not-merged; replaces direct git branch --merged usage with per-branch iteration that emits `branch |
Sequence Diagram(s)
sequenceDiagram
actor User
participant Script as branch-cleanup
participant Git
User->>Script: run branch-cleanup (target main)
Script->>Git: list branches
loop per branch
Script->>Git: check standard merge (merge-base / --merged)
alt standard detected
Script->>Script: return merged:standard
else
Script->>Git: inspect merge-commit messages
alt merge-commit detected
Script->>Script: return merged:merge-commit
else
Script->>Git: compare trees (three-dot / content diff)
alt content identical
Script->>Script: return merged:content-identical
else
Script->>Git: detect cherry-pick equivalence (patch-id / cherry)
alt cherry-picked detected
Script->>Script: return merged:cherry-picked
else
Script->>Script: return not-merged
end
end
end
end
Note right of Script: emit "branch|method"
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
- Inspect
check_if_merged()implementation for correctness and edge cases across all four detection methods. - Verify per-branch loop, output formatting (
branch|method), and integration with gone/protected branch logic. - Validate updated examples and notes reflect behavior for rebased/content-identical branches and potential false positives (e.g., identical content but different intent).
Pre-merge checks and finishing touches
Important
Pre-merge checks failed
Please resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| No Assumed Git Remote Names | ❓ Inconclusive | Unable to locate the branch-cleanup.md file in the repository to verify if git remote names like 'origin' or 'upstream' are hardcoded. | Access the actual modified file and search for hardcoded remote names; ensure remote discovery is dynamic using git remote commands. |
✅ Passed checks (6 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Enhance branch-cleanup with multi-layered merge detection' directly and accurately summarizes the main change: adding multi-method merge detection to the branch-cleanup command. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| No Real People Names In Style References | ✅ Passed | The pull request contains no references to real people by name in style references, plugin commands, skill documentation, or example prompts. |
| Git Push Safety Rules | ✅ Passed | No git push commands, force pushes, or pushes to main/master branches detected in PR changes. |
| No Untrusted Mcp Servers | ✅ Passed | Pull request modifies only branch-cleanup.md documentation with bash logic for git commands. No MCP servers, npm packages, or external dependencies are introduced. |
✨ Finishing touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
plugins/git/commands/branch-cleanup.md(4 hunks)
🔇 Additional comments (5)
plugins/git/commands/branch-cleanup.md (5)
92-137: Multi-layered merge detection is well-structured and properly addresses past concerns.The four methods are logically ordered, each adds incremental coverage, and early returns prevent duplicate classification. Regex escaping is consistently applied, and method-specific limitations are documented.
104-114: Method 2 pattern correctly requires branch name presence; addresses false-positive concern.The pattern evolution from a standalone
pull.?requestmatch to requiring actual branch name reference (either full name, basename in "from" clause, or word-bounded match) significantly reduces false positives. Limiting togit log --mergesensures only real merge commits are evaluated.
139-148: Branch iteration loop correctly uses multi-layered detection; output structure is clean.The loop properly skips the main branch and captures the method label from the return value. Format
branch|methodis parseable and maintains clarity.
37-50: Documentation describes all four detection methods consistently.The specification, categorization, and implementation are aligned. Method ordering and semantics are clearly defined across all sections.
117-133: Error suppression for edge cases is documented; fallback behavior is reasonable.Methods 3 and 4 suppress errors via
2>/dev/null, allowing graceful degradation if a method encounters an edge case (e.g., no common ancestor, invalid branch). Continuing to the next method ensures detection is attempted across multiple strategies. Documentation at lines 119 and 127–128 notes these design choices.
Summary
Your implementation successfully addresses all four past review comments and delivers a robust multi-layered merge detection system:
✅ Merge-commit pattern now uses case-insensitive extended regex matching multiple formats (standard "Merge branch", PR merges, squash contexts) while requiring branch name presence to avoid false positives.
✅ Regex escaping correctly handles all ERE metacharacters including the previously-missing
]and}, using a bracket-safe character class.✅ Example output now shows three distinct branches (feature/bug-fix-123, feature/update-deps, feature/rebased-work) each detected by a different method, accurately reflecting the function's early-return semantics.
✅ Documentation clearly explains the three-dot diff syntax, cherry-pick detection limitations, and intentional error suppression. The explanatory note at line 208 correctly references the rebased branch.
The function design—attempting detection in order (standard → merge-commit → content-identical → cherry-picked) and returning on first match—is sound and prevents duplicate classification. All four methods are properly documented, tested in examples, and have graceful fallback behavior.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
plugins/git/commands/branch-cleanup.md (3)
117-121: Cherry-pick detection may have false positives.The
git cherrymethod detects commits with equivalent patches, but equivalence doesn't guarantee the commits are actually in the main branch history—only that patch content matches. This could incorrectly report "merged:cherry-picked" for branches with coincidentally similar commits.Consider adding explicit verification that equivalent commits are reachable from the main branch, or document this limitation clearly in the comments.
90-125: Error handling forgitcommands is silent—failures could go undetected.The function uses
2>/dev/nullto suppress stderr, which masks genuine errors (e.g., invalid branch name, permission issues, corrupted repository) and makes debugging difficult. Silent failures could cause the function to incorrectly report "not-merged" when a command actually failed.Consider adding explicit error checking or at least logging failures for investigation.
Example improvement:
check_if_merged() { local branch=$1 local main_branch=$2 # Method 1: Standard merge check if git branch --merged "$main_branch" 2>/dev/null | grep -q "^[* ]*${branch}$"; then echo "merged:standard" return 0 fi # ... rest of checks ... # Consider adding a fallback log or warning if git commands fail }
110-113: Verify the three-dot diff syntax handles edge cases correctly.The
git diff --quiet "$main_branch"..."$branch"uses three-dot syntax, which compares the merge base of both branches. This is correct for detecting content-identical branches, but verify:
- What happens if there is no common ancestor (rare but possible in some workflows)?
- The
2>/dev/nullmasks any errors from malformed branch names.Consider adding a comment explaining the three-dot syntax for maintainability, or test this edge case explicitly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
plugins/git/commands/branch-cleanup.md(4 hunks)
mtnbikenc
force-pushed
the
enhance-branch-cleanup-merge-detection
branch
from
8fb9e66 to
c0a63d9
Compare
|
Thanks for the thorough review! I've addressed all the feedback: Major changes:
Nitpick suggestions addressed:
All changes have been amended into the original commit and force-pushed to maintain a clean git history. AI-assisted response via Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/git/commands/branch-cleanup.md (1)
97-97: Escape branch names in regex patterns to handle special characters safely.Branch names can contain characters that are regex metacharacters (e.g., dots in
feature.api). When interpolated without escaping into patterns at lines 97 and 104, this could cause false matches if multiple similar branch names exist differing in a single character. While unlikely to cause issues in typical use (false positives more probable than negatives, and function exits on first match), it violates best practices for regex handling.Consider using
printf '%s\n' "$branch" | sed 's/[[\.*^$()+?{|]/\\&/g'or bash'sprintf %qto escape special characters before interpolation.Apply this diff to safely escape the branch name in Method 1 (line 97):
check_if_merged() { local branch=$1 local main_branch=$2 + local branch_escaped=$(printf '%s\n' "$branch" | sed 's/[[\.*^$()+?{|]/\\&/g') # Method 1: Standard merge check (commits in main history) - if git branch --merged "$main_branch" | grep -q "^[* ]*${branch}$"; then + if git branch --merged "$main_branch" | grep -q "^[* ]*${branch_escaped}$"; then echo "merged:standard" return 0 fiAnd update Method 2 (line 104) to use
$branch_escapedin the pattern's alternatives:- if git log "$main_branch" --merges --oneline | grep -qiE "(merge|squash).*(${branch}|branch[[:space:]]+'${branch}'|pull.?request)"; then + if git log "$main_branch" --merges --oneline | grep -qiE "(merge|squash).*(${branch_escaped}|branch[[:space:]]+'${branch_escaped}'|pull.?request)"; then echo "merged:merge-commit" return 0 fiAlso applies to: 104-104
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
plugins/git/commands/branch-cleanup.md(4 hunks)
🔇 Additional comments (4)
plugins/git/commands/branch-cleanup.md (4)
184-186: Previous feedback has been successfully addressed.The example output now correctly shows three different branches, each detected by a different method (
feature/bug-fix-123for standard,feature/update-depsfor merge-commit, andfeature/rebased-workfor content-identical), resolving the prior concern about duplicate branch names. The merge-commit detection pattern (line 104) has also been substantially improved to handle multiple merge message formats using case-insensitive regex alternatives.
37-50: Documentation of multi-layered detection is clear and accurate.The updated implementation steps (2 and 3) clearly explain each of the four detection methods with their purposes. The descriptions align well with the function implementation and will help users understand when each method applies.
132-141: Branch iteration and merge detection loop is correctly structured.The loop properly filters branches, calls the multi-layered detection function, and outputs the branch name with its detected method. The parameter expansion
${merge_status#merged:}correctly extracts just the method name from the full response.
201-201: Explanatory note enhances user understanding of content-identical detection.The note clarifying that rebased or cherry-picked branches can be considered merged despite different commit hashes is helpful context that justifies why these branches are safe to delete. This addresses a potential source of user confusion.
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
mtnbikenc
force-pushed
the
enhance-branch-cleanup-merge-detection
branch
from
c0a63d9 to
be0321b
Compare
|
Security/Robustness Improvement:
Example vulnerable branch names now handled:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
plugins/git/commands/branch-cleanup.md (3)
97-101: Optional: Use merge-base ancestor check to avoid parsinggit branch --merged.Equivalent, simpler, and fast:
Apply this diff:
- if git branch --merged "$main_branch" | grep -q "^[* ]*${branch_escaped}$"; then + if git merge-base --is-ancestor "$branch" "$main_branch"; then echo "merged:standard" return 0 fi
119-127: Cherry-pick detection is reasonable; document performance caveat.
git cherrycan be slower on very large branches; acceptable here but worth noting.Add: “Performance note: On very large histories, consider limiting range (e.g.,
main_branch...branch) or a commit-count guard.”
133-142: Make branch iteration robust and exclude current branch.Command substitution splits on whitespace; also, don’t consider the current branch.
Apply this diff:
-for branch in $(git branch | grep -v "^\*" | sed 's/^[ ]*//'); do - if [ "$branch" != "$main_branch" ]; then +git for-each-ref --format='%(refname:short)' refs/heads/ | while IFS= read -r branch; do + if [ "$branch" != "$main_branch" ] && [ "$branch" != "$current_branch" ]; then merge_status=$(check_if_merged "$branch" "$main_branch") if [[ "$merge_status" == merged:* ]]; then method=${merge_status#merged:} echo "$branch|$method" fi fi -done +done
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
plugins/git/commands/branch-cleanup.md(4 hunks)
🔇 Additional comments (4)
plugins/git/commands/branch-cleanup.md (4)
37-41: LGTM on documenting the four detection methods. Clear, ordered, and aligns with the implementation below.
46-50: LGTM on categorization labels. The method names are concise and map 1:1 to detection steps.
110-117: LGTM on three-dot content check. Correctly uses merge-base semantics and suppresses noisy errors.If you expect very large histories, consider adding
--no-ext-difffor speed.
185-187: Examples read well and align with detection methods.
Improves the git:branch-cleanup command to detect merged branches using multiple methods, preventing false negatives when branches are rebased, squashed, or cherry-picked. Key improvements: - Added check_if_merged() function with 4 detection methods: * Standard: git branch --merged (commits in main history) * Merge-commit: searches merge commit messages * Content-identical: compares content with git diff (catches rebases) * Cherry-picked: detects equivalent patches with git cherry - Enhanced categorization to show detection method used - Updated examples to demonstrate new merge status labels - Improved output format with explanatory notes for rebased branches This fixes cases where branches show as unmerged despite having identical content in main, such as when maintainers rebase PRs before merging. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
mtnbikenc
force-pushed
the
enhance-branch-cleanup-merge-detection
branch
from
be0321b to
f45398f
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
FYI @smg247, I'm proposing these updates because I used the command on my local repo and it missed feature branches that were already merged. |
1 participant
Improves the git:branch-cleanup command to detect merged branches using multiple methods, preventing false negatives when branches are rebased, squashed, or cherry-picked.
Key improvements:
Added check_if_merged() function with 4 detection methods:
Enhanced categorization to show detection method used
Updated examples to demonstrate new merge status labels
Improved output format with explanatory notes for rebased branches
This fixes cases where branches show as unmerged despite having identical content in main, such as when maintainers rebase PRs before merging.
🤖 Generated with Claude Code
Summary by CodeRabbit