Long commit messages

[nbren12]

A common formatting recommendation for git commits is the 50/72 rule. Titles should be at most 50 characters wide and the body should 72. This is to ensure that the titles can be quickly scanned in a terminal with git log --online and that the body does not overflow when viewed in a 80-char width terminal with some indendation. Also, github seems to truncate commits with title longer than this limit.

We don't always follow this rule, which makes it harder to follow our git history. Should we enforce it with a commit-msg git hook?

[mcgibbon]

What is the command you're talking about? That one isn't recognized on my VM.

$ git log --online
fatal: unrecognized argument: --online

When I was scanning git history to see what had happened over the past year in preparation for the advisory council, any squash-merged PRs were easy to scan over. I only had to spend a little bit more time on non-squashed PRs, didn't have issues with commit title length on those.

This would be somewhat disruptive to my workflow, where I don't worry too much about commit-level details and instead make a clean final commit message on PR to be squash merged. I'd prefer not to have a hook enforcing this.

[nbren12]

Sorry --oneline.

In the github console it is truncated on the project homepage:

And in certain commits: https://github.com/ai2cm/fv3net/commit/04c4d4c358f6f00f635c6803da14046200084ec8

Perhaps github has its own mechanism to ensure squash merged commits don't have very long titles.

[nbren12]

But it would be nice to encourage clean commits somehow. Maybe a simple warning message suggesting the author consider rewording or splitting the commit could work. I find a clean commit history can simplify review and reduce need to ask question about implementation/trial-error. Despite what you say, I think most of us (including you @mcgibbon ) do a pretty good job keeping a clean commit log, so I don't think enforcing standards around this would be that disruptive.

[nbren12]

And in some cases the compression isn't lossy: e.g. "Bring spectral normalization code into fv3fit package " can be reworded to "Vendorize spectral normalization"

[mcgibbon]

Sampling my git logs, it seems like a 55-character limit would only have hit a couple times, so I'm fine with that proposal.

When we squash-merge PRs, it might be prohibitive to get bodies down to size (I generally rely on line-wrap with single line per paragraph), but this is maybe less of a concern.