Silverstripe-framework: RFC-9687 Commit Prefixes

[dnsl48]

Overview

When building changelogs for every release, our tooling relies on commit message prefixes.
Currently, we have the following conventions documented:

| Category | Prefix | Description |
| --- | --- | --- |
| Features and Enhancements | NEW | New feature or major enhancement (both for users and developers) |
| API Changes | API | Any API modifications or deprecations |
| Bugfixes | BUG | Bugfix or minor enhancement on something developers or users are likely to encounter |

The current recommendation is to only prefix noteworthy commits.

Our release tooling also handles the following undocumented prefixes:

| Category | Reg Ex (PCRE) |
| --- | --- |
| Old SS style security identifiers | /^(\[SS-2(\d){3}-(\d){3}\]):?/i |
| API Changes | /^(APICHANGE \|API-CHANGE\|API CHANGE)\b:?/i |
| Features and Enhancements | /^(ENHANCEMENT \|ENHNACEMENT\|ENH\|FEATURE)\b:?/i |
| Bugfixes | /^(BUG FIX \|BUGFIX\|BUGFUX\|FIXED\|FIXING\|FIX)\b:?/i |

The following list of ad-hoc comments is ignored by our release tooling (meaning such commits don't make it into changelogs):

| Reg Ex (PCRE) | Description |
| --- | --- |
| '/^Merge/' | Merge-ups and PR merges |
| '/branch alias/' | branch-alias maintenance |
| '/^Added(.)changelog$/' | auto-generated by release tooling; add changelog to the docs |
| '/^Blocked revisions/' | something from SVN era |
| '/^Initialized merge tracking /' | something from SVN era |
| '/^Created (branches\|tags)/' | couldn't find any entries |
| '/^NOTFORMERGE/' | something from SVN era |
| '/^\s$/' | couldn't find any entries |

Everything else is included to:

• CMS changelogs : Bugfixes section
• CWP changelogs : Other changes

Problem statement

The existing convention is missing some prefixes important for the changelog generation components of the release tooling.
As a workaround we have to use a lot of implicit and undocumented rules to make changelogs more clear and readable.

Suggestion

• Leave the prefixes optional and NOT a requirement for commits

• Update the documented conventions

  • introduce new prefixes required for release tooling to build better (more readable) changelogs and have more granular control
  • document ALL used prefixes
  • remove unnecessary (and undocumented) prefixes

• Update release tooling to follow the documented conventions as close as possible

Implementation

• Leave API "as is"
• Deprecate BUG and make it FIX instead
• Update FIX description and remove or minor enhancement. (NEW or ENH to be used instead)
• Update NEW description to: New feature or major enhancement (both for users and developers)
• Add new prefixes (existing, but undocumented) : FIX, ENH
• Remove undocumented: APICHANGE, API-CHANGE, API CHANGE, ENHNACEMENT, ENH, BUG FIX, BUGFIX, BUGFUX, FIXED, FIXING
• Remove all ad-hoc ignored rules
• Add new prefixes:
  
  
  
  • MNT - Maintenance related commits (e.g. CI)
  
  
  • /^Merge\s(pull\srequest\|branch)\s/ - merge-ups and PR merges (auto-gen by git and github)
  
  
  • /^DOCS?/ - documentation updates and additions
  
  
  • DEP - Dependency updates
  
  
• Explicitly document that all Prefixes are case insensitive

Here is the final list of prefixes suggested:

| Category | Prefix | Description |
| --- | --- | --- |
| API | API | Any API modifications or deprecations |
| Bugs | FIX | Bug fixes |
| Documentation | DOC | Documentation updates and additions |
| Feature | NEW | Add new functionality |
| Feature | ENH | Improve existing functionality |
| Maintenance | MNT | Maintenance commits (e.g. CI) |
| Maintenance | DEP | Dependency version updates |
| Maintenance | Merge | PR merges and merge-ups |
| Security | PCRE: '/^(\[?CVE-(\d){4}-(\d){4,}\]?):?/i' | Security patches |

[dnsl48]

/cc @silverstripe/core-team

[sminnee]

• How would you classify refactoring work?
• Why do we have both "BUG" and "FIX"
• Is it important to keep "CI", "RELEASE", and "DEPS" separate?
• ENHANCEMENT is very long and the recommend length of the first line in a commit is quite limited.

[dnsl48]

How would you classify refactoring work?

Enhancement

Why do we have both "BUG" and "FIX"

IMHO easier to have both than decide which one is better and which we should ditch. Although, could just leave only BUG. Although, from looking at the git logs I concluded that Fix feels more natural in that context sometimes and many people prefer using FIX.

Is it important to keep "CI", "RELEASE", and "DEPS" separate?

I guess not too much. If we consider changelog generation tools as the only user for prefixes, then maybe we could come up with a single prefix for all of them (e.g. MAINTENANCE or MNT). On the other hand, if we want the Git log to look more self-descriptive in general, perhaps it's worth keeping them separate as they address different scenarios.

ENHANCEMENT is very long and the recommend length of the first line in a commit is quite limited.

Fair enough. Could have ENH instead? Although, that's less readable for some people, especially if considering Git log with no context of changelog generation tools.

[sminnee]

IMHO easier to have both than decide which one is better and which we should ditch.

Like when the project had tabs _and_ spaces? ;) I'd say we deprecate BUG but you may still want to have your changelog generation tool support it for BC.

Fair enough. Could have ENH instead? Although, that's less readable for some people, especially if considering Git log with no context of changelog generation tools.

Yeah, I don't have a good answer

[emteknetnz]

Overall, I like the direction this RFC is going in where it's providing better clarification. Some comments.

New feature or major enhancement (both for users and developers)

What's the threshold for when an enhancement is 'major'? I've seen NEW on plenty of rather trivial enhancements, so I'd say just drop the word 'major'

ENH / ENHANCEMENT

IDK. Pretty much what I see in practice is:

• Minor release = NEW
• Patch release = FIX

Minor release generally means it includes a new public function, or config. So no matter how trivial the change it, it is technically NEW functionality

Performance enhancement work generally involves a new class/function or two, so it's NEW

Patch releases are generally just bug FIX's. I see very little in the way of work that is strictly refactoring only. And refactoring with no end-user benefit is hardly worth calling specially in the changelog

I'm just not really seeing the benefit in using it?

remove ... minor enhancement [from BUG/FIX]

Definitely agree there

I'd say we deprecate BUG

Yup, feels weird to prefix you commit with BUG, makes it sound like you're writing bugs rather than fixing them

DEPS

Sounds weird to me, I'd prefer the singular DEP which matches all the other singular prefixes. DEPS? to allow both seems fine to me

CI

Can we change this to CID so that:
a) it allows for continuous deployment related commits, such as https://github.com/silverstripe/silverstripe-framework/blob/4/.github/workflows/main.yml
b) satisfies my "make everything a 3 letter acronym" OCD

RELEASE

Also allow REL or RLS =]

MNT

That said I'd rather just use MNT for anything that's not FIX or NEW or API (side note: I hardly see API used). I don't really see any value to the end-user except for maybe DEP, though even then it's probably very low utility.

[dhensby]

It's definitely a good idea to formalise the commit messages and I like that.

I note that you don't plan to enforce the message formats, which I think is wise as it may cause more friction when contributing. However, if we do decide to change that, there are some good linting tools we could look at using (here's one: https://github.com/marketplace/commit-message-lint)

[dnsl48]

there are some good linting tools we could look at using (here's one: https://github.com/marketplace/commit-message-lint)

Yes, I really like this idea. I don't know how others feel about that, so I suggest anyone who wants to discuss that in more depth, upvote Daniel's message and we could come up with a new RFC when we've got over with this one. Just to spin things more quickly I'd like to split that into a separate discussion.

[dnsl48]

I am sorry, just noticed some markup issues in the original post. Particularly, it did not show the regular expressions in the tables of the Overview section. This is fixed now.

[sminnee]

If we don't have a linter, it means we plan to merge PRs that don't meet this standard. If we plan to do that it means that our tooling needs to deal with, say, 90% compliance rather than 100%.

• is our tooling going to handle that? How will it behave in such a situation?
• is the downside of that situation greater than or less than the downside of making contribution more awkward and/or of forcing mergers to tidy up commit messages (force-pushing back to the submitter's branch) as part of peer review?

[emteknetnz]

When merging a single commit or squash-merging multi-commits via github you get the option to update the commit message, so whoever merges can just add it in without having to go back to the original author

Still plenty of room for human error though, so agree that we won't get 100% compliance

[dnsl48]

If we don't have a linter, it means we plan to merge PRs that don't meet this standard.

Yes, this is the current "de facto". Although, recently we introduced Merge Checklist which includes a reminder for Peer Reviewers to check commit messages.
Either way, currently we only have 3 documented prefixes: API, NEW and BUG. Everything else is unconventional.

is our tooling going to handle that? How will it behave in such a situation?

Yes, sorry, I initially messed up the markdown in the original post above. Now it's fixed. The tables there contain all the commit prefixes that our release tooling recognizes. Everything else is going into Bugfixes section in CMS and Other changes section in CWP.
Here are examples:

• CMS: https://github.com/silverstripe/silverstripe-framework/blob/4/docs/en/04_Changelogs/4.6.1.md
• CWP: https://github.com/silverstripe/cwp/blob/2/docs/en/05_Releases_and_changelogs/cwp_2.6.1.md

is the downside of that situation greater than or less than the downside of making contribution more awkward and/or of forcing mergers to tidy up commit messages (force-pushing back to the submitter's branch) as part of peer review?

I believe this is a separate issue to solve: easier to contribute with rubbish commit messages VS harder to contribute with better commit messages. Our current convention is commit prefixes are optional, which leaves it up to Peer Reviewer to decide.
This RFC does not aim to affect that issue, however. The only outcome we are after right now is to document our Commit Prefixes better and give more granular control over changelog generation.
However, this may be a first step towards more automation and linting (shall we decide we want it). Still, I believe it is better to leave for another RFC after this one is settled down.

[ScopeyNZ]

In the past, I haven't bothered to ask people to fix commit messages if it's the only feedback on the PR. Instead, I've occasionally squashed a PR into one commit with an appropriate message before merging.

I don't think that this should become "your PR will not be merged if you don't follow this rule", but I'm happy to follow whatever standard we set here.

[dnsl48]

Thanks to everyone for the feedback! I'm updating the original post and including the following suggestions:

• (@sminnee, @emteknetnz) Deprecate BUG and leave FIX as the only option for bugfixes
• (@sminnee) ENH for enhancements (no need for the full version ENHANCEMENT)
• (@emteknetnz) DEP instead of DEPS for dependency updates
• (@sminnee, @emteknetnz) MNT (as for maintenance) instead of CI and RELEASE

Another addition from me:

• Prefixes are case insensitive

The suggestions I would like to challenge:

1. Drop ENH in favour of NEW
   There are still some use cases for ENH where NEW doesn't feel right. E.g. tuning configuration settings, simple refactoring or UI improvements are some examples.

2. Using MNT instead of DEP
   I believe changelogs must reflect dependency updates, as they affect user projects, which is not the case for something like travis configs (CI).

[emteknetnz]

I'm fine with keeping ENH and DEP

[sminnee]

I have no objections to Serge's latest recommendations 👍🏻

[dhensby]

When merging a single commit or squash-merging multi-commits via github you get the option to update the commit message, so whoever merges can just add it in without having to go back to the original author

Yes, but we don't use squash-merge. We only use merges.

[maxime-rainville]

@dnsl48 That's a great idea.

• Do we have a prefix, for "this shouldn't be in the changelog"?
• I'm thinking we probably need to rethink how we generate changelog to match that new category list.
• I would be inclined to keep both FIX and BUG just because many community contributors will probably had those prefixes without knowing the convention. But I'm fine with dropping one as well.

@dhensby Last time we chatted about squash-merge internally no one could remember what the problem with squash merging was. We've been using them for a while now. Do we have the down side of squash-merging documented anywhere?

[dhensby]

@dhensby Last time we chatted about squash-merge internally no one could remember what the problem with squash merging was. We've been using them for a while now. Do we have the down side of squash-merging documented anywhere?

Good question - I can't find it either, I thought it had been discussed in a GH issue, but maybe it was actually in slack (which is an issue given we can't see more than 10,000 messages ago).

I think squash-merges are more acceptable than rebase-merges as a rebase merge completely messes with the history of the PR but also the main branch. I'm not sure that this is the place to discuss or go into details about the merits of each approach and for some it may simply be a personal decision. However as you progress from merge -> squash & merge -> "rebase merge" more and more history of the work is lost. The latter two options also get in the way of collaboration, if you are branched off of a branch that then get's squashed or rebased then your work has all these orphaned (and conflicting) commits that then need to be removed, git can't use it's knowledge from previous merge resolutions to keep things tidy.

[sminnee]

From memory, losing a reference to the source PR was the issue - it's good to have that traceability. I believe rebase merge causes that but maybe not squash merge.

For my part I like to avoid squash merge if the PR has been interactively rebased to have some logical sequencing in its commits - it makes git blame a bit better.

But if that care hasn't been taken and the PR is essentially a bunch of "ooops, not quite" commits then squashing makes sense.

[dnsl48]

Hey @maxime-rainville, thanks for chiming in!

Do we have a prefix, for "this shouldn't be in the changelog"?

I'm assuming that's going to be MNT

I'm thinking we probably need to rethink how we generate changelog to match that new category list.

Yes, fair call. We already have some initial thoughts. Some of it we captured here: https://github.com/silverstripe/cow/issues/186

I would be inclined to keep both FIX and BUG just because many community contributors will probably had those prefixes without knowing the convention

We shall support both for at least until the next release, and maybe even longer. However, no need to support both explicitly in the documented convention.

[dnsl48]

Hello @silverstripe/core-team
It looks like all questions are answered and all issues are resolved.
It is ready for a vote.

Please upvote or downvote this message if you agree or disagree with the proposal respectively.

---

Here's the proposed implementation:

• Leave API "as is"
• Deprecate BUG and make it FIX instead
• Update FIX description and remove or minor enhancement (NEW or ENH to be used instead)
• Update NEW description to: New feature or major enhancement (both for users and developers)
• Add new prefixes (existing, but undocumented) : FIX, ENH
• Remove undocumented: APICHANGE, API-CHANGE, API CHANGE, ENHNACEMENT, ENH, BUG FIX, BUGFIX, BUGFUX, FIXED, FIXING
• Remove all ad-hoc ignored rules
• Add new prefixes:
  
  
  
  • MNT - Maintenance related commits (e.g. CI)
  
  
  • /^Merge\s(pull\srequest\|branch)\s/ - merge-ups and PR merges (auto-gen by git and github)
  
  
  • /^DOCS?/ - documentation updates and additions
  
  
  • DEP - Dependency version updates
  
  
• Explicitly document that all Prefixes are case insensitive

Here is the final list of prefixes suggested:

| Category | Prefix | Description |
| --- | --- | --- |
| API | API | Any API modifications or deprecations |
| Bugs | FIX | Bug fixes |
| Documentation | DOC | Documentation updates and additions |
| Feature | NEW | Add new functionality |
| Feature | ENH | Improve existing functionality |
| Maintenance | MNT | Maintenance commits (e.g. CI) |
| Maintenance | DEP | Dependency version updates |
| Maintenance | Merge | PR merges and merge-ups |
| Security | PCRE: '/^(\[?CVE-(\d){4}-(\d){4,}\]?):?/i' | Security patches |

[sminnee]

With Dependency version updates - what about Dependabot?

[chillu]

I think MNT and DEP is unnecessary friction for contributors, it's yet another thing to remember. Are we going to create any views that specifically include commits tagged with these, as opposed to just filtering them out in changelogs just like any other untagged commits? But given this won't be enforced on every contribution, I don't feel strongly about it.

The net benefit from this proposal in my view is that it's more likely for changelog viewers to distinguish notable new features (NEW) from less notable enhancements (ENH), without also relying on a human summarising those manually at the top of said changelog. And this makes auto-generated and per-module changelogs more useful, where we don't put in that extra effort of manual summaries.

[dnsl48]

With Dependency version updates - what about Dependabot?

I think we can handle that in our release tooling. No need to document that separately.

I think MNT and DEP is unnecessary friction for contributors, it's yet another thing to remember. Are we going to create any views that specifically include commits tagged with these, as opposed to just filtering them out in changelogs just like any other untagged commits?

The current idea is to hide MNT in changelogs, but show DEP as it affects user projects. However, we can play with that granularity later when we patch our changelog generator

[ScopeyNZ]

IIRC squash merge doesn't create a merge commit either, if that's something we're concerned about. When I've squash merged in the past, I've done the squash by hand, and merged. And I've really only done it when the separation of commits provides no value - eg. git commit -m typo.

[chillu]

The current idea is to hide MNT in changelogs, but show DEP as it affects user projects. However, we can play with that granularity later when we patch our changelog generator

I'd suggest that the committer should make a call whether dependency updates are classified as ENH, FIX, or not noteworthy. That'll be hard to retain once we do more automated dependency updates (aka Dependabot), but I doubt you can force those bots to prefix with DEP either. I still don't really buy the usefulness of the MNT or DEP prefixes. In the end, there might be a few times when its worth pointing out a dependency security update, otherwise it's just noise. What about nested dependencies? In the end, users interested in this level of detail should do a composer update --dry-run.

[chillu]

Just stumbled on a "kind of changelog standard". Not saying we should adopt it, but interesting read: https://keepachangelog.com/en/1.0.0/

[sminnee]

The main relevant takeaway from that for me was "don't make change logs this way" which is... perhaps a bridge too far right now 😂

Definitely beyond the scope of this PR anyway

[dnsl48]

I still don't really buy the usefulness of the MNT or DEP prefixes.

Usefulness of MNT is to hide the stuff from the changelog. Usefulness of DEP is arguable, but dependencies are affecting users, so showing it in the changelog may be of use to some and not to others. Still, that's a change affecting users, which is not the case for MNT.

I doubt you can force those bots to prefix with DEP either

Those bots usually have their own standard format for commit messages. We can deal with it in the release tooling. I don't think we want to document bots behaviour in our conventions.

when its worth pointing out a dependency security update, otherwise it's just noise

Fair enough. Although, that's still arguable. I think we might play with it and see what happens. If it's not gonna give people much value we should be able to deprecate DEP and start using MNT instead to conceal any dependency updates from the changelogs. However, before we decide that I would try to prototype both versions of the changelog (with and without DEP), ask around for opinions and then make a data-driven decision. Until then it's safer to show in the changelogs everything that affects users (including DEP) IMHO

[ScopeyNZ]

Just noting here that we used to omit anything without API, ENH, or FIX (and related tags) from changelogs, but we starting including all other commits under an "All commits" header for security auditing reasons - the auditors requested a full commit changelog. I assume that requirement isn't changing?

[dnsl48]

for security auditing reasons

For security auditors we generate a separate changelog that includes everything. No need to document that in our open-source conventions.

[ScopeyNZ]

So if that changelog is generated separately, why do we even put any unlabelled commits in the changelog?

[dnsl48]

why do we even put any unlabelled commits in the changelog

My guess is because we don't have strict rules about having commit prefixes, and giving more information in the changelog feels better than giving less.
Regarding "noteworthy changes" - we tend to dedicate separate paragraphs in the changelogs for those, so commit prefixes feel a bit irrelevant in that context.

[dnsl48]

Thanks for the feedback and support everybody!
I think we can call it accepted.
I'll be preparing the follow-up updates for the documentation