feat: match a hunk against its approval-time counterpart - #198
feat: match a hunk against its approval-time counterpart#198asyncawaitpromise wants to merge 1 commit into
Conversation
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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an approval key mechanism to track and match shifted or normalized hunks against previous approvals, preventing unnecessary re-reviews. It includes changes to hunk filtering logic in internal/git/diff.go, the addition of the approvalKey method in internal/git/normalize.go, and comprehensive unit tests. The review feedback suggests adding defensive nil checks for diff files and hunks across these files to prevent potential nil pointer dereference panics.
| oldHunkHashes := make(map[[32]byte]bool) | ||
| oldApprovalKeys := make(map[string]bool) | ||
| for _, d := range context.olderDiff { | ||
| fileName := diffToFilename(d) |
There was a problem hiding this comment.
To prevent a potential nil pointer dereference panic, we should defensively check if d is nil before calling diffToFilename(d).
if d == nil {
continue
}
fileName := diffToFilename(d)| Hunks: make([]codeowners.HunkRange, 0, len(d.Hunks)), | ||
| } | ||
| for _, hunk := range d.Hunks { | ||
| if oldHunkHashes[hunkHash(hunk)] { |
There was a problem hiding this comment.
To prevent a potential nil pointer dereference panic, we should defensively check if hunk is nil before passing it to hunkHash.
if hunk == nil {
continue
}
if oldHunkHashes[hunkHash(hunk)] {| if len(n.steps) == 0 { | ||
| return "", false | ||
| } | ||
| added, removed, ok := hunkBlocks(hunk.Body) |
There was a problem hiding this comment.
To prevent a potential nil pointer dereference panic, we should defensively check if hunk is nil before accessing its fields or passing it to hunkBlocks.
| added, removed, ok := hunkBlocks(hunk.Body) | |
| if hunk == nil { | |
| return "", false | |
| } | |
| added, removed, ok := hunkBlocks(hunk.Body) |
|
Closing this one. Deciding what counts as a change not worth re-reviewing turns out to be a judgement about a particular codebase rather than something this action should carry a default for. The normalizers here encode one set of conventions, and every consumer would inherit them along with the language-awareness they need to stay correct. Replacing it with a seam instead: the action hands out the hunks it is about to attribute to an approval, and a program named by the workflow answers which of them a reviewer has effectively already seen. Same outcome where somebody wants it, no opinion shipped by default. That is open as its own PR. No behaviour in this repo changes as a result of this close. |
1 participant
Related PR(s)
Depends on #186. It sits beside #187, #188 and #189 rather than on top of them, because it changes the subtraction rather than any one flag, so it composes with whichever of those land.
Summary / Background
changesSincetakes two three-dot diffs from the same merge base and subtracts one from the other by hunk hash. Both are anchored at the base, so an edit that shifts a hunk's boundaries re-anchors that hunk over lines the reviewer already approved, and it survives because its bytes no longer match.isTrivialcannot recover that case. Say a reviewer approves a new 30 line function and the author then adds one comment inside it. The surviving hunk is the whole function, and asked "is this trivial?", the honest answer is no, it is thirty lines of new code. So we dismiss over one comment.The question worth asking is not is this hunk trivial but is this hunk trivially different from what was approved, and only comparing the two sides can answer that.
The practical effect today is that a flag only fires when the trivial edit lands on a line the PR had not already touched. Tidying up code you added earlier in the same branch still dismisses, which is the common case.
Code Changes
Build a second lookup beside the raw hashes, keyed on each approval-time hunk's body after the enabled flags' noise is stripped, and drop a current hunk that matches one.
The raw hash still decides first, so this can only ever remove hunks that were previously kept. It never keeps a hunk that was previously removed.
Two places where the key is deliberately narrower than the raw hash:
hunkHashreads the added and removed lines interleaved, so a key built from the two sides separately is looser even with zero normalization. Gating on the step list keeps de-duplication exactly as it is for anyone who has not turned a flag on. There is a test asserting that.Tests cover the boundary-shift case with the flag on and off, a changed call target still surfacing however aggressively it is normalized, the same line in a different file, and the no-key-when-off rule.
One thing this makes more reachable
Flattening nested SCSS selectors normalizes equal, because braces are dropped and whitespace collapses. That happens to be a correct CSS equivalence for a plain de-nest, but there is no CSS model behind it, so one changing specificity or involving
&would normalize equal just as readily.That is not introduced here, and the shape is rare enough that it is unlikely to come up. It is a lot more reachable with this than without it, and I would rather name it than have it found later.