From d6f89f8664312f184d0bed746a090e93caab3ae8 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Wed, 11 Oct 2017 18:49:36 -0400 Subject: [PATCH] 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 --- CONTRIBUTING.md | 4 +- build/common.mk | 6 +- build/variables.mk | 1 + githooks/commit-msg | 3 + pkg/cmd/commitcheck/main.go | 227 +++++++++++++++++++++++++++++++ pkg/cmd/commitcheck/main_test.go | 121 ++++++++++++++++ 6 files changed, 358 insertions(+), 4 deletions(-) create mode 100755 githooks/commit-msg create mode 100644 pkg/cmd/commitcheck/main.go create mode 100644 pkg/cmd/commitcheck/main_test.go diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index dbbe01ffd7a8..bc79c050ca99 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -143,8 +143,8 @@ See our separate [style guide](STYLE.md) document. Commits which affect many packages as a result of a shared dependency change should probably begin their subjects with the name of the shared dependency. Finally, some commits may need to affect many packages in a way which does - not point to a specific package; those commits may begin with "*:" or "all:" - to indicate their reach. + not point to a specific package; those commits may begin with "*:" to indicate + their reach. - Run the linters, code generators, and unit test suites locally: diff --git a/build/common.mk b/build/common.mk index 7c14f54d2e42..3ec32804da33 100644 --- a/build/common.mk +++ b/build/common.mk @@ -152,6 +152,8 @@ $(YARN_INSTALLED_TARGET): $(BOOTSTRAP_TARGET) $(UI_ROOT)/package.json $(UI_ROOT) rm -rf $(UI_ROOT)/node_modules/@types/node touch $@ +LOCAL_INSTALLABLES := $(addprefix $(PKG_ROOT)/cmd/,commitcheck metacheck returncheck) + # We store the bootstrap marker file in the bin directory so that remapping bin, # like we do in the builder container to allow for different host and guest # systems, will trigger bootstrapping in the container as necessary. This is @@ -160,11 +162,11 @@ BOOTSTRAP_TARGET := $(LOCAL_BIN)/.bootstrap # Update the git hooks and install commands from dependencies whenever they # change. -$(BOOTSTRAP_TARGET): $(GITHOOKS) $(REPO_ROOT)/Gopkg.lock +$(BOOTSTRAP_TARGET): $(GITHOOKS) $(REPO_ROOT)/Gopkg.lock $(shell find $(LOCAL_INSTALLABLES)) ifneq ($(GIT_DIR),) git submodule update --init endif - @$(GO_INSTALL) -v $(PKG_ROOT)/cmd/{metacheck,returncheck} \ + @$(GO_INSTALL) -v $(LOCAL_INSTALLABLES) \ $(REPO_ROOT)/vendor/github.com/golang/dep/cmd/dep \ $(REPO_ROOT)/vendor/github.com/client9/misspell/cmd/misspell \ $(REPO_ROOT)/vendor/github.com/cockroachdb/crlfmt \ diff --git a/build/variables.mk b/build/variables.mk index d25d49bcb4ee..88de64e7720e 100644 --- a/build/variables.mk +++ b/build/variables.mk @@ -49,6 +49,7 @@ define VALID_VARS LIBROACH_SRC_DIR LINKFLAGS LOCAL_BIN + LOCAL_INSTALLABLES MACOS MACOSX_DEPLOYMENT_TARGET MAKEFLAGS diff --git a/githooks/commit-msg b/githooks/commit-msg new file mode 100755 index 000000000000..41b19fed3430 --- /dev/null +++ b/githooks/commit-msg @@ -0,0 +1,3 @@ +#!/usr/bin/env bash + +exec bin/commitcheck "$@" diff --git a/pkg/cmd/commitcheck/main.go b/pkg/cmd/commitcheck/main.go new file mode 100644 index 000000000000..ffc4904e1dd7 --- /dev/null +++ b/pkg/cmd/commitcheck/main.go @@ -0,0 +1,227 @@ +// Copyright 2017 The Cockroach Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +// implied. See the License for the specific language governing +// permissions and limitations under the License. +package main + +import ( + "bytes" + "fmt" + "go/build" + "io/ioutil" + "log" + "os" + "os/exec" + "path/filepath" + "strings" + "unicode" + "unicode/utf8" + + "github.com/ghemawat/stream" + "github.com/pkg/errors" +) + +// validScopes is a set of all valid scopes for a commit message of the form +// scope[,scope]: message. It is populated by init with every directory in crdb. +var validScopes = map[string]struct{}{ + // "*" is special and indicates "all scopes." + "*": {}, + + // Following are pseudo-scopes that don't map neatly to a directory. + "mvcc": {}, +} + +// impliedScopePrefixes lists scope prefixes that may be omitted from the +// commit message. For example, including "pkg/sql/" as an implied scope prefix +// allows "pkg/sql/distsqlrun" to be shortened to "distsqlrun". +var impliedScopePrefixes = []string{ + "build/", + "c-deps/", + "docs/", + "pkg/", + "pkg/ccl/", + "pkg/cmd/", + "pkg/internal/", + "pkg/sql/", + "pkg/storage/", + "pkg/testutils/", + "pkg/util/", +} + +// allowFileScope lists directories in which files and their basenames can be +// used directly as scopes. For example, if the "scripts" directory allows file +// scope and contains a file named "azworker.sh", both "azworker" and +// "azworker.sh" become valid scopes. +var allowFileScope = []string{ + ".", ".github", "scripts", +} + +func fileScopeAllowed(dir string) bool { + for _, d := range allowFileScope { + if d == dir { + return true + } + } + return false +} + +func exit(args ...interface{}) { + fmt.Fprint(os.Stderr, args...) + os.Exit(1) +} + +func exitf(format string, args ...interface{}) { + fmt.Fprintf(os.Stderr, format, args...) + os.Exit(1) +} + +func init() { + rootPkg, err := build.Import("github.com/cockroachdb/cockroach", "", build.FindOnly) + if err != nil { + exit(err) + } + + lsFiles := exec.Command("git", "ls-files") + lsFiles.Dir = rootPkg.Dir + stdout, err := lsFiles.StdoutPipe() + if err != nil { + exit(err) + } + stderr := new(bytes.Buffer) + lsFiles.Stderr = stderr + filter := stream.ReadLines(stdout) + + if err := lsFiles.Start(); err != nil { + log.Fatal(err) + } + + if err := stream.ForEach(filter, func(s string) { + dir := filepath.Dir(s) + if dir != "." { + for _, prefix := range impliedScopePrefixes { + validScopes[strings.TrimPrefix(dir, prefix)] = struct{}{} + } + } + if fileScopeAllowed(dir) { + base := filepath.Base(s) + validScopes[base] = struct{}{} + if baseNoExt := strings.TrimSuffix(base, filepath.Ext(base)); baseNoExt != "" { + validScopes[baseNoExt] = struct{}{} + } + } + }); err != nil { + exit(err) + } + + if err := lsFiles.Wait(); err != nil { + exitf("%s failed: %s\n%s", strings.Join(lsFiles.Args, " "), err, stderr.String()) + } +} + +// Everything after scissorsLine in a Git commit message is ignored. This +// behavior is built into Git itself [0]; when using `git commit --verbose`, +// for example, the commit message template includes a scissorsLine. +// +// [0]: https://git-scm.com/docs/git-commit#git-commit-scissors +const scissorsLine = "# ------------------------ >8 ------------------------" + +func checkCommitMessage(message string) error { + if strings.HasPrefix(message, `Revert "`) { + return nil + } + + lines := strings.Split(message, "\n") + for i, line := range lines { + if line == scissorsLine { + break + } else if max := 72; len(line) > max && line[0] != '#' { + return errors.Errorf("line %d exceeds maximum line length of %d characters", i+1, max) + } + } + if len(lines) > 1 && lines[1] != "" { + return errors.New("missing blank line after summary") + } + summary := lines[0] + + if summary == "" { + return errors.New("missing summary") + } + if summary[len(summary)-1] == '.' { + return errors.New("summary should not end in period") + } + + splits := strings.SplitN(summary, ":", 2) + if len(splits) != 2 { + return errors.New("summary does not match format `scope[,scope]: message`") + } + scopes, description := strings.Split(splits[0], ","), strings.TrimSpace(splits[1]) + + for _, scope := range scopes { + if trimmedScope := strings.TrimSpace(scope); trimmedScope != scope { + return errors.Errorf("scope %q has extraneous whitespace", trimmedScope) + } + if _, ok := validScopes[scope]; !ok { + return errors.Errorf("unknown scope %q", scope) + } + } + + if description == "" { + return errors.New("summary missing text after colon") + } + if ch, _ := utf8.DecodeRuneInString(description); unicode.IsUpper(ch) { + return errors.New("text after colon in summary should not begin with capital letter") + } + + return nil +} + +const noLintEnvVar = "COCKROACH_NO_LINT_COMMITS" + +func main() { + if len(os.Args) != 2 { + exitf("usage: %s COMMIT-MSG-FILE\n", os.Args[0]) + } + + if _, noLint := os.LookupEnv(noLintEnvVar); noLint { + os.Exit(0) + } + + message, err := ioutil.ReadFile(os.Args[1]) + if err != nil { + exitf("failed to read commit message file: %s\n", err) + } + + if err := checkCommitMessage(string(message)); err != nil { + fmt.Printf(`commit message warning: %s + +A good commit message has the following form: + + scope[,scope]: short summary in imperative tense + + A detailed description follows the summary here, after a blank line, and + explains the rationale for the change. It should be wrapped at 72 + characters, with blank lines between paragraphs. + + The scope at the beginning of the summary indicates the package (e.g., + "storage") primarily impacted by the change. If multiple packages are + affected, separate them with commas and no whitespace. Commits which affect + no package in particular can use the special scope "*". Note that certain + obvious prefixes ("pkg/", "pkg/sql/", "scripts/", et al.) can be omitted + from the scope. + + See CONTRIBUTING.md for details. + +If you believe this warning is in error, please update this linter. If you wish +to suppress this linter, set %s=1 in your environment.`, err, noLintEnvVar) + fmt.Println() + } +} diff --git a/pkg/cmd/commitcheck/main_test.go b/pkg/cmd/commitcheck/main_test.go new file mode 100644 index 000000000000..a7ce73e0afff --- /dev/null +++ b/pkg/cmd/commitcheck/main_test.go @@ -0,0 +1,121 @@ +// Copyright 2017 The Cockroach Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +// implied. See the License for the specific language governing +// permissions and limitations under the License. + +package main + +import ( + "fmt" + "testing" + + "github.com/cockroachdb/cockroach/pkg/testutils" +) + +func TestCommitMessages(t *testing.T) { + testCases := []struct{ message, err string }{ + {"server,base,storage: frobulate", ""}, + {"github-pull-request-make: paginate results\n\nfoo", ""}, + {"distsqlrun: properly account for top-k sorting memory", ""}, + {"sql/distsqlrun: do not call Finish on a nil span", ""}, + {"base,engine: don't accept StoreSpec.SizeInBytes for temp engines", ""}, + {"cache: cacheStore.del(key interface{}) -> cacheStore.del(e *Entry)", ""}, + {"storage: bring comments on TestLeaseExtensionNotBlockedByRead up to date", ""}, + {".gitattributes: mark help_messages.go to avoid diffs", ""}, + {"RFCS: fix decomissioning RFC's markdown rendering", ""}, + {"CODEOWNERS: avoid @cockroachdb/sql ownership", ""}, + {"azworker: persistent SSH sessions", ""}, + {".gitignore: remove stale entries", ""}, + {"*: force zone.o to link on macOS", ""}, + {"vendor: bump etcd/raft", ""}, + {`Revert "scripts: point pprof at binary"`, ""}, + + { + "", + "missing summary", + }, + { + "\n\n", + "missing summary", + }, + { + "do some stuff", + "summary does not match format `scope\\[,scope\\]: message`", + }, + { + "storage, server: don't reuse leases obtained before a restart", + `scope "server" has extraneous whitespace`, + }, + { + "server:", + "missing text after colon", + }, + { + "storage: ", + `missing text after colon`, + }, + { + "build: Update instructions for building a deployable docker image", + "text after colon in summary should not begin with capital letter", + }, + { + "distsql: increase intermediate precision of decimal calculations in square difference calculations", + "line 1 exceeds maximum line length of 72 characters", + }, + { + "distsql: summary is fine\nbut blank line is missing", + "missing blank line after summary", + }, + { + "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", + "line 3 exceeds maximum line length of 72 characters", + }, + { + ": foo", + `unknown scope ""`, + }, + { + "server,: foo", + `unknown scope ""`, + }, + { + "lint: forbid golang.org/x/sync/singleflight import", + `unknown scope "lint"`, + }, + { + "sql+cli: fix UUID column dumping", + `unknown scope "sql\+cli"`, + }, + { + "all: require go 1.9", + `unknown scope "all"`, + }, + { + "sql/parser: produce context-sensitive help during error recovery.", + "summary should not end in period", + }, + } + + for _, testCase := range testCases { + err := checkCommitMessage(testCase.message) + if !testutils.IsError(err, testCase.err) { + errDisplay := "" + if err != nil { + errDisplay = fmt.Sprintf("%q", err) + } + t.Errorf( + "%q:\nexpected error matching %q, but got %s", + testCase.message, testCase.err, errDisplay, + ) + } + } +}