-
-
Notifications
You must be signed in to change notification settings - Fork 8
fix: skip signoff check merge commits #266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@shenxianpeng 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
Co-authored-by: shenxianpeng <3353385+shenxianpeng@users.noreply.github.com>
Co-authored-by: shenxianpeng <3353385+shenxianpeng@users.noreply.github.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Join our Discord community for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #266 +/- ##
=======================================
Coverage 99.13% 99.14%
=======================================
Files 8 8
Lines 347 350 +3
=======================================
+ Hits 344 347 +3
Misses 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #266 will not alter performanceComparing Summary
Benchmarks breakdown
|
The signoff check was blocking merge commits even though merge commits typically don't require signoff trailers. This caused issues when using commit-check as a PR prerequisite, as GitHub merge commits would fail the signoff check and prevent PR merging.
Problem
When doing merges on GitHub, the signoff trailer is missing from merge commits. If commit-check is configured as a PR check, the signoff requirement blocks legitimate merges:
Before this fix:
Merge pull request #123 from user/feature
→ ❌ FAIL (blocked merge)feat: add new feature
→ ❌ FAIL (correctly requires signoff)Solution
Added merge commit detection logic to
check_commit_signoff()
that mirrors the existing logic incheck_imperative()
. The function now extracts the subject line and skips signoff validation if it starts with "Merge".After this fix:
Merge pull request #123 from user/feature
→ ✅ PASS (merge skipped)feat: add new feature
→ ❌ FAIL (still requires signoff)feat: add new feature\n\nSigned-off-by: User <user@example.com>
→ ✅ PASS (has signoff)Changes
check_commit_signoff()
to skip merge commits by checking if subject starts with "Merge"check_commit_signoff()
The fix is minimal (5 lines changed) and maintains backward compatibility while resolving the merge commit blocking issue.
Fixes #265.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.