feat: match a hunk against its approval-time counterpart

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).

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")
+	}
+}

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)