From 58d3d305efca6cec0daf2890e2441a39042bf22c Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Fri, 21 Aug 2026 13:32:57 -0700 Subject: [PATCH] feat: match a hunk against its approval-time counterpart changesSince subtracts the approval-time diff from the current one on the raw hunk body. Both diffs are taken from the merge base, so an edit that shifts a hunk's boundaries re-anchors it over lines the reviewer already approved: the hunk then differs byte for byte from its counterpart and survives. isTrivial cannot recover that case. It asks whether a hunk is trivial, and a hunk spanning a whole new function is not, however little of it changed since the approval. The question worth asking is whether the hunk is trivially *different from what was approved*, which needs both sides. So build a second lookup keyed on the normalized body of each approval-time hunk, and drop a current hunk that matches one. The raw hash still decides first, so this only ever removes hunks that were previously kept. Deliberately narrower than the raw hash in two ways. The file name is part of the key, because a normalized body says less than a raw one and identity should not extend across files. And no enabled steps means no key at all: the raw hash reads the two sides interleaved, so a key built from them separately is looser, and de-duplication must not change for anyone who has not enabled a flag. --- internal/git/diff.go | 25 +++- internal/git/diff_approval_key_test.go | 156 +++++++++++++++++++++++++ internal/git/normalize.go | 17 +++ 3 files changed, 192 insertions(+), 6 deletions(-) create mode 100644 internal/git/diff_approval_key_test.go diff --git a/internal/git/diff.go b/internal/git/diff.go index 78c07b3..85cbe53 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -180,9 +180,14 @@ func changesSince(context changesSinceContext) ([]codeowners.DiffFile, error) { // For each file, filter out hunks that are in oldDiff // if len(hunks) > 0, add to diffFiles oldHunkHashes := make(map[[32]byte]bool) + oldApprovalKeys := make(map[string]bool) for _, d := range context.olderDiff { + fileName := diffToFilename(d) for _, h := range d.Hunks { oldHunkHashes[hunkHash(h)] = true + if key, ok := context.normalizer.approvalKey(fileName, h); ok { + oldApprovalKeys[key] = true + } } } @@ -196,15 +201,23 @@ func changesSince(context changesSinceContext) ([]codeowners.DiffFile, error) { Hunks: make([]codeowners.HunkRange, 0, len(d.Hunks)), } for _, hunk := range d.Hunks { + if oldHunkHashes[hunkHash(hunk)] { + continue + } + // The raw hash decided first, so this only ever removes a hunk that + // would otherwise have been kept. + if key, ok := context.normalizer.approvalKey(fileName, hunk); ok && oldApprovalKeys[key] { + continue + } // 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), - } - newDiffFile.Hunks = append(newDiffFile.Hunks, newHunkRange) + if context.normalizer.isTrivial(fileName, hunk) { + continue } + newDiffFile.Hunks = append(newDiffFile.Hunks, codeowners.HunkRange{ + Start: int(hunk.NewStartLine), + End: int(hunk.NewStartLine + hunk.NewLines - 1), + }) } // Binary files have no hunks; staleness is intentionally not tracked // for them (there is no hunk content to hash against the older diff). diff --git a/internal/git/diff_approval_key_test.go b/internal/git/diff_approval_key_test.go new file mode 100644 index 0000000..ea3f708 --- /dev/null +++ b/internal/git/diff_approval_key_test.go @@ -0,0 +1,156 @@ +package git + +import ( + "testing" + + owners "github.com/multimediallc/codeowners-plus/internal/config" + "github.com/sourcegraph/go-diff/diff" +) + +func parseDiffOrFail(t *testing.T, text string) []*diff.FileDiff { + t.Helper() + parsed, err := diff.ParseMultiFileDiff([]byte(text)) + if err != nil { + t.Fatalf("parsing diff: %v", err) + } + return parsed +} + +// Every flag is set explicitly so a default-on one cannot ride along unasked. +func retention(formatting bool) *owners.ApprovalRetention { + no, yes := false, true + f := &no + if formatting { + f = &yes + } + return &owners.ApprovalRetention{ + Enabled: true, Whitespace: &no, Comments: &no, + Formatting: f, StringLiterals: &no, Renames: &no, + } +} + +// An edit that shifts a hunk's boundaries re-anchors it over already-approved +// lines, so the hunk survives the raw hash and is too substantial to be trivial. +func TestChangesSinceMatchesShiftedHunkAgainstApproval(t *testing.T) { + // A whole new function, then the call rewrapped: the only change since approval. + const approved = `diff --git a/svc.go b/svc.go +--- a/svc.go ++++ b/svc.go +@@ -10,0 +11,3 @@ ++func Handle(r *Request) error { ++ return dispatch(r, opts, deadline) ++}` + const current = `diff --git a/svc.go b/svc.go +--- a/svc.go ++++ b/svc.go +@@ -10,0 +11,7 @@ ++func Handle(r *Request) error { ++ return dispatch( ++ r, ++ opts, ++ deadline, ++ ) ++}` + + tt := []struct { + name string + retention *owners.ApprovalRetention + wantFiles int + }{ + {"formatting enabled: nothing new to review", retention(true), 0}, + {"formatting disabled: the hunk stands", retention(false), 1}, + {"retention off entirely: the hunk stands", nil, 1}, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + got, err := changesSince(changesSinceContext{ + newerDiff: parseDiffOrFail(t, current), + olderDiff: parseDiffOrFail(t, approved), + normalizer: newNormalizer(tc.retention), + }) + if err != nil { + t.Fatalf("changesSince: %v", err) + } + if len(got) != tc.wantFiles { + t.Errorf("got %d changed files, want %d", len(got), tc.wantFiles) + } + }) + } +} + +// A hunk carrying real change survives however aggressively it is normalized. +func TestChangesSinceKeepsRealChangeAgainstApproval(t *testing.T) { + const approved = `diff --git a/svc.go b/svc.go +--- a/svc.go ++++ b/svc.go +@@ -10,0 +11,3 @@ ++func Handle(r *Request) error { ++ return dispatch(r) ++}` + const current = `diff --git a/svc.go b/svc.go +--- a/svc.go ++++ b/svc.go +@@ -10,0 +11,3 @@ ++func Handle(r *Request) error { ++ return dispatchAsync(r) ++}` + + got, err := changesSince(changesSinceContext{ + newerDiff: parseDiffOrFail(t, current), + olderDiff: parseDiffOrFail(t, approved), + normalizer: newNormalizer(retention(true)), + }) + if err != nil { + t.Fatalf("changesSince: %v", err) + } + if len(got) != 1 { + t.Fatalf("a changed call target must still be reviewable, got %d files", len(got)) + } +} + +// The same text arriving in a second file is new code there. +func TestApprovalKeyIsPerFile(t *testing.T) { + const approved = `diff --git a/one.go b/one.go +--- a/one.go ++++ b/one.go +@@ -1,0 +2 @@ ++ audit(user)` + const current = `diff --git a/two.go b/two.go +--- a/two.go ++++ b/two.go +@@ -1,0 +2 @@ ++ audit(user)` + + got, err := changesSince(changesSinceContext{ + newerDiff: parseDiffOrFail(t, current), + olderDiff: parseDiffOrFail(t, approved), + normalizer: newNormalizer(retention(true)), + }) + if err != nil { + t.Fatalf("changesSince: %v", err) + } + if len(got) != 1 { + t.Fatalf("the same line in a different file is new there, got %d files", len(got)) + } +} + +// The raw hash reads the two sides interleaved, so a key built from them +// separately is looser and must not exist at all while every flag is off. +func TestNoApprovalKeyWhenNoFlagsEnabled(t *testing.T) { + hunk := &diff.Hunk{Body: []byte("+foo()\n-bar()")} + swapped := &diff.Hunk{Body: []byte("-bar()\n+foo()")} + + if hunkHash(hunk) == hunkHash(swapped) { + t.Fatal("hunkHash is expected to read the two sides interleaved") + } + for _, r := range []*owners.ApprovalRetention{nil, {Enabled: false}, retention(false)} { + n := newNormalizer(r) + if _, ok := n.approvalKey("x.go", hunk); ok { + t.Errorf("approvalKey offered a key with no steps enabled: %+v", r) + } + } + if _, ok := newNormalizer(retention(true)).approvalKey("x.go", hunk); !ok { + t.Error("approvalKey withheld a key with a step enabled") + } +} diff --git a/internal/git/normalize.go b/internal/git/normalize.go index 4d5131c..631760d 100644 --- a/internal/git/normalize.go +++ b/internal/git/normalize.go @@ -49,6 +49,23 @@ func (n normalizer) isTrivial(fileName string, hunk *diff.Hunk) bool { return n.normalize(fileName, added) == n.normalize(fileName, removed) } +// approvalKey identifies a hunk by what it says once the enabled flags' noise is +// stripped, so it matches an approval-time counterpart whose bytes differ. +func (n normalizer) approvalKey(fileName string, hunk *diff.Hunk) (string, bool) { + // No key at all, so de-duplication is unchanged while every flag is off. + if len(n.steps) == 0 { + return "", false + } + added, removed, ok := hunkBlocks(hunk.Body) + if !ok { + return "", false + } + // The file name joins the key, unlike in the raw hash: a normalized body says + // less than a raw one, so identity should not reach across files as far. + return fileName + "\x00" + n.normalize(fileName, added) + + "\x00" + n.normalize(fileName, removed), true +} + func (n normalizer) normalize(fileName, block string) string { for _, step := range n.steps { block = step(fileName, block)