check_formalities: allow fixup and squash commits#12
check_formalities: allow fixup and squash commits#12BKPepe wants to merge 1 commit intoGeorgeSapkin:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
|
I think I found a bug. 🤔 
|
There was a problem hiding this comment.
Pull request overview
This pull request adds support for detecting fixup and squash commits in the commit formality checker, allowing them to pass CI with a warning while skipping strict formality checks. This facilitates code review by allowing temporary fixup commits that will be squashed before merging.
Changes:
- Introduced IS_FIXUP global flag and is_in_fixup_mode helper to persist fixup detection state across check resets
- Added is_fixup_or_squash function to detect commits starting with 'fixup!' or 'squash!'
- Modified reset_skip_reasons to re-apply fixup commit skip reason when IS_FIXUP flag is set
- Updated test expectations to account for new fixup detection check
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/check_formalities.sh | Implements fixup/squash commit detection, adds IS_FIXUP flag, modifies check functions to skip formality checks for fixup commits |
| src/test.sh | Updates all test expected results to accommodate new check position, adds test case for fixup commit behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Yeah, that LaTeX bit is not very robust. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| define \ | ||
| -test 'Fixup commit' \ | ||
| -expected '0 0 2 3 3 3 3 3 3 3 3 3 3 3 3 3 3' \ | ||
| -author 'Good Author' \ | ||
| -email 'good.author@example.com' \ | ||
| -subject 'fixup! package: subject' \ | ||
| -body <<-'EOF' | ||
| This is a fixup commit. | ||
|
|
||
| Signed-off-by: Good Author <good.author@example.com> | ||
| EOF |
There was a problem hiding this comment.
The implementation supports both fixup and squash commits (as indicated by the function name is_fixup_or_squash and the regex pattern), but the test suite only includes a test case for fixup commits. Consider adding a test case for squash commits to ensure both types of commits are properly handled. The test would be similar to the fixup test but with subject 'squash! package: subject' and should have the same expected results.
There was a problem hiding this comment.
Similar test? Why not. But maybe this could be only one test, where the name will be Fixup and squash since it is the same, but creating more tests... yeah, done.
Fixup and squash commits are often used during development to organize changes. Currently, the script fails on these commits due to subject formatting checks. Detect commits starting with 'fixup!' or 'squash!', issue a warning, and skip strict formality checks for them. This allows the CI to pass with a warning, facilitating easier review while still signaling that these commits need to be squashed before merging. Unlike 'Revert' commits which only skip subject checks, 'fixup' commits must skip all checks (author, body, signature). Since 'reset_skip_reasons' is called between these checks and clears the exemption stack, introduce an 'IS_FIXUP' flag. This persistent state allows 'reset_skip_reasons' to re-apply the 'fixup commit' skip reason globally for the entire commit processing loop. Also update tests to verify this behavior. Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
|
I think this should be ready |
| if is_warn $?; then | ||
| output_raw "Fixup commit has been detected, skipping formality checks for now. This allows easier review of individual changes. Please squash these commits before merging, after which formality checks will be enforced." | ||
| echo | ||
| IS_FIXUP=1 |
There was a problem hiding this comment.
Maybe fixups could just continue, same as for merge commits, while squash could do most checks? I thought fixups are just that, but squash commits can amend commit messages. So squash should pass all checks without the squash! prefix. I haven't thought about this too deep to be honest.
And maybe the check text should be something like: Pull request must not have any fixup or squash commits.
|
Huh? |
Fixup and squash commits are often used during development to organize changes. Currently, the script fails on these commits due to subject formatting checks.
Detect commits starting with 'fixup!' or 'squash!', issue a warning, and skip strict formality checks for them. This allows the CI to pass with a warning, facilitating easier review while still signaling that these commits need to be squashed before merging.
Unlike 'Revert' commits which only skip subject checks, 'fixup' commits must skip all checks (author, body, signature). Since 'reset_skip_reasons' is called between these checks and clears the exemption stack, we introduce an 'IS_FIXUP' flag. This persistent state allows 'reset_skip_reasons' to re-apply the 'fixup commit' skip reason globally for the entire commit processing loop.
Also update tests to verify this behavior.