51.2 代码评审
一句话
代码评审的产出不是“意见数量”，而是在缺陷最便宜的时刻把它拦下来；决定拦截率的是单次评审的尺寸与时长（200–400 行、不超过 60 分钟），不是评审者的资历。
先破除四个误区
| 误区 | 现实 |
|---|---|
| “找最资深的人看，效果最好” | 超过 400 行或 60 分钟后，所有人的检出率都往下掉。尺寸是主因，人选是次因；把 2000 行的 PR 交给架构师，他一样只能看个开头 |
| “评审要全面，什么都该看” | 格式、命名风格、未使用变量、明显 bug 都能被工具判掉。人看这些，等于把最贵的注意力花在最便宜的判断上 |
| “意见越多，评审越认真” | 意见数量与缺陷发现不相关。真正有行动力的是分级后的 blocking 项；其余会变成“已读不回”的噪音 |
| “LGTM 就是走流程” | 评审记录是责任边界与知识传递的载体：三个月后查“这个设计谁同意的”，只有评审记录能回答 |
数字：尺寸决定检出率
业界被引用最多的一组数据来自 Cisco 对 2500 次评审的统计（SmartBear 汇总，Jason Cohen《Best Kept Secrets of Peer Code Review》，2006）。下面是区间，不是定律：
| 单次评审规模 | 建议时长 | 缺陷检出率（研究区间） | 实际发生了什么 |
|---|---|---|---|
| ≤ 200 行 | 20–30 分钟 | 高 | 能逐行看懂，能提出设计级意见 |
| 200–400 行 | ≤ 60 分钟 | 70–90% | 推荐上限：速度 300–500 行/小时仍可持续 |
| 400–800 行 | 60–90 分钟 | 明显下降 | 只看开头和结尾，中间扫过 |
| > 1000 行 | 数小时 | 漏检为主 | 变成盖章：反正看不完，不如相信作者 |
三条推论（可自己验，不需要信论文）：
- “一天只能认真评 400 行”是硬约束。评审是消耗注意力的劳动，不是可以并行的 IO。超了，后来的 PR 拿到的其实是“盖章”。
- PR 越大，单位行成本越高，因为大 PR 必然导致评审被拆成多次、每次都要重新建立上下文。
- “小 PR”不是礼貌，是吞吐：把 1000 行拆成 5 个 200 行，评审总量没变，但每一段都落在“能看懂”的区间里，且能并行派给不同的人。
评审队列的账：拖住你的是利用率 ρ
评审是一个排队系统：PR 按 λ 到达，评审者按 μ 消化。用 M/M/1 估算（λ、μ 都是每小时 PR 数）：
package main
import "fmt"
// M/M/1 评审队列估算。cap = 团队每小时能评审的代码行数（人数 × 400 行/小时）。
// μ = cap / PR 行数；ρ = λ/μ。ρ 越接近 1，等待时间按 1/(1-ρ) 放大。
func wait(lam, size, cap float64) (rho, queueMin, leadMin, humanMin float64) {
mu := cap / size
rho = lam / mu
if rho >= 1 {
return rho, -1, -1, 0 // 队列发散：积压只会越来越长
}
service := 60 / mu // 单个 PR 的评审时间（分钟）
// 人力投入 = 行数 × 评审人数 × 60 / 400
return rho, rho / (1 - rho) * service, service / (1 - rho), size * cap * 60 / (400 * 400)
}
func main() {
const lam = 1.28 // PR/小时
fmt.Printf("%-24s %5s %10s %12s %12s\n", "场景", "ρ", "排队等待", "端到端停留", "人力/PR")
cases := []struct{ name string; size, cap float64 }{
{"250 行 · 1 人评审", 250, 400},
{"125 行 · 1 人评审", 125, 400},
{"62 行 · 1 人评审", 62, 400},
{"250 行 · 2 人评审", 250, 800},
}
for _, c := range cases {
rho, q, lead, h := wait(lam, c.size, c.cap)
fmt.Printf("%-24s %5.2f %8.1f 分 %10.1f 分 %10.1f 分\n", c.name, rho, q, lead, h)
}
}
输出（本机实测，1.28 PR/小时 ≈ 每天 10 个 PR、8 小时工作制）：
场景 ρ 排队等待 端到端停留 人力/PR
250 行 · 1 人评审 0.80 150.0 分 187.5 分 37.5 分
125 行 · 1 人评审 0.40 12.5 分 31.2 分 18.8 分
62 行 · 1 人评审 0.20 2.3 分 11.6 分 9.3 分
250 行 · 2 人评审 0.40 12.5 分 31.2 分 75.0 分
三条结论：
- 把 250 行拆成两个 125 行，排队等待从 150 分钟降到 12.5 分钟（12 倍），代价是拆 PR 的十分钟。ρ 从 0.80（拐点）降到 0.40（安全区），这是延迟改善的真正来源。
- 加一个评审人，延迟改善一模一样（12.5 分钟），但人力翻倍。 250 行 2 人评审要 75 人分钟；同样 250 行拆成两半、各 1 人评审只要 37.5 人分钟——同样的延迟，一半的人力，且小 PR 的检出率更高（见上面的尺寸表）。
- ρ ≥ 0.9 之后等待爆炸：1 人每小时 1.5 个 PR 时，端到端停留 600 分钟——承诺“24 小时内评审”其实已经做不到，只能靠加班或盖章兑现。
🔑 评审改进的顺序：先拆小 PR，再谈 SLA，最后才加人。加人买到的是吞吐，拆 PR 买到的是“更低的 ρ + 更高检出率 + 一半人力”，两笔账不划算的地方只在拆不动的时候。
二次评审只看增量：git range-diff
第一轮评审后，作者按意见改了三个提交。第二轮如果再从头看整个 diff，等于把评审成本翻倍，而且看的是已经看过的东西。正确工具是 git range-diff：它比较“两个版本区间”，只显示两轮之间变了什么。
$ git range-diff main..$OLD main..HEAD
1: b29d5e9 ! 1: b1c2ce8 feat(order): 实现 Total
@@ order.go: type Item struct {
func Total(items []Item) int {
- return 0
++ if items == nil {
++ return 0
++ }
+ sum := 0
+ for _, it := range items {
+ sum += it.Price * it.Qty
2: c450cb4 = 2: b6b72a6 test(order): 覆盖 Total 基础用例
-: ------- > 3: abe9a01 test(order): 补空输入用例
读法：
| 标记 | 含义 | 评审者要做什么 |
|---|---|---|
= | 提交内容没变（哈希可能因 rebase 变了） | 跳过，不用重看 |
! | 提交变了 | 只看 ++ / -- 行，即相对上一轮新增/删除的行 |
-: ------- | 上一轮没有这个提交 | 新提交，需要通读 |
-: 后面直接是提交 | 这一轮被删掉了 | 确认删除是故意的 |
配套操作是 git commit --fixup=<某提交> + git rebase -i --autosquash，把修改折叠回原提交，让历史保持“一个逻辑改动一个提交”。
⚠️ 但这里有个极易踩的坑：
git rebase -i --autosquash <上一轮评审的 tip>是不会折叠 fixup 的——因为被 fixup 的那个提交不在 rebase 范围内（它在 tip 之前）。必须从上游（main或分支基点）开始 rebase，让目标提交进入 todo 列表。本机实测，两种写法结果完全不同：
$ GIT_SEQUENCE_EDITOR=true git rebase -i --autosquash $OLD # OLD = 上一轮评审的 tip
$ git log --oneline
c666aa9 test(order): 补空输入用例
6411e87 fixup! feat(order): 实现 Total # ← fixup 原样留着，评审者会看到一条垃圾提交
c450cb4 test(order): 覆盖 Total 基础用例
b29d5e9 feat(order): 实现 Total
$ GIT_SEQUENCE_EDITOR=true git rebase -i --autosquash main # 从上游开始
$ git log --oneline
abe9a01 test(order): 补空输入用例
b6b72a6 test(order): 覆盖 Total 基础用例
b1c2ce8 feat(order): 实现 Total # ← 已折叠，历史干净
原因一句话：autosquash 只能在“被 fixup 的提交也在本次 rebase 范围内”时生效。把 rebase.autosquash=true 配好，再把“从 main 起 rebase”写进团队脚本，否则每次都要手工整理。
评审意见分三级，每级有明确的动作
| 级别 | 写法 | 语义 | 作者动作 | 阻塞合并？ |
|---|---|---|---|---|
| blocking | blocking: 这里 panic 会冒到 HTTP handler | 正确性/安全/数据损坏 | 必须改，改完再评 | 是 |
| suggestion | suggestion: 这里可以用 errors.Is，判断更稳 | 更好的写法，非必须 | 采纳或说明理由 | 否 |
| nit | nit: 变量名拼错了 | 琐碎、风格 | 顺手改或不改 | 否 |
两条纪律：
- blocking 必须给判据或替代方案。“这里不太好”不是 blocking，是抱怨；作者无法据此行动。
- 结论只有三种：Approve / Approve with nits（自己改掉即可合）/ Request changes（对应 GitHub 的三种 review 状态）。沉默的 LGTM 不算通过，因为它无法区分“看过了”和“没看”。
人看什么、机器看什么
| 交给机器（自动化） | 只能由人判断 |
|---|---|
| 格式、import 排序、lint、未使用变量 | 设计是否解决了对的问题（需求的正确性） |
| 编译、类型检查、单元测试 | 边界条件：空、超时、并发、部分失败 |
| 覆盖率阈值（只看“有没有测”） | 测试是否测到点上（覆盖率高但断言空转是常态） |
| 依赖漏洞、SAST、密钥扫描 | 错误处理与可运维性：日志够不够定位、指标有没有埋 |
| 提交信息格式、PR 模板字段 | 命名与抽象的语义：这个名字说的是不是这件事 |
| 破坏性变更是否被识别（见 51.3 语义化版本与变更日志） |
🔑 一条硬规则：能在 5 秒内被工具判定的东西，不要出现在人的评审里。每出现一次，都是在花掉一条“人愿意认真读”的额度。工具侧的落点见 52.2 静态检查。
谁能批什么：属主比“随机抽人”可靠
| 分配策略 | 优点 | 风险 |
|---|---|---|
| 作者自选 | 快，能找到懂的人 | 同温层：总找熟人，通过率虚高，盲区固化 |
| 轮值 | 公平，扩散知识 | 抽到不懂的，评审退化成“看起来没问题” |
| 代码属主（CODEOWNERS） | 该看的人一定被拉进来，责任清晰 | 属主成为瓶颈，要配“至少 1 名属主 + 1 名同组” |
.github/CODEOWNERS 的关键语义（GitHub 文档定义，本机未部署验证）：最后匹配的那条生效，* 是兜底规则，因此把“兜底”写在文件最前面：
# 兜底：任何改动都要有组内一人看过
* @org/backend
# 支付与计费：必须由支付组 + 财务系统组共同批
/services/payment/ @org/payment @org/billing
# 数据库迁移：DBA 必看，避免上线时锁表
/migrations/ @org/dba
分支保护应至少要求：1 名属主批准 + CI 全绿 + 对话全部解决。三个条件缺一个，评审就会在压力下退化成“点一下 Approve”。
落地：指名 / 可抄 / 可验 / 可退
指名：代码托管侧用 GitHub Pull Request（review 状态 + CODEOWNERS + branch protection）或 GitLab Merge Request（approval rules）；超大团队/强流程场景用 Gerrit（Change-Id + 单提交 + 强制 rebase，代价是学习曲线）。二次评审用 git range-diff（git ≥ 2.19）。工具侧格式与静态检查见 52.2 静态检查。
可抄：
# ① 让 fixup 自动折叠，且从上游开始 rebase（写进脚本，别靠记忆）
git config --global rebase.autosquash true
git commit -a --fixup=<目标提交>
GIT_SEQUENCE_EDITOR=true git rebase -i --autosquash origin/main
# ② 第二轮评审只看增量
OLD=$(git rev-parse origin/feat/order-total) # 评审第一轮时的 tip
git push --force-with-lease
git range-diff origin/main..$OLD origin/main..HEAD
# ③ 评审前先看规模，超过 400 行就拆
git diff --stat origin/main...HEAD
git diff --numstat origin/main...HEAD | awk '{a+=$1; d+=$2} END {printf "改动 %d 行（+%d/-%d）\n", a+d, a, d}'
可验（本机真实输出）：range-diff 必须出现 ! 与新提交行；折叠后的 git log 里不应再有 fixup! 字样。两条断言任一失败，说明评审者看到的历史和作者以为的不一样——这正是二次评审最容易浪费半小时的地方。
$ git log --oneline | grep -c 'fixup!' # 应为 0
0
$ git range-diff origin/main..$OLD origin/main..HEAD | head -1
1: b29d5e9 ! 1: b1c2ce8 feat(order): 实现 Total
可退：紧急修复需要绕过评审时，用 break-glass 通道（admin merge / 紧急 approve 权限），但必须绑两件事：① 合并时打标签（emergency）并在 24 小时内补审；② 事后在周会公示“本月 break-glass 次数”。没有公示的紧急通道，等于没有评审——因为绕过没有代价。
反模式
| 反模式 | 后果 |
|---|---|
| “大 PR 一次评完”（> 1000 行） | 检出率崩到只看开头，等于盖章；出问题后无人能说“我审过” |
| 评审里讨论格式/命名风格 | 花掉注意力额度，真正的边界问题被淹没 |
| 意见不分级、不给判据 | 作者无法判断哪些必须改，来回拉扯，PR 寿命拉长 |
| 作者自选评审人且总是同一人 | 盲区固化，通过率虚高，知识不扩散 |
| 二次评审重看整个 diff | 评审成本翻倍，且容易漏掉“这一轮到底改了什么” |
| LGTM 无评论无结论状态 | 责任边界模糊；事后无法区分“看过”与“没看” |
| 没有 break-glass，逼人违规合并 | 压力下直接 push 主干，评审记录出现空洞 |
| 只要求“CI 绿”不要求人批 | 自动化能挡语法与测试，挡不住设计与边界 |
自检
Q1 一个 1200 行的 PR，作者说“功能是一体的，拆不开”。你作为评审者，第一件事做什么？
▸ 答案：不接受“拆不开”这个结论，而是按“可独立验证的最小单元”重切：① 纯重构/移动文件先单独提一个 PR（行为不变，几乎不用细看）；② 新接口 + 实现 + 调用方按“先加不删”拆成 2–3 个（每个都能独立编译、独立测试）；③ 测试与实现可以同 PR，但要把“测试证明什么”写在描述里。判据不是“行数”，而是每个 PR 能否被单独回滚而不破坏主干。实在拆不开（比如一次性生成的大文件、迁移脚本），就用“逐段评审 + 明确标注未看部分”，绝不假装看完了。
Q2 评审队列模型里，PR 从 250 行砍到 125 行，等待从 150 分钟降到 12.5 分钟；而加一个评审人得到的延迟完全相同（12.5 分钟）。既然效果一样，为什么优先拆小 PR？
▸ 答案：因为两者的成本结构和副作用不同。① 人力：250 行 2 人评审 = 75 人分钟；250 行拆成两个 125 行、各 1 人评审 = 37.5 人分钟，一半。② 检出率：尺寸表里 200–400 行是检出率仍高的区间，>400 行开始掉；两个人各看 250 行，看到的是同一份超载内容，漏检不会因为人多而减少。③ ρ 的改善机制相同（都让 μ 翻倍），所以延迟一样——但拆 PR 还顺带缩短了分支寿命与回滚粒度（见 51.1 分支策略）。加人只在拆不动（生成代码、大迁移）或需要不同视角（安全 + 领域）时才划算。
Q3 第二轮评审时，作者的 git log 里有一条 fixup! feat(order): 实现 Total。这说明什么？评审者该怎么办？
▸ 答案：说明作者用了 --fixup 但 rebase 时没有让目标提交进入范围（典型错误：从上一轮 tip 起 rebase，而不是从 main 起），或者根本没做 autosquash。后果是：历史里多一条无意义提交，合入主干后 git log 失真，回滚粒度变糊。评审者应要求作者从上游重做 git rebase -i --autosquash origin/main，再用 git range-diff 确认两轮差异只剩预期改动——在历史整理好之前不做实质评审，否则评审的是将被丢弃的中间态。
Q4 为什么说“评审里讨论变量名拼写”是亏的？如果团队就是很在意拼写怎么办？
▸ 答案：亏在机会成本：一次评审的注意力额度约等于“200–400 行、60 分钟”，讨论拼写占掉的是这段额度里最贵的部分，而它本可以由 linter/拼写检查器在 5 秒内判定并自动修复。团队在意拼写没错，错在让它占用人的通道——把规则写进工具（golangci-lint 的 misspell、typos、CI 失败即阻断），既做到了 100% 覆盖，又不用消耗评审注意力。规则要么自动化，要么被遗忘；靠人记的规则，覆盖率随疲劳度下降。
Q5 团队评审 SLA 写的是“24 小时内响应”，但实测端到端停留 600 分钟（10 小时）。问题出在哪？
▸ 答案：SLA 是承诺，排队模型是现实。600 分钟的停留意味着 ρ≈0.94：到达率已经贴近评审吞吐，队列在任何波动下都会涨。修法有优先级：① 降批量（PR 减半 → 服务时间减半，ρ 直接掉到 0.5 以下）；② 降在途 WIP（限制每人同时 1 个待评审 PR，见 51.1 分支策略）；③ 才考虑加人或放宽 SLA。只改 SLA 数字不改 ρ，等于把违约写进文档。
关联
- 前置：51.1 分支策略、40.1 可测试性
- 并列：51.3 语义化版本与变更日志、53.2 单元测试
- 下游：51.4 单体仓库 vs 多仓库
- 支撑：55.1 发布策略