From 1fa98d109c122295a73f028d3b21966e3165f0b3 Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 18 Aug 2026 17:39:14 -0700 Subject: [PATCH] feat: retain approvals across whitespace-only hunks codeowners-plus runs git diff -U0 with no -w, so a reindent produces real hunks with real content hashes and resets the approval. whitespace collapses every run of whitespace to a single space. Collapsed, never deleted: whitespace is what holds two tokens apart, so removing it would let a block which separates two names compare equal to one which runs them into a single name. It rewrites only the gaps between tokens, never the tokens themselves, which is what keeps it the narrowest of the flags. This also brings in the layout rule. Where a hunk's two sides are already the same text before any step has run - a line deleted and added back, a line moved, a hunk of nothing but blank lines - what changed is where the lines sit. Only a flag which reads that may vouch for it, so whitespace and formatting keep such a hunk and the others do not. A flag may only retain a hunk it actually read. --- README.md | 8 ++ internal/git/normalize.go | 15 ++- internal/git/normalize_whitespace.go | 11 ++ internal/git/normalize_whitespace_test.go | 134 ++++++++++++++++++++++ 4 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 internal/git/normalize_whitespace.go create mode 100644 internal/git/normalize_whitespace_test.go diff --git a/README.md b/README.md index 3e931d9..e2e82c9 100644 --- a/README.md +++ b/README.md @@ -368,7 +368,15 @@ What counts as a change not worth re-reviewing is a judgement about a particular 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. +<<<<<<< HEAD `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. +======= +A flag only retains a hunk it read. Where a hunk's two sides are already the same text before any flag has looked at them - a line deleted and added back, a line moved, a hunk of nothing but blank lines - what changed is where the lines sit, so `whitespace` and `formatting` are the only flags which may keep it. `comments`, `string_literals` and `renames` rewrite something else, and a flag which rewrote nothing here has nothing to vouch for. + +`whitespace` ignores indentation, alignment padding, trailing whitespace, blank lines and which line a token happens to sit on. It rewrites only the gaps between tokens, never the tokens themselves, so a change which runs two names together into one is not whitespace. + +`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 a change which moves a statement into or out of a block without editing the statement itself - re-indenting a line in an indentation-sensitive language, or dropping the braces around a multi-statement body - reads as formatting. 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. `formatting` therefore covers `whitespace`; enable `whitespace` alone for the narrower reading. +>>>>>>> 503b85f (feat: retain approvals across whitespace-only hunks) #### Require Both Branch Reviewers (Ownership Handoffs) diff --git a/internal/git/normalize.go b/internal/git/normalize.go index 4d5131c..c8dbdde 100644 --- a/internal/git/normalize.go +++ b/internal/git/normalize.go @@ -13,6 +13,8 @@ import ( // flags' noise is stripped. Renames is not a step: it compares what they leave. type normalizer struct { steps []normalizeStep + // Set when a step reads where a line sits; owns a hunk which only moved lines. + layout bool } // normalizeStep rewrites one side of a hunk, its lines joined by newlines. The @@ -26,9 +28,15 @@ func inAnyLanguage(step func(string) string) normalizeStep { func newNormalizer(retention *owners.ApprovalRetention) normalizer { n := normalizer{} + if retention.WhitespaceEnabled() { + n.steps = append(n.steps, inAnyLanguage(collapseWhitespace)) + } if retention.FormattingEnabled() { n.steps = append(n.steps, inAnyLanguage(collapseFormatting)) } + // Formatting rewrites whitespace around the punctuation it drops, so it reads + // where a line sits just as whitespace does. + n.layout = retention.WhitespaceEnabled() || retention.FormattingEnabled() return n } @@ -42,8 +50,13 @@ func (n normalizer) isTrivial(fileName string, hunk *diff.Hunk) bool { if !ok { return false } + // Sides identical before any step ran (a line deleted and added back, or moved) + // changed only where the lines sit, so only a layout step may vouch for it. + if added == removed { + return n.layout + } // Checked before the steps run, since they drop the indentation that changed. - if reindentsBlock(fileName, added, removed) { + if n.layout && reindentsBlock(fileName, added, removed) { return false } return n.normalize(fileName, added) == n.normalize(fileName, removed) diff --git a/internal/git/normalize_whitespace.go b/internal/git/normalize_whitespace.go new file mode 100644 index 0000000..80f7b42 --- /dev/null +++ b/internal/git/normalize_whitespace.go @@ -0,0 +1,11 @@ +package git + +import ( + "strings" +) + +// Collapsed to one space, never deleted: whitespace holds two tokens apart, so +// dropping it would let `foo bar` compare equal to `foobar`. +func collapseWhitespace(block string) string { + return strings.Join(strings.Fields(block), " ") +} diff --git a/internal/git/normalize_whitespace_test.go b/internal/git/normalize_whitespace_test.go new file mode 100644 index 0000000..c1da28a --- /dev/null +++ b/internal/git/normalize_whitespace_test.go @@ -0,0 +1,134 @@ +package git + +import ( + "testing" + + owners "github.com/multimediallc/codeowners-plus/internal/config" +) + +// Hunks the whitespace step is meant to recognize: the two sides hold the same +// tokens in the same order and disagree only about the gaps between them. +var whitespaceOnlyHunks = []hunkCase{ + { + name: "reindented statement", + file: "billing.go", + body: `- total := price * quantity ++ total := price * quantity`, + }, + { + name: "tabs replaced by spaces", + file: "billing.py", + body: "-\ttotal = price * quantity\n+ total = price * quantity", + }, + { + name: "operands realigned", + file: "billing.py", + body: `-alpha = 1 +-beta = 2 ++alpha = 1 ++beta = 2`, + }, + { + name: "trailing whitespace stripped", + file: "cache.py", + body: "- value = lookup(key) \n+ value = lookup(key)", + }, + { + name: "opening brace moved to its own line", + file: "server.go", + body: `-func handle(w Writer) { ++func handle(w Writer) ++{`, + }, + { + name: "argument list wrapped at an existing space", + file: "billing.py", + body: `- result = compute(alpha, beta) ++ result = compute(alpha, ++ beta)`, + }, + { + name: "separating blank line removed", + file: "billing.py", + body: `-alpha = 1 +- ++alpha = 1`, + }, +} + +// Two sides already the same text before any step ran: the lines moved, and only +// a flag which reads where a line sits has looked at anything here. +var identicalSideHunks = []hunkCase{ + { + name: "line deleted and added back", + file: "billing.py", + body: `- total = price * quantity ++ total = price * quantity`, + }, + { + name: "blank line added", + file: "billing.py", + body: `+`, + }, + { + name: "blank line moved", + file: "billing.py", + body: `- ++`, + }, + { + name: "comment line deleted and added back", + file: "connection.js", + body: `- // reuse the pooled connection ++ // reuse the pooled connection`, + }, +} + +// Formatting rewrites the whitespace around the punctuation it drops, so either +// step alone has to leave the whitespace-only hunks trivial. +var whitespaceAwareRetentions = map[string]*owners.ApprovalRetention{ + "whitespace alone": steps(whitespaceOn), + "formatting alone": steps(formattingOn), + "both": steps(whitespaceOn, formattingOn), +} + +func TestWhitespaceRetainsWhitespaceOnlyHunks(t *testing.T) { + for name, retention := range whitespaceAwareRetentions { + t.Run(name, func(t *testing.T) { + assertTrivial(t, newNormalizer(retention), whitespaceOnlyHunks) + }) + } +} + +func TestWhitespaceAloneDismissesEverythingElse(t *testing.T) { + n := newNormalizer(steps(whitespaceOn)) + + assertSignificant(t, n, significantHunks) +} + +// The layout flags' business and only theirs: a flag which rewrites something +// else read nothing here, so it may not hand the approval back. +func TestOnlyLayoutFlagsRetainIdenticalSides(t *testing.T) { + retained := map[string]*owners.ApprovalRetention{ + "whitespace alone": steps(whitespaceOn), + "formatting alone": steps(formattingOn), + "both layout flags": steps(whitespaceOn, formattingOn), + } + for name, retention := range retained { + t.Run(name, func(t *testing.T) { + assertTrivial(t, newNormalizer(retention), identicalSideHunks) + }) + } + + dismissed := map[string]*owners.ApprovalRetention{ + "comments alone": steps(commentsOn), + "string literals alone": steps(stringLiteralsOn), + "renames alone": steps(renamesOn), + "section on, no flag": {Enabled: true}, + } + for name, retention := range dismissed { + t.Run(name, func(t *testing.T) { + assertSignificant(t, newNormalizer(retention), identicalSideHunks) + }) + } +}