feat: fetch a force-pushed approval commit instead of dismissing blind - #185
feat: fetch a force-pushed approval commit instead of dismissing blind#185asyncawaitpromise wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an 'Approval Retention' feature, allowing configuration to retain existing approvals across specific types of changes (e.g., whitespace, comments, formatting, string literals, renames) and enabling the fetching of orphaned approval commits from the remote repository. The implementation includes updates to the configuration structure, logic to handle git diff options, and comprehensive testing for both the configuration and the new git fetching capabilities. My feedback addresses an issue in the error handling of the fetchRef function, where discarding the command output makes debugging git failures difficult; I have provided a suggestion to include the command output in the returned error.
b313dc9 to
86cc504
Compare
626ea8c to
56807e0
Compare
56807e0 to
eeee60e
Compare
21d8f62 to
5e49d56
Compare
When a branch is rebased or force-pushed, the commit an approval points at stops being reachable from any local ref. ChangesSince then fails on `git diff base...<approvalSHA>`, and the approval lands in badApprovals with no ownership check at all. That dismissal is not "your files changed", it is "I could not tell, so I reset you". GitHub still serves the orphaned object. With `fetch_orphaned_approval` on, a ref which cannot be resolved locally is fetched from origin once and the diff retried, so the approval is judged on its diff rather than lost to a rewritten branch. Fail-safe: if the fetch or the retry fails, the original diff error stays the cause and the approval is dismissed exactly as before. Hardening: - The ref is probed with `cat-file` first, so a diff which failed for some other reason does not spend a remote round trip that cannot help. - The fetch is bounded at 60s. It is the only git call here that waits on a remote; every other one is local and returns promptly. - The ref is passed after a `--` so a ref beginning with a dash cannot be read as an option. `git fetch` accepts --upload-pack, which names a command to run. - The original diff error is preserved as the wrapped cause, with any fetch or retry failure appended. Opt-in only, and default off so that enabling it is always a deliberate choice to add network calls to a run. Coverage badge regenerated.
5e49d56 to
5a26683
Compare
|
Codeowners approval required for this PR: |
Review catch: fetchRef discarded CombinedOutput and returned only the error, so a failed fetch reached the caller as a bare "exit status 128" with nothing about why. getGitDiff already wraps its output, so this was inconsistent with the rest of the file. The scripted test executor had the same bug, returning nil output alongside an error, which is why nothing caught this. It now returns both, the way CombinedOutput does.
Review catch, and the same defect the hunk-filter runner already guards against. exec.CommandContext SIGKILLs git, but `git fetch` spawns git-remote-https, which inherits the CombinedOutput pipe, and Wait blocks until every writer closes. So against an unresponsive remote the call sat there long past the deadline while the error claimed it had timed out. cmd.WaitDelay bounds the wait for the helper to go away. The ref is also now required to look like an object name before it reaches the network. `--` already stopped a dash-leading ref being read as an option, but it does not stop refspec parsing, and `refs/heads/main:refs/heads/injected` created a local ref. The only caller passes a GitHub-issued SHA, so this is defence in depth, and it closes the empty-ref case for free: git reads an empty ref as HEAD, which would have diffed base...HEAD and let an approval pass ownership on the wrong comparison. Test refs are object names now, since that is what production passes, plus rows for a refspec-shaped ref and an empty one. README: `fetch_orphaned_approval` was described as the only setting that adds a network call, which is wrong because self_approval_via_teams fetches team members, and the 60s bound was not true until the WaitDelay fix above.
main gained GitHub Enterprise support (#199) and the v1.11.0 release (#201) while this branch was open. Resolved: - README coverage badge: regenerated from the merged tree; covbadge.yml fails CI on a stale number, so neither side's value would have done. - orphaned_approval_test.go: gh.NewClient took a fourth argument in #199. This branch adds a caller main never saw, so it merged clean and broke the build.
main gained the hunk filters (#200), the urfave/cli bump (#193) and the codeowners.toml parse fix (#184) while this branch was open. Resolved: - README coverage badge: regenerated from the merged tree; covbadge.yml fails CI on a stale number, so neither side's value would have done. - config.go, app.go: this branch and #200 each added a config field and a GitDiff option in the same place. Both are kept. - diff.go: both branches independently added the same functional-options pattern and declared DiffOption identically. One copy is kept, in the position #200 put it, so the file matches what later branches fork from.
|
Codeowners approval required for this PR: |
Summary / Background
When a branch is rebased or force-pushed, the commit an approval points at stops
being reachable from any local ref.
ChangesSincethen fails ongit diff base...<approvalSHA>, and the approval lands inbadApprovalswithno ownership check at all.
That dismissal is not "your files changed". It is "I could not tell, so I reset
you".
The fix
GitHub still serves the orphaned object. With
fetch_orphaned_approvalon, a refwhich cannot be resolved locally is fetched from
originonce and the diffretried, so the approval is judged on its diff rather than lost to a rewritten
branch.
Fail-safe
If the fetch or the retry fails, the original diff error stays the cause and the
approval is dismissed exactly as before. Nothing gets less safe.
Hardening
remote; every other one is local and returns promptly.
--so a ref beginning with a dash cannot be read asan option.
git fetchaccepts--upload-pack, which names a command to run.retry failure appended.
Flag
Opt-in, default off. It is the only setting in
codeowners.tomlwhich adds anetwork call to a run, so enabling it should always be a deliberate choice rather
than something inherited.
Verification
An end-to-end test builds a repository whose approved commit exists only on the
remote, asserts the commit genuinely does not resolve locally (otherwise both
cases would pass for the wrong reason), and checks that the approval is dismissed
with the flag off and recovered with it on.