Skip to content

dist/tools: add no-merge and colon check to commit-msg check#21803

Merged
crasbe merged 5 commits intoRIOT-OS:masterfrom
crasbe:pr/check_commits
Oct 29, 2025
Merged

dist/tools: add no-merge and colon check to commit-msg check#21803
crasbe merged 5 commits intoRIOT-OS:masterfrom
crasbe:pr/check_commits

Conversation

@crasbe
Copy link
Copy Markdown
Contributor

@crasbe crasbe commented Oct 17, 2025 •
edited
Loading

Contribution description

As discussed in #21772, the Commit Message check currently does not test for no-merge commit messages such as "WIP", "TEMP", "DONOTMERGE", "DELETEME", ... (anyone got more ideas btw?)

Also it did not test for a missing colon nor a missing space after the colon. which it does now.

Testing procedure

The script should fail in this PR already and I prepared some test commits. Please note that I had to get creative with the donot/merge commits to not trigger my own script 🤣

Also here is what you'd see on the console and a screenshot of it.

cbuec@W11nMate:~/RIOTstuff/riot-guides/RIOT$ git log --oneline -n8
6d20754a75 (HEAD -> pr/check_commits, origin/pr/check_commits) donot/merge missing colon
6ebc705df8 donot/merge:missing whitespace
fca191bb76 donot/merge: Loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong Boi
a81a95ce2b DONOTMERGE: test
7269957e68 dist/tools: check for colons in commit-msg script
b6b195b146 dist/tools: fix shellcheck warnings in commit-msg script
f113a80b10 dist/tools: check for no-merge keywords in commit-msg
ce69265dc9 (origin/master, origin/HEAD, master) Merge pull request #21337 from crasbe/pr/nucleo-doc

cbuec@W11nMate:~/RIOTstuff/riot-guides/RIOT$ ./dist/tools/commit-msg/check.sh ce69265dc9
Error: The commit message is missing a colon after the area designation.
    "donot/merge missing colon"
Warning: The colon after the area designation is not followed by a space.
    "donot/merge:missing whitespace"
Error: Commit message is longer than 72 characters:
    "donot/merge: Loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong Boi"
Error: Commit message contains no-merge keyword: 'DONOTMERGE: test'
    "DONOTMERGE: test"
Warning: Commit message is longer than 50 (but < 72) characters:
    "dist/tools: fix shellcheck warnings in commit-msg script"
image

Issues/PRs references

Fixes #21772.

@crasbe crasbe requested a review from jia200x as a code owner October 17, 2025 23:00
@crasbe crasbe added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 17, 2025
@github-actions github-actions bot added Area: doc Area: Documentation Area: tools Area: Supplementary tools labels Oct 17, 2025
@crasbe crasbe removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Oct 17, 2025
@riot-ci
Copy link
Copy Markdown

riot-ci commented Oct 17, 2025 •
edited
Loading

Murdock results

✔️ PASSED

bdf88c1 CONTRIBUTING: explain no-merge keywords

Success Failures Total Runtime
1 0 1 01m:41s

Artifacts

AnnsAnns
AnnsAnns reviewed Oct 20, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@AnnsAnns AnnsAnns left a comment

Choose a reason for hiding this comment

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

Looks good, I don't feel confident enough about my shell script knowledge to be the one to approve this though.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 21, 2025

Looking at the code to review it, I was wondering where the "fixup!"/"squash!" etc checks went, turns out, it is in a completely different file

RIOT/dist/tools/pr_check/check.sh

Lines 35 to 41 in c99a260

keyword_filter() {
grep -i \
-e "^ [0-9a-f]\+ .\{0,2\}SQUASH" \
-e "^ [0-9a-f]\+ .\{0,2\}FIX" \
-e "^ [0-9a-f]\+ .\{0,2\}REMOVE *ME" \
-e "^ [0-9a-f]\+ .\{0,2\}Update"
}

Maybe it makes sense to put those the keyword checks there?

@crasbe
Copy link
Copy Markdown
Contributor Author

crasbe commented Oct 22, 2025

Sorry for the force-pushing, I couldn't make the changes just with fixup without screwing everything up 😅

@crasbe
Copy link
Copy Markdown
Contributor Author

crasbe commented Oct 22, 2025

Now it works as desired:
image

image

@miri64 is that how you would like to have it? :)

miri64
miri64 reviewed Oct 23, 2025
View reviewed changes
miri64
miri64 reviewed Oct 23, 2025
View reviewed changes
miri64
miri64 reviewed Oct 23, 2025
View reviewed changes
@crasbe crasbe requested a review from miri64 October 29, 2025 10:47
@crasbe
Copy link
Copy Markdown
Contributor Author

crasbe commented Oct 29, 2025

@miri64 I think all of the review points are addressed now 🤔

miri64
miri64 approved these changes Oct 29, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Just some (potentially follow-up) suggestion for user-experience improvement. Other than that: LGTM. Please squash (and remove the test commits, to clear the tests ;-))

dist/tools/pr_check/check.sh
ANNOTATION="${ANNOTATION}\n\nPLEASE ONLY SQUASH WHEN ASKED BY A "
ANNOTATION=""
while read -r commit; do
ANNOTATION="${ANNOTATION}Commit needs to be squashed or contains a no-merge keyword: \"${commit}\"\n"
Copy link
Copy Markdown
Member

@miri64 miri64 Oct 29, 2025

Choose a reason for hiding this comment

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

Looking at the output of that: Should we maybe add some information what exactly the "no-merge keywords" are.

Copy link
Copy Markdown
Contributor Author

@crasbe crasbe Oct 29, 2025

Choose a reason for hiding this comment

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

Since we already point to https://github.com/RIOT-OS/RIOT/blob/master/CONTRIBUTING.md#squash-commits-after-review, I added some remarks there.

@crasbe crasbe force-pushed the pr/check_commits branch 2 times, most recently from e7be352 to bdf88c1 Compare October 29, 2025 19:48
@crasbe crasbe enabled auto-merge October 29, 2025 19:50
@crasbe crasbe added this pull request to the merge queue Oct 29, 2025
Merged via the queue into RIOT-OS:master with commit 34ec109 Oct 29, 2025
26 checks passed
@crasbe crasbe deleted the pr/check_commits branch October 29, 2025 22:29
@crasbe
Copy link
Copy Markdown
Contributor Author

crasbe commented Oct 29, 2025

Thank you for the reviews :)

@crasbe crasbe mentioned this pull request Nov 6, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 7, 2025
@crasbe
Merge pull request #21859 from crasbe/pr/fix_no_merge_keywords
1e920d3 
dist/tools: fix no-merge-keywords of pr_check (fix regression from #21803)
@benpicco benpicco added this to the Release 2025.10 milestone Dec 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AnnsAnns AnnsAnns AnnsAnns left review comments

@miri64 miri64 miri64 approved these changes

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

Assignees

No one assigned

Labels

Area: doc Area: Documentation Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2025.10

Development

Successfully merging this pull request may close these issues.

static tests: Commit Check does not check for missing Colon

5 participants

@crasbe @riot-ci @miri64 @AnnsAnns @benpicco