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)