commitcheck: lint commit messages

[benesch]

Whipped this up on the train yesterday. Won't be offended if we think this is a bad idea, but seems to work well on the several included test cases.

---

Gently remind developers of the proper commit message style after they
write a non-conforming commit. Developers who really hate the warning
can opt out by setting the COCKROACH_NO_LINT_COMMITS environment
variable.

commitcheck warns about the following:

• lines longer than 72 characters
• a missing blank line between the summary and description
• summaries without a scope: prefix
• summaries that do not separate scopes with a comma and without
  whitespace
• scopes not matching a package/folder name or an approved
  meta-scope, like mvcc or *
• summaries with a capital letter after the colon

[cockroach-teamcity]

This change is 

[tbg]

I'm 👎 unless I am convinced of the benefits of this change. There are disadvantages: eat more CI, have the package-hardcoding below rot, more burden on external contributors, just one more thing that can break in the merge path.

[benesch]

How would you feel about having the linter post a comment with the complaints to the PR instead of blocking the merge? The problem I'm trying to solve is that, as a new employee or external contributor, it's very hard to internalize all the commit message rules for your first PR. Then reviewers often let the bad commit messages slide because it's a pain to make people run through CI again. If we can warn people early, they can address the commit message fix while addressing the rest of their review feedback.

[dt]

Having to wait for CI to fail without a way to run the check locally, in make lint, is a bummer. I wonder if we could at least try to figure out the merge base using some sane heuristics so we could lint locally, pre-PR.

EDIT: e.g. default of origin/master configurable via git config, then the value in any given run overridable via env or flag (ie. when your merge base should be from release branch or whatever).

[a-robinson]

Are we being negatively affected by occasionally having commits without a package prefix? We do a pretty good job of it and the few exceptions have never bothered me, but maybe other people are using them for something that I'm not?

[tbg]

@benesch I'm more on board with a lintbot such as rust-highfive.

[benesch]

@a-robinson if you base email filters on the package prefix, commits with inconsistent/missing package prefixes are a real drag. (Though only inasmuch as the missing prefix propagates to the PR title.)

[tbg]

@benesch can't you base your filters on the below section? Sounds a little unreasonable to force developers to transcribe certain information from their diff into the commit title.

[benesch]

Sounds a little unreasonable to force developers to transcribe certain information from their diff into the commit title.

Er, isn't that exactly what our commit message style guide already does? In my mind, either the scope: bit is useful and we should enforce it, or the scope: bit is not useful and we shouldn't waste our time with it.

[benesch]

As I understand it, the reason to ask for an explicit scope: is because often you incidentally touch files (in, say, util), but the only meaningful changes are in server. It's pretty hard to determine that automatically.

To be honest I'm fairly neutral on whether these scopes are useful. I just want them to be consistent if we're going to have them.

[benesch]

Ok, PTAL! I've updated this to instead install a commit-msg githook which gently warns you when your commit message doesn't match our standards. It does not fail the commit.

[knz]

Please extend the commit message and the PR description at the top with a summary of the checks performed by the linter. It's not obvious to me by reading the code.

[knz]

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/cmd/commitcheck/main.go, line 130 at r1 (raw file):

}

// Everything after scissorsLine in a Git commit message is ignored.

Why? What is this string and where does it come from?

---

pkg/cmd/commitcheck/main.go, line 183 at r1 (raw file):

}

const noLintEnvVar = "COCKROACH_NO_LINT_COMMITS"

Document this in the commit message.

---

Comments from Reviewable

[benesch]

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/cmd/commitcheck/main.go, line 130 at r1 (raw file):

Previously, knz (kena) wrote…

Why? What is this string and where does it come from?

https://git-scm.com/docs/git-commit#git-commit-scissors

It's included in the message (and later stripped) if you run git commit -v.

---

Comments from Reviewable

[knz]

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/cmd/commitcheck/main.go, line 130 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

https://git-scm.com/docs/git-commit#git-commit-scissors

It's included in the message (and later stripped) if you run git commit -v.

Add this explanation in a comment.

---

Comments from Reviewable

[benesch]

Done.

---

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/cmd/commitcheck/main.go, line 130 at r1 (raw file):

Previously, knz (kena) wrote…

Add this explanation in a comment.

Done.

---

pkg/cmd/commitcheck/main.go, line 183 at r1 (raw file):

Previously, knz (kena) wrote…

Document this in the commit message.

Done.

---

Comments from Reviewable

[danhhz]

Does this githook prevent me from doing git commit -m "WIP" to save and push my progress at the end of the day?

I'm generally 👎 on using githooks for anything unless it's absolutely necessary. They tend to be too prescriptive of people's pre-PR workflows.

[benesch]

No, it never fails your commit; it just prints a warning when you have a non-compliant commit message. You can even turn the warning off via an env var if it really bugs you. Anyway, Jordan is cooking up a more targeted set of githooks that only check for "Release note."

…

On Mon, Dec 4, 2017 at 11:27 AM, Daniel Harrison ***@***.***> wrote: Does this githook prevent me from doing git commit -m "WIP" to save and push my progress at the end of the day? I'm generally 👎 on using githooks for anything unless it's absolutely necessary. They tend to be too prescriptive of people's pre-PR workflows. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18782 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA15IDwtTOvdLsXXVvl84idVooPyZKHwks5s9B1_gaJpZM4PkYzp> .

[benesch]

This rotted, and @knz wrote a nice linter for release notes. Closing. ¯_(ツ)_/¯