Skip to content

Commit d6f89f8

Browse files
committed
commitcheck: lint commit messages in git hook
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
1 parent d905960 commit d6f89f8

File tree

6 files changed

+358
-4
lines changed

6 files changed

+358
-4
lines changed

CONTRIBUTING.md

+2-2
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,8 @@ See our separate [style guide](STYLE.md) document.
143143
Commits which affect many packages as a result of a shared dependency change
144144
should probably begin their subjects with the name of the shared dependency.
145145
Finally, some commits may need to affect many packages in a way which does
146-
not point to a specific package; those commits may begin with "*:" or "all:"
147-
to indicate their reach.
146+
not point to a specific package; those commits may begin with "*:" to indicate
147+
their reach.
148148

149149
- Run the linters, code generators, and unit test suites locally:
150150

build/common.mk

+4-2
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,8 @@ $(YARN_INSTALLED_TARGET): $(BOOTSTRAP_TARGET) $(UI_ROOT)/package.json $(UI_ROOT)
152152
rm -rf $(UI_ROOT)/node_modules/@types/node
153153
touch $@
154154

155+
LOCAL_INSTALLABLES := $(addprefix $(PKG_ROOT)/cmd/,commitcheck metacheck returncheck)
156+
155157
# We store the bootstrap marker file in the bin directory so that remapping bin,
156158
# like we do in the builder container to allow for different host and guest
157159
# systems, will trigger bootstrapping in the container as necessary. This is
@@ -160,11 +162,11 @@ BOOTSTRAP_TARGET := $(LOCAL_BIN)/.bootstrap
160162

161163
# Update the git hooks and install commands from dependencies whenever they
162164
# change.
163-
$(BOOTSTRAP_TARGET): $(GITHOOKS) $(REPO_ROOT)/Gopkg.lock
165+
$(BOOTSTRAP_TARGET): $(GITHOOKS) $(REPO_ROOT)/Gopkg.lock $(shell find $(LOCAL_INSTALLABLES))
164166
ifneq ($(GIT_DIR),)
165167
git submodule update --init
166168
endif
167-
@$(GO_INSTALL) -v $(PKG_ROOT)/cmd/{metacheck,returncheck} \
169+
@$(GO_INSTALL) -v $(LOCAL_INSTALLABLES) \
168170
$(REPO_ROOT)/vendor/github.com/golang/dep/cmd/dep \
169171
$(REPO_ROOT)/vendor/github.com/client9/misspell/cmd/misspell \
170172
$(REPO_ROOT)/vendor/github.com/cockroachdb/crlfmt \

build/variables.mk

+1
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ define VALID_VARS
4949
LIBROACH_SRC_DIR
5050
LINKFLAGS
5151
LOCAL_BIN
52+
LOCAL_INSTALLABLES
5253
MACOS
5354
MACOSX_DEPLOYMENT_TARGET
5455
MAKEFLAGS

githooks/commit-msg

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
#!/usr/bin/env bash
2+
3+
exec bin/commitcheck "$@"

pkg/cmd/commitcheck/main.go

+227
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,227 @@
1+
// Copyright 2017 The Cockroach Authors.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
12+
// implied. See the License for the specific language governing
13+
// permissions and limitations under the License.
14+
package main
15+
16+
import (
17+
"bytes"
18+
"fmt"
19+
"go/build"
20+
"io/ioutil"
21+
"log"
22+
"os"
23+
"os/exec"
24+
"path/filepath"
25+
"strings"
26+
"unicode"
27+
"unicode/utf8"
28+
29+
"github.com/ghemawat/stream"
30+
"github.com/pkg/errors"
31+
)
32+
33+
// validScopes is a set of all valid scopes for a commit message of the form
34+
// scope[,scope]: message. It is populated by init with every directory in crdb.
35+
var validScopes = map[string]struct{}{
36+
// "*" is special and indicates "all scopes."
37+
"*": {},
38+
39+
// Following are pseudo-scopes that don't map neatly to a directory.
40+
"mvcc": {},
41+
}
42+
43+
// impliedScopePrefixes lists scope prefixes that may be omitted from the
44+
// commit message. For example, including "pkg/sql/" as an implied scope prefix
45+
// allows "pkg/sql/distsqlrun" to be shortened to "distsqlrun".
46+
var impliedScopePrefixes = []string{
47+
"build/",
48+
"c-deps/",
49+
"docs/",
50+
"pkg/",
51+
"pkg/ccl/",
52+
"pkg/cmd/",
53+
"pkg/internal/",
54+
"pkg/sql/",
55+
"pkg/storage/",
56+
"pkg/testutils/",
57+
"pkg/util/",
58+
}
59+
60+
// allowFileScope lists directories in which files and their basenames can be
61+
// used directly as scopes. For example, if the "scripts" directory allows file
62+
// scope and contains a file named "azworker.sh", both "azworker" and
63+
// "azworker.sh" become valid scopes.
64+
var allowFileScope = []string{
65+
".", ".github", "scripts",
66+
}
67+
68+
func fileScopeAllowed(dir string) bool {
69+
for _, d := range allowFileScope {
70+
if d == dir {
71+
return true
72+
}
73+
}
74+
return false
75+
}
76+
77+
func exit(args ...interface{}) {
78+
fmt.Fprint(os.Stderr, args...)
79+
os.Exit(1)
80+
}
81+
82+
func exitf(format string, args ...interface{}) {
83+
fmt.Fprintf(os.Stderr, format, args...)
84+
os.Exit(1)
85+
}
86+
87+
func init() {
88+
rootPkg, err := build.Import("github.com/cockroachdb/cockroach", "", build.FindOnly)
89+
if err != nil {
90+
exit(err)
91+
}
92+
93+
lsFiles := exec.Command("git", "ls-files")
94+
lsFiles.Dir = rootPkg.Dir
95+
stdout, err := lsFiles.StdoutPipe()
96+
if err != nil {
97+
exit(err)
98+
}
99+
stderr := new(bytes.Buffer)
100+
lsFiles.Stderr = stderr
101+
filter := stream.ReadLines(stdout)
102+
103+
if err := lsFiles.Start(); err != nil {
104+
log.Fatal(err)
105+
}
106+
107+
if err := stream.ForEach(filter, func(s string) {
108+
dir := filepath.Dir(s)
109+
if dir != "." {
110+
for _, prefix := range impliedScopePrefixes {
111+
validScopes[strings.TrimPrefix(dir, prefix)] = struct{}{}
112+
}
113+
}
114+
if fileScopeAllowed(dir) {
115+
base := filepath.Base(s)
116+
validScopes[base] = struct{}{}
117+
if baseNoExt := strings.TrimSuffix(base, filepath.Ext(base)); baseNoExt != "" {
118+
validScopes[baseNoExt] = struct{}{}
119+
}
120+
}
121+
}); err != nil {
122+
exit(err)
123+
}
124+
125+
if err := lsFiles.Wait(); err != nil {
126+
exitf("%s failed: %s\n%s", strings.Join(lsFiles.Args, " "), err, stderr.String())
127+
}
128+
}
129+
130+
// Everything after scissorsLine in a Git commit message is ignored. This
131+
// behavior is built into Git itself [0]; when using `git commit --verbose`,
132+
// for example, the commit message template includes a scissorsLine.
133+
//
134+
// [0]: https://git-scm.com/docs/git-commit#git-commit-scissors
135+
const scissorsLine = "# ------------------------ >8 ------------------------"
136+
137+
func checkCommitMessage(message string) error {
138+
if strings.HasPrefix(message, `Revert "`) {
139+
return nil
140+
}
141+
142+
lines := strings.Split(message, "\n")
143+
for i, line := range lines {
144+
if line == scissorsLine {
145+
break
146+
} else if max := 72; len(line) > max && line[0] != '#' {
147+
return errors.Errorf("line %d exceeds maximum line length of %d characters", i+1, max)
148+
}
149+
}
150+
if len(lines) > 1 && lines[1] != "" {
151+
return errors.New("missing blank line after summary")
152+
}
153+
summary := lines[0]
154+
155+
if summary == "" {
156+
return errors.New("missing summary")
157+
}
158+
if summary[len(summary)-1] == '.' {
159+
return errors.New("summary should not end in period")
160+
}
161+
162+
splits := strings.SplitN(summary, ":", 2)
163+
if len(splits) != 2 {
164+
return errors.New("summary does not match format `scope[,scope]: message`")
165+
}
166+
scopes, description := strings.Split(splits[0], ","), strings.TrimSpace(splits[1])
167+
168+
for _, scope := range scopes {
169+
if trimmedScope := strings.TrimSpace(scope); trimmedScope != scope {
170+
return errors.Errorf("scope %q has extraneous whitespace", trimmedScope)
171+
}
172+
if _, ok := validScopes[scope]; !ok {
173+
return errors.Errorf("unknown scope %q", scope)
174+
}
175+
}
176+
177+
if description == "" {
178+
return errors.New("summary missing text after colon")
179+
}
180+
if ch, _ := utf8.DecodeRuneInString(description); unicode.IsUpper(ch) {
181+
return errors.New("text after colon in summary should not begin with capital letter")
182+
}
183+
184+
return nil
185+
}
186+
187+
const noLintEnvVar = "COCKROACH_NO_LINT_COMMITS"
188+
189+
func main() {
190+
if len(os.Args) != 2 {
191+
exitf("usage: %s COMMIT-MSG-FILE\n", os.Args[0])
192+
}
193+
194+
if _, noLint := os.LookupEnv(noLintEnvVar); noLint {
195+
os.Exit(0)
196+
}
197+
198+
message, err := ioutil.ReadFile(os.Args[1])
199+
if err != nil {
200+
exitf("failed to read commit message file: %s\n", err)
201+
}
202+
203+
if err := checkCommitMessage(string(message)); err != nil {
204+
fmt.Printf(`commit message warning: %s
205+
206+
A good commit message has the following form:
207+
208+
scope[,scope]: short summary in imperative tense
209+
210+
A detailed description follows the summary here, after a blank line, and
211+
explains the rationale for the change. It should be wrapped at 72
212+
characters, with blank lines between paragraphs.
213+
214+
The scope at the beginning of the summary indicates the package (e.g.,
215+
"storage") primarily impacted by the change. If multiple packages are
216+
affected, separate them with commas and no whitespace. Commits which affect
217+
no package in particular can use the special scope "*". Note that certain
218+
obvious prefixes ("pkg/", "pkg/sql/", "scripts/", et al.) can be omitted
219+
from the scope.
220+
221+
See CONTRIBUTING.md for details.
222+
223+
If you believe this warning is in error, please update this linter. If you wish
224+
to suppress this linter, set %s=1 in your environment.`, err, noLintEnvVar)
225+
fmt.Println()
226+
}
227+
}

pkg/cmd/commitcheck/main_test.go

+121
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
// Copyright 2017 The Cockroach Authors.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
12+
// implied. See the License for the specific language governing
13+
// permissions and limitations under the License.
14+
15+
package main
16+
17+
import (
18+
"fmt"
19+
"testing"
20+
21+
"github.com/cockroachdb/cockroach/pkg/testutils"
22+
)
23+
24+
func TestCommitMessages(t *testing.T) {
25+
testCases := []struct{ message, err string }{
26+
{"server,base,storage: frobulate", ""},
27+
{"github-pull-request-make: paginate results\n\nfoo", ""},
28+
{"distsqlrun: properly account for top-k sorting memory", ""},
29+
{"sql/distsqlrun: do not call Finish on a nil span", ""},
30+
{"base,engine: don't accept StoreSpec.SizeInBytes for temp engines", ""},
31+
{"cache: cacheStore.del(key interface{}) -> cacheStore.del(e *Entry)", ""},
32+
{"storage: bring comments on TestLeaseExtensionNotBlockedByRead up to date", ""},
33+
{".gitattributes: mark help_messages.go to avoid diffs", ""},
34+
{"RFCS: fix decomissioning RFC's markdown rendering", ""},
35+
{"CODEOWNERS: avoid @cockroachdb/sql ownership", ""},
36+
{"azworker: persistent SSH sessions", ""},
37+
{".gitignore: remove stale entries", ""},
38+
{"*: force zone.o to link on macOS", ""},
39+
{"vendor: bump etcd/raft", ""},
40+
{`Revert "scripts: point pprof at binary"`, ""},
41+
42+
{
43+
"",
44+
"missing summary",
45+
},
46+
{
47+
"\n\n",
48+
"missing summary",
49+
},
50+
{
51+
"do some stuff",
52+
"summary does not match format `scope\\[,scope\\]: message`",
53+
},
54+
{
55+
"storage, server: don't reuse leases obtained before a restart",
56+
`scope "server" has extraneous whitespace`,
57+
},
58+
{
59+
"server:",
60+
"missing text after colon",
61+
},
62+
{
63+
"storage: ",
64+
`missing text after colon`,
65+
},
66+
{
67+
"build: Update instructions for building a deployable docker image",
68+
"text after colon in summary should not begin with capital letter",
69+
},
70+
{
71+
"distsql: increase intermediate precision of decimal calculations in square difference calculations",
72+
"line 1 exceeds maximum line length of 72 characters",
73+
},
74+
{
75+
"distsql: summary is fine\nbut blank line is missing",
76+
"missing blank line after summary",
77+
},
78+
{
79+
"distsql: summary is fine\n\nbut the extended description is really much too long whoever wrote this commit message needs to add a line break",
80+
"line 3 exceeds maximum line length of 72 characters",
81+
},
82+
{
83+
": foo",
84+
`unknown scope ""`,
85+
},
86+
{
87+
"server,: foo",
88+
`unknown scope ""`,
89+
},
90+
{
91+
"lint: forbid golang.org/x/sync/singleflight import",
92+
`unknown scope "lint"`,
93+
},
94+
{
95+
"sql+cli: fix UUID column dumping",
96+
`unknown scope "sql\+cli"`,
97+
},
98+
{
99+
"all: require go 1.9",
100+
`unknown scope "all"`,
101+
},
102+
{
103+
"sql/parser: produce context-sensitive help during error recovery.",
104+
"summary should not end in period",
105+
},
106+
}
107+
108+
for _, testCase := range testCases {
109+
err := checkCommitMessage(testCase.message)
110+
if !testutils.IsError(err, testCase.err) {
111+
errDisplay := "<nil>"
112+
if err != nil {
113+
errDisplay = fmt.Sprintf("%q", err)
114+
}
115+
t.Errorf(
116+
"%q:\nexpected error matching %q, but got %s",
117+
testCase.message, testCase.err, errDisplay,
118+
)
119+
}
120+
}
121+
}

0 commit comments

Comments
 (0)