diff --git a/README.md b/README.md index 329bb8a..3e931d9 100644 --- a/README.md +++ b/README.md @@ -366,6 +366,10 @@ fetch_orphaned_approval = false What counts as a change not worth re-reviewing is a judgement about a particular codebase, not something to inherit from a default. Two flags deserve extra thought before you name them. A change to a string literal or a rename can alter behavior without changing the shape of the code the approver reviewed, so retaining an approval across one is a stronger claim than the other categories. And `fetch_orphaned_approval` is the only flag in the section which reaches outside the checkout, so it adds network calls to a run. +A change is checked one hunk at a time. Everything the hunk adds is compared against everything it removes, as two whole blocks rather than line by line, so a statement rewrapped across several lines still matches the single line it replaced. A hunk whose two sides are equal once the enabled flags have had their say is treated as if the approver had already seen it. Anything the enabled flags cannot account for dismisses the approval as before. + +`formatting` ignores everything `whitespace` ignores and, on top of that, braces, semicolons and trailing commas. Parentheses are always significant, since their placement decides operator precedence and separates a call from a reference. Only the shape of the code is compared, so dropping the braces around a multi-statement body reads as formatting. Indentation is the exception: where a language delimits a block by it, the same lines moved to a different depth are a change in what runs, so they dismiss. A pair of braces which closes over nothing is an exception: an empty argument, body or literal says something the code does not say without it, so adding or removing one is a change like any other. + #### Require Both Branch Reviewers (Ownership Handoffs) The `require_both_branch_reviewers` feature enables self-service ownership transfers by requiring approval from codeowners defined in **BOTH** the base branch and the PR branch. This creates an AND relationship between ownership rules from both branches. diff --git a/internal/app/app.go b/internal/app/app.go index d962024..885d6a8 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -123,10 +123,11 @@ func (a *App) Run() (*OutputData, error) { // Setup diff context diffContext := git.DiffContext{ - Base: a.client.PR().Base.GetSHA(), - Head: a.client.PR().Head.GetSHA(), - Dir: a.config.RepoDir, - IgnoreDirs: conf.Ignore, + Base: a.client.PR().Base.GetSHA(), + Head: a.client.PR().Head.GetSHA(), + Dir: a.config.RepoDir, + IgnoreDirs: conf.Ignore, + ApprovalRetention: conf.ApprovalRetention, } // Get the diff of the PR diff --git a/internal/git/diff.go b/internal/git/diff.go index 95c74c0..78c07b3 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -9,6 +9,7 @@ import ( "slices" "strings" + owners "github.com/multimediallc/codeowners-plus/internal/config" "github.com/multimediallc/codeowners-plus/pkg/codeowners" "github.com/sourcegraph/go-diff/diff" ) @@ -85,8 +86,9 @@ func (gd *GitDiff) ChangesSince(ref string) ([]codeowners.DiffFile, error) { return nil, fmt.Errorf("failed to get older diff: %w", err) } changesContext := changesSinceContext{ - newerDiff: gd.diff, - olderDiff: olderDiff, + newerDiff: gd.diff, + olderDiff: olderDiff, + normalizer: newNormalizer(gd.context.ApprovalRetention), } diffFiles, err := changesSince(changesContext) if err != nil { @@ -104,11 +106,15 @@ type DiffContext struct { Head string Dir string IgnoreDirs []string + // ApprovalRetention decides which changes may keep an existing approval. + // A nil section retains nothing. + ApprovalRetention *owners.ApprovalRetention } type changesSinceContext struct { - newerDiff []*diff.FileDiff - olderDiff []*diff.FileDiff + newerDiff []*diff.FileDiff + olderDiff []*diff.FileDiff + normalizer normalizer } func diffToFilename(d *diff.FileDiff) string { @@ -190,7 +196,9 @@ func changesSince(context changesSinceContext) ([]codeowners.DiffFile, error) { Hunks: make([]codeowners.HunkRange, 0, len(d.Hunks)), } for _, hunk := range d.Hunks { - if !oldHunkHashes[hunkHash(hunk)] { + // A hunk which normalizes away leaves nothing new to review. The file + // name goes too: what normalizes away depends on the language. + if !oldHunkHashes[hunkHash(hunk)] && !context.normalizer.isTrivial(fileName, hunk) { newHunkRange := codeowners.HunkRange{ Start: int(hunk.NewStartLine), End: int(hunk.NewStartLine + hunk.NewLines - 1), diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index 729e1ad..dc2f87a 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -770,7 +770,7 @@ func TestDiffOfDiffs(t *testing.T) { t.Errorf("Error parsing diff changes: %v", err) } - diffOutput, err := changesSince(changesSinceContext{newDiff, oldDiff}) + diffOutput, err := changesSince(changesSinceContext{newerDiff: newDiff, olderDiff: oldDiff}) if err != nil { t.Errorf("Error getting diff of diffs: %v", err) } diff --git a/internal/git/normalize.go b/internal/git/normalize.go new file mode 100644 index 0000000..4d5131c --- /dev/null +++ b/internal/git/normalize.go @@ -0,0 +1,82 @@ +package git + +import ( + "bufio" + "bytes" + "strings" + + owners "github.com/multimediallc/codeowners-plus/internal/config" + "github.com/sourcegraph/go-diff/diff" +) + +// normalizer reports whether a hunk's two sides are the same code once the enabled +// flags' noise is stripped. Renames is not a step: it compares what they leave. +type normalizer struct { + steps []normalizeStep +} + +// normalizeStep rewrites one side of a hunk, its lines joined by newlines. The +// file name comes too: what a run of punctuation means depends on the language. +type normalizeStep func(fileName, block string) string + +// Adapts a step which reads a block the same way whatever the language. +func inAnyLanguage(step func(string) string) normalizeStep { + return func(_, block string) string { return step(block) } +} + +func newNormalizer(retention *owners.ApprovalRetention) normalizer { + n := normalizer{} + if retention.FormattingEnabled() { + n.steps = append(n.steps, inAnyLanguage(collapseFormatting)) + } + return n +} + +// A flag may only retain a hunk it actually read, so anything the enabled steps +// cannot account for stays non-trivial and an approval is kept only on purpose. +func (n normalizer) isTrivial(fileName string, hunk *diff.Hunk) bool { + if len(n.steps) == 0 { + return false + } + added, removed, ok := hunkBlocks(hunk.Body) + if !ok { + return false + } + // Checked before the steps run, since they drop the indentation that changed. + if reindentsBlock(fileName, added, removed) { + return false + } + return n.normalize(fileName, added) == n.normalize(fileName, removed) +} + +func (n normalizer) normalize(fileName, block string) string { + for _, step := range n.steps { + block = step(fileName, block) + } + return block +} + +// Rewrapping a statement removes one line and adds several, so the sides are joined +// and compared whole; line-wise comparison could never pair them up. +func hunkBlocks(body []byte) (added, removed string, ok bool) { + var addedLines, removedLines []string + + scanner := bufio.NewScanner(bytes.NewReader(body)) + for scanner.Scan() { + line := scanner.Text() + if len(line) == 0 { + continue + } + switch line[0] { + case '+': + addedLines = append(addedLines, line[1:]) + case '-': + removedLines = append(removedLines, line[1:]) + } + } + // A hunk we could not read whole is not a hunk we can vouch for. + if scanner.Err() != nil || (len(addedLines) == 0 && len(removedLines) == 0) { + return "", "", false + } + return strings.Join(addedLines, "\n"), strings.Join(removedLines, "\n"), true +} diff --git a/internal/git/normalize_formatting.go b/internal/git/normalize_formatting.go new file mode 100644 index 0000000..7dfffa9 --- /dev/null +++ b/internal/git/normalize_formatting.go @@ -0,0 +1,92 @@ +package git + +import ( + "strings" + "unicode" + "unicode/utf8" +) + +const ( + // Dropped entirely, so a braced body and a single-statement body compare equal. + structuralPunctuation = "{};" + // Binds to its neighbour, so the spacing around it is a wrapping choice. + tightPunctuation = "()[]," +) + +// Parentheses and empty brace pairs are deliberately kept: they separate a call +// from a reference and decide precedence, so dropping them would hide changes. +func collapseFormatting(block string) string { + var b strings.Builder + b.Grow(len(block)) + + var last rune + pendingSpace := false + writeToken := func(text string, opening, closing rune) { + if pendingSpace { + pendingSpace = false + // Space matters only where two tokens would otherwise run together. + if !isTight(opening) && !isTight(last) { + b.WriteRune(' ') + } + } + b.WriteString(text) + last = closing + } + + for i := 0; i < len(block); { + if end, ok := emptyBracePairAt(block, i); ok { + // A brace closing over nothing is an empty argument, body or literal, + // not structure: the code says something different without it. + writeToken(emptyBracePair, '{', '}') + i = end + continue + } + r, size := utf8.DecodeRuneInString(block[i:]) + i += size + if unicode.IsSpace(r) || strings.ContainsRune(structuralPunctuation, r) { + pendingSpace = b.Len() > 0 + continue + } + writeToken(string(r), r, r) + } + return dropTrailingCommas(b.String()) +} + +const emptyBracePair = "{}" + +// Only whitespace may stand between the two, so a pair holding a body, however +// it is wrapped, is structure like any other. +func emptyBracePairAt(block string, i int) (end int, ok bool) { + if i >= len(block) || block[i] != '{' { + return 0, false + } + closing := skipSpace(block, i+1) + if closing >= len(block) || block[closing] != '}' { + return 0, false + } + return closing + 1, true +} + +func isTight(r rune) bool { + return strings.ContainsRune(tightPunctuation, r) +} + +func dropTrailingCommas(block string) string { + if !strings.Contains(block, ",") { + return block + } + + var b strings.Builder + b.Grow(len(block)) + for i, r := range block { + if r == ',' && closesAfter(block[i+1:]) { + continue + } + b.WriteRune(r) + } + return b.String() +} + +func closesAfter(rest string) bool { + return rest == "" || rest[0] == ')' || rest[0] == ']' +} diff --git a/internal/git/normalize_formatting_test.go b/internal/git/normalize_formatting_test.go new file mode 100644 index 0000000..aa8a41e --- /dev/null +++ b/internal/git/normalize_formatting_test.go @@ -0,0 +1,132 @@ +package git + +import ( + "testing" + + owners "github.com/multimediallc/codeowners-plus/internal/config" +) + +// Several remove a different number of lines than they add, which is the point: +// only a whole-block comparison can pair the two sides up. +var formattingOnlyHunks = []hunkCase{ + { + name: "single statement wrapped in braces", + file: "widget.js", + body: `- if (!isOpen) return ++ if (!isOpen) { ++ return ++ }`, + }, + { + name: "call rewrapped across lines", + file: "billing.py", + body: `- result = compute(alpha, beta, gamma) ++ result = compute( ++ alpha, ++ beta, ++ gamma, ++ )`, + }, + { + name: "nested selectors flattened", + file: "theme.scss", + body: `-.card { +- .title { +- color: red; +- } +-} ++.card .title { color: red; }`, + }, + { + name: "trailing comma added to a list literal", + file: "billing.py", + body: `-values = [alpha, beta] ++values = [alpha, beta,]`, + }, + { + name: "space added after a separator", + file: "billing.py", + body: `- label = concat(alpha,beta) ++ label = concat(alpha, beta)`, + }, + { + name: "statement semicolon dropped", + file: "widget.js", + body: `-const total = sum(items); ++const total = sum(items)`, + }, + { + name: "closing brace alone in its own hunk", + file: "widget.js", + body: `+}`, + }, + { + name: "empty body reflowed onto one line", + file: "widget.js", + body: `- describe("cleanup", () => { +- }) ++ describe("cleanup", () => {})`, + }, + { + name: "empty body respaced", + file: "billing.py", + body: `- render(template, { }) ++ render(template, {})`, + }, +} + +// An empty pair of braces is an argument, a body or a literal rather than block +// structure, so adding or removing one changes what the code says. +var emptyBracePairHunks = []hunkCase{ + { + name: "empty object argument added", + file: "widget.ts", + body: `- register(handler) ++ register(handler, {})`, + }, + { + name: "empty dict argument removed", + file: "billing.py", + body: `- render(template, {}) ++ render(template)`, + }, + { + name: "empty object added to a wrapped list", + file: "widget.js", + body: ` const rows = [ + first, ++ {}, + ]`, + }, + { + name: "empty options argument added before a semicolon", + file: "widget.ts", + body: `- apply(config); ++ apply(config, {});`, + }, +} + +func TestFormattingRetainsFormattingOnlyHunks(t *testing.T) { + assertTrivial(t, newNormalizer(steps(formattingOn)), formattingOnlyHunks) +} + +func TestFormattingAloneDismissesEverythingElse(t *testing.T) { + n := newNormalizer(steps(formattingOn)) + + assertSignificant(t, n, significantHunks) +} + +// An empty pair of braces says something the code does not say without it, so +// formatting may not drop it the way it drops the braces around a body. +func TestFormattingKeepsEmptyBracePairs(t *testing.T) { + retentions := map[string]*owners.ApprovalRetention{ + "formatting alone": steps(formattingOn), + "every step": steps(commentsOn, whitespaceOn, formattingOn, stringLiteralsOn, renamesOn), + } + + for name, retention := range retentions { + t.Run(name, func(t *testing.T) { + assertSignificant(t, newNormalizer(retention), emptyBracePairHunks) + }) + } +} diff --git a/internal/git/normalize_indent.go b/internal/git/normalize_indent.go new file mode 100644 index 0000000..2dc7e07 --- /dev/null +++ b/internal/git/normalize_indent.go @@ -0,0 +1,65 @@ +package git + +import ( + "path" + "strings" +) + +// Languages which delimit a block by how far its lines are indented, so moving a +// statement in or out of one changes what runs rather than how it reads. +var indentDelimitedExtensions = map[string]bool{ + ".py": true, ".pyi": true, ".pyx": true, + ".yaml": true, ".yml": true, + ".sass": true, ".haml": true, ".slim": true, ".pug": true, ".jade": true, + ".nim": true, ".coffee": true, +} + +// PEP 8's indent width, weighing a tab against spaces rather than rendering +// anything, so a tabs-to-spaces conversion at unchanged depth compares equal. +const tabColumns = 4 + +func indentDepth(line string) int { + depth := 0 + for _, r := range line { + switch r { + case '\t': + depth += tabColumns + case ' ': + depth++ + default: + return depth + } + } + return depth +} + +func indentIsSyntax(fileName string) bool { + return indentDelimitedExtensions[strings.ToLower(path.Ext(fileName))] +} + +// reindentsBlock reports whether the two sides are the same lines in the same +// order at a different depth, which collapseWhitespace reads as nothing at all. +func reindentsBlock(fileName, added, removed string) bool { + if !indentIsSyntax(fileName) { + return false + } + addedLines, removedLines := strings.Split(added, "\n"), strings.Split(removed, "\n") + // What keeps this narrow enough to be worth having: rewrapping an argument + // list changes the line count, so it never reaches the depth comparison. + if len(addedLines) != len(removedLines) { + return false + } + moved := false + for i := range addedLines { + a, r := addedLines[i], removedLines[i] + aBody, rBody := strings.TrimLeft(a, " \t"), strings.TrimLeft(r, " \t") + if aBody != rBody { + return false + } + // A blank line has no depth to carry, so its padding is not a move. + if aBody != "" && indentDepth(a) != indentDepth(r) { + moved = true + } + } + return moved +} diff --git a/internal/git/normalize_indent_test.go b/internal/git/normalize_indent_test.go new file mode 100644 index 0000000..7d11abb --- /dev/null +++ b/internal/git/normalize_indent_test.go @@ -0,0 +1,75 @@ +package git + +import "testing" + +// Where indentation delimits a block, moving a statement changes what runs, and +// collapseWhitespace drops the indentation that says so. +func TestReindentInIndentDelimitedLanguageIsNotTrivial(t *testing.T) { + tt := []struct { + name, file, body string + trivial bool + }{ + { + name: "python statement dedented out of a block", + file: "billing.py", + body: "- charge(user)\n+ charge(user)", + }, + { + name: "python statement indented into a block", + file: "billing.py", + body: "- charge(user)\n+ charge(user)", + }, + { + name: "yaml key moved to another parent", + file: "deploy.yaml", + body: "- replicas: 3\n+ replicas: 3", + }, + { + name: "same depth, tabs converted to spaces", + file: "billing.py", + body: "-\tcharge(user)\n+ charge(user)", + trivial: true, + }, + { + name: "trailing whitespace only", + file: "billing.py", + body: "- charge(user) \n+ charge(user)", + trivial: true, + }, + { + name: "indentation is only layout in a braced language", + file: "billing.go", + body: "- charge(user)\n+ charge(user)", + trivial: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + n := newNormalizer(steps(formattingOn)) + got := n.isTrivial(tc.file, hunkOf(tc.body)) + if got != tc.trivial { + t.Errorf("isTrivial = %v, want %v", got, tc.trivial) + } + }) + } +} + +// The guard reads indentation, so it must not fire where nothing reads it. +func TestReindentGuardInertWithoutALayoutFlag(t *testing.T) { + dedent := "- charge(user)\n+ charge(user)" + if newNormalizer(nil).isTrivial("billing.py", hunkOf(dedent)) { + t.Error("no retention configured must retain nothing") + } + if !reindentsBlock("billing.py", " charge(user)", " charge(user)") { + t.Error("reindentsBlock missed a pure re-indent") + } + if reindentsBlock("billing.go", " charge(user)", " charge(user)") { + t.Error("reindentsBlock fired on a braced language") + } + // Rewrapping changes the line count, so the line-for-line match lets it past. + rewrapped := " charge(\n user,\n amount,\n )" + if reindentsBlock("billing.py", rewrapped, " charge(user, amount)") { + t.Error("reindentsBlock fired on a rewrap, which is what it must not do") + } +} diff --git a/internal/git/normalize_lex.go b/internal/git/normalize_lex.go new file mode 100644 index 0000000..2cde923 --- /dev/null +++ b/internal/git/normalize_lex.go @@ -0,0 +1,39 @@ +package git + +import ( + "strings" +) + +func isLineSpace(b byte) bool { + return b == ' ' || b == '\t' +} + +// A literal left open at the end of its line cannot be read, so it is not blanked. +func stringLiteralAt(block string, i int) (quote byte, content string, end int, ok bool) { + if i >= len(block) || !strings.ContainsRune(`"'`+"`", rune(block[i])) { + return 0, "", 0, false + } + quote = block[i] + for j := i + 1; j < len(block); j++ { + switch block[j] { + case '\\': + j++ + case '\n': + return 0, "", 0, false + case quote: + return quote, block[i+1 : j], j + 1, true + } + } + return 0, "", 0, false +} + +func isIdentifierByte(b byte) bool { + return b == '_' || b >= '0' && b <= '9' || b >= 'a' && b <= 'z' || b >= 'A' && b <= 'Z' +} + +func skipSpace(block string, i int) int { + for i < len(block) && (block[i] == ' ' || block[i] == '\t' || block[i] == '\n' || block[i] == '\r') { + i++ + } + return i +} diff --git a/internal/git/normalize_test.go b/internal/git/normalize_test.go new file mode 100644 index 0000000..7304dc4 --- /dev/null +++ b/internal/git/normalize_test.go @@ -0,0 +1,403 @@ +package git + +import ( + "testing" + + owners "github.com/multimediallc/codeowners-plus/internal/config" + "github.com/sourcegraph/go-diff/diff" +) + +// A hunkCase carries the file it changes: the same body classifies differently +// under a different extension, so the name is part of the fixture. +type hunkCase struct { + name string + file string + body string +} + +// Refiles a set of hunks to run the same bodies against another language. +func withFile(file string, cases []hunkCase) []hunkCase { + refiled := make([]hunkCase, 0, len(cases)) + for _, tc := range cases { + tc.file = file + refiled = append(refiled, tc) + } + return refiled +} + +func flag(value bool) *bool { + return &value +} + +type retentionOpt func(*owners.ApprovalRetention) + +func whitespaceOn(r *owners.ApprovalRetention) { r.Whitespace = flag(true) } + +func commentsOn(r *owners.ApprovalRetention) { r.Comments = flag(true) } + +func formattingOn(r *owners.ApprovalRetention) { r.Formatting = flag(true) } + +func stringLiteralsOn(r *owners.ApprovalRetention) { r.StringLiterals = flag(true) } + +func renamesOn(r *owners.ApprovalRetention) { r.Renames = flag(true) } + +// Enables exactly the named steps, so no test depends on what the umbrella covers. +func steps(opts ...retentionOpt) *owners.ApprovalRetention { + retention := &owners.ApprovalRetention{ + Enabled: true, + Whitespace: flag(false), + Comments: flag(false), + Formatting: flag(false), + StringLiterals: flag(false), + Renames: flag(false), + } + for _, opt := range opts { + opt(retention) + } + return retention +} + +// Hunks which must keep dismissing approvals. Each one either changes +// behavior outright or belongs to a step these flags do not turn on. +var significantHunks = []hunkCase{ + { + name: "condition inverted", + file: "widget.js", + body: `- if (isReady) { ++ if (!isReady) {`, + }, + { + name: "guard clause added", + file: "widget.js", + body: ` value = lookup(key) ++ if (value == null) { ++ return ++ }`, + }, + { + name: "reindented and edited at once", + file: "billing.py", + body: `- total = price * quantity ++ total = price + quantity`, + }, + { + name: "parentheses moved between operands", + file: "billing.py", + body: `- limit = (base + extra) * factor ++ limit = base + (extra * factor)`, + }, + { + name: "reference turned into a call", + file: "widget.js", + body: `- button.onClick = submit ++ button.onClick = submit()`, + }, + { + name: "call turned into a bare token sequence", + file: "widget.js", + body: `- render(items) ++ render items`, + }, + { + name: "space between two names removed", + file: "index.html", + body: `-