Enforce good commit messages

[simoncozens]

Working on a 0.9.5 release changelog and it's horrible because I am undisciplined.

I would like a git commit hook which checks the commit messages is formatted as [area] ... where area is taken from a given list of areas.

[alerque]

You're telling me! If it's hard for you to figure out what you mean reading your own commit messages, imagine the rest of us out here.

I don't think a commit hook is the right solution, because there are times when hacking on stuff and you just want to cobble together a commit as a rollback point in case something goes sideways while hacking. The issue here is that such development sessions should get cleaned up using rebase before they are pushed to any permanent branch. Some needlessly incremental commits can be combined (debounced in hardware lingo), others just reworded to properly describe what they do. Requiring proper commit messages should happen before anything hits master or devel branches, but I wouldn't want my local machines slowing up the hacking process before the churn dies down.

Maybe just a reminder message in the commit hook, and an actual requirement on master/devel merges.

[alerque]

Also just a reminder, besides the full commit change log the closed issues in a milestone should be a pretty good summary of what goes into a release — especially if feature branches are used and put through the PR workflow even for little things we hack on independent of any listed issues. Besides having benefits for testing like CI checks, the merge workflow also groups commits by topic and the commit graph shape starts telling more of a story.

[rjmunro]

I would start by blocking commits to master and making all commits go though a branch and a PR. That way other people can review them, and the detail of the PR. You can list all the merges into master with git log --first-parent - this will correspond to all the pull requests, which should be a better summary of the changes since the last release.

I think you have to do "Require pull request reviews before merging" in the branches section of project settings.

[simoncozens]

We did that. What I'm looking to do is ensure that commits are categorised by bug fix, new feature, code cleanup, etc.

[alerque]

I'm not sure how much sense it really makes to have subject line tags for [bugfix], [feature], etc. In my experience following the Linux kernel's (and Progit's) guidelines on always starting commit messages with imperative present tense verbs pretty much solves that while still leaving some room for things that don't neatly fit into one box or the other.

Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behavior.

Noting which areas of the code are effected makes a lot more sense to me because it cuts down on the verbosity of the messages and makes it easier to skip over things that aren't what you're looking for. To this end we would really want a canonical list of areas. In the past few weeks we've used some ad-hoc ones but they aren't really very normalized yet.

• build
• docs
• input
• package
• cli
• shaper
• typesetter
• backend
• ???

[rjmunro]

@alerque You can do git commit -n to skip the commit hook for quick temporary hacks.

[simoncozens]

When we're writing the release notes, the most important thing is not so much the area of operation but whether a change is user-facing or just internal reorganisation. A lot of our development (unfortunately) is things like adding debugging information, tidying up code, improving error messages and so on; stuff which isn't very exciting for a user.

Within the user-facing changes, the most important things to know about are:

• Does this let me do something I couldn't do before? (e.g. [feature], [package])
• Does this improve/add support for a particular [language]?
• Is this a general [improvement] that I might notice? (e.g. better hyphenation, float handling, etc.)
• Does this [fix] something that was broken before?
• Does this introduce a[backward-compatibility] issue?
• ...

[alerque]

For further reading: https://medium.com/jobtome-engineering/how-to-generate-changelog-using-conventional-commits-10be40f5826c

[alerque]

I'm not actually a huge fan of any of the normalized systems I've seen so far, but I am a huge fan of having a normal and sticking to it — and even if we don't like every detail having an outside spec that we follow will make it a lot easier to conform and means we can use other people's tooling.

To that end so far I see:

• Conventional Commits, commitlint and commitlint-bot
• git journal
• git good commit
• turbo git
• fit commit
• gommit
• Automatic Github Changelog Generator

Of those the first one seems to have the most momentum behind it. Should we head that way?

Also check out Gitlab's CHANGELOG system for some heavyweight action! Lastly the lua_code_formatter project uses an iteresting very terse syntax that I don't know what to call.

[simoncozens]

I like the commitlint idea a lot, and agree with the idea of reusing other people’s tooling. Let’s set that up, with the bot. And autogenerating the changelog would be fine if supplemented with manual curation.

[alerque]

Re-opening because even with the configuration in the master branch now the commitlint bot is not doing its job. It is approving messages that should be rejected by the filter.

Locally it works fine. For example if I run commitlint --from upstream/master --to HEAD on my PR's I get a detailed breakdown of what commits meet approval and which don't, but the same results are not being offered as feedback on PRs.

[alerque]

So ... the commitlint-bot is actually working, but it does not yet support custom configuration so it is only using it's default rule set, not the one I configured for this repository. I'm looking into having Travis handle this instead.

[alerque]

Per this comment: z0al/commitlint-bot#1 (comment) ...

It looks like the way to do this is using GH Actions and a separate workflow for commitlinting from the rest of our CI jobs.

[alerque]

Fixed by #752.