Skip to content

check_formalities: allow fixup and squash commits #12

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.

Sign up for GitHub

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

Open
BKPepe wants to merge 1 commit into
base: main
Choose a base branch
from
Open

check_formalities: allow fixup and squash commits #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/check_formalities.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ SKIP_REASONS=()
# Use these global vars to improve header creation readability
COMMIT=""
HEADER_SET=0
IS_FIXUP=0

REPO_PATH=${1:+-C "$1"}
# shellcheck disable=SC2206
Expand Down Expand Up @@ -230,6 +231,8 @@ is_not_alias() { [[ ! "$1" =~ [^[:space:]] ]]; }
# shellcheck disable=SC2329
is_not_name() { [[ ! "$1" =~ [^[:space:]]+[[:space:]][^[:space:]]+ ]]; }
is_revert() { [[ "$1" == "Revert "* ]]; }
is_fixup_or_squash() { [[ "$1" =~ ^(fixup|squash)! ]]; }
is_in_fixup_mode() { [ "$IS_FIXUP" = 1 ]; }
is_warn() { [ "$1" = "$RES_WARN" ]; }
# shellcheck disable=SC2329
omits() { [[ "$1" != *"$2"* ]]; }
Expand Down Expand Up @@ -299,6 +302,10 @@ reset_skip_reasons() {
if [ $? = 0 ]; then
push_skip_reason "authored by $exception"
fi

if is_in_fixup_mode; then
push_skip_reason 'fixup or squash commit'
fi
}

get_arity() {
Expand Down Expand Up @@ -515,6 +522,8 @@ check_body() {
check \
-rule 'Commit message must exist' \
-always \
-skip-if is_in_fixup_mode \
-skip-reason 'fixup or squash commit' \
-fail-if is_body_empty "$body" \
-fail-set-skip 'missing commit message'

Expand Down Expand Up @@ -592,6 +601,7 @@ main() {
do
HEADER_SET=0
COMMIT="$commit"
IS_FIXUP=0

echo "$commit_header"

Expand All @@ -606,6 +616,16 @@ main() {
continue
fi

check \
-rule 'Fixup commit has been detected' \
-warn-if is_fixup_or_squash "$subject"

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
Copy link
Owner

@GeorgeSapkin GeorgeSapkin Jan 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fi

reset_skip_reasons "$author_email"
check_name 'Author' "$author_name"
check_email 'Author' "$author_email"
Expand Down
86 changes: 56 additions & 30 deletions src/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ define() {

define \
-test 'Good commit' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'package: add new feature' \
Expand All @@ -102,7 +102,7 @@ define \

define \
-test 'Subject: double prefix' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'kernel: 6.18: add new feature' \
Expand All @@ -114,7 +114,7 @@ define \

define \
-test 'Subject: double prefix and capitalized first word' \
-expected '0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'kernel: 6.18: Add new feature' \
Expand All @@ -126,7 +126,7 @@ define \

define \
-test 'Bad check parsing test' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'package: add new feature' \
Expand All @@ -142,7 +142,7 @@ define \

define \
-test 'Revert commit' \
-expected '0 0 0 0 0 0 3 3 3 3 3 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 3 3 3 3 3 0 0 0 0 3' \
-author 'Revert Author' \
-email '[email protected]' \
-subject "Revert 'package: add new feature'" \
Expand All @@ -152,10 +152,36 @@ define \
Signed-off-by: Revert Author <[email protected]>
EOF

# shellcheck disable=SC2016
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 '[email protected]' \
-subject 'fixup! package: subject' \
-body <<-'EOF'
This is a fixup commit.

Signed-off-by: Good Author <[email protected]>
EOF
Comment on lines +156 to +166
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@BKPepe BKPepe Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# shellcheck disable=SC2016
define \
-test 'Squash commit' \
-expected '0 0 2 3 3 3 3 3 3 3 3 3 3 3 3 3 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'squash! package: subject' \
-body <<-'EOF'
This is a squash commit.

Signed-off-by: Good Author <[email protected]>
EOF

# shellcheck disable=SC2016
define \
-test 'Body: malicious body shell injection' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'test: malicious body shell injection' \
Expand All @@ -167,7 +193,7 @@ define \

define \
-test 'Body: malicious body check injection' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'test: malicious body check injection' \
Expand All @@ -179,7 +205,7 @@ define \

define \
-test 'Body: missing Signed-off-by but check disabled' \
-expected '0 0 0 0 0 0 0 0 0 0 0 3 3 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 3 3 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'test: fail on missing signed-off-by' \
Expand All @@ -188,7 +214,7 @@ define \

define \
-test 'Body: mismatched Signed-off-by but check disabled' \
-expected '0 0 0 0 0 0 0 0 0 0 0 3 3 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 3 3 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'test: fail on mismatched signed-off-by' \
Expand All @@ -201,7 +227,7 @@ define \

define \
-test 'Bad author email (GitHub noreply)' \
-expected '0 0 0 1 0 1 0 0 0 0 0 0 1 0 0 3' \
-expected '0 0 0 0 1 0 1 0 0 0 0 0 0 1 0 0 3' \
-author 'Bad Email' \
-email '[email protected]' \
-subject 'test: fail on bad author email' \
Expand All @@ -213,7 +239,7 @@ define \

define \
-test 'Subject: starts with whitespace' \
-expected '0 0 0 0 0 0 1 1 3 0 0 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 1 1 3 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject ' package: subject starts with whitespace' \
Expand All @@ -225,7 +251,7 @@ define \

define \
-test 'Subject: no prefix' \
-expected '0 0 0 0 0 0 0 1 3 0 0 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 0 1 3 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'This subject has no prefix' \
Expand All @@ -237,7 +263,7 @@ define \

define \
-test 'Subject: capitalized first word' \
-expected '0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'package: Capitalized first word' \
Expand All @@ -249,7 +275,7 @@ define \

define \
-test 'Subject: ends with a period' \
-expected '0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'package: subject ends with a period.' \
Expand All @@ -261,7 +287,7 @@ define \

define \
-test 'Subject: too long (hard limit)' \
-expected '0 0 0 0 0 0 0 0 0 0 1 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 1 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'package: this subject is way too long and should fail the hard limit check of 60 chars' \
Expand All @@ -273,15 +299,15 @@ define \

define \
-test 'Body: missing Signed-off-by' \
-expected '0 0 0 0 0 0 0 0 0 0 0 1 3 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 1 3 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'test: fail on missing signed-off-by' \
-body 'The Signed-off-by line is missing.'

define \
-test 'Body: mismatched Signed-off-by' \
-expected '0 0 0 0 0 0 0 0 0 0 0 1 3 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 1 3 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'test: fail on mismatched signed-off-by' \
Expand All @@ -293,15 +319,15 @@ define \

define \
-test 'Body: empty' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 1 3 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 3 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'test: fail on empty body' \
-body 'Signed-off-by: Good Author <[email protected]>'

define \
-test 'Author name is a single word' \
-expected '0 0 2 0 2 0 0 0 0 0 0 0 0 0 0 3' \
-expected '0 0 0 2 0 2 0 0 0 0 0 0 0 0 0 0 3' \
-author 'Nickname' \
-email '[email protected]' \
-subject 'test: warn on single-word author name' \
Expand All @@ -313,7 +339,7 @@ define \

define \
-test 'Subject: too long (soft limit)' \
-expected '0 0 0 0 0 0 0 0 0 0 2 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 2 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'package: this subject is long and should trigger a warning' \
Expand All @@ -325,7 +351,7 @@ define \

define \
-test 'Body: line too long' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 2 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 2 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'test: warn on long body line' \
Expand All @@ -337,7 +363,7 @@ define \

define \
-test 'Body: line almost too long' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'test: pass on not too long body line' \
Expand All @@ -349,7 +375,7 @@ define \

define \
-test 'Exception: dependabot' \
-expected '0 0 3 3 3 3 3 3 3 3 3 3 3 0 3 3' \
-expected '0 0 0 3 3 3 3 3 3 3 3 3 3 3 0 3 3' \
-author 'dependabot[bot]' \
-email 'dependabot[bot]@users.noreply.github.com' \
-subject 'CI: bump something from 1 to 2' \
Expand All @@ -360,7 +386,7 @@ define \

define \
-test 'No exception: dependabot' \
-expected '0 0 2 1 2 1 0 0 0 0 0 1 3 0 0 3' \
-expected '0 0 0 2 1 2 1 0 0 0 0 0 1 3 0 0 3' \
-author 'dependabot[bot]' \
-email 'dependabot[bot]@users.noreply.github.com' \
-subject 'CI: bump something from 1 to 2' \
Expand All @@ -370,7 +396,7 @@ define \

define \
-test 'Exception: weblate' \
-expected '0 0 3 3 3 3 3 3 3 3 3 3 3 0 3 3' \
-expected '0 0 0 3 3 3 3 3 3 3 3 3 3 3 0 3 3' \
-author 'Hosted Weblate' \
-email '[email protected]' \
-subject 'Translated using Weblate (English)' \
Expand All @@ -381,7 +407,7 @@ define \

define \
-test 'No exception: weblate' \
-expected '0 0 0 0 0 0 0 1 3 0 0 1 3 0 0 3' \
-expected '0 0 0 0 0 0 0 0 1 3 0 0 1 3 0 0 3' \
-author 'Hosted Weblate' \
-email '[email protected]' \
-subject 'Translated using Weblate (English)' \
Expand All @@ -400,7 +426,7 @@ define \

define \
-test 'PR from master' \
-expected '1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-expected '1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'package: add new feature' \
Expand All @@ -415,7 +441,7 @@ define \

define \
-test 'Feature branch check disabled' \
-expected '3 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-expected '3 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'package: add new feature' \
Expand All @@ -428,7 +454,7 @@ define \

define \
-test 'Feature branch check enabled, PR from main fails' \
-expected '1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-expected '1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'package: add new feature' \
Expand All @@ -442,7 +468,7 @@ define \

define \
-test 'Feature branch check enabled, PR from feature branch passes' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-expected '0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 3' \
-author 'Good Author' \
-email '[email protected]' \
-subject 'package: add new feature' \
Expand Down
Loading