Decision required: Change commit messages to "conventional commits"

[thomaseizinger]

I'd like to propose to change our commit message convention to "conventional commits": https://www.conventionalcommits.org/en/v1.0.0/

The main driver for this is automation in the form of automated changelogs and automated version bumps. See #2902.

In particular, tools like https://github.com/googleapis/release-please can do this but there are many others and I'd like to make this decision independent of which tool we are going to use.

At the moment, the convention for our commit messages is:

• A prefix, which modules the PR touches (PR == commit message for us because we squash-merge PRs)
• A short description on what the PR does

With conventional commits, we would change this to:

• A prefix indicating the kind of change, e.g. feat, fix, chore, test etc
• A short description on what the PR does

See some examples here: https://www.conventionalcommits.org/en/v1.0.0/#examples

I benefits I see are pretty much the ones mentioned in the specification: https://www.conventionalcommits.org/en/v1.0.0/#why-use-conventional-commits

Something that works really well with our current workflow is the squash-merging. It means we don't have to educate users in how to write commit messages but just edit the titles of their PRs to match our conventions. That is not very hard and something one of us can do as we review it. The only important bit is that we need to review that title (and whether the change is breaking) before merging. I'd suggest we use the ! instead of BREAKING CHANGE to identify those.

There are also some downsides to conventional commits:

There is an argument that changelogs should be written manually and not auto-generated. The argument is mostly that changelogs are for humans and thus should be written with priority and clarity in mind, something that is hard to do in an automated fashion. There is more criticism here: https://common-changelog.org/#42-conventional-commits

I think the provided criticism is somewhat valid but can also be dealt with:

1. Our mapping of changelog entry to PR is already 1-to-1.
2. release-please differentiates between e.g. between chore and feat commits. chore PRs will not show up in the changelog. This avoids the "noise" problem.
3. We can always manually sort the changelog after a release by priority.
4. We already do and should continue to write PR titles really descriptive and in imperative form.

@mxinden @jxs @elenaf9 Please voice your opinion on this topic here :)

[mxinden]

I am in favor of this.

I would like to continue putting the path (i.e. in conventional commit terminology it is scope) into the commit message, e.g. swarm, protocols/kad or transports/tcp.

[thomaseizinger]

I would like to continue putting the path (i.e. in conventional commit terminology it is scope) into the commit message, e.g. swarm, protocols/kad or transports/tcp.

At least release-please will figure out what versions to bump based on the touched files so the scope is only informational.

Is there much value in adding the scope? It often takes up a significant portion of the commit message title, which makes it hard to keep under 50 characters (that is the recommended length).

Can we loosen this to a "we MAY add the scope"? I'd personally prefer not to, esp. when multiple things are being touched. GitHub's file free makes it quite to see what is being touched and I am sure there are funky Git one-liners too!

[elenaf9]

👍 In favor of conventional commit messages.

Is there much value in adding the scope? It often takes up a significant portion of the commit message title, which makes it hard to keep under 50 characters (that is the recommended length).

Can we loosen this to a "we MAY add the scope"? I'd personally prefer not to, esp. when multiple things are being touched. GitHub's file free makes it quite to see what is being touched and I am sure there are funky Git one-liners too!

Agreed. I stopped used full paths because of those reasons. In favor of optional scopes where it makes sense.

[thomaseizinger]

Thanks everyone, I'll consider this change to be approved unless someone objects by Friday, Nov 11th.

[mxinden]

No objections on my end.

Thanks everyone, I'll consider this change to be approved unless someone objects by Friday, Nov 11th.

I suggest updating https://github.com/libp2p/rust-libp2p/blob/master/docs/coding-guidelines.md accordingly and consider this the new convention once merged.

Is there much value in adding the scope?

It does help me to quickly understand the context of a pull request. A couple of examples:

• Use Description for commit message Use Description for commit message #3082

  Which description? A simple .github would have helped.

• Avoid collecting all RecordStore records into a Vec in the behaviour Avoid collecting all RecordStore records into a Vec in the behaviour #3021

  Which RecordStore? In our Kademlia implementation? In Gossipsub?

• Use Multiaddr instead of ConnectedPoint in DialError::WrongPeerId Use Multiaddr instead of ConnectedPoint in DialError::WrongPeerId #2793

  Is this in Swarm? Is this in core? Is this the usage of DialError in one of the behaviours?

In favor of optional scopes where it makes sense.

Sounds like a good middle ground.

[thomaseizinger]

I suggest updating https://github.com/libp2p/rust-libp2p/blob/master/docs/coding-guidelines.md accordingly and consider this the new convention once merged.

Where in that document would you like to see this documented?

I was thinking of enforcing this via mergify: #3083

[mxinden]

With #3083 I don't think we need any documentation. Thanks for doing both of these.

[jxs]

with #3165 should we re-think adding documentation?

[thomaseizinger]

with #3165 should we re-think adding documentation?

I'd suggest to use GitHub actions instead. There should be plenty of them out there that either directly validate conventional commits or let you define a regex for the title.

Worst case we use bash :)