diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 0000000..5c6fe22 --- /dev/null +++ b/.editorconfig @@ -0,0 +1,44 @@ +# What an editor should do before anybody runs anything. This file is a request +# and not the rule: internal/format is what refuses a tracked file breaking one +# of the three properties below, and gofmt is what decides Go. +# +# The values are the tree as it already is rather than a preference imposed on +# it. Anything here that no check reads is a courtesy to an editor and nothing +# more. + +root = true + +[*] +charset = utf-8 +end_of_line = lf +insert_final_newline = true +trim_trailing_whitespace = true +indent_style = space +indent_size = 4 + +# gofmt indents with tabs and is the authority for Go formatting, so this +# follows it rather than arguing with it. +[*.go] +indent_style = tab +indent_size = 4 + +# The workflow files and the site are two-space by convention in their own +# ecosystems, and the tree already follows both. +[*.{yml,yaml}] +indent_size = 2 + +[*.{html,css,js}] +indent_size = 2 + +# The manifest's own indentation is four spaces and it is fixed by +# decisions/manifest-schema.md rather than by taste, so this agrees with the +# encoder rather than with a habit. +[*.json] +indent_size = 4 + +# Markdown is prose, so an indent size is meaningless in it. A trailing space is +# not: two of them are a hard line break, which is a real construct somebody may +# want, and the check refuses one anyway. Use a backslash or a blank line, and +# there is one place to argue with that rather than two. +[*.md] +indent_size = 4 diff --git a/.gitattributes b/.gitattributes index d062f75..efddd7d 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,5 +1,12 @@ +# What git stores, which is what every check reads and what every reviewer sees. +# +# Line endings are pinned here rather than left to core.autocrlf, so a file's +# stored bytes do not depend on the checkout setting of whoever committed it. # The generated manifest and the golden fixture it is compared against are -# judged byte for byte, so their line endings cannot depend on the checkout -# setting of whoever ran the test. Pinned here rather than left to core.autocrlf. -# #23 is where this widens to the rest of the tree. -*.json text eol=lf +# judged byte for byte and would be the first casualty, but the reason is not +# theirs alone: a rule that only holds for one file type holds by luck. +# +# A working copy may still have CRLF, and that is fine. internal/format +# normalises before judging, so the formatting leg gives the same verdict on +# either checkout. +* text=auto eol=lf diff --git a/.github/workflows/gate.yml b/.github/workflows/gate.yml index 15902e8..fb2e732 100644 --- a/.github/workflows/gate.yml +++ b/.github/workflows/gate.yml @@ -1,4 +1,5 @@ -# The merge gate: build, test, format. +# The merge gate. The jobs below are the whole of it, and `go run .` names the +# same legs from a shell, so neither list is written out in prose that drifts. # # One job per leg, and each job runs the same entry point a contributor runs # before pushing. A job is not a place where a decision is taken here; every @@ -95,6 +96,19 @@ jobs: go-version-file: go.mod - run: go run . gate format + editorconfig: + name: "Gate: editorconfig" + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: go.mod + - run: go run . gate editorconfig + tests-reach-nothing: name: "Gate: tests-reach-nothing" runs-on: ubuntu-latest diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8868a6f..4c91d33 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,6 +15,36 @@ ones that arrive with it. The site under `docs/` still opens in a browser with no build step. +### Formatting + + gofmt -w . + +That is the whole format command, and on a tree that is already formatted it +changes nothing. gofmt decides Go. Everything else is decided by three +properties `.editorconfig` states and `internal/format` refuses: a final +newline, no trailing whitespace, and no tab indent outside Go. + + go test ./internal/format -count=1 + +is what refuses them. The gate runs the two halves as two legs, `format` and +`editorconfig`, so a red says which of them it was without anybody opening a +log. There is no formatter for the HTML, the YAML or the prose, because one +would be a runtime this tree does not carry, and `.editorconfig` is what an +editor reads instead. + +Line endings do not decide the verdict, and the two halves get there +differently. `internal/format` normalises before judging, so it answers the same +on either checkout, and a test holds that property rather than a sentence +claiming it. gofmt does not normalise: it lists a Go file whose working copy has +CRLF, on any platform. What keeps that from being a Windows-only red is +`.gitattributes`, which pins every tracked file to LF in the working copy as +well as in the object store, so a fresh clone is LF whatever `core.autocrlf` +says locally. + +If gofmt lists a file you have not touched, that is the symptom: the working +copy predates `.gitattributes`. Deleting the file and checking it out again +fixes it, and nothing about the file's content was wrong. + ### The dependency set is empty, and empty is not unlocked There is no `go.sum` in the tree, because nothing is required yet. A build does @@ -67,9 +97,9 @@ Its legs run in order, they stop at the first failure, and the run ends by sayin how many of them it examined, so a run that covered two of three cannot be read as one that covered three and found nothing. -The legs are build, test and format, which is what `decisions/means.md` settles -them as. Which legs exist is printed rather than restated here, because a list in -this file drifts against the one the command runs: +What each leg is made of comes from `decisions/means.md`. Which legs exist is +printed rather than restated here, because a list in this file drifts against the +one the command runs: go run . @@ -83,10 +113,10 @@ because `build`, `deploy` and `report-build-status` are already taken on `main` by the Pages deployment, which has no file in this tree; `internal/gate` holds the prefix, and the suite refuses a leg with no job and a job with no leg. -On a clone where the working copy has CRLF line endings, the format leg lists Go -files you have not touched. The content is not wrong: gofmt does not normalise -before judging, and only `*.json` is pinned to LF today. #23 is where that pin -widens to the rest of the tree. +On a clone made before `.gitattributes` pinned the whole tree to LF, the format +leg lists Go files you have not touched. The content is not wrong: gofmt does not +normalise before judging, and the working copy still holds the carriage returns +the old checkout wrote. The Formatting section above says what fixes it. ## Where things live @@ -114,9 +144,10 @@ the comment at the top of its own file: ls .github/workflows -They are supply-chain and hygiene checks. None of them builds or tests anything, -because there is nothing yet to build or test. That is worth knowing before you -read a green check as the tree being verified. +`gate.yml` is the one that compiles and tests this repository, one job per leg +of the entry point above. The rest are supply-chain and hygiene checks that build +and test nothing, which is worth knowing before you read one of their greens as +the tree being verified. ## Sign your commits diff --git a/internal/format/format.go b/internal/format/format.go new file mode 100644 index 0000000..071f7b2 --- /dev/null +++ b/internal/format/format.go @@ -0,0 +1,144 @@ +// Package format holds the whitespace rules .editorconfig states and the check +// that refuses a tracked file breaking one. +// +// The rules are here rather than in a workflow step because an editor +// configuration is a request and this is the refusal, and because a refusal +// that cannot be run against a planted input cannot be shown to bite. Nothing +// outside the standard library is needed to decide any of them. +// +// gofmt is not re-implemented here. It ships with the toolchain, it already +// decides Go formatting, and decisions/means.md names it. What this adds is the +// part gofmt does not reach: the HTML, the JSON, the workflow YAML and the +// prose, which are most of the tree. +package format + +import ( + "bytes" + "fmt" + "os" + "os/exec" + "path" + "path/filepath" + "strings" +) + +// Finding is one broken rule at one place. Line is 1-based, or 0 when the +// finding is about the file as a whole. +type Finding struct { + Path string + Line int + Rule string + Detail string +} + +func (f Finding) String() string { + if f.Line == 0 { + return fmt.Sprintf("%s: %s: %s", f.Path, f.Rule, f.Detail) + } + return fmt.Sprintf("%s:%d: %s: %s", f.Path, f.Line, f.Rule, f.Detail) +} + +// Rule names, which are what a failure prints. They are constants so a message +// and a test cannot drift apart. +const ( + RuleFinalNewline = "final-newline" + RuleTrailingWhitespace = "trailing-whitespace" + RuleTabIndent = "tab-indent" +) + +// CheckFile judges one file's bytes under the rules for its path. +// +// CRLF is normalised to LF before anything is judged, so the verdict does not +// depend on the checkout that produced the bytes. That is deliberate and it is +// the reason this check can run on Windows and on Linux and agree: what is +// stored is fixed by .gitattributes, and a contributor whose working copy has +// CRLF is not told their tree is broken. A lone carriage return is not +// normalised and not judged; nothing in this tree has one. +// +// A file containing a NUL byte is treated as binary and judged not at all, +// because these rules are about text and a false finding on a binary file is +// how a check gets switched off. +func CheckFile(name string, content []byte) []Finding { + if bytes.IndexByte(content, 0) >= 0 { + return nil + } + content = bytes.ReplaceAll(content, []byte("\r\n"), []byte("\n")) + + var findings []Finding + if len(content) > 0 && content[len(content)-1] != '\n' { + findings = append(findings, Finding{ + Path: name, + Rule: RuleFinalNewline, + Detail: "the file does not end with a newline", + }) + } + + tabsAllowed := tabIndented(name) + lines := strings.Split(strings.TrimSuffix(string(content), "\n"), "\n") + for i, line := range lines { + if line == "" { + continue + } + if strings.HasSuffix(line, " ") || strings.HasSuffix(line, "\t") { + findings = append(findings, Finding{ + Path: name, + Line: i + 1, + Rule: RuleTrailingWhitespace, + Detail: "the line ends in a space or a tab", + }) + } + if !tabsAllowed && strings.HasPrefix(line, "\t") { + findings = append(findings, Finding{ + Path: name, + Line: i + 1, + Rule: RuleTabIndent, + Detail: "the line is indented with a tab, and .editorconfig indents this file type with spaces", + }) + } + } + return findings +} + +// tabIndented says whether .editorconfig indents this path with tabs. Go is the +// only such type in the tree, because gofmt indents with tabs and rewriting it +// would be a fight with the toolchain's own formatter rather than a style. +func tabIndented(name string) bool { + return filepath.Ext(name) == ".go" +} + +// TrackedFiles returns the paths git tracks under root, so an untracked scratch +// file in somebody's working copy never reds the check and every file that will +// reach a reviewer does. +func TrackedFiles(root string) ([]string, error) { + cmd := exec.Command("git", "-C", root, "ls-files", "-z") + out, err := cmd.Output() + if err != nil { + return nil, fmt.Errorf("listing tracked files in %s: %w", root, err) + } + var paths []string + for _, p := range strings.Split(string(out), "\x00") { + if p != "" { + paths = append(paths, p) + } + } + return paths, nil +} + +// CheckTree judges every tracked file under root. Paths in the findings are the +// slash-separated ones git reports, so a failure reads the same on either +// platform. +func CheckTree(root string) ([]Finding, error) { + paths, err := TrackedFiles(root) + if err != nil { + return nil, err + } + var findings []Finding + for _, p := range paths { + content, err := os.ReadFile(filepath.Join(root, filepath.FromSlash(p))) + if err != nil { + return nil, fmt.Errorf("reading %s: %w", p, err) + } + findings = append(findings, CheckFile(path.Clean(p), content)...) + } + return findings, nil +} diff --git a/internal/format/format_test.go b/internal/format/format_test.go new file mode 100644 index 0000000..0221b81 --- /dev/null +++ b/internal/format/format_test.go @@ -0,0 +1,117 @@ +package format + +import ( + "strings" + "testing" +) + +// crlf rewrites LF as CRLF so a test can hand CheckFile the bytes a Windows +// checkout produces. Written as a helper rather than as a literal so no +// carriage return is stored in this file, which .gitattributes fixes as LF. +func crlf(s string) string { + return strings.ReplaceAll(s, "\n", "\r\n") +} + +func rules(findings []Finding) []string { + var out []string + for _, f := range findings { + out = append(out, f.Rule) + } + return out +} + +func TestCheckFileRefusesAMissingFinalNewline(t *testing.T) { + findings := CheckFile("docs/index.html", []byte("

one line

")) + if got := rules(findings); len(got) != 1 || got[0] != RuleFinalNewline { + t.Fatalf("rules refused: %v, want exactly [%s]", got, RuleFinalNewline) + } + if findings[0].Line != 0 { + t.Errorf("line %d, want 0: the rule is about the file, not a line", findings[0].Line) + } +} + +func TestCheckFileRefusesTrailingWhitespace(t *testing.T) { + // A space at the end of the second line, which is what a wrapped paragraph + // leaves behind and what no diff shows. + findings := CheckFile("README.md", []byte("first\nsecond \nthird\n")) + if got := rules(findings); len(got) != 1 || got[0] != RuleTrailingWhitespace { + t.Fatalf("rules refused: %v, want exactly [%s]", got, RuleTrailingWhitespace) + } + if findings[0].Line != 2 { + t.Errorf("line %d, want 2", findings[0].Line) + } +} + +func TestCheckFileRefusesATabIndentOutsideGo(t *testing.T) { + findings := CheckFile(".github/workflows/format.yml", []byte("jobs:\n\tformat:\n")) + if got := rules(findings); len(got) != 1 || got[0] != RuleTabIndent { + t.Fatalf("rules refused: %v, want exactly [%s]", got, RuleTabIndent) + } +} + +func TestCheckFileAllowsATabIndentInGo(t *testing.T) { + if findings := CheckFile("manifest/manifest.go", []byte("func f() {\n\treturn\n}\n")); len(findings) != 0 { + t.Fatalf("refused %v, and gofmt indents Go with tabs", findings) + } +} + +// TestCheckFileGivesTheSameVerdictOnCRLF is the one that decides whether this +// check can be in the gate at all. A formatting rule that reds only on one +// operating system pushes contributors away for a reason they cannot see, so +// the same content judged from a CRLF checkout has to produce the same answer. +func TestCheckFileGivesTheSameVerdictOnCRLF(t *testing.T) { + cases := []struct { + name string + content string + }{ + {"clean.md", "first\nsecond\n"}, + {"trailing.md", "first\nsecond \nthird\n"}, + {"tabbed.yml", "jobs:\n\tformat:\n"}, + } + for _, c := range cases { + lf := rules(CheckFile(c.name, []byte(c.content))) + crlfRules := rules(CheckFile(c.name, []byte(crlf(c.content)))) + if strings.Join(lf, ",") != strings.Join(crlfRules, ",") { + t.Errorf("%s: LF refused %v and CRLF refused %v", c.name, lf, crlfRules) + } + } +} + +// TestCheckFileIgnoresBinary keeps a false finding on a binary file from being +// the reason somebody switches the leg off. +func TestCheckFileIgnoresBinary(t *testing.T) { + if findings := CheckFile("docs/logo.png", []byte("\x89PNG\x00\x1a ")); len(findings) != 0 { + t.Fatalf("refused %v on a file containing a NUL byte", findings) + } +} + +func TestCheckFileAllowsAnEmptyFile(t *testing.T) { + if findings := CheckFile("docs/CNAME", nil); len(findings) != 0 { + t.Fatalf("refused %v on an empty file", findings) + } +} + +// TestTrackedTreeIsFormatted is the leg itself. It reads what git tracks rather +// than what the working directory happens to hold, so a scratch file nobody +// committed cannot red it. +func TestTrackedTreeIsFormatted(t *testing.T) { + const root = "../.." + paths, err := TrackedFiles(root) + if err != nil { + t.Fatalf("listing tracked files: %v", err) + } + if len(paths) == 0 { + t.Fatal("no tracked files found, so this test would pass on an empty tree") + } + + findings, err := CheckTree(root) + if err != nil { + t.Fatalf("checking the tree: %v", err) + } + for _, f := range findings { + t.Errorf("%s", f) + } + if len(findings) > 0 { + t.Logf("%d tracked file(s) checked", len(paths)) + } +} diff --git a/internal/gate/gate.go b/internal/gate/gate.go index 0198926..3b477e4 100644 --- a/internal/gate/gate.go +++ b/internal/gate/gate.go @@ -9,8 +9,9 @@ // the jobs the workflow declares, and the suite refuses a leg with no job and a // job with no leg. // -// What each leg is made of comes from decisions/means.md: Go, go test, and -// gofmt -l over the tree. +// What each leg is made of comes from decisions/means.md, which is why every +// Argv below is the toolchain and nothing installed beside it. Legs is the +// authority for the list; a run prints it, and no comment here repeats it. package gate import ( @@ -80,6 +81,18 @@ func Legs() []Leg { Refuses: "a Go file gofmt would rewrite", OutputIsTheVerdict: true, }, + { + // editorconfig, the half of formatting gofmt does not reach: the + // HTML, the JSON, the workflow YAML and the prose, which are most + // of the tree. Its own leg rather than a widening of format + // because the two are decided by different things and fail for + // different reasons - one is the toolchain's formatter, the other + // is three properties .editorconfig states - and because a leg + // runs one command. + Name: "editorconfig", + Argv: []string{"go", "test", "./internal/format", "-run", "TestTrackedTreeIsFormatted", "-count=1"}, + Refuses: "a tracked file with no final newline, with trailing whitespace, or indented with a tab outside Go", + }, { // gate-tests-reach-nothing, which decisions/headless-and-unelevated.md // names as a leg of this gate. It is its own leg rather than a test