From fe6c358d3a5e588210dcd08a9b8e7c46a56df289 Mon Sep 17 00:00:00 2001 From: dcadolph Date: Sun, 27 Sep 2026 20:50:13 -0500 Subject: [PATCH 1/3] Name the files a commit touches in its subject Subjects were built from the directory, so a commit adding a parser and its test read "Add api" and told a reviewer nothing they could check. They now come from the file list: "Add cmd/parser.go and its test". Every subject is derived from the files and nothing else, which makes it a claim the commit can be held to rather than a label. --- go.mod | 2 + go.sum | 2 + group/heuristic.go | 344 +++++++++++++++++++++++++++++++--------- group/heuristic_test.go | 4 +- group/subject.go | 207 ++++++++++++++++++++++++ group/subject_test.go | 134 ++++++++++++++++ group/units_test.go | 240 ++++++++++++++++++++++++++++ style/style.go | 2 +- 8 files changed, 860 insertions(+), 75 deletions(-) create mode 100644 group/subject.go create mode 100644 group/subject_test.go create mode 100644 group/units_test.go 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/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/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" From 281b2f655dc6e1cf0ce13b69b7a701a8c3ac4a72 Mon Sep 17 00:00:00 2001 From: dcadolph Date: Sun, 27 Sep 2026 20:50:40 -0500 Subject: [PATCH 2/3] Hold back generated output instead of committing it A dirty tree carries build output and caches as often as it carries work, and preen committed them because they were changes like any other. A __pycache__ directory landing in a commit is noise a reviewer has to read past and a file the project never wanted tracked. Generated paths are now held back and named at the approval prompt, with the pattern that caught each one, so the plan says what it declined to commit rather than quietly staging it. A project that keeps such a path on purpose says so in config, and --allow-generated turns the check off for one run. --- DESIGN.md | 19 +++- README.md | 94 +++++++++++++++-- cmd/cmd_preen.go | 21 +++- cmd/cmd_test.go | 83 +++++++++++++++ cmd/execute.go | 7 ++ cmd/review.go | 1 + cmd/settings.go | 28 +++++ config/config.go | 18 ++++ config/config_test.go | 27 +++++ generated/generated.go | 144 +++++++++++++++++++++++++ generated/generated_test.go | 174 ++++++++++++++++++++++++++++++ plan/plan.go | 32 +++++- plan/plan_test.go | 31 ++++++ plan/render.go | 8 ++ run/engine.go | 33 +++++- run/errors.go | 3 + run/fixup.go | 8 +- run/generated_test.go | 204 ++++++++++++++++++++++++++++++++++++ run/support.go | 40 +++++++ skills/preen/SKILL.md | 24 ++++- 20 files changed, 974 insertions(+), 25 deletions(-) create mode 100644 generated/generated.go create mode 100644 generated/generated_test.go create mode 100644 run/generated_test.go 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/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 From 8a9cacf7645298e311625807d43895d7e21a327b Mon Sep 17 00:00:00 2001 From: dcadolph Date: Sun, 27 Sep 2026 20:56:11 -0500 Subject: [PATCH 3/3] Stop the grouper fixture racing its own executable The fixture wrote a shell script and ran it, which races the forks of every other parallel subtest: a child can still hold the write descriptor when the exec lands, and Linux answers ETXTBSY. One subtest in twenty failed with "text file busy" on CI and never on a developer's machine. The script is now handed to sh as an argument. sh is not a file these tests write, so there is no window to lose. --- group/command_test.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) 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.