Commit message validation as a branch protection setting (OD-1357)

jbauer opened 1 year ago

In order to mandate good commit messages I would like to be able to define a template/specification for commits as well as pull request title + description and OneDev should validate against this template/specification in order to deny bad commit messages / pull requests.

One use case is that you could automatically generate changelogs based on commit messages if you know all commits have a specific format. Another use case is that you could generate the next git tag based on commit messages (semantic versioning).

An example of a commit message specification is https://www.conventionalcommits.org

I think this could be implemented by letting the user configure a regex for commit messages / PRs. But as the above conventional commits spec is likely to be difficult to understand and maintain as a large regex I would also not mind if OneDev simply implements a conventional commits mode and let the user only configure allowed commit types (fix, feat, etc), commit scopes (core, docs, sql, etc..) and a max line length for the first line of a commit message.

As a real world example: Angular uses something like the above.

Commits: https://github.com/angular/angular/commits/main
Commit guideline: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Generated Changelog: https://github.com/angular/angular/releases

Some slides with further examples: https://slides.com/marionebl/the-perks-of-committing-with-conventions

Robin Shen changed title 1 year ago

Previous Value	Current Value
Pull Request / Commit message templates with validation	Commit message validation

Robin Shen changed title 1 year ago

Previous Value	Current Value
Commit message validation	Commit message validation as a branch protection setting

OneDev changed state to 'Closed' 1 year ago

Previous Value	Current Value
Open	Closed

OneDev commented 1 year ago

State changed as code fixing the issue is committed (78f305b7)

OneDev changed state to 'Released' 1 year ago

Previous Value	Current Value
Closed	Released

OneDev commented 1 year ago

State changed as build #3627 is successful

jbauer commented 1 year ago

@robin That looks nice! I tried it a bit with a test project and subject / line length checking works well!

Would you mind also add some footer checks as defined in the specification (https://www.conventionalcommits.org/en/v1.0.0/#specification) or is it too complicated?

The spec says that a footer starts after a blank line after the body. The footer consists of a list of key/value combinations following the syntax

<key>:<space><value>
<key><space>#<value>

with key having - instead of white spaces if the key consists of multiple words, e.g. Acked-by: or Reviewed-by:. The one exception of this rule is the use of BREAKING CHANGE which may be allowed as a synonym to BREAKING-CHANGE. However the breaking change footer must be upper case. The <value> can contain newline characters to fulfill the line length limit.

So for a footer you would need to check that the start of a line contains a key/value pair by checking for <key>:<space> or <key><space># or BREAKING CHANGE: or BREAKING-CHANGE (case sensitive).

Currently I can successfully commit something like

fix(core): asdasd

Ipsum est ratione consequatur illo odio rerum delectus nesciunt autem et in corrupti repudiandae ut.
Aut quidem et quaerat dolorem ea quasi laboriosam fugiat ea ducimus quos a expedita. Quidem ut
voluptatem consequatur et mollitia error excepturi accusantium et.

breaking change: Enim dolore dolore excepturi iure nisi dicta ut dolor exercitationem quis ut. 
Est illum ut illo rerum sed est nulla impedit. Odio fugiat minus aut veniam accusamus qui totam 
dolorem quo asperiores debitis illum.
reviewed by: abc
issue 123123

which has an invalid footer. Correct would be

fix(core): asdasd

Ipsum est ratione consequatur illo odio rerum delectus nesciunt autem et in corrupti repudiandae ut.
Aut quidem et quaerat dolorem ea quasi laboriosam fugiat ea ducimus quos a expedita. Quidem ut
voluptatem consequatur et mollitia error excepturi accusantium et.

BREAKING CHANGE: Enim dolore dolore excepturi iure nisi dicta ut dolor exercitationem quis ut. 
Est illum ut illo rerum sed est nulla impedit. Odio fugiat minus aut veniam accusamus qui totam 
dolorem quo asperiores debitis illum.
reviewed-by: abc
issue #123123

Robin Shen commented 1 year ago

I overlooked the footer specification. Filed a bug report:

#1367 - Conventional commit rule of branch protection does not check footer

jbauer commented 1 year ago

@robin Thanks. Because it is not mentioned above: Keep in mind that footer spec also allows blank lines within the footer (because newline is allowed), e.g.

BREAKING CHANGE: Summary of breaking change

Detailed description of breaking change and possible some instructions how 
to deal with the change. This could end up in a generated change log as well.


reviewed-by: abc
issue #123123

Robin Shen commented 12 months ago

Turns out that it is impossible to validate correctness of footer. The spec states that body has free form containing arbitrary number of line breaked paragraphs. So in case of an invalid footer, it can be a valid body. I checked commitlint and it also does not validate footer.

Type	New Feature
Priority	Normal
Assignee	Robin Shen