Support commitfooter formatting
We want to enforce the existence of certain commit message footers.
We do this by introducing a formatter called
fmt:commitfooter-FOOTER-NAME.DISAMBIGUATION-SUFFIX
Since commit message footers commonly have dashes (Signed-off-by,
Change-Id), we can't use '-' as separator between the
DISAMBIGUATION-SUFFIX and the language.
This is solved by changing the separator to '.'. This will invalidate
existing formatter configs.
Change-Id: Ibd6d878d3944746c723fc4d1f8699fe4fbc862f1
diff --git a/cmd/checker/checker.go b/cmd/checker/checker.go
index 6473174..418e4dd 100644
--- a/cmd/checker/checker.go
+++ b/cmd/checker/checker.go
@@ -73,14 +73,18 @@
hash := sha1.New()
hash.Write([]byte(repo))
- uuid := fmt.Sprintf("%s:%s-%x", checkerScheme, language, hash.Sum(nil))
+ uuid := fmt.Sprintf("%s:%s.%x", checkerScheme, language, hash.Sum(nil))
+ cfg, ok := linter.GetFormatter(language)
+ if !ok {
+ return nil, fmt.Errorf("no checker for language %q", language)
+ }
in := gerrit.CheckerInput{
UUID: uuid,
Name: language + " formatting",
Repository: repo,
Description: "check source code formatting.",
Status: "ENABLED",
- Query: linter.Formatters[language].Query,
+ Query: cfg.Query,
}
body, err := json.Marshal(&in)
@@ -108,7 +112,7 @@
// checkerLanguage extracts the language to check for from a checker UUID.
func checkerLanguage(uuid string) (string, bool) {
uuid = strings.TrimPrefix(uuid, checkerScheme+":")
- fields := strings.Split(uuid, "-")
+ fields := strings.Split(uuid, ".")
if len(fields) != 2 {
return "", false
}
@@ -131,8 +135,9 @@
// errIrrelevant is a marker error value used for checks that don't apply for a change.
var errIrrelevant = errors.New("irrelevant")
-// checkChange checks a (change, patchset) for correct formatting in the given language. It returns
-// a list of complaints, or the errIrrelevant error if there is nothing to do.
+// checkChange checks a (change, patchset) for correct formatting in
+// the given language. It returns a list of complaints, or the
+// errIrrelevant error if there is nothing to do.
func (c *gerritChecker) checkChange(changeID string, psID int, language string) ([]string, error) {
ch, err := c.server.GetChange(changeID, strconv.Itoa(psID))
if err != nil {
@@ -140,8 +145,8 @@
}
req := linter.FormatRequest{}
for n, f := range ch.Files {
- cfg := linter.Formatters[language]
- if cfg == nil {
+ cfg, ok := linter.GetFormatter(language)
+ if !ok {
return nil, fmt.Errorf("language %q not configured", language)
}
if !cfg.Regex.MatchString(n) {
@@ -191,13 +196,13 @@
func (c *gerritChecker) Serve() {
for {
- // TODO: real rate limiting.
wait, err := c.processPendingChecks()
if err != nil {
log.Printf("checkAllChecks: %v", err)
}
if wait {
- time.Sleep(10 * time.Second)
+ // TODO: real rate limiting?
+ time.Sleep(c.delay)
}
}
}
diff --git a/cmd/checker/checker_test.go b/cmd/checker/checker_test.go
index 9d2614e..511643f 100644
--- a/cmd/checker/checker_test.go
+++ b/cmd/checker/checker_test.go
@@ -25,6 +25,16 @@
"github.com/google/gerrit-linter/gerrit"
)
+func TestSchemeLanguage(t *testing.T) {
+ lang, ok := checkerLanguage("fmt:commitfooter-Change-Id.1234")
+ if !ok {
+ t.Fatalf("checkerLanguage failed")
+ }
+ if want := "commitfooter-Change-Id"; lang != want {
+ t.Errorf("got %q, want %q", lang, want)
+ }
+}
+
func urlParse(s string) url.URL {
u, err := url.Parse(s)
if err != nil {
@@ -34,12 +44,34 @@
return *u
}
-type changeInfo struct {
+type ChangeInfo struct {
ChangeId string `json:"change_id"`
Number int `json:"_number"`
}
-func TestGerrit(t *testing.T) {
+func createUpdateChecker(t *testing.T, gc *gerritChecker, formatter string) *gerrit.CheckerInfo {
+ checker, err := gc.PostChecker("gerrit-linter-test", "commitmsg", true)
+ if err != nil {
+ // create
+ checker, err = gc.PostChecker("gerrit-linter-test", "commitmsg", false)
+ if err != nil {
+ t.Fatalf("create PostChecker: %v", err)
+ }
+ }
+ return checker
+}
+
+type ChangeInput struct {
+ Project string `json:"project"`
+ Subject string `json:"subject"`
+ Branch string `json:"branch"`
+}
+
+type EditMessageInput struct {
+ Message string `json:"message"`
+}
+
+func TestBasic(t *testing.T) {
g := gerrit.New(urlParse("http://localhost:8080/"))
g.Authenticator = gerrit.NewBasicAuth("admin:secret")
g.Debug = true
@@ -54,29 +86,29 @@
if _, err := g.GetPath("/projects/gerrit-linter-test/"); err != nil {
t.Fatalf("GetPath: %v", err)
}
- msgChecker, err := gc.PostChecker("gerrit-linter-test", "commitmsg", true)
- if err != nil {
- // create
- msgChecker, err = gc.PostChecker("gerrit-linter-test", "commitmsg", false)
- if err != nil {
- t.Fatalf("create PostChecker: %v", err)
- }
+
+ msgChecker := createUpdateChecker(t, gc, "commitmsg")
+ footerLang := "commitfooter-User-Visible"
+ footerChecker := createUpdateChecker(t, gc, footerLang)
+
+ changeInput := ChangeInput{
+ Project: "gerrit-linter-test",
+ Subject: "my linter test change.",
+ Branch: "master",
}
- content, err := g.PostPath("a/changes/",
+ var change ChangeInfo
+ if err := g.PostPathJSON("a/changes/",
"application/json",
- []byte(`{
- "project": "gerrit-linter-test",
- "subject": "my linter test change.",
- "branch": "master"}
-`))
- if err != nil {
- t.Fatal(err)
- }
- var change changeInfo
- if err := gerrit.Unmarshal(content, &change); err != nil {
+ &changeInput, &change); err != nil {
t.Fatal(err)
}
log.Printf("created change %d", change.Number)
+ defer func() {
+ if err := g.PostPathJSON(fmt.Sprintf("a/changes/%d/abandon", change.Number),
+ "application/json", &EditMessageInput{Message: "test succeeded"}, &struct{}{}); err != nil {
+ log.Printf("abandon: %v", err)
+ }
+ }()
gc.processPendingChecks()
@@ -89,23 +121,23 @@
t.Fatalf("got %q, want %q", info.State, statusFail)
}
- if _, err := g.PutPath(fmt.Sprintf("a/changes/%d/message", change.Number), "application/json", []byte(
- fmt.Sprintf(`{"message": "New Commit message\n\nChange-Id: %s\n"}`, change.ChangeId))); err != nil {
+ ignored := ""
+ if err := g.PutPathJSON(fmt.Sprintf("a/changes/%d/message", change.Number), "application/json",
+ &EditMessageInput{Message: fmt.Sprintf("New Commit message\n\nChange-Id: %s\n", change.ChangeId)},
+ &ignored); err != nil {
t.Fatalf("edit message: %v", err)
}
gc.processPendingChecks()
- info, err = g.GetCheck(strconv.Itoa(change.Number), 2, msgChecker.UUID)
- if err != nil {
+ if info, err = g.GetCheck(strconv.Itoa(change.Number), 2, msgChecker.UUID); err != nil {
t.Fatalf("2nd GetCheck: %v", err)
- }
-
- if info.State != statusSuccessful.String() {
+ } else if info.State != statusSuccessful.String() {
t.Fatalf("got %q, want %q", info.State, statusSuccessful)
}
- if _, err := g.PostPath(fmt.Sprintf("a/changes/%d/abandon", change.Number),
- "application/json", []byte(`{"message": "test succeeded"}`)); err != nil {
- t.Fatalf("abandon: %v", err)
+ if info, err = g.GetCheck(strconv.Itoa(change.Number), 2, footerChecker.UUID); err != nil {
+ t.Fatalf("2nd GetCheck: %v", err)
+ } else if info.State != statusSuccessful.String() {
+ t.Fatalf("got %q, want %q", info.State, statusSuccessful)
}
}
diff --git a/gerrit/server.go b/gerrit/server.go
index c7e1c53..359b7fc 100644
--- a/gerrit/server.go
+++ b/gerrit/server.go
@@ -20,6 +20,7 @@
"encoding/json"
"fmt"
"io/ioutil"
+ "log"
"net/http"
"net/url"
"path"
@@ -89,6 +90,14 @@
return g.Get(&u)
}
+func (g *Server) GetPathJSON(p string, data interface{}) error {
+ content, err := g.GetPath(p)
+ if err != nil {
+ return err
+ }
+ return Unmarshal(content, data)
+}
+
// Do runs a HTTP request against the remote server.
func (g *Server) Do(req *http.Request) (*http.Response, error) {
req.Header.Set("User-Agent", g.UserAgent)
@@ -136,6 +145,32 @@
return g.putPostPath("POST", path, contentType, content)
}
+// PostPathJSON does a JSON based REST RPC.
+func (g *Server) PostPathJSON(path, contentType string, input, output interface{}) error {
+ return g.putPostPathJSON("POST", path, contentType, input, output)
+}
+
+// PutPathJSON does a JSON based REST RPC.
+func (g *Server) PutPathJSON(path, contentType string, input, output interface{}) error {
+ return g.putPostPathJSON("PUT", path, contentType, input, output)
+}
+
+// putPostPathJSON does a JSON based REST RPC.
+func (g *Server) putPostPathJSON(method, path, contentType string, input, output interface{}) error {
+ content, err := json.Marshal(input)
+ if err != nil {
+ return err
+ }
+
+ resp, err := g.putPostPath(method, path, contentType, content)
+ if err != nil {
+ log.Println(err, string(content))
+ return err
+ }
+
+ return Unmarshal(resp, output)
+}
+
func (g *Server) putPostPath(method string, pth string, contentType string, content []byte) ([]byte, error) {
u := g.URL
u.Path = path.Join(u.Path, pth)
@@ -182,15 +217,10 @@
// GetChange returns the Change (including file contents) for a given change.
func (g *Server) GetChange(changeID string, revID string) (*Change, error) {
- content, err := g.GetPath(fmt.Sprintf("changes/%s/revisions/%s/files/",
- url.PathEscape(changeID), revID))
- if err != nil {
- return nil, err
- }
- content = bytes.TrimPrefix(content, jsonPrefix)
-
files := map[string]*File{}
- if err := json.Unmarshal(content, &files); err != nil {
+ err := g.GetPathJSON(fmt.Sprintf("changes/%s/revisions/%s/files/",
+ url.PathEscape(changeID), revID), &files)
+ if err != nil {
return nil, err
}
@@ -254,37 +284,21 @@
// PostCheck posts a single check result onto a change.
func (s *Server) PostCheck(changeID string, psID int, input *CheckInput) (*CheckInfo, error) {
- body, err := json.Marshal(input)
+ var output CheckInfo
+ err := s.PostPathJSON(fmt.Sprintf("a/changes/%s/revisions/%d/checks/", changeID, psID),
+ "application/json", input, &output)
if err != nil {
return nil, err
}
- res, err := s.PostPath(fmt.Sprintf("a/changes/%s/revisions/%d/checks/", changeID, psID),
- "application/json", body)
- if err != nil {
- return nil, err
- }
-
- var out CheckInfo
- if err := Unmarshal(res, &out); err != nil {
- return nil, err
- }
-
- return &out, nil
+ return &output, nil
}
+// GetCheck returns info for a single check.
func (s *Server) GetCheck(changeID string, psID int, uuid string) (*CheckInfo, error) {
- u := s.URL
- u.Path = path.Join(u.Path, fmt.Sprintf("changes/%s/revisions/%d/checks/%s", changeID, psID, uuid))
- content, err := s.Get(&u)
- if err != nil {
- return nil, err
- }
-
var out CheckInfo
- if err := Unmarshal(content, &out); err != nil {
- return nil, err
- }
+ err := s.GetPathJSON(fmt.Sprintf("changes/%s/revisions/%d/checks/%s", changeID, psID, uuid),
+ &out)
- return &out, nil
+ return &out, err
}
diff --git a/server.go b/server.go
index f73678f..30f4089 100644
--- a/server.go
+++ b/server.go
@@ -47,8 +47,8 @@
Formatter Formatter
}
-// Formatters holds all the formatters supported
-var Formatters = map[string]*FormatterConfig{
+// formatters holds all the formatters supported
+var formatters = map[string]*FormatterConfig{
"commitmsg": {
Regex: regexp.MustCompile(`^/COMMIT_MSG$`),
Formatter: &commitMsgFormatter{},
@@ -63,7 +63,7 @@
gjf, err := exec.LookPath("google-java-format.jar")
if err == nil {
- Formatters["java"] = &FormatterConfig{
+ formatters["java"] = &FormatterConfig{
Regex: regexp.MustCompile(`\.java$`),
Query: "ext:java",
Formatter: &toolFormatter{
@@ -77,7 +77,7 @@
bzl, err := exec.LookPath("buildifier")
if err == nil {
- Formatters["bzl"] = &FormatterConfig{
+ formatters["bzl"] = &FormatterConfig{
Regex: regexp.MustCompile(`(\.bzl|/BUILD|^BUILD)$`),
Query: "(ext:bzl OR file:BUILD OR file:WORKSPACE)",
Formatter: &toolFormatter{
@@ -91,7 +91,7 @@
gofmt, err := exec.LookPath("gofmt")
if err == nil {
- Formatters["go"] = &FormatterConfig{
+ formatters["go"] = &FormatterConfig{
Regex: regexp.MustCompile(`\.go$`),
Query: "ext:go",
Formatter: &toolFormatter{
@@ -104,16 +104,31 @@
}
}
+func GetFormatter(lang string) (*FormatterConfig, bool) {
+ footerPrefix := "commitfooter-"
+ if strings.HasPrefix(lang, footerPrefix) {
+ return &FormatterConfig{
+ Regex: regexp.MustCompile(`^/COMMIT_MSG$`),
+ Formatter: &commitFooterFormatter{
+ Footer: lang[len(footerPrefix):],
+ },
+ }, true
+ }
+
+ cfg, ok := formatters[lang]
+ return cfg, ok
+}
+
// IsSupported returns if the given language is supported.
func IsSupported(lang string) bool {
- _, ok := Formatters[lang]
+ _, ok := GetFormatter(lang)
return ok
}
// SupportedLanguages returns a list of languages.
func SupportedLanguages() []string {
var r []string
- for l := range Formatters {
+ for l := range formatters {
r = append(r, l)
}
sort.Strings(r)
@@ -138,8 +153,10 @@
for language, fs := range splitByLang(req.Files) {
var buf bytes.Buffer
- entry := Formatters[language]
-
+ entry, ok := GetFormatter(language)
+ if !ok {
+ return fmt.Errorf("linter: no formatter for %q", language)
+ }
out, err := entry.Formatter.Format(fs, &buf)
if err != nil {
return err
@@ -189,6 +206,57 @@
return ""
}
+type commitFooterFormatter struct {
+ Footer string
+}
+
+func (f *commitFooterFormatter) Format(in []File, outSink io.Writer) (out []FormattedFile, err error) {
+ complaint := checkCommitFooter(string(in[0].Content), f.Footer)
+ ff := FormattedFile{}
+ ff.Name = in[0].Name
+ if complaint != "" {
+ ff.Message = complaint
+ } else {
+ ff.Content = in[0].Content
+ }
+ out = append(out, ff)
+ return out, nil
+}
+
+func checkCommitFooter(message, footer string) string {
+ if len(footer) == 0 {
+ return "required footer should be non-empty"
+ }
+
+ blocks := strings.Split(message, "\n\n")
+ if len(blocks) < 2 {
+ return "gerrit changes must have two paragraphs."
+ }
+
+ footerBlock := blocks[len(blocks)-1]
+ lines := strings.Split(footerBlock, "\n")
+ for _, l := range lines {
+ fields := strings.SplitN(l, ":", 2)
+ if len(fields) < 2 {
+ continue
+ }
+
+ if fields[0] != footer {
+ continue
+ }
+
+ value := fields[1]
+ if !strings.HasPrefix(value, " ") {
+ return fmt.Sprintf("footer %q should have space after ':'", fields[1])
+ }
+
+ // length limit?
+ return ""
+ }
+
+ return fmt.Sprintf("footer %q not found", footer)
+}
+
type toolFormatter struct {
bin string
args []string
diff --git a/server_test.go b/server_test.go
index 4ae99a2..08b83df 100644
--- a/server_test.go
+++ b/server_test.go
@@ -42,3 +42,28 @@
}
}
}
+
+func TestCommitFooter(t *testing.T) {
+ for in, want := range map[string]string{
+ `abc`: "two paragraphs",
+ `abc
+
+def
+`: "not found",
+ `abc.
+
+myfooter:abc`: "space after",
+ `abc
+
+Change-Id: Iabc123
+myfooter: value!`: "",
+ } {
+ got := checkCommitFooter(in, "myfooter")
+
+ if want == "" && got != "" {
+ t.Errorf("want empty, got %s", got)
+ } else if !strings.Contains(got, want) {
+ t.Errorf("got %s, want substring %s", got, want)
+ }
+ }
+}
