Enforce a commit message convention project-wide

[gustavovalverde]

Motivation

To automate some tools based on commits, we'd need a commit convention. In the long run, this also helps to hastily identify which commits are fixes, improvements, reactors, documentation, styling, and even breaking changes. And, as a result, it helps us understand which commit might have made a behavioral change, just by seeing the commit title.

Specifications

https://www.conventionalcommits.org/en/v1.0.0/#specification

Designs

• Implement conventional commits
• Found a way to enforce it project-wide
• The best way to enforce this kind of rules is locally (on each developer's terminal)
• Using pre-commit (for this and other things) might be helpful https://pre-commit.com/

Related Work

N/A

[teor2345]

Can you add this discussion to an engineering sync agenda?

[oxarbitrage]

Convention: https://www.conventionalcommits.org/en/v1.0.0/#specification

[dconnolly]

https://github.com/angular/angular/blob/22b96b9/CONTRIBUTING.md#-commit-message-guidelines

[dconnolly]

We could make it compatible with https://keepachangelog.com/en/1.0.0/, which we use.

[conradoplg]

For the types, to be compatible with Keep a Changelog, should we use:

• feat for Added (to comply with conventional commits)
• fix for Fixed (to comply with conventional commits)
• change for Changed
• deprecate for Deprecated
• remove for Removed
• security for Security

?

[dconnolly]

Using this in PR titles seems to be popular

[conradoplg]

My proposal is:

The convention applies to PR titles (which become commit messages for the merged squashed commits). It's optional for the commit messages inside the PR.

Allowed types (enforced), mapped to changelog entry types:

• security for Security
• feat for Added (to comply with conventional commits)
• fix for Fixed (to comply with conventional commits)
• deprecate for Deprecated
• remove for Removed
• change for Changed

If more than one applies, prioritize the higher up in the list.

Allowed scopes (maybe not enforced, just a guideline?):

• An inner crate, without the zebra- prefix (chain, consensus, rpc, etc.)
• ci (.yml workflows, Dockerfiles)
• doc
• build (dependencies and other Cargo.toml changes)
• Epic-related (nu5, lightwalletd)?
• Nothing is allowed

If more than one applies, use your best judgement, favor the one that contains more of the code.

Use ! instead of BREAKING CHANGE to indicate breaking changes.

Examples:

• fix(ci): make it faster
• security(chain): fix buffer overflow

Questions:

• We already use build for some CI and dependabot stuff (e.g. build(deps)). Should we use change(build) instead?
• Should we use the test scope or just favor the crate the test is in?
• Should we care about breaking changes at this point, or just after we have stable releases?

To be discussed later:

• How to enforce it, how to make it contribution-friendly

[teor2345]

• We already use build for some CI and dependabot stuff (e.g. build(deps)). Should we use change(build) instead?

I think feat(build), fix(build), ... are more specific.

• Should we use the test scope or just favor the crate the test is in?

I think feat(test), fix(test), ... are more specific.

• Should we care about breaking changes at this point, or just after we have stable releases?

Can we focus on breaking changes that affect known Zebra users?

For the current epics, that would mean marking breaking changes to:

remote Zcash peers:

• consensus rules
• network protocol

lightwalletd instances:

• existing RPCs that we've tested and beta released

When we have other users, we can start tracking breaking changes for them.

[upbqdn]

I sometimes end up repeating the same thing in the title of a PR; for example: "fix(doc): Fix various doc warnings #4514". I feel like I'm wasting space. I wonder if there's a good way to mitigate this. I could have used the word "Resolve" instead of "Fix" in the title, but that would make it even longer.

[mpguerra]

I think we can close this one now