diff --git a/DESIGN.md b/DESIGN.md index 93a7de5..9aa3b4c 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -16,6 +16,7 @@ The packages divide along one line: mechanics and judgment. | `repo` | A typed layer over git, behind a one-method `Runner` interface. | | `plan` | The intent of a run as data: renderable, editable, validatable. | | `group` | The one judgment call, behind a `Grouper` interface. | +| `generated` | Paths holding generated output, which a commit never carries. | | `style` | Commit message conventions, applied and verified. | | `sweep` | Debris detection. Reports only, never removes. | | `config` | `.preen.toml`, the per-repository defaults. | @@ -32,6 +33,8 @@ matters more than avoiding a process boundary. absorbs unpushed commits back into the tree to be redone. - Groups the changes into self-contained commits, ordered so dependencies land first. The rules group by structure; semantic splitting needs a grouper. +- Holds untracked generated output back from the plan, named with the pattern + that caught it. Overridable with `--allow-generated` or `[generated]`. - Shows a plan and changes nothing until it is approved. The prompt accepts merge, split, move, reword, drop, and reorder. - Stages each group precisely, including a subset of one file's hunks when an @@ -53,8 +56,10 @@ against real repositories rather than a mock. hook reformatting files the run committed, and only when the caller passed `--allow-hook-rewrites`, which is then reported. 2. **A plan accounts for the tree exactly once.** Every change lands in a - commit or a declared leftover, nothing lands twice, no commit is empty. An - edit that breaks this is refused and the previous plan stands. + commit, a declared leftover, or a declared held path, nothing lands twice, + no commit is empty. An edit that breaks this is refused and the previous + plan stands. Generated output is held rather than dropped for this reason: + a path preen declines to commit is still one it accounts for out loud. 3. **Published work is not redone by accident.** A merge whose second parent is reachable from any remote moves the base forward instead of being flattened, and redoing a pushed commit requires `--pushed`. @@ -67,7 +72,9 @@ against real repositories rather than a mock. index move, the working tree does not. `--hard` and `--keep` both delete files the undone commits added, which is a data-loss bug, not a preference. 7. **Nothing is deleted on a guess.** The sweep reports debris and never - removes it. + removes it, and holding generated output back leaves the file untouched in + the working tree. preen never edits `.gitignore` either: doing so mid-run + would move the content hash the conservation check compares. ## Tests @@ -93,6 +100,12 @@ Tag `vX.Y.Z`; the release workflow cross-builds and attaches archives. Keep wrong answer rewrites the wrong commit, so it stays coarse deliberately. - The grouper contract is JSON over stdin and stdout. No reference grouper ships with preen. +- Two unrelated edits to two files in one package still share a commit. Telling + them apart means reading the diff for meaning, which the deterministic rules + will not do; the prompt's `split` and a `--grouper` cover it. +- A held path cannot be recovered at the approval prompt. `drop` moves a path + out of the plan but nothing moves one back in, so the override is + `--allow-generated` or a `[generated] allow` entry. ## External artifacts diff --git a/README.md b/README.md index 0cb5fa8..5dec185 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,9 @@ exactly. A single differing byte rolls the run back to the recovery branch it made before it started. Every run leaves a `preen-backup/` branch and `preen restore` puts you back. [How that works](#it-proves-it-did-not-lose-your-work). -The built-in subjects say where, not why: `Add api`, `Update dependencies`. For +The built-in subjects say what changed, not why: `Add cmd/parser.go and its +test`, `Update dependencies`. Every one of them is built from the file list and +nothing else, so a subject is a claim the commit can be checked against. For messages that explain intent, hand grouping to any program you like with `--grouper`, a model included, and reword anything at the approval prompt. @@ -79,16 +81,20 @@ Planned commits (4): 1. Update dependencies go.mod -2. Add api +2. Add api/server.go and its test api/server.go api/server_test.go -3. Add store +3. Add store/db.go store/db.go -4. Add guide.md +4. Update docs/guide.md docs/guide.md +Held back as generated output (1), left uncommitted: + api/__pycache__/server.cpython-313.pyc [__pycache__/: Python bytecode cache] + Add them to .gitignore, or rerun with --allow-generated to commit them. + Apply this plan? 4 commits [y/n, or ? for edits]: ``` @@ -115,6 +121,10 @@ cannot walk you into losing a change. dependencies, CI, documentation, and configuration, so each commit stands on its own. Structure is what the rules can see; a `--grouper` program judges intent. Add `--gate` to verify each commit builds and passes. +- Refuses to commit generated output. A `__pycache__` directory your `.gitignore` + never listed is untracked as far as git is concerned, and a tool that produces + commits should not help you record a bytecode cache as work. It is named in the + plan with the reason and left where it was. `--allow-generated` commits it. - Absorbs a run of unpushed commits back into the tree and redoes them clean with `--absorb`, no manual reset. - Folds dirty changes into the unpushed commits that introduced them with @@ -140,12 +150,32 @@ It never invents changes and never touches a commit you did not ask it to. The grouping is deterministic and needs no model. preen separates dependency manifests, CI configuration, documentation, and configuration from source, then -groups source by package, keeps a test file with the code it exercises, keeps -rename pairs together, and treats anything you staged by hand as a boundary you -drew deliberately. Dependencies are recorded first and documentation last. - -Because a fixed rule cannot know whether two hunks in one file are one idea or -two, the built-in grouper never splits a file. +groups source by package and treats anything you staged by hand as a boundary +you drew deliberately. Dependencies are recorded first and documentation last. + +Inside a package it works in units rather than files. A unit is a source file +plus the tests that exercise it, which is the one pairing a file name states +outright: `parser_test.go` belongs with `parser.go`, and `test_runner.py` with +`runner.py`. From there: + +- New code is separated from changes to code that was already there, so a new + file and its test do not ride along with two unrelated fixes. +- Tests for code this run did not otherwise touch become their own commit, named + for what they test, and land after the code they depend on. +- A file that moved between directories keeps both halves in one commit, so it + reads as a move rather than a deletion and an unrelated addition. + +Subjects are written from that same evidence and nothing else: the files, their +directory, whether each is new or changed, and whether a test came with its +source. That gives `Add cmd/parser.go and its test` and `Update token.go and +tool.go in cmd` instead of `Update cmd`. Where a subject would run past +`--max-subject`, it gives up detail rather than being cut off, falling back +through the file name to the package name. + +Two unrelated edits to two files in the same package still share a commit. +Telling them apart means reading the diff for meaning, which a fixed rule cannot +do without guessing. For the same reason the built-in grouper never splits a +file: it cannot know whether two hunks are one idea or two. When you want that judgment, hand grouping to a program: @@ -165,6 +195,45 @@ than trusting it. Every guardrail is the same either way. The grouper chooses *what goes where* and nothing else. +## Generated output + +A `__pycache__` directory your `.gitignore` never listed shows up in `git +status` as untracked, which is the same thing a new source file looks like. A +tool whose whole job is producing clean commits should not turn that into `Add +__pycache__`. + +So preen recognizes the paths that hold generated output rather than work: +bytecode caches, installed dependencies, coverage reports, and the files editors +and operating systems leave lying around. An untracked path that matches one is +held back from the plan, named with the pattern that caught it and what it +holds, and left in your working tree exactly as it was. + +Held back is not dropped. The plan still accounts for the path, you see it +before you approve anything, and the content hash preen conserves across the run +still covers it. Nothing is deleted and nothing is written to your `.gitignore`: +editing that file mid-run would change the very content the conservation check +exists to prove was unchanged. + +Only untracked paths are ever held. A path git already tracks was committed +deliberately at some point, and preen does not second-guess that. + +Override it where a project vendors one of these directories on purpose: + +``` +preen --allow-generated +``` + +```toml +[generated] +allow-all = false # commit generated output like anything else +patterns = ["*.snap"] # hold these back too +allow = ["third_party/node_modules"] # commit these despite the patterns +``` + +A pattern with a slash is a path from the repository root and covers everything +under it. A bare name ending in a slash is a directory, matched wherever it +appears. Anything else is a glob on the file name. + ## Message style preen writes a short imperative subject by default. Dictate the format with @@ -198,6 +267,11 @@ allow-no-verify = false [protect] branches = ["develop", "release/*"] + +[generated] +allow-all = false +patterns = ["*.snap"] +allow = ["third_party/node_modules"] ``` Flags beat the config file, which beats the defaults. Every generated message is diff --git a/cmd/cmd_preen.go b/cmd/cmd_preen.go index 60d94ac..9c120d2 100644 --- a/cmd/cmd_preen.go +++ b/cmd/cmd_preen.go @@ -39,12 +39,15 @@ func runPreen(ctx context.Context, env *environment, args []string) (int, error) engine := run.New(repository) engine.Out = env.Out + // The grouper writes subjects, so it is told the length they have to fit. + builtIn := group.Heuristic{RespectStaged: true, MaxSubject: subjectBudget(opts.Style)} + engine.Grouper = builtIn if settings.Grouper != "" { // A grouper that fails or answers with something unusable falls back to // the built-in rules rather than taking the run down with it. engine.Grouper = group.Chain( group.Command{Name: settings.Grouper, Dir: repository.Root()}, - group.NewHeuristic(), + builtIn, ) } if opts.Fixup { @@ -53,7 +56,12 @@ func runPreen(ctx context.Context, env *environment, args []string) (int, error) built, err := engine.Plan(ctx, opts) if err != nil { - if errors.Is(err, run.ErrNothingToDo) { + switch { + case errors.Is(err, run.ErrAllGenerated): + env.printf("Nothing to preen: every change is generated output.\n%v\n", err) + env.println("Add those paths to .gitignore, or rerun with --allow-generated.") + return CodeNothingToDo, nil + case errors.Is(err, run.ErrNothingToDo): env.println("Nothing to preen: the tree is clean and no commits need redoing.") return CodeNothingToDo, nil } @@ -94,7 +102,12 @@ func runFixup(ctx context.Context, env *environment, engine *run.Engine, opts run.Options, settings promptSettings) (int, error) { built, err := engine.PlanFixup(ctx, opts) if err != nil { - if errors.Is(err, run.ErrNothingToDo) { + switch { + case errors.Is(err, run.ErrAllGenerated): + env.printf("Nothing to fold in: every change is generated output.\n%v\n", err) + env.println("Add those paths to .gitignore, or rerun with --allow-generated.") + return CodeNothingToDo, nil + case errors.Is(err, run.ErrNothingToDo): env.println("Nothing to fold in: no dirty changes over unpushed commits.") return CodeNothingToDo, nil } @@ -171,6 +184,8 @@ func parseRunFlags(env *environment, args []string) (opts run.Options, settings fs.BoolVar(&opts.AllowHookRewrites, "allow-hook-rewrites", false, "accept content changes a commit hook made to files the run committed") fs.BoolVar(&opts.Sweep, "sweep", false, "report debug prints and other leftovers in the diff") + fs.BoolVar(&opts.AllowGenerated, "allow-generated", false, + "commit generated output instead of holding it back") fs.BoolVar(&settings.DryRun, "dry-run", false, "show the plan and stop") fs.BoolVar(&settings.Yes, "yes", false, "skip the approval prompt") showVersion := fs.Bool("version", false, "print the version and exit") diff --git a/cmd/cmd_test.go b/cmd/cmd_test.go index e120399..442c6fe 100644 --- a/cmd/cmd_test.go +++ b/cmd/cmd_test.go @@ -546,3 +546,86 @@ func TestFixupCommandNothingToDo(t *testing.T) { t.Errorf("output:\n%s", out) } } + +// TestGeneratedOutputIsReportedNotCommitted checks the end to end behavior for +// the defect that started this work: a bytecode cache git reports as untracked +// must be named in the plan with the reason, and must not reach a commit. +func TestGeneratedOutputIsReportedNotCommitted(t *testing.T) { + t.Parallel() + c := newCLI(t) + c.write("plugins/runner.py", "def run():\n return 1\n") + c.write("plugins/__pycache__/runner.cpython-313.pyc", "\x00compiled\n") + + code, out, err := c.run("y\n") + if err != nil { + t.Fatalf("run: %v", err) + } + if code != CodeOK { + t.Errorf("code = %d, want %d", code, CodeOK) + } + for _, want := range []string{"Held back as generated output", "Python bytecode cache", ".gitignore"} { + if !strings.Contains(out, want) { + t.Errorf("output does not mention %q:\n%s", want, out) + } + } + if recorded := c.git("log", "--name-only", "--format="); strings.Contains(recorded, "__pycache__") { + t.Errorf("the bytecode cache was committed:\n%s", recorded) + } + status := c.git("status", "--porcelain=v1", "--untracked-files=all") + if !strings.Contains(status, "?? plugins/__pycache__/runner.cpython-313.pyc") { + t.Errorf("the held path did not stay in the working tree:\n%s", status) + } +} + +// TestGeneratedOverridesReachTheRun checks that both overrides get through the +// command line, since a repository that vendors one of these paths has to be +// able to commit it. +func TestGeneratedOverridesReachTheRun(t *testing.T) { + t.Parallel() + + tests := []struct { + Name string + Config string + Args []string + WantHeld bool + }{{ // Test 0: Without consent the cache is held back. + Name: "default", + Args: []string{"--dry-run"}, + WantHeld: true, + }, { // Test 1: The flag commits it. + Name: "flag", + Args: []string{"--dry-run", "--allow-generated"}, + WantHeld: false, + }, { // Test 2: The config file grants the same consent. + Name: "config", + Config: "[generated]\nallow-all = true\n", + Args: []string{"--dry-run"}, + WantHeld: false, + }, { // Test 3: The config exempts one path and holds the rest. + Name: "config allow list", + Config: "[generated]\nallow = [\"plugins/__pycache__/\"]\n", + Args: []string{"--dry-run"}, + WantHeld: false, + }} + + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d %s", testNum, test.Name), func(t *testing.T) { + t.Parallel() + c := newCLI(t) + if test.Config != "" { + c.write(".preen.toml", test.Config) + } + c.write("plugins/runner.py", "def run():\n return 1\n") + c.write("plugins/__pycache__/runner.cpython-313.pyc", "\x00compiled\n") + + _, out, err := c.run("", test.Args...) + if err != nil { + t.Fatalf("run: %v", err) + } + held := strings.Contains(out, "Held back as generated output") + if held != test.WantHeld { + t.Errorf("held = %v, want %v, output:\n%s", held, test.WantHeld, out) + } + }) + } +} diff --git a/cmd/execute.go b/cmd/execute.go index f4a36e4..fbab7a0 100644 --- a/cmd/execute.go +++ b/cmd/execute.go @@ -33,6 +33,13 @@ Run flags: --absorb Bring unpushed commits back and redo them. --fixup Fold changes into the unpushed commits that introduced them. --sweep Report debug prints and other leftovers. Never removes them. + --allow-generated + Commit generated output instead of holding it back. Untracked + paths matching a never-commit pattern (__pycache__/, *.pyc, + node_modules/, .DS_Store, coverage output, editor swap files + and the like) are left uncommitted and reported, since they + regenerate themselves. Add [generated] to .preen.toml to + extend or exempt the patterns for one repository. --grouper PROG Group with an external program instead of the built-in rules. PROG reads JSON on stdin and writes JSON on stdout, and can split one file's hunks across commits. It falls back to the diff --git a/cmd/review.go b/cmd/review.go index 40b14c4..f1b5363 100644 --- a/cmd/review.go +++ b/cmd/review.go @@ -86,5 +86,6 @@ func clonePlan(src *plan.Plan) *plan.Plan { dst.Commits[i].Parts = append([]plan.Part(nil), commit.Parts...) } dst.Leftover = append([]plan.Part(nil), src.Leftover...) + dst.Held = append([]plan.Held(nil), src.Held...) return &dst } diff --git a/cmd/settings.go b/cmd/settings.go index e270151..e379ff3 100644 --- a/cmd/settings.go +++ b/cmd/settings.go @@ -36,9 +36,37 @@ func applyConfig(opts *run.Options, cfg config.Config, args []string) { // Protected branches always come from the repository, never from a flag, so // a project can declare them once and no invocation can quietly drop them. opts.Protected = cfg.Protect.Branches + if !flagGiven(args, "allow-generated") && cfg.Generated.AllowAll { + opts.AllowGenerated = true + } + opts.GeneratedPatterns = cfg.Generated.Patterns + opts.GeneratedAllow = cfg.Generated.Allow opts.Style = mergeStyle(fileStyle, opts.Style, args) } +// subjectBudget is the length the built-in grouper aims its subjects at: the +// effective cap, less the room the style layer will take for a prefix. +// +// Working the prefix in here rather than truncating later is what keeps a +// prefixed subject a whole phrase instead of a severed one. +func subjectBudget(s style.Style) int { + budget := s.MaxSubject + if budget <= 0 { + budget = style.DefaultMaxSubject + } + if s.Prefix != "" { + budget -= len([]rune(s.Prefix)) + 1 + } + if budget < minSubjectBudget { + return minSubjectBudget + } + return budget +} + +// minSubjectBudget is the shortest subject budget worth aiming at. Below this a +// subject cannot say anything, so the style layer's truncation takes over. +const minSubjectBudget = 16 + // mergeStyle layers the invocation's style over the file's, keeping a file // value wherever the invocation did not name that flag. func mergeStyle(file, flags style.Style, args []string) style.Style { diff --git a/config/config.go b/config/config.go index 26f1ad1..50522c6 100644 --- a/config/config.go +++ b/config/config.go @@ -28,6 +28,24 @@ type Config struct { Run RunSection `toml:"run"` // Protect names branches that must never be rewritten. Protect ProtectSection `toml:"protect"` + // Generated tunes which paths preen refuses to commit as generated output. + Generated GeneratedSection `toml:"generated"` +} + +// GeneratedSection tunes the never-commit patterns for one repository. +// +// The defaults cover the caches and build output every project produces, and +// this section is how a project that keeps one of those paths on purpose says +// so, or adds a pattern of its own. +type GeneratedSection struct { + // AllowAll commits generated output like any other change, which turns the + // check off for the whole repository. + AllowAll bool `toml:"allow-all"` + // Patterns are extra never-commit patterns. + Patterns []string `toml:"patterns"` + // Allow are paths this repository commits deliberately, exempted from the + // patterns that would otherwise catch them. + Allow []string `toml:"allow"` } // CommitSection is the message style expressed in file form, using the same diff --git a/config/config_test.go b/config/config_test.go index bb37506..28b97ff 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -7,6 +7,9 @@ import ( "path/filepath" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/dcadolph/preen/style" ) @@ -93,6 +96,30 @@ branches = ["develop", "release/*"] } } +// TestLoadGeneratedSection checks that a repository can extend and exempt the +// never-commit patterns, which is how a project that vendors one of those +// directories on purpose keeps committing it. +func TestLoadGeneratedSection(t *testing.T) { + t.Parallel() + root := writeConfig(t, ` +[generated] +allow-all = false +patterns = ["*.snap"] +allow = ["third_party/node_modules"] +`) + cfg, err := Load(root) + if err != nil { + t.Fatalf("Load: %v", err) + } + want := GeneratedSection{ + Patterns: []string{"*.snap"}, + Allow: []string{"third_party/node_modules"}, + } + if diff := cmp.Diff(want, cfg.Generated, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("generated section mismatch (-want +got):\n%s", diff) + } +} + // TestLoadRejectsBadValues checks that a typo surfaces at load time rather // than as surprising behavior in the middle of a run. func TestLoadRejectsBadValues(t *testing.T) { diff --git a/generated/generated.go b/generated/generated.go new file mode 100644 index 0000000..a150047 --- /dev/null +++ b/generated/generated.go @@ -0,0 +1,144 @@ +// Package generated recognizes paths that hold generated output rather than +// work: bytecode caches, installed dependencies, coverage reports, and the +// files an editor or an operating system leaves lying around. +// +// A repository whose .gitignore misses one of these has it reported as +// untracked, which git status cannot tell apart from new work. preen holds such +// a path back from the plan and says why, rather than helping commit something +// that regenerates itself on the next test run. +// +// Every pattern is overridable. A repository that vendors one of these +// directories on purpose is a real thing, and the tool has no business +// overruling it. +package generated + +import ( + "path" + "strings" +) + +// Rule is one never-commit pattern and what it catches. +type Rule struct { + // Pattern is the path pattern, in the syntax matches documents. + Pattern string + // Why names what the pattern catches, so a report can explain the refusal. + Why string +} + +// Configured is the reason given for a pattern the repository added itself, +// where preen has no description of its own to offer. +const Configured = "matched a configured pattern" + +// Defaults are the patterns preen holds back without being asked. +// +// The list is deliberately short and boring: every entry is output some tool +// writes and rewrites, and none of them is a file anyone edits by hand. Broad +// build directories like build/ and dist/ are left out, because plenty of +// projects keep real source under those names. +// +//nolint:gochecknoglobals // Immutable lookup. +var Defaults = []Rule{ + {Pattern: "__pycache__/", Why: "Python bytecode cache"}, + {Pattern: "*.pyc", Why: "compiled Python"}, + {Pattern: "*.pyo", Why: "optimized Python bytecode"}, + {Pattern: ".pytest_cache/", Why: "pytest cache"}, + {Pattern: ".mypy_cache/", Why: "mypy cache"}, + {Pattern: ".ruff_cache/", Why: "ruff cache"}, + {Pattern: "*.egg-info/", Why: "Python packaging metadata"}, + {Pattern: ".venv/", Why: "Python virtual environment"}, + {Pattern: "venv/", Why: "Python virtual environment"}, + {Pattern: "node_modules/", Why: "installed JavaScript dependencies"}, + {Pattern: ".nyc_output/", Why: "JavaScript coverage output"}, + {Pattern: "htmlcov/", Why: "rendered coverage report"}, + {Pattern: "lcov.info", Why: "coverage report"}, + {Pattern: "coverage.out", Why: "Go coverage profile"}, + {Pattern: "coverage.html", Why: "rendered coverage report"}, + {Pattern: "coverage.xml", Why: "coverage report"}, + {Pattern: ".coverage", Why: "Python coverage data"}, + {Pattern: "target/", Why: "build output"}, + {Pattern: ".gradle/", Why: "Gradle cache"}, + {Pattern: "*.class", Why: "compiled Java"}, + {Pattern: ".DS_Store", Why: "macOS directory metadata"}, + {Pattern: "Thumbs.db", Why: "Windows thumbnail cache"}, + {Pattern: "*.swp", Why: "editor swap file"}, + {Pattern: "*.swo", Why: "editor swap file"}, + {Pattern: "*~", Why: "editor backup file"}, + {Pattern: ".idea/", Why: "JetBrains project settings"}, +} + +// Matcher decides whether a path holds generated output. The zero Matcher +// matches nothing, which is the shape callers want when the check is off. +type Matcher struct { + // rules are the patterns a path is tested against. + rules []Rule + // allow are patterns that override the rules, for paths a repository + // commits deliberately. + allow []string +} + +// New returns a Matcher over the default patterns plus extra, with anything +// allow covers exempted. An extra pattern uses the same syntax as a default. +func New(extra, allow []string) Matcher { + rules := make([]Rule, 0, len(Defaults)+len(extra)) + rules = append(rules, Defaults...) + for _, pattern := range extra { + if pattern = strings.TrimSpace(pattern); pattern != "" { + rules = append(rules, Rule{Pattern: pattern, Why: Configured}) + } + } + return Matcher{rules: rules, allow: allow} +} + +// Off returns a Matcher that holds nothing back, which is what the caller's +// consent to commit generated output amounts to. +func Off() Matcher { return Matcher{} } + +// Match returns the rule that catches a path, and whether one did. An allowed +// path never matches, so a repository can vendor a directory the defaults would +// otherwise refuse. +func (m Matcher) Match(p string) (Rule, bool) { + for _, pattern := range m.allow { + if matches(pattern, p) { + return Rule{}, false + } + } + for _, rule := range m.rules { + if matches(rule.Pattern, p) { + return rule, true + } + } + return Rule{}, false +} + +// matches reports whether one pattern covers a path. +// +// Three forms are supported, chosen so a pattern reads the way it would in a +// .gitignore without dragging in that file's full grammar. A pattern holding a +// slash is a path from the repository root and matches that path and everything +// under it. A bare name with a trailing slash is a directory and matches +// wherever in the tree it appears. Anything else is a glob on the file's own +// name, wherever in the tree it sits. +func matches(pattern, p string) bool { + pattern = strings.TrimPrefix(strings.TrimSpace(pattern), "./") + if p == "" { + return false + } + dirOnly := strings.HasSuffix(pattern, "/") + pattern = strings.TrimSuffix(pattern, "/") + if pattern == "" { + return false + } + if strings.Contains(pattern, "/") { + return p == pattern || strings.HasPrefix(p, pattern+"/") + } + if dirOnly { + for _, part := range strings.Split(path.Dir(p), "/") { + if ok, err := path.Match(pattern, part); err == nil && ok { + return true + } + } + return false + } + ok, err := path.Match(pattern, path.Base(p)) + return err == nil && ok +} diff --git a/generated/generated_test.go b/generated/generated_test.go new file mode 100644 index 0000000..adc46fc --- /dev/null +++ b/generated/generated_test.go @@ -0,0 +1,174 @@ +package generated + +import ( + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" +) + +// TestMatchRecognizesGeneratedOutput checks that the default patterns catch the +// paths a tool writes and leave real work alone. +// +// The __pycache__ case is the one that bit: a repository whose .gitignore +// missed it had the directory reported as untracked, and a commit named after a +// bytecode cache was planned as if it were work. +func TestMatchRecognizesGeneratedOutput(t *testing.T) { + t.Parallel() + + tests := []struct { + Name string + Path string + WantPattern string + WantMatch bool + }{{ // Test 0: The bytecode cache that started this, nested deep in a tree. + Name: "nested pycache", + Path: "internal/roundhouse/plugins/__pycache__/switchtender.cpython-313.pyc", + WantPattern: "__pycache__/", + WantMatch: true, + }, { // Test 1: A stray .pyc outside a cache directory. + Name: "loose bytecode", + Path: "plugins/helper.pyc", + WantPattern: "*.pyc", + WantMatch: true, + }, { // Test 2: Operating system metadata at the repository root. + Name: "ds store", + Path: ".DS_Store", + WantPattern: ".DS_Store", + WantMatch: true, + }, { // Test 3: Installed dependencies, however deep the path runs. + Name: "node modules", + Path: "web/node_modules/react/index.js", + WantPattern: "node_modules/", + WantMatch: true, + }, { // Test 4: A directory pattern that is itself a glob. + Name: "egg info", + Path: "src/preen.egg-info/PKG-INFO", + WantPattern: "*.egg-info/", + WantMatch: true, + }, { // Test 5: Coverage output written by a test run. + Name: "coverage profile", + Path: "coverage.out", + WantPattern: "coverage.out", + WantMatch: true, + }, { // Test 6: An editor swap file left open in a directory. + Name: "swap file", + Path: "cmd/.server.go.swp", + WantPattern: "*.swp", + WantMatch: true, + }, { // Test 7: Ordinary source is never held back. + Name: "source", + Path: "internal/roundhouse/plugins/switchtender.py", + WantMatch: false, + }, { // Test 8: A file whose name merely contains a pattern is not a match. + Name: "lookalike", + Path: "docs/node_modules_guide.md", + WantMatch: false, + }, { // Test 9: A directory named target only matches as a directory. + Name: "file named target", + Path: "target", + WantMatch: false, + }, { // Test 10: Build output under a target directory does match. + Name: "target directory", + Path: "rust/target/debug/app", + WantPattern: "target/", + WantMatch: true, + }} + + matcher := New(nil, nil) + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d %s", testNum, test.Name), func(t *testing.T) { + t.Parallel() + rule, matched := matcher.Match(test.Path) + if matched != test.WantMatch { + t.Fatalf("Match(%q) = %v, want %v", test.Path, matched, test.WantMatch) + } + if diff := cmp.Diff(test.WantPattern, rule.Pattern, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("pattern mismatch (-want +got):\n%s", diff) + } + if matched && rule.Why == "" { + t.Errorf("Match(%q) gave no reason, want one to report", test.Path) + } + }) + } +} + +// TestMatcherOverrides checks the ways a repository overrules the defaults, +// since somewhere a project vendors one of these directories on purpose. +func TestMatcherOverrides(t *testing.T) { + t.Parallel() + + tests := []struct { + Name string + Extra []string + Allow []string + Path string + WantMatch bool + }{{ // Test 0: An allowed directory is committed like anything else. + Name: "allowed directory", + Allow: []string{"third_party/node_modules"}, + Path: "third_party/node_modules/react/index.js", + WantMatch: false, + }, { // Test 1: The allowance is scoped to the path it names. + Name: "allowance does not leak", + Allow: []string{"third_party/node_modules"}, + Path: "web/node_modules/react/index.js", + WantMatch: true, + }, { // Test 2: A repository can add a pattern of its own. + Name: "extra pattern", + Extra: []string{"*.snap"}, + Path: "ui/button.snap", + WantMatch: true, + }, { // Test 3: An allowed glob exempts a file name anywhere. + Name: "allowed glob", + Allow: []string{".DS_Store"}, + Path: "assets/.DS_Store", + WantMatch: false, + }, { // Test 4: Blank patterns are ignored rather than matching everything. + Name: "blank pattern", + Extra: []string{" "}, + Path: "cmd/server.go", + WantMatch: false, + }, { // Test 5: A rooted directory pattern exempts that directory alone. + Name: "rooted directory", + Allow: []string{"internal/plugins/__pycache__/"}, + Path: "internal/plugins/__pycache__/runner.cpython-313.pyc", + WantMatch: false, + }, { // Test 6: The same rooted pattern leaves another copy held back. + Name: "rooted directory elsewhere", + Allow: []string{"internal/plugins/__pycache__/"}, + Path: "tools/__pycache__/runner.cpython-313.pyc", + WantMatch: true, + }} + + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d %s", testNum, test.Name), func(t *testing.T) { + t.Parallel() + _, matched := New(test.Extra, test.Allow).Match(test.Path) + if matched != test.WantMatch { + t.Errorf("Match(%q) = %v, want %v", test.Path, matched, test.WantMatch) + } + }) + } +} + +// TestOffMatchesNothing checks that consent to commit generated output turns +// the whole check off rather than narrowing it. +func TestOffMatchesNothing(t *testing.T) { + t.Parallel() + + paths := []string{ + "internal/plugins/__pycache__/runner.cpython-313.pyc", + ".DS_Store", + "web/node_modules/react/index.js", + } + for testNum, path := range paths { + t.Run(fmt.Sprintf("test %d %s", testNum, path), func(t *testing.T) { + t.Parallel() + if _, matched := Off().Match(path); matched { + t.Errorf("Off().Match(%q) held a path back, want nothing held", path) + } + }) + } +} diff --git a/go.mod b/go.mod index 7c9a1e1..bcd37aa 100644 --- a/go.mod +++ b/go.mod @@ -3,3 +3,5 @@ module github.com/dcadolph/preen go 1.26.4 require github.com/BurntSushi/toml v1.6.0 + +require github.com/google/go-cmp v0.7.0 diff --git a/go.sum b/go.sum index f74b269..7d2396d 100644 --- a/go.sum +++ b/go.sum @@ -1,2 +1,4 @@ github.com/BurntSushi/toml v1.6.0 h1:dRaEfpa2VI55EwlIW72hMRHdWouJeRF7TPYhI+AUQjk= github.com/BurntSushi/toml v1.6.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= +github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= +github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= diff --git a/group/command_test.go b/group/command_test.go index c18f3d7..154a068 100644 --- a/group/command_test.go +++ b/group/command_test.go @@ -17,11 +17,17 @@ func fakeGrouper(t *testing.T, response string) Command { t.Helper() dir := t.TempDir() path := filepath.Join(dir, "grouper.sh") - script := "#!/bin/sh\ncat > /dev/null\ncat <<'JSON'\n" + response + "\nJSON\n" - if err := os.WriteFile(path, []byte(script), 0o700); err != nil { //nolint:gosec // Test fixture. + script := "cat > /dev/null\ncat <<'JSON'\n" + response + "\nJSON\n" + if err := os.WriteFile(path, []byte(script), 0o600); err != nil { t.Fatalf("write script: %v", err) } - return Command{Name: path, Dir: dir} + // Handed to sh as an argument rather than executed directly. Executing a + // file this process just wrote races with the forks of every other parallel + // subtest: a child can still hold the write descriptor when the exec lands, + // and Linux answers ETXTBSY. That surfaced as one subtest in twenty failing + // with "text file busy" on CI and never on a developer's machine. sh is not + // a file these tests write, so there is no window to lose. + return Command{Name: "/bin/sh", Args: []string{path}, Dir: dir} } // splitInput is a two-hunk file, the case only a smarter grouper can divide. diff --git a/group/heuristic.go b/group/heuristic.go index 80e4ca5..2a237c7 100644 --- a/group/heuristic.go +++ b/group/heuristic.go @@ -5,11 +5,11 @@ import ( "fmt" "path" "slices" - "sort" "strings" "github.com/dcadolph/preen/plan" "github.com/dcadolph/preen/repo" + "github.com/dcadolph/preen/style" ) // category is a class of change that belongs in its own commit. @@ -35,8 +35,6 @@ func (c category) title() string { return "Update dependencies" case catCI: return "Update CI configuration" - case catStaged: - return "Commit staged work" default: return "" } @@ -57,10 +55,20 @@ var lockFiles = []string{ // whether two hunks in one file are one idea or two, and guessing wrong is // worse than a slightly coarse commit. Splitting is left to a model-backed // grouper or an explicit edit. +// +// Within a directory it works in units rather than files. A unit is a source +// file plus the tests that exercise it, which is the one pairing a file name +// states outright. Units that only add code are separated from units that +// change code already there, and tests written for code this run did not touch +// stand on their own. Nothing here reads a diff, so the shape of the plan +// follows from the file list and is the same every time. type Heuristic struct { // RespectStaged treats already-staged paths as a boundary the user drew by // hand and keeps them in their own commit. RespectStaged bool + // MaxSubject is the subject length generated subjects aim to fit. Zero means + // the default the style layer applies. + MaxSubject int } // NewHeuristic returns a Heuristic with the default behavior, which honors a @@ -69,60 +77,277 @@ func NewHeuristic() Heuristic { return Heuristic{RespectStaged: true} } -// Group clusters changes by category and, for source, by directory. +// subjectCap returns the length a generated subject aims to fit. +func (h Heuristic) subjectCap() int { + if h.MaxSubject > 0 { + return h.MaxSubject + } + return style.DefaultMaxSubject +} + +// shape is how a bucket's commit is named, which follows from why its changes +// were put together. +type shape int + +// The bucket shapes. +const ( + // shapePlain names a commit after the files it holds. + shapePlain shape = iota + // shapeAdd holds units that only add code, which is separable work from + // changing code that is already there. + shapeAdd + // shapeTests holds tests for code this run did not otherwise touch. + shapeTests + // shapeMove holds both halves of a file that moved between directories. + shapeMove +) + +// rank is where a shape falls in the recording order within one directory: a +// file that moved arrives first, then new code, then changes to code already +// there, then tests for code this run did not otherwise touch. Each step +// depends on the one before it, so nothing lands before what it needs. +func (s shape) rank() int { + switch s { + case shapeMove: + return 0 + case shapeAdd: + return 1 + case shapeTests: + return 3 + default: + return 2 + } +} + +// bucket is one planned commit under construction: the changes it holds plus +// the facts its subject is written from. +type bucket struct { + // key identifies the bucket while changes are being collected. + key string + // cat is the category, which sets where the commit lands in the order. + cat category + // dir is the directory the bucket covers, empty when its changes are not + // tied to one place. + dir string + // shape is how the commit is named. + shape shape + // changes are the changes the commit will carry. + changes []repo.Change +} + +// Group clusters changes into commits by category, then by directory, then by +// the units within a directory. func (h Heuristic) Group(_ context.Context, in Input) ([]plan.Commit, error) { - buckets := make(map[string][]repo.Change) - order := make([]string, 0, len(in.Changes)) - for _, change := range in.Changes { - key := h.bucket(change) - if _, seen := buckets[key]; !seen { - order = append(order, key) + moves := movePairs(in.Changes) + buckets := make(map[string]*bucket, len(in.Changes)) + order := make([]*bucket, 0, len(in.Changes)) + for _, unit := range h.units(in.Changes, moves) { + key := unit.key + existing, seen := buckets[key] + if !seen { + existing = &bucket{key: key, cat: unit.cat, dir: unit.dir, shape: unit.shape} + buckets[key] = existing + order = append(order, existing) } - buckets[key] = append(buckets[key], change) + existing.changes = append(existing.changes, unit.changes...) } - sort.SliceStable(order, func(i, j int) bool { - ci, cj := categoryOf(order[i]), categoryOf(order[j]) - if ci != cj { - return ci < cj - } - return order[i] < order[j] - }) + slices.SortStableFunc(order, byOrder) commits := make([]plan.Commit, 0, len(order)) - for _, key := range order { - changes := buckets[key] - sort.Slice(changes, func(i, j int) bool { return changes[i].Path < changes[j].Path }) - parts := make([]plan.Part, 0, len(changes)) - for _, change := range changes { + for _, b := range order { + slices.SortFunc(b.changes, func(a, c repo.Change) int { return strings.Compare(a.Path, c.Path) }) + parts := make([]plan.Part, 0, len(b.changes)) + for _, change := range b.changes { parts = append(parts, plan.Part{Path: change.Path, From: change.From, Kind: change.Kind}) } - commits = append(commits, plan.Commit{Subject: subjectFor(key, changes), Parts: parts}) + commits = append(commits, plan.Commit{Subject: h.subjectFor(*b), Parts: parts}) } return commits, nil } -// bucket returns the grouping key for a change. The key encodes the category -// and, for source, the directory, so ordering can be derived from it later. -func (h Heuristic) bucket(change repo.Change) string { +// byOrder sorts buckets into the order their commits are recorded: category +// first, then directory, then new code before changed code before tests, so +// what a later commit depends on has already landed. +func byOrder(a, b *bucket) int { + if a.cat != b.cat { + return int(a.cat) - int(b.cat) + } + if a.dir != b.dir { + return strings.Compare(a.dir, b.dir) + } + if a.shape.rank() != b.shape.rank() { + return a.shape.rank() - b.shape.rank() + } + return strings.Compare(a.key, b.key) +} + +// unit is a source file and the tests that exercise it, collected with the +// bucket it belongs to. It is the smallest set of paths a file name proves +// belong in one commit. +type unit struct { + // key is the bucket the unit joins. + key string + // cat is the bucket's category. + cat category + // dir is the bucket's directory. + dir string + // shape is how the bucket's commit is named. + shape shape + // changes are the unit's changes. + changes []repo.Change +} + +// units collects the changes into units and decides which bucket each joins. +// +// The unit is decided before the bucket so a test never parts company with the +// source it names, whatever kinds of change the two carry. +func (h Heuristic) units(changes []repo.Change, moves map[string]movePair) []unit { + byKey := make(map[string][]repo.Change, len(changes)) + order := make([]string, 0, len(changes)) + for _, change := range changes { + key := h.unitKey(change, moves) + if _, seen := byKey[key]; !seen { + order = append(order, key) + } + byKey[key] = append(byKey[key], change) + } + units := make([]unit, 0, len(order)) + for _, key := range order { + units = append(units, h.assign(key, byKey[key], moves)) + } + return units +} + +// unitKey names the unit a change belongs to: the move it is half of, the +// staged boundary it sits behind, the source file it tests, or itself. +func (h Heuristic) unitKey(change repo.Change, moves map[string]movePair) string { + if pair, ok := moves[change.Path]; ok { + return pair.Key + } if h.RespectStaged && change.Staged && !change.Unstaged { - return fmt.Sprintf("%d:", catStaged) + return "staged:" + change.Path + } + if source, ok := testSource(change.Path); ok { + return source + } + return change.Path +} + +// assign works out which bucket a unit joins and how that bucket is named. +func (h Heuristic) assign(key string, changes []repo.Change, moves map[string]movePair) unit { + if pair, ok := moves[changes[0].Path]; ok { + return unit{ + key: key, cat: classify(pair.To), dir: dirOf(pair.To), + shape: shapeMove, changes: changes, + } + } + if strings.HasPrefix(key, "staged:") { + return unit{key: "staged", cat: catStaged, shape: shapePlain, changes: changes} } - cat := classify(change.Path) + cat := classify(changes[0].Path) if cat != catSource { - return fmt.Sprintf("%d:", cat) + return unit{key: fmt.Sprintf("cat:%d", cat), cat: cat, shape: shapePlain, changes: changes} } - dir := path.Dir(change.Path) - if dir == "." { - dir = "" + dir := dirOf(changes[0].Path) + shape := shapeFor(changes) + return unit{ + key: fmt.Sprintf("%s|%d", dir, shape), cat: catSource, + dir: dir, shape: shape, changes: changes, + } +} + +// shapeFor decides what kind of work a unit is, from what happened to its files. +// +// A unit whose files are all tests is work on the tests alone, since a unit +// takes its name from the source file and that file did not change. A unit that +// only adds files is new code, which stands apart from a change to code that +// was already there. Anything else is a plain change. +func shapeFor(changes []repo.Change) shape { + tests, added := true, true + for _, change := range changes { + if _, ok := testSource(change.Path); !ok { + tests = false + } + if change.Kind != repo.KindAdded && change.Kind != repo.KindUntracked { + added = false + } + } + switch { + case tests: + return shapeTests + case added: + return shapeAdd + default: + return shapePlain } - return fmt.Sprintf("%d:%s", catSource, dir) } -// categoryOf recovers the category from a bucket key. -func categoryOf(key string) category { - var cat int - _, _ = fmt.Sscanf(key, "%d:", &cat) - return category(cat) +// movePair is a file that moved between directories without git recording a +// rename, seen as one addition and one deletion of the same file name. +type movePair struct { + // Key is the bucket both halves join. + Key string + // From is the path the file left. + From string + // To is the path it arrived at. + To string +} + +// movePairs finds the moves hiding in a change set. +// +// Both halves of a move belong in one commit: split apart they read as a +// deletion of one file and an unrelated addition of another, and a bisect +// landing between them gets a tree with the file missing. The match is only +// made when exactly one addition and one deletion claim a file name, so an +// ambiguous name is left alone rather than paired on a guess. +func movePairs(changes []repo.Change) map[string]movePair { + added, deleted := make(map[string][]string), make(map[string][]string) + for _, change := range changes { + base := path.Base(change.Path) + switch change.Kind { + case repo.KindAdded, repo.KindUntracked: + added[base] = append(added[base], change.Path) + case repo.KindDeleted: + deleted[base] = append(deleted[base], change.Path) + } + } + pairs := make(map[string]movePair, len(added)) + for base, adds := range added { + dels := deleted[base] + if len(adds) != 1 || len(dels) != 1 || dirOf(adds[0]) == dirOf(dels[0]) { + continue + } + pair := movePair{Key: "move:" + base, From: dels[0], To: adds[0]} + pairs[adds[0]] = pair + pairs[dels[0]] = pair + } + return pairs +} + +// testSource returns the path of the source file a test exercises, and whether +// the path reads as a test at all. +// +// The answer comes from the file name and nothing else, following the +// conventions that are near universal in each language: a _test or _spec +// suffix, a .test or .spec infix, or a test_ prefix. The source path it derives +// need not exist, and a caller that cares checks it against the change set. +func testSource(p string) (string, bool) { + dir, base := path.Split(p) + ext := path.Ext(base) + stem := strings.TrimSuffix(base, ext) + switch { + case strings.HasSuffix(stem, "_test"), strings.HasSuffix(stem, "_spec"), + strings.HasSuffix(stem, ".test"), strings.HasSuffix(stem, ".spec"): + stem = stem[:len(stem)-len("_test")] + case strings.HasPrefix(stem, "test_"), strings.HasPrefix(stem, "spec_"): + stem = stem[len("test_"):] + default: + return "", false + } + if stem == "" { + return "", false + } + return dir + stem + ext, true } // classify maps a path onto a category by convention. @@ -151,41 +376,14 @@ func classify(p string) category { return catSource } -// subjectFor writes an imperative subject for a bucket, naming what the group -// touches rather than restating the file list. -func subjectFor(key string, changes []repo.Change) string { - cat := categoryOf(key) - if fixed := cat.title(); fixed != "" { - return fixed - } - verb := verbFor(changes) - switch cat { - case catDocs: - if len(changes) == 1 { - return fmt.Sprintf("%s %s", verb, path.Base(changes[0].Path)) - } - return verb + " documentation" - case catConfig: - if len(changes) == 1 { - return fmt.Sprintf("%s %s", verb, path.Base(changes[0].Path)) - } - return verb + " configuration" - default: - return fmt.Sprintf("%s %s", verb, scopeName(key, changes)) - } -} - -// scopeName names what a source bucket covers: its directory when it has one, -// otherwise the single file it holds or the count of files it does not. -func scopeName(key string, changes []repo.Change) string { - dir := strings.TrimPrefix(key, fmt.Sprintf("%d:", catSource)) - if dir != "" { - return path.Base(dir) - } - if len(changes) == 1 { - return path.Base(changes[0].Path) +// dirOf returns a path's directory, with the repository root as an empty +// string rather than a dot. +func dirOf(p string) string { + dir := path.Dir(p) + if dir == "." { + return "" } - return fmt.Sprintf("%d files", len(changes)) + return dir } // verbFor picks the imperative verb that matches what happened: a group that diff --git a/group/heuristic_test.go b/group/heuristic_test.go index 5aea798..e0fb15e 100644 --- a/group/heuristic_test.go +++ b/group/heuristic_test.go @@ -115,9 +115,11 @@ func TestHeuristicVerbMatchesTheChange(t *testing.T) { WantPrefix: "Update", }, { + // An edit and a deletion are both changes to code that is already + // there, so they share a commit and the verb covers both. Name: "mixed becomes an update", Changes: []repo.Change{ - {Path: "api/server.go", Kind: repo.KindUntracked}, + {Path: "api/server.go", Kind: repo.KindModified}, {Path: "api/old.go", Kind: repo.KindDeleted}, }, WantPrefix: "Update", diff --git a/group/subject.go b/group/subject.go new file mode 100644 index 0000000..4ee6d73 --- /dev/null +++ b/group/subject.go @@ -0,0 +1,207 @@ +package group + +import ( + "fmt" + "path" + "slices" + "strings" + + "github.com/dcadolph/preen/repo" +) + +// maxNamedFiles is how many files a subject names outright before it gives up +// and counts them instead. Past three the list is longer than the cap allows +// and reads as noise rather than information. +const maxNamedFiles = 3 + +// subjectFor writes an imperative subject for a bucket. +// +// Every subject is built from the file list and the kinds of change in it, both +// of which git states outright. Nothing here reads a diff or guesses at intent, +// so a subject preen writes is one the commit can be checked against. +func (h Heuristic) subjectFor(b bucket) string { + if fixed := b.cat.title(); fixed != "" { + return fixed + } + verb := verbFor(b.changes) + switch b.shape { + case shapeMove: + return h.moveSubject(b, verb) + case shapeTests: + return h.testSubject(b, verb) + default: + return h.filesSubject(b, verb) + } +} + +// moveSubject names a file that changed directories, which is the one thing an +// add and a delete together say plainly. +func (h Heuristic) moveSubject(b bucket, verb string) string { + var from, to string + for _, change := range b.changes { + if change.Kind == repo.KindDeleted { + from = change.Path + continue + } + to = change.Path + } + if from == "" || to == "" { + return h.filesSubject(b, verb) + } + name := path.Base(to) + return h.fit( + fmt.Sprintf("Move %s from %s to %s", name, place(dirOf(from)), place(dirOf(to))), + fmt.Sprintf("Move %s to %s", name, place(dirOf(to))), + "Move "+name, + ) +} + +// testSubject names a commit holding tests for code this run did not otherwise +// touch, which is what the tests are for and all that can honestly be said. +func (h Heuristic) testSubject(b bucket, verb string) string { + sources := unitSources(b.changes) + if len(sources) == 1 { + stem := strings.TrimSuffix(path.Base(sources[0]), path.Ext(sources[0])) + return h.fit( + fmt.Sprintf("%s tests for %s", verb, sources[0]), + fmt.Sprintf("%s tests for %s", verb, stem), + ) + } + if b.dir == "" { + return verb + " tests" + } + where := path.Base(b.dir) + return h.fit( + fmt.Sprintf("%s tests for the %s package", verb, where), + fmt.Sprintf("%s tests for %s", verb, where), + ) +} + +// filesSubject names a commit after the files it holds, preferring the most +// specific form that fits the subject cap. +func (h Heuristic) filesSubject(b bucket, verb string) string { + paths := make([]string, 0, len(b.changes)) + for _, change := range b.changes { + paths = append(paths, change.Path) + } + var candidates []string + switch sources := unitSources(b.changes); { + case len(paths) == 1: + candidates = append(candidates, + fmt.Sprintf("%s %s", verb, paths[0]), + fmt.Sprintf("%s %s", verb, path.Base(paths[0]))) + case len(sources) == 1: + // One source and the tests that go with it, which is a single piece of + // work however many files it took. + with := testWord(len(paths) - 1) + candidates = append(candidates, + fmt.Sprintf("%s %s and its %s", verb, sources[0], with), + fmt.Sprintf("%s %s and its %s", verb, path.Base(sources[0]), with)) + } + if names := baseNames(paths); len(names) <= maxNamedFiles { + if b.dir != "" { + candidates = append(candidates, + fmt.Sprintf("%s %s in %s", verb, andList(names), path.Base(b.dir))) + } + candidates = append(candidates, fmt.Sprintf("%s %s", verb, andList(names))) + } + if b.dir != "" { + candidates = append(candidates, + fmt.Sprintf("%s %d files in %s", verb, len(paths), path.Base(b.dir))) + } + return h.fit(append(candidates, b.fallback(verb))...) +} + +// fallback is the least specific subject a bucket can carry, used when nothing +// more specific fits the cap. It names the place rather than the files, which +// is what a subject can always say. +func (b bucket) fallback(verb string) string { + switch b.cat { + case catDocs: + return verb + " documentation" + case catConfig: + return verb + " configuration" + case catStaged: + return "Commit staged work" + default: + if b.dir != "" { + return fmt.Sprintf("%s %s", verb, path.Base(b.dir)) + } + return fmt.Sprintf("%s %d files", verb, len(b.changes)) + } +} + +// fit returns the first candidate within the subject cap, or the last one when +// none fits. +// +// Candidates run from most specific to least, so a subject that will not fit +// gives up detail rather than being cut off mid word by the style layer. +func (h Heuristic) fit(candidates ...string) string { + limit := h.subjectCap() + for _, candidate := range candidates { + if len([]rune(candidate)) <= limit { + return candidate + } + } + return candidates[len(candidates)-1] +} + +// unitSources returns the distinct source files a bucket's changes belong to, +// with each test counted against the file it exercises. +func unitSources(changes []repo.Change) []string { + seen := make(map[string]bool, len(changes)) + sources := make([]string, 0, len(changes)) + for _, change := range changes { + source := change.Path + if named, ok := testSource(change.Path); ok { + source = named + } + if seen[source] { + continue + } + seen[source] = true + sources = append(sources, source) + } + slices.Sort(sources) + return sources +} + +// baseNames returns the file names of a path list, in path order. +func baseNames(paths []string) []string { + names := make([]string, 0, len(paths)) + for _, p := range paths { + names = append(names, path.Base(p)) + } + return names +} + +// andList renders names as an English list. +func andList(names []string) string { + switch len(names) { + case 0: + return "" + case 1: + return names[0] + case 2: + return names[0] + " and " + names[1] + default: + return strings.Join(names[:len(names)-1], ", ") + ", and " + names[len(names)-1] + } +} + +// testWord returns the singular or plural of test for a count. +func testWord(n int) string { + if n == 1 { + return "test" + } + return "tests" +} + +// place names a directory for a subject, calling the repository root by name +// rather than by an empty string. +func place(dir string) string { + if dir == "" { + return "the repository root" + } + return dir +} diff --git a/group/subject_test.go b/group/subject_test.go new file mode 100644 index 0000000..9ec7369 --- /dev/null +++ b/group/subject_test.go @@ -0,0 +1,134 @@ +package group + +import ( + "context" + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" + + "github.com/dcadolph/preen/repo" +) + +// subjectsOf runs the grouper at a given subject cap and returns the subjects +// it wrote, in plan order. +func subjectsOf(t *testing.T, maxSubject int, changes []repo.Change) []string { + t.Helper() + h := Heuristic{RespectStaged: true, MaxSubject: maxSubject} + commits, err := h.Group(context.Background(), Input{Changes: changes}) + if err != nil { + t.Fatalf("Group: %v", err) + } + subjects := make([]string, 0, len(commits)) + for _, commit := range commits { + subjects = append(subjects, commit.Subject) + } + return subjects +} + +// TestSubjectsNameWhatChanged checks that a subject says which files a commit +// holds and what happened to them, rather than naming the directory and +// stopping there. +func TestSubjectsNameWhatChanged(t *testing.T) { + t.Parallel() + + tests := []struct { + Name string + Changes []repo.Change + Want []string + }{{ // Test 0: One file is named outright, path and all. + Name: "single file", + Changes: []repo.Change{modified("internal/backup/store.go")}, + Want: []string{"Update internal/backup/store.go"}, + }, { // Test 1: A handful of files are listed rather than counted. + Name: "a few files", + Changes: []repo.Change{ + modified("cmd/tool.go"), modified("cmd/token.go"), modified("cmd/root.go"), + }, + Want: []string{"Update root.go, token.go, and tool.go in cmd"}, + }, { // Test 2: Past a handful the count is the honest answer. + Name: "many files", + Changes: []repo.Change{ + modified("cmd/a.go"), modified("cmd/b.go"), + modified("cmd/c.go"), modified("cmd/d.go"), + }, + Want: []string{"Update 4 files in cmd"}, + }, { // Test 3: Documentation is one commit, named by its files rather than + // by the word documentation. + Name: "documentation", + Changes: []repo.Change{modified("README.md"), modified("docs/guide.md")}, + Want: []string{"Update README.md and guide.md"}, + }, { // Test 4: New tests for a package say which package. + Name: "tests for a package", + Changes: []repo.Change{ + added("internal/backup/store_test.go"), added("internal/backup/prune_test.go"), + }, + Want: []string{"Add tests for the backup package"}, + }, { // Test 5: A deletion reads as a removal and names the file. + Name: "deletion", + Changes: []repo.Change{deleted("api/legacy.go")}, + Want: []string{"Remove api/legacy.go"}, + }, { // Test 6: Dependency manifests keep their fixed subject. + Name: "dependencies", + Changes: []repo.Change{modified("go.mod"), modified("go.sum")}, + Want: []string{"Update dependencies"}, + }, { // Test 7: Hand-staged work spanning the tree keeps the subject that + // says why it is one commit. + Name: "staged boundary", + Changes: []repo.Change{ + {Path: "api/a.go", Kind: repo.KindModified, Staged: true}, + {Path: "store/b.go", Kind: repo.KindModified, Staged: true}, + {Path: "web/c.go", Kind: repo.KindModified, Staged: true}, + {Path: "docs/d.go", Kind: repo.KindModified, Staged: true}, + }, + Want: []string{"Commit staged work"}, + }} + + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d %s", testNum, test.Name), func(t *testing.T) { + t.Parallel() + got := subjectsOf(t, 0, test.Changes) + if diff := cmp.Diff(test.Want, got); diff != "" { + t.Errorf("subject mismatch (-want +got):\n%s", diff) + } + }) + } +} + +// TestSubjectsGiveUpDetailToFit checks that a tight cap costs the subject +// detail rather than leaving it severed mid word. +// +// The style layer truncates whatever it is handed, so a subject that will not +// fit has to be written shorter in the first place. +func TestSubjectsGiveUpDetailToFit(t *testing.T) { + t.Parallel() + + changes := []repo.Change{modified("internal/roundhouse/plugins/switchtender.py")} + tests := []struct { + Name string + MaxSubject int + Want []string + }{ // Test 0: With room, the full path is named. + {Name: "roomy", MaxSubject: 72, Want: []string{"Update internal/roundhouse/plugins/switchtender.py"}}, + // Test 1: With less room, the file name alone survives. + {Name: "tight", MaxSubject: 30, Want: []string{"Update switchtender.py"}}, + // Test 2: With almost none, the directory is all that is left. + {Name: "cramped", MaxSubject: 16, Want: []string{"Update plugins"}}, + } + + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d %s", testNum, test.Name), func(t *testing.T) { + t.Parallel() + got := subjectsOf(t, test.MaxSubject, changes) + if diff := cmp.Diff(test.Want, got); diff != "" { + t.Errorf("subject mismatch (-want +got):\n%s", diff) + } + for _, subject := range got { + if len([]rune(subject)) > test.MaxSubject { + t.Errorf("subject %q is %d characters, over the cap of %d", + subject, len([]rune(subject)), test.MaxSubject) + } + } + }) + } +} diff --git a/group/units_test.go b/group/units_test.go new file mode 100644 index 0000000..2312089 --- /dev/null +++ b/group/units_test.go @@ -0,0 +1,240 @@ +package group + +import ( + "context" + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + "github.com/dcadolph/preen/repo" +) + +// grouped is one planned commit flattened to the two things a caller judges it +// by: what it is called and what it carries. +type grouped struct { + // Subject is the commit's subject line. + Subject string + // Paths are the files the commit carries, in listing order. + Paths []string +} + +// groupingOf runs the built-in grouper over a change set and flattens the +// result, so a test states the whole plan rather than probing at it. +func groupingOf(t *testing.T, changes []repo.Change) []grouped { + t.Helper() + commits, err := NewHeuristic().Group(context.Background(), Input{Changes: changes}) + if err != nil { + t.Fatalf("Group: %v", err) + } + out := make([]grouped, 0, len(commits)) + for _, commit := range commits { + out = append(out, grouped{Subject: commit.Subject, Paths: commit.Paths()}) + } + return out +} + +// added returns an untracked change, the shape a brand new file arrives in. +func added(path string) repo.Change { + return repo.Change{Path: path, Kind: repo.KindUntracked} +} + +// modified returns a change to a file already tracked. +func modified(path string) repo.Change { + return repo.Change{Path: path, Kind: repo.KindModified} +} + +// deleted returns a removed file. +func deleted(path string) repo.Change { + return repo.Change{Path: path, Kind: repo.KindDeleted} +} + +// TestUnitsShapeTheCommits checks that one directory's changes are divided by +// what they are rather than only by where they live. +// +// Grouping by directory alone is what produced a commit called "Update cmd" +// holding two unrelated fixes and a new test file. The unit is the smallest +// group of paths a file name proves belong together, and new code, changed +// code, and standalone tests are separated from each other. +func TestUnitsShapeTheCommits(t *testing.T) { + t.Parallel() + + tests := []struct { + Name string + Changes []repo.Change + Want []grouped + }{{ // Test 0: The case that exposed the gap: two unrelated edits plus a new + // file and its test, all in one directory. + Name: "new work separated from edits", + Changes: []repo.Change{ + modified("cmd/tool.go"), + modified("cmd/token.go"), + added("cmd/parser.go"), + added("cmd/parser_test.go"), + }, + Want: []grouped{ + { + Subject: "Add cmd/parser.go and its test", + Paths: []string{"cmd/parser.go", "cmd/parser_test.go"}, + }, + { + Subject: "Update token.go and tool.go in cmd", + Paths: []string{"cmd/token.go", "cmd/tool.go"}, + }, + }, + }, { // Test 1: A test for code this run did not touch is its own work and + // lands after the code it depends on. + Name: "standalone tests stand alone", + Changes: []repo.Change{ + modified("cmd/token.go"), + added("cmd/tool_test.go"), + }, + Want: []grouped{ + {Subject: "Update cmd/token.go", Paths: []string{"cmd/token.go"}}, + {Subject: "Add tests for cmd/tool.go", Paths: []string{"cmd/tool_test.go"}}, + }, + }, { // Test 2: A test whose source changed rides with it, whatever kinds of + // change the two carry. + Name: "a test rides with its source", + Changes: []repo.Change{ + modified("cmd/token.go"), + added("cmd/token_test.go"), + }, + Want: []grouped{{ + Subject: "Update cmd/token.go and its test", + Paths: []string{"cmd/token.go", "cmd/token_test.go"}, + }}, + }, { // Test 3: Python conventions are read the same way as Go ones. + Name: "python test naming", + Changes: []repo.Change{ + modified("plugins/runner.py"), + added("plugins/test_runner.py"), + }, + Want: []grouped{{ + Subject: "Update plugins/runner.py and its test", + Paths: []string{"plugins/runner.py", "plugins/test_runner.py"}, + }}, + }, { // Test 4: A file that moved between directories is one commit that says + // so, not a deletion and an unrelated addition. + Name: "a move stays together", + Changes: []repo.Change{ + added("internal/util/text.go"), + deleted("util/text.go"), + }, + Want: []grouped{{ + Subject: "Move text.go from util to internal/util", + Paths: []string{"internal/util/text.go", "util/text.go"}, + }}, + }, { // Test 5: An addition and a deletion with different names are not a + // move, and are not forced into one commit either. + Name: "unrelated add and delete", + Changes: []repo.Change{ + added("api/server.go"), + deleted("api/old.go"), + }, + Want: []grouped{ + {Subject: "Add api/server.go", Paths: []string{"api/server.go"}}, + {Subject: "Remove api/old.go", Paths: []string{"api/old.go"}}, + }, + }, { // Test 6: Two packages never share a commit, whatever their changes. + Name: "packages stay apart", + Changes: []repo.Change{ + added("api/server.go"), + added("store/db.go"), + }, + Want: []grouped{ + {Subject: "Add api/server.go", Paths: []string{"api/server.go"}}, + {Subject: "Add store/db.go", Paths: []string{"store/db.go"}}, + }, + }} + + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d %s", testNum, test.Name), func(t *testing.T) { + t.Parallel() + got := groupingOf(t, test.Changes) + if diff := cmp.Diff(test.Want, got, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("grouping mismatch (-want +got):\n%s", diff) + } + }) + } +} + +// TestMovePairsNeedAnUnambiguousName checks that a move is only claimed when +// exactly one addition and one deletion answer to a file name, since pairing on +// a guess would put two unrelated files in one commit. +func TestMovePairsNeedAnUnambiguousName(t *testing.T) { + t.Parallel() + + tests := []struct { + Name string + Changes []repo.Change + WantCommit int + }{{ // Test 0: One addition and one deletion of the same name is a move. + Name: "unambiguous move", + Changes: []repo.Change{added("new/x.go"), deleted("old/x.go")}, + WantCommit: 1, + }, { // Test 1: Two additions of the same name leave the pairing ambiguous, + // so no move is claimed. + Name: "two candidates", + Changes: []repo.Change{ + added("new/x.go"), added("other/x.go"), deleted("old/x.go"), + }, + WantCommit: 3, + }, { // Test 2: A name that stays in its directory is an edit, not a move. + Name: "same directory", + Changes: []repo.Change{added("pkg/x.go"), deleted("pkg/x.go")}, + WantCommit: 1, + }} + + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d %s", testNum, test.Name), func(t *testing.T) { + t.Parallel() + got := groupingOf(t, test.Changes) + if len(got) != test.WantCommit { + t.Errorf("got %d commits, want %d: %+v", len(got), test.WantCommit, got) + } + }) + } +} + +// TestTestSourceReadsNamingConventions checks the one pairing a file name +// states outright, since every unit is built on it. +func TestTestSourceReadsNamingConventions(t *testing.T) { + t.Parallel() + + tests := []struct { + Path string + WantSource string + WantTest bool + }{ // Test 0: The Go convention. + {Path: "cmd/parser_test.go", WantSource: "cmd/parser.go", WantTest: true}, + // Test 1: The Python convention. + {Path: "plugins/test_runner.py", WantSource: "plugins/runner.py", WantTest: true}, + // Test 2: The JavaScript convention. + {Path: "web/button.test.ts", WantSource: "web/button.ts", WantTest: true}, + // Test 3: The Ruby and JavaScript spec convention. + {Path: "app/user_spec.rb", WantSource: "app/user.rb", WantTest: true}, + // Test 4: A file at the repository root. + {Path: "main_test.go", WantSource: "main.go", WantTest: true}, + // Test 5: Ordinary source is not a test. + {Path: "cmd/parser.go", WantTest: false}, + // Test 6: A name that is only the marker has no source to pair with. + {Path: "cmd/_test.go", WantTest: false}, + // Test 7: A word ending in test is not the marker. + {Path: "cmd/latest.go", WantTest: false}, + } + + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d %s", testNum, test.Path), func(t *testing.T) { + t.Parallel() + source, isTest := testSource(test.Path) + if isTest != test.WantTest { + t.Fatalf("testSource(%q) reported %v, want %v", test.Path, isTest, test.WantTest) + } + if diff := cmp.Diff(test.WantSource, source, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("source mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/plan/plan.go b/plan/plan.go index 521f162..67e8d24 100644 --- a/plan/plan.go +++ b/plan/plan.go @@ -85,6 +85,22 @@ func (c Commit) Paths() []string { return paths } +// Held is a change preen refused to plan because its path holds generated +// output, recorded with the reason so the plan can say why. +// +// A held change stays in the working tree exactly as it was, and it still +// counts toward the coverage check, so holding one back cannot quietly drop +// work: the plan accounts for it, it is shown before approval, and the content +// hash the run conserves still includes it. +type Held struct { + // Part is the change being left uncommitted. + Part Part + // Pattern is the rule that matched the path. + Pattern string + // Why names what the path holds, so the report explains the refusal. + Why string +} + // Plan is the full intent of a run. type Plan struct { // Base is the commit the run resets to, empty for a working-tree-only run. @@ -99,6 +115,9 @@ type Plan struct { Commits []Commit // Leftover are changes deliberately left uncommitted. Leftover []Part + // Held are changes left uncommitted because they hold generated output, + // each with the pattern that caught it. + Held []Held // Push is the exact force push a published rewrite will run, empty for any // run that does not publish. Push string @@ -120,11 +139,13 @@ func (p Plan) Revalidate() error { return p.Validate(p.Covers) } func (p Plan) Resets() bool { return p.Base != "" } // Validate checks that the plan is internally coherent and covers exactly the -// supplied set of changes: every change lands somewhere, nothing lands twice, -// and no commit is empty. +// supplied set of changes: every change lands in a commit, a leftover, or a +// held path, nothing lands twice, and no commit is empty. // // This is the check that keeps a grouping mistake from silently dropping work, -// and it runs before the repository is touched. +// and it runs before the repository is touched. Held paths are counted here for +// that reason: a path preen declines to commit is still a path it has to +// account for out loud. func (p Plan) Validate(changes []repo.Change) error { if len(p.Commits) == 0 { return fmt.Errorf("%w: no commits planned", ErrInvalid) @@ -148,6 +169,11 @@ func (p Plan) Validate(changes []repo.Change) error { return err } } + for _, held := range p.Held { + if err := recordPart(seen, held.Part); err != nil { + return err + } + } return coversAll(seen, changes) } diff --git a/plan/plan_test.go b/plan/plan_test.go index b9c217e..1307ea6 100644 --- a/plan/plan_test.go +++ b/plan/plan_test.go @@ -110,6 +110,37 @@ func TestValidate(t *testing.T) { Changes: changes("api.go"), Want: ErrInvalid, }, + { // Test 10: A held path is accounted for like any other, which is what + // keeps refusing to commit generated output honest rather than + // silent. + Name: "held path covers the change", + Plan: Plan{ + Commits: []Commit{whole("Add api", "api.go")}, + Held: []Held{{ + Part: Part{Path: "__pycache__/api.cpython-313.pyc"}, + Pattern: "__pycache__/", + Why: "Python bytecode cache", + }}, + }, + Changes: changes("api.go", "__pycache__/api.cpython-313.pyc"), + Want: nil, + }, + { // Test 11: Dropping the record of a held path leaves the change + // unaccounted for, so the coverage check catches it. + Name: "held path missing from the plan", + Plan: Plan{Commits: []Commit{whole("Add api", "api.go")}}, + Changes: changes("api.go", "__pycache__/api.cpython-313.pyc"), + Want: ErrInvalid, + }, + { // Test 12: A held path cannot also be claimed by a commit. + Name: "held path claimed twice", + Plan: Plan{ + Commits: []Commit{whole("Add api", "api.go", "cache.pyc")}, + Held: []Held{{Part: Part{Path: "cache.pyc"}, Pattern: "*.pyc"}}, + }, + Changes: changes("api.go", "cache.pyc"), + Want: ErrInvalid, + }, } for testNum, test := range tests { diff --git a/plan/render.go b/plan/render.go index cc3adff..54e1297 100644 --- a/plan/render.go +++ b/plan/render.go @@ -45,6 +45,14 @@ func (p Plan) Render(w io.Writer) error { b.WriteByte('\n') } + if len(p.Held) > 0 { + fmt.Fprintf(&b, "Held back as generated output (%d), left uncommitted:\n", len(p.Held)) + for _, held := range p.Held { + fmt.Fprintf(&b, " %s [%s: %s]\n", held.Part, held.Pattern, held.Why) + } + b.WriteString(" Add them to .gitignore, or rerun with --allow-generated to commit them.\n\n") + } + if len(p.Leftover) > 0 { fmt.Fprintf(&b, "Left uncommitted (%d):\n", len(p.Leftover)) for _, part := range p.Leftover { diff --git a/run/engine.go b/run/engine.go index c44f690..6f23618 100644 --- a/run/engine.go +++ b/run/engine.go @@ -15,6 +15,7 @@ import ( "strings" "time" + "github.com/dcadolph/preen/generated" "github.com/dcadolph/preen/group" "github.com/dcadolph/preen/plan" "github.com/dcadolph/preen/repo" @@ -62,6 +63,25 @@ type Options struct { // Sweep scans the added lines for debris and reports what it finds. It // never removes anything. Sweep bool + // AllowGenerated plans generated output like any other change instead of + // holding it back, which is the consent a repository that vendors such a + // directory deliberately has to give. + AllowGenerated bool + // GeneratedPatterns are extra never-commit patterns the repository declares. + GeneratedPatterns []string + // GeneratedAllow are patterns exempted from the never-commit rules, for + // paths the repository commits on purpose. + GeneratedAllow []string +} + +// generatedMatcher builds the never-commit matcher a run uses. The zero Options +// holds the defaults back, since a tool that produces commits should not need +// to be asked to keep build output out of them. +func (o Options) generatedMatcher() generated.Matcher { + if o.AllowGenerated { + return generated.Off() + } + return generated.New(o.GeneratedPatterns, o.GeneratedAllow) } // Gate runs a check after a commit and reports failure. @@ -176,12 +196,19 @@ func (e *Engine) Plan(ctx context.Context, opts Options) (*plan.Plan, error) { if len(changes) == 0 && len(built.Absorbed) == 0 { return nil, ErrNothingToDo } + // Generated output is held back before the grouper ever sees it, so an + // external grouper cannot plan a commit for it either. + committable, held := holdGenerated(changes, opts.generatedMatcher()) + built.Held = held + if len(committable) == 0 { + return nil, generatedOnly(held) + } - diffs, err := e.Repo.Diff(ctx, pathsOf(changes)...) + diffs, err := e.Repo.Diff(ctx, pathsOf(committable)...) if err != nil { return nil, err } - commits, err := e.Grouper.Group(ctx, group.Input{Changes: changes, Diffs: diffs}) + commits, err := e.Grouper.Group(ctx, group.Input{Changes: committable, Diffs: diffs}) if err != nil { return nil, fmt.Errorf("grouping failed: %w", err) } @@ -198,7 +225,7 @@ func (e *Engine) Plan(ctx context.Context, opts Options) (*plan.Plan, error) { commits[i] = shaped } if opts.Sweep { - for _, finding := range e.sweepAll(diffs, changes) { + for _, finding := range e.sweepAll(diffs, committable) { built.Debris = append(built.Debris, finding.String()) } } diff --git a/run/errors.go b/run/errors.go index 2c2a157..bae032a 100644 --- a/run/errors.go +++ b/run/errors.go @@ -26,4 +26,7 @@ var ( // ErrFixupTarget reports a change no unpushed commit introduced, so there is // nothing to fold it into. ErrFixupTarget = errors.New("no unpushed commit introduced these lines") + // ErrAllGenerated reports a tree whose every change is generated output, so + // there is nothing worth committing. + ErrAllGenerated = errors.New("every change is generated output") ) diff --git a/run/fixup.go b/run/fixup.go index fce8119..c080dc4 100644 --- a/run/fixup.go +++ b/run/fixup.go @@ -59,15 +59,19 @@ func (e *Engine) PlanFixup(ctx context.Context, opts Options) (*FixupPlan, error if len(changes) == 0 { return nil, ErrNothingToDo } + committable, held := holdGenerated(changes, opts.generatedMatcher()) + if len(committable) == 0 { + return nil, generatedOnly(held) + } - built := &plan.Plan{Base: base, Covers: changes} + built := &plan.Plan{Base: base, Covers: changes, Held: held} check, err := e.Repo.CheckMerges(ctx, base) if err != nil { return nil, err } built.MergeSummary = check.Summary() - fixups, leftover, err := e.routeChanges(ctx, base, changes) + fixups, leftover, err := e.routeChanges(ctx, base, committable) if err != nil { return nil, err } diff --git a/run/generated_test.go b/run/generated_test.go new file mode 100644 index 0000000..99444e7 --- /dev/null +++ b/run/generated_test.go @@ -0,0 +1,204 @@ +package run + +import ( + "context" + "errors" + "fmt" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + "github.com/dcadolph/preen/plan" +) + +// pycache is the path that exposed the gap: a Python bytecode cache the test +// suite rewrites on every run, reported as untracked because the repository's +// .gitignore never listed it. +const pycache = "internal/roundhouse/plugins/__pycache__/switchtender.cpython-313.pyc" + +// dirtyWithGenerated writes real work alongside the build droppings that sit +// next to it in a working tree nobody has swept. +func (h *harness) dirtyWithGenerated() { + h.write("internal/roundhouse/plugins/switchtender.py", "def run():\n return 1\n") + h.write(pycache, "\x00\x01compiled\n") + h.write("internal/roundhouse/plugins/__pycache__/helper.cpython-313.pyc", "\x00\x01compiled\n") + h.write(".DS_Store", "macos metadata") +} + +// heldPaths returns the paths a plan held back, in listing order. +func heldPaths(p *plan.Plan) []string { + paths := make([]string, 0, len(p.Held)) + for _, held := range p.Held { + paths = append(paths, held.Part.Path) + } + return paths +} + +// plannedPaths returns every path a plan's commits would record. +func plannedPaths(p *plan.Plan) []string { + var paths []string + for _, commit := range p.Commits { + paths = append(paths, commit.Paths()...) + } + return paths +} + +// TestHoldsGeneratedOutputBack is the regression test for the defect that +// started this: preen planned a commit called "Add __pycache__" holding a +// Python bytecode cache, because git reported it as untracked and nothing in +// preen knew what it was. +// +// The cache must not reach a commit, it must be reported with the reason, and +// it must still be in the working tree afterward, untracked and untouched. +func TestHoldsGeneratedOutputBack(t *testing.T) { + t.Parallel() + ctx := context.Background() + h := newHarness(t) + h.dirtyWithGenerated() + + p, err := h.Plan(ctx, Options{}) + if err != nil { + t.Fatalf("Plan: %v", err) + } + want := []string{ + ".DS_Store", + "internal/roundhouse/plugins/__pycache__/helper.cpython-313.pyc", + pycache, + } + sorted := cmpopts.SortSlices(func(a, b string) bool { return a < b }) + if diff := cmp.Diff(want, heldPaths(p), sorted, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("held paths mismatch (-want +got):\n%s", diff) + } + for _, path := range plannedPaths(p) { + if strings.Contains(path, "__pycache__") || path == ".DS_Store" { + t.Errorf("generated output reached a commit: %s", path) + } + } + for _, held := range p.Held { + if held.Pattern == "" || held.Why == "" { + t.Errorf("held %s without saying why: %+v", held.Part.Path, held) + } + } + + result, err := h.Apply(ctx, p, Options{}) + if err != nil { + t.Fatalf("Apply: %v", err) + } + if result.TreeStart != result.TreeEnd { + t.Errorf("content moved: %s to %s", result.TreeStart, result.TreeEnd) + } + // The cache stays exactly where it was: still there, still untracked. + status := h.git("status", "--porcelain=v1", "--untracked-files=all") + if !strings.Contains(status, "?? "+pycache) { + t.Errorf("the held path is not untracked in the working tree:\n%s", status) + } + if recorded := h.git("log", "--name-only", "--format="); strings.Contains(recorded, "__pycache__") { + t.Errorf("a commit recorded the bytecode cache:\n%s", recorded) + } +} + +// TestGeneratedOverrides checks the ways a caller overrules the never-commit +// patterns, since a repository that vendors one of these paths on purpose has +// to be able to say so. +func TestGeneratedOverrides(t *testing.T) { + t.Parallel() + + tests := []struct { + Name string + Options Options + WantHeld int + WantCommitted bool + }{{ // Test 0: Left alone, the defaults hold the cache back. + Name: "defaults", + Options: Options{}, + WantHeld: 3, + WantCommitted: false, + }, { // Test 1: Consent commits it like any other change. + Name: "allow generated", + Options: Options{AllowGenerated: true}, + WantHeld: 0, + WantCommitted: true, + }, { // Test 2: One exempted path is committed while the rest are held. + Name: "allow one path", + Options: Options{GeneratedAllow: []string{"internal/roundhouse/plugins/__pycache__/"}}, + WantHeld: 1, + WantCommitted: true, + }} + + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d %s", testNum, test.Name), func(t *testing.T) { + t.Parallel() + ctx := context.Background() + h := newHarness(t) + h.dirtyWithGenerated() + + p, err := h.Plan(ctx, test.Options) + if err != nil { + t.Fatalf("Plan: %v", err) + } + if len(p.Held) != test.WantHeld { + t.Errorf("held %d paths, want %d: %v", len(p.Held), test.WantHeld, heldPaths(p)) + } + var committed bool + for _, path := range plannedPaths(p) { + committed = committed || strings.Contains(path, "__pycache__") + } + if committed != test.WantCommitted { + t.Errorf("bytecode cache committed = %v, want %v", committed, test.WantCommitted) + } + result, err := h.Apply(ctx, p, test.Options) + if err != nil { + t.Fatalf("Apply: %v", err) + } + if result.TreeStart != result.TreeEnd { + t.Errorf("content moved: %s to %s", result.TreeStart, result.TreeEnd) + } + }) + } +} + +// TestTrackedGeneratedPathsAreStillCommitted checks that a path git already +// tracks is never held back. +// +// A tracked path was committed deliberately at some point, and a tool that +// refused to commit changes to it would break the one repository in a hundred +// that keeps such a directory on purpose. +func TestTrackedGeneratedPathsAreStillCommitted(t *testing.T) { + t.Parallel() + ctx := context.Background() + h := newHarness(t) + h.write("vendor/node_modules/react/index.js", "module.exports = 1;\n") + h.git("add", "-A") + h.git("commit", "-m", "Vendor the dependency") + h.write("vendor/node_modules/react/index.js", "module.exports = 2;\n") + + p, err := h.Plan(ctx, Options{}) + if err != nil { + t.Fatalf("Plan: %v", err) + } + if len(p.Held) != 0 { + t.Errorf("held a tracked path: %v", heldPaths(p)) + } + if len(plannedPaths(p)) != 1 { + t.Errorf("planned %v, want the tracked change committed", plannedPaths(p)) + } +} + +// TestNothingButGeneratedOutputStops checks that a tree holding only build +// droppings reports that rather than planning a commit for them. +func TestNothingButGeneratedOutputStops(t *testing.T) { + t.Parallel() + ctx := context.Background() + h := newHarness(t) + h.write(pycache, "\x00\x01compiled\n") + + _, err := h.Plan(ctx, Options{}) + if !errors.Is(err, ErrAllGenerated) { + t.Fatalf("Plan = %v, want ErrAllGenerated", err) + } + if !strings.Contains(err.Error(), pycache) { + t.Errorf("error does not name the path: %v", err) + } +} diff --git a/run/support.go b/run/support.go index 8d98ee7..24d3da3 100644 --- a/run/support.go +++ b/run/support.go @@ -2,10 +2,13 @@ package run import ( "context" + "fmt" "os/exec" "path" "strings" + "github.com/dcadolph/preen/generated" + "github.com/dcadolph/preen/plan" "github.com/dcadolph/preen/repo" ) @@ -20,6 +23,43 @@ func (shellGate) Check(ctx context.Context, dir, command string) ([]byte, error) return cmd.CombinedOutput() } +// holdGenerated splits surveyed changes into what a run may commit and what it +// holds back because the path holds generated output. +// +// Only an untracked path is ever held. A path git already tracks was committed +// deliberately at some point, and overruling that would break the repository +// that vendors one of these directories on purpose. Holding a path back does +// not drop it: it becomes a declared part of the plan, it is shown before +// approval, and it stays in the working tree, so the content the run conserves +// is unchanged either way. +func holdGenerated(changes []repo.Change, matcher generated.Matcher) ([]repo.Change, []plan.Held) { + kept := make([]repo.Change, 0, len(changes)) + var held []plan.Held + for _, change := range changes { + rule, matched := matcher.Match(change.Path) + if !matched || change.Kind != repo.KindUntracked { + kept = append(kept, change) + continue + } + held = append(held, plan.Held{ + Part: plan.Part{Path: change.Path, From: change.From, Kind: change.Kind}, + Pattern: rule.Pattern, + Why: rule.Why, + }) + } + return kept, held +} + +// generatedOnly reports a run with nothing left to commit once generated output +// was held back, which needs a .gitignore entry rather than a commit. +func generatedOnly(held []plan.Held) error { + paths := make([]string, 0, len(held)) + for _, entry := range held { + paths = append(paths, entry.Part.Path) + } + return fmt.Errorf("%w: %s", ErrAllGenerated, strings.Join(paths, ", ")) +} + // mergeChanges combines the working tree status with the paths an absorb run // is bringing back, keeping one record per path. The working tree entry wins, // since it carries the staged and unstaged detail. diff --git a/skills/preen/SKILL.md b/skills/preen/SKILL.md index fc21333..a692fbe 100644 --- a/skills/preen/SKILL.md +++ b/skills/preen/SKILL.md @@ -9,7 +9,8 @@ description: >- .preen.toml. Run options: --scope preens only part of the tree, --gate runs a check after each commit, --dry-run plans and stops, --fixup folds dirty changes into the unpushed commits that introduced them, --yes skips the - approval prompt, --pushed grants the explicit ask a pushed rewrite requires. + approval prompt, --pushed grants the explicit ask a pushed rewrite requires, + --allow-generated commits build output preen would otherwise hold back. Triggers: "preen", "split my diff", "clean up my commit history", "fix my last commits", "reword these commits", "resplit my commits", "fold my changes into the right commits". @@ -54,6 +55,8 @@ reword, drop, and reorder interactively. - Only part of the tree: `--scope `, repeatable. - Run tests after each commit: `--gate ""`. - Report debug prints and leftovers: `--sweep`. +- Commit generated output preen held back: `--allow-generated`, and only when + the user says that path belongs in the repository. - Rewrite pushed commits: only when the user explicitly asked, in words or with `--pushed`. Pass `--pushed`, and `--pushed-base ` when they named a range. Every other guardrail stays with the binary. @@ -63,6 +66,25 @@ Message style flags (`--conventional`, `--prefix`, `--max-subject`, through as the user asks. A `.preen.toml` at the repository root sets defaults; flags beat the file. +## Generated output + +preen refuses to commit untracked paths that hold generated output rather than +work: `__pycache__/`, `*.pyc`, `node_modules/`, `.DS_Store`, coverage reports, +editor swap files, and the like. They appear in the plan under "Held back as +generated output", with the pattern that caught each one, and stay in the +working tree untouched. + +Relay that section to the user, since it is the part of the plan that needs a +decision. The right fix is almost always a `.gitignore` entry, which is theirs +to make: preen does not write that file, because editing it mid-run would move +the content hash the conservation check compares. Do not add it yourself +without being asked. + +If the path genuinely belongs in the repository, `--allow-generated` commits +everything held back, and a `[generated] allow = [...]` entry in `.preen.toml` +exempts one path for good. A tree holding nothing but generated output exits +with the nothing-to-do code (4) and says so. + ## Exit codes Distinct codes tell you what happened without parsing output: a rejected plan diff --git a/style/style.go b/style/style.go index 120f5a2..32b791e 100644 --- a/style/style.go +++ b/style/style.go @@ -216,7 +216,7 @@ func conventional(subject string) string { kind = "feat" case strings.HasPrefix(subject, "Fix"): kind = "fix" - case strings.HasPrefix(subject, "Remove"): + case strings.HasPrefix(subject, "Remove"), strings.HasPrefix(subject, "Move"): kind = "refactor" case strings.Contains(strings.ToLower(subject), "document"): kind = "docs"