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) + } + } +}