To me, at least, the issue with that commit message is the signal-to-noise ratio. There is a lot of exposition for each piece of information. I prefer a more declarative commit message. However, from the writing, I suspect this is just due to the committer not being a native English speaker.

e.g. the first paragraph doesn't lose any important information trimming it down to:

"After adding a test matching the contents of router_routes.conf, `bundle exec rake` fails with:

    ArgumentError:
        invalid byte sequence in US-ASCII

"

Realistically, it would have been a better commit message if they'd given the shortlog SHA where the test was added that exposed the bug rather than an explanation of what the test does.

"After adding test <testname> (08c3e17), `bundle exec rake` fails with:"

Hmmm, I've always believed that no commit should break a build, even if you're committing the fix right after. Otherwise you're going to cause problems for `git bisect` or other practices of going through the history to find where a problem may have started.

Do other people commit breaking tests and then fixes?

I think this depends quite a bit on what other contributors are doing - it's one of those cases where several approaches are acceptable, but inconsistency is not.

"Commits should always build" is one doctrine I've seen. As you say, it makes bisecting and other error-analysis approaches easy. On the other hand, it risks either having large, opaque commits, or adding overhead to make intermediate commits build - possibly with flawed/meaningless behavior when they do.

Another is "the trunk should always build". In that case, you'd just squash branch commits down to logical groupings that are easy to analyze, whether or not things build. You can bisect on the trunk, but lose all guarantees about state on branches.

Finally, I've seen variations on "no commits that break the product", "no commits that make things worse", or "no committing failing tests without subsequent fixes". In this case, you can't generally commit broken builds, but can specifically add failing tests. The first rule just means "adding failing tests is ok", the second means "converting runtime bugs to failing tests is ok", and the third means "write your test and fix, but split (and ideally tag) the commits". All of these break bisect, but they guarantee the project itself won't become more broken from commit to commit, and they can help with other forms of reasoning about where bugs first occurred.

Every approach there seems viable if you stick to it. If there's no established practice, I suppose the best choice would be based on what sort of work and debugging is most likely to apply.

I might be missing something basic here. Isn't the "no commit should break a build" impossible to enforce on a codebase where you need to push a commit to run the tests?

Something where you can't test locally, like when testing on multiple architectures or when the tests just take too long for a laptop.

In my workflow, that would be in a feature branch, and exploratory branches can certainly break, but before I made a PR I would rebase my changes such that none of the commits broke the build.

It's also a different situation. The original one is "I've made a test that shows a problem." Your example is a surprise "I don't know whether this will pass my cloud-based tests." I would edit my branch if I had a surprise failure, since my initial code clearly wasn't correct.

I do this, but not in a way then end up on master. It's a driving force behind my preference for squash-and-rebase merge patterns.

A good bugfix PR is often two commits then: one with a test to catch the breakage, another to fix it so the tests pass. Reviewers can see the failing-then-passing CI job logs, so if they agree your test catches the bug, they have additional CI-automated validation your fix worked.

Then as long as you squash when completing the merge, you get the best of both worlds.

Having worked with the committer, I can tell you that he’s definitely a native English speaker.

I prefer that the commit includes the addition of a test in the test suit that get fixed (or a few). This is good because:

* It ensure that the bug is real. [1]

* It ensures that the bug is fixed. [1]

* It prevents reversions (assuming the test are run automatically).

* The test may prevent reversions in other related code, or discover other hidden bugs.

* It brings you closer to a 100% test coverage.

* You don't have to guess how to reproduce the bug, reading the comment.

* If the bug depends on subtle configurations, they should be set in the test. [2]

From time to time there are bugs that are obvious in the code, but they are too difficult to find a test for them.

[1] Been there, done that.

[2] Once I found a bug that depended on the local timezone.

100% test coverage is such an overrated stat.

Write a regression test (including its documentation) instead of just documenting the issue in human interpreted language, immutably.

Your future maintainer will thank you for not having to dig through repository history.

It goes without saying that commits should include tests that cover the change, where possible.

> immutably

That's what makes this modus operandi so powerful IMO - comments in code may go unmaintained, tests may start failing for other reasons, issue trackers come and go, developers leave the company, documentation rots.

The commit message is (unless you have a bad actor) immutably linked to the original change, and that's exactly why you should be thorough in expressing its reason for being. I can git checkout the point in time (perhaps having bisected) and have the information to allow me to reproduce the issue.

≥ Especially when the change appears to be trivial on the surface

Comments about the code should be in the code, where the next dev will see it. The more trivial a change, with far- reaching implications, the more important this is.

Doing so has heaps of benefits: future devs understand ramifications, shows that this code has been scrutinized, makes it easier when doing refactoring /yanking, or porting code.

That said, leaving the life story out will always be a good idea.

Agreed, but in this case, it was an encoding/whitespace change so there isn't really anywhere else to put this info.

IMO the repo is the code

However, I would have done a simpler commit and linked to an issue where I explained the problem/solution in more detail

This assumes your issue tracker doesn't change. I've been at my current position 8 years, and in that time we've had 3, and the first 2 are shut down.

We use Gitlab so it has both in the same project - but like you said that could change

> I don't want go look at an external issue tracker

Related question: are there projects that use git itself as issue tracker?

Pagure [0], Fedora's git forge, hosts code, issues, docs, and pull requests as four separate git repositories under the hood [1]. However, only project administrators can clone most of those repos.

[0]: https://pagure.io/pagure

[1]: https://docs.pagure.org/pagure/usage.html

I can imagine that working to a degree: Make a fork of a commit at an issue, then merge that fork back in with master at point of fix. Bit of a mess in the tree though.

This. It's the same with comments in code:

I don't want to read what the code does (I can read that myself, thanks!), I want to know WHY it does it the way it does it - especially, if there is a more obvious, better way.

Also: People leave companies. Or die. At some point in time, you won't be able to ask the original author.

No thanks. If I had a dime for every function that's so obviously self-documenting to what it does.. etc. If you tell others what it does and why, concisely and thoughtfully, no one has to try and mentally parse the what of some clever undescriptive block of code.

"You spent an enormous amount of time learning X, which is encoded in this three-letter bugfix. Don't make the next person go through that too."