tools/checkpatch.sh: check git commit format

[raiden00pl]

Summary

check git commit format with CI:

1. line length less than 80 for commit title
2. affected subsystem (detecting colon in commit title ":")
3. Signed-off-by line
4. no [VELAPLATO-*|WIP:] string in commit body

Reference: #16908

Impact

automation of boring activities

Testing

CI on private fork

examples:
https://github.com/raiden00pl/nuttx/actions/runs/17235852791/job/48900430219
https://github.com/raiden00pl/nuttx/actions/runs/17233782590/job/48893753448
https://github.com/raiden00pl/nuttx/actions/runs/17237377138/job/48905319722

[raiden00pl]

[DONE] we can also add VELAPLATFO-xxxx detection in commit body

[simbit18]

@raiden00pl Great idea

[michallenc]

I think we should also check for wip, WIP and similar in commit messages. These should not be merged into mainline imo.

[raiden00pl]

@michallenc I added WIP:|wip: to the blacklist (ending with a colon ":") to avoid false positives for words like "wipe".

[acassis]

@raiden00pl do you think it is possible to detect if some commit in the PR starts with a "!" and automatically prepend "[Breaking Change]" in the PR title?

I think it is better the forcing people to include it in the commit and in the PR message as @jerpelea and @cederom are willing... more info: #16823

[cederom]

• Great idea @raiden00pl thank you!! This will really save us all time and effort :-)
• Current git commit template / requirement is defined here https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md#15-commit-requirements. In case of errors you may put a reference so it is crystal clear why verification failed :-)
• Three fields are required: 1. topic, 2. description, 3. signature (git commit -s). You are already checking the title and signature, very nice, would it be possible to also check if description is present? :-)
• Ad. 1 (max line length 72 chars) I am used to 80 but I am fine with 72. We should discuss this on the mailing list so everyone is aware and can relate :-)
• Ad. 2 it may be hard to automate affected subsystem detection. ":" verification seems okay for now. Maybe with a better definition of the "affected subsystem" it would be easier to automate? For instance "boards/nucleo-f429zi:" would be valid because both "boards" and "nucleo-f429zi" are part of the path of affected files? But what when files are changed in many locations, would "boards/arm/stm32:" or "boards/stm32:" be okay is files changed under that location? Or what happens when new files are added and the location does not yet exist? Maybe going backwards from implementation to definition would be helpful here? :D
• Currently we have only one [..] tag defined and this is [BREAKING]. There is ongoing discussion to replace [BREAKING] with ! exclamation mark prefix in the title plus "BREAKING CHANGE:" string in the body. This sounds reasonable, will save git commit and pr title space, and probably we will replace it soon. So any kind of tag [..] may be blocked because we will soon have none defined?
• Update 1: If one of the commits contain breaking change mark (whatever it is) then whole PR should be marked breaking too, even if it contains also non-breaking commits, as @jerpelea suggested. I guess this is to separate breaking commits and PRs to help back porting during a release cycle. This makes sense, and I think maybe we also need some sort of PR cross references defined such as "Depends-on: " and "Provides:" (or something like that) not to miss any PR from a series when back porting? This is longer discussion but UML [1] may be a good nomenclature reference point?

Thanks again! :-)

[1] https://www.omg.org/spec/UML/2.5.1/About-UML/

[cederom]

@acassis: @raiden00pl do you think it is possible to detect if some commit in the PR starts with a "!" and automatically prepend "[Breaking Change]" in the PR title?

I think it is better the forcing people to include it in the commit and in the PR message as @jerpelea and @cederom are willing... more info: #16823

Alan there are several things to consider, all this was already said in referenced PR:

• Current Contributing Guidelines require to mark breaking changes both in git commits and PR titles. This was discussed over several months on the mailing list, then on the PR, and voted, for a good reason, for instance, to improve contributions quality, to help reviewers, to increase project quality, to improve self-compatibility, to improve maintenance.
• I am fine with replacing "[BREAKING]" tag with "!" prefix plus the "BREAKING CHANGE:" description in the body. This will even make automation simpler as it turns out. It will make things shorter, better to parse, we may easily check if both title and body contains required marks, etc.
• Breaking change mark is required in Git Commit because people use that in terminal / editor when browsing code (history) directly (from the git log). This helps users after change is introduced.
• Breaking change mark is required in the Pull Request title because: 1. Release Notes are generated from Pull Requests and contain referencing URL with the title, 2. This will bring more attention to a breaking Pull Request and its proper handling / verification. This helps users before change is introduced.
• Both before and after parts are important. Thus we have that clearly stated as requirement and we will not change that.
• We really need no more complications. The best things are simple. There is no need to change release notes generation scripts, that impacts the release process, we have a Release Manager, and we cannot do anything behind his back. There is no need for scripting to add "!" in the PR title if the user is lazy enough not to type in one character by hand that change will most likely not land into the upstream anyways.

I hope things are okay on your side, take care man! :-)

[raiden00pl]

@raiden00pl do you think it is possible to detect if some commit in the PR starts with a "!" and automatically prepend "[Breaking Change]" in the PR title?

probably this can be done with gh pr edit command, but I've never tested it

[raiden00pl]

I'm wondering if the entire checking logic shouldn't be moved to tools/checkpatch.sh. This way everything that check changes will be in one place, ease to use before creating PR.

[simbit18]

Hi @raiden00pl, I agree that this https://github.com/apache/nuttx/blob/master/.github/workflows/check.yml (tools/checkpatch.sh) is the most appropriate place for maintenance and optimisation of checks.
@lupyuen What do you think?

[raiden00pl]

I moved all checks to tools/checkpatch.sh so they can be run directly from .github/workflows/check.yml.
This way you can easily test changes locally.

[raiden00pl]

@cederom

Three fields are required: 1. topic, 2. description, 3. signature (git commit -s). You are already checking the title and signature, very nice, would it be possible to also check if description is present? :-)

this is already done. The error is triggered when the commit has fewer than 5 lines (including signed-off-by)

Ad. 1 (max line length 72 chars) I am used to 80 but I am fine with 72. We should discuss this on the mailing list so everyone is aware and can relate :-)

as for commit subject 72 most likely is OK, I'm not sure if it will be enough for commit body. Sometimes we need to add into commit body some output from compilation or from NuttX log and these are usually long lines.

Ad. 2 it may be hard to automate affected subsystem detection. ":" verification seems okay for now. Maybe with a better definition of the "affected subsystem" it would be easier to automate? For instance "boards/nucleo-f429zi:" would be valid because both "boards" and "nucleo-f429zi" are part of the path of affected files? But what when files are changed in many locations, would "boards/arm/stm32:" or "boards/stm32:" be okay is files changed under that location? Or what happens when new files are added and the location does not yet exist? Maybe going backwards from implementation to definition would be helpful here? :D

all checks are now integrated in tools/checkpatch.sh, this way it should be easier to integrate more complex checks. But that's a topic for later :)

[raiden00pl]

Another small issue I noticed is the lack of color output for checkpatch.sh. Errors should be clearly displayed in red, and when everything is OK, we should print a green message. I don't know how CI log will print ANSI escape codes, but if needed this behavior can be controlled with a simple flag that disable color output for github workflows.

[acassis]

@cederom forcing people to include it twice will not work, you know that. Today they don't even put it in the commit, imagine asking them to put it in the commit and the include it in the PR message.

I think it is better to use this @raiden00pl solution to simplify things!

[acassis]

Another small issue I noticed is the lack of color output for checkpatch.sh. Errors should be clearly displayed in red, and when everything is OK, we should print a green message. I don't know how CI log will print ANSI escape codes, but if needed this behavior can be controlled with a simple flag that disable color output for github workflows.

Agree! We spend more time scrolling the screen to find the issue than actually fixing it. I remember I discussed it with @btashton in the past about the idea to create a summary at the end with better description of the issue.

[raiden00pl]

After some thought, I removed the line length check for commit message, but left the check for commit title length and increase it to 80. This way, the short git log will be easier to read, but we still have the option to put longer lines in the commit message (like compilation output).

I'm opening this PR, I'll add the colored output to checkpatch.sh later

EDIT: Below is an example of why we need the commit title length limit :)

[cederom]

Thank you @raiden00pl amazing work! :-)

[raiden00pl]

The problem I see now is that the nuttx repo uses the checkpath.sh from the branch used in the pull-request, not the nuttx upstream repo. This way if the PR is not based on the latest master - checking for commit message doesn't work.

I'm not sure if this is the correct approach. We probably should check PRs with the upstream checkpath.sh, not the user branch.

[simbit18]

Hi @raiden00pl, but the workflow check.yml, if I'm not mistaken, uses checkpatch.sh from the
NuttX upstream repository

https://github.com/apache/nuttx/blob/master/.github/workflows/check.yml#L35-#L49

You can view this PR #16941. The branch is not synchronized and has the old checkpath.sh.

https://github.com/acassis/incubator-nuttx/blob/imxrt_dualusb/tools/checkpatch.sh

log
https://github.com/apache/nuttx/actions/runs/17359811421/job/49278273697

Successfully installed black-25.1.0 chardet-5.2.0 click-8.2.1 cmake-format-0.6.13 cmakelang-0.6.13 codespell-2.4.1 cvt2utf-1.3.2 flake8-7.3.0 isort-6.0.1 mccabe-0.7.0 mypy-extensions-1.1.0 packaging-25.0 pathspec-0.12.1 platformdirs-4.4.0 pycodestyle-2.14.0 pyflakes-3.4.0 six-1.17.0
8545a53793 Merge 6aa3bb1c1f6507d1c7223df2b3790c8c8a2288d0 into 3430ad2e1ca113cd0a34b1753840617a8da0dc83
6aa3bb1c1f doc: Add info about usbdisk to arcx-socket-grid
2e30f75957 boards/arcx-socket-grid: Add support to two USBs
581d70fe90 arch/arm/imxrt: Modify USB EHCI driver to support two USBs
../nuttx/tools/checkpatch.sh -c -u -m -g 3430ad2e1ca113cd0a34b1753840617a8da0dc83..HEAD
Used config files:
    1: .codespellrc
All checks pass.

All checks pass. is present in your change to checkpath.sh

[raiden00pl]

@simbit18 That's right, I didn't notice "All checks pass" message. Then the problem is somewhere else, because this PR shouldn't have passed the tests - no commit message in the last commit.

[raiden00pl]

Okay, I see where the problem is. Checking the commit range isn't working properly because the content of all commits from the PR is merged and passed as one long string, causing some checks to not work correctly for many commits (missing signed-of-by, missing commit message and missing colon)

[raiden00pl]

fixed here #16945